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

[Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>]

On Thu, Jul 12, 2018 at 6:23 PM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, 2018-07-12 at 17:25 +0200, Hans de Goede wrote:
> > From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > 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.
> >
> > Note that the running of the event-handler is done from a
> > late_initcall,
> > this is done because the handler AML code may rely on OperationRegions
> > registered by other builtin drivers. This avoids errors like these:
> >
> > [    0.133026] ACPI Error: No handler for Region [XSCG]
> > ((____ptrval____)) [GenericSerialBus] (20180531/evregion-132)
> > [    0.133036] ACPI Error: Region GenericSerialBus (ID=9) has no
> > handler (20180531/exfldio-265)
> > [    0.133046] ACPI Error: Method parse/execution failed
> > \_SB.GPO2._E12, AE_NOT_EXIST (20180531/psparse-516)
> >
>
> Yep, this version I like more.
>
> Benjamin, for sake of testing is it possible to revert (temporary) the
> change you mentioned for _LID and test if this helps as well as stated?

I'll try to do it today, but I can't guarantee if it will be done
today. IIRC, I have been told that the Surface 3 was having
difficulties with recent kernels, so crossing fingers here.

Cheers,
Benjamin

>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> One nitpick below, though I'm fine if you ignore it.
>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > [hdegoede: Document BYT USB mux reliance on initial trigger]
> > [hdegoede: Run event handler from a late_initcall, rather then
> > immediately]
> > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> > Changes in v2:
> > -Use late_initcall_sync to delay running the handler till other
> > drivers
> >  have been probed, rather then using a delayed_workqueue
> > ---
> >  drivers/gpio/gpiolib-acpi.c | 56
> > ++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 55 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index e2232cbcec8b..addd9fecc198 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -25,6 +25,7 @@
> >
> >  struct acpi_gpio_event {
> >       struct list_head node;
> > +     struct list_head initial_sync_list;
> >       acpi_handle handle;
> >       unsigned int pin;
> >       unsigned int irq;
> > @@ -50,6 +51,9 @@ struct acpi_gpio_chip {
> >       struct list_head events;
> >  };
> >
> > +static LIST_HEAD(acpi_gpio_initial_sync_list);
> > +static DEFINE_MUTEX(acpi_gpio_initial_sync_list_lock);
> > +
> >  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> >  {
> >       if (!gc->parent)
> > @@ -85,6 +89,21 @@ static struct gpio_desc *acpi_get_gpiod(char *path,
> > int pin)
> >       return gpiochip_get_desc(chip, pin);
> >  }
> >
> > +static void acpi_gpio_add_to_initial_sync_list(struct acpi_gpio_event
> > *event)
> > +{
> > +     mutex_lock(&acpi_gpio_initial_sync_list_lock);
> > +     list_add(&event->initial_sync_list,
> > &acpi_gpio_initial_sync_list);
> > +     mutex_unlock(&acpi_gpio_initial_sync_list_lock);
> > +}
> > +
> > +static void acpi_gpio_del_from_initial_sync_list(struct
> > acpi_gpio_event *event)
> > +{
> > +     mutex_lock(&acpi_gpio_initial_sync_list_lock);
> > +     if (!list_empty(&event->initial_sync_list))
> > +             list_del_init(&event->initial_sync_list);
> > +     mutex_unlock(&acpi_gpio_initial_sync_list_lock);
> > +}
> > +
> >  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> >  {
> >       struct acpi_gpio_event *event = data;
> > @@ -136,7 +155,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 +186,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");
> > @@ -208,6 +229,7 @@ static acpi_status
> > acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> >       event->irq = irq;
> >       event->pin = pin;
> >       event->desc = desc;
> > +     INIT_LIST_HEAD(&event->initial_sync_list);
> >
> >       ret = request_threaded_irq(event->irq, NULL, handler,
> > irqflags,
> >                                  "ACPI:Event", event);
> > @@ -222,6 +244,18 @@ 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.  Note we run the handlers on late_init, the
> > AML code
> > +      * may refer to OperationRegions from other (builtin) drivers
> > which
> > +      * may be probed after us.
> > +      */
> > +     if (handler == acpi_gpio_irq_handler &&
> > +         (((irqflags & IRQF_TRIGGER_RISING) && value == 1) ||
> > +          ((irqflags & IRQF_TRIGGER_FALLING) && value == 0)))
> > +             acpi_gpio_add_to_initial_sync_list(event);
> > +
> >       return AE_OK;
> >
> >  fail_free_event:
> > @@ -294,6 +328,8 @@ void acpi_gpiochip_free_interrupts(struct
> > gpio_chip *chip)
> >       list_for_each_entry_safe_reverse(event, ep, &acpi_gpio-
> > >events, node) {
> >               struct gpio_desc *desc;
> >
> > +             acpi_gpio_del_from_initial_sync_list(event);
> > +
> >               if (irqd_is_wakeup_set(irq_get_irq_data(event->irq)))
> >                       disable_irq_wake(event->irq);
> >
> > @@ -1158,3 +1194,21 @@ bool acpi_can_fallback_to_crs(struct
> > acpi_device *adev, const char *con_id)
> >
> >       return con_id == NULL;
> >  }
> > +
> > +/* Sync the initial state of handlers after all builtin drivers have
> > probed */
> > +static int acpi_gpio_initial_sync(void)
> > +{
> > +     struct acpi_gpio_event *event, *ep;
> > +
> > +     mutex_lock(&acpi_gpio_initial_sync_list_lock);
> > +     list_for_each_entry_safe(event, ep,
> > &acpi_gpio_initial_sync_list,
> > +                              initial_sync_list) {
> > +             acpi_evaluate_object(event->handle, NULL, NULL,
> > NULL);
> > +             list_del_init(&event->initial_sync_list);
> > +     }
> > +     mutex_unlock(&acpi_gpio_initial_sync_list_lock);
> > +
> > +     return 0;
> > +}
>
> > +/* We must use _sync so that this runs after the first deferred_probe
> > run */
>
> I think you may remove _ from above comment.
>
> > +late_initcall_sync(acpi_gpio_initial_sync);
>
> --
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html