Re: [PATCH v2] hwmon: corsair-psu: add support for critical values

[Guenter Roeck]

On 3/16/21 7:21 AM, Wilken Gottwalt wrote:
> On Mon, 15 Mar 2021 11:00:53 -0700
> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> 
>> On 3/15/21 9:55 AM, Wilken Gottwalt wrote:
>>> On Mon, 15 Mar 2021 08:53:25 -0700
>>> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>>
>>>> On 3/15/21 8:02 AM, Wilken Gottwalt wrote:
>>>>> Adds support for reading the critical values of the temperature sensors
>>>>> and the rail sensors (voltage and current) once and caches them. Updates
>>>>> the naming of the constants following a more clear scheme. Also updates
>>>>> the documentation and fixes a typo.
>>>>>
>>>>> The new sensors output of a Corsair HX850i will look like this:
>>>>> corsairpsu-hid-3-1
>>>>> Adapter: HID adapter
>>>>> v_in:        230.00 V
>>>>> v_out +12v:   12.14 V  (crit min =  +8.41 V, crit max = +15.59 V)
>>>>> v_out +5v:     5.03 V  (crit min =  +3.50 V, crit max =  +6.50 V)
>>>>> v_out +3.3v:   3.30 V  (crit min =  +2.31 V, crit max =  +4.30 V)
>>>>> psu fan:        0 RPM
>>>>> vrm temp:     +46.2°C  (crit = +70.0°C)
>>>>> case temp:    +39.8°C  (crit = +70.0°C)
>>>>> power total: 152.00 W
>>>>> power +12v:  108.00 W
>>>>> power +5v:    41.00 W
>>>>> power +3.3v:   5.00 W
>>>>> curr in:          N/A
>>>>
>>>> What does that mean ? If it isn't supported by the power supply,
>>>> should we drop that entirely ? Maybe drop it via the is_visible
>>>> function if it is available for some variants, but always displaying
>>>> N/A doesn't add value.
>>>>
>>>> This is a bit odd, though, since I would assume it translates
>>>> to the PSU_CMD_IN_AMPS command. Any chance to track down what is
>>>> happening here ?
>>>
>>> I have one of the earliest PSUs of this series, it is just not supported on
>>> mine. I'm not sure if it would be worth the trouble to catch that and turn
>>> it off dynamically.
>>>
>>
>> I think so, because otherwise we'll get complaints about it (people
>> are really picky abut such things lately). Better not display it at all
>> if it is not supported on a given PSU version. This should be relatively
>> easy to catch in the is_visible function.
> 
> So do you have any idea how to do it? The PSU does not tell you what is
> supported or not, you only find out by running the commands. I mean the
> only thing I think of is like I did it for the critical values, but only
> keeping the *_support bits. But if I do it that way, I actually should do
> it for all the commands. This is the point which I ment with "worth the
> trouble."
>

It is not really necessary to do it for all commands; only for those known
to not be supported on all power supplies.

>> Nice PS, anyway. Too bad it is so expensive (and large). Do you know
>> if the HX750i uses the same protocol ?
> 
> All HX_num_i and RM_num_i PSUs support the same protocol. There are only
> small differences in supported commands based on release version. What do
> you mean by "large"? The size of the case? All HXi and RMi should have
> the same size (standard ATX). Maybe you looked at one of the AXi series,

They don't fit into my small mini-ATX chassis, and the "i" series all
seem to be full-size ATX.

[ ... ]
>>
>> Making the code less readable to meet a line limit isn't really that desirable.
>> If you want to stick with one line, you could drop the "hwmon_" from function prefixes
>> instead. Those don't really add any value.
> 
> I know I know, it's a personal taste. I really dislike splitting functions
> headers about serveral lines, especially if indenting is tab based.
> 
Fortunately you can get there by dropping 'hwmon_' from the function names.

Thanks,
Guenter