[PATCH v5 1/3] platform/chrome: wilco_ec: Standardize mailbox interface

[PATCH v5 1/3] platform/chrome: wilco_ec: Standardize mailbox interface

[Nick Crews]

The current API for the wilco EC mailbox interface is bad.

It assumes that most messages sent to the EC follow a similar structure,
with a command byte in MBOX[0], followed by a junk byte, followed by
actual data. This doesn't happen in several cases, such as setting the
RTC time, using the raw debugfs interface, and reading or writing
properties such as the Peak Shift policy (this last to be submitted soon).

Similarly for the response message from the EC, the current interface
assumes that the first byte of data is always 0, and the second byte
is unused. However, in both setting and getting the RTC time, in the
debugfs interface, and for reading and writing properties, this isn't
true.

The current way to resolve this is to use WILCO_EC_FLAG_RAW* flags to
specify when and when not to skip these initial bytes in the sent and
received message. They are confusing and used so much that they are
normal, and not exceptions. In addition, the first byte of
response in the debugfs interface is still always skipped, which is
weird, since this raw interface should be giving the entire result.

Additionally, sent messages assume the first byte is a command, and so
struct wilco_ec_message contains the "command" field. In setting or
getting properties however, the first byte is not a command, and so this
field has to be filled with a byte that isn't actually a command. This
is again inconsistent.

wilco_ec_message contains a result field as well, copied from
wilco_ec_response->result. The message result field should be removed:
if the message fails, the cause is already logged, and the callers are
alerted. They will never care about the actual state of the result flag.

These flags and different cases make the wilco_ec_transfer() function,
used in wilco_ec_mailbox(), really gross, dealing with a bunch of
different cases. It's difficult to figure out what it is doing.

Finally, making these assumptions about the structure of a message make
it so that the messages do not correspond well with the specification
for the EC's mailbox interface. For instance, this interface
specification may say that MBOX[9] in the received message contains
some information, but the calling code needs to remember that the first
byte of response is always skipped, and because it didn't set the
RESPONSE_RAW flag, the next byte is also skipped, so this information
is actually contained within wilco_ec_message->response_data[7]. This
makes it difficult to maintain this code in the future.

To fix these problems this patch standardizes the mailbox interface by:
- Removing the WILCO_EC_FLAG_RAW* flags
- Removing the command and reserved_raw bytes from wilco_ec_request
- Removing the mbox0 byte from wilco_ec_response
- Simplifying wilco_ec_transfer() because of these changes
- Gives the callers of wilco_ec_mailbox() the responsibility of exactly
  and consistently defining the structure of the mailbox request and
  response
- Removing command and result from wilco_ec_message.

This results in the reduction of total code, and makes it much more
maintainable and understandable.

Signed-off-by: Nick Crews <ncrews@chromium.org>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
 drivers/platform/chrome/wilco_ec/debugfs.c | 43 ++++++++-------
 drivers/platform/chrome/wilco_ec/mailbox.c | 53 ++++++++----------
 drivers/rtc/rtc-wilco-ec.c                 | 63 +++++++++++++---------
 include/linux/platform_data/wilco-ec.h     | 22 +-------
 4 files changed, 83 insertions(+), 98 deletions(-)

diff --git a/drivers/platform/chrome/wilco_ec/debugfs.c b/drivers/platform/chrome/wilco_ec/debugfs.c
index c090db2cd5be..17c4c9068aaf 100644
--- a/drivers/platform/chrome/wilco_ec/debugfs.c
+++ b/drivers/platform/chrome/wilco_ec/debugfs.c
@@ -10,25 +10,26 @@
  * by reading from raw.
  *
  * For writing:
- * Bytes 0-1 indicate the message type:
- *         00 F0 = Execute Legacy Command
- *         00 F2 = Read/Write NVRAM Property
- * Byte 2 provides the command code
- * Bytes 3+ consist of the data passed in the request
+ * Bytes 0-1 indicate the message type, one of enum wilco_ec_msg_type
+ * Byte 2+ consist of the data passed in the request, starting at MBOX[0]
  *
- * When referencing the EC interface spec, byte 2 corresponds to MBOX[0],
- * byte 3 corresponds to MBOX[1], etc.
- *
- * At least three bytes are required, for the msg type and command,
- * with additional bytes optional for additional data.
+ * At least three bytes are required for writing, two for the type and at
+ * least a single byte of data. Only the first EC_MAILBOX_DATA_SIZE bytes
+ * of MBOX will be used.
  *
  * Example:
  * // Request EC info type 3 (EC firmware build date)
- * $ echo 00 f0 38 00 03 00 > raw
+ * // Corresponds with sending type 0x00f0 with MBOX = [38, 00, 03, 00]
+ * $ echo 00 f0 38 00 03 00 > /sys/kernel/debug/wilco_ec/raw
  * // View the result. The decoded ASCII result "12/21/18" is
  * // included after the raw hex.
- * $ cat raw
- * 00 31 32 2f 32 31 2f 31 38 00 38 00 01 00 2f 00  .12/21/18.8...
+ * // Corresponds with MBOX = [00, 00, 31, 32, 2f, 32, 31, ...]
+ * $ cat /sys/kernel/debug/wilco_ec/raw
+ * 00 00 31 32 2f 32 31 2f 31 38 00 38 00 01 00 2f 00  ..12/21/18.8...
+ *
+ * Note that the first 32 bytes of the received MBOX[] will be printed,
+ * even if some of the data is junk. It is up to you to know how many of
+ * the first bytes of data are the actual response.
  */
 
 #include <linux/ctype.h>
@@ -136,18 +137,15 @@ static ssize_t raw_write(struct file *file, const char __user *user_buf,
 	ret = parse_hex_sentence(buf, kcount, request_data, TYPE_AND_DATA_SIZE);
 	if (ret < 0)
 		return ret;
-	/* Need at least two bytes for message type and one for command */
+	/* Need at least two bytes for message type and one byte of data */
 	if (ret < 3)
 		return -EINVAL;
 
-	/* Clear response data buffer */
-	memset(debug_info->raw_data, '\0', EC_MAILBOX_DATA_SIZE_EXTENDED);
-
 	msg.type = request_data[0] << 8 | request_data[1];
-	msg.flags = WILCO_EC_FLAG_RAW;
-	msg.command = request_data[2];
-	msg.request_data = ret > 3 ? request_data + 3 : 0;
-	msg.request_size = ret - 3;
+	msg.flags = 0;
+	msg.request_data = request_data + 2;
+	msg.request_size = ret - 2;
+	memset(debug_info->raw_data, 0, sizeof(debug_info->raw_data));
 	msg.response_data = debug_info->raw_data;
 	msg.response_size = EC_MAILBOX_DATA_SIZE;
 
@@ -174,7 +172,8 @@ static ssize_t raw_read(struct file *file, char __user *user_buf, size_t count,
 		fmt_len = hex_dump_to_buffer(debug_info->raw_data,
 					     debug_info->response_size,
 					     16, 1, debug_info->formatted_data,
-					     FORMATTED_BUFFER_SIZE, true);
+					     sizeof(debug_info->formatted_data),
+					     true);
 		/* Only return response the first time it is read */
 		debug_info->response_size = 0;
 	}
diff --git a/drivers/platform/chrome/wilco_ec/mailbox.c b/drivers/platform/chrome/wilco_ec/mailbox.c
index 14355668ddfa..7fb58b487963 100644
--- a/drivers/platform/chrome/wilco_ec/mailbox.c
+++ b/drivers/platform/chrome/wilco_ec/mailbox.c
@@ -92,21 +92,10 @@ static void wilco_ec_prepare(struct wilco_ec_message *msg,
 			     struct wilco_ec_request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
-
-	/* Handle messages without trimming bytes from the request */
-	if (msg->request_size && msg->flags & WILCO_EC_FLAG_RAW_REQUEST) {
-		rq->reserved_raw = *(u8 *)msg->request_data;
-		msg->request_size--;
-		memmove(msg->request_data, msg->request_data + 1,
-			msg->request_size);
-	}
-
-	/* Fill in request packet */
 	rq->struct_version = EC_MAILBOX_PROTO_VERSION;
 	rq->mailbox_id = msg->type;
 	rq->mailbox_version = EC_MAILBOX_VERSION;
-	rq->data_size = msg->request_size + EC_MAILBOX_DATA_EXTRA;
-	rq->command = msg->command;
+	rq->data_size = msg->request_size;
 
 	/* Checksum header and data */
 	rq->checksum = wilco_ec_checksum(rq, sizeof(*rq));
@@ -159,6 +148,12 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 		return -EIO;
 	}
 
+	/*
+	 * The EC always returns either EC_MAILBOX_DATA_SIZE or
+	 * EC_MAILBOX_DATA_SIZE_EXTENDED bytes of data, so we need to
+	 * calculate the checksum on **all** of this data, even if we
+	 * won't use all of it.
+	 */
 	if (msg->flags & WILCO_EC_FLAG_EXTENDED_DATA)
 		size = EC_MAILBOX_DATA_SIZE_EXTENDED;
 	else
@@ -173,33 +168,26 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
 		return -EBADMSG;
 	}
 
