[PATCH 01/15] drivers: phy: add generic PHY framework

Tomasz Figa t.figa at samsung.com
Tue Jul 23 13:48:11 EDT 2013


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




More information about the linux-arm-kernel mailing list