All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
@ 2020-12-07 19:36 Clemens Gruber
  2020-12-07 19:36 ` [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
                   ` (4 more replies)
  0 siblings, 5 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 19:36 UTC (permalink / raw)
  To: linux-pwm
  Cc: Thierry Reding, u.kleine-koenig, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander, 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
- The prescale register is now set to the default value in probe. On
  systems without CONFIG_PM, the chip is woken up at probe time.

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

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

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

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..0425e0bcb81e 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -47,11 +47,11 @@
 #define PCA9685_ALL_LED_OFF_H	0xFD
 #define PCA9685_PRESCALE	0xFE
 
+#define PCA9685_PRESCALE_DEF	0x1E	/* => default frequency of ~200 Hz */
 #define PCA9685_PRESCALE_MIN	0x03	/* => max. frequency of 1526 Hz */
 #define PCA9685_PRESCALE_MAX	0xFF	/* => min. frequency of 24 Hz */
 
 #define PCA9685_COUNTER_RANGE	4096
-#define PCA9685_DEFAULT_PERIOD	5000000	/* Default period_ns = 1/200 Hz */
 #define PCA9685_OSC_CLOCK_MHZ	25	/* Internal oscillator with 25 MHz */
 
 #define PCA9685_NUMREGS		0xFF
@@ -74,7 +74,6 @@
 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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
 	return container_of(chip, struct pca9685, chip);
 }
 
+static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable)
+{
+	unsigned int val = enable ? LED_FULL : 0;
+
+	/* Note: The full OFF bit has the highest precedence */
+
+	if (index >= PCA9685_MAXCHAN) {
+		regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val);
+		return;
+	}
+	regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val);
+}
+
+static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index)
+{
+	unsigned int val = 0;
+
+	if (index >= PCA9685_MAXCHAN)
+		return false;
+
+	regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
+	return val & LED_FULL;
+}
+
+static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable)
+{
+	unsigned int val = enable ? LED_FULL : 0;
+
+	if (index >= PCA9685_MAXCHAN) {
+		regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val);
+		return;
+	}
+	regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val);
+}
+
+static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index)
+{
+	unsigned int val = 0;
+
+	if (index >= PCA9685_MAXCHAN)
+		return false;
+
+	regmap_read(pca->regmap, LED_N_ON_H(index), &val);
+	return val & LED_FULL;
+}
+
+static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off)
+{
+	int reg;
+
+	/* Note: This function also clears the full OFF bit */
+
+	reg = index >= PCA9685_MAXCHAN ?
+		PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index);
+	regmap_write(pca->regmap, reg, off & 0xff);
+
+	reg = index >= PCA9685_MAXCHAN ?
+		PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index);
+	regmap_write(pca->regmap, reg, (off >> 8) & 0xf);
+}
+
+static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index)
+{
+	unsigned int off, val = 0;
+
+	if (index >= PCA9685_MAXCHAN)
+		return 0;
+
+	regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
+	off = (val & 0xf) << 8;
+
+	regmap_read(pca->regmap, LED_N_OFF_L(index), &val);
+	return off | (val & 0xff);
+}
+
 #if IS_ENABLED(CONFIG_GPIOLIB)
 static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
 {
@@ -138,34 +212,31 @@ 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_is_full_off(pca, offset);
 }
 
 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);
+	if (value) {
+		pca9685_pwm_set_full_on(pca, offset, true);
+		/* Clear full OFF bit last */
+		pca9685_pwm_set_full_off(pca, offset, false);
+	} else {
+		/* Set full OFF bit first */
+		pca9685_pwm_set_full_off(pca, offset, true);
+		pca9685_pwm_set_full_on(pca, offset, false);
+	}
 }
 
 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_full_off(pca, offset, true);
 	pm_runtime_put(pca->chip.dev);
 	pca9685_pwm_clear_inuse(pca, offset);
 }
@@ -246,165 +317,98 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
 	}
 }
 
-static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			      int duty_ns, int period_ns)
+static void pca9685_pwm_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 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 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);
+	/* Calculate (chip-wide) period from prescale value */
+	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+	state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+			(val + 1);
 
-		regmap_write(pca->regmap, reg, LED_FULL);
+	/* The (per-channel) polarity is fixed */
+	state->polarity = PWM_POLARITY_NORMAL;
 
-		return 0;
+	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;
 	}
 
-	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);
+	state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm);
 
-		regmap_write(pca->regmap, reg, LED_FULL);
-
-		return 0;
+	if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) {
+		state->duty_cycle = state->period;
+		return;
 	}
 
-	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;
+	duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period;
+	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
 }
 
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+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 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);
+	unsigned long long duty, prescale;
+	unsigned int val = 0;
 
-	regmap_write(pca->regmap, reg, 0);
+	if (state->polarity != PWM_POLARITY_NORMAL)
+		return -EOPNOTSUPP;
 
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_ON_H;
-	else
-		reg = LED_N_ON_H(pwm->hwpwm);
-
-	regmap_write(pca->regmap, reg, 0);
-
-	/*
-	 * Clear the full-off bit.
-	 * It has precedence over the others and must be off.
-	 */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
+	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, "prescaler not set: period out of bounds!\n");
+		return -EINVAL;
+	}
 
-	regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+	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);
 
-	return 0;
-}
+		/* Change the chip-wide output frequency */
+		regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
 
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct pca9685 *pca = to_pca(chip);
-	unsigned int reg;
+		/* Wake the chip up */
+		pca9685_set_sleep_mode(pca, false);
+	}
 
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_H;
-	else
-		reg = LED_N_OFF_H(pwm->hwpwm);
+	duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
+	duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
 
-	regmap_write(pca->regmap, reg, LED_FULL);
+	if (!state->enabled || duty < 1) {
+		/* Set full OFF bit first */
+		pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
+		pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
+		return 0;
+	}
 
-	/* Clear the LED_OFF counter. */
-	if (pwm->hwpwm >= PCA9685_MAXCHAN)
-		reg = PCA9685_ALL_LED_OFF_L;
-	else
-		reg = LED_N_OFF_L(pwm->hwpwm);
+	if (duty == PCA9685_COUNTER_RANGE) {
+		pca9685_pwm_set_full_on(pca, pwm->hwpwm, true);
+		/* Clear full OFF bit last */
+		pca9685_pwm_set_full_off(pca, pwm->hwpwm, false);
+		return 0;
+	}
 
-	regmap_write(pca->regmap, reg, 0x0);
+	pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty);
+	/* Clear full ON bit after OFF time was set */
+	pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
+	return 0;
 }
 
 static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -422,15 +426,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pca9685 *pca = to_pca(chip);
 
-	pca9685_pwm_disable(chip, pwm);
+	pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
 	pm_runtime_put(chip->dev);
 	pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
 }
 
 static const struct pwm_ops pca9685_pwm_ops = {
-	.enable = pca9685_pwm_enable,
-	.disable = pca9685_pwm_disable,
-	.config = pca9685_pwm_config,
+	.get_state = pca9685_pwm_get_state,
+	.apply = pca9685_pwm_apply,
 	.request = pca9685_pwm_request,
 	.free = pca9685_pwm_free,
 	.owner = THIS_MODULE,
@@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
-	pca->period_ns = PCA9685_DEFAULT_PERIOD;
 
 	i2c_set_clientdata(client, pca);
 
@@ -505,14 +507,20 @@ 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
+	 * Always initialize with default prescale, but chip must be
+	 * in sleep mode while changing prescaler.
 	 */
+	pca9685_set_sleep_mode(pca, true);
+	regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF);
+	pm_runtime_set_suspended(&client->dev);
 	pm_runtime_enable(&client->dev);
 
+	if (!IS_ENABLED(CONFIG_PM)) {
+		/* Wake the chip up on non-PM environments */
+		pca9685_set_sleep_mode(pca, false);
+	}
+
 	return 0;
 }
 
@@ -524,7 +532,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.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe
  2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
