diff mbox series

[4/5] gpio: adp5585: Add Analog Devices ADP5585 support

Message ID 20240520195942.11582-5-laurent.pinchart@ideasonboard.com
State Handled Elsewhere
Headers show
Series ADP5585 GPIO expander, PWM and keypad controller support | expand

Commit Message

Laurent Pinchart May 20, 2024, 7:59 p.m. UTC
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.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes compared to the NXP original version

- Add MAINTAINERS entry
- Add prefix to compatible string
- Switch to regmap
- White space fixes
- Use sizeof(*variable)
- Initialize variables at declaration time
- Use mutex scope guards
- Cleanup header includes
- Support R5 GPIO pin
- Reorder functions
- Add bias support
- Return real pin value from .get()
- Add debounce support
- Add drive mode support
- Destroy mutex on remove
- Update copyright
- Update license to GPL-2.0-only
---
 MAINTAINERS                 |   1 +
 drivers/gpio/Kconfig        |   7 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-adp5585.c | 232 ++++++++++++++++++++++++++++++++++++
 4 files changed, 241 insertions(+)
 create mode 100644 drivers/gpio/gpio-adp5585.c

Comments

Linus Walleij May 28, 2024, 11:54 a.m. UTC | #1
Hi Laurent/Haibo,

thanks for your patch!

On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:

> 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.
>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(...)

> +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);
> +
> +       guard(mutex)(&adp5585_gpio->lock);
> +
> +       return regmap_update_bits(adp5585_gpio->regmap,
> +                                 ADP5585_GPIO_DIRECTION_A + bank,
> +                                 bit, 0);

First, I love the guarded mutex!

But doesn't regmap already contain a mutex? Or is this one of those
cases where regmap has been instantiated without a lock?

> +       gc = &adp5585_gpio->gpio_chip;
> +       gc->parent = dev;
> +       gc->direction_input  = adp5585_gpio_direction_input;
> +       gc->direction_output = adp5585_gpio_direction_output;

And chance to implemen ->get_direction()?

Other than this I think the driver is ready for merge, so with the
comments fixed or addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Laurent Pinchart May 28, 2024, 12:27 p.m. UTC | #2
Hi Linus,

On Tue, May 28, 2024 at 01:54:29PM +0200, Linus Walleij wrote:
> On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart wrote:
> 
> > 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.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> (...)
> 
> > +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);
> > +
> > +       guard(mutex)(&adp5585_gpio->lock);
> > +
> > +       return regmap_update_bits(adp5585_gpio->regmap,
> > +                                 ADP5585_GPIO_DIRECTION_A + bank,
> > +                                 bit, 0);
> 
> First, I love the guarded mutex!

Yes, it's neat :-)

> But doesn't regmap already contain a mutex? Or is this one of those
> cases where regmap has been instantiated without a lock?

regmap indeed includes a lock, but it will lock each register access
independently. In adp5585_gpio_get_value() we need to read two
registers atomically, so we need to cover them by a single lock.

I could drop the lock from regmap, but I would then likely need to
introduce a lock in the parent mfd device that both the PWM and GPIO
children would use to protect bus access. That may make sense in the
future, but is a bit overkill for now I think.

> > +       gc = &adp5585_gpio->gpio_chip;
> > +       gc->parent = dev;
> > +       gc->direction_input  = adp5585_gpio_direction_input;
> > +       gc->direction_output = adp5585_gpio_direction_output;
> 
> And chance to implemen ->get_direction()?

Sure, I'll add that to v2.

