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:

quoted

quoted

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.

quoted

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?

quoted

quoted

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

quoted

quoted

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