@ 2020-12-07 19:36 ` Clemens Gruber
  2020-12-07 19:36 ` [PATCH v4 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 19:36 UTC (permalink / raw)
  To: linux-pwm
  Cc: Thierry Reding, u.kleine-koenig, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander, Clemens Gruber

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

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

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

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

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

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 0425e0bcb81e..fdce5fb9f41e 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -486,9 +486,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
 	regmap_write(pca->regmap, PCA9685_MODE1, reg);
 
-	/* Clear all "full off" bits */
+	/* Reset OFF registers to HW default (only full OFF bit is set) */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
-	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* Add an extra channel for ALL_LED */
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v4 3/4] pwm: pca9685: Support staggered output ON times
  2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
  2020-12-07 19:36 ` [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
@ 2020-12-07 19:36 ` Clemens Gruber
  2020-12-07 19:38 ` [PATCH v4 4/4] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 19:36 UTC (permalink / raw)
  To: linux-pwm
  Cc: Thierry Reding, u.kleine-koenig, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander, Clemens Gruber

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

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v3:
- Refactoring: Extracted common functions
- Fixed error in .get_state
- Added vendor prefix to DT property

Changes since v1:
- Rebased

 drivers/pwm/pwm-pca9685.c | 72 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index fdce5fb9f41e..2697138ee95a 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -74,6 +74,7 @@
 struct pca9685 {
 	struct pwm_chip chip;
 	struct regmap *regmap;
+	bool staggered_outputs;
 #if IS_ENABLED(CONFIG_GPIOLIB)
 	struct mutex lock;
 	struct gpio_chip gpio;
@@ -161,6 +162,35 @@ static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index)
 	return off | (val & 0xff);
 }
 
+static void pca9685_pwm_set_on_time(struct pca9685 *pca, int index, unsigned int on)
+{
+	int reg;
+
+	/* Note: This function also clears the full ON bit */
+
+	reg = index >= PCA9685_MAXCHAN ?
+		PCA9685_ALL_LED_ON_L : LED_N_ON_L(index);
+	regmap_write(pca->regmap, reg, on & 0xff);
+
+	reg = index >= PCA9685_MAXCHAN ?
+		PCA9685_ALL_LED_ON_H : LED_N_ON_H(index);
+	regmap_write(pca->regmap, reg, (on >> 8) & 0xf);
+}
+
+static unsigned int pca9685_pwm_on_time(struct pca9685 *pca, int index)
+{
+	unsigned int on, val = 0;
+
+	if (index >= PCA9685_MAXCHAN)
+		return 0;
+
+	regmap_read(pca->regmap, LED_N_ON_H(index), &val);
+	on = (val & 0xf) << 8;
+
+	regmap_read(pca->regmap, LED_N_ON_L(index), &val);
+	return on | (val & 0xff);
+}
+
 #if IS_ENABLED(CONFIG_GPIOLIB)
 static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
 {
@@ -322,7 +352,7 @@ static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty;
-	unsigned int val = 0;
+	unsigned int on, off, val = 0;
 
 	/* Calculate (chip-wide) period from prescale value */
 	regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
@@ -349,7 +379,19 @@ static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		return;
 	}
 
-	duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period;
+	off = pca9685_pwm_off_time(pca, pwm->hwpwm);
+
+	if (pca->staggered_outputs) {
+		on = pca9685_pwm_on_time(pca, pwm->hwpwm);
+
+		if (off >= on)
+			duty = off - on;
+		else
+			duty = off + PCA9685_COUNTER_RANGE - on;
+	} else
+		duty = off;
+
+	duty *= state->period;
 	state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
 }
 
@@ -358,7 +400,7 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct pca9685 *pca = to_pca(chip);
 	unsigned long long duty, prescale;
-	unsigned int val = 0;
+	unsigned int on, off, val = 0;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
 		return -EOPNOTSUPP;
@@ -405,6 +447,24 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return 0;
 	}
 
+	if (pca->staggered_outputs) {
+		if (pwm->hwpwm < PCA9685_MAXCHAN) {
+			/*
+			 * To reduce EMI, the ON times of each channel are
+			 * spread out evenly within the counter range, while
+			 * still maintaining the configured duty cycle
+			 */
+			on = pwm->hwpwm * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
+			off = (on + duty) % PCA9685_COUNTER_RANGE;
+			pca9685_pwm_set_on_time(pca, pwm->hwpwm, on);
+			pca9685_pwm_set_off_time(pca, pwm->hwpwm, off);
+			return 0;
+		}
+
+		/* No staggering possible if "all LEDs" channel is used */
+		pca9685_pwm_set_on_time(pca, PCA9685_MAXCHAN, 0);
+	}
+
 	pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty);
 	/* Clear full ON bit after OFF time was set */
 	pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
@@ -481,6 +541,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 
 	regmap_write(pca->regmap, PCA9685_MODE2, reg);
 
+	pca->staggered_outputs = device_property_read_bool(
+		&client->dev, "nxp,staggered-outputs");
+
 	/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
 	regmap_read(pca->regmap, PCA9685_MODE1, &reg);
 	reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
@@ -489,6 +552,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
 	/* Reset OFF registers to HW default (only full OFF bit is set) */
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
 	regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
+	/* Reset ON registers to HW default */
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+	regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
 
 	pca->chip.ops = &pca9685_pwm_ops;
 	/* Add an extra channel for ALL_LED */
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH v4 4/4] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property
  2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
  2020-12-07 19:36 ` [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
  2020-12-07 19:36 ` [PATCH v4 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
@ 2020-12-07 19:38 ` Clemens Gruber
  2020-12-07 22:00 ` [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Uwe Kleine-König
  2020-12-07 23:22 ` Sven Van Asbroeck
  4 siblings, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 19:38 UTC (permalink / raw)
  To: linux-pwm
  Cc: Thierry Reding, u.kleine-koenig, Lee Jones, linux-kernel, Clemens Gruber

The pca9685 driver supports a new nxp,staggered-outputs property for
reduced current surges and EMI. This adds documentation for the new DT
property.

Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v1:
- Added vendor prefix

 Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
index f21b55c95738..fafe954369dc 100644
--- a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
@@ -12,6 +12,8 @@ Optional properties:
   - invert (bool): boolean to enable inverted logic
   - open-drain (bool): boolean to configure outputs with open-drain structure;
 		       if omitted use totem-pole structure
+  - nxp,staggered-outputs (bool): boolean to enable staggered output ON times to
+				  minimize current surges and EMI
 
 Example:
 
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
                   ` (2 preceding siblings ...)
  2020-12-07 19:38 ` [PATCH v4 4/4] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
@ 2020-12-07 22:00 ` Uwe Kleine-König
  2020-12-07 22:34   ` Sven Van Asbroeck
  2020-12-07 23:13   ` Clemens Gruber
  2020-12-07 23:22 ` Sven Van Asbroeck
  4 siblings, 2 replies; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-07 22:00 UTC (permalink / raw)
  To: Clemens Gruber
  Cc: linux-pwm, Thierry Reding, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander

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

On Mon, Dec 07, 2020 at 08:36:27PM +0100, 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
> - The prescale register is now set to the default value in probe. On
>   systems without CONFIG_PM, the chip is woken up at probe time.
> 
> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values. If one channel is reconfigured with new duty
> cycle and period, the others will keep the same relative duty cycle to
> period ratio as they had before, even though the per-chip / global
> frequency changed. (The PCA9685 has only one prescaler!)

This is not acceptable, if you have two PWM outputs and a consumer
modifies one of them the other must change. So if this chip only
supports a single period length of all channels, the first consumer
enabling a channel defines the period to be used. All later consumers
must live with that. (Also the first must be denied modifying the period
if a second consumer has enabled its PWM.)

> 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.

That means the datasheet is lax because 5079040ns corresponds to
196.88760080645162 Hz but it calls that 200 Hz, right?

I didn't look in the patch in detail, but get the impression it is more
complicated than necessary. For example adding improved PM behaviour
should probably go into a separate patch, also adding the .get_state
callback should be split out.

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 22:00 ` [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Uwe Kleine-König
@ 2020-12-07 22:34   ` Sven Van Asbroeck
  2020-12-07 23:24     ` Clemens Gruber
  2020-12-08  9:17     ` Uwe Kleine-König
  2020-12-07 23:13   ` Clemens Gruber
  1 sibling, 2 replies; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-07 22:34 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Clemens Gruber, linux-pwm, Thierry Reding, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi Uwe,

On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> This is not acceptable, if you have two PWM outputs and a consumer
> modifies one of them the other must change. So if this chip only
> supports a single period length of all channels, the first consumer
> enabling a channel defines the period to be used. All later consumers
> must live with that. (Also the first must be denied modifying the period
> if a second consumer has enabled its PWM.)

That makes sense. However, a possible wrinkle: when more than one pwm channel
is requested, which one is able to change the period?

Example:
1. start with all pwms free
2. pwm_request(0), pwm_apply(period=200Hz)
3. pwm_request(1)
4. pwm_apply(1, period=400Hz) fails?
5. pwm_apply(0, period=400Hz) succeeds?

And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz,
because the pwm core doesn't realize anything has changed. Are you ok
with this behaviour?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 22:00 ` [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Uwe Kleine-König
  2020-12-07 22:34   ` Sven Van Asbroeck
@ 2020-12-07 23:13   ` Clemens Gruber
  2020-12-08  9:10     ` Uwe Kleine-König
  1 sibling, 1 reply; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 23:13 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-pwm, Thierry Reding, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander

On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote:
> On Mon, Dec 07, 2020 at 08:36:27PM +0100, 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
> > - The prescale register is now set to the default value in probe. On
> >   systems without CONFIG_PM, the chip is woken up at probe time.
> > 
> > The hardware readout may return slightly different values than those
> > that were set in apply due to the limited range of possible prescale and
> > counter register values. If one channel is reconfigured with new duty
> > cycle and period, the others will keep the same relative duty cycle to
> > period ratio as they had before, even though the per-chip / global
> > frequency changed. (The PCA9685 has only one prescaler!)
> 
> This is not acceptable, if you have two PWM outputs and a consumer
> modifies one of them the other must change. So if this chip only
> supports a single period length of all channels, the first consumer
> enabling a channel defines the period to be used. All later consumers
> must live with that. (Also the first must be denied modifying the period
> if a second consumer has enabled its PWM.)

Good idea, but is it OK to potentially break users relying on the old
behavior ("the last one who changes the period wins") ?

> 
> > 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.
> 
> That means the datasheet is lax because 5079040ns corresponds to
> 196.88760080645162 Hz but it calls that 200 Hz, right?

Yes, it calls prescale setting 0x1E 200 Hz, but calculating the
period from that prescaler setting leads to 5079040ns (196.9 Hz) as you
mentioned.
Also, the datasheet does not specify frequency accuracy / internal
oscillator specifications. I measured about 207 Hz on one chip and about
205 Hz on another with the scope today, when configuring a 5079040ns
period.

> 
> I didn't look in the patch in detail, but get the impression it is more
> complicated than necessary. For example adding improved PM behaviour
> should probably go into a separate patch, also adding the .get_state
> callback should be split out.

Agreed. I'll split it up more in the next revision!

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
                   ` (3 preceding siblings ...)
  2020-12-07 22:00 ` [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Uwe Kleine-König
@ 2020-12-07 23:22 ` Sven Van Asbroeck
  2020-12-07 23:56   ` Clemens Gruber
  4 siblings, 1 reply; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-07 23:22 UTC (permalink / raw)
  To: Clemens Gruber
  Cc: linux-pwm, Thierry Reding, Uwe Kleine-König, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi Clemens, see below.

On Mon, Dec 7, 2020 at 2:37 PM Clemens Gruber
<clemens.gruber@pqgruber.com> 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
> - The prescale register is now set to the default value in probe. On
>   systems without CONFIG_PM, the chip is woken up at probe time.
>
> The hardware readout may return slightly different values than those
> that were set in apply due to the limited range of possible prescale and
> counter register values. If one channel is reconfigured with new duty
> cycle and period, the others will keep the same relative duty cycle to
> period ratio as they had before, even though the per-chip / global
> frequency changed. (The PCA9685 has only one prescaler!)
>
> Note that although the datasheet mentions 200 Hz as default frequency
> when using the internal 25 MHz oscillator, the calculated period from
> the default prescaler register setting of 30 is 5079040ns.
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> 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 | 335 ++++++++++++++++++++------------------
>  1 file changed, 175 insertions(+), 160 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..0425e0bcb81e 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -47,11 +47,11 @@
>  #define PCA9685_ALL_LED_OFF_H  0xFD
>  #define PCA9685_PRESCALE       0xFE
>
> +#define PCA9685_PRESCALE_DEF   0x1E    /* => default frequency of ~200 Hz */
>  #define PCA9685_PRESCALE_MIN   0x03    /* => max. frequency of 1526 Hz */
>  #define PCA9685_PRESCALE_MAX   0xFF    /* => min. frequency of 24 Hz */
>
>  #define PCA9685_COUNTER_RANGE  4096
> -#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
>  #define PCA9685_OSC_CLOCK_MHZ  25      /* Internal oscillator with 25 MHz */
>
>  #define PCA9685_NUMREGS                0xFF
> @@ -74,7 +74,6 @@
>  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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
>         return container_of(chip, struct pca9685, chip);
>  }
>
> +static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable)
> +{
> +       unsigned int val = enable ? LED_FULL : 0;
> +
> +       /* Note: The full OFF bit has the highest precedence */
> +
> +       if (index >= PCA9685_MAXCHAN) {
> +               regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val);
> +               return;
> +       }
> +       regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val);
> +}
> +
> +static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index)
> +{
> +       unsigned int val = 0;
> +
> +       if (index >= PCA9685_MAXCHAN)
> +               return false;
> +
> +       regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
> +       return val & LED_FULL;
> +}
> +
> +static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable)
> +{
> +       unsigned int val = enable ? LED_FULL : 0;
> +
> +       if (index >= PCA9685_MAXCHAN) {
> +               regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val);
> +               return;
> +       }
> +       regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val);
> +}

If the "full off" bit is set, calling pwm_set_full_on(pca, index, true)
won't actually bring the led full on, correct ?

This can be very confusing. See below for a suggestion to make this clearer.

> +
> +static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index)
> +{
> +       unsigned int val = 0;
> +
> +       if (index >= PCA9685_MAXCHAN)
> +               return false;
> +
> +       regmap_read(pca->regmap, LED_N_ON_H(index), &val);
> +       return val & LED_FULL;
> +}
> +
> +static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off)
> +{
> +       int reg;
> +
> +       /* Note: This function also clears the full OFF bit */
> +
> +       reg = index >= PCA9685_MAXCHAN ?
> +               PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index);
> +       regmap_write(pca->regmap, reg, off & 0xff);
> +
> +       reg = index >= PCA9685_MAXCHAN ?
> +               PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index);
> +       regmap_write(pca->regmap, reg, (off >> 8) & 0xf);
> +}
> +
> +static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index)
> +{
> +       unsigned int off, val = 0;
> +
> +       if (index >= PCA9685_MAXCHAN)
> +               return 0;
> +
> +       regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
> +       off = (val & 0xf) << 8;
> +
> +       regmap_read(pca->regmap, LED_N_OFF_L(index), &val);
> +       return off | (val & 0xff);
> +}
> +
>  #if IS_ENABLED(CONFIG_GPIOLIB)
>  static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
>  {
> @@ -138,34 +212,31 @@ 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_is_full_off(pca, offset);
>  }
>
>  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);
> +       if (value) {
> +               pca9685_pwm_set_full_on(pca, offset, true);
> +               /* Clear full OFF bit last */
> +               pca9685_pwm_set_full_off(pca, offset, false);
> +       } else {
> +               /* Set full OFF bit first */
> +               pca9685_pwm_set_full_off(pca, offset, true);
> +               pca9685_pwm_set_full_on(pca, offset, false);
> +       }
>  }
>
>  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_full_off(pca, offset, true);
>         pm_runtime_put(pca->chip.dev);
>         pca9685_pwm_clear_inuse(pca, offset);
>  }
> @@ -246,165 +317,98 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
>         }
>  }
>
> -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -                             int duty_ns, int period_ns)
> +static void pca9685_pwm_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 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 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);
> +       /* Calculate (chip-wide) period from prescale value */
> +       regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> +       state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> +                       (val + 1);
>
> -               regmap_write(pca->regmap, reg, LED_FULL);
> +       /* The (per-channel) polarity is fixed */
> +       state->polarity = PWM_POLARITY_NORMAL;
>
> -               return 0;
> +       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;
>         }
>
> -       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);
> +       state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm);
>
> -               regmap_write(pca->regmap, reg, LED_FULL);
> -
> -               return 0;
> +       if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) {
> +               state->duty_cycle = state->period;
> +               return;
>         }
>
> -       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;
> +       duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period;
> +       state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
>  }
>
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +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 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);
> +       unsigned long long duty, prescale;
> +       unsigned int val = 0;
>
> -       regmap_write(pca->regmap, reg, 0);
> +       if (state->polarity != PWM_POLARITY_NORMAL)
> +               return -EOPNOTSUPP;
>
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_ON_H;
> -       else
> -               reg = LED_N_ON_H(pwm->hwpwm);
> -
> -       regmap_write(pca->regmap, reg, 0);
> -
> -       /*
> -        * Clear the full-off bit.
> -        * It has precedence over the others and must be off.
> -        */
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_H;
> -       else
> -               reg = LED_N_OFF_H(pwm->hwpwm);
> +       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, "prescaler not set: period out of bounds!\n");
> +               return -EINVAL;
> +       }
>
> -       regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> +       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);
>
> -       return 0;
> -}
> +               /* Change the chip-wide output frequency */
> +               regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
>
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -       struct pca9685 *pca = to_pca(chip);
> -       unsigned int reg;
> +               /* Wake the chip up */
> +               pca9685_set_sleep_mode(pca, false);
> +       }
>
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_H;
> -       else
> -               reg = LED_N_OFF_H(pwm->hwpwm);
> +       duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> +       duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
>
> -       regmap_write(pca->regmap, reg, LED_FULL);
> +       if (!state->enabled || duty < 1) {
> +               /* Set full OFF bit first */
> +               pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
> +               pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
> +               return 0;
> +       }
>
> -       /* Clear the LED_OFF counter. */
> -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> -               reg = PCA9685_ALL_LED_OFF_L;
> -       else
> -               reg = LED_N_OFF_L(pwm->hwpwm);
> +       if (duty == PCA9685_COUNTER_RANGE) {
> +               pca9685_pwm_set_full_on(pca, pwm->hwpwm, true);
> +               /* Clear full OFF bit last */
> +               pca9685_pwm_set_full_off(pca, pwm->hwpwm, false);

I think "this bit first, this bit last" can be confusing and fragile.
It is also repeated in a few different places.
Suggestion on how to improve this, below.

> +               return 0;
> +       }
>
> -       regmap_write(pca->regmap, reg, 0x0);
> +       pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty);
> +       /* Clear full ON bit after OFF time was set */
> +       pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
> +       return 0;
>  }

I think pwm_apply() is broken in the following scenario:

1. pwm_apply(enable=false)
2. pwm_apply(enable=true, duty_cycle=50%, period=200Hz)
3. result: pwm still full off (disabled)

and that is because pwm_apply() clears the "full off" bit only when the user
requests "full on".

I attribute this to the confusing nature of this chip. It's hard to keep the
chip's complete state fully in one's head.

One possible way to keep this manageable is by writing the functions in terms of
*led state* and not in terms of the *bits* it sets:

- set_full_on() should make the led go full on. So it sets "full on"
  and clears "full off" bits.
- set_full_off() should make the led go full off. So it sets the "full off" bit.
- set_duty() brings the led into duty mode. So it should clear "full off",
  clear "full on", and write the on/off times.

And actually, all that's needed is a single function, because duty == 0 means
"full off" and duty == 4095 means "full on".

Example in pseudo code:

static void pca9685_pwm_set_duty(struct pca9685 *pca, int index,
unsigned int duty)
{
    if (duty == 0) {
        /* full off takes precedence */
        regmap_write(full_off(index), ON);
    } else if (duty >= (COUNTER_RANGE - 1)) {
        regmap_write(full_on(index), ON);
        regmap_write(full_off(index), OFF);
    } else {
        regmap_write(off_time(index), duty);
        regmap_write(full_on(index), OFF);
        regmap_write(full_off(index), OFF);
    }
}

static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int index)
{
    if (WARN_ON(index >= PCA9685_MAXCHAN)) {
        /* register not readable, should never happen */
        return 0;
    }
    if (full_off(index) is ON)
        return 0;
    else if (full_on(index) is ON)
        return COUNTER_RANGE - 1;
    return off_time(index);
}

I suspect that once all on/off register accesses go through these two functions,
things will look simpler and there will be less risk of getting it wrong.

To prevent a forest of "index >= PCA9685_MAXCHAN" checks, I suggest creating
an helper function or macro, e.g.

static unsigned int led_off_h(int index)
{
    return (index >= PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H :
LED_N_OFF_H(index);
}

then setting a register looks simple:
regmap_write(pca->regmap, led_off_h(index), LED_FULL);

>
>  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -422,15 +426,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>         struct pca9685 *pca = to_pca(chip);
>
> -       pca9685_pwm_disable(chip, pwm);
> +       pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
>         pm_runtime_put(chip->dev);
>         pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
>  }
>
>  static const struct pwm_ops pca9685_pwm_ops = {
> -       .enable = pca9685_pwm_enable,
> -       .disable = pca9685_pwm_disable,
> -       .config = pca9685_pwm_config,
> +       .get_state = pca9685_pwm_get_state,
> +       .apply = pca9685_pwm_apply,
>         .request = pca9685_pwm_request,
>         .free = pca9685_pwm_free,
>         .owner = THIS_MODULE,
> @@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>                         ret);
>                 return ret;
>         }
> -       pca->period_ns = PCA9685_DEFAULT_PERIOD;
>
>         i2c_set_clientdata(client, pca);
>
> @@ -505,14 +507,20 @@ 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
> +        * Always initialize with default prescale, but chip must be
> +        * in sleep mode while changing prescaler.
>          */
> +       pca9685_set_sleep_mode(pca, true);
> +       regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF);
> +       pm_runtime_set_suspended(&client->dev);
>         pm_runtime_enable(&client->dev);
>
> +       if (!IS_ENABLED(CONFIG_PM)) {
> +               /* Wake the chip up on non-PM environments */
> +               pca9685_set_sleep_mode(pca, false);
> +       }
> +
>         return 0;
>  }
>
> @@ -524,7 +532,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.29.2
>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 22:34   ` Sven Van Asbroeck
@ 2020-12-07 23:24     ` Clemens Gruber
  2020-12-08  9:17     ` Uwe Kleine-König
  1 sibling, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 23:24 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

On Mon, Dec 07, 2020 at 05:34:58PM -0500, Sven Van Asbroeck wrote:
> Hi Uwe,
> 
> On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > This is not acceptable, if you have two PWM outputs and a consumer
> > modifies one of them the other must change. So if this chip only
> > supports a single period length of all channels, the first consumer
> > enabling a channel defines the period to be used. All later consumers
> > must live with that. (Also the first must be denied modifying the period
> > if a second consumer has enabled its PWM.)
> 
> That makes sense. However, a possible wrinkle: when more than one pwm channel
> is requested, which one is able to change the period?
> 
> Example:
> 1. start with all pwms free
> 2. pwm_request(0), pwm_apply(period=200Hz)
> 3. pwm_request(1)
> 4. pwm_apply(1, period=400Hz) fails?
> 5. pwm_apply(0, period=400Hz) succeeds?
> 
> And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz,
> because the pwm core doesn't realize anything has changed. Are you ok
> with this behaviour?

I think we'd have to deny the pwm_apply in step 5 as well. So, only the
first consumer is allowed to change the period and only as long as it is
the only one that is in use / was requested.

But that's definitely a breaking change.

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 23:22 ` Sven Van Asbroeck
@ 2020-12-07 23:56   ` Clemens Gruber
  0 siblings, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-07 23:56 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: linux-pwm, Thierry Reding, Uwe Kleine-König, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi Sven,

On Mon, Dec 07, 2020 at 06:22:08PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, see below.
> 
> On Mon, Dec 7, 2020 at 2:37 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> 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
> > - The prescale register is now set to the default value in probe. On
> >   systems without CONFIG_PM, the chip is woken up at probe time.
> >
> > The hardware readout may return slightly different values than those
> > that were set in apply due to the limited range of possible prescale and
> > counter register values. If one channel is reconfigured with new duty
> > cycle and period, the others will keep the same relative duty cycle to
> > period ratio as they had before, even though the per-chip / global
> > frequency changed. (The PCA9685 has only one prescaler!)
> >
> > Note that although the datasheet mentions 200 Hz as default frequency
> > when using the internal 25 MHz oscillator, the calculated period from
> > the default prescaler register setting of 30 is 5079040ns.
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > 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 | 335 ++++++++++++++++++++------------------
> >  1 file changed, 175 insertions(+), 160 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..0425e0bcb81e 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -47,11 +47,11 @@
> >  #define PCA9685_ALL_LED_OFF_H  0xFD
> >  #define PCA9685_PRESCALE       0xFE
> >
> > +#define PCA9685_PRESCALE_DEF   0x1E    /* => default frequency of ~200 Hz */
> >  #define PCA9685_PRESCALE_MIN   0x03    /* => max. frequency of 1526 Hz */
> >  #define PCA9685_PRESCALE_MAX   0xFF    /* => min. frequency of 24 Hz */
> >
> >  #define PCA9685_COUNTER_RANGE  4096
> > -#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
> >  #define PCA9685_OSC_CLOCK_MHZ  25      /* Internal oscillator with 25 MHz */
> >
> >  #define PCA9685_NUMREGS                0xFF
> > @@ -74,7 +74,6 @@
> >  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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> >         return container_of(chip, struct pca9685, chip);
> >  }
> >
> > +static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable)
> > +{
> > +       unsigned int val = enable ? LED_FULL : 0;
> > +
> > +       /* Note: The full OFF bit has the highest precedence */
> > +
> > +       if (index >= PCA9685_MAXCHAN) {
> > +               regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val);
> > +               return;
> > +       }
> > +       regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val);
> > +}
> > +
> > +static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index)
> > +{
> > +       unsigned int val = 0;
> > +
> > +       if (index >= PCA9685_MAXCHAN)
> > +               return false;
> > +
> > +       regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
> > +       return val & LED_FULL;
> > +}
> > +
> > +static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable)
> > +{
> > +       unsigned int val = enable ? LED_FULL : 0;
> > +
> > +       if (index >= PCA9685_MAXCHAN) {
> > +               regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val);
> > +               return;
> > +       }
> > +       regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val);
> > +}
> 
> If the "full off" bit is set, calling pwm_set_full_on(pca, index, true)
> won't actually bring the led full on, correct ?

