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