* [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
@ 2020-11-24 18:10 Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Clemens Gruber @ 2020-11-24 18:10 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 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 | 251 +++++++++++++++++++-------------------
1 file changed, 128 insertions(+), 123 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..4cfbf1467f15 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,7 @@
struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
- int period_ns;
+ int prescale;
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -87,6 +87,54 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}
+static void pca9685_pwm_full_off(struct pca9685 *pca, int index)
+{
+ int reg;
+
+ /*
+ * Set the full OFF bit to cause the PWM channel to be always off.
+ * The full OFF bit has precedence over the other register values.
+ */
+
+ if (index >= PCA9685_MAXCHAN)
+ reg = PCA9685_ALL_LED_OFF_H;
+ else
+ reg = LED_N_OFF_H(index);
+
+ regmap_write(pca->regmap, reg, LED_FULL);
+}
+
+static void pca9685_pwm_full_on(struct pca9685 *pca, int index)
+{
+ int reg;
+
+ /*
+ * Clear the OFF registers (including the full OFF bit) and set
+ * the full ON bit to cause the PWM channel to be always on.
+ */
+
+ if (index >= PCA9685_MAXCHAN)
+ reg = PCA9685_ALL_LED_OFF_L;
+ else
+ reg = LED_N_OFF_L(index);
+
+ regmap_write(pca->regmap, reg, 0);
+
+ if (index >= PCA9685_MAXCHAN)
+ reg = PCA9685_ALL_LED_OFF_H;
+ else
+ reg = LED_N_OFF_H(index);
+
+ regmap_write(pca->regmap, reg, 0);
+
+ if (index >= PCA9685_MAXCHAN)
+ reg = PCA9685_ALL_LED_ON_H;
+ else
+ reg = LED_N_ON_H(index);
+
+ regmap_write(pca->regmap, reg, LED_FULL);
+}
+
#if IS_ENABLED(CONFIG_GPIOLIB)
static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
{
@@ -141,31 +189,27 @@ static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
struct pwm_device *pwm = &pca->chip.pwms[offset];
unsigned int value;
- regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
+ regmap_read(pca->regmap, LED_N_OFF_H(pwm->hwpwm), &value);
- return value & LED_FULL;
+ return !(value & LED_FULL);
}
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_full_on(pca, offset);
+ else
+ pca9685_pwm_full_off(pca, offset);
}
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_full_off(pca, offset);
pm_runtime_put(pca->chip.dev);
pca9685_pwm_clear_inuse(pca, offset);
}
@@ -246,18 +290,53 @@ 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 int val, duty;
+ int reg;
+
+ /* Calculate (chip-wide) period from cached prescale value */
+ state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+ (pca->prescale + 1);
+
+ /* The (per-channel) polarity is fixed */
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ /* Read out current duty cycle and enabled state */
+ reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
+ LED_N_OFF_H(pwm->hwpwm);
+ regmap_read(pca->regmap, reg, &val);
+ duty = (val & 0xf) << 8;
+
+ state->enabled = !(val & LED_FULL);
+
+ reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
+ LED_N_OFF_L(pwm->hwpwm);
+ regmap_read(pca->regmap, reg, &val);
+ duty |= (val & 0xff);
+
+ if (duty < PCA9685_COUNTER_RANGE) {
+ duty *= state->period;
+ state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
+ } else
+ state->duty_cycle = 0;
+}
+
+static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pca9685 *pca = to_pca(chip);
- unsigned long long duty;
+ unsigned long long duty, prescale;
unsigned int reg;
- int prescale;
- if (period_ns != pca->period_ns) {
- prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
- PCA9685_COUNTER_RANGE * 1000) - 1;
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EOPNOTSUPP;
+ prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
+ PCA9685_COUNTER_RANGE * 1000) - 1;
+ if (prescale != pca->prescale) {
if (prescale >= PCA9685_PRESCALE_MIN &&
prescale <= PCA9685_PRESCALE_MAX) {
/*
@@ -270,12 +349,12 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
pca9685_set_sleep_mode(pca, true);
/* Change the chip-wide output frequency */
- regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
+ regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
/* Wake the chip up */
pca9685_set_sleep_mode(pca, false);
- pca->period_ns = period_ns;
+ pca->prescale = (int)prescale;
} else {
dev_err(chip->dev,
"prescaler not set: period out of bounds!\n");
@@ -283,46 +362,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
}
}
- if (duty_ns < 1) {
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
+ if (!state->enabled || state->duty_cycle < 1) {
+ pca9685_pwm_full_off(pca, pwm->hwpwm);
return 0;
}
- if (duty_ns == period_ns) {
- /* Clear both OFF registers */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- /* Set the full ON bit */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
+ if (state->duty_cycle == state->period) {
+ pca9685_pwm_full_on(pca, pwm->hwpwm);
return 0;
}
- duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
- duty = DIV_ROUND_UP_ULL(duty, period_ns);
+ duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
+ duty = DIV_ROUND_UP_ULL(duty, state->period);
if (pwm->hwpwm >= PCA9685_MAXCHAN)
reg = PCA9685_ALL_LED_OFF_L;
@@ -349,64 +400,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct pca9685 *pca = to_pca(chip);
- unsigned int reg;
-
- /*
- * The PWM subsystem does not support a pre-delay.
- * So, set the ON-timeout to 0
- */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_L;
- else
- reg = LED_N_ON_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0);
-
- /*
- * Clear the full-off bit.
- * It has precedence over the others and must be off.
- */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
-
- return 0;
-}
-
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct pca9685 *pca = to_pca(chip);
- unsigned int reg;
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
- /* Clear the LED_OFF counter. */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-}
-
static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
@@ -422,15 +415,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
- pca9685_pwm_disable(chip, pwm);
+ pca9685_pwm_full_off(pca, pwm->hwpwm);
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 +453,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 +496,21 @@ 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);
+ pca->prescale = PCA9685_PRESCALE_DEF;
+ regmap_write(pca->regmap, PCA9685_PRESCALE, pca->prescale);
+ 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 +522,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] 12+ messages in thread
* [PATCH v3 2/4] pwm: pca9685: Set full OFF bits in probe
2020-11-24 18:10 [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
@ 2020-11-24 18:10 ` Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
2020-11-25 3:49 ` [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
2 siblings, 0 replies; 12+ messages in thread
From: Clemens Gruber @ 2020-11-24 18:10 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 4cfbf1467f15..2b82b35ff0de 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -475,9 +475,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] 12+ messages in thread
* [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times
2020-11-24 18:10 [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
@ 2020-11-24 18:10 ` Clemens Gruber
2020-11-25 3:51 ` Sven Van Asbroeck
2020-11-25 3:49 ` [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
2 siblings, 1 reply; 12+ messages in thread
From: Clemens Gruber @ 2020-11-24 18:10 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 v1:
- Rebased
drivers/pwm/pwm-pca9685.c | 35 ++++++++++++++++++++++++++++++++++-
1 file changed, 34 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 2b82b35ff0de..37c80bc8edcf 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -75,6 +75,7 @@ struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
int prescale;
+ bool staggered_outputs;
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -329,7 +330,7 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct pca9685 *pca = to_pca(chip);
unsigned long long duty, prescale;
- unsigned int reg;
+ unsigned int on, off, reg;
if (state->polarity != PWM_POLARITY_NORMAL)
return -EOPNOTSUPP;
@@ -375,6 +376,32 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
duty = DIV_ROUND_UP_ULL(duty, state->period);
+ if (pca->staggered_outputs) {
+ if (pwm->hwpwm < PCA9685_MAXCHAN) {
+ /*
+ * To reduce EMI, the ON times of each channel are
+ * spread out evenly within the counter range, while
+ * still maintaining the configured duty cycle
+ */
+ on = pwm->hwpwm * PCA9685_COUNTER_RANGE /
+ PCA9685_MAXCHAN;
+ off = (on + duty) % PCA9685_COUNTER_RANGE;
+ regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm),
+ on & 0xff);
+ regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm),
+ (on >> 8) & 0xf);
+ regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm),
+ off & 0xff);
+ regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
+ (off >> 8) & 0xf);
+ return 0;
+ }
+
+ /* No staggering possible if "all LEDs" channel is used */
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+ }
+
if (pwm->hwpwm >= PCA9685_MAXCHAN)
reg = PCA9685_ALL_LED_OFF_L;
else
@@ -470,6 +497,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
regmap_write(pca->regmap, PCA9685_MODE2, reg);
+ pca->staggered_outputs = device_property_read_bool(
+ &client->dev, "staggered-outputs");
+
/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
regmap_read(pca->regmap, PCA9685_MODE1, ®);
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
@@ -478,6 +508,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] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-24 18:10 [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
@ 2020-11-25 3:49 ` Sven Van Asbroeck
2020-11-25 8:35 ` Clemens Gruber
2 siblings, 1 reply; 12+ messages in thread
From: Sven Van Asbroeck @ 2020-11-25 3:49 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, I checked the patch against the datasheet register definitions.
More constructive feedback below.
On Tue, Nov 24, 2020 at 1:10 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 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 | 251 +++++++++++++++++++-------------------
> 1 file changed, 128 insertions(+), 123 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..4cfbf1467f15 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,7 @@
> struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> - int period_ns;
> + int prescale;
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -87,6 +87,54 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +static void pca9685_pwm_full_off(struct pca9685 *pca, int index)
> +{
> + int reg;
> +
> + /*
> + * Set the full OFF bit to cause the PWM channel to be always off.
> + * The full OFF bit has precedence over the other register values.
> + */
> +
> + if (index >= PCA9685_MAXCHAN)
> + reg = PCA9685_ALL_LED_OFF_H;
> + else
> + reg = LED_N_OFF_H(index);
> +
> + regmap_write(pca->regmap, reg, LED_FULL);
> +}
> +
> +static void pca9685_pwm_full_on(struct pca9685 *pca, int index)
> +{
> + int reg;
> +
> + /*
> + * Clear the OFF registers (including the full OFF bit) and set
> + * the full ON bit to cause the PWM channel to be always on.
> + */
> +
> + if (index >= PCA9685_MAXCHAN)
> + reg = PCA9685_ALL_LED_OFF_L;
> + else
> + reg = LED_N_OFF_L(index);
> +
> + regmap_write(pca->regmap, reg, 0);
> +
> + if (index >= PCA9685_MAXCHAN)
> + reg = PCA9685_ALL_LED_OFF_H;
> + else
> + reg = LED_N_OFF_H(index);
> +
> + regmap_write(pca->regmap, reg, 0);
> +
> + if (index >= PCA9685_MAXCHAN)
> + reg = PCA9685_ALL_LED_ON_H;
> + else
> + reg = LED_N_ON_H(index);
> +
> + regmap_write(pca->regmap, reg, LED_FULL);
> +}
> +
> #if IS_ENABLED(CONFIG_GPIOLIB)
> static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> {
> @@ -141,31 +189,27 @@ static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> struct pwm_device *pwm = &pca->chip.pwms[offset];
> unsigned int value;
>
> - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> + regmap_read(pca->regmap, LED_N_OFF_H(pwm->hwpwm), &value);
>
> - return value & LED_FULL;
> + return !(value & LED_FULL);
> }
>
> 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_full_on(pca, offset);
> + else
> + pca9685_pwm_full_off(pca, offset);
> }
>
> 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_full_off(pca, offset);
> pm_runtime_put(pca->chip.dev);
> pca9685_pwm_clear_inuse(pca, offset);
> }
> @@ -246,18 +290,53 @@ 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 int val, duty;
> + int reg;
> +
> + /* Calculate (chip-wide) period from cached prescale value */
> + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> + (pca->prescale + 1);
> +
> + /* The (per-channel) polarity is fixed */
> + state->polarity = PWM_POLARITY_NORMAL;
> +
> + /* Read out current duty cycle and enabled state */
> + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
> + LED_N_OFF_H(pwm->hwpwm);
> + regmap_read(pca->regmap, reg, &val);
The datasheet I found for this chip indicates that every ALL_LED_XXX register
is write-only. Those registers cannot be read back from the chip.
Datasheet "Rev. 4 - 16 April 2015"
> + duty = (val & 0xf) << 8;
> +
> + state->enabled = !(val & LED_FULL);
> +
> + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
> + LED_N_OFF_L(pwm->hwpwm);
> + regmap_read(pca->regmap, reg, &val);
> + duty |= (val & 0xff);
> +
> + if (duty < PCA9685_COUNTER_RANGE) {
How can duty >= 4096 ?
> + duty *= state->period;
> + state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
is this calculation correct?
imagine led_on = 0 (default), and led_off = 4095
then the led is off for one single tick per cycle
but the above formula would calculate it as full on (period == duty_cycle)?
> + } else
> + state->duty_cycle = 0;
what if the full on bit is set. would this function return the correct
duty cycle?
> +}
> +
> +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> {
> struct pca9685 *pca = to_pca(chip);
> - unsigned long long duty;
> + unsigned long long duty, prescale;
> unsigned int reg;
> - int prescale;
>
> - if (period_ns != pca->period_ns) {
> - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> - PCA9685_COUNTER_RANGE * 1000) - 1;
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EOPNOTSUPP;
>
> + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> + PCA9685_COUNTER_RANGE * 1000) - 1;
> + if (prescale != pca->prescale) {
> if (prescale >= PCA9685_PRESCALE_MIN &&
> prescale <= PCA9685_PRESCALE_MAX) {
> /*
> @@ -270,12 +349,12 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> pca9685_set_sleep_mode(pca, true);
>
> /* Change the chip-wide output frequency */
> - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
>
> /* Wake the chip up */
> pca9685_set_sleep_mode(pca, false);
>
> - pca->period_ns = period_ns;
> + pca->prescale = (int)prescale;
> } else {
> dev_err(chip->dev,
> "prescaler not set: period out of bounds!\n");
> @@ -283,46 +362,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> }
> }
>
> - if (duty_ns < 1) {
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> + if (!state->enabled || state->duty_cycle < 1) {
> + pca9685_pwm_full_off(pca, pwm->hwpwm);
> return 0;
> }
>
> - if (duty_ns == period_ns) {
> - /* Clear both OFF registers */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - /* Set the full ON bit */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> + if (state->duty_cycle == state->period) {
> + pca9685_pwm_full_on(pca, pwm->hwpwm);
> return 0;
> }
>
> - duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> - duty = DIV_ROUND_UP_ULL(duty, period_ns);
> + duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> + duty = DIV_ROUND_UP_ULL(duty, state->period);
is there a rounding issue here?
imagine period = 10000, duty_cycle = 9999
then the above formula says duty = 4095/4096
but in reality the requested duty is much closer to full on!
same issue in reverse when period = 10000, duty_cycle = 1
suggestion: do a DIV_ROUND_CLOSEST to calculate the closest duty,
then check against 0 and 4096 to see if full off / full on.
this also prevents led_off and led_on to be programmed with the same
value because of subtle rounding errors (not allowed as per datasheet)
>
> if (pwm->hwpwm >= PCA9685_MAXCHAN)
> reg = PCA9685_ALL_LED_OFF_L;
> @@ -349,64 +400,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - /*
> - * The PWM subsystem does not support a pre-delay.
> - * So, set the ON-timeout to 0
> - */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_L;
> - else
> - reg = LED_N_ON_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0);
> -
> - /*
> - * Clear the full-off bit.
> - * It has precedence over the others and must be off.
> - */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> -
> - return 0;
> -}
> -
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - /* Clear the LED_OFF counter. */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -}
> -
> static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
> @@ -422,15 +415,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
>
> - pca9685_pwm_disable(chip, pwm);
> + pca9685_pwm_full_off(pca, pwm->hwpwm);
> 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 +453,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 +496,21 @@ 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);
> + pca->prescale = PCA9685_PRESCALE_DEF;
> + regmap_write(pca->regmap, PCA9685_PRESCALE, pca->prescale);
> + 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 +522,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] 12+ messages in thread
* Re: [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times
2020-11-24 18:10 ` [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
@ 2020-11-25 3:51 ` Sven Van Asbroeck
2020-11-25 8:40 ` Clemens Gruber
0 siblings, 1 reply; 12+ messages in thread
From: Sven Van Asbroeck @ 2020-11-25 3:51 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
On Tue, Nov 24, 2020 at 1:10 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> 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 v1:
> - Rebased
>
> drivers/pwm/pwm-pca9685.c | 35 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 2b82b35ff0de..37c80bc8edcf 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -75,6 +75,7 @@ struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> int prescale;
> + bool staggered_outputs;
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -329,7 +330,7 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct pca9685 *pca = to_pca(chip);
> unsigned long long duty, prescale;
> - unsigned int reg;
> + unsigned int on, off, reg;
>
> if (state->polarity != PWM_POLARITY_NORMAL)
> return -EOPNOTSUPP;
> @@ -375,6 +376,32 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> duty = DIV_ROUND_UP_ULL(duty, state->period);
>
> + if (pca->staggered_outputs) {
> + if (pwm->hwpwm < PCA9685_MAXCHAN) {
> + /*
> + * To reduce EMI, the ON times of each channel are
> + * spread out evenly within the counter range, while
> + * still maintaining the configured duty cycle
> + */
> + on = pwm->hwpwm * PCA9685_COUNTER_RANGE /
> + PCA9685_MAXCHAN;
> + off = (on + duty) % PCA9685_COUNTER_RANGE;
Will pwm_get_state() still return the correct duty cycle in case
of staggered outputs?
> + regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm),
> + on & 0xff);
> + regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm),
> + (on >> 8) & 0xf);
> + regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm),
> + off & 0xff);
> + regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
> + (off >> 8) & 0xf);
> + return 0;
> + }
> +
> + /* No staggering possible if "all LEDs" channel is used */
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> + }
> +
> if (pwm->hwpwm >= PCA9685_MAXCHAN)
> reg = PCA9685_ALL_LED_OFF_L;
> else
> @@ -470,6 +497,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>
> regmap_write(pca->regmap, PCA9685_MODE2, reg);
>
> + pca->staggered_outputs = device_property_read_bool(
> + &client->dev, "staggered-outputs");
> +
> /* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> regmap_read(pca->regmap, PCA9685_MODE1, ®);
> reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> @@ -478,6 +508,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 [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 3:49 ` [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
@ 2020-11-25 8:35 ` Clemens Gruber
2020-11-25 14:04 ` Clemens Gruber
0 siblings, 1 reply; 12+ messages in thread
From: Clemens Gruber @ 2020-11-25 8:35 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
On Tue, Nov 24, 2020 at 10:49:27PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, I checked the patch against the datasheet register definitions.
> More constructive feedback below.
>
> On Tue, Nov 24, 2020 at 1:10 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 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 | 251 +++++++++++++++++++-------------------
> > 1 file changed, 128 insertions(+), 123 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..4cfbf1467f15 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,7 @@
> > struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > - int period_ns;
> > + int prescale;
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -87,6 +87,54 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > return container_of(chip, struct pca9685, chip);
> > }
> >
> > +static void pca9685_pwm_full_off(struct pca9685 *pca, int index)
> > +{
> > + int reg;
> > +
> > + /*
> > + * Set the full OFF bit to cause the PWM channel to be always off.
> > + * The full OFF bit has precedence over the other register values.
> > + */
> > +
> > + if (index >= PCA9685_MAXCHAN)
> > + reg = PCA9685_ALL_LED_OFF_H;
> > + else
> > + reg = LED_N_OFF_H(index);
> > +
> > + regmap_write(pca->regmap, reg, LED_FULL);
> > +}
> > +
> > +static void pca9685_pwm_full_on(struct pca9685 *pca, int index)
> > +{
> > + int reg;
> > +
> > + /*
> > + * Clear the OFF registers (including the full OFF bit) and set
> > + * the full ON bit to cause the PWM channel to be always on.
> > + */
> > +
> > + if (index >= PCA9685_MAXCHAN)
> > + reg = PCA9685_ALL_LED_OFF_L;
> > + else
> > + reg = LED_N_OFF_L(index);
> > +
> > + regmap_write(pca->regmap, reg, 0);
> > +
> > + if (index >= PCA9685_MAXCHAN)
> > + reg = PCA9685_ALL_LED_OFF_H;
> > + else
> > + reg = LED_N_OFF_H(index);
> > +
> > + regmap_write(pca->regmap, reg, 0);
> > +
> > + if (index >= PCA9685_MAXCHAN)
> > + reg = PCA9685_ALL_LED_ON_H;
> > + else
> > + reg = LED_N_ON_H(index);
> > +
> > + regmap_write(pca->regmap, reg, LED_FULL);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > {
> > @@ -141,31 +189,27 @@ static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > struct pwm_device *pwm = &pca->chip.pwms[offset];
> > unsigned int value;
> >
> > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > + regmap_read(pca->regmap, LED_N_OFF_H(pwm->hwpwm), &value);
> >
> > - return value & LED_FULL;
> > + return !(value & LED_FULL);
> > }
> >
> > 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_full_on(pca, offset);
> > + else
> > + pca9685_pwm_full_off(pca, offset);
> > }
> >
> > 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_full_off(pca, offset);
> > pm_runtime_put(pca->chip.dev);
> > pca9685_pwm_clear_inuse(pca, offset);
> > }
> > @@ -246,18 +290,53 @@ 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 int val, duty;
> > + int reg;
> > +
> > + /* Calculate (chip-wide) period from cached prescale value */
> > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > + (pca->prescale + 1);
> > +
> > + /* The (per-channel) polarity is fixed */
> > + state->polarity = PWM_POLARITY_NORMAL;
> > +
> > + /* Read out current duty cycle and enabled state */
> > + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
> > + LED_N_OFF_H(pwm->hwpwm);
> > + regmap_read(pca->regmap, reg, &val);
>
> The datasheet I found for this chip indicates that every ALL_LED_XXX register
> is write-only. Those registers cannot be read back from the chip.
>
> Datasheet "Rev. 4 - 16 April 2015"
Thanks, good catch! Would you agree that we should just return 0 duty
cycle and disabled state if the "all LEDs" channel is used?
>
> > + duty = (val & 0xf) << 8;
> > +
> > + state->enabled = !(val & LED_FULL);
> > +
> > + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
> > + LED_N_OFF_L(pwm->hwpwm);
> > + regmap_read(pca->regmap, reg, &val);
> > + duty |= (val & 0xff);
> > +
> > + if (duty < PCA9685_COUNTER_RANGE) {
>
> How can duty >= 4096 ?
>
> > + duty *= state->period;
> > + state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
>
> is this calculation correct?
> imagine led_on = 0 (default), and led_off = 4095
> then the led is off for one single tick per cycle
> but the above formula would calculate it as full on (period == duty_cycle)?
>
> > + } else
> > + state->duty_cycle = 0;
>
> what if the full on bit is set. would this function return the correct
> duty cycle?
No.. Sorry for not noticing this myself earlier!
>
>
> > +}
> > +
> > +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > - unsigned long long duty;
> > + unsigned long long duty, prescale;
> > unsigned int reg;
> > - int prescale;
> >
> > - if (period_ns != pca->period_ns) {
> > - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> > - PCA9685_COUNTER_RANGE * 1000) - 1;
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EOPNOTSUPP;
> >
> > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > + if (prescale != pca->prescale) {
> > if (prescale >= PCA9685_PRESCALE_MIN &&
> > prescale <= PCA9685_PRESCALE_MAX) {
> > /*
> > @@ -270,12 +349,12 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > pca9685_set_sleep_mode(pca, true);
> >
> > /* Change the chip-wide output frequency */
> > - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> >
> > /* Wake the chip up */
> > pca9685_set_sleep_mode(pca, false);
> >
> > - pca->period_ns = period_ns;
> > + pca->prescale = (int)prescale;
> > } else {
> > dev_err(chip->dev,
> > "prescaler not set: period out of bounds!\n");
> > @@ -283,46 +362,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > }
> > }
> >
> > - if (duty_ns < 1) {
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > + if (!state->enabled || state->duty_cycle < 1) {
> > + pca9685_pwm_full_off(pca, pwm->hwpwm);
> > return 0;
> > }
> >
> > - if (duty_ns == period_ns) {
> > - /* Clear both OFF registers */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - /* Set the full ON bit */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > + if (state->duty_cycle == state->period) {
> > + pca9685_pwm_full_on(pca, pwm->hwpwm);
> > return 0;
> > }
> >
> > - duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> > - duty = DIV_ROUND_UP_ULL(duty, period_ns);
> > + duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> > + duty = DIV_ROUND_UP_ULL(duty, state->period);
>
> is there a rounding issue here?
> imagine period = 10000, duty_cycle = 9999
> then the above formula says duty = 4095/4096
> but in reality the requested duty is much closer to full on!
> same issue in reverse when period = 10000, duty_cycle = 1
> suggestion: do a DIV_ROUND_CLOSEST to calculate the closest duty,
> then check against 0 and 4096 to see if full off / full on.
> this also prevents led_off and led_on to be programmed with the same
> value because of subtle rounding errors (not allowed as per datasheet)
Yes that would be more accurate, good suggestion!
>
> >
> > if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > reg = PCA9685_ALL_LED_OFF_L;
> > @@ -349,64 +400,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > return 0;
> > }
> >
> > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - struct pca9685 *pca = to_pca(chip);
> > - unsigned int reg;
> > -
> > - /*
> > - * The PWM subsystem does not support a pre-delay.
> > - * So, set the ON-timeout to 0
> > - */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_L;
> > - else
> > - reg = LED_N_ON_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > -
> > - /*
> > - * Clear the full-off bit.
> > - * It has precedence over the others and must be off.
> > - */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > -
> > - return 0;
> > -}
> > -
> > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - struct pca9685 *pca = to_pca(chip);
> > - unsigned int reg;
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - /* Clear the LED_OFF counter. */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -}
> > -
> > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > @@ -422,15 +415,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> >
> > - pca9685_pwm_disable(chip, pwm);
> > + pca9685_pwm_full_off(pca, pwm->hwpwm);
> > 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 +453,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 +496,21 @@ 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);
> > + pca->prescale = PCA9685_PRESCALE_DEF;
> > + regmap_write(pca->regmap, PCA9685_PRESCALE, pca->prescale);
> > + 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 +522,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] 12+ messages in thread
* Re: [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times
2020-11-25 3:51 ` Sven Van Asbroeck
@ 2020-11-25 8:40 ` Clemens Gruber
0 siblings, 0 replies; 12+ messages in thread
From: Clemens Gruber @ 2020-11-25 8:40 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
On Tue, Nov 24, 2020 at 10:51:23PM -0500, Sven Van Asbroeck wrote:
> On Tue, Nov 24, 2020 at 1:10 PM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > 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 v1:
> > - Rebased
> >
> > drivers/pwm/pwm-pca9685.c | 35 ++++++++++++++++++++++++++++++++++-
> > 1 file changed, 34 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 2b82b35ff0de..37c80bc8edcf 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -75,6 +75,7 @@ struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > int prescale;
> > + bool staggered_outputs;
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -329,7 +330,7 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > {
> > struct pca9685 *pca = to_pca(chip);
> > unsigned long long duty, prescale;
> > - unsigned int reg;
> > + unsigned int on, off, reg;
> >
> > if (state->polarity != PWM_POLARITY_NORMAL)
> > return -EOPNOTSUPP;
> > @@ -375,6 +376,32 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> > duty = DIV_ROUND_UP_ULL(duty, state->period);
> >
> > + if (pca->staggered_outputs) {
> > + if (pwm->hwpwm < PCA9685_MAXCHAN) {
> > + /*
> > + * To reduce EMI, the ON times of each channel are
> > + * spread out evenly within the counter range, while
> > + * still maintaining the configured duty cycle
> > + */
> > + on = pwm->hwpwm * PCA9685_COUNTER_RANGE /
> > + PCA9685_MAXCHAN;
> > + off = (on + duty) % PCA9685_COUNTER_RANGE;
>
> Will pwm_get_state() still return the correct duty cycle in case
> of staggered outputs?
No, but it should. I will improve the get_state function in the next
version to cover all cases.
Thanks for your input. This would have been a mess without your feedback
;)
>
> > + regmap_write(pca->regmap, LED_N_ON_L(pwm->hwpwm),
> > + on & 0xff);
> > + regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm),
> > + (on >> 8) & 0xf);
> > + regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm),
> > + off & 0xff);
> > + regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm),
> > + (off >> 8) & 0xf);
> > + return 0;
> > + }
> > +
> > + /* No staggering possible if "all LEDs" channel is used */
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> > + }
> > +
> > if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > reg = PCA9685_ALL_LED_OFF_L;
> > else
> > @@ -470,6 +497,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >
> > regmap_write(pca->regmap, PCA9685_MODE2, reg);
> >
> > + pca->staggered_outputs = device_property_read_bool(
> > + &client->dev, "staggered-outputs");
> > +
> > /* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> > regmap_read(pca->regmap, PCA9685_MODE1, ®);
> > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > @@ -478,6 +508,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 [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 8:35 ` Clemens Gruber
@ 2020-11-25 14:04 ` Clemens Gruber
2020-11-25 15:04 ` Sven Van Asbroeck
0 siblings, 1 reply; 12+ messages in thread
From: Clemens Gruber @ 2020-11-25 14:04 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
On Wed, Nov 25, 2020 at 09:35:08AM +0100, Clemens Gruber wrote:
> On Tue, Nov 24, 2020 at 10:49:27PM -0500, Sven Van Asbroeck wrote:
> > Hi Clemens, I checked the patch against the datasheet register definitions.
> > More constructive feedback below.
> >
> > On Tue, Nov 24, 2020 at 1:10 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 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 | 251 +++++++++++++++++++-------------------
> > > 1 file changed, 128 insertions(+), 123 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 4a55dc18656c..4cfbf1467f15 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,7 @@
> > > struct pca9685 {
> > > struct pwm_chip chip;
> > > struct regmap *regmap;
> > > - int period_ns;
> > > + int prescale;
> > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > struct mutex lock;
> > > struct gpio_chip gpio;
> > > @@ -87,6 +87,54 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > > return container_of(chip, struct pca9685, chip);
> > > }
> > >
> > > +static void pca9685_pwm_full_off(struct pca9685 *pca, int index)
> > > +{
> > > + int reg;
> > > +
> > > + /*
> > > + * Set the full OFF bit to cause the PWM channel to be always off.
> > > + * The full OFF bit has precedence over the other register values.
> > > + */
> > > +
> > > + if (index >= PCA9685_MAXCHAN)
> > > + reg = PCA9685_ALL_LED_OFF_H;
> > > + else
> > > + reg = LED_N_OFF_H(index);
> > > +
> > > + regmap_write(pca->regmap, reg, LED_FULL);
> > > +}
> > > +
> > > +static void pca9685_pwm_full_on(struct pca9685 *pca, int index)
> > > +{
> > > + int reg;
> > > +
> > > + /*
> > > + * Clear the OFF registers (including the full OFF bit) and set
> > > + * the full ON bit to cause the PWM channel to be always on.
> > > + */
> > > +
> > > + if (index >= PCA9685_MAXCHAN)
> > > + reg = PCA9685_ALL_LED_OFF_L;
> > > + else
> > > + reg = LED_N_OFF_L(index);
> > > +
> > > + regmap_write(pca->regmap, reg, 0);
> > > +
> > > + if (index >= PCA9685_MAXCHAN)
> > > + reg = PCA9685_ALL_LED_OFF_H;
> > > + else
> > > + reg = LED_N_OFF_H(index);
> > > +
> > > + regmap_write(pca->regmap, reg, 0);
> > > +
> > > + if (index >= PCA9685_MAXCHAN)
> > > + reg = PCA9685_ALL_LED_ON_H;
> > > + else
> > > + reg = LED_N_ON_H(index);
> > > +
> > > + regmap_write(pca->regmap, reg, LED_FULL);
> > > +}
> > > +
> > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > {
> > > @@ -141,31 +189,27 @@ static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > unsigned int value;
> > >
> > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > + regmap_read(pca->regmap, LED_N_OFF_H(pwm->hwpwm), &value);
> > >
> > > - return value & LED_FULL;
> > > + return !(value & LED_FULL);
> > > }
> > >
> > > 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_full_on(pca, offset);
> > > + else
> > > + pca9685_pwm_full_off(pca, offset);
> > > }
> > >
> > > 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_full_off(pca, offset);
> > > pm_runtime_put(pca->chip.dev);
> > > pca9685_pwm_clear_inuse(pca, offset);
> > > }
> > > @@ -246,18 +290,53 @@ 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 int val, duty;
> > > + int reg;
> > > +
> > > + /* Calculate (chip-wide) period from cached prescale value */
> > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > > + (pca->prescale + 1);
> > > +
> > > + /* The (per-channel) polarity is fixed */
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > + /* Read out current duty cycle and enabled state */
> > > + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H :
> > > + LED_N_OFF_H(pwm->hwpwm);
> > > + regmap_read(pca->regmap, reg, &val);
> >
> > The datasheet I found for this chip indicates that every ALL_LED_XXX register
> > is write-only. Those registers cannot be read back from the chip.
> >
> > Datasheet "Rev. 4 - 16 April 2015"
>
> Thanks, good catch! Would you agree that we should just return 0 duty
> cycle and disabled state if the "all LEDs" channel is used?
Some additional thoughts:
The only other way to determine if the ALL channel is set a certain way
would be to remember it in the pca9685 struct.
(As reading all registers every time .get_state is called would not be a
good idea)
It's really not great that we have to support the "all LEDs" channel at
all but dropping it is probably out of the question.
--
I noticed something else that does not look great:
Let's say you set up pwm channel 0 with a period of 5000000 and after
that you set up pwm channel 1 with a period of 40000000.
So far so good. But if you now set pwm channel 0's period to 5000000
again, the period stays at 40000000. (Tested with /sys/class/pwm)
On the other hand one could argue that the user needs to know that the
PCA9685 has only one prescaler and switching between different periods
on different channels is therefore not recommended?
>
> >
> > > + duty = (val & 0xf) << 8;
> > > +
> > > + state->enabled = !(val & LED_FULL);
> > > +
> > > + reg = pwm->hwpwm >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L :
> > > + LED_N_OFF_L(pwm->hwpwm);
> > > + regmap_read(pca->regmap, reg, &val);
> > > + duty |= (val & 0xff);
> > > +
> > > + if (duty < PCA9685_COUNTER_RANGE) {
> >
> > How can duty >= 4096 ?
> >
> > > + duty *= state->period;
> > > + state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> >
> > is this calculation correct?
> > imagine led_on = 0 (default), and led_off = 4095
> > then the led is off for one single tick per cycle
> > but the above formula would calculate it as full on (period == duty_cycle)?
> >
> > > + } else
> > > + state->duty_cycle = 0;
> >
> > what if the full on bit is set. would this function return the correct
> > duty cycle?
>
> No.. Sorry for not noticing this myself earlier!
>
> >
> >
> > > +}
> > > +
> > > +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > {
> > > struct pca9685 *pca = to_pca(chip);
> > > - unsigned long long duty;
> > > + unsigned long long duty, prescale;
> > > unsigned int reg;
> > > - int prescale;
> > >
> > > - if (period_ns != pca->period_ns) {
> > > - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> > > - PCA9685_COUNTER_RANGE * 1000) - 1;
> > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > + return -EOPNOTSUPP;
> > >
> > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > + if (prescale != pca->prescale) {
> > > if (prescale >= PCA9685_PRESCALE_MIN &&
> > > prescale <= PCA9685_PRESCALE_MAX) {
> > > /*
> > > @@ -270,12 +349,12 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > pca9685_set_sleep_mode(pca, true);
> > >
> > > /* Change the chip-wide output frequency */
> > > - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> > >
> > > /* Wake the chip up */
> > > pca9685_set_sleep_mode(pca, false);
> > >
> > > - pca->period_ns = period_ns;
> > > + pca->prescale = (int)prescale;
> > > } else {
> > > dev_err(chip->dev,
> > > "prescaler not set: period out of bounds!\n");
> > > @@ -283,46 +362,18 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > }
> > > }
> > >
> > > - if (duty_ns < 1) {
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_H;
> > > - else
> > > - reg = LED_N_OFF_H(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, LED_FULL);
> > > -
> > > + if (!state->enabled || state->duty_cycle < 1) {
> > > + pca9685_pwm_full_off(pca, pwm->hwpwm);
> > > return 0;
> > > }
> > >
> > > - if (duty_ns == period_ns) {
> > > - /* Clear both OFF registers */
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_L;
> > > - else
> > > - reg = LED_N_OFF_L(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, 0x0);
> > > -
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_H;
> > > - else
> > > - reg = LED_N_OFF_H(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, 0x0);
> > > -
> > > - /* Set the full ON bit */
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_ON_H;
> > > - else
> > > - reg = LED_N_ON_H(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, LED_FULL);
> > > -
> > > + if (state->duty_cycle == state->period) {
> > > + pca9685_pwm_full_on(pca, pwm->hwpwm);
> > > return 0;
> > > }
> > >
> > > - duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> > > - duty = DIV_ROUND_UP_ULL(duty, period_ns);
> > > + duty = (PCA9685_COUNTER_RANGE - 1) * state->duty_cycle;
> > > + duty = DIV_ROUND_UP_ULL(duty, state->period);
> >
> > is there a rounding issue here?
> > imagine period = 10000, duty_cycle = 9999
> > then the above formula says duty = 4095/4096
> > but in reality the requested duty is much closer to full on!
> > same issue in reverse when period = 10000, duty_cycle = 1
> > suggestion: do a DIV_ROUND_CLOSEST to calculate the closest duty,
> > then check against 0 and 4096 to see if full off / full on.
> > this also prevents led_off and led_on to be programmed with the same
> > value because of subtle rounding errors (not allowed as per datasheet)
>
> Yes that would be more accurate, good suggestion!
>
> >
> > >
> > > if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > reg = PCA9685_ALL_LED_OFF_L;
> > > @@ -349,64 +400,6 @@ static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > > return 0;
> > > }
> > >
> > > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > -{
> > > - struct pca9685 *pca = to_pca(chip);
> > > - unsigned int reg;
> > > -
> > > - /*
> > > - * The PWM subsystem does not support a pre-delay.
> > > - * So, set the ON-timeout to 0
> > > - */
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_ON_L;
> > > - else
> > > - reg = LED_N_ON_L(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, 0);
> > > -
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_ON_H;
> > > - else
> > > - reg = LED_N_ON_H(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, 0);
> > > -
> > > - /*
> > > - * Clear the full-off bit.
> > > - * It has precedence over the others and must be off.
> > > - */
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_H;
> > > - else
> > > - reg = LED_N_OFF_H(pwm->hwpwm);
> > > -
> > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > > -{
> > > - struct pca9685 *pca = to_pca(chip);
> > > - unsigned int reg;
> > > -
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_H;
> > > - else
> > > - reg = LED_N_OFF_H(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, LED_FULL);
> > > -
> > > - /* Clear the LED_OFF counter. */
> > > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > > - reg = PCA9685_ALL_LED_OFF_L;
> > > - else
> > > - reg = LED_N_OFF_L(pwm->hwpwm);
> > > -
> > > - regmap_write(pca->regmap, reg, 0x0);
> > > -}
> > > -
> > > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > > {
> > > struct pca9685 *pca = to_pca(chip);
> > > @@ -422,15 +415,14 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > > {
> > > struct pca9685 *pca = to_pca(chip);
> > >
> > > - pca9685_pwm_disable(chip, pwm);
> > > + pca9685_pwm_full_off(pca, pwm->hwpwm);
> > > 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 +453,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 +496,21 @@ 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);
> > > + pca->prescale = PCA9685_PRESCALE_DEF;
> > > + regmap_write(pca->regmap, PCA9685_PRESCALE, pca->prescale);
> > > + 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 +522,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 for your time!
Clemens
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 14:04 ` Clemens Gruber
@ 2020-11-25 15:04 ` Sven Van Asbroeck
2020-11-25 17:28 ` Clemens Gruber
0 siblings, 1 reply; 12+ messages in thread
From: Sven Van Asbroeck @ 2020-11-25 15:04 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
On Wed, Nov 25, 2020 at 3:35 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> >
> > The datasheet I found for this chip indicates that every ALL_LED_XXX register
> > is write-only. Those registers cannot be read back from the chip.
> >
> > Datasheet "Rev. 4 - 16 April 2015"
>
> Thanks, good catch! Would you agree that we should just return 0 duty
> cycle and disabled state if the "all LEDs" channel is used?
I think get_state() for the all led channel should just return -ENOTSUPP,
if the pwm core will allow that.
Because it's the truth, the chip does not support reading from the all led
channel.
When we start buffering the all led state, we make the code much
more complex, and again we'll run into all sorts of subtle corner cases.
> > > +
> > > + if (duty < PCA9685_COUNTER_RANGE) {
> >
> > How can duty >= 4096 ?
> >
> > > + duty *= state->period;
> > > + state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> >
> > is this calculation correct?
> > imagine led_on = 0 (default), and led_off = 4095
> > then the led is off for one single tick per cycle
> > but the above formula would calculate it as full on (period == duty_cycle)?
I just wanted to make sure you hadn't overlooked the two comments above.
--
Each time I read the code, my thoughts get interrupted by all this
if hwpwm >= MAXCHAN then { one macro } else { another macro } business
which is spread around in the code !
Same thing with the splitting of the value between H and L registers.
Same thing with the LED_FULL bit.
Maybe the code will be much more readable if we do the following?
- keep pca9685_pwm_full_off/full_on but rename to pca9685_pwm_set_full_off/on
- create pca9685_pwm_is_full_off/on
- create pca9685_pwm_set_on_time/set_off_time
Then LED_FULL, >= MAXCHAN, and register splits are fully confined to
these functions, and we can call them freely in the rest of the code,
without getting confused by these details.
--
> I noticed something else that does not look great:
> Let's say you set up pwm channel 0 with a period of 5000000 and after
> that you set up pwm channel 1 with a period of 40000000.
> So far so good. But if you now set pwm channel 0's period to 5000000
> again, the period stays at 40000000. (Tested with /sys/class/pwm)
>
If the driver isn't buggy, this should work ok. Changing the period on one
channel changes the global prescale, which in turn changes the period on
every other channel. But that's ok, because the ON/OFF times are relative
to a 4096-tick counter, so all duty cycles are preserved.
Example:
1. SET channel 0 : duty 50_000 period 100_000 (real duty cycle = 0.5)
2. SET channel 1 : duty 50_000 period 200_000 (real duty cycle = 0.25)
3. GET channel 0 : duty 100_000 period 200_000 (real duty cycle STILL 0.5)
I think this is acceptable behaviour.
--
I have been thinking about how this patch caches the global prescaler.
There is a possible synchronization issue. Sysfs will always work ok, because
it uses a mutex to protect accesses to pwm_set_state(), which means set_state()
will never be called concurrently.
But I do not think there's any protection when the driver is used as a client
in the devicetree, like so:
&i2c1 {
my_pca: pwm@0 {
compatible = "nxp,pca9685-pwm";
reg = <0>;
};
};
acme_device@0 {
pwms = <&my_pca 2 10000000>;
};
acme_device@1 {
pwms = <&my_pca 1 20000000>;
};
For most pwm drivers, this is fine, because their registers are strictly
separated: writes to channel 0 and 1 do not touch the same registers.
But in our case, because of the cached prescale, things can go very wrong.
I think this can be solved by simply not caching prescale. Everything then
stays on the stack, and the last thread to set the prescaler wins.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 15:04 ` Sven Van Asbroeck
@ 2020-11-25 17:28 ` Clemens Gruber
2020-11-25 19:46 ` Sven Van Asbroeck
0 siblings, 1 reply; 12+ messages in thread
From: Clemens Gruber @ 2020-11-25 17:28 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 Wed, Nov 25, 2020 at 10:04:32AM -0500, Sven Van Asbroeck wrote:
> On Wed, Nov 25, 2020 at 3:35 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > >
> > > The datasheet I found for this chip indicates that every ALL_LED_XXX register
> > > is write-only. Those registers cannot be read back from the chip.
> > >
> > > Datasheet "Rev. 4 - 16 April 2015"
> >
> > Thanks, good catch! Would you agree that we should just return 0 duty
> > cycle and disabled state if the "all LEDs" channel is used?
>
> I think get_state() for the all led channel should just return -ENOTSUPP,
> if the pwm core will allow that.
>
> Because it's the truth, the chip does not support reading from the all led
> channel.
I thought about that too, but get_state is a void function and there is
no error/errno variable in pwm_state that could signal a problem.
>
> When we start buffering the all led state, we make the code much
> more complex, and again we'll run into all sorts of subtle corner cases.
What's the lesser evil in your opinion, always returning 0 duty and
disabled for the ALL channel or caching it?
>
> > > > +
> > > > + if (duty < PCA9685_COUNTER_RANGE) {
> > >
> > > How can duty >= 4096 ?
> > >
> > > > + duty *= state->period;
> > > > + state->duty_cycle = duty / (PCA9685_COUNTER_RANGE - 1);
> > >
> > > is this calculation correct?
> > > imagine led_on = 0 (default), and led_off = 4095
> > > then the led is off for one single tick per cycle
> > > but the above formula would calculate it as full on (period == duty_cycle)?
>
> I just wanted to make sure you hadn't overlooked the two comments above.
Yes I saw them, thanks. You are suggesting that we change the scaling of
the relative duty cycle from 0..4095 to 0..4096 and in .apply_state we
do full OFF if 0 and full ON if 4096, so we still have a 4095 duty state
with a single tick off?
Then in .get_state: duty_cycle = (duty * period) / COUNTER_RANGE
Please let me know if I misunderstood you.
>
> --
>
> Each time I read the code, my thoughts get interrupted by all this
> if hwpwm >= MAXCHAN then { one macro } else { another macro } business
> which is spread around in the code !
>
> Same thing with the splitting of the value between H and L registers.
> Same thing with the LED_FULL bit.
Yes, this is indeed confusing.
>
> Maybe the code will be much more readable if we do the following?
>
> - keep pca9685_pwm_full_off/full_on but rename to pca9685_pwm_set_full_off/on
> - create pca9685_pwm_is_full_off/on
> - create pca9685_pwm_set_on_time/set_off_time
>
> Then LED_FULL, >= MAXCHAN, and register splits are fully confined to
> these functions, and we can call them freely in the rest of the code,
> without getting confused by these details.
Great idea!
>
> --
>
> > I noticed something else that does not look great:
> > Let's say you set up pwm channel 0 with a period of 5000000 and after
> > that you set up pwm channel 1 with a period of 40000000.
> > So far so good. But if you now set pwm channel 0's period to 5000000
> > again, the period stays at 40000000. (Tested with /sys/class/pwm)
> >
>
> If the driver isn't buggy, this should work ok. Changing the period on one
> channel changes the global prescale, which in turn changes the period on
> every other channel. But that's ok, because the ON/OFF times are relative
> to a 4096-tick counter, so all duty cycles are preserved.
>
> Example:
> 1. SET channel 0 : duty 50_000 period 100_000 (real duty cycle = 0.5)
> 2. SET channel 1 : duty 50_000 period 200_000 (real duty cycle = 0.25)
> 3. GET channel 0 : duty 100_000 period 200_000 (real duty cycle STILL 0.5)
>
> I think this is acceptable behaviour.
Yes the effect of a global period change to the duty cycles of other
channels is acceptable but that was not what I meant.
I meant that with sysfs, the period does not change if the new value is
the same as the last time that channel's period was set.
See my example above.
But we probably can't do anything about that.
>
> --
>
> I have been thinking about how this patch caches the global prescaler.
> There is a possible synchronization issue. Sysfs will always work ok, because
> it uses a mutex to protect accesses to pwm_set_state(), which means set_state()
> will never be called concurrently.
>
> But I do not think there's any protection when the driver is used as a client
> in the devicetree, like so:
>
> &i2c1 {
> my_pca: pwm@0 {
> compatible = "nxp,pca9685-pwm";
> reg = <0>;
> };
> };
>
> acme_device@0 {
> pwms = <&my_pca 2 10000000>;
> };
>
> acme_device@1 {
> pwms = <&my_pca 1 20000000>;
> };
>
> For most pwm drivers, this is fine, because their registers are strictly
> separated: writes to channel 0 and 1 do not touch the same registers.
>
> But in our case, because of the cached prescale, things can go very wrong.
>
> I think this can be solved by simply not caching prescale. Everything then
> stays on the stack, and the last thread to set the prescaler wins.
OK, regmap accesses are protected with locks but what about the SLEEP
bit that needs to be set/cleared + sleep. Shouldn't we only allow one
thread at one time to change the prescaler of a chip? Otherwise, there
could be synchronization issues there too. (Possible writing to the
prescale register with the SLEEP bit unset by another thread)
If we drop the cache we would have to read the prescale register
whenever we need it (for every channel) but with the upcoming regmap
cache feature, this would probably be OK.
Do you think this should be solved in the same patch as the atomic API
conversion or can we do this in a follow-up patch?
Thanks,
Clemens
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 17:28 ` Clemens Gruber
@ 2020-11-25 19:46 ` Sven Van Asbroeck
2020-11-25 19:54 ` Sven Van Asbroeck
0 siblings, 1 reply; 12+ messages in thread
From: Sven Van Asbroeck @ 2020-11-25 19:46 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
On Wed, Nov 25, 2020 at 12:28 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> What's the lesser evil in your opinion, always returning 0 duty and
> disabled for the ALL channel or caching it?
>
I would definitely suggest returning a pwm_state consisting of all zeroes
for the "all led" channel, instead of caching stuff.
And I don't think it's in any way evil: from a user point of view, reading
back the "all led" channel state makes no sense. Why? Because setting
that channel is just a write to all 16 channels at once. And those channels
may change over time.
For example:
1. set all leds channel = enabled, 50% duty cycle
=> all 16 leds are enabled at 50% duty cycle
2. set led 0 = disabled
3. set led 1 = enabled, 70% duty cycle
4. imagine we now read back the "all leds" state, what should it return?
it makes no sense, because not all leds are in the same state !
>
> Please let me know if I misunderstood you.
>
All good, thanks !
>
> I meant that with sysfs, the period does not change if the new value is
> the same as the last time that channel's period was set.
> See my example above.
>
> But we probably can't do anything about that.
>
Oh I see what you mean. This is because the pwm core makes certain basic
assumptions about pwm devices. It assumes that channels are completely
independent, ie. setting channel X state won't influence channel Y.
And that's not the case for this chip.
I think the current pwm core behaviour is not ideal for us, but acceptable.
Let's get our own house in order before worrying about the core's behaviour.
>
> OK, regmap accesses are protected with locks but what about the SLEEP
> bit that needs to be set/cleared + sleep. Shouldn't we only allow one
> thread at one time to change the prescaler of a chip? Otherwise, there
> could be synchronization issues there too. (Possible writing to the
> prescale register with the SLEEP bit unset by another thread)
Good point. Luckily, the runtime_pm code is completely thread-safe.
It's quite common for drivers to use that code from multiple threads.
That's the cool thing about re-using existing kernel infrastructure,
you get all of that for free, and tested too.
You only reach the danger zone if you're doing things that the core
is completely not expecting. In this case, the core doesn't expect
that a pwm_apply() on one channel modifies any unprotected shared state.
>
> If we drop the cache we would have to read the prescale register
> whenever we need it (for every channel) but with the upcoming regmap
> cache feature, this would probably be OK.
We only need to read the prescale register during pwm_get_state() and
pwm_apply(), correct? I count many regmap_read()s in those functions.
One extra regmap_read() to retrieve the prescale, won't make much difference,
even when regmap cache is off.
>
> Do you think this should be solved in the same patch as the atomic API
> conversion or can we do this in a follow-up patch?
>
Suggestion: in the atomic API patch, do not cache prescale, just read it
out as needed. This is slightly slower, but I suspect that the code
will become shorter and simpler.
Then if you have time && motivation left, enable regmap cache.
But watch out... you'll probably have to invalidate the cache
each time the driver writes to an ALL_LED register. Because that
implies a write to 16 other registers.
But the efficiency saving is probably very limited, and may not be
worth the added complexity.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3 1/4] pwm: pca9685: Switch to atomic API
2020-11-25 19:46 ` Sven Van Asbroeck
@ 2020-11-25 19:54 ` Sven Van Asbroeck
0 siblings, 0 replies; 12+ messages in thread
From: Sven Van Asbroeck @ 2020-11-25 19:54 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
On Wed, Nov 25, 2020 at 2:46 PM Sven Van Asbroeck <thesven73@gmail.com> wrote:
>
> Then if you have time && motivation left, enable regmap cache.
In a follow-up patch, after we arrived at a correctly working driver.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-11-25 19:54 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-24 18:10 [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 2/4] pwm: pca9685: Set full OFF bits in probe Clemens Gruber
2020-11-24 18:10 ` [PATCH v3 3/4] pwm: pca9685: Support staggered output ON times Clemens Gruber
2020-11-25 3:51 ` Sven Van Asbroeck
2020-11-25 8:40 ` Clemens Gruber
2020-11-25 3:49 ` [PATCH v3 1/4] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
2020-11-25 8:35 ` Clemens Gruber
2020-11-25 14:04 ` Clemens Gruber
2020-11-25 15:04 ` Sven Van Asbroeck
2020-11-25 17:28 ` Clemens Gruber
2020-11-25 19:46 ` Sven Van Asbroeck
2020-11-25 19:54 ` Sven Van Asbroeck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).