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@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
> > 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
