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

[Guenter Roeck]

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