Re: [PATCH 4/4] pwm: pca9685: migrate config/enable/disable to apply

[Uwe Kleine-König]

Hello,

On Wed, Feb 26, 2020 at 02:52:29PM +0100, Matthias Schiffer wrote:
> For consistency with disabled state, initialize all LEDs in FULL_OFF
> state during probe.
> 
> This also fixes a broken interaction between config with 100% duty cycle
> (which would set the LED to FULL_ON) and enable (which would unset
> FULL_ON), effectively disabling the LED again when enable was called
> after config. This behaviour was observed with the leds-pwm driver when
> directly switching from 0 to maximum brightness.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@xxxxxxxxxxxxxxx>
> ---
>  drivers/pwm/pwm-pca9685.c | 53 +++++++++++----------------------------
>  1 file changed, 14 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 370691b21107..e266cbbd39bf 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -219,15 +219,16 @@ 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)
>  {
>  	struct pca9685 *pca = to_pca(chip);
>  	unsigned long long duty;
>  	int prescale;
>  
> -	if (period_ns != pca->period_ns) {
> -		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> +	if (state->period != pca->period_ns) {
> +		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ *
> +					     state->period,
>  					     PCA9685_COUNTER_RANGE * 1000) - 1;
>  
>  		if (prescale >= PCA9685_PRESCALE_MIN &&
> @@ -247,7 +248,7 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  			/* Wake the chip up */
>  			pca9685_set_sleep_mode(pca, false);
>  
> -			pca->period_ns = period_ns;
> +			pca->period_ns = state->period;
>  		} else {
>  			dev_err(chip->dev,
>  				"prescaler not set: period out of bounds!\n");
> @@ -255,13 +256,13 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		}
>  	}
>  
> -	if (duty_ns < 1) {
> +	if (!state->enabled || state->duty_cycle < 1) {
> +		/* Set the full OFF bit */
>  		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
> -
>  		return 0;
>  	}
>  
> -	if (duty_ns == period_ns) {
> +	if (state->duty_cycle == state->period) {
>  		/* Clear both OFF registers */
>  		regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
>  		regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0x0);
> @@ -272,8 +273,8 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return 0;
>  	}
>  
> -	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> -	duty = DIV_ROUND_UP_ULL(duty, period_ns);
> +	duty = PCA9685_COUNTER_RANGE * (unsigned long long)state->duty_cycle;
> +	duty = DIV_ROUND_UP_ULL(duty, state->period);
>  
>  	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), (int)duty & 0xff);
>  	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
> @@ -285,29 +286,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;

You seem to ignore state->polarity which is wrong.

>  }
>  
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct pca9685 *pca = to_pca(chip);
> -
> -	/*
> -	 * Clear the full-off bit.
> -	 * It has precedence over the others and must be off.
> -	 */
> -	regmap_update_bits(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL, 0x0);
> -
> -	return 0;
> -}
> -
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct pca9685 *pca = to_pca(chip);
> -
> -	regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), LED_FULL);
> -
> -	/* Clear the LED_OFF counter. */
> -	regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0x0);
> -}
> -
>  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct pca9685 *pca = to_pca(chip);
> @@ -321,14 +299,11 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
> -	pca9685_pwm_disable(chip, pwm);
>  	pm_runtime_put(chip->dev);
>  }
>  
>  static const struct pwm_ops pca9685_pwm_ops = {
> -	.enable = pca9685_pwm_enable,
> -	.disable = pca9685_pwm_disable,
> -	.config = pca9685_pwm_config,
> +	.apply = pca9685_pwm_apply,
>  	.request = pca9685_pwm_request,
>  	.free = pca9685_pwm_free,
>  	.owner = THIS_MODULE,
> @@ -377,9 +352,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  
>  	regmap_write(pca->regmap, PCA9685_MODE2, mode2);
>  
> -	/* clear all "full off" bits */
> +	/* Set all LEDs full off */
>  	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);

This looks wrong or at least unrelated?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |