| Message ID | 20240722062809.915867-1-haibo.chen@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: vf610: add get_direction() support | expand |
Hi Haibo, Am 22.07.24 um 08:28 schrieb haibo.chen@nxp.com: > From: Haibo Chen <haibo.chen@nxp.com> > > For IP which do not contain PDDR, currently use the pinmux API > pinctrl_gpio_direction_input() to config the output/input, pinmux > currently do not support get_direction(). So here add the GPIO > get_direction() support only for the IP which has Port Data > Direction Register (PDDR). > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > --- > drivers/gpio/gpio-vf610.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c > index 07e5e6323e86..08ca8377b19c 100644 > --- a/drivers/gpio/gpio-vf610.c > +++ b/drivers/gpio/gpio-vf610.c > @@ -151,6 +151,19 @@ static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, > 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); > + unsigned long mask = BIT(gpio); thanks for sending this patch. I'm fine with this patch, but could we use u32 to make it clear about the range of the mask? Regards > + > + 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 = > @@ -362,6 +375,12 @@ static int vf610_gpio_probe(struct platform_device *pdev) > 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++)
> -----Original Message----- > From: Stefan Wahren <wahrenst@gmx.net> > Sent: 2024年7月22日 18:32 > To: Bough Chen <haibo.chen@nxp.com>; linus.walleij@linaro.org; > brgl@bgdev.pl > Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; > imx@lists.linux.dev > Subject: Re: [PATCH] gpio: vf610: add get_direction() support > > Hi Haibo, > > Am 22.07.24 um 08:28 schrieb haibo.chen@nxp.com: > > From: Haibo Chen <haibo.chen@nxp.com> > > > > For IP which do not contain PDDR, currently use the pinmux API > > pinctrl_gpio_direction_input() to config the output/input, pinmux > > currently do not support get_direction(). So here add the GPIO > > get_direction() support only for the IP which has Port Data Direction > > Register (PDDR). > > > > Signed-off-by: Haibo Chen <haibo.chen@nxp.com> > > --- > > drivers/gpio/gpio-vf610.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c > > index 07e5e6323e86..08ca8377b19c 100644 > > --- a/drivers/gpio/gpio-vf610.c > > +++ b/drivers/gpio/gpio-vf610.c > > @@ -151,6 +151,19 @@ static int vf610_gpio_direction_output(struct > gpio_chip *chip, unsigned gpio, > > 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); > > + unsigned long mask = BIT(gpio); > thanks for sending this patch. I'm fine with this patch, but could we use u32 to > make it clear about the range of the mask? Yes, u32 seems more clear here, but I notice all other place use unsigned long, so I keep the same code style. I go through the history of this driver, seems no specific explanation about the unsigned long. If anyone know the reason, please comment. Best Regards Haibo Chen > > Regards > > + > > + 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 = > > @@ -362,6 +375,12 @@ static int vf610_gpio_probe(struct platform_device > *pdev) > > 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++)
Am 23.07.24 um 04:45 schrieb Bough Chen: >> -----Original Message----- >> From: Stefan Wahren <wahrenst@gmx.net> >> Sent: 2024年7月22日 18:32 >> To: Bough Chen <haibo.chen@nxp.com>; linus.walleij@linaro.org; >> brgl@bgdev.pl >> Cc: linux-gpio@vger.kernel.org; linux-kernel@vger.kernel.org; >> imx@lists.linux.dev >> Subject: Re: [PATCH] gpio: vf610: add get_direction() support >> >> Hi Haibo, >> >> Am 22.07.24 um 08:28 schrieb haibo.chen@nxp.com: >>> From: Haibo Chen <haibo.chen@nxp.com> >>> >>> For IP which do not contain PDDR, currently use the pinmux API >>> pinctrl_gpio_direction_input() to config the output/input, pinmux >>> currently do not support get_direction(). So here add the GPIO >>> get_direction() support only for the IP which has Port Data Direction >>> Register (PDDR). >>> >>> Signed-off-by: Haibo Chen <haibo.chen@nxp.com> >>> --- >>> drivers/gpio/gpio-vf610.c | 19 +++++++++++++++++++ >>> 1 file changed, 19 insertions(+) >>> >>> diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c >>> index 07e5e6323e86..08ca8377b19c 100644 >>> --- a/drivers/gpio/gpio-vf610.c >>> +++ b/drivers/gpio/gpio-vf610.c >>> @@ -151,6 +151,19 @@ static int vf610_gpio_direction_output(struct >> gpio_chip *chip, unsigned gpio, >>> 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); >>> + unsigned long mask = BIT(gpio); >> thanks for sending this patch. I'm fine with this patch, but could we use u32 to >> make it clear about the range of the mask? > Yes, u32 seems more clear here, but I notice all other place use unsigned long, so I keep the same code style. > I go through the history of this driver, seems no specific explanation about the unsigned long. I understand, but i don't think there is a reason for using unsigned long. So maybe this is a good opportunity to clean this up. Regards
diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c index 07e5e6323e86..08ca8377b19c 100644 --- a/drivers/gpio/gpio-vf610.c +++ b/drivers/gpio/gpio-vf610.c @@ -151,6 +151,19 @@ static int vf610_gpio_direction_output(struct gpio_chip *chip, unsigned gpio, 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); + unsigned long 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 = @@ -362,6 +375,12 @@ static int vf610_gpio_probe(struct platform_device *pdev) 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++)