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

[Guenter Roeck <linux@roeck-us.net>]

On 3/15/21 9:55 AM, Wilken Gottwalt wrote:
> On Mon, 15 Mar 2021 08:53:25 -0700
> Guenter Roeck <linux@roeck-us.net> 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.

Nice PS, anyway. Too bad it is so expensive (and large). Do you know
if the HX750i uses the same protocol ?

[ ... ]

>>> +static void corsairpsu_get_criticals(struct corsairpsu_data *priv)
>>> +{
>>> +	long tmp;
>>> +	int ret;
>>> +	u8 rail;
>>
>> Side note: Using anything but sizeof(int) for index variables often results in more
>> complex code because the compiler needs to mask index operations. This doesn't
>> typically apply to x86 because that architecure has byte operations, but to pretty
>> much every other architecture.
> 
> Yeah, I know what you mean. I thought I match it to the function parameters.

That doesn't really help if it makes the generated code more complex.

> Though I would be more concerned about the "case 1 ... 3" stuff which is
> definitly no liked by static code analysis or even "-pedantic", it's not
> part of the C standards.
> 

Hmm, which C standard are we talking about ? There are more than 1,800
instances of this in the Linux kernel, but I don't recall any static
analyzer or compiler complaints about it.

[ ... ]

>>> -static int corsairpsu_hwmon_ops_read(struct device *dev, enum hwmon_sensor_types type, u32
>>> attr,
>>> -				     int channel, long *val)
>>> +static int corsairpsu_hwmon_temp(struct corsairpsu_data *priv, u32 attr, int channel, long
>>> *val)
>>
>> Please make those functions _<type>_read, not just _<type>. It makes the code easier
>> to understand, and we won't have to change it if write functions are ever added.
> 
> You will laugh... I named the functions that way before, but then I was unhappy
> with hitting the 100 chars line length. ;-)
> 

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.

Thanks,
Guenter