linux-kernel - Re: [PATCH v2 2/4] mfd: cros_ec: add EC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160616153817.GD21702@dell>
Date:	Thu, 16 Jun 2016 16:38:17 +0100
From:	Lee Jones <lee.jones@...aro.org>
To:	Brian Norris <briannorris@...omium.org>
Cc:	Thierry Reding <thierry.reding@...il.com>,
	Olof Johansson <olof@...om.net>, linux-kernel@...r.kernel.org,
	Doug Anderson <dianders@...omium.org>,
	Brian Norris <computersforpeace@...il.com>,
	linux-pwm@...r.kernel.org, devicetree@...r.kernel.org,
	Boris Brezillon <boris.brezillon@...e-electrons.com>,
	Stephen Barber <smbarber@...omium.org>,
	Javier Martinez Canillas <javier@....samsung.com>,
	Benson Leung <bleung@...omium.org>,
	Enric Balletbo <enric.balletbo@...labora.co.uk>,
	Randall Spangler <rspangler@...omium.org>,
	Shawn Nematbakhsh <shawnn@...omium.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Todd Broch <tbroch@...omium.org>,
	Gwendal Grignou <gwendal@...omium.org>,
	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Subject: 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@...omium.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