* [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