[PATCH 1/3] pwm: pca9685: Switch to atomic API

[PATCH 1/3] pwm: pca9685: Switch to atomic API

[Clemens Gruber]

This 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)
- The prescale register is now read out. If one sets a period resulting
  in the same prescale register value, the sleep and write to the
  register is skipped

The hardware readout may return slightly different values than those
that were set in apply due to the limited range of possible prescale and
counter register values. If one channel is reconfigured with new duty
cycle and period, the others will keep the same relative duty cycle to
period ratio as they had before, even though the per-chip / global
frequency changed. (The PCA9685 has only one prescaler!)

Note that although the datasheet mentions 200 Hz as default frequency
when using the internal 25 MHz oscillator, the calculated period from
the default prescaler register setting of 30 is 5079040ns.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 233 ++++++++++++++++++++------------------
 1 file changed, 124 insertions(+), 109 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..20f1314e6754 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
@@ -74,7 +73,7 @@
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
-	int period_ns;
+	int prescale;
 #if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
 	struct gpio_chip gpio;
@@ -246,18 +245,117 @@ 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 void pca9685_pwm_full_off(struct pwm_chip *chip,
+				 struct pwm_device *pwm)
+{
+	struct pca9685 *pca = to_pca(chip);
+	int reg;
+
+	/*
+	 * Set the full OFF bit to cause the PWM channel to be always off.
+	 * The full OFF bit has precedence over the other register values.
+	 */
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN)
+		reg = PCA9685_ALL_LED_OFF_H;
+	else
+		reg = LED_N_OFF_H(pwm->hwpwm);
+
+	regmap_write(pca->regmap, reg, LED_FULL);
+}
+
+static void pca9685_pwm_full_on(struct pwm_chip *chip,
+				struct pwm_device *pwm)
+{
+	struct pca9685 *pca = to_pca(chip);
+	int reg;
+
+	/*
+	 * Clear the OFF registers (including the full OFF bit) and set
+	 * the full ON bit to cause the PWM channel to be always on.
+	 */
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN)
+		reg = PCA9685_ALL_LED_OFF_L;
+	else
+		reg = LED_N_OFF_L(pwm->hwpwm);
+
+	regmap_write(pca->regmap, reg, 0x0);
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN)
+		reg = PCA9685_ALL_LED_OFF_H;
+	else
+		reg = LED_N_OFF_H(pwm->hwpwm);
+
+	regmap_write(pca->regmap, reg, 0x0);
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN)
+		reg = PCA9685_ALL_LED_ON_H;
+	else
+		reg = LED_N_ON_H(pwm->hwpwm);
+
+	regmap_write(pca->regmap, reg, LED_FULL);
+}
+
+static int pca9685_pwm_read_global_period(struct pca9685 *pca)
+{
+	unsigned int prescale = 0;
+
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
+
+	if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
+		return 0;
+
+	return (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+		(prescale + 1);
+}
+
+static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct pca9685 *pca = to_pca(chip);
+	unsigned int val, duty;
+	int reg;
+
+	/* Read out (chip-wide) period */
+	state->period = pca9685_pwm_read_global_period(pca);
+
+	/* The (per-channel) polarity is fixed */
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	/* Read out current duty cycle and enabled state */
+	reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
+		LED_N_OFF_H(pwm->hwpwm);
+	regmap_read(pca->regmap, reg, &val);
+	duty = (val & 0xf) << 8;
+
+	state->enabled = !(val & LED_FULL);
+
+	reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
+		LED_N_OFF_L(pwm->hwpwm);
+	regmap_read(pca->regmap, reg, &val);
+	duty |= (val & 0xff);
+
+	if (duty < PCA9685_COUNTER_RANGE) {
+		duty *= state->period;
+		state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
+	} else
+		state->duty_cycle = 0;
+}
+
+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;
+	unsigned long long duty, prescale;
 	unsigned int reg;
-	int prescale;
 
-	if (period_ns != pca->period_ns) {
-		prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
-					     PCA9685_COUNTER_RANGE * 1000) - 1;
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EOPNOTSUPP;
 
+	prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
+					 PCA9685_COUNTER_RANGE * 1000) - 1;
+	if (prescale != pca->prescale) {
 		if (prescale >= PCA9685_PRESCALE_MIN &&
 			prescale <= PCA9685_PRESCALE_MAX) {
 			/*
@@ -270,12 +368,13 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 			pca9685_set_sleep_mode(pca, true);
 
 			/* Change the chip-wide output frequency */
-			regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
+			regmap_write(pca->regmap, PCA9685_PRESCALE,
+				     (int)prescale);
 
 			/* Wake the chip up */
 			pca9685_set_sleep_mode(pca, false);
 
-			pca->period_ns = period_ns;
+			pca->prescale = (int)prescale;
 		} else {
 			dev_err(chip->dev,
 				"prescaler not set: period out of bounds!\n");
@@ -283,46 +382,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		}
 	}
 
-	if (duty_ns < 1) {
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_H;
-		else
-			reg = LED_N_OFF_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, LED_FULL);
-
+	if (!state->enabled || state->duty_cycle < 1) {
+		pca9685_pwm_full_off(chip, pwm);
 		return 0;
 	}
 
-	if (duty_ns == period_ns) {
-		/* Clear both OFF registers */
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_L;
-		else
-			reg = LED_N_OFF_L(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, 0x0);
-
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_H;
-		else
-			reg = LED_N_OFF_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, 0x0);
-
-		/* Set the full ON bit */
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_ON_H;
-		else
-			reg = LED_N_ON_H(pwm->hwpwm);
-
-		regmap_write(pca->regmap, reg, LED_FULL);
-
+	if (state->duty_cycle == state->period) {
+		pca9685_pwm_full_on(chip, pwm);
 		return 0;
 	}
 
-	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
-	duty = DIV_ROUND_UP_ULL(duty, period_ns);
+	duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
+	duty = DIV_ROUND_UP_ULL(duty, state->period);
 
 	if (pwm->hwpwm >= PCA9685_MAXCHAN)
 		reg = PCA9685_ALL_LED_OFF_L;
@@ -349,64 +420,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-	unsigned int reg;
-
-	/*
-	 * The PWM subsystem does not support a pre-delay.
-	 * So, set the ON-timeout to 0
-	 */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_L;
-	else
-		reg = LED_N_ON_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_H;
-	else
-		reg = LED_N_ON_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0);
-
-	/*
-	 * Clear the full-off bit.
-	 * It has precedence over the others and must be off.
-	 */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
-
-	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
-
-	return 0;
-}
-
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-	unsigned int reg;
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, LED_FULL);
-
-	/* Clear the LED_OFF counter. */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_L;
-	else
-		reg = LED_N_OFF_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0x0);
-}
-
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
@@ -422,15 +435,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
-	pca9685_pwm_disable(chip, pwm);
+	pca9685_pwm_full_off(chip, pwm);
 	pm_runtime_put(chip->dev);
 	pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
 }
 
 static const struct pwm_ops pca9685_pwm_ops = {
-	.enable = pca9685_pwm_enable,
-	.disable = pca9685_pwm_disable,
-	.config = pca9685_pwm_config,
+	.get_state = pca9685_pwm_get_state,
+	.apply = pca9685_pwm_apply,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
@@ -448,7 +460,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 {
 	struct pca9685 *pca;
 	unsigned int reg;
-	int ret;
+	int prescale = 0, ret;
 
 	pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
 	if (!pca)
@@ -461,10 +473,13 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
-	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
+	if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
+		pca->prescale = prescale;
+
 	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
 
 	if (device_property_read_bool(&client->dev, "invert"))
-- 
2.29.2

[PATCH 2/3] pwm: pca9685: Set full OFF bits in probe

[Clemens Gruber]

The full OFF bits are set by default in the PCA9685 LEDn_OFF_H
registers at POR. LEDn_ON_L/H and LEDn_OFF_L default to 0.

The datasheet states that LEDn_OFF and LEDn_ON should never be both set
to the same values.

This patch removes the clearing of the full OFF bit in the probe
function. We still clear the rest of the OFF regs but now we set the
full OFF bit to avoid having both OFF and ON registers set to 0 and
start up in a safe default state.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 20f1314e6754..88028222335f 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -499,9 +499,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
 	regmap_write(pca->regmap, PCA9685_MODE1, reg);
 
-	/* Clear all "full off" bits */
+	/* Reset OFF registers to HW default (only full OFF bit is set) */
 	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);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* Add an extra channel for ALL_LED */
-- 
2.29.2

[PATCH 3/3] pwm: pca9685: Support staggered output ON times

[Clemens Gruber]

The PCA9685 supports staggered LED output ON times to minimize current
surges and reduce EMI.
When this new option is enabled, the ON times of each channel are
delayed by channel number x counter range / 16, which avoids asserting
all enabled outputs at the same counter value while still maintaining
the configured duty cycle of each output.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/pwm-pca9685.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 88028222335f..95958b5cce0d 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -74,6 +74,7 @@ struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
 	int prescale;
+	bool staggered_outputs;
 #if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
 	struct gpio_chip gpio;
@@ -348,7 +349,7 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty, prescale;
-	unsigned int reg;
+	unsigned int on, off, reg;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EOPNOTSUPP;
@@ -395,6 +396,32 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
 	duty = DIV_ROUND_UP_ULL(duty, state->period);
 
+	if (pca->staggered_outputs) {
+		if (pwm->hwpwm < PCA9685_MAXCHAN) {
+			/*
+			 * To reduce EMI, the ON times of each channel are
+			 * spread out evenly within the counter range, while
+			 * still maintaining the configured duty cycle
+			 */
+			on = pwm->hwpwm * PCA9685_COUNTER_RANGE /
+				PCA9685_MAXCHAN;
+			off = (on + duty) % PCA9685_COUNTER_RANGE;
+			regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm),
+				     on & 0xff);
+			regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm),
+				     (on >> 8) & 0xf);
+			regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm),
+				     off & 0xff);
+			regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
+				     (off >> 8) & 0xf);
+			return 0;
+		}
+
+		/* No staggering possible if "all LEDs" channel is used */
+		regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+		regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+	}
+
 	if (pwm->hwpwm >= PCA9685_MAXCHAN)
 		reg = PCA9685_ALL_LED_OFF_L;
 	else
@@ -494,6 +521,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	regmap_write(pca->regmap, PCA9685_MODE2, reg);
 
+	pca->staggered_outputs = device_property_read_bool(
+		&client->dev, "staggered-outputs");
+
 	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
 	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
 	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
@@ -502,6 +532,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	/* Reset OFF registers to HW default (only full OFF bit is set) */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
+	/* Reset ON registers to HW default */
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* Add an extra channel for ALL_LED */
-- 
2.29.2

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Sven Van Asbroeck]

