Re: [PATCH v6 8/8] iio: dac: adi-axi-dac: add registering of child fdt node

["Nuno Sá" <noname.nuno@gmail.com>]

On Thu, 2024-10-17 at 10:32 +0200, Angelo Dureghello wrote:
> On 15.10.2024 08:11, Nuno Sá wrote:
> > On Mon, 2024-10-14 at 16:16 -0500, David Lechner wrote:
> > > On 10/14/24 5:08 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.
> > > > 
> > > > The DAC device is defined as a child node of the backend.
> > > > Registering the child fdt node as a platform devices.
> > > > 
> > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > > ---
> > > >  drivers/iio/dac/adi-axi-dac.c | 53
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 53 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-
> > > > dac.c
> > > > index b887c6343f96..f85e3138d428 100644
> > > > --- a/drivers/iio/dac/adi-axi-dac.c
> > > > +++ b/drivers/iio/dac/adi-axi-dac.c
> > > > @@ -29,6 +29,8 @@
> > > >  #include <linux/iio/buffer.h>
> > > >  #include <linux/iio/iio.h>
> > > >  
> > > > +#include "ad3552r-hs.h"
> > > > +
> > > >  /*
> > > >   * Register definitions:
> > > >   *  
> > > > https://wiki.analog.com/resources/fpga/docs/axi_dac_ip#register_map
> > > > @@ -738,6 +740,39 @@ static int axi_dac_bus_reg_read(struct iio_backend
> > > > *back,
> > > > u32 reg, u32 *val,
> > > >  	return regmap_read(st->regmap, AXI_DAC_CUSTOM_RD_REG, val);
> > > >  }
> > > >  
> > > > +static void axi_dac_child_remove(void *data)
> > > > +{
> > > > +	struct platform_device *pdev = data;
> > > > +
> > > > +	platform_device_unregister(pdev);
> > 
> > Just do platform_device_unregister(data)... Or call the argument pdev for
> > better
> > readability...
> > 
> > > > +}
> > > > +
> > > > +static int axi_dac_create_platform_device(struct axi_dac_state *st,
> > > > +					  struct fwnode_handle *child)
> > > > +{
> > > > +	struct ad3552r_hs_platform_data pdata = {
> > > > +		.bus_reg_read = axi_dac_bus_reg_read,
> > > > +		.bus_reg_write = axi_dac_bus_reg_write,
> > > > +	};
> > > > +	struct platform_device_info pi = {
> > > > +		.parent = st->dev,
> > > > +		.name = fwnode_get_name(child),
> > > > +		.id = PLATFORM_DEVID_AUTO,
> > > > +		.fwnode = child,
> > > > +		.data = &pdata,
> > > > +		.size_data = sizeof(pdata),
> > > > +	};
> > > > +	struct platform_device *pdev;
> > > > +
> > > > +	pdev = platform_device_register_full(&pi);
> > > > +	if (IS_ERR(pdev))
> > > > +		return PTR_ERR(pdev);
> > > > +
> > > > +	device_set_node(&pdev->dev, child);
> > > 
> > > Not sure why Nuno suggested adding device_set_node(). It is
> > > redundant since platform_device_register_full() already does
> > > the same thing.
> > > 
> > 
> > Indeed... I realized that yesterday when (actually) looking at
> > platform_device_register_full(). You just beat me in replying to the email.
> > Sorry for
> > the noise...
> > 
> > > (And setting it after platform_device_register_full() would
> > > be too late anyway since drivers may have already probed.)
> > 
> > > > +
> > > > +	return devm_add_action_or_reset(st->dev, axi_dac_child_remove,
> > > > pdev);
> > > > +}
> > > > +
> > > >  static const struct iio_backend_ops axi_dac_generic_ops = {
> > > >  	.enable = axi_dac_enable,
> > > >  	.disable = axi_dac_disable,
> > > > @@ -874,6 +909,24 @@ 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) {
> > > > +		int val;
> > > > +
> > 
> > I'm starting to come around again if some sort of flag (bus_controller or an
> > explicit
> > has_child) wouldn't make sense (since you may need to re-spin another
> > version). So we
> > could error out in case someone comes up with child nodes on a device that
> > does not
> > support them. 
> > 
> 
> For this, i added a check on io-backend here, that has been asked
> to be removed.

Not sure if it's totally correct but better than the option you suggest below.
So if you prefer that (opposed to an explicit flag), maybe then bring back the
check for the io-backend presence with a comment to make the intent explicit.
Like, "All childs need to point to an io-backend" or something like that. And if
we ever have a usecase where we can have childs without that property, we can
add an explicit flag for this.

> Without adding other flags, i may use has_dac_clk, could it be ok ?
> 

I do not think it's related. Even more since that flag is generic enough that
could be used for another versions of the IP which also have additional clocks
on top of the axi one.

- Nuno Sá
>