From: Peng Fan <peng.fan@nxp.com>
To: Cristian Marussi <cristian.marussi@arm.com>,
Guenter Roeck <linux@roeck-us.net>
Cc: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>,
"groeck7@gmail.com" <groeck7@gmail.com>,
"sudeep.holla@arm.com" <sudeep.holla@arm.com>,
"jdelvare@suse.com" <jdelvare@suse.com>,
"linux-hwmon@vger.kernel.org" <linux-hwmon@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones
Date: Tue, 14 May 2024 02:53:10 +0000 [thread overview]
Message-ID: <DU0PR04MB94172D2602D32AF757A72CAF88E32@DU0PR04MB9417.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <ZbKTBmt_BP6JRLBj@pluto>
> 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
>
prev parent reply other threads:[~2024-05-14 2:53 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-25 6:44 [PATCH V3] hwmon: scmi-hwmon: implement change_mode for thermal zones Peng Fan (OSS)
2024-01-25 6:54 ` Guenter Roeck
2024-01-25 7:06 ` Peng Fan
2024-01-25 9:45 ` Cristian Marussi
2024-01-25 14:25 ` Guenter Roeck
2024-01-25 16:09 ` Cristian Marussi
2024-01-25 16:16 ` Guenter Roeck
2024-01-25 16:57 ` Cristian Marussi
2024-05-14 2:53 ` Peng Fan [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DU0PR04MB94172D2602D32AF757A72CAF88E32@DU0PR04MB9417.eurprd04.prod.outlook.com \
--to=peng.fan@nxp.com \
--cc=cristian.marussi@arm.com \
--cc=groeck7@gmail.com \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=peng.fan@oss.nxp.com \
--cc=sudeep.holla@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).