Received-SPF: pass (google.com: domain of linux-kernel+bounces-36027-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Guenter Roeck <groeck7@gmail.com>
Date: Tue, 23 Jan 2024 12:34:13 -0800
From: Guenter Roeck <linux@roeck-us.net>
To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, jdelvare@suse.com,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
	Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH V2] hwmon: scmi-hwmon: implement change_mode
Message-ID: <ef92f93e-b294-4bb3-bcf4-4f674c01f729@roeck-us.net>
References: <20240123150526.3615901-1-peng.fan@oss.nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240123150526.3615901-1-peng.fan@oss.nxp.com>

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