thermal-sensors: Implement name retrieval.

[iwanders]

Comments in this mailing list entry state that

The read_string callback is supposed to retrieve a pointer to a constant string.

So I think we need to store the names of each sensor in our driver.

Something like this PR seems to do it;

[ivor@papyrus:~]$ sensors
ssam_temp-virtual-0
Adapter: Virtual device
I_RTS1:       +32.8°C  
I_RTS2:       +31.4°C  
I_RTS3:       +31.8°C  
I_RTS4:       +30.0°C  
I_RTS5:       +29.4°C  
I_RTS6:       +29.4°C  
I_RTS7:       +31.4°C  
I_RTS8:       +31.0°C  
I_RTS1:       +30.4°C  
I_RTS2:       +29.6°C  
I_RTS3:       +29.4°C  

Some duplicates though... which seem to be real:

[root@papyrus:/home/ivor/Documents/Code/nixos-surface/surface-aggregator-module/scripts/ssam]# /home/ivor/.nix-profile/bin/python ./ctrl.py request 3 1 14 1 1
d8 14 01 49 5f 52 54 53 31 00 00 00 00 00 00 00 00 00 00 00 00

[root@papyrus:/home/ivor/Documents/Code/nixos-surface/surface-aggregator-module/scripts/ssam]# /home/ivor/.nix-profile/bin/python ./ctrl.py request 3 1 14 9 1
2a bd 02 49 5f 52 54 53 31 00 00 00 00 00 00 00 00 00 00 00 00

[iwanders]

As I remarked in https://github.com/linux-surface/surface-aggregator-module/issues/59#issuecomment-1868168386

I'm actually not too sure if these are names, or 'types' or something, given that I'm seeing duplicates.

[qzed]

Hmm, the duplicate names are unfortunate. I hacked up a small script to test this on the SPX and there the names seem to be always different. Specifically, I get I_RTSn with n from 1 to 8 and VTSn with n from 1 to 5.

[iwanders]

Hmm, the duplicate names are unfortunate. I hacked up a small script to test this on the SPX and there the names seem to be always different. Specifically, I get I_RTSn with n from 1 to 8 and VTSn with n from 1 to 5.

Well, that's what I'd expected from my comment here; https://github.com/linux-surface/surface-aggregator-module/issues/59#issuecomment-1859176231 since those are the adjacent strings in the firmware blob. But that's not what I get on my SP9; I don't get those VTS entries, instead I see RTS1..3 again.

[qzed]

I wonder if that's some kind of firmware bug or on purpose. If it's the first, this could be a hint that MS isn't really using that functionality any more...

[qzed]

Finally got around to look at your latest revision. This looks good to me (with one very small nit-pick from my side).

One thing I've noticed from the review of your patches: It looks like we can/should drop a bunch of the checks in the read/is_visible functions. I'll try to do that tomorrow. After that, I'll try to split it up into two patches (one for adding the temperature readouts and another for the labels to have proper attribution) and I guess then we could submit it upstream. But I'll let you know before I do that and give you the chance to look it over.

[qzed]

After that, I'll try to split it up into two patches (one for adding the temperature readouts and another for the labels to have proper attribution) and I guess then we could submit it upstream. But I'll let you know before I do that and give you the chance to look it over.

I've done that now: https://github.com/linux-surface/kernel/compare/44128b742f9f3b664e55f2db77cb0416bc067cac...0b677302860d4d80c57d2f04cd402d2ffb84d3a1

If this looks good to you and you're okay with the Signed-off-by/Co-Developed-by I'd send this upstream. But I'd wait until after your fan patch got accepted to make sure that there are no conflicts when applying.

[iwanders]

Sounds good, I'll probably send v3 somewhere mid next week.

As for the code here; Where we are retrieving names, there's currently a return if a name retrieval fails;

https://github.com/linux-surface/surface-aggregator-module/blob/307439661819cb0d8a302b618d587688db9fa22e/module/src/clients/surface_temp.c#L214

Is that standard practice that if anything goes wrong we just abort in the probe? Because technically things can still work at that stage, like the temperature retrieval will still work even if one of the name retrievals fails?

And one code review remark here:

https://github.com/linux-surface/surface-aggregator-module/blob/307439661819cb0d8a302b618d587688db9fa22e/module/src/clients/surface_temp.c#L124-L125

I'd just remove the else and put the return 0; at the end of the function (so just remove else). It's identical of course, but the else is unnecessary and makes the return less apparent, so I'd just remove it.

[qzed]

Is that standard practice that if anything goes wrong we just abort in the probe? Because technically things can still work at that stage, like the temperature retrieval will still work even if one of the name retrievals fails?

I was thinking about this a bit. There are two ways it can fail: Something in the communication goes wrong, or something in the assumptions we made about the name interface is wrong (e.g. string not null-terminated). The former should be quite unlikely and could be an indication that something else is failing, and the latter we should try to catch (and it could be an indication of garbage being present in the struct, as opposed to a valid string). So ultimately I'd prefer to have all of that fail in a more noticeable way (so that we can investigate and fix the root cause), rather than silently going on.

[qzed]

I'd just remove the else and put the return 0; at the end of the function (so just remove else). It's identical of course, but the else is unnecessary and makes the return less apparent, so I'd just remove it.

Thanks! I changed it (in the kernel repo) in a similar way now, but swapped the values and condition, i.e.

    if (!(ssam_temp->sensors & BIT(channel)))
        return 0;

    return 0444;

I feel like that fits better with the general style. I've also added a new kernel branch based on the SPX branches (s/surface-aggregator/thermal-sensors). I'll probably continue making changes there and then once submitted and accepted will update the repo here.

[iwanders]

:+1: sounds good.