Re: [hwmon PATCH v3 4/6] hwmon: (adt7470) Fix swapped PWM3 and PWM4 auto mode masks

On 7/27/26 11:09, Luiz Angelo Daros de Luca wrote:
>> On 7/26/26 20:34, Luiz Angelo Daros de Luca wrote:
>>> The ADT7470_PWM3_AUTO_MASK and ADT7470_PWM4_AUTO_MASK macros are
>>> currently defined with swapped bit values.
>>>
>>> According to Table 22 of the ADT7470 datasheet, the Fan Control Mode
>>> Configuration for register 0x69 follows the exact same bit position
>>> layout as register 0x68:
>>> - 0x68 Bit[7] corresponds to BHVR1 (PWM1) -> 0x80
>>> - 0x68 Bit[6] corresponds to BHVR2 (PWM2) -> 0x40
>>> - 0x69 Bit[7] corresponds to BHVR3 (PWM3) -> 0x80
>>> - 0x69 Bit[6] corresponds to BHVR4 (PWM4) -> 0x40
>>>
>>> Consequently, PWM3 should use mask 0x80 and PWM4 should use 0x40.
>>>
>>> This typo did not cause any functional bugs because these specific
>>> macros are never referenced in the driver code. Instead, the driver
>>> correctly applies the configuration by relying on the modulo parity of
>>> the channel index (e.g., `channel % 2`) to selectively apply either
>>> ADT7470_PWM1_AUTO_MASK (0x80) or ADT7470_PWM2_AUTO_MASK (0x40).
>>> Since the bit layout is identical between the two configuration
>>> registers, the hardware is currently configured correctly.
>>>
>>> Fix the macro definitions to reflect the datasheet accurately and
>>> prevent future bugs or confusion during code review and refactoring.
>>> As this is a purely cosmetic fix with no functional impact, a backport
>>> to stable kernels is not necessary.
> 
> If you build with W=2, you indeed get tons of unused macros...
> 
> Initially, I intended to just drop them. However, just like the
> incorrect macro led Sashiko (and other bots) to assume the masks were
> swapped, keeping them correctly defined serves as useful context
> directly in the code. If we drop them entirely, anyone (or any AI
> agent/tool analyzing the driver) would need to fetch and parse the
> datasheet to verify the register layout. While the datasheet is the
> ultimate source of truth, reading PDF tables costs more than reading
> well-defined driver constants, to the point that agents might simply
> skip it.
> 
> We could certainly drop the unused macros and add a comment explaining
> the layout, or even rename them to better reflect their shared usage
> across channels (e.g., ADT7470_PWM_ODD_AUTO_MASK and
> ADT7470_PWM_EVEN_AUTO_MASK). That said, simply fixing the unused
> definition feels like the cleanest and least verbose option. What do
> you think?
> 
>> If the masks are unused, how about just dropping the definitions instead
>> (including the unused ADT7470_REG_PWM12_CFG and ADT7470_REG_PWM34_CFG) ?
>> That could be done as first patch of the series.
> 
> I'm almost afraid of touching anything else here! Fixing bugs in this
> driver feels like fighting a hydra: every time you address one, the
> Sashiko bot sprouts two more. :-) I already have seven.
> 
Good point. Normally a driver should only provide definitions which are
actually used, but for an old driver like this one ends up with a lot of
churn.

Note that you don't_ have_ to fix all the reported pre-existing issues.
It is very much appreciated if you do, and I tried to do it myself, but
nowadays I only fix (or try to fix) the issues marked as critical and/or
as security issues. All I really ask for is that patch authors fix all new
issues (and that they don't tell me that I should accept new issues because
the driver or some other driver is already buggy).

Thanks,
Guenter