| Message ID | 20210406164140.81423-7-clemens.gruber@pqgruber.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v7,1/8] pwm: pca9685: Switch to atomic API | expand |
On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote: > Previously, the last used PWM channel could change the global prescale > setting, even if other channels are already in use. > > Fix it by only allowing the first enabled PWM to change the global > chip-wide prescale setting. If there is more than one channel in use, > the prescale settings resulting from the chosen periods must match. > > GPIOs do not count as enabled PWMs as they are not using the prescaler > and can't change it. > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > --- > Changes since v6: > - Only allow the first PWM that is enabled to change the prescaler, not > the first one that uses the prescaler > > drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------ > 1 file changed, 56 insertions(+), 10 deletions(-) > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index 24221ee7a77a..cf0c98e4ef44 100644 > --- a/drivers/pwm/pwm-pca9685.c > +++ b/drivers/pwm/pwm-pca9685.c > @@ -23,11 +23,11 @@ > #include <linux/bitmap.h> > > /* > - * Because the PCA9685 has only one prescaler per chip, changing the period of > - * one channel affects the period of all 16 PWM outputs! > - * However, the ratio between each configured duty cycle and the chip-wide > - * period remains constant, because the OFF time is set in proportion to the > - * counter range. > + * Because the PCA9685 has only one prescaler per chip, only the first channel > + * that is enabled is allowed to change the prescale register. > + * PWM channels requested afterwards must use a period that results in the same > + * prescale setting as the one set by the first requested channel. > + * GPIOs do not count as enabled PWMs as they are not using the prescaler. > */ > > #define PCA9685_MODE1 0x00 > @@ -78,8 +78,9 @@ > struct pca9685 { > struct pwm_chip chip; > struct regmap *regmap; > -#if IS_ENABLED(CONFIG_GPIOLIB) > struct mutex lock; > + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1); > +#if IS_ENABLED(CONFIG_GPIOLIB) > struct gpio_chip gpio; > DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1); > #endif > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > return container_of(chip, struct pca9685, chip); > } > > +/* This function is supposed to be called with the lock mutex held */ > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel) > +{ > + /* No PWM enabled: Change allowed */ > + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1)) > + return true; > + /* More than one PWM enabled: Change not allowed */ > + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1) > + return false; > + /* > + * Only one PWM enabled: Change allowed if the PWM about to > + * be changed is the one that is already enabled > + */ > + return test_bit(channel, pca->pwms_enabled); Maybe this is a bit more effective?: DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1); bitmap_zero(blablub, PCA9685_MAXCHAN + 1); bitmap_set(blablub, channel); return bitmap_subset(pca->pwms_enabled, blablub); (but that's a minor issue, the suggested algorithm is correct.) So: Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> (side-note: I wonder if the handling of the set-all channel is correct here. But given that it is messy anyhow, (e.g. because setting some state to this set-all channel doesn't influence pwm_get_state for the individual channels) I don't object if there is another problem in this corner case. IMHO just dropping this virtual channel would be nice.) Best regards Uwe
On Wed, Apr 07, 2021 at 08:12:29AM +0200, Uwe Kleine-König wrote: > On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote: > > Previously, the last used PWM channel could change the global prescale > > setting, even if other channels are already in use. > > > > Fix it by only allowing the first enabled PWM to change the global > > chip-wide prescale setting. If there is more than one channel in use, > > the prescale settings resulting from the chosen periods must match. > > > > GPIOs do not count as enabled PWMs as they are not using the prescaler > > and can't change it. > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > --- > > Changes since v6: > > - Only allow the first PWM that is enabled to change the prescaler, not > > the first one that uses the prescaler > > > > drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------ > > 1 file changed, 56 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > index 24221ee7a77a..cf0c98e4ef44 100644 > > --- a/drivers/pwm/pwm-pca9685.c > > +++ b/drivers/pwm/pwm-pca9685.c > > @@ -23,11 +23,11 @@ > > #include <linux/bitmap.h> > > > > /* > > - * Because the PCA9685 has only one prescaler per chip, changing the period of > > - * one channel affects the period of all 16 PWM outputs! > > - * However, the ratio between each configured duty cycle and the chip-wide > > - * period remains constant, because the OFF time is set in proportion to the > > - * counter range. > > + * Because the PCA9685 has only one prescaler per chip, only the first channel > > + * that is enabled is allowed to change the prescale register. > > + * PWM channels requested afterwards must use a period that results in the same > > + * prescale setting as the one set by the first requested channel. > > + * GPIOs do not count as enabled PWMs as they are not using the prescaler. > > */ > > > > #define PCA9685_MODE1 0x00 > > @@ -78,8 +78,9 @@ > > struct pca9685 { > > struct pwm_chip chip; > > struct regmap *regmap; > > -#if IS_ENABLED(CONFIG_GPIOLIB) > > struct mutex lock; > > + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1); > > +#if IS_ENABLED(CONFIG_GPIOLIB) > > struct gpio_chip gpio; > > DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1); > > #endif > > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > > return container_of(chip, struct pca9685, chip); > > } > > > > +/* This function is supposed to be called with the lock mutex held */ > > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel) > > +{ > > + /* No PWM enabled: Change allowed */ > > + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1)) > > + return true; > > + /* More than one PWM enabled: Change not allowed */ > > + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1) > > + return false; > > + /* > > + * Only one PWM enabled: Change allowed if the PWM about to > > + * be changed is the one that is already enabled > > + */ > > + return test_bit(channel, pca->pwms_enabled); > > Maybe this is a bit more effective?: > > DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1); > bitmap_zero(blablub, PCA9685_MAXCHAN + 1); > bitmap_set(blablub, channel); > return bitmap_subset(pca->pwms_enabled, blablub); But if no PWM is enabled, it should return true, not false. > (but that's a minor issue, the suggested algorithm is correct.) I would prefer to keep it explicit because it is a little easier to follow and probably not worth optimizing. > > So: > > Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks. > > (side-note: I wonder if the handling of the set-all channel is correct > here. But given that it is messy anyhow, (e.g. because setting some > state to this set-all channel doesn't influence pwm_get_state for the > individual channels) I don't object if there is another problem in this > corner case. IMHO just dropping this virtual channel would be nice.) As you can't request the all channel and the individual channels together, there shouldn't be any problems. I agree that it would be nice to drop the ALL channel support. Clemens
On Wed, Apr 07, 2021 at 10:41:27PM +0200, Clemens Gruber wrote: > On Wed, Apr 07, 2021 at 08:12:29AM +0200, Uwe Kleine-König wrote: > > On Tue, Apr 06, 2021 at 06:41:39PM +0200, Clemens Gruber wrote: > > > Previously, the last used PWM channel could change the global prescale > > > setting, even if other channels are already in use. > > > > > > Fix it by only allowing the first enabled PWM to change the global > > > chip-wide prescale setting. If there is more than one channel in use, > > > the prescale settings resulting from the chosen periods must match. > > > > > > GPIOs do not count as enabled PWMs as they are not using the prescaler > > > and can't change it. > > > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > > --- > > > Changes since v6: > > > - Only allow the first PWM that is enabled to change the prescaler, not > > > the first one that uses the prescaler > > > > > > drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------ > > > 1 file changed, 56 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > index 24221ee7a77a..cf0c98e4ef44 100644 > > > --- a/drivers/pwm/pwm-pca9685.c > > > +++ b/drivers/pwm/pwm-pca9685.c > > > @@ -23,11 +23,11 @@ > > > #include <linux/bitmap.h> > > > > > > /* > > > - * Because the PCA9685 has only one prescaler per chip, changing the period of > > > - * one channel affects the period of all 16 PWM outputs! > > > - * However, the ratio between each configured duty cycle and the chip-wide > > > - * period remains constant, because the OFF time is set in proportion to the > > > - * counter range. > > > + * Because the PCA9685 has only one prescaler per chip, only the first channel > > > + * that is enabled is allowed to change the prescale register. > > > + * PWM channels requested afterwards must use a period that results in the same > > > + * prescale setting as the one set by the first requested channel. > > > + * GPIOs do not count as enabled PWMs as they are not using the prescaler. > > > */ > > > > > > #define PCA9685_MODE1 0x00 > > > @@ -78,8 +78,9 @@ > > > struct pca9685 { > > > struct pwm_chip chip; > > > struct regmap *regmap; > > > -#if IS_ENABLED(CONFIG_GPIOLIB) > > > struct mutex lock; > > > + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1); > > > +#if IS_ENABLED(CONFIG_GPIOLIB) > > > struct gpio_chip gpio; > > > DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1); > > > #endif > > > @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > > > return container_of(chip, struct pca9685, chip); > > > } > > > > > > +/* This function is supposed to be called with the lock mutex held */ > > > +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel) > > > +{ > > > + /* No PWM enabled: Change allowed */ > > > + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1)) > > > + return true; > > > + /* More than one PWM enabled: Change not allowed */ > > > + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1) > > > + return false; > > > + /* > > > + * Only one PWM enabled: Change allowed if the PWM about to > > > + * be changed is the one that is already enabled > > > + */ > > > + return test_bit(channel, pca->pwms_enabled); > > > > Maybe this is a bit more effective?: > > > > DECLARE_BITMAP(blablub, PCA9685_MAXCHAN + 1); > > bitmap_zero(blablub, PCA9685_MAXCHAN + 1); > > bitmap_set(blablub, channel); > > return bitmap_subset(pca->pwms_enabled, blablub); > > But if no PWM is enabled, it should return true, not false. If no PWM is enabled we have pca->pwms_enabled = empty set which is a subset of every set. So I'd expect this case to be handled just fine. > > (but that's a minor issue, the suggested algorithm is correct.) > > I would prefer to keep it explicit because it is a little easier to > follow and probably not worth optimizing. I didn't find it hard to follow, but I'm willing to accept that this isn't representative. I'm ok with keeping the code as is. > I agree that it would be nice to drop the ALL channel support. Maybe deprecate it using a config item? But no hurry. Best regards Uwe
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 24221ee7a77a..cf0c98e4ef44 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -23,11 +23,11 @@ #include <linux/bitmap.h> /* - * Because the PCA9685 has only one prescaler per chip, changing the period of - * one channel affects the period of all 16 PWM outputs! - * However, the ratio between each configured duty cycle and the chip-wide - * period remains constant, because the OFF time is set in proportion to the - * counter range. + * Because the PCA9685 has only one prescaler per chip, only the first channel + * that is enabled is allowed to change the prescale register. + * PWM channels requested afterwards must use a period that results in the same + * prescale setting as the one set by the first requested channel. + * GPIOs do not count as enabled PWMs as they are not using the prescaler. */ #define PCA9685_MODE1 0x00 @@ -78,8 +78,9 @@ struct pca9685 { struct pwm_chip chip; struct regmap *regmap; -#if IS_ENABLED(CONFIG_GPIOLIB) struct mutex lock; + DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1); +#if IS_ENABLED(CONFIG_GPIOLIB) struct gpio_chip gpio; DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1); #endif @@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) return container_of(chip, struct pca9685, chip); } +/* This function is supposed to be called with the lock mutex held */ +static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel) +{ + /* No PWM enabled: Change allowed */ + if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1)) + return true; + /* More than one PWM enabled: Change not allowed */ + if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1) + return false; + /* + * Only one PWM enabled: Change allowed if the PWM about to + * be changed is the one that is already enabled + */ + return test_bit(channel, pca->pwms_enabled); +} + /* 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) { @@ -265,8 +282,6 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca) { struct device *dev = pca->chip.dev; - mutex_init(&pca->lock); - pca->gpio.label = dev_name(dev); pca->gpio.parent = dev; pca->gpio.request = pca9685_pwm_gpio_request; @@ -310,8 +325,8 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable) } } -static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, - const struct pwm_state *state) +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, prescale; @@ -340,6 +355,12 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, regmap_read(pca->regmap, PCA9685_PRESCALE, &val); if (prescale != val) { + if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) { + dev_err(chip->dev, + "pwm not changed: periods of enabled pwms must match!\n"); + return -EBUSY; + } + /* * Putting the chip briefly into SLEEP mode * at this point won't interfere with the @@ -360,6 +381,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } +static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct pca9685 *pca = to_pca(chip); + int ret; + + mutex_lock(&pca->lock); + ret = __pca9685_pwm_apply(chip, pwm, state); + if (ret == 0) { + if (state->enabled) + set_bit(pwm->hwpwm, pca->pwms_enabled); + else + clear_bit(pwm->hwpwm, pca->pwms_enabled); + } + mutex_unlock(&pca->lock); + + return ret; +} + static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { @@ -420,7 +460,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) { struct pca9685 *pca = to_pca(chip); + mutex_lock(&pca->lock); pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); + clear_bit(pwm->hwpwm, pca->pwms_enabled); + mutex_unlock(&pca->lock); + pm_runtime_put(chip->dev); pca9685_pwm_clear_inuse(pca, pwm->hwpwm); } @@ -461,6 +505,8 @@ static int pca9685_pwm_probe(struct i2c_client *client, i2c_set_clientdata(client, pca); + mutex_init(&pca->lock); + regmap_read(pca->regmap, PCA9685_MODE2, ®); if (device_property_read_bool(&client->dev, "invert"))
Previously, the last used PWM channel could change the global prescale setting, even if other channels are already in use. Fix it by only allowing the first enabled PWM to change the global chip-wide prescale setting. If there is more than one channel in use, the prescale settings resulting from the chosen periods must match. GPIOs do not count as enabled PWMs as they are not using the prescaler and can't change it. Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> --- Changes since v6: - Only allow the first PWM that is enabled to change the prescaler, not the first one that uses the prescaler drivers/pwm/pwm-pca9685.c | 66 +++++++++++++++++++++++++++++++++------ 1 file changed, 56 insertions(+), 10 deletions(-)