Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 2/17/23 14:50, Enrico Weigelt, metux IT consult wrote:
> On 13.02.23 15:25, Hans de Goede wrote:
> 
>> It would be good to know the ACPI names. Also what happens with the ACPI
>> registered LED devices when the pcengines-apu driver loads, does it
>> somehow unregister those ?
> 
> It doesn't (hasn't any code for that). But the even more interesting
> question is: does the acpi driver lock the IO space so my gpio driver,
> and so the apu driver itself, can't initialize at all ? Or are we in a
> situation where two different drivers meddle with the same chip ?

Since it seems that people are using a patched version of
the pcengines-apu driver with openwrt on newer models I would assume
that the answer here is: "No the ACPI code does not lock the IO space"
although it is unclear to me how the ACPI code exposes leds at all ?

Later in the thread you write that they do some hacks to expose
all io-space of the FCH as gpios and then presumably the embed
devicetree-bits pointing to those new GPIOs into ACPI to make
the leds-gpio.c code handle the LEDs ?

> 
>> If the ACPI LEDs are not unregistered and keep working, then I guess there is no userspace
>> API breakage when using pcengines-apu on newer APU models, "just" duplicate LED devices ?
> 
> Supporting both LED name schemes on the newer boards is an interesting
> thought. Do we have some way for aliasing LED names ?
> 
> OTOH, I wonder whether we need the model specific LED naming at all.
> In the old apuv1 driver, there used to be some (unsupported) LED-only
> support for apuv2, which used model-specific naming. When adding full
> apuv2/v3 support, I specifically chose not to do this, since I don't want userland having to care about the specific model version. And the
> naming is a bit more clear on the actual meaning of these LEDs.
> 
>>> - Additionally, we already broke this in the (distant) past because there was a previous APU driver
>>> which used different names still...
>>
>> Just because we have gotten away with it once, does not mean we should do it again :)
> 
> Back then the situation was different. Haven't even found anybody who's
> was actually using this in the field. This ancient driver (actually made
> for acpuv1, which is totally different HW) was only serving the three
> front LEDs, nothing else, and blocked using the other GPIOs (eg. button)
> Some people out there did weird hacks by directly writing registers from
> userland (obviously w/o loading the ancient driver) - and even worse:
> pcengines publically adviced to so.
> 
>> For the new models I'm fine with whatever LED naming is preferred.
> 
> NAK. The problem here is: userland now has to differenciate between
> various models again. Applications suddenly need to be rewritten in
> order to work with the next higher model, or it fails. That might be not
> a problem for home users, but in the industrial field it is a huge
> problem: you suddenly end up with two product configurations or need
> extra SW complexity to cope with several models at runtime - and this
> even grows with the next one.
> 
> Exactly what I wanted to prevent once and for all.

Note that making applications work OOTB on newer platforms is nice
to have, but is not specifically a blocker from the upstream kernel
review pov.  Userspace needing to adjust to e.g. /dev/sda becoming
/dev/nvme is not considerer userspace API breakage and this is
more or less the same.

Still I agree that preserving the userspace API across different
board models is something which we want to do if possible.

> Since the meaning of these LEDs doesn't change, there's just no need to
> change their naming.

I agree that if the LEDs have the same function as before the name should
be preserved to not needlessly make life harder for userspace consumers
of these LEDs.

Regards,

Hans