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

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

[Clemens Gruber]

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

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
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 | 261 ++++++++++++++------------------------
 1 file changed, 92 insertions(+), 169 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..5a2ce97e71fd 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)))
+
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
-	int period_ns;
 #if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
 	struct gpio_chip gpio;
@@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
 	return container_of(chip, struct pca9685, chip);
 }
 
+/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
+static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
+{
+	if (duty == 0) {
+		/* Set the full OFF bit, which has the highest precedence */
+		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+	} else if (duty >= PCA9685_COUNTER_RANGE) {
+		/* Set the full ON bit and clear the full OFF bit */
+		regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
+		regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+	} else {
+		/* Set OFF time (clears the full OFF bit) */
+		regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
+		regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
+		/* Clear the full ON bit */
+		regmap_write(pca->regmap, REG_ON_H(channel), 0);
+	}
+}
+
+static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
+{
+	unsigned int off_h = 0, val = 0;
+
+	if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
+		/* HW does not support reading state of "all LEDs" channel */
+		return 0;
+	}
+
+	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
+	if (off_h & LED_FULL) {
+		/* Full OFF bit is set */
+		return 0;
+	}
+
+	regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
+	if (val & LED_FULL) {
+		/* Full ON bit is set */
+		return PCA9685_COUNTER_RANGE;
+	}
+
+	val = 0;
+	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+	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,56 @@ 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;
-	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 (prescale >= PCA9685_PRESCALE_MIN &&
-			prescale <= PCA9685_PRESCALE_MAX) {
-			/*
-			 * 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);
-
-			/* Wake the chip up */
-			pca9685_set_sleep_mode(pca, false);
-
-			pca->period_ns = period_ns;
-		} else {
-			dev_err(chip->dev,
-				"prescaler not set: period out of bounds!\n");
-			return -EINVAL;
-		}
-	}
+	unsigned long long duty, prescale;
+	unsigned int val = 0;
 
-	if (duty_ns < 1) {
-		if (pwm->hwpwm >= PCA9685_MAXCHAN)
-			reg = PCA9685_ALL_LED_OFF_H;
-		else
-			reg = LED_N_OFF_H(pwm->hwpwm);
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EINVAL;
 
-		regmap_write(pca->regmap, reg, LED_FULL);
-
-		return 0;
+	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 (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);
+	duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
+	duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
 
+	if (duty < 1 || !state->enabled) {
+		pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+		return 0;
+	} else if (duty == PCA9685_COUNTER_RANGE) {
+		pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
 		return 0;
 	}
 
-	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
-	duty = DIV_ROUND_UP_ULL(duty, period_ns);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_L;
-	else
-		reg = LED_N_OFF_L(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, (int)duty & 0xff);
-
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
-
-	/* Clear the full ON bit, otherwise the set OFF time has no effect */
-	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);
-
-	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);
+	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);
 
-	/*
-	 * 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);
+		/* Change the chip-wide output frequency */
+		regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
 
-	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+		/* Wake the chip up */
+		pca9685_set_sleep_mode(pca, false);
+	}
 
+	pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
 	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 +348,13 @@ 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_set_duty(pca, pwm->hwpwm, 0);
 	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,
+	.apply = pca9685_pwm_apply,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
@@ -461,7 +385,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
-	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
@@ -484,9 +407,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 */
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+	/* Reset OFF registers to POR default */
+	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
+	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.31.1

[PATCH v7 2/8] pwm: pca9685: Support hardware readout

[Clemens Gruber]

Implements .get_state to read-out the current hardware state.

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.

Also 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>
---
Changes since v6:
- Added a comment regarding the division (Suggested by Uwe)
- Rebased

 drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 5a2ce97e71fd..d4474c5ff96f 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+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 long long duty;
+	unsigned int val = 0;
+
+	/* Calculate (chip-wide) period from prescale value */
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+	/*
+	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
+	 * The following calculation is therefore only a multiplication
+	 * and we are not losing precision.
+	 */
+	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+			(val + 1);
+
+	/* The (per-channel) polarity is fixed */
+	state->polarity = PWM_POLARITY_NORMAL;
+
+	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
+		/*
+		 * The "all LEDs" channel does not support HW readout
+		 * Return 0 and disabled for backwards compatibility
+		 */
+		state->duty_cycle = 0;
+		state->enabled = false;
+		return;
+	}
+
+	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
+
+	state->enabled = !!duty;
+	if (!state->enabled) {
+		state->duty_cycle = 0;
+		return;
+	} else if (duty == PCA9685_COUNTER_RANGE) {
+		state->duty_cycle = state->period;
+		return;
+	}
+
+	duty *= state->period;
+	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
+}
+
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
@@ -355,6 +400,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 
 static const struct pwm_ops pca9685_pwm_ops = {
 	.apply = pca9685_pwm_apply,
+	.get_state = pca9685_pwm_get_state,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
-- 
2.31.1

[PATCH v7 3/8] pwm: pca9685: Improve runtime PM behavior

[Clemens Gruber]

The chip does not come out of POR in active state but in sleep state.
To be sure (in case the bootloader woke it up) we force it to sleep in
probe.

On kernels without CONFIG_PM, we wake the chip in .probe and put it to
sleep in .remove.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v6:
- Improved !CONFIG_PM handling (wake it up without putting it to sleep
  first)

 drivers/pwm/pwm-pca9685.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index d4474c5ff96f..0bcec04b138a 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -474,13 +474,18 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	/* The chip comes out of power-up in the active state */
-	pm_runtime_set_active(&client->dev);
-	/*
-	 * Enable will put the chip into suspend, which is what we
-	 * want as all outputs are disabled at this point
-	 */
-	pm_runtime_enable(&client->dev);
+	if (IS_ENABLED(CONFIG_PM)) {
+		/*
+		 * The chip comes out of power-up in the sleep state,
+		 * but force it to sleep in case it was woken up before
+		 */
+		pca9685_set_sleep_mode(pca, true);
+		pm_runtime_set_suspended(&client->dev);
+		pm_runtime_enable(&client->dev);
+	} else {
+		/* Wake the chip up on non-PM environments */
+		pca9685_set_sleep_mode(pca, false);
+	}
 
 	return 0;
 }
@@ -493,7 +498,14 @@ static int pca9685_pwm_remove(struct i2c_client *client)
 	ret = pwmchip_remove(&pca->chip);
 	if (ret)
 		return ret;
+
 	pm_runtime_disable(&client->dev);
+
+	if (!IS_ENABLED(CONFIG_PM)) {
+		/* Put chip in sleep state on non-PM environments */
+		pca9685_set_sleep_mode(pca, true);
+	}
+
 	return 0;
 }
 
-- 
2.31.1

[PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

Add the flag and corresponding documentation for the new PWM staggering
mode feature.

Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 Documentation/devicetree/bindings/pwm/pwm.txt | 1 +
 include/dt-bindings/pwm/pwm.h                 | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 084886bd721e..11d539302398 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -46,6 +46,7 @@ period in nanoseconds.
 Optionally, the pwm-specifier can encode a number of flags (defined in
 <dt-bindings/pwm/pwm.h>) in a third cell:
 - PWM_POLARITY_INVERTED: invert the PWM signal polarity
+- PWM_STAGGERING_ALLOWED: allow delayed ON-time for improved EMI
 
 Example with optional PWM specifier for inverse polarity
 
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index ab9a077e3c7d..51d67dec1aad 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -11,5 +11,6 @@
 #define _DT_BINDINGS_PWM_PWM_H
 
 #define PWM_POLARITY_INVERTED			(1 << 0)
+#define PWM_STAGGERING_ALLOWED			(1 << 1)
 
 #endif
-- 
2.31.1

[PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
may (if supported by the HW) delay the ON time of the channel relative
to the channel number.
This does not alter the duty cycle ratio and is only relevant for PWM
chips with less prescalers than channels, which would otherwise assert
multiple or even all enabled channels at the same time.

If this feature is supported by the driver and the flag is set on
multiple channels, their ON times are spread out to improve EMI and
reduce current spikes.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
 drivers/pwm/core.c  | 9 +++++++--
 include/linux/pwm.h | 1 +
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a8eff4b3ee36..f58aad754741 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -153,9 +153,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	pwm->args.period = args->args[1];
 	pwm->args.polarity = PWM_POLARITY_NORMAL;
+	pwm->args.staggering_allowed = false;
 
-	if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
-		pwm->args.polarity = PWM_POLARITY_INVERSED;
+	if (args->args_count > 2) {
+		if (args->args[2] & PWM_POLARITY_INVERTED)
+			pwm->args.polarity = PWM_POLARITY_INVERSED;
+		if (args->args[2] & PWM_STAGGERING_ALLOWED)
+			pwm->args.staggering_allowed = true;
+	}
 
 	return pwm;
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e4d84d4db293..3d5dee8c564f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -41,6 +41,7 @@ enum pwm_polarity {
 struct pwm_args {
 	u64 period;
 	enum pwm_polarity polarity;
+	bool staggering_allowed;
 };
 
 enum {
-- 
2.31.1

[PATCH v7 6/8] pwm: pca9685: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

The PCA9685 supports staggered LED output ON times to minimize current
surges and reduce EMI.
When the PWM_STAGGERING_ALLOWED flag is set for a channel, its ON time
is delayed by channel number x counter range / 16, which avoids
asserting all the 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>
---
Changes since v6:
- Use a per-PWM flag instead of one global DT parameter

 drivers/pwm/pwm-pca9685.c | 60 ++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 14 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 0bcec04b138a..24221ee7a77a 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -93,46 +93,73 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
 /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
 static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
 {
+	struct pwm_device *pwm = &pca->chip.pwms[channel];
+	unsigned int on, off;
+
 	if (duty == 0) {
 		/* Set the full OFF bit, which has the highest precedence */
 		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+		return;
 	} else if (duty >= PCA9685_COUNTER_RANGE) {
 		/* Set the full ON bit and clear the full OFF bit */
 		regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
 		regmap_write(pca->regmap, REG_OFF_H(channel), 0);
-	} else {
-		/* Set OFF time (clears the full OFF bit) */
-		regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
-		regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
-		/* Clear the full ON bit */
-		regmap_write(pca->regmap, REG_ON_H(channel), 0);
+		return;
 	}
+
+
+	if (pwm->args.staggering_allowed && channel < 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 = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
+	} else
+		on = 0;
+
+	off = (on + duty) % PCA9685_COUNTER_RANGE;
+
+	/* Set ON time (clears full ON bit) */
+	regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
+	regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+	/* Set OFF time (clears full OFF bit) */
+	regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
+	regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
 }
 
 static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
 {
-	unsigned int off_h = 0, val = 0;
+	struct pwm_device *pwm = &pca->chip.pwms[channel];
+	unsigned int off = 0, on = 0, val = 0;
 
 	if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
 		/* HW does not support reading state of "all LEDs" channel */
 		return 0;
 	}
 
-	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
-	if (off_h & LED_FULL) {
+	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+	if (off & LED_FULL) {
 		/* Full OFF bit is set */
 		return 0;
 	}
 
-	regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
-	if (val & LED_FULL) {
+	regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+	if (on & LED_FULL) {
 		/* Full ON bit is set */
 		return PCA9685_COUNTER_RANGE;
 	}
 
-	val = 0;
 	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
-	return ((off_h & 0xf) << 8) | (val & 0xff);
+	off = ((off & 0xf) << 8) | (val & 0xff);
+	if (!pwm->args.staggering_allowed)
+		return off;
+
+	/* Read ON register to calculate duty cycle of staggered output */
+	val = 0;
+	regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
+	on = ((on & 0xf) << 8) | (val & 0xff);
+	return (off - on) & (PCA9685_COUNTER_RANGE - 1);
 }
 
 #if IS_ENABLED(CONFIG_GPIOLIB)
@@ -453,9 +480,11 @@ 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);
 
-	/* Reset OFF registers to POR default */
+	/* Reset OFF/ON registers to POR default */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
+	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 */
@@ -464,6 +493,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	pca->chip.dev = &client->dev;
 	pca->chip.base = -1;
 
+	pca->chip.of_xlate = of_pwm_xlate_with_flags;
+	pca->chip.of_pwm_n_cells = 3;
+
 	ret = pwmchip_add(&pca->chip);
 	if (ret < 0)
 		return ret;
-- 
2.31.1

[PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs

[Clemens Gruber]

Previously, the last used PWM channel could change the global prescale
setting, even if other channels are already in use.

Fix it by only allowing the first enabled PWM to change the global
chip-wide prescale setting. If there is more than one channel in use,
the prescale settings resulting from the chosen periods must match.

GPIOs do not count as enabled PWMs as they are not using the prescaler
and can't change it.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v6:
- Only allow the first PWM that is enabled to change the prescaler, not
  the first one that uses the prescaler

 drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 24221ee7a77a..cf0c98e4ef44 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -23,11 +23,11 @@
 #include <linux/bitmap.h>
 
 /*
- * Because the PCA9685 has only one prescaler per chip, changing the period of
- * one channel affects the period of all 16 PWM outputs!
- * However, the ratio between each configured duty cycle and the chip-wide
- * period remains constant, because the OFF time is set in proportion to the
- * counter range.
+ * Because the PCA9685 has only one prescaler per chip, only the first channel
+ * that is enabled is allowed to change the prescale register.
+ * PWM channels requested afterwards must use a period that results in the same
+ * prescale setting as the one set by the first requested channel.
+ * GPIOs do not count as enabled PWMs as they are not using the prescaler.
  */
 
 #define PCA9685_MODE1		0x00
@@ -78,8 +78,9 @@
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
-#if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
+	DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
+#if IS_ENABLED(CONFIG_GPIOLIB)
 	struct gpio_chip gpio;
 	DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
 #endif
@@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
 	return container_of(chip, struct pca9685, chip);
 }
 
+/* This function is supposed to be called with the lock mutex held */
+static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
+{
+	/* No PWM enabled: Change allowed */
+	if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
+		return true;
+	/* More than one PWM enabled: Change not allowed */
+	if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
+		return false;
+	/*
+	 * Only one PWM enabled: Change allowed if the PWM about to
+	 * be changed is the one that is already enabled
+	 */
+	return test_bit(channel, pca->pwms_enabled);
+}
+
 /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
 static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
 {
@@ -265,8 +282,6 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca)
 {
 	struct device *dev = pca->chip.dev;
 
-	mutex_init(&pca->lock);
-
 	pca->gpio.label = dev_name(dev);
 	pca->gpio.parent = dev;
 	pca->gpio.request = pca9685_pwm_gpio_request;
@@ -310,8 +325,8 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
 	}
 }
 
-static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			     const struct pwm_state *state)
+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, prescale;
@@ -340,6 +355,12 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
 	if (prescale != val) {
+		if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) {
+			dev_err(chip->dev,
+				"pwm not changed: periods of enabled pwms must match!\n");
+			return -EBUSY;
+		}
+
 		/*
 		 * Putting the chip briefly into SLEEP mode
 		 * at this point won't interfere with the
@@ -360,6 +381,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 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);
+	int ret;
+
+	mutex_lock(&pca->lock);
+	ret = __pca9685_pwm_apply(chip, pwm, state);
+	if (ret == 0) {
+		if (state->enabled)
+			set_bit(pwm->hwpwm, pca->pwms_enabled);
+		else
+			clear_bit(pwm->hwpwm, pca->pwms_enabled);
+	}
+	mutex_unlock(&pca->lock);
+
+	return ret;
+}
+
 static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				  struct pwm_state *state)
 {
@@ -420,7 +460,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
+	mutex_lock(&pca->lock);
 	pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+	clear_bit(pwm->hwpwm, pca->pwms_enabled);
+	mutex_unlock(&pca->lock);
+
 	pm_runtime_put(chip->dev);
 	pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
 }
@@ -461,6 +505,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, pca);
 
+	mutex_init(&pca->lock);
+
 	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
 
 	if (device_property_read_bool(&client->dev, "invert"))
-- 
2.31.1

[PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls

[Clemens Gruber]

Regmap operations can fail if the underlying subsystem is not working
properly (e.g. hogged I2C bus, etc.)
As this is useful information for the user, print an error message if it
happens.
Let probe fail if the first regmap_read or the first regmap_write fails.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v6:
- Rebased

 drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
 1 file changed, 59 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index cf0c98e4ef44..8a4993882b40 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
 	return test_bit(channel, pca->pwms_enabled);
 }
 
+static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
+{
+	struct device *dev = pca->chip.dev;
+	int err;
+
+	err = regmap_read(pca->regmap, reg, val);
+	if (err != 0)
+		dev_err(dev, "regmap_read of register 0x%x failed: %d\n", reg, err);
+
+	return err;
+}
+
+static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
+{
+	struct device *dev = pca->chip.dev;
+	int err;
+
+	err = regmap_write(pca->regmap, reg, val);
+	if (err != 0)
+		dev_err(dev, "regmap_write to register 0x%x failed: %d\n", reg, err);
+
+	return err;
+}
+
 /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
 static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
 {
@@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
 
 	if (duty == 0) {
 		/* Set the full OFF bit, which has the highest precedence */
-		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+		pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
 		return;
 	} else if (duty >= PCA9685_COUNTER_RANGE) {
 		/* Set the full ON bit and clear the full OFF bit */
-		regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
-		regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+		pca9685_write_reg(pca, REG_ON_H(channel), LED_FULL);
+		pca9685_write_reg(pca, REG_OFF_H(channel), 0);
 		return;
 	}
 
@@ -138,11 +162,11 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
 	off = (on + duty) % PCA9685_COUNTER_RANGE;
 
 	/* Set ON time (clears full ON bit) */
-	regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
-	regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+	pca9685_write_reg(pca, REG_ON_L(channel), on & 0xff);
+	pca9685_write_reg(pca, REG_ON_H(channel), (on >> 8) & 0xf);
 	/* Set OFF time (clears full OFF bit) */
-	regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
-	regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
+	pca9685_write_reg(pca, REG_OFF_L(channel), off & 0xff);
+	pca9685_write_reg(pca, REG_OFF_H(channel), (off >> 8) & 0xf);
 }
 
 static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
@@ -155,26 +179,26 @@ static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
 		return 0;
 	}
 
-	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+	pca9685_read_reg(pca, LED_N_OFF_H(channel), &off);
 	if (off & LED_FULL) {
 		/* Full OFF bit is set */
 		return 0;
 	}
 
-	regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+	pca9685_read_reg(pca, LED_N_ON_H(channel), &on);
 	if (on & LED_FULL) {
 		/* Full ON bit is set */
 		return PCA9685_COUNTER_RANGE;
 	}
 
-	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+	pca9685_read_reg(pca, LED_N_OFF_L(channel), &val);
 	off = ((off & 0xf) << 8) | (val & 0xff);
 	if (!pwm->args.staggering_allowed)
 		return off;
 
 	/* Read ON register to calculate duty cycle of staggered output */
 	val = 0;
-	regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
+	pca9685_read_reg(pca, LED_N_ON_L(channel), &val);
 	on = ((on & 0xf) << 8) | (val & 0xff);
 	return (off - on) & (PCA9685_COUNTER_RANGE - 1);
 }
@@ -317,8 +341,15 @@ static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca)
 
 static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
 {
-	regmap_update_bits(pca->regmap, PCA9685_MODE1,
-			   MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+	struct device *dev = pca->chip.dev;
+	int err = regmap_update_bits(pca->regmap, PCA9685_MODE1,
+				     MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+	if (err != 0) {
+		dev_err(dev, "regmap_update_bits of register 0x%x failed: %d\n",
+			PCA9685_MODE1, err);
+		return;
+	}
+
 	if (!enable) {
 		/* Wait 500us for the oscillator to be back up */
 		udelay(500);
@@ -353,7 +384,7 @@ static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
-	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+	pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
 	if (prescale != val) {
 		if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) {
 			dev_err(chip->dev,
@@ -371,7 +402,7 @@ static int __pca9685_pwm_apply(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, (int)prescale);
+		pca9685_write_reg(pca, PCA9685_PRESCALE, (int)prescale);
 
 		/* Wake the chip up */
 		pca9685_set_sleep_mode(pca, false);
@@ -408,7 +439,7 @@ static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned int val = 0;
 
 	/* Calculate (chip-wide) period from prescale value */
-	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+	pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
 	/*
 	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
 	 * The following calculation is therefore only a multiplication
@@ -507,7 +538,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	mutex_init(&pca->lock);
 
-	regmap_read(pca->regmap, PCA9685_MODE2, &reg);
+	ret = pca9685_read_reg(pca, PCA9685_MODE2, &reg);
+	if (ret != 0)
+		return ret;
 
 	if (device_property_read_bool(&client->dev, "invert"))
 		reg |= MODE2_INVRT;
@@ -519,18 +552,20 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	else
 		reg |= MODE2_OUTDRV;
 
-	regmap_write(pca->regmap, PCA9685_MODE2, reg);
+	ret = pca9685_write_reg(pca, PCA9685_MODE2, reg);
+	if (ret != 0)
+		return ret;
 
 	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
-	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
+	pca9685_read_reg(pca, PCA9685_MODE1, &reg);
 	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
-	regmap_write(pca->regmap, PCA9685_MODE1, reg);
+	pca9685_write_reg(pca, PCA9685_MODE1, reg);
 
 	/* Reset OFF/ON registers to POR default */
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+	pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_L, LED_FULL);
+	pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_H, LED_FULL);
+	pca9685_write_reg(pca, PCA9685_ALL_LED_ON_L, 0);
+	pca9685_write_reg(pca, PCA9685_ALL_LED_ON_H, 0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* Add an extra channel for ALL_LED */
-- 
2.31.1

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

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 15023 bytes --]

Hello,

On Tue, Apr 06, 2021 at 06:41:33PM +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
> 
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> 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 | 261 ++++++++++++++------------------------
>  1 file changed, 92 insertions(+), 169 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..5a2ce97e71fd 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)))
> +
>  struct pca9685 {
>  	struct pwm_chip chip;
>  	struct regmap *regmap;
> -	int period_ns;
>  #if IS_ENABLED(CONFIG_GPIOLIB)
>  	struct mutex lock;
>  	struct gpio_chip gpio;
> @@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
>  	return container_of(chip, struct pca9685, chip);
>  }
>  
> +/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> +{
> +	if (duty == 0) {
> +		/* Set the full OFF bit, which has the highest precedence */
> +		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> +	} else if (duty >= PCA9685_COUNTER_RANGE) {
> +		/* Set the full ON bit and clear the full OFF bit */
> +		regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> +		regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> +	} else {
> +		/* Set OFF time (clears the full OFF bit) */
> +		regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> +		regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> +		/* Clear the full ON bit */
> +		regmap_write(pca->regmap, REG_ON_H(channel), 0);
> +	}
> +}
> +
> +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> +{
> +	unsigned int off_h = 0, val = 0;
> +
> +	if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> +		/* HW does not support reading state of "all LEDs" channel */
> +		return 0;
> +	}
> +
> +	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> +	if (off_h & LED_FULL) {
> +		/* Full OFF bit is set */
> +		return 0;
> +	}
> +
> +	regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> +	if (val & LED_FULL) {
> +		/* Full ON bit is set */
> +		return PCA9685_COUNTER_RANGE;
> +	}
> +
> +	val = 0;

Why do you set val to 0 first? Do you get a compiler warning otherwise?

> +	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> +	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;

Is this a relevant bug fix? If both OFF_H.FULL and ON_H.FULL are set,
the output is low and this was diagnosed before as high.

>  }
>  
>  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);

