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
Date: Tue, 23 Jan 2024 12:34:13 -0800 [thread overview]
Message-ID: <ef92f93e-b294-4bb3-bcf4-4f674c01f729@roeck-us.net> (raw)
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
prev parent reply other threads:[~2024-01-23 20:34 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-23 15:05 [PATCH V2] hwmon: scmi-hwmon: implement change_mode Peng Fan (OSS)
2024-01-23 18:49 ` Cristian Marussi
2024-01-23 18:56 ` Sudeep Holla
2024-01-23 20:34 ` Guenter Roeck [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=ef92f93e-b294-4bb3-bcf4-4f674c01f729@roeck-us.net \
--to=linux@roeck-us.net \
--cc=cristian.marussi@arm.com \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peng.fan@nxp.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.