Message-ID: <ca2c10be-3dc4-45e1-b7fc-f8db29a1b6a0@t-8ch.de>
Date: Mon, 12 May 2025 09:30:39 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: lschyi@...omium.org
Cc: Benson Leung <bleung@...omium.org>, 
	Guenter Roeck <groeck@...omium.org>, Jean Delvare <jdelvare@...e.com>, 
	Guenter Roeck <linux@...ck-us.net>, Jonathan Corbet <corbet@....net>, 
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-doc@...r.kernel.org, Sung-Chi Li <lschyi@...gle.com>
Subject: Re: [PATCH v3 2/3] hwmon: (cros_ec) add PWM control over fans

On 2025-05-12 15:11:56+0800, Sung-Chi Li via B4 Relay wrote:
> From: Sung-Chi Li <lschyi@...omium.org>
> 
> Newer EC firmware supports controlling fans through host commands, so
> adding corresponding implementations for controlling these fans in the
> driver for other kernel services and userspace to control them.
> 
> The driver will first probe the supported host command versions (get and
> set of fan PWM values, get and set of fan control mode) to see if the
> connected EC fulfills the requirements of controlling the fan, then
> exposes corresponding sysfs nodes for userspace to control the fan with
> corresponding read and write implementations.
> As EC will automatically change the fan mode to auto when the device is
> suspended, the power management hooks are added as well to keep the fan
> control mode and fan PWM value consistent during suspend and resume. As
> we need to access the hwmon device in the power management hook, update
> the driver by storing the hwmon device in the driver data as well.
> 
> Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
> ---
>  Documentation/hwmon/cros_ec_hwmon.rst |   5 +-
>  drivers/hwmon/cros_ec_hwmon.c         | 228 ++++++++++++++++++++++++++++++++++
>  2 files changed, 232 insertions(+), 1 deletion(-)

<snip>

>  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
>  {
>  	unsigned int offset;
> @@ -73,7 +117,9 @@ static long cros_ec_hwmon_temp_to_millicelsius(u8 temp)
>  static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			      u32 attr, int channel, long *val)
>  {
> +	u8 control_method;
>  	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +	u8 pwm_value;
>  	int ret = -EOPNOTSUPP;
>  	u16 speed;
>  	u8 temp;

Ordering again.

This should be:

struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
int ret = -EOPNOTSUPP;
u8 control_method;
u8 pwm_value;
u16 speed;
u8 temp;

or:

struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
u8 control_method, pwm_value, temp;
int ret = -EOPNOTSUPP;
u16 speed;

<snip>

> +static inline bool is_cros_ec_cmd_fulfilled(struct cros_ec_device *cros_ec,
> +					    u16 cmd, u8 version)

"fulfilled" -> "available" or "present"

> +{
> +	int ret;
> +
> +	ret = cros_ec_get_cmd_versions(cros_ec, cmd);
> +	return ret >= 0 && (ret & EC_VER_MASK(version));
> +}
> +
> +static bool cros_ec_hwmon_probe_fan_control_supported(struct cros_ec_device *cros_ec)
> +{
> +	if (!IS_ENABLED(CONFIG_PM))
> +		return false;

Why? This should generally work fine without CONFIG_PM.
Only the suspend/resume callbacks are unnecessary in that case.

> +
> +	return is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_PWM_GET_FAN_DUTY, CROS_EC_HWMON_PWM_GET_FAN_DUTY_CMD_VERSION) &&
> +	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_PWM_SET_FAN_DUTY, CROS_EC_HWMON_PWM_SET_FAN_DUTY_CMD_VERSION) &&
> +	       is_cros_ec_cmd_fulfilled(cros_ec, EC_CMD_THERMAL_AUTO_FAN_CTRL, CROS_EC_HWMON_THERMAL_AUTO_FAN_CTRL_CMD_VERSION);
> +}

<snip>

> +static int cros_ec_hwmon_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> +	u8 control_method;
> +	size_t i;
> +	struct cros_ec_hwmon_priv *priv = platform_get_drvdata(pdev);
> +	int ret;

Ordering.

> +
> +	if (!priv->fan_control_supported)
> +		return 0;
> +
> +	/* EC sets fan control to auto after suspended, store settings before suspending. */
> +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> +		if (!(priv->usable_fans & BIT(i)))
> +			continue;
> +
> +		ret = cros_ec_hwmon_read_pwm_enable(priv->cros_ec, i, &control_method);
> +		if (ret) {
> +			dev_warn(&pdev->dev, "failed to get mode setting for fan %zu: %d", i, ret);
> +			continue;
> +		}
> +
> +		if (control_method != 1) {
> +			priv->manual_fans &= ~BIT(i);
> +			continue;
> +		} else {
> +			priv->manual_fans |= BIT(i);
> +		}
> +
> +		ret = cros_ec_hwmon_read_pwm_value(
> +			priv->cros_ec, i, &priv->manual_fan_pwm_values[i]);
> +		/*
> +		 * If failed for storing the value, invalidate the stored mode value by setting it

"If storing the value failed"

> +		 * to auto control. EC will automatically switch to auto mode for that fan after
> +		 * suspended.
> +		 */
> +		if (ret) {
> +			priv->manual_fans &= ~BIT(i);
> +			dev_warn(&pdev->dev, "failed to get PWM setting for fan %zu: %d", i, ret);

Print the warning first, so it's easier to follow the logic.

> +			continue;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> +{
> +	size_t i;
> +	const struct cros_ec_hwmon_priv *priv = platform_get_drvdata(pdev);
> +	int ret;

Ordering.

> +
> +	if (!priv->fan_control_supported)
> +		return 0;
> +
> +	/* EC sets fan control to auto after suspended, restore to settings before suspended. */

"after suspend"

> +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> +		if (!(priv->manual_fans & BIT(i)))
> +			continue;
> +
> +		/*
> +		 * Setting fan PWM value to EC will change the mode to manual for that fan in EC as
> +		 * well, so we do not need to issue a separate fan mode to manual call.
> +		 */
> +		ret = cros_ec_hwmon_set_fan_pwm_val(
> +			priv->cros_ec, i, priv->manual_fan_pwm_values[i]);

You could rename "manual_fan_pwm_values" to "manual_fan_pwm" to save
some linebreaks here and in _suspend().

> +		if (ret)
> +			dev_warn(&pdev->dev, "failed to restore settings for fan %zu: %d", i, ret);

You can use %pe to print errors instead of %d:

"%zu: %pe\n", i, ERR_PTR(ret);

Also missing newline at the end of the message.

> +	}
> +
> +	return 0;
> +}

<snip>

Powered by blists - more mailing lists