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

[Nuno Sá]

On Sun, 2024-09-08 at 13:36 +0100, Jonathan Cameron wrote:
> On Fri, 06 Sep 2024 09:08:59 +0200
> Nuno Sá <noname.nuno@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > 
> > > > 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@xxxxxxxxxxxx>
> > > > Co-developed-by: David Lechner <dlechner@xxxxxxxxxxxx>
> > > > Co-developed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > > > ---
> > > >  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á