From: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
To: Brodie Abrew <brodie_abrew@selinc.com>
Cc: platform-driver-x86@vger.kernel.org, robert.joslyn@redrectangle.org
Subject: Re: [PATCH v2] platform/x86: sel3350-platform: Retain LED state on load and unload
Date: Wed, 6 May 2026 13:55:50 +0300 (EEST) [thread overview]
Message-ID: <65829704-a7b8-208a-255c-6d4586b75772@linux.intel.com> (raw)
In-Reply-To: <20260409212701.88577-1-brodie_abrew@selinc.com>
On Thu, 9 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, 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)) {
IS_ERR_OR_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");
How is this related to preserving led states on load/unload??
--
i.
next prev parent reply other threads:[~2026-05-06 10:55 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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
2026-05-06 10:55 ` Ilpo Järvinen [this message]
2026-05-07 0:49 ` [PATCH v3] " Brodie Abrew
2026-05-07 7:52 ` Ilpo Järvinen
2026-05-11 17:52 ` Ilpo Järvinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=65829704-a7b8-208a-255c-6d4586b75772@linux.intel.com \
--to=ilpo.jarvinen@linux.intel.com \
--cc=brodie_abrew@selinc.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=robert.joslyn@redrectangle.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox