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]>
---
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);
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);
base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
--
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 =]
Utilize the gpio-regmap helper and drop the custom functions
Signed-off-by: Matti Vaittinen <[email protected]>
---
drivers/gpio/Kconfig | 1 +
drivers/gpio/gpio-bd71815.c | 106 ++++++++++--------------------------
2 files changed, 29 insertions(+), 78 deletions(-)
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1dd0ec6727fd..97e1348cd410 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1120,6 +1120,7 @@ config GPIO_BD70528
config GPIO_BD71815
tristate "ROHM BD71815 PMIC GPIO support"
depends on MFD_ROHM_BD71828
+ select GPIO_REGMAP
help
Support for GPO(s) on ROHM BD71815 PMIC. There are two GPOs
available on the ROHM PMIC.
diff --git a/drivers/gpio/gpio-bd71815.c b/drivers/gpio/gpio-bd71815.c
index 08ff2857256f..a241c01e08d1 100644
--- a/drivers/gpio/gpio-bd71815.c
+++ b/drivers/gpio/gpio-bd71815.c
@@ -9,6 +9,7 @@
*/
#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
#include <linux/init.h>
#include <linux/irq.h>
#include <linux/module.h>
@@ -18,81 +19,33 @@
#include <linux/mfd/rohm-bd71815.h>
struct bd71815_gpio {
- /* chip.parent points the MFD which provides DT node and regmap */
- struct gpio_chip chip;
- /* dev points to the platform device for devm and prints */
struct device *dev;
- struct regmap *regmap;
};
-static int bd71815gpo_get(struct gpio_chip *chip, unsigned int offset)
-{
- struct bd71815_gpio *bd71815 = gpiochip_get_data(chip);
- int ret, val;
-
- ret = regmap_read(bd71815->regmap, BD71815_REG_GPO, &val);
- if (ret)
- return ret;
-
- return (val >> offset) & 1;
-}
-
-static void bd71815gpo_set(struct gpio_chip *chip, unsigned int offset,
- int value)
-{
- struct bd71815_gpio *bd71815 = gpiochip_get_data(chip);
- int ret, bit;
-
- bit = BIT(offset);
-
- if (value)
- ret = regmap_set_bits(bd71815->regmap, BD71815_REG_GPO, bit);
- else
- ret = regmap_clear_bits(bd71815->regmap, BD71815_REG_GPO, bit);
-
- if (ret)
- dev_warn(bd71815->dev, "failed to toggle GPO\n");
-}
-
-static int bd71815_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
+static int bd71815_gpio_set_config(struct regmap *regmap, void *drvdata,
+ unsigned int offset,
unsigned long config)
{
- struct bd71815_gpio *bdgpio = gpiochip_get_data(chip);
+ struct bd71815_gpio *bdgpio = (struct bd71815_gpio *)drvdata;
switch (pinconf_to_config_param(config)) {
case PIN_CONFIG_DRIVE_OPEN_DRAIN:
- return regmap_update_bits(bdgpio->regmap,
+ return regmap_update_bits(regmap,
BD71815_REG_GPO,
BD71815_GPIO_DRIVE_MASK << offset,
BD71815_GPIO_OPEN_DRAIN << offset);
case PIN_CONFIG_DRIVE_PUSH_PULL:
- return regmap_update_bits(bdgpio->regmap,
+ return regmap_update_bits(regmap,
BD71815_REG_GPO,
BD71815_GPIO_DRIVE_MASK << offset,
BD71815_GPIO_CMOS << offset);
default:
+ dev_err(bdgpio->dev, "Unsupported config (0x%lx)\n", config);
break;
}
return -ENOTSUPP;
}
-/* BD71815 GPIO is actually GPO */
-static int bd71815gpo_direction_get(struct gpio_chip *gc, unsigned int offset)
-{
- return GPIO_LINE_DIRECTION_OUT;
-}
-
-/* Template for GPIO chip */
-static const struct gpio_chip bd71815gpo_chip = {
- .label = "bd71815",
- .owner = THIS_MODULE,
- .get = bd71815gpo_get,
- .get_direction = bd71815gpo_direction_get,
- .set = bd71815gpo_set,
- .set_config = bd71815_gpio_set_config,
- .can_sleep = true,
-};
-
#define BD71815_TWO_GPIOS GENMASK(1, 0)
#define BD71815_ONE_GPIO BIT(0)
@@ -111,14 +64,16 @@ static const struct gpio_chip bd71815gpo_chip = {
* but allows using it by providing the DT property
* "rohm,enable-hidden-gpo".
*/
-static int bd71815_init_valid_mask(struct gpio_chip *gc,
+static int bd71815_init_valid_mask(struct regmap *regmap, void *drvdata,
unsigned long *valid_mask,
unsigned int ngpios)
{
+ struct bd71815_gpio *bdgpio = (struct bd71815_gpio *)drvdata;
+
if (ngpios != 2)
return 0;
- if (gc->parent && device_property_present(gc->parent,
+ if (bdgpio->dev && device_property_present(bdgpio->dev->parent,
"rohm,enable-hidden-gpo"))
*valid_mask = BD71815_TWO_GPIOS;
else
@@ -127,9 +82,19 @@ static int bd71815_init_valid_mask(struct gpio_chip *gc,
return 0;
}
+/* Template for regmap gpio config */
+static const struct gpio_regmap_config gpio_cfg_template = {
+ .label = "bd71815",
+ .reg_set_base = BD71815_REG_GPO,
+ .ngpio = 2,
+ .set_config = bd71815_gpio_set_config,
+ .init_valid_mask = bd71815_init_valid_mask,
+};
+
static int gpo_bd71815_probe(struct platform_device *pdev)
{
struct bd71815_gpio *g;
+ struct gpio_regmap_config cfg;
struct device *parent, *dev;
/*
@@ -144,30 +109,15 @@ static int gpo_bd71815_probe(struct platform_device *pdev)
if (!g)
return -ENOMEM;
- g->chip = bd71815gpo_chip;
-
- /*
- * FIXME: As writing of this the sysfs interface for GPIO control does
- * not respect the valid_mask. Do not trust it but rather set the ngpios
- * to 1 if "rohm,enable-hidden-gpo" is not given.
- *
- * This check can be removed later if the sysfs export is fixed and
- * if the fix is backported.
- *
- * For now it is safest to just set the ngpios though.
- */
- if (device_property_present(parent, "rohm,enable-hidden-gpo"))
- g->chip.ngpio = 2;
- else
- g->chip.ngpio = 1;
-
- g->chip.init_valid_mask = bd71815_init_valid_mask;
- g->chip.base = -1;
- g->chip.parent = parent;
- g->regmap = dev_get_regmap(parent, NULL);
g->dev = dev;
- return devm_gpiochip_add_data(dev, &g->chip, g);
+ cfg = gpio_cfg_template;
+ cfg.parent = parent;
+ cfg.regmap = dev_get_regmap(parent, NULL);
+ cfg.fwnode = dev_fwnode(dev);
+ cfg.drvdata = g;
+
+ return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &cfg));
}
static struct platform_driver gpo_bd71815_driver = {
--
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 Thu, 2021-05-20 at 14:28 +0300, Matti Vaittinen 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.
>
> Signed-off-by: Matti Vaittinen <[email protected]>
Ouch. Immediately after sending this I noticed that I omitted the
cover-letter. Sorry folks.
In a nutshell - idea is to support providing some IC specific
operations at gpio_regmap registration. This should help broaden the
gpio-regmap IC coverage without the need of exposing the gpio_chip.
Some preliminary discussion can be seen here:
https://lore.kernel.org/linux-gpio/[email protected]/
Best Regards
Matti Vaittinen
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 <[email protected]>
> > ---
> > 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
--
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 for the translation Simon)
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 <[email protected]>
> ---
> 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?
>
> 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;
Thus copying the behavior of gpiolib.
> +
> + 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.
> +
> + 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.
> +
> 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);
>
> base-commit: d07f6ca923ea0927a1024dfccafc5b53b61cfecc
> --
> 2.25.4
-michael
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 <
> > > > [email protected]>
> > > > ---
> > > > 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: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 <[email protected]>
>> > ---
>> > 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
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 <
>> > > > [email protected]>
>> > > > ---
>> > > > 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
On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen
<[email protected]> wrote:
> Utilize the gpio-regmap helper and drop the custom functions
>
> Signed-off-by: Matti Vaittinen <[email protected]>
Very nice!
Reviewed-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Tue, 2021-05-25 at 17:51 +0200, Linus Walleij wrote:
> On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen
> <[email protected]> wrote:
>
> > Utilize the gpio-regmap helper and drop the custom functions
> >
> > Signed-off-by: Matti Vaittinen <[email protected]>
>
> Very nice!
>
> Reviewed-by: Linus Walleij <[email protected]>
Thanks for the review Linus!
Unfortunately I messed up this as I forgot the cover-letter. I am sorry
if this impacted to your checks for updated patch versions. I've sent
couple of new versions after this one.
I drafted and sent v2 but was not completely happy with it. Yesterday I
sent v3 which I think did the "right thing" - but it appears I had a
brain-fart as I forgot to convert both of the other gpio_regmap users..
So, I am hopefully sending out the v4 soon(ish).
Best Regards
Matti Vaittinen