[PATCH] gpio: vf610: add locking to gpio direction functions

[PATCH] gpio: vf610: add locking to gpio direction functions

[Johan Korsnes]

Add locking to `vf610_gpio_direction_input|output()` functions. Without
this locking, a race condition exists between concurrent calls to these
functions, potentially leading to incorrect GPIO direction settings.

Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Cc: Haibo Chen <haibo.chen@nxp.com>
Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>

---
To verify the correctness of this fix, a `trylock` patch was applied,
where after a couple of reboots the race was confirmed. I.e., one user
had to wait before acquiring the lock. With this patch the race has not
been encountered. It's worth mentioning that any type of debugging
(printing, tracing, etc.) would "resolve" the issue.
---
 drivers/gpio/gpio-vf610.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpio/gpio-vf610.c b/drivers/gpio/gpio-vf610.c
index c4f34a347cb6..3527487d42c8 100644
--- a/drivers/gpio/gpio-vf610.c
+++ b/drivers/gpio/gpio-vf610.c
@@ -36,6 +36,7 @@ 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
@@ -121,12 +122,15 @@ 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);
@@ -137,14 +141,17 @@ static int vf610_gpio_direction_output(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;
 
 	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);
@@ -297,6 +304,7 @@ 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;
 
-- 
2.43.0

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Bartosz Golaszewski]

On Thu, 6 Feb 2025 at 19:17, Johan Korsnes <johan.korsnes@remarkable.no> wrote:
>
> Add locking to `vf610_gpio_direction_input|output()` functions. Without
> this locking, a race condition exists between concurrent calls to these
> functions, potentially leading to incorrect GPIO direction settings.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Please use the email address that pops up when you call
scripts/get_maintainer.pl for patch submission next time.

Bart

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Linus Walleij]

Hi Johan,

thanks for your patch!

On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes
<johan.korsnes@remarkable.no> wrote:

> Add locking to `vf610_gpio_direction_input|output()` functions. Without
> this locking, a race condition exists between concurrent calls to these
> functions, potentially leading to incorrect GPIO direction settings.
>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Cc: Haibo Chen <haibo.chen@nxp.com>
> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>

Looks correct to me, verified by looking at the most tested
driver gpio-mmio.c and seeing there is a lock there indeed.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> where after a couple of reboots the race was confirmed. I.e., one user
> had to wait before acquiring the lock. With this patch the race has not
> been encountered. It's worth mentioning that any type of debugging
> (printing, tracing, etc.) would "resolve" the issue.

Typical. I would include this in the commit message, people care.

Looking at the driver it seems vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
could have a similar issue, both write the same register.

Both issues could be fixed by converting the driver to use
gpio-mmio() with bgpio_init() which would also implement
get/set_multiple support for free.

I have no idea why this driver isn't using gpio-mmio.
Not your fault though, just pointing out obvious improvement
opportunities.

Yours,
Linus Walleij

RE: [PATCH] gpio: vf610: add locking to gpio direction functions

[Bough Chen]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@linaro.org>
> Sent: 2025年2月7日 2:29
> To: Johan Korsnes <johan.korsnes@remarkable.no>
> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
> 
> Hi Johan,
> 
> thanks for your patch!
> 
> On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes <johan.korsnes@remarkable.no>
> wrote:
> 
> > Add locking to `vf610_gpio_direction_input|output()` functions.
> > Without this locking, a race condition exists between concurrent calls
> > to these functions, potentially leading to incorrect GPIO direction settings.
> >
> > Cc: Linus Walleij <linus.walleij@linaro.org>
> > Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Cc: Haibo Chen <haibo.chen@nxp.com>
> > Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>
> 
> Looks correct to me, verified by looking at the most tested driver gpio-mmio.c
> and seeing there is a lock there indeed.
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> > where after a couple of reboots the race was confirmed. I.e., one user
> > had to wait before acquiring the lock. With this patch the race has
> > not been encountered. It's worth mentioning that any type of debugging
> > (printing, tracing, etc.) would "resolve" the issue.
> 
> Typical. I would include this in the commit message, people care.
> 
> Looking at the driver it seems vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
> could have a similar issue, both write the same register.

Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().

> 
> Both issues could be fixed by converting the driver to use
> gpio-mmio() with bgpio_init() which would also implement get/set_multiple
> support for free.
> 
> I have no idea why this driver isn't using gpio-mmio.
> Not your fault though, just pointing out obvious improvement opportunities.

I check the code, for vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let the input/output really works, need to call pinctrl_gpio_direction_input() for vf610/imx7ulp/imx8ulp SoC.
Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
This should be the reason why not using gpio-mmio.

Regards
Haibo Chen
> 
> Yours,
> Linus Walleij

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Johan Korsnes]

On 2/7/25 7:21 AM, Bough Chen wrote:
>> -----Original Message-----
>> From: Linus Walleij <linus.walleij@linaro.org>
>> Sent: 2025年2月7日 2:29
>> To: Johan Korsnes <johan.korsnes@remarkable.no>
>> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
>> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
>> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
>>
>> Hi Johan,
>>
>> thanks for your patch!
>>
>> On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes <johan.korsnes@remarkable.no>
>> wrote:
>>
>>> Add locking to `vf610_gpio_direction_input|output()` functions.
>>> Without this locking, a race condition exists between concurrent calls
>>> to these functions, potentially leading to incorrect GPIO direction settings.
>>>
>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> Cc: Haibo Chen <haibo.chen@nxp.com>
>>> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>
>>
>> Looks correct to me, verified by looking at the most tested driver gpio-mmio.c
>> and seeing there is a lock there indeed.
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>
>>> where after a couple of reboots the race was confirmed. I.e., one user
>>> had to wait before acquiring the lock. With this patch the race has
>>> not been encountered. It's worth mentioning that any type of debugging
>>> (printing, tracing, etc.) would "resolve" the issue.
>>
>> Typical. I would include this in the commit message, people care.
>>

Hi Linus and Haibo,

Thanks for the review! I'll include this in v2.

>> Looking at the driver it seems vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
>> could have a similar issue, both write the same register.
> 
> Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().
> 

Could you please explain the race condition we fix by adding locking to
these other functions? F.ex. the vf610_gpio_set(), in which scenario would
the lack of locking cause an issue? It's a single write to either the set
or clear register. Is this related to how the writel_relaxed() works on
different architectures?

Kind regards,
Johan

>>
>> Both issues could be fixed by converting the driver to use
>> gpio-mmio() with bgpio_init() which would also implement get/set_multiple
>> support for free.
>>
>> I have no idea why this driver isn't using gpio-mmio.
>> Not your fault though, just pointing out obvious improvement opportunities.
> 
> I check the code, for vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let the input/output really works, need to call pinctrl_gpio_direction_input() for vf610/imx7ulp/imx8ulp SoC.
> Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
> This should be the reason why not using gpio-mmio.
> 
> Regards
> Haibo Chen
>>
>> Yours,
>> Linus Walleij

RE: [PATCH] gpio: vf610: add locking to gpio direction functions

[Bough Chen]

