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

[PATCH v3 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 (v3),
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 v3:
- Use the port->gpio_base for accessing GPIO registers.
- Specify only GPOR for setting/clearing/reading the
  output, hoping a RMW on that register works with MMIO.
- Rebased on the applied v3 version of Johan's fix patch.
- Link to v2: https://lore.kernel.org/r/20250215-vf610-mmio-v2-0-4a91f8c8e8d5@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   | 105 +++++++-------------------------------------
 include/linux/gpio/driver.h |   3 ++
 4 files changed, 49 insertions(+), 96 deletions(-)
---
base-commit: f751bf0670cbb166c58e99d57373765405178426
change-id: 20250213-vf610-mmio-eddfaeb6b197

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

[PATCH v3 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

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

[Andy Shevchenko]

Wed, Feb 19, 2025 at 10:04:33PM +0100, Linus Walleij kirjoitti:
> 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.

First of all, I like the series and esp. the second patch, thanks!
One small comment below, though.

...

>  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;
>  }

While I understand the desire to avoid +LoCs, I still think it's better from
maintenance p.o.v. to have a symmetry in APIs, i.e. providing

bgpio_free()  // or whatever name suits with possibility to change the above
{
	if (chip->bgpio_pinctrl)
		gpiochip_generic_free(...);
}

...

> +	if (flags & BGPIOF_PINCTRL_BACKEND) {
> +		gc->bgpio_pinctrl = true;
> +		/* Currently this callback is only used for pincontrol */
> +		gc->free = gpiochip_generic_free;
> +	}

And

	gc->free = gpiochip_generic_free;
	...
	if (flags & BGPIOF_PINCTRL_BACKEND)
		gc->bgpio_pinctrl = true;

here.

The rationale that if we ever add something to the request part, we won't
forget to call it in the free part.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 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 v2->v3:
- Use the port->gpio_base for accessing GPIO registers.
- Specify only GPOR for setting/clearing/reading the
  output, hoping a RMW on that register works with MMIO.
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 | 105 ++++++++--------------------------------------
 2 files changed, 18 insertions(+), 88 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 c36a9dbccd4dd5415ed3b90b87afb47e7634c025..4dad7ce0c4dc6a3f412081c7502c0ce6dd5740da 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,78 +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);
-	u32 val;
-
-	if (port->sdata->have_paddr) {
-		guard(spinlock_irqsave)(&port->lock);
-		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-		val &= ~mask;
-		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
-	}
-
-	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);
-	u32 val;
-
-	vf610_gpio_set(chip, gpio, value);
-
-	if (port->sdata->have_paddr) {
-		guard(spinlock_irqsave)(&port->lock);
-		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
-		val |= mask;
-		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
-	}
-
-	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 =
@@ -291,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;
@@ -300,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;
 
@@ -367,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->gpio_base + GPIO_PDIR,
+			 port->gpio_base + GPIO_PDOR,
+			 NULL,
+			 port->sdata->have_paddr ? port->gpio_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 v3 2/2] gpio: vf610: Switch to gpio-mmio

[Bough Chen]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 2025年2月20日 5:05
> 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 v3 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.
> 

I test on imx8ulp-evk and imx943-evk board, gpio works fine. GPIO interrupt also works well.

Reviewed-and-tested-by: Haibo Chen <haibo.chen@nxp.com>

Regards
Haibo Chen
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v2->v3:
> - Use the port->gpio_base for accessing GPIO registers.
> - Specify only GPOR for setting/clearing/reading the
>   output, hoping a RMW on that register works with MMIO.
> 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 | 105 ++++++++--------------------------------------
>  2 files changed, 18 insertions(+), 88 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
> c36a9dbccd4dd5415ed3b90b87afb47e7634c025..4dad7ce0c4dc6a3f412081c7
> 502c0ce6dd5740da 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,78 +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);
> -	u32 val;
> -
> -	if (port->sdata->have_paddr) {
> -		guard(spinlock_irqsave)(&port->lock);
> -		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -		val &= ~mask;
> -		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
> -	}
> -
> -	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);
> -	u32 val;
> -
> -	vf610_gpio_set(chip, gpio, value);
> -
> -	if (port->sdata->have_paddr) {
> -		guard(spinlock_irqsave)(&port->lock);
> -		val = vf610_gpio_readl(port->gpio_base + GPIO_PDDR);
> -		val |= mask;
> -		vf610_gpio_writel(val, port->gpio_base + GPIO_PDDR);
> -	}
> -
> -	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 =
> @@ -291,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;
> @@ -300,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;
> 
> @@ -367,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->gpio_base + GPIO_PDIR,
> +			 port->gpio_base + GPIO_PDOR,
> +			 NULL,
> +			 port->sdata->have_paddr ? port->gpio_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 v3 2/2] gpio: vf610: Switch to gpio-mmio

[Bartosz Golaszewski]

On Fri, Feb 21, 2025 at 4:58 AM Bough Chen <haibo.chen@nxp.com> wrote:
>
> > -----Original Message-----
> > From: Linus Walleij <linus.walleij@linaro.org>
> > Sent: 2025年2月20日 5:05
> > 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 v3 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.
> >
>
> I test on imx8ulp-evk and imx943-evk board, gpio works fine. GPIO interrupt also works well.
>
> Reviewed-and-tested-by: Haibo Chen <haibo.chen@nxp.com>
>

Please don't do this, b4 doesn't pick up things like that. Leave
separate tags for reviews and tests.

Bart

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

[Bough Chen]

> -----Original Message-----
> From: Bartosz Golaszewski <brgl@bgdev.pl>
> Sent: 2025年2月21日 18:17
> To: Bough Chen <haibo.chen@nxp.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Johan Korsnes
> <johan.korsnes@remarkable.no>; imx@lists.linux.dev;
> linux-gpio@vger.kernel.org
> Subject: Re: [PATCH v3 2/2] gpio: vf610: Switch to gpio-mmio
> 
> On Fri, Feb 21, 2025 at 4:58 AM Bough Chen <haibo.chen@nxp.com> wrote:
> >
> > > -----Original Message-----
> > > From: Linus Walleij <linus.walleij@linaro.org>
> > > Sent: 2025年2月20日 5:05
> > > 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 v3 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.
> > >
> >
> > I test on imx8ulp-evk and imx943-evk board, gpio works fine. GPIO interrupt
> also works well.
> >
> > Reviewed-and-tested-by: Haibo Chen <haibo.chen@nxp.com>
> >
> 
> Please don't do this, b4 doesn't pick up things like that. Leave separate tags for
> reviews and tests.

Okay, get it. Please use these tags, I will follow this rule next time.

For this patch set:

Reviewed-by: Haibo Chen <haibo.chen@nxp.com>
Tested-by: Haibo Chen <haibo.chen@nxp.com>


Regards
Haibo Chen
> 
> Bart

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

[Johan Korsnes]

On 2/19/25 10:04 PM, Linus Walleij wrote:
> 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.
> 

Hi,

Could the following headers be dropped?

#include <linux/pinctrl/consumer.h>
#include <linux/spinlock.h>

Kind regards,
Johan

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

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Wed, 19 Feb 2025 22:04:32 +0100, Linus Walleij wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/2] gpio: mmio: Add flag for calling pinctrl back-end
      commit: 2145ba374069ee8edc9d29c2a6b56fe4a28a6e2d
[2/2] gpio: vf610: Switch to gpio-mmio
      commit: da5dd31efd2465ccc9a70a85bdc325e394256689

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>