| Message ID | 20241112015408.3139996-5-ye.zhang@rock-chips.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: rockchip: Update the GPIO driver | expand |
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
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.
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
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
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
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
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)