Message-ID: <20210316152150.5a712927@monster.powergraphx.local>
Date:   Tue, 16 Mar 2021 15:21:50 +0100
From:   Wilken Gottwalt <wilken.gottwalt@...teo.net>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-kernel@...r.kernel.org, Jean Delvare <jdelvare@...e.com>,
        Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: corsair-psu: add support for critical values

On Mon, 15 Mar 2021 11:00:53 -0700
Guenter Roeck <linux@...ck-us.net> wrote:

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

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

> 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,
which are server grade (and are not supported - they use a full USB protocol,
no USB HID) and really expensive (but they sensors there can do even more).

They are expansive because there is only new-old-stock available and they
have some impressive specs. They can deliver some really high currents,
something you need if you run a Threadripper like me or if you use a gfx
card with current spikes (like AMD Vega or NVidia GTX 970).

> [ ... ]
> 
> >>> +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.

I know I know, it's a personal taste. I really dislike splitting functions
headers about serveral lines, especially if indenting is tab based.

> Thanks,
> Guenter

Powered by blists - more mailing lists