> -----Original Message-----
> From: Johan Korsnes <johan.korsnes@remarkable.no>
> Sent: 2025年2月10日 16:53
> To: Bough Chen <haibo.chen@nxp.com>; Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org>; imx@lists.linux.dev
> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
> 
> [You don't often get email from johan.korsnes@remarkable.no. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On 2/7/25 7:21 AM, Bough Chen wrote:
> >> -----Original Message-----
> >> From: Linus Walleij <linus.walleij@linaro.org>
> >> Sent: 2025年2月7日 2:29
> >> To: Johan Korsnes <johan.korsnes@remarkable.no>
> >> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> >> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
> >> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction
> >> functions
> >>
> >> Hi Johan,
> >>
> >> thanks for your patch!
> >>
> >> On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes
> >> <johan.korsnes@remarkable.no>
> >> wrote:
> >>
> >>> Add locking to `vf610_gpio_direction_input|output()` functions.
> >>> Without this locking, a race condition exists between concurrent
> >>> calls to these functions, potentially leading to incorrect GPIO direction
> settings.
> >>>
> >>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>> Cc: Haibo Chen <haibo.chen@nxp.com>
> >>> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>
> >>
> >> Looks correct to me, verified by looking at the most tested driver
> >> gpio-mmio.c and seeing there is a lock there indeed.
> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>
> >>> where after a couple of reboots the race was confirmed. I.e., one
> >>> user had to wait before acquiring the lock. With this patch the race
> >>> has not been encountered. It's worth mentioning that any type of
> >>> debugging (printing, tracing, etc.) would "resolve" the issue.
> >>
> >> Typical. I would include this in the commit message, people care.
> >>
> 
> Hi Linus and Haibo,
> 
> Thanks for the review! I'll include this in v2.
> 
> >> Looking at the driver it seems
> >> vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
> >> could have a similar issue, both write the same register.
> >
> > Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().
> >
> 
> Could you please explain the race condition we fix by adding locking to these
> other functions? F.ex. the vf610_gpio_set(), in which scenario would the lack of
> locking cause an issue? It's a single write to either the set or clear register. Is this
> related to how the writel_relaxed() works on different architectures?
> 

Sorry, I think twice of this condition, seems no need to add lock for these functions.

Regards
Haibo Chen

> Kind regards,
> Johan
> 
> >>
> >> Both issues could be fixed by converting the driver to use
> >> gpio-mmio() with bgpio_init() which would also implement
> >> get/set_multiple support for free.
> >>
> >> I have no idea why this driver isn't using gpio-mmio.
> >> Not your fault though, just pointing out obvious improvement opportunities.
> >
> > I check the code, for
> vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let the
> input/output really works, need to call pinctrl_gpio_direction_input() for
> vf610/imx7ulp/imx8ulp SoC.
> > Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement
> gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
> > This should be the reason why not using gpio-mmio.
> >
> > Regards
> > Haibo Chen
> >>
> >> Yours,
> >> Linus Walleij

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Johan Korsnes]