Correct. Although I added a comment in pwm_set_full_off to explain that
the full OFF bit has precedence and we are (un)setting the full OFF bit
when necessary, I agree that this can and should be improved to be less
confusing.

> 
> This can be very confusing. See below for a suggestion to make this clearer.
> 
> > +
> > +static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index)
> > +{
> > +       unsigned int val = 0;
> > +
> > +       if (index >= PCA9685_MAXCHAN)
> > +               return false;
> > +
> > +       regmap_read(pca->regmap, LED_N_ON_H(index), &val);
> > +       return val & LED_FULL;
> > +}
> > +
> > +static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off)
> > +{
> > +       int reg;
> > +
> > +       /* Note: This function also clears the full OFF bit */
> > +
> > +       reg = index >= PCA9685_MAXCHAN ?
> > +               PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index);
> > +       regmap_write(pca->regmap, reg, off & 0xff);
> > +
> > +       reg = index >= PCA9685_MAXCHAN ?
> > +               PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index);
> > +       regmap_write(pca->regmap, reg, (off >> 8) & 0xf);
> > +}
> > +
> > +static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index)
> > +{
> > +       unsigned int off, val = 0;
> > +
> > +       if (index >= PCA9685_MAXCHAN)
> > +               return 0;
> > +
> > +       regmap_read(pca->regmap, LED_N_OFF_H(index), &val);
> > +       off = (val & 0xf) << 8;
> > +
> > +       regmap_read(pca->regmap, LED_N_OFF_L(index), &val);
> > +       return off | (val & 0xff);
> > +}
> > +
> >  #if IS_ENABLED(CONFIG_GPIOLIB)
> >  static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> >  {
> > @@ -138,34 +212,31 @@ 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_is_full_off(pca, offset);
> >  }
> >
> >  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);
> > +       if (value) {
> > +               pca9685_pwm_set_full_on(pca, offset, true);
> > +               /* Clear full OFF bit last */
> > +               pca9685_pwm_set_full_off(pca, offset, false);
> > +       } else {
> > +               /* Set full OFF bit first */
> > +               pca9685_pwm_set_full_off(pca, offset, true);
> > +               pca9685_pwm_set_full_on(pca, offset, false);
> > +       }
> >  }
> >
> >  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_full_off(pca, offset, true);
> >         pm_runtime_put(pca->chip.dev);
> >         pca9685_pwm_clear_inuse(pca, offset);
> >  }
> > @@ -246,165 +317,98 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> >         }
> >  }
> >
> > -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > -                             int duty_ns, int period_ns)
> > +static void pca9685_pwm_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 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 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);
> > +       /* Calculate (chip-wide) period from prescale value */
> > +       regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > +       state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > +                       (val + 1);
> >
> > -               regmap_write(pca->regmap, reg, LED_FULL);
> > +       /* The (per-channel) polarity is fixed */
> > +       state->polarity = PWM_POLARITY_NORMAL;
> >
> > -               return 0;
> > +       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;
> >         }
> >
> > -       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);
> > +       state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm);
> >
> > -               regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > -               return 0;
> > +       if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) {
> > +               state->duty_cycle = state->period;
> > +               return;
> >         }
> >
> > -       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;
> > +       duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period;
> > +       state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
> >  }
> >
> > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > +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 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);
> > +       unsigned long long duty, prescale;
> > +       unsigned int val = 0;
> >
> > -       regmap_write(pca->regmap, reg, 0);
> > +       if (state->polarity != PWM_POLARITY_NORMAL)
> > +               return -EOPNOTSUPP;
> >
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_ON_H;
> > -       else
> > -               reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > -       regmap_write(pca->regmap, reg, 0);
> > -
> > -       /*
> > -        * Clear the full-off bit.
> > -        * It has precedence over the others and must be off.
> > -        */
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_H;
> > -       else
> > -               reg = LED_N_OFF_H(pwm->hwpwm);
> > +       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, "prescaler not set: period out of bounds!\n");
> > +               return -EINVAL;
> > +       }
> >
> > -       regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > +       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);
> >
> > -       return 0;
> > -}
> > +               /* Change the chip-wide output frequency */
> > +               regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> >
> > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > -       struct pca9685 *pca = to_pca(chip);
> > -       unsigned int reg;
> > +               /* Wake the chip up */
> > +               pca9685_set_sleep_mode(pca, false);
> > +       }
> >
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_H;
> > -       else
> > -               reg = LED_N_OFF_H(pwm->hwpwm);
> > +       duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > +       duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
> >
> > -       regmap_write(pca->regmap, reg, LED_FULL);
> > +       if (!state->enabled || duty < 1) {
> > +               /* Set full OFF bit first */
> > +               pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
> > +               pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
> > +               return 0;
> > +       }
> >
> > -       /* Clear the LED_OFF counter. */
> > -       if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > -               reg = PCA9685_ALL_LED_OFF_L;
> > -       else
> > -               reg = LED_N_OFF_L(pwm->hwpwm);
> > +       if (duty == PCA9685_COUNTER_RANGE) {
> > +               pca9685_pwm_set_full_on(pca, pwm->hwpwm, true);
> > +               /* Clear full OFF bit last */
> > +               pca9685_pwm_set_full_off(pca, pwm->hwpwm, false);
> 
> I think "this bit first, this bit last" can be confusing and fragile.
> It is also repeated in a few different places.
> Suggestion on how to improve this, below.

I wanted to let people know that the order is important, but yes, it is
also a little confusing.

> 
> > +               return 0;
> > +       }
> >
> > -       regmap_write(pca->regmap, reg, 0x0);
> > +       pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty);
> > +       /* Clear full ON bit after OFF time was set */
> > +       pca9685_pwm_set_full_on(pca, pwm->hwpwm, false);
> > +       return 0;
> >  }
> 
> I think pwm_apply() is broken in the following scenario:
> 
> 1. pwm_apply(enable=false)
> 2. pwm_apply(enable=true, duty_cycle=50%, period=200Hz)
> 3. result: pwm still full off (disabled)

No, this works correctly. I'll explain below.

> 
> and that is because pwm_apply() clears the "full off" bit only when the user
> requests "full on".

The full OFF bit is cleared in pca9685_pwm_set_off_time.
I did add a comment in that function that mentions it, but this can be
easily overlooked and is not clear from the name, so also something that
can and should be improved.

> 
> I attribute this to the confusing nature of this chip. It's hard to keep the
> chip's complete state fully in one's head.

I agree, you always have to think about the chip's internals. The
abstractions / common functions should contain the internals as
implementation details, so we do not have to think about them every time
we use on of these functions.

> One possible way to keep this manageable is by writing the functions in terms of
> *led state* and not in terms of the *bits* it sets:
> 
> - set_full_on() should make the led go full on. So it sets "full on"
>   and clears "full off" bits.
> - set_full_off() should make the led go full off. So it sets the "full off" bit.
> - set_duty() brings the led into duty mode. So it should clear "full off",
>   clear "full on", and write the on/off times.
> 
> And actually, all that's needed is a single function, because duty == 0 means
> "full off" and duty == 4095 means "full on".

Interesting! I'll have to think this through tomorrow but I think it
could work. I will wait with the next revision until we decide what to
do about Uwe's suggestion and then send a v5.

> 
> Example in pseudo code:
> 
> static void pca9685_pwm_set_duty(struct pca9685 *pca, int index,
> unsigned int duty)
> {
>     if (duty == 0) {
>         /* full off takes precedence */
>         regmap_write(full_off(index), ON);
>     } else if (duty >= (COUNTER_RANGE - 1)) {
>         regmap_write(full_on(index), ON);
>         regmap_write(full_off(index), OFF);
>     } else {
>         regmap_write(off_time(index), duty);
>         regmap_write(full_on(index), OFF);
>         regmap_write(full_off(index), OFF);
>     }
> }
> 
> static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int index)
> {
>     if (WARN_ON(index >= PCA9685_MAXCHAN)) {
>         /* register not readable, should never happen */
>         return 0;
>     }
>     if (full_off(index) is ON)
>         return 0;
>     else if (full_on(index) is ON)
>         return COUNTER_RANGE - 1;
>     return off_time(index);
> }
> 
> I suspect that once all on/off register accesses go through these two functions,
> things will look simpler and there will be less risk of getting it wrong.
> 
> To prevent a forest of "index >= PCA9685_MAXCHAN" checks, I suggest creating
> an helper function or macro, e.g.
> 
> static unsigned int led_off_h(int index)
> {
>     return (index >= PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H :
> LED_N_OFF_H(index);
> }
> 
> then setting a register looks simple:
> regmap_write(pca->regmap, led_off_h(index), LED_FULL);
> 
> >
> >  static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > @@ -422,15 +426,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >         struct pca9685 *pca = to_pca(chip);
> >
> > -       pca9685_pwm_disable(chip, pwm);
> > +       pca9685_pwm_set_full_off(pca, pwm->hwpwm, true);
> >         pm_runtime_put(chip->dev);
> >         pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> >  }
> >
> >  static const struct pwm_ops pca9685_pwm_ops = {
> > -       .enable = pca9685_pwm_enable,
> > -       .disable = pca9685_pwm_disable,
> > -       .config = pca9685_pwm_config,
> > +       .get_state = pca9685_pwm_get_state,
> > +       .apply = pca9685_pwm_apply,
> >         .request = pca9685_pwm_request,
> >         .free = pca9685_pwm_free,
> >         .owner = THIS_MODULE,
> > @@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >                         ret);
> >                 return ret;
> >         }
> > -       pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >
> >         i2c_set_clientdata(client, pca);
> >
> > @@ -505,14 +507,20 @@ 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
> > +        * Always initialize with default prescale, but chip must be
> > +        * in sleep mode while changing prescaler.
> >          */
> > +       pca9685_set_sleep_mode(pca, true);
> > +       regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF);
> > +       pm_runtime_set_suspended(&client->dev);
> >         pm_runtime_enable(&client->dev);
> >
> > +       if (!IS_ENABLED(CONFIG_PM)) {
> > +               /* Wake the chip up on non-PM environments */
> > +               pca9685_set_sleep_mode(pca, false);
> > +       }
> > +
> >         return 0;
> >  }
> >
> > @@ -524,7 +532,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.29.2
> >

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 23:13   ` Clemens Gruber
@ 2020-12-08  9:10     ` Uwe Kleine-König
  2020-12-08 10:12       ` Clemens Gruber
  0 siblings, 1 reply; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-08  9:10 UTC (permalink / raw)
  To: Clemens Gruber
  Cc: linux-pwm, Thierry Reding, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander

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

Hello Clemens,

On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote:
> On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote:
> > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > The hardware readout may return slightly different values than those
> > > that were set in apply due to the limited range of possible prescale and
> > > counter register values. If one channel is reconfigured with new duty
> > > cycle and period, the others will keep the same relative duty cycle to
> > > period ratio as they had before, even though the per-chip / global
> > > frequency changed. (The PCA9685 has only one prescaler!)
> > 
> > This is not acceptable, if you have two PWM outputs and a consumer
> > modifies one of them the other must change. So if this chip only
> > supports a single period length of all channels, the first consumer
> > enabling a channel defines the period to be used. All later consumers
> > must live with that. (Also the first must be denied modifying the period
> > if a second consumer has enabled its PWM.)
> 
> Good idea, but is it OK to potentially break users relying on the old
> behavior ("the last one who changes the period wins") ?

If this is already in the old code, this probably warrants a separate
fix, and yes, I consider this a severe bug. (Consider one channel
driving a motor and reconfiguring an LED modifies the motor's speed.)

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-07 22:34   ` Sven Van Asbroeck
  2020-12-07 23:24     ` Clemens Gruber
@ 2020-12-08  9:17     ` Uwe Kleine-König
  1 sibling, 0 replies; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-08  9:17 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Clemens Gruber, linux-pwm, Thierry Reding, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

Hello Sven,

On Mon, Dec 07, 2020 at 05:34:58PM -0500, Sven Van Asbroeck wrote:
> On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > This is not acceptable, if you have two PWM outputs and a consumer
> > modifies one of them the other must change. So if this chip only
> > supports a single period length of all channels, the first consumer
> > enabling a channel defines the period to be used. All later consumers
> > must live with that. (Also the first must be denied modifying the period
> > if a second consumer has enabled its PWM.)
> 
> That makes sense. However, a possible wrinkle: when more than one pwm channel
> is requested, which one is able to change the period?
> 
> Example:
> 1. start with all pwms free
> 2. pwm_request(0), pwm_apply(period=200Hz)
> 3. pwm_request(1)
> 4. pwm_apply(1, period=400Hz) fails?

Yes, pwm_apply_state is supposed to fail here (Sidenote: period
is measured in ns, not Hz)

> 5. pwm_apply(0, period=400Hz) succeeds?

This succeeds iff channel 1 isn't enabled. (Unless changing might
change the polarity of pwm #1 even if disabled.)

> And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz,
> because the pwm core doesn't realize anything has changed. Are you ok
> with this behaviour?

"if (5) succeeds" implies channel 1 is disabled (it might otherwise have
been enabled by the bootloader or a previous consumer).

With that sorted out, I'm ok that pwm_get_state() reports .period=200Hz
(or whatever other value) because it also reports .enabled = false which
makes every interpretation of the other values in pwm_state (apart from
.polarity) moot.

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08  9:10     ` Uwe Kleine-König
@ 2020-12-08 10:12       ` Clemens Gruber
  2020-12-08 13:44         ` Thierry Reding
  0 siblings, 1 reply; 32+ messages in thread
From: Clemens Gruber @ 2020-12-08 10:12 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-pwm, Thierry Reding, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander

Hello Uwe,

On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote:
> Hello Clemens,
> 
> On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote:
> > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote:
> > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > > The hardware readout may return slightly different values than those
> > > > that were set in apply due to the limited range of possible prescale and
> > > > counter register values. If one channel is reconfigured with new duty
> > > > cycle and period, the others will keep the same relative duty cycle to
> > > > period ratio as they had before, even though the per-chip / global
> > > > frequency changed. (The PCA9685 has only one prescaler!)
> > > 
> > > This is not acceptable, if you have two PWM outputs and a consumer
> > > modifies one of them the other must change. So if this chip only
> > > supports a single period length of all channels, the first consumer
> > > enabling a channel defines the period to be used. All later consumers
> > > must live with that. (Also the first must be denied modifying the period
> > > if a second consumer has enabled its PWM.)
> > 
> > Good idea, but is it OK to potentially break users relying on the old
> > behavior ("the last one who changes the period wins") ?
> 
> If this is already in the old code, this probably warrants a separate
> fix, and yes, I consider this a severe bug. (Consider one channel
> driving a motor and reconfiguring an LED modifies the motor's speed.)

Yes, but a user could also be relying on the old behavior as follows:
1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns
   (does not care about the frequency as long as it does not flicker)
2. Requests & enables pwm 1 for a motor, using a period of 1000000ns
   (does care about the frequency)

In the previous kernel versions, this would work, but with your
suggested change, (2) would fail and the motor would no longer work.

We are basically changing "the last one to set the period wins" to "the
first one to set the period wins".

If we do it like this, I'll split it out so we can at least revert it if
someone complains that it breaks his application, without reverting the
whole series.

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 10:12       ` Clemens Gruber
@ 2020-12-08 13:44         ` Thierry Reding
  2020-12-08 14:44           ` Sven Van Asbroeck
  0 siblings, 1 reply; 32+ messages in thread
From: Thierry Reding @ 2020-12-08 13:44 UTC (permalink / raw)
  To: Clemens Gruber
  Cc: Uwe Kleine-König, linux-pwm, Lee Jones, linux-kernel,
	Sven Van Asbroeck, Mika Westerberg, David Jander

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

On Tue, Dec 08, 2020 at 11:12:18AM +0100, Clemens Gruber wrote:
> Hello Uwe,
> 
> On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote:
> > Hello Clemens,
> > 
> > On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote:
> > > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote:
> > > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote:
> > > > > The hardware readout may return slightly different values than those
> > > > > that were set in apply due to the limited range of possible prescale and
> > > > > counter register values. If one channel is reconfigured with new duty
> > > > > cycle and period, the others will keep the same relative duty cycle to
> > > > > period ratio as they had before, even though the per-chip / global
> > > > > frequency changed. (The PCA9685 has only one prescaler!)
> > > > 
> > > > This is not acceptable, if you have two PWM outputs and a consumer
> > > > modifies one of them the other must change. So if this chip only
> > > > supports a single period length of all channels, the first consumer
> > > > enabling a channel defines the period to be used. All later consumers
> > > > must live with that. (Also the first must be denied modifying the period
> > > > if a second consumer has enabled its PWM.)
> > > 
> > > Good idea, but is it OK to potentially break users relying on the old
> > > behavior ("the last one who changes the period wins") ?
> > 
> > If this is already in the old code, this probably warrants a separate
> > fix, and yes, I consider this a severe bug. (Consider one channel
> > driving a motor and reconfiguring an LED modifies the motor's speed.)
> 
> Yes, but a user could also be relying on the old behavior as follows:
> 1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns
>    (does not care about the frequency as long as it does not flicker)
> 2. Requests & enables pwm 1 for a motor, using a period of 1000000ns
>    (does care about the frequency)
> 
> In the previous kernel versions, this would work, but with your
> suggested change, (2) would fail and the motor would no longer work.
> 
> We are basically changing "the last one to set the period wins" to "the
> first one to set the period wins".
> 
> If we do it like this, I'll split it out so we can at least revert it if
> someone complains that it breaks his application, without reverting the
> whole series.

Yes, that makes sense to me. We do want to make sure that we don't have
these kinds of races for PWM controllers and other drivers already have
corresponding checks in place.

But I agree that if this is preserving the status quo, then yes, we
should follow up with a separate patch to add that check so that it can
be easily reverted if this indeed break.

Although, if we do get failures after this check has been introduced,
they should be considered bugs and fixed in the right place. Ultimately
this is something that board designers have hopefully already thought
about and if there are two PWM consumers they will usually be able to
run at a common period, in which case fixing these should be as easy as
finding that common period and, well, using it for both consumers.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 13:44         ` Thierry Reding
@ 2020-12-08 14:44           ` Sven Van Asbroeck
  2020-12-08 16:57             ` Thierry Reding
  0 siblings, 1 reply; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-08 14:44 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Clemens Gruber, Uwe Kleine-König, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Uwe, Thierry,

On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> If this is already in the old code, this probably warrants a separate
> fix, and yes, I consider this a severe bug. (Consider one channel
> driving a motor and reconfiguring an LED modifies the motor's speed.)
>

I think you are 100% correct, this would be a severe bug. I have only used
this chip to drive LEDs, where the actual period is not that important. But
for motor control, it's a different story.

Basically you are suggesting: the period (prescaler) can only be changed iff
its use-count is 1.

This however brings up a whole load of additional questions: consider the case
where the chip outputs are also used in gpio mode. the gpio functionality
only sets "full on" and "full off" bits. On a scope, a gpio output will look
identical, no matter the value of the period. So when a gpio output is in use,
does it increment the prescaler use-count ?

Example:
1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
2. output 2: set led mode (full-on bit set)
3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)

Do we have to make (3) fail? I would say no: although output 2 is in use,
it's not actually using the prescaler. Changing prescale won't modify
output 2 in any way.

Which brings us to an even trickier question: what happens if a pwm output
is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
So when it's enabled, it does not use the prescaler.
But! what happens if we now set that output to a different duty cycle?

Example:
1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
  fail? no, because it's not actually using the period (it's full on)
3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
  fail? no, because it's not actually using the period (it's full on)
4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
  fail? no, because only output 1 is using the prescaler
5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
  fail? no, because output 2 is not changing the prescaler
6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
  fail? yes, because output 2 is changing prescaler and it's already in use

IMHO all this can get very complicated and tricky.

We can of course make this much simpler by assumung that gpio or on/off pwms
are actually using the prescaler. But then we'd be limiting this chip's
functionality.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 14:44           ` Sven Van Asbroeck
@ 2020-12-08 16:57             ` Thierry Reding
  2020-12-08 18:15               ` Sven Van Asbroeck
  2020-12-08 18:26               ` Uwe Kleine-König
  0 siblings, 2 replies; 32+ messages in thread
From: Thierry Reding @ 2020-12-08 16:57 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Clemens Gruber, Uwe Kleine-König, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> Uwe, Thierry,
> 
> On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > If this is already in the old code, this probably warrants a separate
> > fix, and yes, I consider this a severe bug. (Consider one channel
> > driving a motor and reconfiguring an LED modifies the motor's speed.)
> >
> 
> I think you are 100% correct, this would be a severe bug. I have only used
> this chip to drive LEDs, where the actual period is not that important. But
> for motor control, it's a different story.
> 
> Basically you are suggesting: the period (prescaler) can only be changed iff
> its use-count is 1.
> 
> This however brings up a whole load of additional questions: consider the case
> where the chip outputs are also used in gpio mode. the gpio functionality
> only sets "full on" and "full off" bits. On a scope, a gpio output will look
> identical, no matter the value of the period. So when a gpio output is in use,
> does it increment the prescaler use-count ?
> 
> Example:
> 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> 2. output 2: set led mode (full-on bit set)
> 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)
> 
> Do we have to make (3) fail? I would say no: although output 2 is in use,
> it's not actually using the prescaler. Changing prescale won't modify
> output 2 in any way.
> 
> Which brings us to an even trickier question: what happens if a pwm output
> is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> So when it's enabled, it does not use the prescaler.
> But! what happens if we now set that output to a different duty cycle?
> 
> Example:
> 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
>   fail? no, because it's not actually using the period (it's full on)
> 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
>   fail? no, because it's not actually using the period (it's full on)
> 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
>   fail? no, because only output 1 is using the prescaler
> 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
>   fail? no, because output 2 is not changing the prescaler
> 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
>   fail? yes, because output 2 is changing prescaler and it's already in use
> 
> IMHO all this can get very complicated and tricky.

Is this really that complicated? I sounds to me like the only thing that
you need is to have some sort of usage count for the prescaler. Whenever
you want to use the prescaler you check that usage count. If it is zero,
then you can just set it to whatever you need. If it isn't zero, that
means somebody else is already using it and you can't change it, which
means you have to check if you're trying to request the value that's
already set. If so, you can succeed, but otherwise you'll have to fail.

> We can of course make this much simpler by assumung that gpio or on/off pwms
> are actually using the prescaler. But then we'd be limiting this chip's
> functionality.

Yeah, this is obviously much simpler, but the cost is a bit high, in my
opinion. I'm fine with this alternative if there aren't any use-cases
where multiple outputs are actually used.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 16:57             ` Thierry Reding
@ 2020-12-08 18:15               ` Sven Van Asbroeck
  2020-12-08 20:25                 ` Uwe Kleine-König
  2020-12-08 18:26               ` Uwe Kleine-König
  1 sibling, 1 reply; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-08 18:15 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Clemens Gruber, Uwe Kleine-König, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

On Tue, Dec 8, 2020 at 11:57 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> Is this really that complicated? I sounds to me like the only thing that
> you need is to have some sort of usage count for the prescaler. Whenever
> you want to use the prescaler you check that usage count. If it is zero,
> then you can just set it to whatever you need. If it isn't zero, that
> means somebody else is already using it and you can't change it, which
> means you have to check if you're trying to request the value that's
> already set. If so, you can succeed, but otherwise you'll have to fail.

+1
I think your suggestion is an elegant solution to get the required behaviour.

One possible complication is synchronization. The sysfs interface has a lock
protecting against concurrent pwm_apply() calls. But the in-kernel
API (e.g. pwm_apply_state()) doesn't seem to. This is not normally a problem
when pwm bits are strictly separated. But in this case we have shared state
(prescale value and use count), so we probably need to protect pwm_apply()
with a mutex?

Not sure if it is currently possible *in practice* for two regulator consumer
drivers to call pwm_apply() from different threads. But Linux is slowly moving
towards asynchronous probing.

Uwe and Thierry, what is your opinion? Do you think we need to worry about
synchronization?

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 16:57             ` Thierry Reding
  2020-12-08 18:15               ` Sven Van Asbroeck
@ 2020-12-08 18:26               ` Uwe Kleine-König
  2020-12-08 20:54                 ` Clemens Gruber
  2020-12-09 17:02                 ` Thierry Reding
  1 sibling, 2 replies; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-08 18:26 UTC (permalink / raw)
  To: Thierry Reding, Sven Van Asbroeck
  Cc: Clemens Gruber, linux-pwm, Lee Jones, Linux Kernel Mailing List,
	Mika Westerberg, David Jander

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

Hello Thierry, hello Sven,

On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > If this is already in the old code, this probably warrants a separate
> > > fix, and yes, I consider this a severe bug. (Consider one channel
> > > driving a motor and reconfiguring an LED modifies the motor's speed.)
> > >
> > 
> > I think you are 100% correct, this would be a severe bug. I have only used
> > this chip to drive LEDs, where the actual period is not that important. But
> > for motor control, it's a different story.
> > 
> > Basically you are suggesting: the period (prescaler) can only be changed iff
> > its use-count is 1.
> > 
> > This however brings up a whole load of additional questions: consider the case
> > where the chip outputs are also used in gpio mode. the gpio functionality
> > only sets "full on" and "full off" bits. On a scope, a gpio output will look
> > identical, no matter the value of the period. So when a gpio output is in use,
> > does it increment the prescaler use-count ?
> > 
> > Example:
> > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > 2. output 2: set led mode (full-on bit set)
> > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)
> > 
> > Do we have to make (3) fail? I would say no: although output 2 is in use,
> > it's not actually using the prescaler. Changing prescale won't modify
> > output 2 in any way.
> > 
> > Which brings us to an even trickier question: what happens if a pwm output
> > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > So when it's enabled, it does not use the prescaler.
> > But! what happens if we now set that output to a different duty cycle?
> > 
> > Example:
> > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> >   fail? no, because it's not actually using the period (it's full on)
> > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> >   fail? no, because it's not actually using the period (it's full on)
> > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> >   fail? no, because only output 1 is using the prescaler
> > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> >   fail? no, because output 2 is not changing the prescaler
> > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> >   fail? yes, because output 2 is changing prescaler and it's already in use
> > 
> > IMHO all this can get very complicated and tricky.
> 
> Is this really that complicated?

