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

[PATCH v5 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 v4:
 - add comment about can_sleep
 - fix config->label typo
 - add config->names property

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  | 352 ++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c      |  20 ++
 include/linux/gpio-regmap.h |  70 +++++++
 include/linux/gpio/driver.h |   3 +
 6 files changed, 450 insertions(+)
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 include/linux/gpio-regmap.h

-- 
2.20.1

[PATCH v5 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 v5 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  | 352 ++++++++++++++++++++++++++++++++++++
 include/linux/gpio-regmap.h |  70 +++++++
 4 files changed, 427 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 7d077be10a0f..bcacd9c74aa8 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..5060ca865276
--- /dev/null
+++ b/drivers/gpio/gpio-regmap.c
@@ -0,0 +1,352 @@
+// 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->names = config->names;
+
+	/*
+	 * If our regmap is fast_io we should probably set can_sleep to false.
+	 * Right now, the regmap doesn't save this property, nor is there any
+	 * access function for it.
+	 * The only regmap type which uses fast_io is regmap-mmio. For now,
+	 * assume a safe default of true here.
+	 */
+	chip->can_sleep = true;
+
+	if (!config->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..d28e63d0ece4
--- /dev/null
+++ b/include/linux/gpio-regmap.h
@@ -0,0 +1,70 @@
+/* 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
+ * @names:		(Optional) Array of names for 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 v5 2/2] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Am 2020-05-28 05:58, schrieb 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  | 352 ++++++++++++++++++++++++++++++++++++
>  include/linux/gpio-regmap.h |  70 +++++++
>  4 files changed, 427 insertions(+)
>  create mode 100644 drivers/gpio/gpio-regmap.c
>  create mode 100644 include/linux/gpio-regmap.h
> 
[..]

> --- /dev/null
> +++ b/include/linux/gpio-regmap.h
> @@ -0,0 +1,70 @@
> +/* 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
> + * @names:		(Optional) Array of names for 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;

damn.. I shouldn't send patches early in the morning. I've forgot to 
actually
enable my GPIO driver before compiling. And of course.. it doesn't 
compile.

const char *const *names;

is missing here. So I have to send a v6 anyway. Let's wait what Linus 
and Bert
says about the location of the gpio-regmap.h header.

Sorry for the noise.

-michael

> +
> +	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 */

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

[Andy Shevchenko]

On Thu, May 28, 2020 at 7:04 AM Michael Walle <michael@walle.cc> wrote:
>
> 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().

More comments from me below.

...


>  # Device drivers. Generally keep list sorted alphabetically

Hmm...

> +obj-$(CONFIG_GPIO_REGMAP)      += gpio-regmap.o
>  obj-$(CONFIG_GPIO_GENERIC)     += gpio-generic.o

...is it?

...

> + * Copyright 2019 Michael Walle <michael@walle.cc>

2020?

...

> +#include <linux/gpio/driver.h>
> +#include <linux/gpio-regmap.h>

Yes, I would like to see this as gpio/regmap.h (in gpio folder).

...

> +static unsigned int gpio_regmap_addr(unsigned int addr)
> +{
> +       return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;

I would prefer rather to have
if (...)
 return 0;

return addr;

here, but any of them fine.

> +}

...

> +/**
> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask

Don't you get plenty of complains from kernel doc validation script?

You forgot to describe all function parameters here.

> + *
> + * Use a simple linear mapping to translate the offset to the bitmask.
> + */

...

> +       return (val & mask) ? 1 : 0;

Hmm... many (new!) GPIO drivers are using !! instead of ternary. Can
we do the same here?

...

> +static int gpio_regmap_get_direction(struct gpio_chip *chip,
> +                                    unsigned int offset)
> +{

> +       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;

Hmm... Doesn't it an erroneous case and we basically shouldn't be here?

> +       }

> +       if (!!(val & mask) ^ invert)
> +               return GPIO_LINE_DIRECTION_OUT;

> +       else

Redundant 'else'.

> +               return GPIO_LINE_DIRECTION_IN;
> +}

> +static int gpio_regmap_set_direction(struct gpio_chip *chip,
> +                                    unsigned int offset, bool output)
> +{

> +       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;

Question as above.

> +       }

> +       if (!invert)
> +               val = (output) ? mask : 0;
> +       else
> +               val = (output) ? 0 : mask;

Why not positive conditional?
Also, too many parentheses.

> +       return regmap_update_bits(gpio->regmap, reg, mask, val);
> +}

...

> +/**
> + * gpio_regmap_register() - Register a generic regmap GPIO controller

> + *

Extra blank line.

> + * @gpio: gpio_regmap device to register
> + *
> + * Returns 0 on success or an errno on failure.
> + */

...

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

Why not positive or here even ternary may look good

        chip->label = config->label ?: dev_name(config->parent);

...

> +               ret = gpiochip_irqchip_add_domain(chip, config->irq_domain);

> +               if (ret < 0)

Does ' < 0' make sense?

> +                       goto err_remove_gpiochip;

...

> +/**
> + * gpio_regmap_unregister() - Unregister a generic regmap GPIO controller

> + *

Extra blank line

> + * @gpio: gpio_regmap device to unregister
> + */

...

> +/**
> + * devm_gpio_regmap_register() - resource managed gpio_regmap_register()

> + *

Ditto.

> + * @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.
> + */

...

> +       gpio = gpio_regmap_register(config);

> +

Extra blank line.

> +       if (!IS_ERR(gpio)) {
> +               *ptr = gpio;
> +               devres_add(dev, ptr);
> +       } else {
> +               devres_free(ptr);
> +       }

...

> +#ifndef _LINUX_GPIO_REGMAP_H
> +#define _LINUX_GPIO_REGMAP_H
> +

Missed a lot, i.e.

struct device;
struct irq_domain;
struct regmap;

> +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.

> + *

Extra blank line.

> + * @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

> + * @names:             (Optional) Array of names for gpios

I don't see it. You really need to enable kernel doc validation warnings.

> + * @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

'reg_mask_xlate' ->  '->reg_mask_xlate()' or '@reg_mask_xlate' or
special C function reference for kernel doc.

> + * register and mask pair. The base address is one of the given reg_*_base.

'reg_*_base' -> 'register base addresses in this structure' ?

> + *
> + * All base addresses may have the special value GPIO_REGMAP_ADDR_ZERO
> + * which forces the address to the value 0.

Also, since we have no separate documentation, describe the rules,
that some of the registers are actually required, and some maybe in
conflict (these rules you have in register function).

> + */

-- 
With Best Regards,
Andy Shevchenko

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

[Michael Walle]

Am 2020-05-28 13:45, schrieb Andy Shevchenko:
> On Thu, May 28, 2020 at 7:04 AM Michael Walle <michael@walle.cc> wrote:
>> 
>> 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().
> 
> More comments from me below.

Thanks for the review.

> 
> ...
> 
> 
>>  # Device drivers. Generally keep list sorted alphabetically
> 
> Hmm...
> 
>> +obj-$(CONFIG_GPIO_REGMAP)      += gpio-regmap.o
>>  obj-$(CONFIG_GPIO_GENERIC)     += gpio-generic.o
> 
> ...is it?

That's because gpio-regmap.o seems not be a driver and more of a generic
thing (like gpio-generic.o) and gpio-generic.o has another rule two 
lines
below and I don't want to put gpio-regmap.o in between.

> 
> ...
> 
>> + * Copyright 2019 Michael Walle <michael@walle.cc>
> 
> 2020?
> 
> ...
> 
>> +#include <linux/gpio/driver.h>
>> +#include <linux/gpio-regmap.h>
> 
> Yes, I would like to see this as gpio/regmap.h (in gpio folder).
> 
> ...
> 
>> +static unsigned int gpio_regmap_addr(unsigned int addr)
>> +{
>> +       return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;
> 
> I would prefer rather to have
> if (...)
>  return 0;
> 
> return addr;
> 
> here, but any of them fine.

yes looks nicer.

> 
>> +}
> 
> ...
> 
>> +/**
>> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
> 
> Don't you get plenty of complains from kernel doc validation script?

now that I know there is one, yes. there are many complains.

> 
> You forgot to describe all function parameters here.
> 
>> + *
>> + * Use a simple linear mapping to translate the offset to the 
>> bitmask.
>> + */

This is a leftover, I'm actually gonna remove it since its not exported
anymore.

> 
> ...
> 
>> +       return (val & mask) ? 1 : 0;
> 
> Hmm... many (new!) GPIO drivers are using !! instead of ternary. Can
> we do the same here?

ok

> 
> ...
> 
>> +static int gpio_regmap_get_direction(struct gpio_chip *chip,
>> +                                    unsigned int offset)
>> +{
> 
>> +       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;
> 
> Hmm... Doesn't it an erroneous case and we basically shouldn't be here?

yeah, I'll return -EOPNOTSUPP. Better than just ignoring, right?

> 
>> +       }
> 
>> +       if (!!(val & mask) ^ invert)
>> +               return GPIO_LINE_DIRECTION_OUT;
> 
>> +       else
> 
> Redundant 'else'.

IMHO, That looks really strange. like it has nothing to do with the
if statement. I'd like to keep that one.

> 
>> +               return GPIO_LINE_DIRECTION_IN;
>> +}
> 
>> +static int gpio_regmap_set_direction(struct gpio_chip *chip,
>> +                                    unsigned int offset, bool output)
>> +{
> 
>> +       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;
> 
> Question as above.

same answer ;)

> 
>> +       }
> 
>> +       if (!invert)
>> +               val = (output) ? mask : 0;
>> +       else
>> +               val = (output) ? 0 : mask;
> 
> Why not positive conditional?
> Also, too many parentheses.

ok

> 
>> +       return regmap_update_bits(gpio->regmap, reg, mask, val);
>> +}
> 
> ...
> 
>> +/**
>> + * gpio_regmap_register() - Register a generic regmap GPIO controller
> 
>> + *
> 
> Extra blank line.

didn't know that. so ok for all kind of these comments.

> 
>> + * @gpio: gpio_regmap device to register
>> + *
>> + * Returns 0 on success or an errno on failure.
>> + */
> 
> ...
> 
>> +       if (!config->label)
>> +               chip->label = dev_name(config->parent);
>> +       else
>> +               chip->label = config->label;
> 
> Why not positive or here even ternary may look good
> 
>         chip->label = config->label ?: dev_name(config->parent);

ok

> 
> ...
> 
>> +               ret = gpiochip_irqchip_add_domain(chip, 
>> config->irq_domain);
> 
>> +               if (ret < 0)
> 
> Does ' < 0' make sense?

more or less, I'll change it to "if (ret)"

> 
>> +                       goto err_remove_gpiochip;
> 
> ...
> 
>> +/**
>> + * gpio_regmap_unregister() - Unregister a generic regmap GPIO 
>> controller
> 
>> + *
> 
> Extra blank line
> 
>> + * @gpio: gpio_regmap device to unregister
>> + */
> 
> ...
> 
>> +/**
>> + * devm_gpio_regmap_register() - resource managed 
>> gpio_regmap_register()
> 
>> + *
> 
> Ditto.
> 
>> + * @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.
>> + */
> 
> ...
> 
>> +       gpio = gpio_regmap_register(config);
> 
>> +
> 
> Extra blank line.

