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

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

[Matti Vaittinen]

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

I did also prepare change where the getters for drvdata and regmap
are used. It can also work - but it does not scale quite as well
if (when) IC drivers need some register information to do custom
operations. Interested people can see the:
https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
for comparison.

Changelog v4:
 - Convert also the existing users.

Changelog v3:
 - divide gpio_regmap into private part and part which contains
   user-visible configurations. This should allow keeping the internal
   data internal to gpio_regmap - while allowing the IC driver to re-use
   configurations it has provided to gpio-regmap without a need of
   storing them to private-data. Furthermore avoid implementing dummy
   'getter-functions' for regmap, driver-data, register details,
   firmware node etc.
 - change devm_add_action() to devm_add_action_or_reset()
 - the bd71815 GPIO driver, completely drop private-data.

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_or_reset()
  gpio: bd71815: Use gpio-regmap

 drivers/gpio/Kconfig                  |   1 +
 drivers/gpio/gpio-bd71815.c           | 121 ++++-----------
 drivers/gpio/gpio-regmap.c            | 212 +++++++++++++++-----------
 drivers/gpio/gpio-sl28cpld.c          |   3 +-
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c |   8 +-
 include/linux/gpio/regmap.h           |  51 +++++--
 6 files changed, 194 insertions(+), 202 deletions(-)


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

[Matti Vaittinen]

[-- Attachment #1: Type: text/plain, Size: 20095 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 v4:
  - Convert also the existing gpio_regmap users.

Changelog v3:

IC drivers which need to implement some own GPIO operations are
likely to need data that is provided in gpio-regmap config. Divide
gpio-regmap in GPIO core specific (gpio_chip, ops and internal
book-keeping if any) and public configurations which can be given
(as read-only data) to IC drivers.

Re-use the gpio_regmap_config struct and pass it as such to IC driver
call-backs to avoid polluting interface and IC drivers with small
getter functions.

Changelog v2: (based on suggestions by Michael Walle)
  - drop gpio_regmap_set_drvdata()
  - drop checks and WARN() for pretty much impossible cases

convert existing regmap users
---
Regarding the bcm63xx_reg_mask_xlate - I might go one step further and
get the BCM63XX_BANK_GPIOS value from the gpio_regmap_config just to
make it a tiny bit simpler to support another variant with different
BANK_GPIOS value. I have no idea if such variant would ever come or if
it is a sane thing so I'll leave it as it was. But this is just one case
worth pointing when considering if it is a good idea to give the
gpio_regmap_config as a parameter to driver callbacks.

 drivers/gpio/gpio-regmap.c            | 189 +++++++++++++++-----------
 drivers/gpio/gpio-sl28cpld.c          |   3 +-
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c |   8 +-
 include/linux/gpio/regmap.h           |  51 ++++---
 4 files changed, 152 insertions(+), 99 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 134cedf151a7..4f0903d1acd5 100644
--- a/drivers/gpio/gpio-regmap.c
+++ b/drivers/gpio/gpio-regmap.c
@@ -12,23 +12,13 @@
 #include <linux/regmap.h>
 
 struct gpio_regmap {
-	struct device *parent;
-	struct regmap *regmap;
+	struct gpio_regmap_config config;
+	struct gpio_regmap_ops *ops;
 	struct gpio_chip gpio_chip;
 
-	int reg_stride;
-	int ngpio_per_reg;
-	unsigned int reg_dat_base;
-	unsigned int reg_set_base;
-	unsigned int reg_clr_base;
-	unsigned int reg_dir_in_base;
-	unsigned int reg_dir_out_base;
-
-	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
-			      unsigned int offset, unsigned int *reg,
-			      unsigned int *mask);
-
-	void *driver_data;
+	int (*reg_mask_xlate)(const struct gpio_regmap_config *config,
+			      unsigned int base, unsigned int offset,
+			      unsigned int *reg, unsigned int *mask);
 };
 
 static unsigned int gpio_regmap_addr(unsigned int addr)
@@ -39,14 +29,35 @@ static unsigned int gpio_regmap_addr(unsigned int addr)
 	return addr;
 }
 
-static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
+static int regmap_gpio_init_valid_mask(struct gpio_chip *gc,
+					unsigned long *valid_mask,
+					unsigned int ngpios)
+{
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+
+	return gpio->ops->init_valid_mask(&gpio->config, valid_mask, ngpios);
+}
+
+static int gpio_regmap_set_config(struct gpio_chip *gc, unsigned int offset,
+				  unsigned long config)
+{
+	struct gpio_regmap *gpio;
+
+	gpio = gpiochip_get_data(gc);
+
+	return gpio->ops->set_config(&gpio->config, offset, config);
+}
+
+static int gpio_regmap_simple_xlate(const struct gpio_regmap_config *config,
 				    unsigned int base, unsigned int offset,
 				    unsigned int *reg, unsigned int *mask)
 {
-	unsigned int line = offset % gpio->ngpio_per_reg;
-	unsigned int stride = offset / gpio->ngpio_per_reg;
+	unsigned int line = offset % config->ngpio_per_reg;
+	unsigned int stride = offset / config->ngpio_per_reg;
 
-	*reg = base + stride * gpio->reg_stride;
+	*reg = base + stride * config->reg_stride;
 	*mask = BIT(line);
 
 	return 0;
@@ -55,20 +66,21 @@ static int gpio_regmap_simple_xlate(struct gpio_regmap *gpio,
 static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	struct gpio_regmap_config *config = &gpio->config;
 	unsigned int base, val, reg, mask;
 	int ret;
 
 	/* we might not have an output register if we are input only */
-	if (gpio->reg_dat_base)
-		base = gpio_regmap_addr(gpio->reg_dat_base);
+	if (config->reg_dat_base)
+		base = gpio_regmap_addr(config->reg_dat_base);
 	else
-		base = gpio_regmap_addr(gpio->reg_set_base);
+		base = gpio_regmap_addr(config->reg_set_base);
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(config, base, offset, &reg, &mask);
 	if (ret)
 		return ret;
 
-	ret = regmap_read(gpio->regmap, reg, &val);
+	ret = regmap_read(config->regmap, reg, &val);
 	if (ret)
 		return ret;
 
@@ -79,53 +91,56 @@ static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
 			    int val)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
-	unsigned int base = gpio_regmap_addr(gpio->reg_set_base);
+	struct gpio_regmap_config *config = &gpio->config;
+	unsigned int base = gpio_regmap_addr(config->reg_set_base);
 	unsigned int reg, mask;
 
-	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	gpio->reg_mask_xlate(config, base, offset, &reg, &mask);
 	if (val)
-		regmap_update_bits(gpio->regmap, reg, mask, mask);
+		regmap_update_bits(config->regmap, reg, mask, mask);
 	else
-		regmap_update_bits(gpio->regmap, reg, mask, 0);
+		regmap_update_bits(config->regmap, reg, mask, 0);
 }
 
 static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
 				       unsigned int offset, int val)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	struct gpio_regmap_config *config = &gpio->config;
 	unsigned int base, reg, mask;
 
 	if (val)
-		base = gpio_regmap_addr(gpio->reg_set_base);
+		base = gpio_regmap_addr(config->reg_set_base);
 	else
-		base = gpio_regmap_addr(gpio->reg_clr_base);
+		base = gpio_regmap_addr(config->reg_clr_base);
 
