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 <[email protected]>
---
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(-)
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 */
--
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 =]
On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
<[email protected]> 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 <[email protected]>
>
> ---
> 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
--
With Best Regards,
Andy Shevchenko
Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> <[email protected]> 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 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
> > <[email protected]> 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
>> > <[email protected]> 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, 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
> > > > <[email protected]> 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, May 21, 2021 at 1:19 PM Michael Walle <[email protected]> wrote:
>
> Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> > <[email protected]> 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.
--
With Best Regards,
Andy Shevchenko