Re: [PATCH v2] leds: apu: extend support for PC Engines APU1 with newer firmware

[Enrico Weigelt, metux IT consult]

On 16.02.21 14:30, Andreas Eberlein wrote:

Hi,

> The DMI_PRODUCT_NAME entry on current firmware of PC Engines APU1 changed
> from "APU" to "apu1"
>
> This modification adds the missing DMI data and thereby the LED support for
> the PC Engines APU1 with firmware versions >= 4.6.8.

Do you have a device for more intensive testing ?

In that case I'd like to suggest splitting the driver into gpio and
gpio-based LED (using leds-gpio) - just like already I did for apu2/3/4.
Maybe this even could also be moveed into the apu2 driver. This probably
just makes sense if there're more gpio-connected devices than just LED)

Personally, I don't have access to the old apu1 board (IIRC not even
produced anymore for several years), so I didn't dare to touch anything
here.

Note that apu1 vs. apu2/3/4 have completely different SOC with different
gpio logic - that was one of the reasons for writing a completely new
driver for apu2+ from scrath, rather than extending the old one.

> --- a/drivers/leds/leds-apu.c
> +++ b/drivers/leds/leds-apu.c
> @@ -83,6 +83,7 @@ static const struct apu_led_profile apu1_led_profile[] = {
>   };
>   
>   static const struct dmi_system_id apu_led_dmi_table[] __initconst = {
> +	/* PC Engines APU with factory bios "SageBios_PCEngines_APU-45" */
>   	{
>   		.ident = "apu",
>   		.matches = {
> @@ -90,6 +91,14 @@ static const struct dmi_system_id apu_led_dmi_table[] __initconst = {
>   			DMI_MATCH(DMI_PRODUCT_NAME, "APU")
>   		}
>   	},
> +	/* PC Engines APU with "Mainline" bios >= 4.6.8 */
> +	{
> +		.ident = "apu",
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "apu1")
> +		}
> +	},
>   	{}
>   };
>   MODULE_DEVICE_TABLE(dmi, apu_led_dmi_table);
> @@ -173,7 +182,7 @@ static int __init apu_led_init(void)
>   	int err;
>   
>   	if (!(dmi_match(DMI_SYS_VENDOR, "PC Engines") &&
> -	      dmi_match(DMI_PRODUCT_NAME, "APU"))) {
> +	      (dmi_match(DMI_PRODUCT_NAME, "APU") || dmi_match(DMI_PRODUCT_NAME, "apu1")))) {
>   		pr_err("No PC Engines APUv1 board detected. For APUv2,3 support, enable CONFIG_PCENGINES_APU2\n");
>   		return -ENODEV;
>   	}

Looks good to me. But don't dare giving official ack, since I don't
have an apu1 board for testing.

Is Alan Mizrahi (original author) still here ?


--mtx


--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@xxxxxxxxx -- +49-151-27565287