| Message ID | 09091e75157ea28dcad1605008532016304356a4.1621509932.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] gpio: regmap: Support few IC specific operations | expand |
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
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
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
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);
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(+) base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc