[PATCH v2 0/3] gpio: gpio-regmap: Support few custom operations

[PATCH v2 0/3] gpio: gpio-regmap: Support few custom operations

[Matti Vaittinen]

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

Support providing some IC specific operations at gpio_regmap registration.

Implementation of few GPIO related functionalities are likely to be
very IC specific. For example the pin-configuration registers and the
pin validity checks. Allow IC driver to provide IC specific functions
which gpio-regmap can utilize for these IC specific configurations.
This should help broaden the gpio-regmap IC coverage without the need
of exposing the registered gpio_chip or struct gpio_regmap to IC drivers.

The set_config and init_valid_mask are used by ROHM BD71815 GPIO driver.
Convert the BD71815 GPIO driver to use gpio-regmap and get rid of some
code. Rest of the ROHM GPIO drivers are to be reworked after the
mechanism of adding IC specific functions is settled.

Some preliminary discussion can be seen here:
https://lore.kernel.org/linux-gpio/c4faac648d3e0c7f3dcb50f7e24c8b322e8c6974.camel@fi.rohmeurope.com/

Changelog v2:
 - Add cover-letter
 - Drop unnecessary checks for callback function validity
 - drop driver_data setting function as it is likely to be a
   race-condition-by-design

---

Matti Vaittinen (3):
  gpio: regmap: Support few IC specific operations
  gpio: gpio-regmap: Use devm_add_action()
  gpio: bd71815: Use gpio-regmap

 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-bd71815.c | 106 ++++++++++--------------------------
 drivers/gpio/gpio-regmap.c  |  62 ++++++++++++++-------
 include/linux/gpio/regmap.h |  14 ++++-
 4 files changed, 84 insertions(+), 99 deletions(-)


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 v2 1/3] gpio: regmap: Support few IC specific operations

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 5910 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>

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

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

[PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Matti Vaittinen]

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

Slightly simplify the devm_gpio_regmap_register() by using the
devm_add_action().

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
Changelog:
  - New patch at v2

 drivers/gpio/gpio-regmap.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index c05370e984b9..4555e59f916e 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -341,9 +341,9 @@ void gpio_regmap_unregister(struct gpio_regmap *gpio)
 }
 EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
 
