Re: [PATCH v3] hwmon: (cros_ec) Add set and get target fan RPM function

[Guenter Roeck]

On 3/18/25 00:45, Sung-Chi Li wrote:
> The ChromeOS embedded controller (EC) supports closed loop fan speed
> control, so add the fan target attribute under hwmon framework, such
> that kernel can expose reading and specifying the desired fan RPM for
> fans connected to the EC.
>
> When probing the cros_ec hwmon module, we also check the supported
> command version of setting target fan RPM. This commit implements the
> version 0 of getting the target fan RPM, which can only read the target
> RPM of the first fan. This commit also implements the version 1 of
> setting the target fan RPM to each fan respectively.
>
> Signed-off-by: Sung-Chi Li <lschyi@xxxxxxxxxxxx>
> ---
> ChromeOS embedded controller (EC) supports closed-loop fan control. We
> anticipate to have the fan related control from the kernel side, so this
> series register the HWMON_F_TARGET attribute, and implement the read and
> write function for setting/reading the target fan RPM from the EC side.
> ---
> Changes in v3:
> - Drop support of v0 setting target fan RPM, thus also simplify
>    implementations.
> - Align coding style to existing code, including using if-else rather
>    than switch-case, and ensure little endian conversion from read data.
> - Only log warning for failed probing get fan target command version
>    instead of fail the whole driver.
> - Link to v2: https://lore.kernel.org/r/20250317-extend_ec_hwmon_fan-v2-1-13670557afe5@xxxxxxxxxxxx
>
> Changes in v2:
> - Squash the read, write, and register of fan target attribute to 1
>    commit, as they are the same topic.
> - Probe the supported command version from EC for setting the target fan
>    RPM, and perform the set fan target RPM based on the supported
>    version.
> - Update the used variable type to kernel types (i.e., u32).
> - Link to v1: https://lore.kernel.org/r/20250313-extend_ec_hwmon_fan-v1-0-5c566776f2c4@xxxxxxxxxxxx
> ---
>   drivers/hwmon/cros_ec_hwmon.c | 90 ++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 85 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 9991c3fa020ac859cbbff29dfb669e53248df885..d54fd85ccb4350fc297bde62a2d98f386ce1a8de 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -21,6 +21,7 @@ struct cros_ec_hwmon_priv {
>   	struct cros_ec_device *cros_ec;
>   	const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES];
>   	u8 usable_fans;
> +	int set_fan_target_rpm_version;
>   };
>   
>   static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> @@ -36,6 +37,21 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index
>   	return 0;
>   }
>   
> +static int cros_ec_hwmon_read_fan_target(struct cros_ec_device *cros_ec,
> +					 u32 *speed)
> +{
> +	struct ec_response_pwm_get_fan_rpm r;
> +	int ret;
> +
> +	ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_TARGET_RPM, NULL, 0,
> +			  &r, sizeof(r));
> +	if (ret < 0)
> +		return ret;
> +
> +	*speed = le32_to_cpu((__force __le32)r.rpm);
> +	return 0;
> +}
> +
>   static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
>   {
>   	unsigned int offset;
> @@ -52,6 +68,31 @@ static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8
>   	return 0;
>   }
>   
> +static int cros_ec_hwmon_set_fan_rpm(struct cros_ec_device *cros_ec, u8 index,
> +				     u16 val)
> +{
> +	struct ec_params_pwm_set_fan_target_rpm_v1 req = {
> +		.rpm = val,
> +		.fan_idx = index,
> +	};
> +	int ret;
> +
> +	ret = cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_TARGET_RPM, &req,
> +			  sizeof(req), NULL, 0);
> +	if (ret < 0)
> +		return ret;
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_write_fan(struct cros_ec_hwmon_priv *priv, u32 attr,
> +				   int channel, long rpm)
> +{
> +	if (attr == hwmon_fan_target)
> +		return cros_ec_hwmon_set_fan_rpm(priv->cros_ec, channel, rpm);
> +	else

Static analyzers will report lots of "else after return is unnecessary"
with this code, and they do have a point.

Guenter