RE: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones

[Peng Fan]

> Subject: Re: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for
> thermal zones
> 
> On Thu, Jan 25, 2024 at 08:16:45AM -0800, Guenter Roeck wrote:
> > On 1/25/24 08:09, Cristian Marussi wrote:
> >
> > > Agreed, but it seems that indeed here the attempt is to make sure
> > > that an accidentally disabled sensor is turned on.
> > >
> >
> > From the patch:
> >
> > +static int scmi_hwmon_thermal_change_mode(struct
> thermal_zone_device *tz,
> > +					  enum thermal_device_mode
> new_mode) {
> > ...
> > +	if (new_mode == THERMAL_DEVICE_ENABLED)
> > +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> > +	else
> > +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> >
> > This clearly contradicts your statement. It leaves the sensor in full
> > control by the thermal subsystem. If the thermal subsystem decides to
> > turn it off, it is turned off.
> 
> Yes, indeed, and this is wrong as you explained; what I meant is that it seems
> to me now after this discussion, and from the commit message, that the
> reason (and the aim of this patch) is different from how it achieves it (as a
> side effect)
> 
> "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."
> 
> So when I said:
> 
> > > Agreed, but it seems that indeed here the attempt is to make sure
> > > that an accidentally disabled sensor is turned on.
> 
> and
> 
> >> In this case seems like the sensor is NOT supposed to be ever
> >>disabled  (not even temporarily), it it just that you want to ensure
> >>that is enabled, so I would say @Peng, should not that be done in the
> >>fw  SCMI server ? (i.e. to turn on, early on, all those resources that
> >>are
> >>  exposed to the agent and meant to be always on)
> 
> I implied to drop this patch and instead make sure the visible-and-always-
> enabled sensor is default-enabled by the SCMI server running in the firmware,
> given that there won't be any need to ever disable it, from the hwmon
> interface NOR from the thermal subsystem.
> 
> Sorry if I have not been clear (but I maybe still well-wrong anyway :D)

Sorry to bring back this old topic.

The tempsensor is disabled at boot, I will check with FW owner to enable it
by default. But the tempsensor will consume some power, if leaving
it always enabled.

Do we need to export a HWMON_T_ENABLE to temperature sensor if
leaving thermal framework only reading temp?

Thanks,
Peng.
> 
> Thanks,
> Cristian
>