Re: [PATCH v2] hwmon: Add LattePanda Sigma EC driver

[Guenter Roeck]

On Mon, Mar 02, 2026 at 03:35:14PM -0300, Mariano Abad wrote:
> The LattePanda Sigma is an x86 single-board computer made by DFRobot,
> featuring an Intel Core i5-1340P and an ITE IT8613E Embedded Controller
> that manages fan speed and thermal monitoring.
> 
> The BIOS declares the ACPI Embedded Controller (PNP0C09) with _STA
> returning 0 and provides only stub ECRD/ECWT methods. Since the kernel's
> ACPI EC subsystem never initializes, ec_read() is not available and
> direct port I/O to the standard ACPI EC ports (0x62/0x66) is used. As
> ACPI never accesses the EC, no ACPI Global Lock or namespace mutex is
> required.
> 
> The driver exposes:
>   - CPU fan speed (RPM, read-only)
>   - Board temperature (unsigned 8-bit, degrees Celsius)
>   - CPU proximity temperature (unsigned 8-bit, degrees Celsius)
> 
> DMI matching restricts the driver to verified LattePanda Sigma hardware
> with BIOS version 5.27. A 'force' module parameter allows loading on
> untested BIOS versions while still requiring vendor/product match.
> 
> The EC register map was discovered through firmware reverse engineering
> and confirmed by physical testing.
> 

I had trouble to find this patch again and found that you sent it as reply
to the first version.

Documentation/process/submittingpatches.rst explains why you should
never do that.

Anyway, additional feedback:

> +/* Read a 16-bit big-endian value from two consecutive EC registers. */
> +static int ec_read_reg16(u8 reg_hi, u8 reg_lo, u16 *val)
> +{
> +	int ret;
> +	u8 hi, lo;
> +
> +	ret = ec_read_reg(reg_hi, &hi);
> +	if (ret)
> +		return ret;
> +
> +	ret = ec_read_reg(reg_lo, &lo);
> +	if (ret)
> +		return ret;
> +
> +	*val = ((u16)hi << 8) | lo;
> +	return 0;
> +}

Is it possible for the 16-bit value (e.g., fan RPM) to roll over between
reading the high and low bytes? Without latching or verifying that the high
byte did not change, reading a rapidly changing 16-bit value as two
independent 8-bit reads can yield a corrupted result (e.g., if it rolls over
from 0x0100 to 0x00FF between reads, producing 0x01FF).

This is AI generated, but it does have a point. lm90 had a similar problem,
which it kind of solved in lm90_read16(). You might want to consider using
a similar approach unless it is guaranteed that this is not a problem.
If so, please add a comment explaining why it is not a problem.

Thanks,
Guenter