[RFC] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq()

Message ID 8c586fd0-7deb-82bf-4c3b-3e2d0a3d1738@xs4all.nl
State New
Headers show
Series
  • [RFC] gpiolib: call gpiochip_(un)lock_as_irq for dis/enable_irq()
Related show

Commit Message

Hans Verkuil July 6, 2018, 11:25 a.m.
If a gpio pin has an interrupt associated with it, but you need to set the
direction of the pin to output, then what you want to do is to disable the
interrupt and change direction to output. And after changing the direction
back to input, then you call enable_irq again.

This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
is first requested and gpiod_direction_output() checks for that. So the
only option is to free the interrupt and re-request it, which is painful.

This RFC patch sets hooks that allow the driver to know when the irq is
disabled and enabled and it sets FLAG_USED_AS_IRQ accordingly.

It works, but I have one open question:

When gpiochip_irq_enable() is called, I currently just WARN if
gpiochip_lock_as_irq() fails and continue after that. It really is
a driver bug, but I wonder if it is also possible to automatically
change the direction to input before enabling the interrupt. I have no
idea how to do this, though, since gpiod_direction_input() needs a
struct gpio_desc whereas in irq_enable() I have a struct gpio_chip.

This patch would be really useful for two CEC drivers:

In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and
re-request the irq (cec_gpio_dis/enable_irq()). This driver implements
low-level support for the HDMI CEC bus. When waiting for something to
happen the gpio pin is set to input with an interrupt to avoid polling.
When writing to the bus it is set to output and the interrupt obviously
has to be removed (or, as I would like to see, just disabled). Requesting
an irq can fail, and handling that is really problematic. Just disabling
and enabling is much easier and cleaner.

The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the
tda998x_cec_calibration() function: this is also a CEC driver but here
the tda998x interrupt pin is overloaded: when calibrating the CEC line
the interrupt pin of the chip is used as a gpio calibration output signal.

This driver disables the interrupt and switches the direction to output
before doing the calibration. This fails for the BeagleBone board unless
I apply this patch. This driver was originally developed for the dove-cubox
board which uses this gpio driver: drivers/gpio/gpio-mvebu.c. For some
reason this worked fine (i.e. it is apparently possible to disable the irq
and change the output without running into the FLAG_USED_AS_IRQ check), but
I don't understand why. I don't have this hardware, so I can't test it.

I'm a newbie when it comes to gpio, so I have no idea how much sense this
patch makes.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Hans Verkuil July 12, 2018, 1:02 p.m. | #1
Ping?

Regards,

	Hans

On 06/07/18 13:25, Hans Verkuil wrote:
> If a gpio pin has an interrupt associated with it, but you need to set the
> direction of the pin to output, then what you want to do is to disable the
> interrupt and change direction to output. And after changing the direction
> back to input, then you call enable_irq again.
> 
> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
> is first requested and gpiod_direction_output() checks for that. So the
> only option is to free the interrupt and re-request it, which is painful.
> 
> This RFC patch sets hooks that allow the driver to know when the irq is
> disabled and enabled and it sets FLAG_USED_AS_IRQ accordingly.
> 
> It works, but I have one open question:
> 
> When gpiochip_irq_enable() is called, I currently just WARN if
> gpiochip_lock_as_irq() fails and continue after that. It really is
> a driver bug, but I wonder if it is also possible to automatically
> change the direction to input before enabling the interrupt. I have no
> idea how to do this, though, since gpiod_direction_input() needs a
> struct gpio_desc whereas in irq_enable() I have a struct gpio_chip.
> 
> This patch would be really useful for two CEC drivers:
> 
> In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and
> re-request the irq (cec_gpio_dis/enable_irq()). This driver implements
> low-level support for the HDMI CEC bus. When waiting for something to
> happen the gpio pin is set to input with an interrupt to avoid polling.
> When writing to the bus it is set to output and the interrupt obviously
> has to be removed (or, as I would like to see, just disabled). Requesting
> an irq can fail, and handling that is really problematic. Just disabling
> and enabling is much easier and cleaner.
> 
> The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the
> tda998x_cec_calibration() function: this is also a CEC driver but here
> the tda998x interrupt pin is overloaded: when calibrating the CEC line
> the interrupt pin of the chip is used as a gpio calibration output signal.
> 
> This driver disables the interrupt and switches the direction to output
> before doing the calibration. This fails for the BeagleBone board unless
> I apply this patch. This driver was originally developed for the dove-cubox
> board which uses this gpio driver: drivers/gpio/gpio-mvebu.c. For some
> reason this worked fine (i.e. it is apparently possible to disable the irq
> and change the output without running into the FLAG_USED_AS_IRQ check), but
> I don't understand why. I don't have this hardware, so I can't test it.
> 
> I'm a newbie when it comes to gpio, so I have no idea how much sense this
> patch makes.
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e11a3bb03820..dcdb457b83b0 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1822,6 +1822,20 @@ static void gpiochip_irq_relres(struct irq_data *d)
>  	module_put(chip->gpiodev->owner);
>  }
> 
> +static void gpiochip_irq_enable(struct irq_data *d)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +	WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
> +}
> +
> +static void gpiochip_irq_disable(struct irq_data *d)
> +{
> +	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +	gpiochip_unlock_as_irq(chip, d->hwirq);
> +}
> +
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
>  	if (!gpiochip_irqchip_irq_valid(chip, offset))
> @@ -1897,6 +1911,10 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>  	    !irqchip->irq_release_resources) {
>  		irqchip->irq_request_resources = gpiochip_irq_reqres;
>  		irqchip->irq_release_resources = gpiochip_irq_relres;
> +		if (!irqchip->irq_enable && !irqchip->irq_disable) {
> +			irqchip->irq_enable = gpiochip_irq_enable;
> +			irqchip->irq_disable = gpiochip_irq_disable;
> +		}
>  	}
> 
>  	if (gpiochip->irq.parent_handler) {
> @@ -2056,6 +2074,10 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
>  	    !irqchip->irq_release_resources) {
>  		irqchip->irq_request_resources = gpiochip_irq_reqres;
>  		irqchip->irq_release_resources = gpiochip_irq_relres;
> +		if (!irqchip->irq_enable && !irqchip->irq_disable) {
> +			irqchip->irq_enable = gpiochip_irq_enable;
> +			irqchip->irq_disable = gpiochip_irq_disable;
> +		}
>  	}
> 
>  	acpi_gpiochip_request_interrupts(gpiochip);
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 18, 2018, 7:59 a.m. | #2
On Fri, Jul 6, 2018 at 1:25 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> If a gpio pin has an interrupt associated with it, but you need to set the
> direction of the pin to output, then what you want to do is to disable the
> interrupt and change direction to output. And after changing the direction
> back to input, then you call enable_irq again.

OK so that kind of works already, but you want to make it more
seamless and convenient (which is nice).

> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
> is first requested and gpiod_direction_output() checks for that. So the
> only option is to free the interrupt and re-request it, which is painful.

It's not the only option (hehe). If you study:
drivers/w1/masters/w1-gpio.c
you see that this uses gpiod_set_raw_value() which will ignore
all flags and protection and just drive the line.

In the W1 case this is done to "recharge" the line.

So when you know exactly what you're doing it's fine to just
drive the line.

> When gpiochip_irq_enable() is called, I currently just WARN if
> gpiochip_lock_as_irq() fails and continue after that. It really is
> a driver bug, but I wonder if it is also possible to automatically
> change the direction to input before enabling the interrupt.

I think I initially had gpiod_lock_as_irq() set direction to
input unless it was already but there was some problem
with that.

>I have no
> idea how to do this, though, since gpiod_direction_input() needs a
> struct gpio_desc whereas in irq_enable() I have a struct gpio_chip.

