linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface
@ 2019-04-03  2:05 Nick Crews
  2019-04-03  2:05 ` [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support Nick Crews
  2019-04-03 14:39 ` [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Enric Balletbo i Serra
  0 siblings, 2 replies; 6+ messages in thread
From: Nick Crews @ 2019-04-03  2:05 UTC (permalink / raw)
  To: enric.balletbo, bleung, linux-leds, jacek.anaszewski, pavel
  Cc: linux-kernel, dlaurie, sjg, groeck, dtor, 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>
---
 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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support
  2019-04-03  2:05 [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Nick Crews
@ 2019-04-03  2:05 ` Nick Crews
  2019-04-03 15:09   ` Enric Balletbo i Serra
  2019-04-03 17:28   ` Jacek Anaszewski
  2019-04-03 14:39 ` [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Enric Balletbo i Serra
  1 sibling, 2 replies; 6+ messages in thread
From: Nick Crews @ 2019-04-03  2:05 UTC (permalink / raw)
  To: enric.balletbo, bleung, linux-leds, jacek.anaszewski, pavel
  Cc: linux-kernel, dlaurie, sjg, groeck, dtor, 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/chromeos::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.

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>
---
 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       | 167 ++++++++++++++++++
 include/linux/platform_data/wilco-ec.h        |  38 ++++
 5 files changed, 274 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..922ac8717b23
--- /dev/null
+++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
@@ -0,0 +1,167 @@
+// 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/chromeos::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 = led_set_brightness_sync(&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 = "chromeos::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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface
  2019-04-03  2:05 [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Nick Crews
  2019-04-03  2:05 ` [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support Nick Crews
@ 2019-04-03 14:39 ` Enric Balletbo i Serra
  2019-04-03 14:47   ` Alexandre Belloni
  1 sibling, 1 reply; 6+ messages in thread
From: Enric Balletbo i Serra @ 2019-04-03 14:39 UTC (permalink / raw)
  To: Nick Crews, bleung, linux-leds, jacek.anaszewski, pavel
  Cc: linux-kernel, dlaurie, sjg, groeck, dtor, Alessandro Zummo,
	Alexandre Belloni, linux-rtc

Hi Nick,

On 3/4/19 4:05, Nick Crews wrote:
> 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>
> ---
>  drivers/platform/chrome/wilco_ec/debugfs.c | 43 ++++++++-------
>  drivers/platform/chrome/wilco_ec/mailbox.c | 53 ++++++++----------
>  drivers/rtc/rtc-wilco-ec.c                 | 63 +++++++++++++---------

I'd like to see an ack from rtc maintainer before merging this. Cc'ing the RTC
maintainers and the rtc mailing list.

The patch looks good to me. I think that this can go all through the
platform/chrome tree if the RTC maintainers are agree.

>  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;

Add and empty line, don't need to resend for this, I can fix myself when I merge.

> +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;
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface
  2019-04-03 14:39 ` [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Enric Balletbo i Serra
@ 2019-04-03 14:47   ` Alexandre Belloni
  0 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2019-04-03 14:47 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Nick Crews, bleung, linux-leds, jacek.anaszewski, pavel,
	linux-kernel, dlaurie, sjg, groeck, dtor, Alessandro Zummo,
	linux-rtc

On 03/04/2019 16:39:35+0200, Enric Balletbo i Serra wrote:
> Hi Nick,
> 
> On 3/4/19 4:05, Nick Crews wrote:
> > 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 +++++++++++++---------
> 
> I'd like to see an ack from rtc maintainer before merging this. Cc'ing the RTC
> maintainers and the rtc mailing list.
> 
> The patch looks good to me. I think that this can go all through the
> platform/chrome tree if the RTC maintainers are agree.
> 
> >  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;
> 
> Add and empty line, don't need to resend for this, I can fix myself when I merge.
> 
> > +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;
> > 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support
  2019-04-03  2:05 ` [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support Nick Crews
@ 2019-04-03 15:09   ` Enric Balletbo i Serra
  2019-04-03 17:28   ` Jacek Anaszewski
  1 sibling, 0 replies; 6+ messages in thread
From: Enric Balletbo i Serra @ 2019-04-03 15:09 UTC (permalink / raw)
  To: Nick Crews, bleung, linux-leds, jacek.anaszewski, pavel
  Cc: linux-kernel, dlaurie, sjg, groeck, dtor

Hi Nick,

On 3/4/19 4:05, 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/chromeos::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.
> 
> 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>
> ---
>  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       | 167 ++++++++++++++++++

I'm fine with this patch, I'll wait to see if any of the LEDS subsystem
maintainer can give you an ack or review.

Thanks,
 Enric


>  include/linux/platform_data/wilco-ec.h        |  38 ++++
>  5 files changed, 274 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?

Typo s/keyboad/keyboard/ don't need to resend for this.

> + * @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..922ac8717b23
> --- /dev/null
> +++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
> @@ -0,0 +1,167 @@
> +// 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/chromeos::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 = led_set_brightness_sync(&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 = "chromeos::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.
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support
  2019-04-03  2:05 ` [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support Nick Crews
  2019-04-03 15:09   ` Enric Balletbo i Serra
@ 2019-04-03 17:28   ` Jacek Anaszewski
  1 sibling, 0 replies; 6+ messages in thread
From: Jacek Anaszewski @ 2019-04-03 17:28 UTC (permalink / raw)
  To: Nick Crews, enric.balletbo, bleung, linux-leds, pavel
  Cc: linux-kernel, dlaurie, sjg, groeck, dtor

Hi Nick,

Thank you for the patch.

I have one comment for the kbd_led_backlight.c

On 4/3/19 4:05 AM, 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/chromeos::kbd_backlight. This driver is modeled
> after the standard Chrome OS keyboard backlight driver at
> drivers/platform/chrome/cros_kbd_led_backlight.c
[...]
> +++ b/drivers/platform/chrome/wilco_ec/kbd_led_backlight.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
[,,,]
> +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 = led_set_brightness_sync(&data->keyboard, KBBL_DEFAULT_BRIGHTNESS);

This is a bit weird. led_set_brightness_sync() has been introduced
specifically for use cases when torch setting should have guaranteed
immediate effect like in case of
drivers/media/v4l2-core/v4l2-flash-led-class.c (this is actually its
sole user). It bypasses internal LED core workqueue. While its use
here is not incorrect per se, it is of no avail to use public LED
subsystem API just for calling brightness_set_blocking op initialized
also in this driver.

Just call keyboard_led_set_brightness() directly.

With that:

Acked-by: Jacek Anaszewski <jacek.anaszewski@gmail.com>

> +	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 = "chromeos::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.
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-04-03 17:28 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-03  2:05 [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Nick Crews
2019-04-03  2:05 ` [PATCH v3 2/2] platform/chrome: Add Wilco EC keyboard backlight LEDs support Nick Crews
2019-04-03 15:09   ` Enric Balletbo i Serra
2019-04-03 17:28   ` Jacek Anaszewski
2019-04-03 14:39 ` [PATCH v3 1/2] platform/chrome: wilco_ec: Standardize mailbox interface Enric Balletbo i Serra
2019-04-03 14:47   ` Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).