Re: [PATCH v6 03/12] pinctrl: Add MAX7360 pinctrl driver

[Andy Shevchenko]

On Wed, Apr 09, 2025 at 05:03:02PM +0200, Mathieu Dubois-Briand wrote:
> On Wed Apr 9, 2025 at 4:55 PM CEST, Mathieu Dubois-Briand wrote:
> > Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins
> > can be used either for GPIO, PWM or rotary encoder functionalities.

...

The all the rest of the driver LGTM, but the below.

> > +	device_set_of_node_from_dev(dev, dev->parent);
> 
> Ok, so this goes a bit against what I said I was going to do on my
> previous series, let me explain why. Same reasoning applies for both
> uses, in PWM and pinctrl drivers.
> 
> With my previous experiments, I came to the conclusion that:
> - Either we should use device_set_of_node_from_dev() as I do here.
> - Or we should add more subnodes in the device tree binding.

> - Also, copying the fwnode with device_set_node() was not possible, as
>   the kernel would then try to apply pinctrl on both the parent and
>   child device.

Hmm... I need to refresh my memory with the old discussions. Can you point out
to the problem statement with that approach?

> I previously said the second solution was probably the way to go, but I
> changed my mind for two reasons.
> 
> First having more subnodes in the device tree was already rejected in
> the past in the reviews of the dt-bindings patch. This do makes sense as
> it would be describing device internals (which should not be made in
> DT), just to ease one specific software implementation (which should
> also be avoided). So I believe this change would again be rejected.
> https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@xxxxxxxxxx/
> 
> But the the second reason is, doing
> 'git grep "device_set_of_node_from_dev.*parent"', I found several
> drivers using device_set_of_node_from_dev() for a similar need. Some of
> these uses are also for MFD child devices:
> - gpio-adp5585.c / pwm-adp5585.c,
> - pwm-ntxec.c,
> - max77620-regulator.c / max77620_thermal.c.
> 
> So, based on this, I believe using device_set_of_node_from_dev() in
> these two drivers is the way to go.

The problem with this solution is that, It's OF-centric. Which shouldn't be
done in a new code (and I don't see impediments to avoid it). Yes, it does
the right thing for the case, but only on OF systems. Note, fwnode is a list
of maximum of two entries (yeah, designed like that right now), can you utilise
that somehow?

-- 
With Best Regards,
Andy Shevchenko