ok

>> +       if (!IS_ERR(gpio)) {
>> +               *ptr = gpio;
>> +               devres_add(dev, ptr);
>> +       } else {
>> +               devres_free(ptr);
>> +       }
> 
> ...
> 
>> +#ifndef _LINUX_GPIO_REGMAP_H
>> +#define _LINUX_GPIO_REGMAP_H
>> +
> 
> Missed a lot, i.e.
> 
> struct device;
> struct irq_domain;
> struct regmap;

oops. right.

> 
>> +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.
> 
>> + *
> 
> Extra blank line.
> 
>> + * @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
> 
>> + * @names:             (Optional) Array of names for gpios
> 
> I don't see it. You really need to enable kernel doc validation 
> warnings.

I've already noticed that in another mail.

> 
>> + * @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
> 
> 'reg_mask_xlate' ->  '->reg_mask_xlate()' or '@reg_mask_xlate' or
> special C function reference for kernel doc.
> 
>> + * register and mask pair. The base address is one of the given 
>> reg_*_base.
> 
> 'reg_*_base' -> 'register base addresses in this structure' ?
> 
>> + *
>> + * All base addresses may have the special value 
>> GPIO_REGMAP_ADDR_ZERO
>> + * which forces the address to the value 0.
> 
> Also, since we have no separate documentation, describe the rules,
> that some of the registers are actually required, and some maybe in
> conflict (these rules you have in register function).

ok

-michael

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

[Andy Shevchenko]

On Thu, May 28, 2020 at 4:00 PM Michael Walle <michael@walle.cc> wrote:
> Am 2020-05-28 13:45, schrieb Andy Shevchenko:
> > On Thu, May 28, 2020 at 7:04 AM Michael Walle <michael@walle.cc> wrote:

> > More comments from me below.
>
> Thanks for the review.

You are welcome! Thanks for doing this actually.

(So, the not commented points I think you agreed with)

...

> >>  # Device drivers. Generally keep list sorted alphabetically
> >
> > Hmm...
> >
> >> +obj-$(CONFIG_GPIO_REGMAP)      += gpio-regmap.o
> >>  obj-$(CONFIG_GPIO_GENERIC)     += gpio-generic.o
> >
> > ...is it?
>
> That's because gpio-regmap.o seems not be a driver and more of a generic
> thing (like gpio-generic.o) and gpio-generic.o has another rule two
> lines
> below and I don't want to put gpio-regmap.o in between.

OK!

...

> >> +       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;
> >
> > Hmm... Doesn't it an erroneous case and we basically shouldn't be here?
>
> yeah, I'll return -EOPNOTSUPP. Better than just ignoring, right?

Yes, that's what I meant.

...

> >> +       if (!!(val & mask) ^ invert)
> >> +               return GPIO_LINE_DIRECTION_OUT;
> >
> >> +       else
> >
> > Redundant 'else'.
>
> IMHO, That looks really strange. like it has nothing to do with the
> if statement. I'd like to keep that one.

We have many drivers done like that, but it's minor, so, up to you and
maintainers.

-- 
With Best Regards,
Andy Shevchenko