LinuxLists.cc - [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

2020-12-07 19:40:53

Subject: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

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 <[email protected]>
---
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

2020-12-07 19:41:15

by Clemens Gruber

[permalink] [raw]

Subject: [PATCH v4 4/4] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property

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 <[email protected]>
---
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

2020-12-07 19:41:44

by Clemens Gruber

[permalink] [raw]

Subject: [PATCH v4 2/4] pwm: pca9685: Set full OFF bits in probe

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 <[email protected]>
---
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

2020-12-07 22:03:15

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

Attachments:

(No filename) (1.58 kB)
signature.asc (499.00 B)
Download all attachments

2020-12-08 09:21:15

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
> <[email protected]> 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

Attachments:

(No filename) (3.47 kB)
signature.asc (849.00 B)
Download all attachments

2020-12-08 18:17:45

by Sven Van Asbroeck

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

2020-12-10 21:41:15

by Sven Van Asbroeck

[permalink] [raw]

Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

On Thu, Dec 10, 2020 at 3:54 PM Clemens Gruber
<[email protected]> 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 !!

2020-12-11 12:32:17

by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH v4 1/4] pwm: pca9685: Switch to atomic API

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

Attachments:

(No filename) (1.99 kB)
signature.asc (849.00 B)
Download all attachments

2020-12-11 17:07:06

Hi Thierry,

On Mon, Dec 14, 2020 at 9:28 AM Thierry Reding <[email protected]> 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