1. Patchew
  2. linux
  3. [PATCH v5 0/4] gpio: rockchip: Update the GPIO driver
  4. View patch

[PATCH v5 4/4] gpio: rockchip: Set input direction when request irq

Ye Zhang posted 4 patches 1 week, 4 days ago
[PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Ye Zhang 1 week, 4 days ago 
Since the GPIO can only generate interrupts when its direction is set to
input, it is set to input before requesting the interrupt resources.

Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 drivers/gpio/gpio-rockchip.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
index c090cac694bf..cdfdd5501a1e 100644
--- a/drivers/gpio/gpio-rockchip.c
+++ b/drivers/gpio/gpio-rockchip.c
@@ -476,8 +476,11 @@ static int rockchip_irq_reqres(struct irq_data *d)
 {
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct rockchip_pin_bank *bank = gc->private;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
 
-	return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
+	rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);
+
+	return gpiochip_reqres_irq(&bank->gpio_chip, hwirq);
 }
 
 static void rockchip_irq_relres(struct irq_data *d)
-- 
2.34.1
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Bartosz Golaszewski 1 week, 4 days ago 
On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
>
> Since the GPIO can only generate interrupts when its direction is set to
> input, it is set to input before requesting the interrupt resources.
>
> Signed-off-by: Ye Zhang <ye.zhang@rock-chips.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> ---
>  drivers/gpio/gpio-rockchip.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index c090cac694bf..cdfdd5501a1e 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -476,8 +476,11 @@ static int rockchip_irq_reqres(struct irq_data *d)
>  {
>         struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
>         struct rockchip_pin_bank *bank = gc->private;
> +       irq_hw_number_t hwirq = irqd_to_hwirq(d);
>
> -       return gpiochip_reqres_irq(&bank->gpio_chip, d->hwirq);
> +       rockchip_gpio_direction_input(&bank->gpio_chip, hwirq);
> +
> +       return gpiochip_reqres_irq(&bank->gpio_chip, hwirq);
>  }
>
>  static void rockchip_irq_relres(struct irq_data *d)
> --
> 2.34.1
>

This looks like a fix to me, do you want it sent for stable? If so,
please add the Fixes tag and put it first in the series.

Bart
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Andy Shevchenko 1 week, 4 days ago 
On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
> >
> > Since the GPIO can only generate interrupts when its direction is set to
> > input, it is set to input before requesting the interrupt resources.

...

> This looks like a fix to me, do you want it sent for stable? If so,
> please add the Fixes tag and put it first in the series.

Independently on the resolution on this, can the first three be applied to
for-next? I think they are valuable from the documentation perspective as
it adds the explanation of the version register bit fields.

The last one seems to me independent (code wise, meaning no potential
conflicts) to the rest and may be applied to for-current later on.

-- 
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Bartosz Golaszewski 1 week, 4 days ago 
On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
> > >
> > > Since the GPIO can only generate interrupts when its direction is set to
> > > input, it is set to input before requesting the interrupt resources.
>
> ...
>
> > This looks like a fix to me, do you want it sent for stable? If so,
> > please add the Fixes tag and put it first in the series.
>
> Independently on the resolution on this, can the first three be applied to
> for-next? I think they are valuable from the documentation perspective as
> it adds the explanation of the version register bit fields.
>
> The last one seems to me independent (code wise, meaning no potential
> conflicts) to the rest and may be applied to for-current later on.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

There's another issue I see with this patch. It effectively changes
the pin's direction behind the back of the GPIOLIB. If a GPIO is
requested, its direction set to output and another orthogonal user
requests the same pin as input, we'll never update the FLAG_IS_OUT
value and I don't think any subsequent behavior can be considered
defined.

I applied the first 3 patches as they look alright.

Bart
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Bartosz Golaszewski 1 week, 4 days ago 
On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
> > > >
> > > > Since the GPIO can only generate interrupts when its direction is set to
> > > > input, it is set to input before requesting the interrupt resources.
> >
> > ...
> >
> > > This looks like a fix to me, do you want it sent for stable? If so,
> > > please add the Fixes tag and put it first in the series.
> >
> > Independently on the resolution on this, can the first three be applied to
> > for-next? I think they are valuable from the documentation perspective as
> > it adds the explanation of the version register bit fields.
> >
> > The last one seems to me independent (code wise, meaning no potential
> > conflicts) to the rest and may be applied to for-current later on.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >
>
> There's another issue I see with this patch. It effectively changes
> the pin's direction behind the back of the GPIOLIB. If a GPIO is
> requested, its direction set to output and another orthogonal user
> requests the same pin as input, we'll never update the FLAG_IS_OUT

I meant to say "same pin as interrupt". Sorry for the noise.

Bart

> value and I don't think any subsequent behavior can be considered
> defined.
>
> I applied the first 3 patches as they look alright.
>
> Bart
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Sebastian Reichel 1 week, 4 days ago 
Hi,

On Tue, Nov 12, 2024 at 01:53:48PM +0100, Bartosz Golaszewski wrote:
> On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
> > > > >
> > > > > Since the GPIO can only generate interrupts when its direction is set to
> > > > > input, it is set to input before requesting the interrupt resources.
> > >
> > > ...
> > >
> > > > This looks like a fix to me, do you want it sent for stable? If so,
> > > > please add the Fixes tag and put it first in the series.
> > >
> > > Independently on the resolution on this, can the first three be applied to
> > > for-next? I think they are valuable from the documentation perspective as
> > > it adds the explanation of the version register bit fields.
> > >
> > > The last one seems to me independent (code wise, meaning no potential
> > > conflicts) to the rest and may be applied to for-current later on.
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
> > >
> > >
> >
> > There's another issue I see with this patch. It effectively changes
> > the pin's direction behind the back of the GPIOLIB. If a GPIO is
> > requested, its direction set to output and another orthogonal user
> > requests the same pin as input, we'll never update the FLAG_IS_OUT
> 
> I meant to say "same pin as interrupt". Sorry for the noise.

GPIO output and interrupt at the same time looks like a misconfiguration
to me. Maybe check for that situation and return -EBUSY?

-- Sebastian

> 
> Bart
> 
> > value and I don't think any subsequent behavior can be considered
> > defined.
> >
> > I applied the first 3 patches as they look alright.
> >
> > Bart
Re: [PATCH v5 4/4] gpio: rockchip: Set input direction when request irq
Posted by Bartosz Golaszewski 1 week, 4 days ago 
On Tue, Nov 12, 2024 at 2:23 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Tue, Nov 12, 2024 at 01:53:48PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Nov 12, 2024 at 1:50 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Tue, Nov 12, 2024 at 11:40 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > > On Tue, Nov 12, 2024 at 09:48:06AM +0100, Bartosz Golaszewski wrote:
> > > > > On Tue, Nov 12, 2024 at 2:54 AM Ye Zhang <ye.zhang@rock-chips.com> wrote:
> > > > > >
> > > > > > Since the GPIO can only generate interrupts when its direction is set to
> > > > > > input, it is set to input before requesting the interrupt resources.
> > > >
> > > > ...
> > > >
> > > > > This looks like a fix to me, do you want it sent for stable? If so,
> > > > > please add the Fixes tag and put it first in the series.
> > > >
> > > > Independently on the resolution on this, can the first three be applied to
> > > > for-next? I think they are valuable from the documentation perspective as
> > > > it adds the explanation of the version register bit fields.
> > > >
> > > > The last one seems to me independent (code wise, meaning no potential
> > > > conflicts) to the rest and may be applied to for-current later on.
> > > >
> > > > --
> > > > With Best Regards,
> > > > Andy Shevchenko
> > > >
> > > >
> > >
> > > There's another issue I see with this patch. It effectively changes
> > > the pin's direction behind the back of the GPIOLIB. If a GPIO is
> > > requested, its direction set to output and another orthogonal user
> > > requests the same pin as input, we'll never update the FLAG_IS_OUT
> >
> > I meant to say "same pin as interrupt". Sorry for the noise.
>
> GPIO output and interrupt at the same time looks like a misconfiguration
> to me. Maybe check for that situation and return -EBUSY?
>

Of course it is. That's why we check for it in
gpiod_direction_output() and in gpiochip_lock_as_irq(): refuse to set
direction to output if the GPIO is used as interrupt and refuse to use
it as interrupt if direction already is output respectively.

This patch however proposes to set the direction to input implicitly
which is wrong. The underlying gpiochip_reqres_irq() will check the
current direction and return -EIO if it's output.

Bart

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.