Message-ID: <YHWQnoYY6LLe4rej@orome.fritz.box>
Date:   Tue, 13 Apr 2021 14:37:50 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     Uwe Kleine-König 
        <u.kleine-koenig@...gutronix.de>, linux-pwm@...r.kernel.org,
        Sven Van Asbroeck <TheSven73@...il.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 1/8] pwm: pca9685: Switch to atomic API

On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> Hi Uwe,
> 
> On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote:
> > Hello Clemens,
> > 
> > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > previously experienced issues:
> > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > >   OFF registers were cleared in disable and the user was required to
> > > > >   call config to restore the duty cycle settings)
> > > > > - If one sets a period resulting in the same prescale register value,
> > > > >   the sleep and write to the register is now skipped
> > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > >   OFF cleared if set to high), which could result in both full OFF and
> > > > >   full ON not being set and on=0, off=0, which is not allowed according
> > > > >   to the datasheet
> > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > >   forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > > 
> > > > > Signed-off-by: Clemens Gruber <clemens.gruber@...ruber.com>
> > > > > ---
> > > > > Changes since v7:
> > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > - Removed unnecessary cast
> > > > > - Use DIV_ROUND_DOWN in .apply
> > > > > 
> > > > > Changes since v6:
> > > > > - Order of a comparison switched for improved readability
> > > > > 
> > > > > Changes since v5:
> > > > > - Function documentation for set_duty
> > > > > - Variable initializations
> > > > > - Print warning if all LEDs channel
> > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > - Improved error messages
> > > > > - Register reset corrections moved to this patch
> > > > > 
> > > > > Changes since v4:
> > > > > - Patches split up
> > > > > - Use a single set_duty function
> > > > > - Improve readability / new macros
> > > > > - Added a patch to restrict prescale changes to the first user
> > > > > 
> > > > > Changes since v3:
> > > > > - Refactoring: Extracted common functions
> > > > > - Read prescale register value instead of caching it
> > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > - Improved duty calculation / mapping to 0..4096
> > > > > 
> > > > > Changes since v2:
> > > > > - Always set default prescale value in probe
> > > > > - Simplified probe code
> > > > > - Inlined functions with one callsite
> > > > > 
> > > > > Changes since v1:
> > > > > - Fixed a logic error
> > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > - Write default prescale reg value if invalid in probe
> > > > > - Reuse full_off/_on functions throughout driver
> > > > > - Use cached prescale value whenever possible
> > > > > 
> > > > >  drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > >  1 file changed, 89 insertions(+), 170 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > index 4a55dc18656c..827b57ced3c2 100644
> > > > > --- a/drivers/pwm/pwm-pca9685.c
> > > > > +++ b/drivers/pwm/pwm-pca9685.c
> > > > > @@ -51,7 +51,6 @@
> > > > >  #define PCA9685_PRESCALE_MAX	0xFF	/* => min. frequency of 24 Hz */
> > > > >  
> > > > >  #define PCA9685_COUNTER_RANGE	4096
> > > > > -#define PCA9685_DEFAULT_PERIOD	5000000	/* Default period_ns = 1/200 Hz */
> > > > >  #define PCA9685_OSC_CLOCK_MHZ	25	/* Internal oscillator with 25 MHz */
> > > > >  
> > > > >  #define PCA9685_NUMREGS		0xFF
> > > > > @@ -71,10 +70,14 @@
> > > > >  #define LED_N_OFF_H(N)	(PCA9685_LEDX_OFF_H + (4 * (N)))
> > > > >  #define LED_N_OFF_L(N)	(PCA9685_LEDX_OFF_L + (4 * (N)))
> > > > >  
> > > > > +#define REG_ON_H(C)	((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
> > > > > +#define REG_ON_L(C)	((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
> > > > > +#define REG_OFF_H(C)	((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
> > > > > +#define REG_OFF_L(C)	((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
> > > > 
> > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > > 
> > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > also be prefixed, right?
> > 
> > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > IMHO it's good style and even though it yields longer name usually it
> > yields easier understandable code. (But this seems to be subjective.)
> 
> I am not sure I want to also rename the existing LED_N_OFF stuff in this
> patch. Maybe we can discuss unifying the macros (either with or without
> prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> match the LED_N_ stuff?

Uwe guessed right, I don't think these need the prefix. There's very
little potential for name clashes and this driver is already called
PCA9685, so adding the prefix everywhere can be annoying, especially
if that then causes you to need line breaks, etc.

For things like function names I usually prefer having a consistent
prefix because it makes it much easier to decipher stack traces. Not so
with the defines here.

But I also don't feel strongly, so either way is fine. If you decide to
go with the prefix, making it consistent throughout the file in a
separate patch would be preferable, though.

> 
> > 
> > > > > +	val = 0;
> > > > > +	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > 
> > > > I asked in the last round why you initialize val. You answered "just to
> > > > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > > > if
> > > > 
> > > > 	ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > 	if (!ret)
> > > > 		/* 
> > 
> > I intended to write something like
> > 
> > 		/* initialize val in case reading LED_N_OFF failed */
> > 
> > > > 		val = 0
> > > > 
> > > > would be better then and also make the intention obvious.
> 
> Maybe a little bit better.. I can change it.
> 
> > > 
> > > I am not sure if that's more clear, but if others find it more obvious
> > > like this, I can change it.
> > > 
> > > > 
> > > > > +	return ((off_h & 0xf) << 8) | (val & 0xff);
> > > > > +}
> > > > > +
> > > > >  #if IS_ENABLED(CONFIG_GPIOLIB)
> > > > >  static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > > >  {
> > > > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > > >  static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > > >  {
> > > > >  	struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > -	struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > -	unsigned int value;
> > > > >  
> > > > > -	regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > > > -
> > > > > -	return value & LED_FULL;
> > > > > +	return pca9685_pwm_get_duty(pca, offset) != 0;
> > > > >  }
> > > > >  
> > > > >  static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > > >  				 int value)
> > > > >  {
> > > > >  	struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > -	struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > -	unsigned int on = value ? LED_FULL : 0;
> > > > > -
> > > > > -	/* Clear both OFF registers */
> > > > > -	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > > > -	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > > > >  
> > > > > -	/* Set the full ON bit */
> > > > > -	regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > > > +	pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > > >  }
> > > > >  
> > > > >  static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > > >  {
> > > > >  	struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > >  
> > > > > -	pca9685_pwm_gpio_set(gpio, offset, 0);
> > > > > +	pca9685_pwm_set_duty(pca, offset, 0);
> > > > >  	pm_runtime_put(pca->chip.dev);
> > > > >  	pca9685_pwm_clear_inuse(pca, offset);
> > > > >  }
> > > > > @@ -246,167 +283,52 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > -			      int duty_ns, int period_ns)
> > > > > +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > > +			     const struct pwm_state *state)
> > > > >  {
> > > > > [...]
> > > > > +	prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > > > +					 PCA9685_COUNTER_RANGE * 1000) - 1;
> > > > > +	if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > > > +		dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > > > +		return -EINVAL;
> > > > >  	}
> > > > >  
> > > > > [...]
> > > > > +	if (!state->enabled) {
> > > > > +		pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > > >  		return 0;
> > > > >  	}
> > > > >  
> > > > > [...]
> > > > > +	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > > > > +	if (prescale != val) {
> > > > > +		/*
> > > > > +		 * Putting the chip briefly into SLEEP mode
> > > > > +		 * at this point won't interfere with the
> > > > > +		 * pm_runtime framework, because the pm_runtime
> > > > > +		 * state is guaranteed active here.
> > > > > +		 */
> > > > > +		/* Put chip into sleep mode */
> > > > > +		pca9685_set_sleep_mode(pca, true);
> > > > >  
> > > > > [...]
> > > > > +		/* Change the chip-wide output frequency */
> > > > > +		regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > > >  
> > > > > -	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > > > +		/* Wake the chip up */
> > > > > +		pca9685_set_sleep_mode(pca, false);
> > > > > +	}
> > > > >  
> > > > > +	duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > > > +	duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> > > > 
> > > > If you round down here you should probably also round down in the
> > > > calculation of prescale. Also note that you're losing precision by using
> > > > state->period.
> > > > 
> > > > Consider the following request: state->period = 4177921 [ns] +
> > > > state->duty_cycle = 100000 [ns], then we get
> > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > actually configure 96 to get 99840 ns.
> > > > 
> > > > So in the end I'd like to have the following:
> > > > 
> > > > 	PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > > 
> > > > (to get the biggest period not bigger than state->period) and
> > > > 
> > > > 	duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > > 
> > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > the example above this yields
> > > > 
> > > > 	PRESCALE = 24
> > > > 	duty = 100
> > > > 
> > > > which results in
> > > > 
> > > > 	.period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > 	.duty_cycle = 100000 [ns]
> > > > 	
> > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > please either stick to the old behaviour or do it right immediately.
> > > 
> > > I avoided rounding down the prescale value because the datasheet has an
> > > example where a round-closest is used, see page 25.
> > 
> > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > rules for Linux PWM drivers :-)
> > 
> > > With your suggested round-down, the example with frequency of 200 Hz
> > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > 
> > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > the better one depends on the consumer, no matter what rounding
> > algorithm the data sheet suggests. Also note that the math here contains
> > surprises you don't expect at first. For example, what PRESCALE value
> > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > press Space now to pause and let you think first :-)] The data sheet's
> > formula suggests:
> > 
> > 	round(25 MHz / (4096 * 284)) - 1 = 20
> > 
> > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > (error = 6.567 Hz), so 21 is the better choice.
> > 
> > Exercise for the reader:
> >  What is the correct formula to really determine the PRESCALE value that
> >  yields the best approximation (i.e. minimizing
> >  abs(real_freq - target_freq)) for a given target_freq?
> > 
> > These things don't happen when you round down only.
> 
> Sure, but it might also be counterintuitive that the Linux driver does
> not use the same formula as the datasheet. And when using 200 Hz, 29 is
> a little closer than 30.
> I once measured the actual frequency and the internal oscillator is not
> very accurate, so even if you think you should get 196.88 Hz, the actual
> frequency measured with a decent scope is about 206 Hz and varies from
> chip to chip (~ 205-207 Hz).
> 
> > 
> > > So would you rather have me keep the old duty rounding behaviour?
> > > 
> > > Meaning: Keep rounding up the duty calculation in apply and use
> > > round-down in the new .get_state function?
> > 
> > There are two things I want:
> > 
> >  a) To improve consistency among the PWM drivers (and to keep the math
> >     simple and unsurprising), the pca9685 driver should use round-down
> >     instead of round-nearest (or whatever mix it is currently using).
> > 
> >  b) .get_state should be the inverse to .apply in the sense that
> >     applying the result of .get_state is idempotent.
> > 
> > I don't care much how you get there, so it's up to you if you do that in
> > this patch that converts to .apply, or if you keep the math as is and
> > then adapt the rounding behaviour in a separate change. But changing the
> > algorithm in this patch and not getting to the "good" one is ugly, so
> > please don't do that.
> 
> OK, then I think the best option is to keep the math as it was before
> and if we want to change the rounding behaviour we do this in a separate
> patch in the future. Then we can continue the discussion wether changing
> the prescaler formula to round-down even though the datasheet does it
> otherwise is the way to go.

I agree, keeping the existing behaviour is preferable. The series
already changes plenty as-is, and rolling the rounding change into it
may make it more difficult to revert/fix later on.

We can always consider a follow-up patch to change it.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists