diff mbox series

[RFC] gpiolib-acpi: make sure we trigger edge events at least once on boot

Message ID 20180706093229.19072-2-hdegoede@redhat.com
State New
Headers show
Series [RFC] gpiolib-acpi: make sure we trigger edge events at least once on boot | expand

Commit Message

Hans de Goede July 6, 2018, 9:32 a.m. UTC 
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>

On some systems using edge triggered ACPI Event Interrupts, the initial
state at boot is not setup by the firmware, instead relying on the edge
irq event handler running at least once to setup the initial state.

2 known examples of this are:

1) The Surface 3 has its _LID state controlled by an ACPI operation region
triggered by a GPIO event:

 OperationRegion (GPOR, GeneralPurposeIo, Zero, One)
 Field (GPOR, ByteAcc, NoLock, Preserve)
 {
     Connection (
         GpioIo (Shared, PullNone, 0x0000, 0x0000, IoRestrictionNone,
             "\\_SB.GPO0", 0x00, ResourceConsumer, ,
             )
             {   // Pin list
                 0x004C
             }
     ),
     HELD,   1
 }

 Method (_E4C, 0, Serialized)  // _Exx: Edge-Triggered GPE
 {
     If ((HELD == One))
     {
         ^^LID.LIDB = One
     }
     Else
     {
         ^^LID.LIDB = Zero
         Notify (LID, 0x80) // Status Change
     }

     Notify (^^PCI0.SPI1.NTRG, One) // Device Check
 }

Currently, the state of LIDB is wrong until the user actually closes or
open the cover. We need to trigger the GPIO event once to update the
internal ACPI state.

Coincidentally, this also enables the Surface 2 integrated HID sensor hub
which also requires an ACPI gpio operation region to start initialization.

2) Various Bay Trail based tablets come with an external USB mux and
TI T1210B USB phy to enable USB gadget mode. The mux is controlled by a
GPIO which is controlled by an edge triggered ACPI Event Interrupt which
monitors the micro-USB ID pin.

When the tablet is connected to a PC (or no cable is plugged in), the ID
pin is high and the tablet should be in gadget mode. But the GPIO
controlling the mux is initialized by the firmware so that the USB data
lines are muxed to the host controller.

This means that if the user wants to use gadget mode, the user needs to
first plug in a host-cable to force the ID pin low and then unplug it
and connect the tablet to a PC, to get the ACPI event handler to run and
switch the mux to device mode,

This commit fixes both by running the event-handler once on boot.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
[hdegoede: Document BYT USB mux reliance on initial trigger]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpiolib-acpi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Mika Westerberg July 6, 2018, 9:46 a.m. UTC | #1 
On Fri, Jul 06, 2018 at 11:32:29AM +0200, Hans de Goede wrote:
> +
> +	/*
> +	 * Make sure we trigger the initial state of the IRQ when
> +	 * using RISING or FALLING.
> +	 */
> +	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> +	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> +		handler(-1, event);
> +

While I don't have anything against this it makes me wonder how it is
handled in Windows side? Any idea?
--
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 de Goede July 6, 2018, 9:52 a.m. UTC | #2 
Hi,

On 07/06/2018 11:46 AM, Mika Westerberg wrote:
> On Fri, Jul 06, 2018 at 11:32:29AM +0200, Hans de Goede wrote:
>> +
>> +	/*
>> +	 * Make sure we trigger the initial state of the IRQ when
>> +	 * using RISING or FALLING.
>> +	 */
>> +	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>> +	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>> +		handler(-1, event);
>> +
> 
> While I don't have anything against this it makes me wonder how it is
> handled in Windows side? Any idea?

I guess Windows does the same, otherwise it should have similar issues,
but that is just a guess.

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
Mika Westerberg July 6, 2018, 10:27 a.m. UTC | #3 
On Fri, Jul 06, 2018 at 11:52:51AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 07/06/2018 11:46 AM, Mika Westerberg wrote:
> > On Fri, Jul 06, 2018 at 11:32:29AM +0200, Hans de Goede wrote:
> > > +
> > > +	/*
> > > +	 * Make sure we trigger the initial state of the IRQ when
> > > +	 * using RISING or FALLING.
> > > +	 */
> > > +	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> > > +	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
> > > +		handler(-1, event);
> > > +
> > 
> > While I don't have anything against this it makes me wonder how it is
> > handled in Windows side? Any idea?
> 
> I guess Windows does the same, otherwise it should have similar issues,
> but that is just a guess.

I see. Is this the ACPI control method lid device (PNP0C0D)? Maybe we
can read its initial state using _LID method?
--
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 de Goede July 6, 2018, 10:30 a.m. UTC | #4 
Hi,

