* 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
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-21 19:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
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?
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?
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.
The PHY's driver (the supplier) uses the platform data to
construct a platform_device structure that represents the PHY.
Until this is done, the controller's driver (the client) cannot
use the PHY.
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.
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).
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?
Alan Stern
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Sylwester Nawrocki @ 2013-07-21 17:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130721154808.GH16598@kroah.com>
On 07/21/2013 05:48 PM, Greg KH wrote:
> On Sun, Jul 21, 2013 at 12:22:48PM +0200, Sascha Hauer wrote:
>> On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote:
>>> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
>>>> On Sat, 20 Jul 2013, Greg KH wrote:
>>>>
>>>>>>>> That should be passed using platform data.
>>>>>>>
>>>>>>> Ick, don't pass strings around, pass pointers. If you have platform
>>>>>>> data you can get to, then put the pointer there, don't use a "name".
>>>>>>
>>>>>> I don't think I understood you here :-s We wont have phy pointer
>>>>>> when we create the device for the controller no?(it'll be done in
>>>>>> board file). Probably I'm missing something.
>>>>>
>>>>> Why will you not have that pointer? You can't rely on the "name" as the
>>>>> device id will not match up, so you should be able to rely on the
>>>>> pointer being in the structure that the board sets up, right?
>>>>>
>>>>> Don't use names, especially as ids can, and will, change, that is going
>>>>> to cause big problems. Use pointers, this is C, we are supposed to be
>>>>> doing that :)
>>>>
>>>> Kishon, I think what Greg means is this: The name you are using must
>>>> be stored somewhere in a data structure constructed by the board file,
>>>> right? Or at least, associated with some data structure somehow.
>>>> Otherwise the platform code wouldn't know which PHY hardware
>>>> corresponded to a particular name.
>>>>
>>>> Greg's suggestion is that you store the address of that data structure
>>>> in the platform data instead of storing the name string. Have the
>>>> consumer pass the data structure's address when it calls phy_create,
>>>> instead of passing the name. Then you don't have to worry about two
>>>> PHYs accidentally ending up with the same name or any other similar
>>>> problems.
>>>
>>> Close, but the issue is that whatever returns from phy_create() should
>>> then be used, no need to call any "find" functions, as you can just use
>>> the pointer that phy_create() returns. Much like all other class api
>>> functions in the kernel work.
>>
>> I think the problem here is to connect two from the bus structure
>> completely independent devices. Several frameworks (ASoC, soc-camera)
>> had this problem and this wasn't solved until the advent of devicetrees
>> and their phandles.
>> phy_create might be called from the probe function of some i2c device
>> (the phy device) and the resulting pointer is then needed in some other
>> platform devices (the user of the phy) probe function.
>> The best solution we have right now is implemented in the clk framework
>> which uses a string matching of the device names in clk_get() (at least
>> in the non-dt case).
>
> I would argue that clocks are wrong here as well, as others have already
> pointed out.
>
> 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.
--
Regards,
Sylwester
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-21 15:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130721102248.GE29785@pengutronix.de>
On Sun, Jul 21, 2013 at 12:22:48PM +0200, Sascha Hauer wrote:
> On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote:
> > On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > > On Sat, 20 Jul 2013, Greg KH wrote:
> > >
> > > > > >>That should be passed using platform data.
> > > > > >
> > > > > >Ick, don't pass strings around, pass pointers. If you have platform
> > > > > >data you can get to, then put the pointer there, don't use a "name".
> > > > >
> > > > > I don't think I understood you here :-s We wont have phy pointer
> > > > > when we create the device for the controller no?(it'll be done in
> > > > > board file). Probably I'm missing something.
> > > >
> > > > Why will you not have that pointer? You can't rely on the "name" as the
> > > > device id will not match up, so you should be able to rely on the
> > > > pointer being in the structure that the board sets up, right?
> > > >
> > > > Don't use names, especially as ids can, and will, change, that is going
> > > > to cause big problems. Use pointers, this is C, we are supposed to be
> > > > doing that :)
> > >
> > > Kishon, I think what Greg means is this: The name you are using must
> > > be stored somewhere in a data structure constructed by the board file,
> > > right? Or at least, associated with some data structure somehow.
> > > Otherwise the platform code wouldn't know which PHY hardware
> > > corresponded to a particular name.
> > >
> > > Greg's suggestion is that you store the address of that data structure
> > > in the platform data instead of storing the name string. Have the
> > > consumer pass the data structure's address when it calls phy_create,
> > > instead of passing the name. Then you don't have to worry about two
> > > PHYs accidentally ending up with the same name or any other similar
> > > problems.
> >
> > Close, but the issue is that whatever returns from phy_create() should
> > then be used, no need to call any "find" functions, as you can just use
> > the pointer that phy_create() returns. Much like all other class api
> > functions in the kernel work.
>
> I think the problem here is to connect two from the bus structure
> completely independent devices. Several frameworks (ASoC, soc-camera)
> had this problem and this wasn't solved until the advent of devicetrees
> and their phandles.
> phy_create might be called from the probe function of some i2c device
> (the phy device) and the resulting pointer is then needed in some other
> platform devices (the user of the phy) probe function.
> The best solution we have right now is implemented in the clk framework
> which uses a string matching of the device names in clk_get() (at least
> in the non-dt case).
I would argue that clocks are wrong here as well, as others have already
pointed out.
What's wrong with the platform_data structure, why can't that be used
for this?
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.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-21 15:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <9748041.Qq1fWJBg6D@flatron>
On Sun, Jul 21, 2013 at 01:12:07PM +0200, Tomasz Figa wrote:
> On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
> > Hi,
> >
> > On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
> > > Hi,
> > >
> > > On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
> > >> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > >>> On Sat, 20 Jul 2013, Greg KH wrote:
> > >>>>>>> That should be passed using platform data.
> > >>>>>>
> > >>>>>> Ick, don't pass strings around, pass pointers. If you have
> > >>>>>> platform
> > >>>>>> data you can get to, then put the pointer there, don't use a
> > >>>>>> "name".
> > >>>>>
> > >>>>> I don't think I understood you here :-s We wont have phy pointer
> > >>>>> when we create the device for the controller no?(it'll be done in
> > >>>>> board file). Probably I'm missing something.
> > >>>>
> > >>>> Why will you not have that pointer? You can't rely on the "name"
> > >>>> as
> > >>>> the device id will not match up, so you should be able to rely on
> > >>>> the pointer being in the structure that the board sets up, right?
> > >>>>
> > >>>> Don't use names, especially as ids can, and will, change, that is
> > >>>> going
> > >>>> to cause big problems. Use pointers, this is C, we are supposed to
> > >>>> be
> > >>>> doing that :)
> > >>>
> > >>> Kishon, I think what Greg means is this: The name you are using
> > >>> must
> > >>> be stored somewhere in a data structure constructed by the board
> > >>> file,
> > >>> right? Or at least, associated with some data structure somehow.
> > >>> Otherwise the platform code wouldn't know which PHY hardware
> > >>> corresponded to a particular name.
> > >>>
> > >>> Greg's suggestion is that you store the address of that data
> > >>> structure
> > >>> in the platform data instead of storing the name string. Have the
> > >>> consumer pass the data structure's address when it calls phy_create,
> > >>> instead of passing the name. Then you don't have to worry about two
> > >>> PHYs accidentally ending up with the same name or any other similar
> > >>> problems.
> > >>
> > >> Close, but the issue is that whatever returns from phy_create()
> > >> should
> > >> then be used, no need to call any "find" functions, as you can just
> > >> use
> > >> the pointer that phy_create() returns. Much like all other class api
> > >> functions in the kernel work.
> > >
> > > I think there is a confusion here about who registers the PHYs.
> > >
> > > All platform code does is registering a platform/i2c/whatever device,
> > > which causes a driver (located in drivers/phy/) to be instantiated.
> > > Such drivers call phy_create(), usually in their probe() callbacks,
> > > so platform_code has no way (and should have no way, for the sake of
> > > layering) to get what phy_create() returns.
Why not put pointers in the platform data structure that can hold these
pointers? I thought that is why we created those structures in the
first place. If not, what are they there for?
> > > IMHO we need a lookup method for PHYs, just like for clocks,
> > > regulators, PWMs or even i2c busses because there are complex cases
> > > when passing just a name using platform data will not work. I would
> > > second what Stephen said [1] and define a structure doing things in a
> > > DT-like way.
> > >
> > > Example;
> > >
> > > [platform code]
> > >
> > > static const struct phy_lookup my_phy_lookup[] = {
> > >
> > > PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
> >
> > The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
> > creating the device, the ids in the device name would change and
> > PHY_LOOKUP wont be useful.
>
> I don't think this is a problem. All the existing lookup methods already
> use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
> can simply add a requirement that the ID must be assigned manually,
> without using PLATFORM_DEVID_AUTO to use PHY lookup.
And I'm saying that this idea, of using a specific name and id, is
frought with fragility and will break in the future in various ways when
devices get added to systems, making these strings constantly have to be
kept up to date with different board configurations.
People, NEVER, hardcode something like an id. The fact that this
happens today with the clock code, doesn't make it right, it makes the
clock code wrong. Others have already said that this is wrong there as
well, as systems change and dynamic ids get used more and more.
Let's not repeat the same mistakes of the past just because we refuse to
learn from them...
So again, the "find a phy by a string" functions should be removed, the
device id should be automatically created by the phy core just to make
things unique in sysfs, and no driver code should _ever_ be reliant on
the number that is being created, and the pointer to the phy structure
should be used everywhere instead.
With those types of changes, I will consider merging this subsystem, but
without them, sorry, I will not.
thanks,
greg k-h
^ permalink raw reply
* kmemleak warning in efifb_probe
From: Alexandra N. Kossovsky @ 2013-07-21 15:11 UTC (permalink / raw)
To: Peter Jones, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-fbdev, linux-kernel
Hello.
I am running linux-3.10.0 with kmemleak and see following warnings
in /sys/kernel/debug/kmemleak:
unreferenced object 0xffff880216fcfe00 (size 512):
comm "swapper/0", pid 1, jiffies 4294895429 (age 1415.320s)
hex dump (first 32 bytes):
00 00 00 00 00 00 00 00 aa aa aa aa aa aa aa aa ................
55 55 55 55 55 55 55 55 ff ff ff ff ff ff ff ff UUUUUUUU........
backtrace:
[<ffffffff813e415c>] kmemleak_alloc+0x21/0x3e
[<ffffffff8111c17f>] kmemleak_alloc_recursive.constprop.57+0x16/0x18
[<ffffffff8111e63b>] __kmalloc+0xf9/0x144
[<ffffffff8123d9cf>] fb_alloc_cmap_gfp+0x47/0xe1
[<ffffffff8123da77>] fb_alloc_cmap+0xe/0x10
[<ffffffff81aff40a>] efifb_probe+0x3e9/0x48f
[<ffffffff812c566f>] platform_drv_probe+0x34/0x5e
[<ffffffff812c3e6d>] driver_probe_device+0x98/0x1b4
[<ffffffff812c3fd7>] __driver_attach+0x4e/0x6f
[<ffffffff812c25bf>] bus_for_each_dev+0x57/0x8a
[<ffffffff812c3984>] driver_attach+0x19/0x1b
[<ffffffff812c362b>] bus_add_driver+0xde/0x201
[<ffffffff812c453f>] driver_register+0x8c/0x110
[<ffffffff812c510d>] platform_driver_register+0x41/0x43
[<ffffffff812c5127>] platform_driver_probe+0x18/0x8a
[<ffffffff81aff002>] efifb_init+0x276/0x295
unreferenced object 0xffff880205e90e00 (size 512):
comm "swapper/0", pid 1, jiffies 4294895429 (age 1415.320s)
hex dump (first 32 bytes):
00 00 00 00 aa aa aa aa 00 00 00 00 55 55 aa aa ............UU..
55 55 55 55 ff ff ff ff 55 55 55 55 ff ff ff ff UUUU....UUUU....
backtrace:
[<ffffffff813e415c>] kmemleak_alloc+0x21/0x3e
[<ffffffff8111c17f>] kmemleak_alloc_recursive.constprop.57+0x16/0x18
[<ffffffff8111e63b>] __kmalloc+0xf9/0x144
[<ffffffff8123d9ea>] fb_alloc_cmap_gfp+0x62/0xe1
[<ffffffff8123da77>] fb_alloc_cmap+0xe/0x10
[<ffffffff81aff40a>] efifb_probe+0x3e9/0x48f
[<ffffffff812c566f>] platform_drv_probe+0x34/0x5e
[<ffffffff812c3e6d>] driver_probe_device+0x98/0x1b4
[<ffffffff812c3fd7>] __driver_attach+0x4e/0x6f
[<ffffffff812c25bf>] bus_for_each_dev+0x57/0x8a
[<ffffffff812c3984>] driver_attach+0x19/0x1b
[<ffffffff812c362b>] bus_add_driver+0xde/0x201
[<ffffffff812c453f>] driver_register+0x8c/0x110
[<ffffffff812c510d>] platform_driver_register+0x41/0x43
[<ffffffff812c5127>] platform_driver_probe+0x18/0x8a
[<ffffffff81aff002>] efifb_init+0x276/0x295
unreferenced object 0xffff880205e91e00 (size 512):
comm "swapper/0", pid 1, jiffies 4294895429 (age 1415.320s)
hex dump (first 32 bytes):
00 00 aa aa 00 00 aa aa 00 00 aa aa 00 00 aa aa ................
55 55 ff ff 55 55 ff ff 55 55 ff ff 55 55 ff ff UU..UU..UU..UU..
backtrace:
[<ffffffff813e415c>] kmemleak_alloc+0x21/0x3e
[<ffffffff8111c17f>] kmemleak_alloc_recursive.constprop.57+0x16/0x18
[<ffffffff8111e63b>] __kmalloc+0xf9/0x144
[<ffffffff8123d9fe>] fb_alloc_cmap_gfp+0x76/0xe1
[<ffffffff8123da77>] fb_alloc_cmap+0xe/0x10
[<ffffffff81aff40a>] efifb_probe+0x3e9/0x48f
[<ffffffff812c566f>] platform_drv_probe+0x34/0x5e
[<ffffffff812c3e6d>] driver_probe_device+0x98/0x1b4
[<ffffffff812c3fd7>] __driver_attach+0x4e/0x6f
[<ffffffff812c25bf>] bus_for_each_dev+0x57/0x8a
[<ffffffff812c3984>] driver_attach+0x19/0x1b
[<ffffffff812c362b>] bus_add_driver+0xde/0x201
[<ffffffff812c453f>] driver_register+0x8c/0x110
[<ffffffff812c510d>] platform_driver_register+0x41/0x43
[<ffffffff812c5127>] platform_driver_probe+0x18/0x8a
[<ffffffff81aff002>] efifb_init+0x276/0x295
unreferenced object 0xffff880205e942e0 (size 32):
comm "swapper/0", pid 1, jiffies 4294895429 (age 1415.344s)
hex dump (first 32 bytes):
00 01 10 00 00 00 ad de 00 02 20 00 00 00 ad de .......... .....
00 2c e9 05 02 88 ff ff 01 5a 5a 5a 5a 5a 5a a5 .,.......ZZZZZZ.
backtrace:
[<ffffffff813e415c>] kmemleak_alloc+0x21/0x3e
[<ffffffff8111c17f>] kmemleak_alloc_recursive.constprop.57+0x16/0x18
[<ffffffff8111e76c>] kmem_cache_alloc_trace+0xe6/0x12e
[<ffffffff8107bb34>] pm_vt_switch_required+0x54/0x86
[<ffffffff8123addb>] register_framebuffer+0x1e0/0x294
[<ffffffff81aff429>] efifb_probe+0x408/0x48f
[<ffffffff812c566f>] platform_drv_probe+0x34/0x5e
[<ffffffff812c3e6d>] driver_probe_device+0x98/0x1b4
[<ffffffff812c3fd7>] __driver_attach+0x4e/0x6f
[<ffffffff812c25bf>] bus_for_each_dev+0x57/0x8a
[<ffffffff812c3984>] driver_attach+0x19/0x1b
[<ffffffff812c362b>] bus_add_driver+0xde/0x201
[<ffffffff812c453f>] driver_register+0x8c/0x110
[<ffffffff812c510d>] platform_driver_register+0x41/0x43
[<ffffffff812c5127>] platform_driver_probe+0x18/0x8a
[<ffffffff81aff002>] efifb_init+0x276/0x295
Feel free to ask for more info about my system;
I can also try a patch.
--
Alexandra N. Kossovsky
OKTET Labs (http://www.oktetlabs.ru/)
e-mail: sasha@oktetlabs.ru
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-21 11:19 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <3839600.WiC1OLF35o@flatron>
Hi,
On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
> Hi,
>
> On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
>> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
>>> On Sat, 20 Jul 2013, Greg KH wrote:
>>>>>>> That should be passed using platform data.
>>>>>>
>>>>>> Ick, don't pass strings around, pass pointers. If you have
>>>>>> platform
>>>>>> data you can get to, then put the pointer there, don't use a
>>>>>> "name".
>>>>>
>>>>> I don't think I understood you here :-s We wont have phy pointer
>>>>> when we create the device for the controller no?(it'll be done in
>>>>> board file). Probably I'm missing something.
>>>>
>>>> Why will you not have that pointer? You can't rely on the "name" as
>>>> the device id will not match up, so you should be able to rely on
>>>> the pointer being in the structure that the board sets up, right?
>>>>
>>>> Don't use names, especially as ids can, and will, change, that is
>>>> going
>>>> to cause big problems. Use pointers, this is C, we are supposed to
>>>> be
>>>> doing that :)
>>>
>>> Kishon, I think what Greg means is this: The name you are using must
>>> be stored somewhere in a data structure constructed by the board file,
>>> right? Or at least, associated with some data structure somehow.
>>> Otherwise the platform code wouldn't know which PHY hardware
>>> corresponded to a particular name.
>>>
>>> Greg's suggestion is that you store the address of that data structure
>>> in the platform data instead of storing the name string. Have the
>>> consumer pass the data structure's address when it calls phy_create,
>>> instead of passing the name. Then you don't have to worry about two
>>> PHYs accidentally ending up with the same name or any other similar
>>> problems.
>>
>> Close, but the issue is that whatever returns from phy_create() should
>> then be used, no need to call any "find" functions, as you can just use
>> the pointer that phy_create() returns. Much like all other class api
>> functions in the kernel work.
>
> I think there is a confusion here about who registers the PHYs.
>
> All platform code does is registering a platform/i2c/whatever device,
> which causes a driver (located in drivers/phy/) to be instantiated. Such
> drivers call phy_create(), usually in their probe() callbacks, so
> platform_code has no way (and should have no way, for the sake of
> layering) to get what phy_create() returns.
right.
>
> IMHO we need a lookup method for PHYs, just like for clocks, regulators,
> PWMs or even i2c busses because there are complex cases when passing just
> a name using platform data will not work. I would second what Stephen said
> [1] and define a structure doing things in a DT-like way.
>
> Example;
>
> [platform code]
>
> static const struct phy_lookup my_phy_lookup[] = {
> PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
creating the device, the ids in the device name would change and
PHY_LOOKUP wont be useful.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-21 11:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51EBC0F5.70601@ti.com>
On Sunday 21 of July 2013 16:37:33 Kishon Vijay Abraham I wrote:
> Hi,
>
> On Sunday 21 July 2013 04:01 PM, Tomasz Figa wrote:
> > Hi,
> >
> > On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
> >> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> >>> On Sat, 20 Jul 2013, Greg KH wrote:
> >>>>>>> That should be passed using platform data.
> >>>>>>
> >>>>>> Ick, don't pass strings around, pass pointers. If you have
> >>>>>> platform
> >>>>>> data you can get to, then put the pointer there, don't use a
> >>>>>> "name".
> >>>>>
> >>>>> I don't think I understood you here :-s We wont have phy pointer
> >>>>> when we create the device for the controller no?(it'll be done in
> >>>>> board file). Probably I'm missing something.
> >>>>
> >>>> Why will you not have that pointer? You can't rely on the "name"
> >>>> as
> >>>> the device id will not match up, so you should be able to rely on
> >>>> the pointer being in the structure that the board sets up, right?
> >>>>
> >>>> Don't use names, especially as ids can, and will, change, that is
> >>>> going
> >>>> to cause big problems. Use pointers, this is C, we are supposed to
> >>>> be
> >>>> doing that :)
> >>>
> >>> Kishon, I think what Greg means is this: The name you are using
> >>> must
> >>> be stored somewhere in a data structure constructed by the board
> >>> file,
> >>> right? Or at least, associated with some data structure somehow.
> >>> Otherwise the platform code wouldn't know which PHY hardware
> >>> corresponded to a particular name.
> >>>
> >>> Greg's suggestion is that you store the address of that data
> >>> structure
> >>> in the platform data instead of storing the name string. Have the
> >>> consumer pass the data structure's address when it calls phy_create,
> >>> instead of passing the name. Then you don't have to worry about two
> >>> PHYs accidentally ending up with the same name or any other similar
> >>> problems.
> >>
> >> Close, but the issue is that whatever returns from phy_create()
> >> should
> >> then be used, no need to call any "find" functions, as you can just
> >> use
> >> the pointer that phy_create() returns. Much like all other class api
> >> functions in the kernel work.
> >
> > I think there is a confusion here about who registers the PHYs.
> >
> > All platform code does is registering a platform/i2c/whatever device,
> > which causes a driver (located in drivers/phy/) to be instantiated.
> > Such drivers call phy_create(), usually in their probe() callbacks,
> > so platform_code has no way (and should have no way, for the sake of
> > layering) to get what phy_create() returns.
>
> right.
>
> > IMHO we need a lookup method for PHYs, just like for clocks,
> > regulators, PWMs or even i2c busses because there are complex cases
> > when passing just a name using platform data will not work. I would
> > second what Stephen said [1] and define a structure doing things in a
> > DT-like way.
> >
> > Example;
> >
> > [platform code]
> >
> > static const struct phy_lookup my_phy_lookup[] = {
> >
> > PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
>
> The only problem here is that if *PLATFORM_DEVID_AUTO* is used while
> creating the device, the ids in the device name would change and
> PHY_LOOKUP wont be useful.
I don't think this is a problem. All the existing lookup methods already
use ID to identify devices (see regulators, clkdev, PWMs, i2c, ...). You
can simply add a requirement that the ID must be assigned manually,
without using PLATFORM_DEVID_AUTO to use PHY lookup.
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Tomasz Figa @ 2013-07-21 10:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130721025910.GA23043@kroah.com>
Hi,
On Saturday 20 of July 2013 19:59:10 Greg KH wrote:
> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > On Sat, 20 Jul 2013, Greg KH wrote:
> > > > >>That should be passed using platform data.
> > > > >
> > > > >Ick, don't pass strings around, pass pointers. If you have
> > > > >platform
> > > > >data you can get to, then put the pointer there, don't use a
> > > > >"name".
> > > >
> > > > I don't think I understood you here :-s We wont have phy pointer
> > > > when we create the device for the controller no?(it'll be done in
> > > > board file). Probably I'm missing something.
> > >
> > > Why will you not have that pointer? You can't rely on the "name" as
> > > the device id will not match up, so you should be able to rely on
> > > the pointer being in the structure that the board sets up, right?
> > >
> > > Don't use names, especially as ids can, and will, change, that is
> > > going
> > > to cause big problems. Use pointers, this is C, we are supposed to
> > > be
> > > doing that :)
> >
> > Kishon, I think what Greg means is this: The name you are using must
> > be stored somewhere in a data structure constructed by the board file,
> > right? Or at least, associated with some data structure somehow.
> > Otherwise the platform code wouldn't know which PHY hardware
> > corresponded to a particular name.
> >
> > Greg's suggestion is that you store the address of that data structure
> > in the platform data instead of storing the name string. Have the
> > consumer pass the data structure's address when it calls phy_create,
> > instead of passing the name. Then you don't have to worry about two
> > PHYs accidentally ending up with the same name or any other similar
> > problems.
>
> Close, but the issue is that whatever returns from phy_create() should
> then be used, no need to call any "find" functions, as you can just use
> the pointer that phy_create() returns. Much like all other class api
> functions in the kernel work.
I think there is a confusion here about who registers the PHYs.
All platform code does is registering a platform/i2c/whatever device,
which causes a driver (located in drivers/phy/) to be instantiated. Such
drivers call phy_create(), usually in their probe() callbacks, so
platform_code has no way (and should have no way, for the sake of
layering) to get what phy_create() returns.
IMHO we need a lookup method for PHYs, just like for clocks, regulators,
PWMs or even i2c busses because there are complex cases when passing just
a name using platform data will not work. I would second what Stephen said
[1] and define a structure doing things in a DT-like way.
Example;
[platform code]
static const struct phy_lookup my_phy_lookup[] = {
PHY_LOOKUP("s3c-hsotg.0", "otg", "samsung-usbphy.1", "phy.2"),
/* etc... */
};
static void my_machine_init(void)
{
/* ... */
phy_register_lookup(my_phy_lookup, ARRAY_SIZE(my_phy_lookup));
/* ... */
}
/* Notice nothing stuffed in platform data. */
[provider code - samsung-usbphy driver]
static int samsung_usbphy_probe(...)
{
/* ... */
for (i = 0; i < PHY_COUNT; ++i) {
usbphy->phy[i].name = "phy";
usbphy->phy[i].id = i;
/* ... */
ret = phy_create(&usbphy->phy);
/* err handling */
}
/* ... */
}
[consumer code - s3c-hsotg driver]
static int s3c_hsotg_probe(...)
{
/* ... */
phy = phy_get(&pdev->dev, "otg");
/* ... */
}
[1] http://thread.gmane.org/gmane.linux.ports.arm.kernel/252813
Best regards,
Tomasz
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Sascha Hauer @ 2013-07-21 10:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130721025910.GA23043@kroah.com>
On Sat, Jul 20, 2013 at 07:59:10PM -0700, Greg KH wrote:
> On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> > On Sat, 20 Jul 2013, Greg KH wrote:
> >
> > > > >>That should be passed using platform data.
> > > > >
> > > > >Ick, don't pass strings around, pass pointers. If you have platform
> > > > >data you can get to, then put the pointer there, don't use a "name".
> > > >
> > > > I don't think I understood you here :-s We wont have phy pointer
> > > > when we create the device for the controller no?(it'll be done in
> > > > board file). Probably I'm missing something.
> > >
> > > Why will you not have that pointer? You can't rely on the "name" as the
> > > device id will not match up, so you should be able to rely on the
> > > pointer being in the structure that the board sets up, right?
> > >
> > > Don't use names, especially as ids can, and will, change, that is going
> > > to cause big problems. Use pointers, this is C, we are supposed to be
> > > doing that :)
> >
> > Kishon, I think what Greg means is this: The name you are using must
> > be stored somewhere in a data structure constructed by the board file,
> > right? Or at least, associated with some data structure somehow.
> > Otherwise the platform code wouldn't know which PHY hardware
> > corresponded to a particular name.
> >
> > Greg's suggestion is that you store the address of that data structure
> > in the platform data instead of storing the name string. Have the
> > consumer pass the data structure's address when it calls phy_create,
> > instead of passing the name. Then you don't have to worry about two
> > PHYs accidentally ending up with the same name or any other similar
> > problems.
>
> Close, but the issue is that whatever returns from phy_create() should
> then be used, no need to call any "find" functions, as you can just use
> the pointer that phy_create() returns. Much like all other class api
> functions in the kernel work.
I think the problem here is to connect two from the bus structure
completely independent devices. Several frameworks (ASoC, soc-camera)
had this problem and this wasn't solved until the advent of devicetrees
and their phandles.
phy_create might be called from the probe function of some i2c device
(the phy device) and the resulting pointer is then needed in some other
platform devices (the user of the phy) probe function.
The best solution we have right now is implemented in the clk framework
which uses a string matching of the device names in clk_get() (at least
in the non-dt case).
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH 2/2] video: imxfb: Add feature to setup PWM Contrast Control Register
From: Alexander Shiyan @ 2013-07-21 8:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374395709-20898-1-git-send-email-shc_work@mail.ru>
This patch adds feature to setup PWM Contrast Control Register.
This register is used to control the signal output at the contrast pin,
which controls contrast of the LCD panel.
Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
Documentation/devicetree/bindings/video/fsl,imx-fb.txt | 1 +
drivers/video/imxfb.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
index 46da08d..be10c65 100644
--- a/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
+++ b/Documentation/devicetree/bindings/video/fsl,imx-fb.txt
@@ -18,6 +18,7 @@ Optional properties:
- fsl,dmacr: DMA Control Register value. This is optional. By default, the
register is not modified as recommended by the datasheet.
- fsl,lscr1: LCDC Sharp Configuration Register value.
+- fsl,pwmr: LCDC PWM Contrast Control Register value.
Example:
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 8e104c4..d98299a 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -806,8 +806,8 @@ static int imxfb_init_fbinfo(struct platform_device *pdev,
fbi->lscr1 = IMXFB_LSCR1_DEFAULT;
of_property_read_u32(np, "fsl,lscr1", &fbi->lscr1);
-
of_property_read_u32(np, "fsl,dmacr", &fbi->dmacr);
+ of_property_read_u32(np, "fsl,pwmr", &fbi->pwmr);
/* These two function pointers could be used by some specific
* platforms. */
--
1.8.1.5
^ permalink raw reply related
* [PATCH 1/2] video: imxfb: Fix retrieve values from DT
From: Alexander Shiyan @ 2013-07-21 8:35 UTC (permalink / raw)
To: linux-arm-kernel
Display settings should be retrieved from "display" node, not from
root fb node. This patch fix this bug.
Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
drivers/video/imxfb.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/video/imxfb.c b/drivers/video/imxfb.c
index 38733ac..8e104c4 100644
--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -753,12 +753,12 @@ static int imxfb_resume(struct platform_device *dev)
#define imxfb_resume NULL
#endif
-static int imxfb_init_fbinfo(struct platform_device *pdev)
+static int imxfb_init_fbinfo(struct platform_device *pdev,
+ struct device_node *np)
{
struct imx_fb_platform_data *pdata = pdev->dev.platform_data;
struct fb_info *info = dev_get_drvdata(&pdev->dev);
struct imxfb_info *fbi = info->par;
- struct device_node *np;
pr_debug("%s\n",__func__);
@@ -799,7 +799,6 @@ static int imxfb_init_fbinfo(struct platform_device *pdev)
fbi->lcd_power = pdata->lcd_power;
fbi->backlight_power = pdata->backlight_power;
} else {
- np = pdev->dev.of_node;
info->var.grayscale = of_property_read_bool(np,
"cmap-greyscale");
fbi->cmap_inverse = of_property_read_bool(np, "cmap-inverse");
@@ -858,6 +857,7 @@ static int imxfb_of_read_mode(struct device *dev, struct device_node *np,
static int imxfb_probe(struct platform_device *pdev)
{
+ struct device_node *display_np = NULL;
struct imxfb_info *fbi;
struct fb_info *info;
struct imx_fb_platform_data *pdata;
@@ -887,7 +887,17 @@ static int imxfb_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, info);
- ret = imxfb_init_fbinfo(pdev);
+ if (pdev->dev.of_node) {
+ display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
+ if (!display_np) {
+ dev_err(&pdev->dev,
+ "No display defined in devicetree\n");
+ ret = -EINVAL;
+ goto failed_init;
+ }
+ }
+
+ ret = imxfb_init_fbinfo(pdev, display_np);
if (ret < 0)
goto failed_init;
@@ -898,16 +908,8 @@ static int imxfb_probe(struct platform_device *pdev)
fbi->mode = pdata->mode;
fbi->num_modes = pdata->num_modes;
} else {
- struct device_node *display_np;
fb_mode = NULL;
- display_np = of_parse_phandle(pdev->dev.of_node, "display", 0);
- if (!display_np) {
- dev_err(&pdev->dev, "No display defined in devicetree\n");
- ret = -EINVAL;
- goto failed_of_parse;
- }
-
/*
* imxfb does not support more modes, we choose only the native
* mode.
--
1.8.1.5
^ permalink raw reply related
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-21 2:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <Pine.LNX.4.44L0.1307202223430.8250-100000@netrider.rowland.org>
On Sat, Jul 20, 2013 at 10:32:26PM -0400, Alan Stern wrote:
> On Sat, 20 Jul 2013, Greg KH wrote:
>
> > > >>That should be passed using platform data.
> > > >
> > > >Ick, don't pass strings around, pass pointers. If you have platform
> > > >data you can get to, then put the pointer there, don't use a "name".
> > >
> > > I don't think I understood you here :-s We wont have phy pointer
> > > when we create the device for the controller no?(it'll be done in
> > > board file). Probably I'm missing something.
> >
> > Why will you not have that pointer? You can't rely on the "name" as the
> > device id will not match up, so you should be able to rely on the
> > pointer being in the structure that the board sets up, right?
> >
> > Don't use names, especially as ids can, and will, change, that is going
> > to cause big problems. Use pointers, this is C, we are supposed to be
> > doing that :)
>
> Kishon, I think what Greg means is this: The name you are using must
> be stored somewhere in a data structure constructed by the board file,
> right? Or at least, associated with some data structure somehow.
> Otherwise the platform code wouldn't know which PHY hardware
> corresponded to a particular name.
>
> Greg's suggestion is that you store the address of that data structure
> in the platform data instead of storing the name string. Have the
> consumer pass the data structure's address when it calls phy_create,
> instead of passing the name. Then you don't have to worry about two
> PHYs accidentally ending up with the same name or any other similar
> problems.
Close, but the issue is that whatever returns from phy_create() should
then be used, no need to call any "find" functions, as you can just use
the pointer that phy_create() returns. Much like all other class api
functions in the kernel work.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Alan Stern @ 2013-07-21 2:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1374129984-765-2-git-send-email-kishon@ti.com>
On Sat, 20 Jul 2013, Greg KH wrote:
> > >>That should be passed using platform data.
> > >
> > >Ick, don't pass strings around, pass pointers. If you have platform
> > >data you can get to, then put the pointer there, don't use a "name".
> >
> > I don't think I understood you here :-s We wont have phy pointer
> > when we create the device for the controller no?(it'll be done in
> > board file). Probably I'm missing something.
>
> Why will you not have that pointer? You can't rely on the "name" as the
> device id will not match up, so you should be able to rely on the
> pointer being in the structure that the board sets up, right?
>
> Don't use names, especially as ids can, and will, change, that is going
> to cause big problems. Use pointers, this is C, we are supposed to be
> doing that :)
Kishon, I think what Greg means is this: The name you are using must
be stored somewhere in a data structure constructed by the board file,
right? Or at least, associated with some data structure somehow.
Otherwise the platform code wouldn't know which PHY hardware
corresponded to a particular name.
Greg's suggestion is that you store the address of that data structure
in the platform data instead of storing the name string. Have the
consumer pass the data structure's address when it calls phy_create,
instead of passing the name. Then you don't have to worry about two
PHYs accidentally ending up with the same name or any other similar
problems.
Alan Stern
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-20 22:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51EA01C4.2010006@ti.com>
On Sat, Jul 20, 2013 at 08:49:32AM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Saturday 20 July 2013 05:20 AM, Greg KH wrote:
> >On Fri, Jul 19, 2013 at 12:06:01PM +0530, Kishon Vijay Abraham I wrote:
> >>Hi,
> >>
> >>On Friday 19 July 2013 11:59 AM, Greg KH wrote:
> >>>On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
> >>>>Hi,
> >>>>
> >>>>On Friday 19 July 2013 11:13 AM, Greg KH wrote:
> >>>>>On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
> >>>>>>>>>>+ ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
> >>>>>>>>>
> >>>>>>>>>Your naming is odd, no "phy" anywhere in it? You rely on the sender to
> >>>>>>>>>never send a duplicate name.id pair? Why not create your own ids based
> >>>>>>>>>on the number of phys in the system, like almost all other classes and
> >>>>>>>>>subsystems do?
> >>>>>>>>
> >>>>>>>>hmm.. some PHY drivers use the id they provide to perform some of their
> >>>>>>>>internal operation as in [1] (This is used only if a single PHY provider
> >>>>>>>>implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
> >>>>>>>>to give the PHY drivers an option to use auto id.
> >>>>>>>>
> >>>>>>>>[1] ->
> >>>>>>>>http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
> >>>>>>>
> >>>>>>>No, who cares about the id? No one outside of the phy core ever should,
> >>>>>>>because you pass back the only pointer that they really do care about,
> >>>>>>>if they need to do anything with the device. Use that, and then you can
> >>>>>>
> >>>>>>hmm.. ok.
> >>>>>>
> >>>>>>>rip out all of the "search for a phy by a string" logic, as that's not
> >>>>>>
> >>>>>>Actually this is needed for non-dt boot case. In the case of dt boot, we use a
> >>>>>>phandle by which the controller can get a reference to the phy. But in the case
> >>>>>>of non-dt boot, the controller can get a reference to the phy only by label.
> >>>>>
> >>>>>I don't understand. They registered the phy, and got back a pointer to
> >>>>>it. Why can't they save it in their local structure to use it again
> >>>>>later if needed? They should never have to "ask" for the device, as the
> >>>>
> >>>>One is a *PHY provider* driver which is a driver for some PHY device. This will
> >>>>use phy_create to create the phy.
> >>>>The other is a *PHY consumer* driver which might be any controller driver (can
> >>>>be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
> >>>>phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
> >>>>>device id might be unknown if there are multiple devices in the system.
> >>>>
> >>>>I agree with you on the device id part. That need not be known to the PHY driver.
> >>>
> >>>How does a consumer know which "label" to use in a non-dt system if
> >>>there are multiple PHYs in the system?
> >>
> >>That should be passed using platform data.
> >
> >Ick, don't pass strings around, pass pointers. If you have platform
> >data you can get to, then put the pointer there, don't use a "name".
>
> I don't think I understood you here :-s We wont have phy pointer
> when we create the device for the controller no?(it'll be done in
> board file). Probably I'm missing something.
Why will you not have that pointer? You can't rely on the "name" as the
device id will not match up, so you should be able to rely on the
pointer being in the structure that the board sets up, right?
Don't use names, especially as ids can, and will, change, that is going
to cause big problems. Use pointers, this is C, we are supposed to be
doing that :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-20 3:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130719235055.GB11498@kroah.com>
Hi,
On Saturday 20 July 2013 05:20 AM, Greg KH wrote:
> On Fri, Jul 19, 2013 at 12:06:01PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Friday 19 July 2013 11:59 AM, Greg KH wrote:
>>> On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
>>>>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>>>>>>>
>>>>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>>>>>>>> never send a duplicate name.id pair? Why not create your own ids based
>>>>>>>>> on the number of phys in the system, like almost all other classes and
>>>>>>>>> subsystems do?
>>>>>>>>
>>>>>>>> hmm.. some PHY drivers use the id they provide to perform some of their
>>>>>>>> internal operation as in [1] (This is used only if a single PHY provider
>>>>>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>>>>>>>> to give the PHY drivers an option to use auto id.
>>>>>>>>
>>>>>>>> [1] ->
>>>>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>>>>>>>
>>>>>>> No, who cares about the id? No one outside of the phy core ever should,
>>>>>>> because you pass back the only pointer that they really do care about,
>>>>>>> if they need to do anything with the device. Use that, and then you can
>>>>>>
>>>>>> hmm.. ok.
>>>>>>
>>>>>>> rip out all of the "search for a phy by a string" logic, as that's not
>>>>>>
>>>>>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
>>>>>> phandle by which the controller can get a reference to the phy. But in the case
>>>>>> of non-dt boot, the controller can get a reference to the phy only by label.
>>>>>
>>>>> I don't understand. They registered the phy, and got back a pointer to
>>>>> it. Why can't they save it in their local structure to use it again
>>>>> later if needed? They should never have to "ask" for the device, as the
>>>>
>>>> One is a *PHY provider* driver which is a driver for some PHY device. This will
>>>> use phy_create to create the phy.
>>>> The other is a *PHY consumer* driver which might be any controller driver (can
>>>> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
>>>> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
>>>>> device id might be unknown if there are multiple devices in the system.
>>>>
>>>> I agree with you on the device id part. That need not be known to the PHY driver.
>>>
>>> How does a consumer know which "label" to use in a non-dt system if
>>> there are multiple PHYs in the system?
>>
>> That should be passed using platform data.
>
> Ick, don't pass strings around, pass pointers. If you have platform
> data you can get to, then put the pointer there, don't use a "name".
I don't think I understood you here :-s We wont have phy pointer when we
create the device for the controller no?(it'll be done in board file).
Probably I'm missing something.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-20 3:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51E96135.9090108@wwwdotorg.org>
Hi,
On Friday 19 July 2013 09:24 PM, Stephen Warren wrote:
> On 07/19/2013 12:36 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Friday 19 July 2013 11:59 AM, Greg KH wrote:
>>> On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
>>>> Hi,
>>>>
>>>> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
>>>>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>>>>>>>
>>>>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>>>>>>>> never send a duplicate name.id pair? Why not create your own ids based
>>>>>>>>> on the number of phys in the system, like almost all other classes and
>>>>>>>>> subsystems do?
>>>>>>>>
>>>>>>>> hmm.. some PHY drivers use the id they provide to perform some of their
>>>>>>>> internal operation as in [1] (This is used only if a single PHY provider
>>>>>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>>>>>>>> to give the PHY drivers an option to use auto id.
>>>>>>>>
>>>>>>>> [1] ->
>>>>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>>>>>>>
>>>>>>> No, who cares about the id? No one outside of the phy core ever should,
>>>>>>> because you pass back the only pointer that they really do care about,
>>>>>>> if they need to do anything with the device. Use that, and then you can
>>>>>>
>>>>>> hmm.. ok.
>>>>>>
>>>>>>> rip out all of the "search for a phy by a string" logic, as that's not
>>>>>>
>>>>>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
>>>>>> phandle by which the controller can get a reference to the phy. But in the case
>>>>>> of non-dt boot, the controller can get a reference to the phy only by label.
>>>>>
>>>>> I don't understand. They registered the phy, and got back a pointer to
>>>>> it. Why can't they save it in their local structure to use it again
>>>>> later if needed? They should never have to "ask" for the device, as the
>>>>
>>>> One is a *PHY provider* driver which is a driver for some PHY device. This will
>>>> use phy_create to create the phy.
>>>> The other is a *PHY consumer* driver which might be any controller driver (can
>>>> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
>>>> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
>>>>> device id might be unknown if there are multiple devices in the system.
>>>>
>>>> I agree with you on the device id part. That need not be known to the PHY driver.
>>>
>>> How does a consumer know which "label" to use in a non-dt system if
>>> there are multiple PHYs in the system?
>>
>> That should be passed using platform data.
>
> I don't think that's right. That's just like passing clock names in
> platform data, which I believe is frowned upon.
>
> Instead, why not take the approach that e.g. regulators have taken? Each
> driver defines the names of the objects that it requires. There is a
> table (registered by a board file) that has lookup key (device name,
We were using a similar approach with USB PHY layer but things started
breaking after the device names are created using PLATFORM_DEVID_AUTO.
Now theres no way to reliably know the device names in advance.
Btw I had such device name binding in my v3 of this patch series [1] and
had some discussion with Grant during that time [2].
[1] ->
http://archive.arm.linux.org.uk/lurker/message/20130320.091200.721a6fb5.hu.html
[2] -> https://lkml.org/lkml/2013/4/22/26
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-19 23:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51E8DE51.1060404@ti.com>
On Fri, Jul 19, 2013 at 12:06:01PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 19 July 2013 11:59 AM, Greg KH wrote:
> > On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
> >> Hi,
> >>
> >> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
> >>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
> >>>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
> >>>>>>>
> >>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
> >>>>>>> never send a duplicate name.id pair? Why not create your own ids based
> >>>>>>> on the number of phys in the system, like almost all other classes and
> >>>>>>> subsystems do?
> >>>>>>
> >>>>>> hmm.. some PHY drivers use the id they provide to perform some of their
> >>>>>> internal operation as in [1] (This is used only if a single PHY provider
> >>>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
> >>>>>> to give the PHY drivers an option to use auto id.
> >>>>>>
> >>>>>> [1] ->
> >>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
> >>>>>
> >>>>> No, who cares about the id? No one outside of the phy core ever should,
> >>>>> because you pass back the only pointer that they really do care about,
> >>>>> if they need to do anything with the device. Use that, and then you can
> >>>>
> >>>> hmm.. ok.
> >>>>
> >>>>> rip out all of the "search for a phy by a string" logic, as that's not
> >>>>
> >>>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
> >>>> phandle by which the controller can get a reference to the phy. But in the case
> >>>> of non-dt boot, the controller can get a reference to the phy only by label.
> >>>
> >>> I don't understand. They registered the phy, and got back a pointer to
> >>> it. Why can't they save it in their local structure to use it again
> >>> later if needed? They should never have to "ask" for the device, as the
> >>
> >> One is a *PHY provider* driver which is a driver for some PHY device. This will
> >> use phy_create to create the phy.
> >> The other is a *PHY consumer* driver which might be any controller driver (can
> >> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
> >> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
> >>> device id might be unknown if there are multiple devices in the system.
> >>
> >> I agree with you on the device id part. That need not be known to the PHY driver.
> >
> > How does a consumer know which "label" to use in a non-dt system if
> > there are multiple PHYs in the system?
>
> That should be passed using platform data.
Ick, don't pass strings around, pass pointers. If you have platform
data you can get to, then put the pointer there, don't use a "name".
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Stephen Warren @ 2013-07-19 15:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51E8DE51.1060404@ti.com>
On 07/19/2013 12:36 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 19 July 2013 11:59 AM, Greg KH wrote:
>> On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
>>>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>>>>>>
>>>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>>>>>>> never send a duplicate name.id pair? Why not create your own ids based
>>>>>>>> on the number of phys in the system, like almost all other classes and
>>>>>>>> subsystems do?
>>>>>>>
>>>>>>> hmm.. some PHY drivers use the id they provide to perform some of their
>>>>>>> internal operation as in [1] (This is used only if a single PHY provider
>>>>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>>>>>>> to give the PHY drivers an option to use auto id.
>>>>>>>
>>>>>>> [1] ->
>>>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>>>>>>
>>>>>> No, who cares about the id? No one outside of the phy core ever should,
>>>>>> because you pass back the only pointer that they really do care about,
>>>>>> if they need to do anything with the device. Use that, and then you can
>>>>>
>>>>> hmm.. ok.
>>>>>
>>>>>> rip out all of the "search for a phy by a string" logic, as that's not
>>>>>
>>>>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
>>>>> phandle by which the controller can get a reference to the phy. But in the case
>>>>> of non-dt boot, the controller can get a reference to the phy only by label.
>>>>
>>>> I don't understand. They registered the phy, and got back a pointer to
>>>> it. Why can't they save it in their local structure to use it again
>>>> later if needed? They should never have to "ask" for the device, as the
>>>
>>> One is a *PHY provider* driver which is a driver for some PHY device. This will
>>> use phy_create to create the phy.
>>> The other is a *PHY consumer* driver which might be any controller driver (can
>>> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
>>> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
>>>> device id might be unknown if there are multiple devices in the system.
>>>
>>> I agree with you on the device id part. That need not be known to the PHY driver.
>>
>> How does a consumer know which "label" to use in a non-dt system if
>> there are multiple PHYs in the system?
>
> That should be passed using platform data.
I don't think that's right. That's just like passing clock names in
platform data, which I believe is frowned upon.
Instead, why not take the approach that e.g. regulators have taken? Each
driver defines the names of the objects that it requires. There is a
table (registered by a board file) that has lookup key (device name,
regulator name), and the output is the regulator device (or global
regulator name).
This is almost the same way that DT works, except that in DT, the table
is represented as properties in the DT. The DT binding defines the names
of those properties, or the strings that must be in the foo-names
properties, just like a driver in non-DT Linux is going to define the
names it expects to be provided with.
That way, you don't need platform data to define the names.
^ permalink raw reply
* Re: [PATCH 2/3] video: hx8357: Make IM pins optional
From: 'Maxime Ripard' @ 2013-07-19 8:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1373945380.7549.26.camel@marge.simpson.net>
[-- Attachment #1: Type: text/plain, Size: 1301 bytes --]
Hi Jingoo, Mike,
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
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");
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
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 0/3] Few ignored framebuffer fixes/additions
From: Maxime Ripard @ 2013-07-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130717143709.f8879fb7f0a5d55859e96838@linux-foundation.org>
[-- Attachment #1: Type: text/plain, Size: 708 bytes --]
Hi Andrew,
On Wed, Jul 17, 2013 at 02:37:09PM -0700, Andrew Morton wrote:
> On Mon, 15 Jul 2013 17:26:59 +0200 Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>
> > Sorry to bother you again with the framebuffer stuff but the new
> > maintainer doesn't appear to be responsive either.
>
> Jean-Christophe is doing things - on 11 July he sent out a call for
> late patches to linux-fbdev@vger.kernel.org.
>
> So hopefully the resend of this patch series will be handled
> appropriately?
Hopefully. We'll see how it turns out then, and let you know.
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- 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: Kishon Vijay Abraham I @ 2013-07-19 6:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130719062941.GA23611@kroah.com>
Hi,
On Friday 19 July 2013 11:59 AM, Greg KH wrote:
> On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
>>> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
>>>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>>>>>
>>>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>>>>>> never send a duplicate name.id pair? Why not create your own ids based
>>>>>>> on the number of phys in the system, like almost all other classes and
>>>>>>> subsystems do?
>>>>>>
>>>>>> hmm.. some PHY drivers use the id they provide to perform some of their
>>>>>> internal operation as in [1] (This is used only if a single PHY provider
>>>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>>>>>> to give the PHY drivers an option to use auto id.
>>>>>>
>>>>>> [1] ->
>>>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>>>>>
>>>>> No, who cares about the id? No one outside of the phy core ever should,
>>>>> because you pass back the only pointer that they really do care about,
>>>>> if they need to do anything with the device. Use that, and then you can
>>>>
>>>> hmm.. ok.
>>>>
>>>>> rip out all of the "search for a phy by a string" logic, as that's not
>>>>
>>>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
>>>> phandle by which the controller can get a reference to the phy. But in the case
>>>> of non-dt boot, the controller can get a reference to the phy only by label.
>>>
>>> I don't understand. They registered the phy, and got back a pointer to
>>> it. Why can't they save it in their local structure to use it again
>>> later if needed? They should never have to "ask" for the device, as the
>>
>> One is a *PHY provider* driver which is a driver for some PHY device. This will
>> use phy_create to create the phy.
>> The other is a *PHY consumer* driver which might be any controller driver (can
>> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
>> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
>>> device id might be unknown if there are multiple devices in the system.
>>
>> I agree with you on the device id part. That need not be known to the PHY driver.
>
> How does a consumer know which "label" to use in a non-dt system if
> there are multiple PHYs in the system?
That should be passed using platform data.
>
> Do you have any drivers that are non-dt using this yet?
yes. tw4030 (used in OMAP3) supports non-dt.
[PATCH 04/15] ARM: OMAP: USB: Add phy binding information
[PATCH 06/15] usb: musb: omap2430: use the new generic PHY framework
of this patch series shows how it's used.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Greg KH @ 2013-07-19 6:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <51E8D4E0.8060200@ti.com>
On Fri, Jul 19, 2013 at 11:25:44AM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Friday 19 July 2013 11:13 AM, Greg KH wrote:
> > On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
> >>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
> >>>>>
> >>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
> >>>>> never send a duplicate name.id pair? Why not create your own ids based
> >>>>> on the number of phys in the system, like almost all other classes and
> >>>>> subsystems do?
> >>>>
> >>>> hmm.. some PHY drivers use the id they provide to perform some of their
> >>>> internal operation as in [1] (This is used only if a single PHY provider
> >>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
> >>>> to give the PHY drivers an option to use auto id.
> >>>>
> >>>> [1] ->
> >>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
> >>>
> >>> No, who cares about the id? No one outside of the phy core ever should,
> >>> because you pass back the only pointer that they really do care about,
> >>> if they need to do anything with the device. Use that, and then you can
> >>
> >> hmm.. ok.
> >>
> >>> rip out all of the "search for a phy by a string" logic, as that's not
> >>
> >> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
> >> phandle by which the controller can get a reference to the phy. But in the case
> >> of non-dt boot, the controller can get a reference to the phy only by label.
> >
> > I don't understand. They registered the phy, and got back a pointer to
> > it. Why can't they save it in their local structure to use it again
> > later if needed? They should never have to "ask" for the device, as the
>
> One is a *PHY provider* driver which is a driver for some PHY device. This will
> use phy_create to create the phy.
> The other is a *PHY consumer* driver which might be any controller driver (can
> be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
> phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
> > device id might be unknown if there are multiple devices in the system.
>
> I agree with you on the device id part. That need not be known to the PHY driver.
How does a consumer know which "label" to use in a non-dt system if
there are multiple PHYs in the system?
Do you have any drivers that are non-dt using this yet?
greg k-h
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-19 5:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130719054311.GA14638@kroah.com>
Hi,
On Friday 19 July 2013 11:13 AM, Greg KH wrote:
> On Fri, Jul 19, 2013 at 11:07:10AM +0530, Kishon Vijay Abraham I wrote:
>>>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>>>
>>>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>>>> never send a duplicate name.id pair? Why not create your own ids based
>>>>> on the number of phys in the system, like almost all other classes and
>>>>> subsystems do?
>>>>
>>>> hmm.. some PHY drivers use the id they provide to perform some of their
>>>> internal operation as in [1] (This is used only if a single PHY provider
>>>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>>>> to give the PHY drivers an option to use auto id.
>>>>
>>>> [1] ->
>>>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>>>
>>> No, who cares about the id? No one outside of the phy core ever should,
>>> because you pass back the only pointer that they really do care about,
>>> if they need to do anything with the device. Use that, and then you can
>>
>> hmm.. ok.
>>
>>> rip out all of the "search for a phy by a string" logic, as that's not
>>
>> Actually this is needed for non-dt boot case. In the case of dt boot, we use a
>> phandle by which the controller can get a reference to the phy. But in the case
>> of non-dt boot, the controller can get a reference to the phy only by label.
>
> I don't understand. They registered the phy, and got back a pointer to
> it. Why can't they save it in their local structure to use it again
> later if needed? They should never have to "ask" for the device, as the
One is a *PHY provider* driver which is a driver for some PHY device. This will
use phy_create to create the phy.
The other is a *PHY consumer* driver which might be any controller driver (can
be USB/SATA/PCIE). The PHY consumer will use phy_get to get a reference to the
phy (by *phandle* in the case of dt boot and *label* in the case of non-dt boot).
> device id might be unknown if there are multiple devices in the system.
I agree with you on the device id part. That need not be known to the PHY driver.
Thanks
Kishon
^ permalink raw reply
* Re: [PATCH 01/15] drivers: phy: add generic PHY framework
From: Kishon Vijay Abraham I @ 2013-07-19 5:49 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20130718154954.GA31961@kroah.com>
Hi,
On Thursday 18 July 2013 09:19 PM, Greg KH wrote:
> On Thu, Jul 18, 2013 at 02:29:52PM +0530, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Thursday 18 July 2013 12:50 PM, Greg KH wrote:
>>> On Thu, Jul 18, 2013 at 12:16:10PM +0530, Kishon Vijay Abraham I wrote:
>>>> +struct phy_provider *__of_phy_provider_register(struct device *dev,
>>>> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
>>>> + struct of_phandle_args *args));
>>>> +struct phy_provider *__devm_of_phy_provider_register(struct device *dev,
>>>> + struct module *owner, struct phy * (*of_xlate)(struct device *dev,
>>>> + struct of_phandle_args *args))
>>>> +
>>>> +__of_phy_provider_register and __devm_of_phy_provider_register can be used to
>>>> +register the phy_provider and it takes device, owner and of_xlate as
>>>> +arguments. For the dt boot case, all PHY providers should use one of the above
>>>> +2 APIs to register the PHY provider.
>>>
>>> Why do you have __ for the prefix of a public function? Is that really
>>> the way that OF handles this type of thing?
>>
>> I have a macro of_phy_provider_register/devm_of_phy_provider_register that
>> calls these functions and should be used by the PHY drivers. Probably I should
>> make a mention of it in the Documentation.
>
> Yes, mention those as you never want to be calling __* functions
> directly, right?
correct.
>
>>>> + ret = dev_set_name(&phy->dev, "%s.%d", dev_name(dev), id);
>>>
>>> Your naming is odd, no "phy" anywhere in it? You rely on the sender to
>>> never send a duplicate name.id pair? Why not create your own ids based
>>> on the number of phys in the system, like almost all other classes and
>>> subsystems do?
>>
>> hmm.. some PHY drivers use the id they provide to perform some of their
>> internal operation as in [1] (This is used only if a single PHY provider
>> implements multiple PHYS). Probably I'll add an option like PLATFORM_DEVID_AUTO
>> to give the PHY drivers an option to use auto id.
>>
>> [1] ->
>> http://archive.arm.linux.org.uk/lurker/message/20130628.134308.4a8f7668.ca.html
>
> No, who cares about the id? No one outside of the phy core ever should,
> because you pass back the only pointer that they really do care about,
> if they need to do anything with the device. Use that, and then you can
hmm.. ok.
> rip out all of the "search for a phy by a string" logic, as that's not
Actually this is needed for non-dt boot case. In the case of dt boot, we use a
phandle by which the controller can get a reference to the phy. But in the case
of non-dt boot, the controller can get a reference to the phy only by label.
> needed either. Just stick to the pointer, it's easier, and safer that
> way.
>
>>>> +static inline int phy_pm_runtime_get(struct phy *phy)
>>>> +{
>>>> + if (WARN(IS_ERR(phy), "Invalid PHY reference\n"))
>>>> + return -EINVAL;
>>>
>>> Why would phy ever not be valid and a error pointer? And why dump the
>>> stack if that happens, that seems really extreme.
>>
>> hmm.. there might be cases where the same controller in one soc needs PHY
>> control and in some other soc does not need PHY control. In such cases, we
>> might get error pointer here.
>> I'll change WARN to dev_err.
>
> I still don't understand. You have control over the code that calls
> these functions, just ensure that they pass in a valid pointer, it's
> that simple. Or am I missing something?
You are right. Valid pointer check can be done in controller code as well.
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