Orthogonal to your patch:
I don't know the customs of GPIO drivers enough, but I wonder that
.free() results in setting the value of the GPIO?!

>  	pm_runtime_put(pca->chip.dev);
>  	pca9685_pwm_clear_inuse(pca, offset);
>  }
> @@ -246,167 +283,56 @@ 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;
> -	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 (prescale >= PCA9685_PRESCALE_MIN &&
> -			prescale <= PCA9685_PRESCALE_MAX) {
> -			/*
> -			 * 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);
> -
> -			/* Wake the chip up */
> -			pca9685_set_sleep_mode(pca, false);
> -
> -			pca->period_ns = period_ns;
> -		} else {
> -			dev_err(chip->dev,
> -				"prescaler not set: period out of bounds!\n");
> -			return -EINVAL;
> -		}
> -	}
> +	unsigned long long duty, prescale;
> +	unsigned int val = 0;
>  
> -	if (duty_ns < 1) {
> -		if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -			reg = PCA9685_ALL_LED_OFF_H;
> -		else
> -			reg = LED_N_OFF_H(pwm->hwpwm);
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -EINVAL;
>  
> -		regmap_write(pca->regmap, reg, LED_FULL);
> -
> -		return 0;
> +	prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> +					 PCA9685_COUNTER_RANGE * 1000) - 1;

Here the multiplication might overflow.  Also if period is small the
result of the division might be 0 and prescale might end up being -1ULL.
(But that's a problem that we had already before, so not a stopper for
this patch.)

> +	if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> +		dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> +		return -EINVAL;
>  	}
>  
> -	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);
> +	duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> +	duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);

Here we're losing precision. In general calculating the duty should be
done using time, not period counter values. (Again, that's an old
problem.)

>  
> +	if (duty < 1 || !state->enabled) {
> +		pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> +		return 0;
> +	} else if (duty == PCA9685_COUNTER_RANGE) {
> +		pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
>  		return 0;
>  	}
>  
> -	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> -	duty = DIV_ROUND_UP_ULL(duty, period_ns);

Oh, the new implementation uses DIV_ROUND_CLOSEST. IMHO either keep the
calculations as is, or use the preferred round-down.

> -
> -	if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -		reg = PCA9685_ALL_LED_OFF_L;
> -	else
> -		reg = LED_N_OFF_L(pwm->hwpwm);
> -
> -	regmap_write(pca->regmap, reg, (int)duty & 0xff);
> -
> -	if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -		reg = PCA9685_ALL_LED_OFF_H;
> -	else
> -		reg = LED_N_OFF_H(pwm->hwpwm);
> -
> -	regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
> -
> -	/* Clear the full ON bit, otherwise the set OFF time has no effect */
> -	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);
> -
> -	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);
> +	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);

I assume it's a requirement to stop the oscillator when changing the
prescaler?

>  
> -	/*
> -	 * 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);
> +		/* Change the chip-wide output frequency */
> +		regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);

The cast isn't necessary, is it?

> -	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> +		/* Wake the chip up */
> +		pca9685_set_sleep_mode(pca, false);
> +	}
>  
> +	pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
>  	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 +348,13 @@ 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_set_duty(pca, pwm->hwpwm, 0);
>  	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,
> +	.apply = pca9685_pwm_apply,
>  	.request = pca9685_pwm_request,
>  	.free = pca9685_pwm_free,
>  	.owner = THIS_MODULE,
> @@ -461,7 +385,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  			ret);
>  		return ret;
>  	}
> -	pca->period_ns = PCA9685_DEFAULT_PERIOD;
>  
>  	i2c_set_clientdata(client, pca);
>  
> @@ -484,9 +407,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 */
> -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> +	/* Reset OFF registers to POR default */
> +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);

Is this hunk unrelated to the patch description?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3311 bytes --]

On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> Implements .get_state to read-out the current hardware state.
> 
> 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.
> 
> Also 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>
> ---
> Changes since v6:
> - Added a comment regarding the division (Suggested by Uwe)
> - Rebased
> 
>  drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 5a2ce97e71fd..d4474c5ff96f 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> +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 long long duty;
> +	unsigned int val = 0;
> +
> +	/* Calculate (chip-wide) period from prescale value */
> +	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> +	/*
> +	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> +	 * The following calculation is therefore only a multiplication
> +	 * and we are not losing precision.
> +	 */
> +	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> +			(val + 1);
> +
> +	/* The (per-channel) polarity is fixed */
> +	state->polarity = PWM_POLARITY_NORMAL;
> +
> +	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> +		/*
> +		 * The "all LEDs" channel does not support HW readout
> +		 * Return 0 and disabled for backwards compatibility
> +		 */
> +		state->duty_cycle = 0;
> +		state->enabled = false;
> +		return;
> +	}
> +
> +	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> +
> +	state->enabled = !!duty;
> +	if (!state->enabled) {
> +		state->duty_cycle = 0;
> +		return;
> +	} else if (duty == PCA9685_COUNTER_RANGE) {
> +		state->duty_cycle = state->period;
> +		return;
> +	}
> +
> +	duty *= state->period;
> +	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;

Given that with duty = 0 the chip is still "on" and changing the duty
will first complete the currently running period, I'd model duty=0 as
enabled. This also simplifies the code a bit, to something like:


	state->enabled = true;
	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
	state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE);

(I'm using round-up here assuming apply uses round-down to get
idempotency. In the current patch set state this is wrong however.)

> +}
> +
>  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct pca9685 *pca = to_pca(chip);

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote:
> Add the flag and corresponding documentation for the new PWM staggering
> mode feature.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>

For the record, I don't like this and still prefer to make this
staggering explicit for the consumer by expanding struct pwm_state with
an .offset member to shift the active phase in the period.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1616 bytes --]

On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> may (if supported by the HW) delay the ON time of the channel relative
> to the channel number.
> This does not alter the duty cycle ratio and is only relevant for PWM
> chips with less prescalers than channels, which would otherwise assert
> multiple or even all enabled channels at the same time.
> 
> If this feature is supported by the driver and the flag is set on
> multiple channels, their ON times are spread out to improve EMI and
> reduce current spikes.

As said in reply to patch 4/8 already: I don't like this idea and
think this should be made explicit using a new offset member in struct
pwm_state instead. That's because I think that the wave form a PWM
generates should be (completely) defined by the consumer and not by a
mix between consumer and device tree. Also the consumer has no (sane)
way to determine if staggering is in use or not.

One side effect (at least for the pca9685) is that when programming a
new duty cycle it takes a bit longer than without staggering until the
new setting is active. 

Another objection I have is that we already have some technical debt
because there are already two different types of drivers (.apply vs
.config+.set_polarity+.enable+.disable) and I would like to unify this
first before introducing new stuff.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3862 bytes --]

On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote:
> Previously, the last used PWM channel could change the global prescale
> setting, even if other channels are already in use.
> 
> Fix it by only allowing the first enabled PWM to change the global
> chip-wide prescale setting. If there is more than one channel in use,
> the prescale settings resulting from the chosen periods must match.
> 
> GPIOs do not count as enabled PWMs as they are not using the prescaler
> and can't change it.
> 
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> Changes since v6:
> - Only allow the first PWM that is enabled to change the prescaler, not
>   the first one that uses the prescaler
> 
>  drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------
>  1 file changed, 56 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 24221ee7a77a..cf0c98e4ef44 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -23,11 +23,11 @@
>  #include <linux/bitmap.h>
>  
>  /*
> - * Because the PCA9685 has only one prescaler per chip, changing the period of
> - * one channel affects the period of all 16 PWM outputs!
> - * However, the ratio between each configured duty cycle and the chip-wide
> - * period remains constant, because the OFF time is set in proportion to the
> - * counter range.
> + * Because the PCA9685 has only one prescaler per chip, only the first channel
> + * that is enabled is allowed to change the prescale register.
> + * PWM channels requested afterwards must use a period that results in the same
> + * prescale setting as the one set by the first requested channel.
> + * GPIOs do not count as enabled PWMs as they are not using the prescaler.
>   */
>  
>  #define PCA9685_MODE1		0x00
> @@ -78,8 +78,9 @@
>  struct pca9685 {
>  	struct pwm_chip chip;
>  	struct regmap *regmap;
> -#if IS_ENABLED(CONFIG_GPIOLIB)
>  	struct mutex lock;
> +	DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
> +#if IS_ENABLED(CONFIG_GPIOLIB)
>  	struct gpio_chip gpio;
>  	DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
>  #endif
> @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
>  	return container_of(chip, struct pca9685, chip);
>  }
>  
> +/* This function is supposed to be called with the lock mutex held */
> +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
> +{
> +	/* No PWM enabled: Change allowed */
> +	if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
> +		return true;
> +	/* More than one PWM enabled: Change not allowed */
> +	if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
> +		return false;
> +	/*
> +	 * Only one PWM enabled: Change allowed if the PWM about to
> +	 * be changed is the one that is already enabled
> +	 */
> +	return test_bit(channel, pca->pwms_enabled);

Maybe this is a bit more effective?:

	DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1);	
	bitmap_zero(blablub, PCA9685_MAXCHAN + 1);
	bitmap_set(blablub, channel);
	return bitmap_subset(pca->pwms_enabled, blablub);

(but that's a minor issue, the suggested algorithm is correct.)

So:

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

(side-note: I wonder if the handling of the set-all channel is correct
here. But given that it is messy anyhow, (e.g. because setting some
state to this set-all channel doesn't influence pwm_get_state for the
individual channels) I don't object if there is another problem in this
corner case. IMHO just dropping this virtual channel would be nice.)

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2417 bytes --]

On Tue, Apr 06, 2021 at 06:41:40PM +0200, Clemens Gruber wrote:
> Regmap operations can fail if the underlying subsystem is not working
> properly (e.g. hogged I2C bus, etc.)
> As this is useful information for the user, print an error message if it
> happens.
> Let probe fail if the first regmap_read or the first regmap_write fails.
> 
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> Changes since v6:
> - Rebased
> 
>  drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
>  1 file changed, 59 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index cf0c98e4ef44..8a4993882b40 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
>  	return test_bit(channel, pca->pwms_enabled);
>  }
>  
> +static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
> +{
> +	struct device *dev = pca->chip.dev;
> +	int err;
> +
> +	err = regmap_read(pca->regmap, reg, val);
> +	if (err != 0)
> +		dev_err(dev, "regmap_read of register 0x%x failed: %d\n", reg, err);

Please use %pe to emit the error code instead of %d.

> +
> +	return err;
> +}
> +
> +static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
> +{
> +	struct device *dev = pca->chip.dev;
> +	int err;
> +
> +	err = regmap_write(pca->regmap, reg, val);
> +	if (err != 0)
> +		dev_err(dev, "regmap_write to register 0x%x failed: %d\n", reg, err);
> +
> +	return err;
> +}
> +
>  /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
>  static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
>  {
> @@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
>  
>  	if (duty == 0) {
>  		/* Set the full OFF bit, which has the highest precedence */
> -		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> +		pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);

You didn't check all return codes? How did you select the calls to
check?

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Clemens Gruber]

Hi,

On Wed, Apr 07, 2021 at 07:24:28AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Tue, Apr 06, 2021 at 06:41:33PM +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
> > 
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > 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 | 261 ++++++++++++++------------------------
> >  1 file changed, 92 insertions(+), 169 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..5a2ce97e71fd 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)))
> > +
> >  struct pca9685 {
> >  	struct pwm_chip chip;
> >  	struct regmap *regmap;
> > -	int period_ns;
> >  #if IS_ENABLED(CONFIG_GPIOLIB)
> >  	struct mutex lock;
> >  	struct gpio_chip gpio;
> > @@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> >  	return container_of(chip, struct pca9685, chip);
> >  }
> >  
> > +/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> > +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > +{
> > +	if (duty == 0) {
> > +		/* Set the full OFF bit, which has the highest precedence */
> > +		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > +	} else if (duty >= PCA9685_COUNTER_RANGE) {
> > +		/* Set the full ON bit and clear the full OFF bit */
> > +		regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > +		regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > +	} else {
> > +		/* Set OFF time (clears the full OFF bit) */
> > +		regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > +		regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > +		/* Clear the full ON bit */
> > +		regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > +	}
> > +}
> > +
> > +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > +{
> > +	unsigned int off_h = 0, val = 0;
> > +
> > +	if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > +		/* HW does not support reading state of "all LEDs" channel */
> > +		return 0;
> > +	}
> > +
> > +	regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > +	if (off_h & LED_FULL) {
> > +		/* Full OFF bit is set */
> > +		return 0;
> > +	}
> > +
> > +	regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > +	if (val & LED_FULL) {
> > +		/* Full ON bit is set */
> > +		return PCA9685_COUNTER_RANGE;
> > +	}
> > +
> > +	val = 0;
> 
> Why do you set val to 0 first? Do you get a compiler warning otherwise?

No, just to have it set to 0 in case regmap_read fails / val was not
set.

> 
> > +	regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > +	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;
> 
> Is this a relevant bug fix? If both OFF_H.FULL and ON_H.FULL are set,
> the output is low and this was diagnosed before as high.

Yes, I think so.
Previously, only the full ON bit was toggled (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.

I should probably mention it in the commit description.

> 
> >  }
> >  
> >  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);
> 
> Orthogonal to your patch:
> I don't know the customs of GPIO drivers enough, but I wonder that
> .free() results in setting the value of the GPIO?!

Yeah. I am not sure about that either, but I kept that as it was before.

> 
> >  	pm_runtime_put(pca->chip.dev);
> >  	pca9685_pwm_clear_inuse(pca, offset);
> >  }
> > @@ -246,167 +283,56 @@ 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;
> > -	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 (prescale >= PCA9685_PRESCALE_MIN &&
> > -			prescale <= PCA9685_PRESCALE_MAX) {
> > -			/*
> > -			 * 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);
> > -
> > -			/* Wake the chip up */
> > -			pca9685_set_sleep_mode(pca, false);
> > -
> > -			pca->period_ns = period_ns;
> > -		} else {
> > -			dev_err(chip->dev,
> > -				"prescaler not set: period out of bounds!\n");
> > -			return -EINVAL;
> > -		}
> > -	}
> > +	unsigned long long duty, prescale;
> > +	unsigned int val = 0;
> >  
> > -	if (duty_ns < 1) {
> > -		if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -			reg = PCA9685_ALL_LED_OFF_H;
> > -		else
> > -			reg = LED_N_OFF_H(pwm->hwpwm);
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> >  
> > -		regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > -		return 0;
> > +	prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > +					 PCA9685_COUNTER_RANGE * 1000) - 1;
> 
> Here the multiplication might overflow.  Also if period is small the
> result of the division might be 0 and prescale might end up being -1ULL.
> (But that's a problem that we had already before, so not a stopper for
> this patch.)

We would catch that with prescale > PCA9685_PRESCALE_MAX (below) though.

> 
> > +	if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > +		dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > +		return -EINVAL;
> >  	}
> >  
> > -	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);
> > +	duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > +	duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
> 
> Here we're losing precision. In general calculating the duty should be
> done using time, not period counter values. (Again, that's an old
> problem.)
> 
> >  
> > +	if (duty < 1 || !state->enabled) {
> > +		pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > +		return 0;
> > +	} else if (duty == PCA9685_COUNTER_RANGE) {
> > +		pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> >  		return 0;
> >  	}
> >  
> > -	duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> > -	duty = DIV_ROUND_UP_ULL(duty, period_ns);
> 
> Oh, the new implementation uses DIV_ROUND_CLOSEST. IMHO either keep the
> calculations as is, or use the preferred round-down.

I can change that one to round-down if that's the preferred method.

> 
> > -
> > -	if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -		reg = PCA9685_ALL_LED_OFF_L;
> > -	else
> > -		reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > -	regmap_write(pca->regmap, reg, (int)duty & 0xff);
> > -
> > -	if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -		reg = PCA9685_ALL_LED_OFF_H;
> > -	else
> > -		reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > -	regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
> > -
> > -	/* Clear the full ON bit, otherwise the set OFF time has no effect */
> > -	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);
> > -
> > -	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);
> > +	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);
> 
> I assume it's a requirement to stop the oscillator when changing the
> prescaler?

Yes.

> 
> >  
> > -	/*
> > -	 * 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);
> > +		/* Change the chip-wide output frequency */
> > +		regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> 
> The cast isn't necessary, is it?

You are right, I will remove it.

> 
> > -	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > +		/* Wake the chip up */
> > +		pca9685_set_sleep_mode(pca, false);
> > +	}
> >  
> > +	pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> >  	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 +348,13 @@ 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_set_duty(pca, pwm->hwpwm, 0);
> >  	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,
> > +	.apply = pca9685_pwm_apply,
> >  	.request = pca9685_pwm_request,
> >  	.free = pca9685_pwm_free,
> >  	.owner = THIS_MODULE,
> > @@ -461,7 +385,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  			ret);
> >  		return ret;
> >  	}
> > -	pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >  
> >  	i2c_set_clientdata(client, pca);
> >  
> > @@ -484,9 +407,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 */
> > -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
> > -	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
> > +	/* Reset OFF registers to POR default */
> > +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> > +	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
> 
> Is this hunk unrelated to the patch description?

It's a fix of the resets because the POR state is LED_FULL, not 0.
Do you want me to extract it into a separate patch?

Thanks,
Clemens

Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout

[Clemens Gruber]

On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> > Implements .get_state to read-out the current hardware state.
> > 
> > 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.
> > 
> > Also 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>
> > ---
> > Changes since v6:
> > - Added a comment regarding the division (Suggested by Uwe)
> > - Rebased
> > 
> >  drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 5a2ce97e71fd..d4474c5ff96f 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	return 0;
> >  }
> >  
> > +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 long long duty;
> > +	unsigned int val = 0;
> > +
> > +	/* Calculate (chip-wide) period from prescale value */
> > +	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > +	/*
> > +	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> > +	 * The following calculation is therefore only a multiplication
> > +	 * and we are not losing precision.
> > +	 */
> > +	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > +			(val + 1);
> > +
> > +	/* The (per-channel) polarity is fixed */
> > +	state->polarity = PWM_POLARITY_NORMAL;
> > +
> > +	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > +		/*
> > +		 * The "all LEDs" channel does not support HW readout
> > +		 * Return 0 and disabled for backwards compatibility
> > +		 */
> > +		state->duty_cycle = 0;
> > +		state->enabled = false;
> > +		return;
> > +	}
> > +
> > +	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > +
> > +	state->enabled = !!duty;
> > +	if (!state->enabled) {
> > +		state->duty_cycle = 0;
> > +		return;
> > +	} else if (duty == PCA9685_COUNTER_RANGE) {
> > +		state->duty_cycle = state->period;
> > +		return;
> > +	}
> > +
> > +	duty *= state->period;
> > +	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> 
> Given that with duty = 0 the chip is still "on" and changing the duty
> will first complete the currently running period, I'd model duty=0 as
> enabled. This also simplifies the code a bit, to something like:
> 
> 
> 	state->enabled = true;
> 	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> 	state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE);
> 
> (I'm using round-up here assuming apply uses round-down to get
> idempotency. In the current patch set state this is wrong however.)

So, in your opinion, every requested PWM of the pca9685 should always be
enabled by default (from the PWM core viewpoint) ?

And this wouldn't break the following because pwm_get_state does not
actually read out the hw state:
pwm_get_state -> enabled=true duty=0
pwm_apply_state -> enabled =false duty=0
pwm_get_state -> enabled=false duty=0

Thanks,
Clemens

Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4107 bytes --]

On Wed, Apr 07, 2021 at 09:33:20AM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> > > Implements .get_state to read-out the current hardware state.
> > > 
> > > 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.
> > > 
> > > Also 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>
> > > ---
> > > Changes since v6:
> > > - Added a comment regarding the division (Suggested by Uwe)
> > > - Rebased
> > > 
> > >  drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 46 insertions(+)
> > > 
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 5a2ce97e71fd..d4474c5ff96f 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > >  	return 0;
> > >  }
> > >  
> > > +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 long long duty;
> > > +	unsigned int val = 0;
> > > +
> > > +	/* Calculate (chip-wide) period from prescale value */
> > > +	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > > +	/*
> > > +	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> > > +	 * The following calculation is therefore only a multiplication
> > > +	 * and we are not losing precision.
> > > +	 */
> > > +	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > > +			(val + 1);
> > > +
> > > +	/* The (per-channel) polarity is fixed */
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > > +		/*
> > > +		 * The "all LEDs" channel does not support HW readout
> > > +		 * Return 0 and disabled for backwards compatibility
> > > +		 */
> > > +		state->duty_cycle = 0;
> > > +		state->enabled = false;
> > > +		return;
> > > +	}
> > > +
> > > +	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > > +
> > > +	state->enabled = !!duty;
> > > +	if (!state->enabled) {
> > > +		state->duty_cycle = 0;
> > > +		return;
> > > +	} else if (duty == PCA9685_COUNTER_RANGE) {
> > > +		state->duty_cycle = state->period;
> > > +		return;
> > > +	}
> > > +
> > > +	duty *= state->period;
> > > +	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> > 
> > Given that with duty = 0 the chip is still "on" and changing the duty
> > will first complete the currently running period, I'd model duty=0 as
> > enabled. This also simplifies the code a bit, to something like:
> > 
> > 
> > 	state->enabled = true;
> > 	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > 	state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE);
> > 
> > (I'm using round-up here assuming apply uses round-down to get
> > idempotency. In the current patch set state this is wrong however.)
> 
> So, in your opinion, every requested PWM of the pca9685 should always be
> enabled by default (from the PWM core viewpoint) ?
> 
> And this wouldn't break the following because pwm_get_state does not
> actually read out the hw state:
> pwm_get_state -> enabled=true duty=0
> pwm_apply_state -> enabled =false duty=0
> pwm_get_state -> enabled=false duty=0

I don't see any breakage here. Either there is none or I failed to grasp
where you see a problem.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 2/8] pwm: pca9685: Support hardware readout

[Clemens Gruber]