Hi Clemens, thank you so much for this contribution.
I no longer have access to this chip, so I cannot test
the changes.

Some friendly/constructive feedback below.

On Wed, Nov 18, 2020 at 12:44 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> This 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)
> - The prescale register is now read out. If one sets a period resulting
>   in the same prescale register value, the sleep and write to the
>   register is skipped
>
> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values. If one channel is reconfigured with new duty
> cycle and period, the others will keep the same relative duty cycle to
> period ratio as they had before, even though the per-chip / global
> frequency changed. (The PCA9685 has only one prescaler!)
>
> Note that although the datasheet mentions 200 Hz as default frequency
> when using the internal 25 MHz oscillator, the calculated period from
> the default prescaler register setting of 30 is 5079040ns.
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
>  drivers/pwm/pwm-pca9685.c | 233 ++++++++++++++++++++------------------
>  1 file changed, 124 insertions(+), 109 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..20f1314e6754 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
> @@ -74,7 +73,7 @@
>  struct pca9685 {
>         struct pwm_chip chip;
>         struct regmap *regmap;
> -       int period_ns;
> +       int prescale;

You've decided to cache the prescale register...

>  #if IS_ENABLED(CONFIG_GPIOLIB)
>         struct mutex lock;
>         struct gpio_chip gpio;
> @@ -246,18 +245,117 @@ 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 void pca9685_pwm_full_off(struct pwm_chip *chip,
> +                                struct pwm_device *pwm)
> +{
> +       struct pca9685 *pca = to_pca(chip);
> +       int reg;
> +
> +       /*
> +        * Set the full OFF bit to cause the PWM channel to be always off.
> +        * The full OFF bit has precedence over the other register values.
> +        */
> +
> +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> +               reg = PCA9685_ALL_LED_OFF_H;
> +       else
> +               reg = LED_N_OFF_H(pwm->hwpwm);
> +
> +       regmap_write(pca->regmap, reg, LED_FULL);
> +}
> +
> +static void pca9685_pwm_full_on(struct pwm_chip *chip,
> +                               struct pwm_device *pwm)
> +{
> +       struct pca9685 *pca = to_pca(chip);
> +       int reg;
> +
> +       /*
> +        * Clear the OFF registers (including the full OFF bit) and set
> +        * the full ON bit to cause the PWM channel to be always on.
> +        */
> +
> +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> +               reg = PCA9685_ALL_LED_OFF_L;
> +       else
> +               reg = LED_N_OFF_L(pwm->hwpwm);
> +
> +       regmap_write(pca->regmap, reg, 0x0);
> +
> +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> +               reg = PCA9685_ALL_LED_OFF_H;
> +       else
> +               reg = LED_N_OFF_H(pwm->hwpwm);
> +
> +       regmap_write(pca->regmap, reg, 0x0);
> +
> +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> +               reg = PCA9685_ALL_LED_ON_H;
> +       else
> +               reg = LED_N_ON_H(pwm->hwpwm);
> +
> +       regmap_write(pca->regmap, reg, LED_FULL);
> +}
> +
> +static int pca9685_pwm_read_global_period(struct pca9685 *pca)

but in this function, you don't seem to use the cached prescale
value, but read it out again instead?

> +{
> +       unsigned int prescale = 0;
> +
> +       regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> +
> +       if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
> +               return 0;
> +
> +       return (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> +               (prescale + 1);
> +}
> +
> +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +                                 struct pwm_state *state)
> +{
> +       struct pca9685 *pca = to_pca(chip);
> +       unsigned int val, duty;
> +       int reg;
> +
> +       /* Read out (chip-wide) period */
> +       state->period = pca9685_pwm_read_global_period(pca);
> +
> +       /* The (per-channel) polarity is fixed */
> +       state->polarity = PWM_POLARITY_NORMAL;
> +
> +       /* Read out current duty cycle and enabled state */
> +       reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
> +               LED_N_OFF_H(pwm->hwpwm);
> +       regmap_read(pca->regmap, reg, &val);
> +       duty = (val & 0xf) << 8;
> +
> +       state->enabled = !(val & LED_FULL);
> +
> +       reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
> +               LED_N_OFF_L(pwm->hwpwm);
> +       regmap_read(pca->regmap, reg, &val);
> +       duty |= (val & 0xff);
> +
> +       if (duty < PCA9685_COUNTER_RANGE) {
> +               duty *= state->period;
> +               state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> +       } else
> +               state->duty_cycle = 0;
> +}
> +
> +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;
> +       unsigned long long duty, prescale;
>         unsigned int reg;
> -       int prescale;
>
> -       if (period_ns != pca->period_ns) {
> -               prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> -                                            PCA9685_COUNTER_RANGE * 1000) - 1;
> +       if (state->polarity != PWM_POLARITY_NORMAL)
> +               return -EOPNOTSUPP;
>
> +       prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> +                                        PCA9685_COUNTER_RANGE * 1000) - 1;
> +       if (prescale != pca->prescale) {

Use of cached prescale here, all good.

>                 if (prescale >= PCA9685_PRESCALE_MIN &&
>                         prescale <= PCA9685_PRESCALE_MAX) {
>                         /*
> @@ -270,12 +368,13 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                         pca9685_set_sleep_mode(pca, true);
>
>                         /* Change the chip-wide output frequency */
> -                       regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> +                       regmap_write(pca->regmap, PCA9685_PRESCALE,
> +                                    (int)prescale);
>
>                         /* Wake the chip up */
>                         pca9685_set_sleep_mode(pca, false);
>
> -                       pca->period_ns = period_ns;
> +                       pca->prescale = (int)prescale;
>                 } else {
>                         dev_err(chip->dev,
>                                 "prescaler not set: period out of bounds!\n");
> @@ -283,46 +382,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>                 }
>         }
>
> -       if (duty_ns < 1) {
> -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -                       reg = PCA9685_ALL_LED_OFF_H;
> -               else
> -                       reg = LED_N_OFF_H(pwm->hwpwm);
> -
> -               regmap_write(pca->regmap, reg, LED_FULL);
> -
> +       if (!state->enabled || state->duty_cycle < 1) {
> +               pca9685_pwm_full_off(chip, pwm);
>                 return 0;
>         }
>
> -       if (duty_ns == period_ns) {
> -               /* Clear both OFF registers */
> -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -                       reg = PCA9685_ALL_LED_OFF_L;
> -               else
> -                       reg = LED_N_OFF_L(pwm->hwpwm);
> -
> -               regmap_write(pca->regmap, reg, 0x0);
> -
> -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -                       reg = PCA9685_ALL_LED_OFF_H;
> -               else
> -                       reg = LED_N_OFF_H(pwm->hwpwm);
> -
> -               regmap_write(pca->regmap, reg, 0x0);
> -
> -               /* Set the full ON bit */
> -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -                       reg = PCA9685_ALL_LED_ON_H;
> -               else
> -                       reg = LED_N_ON_H(pwm->hwpwm);
> -
> -               regmap_write(pca->regmap, reg, LED_FULL);
> -
> +       if (state->duty_cycle == state->period) {
> +               pca9685_pwm_full_on(chip, pwm);
>                 return 0;
>         }
>
> -       duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> -       duty = DIV_ROUND_UP_ULL(duty, period_ns);
> +       duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> +       duty = DIV_ROUND_UP_ULL(duty, state->period);
>
>         if (pwm->hwpwm >= PCA9685_MAXCHAN)
>                 reg = PCA9685_ALL_LED_OFF_L;
> @@ -349,64 +420,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         return 0;
>  }
>
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -       struct pca9685 *pca = to_pca(chip);
> -       unsigned int reg;
> -
> -       /*
> -        * The PWM subsystem does not support a pre-delay.
> -        * So, set the ON-timeout to 0
> -        */
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_ON_L;
> -       else
> -               reg = LED_N_ON_L(pwm->hwpwm);
> -
> -       regmap_write(pca->regmap, reg, 0);
> -
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_ON_H;
> -       else
> -               reg = LED_N_ON_H(pwm->hwpwm);
> -
> -       regmap_write(pca->regmap, reg, 0);
> -
> -       /*
> -        * Clear the full-off bit.
> -        * It has precedence over the others and must be off.
> -        */
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_H;
> -       else
> -               reg = LED_N_OFF_H(pwm->hwpwm);
> -
> -       regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> -
> -       return 0;
> -}
> -
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -       struct pca9685 *pca = to_pca(chip);
> -       unsigned int reg;
> -
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_H;
> -       else
> -               reg = LED_N_OFF_H(pwm->hwpwm);
> -
> -       regmap_write(pca->regmap, reg, LED_FULL);
> -
> -       /* Clear the LED_OFF counter. */
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_L;
> -       else
> -               reg = LED_N_OFF_L(pwm->hwpwm);
> -
> -       regmap_write(pca->regmap, reg, 0x0);
> -}
> -
>  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct pca9685 *pca = to_pca(chip);
> @@ -422,15 +435,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct pca9685 *pca = to_pca(chip);
>
> -       pca9685_pwm_disable(chip, pwm);
> +       pca9685_pwm_full_off(chip, pwm);
>         pm_runtime_put(chip->dev);
>         pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
>  }
>
>  static const struct pwm_ops pca9685_pwm_ops = {
> -       .enable = pca9685_pwm_enable,
> -       .disable = pca9685_pwm_disable,
> -       .config = pca9685_pwm_config,
> +       .get_state = pca9685_pwm_get_state,
> +       .apply = pca9685_pwm_apply,
>         .request = pca9685_pwm_request,
>         .free = pca9685_pwm_free,
>         .owner = THIS_MODULE,
> @@ -448,7 +460,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  {
>         struct pca9685 *pca;
>         unsigned int reg;
> -       int ret;
> +       int prescale = 0, ret;
>
>         pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
>         if (!pca)
> @@ -461,10 +473,13 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>                         ret);
>                 return ret;
>         }
> -       pca->period_ns = PCA9685_DEFAULT_PERIOD;
>
>         i2c_set_clientdata(client, pca);
>
> +       regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> +       if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
> +               pca->prescale = prescale;

