* [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations @ 2021-05-26 6:02 Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 1/3] gpio: regmap: Support few IC specific operations Matti Vaittinen ` (4 more replies) 0 siblings, 5 replies; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 6:02 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, linux-kernel, linux-power, linux-arm-kernel, Andy Shevchenko [-- 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 --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 1/3] gpio: regmap: Support few IC specific operations 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen @ 2021-05-26 6:10 ` Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() Matti Vaittinen ` (3 subsequent siblings) 4 siblings, 0 replies; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 6:10 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, linux-kernel, linux-power, linux-arm-kernel, Andy Shevchenko [-- 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, ®, &mask); + ret = gpio->reg_mask_xlate(config, base, offset, ®, &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, ®, &mask); + gpio->reg_mask_xlate(config, base, offset, ®, &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, ®, &mask); - regmap_write(gpio->regmap, reg, mask); + gpio->reg_mask_xlate(config, base, offset, ®, &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, ®, &mask); + ret = gpio->reg_mask_xlate(config, base, offset, ®, &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, ®, &mask); + ret = gpio->reg_mask_xlate(config, base, offset, ®, &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 --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 1/3] gpio: regmap: Support few IC specific operations Matti Vaittinen @ 2021-05-26 6:10 ` Matti Vaittinen 2021-05-28 17:17 ` Michael Walle 2021-05-26 6:10 ` [PATCH v4 3/3] gpio: bd71815: Use gpio-regmap Matti Vaittinen ` (2 subsequent siblings) 4 siblings, 1 reply; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 6:10 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, linux-kernel, linux-power, linux-arm-kernel, Andy Shevchenko [-- 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 --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() 2021-05-26 6:10 ` [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() Matti Vaittinen @ 2021-05-28 17:17 ` Michael Walle 2021-06-02 11:54 ` Bartosz Golaszewski 0 siblings, 1 reply; 21+ messages in thread From: Michael Walle @ 2021-05-28 17:17 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, linux-kernel, linux-power, linux-arm-kernel, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() 2021-05-28 17:17 ` Michael Walle @ 2021-06-02 11:54 ` Bartosz Golaszewski 2021-06-03 4:26 ` Matti Vaittinen 0 siblings, 1 reply; 21+ messages in thread From: Bartosz Golaszewski @ 2021-06-02 11:54 UTC (permalink / raw) To: Michael Walle, Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, LKML, linux-power, arm-soc, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() 2021-06-02 11:54 ` Bartosz Golaszewski @ 2021-06-03 4:26 ` Matti Vaittinen 2021-06-03 7:40 ` Michael Walle 0 siblings, 1 reply; 21+ messages in thread From: Matti Vaittinen @ 2021-06-03 4:26 UTC (permalink / raw) To: Bartosz Golaszewski, Michael Walle Cc: Linus Walleij, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, LKML, linux-power, arm-soc, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() 2021-06-03 4:26 ` Matti Vaittinen @ 2021-06-03 7:40 ` Michael Walle 0 siblings, 0 replies; 21+ messages in thread From: Michael Walle @ 2021-06-03 7:40 UTC (permalink / raw) To: matti.vaittinen, Matti Vaittinen, Bartosz Golaszewski Cc: Linus Walleij, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, LKML, linux-power, arm-soc, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v4 3/3] gpio: bd71815: Use gpio-regmap 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 1/3] gpio: regmap: Support few IC specific operations Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() Matti Vaittinen @ 2021-05-26 6:10 ` Matti Vaittinen 2021-05-26 8:42 ` [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Andy Shevchenko 2021-05-26 22:46 ` Linus Walleij 4 siblings, 0 replies; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 6:10 UTC (permalink / raw) To: Matti Vaittinen, Matti Vaittinen Cc: Linus Walleij, Bartosz Golaszewski, Matti Vaittinen, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, linux-gpio, linux-kernel, linux-power, linux-arm-kernel, Andy Shevchenko [-- 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 --] ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen ` (2 preceding siblings ...) 2021-05-26 6:10 ` [PATCH v4 3/3] gpio: bd71815: Use gpio-regmap Matti Vaittinen @ 2021-05-26 8:42 ` Andy Shevchenko 2021-05-26 9:07 ` Michael Walle 2021-05-26 22:46 ` Linus Walleij 4 siblings, 1 reply; 21+ messages in thread From: Andy Shevchenko @ 2021-05-26 8:42 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, linux-power, linux-arm Mailing List 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 8:42 ` [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Andy Shevchenko @ 2021-05-26 9:07 ` Michael Walle 2021-05-26 9:44 ` Matti Vaittinen 0 siblings, 1 reply; 21+ messages in thread From: Michael Walle @ 2021-05-26 9:07 UTC (permalink / raw) To: Andy Shevchenko Cc: Matti Vaittinen, Matti Vaittinen, Linus Walleij, Bartosz Golaszewski, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, linux-power, linux-arm Mailing List 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 9:07 ` Michael Walle @ 2021-05-26 9:44 ` Matti Vaittinen 2021-05-26 10:27 ` Michael Walle 0 siblings, 1 reply; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 9:44 UTC (permalink / raw) To: Michael Walle, Andy Shevchenko Cc: Linus Walleij, Bartosz Golaszewski, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, linux-power, linux-arm Mailing List 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 9:44 ` Matti Vaittinen @ 2021-05-26 10:27 ` Michael Walle 2021-05-26 11:00 ` Matti Vaittinen 0 siblings, 1 reply; 21+ messages in thread From: Michael Walle @ 2021-05-26 10:27 UTC (permalink / raw) To: matti.vaittinen Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, linux-power, linux-arm Mailing List 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 10:27 ` Michael Walle @ 2021-05-26 11:00 ` Matti Vaittinen 0 siblings, 0 replies; 21+ messages in thread From: Matti Vaittinen @ 2021-05-26 11:00 UTC (permalink / raw) To: Michael Walle Cc: Andy Shevchenko, Linus Walleij, Bartosz Golaszewski, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, Linux Kernel Mailing List, linux-power, linux-arm Mailing List 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen ` (3 preceding siblings ...) 2021-05-26 8:42 ` [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Andy Shevchenko @ 2021-05-26 22:46 ` Linus Walleij 2021-05-27 6:32 ` Matti Vaittinen 4 siblings, 1 reply; 21+ messages in thread From: Linus Walleij @ 2021-05-26 22:46 UTC (permalink / raw) To: Matti Vaittinen Cc: Matti Vaittinen, Bartosz Golaszewski, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, linux-kernel, linux-power, Linux ARM, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-26 22:46 ` Linus Walleij @ 2021-05-27 6:32 ` Matti Vaittinen 2021-05-28 0:53 ` Linus Walleij 0 siblings, 1 reply; 21+ messages in thread From: Matti Vaittinen @ 2021-05-27 6:32 UTC (permalink / raw) To: Linus Walleij Cc: Bartosz Golaszewski, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, linux-kernel, linux-power, Linux ARM, Andy Shevchenko 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) ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-27 6:32 ` Matti Vaittinen @ 2021-05-28 0:53 ` Linus Walleij 2021-05-28 6:33 ` Vaittinen, Matti 0 siblings, 1 reply; 21+ messages in thread From: Linus Walleij @ 2021-05-28 0:53 UTC (permalink / raw) To: Matti Vaittinen Cc: Bartosz Golaszewski, Michael Walle, Florian Fainelli, bcm-kernel-feedback-list, Jonas Gorski, Álvaro Fernández Rojas, open list:GPIO SUBSYSTEM, linux-kernel, linux-power, Linux ARM, Andy Shevchenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-28 0:53 ` Linus Walleij @ 2021-05-28 6:33 ` Vaittinen, Matti 2021-05-28 14:31 ` Bartosz Golaszewski 0 siblings, 1 reply; 21+ messages in thread From: Vaittinen, Matti @ 2021-05-28 6:33 UTC (permalink / raw) To: linus.walleij Cc: linux-power, bcm-kernel-feedback-list, bgolaszewski, linux-gpio, linux-kernel, michael, noltari, jonas.gorski, f.fainelli, andy.shevchenko, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-28 6:33 ` Vaittinen, Matti @ 2021-05-28 14:31 ` Bartosz Golaszewski 2021-05-28 15:42 ` Vaittinen, Matti 0 siblings, 1 reply; 21+ messages in thread From: Bartosz Golaszewski @ 2021-05-28 14:31 UTC (permalink / raw) To: Vaittinen, Matti Cc: linus.walleij, linux-power, bcm-kernel-feedback-list, linux-gpio, linux-kernel, michael, noltari, jonas.gorski, f.fainelli, andy.shevchenko, linux-arm-kernel 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] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-28 14:31 ` Bartosz Golaszewski @ 2021-05-28 15:42 ` Vaittinen, Matti 2021-05-28 17:23 ` Michael Walle 0 siblings, 1 reply; 21+ messages in thread From: Vaittinen, Matti @ 2021-05-28 15:42 UTC (permalink / raw) To: bgolaszewski Cc: bcm-kernel-feedback-list, linux-arm-kernel, linux-power, linus.walleij, linux-kernel, linux-gpio, jonas.gorski, noltari, michael, andy.shevchenko, f.fainelli 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-28 15:42 ` Vaittinen, Matti @ 2021-05-28 17:23 ` Michael Walle 2021-05-31 7:42 ` Vaittinen, Matti 0 siblings, 1 reply; 21+ messages in thread From: Michael Walle @ 2021-05-28 17:23 UTC (permalink / raw) To: Vaittinen, Matti Cc: bgolaszewski, bcm-kernel-feedback-list, linux-arm-kernel, linux-power, linus.walleij, linux-kernel, linux-gpio, jonas.gorski, noltari, andy.shevchenko, f.fainelli 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations 2021-05-28 17:23 ` Michael Walle @ 2021-05-31 7:42 ` Vaittinen, Matti 0 siblings, 0 replies; 21+ messages in thread From: Vaittinen, Matti @ 2021-05-31 7:42 UTC (permalink / raw) To: michael Cc: bcm-kernel-feedback-list, bgolaszewski, linux-power, linus.walleij, linux-kernel, linux-arm-kernel, jonas.gorski, noltari, linux-gpio, andy.shevchenko, f.fainelli 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2021-06-03 7:40 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-26 6:02 [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 1/3] gpio: regmap: Support few IC specific operations Matti Vaittinen 2021-05-26 6:10 ` [PATCH v4 2/3] gpio: gpio-regmap: Use devm_add_action_or_reset() Matti Vaittinen 2021-05-28 17:17 ` Michael Walle 2021-06-02 11:54 ` Bartosz Golaszewski 2021-06-03 4:26 ` Matti Vaittinen 2021-06-03 7:40 ` Michael Walle 2021-05-26 6:10 ` [PATCH v4 3/3] gpio: bd71815: Use gpio-regmap Matti Vaittinen 2021-05-26 8:42 ` [PATCH v4 0/3] gpio: gpio-regmap: Support few custom operations Andy Shevchenko 2021-05-26 9:07 ` Michael Walle 2021-05-26 9:44 ` Matti Vaittinen 2021-05-26 10:27 ` Michael Walle 2021-05-26 11:00 ` Matti Vaittinen 2021-05-26 22:46 ` Linus Walleij 2021-05-27 6:32 ` Matti Vaittinen 2021-05-28 0:53 ` Linus Walleij 2021-05-28 6:33 ` Vaittinen, Matti 2021-05-28 14:31 ` Bartosz Golaszewski 2021-05-28 15:42 ` Vaittinen, Matti 2021-05-28 17:23 ` Michael Walle 2021-05-31 7:42 ` Vaittinen, Matti
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).