Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support
From: Mathieu Dubois-Briand
Date: Fri Mar 28 2025 - 04:13:34 EST
On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote:
> On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote:
> > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:
>
> > > 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.
> > >
> > > It should be as simple as device_set_node().
> > >
> > > > }
> > >
> > > With that, the conditional becomes
> > >
> > > } else if (is_of_node(fwnode)) {
> > > device_set_node(&pdev->dev, fwnode);
> > > }
> > >
> > > where fwnode is something like
> > >
> > > struct fwnode_handle *fwnode = dev_fwnode(parent);
> >
> > I tried to use device_set_node(), but then I got some other issue: as we
> > now have several devices with the same firmware node, they all share the
> > same properties. In particular, if we do use pinctrl- properties to
> > apply some pinmmuxing, all devices will try to apply this pinmuxing and
> > of course all but one will fail.
> >
> > And this makes me think again about the whole thing, maybe copying the
> > fwnode or of_node from the parent is not the way to go.
> >
> > So today we rely on the parent node for four drivers:
> > - keypad and rotary, just to ease a bit the parsing of some properties,
> > such as the keymap with matrix_keypad_build_keymap(). I can easily do
> > it another way.
> > - PWM and pinctrl drivers, are a bit more complicated, as in both case
> > the device tree node associated with the device is used internally. In
> > one case to find the correct PWM device for PWM clients listed in the
> > device tree, in the other case to find the pinctrl device when
> > applying pinctrl described in the device tree.
> >
> > So maybe I have to find a better way for have this association. One way
> > would be to modify the device tree bindings to add a PWM and a pinctrl
> > node, with their own compatible, so they are associated to the
> > corresponding device. But maybe there is a better way to do it.
>
> Okay, so the main question now, why do the device share their properties
> to begin with? It can be done via fwnode graph or similar APIs (in case
> it is _really_ needed).
I wouldn't say the properties are shared: we have a single node in the
device tree as this is just one device. But as we create several
(software) devices in the MFD driver, we now have several devices linked
with a single device tree node.
One solution would be to create more subnodes in the device tree, one
for pinctrl and one for PWM, but this feels a bit like describing our
software implementation in the device tree instead of describing the
hardware.
--
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com