Export the target RPM fan control by ChromeOS EC under hwmon

[PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Sung-Chi Li 9 months, 1 week ago

Implement the functionality of reading the target fan RPM setting from
ChromeOS embedded controller under framework.

Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
 drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
+{
+	int ret;
+	struct ec_response_pwm_get_fan_rpm r;
+
+	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(r.rpm);
+	return 0;
+}
+
 static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
 {
 	unsigned int offset;
@@ -95,6 +108,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 {
 	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
 	int ret = -EOPNOTSUPP;
+	int32_t target_rpm;
 	u16 speed;
 	u8 temp;
 
@@ -111,6 +125,10 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
 			if (ret == 0)
 				*val = cros_ec_hwmon_is_error_fan(speed);
+		} else if (attr == hwmon_fan_target) {
+			ret = cros_ec_hwmon_read_fan_target(priv->cros_ec, channel, &target_rpm);
+			if (ret == 0)
+				*val = target_rpm;
 		}
 	} else if (type == hwmon_temp) {
 		if (attr == hwmon_temp_input) {

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by kernel test robot 9 months, 1 week ago

Hi Sung-Chi,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 9fbcd7b32bf7c0a5bda0f22c25df29d00a872017]

url:    https://github.com/intel-lab-lkp/linux/commits/Sung-Chi-Li/hwmon-cros_ec-Add-setting-target-fan-RPM-function/20250313-125018
base:   9fbcd7b32bf7c0a5bda0f22c25df29d00a872017
patch link:    https://lore.kernel.org/r/20250313-extend_ec_hwmon_fan-v1-2-5c566776f2c4%40chromium.org
patch subject: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function
config: x86_64-randconfig-121-20250314 (https://download.01.org/0day-ci/archive/20250314/202503141908.rieksBce-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250314/202503141908.rieksBce-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202503141908.rieksBce-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/hwmon/cros_ec_hwmon.c:48:18: sparse: sparse: cast to restricted __le32

vim +48 drivers/hwmon/cros_ec_hwmon.c

    38	
    39	static int cros_ec_hwmon_read_fan_target(struct cros_ec_device *cros_ec, u8 index, int32_t *speed)
    40	{
    41		int ret;
    42		struct ec_response_pwm_get_fan_rpm r;
    43	
    44		ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_TARGET_RPM, NULL, 0, &r, sizeof(r));
    45		if (ret < 0)
    46			return ret;
    47	
  > 48		*speed = le32_to_cpu(r.rpm);
    49		return 0;
    50	}
    51	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Thomas Weißschuh 9 months, 1 week ago

On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
> Implement the functionality of reading the target fan RPM setting from
> ChromeOS embedded controller under framework.
> 
> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
>  drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)

int32_t is a userspace type. In the kernel use i32, or even better u32.

> +{
> +	int ret;
> +	struct ec_response_pwm_get_fan_rpm r;

Switch the variable declarations around.
Also call the request "req".

> +
> +	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(r.rpm);

r.rpm is not marked as __le32, I'm not sure if sparse will complain
about the usage of le32_to_cpu().

> +	return 0;
> +}
> +
>  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
>  {
>  	unsigned int offset;
> @@ -95,6 +108,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  {
>  	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
>  	int ret = -EOPNOTSUPP;
> +	int32_t target_rpm;

Also u32.

>  	u16 speed;
>  	u8 temp;
>  
> @@ -111,6 +125,10 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>  			ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
>  			if (ret == 0)
>  				*val = cros_ec_hwmon_is_error_fan(speed);
> +		} else if (attr == hwmon_fan_target) {
> +			ret = cros_ec_hwmon_read_fan_target(priv->cros_ec, channel, &target_rpm);
> +			if (ret == 0)
> +				*val = target_rpm;
>  		}
>  	} else if (type == hwmon_temp) {
>  		if (attr == hwmon_temp_input) {
> 
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
>

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Sung-Chi, Li 9 months ago

On Thu, Mar 13, 2025 at 05:24:28PM +0100, Thomas Weißschuh wrote:
> On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
> > Implement the functionality of reading the target fan RPM setting from
> > ChromeOS embedded controller under framework.
> > 
> > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > ---
> >  drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
> 
> int32_t is a userspace type. In the kernel use i32, or even better u32.
> 
Sorry for missing this important detail, I will not use userspace type
for following changes.

> > +{
> > +	int ret;
> > +	struct ec_response_pwm_get_fan_rpm r;
> 
> Switch the variable declarations around.
> Also call the request "req".
> 
> > +
> > +	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(r.rpm);
> 
> r.rpm is not marked as __le32, I'm not sure if sparse will complain
> about the usage of le32_to_cpu().
> 
It did. Currently, all devices are running little endians on both AP and EC, so
I think it is ok not to explicitly call the le32_to_cpu?

> > +	return 0;
> > +}
> > +
> >  static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp)
> >  {
> >  	unsigned int offset;
> > @@ -95,6 +108,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> >  {
> >  	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> >  	int ret = -EOPNOTSUPP;
> > +	int32_t target_rpm;
> 
> Also u32.
> 
Same for the kernel type changes.

> >  	u16 speed;
> >  	u8 temp;
> >  
> > @@ -111,6 +125,10 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> >  			ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed);
> >  			if (ret == 0)
> >  				*val = cros_ec_hwmon_is_error_fan(speed);
> > +		} else if (attr == hwmon_fan_target) {
> > +			ret = cros_ec_hwmon_read_fan_target(priv->cros_ec, channel, &target_rpm);
> > +			if (ret == 0)
> > +				*val = target_rpm;
> >  		}
> >  	} else if (type == hwmon_temp) {
> >  		if (attr == hwmon_temp_input) {
> > 
> > -- 
> > 2.49.0.rc0.332.g42c0ae87b1-goog
> >

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Thomas Weißschuh 9 months ago

On 2025-03-17 11:51:19+0800, Sung-Chi, Li wrote:
> On Thu, Mar 13, 2025 at 05:24:28PM +0100, Thomas Weißschuh wrote:
> > On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
> > > Implement the functionality of reading the target fan RPM setting from
> > > ChromeOS embedded controller under framework.
> > > 
> > > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > > ---
> > >  drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
> > > --- a/drivers/hwmon/cros_ec_hwmon.c
> > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
> > 
> > int32_t is a userspace type. In the kernel use i32, or even better u32.
> > 
> Sorry for missing this important detail, I will not use userspace type
> for following changes.

No need to be sorry.

<snip>

> > > +
> > > +	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(r.rpm);
> > 
> > r.rpm is not marked as __le32, I'm not sure if sparse will complain
> > about the usage of le32_to_cpu().
> > 
> It did. Currently, all devices are running little endians on both AP and EC, so
> I think it is ok not to explicitly call the le32_to_cpu?

I think on big endian none of the CrOS EC code in Linux would work.
But as the driver currently already uses leXX_to_cpu() it would be nice
to keep using it consistently.
The nicest solution would be to change the definition of
struct ec_response_pwm_get_fan_rpm to use __le32.
Or add a cast: le32_to_cpu((__force __le32)r.rpm);

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Guenter Roeck 9 months, 1 week ago

On 3/13/25 09:24, Thomas Weißschuh wrote:
> On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
>> Implement the functionality of reading the target fan RPM setting from
>> ChromeOS embedded controller under framework.
>>
>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>> ---
>>   drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>> index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
>> --- a/drivers/hwmon/cros_ec_hwmon.c
>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>> @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
> 
> int32_t is a userspace type. In the kernel use i32, or even better u32.
> 

Seems to be pretty widely used to complain about.

$ git grep int32_t drivers/ | wc
   43662  192381 3555402

Also, in comparison:

$ git grep i32 drivers/ | wc
     820    4009   68486

Guenter

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Thomas Weißschuh 9 months, 1 week ago

On 2025-03-13 16:36:54-0700, Guenter Roeck wrote:
> On 3/13/25 09:24, Thomas Weißschuh wrote:
> > On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
> > > Implement the functionality of reading the target fan RPM setting from
> > > ChromeOS embedded controller under framework.
> > > 
> > > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > > ---
> > >   drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
> > >   1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > > index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
> > > --- a/drivers/hwmon/cros_ec_hwmon.c
> > > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > > @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
> > 
> > int32_t is a userspace type. In the kernel use i32, or even better u32.
> > 
> 
> Seems to be pretty widely used to complain about.

There is even a checkpatch.pl test for it, which should have triggered:
PREFER_KERNEL_TYPES.

> $ git grep int32_t drivers/ | wc
>   43662  192381 3555402

33k of those are in generated amdgpu headers.
This search also happens to include the more frequently used uint32_t.

> Also, in comparison:
> 
> $ git grep i32 drivers/ | wc
>     820    4009   68486

The numbers for u32 look a bit different:

$ git grep u32 drivers/ | wc
    234768 1137059 17410570


Also this specific driver already consistently uses uNN.
This does look wrong:

	int32_t target_rpm;
 	u16 speed;
 	u8 temp;


Thomas

Re: [PATCH 2/3] hwmon: (cros_ec) Add reading target fan RPM function

Posted by Guenter Roeck 9 months, 1 week ago

On 3/14/25 01:52, Thomas Weißschuh wrote:
> On 2025-03-13 16:36:54-0700, Guenter Roeck wrote:
>> On 3/13/25 09:24, Thomas Weißschuh wrote:
>>> On 2025-03-13 12:47:43+0800, Sung-Chi Li wrote:
>>>> Implement the functionality of reading the target fan RPM setting from
>>>> ChromeOS embedded controller under framework.
>>>>
>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>>>> ---
>>>>    drivers/hwmon/cros_ec_hwmon.c | 18 ++++++++++++++++++
>>>>    1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
>>>> index b2fec0768301f116f49c57b8dbfb042b98a573e1..73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3 100644
>>>> --- a/drivers/hwmon/cros_ec_hwmon.c
>>>> +++ b/drivers/hwmon/cros_ec_hwmon.c
>>>> @@ -36,6 +36,19 @@ 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, u8 index, int32_t *speed)
>>>
>>> int32_t is a userspace type. In the kernel use i32, or even better u32.
>>>
>>
>> Seems to be pretty widely used to complain about.
> 
> There is even a checkpatch.pl test for it, which should have triggered:
> PREFER_KERNEL_TYPES.
> 
>> $ git grep int32_t drivers/ | wc
>>    43662  192381 3555402
> 
> 33k of those are in generated amdgpu headers.
> This search also happens to include the more frequently used uint32_t.
> 
>> Also, in comparison:
>>
>> $ git grep i32 drivers/ | wc
>>      820    4009   68486
> 
> The numbers for u32 look a bit different:
> 
> $ git grep u32 drivers/ | wc
>      234768 1137059 17410570
> 
> 
> Also this specific driver already consistently uses uNN.
> This does look wrong:
> 
> 	int32_t target_rpm;
>   	u16 speed;
>   	u8 temp;
> 
_That_ is a much better argument.

Thanks,
Guenter