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

[Mathieu Dubois-Briand]

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);

Hi,

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.


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