Re: [PATCH] [RFC] hwmon: nct6775: Register fan PWMs as PWM chip

[Uwe Kleine-König]

Hello Richard,

On Thu, Feb 26, 2026 at 07:36:44PM +0100, Richard Weinberger wrote:
> ----- Ursprüngliche Mail -----
> > Von: "Uwe Kleine-König" <ukleinek@xxxxxxxxxx>
> >> +	struct nct6775_data *data = pwmchip_get_drvdata(chip);
> >> +	const u8 *wfhw = _wfhw;
> >> +
> >> +	if (get_pwm_period(data, pwm->hwpwm, &wf->period_length_ns))
> >> +		return 1;
> > 
> > That looks wrong. In principle nct6775_pwm_round_waveform_fromhw()
> > doesn't depend on hardware state. It's supposed to just convert the
> > settings stored in _wfhw to wf. If you know that some things are
> > constant during the lifetime of the PWM and you read those from
> > hardware, return a proper error code, not 1.
> 
> I see. Since the frequency is never changed by the driver we could
> read it while probing and use here the cached value.

I don't care much why the period length is constant. With the idiom from
this patch a comment would be nice, reading once is also fine.

> > Rounding down wf->period_length_ns is fine, so this must be:
> > 
> >	if (wf->period_length_ns < cur_period)
> >		return 1;
> 
> But then the period is no longer fixed and something larger than supported
> can get configured. Smaller values get caught, though.

that's wrong. The period is still fixed.

> e.g.
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# cat period 
> 43243
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 43200 > period 
> bash: echo: write error: Invalid argument
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo 50000 > period 
> root@fedora:/sys/class/pwm/pwmchip0/pwm2# echo $?
> 0

Yes, you can request 50000, but the driver will round that down to
43243. If you have commit aa12c7e70319c9746e55e5b00a215119ba838dad, a
look into /sys/kernel/debug/pwm should confirm that.

(That's a weakness of the sysfs API, you cannot get feedback about how
far the driver has to modify your request to adapt it to the hardware.
Did I mention that you should better use libpwm tools for such tests?
pwm_round will tell you what the driver does with a certain request.)

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature