[PATCH] extcon: gpio: Request reasonable interrupts

[PATCH] extcon: gpio: Request reasonable interrupts

[Linus Walleij]

The only thing that makes sense is to request a falling edge interrupt
if the line is active low and a rising edge interrupt if the line is
active high, so just do that and get rid of the assignment from
platform data. The GPIO descriptor knows if the line is active high
or low.

Also make irq a local variable in probe(), it's not used anywhere else.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/extcon/extcon-gpio.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
index 13ba3a6e81d5..a0674f1f3849 100644
--- a/drivers/extcon/extcon-gpio.c
+++ b/drivers/extcon/extcon-gpio.c
@@ -30,26 +30,22 @@
 /**
  * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
  * @edev:		Extcon device.
- * @irq:		Interrupt line for the external connector.
  * @work:		Work fired by the interrupt.
  * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
  *			value.
  * @gpiod:		GPIO descriptor for this external connector.
  * @extcon_id:		The unique id of specific external connector.
  * @debounce:		Debounce time for GPIO IRQ in ms.
- * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
  * @check_on_resume:	Boolean describing whether to check the state of gpio
  *			while resuming from sleep.
  */
 struct gpio_extcon_data {
 	struct extcon_dev *edev;
-	int irq;
 	struct delayed_work work;
 	unsigned long debounce_jiffies;
 	struct gpio_desc *gpiod;
 	unsigned int extcon_id;
 	unsigned long debounce;
-	unsigned long irq_flags;
 	bool check_on_resume;
 };
 
@@ -77,6 +73,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 {
 	struct gpio_extcon_data *data;
 	struct device *dev = &pdev->dev;
+	unsigned long irq_flags;
+	int irq;
 	int ret;
 
 	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
@@ -96,9 +94,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
 	if (IS_ERR(data->gpiod))
 		return PTR_ERR(data->gpiod);
-	data->irq = gpiod_to_irq(data->gpiod);
-	if (data->irq <= 0)
-		return data->irq;
+	irq = gpiod_to_irq(data->gpiod);
+	if (irq <= 0)
+		return irq;
+
+	/*
+	 * It is unlikely that this is an acknowledged interrupt that goes
+	 * away after handling, what we are looking for are falling edges
+	 * if the signal is active low, and rising edges if the signal is
+	 * active high.
+	 */
+	if (gpiod_is_active_low(data->gpiod))
+		irq_flags = IRQF_TRIGGER_FALLING;
+	else
+		irq_flags = IRQF_TRIGGER_RISING;
 
 	/* Allocate the memory of extcon devie and register extcon device */
 	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
@@ -117,8 +126,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
 	 * Request the interrupt of gpio to detect whether external connector
 	 * is attached or detached.
 	 */
-	ret = devm_request_any_context_irq(dev, data->irq,
-					gpio_irq_handler, data->irq_flags,
+	ret = devm_request_any_context_irq(dev, irq,
+					gpio_irq_handler, irq_flags,
 					pdev->name, data);
 	if (ret < 0)
 		return ret;
-- 
2.20.1

Re: [PATCH] extcon: gpio: Request reasonable interrupts

[Chanwoo Choi]

Hi Linus,

On 19. 5. 31. 오전 3:39, Linus Walleij wrote:
> The only thing that makes sense is to request a falling edge interrupt
> if the line is active low and a rising edge interrupt if the line is
> active high, so just do that and get rid of the assignment from
> platform data. The GPIO descriptor knows if the line is active high
> or low.
> 
> Also make irq a local variable in probe(), it's not used anywhere else.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/extcon/extcon-gpio.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/extcon/extcon-gpio.c b/drivers/extcon/extcon-gpio.c
> index 13ba3a6e81d5..a0674f1f3849 100644
> --- a/drivers/extcon/extcon-gpio.c
> +++ b/drivers/extcon/extcon-gpio.c
> @@ -30,26 +30,22 @@
>  /**
>   * struct gpio_extcon_data - A simple GPIO-controlled extcon device state container.
>   * @edev:		Extcon device.
> - * @irq:		Interrupt line for the external connector.
>   * @work:		Work fired by the interrupt.
>   * @debounce_jiffies:	Number of jiffies to wait for the GPIO to stabilize, from the debounce
>   *			value.
>   * @gpiod:		GPIO descriptor for this external connector.
>   * @extcon_id:		The unique id of specific external connector.
>   * @debounce:		Debounce time for GPIO IRQ in ms.
> - * @irq_flags:		IRQ Flags (e.g., IRQF_TRIGGER_LOW).
>   * @check_on_resume:	Boolean describing whether to check the state of gpio
>   *			while resuming from sleep.
>   */
>  struct gpio_extcon_data {
>  	struct extcon_dev *edev;
> -	int irq;
>  	struct delayed_work work;
>  	unsigned long debounce_jiffies;
>  	struct gpio_desc *gpiod;
>  	unsigned int extcon_id;
>  	unsigned long debounce;
> -	unsigned long irq_flags;
>  	bool check_on_resume;
>  };
>  
> @@ -77,6 +73,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  {
>  	struct gpio_extcon_data *data;
>  	struct device *dev = &pdev->dev;
> +	unsigned long irq_flags;
> +	int irq;
>  	int ret;
>  
>  	data = devm_kzalloc(dev, sizeof(struct gpio_extcon_data), GFP_KERNEL);
> @@ -96,9 +94,20 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	data->gpiod = devm_gpiod_get(dev, "extcon", GPIOD_IN);
>  	if (IS_ERR(data->gpiod))
>  		return PTR_ERR(data->gpiod);
> -	data->irq = gpiod_to_irq(data->gpiod);
> -	if (data->irq <= 0)
> -		return data->irq;
> +	irq = gpiod_to_irq(data->gpiod);
> +	if (irq <= 0)
> +		return irq;
> +
> +	/*
> +	 * It is unlikely that this is an acknowledged interrupt that goes
> +	 * away after handling, what we are looking for are falling edges
> +	 * if the signal is active low, and rising edges if the signal is
> +	 * active high.
> +	 */
> +	if (gpiod_is_active_low(data->gpiod))
> +		irq_flags = IRQF_TRIGGER_FALLING;

If gpiod_is_active_low(data->gpiod) is true, irq_flags might be
IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING. How can we sure
that irq_flags is always IRQF_TRIGGER_FALLING?

> +	else
> +		irq_flags = IRQF_TRIGGER_RISING;
>  
>  	/* Allocate the memory of extcon devie and register extcon device */
>  	data->edev = devm_extcon_dev_allocate(dev, &data->extcon_id);
> @@ -117,8 +126,8 @@ static int gpio_extcon_probe(struct platform_device *pdev)
>  	 * Request the interrupt of gpio to detect whether external connector
>  	 * is attached or detached.
>  	 */
> -	ret = devm_request_any_context_irq(dev, data->irq,
> -					gpio_irq_handler, data->irq_flags,
> +	ret = devm_request_any_context_irq(dev, irq,
> +					gpio_irq_handler, irq_flags,
>  					pdev->name, data);
>  	if (ret < 0)
>  		return ret;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] extcon: gpio: Request reasonable interrupts

[Linus Walleij]

On Tue, Jun 4, 2019 at 3:30 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 19. 5. 31. 오전 3:39, Linus Walleij wrote:

> > +     /*
> > +      * It is unlikely that this is an acknowledged interrupt that goes
> > +      * away after handling, what we are looking for are falling edges
> > +      * if the signal is active low, and rising edges if the signal is
> > +      * active high.
> > +      */
> > +     if (gpiod_is_active_low(data->gpiod))
> > +             irq_flags = IRQF_TRIGGER_FALLING;
>
> If gpiod_is_active_low(data->gpiod) is true, irq_flags might be
> IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING. How can we sure
> that irq_flags is always IRQF_TRIGGER_FALLING?

OK correct me if I'm wrong, but this is an external connector and
the GPIO goes low/high when the connector is physically inserted.
If it was level trigged, it would lock up the CPU with interrupts until
it was unplugged again, since there is no way to acknowledge a
level IRQ.

I think level IRQ on GPIOs are only used for logic peripherals
such as ethernet controllers etc where you can talk to the peripheral
and get it to deassert the line and thus acknowledge the IRQ.

So the way I see it only edge triggering makes sense for extcon.

Correct me if I'm wrong.

Yours,
Linus Walleij

Re: [PATCH] extcon: gpio: Request reasonable interrupts

[Chanwoo Choi]

On 19. 6. 8. 오전 6:24, Linus Walleij wrote:
> On Tue, Jun 4, 2019 at 3:30 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>> On 19. 5. 31. 오전 3:39, Linus Walleij wrote:
> 
>>> +     /*
>>> +      * It is unlikely that this is an acknowledged interrupt that goes
>>> +      * away after handling, what we are looking for are falling edges
>>> +      * if the signal is active low, and rising edges if the signal is
>>> +      * active high.
>>> +      */
>>> +     if (gpiod_is_active_low(data->gpiod))
>>> +             irq_flags = IRQF_TRIGGER_FALLING;
>>
>> If gpiod_is_active_low(data->gpiod) is true, irq_flags might be
>> IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING. How can we sure
>> that irq_flags is always IRQF_TRIGGER_FALLING?
> 
> OK correct me if I'm wrong, but this is an external connector and
> the GPIO goes low/high when the connector is physically inserted.
> If it was level trigged, it would lock up the CPU with interrupts until
> it was unplugged again, since there is no way to acknowledge a
> level IRQ.
> 
> I think level IRQ on GPIOs are only used for logic peripherals
> such as ethernet controllers etc where you can talk to the peripheral
> and get it to deassert the line and thus acknowledge the IRQ.
> 
> So the way I see it only edge triggering makes sense for extcon.
> 
> Correct me if I'm wrong.

Sorry for late reply because of vacation.

Actually, I have not thought that the kind of irq_flags are fixed
according to the category of specific h/w device. Until now, as I knew,
the h/w device have to initialize the the kind of irq_flags
for each peripheral device dependency. The each vendor of peripheral device
might design the kind of the kind of irq-flags for detection.

If possible, could you provide some example on mainline kernel?

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH] extcon: gpio: Request reasonable interrupts

[Linus Walleij]

On Mon, Jun 24, 2019 at 2:08 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> On 19. 6. 8. 오전 6:24, Linus Walleij wrote:
> > On Tue, Jun 4, 2019 at 3:30 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
> >> On 19. 5. 31. 오전 3:39, Linus Walleij wrote:
> >
> >>> +     /*
> >>> +      * It is unlikely that this is an acknowledged interrupt that goes
> >>> +      * away after handling, what we are looking for are falling edges
> >>> +      * if the signal is active low, and rising edges if the signal is
> >>> +      * active high.
> >>> +      */
> >>> +     if (gpiod_is_active_low(data->gpiod))
> >>> +             irq_flags = IRQF_TRIGGER_FALLING;
> >>
> >> If gpiod_is_active_low(data->gpiod) is true, irq_flags might be
> >> IRQF_TRIGGER_LOW instead of IRQF_TRIGGER_FALLING. How can we sure
> >> that irq_flags is always IRQF_TRIGGER_FALLING?
> >
> > OK correct me if I'm wrong, but this is an external connector and
> > the GPIO goes low/high when the connector is physically inserted.
> > If it was level trigged, it would lock up the CPU with interrupts until
> > it was unplugged again, since there is no way to acknowledge a
> > level IRQ.
> >
> > I think level IRQ on GPIOs are only used for logic peripherals
> > such as ethernet controllers etc where you can talk to the peripheral
> > and get it to deassert the line and thus acknowledge the IRQ.
> >
> > So the way I see it only edge triggering makes sense for extcon.
> >
> > Correct me if I'm wrong.
>
> Sorry for late reply because of vacation.

Don't worry I am not in a hurry. This is clean-up work :)

> Actually, I have not thought that the kind of irq_flags are fixed
> according to the category of specific h/w device. Until now, as I knew,
> the h/w device have to initialize the the kind of irq_flags
> for each peripheral device dependency. The each vendor of peripheral device
> might design the kind of the kind of irq-flags for detection.
>
> If possible, could you provide some example on mainline kernel?

I don't know exactly what kind of example you are looking
for, but in e.g. drivers/input/keyboard/gpio_keys.c
you find this code:

                isr = gpio_keys_gpio_isr;
                irqflags = IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING;

                switch (button->wakeup_event_action) {
                case EV_ACT_ASSERTED:
                        bdata->wakeup_trigger_type = active_low ?
                                IRQ_TYPE_EDGE_FALLING : IRQ_TYPE_EDGE_RISING;
                        break;
                case EV_ACT_DEASSERTED:
                        bdata->wakeup_trigger_type = active_low ?
                                IRQ_TYPE_EDGE_RISING : IRQ_TYPE_EDGE_FALLING;
                        break;

Is this what you're looking for?

Yours,
Linus Walleij