* [PATCH] platform/x86: sel3350-platform: Retain LED state on load and unload
@ 2026-04-08 21:18 Brodie Abrew
2026-04-09 12:40 ` Ilpo Järvinen
0 siblings, 1 reply; 4+ messages in thread
From: Brodie Abrew @ 2026-04-08 21:18 UTC (permalink / raw)
To: platform-driver-x86; +Cc: robert.joslyn, Brodie Abrew
When the platform driver is loaded or unloaded, it overwrites the
existing LED states. This can cause a loss of early boot state when the
driver loads, andi t can cause the ALARM contact to change state or
flicker.
Explicitly retain the existing LED state to prevent overwriting on
driver load and unload.
Signed-off-by: Brodie Abrew <brodie_abrew@selinc.com>
---
drivers/platform/x86/sel3350-platform.c | 128 ++++++++++++++++++------
1 file changed, 95 insertions(+), 33 deletions(-)
diff --git a/drivers/platform/x86/sel3350-platform.c b/drivers/platform/x86/sel3350-platform.c
index 02e2081e2333..08f40268891f 100644
--- a/drivers/platform/x86/sel3350-platform.c
+++ b/drivers/platform/x86/sel3350-platform.c
@@ -30,19 +30,72 @@
#define SEL_PS_B_DETECT "sel_ps_b_detect"
#define SEL_PS_B_GOOD "sel_ps_b_good"
+#define AUX_LED_GRN1 "sel_aux_led_grn1"
+#define AUX_LED_GRN2 "sel_aux_led_grn2"
+#define AUX_LED_GRN3 "sel_aux_led_grn3"
+#define AUX_LED_GRN4 "sel_aux_led_grn4"
+#define ALARM_STATE_USER "sel_alarm_state_user"
+#define ENABLE_STATE_USER "sel_enable_state_user"
+#define AUX_LED_RED1 "sel_aux_led_red1"
+#define AUX_LED_RED2 "sel_aux_led_red2"
+#define AUX_LED_RED3 "sel_aux_led_red3"
+#define AUX_LED_RED4 "sel_aux_led_red4"
+
+static const char *const sel3350_leds_gpio_names[] = {
+ AUX_LED_GRN1,
+ AUX_LED_GRN2,
+ AUX_LED_GRN3,
+ AUX_LED_GRN4,
+ ALARM_STATE_USER,
+ ENABLE_STATE_USER,
+ AUX_LED_RED1,
+ AUX_LED_RED2,
+ AUX_LED_RED3,
+ AUX_LED_RED4,
+};
+
/* LEDs */
-static const struct gpio_led sel3350_leds[] = {
- { .name = "sel:green:aux1" },
- { .name = "sel:green:aux2" },
- { .name = "sel:green:aux3" },
- { .name = "sel:green:aux4" },
- { .name = "sel:red:alarm" },
+static struct gpio_led sel3350_leds[] = {
+ { .name = "sel:green:aux1",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:green:aux2",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:green:aux3",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:green:aux4",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:red:alarm",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
{ .name = "sel:green:enabled",
- .default_state = LEDS_GPIO_DEFSTATE_ON },
- { .name = "sel:red:aux1" },
- { .name = "sel:red:aux2" },
- { .name = "sel:red:aux3" },
- { .name = "sel:red:aux4" },
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:red:aux1",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:red:aux2",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:red:aux3",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
+ { .name = "sel:red:aux4",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1 },
};
static const struct gpio_led_platform_data sel3350_leds_pdata = {
@@ -50,25 +103,6 @@ static const struct gpio_led_platform_data sel3350_leds_pdata = {
.leds = sel3350_leds,
};
-/* Map GPIOs to LEDs */
-static struct gpiod_lookup_table sel3350_leds_table = {
- .dev_id = "leds-gpio",
- .table = {
- GPIO_LOOKUP_IDX(BXT_NW, 49, NULL, 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 50, NULL, 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 51, NULL, 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 52, NULL, 3, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_W, 20, NULL, 4, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_W, 21, NULL, 5, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 37, NULL, 6, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 38, NULL, 7, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 39, NULL, 8, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 40, NULL, 9, GPIO_ACTIVE_HIGH),
- {},
- }
-};
-
-/* Map GPIOs to power supplies */
static struct gpiod_lookup_table sel3350_gpios_table = {
.dev_id = B2093_GPIO_ACPI_ID ":00",
.table = {
@@ -76,6 +110,16 @@ static struct gpiod_lookup_table sel3350_gpios_table = {
GPIO_LOOKUP(BXT_NW, 45, SEL_PS_A_GOOD, GPIO_ACTIVE_LOW),
GPIO_LOOKUP(BXT_NW, 46, SEL_PS_B_DETECT, GPIO_ACTIVE_LOW),
GPIO_LOOKUP(BXT_NW, 47, SEL_PS_B_GOOD, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP(BXT_NW, 49, AUX_LED_GRN1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 50, AUX_LED_GRN2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 51, AUX_LED_GRN3, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 52, AUX_LED_GRN4, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_W, 20, ALARM_STATE_USER, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_W, 21, ENABLE_STATE_USER, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 37, AUX_LED_RED1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 38, AUX_LED_RED2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 39, AUX_LED_RED3, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 40, AUX_LED_RED4, GPIO_ACTIVE_HIGH),
{},
}
};
@@ -149,6 +193,7 @@ struct sel3350_data {
static int sel3350_probe(struct platform_device *pdev)
{
int rs;
+ int i;
struct sel3350_data *sel3350;
struct power_supply_config ps_cfg = {};
@@ -158,9 +203,19 @@ static int sel3350_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sel3350);
- gpiod_add_lookup_table(&sel3350_leds_table);
gpiod_add_lookup_table(&sel3350_gpios_table);
+ for (i = 0; i < ARRAY_SIZE(sel3350_leds); ++i) {
+ sel3350_leds[i].gpiod = devm_gpiod_get(
+ &pdev->dev, sel3350_leds_gpio_names[i], GPIOD_ASIS);
+ if (IS_ERR(sel3350_leds[i].gpiod) ||
+ (sel3350_leds[i].gpiod == NULL)) {
+ rs = -EPROBE_DEFER;
+ goto err_gpio_loop;
+ }
+ gpiod_set_consumer_name(sel3350_leds[i].gpiod, sel3350_leds[i].name);
+ }
+
sel3350->leds_pdev = platform_device_register_data(
NULL,
"leds-gpio",
@@ -209,11 +264,18 @@ static int sel3350_probe(struct platform_device *pdev)
return 0;
+err_gpio_loop:
+ while (i > 0) {
+ /* Don't clean up the failed get but do clean up all others */
+ i--;
+ devm_gpiod_put(&pdev->dev, sel3350_leds[i].gpiod);
+ }
+ goto err_platform;
+
err_ps:
platform_device_unregister(sel3350->leds_pdev);
err_platform:
gpiod_remove_lookup_table(&sel3350_gpios_table);
- gpiod_remove_lookup_table(&sel3350_leds_table);
return rs;
}
@@ -224,7 +286,6 @@ static void sel3350_remove(struct platform_device *pdev)
platform_device_unregister(sel3350->leds_pdev);
gpiod_remove_lookup_table(&sel3350_gpios_table);
- gpiod_remove_lookup_table(&sel3350_leds_table);
}
static const struct acpi_device_id sel3350_device_ids[] = {
@@ -246,4 +307,5 @@ module_platform_driver(sel3350_platform_driver);
MODULE_AUTHOR("Schweitzer Engineering Laboratories");
MODULE_DESCRIPTION("SEL-3350 platform driver");
MODULE_LICENSE("Dual BSD/GPL");
+MODULE_ALIAS("platform:sel3350");
MODULE_SOFTDEP("pre: pinctrl_broxton leds-gpio");
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86: sel3350-platform: Retain LED state on load and unload
2026-04-08 21:18 [PATCH] platform/x86: sel3350-platform: Retain LED state on load and unload Brodie Abrew
@ 2026-04-09 12:40 ` Ilpo Järvinen
2026-04-09 21:27 ` [PATCH v2] " Brodie Abrew
0 siblings, 1 reply; 4+ messages in thread
From: Ilpo Järvinen @ 2026-04-09 12:40 UTC (permalink / raw)
To: Brodie Abrew; +Cc: platform-driver-x86, robert.joslyn
On Wed, 8 Apr 2026, Brodie Abrew wrote:
> When the platform driver is loaded or unloaded, it overwrites the
> existing LED states. This can cause a loss of early boot state when the
> driver loads, andi t can cause the ALARM contact to change state or
andi t -> and it
> flicker.
>
> Explicitly retain the existing LED state to prevent overwriting on
> driver load and unload.
>
> Signed-off-by: Brodie Abrew <brodie_abrew@selinc.com>
> ---
> drivers/platform/x86/sel3350-platform.c | 128 ++++++++++++++++++------
> 1 file changed, 95 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/platform/x86/sel3350-platform.c b/drivers/platform/x86/sel3350-platform.c
> index 02e2081e2333..08f40268891f 100644
> --- a/drivers/platform/x86/sel3350-platform.c
> +++ b/drivers/platform/x86/sel3350-platform.c
> @@ -30,19 +30,72 @@
> #define SEL_PS_B_DETECT "sel_ps_b_detect"
> #define SEL_PS_B_GOOD "sel_ps_b_good"
>
> +#define AUX_LED_GRN1 "sel_aux_led_grn1"
> +#define AUX_LED_GRN2 "sel_aux_led_grn2"
> +#define AUX_LED_GRN3 "sel_aux_led_grn3"
> +#define AUX_LED_GRN4 "sel_aux_led_grn4"
> +#define ALARM_STATE_USER "sel_alarm_state_user"
> +#define ENABLE_STATE_USER "sel_enable_state_user"
> +#define AUX_LED_RED1 "sel_aux_led_red1"
> +#define AUX_LED_RED2 "sel_aux_led_red2"
> +#define AUX_LED_RED3 "sel_aux_led_red3"
> +#define AUX_LED_RED4 "sel_aux_led_red4"
> +
> +static const char *const sel3350_leds_gpio_names[] = {
> + AUX_LED_GRN1,
> + AUX_LED_GRN2,
> + AUX_LED_GRN3,
> + AUX_LED_GRN4,
> + ALARM_STATE_USER,
> + ENABLE_STATE_USER,
> + AUX_LED_RED1,
> + AUX_LED_RED2,
> + AUX_LED_RED3,
> + AUX_LED_RED4,
> +};
> +
> /* LEDs */
> -static const struct gpio_led sel3350_leds[] = {
> - { .name = "sel:green:aux1" },
> - { .name = "sel:green:aux2" },
> - { .name = "sel:green:aux3" },
> - { .name = "sel:green:aux4" },
> - { .name = "sel:red:alarm" },
> +static struct gpio_led sel3350_leds[] = {
> + { .name = "sel:green:aux1",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:green:aux2",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:green:aux3",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:green:aux4",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:red:alarm",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> { .name = "sel:green:enabled",
> - .default_state = LEDS_GPIO_DEFSTATE_ON },
> - { .name = "sel:red:aux1" },
> - { .name = "sel:red:aux2" },
> - { .name = "sel:red:aux3" },
> - { .name = "sel:red:aux4" },
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:red:aux1",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:red:aux2",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:red:aux3",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
> + { .name = "sel:red:aux4",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1 },
The closing braces should be on own line, and the non-terminating members
should have comma in the end.
> };
>
> static const struct gpio_led_platform_data sel3350_leds_pdata = {
> @@ -50,25 +103,6 @@ static const struct gpio_led_platform_data sel3350_leds_pdata = {
> .leds = sel3350_leds,
> };
>
> -/* Map GPIOs to LEDs */
> -static struct gpiod_lookup_table sel3350_leds_table = {
> - .dev_id = "leds-gpio",
> - .table = {
> - GPIO_LOOKUP_IDX(BXT_NW, 49, NULL, 0, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 50, NULL, 1, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 51, NULL, 2, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 52, NULL, 3, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_W, 20, NULL, 4, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_W, 21, NULL, 5, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 37, NULL, 6, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 38, NULL, 7, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 39, NULL, 8, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 40, NULL, 9, GPIO_ACTIVE_HIGH),
> - {},
> - }
> -};
> -
> -/* Map GPIOs to power supplies */
> static struct gpiod_lookup_table sel3350_gpios_table = {
> .dev_id = B2093_GPIO_ACPI_ID ":00",
> .table = {
> @@ -76,6 +110,16 @@ static struct gpiod_lookup_table sel3350_gpios_table = {
> GPIO_LOOKUP(BXT_NW, 45, SEL_PS_A_GOOD, GPIO_ACTIVE_LOW),
> GPIO_LOOKUP(BXT_NW, 46, SEL_PS_B_DETECT, GPIO_ACTIVE_LOW),
> GPIO_LOOKUP(BXT_NW, 47, SEL_PS_B_GOOD, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP(BXT_NW, 49, AUX_LED_GRN1, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 50, AUX_LED_GRN2, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 51, AUX_LED_GRN3, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 52, AUX_LED_GRN4, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_W, 20, ALARM_STATE_USER, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_W, 21, ENABLE_STATE_USER, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 37, AUX_LED_RED1, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 38, AUX_LED_RED2, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 39, AUX_LED_RED3, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 40, AUX_LED_RED4, GPIO_ACTIVE_HIGH),
> {},
> }
> };
> @@ -149,6 +193,7 @@ struct sel3350_data {
> static int sel3350_probe(struct platform_device *pdev)
> {
> int rs;
> + int i;
> struct sel3350_data *sel3350;
> struct power_supply_config ps_cfg = {};
>
> @@ -158,9 +203,19 @@ static int sel3350_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, sel3350);
>
> - gpiod_add_lookup_table(&sel3350_leds_table);
> gpiod_add_lookup_table(&sel3350_gpios_table);
>
> + for (i = 0; i < ARRAY_SIZE(sel3350_leds); ++i) {
Add include for ARRAY_SIZE()
> + sel3350_leds[i].gpiod = devm_gpiod_get(
> + &pdev->dev, sel3350_leds_gpio_names[i], GPIOD_ASIS);
Align parameters to (, at least the first one easily fits to the first
line.
> + if (IS_ERR(sel3350_leds[i].gpiod) ||
Add include.
> + (sel3350_leds[i].gpiod == NULL)) {
This is misaligned.
> + rs = -EPROBE_DEFER;
> + goto err_gpio_loop;
> + }
> + gpiod_set_consumer_name(sel3350_leds[i].gpiod, sel3350_leds[i].name);
> + }
> +
> sel3350->leds_pdev = platform_device_register_data(
> NULL,
> "leds-gpio",
> @@ -209,11 +264,18 @@ static int sel3350_probe(struct platform_device *pdev)
>
> return 0;
>
> +err_gpio_loop:
> + while (i > 0) {
> + /* Don't clean up the failed get but do clean up all others */
> + i--;
There's more concise form for this which is used elsewhere for similar
loop rollbacks:
while (i--)
You can leave the comment out.
> + devm_gpiod_put(&pdev->dev, sel3350_leds[i].gpiod);
> + }
> + goto err_platform;
> +
> err_ps:
> platform_device_unregister(sel3350->leds_pdev);
> err_platform:
> gpiod_remove_lookup_table(&sel3350_gpios_table);
> - gpiod_remove_lookup_table(&sel3350_leds_table);
>
> return rs;
> }
> @@ -224,7 +286,6 @@ static void sel3350_remove(struct platform_device *pdev)
>
> platform_device_unregister(sel3350->leds_pdev);
> gpiod_remove_lookup_table(&sel3350_gpios_table);
> - gpiod_remove_lookup_table(&sel3350_leds_table);
> }
>
> static const struct acpi_device_id sel3350_device_ids[] = {
> @@ -246,4 +307,5 @@ module_platform_driver(sel3350_platform_driver);
> MODULE_AUTHOR("Schweitzer Engineering Laboratories");
> MODULE_DESCRIPTION("SEL-3350 platform driver");
> MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("platform:sel3350");
> MODULE_SOFTDEP("pre: pinctrl_broxton leds-gpio");
>
--
i.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] platform/x86: sel3350-platform: Retain LED state on load and unload
2026-04-09 12:40 ` Ilpo Järvinen
@ 2026-04-09 21:27 ` Brodie Abrew
2026-04-10 4:38 ` Robert Joslyn
0 siblings, 1 reply; 4+ messages in thread
From: Brodie Abrew @ 2026-04-09 21:27 UTC (permalink / raw)
To: ilpo.jarvinen; +Cc: brodie_abrew, platform-driver-x86, robert.joslyn
When the platform driver is loaded or unloaded, it overwrites the
existing LED states. This can cause a loss of early boot state when the
driver loads, and it can cause the ALARM contact to change state or
flicker.
Explicitly retain the existing LED state to prevent overwriting on
driver load and unload.
Signed-off-by: Brodie Abrew <brodie_abrew@selinc.com>
---
V1 -> V2: Fixed code style, added includes, and fixed other reviewer comments
drivers/platform/x86/sel3350-platform.c | 138 ++++++++++++++++++------
1 file changed, 105 insertions(+), 33 deletions(-)
diff --git a/drivers/platform/x86/sel3350-platform.c b/drivers/platform/x86/sel3350-platform.c
index 02e2081e2333..7b0dc1d11199 100644
--- a/drivers/platform/x86/sel3350-platform.c
+++ b/drivers/platform/x86/sel3350-platform.c
@@ -9,6 +9,8 @@
*/
#include <linux/acpi.h>
+#include <linux/array_size.h>
+#include <linux/err.h>
#include <linux/gpio/consumer.h>
#include <linux/gpio/machine.h>
#include <linux/leds.h>
@@ -30,19 +32,82 @@
#define SEL_PS_B_DETECT "sel_ps_b_detect"
#define SEL_PS_B_GOOD "sel_ps_b_good"
+#define AUX_LED_GRN1 "sel_aux_led_grn1"
+#define AUX_LED_GRN2 "sel_aux_led_grn2"
+#define AUX_LED_GRN3 "sel_aux_led_grn3"
+#define AUX_LED_GRN4 "sel_aux_led_grn4"
+#define ALARM_STATE_USER "sel_alarm_state_user"
+#define ENABLE_STATE_USER "sel_enable_state_user"
+#define AUX_LED_RED1 "sel_aux_led_red1"
+#define AUX_LED_RED2 "sel_aux_led_red2"
+#define AUX_LED_RED3 "sel_aux_led_red3"
+#define AUX_LED_RED4 "sel_aux_led_red4"
+
+static const char *const sel3350_leds_gpio_names[] = {
+ AUX_LED_GRN1,
+ AUX_LED_GRN2,
+ AUX_LED_GRN3,
+ AUX_LED_GRN4,
+ ALARM_STATE_USER,
+ ENABLE_STATE_USER,
+ AUX_LED_RED1,
+ AUX_LED_RED2,
+ AUX_LED_RED3,
+ AUX_LED_RED4,
+};
+
/* LEDs */
-static const struct gpio_led sel3350_leds[] = {
- { .name = "sel:green:aux1" },
- { .name = "sel:green:aux2" },
- { .name = "sel:green:aux3" },
- { .name = "sel:green:aux4" },
- { .name = "sel:red:alarm" },
+static struct gpio_led sel3350_leds[] = {
+ { .name = "sel:green:aux1",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:green:aux2",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:green:aux3",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:green:aux4",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:red:alarm",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
{ .name = "sel:green:enabled",
- .default_state = LEDS_GPIO_DEFSTATE_ON },
- { .name = "sel:red:aux1" },
- { .name = "sel:red:aux2" },
- { .name = "sel:red:aux3" },
- { .name = "sel:red:aux4" },
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:red:aux1",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:red:aux2",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:red:aux3",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
+ { .name = "sel:red:aux4",
+ .default_state = LEDS_GPIO_DEFSTATE_KEEP,
+ .retain_state_suspended = 1,
+ .retain_state_shutdown = 1,
+ },
};
static const struct gpio_led_platform_data sel3350_leds_pdata = {
@@ -50,25 +115,6 @@ static const struct gpio_led_platform_data sel3350_leds_pdata = {
.leds = sel3350_leds,
};
-/* Map GPIOs to LEDs */
-static struct gpiod_lookup_table sel3350_leds_table = {
- .dev_id = "leds-gpio",
- .table = {
- GPIO_LOOKUP_IDX(BXT_NW, 49, NULL, 0, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 50, NULL, 1, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 51, NULL, 2, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_NW, 52, NULL, 3, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_W, 20, NULL, 4, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_W, 21, NULL, 5, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 37, NULL, 6, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 38, NULL, 7, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 39, NULL, 8, GPIO_ACTIVE_HIGH),
- GPIO_LOOKUP_IDX(BXT_SW, 40, NULL, 9, GPIO_ACTIVE_HIGH),
- {},
- }
-};
-
-/* Map GPIOs to power supplies */
static struct gpiod_lookup_table sel3350_gpios_table = {
.dev_id = B2093_GPIO_ACPI_ID ":00",
.table = {
@@ -76,6 +122,16 @@ static struct gpiod_lookup_table sel3350_gpios_table = {
GPIO_LOOKUP(BXT_NW, 45, SEL_PS_A_GOOD, GPIO_ACTIVE_LOW),
GPIO_LOOKUP(BXT_NW, 46, SEL_PS_B_DETECT, GPIO_ACTIVE_LOW),
GPIO_LOOKUP(BXT_NW, 47, SEL_PS_B_GOOD, GPIO_ACTIVE_LOW),
+ GPIO_LOOKUP(BXT_NW, 49, AUX_LED_GRN1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 50, AUX_LED_GRN2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 51, AUX_LED_GRN3, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_NW, 52, AUX_LED_GRN4, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_W, 20, ALARM_STATE_USER, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_W, 21, ENABLE_STATE_USER, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 37, AUX_LED_RED1, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 38, AUX_LED_RED2, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 39, AUX_LED_RED3, GPIO_ACTIVE_HIGH),
+ GPIO_LOOKUP(BXT_SW, 40, AUX_LED_RED4, GPIO_ACTIVE_HIGH),
{},
}
};
@@ -149,6 +205,7 @@ struct sel3350_data {
static int sel3350_probe(struct platform_device *pdev)
{
int rs;
+ int i;
struct sel3350_data *sel3350;
struct power_supply_config ps_cfg = {};
@@ -158,9 +215,20 @@ static int sel3350_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, sel3350);
- gpiod_add_lookup_table(&sel3350_leds_table);
gpiod_add_lookup_table(&sel3350_gpios_table);
+ for (i = 0; i < ARRAY_SIZE(sel3350_leds); ++i) {
+ sel3350_leds[i].gpiod = devm_gpiod_get(&pdev->dev,
+ sel3350_leds_gpio_names[i],
+ GPIOD_ASIS);
+ if (IS_ERR(sel3350_leds[i].gpiod) ||
+ (sel3350_leds[i].gpiod == NULL)) {
+ rs = -EPROBE_DEFER;
+ goto err_gpio_loop;
+ }
+ gpiod_set_consumer_name(sel3350_leds[i].gpiod, sel3350_leds[i].name);
+ }
+
sel3350->leds_pdev = platform_device_register_data(
NULL,
"leds-gpio",
@@ -209,11 +277,15 @@ static int sel3350_probe(struct platform_device *pdev)
return 0;
+err_gpio_loop:
+ while (i--)
+ devm_gpiod_put(&pdev->dev, sel3350_leds[i].gpiod);
+ goto err_platform;
+
err_ps:
platform_device_unregister(sel3350->leds_pdev);
err_platform:
gpiod_remove_lookup_table(&sel3350_gpios_table);
- gpiod_remove_lookup_table(&sel3350_leds_table);
return rs;
}
@@ -224,7 +296,6 @@ static void sel3350_remove(struct platform_device *pdev)
platform_device_unregister(sel3350->leds_pdev);
gpiod_remove_lookup_table(&sel3350_gpios_table);
- gpiod_remove_lookup_table(&sel3350_leds_table);
}
static const struct acpi_device_id sel3350_device_ids[] = {
@@ -246,4 +317,5 @@ module_platform_driver(sel3350_platform_driver);
MODULE_AUTHOR("Schweitzer Engineering Laboratories");
MODULE_DESCRIPTION("SEL-3350 platform driver");
MODULE_LICENSE("Dual BSD/GPL");
+MODULE_ALIAS("platform:sel3350");
MODULE_SOFTDEP("pre: pinctrl_broxton leds-gpio");
--
2.43.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] platform/x86: sel3350-platform: Retain LED state on load and unload
2026-04-09 21:27 ` [PATCH v2] " Brodie Abrew
@ 2026-04-10 4:38 ` Robert Joslyn
0 siblings, 0 replies; 4+ messages in thread
From: Robert Joslyn @ 2026-04-10 4:38 UTC (permalink / raw)
To: Brodie Abrew, ilpo.jarvinen; +Cc: platform-driver-x86
On 4/9/26 14:27, Brodie Abrew wrote:
> When the platform driver is loaded or unloaded, it overwrites the
> existing LED states. This can cause a loss of early boot state when the
> driver loads, and it can cause the ALARM contact to change state or
> flicker.
>
> Explicitly retain the existing LED state to prevent overwriting on
> driver load and unload.
>
> Signed-off-by: Brodie Abrew <brodie_abrew@selinc.com>
> ---
> V1 -> V2: Fixed code style, added includes, and fixed other reviewer comments
>
> drivers/platform/x86/sel3350-platform.c | 138 ++++++++++++++++++------
> 1 file changed, 105 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/platform/x86/sel3350-platform.c b/drivers/platform/x86/sel3350-platform.c
> index 02e2081e2333..7b0dc1d11199 100644
> --- a/drivers/platform/x86/sel3350-platform.c
> +++ b/drivers/platform/x86/sel3350-platform.c
> @@ -9,6 +9,8 @@
> */
>
> #include <linux/acpi.h>
> +#include <linux/array_size.h>
> +#include <linux/err.h>
> #include <linux/gpio/consumer.h>
> #include <linux/gpio/machine.h>
> #include <linux/leds.h>
> @@ -30,19 +32,82 @@
> #define SEL_PS_B_DETECT "sel_ps_b_detect"
> #define SEL_PS_B_GOOD "sel_ps_b_good"
>
> +#define AUX_LED_GRN1 "sel_aux_led_grn1"
> +#define AUX_LED_GRN2 "sel_aux_led_grn2"
> +#define AUX_LED_GRN3 "sel_aux_led_grn3"
> +#define AUX_LED_GRN4 "sel_aux_led_grn4"
> +#define ALARM_STATE_USER "sel_alarm_state_user"
> +#define ENABLE_STATE_USER "sel_enable_state_user"
> +#define AUX_LED_RED1 "sel_aux_led_red1"
> +#define AUX_LED_RED2 "sel_aux_led_red2"
> +#define AUX_LED_RED3 "sel_aux_led_red3"
> +#define AUX_LED_RED4 "sel_aux_led_red4"
> +
> +static const char *const sel3350_leds_gpio_names[] = {
> + AUX_LED_GRN1,
> + AUX_LED_GRN2,
> + AUX_LED_GRN3,
> + AUX_LED_GRN4,
> + ALARM_STATE_USER,
> + ENABLE_STATE_USER,
> + AUX_LED_RED1,
> + AUX_LED_RED2,
> + AUX_LED_RED3,
> + AUX_LED_RED4,
> +};
> +
> /* LEDs */
> -static const struct gpio_led sel3350_leds[] = {
> - { .name = "sel:green:aux1" },
> - { .name = "sel:green:aux2" },
> - { .name = "sel:green:aux3" },
> - { .name = "sel:green:aux4" },
> - { .name = "sel:red:alarm" },
> +static struct gpio_led sel3350_leds[] = {
> + { .name = "sel:green:aux1",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:green:aux2",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:green:aux3",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:green:aux4",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:red:alarm",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> { .name = "sel:green:enabled",
> - .default_state = LEDS_GPIO_DEFSTATE_ON },
> - { .name = "sel:red:aux1" },
> - { .name = "sel:red:aux2" },
> - { .name = "sel:red:aux3" },
> - { .name = "sel:red:aux4" },
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:red:aux1",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:red:aux2",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:red:aux3",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> + { .name = "sel:red:aux4",
> + .default_state = LEDS_GPIO_DEFSTATE_KEEP,
> + .retain_state_suspended = 1,
> + .retain_state_shutdown = 1,
> + },
> };
>
> static const struct gpio_led_platform_data sel3350_leds_pdata = {
> @@ -50,25 +115,6 @@ static const struct gpio_led_platform_data sel3350_leds_pdata = {
> .leds = sel3350_leds,
> };
>
> -/* Map GPIOs to LEDs */
> -static struct gpiod_lookup_table sel3350_leds_table = {
> - .dev_id = "leds-gpio",
> - .table = {
> - GPIO_LOOKUP_IDX(BXT_NW, 49, NULL, 0, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 50, NULL, 1, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 51, NULL, 2, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_NW, 52, NULL, 3, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_W, 20, NULL, 4, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_W, 21, NULL, 5, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 37, NULL, 6, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 38, NULL, 7, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 39, NULL, 8, GPIO_ACTIVE_HIGH),
> - GPIO_LOOKUP_IDX(BXT_SW, 40, NULL, 9, GPIO_ACTIVE_HIGH),
> - {},
> - }
> -};
> -
> -/* Map GPIOs to power supplies */
> static struct gpiod_lookup_table sel3350_gpios_table = {
> .dev_id = B2093_GPIO_ACPI_ID ":00",
> .table = {
> @@ -76,6 +122,16 @@ static struct gpiod_lookup_table sel3350_gpios_table = {
> GPIO_LOOKUP(BXT_NW, 45, SEL_PS_A_GOOD, GPIO_ACTIVE_LOW),
> GPIO_LOOKUP(BXT_NW, 46, SEL_PS_B_DETECT, GPIO_ACTIVE_LOW),
> GPIO_LOOKUP(BXT_NW, 47, SEL_PS_B_GOOD, GPIO_ACTIVE_LOW),
> + GPIO_LOOKUP(BXT_NW, 49, AUX_LED_GRN1, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 50, AUX_LED_GRN2, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 51, AUX_LED_GRN3, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_NW, 52, AUX_LED_GRN4, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_W, 20, ALARM_STATE_USER, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_W, 21, ENABLE_STATE_USER, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 37, AUX_LED_RED1, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 38, AUX_LED_RED2, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 39, AUX_LED_RED3, GPIO_ACTIVE_HIGH),
> + GPIO_LOOKUP(BXT_SW, 40, AUX_LED_RED4, GPIO_ACTIVE_HIGH),
> {},
> }
> };
> @@ -149,6 +205,7 @@ struct sel3350_data {
> static int sel3350_probe(struct platform_device *pdev)
> {
> int rs;
> + int i;
> struct sel3350_data *sel3350;
> struct power_supply_config ps_cfg = {};
>
> @@ -158,9 +215,20 @@ static int sel3350_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, sel3350);
>
> - gpiod_add_lookup_table(&sel3350_leds_table);
> gpiod_add_lookup_table(&sel3350_gpios_table);
>
> + for (i = 0; i < ARRAY_SIZE(sel3350_leds); ++i) {
> + sel3350_leds[i].gpiod = devm_gpiod_get(&pdev->dev,
> + sel3350_leds_gpio_names[i],
> + GPIOD_ASIS);
> + if (IS_ERR(sel3350_leds[i].gpiod) ||
> + (sel3350_leds[i].gpiod == NULL)) {
> + rs = -EPROBE_DEFER;
> + goto err_gpio_loop;
> + }
> + gpiod_set_consumer_name(sel3350_leds[i].gpiod, sel3350_leds[i].name);
> + }
> +
> sel3350->leds_pdev = platform_device_register_data(
> NULL,
> "leds-gpio",
> @@ -209,11 +277,15 @@ static int sel3350_probe(struct platform_device *pdev)
>
> return 0;
>
> +err_gpio_loop:
> + while (i--)
> + devm_gpiod_put(&pdev->dev, sel3350_leds[i].gpiod);
> + goto err_platform;
> +
> err_ps:
> platform_device_unregister(sel3350->leds_pdev);
> err_platform:
> gpiod_remove_lookup_table(&sel3350_gpios_table);
> - gpiod_remove_lookup_table(&sel3350_leds_table);
>
> return rs;
> }
> @@ -224,7 +296,6 @@ static void sel3350_remove(struct platform_device *pdev)
>
> platform_device_unregister(sel3350->leds_pdev);
> gpiod_remove_lookup_table(&sel3350_gpios_table);
> - gpiod_remove_lookup_table(&sel3350_leds_table);
> }
>
> static const struct acpi_device_id sel3350_device_ids[] = {
> @@ -246,4 +317,5 @@ module_platform_driver(sel3350_platform_driver);
> MODULE_AUTHOR("Schweitzer Engineering Laboratories");
> MODULE_DESCRIPTION("SEL-3350 platform driver");
> MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_ALIAS("platform:sel3350");
> MODULE_SOFTDEP("pre: pinctrl_broxton leds-gpio");
I've been working with Brodie on this and have tested both v1 and v2
patches on the SEL-3350 hardware.
Reviewed-by: Robert Joslyn <robert.joslyn@redrectangle.org>
Tested-By: Robert Joslyn <robert.joslyn@redrectangle.org>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2026-04-10 4:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-08 21:18 [PATCH] platform/x86: sel3350-platform: Retain LED state on load and unload Brodie Abrew
2026-04-09 12:40 ` Ilpo Järvinen
2026-04-09 21:27 ` [PATCH v2] " Brodie Abrew
2026-04-10 4:38 ` Robert Joslyn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox