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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240929132109.6162d69d@jic23-huawei>
Date: Sun, 29 Sep 2024 13:21:09 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, dlechner@...libre.com
Subject: Re: [PATCH v3 10/10] iio: backend: adi-axi-dac: add registering of
 child fdt node

On Thu, 19 Sep 2024 11:20:06 +0200
Angelo Dureghello <adureghello@...libre.com> wrote:

> From: Angelo Dureghello <adureghello@...libre.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@...libre.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);
>  
>