-	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
-	regmap_write(gpio->regmap, reg, mask);
+	gpio->reg_mask_xlate(config, base, offset, &reg, &mask);
+	regmap_write(config->regmap, reg, mask);
 }
 
 static int gpio_regmap_get_direction(struct gpio_chip *chip,
 				     unsigned int offset)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	struct gpio_regmap_config *config = &gpio->config;
 	unsigned int base, val, reg, mask;
 	int invert, ret;
 
-	if (gpio->reg_dir_out_base) {
-		base = gpio_regmap_addr(gpio->reg_dir_out_base);
+	if (config->reg_dir_out_base) {
+		base = gpio_regmap_addr(config->reg_dir_out_base);
 		invert = 0;
-	} else if (gpio->reg_dir_in_base) {
-		base = gpio_regmap_addr(gpio->reg_dir_in_base);
+	} else if (config->reg_dir_in_base) {
+		base = gpio_regmap_addr(config->reg_dir_in_base);
 		invert = 1;
 	} else {
 		return -EOPNOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(config, base, offset, &reg, &mask);
 	if (ret)
 		return ret;
 
-	ret = regmap_read(gpio->regmap, reg, &val);
+	ret = regmap_read(config->regmap, reg, &val);
 	if (ret)
 		return ret;
 
@@ -139,20 +154,21 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 				     unsigned int offset, bool output)
 {
 	struct gpio_regmap *gpio = gpiochip_get_data(chip);
+	struct gpio_regmap_config *config = &gpio->config;
 	unsigned int base, val, reg, mask;
 	int invert, ret;
 
-	if (gpio->reg_dir_out_base) {
-		base = gpio_regmap_addr(gpio->reg_dir_out_base);
+	if (config->reg_dir_out_base) {
+		base = gpio_regmap_addr(config->reg_dir_out_base);
 		invert = 0;
-	} else if (gpio->reg_dir_in_base) {
-		base = gpio_regmap_addr(gpio->reg_dir_in_base);
+	} else if (config->reg_dir_in_base) {
+		base = gpio_regmap_addr(config->reg_dir_in_base);
 		invert = 1;
 	} else {
 		return -EOPNOTSUPP;
 	}
 
-	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	ret = gpio->reg_mask_xlate(config, base, offset, &reg, &mask);
 	if (ret)
 		return ret;
 
@@ -161,7 +177,7 @@ static int gpio_regmap_set_direction(struct gpio_chip *chip,
 	else
 		val = output ? mask : 0;
 
-	return regmap_update_bits(gpio->regmap, reg, mask, val);
+	return regmap_update_bits(config->regmap, reg, mask, val);
 }
 
 static int gpio_regmap_direction_input(struct gpio_chip *chip,
@@ -178,25 +194,18 @@ 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;
-}
-EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata);
-
 /**
  * gpio_regmap_register() - Register a generic regmap GPIO controller
- * @config: configuration for gpio_regmap
+ * @config:	configuration for gpio_regmap
+ * @ops:	Provide pointer IC specific functions to handle needs where
+ *		the standard gpio_regmap does not provide generic functions
+ *		or provided functions do not fit the IC. Can be set NULL if
+ *		no IC specific operations are required.
  *
  * Return: A pointer to the registered gpio_regmap or ERR_PTR error value.
  */
-struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config)
+struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config,
+					 const struct gpio_regmap_ops *ops)
 {
 	struct gpio_regmap *gpio;
 	struct gpio_chip *chip;
@@ -225,26 +234,40 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	if (!gpio)
 		return ERR_PTR(-ENOMEM);
 
-	gpio->parent = config->parent;
-	gpio->regmap = config->regmap;
-	gpio->ngpio_per_reg = config->ngpio_per_reg;
-	gpio->reg_stride = config->reg_stride;
-	gpio->reg_mask_xlate = config->reg_mask_xlate;
-	gpio->reg_dat_base = config->reg_dat_base;
-	gpio->reg_set_base = config->reg_set_base;
-	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->config = *config;
+	if (ops) {
+		/*
+		 * We could avoid ops struct allocation if just the
+		 * xlate is given as it is used directly from gpio_regmap.
+		 * I don't think that optimization is worth the hassle as
+		 * there may not be many cases with custom xlate and no other
+		 * ops. We can change this if I am wrong.
+		 */
+		gpio->ops = kzalloc(sizeof(*ops), GFP_KERNEL);
+		if (!gpio->ops) {
+			ret = -ENOMEM;
+			goto err_free_gpio;
+		}
+		*gpio->ops = *ops;
+	}
 
 	/* if not set, assume there is only one register */
-	if (!gpio->ngpio_per_reg)
-		gpio->ngpio_per_reg = config->ngpio;
+	if (!gpio->config.ngpio_per_reg)
+		gpio->config.ngpio_per_reg = config->ngpio;
 
 	/* if not set, assume they are consecutive */
-	if (!gpio->reg_stride)
-		gpio->reg_stride = 1;
+	if (!gpio->config.reg_stride)
+		gpio->config.reg_stride = 1;
 
-	if (!gpio->reg_mask_xlate)
+	/*
+	 * Dublicate the reg_mask_xlate to gpio_regmap so we don't need to
+	 * always check if ops is populated and reg_mask_xlate is given
+	 * - or allocate whole ops struct just for unconditional
+	 * reg_mask_xlate if no ops are required.
+	 */
+	if (ops && ops->reg_mask_xlate)
+		gpio->reg_mask_xlate = ops->reg_mask_xlate;
+	else
 		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
 
 	chip = &gpio->gpio_chip;
@@ -253,7 +276,12 @@ 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->ops) {
+		if (gpio->ops->set_config)
+			chip->set_config = gpio_regmap_set_config;
+		if (gpio->ops->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 */
 	chip->of_node = to_of_node(config->fwnode);
@@ -269,12 +297,12 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	chip->can_sleep = true;
 
 	chip->get = gpio_regmap_get;
-	if (gpio->reg_set_base && gpio->reg_clr_base)
+	if (gpio->config.reg_set_base && gpio->config.reg_clr_base)
 		chip->set = gpio_regmap_set_with_clear;
-	else if (gpio->reg_set_base)
+	else if (gpio->config.reg_set_base)
 		chip->set = gpio_regmap_set;
 
-	if (gpio->reg_dir_in_base || gpio->reg_dir_out_base) {
+	if (gpio->config.reg_dir_in_base || gpio->config.reg_dir_out_base) {
 		chip->get_direction = gpio_regmap_get_direction;
 		chip->direction_input = gpio_regmap_direction_input;
 		chip->direction_output = gpio_regmap_direction_output;
@@ -295,6 +323,7 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 err_remove_gpiochip:
 	gpiochip_remove(chip);
 err_free_gpio:
+	kfree(gpio->ops);
 	kfree(gpio);
 	return ERR_PTR(ret);
 }
@@ -307,6 +336,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_register);
 void gpio_regmap_unregister(struct gpio_regmap *gpio)
 {
 	gpiochip_remove(&gpio->gpio_chip);
+	kfree(gpio->ops);
 	kfree(gpio);
 }
 EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
@@ -328,7 +358,8 @@ static void devm_gpio_regmap_unregister(struct device *dev, void *res)
  * Return: A pointer to the registered gpio_regmap or ERR_PTR error value.
  */
 struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
-					      const struct gpio_regmap_config *config)
+					      const struct gpio_regmap_config *config,
+					      const struct gpio_regmap_ops *ops)
 {
 	struct gpio_regmap **ptr, *gpio;
 
@@ -337,7 +368,7 @@ struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	gpio = gpio_regmap_register(config);
+	gpio = gpio_regmap_register(config, ops);
 	if (!IS_ERR(gpio)) {
 		*ptr = gpio;
 		devres_add(dev, ptr);
diff --git a/drivers/gpio/gpio-sl28cpld.c b/drivers/gpio/gpio-sl28cpld.c
index 52404736ac86..56095763da9b 100644
--- a/drivers/gpio/gpio-sl28cpld.c
+++ b/drivers/gpio/gpio-sl28cpld.c
@@ -136,7 +136,8 @@ static int sl28cpld_gpio_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config,
+							 NULL));
 }
 
 static const struct of_device_id sl28cpld_gpio_of_match[] = {
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm63xx.c b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
index e1285fe2fbc0..46dea6d245ec 100644
--- a/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
+++ b/drivers/pinctrl/bcm/pinctrl-bcm63xx.c
@@ -19,7 +19,7 @@
 #define BCM63XX_DIROUT_REG	0x04
 #define BCM63XX_DATA_REG	0x0c
 
-static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
+static int bcm63xx_reg_mask_xlate(const struct gpio_regmap_config *cfg,
 				  unsigned int base, unsigned int offset,
 				  unsigned int *reg, unsigned int *mask)
 {
@@ -47,6 +47,9 @@ static int bcm63xx_gpio_probe(struct device *dev, struct device_node *node,
 			      struct bcm63xx_pinctrl *pc)
 {
 	struct gpio_regmap_config grc = {0};
+	struct gpio_regmap_ops ops = {
+		.reg_mask_xlate = bcm63xx_reg_mask_xlate,
+	};
 
 	grc.parent = dev;
 	grc.fwnode = &node->fwnode;
@@ -56,9 +59,8 @@ static int bcm63xx_gpio_probe(struct device *dev, struct device_node *node,
 	grc.reg_dat_base = BCM63XX_DATA_REG;
 	grc.reg_dir_out_base = BCM63XX_DIROUT_REG;
 	grc.reg_set_base = BCM63XX_DATA_REG;
-	grc.reg_mask_xlate = bcm63xx_reg_mask_xlate;
 
-	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc, &ops));
 }
 
 int bcm63xx_pinctrl_probe(struct platform_device *pdev,
diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h
index 334dd928042b..ba1a4b14969b 100644
--- a/include/linux/gpio/regmap.h
+++ b/include/linux/gpio/regmap.h
@@ -6,12 +6,39 @@
 struct device;
 struct fwnode_handle;
 struct gpio_regmap;
+struct gpio_regmap_config;
 struct irq_domain;
 struct regmap;
 
 #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1))
 #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
 
+/**
+ * struct gpio_regmap_ops - Custom operations for regmap_gpio.
+ * @reg_mask_xlate:     Translates base address and GPIO
+ *			offset to a register/bitmask pair. If not given the
+ *			default gpio_regmap_simple_xlate() is used.
+ * @set_config:		Hook for all kinds of settings. Uses the same packed
+ *			config format as generic pinconf.
+ * @init_valid_mask:	Routine to initialize @valid_mask, to be used if not
+ *			all GPIOs are valid.
+ *
+ * Provide ustom operations for the regmap_gpio controller where the
+ * standard regmap_gpio operations are insufficient.
+ * 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
+ * base addresses in the gpio_regmap_config structure.
+ */
+struct gpio_regmap_ops {
+	int (*reg_mask_xlate)(const struct gpio_regmap_config *config,
+			      unsigned int base, unsigned int offset,
+			      unsigned int *reg, unsigned int *mask);
+	int (*set_config)(const struct gpio_regmap_config *regmap_config,
+			  unsigned int offset, unsigned long config);
+	int (*init_valid_mask)(const struct gpio_regmap_config *config,
+			       unsigned long *valid_mask, unsigned int ngpios);
+};
+
 /**
  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
  * @parent:		The parent device
@@ -33,14 +60,9 @@ struct regmap;
  * @ngpio_per_reg:	Number of GPIOs per register
  * @irq_domain:		(Optional) IRQ domain if the controller is
  *			interrupt-capable
- * @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.
- *
- * 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
- * base addresses in this structure.
+ * @drvdata:		(Optional) Pointer to IC specific data which is
+ *			not used by gpio-remap but is provided "as is" to
+ *			the driver callback(s).
  *
  * Although all register base addresses are marked as optional, there are
  * several rules:
@@ -74,17 +96,14 @@ struct gpio_regmap_config {
 	int reg_stride;
 	int ngpio_per_reg;
 	struct irq_domain *irq_domain;
-
-	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
-			      unsigned int offset, unsigned int *reg,
-			      unsigned int *mask);
+	void *drvdata;
 };
 
-struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config);
+struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config,
+					 const struct gpio_regmap_ops *ops);
 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);
+					      const struct gpio_regmap_config *config,
+					      const struct gpio_regmap_ops *ops);
 
 #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 v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset()

[Matti Vaittinen]

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

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

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

---
Changelog v3:
 - gpio-regmap: Use the devm_add_action_or_reset() instead of the
   devm_add_action()
---
 drivers/gpio/gpio-regmap.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
index 4f0903d1acd5..ce5bc9e0d684 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);
 }
 
 /**
@@ -361,20 +361,17 @@ struct gpio_regmap *devm_gpio_regmap_register(struct device *dev,
 					      const struct gpio_regmap_config *config,
 					      const struct gpio_regmap_ops *ops)
 {
-	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;
+	int ret;
 
 	gpio = gpio_regmap_register(config, ops);
-	if (!IS_ERR(gpio)) {
-		*ptr = gpio;
-		devres_add(dev, ptr);
-	} else {
-		devres_free(ptr);
-	}
+
+	if (IS_ERR(gpio))
+		return gpio;
+
+	ret = devm_add_action_or_reset(dev, devm_gpio_regmap_unregister, gpio);
+	if (ret)
+		return ERR_PTR(ret);
 
 	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 --]

Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset()

[Michael Walle]

Am 2021-05-26 08:10, schrieb Matti Vaittinen:
> Slightly simplify the devm_gpio_regmap_register() by using the
> devm_add_action_or_reset().

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

-michael

Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset()

[Bartosz Golaszewski]

On Fri, May 28, 2021 at 7:17 PM Michael Walle <michael@walle.cc> wrote:
>
> Am 2021-05-26 08:10, schrieb Matti Vaittinen:
> > Slightly simplify the devm_gpio_regmap_register() by using the
> > devm_add_action_or_reset().
>
> Reviewed-by: Michael Walle <michael@walle.cc>
>
> -michael

This doesn't apply on its own - looks like it depends on patch 1/3.
Would you mind sending it separately rebased on top of my for-next
branch at https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/?

Bart

Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset()

[Matti Vaittinen]

Morning Bart,

On Wed, 2021-06-02 at 13:54 +0200, Bartosz Golaszewski wrote:
> On Fri, May 28, 2021 at 7:17 PM Michael Walle <michael@walle.cc>
> wrote:
> > Am 2021-05-26 08:10, schrieb Matti Vaittinen:
> > > Slightly simplify the devm_gpio_regmap_register() by using the
> > > devm_add_action_or_reset().
> > 
> > Reviewed-by: Michael Walle <michael@walle.cc>
> > 
> > -michael
> 
> This doesn't apply on its own - looks like it depends on patch 1/3.
> Would you mind sending it separately rebased on top of my for-next
> branch at 
> https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/?
> 

Sure. No problem. I'll respin this sole patch today unless Michael
plans adding other changes - in which case it might be best he includes
this just to avoid the conflicts.

> Bart

Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset()

[Michael Walle]

Am 3. Juni 2021 06:26:03 MESZ schrieb Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>:
>Morning Bart,
>
>On Wed, 2021-06-02 at 13:54 +0200, Bartosz Golaszewski wrote:
>> On Fri, May 28, 2021 at 7:17 PM Michael Walle <michael@walle.cc>
>> wrote:
>> > Am 2021-05-26 08:10, schrieb Matti Vaittinen:
>> > > Slightly simplify the devm_gpio_regmap_register() by using the
>> > > devm_add_action_or_reset().
>> > 
>> > Reviewed-by: Michael Walle <michael@walle.cc>
>> > 
>> > -michael
>> 
>> This doesn't apply on its own - looks like it depends on patch 1/3.
>> Would you mind sending it separately rebased on top of my for-next
>> branch at 
>> https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git/?
>> 
>
>Sure. No problem. I'll respin this sole patch today unless Michael
>plans adding other changes - in which case it might be best he includes
>this just to avoid the conflicts.

Both are fine by me. I don't expect any conflicts, but I can also pick this up when I'm picking the gpio_regmap_set_drvdata() stuff of the other patch. I'll do it probably tomorrow. 

-michael

[PATCH v4 3/3] gpio: bd71815: Use gpio-regmap

[Matti Vaittinen]

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

Utilize the gpio-regmap helper and drop the custom functions

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

---
Changelog v3:
  - gpio: Adapt to changes in gpio_regmap
---
 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-bd71815.c | 121 +++++++++---------------------------
 2 files changed, 32 insertions(+), 90 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..45782d376b3d 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>
@@ -17,82 +18,30 @@
 /* For the BD71815 register definitions */
 #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)
+static int bd71815_gpio_set_config(const struct gpio_regmap_config *gr_config,
+				   unsigned int offset, unsigned long config)
 {
-	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,
-				   unsigned long config)
-{
-	struct bd71815_gpio *bdgpio = gpiochip_get_data(chip);
+	struct regmap *regmap = gr_config->regmap;
 
 	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(gr_config->parent, "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,15 +60,17 @@ 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(const struct gpio_regmap_config *c,
 				   unsigned long *valid_mask,
 				   unsigned int ngpios)
 {
+
 	if (ngpios != 2)
 		return 0;
 
-	if (gc->parent && device_property_present(gc->parent,
-						  "rohm,enable-hidden-gpo"))
+	/* The property should be in MFD DT node */
+	if (c->parent && fwnode_property_present(c->fwnode,
+						 "rohm,enable-hidden-gpo"))
 		*valid_mask = BD71815_TWO_GPIOS;
 	else
 		*valid_mask = BD71815_ONE_GPIO;
@@ -127,9 +78,21 @@ 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,
+};
+
+static const struct gpio_regmap_ops ops = {
+	.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;
 
 	/*
@@ -140,34 +103,12 @@ static int gpo_bd71815_probe(struct platform_device *pdev)
 	/* The device-tree and regmap come from MFD => use parent for that */
 	parent = dev->parent;
 
-	g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
-	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;
+	cfg = gpio_cfg_template;
+	cfg.parent = parent;
+	cfg.regmap = dev_get_regmap(parent, NULL);
+	cfg.fwnode = dev_fwnode(dev);
 
-	return devm_gpiochip_add_data(dev, &g->chip, g);
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &cfg, &ops));
 }
 
 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 v4 0/3] gpio: gpio-regmap: Support few custom operations

