Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Cc:     linuxarm@huawei.com, mauro.chehab@huawei.com,
        Mauro Carvalho Chehab <mchehab+huawei@kernel.org>,
        Pavel Machek <pavel@ucw.cz>, gregkh@linuxfoundation.org,
        linux-kernel@vger.kernel.org, linux-leds@vger.kernel.org
Subject: [PATCH v2 05/17] leds: leds-nuc: add all types of brightness
Date:   Tue, 18 May 2021 17:08:54 +0200
Message-Id: <d8a6ba2b5afbe9be108e08ce3f28e8f4d0de4055.1621349814.git.mchehab+huawei@kernel.org>
In-Reply-To: <cover.1621349813.git.mchehab+huawei@kernel.org>
References: <cover.1621349813.git.mchehab+huawei@kernel.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: Mauro Carvalho Chehab <mchehab@kernel.org>
To:     unlisted-recipients:; (no To-header on input)
Precedence: bulk

Improve the logic in order to support not only S0 brightness,
but also the brightness for other indicators and for all
power states.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/leds/leds-nuc.c | 369 +++++++++++++++++++++++++++-------------
 1 file changed, 249 insertions(+), 120 deletions(-)

diff --git a/drivers/leds/leds-nuc.c b/drivers/leds/leds-nuc.c
index e12fa2e7a488..df65bf17e0e6 100644
--- a/drivers/leds/leds-nuc.c
+++ b/drivers/leds/leds-nuc.c
@@ -55,21 +55,89 @@ enum led_new_get_subcmd {
 	LED_NEW_GET_CONTROL_ITEM	= 0x01,
 };
 
+enum led_function {
+	LED_FUNC_BRIGHTNESS,
+	LED_FUNC_COLOR1,
+	LED_FUNC_COLOR_GREEN,
+	LED_FUNC_COLOR_BLUE,
+
+	LED_FUNC_BLINK_BEHAVIOR,
+	LED_FUNC_BLINK_FREQ,
+
+	LED_FUNC_HDD_BEHAVIOR,
+	LED_FUNC_ETH_TYPE,
+	LED_FUNC_POWER_LIMIT_SCHEME,
+
+	MAX_LED_FUNC
+};
+
+enum led_indicators {
+	LED_IND_POWER_STATE,
+	LED_IND_HDD_ACTIVITY,
+	LED_IND_ETHERNET,
+	LED_IND_WIFI,
+	LED_IND_SOFTWARE,
+	LED_IND_POWER_LIMIT,
+	LED_IND_DISABLE,
+
+	MAX_IND = LED_IND_DISABLE
+};
+
+/*
+ * control items ID for each of the valid indicators on spec Rev 0.64.
+ */
+static const u8 led_func_rev_0_64[MAX_IND][MAX_LED_FUNC] = {
+	[LED_IND_POWER_STATE] = {	/* Offsets for each power state */
+		[LED_FUNC_BRIGHTNESS]		= 0x00,
+		[LED_FUNC_BLINK_BEHAVIOR]	= 0x01,
+		[LED_FUNC_BLINK_FREQ]		= 0x02,
+		[LED_FUNC_COLOR1]		= 0x03,
+		[LED_FUNC_COLOR_GREEN]		= 0x04,
+		[LED_FUNC_COLOR_BLUE]		= 0x05
+	},
+	[LED_IND_HDD_ACTIVITY] = {
+		[LED_FUNC_BRIGHTNESS]		= 0x00,
+		[LED_FUNC_COLOR1]		= 0x01,
+		[LED_FUNC_COLOR_GREEN]		= 0x02,
+		[LED_FUNC_COLOR_BLUE]		= 0x03,
+		[LED_FUNC_HDD_BEHAVIOR]		= 0x04
+	},
+	[LED_IND_ETHERNET] = {
+		[LED_FUNC_ETH_TYPE]		= 0x00,
+		[LED_FUNC_BRIGHTNESS]		= 0x01,
+		[LED_FUNC_COLOR1]		= 0x02,
+		[LED_FUNC_COLOR_GREEN]		= 0x03,
+		[LED_FUNC_COLOR_BLUE]		= 0x04
+	},
+	[LED_IND_WIFI] = {
+		[LED_FUNC_BRIGHTNESS]		= 0x00,
+		[LED_FUNC_COLOR1]		= 0x01,
+		[LED_FUNC_COLOR_GREEN]		= 0x02,
+		[LED_FUNC_COLOR_BLUE]		= 0x03
+	},
+	[LED_IND_SOFTWARE] = {
+		[LED_FUNC_BRIGHTNESS]		= 0x00,
+		[LED_FUNC_BLINK_BEHAVIOR]	= 0x01,
+		[LED_FUNC_BLINK_FREQ]		= 0x02,
+		[LED_FUNC_COLOR1]		= 0x03,
+		[LED_FUNC_COLOR_GREEN]		= 0x04,
+		[LED_FUNC_COLOR_BLUE]		= 0x05
+	},
+	[LED_IND_POWER_LIMIT] = {
+		[LED_FUNC_POWER_LIMIT_SCHEME]	= 0x00,
+		[LED_FUNC_BRIGHTNESS]		= 0x01,
+		[LED_FUNC_COLOR1]		= 0x02,
+		[LED_FUNC_COLOR_GREEN]		= 0x03,
+		[LED_FUNC_COLOR_BLUE]		= 0x04
+	},
+};
+
 /* LED color indicator */
 #define LED_BLUE_AMBER		BIT(0)
 #define LED_BLUE_WHITE		BIT(1)
 #define LED_RGB			BIT(2)
 #define	LED_SINGLE_COLOR	BIT(3)
 
-/* LED indicator options */
-#define LED_IND_POWER_STATE	BIT(0)
-#define LED_IND_HDD_ACTIVITY	BIT(1)
-#define LED_IND_ETHERNET	BIT(2)
-#define LED_IND_WIFI		BIT(3)
-#define LED_IND_SOFTWARE	BIT(4)
-#define LED_IND_POWER_LIMIT	BIT(5)
-#define LED_IND_DISABLE		BIT(6)
-
 static const char * const led_names[] = {
 	"nuc::power",
 	"nuc::hdd",
@@ -87,7 +155,6 @@ struct nuc_nmi_led {
 	u8 indicator;
 	u32 color_type;
 	u32 avail_indicators;
-	u32 control_items;
 };
 
 struct nuc_wmi {
@@ -201,9 +268,9 @@ static int nuc_nmi_cmd(struct device *dev,
 static int nuc_wmi_query_leds(struct device *dev)
 {
 	struct nuc_wmi *priv = dev_get_drvdata(dev);
-	u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
+	u8 input[NUM_INPUT_ARGS] = { 0 };
 	u8 output[NUM_OUTPUT_ARGS];
-	int i, id, ret, ver = LED_API_UNKNOWN;
+	int id, ret, ver = LED_API_UNKNOWN;
 	u8 leds;
 
 	/*
@@ -214,9 +281,8 @@ static int nuc_wmi_query_leds(struct device *dev)
 	 * FIXME: Should add a fallback code for it to work with older NUCs,
 	 * as LED_QUERY returns an error on older devices like Skull Canyon.
 	 */
-	cmd = LED_QUERY;
 	input[0] = LED_QUERY_LIST_ALL;
-	ret = nuc_nmi_cmd(dev, cmd, input, output);
+	ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);
 	if (ret == -ENOENT) {
 		ver = LED_API_NUC6;
 	} else if (ret) {
@@ -255,12 +321,11 @@ static int nuc_wmi_query_leds(struct device *dev)
 
 		led->id = id;
 
-		cmd = LED_QUERY;
 		input[0] = LED_QUERY_COLOR_TYPE;
 		input[1] = id;
-		ret = nuc_nmi_cmd(dev, cmd, input, output);
+		ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);
 		if (ret) {
-			dev_warn(dev, "error %d on led %i\n", ret, i);
+			dev_warn(dev, "error %d on led %i\n", ret, id);
 			return ret;
 		}
 
@@ -268,23 +333,11 @@ static int nuc_wmi_query_leds(struct device *dev)
 				  output[1] << 8 |
 				  output[2] << 16;
 
-		cmd = LED_NEW_GET_STATUS;
-		input[0] = LED_NEW_GET_CURRENT_INDICATOR;
-		input[1] = i;
-		ret = nuc_nmi_cmd(dev, cmd, input, output);
-		if (ret) {
-			dev_warn(dev, "error %d on led %i\n", ret, i);
-			return ret;
-		}
-
-		led->indicator = output[0];
-
-		cmd = LED_QUERY;
 		input[0] = LED_QUERY_INDICATOR_OPTIONS;
-		input[1] = i;
-		ret = nuc_nmi_cmd(dev, cmd, input, output);
+		input[1] = id;
+		ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);
 		if (ret) {
-			dev_warn(dev, "error %d on led %i\n", ret, i);
+			dev_warn(dev, "error %d on led %i\n", ret, id);
 			return ret;
 		}
 
@@ -292,23 +345,18 @@ static int nuc_wmi_query_leds(struct device *dev)
 					output[1] << 8 |
 					output[2] << 16;
 
-		cmd = LED_QUERY;
-		input[0] = LED_QUERY_CONTROL_ITEMS;
-		input[1] = i;
-		input[2] = led->indicator;
-		ret = nuc_nmi_cmd(dev, cmd, input, output);
+		input[0] = LED_NEW_GET_CURRENT_INDICATOR;
+		input[1] = id;
+		ret = nuc_nmi_cmd(dev, LED_NEW_GET_STATUS, input, output);
 		if (ret) {
-			dev_warn(dev, "error %d on led %i\n", ret, i);
+			dev_warn(dev, "error %d on led %i\n", ret, id);
 			return ret;
 		}
+		led->indicator = output[0];
 
-		led->control_items = output[0]      |
-				     output[1] << 8 |
-				     output[2] << 16;
-
-		dev_dbg(dev, "%s: id: %02x, color type: %06x, indicator: %06x, control items: %06x\n",
-			led_names[led->id], led->id,
-			led->color_type, led->indicator, led->control_items);
+		dev_dbg(dev, "%s: id: %02x, color type: %06x, indicator: %02x (avail %06x)\n",
+			led_names[led->id], led->id, led->color_type,
+			led->indicator, led->avail_indicators);
 
 		priv->num_leds++;
 	}
@@ -316,6 +364,82 @@ static int nuc_wmi_query_leds(struct device *dev)
 	return 0;
 }
 
