[PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed Wildgoose]

The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
will cause the kernel to automatically create led + gpio_key devices for
the platform. This means that the platform setup now creates duplicates
of all these led/key devices.

Anyone with a much older bios can use the 'leds-apu' driver to get the
same set of led devices as created by the kernel with a modern bios.

Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
---
 drivers/platform/x86/pcengines-apuv2.c | 76 +-------------------------
 1 file changed, 1 insertion(+), 75 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index c37349f97..41e3005cd 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -72,60 +72,6 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
 	.gpio_names	= apu2_gpio_names,
 };
 
-/* GPIO LEDs device */
-
-static const struct gpio_led apu2_leds[] = {
-	{ .name = "apu:green:1" },
-	{ .name = "apu:green:2" },
-	{ .name = "apu:green:3" },
-};
-
-static const struct gpio_led_platform_data apu2_leds_pdata = {
-	.num_leds	= ARRAY_SIZE(apu2_leds),
-	.leds		= apu2_leds,
-};
-
-static struct gpiod_lookup_table gpios_led_table = {
-	.dev_id = "leds-gpio",
-	.table = {
-		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_LED1,
-				NULL, 0, GPIO_ACTIVE_LOW),
-		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_LED2,
-				NULL, 1, GPIO_ACTIVE_LOW),
-		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_LED3,
-				NULL, 2, GPIO_ACTIVE_LOW),
-	}
-};
-
-/* GPIO keyboard device */
-
-static struct gpio_keys_button apu2_keys_buttons[] = {
-	{
-		.code			= KEY_RESTART,
-		.active_low		= 1,
-		.desc			= "front button",
-		.type			= EV_KEY,
-		.debounce_interval	= 10,
-		.value			= 1,
-	},
-};
-
-static const struct gpio_keys_platform_data apu2_keys_pdata = {
-	.buttons	= apu2_keys_buttons,
-	.nbuttons	= ARRAY_SIZE(apu2_keys_buttons),
-	.poll_interval	= 100,
-	.rep		= 0,
-	.name		= "apu2-keys",
-};
-
-static struct gpiod_lookup_table gpios_key_table = {
-	.dev_id = "gpio-keys-polled",
-	.table = {
-		GPIO_LOOKUP_IDX(AMD_FCH_GPIO_DRIVER_NAME, APU2_GPIO_LINE_MODESW,
-				NULL, 0, GPIO_ACTIVE_LOW),
-	}
-};
-
 /* Board setup */
 
 /* Note: matching works on string prefix, so "apu2" must come before "apu" */
@@ -217,8 +163,6 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 };
 
 static struct platform_device *apu_gpio_pdev;
-static struct platform_device *apu_leds_pdev;
-static struct platform_device *apu_keys_pdev;
 
 static struct platform_device * __init apu_create_pdev(
 	const char *name,
@@ -247,38 +191,20 @@ static int __init apu_board_init(void)
 
 	id = dmi_first_match(apu_gpio_dmi_table);
 	if (!id) {
-		pr_err("failed to detect APU board via DMI\n");
+		pr_err("No APU board detected via DMI\n");
 		return -ENODEV;
 	}
 
-	gpiod_add_lookup_table(&gpios_led_table);
-	gpiod_add_lookup_table(&gpios_key_table);
-
 	apu_gpio_pdev = apu_create_pdev(
 		AMD_FCH_GPIO_DRIVER_NAME,
 		id->driver_data,
 		sizeof(struct amd_fch_gpio_pdata));
 
-	apu_leds_pdev = apu_create_pdev(
-		"leds-gpio",
-		&apu2_leds_pdata,
-		sizeof(apu2_leds_pdata));
-
-	apu_keys_pdev = apu_create_pdev(
-		"gpio-keys-polled",
-		&apu2_keys_pdata,
-		sizeof(apu2_keys_pdata));
-
 	return 0;
 }
 
 static void __exit apu_board_exit(void)
 {
-	gpiod_remove_lookup_table(&gpios_led_table);
-	gpiod_remove_lookup_table(&gpios_key_table);
-
-	platform_device_unregister(apu_keys_pdev);
-	platform_device_unregister(apu_leds_pdev);
 	platform_device_unregister(apu_gpio_pdev);
 }
 
-- 
2.26.2

[PATCH 2/2] x86: Support APU5 in PCEngines platform driver

[Ed Wildgoose]

PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
It also has support for 3x LTE modems with 6x SIM slots (pairs with a
SIM switch device). Each mpcie slot for modems has a reset GPIO

To ensure that the naming is sane between APU2/3/4/5 I have renamed the
reset GPIOs to be modem1-reset, modem2-reset, etc. This is significant
because the slots that can be reset change between APU2 and APU3/4

GPIO for simswap is moved to the end of the list as it could be dropped
for APU2 boards (but causes no harm to leave it in, hardware could be
added to a future rev of the board).

Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
many lines are moved around and there are simply more
modems/resets/sim-swap lines to breakout.

Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
---
 drivers/platform/x86/pcengines-apuv2.c | 175 +++++++++++++++++++------
 1 file changed, 136 insertions(+), 39 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index 41e3005cd..4dfc9ff9f 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 /*
- * PC-Engines APUv2/APUv3 board platform driver
+ * PC-Engines APUv2-5 board platform driver
  * for GPIO buttons and LEDs
  *
  * Copyright (C) 2018 metux IT consult
@@ -22,38 +22,73 @@
 #include <linux/platform_data/gpio/gpio-amd-fch.h>
 
 /*
- * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
+ * NOTE: this driver only supports APUv2-5 - not APUv1, as this one
  * has completely different register layouts.
  */
 
+/*
+ * There are a number of APU2 variants, with differing features
+ * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
+ * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
+ * However, most APU3/4 have a SIM switch which we default on to reverse
+ * the order and keep physical SIM order matching physical modem order
+ *
+ * APU5 has 3x SIM sockets, all with a SIM switch
+ * several GPIOs are shuffled (see schematic), including MODESW
+ */
+
+/* Match types for dmi */
+#define DMI_MATCH_APU2	2
+#define DMI_MATCH_APU5	5
+
 /* Register mappings */
 #define APU2_GPIO_REG_LED1		AMD_FCH_GPIO_REG_GPIO57
 #define APU2_GPIO_REG_LED2		AMD_FCH_GPIO_REG_GPIO58
 #define APU2_GPIO_REG_LED3		AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
 #define APU2_GPIO_REG_MODESW		AMD_FCH_GPIO_REG_GPIO32_GE1
 #define APU2_GPIO_REG_SIMSWAP		AMD_FCH_GPIO_REG_GPIO33_GE2
-#define APU2_GPIO_REG_MPCIE2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
-#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+
+#define APU5_GPIO_REG_MODESW		AMT_FCH_GPIO_REG_GEVT22
+#define APU5_GPIO_REG_SIMSWAP1		AMD_FCH_GPIO_REG_GPIO68
+#define APU5_GPIO_REG_SIMSWAP2		AMD_FCH_GPIO_REG_GPIO32_GE1
+#define APU5_GPIO_REG_SIMSWAP3		AMD_FCH_GPIO_REG_GPIO33_GE2
+#define APU5_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU5_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+#define APU5_GPIO_REG_RESETM3		AMD_FCH_GPIO_REG_GPIO64
 
 /* Order in which the GPIO lines are defined in the register list */
 #define APU2_GPIO_LINE_LED1		0
 #define APU2_GPIO_LINE_LED2		1
 #define APU2_GPIO_LINE_LED3		2
 #define APU2_GPIO_LINE_MODESW		3
-#define APU2_GPIO_LINE_SIMSWAP		4
-#define APU2_GPIO_LINE_MPCIE2		5
-#define APU2_GPIO_LINE_MPCIE3		6
+#define APU2_GPIO_LINE_RESETM1		4
+#define APU2_GPIO_LINE_RESETM2		5
+#define APU2_GPIO_LINE_SIMSWAP		6
 
-/* GPIO device */
+#define APU5_GPIO_LINE_LED1		0
+#define APU5_GPIO_LINE_LED2		1
+#define APU5_GPIO_LINE_LED3		2
+#define APU5_GPIO_LINE_MODESW		3
+#define APU5_GPIO_LINE_RESETM1		4
+#define APU5_GPIO_LINE_RESETM2		5
+#define APU5_GPIO_LINE_RESETM3		6
+#define APU5_GPIO_LINE_SIMSWAP1		7
+#define APU5_GPIO_LINE_SIMSWAP2		8
+#define APU5_GPIO_LINE_SIMSWAP3		9
+
+
+/* GPIO device - APU2/3/4 */
 
 static int apu2_gpio_regs[] = {
 	[APU2_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
 	[APU2_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
 	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
 	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
+	[APU2_GPIO_LINE_RESETM1]	= APU2_GPIO_REG_RESETM1,
+	[APU2_GPIO_LINE_RESETM2]	= APU2_GPIO_REG_RESETM2,
 	[APU2_GPIO_LINE_SIMSWAP]	= APU2_GPIO_REG_SIMSWAP,
-	[APU2_GPIO_LINE_MPCIE2]		= APU2_GPIO_REG_MPCIE2,
-	[APU2_GPIO_LINE_MPCIE3]		= APU2_GPIO_REG_MPCIE3,
 };
 
 static const char * const apu2_gpio_names[] = {
@@ -61,9 +96,9 @@ static const char * const apu2_gpio_names[] = {
 	[APU2_GPIO_LINE_LED2]		= "front-led2",
 	[APU2_GPIO_LINE_LED3]		= "front-led3",
 	[APU2_GPIO_LINE_MODESW]		= "front-button",
+	[APU2_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU2_GPIO_LINE_RESETM2]	= "modem2-reset",
 	[APU2_GPIO_LINE_SIMSWAP]	= "simswap",
-	[APU2_GPIO_LINE_MPCIE2]		= "mpcie2_reset",
-	[APU2_GPIO_LINE_MPCIE3]		= "mpcie3_reset",
 };
 
 static const struct amd_fch_gpio_pdata board_apu2 = {
@@ -72,8 +107,55 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
 	.gpio_names	= apu2_gpio_names,
 };
 
+/* GPIO device - APU5 */
+
+static int apu5_gpio_regs[] = {
+	[APU5_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
+	[APU5_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
+	[APU5_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
+	[APU5_GPIO_LINE_MODESW]		= APU5_GPIO_REG_MODESW,
+	[APU5_GPIO_LINE_RESETM1]	= APU5_GPIO_REG_RESETM1,
+	[APU5_GPIO_LINE_RESETM2]	= APU5_GPIO_REG_RESETM2,
+	[APU5_GPIO_LINE_RESETM3]	= APU5_GPIO_REG_RESETM3,
+	[APU5_GPIO_LINE_SIMSWAP1]	= APU5_GPIO_REG_SIMSWAP1,
+	[APU5_GPIO_LINE_SIMSWAP2]	= APU5_GPIO_REG_SIMSWAP2,
+	[APU5_GPIO_LINE_SIMSWAP3]	= APU5_GPIO_REG_SIMSWAP3,
+};
+
+static const char * const apu5_gpio_names[] = {
+	[APU5_GPIO_LINE_LED1]		= "front-led1",
+	[APU5_GPIO_LINE_LED2]		= "front-led2",
+	[APU5_GPIO_LINE_LED3]		= "front-led3",
+	[APU5_GPIO_LINE_MODESW]		= "front-button",
+	[APU5_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU5_GPIO_LINE_RESETM2]	= "modem2-reset",
+	[APU5_GPIO_LINE_RESETM3]	= "modem3-reset",
+	[APU5_GPIO_LINE_SIMSWAP1]	= "simswap1",
+	[APU5_GPIO_LINE_SIMSWAP2]	= "simswap2",
+	[APU5_GPIO_LINE_SIMSWAP3]	= "simswap3",
+};
+
+static const struct amd_fch_gpio_pdata board_apu5 = {
+	.gpio_num	= ARRAY_SIZE(apu5_gpio_regs),
+	.gpio_reg	= apu5_gpio_regs,
+	.gpio_names	= apu5_gpio_names,
+};
+
+
 /* Board setup */
 
+struct apu_driver_data {
+	const struct amd_fch_gpio_pdata *board_data;
+};
+
+static struct apu_driver_data apu2_driver_data = {
+	.board_data = &board_apu2,
+};
+
+static struct apu_driver_data apu5_driver_data = {
+	.board_data = &board_apu5,
+};
+
 /* Note: matching works on string prefix, so "apu2" must come before "apu" */
 static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 
@@ -84,25 +166,25 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU2 w/ legacy BIOS >= 4.0.8 */
+	/* APU2 w/ mainline BIOS */
 	{
 		.ident		= "apu2",
 		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU2 w/ mainline BIOS */
+	/* APU2 w/ legacy BIOS >= 4.0.8 */
 	{
 		.ident		= "apu2",
 		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
 
 	/* APU3 w/ legacy BIOS < 4.0.8 */
@@ -112,52 +194,61 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU3 w/ legacy BIOS >= 4.0.8 */
+	/* APU3 w/ mainline BIOS */
 	{
-		.ident       = "apu3",
-		.matches     = {
+		.ident		= "apu3",
+		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU3 w/ mainline BIOS */
+	/* APU3 w/ legacy BIOS >= 4.0.8 */
 	{
-		.ident       = "apu3",
-		.matches     = {
+		.ident		= "apu3",
+		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
 	/* APU4 w/ legacy BIOS < 4.0.8 */
 	{
-		.ident        = "apu4",
-		.matches    = {
+		.ident		= "apu4",
+		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU4 w/ legacy BIOS >= 4.0.8 */
+	/* APU4 w/ mainline BIOS */
 	{
-		.ident       = "apu4",
-		.matches     = {
+		.ident		= "apu4",
+		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
-	/* APU4 w/ mainline BIOS */
+	/* APU4 w/ legacy BIOS >= 4.0.8 */
 	{
-		.ident       = "apu4",
-		.matches     = {
+		.ident		= "apu4",
+		.matches	= {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
+	},
+	/* APU5 w/ mainline BIOS */
+	{
+		.ident		= "apu5",
+		.matches	= {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu5")
+		},
+		.driver_data	= (void *)&apu5_driver_data,
 	},
 	{}
 };
@@ -188,16 +279,22 @@ static struct platform_device * __init apu_create_pdev(
 static int __init apu_board_init(void)
 {
 	const struct dmi_system_id *id;
+	const struct apu_driver_data *driver_data;
 
 	id = dmi_first_match(apu_gpio_dmi_table);
 	if (!id) {
 		pr_err("No APU board detected via DMI\n");
 		return -ENODEV;
+	} else {
+		pr_debug("Detected APU board: %s\n",
+				dmi_get_system_info(DMI_BOARD_NAME));
 	}
 
+	driver_data = id->driver_data;
+
 	apu_gpio_pdev = apu_create_pdev(
 		AMD_FCH_GPIO_DRIVER_NAME,
-		id->driver_data,
+		driver_data->board_data,
 		sizeof(struct amd_fch_gpio_pdata));
 
 	return 0;
@@ -212,7 +309,7 @@ module_init(apu_board_init);
 module_exit(apu_board_exit);
 
 MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
-MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
+MODULE_DESCRIPTION("PC Engines APUv2-5 board GPIO/LEDs/keys driver");
 MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
 MODULE_ALIAS("platform:pcengines-apuv2");
-- 
2.26.2

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

Hi, I've been adding support for the PC Engines APU5 board, which is a variant of the APU 2-4 boards
with some nice features. The current platform driver for pcengines boards has some redundant
features with regards to recent bios/firmware packages for the board as they now set the ACPI tables
to indicate GPIOs for keys and leds. So I've submitted a patch to eliminate this. It could be argued
that it's useful to support older firmware versions, but there is also a 'leds-apu' driver which a)
probably ought to be marked deprecated with a view to removing it and b) implements the leds even on
antique firmware versions.

In implementing the APU5 I changed some of the exported gpio names to make them more closely match
functionality across all the boards. For example APU2 vs APU4 both support 2x LTE options, but in
different mpcie slots and this affects the numbering of options, but not the sense of them (so I
renamed them based on the intention of the option). This is particularly true on APU5 which supports
3x LTE cards


Can I get some advice: It would be helpful if the kernel would export the GPIOs to user-space
automatically since toggling SIM slots is fairly useful task in userspace. At least for me the gpio
numbers seem to jump around depending on the order of module loading, so doing something involving
/sys/class/gpio/export isn't obviously an easy process. Reviewing the fine documentation suggests
that I need to use gpio_export() to achieve this, but I concede I'm really not clear how to
implement this in the platform module as currently structured... Any tips please?

Thanks

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 22.09.20 00:17, Ed W wrote:
> Hi, I've been adding support for the PC Engines APU5 board, which is a variant of the APU 2-4 boards
> with some nice features. The current platform driver for pcengines boards has some redundant
> features with regards to recent bios/firmware packages for the board as they now set the ACPI tables
> to indicate GPIOs for keys and leds. 

NAK. Breaks existing userlands in the field (literally field), forcing
users to fw upgrade is not an option (field roll would be realy expensive).

> So I've submitted a patch to eliminate this. It could be argued
> that it's useful to support older firmware versions, but there is also a 'leds-apu' driver which a)
> probably ought to be marked deprecated with a view to removing it and b) implements the leds even on
> antique firmware versions.

leds-apu is only for *OLD* apu1 - it does *not* work with v2/3/4,
completely different chipset.

> In implementing the APU5 I changed some of the exported gpio names to make them more closely match
> functionality across all the boards. > For example APU2 vs APU4 both support 2x LTE options, but in
> different mpcie slots and this affects the numbering of options, but not the sense of them (so I
> renamed them based on the intention of the option). This is particularly true on APU5 which supports
> 3x LTE cards

Dont break existing userlands.

> Can I get some advice: It would be helpful if the kernel would export the GPIOs to user-space
> automatically since toggling SIM slots is fairly useful task in userspace. 

This is planned to be moved to either an own subsystem or into rfkill
(which would have to be extended for such things).

Using raw gpio's isn't a good idea - it's not portable.

> At least for me the gpio
> numbers seem to jump around depending on the order of module loading, so doing something involving
> /sys/class/gpio/export isn't obviously an easy process. 

Exactly. That's why they should be bound to more high level drivers.
Gpio numbers are anything but stable.


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Hans de Goede]

Hi Enrico and Ed W,

Quick self intro: I have take over drivers/platform/x86
maintainership from Andy; and I'm working my way through
the backlog of old patches in patchwork:
https://patchwork.kernel.org/project/platform-driver-x86/list/

On 10/12/20 9:39 PM, Enrico Weigelt, metux IT consult wrote:
> On 22.09.20 00:17, Ed W wrote:
>> Hi, I've been adding support for the PC Engines APU5 board, which is a variant of the APU 2-4 boards
>> with some nice features. The current platform driver for pcengines boards has some redundant
>> features with regards to recent bios/firmware packages for the board as they now set the ACPI tables
>> to indicate GPIOs for keys and leds.
> 
> NAK. Breaks existing userlands in the field (literally field), forcing
> users to fw upgrade is not an option (field roll would be realy expensive).

Thank you Enrico, I was wondering the same (what about userspace breakage)
when I was looking at this patch. It is good to have confirmation that
userspace breakage is a real issue here.

And I completely agree with Enrico, we cannot go and break userspace,
so some other solution will need to be found here.

I'm marking this series as changes requested in patchwork, please submit
a new version which does not break existing userspace.

Regards,

Hans

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

On 13/10/2020 09:48, Hans de Goede wrote:

> On 10/12/20 9:39 PM, Enrico Weigelt, metux IT consult wrote:
>> On 22.09.20 00:17, Ed W wrote:
>>> Hi, I've been adding support for the PC Engines APU5 board, which is a variant of the APU 2-4
>>> boards
>>> with some nice features. The current platform driver for pcengines boards has some redundant
>>> features with regards to recent bios/firmware packages for the board as they now set the ACPI
>>> tables
>>> to indicate GPIOs for keys and leds.
>>
>> NAK. Breaks existing userlands in the field (literally field), forcing
>> users to fw upgrade is not an option (field roll would be realy expensive).
>
> Thank you Enrico, I was wondering the same (what about userspace breakage)
> when I was looking at this patch. It is good to have confirmation that
> userspace breakage is a real issue here.


This isn't the whole story.

The original naming was board specific. Then Enrico (not unreasonably - I actually prefer his
naming) changed the naming to be non board specific. Then within 2 months PC Engines introduced ACPI
based config using the old names.

So if we are holding "userspace breakage" as the gold standard, then the original (also the current)
names have actually been around longest and likely cause the least userspace breakage.

Also, some other pieces of this module have already been removed (SIM Swap), so there is an existing
precedent for "userspace breakage" and trimming down this platform driver.


In big picture terms, changing the name of the LED device doesn't seem a huge concern to me... A
udev rule can setup compatibility forwards/backwards quite trivially I think?

Kind regards

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 13.10.20 23:46, Ed W wrote:

> The original naming was board specific. Then Enrico (not unreasonably - I actually prefer his
> naming) changed the naming to be non board specific. Then within 2 months PC Engines introduced ACPI
> based config using the old names.

Which "old names" are you referring to ?
The really old apuv1 led-only driver ?

> So if we are holding "userspace breakage" as the gold standard, then the original (also the current)
> names have actually been around longest and likely cause the least userspace breakage.

Exactly. Linus often stated "dont break userland" as a primary goal, and
that with really good reasons: the kernel is *the* hardware abstraction
layer. Having userland to deal with thousands of hardware details in
userland would cause extreme management complexity.

> Also, some other pieces of this module have already been removed (SIM Swap), so there is an existing
> precedent for "userspace breakage" and trimming down this platform driver.

Not quite. SIM swap hasn't been actually used in the field (at least as
far as I know). And we're planning to put it into different subsystem
(probably rfkill) anyways.

> In big picture terms, changing the name of the LED device doesn't seem a huge concern to me... A
> udev rule can setup compatibility forwards/backwards quite trivially I think?

Small kernel update causes existing applications to FAIL. Applications
now have to be changed to deal with *different* configuration, based on
factors like BIOS version.

We're dealing with embedded applications. There is no operator of these
boxes. Maybe some times an operator of the machinary comes around - and
needs to rely on the LEDs. Not as critial as an direction indicator in
a car, but still important.


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

On 12/10/2020 20:39, Enrico Weigelt, metux IT consult wrote:
> On 22.09.20 00:17, Ed W wrote:
>> Hi, I've been adding support for the PC Engines APU5 board, which is a variant of the APU 2-4 boards
>> with some nice features. The current platform driver for pcengines boards has some redundant
>> features with regards to recent bios/firmware packages for the board as they now set the ACPI tables
>> to indicate GPIOs for keys and leds. 
> NAK. Breaks existing userlands in the field (literally field), forcing
> users to fw upgrade is not an option (field roll would be realy expensive).


But why are users "in the field" updating a kernel willy nilly without also updating the userland
software that talks to it? Why is the kernel upgrade trivial, but the fw upgrade is not an option?
Why not also update the app or setup a udev rule?

I would understand if we were talking something fairly major, but it's the case of matching a
filename that YOU changed from an old name to the current name and it's now changing back to the
original name?


>> So I've submitted a patch to eliminate this. It could be argued
>> that it's useful to support older firmware versions, but there is also a 'leds-apu' driver which a)
>> probably ought to be marked deprecated with a view to removing it and b) implements the leds even on
>> antique firmware versions.
> leds-apu is only for *OLD* apu1 - it does *not* work with v2/3/4,
> completely different chipset.


That's extremely disingenuous!!

It USED to work for the APU2-4 except that YOU removed support for APU2-4 from that module!!


>> In implementing the APU5 I changed some of the exported gpio names to make them more closely match
>> functionality across all the boards. > For example APU2 vs APU4 both support 2x LTE options, but in
>> different mpcie slots and this affects the numbering of options, but not the sense of them (so I
>> renamed them based on the intention of the option). This is particularly true on APU5 which supports
>> 3x LTE cards
> Dont break existing userlands.


But YOU already did that!!

Look, the original situation was that:

- up to July 2019 there was a kernel module that named the LEDs in the form apu2:green:1, etc.

- In July 2019 you removed that and "broke all of userland by renaming the LEDs" (never break
userland right?)

- Then in Sept 2019, PCEngines released a new bios/firmware line which setup ACPI correctly to
register these GPIOs with some default names along the lines of apu2:green:1 or similar. So now we
are back to the original naming convention

- This meant that from Sept 2019 your module created duplicate LEDs with a different set of names


So the situationt boils down to:

- LEDs have been named like "apu2:green:1" continuously, with a brief outage in Aug-Sep 2019.

- You have introduced a new module which unnecessarily duplicates those LEDs for users of this board
with a bios/firmware post Sep 2019.

- Your new naming convention isn't the same as this historic naming convention


Now don't get me wrong, I prefer your module naming, but you are very disingenuous to claim that I'm
trying to break userspace when you already did it!

Plus I see no future for the LED piece of your module given that it's done by ACPI in all modern
bios? Do you really want a duplicate set of LEDs to exist forever in userspace? This doesn't seem to
be workable?


Lets be clear, the current situation is:

- LED names change from "apu:green:1" to "apu2:green:1".

- This can be trivially worked around with some symlinks and/or a udev rule

- The historic name has been "apu2:green:1" in the original LED module and now in ACPI. I'm not wild
about this naming convention, but it's been around longest. If one has to pick which userspace to
break, then this seems to have precedence.

- Your LED based SIM toggle HAS already gone. So you have another example of userspace being broken
right there. (Seems that this rule isn't so concrete?). So you already need to (significantly?)
adjust your userspace code - I'm not seeing how/why the LED change is such a blocker?


>> Can I get some advice: It would be helpful if the kernel would export the GPIOs to user-space
>> automatically since toggling SIM slots is fairly useful task in userspace. 
> This is planned to be moved to either an own subsystem or into rfkill
> (which would have to be extended for such things).


Can you send me a pointer to this planning? Is this something concrete or aspirational?

I need something I can use right now for SIM swap. exporting GPIOs with known names seems no more
evil than exporting LEDs with known names? Do you have any concrete suggestion for the here and now?

Given that the LED based sim swap is already removed from the kernel, how are you planning to toggle
SIM swap in userspace?


Hans, can I ask you to look again at the history of this please. Bearing in mind the speed kernel
stuff takes to get to end users, we are talking about a very small window of userland changes here.
I would vote for simplifying this module and trying to reduce some baggage. However, my main goal is
to get support in for APU5. Second goal is to reduce the duplicate LED devices. Beyond that I'm not
so fussed?

Thanks all

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Hans de Goede]

Hi,

On 10/13/20 11:40 PM, Ed W wrote:

<snip>

> Hans, can I ask you to look again at the history of this please. Bearing in mind the speed kernel
> stuff takes to get to end users, we are talking about a very small window of userland changes here.
> I would vote for simplifying this module and trying to reduce some baggage. However, my main goal is
> to get support in for APU5. Second goal is to reduce the duplicate LED devices. Beyond that I'm not
> so fussed?

Honestly I would prefer for you and Enrico to come to some sort of
consensus here, since you both know this code a lot better then I do.

If you cannot come to a consensus then I guess I will have to make
a decision here, but I would really prefer not to have to
arbitrate here.

Also note that I did already take a peek at the backlog before
Enrico's email and I was already wondering about the userspace
breakage _myself_ before Enrico chimed in. I did not reply then
because I only took a quick peek and decided to deal with the
backlog later.

As for the history of this all. Just because the userspace API
was broken once and we got away with it (IOW no body complained I
guess), does not mean that we should do this again.

Generally speaking there is a very hard rule that once shipped we
never break the userspace API and if I don't enforce that rule
then Torvalds will and in the process get angry at me
(been there done that). So sorry, but breaking userspace is
really not an option.

Also you mention in the commit messages for this patch that the
code is being removed because a new BIOS now enumerates them
through the new device-tree embedded in ACPI tables mechanism,
correct ?

That means that if people stick with the old BIOS and get a new
kernel they will loose all access to the LED functionality that
seems quite bad?  Note that also as a general, but certainly
as a pdx86 rule we try very hard to not rely on people installing
BIOS updates because whole troves of users do not install BIOS updates
(I understand that these boards are all kinds of special, so this may
apply here even more (or less so)).

So I have a suggested compromise:

Keep the current LED/gpio setup code, but make executing it conditional
on the BIOS version and skip the LED/gpio setup when the new BIOS is
present to avoid having duplicate LED entries, etc. in that case.

I guess this would still break userspace because if I understand things
correctly the new ACPI based setup uses different LED names ? That
seems unfortunate, but I guess that from the kernel pov we can just
blame the BIOS for this, and since we definitely do not want duplicate
LED entries for the same LED, this seems the least bad choice.

Enrico, would that work for you ?

Regards,

Hans

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

On 14/10/2020 09:41, Hans de Goede wrote:
>
> So I have a suggested compromise:
>
> Keep the current LED/gpio setup code, but make executing it conditional
> on the BIOS version and skip the LED/gpio setup when the new BIOS is
> present to avoid having duplicate LED entries, etc. in that case.
>
> I guess this would still break userspace because if I understand things
> correctly the new ACPI based setup uses different LED names ? That
> seems unfortunate, but I guess that from the kernel pov we can just
> blame the BIOS for this, and since we definitely do not want duplicate
> LED entries for the same LED, this seems the least bad choice.
>
> Enrico, would that work for you ? 


I'm cool with this. Enrico?

I may have some time imminently to have a stab at a new patch. Obviously any help structuring this
would be appreciated - it feels clumsy using the existing detection mechanism, I think whatever I
come up with you should kick back and recommend a new board detection structure, but perhaps we can
shortcut that step with a few comments up front?

Cheers

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Hans de Goede]

Hi,

On 10/14/20 1:21 PM, Ed W wrote:
> On 14/10/2020 09:41, Hans de Goede wrote:
>>
>> So I have a suggested compromise:
>>
>> Keep the current LED/gpio setup code, but make executing it conditional
>> on the BIOS version and skip the LED/gpio setup when the new BIOS is
>> present to avoid having duplicate LED entries, etc. in that case.
>>
>> I guess this would still break userspace because if I understand things
>> correctly the new ACPI based setup uses different LED names ? That
>> seems unfortunate, but I guess that from the kernel pov we can just
>> blame the BIOS for this, and since we definitely do not want duplicate
>> LED entries for the same LED, this seems the least bad choice.
>>
>> Enrico, would that work for you ?
> 
> 
> I'm cool with this. Enrico?
> 
> I may have some time imminently to have a stab at a new patch. Obviously any help structuring this
> would be appreciated - it feels clumsy using the existing detection mechanism, I think whatever I
> come up with you should kick back and recommend a new board detection structure, but perhaps we can
> shortcut that step with a few comments up front?

I'm afraid I do not have any wisdom to share here. I would use the DMI bios-version
or bios-date strings for the detection, but I guess that is obvious.

Other then I guess I would do a preparation patch restructuring the code so that
the whole conditional part becomes a single if, e.g.:

	if (old_bios()) {
		ret = register_leds_and_gpio_for_old_bios()
		if (ret)
			goto error_cleanup;
	}

So in a separate preparation commit put all the code which you tried to
remove earlier in a single helper function (feel free to pick a different name).

And then in that prep patch the above would look like this:

	ret = register_leds_and_gpio_for_old_bios()
	if (ret)
		goto error_cleanup;

And a follow-up commit adding the new/old bios detection would
introduce the if.

And do the same for the cleanup parts for module unloading.

I hope this helps...

Regards,

Hans

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

On 14/10/2020 12:29, Hans de Goede wrote:
> Hi,
>
> On 10/14/20 1:21 PM, Ed W wrote:
>> On 14/10/2020 09:41, Hans de Goede wrote:
>>>
>>> So I have a suggested compromise:
>>>
>>> Keep the current LED/gpio setup code, but make executing it conditional
>>> on the BIOS version and skip the LED/gpio setup when the new BIOS is
>>> present to avoid having duplicate LED entries, etc. in that case.
>>>
>>> I guess this would still break userspace because if I understand things
>>> correctly the new ACPI based setup uses different LED names ? That
>>> seems unfortunate, but I guess that from the kernel pov we can just
>>> blame the BIOS for this, and since we definitely do not want duplicate
>>> LED entries for the same LED, this seems the least bad choice.
>>>
>>> Enrico, would that work for you ?
>>
>>
>> I'm cool with this. Enrico?
>>
>> I may have some time imminently to have a stab at a new patch. Obviously any help structuring this
>> would be appreciated - it feels clumsy using the existing detection mechanism, I think whatever I
>> come up with you should kick back and recommend a new board detection structure, but perhaps we can
>> shortcut that step with a few comments up front?
>
> I'm afraid I do not have any wisdom to share here. I would use the DMI bios-version
> or bios-date strings for the detection, but I guess that is obvious. 


Hi Hans & Enrico

OK, I've just sent a new patch which conditionally configures GPIOs for boards with older firmware's
(older than 4.10.0).

This is followed up by the patch I really want to try and get in, which is to add support for APU5
and APU6. Particularly APU5 is quite interesting to me and significantly different to previous
boards in that it has a lot more mpcie slots that can be used for LTE modules or wifi cards. This
creates the realisation that the reset and sim-swap lines are always wired to the LTE slots, not to
the mpcie slots (although often they overlap in functionality), so naming is corrected here. That
said, I don't think the reset lines function on most iterations of boards, so possibly supporting
those lines with GPIOs is redundant anyway...

APU6 is also a special order and is essentially the same as an APU4, so I have added detection for
this also.

I don't know if it's useful, but I uploaded a couple of scripts for beeping and flashing the leds.
Here I just used globs to handle the different naming on the different boards (since I need to
handle the older Alix boards as well). Enrico, is this useful to you?

    https://github.com/nippynetworks/gpio-utils


As an aside, these boards are super easy to flash as they support flashrom. So I'm personally giving
some thought to bundling an updater into our software build. The generic bios is quite slow to
startup and I would like to prepare a customised version with shorter timeouts. Happy to work with
you on something separately if this is interesting?

Hans, thanks if you can look this over.

Regards

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 21.10.20 23:54, Ed W wrote:

Hi,

> OK, I've just sent a new patch which conditionally configures GPIOs for boards with older firmware's
> (older than 4.10.0).

as mentioned in my reply to it: still breaks existing userlands as
device names and keycodes change. userland would need to become
dependent on bios version. Exactly one of those kind of instability
we do NOT want.

Remember: these boards are used for embedded applications - an area
where we do not have any operator and need a very high level of
stability (sending a technician costs 10x the price of the board).

> This is followed up by the patch I really want to try and get in, which is to add support for APU5
> and APU6. Particularly APU5 is quite interesting to me and significantly different to previous
> boards in that it has a lot more mpcie slots that can be used for LTE modules or wifi cards.

Okay, why not just treating them differently ?

Personally, I don't have an apu5, so can't tell much of them yet. But if
they're so different, I can live with those having different names /
keycode, assuming all this stuff is already supported by the lowest BIOS
version they've ever been shipped with (remember: requirement of BIOS
upgrade in the field is also not an option). Can you please check
whether that's the case ?

In that case we don't need any per bios-version special magic, just
another entry in the DMI match table and separate board structs.

For the mpcie reset lines we really should consider using the reset
controller framework or add it to pci core (so pci_bus drivers also
expose it to sysfs, in order to let userland trigger it). Tricky part
is letting the board driver attach the reset functionality to the
generic pci_bus driver. (grmpf, it wasn't ugly acpi, but dt instead,
this would be pretty trivial via dt-overlay :o).

> This
> creates the realisation that the reset and sim-swap lines are always wired to the LTE slots, not to
> the mpcie slots (although often they overlap in functionality), so naming is corrected here. 

Sorry, but i'm confused. What's the actual difference between mpcie and
LTE slots ? What is an LTE slot anyways ? Are you talking about the USB
lanes on the M2 slot ?

Does anybody happen to have the FULL M2 spec available ?

According to the pinouts i've found, the reset line is dedicated to
PCIE, but perhaps we should treat it as "card reset" - IOW the whole
card resets, no matter which channels (sata, pcie, usb, ...) it is
actually using.

If that's the case, maybe we should reconsider the whole bus topology,
perhaps introduce an pseudo bus for representing the m2 connector,
which has the corresponding pcie/sata/usb/... ports as childs.
(I'm going to ask the corresponding bus maintainers for their oppinions)

> That
> said, I don't think the reset lines function on most iterations of boards, so possibly supporting
> those lines with GPIOs is redundant anyway...

My observation (on apu2/3/4) is that it seems to dependend on the
cards, eg. one baseband I've testing did the reset (sometimes even
needs on after poweron), another one just seemed to ignore it. Both
were running via USB.

No idea, whether that also depends on BIOS version ...

> I don't know if it's useful, but I uploaded a couple of scripts for beeping and flashing the leds.
> Here I just used globs to handle the different naming on the different boards (since I need to
> handle the older Alix boards as well). Enrico, is this useful to you?
> 
>     https://github.com/nippynetworks/gpio-utils

The swap_sim script illustrates a good point, that I resisted to think
about: sim swapping needs extra, baseband specific logic, to make it
actually work :(

So, I'm still unsure whether this stuff should go into a separate
portmux subsystem or an extended rfkill (where the basebands have to
be put into) ... both options still a lot of work to do.

> As an aside, these boards are super easy to flash as they support flashrom. 

Easy in the lab. But not in the field - if *anything* goes wrong,
technician needs to fly out - several k$ plus days of downtime,
until the technician reaches the place.

Let me rephrase this again: embedded devices, litterally in the field.

For kernel and userland, we already have good ways to handle redundancy
(automatic rescue boot in bootloader, serial console, wdt, ...), but
for the BIOS we don't have that.

> The generic bios is quite slow to startup and I would like to prepare 
> a customised version with shorter timeouts. Happy to work with
> you on something separately if this is interesting?

Actually, I already planned an actual factory setup for these boxes,
which includes the whole process, hw testing, bios deployment,
OS/application deployment, etc, etc. Unfortunately, this is lots of
work to do (has to be someting that arbitrary field technicians,
who aren't sw engineers, can actually do on their own).

Tried to get some sponsoring from pcengines, but they don't even seems
to be interested in an arranging simple things like LED names.
Damn, I'm really unhappy with these folks - we could have prevented much
of this whole mess if they would talked to me (they know that I'm the
apu-board kernel maintainer) :(



Let's try summarize the current state of knowledge:

* apu2..4:
  * BIOS support highly depends on BIOS version, we need to
    support older versions.
  * Field depends on on driver's naming, keycodes, ...
* apu5/6:
  * no known field-users that depend on the driver's naming scheme yet,
    (actually not supported yet) - we possibly could tolerate change
    in naming
  * assuming that all boards in the field already have recent enough
    BIOS that support LEDs and keys
  * suggestion: only implement the missing parts (eg. dont instantiate
    LEDs/Keys for them) - need to make sure there's no conflict with
    acpi-instantiated gpios, or grab these instead
* mpcie-reset:
  * no known field-users that depend on that - only has been used as
    debug tool and mapping to LEDs declared as temporary solution
    (planned to move it to reset controller subsystem or into pci
    subsystem)
  * therefore: userland API changes acceptable
  * open question:
    * do these lines actually belong to pcie or to the whole m2 card
      (which might not even use pcie, instead usb or sata) ?
    * shall we have a separate (pseudo)bus for m2, which controls
      these lines ?
    * do we need per-card special handlings (timing, ...) ?
* simsw:
  * similar to mpcie-reset (still experimental, ...) ...
  * seems to be really bound to M2 ports

Proposal:
  * introduce M2 (pseudo-)bus for modelling M2 ports + functionality
  * per-port reset lines (considered "card reset" instead of just
    pcie reset)
  * sim switch functionality
  * exposes switch/reset functionality via sysfs to userland
  * possibly publishes extra card information (if available)
  * linking to actual bus'es (currently active) on the card,
    which come from different subsystems (pci, usb, ...)


I'm going to write a small proposal for this new m2-port subsystem,
an post it @lkml.


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Ed W]

Hi


>> This is followed up by the patch I really want to try and get in, which is to add support for APU5
>> and APU6. Particularly APU5 is quite interesting to me and significantly different to previous
>> boards in that it has a lot more mpcie slots that can be used for LTE modules or wifi cards.
> Okay, why not just treating them differently ?
>
> Personally, I don't have an apu5, so can't tell much of them yet. But if
> they're so different, I can live with those having different names /
> keycode, assuming all this stuff is already supported by the lowest BIOS
> version they've ever been shipped with (remember: requirement of BIOS
> upgrade in the field is also not an option). Can you please check
> whether that's the case ?
>
> In that case we don't need any per bios-version special magic, just
> another entry in the DMI match table and separate board structs.


You are reaching the wrong conclusion I feel?

a) your proposal doesn't address that ACPI and the upstream board configure all this stuff already.
We are creating duplicate devices right now. Patch 1 removes the duplicate device. I don't see that
your proposal addresses the duplicate devices?

b) there is no reason for naming to be different between APU2-4 and 5. Lets standardise. I use the
boards somewhat interchangeably

c) If you check my patch, there is already a DMI match for APU5, so this can do whatever you want it
to, however, since it's green fields, I've set it up to match the way APU2-4 is working (in my patch)

d) Your driver already broke my userspace a year back when you changed these names. I've just
supported all/both versions of the LED naming conventions since then (it isn't hard). I do believe
we are overthinking this? It's really trivial to incorporate?



> For the mpcie reset lines we really should consider using the reset
> controller framework or add it to pci core (so pci_bus drivers also
> expose it to sysfs, in order to let userland trigger it). Tricky part
> is letting the board driver attach the reset functionality to the
> generic pci_bus driver. (grmpf, it wasn't ugly acpi, but dt instead,
> this would be pretty trivial via dt-overlay :o).


Knock yourself out. However, make sure that you read Pascal's email to you where he states that
these lines aren't actually wired up on many of the board revisions?


>> This
>> creates the realisation that the reset and sim-swap lines are always wired to the LTE slots, not to
>> the mpcie slots (although often they overlap in functionality), so naming is corrected here. 
> Sorry, but i'm confused. What's the actual difference between mpcie and
> LTE slots ? What is an LTE slot anyways ? Are you talking about the USB
> lanes on the M2 slot ?


The APU2-4 have

- 3x mpcie *shaped* slots

- On some boards there are two of these wired with the pcie signals

- On some boards there are two or three of these wired with USB signals

- On some boards there are mSATA signals to one of these slots

- On some boards there are either 1x or 2x SIM cards wired to some of the slots


What is common is that the reset lines (and rfkill) go to the same slots as the SIM lines.

So my suggestion is to name these lines based on "purpose", ie modem1 and modem2, which is 100%
consistent, rather than slot number, since that varies based on which board you pickup off the pile


The schematics are all publicly available. Although I think you may have to ask Pascal for the
APU5/6 ones. The rest are on the website


I'm confused about what you mean by M2 slot? There are no m.2 slots on the board? only mpcie, some
with USB signals, some with SIM?


> According to the pinouts i've found, the reset line is dedicated to
> PCIE, but perhaps we should treat it as "card reset" - IOW the whole
> card resets, no matter which channels (sata, pcie, usb, ...) it is
> actually using.


Please read Pascal's email to you. He disconnected these lines on many board revisions.


>> I don't know if it's useful, but I uploaded a couple of scripts for beeping and flashing the leds.
>> Here I just used globs to handle the different naming on the different boards (since I need to
>> handle the older Alix boards as well). Enrico, is this useful to you?
>>
>>     https://github.com/nippynetworks/gpio-utils
> The swap_sim script illustrates a good point, that I resisted to think
> about: sim swapping needs extra, baseband specific logic, to make it
> actually work :(


We just need the GPIOs exposing in some way. Nothing else. Everything else is done in userspace


> So, I'm still unsure whether this stuff should go into a separate
> portmux subsystem or an extended rfkill (where the basebands have to
> be put into) ... both options still a lot of work to do.


Lets please focus on things we can achieve in the near future in this thread


>> As an aside, these boards are super easy to flash as they support flashrom. 
> Easy in the lab. But not in the field - if *anything* goes wrong,
> technician needs to fly out - several k$ plus days of downtime,
> until the technician reaches the place.


Not disagreeing, but it seems about as reliable as doing a software upgrade. If you do those then
you are likely safe doing this



>> The generic bios is quite slow to startup and I would like to prepare 
>> a customised version with shorter timeouts. Happy to work with
>> you on something separately if this is interesting?
> Actually, I already planned an actual factory setup for these boxes,
> which includes the whole process, hw testing, bios deployment,
> OS/application deployment, etc, etc. Unfortunately, this is lots of
> work to do (has to be someting that arbitrary field technicians,
> who aren't sw engineers, can actually do on their own).


I've already got one

Hint: use netboot for simplest setup


> Tried to get some sponsoring from pcengines, but they don't even seems
> to be interested in an arranging simple things like LED names.
> Damn, I'm really unhappy with these folks - we could have prevented much
> of this whole mess if they would talked to me (they know that I'm the
> apu-board kernel maintainer) :(


I'm hiring. Contact me offlist if you want to talk further?



> Let's try summarize the current state of knowledge:
>
> * apu2..4:
>   * BIOS support highly depends on BIOS version, we need to
>     support older versions.
>   * Field depends on on driver's naming, keycodes, ...


Realistically that's only partially true:

- older kernels and older bios has "old names"

- kernels from a year back with your driver have a mix of "new" or "new + old" names depending on
whether the bios is newer than 4.10.

- After the patch I proposed: newer bios and newer kernel use ACPI, older bios match your driver,
people upgrading from kernels more than about a year old don't notice since they move from "old"
names to "acpi names" which are identical.



> * simsw:
>   * similar to mpcie-reset (still experimental, ...) ...
>   * seems to be really bound to M2 ports


Not experimental. Works reliably. Just don't toggle them during bootup (as your driver was doing in
the past - current driver doesn't do this anymore - problem resolved)

The GPIO lines are wired to a SIM swap chip which does all the work. The LTE cards don't know or see
what is happening


Regards

Ed W

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 22.10.20 17:10, Ed W wrote:

Hi,

> You are reaching the wrong conclusion I feel?
> 
> a) your proposal doesn't address that ACPI and the upstream board configure all this stuff already.

Only for those with recent enough firmware. As already mentioned, we
can't expect users to do BIOS upgrade in the *field* or have an
dependencies between BIOS and kernel versions.

> We are creating duplicate devices right now. 

Does that cause any actual failures ?

> b) there is no reason for naming to be different between APU2-4 and 5. Lets standardise. I use the
> boards somewhat interchangeably

I'm fine with (actually prefer) having the same naming also on apu5.
Actually, your argument is exactly why I did it this way for apu[234].

> c) If you check my patch, there is already a DMI match for APU5, so this can do whatever you want it
> to, however, since it's green fields, I've set it up to match the way APU2-4 is working (in my patch)

Feel free to repost the patch for adding apu5 only.
(too much traffic @lkml to keep everything)

> d) Your driver already broke my userspace a year back when you changed these names. I've just
> supported all/both versions of the LED naming conventions since then (it isn't hard). I do believe
> we are overthinking this? It's really trivial to incorporate?

Might be trivial for your case, lucky for you. My cases (and those I
just got distant reports from) aren't so trivial, most of this hasn't
much to do with actual coding (unfortunately, embedded world isn't as
agile as some might want it to be)

> Knock yourself out. However, make sure that you read Pascal's email to you where he states that
> these lines aren't actually wired up on many of the board revisions?

I know, but on some they are. The final implementation will take care
of that (at least if we can gather that information somewhere).

> The APU2-4 have
> 
> - 3x mpcie *shaped* slots
> 
> - On some boards there are two of these wired with the pcie signals
> - On some boards there are two or three of these wired with USB signals
> - On some boards there are mSATA signals to one of these slots
> - On some boards there are either 1x or 2x SIM cards wired to some of the slots

ACK, that's pretty much my information - just don't recall the exact
details for each one. (would have to dig into schematics again). I can
confirm that each one (of those I have/had) has at least one msata,
one pcie, one usb+sim - some have multiple functions.

But I still wonder what exactly you mean w/ "LTE-Port". Just an alias
for those having USB+SIM ? (sorry if I'm too nitpicking, but I've
learned that exact terminology often is very important - perhaps I've
been a lawyer in some former life ;-))

> What is common is that the reset lines (and rfkill) go to the same slots as the SIM lines.

Let's rephrase it: those w/ USB+SIM also have a reset line.

> So my suggestion is to name these lines based on "purpose", ie modem1 and modem2, which is 100%
> consistent, rather than slot number, since that varies based on which board you pickup off the pile

NAK. They still can serve several kinds of purpose, eg. those w/ USB or
PCIE *and* SIM are by far not limited for modem's (and I'm really glad
about that).

Does it make any sense having WLAN, SSD, DAC, etc in an "modem port" ?
I don't believe so.

> I'm confused about what you mean by M2 slot? There are no m.2 slots on the board? only mpcie, some
> with USB signals, some with SIM?

Sorry, I've just been confused, believing that NGFF (in its different
types) was an superset of mpcie - both can carry different signal types,
You're right, I've been mistaken on that.

I've just been looking for some adequate name for those kinds of ports
which may have pcie, usb, sata, etc, all on one connector. Suggestions
welcomed.

Now we have at least two types of such things now on the table:
mpcie/msata/usb (like apu*) and M.2. From embedded world, I know of
several (propritary) port types fitting into similar category.
PDMI and similar stuff (apple connectors, ...) probably, too

I'll have to rethink what's the optimal approach with that, probably
by a new subsystem (even though there's a little bit of overlap with
extconn). Yes, that's a whole topic on it's own - we went quite
offtopic.

> We just need the GPIOs exposing in some way. Nothing else. Everything else is done in userspace

Might be fine for you, but not for me. Exposing raw gpios isn't an
entirely good idea (except for rare cases where kernel really cannot
know what's the actual function of those IOs might be)

> Not disagreeing, but it seems about as reliable as doing a software upgrade. If you do those then
> you are likely safe doing this

Already mentioned, BIOS is very different from OS. We've got several
techniques for rescuing a machine from failed kernel (even remotely),
but no such thing for broken BIOS - that requires physical replacement
in the field.

> Hint: use netboot for simplest setup

Assuming there is another server in the LAN. We don't have this luxury.
We're already happy with an dialup land line or some serial link routed
out of some PLC. Remember: this is embedded, not office PC.

>> Let's try summarize the current state of knowledge:
>>
>> * apu2..4:
>>   * BIOS support highly depends on BIOS version, we need to
>>     support older versions.
>>   * Field depends on on driver's naming, keycodes, ...
> 
> 
> Realistically that's only partially true:
> 
> - older kernels and older bios has "old names"
> 
> - kernels from a year back with your driver have a mix of "new" or "new + old" names depending on
> whether the bios is newer than 4.10.

Yes, and you suggested was removing the LEDs entirely. This means more
radical changes, including loosing it all on older bios (which still is
heavily used in the field).

It certainly would be much simpler, if we could just run BIOS upgrades
everywhere,
but then the question would be: who's gonna pay the bill for that ? And
who's responsible if anything goes wrong ?

> - After the patch I proposed: newer bios and newer kernel use ACPI, older bios match your driver,
> people upgrading from kernels more than about a year old don't notice since they move from "old"
> names to "acpi names" which are identical.

Very most of those people will only notice something completely new
appearing - already mentioned: haven't found a single distro that had
that enabled, nobody in the apu community seem to even know about it.

Congratulations, you're the first person I've ever known who was
actually using it :p

>> * simsw:
>>   * similar to mpcie-reset (still experimental, ...) ...
>>   * seems to be really bound to M2 ports
> 
> Not experimental. Works reliably. 

I (maintainer) considered it experimental due to lack of testing
and being a bit unreliable.

> Just don't toggle them during bootup (as your driver was doing in
> the past - current driver doesn't do this anymore - problem resolved)

And thanks for that report.

> The GPIO lines are wired to a SIM swap chip which does all the work. 
> The LTE cards don't know or see what is happening

On which boards did you see that ?

For my boards (don't recall which combinations I've actually tested),
it seemed to work sometimes, but not very reliable. (probably because
of the bug you've mentioned).


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 14.10.20 10:41, Hans de Goede wrote:

Hi,

> Keep the current LED/gpio setup code, but make executing it conditional
> on the BIOS version and skip the LED/gpio setup when the new BIOS is
> present to avoid having duplicate LED entries, etc. in that case.
> 
> I guess this would still break userspace because if I understand things
> correctly the new ACPI based setup uses different LED names ? That
> seems unfortunate, but I guess that from the kernel pov we can just
> blame the BIOS for this, and since we definitely do not want duplicate
> LED entries for the same LED, this seems the least bad choice.

Sorry, but not fine. When a newer box is taken from storage into
production (eg. replacement or new installation), application breaks.
LED isn't the only problem, also affects buttons.

The whole reaons why I invested all the time for writing general
purpose drivers (fch-gpio is separate from board driver) and bringing
it to mainline was having clean and generic support for these boards,
instead of having to carry around special patch queues forever and
in near future just using stock distro kernel. I guess that's the
main reason for very most mainlined drivers. This will be defeated
as soon as the whole thing becomes board/bios specific again.

I purposely slept ofter the naming several times, to make sure it's
future proof and tells exactly what these leds are for. Otherwise I
could just have picked numbers and fruits.

Please don't change the names. Field relies on them.

Actually, I've already considered adding another workaround for removing
the ACPI gpio/leds. Haven't had the time for a close analysis the acpi
bytecode actually does, and what the kernel actually makes out of it.
As soon as I encounter an conflict (eg. locks the iomem from gpio-amd
fch, messes up gpio states, etc), I'll have to go that route. There
already have been bug reports in that direction (eg. simsw and reset
issues), which I could not yet reproduce (on the older bios versions).



--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Hans de Goede]

Hi,

On 10/19/20 5:44 PM, Enrico Weigelt, metux IT consult wrote:
> On 14.10.20 10:41, Hans de Goede wrote:
> 
> Hi,
> 
>> Keep the current LED/gpio setup code, but make executing it conditional
>> on the BIOS version and skip the LED/gpio setup when the new BIOS is
>> present to avoid having duplicate LED entries, etc. in that case.
>>
>> I guess this would still break userspace because if I understand things
>> correctly the new ACPI based setup uses different LED names ? That
>> seems unfortunate, but I guess that from the kernel pov we can just
>> blame the BIOS for this, and since we definitely do not want duplicate
>> LED entries for the same LED, this seems the least bad choice.
> 
> Sorry, but not fine. When a newer box is taken from storage into
> production (eg. replacement or new installation), application breaks.
> LED isn't the only problem, also affects buttons.
> 
> The whole reaons why I invested all the time for writing general
> purpose drivers (fch-gpio is separate from board driver) and bringing
> it to mainline was having clean and generic support for these boards,
> instead of having to carry around special patch queues forever and
> in near future just using stock distro kernel. I guess that's the
> main reason for very most mainlined drivers.

Ack and that is how things should be done.

> This will be defeated
> as soon as the whole thing becomes board/bios specific again.

I hear you, but if newer BIOS versions all of a sudden start
declaring their own stuff, then we need to come up with some
solution here...

Not sure what that solution should be though.

Regards,

Hans

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 19.10.20 20:37, Hans de Goede wrote:

Hi folks,

> I hear you, but if newer BIOS versions all of a sudden start
> declaring their own stuff, then we need to come up with some
> solution here...
> 
> Not sure what that solution should be though.

You're right. IMHO we should first get a clear picture, which problems
actually arise. (unfortunately, I can't easily flash firmware on my
DUTs for several reasons, eg. I dont actually own them :o).

If it's just about duplicate LEDs, we could either just ignore that
or add yet another  board specific quirks in the acpi probing for
ignoring the duplicate devices - depending on the existing kconfig
symbol.



--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@metux.net -- +49-151-27565287

[PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[Ed Wildgoose]

The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
will cause the kernel to automatically create led + gpio_key devices for
the platform. This means that the platform setup now creates duplicates
of all these led/key devices.

Driver conditionally initialises leds/keys only for older bios.

Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
---
 drivers/platform/x86/pcengines-apuv2.c | 115 +++++++++++++++++++------
 1 file changed, 90 insertions(+), 25 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index c37349f97..45f7a89de 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -128,6 +128,18 @@ static struct gpiod_lookup_table gpios_key_table = {
 
 /* Board setup */
 
+struct apu_driver_data {
+	const struct amd_fch_gpio_pdata *board_data;
+	const struct gpio_keys_platform_data *apu_keys_pdata;
+	const struct gpio_led_platform_data *apu_leds_pdata;
+};
+
+static struct apu_driver_data apu2_driver_data = {
+	.board_data = &board_apu2,
+	.apu_keys_pdata = &apu2_keys_pdata,
+	.apu_leds_pdata = &apu2_leds_pdata
+};
+
 /* Note: matching works on string prefix, so "apu2" must come before "apu" */
 static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 
@@ -138,7 +150,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
 	/* APU2 w/ legacy BIOS >= 4.0.8 */
 	{
@@ -147,7 +159,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
 	/* APU2 w/ mainline BIOS */
 	{
@@ -156,7 +168,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
 		},
-		.driver_data	= (void *)&board_apu2,
+		.driver_data	= (void *)&apu2_driver_data,
 	},
 
 	/* APU3 w/ legacy BIOS < 4.0.8 */
@@ -166,7 +178,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	/* APU3 w/ legacy BIOS >= 4.0.8 */
 	{
@@ -175,7 +187,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	/* APU3 w/ mainline BIOS */
 	{
@@ -184,7 +196,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	/* APU4 w/ legacy BIOS < 4.0.8 */
 	{
@@ -193,7 +205,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "APU4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	/* APU4 w/ legacy BIOS >= 4.0.8 */
 	{
@@ -202,7 +214,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "apu4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	/* APU4 w/ mainline BIOS */
 	{
@@ -211,11 +223,42 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
 			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu4")
 		},
-		.driver_data = (void *)&board_apu2,
+		.driver_data = (void *)&apu2_driver_data,
 	},
 	{}
 };
 
+int cmp_version(const char *base, const char *test)
+{
+	int i;
+	int oct_base[4], oct_test[4];
+	int matched_base, matched;
+
+	matched_base = sscanf(base, "v%4d.%4d.%4d.%4d",
+					&oct_base[0], &oct_base[1],
+					&oct_base[2], &oct_base[3]);
+	matched = sscanf(test, "v%4d.%4d.%4d.%4d",
+					&oct_test[0], &oct_test[1],
+					&oct_test[2], &oct_test[3]);
+
+	/* opinionated: match as lower if we can't parse test version */
+	if (matched == 0)
+		return -1;
+
+	if (matched_base < matched)
+		matched = matched_base;
+
+	for (i = 0; i < matched; i++) {
+		if (oct_test[i] > oct_base[i])
+			return 1;
+		else if (oct_test[i] < oct_base[i])
+			return -1;
+	}
+
+	return 0;
+}
+
+
 static struct platform_device *apu_gpio_pdev;
 static struct platform_device *apu_leds_pdev;
 static struct platform_device *apu_keys_pdev;
@@ -244,41 +287,63 @@ static struct platform_device * __init apu_create_pdev(
 static int __init apu_board_init(void)
 {
 	const struct dmi_system_id *id;
+	const struct apu_driver_data *driver_data;
+	char const *bios_ver;
+
+	apu_leds_pdev = apu_keys_pdev = NULL;
 
 	id = dmi_first_match(apu_gpio_dmi_table);
 	if (!id) {
 		pr_err("failed to detect APU board via DMI\n");
 		return -ENODEV;
+	} else {
+		pr_info("Detected APU board: %s - BIOS: %s\n",
+		dmi_get_system_info(DMI_BOARD_NAME),
+		dmi_get_system_info(DMI_BIOS_VERSION));
 	}
 
-	gpiod_add_lookup_table(&gpios_led_table);
-	gpiod_add_lookup_table(&gpios_key_table);
+	driver_data = id->driver_data;
+	bios_ver = dmi_get_system_info(DMI_BIOS_VERSION);
 
 	apu_gpio_pdev = apu_create_pdev(
 		AMD_FCH_GPIO_DRIVER_NAME,
-		id->driver_data,
+		driver_data->board_data,
 		sizeof(struct amd_fch_gpio_pdata));
 
-	apu_leds_pdev = apu_create_pdev(
-		"leds-gpio",
-		&apu2_leds_pdata,
-		sizeof(apu2_leds_pdata));
-
-	apu_keys_pdev = apu_create_pdev(
-		"gpio-keys-polled",
-		&apu2_keys_pdata,
-		sizeof(apu2_keys_pdata));
+	if (cmp_version("v4.10", bios_ver) < 0) {
+		/* Newer bios configure LEDs/keys via ACPI */
+
+		if (driver_data->apu_keys_pdata) {
+			gpiod_add_lookup_table(&gpios_key_table);
+			apu_keys_pdev = apu_create_pdev(
+				"gpio-keys-polled",
+				driver_data->apu_keys_pdata,
+				sizeof(struct gpio_keys_platform_data));
+		}
+
+		if (driver_data->apu_leds_pdata) {
+			gpiod_add_lookup_table(&gpios_led_table);
+			apu_leds_pdev = apu_create_pdev(
+				"leds-gpio",
+				driver_data->apu_leds_pdata,
+				sizeof(struct gpio_led_platform_data));
+		}
+	}
 
 	return 0;
 }
 
 static void __exit apu_board_exit(void)
 {
-	gpiod_remove_lookup_table(&gpios_led_table);
-	gpiod_remove_lookup_table(&gpios_key_table);
+	if (apu_keys_pdev) {
+		gpiod_remove_lookup_table(&gpios_key_table);
+		platform_device_unregister(apu_keys_pdev);
+	}
+	if (apu_leds_pdev) {
+		gpiod_remove_lookup_table(&gpios_led_table);
+		platform_device_unregister(apu_leds_pdev);
+	}
 
-	platform_device_unregister(apu_keys_pdev);
-	platform_device_unregister(apu_leds_pdev);
 	platform_device_unregister(apu_gpio_pdev);
 }
 
-- 
2.26.2

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

[Ed Wildgoose]

PCEngines make a number of SBC. APU5 has 5 mpcie slots + MSATA
It also has support for 3x LTE modems with 6x SIM slots (pairs with a
SIM switch device). Each mpcie slot for modems has a reset GPIO

To ensure that the naming is sane between APU2-6 the GPIOS are
renamed to be modem1-reset, modem2-reset, etc. This is significant
because the slots that can be reset change between APU2 and APU3/4

GPIO for simswap is moved to the end of the list as it could be dropped
for APU2 boards (but causes no harm to leave it in, hardware could be
added to a future rev of the board).

Structure of the GPIOs for APU5 is extremely similar to APU2-4, but
many lines are moved around and there are simply more
modems/resets/sim-swap lines to breakout.

Also added APU6, which is essentially APU4 with a different ethernet
interface

Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
---
 drivers/platform/x86/pcengines-apuv2.c | 123 +++++++++++++++++++++----
 1 file changed, 107 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index 45f7a89de..089756693 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
 /*
- * PC-Engines APUv2/APUv3 board platform driver
+ * PC-Engines APUv2-6 board platform driver
  * for GPIO buttons and LEDs
  *
  * Copyright (C) 2018 metux IT consult
@@ -22,38 +22,70 @@
 #include <linux/platform_data/gpio/gpio-amd-fch.h>
 
 /*
- * NOTE: this driver only supports APUv2/3 - not APUv1, as this one
+ * NOTE: this driver only supports APUv2-6 - not APUv1, as this one
  * has completely different register layouts.
  */
 
+/*
+ * There are a number of APU variants, with differing features
+ * APU2 has SIM slots 1/2 mapping to mPCIe sockets 1/2
+ * APU3/4 moved SIM slot 1 to mPCIe socket 3, ie logically reversed
+ * However, most APU3/4 have a SIM switch which we default on to reverse
+ * the order and keep physical SIM order matching physical modem order
+ * APU6 is approximately the same as APU4 with different ethernet layout
+ *
+ * APU5 has 3x SIM sockets, all with a SIM switch
+ * several GPIOs are shuffled (see schematic), including MODESW
+ */
+
 /* Register mappings */
 #define APU2_GPIO_REG_LED1		AMD_FCH_GPIO_REG_GPIO57
 #define APU2_GPIO_REG_LED2		AMD_FCH_GPIO_REG_GPIO58
 #define APU2_GPIO_REG_LED3		AMD_FCH_GPIO_REG_GPIO59_DEVSLP1
 #define APU2_GPIO_REG_MODESW		AMD_FCH_GPIO_REG_GPIO32_GE1
 #define APU2_GPIO_REG_SIMSWAP		AMD_FCH_GPIO_REG_GPIO33_GE2
-#define APU2_GPIO_REG_MPCIE2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
-#define APU2_GPIO_REG_MPCIE3		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU2_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+
+#define APU5_GPIO_REG_MODESW		AMT_FCH_GPIO_REG_GEVT22
+#define APU5_GPIO_REG_SIMSWAP1		AMD_FCH_GPIO_REG_GPIO68
+#define APU5_GPIO_REG_SIMSWAP2		AMD_FCH_GPIO_REG_GPIO32_GE1
+#define APU5_GPIO_REG_SIMSWAP3		AMD_FCH_GPIO_REG_GPIO33_GE2
+#define APU5_GPIO_REG_RESETM1		AMD_FCH_GPIO_REG_GPIO51
+#define APU5_GPIO_REG_RESETM2		AMD_FCH_GPIO_REG_GPIO55_DEVSLP0
+#define APU5_GPIO_REG_RESETM3		AMD_FCH_GPIO_REG_GPIO64
 
 /* Order in which the GPIO lines are defined in the register list */
 #define APU2_GPIO_LINE_LED1		0
 #define APU2_GPIO_LINE_LED2		1
 #define APU2_GPIO_LINE_LED3		2
 #define APU2_GPIO_LINE_MODESW		3
-#define APU2_GPIO_LINE_SIMSWAP		4
-#define APU2_GPIO_LINE_MPCIE2		5
-#define APU2_GPIO_LINE_MPCIE3		6
+#define APU2_GPIO_LINE_RESETM1		4
+#define APU2_GPIO_LINE_RESETM2		5
+#define APU2_GPIO_LINE_SIMSWAP		6
+
+#define APU5_GPIO_LINE_LED1		0
+#define APU5_GPIO_LINE_LED2		1
+#define APU5_GPIO_LINE_LED3		2
+#define APU5_GPIO_LINE_MODESW		3
+#define APU5_GPIO_LINE_RESETM1		4
+#define APU5_GPIO_LINE_RESETM2		5
+#define APU5_GPIO_LINE_RESETM3		6
+#define APU5_GPIO_LINE_SIMSWAP1		7
+#define APU5_GPIO_LINE_SIMSWAP2		8
+#define APU5_GPIO_LINE_SIMSWAP3		9
+
 
-/* GPIO device */
+/* GPIO device - APU2/3/4/6 */
 
 static int apu2_gpio_regs[] = {
 	[APU2_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
 	[APU2_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
 	[APU2_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
 	[APU2_GPIO_LINE_MODESW]		= APU2_GPIO_REG_MODESW,
+	[APU2_GPIO_LINE_RESETM1]	= APU2_GPIO_REG_RESETM1,
+	[APU2_GPIO_LINE_RESETM2]	= APU2_GPIO_REG_RESETM2,
 	[APU2_GPIO_LINE_SIMSWAP]	= APU2_GPIO_REG_SIMSWAP,
-	[APU2_GPIO_LINE_MPCIE2]		= APU2_GPIO_REG_MPCIE2,
-	[APU2_GPIO_LINE_MPCIE3]		= APU2_GPIO_REG_MPCIE3,
 };
 
 static const char * const apu2_gpio_names[] = {
@@ -61,9 +93,9 @@ static const char * const apu2_gpio_names[] = {
 	[APU2_GPIO_LINE_LED2]		= "front-led2",
 	[APU2_GPIO_LINE_LED3]		= "front-led3",
 	[APU2_GPIO_LINE_MODESW]		= "front-button",
+	[APU2_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU2_GPIO_LINE_RESETM2]	= "modem2-reset",
 	[APU2_GPIO_LINE_SIMSWAP]	= "simswap",
-	[APU2_GPIO_LINE_MPCIE2]		= "mpcie2_reset",
-	[APU2_GPIO_LINE_MPCIE3]		= "mpcie3_reset",
 };
 
 static const struct amd_fch_gpio_pdata board_apu2 = {
@@ -72,6 +104,41 @@ static const struct amd_fch_gpio_pdata board_apu2 = {
 	.gpio_names	= apu2_gpio_names,
 };
 
+/* GPIO device - APU5 */
+
+static int apu5_gpio_regs[] = {
+	[APU5_GPIO_LINE_LED1]		= APU2_GPIO_REG_LED1,
+	[APU5_GPIO_LINE_LED2]		= APU2_GPIO_REG_LED2,
+	[APU5_GPIO_LINE_LED3]		= APU2_GPIO_REG_LED3,
+	[APU5_GPIO_LINE_MODESW]		= APU5_GPIO_REG_MODESW,
+	[APU5_GPIO_LINE_RESETM1]	= APU5_GPIO_REG_RESETM1,
+	[APU5_GPIO_LINE_RESETM2]	= APU5_GPIO_REG_RESETM2,
+	[APU5_GPIO_LINE_RESETM3]	= APU5_GPIO_REG_RESETM3,
+	[APU5_GPIO_LINE_SIMSWAP1]	= APU5_GPIO_REG_SIMSWAP1,
+	[APU5_GPIO_LINE_SIMSWAP2]	= APU5_GPIO_REG_SIMSWAP2,
+	[APU5_GPIO_LINE_SIMSWAP3]	= APU5_GPIO_REG_SIMSWAP3,
+};
+
+static const char * const apu5_gpio_names[] = {
+	[APU5_GPIO_LINE_LED1]		= "front-led1",
+	[APU5_GPIO_LINE_LED2]		= "front-led2",
+	[APU5_GPIO_LINE_LED3]		= "front-led3",
+	[APU5_GPIO_LINE_MODESW]		= "front-button",
+	[APU5_GPIO_LINE_RESETM1]	= "modem1-reset",
+	[APU5_GPIO_LINE_RESETM2]	= "modem2-reset",
+	[APU5_GPIO_LINE_RESETM3]	= "modem3-reset",
+	[APU5_GPIO_LINE_SIMSWAP1]	= "simswap1",
+	[APU5_GPIO_LINE_SIMSWAP2]	= "simswap2",
+	[APU5_GPIO_LINE_SIMSWAP3]	= "simswap3",
+};
+
+static const struct amd_fch_gpio_pdata board_apu5 = {
+	.gpio_num	= ARRAY_SIZE(apu5_gpio_regs),
+	.gpio_reg	= apu5_gpio_regs,
+	.gpio_names	= apu5_gpio_names,
+};
+
+
 /* GPIO LEDs device */
 
 static const struct gpio_led apu2_leds[] = {
@@ -140,6 +207,12 @@ static struct apu_driver_data apu2_driver_data = {
 	.apu_leds_pdata = &apu2_leds_pdata
 };
 
+static struct apu_driver_data apu5_driver_data = {
+	.board_data = &board_apu5,
+	.apu_keys_pdata = NULL,
+	.apu_leds_pdata = NULL
+};
+
 /* Note: matching works on string prefix, so "apu2" must come before "apu" */
 static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 
@@ -225,6 +298,24 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 		},
 		.driver_data = (void *)&apu2_driver_data,
 	},
+	/* APU5 w/ mainline BIOS */
+	{
+		.ident		= "apu5",
+		.matches	= {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu5")
+		},
+		.driver_data	= (void *)&apu5_driver_data,
+	},
+	/* APU6 w/ mainline BIOS */
+	{
+		.ident		= "apu6",
+		.matches	= {
+			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
+			DMI_MATCH(DMI_BOARD_NAME, "apu6")
+		},
+		.driver_data	= (void *)&apu2_driver_data,
+	},
 	{}
 };
 
@@ -294,12 +385,12 @@ static int __init apu_board_init(void)
 
 	id = dmi_first_match(apu_gpio_dmi_table);
 	if (!id) {
-		pr_err("failed to detect APU board via DMI\n");
+		pr_err("No APU board detected via DMI\n");
 		return -ENODEV;
 	} else {
 		pr_info("Detected APU board: %s - BIOS: %s\n",
-		dmi_get_system_info(DMI_BOARD_NAME),
-		dmi_get_system_info(DMI_BIOS_VERSION));
+			dmi_get_system_info(DMI_BOARD_NAME),
+			dmi_get_system_info(DMI_BIOS_VERSION));
 	}
 
 	driver_data = id->driver_data;
@@ -351,7 +442,7 @@ module_init(apu_board_init);
 module_exit(apu_board_exit);
 
 MODULE_AUTHOR("Enrico Weigelt, metux IT consult <info@metux.net>");
-MODULE_DESCRIPTION("PC Engines APUv2/APUv3 board GPIO/LEDs/keys driver");
+MODULE_DESCRIPTION("PC Engines APUv2-6 board GPIO/LEDs/keys driver");
 MODULE_LICENSE("GPL");
 MODULE_DEVICE_TABLE(dmi, apu_gpio_dmi_table);
 MODULE_ALIAS("platform:pcengines-apuv2");
-- 
2.26.2

Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3209 bytes --]

Hi Ed,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/master]
[also build test WARNING on linus/master linux/master platform-drivers-x86/for-next v5.9 next-20201021]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ed-Wildgoose/x86-Conditional-init-of-pcengines-leds-keys-gpios/20201022-054433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2ee263eb855963c3ce8d04191c8a92e13e6096f2
config: x86_64-randconfig-a011-20201021 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2eac8ce820e6c9fe51bf93b55cb8a781b8b9fc7c)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/f8923b8d1bb4907e1138ee7b92f01a7f767e54b8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ed-Wildgoose/x86-Conditional-init-of-pcengines-leds-keys-gpios/20201022-054433
        git checkout f8923b8d1bb4907e1138ee7b92f01a7f767e54b8
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/platform/x86/pcengines-apuv2.c:231:5: warning: no previous prototype for function 'cmp_version' [-Wmissing-prototypes]
   int cmp_version(const char *base, const char *test)
       ^
   drivers/platform/x86/pcengines-apuv2.c:231:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int cmp_version(const char *base, const char *test)
   ^
   static 
   1 warning generated.

vim +/cmp_version +231 drivers/platform/x86/pcengines-apuv2.c

   230	
 > 231	int cmp_version(const char *base, const char *test)
   232	{
   233		int i;
   234		int oct_base[4], oct_test[4];
   235		int matched_base, matched;
   236	
   237		matched_base = sscanf(base, "v%4d.%4d.%4d.%4d",
   238						&oct_base[0], &oct_base[1],
   239						&oct_base[2], &oct_base[3]);
   240		matched = sscanf(test, "v%4d.%4d.%4d.%4d",
   241						&oct_test[0], &oct_test[1],
   242						&oct_test[2], &oct_test[3]);
   243	
   244		/* opinionated: match as lower if we can't parse test version */
   245		if (matched == 0)
   246			return -1;
   247	
   248		if (matched_base < matched)
   249			matched = matched_base;
   250	
   251		for (i = 0; i < matched; i++) {
   252			if (oct_test[i] > oct_base[i])
   253				return 1;
   254			else if (oct_test[i] < oct_base[i])
   255				return -1;
   256		}
   257	
   258		return 0;
   259	}
   260	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32463 bytes --]

Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[Enrico Weigelt, metux IT consult]

On 21.10.20 23:41, Ed Wildgoose wrote:

Hi,

> The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
> will cause the kernel to automatically create led + gpio_key devices for
> the platform. This means that the platform setup now creates duplicates
> of all these led/key devices.
> 
> Driver conditionally initialises leds/keys only for older bios.

still breaks existing userlands as device names change - LEDs as well as
keys, and keycodes also change.

IOW: userland needs to be depending on specific BIOS versions.


--mtx


> 
> Signed-off-by: Ed Wildgoose <lists@wildgooses.com>
> ---
>  drivers/platform/x86/pcengines-apuv2.c | 115 +++++++++++++++++++------
>  1 file changed, 90 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
> index c37349f97..45f7a89de 100644
> --- a/drivers/platform/x86/pcengines-apuv2.c
> +++ b/drivers/platform/x86/pcengines-apuv2.c
> @@ -128,6 +128,18 @@ static struct gpiod_lookup_table gpios_key_table = {
>  
>  /* Board setup */
>  
> +struct apu_driver_data {
> +	const struct amd_fch_gpio_pdata *board_data;
> +	const struct gpio_keys_platform_data *apu_keys_pdata;
> +	const struct gpio_led_platform_data *apu_leds_pdata;
> +};
> +
> +static struct apu_driver_data apu2_driver_data = {
> +	.board_data = &board_apu2,
> +	.apu_keys_pdata = &apu2_keys_pdata,
> +	.apu_leds_pdata = &apu2_leds_pdata
> +};
> +
>  /* Note: matching works on string prefix, so "apu2" must come before "apu" */
>  static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  
> @@ -138,7 +150,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  	/* APU2 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -147,7 +159,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  	/* APU2 w/ mainline BIOS */
>  	{
> @@ -156,7 +168,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu2")
>  		},
> -		.driver_data	= (void *)&board_apu2,
> +		.driver_data	= (void *)&apu2_driver_data,
>  	},
>  
>  	/* APU3 w/ legacy BIOS < 4.0.8 */
> @@ -166,7 +178,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU3 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -175,7 +187,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU3 w/ mainline BIOS */
>  	{
> @@ -184,7 +196,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu3")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ legacy BIOS < 4.0.8 */
>  	{
> @@ -193,7 +205,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "APU4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ legacy BIOS >= 4.0.8 */
>  	{
> @@ -202,7 +214,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "apu4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	/* APU4 w/ mainline BIOS */
>  	{
> @@ -211,11 +223,42 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "PC Engines"),
>  			DMI_MATCH(DMI_BOARD_NAME, "PC Engines apu4")
>  		},
> -		.driver_data = (void *)&board_apu2,
> +		.driver_data = (void *)&apu2_driver_data,
>  	},
>  	{}
>  };
>  
> +int cmp_version(const char *base, const char *test)
> +{
> +	int i;
> +	int oct_base[4], oct_test[4];
> +	int matched_base, matched;
> +
> +	matched_base = sscanf(base, "v%4d.%4d.%4d.%4d",
> +					&oct_base[0], &oct_base[1],
> +					&oct_base[2], &oct_base[3]);
> +	matched = sscanf(test, "v%4d.%4d.%4d.%4d",
> +					&oct_test[0], &oct_test[1],
> +					&oct_test[2], &oct_test[3]);
> +
> +	/* opinionated: match as lower if we can't parse test version */
> +	if (matched == 0)
> +		return -1;
> +
> +	if (matched_base < matched)
> +		matched = matched_base;
> +
> +	for (i = 0; i < matched; i++) {
> +		if (oct_test[i] > oct_base[i])
> +			return 1;
> +		else if (oct_test[i] < oct_base[i])
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +
>  static struct platform_device *apu_gpio_pdev;
>  static struct platform_device *apu_leds_pdev;
>  static struct platform_device *apu_keys_pdev;
> @@ -244,41 +287,63 @@ static struct platform_device * __init apu_create_pdev(
>  static int __init apu_board_init(void)
>  {
>  	const struct dmi_system_id *id;
> +	const struct apu_driver_data *driver_data;
> +	char const *bios_ver;
> +
> +	apu_leds_pdev = apu_keys_pdev = NULL;
>  
>  	id = dmi_first_match(apu_gpio_dmi_table);
>  	if (!id) {
>  		pr_err("failed to detect APU board via DMI\n");
>  		return -ENODEV;
> +	} else {
> +		pr_info("Detected APU board: %s - BIOS: %s\n",
> +		dmi_get_system_info(DMI_BOARD_NAME),
> +		dmi_get_system_info(DMI_BIOS_VERSION));
>  	}
>  
> -	gpiod_add_lookup_table(&gpios_led_table);
> -	gpiod_add_lookup_table(&gpios_key_table);
> +	driver_data = id->driver_data;
> +	bios_ver = dmi_get_system_info(DMI_BIOS_VERSION);
>  
>  	apu_gpio_pdev = apu_create_pdev(
>  		AMD_FCH_GPIO_DRIVER_NAME,
> -		id->driver_data,
> +		driver_data->board_data,
>  		sizeof(struct amd_fch_gpio_pdata));
>  
> -	apu_leds_pdev = apu_create_pdev(
> -		"leds-gpio",
> -		&apu2_leds_pdata,
> -		sizeof(apu2_leds_pdata));
> -
> -	apu_keys_pdev = apu_create_pdev(
> -		"gpio-keys-polled",
> -		&apu2_keys_pdata,
> -		sizeof(apu2_keys_pdata));
> +	if (cmp_version("v4.10", bios_ver) < 0) {
> +		/* Newer bios configure LEDs/keys via ACPI */
> +
> +		if (driver_data->apu_keys_pdata) {
> +			gpiod_add_lookup_table(&gpios_key_table);
> +			apu_keys_pdev = apu_create_pdev(
> +				"gpio-keys-polled",
> +				driver_data->apu_keys_pdata,
> +				sizeof(struct gpio_keys_platform_data));
> +		}
> +
> +		if (driver_data->apu_leds_pdata) {
> +			gpiod_add_lookup_table(&gpios_led_table);
> +			apu_leds_pdev = apu_create_pdev(
> +				"leds-gpio",
> +				driver_data->apu_leds_pdata,
> +				sizeof(struct gpio_led_platform_data));
> +		}
> +	}
>  
>  	return 0;
>  }
>  
>  static void __exit apu_board_exit(void)
>  {
> -	gpiod_remove_lookup_table(&gpios_led_table);
> -	gpiod_remove_lookup_table(&gpios_key_table);
> +	if (apu_keys_pdev) {
> +		gpiod_remove_lookup_table(&gpios_key_table);
> +		platform_device_unregister(apu_keys_pdev);
> +	}
> +	if (apu_leds_pdev) {
> +		gpiod_remove_lookup_table(&gpios_led_table);
> +		platform_device_unregister(apu_leds_pdev);
> +	}
>  
> -	platform_device_unregister(apu_keys_pdev);
> -	platform_device_unregister(apu_leds_pdev);
>  	platform_device_unregister(apu_gpio_pdev);
>  }
>  
> 

-- 
---
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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[Ed W]

On 22/10/2020 10:22, Enrico Weigelt, metux IT consult wrote:
> On 21.10.20 23:41, Ed Wildgoose wrote:
>
> Hi,
>
>> The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
>> will cause the kernel to automatically create led + gpio_key devices for
>> the platform. This means that the platform setup now creates duplicates
>> of all these led/key devices.
>>
>> Driver conditionally initialises leds/keys only for older bios.
> still breaks existing userlands as device names change - LEDs as well as
> keys, and keycodes also change.
>
> IOW: userland needs to be depending on specific BIOS versions.
>
>
> --mtx


As a compromise can you change your userland to cope with dynamic names? I see two simple ways:

1) udev rule to set name as you wish

2) I uploaded a little script which uses simple globs to just figure out the base name depending on
the board (also handles different board types entirely as well)


I realise you have some stuff running with this, I don't know your situation specifically, but this
feels like a really, really, small change to work around? Can you elaborate a little on why your
users will be updating kernels without updating user code? Is there really no way to stick a
compatibility udev rule in for your situation?

To recap though, the situation for many years was that the naming convention was board specific.
There is then just a small window (less than a year I think?) where users saw the name change to be
non board specific (ie your new module). I would hazard a guess that given the speed of mainstream
releases, few end users actually saw this change yet, or would notice? Those who did already
accommodated the name change, so I would hazard a guess they can cope with the revert? (or not even
notice?)


Look, just propose a solution, I don't really mind. To me this is SUCH a TRIVIAL rename that I've
already incorporated support for both naming conventions into my apps. I just want to get APU5/6
support in, which is affecting some real boards I want to use in volume. I don't feel the current
situation of duplicate devices should stay though.

My opinion is that we punt "this breaks userland" to being the board vendors problem now. The board
is configured largely through ACPI, so if the upstream changes the ACPI, then it's on them, not us.
This seems to be the direction the kernel is heading, with ACPI and device trees being used to
configure the boards, in preference to heavy platform drivers?


Hans, can you arbitrate here please? Your previous suggestion was to implement a compatibility shim
for older BIOS, that's done here. Happy to implement something else, just need a guide?

Thanks all

Ed W

Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[Enrico Weigelt, metux IT consult]

On 22.10.20 11:38, Ed W wrote:

Hi,

> As a compromise can you change your userland to cope with dynamic names? I see two simple ways:
> 
> 1) udev rule to set name as you wish

can you give an example of udev rule depending on bios version ?

> To recap though, the situation for many years was that the naming convention was board specific.

Yes, I perhaps should have changed them to something like
"frontpanel:...", but I left them just like they were for apu1.
Don't see any need to have them separate between board revisions.
(nobody ever complained about it, couldn't even find a user of this
old led-only driver on apu>=2 boards - everybody seemed to either ignore
it completely or use raw gpios via /dev/mem - maybe because the driver
did not support and even prevented using the key or other gpios).

BTW: Pcengines folks officially recommended that "userland driver"
via /dev/mem. And I couldn't find a single distro that shipped the
old leds-only driver.

Now pcengines folks came around with yet another different naming. No
idea why the had to put in the word "led" into led names. Oh, hell, we
ca be lucky their didn't use the schematics signal names ...

> There is then just a small window (less than a year I think?) where users saw the name change to be
> non board specific (ie your new module). I would hazard a guess that given the speed of mainstream
> releases, few end users actually saw this change yet, or would notice? Those who did already
> accommodated the name change, so I would hazard a guess they can cope with the revert? (or not even
> notice?)

It has been communicated and discussed in APU community (and directly
to pcengines, too) Most of the responses were happy to have a proper
driver now, the existance of the old driver wasn't actually well-known.

> I just want to get APU5/6
> support in, which is affecting some real boards I want to use in volume. I don't feel the current
> situation of duplicate devices should stay though.

As already mentioned, for APU5/6 I don't mind if LED name changes
(again, assuming all boards in the field already have recent enough fw).

BTW: I wonder why you're so keen on changing things for apu2..4, if
you're only interested on apu5/6.

> My opinion is that we punt "this breaks userland" to being the board vendors problem now. 
> The board is configured largely through ACPI, so if the upstream
> changes the ACPI, then it's on them, not us.

And what's the board vendor going to do about that ? Practically ?
They didn't even talk to me (known I'm the maintainer of the driver for
their boards) - sorry, I don't think they ever do anything.

In the end, responsibility for the whole operating system is ours, the
kernel maintainer's. It's always been that way - just have a look at the
thousands of board or device specific quirks all around the kernel. This
one of the major points where Linux is different from Windows.

Interface/protocol stability never has been a virtue of hw vendors,
even in heavily standardized fields like USB. (okay, I have to stop
myself from ranting against hw vendors, otherwise I'd have to write an
1k pages book about that :o)

We, the kernel maintainers, are the ones who get blamed if anything
breaks. Linux has a long history of stability and consistency (one of
the major reaons for picking Linux in professional fields, especially
for embedded). We put *a lot* of efforts into that, this takes up a
*huge* portion of kernel maintenance work.

> This seems to be the direction the kernel is heading, with ACPI and device trees being used to
> configure the boards, in preference to heavy platform drivers?

Device tree is very different from ACPI (not just technically).
We've got coordinated, contigious standardization processes here.
Not by corporate committees, but the folks on the basis, who're doing
the actual work and know the actual requirements and environments.
Most of the work is actually coming from the Linux kernel community.
And DT is designed to be easily customized (eg. rewriting in by the
bootloader, dynamic overlays, ...). Ever tried that w/ ACPI ?


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Conditional init of pcengines leds/keys gpios

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

Hi Ed,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/master]
[also build test WARNING on linus/master linux/master platform-drivers-x86/for-next v5.9 next-20201022]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Ed-Wildgoose/x86-Conditional-init-of-pcengines-leds-keys-gpios/20201022-054433
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 2ee263eb855963c3ce8d04191c8a92e13e6096f2
config: i386-randconfig-s002-20201022 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-17-g2d3af347-dirty
        # https://github.com/0day-ci/linux/commit/f8923b8d1bb4907e1138ee7b92f01a7f767e54b8
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Ed-Wildgoose/x86-Conditional-init-of-pcengines-leds-keys-gpios/20201022-054433
        git checkout f8923b8d1bb4907e1138ee7b92f01a7f767e54b8
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/platform/x86/pcengines-apuv2.c:231:5: sparse: sparse: symbol 'cmp_version' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36567 bytes --]

[RFC PATCH] x86: cmp_version() can be static

[kernel test robot]

Signed-off-by: kernel test robot <lkp@intel.com>
---
 pcengines-apuv2.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/x86/pcengines-apuv2.c b/drivers/platform/x86/pcengines-apuv2.c
index 45f7a89de2780c..2f579bf9917645 100644
--- a/drivers/platform/x86/pcengines-apuv2.c
+++ b/drivers/platform/x86/pcengines-apuv2.c
@@ -228,7 +228,7 @@ static const struct dmi_system_id apu_gpio_dmi_table[] __initconst = {
 	{}
 };
 
-int cmp_version(const char *base, const char *test)
+static int cmp_version(const char *base, const char *test)
 {
 	int i;
 	int oct_base[4], oct_test[4];

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 13.10.20 23:40, Ed W wrote:

> But why are users "in the field" 

"field" here means litterlly field. Far away from any human being.

> updating a kernel willy nilly without also updating the userland
> software that talks to it? 

Of course, we're always testing. Obviously, in the lab, not in the
field. And we don't wanna have to adapt existing, well tested, embedded
applications for dozens of BIOS versions, which might or might not
have certain functionality (it's not just for LEDs, but all the other
gpio-attached devices, eg. keys, mpcie reset, simsw, ...), etc.

> Why is the kernel upgrade trivial, but the fw upgrade is not an option?

Because technicians have to fly out to the installations and replace
the whole board (no, certainly no remote updates of the BIOS). The costs
per installation are a factor of the board price.

> Why not also update the app or setup a udev rule?

Again, BIOS version specific. And it's a not just a udev rule, it's
a lot of paper work in the application qualification.

This is an embedded device, not an cheap office pc.

> I would understand if we were talking something fairly major, 
> but it's the case of matching a
> filename that YOU changed from an old name to the current name and it's now changing back to the
> original name?

I did not change anything, I wrote a completely new driver with full
gpio support and attached devices.

pcengine folks ignored it for a long time, suddenly the started adding
incompatible stuff to their newer firmware.

> That's extremely disingenuous!!

No, its correct. The apuv1 board (more precisely its SoC) has a
completely different FCH. The old driver had some rudimentary support
just for the front leds, which actually worked properly on none of my
testing boards. I've did several surveys in the apu community -
everybody was using some userland program doing raw iomem access
(/dev/mem). Haven't found a single Distro that ever shipped that old
driver.

> It USED to work for the APU2-4 except that YOU removed support for APU2-4 from that module!!

Yes, I've proposed removing it, because I could not find a single person
who actually used it on apu2/3/4 boards. This might have to with the
fact that folks were happy that they now could use other gpio-connected
devices, too.

And, BTW, it did conflict with the new driver.

Note: the old driver is *only* for LEDs, not gpios as such, nor other
gpio-attached devices.

<skipping stuff that already had been answered>

> - Your LED based SIM toggle HAS already gone. So you have another example of userspace being broken
> right there. (Seems that this rule isn't so concrete?). 

Without my knowledge and ackknowledge as the maintainer !

> So you already need to (significantly?)
> adjust your userspace code - I'm not seeing how/why the LED change is such a blocker?

simsw isn't actively used in the field, the other gpio-consumers (leds,
keys, reset, ...) are used in the field. litterally field.

simsw was a quick shot on purpose, planned to be replaced by rfkill or
portmux. Both still experimental and nothing ready for mainline yet.


--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@metux.net -- +49-151-27565287

Re: [PATCH 1/2] x86: Remove led/gpio setup from pcengines platform driver

[Enrico Weigelt, metux IT consult]

On 21.09.20 23:59, Ed Wildgoose wrote:
> The pcengines bios/firmware includes ACPI tables (since 4.10.0.1) which
> will cause the kernel to automatically create led + gpio_key devices for
> the platform. This means that the platform setup now creates duplicates
> of all these led/key devices.

Absolutely not. This breaks a high number of machines in the field,
where FW upgrade is not an option.

> Anyone with a much older bios can use the 'leds-apu' driver to get the
> same set of led devices as created by the kernel with a modern bios.

No, this ancient and obsolete driver does not support more recent boards
and lacks lots of other important functionality, eg. keys, where
userland relies on. Never break userland.

--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@metux.net -- +49-151-27565287