On Wed, Apr 07, 2021 at 11:09:43AM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 07, 2021 at 09:33:20AM +0200, Clemens Gruber wrote:
> > On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote:
> > > > Implements .get_state to read-out the current hardware state.
> > > > 
> > > > 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.
> > > > 
> > > > Also 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>
> > > > ---
> > > > Changes since v6:
> > > > - Added a comment regarding the division (Suggested by Uwe)
> > > > - Rebased
> > > > 
> > > >  drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 46 insertions(+)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > index 5a2ce97e71fd..d4474c5ff96f 100644
> > > > --- a/drivers/pwm/pwm-pca9685.c
> > > > +++ b/drivers/pwm/pwm-pca9685.c
> > > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +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 long long duty;
> > > > +	unsigned int val = 0;
> > > > +
> > > > +	/* Calculate (chip-wide) period from prescale value */
> > > > +	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > > > +	/*
> > > > +	 * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
> > > > +	 * The following calculation is therefore only a multiplication
> > > > +	 * and we are not losing precision.
> > > > +	 */
> > > > +	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > > > +			(val + 1);
> > > > +
> > > > +	/* The (per-channel) polarity is fixed */
> > > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > > +
> > > > +	if (pwm->hwpwm >= PCA9685_MAXCHAN) {
> > > > +		/*
> > > > +		 * The "all LEDs" channel does not support HW readout
> > > > +		 * Return 0 and disabled for backwards compatibility
> > > > +		 */
> > > > +		state->duty_cycle = 0;
> > > > +		state->enabled = false;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > > > +
> > > > +	state->enabled = !!duty;
> > > > +	if (!state->enabled) {
> > > > +		state->duty_cycle = 0;
> > > > +		return;
> > > > +	} else if (duty == PCA9685_COUNTER_RANGE) {
> > > > +		state->duty_cycle = state->period;
> > > > +		return;
> > > > +	}
> > > > +
> > > > +	duty *= state->period;
> > > > +	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> > > 
> > > Given that with duty = 0 the chip is still "on" and changing the duty
> > > will first complete the currently running period, I'd model duty=0 as
> > > enabled. This also simplifies the code a bit, to something like:
> > > 
> > > 
> > > 	state->enabled = true;
> > > 	duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
> > > 	state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE);
> > > 
> > > (I'm using round-up here assuming apply uses round-down to get
> > > idempotency. In the current patch set state this is wrong however.)
> > 
> > So, in your opinion, every requested PWM of the pca9685 should always be
> > enabled by default (from the PWM core viewpoint) ?
> > 
> > And this wouldn't break the following because pwm_get_state does not
> > actually read out the hw state:
> > pwm_get_state -> enabled=true duty=0
> > pwm_apply_state -> enabled =false duty=0
> > pwm_get_state -> enabled=false duty=0
> 
> I don't see any breakage here. Either there is none or I failed to grasp
> where you see a problem.

Me neither, I was just thinking out loud.

Clemens

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > may (if supported by the HW) delay the ON time of the channel relative
> > to the channel number.
> > This does not alter the duty cycle ratio and is only relevant for PWM
> > chips with less prescalers than channels, which would otherwise assert
> > multiple or even all enabled channels at the same time.
> > 
> > If this feature is supported by the driver and the flag is set on
> > multiple channels, their ON times are spread out to improve EMI and
> > reduce current spikes.
> 
> As said in reply to patch 4/8 already: I don't like this idea and
> think this should be made explicit using a new offset member in struct
> pwm_state instead. That's because I think that the wave form a PWM
> generates should be (completely) defined by the consumer and not by a
> mix between consumer and device tree. Also the consumer has no (sane)
> way to determine if staggering is in use or not.

I don't think offsets are ideal for this feature: It makes it more
cumbersome for the user, because he has to allocate the offsets
himself instead of a simple on/off switch.
The envisioned usecase is: "I want better EMI behavior and don't care
about the individual channels no longer being asserted at the exact same
time".

> One side effect (at least for the pca9685) is that when programming a
> new duty cycle it takes a bit longer than without staggering until the
> new setting is active. 

Yes, but it can be turned off if this is a problem, now even per-PWM.

> Another objection I have is that we already have some technical debt
> because there are already two different types of drivers (.apply vs
> .config+.set_polarity+.enable+.disable) and I would like to unify this
> first before introducing new stuff.

But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
I am only adding another flag.

Thierry: What's your take on this?

Thanks,
Clemens

Re: [PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs

[Clemens Gruber]

On Wed, Apr 07, 2021 at 08:12:29AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote:
> > Previously, the last used PWM channel could change the global prescale
> > setting, even if other channels are already in use.
> > 
> > Fix it by only allowing the first enabled PWM to change the global
> > chip-wide prescale setting. If there is more than one channel in use,
> > the prescale settings resulting from the chosen periods must match.
> > 
> > GPIOs do not count as enabled PWMs as they are not using the prescaler
> > and can't change it.
> > 
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > Changes since v6:
> > - Only allow the first PWM that is enabled to change the prescaler, not
> >   the first one that uses the prescaler
> > 
> >  drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------
> >  1 file changed, 56 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 24221ee7a77a..cf0c98e4ef44 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -23,11 +23,11 @@
> >  #include <linux/bitmap.h>
> >  
> >  /*
> > - * Because the PCA9685 has only one prescaler per chip, changing the period of
> > - * one channel affects the period of all 16 PWM outputs!
> > - * However, the ratio between each configured duty cycle and the chip-wide
> > - * period remains constant, because the OFF time is set in proportion to the
> > - * counter range.
> > + * Because the PCA9685 has only one prescaler per chip, only the first channel
> > + * that is enabled is allowed to change the prescale register.
> > + * PWM channels requested afterwards must use a period that results in the same
> > + * prescale setting as the one set by the first requested channel.
> > + * GPIOs do not count as enabled PWMs as they are not using the prescaler.
> >   */
> >  
> >  #define PCA9685_MODE1		0x00
> > @@ -78,8 +78,9 @@
> >  struct pca9685 {
> >  	struct pwm_chip chip;
> >  	struct regmap *regmap;
> > -#if IS_ENABLED(CONFIG_GPIOLIB)
> >  	struct mutex lock;
> > +	DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
> > +#if IS_ENABLED(CONFIG_GPIOLIB)
> >  	struct gpio_chip gpio;
> >  	DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
> >  #endif
> > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> >  	return container_of(chip, struct pca9685, chip);
> >  }
> >  
> > +/* This function is supposed to be called with the lock mutex held */
> > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
> > +{
> > +	/* No PWM enabled: Change allowed */
> > +	if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
> > +		return true;
> > +	/* More than one PWM enabled: Change not allowed */
> > +	if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
> > +		return false;
> > +	/*
> > +	 * Only one PWM enabled: Change allowed if the PWM about to
> > +	 * be changed is the one that is already enabled
> > +	 */
> > +	return test_bit(channel, pca->pwms_enabled);
> 
> Maybe this is a bit more effective?:
> 
> 	DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1);	
> 	bitmap_zero(blablub, PCA9685_MAXCHAN + 1);
> 	bitmap_set(blablub, channel);
> 	return bitmap_subset(pca->pwms_enabled, blablub);

But if no PWM is enabled, it should return true, not false.

> (but that's a minor issue, the suggested algorithm is correct.)

I would prefer to keep it explicit because it is a little easier to
follow and probably not worth optimizing.

> 
> So:
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks.

> 
> (side-note: I wonder if the handling of the set-all channel is correct
> here. But given that it is messy anyhow, (e.g. because setting some
> state to this set-all channel doesn't influence pwm_get_state for the
> individual channels) I don't object if there is another problem in this
> corner case. IMHO just dropping this virtual channel would be nice.)

As you can't request the all channel and the individual channels
together, there shouldn't be any problems.

I agree that it would be nice to drop the ALL channel support.

Clemens

Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls

[Clemens Gruber]

On Wed, Apr 07, 2021 at 08:16:19AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 06:41:40PM +0200, Clemens Gruber wrote:
> > Regmap operations can fail if the underlying subsystem is not working
> > properly (e.g. hogged I2C bus, etc.)
> > As this is useful information for the user, print an error message if it
> > happens.
> > Let probe fail if the first regmap_read or the first regmap_write fails.
> > 
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > Changes since v6:
> > - Rebased
> > 
> >  drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 59 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index cf0c98e4ef44..8a4993882b40 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
> >  	return test_bit(channel, pca->pwms_enabled);
> >  }
> >  
> > +static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
> > +{
> > +	struct device *dev = pca->chip.dev;
> > +	int err;
> > +
> > +	err = regmap_read(pca->regmap, reg, val);
> > +	if (err != 0)
> > +		dev_err(dev, "regmap_read of register 0x%x failed: %d\n", reg, err);
> 
> Please use %pe to emit the error code instead of %d.

Will do.

> 
> > +
> > +	return err;
> > +}
> > +
> > +static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
> > +{
> > +	struct device *dev = pca->chip.dev;
> > +	int err;
> > +
> > +	err = regmap_write(pca->regmap, reg, val);
> > +	if (err != 0)
> > +		dev_err(dev, "regmap_write to register 0x%x failed: %d\n", reg, err);
> > +
> > +	return err;
> > +}
> > +
> >  /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> >  static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> >  {
> > @@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
> >  
> >  	if (duty == 0) {
> >  		/* Set the full OFF bit, which has the highest precedence */
> > -		regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > +		pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
> 
> You didn't check all return codes? How did you select the calls to
> check?

No, because it would become a big mess and really obstruct readability
in my opinion.

So I chose some kind of middleground:
I decided to check the first regmap_read and regmap_write in probe and
return the error code if something goes wrong there.
If something goes wrong after probe, I only print an error message.

Is that acceptable?

Thanks,
Clemens

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3031 bytes --]

On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > > may (if supported by the HW) delay the ON time of the channel relative
> > > to the channel number.
> > > This does not alter the duty cycle ratio and is only relevant for PWM
> > > chips with less prescalers than channels, which would otherwise assert
> > > multiple or even all enabled channels at the same time.
> > > 
> > > If this feature is supported by the driver and the flag is set on
> > > multiple channels, their ON times are spread out to improve EMI and
> > > reduce current spikes.
> > 
> > As said in reply to patch 4/8 already: I don't like this idea and
> > think this should be made explicit using a new offset member in struct
> > pwm_state instead. That's because I think that the wave form a PWM
> > generates should be (completely) defined by the consumer and not by a
> > mix between consumer and device tree. Also the consumer has no (sane)
> > way to determine if staggering is in use or not.
> 
> I don't think offsets are ideal for this feature: It makes it more
> cumbersome for the user, because he has to allocate the offsets
> himself instead of a simple on/off switch.
> The envisioned usecase is: "I want better EMI behavior and don't care
> about the individual channels no longer being asserted at the exact same
> time".

The formal thing is: "I want better EMI behavior and don't care if
periods start with the active phase, it might be anywhere, even over a
period boundary." Being asserted at the exact same time is just a detail
for the pca9685.
 
> > One side effect (at least for the pca9685) is that when programming a
> > new duty cycle it takes a bit longer than without staggering until the
> > new setting is active. 
> 
> Yes, but it can be turned off if this is a problem, now even per-PWM.

Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but
details.)

> > Another objection I have is that we already have some technical debt
> > because there are already two different types of drivers (.apply vs
> > .config+.set_polarity+.enable+.disable) and I would like to unify this
> > first before introducing new stuff.
> 
> But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
> I am only adding another flag.

I understand your reasoning, and similar to "This diplay backlight needs
an inverted PWM (as a low duty-cycle results in a high brightness" this
semantic "This consumer doesn't care if the active cycle is anywhere in
the period". Hmm, maybe I just have to think about it a bit more to
become friends with that thought.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 7/8] pwm: pca9685: Restrict period change for enabled PWMs

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4469 bytes --]

On Wed, Apr 07, 2021 at 10:41:27PM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 08:12:29AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote:
> > > Previously, the last used PWM channel could change the global prescale
> > > setting, even if other channels are already in use.
> > > 
> > > Fix it by only allowing the first enabled PWM to change the global
> > > chip-wide prescale setting. If there is more than one channel in use,
> > > the prescale settings resulting from the chosen periods must match.
> > > 
> > > GPIOs do not count as enabled PWMs as they are not using the prescaler
> > > and can't change it.
> > > 
> > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > > ---
> > > Changes since v6:
> > > - Only allow the first PWM that is enabled to change the prescaler, not
> > >   the first one that uses the prescaler
> > > 
> > >  drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------
> > >  1 file changed, 56 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 24221ee7a77a..cf0c98e4ef44 100644
> > > --- a/drivers/pwm/pwm-pca9685.c
> > > +++ b/drivers/pwm/pwm-pca9685.c
> > > @@ -23,11 +23,11 @@
> > >  #include <linux/bitmap.h>
> > >  
> > >  /*
> > > - * Because the PCA9685 has only one prescaler per chip, changing the period of
> > > - * one channel affects the period of all 16 PWM outputs!
> > > - * However, the ratio between each configured duty cycle and the chip-wide
> > > - * period remains constant, because the OFF time is set in proportion to the
> > > - * counter range.
> > > + * Because the PCA9685 has only one prescaler per chip, only the first channel
> > > + * that is enabled is allowed to change the prescale register.
> > > + * PWM channels requested afterwards must use a period that results in the same
> > > + * prescale setting as the one set by the first requested channel.
> > > + * GPIOs do not count as enabled PWMs as they are not using the prescaler.
> > >   */
> > >  
> > >  #define PCA9685_MODE1		0x00
> > > @@ -78,8 +78,9 @@
> > >  struct pca9685 {
> > >  	struct pwm_chip chip;
> > >  	struct regmap *regmap;
> > > -#if IS_ENABLED(CONFIG_GPIOLIB)
> > >  	struct mutex lock;
> > > +	DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
> > > +#if IS_ENABLED(CONFIG_GPIOLIB)
> > >  	struct gpio_chip gpio;
> > >  	DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
> > >  #endif
> > > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > >  	return container_of(chip, struct pca9685, chip);
> > >  }
> > >  
> > > +/* This function is supposed to be called with the lock mutex held */
> > > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
> > > +{
> > > +	/* No PWM enabled: Change allowed */
> > > +	if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
> > > +		return true;
> > > +	/* More than one PWM enabled: Change not allowed */
> > > +	if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
> > > +		return false;
> > > +	/*
> > > +	 * Only one PWM enabled: Change allowed if the PWM about to
> > > +	 * be changed is the one that is already enabled
> > > +	 */
> > > +	return test_bit(channel, pca->pwms_enabled);
> > 
> > Maybe this is a bit more effective?:
> > 
> > 	DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1);	
> > 	bitmap_zero(blablub, PCA9685_MAXCHAN + 1);
> > 	bitmap_set(blablub, channel);
> > 	return bitmap_subset(pca->pwms_enabled, blablub);
> 
> But if no PWM is enabled, it should return true, not false.

If no PWM is enabled we have pca->pwms_enabled = empty set which is a
subset of every set. So I'd expect this case to be handled just fine.

> > (but that's a minor issue, the suggested algorithm is correct.)
> 
> I would prefer to keep it explicit because it is a little easier to
> follow and probably not worth optimizing.

I didn't find it hard to follow, but I'm willing to accept that this
isn't representative. I'm ok with keeping the code as is.
 
> I agree that it would be nice to drop the ALL channel support.

Maybe deprecate it using a config item? But no hurry.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 8/8] pwm: pca9685: Add error messages for failed regmap calls

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

Hello Clemens,

On Wed, Apr 07, 2021 at 10:47:45PM +0200, Clemens Gruber wrote:
> On Wed, Apr 07, 2021 at 08:16:19AM +0200, Uwe Kleine-König wrote:
> > You didn't check all return codes? How did you select the calls to
> > check?
> 
> No, because it would become a big mess and really obstruct readability
> in my opinion.
> 
> So I chose some kind of middleground:
> I decided to check the first regmap_read and regmap_write in probe and
> return the error code if something goes wrong there.
> If something goes wrong after probe, I only print an error message.
> 
> Is that acceptable?

I wanted to have that in the commit log, but just noticed that I didn't
read it carefully enough, it's already there.

So if you change %d in the error messages to %pe I'm happy with this
patch.

Thanks
Uwe


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4182 bytes --]

On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote:
> On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote:
> > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > > > may (if supported by the HW) delay the ON time of the channel relative
> > > > to the channel number.
> > > > This does not alter the duty cycle ratio and is only relevant for PWM
> > > > chips with less prescalers than channels, which would otherwise assert
> > > > multiple or even all enabled channels at the same time.
> > > > 
> > > > If this feature is supported by the driver and the flag is set on
> > > > multiple channels, their ON times are spread out to improve EMI and
> > > > reduce current spikes.
> > > 
> > > As said in reply to patch 4/8 already: I don't like this idea and
> > > think this should be made explicit using a new offset member in struct
> > > pwm_state instead. That's because I think that the wave form a PWM
> > > generates should be (completely) defined by the consumer and not by a
> > > mix between consumer and device tree. Also the consumer has no (sane)
> > > way to determine if staggering is in use or not.
> > 
> > I don't think offsets are ideal for this feature: It makes it more
> > cumbersome for the user, because he has to allocate the offsets
> > himself instead of a simple on/off switch.
> > The envisioned usecase is: "I want better EMI behavior and don't care
> > about the individual channels no longer being asserted at the exact same
> > time".
> 
> The formal thing is: "I want better EMI behavior and don't care if
> periods start with the active phase, it might be anywhere, even over a
> period boundary." Being asserted at the exact same time is just a detail
> for the pca9685.
>  
> > > One side effect (at least for the pca9685) is that when programming a
> > > new duty cycle it takes a bit longer than without staggering until the
> > > new setting is active. 
> > 
> > Yes, but it can be turned off if this is a problem, now even per-PWM.
> 
> Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but
> details.)
> 
> > > Another objection I have is that we already have some technical debt
> > > because there are already two different types of drivers (.apply vs
> > > .config+.set_polarity+.enable+.disable) and I would like to unify this
> > > first before introducing new stuff.
> > 
> > But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
> > I am only adding another flag.
> 
> I understand your reasoning, and similar to "This diplay backlight needs
> an inverted PWM (as a low duty-cycle results in a high brightness" this
> semantic "This consumer doesn't care if the active cycle is anywhere in
> the period". Hmm, maybe I just have to think about it a bit more to
> become friends with that thought.

Yes, I think that's basically what this is saying. I think we're perhaps
getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
the impression that we're dealing with some provider-specific feature,
whereas what we really want to express is that the PWM doesn't care
exactly when the active cycle starts and based on that a provider that
can support it may optimize the EMI behavior.

Maybe we can find a better name for this? Ultimately what this means is
that the consumer is primarily interested in the power output of the PWM
rather than the exact shape of the signal. So perhaps something like
PWM_USAGE_POWER would be more appropriate.

Come to think of it, a flag like that might even be useful to implement
the common case of emulating inverted polarity with reversing the duty
cycle. That is, if PWM_USAGE_POWER | PWM_POLARITY_INVERSED was specified
and the PWM provider did not support polarity inversion, the inversion
could still be implemented using emulation. Currently we push that logic
down into consumers, but this could be a way to bring that up into
drivers, or perhaps even the core.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote:
> > On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote:
> > > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > > > > may (if supported by the HW) delay the ON time of the channel relative
> > > > > to the channel number.
> > > > > This does not alter the duty cycle ratio and is only relevant for PWM
> > > > > chips with less prescalers than channels, which would otherwise assert
> > > > > multiple or even all enabled channels at the same time.
> > > > > 
> > > > > If this feature is supported by the driver and the flag is set on
> > > > > multiple channels, their ON times are spread out to improve EMI and
> > > > > reduce current spikes.
> > > > 
> > > > As said in reply to patch 4/8 already: I don't like this idea and
> > > > think this should be made explicit using a new offset member in struct
> > > > pwm_state instead. That's because I think that the wave form a PWM
> > > > generates should be (completely) defined by the consumer and not by a
> > > > mix between consumer and device tree. Also the consumer has no (sane)
> > > > way to determine if staggering is in use or not.
> > > 
> > > I don't think offsets are ideal for this feature: It makes it more
> > > cumbersome for the user, because he has to allocate the offsets
> > > himself instead of a simple on/off switch.
> > > The envisioned usecase is: "I want better EMI behavior and don't care
> > > about the individual channels no longer being asserted at the exact same
> > > time".
> > 
> > The formal thing is: "I want better EMI behavior and don't care if
> > periods start with the active phase, it might be anywhere, even over a
> > period boundary." Being asserted at the exact same time is just a detail
> > for the pca9685.
> >  
> > > > One side effect (at least for the pca9685) is that when programming a
> > > > new duty cycle it takes a bit longer than without staggering until the
> > > > new setting is active. 
> > > 
> > > Yes, but it can be turned off if this is a problem, now even per-PWM.
> > 
> > Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but
> > details.)
> > 
> > > > Another objection I have is that we already have some technical debt
> > > > because there are already two different types of drivers (.apply vs
> > > > .config+.set_polarity+.enable+.disable) and I would like to unify this
> > > > first before introducing new stuff.
> > > 
> > > But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
> > > I am only adding another flag.
> > 
> > I understand your reasoning, and similar to "This diplay backlight needs
> > an inverted PWM (as a low duty-cycle results in a high brightness" this
> > semantic "This consumer doesn't care if the active cycle is anywhere in
> > the period". Hmm, maybe I just have to think about it a bit more to
> > become friends with that thought.
> 
> Yes, I think that's basically what this is saying. I think we're perhaps
> getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> the impression that we're dealing with some provider-specific feature,
> whereas what we really want to express is that the PWM doesn't care
> exactly when the active cycle starts and based on that a provider that
> can support it may optimize the EMI behavior.
> 
> Maybe we can find a better name for this? Ultimately what this means is
> that the consumer is primarily interested in the power output of the PWM
> rather than the exact shape of the signal. So perhaps something like
> PWM_USAGE_POWER would be more appropriate.

Yes, although it would then no longer be obvious that this feature leads
to improved EMI behavior, as long as we mention that in the docs, I
think it's a good idea

Maybe document it as follows?
PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
for EMI improvements, as long as the power output stays the same

> 
> Come to think of it, a flag like that might even be useful to implement
> the common case of emulating inverted polarity with reversing the duty
> cycle. That is, if PWM_USAGE_POWER | PWM_POLARITY_INVERSED was specified
> and the PWM provider did not support polarity inversion, the inversion
> could still be implemented using emulation. Currently we push that logic
> down into consumers, but this could be a way to bring that up into
> drivers, or perhaps even the core.

Interesting, but that would be left for another series in the future, I
assume?

Thanks,
Clemens

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1743 bytes --]

On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > Yes, I think that's basically what this is saying. I think we're perhaps
> > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > the impression that we're dealing with some provider-specific feature,
> > whereas what we really want to express is that the PWM doesn't care
> > exactly when the active cycle starts and based on that a provider that
> > can support it may optimize the EMI behavior.
> > 
> > Maybe we can find a better name for this? Ultimately what this means is
> > that the consumer is primarily interested in the power output of the PWM
> > rather than the exact shape of the signal. So perhaps something like
> > PWM_USAGE_POWER would be more appropriate.
> 
> Yes, although it would then no longer be obvious that this feature leads
> to improved EMI behavior, as long as we mention that in the docs, I
> think it's a good idea
> 
> Maybe document it as follows?
> PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> for EMI improvements, as long as the power output stays the same

I don't like both names, because for someone who is only halfway into
PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?
When a consumer is only interested in the power output than

	.period = 20
	.duty_cycle = 5

would also be an allowed response for the request

	.period = 200
	.duty_cycle = 50

and this is not what is in the focus here.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > > Yes, I think that's basically what this is saying. I think we're perhaps
> > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > > the impression that we're dealing with some provider-specific feature,
> > > whereas what we really want to express is that the PWM doesn't care
> > > exactly when the active cycle starts and based on that a provider that
> > > can support it may optimize the EMI behavior.
> > > 
> > > Maybe we can find a better name for this? Ultimately what this means is
> > > that the consumer is primarily interested in the power output of the PWM
> > > rather than the exact shape of the signal. So perhaps something like
> > > PWM_USAGE_POWER would be more appropriate.
> > 
> > Yes, although it would then no longer be obvious that this feature leads
> > to improved EMI behavior, as long as we mention that in the docs, I
> > think it's a good idea
> > 
> > Maybe document it as follows?
> > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> > for EMI improvements, as long as the power output stays the same
> 
> I don't like both names, because for someone who is only halfway into
> PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?

Sounds good to me.

> When a consumer is only interested in the power output than
> 
> 	.period = 20
> 	.duty_cycle = 5
> 
> would also be an allowed response for the request
> 
> 	.period = 200
> 	.duty_cycle = 50
> 
> and this is not what is in the focus here.

Right.

If Thierry agrees, I can spin up a new revision.

Maybe we can get it into 5.13 after all.

Thanks,
Clemens

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5586 bytes --]

On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > On Wed, Apr 07, 2021 at 11:34:03PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Apr 07, 2021 at 10:21:10PM +0200, Clemens Gruber wrote:
> > > > On Wed, Apr 07, 2021 at 07:46:58AM +0200, Uwe Kleine-König wrote:
> > > > > On Tue, Apr 06, 2021 at 06:41:37PM +0200, Clemens Gruber wrote:
> > > > > > If the flag PWM_STAGGERING_ALLOWED is set on a channel, the PWM driver
> > > > > > may (if supported by the HW) delay the ON time of the channel relative
> > > > > > to the channel number.
> > > > > > This does not alter the duty cycle ratio and is only relevant for PWM
> > > > > > chips with less prescalers than channels, which would otherwise assert
> > > > > > multiple or even all enabled channels at the same time.
> > > > > > 
> > > > > > If this feature is supported by the driver and the flag is set on
> > > > > > multiple channels, their ON times are spread out to improve EMI and
> > > > > > reduce current spikes.
> > > > > 
> > > > > As said in reply to patch 4/8 already: I don't like this idea and
> > > > > think this should be made explicit using a new offset member in struct
> > > > > pwm_state instead. That's because I think that the wave form a PWM
> > > > > generates should be (completely) defined by the consumer and not by a
> > > > > mix between consumer and device tree. Also the consumer has no (sane)
> > > > > way to determine if staggering is in use or not.
> > > > 
> > > > I don't think offsets are ideal for this feature: It makes it more
> > > > cumbersome for the user, because he has to allocate the offsets
> > > > himself instead of a simple on/off switch.
> > > > The envisioned usecase is: "I want better EMI behavior and don't care
> > > > about the individual channels no longer being asserted at the exact same
> > > > time".
> > > 
> > > The formal thing is: "I want better EMI behavior and don't care if
> > > periods start with the active phase, it might be anywhere, even over a
> > > period boundary." Being asserted at the exact same time is just a detail
> > > for the pca9685.
> > >  
> > > > > One side effect (at least for the pca9685) is that when programming a
> > > > > new duty cycle it takes a bit longer than without staggering until the
> > > > > new setting is active. 
> > > > 
> > > > Yes, but it can be turned off if this is a problem, now even per-PWM.
> > > 
> > > Yes and that is a good thing. (BTW: I'd call it per-PWM-consumer, but
> > > details.)
> > > 
> > > > > Another objection I have is that we already have some technical debt
> > > > > because there are already two different types of drivers (.apply vs
> > > > > .config+.set_polarity+.enable+.disable) and I would like to unify this
> > > > > first before introducing new stuff.
> > > > 
> > > > But there is already PWM_POLARITY_INVERTED, which can be set in the DT.
> > > > I am only adding another flag.
> > > 
> > > I understand your reasoning, and similar to "This diplay backlight needs
> > > an inverted PWM (as a low duty-cycle results in a high brightness" this
> > > semantic "This consumer doesn't care if the active cycle is anywhere in
> > > the period". Hmm, maybe I just have to think about it a bit more to
> > > become friends with that thought.
> > 
> > Yes, I think that's basically what this is saying. I think we're perhaps
> > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > the impression that we're dealing with some provider-specific feature,
> > whereas what we really want to express is that the PWM doesn't care
> > exactly when the active cycle starts and based on that a provider that
> > can support it may optimize the EMI behavior.
> > 
> > Maybe we can find a better name for this? Ultimately what this means is
> > that the consumer is primarily interested in the power output of the PWM
> > rather than the exact shape of the signal. So perhaps something like
> > PWM_USAGE_POWER would be more appropriate.
> 
> Yes, although it would then no longer be obvious that this feature leads
> to improved EMI behavior, as long as we mention that in the docs, I
> think it's a good idea
> 
> Maybe document it as follows?
> PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> for EMI improvements, as long as the power output stays the same

That's a very narrow definition of what this does, but yeah, I think the
more we document this the better.

However, the intention for naming this PWM_USAGE_POWER is to make it
clear that from the *consumer* side of things we use the PWM for its
power output and therefore we don't care about the signal actually looks
like, except that it should provide power equivalent to what the
consumer requested.

> > Come to think of it, a flag like that might even be useful to implement
> > the common case of emulating inverted polarity with reversing the duty
> > cycle. That is, if PWM_USAGE_POWER | PWM_POLARITY_INVERSED was specified
> > and the PWM provider did not support polarity inversion, the inversion
> > could still be implemented using emulation. Currently we push that logic
> > down into consumers, but this could be a way to bring that up into
> > drivers, or perhaps even the core.
> 
> Interesting, but that would be left for another series in the future, I
> assume?

Yes, of course. I was merely thinking about what could be done with this
if we describe it in this generic way.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > > Yes, I think that's basically what this is saying. I think we're perhaps
> > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > > the impression that we're dealing with some provider-specific feature,
> > > whereas what we really want to express is that the PWM doesn't care
> > > exactly when the active cycle starts and based on that a provider that
> > > can support it may optimize the EMI behavior.
> > > 
> > > Maybe we can find a better name for this? Ultimately what this means is
> > > that the consumer is primarily interested in the power output of the PWM
> > > rather than the exact shape of the signal. So perhaps something like
> > > PWM_USAGE_POWER would be more appropriate.
> > 
> > Yes, although it would then no longer be obvious that this feature leads
> > to improved EMI behavior, as long as we mention that in the docs, I
> > think it's a good idea
> > 
> > Maybe document it as follows?
> > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> > for EMI improvements, as long as the power output stays the same
> 
> I don't like both names, because for someone who is only halfway into
> PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?

Heh... how's that any more understandable?

> When a consumer is only interested in the power output than
> 
> 	.period = 20
> 	.duty_cycle = 5
> 
> would also be an allowed response for the request
> 
> 	.period = 200
> 	.duty_cycle = 50
> 
> and this is not what is in the focus here.

Actually, that's *exactly* what's important here. From a consumer point
of view the output power is the key in this case. The specifier is a
description of a particular PWM in the consumer context. And the
consumer not going to care what exactly the PWM controller might end up
configuring to achieve best results. If the controller allows the phase
shift to be changed and the constraints allow it, then that's great, but
it isn't something that the consumer has to know if all it wants is that
the power output is as requested.

Put another way, the more generically we can describe the constraints or
use cases, the more flexibility we get for drivers to fulfill those
constraints. For example one controller might support phase shifting and
use that for PWM_USAGE_POWER for better EMI behaviour. But another PWM
controller may not support it. But it could perhaps want to optimize the
PWM signal by reversing the polarity of one channel or whatever other
mechanism there may be.

If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that
support programmable phase shift will be able to support this. If some
other mechanism can also be used to support "equivalent power" use
cases, that would have to be described as some other flag, which has
essentially the same meaning. So you can get into a situation where you
have multiple flags used for the same thing.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

On Wed, Apr 07, 2021 at 07:33:57AM +0200, Uwe Kleine-König wrote:
> On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote:
> > Add the flag and corresponding documentation for the new PWM staggering
> > mode feature.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> 
> For the record, I don't like this and still prefer to make this
> staggering explicit for the consumer by expanding struct pwm_state with
> an .offset member to shift the active phase in the period.

How are consumers supposed to know which offset to choose? And worse:
how should consumers even know that the driver supports phase shifts?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 3/8] pwm: pca9685: Improve runtime PM behavior

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2139 bytes --]

On Tue, Apr 06, 2021 at 06:41:35PM +0200, Clemens Gruber wrote:
> The chip does not come out of POR in active state but in sleep state.
> To be sure (in case the bootloader woke it up) we force it to sleep in
> probe.
> 
> On kernels without CONFIG_PM, we wake the chip in .probe and put it to
> sleep in .remove.
> 
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> Changes since v6:
> - Improved !CONFIG_PM handling (wake it up without putting it to sleep
>   first)
> 
>  drivers/pwm/pwm-pca9685.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index d4474c5ff96f..0bcec04b138a 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -474,13 +474,18 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	/* The chip comes out of power-up in the active state */
> -	pm_runtime_set_active(&client->dev);
> -	/*
> -	 * Enable will put the chip into suspend, which is what we
> -	 * want as all outputs are disabled at this point
> -	 */
> -	pm_runtime_enable(&client->dev);
> +	if (IS_ENABLED(CONFIG_PM)) {

This looks odd to me. I've seen similar constructs, but they usually go
something like this (I think):

	pm_runtime_enable(&client->dev);

	if (!pm_runtime_enabled(&client->dev)) {
		/* resume device */
	}

Which I guess in your would be somewhat the opposite and it wouldn't
actually resume the device but rather put it to sleep.

Perhaps something like this:

	pm_runtime_enable(&client->dev);

	if (pm_runtime_enabled(&client->dev)) {
		pca9685_set_sleep_mode(pca, true);
		pm_runtime_set_suspended(&client->dev);
	} else {
		/* wake the chip up on non-PM environments */
		pca9685_set_sleep_mode(pca, false);
	}

? I think that's slightly more correct than your original because it
takes into account things like sysfs power control and such. It also
doesn't rely on the config option alone but instead uses the runtime
PM API to achieve this more transparently.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Clemens Gruber]

On Fri, Apr 09, 2021 at 01:25:36PM +0200, Thierry Reding wrote:
> On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> > > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > > > Yes, I think that's basically what this is saying. I think we're perhaps
> > > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > > > the impression that we're dealing with some provider-specific feature,
> > > > whereas what we really want to express is that the PWM doesn't care
> > > > exactly when the active cycle starts and based on that a provider that
> > > > can support it may optimize the EMI behavior.
> > > > 
> > > > Maybe we can find a better name for this? Ultimately what this means is
> > > > that the consumer is primarily interested in the power output of the PWM
> > > > rather than the exact shape of the signal. So perhaps something like
> > > > PWM_USAGE_POWER would be more appropriate.
> > > 
> > > Yes, although it would then no longer be obvious that this feature leads
> > > to improved EMI behavior, as long as we mention that in the docs, I
> > > think it's a good idea
> > > 
> > > Maybe document it as follows?
> > > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> > > for EMI improvements, as long as the power output stays the same
> > 
> > I don't like both names, because for someone who is only halfway into
> > PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?
> 
> Heh... how's that any more understandable?
> 
> > When a consumer is only interested in the power output than
> > 
> > 	.period = 20
> > 	.duty_cycle = 5
> > 
> > would also be an allowed response for the request
> > 
> > 	.period = 200
> > 	.duty_cycle = 50
> > 
> > and this is not what is in the focus here.
> 
> Actually, that's *exactly* what's important here. From a consumer point
> of view the output power is the key in this case. The specifier is a
> description of a particular PWM in the consumer context. And the
> consumer not going to care what exactly the PWM controller might end up
> configuring to achieve best results. If the controller allows the phase
> shift to be changed and the constraints allow it, then that's great, but
> it isn't something that the consumer has to know if all it wants is that
> the power output is as requested.
> 
> Put another way, the more generically we can describe the constraints or
> use cases, the more flexibility we get for drivers to fulfill those
> constraints. For example one controller might support phase shifting and
> use that for PWM_USAGE_POWER for better EMI behaviour. But another PWM
> controller may not support it. But it could perhaps want to optimize the
> PWM signal by reversing the polarity of one channel or whatever other
> mechanism there may be.
> 
> If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that
> support programmable phase shift will be able to support this. If some
> other mechanism can also be used to support "equivalent power" use
> cases, that would have to be described as some other flag, which has
> essentially the same meaning. So you can get into a situation where you
> have multiple flags used for the same thing.

I see what you mean. We have more flexibility with PWM_USAGE_POWER. The
only downside is that there is no real connection to the improved EMI
but I guess that's what documentation is for.

I will try to document it as follows:
- PWM_USAGE_POWER - Only care about the power output of the signal. This
  allows drivers (if supported) to optimize the signals, for example to
  improve EMI and reduce current spikes.

Maybe I then add a comment describing the specific optimization in the
pca9685 code, maybe like this:
If PWM_USAGE_POWER is set on a PWM, the pca9685 driver will phase shift
the individual channels relative to their channel number. This improves
EMI because the enabled channels no longer turn on at the same time,
while still maintaining the configured duty cycle.

Thanks,
Clemens

Re: [PATCH v7 3/8] pwm: pca9685: Improve runtime PM behavior

[Clemens Gruber]

On Fri, Apr 09, 2021 at 03:03:20PM +0200, Thierry Reding wrote:
> On Tue, Apr 06, 2021 at 06:41:35PM +0200, Clemens Gruber wrote:
> > The chip does not come out of POR in active state but in sleep state.
> > To be sure (in case the bootloader woke it up) we force it to sleep in
> > probe.
> > 
> > On kernels without CONFIG_PM, we wake the chip in .probe and put it to
> > sleep in .remove.
> > 
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > Changes since v6:
> > - Improved !CONFIG_PM handling (wake it up without putting it to sleep
> >   first)
> > 
> >  drivers/pwm/pwm-pca9685.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index d4474c5ff96f..0bcec04b138a 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -474,13 +474,18 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > -	/* The chip comes out of power-up in the active state */
> > -	pm_runtime_set_active(&client->dev);
> > -	/*
> > -	 * Enable will put the chip into suspend, which is what we
> > -	 * want as all outputs are disabled at this point
> > -	 */
> > -	pm_runtime_enable(&client->dev);
> > +	if (IS_ENABLED(CONFIG_PM)) {
> 
> This looks odd to me. I've seen similar constructs, but they usually go
> something like this (I think):
> 
> 	pm_runtime_enable(&client->dev);
> 
> 	if (!pm_runtime_enabled(&client->dev)) {
> 		/* resume device */
> 	}
> 
> Which I guess in your would be somewhat the opposite and it wouldn't
> actually resume the device but rather put it to sleep.

Yes, I wanted to keep it in sleep mode if runtime PM is supported (to be
woken up later) and otherwise just wake it up in probe.

> 
> Perhaps something like this:
> 
> 	pm_runtime_enable(&client->dev);
> 
> 	if (pm_runtime_enabled(&client->dev)) {
> 		pca9685_set_sleep_mode(pca, true);
> 		pm_runtime_set_suspended(&client->dev);
> 	} else {
> 		/* wake the chip up on non-PM environments */
> 		pca9685_set_sleep_mode(pca, false);
> 	}
> 
> ? I think that's slightly more correct than your original because it
> takes into account things like sysfs power control and such. It also
> doesn't rely on the config option alone but instead uses the runtime
> PM API to achieve this more transparently.

Ah, yes, I missed the fact that runtime could be disabled 'at runtime'
via sysfs as well, so yes, that's more correct and pm_runtime_enabled
will just return false if !CONFIG_PM, so that should work as well.

Thanks,
Clemens

Re: [PATCH v7 5/8] pwm: core: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 5978 bytes --]

Hello Thierry,

On Fri, Apr 09, 2021 at 01:25:36PM +0200, Thierry Reding wrote:
> On Thu, Apr 08, 2021 at 07:36:37PM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 08, 2021 at 05:51:36PM +0200, Clemens Gruber wrote:
> > > On Thu, Apr 08, 2021 at 02:50:40PM +0200, Thierry Reding wrote:
> > > > Yes, I think that's basically what this is saying. I think we're perhaps
> > > > getting hung up on the terminology here. PWM_STAGGERING_ALLOWED gives
> > > > the impression that we're dealing with some provider-specific feature,
> > > > whereas what we really want to express is that the PWM doesn't care
> > > > exactly when the active cycle starts and based on that a provider that
> > > > can support it may optimize the EMI behavior.
> > > > 
> > > > Maybe we can find a better name for this? Ultimately what this means is
> > > > that the consumer is primarily interested in the power output of the PWM
> > > > rather than the exact shape of the signal. So perhaps something like
> > > > PWM_USAGE_POWER would be more appropriate.
> > > 
> > > Yes, although it would then no longer be obvious that this feature leads
> > > to improved EMI behavior, as long as we mention that in the docs, I
> > > think it's a good idea
> > > 
> > > Maybe document it as follows?
> > > PWM_USAGE_POWER - Allow the driver to delay the start of the cycle
> > > for EMI improvements, as long as the power output stays the same
> > 
> > I don't like both names, because for someone who is only halfway into
> > PWM stuff it is not understandable. Maybe ALLOW_PHASE_SHIFT?
> 
> Heh... how's that any more understandable?

The questions that come to (my) mind when reading PWM_USAGE_POWER are:
So the PWM is allowed to use some power? The PWM is used to provide a
power source (like a regulator)? Has this something to do with a
permission to use the PWM (the power to use it)?

Please try googling for "usage power", and compare it with the results
you get for "phase shift".

> > When a consumer is only interested in the power output than
> > 
> > 	.period = 20
> > 	.duty_cycle = 5
> > 
> > would also be an allowed response for the request
> > 
> > 	.period = 200
> > 	.duty_cycle = 50
> > 
> > and this is not what is in the focus here.
> 
> Actually, that's *exactly* what's important here. From a consumer point
> of view the output power is the key in this case.

OK, so if I understand you correctly, you want indeed allow

 	.period = 200
 	.duty_cycle = 50

when

	.period = 20
	.duty_cycle = 5

was requested? Do you want also allow .period = 20000 + .duty_cycle =
5000? How would you limit what is allowed? I'd expect we don't want to
allow .period = 20000000000 + .duty_cycle = 5000000000? What should a
driver for a PWM backlight pass to pwm_apply_state if the PWM period
should be between 4000000 ns and 16666666 ns? (This comes from the first
backlight datasheet I found where the valid range for the brightness
input is 60 to 250Hz.) Maybe saying that only making the period smaller
would be an idea; but the motor bridge I recently worked with[1] limits
the PWM frequency to "up to 20 kHz", so this isn't an universally good
idea either.

[1] https://www.st.com/resource/en/datasheet/vnh5019a-e.pdf

> The specifier is a description of a particular PWM in the consumer
> context. And the consumer not going to care what exactly the PWM
> controller might end up configuring to achieve best results. If the
> controller allows the phase shift to be changed and the constraints
> allow it, then that's great, but it isn't something that the consumer
> has to know if all it wants is that the power output is as requested.

Yes, if ALLOW_PHASE_SHIFT isn't what the consumer actually wants, they
shouldn't use it. Agreed.

> Put another way, the more generically we can describe the constraints
> or use cases, the more flexibility we get for drivers to fulfill those
> constraints.

Yes, from the POV of a lowlevel driver the more general the better. From
the POV of a consumer this isn't universally true, because the consumer
might only accept a subset of the freedom this general flag gives to the
lowlevel driver.

> For example one controller might support phase shifting
> and use that for PWM_USAGE_POWER for better EMI behaviour. But another
> PWM controller may not support it. But it could perhaps want to
> optimize the PWM signal by reversing the polarity of one channel or
> whatever other mechanism there may be.
> 
> If we add a flag such as ALLOW_PHASE_SHIFT, then only controllers that
> support programmable phase shift will be able to support this.

That's wrong, drivers that support the polarity that was not requested
can make use of it, too. That's something you already pointed out
yourself (or I misunderstood you). (Then

	.period = X
	.duty_cycle = Y
	.polarity = PWM_POLARITY_NORMAL

is "equivalent" to

	.period = X
	.duty_cycle = X - Y
	.polarity = PWM_POLARITY_INVERSED

as the signals only differ by a phase shift.)

> If some other mechanism can also be used to support "equivalent power"
> use cases, that would have to be described as some other flag, which
> has essentially the same meaning. So you can get into a situation
> where you have multiple flags used for the same thing.

If they are for the same thing, you don't need another flag. Your
concern is only valid if all consumers that are ok to accept a phase
shifted PWM signal really only care about the relative duty cycle. I
think that's wrong. (All consumers that use the PWM as something like a
clock signal where something happens on the rising and/or falling edge
and want this something happen with a certain frequency care about the
period, but not the phase shift.)

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1365 bytes --]

Hello Thierry,

On Fri, Apr 09, 2021 at 02:27:34PM +0200, Thierry Reding wrote:
> On Wed, Apr 07, 2021 at 07:33:57AM +0200, Uwe Kleine-König wrote:
> > On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote:
> > > Add the flag and corresponding documentation for the new PWM staggering
> > > mode feature.
> > > 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > 
> > For the record, I don't like this and still prefer to make this
> > staggering explicit for the consumer by expanding struct pwm_state with
> > an .offset member to shift the active phase in the period.
> 
> How are consumers supposed to know which offset to choose? And worse:
> how should consumers even know that the driver supports phase shifts?

I'm aware that we're a long way from being able to use this. The clean
approach would be to get the offset from the device tree in the same way
as the period. And in the meantime I agree that introducing a flag that
allows to shift the active part in the period is a sane idea. So I
suggest we concentrate on getting the details in the corresponding
discussion straight.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v7 4/8] dt-bindings: pwm: Support new PWM_STAGGERING_ALLOWED flag

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 492 bytes --]

Hello Rob,

On Tue, Apr 06, 2021 at 06:41:36PM +0200, Clemens Gruber wrote:
> Add the flag and corresponding documentation for the new PWM staggering
> mode feature.
> 
> Cc: Rob Herring <robh+dt@kernel.org>

For now reviewing this patch is not necessary, we're discussing a better
name for this flag.

Best regards
Uwe

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]