[PATCH v6 0/3] gpio: generic regmap implementation

[PATCH v6 0/3] 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 v5:
 - add names property
 - addressed Alex' remarks:
   - moved header from i/l/gpio-regmap.h to i/l/gpio/regmap.h
   - fixed kernel doc
   - described rules how to use the register base offsets
   - missing forward declarations
   - style changes
   - return -EOPNOTSUPP, in code path we should never reach
 - added check to have input and output base regs if there is a direction
   register
 - added MAINTAINERS patch

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()

Michael Walle (3):
  gpiolib: Introduce gpiochip_irqchip_add_domain()
  gpio: add a reusable generic gpio_chip using regmap
  MAINTAINERS: Add gpio regmap section

 MAINTAINERS                 |   6 +
 drivers/gpio/Kconfig        |   4 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-regmap.c  | 349 ++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpiolib.c      |  20 +++
 include/linux/gpio/driver.h |   3 +
 include/linux/gpio/regmap.h |  86 +++++++++
 7 files changed, 469 insertions(+)
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 include/linux/gpio/regmap.h

-- 
2.20.1

[PATCH v6 1/3] 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 v6 2/3] 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  | 349 ++++++++++++++++++++++++++++++++++++
 include/linux/gpio/regmap.h |  86 +++++++++
 4 files changed, 440 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..5412cb3b0b2a
--- /dev/null
+++ b/drivers/gpio/gpio-regmap.c
@@ -0,0 +1,349 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * regmap based generic GPIO driver
+ *
+ * Copyright 2020 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)
+{
+	if (addr == GPIO_REGMAP_ADDR_ZERO)
+		return 0;
+
+	return addr;
+}
+
+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);
+}
+
+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 -EOPNOTSUPP;
+	}
+
+	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 -EOPNOTSUPP;
+	}
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	if (invert)
+		val = output ? 0 : mask;
+	else
+		val = output ? mask : 0;
+
+	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
+ * @config: configuration for gpio_regmap
+ *
+ * Return: A pointer to the registered gpio_regmap or ERR_PTR error value.
+ */
+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);
+
+	/* if we have a direction register we need both input and output */
+	if ((config->reg_dir_out_base || config->reg_dir_in_base) &&
+	    (!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;
+	chip->label = config->label ?: dev_name(config->parent);
+
+	/*
+	 * 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;
+
+	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)
+			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
+ * @config: configuration for gpio_regmap
+ *
+ * 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.
+ *
+ * Return: A pointer to the registered gpio_regmap or ERR_PTR error value.
+ */
+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..4c1e6b34e824
--- /dev/null
+++ b/include/linux/gpio/regmap.h
@@ -0,0 +1,86 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _LINUX_GPIO_REGMAP_H
+#define _LINUX_GPIO_REGMAP_H
+
+struct device;
+struct gpio_regmap;
+struct irq_domain;
+struct 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 register
+ * base addresses in this structure.
+ *
+ * Although all register base addresses are marked as optional, there are
+ * several rules:
+ *     1. if you only have @reg_dat_base set, then it is input-only
+ *     2. if you only have @reg_set_base set, then it is output-only
+ *     3. if you have either @reg_dir_in_base or @reg_dir_out_base set, then
+ *        you have to set both @reg_dat_base and @reg_set_base
+ *     4. if you have @reg_set_base set, you may also set @reg_clr_base to have
+ *        two different registers for setting and clearing the output. This is
+ *        also valid for the output-only case.
+ *     5. @reg_dir_in_base and @reg_dir_out_base are exclusive; is there really
+ *        hardware which has redundant registers?
+ *
+ * Note: 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;
+	const char *const *names;
+
+	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 v6 2/3] gpio: add a reusable generic gpio_chip using regmap

[Michael Walle]

Am 2020-05-28 17:55, schrieb Pierre-Louis Bossart:
>> +    /* if we have a direction register we need both input and
>> output */
>> +    if ((config->reg_dir_out_base || config->reg_dir_in_base) &&
>> +        (!config->reg_dat_base || !config->reg_set_base))
>> +        return ERR_PTR(-EINVAL);
>  This failed for me since I didn't have the 'dat' base assigned. I
> still can't figure out what 'dat' stands for...I just assigned it to
> the same offset as the 'set' base but really don't understand what
> this is supposed to do.

DAT is the data register, aka input register, if the GPIO is in input
mode.

If I read the datasheet correctly you should use the following:

PCM512x_GPIO_EN should be reg_dir_out_base
PCM512x_GPIO_CONTROL_1 should be reg_set_base
PCM512x_GPIN should be reg_dat_base

no custom xlate necessary. GPIN looks a bit fishy in that datasheet:
  http://www.ti.com/lit/ds/symlink/pcm5121.pdf?ts=1590684141147

PCM512x_GPIO_OUTPUT_1..6 is pinmux control and shouldn't be part of
the gpio-regmap. Your driver needs to take care of that.

>> +
>> +    /* 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);
> 
> and this second test seems to contradict the notion of 'both input and
> output' above?

dir_out_base is used if the register is high active to select an output.
dir_in_base is used for a low active register. Thus both bases are used
to switch a GPIO between input and output.

> re-adding comment from previous series:
>  >> 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.
> 
> I still have the warnings with this version, not sure if you wanted to
> fix it in the v6 or is this needs to be fixed in another piece of
> code/patch. How would we go about removing this warning?

There is no fix in gpio-regmap. wherever this GPIO is connected to must
not call gpiod_set_value() but have to use gpiod_set_value_cansleep().

-michael

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

[Pierre-Louis Bossart]

>>> +    /* if we have a direction register we need both input and
>>> output */
>>> +    if ((config->reg_dir_out_base || config->reg_dir_in_base) &&
>>> +        (!config->reg_dat_base || !config->reg_set_base))
>>> +        return ERR_PTR(-EINVAL);
>>  This failed for me since I didn't have the 'dat' base assigned. I
>> still can't figure out what 'dat' stands for...I just assigned it to
>> the same offset as the 'set' base but really don't understand what
>> this is supposed to do.
> 
> DAT is the data register, aka input register, if the GPIO is in input
> mode.
> 
> If I read the datasheet correctly you should use the following:
> 
> PCM512x_GPIO_EN should be reg_dir_out_base
> PCM512x_GPIO_CONTROL_1 should be reg_set_base
> PCM512x_GPIN should be reg_dat_base
> 
> no custom xlate necessary. GPIN looks a bit fishy in that datasheet:
>   http://www.ti.com/lit/ds/symlink/pcm5121.pdf?ts=1590684141147

ok, I updated that part - can't test it though with the boards I have, 
only output is supported.

> PCM512x_GPIO_OUTPUT_1..6 is pinmux control and shouldn't be part of
> the gpio-regmap. Your driver needs to take care of that.

yes, that's what I did.

>>> +
>>> +    /* 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);
>>
>> and this second test seems to contradict the notion of 'both input and
>> output' above?
> 
> dir_out_base is used if the register is high active to select an output.
> dir_in_base is used for a low active register. Thus both bases are used
> to switch a GPIO between input and output.

ok, makes sense now, thanks for the explanations.

>> re-adding comment from previous series:
>>  >> 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.
>>
>> I still have the warnings with this version, not sure if you wanted to
>> fix it in the v6 or is this needs to be fixed in another piece of
>> code/patch. How would we go about removing this warning?
> 
> There is no fix in gpio-regmap. wherever this GPIO is connected to must
> not call gpiod_set_value() but have to use gpiod_set_value_cansleep().

yes, I figured this out and corrected the drivers, works just fine now. 
My cleaned-up code is at 
https://github.com/thesofproject/linux/pull/1945, I'll resubmit the v2 
based on these patches when they are merged and available in Mark 
Brown's tree (and I need to address the clock-related feedback).

Thanks for your work Michael, really nice and useful, feel free to add 
the following tag:

Tested-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

[PATCH v6 3/3] MAINTAINERS: Add gpio regmap section

[Michael Walle]

Add myself as a reviewer for the gpio regmap.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3a003f310574..f3458debb0c9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7340,6 +7340,12 @@ S:	Maintained
 F:	drivers/gpio/gpio-mockup.c
 F:	tools/testing/selftests/gpio/
 
+GPIO REGMAP
+R:	Michael Walle <michael@walle.cc>
+S:	Maintained
+F:	drivers/gpio/gpio-regmap.c
+F:	include/linux/gpio/regmap.h
+
 GPIO SUBSYSTEM
 M:	Linus Walleij <linus.walleij@linaro.org>
 M:	Bartosz Golaszewski <bgolaszewski@baylibre.com>
-- 
2.20.1

Re: [PATCH v6 0/3] gpio: generic regmap implementation

[Andy Shevchenko]

On Thu, May 28, 2020 at 5:59 PM Michael Walle <michael@walle.cc> wrote:
>
> 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.
>

v6 looks pretty much good to me, thanks!

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Changes since v5:
>  - add names property
>  - addressed Alex' remarks:
>    - moved header from i/l/gpio-regmap.h to i/l/gpio/regmap.h
>    - fixed kernel doc
>    - described rules how to use the register base offsets
>    - missing forward declarations
>    - style changes
>    - return -EOPNOTSUPP, in code path we should never reach
>  - added check to have input and output base regs if there is a direction
>    register
>  - added MAINTAINERS patch
>
> 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()
>
> Michael Walle (3):
>   gpiolib: Introduce gpiochip_irqchip_add_domain()
>   gpio: add a reusable generic gpio_chip using regmap
>   MAINTAINERS: Add gpio regmap section
>
>  MAINTAINERS                 |   6 +
>  drivers/gpio/Kconfig        |   4 +
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-regmap.c  | 349 ++++++++++++++++++++++++++++++++++++
>  drivers/gpio/gpiolib.c      |  20 +++
>  include/linux/gpio/driver.h |   3 +
>  include/linux/gpio/regmap.h |  86 +++++++++
>  7 files changed, 469 insertions(+)
>  create mode 100644 drivers/gpio/gpio-regmap.c
>  create mode 100644 include/linux/gpio/regmap.h
>
> --
> 2.20.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v6 0/3] gpio: generic regmap implementation

[Linus Walleij]

On Thu, May 28, 2020 at 4:59 PM Michael Walle <michael@walle.cc> wrote:

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

Patches applied as some of the last stuff before sending my pull
requests :)

Let's work with this as a base, and thanks a *LOT* for your work
so far!

Yours,
Linus Walleij