Message-ID: <f63hek7p5d74xrkcbm7wk5s3fc6mlgyjkiovpyprlro4blyckf@in5an2tk6twb>
Date: Fri, 2 May 2025 13:31:55 +0800
From: Sung-Chi Li <lschyi@...omium.org>
To: Thomas Weißschuh <linux@...ssschuh.net>
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
Subject: Re: [PATCH 2/3] hwmon: (cros_ec) add PWM control over fans

On Wed, Apr 30, 2025 at 04:48:15PM +0200, Thomas Weißschuh wrote:
> On 2025-04-30 15:00:10+0800, Sung-Chi Li wrote:
> > On Tue, Apr 29, 2025 at 11:20:09PM +0200, Thomas Weißschuh wrote:
> > > On 2025-04-29 16:14:22+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         | 237 +++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 237 insertions(+), 5 deletions(-)
> 
> <snip>
> 
> > > > +static int cros_ec_hwmon_read_pwm_value(struct cros_ec_device *cros_ec,
> > > > +					u8 index, u8 *pwm_value)
> > > > +{
> > > > +	int ret = cros_ec_hwmon_read_pwm_raw_value(cros_ec, index, pwm_value);
> > > 
> > > The _raw_ function is unnecessary.
> > > 
> > 
> > This is to share with the `cros_ec_hwmon_cooling_get_cur_state`, and there is a
> > unit conversion needed, so extract the same process into a _raw_ function.
> 
> What's the advantage of scaling the value for the cooling device?
> The hwmon core thermal zone implementation also uses the hwmon values
> directly.
> 

After rethinking about this, I think using the same unit between hwmon and
thermal cooling devices is easier. As a result, will remove the _raw_ functions,
and update logics in the cooling device commit.

> > > > +
> > > > +	if (ret == 0)
> > > > +		*pwm_value = *pwm_value * 255 / 100;
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +static int cros_ec_hwmon_read_pwm_enable(struct cros_ec_device *cros_ec,
> > > > +					 u8 index, u8 *control_method)
> > > > +{
> > > > +	struct ec_params_auto_fan_ctrl_v2 req = {
> > > > +		.fan_idx = index,
> > > > +		.cmd = EC_AUTO_FAN_CONTROL_CMD_GET,
> > > > +	};
> > > > +	struct ec_response_auto_fan_control resp;
> > > > +	int ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req,
> > > > +			      sizeof(req), &resp, sizeof(resp));
> > > 
> > > Keep &foo and sizeof(foo) together on the same line please.
> > > 
> > 
> > This is automatically formatted by clang-format. I will keep it like this in the
> > v2 patch. If it is important for readablity, please share with me, and I will
> > update that in the v2 patch.
> 
> It's not that important. But unfortunate that clang-format will make the
> formatting worse.
> 

Ok, I will keep them in the same row.

<snip>

> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	/* No CROS EC supports no fan speed control */
> > > > +	if (val == 0)
> > > > +		return -EOPNOTSUPP;
> > > > +
> > > > +	req.set_auto = (val != 1) ? true : false;
> > > > +	ret = cros_ec_cmd(cros_ec, 2, EC_CMD_THERMAL_AUTO_FAN_CTRL, &req,
> > > > +			  sizeof(req), NULL, 0);
> > > 
> > > Use a full 100 columns.
> > > 
> > 
> > Hmm, I found the style guide actually strongly prefer 80:
> > https://www.kernel.org/doc/html/v6.14/process/coding-style.html#breaking-long-lines-and-strings
> 
> I don't think it is a strong recommendation anymore.
> The hwmon core also doesn't seem to be religious about it.
> 

Ok, I will update the series with 100 columns.

<snip>

> > > > +static int cros_ec_hwmon_resume(struct platform_device *pdev)
> > > > +{
> > > > +	const struct cros_ec_hwmon_platform_priv *platform_priv =
> > > > +		dev_get_drvdata(&pdev->dev);
> > > > +	const struct cros_ec_hwmon_priv *priv =
> > > > +		dev_get_drvdata(platform_priv->hwmon_dev);
> > > > +	size_t i;
> > > > +	int ret;
> > > > +
> > > > +	if (!priv->fan_control_supported)
> > > > +		return 0;
> > > > +
> > > > +	/*
> > > > +	 * EC sets fan control to auto after suspended, restore settings to
> > > > +	 * before suspended.
> > > > +	 */
> > > > +	for (i = 0; i < EC_FAN_SPEED_ENTRIES; i++) {
> > > > +		if (!(priv->manual_fans & BIT(i)))
> > > > +			continue;
> > > 
> > > Given that we can read the actual state from the EC I'd prefer to read
> > > it back and store it during suspend() instead of storing it during write().
> > > 
> > 
> > Do you mean reading fan mode and fan PWM value during suspend, or we will keep
> > updating `manual_fans` while write(), and do not cache the PWM value while
> > write()? That involves whether we need to send a get fan mode for every write
> > PWM value.
> 
> This one:
> "reading fan mode and fan PWM value during suspend"
> 

Sounds good, update the logic to only caching values when suspending, and remove
the behavior of caching values when writing. The write of PWM values is then
changed to fetch the fan control method first.

Powered by blists - more mailing lists