[PATCH v3 0/4] pwm: add support for ChromeOS EC PWM

[PATCH v3 0/4] pwm: add support for ChromeOS EC PWM

[Brian Norris]

Hi,

This is version 3 of my series to support the new ChromeOS EC PWM API, so we
can control, e.g., a PWM backlight when its PWM is attached to the EC. It uses
Boris's latest "atomic" hooks for the PWM API (i.e., the ->apply() callback),
which were recently merged.

It seems nice to have the cros_ec_cmd_xfer_status() helper, which we have
locally in the ChromeOS kernel, and which has been part of another larger patch
series:

https://lkml.org/lkml/2016/4/12/342

So I've picked it into this series as well. Tomeu has acknowledged his USB PD
series is on hold for now, so this should be OK.

Note that after some style bikeshedding, I proposed to put off rewriting the
entire cros_ec_commands.h header at the moment, due to the shared nature of
this file. I plan to follow up with this bug report:

https://bugs.chromium.org/p/chromium/issues/detail?id=621123

As this touches MFD (sort of), drivers/platform/chrome/, and drivers/pwm/, I'm
still not sure who it should all go through: Lee, Thierry, or Olof?

Please review.

Regards,
Brian

Change log (also documented in each patch):

v3:
 * fix some int->hex
 * fix a small bug in the handling of 'disabled' vs. 'duty_cycle == 0'
 * collect acks, tested-by

v2:
 * drop the "google,max-pwms" property
 * separate the cros_ec vs. PWM abstractions a little more clearly in the driver
 * remove dynamic kzalloc()'s and rely on on-stack memory instead

Brian Norris (3):
  mfd: cros_ec: add EC_PWM function definitions
  doc: dt: pwm: add binding for ChromeOS EC PWM
  pwm: add ChromeOS EC PWM driver

Tomeu Vizoso (1):
  mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

 .../devicetree/bindings/pwm/google,cros-ec-pwm.txt |  23 ++
 drivers/platform/chrome/cros_ec_proto.c            |  15 ++
 drivers/pwm/Kconfig                                |   7 +
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-cros-ec.c                          | 246 +++++++++++++++++++++
 include/linux/mfd/cros_ec.h                        |  18 ++
 include/linux/mfd/cros_ec_commands.h               |  31 +++
 7 files changed, 341 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
 create mode 100644 drivers/pwm/pwm-cros-ec.c

-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

From: Tomeu Vizoso <tomeu.vizoso@collabora.com>

So that callers of cros_ec_cmd_xfer don't have to repeat boilerplate
code when checking for errors from the EC side.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Reviewed-by: Benson Leung <bleung@chromium.org>
Signed-off-by: Brian Norris <briannorris@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
v3:
 * successfully spell success

v2:
 * no change
---
 drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
 include/linux/mfd/cros_ec.h             | 18 ++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index b6e161f71b26..ecc544c728f0 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -380,3 +380,18 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 	return ret;
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer);
+
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg)
+{
+	int ret;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret < 0)
+		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+	else if (msg->result != EC_RES_SUCCESS)
+		return -EECRESULT - msg->result;
+
+	return ret;
+}
+EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 64184d27e3cd..7671704aa2d5 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -40,6 +40,9 @@
 #define EC_MAX_REQUEST_OVERHEAD		1
 #define EC_MAX_RESPONSE_OVERHEAD	2
 
+/* ec_command return value for non-success result from EC */
+#define EECRESULT 1000
+
 /*
  * Command interface between EC and AP, for LPC, I2C and SPI interfaces.
  */
@@ -226,6 +229,21 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
 		     struct cros_ec_command *msg);
 
 /**
+ * cros_ec_cmd_xfer_status - Send a command to the ChromeOS EC
+ *
+ * This function is identical to cros_ec_cmd_xfer, except it returns success
+ * status only if both the command was transmitted successfully and the EC
+ * replied with success status. It's not necessary to check msg->result when
+ * using this function.
+ *
+ * @ec_dev: EC device
+ * @msg: Message to write
+ * @return: Num. of bytes transferred on success, <0 on failure
+ */
+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
+			    struct cros_ec_command *msg);
+
+/**
  * cros_ec_remove - Remove a ChromeOS EC
  *
  * Call this to deregister a ChromeOS EC, then clean up any private data.
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 2/4] mfd: cros_ec: add EC_PWM function definitions

[Brian Norris]

The EC_CMD_PWM_{GET,SET}_DUTY commands allow us to control a PWM that is
attached to the EC, rather than the main host SoC. The API provides
functionality-based (e.g., keyboard light, backlight) or index-based
addressing of the PWM(s). Duty cycles are represented by a 16-bit value,
where 0 maps to 0% duty cycle and U16_MAX maps to 100%. The period
cannot be controlled.

This command set is more generic than, e.g.,
EC_CMD_PWM_{GET,SET}_KEYBOARD_BACKLIGHT and could possibly used to
replace it on future products.

Let's update the command header to include the definitions.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v3:
 * convert from 65535 to 0xffff

v2:
 * no changes
---
 include/linux/mfd/cros_ec_commands.h | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index 13b630c10d4c..7e7a8d4b4551 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -949,6 +949,37 @@ struct ec_params_pwm_set_fan_duty {
 	uint32_t percent;
 } __packed;
 
+#define EC_CMD_PWM_SET_DUTY 0x25
+/* 16 bit duty cycle, 0xffff = 100% */
+#define EC_PWM_MAX_DUTY 0xffff
+
+enum ec_pwm_type {
+	/* All types, indexed by board-specific enum pwm_channel */
+	EC_PWM_TYPE_GENERIC = 0,
+	/* Keyboard backlight */
+	EC_PWM_TYPE_KB_LIGHT,
+	/* Display backlight */
+	EC_PWM_TYPE_DISPLAY_LIGHT,
+	EC_PWM_TYPE_COUNT,
+};
+
+struct ec_params_pwm_set_duty {
+	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
+	uint8_t pwm_type;  /* ec_pwm_type */
+	uint8_t index;     /* Type-specific index, or 0 if unique */
+} __packed;
+
+#define EC_CMD_PWM_GET_DUTY 0x26
+
+struct ec_params_pwm_get_duty {
+	uint8_t pwm_type;  /* ec_pwm_type */
+	uint8_t index;     /* Type-specific index, or 0 if unique */
+} __packed;
+
+struct ec_response_pwm_get_duty {
+	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
+} __packed;
+
 /*****************************************************************************/
 /*
  * Lightbar commands. This looks worse than it is. Since we only use one HOST
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 3/4] doc: dt: pwm: add binding for ChromeOS EC PWM

[Brian Norris]

The ChromeOS Embedded Controller can support controlling its attached
PWMs via its host-command interface. The number of supported PWMs varies
on a per-board basis, but we can autodetect this by checking the error
codes, so we don't need an extra property for this. And because the EC
only allows specifying the duty cycle and not the period, we don't
specify the period via pwm-cells, and instead have only support 1 cell
-- to specify the index.

Signed-off-by: Brian Norris <briannorris@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
---
v3:
 * add Rob's ack

v2: dropped the "google,max-pwms" property
---
 .../devicetree/bindings/pwm/google,cros-ec-pwm.txt | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
new file mode 100644
index 000000000000..472bd46ab5a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/google,cros-ec-pwm.txt
@@ -0,0 +1,23 @@
+* PWM controlled by ChromeOS EC
+
+Google's ChromeOS EC PWM is a simple PWM attached to the Embedded Controller
+(EC) and controlled via a host-command interface.
+
+An EC PWM node should be only found as a sub-node of the EC node (see
+Documentation/devicetree/bindings/mfd/cros-ec.txt).
+
+Required properties:
+- compatible: Must contain "google,cros-ec-pwm"
+- #pwm-cells: Should be 1. The cell specifies the PWM index.
+
+Example:
+	cros-ec@0 {
+		compatible = "google,cros-ec-spi";
+
+		...
+
+		cros_ec_pwm: ec-pwm {
+			compatible = "google,cros-ec-pwm";
+			#pwm-cells = <1>;
+		};
+	};
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 4/4] pwm: add ChromeOS EC PWM driver

[Brian Norris]

Use the new ChromeOS EC EC_CMD_PWM_{GET,SET}_DUTY commands to control
one or more PWMs attached to the Embedded Controller. Because the EC
allows us to modify the duty cycle (as a percentage, where U16_MAX is
100%) but not the period, we assign the period a fixed value of
EC_PWM_MAX_DUTY and reject all attempts to change it.

This driver supports only device tree at the moment, because that
provides a very flexible way of describing the relationship between PWMs
and their consumer devices (e.g., backlight). On a non-DT system, we'll
probably want to use the non-GENERIC addressing (i.e., we'll need to
make special device instances that will use EC_PWM_TYPE_KB_LIGHT or
EC_PWM_TYPE_DISPLAY_LIGHT), as well as the relatively inflexible
pwm_lookup infrastructure for matching devices. Defer that work for now.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v3:
 * handle 'disabled' properly in apply(), since EC conflates 0
   duty-cycle and disabled state

v2:
 * refactor some abstractions to separate the PWM layer handling from
   the cros_ec handling
 * remove dynamic kzalloc()'s and rely on on-stack memory instead
 * auto-probe the number of PWMs supported
---
 drivers/pwm/Kconfig       |   7 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-cros-ec.c | 246 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/pwm/pwm-cros-ec.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index c182efc62c7b..4f2b16a50f42 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -137,6 +137,13 @@ config PWM_CRC
 	  Generic PWM framework driver for Crystalcove (CRC) PMIC based PWM
 	  control.
 
+config PWM_CROS_EC
+	tristate "ChromeOS EC PWM driver"
+	depends on MFD_CROS_EC
+	help
+	  PWM driver for exposing a PWM attached to the ChromeOS Embedded
+	  Controller.
+
 config PWM_EP93XX
 	tristate "Cirrus Logic EP93xx PWM support"
 	depends on ARCH_EP93XX
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index dd35bc121a18..ffde923cf3df 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
+obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
new file mode 100644
index 000000000000..ec3bb8ec4e8a
--- /dev/null
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright (C) 2016 Google, Inc
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2, as published by
+ * the Free Software Foundation.
+ *
+ * Expose a PWM controlled by the ChromeOS EC to the host processor.
+ */
+
+#include <linux/module.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/slab.h>
+
+/**
+ * struct cros_ec_pwm_device - Driver data for EC PWM
+ *
+ * @dev: Device node
+ * @ec: Pointer to EC device
+ * @chip: PWM controller chip
+ */
+struct cros_ec_pwm_device {
+	struct device *dev;
+	struct cros_ec_device *ec;
+	struct pwm_chip chip;
+};
+
+static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
+{
+	return container_of(c, struct cros_ec_pwm_device, chip);
+}
+
+static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
+{
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_pwm_set_duty params;
+	} buf;
+	struct ec_params_pwm_set_duty *params = &buf.params;
+	struct cros_ec_command *msg = &buf.msg;
+
+	memset(&buf, 0, sizeof(buf));
+
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_SET_DUTY;
+	msg->insize = 0;
+	msg->outsize = sizeof(*params);
+
+	params->duty = duty;
+	params->pwm_type = EC_PWM_TYPE_GENERIC;
+	params->index = index;
+
+	return cros_ec_cmd_xfer_status(ec, msg);
+}
+
+static int cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index)
+{
+	struct {
+		struct cros_ec_command msg;
+		union {
+			struct ec_params_pwm_get_duty params;
+			struct ec_response_pwm_get_duty resp;
+		};
+	} buf;
+	struct ec_params_pwm_get_duty *params = &buf.params;
+	struct ec_response_pwm_get_duty *resp = &buf.resp;
+	struct cros_ec_command *msg = &buf.msg;
+	int ret;
+
+	memset(&buf, 0, sizeof(buf));
+
+	msg->version = 0;
+	msg->command = EC_CMD_PWM_GET_DUTY;
+	msg->insize = sizeof(*params);
+	msg->outsize = sizeof(*resp);
+
+	params->pwm_type = EC_PWM_TYPE_GENERIC;
+	params->index = index;
+
+	ret = cros_ec_cmd_xfer_status(ec, msg);
+	if (ret < 0)
+		return ret;
+
+	return resp->duty;
+}
+
+static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+	int duty_cycle;
+
+	/* The EC won't let us change the period */
+	if (state->period != EC_PWM_MAX_DUTY)
+		return -EINVAL;
+
+	/*
+	 * EC doesn't separate the concept of duty cycle and enabled, but
+	 * kernel does. Translate.
+	 */
+	duty_cycle = state->enabled ? state->duty_cycle : 0;
+
+	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
+}
+
+static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+	int ret;
+
+	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
+	if (ret < 0) {
+		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
+		return;
+	}
+
+	state->enabled = (ret > 0);
+	state->period = EC_PWM_MAX_DUTY;
+
+	/* Note that "disabled" and "duty cycle == 0" are treated the same */
+	state->duty_cycle = ret;
+}
+
+static struct pwm_device *
+cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
+{
+	struct pwm_device *pwm;
+
+	if (args->args[0] >= pc->npwm)
+		return ERR_PTR(-EINVAL);
+
+	pwm = pwm_request_from_chip(pc, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	/* The EC won't let us change the period */
+	pwm->args.period = EC_PWM_MAX_DUTY;
+
+	return pwm;
+}
+
+static const struct pwm_ops cros_ec_pwm_ops = {
+	.get_state	= cros_ec_pwm_get_state,
+	.apply		= cros_ec_pwm_apply,
+	.owner		= THIS_MODULE,
+};
+
+static int cros_ec_num_pwms(struct cros_ec_device *ec)
+{
+	int i, ret;
+
+	/* The index field is only 8 bits */
+	for (i = 0; i <= U8_MAX; i++) {
+		ret = cros_ec_pwm_get_duty(ec, i);
+		/*
+		 * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM responses;
+		 * everything else is treated as an error
+		 */
+		if (ret == -EECRESULT - EC_RES_INVALID_COMMAND)
+			return -ENODEV;
+		else if (ret == -EECRESULT - EC_RES_INVALID_PARAM)
+			return i;
+		else if (ret < 0)
+			return ret;
+	}
+
+	return U8_MAX;
+}
+
+static int cros_ec_pwm_probe(struct platform_device *pdev)
+{
+	struct cros_ec_device *ec = dev_get_drvdata(pdev->dev.parent);
+	struct device *dev = &pdev->dev;
+	struct cros_ec_pwm_device *ec_pwm;
+	struct pwm_chip *chip;
+	int ret;
+
+	if (!ec) {
+		dev_err(dev, "no parent EC device\n");
+		return -EINVAL;
+	}
+
+	ec_pwm = devm_kzalloc(dev, sizeof(*ec_pwm), GFP_KERNEL);
+	if (!ec_pwm)
+		return -ENOMEM;
+	chip = &ec_pwm->chip;
+	ec_pwm->ec = ec;
+
+	/* PWM chip */
+	chip->dev = dev;
+	chip->ops = &cros_ec_pwm_ops;
+	chip->of_xlate = cros_ec_pwm_xlate;
+	chip->of_pwm_n_cells = 1;
+	chip->base = -1;
+	ret = cros_ec_num_pwms(ec);
+	if (ret < 0) {
+		dev_err(dev, "Couldn't find PWMs: %d\n", ret);
+		return ret;
+	}
+	chip->npwm = ret;
+	dev_dbg(dev, "Probed %u PWMs\n", chip->npwm);
+
+	ret = pwmchip_add(chip);
+	if (ret < 0) {
+		dev_err(dev, "cannot register PWM: %d\n", ret);
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, ec_pwm);
+
+	return ret;
+}
+
+static int cros_ec_pwm_remove(struct platform_device *dev)
+{
+	struct cros_ec_pwm_device *ec_pwm = platform_get_drvdata(dev);
+	struct pwm_chip *chip = &ec_pwm->chip;
+
+	return pwmchip_remove(chip);
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id cros_ec_pwm_of_match[] = {
+	{ .compatible = "google,cros-ec-pwm" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, cros_ec_pwm_of_match);
+#endif
+
+static struct platform_driver cros_ec_pwm_driver = {
+	.probe = cros_ec_pwm_probe,
+	.remove = cros_ec_pwm_remove,
+	.driver = {
+		.name = "cros-ec-pwm",
+		.of_match_table = of_match_ptr(cros_ec_pwm_of_match),
+	},
+};
+module_platform_driver(cros_ec_pwm_driver);
+
+MODULE_ALIAS("platform:cros-ec-pwm");
+MODULE_DESCRIPTION("ChromeOS EC PWM driver");
+MODULE_LICENSE("GPL v2");
-- 
2.8.0.rc3.226.g39d4020

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Guenter Roeck]

On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
> From: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> 
> So that callers of cros_ec_cmd_xfer don't have to repeat boilerplate
> code when checking for errors from the EC side.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Reviewed-by: Benson Leung <bleung@chromium.org>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> v3:
>  * successfully spell success
> 
> v2:
>  * no change
> ---
>  drivers/platform/chrome/cros_ec_proto.c | 15 +++++++++++++++
>  include/linux/mfd/cros_ec.h             | 18 ++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index b6e161f71b26..ecc544c728f0 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -380,3 +380,18 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
>  	return ret;
>  }
>  EXPORT_SYMBOL(cros_ec_cmd_xfer);
> +
> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> +			    struct cros_ec_command *msg)
> +{
> +	int ret;
> +
> +	ret = cros_ec_cmd_xfer(ec_dev, msg);
> +	if (ret < 0)
> +		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> +	else if (msg->result != EC_RES_SUCCESS)
> +		return -EECRESULT - msg->result;

I have been wondering about the error return codes here, and if they should be
converted to standard Linux error codes. For example, I just hit error -1003
with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
in Linux terms, -EINVAL. I think it would be better to use standard error
codes, especially since some of the errors are logged.

Guenter

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
> > +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> > +			    struct cros_ec_command *msg)
> > +{
> > +	int ret;
> > +
> > +	ret = cros_ec_cmd_xfer(ec_dev, msg);
> > +	if (ret < 0)
> > +		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> > +	else if (msg->result != EC_RES_SUCCESS)
> > +		return -EECRESULT - msg->result;
> 
> I have been wondering about the error return codes here, and if they should be
> converted to standard Linux error codes. For example, I just hit error -1003
> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
> in Linux terms, -EINVAL. I think it would be better to use standard error
> codes, especially since some of the errors are logged.

How do you propose we do that? Do all of the following become EINVAL?

        EC_RES_INVALID_COMMAND
        EC_RES_INVALID_PARAM
        EC_RES_INVALID_VERSION
        EC_RES_INVALID_HEADER

We lose a lot of information that way. And particularly, cros_ec_num_pwms()
won't be able to count as accurately. But I can just go back to not using this
API if that's what you'd like...

Brian

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Guenter Roeck]

On 06/17/2016 06:08 PM, Brian Norris wrote:
> On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
>> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
>>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>>> +			    struct cros_ec_command *msg)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = cros_ec_cmd_xfer(ec_dev, msg);
>>> +	if (ret < 0)
>>> +		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
>>> +	else if (msg->result != EC_RES_SUCCESS)
>>> +		return -EECRESULT - msg->result;
>>
>> I have been wondering about the error return codes here, and if they should be
>> converted to standard Linux error codes. For example, I just hit error -1003
>> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
>> in Linux terms, -EINVAL. I think it would be better to use standard error
>> codes, especially since some of the errors are logged.
>
> How do you propose we do that? Do all of the following become EINVAL?
>
>          EC_RES_INVALID_COMMAND
>          EC_RES_INVALID_PARAM
>          EC_RES_INVALID_VERSION
>          EC_RES_INVALID_HEADER
>

Personal preference, but yes.

> We lose a lot of information that way. And particularly, cros_ec_num_pwms()
> won't be able to count as accurately. But I can just go back to not using this

You lost me there, sorry.

> API if that's what you'd like...
>

That isn't what I suggested.

Guenter

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Guenter Roeck]

On 06/17/2016 06:08 PM, Brian Norris wrote:
> On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
>> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
>>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>>> +			    struct cros_ec_command *msg)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = cros_ec_cmd_xfer(ec_dev, msg);
>>> +	if (ret < 0)
>>> +		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
>>> +	else if (msg->result != EC_RES_SUCCESS)
>>> +		return -EECRESULT - msg->result;
>>
>> I have been wondering about the error return codes here, and if they should be
>> converted to standard Linux error codes. For example, I just hit error -1003
>> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
>> in Linux terms, -EINVAL. I think it would be better to use standard error
>> codes, especially since some of the errors are logged.
>
> How do you propose we do that? Do all of the following become EINVAL?
>
>          EC_RES_INVALID_COMMAND

	-EOPNOTSUPP

>          EC_RES_INVALID_PARAM

	-EINVAL or -EBADMSG

>          EC_RES_INVALID_VERSION

	-EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT

>          EC_RES_INVALID_HEADER

	-EPROTO or -EBADR or -EBADE

Doesn't look that bad to me. Also, the raw error could still be logged,
for example with dev_dbg().

Guenter

>
> We lose a lot of information that way. And particularly, cros_ec_num_pwms()
> won't be able to count as accurately. But I can just go back to not using this
> API if that's what you'd like...
>
> Brian
>

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Javier Martinez Canillas]

Hello Guenter, Brian

On 06/18/2016 01:09 PM, Guenter Roeck wrote:
> On 06/17/2016 06:08 PM, Brian Norris wrote:
>> On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
>>> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
>>>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
>>>> +                struct cros_ec_command *msg)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = cros_ec_cmd_xfer(ec_dev, msg);
>>>> +    if (ret < 0)
>>>> +        dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
>>>> +    else if (msg->result != EC_RES_SUCCESS)
>>>> +        return -EECRESULT - msg->result;
>>>
>>> I have been wondering about the error return codes here, and if they should be
>>> converted to standard Linux error codes. For example, I just hit error -1003
>>> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
>>> in Linux terms, -EINVAL. I think it would be better to use standard error
>>> codes, especially since some of the errors are logged.
>>

Agreed, specially since drivers may (wrongly) propagate whatever is returned
by this function to higher layers where the ChromeOS EC firmware error codes
makes no sense. So that will be a bug and can increase the cognitive load of
getting some weird error codes in core kernel code and developers may wonder
from where those came from until finally find that a EC driver returned that.

>> How do you propose we do that? Do all of the following become EINVAL?
>>

Yes, I would just do that.

The idea of this helper is to remove duplicated code and AFAICT what most EC
drivers do is something similar to the following:

        ret = cros_ec_cmd_xfer(ec, msg);
        if (ret < 0)
	   	return ret;

        if (msg->result != EC_RES_SUCCESS) {
                dev_dbg(ec->dev, "EC result %d\n", msg->result);
                return -EINVAL;
        }

So in practice what most drivers really care is if the result was successful
or not, I don't see specific EC error handling in the EC drivers.  The real
EC error code is still in the message anyways so drivers that do cares about
the real EC error can look at msg->result instead.

>>          EC_RES_INVALID_COMMAND
> 
>     -EOPNOTSUPP
> 
>>          EC_RES_INVALID_PARAM
> 
>     -EINVAL or -EBADMSG
> 
>>          EC_RES_INVALID_VERSION
> 
>     -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT
> 
>>          EC_RES_INVALID_HEADER
> 
>     -EPROTO or -EBADR or -EBADE
> 
> Doesn't look that bad to me. Also, the raw error could still be logged,
> for example with dev_dbg().
>

Yes, I think that adding a dev_dbg() with the real EC error code should
be enough, that's basically what drivers do since they can't propagate
the EC error to higher layers anyways.

> Guenter
> 
>>

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

On Fri, Jun 17, 2016 at 09:12:20PM -0700, Guenter Roeck wrote:
> On 06/17/2016 06:08 PM, Brian Norris wrote:
> >On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
> >>On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
> >>>+int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> >>>+			    struct cros_ec_command *msg)
> >>>+{
> >>>+	int ret;
> >>>+
> >>>+	ret = cros_ec_cmd_xfer(ec_dev, msg);
> >>>+	if (ret < 0)
> >>>+		dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> >>>+	else if (msg->result != EC_RES_SUCCESS)
> >>>+		return -EECRESULT - msg->result;
> >>
> >>I have been wondering about the error return codes here, and if they should be
> >>converted to standard Linux error codes. For example, I just hit error -1003
> >>with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
> >>in Linux terms, -EINVAL. I think it would be better to use standard error
> >>codes, especially since some of the errors are logged.
> >
> >How do you propose we do that? Do all of the following become EINVAL?
> >
> >         EC_RES_INVALID_COMMAND
> >         EC_RES_INVALID_PARAM
> >         EC_RES_INVALID_VERSION
> >         EC_RES_INVALID_HEADER
> >
> 
> Personal preference, but yes.
> 
> >We lose a lot of information that way. And particularly, cros_ec_num_pwms()
> >won't be able to count as accurately. But I can just go back to not using this
> 
> You lost me there, sorry.

If you look at the currently-proposed user of this API (patch 4), some
of the code in cros_ec_num_pwms() looks like this:

               /*
                * We look for SUCCESS, INVALID_COMMAND, or INVALID_PARAM responses;
                * everything else is treated as an error
                */
               if (ret == -EECRESULT - EC_RES_INVALID_COMMAND)
                       return -ENODEV;
               else if (ret == -EECRESULT - EC_RES_INVALID_PARAM)
                       return i;
               else if (ret < 0)
                       return ret;

I'd really like to know the difference between EC_RES_INVALID_COMMAND,
EC_RES_INVALID_PARAM, and all other error codes. You're suggesting
aliasing those with generic Linux error codes which could easily be used
for other things (e.g., reporting errors from the I2C or SPI layers,
which I'd want to treat differently). So either I decrease the
preciseness of the above code, or I just use cros_ec_cmd_xfer() instead
of cros_ec_cmd_xfer_status(), and then have to reimplement the error
handling that cros_ec_cmd_xfer_status() was going to (sort of) abstract
away.

But maybe that just means that pwm-cros-ec.c is really the odd-man-out
use case compared to the other users of cros_ec_cmd_xfer_status() in the
Chrome OS kernel tree, and so I should be using cros_ec_cmd_xfer().

> >API if that's what you'd like...
> >
> 
> That isn't what I suggested.

Brian

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

Hi,

On Mon, Jun 20, 2016 at 09:46:57AM -0400, Javier Martinez Canillas wrote:
> On 06/18/2016 01:09 PM, Guenter Roeck wrote:
> > On 06/17/2016 06:08 PM, Brian Norris wrote:
> >> On Fri, Jun 17, 2016 at 02:41:51PM -0700, Guenter Roeck wrote:
> >>> On Fri, Jun 17, 2016 at 12:58:12PM -0700, Brian Norris wrote:
> >>>> +int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
> >>>> +                struct cros_ec_command *msg)
> >>>> +{
> >>>> +    int ret;
> >>>> +
> >>>> +    ret = cros_ec_cmd_xfer(ec_dev, msg);
> >>>> +    if (ret < 0)
> >>>> +        dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
> >>>> +    else if (msg->result != EC_RES_SUCCESS)
> >>>> +        return -EECRESULT - msg->result;
> >>>
> >>> I have been wondering about the error return codes here, and if they should be
> >>> converted to standard Linux error codes. For example, I just hit error -1003
> >>> with a driver I am working on. This translates to EC_RES_INVALID_PARAM, or,
> >>> in Linux terms, -EINVAL. I think it would be better to use standard error
> >>> codes, especially since some of the errors are logged.
> >>
> 
> Agreed, specially since drivers may (wrongly) propagate whatever is returned
> by this function to higher layers where the ChromeOS EC firmware error codes
> makes no sense. So that will be a bug and can increase the cognitive load of
> getting some weird error codes in core kernel code and developers may wonder
> from where those came from until finally find that a EC driver returned that.

Agreed, I suppose. It's probably best to make it unlikely other that
"client" drivers of cros-ec will blindly propagate nonstandard error
codes.

> >> How do you propose we do that? Do all of the following become EINVAL?
> >>
> 
> Yes, I would just do that.
> 
> The idea of this helper is to remove duplicated code and AFAICT what most EC
> drivers do is something similar to the following:
> 
>         ret = cros_ec_cmd_xfer(ec, msg);
>         if (ret < 0)
> 	   	return ret;
> 
>         if (msg->result != EC_RES_SUCCESS) {
>                 dev_dbg(ec->dev, "EC result %d\n", msg->result);
>                 return -EINVAL;
>         }
> 
> So in practice what most drivers really care is if the result was successful
> or not, I don't see specific EC error handling in the EC drivers.  The real
> EC error code is still in the message anyways so drivers that do cares about
> the real EC error can look at msg->result instead.

Ah, well that last point is a nice reminder I guess, although that still
doesn't fit the way cros_ec_num_pwms() uses this API. But I can try to
refactor it to fit; cros_ec_num_pwms() will need to get two return
codes from cros_ec_pwm_get_duty() -- uglier, IMO, but doable.

> >>          EC_RES_INVALID_COMMAND
> > 
> >     -EOPNOTSUPP
> > 
> >>          EC_RES_INVALID_PARAM
> > 
> >     -EINVAL or -EBADMSG
> > 
> >>          EC_RES_INVALID_VERSION
> > 
> >     -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT
> > 
> >>          EC_RES_INVALID_HEADER
> > 
> >     -EPROTO or -EBADR or -EBADE
> > 
> > Doesn't look that bad to me. Also, the raw error could still be logged,
> > for example with dev_dbg().
> >
> 
> Yes, I think that adding a dev_dbg() with the real EC error code should
> be enough, that's basically what drivers do since they can't propagate
> the EC error to higher layers anyways.

I'll take a look at adding an error code translation table when I get a
chance. Hopefully that doesn't delay the others who are planning to use
this API shortly...

Brian

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Brian Norris]

Hi,

On Mon, Jun 20, 2016 at 10:44:55AM -0700, Brian Norris wrote:
> On Mon, Jun 20, 2016 at 09:46:57AM -0400, Javier Martinez Canillas wrote:
> > On 06/18/2016 01:09 PM, Guenter Roeck wrote:
> > > On 06/17/2016 06:08 PM, Brian Norris wrote:
> > >> How do you propose we do that? Do all of the following become EINVAL?
> > >>
> > 
> > Yes, I would just do that.
> > 
> > The idea of this helper is to remove duplicated code and AFAICT what most EC
> > drivers do is something similar to the following:
> > 
> >         ret = cros_ec_cmd_xfer(ec, msg);
> >         if (ret < 0)
> > 	   	return ret;
> > 
> >         if (msg->result != EC_RES_SUCCESS) {
> >                 dev_dbg(ec->dev, "EC result %d\n", msg->result);
> >                 return -EINVAL;
> >         }
> > 
> > So in practice what most drivers really care is if the result was successful
> > or not, I don't see specific EC error handling in the EC drivers.  The real
> > EC error code is still in the message anyways so drivers that do cares about
> > the real EC error can look at msg->result instead.
[...]
> > >>          EC_RES_INVALID_COMMAND
> > > 
> > >     -EOPNOTSUPP
> > > 
> > >>          EC_RES_INVALID_PARAM
> > > 
> > >     -EINVAL or -EBADMSG
> > > 
> > >>          EC_RES_INVALID_VERSION
> > > 
> > >     -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT
> > > 
> > >>          EC_RES_INVALID_HEADER
> > > 
> > >     -EPROTO or -EBADR or -EBADE
> > > 
> > > Doesn't look that bad to me. Also, the raw error could still be logged,
> > > for example with dev_dbg().
> > >
> > 
> > Yes, I think that adding a dev_dbg() with the real EC error code should
> > be enough, that's basically what drivers do since they can't propagate
> > the EC error to higher layers anyways.
> 
> I'll take a look at adding an error code translation table when I get a
> chance. Hopefully that doesn't delay the others who are planning to use
> this API shortly...

Actually, I had some second thoughts, and others brought similar
concerns up to me:

What do we really win by doing the translation? It'll be difficult to do
a 1:1 translation, and any time the EC adds additional error types,
we'll have to update the translation. What's more, AFAICT, no one will
really be looking at the translated error codes now, and will just be
blindly passing them on. So maybe it makes more sense to just pick a
single error code and pass that on instead. Possibly just -EPROTO (or
nominate your favorite) for all EC error results, and if someone cares,
they decode msg->result in their driver, or check the dev_dbg() log?

Brian

Re: [v3,1/4] mfd: cros_ec: Add cros_ec_cmd_xfer_status helper

[Javier Martinez Canillas]

Hello Brian,

On 06/20/2016 02:10 PM, Brian Norris wrote:
> Hi,
> 
> On Mon, Jun 20, 2016 at 10:44:55AM -0700, Brian Norris wrote:
>> On Mon, Jun 20, 2016 at 09:46:57AM -0400, Javier Martinez Canillas wrote:
>>> On 06/18/2016 01:09 PM, Guenter Roeck wrote:
>>>> On 06/17/2016 06:08 PM, Brian Norris wrote:
>>>>> How do you propose we do that? Do all of the following become EINVAL?
>>>>>
>>>
>>> Yes, I would just do that.
>>>
>>> The idea of this helper is to remove duplicated code and AFAICT what most EC
>>> drivers do is something similar to the following:
>>>
>>>         ret = cros_ec_cmd_xfer(ec, msg);
>>>         if (ret < 0)
>>> 	   	return ret;
>>>
>>>         if (msg->result != EC_RES_SUCCESS) {
>>>                 dev_dbg(ec->dev, "EC result %d\n", msg->result);
>>>                 return -EINVAL;
>>>         }
>>>
>>> So in practice what most drivers really care is if the result was successful
>>> or not, I don't see specific EC error handling in the EC drivers.  The real
>>> EC error code is still in the message anyways so drivers that do cares about
>>> the real EC error can look at msg->result instead.
> [...]
>>>>>          EC_RES_INVALID_COMMAND
>>>>
>>>>     -EOPNOTSUPP
>>>>
>>>>>          EC_RES_INVALID_PARAM
>>>>
>>>>     -EINVAL or -EBADMSG
>>>>
>>>>>          EC_RES_INVALID_VERSION
>>>>
>>>>     -EPROTO or -EBADR or -EBADE or -EBADRQC or -EPROTOOPT
>>>>
>>>>>          EC_RES_INVALID_HEADER
>>>>
>>>>     -EPROTO or -EBADR or -EBADE
>>>>
>>>> Doesn't look that bad to me. Also, the raw error could still be logged,
>>>> for example with dev_dbg().
>>>>
>>>
>>> Yes, I think that adding a dev_dbg() with the real EC error code should
>>> be enough, that's basically what drivers do since they can't propagate
>>> the EC error to higher layers anyways.
>>
>> I'll take a look at adding an error code translation table when I get a
>> chance. Hopefully that doesn't delay the others who are planning to use
>> this API shortly...
> 
> Actually, I had some second thoughts, and others brought similar
> concerns up to me:
> 
> What do we really win by doing the translation? It'll be difficult to do
> a 1:1 translation, and any time the EC adds additional error types,
> we'll have to update the translation. What's more, AFAICT, no one will
> really be looking at the translated error codes now, and will just be
> blindly passing them on. So maybe it makes more sense to just pick a
> single error code and pass that on instead. Possibly just -EPROTO (or
> nominate your favorite) for all EC error results, and if someone cares,


I think I didn't make clear in my previous email but yes, I also think
that a translate table for errors is not a good idea and that just a
single -EINVAL (or another error if is more suitable for all EC errors)
will be enough. I say -EINVAL because that's what you first mentioned
in a previous email and that's what most EC clients drivers are using.

I do believe that EC specific errors shouldn't be returned in a Linux
function though for the reasons I explained before.

> they decode msg->result in their driver, or check the dev_dbg() log?
>

And yes, I also think that a dev_dbg() is a good idea (the more data
we have about an error, the better) and as mentioned the EC error can
always be found in the EC message result field.
 
> Brian
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [PATCH v3 2/4] mfd: cros_ec: add EC_PWM function definitions

[Lee Jones]

On Fri, 17 Jun 2016, Brian Norris wrote:

> The EC_CMD_PWM_{GET,SET}_DUTY commands allow us to control a PWM that is
> attached to the EC, rather than the main host SoC. The API provides
> functionality-based (e.g., keyboard light, backlight) or index-based
> addressing of the PWM(s). Duty cycles are represented by a 16-bit value,
> where 0 maps to 0% duty cycle and U16_MAX maps to 100%. The period
> cannot be controlled.
> 
> This command set is more generic than, e.g.,
> EC_CMD_PWM_{GET,SET}_KEYBOARD_BACKLIGHT and could possibly used to
> replace it on future products.
> 
> Let's update the command header to include the definitions.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> v3:
>  * convert from 65535 to 0xffff
> 
> v2:
>  * no changes
> ---
>  include/linux/mfd/cros_ec_commands.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)

For my own reference:
  Acked-by: Lee Jones <lee.jones@linaro.org>
  
> diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
> index 13b630c10d4c..7e7a8d4b4551 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h
> @@ -949,6 +949,37 @@ struct ec_params_pwm_set_fan_duty {
>  	uint32_t percent;
>  } __packed;
>  
> +#define EC_CMD_PWM_SET_DUTY 0x25
> +/* 16 bit duty cycle, 0xffff = 100% */
> +#define EC_PWM_MAX_DUTY 0xffff
> +
> +enum ec_pwm_type {
> +	/* All types, indexed by board-specific enum pwm_channel */
> +	EC_PWM_TYPE_GENERIC = 0,
> +	/* Keyboard backlight */
> +	EC_PWM_TYPE_KB_LIGHT,
> +	/* Display backlight */
> +	EC_PWM_TYPE_DISPLAY_LIGHT,
> +	EC_PWM_TYPE_COUNT,
> +};
> +
> +struct ec_params_pwm_set_duty {
> +	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
> +	uint8_t pwm_type;  /* ec_pwm_type */
> +	uint8_t index;     /* Type-specific index, or 0 if unique */
> +} __packed;
> +
> +#define EC_CMD_PWM_GET_DUTY 0x26
> +
> +struct ec_params_pwm_get_duty {
> +	uint8_t pwm_type;  /* ec_pwm_type */
> +	uint8_t index;     /* Type-specific index, or 0 if unique */
> +} __packed;
> +
> +struct ec_response_pwm_get_duty {
> +	uint16_t duty;     /* Duty cycle, EC_PWM_MAX_DUTY = 100% */
> +} __packed;
> +
>  /*****************************************************************************/
>  /*
>   * Lightbar commands. This looks worse than it is. Since we only use one HOST

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog