Lee,

On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones [off-list ref] wrote:

On Thu, 02 Jun 2016, Brian Norris wrote:

quoted

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>
---
v2: no change

 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..d673575e0ada 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, 65535 = 100% */
+#define EC_PWM_MAX_DUTY 65535

Any reason this isn't represented in hex, like we do normally?

quoted

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

Are these comments really necessary?  I'd recommend that if your
defines require comments, then they are not adequately named.  In this
case however, I'd suggest that they are and the comments are
superfluous.

quoted

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

Please use kerneldoc format.

quoted

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

Probably the reason for all of these non-kernel-isms is that this
isn't a kernel file.  From the top of the file:

 * NOTE: This file is copied verbatim from the ChromeOS EC Open Source
 * project in an attempt to make future updates easy to make.

So the source of truth for this file is
<https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.

Someone could probably submit a CL to that project to make it a little
more kernel-ish and then we'd have to see if the EC team would accept
such changes...


-Doug