On 07/06/2018 12:27 PM, Mika Westerberg wrote:
> On Fri, Jul 06, 2018 at 11:52:51AM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 07/06/2018 11:46 AM, Mika Westerberg wrote:
>>> On Fri, Jul 06, 2018 at 11:32:29AM +0200, Hans de Goede wrote:
>>>> +
>>>> +	/*
>>>> +	 * Make sure we trigger the initial state of the IRQ when
>>>> +	 * using RISING or FALLING.
>>>> +	 */
>>>> +	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
>>>> +	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
>>>> +		handler(-1, event);
>>>> +
>>>
>>> While I don't have anything against this it makes me wonder how it is
>>> handled in Windows side? Any idea?
>>
>> I guess Windows does the same, otherwise it should have similar issues,
>> but that is just a guess.
> 
> I see. Is this the ACPI control method lid device (PNP0C0D)? Maybe we
> can read its initial state using _LID method?

No the problem is the _LID method reads an APCI variable which gets set
by the _AEI handler. So if the _AEI handler has not run that variable will
just be 0, which may or may not reflect the actual LID state.

The 2nd case described in the commit message is the _AIE handler driving
a GPIO and that GPIO / the mux controlled by that GPIO not reflecting the
cable-type plugged into the micro-usb until the handler runs (this is the
one I still need to verify it is actually fixed by this patch).

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
Andy Shevchenko July 6, 2018, 11:13 a.m. UTC | #5 
On Fri, 2018-07-06 at 11:32 +0200, Hans de Goede wrote:
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> 
> On some systems using edge triggered ACPI Event Interrupts, the
> initial
> state at boot is not setup by the firmware, instead relying on the
> edge
> irq event handler running at least once to setup the initial state.

I have read the discussion and it sounds like the best we can have right
now (if it's indeed fixes all issues listed).


One nit

> +		handler(-1, event);

use 'irq' instead of '-1'.
Mika Westerberg July 6, 2018, 11:25 a.m. UTC | #6 
On Fri, Jul 06, 2018 at 12:30:47PM +0200, Hans de Goede wrote:
> > I see. Is this the ACPI control method lid device (PNP0C0D)? Maybe we
> > can read its initial state using _LID method?
> 
> No the problem is the _LID method reads an APCI variable which gets set
> by the _AEI handler. So if the _AEI handler has not run that variable will
> just be 0, which may or may not reflect the actual LID state.
> 
> The 2nd case described in the commit message is the _AIE handler driving
> a GPIO and that GPIO / the mux controlled by that GPIO not reflecting the
> cable-type plugged into the micro-usb until the handler runs (this is the
> one I still need to verify it is actually fixed by this patch).

OK, thanks for the explanation.
--
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 de Goede July 7, 2018, 6:36 a.m. UTC | #7 
Hi,

On 07/06/2018 01:13 PM, Andy Shevchenko wrote:
> On Fri, 2018-07-06 at 11:32 +0200, Hans de Goede wrote:
>> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>
>> On some systems using edge triggered ACPI Event Interrupts, the
>> initial
>> state at boot is not setup by the firmware, instead relying on the
>> edge
>> irq event handler running at least once to setup the initial state.
> 
> I have read the discussion and it sounds like the best we can have right
> now (if it's indeed fixes all issues listed).
> 
> 
> One nit
> 
>> +		handler(-1, event);
> 
> use 'irq' instead of '-1'.

Ok, I've fixed this up in my local version. I will send out a non
RFC version with this fixed once I've had a chance to run the
tests which I want to run.

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
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e2232cbcec8b..7749b2ff7367 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -136,7 +136,7 @@  static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 	irq_handler_t handler = NULL;
 	struct gpio_desc *desc;
 	unsigned long irqflags;
-	int ret, pin, irq;
+	int ret, pin, irq, value;
 
 	if (!acpi_gpio_get_irq_resource(ares, &agpio))
 		return AE_OK;
@@ -167,6 +167,8 @@  static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 
 	gpiod_direction_input(desc);
 
+	value = gpiod_get_value(desc);
+
 	ret = gpiochip_lock_as_irq(chip, pin);
 	if (ret) {
 		dev_err(chip->parent, "Failed to lock GPIO as interrupt\n");
@@ -222,6 +224,15 @@  static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 		enable_irq_wake(irq);
 
 	list_add_tail(&event->node, &acpi_gpio->events);
+
+	/*
+	 * Make sure we trigger the initial state of the IRQ when
+	 * using RISING or FALLING.
+	 */
+	if (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
+	    ((irqflags & IRQF_TRIGGER_FALLING) && value == 0))
+		handler(-1, event);
+
 	return AE_OK;
 
 fail_free_event: