Re: [PATCH v3 10/10] iio: backend: adi-axi-dac: add registering of child fdt node

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Sep 2024 11:20:06 +0200
Angelo Dureghello <adureghello@baylibre.com> 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>
A few minor comments inline. 
> ---
>  drivers/iio/dac/adi-axi-dac.c | 52 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> index 3ca3a14c575b..2afc1442cd5a 100644
> --- a/drivers/iio/dac/adi-axi-dac.c
> +++ b/drivers/iio/dac/adi-axi-dac.c
> @@ -18,6 +18,7 @@
>  #include <linux/module.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/mutex.h>
> +#include <linux/platform_data/ad3552r-axi.h>
>  #include <linux/platform_device.h>
>  #include <linux/property.h>
>  #include <linux/regmap.h>
> @@ -109,6 +110,8 @@ struct axi_dac_info {
>  struct axi_dac_state {
>  	struct regmap *regmap;
>  	struct device *dev;
> +	/* Target DAC platform device */
> +	struct platform_device *dac_pdev;
>  	/*
>  	 * lock to protect multiple accesses to the device registers and global
>  	 * data/variables.
> @@ -757,6 +760,32 @@ static int axi_dac_bus_reg_read(struct iio_backend *back, u32 reg,
>  	}
>  }
>  
> +static int axi_dac_create_platform_device(struct axi_dac_state *st,
> +					  struct fwnode_handle *child)
> +{
> +	struct ad3552r_axi_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);
> +

Register a devm cleanup here via devm_add_action_or_reset()
(see below for why)

> +	st->dac_pdev = pdev;
> +
> +	return 0;
> +}
> +
>  static const struct iio_backend_ops axi_dac_generic_ops = {
>  	.enable = axi_dac_enable,
>  	.disable = axi_dac_disable,
> @@ -791,13 +820,22 @@ static const struct regmap_config axi_dac_regmap_config = {
>  	.max_register = 0x0800,
>  };
>  
> +static void axi_dac_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct axi_dac_state *st = dev_get_drvdata(dev);
> +
> +	if (st->dac_pdev)
> +		platform_device_unregister(st->dac_pdev);

Use a devm_add_action_or_reset() So we don't need to introduce
a remove just to handle an optional bit of cleanup.
Also makes it much less likely we'll introduce ordering bugs
as the driver gets more complex in future.


> +}
> +
>  static int axi_dac_probe(struct platform_device *pdev)
>  {
>  	struct axi_dac_state *st;
>  	void __iomem *base;
>  	unsigned int ver;
>  	struct clk *clk;
> -	int ret;
> +	int ret, val;
>  
>  	st = devm_kzalloc(&pdev->dev, sizeof(*st), GFP_KERNEL);
>  	if (!st)
> @@ -871,6 +909,17 @@ 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) {
> +		/* Processing only reg 0 node */

Pull int val; in here as not used outside of this scope.
Also, should only do this at all for compatibles that we know this
makes sense for.  Use some part specific flag to make that decision.


> +		ret = fwnode_property_read_u32(child, "reg", &val);
> +		if (ret || val != 0)
> +			continue;
> +
> +		ret = axi_dac_create_platform_device(st, child);
> +		if (ret)
> +			continue;
> +	}
> +
>  	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),
> @@ -901,6 +950,7 @@ static struct platform_driver axi_dac_driver = {
>  		.of_match_table = axi_dac_of_match,
>  	},
>  	.probe = axi_dac_probe,
> +	.remove = axi_dac_remove,
>  };
>  module_platform_driver(axi_dac_driver);
>  
>