Re: [hwmon-next PATCH v4 3/4] hwmon: (adt7470) Expose fan control via PWM framework

[Luiz Angelo Daros de Luca]

Hi Guenter, Uwe,

Thank you both for the detailed reviews. I hope I'll address all the
points discussed in the next version.

> >> No, I won't accept this. It has to be either or. A pwm channel modeled as pwm
> >> subsystem channel must not be visible via hwmon attributes.
> >
> > I didn't look in detail, but maybe my red line is a bit more lax. IMHO
> > it's not OK to be able to modify a setting that results from pushing
> > hwmon knobs using PWM operations or vice-versa. Switching between
> > exclusive use at runtime is IMHO ok.
> >
> It is my call to make, and my call is either-or to avoid confusion.

Guenter, I agree and followed your strict "either-or" directive. If
the device is bound via the PWM framework (i.e., #pwm-cells is present
in the DT), the driver completely hides all legacy hwmon PWM
attributes.

One implementation detail: currently, the driver hides the hwmon
attributes only if #pwm-cells is present and IS_REACHABLE(CONFIG_PWM)
is true. If the DTS has #pwm-cells but the PWM framework is disabled
in the kernel, it emits a dev_warn and falls back to exposing the
hwmon attributes. Please let me know if you would prefer to
unconditionally hide the hwmon attributes merely based on the presence
of the DT property, regardless of the CONFIG_PWM state.

> > +static int adt7470_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > +                                        struct pwm_device *pwm,
> > +                                        const struct pwm_waveform *wf,
> > +                                        void *_wfhw)
> > +{
> > +     struct adt7470_data *data = pwmchip_get_drvdata(chip);
> > +     struct adt7470_pwm_wfhw *wfhw = _wfhw;
> > +     u64 period_ns;
> > +
> > +     if (wf->duty_length_ns == 0) {
> > +             wfhw->val = 0;
> > +             return 0;
> > +     }
> > +
> > +     /*
> > +      * The PWM frequency (period) is a single chip-wide setting shared by
> > +      * all 4 channels, so it cannot be changed on a per-pwm_device basis
> > +      * through this API. The duty cycle is rounded against the currently
> > +      * configured hardware period rather than the period requested in
> > +      * @wf; round_waveform_fromhw() reports the actual resulting
> > +      * waveform back so the core/consumer can detect a mismatch.
> > +      */
> > +     period_ns = DIV_ROUND_CLOSEST(NSEC_PER_SEC, data->pwm_freq);
> > +     wfhw->val = min_t(u64,
> > +                       mul_u64_u64_div_u64(wf->duty_length_ns, ADT7470_PWM_MAX, period_ns),
> > +                       ADT7470_PWM_MAX);
>
> This is wrong. Please test your driver with PWM_DEBUG enabled, this
> should emit warnings.
>
> Here the right thing is (untested):
>
>         if (!wf->period_length_ns) {
>                 wfhw->val = 0;
>                 return 0;
>         }
>
>         actual_period = NSEC_PER_SEC / data->pwm_freq;
>
>         if (wf->duty_length_ns >= actual_period)
>                 val = ADT7470_PWM_MAX;
>         else
>                 val = mul_u64_u64_div_u64(wf->duty_length_ns, ADT7470_PWM_MAX, actual_period);
>
>         return (actual_period < wf->period_length_ns) ? 0 : 1;

Uwe, thank you for the waveform callbacks snippet. It was much
cleaner. I only had to make one small mathematical adjustment to the
actual_period calculation in tohw() to satisfy CONFIG_PWM_DEBUG.

When using a truncating division (NSEC_PER_SEC / data->pwm_freq), it
triggered a "Wrong rounding" error during testing:

pwm pwmchip0: Wrong rounding: requested 714285/714286 [+0], result
714286/714286 [+0]

For example, at 1400 Hz, the truncating division gives an
actual_period of 714285 ns. If the framework requests a duty cycle of
714285 ns and a period of 714286 ns, the evaluation (714285 * 255) /
714285 yields a register value of 255. When read back in fromhw()
(which correctly rounds the period up to 714286 ns), the resulting
duty cycle becomes 714286 ns. Since 714286 > 714285, CONFIG_PWM_DEBUG
rightfully complains about the duty cycle inflation.

To ensure the calculated register value is strictly rounded down, I
changed the actual_period denominator in tohw() to use
DIV_ROUND_UP_ULL(NSEC_PER_SEC, data->pwm_freq). Maximizing the
denominator guarantees the resulting duty cycle fraction never
inflates, effectively passing all strict debug checks.

Thanks again for the time and guidance.

Best regards,

Luiz