linux-kernel - Re: [PATCH v3 2/3] hwmon: (cros

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aFJqLkkdI86V3fM9@google.com>
Date: Wed, 18 Jun 2025 15:26:38 +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 v3 2/3] hwmon: (cros_ec) add PWM control over fans

On Mon, May 12, 2025 at 09:30:39AM +0200, Thomas Weißschuh wrote:

Sorry for the late reply, I missed mails for this series.

> On 2025-05-12 15:11:56+0800, Sung-Chi Li via B4 Relay wrote:
> > From: Sung-Chi Li <lschyi@...omium.org>
> >  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>
> 

Would you mind to share the sorting logic, so I do not bother you with checking
these styling issue? Initially, I thought the sorting is based on the variable
name, but after seeing your example (which I am appreciated), I am not sure how
the sorting works. Is it sorted along with the variable types (
"u8 control_method", "u16 speed", etc)? If that is the case, why "u16 speed" is
in the middle of the u8 group variables?

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

I treat fan control should include restoring the fan setting after resume, so
I think if no CONFIG_PM, the fan control is not complete. I am good with
removing this check, and if you have any thoughts after this explanation, please
share with me, otherwise I will remove it in the next series.