[PATCH 0/3] Export the target RPM fan control by ChromeOS EC under hwmon

[PATCH 0/3] Export the target RPM fan control by ChromeOS EC under hwmon

[Sung-Chi Li]

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.

Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
Sung-Chi Li (3):
      hwmon: (cros_ec) Add setting target fan RPM function
      hwmon: (cros_ec) Add reading target fan RPM function
      hwmon: (cros_ec) Register fan target attribute

 drivers/hwmon/cros_ec_hwmon.c | 69 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 64 insertions(+), 5 deletions(-)
---
base-commit: 9fbcd7b32bf7c0a5bda0f22c25df29d00a872017
change-id: 20250313-extend_ec_hwmon_fan-a5f59aab2cb3

Best regards,
-- 
Sung-Chi Li <lschyi@chromium.org>

[PATCH 1/3] hwmon: (cros_ec) Add setting target fan RPM function

[Sung-Chi Li]

Implement the functionality of setting the target fan RPM to ChromeOS
embedded controller under hwmon framework.

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

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 9991c3fa020ac859cbbff29dfb669e53248df885..b2fec0768301f116f49c57b8dbfb042b98a573e1 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -52,6 +52,26 @@ 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 p_v1 = {
+		.rpm = val,
+		.fan_idx = index,
+	};
+
+	return cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_TARGET_RPM, &p_v1, sizeof(p_v1), NULL, 0);
+}
+
+static int cros_ec_hwmon_write_fan(struct cros_ec_device *cros_ec, u32 attr, int channel, long rpm)
+{
+	switch (attr) {
+	case hwmon_fan_target:
+		return cros_ec_hwmon_set_fan_rpm(cros_ec, channel, rpm);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static bool cros_ec_hwmon_is_error_fan(u16 speed)
 {
 	return speed == EC_FAN_SPEED_NOT_PRESENT || speed == EC_FAN_SPEED_STALLED;
@@ -140,6 +160,19 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
 	return 0;
 }
 
+static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
+			      u32 attr, int channel, long val)
+{
+	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
+
+	switch (type) {
+	case hwmon_fan:
+		return cros_ec_hwmon_write_fan(priv->cros_ec, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
 	HWMON_CHANNEL_INFO(fan,
@@ -179,6 +212,7 @@ static const struct hwmon_ops cros_ec_hwmon_ops = {
 	.read = cros_ec_hwmon_read,
 	.read_string = cros_ec_hwmon_read_string,
 	.is_visible = cros_ec_hwmon_is_visible,
+	.write = cros_ec_hwmon_write,
 };
 
 static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

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

[Thomas Weißschuh]

On 2025-03-13 12:47:42+0800, Sung-Chi Li wrote:
> Implement the functionality of setting the target fan RPM to ChromeOS
> embedded controller under hwmon framework.
> 
> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
>  drivers/hwmon/cros_ec_hwmon.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 9991c3fa020ac859cbbff29dfb669e53248df885..b2fec0768301f116f49c57b8dbfb042b98a573e1 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -52,6 +52,26 @@ 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 p_v1 = {

The v1 protocol was "only" introduces in 2014.
Could it be possible that devices without that command are still in use?
If so the presence of the command should be probed.

What is the name p_v1 supposed to mean? Call it "req", like other parts
of the driver.

> +		.rpm = val,
> +		.fan_idx = index,
> +	};
> +
> +	return cros_ec_cmd(cros_ec, 1, EC_CMD_PWM_SET_FAN_TARGET_RPM, &p_v1, sizeof(p_v1), NULL, 0);

cros_ec_cmd() signals success with an exitcode >= 0, while the hwmon
APIs only expect 0. In this specific case the success value will also
always be zero, as no response is sent by the EC, but for clarity I
prefer to have an explicit check.

> +}
> +
> +static int cros_ec_hwmon_write_fan(struct cros_ec_device *cros_ec, u32 attr, int channel, long rpm)
> +{
> +	switch (attr) {
> +	case hwmon_fan_target:
> +		return cros_ec_hwmon_set_fan_rpm(cros_ec, channel, rpm);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static bool cros_ec_hwmon_is_error_fan(u16 speed)
>  {
>  	return speed == EC_FAN_SPEED_NOT_PRESENT || speed == EC_FAN_SPEED_STALLED;
> @@ -140,6 +160,19 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
>  	return 0;
>  }
>  
> +static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long val)
> +{
> +	struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev);
> +
> +	switch (type) {
> +	case hwmon_fan:
> +		return cros_ec_hwmon_write_fan(priv->cros_ec, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>  static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
>  	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
>  	HWMON_CHANNEL_INFO(fan,
> @@ -179,6 +212,7 @@ static const struct hwmon_ops cros_ec_hwmon_ops = {
>  	.read = cros_ec_hwmon_read,
>  	.read_string = cros_ec_hwmon_read_string,
>  	.is_visible = cros_ec_hwmon_is_visible,
> +	.write = cros_ec_hwmon_write,

Move the .write directly after .read_string.

>  };
>  
>  static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
> 
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 

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

[Sung-Chi, Li]

On Thu, Mar 13, 2025 at 05:21:35PM +0100, Thomas Weißschuh wrote:
> On 2025-03-13 12:47:42+0800, Sung-Chi Li wrote:
> > Implement the functionality of setting the target fan RPM to ChromeOS
> > embedded controller under hwmon framework.
> > 
> > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > ---
> >  drivers/hwmon/cros_ec_hwmon.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> > index 9991c3fa020ac859cbbff29dfb669e53248df885..b2fec0768301f116f49c57b8dbfb042b98a573e1 100644
> > --- a/drivers/hwmon/cros_ec_hwmon.c
> > +++ b/drivers/hwmon/cros_ec_hwmon.c
> > @@ -52,6 +52,26 @@ 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 p_v1 = {
> 
> The v1 protocol was "only" introduces in 2014.
> Could it be possible that devices without that command are still in use?
> If so the presence of the command should be probed.
> 
You are right, I will probe the command versions for both get and set fan RPM
values in the next change, and perform different handling based on the version.

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

[Sung-Chi Li]

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

[Thomas Weißschuh]

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

[Guenter Roeck]

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

[Thomas Weißschuh]

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

[Guenter Roeck]

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

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

[Sung-Chi, Li]

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

[Thomas Weißschuh]

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

[kernel test robot]

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

[PATCH 3/3] hwmon: (cros_ec) Register fan target attribute

[Sung-Chi Li]

The ChromeOS embedded controller (EC) supports closed loop fan speed
control, so add this attribute under hwmon framework such that kernel
can specify the desired fan RPM for fans connected to the EC.

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

diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 73bfcbbaf9531be6b753cfef8045fd5dab5b2ab3..56a8ee13ec2a9f8e7127815a530d2a254a45bf55 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -168,8 +168,15 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type
 	const struct cros_ec_hwmon_priv *priv = data;
 
 	if (type == hwmon_fan) {
-		if (priv->usable_fans & BIT(channel))
+		if (!(priv->usable_fans & BIT(channel)))
+			return 0;
+
+		switch (attr) {
+		case hwmon_fan_target:
+			return 0644;
+		default:
 			return 0444;
+		}
 	} else if (type == hwmon_temp) {
 		if (priv->temp_sensor_names[channel])
 			return 0444;
@@ -194,10 +201,10 @@ static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
 static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = {
 	HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ),
 	HWMON_CHANNEL_INFO(fan,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT,
-			   HWMON_F_INPUT | HWMON_F_FAULT),
+			   HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
+			   HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
+			   HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET,
+			   HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET),
 	HWMON_CHANNEL_INFO(temp,
 			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,
 			   HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL,

-- 
2.49.0.rc0.332.g42c0ae87b1-goog

Re: [PATCH 0/3] Export the target RPM fan control by ChromeOS EC under hwmon

[Thomas Weißschuh]

Hi Sung-Chi,

On 2025-03-13 12:47:41+0800, Sung-Chi Li wrote:
> 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.

In general the idea is fine. I'll post some comments for the patches.

The series should be squashed into a single patch. All the new code is
closely related to each other.

> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
> Sung-Chi Li (3):
>       hwmon: (cros_ec) Add setting target fan RPM function
>       hwmon: (cros_ec) Add reading target fan RPM function
>       hwmon: (cros_ec) Register fan target attribute
> 
>  drivers/hwmon/cros_ec_hwmon.c | 69 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 64 insertions(+), 5 deletions(-)
> ---
> base-commit: 9fbcd7b32bf7c0a5bda0f22c25df29d00a872017
> change-id: 20250313-extend_ec_hwmon_fan-a5f59aab2cb3
> 
> Best regards,
> -- 
> Sung-Chi Li <lschyi@chromium.org>