-static void devm_gpio_regmap_unregister(struct device *dev, void *res)
+static void devm_gpio_regmap_unregister(void *res)
 {
-	gpio_regmap_unregister(*(struct gpio_regmap **)res);
+	gpio_regmap_unregister(res);
 }
 
 /**
@@ -360,20 +360,12 @@ static void devm_gpio_regmap_unregister(struct device *dev, void *res)
 struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 					      const struct gpio_regmap_config *config)
 {
-	struct gpio_regmap **ptr, *gpio;
-
-	ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
-			   GFP_KERNEL);
-	if (!ptr)
-		return ERR_PTR(-ENOMEM);
+	struct gpio_regmap *gpio;
 
 	gpio = gpio_regmap_register(config);
-	if (!IS_ERR(gpio)) {
-		*ptr = gpio;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+
+	if (!IS_ERR(gpio))
+		devm_add_action(dev, devm_gpio_regmap_unregister, gpio);
 
 	return gpio;
 }
-- 
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 v2 3/3] gpio: bd71815: Use gpio-regmap

[Matti Vaittinen]

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

Utilize the gpio-regmap helper and drop the custom functions

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
Changelog:
 - No changes

 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 v2 1/3] gpio: regmap: Support few IC specific operations

[Michael Walle]

Am 2021-05-21 08:27, 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>

Reviewed-by: Michael Walle <michael@walle.cc>

-michael

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Michael Walle]

Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> Slightly simplify the devm_gpio_regmap_register() by using the
> devm_add_action().

Hm, nice, but what bothers me a bit is that no other subsystem
does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
They also store just one pointer, thus could be simplified in the
same way. What I don't know is if devm_add_action() was intended
to be used this way. So I can't say much for this patch ;)

-michael

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Bjørn Mork]

Michael Walle <michael@walle.cc> writes:

> Am 2021-05-21 08:28, schrieb Matti Vaittinen:
>> Slightly simplify the devm_gpio_regmap_register() by using the
>> devm_add_action().
>
> Hm, nice, but what bothers me a bit is that no other subsystem
> does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> They also store just one pointer, thus could be simplified in the
> same way. What I don't know is if devm_add_action() was intended
> to be used this way. So I can't say much for this patch ;)

There are some examples.  Like:

int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter *adapter)
{
        int ret;

        ret = i2c_add_adapter(adapter);
        if (ret)
                return ret;

        return devm_add_action_or_reset(dev, devm_i2c_del_adapter, adapter);
}


You should probably use the devm_add_action_or_reset() wrapper here too,
catching the unlikely devm_add_action() alloc failure.


Bjørn

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> 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 <matti.vaittinen@fi.rohmeurope.com>
>
> ---
> 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

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Andy Shevchenko]

On Fri, May 21, 2021 at 12:54 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Slightly simplify the devm_gpio_regmap_register() by using the
> devm_add_action().

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
> ---
> Changelog:
>   - New patch at v2
>
>  drivers/gpio/gpio-regmap.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> index c05370e984b9..4555e59f916e 100644
> --- a/drivers/gpio/gpio-regmap.c
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -341,9 +341,9 @@ void gpio_regmap_unregister(struct gpio_regmap *gpio)
>  }
>  EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
>
> -static void devm_gpio_regmap_unregister(struct device *dev, void *res)
> +static void devm_gpio_regmap_unregister(void *res)
>  {
> -       gpio_regmap_unregister(*(struct gpio_regmap **)res);
> +       gpio_regmap_unregister(res);
>  }
>
>  /**
> @@ -360,20 +360,12 @@ static void devm_gpio_regmap_unregister(struct device *dev, void *res)
>  struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
>                                               const struct gpio_regmap_config *config)
>  {
> -       struct gpio_regmap **ptr, *gpio;
> -
> -       ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
> -                          GFP_KERNEL);
> -       if (!ptr)
> -               return ERR_PTR(-ENOMEM);
> +       struct gpio_regmap *gpio;
>
>         gpio = gpio_regmap_register(config);
> -       if (!IS_ERR(gpio)) {
> -               *ptr = gpio;
> -               devres_add(dev, ptr);
> -       } else {
> -               devres_free(ptr);
> -       }
> +
> +       if (!IS_ERR(gpio))
> +               devm_add_action(dev, devm_gpio_regmap_unregister, gpio);
>
>         return gpio;
>  }
> --
> 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 =]



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/3] gpio: bd71815: Use gpio-regmap

[Andy Shevchenko]

On Fri, May 21, 2021 at 12:54 PM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> Utilize the gpio-regmap helper and drop the custom functions

I like the statistics of this change!
FWIW,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

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



-- 
With Best Regards,
Andy Shevchenko

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

[Michael Walle]

Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> 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

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

[Vaittinen, Matti]

On Fri, 2021-05-21 at 13:09 +0300, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> 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 <matti.vaittinen@fi.rohmeurope.com>
> > 
> > ---
> > Changelog v2: (based on suggestions by Michael Walle)
> >   - drop gpio_regmap_set_drvdata()
> 
> But why do we have gpio_regmap_get_drvdata()

I was thinking the drivers might still need to get the drvdata. But now
I don't see how they could easily get it via this API. Yes, the
regmap_gpio struct (which is used to obtain the data via this API) is
returned by the registration - but the only place where IC driver could
store this is in the drvdata :) So yes, this is next to useless now.

Sure this API could be used in probe, after the registration - but
there the driver should have access to drvdata and config anyways.

I'll drop also the gpio_regmap_get_drvdata() in next version. Thanks
for pointing this out.

>  and why is it different
> now to the new member handling?

Hmm. I am unsure what you mean by this?

> 
> >   - drop checks and WARN() for pretty much impossible cases

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

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

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Vaittinen, Matti]

On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> Michael Walle <michael@walle.cc> writes:
> 
> > Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> > > Slightly simplify the devm_gpio_regmap_register() by using the
> > > devm_add_action().
> > 
> > Hm, nice, but what bothers me a bit is that no other subsystem
> > does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> > They also store just one pointer, thus could be simplified in the
> > same way. What I don't know is if devm_add_action() was intended
> > to be used this way. So I can't say much for this patch ;)
> 
> There are some examples.  Like:
> 
> int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter
> *adapter)
> {
>         int ret;
> 
>         ret = i2c_add_adapter(adapter);
>         if (ret)
>                 return ret;
> 
>         return devm_add_action_or_reset(dev, devm_i2c_del_adapter,
> adapter);
> }
> 
> 
> You should probably use the devm_add_action_or_reset() wrapper here
> too,
> catching the unlikely devm_add_action() alloc failure.
> 

I was thinking of it but as the gpio registration succeeded I was
thinking that we could go on with it - (which means we can proceed but
the gpio is never released.)

I am not sure how much difference it makes in the case of small alloc
failure ;)

But as it seems I am in any case re-spinning this I can change this to
the devm_add_action_or_reset() and fail the gpio_regmap registration if
alloc fails.

Best Regards
	Matti Vaittinen

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

[Michael Walle]

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

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

[Andy Shevchenko]

On Fri, May 21, 2021 at 1:19 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> 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

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

[Vaittinen, Matti]

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

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

[Vaittinen, Matti]

On Fri, 2021-05-21 at 14:19 +0300, Andy Shevchenko wrote:
> On Fri, May 21, 2021 at 1:19 PM Michael Walle <michael@walle.cc>
> wrote:
> > Am 2021-05-21 12:09, schrieb Andy Shevchenko:
> > > On Fri, May 21, 2021 at 12:53 PM Matti Vaittinen
> > > <matti.vaittinen@fi.rohmeurope.com> 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.
> 
The rationale beind placing the drvdata pointer / setting in the config
struct is avoiding races in case the drvdata is needed by operations
called during the gpio_chip registration. (If the drvdata is needed for
example at the set_config() or init_valid_mask()). Providing the
drvdata only after we get the gpio_regmap from registration is too
late.

It's actually hard to see am use-case for drvdata where providing it
after gpio registration would not be racy.

Getting the drvdata does not have similar race condition as setting it.
Thus the API for getting it is Ok.

Best Regards
	Matti Vaittinen

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Bartosz Golaszewski]

On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>
>
> On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> > Michael Walle <michael@walle.cc> writes:
> >
> > > Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> > > > Slightly simplify the devm_gpio_regmap_register() by using the
> > > > devm_add_action().
> > >
> > > Hm, nice, but what bothers me a bit is that no other subsystem
> > > does it that way, eg. hwmon/hwmon.c or watchdog/watchdog_core.c.
> > > They also store just one pointer, thus could be simplified in the
> > > same way. What I don't know is if devm_add_action() was intended
> > > to be used this way. So I can't say much for this patch ;)
> >
> > There are some examples.  Like:
> >
> > int devm_i2c_add_adapter(struct device *dev, struct i2c_adapter
> > *adapter)
> > {
> >         int ret;
> >
> >         ret = i2c_add_adapter(adapter);
> >         if (ret)
> >                 return ret;
> >
> >         return devm_add_action_or_reset(dev, devm_i2c_del_adapter,
> > adapter);
> > }
> >
> >
> > You should probably use the devm_add_action_or_reset() wrapper here
> > too,
> > catching the unlikely devm_add_action() alloc failure.
> >
>
> I was thinking of it but as the gpio registration succeeded I was
> thinking that we could go on with it - (which means we can proceed but
> the gpio is never released.)
>
> I am not sure how much difference it makes in the case of small alloc
> failure ;)
>
> But as it seems I am in any case re-spinning this I can change this to
> the devm_add_action_or_reset() and fail the gpio_regmap registration if
> alloc fails.
>
> Best Regards
>         Matti Vaittinen

Hi Matti,

Please use the reset variant. We always want to roll-back the changes
done in a function before the failure and propagate the error code.

Bart

Re: [PATCH v2 2/3] gpio: gpio-regmap: Use devm_add_action()

[Vaittinen, Matti]

Morning folks!

On Fri, 2021-05-21 at 18:35 +0200, Bartosz Golaszewski wrote:
> On Fri, May 21, 2021 at 12:31 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > 
> > On Fri, 2021-05-21 at 10:38 +0200, Bjørn Mork wrote:
> > > Michael Walle <michael@walle.cc> writes:
> > > 
> > > > Am 2021-05-21 08:28, schrieb Matti Vaittinen:
> > > > > Slightly simplify the devm_gpio_regmap_register() by using
> > > > > the
> > > > > devm_add_action().
> > > > 
> > > > 
> > > You should probably use the devm_add_action_or_reset() wrapper
> > > here
> > > too,
> > > catching the unlikely devm_add_action() alloc failure.
> > > 
> > 
> > I was thinking of it but as the gpio registration succeeded I was
> > thinking that we could go on with it - (which means we can proceed
> > but
> > the gpio is never released.)
> > 
> > I am not sure how much difference it makes in the case of small
> > alloc
> > failure ;)
> > 
> > But as it seems I am in any case re-spinning this I can change this
> > to
> > the devm_add_action_or_reset() and fail the gpio_regmap
> > registration if
> > alloc fails.
> > 
> > Best Regards
> >         Matti Vaittinen
> 
> Hi Matti,
> 
> Please use the reset variant. We always want to roll-back the changes
> done in a function before the failure and propagate the error code.

Right. I'll do that. I hope to be able to re-spin this today.

Best Regards
	-- Matti