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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Fri, Jun 17, 2016 at 09:06:35AM +0100, Lee Jones wrote:
> On Thu, 16 Jun 2016, Doug Anderson wrote:
> > On Thu, Jun 16, 2016 at 8:38 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > > On Thu, 02 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>
> > >> ---
> > >> 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?
> > >
> > >> +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.
> > >
> > >> +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.
> > >
> > >> +#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...
> 
> Hmmm... that kinda puts me in a difficult position.  Do I except
> non-kernel code, which does not conform to our stands?

I think usually code that doesn't adhere to kernel coding style ends up
in the staging tree until it's been cleaned up enough.

The rule doesn't quite apply here because cleaning up isn't the issue.
But I don't know if we have any best practices for this kind of thing.

One thing that I've seen done in the past is to have this kind of
cross-OS header generated from some sort of definition file (XML, ...)
with an output filter for all supported OSes. Is that something that
could perhaps be done here?

That said, this might not even be worth it in this case. While I see how
it makes sense to avoid work updating this file for various coding
styles, the content in this file defines an ABI, so really the only
changes will be additions, and once they're merged they become set in
stone anyway. The amount of work updating the header, even taking into
account different coding styles should be very low.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]