* [PATCH 1/2] gpio: regmap: Support few IC specific operations @ 2021-05-20 11:28 Matti Vaittinen 2021-05-20 11:29 ` [PATCH 2/2] gpio: bd71815: Use gpio-regmap Matti Vaittinen ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Matti Vaittinen @ 2021-05-20 11:28 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, linux-gpio, linux-kernel, linux-power [-- Attachment #1: Type: text/plain, Size: 5500 bytes --] The set_config and init_valid_mask GPIO operations are usually very IC specific. Allow IC drivers to provide these custom operations at gpio-regmap registration. Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/gpio/gpio-regmap.c | 49 +++++++++++++++++++++++++++++++++++++ include/linux/gpio/regmap.h | 13 ++++++++++ 2 files changed, 62 insertions(+) diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 134cedf151a7..315285cacd3f 100644 --- a/drivers/gpio/gpio-regmap.c +++ b/drivers/gpio/gpio-regmap.c @@ -27,6 +27,10 @@ struct gpio_regmap { int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask); + int (*set_config)(struct regmap *regmap, void *drvdata, + unsigned int offset, unsigned long config); + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, + unsigned long *valid_mask, unsigned int ngpios); void *driver_data; }; @@ -39,6 +43,43 @@ static unsigned int gpio_regmap_addr(unsigned int addr) return addr; } +static int regmap_gpio_init_valid_mask(struct gpio_chip *gc, + unsigned long *valid_mask, + unsigned int ngpios) +{ + struct gpio_regmap *gpio; + void *drvdata; + + gpio = gpiochip_get_data(gc); + + if (!gpio->init_valid_mask) { + WARN_ON(!gpio->init_valid_mask); + return -EINVAL; + } + + drvdata = gpio_regmap_get_drvdata(gpio); + + return gpio->init_valid_mask(gpio->regmap, drvdata, valid_mask, ngpios); +} + +static int gpio_regmap_set_config(struct gpio_chip *gc, unsigned int offset, + unsigned long config) +{ + struct gpio_regmap *gpio; + void *drvdata; + + gpio = gpiochip_get_data(gc); + + if (!gpio->set_config) { + WARN_ON(!gpio->set_config); + return -EINVAL; + } + + drvdata = gpio_regmap_get_drvdata(gpio); + + return gpio->set_config(gpio->regmap, drvdata, offset, config); +} + static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask) @@ -235,6 +276,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config gpio->reg_clr_base = config->reg_clr_base; gpio->reg_dir_in_base = config->reg_dir_in_base; gpio->reg_dir_out_base = config->reg_dir_out_base; + gpio->set_config = config->set_config; + gpio->init_valid_mask = config->init_valid_mask; /* if not set, assume there is only one register */ if (!gpio->ngpio_per_reg) @@ -253,6 +296,10 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config chip->ngpio = config->ngpio; chip->names = config->names; chip->label = config->label ?: dev_name(config->parent); + if (gpio->set_config) + chip->set_config = gpio_regmap_set_config; + if (gpio->init_valid_mask) + chip->init_valid_mask = regmap_gpio_init_valid_mask; #if defined(CONFIG_OF_GPIO) /* gpiolib will use of_node of the parent if chip->of_node is NULL */ @@ -280,6 +327,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config chip->direction_output = gpio_regmap_direction_output; } + gpio_regmap_set_drvdata(gpio, config->drvdata); + ret = gpiochip_add_data(chip, gpio); if (ret < 0) goto err_free_gpio; diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h index 334dd928042b..c382a3caefc3 100644 --- a/include/linux/gpio/regmap.h +++ b/include/linux/gpio/regmap.h @@ -33,10 +33,18 @@ struct regmap; * @ngpio_per_reg: Number of GPIOs per register * @irq_domain: (Optional) IRQ domain if the controller is * interrupt-capable + * @drvdata: (Optional) Pointer to IC specific data which is + * not used by gpio-remap but is provided "as is" to + * the driver callback(s). + * * @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. + * @set_config: (Optional) hook for all kinds of settings. Uses + * the same packed config format as generic pinconf. + * @init_valid_mask: (Optional) routine to initialize @valid_mask, to + * be used if not all GPIOs are valid. * * The ->reg_mask_xlate translates a given base address and GPIO offset to * register and mask pair. The base address is one of the given register @@ -74,10 +82,15 @@ struct gpio_regmap_config { int reg_stride; int ngpio_per_reg; struct irq_domain *irq_domain; + void *drvdata; int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, unsigned int offset, unsigned int *reg, unsigned int *mask); + int (*set_config)(struct regmap *regmap, void *drvdata, + unsigned int offset, unsigned long config); + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, + unsigned long *valid_mask, unsigned int ngpios); }; struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config); base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] gpio: bd71815: Use gpio-regmap 2021-05-20 11:28 [PATCH 1/2] gpio: regmap: Support few IC specific operations Matti Vaittinen @ 2021-05-20 11:29 ` Matti Vaittinen 2021-05-25 15:51 ` Linus Walleij 2021-05-20 11:39 ` [PATCH 1/2] gpio: regmap: Support few IC specific operations Vaittinen, Matti 2021-05-20 11:42 ` Michael Walle 2 siblings, 1 reply; 10+ messages in thread From: Matti Vaittinen @ 2021-05-20 11:29 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, linux-gpio, linux-kernel, linux-power [-- Attachment #1: Type: text/plain, Size: 6380 bytes --] Utilize the gpio-regmap helper and drop the custom functions Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> --- drivers/gpio/Kconfig | 1 + drivers/gpio/gpio-bd71815.c | 106 ++++++++++-------------------------- 2 files changed, 29 insertions(+), 78 deletions(-) diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 1dd0ec6727fd..97e1348cd410 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -1120,6 +1120,7 @@ config GPIO_BD70528 config GPIO_BD71815 tristate "ROHM BD71815 PMIC GPIO support" depends on MFD_ROHM_BD71828 + select GPIO_REGMAP help Support for GPO(s) on ROHM BD71815 PMIC. There are two GPOs available on the ROHM PMIC. diff --git a/drivers/gpio/gpio-bd71815.c b/drivers/gpio/gpio-bd71815.c index 08ff2857256f..a241c01e08d1 100644 --- a/drivers/gpio/gpio-bd71815.c +++ b/drivers/gpio/gpio-bd71815.c @@ -9,6 +9,7 @@ */ #include <linux/gpio/driver.h> +#include <linux/gpio/regmap.h> #include <linux/init.h> #include <linux/irq.h> #include <linux/module.h> @@ -18,81 +19,33 @@ #include <linux/mfd/rohm-bd71815.h> struct bd71815_gpio { - /* chip.parent points the MFD which provides DT node and regmap */ - struct gpio_chip chip; - /* dev points to the platform device for devm and prints */ struct device *dev; - struct regmap *regmap; }; -static int bd71815gpo_get(struct gpio_chip *chip, unsigned int offset) -{ - struct bd71815_gpio *bd71815 = gpiochip_get_data(chip); - int ret, val; - - ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val); - if (ret) - return ret; - - return (val >> offset) & 1; -} - -static void bd71815gpo_set(struct gpio_chip *chip, unsigned int offset, - int value) -{ - struct bd71815_gpio *bd71815 = gpiochip_get_data(chip); - int ret, bit; - - bit = BIT(offset); - - if (value) - ret = regmap_set_bits(bd71815->regmap, BD71815_REG_GPO, bit); - else - ret = regmap_clear_bits(bd71815->regmap, BD71815_REG_GPO, bit); - - if (ret) - dev_warn(bd71815->dev, "failed to toggle GPO\n"); -} - -static int bd71815_gpio_set_config(struct gpio_chip *chip, unsigned int offset, +static int bd71815_gpio_set_config(struct regmap *regmap, void *drvdata, + unsigned int offset, unsigned long config) { - struct bd71815_gpio *bdgpio = gpiochip_get_data(chip); + struct bd71815_gpio *bdgpio = (struct bd71815_gpio *)drvdata; switch (pinconf_to_config_param(config)) { case PIN_CONFIG_DRIVE_OPEN_DRAIN: - return regmap_update_bits(bdgpio->regmap, + return regmap_update_bits(regmap, BD71815_REG_GPO, BD71815_GPIO_DRIVE_MASK << offset, BD71815_GPIO_OPEN_DRAIN << offset); case PIN_CONFIG_DRIVE_PUSH_PULL: - return regmap_update_bits(bdgpio->regmap, + return regmap_update_bits(regmap, BD71815_REG_GPO, BD71815_GPIO_DRIVE_MASK << offset, BD71815_GPIO_CMOS << offset); default: + dev_err(bdgpio->dev, "Unsupported config (0x%lx)\n", config); break; } return -ENOTSUPP; } -/* BD71815 GPIO is actually GPO */ -static int bd71815gpo_direction_get(struct gpio_chip *gc, unsigned int offset) -{ - return GPIO_LINE_DIRECTION_OUT; -} - -/* Template for GPIO chip */ -static const struct gpio_chip bd71815gpo_chip = { - .label = "bd71815", - .owner = THIS_MODULE, - .get = bd71815gpo_get, - .get_direction = bd71815gpo_direction_get, - .set = bd71815gpo_set, - .set_config = bd71815_gpio_set_config, - .can_sleep = true, -}; - #define BD71815_TWO_GPIOS GENMASK(1, 0) #define BD71815_ONE_GPIO BIT(0) @@ -111,14 +64,16 @@ static const struct gpio_chip bd71815gpo_chip = { * but allows using it by providing the DT property * "rohm,enable-hidden-gpo". */ -static int bd71815_init_valid_mask(struct gpio_chip *gc, +static int bd71815_init_valid_mask(struct regmap *regmap, void *drvdata, unsigned long *valid_mask, unsigned int ngpios) { + struct bd71815_gpio *bdgpio = (struct bd71815_gpio *)drvdata; + if (ngpios != 2) return 0; - if (gc->parent && device_property_present(gc->parent, + if (bdgpio->dev && device_property_present(bdgpio->dev->parent, "rohm,enable-hidden-gpo")) *valid_mask = BD71815_TWO_GPIOS; else @@ -127,9 +82,19 @@ static int bd71815_init_valid_mask(struct gpio_chip *gc, return 0; } +/* Template for regmap gpio config */ +static const struct gpio_regmap_config gpio_cfg_template = { + .label = "bd71815", + .reg_set_base = BD71815_REG_GPO, + .ngpio = 2, + .set_config = bd71815_gpio_set_config, + .init_valid_mask = bd71815_init_valid_mask, +}; + static int gpo_bd71815_probe(struct platform_device *pdev) { struct bd71815_gpio *g; + struct gpio_regmap_config cfg; struct device *parent, *dev; /* @@ -144,30 +109,15 @@ static int gpo_bd71815_probe(struct platform_device *pdev) if (!g) return -ENOMEM; - g->chip = bd71815gpo_chip; - - /* - * FIXME: As writing of this the sysfs interface for GPIO control does - * not respect the valid_mask. Do not trust it but rather set the ngpios - * to 1 if "rohm,enable-hidden-gpo" is not given. - * - * This check can be removed later if the sysfs export is fixed and - * if the fix is backported. - * - * For now it is safest to just set the ngpios though. - */ - if (device_property_present(parent, "rohm,enable-hidden-gpo")) - g->chip.ngpio = 2; - else - g->chip.ngpio = 1; - - g->chip.init_valid_mask = bd71815_init_valid_mask; - g->chip.base = -1; - g->chip.parent = parent; - g->regmap = dev_get_regmap(parent, NULL); g->dev = dev; - return devm_gpiochip_add_data(dev, &g->chip, g); + cfg = gpio_cfg_template; + cfg.parent = parent; + cfg.regmap = dev_get_regmap(parent, NULL); + cfg.fwnode = dev_fwnode(dev); + cfg.drvdata = g; + + return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &cfg)); } static struct platform_driver gpo_bd71815_driver = { -- 2.25.4 -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gpio: bd71815: Use gpio-regmap 2021-05-20 11:29 ` [PATCH 2/2] gpio: bd71815: Use gpio-regmap Matti Vaittinen @ 2021-05-25 15:51 ` Linus Walleij 2021-05-26 5:40 ` Vaittinen, Matti 0 siblings, 1 reply; 10+ messages in thread From: Linus Walleij @ 2021-05-25 15:51 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Bartosz Golaszewski, Michael Walle, open list:GPIO SUBSYSTEM, linux-kernel, linux-power On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> wrote: > Utilize the gpio-regmap helper and drop the custom functions > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Very nice! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gpio: bd71815: Use gpio-regmap 2021-05-25 15:51 ` Linus Walleij @ 2021-05-26 5:40 ` Vaittinen, Matti 0 siblings, 0 replies; 10+ messages in thread From: Vaittinen, Matti @ 2021-05-26 5:40 UTC (permalink / raw) To: linus.walleij Cc: linux-power, linux-gpio, bgolaszewski, michael, linux-kernel On Tue, 2021-05-25 at 17:51 +0200, Linus Walleij wrote: > On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > Utilize the gpio-regmap helper and drop the custom functions > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Very nice! > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thanks for the review Linus! Unfortunately I messed up this as I forgot the cover-letter. I am sorry if this impacted to your checks for updated patch versions. I've sent couple of new versions after this one. I drafted and sent v2 but was not completely happy with it. Yesterday I sent v3 which I think did the "right thing" - but it appears I had a brain-fart as I forgot to convert both of the other gpio_regmap users.. So, I am hopefully sending out the v4 soon(ish). Best Regards Matti Vaittinen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 11:28 [PATCH 1/2] gpio: regmap: Support few IC specific operations Matti Vaittinen 2021-05-20 11:29 ` [PATCH 2/2] gpio: bd71815: Use gpio-regmap Matti Vaittinen @ 2021-05-20 11:39 ` Vaittinen, Matti 2021-05-20 11:42 ` Michael Walle 2 siblings, 0 replies; 10+ messages in thread From: Vaittinen, Matti @ 2021-05-20 11:39 UTC (permalink / raw) To: Vaittinen, Matti Cc: linux-power, linux-gpio, bgolaszewski, michael, linux-kernel, linus.walleij [-- Attachment #1: Type: text/plain, Size: 797 bytes --] On Thu, 2021-05-20 at 14:28 +0300, Matti Vaittinen wrote: > The set_config and init_valid_mask GPIO operations are usually very > IC > specific. Allow IC drivers to provide these custom operations at > gpio-regmap registration. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Ouch. Immediately after sending this I noticed that I omitted the cover-letter. Sorry folks. In a nutshell - idea is to support providing some IC specific operations at gpio_regmap registration. This should help broaden the gpio-regmap IC coverage without the need of exposing the gpio_chip. Some preliminary discussion can be seen here: https://lore.kernel.org/linux-gpio/c4faac648d3e0c7f3dcb50f7e24c8b322e8c6974.camel@fi.rohmeurope.com/ Best Regards Matti Vaittinen [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 11:28 [PATCH 1/2] gpio: regmap: Support few IC specific operations Matti Vaittinen 2021-05-20 11:29 ` [PATCH 2/2] gpio: bd71815: Use gpio-regmap Matti Vaittinen 2021-05-20 11:39 ` [PATCH 1/2] gpio: regmap: Support few IC specific operations Vaittinen, Matti @ 2021-05-20 11:42 ` Michael Walle 2021-05-20 12:00 ` Matti Vaittinen 2 siblings, 1 reply; 10+ messages in thread From: Michael Walle @ 2021-05-20 11:42 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel, linux-power Hi Matti, Am 2021-05-20 13:28, schrieb Matti Vaittinen: > The set_config and init_valid_mask GPIO operations are usually very IC > specific. Allow IC drivers to provide these custom operations at > gpio-regmap registration. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > --- > drivers/gpio/gpio-regmap.c | 49 +++++++++++++++++++++++++++++++++++++ > include/linux/gpio/regmap.h | 13 ++++++++++ > 2 files changed, 62 insertions(+) > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c > index 134cedf151a7..315285cacd3f 100644 > --- a/drivers/gpio/gpio-regmap.c > +++ b/drivers/gpio/gpio-regmap.c > @@ -27,6 +27,10 @@ struct gpio_regmap { > int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, > unsigned int offset, unsigned int *reg, > unsigned int *mask); > + int (*set_config)(struct regmap *regmap, void *drvdata, > + unsigned int offset, unsigned long config); > + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, > + unsigned long *valid_mask, unsigned int ngpios); Maybe we should also make the first argument a "struct gpio_regmap" and provide a new gpio_regmap_get_regmap(struct gpio_regmap). Thus having a similar api as for the reg_mask_xlate(). Andy? > > void *driver_data; > }; > @@ -39,6 +43,43 @@ static unsigned int gpio_regmap_addr(unsigned int > addr) > return addr; > } > > +static int regmap_gpio_init_valid_mask(struct gpio_chip *gc, > + unsigned long *valid_mask, > + unsigned int ngpios) > +{ > + struct gpio_regmap *gpio; > + void *drvdata; > + > + gpio = gpiochip_get_data(gc); > + > + if (!gpio->init_valid_mask) { > + WARN_ON(!gpio->init_valid_mask); > + return -EINVAL; > + } Why not the following? if (!gpio->init_valid_mask) return 0; Thus copying the behavior of gpiolib. > + > + drvdata = gpio_regmap_get_drvdata(gpio); > + > + return gpio->init_valid_mask(gpio->regmap, drvdata, valid_mask, > ngpios); > +} > + > +static int gpio_regmap_set_config(struct gpio_chip *gc, unsigned int > offset, > + unsigned long config) > +{ > + struct gpio_regmap *gpio; > + void *drvdata; > + > + gpio = gpiochip_get_data(gc); > + > + if (!gpio->set_config) { > + WARN_ON(!gpio->set_config); > + return -EINVAL; > + } same here, return -ENOTSUPP. > + > + drvdata = gpio_regmap_get_drvdata(gpio); > + > + return gpio->set_config(gpio->regmap, drvdata, offset, config); > +} > + > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, > unsigned int base, unsigned int offset, > unsigned int *reg, unsigned int *mask) > @@ -235,6 +276,8 @@ struct gpio_regmap *gpio_regmap_register(const > struct gpio_regmap_config *config > gpio->reg_clr_base = config->reg_clr_base; > gpio->reg_dir_in_base = config->reg_dir_in_base; > gpio->reg_dir_out_base = config->reg_dir_out_base; > + gpio->set_config = config->set_config; > + gpio->init_valid_mask = config->init_valid_mask; > > /* if not set, assume there is only one register */ > if (!gpio->ngpio_per_reg) > @@ -253,6 +296,10 @@ struct gpio_regmap *gpio_regmap_register(const > struct gpio_regmap_config *config > chip->ngpio = config->ngpio; > chip->names = config->names; > chip->label = config->label ?: dev_name(config->parent); > + if (gpio->set_config) > + chip->set_config = gpio_regmap_set_config; > + if (gpio->init_valid_mask) > + chip->init_valid_mask = regmap_gpio_init_valid_mask; > > #if defined(CONFIG_OF_GPIO) > /* gpiolib will use of_node of the parent if chip->of_node is NULL */ > @@ -280,6 +327,8 @@ struct gpio_regmap *gpio_regmap_register(const > struct gpio_regmap_config *config > chip->direction_output = gpio_regmap_direction_output; > } > > + gpio_regmap_set_drvdata(gpio, config->drvdata); I'm wondering if we need the gpio_regmap_set_drvdata() anymore or if we can just drop it entirely. > + > ret = gpiochip_add_data(chip, gpio); > if (ret < 0) > goto err_free_gpio; > diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h > index 334dd928042b..c382a3caefc3 100644 > --- a/include/linux/gpio/regmap.h > +++ b/include/linux/gpio/regmap.h > @@ -33,10 +33,18 @@ struct regmap; > * @ngpio_per_reg: Number of GPIOs per register > * @irq_domain: (Optional) IRQ domain if the controller is > * interrupt-capable > + * @drvdata: (Optional) Pointer to IC specific data which is > + * not used by gpio-remap but is provided "as is" to > + * the driver callback(s). > + * > * @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. > + * @set_config: (Optional) hook for all kinds of settings. Uses > + * the same packed config format as generic pinconf. > + * @init_valid_mask: (Optional) routine to initialize @valid_mask, to > + * be used if not all GPIOs are valid. > * > * The ->reg_mask_xlate translates a given base address and GPIO > offset to > * register and mask pair. The base address is one of the given > register > @@ -74,10 +82,15 @@ struct gpio_regmap_config { > int reg_stride; > int ngpio_per_reg; > struct irq_domain *irq_domain; > + void *drvdata; > > int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, > unsigned int offset, unsigned int *reg, > unsigned int *mask); > + int (*set_config)(struct regmap *regmap, void *drvdata, > + unsigned int offset, unsigned long config); > + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, > + unsigned long *valid_mask, unsigned int ngpios); > }; > > struct gpio_regmap *gpio_regmap_register(const struct > gpio_regmap_config *config); > > base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc > -- > 2.25.4 -michael ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 11:42 ` Michael Walle @ 2021-05-20 12:00 ` Matti Vaittinen 2021-05-20 12:22 ` Michael Walle 0 siblings, 1 reply; 10+ messages in thread From: Matti Vaittinen @ 2021-05-20 12:00 UTC (permalink / raw) To: Michael Walle Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel, linux-power On Thu, 2021-05-20 at 13:42 +0200, Michael Walle wrote: > Hi Matti, > > Am 2021-05-20 13:28, schrieb Matti Vaittinen: > > The set_config and init_valid_mask GPIO operations are usually very > > IC > > specific. Allow IC drivers to provide these custom operations at > > gpio-regmap registration. > > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/gpio/gpio-regmap.c | 49 > > +++++++++++++++++++++++++++++++++++++ > > include/linux/gpio/regmap.h | 13 ++++++++++ > > 2 files changed, 62 insertions(+) > > > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio- > > regmap.c > > index 134cedf151a7..315285cacd3f 100644 > > --- a/drivers/gpio/gpio-regmap.c > > +++ b/drivers/gpio/gpio-regmap.c > > @@ -27,6 +27,10 @@ struct gpio_regmap { > > int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int > > base, > > unsigned int offset, unsigned int *reg, > > unsigned int *mask); > > + int (*set_config)(struct regmap *regmap, void *drvdata, > > + unsigned int offset, unsigned long config); > > + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, > > + unsigned long *valid_mask, unsigned int > > ngpios); > > Maybe we should also make the first argument a "struct gpio_regmap" > and provide a new gpio_regmap_get_regmap(struct gpio_regmap). Thus > having a similar api as for the reg_mask_xlate(). Andy? I don't really see the reason of making this any more complicated for IC drivers. If we don't open the struct gpio_regmap to IC drivers - then they never need the struct gpio_regmap pointer itself but each IC driver would need to do some unnecessary function call (gpio_regmap_get_regmap() in this case). I'd say that would be unnecessary bloat. > > > void *driver_data; > > }; > > @@ -39,6 +43,43 @@ static unsigned int gpio_regmap_addr(unsigned > > int > > addr) > > return addr; > > } > > > > +static int regmap_gpio_init_valid_mask(struct gpio_chip *gc, > > + unsigned long *valid_mask, > > + unsigned int ngpios) > > +{ > > + struct gpio_regmap *gpio; > > + void *drvdata; > > + > > + gpio = gpiochip_get_data(gc); > > + > > + if (!gpio->init_valid_mask) { > > + WARN_ON(!gpio->init_valid_mask); > > + return -EINVAL; > > + } > > Why not the following? > > if (!gpio->init_valid_mask) > return 0; It just feels like an error if regmap_gpio_init_valid_mask() is ever called by core without having the gpio->init_valid_mask set. Probably this would mean that the someone has errorneously modified the gpio- >init_valid_mask set after gpio_regmap registration - whih smells like a problem. Thus the WARN() sounds like a correct course of action to me. (I may be wrong though - see below) > Thus copying the behavior of gpiolib. I must admit I didn't check how this is implemented in gpiolib. But the gpio_chip's init_valid_mask should not be set if regmap_gpio_config does not have valid init_valid_mask pointer at registration. Thus it smells like an error to me if the GPIO core calls the regmap_gpio_init_valid_mask() and regmap_gpio has not set the init_valid_mask pointer. But as I said, I haven't looked in gpiolib for this so I may be wrong. > > > + > > + drvdata = gpio_regmap_get_drvdata(gpio); > > + > > + return gpio->init_valid_mask(gpio->regmap, drvdata, > > valid_mask, > > ngpios); > > +} > > + > > +static int gpio_regmap_set_config(struct gpio_chip *gc, unsigned > > int > > offset, > > + unsigned long config) > > +{ > > + struct gpio_regmap *gpio; > > + void *drvdata; > > + > > + gpio = gpiochip_get_data(gc); > > + > > + if (!gpio->set_config) { > > + WARN_ON(!gpio->set_config); > > + return -EINVAL; > > + } > > same here, return -ENOTSUPP. As above - if (!gpio->set_config) { the gpio-core should never call gpio_regmap_set_config() if the } Maybe I should add a comment to clarify the WARN() ? > > > + > > + drvdata = gpio_regmap_get_drvdata(gpio); > > + > > + return gpio->set_config(gpio->regmap, drvdata, offset, config); > > +} > > + > > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, > > unsigned int base, unsigned int > > offset, > > unsigned int *reg, unsigned int > > *mask) > > @@ -235,6 +276,8 @@ struct gpio_regmap *gpio_regmap_register(const > > struct gpio_regmap_config *config > > gpio->reg_clr_base = config->reg_clr_base; > > gpio->reg_dir_in_base = config->reg_dir_in_base; > > gpio->reg_dir_out_base = config->reg_dir_out_base; > > + gpio->set_config = config->set_config; > > + gpio->init_valid_mask = config->init_valid_mask; > > > > /* if not set, assume there is only one register */ > > if (!gpio->ngpio_per_reg) > > @@ -253,6 +296,10 @@ struct gpio_regmap *gpio_regmap_register(const > > struct gpio_regmap_config *config > > chip->ngpio = config->ngpio; > > chip->names = config->names; > > chip->label = config->label ?: dev_name(config->parent); > > + if (gpio->set_config) > > + chip->set_config = gpio_regmap_set_config; > > + if (gpio->init_valid_mask) > > + chip->init_valid_mask = regmap_gpio_init_valid_mask; > > > > #if defined(CONFIG_OF_GPIO) > > /* gpiolib will use of_node of the parent if chip->of_node is > > NULL */ > > @@ -280,6 +327,8 @@ struct gpio_regmap *gpio_regmap_register(const > > struct gpio_regmap_config *config > > chip->direction_output = gpio_regmap_direction_output; > > } > > > > + gpio_regmap_set_drvdata(gpio, config->drvdata); > > I'm wondering if we need the gpio_regmap_set_drvdata() anymore or if > we can just drop it entirely. I wouldn't drop it. I think there _may_ be cases where the drvdata is set only after the registration. (Just my gut-feeling, I may be wrong though) Best Regards Matti Vaittinen -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 12:00 ` Matti Vaittinen @ 2021-05-20 12:22 ` Michael Walle 2021-05-20 12:42 ` Vaittinen, Matti 0 siblings, 1 reply; 10+ messages in thread From: Michael Walle @ 2021-05-20 12:22 UTC (permalink / raw) To: matti.vaittinen Cc: Linus Walleij, Bartosz Golaszewski, linux-gpio, linux-kernel, linux-power Am 2021-05-20 14:00, schrieb Matti Vaittinen: > On Thu, 2021-05-20 at 13:42 +0200, Michael Walle wrote: >> Am 2021-05-20 13:28, schrieb Matti Vaittinen: >> > The set_config and init_valid_mask GPIO operations are usually very >> > IC >> > specific. Allow IC drivers to provide these custom operations at >> > gpio-regmap registration. >> > >> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> >> > --- >> > drivers/gpio/gpio-regmap.c | 49 >> > +++++++++++++++++++++++++++++++++++++ >> > include/linux/gpio/regmap.h | 13 ++++++++++ >> > 2 files changed, 62 insertions(+) >> > >> > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio- >> > regmap.c >> > index 134cedf151a7..315285cacd3f 100644 >> > --- a/drivers/gpio/gpio-regmap.c >> > +++ b/drivers/gpio/gpio-regmap.c >> > @@ -27,6 +27,10 @@ struct gpio_regmap { >> > int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int >> > base, >> > unsigned int offset, unsigned int *reg, >> > unsigned int *mask); >> > + int (*set_config)(struct regmap *regmap, void *drvdata, >> > + unsigned int offset, unsigned long config); >> > + int (*init_valid_mask)(struct regmap *regmap, void *drvdata, >> > + unsigned long *valid_mask, unsigned int >> > ngpios); >> >> Maybe we should also make the first argument a "struct gpio_regmap" >> and provide a new gpio_regmap_get_regmap(struct gpio_regmap). Thus >> having a similar api as for the reg_mask_xlate(). Andy? > > I don't really see the reason of making this any more complicated for > IC drivers. If we don't open the struct gpio_regmap to IC drivers - > then they never need the struct gpio_regmap pointer itself but each IC > driver would need to do some unnecessary function call > (gpio_regmap_get_regmap() in this case). I'd say that would be > unnecessary bloat. If there is ever the need of additional parameters, you'll have to modify that parameter list. Otherwise you'll just have to add a new function. Thus might be more future proof. But I won't object to it. >> > void *driver_data; >> > }; >> > @@ -39,6 +43,43 @@ static unsigned int gpio_regmap_addr(unsigned >> > int >> > addr) >> > return addr; >> > } >> > >> > +static int regmap_gpio_init_valid_mask(struct gpio_chip *gc, >> > + unsigned long *valid_mask, >> > + unsigned int ngpios) >> > +{ >> > + struct gpio_regmap *gpio; >> > + void *drvdata; >> > + >> > + gpio = gpiochip_get_data(gc); >> > + >> > + if (!gpio->init_valid_mask) { >> > + WARN_ON(!gpio->init_valid_mask); >> > + return -EINVAL; >> > + } >> >> Why not the following? >> >> if (!gpio->init_valid_mask) >> return 0; > > It just feels like an error if regmap_gpio_init_valid_mask() is ever > called by core without having the gpio->init_valid_mask set. Probably > this would mean that the someone has errorneously modified the gpio- >> init_valid_mask set after gpio_regmap registration - whih smells like > a problem. Thus the WARN() sounds like a correct course of action to > me. (I may be wrong though - see below) > >> Thus copying the behavior of gpiolib. > > I must admit I didn't check how this is implemented in gpiolib. But the > gpio_chip's init_valid_mask should not be set if regmap_gpio_config > does not have valid init_valid_mask pointer at registration. Thus it > smells like an error to me if the GPIO core calls the > regmap_gpio_init_valid_mask() and regmap_gpio has not set the > init_valid_mask pointer. But as I said, I haven't looked in gpiolib for > this so I may be wrong. Oh, I missed that you only set it when it is set in the gpio_regmap_config. Thus, I'd say drop it entirely? It is only within this module where things might go wrong. >> > + >> > + drvdata = gpio_regmap_get_drvdata(gpio); >> > + >> > + return gpio->init_valid_mask(gpio->regmap, drvdata, >> > valid_mask, >> > ngpios); >> > +} >> > + >> > +static int gpio_regmap_set_config(struct gpio_chip *gc, unsigned >> > int >> > offset, >> > + unsigned long config) >> > +{ >> > + struct gpio_regmap *gpio; >> > + void *drvdata; >> > + >> > + gpio = gpiochip_get_data(gc); >> > + >> > + if (!gpio->set_config) { >> > + WARN_ON(!gpio->set_config); >> > + return -EINVAL; >> > + } >> >> same here, return -ENOTSUPP. > > As above - > if (!gpio->set_config) { > the gpio-core should never call gpio_regmap_set_config() if the > } > > Maybe I should add a comment to clarify the WARN() ? >> >> > + >> > + drvdata = gpio_regmap_get_drvdata(gpio); >> > + >> > + return gpio->set_config(gpio->regmap, drvdata, offset, config); >> > +} >> > + >> > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, >> > unsigned int base, unsigned int >> > offset, >> > unsigned int *reg, unsigned int >> > *mask) >> > @@ -235,6 +276,8 @@ struct gpio_regmap *gpio_regmap_register(const >> > struct gpio_regmap_config *config >> > gpio->reg_clr_base = config->reg_clr_base; >> > gpio->reg_dir_in_base = config->reg_dir_in_base; >> > gpio->reg_dir_out_base = config->reg_dir_out_base; >> > + gpio->set_config = config->set_config; >> > + gpio->init_valid_mask = config->init_valid_mask; >> > >> > /* if not set, assume there is only one register */ >> > if (!gpio->ngpio_per_reg) >> > @@ -253,6 +296,10 @@ struct gpio_regmap *gpio_regmap_register(const >> > struct gpio_regmap_config *config >> > chip->ngpio = config->ngpio; >> > chip->names = config->names; >> > chip->label = config->label ?: dev_name(config->parent); >> > + if (gpio->set_config) >> > + chip->set_config = gpio_regmap_set_config; >> > + if (gpio->init_valid_mask) >> > + chip->init_valid_mask = regmap_gpio_init_valid_mask; >> > >> > #if defined(CONFIG_OF_GPIO) >> > /* gpiolib will use of_node of the parent if chip->of_node is >> > NULL */ >> > @@ -280,6 +327,8 @@ struct gpio_regmap *gpio_regmap_register(const >> > struct gpio_regmap_config *config >> > chip->direction_output = gpio_regmap_direction_output; >> > } >> > >> > + gpio_regmap_set_drvdata(gpio, config->drvdata); >> >> I'm wondering if we need the gpio_regmap_set_drvdata() anymore or if >> we can just drop it entirely. > > I wouldn't drop it. I think there _may_ be cases where the drvdata is > set only after the registration. (Just my gut-feeling, I may be wrong > though) Ok, but as you already mentioned on IRC, it could be a bit of a trap because it might already be used after gpiochip_add_data() and thus be NULL if not provided by gpio_regmap_config(). -michael ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 12:22 ` Michael Walle @ 2021-05-20 12:42 ` Vaittinen, Matti 2021-05-21 7:10 ` Michael Walle 0 siblings, 1 reply; 10+ messages in thread From: Vaittinen, Matti @ 2021-05-20 12:42 UTC (permalink / raw) To: michael Cc: linux-power, linux-gpio, bgolaszewski, linux-kernel, linus.walleij On Thu, 2021-05-20 at 14:22 +0200, Michael Walle wrote: > Am 2021-05-20 14:00, schrieb Matti Vaittinen: > > On Thu, 2021-05-20 at 13:42 +0200, Michael Walle wrote: > > > Am 2021-05-20 13:28, schrieb Matti Vaittinen: > > > > The set_config and init_valid_mask GPIO operations are usually > > > > very > > > > IC > > > > specific. Allow IC drivers to provide these custom operations > > > > at > > > > gpio-regmap registration. > > > > > > > > Signed-off-by: Matti Vaittinen < > > > > matti.vaittinen@fi.rohmeurope.com> > > > > --- > > > > drivers/gpio/gpio-regmap.c | 49 > > > > +++++++++++++++++++++++++++++++++++++ > > > > include/linux/gpio/regmap.h | 13 ++++++++++ > > > > 2 files changed, 62 insertions(+) > > > > > > > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio- > > > > regmap.c > > > > index 134cedf151a7..315285cacd3f 100644 > > > > --- a/drivers/gpio/gpio-regmap.c > > > > +++ b/drivers/gpio/gpio-regmap.c > > > > @@ -27,6 +27,10 @@ struct gpio_regmap { > > > > int (*reg_mask_xlate)(struct gpio_regmap *gpio, > > > > unsigned int > > > > base, > > > > unsigned int offset, unsigned int > > > > *reg, > > > > unsigned int *mask); > > > > + int (*set_config)(struct regmap *regmap, void *drvdata, > > > > + unsigned int offset, unsigned long > > > > config); > > > > + int (*init_valid_mask)(struct regmap *regmap, void > > > > *drvdata, > > > > + unsigned long *valid_mask, > > > > unsigned int > > > > ngpios); > > > > > > Maybe we should also make the first argument a "struct > > > gpio_regmap" > > > and provide a new gpio_regmap_get_regmap(struct gpio_regmap). > > > Thus > > > having a similar api as for the reg_mask_xlate(). Andy? > > > > I don't really see the reason of making this any more complicated > > for > > IC drivers. If we don't open the struct gpio_regmap to IC drivers - > > then they never need the struct gpio_regmap pointer itself but each > > IC > > driver would need to do some unnecessary function call > > (gpio_regmap_get_regmap() in this case). I'd say that would be > > unnecessary bloat. > > If there is ever the need of additional parameters, you'll have to > modify that parameter list. Otherwise you'll just have to add a new > function. Thus might be more future proof. I do hope the "void *drvdata" allows enough flexibility so that there is no need to add new parameters. And if there is, then I don't see how the struct gpio_regmap pointer would have saved us - unless we open the contents of struct gpio_regmap to IC drivers. (Which might make sense because that already contains plenty of register details which may need to be duplicated to drvdata for some IC-specific callbacks. Here we again have analogy to regulator_desc - which I have often used also in IC-specific custom callbacks. But as long as we hope to keep the struct gpio_regmap private I would not add it in arguments). > But I won't object to it. > > > > @@ -39,6 +43,43 @@ static unsigned int > > > > gpio_regmap_addr(unsigned > > > > int > > > > addr) > > > > return addr; > > > > } > > > > > > > > +static int regmap_gpio_init_valid_mask(struct gpio_chip *gc, > > > > + unsigned long > > > > *valid_mask, > > > > + unsigned int ngpios) > > > > +{ > > > > + struct gpio_regmap *gpio; > > > > + void *drvdata; > > > > + > > > > + gpio = gpiochip_get_data(gc); > > > > + > > > > + if (!gpio->init_valid_mask) { > > > > + WARN_ON(!gpio->init_valid_mask); > > > > + return -EINVAL; > > > > + } > > > > > > Why not the following? > > > > > > if (!gpio->init_valid_mask) > > > return 0; > > > > It just feels like an error if regmap_gpio_init_valid_mask() is > > ever > > called by core without having the gpio->init_valid_mask set. > > Probably > > this would mean that the someone has errorneously modified the > > gpio- > > > init_valid_mask set after gpio_regmap registration - whih smells > > > like > > a problem. Thus the WARN() sounds like a correct course of action > > to > > me. (I may be wrong though - see below) > > > > > Thus copying the behavior of gpiolib. > > > > I must admit I didn't check how this is implemented in gpiolib. But > > the > > gpio_chip's init_valid_mask should not be set if regmap_gpio_config > > does not have valid init_valid_mask pointer at registration. Thus > > it > > smells like an error to me if the GPIO core calls the > > regmap_gpio_init_valid_mask() and regmap_gpio has not set the > > init_valid_mask pointer. But as I said, I haven't looked in gpiolib > > for > > this so I may be wrong. > > Oh, I missed that you only set it when it is set in the > gpio_regmap_config. Thus, I'd say drop it entirely? It is only within > this module where things might go wrong. I have no strong opinion on this. I can drop it if it's not needed. > > > > + > > > > + drvdata = gpio_regmap_get_drvdata(gpio); > > > > + > > > > + return gpio->init_valid_mask(gpio->regmap, drvdata, > > > > valid_mask, > > > > ngpios); > > > > +} > > > > + > > > > +static int gpio_regmap_set_config(struct gpio_chip *gc, > > > > unsigned > > > > int > > > > offset, > > > > + unsigned long config) > > > > +{ > > > > + struct gpio_regmap *gpio; > > > > + void *drvdata; > > > > + > > > > + gpio = gpiochip_get_data(gc); > > > > + > > > > + if (!gpio->set_config) { > > > > + WARN_ON(!gpio->set_config); > > > > + return -EINVAL; > > > > + } > > > > > > same here, return -ENOTSUPP. > > > > As above - > > if (!gpio->set_config) { > > the gpio-core should never call gpio_regmap_set_config() if the > > } > > > > Maybe I should add a comment to clarify the WARN() ? > > > > + > > > > + drvdata = gpio_regmap_get_drvdata(gpio); > > > > + > > > > + return gpio->set_config(gpio->regmap, drvdata, offset, > > > > config); > > > > +} > > > > + > > > > static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, > > > > unsigned int base, unsigned > > > > int > > > > offset, > > > > unsigned int *reg, unsigned > > > > int > > > > *mask) > > > > @@ -235,6 +276,8 @@ struct gpio_regmap > > > > *gpio_regmap_register(const > > > > struct gpio_regmap_config *config > > > > gpio->reg_clr_base = config->reg_clr_base; > > > > gpio->reg_dir_in_base = config->reg_dir_in_base; > > > > gpio->reg_dir_out_base = config->reg_dir_out_base; > > > > + gpio->set_config = config->set_config; > > > > + gpio->init_valid_mask = config->init_valid_mask; > > > > > > > > /* if not set, assume there is only one register */ > > > > if (!gpio->ngpio_per_reg) > > > > @@ -253,6 +296,10 @@ struct gpio_regmap > > > > *gpio_regmap_register(const > > > > struct gpio_regmap_config *config > > > > chip->ngpio = config->ngpio; > > > > chip->names = config->names; > > > > chip->label = config->label ?: dev_name(config- > > > > > parent); > > > > + if (gpio->set_config) > > > > + chip->set_config = gpio_regmap_set_config; > > > > + if (gpio->init_valid_mask) > > > > + chip->init_valid_mask = > > > > regmap_gpio_init_valid_mask; > > > > > > > > #if defined(CONFIG_OF_GPIO) > > > > /* gpiolib will use of_node of the parent if chip- > > > > > of_node is > > > > NULL */ > > > > @@ -280,6 +327,8 @@ struct gpio_regmap > > > > *gpio_regmap_register(const > > > > struct gpio_regmap_config *config > > > > chip->direction_output = > > > > gpio_regmap_direction_output; > > > > } > > > > > > > > + gpio_regmap_set_drvdata(gpio, config->drvdata); > > > > > > I'm wondering if we need the gpio_regmap_set_drvdata() anymore or > > > if > > > we can just drop it entirely. > > > > I wouldn't drop it. I think there _may_ be cases where the drvdata > > is > > set only after the registration. (Just my gut-feeling, I may be > > wrong > > though) > > Ok, but as you already mentioned on IRC, it could be a bit of a trap > because it might already be used after gpiochip_add_data() and thus > be NULL if not provided by gpio_regmap_config(). Hmm.. I think you are right. Setting the drvdata after registration is a call for a race. After that reasoning I agree with you that this should be dropped. Best Regards Matti Vaittinen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations 2021-05-20 12:42 ` Vaittinen, Matti @ 2021-05-21 7:10 ` Michael Walle 0 siblings, 0 replies; 10+ messages in thread From: Michael Walle @ 2021-05-21 7:10 UTC (permalink / raw) To: Vaittinen, Matti Cc: linux-power, linux-gpio, bgolaszewski, linux-kernel, linus.walleij Am 2021-05-20 14:42, schrieb Vaittinen, Matti: > On Thu, 2021-05-20 at 14:22 +0200, Michael Walle wrote: >> Am 2021-05-20 14:00, schrieb Matti Vaittinen: >> > On Thu, 2021-05-20 at 13:42 +0200, Michael Walle wrote: >> > > Am 2021-05-20 13:28, schrieb Matti Vaittinen: >> > > > The set_config and init_valid_mask GPIO operations are usually >> > > > very >> > > > IC >> > > > specific. Allow IC drivers to provide these custom operations >> > > > at >> > > > gpio-regmap registration. >> > > > >> > > > Signed-off-by: Matti Vaittinen < >> > > > matti.vaittinen@fi.rohmeurope.com> >> > > > --- >> > > > drivers/gpio/gpio-regmap.c | 49 >> > > > +++++++++++++++++++++++++++++++++++++ >> > > > include/linux/gpio/regmap.h | 13 ++++++++++ >> > > > 2 files changed, 62 insertions(+) >> > > > >> > > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio- >> > > > regmap.c >> > > > index 134cedf151a7..315285cacd3f 100644 >> > > > --- a/drivers/gpio/gpio-regmap.c >> > > > +++ b/drivers/gpio/gpio-regmap.c >> > > > @@ -27,6 +27,10 @@ struct gpio_regmap { >> > > > int (*reg_mask_xlate)(struct gpio_regmap *gpio, >> > > > unsigned int >> > > > base, >> > > > unsigned int offset, unsigned int >> > > > *reg, >> > > > unsigned int *mask); >> > > > + int (*set_config)(struct regmap *regmap, void *drvdata, >> > > > + unsigned int offset, unsigned long >> > > > config); >> > > > + int (*init_valid_mask)(struct regmap *regmap, void >> > > > *drvdata, >> > > > + unsigned long *valid_mask, >> > > > unsigned int >> > > > ngpios); >> > > >> > > Maybe we should also make the first argument a "struct >> > > gpio_regmap" >> > > and provide a new gpio_regmap_get_regmap(struct gpio_regmap). >> > > Thus >> > > having a similar api as for the reg_mask_xlate(). Andy? >> > >> > I don't really see the reason of making this any more complicated >> > for >> > IC drivers. If we don't open the struct gpio_regmap to IC drivers - >> > then they never need the struct gpio_regmap pointer itself but each >> > IC >> > driver would need to do some unnecessary function call >> > (gpio_regmap_get_regmap() in this case). I'd say that would be >> > unnecessary bloat. >> >> If there is ever the need of additional parameters, you'll have to >> modify that parameter list. Otherwise you'll just have to add a new >> function. Thus might be more future proof. > > I do hope the "void *drvdata" allows enough flexibility so that there > is no need to add new parameters. Thats for information passed from the user of gpio_regmap to the callbacks. > And if there is, then I don't see how > the struct gpio_regmap pointer would have saved us - unless we open the > contents of struct gpio_regmap to IC drivers. (Which might make sense > because that already contains plenty of register details which may need > to be duplicated to drvdata for some IC-specific callbacks. Here we > again have analogy to regulator_desc - which I have often used also in > IC-specific custom callbacks. But as long as we hope to keep the struct > gpio_regmap private I would not add it in arguments). Because that (opaque) argument is then used for the helper functions of gpio_regmap. -michael ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-05-26 5:40 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-20 11:28 [PATCH 1/2] gpio: regmap: Support few IC specific operations Matti Vaittinen 2021-05-20 11:29 ` [PATCH 2/2] gpio: bd71815: Use gpio-regmap Matti Vaittinen 2021-05-25 15:51 ` Linus Walleij 2021-05-26 5:40 ` Vaittinen, Matti 2021-05-20 11:39 ` [PATCH 1/2] gpio: regmap: Support few IC specific operations Vaittinen, Matti 2021-05-20 11:42 ` Michael Walle 2021-05-20 12:00 ` Matti Vaittinen 2021-05-20 12:22 ` Michael Walle 2021-05-20 12:42 ` Vaittinen, Matti 2021-05-21 7:10 ` Michael Walle
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.