> Other than this I think the driver is ready for merge, so with the
> comments fixed or addressed:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Thank you.
Laurent Pinchart May 28, 2024, 6:09 p.m. UTC | #3
On Tue, May 28, 2024 at 03:27:34PM +0300, Laurent Pinchart wrote:
> Hi Linus,
> 
> On Tue, May 28, 2024 at 01:54:29PM +0200, Linus Walleij wrote:
> > On Mon, May 20, 2024 at 9:59 PM Laurent Pinchart wrote:
> > 
> > > 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.
> > >
> > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > (...)
> > 
> > > +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);
> > > +
> > > +       guard(mutex)(&adp5585_gpio->lock);
> > > +
> > > +       return regmap_update_bits(adp5585_gpio->regmap,
> > > +                                 ADP5585_GPIO_DIRECTION_A + bank,
> > > +                                 bit, 0);
> > 
> > First, I love the guarded mutex!
> 
> Yes, it's neat :-)
> 
> > But doesn't regmap already contain a mutex? Or is this one of those
> > cases where regmap has been instantiated without a lock?
> 
> regmap indeed includes a lock, but it will lock each register access
> independently. In adp5585_gpio_get_value() we need to read two
> registers atomically, so we need to cover them by a single lock.
> 
> I could drop the lock from regmap, but I would then likely need to
> introduce a lock in the parent mfd device that both the PWM and GPIO
> children would use to protect bus access. That may make sense in the
> future, but is a bit overkill for now I think.

Actually, I think I can drop the lock. Concurrent access to the
registers from different GPIOs are protected by the regmap lock, and
concurrent access to groups of registers for the same GPIO isn't a valid
use case as callers shouldn't do that.

> > > +       gc = &adp5585_gpio->gpio_chip;
> > > +       gc->parent = dev;
> > > +       gc->direction_input  = adp5585_gpio_direction_input;
> > > +       gc->direction_output = adp5585_gpio_direction_output;
> > 
> > And chance to implemen ->get_direction()?
> 
> Sure, I'll add that to v2.
> 
> > Other than this I think the driver is ready for merge, so with the
> > comments fixed or addressed:
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Thank you.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 7150f091b69b..5689fec270ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -503,6 +503,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 b50d0b470849..79d004e31a47 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1206,6 +1206,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 fdd28c58d890..b44b6bc3a74f 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..30e538f8d1ac
--- /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/mutex.h>
+#include <linux/of.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;
+	struct mutex lock;
+};
+
+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);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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;
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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 val;
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	regmap_read(adp5585_gpio->regmap, ADP5585_GPI_STATUS_A + 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);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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);
+
+	guard(mutex)(&adp5585_gpio->lock);
+
+	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(&pdev->dev, sizeof(*adp5585_gpio), GFP_KERNEL);
+	if (!adp5585_gpio)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, adp5585_gpio);
+
+	adp5585_gpio->regmap = adp5585->regmap;
+
+	mutex_init(&adp5585_gpio->lock);
+
+	gc = &adp5585_gpio->gpio_chip;
+	gc->parent = dev;
+	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(&pdev->dev, &adp5585_gpio->gpio_chip,
+				     adp5585_gpio);
+	if (ret) {
+		mutex_destroy(&adp5585_gpio->lock);
+		return dev_err_probe(&pdev->dev, ret, "failed to add GPIO chip\n");
+	}
+
+	return 0;
+}
+
+static void adp5585_gpio_remove(struct platform_device *pdev)
+{
+	struct adp5585_gpio_dev *adp5585_gpio = platform_get_drvdata(pdev);
+
+	mutex_destroy(&adp5585_gpio->lock);
+}
+
+static const struct of_device_id adp5585_of_match[] = {
+	{ .compatible = "adi,adp5585-gpio" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, adp5585_of_match);
+
+static struct platform_driver adp5585_gpio_driver = {
+	.driver	= {
+		.name = "adp5585-gpio",
+		.of_match_table = adp5585_of_match,
+	},
+	.probe = adp5585_gpio_probe,
+	.remove_new = adp5585_gpio_remove,
+};
+module_platform_driver(adp5585_gpio_driver);
+
+MODULE_AUTHOR("Haibo Chen <haibo.chen@nxp.com>");
+MODULE_DESCRIPTION("GPIO ADP5585 Driver");
+MODULE_LICENSE("GPL");