On Thu, Jun 16, 2016 at 04:38:17PM +0100, Lee Jones 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?

Hex would probably be clearer. I'll try to change that.

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.

I don't think your rule holds water: there are definitely cases where
defines/enums require (or at least are better with) additional comments.
Sentence-long identifier names are not very readable, but sometimes a
sentence of comment can help.

Anyway, I think two of the three are probably unnecessary, if you really
want to ask. The first (EC_PWM_TYPE_GENERIC) seems useful.

But then, how do you suggest handling this in conjunction with your
kerneldoc suggestion? IIRC, kerneldoc requires that if one
entry/field/parameter is documented, then all most be documented. So
avoid kerneldoc on the enum, and just use inline comments?

[snip the rest, which was discussed in other branches of this thread]

Brian