| Message ID | 20240528190315.3865-1-laurent.pinchart@ideasonboard.com |
|---|---|
| Headers | show |
| Series | ADP5585 GPIO expander, PWM and keypad controller support | expand |
Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti: > From: Haibo Chen <haibo.chen@nxp.com> > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > This driver supports the GPIO function using the platform device > registered by the core MFD driver. > > The driver is derived from an initial implementation from NXP, available > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > adp5585-gpio support") in their BSP kernel tree. It has been extensively > rewritten. Why is this not using gpio-regmap? ... > +#include <linux/device.h> > +#include <linux/gpio/driver.h> > +#include <linux/mfd/adp5585.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> + types.h ... > + bit = off * 2 + (off > 5 ? 4 : 0); Right, but can you use >= 6 here which immediately follows to the next question, i.e. why not use bank in this conditional? ... > + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); (see below) > + struct adp5585_gpio_dev *adp5585_gpio; > + struct device *dev = &pdev->dev; struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > + struct gpio_chip *gc; > + int ret; ... > + platform_set_drvdata(pdev, adp5585_gpio); Any use of driver data? ... > + device_set_of_node_from_dev(dev, dev->parent); Why not device_set_node()?
Hi Andy, Thank you for the patch. On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: > Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > matrix decoder, programmable logic, reset generator, and PWM generator. > > This driver supports the GPIO function using the platform device > > registered by the core MFD driver. > > > > The driver is derived from an initial implementation from NXP, available > > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > > adp5585-gpio support") in their BSP kernel tree. It has been extensively > > rewritten. > > Why is this not using gpio-regmap? > > ... > > > +#include <linux/device.h> > > +#include <linux/gpio/driver.h> > > +#include <linux/mfd/adp5585.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/regmap.h> > > + types.h > > ... > > > + bit = off * 2 + (off > 5 ? 4 : 0); > > Right, but can you use >= 6 here which immediately follows to the next > question, i.e. why not use bank in this conditional? The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a set of registers with the same layout. Here the layout is different, the registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd rather not use ADP5585_BANK() either. I have decided to use > 5 instead of >= 6 to match the R5 field name in the comment above: /* * The bias configuration fields are 2 bits wide and laid down in * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits * after R5. */ > ... > > > + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > (see below) > > > + struct adp5585_gpio_dev *adp5585_gpio; > > + struct device *dev = &pdev->dev; > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); I prefer keeping the current ordering, with long lines first, I think that's more readable. > > + struct gpio_chip *gc; > > + int ret; > > ... > > > + platform_set_drvdata(pdev, adp5585_gpio); > > Any use of driver data? In v1, not v2. I'll drop it. > ... > > > + 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.
On Tue, May 28, 2024 at 11:22:18PM +0300, Laurent Pinchart wrote: > On Tue, May 28, 2024 at 11:20:45PM +0300, Laurent Pinchart wrote: > > Hi Andy, > > > > Thank you for the patch. I meant for the review. It's getting late. > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: > > > Tue, May 28, 2024 at 10:03:13PM +0300, Laurent Pinchart kirjoitti: > > > > From: Haibo Chen <haibo.chen@nxp.com> > > > > > > > > The ADP5585 is a 10/11 input/output port expander with a built in keypad > > > > matrix decoder, programmable logic, reset generator, and PWM generator. > > > > This driver supports the GPIO function using the platform device > > > > registered by the core MFD driver. > > > > > > > > The driver is derived from an initial implementation from NXP, available > > > > in commit 451f61b46b76 ("MLK-25917-2 gpio: adp5585-gpio: add > > > > adp5585-gpio support") in their BSP kernel tree. It has been extensively > > > > rewritten. > > > > > > Why is this not using gpio-regmap? > > I forgot to answer this: > > I don't think it's a good match for the hardware. > > > > ... > > > > > > > +#include <linux/device.h> > > > > +#include <linux/gpio/driver.h> > > > > +#include <linux/mfd/adp5585.h> > > > > +#include <linux/module.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/regmap.h> > > > > > > + types.h > > > > > > ... > > > > > > > + bit = off * 2 + (off > 5 ? 4 : 0); > > > > > > Right, but can you use >= 6 here which immediately follows to the next > > > question, i.e. why not use bank in this conditional? > > > > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a > > set of registers with the same layout. Here the layout is different, the > > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd > > rather not use ADP5585_BANK() either. I have decided to use > 5 instead > > of >= 6 to match the R5 field name in the comment above: > > > > /* > > * The bias configuration fields are 2 bits wide and laid down in > > * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits > > * after R5. > > */ > > > > > ... > > > > > > > + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > > > > (see below) > > > > > > > + struct adp5585_gpio_dev *adp5585_gpio; > > > > + struct device *dev = &pdev->dev; > > > > > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > > > > I prefer keeping the current ordering, with long lines first, I think > > that's more readable. > > > > > > + struct gpio_chip *gc; > > > > + int ret; > > > > > > ... > > > > > > > + platform_set_drvdata(pdev, adp5585_gpio); > > > > > > Any use of driver data? > > > > In v1, not v2. I'll drop it. > > > > > ... > > > > > > > + 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. > > > > -- > > Regards, > > > > Laurent Pinchart > > -- > Regards, > > Laurent Pinchart
On Tue, 28 May 2024 22:03:11 +0300, Laurent Pinchart wrote: > The ADP5585 is a 10/11 input/output port expander with a built in keypad > matrix decoder, programmable logic, reset generator, and PWM generator. > These bindings model the device as an MFD, and support the GPIO expander > and PWM functions. > > These bindings support the GPIO and PWM functions. > > Drop the existing adi,adp5585 and adi,adp5585-02 compatible strings from > trivial-devices.yaml. They have been added there by mistake as the > driver that was submitted at the same time used different compatible > strings. We can take them over safely. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > I've limited the bindings to GPIO and PWM as I lack hardware to design, > implement and test the rest of the features the chip supports. > > Changes since v1: > > - Squash "dt-bindings: trivial-devices: Drop adi,adp5585 and > adi,adp5585-02" into this patch > - Merge child nodes into parent node > --- > .../devicetree/bindings/mfd/adi,adp5585.yaml | 107 ++++++++++++++++++ > .../devicetree/bindings/trivial-devices.yaml | 4 - > MAINTAINERS | 7 ++ > 3 files changed, 114 insertions(+), 4 deletions(-) > create mode 100644 Documentation/devicetree/bindings/mfd/adi,adp5585.yaml > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml# /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/adi,adp5585.example.dtb: mfd@34: 'gpio' is a required property from schema $id: http://devicetree.org/schemas/mfd/adi,adp5585.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240528190315.3865-2-laurent.pinchart@ideasonboard.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: ... > > > + bit = off * 2 + (off > 5 ? 4 : 0); > > > > Right, but can you use >= 6 here which immediately follows to the next > > question, i.e. why not use bank in this conditional? > > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a > set of registers with the same layout. Here the layout is different, the > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd > rather not use ADP5585_BANK() either. I have decided to use > 5 instead > of >= 6 to match the R5 field name in the comment above: > > /* > * The bias configuration fields are 2 bits wide and laid down in > * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits > * after R5. > */ First of all, the 5 sounds misleading as one needs to think about "how many are exactly per the register" and the answer AFAIU is 6. >= 6 shows this. Second, I haven't mentioned _BANK(), what I meant is something to unsigned int bank = ... >= 6 ? : ; ... > > > + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > > (see below) > > > > > + struct adp5585_gpio_dev *adp5585_gpio; > > > + struct device *dev = &pdev->dev; > > > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > > I prefer keeping the current ordering, with long lines first, I think > that's more readable. Does the compiler optimise these two? > > > + struct gpio_chip *gc; > > > + int ret; ... > > > + 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?
On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote: > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote: > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: > > ... > > > > > + bit = off * 2 + (off > 5 ? 4 : 0); > > > > > > Right, but can you use >= 6 here which immediately follows to the next > > > question, i.e. why not use bank in this conditional? > > > > The ADP5585_BANK() macro is meant to be used with ADP5585_BIT(), for a > > set of registers with the same layout. Here the layout is different, the > > registers contain multi-bit fields. I can't use ADP5585_BIT(), so I'd > > rather not use ADP5585_BANK() either. I have decided to use > 5 instead > > of >= 6 to match the R5 field name in the comment above: > > > > /* > > * The bias configuration fields are 2 bits wide and laid down in > > * consecutive registers ADP5585_RPULL_CONFIG_*, with a hole of 4 bits > > * after R5. > > */ > > First of all, the 5 sounds misleading as one needs to think about "how > many are exactly per the register" and the answer AFAIU is 6. >= 6 > shows this. Second, I haven't mentioned _BANK(), what I meant is > something to > > unsigned int bank = ... >= 6 ? : ; That doesn't reflect the organisation of the bits in the registers. If you're interested, please check the datasheet. > ... > > > > > + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); > > > > > > (see below) > > > > > > > + struct adp5585_gpio_dev *adp5585_gpio; > > > > + struct device *dev = &pdev->dev; > > > > > > struct adp5585_dev *adp5585 = dev_get_drvdata(dev->parent); > > > > I prefer keeping the current ordering, with long lines first, I think > > that's more readable. > > Does the compiler optimise these two? If anyone is interested in figuring out, I'll let them test :-) > > > > + struct gpio_chip *gc; > > > > + int ret; > > ... > > > > > + 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). This has to be handled by the driver. device_set_of_node_from_dev() handles it.
On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote: > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote: > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: ... > > > > > + 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)? > 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 nobody knows what they are doing and you are right, or you are doing something which is not needed. -- With Best Regards, Andy Shevchenko
On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote: > > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote: > > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote: > > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > + 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 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 > 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.
On Wed, May 29, 2024 at 5:35 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote: > > On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote: > > > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote: > > > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote: > > > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: ... > > > > > > > + 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.) > > > 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) > > 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?
On Wed, May 29, 2024 at 06:00:19PM +0300, Andy Shevchenko wrote: > On Wed, May 29, 2024 at 5:35 PM Laurent Pinchart wrote: > > On Wed, May 29, 2024 at 05:24:03PM +0300, Andy Shevchenko wrote: > > > On Wed, May 29, 2024 at 12:48 PM Laurent Pinchart wrote: > > > > On Wed, May 29, 2024 at 09:16:43AM +0300, Andy Shevchenko wrote: > > > > > On Tue, May 28, 2024 at 11:20 PM Laurent Pinchart wrote: > > > > > > On Tue, May 28, 2024 at 10:36:06PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > > > + 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.) I wish this was documented, I could then point you to a nice text that explains it all :-) My understanding comes from reading platform_device_release(), and from failing to find other of_node_put() calls that would be relevant to this. > > > > 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) > > > > 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? It is true that the parent device will not be destroyed as long as the child device exists. The parent device's of_node will thus be kept around by the reference that the parent device holds on it. However, if I set dev.of_node for the child device without acquiring a reference, we will have dev.of_node pointing to the same device_node, with a single reference to it. This means that when the child device is destroyed and platform_device_release() is called for it, of_node_put() will release that reference, and the parent dev.of_node will point to a device_node without holding a reference. Then, when the parent device, which is an i2c_client, is unregistered, the call to i2c_unregister_device() will call of_node_put(), releasing a reference we don't have. The order of i2c_unregister_device() and platform_device_release() may be swapped in practice, I haven't double-checked, but the reasoning still holds. We have two functions that expect dev.of_node to hold a reference, so both dev.of_node need to hold a reference. Another way to handle the problem would be to borrow the parent's reference in the probe function of the child, and set dev.of_node to NULL manually in the child .remove() function. This will ensure that platform_device_release(), which is called after .remove() (not necessarily directly after, but certainly not before), will not attempt to incorrectly release a reference on dev.of_node. This will however not be safe if i2c_unregister_device() is called before the child .remove(), as the child dev.of_node would then for some amount of time point to a device_node without a reference. TL;DR: Using device_set_of_node_from_dev() seems the safest option, especially given that it was designed for this purpose.