On 2/10/25 10:35 AM, Bough Chen wrote:
>> -----Original Message-----
>> From: Johan Korsnes <johan.korsnes@remarkable.no>
>> Sent: 2025年2月10日 16:53
>> To: Bough Chen <haibo.chen@nxp.com>; Linus Walleij <linus.walleij@linaro.org>
>> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
>> <bartosz.golaszewski@linaro.org>; imx@lists.linux.dev
>> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
>>
>> [You don't often get email from johan.korsnes@remarkable.no. Learn why this is
>> important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> On 2/7/25 7:21 AM, Bough Chen wrote:
>>>> -----Original Message-----
>>>> From: Linus Walleij <linus.walleij@linaro.org>
>>>> Sent: 2025年2月7日 2:29
>>>> To: Johan Korsnes <johan.korsnes@remarkable.no>
>>>> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
>>>> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
>>>> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction
>>>> functions
>>>>
>>>> Hi Johan,
>>>>
>>>> thanks for your patch!
>>>>
>>>> On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes
>>>> <johan.korsnes@remarkable.no>
>>>> wrote:
>>>>
>>>>> Add locking to `vf610_gpio_direction_input|output()` functions.
>>>>> Without this locking, a race condition exists between concurrent
>>>>> calls to these functions, potentially leading to incorrect GPIO direction
>> settings.
>>>>>
>>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
>>>>> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>>> Cc: Haibo Chen <haibo.chen@nxp.com>
>>>>> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>
>>>>
>>>> Looks correct to me, verified by looking at the most tested driver
>>>> gpio-mmio.c and seeing there is a lock there indeed.
>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>>>
>>>>> where after a couple of reboots the race was confirmed. I.e., one
>>>>> user had to wait before acquiring the lock. With this patch the race
>>>>> has not been encountered. It's worth mentioning that any type of
>>>>> debugging (printing, tracing, etc.) would "resolve" the issue.
>>>>
>>>> Typical. I would include this in the commit message, people care.
>>>>
>>
>> Hi Linus and Haibo,
>>
>> Thanks for the review! I'll include this in v2.
>>
>>>> Looking at the driver it seems
>>>> vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
>>>> could have a similar issue, both write the same register.
>>>
>>> Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().
>>>
>>
>> Could you please explain the race condition we fix by adding locking to these
>> other functions? F.ex. the vf610_gpio_set(), in which scenario would the lack of
>> locking cause an issue? It's a single write to either the set or clear register. Is this
>> related to how the writel_relaxed() works on different architectures?
>>
> 
> Sorry, I think twice of this condition, seems no need to add lock for these functions.
> 

Hi Haibo,

May I add your Reviewed-by or Acked-by in v2?

Kind regards,
Johan

> Regards
> Haibo Chen
> 
>> Kind regards,
>> Johan
>>
>>>>
>>>> Both issues could be fixed by converting the driver to use
>>>> gpio-mmio() with bgpio_init() which would also implement
>>>> get/set_multiple support for free.
>>>>
>>>> I have no idea why this driver isn't using gpio-mmio.
>>>> Not your fault though, just pointing out obvious improvement opportunities.
>>>
>>> I check the code, for
>> vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let the
>> input/output really works, need to call pinctrl_gpio_direction_input() for
>> vf610/imx7ulp/imx8ulp SoC.
>>> Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement
>> gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
>>> This should be the reason why not using gpio-mmio.
>>>
>>> Regards
>>> Haibo Chen
>>>>
>>>> Yours,
>>>> Linus Walleij
> 

RE: [PATCH] gpio: vf610: add locking to gpio direction functions

[Bough Chen]

> -----Original Message-----
> From: Johan Korsnes <johan.korsnes@remarkable.no>
> Sent: 2025年2月13日 14:13
> To: Bough Chen <haibo.chen@nxp.com>; Linus Walleij <linus.walleij@linaro.org>
> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> <bartosz.golaszewski@linaro.org>; imx@lists.linux.dev
> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction functions
> 
> On 2/10/25 10:35 AM, Bough Chen wrote:
> >> -----Original Message-----
> >> From: Johan Korsnes <johan.korsnes@remarkable.no>
> >> Sent: 2025年2月10日 16:53
> >> To: Bough Chen <haibo.chen@nxp.com>; Linus Walleij
> >> <linus.walleij@linaro.org>
> >> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> >> <bartosz.golaszewski@linaro.org>; imx@lists.linux.dev
> >> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction
> >> functions
> >>
> >> [You don't often get email from johan.korsnes@remarkable.no. Learn
> >> why this is important at
> >> https://aka.ms/LearnAboutSenderIdentification ]
> >>
> >> On 2/7/25 7:21 AM, Bough Chen wrote:
> >>>> -----Original Message-----
> >>>> From: Linus Walleij <linus.walleij@linaro.org>
> >>>> Sent: 2025年2月7日 2:29
> >>>> To: Johan Korsnes <johan.korsnes@remarkable.no>
> >>>> Cc: linux-gpio@vger.kernel.org; Bartosz Golaszewski
> >>>> <bartosz.golaszewski@linaro.org>; Bough Chen <haibo.chen@nxp.com>
> >>>> Subject: Re: [PATCH] gpio: vf610: add locking to gpio direction
> >>>> functions
> >>>>
> >>>> Hi Johan,
> >>>>
> >>>> thanks for your patch!
> >>>>
> >>>> On Thu, Feb 6, 2025 at 7:17 PM Johan Korsnes
> >>>> <johan.korsnes@remarkable.no>
> >>>> wrote:
> >>>>
> >>>>> Add locking to `vf610_gpio_direction_input|output()` functions.
> >>>>> Without this locking, a race condition exists between concurrent
> >>>>> calls to these functions, potentially leading to incorrect GPIO
> >>>>> direction
> >> settings.
> >>>>>
> >>>>> Cc: Linus Walleij <linus.walleij@linaro.org>
> >>>>> Cc: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>>> Cc: Haibo Chen <haibo.chen@nxp.com>
> >>>>> Signed-off-by: Johan Korsnes <johan.korsnes@remarkable.no>
> >>>>
> >>>> Looks correct to me, verified by looking at the most tested driver
> >>>> gpio-mmio.c and seeing there is a lock there indeed.
> >>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> >>>>
> >>>>> where after a couple of reboots the race was confirmed. I.e., one
> >>>>> user had to wait before acquiring the lock. With this patch the
> >>>>> race has not been encountered. It's worth mentioning that any type
> >>>>> of debugging (printing, tracing, etc.) would "resolve" the issue.
> >>>>
> >>>> Typical. I would include this in the commit message, people care.
> >>>>
> >>
> >> Hi Linus and Haibo,
> >>
> >> Thanks for the review! I'll include this in v2.
> >>
> >>>> Looking at the driver it seems
> >>>> vf610_gpio_irq_mask()/vf610_gpio_irq_unmask()
> >>>> could have a similar issue, both write the same register.
> >>>
> >>> Indeed, and also the vf610_gpio_set() / vf610_gpio_irq_ack().
> >>>
> >>
> >> Could you please explain the race condition we fix by adding locking
> >> to these other functions? F.ex. the vf610_gpio_set(), in which
> >> scenario would the lack of locking cause an issue? It's a single
> >> write to either the set or clear register. Is this related to how the
> writel_relaxed() works on different architectures?
> >>
> >
> > Sorry, I think twice of this condition, seems no need to add lock for these
> functions.
> >
> 
> Hi Haibo,
> 
> May I add your Reviewed-by or Acked-by in v2?

Yes, sure.

Regards
Haibo Chen
> 
> Kind regards,
> Johan
> 
> > Regards
> > Haibo Chen
> >
> >> Kind regards,
> >> Johan
> >>
> >>>>
> >>>> Both issues could be fixed by converting the driver to use
> >>>> gpio-mmio() with bgpio_init() which would also implement
> >>>> get/set_multiple support for free.
> >>>>
> >>>> I have no idea why this driver isn't using gpio-mmio.
> >>>> Not your fault though, just pointing out obvious improvement
> opportunities.
> >>>
> >>> I check the code, for
> >> vf610_gpio_direction_input()/vf610_gpio_direction_output(), to let
> >> the input/output really works, need to call
> >> pinctrl_gpio_direction_input() for vf610/imx7ulp/imx8ulp SoC.
> >>> Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement
> >> gpio_set_direction callback. Also for imx7ulp/imx8ulp pinctrl drivers.
> >>> This should be the reason why not using gpio-mmio.
> >>>
> >>> Regards
> >>> Haibo Chen
> >>>>
> >>>> Yours,
> >>>> Linus Walleij
> >

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Linus Walleij]

On Fri, Feb 7, 2025 at 7:21 AM Bough Chen <haibo.chen@nxp.com> wrote:
[Me]:
> > I have no idea why this driver isn't using gpio-mmio.
> > Not your fault though, just pointing out obvious improvement opportunities.
>
> I check the code, for vf610_gpio_direction_input()/vf610_gpio_direction_output(),
> to let the input/output really works, need to call pinctrl_gpio_direction_input()
> for vf610/imx7ulp/imx8ulp SoC.
> Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement gpio_set_direction
> callback. Also for imx7ulp/imx8ulp pinctrl drivers.
> This should be the reason why not using gpio-mmio.

If this is the *only* thing you need additionally from gpio-mmio
then what about just adding a new flag to bgpio_init() function
in <linux/gpio/driver.h>:

#define BGPIOF_PINCTRL_BACKEND      BIT(7)

That makes gpio-mmio call these callbacks when setting
direction?

Yours,
Linus Walleij

Re: [PATCH] gpio: vf610: add locking to gpio direction functions

[Linus Walleij]

On Thu, Feb 13, 2025 at 10:45 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Feb 7, 2025 at 7:21 AM Bough Chen <haibo.chen@nxp.com> wrote:
> [Me]:
> > > I have no idea why this driver isn't using gpio-mmio.
> > > Not your fault though, just pointing out obvious improvement opportunities.
> >
> > I check the code, for vf610_gpio_direction_input()/vf610_gpio_direction_output(),
> > to let the input/output really works, need to call pinctrl_gpio_direction_input()
> > for vf610/imx7ulp/imx8ulp SoC.
> > Refer to drivers/pinctrl/freescale/pinctrl-vf610.c, it implement gpio_set_direction
> > callback. Also for imx7ulp/imx8ulp pinctrl drivers.
> > This should be the reason why not using gpio-mmio.
>
> If this is the *only* thing you need additionally from gpio-mmio
> then what about just adding a new flag to bgpio_init() function
> in <linux/gpio/driver.h>:
>
> #define BGPIOF_PINCTRL_BACKEND      BIT(7)
>
> That makes gpio-mmio call these callbacks when setting
> direction?

It's easier if I demonstrate.

I wrote patches to do this, will send them now, test it out!

Yours,
Linus Walleij