diff mbox series

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

Message ID 20241014-wip-bl-ad3552r-axi-v0-iio-testing-v6-8-eeef0c1e0e56@baylibre.com (mailing list archive)
State New
Headers show
Series iio: add support for the ad3552r AXI DAC IP | expand

Commit Message

Angelo Dureghello Oct. 14, 2024, 10:08 a.m. UTC 
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(+)

Comments

David Lechner Oct. 14, 2024, 9:16 p.m. UTC | #1 
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);
> +}
> +
> +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.

(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;
> +
> +		/* Processing only reg 0 node */
> +		ret = fwnode_property_read_u32(child, "reg", &val);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret,
> +						"child node missing.");

Shouldn't the error message say that there is a problem with the reg
property? We already have a handle to the child node, so the child node
isn't missing.

> +		if (val != 0)
> +			return dev_err_probe(&pdev->dev, -EINVAL,
> +						"invalid node address.");
> +
> +		ret = axi_dac_create_platform_device(st, child);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, -EINVAL,
> +						"could not create device.");
> +	}
> +
>  	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),
>
Nuno Sá Oct. 15, 2024, 6:11 a.m. UTC | #2 
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. 

Anyways, I'll leave this up to you and maybe others can also argue about this...

> > +		/* Processing only reg 0 node */
> > +		ret = fwnode_property_read_u32(child, "reg", &val);
> > +		if (ret)
> > +			return dev_err_probe(&pdev->dev, ret,
> > +						"child node missing.");
> 
> Shouldn't the error message say that there is a problem with the reg
> property? We already have a handle to the child node, so the child node
> isn't missing.

Makes sense... like "reg property missing" - something on those lines.

- Nuno Sá
Angelo Dureghello Oct. 17, 2024, 8:32 a.m. UTC | #3 
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.

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

> Anyways, I'll leave this up to you and maybe others can also argue about this...
> 
> > > +		/* Processing only reg 0 node */
> > > +		ret = fwnode_property_read_u32(child, "reg", &val);
> > > +		if (ret)
> > > +			return dev_err_probe(&pdev->dev, ret,
> > > +						"child node missing.");
> > 
> > Shouldn't the error message say that there is a problem with the reg
> > property? We already have a handle to the child node, so the child node
> > isn't missing.
> 
> Makes sense... like "reg property missing" - something on those lines.
> 
> - Nuno Sá
> 
> 
>
Nuno Sá Oct. 17, 2024, 3:13 p.m. UTC | #4 
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á
>
diff mbox series

Patch

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);
+}
+
+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);
+
+	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;
+
+		/* Processing only reg 0 node */
+		ret = fwnode_property_read_u32(child, "reg", &val);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+						"child node missing.");
+		if (val != 0)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+						"invalid node address.");
+
+		ret = axi_dac_create_platform_device(st, child);
+		if (ret)
+			return dev_err_probe(&pdev->dev, -EINVAL,
+						"could not create device.");
+	}
+
 	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),