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

[Bough Chen <haibo.chen@xxxxxxx>]

+ imx global review list. Maybe someone may have comments.

Regards
Haibo Chen

> -----Original Message-----
> From: Bough Chen
> Sent: 2025年2月14日 19:28
> To: Linus Walleij <linus.walleij@xxxxxxxxxx>; Johan Korsnes
> <johan.korsnes@xxxxxxxxxxxxx>; Bartosz Golaszewski <brgl@xxxxxxxx>
> Cc: linux-gpio@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/2] gpio: vg610: Switch to gpio-mmio
> 
> > -----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..ab104ce85ee6cef1549d31
> 74
> > 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..858ced17ae5e5f2726ccb85
> 22
> > 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