Re: [PATCH v6 6/7] pwm: pca9685: Restrict period change for prescaler users

[Clemens Gruber <clemens.gruber@pqgruber.com>]

On Mon, Mar 29, 2021 at 07:15:59PM +0200, Uwe Kleine-König wrote:
> On Mon, Mar 29, 2021 at 02:57:06PM +0200, Clemens Gruber wrote:
> > @@ -330,14 +345,22 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  
> >  	if (!state->enabled || duty < 1) {
> >  		pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > +		clear_bit(pwm->hwpwm, pca->prescaler_users);
> 
> Hmm, so if "my" channel runs at say
> 
> 	.duty_cycle = 2539520 ns
> 	.period = 5079040 ns
> 
> and I call pwm_apply_state(mypwm, { .duty_cycle = 0, .period = 5079040,
> enabled = true }); it might happen that another channel modifies the
> period and I won't be able to return to the initial setting.

Yes, that's correct.

But that also applies to PWMs set to 100%:

pwm_apply_state(mypwm, { .duty_cycle = 5079040, .period = 5079040,
enabled = true });

As this sets the full ON bit and does not use the prescaler, with the
current code, another channel could modify the period and you wouldn't
be able to return to the initial setting of 50%.

> 
> So I think it's sensible to only clear the user bit if the PWM is
> disabled, but not if it is configured for duty_cycle = 0.
> 
> Does this make sense?

With both cases in mind, you are suggesting we block modifications of
the prescaler if other PWMs are enabled and not if other PWMs are using
the prescaler?

Clemens