On Sun, 2024-09-08 at 13:36 +0100, Jonathan Cameron wrote: > On Fri, 06 Sep 2024 09:08:59 +0200 > Nuno Sá <noname.nuno@xxxxxxxxx> wrote: > > > 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@xxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxx> > > > > Co-developed-by: David Lechner <dlechner@xxxxxxxxxxxx> > > > > Co-developed-by: Nuno Sá <nuno.sa@xxxxxxxxxx> > > > > --- > > > > 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". > > Ah. That oddity strikes again... > > > > > > > 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. > > For the read/write using platform data does seem reasonable to me. > Agreed that DDR is dataplane (at least sometimes) so backend ops probably > appropriate. > > > > > > > > > > + > > > > + 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). > > Agreed it needs testing but I'm not sure why it would already have gone. > The driver would have unbound, but the platform device /child would still be > there > I think at time of removal. Can probably get away with devm to tear > it down when the backend device then goes away. > > Maybe I'm missing some subtlety though. > Hmm, I guess there was some confusion of me not explaining myself. What I wanted to say is that we will be unregistering the same device twice. But yeah, maybe it ends up just being a NOP in the driver core. But yeah, we definitely need to test this and make sure that the device is only gone by the time the parent device calls platform_device_unregister() on it. - Nuno Sá