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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fbb79ac45fdf1103fa4e70373c90817b38ad9cfd.camel@gmail.com>
Date: Thu, 17 Oct 2024 17:13:30 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá	
 <nuno.sa@...log.com>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich	 <Michael.Hennerich@...log.com>, Jonathan Cameron
 <jic23@...nel.org>, 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,
 devicetree@...r.kernel.org, 	linux-kernel@...r.kernel.org, Mark Brown
 <broonie@...nel.org>
Subject: Re: [PATCH v6 8/8] iio: dac: adi-axi-dac: add registering of child
 fdt node

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@...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>
> > > > ---
> > > >  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á
> 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