Received-SPF: pass (google.com: domain of linux-kernel+bounces-38979-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Thu, 25 Jan 2024 16:57:42 +0000
From: Cristian Marussi <cristian.marussi@arm.com>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Peng Fan <peng.fan@nxp.com>, "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
Message-ID: <ZbKTBmt_BP6JRLBj@pluto>
References: <20240125064422.347002-1-peng.fan@oss.nxp.com>
 <b839f83f-c8c7-4fa8-8597-bdde1b40168a@roeck-us.net>
 <DU0PR04MB9417DAD2DBB8820344FEFB07887A2@DU0PR04MB9417.eurprd04.prod.outlook.com>
 <540cf4b3-ebf6-4a85-84c4-c012a69db416@roeck-us.net>
 <ZbKHpFRGoaQpWX16@pluto>
 <da42560b-32b2-49c6-9aeb-b7fbd5b5577c@roeck-us.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <da42560b-32b2-49c6-9aeb-b7fbd5b5577c@roeck-us.net>

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)

Thanks,
Cristian