From: Hans de Goede <hdegoede@redhat.com>
To: "Maximilian Luz" <luzmaximilian@gmail.com>,
"Guenter Roeck" <linux@roeck-us.net>,
"Jean Delvare" <jdelvare@suse.com>,
"Ilpo Järvinen" <ilpo.jarvinen@linux.intel.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: Mon, 8 Apr 2024 15:32:56 +0200 [thread overview]
Message-ID: <28bd0070-28e3-40d6-845a-7ac3d3cf67f2@redhat.com> (raw)
In-Reply-To: <e8ccce25-86d5-492a-8fb4-3f39036fa91a@gmail.com>
Hi,
On 3/30/24 1:58 PM, Maximilian Luz wrote:
> 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.
<snip>
Right this is all working as designed, it is just that Microsoft has
gone a pretty custom route with the Surface devices.
Guenter another way of looking at this is if there were 2 ACPI devices
describing the fan vs the temperature monitoring capabilities that too
would result in 2 drivers even though the underlying ACPI AML code
might end up talking to the same monitoring-IC in the end.
I'll go and merge patch 3/3 of this series. I'll leave merging
1/3 and 2/3 up to the hwmon subsystem of course.
Regards,
Hans
next prev parent reply other threads:[~2024-04-08 13:33 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
2024-04-08 13:32 ` Hans de Goede [this message]
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=28bd0070-28e3-40d6-845a-7ac3d3cf67f2@redhat.com \
--to=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=luzmaximilian@gmail.com \
--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).