* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Mark Brown @ 2013-07-23 17:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723173710.GB28284@kroah.com>
[-- Attachment #1: Type: text/plain, Size: 757 bytes --]
On Tue, Jul 23, 2013 at 10:37:11AM -0700, Greg KH wrote:
> On Tue, Jul 23, 2013 at 06:50:29PM +0200, Tomasz Figa wrote:
> > 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.
What are the problems you are seeing with doing things with lookups?
Having to write platform data for everything gets old fast and the code
duplication is pretty tedious...
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 17:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1446965.6APW5ZgLBW@amdc1227>
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
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Mark Brown @ 2013-07-23 17:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231017290.1304-100000@iolanthe.rowland.org>
[-- Attachment #1: Type: text/plain, Size: 646 bytes --]
On Tue, Jul 23, 2013 at 10:37:05AM -0400, Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > > > 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?
Platform data is nothing to do with the platform bus - it's board
specific data (ie, data for the platform) and can be done with any
device.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723161846.GD2486@kroah.com>
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:
> > Hi,
> >
> > On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> > > On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > >> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> > >>> Hi Alan,
> > >
> > > Thanks for helping to clarify the issues here.
> > >
> > >>>> 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.
> >
> > >>>>>> PHY. Currently this information is represented by name or
> > >>
> > >> ID
> > >>
> > >>>>>> 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.
> >
> > >>>> 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.
> >
> > >>> 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
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-23 16:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130723161846.GD2486@kroah.com>
Hi Greg,
On Tuesday 23 July 2013 09:48 PM, Greg KH wrote:
> On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
>>> On Tue, 23 Jul 2013, Tomasz Figa wrote:
>>>
>>>> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
>>>>> Hi Alan,
>>>
>>> Thanks for helping to clarify the issues here.
>>>
>>>>>> 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?
.
.
<snip>
.
.
>>
>> 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?
>
> 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.)
>
> 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".
The problem is the board file won't have the *phy* pointer. *phy* pointer is
created at a much later time when the phy driver is probed.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 16:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51EEAF32.4040905@ti.com>
On Tue, Jul 23, 2013 at 09:58:34PM +0530, Kishon Vijay Abraham I wrote:
> Hi Greg,
>
> On Tuesday 23 July 2013 09:48 PM, Greg KH wrote:
> > On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> >>> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> >>>
> >>>> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> >>>>> Hi Alan,
> >>>
> >>> Thanks for helping to clarify the issues here.
> >>>
> >>>>>> 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?
> .
> .
> <snip>
> .
> .
> >>
> >> 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?
> >
> > 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.)
> >
> > 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".
>
> The problem is the board file won't have the *phy* pointer. *phy* pointer is
> created at a much later time when the phy driver is probed.
Ok, then save it then, as no one could have used it before then, right?
All I don't want to see is any "get by name/void *" functions in the
api, as that way is fragile and will break, as people have already
shown.
Just pass the real pointer around. If that is somehow a problem, then
something larger is a problem with how board devices are tied together :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-23 16:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51EE9EC0.6060905@ti.com>
On Tue, Jul 23, 2013 at 08:48:24PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> > On Tue, 23 Jul 2013, Tomasz Figa wrote:
> >
> >> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> >>> Hi Alan,
> >
> > Thanks for helping to clarify the issues here.
> >
> >>>> 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.
> >
> >>>>>> PHY. Currently this information is represented by name or
> >> ID
> >>>>>> 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.
> >
> >>>> 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.
> >
> >>> 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?
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.)
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".
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-23 15:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231017290.1304-100000@iolanthe.rowland.org>
Hi,
On Tuesday 23 July 2013 08:07 PM, Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
>
>> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
>>> Hi Alan,
>
> Thanks for helping to clarify the issues here.
>
>>>> 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.
>
>>>>>> PHY. Currently this information is represented by name or
>> ID
>>>>>> 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.
>
>>>> 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.
>
>>> 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?
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 14:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307231017290.1304-100000@iolanthe.rowland.org>
On Tuesday 23 of July 2013 10:37:05 Alan Stern wrote:
> On Tue, 23 Jul 2013, Tomasz Figa wrote:
> > On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> > > Hi Alan,
>
> Thanks for helping to clarify the issues here.
>
> > > > 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?
>
> > > > > > PHY. Currently this information is represented by name or
> >
> > ID
> >
> > > > > > 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.
I have provided a code example in [1]. Feel free to ask questions about
those code snippets.
[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813/focus 889
> > > > 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.
It's still a random, driver-specific global symbol exported from board file
to drivers.
> > > 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);
>
> And so on. This explicitly gives the connection between PHYs and
> controllers. The PHY providers would use &phy1 or &phy2, and the PHY
> consumers would use &host1 or &host2.
This could work assuming that only one SoC and one board is supported in
single kernel image. However it's not the case.
We've used to support multiple boards since a long time already and now for
selected platforms we even support multiplatform, i.e. multiple SoCs in
single zImage. Such solution will not work.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-23 14:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Tue, 23 Jul 2013, Tomasz Figa wrote:
> On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> > Hi Alan,
Thanks for helping to clarify the issues here.
> > > 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?
> > > > > PHY. Currently this information is represented by name or
> ID
> > > > > 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.
> > > 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.
> > 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);
And so on. This explicitly gives the connection between PHYs and
controllers. The PHY providers would use &phy1 or &phy2, and the PHY
consumers would use &host1 or &host2.
Alan Stern
^ permalink raw reply
* Re: [PATCH] omapfb: In omapfb_probe return -EPROBE_DEFER when display driver is not loaded yet
From: Tomi Valkeinen @ 2013-07-23 10:01 UTC (permalink / raw)
To: Pavel Machek, Pali Rohár
Cc: Jean-Christophe Plagniol-Villard, linux-omap, linux-fbdev,
linux-kernel, Aaro Koskinen, Tony Lindgren
In-Reply-To: <20130713182733.GA25019@amd.pavel.ucw.cz>
[-- Attachment #1: Type: text/plain, Size: 2852 bytes --]
On 13/07/13 21:27, Pavel Machek wrote:
> On Wed 2013-07-10 15:08:59, Pali Rohár wrote:
>> * On RX-51 probing for acx565akm driver is later then for omapfb which cause that omapfb probe fail and framebuffer is not working
>> * EPROBE_DEFER causing that kernel try to probe for omapfb later again which fixing this problem
>>
>> * Without this patch display on Nokia RX-51 (N900) phone not working
>>
>> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
>
> Tested-by: Pavel Machek <pavel@ucw.cz>
Which kernel version is this? Does it have
dfbc32316c6991010328c21e6046b05bac57eb84 (OMAPFB: defer probe if no displays)?
Then again, rx51 has tv-output, which probably gets probed early. So omapfb
does see one functional display, and starts, even if the LCD is not available
yet.
> (Actually, do we know which commit broke the ordering? We may want to
> simply revert that one...)
Well, my understand that this is how it's supposed to work. There's no defined
order with the driver probes, and the drivers just have to deal with their
dependencies not being there yet.
I don't have a perfect solution for this problem for omapfb. omapfb doesn't
support dynamically adding displays, so all the displays it uses have to be
probed before omapfb. And omapfb doesn't know which displays will be probed.
The patch below was added for 3.11. Does it fix the issue for you? Perhaps it
should be added for 3.10 also.
Tomi
commit e9f322b4913e5d3e5c5d21dc462ca6f8a86e1df1
Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Thu May 23 16:41:25 2013 +0300
OMAPFB: use EPROBE_DEFER if default display is not present
Currently omapfb returns EPROBE_DEFER if no displays have been probed at
the time omapfb is probed. However, sometimes some of the displays have
been probed at that time, but not all. We can't return EPROBE_DEFER in
that case, because then one missing driver would cause omapfb to defer
always, preventing any display from working.
However, if the user has defined a default display, we can presume that
the driver for that display is eventually loaded. Thus, this patch
changes omapfb to return EPROBE_DEFER in case default display is not
found.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
diff --git a/drivers/video/omap2/omapfb/omapfb-main.c b/drivers/video/omap2/omapfb/omapfb-main.c
index 528e453..27d6905 100644
--- a/drivers/video/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/omap2/omapfb/omapfb-main.c
@@ -2503,7 +2503,7 @@ static int omapfb_probe(struct platform_device *pdev)
if (def_display == NULL) {
dev_err(fbdev->dev, "failed to find default display\n");
- r = -EINVAL;
+ r = -EPROBE_DEFER;
goto cleanup;
}
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply related
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 7:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1714400.neMPBWOlzi@flatron>
[Fixed address of devicetree mailing list and added more people on CC.]
For reference, full thread can be found under following link:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813
Best regards,
Tomasz
On Tuesday 23 of July 2013 09:29:32 Tomasz Figa wrote:
> Hi Alan,
>
> On Monday 22 of July 2013 10:44:39 Alan Stern wrote:
> > On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
> > > > The PHY and the controller it is attached to are both
physical
> > > > devices.
> > > >
> > > > The connection between them is hardwired by the system
> > > > manufacturer and cannot be changed by software.
> > > >
> > > > PHYs are generally described by fixed system-specific
board
> > > > files or by Device Tree information. Are they ever
discovered
> > > > dynamically?
> > >
> > > No. They are created just like any other platform devices are
> > > created.
> >
> > Okay. Are PHYs _always_ platform devices?
>
> They can be i2c, spi or any other device types as well.
>
> > > > Is the same true for the controllers attached to the PHYs?
> > > > If not -- if both a PHY and a controller are discovered
> > > > dynamically -- how does the kernel know whether they are
> > > > connected to each other?
> > >
> > > No differences here. Both PHY and controller will have dt
> > > information
> > > or hwmod data using which platform devices will be created.
> > >
> > > > The kernel needs to know which controller is attached to
which
> > > > PHY. Currently this information is represented by name or
ID
> > > > 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.)
>
> > > > The PHY's driver (the supplier) uses the platform data to
> > > > construct a platform_device structure that represents the
PHY.
> > >
> > > Currently the driver assigns static labels (corresponding to the
> > > label
> > > used in the platform data of the controller).
> > >
> > > > Until this is done, the controller's driver (the client)
cannot
> > > > use the PHY.
> > >
> > > right.
> > >
> > > > Since there is no parent-child relation between the PHY
and the
> > > > controller, there is no guarantee that the PHY's driver
will be
> > > > ready when the controller's driver wants to use it. A
deferred
> > > > probe may be needed.
> > >
> > > right.
> > >
> > > > The issue (or one of the issues) in this discussion is
that
> > > > Greg does not like the idea of using names or IDs to
associate
> > > > PHYs with controllers, because they are too prone to
> > > > duplications or other errors. Pointers are more reliable.
> > > >
> > > > But pointers to what? Since the only data known to be
> > > > available to both the PHY driver and controller driver is
the
> > > > platform data, the obvious answer is a pointer to platform
data
> > > > (either for the PHY or for the controller, or maybe both).
> > >
> > > hmm.. it's not going to be simple though as the platform device for
> > > the PHY and controller can be created in entirely different places.
> > > e.g., in some cases the PHY device is a child of some mfd core
> > > device
> > > (the device will be created in drivers/mfd) and the controller
> > > driver
> > > (usually) is created in board file. I guess then we have to come up
> > > with something to share a pointer in two different files.
> >
> > The ability for two different source files to share a pointer to a
> > data
> > item defined in a third source file has been around since long before
> > the C language was invented. :-)
> >
> > 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
> 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.
>
> Best regards,
> Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-23 7:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307221028440.1495-100000@iolanthe.rowland.org>
Hi Alan,
On Monday 22 of July 2013 10:44:39 Alan Stern wrote:
> On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
> > > The PHY and the controller it is attached to are both physical
> > > devices.
> > >
> > > The connection between them is hardwired by the system
> > > manufacturer and cannot be changed by software.
> > >
> > > PHYs are generally described by fixed system-specific board
> > > files or by Device Tree information. Are they ever discovered
> > > dynamically?
> >
> > No. They are created just like any other platform devices are created.
>
> Okay. Are PHYs _always_ platform devices?
They can be i2c, spi or any other device types as well.
> > > Is the same true for the controllers attached to the PHYs?
> > > If not -- if both a PHY and a controller are discovered
> > > dynamically -- how does the kernel know whether they are
> > > connected to each other?
> >
> > No differences here. Both PHY and controller will have dt information
> > or hwmod data using which platform devices will be created.
> >
> > > The kernel needs to know which controller is attached to which
> > > PHY. Currently this information is represented by name or ID
> > > 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.)
> > > The PHY's driver (the supplier) uses the platform data to
> > > construct a platform_device structure that represents the PHY.
> >
> > Currently the driver assigns static labels (corresponding to the label
> > used in the platform data of the controller).
> >
> > > Until this is done, the controller's driver (the client) cannot
> > > use the PHY.
> >
> > right.
> >
> > > Since there is no parent-child relation between the PHY and the
> > > controller, there is no guarantee that the PHY's driver will be
> > > ready when the controller's driver wants to use it. A deferred
> > > probe may be needed.
> >
> > right.
> >
> > > The issue (or one of the issues) in this discussion is that
> > > Greg does not like the idea of using names or IDs to associate
> > > PHYs with controllers, because they are too prone to
> > > duplications or other errors. Pointers are more reliable.
> > >
> > > But pointers to what? Since the only data known to be
> > > available to both the PHY driver and controller driver is the
> > > platform data, the obvious answer is a pointer to platform data
> > > (either for the PHY or for the controller, or maybe both).
> >
> > hmm.. it's not going to be simple though as the platform device for
> > the PHY and controller can be created in entirely different places.
> > e.g., in some cases the PHY device is a child of some mfd core device
> > (the device will be created in drivers/mfd) and the controller driver
> > (usually) is created in board file. I guess then we have to come up
> > with something to share a pointer in two different files.
>
> The ability for two different source files to share a pointer to a data
> item defined in a third source file has been around since long before
> the C language was invented. :-)
>
> 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
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.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-23 5:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307221028440.1495-100000@iolanthe.rowland.org>
Hi,
On Monday 22 July 2013 08:14 PM, Alan Stern wrote:
> On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
>
>>> The PHY and the controller it is attached to are both physical
>>> devices.
>>>
>>> The connection between them is hardwired by the system
>>> manufacturer and cannot be changed by software.
>>>
>>> PHYs are generally described by fixed system-specific board
>>> files or by Device Tree information. Are they ever discovered
>>> dynamically?
>>
>> No. They are created just like any other platform devices are created.
>
> Okay. Are PHYs _always_ platform devices?
Not always. It can be any other device also.
>
>>> Is the same true for the controllers attached to the PHYs?
>>> If not -- if both a PHY and a controller are discovered
>>> dynamically -- how does the kernel know whether they are
>>> connected to each other?
>>
>> No differences here. Both PHY and controller will have dt information or hwmod
>> data using which platform devices will be created.
>>>
>>> The kernel needs to know which controller is attached to which
>>> PHY. Currently this information is represented by name or ID
>>> 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?
>
>>> The PHY's driver (the supplier) uses the platform data to
>>> construct a platform_device structure that represents the PHY.
>>
>> Currently the driver assigns static labels (corresponding to the label used in
>> the platform data of the controller).
>>> Until this is done, the controller's driver (the client) cannot
>>> use the PHY.
>>
>> right.
>>>
>>> Since there is no parent-child relation between the PHY and the
>>> controller, there is no guarantee that the PHY's driver will be
>>> ready when the controller's driver wants to use it. A deferred
>>> probe may be needed.
>>
>> right.
>>>
>>> The issue (or one of the issues) in this discussion is that
>>> Greg does not like the idea of using names or IDs to associate
>>> PHYs with controllers, because they are too prone to
>>> duplications or other errors. Pointers are more reliable.
>>>
>>> But pointers to what? Since the only data known to be
>>> available to both the PHY driver and controller driver is the
>>> platform data, the obvious answer is a pointer to platform data
>>> (either for the PHY or for the controller, or maybe both).
>>
>> hmm.. it's not going to be simple though as the platform device for the PHY and
>> controller can be created in entirely different places. e.g., in some cases the
>> PHY device is a child of some mfd core device (the device will be created in
>> drivers/mfd) and the controller driver (usually) is created in board file. I
>> guess then we have to come up with something to share a pointer in two
>> different files.
>
> The ability for two different source files to share a pointer to a data
> item defined in a third source file has been around since long before
> the C language was invented. :-)
>
> 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.
Alright. Thanks for the hint :-)
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-23 5:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130722150458.GA18181@kroah.com>
Hi,
On Monday 22 July 2013 08:34 PM, Greg KH wrote:
> On Mon, Jul 22, 2013 at 12:55:18PM +0530, Kishon Vijay Abraham I wrote:
>>> The issue (or one of the issues) in this discussion is that
>>> Greg does not like the idea of using names or IDs to associate
>>> PHYs with controllers, because they are too prone to
>>> duplications or other errors. Pointers are more reliable.
>>>
>>> But pointers to what? Since the only data known to be
>>> available to both the PHY driver and controller driver is the
>>> platform data, the obvious answer is a pointer to platform data
>>> (either for the PHY or for the controller, or maybe both).
>>
>> hmm.. it's not going to be simple though as the platform device for the PHY and
>> controller can be created in entirely different places. e.g., in some cases the
>> PHY device is a child of some mfd core device (the device will be created in
>> drivers/mfd) and the controller driver (usually) is created in board file. I
>> guess then we have to come up with something to share a pointer in two
>> different files.
>
> What's wrong with using the platform_data structure that is unique to
> all boards (see include/platform_data/ for examples)? Isn't that what
> this structure is there for?
Alright. I got some ideas from Alan Stern. I'll use it with platform_data and
repost the series.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH] drivers: video: fbmem: add signed type cast for comparation.
From: Chen Gang @ 2013-07-23 0:04 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECE851.4030002@asianux.com>
On 07/22/2013 07:16 PM, Geert Uytterhoeven wrote:
> On Mon, Jul 22, 2013 at 10:07 AM, Chen Gang <gang.chen@asianux.com> wrote:
>> For 'con2fb.framebuffer', it can be '-1' as an invalid value, so it
>> need related type cast for its comparing.
>>
>> The related warning:
>>
>> drivers/video/fbmem.c:1169:3: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>>
>>
>> Signed-off-by: Chen Gang <gang.chen@asianux.com>
>> ---
>> drivers/video/fbmem.c | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index 36e1fe2..8e8225c 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -1166,7 +1166,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
>> return -EFAULT;
>> if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
>> return -EINVAL;
>> - if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
>> + if ((int)con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
>> return -EINVAL;
>
> Instead of adding the cast, you can also just remove the check, as it's useless.
> If it's `-1' for invalid, after conversion to unsigned, it will
> trigger the check
> ">= FB_MAX".
>
OH, yes, thanks.
Original implementation is no issue (the compiler will skip the first
checking automatically).
If suitable, I should send patch v2.
Thanks.
> Gr{oetje,eeting}s,
>
> Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
> -- Linus Torvalds
>
>
--
Chen Gang
^ permalink raw reply
* [PATCH] vga16fb: remove unused variable
From: andi.shyti @ 2013-07-22 15:41 UTC (permalink / raw)
To: plagnioj, tomi.valkeinen; +Cc: linux-fbdev, linux-kernel, andi, andi.shyti
In-Reply-To: <20130710225659.GA14107@hercules>
From: Andi Shyti <andi.shyti@gmail.com>
This patch gets rid of the following warning:
drivers/video/vga16fb.c: In function ‘vga16fb_destroy’:
drivers/video/vga16fb.c:1268:26: warning: unused variable ‘dev’ [-Wunused-variable]
struct platform_device *dev = container_of(info->device, struct platform_device, dev);
^
As described, the 'dev' variable is no longer used, therefore it
can be removed.
Signed-off-by: Andi Shyti <andi.shyti@gmail.com>
---
drivers/video/vga16fb.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/vga16fb.c b/drivers/video/vga16fb.c
index 830ded4..2827333 100644
--- a/drivers/video/vga16fb.c
+++ b/drivers/video/vga16fb.c
@@ -1265,7 +1265,6 @@ static void vga16fb_imageblit(struct fb_info *info, const struct fb_image *image
static void vga16fb_destroy(struct fb_info *info)
{
- struct platform_device *dev = container_of(info->device, struct platform_device, dev);
iounmap(info->screen_base);
fb_dealloc_cmap(&info->cmap);
/* XXX unshare VGA regions */
--
1.8.3.2
^ permalink raw reply related
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-22 15:04 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51ECDE5E.3050104@ti.com>
On Mon, Jul 22, 2013 at 12:55:18PM +0530, Kishon Vijay Abraham I wrote:
> > The issue (or one of the issues) in this discussion is that
> > Greg does not like the idea of using names or IDs to associate
> > PHYs with controllers, because they are too prone to
> > duplications or other errors. Pointers are more reliable.
> >
> > But pointers to what? Since the only data known to be
> > available to both the PHY driver and controller driver is the
> > platform data, the obvious answer is a pointer to platform data
> > (either for the PHY or for the controller, or maybe both).
>
> hmm.. it's not going to be simple though as the platform device for the PHY and
> controller can be created in entirely different places. e.g., in some cases the
> PHY device is a child of some mfd core device (the device will be created in
> drivers/mfd) and the controller driver (usually) is created in board file. I
> guess then we have to come up with something to share a pointer in two
> different files.
What's wrong with using the platform_data structure that is unique to
all boards (see include/platform_data/ for examples)? Isn't that what
this structure is there for?
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-22 14:44 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Mon, 22 Jul 2013, Kishon Vijay Abraham I wrote:
> > The PHY and the controller it is attached to are both physical
> > devices.
> >
> > The connection between them is hardwired by the system
> > manufacturer and cannot be changed by software.
> >
> > PHYs are generally described by fixed system-specific board
> > files or by Device Tree information. Are they ever discovered
> > dynamically?
>
> No. They are created just like any other platform devices are created.
Okay. Are PHYs _always_ platform devices?
> > Is the same true for the controllers attached to the PHYs?
> > If not -- if both a PHY and a controller are discovered
> > dynamically -- how does the kernel know whether they are
> > connected to each other?
>
> No differences here. Both PHY and controller will have dt information or hwmod
> data using which platform devices will be created.
> >
> > The kernel needs to know which controller is attached to which
> > PHY. Currently this information is represented by name or ID
> > 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?
> > The PHY's driver (the supplier) uses the platform data to
> > construct a platform_device structure that represents the PHY.
>
> Currently the driver assigns static labels (corresponding to the label used in
> the platform data of the controller).
> > Until this is done, the controller's driver (the client) cannot
> > use the PHY.
>
> right.
> >
> > Since there is no parent-child relation between the PHY and the
> > controller, there is no guarantee that the PHY's driver will be
> > ready when the controller's driver wants to use it. A deferred
> > probe may be needed.
>
> right.
> >
> > The issue (or one of the issues) in this discussion is that
> > Greg does not like the idea of using names or IDs to associate
> > PHYs with controllers, because they are too prone to
> > duplications or other errors. Pointers are more reliable.
> >
> > But pointers to what? Since the only data known to be
> > available to both the PHY driver and controller driver is the
> > platform data, the obvious answer is a pointer to platform data
> > (either for the PHY or for the controller, or maybe both).
>
> hmm.. it's not going to be simple though as the platform device for the PHY and
> controller can be created in entirely different places. e.g., in some cases the
> PHY device is a child of some mfd core device (the device will be created in
> drivers/mfd) and the controller driver (usually) is created in board file. I
> guess then we have to come up with something to share a pointer in two
> different files.
The ability for two different source files to share a pointer to a data
item defined in a third source file has been around since long before
the C language was invented. :-)
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.
> > Probably some of the details above are wrong; please indicate where I
> > have gone astray. Also, I'm not clear about the role played by various
> > APIs. For example, where does phy_create() fit into this picture?
>
> phy_create is the API by which the PHY's driver (the supplier) hook into the
> PHY framework. It's like the controller driver will always interact with the
> PHY driver through the PHY framework.
Alan Stern
^ permalink raw reply
* Re: [PATCH] drivers: video: fbmem: add signed type cast for comparation.
From: Geert Uytterhoeven @ 2013-07-22 11:16 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <51ECE851.4030002@asianux.com>
On Mon, Jul 22, 2013 at 10:07 AM, Chen Gang <gang.chen@asianux.com> wrote:
> For 'con2fb.framebuffer', it can be '-1' as an invalid value, so it
> need related type cast for its comparing.
>
> The related warning:
>
> drivers/video/fbmem.c:1169:3: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
>
>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
> drivers/video/fbmem.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index 36e1fe2..8e8225c 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -1166,7 +1166,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
> return -EFAULT;
> if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
> return -EINVAL;
> - if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
> + if ((int)con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
> return -EINVAL;
Instead of adding the cast, you can also just remove the check, as it's useless.
If it's `-1' for invalid, after conversion to unsigned, it will
trigger the check
">= FB_MAX".
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] drivers: video: i740fb: add 'default' processing contents for 'switch'.
From: Chen Gang @ 2013-07-22 8:45 UTC (permalink / raw)
To: linux-fbdev
Need add related 'default' processing contents for 'switch', or may
report 'wm' uninitialized warning.
The related warning:
drivers/video/i740fb.c:662:26: warning: ‘wm’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
drivers/video/i740fb.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/video/i740fb.c b/drivers/video/i740fb.c
index 6c48388..e82e767 100644
--- a/drivers/video/i740fb.c
+++ b/drivers/video/i740fb.c
@@ -336,6 +336,9 @@ static u32 i740_calc_fifo(struct i740fb_par *par, u32 freq, int bpp)
wm = 0x16110000;
}
break;
+ default:
+ wm = 0;
+ BUG();
}
return wm;
--
1.7.7.6
^ permalink raw reply related
* Re: [PATCH 2/3] video: hx8357: Make IM pins optional
From: Jingoo Han @ 2013-07-22 8:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130719083555.GC5106@lukather>
On Friday, July 19, 2013 5:36 PM, Maxime Ripard wrote:
> On Tue, Jul 16, 2013 at 05:29:40AM +0200, Mike Galbraith wrote:
> > On Tue, 2013-07-16 at 09:49 +0900, Jingoo Han wrote:
> > > On Tuesday, July 16, 2013 12:27 AM, Maxime Ripard wrote:
> >
> > > > +
> > > > + ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > > > + GPIOF_OUT_INIT_LOW, "im_pins");
> > >
> > > This makes a checkpatch warning such as 'WARNING: line over 80 characters'.
> > > How about the following?
> > >
> > > ret = devm_gpio_request_one(&spi->dev, lcd->im_pins[i],
> > > GPIOF_OUT_INIT_LOW, "im_pins");
> >
> > IIRC, some maintainers gripe (davem?) when they see such alignment,
> > preferring the original arg below arg alignment vs strict 80 column.
>
> As far as I know, the coding guide styles are quite fuzzy about this:
> - The new line is not required to be aligned with the braces above
> - Yet, the emacs config given does indent like this.
> - 80 characters is said not to be a hard limit
Even though 80 characters is not a hard limit, 80 characters is preferred
if possible.
>
> I don't really know if there's a better solution here, except maybe:
> ret = devm_gpio_request_one(&spi-dev, lcd->im_pins[i],
> GPIOF_OUT_INIT_LOW,
> "im_pins");
Yes, I think that this can be used. :)
Best regards,
Jingoo Han
>
> But it's not really a big deal, is it?
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
^ permalink raw reply
* [PATCH] drivers: video: fbcmap: add signed type cast for comparation
From: Chen Gang @ 2013-07-22 8:26 UTC (permalink / raw)
To: linux-fbdev
According to fb_set_cmap() in this file, knows about it is necessary to
check 'start' whether less than zero or not, so need add related type
cast for its comparing.
The related warning:
drivers/video/fbcmap.c:288:2: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
drivers/video/fbcmap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index 5c3960d..44c88e3 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -285,7 +285,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
rc = -ENODEV;
goto out;
}
- if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
+ if ((int)cmap->start < 0 || (!info->fbops->fb_setcolreg &&
!info->fbops->fb_setcmap)) {
rc = -EINVAL;
goto out1;
--
1.7.7.6
^ permalink raw reply related
* [PATCH] drivers: video: fbmem: add signed type cast for comparation.
From: Chen Gang @ 2013-07-22 8:07 UTC (permalink / raw)
To: linux-fbdev
For 'con2fb.framebuffer', it can be '-1' as an invalid value, so it
need related type cast for its comparing.
The related warning:
drivers/video/fbmem.c:1169:3: warning: comparison of unsigned expression < 0 is always false [-Wtype-limits]
Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
drivers/video/fbmem.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 36e1fe2..8e8225c 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1166,7 +1166,7 @@ static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,
return -EFAULT;
if (con2fb.console < 1 || con2fb.console > MAX_NR_CONSOLES)
return -EINVAL;
- if (con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
+ if ((int)con2fb.framebuffer < 0 || con2fb.framebuffer >= FB_MAX)
return -EINVAL;
if (!registered_fb[con2fb.framebuffer])
request_module("fb%d", con2fb.framebuffer);
--
1.7.7.6
^ permalink raw reply related
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-22 7:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307211453450.19133-100000@netrider.rowland.org>
Hi,
On Monday 22 July 2013 12:52 AM, Alan Stern wrote:
> On Sun, 21 Jul 2013, Sylwester Nawrocki wrote:
>
>>> What's wrong with the platform_data structure, why can't that be used
>>> for this?
>>
>> At the point the platform data of some driver is initialized, e.g. in
>> board setup code the PHY pointer is not known, since the PHY supplier
>> driver has not initialized yet. Even though we wanted to pass pointer
>> to a PHY through some notifier call, it would have been not clear
>> which PHY user driver should match on such notifier. A single PHY
>> supplier driver can create M PHY objects and this needs to be mapped
>> to N PHY user drivers. This mapping needs to be defined somewhere by
>> the system integrator. It works well with device tree, but except that
>> there seems to be no other reliable infrastructure in the kernel to
>> define links/dependencies between devices, since device identifiers are
>> not known in advance in all cases.
>>
>> What Tomasz proposed seems currently most reasonable to me for non-dt.
>>
>>> Or, if not, we can always add pointers to the platform device structure,
>>> or even the main 'struct device' as well, that's what it is there for.
>>
>> Still we would need to solve a problem which platform device structure
>> gets which PHY pointer.
>
> Can you explain the issues in more detail? I don't fully understand
> the situation.
>
> Here's what I think I know:
>
> The PHY and the controller it is attached to are both physical
> devices.
>
> The connection between them is hardwired by the system
> manufacturer and cannot be changed by software.
>
> PHYs are generally described by fixed system-specific board
> files or by Device Tree information. Are they ever discovered
> dynamically?
No. They are created just like any other platform devices are created.
>
> Is the same true for the controllers attached to the PHYs?
> If not -- if both a PHY and a controller are discovered
> dynamically -- how does the kernel know whether they are
> connected to each other?
No differences here. Both PHY and controller will have dt information or hwmod
data using which platform devices will be created.
>
> The kernel needs to know which controller is attached to which
> PHY. Currently this information is represented by name or ID
> strings embedded in platform data.
right. It's embedded in the platform data of the controller.
>
> The PHY's driver (the supplier) uses the platform data to
> construct a platform_device structure that represents the PHY.
Currently the driver assigns static labels (corresponding to the label used in
the platform data of the controller).
> Until this is done, the controller's driver (the client) cannot
> use the PHY.
right.
>
> Since there is no parent-child relation between the PHY and the
> controller, there is no guarantee that the PHY's driver will be
> ready when the controller's driver wants to use it. A deferred
> probe may be needed.
right.
>
> The issue (or one of the issues) in this discussion is that
> Greg does not like the idea of using names or IDs to associate
> PHYs with controllers, because they are too prone to
> duplications or other errors. Pointers are more reliable.
>
> But pointers to what? Since the only data known to be
> available to both the PHY driver and controller driver is the
> platform data, the obvious answer is a pointer to platform data
> (either for the PHY or for the controller, or maybe both).
hmm.. it's not going to be simple though as the platform device for the PHY and
controller can be created in entirely different places. e.g., in some cases the
PHY device is a child of some mfd core device (the device will be created in
drivers/mfd) and the controller driver (usually) is created in board file. I
guess then we have to come up with something to share a pointer in two
different files.
>
> Probably some of the details above are wrong; please indicate where I
> have gone astray. Also, I'm not clear about the role played by various
> APIs. For example, where does phy_create() fit into this picture?
phy_create is the API by which the PHY's driver (the supplier) hook into the
PHY framework. It's like the controller driver will always interact with the
PHY driver through the PHY framework.
Thanks
Kishon
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox