| Message ID | 20240122080441.1957022-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: scmi-hwmon: implement change_mode | expand |
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
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
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, };