Re: [PATCH v2 3/4] gpio: adp5585: Add Analog Devices ADP5585 support

On Wed, May 29, 2024 at 5:35 PM Laurent Pinchart
[off-list ref] wrote:

On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote:

quoted

On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote:

quoted

On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote:

quoted

On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote:

quoted

On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote:

...

quoted

quoted

quoted

quoted

quoted

quoted

+   device_set_of_node_from_dev(dev, dev->parent);

Why not device_set_node()?

Because device_set_of_node_from_dev() is meant for this exact use case,
where the same node is used for multiple devices. It also puts any
previous dev->of_node, ensuring proper refcounting when devices are
unbound and rebound, without being deleted.

When will the refcount be dropped (in case of removal of this device)?
Or you mean it shouldn't?

Any refcount taken on the OF node needs to be dropped. The device core
only drops the refcount when the device is being deleted, not when
there's an unbind-rebind cycle without deletion of the device (as
happens for instance when the module is unloaded and reloaded).

Under "device" you meant the real hardware, as Linux device (instance
of the struct device object) is being rebuilt AFAIK)?

I mean struct device. The driver core will drop the reference in
platform_device_release(), called when the last reference to the
platform device is released, just before freeing the platform_device
instance. This happens after the device is removed from the system (e.g.
hot-unplug), but not when a device is unbound from a driver and rebound
(e.g. module unload and reload).

This is something I need to refresh in my memory. Any pointers (the
links to the exact code lines are also okay) where I can find the
proof of what you are saying. (It's not that I untrust you, it's just
that I take my time on studying it.)

quoted

quoted

This has
to be handled by the driver. device_set_of_node_from_dev() handles it.

But why do you need to keep a parent node reference bumped?
Only very few drivers in the kernel use this API and I believe either

s/very/a/ (sorry for the confusion)

quoted

nobody knows what they are doing and you are right, or you are doing
something which is not needed.

I need to set the of_node and fwnode fields of struct device to enable
OF-based lookups of GPIOs and PWMs. The of_node field is meant to be
populated by the driver core when the device is created, with a
reference to the OF node. When populated directly by driver, this needs
to be taken into account, and drivers need to ensure the reference will
be released correctly. device_set_of_node_from_dev() is meant for that.

What you are doing is sharing the parent node with the child, but at
the same time you bump the parent's reference count. As this is a
child of MFD I don't think you need this as MFD already takes care of
it via parent -> child natural dependencies. Is it incorrect
understanding?

-- 
With Best Regards,
Andy Shevchenko