Re: [RFC PATCH 1/1] hwmon: Add Minisforum UM780 XTX EC monitoring and fan control

["Sebastián Peyrott" <speyrott@gmail.com>]

Thanks for the review. I have prepared the following changes for v2.

> pwm[12]_enable can be implemented as standard attributes. Also,
> looking into the code, pwm2_enable always returns 2, can not be modified,
> and is therefore pointless.

Agreed. pwm1_enable now uses the standard hwmon callbacks, and I removed the
constant pwm2_enable attribute.

> On top of that, the different definitions for pwm1_enable and pwm2_enable
> are confusing. What is the "OEM B1" and "OEM B2" CPU profile, and what
> does it have to do with fan control enable status ?

The EC firmware implements two predefined automatic CPU fan curves. The EC
stores the selected profile as 0xb1 or 0xb2 in its profile register, which is
why I called them B1 and B2. In practice B1 is slightly less aggressive than
B2, although the difference between them is small.

I mapped them to pwm1_enable values 2 and 3 because both select automatic fan
control modes; Documentation/ABI/testing/sysfs-class-hwmon defines values 2+
that way and leaves the individual mode details to the driver documentation.
If B1 and B2 are not useful names for users, I am happy to document them under
more descriptive names instead.

> What is the point of this (constant) non-standard attribute ?

I removed pwm2_auto_channels_temp.

> The pwm2_auto_point[12]_pwm attributes are constant values and therefore
> pointless.

Agreed, I removed them.

> Use hwmon subsystem lock and guard().

Done. The standard attributes rely on the hwmon core serialization. The two
extra threshold attributes and the PM callbacks use guard(hwmon_lock), and the
private mutex is gone.

> clamp and round. We do not expect users to know exact ranges and values.

Done. Threshold values are rounded to whole degrees Celsius and clamped while
preserving strict ordering.

> Since UM780XTX_EC_SYS_POINT3 is never modified, why not read it once ?

It is now read once at probe.

> At least in the current code this is pointless. Just returning 0444 would do.

I simplified is_visible accordingly, with the additional 0644 case for the
standard pwm1_enable attribute.

> channel is always < 2

I removed both redundant channel checks and the redundant locks in the core
read callback.

> Drop "fan control temperature"
>
> Drop "fan". It is confusing that temperature and fan index values are
> reversed. Why is temp1 the SYS temperature, but fan1 the CPU fan speed ?

The labels are now simply "CPU" and "SYS", and the channels are aligned so
temp1/fan1 are CPU and temp2/fan2 are SYS.

> Pointless check. That is what is_visible is for.

Removed.

> Since those are cached states, how would this ever happen ?

I removed the redundant range check from the restore path. State is validated
when it is cached.

> Why those noisy warnings on errors, but no warning if sys_points_valid
> and/or profile_valid are false due to failed range checks ?

I removed the validity flags and runtime warnings. State caching now returns
an error for either transport or validation failures, and probe fails if the
initial state is not coherent.

> This is highly unusual. Normally suspend would save the state
> and resume would restore it. What is the point of _checking_ the state
> on resume ?
>
> Also, what is the point of the delay after resume ? The cached values
> could have been changed via sysfs attributes in the meantime, causing
> spurious warnings.

I replaced the delayed work and module parameter with the conventional
suspend/save and synchronous resume/restore pattern. On the target system I
verified that the selected CPU profile and 50/70 C SYS thresholds were correct
in the first userspace sample after s2idle resume and remained stable for 15
seconds.

I will give this some time for further feedback before sending v2.

Thanks,
Sebastian