[Andy Shevchenko]

On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:
>
> 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/
>
> I did also prepare change where the getters for drvdata and regmap
> are used. It can also work - but it does not scale quite as well
> if (when) IC drivers need some register information to do custom
> operations. Interested people can see the:
> https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
> for comparison.

Entire series looks good to me,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Changelog v4:
>  - Convert also the existing users.
>
> Changelog v3:
>  - divide gpio_regmap into private part and part which contains
>    user-visible configurations. This should allow keeping the internal
>    data internal to gpio_regmap - while allowing the IC driver to re-use
>    configurations it has provided to gpio-regmap without a need of
>    storing them to private-data. Furthermore avoid implementing dummy
>    'getter-functions' for regmap, driver-data, register details,
>    firmware node etc.
>  - change devm_add_action() to devm_add_action_or_reset()
>  - the bd71815 GPIO driver, completely drop private-data.
>
> 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_or_reset()
>   gpio: bd71815: Use gpio-regmap
>
>  drivers/gpio/Kconfig                  |   1 +
>  drivers/gpio/gpio-bd71815.c           | 121 ++++-----------
>  drivers/gpio/gpio-regmap.c            | 212 +++++++++++++++-----------
>  drivers/gpio/gpio-sl28cpld.c          |   3 +-
>  drivers/pinctrl/bcm/pinctrl-bcm63xx.c |   8 +-
>  include/linux/gpio/regmap.h           |  51 +++++--
>  6 files changed, 194 insertions(+), 202 deletions(-)
>
>
> base-commit: c4681547bcce777daf576925a966ffa824edd09d
> --
> 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 v4 0/3] gpio: gpio-regmap: Support few custom operations

[Michael Walle]

Am 2021-05-26 10:42, schrieb Andy Shevchenko:
> On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
>> 
>> 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/
>> 
>> I did also prepare change where the getters for drvdata and regmap
>> are used. It can also work - but it does not scale quite as well
>> if (when) IC drivers need some register information to do custom
>> operations. Interested people can see the:
>> https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
>> for comparison.
> 
> Entire series looks good to me,

Sorry, for being late to this. I got sidetracked.

TBH, I don't like the we have the config struct in the callbacks. Why
would you need all this information in the callback? And it doesn't
help you to call back into gpio-regmap once there are further methods
provided by gpio-regmap.

Either we hide away the internals completely (which I still prefer!) or
we open up the gpio_regmap struct. But this is somewhere in between. As
the user, you could already attach the config to the opaque data pointer
and get the same result.

I don't see how the following is an overhead:

int gpio_regmap_callback(struct gpio_regmap *gpio, ..)
{
     struct regmap *regmap = gpio_regmap_get_regmap(gpio);
     struct driver_priv *data = gpio_regmap_get_drvdata(gpio);
     ...
}

It doesn't clutter anything, there is just a small runtime overhead (is
it?). Again this let you keep adding stuff in the future without
changing any users. So what are the drawbacks of this?

Also I'd like to keep the duplication of the "struct gpio_regmap" 
members
and the config members. The gpio_regmap_config is just a struct so
the _register won't get cluttered with arguments.

I'm still not opposed to convert gpio-regmap into helpers as mentioned
earlier. But until then, I'd really keep the "struct gpio_regmap *gpio"
opaque pointer.

-michael

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

[Matti Vaittinen]

On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote:
> Am 2021-05-26 10:42, schrieb Andy Shevchenko:
> > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
> > <matti.vaittinen@fi.rohmeurope.com> wrote:
> > > 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/
> > > 
> > > I did also prepare change where the getters for drvdata and
> > > regmap
> > > are used. It can also work - but it does not scale quite as well
> > > if (when) IC drivers need some register information to do custom
> > > operations. Interested people can see the:
> > > https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
> > > for comparison.
> > 
> > Entire series looks good to me,
> 
> Sorry, for being late to this. I got sidetracked.
> 
> TBH, I don't like the we have the config struct in the callbacks. Why
> would you need all this information in the callback?

I believe there will be cases when the register information is needed
in callbacks. I don't know the GPIO controllers in details so that I
could give you an real-word example. I guess other people on the list
know the usual GPIO quirks far better than I do. I however have seen
bunch of hardware - and usually each IC has _some_ strange stuff. I
would be surprized if there weren't any cases where the one operation
"toggle X" would not require access to another register which is used
to control "feature Y" - and usually only once in a blue moon. Purely
imaginatory example could be that in order to change direction to
input, one would need to ensure some bit in a output configuration
register is cleared. Then it would be beneficial to have the register
description in call-back.

Or, if we look at the pinctrl-bcm63xx.c - another imaginatory case - we
would get another HW variant with different BCM63XX_BANK_GPIOS value.
Now the IC would not need to store the correct BCM63XX_BANK_GPIOS in
driver data for the xlate-callback - it could directly read the
ngpio_per_reg from config.

As I said, these cases are imaginatory - I don't know the GPIO
controllers well enough to give real-world examples - but I am positive
there are such.


>  And it doesn't
> help you to call back into gpio-regmap once there are further methods
> provided by gpio-regmap.

If we later need this we can use container_of(), right?

> Either we hide away the internals completely (which I still prefer!)
> or
> we open up the gpio_regmap struct. But this is somewhere in between. 

Yes. And I think this is the simplest and cleanest solution which still
provides decent amount of protection, while cuts off the boilerplate.
Additionally this does not add any extra structures because IC drivers
already know the config. Some gpio_regmap internals (like gpio_chip)
can still be kept internal - while config (which in any case is
populated by the IC driver) is public.

> As
> the user, you could already attach the config to the opaque data
> pointer
> and get the same result.

Actually no. This would require user to permanently store the config in
memory which would either duplicate the config or give IC driver a
pointer to gpio_regmap internals. This solution still gives pointer to
gpio_regmap config - but at least we can set it const in function
parameters.

> 
> I don't see how the following is an overhead:
> 
> int gpio_regmap_callback(struct gpio_regmap *gpio, ..)
> {
>      struct regmap *regmap = gpio_regmap_get_regmap(gpio);
>      struct driver_priv *data = gpio_regmap_get_drvdata(gpio);
>      ...
> }

> It doesn't clutter anything, there is just a small runtime overhead
> (is
> it?). Again this let you keep adding stuff in the future without
> changing any users. So what are the drawbacks of this?
> 

It still is overhead. Additionally, I dislike mixing function calls
with declarations - I know that's probably just my personal preference
though. And what is not shown here is the need to declare, define and
export these functions from gpio_regmap. And this is really just
unnecessary boilerplate to me.

> 
> Also I'd like to keep the duplication of the "struct gpio_regmap" 
> members
> and the config members. The gpio_regmap_config is just a struct so
> the _register won't get cluttered with arguments.

The config (as passed from IC driver at register) is dublication. We
do:
gpio->config = *config;

It's just not all meld in the same level.

Best Regards
	Matti Vaittinen

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

[Michael Walle]

Am 2021-05-26 11:44, schrieb Matti Vaittinen:
> On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote:
>> Am 2021-05-26 10:42, schrieb Andy Shevchenko:
>> > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
>> > <matti.vaittinen@fi.rohmeurope.com> wrote:
>> > > 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/
>> > >
>> > > I did also prepare change where the getters for drvdata and
>> > > regmap
>> > > are used. It can also work - but it does not scale quite as well
>> > > if (when) IC drivers need some register information to do custom
>> > > operations. Interested people can see the:
>> > > https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
>> > > for comparison.
>> >
>> > Entire series looks good to me,
>> 
>> Sorry, for being late to this. I got sidetracked.
>> 
>> TBH, I don't like the we have the config struct in the callbacks. Why
>> would you need all this information in the callback?
> 
> I believe there will be cases when the register information is needed
> in callbacks. I don't know the GPIO controllers in details so that I
> could give you an real-word example. I guess other people on the list
> know the usual GPIO quirks far better than I do. I however have seen
> bunch of hardware - and usually each IC has _some_ strange stuff. I
> would be surprized if there weren't any cases where the one operation
> "toggle X" would not require access to another register which is used
> to control "feature Y" - and usually only once in a blue moon. Purely
> imaginatory example could be that in order to change direction to
> input, one would need to ensure some bit in a output configuration
> register is cleared. Then it would be beneficial to have the register
> description in call-back.

Doing something depening on the offsets of some registers sounds like
a hack to me.

> Or, if we look at the pinctrl-bcm63xx.c - another imaginatory case - we
> would get another HW variant with different BCM63XX_BANK_GPIOS value.
> Now the IC would not need to store the correct BCM63XX_BANK_GPIOS in
> driver data for the xlate-callback - it could directly read the
> ngpio_per_reg from config.

which also sounds like a hack, where one really should provide a
driver priv to distiguish between different variant.

> As I said, these cases are imaginatory - I don't know the GPIO
> controllers well enough to give real-world examples - but I am positive
> there are such.
> 
> 
>>  And it doesn't
>> help you to call back into gpio-regmap once there are further methods
>> provided by gpio-regmap.
> 
> If we later need this we can use container_of(), right?

