Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1424202 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Lee Jones <lee.jones@linaro.org> - 2016-06-16 17:40 +0200
    Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Doug Anderson <dianders@chromium.org> - 2016-06-16 18:00 +0200
      Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Lee Jones <lee.jones@linaro.org> - 2016-06-17 10:10 +0200
        Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Doug Anderson <dianders@chromium.org> - 2016-06-17 17:30 +0200
          Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Lee Jones <lee.jones@linaro.org> - 2016-06-20 10:10 +0200
            Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Brian Norris <briannorris@chromium.org> - 2016-06-20 20:00 +0200
        Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Thierry Reding <thierry.reding@gmail.com> - 2016-06-17 18:00 +0200
          Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Brian Norris <briannorris@chromium.org> - 2016-06-17 21:20 +0200
    Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Brian Norris <briannorris@chromium.org> - 2016-06-17 21:30 +0200
      Re: [PATCH v2 2/4] mfd: cros_ec: add EC_PWM function definitions Lee Jones <lee.jones@linaro.org> - 2016-06-20 09:50 +0200

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

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;

As above.

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

[toc] | [next] | [standalone]

#1424222

Lee,

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


-Doug

[toc] | [prev] | [next] | [standalone]

#1424787

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?

Naturally I'd be happier if you could try to make the code more
'kernely'.  The practices I mention above are still good ones, even if
you're not writing kernel specific code.

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

[toc] | [prev] | [next] | [standalone]

#1425216

Lee,

On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
>> 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?

What about if Brian made sure to just fully copy the latest version of
"cros_ec_commands.h" from the EC codebase and changed this commit
message to say:

Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
code base, which is the source of truth for this file.  See
<https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.

From the commit message it would be clear that this is an external
file linked into the kernel for convenience.


> Naturally I'd be happier if you could try to make the code more
> 'kernely'.  The practices I mention above are still good ones, even if
> you're not writing kernel specific code.

In general requesting that code from outside the kernel conform to
"kerneldoc" seems like a bit of a stretch.  In general having some
type of parse-able format for comments is nice, but I could see that
in the Chrome OS EC codebase it would be a bit overkill.


Also: it would be awfully strange if we suddenly started changing the
coding convention of this file or we had half the file in one
convention and half in another.  The rest of this file is in EC
convention and it seems sane to keep it that way...


-Doug

[toc] | [prev] | [next] | [standalone]

#1426314

On Fri, 17 Jun 2016, Doug Anderson wrote:

> Lee,
> 
> On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
> >> 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?
> 
> What about if Brian made sure to just fully copy the latest version of
> "cros_ec_commands.h" from the EC codebase and changed this commit
> message to say:
> 
> Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
> code base, which is the source of truth for this file.  See
> <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> 
> From the commit message it would be clear that this is an external
> file linked into the kernel for convenience.
> 
> 
> > Naturally I'd be happier if you could try to make the code more
> > 'kernely'.  The practices I mention above are still good ones, even if
> > you're not writing kernel specific code.
> 
> In general requesting that code from outside the kernel conform to
> "kerneldoc" seems like a bit of a stretch.  In general having some
> type of parse-able format for comments is nice, but I could see that
> in the Chrome OS EC codebase it would be a bit overkill.

It's unfortunate that kerneldoc is named so, since I think it's a nice
way to write structure/function headers regardless of code base and
not overkill at all.  It's certainly harder to convince !kernel code
to use the format, or at least easier for others to push back due to
the fact that is has 'kernel' in the name.

> Also: it would be awfully strange if we suddenly started changing the
> coding convention of this file or we had half the file in one
> convention and half in another.  The rest of this file is in EC
> convention and it seems sane to keep it that way...

Right.  It's also a shame we're only catching this now.  Really we
should have had this discussion in the first instance.

Taking into consideration that this file is already in the kernel and
that it's current format is also represented, I'm willing to keep
adding to it.  I would like to see an internal request to adopt
so-called kerneldoc.  Not because I am wish to blindly push our
standards to other code-bases, but because I am an advocate of the
format in general.

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

[toc] | [prev] | [next] | [standalone]

#1426879

Hi Lee,

On Mon, Jun 20, 2016 at 09:00:51AM +0100, Lee Jones wrote:
> On Fri, 17 Jun 2016, Doug Anderson wrote:
> > On Fri, Jun 17, 2016 at 1:06 AM, Lee Jones <lee.jones@linaro.org> wrote:
> > >> 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?
> > 
> > What about if Brian made sure to just fully copy the latest version of
> > "cros_ec_commands.h" from the EC codebase and changed this commit
> > message to say:
> > 
> > Copy the latest version of "cros_ec_commands.h" from the Chrome OS EC
> > code base, which is the source of truth for this file.  See
> > <https://chromium.googlesource.com/chromiumos/platform/ec/+/master/include/ec_commands.h>.
> > 
> > From the commit message it would be clear that this is an external
> > file linked into the kernel for convenience.
> > 
> > 
> > > Naturally I'd be happier if you could try to make the code more
> > > 'kernely'.  The practices I mention above are still good ones, even if
> > > you're not writing kernel specific code.
> > 
> > In general requesting that code from outside the kernel conform to
> > "kerneldoc" seems like a bit of a stretch.  In general having some
> > type of parse-able format for comments is nice, but I could see that
> > in the Chrome OS EC codebase it would be a bit overkill.
> 
> It's unfortunate that kerneldoc is named so, since I think it's a nice
> way to write structure/function headers regardless of code base and
> not overkill at all.  It's certainly harder to convince !kernel code
> to use the format, or at least easier for others to push back due to
> the fact that is has 'kernel' in the name.
> 
> > Also: it would be awfully strange if we suddenly started changing the
> > coding convention of this file or we had half the file in one
> > convention and half in another.  The rest of this file is in EC
> > convention and it seems sane to keep it that way...
> 
> Right.  It's also a shame we're only catching this now.  Really we
> should have had this discussion in the first instance.

Well, I see your sign-off on 2 of 4 patches to this file :)

> Taking into consideration that this file is already in the kernel and
> that it's current format is also represented, I'm willing to keep
> adding to it.

Thanks, that seems quite reasonable.

> I would like to see an internal request to adopt
> so-called kerneldoc.  Not because I am wish to blindly push our
> standards to other code-bases, but because I am an advocate of the
> format in general.

As mentioned in another branch of this thread, such a request has
already been made (and it's public, so feel free to follow if you're
so inclined):

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

It seems as if the firmware authors have already agreed that adopting a
more consistent style like kerneldoc would be a net postive. So you
should expect to eventually see a patch to update this file. But in the
meantime, I think it would make things sanest if we update the file
incrementally until that is done.

Regards,
Brian

[toc] | [prev] | [next] | [standalone]

#1425263

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1425391

On Fri, Jun 17, 2016 at 05:55:52PM +0200, Thierry Reding wrote:
> 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:
[...]

> > > > Please use kerneldoc format.

[...]

> > > 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 could have sworn that there was an exception for existing code;
changing styles just for the sake of changing styles can hurt more than
help (e.g., in this instance, in comparing this file with the source of
truth -- the Chrome OS EC firmware sources). But I can't find an
explicit reference to this right now...

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

I think I can safely say that rewriting this in XML is out of the
question for us.

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

IMO, there is value in being able to compare the two sources, and there
is little (but non-zero) value in making these rather minor suggested
changes. Additionally, reformatting this particular change to use
kerneldoc, when the rest of the file doesn't, seems inconsistent. How
does the following sound instead?

(1) Keep the existing patch as-is (perhaps modulo one or two of Lee's
    non-kerneldoc-related comments)
(2) I send a follow-up patch, to help synchronize the out-of-tree EC
    header with the kernel header, after we've considered converting it
    to better match kernel styles. BTW, we've filed a bug for this
    discussion [1], to ensure it doesn't get dropped on the floor.

Brian

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

[toc] | [prev] | [next] | [standalone]

#1425393

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

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

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

[toc] | [prev] | [next] | [standalone]

#1426290

On Fri, 17 Jun 2016, Brian Norris wrote:

> On Thu, Jun 16, 2016 at 04:38:17PM +0100, Lee Jones 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?
> 
> Hex would probably be clearer. I'll try to change that.
> 
> > > +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.

I was generalising.  There will always be exceptions to the rule, but
in the standard case we can be forthcoming enough with our naming
conventions that comments aren't required.

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

Sounds reasonable.

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web