Re: [PATCH 1/3] hwmon: Add thermal sensor driver for Surface Aggregator Module

[Maximilian Luz]

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@xxxxxxxxx>
>
> 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