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