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

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 06 Sep 2024 09:08:59 +0200
Nuno Sá <noname.nuno@gmail.com> 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 <adureghello@baylibre.com>
> > > 
> > > 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 <adureghello@baylibre.com>
> > > Co-developed-by: David Lechner <dlechner@baylibre.com>
> > > Co-developed-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  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á
> 
> >