Messages in this thread | | | Date | Thu, 20 May 2021 14:22:19 +0200 | From | Michael Walle <> | Subject | Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations |
| |
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
| |