* [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
@ 2020-12-15 21:22 Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 2/7] pwm: pca9685: Support hardware readout Clemens Gruber
` (7 more replies)
0 siblings, 8 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, 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
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Changes since v4:
- Patches split up
- Use a single set_duty function
- Improve readability / new macros
- Added a patch to restrict prescale changes to the first user
Changes since v3:
- Refactoring: Extracted common functions
- Read prescale register value instead of caching it
- Return all zeros and disabled for "all LEDs" channel state
- Improved duty calculation / mapping to 0..4096
Changes since v2:
- Always set default prescale value in probe
- Simplified probe code
- Inlined functions with one callsite
Changes since v1:
- Fixed a logic error
- Impoved PM runtime handling and fixed !CONFIG_PM
- Write default prescale reg value if invalid in probe
- Reuse full_off/_on functions throughout driver
- Use cached prescale value whenever possible
drivers/pwm/pwm-pca9685.c | 253 +++++++++++++-------------------------
1 file changed, 87 insertions(+), 166 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..1b5b5fb93b43 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -51,7 +51,6 @@
#define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */
#define PCA9685_COUNTER_RANGE 4096
-#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
#define PCA9685_OSC_CLOCK_MHZ 25 /* Internal oscillator with 25 MHz */
#define PCA9685_NUMREGS 0xFF
@@ -71,10 +70,14 @@
#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
+#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
+#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
+#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
+#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
+
struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
- int period_ns;
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -87,6 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}
+static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
+{
+ if (duty == 0) {
+ /* Set the full OFF bit, which has the highest precedence */
+ regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ } else if (duty >= PCA9685_COUNTER_RANGE) {
+ /* Set the full ON bit and clear the full OFF bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
+ regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+ } else {
+ /* Set OFF time (clears the full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
+ /* Clear the full ON bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), 0);
+ }
+}
+
+static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
+{
+ unsigned int off_h, val;
+
+ if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
+ /* Hardware readout not supported for "all LEDs" channel */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
+ if (off_h & LED_FULL) {
+ /* Full OFF bit is set */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
+ if (val & LED_FULL) {
+ /* Full ON bit is set */
+ return PCA9685_COUNTER_RANGE;
+ }
+
+ regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ return ((off_h & 0xf) << 8) | (val & 0xff);
+}
+
#if IS_ENABLED(CONFIG_GPIOLIB)
static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
{
@@ -138,34 +184,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- struct pwm_device *pwm = &pca->chip.pwms[offset];
- unsigned int value;
- regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
-
- return value & LED_FULL;
+ return pca9685_pwm_get_duty(pca, offset) != 0;
}
static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
int value)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- struct pwm_device *pwm = &pca->chip.pwms[offset];
- unsigned int on = value ? LED_FULL : 0;
-
- /* Clear both OFF registers */
- regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
- regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
- /* Set the full ON bit */
- regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
+ pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
}
static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- pca9685_pwm_gpio_set(gpio, offset, 0);
+ pca9685_pwm_set_duty(pca, offset, 0);
pm_runtime_put(pca->chip.dev);
pca9685_pwm_clear_inuse(pca, offset);
}
@@ -246,167 +281,56 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
}
}
-static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
- int duty_ns, int period_ns)
+static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pca9685 *pca = to_pca(chip);
- unsigned long long duty;
- unsigned int reg;
- int prescale;
-
- if (period_ns != pca->period_ns) {
- prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
- PCA9685_COUNTER_RANGE * 1000) - 1;
-
- if (prescale >= PCA9685_PRESCALE_MIN &&
- prescale <= PCA9685_PRESCALE_MAX) {
- /*
- * Putting the chip briefly into SLEEP mode
- * at this point won't interfere with the
- * pm_runtime framework, because the pm_runtime
- * state is guaranteed active here.
- */
- /* Put chip into sleep mode */
- pca9685_set_sleep_mode(pca, true);
-
- /* Change the chip-wide output frequency */
- regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
-
- /* Wake the chip up */
- pca9685_set_sleep_mode(pca, false);
-
- pca->period_ns = period_ns;
- } else {
- dev_err(chip->dev,
- "prescaler not set: period out of bounds!\n");
- return -EINVAL;
- }
- }
+ unsigned long long duty, prescale;
+ unsigned int val = 0;
- if (duty_ns < 1) {
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EOPNOTSUPP;
- regmap_write(pca->regmap, reg, LED_FULL);
-
- return 0;
+ prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
+ PCA9685_COUNTER_RANGE * 1000) - 1;
+ if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
+ dev_err(chip->dev, "prescaler not set: period out of bounds!\n");
+ return -EINVAL;
}
- if (duty_ns == period_ns) {
- /* Clear both OFF registers */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- /* Set the full ON bit */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
+ duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
+ duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
+ if (!state->enabled || duty < 1) {
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+ return 0;
+ } else if (duty == PCA9685_COUNTER_RANGE) {
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
return 0;
}
- duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
- duty = DIV_ROUND_UP_ULL(duty, period_ns);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, (int)duty & 0xff);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
-
- /* Clear the full ON bit, otherwise the set OFF time has no effect */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0);
-
- return 0;
-}
-
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct pca9685 *pca = to_pca(chip);
- unsigned int reg;
-
- /*
- * The PWM subsystem does not support a pre-delay.
- * So, set the ON-timeout to 0
- */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_L;
- else
- reg = LED_N_ON_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0);
+ regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ if (prescale != val) {
+ /*
+ * Putting the chip briefly into SLEEP mode
+ * at this point won't interfere with the
+ * pm_runtime framework, because the pm_runtime
+ * state is guaranteed active here.
+ */
+ /* Put chip into sleep mode */
+ pca9685_set_sleep_mode(pca, true);
- /*
- * Clear the full-off bit.
- * It has precedence over the others and must be off.
- */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
+ /* Change the chip-wide output frequency */
+ regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
- regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+ /* Wake the chip up */
+ pca9685_set_sleep_mode(pca, false);
+ }
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
return 0;
}
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct pca9685 *pca = to_pca(chip);
- unsigned int reg;
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
- /* Clear the LED_OFF counter. */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-}
-
static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
@@ -422,15 +346,13 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
- pca9685_pwm_disable(chip, pwm);
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
pm_runtime_put(chip->dev);
pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
}
static const struct pwm_ops pca9685_pwm_ops = {
- .enable = pca9685_pwm_enable,
- .disable = pca9685_pwm_disable,
- .config = pca9685_pwm_config,
+ .apply = pca9685_pwm_apply,
.request = pca9685_pwm_request,
.free = pca9685_pwm_free,
.owner = THIS_MODULE,
@@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
ret);
return ret;
}
- pca->period_ns = PCA9685_DEFAULT_PERIOD;
i2c_set_clientdata(client, pca);
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 2/7] pwm: pca9685: Support hardware readout
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 3/7] pwm: pca9685: Improve runtime PM behavior Clemens Gruber
` (6 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander, Clemens Gruber
Implements .get_state to read-out the current hardware state.
The hardware readout may return slightly different values than those
that were set in apply due to the limited range of possible prescale and
counter register values.
Also note that although the datasheet mentions 200 Hz as default
frequency when using the internal 25 MHz oscillator, the calculated
period from the default prescaler register setting of 30 is 5079040ns.
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 1b5b5fb93b43..b3398963c0ff 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -331,6 +331,46 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
+static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct pca9685 *pca = to_pca(chip);
+ unsigned long long duty;
+ unsigned int val;
+
+ /* 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);
+
+ /* The (per-channel) polarity is fixed */
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ if (pwm->hwpwm >= PCA9685_MAXCHAN) {
+ /*
+ * The "all LEDs" channel does not support HW readout
+ * Return 0 and disabled for backwards compatibility
+ */
+ state->duty_cycle = 0;
+ state->enabled = false;
+ return;
+ }
+
+ duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
+
+ state->enabled = !!duty;
+ if (!state->enabled) {
+ state->duty_cycle = 0;
+ return;
+ } else if (duty == PCA9685_COUNTER_RANGE) {
+ state->duty_cycle = state->period;
+ return;
+ }
+
+ duty *= state->period;
+ state->duty_cycle = duty / PCA9685_COUNTER_RANGE;
+}
+
static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
@@ -353,6 +393,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
static const struct pwm_ops pca9685_pwm_ops = {
.apply = pca9685_pwm_apply,
+ .get_state = pca9685_pwm_get_state,
.request = pca9685_pwm_request,
.free = pca9685_pwm_free,
.owner = THIS_MODULE,
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 3/7] pwm: pca9685: Improve runtime PM behavior
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 2/7] pwm: pca9685: Support hardware readout Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe Clemens Gruber
` (5 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander, Clemens Gruber
The chip does not come out of POR in active state but in sleep state.
To be sure (in case the bootloader woke it up) we force it to sleep in
probe.
On kernels without CONFIG_PM, we wake the chip in .probe and put it to
sleep in .remove.
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
drivers/pwm/pwm-pca9685.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index b3398963c0ff..7b14447f3c05 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -467,14 +467,19 @@ 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
+ * The chip comes out of power-up in the sleep state,
+ * but force it to sleep in case it was woken up before
*/
+ pca9685_set_sleep_mode(pca, true);
+ pm_runtime_set_suspended(&client->dev);
pm_runtime_enable(&client->dev);
+ if (!IS_ENABLED(CONFIG_PM)) {
+ /* Wake the chip up on non-PM environments */
+ pca9685_set_sleep_mode(pca, false);
+ }
+
return 0;
}
@@ -486,7 +491,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] 25+ messages in thread
* [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 2/7] pwm: pca9685: Support hardware readout Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 3/7] pwm: pca9685: Improve runtime PM behavior Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-17 4:02 ` Sven Van Asbroeck
2021-03-01 21:46 ` Uwe Kleine-König
2020-12-15 21:22 ` [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times Clemens Gruber
` (4 subsequent siblings)
7 siblings, 2 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander, Clemens Gruber
Reset the prescale and ON/OFF registers to their POR default state in
the probe function. Otherwise, the PWMs could still be active after a
watchdog reset and reboot, etc.
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
drivers/pwm/pwm-pca9685.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 7b14447f3c05..38aadaf50996 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -47,6 +47,7 @@
#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 */
@@ -446,9 +447,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
regmap_write(pca->regmap, PCA9685_MODE1, reg);
- /* Clear all "full off" bits */
+ /* Reset ON/OFF registers to HW defaults (only full OFF bit is set) */
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
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 */
@@ -470,8 +473,10 @@ static int pca9685_pwm_probe(struct i2c_client *client,
/*
* The chip comes out of power-up in the sleep state,
* but force it to sleep in case it was woken up before
+ * and set the default prescale value
*/
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);
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
` (2 preceding siblings ...)
2020-12-15 21:22 ` [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-17 4:02 ` Sven Van Asbroeck
2020-12-15 21:22 ` [PATCH v5 6/7] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
` (3 subsequent siblings)
7 siblings, 1 reply; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, 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>
---
drivers/pwm/pwm-pca9685.c | 62 +++++++++++++++++++++++++++++++--------
1 file changed, 50 insertions(+), 12 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 38aadaf50996..ff916980de49 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -79,6 +79,7 @@
struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
+ bool staggered_outputs;
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -93,45 +94,79 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
+ unsigned int on, off;
+
if (duty == 0) {
/* Set the full OFF bit, which has the highest precedence */
regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ return;
} else if (duty >= PCA9685_COUNTER_RANGE) {
/* Set the full ON bit and clear the full OFF bit */
regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
regmap_write(pca->regmap, REG_OFF_H(channel), 0);
- } else {
- /* Set OFF time (clears the full OFF bit) */
- regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
- regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
- /* Clear the full ON bit */
- regmap_write(pca->regmap, REG_ON_H(channel), 0);
+ return;
+ }
+
+ if (pca->staggered_outputs) {
+ if (channel < PCA9685_MAXCHAN) {
+ /*
+ * To reduce EMI, the ON times of each channel are
+ * spread out evenly within the counter range, while
+ * still maintaining the configured duty cycle
+ */
+ on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
+ off = (on + duty) % PCA9685_COUNTER_RANGE;
+ regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
+ regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+ regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
+ return;
+ }
+ /* No staggering possible if "all LEDs" channel is used */
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
}
+ /* Set OFF time (clears the full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
+ /* Clear the full ON bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), 0);
}
static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
{
- unsigned int off_h, val;
+ unsigned int off, on, val;
if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
/* Hardware readout not supported for "all LEDs" channel */
return 0;
}
- regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
- if (off_h & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+ if (off & LED_FULL) {
/* Full OFF bit is set */
return 0;
}
- regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
- if (val & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+ if (on & LED_FULL) {
/* Full ON bit is set */
return PCA9685_COUNTER_RANGE;
}
regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
- return ((off_h & 0xf) << 8) | (val & 0xff);
+ off = ((off & 0xf) << 8) | (val & 0xff);
+
+ if (pca->staggered_outputs) {
+ regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
+ on = ((on & 0xf) << 8) | (val & 0xff);
+
+ if (off >= on)
+ return off - on;
+ else
+ return off + PCA9685_COUNTER_RANGE - on;
+ }
+
+ return off;
}
#if IS_ENABLED(CONFIG_GPIOLIB)
@@ -442,6 +477,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
regmap_write(pca->regmap, PCA9685_MODE2, reg);
+ pca->staggered_outputs = device_property_read_bool(
+ &client->dev, "nxp,staggered-outputs");
+
/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
regmap_read(pca->regmap, PCA9685_MODE1, ®);
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 6/7] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
` (3 preceding siblings ...)
2020-12-15 21:22 ` [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users Clemens Gruber
` (2 subsequent siblings)
7 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander, Clemens Gruber
The pca9685 driver supports a new nxp,staggered-outputs property for
reduced current surges and EMI. This adds documentation for the new DT
property.
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt | 2 ++
1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
index f21b55c95738..fafe954369dc 100644
--- a/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/nxp,pca9685-pwm.txt
@@ -12,6 +12,8 @@ Optional properties:
- invert (bool): boolean to enable inverted logic
- open-drain (bool): boolean to configure outputs with open-drain structure;
if omitted use totem-pole structure
+ - nxp,staggered-outputs (bool): boolean to enable staggered output ON times to
+ minimize current surges and EMI
Example:
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
` (4 preceding siblings ...)
2020-12-15 21:22 ` [PATCH v5 6/7] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
@ 2020-12-15 21:22 ` Clemens Gruber
2020-12-17 4:03 ` Sven Van Asbroeck
2020-12-17 3:58 ` [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
2021-03-01 21:44 ` Uwe Kleine-König
7 siblings, 1 reply; 25+ messages in thread
From: Clemens Gruber @ 2020-12-15 21:22 UTC (permalink / raw)
To: linux-pwm
Cc: Thierry Reding, u.kleine-koenig, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander, Clemens Gruber
Previously, the last used PWM channel could change the global prescale
setting, even if other channels were already in use.
Fix it by only allowing the first user of the prescaler to change the
global chip-wide prescale setting. If there is more than one channel in
use, the prescale settings resulting from the chosen periods must match.
PWMs that are disabled or have a duty cycle of 0% or 100% are not
considered to be using the prescaler as they have the full OFF or full
ON bits set. This also applies to channels used as GPIOs.
Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
---
drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------
1 file changed, 44 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index ff916980de49..438492d4aed4 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -23,11 +23,11 @@
#include <linux/bitmap.h>
/*
- * Because the PCA9685 has only one prescaler per chip, changing the period of
- * one channel affects the period of all 16 PWM outputs!
- * However, the ratio between each configured duty cycle and the chip-wide
- * period remains constant, because the OFF time is set in proportion to the
- * counter range.
+ * Because the PCA9685 has only one prescaler per chip, only the first channel
+ * that uses the prescaler is allowed to change the prescale register.
+ * PWM channels requested afterwards must use a period that results in the same
+ * prescale setting as the one set by the first requested channel, unless they
+ * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON).
*/
#define PCA9685_MODE1 0x00
@@ -80,6 +80,8 @@ struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
bool staggered_outputs;
+ struct mutex prescaler_users_lock;
+ DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1);
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}
+/* This function is supposed to be called with the prescaler_users_lock held */
+static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel)
+{
+ /*
+ * A PWM channel may only change the prescaler if there are no users of
+ * the prescaler yet or that same channel is the only one in use.
+ */
+ return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) ||
+ (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 &&
+ test_bit(channel, pca->prescaler_users));
+}
+
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
unsigned int on, off;
@@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
+ mutex_lock(&pca->prescaler_users_lock);
+
if (!state->enabled || duty < 1) {
pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
- return 0;
+ goto prescaler_unused;
} else if (duty == PCA9685_COUNTER_RANGE) {
pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
- return 0;
+ goto prescaler_unused;
}
regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
if (prescale != val) {
+ if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) {
+ mutex_unlock(&pca->prescaler_users_lock);
+ dev_err(chip->dev,
+ "prescaler not set: already in use with different setting!\n");
+ return -EBUSY;
+ }
+
/*
* Putting the chip briefly into SLEEP mode
* at this point won't interfere with the
@@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
}
pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
+
+ set_bit(pwm->hwpwm, pca->prescaler_users);
+ mutex_unlock(&pca->prescaler_users_lock);
+ return 0;
+
+prescaler_unused:
+ clear_bit(pwm->hwpwm, pca->prescaler_users);
+ mutex_unlock(&pca->prescaler_users_lock);
return 0;
}
@@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
+ mutex_lock(&pca->prescaler_users_lock);
+ clear_bit(pwm->hwpwm, pca->prescaler_users);
pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+ mutex_unlock(&pca->prescaler_users_lock);
+
pm_runtime_put(chip->dev);
pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
}
@@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
i2c_set_clientdata(client, pca);
+ mutex_init(&pca->prescaler_users_lock);
+
regmap_read(pca->regmap, PCA9685_MODE2, ®);
if (device_property_read_bool(&client->dev, "invert"))
--
2.29.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
` (5 preceding siblings ...)
2020-12-15 21:22 ` [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users Clemens Gruber
@ 2020-12-17 3:58 ` Sven Van Asbroeck
2020-12-17 16:48 ` Clemens Gruber
2021-03-01 21:44 ` Uwe Kleine-König
7 siblings, 1 reply; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 3:58 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, this looks compact, simple and neat. I like it a lot !!
Few very minor nits below.
On Wed, Dec 16, 2020 at 7:53 AM 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
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> Changes since v4:
> - Patches split up
> - Use a single set_duty function
> - Improve readability / new macros
> - Added a patch to restrict prescale changes to the first user
>
> Changes since v3:
> - Refactoring: Extracted common functions
> - Read prescale register value instead of caching it
> - Return all zeros and disabled for "all LEDs" channel state
> - Improved duty calculation / mapping to 0..4096
>
> Changes since v2:
> - Always set default prescale value in probe
> - Simplified probe code
> - Inlined functions with one callsite
>
> Changes since v1:
> - Fixed a logic error
> - Impoved PM runtime handling and fixed !CONFIG_PM
> - Write default prescale reg value if invalid in probe
> - Reuse full_off/_on functions throughout driver
> - Use cached prescale value whenever possible
>
> drivers/pwm/pwm-pca9685.c | 253 +++++++++++++-------------------------
> 1 file changed, 87 insertions(+), 166 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..1b5b5fb93b43 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -51,7 +51,6 @@
> #define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */
>
> #define PCA9685_COUNTER_RANGE 4096
> -#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
> #define PCA9685_OSC_CLOCK_MHZ 25 /* Internal oscillator with 25 MHz */
>
> #define PCA9685_NUMREGS 0xFF
> @@ -71,10 +70,14 @@
> #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
>
> +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
> +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
> +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
> +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
Yes !!
> +
> struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> - int period_ns;
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -87,6 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> +{
Add brief function documentation to clarify that 'duty' sets the duty cycle
_ratio_ to 'duty/4096' on ? I.e.
duty == 2048 => duty cycle ratio = 2048/4096 = 50% on
duty == 4096 => duty cycle ratio = 4096/4086 = 100% on etc
> + if (duty == 0) {
> + /* Set the full OFF bit, which has the highest precedence */
> + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + } else if (duty >= PCA9685_COUNTER_RANGE) {
> + /* Set the full ON bit and clear the full OFF bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> + } else {
> + /* Set OFF time (clears the full OFF bit) */
> + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> + /* Clear the full ON bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> + }
> +}
> +
> +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> +{
> + unsigned int off_h, val;
> +
> + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> + /* Hardware readout not supported for "all LEDs" channel */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> + if (off_h & LED_FULL) {
I believe this may trigger bots which are monitoring patches on LKML:
if regmap_read() somehow fails, off_h will be used uninitialized.
Prevent by initializing off_h and val?
> + /* Full OFF bit is set */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> + if (val & LED_FULL) {
> + /* Full ON bit is set */
> + return PCA9685_COUNTER_RANGE;
> + }
> +
> + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> + return ((off_h & 0xf) << 8) | (val & 0xff);
> +}
> +
> #if IS_ENABLED(CONFIG_GPIOLIB)
> static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> {
> @@ -138,34 +184,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
> - struct pwm_device *pwm = &pca->chip.pwms[offset];
> - unsigned int value;
>
> - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> -
> - return value & LED_FULL;
> + return pca9685_pwm_get_duty(pca, offset) != 0;
> }
>
> static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> int value)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
> - struct pwm_device *pwm = &pca->chip.pwms[offset];
> - unsigned int on = value ? LED_FULL : 0;
> -
> - /* Clear both OFF registers */
> - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
>
> - /* Set the full ON bit */
> - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> }
>
> static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
>
> - pca9685_pwm_gpio_set(gpio, offset, 0);
> + pca9685_pwm_set_duty(pca, offset, 0);
> pm_runtime_put(pca->chip.dev);
> pca9685_pwm_clear_inuse(pca, offset);
> }
> @@ -246,167 +281,56 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> }
> }
>
> -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> - int duty_ns, int period_ns)
> +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> {
> struct pca9685 *pca = to_pca(chip);
> - unsigned long long duty;
> - unsigned int reg;
> - int prescale;
> -
> - if (period_ns != pca->period_ns) {
> - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> - PCA9685_COUNTER_RANGE * 1000) - 1;
> -
> - if (prescale >= PCA9685_PRESCALE_MIN &&
> - prescale <= PCA9685_PRESCALE_MAX) {
> - /*
> - * Putting the chip briefly into SLEEP mode
> - * at this point won't interfere with the
> - * pm_runtime framework, because the pm_runtime
> - * state is guaranteed active here.
> - */
> - /* Put chip into sleep mode */
> - pca9685_set_sleep_mode(pca, true);
> -
> - /* Change the chip-wide output frequency */
> - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> -
> - /* Wake the chip up */
> - pca9685_set_sleep_mode(pca, false);
> -
> - pca->period_ns = period_ns;
> - } else {
> - dev_err(chip->dev,
> - "prescaler not set: period out of bounds!\n");
> - return -EINVAL;
> - }
> - }
> + unsigned long long duty, prescale;
> + unsigned int val = 0;
>
> - if (duty_ns < 1) {
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EOPNOTSUPP;
>
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - return 0;
> + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> + PCA9685_COUNTER_RANGE * 1000) - 1;
> + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> + dev_err(chip->dev, "prescaler not set: period out of bounds!\n");
would "pwm not changed: period out of bounds" be a clearer error message here?
> + return -EINVAL;
> }
>
> - if (duty_ns == period_ns) {
> - /* Clear both OFF registers */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - /* Set the full ON bit */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
>
> + if (!state->enabled || duty < 1) {
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> + return 0;
> + } else if (duty == PCA9685_COUNTER_RANGE) {
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> return 0;
> }
>
> - duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> - duty = DIV_ROUND_UP_ULL(duty, period_ns);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, (int)duty & 0xff);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
> -
> - /* Clear the full ON bit, otherwise the set OFF time has no effect */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0);
> -
> - return 0;
> -}
> -
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - /*
> - * The PWM subsystem does not support a pre-delay.
> - * So, set the ON-timeout to 0
> - */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_L;
> - else
> - reg = LED_N_ON_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0);
> + regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> + if (prescale != val) {
> + /*
> + * Putting the chip briefly into SLEEP mode
> + * at this point won't interfere with the
> + * pm_runtime framework, because the pm_runtime
> + * state is guaranteed active here.
> + */
> + /* Put chip into sleep mode */
> + pca9685_set_sleep_mode(pca, true);
>
> - /*
> - * Clear the full-off bit.
> - * It has precedence over the others and must be off.
> - */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> + /* Change the chip-wide output frequency */
> + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
>
> - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> + /* Wake the chip up */
> + pca9685_set_sleep_mode(pca, false);
> + }
>
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> return 0;
> }
>
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - /* Clear the LED_OFF counter. */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -}
> -
> static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
> @@ -422,15 +346,13 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
>
> - pca9685_pwm_disable(chip, pwm);
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> pm_runtime_put(chip->dev);
> pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> }
>
> static const struct pwm_ops pca9685_pwm_ops = {
> - .enable = pca9685_pwm_enable,
> - .disable = pca9685_pwm_disable,
> - .config = pca9685_pwm_config,
> + .apply = pca9685_pwm_apply,
> .request = pca9685_pwm_request,
> .free = pca9685_pwm_free,
> .owner = THIS_MODULE,
> @@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> ret);
> return ret;
> }
> - pca->period_ns = PCA9685_DEFAULT_PERIOD;
>
> i2c_set_clientdata(client, pca);
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
2020-12-15 21:22 ` [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe Clemens Gruber
@ 2020-12-17 4:02 ` Sven Van Asbroeck
2020-12-17 17:45 ` Clemens Gruber
2021-03-01 21:46 ` Uwe Kleine-König
1 sibling, 1 reply; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 4:02 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, minor nit below.
On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> Reset the prescale and ON/OFF registers to their POR default state in
> the probe function. Otherwise, the PWMs could still be active after a
> watchdog reset and reboot, etc.
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> drivers/pwm/pwm-pca9685.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 7b14447f3c05..38aadaf50996 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -47,6 +47,7 @@
> #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 */
>
> @@ -446,9 +447,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> regmap_write(pca->regmap, PCA9685_MODE1, reg);
>
> - /* Clear all "full off" bits */
> + /* Reset ON/OFF registers to HW defaults (only full OFF bit is set) */
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> 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 */
> @@ -470,8 +473,10 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> /*
> * The chip comes out of power-up in the sleep state,
> * but force it to sleep in case it was woken up before
> + * and set the default prescale value
> */
> 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);
Consider making it clearer that prescale can only be touched when the chip is
in sleep mode. Suggestion:
/* set the default prescale value - chip _must_ be in sleep mode */
regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF);
>
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times
2020-12-15 21:22 ` [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times Clemens Gruber
@ 2020-12-17 4:02 ` Sven Van Asbroeck
2020-12-17 17:50 ` Clemens Gruber
0 siblings, 1 reply; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 4:02 UTC (permalink / raw)
To: Clemens Gruber
Cc: linux-pwm, Thierry Reding, Uwe Kleine-König, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
Hi Clemens, see below.
On Wed, Dec 16, 2020 at 7:53 AM 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>
> ---
> drivers/pwm/pwm-pca9685.c | 62 +++++++++++++++++++++++++++++++--------
> 1 file changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 38aadaf50996..ff916980de49 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -79,6 +79,7 @@
> struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> + bool staggered_outputs;
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -93,45 +94,79 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
>
> static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> {
> + unsigned int on, off;
> +
> if (duty == 0) {
> /* Set the full OFF bit, which has the highest precedence */
> regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + return;
> } else if (duty >= PCA9685_COUNTER_RANGE) {
> /* Set the full ON bit and clear the full OFF bit */
> regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> - } else {
> - /* Set OFF time (clears the full OFF bit) */
> - regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> - regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> - /* Clear the full ON bit */
> - regmap_write(pca->regmap, REG_ON_H(channel), 0);
> + return;
> + }
> +
> + if (pca->staggered_outputs) {
> + if (channel < PCA9685_MAXCHAN) {
> + /*
> + * To reduce EMI, the ON times of each channel are
> + * spread out evenly within the counter range, while
> + * still maintaining the configured duty cycle
> + */
> + on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
> + off = (on + duty) % PCA9685_COUNTER_RANGE;
> + regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
> + regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
> + regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
> + return;
> + }
> + /* No staggering possible if "all LEDs" channel is used */
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> }
> + /* Set OFF time (clears the full OFF bit) */
> + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> + /* Clear the full ON bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> }
I find the set_duty() function quite hard to follow.
Can we simplify this by eliminating the !staggered_outputs special case?
E.g. always calculate and write 'on' and 'off'.
But if !staggered_outputs => on = 0 and off = duty.
Yes this adds one extra/unnecessary register write in the !staggered case,
but simplicity/maintainability >>> efficiency here. And this "issue" will
disappear when we switch on regmap caching.
>
> static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> {
> - unsigned int off_h, val;
> + unsigned int off, on, val;
>
> if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> /* Hardware readout not supported for "all LEDs" channel */
> return 0;
> }
>
> - regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> - if (off_h & LED_FULL) {
> + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
> + if (off & LED_FULL) {
> /* Full OFF bit is set */
> return 0;
> }
>
> - regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> - if (val & LED_FULL) {
> + regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
> + if (on & LED_FULL) {
> /* Full ON bit is set */
> return PCA9685_COUNTER_RANGE;
> }
>
> regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> - return ((off_h & 0xf) << 8) | (val & 0xff);
> + off = ((off & 0xf) << 8) | (val & 0xff);
> +
> + if (pca->staggered_outputs) {
> + regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
> + on = ((on & 0xf) << 8) | (val & 0xff);
> +
> + if (off >= on)
> + return off - on;
> + else
> + return off + PCA9685_COUNTER_RANGE - on;
I think the if/else is unnecessary. unsigned int is twos-complement,
so we can just write:
return (off - on) & 0xfff; (or PCA9685_COUNTER_RANGE - 1)
and it will "magically" work even if off < on.
> + }
> +
> + return off;
> }
As in set_duty(), consider removing the !staggered_outputs special case,
to make this function clearer and simpler.
>
> #if IS_ENABLED(CONFIG_GPIOLIB)
> @@ -442,6 +477,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>
> regmap_write(pca->regmap, PCA9685_MODE2, reg);
>
> + pca->staggered_outputs = device_property_read_bool(
> + &client->dev, "nxp,staggered-outputs");
> +
> /* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> regmap_read(pca->regmap, PCA9685_MODE1, ®);
> reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users
2020-12-15 21:22 ` [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users Clemens Gruber
@ 2020-12-17 4:03 ` Sven Van Asbroeck
2020-12-17 18:07 ` Clemens Gruber
0 siblings, 1 reply; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 4:03 UTC (permalink / raw)
To: Clemens Gruber
Cc: linux-pwm, Thierry Reding, Uwe Kleine-König, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
Hi Clemens, see below.
On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> Previously, the last used PWM channel could change the global prescale
> setting, even if other channels were already in use.
>
> Fix it by only allowing the first user of the prescaler to change the
> global chip-wide prescale setting. If there is more than one channel in
> use, the prescale settings resulting from the chosen periods must match.
>
> PWMs that are disabled or have a duty cycle of 0% or 100% are not
> considered to be using the prescaler as they have the full OFF or full
> ON bits set. This also applies to channels used as GPIOs.
>
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------
> 1 file changed, 44 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index ff916980de49..438492d4aed4 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -23,11 +23,11 @@
> #include <linux/bitmap.h>
>
> /*
> - * Because the PCA9685 has only one prescaler per chip, changing the period of
> - * one channel affects the period of all 16 PWM outputs!
> - * However, the ratio between each configured duty cycle and the chip-wide
> - * period remains constant, because the OFF time is set in proportion to the
> - * counter range.
> + * Because the PCA9685 has only one prescaler per chip, only the first channel
> + * that uses the prescaler is allowed to change the prescale register.
> + * PWM channels requested afterwards must use a period that results in the same
> + * prescale setting as the one set by the first requested channel, unless they
> + * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON).
> */
>
> #define PCA9685_MODE1 0x00
> @@ -80,6 +80,8 @@ struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> bool staggered_outputs;
> + struct mutex prescaler_users_lock;
Keep things simple by re-using the "struct mutex lock" below?
This code isn't performance-intensive, so having a single lock for
pwm/gpio requests + pwm_apply() is probably ok.
> + DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1);
Rename to pwms_use_prescale ?
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +/* This function is supposed to be called with the prescaler_users_lock held */
> +static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel)
Drop the inline? Only the compiler knows if inlining this function makes sense
on a platform (armv7, x86, etc). Compilers are usually better at this then
humans...
Rename to pca9685_prescaler_can_change() ?
> +{
> + /*
> + * A PWM channel may only change the prescaler if there are no users of
> + * the prescaler yet or that same channel is the only one in use.
> + */
> + return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) ||
> + (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 &&
> + test_bit(channel, pca->prescaler_users));
> +}
I found this logic expression quite complex to read. Perhaps simplify by using
a few steps? For example:
/* if prescaler not in use, we can always change it */
if (empty) return true;
/* if more than one pwm is using the prescaler, we can never change it */
if (weight > 1) return false;
/* one pwm is using the prescaler, we can only change it if it's us */
return test_bit(us);
> +
> static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> {
> unsigned int on, off;
> @@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
>
> + mutex_lock(&pca->prescaler_users_lock);
> +
> if (!state->enabled || duty < 1) {
> pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> - return 0;
> + goto prescaler_unused;
> } else if (duty == PCA9685_COUNTER_RANGE) {
> pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> - return 0;
> + goto prescaler_unused;
> }
>
> regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> if (prescale != val) {
> + if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) {
> + mutex_unlock(&pca->prescaler_users_lock);
> + dev_err(chip->dev,
> + "prescaler not set: already in use with different setting!\n");
> + return -EBUSY;
> + }
> +
> /*
> * Putting the chip briefly into SLEEP mode
> * at this point won't interfere with the
> @@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> }
>
> pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> +
> + set_bit(pwm->hwpwm, pca->prescaler_users);
> + mutex_unlock(&pca->prescaler_users_lock);
> + return 0;
> +
> +prescaler_unused:
> + clear_bit(pwm->hwpwm, pca->prescaler_users);
> + mutex_unlock(&pca->prescaler_users_lock);
> return 0;
> }
The need for the mutex makes this function quite "messy": we have to guard all
the exits, and that's easy to forget.
Maybe simplify the function by moving the mutex to a helper?
Example:
static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
... just do stuff and don't worry about the mutex
}
static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
const struct pwm_state *state)
{
/* document why we serialize pwm_apply */
mutex_lock();
__pca9685_pwm_apply(chip, pwm, state);
mutex_unlock();
}
>
> @@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
>
> + mutex_lock(&pca->prescaler_users_lock);
> + clear_bit(pwm->hwpwm, pca->prescaler_users);
> pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> + mutex_unlock(&pca->prescaler_users_lock);
> +
> pm_runtime_put(chip->dev);
> pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> }
> @@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
>
> i2c_set_clientdata(client, pca);
>
> + mutex_init(&pca->prescaler_users_lock);
> +
> regmap_read(pca->regmap, PCA9685_MODE2, ®);
>
> if (device_property_read_bool(&client->dev, "invert"))
> --
> 2.29.2
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-17 3:58 ` [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
@ 2020-12-17 16:48 ` Clemens Gruber
2020-12-17 17:10 ` Sven Van Asbroeck
0 siblings, 1 reply; 25+ messages in thread
From: Clemens Gruber @ 2020-12-17 16:48 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, Dec 16, 2020 at 10:58:07PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, this looks compact, simple and neat. I like it a lot !!
Thanks!
>
> Few very minor nits below.
>
> On Wed, Dec 16, 2020 at 7:53 AM 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
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > Changes since v4:
> > - Patches split up
> > - Use a single set_duty function
> > - Improve readability / new macros
> > - Added a patch to restrict prescale changes to the first user
> >
> > Changes since v3:
> > - Refactoring: Extracted common functions
> > - Read prescale register value instead of caching it
> > - Return all zeros and disabled for "all LEDs" channel state
> > - Improved duty calculation / mapping to 0..4096
> >
> > Changes since v2:
> > - Always set default prescale value in probe
> > - Simplified probe code
> > - Inlined functions with one callsite
> >
> > Changes since v1:
> > - Fixed a logic error
> > - Impoved PM runtime handling and fixed !CONFIG_PM
> > - Write default prescale reg value if invalid in probe
> > - Reuse full_off/_on functions throughout driver
> > - Use cached prescale value whenever possible
> >
> > drivers/pwm/pwm-pca9685.c | 253 +++++++++++++-------------------------
> > 1 file changed, 87 insertions(+), 166 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..1b5b5fb93b43 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -51,7 +51,6 @@
> > #define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */
> >
> > #define PCA9685_COUNTER_RANGE 4096
> > -#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
> > #define PCA9685_OSC_CLOCK_MHZ 25 /* Internal oscillator with 25 MHz */
> >
> > #define PCA9685_NUMREGS 0xFF
> > @@ -71,10 +70,14 @@
> > #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> > #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
> >
> > +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
> > +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
> > +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
> > +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
>
> Yes !!
>
> > +
> > struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > - int period_ns;
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -87,6 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > return container_of(chip, struct pca9685, chip);
> > }
> >
> > +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > +{
>
> Add brief function documentation to clarify that 'duty' sets the duty cycle
> _ratio_ to 'duty/4096' on ? I.e.
> duty == 2048 => duty cycle ratio = 2048/4096 = 50% on
> duty == 4096 => duty cycle ratio = 4096/4086 = 100% on etc
Will do.
>
> > + if (duty == 0) {
> > + /* Set the full OFF bit, which has the highest precedence */
> > + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > + } else if (duty >= PCA9685_COUNTER_RANGE) {
> > + /* Set the full ON bit and clear the full OFF bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > + } else {
> > + /* Set OFF time (clears the full OFF bit) */
> > + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > + /* Clear the full ON bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > + }
> > +}
> > +
> > +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > +{
> > + unsigned int off_h, val;
> > +
> > + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > + /* Hardware readout not supported for "all LEDs" channel */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > + if (off_h & LED_FULL) {
>
> I believe this may trigger bots which are monitoring patches on LKML:
> if regmap_read() somehow fails, off_h will be used uninitialized.
>
> Prevent by initializing off_h and val?
I can initialize the values to 0 of course and check the file for other
places with missing initializations.
Or would it be better to check the return codes of regmap_read/write in
such cases? I'm not sure.
>
> > + /* Full OFF bit is set */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > + if (val & LED_FULL) {
> > + /* Full ON bit is set */
> > + return PCA9685_COUNTER_RANGE;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > {
> > @@ -138,34 +184,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > - unsigned int value;
> >
> > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > -
> > - return value & LED_FULL;
> > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > }
> >
> > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > int value)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > - unsigned int on = value ? LED_FULL : 0;
> > -
> > - /* Clear both OFF registers */
> > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> >
> > - /* Set the full ON bit */
> > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > }
> >
> > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> >
> > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > + pca9685_pwm_set_duty(pca, offset, 0);
> > pm_runtime_put(pca->chip.dev);
> > pca9685_pwm_clear_inuse(pca, offset);
> > }
> > @@ -246,167 +281,56 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> > }
> > }
> >
> > -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > - int duty_ns, int period_ns)
> > +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > - unsigned long long duty;
> > - unsigned int reg;
> > - int prescale;
> > -
> > - if (period_ns != pca->period_ns) {
> > - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> > - PCA9685_COUNTER_RANGE * 1000) - 1;
> > -
> > - if (prescale >= PCA9685_PRESCALE_MIN &&
> > - prescale <= PCA9685_PRESCALE_MAX) {
> > - /*
> > - * Putting the chip briefly into SLEEP mode
> > - * at this point won't interfere with the
> > - * pm_runtime framework, because the pm_runtime
> > - * state is guaranteed active here.
> > - */
> > - /* Put chip into sleep mode */
> > - pca9685_set_sleep_mode(pca, true);
> > -
> > - /* Change the chip-wide output frequency */
> > - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > -
> > - /* Wake the chip up */
> > - pca9685_set_sleep_mode(pca, false);
> > -
> > - pca->period_ns = period_ns;
> > - } else {
> > - dev_err(chip->dev,
> > - "prescaler not set: period out of bounds!\n");
> > - return -EINVAL;
> > - }
> > - }
> > + unsigned long long duty, prescale;
> > + unsigned int val = 0;
> >
> > - if (duty_ns < 1) {
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EOPNOTSUPP;
> >
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - return 0;
> > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > + dev_err(chip->dev, "prescaler not set: period out of bounds!\n");
>
> would "pwm not changed: period out of bounds" be a clearer error message here?
Yes, I think so. I'll change it in the next version.
>
> > + return -EINVAL;
> > }
> >
> > - if (duty_ns == period_ns) {
> > - /* Clear both OFF registers */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - /* Set the full ON bit */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
> >
> > + if (!state->enabled || duty < 1) {
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > + return 0;
> > + } else if (duty == PCA9685_COUNTER_RANGE) {
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > return 0;
> > }
> >
> > - duty = PCA9685_COUNTER_RANGE * (unsigned long long)duty_ns;
> > - duty = DIV_ROUND_UP_ULL(duty, period_ns);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, (int)duty & 0xff);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, ((int)duty >> 8) & 0xf);
> > -
> > - /* Clear the full ON bit, otherwise the set OFF time has no effect */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > -
> > - return 0;
> > -}
> > -
> > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - struct pca9685 *pca = to_pca(chip);
> > - unsigned int reg;
> > -
> > - /*
> > - * The PWM subsystem does not support a pre-delay.
> > - * So, set the ON-timeout to 0
> > - */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_L;
> > - else
> > - reg = LED_N_ON_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > + if (prescale != val) {
> > + /*
> > + * Putting the chip briefly into SLEEP mode
> > + * at this point won't interfere with the
> > + * pm_runtime framework, because the pm_runtime
> > + * state is guaranteed active here.
> > + */
> > + /* Put chip into sleep mode */
> > + pca9685_set_sleep_mode(pca, true);
> >
> > - /*
> > - * Clear the full-off bit.
> > - * It has precedence over the others and must be off.
> > - */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > + /* Change the chip-wide output frequency */
> > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> >
> > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > + /* Wake the chip up */
> > + pca9685_set_sleep_mode(pca, false);
> > + }
> >
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > return 0;
> > }
> >
> > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - struct pca9685 *pca = to_pca(chip);
> > - unsigned int reg;
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - /* Clear the LED_OFF counter. */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -}
> > -
> > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > @@ -422,15 +346,13 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> >
> > - pca9685_pwm_disable(chip, pwm);
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > pm_runtime_put(chip->dev);
> > pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> > }
> >
> > static const struct pwm_ops pca9685_pwm_ops = {
> > - .enable = pca9685_pwm_enable,
> > - .disable = pca9685_pwm_disable,
> > - .config = pca9685_pwm_config,
> > + .apply = pca9685_pwm_apply,
> > .request = pca9685_pwm_request,
> > .free = pca9685_pwm_free,
> > .owner = THIS_MODULE,
> > @@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > ret);
> > return ret;
> > }
> > - pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >
> > i2c_set_clientdata(client, pca);
> >
> > --
> > 2.29.2
> >
Thanks,
Clemens
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-17 16:48 ` Clemens Gruber
@ 2020-12-17 17:10 ` Sven Van Asbroeck
2021-03-01 21:41 ` Uwe Kleine-König
2021-03-22 7:58 ` Thierry Reding
0 siblings, 2 replies; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 17:10 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 Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> I can initialize the values to 0 of course and check the file for other
> places with missing initializations.
>
> Or would it be better to check the return codes of regmap_read/write in
> such cases? I'm not sure.
I think that checking the regmap_read/write return values is overkill
in this driver. These functions can't realistically fail, except if the i2c
bus is bad, i.e. h/w failure or intermittency. And that's an externality
which I believe we can ignore.
Maybe Thierry or Uwe have further insights here.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
2020-12-17 4:02 ` Sven Van Asbroeck
@ 2020-12-17 17:45 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-17 17:45 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, Dec 16, 2020 at 11:02:03PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, minor nit below.
>
> On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > Reset the prescale and ON/OFF registers to their POR default state in
> > the probe function. Otherwise, the PWMs could still be active after a
> > watchdog reset and reboot, etc.
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > drivers/pwm/pwm-pca9685.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 7b14447f3c05..38aadaf50996 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -47,6 +47,7 @@
> > #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 */
> >
> > @@ -446,9 +447,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > regmap_write(pca->regmap, PCA9685_MODE1, reg);
> >
> > - /* Clear all "full off" bits */
> > + /* Reset ON/OFF registers to HW defaults (only full OFF bit is set) */
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> > 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 */
> > @@ -470,8 +473,10 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > /*
> > * The chip comes out of power-up in the sleep state,
> > * but force it to sleep in case it was woken up before
> > + * and set the default prescale value
> > */
> > 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);
>
> Consider making it clearer that prescale can only be touched when the chip is
> in sleep mode. Suggestion:
>
> /* set the default prescale value - chip _must_ be in sleep mode */
> regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF);
Good point, thanks.
>
> >
> > --
> > 2.29.2
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times
2020-12-17 4:02 ` Sven Van Asbroeck
@ 2020-12-17 17:50 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2020-12-17 17:50 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, Dec 16, 2020 at 11:02:57PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, see below.
>
> On Wed, Dec 16, 2020 at 7:53 AM 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>
> > ---
> > drivers/pwm/pwm-pca9685.c | 62 +++++++++++++++++++++++++++++++--------
> > 1 file changed, 50 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 38aadaf50996..ff916980de49 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -79,6 +79,7 @@
> > struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > + bool staggered_outputs;
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -93,45 +94,79 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> >
> > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > {
> > + unsigned int on, off;
> > +
> > if (duty == 0) {
> > /* Set the full OFF bit, which has the highest precedence */
> > regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > + return;
> > } else if (duty >= PCA9685_COUNTER_RANGE) {
> > /* Set the full ON bit and clear the full OFF bit */
> > regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > - } else {
> > - /* Set OFF time (clears the full OFF bit) */
> > - regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > - regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > - /* Clear the full ON bit */
> > - regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > + return;
> > + }
> > +
> > + if (pca->staggered_outputs) {
> > + if (channel < PCA9685_MAXCHAN) {
> > + /*
> > + * To reduce EMI, the ON times of each channel are
> > + * spread out evenly within the counter range, while
> > + * still maintaining the configured duty cycle
> > + */
> > + on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
> > + off = (on + duty) % PCA9685_COUNTER_RANGE;
> > + regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
> > + regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
> > + regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
> > + return;
> > + }
> > + /* No staggering possible if "all LEDs" channel is used */
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > }
> > + /* Set OFF time (clears the full OFF bit) */
> > + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > + /* Clear the full ON bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > }
>
> I find the set_duty() function quite hard to follow.
> Can we simplify this by eliminating the !staggered_outputs special case?
> E.g. always calculate and write 'on' and 'off'.
> But if !staggered_outputs => on = 0 and off = duty.
>
> Yes this adds one extra/unnecessary register write in the !staggered case,
> but simplicity/maintainability >>> efficiency here. And this "issue" will
> disappear when we switch on regmap caching.
No objections, I will eliminate the special case.
>
> >
> > static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > {
> > - unsigned int off_h, val;
> > + unsigned int off, on, val;
> >
> > if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > /* Hardware readout not supported for "all LEDs" channel */
> > return 0;
> > }
> >
> > - regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > - if (off_h & LED_FULL) {
> > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
> > + if (off & LED_FULL) {
> > /* Full OFF bit is set */
> > return 0;
> > }
> >
> > - regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > - if (val & LED_FULL) {
> > + regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
> > + if (on & LED_FULL) {
> > /* Full ON bit is set */
> > return PCA9685_COUNTER_RANGE;
> > }
> >
> > regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > - return ((off_h & 0xf) << 8) | (val & 0xff);
> > + off = ((off & 0xf) << 8) | (val & 0xff);
> > +
> > + if (pca->staggered_outputs) {
> > + regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
> > + on = ((on & 0xf) << 8) | (val & 0xff);
> > +
> > + if (off >= on)
> > + return off - on;
> > + else
> > + return off + PCA9685_COUNTER_RANGE - on;
>
> I think the if/else is unnecessary. unsigned int is twos-complement,
> so we can just write:
>
> return (off - on) & 0xfff; (or PCA9685_COUNTER_RANGE - 1)
>
> and it will "magically" work even if off < on.
Ah, yes of course. Good catch!
>
>
> > + }
> > +
> > + return off;
> > }
>
> As in set_duty(), consider removing the !staggered_outputs special case,
> to make this function clearer and simpler.
Yes, thanks.
>
> >
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > @@ -442,6 +477,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >
> > regmap_write(pca->regmap, PCA9685_MODE2, reg);
> >
> > + pca->staggered_outputs = device_property_read_bool(
> > + &client->dev, "nxp,staggered-outputs");
> > +
> > /* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
> > regmap_read(pca->regmap, PCA9685_MODE1, ®);
> > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > --
> > 2.29.2
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users
2020-12-17 4:03 ` Sven Van Asbroeck
@ 2020-12-17 18:07 ` Clemens Gruber
2020-12-17 18:17 ` Sven Van Asbroeck
0 siblings, 1 reply; 25+ messages in thread
From: Clemens Gruber @ 2020-12-17 18:07 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, Dec 16, 2020 at 11:03:39PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, see below.
>
> On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > Previously, the last used PWM channel could change the global prescale
> > setting, even if other channels were already in use.
> >
> > Fix it by only allowing the first user of the prescaler to change the
> > global chip-wide prescale setting. If there is more than one channel in
> > use, the prescale settings resulting from the chosen periods must match.
> >
> > PWMs that are disabled or have a duty cycle of 0% or 100% are not
> > considered to be using the prescaler as they have the full OFF or full
> > ON bits set. This also applies to channels used as GPIOs.
> >
> > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> > ---
> > drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------
> > 1 file changed, 44 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index ff916980de49..438492d4aed4 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -23,11 +23,11 @@
> > #include <linux/bitmap.h>
> >
> > /*
> > - * Because the PCA9685 has only one prescaler per chip, changing the period of
> > - * one channel affects the period of all 16 PWM outputs!
> > - * However, the ratio between each configured duty cycle and the chip-wide
> > - * period remains constant, because the OFF time is set in proportion to the
> > - * counter range.
> > + * Because the PCA9685 has only one prescaler per chip, only the first channel
> > + * that uses the prescaler is allowed to change the prescale register.
> > + * PWM channels requested afterwards must use a period that results in the same
> > + * prescale setting as the one set by the first requested channel, unless they
> > + * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON).
> > */
> >
> > #define PCA9685_MODE1 0x00
> > @@ -80,6 +80,8 @@ struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > bool staggered_outputs;
> > + struct mutex prescaler_users_lock;
>
> Keep things simple by re-using the "struct mutex lock" below?
> This code isn't performance-intensive, so having a single lock for
> pwm/gpio requests + pwm_apply() is probably ok.
Yes, I think this could work. Good idea.
>
> > + DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1);
>
> Rename to pwms_use_prescale ?
Yes, fine with me.
>
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > return container_of(chip, struct pca9685, chip);
> > }
> >
> > +/* This function is supposed to be called with the prescaler_users_lock held */
> > +static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel)
>
> Drop the inline? Only the compiler knows if inlining this function makes sense
> on a platform (armv7, x86, etc). Compilers are usually better at this then
> humans...
You're probably right. I will drop the inline.
>
> Rename to pca9685_prescaler_can_change() ?
Sounds good!
>
> > +{
> > + /*
> > + * A PWM channel may only change the prescaler if there are no users of
> > + * the prescaler yet or that same channel is the only one in use.
> > + */
> > + return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) ||
> > + (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 &&
> > + test_bit(channel, pca->prescaler_users));
> > +}
>
> I found this logic expression quite complex to read. Perhaps simplify by using
> a few steps? For example:
>
> /* if prescaler not in use, we can always change it */
> if (empty) return true;
> /* if more than one pwm is using the prescaler, we can never change it */
> if (weight > 1) return false;
> /* one pwm is using the prescaler, we can only change it if it's us */
> return test_bit(us);
Good point, I will simplify it!
>
> > +
> > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > {
> > unsigned int on, off;
> > @@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
> >
> > + mutex_lock(&pca->prescaler_users_lock);
> > +
> > if (!state->enabled || duty < 1) {
> > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > - return 0;
> > + goto prescaler_unused;
> > } else if (duty == PCA9685_COUNTER_RANGE) {
> > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > - return 0;
> > + goto prescaler_unused;
> > }
> >
> > regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
> > if (prescale != val) {
> > + if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) {
> > + mutex_unlock(&pca->prescaler_users_lock);
> > + dev_err(chip->dev,
> > + "prescaler not set: already in use with different setting!\n");
> > + return -EBUSY;
> > + }
> > +
> > /*
> > * Putting the chip briefly into SLEEP mode
> > * at this point won't interfere with the
> > @@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > }
> >
> > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > +
> > + set_bit(pwm->hwpwm, pca->prescaler_users);
> > + mutex_unlock(&pca->prescaler_users_lock);
> > + return 0;
> > +
> > +prescaler_unused:
> > + clear_bit(pwm->hwpwm, pca->prescaler_users);
> > + mutex_unlock(&pca->prescaler_users_lock);
> > return 0;
> > }
>
> The need for the mutex makes this function quite "messy": we have to guard all
> the exits, and that's easy to forget.
I agree.
>
> Maybe simplify the function by moving the mutex to a helper?
> Example:
>
> static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> ... just do stuff and don't worry about the mutex
> }
>
> static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> const struct pwm_state *state)
> {
> /* document why we serialize pwm_apply */
> mutex_lock();
> __pca9685_pwm_apply(chip, pwm, state);
> mutex_unlock();
> }
Also a good idea!
As always, great review! Thank you!
>
> >
> > @@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> >
> > + mutex_lock(&pca->prescaler_users_lock);
> > + clear_bit(pwm->hwpwm, pca->prescaler_users);
> > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > + mutex_unlock(&pca->prescaler_users_lock);
> > +
> > pm_runtime_put(chip->dev);
> > pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
> > }
> > @@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> >
> > i2c_set_clientdata(client, pca);
> >
> > + mutex_init(&pca->prescaler_users_lock);
> > +
> > regmap_read(pca->regmap, PCA9685_MODE2, ®);
> >
> > if (device_property_read_bool(&client->dev, "invert"))
> > --
> > 2.29.2
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users
2020-12-17 18:07 ` Clemens Gruber
@ 2020-12-17 18:17 ` Sven Van Asbroeck
0 siblings, 0 replies; 25+ messages in thread
From: Sven Van Asbroeck @ 2020-12-17 18:17 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 Thu, Dec 17, 2020 at 1:07 PM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> As always, great review! Thank you!
>
My pleasure, it's great to help out.
And thanks, you're the one doing all the hard work :)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-17 17:10 ` Sven Van Asbroeck
@ 2021-03-01 21:41 ` Uwe Kleine-König
2021-03-04 13:10 ` Clemens Gruber
2021-03-22 7:58 ` Thierry Reding
1 sibling, 1 reply; 25+ messages in thread
From: Uwe Kleine-König @ 2021-03-01 21:41 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Clemens Gruber, linux-pwm, Thierry Reding, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
>
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.
>
> Maybe Thierry or Uwe have further insights here.
I'm a fan of full checking, but I'm not sure what's Thierry's position
on that.
My reasoning is: If the bus is bad and a request to modify the PWM fails
because of that, the PWM consumer probably wants to know.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
` (6 preceding siblings ...)
2020-12-17 3:58 ` [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
@ 2021-03-01 21:44 ` Uwe Kleine-König
2021-03-04 13:12 ` Clemens Gruber
7 siblings, 1 reply; 25+ messages in thread
From: Uwe Kleine-König @ 2021-03-01 21:44 UTC (permalink / raw)
To: Clemens Gruber
Cc: linux-pwm, Thierry Reding, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander
[-- Attachment #1: Type: text/plain, Size: 467 bytes --]
Hello Clemens,
On Tue, Dec 15, 2020 at 10:22:22PM +0100, Clemens Gruber wrote:
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EOPNOTSUPP;
We agreed on -EINVAL for that one since 2b1c1a5d5148.
Other than that the patch looks ok (but note I only looked quickly).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
2020-12-15 21:22 ` [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe Clemens Gruber
2020-12-17 4:02 ` Sven Van Asbroeck
@ 2021-03-01 21:46 ` Uwe Kleine-König
2021-03-04 13:16 ` Clemens Gruber
1 sibling, 1 reply; 25+ messages in thread
From: Uwe Kleine-König @ 2021-03-01 21:46 UTC (permalink / raw)
To: Clemens Gruber
Cc: linux-pwm, Thierry Reding, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander
[-- Attachment #1: Type: text/plain, Size: 640 bytes --]
Hello Clemens,
On Tue, Dec 15, 2020 at 10:22:25PM +0100, Clemens Gruber wrote:
> Reset the prescale and ON/OFF registers to their POR default state in
> the probe function. Otherwise, the PWMs could still be active after a
> watchdog reset and reboot, etc.
My memories are swapped out because it's already so long ago I looked
into this series. I thought it was already said that taking over the
configured state in probe is the nice thing to do?!
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2021-03-01 21:41 ` Uwe Kleine-König
@ 2021-03-04 13:10 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2021-03-04 13:10 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Sven Van Asbroeck, linux-pwm, Thierry Reding, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
Hi Uwe,
On Mon, Mar 01, 2021 at 10:41:15PM +0100, Uwe Kleine-König wrote:
> On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > >
> > > I can initialize the values to 0 of course and check the file for other
> > > places with missing initializations.
> > >
> > > Or would it be better to check the return codes of regmap_read/write in
> > > such cases? I'm not sure.
> >
> > I think that checking the regmap_read/write return values is overkill
> > in this driver. These functions can't realistically fail, except if the i2c
> > bus is bad, i.e. h/w failure or intermittency. And that's an externality
> > which I believe we can ignore.
> >
> > Maybe Thierry or Uwe have further insights here.
>
> I'm a fan of full checking, but I'm not sure what's Thierry's position
> on that.
>
> My reasoning is: If the bus is bad and a request to modify the PWM fails
> because of that, the PWM consumer probably wants to know.
I see. Then I'd suggest that we postpone adding these checks until we
get a response from Thierry and if he agrees with you, we could add
these checks in a separate patch series?
Thanks,
Clemens
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2021-03-01 21:44 ` Uwe Kleine-König
@ 2021-03-04 13:12 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2021-03-04 13:12 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Thierry Reding, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander
Hi Uwe,
On Mon, Mar 01, 2021 at 10:44:07PM +0100, Uwe Kleine-König wrote:
> Hello Clemens,
>
> On Tue, Dec 15, 2020 at 10:22:22PM +0100, Clemens Gruber wrote:
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EOPNOTSUPP;
>
> We agreed on -EINVAL for that one since 2b1c1a5d5148.
>
> Other than that the patch looks ok (but note I only looked quickly).
Thanks for looking over it. This will be -EINVAL in the next revision.
Best regards,
Clemens
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe
2021-03-01 21:46 ` Uwe Kleine-König
@ 2021-03-04 13:16 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2021-03-04 13:16 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: linux-pwm, Thierry Reding, Sven Van Asbroeck, Lee Jones,
linux-kernel, Mika Westerberg, David Jander
On Mon, Mar 01, 2021 at 10:46:33PM +0100, Uwe Kleine-König wrote:
> Hello Clemens,
>
> On Tue, Dec 15, 2020 at 10:22:25PM +0100, Clemens Gruber wrote:
> > Reset the prescale and ON/OFF registers to their POR default state in
> > the probe function. Otherwise, the PWMs could still be active after a
> > watchdog reset and reboot, etc.
>
> My memories are swapped out because it's already so long ago I looked
> into this series. I thought it was already said that taking over the
> configured state in probe is the nice thing to do?!
Yes, but Sven voiced some concerns about the introduced complexities
when removing the resets.
I was torn between the two options.
I think it would be a good idea to first switch to the atomic API while
keeping the resets and then evaluate removing them in a separate patch
series.
Thanks,
Clemens
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2020-12-17 17:10 ` Sven Van Asbroeck
2021-03-01 21:41 ` Uwe Kleine-König
@ 2021-03-22 7:58 ` Thierry Reding
2021-03-27 15:54 ` Clemens Gruber
1 sibling, 1 reply; 25+ messages in thread
From: Thierry Reding @ 2021-03-22 7:58 UTC (permalink / raw)
To: Sven Van Asbroeck
Cc: Clemens Gruber, linux-pwm, Uwe Kleine-König, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <clemens.gruber@pqgruber.com> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
>
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.
I think there are (rare) occasions where it's fine to not check for
errors, i.e. if you definitively know that calls can't fail. However,
given that this uses regmap and you don't really know what's backing
this, I think it's always better to err on the side of caution and
properly check the return values.
The fact that this can be externally caused is actually a reason why
we shouldn't be ignoring any errors. If there's a chip that's hogging
the I2C bus or if you've even just mistyped the I2C client's address
in DT, it's better if the PWM driver tells you with an error message
than if it is silently ignoring the errors and keeps you guessing at
why the PWM isn't behaving the way it should.
Granted, the error code isn't always going to pinpoint exactly what's
going wrong, but for serious errors often the I2C bus driver will let
you know with an extra error message. However, it's much easier to go
looking for that error message if the PWM driver lets you know that
something went wrong.
Please just add full checking of regmap operations.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/7] pwm: pca9685: Switch to atomic API
2021-03-22 7:58 ` Thierry Reding
@ 2021-03-27 15:54 ` Clemens Gruber
0 siblings, 0 replies; 25+ messages in thread
From: Clemens Gruber @ 2021-03-27 15:54 UTC (permalink / raw)
To: Thierry Reding
Cc: Sven Van Asbroeck, linux-pwm, Uwe Kleine-König, Lee Jones,
Linux Kernel Mailing List, Mika Westerberg, David Jander
Hi Thierry,
On Mon, Mar 22, 2021 at 08:58:35AM +0100, Thierry Reding wrote:
> On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> > <clemens.gruber@pqgruber.com> wrote:
> > >
> > > I can initialize the values to 0 of course and check the file for other
> > > places with missing initializations.
> > >
> > > Or would it be better to check the return codes of regmap_read/write in
> > > such cases? I'm not sure.
> >
> > I think that checking the regmap_read/write return values is overkill
> > in this driver. These functions can't realistically fail, except if the i2c
> > bus is bad, i.e. h/w failure or intermittency. And that's an externality
> > which I believe we can ignore.
>
> I think there are (rare) occasions where it's fine to not check for
> errors, i.e. if you definitively know that calls can't fail. However,
> given that this uses regmap and you don't really know what's backing
> this, I think it's always better to err on the side of caution and
> properly check the return values.
>
> The fact that this can be externally caused is actually a reason why
> we shouldn't be ignoring any errors. If there's a chip that's hogging
> the I2C bus or if you've even just mistyped the I2C client's address
> in DT, it's better if the PWM driver tells you with an error message
> than if it is silently ignoring the errors and keeps you guessing at
> why the PWM isn't behaving the way it should.
>
> Granted, the error code isn't always going to pinpoint exactly what's
> going wrong, but for serious errors often the I2C bus driver will let
> you know with an extra error message. However, it's much easier to go
> looking for that error message if the PWM driver lets you know that
> something went wrong.
>
> Please just add full checking of regmap operations.
OK, I will create a separate patch adding these checks in the next
series.
This will lead to > 20 additional dev_err statements, let me know if I
should instead just return the error code and not add dev_err's for
every failed regmap operation.
Clemens
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-03-27 15:55 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-15 21:22 [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 2/7] pwm: pca9685: Support hardware readout Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 3/7] pwm: pca9685: Improve runtime PM behavior Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 4/7] pwm: pca9685: Reset registers to POR state in probe Clemens Gruber
2020-12-17 4:02 ` Sven Van Asbroeck
2020-12-17 17:45 ` Clemens Gruber
2021-03-01 21:46 ` Uwe Kleine-König
2021-03-04 13:16 ` Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 5/7] pwm: pca9685: Support staggered output ON times Clemens Gruber
2020-12-17 4:02 ` Sven Van Asbroeck
2020-12-17 17:50 ` Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 6/7] dt-bindings: pwm: pca9685: Add nxp,staggered-outputs property Clemens Gruber
2020-12-15 21:22 ` [PATCH v5 7/7] pwm: pca9685: Restrict period change for prescaler users Clemens Gruber
2020-12-17 4:03 ` Sven Van Asbroeck
2020-12-17 18:07 ` Clemens Gruber
2020-12-17 18:17 ` Sven Van Asbroeck
2020-12-17 3:58 ` [PATCH v5 1/7] pwm: pca9685: Switch to atomic API Sven Van Asbroeck
2020-12-17 16:48 ` Clemens Gruber
2020-12-17 17:10 ` Sven Van Asbroeck
2021-03-01 21:41 ` Uwe Kleine-König
2021-03-04 13:10 ` Clemens Gruber
2021-03-22 7:58 ` Thierry Reding
2021-03-27 15:54 ` Clemens Gruber
2021-03-01 21:44 ` Uwe Kleine-König
2021-03-04 13:12 ` Clemens Gruber
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).