| Message ID | 20201215212228.185517-7-clemens.gruber@pqgruber.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v5,1/7] pwm: pca9685: Switch to atomic API | expand |
Hi Clemens, see below. On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > Previously, the last used PWM channel could change the global prescale > setting, even if other channels were already in use. > > Fix it by only allowing the first user of the prescaler to change the > global chip-wide prescale setting. If there is more than one channel in > use, the prescale settings resulting from the chosen periods must match. > > PWMs that are disabled or have a duty cycle of 0% or 100% are not > considered to be using the prescaler as they have the full OFF or full > ON bits set. This also applies to channels used as GPIOs. > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > --- > drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------ > 1 file changed, 44 insertions(+), 7 deletions(-) > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index ff916980de49..438492d4aed4 100644 > --- a/drivers/pwm/pwm-pca9685.c > +++ b/drivers/pwm/pwm-pca9685.c > @@ -23,11 +23,11 @@ > #include <linux/bitmap.h> > > /* > - * Because the PCA9685 has only one prescaler per chip, changing the period of > - * one channel affects the period of all 16 PWM outputs! > - * However, the ratio between each configured duty cycle and the chip-wide > - * period remains constant, because the OFF time is set in proportion to the > - * counter range. > + * Because the PCA9685 has only one prescaler per chip, only the first channel > + * that uses the prescaler is allowed to change the prescale register. > + * PWM channels requested afterwards must use a period that results in the same > + * prescale setting as the one set by the first requested channel, unless they > + * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON). > */ > > #define PCA9685_MODE1 0x00 > @@ -80,6 +80,8 @@ struct pca9685 { > struct pwm_chip chip; > struct regmap *regmap; > bool staggered_outputs; > + struct mutex prescaler_users_lock; Keep things simple by re-using the "struct mutex lock" below? This code isn't performance-intensive, so having a single lock for pwm/gpio requests + pwm_apply() is probably ok. > + DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1); Rename to pwms_use_prescale ? > #if IS_ENABLED(CONFIG_GPIOLIB) > struct mutex lock; > struct gpio_chip gpio; > @@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > return container_of(chip, struct pca9685, chip); > } > > +/* This function is supposed to be called with the prescaler_users_lock held */ > +static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel) Drop the inline? Only the compiler knows if inlining this function makes sense on a platform (armv7, x86, etc). Compilers are usually better at this then humans... Rename to pca9685_prescaler_can_change() ? > +{ > + /* > + * A PWM channel may only change the prescaler if there are no users of > + * the prescaler yet or that same channel is the only one in use. > + */ > + return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) || > + (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 && > + test_bit(channel, pca->prescaler_users)); > +} I found this logic expression quite complex to read. Perhaps simplify by using a few steps? For example: /* if prescaler not in use, we can always change it */ if (empty) return true; /* if more than one pwm is using the prescaler, we can never change it */ if (weight > 1) return false; /* one pwm is using the prescaler, we can only change it if it's us */ return test_bit(us); > + > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty) > { > unsigned int on, off; > @@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > duty = PCA9685_COUNTER_RANGE * state->duty_cycle; > duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); > > + mutex_lock(&pca->prescaler_users_lock); > + > if (!state->enabled || duty < 1) { > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); > - return 0; > + goto prescaler_unused; > } else if (duty == PCA9685_COUNTER_RANGE) { > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); > - return 0; > + goto prescaler_unused; > } > > regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > if (prescale != val) { > + if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) { > + mutex_unlock(&pca->prescaler_users_lock); > + dev_err(chip->dev, > + "prescaler not set: already in use with different setting!\n"); > + return -EBUSY; > + } > + > /* > * Putting the chip briefly into SLEEP mode > * at this point won't interfere with the > @@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > } > > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); > + > + set_bit(pwm->hwpwm, pca->prescaler_users); > + mutex_unlock(&pca->prescaler_users_lock); > + return 0; > + > +prescaler_unused: > + clear_bit(pwm->hwpwm, pca->prescaler_users); > + mutex_unlock(&pca->prescaler_users_lock); > return 0; > } The need for the mutex makes this function quite "messy": we have to guard all the exits, and that's easy to forget. Maybe simplify the function by moving the mutex to a helper? Example: static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { ... just do stuff and don't worry about the mutex } static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { /* document why we serialize pwm_apply */ mutex_lock(); __pca9685_pwm_apply(chip, pwm, state); mutex_unlock(); } > > @@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct pca9685 *pca = to_pca(chip); > > + mutex_lock(&pca->prescaler_users_lock); > + clear_bit(pwm->hwpwm, pca->prescaler_users); > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); > + mutex_unlock(&pca->prescaler_users_lock); > + > pm_runtime_put(chip->dev); > pca9685_pwm_clear_inuse(pca, pwm->hwpwm); > } > @@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client, > > i2c_set_clientdata(client, pca); > > + mutex_init(&pca->prescaler_users_lock); > + > regmap_read(pca->regmap, PCA9685_MODE2, ®); > > if (device_property_read_bool(&client->dev, "invert")) > -- > 2.29.2 >
Hi Sven, On Wed, Dec 16, 2020 at 11:03:39PM -0500, Sven Van Asbroeck wrote: > Hi Clemens, see below. > > On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > > Previously, the last used PWM channel could change the global prescale > > setting, even if other channels were already in use. > > > > Fix it by only allowing the first user of the prescaler to change the > > global chip-wide prescale setting. If there is more than one channel in > > use, the prescale settings resulting from the chosen periods must match. > > > > PWMs that are disabled or have a duty cycle of 0% or 100% are not > > considered to be using the prescaler as they have the full OFF or full > > ON bits set. This also applies to channels used as GPIOs. > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > --- > > drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------ > > 1 file changed, 44 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > index ff916980de49..438492d4aed4 100644 > > --- a/drivers/pwm/pwm-pca9685.c > > +++ b/drivers/pwm/pwm-pca9685.c > > @@ -23,11 +23,11 @@ > > #include <linux/bitmap.h> > > > > /* > > - * Because the PCA9685 has only one prescaler per chip, changing the period of > > - * one channel affects the period of all 16 PWM outputs! > > - * However, the ratio between each configured duty cycle and the chip-wide > > - * period remains constant, because the OFF time is set in proportion to the > > - * counter range. > > + * Because the PCA9685 has only one prescaler per chip, only the first channel > > + * that uses the prescaler is allowed to change the prescale register. > > + * PWM channels requested afterwards must use a period that results in the same > > + * prescale setting as the one set by the first requested channel, unless they > > + * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON). > > */ > > > > #define PCA9685_MODE1 0x00 > > @@ -80,6 +80,8 @@ struct pca9685 { > > struct pwm_chip chip; > > struct regmap *regmap; > > bool staggered_outputs; > > + struct mutex prescaler_users_lock; > > Keep things simple by re-using the "struct mutex lock" below? > This code isn't performance-intensive, so having a single lock for > pwm/gpio requests + pwm_apply() is probably ok. Yes, I think this could work. Good idea. > > > + DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1); > > Rename to pwms_use_prescale ? Yes, fine with me. > > > #if IS_ENABLED(CONFIG_GPIOLIB) > > struct mutex lock; > > struct gpio_chip gpio; > > @@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > > return container_of(chip, struct pca9685, chip); > > } > > > > +/* This function is supposed to be called with the prescaler_users_lock held */ > > +static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel) > > Drop the inline? Only the compiler knows if inlining this function makes sense > on a platform (armv7, x86, etc). Compilers are usually better at this then > humans... You're probably right. I will drop the inline. > > Rename to pca9685_prescaler_can_change() ? Sounds good! > > > +{ > > + /* > > + * A PWM channel may only change the prescaler if there are no users of > > + * the prescaler yet or that same channel is the only one in use. > > + */ > > + return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) || > > + (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 && > > + test_bit(channel, pca->prescaler_users)); > > +} > > I found this logic expression quite complex to read. Perhaps simplify by using > a few steps? For example: > > /* if prescaler not in use, we can always change it */ > if (empty) return true; > /* if more than one pwm is using the prescaler, we can never change it */ > if (weight > 1) return false; > /* one pwm is using the prescaler, we can only change it if it's us */ > return test_bit(us); Good point, I will simplify it! > > > + > > static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty) > > { > > unsigned int on, off; > > @@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > duty = PCA9685_COUNTER_RANGE * state->duty_cycle; > > duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); > > > > + mutex_lock(&pca->prescaler_users_lock); > > + > > if (!state->enabled || duty < 1) { > > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); > > - return 0; > > + goto prescaler_unused; > > } else if (duty == PCA9685_COUNTER_RANGE) { > > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); > > - return 0; > > + goto prescaler_unused; > > } > > > > regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > > if (prescale != val) { > > + if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) { > > + mutex_unlock(&pca->prescaler_users_lock); > > + dev_err(chip->dev, > > + "prescaler not set: already in use with different setting!\n"); > > + return -EBUSY; > > + } > > + > > /* > > * Putting the chip briefly into SLEEP mode > > * at this point won't interfere with the > > @@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > } > > > > pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); > > + > > + set_bit(pwm->hwpwm, pca->prescaler_users); > > + mutex_unlock(&pca->prescaler_users_lock); > > + return 0; > > + > > +prescaler_unused: > > + clear_bit(pwm->hwpwm, pca->prescaler_users); > > + mutex_unlock(&pca->prescaler_users_lock); > > return 0; > > } > > The need for the mutex makes this function quite "messy": we have to guard all > the exits, and that's easy to forget. I agree. > > Maybe simplify the function by moving the mutex to a helper? > Example: > > static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > ... just do stuff and don't worry about the mutex > } > > static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > /* document why we serialize pwm_apply */ > mutex_lock(); > __pca9685_pwm_apply(chip, pwm, state); > mutex_unlock(); > } Also a good idea! As always, great review! Thank you! > > > > > @@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > { > > struct pca9685 *pca = to_pca(chip); > > > > + mutex_lock(&pca->prescaler_users_lock); > > + clear_bit(pwm->hwpwm, pca->prescaler_users); > > pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); > > + mutex_unlock(&pca->prescaler_users_lock); > > + > > pm_runtime_put(chip->dev); > > pca9685_pwm_clear_inuse(pca, pwm->hwpwm); > > } > > @@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client, > > > > i2c_set_clientdata(client, pca); > > > > + mutex_init(&pca->prescaler_users_lock); > > + > > regmap_read(pca->regmap, PCA9685_MODE2, ®); > > > > if (device_property_read_bool(&client->dev, "invert")) > > -- > > 2.29.2 > >
On Thu, Dec 17, 2020 at 1:07 PM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > As always, great review! Thank you! > My pleasure, it's great to help out. And thanks, you're the one doing all the hard work :)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index ff916980de49..438492d4aed4 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -23,11 +23,11 @@ #include <linux/bitmap.h> /* - * Because the PCA9685 has only one prescaler per chip, changing the period of - * one channel affects the period of all 16 PWM outputs! - * However, the ratio between each configured duty cycle and the chip-wide - * period remains constant, because the OFF time is set in proportion to the - * counter range. + * Because the PCA9685 has only one prescaler per chip, only the first channel + * that uses the prescaler is allowed to change the prescale register. + * PWM channels requested afterwards must use a period that results in the same + * prescale setting as the one set by the first requested channel, unless they + * use duty cycles of 0% or 100% (prescaler not used for full OFF/ON). */ #define PCA9685_MODE1 0x00 @@ -80,6 +80,8 @@ struct pca9685 { struct pwm_chip chip; struct regmap *regmap; bool staggered_outputs; + struct mutex prescaler_users_lock; + DECLARE_BITMAP(prescaler_users, PCA9685_MAXCHAN + 1); #if IS_ENABLED(CONFIG_GPIOLIB) struct mutex lock; struct gpio_chip gpio; @@ -92,6 +94,18 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) return container_of(chip, struct pca9685, chip); } +/* This function is supposed to be called with the prescaler_users_lock held */ +static inline bool pca9685_may_change_prescaler(struct pca9685 *pca, int channel) +{ + /* + * A PWM channel may only change the prescaler if there are no users of + * the prescaler yet or that same channel is the only one in use. + */ + return bitmap_empty(pca->prescaler_users, PCA9685_MAXCHAN + 1) || + (bitmap_weight(pca->prescaler_users, PCA9685_MAXCHAN + 1) == 1 && + test_bit(channel, pca->prescaler_users)); +} + static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty) { unsigned int on, off; @@ -337,16 +351,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, duty = PCA9685_COUNTER_RANGE * state->duty_cycle; duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); + mutex_lock(&pca->prescaler_users_lock); + if (!state->enabled || duty < 1) { pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); - return 0; + goto prescaler_unused; } else if (duty == PCA9685_COUNTER_RANGE) { pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); - return 0; + goto prescaler_unused; } regmap_read(pca->regmap, PCA9685_PRESCALE, &val); if (prescale != val) { + if (!pca9685_may_change_prescaler(pca, pwm->hwpwm)) { + mutex_unlock(&pca->prescaler_users_lock); + dev_err(chip->dev, + "prescaler not set: already in use with different setting!\n"); + return -EBUSY; + } + /* * Putting the chip briefly into SLEEP mode * at this point won't interfere with the @@ -364,6 +387,14 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, } pca9685_pwm_set_duty(pca, pwm->hwpwm, duty); + + set_bit(pwm->hwpwm, pca->prescaler_users); + mutex_unlock(&pca->prescaler_users_lock); + return 0; + +prescaler_unused: + clear_bit(pwm->hwpwm, pca->prescaler_users); + mutex_unlock(&pca->prescaler_users_lock); return 0; } @@ -422,7 +453,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) { struct pca9685 *pca = to_pca(chip); + mutex_lock(&pca->prescaler_users_lock); + clear_bit(pwm->hwpwm, pca->prescaler_users); pca9685_pwm_set_duty(pca, pwm->hwpwm, 0); + mutex_unlock(&pca->prescaler_users_lock); + pm_runtime_put(chip->dev); pca9685_pwm_clear_inuse(pca, pwm->hwpwm); } @@ -463,6 +498,8 @@ static int pca9685_pwm_probe(struct i2c_client *client, i2c_set_clientdata(client, pca); + mutex_init(&pca->prescaler_users_lock); + regmap_read(pca->regmap, PCA9685_MODE2, ®); if (device_property_read_bool(&client->dev, "invert"))
Previously, the last used PWM channel could change the global prescale setting, even if other channels were already in use. Fix it by only allowing the first user of the prescaler to change the global chip-wide prescale setting. If there is more than one channel in use, the prescale settings resulting from the chosen periods must match. PWMs that are disabled or have a duty cycle of 0% or 100% are not considered to be using the prescaler as they have the full OFF or full ON bits set. This also applies to channels used as GPIOs. Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> --- drivers/pwm/pwm-pca9685.c | 51 +++++++++++++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 7 deletions(-)