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

["Mathieu Dubois-Briand" <mathieu.dubois-briand@bootlin.com>]

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@bootlin.com 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);
        }


-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com