[PATCH] platform: set of_node in platform_device_register_full()

[PATCH] platform: set of_node in platform_device_register_full()

[Mans Rullgard]

If the provided fwnode is an OF node, set dev.of_node as well.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/base/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..853a1d0e5845 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
 
 	pdev->dev.parent = pdevinfo->parent;
 	pdev->dev.fwnode = pdevinfo->fwnode;
+	pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
 
 	if (pdevinfo->dma_mask) {
 		/*
-- 
2.20.1

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
>
> If the provided fwnode is an OF node, set dev.of_node as well.
>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/base/platform.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..853a1d0e5845 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>
>         pdev->dev.parent = pdevinfo->parent;
>         pdev->dev.fwnode = pdevinfo->fwnode;
> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));

of_node_get() generally does a kobject_get() on the node's kobject, so
when is that reference dropped?  Or if it doesn't need to be dropped
at all, why is this the case?

>
>         if (pdevinfo->dma_mask) {
>                 /*
> --
> 2.20.1
>

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
>>
>> If the provided fwnode is an OF node, set dev.of_node as well.
>>
>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>  drivers/base/platform.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index dff82a3c2caa..853a1d0e5845 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>>
>>         pdev->dev.parent = pdevinfo->parent;
>>         pdev->dev.fwnode = pdevinfo->fwnode;
>> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>
> of_node_get() generally does a kobject_get() on the node's kobject, so
> when is that reference dropped?  Or if it doesn't need to be dropped
> at all, why is this the case?

platform_device_release() calls of_device_node_put().

-- 
Måns Rullgård

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård <mans@mansr.com> wrote:
>
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
> >>
> >> If the provided fwnode is an OF node, set dev.of_node as well.
> >>
> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> ---
> >>  drivers/base/platform.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> index dff82a3c2caa..853a1d0e5845 100644
> >> --- a/drivers/base/platform.c
> >> +++ b/drivers/base/platform.c
> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
> >>
> >>         pdev->dev.parent = pdevinfo->parent;
> >>         pdev->dev.fwnode = pdevinfo->fwnode;
> >> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> >
> > of_node_get() generally does a kobject_get() on the node's kobject, so
> > when is that reference dropped?  Or if it doesn't need to be dropped
> > at all, why is this the case?
>
> platform_device_release() calls of_device_node_put().

Yes, it does, but this is the reference that's already acquired for
devices added while parsing DT, isn't it?

Your change adds an extra reference AFAICS.

Also, why is this patch needed?

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård <mans@mansr.com> wrote:
>>
>> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>>
>> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
>> >>
>> >> If the provided fwnode is an OF node, set dev.of_node as well.
>> >>
>> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> >> ---
>> >>  drivers/base/platform.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> >> index dff82a3c2caa..853a1d0e5845 100644
>> >> --- a/drivers/base/platform.c
>> >> +++ b/drivers/base/platform.c
>> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>> >>
>> >>         pdev->dev.parent = pdevinfo->parent;
>> >>         pdev->dev.fwnode = pdevinfo->fwnode;
>> >> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>> >
>> > of_node_get() generally does a kobject_get() on the node's kobject, so
>> > when is that reference dropped?  Or if it doesn't need to be dropped
>> > at all, why is this the case?
>>
>> platform_device_release() calls of_device_node_put().
>
> Yes, it does, but this is the reference that's already acquired for
> devices added while parsing DT, isn't it?
>
> Your change adds an extra reference AFAICS.
>
> Also, why is this patch needed?

Some drivers are just shims that create extra "glue" devices with the DT
device as parent and have the real driver bind to these.  In other
cases, the same real driver matches the DT node directly.  When a glue
device is used, it needs to get a reference to the original DT node in
order for the main driver to access properties and child nodes.

Right now, my problem is that the suxi-musb driver creates such a glue
device for the musb core driver to bind to without setting of_node.
This means devices attached to this USB interface don't get associated
with DT nodes, if present, the way they do with EHCI.

The sunxi-musb driver uses platform_device_register_full(), so this
seemed like the easiest way to let it set of_node of the new device.
Since this creates a second reference to the same node, of_node_get()
is required.

If you'd prefer solving this in some other way, please tell me how.

-- 
Måns Rullgård

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Wed, Feb 20, 2019 at 11:41 AM Måns Rullgård <mans@mansr.com> wrote:
>
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
> > On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård <mans@mansr.com> wrote:
> >>
> >> "Rafael J. Wysocki" <rafael@kernel.org> writes:
> >>
> >> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
> >> >>
> >> >> If the provided fwnode is an OF node, set dev.of_node as well.
> >> >>
> >> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> >> ---
> >> >>  drivers/base/platform.c | 1 +
> >> >>  1 file changed, 1 insertion(+)
> >> >>
> >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> >> index dff82a3c2caa..853a1d0e5845 100644
> >> >> --- a/drivers/base/platform.c
> >> >> +++ b/drivers/base/platform.c
> >> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
> >> >>
> >> >>         pdev->dev.parent = pdevinfo->parent;
> >> >>         pdev->dev.fwnode = pdevinfo->fwnode;
> >> >> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> >> >
> >> > of_node_get() generally does a kobject_get() on the node's kobject, so
> >> > when is that reference dropped?  Or if it doesn't need to be dropped
> >> > at all, why is this the case?
> >>
> >> platform_device_release() calls of_device_node_put().
> >
> > Yes, it does, but this is the reference that's already acquired for
> > devices added while parsing DT, isn't it?
> >
> > Your change adds an extra reference AFAICS.
> >
> > Also, why is this patch needed?
>
> Some drivers are just shims that create extra "glue" devices with the DT
> device as parent and have the real driver bind to these.  In other
> cases, the same real driver matches the DT node directly.  When a glue
> device is used, it needs to get a reference to the original DT node in
> order for the main driver to access properties and child nodes.
>
> Right now, my problem is that the suxi-musb driver creates such a glue
> device for the musb core driver to bind to without setting of_node.
> This means devices attached to this USB interface don't get associated
> with DT nodes, if present, the way they do with EHCI.

You really should describe problems that you want to address in patch
changelogs.  This helps a lot to understand the motivation for the
changes.

> The sunxi-musb driver uses platform_device_register_full(), so this
> seemed like the easiest way to let it set of_node of the new device.
> Since this creates a second reference to the same node, of_node_get()
> is required.

But what about devices that already have of_node set at this point?

Maybe check if of_node is NULL before trying to set it?

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, Feb 20, 2019 at 11:41 AM Måns Rullgård <mans@mansr.com> wrote:
>>
>> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>>
>> > On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård <mans@mansr.com> wrote:
>> >>
>> >> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>> >>
>> >> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
>> >> >>
>> >> >> If the provided fwnode is an OF node, set dev.of_node as well.
>> >> >>
>> >> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> >> >> ---
>> >> >>  drivers/base/platform.c | 1 +
>> >> >>  1 file changed, 1 insertion(+)
>> >> >>
>> >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> >> >> index dff82a3c2caa..853a1d0e5845 100644
>> >> >> --- a/drivers/base/platform.c
>> >> >> +++ b/drivers/base/platform.c
>> >> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>> >> >>
>> >> >>         pdev->dev.parent = pdevinfo->parent;
>> >> >>         pdev->dev.fwnode = pdevinfo->fwnode;
>> >> >> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>> >> >
>> >> > of_node_get() generally does a kobject_get() on the node's kobject, so
>> >> > when is that reference dropped?  Or if it doesn't need to be dropped
>> >> > at all, why is this the case?
>> >>
>> >> platform_device_release() calls of_device_node_put().
>> >
>> > Yes, it does, but this is the reference that's already acquired for
>> > devices added while parsing DT, isn't it?
>> >
>> > Your change adds an extra reference AFAICS.
>> >
>> > Also, why is this patch needed?
>>
>> Some drivers are just shims that create extra "glue" devices with the DT
>> device as parent and have the real driver bind to these.  In other
>> cases, the same real driver matches the DT node directly.  When a glue
>> device is used, it needs to get a reference to the original DT node in
>> order for the main driver to access properties and child nodes.
>>
>> Right now, my problem is that the suxi-musb driver creates such a glue
>> device for the musb core driver to bind to without setting of_node.
>> This means devices attached to this USB interface don't get associated
>> with DT nodes, if present, the way they do with EHCI.
>
> You really should describe problems that you want to address in patch
> changelogs.  This helps a lot to understand the motivation for the
> changes.

Do you want me to send a new patch with the above explanation in the
commit message?

>> The sunxi-musb driver uses platform_device_register_full(), so this
>> seemed like the easiest way to let it set of_node of the new device.
>> Since this creates a second reference to the same node, of_node_get()
>> is required.
>
> But what about devices that already have of_node set at this point?
>
> Maybe check if of_node is NULL before trying to set it?

It's a brand new device allocated a few lines above.  of_node has to be
null here.

-- 
Måns Rullgård

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Wed, Feb 20, 2019 at 12:02 PM Måns Rullgård <mans@mansr.com> wrote:
>
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
> > On Wed, Feb 20, 2019 at 11:41 AM Måns Rullgård <mans@mansr.com> wrote:
> >>
> >> "Rafael J. Wysocki" <rafael@kernel.org> writes:
> >>
> >> > On Mon, Feb 18, 2019 at 12:10 PM Måns Rullgård <mans@mansr.com> wrote:
> >> >>
> >> >> "Rafael J. Wysocki" <rafael@kernel.org> writes:
> >> >>
> >> >> > On Sat, Feb 16, 2019 at 5:50 PM Mans Rullgard <mans@mansr.com> wrote:
> >> >> >>
> >> >> >> If the provided fwnode is an OF node, set dev.of_node as well.
> >> >> >>
> >> >> >> Signed-off-by: Mans Rullgard <mans@mansr.com>
> >> >> >> ---
> >> >> >>  drivers/base/platform.c | 1 +
> >> >> >>  1 file changed, 1 insertion(+)
> >> >> >>
> >> >> >> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> >> >> >> index dff82a3c2caa..853a1d0e5845 100644
> >> >> >> --- a/drivers/base/platform.c
> >> >> >> +++ b/drivers/base/platform.c
> >> >> >> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
> >> >> >>
> >> >> >>         pdev->dev.parent = pdevinfo->parent;
> >> >> >>         pdev->dev.fwnode = pdevinfo->fwnode;
> >> >> >> +       pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
> >> >> >
> >> >> > of_node_get() generally does a kobject_get() on the node's kobject, so
> >> >> > when is that reference dropped?  Or if it doesn't need to be dropped
> >> >> > at all, why is this the case?
> >> >>
> >> >> platform_device_release() calls of_device_node_put().
> >> >
> >> > Yes, it does, but this is the reference that's already acquired for
> >> > devices added while parsing DT, isn't it?
> >> >
> >> > Your change adds an extra reference AFAICS.
> >> >
> >> > Also, why is this patch needed?
> >>
> >> Some drivers are just shims that create extra "glue" devices with the DT
> >> device as parent and have the real driver bind to these.  In other
> >> cases, the same real driver matches the DT node directly.  When a glue
> >> device is used, it needs to get a reference to the original DT node in
> >> order for the main driver to access properties and child nodes.
> >>
> >> Right now, my problem is that the suxi-musb driver creates such a glue
> >> device for the musb core driver to bind to without setting of_node.
> >> This means devices attached to this USB interface don't get associated
> >> with DT nodes, if present, the way they do with EHCI.
> >
> > You really should describe problems that you want to address in patch
> > changelogs.  This helps a lot to understand the motivation for the
> > changes.
>
> Do you want me to send a new patch with the above explanation in the
> commit message?

Yes, please.

> >> The sunxi-musb driver uses platform_device_register_full(), so this
> >> seemed like the easiest way to let it set of_node of the new device.
> >> Since this creates a second reference to the same node, of_node_get()
> >> is required.
> >
> > But what about devices that already have of_node set at this point?
> >
> > Maybe check if of_node is NULL before trying to set it?
>
> It's a brand new device allocated a few lines above.  of_node has to be
> null here.

Right, sorry for the confusion.

[PATCH] platform: set of_node in platform_device_register_full()

[Mans Rullgard]

If the provided fwnode is an OF node, set dev.of_node as well.

Some drivers are just shims that create extra "glue" devices with the
DT device as parent and have the real driver bind to these.  In these
cases, the glue device needs to get a reference to the original DT node
in order for the main driver to access properties and child nodes.

For example, the sunxi-musb driver creates such a glue device using
platform_device_register_full().  Consequently, devices attached to
this USB interface don't get associated with DT nodes, if present,
the way they do with EHCI.

This change will allow sunxi-musb and similar driver to easily
propagate the DT node to child devices as required.

Signed-off-by: Mans Rullgard <mans@mansr.com>
---
 drivers/base/platform.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index dff82a3c2caa..853a1d0e5845 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
 
 	pdev->dev.parent = pdevinfo->parent;
 	pdev->dev.fwnode = pdevinfo->fwnode;
+	pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
 
 	if (pdevinfo->dma_mask) {
 		/*
-- 
2.20.1

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Johan Hovold]

On Wed, Feb 20, 2019 at 11:35:06AM +0000, Mans Rullgard wrote:
> If the provided fwnode is an OF node, set dev.of_node as well.
> 
> Some drivers are just shims that create extra "glue" devices with the
> DT device as parent and have the real driver bind to these.  In these
> cases, the glue device needs to get a reference to the original DT node
> in order for the main driver to access properties and child nodes.
> 
> For example, the sunxi-musb driver creates such a glue device using
> platform_device_register_full().  Consequently, devices attached to
> this USB interface don't get associated with DT nodes, if present,
> the way they do with EHCI.
> 
> This change will allow sunxi-musb and similar driver to easily
> propagate the DT node to child devices as required.

Just a drive-by comment, didn't look to closely at this patch, but this
all sounds familiar.

Note that if both platform devices are bound to drivers you may end up
with some resources like pinctrl which are handled automatically by
driver core at probe time to be requested twice (and failing the second
time).

Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
device-tree node"), which provides a means to avoid this, and
49484abd93ab ("USB: musb: dsps: propagate device-tree node").

> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/base/platform.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..853a1d0e5845 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>  
>  	pdev->dev.parent = pdevinfo->parent;
>  	pdev->dev.fwnode = pdevinfo->fwnode;
> +	pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>  
>  	if (pdevinfo->dma_mask) {
>  		/*

Johan

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

Johan Hovold <johan@kernel.org> writes:

> On Wed, Feb 20, 2019 at 11:35:06AM +0000, Mans Rullgard wrote:
>> If the provided fwnode is an OF node, set dev.of_node as well.
>> 
>> Some drivers are just shims that create extra "glue" devices with the
>> DT device as parent and have the real driver bind to these.  In these
>> cases, the glue device needs to get a reference to the original DT node
>> in order for the main driver to access properties and child nodes.
>> 
>> For example, the sunxi-musb driver creates such a glue device using
>> platform_device_register_full().  Consequently, devices attached to
>> this USB interface don't get associated with DT nodes, if present,
>> the way they do with EHCI.
>> 
>> This change will allow sunxi-musb and similar driver to easily
>> propagate the DT node to child devices as required.
>
> Just a drive-by comment, didn't look to closely at this patch, but this
> all sounds familiar.
>
> Note that if both platform devices are bound to drivers you may end up
> with some resources like pinctrl which are handled automatically by
> driver core at probe time to be requested twice (and failing the second
> time).
>
> Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
> device-tree node"), which provides a means to avoid this, and
> 49484abd93ab ("USB: musb: dsps: propagate device-tree node").

Thanks, and ugh.  So we should be setting the of_node_reused flag when
this is the case.  It's easy for the musb-dsps driver since it doesn't
use platform_device_register_full() and can do this before the
device_add() call.  How can we convey that this flag needs to be set?

>> Signed-off-by: Mans Rullgard <mans@mansr.com>
>> ---
>>  drivers/base/platform.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
>> index dff82a3c2caa..853a1d0e5845 100644
>> --- a/drivers/base/platform.c
>> +++ b/drivers/base/platform.c
>> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>>  
>>  	pdev->dev.parent = pdevinfo->parent;
>>  	pdev->dev.fwnode = pdevinfo->fwnode;
>> +	pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>>  
>>  	if (pdevinfo->dma_mask) {
>>  		/*
>
> Johan

-- 
Måns Rullgård

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Wed, Feb 20, 2019 at 1:12 PM Måns Rullgård <mans@mansr.com> wrote:
>
> Johan Hovold <johan@kernel.org> writes:
>
> > On Wed, Feb 20, 2019 at 11:35:06AM +0000, Mans Rullgard wrote:
> >> If the provided fwnode is an OF node, set dev.of_node as well.
> >>
> >> Some drivers are just shims that create extra "glue" devices with the
> >> DT device as parent and have the real driver bind to these.  In these
> >> cases, the glue device needs to get a reference to the original DT node
> >> in order for the main driver to access properties and child nodes.
> >>
> >> For example, the sunxi-musb driver creates such a glue device using
> >> platform_device_register_full().  Consequently, devices attached to
> >> this USB interface don't get associated with DT nodes, if present,
> >> the way they do with EHCI.
> >>
> >> This change will allow sunxi-musb and similar driver to easily
> >> propagate the DT node to child devices as required.
> >
> > Just a drive-by comment, didn't look to closely at this patch, but this
> > all sounds familiar.
> >
> > Note that if both platform devices are bound to drivers you may end up
> > with some resources like pinctrl which are handled automatically by
> > driver core at probe time to be requested twice (and failing the second
> > time).
> >
> > Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
> > device-tree node"), which provides a means to avoid this, and
> > 49484abd93ab ("USB: musb: dsps: propagate device-tree node").
>
> Thanks, and ugh.  So we should be setting the of_node_reused flag when
> this is the case.  It's easy for the musb-dsps driver since it doesn't
> use platform_device_register_full() and can do this before the
> device_add() call.  How can we convey that this flag needs to be set?

Through pdevinfo I guess?

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

"Rafael J. Wysocki" <rafael@kernel.org> writes:

> On Wed, Feb 20, 2019 at 1:12 PM Måns Rullgård <mans@mansr.com> wrote:
>>
>> Johan Hovold <johan@kernel.org> writes:
>>
>> > On Wed, Feb 20, 2019 at 11:35:06AM +0000, Mans Rullgard wrote:
>> >> If the provided fwnode is an OF node, set dev.of_node as well.
>> >>
>> >> Some drivers are just shims that create extra "glue" devices with the
>> >> DT device as parent and have the real driver bind to these.  In these
>> >> cases, the glue device needs to get a reference to the original DT node
>> >> in order for the main driver to access properties and child nodes.
>> >>
>> >> For example, the sunxi-musb driver creates such a glue device using
>> >> platform_device_register_full().  Consequently, devices attached to
>> >> this USB interface don't get associated with DT nodes, if present,
>> >> the way they do with EHCI.
>> >>
>> >> This change will allow sunxi-musb and similar driver to easily
>> >> propagate the DT node to child devices as required.
>> >
>> > Just a drive-by comment, didn't look to closely at this patch, but this
>> > all sounds familiar.
>> >
>> > Note that if both platform devices are bound to drivers you may end up
>> > with some resources like pinctrl which are handled automatically by
>> > driver core at probe time to be requested twice (and failing the second
>> > time).
>> >
>> > Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
>> > device-tree node"), which provides a means to avoid this, and
>> > 49484abd93ab ("USB: musb: dsps: propagate device-tree node").
>>
>> Thanks, and ugh.  So we should be setting the of_node_reused flag when
>> this is the case.  It's easy for the musb-dsps driver since it doesn't
>> use platform_device_register_full() and can do this before the
>> device_add() call.  How can we convey that this flag needs to be set?
>
> Through pdevinfo I guess?

Not without adding another field to it.  The most direct is of course to
simply add an of_node_reused flag there too and copy it over.  Would
that be OK, or is there a better way?

-- 
Måns Rullgård

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Rafael J. Wysocki]

On Wed, Feb 20, 2019 at 1:26 PM Måns Rullgård <mans@mansr.com> wrote:
>
> "Rafael J. Wysocki" <rafael@kernel.org> writes:
>
> > On Wed, Feb 20, 2019 at 1:12 PM Måns Rullgård <mans@mansr.com> wrote:
> >>
> >> Johan Hovold <johan@kernel.org> writes:
> >>
> >> > On Wed, Feb 20, 2019 at 11:35:06AM +0000, Mans Rullgard wrote:
> >> >> If the provided fwnode is an OF node, set dev.of_node as well.
> >> >>
> >> >> Some drivers are just shims that create extra "glue" devices with the
> >> >> DT device as parent and have the real driver bind to these.  In these
> >> >> cases, the glue device needs to get a reference to the original DT node
> >> >> in order for the main driver to access properties and child nodes.
> >> >>
> >> >> For example, the sunxi-musb driver creates such a glue device using
> >> >> platform_device_register_full().  Consequently, devices attached to
> >> >> this USB interface don't get associated with DT nodes, if present,
> >> >> the way they do with EHCI.
> >> >>
> >> >> This change will allow sunxi-musb and similar driver to easily
> >> >> propagate the DT node to child devices as required.
> >> >
> >> > Just a drive-by comment, didn't look to closely at this patch, but this
> >> > all sounds familiar.
> >> >
> >> > Note that if both platform devices are bound to drivers you may end up
> >> > with some resources like pinctrl which are handled automatically by
> >> > driver core at probe time to be requested twice (and failing the second
> >> > time).
> >> >
> >> > Take a look at 4e75e1d7dac9 ("driver core: add helper to reuse a
> >> > device-tree node"), which provides a means to avoid this, and
> >> > 49484abd93ab ("USB: musb: dsps: propagate device-tree node").
> >>
> >> Thanks, and ugh.  So we should be setting the of_node_reused flag when
> >> this is the case.  It's easy for the musb-dsps driver since it doesn't
> >> use platform_device_register_full() and can do this before the
> >> device_add() call.  How can we convey that this flag needs to be set?
> >
> > Through pdevinfo I guess?
>
> Not without adding another field to it.  The most direct is of course to
> simply add an of_node_reused flag there too and copy it over.  Would
> that be OK, or is there a better way?

That's what I meant. :-)

Re: [PATCH] platform: set of_node in platform_device_register_full()

[Måns Rullgård]

Mans Rullgard <mans@mansr.com> writes:

> If the provided fwnode is an OF node, set dev.of_node as well.
>
> Some drivers are just shims that create extra "glue" devices with the
> DT device as parent and have the real driver bind to these.  In these
> cases, the glue device needs to get a reference to the original DT node
> in order for the main driver to access properties and child nodes.
>
> For example, the sunxi-musb driver creates such a glue device using
> platform_device_register_full().  Consequently, devices attached to
> this USB interface don't get associated with DT nodes, if present,
> the way they do with EHCI.
>
> This change will allow sunxi-musb and similar driver to easily
> propagate the DT node to child devices as required.
>
> Signed-off-by: Mans Rullgard <mans@mansr.com>
> ---
>  drivers/base/platform.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index dff82a3c2caa..853a1d0e5845 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -512,6 +512,7 @@ struct platform_device *platform_device_register_full(
>
>  	pdev->dev.parent = pdevinfo->parent;
>  	pdev->dev.fwnode = pdevinfo->fwnode;
> +	pdev->dev.of_node = of_node_get(to_of_node(pdev->dev.fwnode));
>
>  	if (pdevinfo->dma_mask) {
>  		/*
> -- 

Sorry, I forgot to add a v2 to this.  Anyway, the only change is the
commit message.

-- 
Måns Rullgård