[PATCH v4 0/2] gpio: generic regmap implementation

[PATCH v4 0/2] gpio: generic regmap implementation

[Michael Walle]

This series is a split off of the sl28cpld series:
https://lore.kernel.org/linux-gpio/20200423174543.17161-1-michael@walle.cc/

I wasn't sure if I should also include the gpiochip_irqchip_add_domain()
patch here. So feel free to skip it. OTOH if you use interrupts with
gpio-regmap it is quite handy.

For an actual user see the patch 11/16 ("gpio: add support for the sl28cpld
GPIO controller") of the series above.

Changes since v3:
 - set reg_dat_base, that was actually broken
 - fix typo
 - fix swapped reg_in_dir/reg_out_dir documentation
 - use "goto err" in error path in gpio_regmap_register()

Changes since v2:
 See changelog in the former patch series.

Michael Walle (2):
  gpiolib: Introduce gpiochip_irqchip_add_domain()
  gpio: add a reusable generic gpio_chip using regmap

 drivers/gpio/Kconfig        |   4 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-regmap.c  | 343 ++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c      |  20 +++
 include/linux/gpio-regmap.h |  69 ++++++++
 include/linux/gpio/driver.h |   3 +
 6 files changed, 440 insertions(+)
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 include/linux/gpio-regmap.h

-- 
2.20.1

[PATCH v4 1/2] gpiolib: Introduce gpiochip_irqchip_add_domain()

[Michael Walle]

The function connects an IRQ domain to a gpiochip and reuses
gpiochip_to_irq() which is provided by gpiolib.

gpiochip_irqchip_* and regmap_irq partially provide the same
functionality. This function will help to connect just the
minimal functionality of the gpiochip_irqchip which is needed to
work together with regmap-irq.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/gpio/gpiolib.c      | 20 ++++++++++++++++++++
 include/linux/gpio/driver.h |  3 +++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index eaa0e209188d..d07f763c9c0b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2756,6 +2756,26 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gc,
 }
 EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_key);
 
+/**
+ * gpiochip_irqchip_add_domain() - adds an irqdomain to a gpiochip
+ * @gc: the gpiochip to add the irqchip to
+ * @domain: the irqdomain to add to the gpiochip
+ *
+ * This function adds an IRQ domain to the gpiochip.
+ */
+int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
+				struct irq_domain *domain)
+{
+	if (!domain)
+		return -EINVAL;
+
+	gc->to_irq = gpiochip_to_irq;
+	gc->irq.domain = domain;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpiochip_irqchip_add_domain);
+
 #else /* CONFIG_GPIOLIB_IRQCHIP */
 
 static inline int gpiochip_add_irqchip(struct gpio_chip *gc,
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 8c41ae41b6bb..ee30065b6f61 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -599,6 +599,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gc,
 bool gpiochip_irqchip_irq_valid(const struct gpio_chip *gc,
 				unsigned int offset);
 
+int gpiochip_irqchip_add_domain(struct gpio_chip *gc,
+				struct irq_domain *domain);
+
 #ifdef CONFIG_LOCKDEP
 
 /*
-- 
2.20.1

[PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

There are quite a lot simple GPIO controller which are using regmap to
access the hardware. This driver tries to be a base to unify existing
code into one place. This won't cover everything but it should be a good
starting point.

It does not implement its own irq_chip because there is already a
generic one for regmap based devices. Instead, the irq_chip will be
instantiated in the parent driver and its irq domain will be associate
to this driver.

For now it consists of the usual registers, like set (and an optional
clear) data register, an input register and direction registers.
Out-of-the-box, it supports consecutive register mappings and mappings
where the registers have gaps between them with a linear mapping between
GPIO offset and bit position. For weirder mappings the user can register
its own .xlate().

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/gpio/Kconfig        |   4 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-regmap.c  | 343 ++++++++++++++++++++++++++++++++++++
 include/linux/gpio-regmap.h |  69 ++++++++
 4 files changed, 417 insertions(+)
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 include/linux/gpio-regmap.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 03c01f4aa316..77b249fe1412 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -73,6 +73,10 @@ config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
 	tristate
 
+config GPIO_REGMAP
+	depends on REGMAP
+	tristate
+
 # put drivers in the right section, in alphabetical order
 
 # This symbol is selected by both I2C and SPI expanders
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 65bf3940e33c..1e4894e0bf0f 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 
 # Device drivers. Generally keep list sorted alphabetically
+obj-$(CONFIG_GPIO_REGMAP)	+= gpio-regmap.o
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
 
 # directly supported by gpio-generic
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
new file mode 100644
index 000000000000..3cb0e8493835
--- /dev/null
+++ b/drivers/gpio/gpio-regmap.c
@@ -0,0 +1,343 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * regmap based generic GPIO driver
+ *
+ * Copyright 2019 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio-regmap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+struct gpio_regmap {
+	struct device *parent;
+	struct regmap *regmap;
+	struct gpio_chip gpio_chip;
+
+	int reg_stride;
+	int ngpio_per_reg;
+	unsigned int reg_dat_base;
+	unsigned int reg_set_base;
+	unsigned int reg_clr_base;
+	unsigned int reg_dir_in_base;
+	unsigned int reg_dir_out_base;
+
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
+			      unsigned int offset, unsigned int *reg,
+			      unsigned int *mask);
+
+	void *driver_data;
+};
+
+static unsigned int gpio_regmap_addr(unsigned int addr)
+{
+	return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;
+}
+
+/**
+ * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
+ *
+ * Use a simple linear mapping to translate the offset to the bitmask.
+ */
+static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
+				    unsigned int base, unsigned int offset,
+				    unsigned int *reg, unsigned int *mask)
+{
+	unsigned int line = offset % gpio->ngpio_per_reg;
+	unsigned int stride = offset / gpio->ngpio_per_reg;
+
+	*reg = base + stride * gpio->reg_stride;
+	*mask = BIT(line);
+
+	return 0;
+}
+
+static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	unsigned int base, val, reg, mask;
+	int ret;
+
+	/* we might not have an output register if we are input only */
+	if (gpio->reg_dat_base)
+		base = gpio_regmap_addr(gpio->reg_dat_base);
+	else
+		base = gpio_regmap_addr(gpio->reg_set_base);
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(gpio->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return (val & mask) ? 1 : 0;
+}
+
+static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
+			    int val)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
+	unsigned int reg, mask;
+
+	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (val)
+		regmap_update_bits(gpio->regmap, reg, mask, mask);
+	else
+		regmap_update_bits(gpio->regmap, reg, mask, 0);
+}
+
+static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
+				       unsigned int offset, int val)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	unsigned int base, reg, mask;
+
+	if (val)
+		base = gpio_regmap_addr(gpio->reg_set_base);
+	else
+		base = gpio_regmap_addr(gpio->reg_clr_base);
+
+	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	regmap_write(gpio->regmap, reg, mask);
+}
+
+static int gpio_regmap_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	unsigned int base, val, reg, mask;
+	int invert, ret;
+
+	if (gpio->reg_dir_out_base) {
+		base = gpio_regmap_addr(gpio->reg_dir_out_base);
+		invert = 0;
+	} else if (gpio->reg_dir_in_base) {
+		base = gpio_regmap_addr(gpio->reg_dir_in_base);
+		invert = 1;
+	} else {
+		return GPIO_LINE_DIRECTION_IN;
+	}
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(gpio->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	if (!!(val & mask) ^ invert)
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int gpio_regmap_set_direction(struct gpio_chip *chip,
+				     unsigned int offset, bool output)
+{
+	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	unsigned int base, val, reg, mask;
+	int invert, ret;
+
+	if (gpio->reg_dir_out_base) {
+		base = gpio_regmap_addr(gpio->reg_dir_out_base);
+		invert = 0;
+	} else if (gpio->reg_dir_in_base) {
+		base = gpio_regmap_addr(gpio->reg_dir_in_base);
+		invert = 1;
+	} else {
+		return 0;
+	}
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	if (!invert)
+		val = (output) ? mask : 0;
+	else
+		val = (output) ? 0 : mask;
+
+	return regmap_update_bits(gpio->regmap, reg, mask, val);
+}
+
+static int gpio_regmap_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	return gpio_regmap_set_direction(chip, offset, false);
+}
+
+static int gpio_regmap_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	gpio_regmap_set(chip, offset, value);
+
+	return gpio_regmap_set_direction(chip, offset, true);
+}
+
+void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data)
+{
+	gpio->driver_data = data;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_set_drvdata);
+
+void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
+{
+	return gpio->driver_data;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
+
+/**
+ * gpio_regmap_register() - Register a generic regmap GPIO controller
+ *
+ * @gpio: gpio_regmap device to register
+ *
+ * Returns 0 on success or an errno on failure.
+ */
+struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
+{
+	struct gpio_regmap *gpio;
+	struct gpio_chip *chip;
+	int ret;
+
+	if (!config->parent)
+		return ERR_PTR(-EINVAL);
+
+	if (!config->ngpio)
+		return ERR_PTR(-EINVAL);
+
+	/* we need at least one */
+	if (!config->reg_dat_base && !config->reg_set_base)
+		return ERR_PTR(-EINVAL);
+
+	/* we don't support having both registers simultaneously for now */
+	if (config->reg_dir_out_base && config->reg_dir_in_base)
+		return ERR_PTR(-EINVAL);
+
+	gpio = kzalloc(sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return ERR_PTR(-ENOMEM);
+
+	gpio->parent = config->parent;
+	gpio->regmap = config->regmap;
+	gpio->ngpio_per_reg = config->ngpio_per_reg;
+	gpio->reg_stride = config->reg_stride;
+	gpio->reg_mask_xlate = config->reg_mask_xlate;
+	gpio->reg_dat_base = config->reg_dat_base;
+	gpio->reg_set_base = config->reg_set_base;
+	gpio->reg_clr_base = config->reg_clr_base;
+	gpio->reg_dir_in_base = config->reg_dir_in_base;
+	gpio->reg_dir_out_base = config->reg_dir_out_base;
+
+	/* if not set, assume there is only one register */
+	if (!gpio->ngpio_per_reg)
+		gpio->ngpio_per_reg = config->ngpio;
+
+	/* if not set, assume they are consecutive */
+	if (!gpio->reg_stride)
+		gpio->reg_stride = 1;
+
+	if (!gpio->reg_mask_xlate)
+		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
+
+	chip = &gpio->gpio_chip;
+	chip->parent = config->parent;
+	chip->base = -1;
+	chip->ngpio = config->ngpio;
+	chip->can_sleep = true;
+
+	if (!chip->label)
+		chip->label = dev_name(config->parent);
+	else
+		chip->label = config->label;
+
+	chip->get = gpio_regmap_get;
+	if (gpio->reg_set_base && gpio->reg_clr_base)
+		chip->set = gpio_regmap_set_with_clear;
+	else if (gpio->reg_set_base)
+		chip->set = gpio_regmap_set;
+
+	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
+		chip->get_direction = gpio_regmap_get_direction;
+		chip->direction_input = gpio_regmap_direction_input;
+		chip->direction_output = gpio_regmap_direction_output;
+	}
+
+	ret = gpiochip_add_data(chip, gpio);
+	if (ret < 0)
+		goto err_free_gpio;
+
+	if (config->irq_domain) {
+		ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);
+		if (ret < 0)
+			goto err_remove_gpiochip;
+	}
+
+	return gpio;
+
+err_remove_gpiochip:
+	gpiochip_remove(chip);
+err_free_gpio:
+	kfree(gpio);
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_register);
+
+/**
+ * gpio_regmap_unregister() - Unregister a generic regmap GPIO controller
+ *
+ * @gpio: gpio_regmap device to unregister
+ */
+void gpio_regmap_unregister(struct gpio_regmap *gpio)
+{
+	gpiochip_remove(&gpio->gpio_chip);
+	kfree(gpio);
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
+
+static void devm_gpio_regmap_unregister(struct device *dev, void *res)
+{
+	gpio_regmap_unregister(*(struct gpio_regmap **)res);
+}
+
+/**
+ * devm_gpio_regmap_register() - resource managed gpio_regmap_register()
+ *
+ * @dev: device that is registering this GPIO device
+ * @gpio: gpio_regmap device to register
+ *
+ * Managed gpio_regmap_register(). For generic regmap GPIO device registered by
+ * this function, gpio_regmap_unregister() is automatically called on driver
+ * detach. See gpio_regmap_register() for more information.
+ */
+struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
+					      const struct gpio_regmap_config *config)
+{
+	struct gpio_regmap **ptr, *gpio;
+
+	ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return ERR_PTR(-ENOMEM);
+
+	gpio = gpio_regmap_register(config);
+
+	if (!IS_ERR(gpio)) {
+		*ptr = gpio;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return gpio;
+}
+EXPORT_SYMBOL_GPL(devm_gpio_regmap_register);
+
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_DESCRIPTION("GPIO generic regmap driver core");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
new file mode 100644
index 000000000000..bbdb2d79ef8f
--- /dev/null
+++ b/include/linux/gpio-regmap.h
@@ -0,0 +1,69 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _LINUX_GPIO_REGMAP_H
+#define _LINUX_GPIO_REGMAP_H
+
+struct gpio_regmap;
+
+#define GPIO_REGMAP_ADDR_ZERO ((unsigned long)(-1))
+#define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
+
+/**
+ * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
+ *
+ * @parent:		The parent device
+ * @regmap:		The regmap used to access the registers
+ *			given, the name of the device is used
+ * @label:		(Optional) Descriptive name for GPIO controller.
+ *			If not given, the name of the device is used.
+ * @ngpio:		Number of GPIOs
+ * @reg_dat_base:	(Optional) (in) register base address
+ * @reg_set_base:	(Optional) set register base address
+ * @reg_clr_base:	(Optional) clear register base address
+ * @reg_dir_in_base:	(Optional) in setting register base address
+ * @reg_dir_out_base:	(Optional) out setting register base address
+ * @reg_stride:		(Optional) May be set if the registers (of the
+ *			same type, dat, set, etc) are not consecutive.
+ * @ngpio_per_reg:	Number of GPIOs per register
+ * @irq_domain:		(Optional) IRQ domain if the controller is
+ *			interrupt-capable
+ * @reg_mask_xlate:     (Optional) Translates base address and GPIO
+ *			offset to a register/bitmask pair. If not
+ *			given the default gpio_regmap_simple_xlate()
+ *			is used.
+ *
+ * The reg_mask_xlate translates a given base address and GPIO offset to
+ * register and mask pair. The base address is one of the given reg_*_base.
+ *
+ * All base addresses may have the special value GPIO_REGMAP_ADDR_ZERO
+ * which forces the address to the value 0.
+ */
+struct gpio_regmap_config {
+	struct device *parent;
+	struct regmap *regmap;
+
+	const char *label;
+	int ngpio;
+
+	unsigned int reg_dat_base;
+	unsigned int reg_set_base;
+	unsigned int reg_clr_base;
+	unsigned int reg_dir_in_base;
+	unsigned int reg_dir_out_base;
+	int reg_stride;
+	int ngpio_per_reg;
+	struct irq_domain *irq_domain;
+
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
+			      unsigned int offset, unsigned int *reg,
+			      unsigned int *mask);
+};
+
+struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config);
+void gpio_regmap_unregister(struct gpio_regmap *gpio);
+struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
+					      const struct gpio_regmap_config *config);
+void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data);
+void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio);
+
+#endif /* _LINUX_GPIO_REGMAP_H */
-- 
2.20.1

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Hi,

