Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations
From: Michael Walle
Date: Thu May 20 2021 - 08:58:51 EST
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@xxxxxxxxxxxxxxxxx>
> ---
> 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