[PATCH v2 0/2] gpio-mmio: Extend to handle pinctrl back-ends

[PATCH v2 0/2] gpio-mmio: Extend to handle pinctrl back-ends

[Linus Walleij]

If we're using gpio-mmio with a pinctrl backend the
direction callbacks need to finalize their work by
calling into the pin control back-end.

As I was made aware that the vf610 driver was missing
only that to use gpio-mmio instead of custom code,
I took a stab at it.

This patch is made on top of Johan Korsnes bug fix,
so it needs to be applied after that is in, if this
works.

Plese try it out on vf610!

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Use the dual input/output set/clear registers for output.
- Provide the BGPIOF_READ_OUTPUT_REG_SET flag so the driver
  behaves as described in the commit message...
- Drop the now unused spinlock (gpio-mmio has its own).
- Fix a speling mistake.
- Link to v1: https://lore.kernel.org/r/20250214-vf610-mmio-v1-0-6cccd0292e84@linaro.org

---
Linus Walleij (2):
      gpio: mmio: Add flag for calling pinctrl back-end
      gpio: vf610: Switch to gpio-mmio

 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-mmio.c    |  36 +++++++++++----
 drivers/gpio/gpio-vf610.c   | 109 +++++++-------------------------------------
 include/linux/gpio/driver.h |   3 ++
 4 files changed, 49 insertions(+), 100 deletions(-)
---
base-commit: 8e2ad024bbbbd1add00e9ddc4aa943d3a27fa146
change-id: 20250213-vf610-mmio-eddfaeb6b197

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v2 1/2] gpio: mmio: Add flag for calling pinctrl back-end

[Linus Walleij]

It turns out that with this flag we can switch over an entire
driver to use gpio-mmio instead of a bunch of custom code,
also providing get/set_multiple() to it in the process, so it
seems like a reasonable feature to add.