That is not working anyway, you need to handle this on a gpio_desc
level.

> In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and
> re-request the irq (cec_gpio_dis/enable_irq()). This driver implements
> low-level support for the HDMI CEC bus. When waiting for something to
> happen the gpio pin is set to input with an interrupt to avoid polling.
> When writing to the bus it is set to output and the interrupt obviously
> has to be removed (or, as I would like to see, just disabled). Requesting
> an irq can fail, and handling that is really problematic. Just disabling
> and enabling is much easier and cleaner.

Yeah. But it works, kind of atleast...

> The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the
> tda998x_cec_calibration() function: this is also a CEC driver but here
> the tda998x interrupt pin is overloaded: when calibrating the CEC line
> the interrupt pin of the chip is used as a gpio calibration output signal.

That sounds like you should use the raw accessor in this case.

> +static void gpiochip_irq_enable(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
> +}
> +
> +static void gpiochip_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
> +
> +       gpiochip_unlock_as_irq(chip, d->hwirq);
> +}

This will not work other than with drivers that use GPIOLIB_IRQCHIP
right? Since there are a lot of drivers that don't used that,
this is not going to work.

The key is that not all GPIO controllers enumerate their IRQs from
zero, and some have "holes" in the IRQ or GPIO rangem so
d->hwirq is not generally the same as the GPIO offset, it just happens
to be like that in many cases, but it is a hardware specific.

> +               if (!irqchip->irq_enable && !irqchip->irq_disable) {
> +                       irqchip->irq_enable = gpiochip_irq_enable;
> +                       irqchip->irq_disable = gpiochip_irq_disable;
> +               }

So these hooks would have to be in the irqchip driver and opt-in.
But I don't know about that.

The lock_as_irq() callback is in the irqchip .request_resources since
it is the only safe slowpath call from irqchip.

I initially had these hooks in startup/shutdown IRQ but it didn't work
because almost all irqchip callbacks are implicitly callable from
fastpath IIRC.

See commits:
commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
"genirq: Provide irq_request/release_resources chip callbacks"
commit 57ef04288abd27a717287a652d324f95cb77c3c6
"gpio: switch drivers to use new callback"

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil July 18, 2018, 8:56 a.m. | #3
On 18/07/18 09:59, Linus Walleij wrote:
> On Fri, Jul 6, 2018 at 1:25 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> If a gpio pin has an interrupt associated with it, but you need to set the
>> direction of the pin to output, then what you want to do is to disable the
>> interrupt and change direction to output. And after changing the direction
>> back to input, then you call enable_irq again.
> 
> OK so that kind of works already, but you want to make it more
> seamless and convenient (which is nice).
> 
>> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
>> is first requested and gpiod_direction_output() checks for that. So the
>> only option is to free the interrupt and re-request it, which is painful.
> 
> It's not the only option (hehe). If you study:
> drivers/w1/masters/w1-gpio.c
> you see that this uses gpiod_set_raw_value() which will ignore
> all flags and protection and just drive the line.
> 
> In the W1 case this is done to "recharge" the line.
> 
> So when you know exactly what you're doing it's fine to just
> drive the line.

But the problem is not with setting the gpio, it's with changing the
direction.

I just noticed that there is a gpiod_direction_output_raw() as well
that skips the IRQ test, but gpiod_direction_output() does more things
than gpiod_direction_output_raw(), in particular it is calling
gpio_set_drive_single_ended().

It looks like I can work around this by:

1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended()
   is called.
2) call gpiod_direction_input() so it is safe to add the interrupt.
3) call request_irq
4) If I now want to switch from input to output I can disable the irq
   and call gpiod_direction_output_raw() and use gpiod_is_active_low() to
   determine whether active is high or low (since I still need that).

I don't see where gpiod_set_raw_value() comes in. In fact, it seems that
gpiod_set_value() just changes the direction automatically and that there
is no need to explicitly call gpiod_direction_output() at all, except once
during initialization to ensure that gpio_set_drive_single_ended() is called.
If true, then in point 4 above I can just call gpiod_set_value and drop the
gpiod_direction_output()/_input() calls altogether.

Or am I missing something?

Note: the CEC pin used in cec-gpio.c is open drain.

> 
>> When gpiochip_irq_enable() is called, I currently just WARN if
>> gpiochip_lock_as_irq() fails and continue after that. It really is
>> a driver bug, but I wonder if it is also possible to automatically
>> change the direction to input before enabling the interrupt.
> 
> I think I initially had gpiod_lock_as_irq() set direction to
> input unless it was already but there was some problem
> with that.
> 
>> I have no
>> idea how to do this, though, since gpiod_direction_input() needs a
>> struct gpio_desc whereas in irq_enable() I have a struct gpio_chip.
> 
> That is not working anyway, you need to handle this on a gpio_desc
> level.

I suspected as much, but it is good to have confirmation of this.

> 
>> In drivers/media/platform/cec-gpio/cec-gpio.c I have to free and
>> re-request the irq (cec_gpio_dis/enable_irq()). This driver implements
>> low-level support for the HDMI CEC bus. When waiting for something to
>> happen the gpio pin is set to input with an interrupt to avoid polling.
>> When writing to the bus it is set to output and the interrupt obviously
>> has to be removed (or, as I would like to see, just disabled). Requesting
>> an irq can fail, and handling that is really problematic. Just disabling
>> and enabling is much easier and cleaner.
> 
> Yeah. But it works, kind of atleast...
> 
>> The other driver is drivers/gpu/drm/i2c/tda998x_drv.c, specifically the
>> tda998x_cec_calibration() function: this is also a CEC driver but here
>> the tda998x interrupt pin is overloaded: when calibrating the CEC line
>> the interrupt pin of the chip is used as a gpio calibration output signal.
> 
> That sounds like you should use the raw accessor in this case.
> 
>> +static void gpiochip_irq_enable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
>> +}
>> +
>> +static void gpiochip_irq_disable(struct irq_data *d)
>> +{
>> +       struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
>> +
>> +       gpiochip_unlock_as_irq(chip, d->hwirq);
>> +}
> 
> This will not work other than with drivers that use GPIOLIB_IRQCHIP
> right? Since there are a lot of drivers that don't used that,
> this is not going to work.

Correct. But it is this specific case that causes the problem.
If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations,
then those should be fixed there as well, i.e. by adding irq_enable/irq_disable
hooks.

Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver
for cec-gpio might be the tegra driver.

> The key is that not all GPIO controllers enumerate their IRQs from
> zero, and some have "holes" in the IRQ or GPIO rangem so
> d->hwirq is not generally the same as the GPIO offset, it just happens
> to be like that in many cases, but it is a hardware specific.
> 
>> +               if (!irqchip->irq_enable && !irqchip->irq_disable) {
>> +                       irqchip->irq_enable = gpiochip_irq_enable;
>> +                       irqchip->irq_disable = gpiochip_irq_disable;
>> +               }
> 
> So these hooks would have to be in the irqchip driver and opt-in.
> But I don't know about that.
> 
> The lock_as_irq() callback is in the irqchip .request_resources since
> it is the only safe slowpath call from irqchip.
> 
> I initially had these hooks in startup/shutdown IRQ but it didn't work
> because almost all irqchip callbacks are implicitly callable from
> fastpath IIRC.
> 
> See commits:
> commit c1bacbae8192dd2a9ebadd22d793b68054f6c6e5
> "genirq: Provide irq_request/release_resources chip callbacks"
> commit 57ef04288abd27a717287a652d324f95cb77c3c6
> "gpio: switch drivers to use new callback"

If the proposed work-around above works, then I'm happy to use that.
Alternatively we could make a gpiod_direction_output_ignore_irq()
call that just skips the FLAG_USED_AS_IRQ check.

Regards,

	Hans

> 
> Yours,
> Linus Walleij
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 20, 2018, 8:06 a.m. | #4
On Wed, Jul 18, 2018 at 10:56 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:

> >> This does not work today since FLAG_USED_AS_IRQ is set when the interrupt
> >> is first requested and gpiod_direction_output() checks for that. So the
> >> only option is to free the interrupt and re-request it, which is painful.
> >
> > It's not the only option (hehe). If you study:
> > drivers/w1/masters/w1-gpio.c
> > you see that this uses gpiod_set_raw_value() which will ignore
> > all flags and protection and just drive the line.
> >
> > In the W1 case this is done to "recharge" the line.
> >
> > So when you know exactly what you're doing it's fine to just
> > drive the line.
>
> But the problem is not with setting the gpio, it's with changing the
> direction.

So I was thinking the only reason to change the direction from
input to output would be to drive a value on the output, so then
you could request the line as input and use the raw accessor to
anyways actively drive the input when it needs driving.

> I just noticed that there is a gpiod_direction_output_raw() as well
> that skips the IRQ test, but gpiod_direction_output() does more things
> than gpiod_direction_output_raw(), in particular it is calling
> gpio_set_drive_single_ended().

So what happens is that you want some of the semantic checks
in gpiolib and not all of them.

What we want to do is achieve a 1-1 mapping between the
semantics and your usecase.

I'm trying to figure out what is the general usecase that is lurking
behind this, because I hardly think it is restricted to CEC.

> It looks like I can work around this by:
>
> 1) calling gpiod_direction_output() to ensure gpio_set_drive_single_ended()
>    is called.
> 2) call gpiod_direction_input() so it is safe to add the interrupt.
> 3) call request_irq
> 4) If I now want to switch from input to output I can disable the irq
>    and call gpiod_direction_output_raw() and use gpiod_is_active_low() to
>    determine whether active is high or low (since I still need that).

Yeah that starts to bypass all the semantics in the core.

If this works I guess go for it, I'm just worried about that it
starts to look a bit duct-tape-y.

I think the core of your problem is really that the IRQchip
abstraction is not providing an easy way to unhinge the IRQ
on the line (including unlocking the line as IRQ in gpiolib) so
you can dynamically switch the IRQ on and off.

Are we trying to work around an irqchip shortcoming in gpiolib?

> I don't see where gpiod_set_raw_value() comes in.

I just inverted your usecase and made input the default mode
and output the (raw) exception instead of the other way around.
Have you tried this? Or is is counter-intuitive?

> In fact, it seems that
> gpiod_set_value() just changes the direction automatically and that there
> is no need to explicitly call gpiod_direction_output() at all,

gpiod_set_value() does not change the direction in the
general sense.

I guess you refer to the open drain code that will use direction
change to input as a way to drive the line high (through the
pull-up resistor on the line).

This does not happen if the chip has an explicit .set_config()
callback that can set a special bit (or so) to make the hardware
natively support open drain. Switching the line to input is
just open drain emulation by using the high impedance state
(input mode) as open drain.

> except once
> during initialization to ensure that gpio_set_drive_single_ended() is called.
> If true, then in point 4 above I can just call gpiod_set_value and drop the
> gpiod_direction_output()/_input() calls altogether.

This seems fragile. If you are relying on the "switch to input mode"
as described above, you are both exploiting gpiolib semantics
and the fact that your particular GPIO controller does not have
native open drain support.

> > This will not work other than with drivers that use GPIOLIB_IRQCHIP
> > right? Since there are a lot of drivers that don't used that,
> > this is not going to work.
>
> Correct. But it is this specific case that causes the problem.
> If either of these CEC drivers are used in non-GPIOLIB_IRQCHIP situations,
> then those should be fixed there as well, i.e. by adding irq_enable/irq_disable
> hooks.
>
> Looking at 'git grep gpiochip_lock_as_irq' the only relevant gpio driver
> for cec-gpio might be the tegra driver.

Yeah ... but I think that if we add a semantic like that we need to
implement it in all chips. Users will start to expect that they
can always do this. Or you will soon see "why is cec-gpio not
working for me now!?" and "use another GPIO controller" is not
going to be a good answer to that. So it seems like a support
disaster. The CEC GPIO driver is supposed to be generic I
think.

> If the proposed work-around above works, then I'm happy to use that.
> Alternatively we could make a gpiod_direction_output_ignore_irq()
> call that just skips the FLAG_USED_AS_IRQ check.

It all seems pretty duct-tapey.

Isn't the problem that attaching/removing the IRQ from the GPIO
line is heavy/hard to do?

So you are essentially trying to work around a problem with
irqchip by duct-taping around with new semantics in gpiolib?

Maybe it is better if we discuss this with the irqchip maintainers
in that case?

I've been told to use the .irq_request_resources() and
.irq_release_resources() in struct irq_chip to call
.gpiochip_lock_as_irq() and .gpiochip_unlock_as_irq().

But when you look at these functions:
gpiochip_unlock_as_irq() and gpiochip_unlock_as_irq(),
there is nothing that requires slowpath in them. They can
be called with a spinlock held and IRQs disabled.

I think the real problem is that GPIOLIB_IRQCHIP, that
many irqchips use, use module_get()/put() which are slowpath:

static int gpiochip_irq_reqres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        if (!try_module_get(chip->gpiodev->owner))
                return -ENODEV;

        if (gpiochip_lock_as_irq(chip, d->hwirq)) {
                chip_err(chip,
                        "unable to lock HW IRQ %lu for IRQ\n",
                        d->hwirq);
                module_put(chip->gpiodev->owner);
                return -EINVAL;
        }
        return 0;
}

static void gpiochip_irq_relres(struct irq_data *d)
{
        struct gpio_chip *chip = irq_data_get_irq_chip_data(d);

        gpiochip_unlock_as_irq(chip, d->hwirq);
        module_put(chip->gpiodev->owner);
}

This leads to the situtation that you can only attach/remove
an IRQ from a GPIO line by a whole cycle of [devm_]request_irq()
[devm_]free_irq().

You would probably think it's fine if you didn't have to do this
heavy lifting and could just request them on probe and
call enable_irq() and disable_irq() when needed, and have these
call down into the irq_chip .irq_enable() and .irq_disable() and
lock the GPIO line as input *there* instead, and then change
this everywhere (yes patch all gpio_chips with an irqchip
driver in that case).

As drivers have likely sometimes already assigned function
pointers to .irq_enable() and .irq_disable() these have to
be copied and stored in struct gpio_irq_chip() by
gpiochip_set_cascaded_irqchip() and called in sequence.
But it can be done.

What about changing the GPIOLIB_IRQCHIP code to just
do the module_get()/put() part on .irq_request_resources()
and move the locking to the .irq_enable()/.irq_disable()
callbacks so that we can enable and disable irqs on the fly
in a better way? (BIG WIN!)

What about going and reinvestigating this instead?

I'm sorry that I can't present any simple solution, but hey,
I presented a really complicated solution instead, isn't it
great! :D

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil July 21, 2018, 8:46 a.m. | #5
Hi Linus,

On 20/07/18 10:06, Linus Walleij wrote:

<snip>

