linux-kernel - Re: [PATCH v2 4/9] 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <38cc5356fc737460f6962d6aae274e72f5b5c73d.camel@gmail.com>
Date: Fri, 06 Sep 2024 09:08:59 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: David Lechner <dlechner@...libre.com>, Angelo Dureghello
 <adureghello@...libre.com>, Lars-Peter Clausen <lars@...afoo.de>, Michael
 Hennerich <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...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>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/9] iio: backend adi-axi-dac: add registering of
 child fdt node

On Thu, 2024-09-05 at 14:19 -0500, David Lechner wrote:
> On 9/5/24 10:17 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, with the DAC device that
> > is actually using the backend set as a child node of the backend.
> > 
> > To obtain this, registering all the child fdt nodes as platform
> > devices.
> > 
> > Signed-off-by: Angelo Dureghello <adureghello@...libre.com>
> > Co-developed-by: David Lechner <dlechner@...libre.com>
> > Co-developed-by: Nuno Sá <nuno.sa@...log.com>
> > ---
> >  drivers/iio/dac/adi-axi-dac.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/iio/dac/adi-axi-dac.c b/drivers/iio/dac/adi-axi-dac.c
> > index cc31e1dcd1df..e883cd557b6a 100644
> > --- a/drivers/iio/dac/adi-axi-dac.c
> > +++ b/drivers/iio/dac/adi-axi-dac.c
> > @@ -783,6 +783,7 @@ static int axi_dac_probe(struct platform_device *pdev)
> >  {
> >  	struct axi_dac_state *st;
> >  	const struct axi_dac_info *info;
> > +	struct platform_device *child_pdev;
> >  	void __iomem *base;
> >  	unsigned int ver;
> >  	struct clk *clk;
> > @@ -862,6 +863,20 @@ 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) {
> 
> This should use fwnode_for_each_available_child_node() so that it skips
> nodes with status != "okay".
> 
> Would be nice to introduce a scoped version of this function too.
> 
> Also, if we are allowing multiple devices on the bus, the DT bindings
> need to have a reg property that is unique for each child.
> 
> > +		struct platform_device_info pi;
> > +
> > +		memset(&pi, 0, sizeof(pi));
> 
> struct platform_device_info pi = { };
> 
> avoids the need for memset().
> 
> > +
> > +		pi.name = fwnode_get_name(child);
> > +		pi.id = PLATFORM_DEVID_AUTO;
> > +		pi.fwnode = child;
> 
> Need to have pi.parent = &pdev->dev;
> 
> It could also make sense to have all of the primary bus functions
> (reg read/write, ddr enable/disable, etc.) passed here as platform
> data instead of having everything go through the IIO backend.

Note that ddr enable/disable is something that makes sense to be in the backend
anyways as it is something that exists in LVDS/CMOS interfaces that are only running
the dataplane. Bus operations like read/write could make sense but that would mean an
interface directly between the axi-dac and the child devices (bypassing the backend
or any other middle layer - maybe we could create a tiny adi-axi-bus layer on the IIO
topdir or any other place in IIO) which I'm not so sure (and is a bit odd). OTOH,
this bus stuff goes a bit out of scope of the backend main idea/goal so yeah... Well,
let's see what others have to say about it but I don't dislike the idea.

> 
> > +
> > +		child_pdev = platform_device_register_full(&pi);
> > +		if (IS_ERR(child_pdev))
> > +			return PTR_ERR(child_pdev);
> 
> These devices need to be unregistered on any error return and when
> the parent device is removed.
> 

Definitely this needs to be tested by manually unbinding the axi-dac device for
example. I'm not really sure how this will look like and if there's any problem in
removing twice the same device (likely there is). The thing is that when we connect a
frontend with it's backend, a devlink is created (that guarantees that the frontend
is removed before the backend). So, I'm fairly confident that if we add a devm action
in here to unregister the child devices, by the time we unregister the child, it
should be already gone (unless driver core somehow handles this).

All of the above needs careful testing but one way out it (and since in here we have
the parent - child relationship), we could add a boolean flag 'skip_devlink' to
'struct iio_backend_info' so that devlinks are skipped on these arrangements. Or we
could automatically detect that the frontend is a child of the backend and skip the
link (though an explicit flag might be better).

- Nuno Sá

>