Skip to contentSkip to conversation

pinctrl: bcm63xx: properly request gpios

PATCH In review Álvaro Fernández Rojas 3 participants 6 messages Jan 7, 2025 – Jan 14, 2025

Subsystems (3)
GPIO Pin control GPIO regmap
lore
Share & tools
lore

Álvaro Fernández Rojas

author
#0 lore raw

cover pinctrl: bcm63xx: properly request gpios

Add gpio_chip request/free functions to gpio-regmap and use them on bcm63xx in order to properly call gpio_request_enable when requesting a gpio.

Álvaro Fernández Rojas (2):
gpio: regmap: add request/free gpio_chip functions
pinctrl: bcm63xx: implement gpio_regmap request/free

 drivers/gpio/gpio-regmap.c            |  2 ++
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 14 ++++++++++++++
 include/linux/gpio/regmap.h           |  4 ++++
 3 files changed, 20 insertions(+)
Signature · 1 line
2.39.5
author
#1 lore raw

PATCH 1/2 gpio: regmap: add request/free gpio_chip functions

Allow configuring gpio_chip request/free functions when creating a gpio-regmap.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/gpio/gpio-regmap.c  | 2 ++
 include/linux/gpio/regmap.h | 4 ++++
 2 files changed, 6 insertions(+)
drivers/gpio/gpio-regmap.c +2 -0
index 71684dee2ca5..32ec85b41653 100644
@@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
 	chip->names = config->names;
 	chip->label = config->label ?: dev_name(config->parent);
 	chip->can_sleep = regmap_might_sleep(config->regmap);
+	chip->request = config->request;
+	chip->free = config->free;
 
 	chip->get = gpio_regmap_get;
 	if (gpio->reg_set_base && gpio->reg_clr_base)
include/linux/gpio/regmap.h +4 -0
index a9f7b7faf57b..cd4ea9661eea 100644
@@ -5,6 +5,7 @@
 
 struct device;
 struct fwnode_handle;
+struct gpio_chip;
 struct gpio_regmap;
 struct irq_domain;
 struct regmap;

@@ -82,6 +83,9 @@ struct gpio_regmap_config {
 			      unsigned int offset, unsigned int *reg,
 			      unsigned int *mask);
 
+	int (*request)(struct gpio_chip *chip, unsigned int offset);
+	void (*free)(struct gpio_chip *chip, unsigned int offset);
+
 	void *drvdata;
 };
Signature · 1 line
2.39.5
#2 lore raw

Re: [PATCH 1/2] gpio: regmap: add request/free gpio_chip functions

Hi,

Quoted 14 lines · Álvaro Fernández Rojas

Allow configuring gpio_chip request/free functions when creating a gpio-regmap.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/gpio/gpio-regmap.c  | 2 ++
 include/linux/gpio/regmap.h | 4 ++++
 2 files changed, 6 insertions(+)
drivers/gpio/gpio-regmap.c +0 -0
index 71684dee2ca5..32ec85b41653 100644

drivers/gpio/gpio-regmap.c struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config · line 265

Quoted 5 lines
drivers/gpio/gpio-regmap.c +2 -0
@@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
        chip->names = config->names;
        chip->label = config->label ?: dev_name(config->parent);
        chip->can_sleep = regmap_might_sleep(config->regmap);
+       chip->request = config->request;
+       chip->free = config->free;

I wonder if you couldn't just use gpiochip_generic_request() / gpiochip_generic_free() unconditionally here. AFAIU, these don't do anything when there are no GPIO ranges defined (so should not interfere with non-pinctrl linked devices), as well as nothing when CONFIG_PINCTRL isn't enabled, so they should be NOPs if there is no pinctrl link, and do the right thing if there is one.

Best Regards,
Jonas

author
#3 lore raw

Re: [PATCH 1/2] gpio: regmap: add request/free gpio_chip functions

Quoted 32 lines · Jonas Gorski

Hi,

Quoted 14 lines · Álvaro Fernández Rojas

Allow configuring gpio_chip request/free functions when creating a gpio-regmap.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/gpio/gpio-regmap.c  | 2 ++
 include/linux/gpio/regmap.h | 4 ++++
 2 files changed, 6 insertions(+)
