[PATCH 1/2] gpio: regmap: Support few IC specific operations

[PATCH 1/2] gpio: regmap: Support few IC specific operations

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5500 bytes --]

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);
 
 	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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 2/2] gpio: bd71815: Use gpio-regmap

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 6380 bytes --]

Utilize the gpio-regmap helper and drop the custom functions

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 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 =] 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[Vaittinen, Matti]

[-- Attachment #1: Type: text/plain, Size: 797 bytes --]


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 <matti.vaittinen@fi.rohmeurope.com>

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/c4faac648d3e0c7f3dcb50f7e24c8b322e8c6974.camel@fi.rohmeurope.com/

Best Regards
	Matti Vaittinen


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[Michael Walle]

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 <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?

> 
>  	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

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[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 <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.

> 
> >  	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)

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[Michael Walle]

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

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[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 <
> > > > 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.

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

Re: [PATCH 1/2] gpio: regmap: Support few IC specific operations

[Michael Walle]

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 <
>> > > > 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.
> 
> 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

Re: [PATCH 2/2] gpio: bd71815: Use gpio-regmap

[Linus Walleij]

On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> Utilize the gpio-regmap helper and drop the custom functions
>
> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

Very nice!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH 2/2] gpio: bd71815: Use gpio-regmap

[Vaittinen, Matti]

On Tue, 2021-05-25 at 17:51 +0200, Linus Walleij wrote:
> On Thu, May 20, 2021 at 1:30 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > Utilize the gpio-regmap helper and drop the custom functions
> > 
> > Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> 
> Very nice!
> 
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

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