-	/* Check that the EC reported success */
-	msg->result = rs->result;
-	if (msg->result) {
-		dev_dbg(ec->dev, "bad response: 0x%02x\n", msg->result);
+	if (rs->result) {
+		dev_dbg(ec->dev, "EC reported failure: 0x%02x\n", rs->result);
 		return -EBADMSG;
 	}
 
-	/* Check the returned data size, skipping the header */
 	if (rs->data_size != size) {
 		dev_dbg(ec->dev, "unexpected packet size (%u != %zu)",
 			rs->data_size, size);
 		return -EMSGSIZE;
 	}
 
-	/* Skip 1 response data byte unless specified */
-	size = (msg->flags & WILCO_EC_FLAG_RAW_RESPONSE) ? 0 : 1;
-	if ((ssize_t) rs->data_size - size < msg->response_size) {
-		dev_dbg(ec->dev, "response data too short (%zd < %zu)",
-			(ssize_t) rs->data_size - size, msg->response_size);
+	if (rs->data_size < msg->response_size) {
+		dev_dbg(ec->dev, "EC didn't return enough data (%u < %zu)",
+			rs->data_size, msg->response_size);
 		return -EMSGSIZE;
 	}
 
-	/* Ignore response data bytes as requested */
-	memcpy(msg->response_data, rs->data + size, msg->response_size);
+	memcpy(msg->response_data, rs->data, msg->response_size);
 
-	/* Return actual amount of data received */
-	return msg->response_size;
+	return rs->data_size;
 }
 
 /**
@@ -207,10 +195,12 @@ static int wilco_ec_transfer(struct wilco_ec_device *ec,
  * @ec: EC device.
  * @msg: EC message data for request and response.
  *
- * On entry msg->type, msg->flags, msg->command, msg->request_size,
- * msg->response_size, and msg->request_data should all be filled in.
+ * On entry msg->type, msg->request_size, and msg->request_data should all be
+ * filled in. If desired, msg->flags can be set.
  *
- * On exit msg->result and msg->response_data will be filled.
+ * If a response is expected, msg->response_size should be set, and
+ * msg->response_data should point to a buffer with enough space. On exit
+ * msg->response_data will be filled.
  *
  * Return: number of bytes received or negative error code on failure.
  */
@@ -219,9 +209,8 @@ int wilco_ec_mailbox(struct wilco_ec_device *ec, struct wilco_ec_message *msg)
 	struct wilco_ec_request *rq;
 	int ret;
 
-	dev_dbg(ec->dev, "cmd=%02x type=%04x flags=%02x rslen=%zu rqlen=%zu\n",
-		msg->command, msg->type, msg->flags, msg->response_size,
-		msg->request_size);
+	dev_dbg(ec->dev, "type=%04x flags=%02x rslen=%zu rqlen=%zu\n",
+		msg->type, msg->flags, msg->response_size, msg->request_size);
 
 	mutex_lock(&ec->mailbox_lock);
 	/* Prepare request packet */
diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c
index e62bda0cb53e..8ad4c4e6d557 100644
--- a/drivers/rtc/rtc-wilco-ec.c
+++ b/drivers/rtc/rtc-wilco-ec.c
@@ -21,8 +21,20 @@
 #define EC_CMOS_TOD_WRITE		0x02
 #define EC_CMOS_TOD_READ		0x08
 
+/* Message sent to the EC to request the current time. */
+struct ec_rtc_read_request {
+	u8 command;
+	u8 reserved;
+	u8 param;
+} __packed;
+static struct ec_rtc_read_request read_rq = {
+	.command = EC_COMMAND_CMOS,
+	.param = EC_CMOS_TOD_READ,
+};
+
 /**
- * struct ec_rtc_read - Format of RTC returned by EC.
+ * struct ec_rtc_read_response - Format of RTC returned by EC.
+ * @reserved: Unused byte
  * @second: Second value (0..59)
  * @minute: Minute value (0..59)
  * @hour: Hour value (0..23)
@@ -33,7 +45,8 @@
  *
  * All values are presented in binary (not BCD).
  */
-struct ec_rtc_read {
+struct ec_rtc_read_response {
+	u8 reserved;
 	u8 second;
 	u8 minute;
 	u8 hour;
@@ -44,8 +57,10 @@ struct ec_rtc_read {
 } __packed;
 
 /**
- * struct ec_rtc_write - Format of RTC sent to the EC.
- * @param: EC_CMOS_TOD_WRITE
+ * struct ec_rtc_write_request - Format of RTC sent to the EC.
+ * @command: Always EC_COMMAND_CMOS
+ * @reserved: Unused byte
+ * @param: Always EC_CMOS_TOD_WRITE
  * @century: Century value (full year / 100)
  * @year: Year value (full year % 100)
  * @month: Month value (1..12)
@@ -57,7 +72,9 @@ struct ec_rtc_read {
  *
  * All values are presented in BCD.
  */
-struct ec_rtc_write {
+struct ec_rtc_write_request {
+	u8 command;
+	u8 reserved;
 	u8 param;
 	u8 century;
 	u8 year;
@@ -72,19 +89,17 @@ struct ec_rtc_write {
 static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
 {
 	struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
-	u8 param = EC_CMOS_TOD_READ;
-	struct ec_rtc_read rtc;
-	struct wilco_ec_message msg = {
-		.type = WILCO_EC_MSG_LEGACY,
-		.flags = WILCO_EC_FLAG_RAW_RESPONSE,
-		.command = EC_COMMAND_CMOS,
-		.request_data = &param,
-		.request_size = sizeof(param),
-		.response_data = &rtc,
-		.response_size = sizeof(rtc),
-	};
+	struct ec_rtc_read_response rtc;
+	struct wilco_ec_message msg;
 	int ret;
 
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &read_rq;
+	msg.request_size = sizeof(read_rq);
+	msg.response_data = &rtc;
+	msg.response_size = sizeof(rtc);
+
 	ret = wilco_ec_mailbox(ec, &msg);
 	if (ret < 0)
 		return ret;
@@ -106,14 +121,8 @@ static int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm)
 static int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm)
 {
 	struct wilco_ec_device *ec = dev_get_drvdata(dev->parent);
-	struct ec_rtc_write rtc;
-	struct wilco_ec_message msg = {
-		.type = WILCO_EC_MSG_LEGACY,
-		.flags = WILCO_EC_FLAG_RAW_RESPONSE,
-		.command = EC_COMMAND_CMOS,
-		.request_data = &rtc,
-		.request_size = sizeof(rtc),
-	};
+	struct ec_rtc_write_request rtc;
+	struct wilco_ec_message msg;
 	int year = tm->tm_year + 1900;
 	/*
 	 * Convert from 0=Sunday to 0=Saturday for the EC
@@ -123,6 +132,7 @@ static int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm)
 	int wday = tm->tm_wday == 6 ? 0 : tm->tm_wday + 1;
 	int ret;
 
+	rtc.command	= EC_COMMAND_CMOS;
 	rtc.param	= EC_CMOS_TOD_WRITE;
 	rtc.century	= bin2bcd(year / 100);
 	rtc.year	= bin2bcd(year % 100);
@@ -133,6 +143,11 @@ static int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm)
 	rtc.second	= bin2bcd(tm->tm_sec);
 	rtc.weekday	= bin2bcd(wday);
 
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &rtc;
+	msg.request_size = sizeof(rtc);
+
 	ret = wilco_ec_mailbox(ec, &msg);
 	if (ret < 0)
 		return ret;
diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
index 446473a46b88..1ff224793c99 100644
--- a/include/linux/platform_data/wilco-ec.h
+++ b/include/linux/platform_data/wilco-ec.h
@@ -14,10 +14,6 @@
 /* Message flags for using the mailbox() interface */
 #define WILCO_EC_FLAG_NO_RESPONSE	BIT(0) /* EC does not respond */
 #define WILCO_EC_FLAG_EXTENDED_DATA	BIT(1) /* EC returns 256 data bytes */
-#define WILCO_EC_FLAG_RAW_REQUEST	BIT(2) /* Do not trim request data */
-#define WILCO_EC_FLAG_RAW_RESPONSE	BIT(3) /* Do not trim response data */
-#define WILCO_EC_FLAG_RAW		(WILCO_EC_FLAG_RAW_REQUEST | \
-					 WILCO_EC_FLAG_RAW_RESPONSE)
 
 /* Normal commands have a maximum 32 bytes of data */
 #define EC_MAILBOX_DATA_SIZE		32
@@ -56,10 +52,7 @@ struct wilco_ec_device {
  * @mailbox_id: Mailbox identifier, specifies the command set.
  * @mailbox_version: Mailbox interface version %EC_MAILBOX_VERSION
  * @reserved: Set to zero.
- * @data_size: Length of request, data + last 2 bytes of the header.
- * @command: Mailbox command code, unique for each mailbox_id set.
- * @reserved_raw: Set to zero for most commands, but is used by
- *                some command types and for raw commands.
+ * @data_size: Length of following data.
  */
 struct wilco_ec_request {
 	u8 struct_version;
@@ -68,8 +61,6 @@ struct wilco_ec_request {
 	u8 mailbox_version;
 	u8 reserved;
 	u16 data_size;
-	u8 command;
-	u8 reserved_raw;
 } __packed;
 
 /**
@@ -79,8 +70,6 @@ struct wilco_ec_request {
  * @result: Result code from the EC.  Non-zero indicates an error.
  * @data_size: Length of the response data buffer.
  * @reserved: Set to zero.
- * @mbox0: EC returned data at offset 0 is unused (always 0) so this byte
- *         is treated as part of the header instead of the data.
  * @data: Response data buffer.  Max size is %EC_MAILBOX_DATA_SIZE_EXTENDED.
  */
 struct wilco_ec_response {
@@ -89,7 +78,6 @@ struct wilco_ec_response {
 	u16 result;
 	u16 data_size;
 	u8 reserved[2];
-	u8 mbox0;
 	u8 data[0];
 } __packed;
 
@@ -111,21 +99,15 @@ enum wilco_ec_msg_type {
  * struct wilco_ec_message - Request and response message.
  * @type: Mailbox message type.
  * @flags: Message flags, e.g. %WILCO_EC_FLAG_NO_RESPONSE.
- * @command: Mailbox command code.
- * @result: Result code from the EC.  Non-zero indicates an error.
  * @request_size: Number of bytes to send to the EC.
  * @request_data: Buffer containing the request data.
- * @response_size: Number of bytes expected from the EC.
- *                 This is 32 by default and 256 if the flag
- *                 is set for %WILCO_EC_FLAG_EXTENDED_DATA
+ * @response_size: Number of bytes to read from EC.
  * @response_data: Buffer containing the response data, should be
  *                 response_size bytes and allocated by caller.
  */
 struct wilco_ec_message {
 	enum wilco_ec_msg_type type;
 	u8 flags;
-	u8 command;
-	u8 result;
 	size_t request_size;
 	void *request_data;
 	size_t response_size;
-- 
2.20.1

[PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Nick Crews]

The EC is in charge of controlling the keyboard backlight on
the Wilco platform. We expose a standard LED class device at
/sys/class/leds/platform::kbd_backlight. This driver is modeled
after the standard Chrome OS keyboard backlight driver at
drivers/platform/chrome/cros_kbd_led_backlight.c

Some Wilco devices do not support a keyboard backlight. This
is checked via wilco_ec_keyboard_leds_exist() in the core driver,
and a platform_device will only be registered by the core if
a backlight is supported.

After an EC reset the backlight could be in a non-PWM mode.
Earlier in the boot sequence the BIOS should send a command to
the EC to set the brightness, so things **should** be set up,
but we double check in probe() as we query the initial brightness.
If not set up, then set the brightness to 0.

Since the EC will never change the backlight level of its own accord,
we don't need to implement a brightness_get() method.

v5 changes:
-Rename the LED device to "platform::kbd_backlight", to
denote that this is the built-in system keyboard.

v4 changes:
-Call keyboard_led_set_brightness() directly within
 initialize_brightness(), instead of calling the library function.

v3 changes:
-Since this behaves the same as the standard Chrome OS keyboard
 backlight, rename the led device to "chromeos::kbd_backlight"
-Move wilco_ec_keyboard_backlight_exists() into core module, so
 that the core does not depend upon the keyboard backlight driver.
-This required moving some code into wilco-ec.h
-Refactor out some common code in set_brightness() and
 initialize_brightness()

v2 changes:
-Remove and fix uses of led vs LED in kconfig
-Assume BIOS initializes brightness, but double check in probe()
-Remove get_brightness() callback, as EC never changes brightness
 by itself.
-Use a __packed struct as message instead of opaque array
-Add exported wilco_ec_keyboard_leds_exist() so the core driver
 now only creates a platform _device if relevant
-Fix use of keyboard_led_set_brightness() since it can sleep

Signed-off-by: Nick Crews <ncrews@chromium.org>
Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>
---
 drivers/platform/chrome/wilco_ec/Kconfig      |   9 +
 drivers/platform/chrome/wilco_ec/Makefile     |   2 +
 drivers/platform/chrome/wilco_ec/core.c       |  58 ++++++
 .../chrome/wilco_ec/kbd_led_backlight.c       | 168 ++++++++++++++++++
 include/linux/platform_data/wilco-ec.h        |  38 ++++
 5 files changed, 275 insertions(+)
 create mode 100644 drivers/platform/chrome/wilco_ec/kbd_led_backlight.c

diff --git a/drivers/platform/chrome/wilco_ec/Kconfig b/drivers/platform/chrome/wilco_ec/Kconfig
index e09e4cebe9b4..d8cf4a60d1b5 100644
--- a/drivers/platform/chrome/wilco_ec/Kconfig
+++ b/drivers/platform/chrome/wilco_ec/Kconfig
@@ -18,3 +18,12 @@ config WILCO_EC_DEBUGFS
 	  manipulation and allow for testing arbitrary commands.  This
 	  interface is intended for debug only and will not be present
 	  on production devices.
+
+config WILCO_EC_KBD_BACKLIGHT
+	tristate "Enable keyboard backlight control"
+	depends on WILCO_EC
+	help
+	  If you say Y here, you get support to set the keyboard backlight
+	  brightness. This happens via a standard LED driver that uses the
+	  Wilco EC mailbox interface. A standard LED class device will
+	  appear under /sys/class/leds/chromeos::kbd_backlight
diff --git a/drivers/platform/chrome/wilco_ec/Makefile b/drivers/platform/chrome/wilco_ec/Makefile
index 063e7fb4ea17..8436539813cd 100644
--- a/drivers/platform/chrome/wilco_ec/Makefile
+++ b/drivers/platform/chrome/wilco_ec/Makefile
@@ -4,3 +4,5 @@ wilco_ec-objs				:= core.o mailbox.o
 obj-$(CONFIG_WILCO_EC)			+= wilco_ec.o
 wilco_ec_debugfs-objs			:= debugfs.o
 obj-$(CONFIG_WILCO_EC_DEBUGFS)		+= wilco_ec_debugfs.o
+wilco_kbd_backlight-objs		:= kbd_led_backlight.o
+obj-$(CONFIG_WILCO_EC_KBD_BACKLIGHT)	+= wilco_kbd_backlight.o
diff --git a/drivers/platform/chrome/wilco_ec/core.c b/drivers/platform/chrome/wilco_ec/core.c
index 05e1e2be1c91..3c45c157b7da 100644
--- a/drivers/platform/chrome/wilco_ec/core.c
+++ b/drivers/platform/chrome/wilco_ec/core.c
@@ -38,11 +38,47 @@ static struct resource *wilco_get_resource(struct platform_device *pdev,
 				   dev_name(dev));
 }
 
+/**
+ * wilco_ec_keyboard_backlight_exists() - Is the keyboad backlight supported?
+ * @ec: EC device to query.
+ * @exists: Return value to fill in.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+static int wilco_ec_keyboard_backlight_exists(struct wilco_ec_device *ec,
+					      bool *exists)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	struct wilco_ec_message msg;
+	int ret;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_FEATURES;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &request;
+	msg.request_size = sizeof(request);
+	msg.response_data = &response;
+	msg.response_size = sizeof(response);
+
+	ret = wilco_ec_mailbox(ec, &msg);
+	if (ret < 0)
+		return ret;
+
+	*exists = response.status != 0xFF;
+
+	return 0;
+}
+
 static int wilco_ec_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct wilco_ec_device *ec;
 	int ret;
+	bool kbbl_exists;
 
 	ec = devm_kzalloc(dev, sizeof(*ec), GFP_KERNEL);
 	if (!ec)
@@ -89,8 +125,29 @@ static int wilco_ec_probe(struct platform_device *pdev)
 		goto unregister_debugfs;
 	}
 
+	/* Register child dev to be found by the keyboard backlight driver. */
+	ret = wilco_ec_keyboard_backlight_exists(ec, &kbbl_exists);
+	if (ret) {
+		dev_err(ec->dev,
+			"Failed checking keyboard backlight support: %d", ret);
+		goto unregister_rtc;
+	}
+	if (kbbl_exists) {
+		ec->kbbl_pdev = platform_device_register_data(dev,
+						"wilco-kbd-backlight",
+						PLATFORM_DEVID_AUTO, NULL, 0);
+		if (IS_ERR(ec->kbbl_pdev)) {
+			dev_err(dev,
+				"Failed to create keyboard backlight pdev\n");
+			ret = PTR_ERR(ec->kbbl_pdev);
+			goto unregister_rtc;
+		}
+	}
+
 	return 0;
 
+unregister_rtc:
+	platform_device_unregister(ec->rtc_pdev);
 unregister_debugfs:
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
@@ -102,6 +159,7 @@ static int wilco_ec_remove(struct platform_device *pdev)
 {
 	struct wilco_ec_device *ec = platform_get_drvdata(pdev);
 
+	platform_device_unregister(ec->kbbl_pdev);
 	platform_device_unregister(ec->rtc_pdev);
 	if (ec->debugfs_pdev)
 		platform_device_unregister(ec->debugfs_pdev);
diff --git a/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
new file mode 100644
index 000000000000..8fa4b10b6392
--- /dev/null
+++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Keyboard backlight LED driver for the Wilco Embedded Controller
+ *
+ * Copyright 2019 Google LLC
+ *
+ * The EC is in charge of controlling the keyboard backlight on
+ * the Wilco platform. We expose a standard LED class device at
+ * /sys/class/leds/platform::kbd_backlight. Power Manager normally
+ * controls the backlight by writing a percentage in range [0, 100]
+ * to the brightness property. This driver is modeled after the
+ * standard Chrome OS keyboard backlight driver at
+ * drivers/platform/chrome/cros_kbd_led_backlight.c
+ *
+ * Some Wilco devices do not support a keyboard backlight. This
+ * is checked via wilco_ec_keyboard_backlight_exists() in the core driver,
+ * and a platform_device will only be registered by the core if
+ * a backlight is supported.
+ *
+ * After an EC reset the backlight could be in a non-PWM mode.
+ * Earlier in the boot sequence the BIOS should send a command to
+ * the EC to set the brightness, so things **should** be set up,
+ * but we double check in probe() as we query the initial brightness.
+ * If not set up, then we set the brightness to KBBL_DEFAULT_BRIGHTNESS.
+ *
+ * Since the EC will never change the backlight level of its own accord,
+ * we don't need to implement a brightness_get() method.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_data/wilco-ec.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define DRV_NAME		"wilco-kbd-backlight"
+
+#define KBBL_DEFAULT_BRIGHTNESS	0
+
+struct wilco_keyboard_led_data {
+	struct wilco_ec_device *ec;
+	struct led_classdev keyboard;
+};
+
+/* Send a request, get a response, and check that the response is good. */
+static int send_kbbl_msg(struct wilco_ec_device *ec,
+			 const struct wilco_ec_kbbl_msg *request,
+			 struct wilco_ec_kbbl_msg *response)
+{
+	struct wilco_ec_message msg;
+	int ret;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.type = WILCO_EC_MSG_LEGACY;
+	msg.request_data = &request;
+	msg.request_size = sizeof(request);
+	msg.response_data = &response;
+	msg.response_size = sizeof(response);
+
+	ret = wilco_ec_mailbox(ec, &msg);
+	if (ret < 0)
+		dev_err(ec->dev, "Failed sending brightness command: %d", ret);
+
+	if (response->status) {
+		dev_err(ec->dev,
+			"EC reported failure sending brightness command: %d",
+			response->status);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+/* This may sleep because it uses wilco_ec_mailbox() */
+static int keyboard_led_set_brightness(struct led_classdev *cdev,
+				       enum led_brightness brightness)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	struct wilco_keyboard_led_data *data;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_STATE;
+	request.mode = WILCO_KBBL_MODE_FLAG_PWM;
+	request.percent = brightness;
+
+	data = container_of(cdev, struct wilco_keyboard_led_data, keyboard);
+	return send_kbbl_msg(data->ec, &request, &response);
+}
+
+/*
+ * Get the current brightness, ensuring that we are in PWM mode. If not
+ * in PWM mode, then the current brightness is meaningless, so set the
+ * brightness to KBBL_DEFAULT_BRIGHTNESS.
+ *
+ * Return: Final brightness of the keyboard, or negative error code on failure.
+ */
+static int initialize_brightness(struct wilco_keyboard_led_data *data)
+{
+	struct wilco_ec_kbbl_msg request;
+	struct wilco_ec_kbbl_msg response;
+	int ret;
+
+	memset(&request, 0, sizeof(request));
+	request.command = WILCO_EC_COMMAND_KBBL;
+	request.subcmd = WILCO_KBBL_SUBCMD_GET_STATE;
+
+	ret = send_kbbl_msg(data->ec, &request, &response);
+	if (ret < 0)
+		return ret;
+
+	if (response.mode & WILCO_KBBL_MODE_FLAG_PWM)
+		return response.percent;
+
+	dev_warn(data->ec->dev, "Keyboard brightness not initialized by BIOS");
+	ret = keyboard_led_set_brightness(&data->keyboard,
+					  KBBL_DEFAULT_BRIGHTNESS);
+	if (ret < 0)
+		return ret;
+
+	return KBBL_DEFAULT_BRIGHTNESS;
+}
+
+static int keyboard_led_probe(struct platform_device *pdev)
+{
+	struct wilco_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct wilco_keyboard_led_data *data;
+	int ret;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->ec = ec;
+	/* This acts the same as the CrOS backlight, so use the same name */
+	data->keyboard.name = "platform::kbd_backlight";
+	data->keyboard.max_brightness = 100;
+	data->keyboard.flags = LED_CORE_SUSPENDRESUME;
+	data->keyboard.brightness_set_blocking = keyboard_led_set_brightness;
+	ret = initialize_brightness(data);
+	if (ret < 0)
+		return ret;
+	data->keyboard.brightness = ret;
+
+	ret = devm_led_classdev_register(&pdev->dev, &data->keyboard);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static struct platform_driver keyboard_led_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.probe = keyboard_led_probe,
+};
+module_platform_driver(keyboard_led_driver);
+
+MODULE_AUTHOR("Nick Crews <ncrews@chromium.org>");
+MODULE_DESCRIPTION("Wilco keyboard backlight LED driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
diff --git a/include/linux/platform_data/wilco-ec.h b/include/linux/platform_data/wilco-ec.h
index 1ff224793c99..c3965b7f397d 100644
--- a/include/linux/platform_data/wilco-ec.h
+++ b/include/linux/platform_data/wilco-ec.h
@@ -32,6 +32,7 @@
  * @data_size: Size of the data buffer used for EC communication.
  * @debugfs_pdev: The child platform_device used by the debugfs sub-driver.
  * @rtc_pdev: The child platform_device used by the RTC sub-driver.
+ * @kbbl_pdev: The child pdev used by the keyboard backlight sub-driver.
  */
 struct wilco_ec_device {
 	struct device *dev;
@@ -43,6 +44,7 @@ struct wilco_ec_device {
 	size_t data_size;
 	struct platform_device *debugfs_pdev;
 	struct platform_device *rtc_pdev;
+	struct platform_device *kbbl_pdev;
 };
 
 /**
@@ -114,6 +116,42 @@ struct wilco_ec_message {
 	void *response_data;
 };
 
+/* Constants and structs useful for keyboard backlight (KBBL) control */
+
+#define WILCO_EC_COMMAND_KBBL		0x75
+#define WILCO_KBBL_MODE_FLAG_PWM	BIT(1)	/* Set brightness by percent. */
+
+/**
+ * enum kbbl_subcommand - What action does the EC perform?
+ * @WILCO_KBBL_SUBCMD_GET_FEATURES: Request available functionality from EC.
+ * @WILCO_KBBL_SUBCMD_GET_STATE: Request current mode and brightness from EC.
+ * @WILCO_KBBL_SUBCMD_SET_STATE: Write mode and brightness to EC.
+ */
+enum kbbl_subcommand {
+	WILCO_KBBL_SUBCMD_GET_FEATURES = 0x00,
+	WILCO_KBBL_SUBCMD_GET_STATE = 0x01,
+	WILCO_KBBL_SUBCMD_SET_STATE = 0x02,
+};
+
+/**
+ * struct wilco_ec_kbbl_msg - Message to/from EC for keyboard backlight control.
+ * @command: Always WILCO_EC_COMMAND_KBBL.
+ * @status: Set by EC to 0 on success, 0xFF on failure.
+ * @subcmd: One of enum kbbl_subcommand.
+ * @mode: Bit flags for used mode, we want to use WILCO_KBBL_MODE_FLAG_PWM.
+ * @percent: Brightness in 0-100. Only meaningful in PWM mode.
+ */
+struct wilco_ec_kbbl_msg {
+	u8 command;
+	u8 status;
+	u8 subcmd;
+	u8 reserved3;
+	u8 mode;
+	u8 reserved5to8[4];
+	u8 percent;
+	u8 reserved10to15[6];
+} __packed;
+
 /**
  * wilco_ec_mailbox() - Send request to the EC and receive the response.
  * @ec: Wilco EC device.
-- 
2.20.1

[PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Nick Crews]

We want all backlights for the system keyboard to
use a common name, so the name "platform::kbd_backlight"
would be better than the current "chromeos::kbd_backlight"
name. Normally this wouldn't be worth changing, but the new
Wilco keyboard backlight driver uses the "platform" name.
We want to make it so all Chrome OS devices are consistent,
so we'll change the name here too.

The Power Manager daemon only looks for LEDs that match the
pattern "*:kbd_backlight", so this change won't affect that.

Signed-off-by: Nick Crews <ncrews@chromium.org>
---
 drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
index aa409f0201fb..c56c8405f39c 100644
--- a/drivers/platform/chrome/cros_kbd_led_backlight.c
+++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
@@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
 	if (!cdev)
 		return -ENOMEM;
 
-	cdev->name = "chromeos::kbd_backlight";
+	cdev->name = "platform::kbd_backlight";
 	cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
 	cdev->flags |= LED_CORE_SUSPENDRESUME;
 	cdev->brightness_set = keyboard_led_set_brightness;
-- 
2.20.1

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Guenter Roeck]

On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
>
> We want all backlights for the system keyboard to
> use a common name, so the name "platform::kbd_backlight"
> would be better than the current "chromeos::kbd_backlight"
> name. Normally this wouldn't be worth changing, but the new
> Wilco keyboard backlight driver uses the "platform" name.
> We want to make it so all Chrome OS devices are consistent,
> so we'll change the name here too.
>

Wondering - who is the "we" you are talking about ?

Guenter

> The Power Manager daemon only looks for LEDs that match the
> pattern "*:kbd_backlight", so this change won't affect that.
>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index aa409f0201fb..c56c8405f39c 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
>         if (!cdev)
>                 return -ENOMEM;
>
> -       cdev->name = "chromeos::kbd_backlight";
> +       cdev->name = "platform::kbd_backlight";
>         cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
>         cdev->flags |= LED_CORE_SUSPENDRESUME;
>         cdev->brightness_set = keyboard_led_set_brightness;
> --
> 2.20.1
>

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
>
> On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> >
> > We want all backlights for the system keyboard to
> > use a common name, so the name "platform::kbd_backlight"
> > would be better than the current "chromeos::kbd_backlight"
> > name. Normally this wouldn't be worth changing, but the new
> > Wilco keyboard backlight driver uses the "platform" name.
> > We want to make it so all Chrome OS devices are consistent,
> > so we'll change the name here too.
> >
>
> Wondering - who is the "we" you are talking about ?

This also has a potential of breaking existing setups if somebody did
happen to match on entire name instead of suffix. Such changes have to
be considered very carefully; at this point I am against of doing
this.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Nick Crews]

On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
>
> On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> >
> > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > >
> > > We want all backlights for the system keyboard to
> > > use a common name, so the name "platform::kbd_backlight"
> > > would be better than the current "chromeos::kbd_backlight"
> > > name. Normally this wouldn't be worth changing, but the new
> > > Wilco keyboard backlight driver uses the "platform" name.
> > > We want to make it so all Chrome OS devices are consistent,
> > > so we'll change the name here too.
> > >
> >
> > Wondering - who is the "we" you are talking about ?

You're right, I should have been more precise.
I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
in that comment, but I would guess that could mean the other LED maintainers
as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
though perhaps he was only considering our use of the LED via powerd,
and not users in general. I'm guessing Pavel's and Enric's meanings though,
excuse me if I am misinterpreting.

>
> This also has a potential of breaking existing setups if somebody did
> happen to match on entire name instead of suffix. Such changes have to
> be considered very carefully; at this point I am against of doing
> this.

Would it make sense to keep the old name as is, and only make the new
Wilco name begin with "platform:"? What would you think is best?

>
> Thanks.
>
> --
> Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote:
>
> On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
> >
> > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> > >
> > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > > >
> > > > We want all backlights for the system keyboard to
> > > > use a common name, so the name "platform::kbd_backlight"
> > > > would be better than the current "chromeos::kbd_backlight"
> > > > name. Normally this wouldn't be worth changing, but the new
> > > > Wilco keyboard backlight driver uses the "platform" name.
> > > > We want to make it so all Chrome OS devices are consistent,
> > > > so we'll change the name here too.
> > > >
> > >
> > > Wondering - who is the "we" you are talking about ?
>
> You're right, I should have been more precise.
> I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> in that comment, but I would guess that could mean the other LED maintainers
> as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> though perhaps he was only considering our use of the LED via powerd,
> and not users in general. I'm guessing Pavel's and Enric's meanings though,
> excuse me if I am misinterpreting.
>
> >
> > This also has a potential of breaking existing setups if somebody did
> > happen to match on entire name instead of suffix. Such changes have to
> > be considered very carefully; at this point I am against of doing
> > this.
>
> Would it make sense to keep the old name as is, and only make the new
> Wilco name begin with "platform:"? What would you think is best?

Given that we do not have a single instance of platform::kbd_backlight
in kernel at this time I have no idea why Pavel is trying to push this
for Wilco driver.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2919 bytes --]

Hi!

> You're right, I should have been more precise.
> I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> in that comment, but I would guess that could mean the other LED maintainers
> as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> though perhaps he was only considering our use of the LED via powerd,
> and not users in general. I'm guessing Pavel's and Enric's meanings though,
> excuse me if I am misinterpreting.
> 
> >
> > This also has a potential of breaking existing setups if somebody did
> > happen to match on entire name instead of suffix. Such changes have to
> > be considered very carefully; at this point I am against of doing
> > this.
> 
> Would it make sense to keep the old name as is, and only make the new
> Wilco name begin with "platform:"? What would you think is best?

That works for me.

I think we might be able to get away with changing the name (we have
some really bad names in the LED subsystem, and it would be nice to
fix them), but my priority at the moment is not to extend the problem.

								Pavel

-*- org -*-

It is somehow important to provide consistent interface to the
userland. LED devices have one problem there, and that is naming of
directories in /sys/class/leds. It would be nice if userland would
just know right "name" for given LED function, but situation got more
complex.

Anyway, if backwards compatibility is not an issue, new code should
use one of the "good" names from this list, and you should extend the
list where applicable.

Bad names are listed, too; in case you are writing application that
wants to use particular feature, you should probe for good name, first,
but then try the bad ones, too.

* Keyboards

Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)

Set of common keyboard LEDs, going back to PC AT or so.

Good: "platform::kbd_backlight"
Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

Frontlight/backlight of main keyboard.

Bad: "button-backlight" (Motorola Droid 4)

Some phones have touch buttons below screen; it is different from main
keyboard. And this is their backlight.

* Sound subsystem

Good: "platform:*:mute"
Good: "platform:*:micmute"

LEDs on notebook body, indicating that sound input / output is muted.

* System notification

Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)

Phones usually have multi-color status LED.

* Power management

Good: "platform:*:charging" (allwinner sun50i)


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote:
> On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote:
> >
> > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
> > >
> > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> > > >
> > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > > > >
> > > > > We want all backlights for the system keyboard to
> > > > > use a common name, so the name "platform::kbd_backlight"
> > > > > would be better than the current "chromeos::kbd_backlight"
> > > > > name. Normally this wouldn't be worth changing, but the new
> > > > > Wilco keyboard backlight driver uses the "platform" name.
> > > > > We want to make it so all Chrome OS devices are consistent,
> > > > > so we'll change the name here too.
> > > > >
> > > >
> > > > Wondering - who is the "we" you are talking about ?
> >
> > You're right, I should have been more precise.
> > I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> > in that comment, but I would guess that could mean the other LED maintainers
> > as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> > though perhaps he was only considering our use of the LED via powerd,
> > and not users in general. I'm guessing Pavel's and Enric's meanings though,
> > excuse me if I am misinterpreting.
> >
> > >
> > > This also has a potential of breaking existing setups if somebody did
> > > happen to match on entire name instead of suffix. Such changes have to
> > > be considered very carefully; at this point I am against of doing
> > > this.
> >
> > Would it make sense to keep the old name as is, and only make the new
> > Wilco name begin with "platform:"? What would you think is best?
> 
> Given that we do not have a single instance of platform::kbd_backlight
> in kernel at this time I have no idea why Pavel is trying to push this
> for Wilco driver.

See the documentation in the email I sent few seconds ago. I hope it
explains my reasoning, if not, I'll explain it.

								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote:
> > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote:
> > >
> > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
> > > >
> > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> > > > >
> > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > > > > >
> > > > > > We want all backlights for the system keyboard to
> > > > > > use a common name, so the name "platform::kbd_backlight"
> > > > > > would be better than the current "chromeos::kbd_backlight"
> > > > > > name. Normally this wouldn't be worth changing, but the new
> > > > > > Wilco keyboard backlight driver uses the "platform" name.
> > > > > > We want to make it so all Chrome OS devices are consistent,
> > > > > > so we'll change the name here too.
> > > > > >
> > > > >
> > > > > Wondering - who is the "we" you are talking about ?
> > >
> > > You're right, I should have been more precise.
> > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> > > in that comment, but I would guess that could mean the other LED maintainers
> > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> > > though perhaps he was only considering our use of the LED via powerd,
> > > and not users in general. I'm guessing Pavel's and Enric's meanings though,
> > > excuse me if I am misinterpreting.
> > >
> > > >
> > > > This also has a potential of breaking existing setups if somebody did
> > > > happen to match on entire name instead of suffix. Such changes have to
> > > > be considered very carefully; at this point I am against of doing
> > > > this.
> > >
> > > Would it make sense to keep the old name as is, and only make the new
> > > Wilco name begin with "platform:"? What would you think is best?
> >
> > Given that we do not have a single instance of platform::kbd_backlight
> > in kernel at this time I have no idea why Pavel is trying to push this
> > for Wilco driver.
>
> See the documentation in the email I sent few seconds ago. I hope it
> explains my reasoning, if not, I'll explain it.

Yes, I see the doc and I do not think I agree with it. If you look at
the LED docs you will see:

LED Device Naming
=================

Is currently of the form:

"devicename:colour:function"

It is *function* and maybe color that userspace is interested in, and
here we have proper standardization in form of "kbd_backlight". Device
name is, well, device name. It should uniquely identify the device led
is attached to, but otherwise is rarely interesting. If userspace is
really concerned what kind of keyboard backlight it is it should
investigate parent device(s) and see what they end up with.

Thanks.

--
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]

On Thu 2019-04-04 12:05:39, Dmitry Torokhov wrote:
> On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote:
> > > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote:
> > > >
> > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
> > > > >
> > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> > > > > >
> > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > > > > > >
> > > > > > > We want all backlights for the system keyboard to
> > > > > > > use a common name, so the name "platform::kbd_backlight"
> > > > > > > would be better than the current "chromeos::kbd_backlight"
> > > > > > > name. Normally this wouldn't be worth changing, but the new
> > > > > > > Wilco keyboard backlight driver uses the "platform" name.
> > > > > > > We want to make it so all Chrome OS devices are consistent,
> > > > > > > so we'll change the name here too.
> > > > > > >
> > > > > >
> > > > > > Wondering - who is the "we" you are talking about ?
> > > >
> > > > You're right, I should have been more precise.
> > > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> > > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> > > > in that comment, but I would guess that could mean the other LED maintainers
> > > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> > > > though perhaps he was only considering our use of the LED via powerd,
> > > > and not users in general. I'm guessing Pavel's and Enric's meanings though,
> > > > excuse me if I am misinterpreting.
> > > >
> > > > >
> > > > > This also has a potential of breaking existing setups if somebody did
> > > > > happen to match on entire name instead of suffix. Such changes have to
> > > > > be considered very carefully; at this point I am against of doing
> > > > > this.
> > > >
> > > > Would it make sense to keep the old name as is, and only make the new
> > > > Wilco name begin with "platform:"? What would you think is best?
> > >
> > > Given that we do not have a single instance of platform::kbd_backlight
> > > in kernel at this time I have no idea why Pavel is trying to push this
> > > for Wilco driver.
> >
> > See the documentation in the email I sent few seconds ago. I hope it
> > explains my reasoning, if not, I'll explain it.
> 
> Yes, I see the doc and I do not think I agree with it. If you look at
> the LED docs you will see:

If you take a look at mailing list archive, this is currently being
worked on. And you might want to take a look at MAINTAINERS file :-)

> LED Device Naming
> =================
> 
> Is currently of the form:
> 
> "devicename:colour:function"
> 
> It is *function* and maybe color that userspace is interested in, and
> here we have proper standardization in form of "kbd_backlight". Device
> name is, well, device name. It should uniquely identify the device led
> is attached to, but otherwise is rarely interesting. If userspace is
> really concerned what kind of keyboard backlight it is it should
> investigate parent device(s) and see what they end up with.

That does not work. Userspace wants to know if it is internal keyboard
or USB keyboard, not what kind of i2c controller the LED is connected
to.

grep for platform::mic_mute .

(And platform is even pretty good match for how the LED is connected
in your case).
									Pavel


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1332 bytes --]

On Thu 2019-04-04 11:10:08, Nick Crews wrote:
> The EC is in charge of controlling the keyboard backlight on
> the Wilco platform. We expose a standard LED class device at
> /sys/class/leds/platform::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
> 
> Some Wilco devices do not support a keyboard backlight. This
> is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> and a platform_device will only be registered by the core if
> a backlight is supported.
> 
> After an EC reset the backlight could be in a non-PWM mode.
> Earlier in the boot sequence the BIOS should send a command to
> the EC to set the brightness, so things **should** be set up,
> but we double check in probe() as we query the initial brightness.
> If not set up, then set the brightness to 0.
> 
> Since the EC will never change the backlight level of its own accord,
> we don't need to implement a brightness_get() method.
...
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 12:19 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2019-04-04 12:05:39, Dmitry Torokhov wrote:
> > On Thu, Apr 4, 2019 at 11:59 AM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > On Thu 2019-04-04 11:55:27, Dmitry Torokhov wrote:
> > > > On Thu, Apr 4, 2019 at 11:41 AM Nick Crews <ncrews@chromium.org> wrote:
> > > > >
> > > > > On Thu, Apr 4, 2019 at 11:43 AM Dmitry Torokhov <dtor@google.com> wrote:
> > > > > >
> > > > > > On Thu, Apr 4, 2019 at 10:36 AM Guenter Roeck <groeck@google.com> wrote:
> > > > > > >
> > > > > > > On Thu, Apr 4, 2019 at 10:11 AM Nick Crews <ncrews@chromium.org> wrote:
> > > > > > > >
> > > > > > > > We want all backlights for the system keyboard to
> > > > > > > > use a common name, so the name "platform::kbd_backlight"
> > > > > > > > would be better than the current "chromeos::kbd_backlight"
> > > > > > > > name. Normally this wouldn't be worth changing, but the new
> > > > > > > > Wilco keyboard backlight driver uses the "platform" name.
> > > > > > > > We want to make it so all Chrome OS devices are consistent,
> > > > > > > > so we'll change the name here too.
> > > > > > > >
> > > > > > >
> > > > > > > Wondering - who is the "we" you are talking about ?
> > > > >
> > > > > You're right, I should have been more precise.
> > > > > I was referring to Pavel, Enric, and myself. Pavel had this opinion here:
> > > > > https://lkml.org/lkml/2019/4/4/1040. I don't know what Pavel meant by "we"
> > > > > in that comment, but I would guess that could mean the other LED maintainers
> > > > > as well? I talked with Enric 1:1 and he didn't see the problem with changing it,
> > > > > though perhaps he was only considering our use of the LED via powerd,
> > > > > and not users in general. I'm guessing Pavel's and Enric's meanings though,
> > > > > excuse me if I am misinterpreting.
> > > > >
> > > > > >
> > > > > > This also has a potential of breaking existing setups if somebody did
> > > > > > happen to match on entire name instead of suffix. Such changes have to
> > > > > > be considered very carefully; at this point I am against of doing
> > > > > > this.
> > > > >
> > > > > Would it make sense to keep the old name as is, and only make the new
> > > > > Wilco name begin with "platform:"? What would you think is best?
> > > >
> > > > Given that we do not have a single instance of platform::kbd_backlight
> > > > in kernel at this time I have no idea why Pavel is trying to push this
> > > > for Wilco driver.
> > >
> > > See the documentation in the email I sent few seconds ago. I hope it
> > > explains my reasoning, if not, I'll explain it.
> >
> > Yes, I see the doc and I do not think I agree with it. If you look at
> > the LED docs you will see:
>
> If you take a look at mailing list archive, this is currently being
> worked on. And you might want to take a look at MAINTAINERS file :-)

So? Being maintainer does not give you free reign to merge stuff that
does not quite make sense, you still need to discuss it and convince
people.

>
> > LED Device Naming
> > =================
> >
> > Is currently of the form:
> >
> > "devicename:colour:function"
> >
> > It is *function* and maybe color that userspace is interested in, and
> > here we have proper standardization in form of "kbd_backlight". Device
> > name is, well, device name. It should uniquely identify the device led
> > is attached to, but otherwise is rarely interesting. If userspace is
> > really concerned what kind of keyboard backlight it is it should
> > investigate parent device(s) and see what they end up with.
>
> That does not work. Userspace wants to know if it is internal keyboard
> or USB keyboard, not what kind of i2c controller the LED is connected
> to.

Why does userspace want to know it? What does it do differently with
backlit external keyboards vs internal ones? And how does it decide if
USB keyboard is internal or not given that there are devices with
internal USB keyboards?

>
> grep for platform::mic_mute .
>
> (And platform is even pretty good match for how the LED is connected
> in your case).

Until we get a secondary interface that is also "platform"...

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1131 bytes --]

Hi!

> > > It is *function* and maybe color that userspace is interested in, and
> > > here we have proper standardization in form of "kbd_backlight". Device
> > > name is, well, device name. It should uniquely identify the device led
> > > is attached to, but otherwise is rarely interesting. If userspace is
> > > really concerned what kind of keyboard backlight it is it should
> > > investigate parent device(s) and see what they end up with.
> >
> > That does not work. Userspace wants to know if it is internal keyboard
> > or USB keyboard, not what kind of i2c controller the LED is connected
> > to.
> 
> Why does userspace want to know it?

For example to turn off backlight on internal keyboard when the lid is closed.

> > grep for platform::mic_mute .
> >
> > (And platform is even pretty good match for how the LED is connected
> > in your case).
> 
> Until we get a secondary interface that is also "platform"...

How would that happen?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > It is *function* and maybe color that userspace is interested in, and
> > > > here we have proper standardization in form of "kbd_backlight". Device
> > > > name is, well, device name. It should uniquely identify the device led
> > > > is attached to, but otherwise is rarely interesting. If userspace is
> > > > really concerned what kind of keyboard backlight it is it should
> > > > investigate parent device(s) and see what they end up with.
> > >
> > > That does not work. Userspace wants to know if it is internal keyboard
> > > or USB keyboard, not what kind of i2c controller the LED is connected
> > > to.
> >
> > Why does userspace want to know it?
>
> For example to turn off backlight on internal keyboard when the lid is closed.

Would you not want to turn off external as well?

And what to do if internal keyboard is not platform but USB? Like
Google "Whiskers"? (I am not sure why you decided to drop my mention
of internal USB keyboards completely off your reply).

>
> > > grep for platform::mic_mute .
> > >
> > > (And platform is even pretty good match for how the LED is connected
> > > in your case).
> >
> > Until we get a secondary interface that is also "platform"...
>
> How would that happen?

It won't happen on Wilco, but you can't imagine that several blocks
get reused in the same device and they end up clashing?

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2270 bytes --]

On Thu 2019-04-04 13:13:34, Dmitry Torokhov wrote:
> On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > > > It is *function* and maybe color that userspace is interested in, and
> > > > > here we have proper standardization in form of "kbd_backlight". Device
> > > > > name is, well, device name. It should uniquely identify the device led
> > > > > is attached to, but otherwise is rarely interesting. If userspace is
> > > > > really concerned what kind of keyboard backlight it is it should
> > > > > investigate parent device(s) and see what they end up with.
> > > >
> > > > That does not work. Userspace wants to know if it is internal keyboard
> > > > or USB keyboard, not what kind of i2c controller the LED is connected
> > > > to.
> > >
> > > Why does userspace want to know it?
> >
> > For example to turn off backlight on internal keyboard when the lid is closed.
> 
> Would you not want to turn off external as well?

Not really. If I'm using machine with lid closed, external monitor and
USB keyboard attached, I want to control backlight of USB keyboard,
but backlight of internal keyboard can stay off.

> And what to do if internal keyboard is not platform but USB? Like
> Google "Whiskers"? (I am not sure why you decided to drop my mention
> of internal USB keyboards completely off your reply).

I don't have answers for everything. Even if you have USB keyboard, you'll
likely still have backlight connected to embedded controller. If not,
then maybe you have exception userland needs to know about.

Still better than making everything an exception.

> > > > grep for platform::mic_mute .
> > > >
> > > > (And platform is even pretty good match for how the LED is connected
> > > > in your case).
> > >
> > > Until we get a secondary interface that is also "platform"...
> >
> > How would that happen?
> 
> It won't happen on Wilco, but you can't imagine that several blocks
> get reused in the same device and they end up clashing?

In the end, there will be just one internal keyboard backlight, right?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 1:20 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2019-04-04 13:13:34, Dmitry Torokhov wrote:
> > On Thu, Apr 4, 2019 at 1:06 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > Hi!
> > >
> > > > > > It is *function* and maybe color that userspace is interested in, and
> > > > > > here we have proper standardization in form of "kbd_backlight". Device
> > > > > > name is, well, device name. It should uniquely identify the device led
> > > > > > is attached to, but otherwise is rarely interesting. If userspace is
> > > > > > really concerned what kind of keyboard backlight it is it should
> > > > > > investigate parent device(s) and see what they end up with.
> > > > >
> > > > > That does not work. Userspace wants to know if it is internal keyboard
> > > > > or USB keyboard, not what kind of i2c controller the LED is connected
> > > > > to.
> > > >
> > > > Why does userspace want to know it?
> > >
> > > For example to turn off backlight on internal keyboard when the lid is closed.
> >
> > Would you not want to turn off external as well?
>
> Not really. If I'm using machine with lid closed, external monitor and
> USB keyboard attached, I want to control backlight of USB keyboard,
> but backlight of internal keyboard can stay off.
>
> > And what to do if internal keyboard is not platform but USB? Like
> > Google "Whiskers"? (I am not sure why you decided to drop my mention
> > of internal USB keyboards completely off your reply).
>
> I don't have answers for everything. Even if you have USB keyboard, you'll
> likely still have backlight connected to embedded controller. If not,
> then maybe you have exception userland needs to know about.
>
> Still better than making everything an exception.

You do not need to make everything exception. You just need to look
beyond the name, and see how the device is connected. And then apply
your exceptions for "weird" devices.

>
> > > > > grep for platform::mic_mute .
> > > > >
> > > > > (And platform is even pretty good match for how the LED is connected
> > > > > in your case).
> > > >
> > > > Until we get a secondary interface that is also "platform"...
> > >
> > > How would that happen?
> >
> > It won't happen on Wilco, but you can't imagine that several blocks
> > get reused in the same device and they end up clashing?
>
> In the end, there will be just one internal keyboard backlight, right?

¯\_(ツ)_/¯ Maybe. Maybe not. Who knows what HW designers will come up with.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1377 bytes --]

Hi!

> > > And what to do if internal keyboard is not platform but USB? Like
> > > Google "Whiskers"? (I am not sure why you decided to drop my mention
> > > of internal USB keyboards completely off your reply).
> >
> > I don't have answers for everything. Even if you have USB keyboard, you'll
> > likely still have backlight connected to embedded controller. If not,
> > then maybe you have exception userland needs to know about.
> >
> > Still better than making everything an exception.
> 
> You do not need to make everything exception. You just need to look
> beyond the name, and see how the device is connected. And then apply
> your exceptions for "weird" devices.

"Where it is connected" is not interesting to the userland. "Is it
backlight for internal keyboard" is the right question. It may be
connected to embedded controller or some kind of controller over
i2c... my shell scripts should not need to know about architecture of
every notebook out there.

But I don't see why I should do additional work when its trivial for
kernel to just name the LED in an useful way.

"platform::kbd_backlight" has no disadvantages compared to
"wilco::kbd_backlight" ... so lets just use it.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > And what to do if internal keyboard is not platform but USB? Like
> > > > Google "Whiskers"? (I am not sure why you decided to drop my mention
> > > > of internal USB keyboards completely off your reply).
> > >
> > > I don't have answers for everything. Even if you have USB keyboard, you'll
> > > likely still have backlight connected to embedded controller. If not,
> > > then maybe you have exception userland needs to know about.
> > >
> > > Still better than making everything an exception.
> >
> > You do not need to make everything exception. You just need to look
> > beyond the name, and see how the device is connected. And then apply
> > your exceptions for "weird" devices.
>
> "Where it is connected" is not interesting to the userland. "Is it
> backlight for internal keyboard" is the right question. It may be
> connected to embedded controller or some kind of controller over
> i2c... my shell scripts should not need to know about architecture of
> every notebook out there.

Then your scripts will be failing for some setups.

>
> But I don't see why I should do additional work when its trivial for
> kernel to just name the LED in an useful way.
>
> "platform::kbd_backlight" has no disadvantages compared to
> "wilco::kbd_backlight" ... so lets just use it.

It has disadvantages because it promises more than it can deliver IMO.
If device name != "platform::kbd_backlight" it does not mean that it
is not internal keyboard. And you still have not resolved how you will
handle cases when there is more than one deice that can be considered
internal and may have a backlight.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2322 bytes --]

On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
> >
> > Hi!
> >
> > > > > And what to do if internal keyboard is not platform but USB? Like
> > > > > Google "Whiskers"? (I am not sure why you decided to drop my mention
> > > > > of internal USB keyboards completely off your reply).
> > > >
> > > > I don't have answers for everything. Even if you have USB keyboard, you'll
> > > > likely still have backlight connected to embedded controller. If not,
> > > > then maybe you have exception userland needs to know about.
> > > >
> > > > Still better than making everything an exception.
> > >
> > > You do not need to make everything exception. You just need to look
> > > beyond the name, and see how the device is connected. And then apply
> > > your exceptions for "weird" devices.
> >
> > "Where it is connected" is not interesting to the userland. "Is it
> > backlight for internal keyboard" is the right question. It may be
> > connected to embedded controller or some kind of controller over
> > i2c... my shell scripts should not need to know about architecture of
> > every notebook out there.
> 
> Then your scripts will be failing for some setups.

Well, yes. Do you want to guess what "lp5523:kb3" is?

> > But I don't see why I should do additional work when its trivial for
> > kernel to just name the LED in an useful way.
> >
> > "platform::kbd_backlight" has no disadvantages compared to
> > "wilco::kbd_backlight" ... so lets just use it.
> 
> It has disadvantages because it promises more than it can deliver IMO.
> If device name != "platform::kbd_backlight" it does not mean that it
> is not internal keyboard.

My promise is if "platform::kbd_backlight" exists, it is backlight for
internal keyboard. (And second half is "if it is easy for kernel, we
name backlight for internal keyboard platform::kbd_backlight").

> And you still have not resolved how you will
> handle cases when there is more than one deice that can be considered
> internal and may have a backlight.

Is that realistic? How would that device look like?

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Guenter Roeck]

On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
> > On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
> > >
> > > Hi!
> > >
> > > > > > And what to do if internal keyboard is not platform but USB? Like
> > > > > > Google "Whiskers"? (I am not sure why you decided to drop my mention
> > > > > > of internal USB keyboards completely off your reply).
> > > > >
> > > > > I don't have answers for everything. Even if you have USB keyboard, you'll
> > > > > likely still have backlight connected to embedded controller. If not,
> > > > > then maybe you have exception userland needs to know about.
> > > > >
> > > > > Still better than making everything an exception.
> > > >
> > > > You do not need to make everything exception. You just need to look
> > > > beyond the name, and see how the device is connected. And then apply
> > > > your exceptions for "weird" devices.
> > >
> > > "Where it is connected" is not interesting to the userland. "Is it
> > > backlight for internal keyboard" is the right question. It may be
> > > connected to embedded controller or some kind of controller over
> > > i2c... my shell scripts should not need to know about architecture of
> > > every notebook out there.
> >
> > Then your scripts will be failing for some setups.
>
> Well, yes. Do you want to guess what "lp5523:kb3" is?
>

Oh, please. The discussion is about the driver name part, which you
want to overload with some string to mean "internal", which in turn
is, if anything, part of the functionality.

With "platform", you'll at some point have two
"platform::kbd_backlight" entries. Remind me to send you a "told you
so" when that happens.

Guenter

> > > But I don't see why I should do additional work when its trivial for
> > > kernel to just name the LED in an useful way.
> > >
> > > "platform::kbd_backlight" has no disadvantages compared to
> > > "wilco::kbd_backlight" ... so lets just use it.
> >
> > It has disadvantages because it promises more than it can deliver IMO.
> > If device name != "platform::kbd_backlight" it does not mean that it
> > is not internal keyboard.
>
> My promise is if "platform::kbd_backlight" exists, it is backlight for
> internal keyboard. (And second half is "if it is easy for kernel, we
> name backlight for internal keyboard platform::kbd_backlight").
>
> > And you still have not resolved how you will
> > handle cases when there is more than one deice that can be considered
> > internal and may have a backlight.
>
> Is that realistic? How would that device look like?
>
>                                                                         Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Enric Balletbo i Serra]

Hi,

On 5/4/19 0:42, Guenter Roeck wrote:
> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
>>
>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>>
>>>> Hi!
>>>>
>>>>>>> And what to do if internal keyboard is not platform but USB? Like
>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention
>>>>>>> of internal USB keyboards completely off your reply).
>>>>>>
>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll
>>>>>> likely still have backlight connected to embedded controller. If not,
>>>>>> then maybe you have exception userland needs to know about.
>>>>>>
>>>>>> Still better than making everything an exception.
>>>>>
>>>>> You do not need to make everything exception. You just need to look
>>>>> beyond the name, and see how the device is connected. And then apply
>>>>> your exceptions for "weird" devices.
>>>>
>>>> "Where it is connected" is not interesting to the userland. "Is it
>>>> backlight for internal keyboard" is the right question. It may be
>>>> connected to embedded controller or some kind of controller over
>>>> i2c... my shell scripts should not need to know about architecture of
>>>> every notebook out there.
>>>
>>> Then your scripts will be failing for some setups.
>>
>> Well, yes. Do you want to guess what "lp5523:kb3" is?
>>
> 
> Oh, please. The discussion is about the driver name part, which you
> want to overload with some string to mean "internal", which in turn
> is, if anything, part of the functionality.
> 
> With "platform", you'll at some point have two
> "platform::kbd_backlight" entries. Remind me to send you a "told you
> so" when that happens.
> 
> Guenter
> 
>>>> But I don't see why I should do additional work when its trivial for
>>>> kernel to just name the LED in an useful way.
>>>>
>>>> "platform::kbd_backlight" has no disadvantages compared to
>>>> "wilco::kbd_backlight" ... so lets just use it.
>>>
>>> It has disadvantages because it promises more than it can deliver IMO.
>>> If device name != "platform::kbd_backlight" it does not mean that it
>>> is not internal keyboard.
>>
>> My promise is if "platform::kbd_backlight" exists, it is backlight for
>> internal keyboard. (And second half is "if it is easy for kernel, we
>> name backlight for internal keyboard platform::kbd_backlight").
>>
>>> And you still have not resolved how you will
>>> handle cases when there is more than one deice that can be considered
>>> internal and may have a backlight.
>>
>> Is that realistic? How would that device look like?
>>

Maybe is something "weird" in the PC/laptop world but in the Embedded world is
not as weird as you think. I worked on devices that has two internal backlights,
one to lit the qwerty keyboard and the other one to lit the numeric pad. We used
the device field to differentiate both.

  tclkeyboard::kbd_backlight
  tclnumpad::kbd_backlight

Taking this to the extreme you can also think in a device where every key has
its own LED backlight, this happens for example in this device [1]. The device
can lit only specific keys giving to the user a word prediction experience (i.e
After press a key, only the keys that match with a possible word are lit on)

- Enric

[1] https://www.abilia.com/en/product/lightwriter-sl50

>>                                                                         Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Jacek Anaszewski]

Hi all,

On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote:
> Hi,
> 
> On 5/4/19 0:42, Guenter Roeck wrote:
>> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>
>>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
>>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>>>
>>>>> Hi!
>>>>>
>>>>>>>> And what to do if internal keyboard is not platform but USB? Like
>>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention
>>>>>>>> of internal USB keyboards completely off your reply).
>>>>>>>
>>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll
>>>>>>> likely still have backlight connected to embedded controller. If not,
>>>>>>> then maybe you have exception userland needs to know about.
>>>>>>>
>>>>>>> Still better than making everything an exception.
>>>>>>
>>>>>> You do not need to make everything exception. You just need to look
>>>>>> beyond the name, and see how the device is connected. And then apply
>>>>>> your exceptions for "weird" devices.
>>>>>
>>>>> "Where it is connected" is not interesting to the userland. "Is it
>>>>> backlight for internal keyboard" is the right question. It may be
>>>>> connected to embedded controller or some kind of controller over
>>>>> i2c... my shell scripts should not need to know about architecture of
>>>>> every notebook out there.
>>>>
>>>> Then your scripts will be failing for some setups.
>>>
>>> Well, yes. Do you want to guess what "lp5523:kb3" is?
>>>
>>
>> Oh, please. The discussion is about the driver name part, which you
>> want to overload with some string to mean "internal", which in turn
>> is, if anything, part of the functionality.
>>
>> With "platform", you'll at some point have two
>> "platform::kbd_backlight" entries. Remind me to send you a "told you
>> so" when that happens.
>>
>> Guenter
>>
>>>>> But I don't see why I should do additional work when its trivial for
>>>>> kernel to just name the LED in an useful way.
>>>>>
>>>>> "platform::kbd_backlight" has no disadvantages compared to
>>>>> "wilco::kbd_backlight" ... so lets just use it.
>>>>
>>>> It has disadvantages because it promises more than it can deliver IMO.
>>>> If device name != "platform::kbd_backlight" it does not mean that it
>>>> is not internal keyboard.
>>>
>>> My promise is if "platform::kbd_backlight" exists, it is backlight for
>>> internal keyboard. (And second half is "if it is easy for kernel, we
>>> name backlight for internal keyboard platform::kbd_backlight").
>>>
>>>> And you still have not resolved how you will
>>>> handle cases when there is more than one deice that can be considered
>>>> internal and may have a backlight.
>>>
>>> Is that realistic? How would that device look like?
>>>
> 
> Maybe is something "weird" in the PC/laptop world but in the Embedded world is
> not as weird as you think. I worked on devices that has two internal backlights,
> one to lit the qwerty keyboard and the other one to lit the numeric pad. We used
> the device field to differentiate both.
> 
>    keyboardist::kbd_backlight
>    tclnumpad::kbd_backlight
> 
> Taking this to the extreme you can also think in a device where every key has
> its own LED backlight, this happens for example in this device [1]. The device
> can lit only specific keys giving to the user a word prediction experience (i.e
> After press a key, only the keys that match with a possible word are lit on)

While we have your attention at the subject of LED naming I would like
to invite you all to reviewing my recent patch set [0], available
also on the led_naming_v3 branch of linux-leds.git [1].

The patch set introduces generic, backward compatible mechanism for
composing LED class devices names. It also aims to deprecate current
LED naming convention and encourage dropping "devicename" section.

Patch 5/25 from the discussed patch set includes
get_led_device_name_info.sh script proving that parent device name
of the LED class device is available in the sysfs and its presence
in the LED name is unjustified and redundant. The argument being raised
here related to name clash risk when there is no unique devicename
section included into the LED name is unjustified since LED core has
a protection against that and adds "_n" numerical suffix to the
requested LED name when it is already taken.

The patch set introduces also a set of predefined LED_FUNCTION
names to be used in DT bindings. This along with the removal
of devicename section from LED naming pattern will help to keep
LED sysfs interface more uniform and not varying depending on
underlaying hardware driving the LEDs.

Regarding the problem discussed in this thread - I would not necessarily
go for "platform" in place of devicename LED name section in the
cros_kbd_led_backlight driver. If we change it (should we at all - it is
already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
part. It believe it will be possible to retrieve this name with
get_led_device_info.sh script. It would be good exercise to check
it out.

In cases like above:

     keyboardist::kbd_backlight
     tclnumpad::kbd_backlight

we could do with the following:

     :kbd-backlight
     :numpad-backlight

I used hyphens instead of underscores since we will have this convention
in the LED_FUNCTION names, which is more common for Device Tree, and
some of existing LED triggers.

W could also think of placing common LED_FUNCTION definitions in
include/leds/led-functions.h and include it in both include/leds/leds.h
and include/dt-bindings/leds/common.h, so that they would be more
naturally accessible for non DT based drivers.


[0] https://lkml.org/lkml/2019/3/31/222
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/j.anaszewski/linux-leds.git/log/?h=led_naming_v3

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Guenter Roeck]

On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> The EC is in charge of controlling the keyboard backlight on
> the Wilco platform. We expose a standard LED class device at
> /sys/class/leds/platform::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
> 
> Some Wilco devices do not support a keyboard backlight. This
> is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> and a platform_device will only be registered by the core if
> a backlight is supported.
> 
> After an EC reset the backlight could be in a non-PWM mode.
> Earlier in the boot sequence the BIOS should send a command to
> the EC to set the brightness, so things **should** be set up,
> but we double check in probe() as we query the initial brightness.
> If not set up, then set the brightness to 0.
> 
> Since the EC will never change the backlight level of its own accord,
> we don't need to implement a brightness_get() method.
> 
> v5 changes:
> -Rename the LED device to "platform::kbd_backlight", to
> denote that this is the built-in system keyboard.
> 

NACK.

Per Documentation/leds/leds-class.txt, LED devices are named
	"devicename:colour:function"

This document also states "The naming scheme above leaves scope
for further attributes should they be needed". It does not permit,
however, to redefine one of the fields to mean "location", much less
the declaration that a devicename of "platform" shall refer to an
"internal" backlight, or that there shall be no more than one
"internal" backlight in a given system.

On top of that, with this naming scheme any system with more than
one internal backlight may result in duplicate sysfs attribute names.

If an attribute for "location" is desired or necessary as part of
the LED attribute name, such a change should be well defined, and
it should be documented, and it must not create the risk of
duplicate sysfs attribute names.

Thanks,
Guenter

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2181 bytes --]

On Fri 2019-04-05 13:15:34, Guenter Roeck wrote:
> On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> > The EC is in charge of controlling the keyboard backlight on
> > the Wilco platform. We expose a standard LED class device at
> > /sys/class/leds/platform::kbd_backlight. This driver is modeled
> > after the standard Chrome OS keyboard backlight driver at
> > drivers/platform/chrome/cros_kbd_led_backlight.c
> > 
> > Some Wilco devices do not support a keyboard backlight. This
> > is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> > and a platform_device will only be registered by the core if
> > a backlight is supported.
> > 
> > After an EC reset the backlight could be in a non-PWM mode.
> > Earlier in the boot sequence the BIOS should send a command to
> > the EC to set the brightness, so things **should** be set up,
> > but we double check in probe() as we query the initial brightness.
> > If not set up, then set the brightness to 0.
> > 
> > Since the EC will never change the backlight level of its own accord,
> > we don't need to implement a brightness_get() method.
> > 
> > v5 changes:
> > -Rename the LED device to "platform::kbd_backlight", to
> > denote that this is the built-in system keyboard.
> > 
> 
> NACK.

Please keep it as it is, it is okay.

> Per Documentation/leds/leds-class.txt, LED devices are named
> 	"devicename:colour:function"

You failed to follow threads explaining this is being changed, even
when I pointed you at them. What you are doing here is not helpful.

> This document also states "The naming scheme above leaves scope
> for further attributes should they be needed". It does not permit,
> however, to redefine one of the fields to mean "location", much less
> the declaration that a devicename of "platform" shall refer to an
> "internal" backlight, or that there shall be no more than one
> "internal" backlight in a given system.

"platform" is as good devicename as "wilco" or "chromeos".
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1755 bytes --]

Hi!

> The patch set introduces also a set of predefined LED_FUNCTION
> names to be used in DT bindings. This along with the removal
> of devicename section from LED naming pattern will help to keep
> LED sysfs interface more uniform and not varying depending on
> underlaying hardware driving the LEDs.
> 
> Regarding the problem discussed in this thread - I would not necessarily
> go for "platform" in place of devicename LED name section in the
> cros_kbd_led_backlight driver. If we change it (should we at all - it is
> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
> part. It believe it will be possible to retrieve this name with
> get_led_device_info.sh script. It would be good exercise to check
> it out.

I am not sure about existing driver. Important thing for me is that
new drivers use consistent naming.

> In cases like above:
> 
>     keyboardist::kbd_backlight
>     tclnumpad::kbd_backlight
> 
> we could do with the following:
> 
>     :kbd-backlight
>     :numpad-backlight
> 
> I used hyphens instead of underscores since we will have this convention
> in the LED_FUNCTION names, which is more common for Device Tree, and
> some of existing LED triggers.

Existing userspace already searches for *:kbd_backlight", AFAICT, so
we probably want to keep the "_".

I don't care much if we use "platform:" or no prefix at all for
backlight of internal keyboard, as long as it is consistent across all
devices.

We certainly want to use some prefix (probably inputX:) for backlight
on USB keyboards.

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Jacek Anaszewski]

Hi Pavel,

On 4/6/19 11:53 AM, Pavel Machek wrote:
> Hi!
> 
>> The patch set introduces also a set of predefined LED_FUNCTION
>> names to be used in DT bindings. This along with the removal
>> of devicename section from LED naming pattern will help to keep
>> LED sysfs interface more uniform and not varying depending on
>> underlaying hardware driving the LEDs.
>>
>> Regarding the problem discussed in this thread - I would not necessarily
>> go for "platform" in place of devicename LED name section in the
>> cros_kbd_led_backlight driver. If we change it (should we at all - it is
>> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
>> part. It believe it will be possible to retrieve this name with
>> get_led_device_info.sh script. It would be good exercise to check
>> it out.
> 
> I am not sure about existing driver. Important thing for me is that
> new drivers use consistent naming.
> 
>> In cases like above:
>>
>>      keyboardist::kbd_backlight
>>      tclnumpad::kbd_backlight
>>
>> we could do with the following:
>>
>>      :kbd-backlight
>>      :numpad-backlight
>>
>> I used hyphens instead of underscores since we will have this convention
>> in the LED_FUNCTION names, which is more common for Device Tree, and
>> some of existing LED triggers.
> 
> Existing userspace already searches for *:kbd_backlight", AFAICT, so
> we probably want to keep the "_".

OK, but it should be an exception but not a rule.
This "kbd-*" naming is used in input and tty subsystems which register
keyboard triggers with this style:

~/kernel$ git grep  ".*[\":]kbd-" -- "*.c"
drivers/input/input-leds.c:     [LED_NUML]      = { "numlock", 
VT_TRIGGER("kbd-numlock") },
drivers/input/input-leds.c:     [LED_CAPSL]     = { "capslock", 
VT_TRIGGER("kbd-capslock") },
drivers/input/input-leds.c:     [LED_SCROLLL]   = { "scrolllock", 
VT_TRIGGER("kbd-scrolllock") },
drivers/input/input-leds.c:     [LED_KANA]      = { "kana", 
VT_TRIGGER("kbd-kanalock") },
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_SCROLLOCK, 
"kbd-scrolllock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_NUMLOCK, 
"kbd-numlock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_CAPSLOCK, 
"kbd-capslock"),
drivers/tty/vt/keyboard.c:      KBD_LED_TRIGGER(VC_KANALOCK, 
"kbd-kanalock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTLOCK, 
"kbd-shiftlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_ALTGRLOCK, 
"kbd-altgrlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLLOCK, 
"kbd-ctrllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_ALTLOCK, 
"kbd-altlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTLLOCK, 
"kbd-shiftllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_SHIFTRLOCK, 
"kbd-shiftrlock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLLLOCK, 
"kbd-ctrlllock"),
drivers/tty/vt/keyboard.c:      KBD_LOCKSTATE_TRIGGER(VC_CTRLRLOCK, 
"kbd-ctrlrlock"),

"kbd_" naming is used only in case of backlight LEDs:

~/kernel$ git grep  ".*[\":]kbd_" -- "*.c"
drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name = 
"asus::kbd_backlight";
drivers/hid/hid-google-hammer.c:        kbd_backlight->cdev.name = 
"hammer::kbd_backlight";
drivers/hwmon/applesmc.c:       .name                   = 
"smc::kbd_backlight",
drivers/input/misc/ims-pcu.c:            "pcu%d::kbd_backlight", 
pcu->device_no);
drivers/platform/chrome/cros_kbd_led_backlight.c:       cdev->name = 
"chromeos::kbd_backlight";
drivers/platform/x86/asus-laptop.c:             cdev->name = 
"asus::kbd_backlight";
drivers/platform/x86/asus-wmi.c:                asus->kbd_led.name = 
"asus::kbd_backlight";
drivers/platform/x86/dell-laptop.c:     .name           = 
"dell::kbd_backlight",
drivers/platform/x86/samsung-laptop.c:          samsung->kbd_led.name = 
"samsung::kbd_backlight";
drivers/platform/x86/sony-laptop.c:     kbdbl_ctl->mode_attr.attr.name = 
"kbd_backlight";
drivers/platform/x86/sony-laptop.c: 
kbdbl_ctl->timeout_attr.attr.name = "kbd_backlight_timeout";
drivers/platform/x86/thinkpad_acpi.c:           .name           = 
"tpacpi::kbd_backlight",
drivers/platform/x86/toshiba_acpi.c:            dev->kbd_led.name = 
"toshiba::kbd_backlight";

With LEDs in platform drivers is that problem that we have the name
compiled into the kernel. Maybe to make it more flexible we could
use kernel config to choose between new "kbd-" and legacy "kbd_"
naming.

> I don't care much if we use "platform:" or no prefix at all for
> backlight of internal keyboard, as long as it is consistent across all
> devices.
> 
> We certainly want to use some prefix (probably inputX:) for backlight
> on USB keyboards.

For LEDs exposed through the input-LED bridge my get_led_device_info.sh
script nicely reports:

associated input node: input1

It just does:

readlink input1\:\:numlock/device

which prints: "../../input1 "

And I believe that for backlight LEDs exposed by input
subsystem it should be similarly since the input device
related to USB keyboard is a child of USB device:

/sys/class/leds# readlink input1::numlock
../../devices/pci0000:00/0000:00:14.0/usb2/2-4/2-4:1.0/0003:1C4F:0002.0002/input/input1/input1::numlock

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2287 bytes --]

Hi!

> >I am not sure about existing driver. Important thing for me is that
> >new drivers use consistent naming.
> >
> >>In cases like above:
> >>
> >>     keyboardist::kbd_backlight
> >>     tclnumpad::kbd_backlight
> >>
> >>we could do with the following:
> >>
> >>     :kbd-backlight
> >>     :numpad-backlight
> >>
> >>I used hyphens instead of underscores since we will have this convention
> >>in the LED_FUNCTION names, which is more common for Device Tree, and
> >>some of existing LED triggers.
> >
> >Existing userspace already searches for *:kbd_backlight", AFAICT, so
> >we probably want to keep the "_".
> 
> OK, but it should be an exception but not a rule.
> This "kbd-*" naming is used in input and tty subsystems which register
> keyboard triggers with this style:
> 
> ~/kernel$ git grep  ".*[\":]kbd-" -- "*.c"
> drivers/input/input-leds.c:     [LED_NUML]      = { "numlock",
> VT_TRIGGER("kbd-numlock") },

> "kbd_" naming is used only in case of backlight LEDs:
> 
> ~/kernel$ git grep  ".*[\":]kbd_" -- "*.c"
> drivers/hid/hid-asus.c: drvdata->kbd_backlight->cdev.name =
> "asus::kbd_backlight";

Yes, but userland already knows about kbd_backlight, so we really can
not change this.

> With LEDs in platform drivers is that problem that we have the name
> compiled into the kernel. Maybe to make it more flexible we could
> use kernel config to choose between new "kbd-" and legacy "kbd_"
> naming.

No, I don't think that is suitable use for config option.

> >I don't care much if we use "platform:" or no prefix at all for
> >backlight of internal keyboard, as long as it is consistent across all
> >devices.
> >
> >We certainly want to use some prefix (probably inputX:) for backlight
> >on USB keyboards.
> 
> For LEDs exposed through the input-LED bridge my get_led_device_info.sh
> script nicely reports:
> 
> associated input node: input1
> 
> It just does:
> 
> readlink input1\:\:numlock/device
> 
> which prints: "../../input1 "

Yes, that will probably work for USB keyboards. (But not for internal
keyboards on phones, etc).


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Dmitry Torokhov]

On Sat, Apr 6, 2019 at 1:41 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> On Fri 2019-04-05 13:15:34, Guenter Roeck wrote:
> > On Thu, Apr 04, 2019 at 11:10:08AM -0600, Nick Crews wrote:
> > > The EC is in charge of controlling the keyboard backlight on
> > > the Wilco platform. We expose a standard LED class device at
> > > /sys/class/leds/platform::kbd_backlight. This driver is modeled
> > > after the standard Chrome OS keyboard backlight driver at
> > > drivers/platform/chrome/cros_kbd_led_backlight.c
> > >
> > > Some Wilco devices do not support a keyboard backlight. This
> > > is checked via wilco_ec_keyboard_leds_exist() in the core driver,
> > > and a platform_device will only be registered by the core if
> > > a backlight is supported.
> > >
> > > After an EC reset the backlight could be in a non-PWM mode.
> > > Earlier in the boot sequence the BIOS should send a command to
> > > the EC to set the brightness, so things **should** be set up,
> > > but we double check in probe() as we query the initial brightness.
> > > If not set up, then set the brightness to 0.
> > >
> > > Since the EC will never change the backlight level of its own accord,
> > > we don't need to implement a brightness_get() method.
> > >
> > > v5 changes:
> > > -Rename the LED device to "platform::kbd_backlight", to
> > > denote that this is the built-in system keyboard.
> > >
> >
> > NACK.
>
> Please keep it as it is, it is okay.
>
> > Per Documentation/leds/leds-class.txt, LED devices are named
> >       "devicename:colour:function"
>
> You failed to follow threads explaining this is being changed, even
> when I pointed you at them. What you are doing here is not helpful.

Pavel, what I find is unhelpful is you requiring to conform to the new
rules that have not been accepted yet and for which there clearly are
objections. You keep ignoring all the issues that we continue to point
out with your proposed scheme.

I will go and try to reply to Jacek's thread, but it is my firm belief
that changing naming scheme is not what we need to do here.

>
> > This document also states "The naming scheme above leaves scope
> > for further attributes should they be needed". It does not permit,
> > however, to redefine one of the fields to mean "location", much less
> > the declaration that a devicename of "platform" shall refer to an
> > "internal" backlight, or that there shall be no more than one
> > "internal" backlight in a given system.
>
> "platform" is as good devicename as "wilco" or "chromeos".

No, because "platform" is not a device, it is something that you are
trying to assign a magic meaning to.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

Hi Jacek,

On Fri, Apr 5, 2019 at 1:00 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> Hi all,
>
> On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote:
> > Hi,
> >
> > On 5/4/19 0:42, Guenter Roeck wrote:
> >> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
> >>>
> >>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
> >>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>>>>> And what to do if internal keyboard is not platform but USB? Like
> >>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention
> >>>>>>>> of internal USB keyboards completely off your reply).
> >>>>>>>
> >>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll
> >>>>>>> likely still have backlight connected to embedded controller. If not,
> >>>>>>> then maybe you have exception userland needs to know about.
> >>>>>>>
> >>>>>>> Still better than making everything an exception.
> >>>>>>
> >>>>>> You do not need to make everything exception. You just need to look
> >>>>>> beyond the name, and see how the device is connected. And then apply
> >>>>>> your exceptions for "weird" devices.
> >>>>>
> >>>>> "Where it is connected" is not interesting to the userland. "Is it
> >>>>> backlight for internal keyboard" is the right question. It may be
> >>>>> connected to embedded controller or some kind of controller over
> >>>>> i2c... my shell scripts should not need to know about architecture of
> >>>>> every notebook out there.
> >>>>
> >>>> Then your scripts will be failing for some setups.
> >>>
> >>> Well, yes. Do you want to guess what "lp5523:kb3" is?
> >>>
> >>
> >> Oh, please. The discussion is about the driver name part, which you
> >> want to overload with some string to mean "internal", which in turn
> >> is, if anything, part of the functionality.
> >>
> >> With "platform", you'll at some point have two
> >> "platform::kbd_backlight" entries. Remind me to send you a "told you
> >> so" when that happens.
> >>
> >> Guenter
> >>
> >>>>> But I don't see why I should do additional work when its trivial for
> >>>>> kernel to just name the LED in an useful way.
> >>>>>
> >>>>> "platform::kbd_backlight" has no disadvantages compared to
> >>>>> "wilco::kbd_backlight" ... so lets just use it.
> >>>>
> >>>> It has disadvantages because it promises more than it can deliver IMO.
> >>>> If device name != "platform::kbd_backlight" it does not mean that it
> >>>> is not internal keyboard.
> >>>
> >>> My promise is if "platform::kbd_backlight" exists, it is backlight for
> >>> internal keyboard. (And second half is "if it is easy for kernel, we
> >>> name backlight for internal keyboard platform::kbd_backlight").
> >>>
> >>>> And you still have not resolved how you will
> >>>> handle cases when there is more than one deice that can be considered
> >>>> internal and may have a backlight.
> >>>
> >>> Is that realistic? How would that device look like?
> >>>
> >
> > Maybe is something "weird" in the PC/laptop world but in the Embedded world is
> > not as weird as you think. I worked on devices that has two internal backlights,
> > one to lit the qwerty keyboard and the other one to lit the numeric pad. We used
> > the device field to differentiate both.
> >
> >    keyboardist::kbd_backlight
> >    tclnumpad::kbd_backlight
> >
> > Taking this to the extreme you can also think in a device where every key has
> > its own LED backlight, this happens for example in this device [1]. The device
> > can lit only specific keys giving to the user a word prediction experience (i.e
> > After press a key, only the keys that match with a possible word are lit on)
>
> While we have your attention at the subject of LED naming I would like
> to invite you all to reviewing my recent patch set [0], available
> also on the led_naming_v3 branch of linux-leds.git [1].
>
> The patch set introduces generic, backward compatible mechanism for
> composing LED class devices names. It also aims to deprecate current
> LED naming convention and encourage dropping "devicename" section.

>From looking at the docs section it looks like you propose to move
from "device:color:fucntion" to simply "color:function" naming, and
expect to have a suffix "_<n>" to avoid problem with duplicate LED
names. I do not think this is quite backward compatible, since
previously userspace was supposed to split the device name on the
colon boundaries and extract the 3rd component if it wanted to
determine function. With the new proposed scheme it has to be modified
to try and also fetch the 2nd component if there isn't 3rd one and
consider it as function as well. It also need to recognize potential
suffixes and drop them before matching on function name.

I think if you want flexibility you really need to switch from
encoding the information in the name to add LED class attributes
describing the LED in more detail. This might include information
about LEd placement (internal/external) if such information is
available, and other additional attributes, if needed. Updated
userspace can make use of these new attributes, leaving existing
userspace decoding legacy names.

>
> Patch 5/25 from the discussed patch set includes
> get_led_device_name_info.sh script proving that parent device name
> of the LED class device is available in the sysfs and its presence
> in the LED name is unjustified and redundant.  The argument being raised
> here related to name clash risk when there is no unique devicename
> section included into the LED name is unjustified since LED core has
> a protection against that and adds "_n" numerical suffix to the
> requested LED name when it is already taken.

This scheme breaks userspace that does not expect additional suffixes
attached to function name.

>
> The patch set introduces also a set of predefined LED_FUNCTION
> names to be used in DT bindings. This along with the removal
> of devicename section from LED naming pattern will help to keep
> LED sysfs interface more uniform and not varying depending on
> underlaying hardware driving the LEDs.
>
> Regarding the problem discussed in this thread - I would not necessarily
> go for "platform" in place of devicename LED name section in the
> cros_kbd_led_backlight driver. If we change it (should we at all - it is
> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
> part. It believe it will be possible to retrieve this name with
> get_led_device_info.sh script. It would be good exercise to check
> it out.
>
> In cases like above:
>
>      keyboardist::kbd_backlight
>      tclnumpad::kbd_backlight
>
> we could do with the following:
>
>      :kbd-backlight
>      :numpad-backlight
>
> I used hyphens instead of underscores since we will have this convention
> in the LED_FUNCTION names, which is more common for Device Tree, and
> some of existing LED triggers.

I am not sure what device tree has to do with it. ACPI for example
likes all caps and sort names with numbers, but we do not let it
propagate into the kernel.

We also should not be changing existing function names as existing
userspace relies on them.

>
> W could also think of placing common LED_FUNCTION definitions in
> include/leds/led-functions.h and include it in both include/leds/leds.h
> and include/dt-bindings/leds/common.h, so that they would be more
> naturally accessible for non DT based drivers.

This makes total sense.

Thanks.

-- 
Dmitry

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Dmitry Torokhov]

Hi Jacek,

On Fri, Apr 5, 2019 at 1:00 PM Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>
> Hi all,
>
> On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote:
> > Hi,
> >
> > On 5/4/19 0:42, Guenter Roeck wrote:
> >> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
> >>>
> >>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
> >>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
> >>>>>
> >>>>> Hi!
> >>>>>
> >>>>>>>> And what to do if internal keyboard is not platform but USB? Like
> >>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention
> >>>>>>>> of internal USB keyboards completely off your reply).
> >>>>>>>
> >>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll
> >>>>>>> likely still have backlight connected to embedded controller. If not,
> >>>>>>> then maybe you have exception userland needs to know about.
> >>>>>>>
> >>>>>>> Still better than making everything an exception.
> >>>>>>
> >>>>>> You do not need to make everything exception. You just need to look
> >>>>>> beyond the name, and see how the device is connected. And then apply
> >>>>>> your exceptions for "weird" devices.
> >>>>>
> >>>>> "Where it is connected" is not interesting to the userland. "Is it
> >>>>> backlight for internal keyboard" is the right question. It may be
> >>>>> connected to embedded controller or some kind of controller over
> >>>>> i2c... my shell scripts should not need to know about architecture of
> >>>>> every notebook out there.
> >>>>
> >>>> Then your scripts will be failing for some setups.
> >>>
> >>> Well, yes. Do you want to guess what "lp5523:kb3" is?
> >>>
> >>
> >> Oh, please. The discussion is about the driver name part, which you
> >> want to overload with some string to mean "internal", which in turn
> >> is, if anything, part of the functionality.
> >>
> >> With "platform", you'll at some point have two
> >> "platform::kbd_backlight" entries. Remind me to send you a "told you
> >> so" when that happens.
> >>
> >> Guenter
> >>
> >>>>> But I don't see why I should do additional work when its trivial for
> >>>>> kernel to just name the LED in an useful way.
> >>>>>
> >>>>> "platform::kbd_backlight" has no disadvantages compared to
> >>>>> "wilco::kbd_backlight" ... so lets just use it.
> >>>>
> >>>> It has disadvantages because it promises more than it can deliver IMO.
> >>>> If device name != "platform::kbd_backlight" it does not mean that it
> >>>> is not internal keyboard.
> >>>
> >>> My promise is if "platform::kbd_backlight" exists, it is backlight for
> >>> internal keyboard. (And second half is "if it is easy for kernel, we
> >>> name backlight for internal keyboard platform::kbd_backlight").
> >>>
> >>>> And you still have not resolved how you will
> >>>> handle cases when there is more than one deice that can be considered
> >>>> internal and may have a backlight.
> >>>
> >>> Is that realistic? How would that device look like?
> >>>
> >
> > Maybe is something "weird" in the PC/laptop world but in the Embedded world is
> > not as weird as you think. I worked on devices that has two internal backlights,
> > one to lit the qwerty keyboard and the other one to lit the numeric pad. We used
> > the device field to differentiate both.
> >
> >    keyboardist::kbd_backlight
> >    tclnumpad::kbd_backlight
> >
> > Taking this to the extreme you can also think in a device where every key has
> > its own LED backlight, this happens for example in this device [1]. The device
> > can lit only specific keys giving to the user a word prediction experience (i.e
> > After press a key, only the keys that match with a possible word are lit on)
>
> While we have your attention at the subject of LED naming I would like
> to invite you all to reviewing my recent patch set [0], available
> also on the led_naming_v3 branch of linux-leds.git [1].
>
> The patch set introduces generic, backward compatible mechanism for
> composing LED class devices names. It also aims to deprecate current
> LED naming convention and encourage dropping "devicename" section.

From looking at the docs section it looks like you propose to move
from "device:color:fucntion" to simply "color:function" naming, and
expect to have a suffix "_<n>" to avoid problem with duplicate LED
names. I do not think this is quite backward compatible, since
previously userspace was supposed to split the device name on the
colon boundaries and extract the 3rd component if it wanted to
determine function. With the new proposed scheme it has to be modified
to try and also fetch the 2nd component if there isn't 3rd one and
consider it as function as well. It also need to recognize potential
suffixes and drop them before matching on function name.

I think if you want flexibility you really need to switch from
encoding the information in the name to add LED class attributes
describing the LED in more detail. This might include information
about LEd placement (internal/external) if such information is
available, and other additional attributes, if needed. Updated
userspace can make use of these new attributes, leaving existing
userspace decoding legacy names.

>
> Patch 5/25 from the discussed patch set includes
> get_led_device_name_info.sh script proving that parent device name
> of the LED class device is available in the sysfs and its presence
> in the LED name is unjustified and redundant.  The argument being raised
> here related to name clash risk when there is no unique devicename
> section included into the LED name is unjustified since LED core has
> a protection against that and adds "_n" numerical suffix to the
> requested LED name when it is already taken.

This scheme breaks userspace that does not expect additional suffixes
attached to function name.

>
> The patch set introduces also a set of predefined LED_FUNCTION
> names to be used in DT bindings. This along with the removal
> of devicename section from LED naming pattern will help to keep
> LED sysfs interface more uniform and not varying depending on
> underlaying hardware driving the LEDs.
>
> Regarding the problem discussed in this thread - I would not necessarily
> go for "platform" in place of devicename LED name section in the
> cros_kbd_led_backlight driver. If we change it (should we at all - it is
> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
> part. It believe it will be possible to retrieve this name with
> get_led_device_info.sh script. It would be good exercise to check
> it out.
>
> In cases like above:
>
>      keyboardist::kbd_backlight
>      tclnumpad::kbd_backlight
>
> we could do with the following:
>
>      :kbd-backlight
>      :numpad-backlight
>
> I used hyphens instead of underscores since we will have this convention
> in the LED_FUNCTION names, which is more common for Device Tree, and
> some of existing LED triggers.

I am not sure what device tree has to do with it. ACPI for example
likes all caps and sort names with numbers, but we do not let it
propagate into the kernel.

We also should not be changing existing function names as existing
userspace relies on them.

>
> W could also think of placing common LED_FUNCTION definitions in
> include/leds/led-functions.h and include it in both include/leds/leds.h
> and include/dt-bindings/leds/common.h, so that they would be more
> naturally accessible for non DT based drivers.

This makes total sense.

Thanks.

-- 
Dmitry

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 791 bytes --]


> > > This document also states "The naming scheme above leaves scope
> > > for further attributes should they be needed". It does not permit,
> > > however, to redefine one of the fields to mean "location", much less
> > > the declaration that a devicename of "platform" shall refer to an
> > > "internal" backlight, or that there shall be no more than one
> > > "internal" backlight in a given system.
> >
> > "platform" is as good devicename as "wilco" or "chromeos".
> 
> No, because "platform" is not a device, it is something that you are
> trying to assign a magic meaning to.

"chromeos" is not a device, either.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Dmitry Torokhov]

On Sun, Apr 7, 2019 at 3:18 PM Pavel Machek <pavel@ucw.cz> wrote:
>
>
> > > > This document also states "The naming scheme above leaves scope
> > > > for further attributes should they be needed". It does not permit,
> > > > however, to redefine one of the fields to mean "location", much less
> > > > the declaration that a devicename of "platform" shall refer to an
> > > > "internal" backlight, or that there shall be no more than one
> > > > "internal" backlight in a given system.
> > >
> > > "platform" is as good devicename as "wilco" or "chromeos".
> >
> > No, because "platform" is not a device, it is something that you are
> > trying to assign a magic meaning to.
>
> "chromeos" is not a device, either.

I agree, it is not a device name. We do not assign any specific
meaning to it though. We could change it to "cros_ec" if so desired
and nothing should break.

Thanks.

-- 
Dmitry

Keyboard backlight LED naming was Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2716 bytes --]

Hi!

> > > > > This document also states "The naming scheme above leaves scope
> > > > > for further attributes should they be needed". It does not permit,
> > > > > however, to redefine one of the fields to mean "location", much less
> > > > > the declaration that a devicename of "platform" shall refer to an
> > > > > "internal" backlight, or that there shall be no more than one
> > > > > "internal" backlight in a given system.
> > > >
> > > > "platform" is as good devicename as "wilco" or "chromeos".
> > >
> > > No, because "platform" is not a device, it is something that you are
> > > trying to assign a magic meaning to.
> >
> > "chromeos" is not a device, either.
> 
> I agree, it is not a device name. We do not assign any specific
> meaning to it though. We could change it to "cros_ec" if so desired
> and nothing should break.

Yes. And you can also change it to "platform" and nothing will break
:-). Can we end the discussion here?

If not, lets take a look at existing names:

./drivers/platform/x86/asus-laptop.c:		cdev->name = "asus::kbd_backlight";
./drivers/platform/x86/samsung-laptop.c:		samsung->kbd_led.name = "samsung::kbd_backlight";
./drivers/platform/x86/thinkpad_acpi.c:		.name		= "tpacpi::kbd_backlight",
./drivers/platform/x86/toshiba_acpi.c:		dev->kbd_led.name = "toshiba::kbd_backlight";
./drivers/platform/x86/asus-wmi.c:		asus->kbd_led.name = "asus::kbd_backlight";
./drivers/platform/x86/dell-laptop.c:	.name           = "dell::kbd_backlight",
./drivers/platform/chrome/cros_kbd_led_backlight.c:	cdev->name = "chromeos::kbd_backlight";
./drivers/hwmon/applesmc.c:	.name			= "smc::kbd_backlight",
./drivers/hid/hid-asus.c:	drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
./drivers/hid/hid-google-hammer.c:	kbd_backlight->cdev.name = "hammer::kbd_backlight";
./drivers/input/misc/ims-pcu.c:		 "pcu%d::kbd_backlight", pcu->device_no);

asus, samsung, toshiba, asus, dell, chromeos... Those are really not
device names, either. But in these cases, LED is probably controlled
by EC, or by ACPI BIOS talking to the EC. People usually call such
devices "platform devices".

(Linux Platform Device Driver - CodeProject
https://www.codeproject.com/tips/1080177/linux-platform-device-driver
A platform device is one which is hardwired on the board and hence not
hot-pluggable. )

You can take a look at discussion around micmute LED.

Thus "platform" is quite suitable name in your case, and incidentaly,
it will be more useful for userspace than "cros_ec".

								Pavel
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Keyboard backlight LED naming was Re: [PATCH v5 2/3] platform/chrome: Add Wilco EC keyboard backlight LEDs support

[Guenter Roeck]

On Mon, Apr 8, 2019 at 2:41 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > > > > This document also states "The naming scheme above leaves scope
> > > > > > for further attributes should they be needed". It does not permit,
> > > > > > however, to redefine one of the fields to mean "location", much less
> > > > > > the declaration that a devicename of "platform" shall refer to an
> > > > > > "internal" backlight, or that there shall be no more than one
> > > > > > "internal" backlight in a given system.
> > > > >
> > > > > "platform" is as good devicename as "wilco" or "chromeos".
> > > >
> > > > No, because "platform" is not a device, it is something that you are
> > > > trying to assign a magic meaning to.
> > >
> > > "chromeos" is not a device, either.
> >
> > I agree, it is not a device name. We do not assign any specific
> > meaning to it though. We could change it to "cros_ec" if so desired
> > and nothing should break.
>
> Yes. And you can also change it to "platform" and nothing will break
> :-). Can we end the discussion here?
>
> If not, lets take a look at existing names:
>
> ./drivers/platform/x86/asus-laptop.c:           cdev->name = "asus::kbd_backlight";
> ./drivers/platform/x86/samsung-laptop.c:                samsung->kbd_led.name = "samsung::kbd_backlight";
> ./drivers/platform/x86/thinkpad_acpi.c:         .name           = "tpacpi::kbd_backlight",
> ./drivers/platform/x86/toshiba_acpi.c:          dev->kbd_led.name = "toshiba::kbd_backlight";
> ./drivers/platform/x86/asus-wmi.c:              asus->kbd_led.name = "asus::kbd_backlight";
> ./drivers/platform/x86/dell-laptop.c:   .name           = "dell::kbd_backlight",
> ./drivers/platform/chrome/cros_kbd_led_backlight.c:     cdev->name = "chromeos::kbd_backlight";
> ./drivers/hwmon/applesmc.c:     .name                   = "smc::kbd_backlight",
> ./drivers/hid/hid-asus.c:       drvdata->kbd_backlight->cdev.name = "asus::kbd_backlight";
> ./drivers/hid/hid-google-hammer.c:      kbd_backlight->cdev.name = "hammer::kbd_backlight";
> ./drivers/input/misc/ims-pcu.c:          "pcu%d::kbd_backlight", pcu->device_no);
>
> asus, samsung, toshiba, asus, dell, chromeos... Those are really not
> device names, either. But in these cases, LED is probably controlled
> by EC, or by ACPI BIOS talking to the EC. People usually call such
> devices "platform devices".
>
> (Linux Platform Device Driver - CodeProject
> https://www.codeproject.com/tips/1080177/linux-platform-device-driver
> A platform device is one which is hardwired on the board and hence not
> hot-pluggable. )
>
> You can take a look at discussion around micmute LED.
>
> Thus "platform" is quite suitable name in your case, and incidentaly,
> it will be more useful for userspace than "cros_ec".
>
No, it isn't. All those name are at least roughly associated with the
driver name, and they all do reflect the abbreviated driver name.
"platform" isn't, and is prone to duplicates.

If you want the location reflected in the led sysfs atttribute name,
say so. "platform" is neither a driver name nor a location. On a side
note, appending a number to the attribute name is not a good idea. In
addition to not being backward compatible, it would be prone to
renumbering at each reboot.

Guenter

>                                                                 Pavel
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Jacek Anaszewski]

Hi Dmitry,

Thanks for the review.

On 4/8/19 12:01 AM, Dmitry Torokhov wrote:
> Hi Jacek,
> 
> On Fri, Apr 5, 2019 at 1:00 PM Jacek Anaszewski
> <jacek.anaszewski@gmail.com> wrote:
>>
>> Hi all,
>>
>> On 4/5/19 10:42 AM, Enric Balletbo i Serra wrote:
>>> Hi,
>>>
>>> On 5/4/19 0:42, Guenter Roeck wrote:
>>>> On Thu, Apr 4, 2019 at 3:05 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>>>
>>>>> On Thu 2019-04-04 14:48:35, Dmitry Torokhov wrote:
>>>>>> On Thu, Apr 4, 2019 at 1:42 PM Pavel Machek <pavel@ucw.cz> wrote:
>>>>>>>
>>>>>>> Hi!
>>>>>>>
>>>>>>>>>> And what to do if internal keyboard is not platform but USB? Like
>>>>>>>>>> Google "Whiskers"? (I am not sure why you decided to drop my mention
>>>>>>>>>> of internal USB keyboards completely off your reply).
>>>>>>>>>
>>>>>>>>> I don't have answers for everything. Even if you have USB keyboard, you'll
>>>>>>>>> likely still have backlight connected to embedded controller. If not,
>>>>>>>>> then maybe you have exception userland needs to know about.
>>>>>>>>>
>>>>>>>>> Still better than making everything an exception.
>>>>>>>>
>>>>>>>> You do not need to make everything exception. You just need to look
>>>>>>>> beyond the name, and see how the device is connected. And then apply
>>>>>>>> your exceptions for "weird" devices.
>>>>>>>
>>>>>>> "Where it is connected" is not interesting to the userland. "Is it
>>>>>>> backlight for internal keyboard" is the right question. It may be
>>>>>>> connected to embedded controller or some kind of controller over
>>>>>>> i2c... my shell scripts should not need to know about architecture of
>>>>>>> every notebook out there.
>>>>>>
>>>>>> Then your scripts will be failing for some setups.
>>>>>
>>>>> Well, yes. Do you want to guess what "lp5523:kb3" is?
>>>>>
>>>>
>>>> Oh, please. The discussion is about the driver name part, which you
>>>> want to overload with some string to mean "internal", which in turn
>>>> is, if anything, part of the functionality.
>>>>
>>>> With "platform", you'll at some point have two
>>>> "platform::kbd_backlight" entries. Remind me to send you a "told you
>>>> so" when that happens.
>>>>
>>>> Guenter
>>>>
>>>>>>> But I don't see why I should do additional work when its trivial for
>>>>>>> kernel to just name the LED in an useful way.
>>>>>>>
>>>>>>> "platform::kbd_backlight" has no disadvantages compared to
>>>>>>> "wilco::kbd_backlight" ... so lets just use it.
>>>>>>
>>>>>> It has disadvantages because it promises more than it can deliver IMO.
>>>>>> If device name != "platform::kbd_backlight" it does not mean that it
>>>>>> is not internal keyboard.
>>>>>
>>>>> My promise is if "platform::kbd_backlight" exists, it is backlight for
>>>>> internal keyboard. (And second half is "if it is easy for kernel, we
>>>>> name backlight for internal keyboard platform::kbd_backlight").
>>>>>
>>>>>> And you still have not resolved how you will
>>>>>> handle cases when there is more than one deice that can be considered
>>>>>> internal and may have a backlight.
>>>>>
>>>>> Is that realistic? How would that device look like?
>>>>>
>>>
>>> Maybe is something "weird" in the PC/laptop world but in the Embedded world is
>>> not as weird as you think. I worked on devices that has two internal backlights,
>>> one to lit the qwerty keyboard and the other one to lit the numeric pad. We used
>>> the device field to differentiate both.
>>>
>>>     keyboardist::kbd_backlight
>>>     tclnumpad::kbd_backlight
>>>
>>> Taking this to the extreme you can also think in a device where every key has
>>> its own LED backlight, this happens for example in this device [1]. The device
>>> can lit only specific keys giving to the user a word prediction experience (i.e
>>> After press a key, only the keys that match with a possible word are lit on)
>>
>> While we have your attention at the subject of LED naming I would like
>> to invite you all to reviewing my recent patch set [0], available
>> also on the led_naming_v3 branch of linux-leds.git [1].
>>
>> The patch set introduces generic, backward compatible mechanism for
>> composing LED class devices names. It also aims to deprecate current
>> LED naming convention and encourage dropping "devicename" section.
> 
>>From looking at the docs section it looks like you propose to move
> from "device:color:fucntion" to simply "color:function" naming, and
> expect to have a suffix "_<n>" to avoid problem with duplicate LED
> names. I do not think this is quite backward compatible, since
> previously userspace was supposed to split the device name on the
> colon boundaries and extract the 3rd component if it wanted to
> determine function. With the new proposed scheme it has to be modified
> to try and also fetch the 2nd component if there isn't 3rd one and
> consider it as function as well. It also need to recognize potential
> suffixes and drop them before matching on function name.

The feature adding "_n" suffixes is not being added in my patch set,
it only gets documented. It was added back in 2015 to cover the
case when a LED with the name already present in the system is being
added via DT overlay:

commit a96aa64cb5723d941de879a9cd1fea025d6acb1b
Author: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
Date:   Mon Mar 30 10:45:59 2015 -0700

     leds/led-class: Handle LEDs with the same name

     The current code expected that every LED had an unique name. This is a
     legit expectation when the device tree can no be modified or extended.
     But with device tree overlays this requirement can be easily broken.

     This patch finds out if the name is already in use and adds the suffix
     _1, _2... if not.

     Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
     Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
     Signed-off-by: Bryan Wu <cooloney@gmail.com>

If LEDs will be properly assigned function names, there will be
no need for resorting to this fallback mechanism. I believe, that
at least in case of platform drivers you are not going to have two
LEDs with exactly the same function name, which is going to reflect what
would be printed on the sticker next to the LED on the device case.

One type of devices for which preserving devicename will make sense
are USB ones. In case of my wifi dongle I get LED name like
mt7601u-phy0. The name which would introduce added value would be:
"phy0::wlan".

The backward compatibility I mention in the patch refers to
keeping the support for devicename:color:function.
It means that operation of all existing drivers will not be
affected. New drivers will also be able to stick to the
old naming convention, however people will be encouraged
to drop the devicename section as it has no added value beside
more "human readable" LED names. This however is not a question
in case of devfs nodes, right?

Generally  the goal is to gradually get rid of this clumsy naming,
which includes frequently the chipset name or other arbitrary
names like vendor or platform name.

Userspace will always be able to retrieve hardware related
details, which are available in the sysfs all the time.
Please check the get_led_device_info.sh script being added
in the patch set.

> I think if you want flexibility you really need to switch from
> encoding the information in the name to add LED class attributes
> describing the LED in more detail. This might include information
> about LEd placement (internal/external) if such information is
> available, and other additional attributes, if needed. Updated
> userspace can make use of these new attributes, leaving existing
> userspace decoding legacy names.
> 
>>
>> Patch 5/25 from the discussed patch set includes
>> get_led_device_name_info.sh script proving that parent device name
>> of the LED class device is available in the sysfs and its presence
>> in the LED name is unjustified and redundant.  The argument being raised
>> here related to name clash risk when there is no unique devicename
>> section included into the LED name is unjustified since LED core has
>> a protection against that and adds "_n" numerical suffix to the
>> requested LED name when it is already taken.
> 
> This scheme breaks userspace that does not expect additional suffixes
> attached to function name.

Like I explained above the addition of suffixes is not a part of
this patch set, but a pre-existing fallback for avoiding LED name
clash.

In order to get the gist of the changes it is required to go
through the first five patches of the patch set and read commit
messages as well as the documentation of the functions being
added.

>> The patch set introduces also a set of predefined LED_FUNCTION
>> names to be used in DT bindings. This along with the removal
>> of devicename section from LED naming pattern will help to keep
>> LED sysfs interface more uniform and not varying depending on
>> underlaying hardware driving the LEDs.
>>
>> Regarding the problem discussed in this thread - I would not necessarily
>> go for "platform" in place of devicename LED name section in the
>> cros_kbd_led_backlight driver. If we change it (should we at all - it is
>> already in 5.0 AFAICS?), then I would opt for dropping the "chromeos:"
>> part. It believe it will be possible to retrieve this name with
>> get_led_device_info.sh script. It would be good exercise to check
>> it out.
>>
>> In cases like above:
>>
>>       keyboardist::kbd_backlight
>>       tclnumpad::kbd_backlight
>>
>> we could do with the following:
>>
>>       :kbd-backlight
>>       :numpad-backlight
>>
>> I used hyphens instead of underscores since we will have this convention
>> in the LED_FUNCTION names, which is more common for Device Tree, and
>> some of existing LED triggers.
> 
> I am not sure what device tree has to do with it. ACPI for example
> likes all caps and sort names with numbers, but we do not let it
> propagate into the kernel.

For DT based LED class devices the LED name comes directly from DT label
property.

> We also should not be changing existing function names as existing
> userspace relies on them.

This is obvious. I meant making it configurable via kernel config
e.g.: CONFIG_LEDS_LEGACY_NAMES.

>> W could also think of placing common LED_FUNCTION definitions in
>> include/leds/led-functions.h and include it in both include/leds/leds.h
>> and include/dt-bindings/leds/common.h, so that they would be more
>> naturally accessible for non DT based drivers.
> 
> This makes total sense.
> 
> Thanks.
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v5 3/3] platform/chrome: Standardize Chrome OS keyboard backlight name

[Nick Crews]

I've just found a few [embarrassing :)] bugs in this version,
so after we figure out the naming, please wait for me to send
out another patch that fixes these.

Thanks, Nick

On Thu, Apr 4, 2019 at 11:10 AM Nick Crews <ncrews@chromium.org> wrote:
>
> We want all backlights for the system keyboard to
> use a common name, so the name "platform::kbd_backlight"
> would be better than the current "chromeos::kbd_backlight"
> name. Normally this wouldn't be worth changing, but the new
> Wilco keyboard backlight driver uses the "platform" name.
> We want to make it so all Chrome OS devices are consistent,
> so we'll change the name here too.
>
> The Power Manager daemon only looks for LEDs that match the
> pattern "*:kbd_backlight", so this change won't affect that.
>
> Signed-off-by: Nick Crews <ncrews@chromium.org>
> ---
>  drivers/platform/chrome/cros_kbd_led_backlight.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/platform/chrome/cros_kbd_led_backlight.c b/drivers/platform/chrome/cros_kbd_led_backlight.c
> index aa409f0201fb..c56c8405f39c 100644
> --- a/drivers/platform/chrome/cros_kbd_led_backlight.c
> +++ b/drivers/platform/chrome/cros_kbd_led_backlight.c
> @@ -77,7 +77,7 @@ static int keyboard_led_probe(struct platform_device *pdev)
>         if (!cdev)
>                 return -ENOMEM;
>
> -       cdev->name = "chromeos::kbd_backlight";
> +       cdev->name = "platform::kbd_backlight";
>         cdev->max_brightness = ACPI_KEYBOARD_BACKLIGHT_MAX;
>         cdev->flags |= LED_CORE_SUSPENDRESUME;
>         cdev->brightness_set = keyboard_led_set_brightness;
> --
> 2.20.1
>