Re: RFC: pinctrl: baytrail: Need to fixup mux setting due to broken BIOS/DSDT

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Jan 15, 2022 at 6:45 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi Andy, Mika,
>
> For one of the x86 Android tablets with broken DSDTs which I'm working on
> I need to change the mux value of pin 6 of SUS aka INT33FC:02 from 0 to 1,
> this changes it from a normal GPIO to outputting the PMC's 32KHz clk.
> This is needed for the jack-detection in the audio codec which needs an
> external 32KHz clock to work and that is connected to pin 6 of SUS.
>
> On the Windows version of the same tablet (which uses slightly different
> hardware, e.g. there is an embedded controller on the board which the
> Android version lacks) there is an ACPI call to toggle the mux, since
> the firmware does not set it for us.
>
> So the x86-android-tablets.c code for working around all the firmware
> challenges on these devices will need a way to toggle the mux and
> directly poking it itself is a bad idea because of the need
> to serialize all accesses to the GPIO islands on byt, see:
>
> 39ce8150a079 ("pinctrl: baytrail: Serialize all register access")
>
> So I see 2 possible options:
>
> 1. Add a pingroup for this pin in drivers/pinctrl/intel/pinctrl-baytrail.c
> and then mimick the pwm0 pinconf setting code from
> drivers/gpu/drm/i915/display/intel_dsi_vbt.c in x86-android-tablets.c
> This seems the cleanest, but I'm leaning a bit towards:

If you meant
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/i915/display/intel_dsi_vbt.c#L858
this is definitely the way to go.

> 2. Do EXPORT_SYMBOL_GPL(byt_lock); in pinctrl-baytrail.c and then just
> do a ioremap + raw poke of the pad_conf0 register in x86-android-tablets.c.

Sorry, I do not like the 2. at all. Let's keep that lock private to the driver.



> This avoids adding an extra pingroup just to workaround around the
> buggy firmware on these tablets; and more importantly this gives the
> x86-android-tablets.c code a flexible way to fixup various possible
> GPIO/pinctrl misconfigurations with needing to push workarounds into
> pinctrl-baytrail.c.
>
> To make sure others are not tempted to also take the byt_lock (which
> needs a rename before exporting) I plan to not add byt_lock to any header
> files. Instead the x86-android-tablets.c code can just do an extern
> declaration of it inside the .c file.

This is just a point to be against doing this. We are gaining
(perhaps) more cleanups against W=1 and CONFIG_WERROR=y and this case
might not work well with that (IIRC something similar in arch/x86 when
I tried that combination).

> ###
>
> Typing this out I think that 1 is cleaner and so far on a number of buggy
> DSDT Android tablets this is the only case I've found where poking the
> GPIO/pinctrl registers "directly" is necessary.
>
> So I guess I should go with solution 1 and if it turns out more workarounds
> are necessary then we can reconsider the (somewhat ugly) option 2.
>
> Before moving forward with either approach I would like to hear your
> thoughts on this, so please let me know what you think ?
>
> Regards,
>
> Hans
>
>


-- 
With Best Regards,
Andy Shevchenko