> You would probably think it's fine if you didn't have to do this
> heavy lifting and could just request them on probe and
> call enable_irq() and disable_irq() when needed, and have these
> call down into the irq_chip .irq_enable() and .irq_disable() and
> lock the GPIO line as input *there* instead, and then change
> this everywhere (yes patch all gpio_chips with an irqchip
> driver in that case).
> 
> As drivers have likely sometimes already assigned function
> pointers to .irq_enable() and .irq_disable() these have to
> be copied and stored in struct gpio_irq_chip() by
> gpiochip_set_cascaded_irqchip() and called in sequence.
> But it can be done.
> 
> What about changing the GPIOLIB_IRQCHIP code to just
> do the module_get()/put() part on .irq_request_resources()
> and move the locking to the .irq_enable()/.irq_disable()
> callbacks so that we can enable and disable irqs on the fly
> in a better way? (BIG WIN!)
> 
> What about going and reinvestigating this instead?
> 
> I'm sorry that I can't present any simple solution, but hey,
> I presented a really complicated solution instead, isn't it
> great! :D

I did do some investigation into this, but it made me very unhappy:
it's a lot of low-level changes in a lot of drivers for a lot of
different boards, some of which are probably hard to test. Scary.

But I've come up with a much simpler alternative: add two new
gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.

An RFC patch is below (documentation is missing, but it works fine).

Basically if you have a gpio which has an interrupt, then you can use
these functions to disable and enable it. It enables/disables the interrupt
and calls gpiochip_(un)lock_as_irq() as well.

It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure
that the direction is set correctly. I have not done so, but if you think it is
useful, then it's trivial to do. Right now it will return an error if the
direction is still set to output.

If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not
sure if that's a bug or a feature. It's easy enough to change.

Let me know what you think. IMHO this avoids a huge amount of churn in code you
really do no want to touch and it does exactly what you want it to do. And it
looks pretty clean as well, both in gpiolib and in the drivers.

Regards,

	Hans

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/gpio/gpiolib.c        | 83 ++++++++++++++++++++++++++---------
 include/linux/gpio/consumer.h |  3 ++
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..75ad6177fa95 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc)
 }
 EXPORT_SYMBOL_GPL(gpiod_to_irq);

-/**
- * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
- * @chip: the chip the GPIO to lock belongs to
- * @offset: the offset of the GPIO to lock as IRQ
- *
- * This is used directly by GPIO drivers that want to lock down
- * a certain GPIO line to be used for IRQs.
- */
-int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+static int gpiodesc_lock_as_irq(struct gpio_desc *desc)
 {
-	struct gpio_desc *desc;
-
-	desc = gpiochip_get_desc(chip, offset);
-	if (IS_ERR(desc))
-		return PTR_ERR(desc);
+	struct gpio_chip *chip = gpiod_to_chip(desc);

 	/*
 	 * If it's fast: flush the direction setting if something changed
 	 * behind our back
 	 */
 	if (!chip->can_sleep && chip->get_direction) {
-		int dir = chip->get_direction(chip, offset);
+		int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc));

 		if (dir)
 			clear_bit(FLAG_IS_OUT, &desc->flags);
@@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)

 	return 0;
 }
+
+/**
+ * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
+ * @chip: the chip the GPIO to lock belongs to
+ * @offset: the offset of the GPIO to lock as IRQ
+ *
+ * This is used directly by GPIO drivers that want to lock down
+ * a certain GPIO line to be used for IRQs.
+ */
+int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_desc *desc;
+
+	desc = gpiochip_get_desc(chip, offset);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+	return gpiodesc_lock_as_irq(desc);
+}
 EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);

+int gpiod_enable_irq(struct gpio_desc *desc)
+{
+	int irq;
+	int ret;
+
+	VALIDATE_DESC(desc);
+
+	irq = gpiod_to_irq(desc);
+	if (irq < 0)
+		return 0;
+
+	ret = gpiodesc_lock_as_irq(desc);
+	if (!ret)
+		enable_irq(irq);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpiod_enable_irq);
+
+static void gpiodesc_unlock_as_irq(struct gpio_desc *desc)
+{
+	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
+
+	/* If we only had this marking, erase it */
+	if (desc->label && !strcmp(desc->label, "interrupt"))
+		desc_set_label(desc, NULL);
+}
+
 /**
  * gpiochip_unlock_as_irq() - unlock a GPIO used as IRQ
  * @chip: the chip the GPIO to lock belongs to
@@ -3294,14 +3327,24 @@ void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
 	if (IS_ERR(desc))
 		return;

-	clear_bit(FLAG_USED_AS_IRQ, &desc->flags);
-
-	/* If we only had this marking, erase it */
-	if (desc->label && !strcmp(desc->label, "interrupt"))
-		desc_set_label(desc, NULL);
+	gpiodesc_unlock_as_irq(desc);
 }
 EXPORT_SYMBOL_GPL(gpiochip_unlock_as_irq);

+void gpiod_disable_irq(struct gpio_desc *desc)
+{
+	int irq;
+
+	VALIDATE_DESC_VOID(desc);
+	irq = gpiod_to_irq(desc);
+	if (irq < 0)
+		return;
+
+	disable_irq(irq);
+	gpiodesc_unlock_as_irq(desc);
+}
+EXPORT_SYMBOL_GPL(gpiod_disable_irq);
+
 bool gpiochip_line_is_irq(struct gpio_chip *chip, unsigned int offset)
 {
 	if (offset >= chip->ngpio)
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 243112c7fa7d..c7a9d154df22 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -104,6 +104,9 @@ int gpiod_direction_input(struct gpio_desc *desc);
 int gpiod_direction_output(struct gpio_desc *desc, int value);
 int gpiod_direction_output_raw(struct gpio_desc *desc, int value);

+int gpiod_enable_irq(struct gpio_desc *desc);
+void gpiod_disable_irq(struct gpio_desc *desc);
+
 /* Value get/set from non-sleeping context */
 int gpiod_get_value(const struct gpio_desc *desc);
 int gpiod_get_array_value(unsigned int array_size,
Marc Zyngier July 21, 2018, 10:08 a.m. | #6
Hi Hans,

On Sat, 21 Jul 2018 09:46:19 +0100,
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
> Hi Linus,
> 
> On 20/07/18 10:06, Linus Walleij wrote:
> 
> <snip>
> 
> > You would probably think it's fine if you didn't have to do this
> > heavy lifting and could just request them on probe and
> > call enable_irq() and disable_irq() when needed, and have these
> > call down into the irq_chip .irq_enable() and .irq_disable() and
> > lock the GPIO line as input *there* instead, and then change
> > this everywhere (yes patch all gpio_chips with an irqchip
> > driver in that case).
> > 
> > As drivers have likely sometimes already assigned function
> > pointers to .irq_enable() and .irq_disable() these have to
> > be copied and stored in struct gpio_irq_chip() by
> > gpiochip_set_cascaded_irqchip() and called in sequence.
> > But it can be done.
> > 
> > What about changing the GPIOLIB_IRQCHIP code to just
> > do the module_get()/put() part on .irq_request_resources()
> > and move the locking to the .irq_enable()/.irq_disable()
> > callbacks so that we can enable and disable irqs on the fly
> > in a better way? (BIG WIN!)
> > 
> > What about going and reinvestigating this instead?
> > 
> > I'm sorry that I can't present any simple solution, but hey,
> > I presented a really complicated solution instead, isn't it
> > great! :D
> 
> I did do some investigation into this, but it made me very unhappy:
> it's a lot of low-level changes in a lot of drivers for a lot of
> different boards, some of which are probably hard to test. Scary.
> 
> But I've come up with a much simpler alternative: add two new
> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.
> 
> An RFC patch is below (documentation is missing, but it works fine).
> 
> Basically if you have a gpio which has an interrupt, then you can use
> these functions to disable and enable it. It enables/disables the interrupt
> and calls gpiochip_(un)lock_as_irq() as well.
> 
> It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure
> that the direction is set correctly. I have not done so, but if you think it is
> useful, then it's trivial to do. Right now it will return an error if the
> direction is still set to output.
> 
> If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not
> sure if that's a bug or a feature. It's easy enough to change.
> 
> Let me know what you think. IMHO this avoids a huge amount of churn in code you
> really do no want to touch and it does exactly what you want it to do. And it
> looks pretty clean as well, both in gpiolib and in the drivers.
> 
> Regards,
> 
> 	Hans
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/gpio/gpiolib.c        | 83 ++++++++++++++++++++++++++---------
>  include/linux/gpio/consumer.h |  3 ++
>  2 files changed, 66 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e11a3bb03820..75ad6177fa95 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc)
>  }
>  EXPORT_SYMBOL_GPL(gpiod_to_irq);
> 
> -/**
> - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
> - * @chip: the chip the GPIO to lock belongs to
> - * @offset: the offset of the GPIO to lock as IRQ
> - *
> - * This is used directly by GPIO drivers that want to lock down
> - * a certain GPIO line to be used for IRQs.
> - */
> -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +static int gpiodesc_lock_as_irq(struct gpio_desc *desc)
>  {
> -	struct gpio_desc *desc;
> -
> -	desc = gpiochip_get_desc(chip, offset);
> -	if (IS_ERR(desc))
> -		return PTR_ERR(desc);
> +	struct gpio_chip *chip = gpiod_to_chip(desc);
> 
>  	/*
>  	 * If it's fast: flush the direction setting if something changed
>  	 * behind our back
>  	 */
>  	if (!chip->can_sleep && chip->get_direction) {
> -		int dir = chip->get_direction(chip, offset);
> +		int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc));
> 
>  		if (dir)
>  			clear_bit(FLAG_IS_OUT, &desc->flags);
> @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> 
>  	return 0;
>  }
> +
> +/**
> + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
> + * @chip: the chip the GPIO to lock belongs to
> + * @offset: the offset of the GPIO to lock as IRQ
> + *
> + * This is used directly by GPIO drivers that want to lock down
> + * a certain GPIO line to be used for IRQs.
> + */
> +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct gpio_desc *desc;
> +
> +	desc = gpiochip_get_desc(chip, offset);
> +	if (IS_ERR(desc))
> +		return PTR_ERR(desc);
> +	return gpiodesc_lock_as_irq(desc);
> +}
>  EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
> 
> +int gpiod_enable_irq(struct gpio_desc *desc)
> +{
> +	int irq;
> +	int ret;
> +
> +	VALIDATE_DESC(desc);
> +
> +	irq = gpiod_to_irq(desc);
> +	if (irq < 0)
> +		return 0;
> +
> +	ret = gpiodesc_lock_as_irq(desc);
> +	if (!ret)
> +		enable_irq(irq);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpiod_enable_irq);