Of course, but isn't your argument to have less boilerplate? ;) And
again, I don't thing the config is the correct first parameter here
for the callback. And it would be different from all the other
subsystems in linux (as far as I know, please correct me if I'm wrong),
which have "their" (sometimes opaque, sometimes not) pointer as the
first argument.

>> Either we hide away the internals completely (which I still prefer!)
>> or
>> we open up the gpio_regmap struct. But this is somewhere in between.
> 
> Yes. And I think this is the simplest and cleanest solution which still
> provides decent amount of protection, while cuts off the boilerplate.

I really don't find this solution "clean".

> Additionally this does not add any extra structures because IC drivers
> already know the config. Some gpio_regmap internals (like gpio_chip)
> can still be kept internal - while config (which in any case is
> populated by the IC driver) is public.
> 
>> As
>> the user, you could already attach the config to the opaque data
>> pointer
>> and get the same result.
> 
> Actually no. This would require user to permanently store the config in
> memory which would either duplicate the config or give IC driver a
> pointer to gpio_regmap internals. This solution still gives pointer to
> gpio_regmap config - but at least we can set it const in function
> parameters.

Of course, your caller has to make sure it will allocate the memory
and doesn't just allocate it on the stack. You're doing the same,
just in gpio-regmap.

>> I don't see how the following is an overhead:
>> 
>> int gpio_regmap_callback(struct gpio_regmap *gpio, ..)
>> {
>>      struct regmap *regmap = gpio_regmap_get_regmap(gpio);
>>      struct driver_priv *data = gpio_regmap_get_drvdata(gpio);
>>      ...
>> }
> 
>> It doesn't clutter anything, there is just a small runtime overhead
>> (is
>> it?). Again this let you keep adding stuff in the future without
>> changing any users. So what are the drawbacks of this?
>> 
> 
> It still is overhead. Additionally, I dislike mixing function calls
> with declarations - I know that's probably just my personal preference
> though.

Well yes, thats just a matter of taste. Everyone is doing
platform_get_drvdata(), for example. If you want to keep something
internal you'd need accessor methods.

> And what is not shown here is the need to declare, define and
> export these functions from gpio_regmap. And this is really just
> unnecessary boilerplate to me.

Exporting the functions is just adding two lines in gpio/regmap.h. How 
can
this be an argument for an overhead on the users?

>> Also I'd like to keep the duplication of the "struct gpio_regmap"
>> members
>> and the config members. The gpio_regmap_config is just a struct so
>> the _register won't get cluttered with arguments.
> 
> The config (as passed from IC driver at register) is dublication. We
> do:
> gpio->config = *config;

Yes and I actually had that during my initial development, but decided
against it, to decouple the information you'll need later and some
you might just discard after probe.

I'm afraid, but I really don't like having the gpio_regmap_config as
the first parameter on callbacks, just because I think this is the
wrong approach, so I vote against this change. I guess it is up
to Linus to decide on this.

Don't get me wrong, I'm all open for change, but there seems to be two
equal approaches to your problem, which just depends on personal
taste.

-michael

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

[Matti Vaittinen]

On Wed, 2021-05-26 at 12:27 +0200, Michael Walle wrote:
> Am 2021-05-26 11:44, schrieb Matti Vaittinen:
> > On Wed, 2021-05-26 at 11:07 +0200, Michael Walle wrote:
> > > Am 2021-05-26 10:42, schrieb Andy Shevchenko:
> > > > On Wed, May 26, 2021 at 9:02 AM Matti Vaittinen
> > > > <matti.vaittinen@fi.rohmeurope.com> wrote:
> > > > > 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/
> > > > > 
> > > > > I did also prepare change where the getters for drvdata and
> > > > > regmap
> > > > > are used. It can also work - but it does not scale quite as
> > > > > well
> > > > > if (when) IC drivers need some register information to do
> > > > > custom
> > > > > operations. Interested people can see the:
> > > > > https://github.com/M-Vaittinen/linux/commits/gpio-regmap-getters
> > > > > for comparison.
> > > > 
> > > > Entire series looks good to me,
> > > 
> > > Sorry, for being late to this. I got sidetracked.
> > > 
> > > TBH, I don't like the we have the config struct in the callbacks.
> > > Why
> > > would you need all this information in the callback?
> > 
> > I believe there will be cases when the register information is
> > needed
> > in callbacks. I don't know the GPIO controllers in details so that
> > I
> > could give you an real-word example. I guess other people on the
> > list
> > know the usual GPIO quirks far better than I do. I however have
> > seen
> > bunch of hardware - and usually each IC has _some_ strange stuff. I
> > would be surprized if there weren't any cases where the one
> > operation
> > "toggle X" would not require access to another register which is
> > used
> > to control "feature Y" - and usually only once in a blue moon.
> > Purely
> > imaginatory example could be that in order to change direction to
> > input, one would need to ensure some bit in a output configuration
> > register is cleared. Then it would be beneficial to have the
> > register
> > description in call-back.
> 
> Doing something depening on the offsets of some registers sounds like
> a hack to me.

Hm. I an unsure I understood your comment. If the hardware requires
setting a bit - then we must do it using the bit offset. What we win
here is that we can get the register address from config w/o allocating
driver data.

> > Or, if we look at the pinctrl-bcm63xx.c - another imaginatory case
> > - we
> > would get another HW variant with different BCM63XX_BANK_GPIOS
> > value.
> > Now the IC would not need to store the correct BCM63XX_BANK_GPIOS
> > in
> > driver data for the xlate-callback - it could directly read the
> > ngpio_per_reg from config.
> 
> which also sounds like a hack, where one really should provide a
> driver priv to distiguish between different variant.

I disagree here. Storing a IC type flag in private data does not scale
well when we have many variants and many 'entities' that depend on the
variant. If we just have the IC type passed to call-back, then the
call-back must have all variant details hard-coded. It scales better if
we can just provide the correct value for the variant - not the variant
type.

> > As I said, these cases are imaginatory - I don't know the GPIO
> > controllers well enough to give real-world examples - but I am
> > positive
> > there are such.
> > 
> > 
> > >  And it doesn't
> > > help you to call back into gpio-regmap once there are further
> > > methods
> > > provided by gpio-regmap.
> > 
> > If we later need this we can use container_of(), right?
> 
> Of course, but isn't your argument to have less boilerplate? ;)

Yes. We can provide the function "gpio_regmap_get_from_config()" which
returns the pointer to struct gpio_regmap - _if ever needed_ instead of
providing bunch of getter functions. (That would be driver_data, regmap
and firmware node already now just to fulfill the needs to the bd71815
- which probably is one of the simplest controllers). So one
potentially needed function vs three that would be needed already now.

>  And
> again, I don't thing the config is the correct first parameter here
> for the callback. And it would be different from all the other
> subsystems in linux (as far as I know, please correct me if I'm
> wrong),
> which have "their" (sometimes opaque, sometimes not) pointer as the
> first argument.

I only know handful of subsystems - and most of those expose the
internals. That's why I suggested it at first. Andy and you both
thought exposing the gpio_regmap or gpio_chip to gpio_regmap users was
a bad idea - hence I suggested this solution - which actually looks
good to me :)

> 
> > > Either we hide away the internals completely (which I still
> > > prefer!)
> > > or
> > > we open up the gpio_regmap struct. But this is somewhere in
> > > between.
> > 
> > Yes. And I think this is the simplest and cleanest solution which
> > still
> > provides decent amount of protection, while cuts off the
> > boilerplate.
> 
> I really don't find this solution "clean".
> 
> > Additionally this does not add any extra structures because IC
> > drivers
> > already know the config. Some gpio_regmap internals (like
> > gpio_chip)
> > can still be kept internal - while config (which in any case is
> > populated by the IC driver) is public.
> > 
> > > As
> > > the user, you could already attach the config to the opaque data
> > > pointer
> > > and get the same result.
> > 
> > Actually no. This would require user to permanently store the
> > config in
> > memory which would either duplicate the config or give IC driver a
> > pointer to gpio_regmap internals. This solution still gives pointer
> > to
> > gpio_regmap config - but at least we can set it const in function
> > parameters.
> 
> Of course, your caller has to make sure it will allocate the memory
> and doesn't just allocate it on the stack. You're doing the same,
> just in gpio-regmap.

Point was that this way we are not necessarily permanently allocating
the memory both in the gpio_regmap _and_ the IC driver. We are also not
allowing IC driver to change config stored in gpio_regmap (well,
without some tricks anyways).

