Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

[Andy Shevchenko]

On Wed, Mar 26, 2025 at 06:46:41PM +0100, Uwe Kleine-König wrote:
> On Wed, Mar 26, 2025 at 05:49:07PM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 26, 2025 at 03:44:28PM +0100, Mathieu Dubois-Briand wrote:
> > > On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote:
> > > > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote:
> > > > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote:
> > > > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote:
> > > > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote:
> > > > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@xxxxxxxxxxx wrote:

...

> > > > > > > > > +	chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0);
> > > > > > > > 
> > > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may
> > > > > > > > not be realised. If you really need this, it has to have a very good comment
> > > > > > > > explaining why and object lifetimes.
> > > > > > > 
> > > > > > > Pretty sure this is broken. This results for example in the device link
> > > > > > > being created on the parent. So if the pwm devices goes away a consumer
> > > > > > > might not notice (at least in the usual way). I guess this was done to
> > > > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that
> > > > > > > needs a different adaption. That will probably involve calling
> > > > > > > device_set_of_node_from_dev().
> > > > > >
> > > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by
> > > > > > default. I believe it should just work if we drop that '->parent' part.
> > > > > 
> > > > > Are you sure about that?
> > > >
> > > > Yes and no. If your DT looks like (pseudo code as I don't know
> > > > DTS syntax by heart):
> > > >
> > > > 	device: {
> > > > 		parent-property = value;
> > > > 		child0:
> > > > 			...
> > > > 		child1:
> > > > 			...
> > > > 	}
> > > >
> > > > the parent-property value is automatically accessible via fwnode API,
> > > > but I don't know what will happen to the cases when each of the children
> > > > has its own compatible string. This might be your case, but again,
> > > > I'm not an expert in DT.
> > > >
> > > 
> > > On my side:
> > > - Some MFD child do have a child node in the device tree, with an
> > >   associated compatible value. No problem for these, they do get correct
> > >   of_node/fwnode values pointing on the child device tree node.
> > > - Some MFD child do not have any node in the device tree, and for these,
> > >   they have to use properties from the parent (MFD) device tree node.
> > >   And here we do have some problems.
> > > 
> > > > > On my side it does not work if I just drop the '->parent', this is why I
> > > > > ended whit this (bad) pattern.
> > > >
> > > > > Now it does work if I do call device_set_of_node_from_dev() manually,
> > > >
> > > > AFAICT, this is wrong API to be called in the children. Are you talking about
> > > > parent code?
> > > >
> > > 
> > > I believe I cannot do it in the parent code, as I would need to do it
> > > after the call to devm_mfd_add_devices(), and so it might happen after
> > > the probe. I still tried to see how it behaved, and it looks like PWM
> > > core really did not expect to get an of_node assigned to the device
> > > after adding the PWM device.
> > > 
> > > So either I can do something in MFD core or in sub devices probe(), or I
> > > need to come with a different way to do things.
> > > 
> > > > > so it's definitely better. But I believe the MFD core is not propagating
> > > > > OF data, and I did not find where it would do that in the code. Yet it
> > > > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we
> > > > > do something bad in our MFD driver?
> > > >
> > > > ...or MFD needs something to have... Dunno.
> > > 
> > > I have something working with a very simple change in mfd-core.c, but
> > > I'm really not confident it won't break anything else. I wish I could
> > > get some insights from an MFD expert.
> > > 
> > > @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id,
> > >                 if (!pdev->dev.of_node)
> > >                         pr_warn("%s: Failed to locate of_node [id: %d]\n",
> > >                                 cell->name, platform_id);
> > > +       } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) {
> > > +               device_set_of_node_from_dev(&pdev->dev, parent);
> > 
> > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > and on the second call to it you will have a warning from refcount library.
> 
> device_set_of_node_from_dev() does:
> 
> 	of_node_put(pdev->dev->of_node);
> 	pdev->dev->of_node = of_node_get(parent->of_node);
> 	pdev->dev->of_node_reused = true;
> 
> Note that pdev isn't the platform device associated with the parent
> device but the just allocated one representing the subdevice so
> pdev->dev->of_node is NULL and the parent refcount isn't dropped.

Ah, I stand corrected, thanks! Okay, so what it does basically, it drops
a reference for the child, and propagates the parent's node at the same time
bumping its reference count. Sounds legit, but this should be done equally for
DT, ACPI and software node cases.

> But I'm unsure if this is the right place to call it or if
> device_set_node() is indeed enough

This all about node reference counting. Whatever the correct choice for DT.
Anyway this sounds right to do for any of the system, but also note, that
this API is good when device node is not backed by the struct device,
otherwise, the struct device reference count covers the device node AFAIU.

TL;DR: What are the object lifetimes for struct device and struct device_node
(struct fwnode_handle)?

> (also I wonder if
> device_set_of_node_from_dev() should care for fwnode).
> I'll keep that question for someone else.

-- 
With Best Regards,
Andy Shevchenko