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

[Bough Chen <haibo.chen@xxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: 2025年2月14日 7:09
> To: Johan Korsnes <johan.korsnes@xxxxxxxxxxxxx>; Bough Chen
> <haibo.chen@xxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx; Linus Walleij <linus.walleij@xxxxxxxxxx>
> Subject: [PATCH 2/2] gpio: vg610: 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.
> 

Hi Linus,

I did a quick test for these two patches, meet some issue.

One pin on board is used as SD card detect, no matter I insert the card or not, gpioget tool always get inactive state, this wrong. Seems driver can't get correct input data.

I check the code, seems vf610 has another limitation, refer to the original code logic in vf610_gpio_get():
If the gpio is config as input, need to read GPIO_PDIR to get the input data, if gpio is config at output, need to read GPIO_PDOR.

But for bgpio_init, we fix GPIO_PDOR to void __iomem *dat

Regards
Haibo Chen

 
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig      |   1 +
>  drivers/gpio/gpio-vf610.c | 100 +++++-----------------------------------------
>  2 files changed, 10 insertions(+), 91 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..858ced17ae5e5f2726ccb8522
> cd3e4b6a5041d99 100644
> --- a/drivers/gpio/gpio-vf610.c
> +++ b/drivers/gpio/gpio-vf610.c
> @@ -94,82 +94,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 =
> @@ -371,24 +295,18 @@ static int vf610_gpio_probe(struct platform_device
> *pdev)
>  	}
> 
>  	gc = &port->gc;
> -	gc->parent = dev;
> +	ret = bgpio_init(gc, dev, 4,
> +			 port->base + GPIO_PDOR,
> +			 port->base + GPIO_PCOR,
> +			 NULL,
> +			 port->sdata->have_paddr ? port->base + GPIO_PDDR : NULL,
> +			 NULL,
> +			 BGPIOF_PINCTRL_BACKEND);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "unable to init generic GPIO\n");
>  	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;
> -	/*
> -	 * only IP has Port Data Direction Register(PDDR) can
> -	 * support get direction
> -	 */
> -	if (port->sdata->have_paddr)
> -		gc->get_direction = vf610_gpio_get_direction;
> -
>  	/* Mask all GPIO interrupts */
>  	for (i = 0; i < gc->ngpio; i++)
>  		vf610_gpio_writel(0, port->base + PORT_PCR(i));
> 
> --
> 2.48.1