Re: [PATCH v2] hwmon: Add thermal sensor driver for Surface Aggregator Module

[Maximilian Luz <luzmaximilian@gmail.com>]

On 8/19/24 12:26 PM, Konrad Dybcio wrote:
> On 10.08.2024 3:14 AM, Konrad Dybcio wrote:
>> On 10.08.2024 3:04 AM, Maximilian Luz wrote:
>>> On 8/10/24 1:35 AM, Konrad Dybcio wrote:
>>>> On 5.08.2024 1:08 AM, 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.
>>>>>
>>>>> The EC can have up to 16 thermal sensors connected via a single
>>>>> sub-device, each providing temperature readings and a label string.
>>>>>
>>>>> Link: https://github.com/linux-surface/surface-aggregator-module/issues/59
>>>>> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
>>>>> Co-developed-by: Ivor Wanders <ivor@iwanders.net>
>>>>> Signed-off-by: Ivor Wanders <ivor@iwanders.net>
>>>>> Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com>
>>>>>
>>>>> ---
>>>>
>>>> Gave it a shot on SL7, some names are repeated and one sensor is
>>>> totally busted
>>>>
>>>> /sys/class/hwmon/hwmon66/name:surface_thermal
>>>> /sys/class/hwmon/hwmon66/temp10_input:32200
>>>> /sys/class/hwmon/hwmon66/temp10_label:I_RTS2
>>>> /sys/class/hwmon/hwmon66/temp11_input:31600
>>>> /sys/class/hwmon/hwmon66/temp11_label:I_RTS3
>>>> /sys/class/hwmon/hwmon66/temp12_input:38000
>>>> /sys/class/hwmon/hwmon66/temp12_label:I_RTS4
>>>> /sys/class/hwmon/hwmon66/temp1_input:43900
>>>> /sys/class/hwmon/hwmon66/temp1_label:I_RTS1
>>>> /sys/class/hwmon/hwmon66/temp2_input:44000
>>>> /sys/class/hwmon/hwmon66/temp2_label:I_RTS2
>>>> /sys/class/hwmon/hwmon66/temp3_input:47300
>>>> /sys/class/hwmon/hwmon66/temp3_label:I_RTS3
>>>> /sys/class/hwmon/hwmon66/temp4_input:-273100
>>>> /sys/class/hwmon/hwmon66/temp4_label:I_RTS4
>>>> /sys/class/hwmon/hwmon66/temp5_input:31300
>>>> /sys/class/hwmon/hwmon66/temp5_label:I_RTS5
>>>> /sys/class/hwmon/hwmon66/temp9_input:37100
>>>> /sys/class/hwmon/hwmon66/temp9_label:I_RTS1
>>>
>>> Hmm, on the SPX it looks like this:
>>>
>>> I_RTS1:       +31.9°C
>>> I_RTS2:       +31.3°C
>>> I_RTS3:       +31.4°C
>>> I_RTS4:       +28.3°C
>>> I_RTS5:       +29.3°C
>>> I_RTS6:       +29.3°C
>>> I_RTS7:       +29.3°C
>>> I_RTS8:       +29.3°C
>>> VTS1:         +30.2°C
>>> VTS2:          +0.0°C
>>> VTS3:          +0.0°C
>>> VTS4:          +0.0°C
>>> VTS5:          +0.0°C
>>>
>>> So VTS2-5 seem like they may not actually be connected, but the rest at
>>> least look somewhat sensible. I'd probably still keep the names as they
>>> at least might give an indication what the sensors could be for.
>>>
>>> But there's a good chance that we're missing something on how MS
>>> envisions these sensors to work exactly.
>>
>> I think the takeaway here is that I'll keep the sensors disabled for
>> now on SL7 until we have a better idea and not block this driver
> 
> I had an idea.. maybe the busted sensor is for something modem-related.
> My unit doesn't come with one.

I think that could make sense. It is interesting though that SAM says
it's present (there's a bit-field indicating which sensor is there),
but I guess they just hard-code that the same way across all variants.

Best regards,
Max