drivers/gpio/gpio-regmap.c +0 -0
index 71684dee2ca5..32ec85b41653 100644

drivers/gpio/gpio-regmap.c struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config · line 265

Quoted 5 lines
drivers/gpio/gpio-regmap.c +2 -0
@@ -261,6 +261,8 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config
        chip->names = config->names;
        chip->label = config->label ?: dev_name(config->parent);
        chip->can_sleep = regmap_might_sleep(config->regmap);
+       chip->request = config->request;
+       chip->free = config->free;

I wonder if you couldn't just use gpiochip_generic_request() / gpiochip_generic_free() unconditionally here. AFAIU, these don't do anything when there are no GPIO ranges defined (so should not interfere with non-pinctrl linked devices), as well as nothing when CONFIG_PINCTRL isn't enabled, so they should be NOPs if there is no pinctrl link, and do the right thing if there is one.

Quoted 2 lines

Best Regards,
Jonas

@Jonas Your proposal is now used in OpenWrt for realtek
https://github.com/openwrt/openwrt/blob/6ef6014887c393dc07f0349028d93e4fa82e0733/target/linux/realtek/patches-6.6/801-gpio-regmap-Use-generic-request-free-ops.patch,
so I guess that we could send that patch to linux-gpio instead.

@Sander can you send that patch to linux-gpio? We need it for bmips too:
https://github.com/openwrt/openwrt/pull/17487

Best regards,
Álvaro.

author
#4 lore raw

PATCH 2/2 pinctrl: bcm63xx: implement gpio_regmap request/free

In reply to Álvaro Fernández Rojas

Implement pinctrl gpio request/free functions on gpio_regmap, which ensures calling gpio_request_enable on bcm63xx pinctrl drivers when a gpio is requested.

Signed-off-by Álvaro Fernández Rojas <[email protected]>
---
 drivers/pinctrl/bcm/pinctrl-bcm63xx.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
drivers/pinctrl/bcm/pinctrl-bcm63xx.c +14 -0
index 59d2ce8462d8..4abd52613dfe 100644
@@ -6,10 +6,12 @@
  * Copyright (C) 2016 Jonas Gorski <[email protected]>
  */
 
+#include <linux/gpio/driver.h>
 #include <linux/gpio/regmap.h>
 #include <linux/mfd/syscon.h>
 #include <linux/mod_devicetable.h>
 #include <linux/of.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 
 #include "pinctrl-bcm63xx.h"

@@ -32,6 +34,16 @@ static int bcm63xx_reg_mask_xlate(struct gpio_regmap *gpio,
 	return 0;
 }
 
+static int bcm63xx_request(struct gpio_chip *chip, unsigned int offset)
+{
+	return pinctrl_gpio_request(chip, offset);
+}
+
+static void bcm63xx_free(struct gpio_chip *chip, unsigned int offset)
+{
+	pinctrl_gpio_free(chip, offset);
+}
+
 static const struct of_device_id bcm63xx_gpio_of_match[] = {
 	{ .compatible = "brcm,bcm6318-gpio", },
 	{ .compatible = "brcm,bcm6328-gpio", },

@@ -57,6 +69,8 @@ static int bcm63xx_gpio_probe(struct device *dev, struct device_node *node,
 	grc.reg_dir_out_base = BCM63XX_DIROUT_REG;
 	grc.reg_set_base = BCM63XX_DATA_REG;
 	grc.reg_mask_xlate = bcm63xx_reg_mask_xlate;
+	grc.request = bcm63xx_request;
+	grc.free = bcm63xx_free;
 
 	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(dev, &grc));
 }
Signature · 1 line
2.39.5
#5 lore raw
In reply to Álvaro Fernández Rojas
Quoted 2 lines · Álvaro Fernández Rojas

Add gpio_chip request/free functions to gpio-regmap and use them on bcm63xx in order to properly call gpio_request_enable when requesting a gpio.

It's a bit hacky, Sander found the silver bullet patch and Bartosz has merged it! But thanks for looking after this issue.

Yours,
Linus Walleij