+static bool nuc_wmi_test_control(struct device *dev,
+				 struct nuc_nmi_led *led, u8 ctrl)
+{
+	int ret, avail_ctrls;
+	u8 output[NUM_OUTPUT_ARGS];
+	u8 input[NUM_INPUT_ARGS] = {
+		LED_QUERY_CONTROL_ITEMS,
+		led->id,
+		led->indicator
+	};
+
+	ret = nuc_nmi_cmd(dev, LED_QUERY, input, output);
+	if (ret)
+		return false;
+
+	avail_ctrls = output[0]      |
+		      output[1] << 8 |
+		      output[2] << 16;
+
+	return avail_ctrls & BIT(ctrl);
+}
+
+static int nuc_wmi_get_brightness_offset(struct device *dev,
+					 struct nuc_nmi_led *led, u8 offset)
+{
+	u8 input[NUM_INPUT_ARGS];
+	u8 output[NUM_OUTPUT_ARGS];
+	int ret, ctrl;
+
+	if (led->indicator == LED_IND_DISABLE)
+		return -ENODEV;
+
+	ctrl = led_func_rev_0_64[led->indicator][LED_FUNC_BRIGHTNESS] + offset;
+
+	if (!nuc_wmi_test_control(dev, led, ctrl))
+		return -ENODEV;
+
+	input[0] = LED_NEW_GET_CONTROL_ITEM;
+	input[1] = led->id;
+	input[2] = led->indicator;
+	input[3] = ctrl;
+
+	ret = nuc_nmi_cmd(dev, LED_NEW_GET_STATUS, input, output);
+	if (ret)
+		return ret;
+
+	dev_dbg(dev, "%s: id: %02x, brightness: %02x\n",
+		led_names[led->id], led->id, output[0]);
+
+	return output[0];
+}
+
+static ssize_t nuc_wmi_set_brightness_offset(struct device *dev,
+					     struct nuc_nmi_led *led,
+					     u8 offset,
+					     u8 val)
+{
+	u8 input[NUM_INPUT_ARGS];
+	int ctrl;
+
+	if (led->indicator == LED_IND_DISABLE)
+		return -ENODEV;
+
+	ctrl = led_func_rev_0_64[led->indicator][LED_FUNC_BRIGHTNESS] + offset;
+
+	if (!nuc_wmi_test_control(dev, led, ctrl))
+		return -ENODEV;
+
+	input[0] = led->id;
+	input[1] = led->indicator;
+	input[2] = ctrl;
+	input[3] = val;
+
+	return nuc_nmi_cmd(dev, LED_SET_VALUE, input, NULL);
+}
+
 /*
  * LED show/store routines
  */
@@ -323,6 +447,21 @@ static int nuc_wmi_query_leds(struct device *dev)
 #define LED_ATTR_RW(_name) \
 	DEVICE_ATTR(_name, 0644, show_##_name, store_##_name)
 
+#define LED_ATTR_POWER_STATE_RW(_name, offset)				       \
+	static ssize_t show_##_name(struct device *dev,			       \
+				    struct device_attribute *attr,	       \
+				    char *buf)				       \
+	{								       \
+		return show_brightness_offset(dev, attr, offset, buf);	       \
+	}								       \
+	static ssize_t store_##_name(struct device *dev,		       \
+				    struct device_attribute *attr,	       \
+				    const char *buf, size_t len)	       \
+	{								       \
+		return store_brightness_offset(dev, attr, offset, buf, len);   \
+	}								       \
+	static DEVICE_ATTR(_name, 0644, show_##_name, store_##_name)
+
 static const char * const led_indicators[] = {
 	"Power State",
 	"HDD Activity",
@@ -395,98 +534,93 @@ static ssize_t store_indicator(struct device *dev,
 	return -EINVAL;
 }
 
-/* Get S0 brightness */
-static ssize_t show_s0_brightness(struct device *dev,
-				  struct device_attribute *attr,
-				  char *buf)
+/* Get brightness */
+static ssize_t show_brightness_offset(struct device *dev,
+				      struct device_attribute *attr,
+				      u8 offset,
+				      char *buf)
 {
 	struct led_classdev *cdev = dev_get_drvdata(dev);
 	struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
-	u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
-	u8 output[NUM_OUTPUT_ARGS];
 	int ret;
 
-	cmd = LED_NEW_GET_STATUS;
-	input[0] = LED_NEW_GET_CONTROL_ITEM;
-	input[1] = led->id;
-	input[2] = led->indicator;
-	input[3] = 0;
+	if (led->indicator != LED_IND_POWER_STATE)
+		return -ENODEV;
 
-	ret = nuc_nmi_cmd(dev, cmd, input, output);
-	if (ret)
+	ret = nuc_wmi_get_brightness_offset(dev, led, offset);
+	if (ret < 0)
 		return ret;
 
-	/* Multicolor uses a scale from 0 to 100 */
-	if (led->color_type & (LED_BLUE_AMBER | LED_BLUE_WHITE | LED_RGB))
-		return scnprintf(buf, PAGE_SIZE, "%d%%\n", output[0]);
-
-	/* single color uses 0, 50% and 100% */
-	return scnprintf(buf, PAGE_SIZE, "%d%%\n", output[0] * 50);
+	return scnprintf(buf, PAGE_SIZE, "%d\n", ret);
 }
 
-/* Change S0 brightness */
-static ssize_t store_s0_brightness(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t len)
+/* Change brightness */
+static ssize_t store_brightness_offset(struct device *dev,
+				       struct device_attribute *attr,
+				       u8 offset,
+				       const char *buf, size_t len)
 {
 	struct led_classdev *cdev = dev_get_drvdata(dev);
 	struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
-	u8 cmd, input[NUM_INPUT_ARGS] = { 0 };
 	int ret;
 	u8 val;
 
-	if (led->indicator == LED_IND_DISABLE) {
-		dev_dbg(dev, "Led %s is disabled. ignoring it.\n", cdev->name);
-		return -EACCES;
-	}
+	if (led->indicator != LED_IND_POWER_STATE)
+		return -ENODEV;
 
 	if (kstrtou8(buf, 0, &val) || val > 100)
 		return -EINVAL;
 
-	/*
-	 * For single-color LEDs, the value should be between 0 to 2, but,
-	 * in order to have a consistent API, let's always handle it as if
-	 * it is a percentage, for both multicolor and single color LEDs.
-	 * So:
-	 *	value == 0 will disable the LED
-	 * 	value up to 74% will set it the brightness to 50%
-	 * 	value equal or above 75% will use the maximum brightness.
-	 */
-	if (!(led->color_type & (LED_BLUE_AMBER | LED_BLUE_WHITE | LED_RGB))) {
-		if (val > 0 && val < 75)
-			val = 1;
-		if (val >= 75)
-			val = 2;
-	}
-
-	cmd = LED_SET_VALUE;
-	input[0] = led->id;
-	input[1] = led->indicator;
-	input[2] = 0;		/* FIXME: replace by an enum */
-	input[3] = val;
-
-	ret = nuc_nmi_cmd(dev, cmd, input, NULL);
+	ret = nuc_wmi_set_brightness_offset(dev, led, offset, val);
 	if (ret)
 		return ret;
 
 	return len;
 }
 
+static enum led_brightness nuc_wmi_get_brightness(struct led_classdev *cdev)
+{
+	struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+
+	if (led->indicator == LED_IND_POWER_STATE)
+		return -ENODEV;
+
+	return nuc_wmi_get_brightness_offset(cdev->dev, led, 0);
+}
+
+static int nuc_wmi_set_brightness(struct led_classdev *cdev,
+				  enum led_brightness brightness)
+{
+	struct nuc_nmi_led *led = container_of(cdev, struct nuc_nmi_led, cdev);
+
+	if (led->indicator == LED_IND_POWER_STATE)
+		return -ENODEV;
+
+	return nuc_wmi_set_brightness_offset(cdev->dev, led, 0, brightness);
+}
+
 static LED_ATTR_RW(indicator);
-static LED_ATTR_RW(s0_brightness);
+
+LED_ATTR_POWER_STATE_RW(s0_brightness, 0x00);
+LED_ATTR_POWER_STATE_RW(s3_brightness, 0x06);
+LED_ATTR_POWER_STATE_RW(s5_brightness, 0x0c);
+LED_ATTR_POWER_STATE_RW(ready_mode_brightness, 0x12);
 
 /*
- * Attributes for multicolor LEDs
+ * Attributes for LEDs
  */
 
-static struct attribute *nuc_wmi_multicolor_led_attr[] = {
+static struct attribute *nuc_wmi_led_attr[] = {
 	&dev_attr_indicator.attr,
 	&dev_attr_s0_brightness.attr,
+	&dev_attr_s3_brightness.attr,
+	&dev_attr_s5_brightness.attr,
+	&dev_attr_ready_mode_brightness.attr,
 	NULL,
 };
 
 static const struct attribute_group nuc_wmi_led_attribute_group = {
-	.attrs		= nuc_wmi_multicolor_led_attr,
+	.attrs = nuc_wmi_led_attr,
 };
 
 static const struct attribute_group *nuc_wmi_led_attribute_groups[] = {
@@ -496,30 +630,25 @@ static const struct attribute_group *nuc_wmi_led_attribute_groups[] = {
 
 static int nuc_wmi_led_register(struct device *dev, struct nuc_nmi_led *led)
 {
+	int brightness = nuc_wmi_get_brightness_offset(dev, led, 0);
+
 	led->cdev.name = led_names[led->id];
-
 	led->dev = dev;
 	led->cdev.groups = nuc_wmi_led_attribute_groups;
+	led->cdev.brightness_get = nuc_wmi_get_brightness;
+	led->cdev.brightness_set_blocking = nuc_wmi_set_brightness;
 
-	/*
-	 * It can't let the classdev to manage the brightness due to several
-	 * reasons:
-	 *
-	 * 1) classdev has some internal logic to manage the brightness,
-	 *    at set_brightness_delayed(), which starts disabling the LEDs;
-	 *    While this makes sense on most cases, here, it would appear
-	 *    that the NUC was powered off, which is not what happens;
-	 * 2) classdev unconditionally tries to set brightness for all
-	 *    leds, including the ones that were software-disabled or
-	 *    disabled disabled via BIOS menu;
-	 * 3) There are 3 types of brightness values for each LED, depending
-	 *    on the CPU power state: S0, S3 and S5.
-	 *
-	 * So, the best seems to export everything via sysfs attributes
-	 * directly. This would require some further changes at the
-	 * LED class, though, or we would need to create our own LED
-	 * class, which seems wrong.
-	 */
+	if (led->color_type & LED_SINGLE_COLOR)
+		led->cdev.max_brightness = 2;
+	else
+		led->cdev.max_brightness = 100;
+
+	/* Ensure that the current bright will be preserved */
+	if (brightness >= 0)
+		led->cdev.delayed_set_value = brightness;
+
+	/* Suppress warnings for the LED(s) indicating the power state */
+	led->cdev.flags = LED_HW_PLUGGABLE | LED_UNREGISTERING;
 
 	return devm_led_classdev_register(dev, &led->cdev);
 }
-- 
2.31.1