Hi Guenter,
> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for
> thermal zones
>
> On 1/24/24 22:44, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > The thermal sensors maybe disabled before kernel boot, so add
> > change_mode for thermal zones to support configuring the thermal
> > sensor to enabled state. If reading the temperature when the sensor is
> > disabled, there will be error reported.
> >
> > The cost is an extra config_get all to SCMI firmware to get the status
> > of the thermal sensor. No function level impact.
> >
> > Reviewed-by: Cristian Marussi <cristian.marussi@xxxxxxx>
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >
> > V3:
> > Update commit log to show it only applys to thermal
> > Add comments in code
> > Add R-b from Cristian
> >
>
> You didn't address my question regarding the behavior of hwmon attributes if
> a sensor is disabled.
Would you please share a bit more on what attributes?
You mean the files under /sys/class/hwmon/hwmon0?
If the sensor is disabled, when cat temp[x]_input, it will
report:
root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp3_input
cat: temp3_input: Protocol error
For enabled, it will report value:
root@imx95-19x19-lpddr5-evk:/sys/class/hwmon/hwmon0# cat temp1_input
31900
>
> > Guenter, I Cced linux@xxxxxxxxxxxx when sending V1/V2
> > Let me Cc Guenter Roeck <groeck7@xxxxxxxxx> in V3, hope you not mind
> >
> This time I received it twice ;-).
>
> > V2:
> > Use SCMI_SENS_CFG_IS_ENABLED & clear BIT[31:9] before update
> > config(Thanks Cristian)
> >
> > drivers/hwmon/scmi-hwmon.c | 39
> ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 39 insertions(+)
> >
> > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > index 364199b332c0..af2267fea5f0 100644
> > --- a/drivers/hwmon/scmi-hwmon.c
> > +++ b/drivers/hwmon/scmi-hwmon.c
> > @@ -151,7 +151,46 @@ 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;
> > +
> > + /*
> > + * Per SENSOR_CONFIG_SET sensor_config description:
> > + * BIT[31:11] should be set to 0 if the sensor update interval does
> > + * not need to be updated, so clear them.
> > + * And SENSOR_CONFIG_GET does not return round up/down, so also
> clear
> > + * BIT[10:9] round up/down.
>
> What does "clear" mean ? Is it going to round up ? Round down ? And why
> would it be necessary to clear those bits if SENSOR_CONFIG_GET does not
> return the current setting in the first place ?
This is to follow Cristian's suggestion to clear [31:9], because we only need
to set the sensor to enabled state, no other attributes.
My understanding is with BIT[31:11] set to 0, BIT[10:9] will not take effect.
Cristian may help comment more since he suggested to clear them in V1/V2
You are right, currently config_get will return [10:2] as reserved,
so config_set bit[10:9] no need touch. But config_get bit[10:2]
may update to return the value in future SCMI spec?
Cristian or Sudeep may help comment here.
Thanks,
Peng.
>
> Thanks,
> Guenter
>
> > + */
> > + 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,
> > };
> >
