[PATCH V2] hwmon: scmi-hwmon: implement change_mode

[PATCH V2] hwmon: scmi-hwmon: implement change_mode

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

The sensor maybe disabled before kernel boot, so add change_mode
to support configuring the sensor to enabled state.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update config(Thanks Cristian)

 drivers/hwmon/scmi-hwmon.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index 364199b332c0..913acd1b137b 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -151,7 +151,39 @@ static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz,
 	return ret;
 }
 
+static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz,
+					  enum thermal_device_mode new_mode)
+{
+	int ret;
+	u32 config;
+	enum thermal_device_mode cur_mode = THERMAL_DEVICE_DISABLED;
+	struct scmi_thermal_sensor *th_sensor = thermal_zone_device_priv(tz);
+
+	ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id,
+				     &config);
+	if (ret)
+		return ret;
+
+	if (SCMI_SENS_CFG_IS_ENABLED(config))
+		cur_mode = THERMAL_DEVICE_ENABLED;
+
+	if (cur_mode == new_mode)
+		return 0;
+
+	config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK |
+		    SCMI_SENS_CFG_UPDATE_EXP_MASK |
+		    SCMI_SENS_CFG_ROUND_MASK);
+	if (new_mode == THERMAL_DEVICE_ENABLED)
+		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
+	else
+		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
+
+	return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id,
+				      config);
+}
+
 static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops = {
+	.change_mode = scmi_hwmon_thermal_change_mode,
 	.get_temp = scmi_hwmon_thermal_get_temp,
 };
 
-- 
2.37.1

Re: [PATCH V2] hwmon: scmi-hwmon: implement change_mode

[Cristian Marussi]

On Tue, Jan 23, 2024 at 11:05:26PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The sensor maybe disabled before kernel boot, so add change_mode
> to support configuring the sensor to enabled state.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---

In general LGTM.
It would be better clearly explaining in the commit message that this
change is around HWMON thermal zones and also add a comment down below
to justify why we have decided to clear out those config bits.

> 
> V2:
>  Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update config(Thanks Cristian)
> 
>  drivers/hwmon/scmi-hwmon.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 364199b332c0..913acd1b137b 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -151,7 +151,39 @@ static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz,
>  	return ret;
>  }
>  
> +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz,
> +					  enum thermal_device_mode new_mode)
> +{
> +	int ret;
> +	u32 config;
> +	enum thermal_device_mode cur_mode = THERMAL_DEVICE_DISABLED;
> +	struct scmi_thermal_sensor *th_sensor = thermal_zone_device_priv(tz);
> +
> +	ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id,
> +				     &config);
> +	if (ret)
> +		return ret;
> +
> +	if (SCMI_SENS_CFG_IS_ENABLED(config))
> +		cur_mode = THERMAL_DEVICE_ENABLED;
> +
> +	if (cur_mode == new_mode)
> +		return 0;
> +

This config bits_clearing is worth an explanation in a comment (like we
did in the mail thread...)

> +	config &= ~(SCMI_SENS_CFG_UPDATE_SECS_MASK |
> +		    SCMI_SENS_CFG_UPDATE_EXP_MASK |
> +		    SCMI_SENS_CFG_ROUND_MASK);
> +	if (new_mode == THERMAL_DEVICE_ENABLED)
> +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +	else
> +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +
> +	return sensor_ops->config_set(th_sensor->ph, th_sensor->info->id,
> +				      config);
> +}
> +

Other than this,

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

Re: [PATCH V2] hwmon: scmi-hwmon: implement change_mode

[Sudeep Holla]

On Tue, Jan 23, 2024 at 11:05:26PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 

hwmon: scmi-hwmon: implement change_mode

The above subject gives me no clue as what this change wants to achieve.
At minimum you need to mention thermal zones as HWMON supports more than
just thermal sensors and change mode mentioned in $subject applies to
only thermal zones.

> The sensor maybe disabled before kernel boot, so add change_mode
> to support configuring the sensor to enabled state.
>

Again above applies to thermal zones only in this patch. It doesn't
cover non-thermal sensors, so prefer if you refer it as thermal zones
instead of sensors.

The change itself looks good. I will ack once you fix the subject and
description so that Guenter can pick up the change.

-- 
Regards,
Sudeep

Re: [PATCH V2] hwmon: scmi-hwmon: implement change_mode

[Guenter Roeck]

On Tue, Jan 23, 2024 at 11:05:26PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The sensor maybe disabled before kernel boot, so add change_mode
> to support configuring the sensor to enabled state.
> 

As mentioned by others, this will require a better explanation.
It only affects thermal sensors, and the scope is not provided.
Specifically, neither subject nor description explain that this
change is primarily for thermal subsystem functionality, and the
(non ?) impact on the hwmon device is not explained.

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

As a general comment, I have not received the original patch, and instead
pulled it from patchwork. I also did not receive the initial version
of this patch.

Maybe it is just me, but e-mail from kernel.org has been sporadic
in the last month or so and I have been missing various e-mails.
I would suggest for everyone to copy me directly if a response from me
is expected or desired.

Anyway, this change looks like it enables / disables individual temperature
sensors. What is the expected result for the hwmon device, or in other
words what happens if a sensor is disabled through the thermal subsystem
and the "sensors" command is executed ? The impact (or lack of it) should
be explained.

Also, if my interpretation is correct, you'll need to explain why you
did not (also) implement the hwmon "enable" attribute for temperature
(and possibly other) sensors.

Thanks,
Guenter