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 v3] platform/x86: sel3350-platform: Retain LED state on load and unload
Date: Thu, 7 May 2026 10:52:26 +0300 (EEST) [thread overview]
Message-ID: <1f6f8f8a-8a4a-07ef-3979-3fc76f557a3c@linux.intel.com> (raw)
In-Reply-To: <20260507004916.6710-1-brodie_abrew@selinc.com>
On Wed, 6 May 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>
Hi,
Thanks for the update.
The patch submitter is expected to collect tags and include them into the
submission of the next version. The reason for that is that our maintainer
tools only pick the tags automatically for the current version of the
patch. No need to resubmit to fix this (see below).
There's a somewhat fuzzy line when the tags becomes invalid and should not
be carried over when the changes between versions are very extensive but
I don't think there were any major enough changes from v2 -> v3.
I'm adding the tags again to this thread so they don't get forgotten (I'll
process this patch a bit later):
Reviewed-by: Robert Joslyn <robert.joslyn@redrectangle.org>
Tested-By: Robert Joslyn <robert.joslyn@redrectangle.org>
The code change seemed fine now.
--
i.
> ---
> V1 -> V2: Fixed code style, added includes, and fixed other reviewer comments
> V2 -> V3: Simplified if statement and removed unnecessary MODULE_ALIAS
>
> drivers/platform/x86/sel3350-platform.c | 136 ++++++++++++++++++------
> 1 file changed, 103 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/platform/x86/sel3350-platform.c b/drivers/platform/x86/sel3350-platform.c
> index 02e2081e2333..f3a314235632 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,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_OR_NULL(sel3350_leds[i].gpiod)) {
> + 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 +276,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 +295,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[] = {
>
next prev parent reply other threads:[~2026-05-07 7:52 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
2026-05-07 0:49 ` [PATCH v3] " Brodie Abrew
2026-05-07 7:52 ` Ilpo Järvinen [this message]
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=1f6f8f8a-8a4a-07ef-3979-3fc76f557a3c@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