| Message ID | 20210412132745.76609-6-clemens.gruber@pqgruber.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8,1/8] pwm: pca9685: Switch to atomic API | expand |
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 <clemens.gruber@pqgruber.com> > --- > 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
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 <clemens.gruber@pqgruber.com> > > --- > > 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
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
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;
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 <clemens.gruber@pqgruber.com> --- drivers/pwm/pwm-pca9685.c | 63 ++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 14 deletions(-)