I'm not sure this will cache the prescale value correctly,
the logic seems inverted.

You appear to mix cached and uncached uses of prescale,
is there a need for this? If not, perhaps pick one and use
it consistently?

Perhaps you can define a is_prescale_valid() helper,
which is easier to read.
And it can be easily negated:
    if (!is_prescale_valid(prescale))
without getting confused between </>. <=/>=, and ||/&&.

Also, if the prescale register contains an invalid value
during probe(), e.g. 0x00 or 0x01, would it make sense
to explicitly overwrite it with a valid setting?

> +
>         regmap_read(pca->regmap, PCA9685_MODE2, &reg);
>
>         if (device_property_read_bool(&client->dev, "invert"))
> --
> 2.29.2
>

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Clemens Gruber]

Hi Sven,

thank you for your fast response and helpful feedback! I'll answer below.

On Wed, Nov 18, 2020 at 07:18:04PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, thank you so much for this contribution.
> I no longer have access to this chip, so I cannot test
> the changes.
> 
> Some friendly/constructive feedback below.
> 
> On Wed, Nov 18, 2020 at 12:44 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > This 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)
> > - The prescale register is now read out. If one sets a period resulting
> >   in the same prescale register value, the sleep and write to the
> >   register is skipped
> >
> > The hardware readout may return slightly different values than those
> > that were set in apply due to the limited range of possible prescale and
> > counter register values. If one channel is reconfigured with new duty
> > cycle and period, the others will keep the same relative duty cycle to
> > period ratio as they had before, even though the per-chip / global
> > frequency changed. (The PCA9685 has only one prescaler!)
> >
> > Note that although the datasheet mentions 200 Hz as default frequency
> > when using the internal 25 MHz oscillator, the calculated period from
> > the default prescaler register setting of 30 is 5079040ns.
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> >  drivers/pwm/pwm-pca9685.c | 233 ++++++++++++++++++++------------------
> >  1 file changed, 124 insertions(+), 109 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..20f1314e6754 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
> > @@ -74,7 +73,7 @@
> >  struct pca9685 {
> >         struct pwm_chip chip;
> >         struct regmap *regmap;
> > -       int period_ns;
> > +       int prescale;
> 
> You've decided to cache the prescale register...
> 
> >  #if IS_ENABLED(CONFIG_GPIOLIB)
> >         struct mutex lock;
> >         struct gpio_chip gpio;
> > @@ -246,18 +245,117 @@ 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 void pca9685_pwm_full_off(struct pwm_chip *chip,
> > +                                struct pwm_device *pwm)
> > +{
> > +       struct pca9685 *pca = to_pca(chip);
> > +       int reg;
> > +
> > +       /*
> > +        * Set the full OFF bit to cause the PWM channel to be always off.
> > +        * The full OFF bit has precedence over the other register values.
> > +        */
> > +
> > +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > +               reg = PCA9685_ALL_LED_OFF_H;
> > +       else
> > +               reg = LED_N_OFF_H(pwm->hwpwm);
> > +
> > +       regmap_write(pca->regmap, reg, LED_FULL);
> > +}
> > +
> > +static void pca9685_pwm_full_on(struct pwm_chip *chip,
> > +                               struct pwm_device *pwm)
> > +{
> > +       struct pca9685 *pca = to_pca(chip);
> > +       int reg;
> > +
> > +       /*
> > +        * Clear the OFF registers (including the full OFF bit) and set
> > +        * the full ON bit to cause the PWM channel to be always on.
> > +        */
> > +
> > +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > +               reg = PCA9685_ALL_LED_OFF_L;
> > +       else
> > +               reg = LED_N_OFF_L(pwm->hwpwm);
> > +
> > +       regmap_write(pca->regmap, reg, 0x0);
> > +
> > +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > +               reg = PCA9685_ALL_LED_OFF_H;
> > +       else
> > +               reg = LED_N_OFF_H(pwm->hwpwm);
> > +
> > +       regmap_write(pca->regmap, reg, 0x0);
> > +
> > +       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > +               reg = PCA9685_ALL_LED_ON_H;
> > +       else
> > +               reg = LED_N_ON_H(pwm->hwpwm);
> > +
> > +       regmap_write(pca->regmap, reg, LED_FULL);
> > +}
> > +
> > +static int pca9685_pwm_read_global_period(struct pca9685 *pca)
> 
> but in this function, you don't seem to use the cached prescale
> value, but read it out again instead?

Good point.
I wanted .get_state to read out the actual HW state but the cached
prescale register should always be identical to that, so it should be OK
to use it here.

> > +{
> > +       unsigned int prescale = 0;
> > +
> > +       regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> > +
> > +       if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
> > +               return 0;
> > +
> > +       return (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > +               (prescale + 1);
> > +}
> > +
> > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > +                                 struct pwm_state *state)
> > +{
> > +       struct pca9685 *pca = to_pca(chip);
> > +       unsigned int val, duty;
> > +       int reg;
> > +
> > +       /* Read out (chip-wide) period */
> > +       state->period = pca9685_pwm_read_global_period(pca);
> > +
> > +       /* The (per-channel) polarity is fixed */
> > +       state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +       /* Read out current duty cycle and enabled state */
> > +       reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
> > +               LED_N_OFF_H(pwm->hwpwm);
> > +       regmap_read(pca->regmap, reg, &val);
> > +       duty = (val & 0xf) << 8;
> > +
> > +       state->enabled = !(val & LED_FULL);
> > +
> > +       reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
> > +               LED_N_OFF_L(pwm->hwpwm);
> > +       regmap_read(pca->regmap, reg, &val);
> > +       duty |= (val & 0xff);
> > +
> > +       if (duty < PCA9685_COUNTER_RANGE) {
> > +               duty *= state->period;
> > +               state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> > +       } else
> > +               state->duty_cycle = 0;
> > +}
> > +
> > +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;
> > +       unsigned long long duty, prescale;
> >         unsigned int reg;
> > -       int prescale;
> >
> > -       if (period_ns != pca->period_ns) {
> > -               prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> > -                                            PCA9685_COUNTER_RANGE * 1000) - 1;
> > +       if (state->polarity != PWM_POLARITY_NORMAL)
> > +               return -EOPNOTSUPP;
> >
> > +       prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > +                                        PCA9685_COUNTER_RANGE * 1000) - 1;
> > +       if (prescale != pca->prescale) {
> 
> Use of cached prescale here, all good.
> 
> >                 if (prescale >= PCA9685_PRESCALE_MIN &&
> >                         prescale <= PCA9685_PRESCALE_MAX) {
> >                         /*
> > @@ -270,12 +368,13 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >                         pca9685_set_sleep_mode(pca, true);
> >
> >                         /* Change the chip-wide output frequency */
> > -                       regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > +                       regmap_write(pca->regmap, PCA9685_PRESCALE,
> > +                                    (int)prescale);
> >
> >                         /* Wake the chip up */
> >                         pca9685_set_sleep_mode(pca, false);
> >
> > -                       pca->period_ns = period_ns;
> > +                       pca->prescale = (int)prescale;
> >                 } else {
> >                         dev_err(chip->dev,
> >                                 "prescaler not set: period out of bounds!\n");
> > @@ -283,46 +382,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >                 }
> >         }
> >
> > -       if (duty_ns < 1) {
> > -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -                       reg = PCA9685_ALL_LED_OFF_H;
> > -               else
> > -                       reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > -               regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > +       if (!state->enabled || state->duty_cycle < 1) {
> > +               pca9685_pwm_full_off(chip, pwm);
> >                 return 0;
> >         }
> >
> > -       if (duty_ns == period_ns) {
> > -               /* Clear both OFF registers */
> > -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -                       reg = PCA9685_ALL_LED_OFF_L;
> > -               else
> > -                       reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > -               regmap_write(pca->regmap, reg, 0x0);
> > -
> > -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -                       reg = PCA9685_ALL_LED_OFF_H;
> > -               else
> > -                       reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > -               regmap_write(pca->regmap, reg, 0x0);
> > -
> > -               /* Set the full ON bit */
> > -               if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -                       reg = PCA9685_ALL_LED_ON_H;
> > -               else
> > -                       reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > -               regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > +       if (state->duty_cycle == state->period) {
> > +               pca9685_pwm_full_on(chip, pwm);
> >                 return 0;
> >         }
> >
> > -       duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> > -       duty = DIV_ROUND_UP_ULL(duty, period_ns);
> > +       duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> > +       duty = DIV_ROUND_UP_ULL(duty, state->period);
> >
> >         if (pwm->hwpwm >= PCA9685_MAXCHAN)
> >                 reg = PCA9685_ALL_LED_OFF_L;
> > @@ -349,64 +420,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >         return 0;
> >  }
> >
> > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > -       struct pca9685 *pca = to_pca(chip);
> > -       unsigned int reg;
> > -
> > -       /*
> > -        * The PWM subsystem does not support a pre-delay.
> > -        * So, set the ON-timeout to 0
> > -        */
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_ON_L;
> > -       else
> > -               reg = LED_N_ON_L(pwm->hwpwm);
> > -
> > -       regmap_write(pca->regmap, reg, 0);
> > -
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_ON_H;
> > -       else
> > -               reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > -       regmap_write(pca->regmap, reg, 0);
> > -
> > -       /*
> > -        * Clear the full-off bit.
> > -        * It has precedence over the others and must be off.
> > -        */
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_H;
> > -       else
> > -               reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > -       regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > -
> > -       return 0;
> > -}
> > -
> > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > -       struct pca9685 *pca = to_pca(chip);
> > -       unsigned int reg;
> > -
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_H;
> > -       else
> > -               reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > -       regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > -       /* Clear the LED_OFF counter. */
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_L;
> > -       else
> > -               reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > -       regmap_write(pca->regmap, reg, 0x0);
> > -}
> > -
> >  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >         struct pca9685 *pca = to_pca(chip);
> > @@ -422,15 +435,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >         struct pca9685 *pca = to_pca(chip);
> >
> > -       pca9685_pwm_disable(chip, pwm);
> > +       pca9685_pwm_full_off(chip, pwm);
> >         pm_runtime_put(chip->dev);
> >         pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> >  }
> >
> >  static const struct pwm_ops pca9685_pwm_ops = {
> > -       .enable = pca9685_pwm_enable,
> > -       .disable = pca9685_pwm_disable,
> > -       .config = pca9685_pwm_config,
> > +       .get_state = pca9685_pwm_get_state,
> > +       .apply = pca9685_pwm_apply,
> >         .request = pca9685_pwm_request,
> >         .free = pca9685_pwm_free,
> >         .owner = THIS_MODULE,
> > @@ -448,7 +460,7 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  {
> >         struct pca9685 *pca;
> >         unsigned int reg;
> > -       int ret;
> > +       int prescale = 0, ret;
> >
> >         pca = devm_kzalloc(&client->dev, sizeof(*pca), GFP_KERNEL);
> >         if (!pca)
> > @@ -461,10 +473,13 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >                         ret);
> >                 return ret;
> >         }
> > -       pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >
> >         i2c_set_clientdata(client, pca);
> >
> > +       regmap_read(pca->regmap, PCA9685_PRESCALE, &prescale);
> > +       if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX)
> > +               pca->prescale = prescale;
> 
> I'm not sure this will cache the prescale value correctly,
> the logic seems inverted

You are right, that's a mistake.

> You appear to mix cached and uncached uses of prescale,
> is there a need for this? If not, perhaps pick one and use
> it consistently?

Yes, sticking to the cached value is probably the way to go.

> Perhaps you can define a is_prescale_valid() helper,
> which is easier to read.
> And it can be easily negated:
>     if (!is_prescale_valid(prescale))
> without getting confused between </>. <=/>=, and ||/&&.

Good idea!

> Also, if the prescale register contains an invalid value
> during probe(), e.g. 0x00 or 0x01, would it make sense
> to explicitly overwrite it with a valid setting?

As long as it is overwritten with a correct setting when the PWM is used
for the first time, it should be OK?

Thanks,
Clemens

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Sven Van Asbroeck]

On Thu, Nov 19, 2020 at 5:00 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> > You appear to mix cached and uncached uses of prescale,
> > is there a need for this? If not, perhaps pick one and use
> > it consistently?
>
> Yes, sticking to the cached value is probably the way to go.
>

I would suggest going one step further, and turn on the cache in
regmap, i.e. .cache_type = REGCACHE_RBTREE, then:
- no need to cache pca->prescale explicitly, you can just read it with
  regmap_read() every time, and it won't result in bus activity.
  then you can eliminate pca->prescale, which simplifies the driver.
- pca9685_pwm_get_state() no longer results in bus reads, every regmap_read()
  is cached, this is extremely efficient.
- pca9685_pwm_apply() and pca9685_pwm_gpio_set() now only does bus writes if
  registers actually change, i.e. calling pwm_apply() multiple times in a row
  with the same parameters, writes the registers only once.

We can do this safely because this chip never actively writes to its
registers (as far as I know).

But maybe that's a suggestion for a follow-up patch...

> > Also, if the prescale register contains an invalid value
> > during probe(), e.g. 0x00 or 0x01, would it make sense
> > to explicitly overwrite it with a valid setting?
>
> As long as it is overwritten with a correct setting when the PWM is used
> for the first time, it should be OK?

I'm not sure. Consider the following scenario:
- prescale register is invalid at probe, say it contains 0x02
- user calls pwm_apply() but with an invalid period, which results
  in a calculated prescale value of 0x02
- pca9685_pwm_apply() skips prescale setup because prescale does not
  change, returns OK(0)
- user believes setup was ok, actually it's broken...

Also, some people use this chip exclusively as a gpiochip, in that
case the prescale register is never touched. So an invalid prescale
at probe is never corrected.

Speaking of the gpiochip side, would it make sense to call
pca9685_pwm_full_on()/_off() in pca9685_pwm_gpio_set() too?

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Clemens Gruber]

On Thu, Nov 19, 2020 at 09:58:26AM -0500, Sven Van Asbroeck wrote:
> On Thu, Nov 19, 2020 at 5:00 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > > You appear to mix cached and uncached uses of prescale,
> > > is there a need for this? If not, perhaps pick one and use
> > > it consistently?
> >
> > Yes, sticking to the cached value is probably the way to go.
> >
> 
> I would suggest going one step further, and turn on the cache in
> regmap, i.e. .cache_type = REGCACHE_RBTREE, then:
> - no need to cache pca->prescale explicitly, you can just read it with
>   regmap_read() every time, and it won't result in bus activity.
>   then you can eliminate pca->prescale, which simplifies the driver.
> - pca9685_pwm_get_state() no longer results in bus reads, every regmap_read()
>   is cached, this is extremely efficient.
> - pca9685_pwm_apply() and pca9685_pwm_gpio_set() now only does bus writes if
>   registers actually change, i.e. calling pwm_apply() multiple times in a row
>   with the same parameters, writes the registers only once.

Interesting, I will look into that.

> 
> We can do this safely because this chip never actively writes to its
> registers (as far as I know).

I think so too.

> 
> But maybe that's a suggestion for a follow-up patch...
> 
> > > Also, if the prescale register contains an invalid value
> > > during probe(), e.g. 0x00 or 0x01, would it make sense
> > > to explicitly overwrite it with a valid setting?
> >
> > As long as it is overwritten with a correct setting when the PWM is used
> > for the first time, it should be OK?
> 
> I'm not sure. Consider the following scenario:
> - prescale register is invalid at probe, say it contains 0x02
> - user calls pwm_apply() but with an invalid period, which results
>   in a calculated prescale value of 0x02
> - pca9685_pwm_apply() skips prescale setup because prescale does not
>   change, returns OK(0)
> - user believes setup was ok, actually it's broken...

Makes sense. I will write the default prescale setting in case we read
an invalid one from the register.

> 
> Also, some people use this chip exclusively as a gpiochip, in that
> case the prescale register is never touched. So an invalid prescale
> at probe is never corrected.
> 
> Speaking of the gpiochip side, would it make sense to call
> pca9685_pwm_full_on()/_off() in pca9685_pwm_gpio_set() too?

Yes, I think so. Would be cleaner and we avoid setting all registers to
0 when the GPIO is disabled.

--

One thing I noticed: The driver currently assumes that it comes out of
POR in "active" state (comment at end of probe and PM calls).
However, the SLEEP bit is set by default / after POR.

Do you agree with the following solution?
1) In .probe: call pm_runtime_set_suspended() instead of _set_active()
   (If CONFIG_PM is enabled, the SLEEP bit will be cleared in .resume)
2) If !CONFIG_PM: Clear the SLEEP bit in .probe

Thanks,
Clemens

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Sven Van Asbroeck]

On Thu, Nov 19, 2020 at 11:00 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> One thing I noticed: The driver currently assumes that it comes out of
> POR in "active" state (comment at end of probe and PM calls).
> However, the SLEEP bit is set by default / after POR.

Very good point, the comment is probably not correct.

It would be wrong to assume that the chip is in any particular
state when probe() runs. There is no reset pin, so the CPU running
Linux could have reset while manipulating the chip, there could even
be leftover state from a bootloader talking to the chip, etc...

I remember running into this myself at the time.

However, we want to make sure that the runtime pm puts the chip to sleep,
no matter its initial state. So the current code is correct, but the
comment can be changed to:

/*
 * Chip activity state unknown. Tell the runtime pm that the chip is
 * active, so pm_runtime_enable() will force it into suspend.
 * Which is what we want as all outputs are disabled at this point.
 */

> 2) If !CONFIG_PM: Clear the SLEEP bit in .probe

Is anyone likely to use this driver without CONFIG_PM? My kernel won't even
build without it...

If you personally have no use for it, then I would not bother with the
!CONFIG_PM case. Just formalize in Kconfig that the driver needs PM.

I think we can add "depends on PM" or "select PM", I am not sure which one
to use here.

Sven

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Clemens Gruber]

On Thu, Nov 19, 2020 at 12:29:26PM -0500, Sven Van Asbroeck wrote:
> On Thu, Nov 19, 2020 at 11:00 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > One thing I noticed: The driver currently assumes that it comes out of
> > POR in "active" state (comment at end of probe and PM calls).
> > However, the SLEEP bit is set by default / after POR.
> 
> Very good point, the comment is probably not correct.
> 
> It would be wrong to assume that the chip is in any particular
> state when probe() runs. There is no reset pin, so the CPU running
> Linux could have reset while manipulating the chip, there could even
> be leftover state from a bootloader talking to the chip, etc...
> 
> I remember running into this myself at the time.
> 
> However, we want to make sure that the runtime pm puts the chip to sleep,
> no matter its initial state. So the current code is correct, but the
> comment can be changed to:
> 
> /*
>  * Chip activity state unknown. Tell the runtime pm that the chip is
>  * active, so pm_runtime_enable() will force it into suspend.
>  * Which is what we want as all outputs are disabled at this point.
>  */

I think it is better if we set the correct runtime pm state in .probe,
depending on the SLEEP bit being set. Then, if the chip is already in
SLEEP state, there is no need for the suspend function to be called.

> > 2) If !CONFIG_PM: Clear the SLEEP bit in .probe
> 
> Is anyone likely to use this driver without CONFIG_PM? My kernel won't even
> build without it...
> 
> If you personally have no use for it, then I would not bother with the
> !CONFIG_PM case. Just formalize in Kconfig that the driver needs PM.
> 
> I think we can add "depends on PM" or "select PM", I am not sure which one
> to use here.

I'd rather continue supporting this driver with !CONFIG_PM. (In our
company we have a product with a !CONFIG_PM build using this driver)

I am thinking about the following solution:
#ifdef CONFIG_PM
  /* Set runtime PM status according to chip sleep state */
  if (reg & MODE1_SLEEP)
    pm_runtime_set_suspended(..);
  else
    pm_runtime_set_active(..);

  pm_runtime_enable(..);
#else
  /* If in SLEEP state on non-PM environments, wake the chip up */
  if (reg & MODE1_SLEEP)
    pca9685_set_sleep_mode(.., false)
#endif

--

About the regmap cache: I looked into it and think it is a good idea but
it's probably best to get these patches merged first and then rework the
driver to using the regmap cache?

Thanks for your help!

Clemens

Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API

[Sven Van Asbroeck]

On Sat, Nov 21, 2020 at 9:58 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> I'd rather continue supporting this driver with !CONFIG_PM. (In our
> company we have a product with a !CONFIG_PM build using this driver)

Absolutely, makes sense. If you do add support for !CONFIG_PM, then it's
important that both PM and !PM cases get tested by you.

>
> I am thinking about the following solution:
> #ifdef CONFIG_PM
>   /* Set runtime PM status according to chip sleep state */
>   if (reg & MODE1_SLEEP)
>     pm_runtime_set_suspended(..);
>   else
>     pm_runtime_set_active(..);
>
>   pm_runtime_enable(..);
> #else
>   /* If in SLEEP state on non-PM environments, wake the chip up */
>   if (reg & MODE1_SLEEP)
>     pca9685_set_sleep_mode(.., false)
> #endif

I don't think we need the #ifdef CONFIG_PM, because all pm_runtime_xxx
functions become no-ops when !CONFIG_PM.

Also, I believe "if (IS_ENABLED(CONFIG_XXX))" is preferred, because
it allows the compiler to syntax-check disabled code.

How about the following? It should be correct, short, and easy to understand.
Yes, there's one single unnecessary register write (+ 500us delay if !PM) when
the chip is already active on probe(). But maybe that's worth it if it makes
the code easier to understand?

probe()
{
    ...
    pm_runtime_set_active(&client->dev);
    pm_runtime_enable(&client->dev);

    if (!IS_ENABLED(CONFIG_PM))
        pca9685_set_sleep_mode(pca, false);

    return 0;
}

remove()
{
    ...
    pm_runtime_disable(&client->dev);

    if (!IS_ENABLED(CONFIG_PM))
        pca9685_set_sleep_mode(pca, true);

    return 0;
}

>
> About the regmap cache: I looked into it and think it is a good idea but
> it's probably best to get these patches merged first and then rework the
> driver to using the regmap cache?

Good suggestion, I agree.