I think it is.

> I sounds to me like the only thing that you need is to have some sort
> of usage count for the prescaler. Whenever you want to use the
> prescaler you check that usage count. If it is zero, then you can just
> set it to whatever you need. If it isn't zero, that means somebody
> else is already using it and you can't change it, which means you have
> to check if you're trying to request the value that's already set. If
> so, you can succeed, but otherwise you'll have to fail.

With this abstraction Sven's questions are changed to:

Does a PWM that runs at 0% or 100% use the prescaler?

If yes, you limit the possibilities of the brother channels. And if not,
it will not be possible to go to a 50% relative duty cycle while
retaining the period. Both sounds not optimal.
 
> > We can of course make this much simpler by assumung that gpio or on/off pwms
> > are actually using the prescaler. But then we'd be limiting this chip's
> > functionality.
> 
> Yeah, this is obviously much simpler, but the cost is a bit high, in my
> opinion. I'm fine with this alternative if there aren't any use-cases
> where multiple outputs are actually used.

This metric is wishy-washy; of course you can construct a use-case. I'd
still go for this simpler option and assume the prescaler used if the
PWM runs at 0% or 100%, but not when it is a GPIO.

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 18:15               ` Sven Van Asbroeck
@ 2020-12-08 20:25                 ` Uwe Kleine-König
  0 siblings, 0 replies; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-08 20:25 UTC (permalink / raw)
  To: Sven Van Asbroeck
  Cc: Thierry Reding, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

Hello Sven,

On Tue, Dec 08, 2020 at 01:15:10PM -0500, Sven Van Asbroeck wrote:
> On Tue, Dec 8, 2020 at 11:57 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > Is this really that complicated? I sounds to me like the only thing that
> > you need is to have some sort of usage count for the prescaler. Whenever
> > you want to use the prescaler you check that usage count. If it is zero,
> > then you can just set it to whatever you need. If it isn't zero, that
> > means somebody else is already using it and you can't change it, which
> > means you have to check if you're trying to request the value that's
> > already set. If so, you can succeed, but otherwise you'll have to fail.
> 
> +1
> I think your suggestion is an elegant solution to get the required behaviour.
> 
> One possible complication is synchronization. The sysfs interface has a lock
> protecting against concurrent pwm_apply() calls. But the in-kernel
> API (e.g. pwm_apply_state()) doesn't seem to. This is not normally a problem
> when pwm bits are strictly separated. But in this case we have shared state
> (prescale value and use count), so we probably need to protect pwm_apply()
> with a mutex?

Right, you need a lock. You can look at pwm-imx-tpm.c which has a
similar limitation.
 
> Not sure if it is currently possible *in practice* for two regulator consumer
> drivers to call pwm_apply() from different threads. But Linux is slowly moving
> towards asynchronous probing.

You must assume that there is concurrent access to different channels of
your hardware.

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 18:26               ` Uwe Kleine-König
@ 2020-12-08 20:54                 ` Clemens Gruber
  2020-12-09 17:02                 ` Thierry Reding
  1 sibling, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-08 20:54 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, Sven Van Asbroeck, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi everyone,

On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> Hello Thierry, hello Sven,
> 
> On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > If this is already in the old code, this probably warrants a separate
> > > > fix, and yes, I consider this a severe bug. (Consider one channel
> > > > driving a motor and reconfiguring an LED modifies the motor's speed.)
> > > >
> > > 
> > > I think you are 100% correct, this would be a severe bug. I have only used
> > > this chip to drive LEDs, where the actual period is not that important. But
> > > for motor control, it's a different story.
> > > 
> > > Basically you are suggesting: the period (prescaler) can only be changed iff
> > > its use-count is 1.
> > > 
> > > This however brings up a whole load of additional questions: consider the case
> > > where the chip outputs are also used in gpio mode. the gpio functionality
> > > only sets "full on" and "full off" bits. On a scope, a gpio output will look
> > > identical, no matter the value of the period. So when a gpio output is in use,
> > > does it increment the prescaler use-count ?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > 2. output 2: set led mode (full-on bit set)
> > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)
> > > 
> > > Do we have to make (3) fail? I would say no: although output 2 is in use,
> > > it's not actually using the prescaler. Changing prescale won't modify
> > > output 2 in any way.
> > > 
> > > Which brings us to an even trickier question: what happens if a pwm output
> > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > So when it's enabled, it does not use the prescaler.
> > > But! what happens if we now set that output to a different duty cycle?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > >   fail? no, because only output 1 is using the prescaler
> > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > >   fail? no, because output 2 is not changing the prescaler
> > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > 
> > > IMHO all this can get very complicated and tricky.
> > 
> > Is this really that complicated?
> 
> I think it is.
> 
> > I sounds to me like the only thing that you need is to have some sort
> > of usage count for the prescaler. Whenever you want to use the
> > prescaler you check that usage count. If it is zero, then you can just
> > set it to whatever you need. If it isn't zero, that means somebody
> > else is already using it and you can't change it, which means you have
> > to check if you're trying to request the value that's already set. If
> > so, you can succeed, but otherwise you'll have to fail.
> 
> With this abstraction Sven's questions are changed to:
> 
> Does a PWM that runs at 0% or 100% use the prescaler?
> 
> If yes, you limit the possibilities of the brother channels. And if not,
> it will not be possible to go to a 50% relative duty cycle while
> retaining the period. Both sounds not optimal.

In my opinion, limiting the possibilities of brother channels is
preferrable to introducing another restriction: Not being able to
reconfigure a duty cycle from 0%/100% to something else while keeping
the previously set period.
Better deny the period change in the first place, even if the duty cycle
is 0% or 100%.

>  
> > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > are actually using the prescaler. But then we'd be limiting this chip's
> > > functionality.
> > 
> > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > opinion. I'm fine with this alternative if there aren't any use-cases
> > where multiple outputs are actually used.
> 
> This metric is wishy-washy; of course you can construct a use-case. I'd
> still go for this simpler option and assume the prescaler used if the
> PWM runs at 0% or 100%, but not when it is a GPIO.

I'd also prefer this solution.

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-08 18:26               ` Uwe Kleine-König
  2020-12-08 20:54                 ` Clemens Gruber
@ 2020-12-09 17:02                 ` Thierry Reding
  2020-12-10  9:01                   ` Uwe Kleine-König
  1 sibling, 1 reply; 32+ messages in thread
From: Thierry Reding @ 2020-12-09 17:02 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> Hello Thierry, hello Sven,
> 
> On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > If this is already in the old code, this probably warrants a separate
> > > > fix, and yes, I consider this a severe bug. (Consider one channel
> > > > driving a motor and reconfiguring an LED modifies the motor's speed.)
> > > >
> > > 
> > > I think you are 100% correct, this would be a severe bug. I have only used
> > > this chip to drive LEDs, where the actual period is not that important. But
> > > for motor control, it's a different story.
> > > 
> > > Basically you are suggesting: the period (prescaler) can only be changed iff
> > > its use-count is 1.
> > > 
> > > This however brings up a whole load of additional questions: consider the case
> > > where the chip outputs are also used in gpio mode. the gpio functionality
> > > only sets "full on" and "full off" bits. On a scope, a gpio output will look
> > > identical, no matter the value of the period. So when a gpio output is in use,
> > > does it increment the prescaler use-count ?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > 2. output 2: set led mode (full-on bit set)
> > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz)
> > > 
> > > Do we have to make (3) fail? I would say no: although output 2 is in use,
> > > it's not actually using the prescaler. Changing prescale won't modify
> > > output 2 in any way.
> > > 
> > > Which brings us to an even trickier question: what happens if a pwm output
> > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > So when it's enabled, it does not use the prescaler.
> > > But! what happens if we now set that output to a different duty cycle?
> > > 
> > > Example:
> > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > >   fail? no, because it's not actually using the period (it's full on)
> > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > >   fail? no, because only output 1 is using the prescaler
> > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > >   fail? no, because output 2 is not changing the prescaler
> > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > 
> > > IMHO all this can get very complicated and tricky.
> > 
> > Is this really that complicated?
> 
> I think it is.

Care to specify what exactly is complicated about it? You're just saying
that you don't like the restrictions that this implements, but there's
really nothing we can do about that because the hardware just doesn't
give you that flexibility.

> > I sounds to me like the only thing that you need is to have some sort
> > of usage count for the prescaler. Whenever you want to use the
> > prescaler you check that usage count. If it is zero, then you can just
> > set it to whatever you need. If it isn't zero, that means somebody
> > else is already using it and you can't change it, which means you have
> > to check if you're trying to request the value that's already set. If
> > so, you can succeed, but otherwise you'll have to fail.
> 
> With this abstraction Sven's questions are changed to:
> 
> Does a PWM that runs at 0% or 100% use the prescaler?
> 
> If yes, you limit the possibilities of the brother channels. And if not,
> it will not be possible to go to a 50% relative duty cycle while
> retaining the period. Both sounds not optimal.

Again, this is a restriction imposed by the hardware design and there's
nothing in software that we can do about that. The only thing I proposed
was a simple way to detect the circumstances and make sure we can deal
with it.

And that's obviously subject to the kind of policy we want to implement.
I don't think it's necessarily a bad thing to give people the most
flexibility. If they know that one PWM channel is only ever going to be
full-on/full-off, then they can still use that other channel in whatever
way they want. If, on the other hand, we assume that the prescaler is
always going to be used we limit the flexibility even if we don't
necessarily have to.

Obviously if you want to use both channels at partial duty-cycles there
isn't much you can do and you really have to make sure they both run at
the same frequency/period. But that's usually something that you can
deal with by just choosing a period that works for both.

And if that's not possible, well, then you better just use a different
PWM controller to begin with, because you just can't make it work.

> > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > are actually using the prescaler. But then we'd be limiting this chip's
> > > functionality.
> > 
> > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > opinion. I'm fine with this alternative if there aren't any use-cases
> > where multiple outputs are actually used.
> 
> This metric is wishy-washy; of course you can construct a use-case. I'd
> still go for this simpler option and assume the prescaler used if the
> PWM runs at 0% or 100%, but not when it is a GPIO.

I don't understand what you're saying here. On one hand you seem to
object to what I was saying, but then you agree with it?

And I'm not asking anyone to make up any artificial use-case. What I'm
saying is that if there aren't any existing use-cases that would break
if we assume a pre-scaler is used for full-on/full-off, then I'm okay
with making that assumption and simplifying the driver. If there were
use-cases, then that assumption would break existing users and that's
not something I'm willing to accept.

Anything wrong with that metric in your opinion?

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-09 17:02                 ` Thierry Reding
@ 2020-12-10  9:01                   ` Uwe Kleine-König
  2020-12-10 17:10                     ` Thierry Reding
  0 siblings, 1 reply; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-10  9:01 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

Hello Thierry,

On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote:
> On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> > Hello Thierry, hello Sven,
> > 
> > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > > Which brings us to an even trickier question: what happens if a pwm output
> > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > > So when it's enabled, it does not use the prescaler.
> > > > But! what happens if we now set that output to a different duty cycle?
> > > > 
> > > > Example:
> > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > > >   fail? no, because it's not actually using the period (it's full on)
> > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > > >   fail? no, because it's not actually using the period (it's full on)
> > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > > >   fail? no, because only output 1 is using the prescaler
> > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > > >   fail? no, because output 2 is not changing the prescaler
> > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > > 
> > > > IMHO all this can get very complicated and tricky.
> > > 
> > > Is this really that complicated?
> > 
> > I think it is.
> 
> Care to specify what exactly is complicated about it? You're just saying
> that you don't like the restrictions that this implements, but there's
> really nothing we can do about that because the hardware just doesn't
> give you that flexibility.

The complicated thing is to chose how to map the hardware imposed
limitations to the consumers of the two (or more?) channels. And the
problem is there is no golden way that is objectively better than all
others.

> > > I sounds to me like the only thing that you need is to have some sort
> > > of usage count for the prescaler. Whenever you want to use the
> > > prescaler you check that usage count. If it is zero, then you can just
> > > set it to whatever you need. If it isn't zero, that means somebody
> > > else is already using it and you can't change it, which means you have
> > > to check if you're trying to request the value that's already set. If
> > > so, you can succeed, but otherwise you'll have to fail.
> > 
> > With this abstraction Sven's questions are changed to:
> > 
> > Does a PWM that runs at 0% or 100% use the prescaler?
> > 
> > If yes, you limit the possibilities of the brother channels. And if not,
> > it will not be possible to go to a 50% relative duty cycle while
> > retaining the period. Both sounds not optimal.
> 
> Again, this is a restriction imposed by the hardware design and there's
> nothing in software that we can do about that. The only thing I proposed
> was a simple way to detect the circumstances and make sure we can deal
> with it.

The point I want to make is, that with the usage counter you suggested
you just shifted the problem and didn't solve it. I agree we need a
usage counter, but you still have to think about how you want to answer
the original questions by Sven. Depending on that you have to
consider a channel running at 0% or 100% a user, or not.  (Or the other
way round: You select a policy if you consider 0% and 100% a use and
implicitly answer the questions with it.)

> And that's obviously subject to the kind of policy we want to implement.
> I don't think it's necessarily a bad thing to give people the most
> flexibility. If they know that one PWM channel is only ever going to be
> full-on/full-off, then they can still use that other channel in whatever
> way they want. If, on the other hand, we assume that the prescaler is
> always going to be used we limit the flexibility even if we don't
> necessarily have to.

I think we agree here, just with different words. The only thing I doubt
is: You wrote: "If they know $X, then they can still use that other
channel in whatever way they want." Who is "they"? How can they know
that $X is valid for someone else, or anyone else? Or is it enough that
"they" know this about their own use? Now Clemens wants to improve the
driver, does he need to consider "them" in the mainline driver
implementation? If Clemens chooses one way or the other, will there be
someone who then will produce a use case contradicting the implemented
policy? How will you (or who will then) decide which use-case is more
important?

> Obviously if you want to use both channels at partial duty-cycles there
> isn't much you can do and you really have to make sure they both run at
> the same frequency/period. But that's usually something that you can
> deal with by just choosing a period that works for both.
>
> And if that's not possible, well, then you better just use a different
> PWM controller to begin with, because you just can't make it work.

Yes.

> > > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > > are actually using the prescaler. But then we'd be limiting this chip's
> > > > functionality.
> > > 
> > > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > > opinion. I'm fine with this alternative if there aren't any use-cases
> > > where multiple outputs are actually used.
> > 
> > This metric is wishy-washy; of course you can construct a use-case. I'd
> > still go for this simpler option and assume the prescaler used if the
> > PWM runs at 0% or 100%, but not when it is a GPIO.
> 
> I don't understand what you're saying here. On one hand you seem to
> object to what I was saying, but then you agree with it?
> 
> And I'm not asking anyone to make up any artificial use-case. What I'm
> saying is that if there aren't any existing use-cases that would break
> if we assume a pre-scaler is used for full-on/full-off, then I'm okay
> with making that assumption and simplifying the driver. If there were
> use-cases, then that assumption would break existing users and that's
> not something I'm willing to accept.
> 
> Anything wrong with that metric in your opinion?

The part I called wishy-washy is: "[....] if there aren't any use-cases
where [...]". Who should decide what are (relevant) use-cases? We
already agreed above that we talk about a hardware that doesn't allow us
to consider it consisting of 2 independent channels and so we somehow
have to limit their capabilities exposed by the PWM API. And whatever
compromise we make, it's not hard to find a more or less realistic use
case where the compromise hurts. So my interpretation of your words are:
"I'm fine with this alternative if $somethingimpossible" which is not
helpful.

So yes, there is something wrong with your metric because it's IMHO
impossible for a driver author to actually use it.

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-10  9:01                   ` Uwe Kleine-König
@ 2020-12-10 17:10                     ` Thierry Reding
  2020-12-10 20:39                       ` Uwe Kleine-König
  2020-12-10 20:54                       ` Clemens Gruber
  0 siblings, 2 replies; 32+ messages in thread
From: Thierry Reding @ 2020-12-10 17:10 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

On Thu, Dec 10, 2020 at 10:01:24AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote:
> > On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> > > Hello Thierry, hello Sven,
> > > 
> > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > > > Which brings us to an even trickier question: what happens if a pwm output
> > > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > > > So when it's enabled, it does not use the prescaler.
> > > > > But! what happens if we now set that output to a different duty cycle?
> > > > > 
> > > > > Example:
> > > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > > > >   fail? no, because it's not actually using the period (it's full on)
> > > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > > > >   fail? no, because it's not actually using the period (it's full on)
> > > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > > > >   fail? no, because only output 1 is using the prescaler
> > > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > > > >   fail? no, because output 2 is not changing the prescaler
> > > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > > > 
> > > > > IMHO all this can get very complicated and tricky.
> > > > 
> > > > Is this really that complicated?
> > > 
> > > I think it is.
> > 
> > Care to specify what exactly is complicated about it? You're just saying
> > that you don't like the restrictions that this implements, but there's
> > really nothing we can do about that because the hardware just doesn't
> > give you that flexibility.
> 
> The complicated thing is to chose how to map the hardware imposed
> limitations to the consumers of the two (or more?) channels. And the
> problem is there is no golden way that is objectively better than all
> others.
> 
> > > > I sounds to me like the only thing that you need is to have some sort
> > > > of usage count for the prescaler. Whenever you want to use the
> > > > prescaler you check that usage count. If it is zero, then you can just
> > > > set it to whatever you need. If it isn't zero, that means somebody
> > > > else is already using it and you can't change it, which means you have
> > > > to check if you're trying to request the value that's already set. If
> > > > so, you can succeed, but otherwise you'll have to fail.
> > > 
> > > With this abstraction Sven's questions are changed to:
> > > 
> > > Does a PWM that runs at 0% or 100% use the prescaler?
> > > 
> > > If yes, you limit the possibilities of the brother channels. And if not,
> > > it will not be possible to go to a 50% relative duty cycle while
> > > retaining the period. Both sounds not optimal.
> > 
> > Again, this is a restriction imposed by the hardware design and there's
> > nothing in software that we can do about that. The only thing I proposed
> > was a simple way to detect the circumstances and make sure we can deal
> > with it.
> 
> The point I want to make is, that with the usage counter you suggested
> you just shifted the problem and didn't solve it. I agree we need a
> usage counter, but you still have to think about how you want to answer
> the original questions by Sven. Depending on that you have to
> consider a channel running at 0% or 100% a user, or not.  (Or the other
> way round: You select a policy if you consider 0% and 100% a use and
> implicitly answer the questions with it.)

The way I see it, and I think this matches what was said earlier, we
have two options:

  1) Use two channels as proper PWMs. In that case, if they happen to
     run at the same period, they are free to choose the duty-cycle and
     can be treated as independent PWM channels. If the run at different
     periods, one of them can only be used as either full-on or full-off
     PWM. It would still be a PWM, but just with additional restrictions
     on the duty cycle.

  2) Use one channel as full PWM and the other channel as GPIO.

2) is a subset of 1) because we can implement 2) using 1) by treating
the full-on/full-off PWM as a GPIO pin. In my opinion, that makes the
first policy better because it is more flexible.

> > And that's obviously subject to the kind of policy we want to implement.
> > I don't think it's necessarily a bad thing to give people the most
> > flexibility. If they know that one PWM channel is only ever going to be
> > full-on/full-off, then they can still use that other channel in whatever
> > way they want. If, on the other hand, we assume that the prescaler is
> > always going to be used we limit the flexibility even if we don't
> > necessarily have to.
> 
> I think we agree here, just with different words. The only thing I doubt
> is: You wrote: "If they know $X, then they can still use that other
> channel in whatever way they want." Who is "they"? How can they know
> that $X is valid for someone else, or anyone else? Or is it enough that
> "they" know this about their own use? Now Clemens wants to improve the
> driver, does he need to consider "them" in the mainline driver
> implementation? If Clemens chooses one way or the other, will there be
> someone who then will produce a use case contradicting the implemented
> policy? How will you (or who will then) decide which use-case is more
> important?

"They" refers to whoever writes the DT. Given the hardware restrictions,
the use-cases are already limited, so you are not going to get into a
situation where option 1) wouldn't work for any *supported* use-cases.
You can obviously always come up with a use-case where it wouldn't work,
but if that's not supported by the hardware, then obviously there's not
a thing we can do about it, as we already concluded.

However, if you know that your hardware is restricted in this way, then
the hardware designers will already have ensured that the supported use-
cases don't conflict with the hardware.

So if for example you want to support both a PWM fan and an LED with the
two outputs, then that's going to work because you can select a period
that works for both and run them both at the same period but at
different duty cycles.

If you have some other setup where one use-case requires the PWM period
to be adjustable, then by definition with this hardware the second
channel can't be required to be adjustable as well. In that case you can
not rely on the second channel to be anything but full-on/full-off. But
that's still enough to implement a basic LED, so that's what the board
might support.

Now you can obviously implement a simple LED using either a GPIO or a
PWM, so it doesn't really matter in that case which policy we choose
here. However, a GPIO is a special case of a PWM, so I think modelling
the second output as a full PWM is better policy because it gives you a
tiny bit more flexibility.

So what I'm saying is that the hardware already dictates the set of
possible use-cases, so if anyone is going to use this in a way that
isn't supported, then it's okay to fail.

> > Obviously if you want to use both channels at partial duty-cycles there
> > isn't much you can do and you really have to make sure they both run at
> > the same frequency/period. But that's usually something that you can
> > deal with by just choosing a period that works for both.
> >
> > And if that's not possible, well, then you better just use a different
> > PWM controller to begin with, because you just can't make it work.
> 
> Yes.
> 
> > > > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > > > are actually using the prescaler. But then we'd be limiting this chip's
> > > > > functionality.
> > > > 
> > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > > > opinion. I'm fine with this alternative if there aren't any use-cases
> > > > where multiple outputs are actually used.
> > > 
> > > This metric is wishy-washy; of course you can construct a use-case. I'd
> > > still go for this simpler option and assume the prescaler used if the
> > > PWM runs at 0% or 100%, but not when it is a GPIO.
> > 
> > I don't understand what you're saying here. On one hand you seem to
> > object to what I was saying, but then you agree with it?
> > 
> > And I'm not asking anyone to make up any artificial use-case. What I'm
> > saying is that if there aren't any existing use-cases that would break
> > if we assume a pre-scaler is used for full-on/full-off, then I'm okay
> > with making that assumption and simplifying the driver. If there were
> > use-cases, then that assumption would break existing users and that's
> > not something I'm willing to accept.
> > 
> > Anything wrong with that metric in your opinion?
> 
> The part I called wishy-washy is: "[....] if there aren't any use-cases
> where [...]". Who should decide what are (relevant) use-cases? We
> already agreed above that we talk about a hardware that doesn't allow us
> to consider it consisting of 2 independent channels and so we somehow
> have to limit their capabilities exposed by the PWM API. And whatever
> compromise we make, it's not hard to find a more or less realistic use
> case where the compromise hurts. So my interpretation of your words are:
> "I'm fine with this alternative if $somethingimpossible" which is not
> helpful.
> 
> So yes, there is something wrong with your metric because it's IMHO
> impossible for a driver author to actually use it.

Like I said, that's not what I was saying. I was merely saying that if
there aren't any use-cases that current users rely on that would be
broken by using this simpler implementation, then I'm okay with it, even
if it's less flexible than a more complicated implementation. It should
be possible to determine what the current users are by inspecting device
trees present in the kernel. Anything outside the kernel isn't something
we need to consider, as usual.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-10 17:10                     ` Thierry Reding
@ 2020-12-10 20:39                       ` Uwe Kleine-König
  2020-12-11  8:33                         ` Thierry Reding
  2020-12-10 20:54                       ` Clemens Gruber
  1 sibling, 1 reply; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-10 20:39 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

Hello Thierry,

On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> Like I said, that's not what I was saying. I was merely saying that if
> there aren't any use-cases that current users rely on that would be
> broken by using this simpler implementation, then I'm okay with it, even
> if it's less flexible than a more complicated implementation. It should
> be possible to determine what the current users are by inspecting device
> trees present in the kernel. Anything outside the kernel isn't something
> we need to consider, as usual.

If "users in mainline" is the criteria that's a word.

So you agree we remove the following drivers?:

 - pwm-hibvt.c
   Last driver specific change in Feb 2019, no mainline user
 - pwm-sprd.c
   Last driver specific change in Aug 2019, no mainline user

Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder
if it makes sense to check the machine.dts files if some of the PMWs are
completely unused. Do you consider status = "okay" a use that we have to
retain even if the node has no phandle?

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-10 17:10                     ` Thierry Reding
  2020-12-10 20:39                       ` Uwe Kleine-König
@ 2020-12-10 20:54                       ` Clemens Gruber
  2020-12-10 21:37                         ` Sven Van Asbroeck
  1 sibling, 1 reply; 32+ messages in thread
From: Clemens Gruber @ 2020-12-10 20:54 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Uwe Kleine-König, Sven Van Asbroeck, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> On Thu, Dec 10, 2020 at 10:01:24AM +0100, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote:
> > > On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote:
> > > > Hello Thierry, hello Sven,
> > > > 
> > > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote:
> > > > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote:
> > > > > > Which brings us to an even trickier question: what happens if a pwm output
> > > > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output.
> > > > > > So when it's enabled, it does not use the prescaler.
> > > > > > But! what happens if we now set that output to a different duty cycle?
> > > > > > 
> > > > > > Example:
> > > > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/200Hz)
> > > > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz)
> > > > > >   fail? no, because it's not actually using the period (it's full on)
> > > > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz)
> > > > > >   fail? no, because it's not actually using the period (it's full on)
> > > > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%,  period=1/400Hz)
> > > > > >   fail? no, because only output 1 is using the prescaler
> > > > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz)
> > > > > >   fail? no, because output 2 is not changing the prescaler
> > > > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz)
> > > > > >   fail? yes, because output 2 is changing prescaler and it's already in use
> > > > > > 
> > > > > > IMHO all this can get very complicated and tricky.
> > > > > 
> > > > > Is this really that complicated?
> > > > 
> > > > I think it is.
> > > 
> > > Care to specify what exactly is complicated about it? You're just saying
> > > that you don't like the restrictions that this implements, but there's
> > > really nothing we can do about that because the hardware just doesn't
> > > give you that flexibility.
> > 
> > The complicated thing is to chose how to map the hardware imposed
> > limitations to the consumers of the two (or more?) channels. And the
> > problem is there is no golden way that is objectively better than all
> > others.
> > 
> > > > > I sounds to me like the only thing that you need is to have some sort
> > > > > of usage count for the prescaler. Whenever you want to use the
> > > > > prescaler you check that usage count. If it is zero, then you can just
> > > > > set it to whatever you need. If it isn't zero, that means somebody
> > > > > else is already using it and you can't change it, which means you have
> > > > > to check if you're trying to request the value that's already set. If
> > > > > so, you can succeed, but otherwise you'll have to fail.
> > > > 
> > > > With this abstraction Sven's questions are changed to:
> > > > 
> > > > Does a PWM that runs at 0% or 100% use the prescaler?
> > > > 
> > > > If yes, you limit the possibilities of the brother channels. And if not,
> > > > it will not be possible to go to a 50% relative duty cycle while
> > > > retaining the period. Both sounds not optimal.
> > > 
> > > Again, this is a restriction imposed by the hardware design and there's
> > > nothing in software that we can do about that. The only thing I proposed
> > > was a simple way to detect the circumstances and make sure we can deal
> > > with it.
> > 
> > The point I want to make is, that with the usage counter you suggested
> > you just shifted the problem and didn't solve it. I agree we need a
> > usage counter, but you still have to think about how you want to answer
> > the original questions by Sven. Depending on that you have to
> > consider a channel running at 0% or 100% a user, or not.  (Or the other
> > way round: You select a policy if you consider 0% and 100% a use and
> > implicitly answer the questions with it.)
> 
> The way I see it, and I think this matches what was said earlier, we
> have two options:
> 
>   1) Use two channels as proper PWMs. In that case, if they happen to
>      run at the same period, they are free to choose the duty-cycle and
>      can be treated as independent PWM channels. If the run at different
>      periods, one of them can only be used as either full-on or full-off
>      PWM. It would still be a PWM, but just with additional restrictions
>      on the duty cycle.
> 
>   2) Use one channel as full PWM and the other channel as GPIO.
> 
> 2) is a subset of 1) because we can implement 2) using 1) by treating
> the full-on/full-off PWM as a GPIO pin. In my opinion, that makes the
> first policy better because it is more flexible.
> 
> > > And that's obviously subject to the kind of policy we want to implement.
> > > I don't think it's necessarily a bad thing to give people the most
> > > flexibility. If they know that one PWM channel is only ever going to be
> > > full-on/full-off, then they can still use that other channel in whatever
> > > way they want. If, on the other hand, we assume that the prescaler is
> > > always going to be used we limit the flexibility even if we don't
> > > necessarily have to.
> > 
> > I think we agree here, just with different words. The only thing I doubt
> > is: You wrote: "If they know $X, then they can still use that other
> > channel in whatever way they want." Who is "they"? How can they know
> > that $X is valid for someone else, or anyone else? Or is it enough that
> > "they" know this about their own use? Now Clemens wants to improve the
> > driver, does he need to consider "them" in the mainline driver
> > implementation? If Clemens chooses one way or the other, will there be
> > someone who then will produce a use case contradicting the implemented
> > policy? How will you (or who will then) decide which use-case is more
> > important?
> 
> "They" refers to whoever writes the DT. Given the hardware restrictions,
> the use-cases are already limited, so you are not going to get into a
> situation where option 1) wouldn't work for any *supported* use-cases.
> You can obviously always come up with a use-case where it wouldn't work,
> but if that's not supported by the hardware, then obviously there's not
> a thing we can do about it, as we already concluded.
> 
> However, if you know that your hardware is restricted in this way, then
> the hardware designers will already have ensured that the supported use-
> cases don't conflict with the hardware.
> 
> So if for example you want to support both a PWM fan and an LED with the
> two outputs, then that's going to work because you can select a period
> that works for both and run them both at the same period but at
> different duty cycles.
> 
> If you have some other setup where one use-case requires the PWM period
> to be adjustable, then by definition with this hardware the second
> channel can't be required to be adjustable as well. In that case you can
> not rely on the second channel to be anything but full-on/full-off. But
> that's still enough to implement a basic LED, so that's what the board
> might support.
> 
> Now you can obviously implement a simple LED using either a GPIO or a
> PWM, so it doesn't really matter in that case which policy we choose
> here. However, a GPIO is a special case of a PWM, so I think modelling
> the second output as a full PWM is better policy because it gives you a
> tiny bit more flexibility.
> 
> So what I'm saying is that the hardware already dictates the set of
> possible use-cases, so if anyone is going to use this in a way that
> isn't supported, then it's okay to fail.
> 
> > > Obviously if you want to use both channels at partial duty-cycles there
> > > isn't much you can do and you really have to make sure they both run at
> > > the same frequency/period. But that's usually something that you can
> > > deal with by just choosing a period that works for both.
> > >
> > > And if that's not possible, well, then you better just use a different
> > > PWM controller to begin with, because you just can't make it work.
> > 
> > Yes.
> > 
> > > > > > We can of course make this much simpler by assumung that gpio or on/off pwms
> > > > > > are actually using the prescaler. But then we'd be limiting this chip's
> > > > > > functionality.
> > > > > 
> > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my
> > > > > opinion. I'm fine with this alternative if there aren't any use-cases
> > > > > where multiple outputs are actually used.
> > > > 
> > > > This metric is wishy-washy; of course you can construct a use-case. I'd
> > > > still go for this simpler option and assume the prescaler used if the
> > > > PWM runs at 0% or 100%, but not when it is a GPIO.
> > > 
> > > I don't understand what you're saying here. On one hand you seem to
> > > object to what I was saying, but then you agree with it?
> > > 
> > > And I'm not asking anyone to make up any artificial use-case. What I'm
> > > saying is that if there aren't any existing use-cases that would break
> > > if we assume a pre-scaler is used for full-on/full-off, then I'm okay
> > > with making that assumption and simplifying the driver. If there were
> > > use-cases, then that assumption would break existing users and that's
> > > not something I'm willing to accept.
> > > 
> > > Anything wrong with that metric in your opinion?
> > 
> > The part I called wishy-washy is: "[....] if there aren't any use-cases
> > where [...]". Who should decide what are (relevant) use-cases? We
> > already agreed above that we talk about a hardware that doesn't allow us
> > to consider it consisting of 2 independent channels and so we somehow
> > have to limit their capabilities exposed by the PWM API. And whatever
> > compromise we make, it's not hard to find a more or less realistic use
> > case where the compromise hurts. So my interpretation of your words are:
> > "I'm fine with this alternative if $somethingimpossible" which is not
> > helpful.
> > 
> > So yes, there is something wrong with your metric because it's IMHO
> > impossible for a driver author to actually use it.
> 
> Like I said, that's not what I was saying. I was merely saying that if
> there aren't any use-cases that current users rely on that would be
> broken by using this simpler implementation, then I'm okay with it, even
> if it's less flexible than a more complicated implementation. It should
> be possible to determine what the current users are by inspecting device
> trees present in the kernel. Anything outside the kernel isn't something
> we need to consider, as usual.

I grepped for pca9685 and found no current in-kernel users of this
driver.

After reading your reasoning in this mail and rethinking the whole
situation, I do no longer have any objections to the more complex
solution. (Allowing 0% and 100% duty cycle channels with any period)

I first thought it would be too confusing to block users from changing a
duty cycle when in reality the period is the problem.
However, if we log an error message, explaining that the periods have to
match if duty cycles > 0 and < 100% are used, I think it's OK.

Uwe, Sven: Do you have any objections?

If not, I will prepare a v5 next week.

Thanks,
Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-10 20:54                       ` Clemens Gruber
@ 2020-12-10 21:37                         ` Sven Van Asbroeck
  0 siblings, 0 replies; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-10 21:37 UTC (permalink / raw)
  To: Clemens Gruber
  Cc: Thierry Reding, Uwe Kleine-König, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

On Thu, Dec 10, 2020 at 3:54 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> After reading your reasoning in this mail and rethinking the whole
> situation, I do no longer have any objections to the more complex
> solution. (Allowing 0% and 100% duty cycle channels with any period)
>
> I first thought it would be too confusing to block users from changing a
> duty cycle when in reality the period is the problem.
> However, if we log an error message, explaining that the periods have to
> match if duty cycles > 0 and < 100% are used, I think it's OK.
>
> Uwe, Sven: Do you have any objections?

No objections, as long as the "fully flexible" solution doesn't get too
complex. Complex code is hard to maintain and extend, but obviously that
decision is for Thierry or Uwe to make.

Thinking this through a little bit, I don't think the "fully flexible"
solution has to be that complex. Keeping track of prescale-inuse may
have to involve a bitmask and not a counter, such as here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.10-rc7#n81

That way, channels can do set_bit() to mark that they use the prescaler,
clear_bit() when giving up the prescaler, and if (bitmap_weight() <= 1) to
check if they can make a prescale change.

But this is theory - the only way to find out what's best, is to actually
write the code...

> If not, I will prepare a v5 next week.

Looking forward to it, thank you !!

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-10 20:39                       ` Uwe Kleine-König
@ 2020-12-11  8:33                         ` Thierry Reding
  2020-12-11 10:34                           ` Uwe Kleine-König
  0 siblings, 1 reply; 32+ messages in thread
From: Thierry Reding @ 2020-12-11  8:33 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> > Like I said, that's not what I was saying. I was merely saying that if
> > there aren't any use-cases that current users rely on that would be
> > broken by using this simpler implementation, then I'm okay with it, even
> > if it's less flexible than a more complicated implementation. It should
> > be possible to determine what the current users are by inspecting device
> > trees present in the kernel. Anything outside the kernel isn't something
> > we need to consider, as usual.
> 
> If "users in mainline" is the criteria that's a word.

I didn't say "users in mainline", I said "use-cases". What I don't want
to happen is for this change under discussion to break any existing use-
cases of any existing users in the kernel. I said that we can determine
what the existing users are by looking at which device trees use the
compatible strings that the driver matches on.

> So you agree we remove the following drivers?:
> 
>  - pwm-hibvt.c
>    Last driver specific change in Feb 2019, no mainline user
>  - pwm-sprd.c
>    Last driver specific change in Aug 2019, no mainline user

No, that's an extrapolation of what I was saying above. Drivers with no
apparent users are a separate topic, so don't conflate it with the issue
at hand.

While it's certainly unfortunate that these don't seem to be used, I see
no reason why we should remove them. They don't create much of a
maintenance burden, so I'm fine with keeping them in the hopes that
users may still show up at some point.

> Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder
> if it makes sense to check the machine.dts files if some of the PMWs are
> completely unused. Do you consider status = "okay" a use that we have to
> retain even if the node has no phandle?

A PWM controller may be in use via sysfs even if it has no phandle.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-11  8:33                         ` Thierry Reding
@ 2020-12-11 10:34                           ` Uwe Kleine-König
  2020-12-14 14:28                             ` Thierry Reding
  0 siblings, 1 reply; 32+ messages in thread
From: Uwe Kleine-König @ 2020-12-11 10:34 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

Hello Thierry,

On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote:
> On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote:
> > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> > > Like I said, that's not what I was saying. I was merely saying that if
> > > there aren't any use-cases that current users rely on that would be
> > > broken by using this simpler implementation, then I'm okay with it, even
> > > if it's less flexible than a more complicated implementation. It should
> > > be possible to determine what the current users are by inspecting device
> > > trees present in the kernel. Anything outside the kernel isn't something
> > > we need to consider, as usual.
> > 
> > If "users in mainline" is the criteria that's a word.
> 
> I didn't say "users in mainline", I said "use-cases". What I don't want
> to happen is for this change under discussion to break any existing use-
> cases of any existing users in the kernel. I said that we can determine
> what the existing users are by looking at which device trees use the
> compatible strings that the driver matches on.
> 
> > So you agree we remove the following drivers?:
> > 
> >  - pwm-hibvt.c
> >    Last driver specific change in Feb 2019, no mainline user
> >  - pwm-sprd.c
> >    Last driver specific change in Aug 2019, no mainline user
> 
> No, that's an extrapolation of what I was saying above. Drivers with no
> apparent users are a separate topic, so don't conflate it with the issue
> at hand.

I cannot follow (and I think that's the problem between us and why those
conflicts happen between us). For me it's a logic consequence of
"Anything outside the kernel isn't something we need to consider, as
usual." that drivers that are untouched for some period and have no
mainline users can/should go away. (Is "extrapolation" as strong as
"implication", or has it subjective interpretation added? My dictionary
isn't accurate enough for that question.) But it seems there is
something with my logic or you not saying exactly what you actually
mean. (Did I miss any option? If yes it might be covered by a problem in
my logic.)

Having said that, even in the question at hand (i.e. what is the better
compromise for mapping the inter-channel hardware limitations to
software policy in the pac9685 driver) the idea "let's inspecting device
trees present in the kernel" doesn't work, because for this driver there
are none, too. (It might be used by a mainline machine via ACPI, but
this is even less possible to consider for our judgements than a device
tree with such a device and no user but the sysfs PWM interface.)

> While it's certainly unfortunate that these don't seem to be used, I see
> no reason why we should remove them. They don't create much of a
> maintenance burden, so I'm fine with keeping them in the hopes that
> users may still show up at some point.

The problem I have with them is that I expect your voice of dissent when
I find the time to improve the rounding behaviour of these drivers.
My ultimate goal is to make the PWM framework a system where consumers
can rely on a consistent behaviour of the API and a way to actually
order what they need and get it. I'm not entirely sure we agree that
we're not there yet.

> > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder
> > if it makes sense to check the machine.dts files if some of the PMWs are
> > completely unused. Do you consider status = "okay" a use that we have to
> > retain even if the node has no phandle?
> 
> A PWM controller may be in use via sysfs even if it has no phandle.

Yeah, I expected you will say that. (And I agree.)

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 --]

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-11 10:34                           ` Uwe Kleine-König
@ 2020-12-14 14:28                             ` Thierry Reding
  2020-12-14 16:09                               ` Clemens Gruber
  2020-12-14 16:27                               ` Sven Van Asbroeck
  0 siblings, 2 replies; 32+ messages in thread
