From: Maximilian Luz <luzmaximilian@gmail.com>
To: "Guenter Roeck" <linux@roeck-us.net>,
"Jean Delvare" <jdelvare@suse.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>,
"Hans de Goede" <hdegoede@redhat.com>
Cc: Ivor Wanders <ivor@iwanders.net>,
linux-kernel@vger.kernel.org,
platform-driver-x86@vger.kernel.org, linux-hwmon@vger.kernel.org
Subject: Re: [PATCH 1/3] hwmon: Add thermal sensor driver for Surface Aggregator Module
Date: Sat, 30 Mar 2024 13:58:44 +0100 [thread overview]
Message-ID: <e8ccce25-86d5-492a-8fb4-3f39036fa91a@gmail.com> (raw)
In-Reply-To: <d49ea735-3113-4c1f-a8dc-c6d8e821c4f1@roeck-us.net>
On 3/30/24 12:58, Guenter Roeck wrote:
> On 3/30/24 04:24, Maximilian Luz wrote:
>> Some of the newer Microsoft Surface devices (such as the Surface Book
>> 3 and Pro 9) have thermal sensors connected via the Surface Aggregator
>> Module (the embedded controller on those devices). Add a basic driver
>> to read out the temperature values of those sensors.
>>
>> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59
>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>
> I very much dislike the idea of having multiple drivers for hardware
> monitoring on the same system. Please explain in detail why this and
> the fan driver for the same system need separate drivers.
>
> I'll also very much want to know if we will see submissions for separate
> voltage, current, power, energy, humidity, and/or other hardware monitoring
> entities for the same system later on.
The Surface Aggregator EC is not really a single device, but rather a
collection of subsystems. For example, there's one for the battery, one
for thermal sensors, and a separate one for the fan. Not all subsystems
are present on all devices with that EC, so we have modeled them as
separate subdevices of the parent EC controller. This makes it quite a
bit easier to maintain. Especially, since I haven't found any reliable
way to auto-detect the active/supported subsystems.
For example: The Surface Book 3 has thermal sensors that can be accessed
via this driver as well as a fan. As far as I know, however, the fan
subsystem has been introduced later and the Surface Book 3 doesn't
support that yet. So there's (as far as I know) no fan-monitoring
support. Trying to access that will fail with a timeout. For that reason
(but not specifically for that device), we have introduced the split
into subystem devices, which are maunally registered in
surface_aggregator_registry.c based on what we know the device actually
supports.
Further, the devices created for these subsystems also act as a binding
mechanism to the subsystem, speficying the subsystem ID/category used
for making requests to it. For example, this driver probes for
SSAM_SDEV(TMP, SAM, 0x00, 0x02)
meaning it looks for a device of the TMP subsystem on the SAM target ID
(which can be seen as a bus number) with instance 0 and function 2. This
(in particular subsystem ID and target ID) are directly used when making
requests to the EC. So if we find out down the line that temperature
sensors can be accessed on target ID KIP in addition to SAM, it's as easy
as adding a new device match to the driver.
I believe that this would be more difficult if the driver is merged
together: Doing so would require us to figure out what's present and
what we can or should access inside of the driver (instead of via the
already established registry). So it would either require us to do a
certain amount of hardcoding and if/else branches or we would have to
introduce a bunch of device properties and a meta-device just to bundle
all monitoring-related subsystems together, and again use a bunch of
if/else branches... And in both cases, the direct subsystem binding
originally intended in the device structure of the Surface Aggregator
bus goes out of the window.
So, in my opinion at least, having separate smaller but specific drivers
and leaving that "logic" to the registry driver makes this much more
maintainable.
Regarding other monitoring entities: In short, I don't know. I am not
aware of any other (current/power/...) monitoring capabilities of the EC
right now, but I can't guarantee that MS won't introduce a new subsystem
for those down the line (or that there already is one and we just don't
know about it). At which point, it will quite likely only be supported
on some (probably newer) devices again.
Best regards,
Max
next prev parent reply other threads:[~2024-03-30 12:58 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-30 11:23 [PATCH 0/3] Add thermal sensor driver for Surface Aggregator Maximilian Luz
2024-03-30 11:24 ` [PATCH 1/3] hwmon: Add thermal sensor driver for Surface Aggregator Module Maximilian Luz
2024-03-30 11:58 ` Guenter Roeck
2024-03-30 12:58 ` Maximilian Luz [this message]
2024-04-08 13:32 ` Hans de Goede
2024-04-08 13:33 ` Hans de Goede
2024-04-16 13:15 ` Guenter Roeck
2024-04-16 13:27 ` Guenter Roeck
2024-04-16 18:12 ` Maximilian Luz
2024-03-30 11:24 ` [PATCH 2/3] hwmon: surface_temp: Add support for sensor names Maximilian Luz
2024-04-08 13:34 ` Hans de Goede
2024-04-16 13:30 ` Guenter Roeck
2024-04-16 19:00 ` Maximilian Luz
2024-04-16 21:08 ` Guenter Roeck
2024-05-02 20:05 ` Maximilian Luz
2024-04-16 21:34 ` Ivor Wanders
2024-05-02 20:04 ` Maximilian Luz
2024-03-30 11:24 ` [PATCH 3/3] platform/surface: aggregator_registry: Add support for thermal sensors on the Surface Pro 9 Maximilian Luz
2024-04-08 15:22 ` Hans de Goede
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=e8ccce25-86d5-492a-8fb4-3f39036fa91a@gmail.com \
--to=luzmaximilian@gmail.com \
--cc=hdegoede@redhat.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=ivor@iwanders.net \
--cc=jdelvare@suse.com \
--cc=linux-hwmon@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=platform-driver-x86@vger.kernel.org \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).