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

On Tuesday 23 of July 2013 09:18:46 Greg KH wrote:

On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:

quoted

Hi,

On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:

quoted

On Tue, 23 Jul 2013, Tomasz Figa wrote:

quoted

On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:

quoted

Hi Alan,

Thanks for helping to clarify the issues here.

quoted

quoted

quoted

Okay.  Are PHYs _always_ platform devices?

They can be i2c, spi or any other device types as well.

In those other cases, presumably there is no platform data associated
with the PHY since it isn't a platform device.  Then how does the
kernel know which controller is attached to the PHY?  Is this spelled
out in platform data associated with the PHY's i2c/spi/whatever
parent?

Yes. I think we could use i2c_board_info for passing platform data.

quoted

quoted

quoted

quoted

quoted

quoted

	PHY.  Currently this information is represented by name or

ID

quoted

quoted

quoted

quoted

	strings embedded in platform data.

right. It's embedded in the platform data of the controller.

It must also be embedded in the PHY's platform data somehow.
Otherwise, how would the kernel know which PHY to use?

By using a PHY lookup as Stephen and I suggested in our previous
replies. Without any extra data in platform data. (I have even
posted a
code example.)

I don't understand, because I don't know what "a PHY lookup" does.

It is how the PHY framework finds a PHY, when the controller (say
USB)requests a PHY from the PHY framework.

quoted

quoted

quoted

quoted

In this case, it doesn't matter where the platform_device
structures
are created or where the driver source code is.  Let's take a
simple
example.  Suppose the system design includes a PHY named "foo". 
Then
the board file could contain:

struct phy_info { ... } phy_foo;
EXPORT_SYMBOL_GPL(phy_foo);

and a header file would contain:

extern struct phy_info phy_foo;

The PHY supplier could then call phy_create(&phy_foo), and the PHY
client could call phy_find(&phy_foo).  Or something like that;
make up
your own structure tags and function names.

It's still possible to have conflicts, but now two PHYs with the
same
name (or a misspelled name somewhere) will cause an error at link
time.

This is incorrect, sorry. First of all it's a layering violation -
you
export random driver-specific symbols from one driver to another.
Then

No, that's not what I said.  Neither the PHY driver nor the
controller
driver exports anything to the other.  Instead, both drivers use data
exported by the board file.

I think instead we can use the same data while creating the platform
data of the controller and the PHY.
The PHY driver while creating the PHY (using PHY framework) will also
pass the *data* it actually got from the platform data to the
framework. The PHY user driver (USB), while requesting for the PHY
(from the PHY framework) will pass the *data* it got from its platform
data.
The PHY framework can do a comparison of the *data* pointers it has and
return the appropriate PHY to the controller.

quoted

quoted

quoted

imagine 4 SoCs - A, B, C, D. There are two PHY types PHY1 and PHY2
and
there are two types of consumer drivers (e.g. USB host
controllers). Now
consider following mapping:

SoC	PHY	consumer
A	PHY1	HOST1
B	PHY1	HOST2
C	PHY2	HOST1
D	PHY2	HOST2

So we have to be able to use any of the PHYs with any of the host
drivers. This means you would have to export symbol with the same
name
from both PHY drivers, which obviously would not work in this case,
because having both drivers enabled (in a multiplatform aware
configuration) would lead to linking conflict.

You're right; the scheme was too simple.  Instead, the board file
must
export two types of data structures, one for PHYs and one for
controllers.  Like this:

struct phy_info {

	/* Info for the controller attached to this PHY */
	struct controller_info	*hinfo;

};

struct controller_info {

	/* Info for the PHY which this controller is attached to */
	struct phy_info		*pinfo;

};

The board file for SoC A would contain:

struct phy_info phy1 = {&host1);
EXPORT_SYMBOL(phy1);
struct controller_info host1 = {&phy1};
EXPORT_SYMBOL(host1);

The board file for SoC B would contain:

struct phy_info phy1 = {&host2);
EXPORT_SYMBOL(phy1);
struct controller_info host2 = {&phy1};
EXPORT_SYMBOL(host2);

I meant something like this
struct phy_info {

	const char *name;

};

struct phy_platform_data {

	.
	.
	struct phy_info *info;

};

struct usb_controller_platform_data {

	.
	.
	struct phy_info *info;

};

struct phy_info phy_info;

While creating the phy device

	struct phy_platform_data phy_data;
	phy_data.info = &info;
	platform_device_add_data(pdev, &phy_data, sizeof(*phy_data))
	platform_device_add();

While creating the controller device

	struct usb_controller_platform_data controller_data;
	controller_data.info = &info;
	platform_device_add_data(pdev, &controller_data,
	sizeof(*controller_data)) platform_device_add();

Then modify PHY framework API phy create

	phy_create((struct device *dev, const struct phy_ops *ops,
	
        void *priv)  {//API changed to take void pointer instead of
        label
		
		. //existing implementation
		.
		phy->priv = priv;
	
	}
	
	struct phy *phy_get(struct device *dev, const char *string, void
	*priv) {> 
//API changed to take an additional pointer

		phy_lookup(priv)
	
	}
	
	static struct phy *phy_lookup(void *priv) {
	
		.
		.
		if (phy->priv=priv) //instead of string comparison, we'll use
		pointer
		
			return phy;
	
	}

PHY driver should be like

	phy_create((dev, ops, pdata->info);

The controller driver would do

	phy_get(dev, NULL, pdata->info);

Now the PHY framework will check for a match of *priv* pointer and
return the PHY.

I think this should be possible?

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.

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?

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.

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.

Best regards,
Tomasz