[v2,4/9] iio: backend adi-axi-dac: add registering of child fdt node

Commit Message

Angelo Dureghello Sept. 5, 2024, 3:17 p.m. UTC

From: Angelo Dureghello <[e-mail]>

Change to obtain the fdt use case as reported in the
adi,ad3552r.yaml file in this patchset, with the DAC device that
is actually using the backend set as a child node of the backend.

To obtain this, registering all the child fdt nodes as platform
devices.

Signed-off-by: Angelo Dureghello <[e-mail]>
Co-developed-by: David Lechner <[e-mail]>
Co-developed-by: Nuno Sá <[e-mail]>
---
 drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

Comments

David Lechner Sept. 5, 2024, 7:19 p.m. UTC | #1

On 9/5/24 10:17 AM, Angelo Dureghello wrote:
> From: Angelo Dureghello <[e-mail]>
> 
> Change to obtain the fdt use case as reported in the
> adi,ad3552r.yaml file in this patchset, with the DAC device that
> is actually using the backend set as a child node of the backend.
> 
> To obtain this, registering all the child fdt nodes as platform
> devices.
> 
> Signed-off-by: Angelo Dureghello <[e-mail]>
> Co-developed-by: David Lechner <[e-mail]>
> Co-developed-by: Nuno Sá <[e-mail]>
> ---
>  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index cc31e1dcd1df..e883cd557b6a 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
>  {
>  	struct axi_dac_state *st;
>  	const struct axi_dac_info *info;
> +	struct platform_device *child_pdev;
>  	void __iomem *base;
>  	unsigned int ver;
>  	struct clk *clk;
> @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "failed to register iio backend\n");
>  
> +	device_for_each_child_node_scoped(&pdev->dev, child) {

This should use fwnode_for_each_available_child_node() so that it skips
nodes with status != "okay".

Would be nice to introduce a scoped version of this function too.

Also, if we are allowing multiple devices on the bus, the DT bindings
need to have a reg property that is unique for each child.

> +		struct platform_device_info pi;
> +
> +		memset(&pi, 0, sizeof(pi));

struct platform_device_info pi = { };

avoids the need for memset().

> +
> +		pi.name = fwnode_get_name(child);
> +		pi.id = PLATFORM_DEVID_AUTO;
> +		pi.fwnode = child;

Need to have pi.parent = &pdev->dev;

It could also make sense to have all of the primary bus functions
(reg read/write, ddr enable/disable, etc.) passed here as platform
data instead of having everything go through the IIO backend.

> +
> +		child_pdev = platform_device_register_full(&pi);
> +		if (IS_ERR(child_pdev))
> +			return PTR_ERR(child_pdev);

These devices need to be unregistered on any error return and when
the parent device is removed.

> +	}
> +
>  	dev_info(&pdev->dev, "AXI DAC IP core (%d.%.2d.%c) probed\n",
>  		 ADI_AXI_PCORE_VER_MAJOR(ver),
>  		 ADI_AXI_PCORE_VER_MINOR(ver),
>

Nuno Sá Sept. 6, 2024, 5:42 a.m. UTC | #2

On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
> On 9/5/24 10:17 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <[e-mail]>
> > 
> > Change to obtain the fdt use case as reported in the
> > adi,ad3552r.yaml file in this patchset, with the DAC device that
> > is actually using the backend set as a child node of the backend.
> > 
> > To obtain this, registering all the child fdt nodes as platform
> > devices.
> > 
> > Signed-off-by: Angelo Dureghello <[e-mail]>
> > Co-developed-by: David Lechner <[e-mail]>
> > Co-developed-by: Nuno Sá <[e-mail]>
> > ---
> >  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > index cc31e1dcd1df..e883cd557b6a 100644
> > --- a/drivers/iio/dac/adi-axi-dac.c
> > +++ b/drivers/iio/dac/adi-axi-dac.c
> > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
> >  {
> >  	struct axi_dac_state *st;
> >  	const struct axi_dac_info *info;
> > +	struct platform_device *child_pdev;
> >  	void __iomem *base;
> >  	unsigned int ver;
> >  	struct clk *clk;
> > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev)
> >  		return dev_err_probe(&pdev->dev, ret,
> >  				     "failed to register iio backend\n");
> >  
> > +	device_for_each_child_node_scoped(&pdev->dev, child) {
> 
> This should use fwnode_for_each_available_child_node() so that it skips
> nodes with status != "okay".
> 

device_for_each_child_node() already only looks at available nodes IIRC

- Nuno Sá

Nuno Sá Sept. 6, 2024, 7:08 a.m. UTC | #3

On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
> On 9/5/24 10:17 AM, Angelo Dureghello wrote:
> > From: Angelo Dureghello <[e-mail]>
> > 
> > Change to obtain the fdt use case as reported in the
> > adi,ad3552r.yaml file in this patchset, with the DAC device that
> > is actually using the backend set as a child node of the backend.
> > 
> > To obtain this, registering all the child fdt nodes as platform
> > devices.
> > 
> > Signed-off-by: Angelo Dureghello <[e-mail]>
> > Co-developed-by: David Lechner <[e-mail]>
> > Co-developed-by: Nuno Sá <[e-mail]>
> > ---
> >  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > index cc31e1dcd1df..e883cd557b6a 100644
> > --- a/drivers/iio/dac/adi-axi-dac.c
> > +++ b/drivers/iio/dac/adi-axi-dac.c
> > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
> >  {
> >  	struct axi_dac_state *st;
> >  	const struct axi_dac_info *info;
> > +	struct platform_device *child_pdev;
> >  	void __iomem *base;
> >  	unsigned int ver;
> >  	struct clk *clk;
> > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev)
> >  		return dev_err_probe(&pdev->dev, ret,
> >  				     "failed to register iio backend\n");
> >  
> > +	device_for_each_child_node_scoped(&pdev->dev, child) {
> 
> This should use fwnode_for_each_available_child_node() so that it skips
> nodes with status != "okay".
> 
> Would be nice to introduce a scoped version of this function too.
> 
> Also, if we are allowing multiple devices on the bus, the DT bindings
> need to have a reg property that is unique for each child.
> 
> > +		struct platform_device_info pi;
> > +
> > +		memset(&pi, 0, sizeof(pi));
> 
> struct platform_device_info pi = { };
> 
> avoids the need for memset().
> 
> > +
> > +		pi.name = fwnode_get_name(child);
> > +		pi.id = PLATFORM_DEVID_AUTO;
> > +		pi.fwnode = child;
> 
> Need to have pi.parent = &pdev->dev;
> 
> It could also make sense to have all of the primary bus functions
> (reg read/write, ddr enable/disable, etc.) passed here as platform
> data instead of having everything go through the IIO backend.

Note that ddr enable/disable is something that makes sense to be in the backend
anyways as it is something that exists in LVDS/CMOS interfaces that are only running
the dataplane. Bus operations like read/write could make sense but that would mean an
interface directly between the axi-dac and the child devices (bypassing the backend
or any other middle layer - maybe we could create a tiny adi-axi-bus layer on the IIO
topdir or any other place in IIO) which I'm not so sure (and is a bit odd). OTOH,
this bus stuff goes a bit out of scope of the backend main idea/goal so yeah... Well,
let's see what others have to say about it but I don't dislike the idea.

> 
> > +
> > +		child_pdev = platform_device_register_full(&pi);
> > +		if (IS_ERR(child_pdev))
> > +			return PTR_ERR(child_pdev);
> 
> These devices need to be unregistered on any error return and when
> the parent device is removed.
> 

Definitely this needs to be tested by manually unbinding the axi-dac device for
example. I'm not really sure how this will look like and if there's any problem in
removing twice the same device (likely there is). The thing is that when we connect a
frontend with it's backend, a devlink is created (that guarantees that the frontend
is removed before the backend). So, I'm fairly confident that if we add a devm action
in here to unregister the child devices, by the time we unregister the child, it
should be already gone (unless driver core somehow handles this).

All of the above needs careful testing but one way out it (and since in here we have
the parent - child relationship), we could add a boolean flag 'skip_devlink' to
'struct iio_backend_info' so that devlinks are skipped on these arrangements. Or we
could automatically detect that the frontend is a child of the backend and skip the
link (though an explicit flag might be better).

- Nuno Sá

>

David Lechner Sept. 6, 2024, 1:52 p.m. UTC | #4

On 9/6/24 12:42 AM, Nuno Sá wrote:
> On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
>> On 9/5/24 10:17 AM, Angelo Dureghello wrote:
>>> From: Angelo Dureghello <[e-mail]>
>>>
>>> Change to obtain the fdt use case as reported in the
>>> adi,ad3552r.yaml file in this patchset, with the DAC device that
>>> is actually using the backend set as a child node of the backend.
>>>
>>> To obtain this, registering all the child fdt nodes as platform
>>> devices.
>>>
>>> Signed-off-by: Angelo Dureghello <[e-mail]>
>>> Co-developed-by: David Lechner <[e-mail]>
>>> Co-developed-by: Nuno Sá <[e-mail]>
>>> ---
>>>  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
>>> index cc31e1dcd1df..e883cd557b6a 100644
>>> --- a/drivers/iio/dac/adi-axi-dac.c
>>> +++ b/drivers/iio/dac/adi-axi-dac.c
>>> @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
>>>  {
>>>  	struct axi_dac_state *st;
>>>  	const struct axi_dac_info *info;
>>> +	struct platform_device *child_pdev;
>>>  	void __iomem *base;
>>>  	unsigned int ver;
>>>  	struct clk *clk;
>>> @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev)
>>>  		return dev_err_probe(&pdev->dev, ret,
>>>  				     "failed to register iio backend\n");
>>>  
>>> +	device_for_each_child_node_scoped(&pdev->dev, child) {
>>
>> This should use fwnode_for_each_available_child_node() so that it skips
>> nodes with status != "okay".
>>
> 
> device_for_each_child_node() already only looks at available nodes IIRC
> 
> - Nuno Sá
> 

Ah, you are right, I did not dig deep enough.

Jonathan Cameron Sept. 8, 2024, 12:36 p.m. UTC | #5

On Fri, 06 Sep 2024 09:08:59 +0200
Nuno Sá <[e-mail]> wrote:

> On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
> > On 9/5/24 10:17 AM, Angelo Dureghello wrote:  
> > > From: Angelo Dureghello <[e-mail]>
> > > 
> > > Change to obtain the fdt use case as reported in the
> > > adi,ad3552r.yaml file in this patchset, with the DAC device that
> > > is actually using the backend set as a child node of the backend.
> > > 
> > > To obtain this, registering all the child fdt nodes as platform
> > > devices.
> > > 
> > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > Co-developed-by: David Lechner <[e-mail]>
> > > Co-developed-by: Nuno Sá <[e-mail]>
> > > ---
> > >  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > > index cc31e1dcd1df..e883cd557b6a 100644
> > > --- a/drivers/iio/dac/adi-axi-dac.c
> > > +++ b/drivers/iio/dac/adi-axi-dac.c
> > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
> > >  {
> > >  	struct axi_dac_state *st;
> > >  	const struct axi_dac_info *info;
> > > +	struct platform_device *child_pdev;
> > >  	void __iomem *base;
> > >  	unsigned int ver;
> > >  	struct clk *clk;
> > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device *pdev)
> > >  		return dev_err_probe(&pdev->dev, ret,
> > >  				     "failed to register iio backend\n");
> > >  
> > > +	device_for_each_child_node_scoped(&pdev->dev, child) {  
> > 
> > This should use fwnode_for_each_available_child_node() so that it skips
> > nodes with status != "okay".

Ah. That oddity strikes again...

> > 
> > Would be nice to introduce a scoped version of this function too.
> > 
> > Also, if we are allowing multiple devices on the bus, the DT bindings
> > need to have a reg property that is unique for each child.
> >   
> > > +		struct platform_device_info pi;
> > > +
> > > +		memset(&pi, 0, sizeof(pi));  
> > 
> > struct platform_device_info pi = { };
> > 
> > avoids the need for memset().
> >   
> > > +
> > > +		pi.name = fwnode_get_name(child);
> > > +		pi.id = PLATFORM_DEVID_AUTO;
> > > +		pi.fwnode = child;  
> > 
> > Need to have pi.parent = &pdev->dev;
> > 
> > It could also make sense to have all of the primary bus functions
> > (reg read/write, ddr enable/disable, etc.) passed here as platform
> > data instead of having everything go through the IIO backend.  
> 
> Note that ddr enable/disable is something that makes sense to be in the backend
> anyways as it is something that exists in LVDS/CMOS interfaces that are only running
> the dataplane. Bus operations like read/write could make sense but that would mean an
> interface directly between the axi-dac and the child devices (bypassing the backend
> or any other middle layer - maybe we could create a tiny adi-axi-bus layer on the IIO
> topdir or any other place in IIO) which I'm not so sure (and is a bit odd). OTOH,
> this bus stuff goes a bit out of scope of the backend main idea/goal so yeah... Well,
> let's see what others have to say about it but I don't dislike the idea.

For the read/write using platform data does seem reasonable to me.
Agreed that DDR is dataplane (at least sometimes) so backend ops probably appropriate.

> 
> >   
> > > +
> > > +		child_pdev = platform_device_register_full(&pi);
> > > +		if (IS_ERR(child_pdev))
> > > +			return PTR_ERR(child_pdev);  
> > 
> > These devices need to be unregistered on any error return and when
> > the parent device is removed.
> >   
> 
> Definitely this needs to be tested by manually unbinding the axi-dac device for
> example. I'm not really sure how this will look like and if there's any problem in
> removing twice the same device (likely there is). The thing is that when we connect a
> frontend with it's backend, a devlink is created (that guarantees that the frontend
> is removed before the backend). So, I'm fairly confident that if we add a devm action
> in here to unregister the child devices, by the time we unregister the child, it
> should be already gone (unless driver core somehow handles this).
> 
> All of the above needs careful testing but one way out it (and since in here we have
> the parent - child relationship), we could add a boolean flag 'skip_devlink' to
> 'struct iio_backend_info' so that devlinks are skipped on these arrangements. Or we
> could automatically detect that the frontend is a child of the backend and skip the
> link (though an explicit flag might be better).

Agreed it needs testing but I'm not sure why it would already have gone.
The driver would have unbound, but the platform device /child would still be there
I think at time of removal. Can probably get away with devm to tear
it down when the backend device then goes away.

Maybe I'm missing some subtlety though.

Jonathan


> 
> - Nuno Sá
> 
> >

Nuno Sá Sept. 9, 2024, 7:53 a.m. UTC | #6

On Sun, 2024-09-08 at 13:36 +0100, Jonathan Cameron wrote:
> On Fri, 06 Sep 2024 09:08:59 +0200
> Nuno Sá <[e-mail]> wrote:
> 
> > On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
> > > On 9/5/24 10:17 AM, Angelo Dureghello wrote:  
> > > > From: Angelo Dureghello <[e-mail]>
> > > > 
> > > > Change to obtain the fdt use case as reported in the
> > > > adi,ad3552r.yaml file in this patchset, with the DAC device that
> > > > is actually using the backend set as a child node of the backend.
> > > > 
> > > > To obtain this, registering all the child fdt nodes as platform
> > > > devices.
> > > > 
> > > > Signed-off-by: Angelo Dureghello <[e-mail]>
> > > > Co-developed-by: David Lechner <[e-mail]>
> > > > Co-developed-by: Nuno Sá <[e-mail]>
> > > > ---
> > > >  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-
> > > > dac.c
> > > > index cc31e1dcd1df..e883cd557b6a 100644
> > > > --- a/drivers/iio/dac/adi-axi-dac.c
> > > > +++ b/drivers/iio/dac/adi-axi-dac.c
> > > > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device
> > > > *pdev)
> > > >  {
> > > >  	struct axi_dac_state *st;
> > > >  	const struct axi_dac_info *info;
> > > > +	struct platform_device *child_pdev;
> > > >  	void __iomem *base;
> > > >  	unsigned int ver;
> > > >  	struct clk *clk;
> > > > @@ -862,6 +863,20 @@ static int axi_dac_probe(struct platform_device
> > > > *pdev)
> > > >  		return dev_err_probe(&pdev->dev, ret,
> > > >  				     "failed to register iio
> > > > backend\n");
> > > >  
> > > > +	device_for_each_child_node_scoped(&pdev->dev, child) {  
> > > 
> > > This should use fwnode_for_each_available_child_node() so that it skips
> > > nodes with status != "okay".
> 
> Ah. That oddity strikes again...
> 
> > > 
> > > Would be nice to introduce a scoped version of this function too.
> > > 
> > > Also, if we are allowing multiple devices on the bus, the DT bindings
> > > need to have a reg property that is unique for each child.
> > >   
> > > > +		struct platform_device_info pi;
> > > > +
> > > > +		memset(&pi, 0, sizeof(pi));  
> > > 
> > > struct platform_device_info pi = { };
> > > 
> > > avoids the need for memset().
> > >   
> > > > +
> > > > +		pi.name = fwnode_get_name(child);
> > > > +		pi.id = PLATFORM_DEVID_AUTO;
> > > > +		pi.fwnode = child;  
> > > 
> > > Need to have pi.parent = &pdev->dev;
> > > 
> > > It could also make sense to have all of the primary bus functions
> > > (reg read/write, ddr enable/disable, etc.) passed here as platform
> > > data instead of having everything go through the IIO backend.  
> > 
> > Note that ddr enable/disable is something that makes sense to be in the
> > backend
> > anyways as it is something that exists in LVDS/CMOS interfaces that are only
> > running
> > the dataplane. Bus operations like read/write could make sense but that
> > would mean an
> > interface directly between the axi-dac and the child devices (bypassing the
> > backend
> > or any other middle layer - maybe we could create a tiny adi-axi-bus layer
> > on the IIO
> > topdir or any other place in IIO) which I'm not so sure (and is a bit odd).
> > OTOH,
> > this bus stuff goes a bit out of scope of the backend main idea/goal so
> > yeah... Well,
> > let's see what others have to say about it but I don't dislike the idea.
> 
> For the read/write using platform data does seem reasonable to me.
> Agreed that DDR is dataplane (at least sometimes) so backend ops probably
> appropriate.
> 
> > 
> > >   
> > > > +
> > > > +		child_pdev = platform_device_register_full(&pi);
> > > > +		if (IS_ERR(child_pdev))
> > > > +			return PTR_ERR(child_pdev);  
> > > 
> > > These devices need to be unregistered on any error return and when
> > > the parent device is removed.
> > >   
> > 
> > Definitely this needs to be tested by manually unbinding the axi-dac device
> > for
> > example. I'm not really sure how this will look like and if there's any
> > problem in
> > removing twice the same device (likely there is). The thing is that when we
> > connect a
> > frontend with it's backend, a devlink is created (that guarantees that the
> > frontend
> > is removed before the backend). So, I'm fairly confident that if we add a
> > devm action
> > in here to unregister the child devices, by the time we unregister the
> > child, it
> > should be already gone (unless driver core somehow handles this).
> > 
> > All of the above needs careful testing but one way out it (and since in here
> > we have
> > the parent - child relationship), we could add a boolean flag 'skip_devlink'
> > to
> > 'struct iio_backend_info' so that devlinks are skipped on these
> > arrangements. Or we
> > could automatically detect that the frontend is a child of the backend and
> > skip the
> > link (though an explicit flag might be better).
> 
> Agreed it needs testing but I'm not sure why it would already have gone.
> The driver would have unbound, but the platform device /child would still be
> there
> I think at time of removal. Can probably get away with devm to tear
> it down when the backend device then goes away.
> 
> Maybe I'm missing some subtlety though.
> 

Hmm, I guess there was some confusion of me not explaining myself. What I wanted
to say is that we will be unregistering the same device twice. But yeah, maybe
it ends up just being a NOP in the driver core. But yeah, we definitely need to
test this and make sure that the device is only gone by the time the parent
device calls platform_device_unregister() on it.

- Nuno Sá

Patch

diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
index cc31e1dcd1df..e883cd557b6a 100644
--- a/drivers/iio/dac/adi-axi-dac.c
+++ b/drivers/iio/dac/adi-axi-dac.c
@@ -783,6 +783,7 @@  static int axi_dac_probe(struct platform_device *pdev)
 {
 	struct axi_dac_state *st;
 	const struct axi_dac_info *info;
+	struct platform_device *child_pdev;
 	void __iomem *base;
 	unsigned int ver;
 	struct clk *clk;
@@ -862,6 +863,20 @@  static int axi_dac_probe(struct platform_device *pdev)
 		return dev_err_probe(&pdev->dev, ret,
 				     "failed to register iio backend\n");
 
+	device_for_each_child_node_scoped(&pdev->dev, child) {
+		struct platform_device_info pi;
+
+		memset(&pi, 0, sizeof(pi));
+
+		pi.name = fwnode_get_name(child);
+		pi.id = PLATFORM_DEVID_AUTO;
+		pi.fwnode = child;
+
+		child_pdev = platform_device_register_full(&pi);
+		if (IS_ERR(child_pdev))
+			return PTR_ERR(child_pdev);
+	}
+
 	dev_info(&pdev->dev, "AXI DAC IP core (%d.%.2d.%c) probed\n",
 		 ADI_AXI_PCORE_VER_MAJOR(ver),
 		 ADI_AXI_PCORE_VER_MINOR(ver),