> 
> > > I don't see how the following is an overhead:
> > > 
> > > int gpio_regmap_callback(struct gpio_regmap *gpio, ..)
> > > {
> > >      struct regmap *regmap = gpio_regmap_get_regmap(gpio);
> > >      struct driver_priv *data = gpio_regmap_get_drvdata(gpio);
> > >      ...
> > > }
> > > It doesn't clutter anything, there is just a small runtime
> > > overhead
> > > (is
> > > it?). Again this let you keep adding stuff in the future without
> > > changing any users. So what are the drawbacks of this?
> > > 
> > 
> > It still is overhead. Additionally, I dislike mixing function calls
> > with declarations - I know that's probably just my personal
> > preference
> > though.
> 
> Well yes, thats just a matter of taste. Everyone is doing
> platform_get_drvdata(), for example. If you want to keep something
> internal you'd need accessor methods.
> 
> > And what is not shown here is the need to declare, define and
> > export these functions from gpio_regmap. And this is really just
> > unnecessary boilerplate to me.
> 
> Exporting the functions is just adding two lines in gpio/regmap.h.
> How 
> can
> this be an argument for an overhead on the users?
> 
> > > Also I'd like to keep the duplication of the "struct gpio_regmap"
> > > members
> > > and the config members. The gpio_regmap_config is just a struct
> > > so
> > > the _register won't get cluttered with arguments.
> > 
> > The config (as passed from IC driver at register) is dublication.
> > We
> > do:
> > gpio->config = *config;
> 
> Yes and I actually had that during my initial development, but
> decided
> against it, to decouple the information you'll need later and some
> you might just discard after probe.
> 
> I'm afraid, but I really don't like having the gpio_regmap_config as
> the first parameter on callbacks, just because I think this is the
> wrong approach, so I vote against this change. I guess it is up
> to Linus to decide on this.
> 
> Don't get me wrong, I'm all open for change, but there seems to be
> two
> equal approaches to your problem, which just depends on personal
> taste.

No problem Michael - we are all allowed to have our own opinions - even
the wrong ones ;) Nah, really, the discussion is good. It rarely hurts
the outcome. I think I've explained my view now too. Let's see what
other think :)

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 v4 0/3] gpio: gpio-regmap: Support few custom operations

[Linus Walleij]

On Wed, May 26, 2021 at 8:02 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> Support providing some IC specific operations at gpio_regmap registration.

I see there is some discussion around the abstractions here.

I can only say how we designed gpio-mmio.c (CONFIG_GPIO_GENERIC).

It was designed for GPIO controllers with 8, 16 or 32 bits of GPIO,
each stuffed in a consecutive bit in a set of registers. We later
amended it to deal with bigendian as well, and 64 bit registers,
and some quirks around the registers (like just readable etc).

But that's it. For anything more complex we have opted for
users to write their own drivers with elaborate code.

As library it can sometimes be combined with an irqchip,
if the interrupts are simple.

But overall: each GPIO needs to be a single bit, not 2 not 3
not in every 7th register etc.

I would not try to turn gpio regmap into a Rube Goldberg Machine
panacea-fit-all for all kinds of register and bit layouts, it's nice to
be able to combine it with an interrupt chip or pin controller if those
functions are also simple, like the set/get registers.

Any too bold ambitions will be hard to maintain, I think.

Yours,
Linus Walleij

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

[Matti Vaittinen]

On Thu, 2021-05-27 at 00:46 +0200, Linus Walleij wrote:
> On Wed, May 26, 2021 at 8:02 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > Support providing some IC specific operations at gpio_regmap
> > registration.
> 
> I see there is some discussion around the abstractions here.
> 
> I can only say how we designed gpio-mmio.c (CONFIG_GPIO_GENERIC).
> 
> It was designed for GPIO controllers with 8, 16 or 32 bits of GPIO,
> each stuffed in a consecutive bit in a set of registers. We later
> amended it to deal with bigendian as well, and 64 bit registers,
> and some quirks around the registers (like just readable etc).
> 
> But that's it. For anything more complex we have opted for
> users to write their own drivers with elaborate code.
> 
> As library it can sometimes be combined with an irqchip,
> if the interrupts are simple.
> 
> But overall: each GPIO needs to be a single bit, not 2 not 3
> not in every 7th register etc.
> 
> I would not try to turn gpio regmap into a Rube Goldberg Machine
> panacea-fit-all for all kinds of register and bit layouts, 

This is exactly the point of this patch series. The goal is that
complex hardware is handled by IC specific drivers. But at the same
time, hardware which has 'standard parts' and 'complex parts' could re-
use the well-defined gpio_regmap parts instead of duplicating that code
in IC driver. As far as I understand we do agree with Michael about the
benefits of this overall idea. Current patch only allows IC specific
bits for init_valid_mask and set_config - but I personally see no
problem in expanding this if needed. Place to add more callbacks would
be there - whether to add something or not can then be evaluated case
by case.

I think that the disagreement boils down to few styling issues - and
one more pragmatic one. And only what comes to how we allow
implementing the IC specific call-backs for these more complex HW
specific cases. "Styling" boils down to providing getter-functions for
well-defined gpio_regmap properties like regmap, device and fwnode
pointers Vs. exposing these in a well-defined structure as function
parameters.

The more pragmatic question is how the IC specific bits are provided to
the callbacks. My approach would be allowing IC drivers to use the
existing regmap_gpio config structure as I think in many cases that
would be enough and IC drivers could omit the driver_data, like gpio-
bd71815.c could do. I guess I could go through some of the existing
drivers and see if others would benefit from this too. The other option
I see is to force the IC driver to always allocate drvdata if it needs
any HW information in the callbacks - no matter if this is same
information it has already passed to gpio-regmap in the config. 

> it's nice to
> be able to combine it with an interrupt chip or pin controller if
> those
> functions are also simple, like the set/get registers.
> 
> Any too bold ambitions will be hard to maintain, I think.

In my eyes maintaining drivers which all allocate own set of private
data gets much more messy than maintaining set of drivers most of which
use same predefined config struct and only allocate drvdata if really
needed. After that being said - I am not the one maintaining this :] So
at the end of the day it's fair to go on in a way Michael and You find
easiest to maintain. It is just my view that this series would be the
way to allow many of the IC drivers using regmap to enjoy the benefits
of the gpio-regmap - while still maintaining the formal structure. 

Please, let me know if you think I should see how much this would drop
the code from some of the existing GPIO drivers. It makes no sense to
invest more time in this if others are 100% against it.

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 v4 0/3] gpio: gpio-regmap: Support few custom operations

[Linus Walleij]

On Thu, May 27, 2021 at 8:32 AM Matti Vaittinen
<matti.vaittinen@fi.rohmeurope.com> wrote:

> I think that the disagreement boils down to few styling issues - and
> one more pragmatic one. And only what comes to how we allow
> implementing the IC specific call-backs for these more complex HW
> specific cases. "Styling" boils down to providing getter-functions for
> well-defined gpio_regmap properties like regmap, device and fwnode
> pointers Vs. exposing these in a well-defined structure as function
> parameters.

Just do it the way the maintainer likes it I guess? Michael wrote
the driver so do it in his fashion.

> So
> at the end of the day it's fair to go on in a way Michael and You find
> easiest to maintain.

What makes things easy for me to maintain is active and happy
driver maintainers, so it is paramount that the file looks to Michael
like something he wants to keep maintaining. This removes work
from me and Bartosz.

Maintainer style quirks are common, I have some myself (like
never allowing __underscore_functions) we just adapt to their
quirks and be good diplomats.

Yours,
Linus Walleij

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

[Vaittinen, Matti]