Am 2020-05-26 19:29, schrieb Pierre-Louis Bossart:
>> +struct gpio_regmap {
>> +    struct device *parent;
>> +    struct regmap *regmap;
>> +    struct gpio_chip gpio_chip;
>> +
>> +    int reg_stride;
>> +    int ngpio_per_reg;
>> +    unsigned int reg_dat_base;
>> +    unsigned int reg_set_base;
>> +    unsigned int reg_clr_base;
>> +    unsigned int reg_dir_in_base;
>> +    unsigned int reg_dir_out_base;
> 
> I wonder if a base is enough, shouldn't there be a 'last' or something
> that constrains the range of regmap addresses to be used for gpios?

This should be covered on the regmap, shouldn't it?

> related question since I couldn't figure out how to convert my PCM512x
> example, where there are 6 GPIOs configured with 3 regmap-visible
> registers [1], to this mapping.
> 
> GPIO_EN defines if the GPIO is used or not (each bitfield is tied to a
> GPIO)
> 
> GPIO_CONTROL_1 defines the level (each bitfield is tied to a GPIO)
> 
> GPIO_OUTPUT_1+offset defines what signal is routed to each GPIO. I am
> really not sure how this part would be handled?
> 
> That's 8 registers total to deal with GPIOs.

Looks like you need a custom xlate function:

int pcm512x_gpio_regmap_xlate(struct gpio_regmap *gpio, unsigned int 
base,
                               unsigned int offset, unsigned int *reg,
                               unsigned int *mask)
{
   switch (base)
   case GPIO_EN:
   case GPIO_CONTROL_1:
      *reg = base;
      *mask = (1 << offset);
      break;
   case GPIO_OUTPUT_1:
      *reg = base + offset;
      *mask = ...
      break;
}

base is always one of the xx_base properties in the "struct
gpio_regmap_config".

>> +/**
>> + * struct gpio_regmap_config - Description of a generic regmap
>> gpio_chip.
>> + *
>> + * @parent:        The parent device
>> + * @regmap:        The regmap used to access the registers
>> + *            given, the name of the device is used
>> + * @label:        (Optional) Descriptive name for GPIO controller.
>> + *            If not given, the name of the device is used.
>> + * @ngpio:        Number of GPIOs
>> + * @reg_dat_base:    (Optional) (in) register base address
>> + * @reg_set_base:    (Optional) set register base address
>> + * @reg_clr_base:    (Optional) clear register base address
>> + * @reg_dir_in_base:    (Optional) in setting register base address
>> + * @reg_dir_out_base:    (Optional) out setting register base
>> address
>> + * @reg_stride:        (Optional) May be set if the registers (of
>> the
>> + *            same type, dat, set, etc) are not consecutive.
>> + * @ngpio_per_reg:    Number of GPIOs per register
>> + * @irq_domain:        (Optional) IRQ domain if the controller is
>> + *            interrupt-capable
>> + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
>> + *            offset to a register/bitmask pair. If not
>> + *            given the default gpio_regmap_simple_xlate()
>> + *            is used.
>> + *
>> + * The reg_mask_xlate translates a given base address and GPIO
>> offset to
>> + * register and mask pair. The base address is one of the given
>> reg_*_base.
>> + *
>> + * All base addresses may have the special value
>> GPIO_REGMAP_ADDR_ZERO
>> + * which forces the address to the value 0.
>> + */
>> +struct gpio_regmap_config {
>> +    struct device *parent;
>> +    struct regmap *regmap;
>> +
>> +    const char *label;
>> +    int ngpio;
> 
> could we add a .names field for the gpio_chip, I found this useful for
> PCM512x GPIO support, e.g.

Sure, I have the names in the device tree.

But I'd prefer that you'd do a patch on top of this (assuming it is
applied soon), because you can actually test it and there might be
missing more.

[snip]

-michael

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Pierre-Louis Bossart]

Hi Michael,

>>> +struct gpio_regmap_config {
>>> +    struct device *parent;
>>> +    struct regmap *regmap;
>>> +
>>> +    const char *label;
>>> +    int ngpio;
>>
>> could we add a .names field for the gpio_chip, I found this useful for
>> PCM512x GPIO support, e.g.
> 
> Sure, I have the names in the device tree.
> 
> But I'd prefer that you'd do a patch on top of this (assuming it is
> applied soon), because you can actually test it and there might be
> missing more.

I am happy to report that this gpio-regmap worked like a charm for me, 
after I applied the minor diff below (complete code at 
https://github.com/plbossart/sound/tree/fix/regmap-gpios).

I worked around my previous comments by forcing the GPIO internal 
routing directly in regmap, and that allowed me to only play with the 
_set and _dir bases. I see the LEDs and clock selected as before, quite 
nice indeed.

The chip->label test is probably wrong, since the gpio_chip structure is 
zeroed out if(!chip->label) is always true so the label is always set to 
the device name. I don't know what the intent was so just removed that 
test - maybe the correct test should be if (!config->label) ?

I added the names support as well, and btw I don't understand how one 
would get them through device tree?

I still have a series of odd warnings I didn't have before:

[  101.400263] WARNING: CPU: 3 PID: 1129 at drivers/gpio/gpiolib.c:4084 
gpiod_set_value+0x3f/0x50

This seems to come from
	/* Should be using gpiod_set_value_cansleep() */
	WARN_ON(desc->gdev->chip->can_sleep);

so maybe we need an option here as well? Or use a different function?

Anyways, that gpio-regmap does simplify my code a great deal so thanks 
for this work, much appreciated.
-Pierre

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 3cb0e8493835..678d644a0a4b 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -251,10 +251,8 @@ struct gpio_regmap *gpio_regmap_register(const 
struct gpio_regmap_config *config
         chip->ngpio = config->ngpio;
         chip->can_sleep = true;

-       if (!chip->label)
-               chip->label = dev_name(config->parent);
-       else
-               chip->label = config->label;
+       chip->label = config->label;
+       chip->names = config->names;

         chip->get = gpio_regmap_get;
         if (gpio->reg_set_base && gpio->reg_clr_base)
diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
index bbdb2d79ef8f..c1f3e36ebf33 100644
--- a/include/linux/gpio-regmap.h
+++ b/include/linux/gpio-regmap.h
@@ -16,6 +16,7 @@ struct gpio_regmap;
   *                     given, the name of the device is used
   * @label:             (Optional) Descriptive name for GPIO controller.
   *                     If not given, the name of the device is used.
+ * @names:             (Optional) Array of names for gpios
   * @ngpio:             Number of GPIOs
   * @reg_dat_base:      (Optional) (in) register base address
   * @reg_set_base:      (Optional) set register base address
@@ -43,6 +44,7 @@ struct gpio_regmap_config {
         struct regmap *regmap;

         const char *label;
+       const char *const *names;
         int ngpio;

         unsigned int reg_dat_base;

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Hi,

Am 2020-05-28 02:31, schrieb Pierre-Louis Bossart:
> Hi Michael,
> 
>>>> +struct gpio_regmap_config {
>>>> +    struct device *parent;
>>>> +    struct regmap *regmap;
>>>> +
>>>> +    const char *label;
>>>> +    int ngpio;
>>> 
>>> could we add a .names field for the gpio_chip, I found this useful 
>>> for
>>> PCM512x GPIO support, e.g.
>> 
>> Sure, I have the names in the device tree.
>> 
>> But I'd prefer that you'd do a patch on top of this (assuming it is
>> applied soon), because you can actually test it and there might be
>> missing more.
> 
> I am happy to report that this gpio-regmap worked like a charm for me,
> after I applied the minor diff below (complete code at
> https://github.com/plbossart/sound/tree/fix/regmap-gpios).
> 
> I worked around my previous comments by forcing the GPIO internal
> routing directly in regmap, and that allowed me to only play with the
> _set and _dir bases. I see the LEDs and clock selected as before,
> quite nice indeed.
> 
> The chip->label test is probably wrong, since the gpio_chip structure
> is zeroed out if(!chip->label) is always true so the label is always
> set to the device name. I don't know what the intent was so just
> removed that test - maybe the correct test should be if
> (!config->label) ?

yes, that was a typo. should have been if (!config->label).

I've send a v5 with that fix and your names property.

> I added the names support as well, and btw I don't understand how one
> would get them through device tree?

gpio-line-names property, see
Documentation/devicetree/bindings/gpio/gpio.txt.

> I still have a series of odd warnings I didn't have before:
> 
> [  101.400263] WARNING: CPU: 3 PID: 1129 at
> drivers/gpio/gpiolib.c:4084 gpiod_set_value+0x3f/0x50
> 
> This seems to come from
> 	/* Should be using gpiod_set_value_cansleep() */
> 	WARN_ON(desc->gdev->chip->can_sleep);

Right now, gpio-regmap hardcodes can_sleep to true. But the only regmap
which don't sleep is regmap-mmio. The PCM512x seems to be either I2C or
SPI, which can both sleep. So this warning is actually correct and
wherever this gpio is set should do it by calling the _cansleep()
version.

> so maybe we need an option here as well? Or use a different function?
> 
> Anyways, that gpio-regmap does simplify my code a great deal so thanks
> for this work, much appreciated.

Glad to see that there are more users for it ;)

-michael

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Andy Shevchenko]

On Thu, May 28, 2020 at 7:11 AM Michael Walle <michael@walle.cc> wrote:
> Am 2020-05-28 02:31, schrieb Pierre-Louis Bossart:


> I've send a v5 with that fix and your names property.

Did you already? Because I have a question why we have gpio-regmap.h
instead of gpio/regmap.h ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Am 2020-05-28 10:37, schrieb Andy Shevchenko:
> On Thu, May 28, 2020 at 7:11 AM Michael Walle <michael@walle.cc> wrote:
>> Am 2020-05-28 02:31, schrieb Pierre-Louis Bossart:
> 
> 
>> I've send a v5 with that fix and your names property.
> 
> Did you already?

Yes, I forgot to add you :(

> Because I have a question why we have gpio-regmap.h instead of
> gpio/regmap.h ?

No particular reason, I thought gpio/ was for low level gpio stuff (like
the consumer.h, driver.h etc) or lets say - more integrated stuff.

I don't have a strong opinion. I can send a v6 with that change, on
short notice if the maintainers prefer that location.

-michael

Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap

[Andy Shevchenko]

On Thu, May 28, 2020 at 10:46:56AM +0200, Michael Walle wrote:
> Am 2020-05-28 10:37, schrieb Andy Shevchenko:
> > On Thu, May 28, 2020 at 7:11 AM Michael Walle <michael@walle.cc> wrote:
> > > Am 2020-05-28 02:31, schrieb Pierre-Louis Bossart:
> > 
> > 
> > > I've send a v5 with that fix and your names property.
> > 
> > Did you already?
> 
> Yes, I forgot to add you :(
> 
> > Because I have a question why we have gpio-regmap.h instead of
> > gpio/regmap.h ?
> 
> No particular reason, I thought gpio/ was for low level gpio stuff (like
> the consumer.h, driver.h etc) or lets say - more integrated stuff.
> 
> I don't have a strong opinion. I can send a v6 with that change, on
> short notice if the maintainers prefer that location.

I guess it's quite close to core and belongs to GPIO family of headers, I
definitely would like to see it there, but let's wait for Linus and Bart to
speak up.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 0/2] gpio: generic regmap implementation

[Bartosz Golaszewski]

pon., 25 maj 2020 o 18:07 Michael Walle <michael@walle.cc> napisał(a):
>
> This series is a split off of the sl28cpld series:
> https://lore.kernel.org/linux-gpio/20200423174543.17161-1-michael@walle.cc/
>
> I wasn't sure if I should also include the gpiochip_irqchip_add_domain()
> patch here. So feel free to skip it. OTOH if you use interrupts with
> gpio-regmap it is quite handy.
>
> For an actual user see the patch 11/16 ("gpio: add support for the sl28cpld
> GPIO controller") of the series above.
>
> Changes since v3:
>  - set reg_dat_base, that was actually broken
>  - fix typo
>  - fix swapped reg_in_dir/reg_out_dir documentation
>  - use "goto err" in error path in gpio_regmap_register()
>
> Changes since v2:
>  See changelog in the former patch series.
>
> Michael Walle (2):
>   gpiolib: Introduce gpiochip_irqchip_add_domain()
>   gpio: add a reusable generic gpio_chip using regmap
>
>  drivers/gpio/Kconfig        |   4 +
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-regmap.c  | 343 ++++++++++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c      |  20 +++
>  include/linux/gpio-regmap.h |  69 ++++++++
>  include/linux/gpio/driver.h |   3 +
>  6 files changed, 440 insertions(+)
>  create mode 100644 drivers/gpio/gpio-regmap.c
>  create mode 100644 include/linux/gpio-regmap.h
>
> --
> 2.20.1
>

Both patches look good to me. I'd say we should apply them now and
then fix any potential problems during the v5.8 release cycle. It's
not like it's a user-space facing, stable interface, we don't have to
be super careful when merging this kind of stuff.

Bart