{enable,disable}_irq() are supposed to allow nesting. Do you intent to
preserve the same semantics? This is specially important if you allow
sharing of the interrupt line between devices.

Thanks,

	M.
Hans Verkuil July 21, 2018, 10:16 a.m. | #7
On 21/07/18 12:08, Marc Zyngier wrote:
> Hi Hans,
> 
> On Sat, 21 Jul 2018 09:46:19 +0100,
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> Hi Linus,
>>
>> On 20/07/18 10:06, Linus Walleij wrote:
>>
>> <snip>
>>
>>> You would probably think it's fine if you didn't have to do this
>>> heavy lifting and could just request them on probe and
>>> call enable_irq() and disable_irq() when needed, and have these
>>> call down into the irq_chip .irq_enable() and .irq_disable() and
>>> lock the GPIO line as input *there* instead, and then change
>>> this everywhere (yes patch all gpio_chips with an irqchip
>>> driver in that case).
>>>
>>> As drivers have likely sometimes already assigned function
>>> pointers to .irq_enable() and .irq_disable() these have to
>>> be copied and stored in struct gpio_irq_chip() by
>>> gpiochip_set_cascaded_irqchip() and called in sequence.
>>> But it can be done.
>>>
>>> What about changing the GPIOLIB_IRQCHIP code to just
>>> do the module_get()/put() part on .irq_request_resources()
>>> and move the locking to the .irq_enable()/.irq_disable()
>>> callbacks so that we can enable and disable irqs on the fly
>>> in a better way? (BIG WIN!)
>>>
>>> What about going and reinvestigating this instead?
>>>
>>> I'm sorry that I can't present any simple solution, but hey,
>>> I presented a really complicated solution instead, isn't it
>>> great! :D
>>
>> I did do some investigation into this, but it made me very unhappy:
>> it's a lot of low-level changes in a lot of drivers for a lot of
>> different boards, some of which are probably hard to test. Scary.
>>
>> But I've come up with a much simpler alternative: add two new
>> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.
>>
>> An RFC patch is below (documentation is missing, but it works fine).
>>
>> Basically if you have a gpio which has an interrupt, then you can use
>> these functions to disable and enable it. It enables/disables the interrupt
>> and calls gpiochip_(un)lock_as_irq() as well.
>>
>> It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure
>> that the direction is set correctly. I have not done so, but if you think it is
>> useful, then it's trivial to do. Right now it will return an error if the
>> direction is still set to output.
>>
>> If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not
>> sure if that's a bug or a feature. It's easy enough to change.
>>
>> Let me know what you think. IMHO this avoids a huge amount of churn in code you
>> really do no want to touch and it does exactly what you want it to do. And it
>> looks pretty clean as well, both in gpiolib and in the drivers.
>>
>> Regards,
>>
>> 	Hans
>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> ---
>>  drivers/gpio/gpiolib.c        | 83 ++++++++++++++++++++++++++---------
>>  include/linux/gpio/consumer.h |  3 ++
>>  2 files changed, 66 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index e11a3bb03820..75ad6177fa95 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc)
>>  }
>>  EXPORT_SYMBOL_GPL(gpiod_to_irq);
>>
>> -/**
>> - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
>> - * @chip: the chip the GPIO to lock belongs to
>> - * @offset: the offset of the GPIO to lock as IRQ
>> - *
>> - * This is used directly by GPIO drivers that want to lock down
>> - * a certain GPIO line to be used for IRQs.
>> - */
>> -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
>> +static int gpiodesc_lock_as_irq(struct gpio_desc *desc)
>>  {
>> -	struct gpio_desc *desc;
>> -
>> -	desc = gpiochip_get_desc(chip, offset);
>> -	if (IS_ERR(desc))
>> -		return PTR_ERR(desc);
>> +	struct gpio_chip *chip = gpiod_to_chip(desc);
>>
>>  	/*
>>  	 * If it's fast: flush the direction setting if something changed
>>  	 * behind our back
>>  	 */
>>  	if (!chip->can_sleep && chip->get_direction) {
>> -		int dir = chip->get_direction(chip, offset);
>> +		int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc));
>>
>>  		if (dir)
>>  			clear_bit(FLAG_IS_OUT, &desc->flags);
>> @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
>>
>>  	return 0;
>>  }
>> +
>> +/**
>> + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
>> + * @chip: the chip the GPIO to lock belongs to
>> + * @offset: the offset of the GPIO to lock as IRQ
>> + *
>> + * This is used directly by GPIO drivers that want to lock down
>> + * a certain GPIO line to be used for IRQs.
>> + */
>> +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
>> +{
>> +	struct gpio_desc *desc;
>> +
>> +	desc = gpiochip_get_desc(chip, offset);
>> +	if (IS_ERR(desc))
>> +		return PTR_ERR(desc);
>> +	return gpiodesc_lock_as_irq(desc);
>> +}
>>  EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
>>
>> +int gpiod_enable_irq(struct gpio_desc *desc)
>> +{
>> +	int irq;
>> +	int ret;
>> +
>> +	VALIDATE_DESC(desc);
>> +
>> +	irq = gpiod_to_irq(desc);
>> +	if (irq < 0)
>> +		return 0;
>> +
>> +	ret = gpiodesc_lock_as_irq(desc);
>> +	if (!ret)
>> +		enable_irq(irq);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(gpiod_enable_irq);
> 
> {enable,disable}_irq() are supposed to allow nesting. Do you intent to
> preserve the same semantics? This is specially important if you allow
> sharing of the interrupt line between devices.

This code doesn't preserve it. E.g. calling gpiod_disable_irq twice, followed
by gpiod_enable_irq once will mark the GPIO line as being used for an interrupt,
even though the irq is not actually enabled yet.

I guess gpiodesc_(un)lock_as_irq can be adapter to support this by adding some
counter, but for the use-case I have it makes no sense to support nesting.

It's very rare that you need to do this, I'm just unlucky that I have two drivers
that need this. In both cases because an input gpio pin with an interrupt has to be
temporarily switched to an output pin to drive the bus.

I could add a WARN_ON if an attempt to nest these calls is detected.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier July 21, 2018, 12:30 p.m. | #8
On Sat, 21 Jul 2018 12:16:42 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> On 21/07/18 12:08, Marc Zyngier wrote:
> > Hi Hans,
> > 
> > On Sat, 21 Jul 2018 09:46:19 +0100,
> > Hans Verkuil <hverkuil@xs4all.nl> wrote:  
> >>
> >> Hi Linus,
> >>
> >> On 20/07/18 10:06, Linus Walleij wrote:
> >>
> >> <snip>
> >>  
> >>> You would probably think it's fine if you didn't have to do this
> >>> heavy lifting and could just request them on probe and
> >>> call enable_irq() and disable_irq() when needed, and have these
> >>> call down into the irq_chip .irq_enable() and .irq_disable() and
> >>> lock the GPIO line as input *there* instead, and then change
> >>> this everywhere (yes patch all gpio_chips with an irqchip
> >>> driver in that case).
> >>>
> >>> As drivers have likely sometimes already assigned function
> >>> pointers to .irq_enable() and .irq_disable() these have to
> >>> be copied and stored in struct gpio_irq_chip() by
> >>> gpiochip_set_cascaded_irqchip() and called in sequence.
> >>> But it can be done.
> >>>
> >>> What about changing the GPIOLIB_IRQCHIP code to just
> >>> do the module_get()/put() part on .irq_request_resources()
> >>> and move the locking to the .irq_enable()/.irq_disable()
> >>> callbacks so that we can enable and disable irqs on the fly
> >>> in a better way? (BIG WIN!)
> >>>
> >>> What about going and reinvestigating this instead?
> >>>
> >>> I'm sorry that I can't present any simple solution, but hey,
> >>> I presented a really complicated solution instead, isn't it
> >>> great! :D  
> >>
> >> I did do some investigation into this, but it made me very unhappy:
> >> it's a lot of low-level changes in a lot of drivers for a lot of
> >> different boards, some of which are probably hard to test. Scary.
> >>
> >> But I've come up with a much simpler alternative: add two new
> >> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.
> >>
> >> An RFC patch is below (documentation is missing, but it works fine).
> >>
> >> Basically if you have a gpio which has an interrupt, then you can use
> >> these functions to disable and enable it. It enables/disables the interrupt
> >> and calls gpiochip_(un)lock_as_irq() as well.
> >>
> >> It is even possible to call gpiod_direction_input in gpiod_enable_irq to ensure
> >> that the direction is set correctly. I have not done so, but if you think it is
> >> useful, then it's trivial to do. Right now it will return an error if the
> >> direction is still set to output.
> >>
> >> If there is no associated interrupt, then gpiod_enable_irq just returns 0. Not
> >> sure if that's a bug or a feature. It's easy enough to change.
> >>
> >> Let me know what you think. IMHO this avoids a huge amount of churn in code you
> >> really do no want to touch and it does exactly what you want it to do. And it
> >> looks pretty clean as well, both in gpiolib and in the drivers.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>
> >> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >> ---
> >>  drivers/gpio/gpiolib.c        | 83 ++++++++++++++++++++++++++---------
> >>  include/linux/gpio/consumer.h |  3 ++
> >>  2 files changed, 66 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> >> index e11a3bb03820..75ad6177fa95 100644
> >> --- a/drivers/gpio/gpiolib.c
> >> +++ b/drivers/gpio/gpiolib.c
> >> @@ -3228,28 +3228,16 @@ int gpiod_to_irq(const struct gpio_desc *desc)
> >>  }
> >>  EXPORT_SYMBOL_GPL(gpiod_to_irq);
> >>
> >> -/**
> >> - * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
> >> - * @chip: the chip the GPIO to lock belongs to
> >> - * @offset: the offset of the GPIO to lock as IRQ
> >> - *
> >> - * This is used directly by GPIO drivers that want to lock down
> >> - * a certain GPIO line to be used for IRQs.
> >> - */
> >> -int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> >> +static int gpiodesc_lock_as_irq(struct gpio_desc *desc)
> >>  {
> >> -	struct gpio_desc *desc;
> >> -
> >> -	desc = gpiochip_get_desc(chip, offset);
> >> -	if (IS_ERR(desc))
> >> -		return PTR_ERR(desc);
> >> +	struct gpio_chip *chip = gpiod_to_chip(desc);
> >>
> >>  	/*
> >>  	 * If it's fast: flush the direction setting if something changed
> >>  	 * behind our back
> >>  	 */
> >>  	if (!chip->can_sleep && chip->get_direction) {
> >> -		int dir = chip->get_direction(chip, offset);
> >> +		int dir = chip->get_direction(chip, gpio_chip_hwgpio(desc));
> >>
> >>  		if (dir)
> >>  			clear_bit(FLAG_IS_OUT, &desc->flags);
> >> @@ -3276,8 +3264,53 @@ int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> >>
> >>  	return 0;
> >>  }
> >> +
> >> +/**
> >> + * gpiochip_lock_as_irq() - lock a GPIO to be used as IRQ
> >> + * @chip: the chip the GPIO to lock belongs to
> >> + * @offset: the offset of the GPIO to lock as IRQ
> >> + *
> >> + * This is used directly by GPIO drivers that want to lock down
> >> + * a certain GPIO line to be used for IRQs.
> >> + */
> >> +int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
> >> +{
> >> +	struct gpio_desc *desc;
> >> +
> >> +	desc = gpiochip_get_desc(chip, offset);
> >> +	if (IS_ERR(desc))
> >> +		return PTR_ERR(desc);
> >> +	return gpiodesc_lock_as_irq(desc);
> >> +}
> >>  EXPORT_SYMBOL_GPL(gpiochip_lock_as_irq);
> >>
> >> +int gpiod_enable_irq(struct gpio_desc *desc)
> >> +{
> >> +	int irq;
> >> +	int ret;
> >> +
> >> +	VALIDATE_DESC(desc);
> >> +
> >> +	irq = gpiod_to_irq(desc);
> >> +	if (irq < 0)
> >> +		return 0;
> >> +
> >> +	ret = gpiodesc_lock_as_irq(desc);
> >> +	if (!ret)
> >> +		enable_irq(irq);
> >> +	return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(gpiod_enable_irq);  
> > 
> > {enable,disable}_irq() are supposed to allow nesting. Do you intent to
> > preserve the same semantics? This is specially important if you allow
> > sharing of the interrupt line between devices.  
> 
> This code doesn't preserve it. E.g. calling gpiod_disable_irq twice, followed
> by gpiod_enable_irq once will mark the GPIO line as being used for an interrupt,
> even though the irq is not actually enabled yet.
> 
> I guess gpiodesc_(un)lock_as_irq can be adapter to support this by adding some
> counter, but for the use-case I have it makes no sense to support nesting.

I appreciate that your use case is somewhat limited, but if we're
introducing a new API, it should be consistent with the rest of the IRQ
subsystem.

Also, duplicating the counter is likely to lead to all kind of horrible
situations if one driver uses your new API, and the other uses the
standard {enable,disable}_irq calls (as it ought to be able to -- after
all this is "just an interrupt").

> It's very rare that you need to do this, I'm just unlucky that I have
> two drivers that need this. In both cases because an input gpio pin
> with an interrupt has to be temporarily switched to an output pin to
> drive the bus.

I'm not sure I quite understand the nature of the problem. What happens
to the devices when you switch the GPIO pin to being an output?

	M.
Linus Walleij July 22, 2018, 10:49 p.m. | #9
On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> Me:
> > You would probably think it's fine if you didn't have to do this
> > heavy lifting and could just request them on probe and
> > call enable_irq() and disable_irq() when needed, and have these
> > call down into the irq_chip .irq_enable() and .irq_disable() and
> > lock the GPIO line as input *there* instead, and then change
> > this everywhere (yes patch all gpio_chips with an irqchip
> > driver in that case).
> >
> > As drivers have likely sometimes already assigned function
> > pointers to .irq_enable() and .irq_disable() these have to
> > be copied and stored in struct gpio_irq_chip() by
> > gpiochip_set_cascaded_irqchip() and called in sequence.
> > But it can be done.
> >
> > What about changing the GPIOLIB_IRQCHIP code to just
> > do the module_get()/put() part on .irq_request_resources()
> > and move the locking to the .irq_enable()/.irq_disable()
> > callbacks so that we can enable and disable irqs on the fly
> > in a better way? (BIG WIN!)
> >
> > What about going and reinvestigating this instead?
> >
> > I'm sorry that I can't present any simple solution, but hey,
> > I presented a really complicated solution instead, isn't it
> > great! :D
>
> I did do some investigation into this, but it made me very unhappy:
> it's a lot of low-level changes in a lot of drivers for a lot of
> different boards, some of which are probably hard to test. Scary.
>
> But I've come up with a much simpler alternative: add two new
> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.

I see what you are doing but it is kludgy and makes things
messy IMO.

It should be possible to just call irq_disable() and have the GPIO
line unlocked from the irqchip callback, clean and simple.

With this approach we are requireing special semantics and
consumers all of a sudden have to know that this IRQ line is
on a GPIO, and they did not need to know that before, and
should not need to know.

It should be just like this:

d = devm_gpiod_get(dev, GPIOD_IN);
i = gpiod_to_irq(d);
devm_request_irq(dev, i); // this locks the GPIO as IRQ
(...)
disable_irq(i); // this unlocks the GPIO as IRQ
gpiod_direction_output(d, 1);
(...)
gpiod_direction_input(d);
enable_irq(i); // this locks the GPIO as IRQ

I don't think it's a good idea to interperse this with any
GPIO-specific semantics (apart from gpiod_to_irq()) as it only
makes things unintuitive.

I feel strongly the right way is to make this work.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hans Verkuil July 23, 2018, 10:29 a.m. | #10
On 23/07/18 00:49, Linus Walleij wrote:
> On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>> Me:
>>> You would probably think it's fine if you didn't have to do this
>>> heavy lifting and could just request them on probe and
>>> call enable_irq() and disable_irq() when needed, and have these
>>> call down into the irq_chip .irq_enable() and .irq_disable() and
>>> lock the GPIO line as input *there* instead, and then change
>>> this everywhere (yes patch all gpio_chips with an irqchip
>>> driver in that case).
>>>
>>> As drivers have likely sometimes already assigned function
>>> pointers to .irq_enable() and .irq_disable() these have to
>>> be copied and stored in struct gpio_irq_chip() by
>>> gpiochip_set_cascaded_irqchip() and called in sequence.
>>> But it can be done.
>>>
>>> What about changing the GPIOLIB_IRQCHIP code to just
>>> do the module_get()/put() part on .irq_request_resources()
>>> and move the locking to the .irq_enable()/.irq_disable()
>>> callbacks so that we can enable and disable irqs on the fly
>>> in a better way? (BIG WIN!)
>>>
>>> What about going and reinvestigating this instead?
>>>
>>> I'm sorry that I can't present any simple solution, but hey,
>>> I presented a really complicated solution instead, isn't it
>>> great! :D
>>
>> I did do some investigation into this, but it made me very unhappy:
>> it's a lot of low-level changes in a lot of drivers for a lot of
>> different boards, some of which are probably hard to test. Scary.
>>
>> But I've come up with a much simpler alternative: add two new
>> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.
> 
> I see what you are doing but it is kludgy and makes things
> messy IMO.
> 
> It should be possible to just call irq_disable() and have the GPIO
> line unlocked from the irqchip callback, clean and simple.
> 
> With this approach we are requireing special semantics and
> consumers all of a sudden have to know that this IRQ line is
> on a GPIO, and they did not need to know that before, and
> should not need to know.
> 
> It should be just like this:
> 
> d = devm_gpiod_get(dev, GPIOD_IN);
> i = gpiod_to_irq(d);
> devm_request_irq(dev, i); // this locks the GPIO as IRQ
> (...)
> disable_irq(i); // this unlocks the GPIO as IRQ
> gpiod_direction_output(d, 1);
> (...)
> gpiod_direction_input(d);
> enable_irq(i); // this locks the GPIO as IRQ
> 
> I don't think it's a good idea to interperse this with any
> GPIO-specific semantics (apart from gpiod_to_irq()) as it only
> makes things unintuitive.
> 
> I feel strongly the right way is to make this work.

Are you really, really sure about this? As far as I can tell all of these drivers
would have to be updated, just for something that is very specific to two CEC drivers:

$ git grep -l gpiochip_lock_as_irq
Documentation/driver-api/gpio/driver.rst
drivers/gpio/gpio-bcm-kona.c
drivers/gpio/gpio-dwapb.c
drivers/gpio/gpio-em.c
drivers/gpio/gpio-tegra.c
drivers/gpio/gpio-thunderx.c
drivers/gpio/gpio-uniphier.c
drivers/gpio/gpio-vr41xx.c
drivers/gpio/gpio-xgene-sb.c
drivers/gpio/gpiolib-acpi.c
drivers/gpio/gpiolib-sysfs.c
drivers/gpio/gpiolib.c
drivers/hid/hid-cp2112.c
drivers/pinctrl/mediatek/mtk-eint.c
drivers/pinctrl/pinctrl-st.c
drivers/pinctrl/samsung/pinctrl-exynos.c
drivers/pinctrl/stm32/pinctrl-stm32.c
drivers/pinctrl/sunxi/pinctrl-sunxi.c
include/linux/gpio.h
include/linux/gpio/driver.h

$ git grep -l \\\.irq_enable.*= drivers/gpio/
drivers/gpio/gpio-ath79.c
drivers/gpio/gpio-davinci.c
drivers/gpio/gpio-dwapb.c
drivers/gpio/gpio-ingenic.c
drivers/gpio/gpio-lynxpoint.c
drivers/gpio/gpio-ml-ioh.c
drivers/gpio/gpio-pcf857x.c
drivers/gpio/gpio-sta2x11.c
drivers/gpio/gpio-thunderx.c
drivers/gpio/gpio-timberdale.c
drivers/gpio/gpio-zynq.c

$ git grep -l \\\.irq_enable.*= drivers/pinctrl/
drivers/pinctrl/bcm/pinctrl-bcm2835.c
drivers/pinctrl/bcm/pinctrl-nsp-gpio.c
drivers/pinctrl/intel/pinctrl-intel.c
drivers/pinctrl/pinctrl-amd.c
drivers/pinctrl/pinctrl-coh901.c
drivers/pinctrl/pinctrl-rockchip.c
drivers/pinctrl/pinctrl-single.c
drivers/pinctrl/spear/pinctrl-plgpio.c
drivers/pinctrl/sunxi/pinctrl-sunxi.c

Furthermore, I can also work around it in the CEC drivers themselves (using
gpiod_direction_output_raw or something similar as discussed earlier, or even
by just calling free_irq and requesting it again after the CEC calibration
finished, i.e. the same that I do today in cec-gpio as well).

I just think this is a high-risk approach for something that is rarely needed.
In all the years that this has been around this is the first time that this
has been requested.

And to be honest, I don't think it is something I'm willing to spend that
much time on. If there was an expectation that this would be needed more
frequently in the future, then that might change, but I don't think that's
likely.

Personally I think that my proposal is the middle ground between a driver
workaround and reworking a lot of gpio drivers. I.e. it's not ideal, but
does the job. And if this turns out that this is needed more frequently,
then it is always an option to go for the better solution.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier July 23, 2018, 12:40 p.m. | #11
On 23/07/18 11:29, Hans Verkuil wrote:
> On 23/07/18 00:49, Linus Walleij wrote:
>> On Sat, Jul 21, 2018 at 10:46 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>> Me:
>>>> You would probably think it's fine if you didn't have to do this
>>>> heavy lifting and could just request them on probe and
>>>> call enable_irq() and disable_irq() when needed, and have these
>>>> call down into the irq_chip .irq_enable() and .irq_disable() and
>>>> lock the GPIO line as input *there* instead, and then change
>>>> this everywhere (yes patch all gpio_chips with an irqchip
>>>> driver in that case).
>>>>
>>>> As drivers have likely sometimes already assigned function
>>>> pointers to .irq_enable() and .irq_disable() these have to
>>>> be copied and stored in struct gpio_irq_chip() by
>>>> gpiochip_set_cascaded_irqchip() and called in sequence.
>>>> But it can be done.
>>>>
>>>> What about changing the GPIOLIB_IRQCHIP code to just
>>>> do the module_get()/put() part on .irq_request_resources()
>>>> and move the locking to the .irq_enable()/.irq_disable()
>>>> callbacks so that we can enable and disable irqs on the fly
>>>> in a better way? (BIG WIN!)
>>>>
>>>> What about going and reinvestigating this instead?
>>>>
>>>> I'm sorry that I can't present any simple solution, but hey,
>>>> I presented a really complicated solution instead, isn't it
>>>> great! :D
>>>
>>> I did do some investigation into this, but it made me very unhappy:
>>> it's a lot of low-level changes in a lot of drivers for a lot of
>>> different boards, some of which are probably hard to test. Scary.
>>>
>>> But I've come up with a much simpler alternative: add two new
>>> gpiolib functions: gpiod_enable_irq and gpiod_disable_irq.
>>
>> I see what you are doing but it is kludgy and makes things
>> messy IMO.
>>
>> It should be possible to just call irq_disable() and have the GPIO
>> line unlocked from the irqchip callback, clean and simple.
>>
>> With this approach we are requireing special semantics and
>> consumers all of a sudden have to know that this IRQ line is
>> on a GPIO, and they did not need to know that before, and
>> should not need to know.
>>
>> It should be just like this:
>>
>> d = devm_gpiod_get(dev, GPIOD_IN);
>> i = gpiod_to_irq(d);
>> devm_request_irq(dev, i); // this locks the GPIO as IRQ
>> (...)
>> disable_irq(i); // this unlocks the GPIO as IRQ
>> gpiod_direction_output(d, 1);
>> (...)
>> gpiod_direction_input(d);
>> enable_irq(i); // this locks the GPIO as IRQ
>>
>> I don't think it's a good idea to interperse this with any
>> GPIO-specific semantics (apart from gpiod_to_irq()) as it only
>> makes things unintuitive.
>>
>> I feel strongly the right way is to make this work.

I fully agree here. Creating a gpio-specific API that doesn't
interoperate with the normal IRQ API is just creating a absolute mess.
On the other hand, seamlessly integrating the GPIO configuration as part
of the IRQ API makes complete sense.

> Are you really, really sure about this? As far as I can tell all of these drivers
> would have to be updated, just for something that is very specific to two CEC drivers:

[...]

And that's absolutely fine. we do bulk changes in existing drivers all
the time, specially if this leads to a better integration of close
subsystems.

> Furthermore, I can also work around it in the CEC drivers themselves (using
> gpiod_direction_output_raw or something similar as discussed earlier, or even
> by just calling free_irq and requesting it again after the CEC calibration
> finished, i.e. the same that I do today in cec-gpio as well).
> 
> I just think this is a high-risk approach for something that is rarely needed.
> In all the years that this has been around this is the first time that this
> has been requested.

I can see two options: either we support something fully, in a way that
is integrated with the rest of the kernel, or we don't. There is enough
horrible hacks we have to live with already.

> And to be honest, I don't think it is something I'm willing to spend that
> much time on. If there was an expectation that this would be needed more
> frequently in the future, then that might change, but I don't think that's
> likely.

There is obviously a need, since someone managed to design a piece of HW
that requires something that doesn't fit in the current model. The real
question is whether we want to support it or not.

I'm definitely willing to add support for it, but not at the cost of a
third rate kludge.

> Personally I think that my proposal is the middle ground between a driver
> workaround and reworking a lot of gpio drivers. I.e. it's not ideal, but
> does the job. And if this turns out that this is needed more frequently,
> then it is always an option to go for the better solution.

As one of the folks who would end-up maintaining the resulting mess, my
answer is "no, thank you". Linus has outlined something that makes sense
for both the GPIO and IRQ subsystems. I appreciate this would take time
and effort, but supporting silly HW comes at that cost.

Thanks,

	M.

Patch

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e11a3bb03820..dcdb457b83b0 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1822,6 +1822,20 @@  static void gpiochip_irq_relres(struct irq_data *d)
 	module_put(chip->gpiodev->owner);
 }

+static void gpiochip_irq_enable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	WARN_ON(gpiochip_lock_as_irq(chip, d->hwirq));
+}
+
+static void gpiochip_irq_disable(struct irq_data *d)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
+
+	gpiochip_unlock_as_irq(chip, d->hwirq);
+}
+
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
@@ -1897,6 +1911,10 @@  static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 	    !irqchip->irq_release_resources) {
 		irqchip->irq_request_resources = gpiochip_irq_reqres;
 		irqchip->irq_release_resources = gpiochip_irq_relres;
+		if (!irqchip->irq_enable && !irqchip->irq_disable) {
+			irqchip->irq_enable = gpiochip_irq_enable;
+			irqchip->irq_disable = gpiochip_irq_disable;
+		}
 	}

 	if (gpiochip->irq.parent_handler) {
@@ -2056,6 +2074,10 @@  int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 	    !irqchip->irq_release_resources) {
 		irqchip->irq_request_resources = gpiochip_irq_reqres;
 		irqchip->irq_release_resources = gpiochip_irq_relres;
+		if (!irqchip->irq_enable && !irqchip->irq_disable) {
+			irqchip->irq_enable = gpiochip_irq_enable;
+			irqchip->irq_disable = gpiochip_irq_disable;
+		}
 	}

 	acpi_gpiochip_request_interrupts(gpiochip);