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