| Message ID | 20240528190315.3865-4-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| 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:20:45PM +0300, Laurent Pinchart wrote: > 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? 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
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, 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.
diff --git a/MAINTAINERS b/MAINTAINERS index 77b93fbdf5cc..9c560d9a590a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -532,6 +532,7 @@ L: linux-gpio@vger.kernel.org L: linux-pwm@vger.kernel.org S: Maintained F: Documentation/devicetree/bindings/*/adi,adp5585*.yaml +F: drivers/gpio/gpio-adp5585.c F: drivers/mfd/adp5585.c F: include/linux/mfd/adp5585.h diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 3dbddec07028..67286886db5e 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1233,6 +1233,13 @@ config GPIO_ADP5520 This option enables support for on-chip GPIO found on Analog Devices ADP5520 PMICs. +config GPIO_ADP5585 + tristate "GPIO Support for ADP5585" + depends on MFD_ADP5585 + help + This option enables support for the GPIO function found in the Analog + Devices ADP5585. + config GPIO_ALTERA_A10SR tristate "Altera Arria10 System Resource GPIO" depends on MFD_ALTERA_A10SR diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index e2a53013780e..04bfa2bc7e11 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -26,6 +26,7 @@ obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o obj-$(CONFIG_GPIO_74XX_MMIO) += gpio-74xx-mmio.o obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o +obj-$(CONFIG_GPIO_ADP5585) += gpio-adp5585.o obj-$(CONFIG_GPIO_AGGREGATOR) += gpio-aggregator.o obj-$(CONFIG_GPIO_ALTERA_A10SR) += gpio-altera-a10sr.o obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o diff --git a/drivers/gpio/gpio-adp5585.c b/drivers/gpio/gpio-adp5585.c new file mode 100644 index 000000000000..bd6b87ec5dac --- /dev/null +++ b/drivers/gpio/gpio-adp5585.c @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Analog Devices ADP5585 GPIO driver + * + * Copyright 2022 NXP + * Copyright 2024 Ideas on Board Oy + */ + +#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> + +#define ADP5585_GPIO_MAX 11 + +struct adp5585_gpio_dev { + struct gpio_chip gpio_chip; + struct regmap *regmap; +}; + +static int adp5585_gpio_get_direction(struct gpio_chip *chip, unsigned int off) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + unsigned int val; + + regmap_read(adp5585_gpio->regmap, ADP5585_GPIO_DIRECTION_A + bank, &val); + + return val & bit ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN; +} + +static int adp5585_gpio_direction_input(struct gpio_chip *chip, unsigned int off) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + + return regmap_update_bits(adp5585_gpio->regmap, + ADP5585_GPIO_DIRECTION_A + bank, + bit, 0); +} + +static int adp5585_gpio_direction_output(struct gpio_chip *chip, unsigned int off, int val) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + int ret; + + ret = regmap_update_bits(adp5585_gpio->regmap, + ADP5585_GPO_DATA_OUT_A + bank, bit, + val ? bit : 0); + if (ret) + return ret; + + return regmap_update_bits(adp5585_gpio->regmap, + ADP5585_GPIO_DIRECTION_A + bank, + bit, bit); +} + +static int adp5585_gpio_get_value(struct gpio_chip *chip, unsigned int off) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + unsigned int reg; + unsigned int val; + + /* + * The input status register doesn't reflect the pin state when the + * GPIO is configured as an output. Check the direction, and read the + * input status from GPI_STATUS or output value from GPO_DATA_OUT + * accordingly. + * + * We don't need any locking, as concurrent access to the same GPIO + * isn't allowed by the GPIO API, so there's no risk of the + * .direction_input(), .direction_output() or .set() operations racing + * with this. + */ + regmap_read(adp5585_gpio->regmap, ADP5585_GPIO_DIRECTION_A + bank, &val); + reg = val & bit ? ADP5585_GPO_DATA_OUT_A : ADP5585_GPI_STATUS_A; + regmap_read(adp5585_gpio->regmap, reg + bank, &val); + + return !!(val & bit); +} + +static void adp5585_gpio_set_value(struct gpio_chip *chip, unsigned int off, int val) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + + regmap_update_bits(adp5585_gpio->regmap, ADP5585_GPO_DATA_OUT_A + bank, + bit, val ? bit : 0); +} + +static int adp5585_gpio_set_bias(struct adp5585_gpio_dev *adp5585_gpio, + unsigned int off, unsigned int bias) +{ + unsigned int bit, reg, mask, val; + + /* + * 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. + */ + bit = off * 2 + (off > 5 ? 4 : 0); + reg = ADP5585_RPULL_CONFIG_A + bit / 8; + mask = ADP5585_Rx_PULL_CFG(bit % 8, ADP5585_Rx_PULL_CFG_MASK); + val = ADP5585_Rx_PULL_CFG(bit % 8, bias); + + return regmap_update_bits(adp5585_gpio->regmap, reg, mask, val); +} + +static int adp5585_gpio_set_drive(struct adp5585_gpio_dev *adp5585_gpio, + unsigned int off, enum pin_config_param drive) +{ + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + + return regmap_update_bits(adp5585_gpio->regmap, + ADP5585_GPO_OUT_MODE_A + bank, bit, + drive == PIN_CONFIG_DRIVE_OPEN_DRAIN ? 1 : 0); +} + +static int adp5585_gpio_set_debounce(struct adp5585_gpio_dev *adp5585_gpio, + unsigned int off, unsigned int debounce) +{ + unsigned int bank = ADP5585_BANK(off); + unsigned int bit = ADP5585_BIT(off); + + return regmap_update_bits(adp5585_gpio->regmap, + ADP5585_DEBOUNCE_DIS_A + bank, bit, + debounce ? 0 : 1); +} + +static int adp5585_gpio_set_config(struct gpio_chip *chip, unsigned int off, + unsigned long config) +{ + struct adp5585_gpio_dev *adp5585_gpio = gpiochip_get_data(chip); + enum pin_config_param param = pinconf_to_config_param(config); + u32 arg = pinconf_to_config_argument(config); + + switch (param) { + case PIN_CONFIG_BIAS_DISABLE: + return adp5585_gpio_set_bias(adp5585_gpio, off, + ADP5585_Rx_PULL_CFG_DISABLE); + + case PIN_CONFIG_BIAS_PULL_DOWN: + return adp5585_gpio_set_bias(adp5585_gpio, off, arg ? + ADP5585_Rx_PULL_CFG_PD_300K : + ADP5585_Rx_PULL_CFG_DISABLE); + + case PIN_CONFIG_BIAS_PULL_UP: + return adp5585_gpio_set_bias(adp5585_gpio, off, arg ? + ADP5585_Rx_PULL_CFG_PU_300K : + ADP5585_Rx_PULL_CFG_DISABLE); + + case PIN_CONFIG_DRIVE_OPEN_DRAIN: + case PIN_CONFIG_DRIVE_PUSH_PULL: + return adp5585_gpio_set_drive(adp5585_gpio, off, param); + + case PIN_CONFIG_INPUT_DEBOUNCE: + return adp5585_gpio_set_debounce(adp5585_gpio, off, arg); + + default: + return -ENOTSUPP; + }; +} + +static int adp5585_gpio_probe(struct platform_device *pdev) +{ + struct adp5585_dev *adp5585 = dev_get_drvdata(pdev->dev.parent); + struct adp5585_gpio_dev *adp5585_gpio; + struct device *dev = &pdev->dev; + struct gpio_chip *gc; + int ret; + + adp5585_gpio = devm_kzalloc(dev, sizeof(*adp5585_gpio), GFP_KERNEL); + if (!adp5585_gpio) + return -ENOMEM; + + platform_set_drvdata(pdev, adp5585_gpio); + + adp5585_gpio->regmap = adp5585->regmap; + + device_set_of_node_from_dev(dev, dev->parent); + + gc = &adp5585_gpio->gpio_chip; + gc->parent = dev; + gc->get_direction = adp5585_gpio_get_direction; + gc->direction_input = adp5585_gpio_direction_input; + gc->direction_output = adp5585_gpio_direction_output; + gc->get = adp5585_gpio_get_value; + gc->set = adp5585_gpio_set_value; + gc->set_config = adp5585_gpio_set_config; + gc->can_sleep = true; + + gc->base = -1; + gc->ngpio = ADP5585_GPIO_MAX; + gc->label = pdev->name; + gc->owner = THIS_MODULE; + + ret = devm_gpiochip_add_data(dev, &adp5585_gpio->gpio_chip, + adp5585_gpio); + if (ret) + return dev_err_probe(dev, ret, "failed to add GPIO chip\n"); + + return 0; +} + +static const struct platform_device_id adp5585_gpio_id_table[] = { + { "adp5585-gpio" }, + { /* Sentinel */ }, +}; +MODULE_DEVICE_TABLE(platform, adp5585_gpio_id_table); + +static struct platform_driver adp5585_gpio_driver = { + .driver = { + .name = "adp5585-gpio", + }, + .probe = adp5585_gpio_probe, + .id_table = adp5585_gpio_id_table, +}; +module_platform_driver(adp5585_gpio_driver); + +MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>"); +MODULE_DESCRIPTION("GPIO ADP5585 Driver"); +MODULE_LICENSE("GPL");