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

On 7/27/26 12:00, Guenter Roeck wrote:
> 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).
> 
Having said that, and unless I am missing something, it looks like the
only two patches with new issues are the last two patches of the series.
I'll be happy to apply the series after you fixed those issues.

Thanks,
Guenter