From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 58C7729C33F for ; Thu, 7 May 2026 07:52:32 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.16 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778140354; cv=none; b=Po9J6o/Oh5Dq8xCVKTawqZCJe2bA7kqXfejXOVL/+e0LbBDwbGSLsr7QSERhLlZINrPl4jnE3TI/pr7yEUEgahgiuZ2eMWlMi5aAeXyUieWnkLjPvvl4oOsD7uMrnYR38/ApFz1oV4/d6rgghgGoIBYpX5b7ewFdNvPd+ZxFxVY= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1778140354; c=relaxed/simple; bh=jzU0gvawixkGoLbKCnJOapblF08eXxELsJ+5TB5AIqg=; h=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=SPx1nOvVf4EJozhqgPmJlISm0OgIRhaGW3blUVITsrvWEEtQlcbmr3Dh3BsClsI7G3vXK9r2Ejw/6EhKZGnelpzAgMtFkn23UcC4pRapwEkIWX9U++azmSdxpMcrTm2UzlgzabBX6vb/pMi5awdwWIeOfImSEN+79RMZeE4soXY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=pass smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=QiWe89Yo; arc=none smtp.client-ip=192.198.163.16 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="QiWe89Yo" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1778140352; x=1809676352; h=from:date:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=jzU0gvawixkGoLbKCnJOapblF08eXxELsJ+5TB5AIqg=; b=QiWe89YoT/lrhh2IGRQZNphP/n0+pPXjHQD3JqbQjF6pEG4cR0qr82YW lLzpQhYjXHMUUmbfjuA3feTBR2C6IkdCCreXpeQ4rzpRMt+CsI3m8Ay4r dKOK39L1moRrkTxXgOn5UvELiF9rpDonKzUcS1SpdS/v1/v89hQZE8kpx 4U4i5WtEtsa1so9jwbbqZOGBhAYEy9sXneLSgOyv2sVxFPDt0kbyMwhS1 59emXKqTAqv7NlNljSwt/JH0MLu/3gVUDqrH5KM1ZSwrtsNpLtEZ7J3UO B2N8A3nU1KlLEMHb00hgQwMmi9cc5A6G7Nq/4d/CyipV5gGxEBMC7pnZX g==; X-CSE-ConnectionGUID: dn5DwInLTlmawmxLMA0eJw== X-CSE-MsgGUID: NjLGNn+JSQCxW2OPUhE2ew== X-IronPort-AV: E=McAfee;i="6800,10657,11778"; a="66612989" X-IronPort-AV: E=Sophos;i="6.23,221,1770624000"; d="scan'208";a="66612989" Received: from orviesa007.jf.intel.com ([10.64.159.147]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2026 00:52:32 -0700 X-CSE-ConnectionGUID: FOf5t12URT+FnrZ9drNFNg== X-CSE-MsgGUID: x75ugP6+TmWL621qCupKfA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.23,221,1770624000"; d="scan'208";a="236651448" Received: from ijarvine-mobl1.ger.corp.intel.com (HELO localhost) ([10.245.245.116]) by orviesa007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 07 May 2026 00:52:30 -0700 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Thu, 7 May 2026 10:52:26 +0300 (EEST) To: Brodie Abrew 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 In-Reply-To: <20260507004916.6710-1-brodie_abrew@selinc.com> Message-ID: <1f6f8f8a-8a4a-07ef-3979-3fc76f557a3c@linux.intel.com> References: <65829704-a7b8-208a-255c-6d4586b75772@linux.intel.com> <20260507004916.6710-1-brodie_abrew@selinc.com> Precedence: bulk X-Mailing-List: platform-driver-x86@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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 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 Tested-By: Robert Joslyn 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 > +#include > +#include > #include > #include > #include > @@ -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[] = { >