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
- Previously, only the full ON bit was toggled in GPIO mode (and full
OFF cleared if set to high), which could result in both full OFF and
full ON not being set and on=0, off=0, which is not allowed according
to the datasheet
- The OFF registers were reset to 0 in probe, which could lead to the
forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
Signed-off-by: Clemens Gruber <[email protected]>
---
Changes since v7:
- Moved check for !state->enabled before prescaler configuration
- Removed unnecessary cast
- Use DIV_ROUND_DOWN in .apply
Changes since v6:
- Order of a comparison switched for improved readability
Changes since v5:
- Function documentation for set_duty
- Variable initializations
- Print warning if all LEDs channel
- Changed EOPNOTSUPP to EINVAL
- Improved error messages
- Register reset corrections moved to this patch
Changes since v4:
- Patches split up
- Use a single set_duty function
- Improve readability / new macros
- Added a patch to restrict prescale changes to the first user
Changes since v3:
- Refactoring: Extracted common functions
- Read prescale register value instead of caching it
- Return all zeros and disabled for "all LEDs" channel state
- Improved duty calculation / mapping to 0..4096
Changes since v2:
- Always set default prescale value in probe
- Simplified probe code
- Inlined functions with one callsite
Changes since v1:
- Fixed a logic error
- Impoved PM runtime handling and fixed !CONFIG_PM
- Write default prescale reg value if invalid in probe
- Reuse full_off/_on functions throughout driver
- Use cached prescale value whenever possible
drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
1 file changed, 89 insertions(+), 170 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..827b57ced3c2 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -51,7 +51,6 @@
#define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */
#define PCA9685_COUNTER_RANGE 4096
-#define PCA9685_DEFAULT_PERIOD 5000000 /* Default period_ns = 1/200 Hz */
#define PCA9685_OSC_CLOCK_MHZ 25 /* Internal oscillator with 25 MHz */
#define PCA9685_NUMREGS 0xFF
@@ -71,10 +70,14 @@
#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
+#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
+#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
+#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
+#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
+
struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
- int period_ns;
#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
struct gpio_chip gpio;
@@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}
+/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
+static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
+{
+ if (duty == 0) {
+ /* Set the full OFF bit, which has the highest precedence */
+ regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ } else if (duty >= PCA9685_COUNTER_RANGE) {
+ /* Set the full ON bit and clear the full OFF bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
+ regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+ } else {
+ /* Set OFF time (clears the full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
+ /* Clear the full ON bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), 0);
+ }
+}
+
+static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
+{
+ unsigned int off_h = 0, val = 0;
+
+ if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
+ /* HW does not support reading state of "all LEDs" channel */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
+ if (off_h & LED_FULL) {
+ /* Full OFF bit is set */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
+ if (val & LED_FULL) {
+ /* Full ON bit is set */
+ return PCA9685_COUNTER_RANGE;
+ }
+
+ val = 0;
+ regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ return ((off_h & 0xf) << 8) | (val & 0xff);
+}
+
#if IS_ENABLED(CONFIG_GPIOLIB)
static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
{
@@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- struct pwm_device *pwm = &pca->chip.pwms[offset];
- unsigned int value;
- regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
-
- return value & LED_FULL;
+ return pca9685_pwm_get_duty(pca, offset) != 0;
}
static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
int value)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- struct pwm_device *pwm = &pca->chip.pwms[offset];
- unsigned int on = value ? LED_FULL : 0;
-
- /* Clear both OFF registers */
- regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
- regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
- /* Set the full ON bit */
- regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
+ pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
}
static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
{
struct pca9685 *pca = gpiochip_get_data(gpio);
- pca9685_pwm_gpio_set(gpio, offset, 0);
+ pca9685_pwm_set_duty(pca, offset, 0);
pm_runtime_put(pca->chip.dev);
pca9685_pwm_clear_inuse(pca, offset);
}
@@ -246,167 +283,52 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
}
}
-static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
- int duty_ns, int period_ns)
+static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pca9685 *pca = to_pca(chip);
- unsigned long long duty;
- unsigned int reg;
- int prescale;
-
- if (period_ns != pca->period_ns) {
- prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
- PCA9685_COUNTER_RANGE * 1000) - 1;
-
- if (prescale >= PCA9685_PRESCALE_MIN &&
- prescale <= PCA9685_PRESCALE_MAX) {
- /*
- * Putting the chip briefly into SLEEP mode
- * at this point won't interfere with the
- * pm_runtime framework, because the pm_runtime
- * state is guaranteed active here.
- */
- /* Put chip into sleep mode */
- pca9685_set_sleep_mode(pca, true);
-
- /* Change the chip-wide output frequency */
- regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
-
- /* Wake the chip up */
- pca9685_set_sleep_mode(pca, false);
-
- pca->period_ns = period_ns;
- } else {
- dev_err(chip->dev,
- "prescaler not set: period out of bounds!\n");
- return -EINVAL;
- }
- }
+ unsigned long long duty, prescale;
+ unsigned int val = 0;
- if (duty_ns < 1) {
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EINVAL;
- regmap_write(pca->regmap, reg, LED_FULL);
-
- return 0;
+ prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
+ PCA9685_COUNTER_RANGE * 1000) - 1;
+ if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
+ dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
+ return -EINVAL;
}
- if (duty_ns == period_ns) {
- /* Clear both OFF registers */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_L;
- else
- reg = LED_N_OFF_L(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, 0x0);
-
- /* Set the full ON bit */
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_ON_H;
- else
- reg = LED_N_ON_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
+ if (!state->enabled) {
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
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, prescale);
- regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+ /* Wake the chip up */
+ pca9685_set_sleep_mode(pca, false);
+ }
+ duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
+ duty = DIV_ROUND_DOWN_ULL(duty, state->period);
+ 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 +344,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 +381,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
ret);
return ret;
}
- pca->period_ns = PCA9685_DEFAULT_PERIOD;
i2c_set_clientdata(client, pca);
@@ -484,9 +403,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
regmap_write(pca->regmap, PCA9685_MODE1, reg);
- /* Clear all "full off" bits */
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, 0);
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, 0);
+ /* Reset OFF registers to POR default */
+ regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
pca->chip.ops = &pca9685_pwm_ops;
/* Add an extra channel for ALL_LED */
--
2.31.1
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.
If runtime PM is disabled, we instead wake the chip in .probe and put it
to sleep in .remove.
Signed-off-by: Clemens Gruber <[email protected]>
---
Changes since v7:
- Handle sysfs power control as well and not just CONFIG_PM
Changes since v6:
- Improved !CONFIG_PM handling (wake it up without putting it to sleep
first)
drivers/pwm/pwm-pca9685.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index b39c0ba701ab..7f97965033e7 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -460,14 +460,20 @@ static int pca9685_pwm_probe(struct i2c_client *client,
return ret;
}
- /* The chip comes out of power-up in the active state */
- pm_runtime_set_active(&client->dev);
- /*
- * Enable will put the chip into suspend, which is what we
- * want as all outputs are disabled at this point
- */
pm_runtime_enable(&client->dev);
+ if (pm_runtime_enabled(&client->dev)) {
+ /*
+ * Although the chip comes out of power-up in the sleep state,
+ * we force it to sleep in case it was woken up before
+ */
+ pca9685_set_sleep_mode(pca, true);
+ pm_runtime_set_suspended(&client->dev);
+ } else {
+ /* Wake the chip up if runtime PM is disabled */
+ pca9685_set_sleep_mode(pca, false);
+ }
+
return 0;
}
@@ -479,7 +485,14 @@ static int pca9685_pwm_remove(struct i2c_client *client)
ret = pwmchip_remove(&pca->chip);
if (ret)
return ret;
+
+ if (!pm_runtime_enabled(&client->dev)) {
+ /* Put chip in sleep state if runtime PM is disabled */
+ pca9685_set_sleep_mode(pca, true);
+ }
+
pm_runtime_disable(&client->dev);
+
return 0;
}
--
2.31.1
Regmap operations can fail if the underlying subsystem is not working
properly (e.g. hogged I2C bus, etc.)
As this is useful information for the user, print an error message if it
happens.
Let probe fail if the first regmap_read or the first regmap_write fails.
Signed-off-by: Clemens Gruber <[email protected]>
---
Changes since v7:
- Use %pe instead of %d for error codes (Suggested by Uwe)
drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
1 file changed, 59 insertions(+), 24 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4583edd5e477..b0b321203947 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
return test_bit(channel, pca->pwms_enabled);
}
+static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
+{
+ struct device *dev = pca->chip.dev;
+ int err;
+
+ err = regmap_read(pca->regmap, reg, val);
+ if (err != 0)
+ dev_err(dev, "regmap_read of register 0x%x failed: %pe\n", reg, ERR_PTR(err));
+
+ return err;
+}
+
+static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
+{
+ struct device *dev = pca->chip.dev;
+ int err;
+
+ err = regmap_write(pca->regmap, reg, val);
+ if (err != 0)
+ dev_err(dev, "regmap_write to register 0x%x failed: %pe\n", reg, ERR_PTR(err));
+
+ return err;
+}
+
/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
@@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
if (duty == 0) {
/* Set the full OFF bit, which has the highest precedence */
- regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
return;
} else if (duty >= PCA9685_COUNTER_RANGE) {
/* Set the full ON bit and clear the full OFF bit */
- regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
- regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+ pca9685_write_reg(pca, REG_ON_H(channel), LED_FULL);
+ pca9685_write_reg(pca, REG_OFF_H(channel), 0);
return;
}
@@ -141,11 +165,11 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
off = (on + duty) % PCA9685_COUNTER_RANGE;
/* Set ON time (clears full ON bit) */
- regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
- regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+ pca9685_write_reg(pca, REG_ON_L(channel), on & 0xff);
+ pca9685_write_reg(pca, REG_ON_H(channel), (on >> 8) & 0xf);
/* Set OFF time (clears full OFF bit) */
- regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
- regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
+ pca9685_write_reg(pca, REG_OFF_L(channel), off & 0xff);
+ pca9685_write_reg(pca, REG_OFF_H(channel), (off >> 8) & 0xf);
}
static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
@@ -158,26 +182,26 @@ static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
return 0;
}
- regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+ pca9685_read_reg(pca, LED_N_OFF_H(channel), &off);
if (off & LED_FULL) {
/* Full OFF bit is set */
return 0;
}
- regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+ pca9685_read_reg(pca, LED_N_ON_H(channel), &on);
if (on & LED_FULL) {
/* Full ON bit is set */
return PCA9685_COUNTER_RANGE;
}
- regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ pca9685_read_reg(pca, LED_N_OFF_L(channel), &val);
off = ((off & 0xf) << 8) | (val & 0xff);
if (!pwm->args.usage_power)
return off;
/* Read ON register to calculate duty cycle of staggered output */
val = 0;
- regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
+ pca9685_read_reg(pca, LED_N_ON_L(channel), &val);
on = ((on & 0xf) << 8) | (val & 0xff);
return (off - on) & (PCA9685_COUNTER_RANGE - 1);
}
@@ -320,8 +344,15 @@ static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca)
static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
{
- regmap_update_bits(pca->regmap, PCA9685_MODE1,
- MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+ struct device *dev = pca->chip.dev;
+ int err = regmap_update_bits(pca->regmap, PCA9685_MODE1,
+ MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+ if (err != 0) {
+ dev_err(dev, "regmap_update_bits of register 0x%x failed: %pe\n",
+ PCA9685_MODE1, ERR_PTR(err));
+ return;
+ }
+
if (!enable) {
/* Wait 500us for the oscillator to be back up */
udelay(500);
@@ -350,7 +381,7 @@ static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
- regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
if (prescale != val) {
if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) {
dev_err(chip->dev,
@@ -368,7 +399,7 @@ static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
pca9685_set_sleep_mode(pca, true);
/* Change the chip-wide output frequency */
- regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
+ pca9685_write_reg(pca, PCA9685_PRESCALE, prescale);
/* Wake the chip up */
pca9685_set_sleep_mode(pca, false);
@@ -407,7 +438,7 @@ static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
unsigned int val = 0;
/* Calculate (chip-wide) period from prescale value */
- regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
/*
* PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
* The following calculation is therefore only a multiplication
@@ -504,7 +535,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
mutex_init(&pca->lock);
- regmap_read(pca->regmap, PCA9685_MODE2, ®);
+ ret = pca9685_read_reg(pca, PCA9685_MODE2, ®);
+ if (ret != 0)
+ return ret;
if (device_property_read_bool(&client->dev, "invert"))
reg |= MODE2_INVRT;
@@ -516,18 +549,20 @@ static int pca9685_pwm_probe(struct i2c_client *client,
else
reg |= MODE2_OUTDRV;
- regmap_write(pca->regmap, PCA9685_MODE2, reg);
+ ret = pca9685_write_reg(pca, PCA9685_MODE2, reg);
+ if (ret != 0)
+ return ret;
/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
- regmap_read(pca->regmap, PCA9685_MODE1, ®);
+ pca9685_read_reg(pca, PCA9685_MODE1, ®);
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
- regmap_write(pca->regmap, PCA9685_MODE1, reg);
+ pca9685_write_reg(pca, PCA9685_MODE1, reg);
/* Reset OFF/ON registers to POR default */
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
- regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
- regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_L, LED_FULL);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_H, LED_FULL);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_ON_L, 0);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_ON_H, 0);
pca->chip.ops = &pca9685_pwm_ops;
/* Add an extra channel for ALL_LED */
--
2.31.1
Hello Clemens,
On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> The switch to the atomic API goes hand in hand with a few fixes to
> previously experienced issues:
> - The duty cycle is no longer lost after disable/enable (previously the
> OFF registers were cleared in disable and the user was required to
> call config to restore the duty cycle settings)
> - If one sets a period resulting in the same prescale register value,
> the sleep and write to the register is now skipped
> - Previously, only the full ON bit was toggled in GPIO mode (and full
> OFF cleared if set to high), which could result in both full OFF and
> full ON not being set and on=0, off=0, which is not allowed according
> to the datasheet
> - The OFF registers were reset to 0 in probe, which could lead to the
> forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
>
> Signed-off-by: Clemens Gruber <[email protected]>
> ---
> Changes since v7:
> - Moved check for !state->enabled before prescaler configuration
> - Removed unnecessary cast
> - Use DIV_ROUND_DOWN in .apply
>
> Changes since v6:
> - Order of a comparison switched for improved readability
>
> Changes since v5:
> - Function documentation for set_duty
> - Variable initializations
> - Print warning if all LEDs channel
> - Changed EOPNOTSUPP to EINVAL
> - Improved error messages
> - Register reset corrections moved to this patch
>
> Changes since v4:
> - Patches split up
> - Use a single set_duty function
> - Improve readability / new macros
> - Added a patch to restrict prescale changes to the first user
>
> Changes since v3:
> - Refactoring: Extracted common functions
> - Read prescale register value instead of caching it
> - Return all zeros and disabled for "all LEDs" channel state
> - Improved duty calculation / mapping to 0..4096
>
> Changes since v2:
> - Always set default prescale value in probe
> - Simplified probe code
> - Inlined functions with one callsite
>
> Changes since v1:
> - Fixed a logic error
> - Impoved PM runtime handling and fixed !CONFIG_PM
> - Write default prescale reg value if invalid in probe
> - Reuse full_off/_on functions throughout driver
> - Use cached prescale value whenever possible
>
> drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> 1 file changed, 89 insertions(+), 170 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..827b57ced3c2 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)))
I'd like to see these named PCA9685_REG_ON_H etc.
> struct pca9685 {
> struct pwm_chip chip;
> struct regmap *regmap;
> - int period_ns;
> #if IS_ENABLED(CONFIG_GPIOLIB)
> struct mutex lock;
> struct gpio_chip gpio;
> @@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> +{
> + if (duty == 0) {
> + /* Set the full OFF bit, which has the highest precedence */
> + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + } else if (duty >= PCA9685_COUNTER_RANGE) {
> + /* Set the full ON bit and clear the full OFF bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> + } else {
> + /* Set OFF time (clears the full OFF bit) */
> + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> + /* Clear the full ON bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> + }
> +}
> +
> +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> +{
> + unsigned int off_h = 0, val = 0;
> +
> + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> + /* HW does not support reading state of "all LEDs" channel */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> + if (off_h & LED_FULL) {
> + /* Full OFF bit is set */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> + if (val & LED_FULL) {
> + /* Full ON bit is set */
> + return PCA9685_COUNTER_RANGE;
> + }
> +
> + val = 0;
> + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
I asked in the last round why you initialize val. You answered "just to
have it set to 0 in case regmap_read fails / val was not set." I wonder
if
ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
if (!ret)
/*
val = 0
would be better then and also make the intention obvious.
> + return ((off_h & 0xf) << 8) | (val & 0xff);
> +}
> +
> #if IS_ENABLED(CONFIG_GPIOLIB)
> static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> {
> @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
> - struct pwm_device *pwm = &pca->chip.pwms[offset];
> - unsigned int value;
>
> - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> -
> - return value & LED_FULL;
> + return pca9685_pwm_get_duty(pca, offset) != 0;
> }
>
> static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> int value)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
> - struct pwm_device *pwm = &pca->chip.pwms[offset];
> - unsigned int on = value ? LED_FULL : 0;
> -
> - /* Clear both OFF registers */
> - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
>
> - /* Set the full ON bit */
> - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> }
>
> static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> {
> struct pca9685 *pca = gpiochip_get_data(gpio);
>
> - pca9685_pwm_gpio_set(gpio, offset, 0);
> + pca9685_pwm_set_duty(pca, offset, 0);
> pm_runtime_put(pca->chip.dev);
> pca9685_pwm_clear_inuse(pca, offset);
> }
> @@ -246,167 +283,52 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> }
> }
>
> -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> - int duty_ns, int period_ns)
> +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> {
> struct pca9685 *pca = to_pca(chip);
> - unsigned long long duty;
> - unsigned int reg;
> - int prescale;
> -
> - if (period_ns != pca->period_ns) {
> - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> - PCA9685_COUNTER_RANGE * 1000) - 1;
> -
> - if (prescale >= PCA9685_PRESCALE_MIN &&
> - prescale <= PCA9685_PRESCALE_MAX) {
> - /*
> - * Putting the chip briefly into SLEEP mode
> - * at this point won't interfere with the
> - * pm_runtime framework, because the pm_runtime
> - * state is guaranteed active here.
> - */
> - /* Put chip into sleep mode */
> - pca9685_set_sleep_mode(pca, true);
> -
> - /* Change the chip-wide output frequency */
> - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> -
> - /* Wake the chip up */
> - pca9685_set_sleep_mode(pca, false);
> -
> - pca->period_ns = period_ns;
> - } else {
> - dev_err(chip->dev,
> - "prescaler not set: period out of bounds!\n");
> - return -EINVAL;
> - }
> - }
> + unsigned long long duty, prescale;
> + unsigned int val = 0;
>
> - if (duty_ns < 1) {
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EINVAL;
>
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - return 0;
> + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> + PCA9685_COUNTER_RANGE * 1000) - 1;
> + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> + return -EINVAL;
> }
>
> - if (duty_ns == period_ns) {
> - /* Clear both OFF registers */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_L;
> - else
> - reg = LED_N_OFF_L(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, 0x0);
> -
> - /* Set the full ON bit */
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_ON_H;
> - else
> - reg = LED_N_ON_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> + if (!state->enabled) {
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> 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, prescale);
>
> - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> + /* Wake the chip up */
> + pca9685_set_sleep_mode(pca, false);
> + }
>
> + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
If you round down here you should probably also round down in the
calculation of prescale. Also note that you're losing precision by using
state->period.
Consider the following request: state->period = 4177921 [ns] +
state->duty_cycle = 100000 [ns], then we get
PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
actually configure 96 to get 99840 ns.
So in the end I'd like to have the following:
PRESCALE = round-down(25 * state->period / 4096000) - 1
(to get the biggest period not bigger than state->period) and
duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
(to get the biggest duty cycle not bigger than state->duty_cycle). With
the example above this yields
PRESCALE = 24
duty = 100
which results in
.period = 4096000 / 25 * 25 = 4096000 [ns]
.duty_cycle = 100000 [ns]
Now you have a mixture of old and new with no consistent behaviour. So
please either stick to the old behaviour or do it right immediately.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > The switch to the atomic API goes hand in hand with a few fixes to
> > previously experienced issues:
> > - The duty cycle is no longer lost after disable/enable (previously the
> > OFF registers were cleared in disable and the user was required to
> > call config to restore the duty cycle settings)
> > - If one sets a period resulting in the same prescale register value,
> > the sleep and write to the register is now skipped
> > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > OFF cleared if set to high), which could result in both full OFF and
> > full ON not being set and on=0, off=0, which is not allowed according
> > to the datasheet
> > - The OFF registers were reset to 0 in probe, which could lead to the
> > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> >
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > Changes since v7:
> > - Moved check for !state->enabled before prescaler configuration
> > - Removed unnecessary cast
> > - Use DIV_ROUND_DOWN in .apply
> >
> > Changes since v6:
> > - Order of a comparison switched for improved readability
> >
> > Changes since v5:
> > - Function documentation for set_duty
> > - Variable initializations
> > - Print warning if all LEDs channel
> > - Changed EOPNOTSUPP to EINVAL
> > - Improved error messages
> > - Register reset corrections moved to this patch
> >
> > Changes since v4:
> > - Patches split up
> > - Use a single set_duty function
> > - Improve readability / new macros
> > - Added a patch to restrict prescale changes to the first user
> >
> > Changes since v3:
> > - Refactoring: Extracted common functions
> > - Read prescale register value instead of caching it
> > - Return all zeros and disabled for "all LEDs" channel state
> > - Improved duty calculation / mapping to 0..4096
> >
> > Changes since v2:
> > - Always set default prescale value in probe
> > - Simplified probe code
> > - Inlined functions with one callsite
> >
> > Changes since v1:
> > - Fixed a logic error
> > - Impoved PM runtime handling and fixed !CONFIG_PM
> > - Write default prescale reg value if invalid in probe
> > - Reuse full_off/_on functions throughout driver
> > - Use cached prescale value whenever possible
> >
> > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > 1 file changed, 89 insertions(+), 170 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..827b57ced3c2 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)))
>
> I'd like to see these named PCA9685_REG_ON_H etc.
I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
not have a prefix. If the prefix is mandatory, I think LED_N_.. should
also be prefixed, right?
>
> > struct pca9685 {
> > struct pwm_chip chip;
> > struct regmap *regmap;
> > - int period_ns;
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > struct mutex lock;
> > struct gpio_chip gpio;
> > @@ -87,6 +90,51 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > return container_of(chip, struct pca9685, chip);
> > }
> >
> > +/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> > +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > +{
> > + if (duty == 0) {
> > + /* Set the full OFF bit, which has the highest precedence */
> > + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > + } else if (duty >= PCA9685_COUNTER_RANGE) {
> > + /* Set the full ON bit and clear the full OFF bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > + } else {
> > + /* Set OFF time (clears the full OFF bit) */
> > + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > + /* Clear the full ON bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > + }
> > +}
> > +
> > +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > +{
> > + unsigned int off_h = 0, val = 0;
> > +
> > + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > + /* HW does not support reading state of "all LEDs" channel */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > + if (off_h & LED_FULL) {
> > + /* Full OFF bit is set */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > + if (val & LED_FULL) {
> > + /* Full ON bit is set */
> > + return PCA9685_COUNTER_RANGE;
> > + }
> > +
> > + val = 0;
> > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
>
> I asked in the last round why you initialize val. You answered "just to
> have it set to 0 in case regmap_read fails / val was not set." I wonder
> if
>
> ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> if (!ret)
> /*
> val = 0
>
> would be better then and also make the intention obvious.
I am not sure if that's more clear, but if others find it more obvious
like this, I can change it.
>
> > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > {
> > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > - unsigned int value;
> >
> > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > -
> > - return value & LED_FULL;
> > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > }
> >
> > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > int value)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > - unsigned int on = value ? LED_FULL : 0;
> > -
> > - /* Clear both OFF registers */
> > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> >
> > - /* Set the full ON bit */
> > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > }
> >
> > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > {
> > struct pca9685 *pca = gpiochip_get_data(gpio);
> >
> > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > + pca9685_pwm_set_duty(pca, offset, 0);
> > pm_runtime_put(pca->chip.dev);
> > pca9685_pwm_clear_inuse(pca, offset);
> > }
> > @@ -246,167 +283,52 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
> > }
> > }
> >
> > -static int pca9685_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> > - int duty_ns, int period_ns)
> > +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > - unsigned long long duty;
> > - unsigned int reg;
> > - int prescale;
> > -
> > - if (period_ns != pca->period_ns) {
> > - prescale = DIV_ROUND_CLOSEST(PCA9685_OSC_CLOCK_MHZ * period_ns,
> > - PCA9685_COUNTER_RANGE * 1000) - 1;
> > -
> > - if (prescale >= PCA9685_PRESCALE_MIN &&
> > - prescale <= PCA9685_PRESCALE_MAX) {
> > - /*
> > - * Putting the chip briefly into SLEEP mode
> > - * at this point won't interfere with the
> > - * pm_runtime framework, because the pm_runtime
> > - * state is guaranteed active here.
> > - */
> > - /* Put chip into sleep mode */
> > - pca9685_set_sleep_mode(pca, true);
> > -
> > - /* Change the chip-wide output frequency */
> > - regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > -
> > - /* Wake the chip up */
> > - pca9685_set_sleep_mode(pca, false);
> > -
> > - pca->period_ns = period_ns;
> > - } else {
> > - dev_err(chip->dev,
> > - "prescaler not set: period out of bounds!\n");
> > - return -EINVAL;
> > - }
> > - }
> > + unsigned long long duty, prescale;
> > + unsigned int val = 0;
> >
> > - if (duty_ns < 1) {
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EINVAL;
> >
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - return 0;
> > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > + return -EINVAL;
> > }
> >
> > - if (duty_ns == period_ns) {
> > - /* Clear both OFF registers */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_L;
> > - else
> > - reg = LED_N_OFF_L(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, 0x0);
> > -
> > - /* Set the full ON bit */
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_ON_H;
> > - else
> > - reg = LED_N_ON_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > + if (!state->enabled) {
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > 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, prescale);
> >
> > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > + /* Wake the chip up */
> > + pca9685_set_sleep_mode(pca, false);
> > + }
> >
> > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
>
> If you round down here you should probably also round down in the
> calculation of prescale. Also note that you're losing precision by using
> state->period.
>
> Consider the following request: state->period = 4177921 [ns] +
> state->duty_cycle = 100000 [ns], then we get
> PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> actually configure 96 to get 99840 ns.
>
> So in the end I'd like to have the following:
>
> PRESCALE = round-down(25 * state->period / 4096000) - 1
>
> (to get the biggest period not bigger than state->period) and
>
> duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
>
> (to get the biggest duty cycle not bigger than state->duty_cycle). With
> the example above this yields
>
> PRESCALE = 24
> duty = 100
>
> which results in
>
> .period = 4096000 / 25 * 25 = 4096000 [ns]
> .duty_cycle = 100000 [ns]
>
> Now you have a mixture of old and new with no consistent behaviour. So
> please either stick to the old behaviour or do it right immediately.
I avoided rounding down the prescale value because the datasheet has an
example where a round-closest is used, see page 25.
With your suggested round-down, the example with frequency of 200 Hz
would no longer result in 30 but 29 and that contradicts the datasheet.
So would you rather have me keep the old duty rounding behaviour?
Meaning: Keep rounding up the duty calculation in apply and use
round-down in the new .get_state function?
Thanks,
Clemens
Add the flag and corresponding documentation for PWM_USAGE_POWER.
Cc: Rob Herring <[email protected]>
Signed-off-by: Clemens Gruber <[email protected]>
---
Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
include/dt-bindings/pwm/pwm.h | 1 +
2 files changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 084886bd721e..fe3a28f887c0 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -46,6 +46,9 @@ period in nanoseconds.
Optionally, the pwm-specifier can encode a number of flags (defined in
<dt-bindings/pwm/pwm.h>) in a third cell:
- PWM_POLARITY_INVERTED: invert the PWM signal polarity
+- PWM_USAGE_POWER: Only care about the power output of the signal. This
+ allows drivers (if supported) to optimize the signals, for example to
+ improve EMI and reduce current spikes.
Example with optional PWM specifier for inverse polarity
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index ab9a077e3c7d..0d5a8f0c0035 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -11,5 +11,6 @@
#define _DT_BINDINGS_PWM_PWM_H
#define PWM_POLARITY_INVERTED (1 << 0)
+#define PWM_USAGE_POWER (1 << 1)
#endif
--
2.31.1
If PWM_USAGE_POWER is set on a PWM, the pca9685 driver will phase shift
the individual channels relative to their channel number. This improves
EMI because the enabled channels no longer turn on at the same time,
while still maintaining the configured duty cycle / power output.
Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 63 ++++++++++++++++++++++++++++++---------
1 file changed, 49 insertions(+), 14 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 7f97965033e7..410b93b115dc 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -93,46 +93,76 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
+ struct pwm_device *pwm = &pca->chip.pwms[channel];
+ unsigned int on, off;
+
if (duty == 0) {
/* Set the full OFF bit, which has the highest precedence */
regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ return;
} else if (duty >= PCA9685_COUNTER_RANGE) {
/* Set the full ON bit and clear the full OFF bit */
regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
regmap_write(pca->regmap, REG_OFF_H(channel), 0);
- } else {
- /* Set OFF time (clears the full OFF bit) */
- regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
- regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
- /* Clear the full ON bit */
- regmap_write(pca->regmap, REG_ON_H(channel), 0);
+ return;
}
+
+
+ if (pwm->args.usage_power && channel < PCA9685_MAXCHAN) {
+ /*
+ * If PWM_USAGE_POWER is set on a PWM, the pca9685
+ * driver will phase shift the individual channels
+ * relative to their channel number.
+ * This improves EMI because the enabled channels no
+ * longer turn on at the same time, while still
+ * maintaining the configured duty cycle / power output.
+ */
+ on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
+ } else
+ on = 0;
+
+ off = (on + duty) % PCA9685_COUNTER_RANGE;
+
+ /* Set ON time (clears full ON bit) */
+ regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
+ regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+ /* Set OFF time (clears full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
}
static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
{
- unsigned int off_h = 0, val = 0;
+ struct pwm_device *pwm = &pca->chip.pwms[channel];
+ unsigned int off = 0, on = 0, val = 0;
if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
/* HW does not support reading state of "all LEDs" channel */
return 0;
}
- regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
- if (off_h & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+ if (off & LED_FULL) {
/* Full OFF bit is set */
return 0;
}
- regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
- if (val & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+ if (on & LED_FULL) {
/* Full ON bit is set */
return PCA9685_COUNTER_RANGE;
}
- val = 0;
regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
- return ((off_h & 0xf) << 8) | (val & 0xff);
+ off = ((off & 0xf) << 8) | (val & 0xff);
+ if (!pwm->args.usage_power)
+ return off;
+
+ /* Read ON register to calculate duty cycle of staggered output */
+ val = 0;
+ regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
+ on = ((on & 0xf) << 8) | (val & 0xff);
+ return (off - on) & (PCA9685_COUNTER_RANGE - 1);
}
#if IS_ENABLED(CONFIG_GPIOLIB)
@@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
regmap_write(pca->regmap, PCA9685_MODE1, reg);
- /* Reset OFF registers to POR default */
+ /* Reset OFF/ON registers to POR default */
regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
pca->chip.ops = &pca9685_pwm_ops;
/* Add an extra channel for ALL_LED */
@@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
pca->chip.dev = &client->dev;
pca->chip.base = -1;
+ pca->chip.of_xlate = of_pwm_xlate_with_flags;
+ pca->chip.of_pwm_n_cells = 3;
+
ret = pwmchip_add(&pca->chip);
if (ret < 0)
return ret;
--
2.31.1
Implement .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 <[email protected]>
---
Changes since v7:
- Always return enabled=true for channels except "all LEDs" channel
- Use DIV_ROUND_UP in .get_state
Changes since v6:
- Added a comment regarding the division (Suggested by Uwe)
- Rebased
drivers/pwm/pwm-pca9685.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 827b57ced3c2..b39c0ba701ab 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -329,6 +329,41 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
+static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+ struct pwm_state *state)
+{
+ struct pca9685 *pca = to_pca(chip);
+ unsigned long long duty;
+ unsigned int val = 0;
+
+ /* Calculate (chip-wide) period from prescale value */
+ regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ /*
+ * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
+ * The following calculation is therefore only a multiplication
+ * and we are not losing precision.
+ */
+ state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
+ (val + 1);
+
+ /* The (per-channel) polarity is fixed */
+ state->polarity = PWM_POLARITY_NORMAL;
+
+ if (pwm->hwpwm >= PCA9685_MAXCHAN) {
+ /*
+ * The "all LEDs" channel does not support HW readout
+ * Return 0 and disabled for backwards compatibility
+ */
+ state->duty_cycle = 0;
+ state->enabled = false;
+ return;
+ }
+
+ state->enabled = true;
+ duty = pca9685_pwm_get_duty(pca, pwm->hwpwm);
+ state->duty_cycle = DIV_ROUND_UP_ULL(duty * state->period, PCA9685_COUNTER_RANGE);
+}
+
static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
@@ -351,6 +386,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
static const struct pwm_ops pca9685_pwm_ops = {
.apply = pca9685_pwm_apply,
+ .get_state = pca9685_pwm_get_state,
.request = pca9685_pwm_request,
.free = pca9685_pwm_free,
.owner = THIS_MODULE,
--
2.31.1
If the flag PWM_USAGE_POWER is set on a channel, the PWM driver may
optimize the signal as long as the power output is not changed.
Depending on the specific driver, the optimization could for example
improve EMI (if supported) by phase-shifting the individual channels.
Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/core.c | 9 +++++++--
include/linux/pwm.h | 1 +
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index a8eff4b3ee36..56a9c739e1b2 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -153,9 +153,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
pwm->args.period = args->args[1];
pwm->args.polarity = PWM_POLARITY_NORMAL;
+ pwm->args.usage_power = false;
- if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED)
- pwm->args.polarity = PWM_POLARITY_INVERSED;
+ if (args->args_count > 2) {
+ if (args->args[2] & PWM_POLARITY_INVERTED)
+ pwm->args.polarity = PWM_POLARITY_INVERSED;
+ if (args->args[2] & PWM_USAGE_POWER)
+ pwm->args.usage_power = true;
+ }
return pwm;
}
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e4d84d4db293..555e050e8bec 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -41,6 +41,7 @@ enum pwm_polarity {
struct pwm_args {
u64 period;
enum pwm_polarity polarity;
+ bool usage_power;
};
enum {
--
2.31.1
On Mon, Apr 12, 2021 at 03:27:43PM +0200, Clemens Gruber wrote:
> If PWM_USAGE_POWER is set on a PWM, the pca9685 driver will phase shift
> the individual channels relative to their channel number. This improves
> EMI because the enabled channels no longer turn on at the same time,
> while still maintaining the configured duty cycle / power output.
>
> Signed-off-by: Clemens Gruber <[email protected]>
> ---
> drivers/pwm/pwm-pca9685.c | 63 ++++++++++++++++++++++++++++++---------
> 1 file changed, 49 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 7f97965033e7..410b93b115dc 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -93,46 +93,76 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> {
> + struct pwm_device *pwm = &pca->chip.pwms[channel];
> + unsigned int on, off;
> +
> if (duty == 0) {
> /* Set the full OFF bit, which has the highest precedence */
> regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + return;
> } else if (duty >= PCA9685_COUNTER_RANGE) {
> /* Set the full ON bit and clear the full OFF bit */
> regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> - } else {
> - /* Set OFF time (clears the full OFF bit) */
> - regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> - regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> - /* Clear the full ON bit */
> - regmap_write(pca->regmap, REG_ON_H(channel), 0);
> + return;
> }
> +
> +
> + if (pwm->args.usage_power && channel < PCA9685_MAXCHAN) {
> + /*
> + * If PWM_USAGE_POWER is set on a PWM, the pca9685
> + * driver will phase shift the individual channels
> + * relative to their channel number.
> + * This improves EMI because the enabled channels no
> + * longer turn on at the same time, while still
> + * maintaining the configured duty cycle / power output.
> + */
> + on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
> + } else
> + on = 0;
> +
> + off = (on + duty) % PCA9685_COUNTER_RANGE;
> +
> + /* Set ON time (clears full ON bit) */
> + regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
> + regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
> + /* Set OFF time (clears full OFF bit) */
> + regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
> }
>
> static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> {
> - unsigned int off_h = 0, val = 0;
> + struct pwm_device *pwm = &pca->chip.pwms[channel];
> + unsigned int off = 0, on = 0, val = 0;
>
> if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> /* HW does not support reading state of "all LEDs" channel */
> return 0;
> }
>
> - regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> - if (off_h & LED_FULL) {
> + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
> + if (off & LED_FULL) {
> /* Full OFF bit is set */
> return 0;
> }
>
> - regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> - if (val & LED_FULL) {
> + regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
> + if (on & LED_FULL) {
> /* Full ON bit is set */
> return PCA9685_COUNTER_RANGE;
> }
>
> - val = 0;
> regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> - return ((off_h & 0xf) << 8) | (val & 0xff);
> + off = ((off & 0xf) << 8) | (val & 0xff);
> + if (!pwm->args.usage_power)
> + return off;
> +
> + /* Read ON register to calculate duty cycle of staggered output */
> + val = 0;
> + regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
> + on = ((on & 0xf) << 8) | (val & 0xff);
> + return (off - on) & (PCA9685_COUNTER_RANGE - 1);
If LED_N_ON is != 0 but usage_power is false, the returned state is
bogus.
> }
>
> #if IS_ENABLED(CONFIG_GPIOLIB)
> @@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> regmap_write(pca->regmap, PCA9685_MODE1, reg);
>
> - /* Reset OFF registers to POR default */
> + /* Reset OFF/ON registers to POR default */
> regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
>
> pca->chip.ops = &pca9685_pwm_ops;
> /* Add an extra channel for ALL_LED */
> @@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> pca->chip.dev = &client->dev;
> pca->chip.base = -1;
>
> + pca->chip.of_xlate = of_pwm_xlate_with_flags;
> + pca->chip.of_pwm_n_cells = 3;
> +
Huh, you're incompatibly changing the device tree binding here.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi,
On Mon, Apr 12, 2021 at 06:30:45PM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Apr 12, 2021 at 03:27:43PM +0200, Clemens Gruber wrote:
> > If PWM_USAGE_POWER is set on a PWM, the pca9685 driver will phase shift
> > the individual channels relative to their channel number. This improves
> > EMI because the enabled channels no longer turn on at the same time,
> > while still maintaining the configured duty cycle / power output.
> >
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > drivers/pwm/pwm-pca9685.c | 63 ++++++++++++++++++++++++++++++---------
> > 1 file changed, 49 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 7f97965033e7..410b93b115dc 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -93,46 +93,76 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > /* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
> > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > {
> > + struct pwm_device *pwm = &pca->chip.pwms[channel];
> > + unsigned int on, off;
> > +
> > if (duty == 0) {
> > /* Set the full OFF bit, which has the highest precedence */
> > regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > + return;
> > } else if (duty >= PCA9685_COUNTER_RANGE) {
> > /* Set the full ON bit and clear the full OFF bit */
> > regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > - } else {
> > - /* Set OFF time (clears the full OFF bit) */
> > - regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > - regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > - /* Clear the full ON bit */
> > - regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > + return;
> > }
> > +
> > +
> > + if (pwm->args.usage_power && channel < PCA9685_MAXCHAN) {
> > + /*
> > + * If PWM_USAGE_POWER is set on a PWM, the pca9685
> > + * driver will phase shift the individual channels
> > + * relative to their channel number.
> > + * This improves EMI because the enabled channels no
> > + * longer turn on at the same time, while still
> > + * maintaining the configured duty cycle / power output.
> > + */
> > + on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
> > + } else
> > + on = 0;
> > +
> > + off = (on + duty) % PCA9685_COUNTER_RANGE;
> > +
> > + /* Set ON time (clears full ON bit) */
> > + regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
> > + regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
> > + /* Set OFF time (clears full OFF bit) */
> > + regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
> > }
> >
> > static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > {
> > - unsigned int off_h = 0, val = 0;
> > + struct pwm_device *pwm = &pca->chip.pwms[channel];
> > + unsigned int off = 0, on = 0, val = 0;
> >
> > if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > /* HW does not support reading state of "all LEDs" channel */
> > return 0;
> > }
> >
> > - regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > - if (off_h & LED_FULL) {
> > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
> > + if (off & LED_FULL) {
> > /* Full OFF bit is set */
> > return 0;
> > }
> >
> > - regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > - if (val & LED_FULL) {
> > + regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
> > + if (on & LED_FULL) {
> > /* Full ON bit is set */
> > return PCA9685_COUNTER_RANGE;
> > }
> >
> > - val = 0;
> > regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > - return ((off_h & 0xf) << 8) | (val & 0xff);
> > + off = ((off & 0xf) << 8) | (val & 0xff);
> > + if (!pwm->args.usage_power)
> > + return off;
> > +
> > + /* Read ON register to calculate duty cycle of staggered output */
> > + val = 0;
> > + regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
> > + on = ((on & 0xf) << 8) | (val & 0xff);
> > + return (off - on) & (PCA9685_COUNTER_RANGE - 1);
>
> If LED_N_ON is != 0 but usage_power is false, the returned state is
> bogus.
If usage_power is false, LED_N_ON is guaranteed to be 0. It is reset to
0 in probe and never changed. Or did I miss something?
>
> > }
> >
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > @@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > regmap_write(pca->regmap, PCA9685_MODE1, reg);
> >
> > - /* Reset OFF registers to POR default */
> > + /* Reset OFF/ON registers to POR default */
> > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> >
> > pca->chip.ops = &pca9685_pwm_ops;
> > /* Add an extra channel for ALL_LED */
> > @@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > pca->chip.dev = &client->dev;
> > pca->chip.base = -1;
> >
> > + pca->chip.of_xlate = of_pwm_xlate_with_flags;
> > + pca->chip.of_pwm_n_cells = 3;
> > +
>
> Huh, you're incompatibly changing the device tree binding here.
No, I don't think so:
The third cell is optional with of_pwm_xlate_with_flags.
So previous DTs with pwm-cells = <2> will still work.
If you want to use the new flag for some PWMs you have to set pwm-cells
to <3> and PWM_USAGE_POWER (or 0) in the third field at the consumer.
This should not break backwards compatibility. Let me know if I missed
something.
Thanks for your review,
Clemens
Hello Clemens,
On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > The switch to the atomic API goes hand in hand with a few fixes to
> > > previously experienced issues:
> > > - The duty cycle is no longer lost after disable/enable (previously the
> > > OFF registers were cleared in disable and the user was required to
> > > call config to restore the duty cycle settings)
> > > - If one sets a period resulting in the same prescale register value,
> > > the sleep and write to the register is now skipped
> > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > OFF cleared if set to high), which could result in both full OFF and
> > > full ON not being set and on=0, off=0, which is not allowed according
> > > to the datasheet
> > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > >
> > > Signed-off-by: Clemens Gruber <[email protected]>
> > > ---
> > > Changes since v7:
> > > - Moved check for !state->enabled before prescaler configuration
> > > - Removed unnecessary cast
> > > - Use DIV_ROUND_DOWN in .apply
> > >
> > > Changes since v6:
> > > - Order of a comparison switched for improved readability
> > >
> > > Changes since v5:
> > > - Function documentation for set_duty
> > > - Variable initializations
> > > - Print warning if all LEDs channel
> > > - Changed EOPNOTSUPP to EINVAL
> > > - Improved error messages
> > > - Register reset corrections moved to this patch
> > >
> > > Changes since v4:
> > > - Patches split up
> > > - Use a single set_duty function
> > > - Improve readability / new macros
> > > - Added a patch to restrict prescale changes to the first user
> > >
> > > Changes since v3:
> > > - Refactoring: Extracted common functions
> > > - Read prescale register value instead of caching it
> > > - Return all zeros and disabled for "all LEDs" channel state
> > > - Improved duty calculation / mapping to 0..4096
> > >
> > > Changes since v2:
> > > - Always set default prescale value in probe
> > > - Simplified probe code
> > > - Inlined functions with one callsite
> > >
> > > Changes since v1:
> > > - Fixed a logic error
> > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > - Write default prescale reg value if invalid in probe
> > > - Reuse full_off/_on functions throughout driver
> > > - Use cached prescale value whenever possible
> > >
> > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > index 4a55dc18656c..827b57ced3c2 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)))
> >
> > I'd like to see these named PCA9685_REG_ON_H etc.
>
> I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> also be prefixed, right?
I'd like to seem the prefixed (and assume that Thierry doesn't agree).
IMHO it's good style and even though it yields longer name usually it
yields easier understandable code. (But this seems to be subjective.)
> > > + val = 0;
> > > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> >
> > I asked in the last round why you initialize val. You answered "just to
> > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > if
> >
> > ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > if (!ret)
> > /*
I intended to write something like
/* initialize val in case reading LED_N_OFF failed */
> > val = 0
> >
> > would be better then and also make the intention obvious.
>
> I am not sure if that's more clear, but if others find it more obvious
> like this, I can change it.
>
> >
> > > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > > +}
> > > +
> > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > {
> > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > {
> > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > - unsigned int value;
> > >
> > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > -
> > > - return value & LED_FULL;
> > > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > > }
> > >
> > > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > int value)
> > > {
> > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > - unsigned int on = value ? LED_FULL : 0;
> > > -
> > > - /* Clear both OFF registers */
> > > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > >
> > > - /* Set the full ON bit */
> > > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > }
> > >
> > > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > {
> > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > >
> > > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > > + pca9685_pwm_set_duty(pca, offset, 0);
> > > pm_runtime_put(pca->chip.dev);
> > > pca9685_pwm_clear_inuse(pca, offset);
> > > }
> > > @@ -246,167 +283,52 @@ 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)
> > > {
> > > [...]
> > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > + return -EINVAL;
> > > }
> > >
> > > [...]
> > > + if (!state->enabled) {
> > > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > return 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);
> > >
> > > [...]
> > > + /* Change the chip-wide output frequency */
> > > + regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > >
> > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > + /* Wake the chip up */
> > > + pca9685_set_sleep_mode(pca, false);
> > > + }
> > >
> > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> >
> > If you round down here you should probably also round down in the
> > calculation of prescale. Also note that you're losing precision by using
> > state->period.
> >
> > Consider the following request: state->period = 4177921 [ns] +
> > state->duty_cycle = 100000 [ns], then we get
> > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > actually configure 96 to get 99840 ns.
> >
> > So in the end I'd like to have the following:
> >
> > PRESCALE = round-down(25 * state->period / 4096000) - 1
> >
> > (to get the biggest period not bigger than state->period) and
> >
> > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> >
> > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > the example above this yields
> >
> > PRESCALE = 24
> > duty = 100
> >
> > which results in
> >
> > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > .duty_cycle = 100000 [ns]
> >
> > Now you have a mixture of old and new with no consistent behaviour. So
> > please either stick to the old behaviour or do it right immediately.
>
> I avoided rounding down the prescale value because the datasheet has an
> example where a round-closest is used, see page 25.
The hardware guy who wrote this data sheet wasn't aware of the rounding
rules for Linux PWM drivers :-)
> With your suggested round-down, the example with frequency of 200 Hz
> would no longer result in 30 but 29 and that contradicts the datasheet.
Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
29 or 30, you don't get 200 Hz. And which of the two possible values is
the better one depends on the consumer, no matter what rounding
algorithm the data sheet suggests. Also note that the math here contains
surprises you don't expect at first. For example, what PRESCALE value
would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
press Space now to pause and let you think first :-)] The data sheet's
formula suggests:
round(25 MHz / (4096 * 284)) - 1 = 20
The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
(error = 6.567 Hz), so 21 is the better choice.
Exercise for the reader:
What is the correct formula to really determine the PRESCALE value that
yields the best approximation (i.e. minimizing
abs(real_freq - target_freq)) for a given target_freq?
These things don't happen when you round down only.
> So would you rather have me keep the old duty rounding behaviour?
>
> Meaning: Keep rounding up the duty calculation in apply and use
> round-down in the new .get_state function?
There are two things I want:
a) To improve consistency among the PWM drivers (and to keep the math
simple and unsurprising), the pca9685 driver should use round-down
instead of round-nearest (or whatever mix it is currently using).
b) .get_state should be the inverse to .apply in the sense that
applying the result of .get_state is idempotent.
I don't care much how you get there, so it's up to you if you do that in
this patch that converts to .apply, or if you keep the math as is and
then adapt the rounding behaviour in a separate change. But changing the
algorithm in this patch and not getting to the "good" one is ugly, so
please don't do that.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Clemens,
On Mon, Apr 12, 2021 at 07:11:58PM +0200, Clemens Gruber wrote:
> On Mon, Apr 12, 2021 at 06:30:45PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Apr 12, 2021 at 03:27:43PM +0200, Clemens Gruber wrote:
> > > static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > > {
> > > - unsigned int off_h = 0, val = 0;
> > > + struct pwm_device *pwm = &pca->chip.pwms[channel];
> > > + unsigned int off = 0, on = 0, val = 0;
> > >
> > > if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > > /* HW does not support reading state of "all LEDs" channel */
> > > return 0;
> > > }
> > >
> > > - regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > > - if (off_h & LED_FULL) {
> > > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
> > > + if (off & LED_FULL) {
> > > /* Full OFF bit is set */
> > > return 0;
> > > }
> > >
> > > - regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > > - if (val & LED_FULL) {
> > > + regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
> > > + if (on & LED_FULL) {
> > > /* Full ON bit is set */
> > > return PCA9685_COUNTER_RANGE;
> > > }
> > >
> > > - val = 0;
> > > regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > - return ((off_h & 0xf) << 8) | (val & 0xff);
> > > + off = ((off & 0xf) << 8) | (val & 0xff);
> > > + if (!pwm->args.usage_power)
> > > + return off;
> > > +
> > > + /* Read ON register to calculate duty cycle of staggered output */
> > > + val = 0;
> > > + regmap_read(pca->regmap, LED_N_ON_L(channel), &val);
> > > + on = ((on & 0xf) << 8) | (val & 0xff);
> > > + return (off - on) & (PCA9685_COUNTER_RANGE - 1);
> >
> > If LED_N_ON is != 0 but usage_power is false, the returned state is
> > bogus.
>
> If usage_power is false, LED_N_ON is guaranteed to be 0. It is reset to
> 0 in probe and never changed. Or did I miss something?
Ah right, so my concern is only a challenge once the reset in probe goes
away.
> > > }
> > >
> > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > @@ -439,9 +469,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > > reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
> > > regmap_write(pca->regmap, PCA9685_MODE1, reg);
> > >
> > > - /* Reset OFF registers to POR default */
> > > + /* Reset OFF/ON registers to POR default */
> > > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
> > > regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
> > > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
> > > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
> > >
> > > pca->chip.ops = &pca9685_pwm_ops;
> > > /* Add an extra channel for ALL_LED */
> > > @@ -450,6 +482,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > > pca->chip.dev = &client->dev;
> > > pca->chip.base = -1;
> > >
> > > + pca->chip.of_xlate = of_pwm_xlate_with_flags;
> > > + pca->chip.of_pwm_n_cells = 3;
> > > +
> >
> > Huh, you're incompatibly changing the device tree binding here.
>
> No, I don't think so:
>
> The third cell is optional with of_pwm_xlate_with_flags.
> So previous DTs with pwm-cells = <2> will still work.
I thought that .of_pwm_n_cells was enforced, let me check the code ... I
had in mind that of_pwm_get() enforced that, but I cannot find it, so I
guess you're right and my concern is unjustified.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> Hi Uwe,
>
> On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > Hello Clemens,
> >
> > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > previously experienced issues:
> > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > > OFF registers were cleared in disable and the user was required to
> > > > > call config to restore the duty cycle settings)
> > > > > - If one sets a period resulting in the same prescale register value,
> > > > > the sleep and write to the register is now skipped
> > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > > OFF cleared if set to high), which could result in both full OFF and
> > > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > > to the datasheet
> > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > >
> > > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > > ---
> > > > > Changes since v7:
> > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > - Removed unnecessary cast
> > > > > - Use DIV_ROUND_DOWN in .apply
> > > > >
> > > > > Changes since v6:
> > > > > - Order of a comparison switched for improved readability
> > > > >
> > > > > Changes since v5:
> > > > > - Function documentation for set_duty
> > > > > - Variable initializations
> > > > > - Print warning if all LEDs channel
> > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > - Improved error messages
> > > > > - Register reset corrections moved to this patch
> > > > >
> > > > > Changes since v4:
> > > > > - Patches split up
> > > > > - Use a single set_duty function
> > > > > - Improve readability / new macros
> > > > > - Added a patch to restrict prescale changes to the first user
> > > > >
> > > > > Changes since v3:
> > > > > - Refactoring: Extracted common functions
> > > > > - Read prescale register value instead of caching it
> > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > - Improved duty calculation / mapping to 0..4096
> > > > >
> > > > > Changes since v2:
> > > > > - Always set default prescale value in probe
> > > > > - Simplified probe code
> > > > > - Inlined functions with one callsite
> > > > >
> > > > > Changes since v1:
> > > > > - Fixed a logic error
> > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > - Write default prescale reg value if invalid in probe
> > > > > - Reuse full_off/_on functions throughout driver
> > > > > - Use cached prescale value whenever possible
> > > > >
> > > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > index 4a55dc18656c..827b57ced3c2 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)))
> > > >
> > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > >
> > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > also be prefixed, right?
> >
> > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > IMHO it's good style and even though it yields longer name usually it
> > yields easier understandable code. (But this seems to be subjective.)
>
> I am not sure I want to also rename the existing LED_N_OFF stuff in this
> patch. Maybe we can discuss unifying the macros (either with or without
> prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> match the LED_N_ stuff?
>
> >
> > > > > + val = 0;
> > > > > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > >
> > > > I asked in the last round why you initialize val. You answered "just to
> > > > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > > > if
> > > >
> > > > ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > if (!ret)
> > > > /*
> >
> > I intended to write something like
> >
> > /* initialize val in case reading LED_N_OFF failed */
> >
> > > > val = 0
> > > >
> > > > would be better then and also make the intention obvious.
>
> Maybe a little bit better.. I can change it.
>
> > >
> > > I am not sure if that's more clear, but if others find it more obvious
> > > like this, I can change it.
> > >
> > > >
> > > > > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > > > > +}
> > > > > +
> > > > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > > > {
> > > > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > - unsigned int value;
> > > > >
> > > > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > > > -
> > > > > - return value & LED_FULL;
> > > > > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > > > > }
> > > > >
> > > > > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > > > int value)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > - unsigned int on = value ? LED_FULL : 0;
> > > > > -
> > > > > - /* Clear both OFF registers */
> > > > > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > > > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > > > >
> > > > > - /* Set the full ON bit */
> > > > > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > > > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > > > }
> > > > >
> > > > > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > >
> > > > > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > > > > + pca9685_pwm_set_duty(pca, offset, 0);
> > > > > pm_runtime_put(pca->chip.dev);
> > > > > pca9685_pwm_clear_inuse(pca, offset);
> > > > > }
> > > > > @@ -246,167 +283,52 @@ 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)
> > > > > {
> > > > > [...]
> > > > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > > > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > > > + return -EINVAL;
> > > > > }
> > > > >
> > > > > [...]
> > > > > + if (!state->enabled) {
> > > > > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > > > return 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);
> > > > >
> > > > > [...]
> > > > > + /* Change the chip-wide output frequency */
> > > > > + regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > > >
> > > > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > > > + /* Wake the chip up */
> > > > > + pca9685_set_sleep_mode(pca, false);
> > > > > + }
> > > > >
> > > > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > > > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> > > >
> > > > If you round down here you should probably also round down in the
> > > > calculation of prescale. Also note that you're losing precision by using
> > > > state->period.
> > > >
> > > > Consider the following request: state->period = 4177921 [ns] +
> > > > state->duty_cycle = 100000 [ns], then we get
> > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > actually configure 96 to get 99840 ns.
> > > >
> > > > So in the end I'd like to have the following:
> > > >
> > > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > >
> > > > (to get the biggest period not bigger than state->period) and
> > > >
> > > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > >
> > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > the example above this yields
> > > >
> > > > PRESCALE = 24
> > > > duty = 100
> > > >
> > > > which results in
> > > >
> > > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > .duty_cycle = 100000 [ns]
> > > >
> > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > please either stick to the old behaviour or do it right immediately.
> > >
> > > I avoided rounding down the prescale value because the datasheet has an
> > > example where a round-closest is used, see page 25.
> >
> > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > rules for Linux PWM drivers :-)
> >
> > > With your suggested round-down, the example with frequency of 200 Hz
> > > would no longer result in 30 but 29 and that contradicts the datasheet.
> >
> > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > the better one depends on the consumer, no matter what rounding
> > algorithm the data sheet suggests. Also note that the math here contains
> > surprises you don't expect at first. For example, what PRESCALE value
> > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > press Space now to pause and let you think first :-)] The data sheet's
> > formula suggests:
> >
> > round(25 MHz / (4096 * 284)) - 1 = 20
> >
> > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > (error = 6.567 Hz), so 21 is the better choice.
> >
> > Exercise for the reader:
> > What is the correct formula to really determine the PRESCALE value that
> > yields the best approximation (i.e. minimizing
> > abs(real_freq - target_freq)) for a given target_freq?
> >
> > These things don't happen when you round down only.
>
> Sure, but it might also be counterintuitive that the Linux driver does
> not use the same formula as the datasheet. And when using 200 Hz, 29 is
> a little closer than 30.
Sorry, I meant: 30 (196.88 Hz) is a little closer than 29 (203.45 Hz).
> I once measured the actual frequency and the internal oscillator is not
> very accurate, so even if you think you should get 196.88 Hz, the actual
> frequency measured with a decent scope is about 206 Hz and varies from
> chip to chip (~?205-207 Hz).
>
> >
> > > So would you rather have me keep the old duty rounding behaviour?
> > >
> > > Meaning: Keep rounding up the duty calculation in apply and use
> > > round-down in the new .get_state function?
> >
> > There are two things I want:
> >
> > a) To improve consistency among the PWM drivers (and to keep the math
> > simple and unsurprising), the pca9685 driver should use round-down
> > instead of round-nearest (or whatever mix it is currently using).
> >
> > b) .get_state should be the inverse to .apply in the sense that
> > applying the result of .get_state is idempotent.
> >
> > I don't care much how you get there, so it's up to you if you do that in
> > this patch that converts to .apply, or if you keep the math as is and
> > then adapt the rounding behaviour in a separate change. But changing the
> > algorithm in this patch and not getting to the "good" one is ugly, so
> > please don't do that.
>
> OK, then I think the best option is to keep the math as it was before
> and if we want to change the rounding behaviour we do this in a separate
> patch in the future. Then we can continue the discussion wether changing
> the prescaler formula to round-down even though the datasheet does it
> otherwise is the way to go.
>
> Thanks,
> Clemens
On Tue, Apr 13, 2021 at 02:37:50PM +0200, Thierry Reding wrote:
> On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> > Hi Uwe,
> >
> > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > > Hello Clemens,
> > >
> > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > > previously experienced issues:
> > > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > > > OFF registers were cleared in disable and the user was required to
> > > > > > call config to restore the duty cycle settings)
> > > > > > - If one sets a period resulting in the same prescale register value,
> > > > > > the sleep and write to the register is now skipped
> > > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > > > OFF cleared if set to high), which could result in both full OFF and
> > > > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > > > to the datasheet
> > > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > > >
> > > > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > > > ---
> > > > > > Changes since v7:
> > > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > > - Removed unnecessary cast
> > > > > > - Use DIV_ROUND_DOWN in .apply
> > > > > >
> > > > > > Changes since v6:
> > > > > > - Order of a comparison switched for improved readability
> > > > > >
> > > > > > Changes since v5:
> > > > > > - Function documentation for set_duty
> > > > > > - Variable initializations
> > > > > > - Print warning if all LEDs channel
> > > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > > - Improved error messages
> > > > > > - Register reset corrections moved to this patch
> > > > > >
> > > > > > Changes since v4:
> > > > > > - Patches split up
> > > > > > - Use a single set_duty function
> > > > > > - Improve readability / new macros
> > > > > > - Added a patch to restrict prescale changes to the first user
> > > > > >
> > > > > > Changes since v3:
> > > > > > - Refactoring: Extracted common functions
> > > > > > - Read prescale register value instead of caching it
> > > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > > - Improved duty calculation / mapping to 0..4096
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Always set default prescale value in probe
> > > > > > - Simplified probe code
> > > > > > - Inlined functions with one callsite
> > > > > >
> > > > > > Changes since v1:
> > > > > > - Fixed a logic error
> > > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > > - Write default prescale reg value if invalid in probe
> > > > > > - Reuse full_off/_on functions throughout driver
> > > > > > - Use cached prescale value whenever possible
> > > > > >
> > > > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > > index 4a55dc18656c..827b57ced3c2 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)))
> > > > >
> > > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > > >
> > > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > > also be prefixed, right?
> > >
> > > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > > IMHO it's good style and even though it yields longer name usually it
> > > yields easier understandable code. (But this seems to be subjective.)
> >
> > I am not sure I want to also rename the existing LED_N_OFF stuff in this
> > patch. Maybe we can discuss unifying the macros (either with or without
> > prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> > match the LED_N_ stuff?
>
> Uwe guessed right, I don't think these need the prefix. There's very
> little potential for name clashes and this driver is already called
> PCA9685, so adding the prefix everywhere can be annoying, especially
> if that then causes you to need line breaks, etc.
>
> For things like function names I usually prefer having a consistent
> prefix because it makes it much easier to decipher stack traces. Not so
> with the defines here.
>
> But I also don't feel strongly, so either way is fine. If you decide to
> go with the prefix, making it consistent throughout the file in a
> separate patch would be preferable, though.
OK. I will keep them without a prefix for now.
>
> >
> > >
> > > > > > + val = 0;
> > > > > > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > >
> > > > > I asked in the last round why you initialize val. You answered "just to
> > > > > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > > > > if
> > > > >
> > > > > ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > > if (!ret)
> > > > > /*
> > >
> > > I intended to write something like
> > >
> > > /* initialize val in case reading LED_N_OFF failed */
> > >
> > > > > val = 0
> > > > >
> > > > > would be better then and also make the intention obvious.
> >
> > Maybe a little bit better.. I can change it.
> >
> > > >
> > > > I am not sure if that's more clear, but if others find it more obvious
> > > > like this, I can change it.
> > > >
> > > > >
> > > > > > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > > > > > +}
> > > > > > +
> > > > > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > > > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > > > > {
> > > > > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > > > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > > > > {
> > > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > > - unsigned int value;
> > > > > >
> > > > > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > > > > -
> > > > > > - return value & LED_FULL;
> > > > > > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > > > > > }
> > > > > >
> > > > > > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > > > > int value)
> > > > > > {
> > > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > > - unsigned int on = value ? LED_FULL : 0;
> > > > > > -
> > > > > > - /* Clear both OFF registers */
> > > > > > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > > > > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > > > > >
> > > > > > - /* Set the full ON bit */
> > > > > > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > > > > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > > > > }
> > > > > >
> > > > > > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > > > > {
> > > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > >
> > > > > > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > > > > > + pca9685_pwm_set_duty(pca, offset, 0);
> > > > > > pm_runtime_put(pca->chip.dev);
> > > > > > pca9685_pwm_clear_inuse(pca, offset);
> > > > > > }
> > > > > > @@ -246,167 +283,52 @@ 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)
> > > > > > {
> > > > > > [...]
> > > > > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > > > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > > > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > > > > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > > > > + return -EINVAL;
> > > > > > }
> > > > > >
> > > > > > [...]
> > > > > > + if (!state->enabled) {
> > > > > > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > > > > return 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);
> > > > > >
> > > > > > [...]
> > > > > > + /* Change the chip-wide output frequency */
> > > > > > + regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > > > >
> > > > > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > > > > + /* Wake the chip up */
> > > > > > + pca9685_set_sleep_mode(pca, false);
> > > > > > + }
> > > > > >
> > > > > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > > > > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> > > > >
> > > > > If you round down here you should probably also round down in the
> > > > > calculation of prescale. Also note that you're losing precision by using
> > > > > state->period.
> > > > >
> > > > > Consider the following request: state->period = 4177921 [ns] +
> > > > > state->duty_cycle = 100000 [ns], then we get
> > > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > > actually configure 96 to get 99840 ns.
> > > > >
> > > > > So in the end I'd like to have the following:
> > > > >
> > > > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > > >
> > > > > (to get the biggest period not bigger than state->period) and
> > > > >
> > > > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > > >
> > > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > > the example above this yields
> > > > >
> > > > > PRESCALE = 24
> > > > > duty = 100
> > > > >
> > > > > which results in
> > > > >
> > > > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > > .duty_cycle = 100000 [ns]
> > > > >
> > > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > > please either stick to the old behaviour or do it right immediately.
> > > >
> > > > I avoided rounding down the prescale value because the datasheet has an
> > > > example where a round-closest is used, see page 25.
> > >
> > > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > > rules for Linux PWM drivers :-)
> > >
> > > > With your suggested round-down, the example with frequency of 200 Hz
> > > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > >
> > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > > the better one depends on the consumer, no matter what rounding
> > > algorithm the data sheet suggests. Also note that the math here contains
> > > surprises you don't expect at first. For example, what PRESCALE value
> > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > > press Space now to pause and let you think first :-)] The data sheet's
> > > formula suggests:
> > >
> > > round(25 MHz / (4096 * 284)) - 1 = 20
> > >
> > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > > (error = 6.567 Hz), so 21 is the better choice.
> > >
> > > Exercise for the reader:
> > > What is the correct formula to really determine the PRESCALE value that
> > > yields the best approximation (i.e. minimizing
> > > abs(real_freq - target_freq)) for a given target_freq?
> > >
> > > These things don't happen when you round down only.
> >
> > Sure, but it might also be counterintuitive that the Linux driver does
> > not use the same formula as the datasheet. And when using 200 Hz, 29 is
> > a little closer than 30.
> > I once measured the actual frequency and the internal oscillator is not
> > very accurate, so even if you think you should get 196.88 Hz, the actual
> > frequency measured with a decent scope is about 206 Hz and varies from
> > chip to chip (~?205-207 Hz).
> >
> > >
> > > > So would you rather have me keep the old duty rounding behaviour?
> > > >
> > > > Meaning: Keep rounding up the duty calculation in apply and use
> > > > round-down in the new .get_state function?
> > >
> > > There are two things I want:
> > >
> > > a) To improve consistency among the PWM drivers (and to keep the math
> > > simple and unsurprising), the pca9685 driver should use round-down
> > > instead of round-nearest (or whatever mix it is currently using).
> > >
> > > b) .get_state should be the inverse to .apply in the sense that
> > > applying the result of .get_state is idempotent.
> > >
> > > I don't care much how you get there, so it's up to you if you do that in
> > > this patch that converts to .apply, or if you keep the math as is and
> > > then adapt the rounding behaviour in a separate change. But changing the
> > > algorithm in this patch and not getting to the "good" one is ugly, so
> > > please don't do that.
> >
> > OK, then I think the best option is to keep the math as it was before
> > and if we want to change the rounding behaviour we do this in a separate
> > patch in the future. Then we can continue the discussion wether changing
> > the prescaler formula to round-down even though the datasheet does it
> > otherwise is the way to go.
>
> I agree, keeping the existing behaviour is preferable. The series
> already changes plenty as-is, and rolling the rounding change into it
> may make it more difficult to revert/fix later on.
>
> We can always consider a follow-up patch to change it.
Sounds good.
I will keep the math as-is and we can improve the rounding and avoid the
loss of precision in a future series.
Thanks,
Clemens
Hi Uwe,
On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > previously experienced issues:
> > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > OFF registers were cleared in disable and the user was required to
> > > > call config to restore the duty cycle settings)
> > > > - If one sets a period resulting in the same prescale register value,
> > > > the sleep and write to the register is now skipped
> > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > OFF cleared if set to high), which could result in both full OFF and
> > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > to the datasheet
> > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > >
> > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > ---
> > > > Changes since v7:
> > > > - Moved check for !state->enabled before prescaler configuration
> > > > - Removed unnecessary cast
> > > > - Use DIV_ROUND_DOWN in .apply
> > > >
> > > > Changes since v6:
> > > > - Order of a comparison switched for improved readability
> > > >
> > > > Changes since v5:
> > > > - Function documentation for set_duty
> > > > - Variable initializations
> > > > - Print warning if all LEDs channel
> > > > - Changed EOPNOTSUPP to EINVAL
> > > > - Improved error messages
> > > > - Register reset corrections moved to this patch
> > > >
> > > > Changes since v4:
> > > > - Patches split up
> > > > - Use a single set_duty function
> > > > - Improve readability / new macros
> > > > - Added a patch to restrict prescale changes to the first user
> > > >
> > > > Changes since v3:
> > > > - Refactoring: Extracted common functions
> > > > - Read prescale register value instead of caching it
> > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > - Improved duty calculation / mapping to 0..4096
> > > >
> > > > Changes since v2:
> > > > - Always set default prescale value in probe
> > > > - Simplified probe code
> > > > - Inlined functions with one callsite
> > > >
> > > > Changes since v1:
> > > > - Fixed a logic error
> > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > - Write default prescale reg value if invalid in probe
> > > > - Reuse full_off/_on functions throughout driver
> > > > - Use cached prescale value whenever possible
> > > >
> > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > index 4a55dc18656c..827b57ced3c2 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)))
> > >
> > > I'd like to see these named PCA9685_REG_ON_H etc.
> >
> > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > also be prefixed, right?
>
> I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> IMHO it's good style and even though it yields longer name usually it
> yields easier understandable code. (But this seems to be subjective.)
I am not sure I want to also rename the existing LED_N_OFF stuff in this
patch. Maybe we can discuss unifying the macros (either with or without
prefix) in a later patch and I keep the REG_ON_ stuff for now without to
match the LED_N_ stuff?
>
> > > > + val = 0;
> > > > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > >
> > > I asked in the last round why you initialize val. You answered "just to
> > > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > > if
> > >
> > > ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > if (!ret)
> > > /*
>
> I intended to write something like
>
> /* initialize val in case reading LED_N_OFF failed */
>
> > > val = 0
> > >
> > > would be better then and also make the intention obvious.
Maybe a little bit better.. I can change it.
> >
> > I am not sure if that's more clear, but if others find it more obvious
> > like this, I can change it.
> >
> > >
> > > > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > > > +}
> > > > +
> > > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > > {
> > > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > > {
> > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > - unsigned int value;
> > > >
> > > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > > -
> > > > - return value & LED_FULL;
> > > > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > > > }
> > > >
> > > > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > > int value)
> > > > {
> > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > - unsigned int on = value ? LED_FULL : 0;
> > > > -
> > > > - /* Clear both OFF registers */
> > > > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > > >
> > > > - /* Set the full ON bit */
> > > > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > > }
> > > >
> > > > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > > {
> > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > >
> > > > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > > > + pca9685_pwm_set_duty(pca, offset, 0);
> > > > pm_runtime_put(pca->chip.dev);
> > > > pca9685_pwm_clear_inuse(pca, offset);
> > > > }
> > > > @@ -246,167 +283,52 @@ 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)
> > > > {
> > > > [...]
> > > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > > + return -EINVAL;
> > > > }
> > > >
> > > > [...]
> > > > + if (!state->enabled) {
> > > > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > > return 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);
> > > >
> > > > [...]
> > > > + /* Change the chip-wide output frequency */
> > > > + regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > >
> > > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > > + /* Wake the chip up */
> > > > + pca9685_set_sleep_mode(pca, false);
> > > > + }
> > > >
> > > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> > >
> > > If you round down here you should probably also round down in the
> > > calculation of prescale. Also note that you're losing precision by using
> > > state->period.
> > >
> > > Consider the following request: state->period = 4177921 [ns] +
> > > state->duty_cycle = 100000 [ns], then we get
> > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > actually configure 96 to get 99840 ns.
> > >
> > > So in the end I'd like to have the following:
> > >
> > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > >
> > > (to get the biggest period not bigger than state->period) and
> > >
> > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > >
> > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > the example above this yields
> > >
> > > PRESCALE = 24
> > > duty = 100
> > >
> > > which results in
> > >
> > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > .duty_cycle = 100000 [ns]
> > >
> > > Now you have a mixture of old and new with no consistent behaviour. So
> > > please either stick to the old behaviour or do it right immediately.
> >
> > I avoided rounding down the prescale value because the datasheet has an
> > example where a round-closest is used, see page 25.
>
> The hardware guy who wrote this data sheet wasn't aware of the rounding
> rules for Linux PWM drivers :-)
>
> > With your suggested round-down, the example with frequency of 200 Hz
> > would no longer result in 30 but 29 and that contradicts the datasheet.
>
> Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> 29 or 30, you don't get 200 Hz. And which of the two possible values is
> the better one depends on the consumer, no matter what rounding
> algorithm the data sheet suggests. Also note that the math here contains
> surprises you don't expect at first. For example, what PRESCALE value
> would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> press Space now to pause and let you think first :-)] The data sheet's
> formula suggests:
>
> round(25 MHz / (4096 * 284)) - 1 = 20
>
> The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> (error = 6.567 Hz), so 21 is the better choice.
>
> Exercise for the reader:
> What is the correct formula to really determine the PRESCALE value that
> yields the best approximation (i.e. minimizing
> abs(real_freq - target_freq)) for a given target_freq?
>
> These things don't happen when you round down only.
Sure, but it might also be counterintuitive that the Linux driver does
not use the same formula as the datasheet. And when using 200 Hz, 29 is
a little closer than 30.
I once measured the actual frequency and the internal oscillator is not
very accurate, so even if you think you should get 196.88 Hz, the actual
frequency measured with a decent scope is about 206 Hz and varies from
chip to chip (~?205-207 Hz).
>
> > So would you rather have me keep the old duty rounding behaviour?
> >
> > Meaning: Keep rounding up the duty calculation in apply and use
> > round-down in the new .get_state function?
>
> There are two things I want:
>
> a) To improve consistency among the PWM drivers (and to keep the math
> simple and unsurprising), the pca9685 driver should use round-down
> instead of round-nearest (or whatever mix it is currently using).
>
> b) .get_state should be the inverse to .apply in the sense that
> applying the result of .get_state is idempotent.
>
> I don't care much how you get there, so it's up to you if you do that in
> this patch that converts to .apply, or if you keep the math as is and
> then adapt the rounding behaviour in a separate change. But changing the
> algorithm in this patch and not getting to the "good" one is ugly, so
> please don't do that.
OK, then I think the best option is to keep the math as it was before
and if we want to change the rounding behaviour we do this in a separate
patch in the future. Then we can continue the discussion wether changing
the prescaler formula to round-down even though the datasheet does it
otherwise is the way to go.
Thanks,
Clemens
On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> Hi Uwe,
>
> On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-König wrote:
> > Hello Clemens,
> >
> > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > previously experienced issues:
> > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > > OFF registers were cleared in disable and the user was required to
> > > > > call config to restore the duty cycle settings)
> > > > > - If one sets a period resulting in the same prescale register value,
> > > > > the sleep and write to the register is now skipped
> > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > > OFF cleared if set to high), which could result in both full OFF and
> > > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > > to the datasheet
> > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > >
> > > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > > ---
> > > > > Changes since v7:
> > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > - Removed unnecessary cast
> > > > > - Use DIV_ROUND_DOWN in .apply
> > > > >
> > > > > Changes since v6:
> > > > > - Order of a comparison switched for improved readability
> > > > >
> > > > > Changes since v5:
> > > > > - Function documentation for set_duty
> > > > > - Variable initializations
> > > > > - Print warning if all LEDs channel
> > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > - Improved error messages
> > > > > - Register reset corrections moved to this patch
> > > > >
> > > > > Changes since v4:
> > > > > - Patches split up
> > > > > - Use a single set_duty function
> > > > > - Improve readability / new macros
> > > > > - Added a patch to restrict prescale changes to the first user
> > > > >
> > > > > Changes since v3:
> > > > > - Refactoring: Extracted common functions
> > > > > - Read prescale register value instead of caching it
> > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > - Improved duty calculation / mapping to 0..4096
> > > > >
> > > > > Changes since v2:
> > > > > - Always set default prescale value in probe
> > > > > - Simplified probe code
> > > > > - Inlined functions with one callsite
> > > > >
> > > > > Changes since v1:
> > > > > - Fixed a logic error
> > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > - Write default prescale reg value if invalid in probe
> > > > > - Reuse full_off/_on functions throughout driver
> > > > > - Use cached prescale value whenever possible
> > > > >
> > > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > index 4a55dc18656c..827b57ced3c2 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)))
> > > >
> > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > >
> > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > also be prefixed, right?
> >
> > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > IMHO it's good style and even though it yields longer name usually it
> > yields easier understandable code. (But this seems to be subjective.)
>
> I am not sure I want to also rename the existing LED_N_OFF stuff in this
> patch. Maybe we can discuss unifying the macros (either with or without
> prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> match the LED_N_ stuff?
Uwe guessed right, I don't think these need the prefix. There's very
little potential for name clashes and this driver is already called
PCA9685, so adding the prefix everywhere can be annoying, especially
if that then causes you to need line breaks, etc.
For things like function names I usually prefer having a consistent
prefix because it makes it much easier to decipher stack traces. Not so
with the defines here.
But I also don't feel strongly, so either way is fine. If you decide to
go with the prefix, making it consistent throughout the file in a
separate patch would be preferable, though.
>
> >
> > > > > + val = 0;
> > > > > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > >
> > > > I asked in the last round why you initialize val. You answered "just to
> > > > have it set to 0 in case regmap_read fails / val was not set." I wonder
> > > > if
> > > >
> > > > ret = regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > > > if (!ret)
> > > > /*
> >
> > I intended to write something like
> >
> > /* initialize val in case reading LED_N_OFF failed */
> >
> > > > val = 0
> > > >
> > > > would be better then and also make the intention obvious.
>
> Maybe a little bit better.. I can change it.
>
> > >
> > > I am not sure if that's more clear, but if others find it more obvious
> > > like this, I can change it.
> > >
> > > >
> > > > > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > > > > +}
> > > > > +
> > > > > #if IS_ENABLED(CONFIG_GPIOLIB)
> > > > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > > > > {
> > > > > @@ -138,34 +186,23 @@ static int pca9685_pwm_gpio_request(struct gpio_chip *gpio, unsigned int offset)
> > > > > static int pca9685_pwm_gpio_get(struct gpio_chip *gpio, unsigned int offset)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > - unsigned int value;
> > > > >
> > > > > - regmap_read(pca->regmap, LED_N_ON_H(pwm->hwpwm), &value);
> > > > > -
> > > > > - return value & LED_FULL;
> > > > > + return pca9685_pwm_get_duty(pca, offset) != 0;
> > > > > }
> > > > >
> > > > > static void pca9685_pwm_gpio_set(struct gpio_chip *gpio, unsigned int offset,
> > > > > int value)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > > - struct pwm_device *pwm = &pca->chip.pwms[offset];
> > > > > - unsigned int on = value ? LED_FULL : 0;
> > > > > -
> > > > > - /* Clear both OFF registers */
> > > > > - regmap_write(pca->regmap, LED_N_OFF_L(pwm->hwpwm), 0);
> > > > > - regmap_write(pca->regmap, LED_N_OFF_H(pwm->hwpwm), 0);
> > > > >
> > > > > - /* Set the full ON bit */
> > > > > - regmap_write(pca->regmap, LED_N_ON_H(pwm->hwpwm), on);
> > > > > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > > > > }
> > > > >
> > > > > static void pca9685_pwm_gpio_free(struct gpio_chip *gpio, unsigned int offset)
> > > > > {
> > > > > struct pca9685 *pca = gpiochip_get_data(gpio);
> > > > >
> > > > > - pca9685_pwm_gpio_set(gpio, offset, 0);
> > > > > + pca9685_pwm_set_duty(pca, offset, 0);
> > > > > pm_runtime_put(pca->chip.dev);
> > > > > pca9685_pwm_clear_inuse(pca, offset);
> > > > > }
> > > > > @@ -246,167 +283,52 @@ 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)
> > > > > {
> > > > > [...]
> > > > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period,
> > > > > + PCA9685_COUNTER_RANGE * 1000) - 1;
> > > > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) {
> > > > > + dev_err(chip->dev, "pwm not changed: period out of bounds!\n");
> > > > > + return -EINVAL;
> > > > > }
> > > > >
> > > > > [...]
> > > > > + if (!state->enabled) {
> > > > > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > > > > return 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);
> > > > >
> > > > > [...]
> > > > > + /* Change the chip-wide output frequency */
> > > > > + regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
> > > > >
> > > > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > > > > + /* Wake the chip up */
> > > > > + pca9685_set_sleep_mode(pca, false);
> > > > > + }
> > > > >
> > > > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > > > > + duty = DIV_ROUND_DOWN_ULL(duty, state->period);
> > > >
> > > > If you round down here you should probably also round down in the
> > > > calculation of prescale. Also note that you're losing precision by using
> > > > state->period.
> > > >
> > > > Consider the following request: state->period = 4177921 [ns] +
> > > > state->duty_cycle = 100000 [ns], then we get
> > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > actually configure 96 to get 99840 ns.
> > > >
> > > > So in the end I'd like to have the following:
> > > >
> > > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > >
> > > > (to get the biggest period not bigger than state->period) and
> > > >
> > > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > >
> > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > the example above this yields
> > > >
> > > > PRESCALE = 24
> > > > duty = 100
> > > >
> > > > which results in
> > > >
> > > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > .duty_cycle = 100000 [ns]
> > > >
> > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > please either stick to the old behaviour or do it right immediately.
> > >
> > > I avoided rounding down the prescale value because the datasheet has an
> > > example where a round-closest is used, see page 25.
> >
> > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > rules for Linux PWM drivers :-)
> >
> > > With your suggested round-down, the example with frequency of 200 Hz
> > > would no longer result in 30 but 29 and that contradicts the datasheet.
> >
> > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > the better one depends on the consumer, no matter what rounding
> > algorithm the data sheet suggests. Also note that the math here contains
> > surprises you don't expect at first. For example, what PRESCALE value
> > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > press Space now to pause and let you think first :-)] The data sheet's
> > formula suggests:
> >
> > round(25 MHz / (4096 * 284)) - 1 = 20
> >
> > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > (error = 6.567 Hz), so 21 is the better choice.
> >
> > Exercise for the reader:
> > What is the correct formula to really determine the PRESCALE value that
> > yields the best approximation (i.e. minimizing
> > abs(real_freq - target_freq)) for a given target_freq?
> >
> > These things don't happen when you round down only.
>
> Sure, but it might also be counterintuitive that the Linux driver does
> not use the same formula as the datasheet. And when using 200 Hz, 29 is
> a little closer than 30.
> I once measured the actual frequency and the internal oscillator is not
> very accurate, so even if you think you should get 196.88 Hz, the actual
> frequency measured with a decent scope is about 206 Hz and varies from
> chip to chip (~ 205-207 Hz).
>
> >
> > > So would you rather have me keep the old duty rounding behaviour?
> > >
> > > Meaning: Keep rounding up the duty calculation in apply and use
> > > round-down in the new .get_state function?
> >
> > There are two things I want:
> >
> > a) To improve consistency among the PWM drivers (and to keep the math
> > simple and unsurprising), the pca9685 driver should use round-down
> > instead of round-nearest (or whatever mix it is currently using).
> >
> > b) .get_state should be the inverse to .apply in the sense that
> > applying the result of .get_state is idempotent.
> >
> > I don't care much how you get there, so it's up to you if you do that in
> > this patch that converts to .apply, or if you keep the math as is and
> > then adapt the rounding behaviour in a separate change. But changing the
> > algorithm in this patch and not getting to the "good" one is ugly, so
> > please don't do that.
>
> OK, then I think the best option is to keep the math as it was before
> and if we want to change the rounding behaviour we do this in a separate
> patch in the future. Then we can continue the discussion wether changing
> the prescaler formula to round-down even though the datasheet does it
> otherwise is the way to go.
I agree, keeping the existing behaviour is preferable. The series
already changes plenty as-is, and rolling the rounding change into it
may make it more difficult to revert/fix later on.
We can always consider a follow-up patch to change it.
Thierry
Hello Clemens,
On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > previously experienced issues:
> > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > > OFF registers were cleared in disable and the user was required to
> > > > > call config to restore the duty cycle settings)
> > > > > - If one sets a period resulting in the same prescale register value,
> > > > > the sleep and write to the register is now skipped
> > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > > OFF cleared if set to high), which could result in both full OFF and
> > > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > > to the datasheet
> > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > >
> > > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > > ---
> > > > > Changes since v7:
> > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > - Removed unnecessary cast
> > > > > - Use DIV_ROUND_DOWN in .apply
> > > > >
> > > > > Changes since v6:
> > > > > - Order of a comparison switched for improved readability
> > > > >
> > > > > Changes since v5:
> > > > > - Function documentation for set_duty
> > > > > - Variable initializations
> > > > > - Print warning if all LEDs channel
> > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > - Improved error messages
> > > > > - Register reset corrections moved to this patch
> > > > >
> > > > > Changes since v4:
> > > > > - Patches split up
> > > > > - Use a single set_duty function
> > > > > - Improve readability / new macros
> > > > > - Added a patch to restrict prescale changes to the first user
> > > > >
> > > > > Changes since v3:
> > > > > - Refactoring: Extracted common functions
> > > > > - Read prescale register value instead of caching it
> > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > - Improved duty calculation / mapping to 0..4096
> > > > >
> > > > > Changes since v2:
> > > > > - Always set default prescale value in probe
> > > > > - Simplified probe code
> > > > > - Inlined functions with one callsite
> > > > >
> > > > > Changes since v1:
> > > > > - Fixed a logic error
> > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > - Write default prescale reg value if invalid in probe
> > > > > - Reuse full_off/_on functions throughout driver
> > > > > - Use cached prescale value whenever possible
> > > > >
> > > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > index 4a55dc18656c..827b57ced3c2 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)))
> > > >
> > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > >
> > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > also be prefixed, right?
> >
> > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > IMHO it's good style and even though it yields longer name usually it
> > yields easier understandable code. (But this seems to be subjective.)
>
> I am not sure I want to also rename the existing LED_N_OFF stuff in this
> patch. Maybe we can discuss unifying the macros (either with or without
> prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> match the LED_N_ stuff?
While consistency is fine I agree that this patch is already big and
letting it do the things similar to other stuff in this driver is ok.
> > > > Consider the following request: state->period = 4177921 [ns] +
> > > > state->duty_cycle = 100000 [ns], then we get
> > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > actually configure 96 to get 99840 ns.
> > > >
> > > > So in the end I'd like to have the following:
> > > >
> > > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > >
> > > > (to get the biggest period not bigger than state->period) and
> > > >
> > > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > >
> > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > the example above this yields
> > > >
> > > > PRESCALE = 24
> > > > duty = 100
> > > >
> > > > which results in
> > > >
> > > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > .duty_cycle = 100000 [ns]
> > > >
> > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > please either stick to the old behaviour or do it right immediately.
> > >
> > > I avoided rounding down the prescale value because the datasheet has an
> > > example where a round-closest is used, see page 25.
> >
> > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > rules for Linux PWM drivers :-)
> >
> > > With your suggested round-down, the example with frequency of 200 Hz
> > > would no longer result in 30 but 29 and that contradicts the datasheet.
> >
> > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > the better one depends on the consumer, no matter what rounding
> > algorithm the data sheet suggests. Also note that the math here contains
> > surprises you don't expect at first. For example, what PRESCALE value
> > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > press Space now to pause and let you think first :-)] The data sheet's
> > formula suggests:
> >
> > round(25 MHz / (4096 * 284)) - 1 = 20
> >
> > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > (error = 6.567 Hz), so 21 is the better choice.
> >
> > Exercise for the reader:
> > What is the correct formula to really determine the PRESCALE value that
> > yields the best approximation (i.e. minimizing
> > abs(real_freq - target_freq)) for a given target_freq?
I wonder if you tried this.
> > These things don't happen when you round down only.
>
> Sure, but it might also be counterintuitive that the Linux driver does
> not use the same formula as the datasheet. And when using 200 Hz, 29 is
> a little closer than 30.
First let me state that I consider keeping the math as is in this patch
a good idea. So to argue already for the future:
I value consistency among the various pwm lowlevel drivers higher than
what an individual hardware engineer happened to write in a data sheet.
That engineer was successful in describing the functionality of the chip
and that's where her/his job ends. How a driver should behave is to be
decided by us.
> I once measured the actual frequency and the internal oscillator is not
> very accurate, so even if you think you should get 196.88 Hz, the actual
> frequency measured with a decent scope is about 206 Hz and varies from
> chip to chip (~?205-207 Hz).
Huh. Did you do further measurements that suggest that the oscillator
doesn't run at 25 MHz but maybe at 26 MHz (which would yield 204.7552
Hz)? (Assume this is true, what do you think: Should be use the formula
that matches reality, or should we stick to the formula defined in the
data sheet?)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > > On Mon, Apr 12, 2021 at 06:18:08PM +0200, Uwe Kleine-K?nig wrote:
> > > > > On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > > > > > The switch to the atomic API goes hand in hand with a few fixes to
> > > > > > previously experienced issues:
> > > > > > - The duty cycle is no longer lost after disable/enable (previously the
> > > > > > OFF registers were cleared in disable and the user was required to
> > > > > > call config to restore the duty cycle settings)
> > > > > > - If one sets a period resulting in the same prescale register value,
> > > > > > the sleep and write to the register is now skipped
> > > > > > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > > > > > OFF cleared if set to high), which could result in both full OFF and
> > > > > > full ON not being set and on=0, off=0, which is not allowed according
> > > > > > to the datasheet
> > > > > > - The OFF registers were reset to 0 in probe, which could lead to the
> > > > > > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
> > > > > >
> > > > > > Signed-off-by: Clemens Gruber <[email protected]>
> > > > > > ---
> > > > > > Changes since v7:
> > > > > > - Moved check for !state->enabled before prescaler configuration
> > > > > > - Removed unnecessary cast
> > > > > > - Use DIV_ROUND_DOWN in .apply
> > > > > >
> > > > > > Changes since v6:
> > > > > > - Order of a comparison switched for improved readability
> > > > > >
> > > > > > Changes since v5:
> > > > > > - Function documentation for set_duty
> > > > > > - Variable initializations
> > > > > > - Print warning if all LEDs channel
> > > > > > - Changed EOPNOTSUPP to EINVAL
> > > > > > - Improved error messages
> > > > > > - Register reset corrections moved to this patch
> > > > > >
> > > > > > Changes since v4:
> > > > > > - Patches split up
> > > > > > - Use a single set_duty function
> > > > > > - Improve readability / new macros
> > > > > > - Added a patch to restrict prescale changes to the first user
> > > > > >
> > > > > > Changes since v3:
> > > > > > - Refactoring: Extracted common functions
> > > > > > - Read prescale register value instead of caching it
> > > > > > - Return all zeros and disabled for "all LEDs" channel state
> > > > > > - Improved duty calculation / mapping to 0..4096
> > > > > >
> > > > > > Changes since v2:
> > > > > > - Always set default prescale value in probe
> > > > > > - Simplified probe code
> > > > > > - Inlined functions with one callsite
> > > > > >
> > > > > > Changes since v1:
> > > > > > - Fixed a logic error
> > > > > > - Impoved PM runtime handling and fixed !CONFIG_PM
> > > > > > - Write default prescale reg value if invalid in probe
> > > > > > - Reuse full_off/_on functions throughout driver
> > > > > > - Use cached prescale value whenever possible
> > > > > >
> > > > > > drivers/pwm/pwm-pca9685.c | 259 +++++++++++++-------------------------
> > > > > > 1 file changed, 89 insertions(+), 170 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > > > > > index 4a55dc18656c..827b57ced3c2 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)))
> > > > >
> > > > > I'd like to see these named PCA9685_REG_ON_H etc.
> > > >
> > > > I did not use the prefix because the existing LED_N_ON/OFF_H/L also do
> > > > not have a prefix. If the prefix is mandatory, I think LED_N_.. should
> > > > also be prefixed, right?
> > >
> > > I'd like to seem the prefixed (and assume that Thierry doesn't agree).
> > > IMHO it's good style and even though it yields longer name usually it
> > > yields easier understandable code. (But this seems to be subjective.)
> >
> > I am not sure I want to also rename the existing LED_N_OFF stuff in this
> > patch. Maybe we can discuss unifying the macros (either with or without
> > prefix) in a later patch and I keep the REG_ON_ stuff for now without to
> > match the LED_N_ stuff?
>
> While consistency is fine I agree that this patch is already big and
> letting it do the things similar to other stuff in this driver is ok.
>
> > > > > Consider the following request: state->period = 4177921 [ns] +
> > > > > state->duty_cycle = 100000 [ns], then we get
> > > > > PRESCALE = round(25 * state->period / 4096000) - 1 = 25 and so an actual
> > > > > period of 4096000 / 25 * (25 + 1) = 4259840 [ns]. If you now calculate
> > > > > the duty using 4096 * 100000 / 4177920 = 98, this corresponds to an
> > > > > actual duty cycle of 98 * 4259840 / 4096 = 101920 ns while you should
> > > > > actually configure 96 to get 99840 ns.
> > > > >
> > > > > So in the end I'd like to have the following:
> > > > >
> > > > > PRESCALE = round-down(25 * state->period / 4096000) - 1
> > > > >
> > > > > (to get the biggest period not bigger than state->period) and
> > > > >
> > > > > duty = round-down(state->duty_cycle * 25 / ((PRESCALE + 1) * 1000))
> > > > >
> > > > > (to get the biggest duty cycle not bigger than state->duty_cycle). With
> > > > > the example above this yields
> > > > >
> > > > > PRESCALE = 24
> > > > > duty = 100
> > > > >
> > > > > which results in
> > > > >
> > > > > .period = 4096000 / 25 * 25 = 4096000 [ns]
> > > > > .duty_cycle = 100000 [ns]
> > > > >
> > > > > Now you have a mixture of old and new with no consistent behaviour. So
> > > > > please either stick to the old behaviour or do it right immediately.
> > > >
> > > > I avoided rounding down the prescale value because the datasheet has an
> > > > example where a round-closest is used, see page 25.
> > >
> > > The hardware guy who wrote this data sheet wasn't aware of the rounding
> > > rules for Linux PWM drivers :-)
> > >
> > > > With your suggested round-down, the example with frequency of 200 Hz
> > > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > >
> > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > > the better one depends on the consumer, no matter what rounding
> > > algorithm the data sheet suggests. Also note that the math here contains
> > > surprises you don't expect at first. For example, what PRESCALE value
> > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > > press Space now to pause and let you think first :-)] The data sheet's
> > > formula suggests:
> > >
> > > round(25 MHz / (4096 * 284)) - 1 = 20
> > >
> > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > > (error = 6.567 Hz), so 21 is the better choice.
> > >
> > > Exercise for the reader:
> > > What is the correct formula to really determine the PRESCALE value that
> > > yields the best approximation (i.e. minimizing
> > > abs(real_freq - target_freq)) for a given target_freq?
>
> I wonder if you tried this.
We could calculate both round-up and round-down and decide which one is
closer to "real freq" (even though that is not the actual frequency but
just our backwards-calculated frequency).
But I can't give you a formula with minimized abs(real_freq-target_freq)
Is it a different round point than 0.5 and maybe relative to f ?
Please enlighten us :-)
>
> > > These things don't happen when you round down only.
> >
> > Sure, but it might also be counterintuitive that the Linux driver does
> > not use the same formula as the datasheet. And when using 200 Hz, 29 is
> > a little closer than 30.
>
> First let me state that I consider keeping the math as is in this patch
> a good idea. So to argue already for the future:
>
> I value consistency among the various pwm lowlevel drivers higher than
> what an individual hardware engineer happened to write in a data sheet.
> That engineer was successful in describing the functionality of the chip
> and that's where her/his job ends. How a driver should behave is to be
> decided by us.
>
> > I once measured the actual frequency and the internal oscillator is not
> > very accurate, so even if you think you should get 196.88 Hz, the actual
> > frequency measured with a decent scope is about 206 Hz and varies from
> > chip to chip (~?205-207 Hz).
>
> Huh. Did you do further measurements that suggest that the oscillator
> doesn't run at 25 MHz but maybe at 26 MHz (which would yield 204.7552
> Hz)? (Assume this is true, what do you think: Should be use the formula
> that matches reality, or should we stick to the formula defined in the
> data sheet?)
No, I just tested this for 3-4 chips and for them I measured these
errors, but that's just a small sample.
Maybe the next few measurements would indicate that it runs at 24 MHz
and it evens out at 25 MHz for all produced chips, but we just have
enough information imho.
Clemens
On Wed, Apr 14, 2021 at 02:09:14PM +0200, Clemens Gruber wrote:
> Hi Uwe,
>
> On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-K?nig wrote:
> > Hello Clemens,
> >
> > On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > > > With your suggested round-down, the example with frequency of 200 Hz
> > > > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > > >
> > > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > > > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > > > the better one depends on the consumer, no matter what rounding
> > > > algorithm the data sheet suggests. Also note that the math here contains
> > > > surprises you don't expect at first. For example, what PRESCALE value
> > > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > > > press Space now to pause and let you think first :-)] The data sheet's
> > > > formula suggests:
> > > >
> > > > round(25 MHz / (4096 * 284)) - 1 = 20
> > > >
> > > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > > > (error = 6.567 Hz), so 21 is the better choice.
> > > >
> > > > Exercise for the reader:
> > > > What is the correct formula to really determine the PRESCALE value that
> > > > yields the best approximation (i.e. minimizing
> > > > abs(real_freq - target_freq)) for a given target_freq?
> >
> > I wonder if you tried this.
>
> We could calculate both round-up and round-down and decide which one is
> closer to "real freq" (even though that is not the actual frequency but
> just our backwards-calculated frequency).
Yeah, the backwards-calculated frequency is the best assumption we
have.
> But I can't give you a formula with minimized abs(real_freq-target_freq)
> Is it a different round point than 0.5 and maybe relative to f ?
>
> Please enlighten us :-)
Sorry, I cannot. I spend ~20 min today after lunch with pencil and
paper, but without success. I was aware that it isn't trivial and this
is the main reason I established round-down as default for new drivers
instead of round-nearest.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, Apr 14, 2021 at 09:21:31PM +0200, Uwe Kleine-K?nig wrote:
> On Wed, Apr 14, 2021 at 02:09:14PM +0200, Clemens Gruber wrote:
> > Hi Uwe,
> >
> > On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-K?nig wrote:
> > > Hello Clemens,
> > >
> > > On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> > > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > > > > With your suggested round-down, the example with frequency of 200 Hz
> > > > > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > > > >
> > > > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > > > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > > > > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > > > > the better one depends on the consumer, no matter what rounding
> > > > > algorithm the data sheet suggests. Also note that the math here contains
> > > > > surprises you don't expect at first. For example, what PRESCALE value
> > > > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > > > > press Space now to pause and let you think first :-)] The data sheet's
> > > > > formula suggests:
> > > > >
> > > > > round(25 MHz / (4096 * 284)) - 1 = 20
> > > > >
> > > > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > > > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > > > > (error = 6.567 Hz), so 21 is the better choice.
> > > > >
> > > > > Exercise for the reader:
> > > > > What is the correct formula to really determine the PRESCALE value that
> > > > > yields the best approximation (i.e. minimizing
> > > > > abs(real_freq - target_freq)) for a given target_freq?
> > >
> > > I wonder if you tried this.
> >
> > We could calculate both round-up and round-down and decide which one is
> > closer to "real freq" (even though that is not the actual frequency but
> > just our backwards-calculated frequency).
>
> Yeah, the backwards-calculated frequency is the best assumption we
> have.
>
> > But I can't give you a formula with minimized abs(real_freq-target_freq)
> > Is it a different round point than 0.5 and maybe relative to f ?
> >
> > Please enlighten us :-)
>
> Sorry, I cannot. I spend ~20 min today after lunch with pencil and
> paper, but without success. I was aware that it isn't trivial and this
> is the main reason I established round-down as default for new drivers
> instead of round-nearest.
Oh, I thought you already solved it. I tried too for a while but was
unsuccessful. Not trivial indeed!
But regarding you establishing round-down: Wouldn't it be even better if
the driver did what I suggested above, namely calculate backwards from
both the rounded-up as well as the rounded-down prescale value and then
write the one with the smallest abs(f_target - f_real) to the register?
Clemens
On Wed, Apr 14, 2021 at 09:45:32PM +0200, Clemens Gruber wrote:
> On Wed, Apr 14, 2021 at 09:21:31PM +0200, Uwe Kleine-K?nig wrote:
> > On Wed, Apr 14, 2021 at 02:09:14PM +0200, Clemens Gruber wrote:
> > > Hi Uwe,
> > >
> > > On Tue, Apr 13, 2021 at 09:38:18PM +0200, Uwe Kleine-K?nig wrote:
> > > > Hello Clemens,
> > > >
> > > > On Tue, Apr 13, 2021 at 02:11:38PM +0200, Clemens Gruber wrote:
> > > > > On Mon, Apr 12, 2021 at 10:10:19PM +0200, Uwe Kleine-K?nig wrote:
> > > > > > On Mon, Apr 12, 2021 at 06:39:28PM +0200, Clemens Gruber wrote:
> > > > > > > With your suggested round-down, the example with frequency of 200 Hz
> > > > > > > would no longer result in 30 but 29 and that contradicts the datasheet.
> > > > > >
> > > > > > Well, with PRESCALE = 30 we get a frequency of 196.88 Hz and with
> > > > > > PRESCALE = 29 we get a frequency of 203.45 Hz. So no matter if you pick
> > > > > > 29 or 30, you don't get 200 Hz. And which of the two possible values is
> > > > > > the better one depends on the consumer, no matter what rounding
> > > > > > algorithm the data sheet suggests. Also note that the math here contains
> > > > > > surprises you don't expect at first. For example, what PRESCALE value
> > > > > > would you pick to get 284 Hz? [If my mail was a video, I'd suggest to
> > > > > > press Space now to pause and let you think first :-)] The data sheet's
> > > > > > formula suggests:
> > > > > >
> > > > > > round(25 MHz / (4096 * 284)) - 1 = 20
> > > > > >
> > > > > > The resulting frequency when picking PRESCALE = 20 is 290.644 Hz (so an
> > > > > > error of 6.644 Hz). If instead you pick PRESCALE = 21 you get 277.433 Hz
> > > > > > (error = 6.567 Hz), so 21 is the better choice.
> > > > > >
> > > > > > Exercise for the reader:
> > > > > > What is the correct formula to really determine the PRESCALE value that
> > > > > > yields the best approximation (i.e. minimizing
> > > > > > abs(real_freq - target_freq)) for a given target_freq?
> > > >
> > > > I wonder if you tried this.
> > >
> > > We could calculate both round-up and round-down and decide which one is
> > > closer to "real freq" (even though that is not the actual frequency but
> > > just our backwards-calculated frequency).
> >
> > Yeah, the backwards-calculated frequency is the best assumption we
> > have.
> >
> > > But I can't give you a formula with minimized abs(real_freq-target_freq)
> > > Is it a different round point than 0.5 and maybe relative to f ?
> > >
> > > Please enlighten us :-)
> >
> > Sorry, I cannot. I spend ~20 min today after lunch with pencil and
> > paper, but without success. I was aware that it isn't trivial and this
> > is the main reason I established round-down as default for new drivers
> > instead of round-nearest.
>
> Oh, I thought you already solved it. I tried too for a while but was
> unsuccessful. Not trivial indeed!
>
> But regarding you establishing round-down: Wouldn't it be even better if
> the driver did what I suggested above, namely calculate backwards from
> both the rounded-up as well as the rounded-down prescale value and then
> write the one with the smallest abs(f_target - f_real) to the register?
No, I don't think so for several reasons. First, just rounding down is
easier (and keeping lowlevel drivers rules and implementation easy is
IMHO a good goal). The second reason is that round-nearest is a bit
ambigous because round to the nearest frequency is slightly different to
round to the nearest period length. So to actually implement (or use)
it correctly, people have to grasp that difference. Compared to that
rounding down the period length corresponds 1:1 to rounding up
frequency. That's easy.
For the third reason I have to backup a bit: I intend to introduce a
function pwm_round_rate that predicts what pwm_apply_rate will actually
implement. Of course it must have the same rounding rules. This allows
to implement efficient search for consumers that e.g. prefer
round-nearest time, or round-nearest frequency. I'm convinced that
searching the optimal request to make is easier if round_rate uses
round-down and not round-nearest.
All three reasons boil down to "the math for round-down is just simpler
(for implementers and for users) than with round-nearest".
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> Add the flag and corresponding documentation for PWM_USAGE_POWER.
>
> Cc: Rob Herring <[email protected]>
> Signed-off-by: Clemens Gruber <[email protected]>
> ---
> Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> include/dt-bindings/pwm/pwm.h | 1 +
> 2 files changed, 4 insertions(+)
Rob, what are your thoughts on this? I've been thinking about this some
more and I'm having second thoughts about putting this into device tree
because it doesn't actually describe a property of the PWM hardware but
rather a use-case specific hint. It's a bit of a gray area because this
is just part of the PWM specifier which already has use-case specific
"configuration", such as the period and the polarity.
Perhaps a better place for this is within the PWM API? We could add the
same information into struct pwm_state and then consumers that don't
care about specifics of the signal (such as pwm-backlight) can set that
flag when they request a state to be applied.
Thierry
On Fri, Apr 16, 2021 at 03:55:11PM +0200, Thierry Reding wrote:
> On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > Add the flag and corresponding documentation for PWM_USAGE_POWER.
> >
> > Cc: Rob Herring <[email protected]>
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > include/dt-bindings/pwm/pwm.h | 1 +
> > 2 files changed, 4 insertions(+)
>
> Rob, what are your thoughts on this? I've been thinking about this some
> more and I'm having second thoughts about putting this into device tree
> because it doesn't actually describe a property of the PWM hardware but
> rather a use-case specific hint. It's a bit of a gray area because this
> is just part of the PWM specifier which already has use-case specific
> "configuration", such as the period and the polarity.
>
> Perhaps a better place for this is within the PWM API? We could add the
> same information into struct pwm_state and then consumers that don't
> care about specifics of the signal (such as pwm-backlight) can set that
> flag when they request a state to be applied.
I just want to note that in my opinion, this is not a flag that is
changed often, so is it really a good idea to require setting this
wherever PWM state is applied? Also, this can't be read-out in
.get_state.
Thierry: If this discussion carries on and a v10 is required: Could you
maybe merge the uncontroversial patches 1 to 3 of v9 separately and
maybe get those in 5.12 ? Patches 4 to 8 can probably wait for 5.13 and
have some time in linux-next.
Thanks,
Clemens
On Fri, Apr 16, 2021 at 8:54 AM Thierry Reding <[email protected]> wrote:
>
> On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > Add the flag and corresponding documentation for PWM_USAGE_POWER.
> >
> > Cc: Rob Herring <[email protected]>
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > include/dt-bindings/pwm/pwm.h | 1 +
> > 2 files changed, 4 insertions(+)
>
> Rob, what are your thoughts on this? I've been thinking about this some
> more and I'm having second thoughts about putting this into device tree
> because it doesn't actually describe a property of the PWM hardware but
> rather a use-case specific hint. It's a bit of a gray area because this
> is just part of the PWM specifier which already has use-case specific
> "configuration", such as the period and the polarity.
I'm pretty neutral. My main hesitation from what I've followed is
'power' seems a bit indirect. A PWM signal doesn't have a 'power' any
more than a GPIO signal does.
> Perhaps a better place for this is within the PWM API? We could add the
> same information into struct pwm_state and then consumers that don't
> care about specifics of the signal (such as pwm-backlight) can set that
> flag when they request a state to be applied.
Yeah, seems like this is fairly well tied to the class of consumer.
Rob
On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> The switch to the atomic API goes hand in hand with a few fixes to
> previously experienced issues:
> - The duty cycle is no longer lost after disable/enable (previously the
> OFF registers were cleared in disable and the user was required to
> call config to restore the duty cycle settings)
> - If one sets a period resulting in the same prescale register value,
> the sleep and write to the register is now skipped
> - Previously, only the full ON bit was toggled in GPIO mode (and full
> OFF cleared if set to high), which could result in both full OFF and
> full ON not being set and on=0, off=0, which is not allowed according
> to the datasheet
> - The OFF registers were reset to 0 in probe, which could lead to the
> forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
I didn't recheck all details, but the patch is definitively an
improvement, so:
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Apr 16, 2021 at 05:54:29PM +0200, Clemens Gruber wrote:
> On Fri, Apr 16, 2021 at 03:55:11PM +0200, Thierry Reding wrote:
> > On Mon, Apr 12, 2021 at 03:27:41PM +0200, Clemens Gruber wrote:
> > > Add the flag and corresponding documentation for PWM_USAGE_POWER.
> > >
> > > Cc: Rob Herring <[email protected]>
> > > Signed-off-by: Clemens Gruber <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/pwm/pwm.txt | 3 +++
> > > include/dt-bindings/pwm/pwm.h | 1 +
> > > 2 files changed, 4 insertions(+)
> >
> > Rob, what are your thoughts on this? I've been thinking about this some
> > more and I'm having second thoughts about putting this into device tree
> > because it doesn't actually describe a property of the PWM hardware but
> > rather a use-case specific hint. It's a bit of a gray area because this
> > is just part of the PWM specifier which already has use-case specific
> > "configuration", such as the period and the polarity.
This is something I'd prefer over making it part of the device tree API.
I still don't think it's a good idea but when we keep it in-kernel we
can at least easier modify it in the future.
> > Perhaps a better place for this is within the PWM API? We could add the
> > same information into struct pwm_state and then consumers that don't
> > care about specifics of the signal (such as pwm-backlight) can set that
> > flag when they request a state to be applied.
>
> I just want to note that in my opinion, this is not a flag that is
> changed often, so is it really a good idea to require setting this
> wherever PWM state is applied? Also, this can't be read-out in
> .get_state.
Not being able to read it out isn't a problem in my eyes.
> Thierry: If this discussion carries on and a v10 is required: Could you
> maybe merge the uncontroversial patches 1 to 3 of v9 separately and
> maybe get those in 5.12 ? Patches 4 to 8 can probably wait for 5.13 and
> have some time in linux-next.
I'm ok in getting those into next now and than into the upcoming merge
window. That won't make them part of 5.12 however, but 5.13-rc1. IMHO
patches 7 and 8 can go in, too.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Apr 12, 2021 at 03:27:45PM +0200, Clemens Gruber wrote:
> Regmap operations can fail if the underlying subsystem is not working
> properly (e.g. hogged I2C bus, etc.)
> As this is useful information for the user, print an error message if it
> happens.
> Let probe fail if the first regmap_read or the first regmap_write fails.
>
> Signed-off-by: Clemens Gruber <[email protected]>
Acked-by: Uwe Kleine-K?nig <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi,
On Sat, Apr 17, 2021 at 05:37:28PM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Apr 12, 2021 at 03:27:38PM +0200, Clemens Gruber wrote:
> > The switch to the atomic API goes hand in hand with a few fixes to
> > previously experienced issues:
> > - The duty cycle is no longer lost after disable/enable (previously the
> > OFF registers were cleared in disable and the user was required to
> > call config to restore the duty cycle settings)
> > - If one sets a period resulting in the same prescale register value,
> > the sleep and write to the register is now skipped
> > - Previously, only the full ON bit was toggled in GPIO mode (and full
> > OFF cleared if set to high), which could result in both full OFF and
> > full ON not being set and on=0, off=0, which is not allowed according
> > to the datasheet
> > - The OFF registers were reset to 0 in probe, which could lead to the
> > forbidden on=0, off=0. Fixed by resetting to POR default (full OFF)
>
> I didn't recheck all details, but the patch is definitively an
> improvement, so:
>
> Acked-by: Uwe Kleine-K?nig <[email protected]>
Thanks, but there is a newer version v9, I assume your acks are meant
for the newer one?
Clemens