Re: [PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap
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 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 2/2] gpio: add a reusable generic gpio_chip using regmap
Am 2020-05-28 10:37, schrieb Andy Shevchenko: On Thu, May 28, 2020 at 7:11 AM Michael Walle 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
On Thu, May 28, 2020 at 7:11 AM Michael Walle 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
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
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
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
[PATCH v4 2/2] gpio: add a reusable generic gpio_chip using regmap
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
---
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 ..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
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+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, , );
+ if (ret)
+ return ret;
+
+ ret = regmap_read(gpio->regmap, reg, );
+ 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, , );
+ if (val)
+ regmap_update_bits(gpio->regmap, reg, mask, mask);
+ else
+ regmap_update_bits(gpio->regmap, reg, mask, 0);
+}
+
+static void
