| Message ID | 6e319c22b41747e3911c7a5cad877134cabc9231.1621577204.git.matti.vaittinen@fi.rohmeurope.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: gpio-regmap: Support few custom operations | expand |
Am 2021-05-21 08:27, 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> Reviewed-by: Michael Walle <michael@walle.cc> -michael
On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> 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. Thanks for this. In general (from design p.o.v.) looks good to me, one question below, though. > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > Changelog v2: (based on suggestions by Michael Walle) > - drop gpio_regmap_set_drvdata() But why do we have gpio_regmap_get_drvdata() and why is it different now to the new member handling? > - drop checks and WARN() for pretty much impossible cases
Am 2021-05-21 12:09, schrieb Andy Shevchenko: > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> wrote: >> Changelog v2: (based on suggestions by Michael Walle) >> - drop gpio_regmap_set_drvdata() > > But why do we have gpio_regmap_get_drvdata() and why is it different > now to the new member handling? Eg. the reg_mask_xlate() callback is just passed a "struct gpio_regmap*". If someone needs to access private data there, gpio_regmap_get_drvdata() is used. At least that was its intention. Thus I was also suggesting to use "struct gpio_regmap*" in the newer callbacks. I don't get what you mean by "different to the new member handling"? -michael
On Fri, 2021-05-21 at 13:09 +0300, Andy Shevchenko wrote: > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > <matti.vaittinen@fi.rohmeurope.com> 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. > > Thanks for this. In general (from design p.o.v.) looks good to me, > one > question below, though. > > > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > > --- > > Changelog v2: (based on suggestions by Michael Walle) > > - drop gpio_regmap_set_drvdata() > > But why do we have gpio_regmap_get_drvdata() I was thinking the drivers might still need to get the drvdata. But now I don't see how they could easily get it via this API. Yes, the regmap_gpio struct (which is used to obtain the data via this API) is returned by the registration - but the only place where IC driver could store this is in the drvdata :) So yes, this is next to useless now. Sure this API could be used in probe, after the registration - but there the driver should have access to drvdata and config anyways. I'll drop also the gpio_regmap_get_drvdata() in next version. Thanks for pointing this out. > and why is it different > now to the new member handling? Hmm. I am unsure what you mean by this? > > > - drop checks and WARN() for pretty much impossible cases
On Fri, 2021-05-21 at 12:19 +0200, Michael Walle wrote: > Am 2021-05-21 12:09, schrieb Andy Shevchenko: > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > Changelog v2: (based on suggestions by Michael Walle) > > > - drop gpio_regmap_set_drvdata() > > > > But why do we have gpio_regmap_get_drvdata() and why is it > > different > > now to the new member handling? > > Eg. the reg_mask_xlate() callback is just passed a "struct > gpio_regmap*". > If someone needs to access private data there, > gpio_regmap_get_drvdata() > is used. At least that was its intention. I would help the IC driver here too and just directly provide the drvdata pointer as argument. I don't see much value in providing the regmap_gpio pointer as IC driver can not dereference it. > > Thus I was also suggesting to use "struct gpio_regmap*" in the newer > callbacks. > > I don't get what you mean by "different to the new member handling"? > > -michael
Am 2021-05-21 12:25, schrieb Vaittinen, Matti: > On Fri, 2021-05-21 at 12:19 +0200, Michael Walle wrote: >> Am 2021-05-21 12:09, schrieb Andy Shevchenko: >> > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen >> > <matti.vaittinen@fi.rohmeurope.com> wrote: >> > > Changelog v2: (based on suggestions by Michael Walle) >> > > - drop gpio_regmap_set_drvdata() >> > >> > But why do we have gpio_regmap_get_drvdata() and why is it >> > different >> > now to the new member handling? >> >> Eg. the reg_mask_xlate() callback is just passed a "struct >> gpio_regmap*". >> If someone needs to access private data there, >> gpio_regmap_get_drvdata() >> is used. At least that was its intention. > > I would help the IC driver here too and just directly provide the > drvdata pointer as argument. I don't see much value in providing the > regmap_gpio pointer as IC driver can not dereference it. What is it with the "it's useless if one cannot dereference it"? You're also passing "struct regmap *" which you cannot dereference. It's an opaque pointer you need to pass to gpio_regmap to call a function there. What is the problem with letting gpio_regmap derefence its internal data structure and return the value for you? Adding the drvdata to reg_mask_xlate() highlights my former concern; you need to keep chaning the users to add another parameter. What if xlate() needs the regmap, too? Then you need to change it again. Granted this is a silly example, but you should get my point. It is by far more easy to just add another new gpio_regmap_*(struct gpio_regmap *) function and you don't have to change existing users. Also what if gpio_regmap provides some useful helper function in the future, it will likely need its internal data struct. -michael
On Fri, May 21, 2021 at 1:19 PM Michael Walle <michael@walle.cc> wrote: > > Am 2021-05-21 12:09, schrieb Andy Shevchenko: > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > > <matti.vaittinen@fi.rohmeurope.com> wrote: > >> Changelog v2: (based on suggestions by Michael Walle) > >> - drop gpio_regmap_set_drvdata() > > > > But why do we have gpio_regmap_get_drvdata() and why is it different > > now to the new member handling? > > Eg. the reg_mask_xlate() callback is just passed a "struct > gpio_regmap*". > If someone needs to access private data there, gpio_regmap_get_drvdata() > is used. At least that was its intention. > > Thus I was also suggesting to use "struct gpio_regmap*" in the newer > callbacks. > > I don't get what you mean by "different to the new member handling"? Currently we have a symmetrical API that is getter and setter against a certain field. Now this change drops the setter and introduces some other field somewhere else. Sounds to me: - either this has to be split into two changes with explanation of what's going on - or something odd is happening here which I do not understand.
On Fri, 2021-05-21 at 12:46 +0200, Michael Walle wrote: > Am 2021-05-21 12:25, schrieb Vaittinen, Matti: > > On Fri, 2021-05-21 at 12:19 +0200, Michael Walle wrote: > > > Am 2021-05-21 12:09, schrieb Andy Shevchenko: > > > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > > Changelog v2: (based on suggestions by Michael Walle) > > > > > - drop gpio_regmap_set_drvdata() > > > > > > > > But why do we have gpio_regmap_get_drvdata() and why is it > > > > different > > > > now to the new member handling? > > > > > > Eg. the reg_mask_xlate() callback is just passed a "struct > > > gpio_regmap*". > > > If someone needs to access private data there, > > > gpio_regmap_get_drvdata() > > > is used. At least that was its intention. > > > > I would help the IC driver here too and just directly provide the > > drvdata pointer as argument. I don't see much value in providing > > the > > regmap_gpio pointer as IC driver can not dereference it. > > What is it with the "it's useless if one cannot dereference it"? > You're > also passing "struct regmap *" which you cannot dereference. It's an > opaque pointer you need to pass to gpio_regmap to call a function > there. > > What is the problem with letting gpio_regmap derefence its internal > data > structure and return the value for you? > > Adding the drvdata to reg_mask_xlate() highlights my former concern; > you > need to keep chaning the users to add another parameter. What if > xlate() > needs the regmap, too? Then you need to change it again. Granted this > is > a silly example, but you should get my point. It is by far more easy > to I don't think it's a silly example. What I am not liking here is always adding a dummy function for getting a regmap_gpio members which are likely to be needed by all users. (drvdata, regmap). It makes IC drivers more complicated when they need to call a function just to get a member value. Sure this adds some flexibility what comes to potential changes - but it only is beneficial if the changes ever happen. Else these getters and setters are just boilerplate. > just add another new gpio_regmap_*(struct gpio_regmap *) function and > you don't have to change existing users. The regmap is not a problem as it is in any case coming to the gpio- regamp from the IC driver. The IC driver can store regmap in drvdata. (Sure it would be then stored in two places - gpio_regmap and drvdata). > Also what if gpio_regmap provides some useful helper function in the > future, it will likely need its internal data struct. That is is a valid point. As I said, I just don't like the idea that things which are likely to be used must be fetched via an API call. Those should be provided as such. But yes, it does not mean that there would never be case(s) where the gpio_regmap should not be used as a handle. Thanks for the patience while explaining me your point Michael :) I'll ponder this for a lil while before creating a new version (probably at next week). Best Regards Matti Vaittinen
On Fri, 2021-05-21 at 14:19 +0300, Andy Shevchenko wrote: > On Fri, May 21, 2021 at 1:19 PM Michael Walle <michael@walle.cc> > wrote: > > Am 2021-05-21 12:09, schrieb Andy Shevchenko: > > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen > > > <matti.vaittinen@fi.rohmeurope.com> wrote: > > > > Changelog v2: (based on suggestions by Michael Walle) > > > > - drop gpio_regmap_set_drvdata() > > > > > > But why do we have gpio_regmap_get_drvdata() and why is it > > > different > > > now to the new member handling? > > > > Eg. the reg_mask_xlate() callback is just passed a "struct > > gpio_regmap*". > > If someone needs to access private data there, > > gpio_regmap_get_drvdata() > > is used. At least that was its intention. > > > > Thus I was also suggesting to use "struct gpio_regmap*" in the > > newer > > callbacks. > > > > I don't get what you mean by "different to the new member > > handling"? > > Currently we have a symmetrical API that is getter and setter against > a certain field. > Now this change drops the setter and introduces some other field > somewhere else. > Sounds to me: > - either this has to be split into two changes with explanation of > what's going on > - or something odd is happening here which I do not understand. > The rationale beind placing the drvdata pointer / setting in the config struct is avoiding races in case the drvdata is needed by operations called during the gpio_chip registration. (If the drvdata is needed for example at the set_config() or init_valid_mask()). Providing the drvdata only after we get the gpio_regmap from registration is too late. It's actually hard to see am use-case for drvdata where providing it after gpio registration would not be racy. Getting the drvdata does not have similar race condition as setting it. Thus the API for getting it is Ok. Best Regards Matti Vaittinen
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c index 134cedf151a7..c05370e984b9 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,31 @@ 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); + 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); + 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) @@ -178,12 +207,6 @@ static int gpio_regmap_direction_output(struct gpio_chip *chip, return gpio_regmap_set_direction(chip, offset, true); } -void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data) -{ - gpio->driver_data = data; -} -EXPORT_SYMBOL_GPL(gpio_regmap_set_drvdata); - void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio) { return gpio->driver_data; @@ -235,6 +258,9 @@ 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->driver_data = config->drvdata; + 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 +279,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 */ diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h index 334dd928042b..96ab3151db96 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,17 +82,21 @@ 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); void gpio_regmap_unregister(struct gpio_regmap *gpio); struct gpio_regmap *devm_gpio_regmap_register(struct device *dev, const struct gpio_regmap_config *config); -void gpio_regmap_set_drvdata(struct gpio_regmap *gpio, void *data); void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio); #endif /* _LINUX_GPIO_REGMAP_H */
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> --- Changelog v2: (based on suggestions by Michael Walle) - drop gpio_regmap_set_drvdata() - drop checks and WARN() for pretty much impossible cases --- drivers/gpio/gpio-regmap.c | 42 +++++++++++++++++++++++++++++++------ include/linux/gpio/regmap.h | 14 ++++++++++++- 2 files changed, 49 insertions(+), 7 deletions(-)