Re: [PATCH v4 3/9] gpiolib: acpi: Take into account debounce settings

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

+Cc: my work address back (by some reason Gmail(?) dropped it)

On Mon, Nov 9, 2020 at 1:53 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> On 11/9/20 12:45 PM, Andy Shevchenko wrote:
> > On Sun, Nov 08, 2020 at 10:31:32AM +0100, Hans de Goede wrote:
> >> On 11/7/20 4:26 PM, Andy Shevchenko wrote:
> >>> On Sat, Nov 7, 2020 at 4:49 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>>> On 11/6/20 8:22 PM, Andy Shevchenko wrote:
> >
> > ...
> >
> >>> Thank you very much for the testing! I remember that I fixed debounce
> >>> for BayTrail, but it seems I have yet to fix Cherry Trail pin control
> >>> as a prerequisite to this patch.
> >>>
> >>> And like I said this series is definitely not for backporting.
> >>
> >> Independent of fixing the CherryTrail pinctrl driver to support this,
> >> I strongly believe that -ENOTSUPP should be ignored (treated as success)
> >> by this patch. Remember ACPI is not only used on x86 but also on ARM
> >> now a days. We simply cannot guarantee that all pinctrls will support
> >> (let alone implement) debounce settings. E.g. I'm pretty sure that
> >> the pinctrl on the popular Allwinner A64 does not support debouncing
> >> and there are builts using a combination of uboot + EDK2 to boot!
> >>
> >> The documentation for gpiod_set_debounce even explicitly mentioned that
> >> -ENOTSUPP is an error which one may expect (and thus treat specially).
> >>
> >> The same goes for the bias stuff too.
> >
> > While for debounce I absolutely agree with you I don't think it applies to
> > bias. ACPI table is coupled with a platform and setting bias == !PullNone

s/None/Default/

> > implies that bias is supported.
>
> What about PullDefault ? I can easily see DSDT writers using PullDefault
> on platforms where bias setting is not supported.

PullDefault is ASIS in terms of BIAS, it's always supported.
I mistakenly took None in the upper paragraph, but I got your point.

> > If we break something with this it means:
> > - ACPI table is broken and we need a quirk
>
> Broken ACPI tables are as common as rain in the Netherlands, where ever
> possible we want to deal with these / workaround the brokenness
> without requiring per device quirks. Requiring a per device quirk for
> every broken ACPI table out there does not scale.

Okay.

> > - GPIO library is broken on architectural level and needs not to return
> >   ENOTSUPP for the flags configuration.
>
> Usually we handle features not being implemented gracefully. E.g chances
> are good that whatever bias is required was already setup by the firmware
> (or bootloader in the uboot + EDK2 case). Making bias set failures a
> critical error will likely regress working platforms in various cases.
>
> Keep in mind we have an existing userbase where things is working fine
> without taking the bias settings into account now. So if we hit the
> -ENOTSUPP case on any platform out there, then that is a regression
> plain and simple and as you know regressions are a big red flag.

Yeah, probably the best is to have these things optional and thus
return only real error cases.

> Since it is really easy to avoid the regression here we really
> should avoid it IMHO. What about just printing a warning on ENOTSUPP
> when setting the bias instead ?

So, looking into this deeper I have got the following:

FUNC            Relation to ENOTSUPP

gpiod_set_config()  returns if not supported
gpiod_set_debounce()  as gpiod_set_config() above
gpio_set_debounce()  legacy wrapper on top of gpiod_set_debounce()
gpiod_set_transitory()  skips it (returns okay) with debug message
gpio_set_config()  returns if not supported
gpio_set_bias()  skips it (returns okay)

Now, what the heck are the last ones? Why do we have this difference?
(I meant APIs)

So, I will try to unify it. Not sure that we need to issue a message
in case something is not supported. Debug one? Might be, but don't
think we need it right now.


-- 
With Best Regards,
Andy Shevchenko