| Message ID | 20200402203656.27047-11-michael@walle.cc |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for Kontron sl28cpld | expand |
czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a): > > 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 > instanciated 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> Hi Michael, Thanks for doing this! When looking at other generic drivers: gpio-mmio and gpio-reg I can see there are some corner-cases and more specific configuration options we could add but it's not a blocker, we'll probably be extending this one as we convert more drivers to using it. Personally I'd love to see gpio-mmio and gpio-reg removed and replaced by a single, generic regmap interface eventually. > --- > drivers/gpio/Kconfig | 4 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-regmap.c | 320 ++++++++++++++++++++++++++++++++++++ > include/linux/gpio-regmap.h | 88 ++++++++++ > 4 files changed, 413 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 1b96169d84f7..a8e148f4b2e0 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 b2cfc21a97f3..93e139fdfa57 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..cc4437dc0521 > --- /dev/null > +++ b/drivers/gpio/gpio-regmap.c > @@ -0,0 +1,320 @@ > +// 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_data { > + struct gpio_chip gpio_chip; > + struct gpio_regmap *gpio; > +}; > + > +/** > + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask > + * > + * Use a simple linear mapping to translate the offset to the bitmask. > + */ > +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; > +} > +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate); Why does this need to be exported? > + > +static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gpio_regmap_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + unsigned int base; > + unsigned int val, reg, mask; This can fit on a single line with base. Same elsewhere. > + int ret; > + > + /* we might not have an output register if we are input only */ > + if (gpio->reg_dat_base.valid) > + base = gpio->reg_dat_base.addr; > + else > + base = gpio->reg_set_base.addr; > + > + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + unsigned int base = gpio->reg_set_base.addr; > + unsigned int reg, mask; > + > + gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + unsigned int base; > + unsigned int reg, mask; > + > + if (val) > + base = gpio->reg_set_base.addr; > + else > + base = gpio->reg_clr_base.addr; > + > + gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); > + regmap_write(gpio->regmap, reg, mask); > +} > + > +static int gpio_regmap_get_direction(struct gpio_chip *chip, > + unsigned int offset) > +{ > + struct gpio_regmap_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + unsigned int val, reg, mask; > + unsigned int base; > + int invert; > + int ret; > + > + if (gpio->reg_dir_out_base.valid) { > + base = gpio->reg_dir_out_base.addr; > + invert = 0; > + } else if (gpio->reg_dir_in_base.valid) { > + base = gpio->reg_dir_in_base.addr; > + invert = 1; > + } else { > + return GPIO_LINE_DIRECTION_IN; > + } > + > + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + unsigned int val, reg, mask; > + unsigned int base; > + int invert; > + int ret; > + > + if (gpio->reg_dir_out_base.valid) { > + base = gpio->reg_dir_out_base.addr; > + invert = 0; > + } else if (gpio->reg_dir_in_base.valid) { > + base = gpio->reg_dir_in_base.addr; > + invert = 1; > + } else { > + return 0; > + } > + > + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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); > +} > + > +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gpio_regmap_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + > + /* the user might have its own .to_irq callback */ > + if (gpio->to_irq) > + return gpio->to_irq(gpio, offset); > + > + return irq_create_mapping(gpio->irq_domain, offset); > +} > + > +/** > + * gpio_regmap_register() - Register a generic regmap GPIO controller > + * > + * @gpio: gpio_regmap device to register > + * > + * Returns 0 on success or an errno on failure. > + */ > +int gpio_regmap_register(struct gpio_regmap *gpio) > +{ > + struct gpio_regmap_data *d; > + struct gpio_chip *chip; > + int ret; > + > + if (!gpio->parent) > + return -EINVAL; > + > + if (!gpio->ngpio) > + return -EINVAL; > + > + /* we need at least one */ > + if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid) > + return -EINVAL; > + > + /* we don't support having both registers simulaniously for now */ > + if (gpio->reg_dir_out_base.valid && gpio->reg_dir_in_base.valid) > + return -EINVAL; > + > + /* if not set, assume they are consecutive */ > + if (!gpio->reg_stride) > + gpio->reg_stride = 1; > + > + /* if not set, assume there is only one register */ > + if (!gpio->ngpio_per_reg) > + gpio->ngpio_per_reg = gpio->ngpio; > + > + if (!gpio->reg_mask_xlate) > + gpio->reg_mask_xlate = gpio_regmap_simple_xlate; > + > + d = kzalloc(sizeof(*d), GFP_KERNEL); > + if (!d) > + return -ENOMEM; > + > + gpio->data = d; > + d->gpio = gpio; > + > + chip = &d->gpio_chip; > + chip->parent = gpio->parent; > + chip->label = gpio->label; > + chip->base = -1; > + chip->ngpio = gpio->ngpio; > + chip->can_sleep = true; > + chip->get = gpio_regmap_get; > + > + if (!chip->label) > + chip->label = dev_name(gpio->parent); > + > + if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid) > + chip->set = gpio_regmap_set_with_clear; > + else if (gpio->reg_set_base.valid) > + chip->set = gpio_regmap_set; > + > + if (gpio->reg_dir_in_base.valid || gpio->reg_dir_out_base.valid) { > + chip->get_direction = gpio_regmap_get_direction; > + chip->direction_input = gpio_regmap_direction_input; > + chip->direction_output = gpio_regmap_direction_output; > + } > + > + if (gpio->irq_domain) > + chip->to_irq = gpio_regmap_to_irq; > + > + ret = gpiochip_add_data(chip, d); > + if (ret < 0) > + goto err_alloc; > + > + return 0; > + > +err_alloc: > + kfree(d); > + return 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->data->gpio_chip); > + kfree(gpio->data); > +} > +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. > + */ > +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio) > +{ > + struct gpio_regmap **ptr; > + int ret; > + > + ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr), > + GFP_KERNEL); > + if (!ptr) > + return -ENOMEM; > + > + ret = gpio_regmap_register(gpio); > + if (ret) { > + devres_free(ptr); > + return ret; > + } > + > + *ptr = gpio; > + devres_add(dev, ptr); > + > + return 0; > +} > +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..ad63955e0e43 > --- /dev/null > +++ b/include/linux/gpio-regmap.h > @@ -0,0 +1,88 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > + > +#ifndef _LINUX_GPIO_REGMAP_H > +#define _LINUX_GPIO_REGMAP_H > + > +struct gpio_regmap_addr { > + unsigned int addr; > + bool valid; > +}; I'm not quite sure what the meaning behind the valid field here is. When would we potentially set it to false? > +#define GPIO_REGMAP_ADDR(_addr) \ > + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true }) > + > +/** > + * struct gpio_regmap - Description of a generic regmap gpio_chip. > + * > + * @parent: The parent device > + * @regmap: The regmap use to access the registers s/use/used/ > + * 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) out setting register base address > + * @reg_dir_out_base: (Optional) in 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. > + * @to_irq: (Optional) Maps GPIO offset to a irq number. > + * By default assumes a linear mapping of the > + * given irq_domain. > + * @driver_data: Pointer to the drivers private data. Not used by > + * gpio-regmap. > + * > + * 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. > + */ > +struct gpio_regmap { I'd prefer to follow a pattern seen in other such APIs of calling this structure gpio_regmap_config and creating another private structure called gpio_regmap used in callbacks that would only contain necessary fields. > + struct device *parent; > + struct regmap *regmap; > + struct gpio_regmap_data *data; > + > + const char *label; > + int ngpio; > + > + struct gpio_regmap_addr reg_dat_base; > + struct gpio_regmap_addr reg_set_base; > + struct gpio_regmap_addr reg_clr_base; > + struct gpio_regmap_addr reg_dir_in_base; > + struct gpio_regmap_addr 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); > + int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset); > + > + void *driver_data; > +}; > + > +static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, > + void *data) > +{ > + gpio->driver_data = data; > +} > + > +static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio) > +{ > + return gpio->driver_data; > +} > + > +int gpio_regmap_register(struct gpio_regmap *gpio); > +void gpio_regmap_unregister(struct gpio_regmap *gpio); > +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio); > +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base, > + unsigned int offset, > + unsigned int *reg, unsigned int *mask); > + > +#endif /* _LINUX_GPIO_REGMAP_H */ > -- > 2.20.1 > Overall looks really nice. I'm happy we'll have it in v5.8. Best regards, Bartosz Golaszewski
Hi Bartosz, Hi Mark Brown, Am 2020-04-06 09:47, schrieb Bartosz Golaszewski: > czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a): >> >> 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 >> instanciated 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> > > Hi Michael, > > Thanks for doing this! When looking at other generic drivers: > gpio-mmio and gpio-reg I can see there are some corner-cases and more > specific configuration options we could add I didn't want to copy every bit without being able to test it. > but it's not a blocker, > we'll probably be extending this one as we convert more drivers to > using it. correct, that was also my plan. > Personally I'd love to see gpio-mmio and gpio-reg removed > and replaced by a single, generic regmap interface eventually. agreed. >> --- >> drivers/gpio/Kconfig | 4 + >> drivers/gpio/Makefile | 1 + >> drivers/gpio/gpio-regmap.c | 320 >> ++++++++++++++++++++++++++++++++++++ >> include/linux/gpio-regmap.h | 88 ++++++++++ >> 4 files changed, 413 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 1b96169d84f7..a8e148f4b2e0 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 b2cfc21a97f3..93e139fdfa57 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..cc4437dc0521 >> --- /dev/null >> +++ b/drivers/gpio/gpio-regmap.c >> @@ -0,0 +1,320 @@ >> +// 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_data { >> + struct gpio_chip gpio_chip; >> + struct gpio_regmap *gpio; >> +}; >> + >> +/** >> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask >> + * >> + * Use a simple linear mapping to translate the offset to the >> bitmask. >> + */ >> +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; >> +} >> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate); > > Why does this need to be exported? Mh, the idea was that a user could also set this xlate() by himself (for whatever reason). But since it is the default, it is not really necessary. That being said, I don't care if its only local to this module. >> + >> +static int gpio_regmap_get(struct gpio_chip *chip, unsigned int >> offset) >> +{ >> + struct gpio_regmap_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + unsigned int base; >> + unsigned int val, reg, mask; > > This can fit on a single line with base. Same elsewhere. > >> + int ret; >> + >> + /* we might not have an output register if we are input only >> */ >> + if (gpio->reg_dat_base.valid) >> + base = gpio->reg_dat_base.addr; >> + else >> + base = gpio->reg_set_base.addr; >> + >> + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + unsigned int base = gpio->reg_set_base.addr; >> + unsigned int reg, mask; >> + >> + gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + unsigned int base; >> + unsigned int reg, mask; >> + >> + if (val) >> + base = gpio->reg_set_base.addr; >> + else >> + base = gpio->reg_clr_base.addr; >> + >> + gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); >> + regmap_write(gpio->regmap, reg, mask); >> +} >> + >> +static int gpio_regmap_get_direction(struct gpio_chip *chip, >> + unsigned int offset) >> +{ >> + struct gpio_regmap_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + unsigned int val, reg, mask; >> + unsigned int base; >> + int invert; >> + int ret; >> + >> + if (gpio->reg_dir_out_base.valid) { >> + base = gpio->reg_dir_out_base.addr; >> + invert = 0; >> + } else if (gpio->reg_dir_in_base.valid) { >> + base = gpio->reg_dir_in_base.addr; >> + invert = 1; >> + } else { >> + return GPIO_LINE_DIRECTION_IN; >> + } >> + >> + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + unsigned int val, reg, mask; >> + unsigned int base; >> + int invert; >> + int ret; >> + >> + if (gpio->reg_dir_out_base.valid) { >> + base = gpio->reg_dir_out_base.addr; >> + invert = 0; >> + } else if (gpio->reg_dir_in_base.valid) { >> + base = gpio->reg_dir_in_base.addr; >> + invert = 1; >> + } else { >> + return 0; >> + } >> + >> + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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); >> +} >> + >> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int >> offset) >> +{ >> + struct gpio_regmap_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + >> + /* the user might have its own .to_irq callback */ >> + if (gpio->to_irq) >> + return gpio->to_irq(gpio, offset); >> + >> + return irq_create_mapping(gpio->irq_domain, offset); >> +} >> + >> +/** >> + * gpio_regmap_register() - Register a generic regmap GPIO controller >> + * >> + * @gpio: gpio_regmap device to register >> + * >> + * Returns 0 on success or an errno on failure. >> + */ >> +int gpio_regmap_register(struct gpio_regmap *gpio) >> +{ >> + struct gpio_regmap_data *d; >> + struct gpio_chip *chip; >> + int ret; >> + >> + if (!gpio->parent) >> + return -EINVAL; >> + >> + if (!gpio->ngpio) >> + return -EINVAL; >> + >> + /* we need at least one */ >> + if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid) >> + return -EINVAL; >> + >> + /* we don't support having both registers simulaniously for >> now */ >> + if (gpio->reg_dir_out_base.valid && >> gpio->reg_dir_in_base.valid) >> + return -EINVAL; >> + >> + /* if not set, assume they are consecutive */ >> + if (!gpio->reg_stride) >> + gpio->reg_stride = 1; >> + >> + /* if not set, assume there is only one register */ >> + if (!gpio->ngpio_per_reg) >> + gpio->ngpio_per_reg = gpio->ngpio; >> + >> + if (!gpio->reg_mask_xlate) >> + gpio->reg_mask_xlate = gpio_regmap_simple_xlate; >> + >> + d = kzalloc(sizeof(*d), GFP_KERNEL); >> + if (!d) >> + return -ENOMEM; >> + >> + gpio->data = d; >> + d->gpio = gpio; >> + >> + chip = &d->gpio_chip; >> + chip->parent = gpio->parent; >> + chip->label = gpio->label; >> + chip->base = -1; >> + chip->ngpio = gpio->ngpio; >> + chip->can_sleep = true; >> + chip->get = gpio_regmap_get; >> + >> + if (!chip->label) >> + chip->label = dev_name(gpio->parent); >> + >> + if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid) >> + chip->set = gpio_regmap_set_with_clear; >> + else if (gpio->reg_set_base.valid) >> + chip->set = gpio_regmap_set; >> + >> + if (gpio->reg_dir_in_base.valid || >> gpio->reg_dir_out_base.valid) { >> + chip->get_direction = gpio_regmap_get_direction; >> + chip->direction_input = gpio_regmap_direction_input; >> + chip->direction_output = gpio_regmap_direction_output; >> + } >> + >> + if (gpio->irq_domain) >> + chip->to_irq = gpio_regmap_to_irq; >> + >> + ret = gpiochip_add_data(chip, d); >> + if (ret < 0) >> + goto err_alloc; >> + >> + return 0; >> + >> +err_alloc: >> + kfree(d); >> + return 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->data->gpio_chip); >> + kfree(gpio->data); >> +} >> +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. >> + */ >> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap >> *gpio) >> +{ >> + struct gpio_regmap **ptr; >> + int ret; >> + >> + ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr), >> + GFP_KERNEL); >> + if (!ptr) >> + return -ENOMEM; >> + >> + ret = gpio_regmap_register(gpio); >> + if (ret) { >> + devres_free(ptr); >> + return ret; >> + } >> + >> + *ptr = gpio; >> + devres_add(dev, ptr); >> + >> + return 0; >> +} >> +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..ad63955e0e43 >> --- /dev/null >> +++ b/include/linux/gpio-regmap.h >> @@ -0,0 +1,88 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> + >> +#ifndef _LINUX_GPIO_REGMAP_H >> +#define _LINUX_GPIO_REGMAP_H >> + >> +struct gpio_regmap_addr { >> + unsigned int addr; >> + bool valid; >> +}; > > I'm not quite sure what the meaning behind the valid field here is. > When would we potentially set it to false? Some base addresses are optional, but on the other hand, a base address of 0 could also be valid. So I cannot use 0 as an indicator whether a base address is set or not. The generic mmio driver has some special case for the ack base, where there is a use_ack flag which forces to use the ack register even if its zero. So I've had a look at the kernel if there is a better idiom for that, but I haven't found anything. So the best from a user perspective I've could come up with was: ->base_reg = GPIO_REGMAP_ADDR(addr); I'm open for suggestions. > >> +#define GPIO_REGMAP_ADDR(_addr) \ >> + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true }) >> + >> +/** >> + * struct gpio_regmap - Description of a generic regmap gpio_chip. >> + * >> + * @parent: The parent device >> + * @regmap: The regmap use to access the registers > > s/use/used/ > >> + * 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) out setting register base address >> + * @reg_dir_out_base: (Optional) in 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. >> + * @to_irq: (Optional) Maps GPIO offset to a irq number. >> + * By default assumes a linear mapping of the >> + * given irq_domain. >> + * @driver_data: Pointer to the drivers private data. Not used >> by >> + * gpio-regmap. >> + * >> + * 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. >> + */ >> +struct gpio_regmap { > > I'd prefer to follow a pattern seen in other such APIs of calling this > structure gpio_regmap_config and creating another private structure > called gpio_regmap used in callbacks that would only contain necessary > fields. something like the following? struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *) but if that structure is private, how can a callback access individual elements? Or do you mean private in "local to the gpio drivers"? Also I was unsure about the naming, eg. some use stuff_register()/stuff_unregister() and some stuff_add()/stuff_remove(). > >> + struct device *parent; >> + struct regmap *regmap; >> + struct gpio_regmap_data *data; >> + >> + const char *label; >> + int ngpio; >> + >> + struct gpio_regmap_addr reg_dat_base; >> + struct gpio_regmap_addr reg_set_base; >> + struct gpio_regmap_addr reg_clr_base; >> + struct gpio_regmap_addr reg_dir_in_base; >> + struct gpio_regmap_addr 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); >> + int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset); >> + >> + void *driver_data; >> +}; >> + >> +static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, >> + void *data) >> +{ >> + gpio->driver_data = data; >> +} >> + >> +static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio) >> +{ >> + return gpio->driver_data; >> +} >> + >> +int gpio_regmap_register(struct gpio_regmap *gpio); >> +void gpio_regmap_unregister(struct gpio_regmap *gpio); >> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap >> *gpio); >> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int >> base, >> + unsigned int offset, >> + unsigned int *reg, unsigned int *mask); >> + >> +#endif /* _LINUX_GPIO_REGMAP_H */ >> -- >> 2.20.1 >> > > Overall looks really nice. I'm happy we'll have it in v5.8. Thanks, one thing I'm uncertain about is the regmap_irq change and if that is acceptable. So Mark would need to comment on that. -michael
pon., 6 kwi 2020 o 12:10 Michael Walle <michael@walle.cc> napisał(a): > > > Hi Bartosz, Hi Mark Brown, > > Am 2020-04-06 09:47, schrieb Bartosz Golaszewski: > > czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a): > >> > >> 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 > >> instanciated 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> > > > > Hi Michael, > > > > Thanks for doing this! When looking at other generic drivers: > > gpio-mmio and gpio-reg I can see there are some corner-cases and more > > specific configuration options we could add > > I didn't want to copy every bit without being able to test it. > Sure, I didn't mean we need to do it now - just set it as the future goal. > > but it's not a blocker, > > we'll probably be extending this one as we convert more drivers to > > using it. > > correct, that was also my plan. > > > Personally I'd love to see gpio-mmio and gpio-reg removed > > and replaced by a single, generic regmap interface eventually. > > agreed. > > [snip!] > >> + > >> +/** > >> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask > >> + * > >> + * Use a simple linear mapping to translate the offset to the > >> bitmask. > >> + */ > >> +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; > >> +} > >> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate); > > > > Why does this need to be exported? > > Mh, the idea was that a user could also set this xlate() by himself (for > whatever reason). But since it is the default, it is not really > necessary. > That being said, I don't care if its only local to this module. > Let's only export symbols that have external users then. [snip!] > >> + > >> +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..ad63955e0e43 > >> --- /dev/null > >> +++ b/include/linux/gpio-regmap.h > >> @@ -0,0 +1,88 @@ > >> +/* SPDX-License-Identifier: GPL-2.0-only */ > >> + > >> +#ifndef _LINUX_GPIO_REGMAP_H > >> +#define _LINUX_GPIO_REGMAP_H > >> + > >> +struct gpio_regmap_addr { > >> + unsigned int addr; > >> + bool valid; > >> +}; > > > > I'm not quite sure what the meaning behind the valid field here is. > > When would we potentially set it to false? > > Some base addresses are optional, but on the other hand, a base address > of 0 could also be valid. So I cannot use 0 as an indicator whether a > base address is set or not. The generic mmio driver has some special > case for the ack base, where there is a use_ack flag which forces to > use the ack register even if its zero. So I've had a look at the kernel > if there is a better idiom for that, but I haven't found anything. > > So the best from a user perspective I've could come up with was: > > ->base_reg = GPIO_REGMAP_ADDR(addr); > > I'm open for suggestions. > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in IS_ERR() returning true? > > > >> +#define GPIO_REGMAP_ADDR(_addr) \ > >> + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true }) > >> + > >> +/** > >> + * struct gpio_regmap - Description of a generic regmap gpio_chip. > >> + * > >> + * @parent: The parent device > >> + * @regmap: The regmap use to access the registers > > > > s/use/used/ > > > >> + * 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) out setting register base address > >> + * @reg_dir_out_base: (Optional) in 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. > >> + * @to_irq: (Optional) Maps GPIO offset to a irq number. > >> + * By default assumes a linear mapping of the > >> + * given irq_domain. > >> + * @driver_data: Pointer to the drivers private data. Not used > >> by > >> + * gpio-regmap. > >> + * > >> + * 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. > >> + */ > >> +struct gpio_regmap { > > > > I'd prefer to follow a pattern seen in other such APIs of calling this > > structure gpio_regmap_config and creating another private structure > > called gpio_regmap used in callbacks that would only contain necessary > > fields. > > something like the following? > > struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *) > > but if that structure is private, how can a callback access individual > elements? Or do you mean private in "local to the gpio drivers"? > Either making the structure local to drivers/gpio or making it entirely opaque and providing accessor functions. Depending on how much of the structure one may want to access. > Also I was unsure about the naming, eg. some use > stuff_register()/stuff_unregister() and some stuff_add()/stuff_remove(). > register/unregister is fine with me. Bart
Am 2020-04-14 11:50, schrieb Bartosz Golaszewski: > pon., 6 kwi 2020 o 12:10 Michael Walle <michael@walle.cc> napisał(a): >> >> >> Hi Bartosz, Hi Mark Brown, >> >> Am 2020-04-06 09:47, schrieb Bartosz Golaszewski: >> > czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a): >> >> >> >> 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 >> >> instanciated 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> >> > >> > Hi Michael, >> > >> > Thanks for doing this! When looking at other generic drivers: >> > gpio-mmio and gpio-reg I can see there are some corner-cases and more >> > specific configuration options we could add >> >> I didn't want to copy every bit without being able to test it. >> > > Sure, I didn't mean we need to do it now - just set it as the future > goal. > >> > but it's not a blocker, >> > we'll probably be extending this one as we convert more drivers to >> > using it. >> >> correct, that was also my plan. >> >> > Personally I'd love to see gpio-mmio and gpio-reg removed >> > and replaced by a single, generic regmap interface eventually. >> >> agreed. >> >> > > [snip!] > >> >> + >> >> +/** >> >> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask >> >> + * >> >> + * Use a simple linear mapping to translate the offset to the >> >> bitmask. >> >> + */ >> >> +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; >> >> +} >> >> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate); >> > >> > Why does this need to be exported? >> >> Mh, the idea was that a user could also set this xlate() by himself >> (for >> whatever reason). But since it is the default, it is not really >> necessary. >> That being said, I don't care if its only local to this module. >> > > Let's only export symbols that have external users then. > > [snip!] > >> >> + >> >> +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..ad63955e0e43 >> >> --- /dev/null >> >> +++ b/include/linux/gpio-regmap.h >> >> @@ -0,0 +1,88 @@ >> >> +/* SPDX-License-Identifier: GPL-2.0-only */ >> >> + >> >> +#ifndef _LINUX_GPIO_REGMAP_H >> >> +#define _LINUX_GPIO_REGMAP_H >> >> + >> >> +struct gpio_regmap_addr { >> >> + unsigned int addr; >> >> + bool valid; >> >> +}; >> > >> > I'm not quite sure what the meaning behind the valid field here is. >> > When would we potentially set it to false? >> >> Some base addresses are optional, but on the other hand, a base >> address >> of 0 could also be valid. So I cannot use 0 as an indicator whether a >> base address is set or not. The generic mmio driver has some special >> case for the ack base, where there is a use_ack flag which forces to >> use the ack register even if its zero. So I've had a look at the >> kernel >> if there is a better idiom for that, but I haven't found anything. >> >> So the best from a user perspective I've could come up with was: >> >> ->base_reg = GPIO_REGMAP_ADDR(addr); >> >> I'm open for suggestions. >> > > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in > IS_ERR() returning true? Unfortunatly, its not a pointer, but only a regular unsigned int (ie the type the regmap API has for its "reg" property). It could be a pointer of course but then the user would have to allocate additional memory. -michael > >> > >> >> +#define GPIO_REGMAP_ADDR(_addr) \ >> >> + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true }) >> >> + >> >> +/** >> >> + * struct gpio_regmap - Description of a generic regmap gpio_chip. >> >> + * >> >> + * @parent: The parent device >> >> + * @regmap: The regmap use to access the registers >> > >> > s/use/used/ >> > >> >> + * 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) out setting register base address >> >> + * @reg_dir_out_base: (Optional) in 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. >> >> + * @to_irq: (Optional) Maps GPIO offset to a irq number. >> >> + * By default assumes a linear mapping of the >> >> + * given irq_domain. >> >> + * @driver_data: Pointer to the drivers private data. Not used >> >> by >> >> + * gpio-regmap. >> >> + * >> >> + * 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. >> >> + */ >> >> +struct gpio_regmap { >> > >> > I'd prefer to follow a pattern seen in other such APIs of calling this >> > structure gpio_regmap_config and creating another private structure >> > called gpio_regmap used in callbacks that would only contain necessary >> > fields. >> >> something like the following? >> >> struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *) >> >> but if that structure is private, how can a callback access individual >> elements? Or do you mean private in "local to the gpio drivers"? >> > > Either making the structure local to drivers/gpio or making it > entirely opaque and providing accessor functions. Depending on how > much of the structure one may want to access. > >> Also I was unsure about the naming, eg. some use >> stuff_register()/stuff_unregister() and some >> stuff_add()/stuff_remove(). >> > > register/unregister is fine with me. > > Bart
wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a): > >> > >> So the best from a user perspective I've could come up with was: > >> > >> ->base_reg = GPIO_REGMAP_ADDR(addr); > >> > >> I'm open for suggestions. > >> > > > > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in > > IS_ERR() returning true? > > Unfortunatly, its not a pointer, but only a regular unsigned int (ie > the type the regmap API has for its "reg" property). It could be a > pointer of course but then the user would have to allocate additional > memory. > > -michael > Eek, of course it's not a pointer. If possible I'd like to avoid this GPIO_REGMAP_ADDR() macro, so how about having some separate field for invalid offsets making every offset 'valid' by default? Linus: do you have a better idea? Bart
On Tue, Apr 14, 2020 at 12:07:01PM +0200, Michael Walle wrote: > Am 2020-04-14 11:50, schrieb Bartosz Golaszewski: > > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in > > IS_ERR() returning true? > Unfortunatly, its not a pointer, but only a regular unsigned int (ie > the type the regmap API has for its "reg" property). It could be a > pointer of course but then the user would have to allocate additional > memory. You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some other suitably implausible address and use that as a value. It's possible that there might be a collision with a real address on some device but it should be sufficiently unlikely to be useful, especially if it's not something regmap in general goes and evaluates. For extra safety we could have an API for allowing users to query the register validity information regmap has (or can be given) and gpiolib could then use that to figure out if the value was actually a dummy value but that's probably overdoing it.
Am 2020-04-14 19:21, schrieb Mark Brown: > On Tue, Apr 14, 2020 at 12:07:01PM +0200, Michael Walle wrote: >> Am 2020-04-14 11:50, schrieb Bartosz Golaszewski: > >> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in >> > IS_ERR() returning true? > >> Unfortunatly, its not a pointer, but only a regular unsigned int (ie >> the type the regmap API has for its "reg" property). It could be a >> pointer of course but then the user would have to allocate additional >> memory. > > You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some > other suitably implausible address and use that as a value. It's > possible that there might be a collision with a real address on some > device but it should be sufficiently unlikely to be useful, especially > if it's not something regmap in general goes and evaluates. For extra > safety we could have an API for allowing users to query the register > validity information regmap has (or can be given) and gpiolib could > then > use that to figure out if the value was actually a dummy value but > that's probably overdoing it. If possible, I'd like to have the opposite logic. That is, if it is not set it should be invalid. If we have a magic macro like REGMAP_INVALID_ADDR, we must assign it to all the unused addresses. Thus every driver would have to assign all addresses and if in the future there will be some added, we'd have to touch all the drivers which use gpio_regmap. -michael
On Tue, Apr 14, 2020 at 08:36:23PM +0200, Michael Walle wrote: > Am 2020-04-14 19:21, schrieb Mark Brown: > > You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some > > other suitably implausible address and use that as a value. It's > > possible that there might be a collision with a real address on some > > device but it should be sufficiently unlikely to be useful, especially > > if it's not something regmap in general goes and evaluates. For extra > > safety we could have an API for allowing users to query the register > > validity information regmap has (or can be given) and gpiolib could then > > use that to figure out if the value was actually a dummy value but > > that's probably overdoing it. > If possible, I'd like to have the opposite logic. That is, if it is not > set it should be invalid. If we have a magic macro like > REGMAP_INVALID_ADDR, we must assign it to all the unused addresses. Thus > every driver would have to assign all addresses and if in the future > there will be some added, we'd have to touch all the drivers which use > gpio_regmap. Sure, for that you'd need a separate flag since zero is such a commonly valid address.
Am 2020-04-14 19:00, schrieb Bartosz Golaszewski: > wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a): >> >> >> >> So the best from a user perspective I've could come up with was: >> >> >> >> ->base_reg = GPIO_REGMAP_ADDR(addr); >> >> >> >> I'm open for suggestions. >> >> >> > >> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in >> > IS_ERR() returning true? >> >> Unfortunatly, its not a pointer, but only a regular unsigned int (ie >> the type the regmap API has for its "reg" property). It could be a >> pointer of course but then the user would have to allocate additional >> memory. >> >> -michael >> > > Eek, of course it's not a pointer. If possible I'd like to avoid this > GPIO_REGMAP_ADDR() macro, so how about having some separate field for > invalid offsets making every offset 'valid' by default? IMHO this has the same problems as mentioned in the response to Mark's idea. Normally, the user sets only some addresses, thus he has to mark all other as invalid. And if you add another address, you have to touch all the drivers to mark it as invalid. We could add some force bits like the "use_ack" flag in the bgpio driver, where you can force the use of the value 0. But I'd really like to find a better way.. -michael > > Linus: do you have a better idea? > > Bart
Hi Mark, Hi Bartosz, Hi Linus, Am 2020-04-14 20:41, schrieb Michael Walle: > Am 2020-04-14 19:00, schrieb Bartosz Golaszewski: >> wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a): >>> >> >>> >> So the best from a user perspective I've could come up with was: >>> >> >>> >> ->base_reg = GPIO_REGMAP_ADDR(addr); >>> >> >>> >> I'm open for suggestions. >>> >> >>> > >>> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in >>> > IS_ERR() returning true? >>> >>> Unfortunatly, its not a pointer, but only a regular unsigned int (ie >>> the type the regmap API has for its "reg" property). It could be a >>> pointer of course but then the user would have to allocate additional >>> memory. >>> >>> -michael >>> >> >> Eek, of course it's not a pointer. If possible I'd like to avoid this >> GPIO_REGMAP_ADDR() macro, so how about having some separate field for >> invalid offsets making every offset 'valid' by default? > > IMHO this has the same problems as mentioned in the response to Mark's > idea. Normally, the user sets only some addresses, thus he has to mark > all other as invalid. And if you add another address, you have to touch > all the drivers to mark it as invalid. > > We could add some force bits like the "use_ack" flag in the bgpio > driver, > where you can force the use of the value 0. But I'd really like to find > a better way.. So what about the following: #define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1) So this way the user might assign the base addresses the normal way except when he wants to use zero, in that case he has to use ->base_adr = GPIO_REGMAP_ADDR_ZERO; gpio-regmap.c could use then: if (base_addr) something_useful(gpio_regmap_addr(base_addr)); unsigned int gpio_regmap_addr(unsigned int addr) { return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr; } -michael
On Tue, Apr 14, 2020 at 9:57 PM Michael Walle <michael@walle.cc> wrote: > So what about the following: > > #define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1) Yeah with regmap explicitly using int I guess we can't use S32_MAX, so that is fair. > So this way the user might assign the base addresses the normal way > except when he wants to use zero, in that case he has to use > > ->base_adr = GPIO_REGMAP_ADDR_ZERO; > > gpio-regmap.c could use then: > > if (base_addr) > something_useful(gpio_regmap_addr(base_addr)); > > unsigned int gpio_regmap_addr(unsigned int addr) > { > return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr; > } That's reasonably clean. Yours, Linus Walleij
On Thu, Apr 2, 2020 at 10:37 PM 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 > instanciated 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> Overall I really like this driver and I think we should merge is as soon as it is in reasonable shape and then improve on top so we can start migrating drivers to it. > +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset) > +{ > + struct gpio_regmap_data *data = gpiochip_get_data(chip); > + struct gpio_regmap *gpio = data->gpio; > + > + /* the user might have its own .to_irq callback */ > + if (gpio->to_irq) > + return gpio->to_irq(gpio, offset); > + > + return irq_create_mapping(gpio->irq_domain, offset); I think that should at least be irq_find_mapping(), the mapping should definately not be created by the .to_irq() callback since that is just a convenience function. > + if (gpio->irq_domain) > + chip->to_irq = gpio_regmap_to_irq; I don't know about this. (...) > + * @irq_domain: (Optional) IRQ domain if the controller is > + * interrupt-capable (...) > + struct irq_domain *irq_domain; I don't think this is a good storage place for the irqdomain, we already have gpio_irq_chip inside gpio_chip and that has an irqdomain, we should strive to reuse that infrastructure also for regmap GPIO I think, for now I would just leave .to_irq() out of this and let the driver deal with any irqs. Yours, Linus Walleij
Am 2020-04-16 11:20, schrieb Linus Walleij: > On Tue, Apr 14, 2020 at 9:57 PM Michael Walle <michael@walle.cc> wrote: > >> So what about the following: >> >> #define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1) > > Yeah with regmap explicitly using int I guess we can't use > S32_MAX, so that is fair. > >> So this way the user might assign the base addresses the normal way >> except when he wants to use zero, in that case he has to use >> >> ->base_adr = GPIO_REGMAP_ADDR_ZERO; >> >> gpio-regmap.c could use then: >> >> if (base_addr) >> something_useful(gpio_regmap_addr(base_addr)); >> >> unsigned int gpio_regmap_addr(unsigned int addr) >> { >> return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr; >> } > > That's reasonably clean. Ok, at least on that side. For my sl28 gpio driver I then have the problem that depending on 'base' I might have to use GPIO_REGMAP_ADDR_ZERO: #define GPIO_REG_DIR 0 config.reg_dir_out_base = base + GPIO_REG_DIR; So there is still a convenience macro: #define GPIO_REGMAP_ADDR(addr) ((addr) ? addr : GPIO_REGMAP_ADDR_ZERO) which you can use if you can't be sure that the address is not non-zero. So the code in my sl28 gpio driver looks like: config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR); I'll respin the patch with the current remarks. -michael
Hi Linus, Am 2020-04-16 11:27, schrieb Linus Walleij: > On Thu, Apr 2, 2020 at 10:37 PM 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 >> instanciated 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> > > Overall I really like this driver and I think we should merge is as > soon > as it is in reasonable shape and then improve on top so we can start > migrating drivers to it. > >> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int >> offset) >> +{ >> + struct gpio_regmap_data *data = gpiochip_get_data(chip); >> + struct gpio_regmap *gpio = data->gpio; >> + >> + /* the user might have its own .to_irq callback */ >> + if (gpio->to_irq) >> + return gpio->to_irq(gpio, offset); >> + >> + return irq_create_mapping(gpio->irq_domain, offset); > > I think that should at least be irq_find_mapping(), the mapping should > definately not be created by the .to_irq() callback since that is just > a convenience function. what do you mean by conenience function? are there other ways? if you use irq_find_mapping() who will create the mappings? most gpio drivers use a similar function like gpio_regmap_to_irq(). > >> + if (gpio->irq_domain) >> + chip->to_irq = gpio_regmap_to_irq; > > I don't know about this. > (...) >> + * @irq_domain: (Optional) IRQ domain if the >> controller is >> + * interrupt-capable > (...) >> + struct irq_domain *irq_domain; > > I don't think this is a good storage place for the irqdomain, we > already have > gpio_irq_chip inside gpio_chip and that has an irqdomain, we should > strive to reuse that infrastructure also for regmap GPIO I think, for > now > I would just leave .to_irq() out of this and let the driver deal with > any > irqs. How would a driver attach the to_irq callback then? At the moment, the gpio_regmap doesn't expose the gpio_chip. So either we have to do that or the config still have to have a .to_irq property. -michael
Hi Linus, Am 2020-04-17 08:34, schrieb Michael Walle: > Hi Linus, > > Am 2020-04-16 11:27, schrieb Linus Walleij: >> On Thu, Apr 2, 2020 at 10:37 PM 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 >>> instanciated 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> >> >> Overall I really like this driver and I think we should merge is as >> soon >> as it is in reasonable shape and then improve on top so we can start >> migrating drivers to it. >> >>> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int >>> offset) >>> +{ >>> + struct gpio_regmap_data *data = gpiochip_get_data(chip); >>> + struct gpio_regmap *gpio = data->gpio; >>> + >>> + /* the user might have its own .to_irq callback */ >>> + if (gpio->to_irq) >>> + return gpio->to_irq(gpio, offset); >>> + >>> + return irq_create_mapping(gpio->irq_domain, offset); >> >> I think that should at least be irq_find_mapping(), the mapping should >> definately not be created by the .to_irq() callback since that is just >> a convenience function. > > what do you mean by conenience function? are there other ways? if you > use > irq_find_mapping() who will create the mappings? most gpio drivers use > a > similar function like gpio_regmap_to_irq(). > >> >>> + if (gpio->irq_domain) >>> + chip->to_irq = gpio_regmap_to_irq; >> >> I don't know about this. >> (...) >>> + * @irq_domain: (Optional) IRQ domain if the >>> controller is >>> + * interrupt-capable >> (...) >>> + struct irq_domain *irq_domain; >> >> I don't think this is a good storage place for the irqdomain, we >> already have >> gpio_irq_chip inside gpio_chip and that has an irqdomain, we should >> strive to reuse that infrastructure also for regmap GPIO I think, for >> now >> I would just leave .to_irq() out of this and let the driver deal with >> any >> irqs. > > How would a driver attach the to_irq callback then? At the moment, the > gpio_regmap doesn't expose the gpio_chip. So either we have to do that > or > the config still have to have a .to_irq property. Also, if I move the interrupt hanling completely out of the gpio-regmap, the driver would have to deal with "struct gpio_chip" which I would like to avoid if possible and keep it private to gpio-regmap. Unfortunately, I don't have much experience how a good API for the interrupt handling and the gpio-regmap might look like. And there seems to be some overlap between regmap-irq and the interrupt stuff in gpiolib. For example, both provide and set the irq_domain_ops. Thus handing the domain over to gpio-regmap looked like a good idea to me. I get you point, that there is already a irqdomain in gpiolib and also a _to_irq() which is the same as the current implementation in gpio-regmap. Maybe it makes sense to just have a new function int gpiolib_add_irqdomain(struct gpio_chip *gc, struct irq_domain domain) { gc->irq.domain = domain; gc->to_irq = gpiochip_to_irq; } which is called by gpio_regmap_register() if a config->irq_domain is given. -michael
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 1b96169d84f7..a8e148f4b2e0 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 b2cfc21a97f3..93e139fdfa57 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..cc4437dc0521 --- /dev/null +++ b/drivers/gpio/gpio-regmap.c @@ -0,0 +1,320 @@ +// 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_data { + struct gpio_chip gpio_chip; + struct gpio_regmap *gpio; +}; + +/** + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask + * + * Use a simple linear mapping to translate the offset to the bitmask. + */ +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; +} +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate); + +static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset) +{ + struct gpio_regmap_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + unsigned int base; + unsigned int val, reg, mask; + int ret; + + /* we might not have an output register if we are input only */ + if (gpio->reg_dat_base.valid) + base = gpio->reg_dat_base.addr; + else + base = gpio->reg_set_base.addr; + + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + unsigned int base = gpio->reg_set_base.addr; + unsigned int reg, mask; + + gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + unsigned int base; + unsigned int reg, mask; + + if (val) + base = gpio->reg_set_base.addr; + else + base = gpio->reg_clr_base.addr; + + gpio->reg_mask_xlate(gpio, base, offset, ®, &mask); + regmap_write(gpio->regmap, reg, mask); +} + +static int gpio_regmap_get_direction(struct gpio_chip *chip, + unsigned int offset) +{ + struct gpio_regmap_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + unsigned int val, reg, mask; + unsigned int base; + int invert; + int ret; + + if (gpio->reg_dir_out_base.valid) { + base = gpio->reg_dir_out_base.addr; + invert = 0; + } else if (gpio->reg_dir_in_base.valid) { + base = gpio->reg_dir_in_base.addr; + invert = 1; + } else { + return GPIO_LINE_DIRECTION_IN; + } + + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + unsigned int val, reg, mask; + unsigned int base; + int invert; + int ret; + + if (gpio->reg_dir_out_base.valid) { + base = gpio->reg_dir_out_base.addr; + invert = 0; + } else if (gpio->reg_dir_in_base.valid) { + base = gpio->reg_dir_in_base.addr; + invert = 1; + } else { + return 0; + } + + ret = gpio->reg_mask_xlate(gpio, base, offset, ®, &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); +} + +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset) +{ + struct gpio_regmap_data *data = gpiochip_get_data(chip); + struct gpio_regmap *gpio = data->gpio; + + /* the user might have its own .to_irq callback */ + if (gpio->to_irq) + return gpio->to_irq(gpio, offset); + + return irq_create_mapping(gpio->irq_domain, offset); +} + +/** + * gpio_regmap_register() - Register a generic regmap GPIO controller + * + * @gpio: gpio_regmap device to register + * + * Returns 0 on success or an errno on failure. + */ +int gpio_regmap_register(struct gpio_regmap *gpio) +{ + struct gpio_regmap_data *d; + struct gpio_chip *chip; + int ret; + + if (!gpio->parent) + return -EINVAL; + + if (!gpio->ngpio) + return -EINVAL; + + /* we need at least one */ + if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid) + return -EINVAL; + + /* we don't support having both registers simulaniously for now */ + if (gpio->reg_dir_out_base.valid && gpio->reg_dir_in_base.valid) + return -EINVAL; + + /* if not set, assume they are consecutive */ + if (!gpio->reg_stride) + gpio->reg_stride = 1; + + /* if not set, assume there is only one register */ + if (!gpio->ngpio_per_reg) + gpio->ngpio_per_reg = gpio->ngpio; + + if (!gpio->reg_mask_xlate) + gpio->reg_mask_xlate = gpio_regmap_simple_xlate; + + d = kzalloc(sizeof(*d), GFP_KERNEL); + if (!d) + return -ENOMEM; + + gpio->data = d; + d->gpio = gpio; + + chip = &d->gpio_chip; + chip->parent = gpio->parent; + chip->label = gpio->label; + chip->base = -1; + chip->ngpio = gpio->ngpio; + chip->can_sleep = true; + chip->get = gpio_regmap_get; + + if (!chip->label) + chip->label = dev_name(gpio->parent); + + if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid) + chip->set = gpio_regmap_set_with_clear; + else if (gpio->reg_set_base.valid) + chip->set = gpio_regmap_set; + + if (gpio->reg_dir_in_base.valid || gpio->reg_dir_out_base.valid) { + chip->get_direction = gpio_regmap_get_direction; + chip->direction_input = gpio_regmap_direction_input; + chip->direction_output = gpio_regmap_direction_output; + } + + if (gpio->irq_domain) + chip->to_irq = gpio_regmap_to_irq; + + ret = gpiochip_add_data(chip, d); + if (ret < 0) + goto err_alloc; + + return 0; + +err_alloc: + kfree(d); + return 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->data->gpio_chip); + kfree(gpio->data); +} +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. + */ +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio) +{ + struct gpio_regmap **ptr; + int ret; + + ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr), + GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = gpio_regmap_register(gpio); + if (ret) { + devres_free(ptr); + return ret; + } + + *ptr = gpio; + devres_add(dev, ptr); + + return 0; +} +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..ad63955e0e43 --- /dev/null +++ b/include/linux/gpio-regmap.h @@ -0,0 +1,88 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _LINUX_GPIO_REGMAP_H +#define _LINUX_GPIO_REGMAP_H + +struct gpio_regmap_addr { + unsigned int addr; + bool valid; +}; +#define GPIO_REGMAP_ADDR(_addr) \ + ((struct gpio_regmap_addr) { .addr = _addr, .valid = true }) + +/** + * struct gpio_regmap - Description of a generic regmap gpio_chip. + * + * @parent: The parent device + * @regmap: The regmap use 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) out setting register base address + * @reg_dir_out_base: (Optional) in 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. + * @to_irq: (Optional) Maps GPIO offset to a irq number. + * By default assumes a linear mapping of the + * given irq_domain. + * @driver_data: Pointer to the drivers private data. Not used by + * gpio-regmap. + * + * 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. + */ +struct gpio_regmap { + struct device *parent; + struct regmap *regmap; + struct gpio_regmap_data *data; + + const char *label; + int ngpio; + + struct gpio_regmap_addr reg_dat_base; + struct gpio_regmap_addr reg_set_base; + struct gpio_regmap_addr reg_clr_base; + struct gpio_regmap_addr reg_dir_in_base; + struct gpio_regmap_addr 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); + int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset); + + void *driver_data; +}; + +static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, + void *data) +{ + gpio->driver_data = data; +} + +static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio) +{ + return gpio->driver_data; +} + +int gpio_regmap_register(struct gpio_regmap *gpio); +void gpio_regmap_unregister(struct gpio_regmap *gpio); +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio); +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base, + unsigned int offset, + unsigned int *reg, unsigned int *mask); + +#endif /* _LINUX_GPIO_REGMAP_H */
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 instanciated 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 | 320 ++++++++++++++++++++++++++++++++++++ include/linux/gpio-regmap.h | 88 ++++++++++ 4 files changed, 413 insertions(+) create mode 100644 drivers/gpio/gpio-regmap.c create mode 100644 include/linux/gpio-regmap.h