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

[Greg KH <gregkh@linuxfoundation.org>]

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 :)

> 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).

> > 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.

> > 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?

thanks,

greg k-h