[PATCH] hwmon: scmi-hwmon: implement change_mode

[PATCH] 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>
---
 drivers/hwmon/scmi-hwmon.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index 364199b332c0..f7bd63d52d15 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -151,7 +151,36 @@ 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 (config & BIT(0))
+		cur_mode = THERMAL_DEVICE_ENABLED;
+
+	if (cur_mode == new_mode)
+		return 0;
+
+	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] hwmon: scmi-hwmon: implement change_mode

[Cristian Marussi]

On Mon, Jan 22, 2024 at 04:04:41PM +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.
> 

Hi Peng,

minor remarks down below

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/hwmon/scmi-hwmon.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 364199b332c0..f7bd63d52d15 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -151,7 +151,36 @@ 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 (config & BIT(0))

if SCMI_SENS_CFG_IS_ENABLED(config)

> +		cur_mode = THERMAL_DEVICE_ENABLED;
> +
> +	if (cur_mode == new_mode)
> +		return 0;
> +
> +	if (new_mode == THERMAL_DEVICE_ENABLED)
> +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +	else
> +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +

Here you are ORing the enable bit to preserve the value obtained by
config_get (rightly so), BUT unfortunately looking at the spec
CONFIG_SET uses bits [10:9] for setting the rounding mode while
CONFIG_GET does NOT report (bits [10:2] are Reserved) so you are
in fact setting ROUNDING to 00 probably, meaning ROUND_DOWN.

We could have to add (in the future not now) something in CONFIG_GET
to get the round mode (I'll speak with Souvik) BUT in the meantime I
wonder if it is not more safe here to just explicitly set the mode to
10 to signify it is up to the platform to autonomously choose how to
round. (there are SCMI_SENS_CFG_ROUND_ macros and mask in
scmi_protocol.h)

Other than these small things, LGTM.

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

Thanks,
Cristian

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

[Cristian Marussi]

On Mon, Jan 22, 2024 at 10:59:18AM +0000, Cristian Marussi wrote:
> On Mon, Jan 22, 2024 at 04:04:41PM +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.
> > 
> 
> Hi Peng,
> 
> minor remarks down below
> 
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/hwmon/scmi-hwmon.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > index 364199b332c0..f7bd63d52d15 100644
> > --- a/drivers/hwmon/scmi-hwmon.c
> > +++ b/drivers/hwmon/scmi-hwmon.c
> > @@ -151,7 +151,36 @@ 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 (config & BIT(0))
> 
> if SCMI_SENS_CFG_IS_ENABLED(config)
> 
> > +		cur_mode = THERMAL_DEVICE_ENABLED;
> > +
> > +	if (cur_mode == new_mode)
> > +		return 0;
> > +
> > +	if (new_mode == THERMAL_DEVICE_ENABLED)
> > +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +	else
> > +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +
> 
> Here you are ORing the enable bit to preserve the value obtained by
> config_get (rightly so), BUT unfortunately looking at the spec
> CONFIG_SET uses bits [10:9] for setting the rounding mode while
> CONFIG_GET does NOT report (bits [10:2] are Reserved) so you are
> in fact setting ROUNDING to 00 probably, meaning ROUND_DOWN.
> 
> We could have to add (in the future not now) something in CONFIG_GET
> to get the round mode (I'll speak with Souvik) BUT in the meantime I
> wonder if it is not more safe here to just explicitly set the mode to
> 10 to signify it is up to the platform to autonomously choose how to
> round. (there are SCMI_SENS_CFG_ROUND_ macros and mask in
> scmi_protocol.h)

...so after a quick check with ATG, this ROUNDING mode is indeed related
to this specific CONFIG_SET request being issued...it is not a general
sensor config, so it is not and will not be needed to be reported from
CONFIG_GET (and was indeed clear from the spec my bad...).

Moreover looking better at SENSOR_CONFIG_SET spec the sensor_update_interval
field can be set to 0 if not required to be updated....so I suppose the
better policy here is to just clear out all the bits [31:9] before setting
the enable bit, while KEEPING the original timestamp bit untouched.

A bit tricky...sorry for the confusion.

Thanks,
Cristian