The generic pin control backend requires us to call the
gpiochip_generic_request(), gpiochip_generic_free(),
pinctrl_gpio_direction_output() and pinctrl_gpio_direction_input()
callbacks, so if the new flag for a pin control back-end
is set, we make sure these functions get called as
expected.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-mmio.c    | 36 ++++++++++++++++++++++++++++--------
 include/linux/gpio/driver.h |  3 +++
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d89e78f0ead31f30c014b201cca4e32ecb377118..d4f72a0f6ed89878d64055ab28d888a7be08b126 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -49,6 +49,7 @@ o        `                     ~~~~\___/~~~~    ` controller in FPGA is ,.`
 #include <linux/log2.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
 #include <linux/slab.h>
@@ -323,9 +324,19 @@ static void bgpio_set_multiple_with_clear(struct gpio_chip *gc,
 		gc->write_reg(gc->reg_clr, clear_mask);
 }
 
+static int bgpio_dir_return(struct gpio_chip *gc, unsigned int gpio, bool dir_out)
+{
+	if (!gc->bgpio_pinctrl)
+		return 0;
+	if (dir_out)
+		return pinctrl_gpio_direction_output(gc, gpio);
+	else
+		return pinctrl_gpio_direction_input(gc, gpio);
+}
+
 static int bgpio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
-	return 0;
+	return bgpio_dir_return(gc, gpio, false);
 }
 
 static int bgpio_dir_out_err(struct gpio_chip *gc, unsigned int gpio,
@@ -339,7 +350,7 @@ static int bgpio_simple_dir_out(struct gpio_chip *gc, unsigned int gpio,
 {
 	gc->set(gc, gpio, val);
 
-	return 0;
+	return bgpio_dir_return(gc, gpio, true);
 }
 
 static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
@@ -357,7 +368,7 @@ static int bgpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
 
 	raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
-	return 0;
+	return bgpio_dir_return(gc, gpio, false);
 }
 
 static int bgpio_get_dir(struct gpio_chip *gc, unsigned int gpio)
@@ -403,7 +414,7 @@ static int bgpio_dir_out_dir_first(struct gpio_chip *gc, unsigned int gpio,
 {
 	bgpio_dir_out(gc, gpio, val);
 	gc->set(gc, gpio, val);
-	return 0;
+	return bgpio_dir_return(gc, gpio, true);
 }
 
 static int bgpio_dir_out_val_first(struct gpio_chip *gc, unsigned int gpio,
@@ -411,7 +422,7 @@ static int bgpio_dir_out_val_first(struct gpio_chip *gc, unsigned int gpio,
 {
 	gc->set(gc, gpio, val);
 	bgpio_dir_out(gc, gpio, val);
-	return 0;
+	return bgpio_dir_return(gc, gpio, true);
 }
 
 static int bgpio_setup_accessors(struct device *dev,
@@ -562,10 +573,13 @@ static int bgpio_setup_direction(struct gpio_chip *gc,
 
 static int bgpio_request(struct gpio_chip *chip, unsigned gpio_pin)
 {
-	if (gpio_pin < chip->ngpio)
-		return 0;
+	if (gpio_pin >= chip->ngpio)
+		return -EINVAL;
 
-	return -EINVAL;
+	if (chip->bgpio_pinctrl)
+		return gpiochip_generic_request(chip, gpio_pin);
+
+	return 0;
 }
 
 /**
@@ -632,6 +646,12 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 	if (ret)
 		return ret;
 
+	if (flags & BGPIOF_PINCTRL_BACKEND) {
+		gc->bgpio_pinctrl = true;
+		/* Currently this callback is only used for pincontrol */
+		gc->free = gpiochip_generic_free;
+	}
+
 	gc->bgpio_data = gc->read_reg(gc->reg_dat);
 	if (gc->set == bgpio_set_set &&
 			!(flags & BGPIOF_UNREADABLE_REG_SET))
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 2dd7cb9cc270a68ddedbcdd5d44e0d0f88dfa785..e867d52daaf26827324f9e17b5c19c55978ed7e7 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -394,6 +394,7 @@ struct gpio_irq_chip {
  * @reg_dir_in: direction in setting register for generic GPIO
  * @bgpio_dir_unreadable: indicates that the direction register(s) cannot
  *	be read and we need to rely on out internal state tracking.
+ * @bgpio_pinctrl: the generic GPIO uses a pin control backend.
  * @bgpio_bits: number of register bits used for a generic GPIO i.e.
  *	<register width> * 8
  * @bgpio_lock: used to lock chip->bgpio_data. Also, this is needed to keep
@@ -478,6 +479,7 @@ struct gpio_chip {
 	void __iomem *reg_dir_out;
 	void __iomem *reg_dir_in;
 	bool bgpio_dir_unreadable;
+	bool bgpio_pinctrl;
 	int bgpio_bits;
 	raw_spinlock_t bgpio_lock;
 	unsigned long bgpio_data;
@@ -713,6 +715,7 @@ int bgpio_init(struct gpio_chip *gc, struct device *dev,
 #define BGPIOF_READ_OUTPUT_REG_SET	BIT(4) /* reg_set stores output value */
 #define BGPIOF_NO_OUTPUT		BIT(5) /* only input */
 #define BGPIOF_NO_SET_ON_INPUT		BIT(6)
+#define BGPIOF_PINCTRL_BACKEND		BIT(7) /* Call pinctrl direction setters */
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
 int gpiochip_irqchip_add_domain(struct gpio_chip *gc,

-- 
2.48.1

[PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio

[Linus Walleij]

After adding a pinctrl flag to gpio-mmio we can use it
for driving gpio-vf610.

The existing code has the same semantics and the generic
gpio-mmio, including reading from the data out register
when the direction is set to input, and it can also handle
the absence of the direction register better than the
current driver: we get the direction from the shadow
direction registers in gpio-mmio instead.

Since gpio-mmio has an internal spinlock we can drop the
direction-protecting spinlock from the driver.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Use the dual input/output set/clear registers for output.
- Provide the BGPIOF_READ_OUTPUT_REG_SET flag so the driver
  behaves as described in the commit message...
- Drop the now unused spinlock (gpio-mmio has its own).
---
 drivers/gpio/Kconfig      |   1 +
 drivers/gpio/gpio-vf610.c | 109 ++++++++--------------------------------------
 2 files changed, 18 insertions(+), 92 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index add5ad29a673c09082a913cb2404073b2034af48..ab104ce85ee6cef1549d31744625bcc625d75179 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -756,6 +756,7 @@ config GPIO_VF610
 	default y if SOC_VF610
 	depends on ARCH_MXC || COMPILE_TEST
 	select GPIOLIB_IRQCHIP
+	select GPIO_GENERIC
 	help
 	  Say yes here to support i.MX or Vybrid vf610 GPIOs.
 
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index 3527487d42c8ac3ef39c3be468dd5177c85f6b44..4aa80dae9badbe8a9cc7b2adacf321ed237c3b57 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -36,7 +36,6 @@ struct vf610_gpio_port {
 	struct clk *clk_port;
 	struct clk *clk_gpio;
 	int irq;
-	spinlock_t lock; /* protect gpio direction registers */
 };
 
 #define GPIO_PDOR		0x00
@@ -94,82 +93,6 @@ static inline u32 vf610_gpio_readl(void __iomem *reg)
 	return readl_relaxed(reg);
 }
 
-static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio)
-{
-	struct vf610_gpio_port *port = gpiochip_get_data(gc);
-	u32 mask = BIT(gpio);
-	unsigned long offset = GPIO_PDIR;
-
-	if (port->sdata->have_paddr) {
-		mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-		if (mask)
-			offset = GPIO_PDOR;
-	}
-
-	return !!(vf610_gpio_readl(port->gpio_base + offset) & BIT(gpio));
-}
-
-static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
-{
-	struct vf610_gpio_port *port = gpiochip_get_data(gc);
-	u32 mask = BIT(gpio);
-	unsigned long offset = val ? GPIO_PSOR : GPIO_PCOR;
-
-	vf610_gpio_writel(mask, port->gpio_base + offset);
-}
-
-static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
-{
-	struct vf610_gpio_port *port = gpiochip_get_data(chip);
-	u32 mask = BIT(gpio);
-	unsigned long flags;
-	u32 val;
-
-	if (port->sdata->have_paddr) {
-		spin_lock_irqsave(&port->lock, flags);
-		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-		val &= ~mask;
-		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
-
-	return pinctrl_gpio_direction_input(chip, gpio);
-}
-
-static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned int gpio,
-				       int value)
-{
-	struct vf610_gpio_port *port = gpiochip_get_data(chip);
-	u32 mask = BIT(gpio);
-	unsigned long flags;
-	u32 val;
-
-	vf610_gpio_set(chip, gpio, value);
-
-	if (port->sdata->have_paddr) {
-		spin_lock_irqsave(&port->lock, flags);
-		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-		val |= mask;
-		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
-		spin_unlock_irqrestore(&port->lock, flags);
-	}
-
-	return pinctrl_gpio_direction_output(chip, gpio);
-}
-
-static int vf610_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
-{
-	struct vf610_gpio_port *port = gpiochip_get_data(gc);
-	u32 mask = BIT(gpio);
-
-	mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-
-	if (mask)
-		return GPIO_LINE_DIRECTION_OUT;
-
-	return GPIO_LINE_DIRECTION_IN;
-}
-
 static void vf610_gpio_irq_handler(struct irq_desc *desc)
 {
 	struct vf610_gpio_port *port =
@@ -295,6 +218,7 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 	struct vf610_gpio_port *port;
 	struct gpio_chip *gc;
 	struct gpio_irq_chip *girq;
+	unsigned long flags;
 	int i;
 	int ret;
 	bool dual_base;
@@ -304,7 +228,6 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	port->sdata = device_get_match_data(dev);
-	spin_lock_init(&port->lock);
 
 	dual_base = port->sdata->have_dual_base;
 
@@ -371,23 +294,25 @@ static int vf610_gpio_probe(struct platform_device *pdev)
 	}
 
 	gc = &port->gc;
-	gc->parent = dev;
-	gc->label = dev_name(dev);
-	gc->ngpio = VF610_GPIO_PER_PORT;
-	gc->base = -1;
-
-	gc->request = gpiochip_generic_request;
-	gc->free = gpiochip_generic_free;
-	gc->direction_input = vf610_gpio_direction_input;
-	gc->get = vf610_gpio_get;
-	gc->direction_output = vf610_gpio_direction_output;
-	gc->set = vf610_gpio_set;
+	flags = BGPIOF_PINCTRL_BACKEND;
 	/*
-	 * only IP has Port Data Direction Register(PDDR) can
-	 * support get direction
+	 * We only read the output register for current value on output
+	 * lines if the direction register is available so we can switch
+	 * direction.
 	 */
 	if (port->sdata->have_paddr)
-		gc->get_direction = vf610_gpio_get_direction;
+		flags |= BGPIOF_READ_OUTPUT_REG_SET;
+	ret = bgpio_init(gc, dev, 4,
+			 port->base + GPIO_PDOR,
+			 port->base + GPIO_PSOR,
+			 port->base + GPIO_PCOR,
+			 port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL,
+			 NULL,
+			 flags);
+	if (ret)
+		return dev_err_probe(dev, ret, "unable to init generic GPIO\n");
+	gc->label = dev_name(dev);
+	gc->base = -1;
 
 	/* Mask all GPIO interrupts */
 	for (i = 0; i < gc->ngpio; i++)

-- 
2.48.1

RE: [PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio

[Bough Chen]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 2025年2月15日 7:13
> To: Johan Korsnes <johan.korsnes@remarkable.no>; Bough Chen
> <haibo.chen@nxp.com>; Bartosz Golaszewski <brgl@bgdev.pl>;
> imx@lists.linux.dev
> Cc: linux-gpio@vger.kernel.org; Linus Walleij <linus.walleij@linaro.org>
> Subject: [PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio
> 
> After adding a pinctrl flag to gpio-mmio we can use it for driving gpio-vf610.
> 
> The existing code has the same semantics and the generic gpio-mmio, including
> reading from the data out register when the direction is set to input, and it can
> also handle the absence of the direction register better than the current driver:
> we get the direction from the shadow direction registers in gpio-mmio instead.
> 
> Since gpio-mmio has an internal spinlock we can drop the direction-protecting
> spinlock from the driver.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Use the dual input/output set/clear registers for output.
> - Provide the BGPIOF_READ_OUTPUT_REG_SET flag so the driver
>   behaves as described in the commit message...
> - Drop the now unused spinlock (gpio-mmio has its own).
> ---
>  drivers/gpio/Kconfig      |   1 +
>  drivers/gpio/gpio-vf610.c | 109 ++++++++--------------------------------------
>  2 files changed, 18 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
> add5ad29a673c09082a913cb2404073b2034af48..ab104ce85ee6cef1549d3174
> 4625bcc625d75179 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -756,6 +756,7 @@ config GPIO_VF610
>  	default y if SOC_VF610
>  	depends on ARCH_MXC || COMPILE_TEST
>  	select GPIOLIB_IRQCHIP
> +	select GPIO_GENERIC
>  	help
>  	  Say yes here to support i.MX or Vybrid vf610 GPIOs.
> 
> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index
> 3527487d42c8ac3ef39c3be468dd5177c85f6b44..4aa80dae9badbe8a9cc7b2ad
> acf321ed237c3b57 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -36,7 +36,6 @@ struct vf610_gpio_port {
>  	struct clk *clk_port;
>  	struct clk *clk_gpio;
>  	int irq;
> -	spinlock_t lock; /* protect gpio direction registers */
>  };
> 
>  #define GPIO_PDOR		0x00
> @@ -94,82 +93,6 @@ static inline u32 vf610_gpio_readl(void __iomem *reg)
>  	return readl_relaxed(reg);
>  }
> 
> -static int vf610_gpio_get(struct gpio_chip *gc, unsigned int gpio) -{
> -	struct vf610_gpio_port *port = gpiochip_get_data(gc);
> -	u32 mask = BIT(gpio);
> -	unsigned long offset = GPIO_PDIR;
> -
> -	if (port->sdata->have_paddr) {
> -		mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -		if (mask)
> -			offset = GPIO_PDOR;
> -	}
> -
> -	return !!(vf610_gpio_readl(port->gpio_base + offset) & BIT(gpio));
> -}
> -
> -static void vf610_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val) -{
> -	struct vf610_gpio_port *port = gpiochip_get_data(gc);
> -	u32 mask = BIT(gpio);
> -	unsigned long offset = val ? GPIO_PSOR : GPIO_PCOR;
> -
> -	vf610_gpio_writel(mask, port->gpio_base + offset);
> -}
> -
> -static int vf610_gpio_direction_input(struct gpio_chip *chip, unsigned int gpio)
> -{
> -	struct vf610_gpio_port *port = gpiochip_get_data(chip);
> -	u32 mask = BIT(gpio);
> -	unsigned long flags;
> -	u32 val;
> -
> -	if (port->sdata->have_paddr) {
> -		spin_lock_irqsave(&port->lock, flags);
> -		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -		val &= ~mask;
> -		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
> -		spin_unlock_irqrestore(&port->lock, flags);
> -	}
> -
> -	return pinctrl_gpio_direction_input(chip, gpio);
> -}
> -
> -static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned int
> gpio,
> -				       int value)
> -{
> -	struct vf610_gpio_port *port = gpiochip_get_data(chip);
> -	u32 mask = BIT(gpio);
> -	unsigned long flags;
> -	u32 val;
> -
> -	vf610_gpio_set(chip, gpio, value);
> -
> -	if (port->sdata->have_paddr) {
> -		spin_lock_irqsave(&port->lock, flags);
> -		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -		val |= mask;
> -		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
> -		spin_unlock_irqrestore(&port->lock, flags);
> -	}
> -
> -	return pinctrl_gpio_direction_output(chip, gpio);
> -}
> -
> -static int vf610_gpio_get_direction(struct gpio_chip *gc, unsigned int gpio) -{
> -	struct vf610_gpio_port *port = gpiochip_get_data(gc);
> -	u32 mask = BIT(gpio);
> -
> -	mask &= vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -
> -	if (mask)
> -		return GPIO_LINE_DIRECTION_OUT;
> -
> -	return GPIO_LINE_DIRECTION_IN;
> -}
> -
>  static void vf610_gpio_irq_handler(struct irq_desc *desc)  {
>  	struct vf610_gpio_port *port =
> @@ -295,6 +218,7 @@ static int vf610_gpio_probe(struct platform_device
> *pdev)
>  	struct vf610_gpio_port *port;
>  	struct gpio_chip *gc;
>  	struct gpio_irq_chip *girq;
> +	unsigned long flags;
>  	int i;
>  	int ret;
>  	bool dual_base;
> @@ -304,7 +228,6 @@ static int vf610_gpio_probe(struct platform_device
> *pdev)
>  		return -ENOMEM;
> 
>  	port->sdata = device_get_match_data(dev);
> -	spin_lock_init(&port->lock);
> 
>  	dual_base = port->sdata->have_dual_base;
> 
> @@ -371,23 +294,25 @@ static int vf610_gpio_probe(struct platform_device
> *pdev)
>  	}
> 
>  	gc = &port->gc;
> -	gc->parent = dev;
> -	gc->label = dev_name(dev);
> -	gc->ngpio = VF610_GPIO_PER_PORT;
> -	gc->base = -1;
> -
> -	gc->request = gpiochip_generic_request;
> -	gc->free = gpiochip_generic_free;
> -	gc->direction_input = vf610_gpio_direction_input;
> -	gc->get = vf610_gpio_get;
> -	gc->direction_output = vf610_gpio_direction_output;
> -	gc->set = vf610_gpio_set;
> +	flags = BGPIOF_PINCTRL_BACKEND;
>  	/*
> -	 * only IP has Port Data Direction Register(PDDR) can
> -	 * support get direction
> +	 * We only read the output register for current value on output
> +	 * lines if the direction register is available so we can switch
> +	 * direction.
>  	 */
>  	if (port->sdata->have_paddr)
> -		gc->get_direction = vf610_gpio_get_direction;
> +		flags |= BGPIOF_READ_OUTPUT_REG_SET;
> +	ret = bgpio_init(gc, dev, 4,
> +			 port->base + GPIO_PDOR,
> +			 port->base + GPIO_PSOR,
> +			 port->base + GPIO_PCOR,

Hi Linus,

Two issue here:

1, need to use port->gpio_base instead port->base.

2, vf610 has register GPIO_PDIR, if find the direction is input, need to read this register to get the input value.
But for mmio driver, it will read the gc->reg_set, which is GPIO_PSOR with flags BGPIOF_READ_OUTPUT_REG_SET,  this is not correct. Seems current bgpio_init() need to add another parameter to input this GPIO_PDIR, but this will has big impact. 


I try to adjust the bgpio_init like this:
ret = bgpio_init(gc, dev, 4, port->gpio_base + GPIO_PDIR, port->gpio_base + GPIO_PSOR, port->gpio_base + GPIO_PCOR, port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, NULL, flags);

But still can't work fine, because when gpio is config in output mode, it will read GPIO_PSOR as output value, but for vf610, read GPIO_PSOR/GPIO_PCOR already return 0

root@imx943evk:/unit_tests# ./memtool 0x43840048=0x00100000  (write PCOR)
Writing 32-bit value 0x100000 to address 0x43840048
root@imx943evk:/unit_tests# ./memtool 0x43840040 7        (check PCOR after write, it is 0, but find bit 20 of PDOR change to 0, means the PCOR logic works)
E
Reading 0x7 count starting at address 0x43840040

0x43840040:  00000000 00000000 00000000 00000000
0x43840050:  0000000C 08100000 00000000

root@imx943evk:/unit_tests# ./memtool 0x43840044=0x00100000  (write PSOR)
Writing 32-bit value 0x100000 to address 0x43840044
root@imx943evk:/unit_tests# ./memtool 0x43840040 7       (check PSOR after write, it is 0, but find bit 20 of PDOR change to 1, means the PSOR logic works)
E
Reading 0x7 count starting at address 0x43840040

0x43840040:  00100000 00000000 00000000 00000000
0x43840050:  0000000C 08100000 00000000


Regards
Haibo Chen

> +			 port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL,
> +			 NULL,
> +			 flags);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to init generic GPIO\n");
> +	gc->label = dev_name(dev);
> +	gc->base = -1;
> 
>  	/* Mask all GPIO interrupts */
>  	for (i = 0; i < gc->ngpio; i++)
> 
> --
> 2.48.1

Re: [PATCH v2 2/2] gpio: vf610: Switch to gpio-mmio

[Linus Walleij]

Hi Bough,

thanks for testing all my patches!

On Mon, Feb 17, 2025 at 12:24 PM Bough Chen <haibo.chen@nxp.com> wrote:

> 1, need to use port->gpio_base instead port->base.

Oh, what a mistake. Fixed.

> 2, vf610 has register GPIO_PDIR, if find the direction is input, need to read this register to get the input value.
> But for mmio driver, it will read the gc->reg_set, which is GPIO_PSOR with flags
> BGPIOF_READ_OUTPUT_REG_SET,  this is not correct. Seems current bgpio_init()
> need to add another parameter to input this GPIO_PDIR, but this will has big impact.
>
> I try to adjust the bgpio_init like this:
> ret = bgpio_init(gc, dev, 4, port->gpio_base + GPIO_PDIR, port->gpio_base + GPIO_PSOR,
>  port->gpio_base + GPIO_PCOR, port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL, NULL, flags);
>
> But still can't work fine, because when gpio is config in output mode, it will read GPIO_PSOR
> as output value, but for vf610, read GPIO_PSOR/GPIO_PCOR already return 0

But what happens if we *only* specify PSOR as setting register to bgpio?

Usually when there are set/clear registers, the block also allows direct
read/modify/write on the output data register (I have seen this once
before...)

I sent a v3 like that. Does this work?

Else I think we should perhaps simply bite the bullet and add a new
argument to bgpio_init() for passing set/clear/data triplets. It can't
be avoided: if this sort of hardware does exist with set and clear
and a non-writeable data register, then gpio-mmio should support
it.

Yours,
Linus Walleij