Re: [PATCH v1 1/3] gpiolib: acpi: Respect bias settings for GpioInt() resource

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Oct 22, 2020 at 12:32 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 10/21/20 6:38 PM, Andy Shevchenko wrote:
> > On Wed, Oct 21, 2020 at 12:58:54PM +0300, Mika Westerberg wrote:
> >> On Wed, Oct 14, 2020 at 04:31:52PM +0300, Andy Shevchenko wrote:
> >>> In some cases the GpioInt() resource is coming with bias settings
> >>> which may affect system functioning. Respect bias settings for
> >>> GpioInt() resource by calling acpi_gpio_update_gpiod_*flags() API
> >>> in acpi_dev_gpio_irq_get().
> >>>
> >>> While at it, refactor to configure flags first and, only when succeeded,
> >>> map the IRQ descriptor.
> >
> > ...
> >
> >>> -                   irq = gpiod_to_irq(desc);
> >>> -                   if (irq < 0)
> >>> -                           return irq;
> >>> +                   acpi_gpio_update_gpiod_flags(&dflags, &info);
> >>> +                   acpi_gpio_update_gpiod_lookup_flags(&lflags, &info);
> >>>
> >>>                     snprintf(label, sizeof(label), "GpioInt() %d", index);
> >>> -                   ret = gpiod_configure_flags(desc, label, lflags, info.flags);
> >>> +                   ret = gpiod_configure_flags(desc, label, lflags, dflags);
> >>>                     if (ret < 0)
> >>>                             return ret;
> >>>
> >>> +                   irq = gpiod_to_irq(desc);
> >>> +                   if (irq < 0)
> >>> +                           return irq;
> >>
> >> Should the above be undone if the conversion here fails?
> >
> > But wouldn't it be not good if we changed direction, for example, and then
> > change it back? (IRQ requires input, which is safer, right?)
> >
> > This makes me think what gpiod_to_irq() may do for physical state of the pin.
> > On the brief search it seems there is no side effect on the pin with that
> > function, so, perhaps the original order has that in mind to not shuffle with
> > line if mapping can't be established. But if setting flags fail, we may got
> > into the state which is not equal to the initial one, right?
> >
> > So, in either case I see no good way to roll back the physical pin state
> > changes. But I can return ordering of the calls in next version.
> >
> > What do you think?
>
> I think it would be good to do a new version where you keep the original
> ordering.
>
> Also if you decide to keep the ordering change, that really should be
> in a separate commit and not squashed into this one, so that e.g. a bisect
> can determine the difference between the ordering change or the flags
> changes causing any issues.

Ack. Thanks Hans, Mika for your comments! I'll revert that piece of
change. I dunno what I had in mind when I did it in the first place...

-- 
With Best Regards,
Andy Shevchenko