From: Thierry Reding @ 2020-12-14 14:28 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Sven Van Asbroeck, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

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

On Fri, Dec 11, 2020 at 11:34:54AM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote:
> > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote:
> > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> > > > Like I said, that's not what I was saying. I was merely saying that if
> > > > there aren't any use-cases that current users rely on that would be
> > > > broken by using this simpler implementation, then I'm okay with it, even
> > > > if it's less flexible than a more complicated implementation. It should
> > > > be possible to determine what the current users are by inspecting device
> > > > trees present in the kernel. Anything outside the kernel isn't something
> > > > we need to consider, as usual.
> > > 
> > > If "users in mainline" is the criteria that's a word.
> > 
> > I didn't say "users in mainline", I said "use-cases". What I don't want
> > to happen is for this change under discussion to break any existing use-
> > cases of any existing users in the kernel. I said that we can determine
> > what the existing users are by looking at which device trees use the
> > compatible strings that the driver matches on.
> > 
> > > So you agree we remove the following drivers?:
> > > 
> > >  - pwm-hibvt.c
> > >    Last driver specific change in Feb 2019, no mainline user
> > >  - pwm-sprd.c
> > >    Last driver specific change in Aug 2019, no mainline user
> > 
> > No, that's an extrapolation of what I was saying above. Drivers with no
> > apparent users are a separate topic, so don't conflate it with the issue
> > at hand.
> 
> I cannot follow (and I think that's the problem between us and why those
> conflicts happen between us). For me it's a logic consequence of
> "Anything outside the kernel isn't something we need to consider, as
> usual." that drivers that are untouched for some period and have no
> mainline users can/should go away. (Is "extrapolation" as strong as
> "implication", or has it subjective interpretation added? My dictionary
> isn't accurate enough for that question.) But it seems there is
> something with my logic or you not saying exactly what you actually
> mean. (Did I miss any option? If yes it might be covered by a problem in
> my logic.)

To me this is not as black and white as it seems to be for you. First I
wasn't talking about unused drivers, but about use-cases that are not
represented in mainline. Secondly I didn't say anything about removing
support for use-cases that weren't in use upstream. All I said was that
I didn't want any changes to regress existing use-cases.

"Guessing" how that statement reflects on my thoughts about unused
drivers is extrapolation. Your logic implication could've been correct,
but in this case it wasn't because I consider a driver that was
upstreamed to be part of the kernel, and people invested a fair amount
of work to get it to that point, so I'm in no hurry to get rid of them.
Instead, I prefer to give people the benefit of the doubt and assume
that they had planned to get users upstream and for some reason just
haven't gotten around to it.

On the other hand, almost 18-24 months without activity is quite long. A
compromise that works well for me is to keep drivers, even unused ones,
as long as they're not getting in the way.

> Having said that, even in the question at hand (i.e. what is the better
> compromise for mapping the inter-channel hardware limitations to
> software policy in the pac9685 driver) the idea "let's inspecting device
> trees present in the kernel" doesn't work, because for this driver there
> are none, too. (It might be used by a mainline machine via ACPI, but
> this is even less possible to consider for our judgements than a device
> tree with such a device and no user but the sysfs PWM interface.)

Perhaps Clemens and Sven can shed some light into how this driver is
being used. There clearly seem to be people interested in this driver,
so why are there no consumers of this upstream. What's keeping people
from upstreaming device trees that make use of this?

> > While it's certainly unfortunate that these don't seem to be used, I see
> > no reason why we should remove them. They don't create much of a
> > maintenance burden, so I'm fine with keeping them in the hopes that
> > users may still show up at some point.
> 
> The problem I have with them is that I expect your voice of dissent when
> I find the time to improve the rounding behaviour of these drivers.
> My ultimate goal is to make the PWM framework a system where consumers
> can rely on a consistent behaviour of the API and a way to actually
> order what they need and get it. I'm not entirely sure we agree that
> we're not there yet.

I don't think we entirely agree that the framework is currently
inconsistent. As you and others have shown in various threads, one way
of rounding isn't necessarily always the best way. Some consumers expect
a certain accuracy and don't care whether we over- or undershoot. Some
controllers may give more accurate results when rounding up than when
rounding down and for others rounding to nearest is the best option. A
lot of consumers don't care all that much about rounding as long as the
duty-cycle/period ratio is reasonably close.

So there are a lot of variables in all of this and I don't think we can
fix the rules so that they work for everyone. That's why I think we need
to stick with the status quo that does work for pretty much everyone so
far by being not overly strict and then extend that if we have to (i.e.
if we come across a use-case where the current status quo doesn't work).

> > > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder
> > > if it makes sense to check the machine.dts files if some of the PMWs are
> > > completely unused. Do you consider status = "okay" a use that we have to
> > > retain even if the node has no phandle?
> > 
> > A PWM controller may be in use via sysfs even if it has no phandle.
> 
> Yeah, I expected you will say that. (And I agree.)

sysfs on the PWM side does complicate things a little bit, but on the
other hand it's not very different from other consumers. For any change
at the API level (such as adding rounding modes, if we have to) we just
need to make sure this can be represented using the sysfs interface as
well.

And by the way, as an additional argument against removing seemingly
unused drivers: it's technically possible to instantiate a PCA9685 from
userspace using sysfs, and there might be people relying on this to get
their job done. Again, there are many shades of grey here and lots of
unknowns, and extrapolation doesn't work very well under those
circumstances.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-14 14:28                             ` Thierry Reding
@ 2020-12-14 16:09                               ` Clemens Gruber
  2020-12-14 16:27                               ` Sven Van Asbroeck
  1 sibling, 0 replies; 32+ messages in thread
From: Clemens Gruber @ 2020-12-14 16:09 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Uwe Kleine-König, Sven Van Asbroeck, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

On Mon, Dec 14, 2020 at 03:28:24PM +0100, Thierry Reding wrote:
> On Fri, Dec 11, 2020 at 11:34:54AM +0100, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote:
> > > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote:
> > > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote:
> > > > > Like I said, that's not what I was saying. I was merely saying that if
> > > > > there aren't any use-cases that current users rely on that would be
> > > > > broken by using this simpler implementation, then I'm okay with it, even
> > > > > if it's less flexible than a more complicated implementation. It should
> > > > > be possible to determine what the current users are by inspecting device
> > > > > trees present in the kernel. Anything outside the kernel isn't something
> > > > > we need to consider, as usual.
> > > > 
> > > > If "users in mainline" is the criteria that's a word.
> > > 
> > > I didn't say "users in mainline", I said "use-cases". What I don't want
> > > to happen is for this change under discussion to break any existing use-
> > > cases of any existing users in the kernel. I said that we can determine
> > > what the existing users are by looking at which device trees use the
> > > compatible strings that the driver matches on.
> > > 
> > > > So you agree we remove the following drivers?:
> > > > 
> > > >  - pwm-hibvt.c
> > > >    Last driver specific change in Feb 2019, no mainline user
> > > >  - pwm-sprd.c
> > > >    Last driver specific change in Aug 2019, no mainline user
> > > 
> > > No, that's an extrapolation of what I was saying above. Drivers with no
> > > apparent users are a separate topic, so don't conflate it with the issue
> > > at hand.
> > 
> > I cannot follow (and I think that's the problem between us and why those
> > conflicts happen between us). For me it's a logic consequence of
> > "Anything outside the kernel isn't something we need to consider, as
> > usual." that drivers that are untouched for some period and have no
> > mainline users can/should go away. (Is "extrapolation" as strong as
> > "implication", or has it subjective interpretation added? My dictionary
> > isn't accurate enough for that question.) But it seems there is
> > something with my logic or you not saying exactly what you actually
> > mean. (Did I miss any option? If yes it might be covered by a problem in
> > my logic.)
> 
> To me this is not as black and white as it seems to be for you. First I
> wasn't talking about unused drivers, but about use-cases that are not
> represented in mainline. Secondly I didn't say anything about removing
> support for use-cases that weren't in use upstream. All I said was that
> I didn't want any changes to regress existing use-cases.
> 
> "Guessing" how that statement reflects on my thoughts about unused
> drivers is extrapolation. Your logic implication could've been correct,
> but in this case it wasn't because I consider a driver that was
> upstreamed to be part of the kernel, and people invested a fair amount
> of work to get it to that point, so I'm in no hurry to get rid of them.
> Instead, I prefer to give people the benefit of the doubt and assume
> that they had planned to get users upstream and for some reason just
> haven't gotten around to it.
> 
> On the other hand, almost 18-24 months without activity is quite long. A
> compromise that works well for me is to keep drivers, even unused ones,
> as long as they're not getting in the way.
> 
> > Having said that, even in the question at hand (i.e. what is the better
> > compromise for mapping the inter-channel hardware limitations to
> > software policy in the pac9685 driver) the idea "let's inspecting device
> > trees present in the kernel" doesn't work, because for this driver there
> > are none, too. (It might be used by a mainline machine via ACPI, but
> > this is even less possible to consider for our judgements than a device
> > tree with such a device and no user but the sysfs PWM interface.)
> 
> Perhaps Clemens and Sven can shed some light into how this driver is
> being used. There clearly seem to be people interested in this driver,
> so why are there no consumers of this upstream. What's keeping people
> from upstreaming device trees that make use of this?

Our DT using the pca9685 is for an embedded board within a product and
that board within is not sold alone.
That's the reason why I did not upstream it yet, because I did not know
if it is acceptable to upstream DTs for boards that are not really of
great use to other people, because they can't (easily) get the hardware,
unless they buy a big beer dispensing system.
If that's not an issue then I am willig to upstream it of course.

Clemens

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-14 14:28                             ` Thierry Reding
  2020-12-14 16:09                               ` Clemens Gruber
@ 2020-12-14 16:27                               ` Sven Van Asbroeck
  2020-12-14 16:44                                 ` Sven Van Asbroeck
  1 sibling, 1 reply; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-14 16:27 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Uwe Kleine-König, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi Thierry,

On Mon, Dec 14, 2020 at 9:28 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
>
> Perhaps Clemens and Sven can shed some light into how this driver is
> being used. There clearly seem to be people interested in this driver,
> so why are there no consumers of this upstream. What's keeping people
> from upstreaming device trees that make use of this?
>

There are many reasons why a driver may not appear in the devicetree.
In my specific case, I backported the PCA9685 driver to a 4.1 Android vendor
kernel. This is too far behind to upstream. Also, the company regards the
devicetree as a trade secret, which it is entitled to do, as devicetrees
tend to be dual-licensed (GPL and MIT).

More generally, I believe that the PCA9685 is quite popular in the Raspberry
Pi world. Raspi devicetrees are not part of mainline, for various reasons
that we don't need to get into here.

Example:
https://learn.adafruit.com/adafruit-16-channel-servo-driver-with-raspberry-pi

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API
  2020-12-14 16:27                               ` Sven Van Asbroeck
@ 2020-12-14 16:44                                 ` Sven Van Asbroeck
  0 siblings, 0 replies; 32+ messages in thread
From: Sven Van Asbroeck @ 2020-12-14 16:44 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Uwe Kleine-König, Clemens Gruber, linux-pwm, Lee Jones,
	Linux Kernel Mailing List, Mika Westerberg, David Jander

Hi Thierry,

On Mon, Dec 14, 2020 at 9:28 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
>
> Perhaps Clemens and Sven can shed some light into how this driver is
> being used. There clearly seem to be people interested in this driver,
> so why are there no consumers of this upstream. What's keeping people
> from upstreaming device trees that make use of this?
>

Also, there's this section in the driver:

#ifdef CONFIG_ACPI
static const struct acpi_device_id pca9685_acpi_ids[] = {
{ "INT3492", 0 },
{ /* sentinel */ },
};
MODULE_DEVICE_TABLE(acpi, pca9685_acpi_ids);
#endif

Which means there might be arm64 or intel devices out there that define
presence of this chip via an ACPI tree. Which exists only inside bioses
and would not show up in any devicetree.

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2020-12-14 16:45 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-07 19:36 [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-12-07 19:36 ` [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
2020-12-07 19:36 ` [PATCH v4 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
2020-12-07 19:38 ` [PATCH v4 4/4] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
2020-12-07 22:00 ` [PATCH v4 1/4] pwm: pca9685: Switch to atomic API Uwe Kleine-König
2020-12-07 22:34   ` Sven Van Asbroeck
2020-12-07 23:24     ` Clemens Gruber
2020-12-08  9:17     ` Uwe Kleine-König
2020-12-07 23:13   ` Clemens Gruber
2020-12-08  9:10     ` Uwe Kleine-König
2020-12-08 10:12       ` Clemens Gruber
2020-12-08 13:44         ` Thierry Reding
2020-12-08 14:44           ` Sven Van Asbroeck
2020-12-08 16:57             ` Thierry Reding
2020-12-08 18:15               ` Sven Van Asbroeck
2020-12-08 20:25                 ` Uwe Kleine-König
2020-12-08 18:26               ` Uwe Kleine-König
2020-12-08 20:54                 ` Clemens Gruber
2020-12-09 17:02                 ` Thierry Reding
2020-12-10  9:01                   ` Uwe Kleine-König
2020-12-10 17:10                     ` Thierry Reding
2020-12-10 20:39                       ` Uwe Kleine-König
2020-12-11  8:33                         ` Thierry Reding
2020-12-11 10:34                           ` Uwe Kleine-König
2020-12-14 14:28                             ` Thierry Reding
2020-12-14 16:09                               ` Clemens Gruber
2020-12-14 16:27                               ` Sven Van Asbroeck
2020-12-14 16:44                                 ` Sven Van Asbroeck
2020-12-10 20:54                       ` Clemens Gruber
2020-12-10 21:37                         ` Sven Van Asbroeck
2020-12-07 23:22 ` Sven Van Asbroeck
2020-12-07 23:56   ` Clemens Gruber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.