On Tuesday 23 of July 2013 10:37:11 Greg KH wrote:
> On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > > Ick, no. Why can't you just pass the pointer to the phy itself? If
> > > you
> > > had a "priv" pointer to search from, then you could have just passed
> > > the
> > > original phy pointer in the first place, right?
> >
> > IMHO it would be better if you provided some code example, but let's
> > try to check if I understood you correctly.
>
> It's not my code that I want to have added, so I don't have to write
> examples, I just get to complain about the existing stuff :)
Still, I think that some small code snippets illustrating the idea are
really helpful.
> > 8><--------------------------------------------------------------------
> > ----
> >
> > [Board file]
> >
> > static struct phy my_phy;
> >
> > static struct platform_device phy_pdev = {
> >
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > static struct platform_device phy_pdev = {
> >
> > /* ... */
> > .platform_data = &my_phy;
> > /* ... */
> >
> > };
> >
> > [Provider driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_create(phy);
> >
> > [Consumer driver]
> >
> > struct phy *phy = pdev->dev.platform_data;
> >
> > ret = phy_get(&pdev->dev, phy);
> >
> > -----------------------------------------------------------------------
> > -><8
> >
> > Is this what you mean?
>
> No. Well, kind of. What's wrong with using the platform data structure
> unique to the board to have the pointer?
>
> For example (just randomly picking one), the ata-pxa driver would change
> include/linux/platform_data/ata-pxa.h to have a phy pointer in it:
>
> struct phy;
>
> struct pata_pxa_pdata {
> /* PXA DMA DREQ<0:2> pin */
> uint32_t dma_dreq;
> /* Register shift */
> uint32_t reg_shift;
> /* IRQ flags */
> uint32_t irq_flags;
> /* PHY */
> struct phy *phy;
> };
>
> Then, when you create the platform, set the phy* pointer with a call to
> phy_create(). Then you can use that pointer wherever that plaform data
> is available (i.e. whereever platform_data is at).
Hmm? So, do you suggest to call phy_create() from board file? What phy_ops
struct and other hardware parameters would it take?
> > > The issue is that a string "name" is not going to scale at all, as it
> > > requires hard-coded information that will change over time (as the
> > > existing clock interface is already showing.)
> >
> > I fully agree that a simple, single string will not scale even in some,
> > not so uncommon cases, but there is already a lot of existing lookup
> > solutions over the kernel and so there is no point in introducing
> > another one.
> I'm trying to get _rid_ of lookup "solutions" and just use a real
> pointer, as you should. I'll go tackle those other ones after this one
> is taken care of, to show how the others should be handled as well.
There was a reason for introducing lookup solutions. The reason was that in
board file there is no way to get a pointer to something that is going to be
created much later in time. We don't do time travel ;-).
> > > Please just pass the real "phy" pointer around, that's what it is
> > > there
> > > for. Your "board binding" logic/code should be able to handle this,
> > > as
> > > it somehow was going to do the same thing with a "name".
> >
> > It's technically correct, but quality of this solution isn't really
> > nice, because it's a layering violation (at least if I understood what
> > you mean). This is because you need to have full definition of struct
> > phy in board file and a structure that is used as private data in PHY
> > core comes from platform code.
>
> No, just a pointer, you don't need the "full" structure until you get to
> some .c code that actually manipulates the phy itself, for all other
> places, you are just dealing with a pointer and a structure you never
> reference.
>
> Does that make more sense?
Well, to the point that I think I now understood your suggestion.
Unfortunately the suggestion alone isn't really something that can be done,
considering how driver core and generic frameworks work.
Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