On Fri, 2021-05-28 at 02:53 +0200, Linus Walleij wrote:
> On Thu, May 27, 2021 at 8:32 AM Matti Vaittinen
> <matti.vaittinen@fi.rohmeurope.com> wrote:
> 
> > I think that the disagreement boils down to few styling issues -
> > and
> > one more pragmatic one. And only what comes to how we allow
> > implementing the IC specific call-backs for these more complex HW
> > specific cases. "Styling" boils down to providing getter-functions
> > for
> > well-defined gpio_regmap properties like regmap, device and fwnode
> > pointers Vs. exposing these in a well-defined structure as function
> > parameters.
> 
> Just do it the way the maintainer likes it I guess? Michael wrote
> the driver so do it in his fashion.

This is fair game I'd say. If there is no compromise - then it should
really be in a maintainer's fashion or not at all.

> 
> > So
> > at the end of the day it's fair to go on in a way Michael and You
> > find
> > easiest to maintain.
> 
> What makes things easy for me to maintain is active and happy
> driver maintainers, so it is paramount that the file looks to Michael
> like something he wants to keep maintaining. This removes work
> from me and Bartosz.

I agree. When someone takes care of a driver, he should be happy with
it. Period. And thanks to Michael for writing this driver and reviewing
the patches. Reviewing is hard work.

On the other hand, I don't enjoy writing code I am unhappy with either.
And as this particular piece of code is not a paid task for me, I do
this for fun. gpio-regmap is not mandatory for my drivers now. So, I'll
just opt-out from this change. I'll happily use the gpio-regmap where
it fits, when it fits.

So, during the last cycle I promised to look the gpio-regmap usage on
ROHM IC drivers: Currently it does not fit for the BD71815 as it lack
support for init_valid_mask and set_config. It does not work for the
BD71828 either due to lack of set_config and special handling of
HALL_GPIO. Regarding the BD70528 - with my heart bleeding I just sent a
set of patches to remove this IC's drivers completely. It seems the IC
never really took off. The BD9571 was not authored by me and I don't
have that IC - but at quick glance it seems that driver might work with
gpio-regmap. Conversion would require some testing.

> Maintainer style quirks are common, I have some myself (like
> never allowing __underscore_functions)

I like this. The Linux kernel community is well known for strict and
enforced styling guide. I've also worked in an another pretty huge
project, where every developer was pretty much allowed to use their own
style. I think it resulted happier developers and definitely made code-
readers much more careful ;) I'd claim that made people to pay more
attention when reading code - you could _never_ safely assume. It was
also somehow funny that at times one was able to recognize a file
author just by looking the code :)

Well, I would not suggest that to the Linux kernel - but I definitely
like allowing few maintainer quirks here and there. And I am definitely
hoping to see happy maintainers. Even though the __foo() for internal
functions is the right thing to do (tm) ;)

>  we just adapt to their
> quirks and be good diplomats.

Or keep whingeing and acting up - depending on the person XD

Keep up the good work you all :)

Best Regards
	--Matti

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

[Bartosz Golaszewski]

On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
>

[snip]

> >
> > What makes things easy for me to maintain is active and happy
> > driver maintainers, so it is paramount that the file looks to Michael
> > like something he wants to keep maintaining. This removes work
> > from me and Bartosz.
>
> I agree. When someone takes care of a driver, he should be happy with
> it. Period. And thanks to Michael for writing this driver and reviewing
> the patches. Reviewing is hard work.
>
> On the other hand, I don't enjoy writing code I am unhappy with either.
> And as this particular piece of code is not a paid task for me, I do
> this for fun. gpio-regmap is not mandatory for my drivers now. So, I'll
> just opt-out from this change. I'll happily use the gpio-regmap where
> it fits, when it fits.
>

I take it that path 2/3 is still good to go?

Bart

[snip]

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

[Vaittinen, Matti]

Hi Bartosz,

On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote:
> On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> 
> [snip]
> 
> > > What makes things easy for me to maintain is active and happy
> > > driver maintainers, so it is paramount that the file looks to
> > > Michael
> > > like something he wants to keep maintaining. This removes work
> > > from me and Bartosz.
> > 
> > I agree. When someone takes care of a driver, he should be happy
> > with
> > it. Period. And thanks to Michael for writing this driver and
> > reviewing
> > the patches. Reviewing is hard work.
> > 
> > On the other hand, I don't enjoy writing code I am unhappy with
> > either.
> > And as this particular piece of code is not a paid task for me, I
> > do
> > this for fun. gpio-regmap is not mandatory for my drivers now. So,
> > I'll
> > just opt-out from this change. I'll happily use the gpio-regmap
> > where
> > it fits, when it fits.
> > 
> 
> I take it that path 2/3 is still good to go?

I don't think it had explicit ack from Michael yet - but I think it was
not objected either. I can respin it alone if needed but would help me
if you just pick it from this series (assuming it's Ok for others).

Best Regards
	Matti Vaittinen

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

[Michael Walle]

Hi Matti,

Am 2021-05-28 17:42, schrieb Vaittinen, Matti:
> Hi Bartosz,
> 
> On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote:
>> On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti
>> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
>> 
>> [snip]
>> 
>> > > What makes things easy for me to maintain is active and happy
>> > > driver maintainers, so it is paramount that the file looks to
>> > > Michael
>> > > like something he wants to keep maintaining. This removes work
>> > > from me and Bartosz.
>> >
>> > I agree. When someone takes care of a driver, he should be happy
>> > with
>> > it. Period. And thanks to Michael for writing this driver and
>> > reviewing
>> > the patches. Reviewing is hard work.
>> >
>> > On the other hand, I don't enjoy writing code I am unhappy with
>> > either.
>> > And as this particular piece of code is not a paid task for me, I
>> > do
>> > this for fun. gpio-regmap is not mandatory for my drivers now. So,
>> > I'll
>> > just opt-out from this change. I'll happily use the gpio-regmap
>> > where
>> > it fits, when it fits.
>> >
>> 
>> I take it that path 2/3 is still good to go?
> 
> I don't think it had explicit ack from Michael yet - but I think it was
> not objected either. I can respin it alone if needed but would help me
> if you just pick it from this series (assuming it's Ok for others).

Just sent my R-b.

I'd pick the removal of the gpio_regmap_set_drvdata(), too. If you're
fine with it I'd submit a new patch with just that.

-michael

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

[Vaittinen, Matti]

On Fri, 2021-05-28 at 19:23 +0200, Michael Walle wrote:
> Hi Matti,
> 
> Am 2021-05-28 17:42, schrieb Vaittinen, Matti:
> > Hi Bartosz,
> > 
> > On Fri, 2021-05-28 at 16:31 +0200, Bartosz Golaszewski wrote:
> > > On Fri, May 28, 2021 at 8:33 AM Vaittinen, Matti
> > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > 
> > > I take it that path 2/3 is still good to go?
> > 
> > I don't think it had explicit ack from Michael yet - but I think it
> > was
> > not objected either. I can respin it alone if needed but would help
> > me
> > if you just pick it from this series (assuming it's Ok for others).
> 
> Just sent my R-b.
> 
> I'd pick the removal of the gpio_regmap_set_drvdata(), too. If you're
> fine with it I'd submit a new patch with just that.

Right. Besides the 2/3 might require the 1/3 to apply cleanly. I
somehow thought I sent the devm_add_action_or_reset() as first change
in the series.

Feel free to rework the patches as the best fits your purposes :)

Best Regards
	Matti