| Message ID | 20201207193629.493241-1-clemens.gruber@pqgruber.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/4] pwm: pca9685: Switch to atomic API | expand |
On Mon, Dec 07, 2020 at 08:36:27PM +0100, 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 > - The prescale register is now set to the default value in probe. On > systems without CONFIG_PM, the chip is woken up at probe time. > > 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. If one channel is reconfigured with new duty > cycle and period, the others will keep the same relative duty cycle to > period ratio as they had before, even though the per-chip / global > frequency changed. (The PCA9685 has only one prescaler!) This is not acceptable, if you have two PWM outputs and a consumer modifies one of them the other must change. So if this chip only supports a single period length of all channels, the first consumer enabling a channel defines the period to be used. All later consumers must live with that. (Also the first must be denied modifying the period if a second consumer has enabled its PWM.) > 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. That means the datasheet is lax because 5079040ns corresponds to 196.88760080645162 Hz but it calls that 200 Hz, right? I didn't look in the patch in detail, but get the impression it is more complicated than necessary. For example adding improved PM behaviour should probably go into a separate patch, also adding the .get_state callback should be split out. Best regards Uwe
Hi Uwe, On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > This is not acceptable, if you have two PWM outputs and a consumer > modifies one of them the other must change. So if this chip only > supports a single period length of all channels, the first consumer > enabling a channel defines the period to be used. All later consumers > must live with that. (Also the first must be denied modifying the period > if a second consumer has enabled its PWM.) That makes sense. However, a possible wrinkle: when more than one pwm channel is requested, which one is able to change the period? Example: 1. start with all pwms free 2. pwm_request(0), pwm_apply(period=200Hz) 3. pwm_request(1) 4. pwm_apply(1, period=400Hz) fails? 5. pwm_apply(0, period=400Hz) succeeds? And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz, because the pwm core doesn't realize anything has changed. Are you ok with this behaviour?
On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote: > On Mon, Dec 07, 2020 at 08:36:27PM +0100, 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 > > - The prescale register is now set to the default value in probe. On > > systems without CONFIG_PM, the chip is woken up at probe time. > > > > 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. If one channel is reconfigured with new duty > > cycle and period, the others will keep the same relative duty cycle to > > period ratio as they had before, even though the per-chip / global > > frequency changed. (The PCA9685 has only one prescaler!) > > This is not acceptable, if you have two PWM outputs and a consumer > modifies one of them the other must change. So if this chip only > supports a single period length of all channels, the first consumer > enabling a channel defines the period to be used. All later consumers > must live with that. (Also the first must be denied modifying the period > if a second consumer has enabled its PWM.) Good idea, but is it OK to potentially break users relying on the old behavior ("the last one who changes the period wins") ? > > > 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. > > That means the datasheet is lax because 5079040ns corresponds to > 196.88760080645162 Hz but it calls that 200 Hz, right? Yes, it calls prescale setting 0x1E 200 Hz, but calculating the period from that prescaler setting leads to 5079040ns (196.9 Hz) as you mentioned. Also, the datasheet does not specify frequency accuracy / internal oscillator specifications. I measured about 207 Hz on one chip and about 205 Hz on another with the scope today, when configuring a 5079040ns period. > > I didn't look in the patch in detail, but get the impression it is more > complicated than necessary. For example adding improved PM behaviour > should probably go into a separate patch, also adding the .get_state > callback should be split out. Agreed. I'll split it up more in the next revision! Thanks, Clemens
Hi Clemens, see below. On Mon, Dec 7, 2020 at 2:37 PM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > The switch to the atomic API goes hand in hand with a few fixes to > previously experienced issues: > - The duty cycle is no longer lost after disable/enable (previously the > OFF registers were cleared in disable and the user was required to > call config to restore the duty cycle settings) > - If one sets a period resulting in the same prescale register value, > the sleep and write to the register is now skipped > - The prescale register is now set to the default value in probe. On > systems without CONFIG_PM, the chip is woken up at probe time. > > 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. If one channel is reconfigured with new duty > cycle and period, the others will keep the same relative duty cycle to > period ratio as they had before, even though the per-chip / global > frequency changed. (The PCA9685 has only one prescaler!) > > Note that although the datasheet mentions 200 Hz as default frequency > when using the internal 25 MHz oscillator, the calculated period from > the default prescaler register setting of 30 is 5079040ns. > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > --- > 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 | 335 ++++++++++++++++++++------------------ > 1 file changed, 175 insertions(+), 160 deletions(-) > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index 4a55dc18656c..0425e0bcb81e 100644 > --- a/drivers/pwm/pwm-pca9685.c > +++ b/drivers/pwm/pwm-pca9685.c > @@ -47,11 +47,11 @@ > #define PCA9685_ALL_LED_OFF_H 0xFD > #define PCA9685_PRESCALE 0xFE > > +#define PCA9685_PRESCALE_DEF 0x1E /* => default frequency of ~200 Hz */ > #define PCA9685_PRESCALE_MIN 0x03 /* => max. frequency of 1526 Hz */ > #define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */ > > #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 > @@ -74,7 +74,6 @@ > 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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > return container_of(chip, struct pca9685, chip); > } > > +static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable) > +{ > + unsigned int val = enable ? LED_FULL : 0; > + > + /* Note: The full OFF bit has the highest precedence */ > + > + if (index >= PCA9685_MAXCHAN) { > + regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val); > + return; > + } > + regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val); > +} > + > +static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index) > +{ > + unsigned int val = 0; > + > + if (index >= PCA9685_MAXCHAN) > + return false; > + > + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); > + return val & LED_FULL; > +} > + > +static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable) > +{ > + unsigned int val = enable ? LED_FULL : 0; > + > + if (index >= PCA9685_MAXCHAN) { > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val); > + return; > + } > + regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val); > +} If the "full off" bit is set, calling pwm_set_full_on(pca, index, true) won't actually bring the led full on, correct ? This can be very confusing. See below for a suggestion to make this clearer. > + > +static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index) > +{ > + unsigned int val = 0; > + > + if (index >= PCA9685_MAXCHAN) > + return false; > + > + regmap_read(pca->regmap, LED_N_ON_H(index), &val); > + return val & LED_FULL; > +} > + > +static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off) > +{ > + int reg; > + > + /* Note: This function also clears the full OFF bit */ > + > + reg = index >= PCA9685_MAXCHAN ? > + PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index); > + regmap_write(pca->regmap, reg, off & 0xff); > + > + reg = index >= PCA9685_MAXCHAN ? > + PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index); > + regmap_write(pca->regmap, reg, (off >> 8) & 0xf); > +} > + > +static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index) > +{ > + unsigned int off, val = 0; > + > + if (index >= PCA9685_MAXCHAN) > + return 0; > + > + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); > + off = (val & 0xf) << 8; > + > + regmap_read(pca->regmap, LED_N_OFF_L(index), &val); > + return off | (val & 0xff); > +} > + > #if IS_ENABLED(CONFIG_GPIOLIB) > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx) > { > @@ -138,34 +212,31 @@ 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_is_full_off(pca, offset); > } > > 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); > + if (value) { > + pca9685_pwm_set_full_on(pca, offset, true); > + /* Clear full OFF bit last */ > + pca9685_pwm_set_full_off(pca, offset, false); > + } else { > + /* Set full OFF bit first */ > + pca9685_pwm_set_full_off(pca, offset, true); > + pca9685_pwm_set_full_on(pca, offset, false); > + } > } > > 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_full_off(pca, offset, true); > pm_runtime_put(pca->chip.dev); > pca9685_pwm_clear_inuse(pca, offset); > } > @@ -246,165 +317,98 @@ 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 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 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 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); > + /* Calculate (chip-wide) period from prescale value */ > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > + (val + 1); > > - regmap_write(pca->regmap, reg, LED_FULL); > + /* The (per-channel) polarity is fixed */ > + state->polarity = PWM_POLARITY_NORMAL; > > - return 0; > + 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; > } > > - 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); > + state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm); > > - regmap_write(pca->regmap, reg, LED_FULL); > - > - return 0; > + if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) { > + state->duty_cycle = state->period; > + return; > } > > - 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; > + duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period; > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; > } > > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > +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 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); > + unsigned long long duty, prescale; > + unsigned int val = 0; > > - regmap_write(pca->regmap, reg, 0); > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -EOPNOTSUPP; > > - 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); > - > - /* > - * 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); > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period, > + PCA9685_COUNTER_RANGE * 1000) - 1; > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) { > + dev_err(chip->dev, "prescaler not set: period out of bounds!\n"); > + return -EINVAL; > + } > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0); > + 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); > > - return 0; > -} > + /* Change the chip-wide output frequency */ > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale); > > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > -{ > - struct pca9685 *pca = to_pca(chip); > - unsigned int reg; > + /* Wake the chip up */ > + pca9685_set_sleep_mode(pca, false); > + } > > - if (pwm->hwpwm >= PCA9685_MAXCHAN) > - reg = PCA9685_ALL_LED_OFF_H; > - else > - reg = LED_N_OFF_H(pwm->hwpwm); > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle; > + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); > > - regmap_write(pca->regmap, reg, LED_FULL); > + if (!state->enabled || duty < 1) { > + /* Set full OFF bit first */ > + pca9685_pwm_set_full_off(pca, pwm->hwpwm, true); > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); > + return 0; > + } > > - /* Clear the LED_OFF counter. */ > - if (pwm->hwpwm >= PCA9685_MAXCHAN) > - reg = PCA9685_ALL_LED_OFF_L; > - else > - reg = LED_N_OFF_L(pwm->hwpwm); > + if (duty == PCA9685_COUNTER_RANGE) { > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, true); > + /* Clear full OFF bit last */ > + pca9685_pwm_set_full_off(pca, pwm->hwpwm, false); I think "this bit first, this bit last" can be confusing and fragile. It is also repeated in a few different places. Suggestion on how to improve this, below. > + return 0; > + } > > - regmap_write(pca->regmap, reg, 0x0); > + pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty); > + /* Clear full ON bit after OFF time was set */ > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); > + return 0; > } I think pwm_apply() is broken in the following scenario: 1. pwm_apply(enable=false) 2. pwm_apply(enable=true, duty_cycle=50%, period=200Hz) 3. result: pwm still full off (disabled) and that is because pwm_apply() clears the "full off" bit only when the user requests "full on". I attribute this to the confusing nature of this chip. It's hard to keep the chip's complete state fully in one's head. One possible way to keep this manageable is by writing the functions in terms of *led state* and not in terms of the *bits* it sets: - set_full_on() should make the led go full on. So it sets "full on" and clears "full off" bits. - set_full_off() should make the led go full off. So it sets the "full off" bit. - set_duty() brings the led into duty mode. So it should clear "full off", clear "full on", and write the on/off times. And actually, all that's needed is a single function, because duty == 0 means "full off" and duty == 4095 means "full on". Example in pseudo code: static void pca9685_pwm_set_duty(struct pca9685 *pca, int index, unsigned int duty) { if (duty == 0) { /* full off takes precedence */ regmap_write(full_off(index), ON); } else if (duty >= (COUNTER_RANGE - 1)) { regmap_write(full_on(index), ON); regmap_write(full_off(index), OFF); } else { regmap_write(off_time(index), duty); regmap_write(full_on(index), OFF); regmap_write(full_off(index), OFF); } } static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int index) { if (WARN_ON(index >= PCA9685_MAXCHAN)) { /* register not readable, should never happen */ return 0; } if (full_off(index) is ON) return 0; else if (full_on(index) is ON) return COUNTER_RANGE - 1; return off_time(index); } I suspect that once all on/off register accesses go through these two functions, things will look simpler and there will be less risk of getting it wrong. To prevent a forest of "index >= PCA9685_MAXCHAN" checks, I suggest creating an helper function or macro, e.g. static unsigned int led_off_h(int index) { return (index >= PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index); } then setting a register looks simple: regmap_write(pca->regmap, led_off_h(index), LED_FULL); > > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > @@ -422,15 +426,14 @@ 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_full_off(pca, pwm->hwpwm, true); > 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, > + .get_state = pca9685_pwm_get_state, > + .apply = pca9685_pwm_apply, > .request = pca9685_pwm_request, > .free = pca9685_pwm_free, > .owner = THIS_MODULE, > @@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client, > ret); > return ret; > } > - pca->period_ns = PCA9685_DEFAULT_PERIOD; > > i2c_set_clientdata(client, pca); > > @@ -505,14 +507,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 > + * Always initialize with default prescale, but chip must be > + * in sleep mode while changing prescaler. > */ > + pca9685_set_sleep_mode(pca, true); > + regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF); > + pm_runtime_set_suspended(&client->dev); > pm_runtime_enable(&client->dev); > > + if (!IS_ENABLED(CONFIG_PM)) { > + /* Wake the chip up on non-PM environments */ > + pca9685_set_sleep_mode(pca, false); > + } > + > return 0; > } > > @@ -524,7 +532,14 @@ static int pca9685_pwm_remove(struct i2c_client *client) > ret = pwmchip_remove(&pca->chip); > if (ret) > return ret; > + > pm_runtime_disable(&client->dev); > + > + if (!IS_ENABLED(CONFIG_PM)) { > + /* Put chip in sleep state on non-PM environments */ > + pca9685_set_sleep_mode(pca, true); > + } > + > return 0; > } > > -- > 2.29.2 >
On Mon, Dec 07, 2020 at 05:34:58PM -0500, Sven Van Asbroeck wrote: > Hi Uwe, > > On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > This is not acceptable, if you have two PWM outputs and a consumer > > modifies one of them the other must change. So if this chip only > > supports a single period length of all channels, the first consumer > > enabling a channel defines the period to be used. All later consumers > > must live with that. (Also the first must be denied modifying the period > > if a second consumer has enabled its PWM.) > > That makes sense. However, a possible wrinkle: when more than one pwm channel > is requested, which one is able to change the period? > > Example: > 1. start with all pwms free > 2. pwm_request(0), pwm_apply(period=200Hz) > 3. pwm_request(1) > 4. pwm_apply(1, period=400Hz) fails? > 5. pwm_apply(0, period=400Hz) succeeds? > > And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz, > because the pwm core doesn't realize anything has changed. Are you ok > with this behaviour? I think we'd have to deny the pwm_apply in step 5 as well. So, only the first consumer is allowed to change the period and only as long as it is the only one that is in use / was requested. But that's definitely a breaking change. Thanks, Clemens
Hi Sven, On Mon, Dec 07, 2020 at 06:22:08PM -0500, Sven Van Asbroeck wrote: > Hi Clemens, see below. > > On Mon, Dec 7, 2020 at 2:37 PM Clemens Gruber > <clemens.gruber@pqgruber.com> wrote: > > > > The switch to the atomic API goes hand in hand with a few fixes to > > previously experienced issues: > > - The duty cycle is no longer lost after disable/enable (previously the > > OFF registers were cleared in disable and the user was required to > > call config to restore the duty cycle settings) > > - If one sets a period resulting in the same prescale register value, > > the sleep and write to the register is now skipped > > - The prescale register is now set to the default value in probe. On > > systems without CONFIG_PM, the chip is woken up at probe time. > > > > 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. If one channel is reconfigured with new duty > > cycle and period, the others will keep the same relative duty cycle to > > period ratio as they had before, even though the per-chip / global > > frequency changed. (The PCA9685 has only one prescaler!) > > > > Note that although the datasheet mentions 200 Hz as default frequency > > when using the internal 25 MHz oscillator, the calculated period from > > the default prescaler register setting of 30 is 5079040ns. > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > --- > > 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 | 335 ++++++++++++++++++++------------------ > > 1 file changed, 175 insertions(+), 160 deletions(-) > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > index 4a55dc18656c..0425e0bcb81e 100644 > > --- a/drivers/pwm/pwm-pca9685.c > > +++ b/drivers/pwm/pwm-pca9685.c > > @@ -47,11 +47,11 @@ > > #define PCA9685_ALL_LED_OFF_H 0xFD > > #define PCA9685_PRESCALE 0xFE > > > > +#define PCA9685_PRESCALE_DEF 0x1E /* => default frequency of ~200 Hz */ > > #define PCA9685_PRESCALE_MIN 0x03 /* => max. frequency of 1526 Hz */ > > #define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */ > > > > #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 > > @@ -74,7 +74,6 @@ > > 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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) > > return container_of(chip, struct pca9685, chip); > > } > > > > +static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable) > > +{ > > + unsigned int val = enable ? LED_FULL : 0; > > + > > + /* Note: The full OFF bit has the highest precedence */ > > + > > + if (index >= PCA9685_MAXCHAN) { > > + regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val); > > + return; > > + } > > + regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val); > > +} > > + > > +static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index) > > +{ > > + unsigned int val = 0; > > + > > + if (index >= PCA9685_MAXCHAN) > > + return false; > > + > > + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); > > + return val & LED_FULL; > > +} > > + > > +static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable) > > +{ > > + unsigned int val = enable ? LED_FULL : 0; > > + > > + if (index >= PCA9685_MAXCHAN) { > > + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val); > > + return; > > + } > > + regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val); > > +} > > If the "full off" bit is set, calling pwm_set_full_on(pca, index, true) > won't actually bring the led full on, correct ? Correct. Although I added a comment in pwm_set_full_off to explain that the full OFF bit has precedence and we are (un)setting the full OFF bit when necessary, I agree that this can and should be improved to be less confusing. > > This can be very confusing. See below for a suggestion to make this clearer. > > > + > > +static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index) > > +{ > > + unsigned int val = 0; > > + > > + if (index >= PCA9685_MAXCHAN) > > + return false; > > + > > + regmap_read(pca->regmap, LED_N_ON_H(index), &val); > > + return val & LED_FULL; > > +} > > + > > +static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off) > > +{ > > + int reg; > > + > > + /* Note: This function also clears the full OFF bit */ > > + > > + reg = index >= PCA9685_MAXCHAN ? > > + PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index); > > + regmap_write(pca->regmap, reg, off & 0xff); > > + > > + reg = index >= PCA9685_MAXCHAN ? > > + PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index); > > + regmap_write(pca->regmap, reg, (off >> 8) & 0xf); > > +} > > + > > +static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index) > > +{ > > + unsigned int off, val = 0; > > + > > + if (index >= PCA9685_MAXCHAN) > > + return 0; > > + > > + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); > > + off = (val & 0xf) << 8; > > + > > + regmap_read(pca->regmap, LED_N_OFF_L(index), &val); > > + return off | (val & 0xff); > > +} > > + > > #if IS_ENABLED(CONFIG_GPIOLIB) > > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx) > > { > > @@ -138,34 +212,31 @@ 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_is_full_off(pca, offset); > > } > > > > 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); > > + if (value) { > > + pca9685_pwm_set_full_on(pca, offset, true); > > + /* Clear full OFF bit last */ > > + pca9685_pwm_set_full_off(pca, offset, false); > > + } else { > > + /* Set full OFF bit first */ > > + pca9685_pwm_set_full_off(pca, offset, true); > > + pca9685_pwm_set_full_on(pca, offset, false); > > + } > > } > > > > 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_full_off(pca, offset, true); > > pm_runtime_put(pca->chip.dev); > > pca9685_pwm_clear_inuse(pca, offset); > > } > > @@ -246,165 +317,98 @@ 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 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 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 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); > > + /* Calculate (chip-wide) period from prescale value */ > > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > > + (val + 1); > > > > - regmap_write(pca->regmap, reg, LED_FULL); > > + /* The (per-channel) polarity is fixed */ > > + state->polarity = PWM_POLARITY_NORMAL; > > > > - return 0; > > + 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; > > } > > > > - 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); > > + state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm); > > > > - regmap_write(pca->regmap, reg, LED_FULL); > > - > > - return 0; > > + if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) { > > + state->duty_cycle = state->period; > > + return; > > } > > > > - 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; > > + duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period; > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; > > } > > > > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > > +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 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); > > + unsigned long long duty, prescale; > > + unsigned int val = 0; > > > > - regmap_write(pca->regmap, reg, 0); > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -EOPNOTSUPP; > > > > - 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); > > - > > - /* > > - * 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); > > + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period, > > + PCA9685_COUNTER_RANGE * 1000) - 1; > > + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) { > > + dev_err(chip->dev, "prescaler not set: period out of bounds!\n"); > > + return -EINVAL; > > + } > > > > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0); > > + 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); > > > > - return 0; > > -} > > + /* Change the chip-wide output frequency */ > > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale); > > > > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > > -{ > > - struct pca9685 *pca = to_pca(chip); > > - unsigned int reg; > > + /* Wake the chip up */ > > + pca9685_set_sleep_mode(pca, false); > > + } > > > > - if (pwm->hwpwm >= PCA9685_MAXCHAN) > > - reg = PCA9685_ALL_LED_OFF_H; > > - else > > - reg = LED_N_OFF_H(pwm->hwpwm); > > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle; > > + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); > > > > - regmap_write(pca->regmap, reg, LED_FULL); > > + if (!state->enabled || duty < 1) { > > + /* Set full OFF bit first */ > > + pca9685_pwm_set_full_off(pca, pwm->hwpwm, true); > > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); > > + return 0; > > + } > > > > - /* Clear the LED_OFF counter. */ > > - if (pwm->hwpwm >= PCA9685_MAXCHAN) > > - reg = PCA9685_ALL_LED_OFF_L; > > - else > > - reg = LED_N_OFF_L(pwm->hwpwm); > > + if (duty == PCA9685_COUNTER_RANGE) { > > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, true); > > + /* Clear full OFF bit last */ > > + pca9685_pwm_set_full_off(pca, pwm->hwpwm, false); > > I think "this bit first, this bit last" can be confusing and fragile. > It is also repeated in a few different places. > Suggestion on how to improve this, below. I wanted to let people know that the order is important, but yes, it is also a little confusing. > > > + return 0; > > + } > > > > - regmap_write(pca->regmap, reg, 0x0); > > + pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty); > > + /* Clear full ON bit after OFF time was set */ > > + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); > > + return 0; > > } > > I think pwm_apply() is broken in the following scenario: > > 1. pwm_apply(enable=false) > 2. pwm_apply(enable=true, duty_cycle=50%, period=200Hz) > 3. result: pwm still full off (disabled) No, this works correctly. I'll explain below. > > and that is because pwm_apply() clears the "full off" bit only when the user > requests "full on". The full OFF bit is cleared in pca9685_pwm_set_off_time. I did add a comment in that function that mentions it, but this can be easily overlooked and is not clear from the name, so also something that can and should be improved. > > I attribute this to the confusing nature of this chip. It's hard to keep the > chip's complete state fully in one's head. I agree, you always have to think about the chip's internals. The abstractions / common functions should contain the internals as implementation details, so we do not have to think about them every time we use on of these functions. > One possible way to keep this manageable is by writing the functions in terms of > *led state* and not in terms of the *bits* it sets: > > - set_full_on() should make the led go full on. So it sets "full on" > and clears "full off" bits. > - set_full_off() should make the led go full off. So it sets the "full off" bit. > - set_duty() brings the led into duty mode. So it should clear "full off", > clear "full on", and write the on/off times. > > And actually, all that's needed is a single function, because duty == 0 means > "full off" and duty == 4095 means "full on". Interesting! I'll have to think this through tomorrow but I think it could work. I will wait with the next revision until we decide what to do about Uwe's suggestion and then send a v5. > > Example in pseudo code: > > static void pca9685_pwm_set_duty(struct pca9685 *pca, int index, > unsigned int duty) > { > if (duty == 0) { > /* full off takes precedence */ > regmap_write(full_off(index), ON); > } else if (duty >= (COUNTER_RANGE - 1)) { > regmap_write(full_on(index), ON); > regmap_write(full_off(index), OFF); > } else { > regmap_write(off_time(index), duty); > regmap_write(full_on(index), OFF); > regmap_write(full_off(index), OFF); > } > } > > static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int index) > { > if (WARN_ON(index >= PCA9685_MAXCHAN)) { > /* register not readable, should never happen */ > return 0; > } > if (full_off(index) is ON) > return 0; > else if (full_on(index) is ON) > return COUNTER_RANGE - 1; > return off_time(index); > } > > I suspect that once all on/off register accesses go through these two functions, > things will look simpler and there will be less risk of getting it wrong. > > To prevent a forest of "index >= PCA9685_MAXCHAN" checks, I suggest creating > an helper function or macro, e.g. > > static unsigned int led_off_h(int index) > { > return (index >= PCA9685_MAXCHAN) ? PCA9685_ALL_LED_OFF_H : > LED_N_OFF_H(index); > } > > then setting a register looks simple: > regmap_write(pca->regmap, led_off_h(index), LED_FULL); > > > > > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > @@ -422,15 +426,14 @@ 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_full_off(pca, pwm->hwpwm, true); > > 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, > > + .get_state = pca9685_pwm_get_state, > > + .apply = pca9685_pwm_apply, > > .request = pca9685_pwm_request, > > .free = pca9685_pwm_free, > > .owner = THIS_MODULE, > > @@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client, > > ret); > > return ret; > > } > > - pca->period_ns = PCA9685_DEFAULT_PERIOD; > > > > i2c_set_clientdata(client, pca); > > > > @@ -505,14 +507,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 > > + * Always initialize with default prescale, but chip must be > > + * in sleep mode while changing prescaler. > > */ > > + pca9685_set_sleep_mode(pca, true); > > + regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF); > > + pm_runtime_set_suspended(&client->dev); > > pm_runtime_enable(&client->dev); > > > > + if (!IS_ENABLED(CONFIG_PM)) { > > + /* Wake the chip up on non-PM environments */ > > + pca9685_set_sleep_mode(pca, false); > > + } > > + > > return 0; > > } > > > > @@ -524,7 +532,14 @@ static int pca9685_pwm_remove(struct i2c_client *client) > > ret = pwmchip_remove(&pca->chip); > > if (ret) > > return ret; > > + > > pm_runtime_disable(&client->dev); > > + > > + if (!IS_ENABLED(CONFIG_PM)) { > > + /* Put chip in sleep state on non-PM environments */ > > + pca9685_set_sleep_mode(pca, true); > > + } > > + > > return 0; > > } > > > > -- > > 2.29.2 > > Thanks, Clemens
Hello Clemens, On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote: > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote: > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote: > > > 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. If one channel is reconfigured with new duty > > > cycle and period, the others will keep the same relative duty cycle to > > > period ratio as they had before, even though the per-chip / global > > > frequency changed. (The PCA9685 has only one prescaler!) > > > > This is not acceptable, if you have two PWM outputs and a consumer > > modifies one of them the other must change. So if this chip only > > supports a single period length of all channels, the first consumer > > enabling a channel defines the period to be used. All later consumers > > must live with that. (Also the first must be denied modifying the period > > if a second consumer has enabled its PWM.) > > Good idea, but is it OK to potentially break users relying on the old > behavior ("the last one who changes the period wins") ? If this is already in the old code, this probably warrants a separate fix, and yes, I consider this a severe bug. (Consider one channel driving a motor and reconfiguring an LED modifies the motor's speed.) Best regards Uwe
Hello Sven, On Mon, Dec 07, 2020 at 05:34:58PM -0500, Sven Van Asbroeck wrote: > On Mon, Dec 7, 2020 at 5:00 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > This is not acceptable, if you have two PWM outputs and a consumer > > modifies one of them the other must change. So if this chip only > > supports a single period length of all channels, the first consumer > > enabling a channel defines the period to be used. All later consumers > > must live with that. (Also the first must be denied modifying the period > > if a second consumer has enabled its PWM.) > > That makes sense. However, a possible wrinkle: when more than one pwm channel > is requested, which one is able to change the period? > > Example: > 1. start with all pwms free > 2. pwm_request(0), pwm_apply(period=200Hz) > 3. pwm_request(1) > 4. pwm_apply(1, period=400Hz) fails? Yes, pwm_apply_state is supposed to fail here (Sidenote: period is measured in ns, not Hz) > 5. pwm_apply(0, period=400Hz) succeeds? This succeeds iff channel 1 isn't enabled. (Unless changing might change the polarity of pwm #1 even if disabled.) > And if (5) succeeds, then pwm_get_state(1) will still return period=200Hz, > because the pwm core doesn't realize anything has changed. Are you ok > with this behaviour? "if (5) succeeds" implies channel 1 is disabled (it might otherwise have been enabled by the bootloader or a previous consumer). With that sorted out, I'm ok that pwm_get_state() reports .period=200Hz (or whatever other value) because it also reports .enabled = false which makes every interpretation of the other values in pwm_state (apart from .polarity) moot. Best regards Uwe
Hello Uwe, On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote: > Hello Clemens, > > On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote: > > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote: > > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote: > > > > 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. If one channel is reconfigured with new duty > > > > cycle and period, the others will keep the same relative duty cycle to > > > > period ratio as they had before, even though the per-chip / global > > > > frequency changed. (The PCA9685 has only one prescaler!) > > > > > > This is not acceptable, if you have two PWM outputs and a consumer > > > modifies one of them the other must change. So if this chip only > > > supports a single period length of all channels, the first consumer > > > enabling a channel defines the period to be used. All later consumers > > > must live with that. (Also the first must be denied modifying the period > > > if a second consumer has enabled its PWM.) > > > > Good idea, but is it OK to potentially break users relying on the old > > behavior ("the last one who changes the period wins") ? > > If this is already in the old code, this probably warrants a separate > fix, and yes, I consider this a severe bug. (Consider one channel > driving a motor and reconfiguring an LED modifies the motor's speed.) Yes, but a user could also be relying on the old behavior as follows: 1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns (does not care about the frequency as long as it does not flicker) 2. Requests & enables pwm 1 for a motor, using a period of 1000000ns (does care about the frequency) In the previous kernel versions, this would work, but with your suggested change, (2) would fail and the motor would no longer work. We are basically changing "the last one to set the period wins" to "the first one to set the period wins". If we do it like this, I'll split it out so we can at least revert it if someone complains that it breaks his application, without reverting the whole series. Thanks, Clemens
On Tue, Dec 08, 2020 at 11:12:18AM +0100, Clemens Gruber wrote: > Hello Uwe, > > On Tue, Dec 08, 2020 at 10:10:33AM +0100, Uwe Kleine-König wrote: > > Hello Clemens, > > > > On Tue, Dec 08, 2020 at 12:13:44AM +0100, Clemens Gruber wrote: > > > On Mon, Dec 07, 2020 at 11:00:25PM +0100, Uwe Kleine-König wrote: > > > > On Mon, Dec 07, 2020 at 08:36:27PM +0100, Clemens Gruber wrote: > > > > > 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. If one channel is reconfigured with new duty > > > > > cycle and period, the others will keep the same relative duty cycle to > > > > > period ratio as they had before, even though the per-chip / global > > > > > frequency changed. (The PCA9685 has only one prescaler!) > > > > > > > > This is not acceptable, if you have two PWM outputs and a consumer > > > > modifies one of them the other must change. So if this chip only > > > > supports a single period length of all channels, the first consumer > > > > enabling a channel defines the period to be used. All later consumers > > > > must live with that. (Also the first must be denied modifying the period > > > > if a second consumer has enabled its PWM.) > > > > > > Good idea, but is it OK to potentially break users relying on the old > > > behavior ("the last one who changes the period wins") ? > > > > If this is already in the old code, this probably warrants a separate > > fix, and yes, I consider this a severe bug. (Consider one channel > > driving a motor and reconfiguring an LED modifies the motor's speed.) > > Yes, but a user could also be relying on the old behavior as follows: > 1. Requests & enables pwm 0 for a backlight, using a period of 5000000ns > (does not care about the frequency as long as it does not flicker) > 2. Requests & enables pwm 1 for a motor, using a period of 1000000ns > (does care about the frequency) > > In the previous kernel versions, this would work, but with your > suggested change, (2) would fail and the motor would no longer work. > > We are basically changing "the last one to set the period wins" to "the > first one to set the period wins". > > If we do it like this, I'll split it out so we can at least revert it if > someone complains that it breaks his application, without reverting the > whole series. Yes, that makes sense to me. We do want to make sure that we don't have these kinds of races for PWM controllers and other drivers already have corresponding checks in place. But I agree that if this is preserving the status quo, then yes, we should follow up with a separate patch to add that check so that it can be easily reverted if this indeed break. Although, if we do get failures after this check has been introduced, they should be considered bugs and fixed in the right place. Ultimately this is something that board designers have hopefully already thought about and if there are two PWM consumers they will usually be able to run at a common period, in which case fixing these should be as easy as finding that common period and, well, using it for both consumers. Thierry
Uwe, Thierry, On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > If this is already in the old code, this probably warrants a separate > fix, and yes, I consider this a severe bug. (Consider one channel > driving a motor and reconfiguring an LED modifies the motor's speed.) > I think you are 100% correct, this would be a severe bug. I have only used this chip to drive LEDs, where the actual period is not that important. But for motor control, it's a different story. Basically you are suggesting: the period (prescaler) can only be changed iff its use-count is 1. This however brings up a whole load of additional questions: consider the case where the chip outputs are also used in gpio mode. the gpio functionality only sets "full on" and "full off" bits. On a scope, a gpio output will look identical, no matter the value of the period. So when a gpio output is in use, does it increment the prescaler use-count ? Example: 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) 2. output 2: set led mode (full-on bit set) 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz) Do we have to make (3) fail? I would say no: although output 2 is in use, it's not actually using the prescaler. Changing prescale won't modify output 2 in any way. Which brings us to an even trickier question: what happens if a pwm output is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. So when it's enabled, it does not use the prescaler. But! what happens if we now set that output to a different duty cycle? Example: 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) fail? no, because it's not actually using the period (it's full on) 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) fail? no, because it's not actually using the period (it's full on) 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) fail? no, because only output 1 is using the prescaler 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) fail? no, because output 2 is not changing the prescaler 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) fail? yes, because output 2 is changing prescaler and it's already in use IMHO all this can get very complicated and tricky. We can of course make this much simpler by assumung that gpio or on/off pwms are actually using the prescaler. But then we'd be limiting this chip's functionality.
On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > Uwe, Thierry, > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > If this is already in the old code, this probably warrants a separate > > fix, and yes, I consider this a severe bug. (Consider one channel > > driving a motor and reconfiguring an LED modifies the motor's speed.) > > > > I think you are 100% correct, this would be a severe bug. I have only used > this chip to drive LEDs, where the actual period is not that important. But > for motor control, it's a different story. > > Basically you are suggesting: the period (prescaler) can only be changed iff > its use-count is 1. > > This however brings up a whole load of additional questions: consider the case > where the chip outputs are also used in gpio mode. the gpio functionality > only sets "full on" and "full off" bits. On a scope, a gpio output will look > identical, no matter the value of the period. So when a gpio output is in use, > does it increment the prescaler use-count ? > > Example: > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > 2. output 2: set led mode (full-on bit set) > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz) > > Do we have to make (3) fail? I would say no: although output 2 is in use, > it's not actually using the prescaler. Changing prescale won't modify > output 2 in any way. > > Which brings us to an even trickier question: what happens if a pwm output > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > So when it's enabled, it does not use the prescaler. > But! what happens if we now set that output to a different duty cycle? > > Example: > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > fail? no, because it's not actually using the period (it's full on) > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > fail? no, because it's not actually using the period (it's full on) > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > fail? no, because only output 1 is using the prescaler > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > fail? no, because output 2 is not changing the prescaler > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > fail? yes, because output 2 is changing prescaler and it's already in use > > IMHO all this can get very complicated and tricky. Is this really that complicated? I sounds to me like the only thing that you need is to have some sort of usage count for the prescaler. Whenever you want to use the prescaler you check that usage count. If it is zero, then you can just set it to whatever you need. If it isn't zero, that means somebody else is already using it and you can't change it, which means you have to check if you're trying to request the value that's already set. If so, you can succeed, but otherwise you'll have to fail. > We can of course make this much simpler by assumung that gpio or on/off pwms > are actually using the prescaler. But then we'd be limiting this chip's > functionality. Yeah, this is obviously much simpler, but the cost is a bit high, in my opinion. I'm fine with this alternative if there aren't any use-cases where multiple outputs are actually used. Thierry
On Tue, Dec 8, 2020 at 11:57 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > Is this really that complicated? I sounds to me like the only thing that > you need is to have some sort of usage count for the prescaler. Whenever > you want to use the prescaler you check that usage count. If it is zero, > then you can just set it to whatever you need. If it isn't zero, that > means somebody else is already using it and you can't change it, which > means you have to check if you're trying to request the value that's > already set. If so, you can succeed, but otherwise you'll have to fail. +1 I think your suggestion is an elegant solution to get the required behaviour. One possible complication is synchronization. The sysfs interface has a lock protecting against concurrent pwm_apply() calls. But the in-kernel API (e.g. pwm_apply_state()) doesn't seem to. This is not normally a problem when pwm bits are strictly separated. But in this case we have shared state (prescale value and use count), so we probably need to protect pwm_apply() with a mutex? Not sure if it is currently possible *in practice* for two regulator consumer drivers to call pwm_apply() from different threads. But Linux is slowly moving towards asynchronous probing. Uwe and Thierry, what is your opinion? Do you think we need to worry about synchronization?
Hello Thierry, hello Sven, On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > If this is already in the old code, this probably warrants a separate > > > fix, and yes, I consider this a severe bug. (Consider one channel > > > driving a motor and reconfiguring an LED modifies the motor's speed.) > > > > > > > I think you are 100% correct, this would be a severe bug. I have only used > > this chip to drive LEDs, where the actual period is not that important. But > > for motor control, it's a different story. > > > > Basically you are suggesting: the period (prescaler) can only be changed iff > > its use-count is 1. > > > > This however brings up a whole load of additional questions: consider the case > > where the chip outputs are also used in gpio mode. the gpio functionality > > only sets "full on" and "full off" bits. On a scope, a gpio output will look > > identical, no matter the value of the period. So when a gpio output is in use, > > does it increment the prescaler use-count ? > > > > Example: > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > 2. output 2: set led mode (full-on bit set) > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz) > > > > Do we have to make (3) fail? I would say no: although output 2 is in use, > > it's not actually using the prescaler. Changing prescale won't modify > > output 2 in any way. > > > > Which brings us to an even trickier question: what happens if a pwm output > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > So when it's enabled, it does not use the prescaler. > > But! what happens if we now set that output to a different duty cycle? > > > > Example: > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > fail? no, because it's not actually using the period (it's full on) > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > fail? no, because it's not actually using the period (it's full on) > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > fail? no, because only output 1 is using the prescaler > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > fail? no, because output 2 is not changing the prescaler > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > IMHO all this can get very complicated and tricky. > > Is this really that complicated? I think it is. > I sounds to me like the only thing that you need is to have some sort > of usage count for the prescaler. Whenever you want to use the > prescaler you check that usage count. If it is zero, then you can just > set it to whatever you need. If it isn't zero, that means somebody > else is already using it and you can't change it, which means you have > to check if you're trying to request the value that's already set. If > so, you can succeed, but otherwise you'll have to fail. With this abstraction Sven's questions are changed to: Does a PWM that runs at 0% or 100% use the prescaler? If yes, you limit the possibilities of the brother channels. And if not, it will not be possible to go to a 50% relative duty cycle while retaining the period. Both sounds not optimal. > > We can of course make this much simpler by assumung that gpio or on/off pwms > > are actually using the prescaler. But then we'd be limiting this chip's > > functionality. > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > opinion. I'm fine with this alternative if there aren't any use-cases > where multiple outputs are actually used. This metric is wishy-washy; of course you can construct a use-case. I'd still go for this simpler option and assume the prescaler used if the PWM runs at 0% or 100%, but not when it is a GPIO. Best regards Uwe
Hello Sven, On Tue, Dec 08, 2020 at 01:15:10PM -0500, Sven Van Asbroeck wrote: > On Tue, Dec 8, 2020 at 11:57 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > > Is this really that complicated? I sounds to me like the only thing that > > you need is to have some sort of usage count for the prescaler. Whenever > > you want to use the prescaler you check that usage count. If it is zero, > > then you can just set it to whatever you need. If it isn't zero, that > > means somebody else is already using it and you can't change it, which > > means you have to check if you're trying to request the value that's > > already set. If so, you can succeed, but otherwise you'll have to fail. > > +1 > I think your suggestion is an elegant solution to get the required behaviour. > > One possible complication is synchronization. The sysfs interface has a lock > protecting against concurrent pwm_apply() calls. But the in-kernel > API (e.g. pwm_apply_state()) doesn't seem to. This is not normally a problem > when pwm bits are strictly separated. But in this case we have shared state > (prescale value and use count), so we probably need to protect pwm_apply() > with a mutex? Right, you need a lock. You can look at pwm-imx-tpm.c which has a similar limitation. > Not sure if it is currently possible *in practice* for two regulator consumer > drivers to call pwm_apply() from different threads. But Linux is slowly moving > towards asynchronous probing. You must assume that there is concurrent access to different channels of your hardware. Best regards Uwe
Hi everyone, On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote: > Hello Thierry, hello Sven, > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > If this is already in the old code, this probably warrants a separate > > > > fix, and yes, I consider this a severe bug. (Consider one channel > > > > driving a motor and reconfiguring an LED modifies the motor's speed.) > > > > > > > > > > I think you are 100% correct, this would be a severe bug. I have only used > > > this chip to drive LEDs, where the actual period is not that important. But > > > for motor control, it's a different story. > > > > > > Basically you are suggesting: the period (prescaler) can only be changed iff > > > its use-count is 1. > > > > > > This however brings up a whole load of additional questions: consider the case > > > where the chip outputs are also used in gpio mode. the gpio functionality > > > only sets "full on" and "full off" bits. On a scope, a gpio output will look > > > identical, no matter the value of the period. So when a gpio output is in use, > > > does it increment the prescaler use-count ? > > > > > > Example: > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > 2. output 2: set led mode (full-on bit set) > > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz) > > > > > > Do we have to make (3) fail? I would say no: although output 2 is in use, > > > it's not actually using the prescaler. Changing prescale won't modify > > > output 2 in any way. > > > > > > Which brings us to an even trickier question: what happens if a pwm output > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > > So when it's enabled, it does not use the prescaler. > > > But! what happens if we now set that output to a different duty cycle? > > > > > > Example: > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > > fail? no, because it's not actually using the period (it's full on) > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > > fail? no, because it's not actually using the period (it's full on) > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > fail? no, because only output 1 is using the prescaler > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > fail? no, because output 2 is not changing the prescaler > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > > > IMHO all this can get very complicated and tricky. > > > > Is this really that complicated? > > I think it is. > > > I sounds to me like the only thing that you need is to have some sort > > of usage count for the prescaler. Whenever you want to use the > > prescaler you check that usage count. If it is zero, then you can just > > set it to whatever you need. If it isn't zero, that means somebody > > else is already using it and you can't change it, which means you have > > to check if you're trying to request the value that's already set. If > > so, you can succeed, but otherwise you'll have to fail. > > With this abstraction Sven's questions are changed to: > > Does a PWM that runs at 0% or 100% use the prescaler? > > If yes, you limit the possibilities of the brother channels. And if not, > it will not be possible to go to a 50% relative duty cycle while > retaining the period. Both sounds not optimal. In my opinion, limiting the possibilities of brother channels is preferrable to introducing another restriction: Not being able to reconfigure a duty cycle from 0%/100% to something else while keeping the previously set period. Better deny the period change in the first place, even if the duty cycle is 0% or 100%. > > > > We can of course make this much simpler by assumung that gpio or on/off pwms > > > are actually using the prescaler. But then we'd be limiting this chip's > > > functionality. > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > > opinion. I'm fine with this alternative if there aren't any use-cases > > where multiple outputs are actually used. > > This metric is wishy-washy; of course you can construct a use-case. I'd > still go for this simpler option and assume the prescaler used if the > PWM runs at 0% or 100%, but not when it is a GPIO. I'd also prefer this solution. Thanks, Clemens
On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote: > Hello Thierry, hello Sven, > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > > On Tue, Dec 8, 2020 at 4:10 AM Uwe Kleine-König > > > <u.kleine-koenig@pengutronix.de> wrote: > > > > > > > > If this is already in the old code, this probably warrants a separate > > > > fix, and yes, I consider this a severe bug. (Consider one channel > > > > driving a motor and reconfiguring an LED modifies the motor's speed.) > > > > > > > > > > I think you are 100% correct, this would be a severe bug. I have only used > > > this chip to drive LEDs, where the actual period is not that important. But > > > for motor control, it's a different story. > > > > > > Basically you are suggesting: the period (prescaler) can only be changed iff > > > its use-count is 1. > > > > > > This however brings up a whole load of additional questions: consider the case > > > where the chip outputs are also used in gpio mode. the gpio functionality > > > only sets "full on" and "full off" bits. On a scope, a gpio output will look > > > identical, no matter the value of the period. So when a gpio output is in use, > > > does it increment the prescaler use-count ? > > > > > > Example: > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > 2. output 2: set led mode (full-on bit set) > > > 3. output 1: change period(enabled=true, duty_cycle=50%, period=1/100Hz) > > > > > > Do we have to make (3) fail? I would say no: although output 2 is in use, > > > it's not actually using the prescaler. Changing prescale won't modify > > > output 2 in any way. > > > > > > Which brings us to an even trickier question: what happens if a pwm output > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > > So when it's enabled, it does not use the prescaler. > > > But! what happens if we now set that output to a different duty cycle? > > > > > > Example: > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > > fail? no, because it's not actually using the period (it's full on) > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > > fail? no, because it's not actually using the period (it's full on) > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > fail? no, because only output 1 is using the prescaler > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > fail? no, because output 2 is not changing the prescaler > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > > > IMHO all this can get very complicated and tricky. > > > > Is this really that complicated? > > I think it is. Care to specify what exactly is complicated about it? You're just saying that you don't like the restrictions that this implements, but there's really nothing we can do about that because the hardware just doesn't give you that flexibility. > > I sounds to me like the only thing that you need is to have some sort > > of usage count for the prescaler. Whenever you want to use the > > prescaler you check that usage count. If it is zero, then you can just > > set it to whatever you need. If it isn't zero, that means somebody > > else is already using it and you can't change it, which means you have > > to check if you're trying to request the value that's already set. If > > so, you can succeed, but otherwise you'll have to fail. > > With this abstraction Sven's questions are changed to: > > Does a PWM that runs at 0% or 100% use the prescaler? > > If yes, you limit the possibilities of the brother channels. And if not, > it will not be possible to go to a 50% relative duty cycle while > retaining the period. Both sounds not optimal. Again, this is a restriction imposed by the hardware design and there's nothing in software that we can do about that. The only thing I proposed was a simple way to detect the circumstances and make sure we can deal with it. And that's obviously subject to the kind of policy we want to implement. I don't think it's necessarily a bad thing to give people the most flexibility. If they know that one PWM channel is only ever going to be full-on/full-off, then they can still use that other channel in whatever way they want. If, on the other hand, we assume that the prescaler is always going to be used we limit the flexibility even if we don't necessarily have to. Obviously if you want to use both channels at partial duty-cycles there isn't much you can do and you really have to make sure they both run at the same frequency/period. But that's usually something that you can deal with by just choosing a period that works for both. And if that's not possible, well, then you better just use a different PWM controller to begin with, because you just can't make it work. > > > We can of course make this much simpler by assumung that gpio or on/off pwms > > > are actually using the prescaler. But then we'd be limiting this chip's > > > functionality. > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > > opinion. I'm fine with this alternative if there aren't any use-cases > > where multiple outputs are actually used. > > This metric is wishy-washy; of course you can construct a use-case. I'd > still go for this simpler option and assume the prescaler used if the > PWM runs at 0% or 100%, but not when it is a GPIO. I don't understand what you're saying here. On one hand you seem to object to what I was saying, but then you agree with it? And I'm not asking anyone to make up any artificial use-case. What I'm saying is that if there aren't any existing use-cases that would break if we assume a pre-scaler is used for full-on/full-off, then I'm okay with making that assumption and simplifying the driver. If there were use-cases, then that assumption would break existing users and that's not something I'm willing to accept. Anything wrong with that metric in your opinion? Thierry
Hello Thierry, On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote: > On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote: > > Hello Thierry, hello Sven, > > > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > > > Which brings us to an even trickier question: what happens if a pwm output > > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > > > So when it's enabled, it does not use the prescaler. > > > > But! what happens if we now set that output to a different duty cycle? > > > > > > > > Example: > > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > > > fail? no, because it's not actually using the period (it's full on) > > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > > > fail? no, because it's not actually using the period (it's full on) > > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > fail? no, because only output 1 is using the prescaler > > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > fail? no, because output 2 is not changing the prescaler > > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > > > > > IMHO all this can get very complicated and tricky. > > > > > > Is this really that complicated? > > > > I think it is. > > Care to specify what exactly is complicated about it? You're just saying > that you don't like the restrictions that this implements, but there's > really nothing we can do about that because the hardware just doesn't > give you that flexibility. The complicated thing is to chose how to map the hardware imposed limitations to the consumers of the two (or more?) channels. And the problem is there is no golden way that is objectively better than all others. > > > I sounds to me like the only thing that you need is to have some sort > > > of usage count for the prescaler. Whenever you want to use the > > > prescaler you check that usage count. If it is zero, then you can just > > > set it to whatever you need. If it isn't zero, that means somebody > > > else is already using it and you can't change it, which means you have > > > to check if you're trying to request the value that's already set. If > > > so, you can succeed, but otherwise you'll have to fail. > > > > With this abstraction Sven's questions are changed to: > > > > Does a PWM that runs at 0% or 100% use the prescaler? > > > > If yes, you limit the possibilities of the brother channels. And if not, > > it will not be possible to go to a 50% relative duty cycle while > > retaining the period. Both sounds not optimal. > > Again, this is a restriction imposed by the hardware design and there's > nothing in software that we can do about that. The only thing I proposed > was a simple way to detect the circumstances and make sure we can deal > with it. The point I want to make is, that with the usage counter you suggested you just shifted the problem and didn't solve it. I agree we need a usage counter, but you still have to think about how you want to answer the original questions by Sven. Depending on that you have to consider a channel running at 0% or 100% a user, or not. (Or the other way round: You select a policy if you consider 0% and 100% a use and implicitly answer the questions with it.) > And that's obviously subject to the kind of policy we want to implement. > I don't think it's necessarily a bad thing to give people the most > flexibility. If they know that one PWM channel is only ever going to be > full-on/full-off, then they can still use that other channel in whatever > way they want. If, on the other hand, we assume that the prescaler is > always going to be used we limit the flexibility even if we don't > necessarily have to. I think we agree here, just with different words. The only thing I doubt is: You wrote: "If they know $X, then they can still use that other channel in whatever way they want." Who is "they"? How can they know that $X is valid for someone else, or anyone else? Or is it enough that "they" know this about their own use? Now Clemens wants to improve the driver, does he need to consider "them" in the mainline driver implementation? If Clemens chooses one way or the other, will there be someone who then will produce a use case contradicting the implemented policy? How will you (or who will then) decide which use-case is more important? > Obviously if you want to use both channels at partial duty-cycles there > isn't much you can do and you really have to make sure they both run at > the same frequency/period. But that's usually something that you can > deal with by just choosing a period that works for both. > > And if that's not possible, well, then you better just use a different > PWM controller to begin with, because you just can't make it work. Yes. > > > > We can of course make this much simpler by assumung that gpio or on/off pwms > > > > are actually using the prescaler. But then we'd be limiting this chip's > > > > functionality. > > > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > > > opinion. I'm fine with this alternative if there aren't any use-cases > > > where multiple outputs are actually used. > > > > This metric is wishy-washy; of course you can construct a use-case. I'd > > still go for this simpler option and assume the prescaler used if the > > PWM runs at 0% or 100%, but not when it is a GPIO. > > I don't understand what you're saying here. On one hand you seem to > object to what I was saying, but then you agree with it? > > And I'm not asking anyone to make up any artificial use-case. What I'm > saying is that if there aren't any existing use-cases that would break > if we assume a pre-scaler is used for full-on/full-off, then I'm okay > with making that assumption and simplifying the driver. If there were > use-cases, then that assumption would break existing users and that's > not something I'm willing to accept. > > Anything wrong with that metric in your opinion? The part I called wishy-washy is: "[....] if there aren't any use-cases where [...]". Who should decide what are (relevant) use-cases? We already agreed above that we talk about a hardware that doesn't allow us to consider it consisting of 2 independent channels and so we somehow have to limit their capabilities exposed by the PWM API. And whatever compromise we make, it's not hard to find a more or less realistic use case where the compromise hurts. So my interpretation of your words are: "I'm fine with this alternative if $somethingimpossible" which is not helpful. So yes, there is something wrong with your metric because it's IMHO impossible for a driver author to actually use it. Best regards Uwe
On Thu, Dec 10, 2020 at 10:01:24AM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote: > > On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote: > > > Hello Thierry, hello Sven, > > > > > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > > > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > > > > Which brings us to an even trickier question: what happens if a pwm output > > > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > > > > So when it's enabled, it does not use the prescaler. > > > > > But! what happens if we now set that output to a different duty cycle? > > > > > > > > > > Example: > > > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > > > > fail? no, because it's not actually using the period (it's full on) > > > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > > > > fail? no, because it's not actually using the period (it's full on) > > > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > > fail? no, because only output 1 is using the prescaler > > > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > > fail? no, because output 2 is not changing the prescaler > > > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > > > > > > > IMHO all this can get very complicated and tricky. > > > > > > > > Is this really that complicated? > > > > > > I think it is. > > > > Care to specify what exactly is complicated about it? You're just saying > > that you don't like the restrictions that this implements, but there's > > really nothing we can do about that because the hardware just doesn't > > give you that flexibility. > > The complicated thing is to chose how to map the hardware imposed > limitations to the consumers of the two (or more?) channels. And the > problem is there is no golden way that is objectively better than all > others. > > > > > I sounds to me like the only thing that you need is to have some sort > > > > of usage count for the prescaler. Whenever you want to use the > > > > prescaler you check that usage count. If it is zero, then you can just > > > > set it to whatever you need. If it isn't zero, that means somebody > > > > else is already using it and you can't change it, which means you have > > > > to check if you're trying to request the value that's already set. If > > > > so, you can succeed, but otherwise you'll have to fail. > > > > > > With this abstraction Sven's questions are changed to: > > > > > > Does a PWM that runs at 0% or 100% use the prescaler? > > > > > > If yes, you limit the possibilities of the brother channels. And if not, > > > it will not be possible to go to a 50% relative duty cycle while > > > retaining the period. Both sounds not optimal. > > > > Again, this is a restriction imposed by the hardware design and there's > > nothing in software that we can do about that. The only thing I proposed > > was a simple way to detect the circumstances and make sure we can deal > > with it. > > The point I want to make is, that with the usage counter you suggested > you just shifted the problem and didn't solve it. I agree we need a > usage counter, but you still have to think about how you want to answer > the original questions by Sven. Depending on that you have to > consider a channel running at 0% or 100% a user, or not. (Or the other > way round: You select a policy if you consider 0% and 100% a use and > implicitly answer the questions with it.) The way I see it, and I think this matches what was said earlier, we have two options: 1) Use two channels as proper PWMs. In that case, if they happen to run at the same period, they are free to choose the duty-cycle and can be treated as independent PWM channels. If the run at different periods, one of them can only be used as either full-on or full-off PWM. It would still be a PWM, but just with additional restrictions on the duty cycle. 2) Use one channel as full PWM and the other channel as GPIO. 2) is a subset of 1) because we can implement 2) using 1) by treating the full-on/full-off PWM as a GPIO pin. In my opinion, that makes the first policy better because it is more flexible. > > And that's obviously subject to the kind of policy we want to implement. > > I don't think it's necessarily a bad thing to give people the most > > flexibility. If they know that one PWM channel is only ever going to be > > full-on/full-off, then they can still use that other channel in whatever > > way they want. If, on the other hand, we assume that the prescaler is > > always going to be used we limit the flexibility even if we don't > > necessarily have to. > > I think we agree here, just with different words. The only thing I doubt > is: You wrote: "If they know $X, then they can still use that other > channel in whatever way they want." Who is "they"? How can they know > that $X is valid for someone else, or anyone else? Or is it enough that > "they" know this about their own use? Now Clemens wants to improve the > driver, does he need to consider "them" in the mainline driver > implementation? If Clemens chooses one way or the other, will there be > someone who then will produce a use case contradicting the implemented > policy? How will you (or who will then) decide which use-case is more > important? "They" refers to whoever writes the DT. Given the hardware restrictions, the use-cases are already limited, so you are not going to get into a situation where option 1) wouldn't work for any *supported* use-cases. You can obviously always come up with a use-case where it wouldn't work, but if that's not supported by the hardware, then obviously there's not a thing we can do about it, as we already concluded. However, if you know that your hardware is restricted in this way, then the hardware designers will already have ensured that the supported use- cases don't conflict with the hardware. So if for example you want to support both a PWM fan and an LED with the two outputs, then that's going to work because you can select a period that works for both and run them both at the same period but at different duty cycles. If you have some other setup where one use-case requires the PWM period to be adjustable, then by definition with this hardware the second channel can't be required to be adjustable as well. In that case you can not rely on the second channel to be anything but full-on/full-off. But that's still enough to implement a basic LED, so that's what the board might support. Now you can obviously implement a simple LED using either a GPIO or a PWM, so it doesn't really matter in that case which policy we choose here. However, a GPIO is a special case of a PWM, so I think modelling the second output as a full PWM is better policy because it gives you a tiny bit more flexibility. So what I'm saying is that the hardware already dictates the set of possible use-cases, so if anyone is going to use this in a way that isn't supported, then it's okay to fail. > > Obviously if you want to use both channels at partial duty-cycles there > > isn't much you can do and you really have to make sure they both run at > > the same frequency/period. But that's usually something that you can > > deal with by just choosing a period that works for both. > > > > And if that's not possible, well, then you better just use a different > > PWM controller to begin with, because you just can't make it work. > > Yes. > > > > > > We can of course make this much simpler by assumung that gpio or on/off pwms > > > > > are actually using the prescaler. But then we'd be limiting this chip's > > > > > functionality. > > > > > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > > > > opinion. I'm fine with this alternative if there aren't any use-cases > > > > where multiple outputs are actually used. > > > > > > This metric is wishy-washy; of course you can construct a use-case. I'd > > > still go for this simpler option and assume the prescaler used if the > > > PWM runs at 0% or 100%, but not when it is a GPIO. > > > > I don't understand what you're saying here. On one hand you seem to > > object to what I was saying, but then you agree with it? > > > > And I'm not asking anyone to make up any artificial use-case. What I'm > > saying is that if there aren't any existing use-cases that would break > > if we assume a pre-scaler is used for full-on/full-off, then I'm okay > > with making that assumption and simplifying the driver. If there were > > use-cases, then that assumption would break existing users and that's > > not something I'm willing to accept. > > > > Anything wrong with that metric in your opinion? > > The part I called wishy-washy is: "[....] if there aren't any use-cases > where [...]". Who should decide what are (relevant) use-cases? We > already agreed above that we talk about a hardware that doesn't allow us > to consider it consisting of 2 independent channels and so we somehow > have to limit their capabilities exposed by the PWM API. And whatever > compromise we make, it's not hard to find a more or less realistic use > case where the compromise hurts. So my interpretation of your words are: > "I'm fine with this alternative if $somethingimpossible" which is not > helpful. > > So yes, there is something wrong with your metric because it's IMHO > impossible for a driver author to actually use it. Like I said, that's not what I was saying. I was merely saying that if there aren't any use-cases that current users rely on that would be broken by using this simpler implementation, then I'm okay with it, even if it's less flexible than a more complicated implementation. It should be possible to determine what the current users are by inspecting device trees present in the kernel. Anything outside the kernel isn't something we need to consider, as usual. Thierry
Hello Thierry, On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > Like I said, that's not what I was saying. I was merely saying that if > there aren't any use-cases that current users rely on that would be > broken by using this simpler implementation, then I'm okay with it, even > if it's less flexible than a more complicated implementation. It should > be possible to determine what the current users are by inspecting device > trees present in the kernel. Anything outside the kernel isn't something > we need to consider, as usual. If "users in mainline" is the criteria that's a word. So you agree we remove the following drivers?: - pwm-hibvt.c Last driver specific change in Feb 2019, no mainline user - pwm-sprd.c Last driver specific change in Aug 2019, no mainline user Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder if it makes sense to check the machine.dts files if some of the PMWs are completely unused. Do you consider status = "okay" a use that we have to retain even if the node has no phandle? Best regards Uwe
On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > On Thu, Dec 10, 2020 at 10:01:24AM +0100, Uwe Kleine-König wrote: > > Hello Thierry, > > > > On Wed, Dec 09, 2020 at 06:02:16PM +0100, Thierry Reding wrote: > > > On Tue, Dec 08, 2020 at 07:26:37PM +0100, Uwe Kleine-König wrote: > > > > Hello Thierry, hello Sven, > > > > > > > > On Tue, Dec 08, 2020 at 05:57:12PM +0100, Thierry Reding wrote: > > > > > On Tue, Dec 08, 2020 at 09:44:42AM -0500, Sven Van Asbroeck wrote: > > > > > > Which brings us to an even trickier question: what happens if a pwm output > > > > > > is set to 0% or 100% duty cycle? In that case, it'll behave like a gpio output. > > > > > > So when it's enabled, it does not use the prescaler. > > > > > > But! what happens if we now set that output to a different duty cycle? > > > > > > > > > > > > Example: > > > > > > 1. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > > > 2. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/400Hz) > > > > > > fail? no, because it's not actually using the period (it's full on) > > > > > > 3. output 2: set pwm mode (enabled=true, duty_cycle=100%, period=1/200Hz) > > > > > > fail? no, because it's not actually using the period (it's full on) > > > > > > 4. output 1: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > > > fail? no, because only output 1 is using the prescaler > > > > > > 5. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/400Hz) > > > > > > fail? no, because output 2 is not changing the prescaler > > > > > > 6. output 2: set pwm mode (enabled=true, duty_cycle=50%, period=1/200Hz) > > > > > > fail? yes, because output 2 is changing prescaler and it's already in use > > > > > > > > > > > > IMHO all this can get very complicated and tricky. > > > > > > > > > > Is this really that complicated? > > > > > > > > I think it is. > > > > > > Care to specify what exactly is complicated about it? You're just saying > > > that you don't like the restrictions that this implements, but there's > > > really nothing we can do about that because the hardware just doesn't > > > give you that flexibility. > > > > The complicated thing is to chose how to map the hardware imposed > > limitations to the consumers of the two (or more?) channels. And the > > problem is there is no golden way that is objectively better than all > > others. > > > > > > > I sounds to me like the only thing that you need is to have some sort > > > > > of usage count for the prescaler. Whenever you want to use the > > > > > prescaler you check that usage count. If it is zero, then you can just > > > > > set it to whatever you need. If it isn't zero, that means somebody > > > > > else is already using it and you can't change it, which means you have > > > > > to check if you're trying to request the value that's already set. If > > > > > so, you can succeed, but otherwise you'll have to fail. > > > > > > > > With this abstraction Sven's questions are changed to: > > > > > > > > Does a PWM that runs at 0% or 100% use the prescaler? > > > > > > > > If yes, you limit the possibilities of the brother channels. And if not, > > > > it will not be possible to go to a 50% relative duty cycle while > > > > retaining the period. Both sounds not optimal. > > > > > > Again, this is a restriction imposed by the hardware design and there's > > > nothing in software that we can do about that. The only thing I proposed > > > was a simple way to detect the circumstances and make sure we can deal > > > with it. > > > > The point I want to make is, that with the usage counter you suggested > > you just shifted the problem and didn't solve it. I agree we need a > > usage counter, but you still have to think about how you want to answer > > the original questions by Sven. Depending on that you have to > > consider a channel running at 0% or 100% a user, or not. (Or the other > > way round: You select a policy if you consider 0% and 100% a use and > > implicitly answer the questions with it.) > > The way I see it, and I think this matches what was said earlier, we > have two options: > > 1) Use two channels as proper PWMs. In that case, if they happen to > run at the same period, they are free to choose the duty-cycle and > can be treated as independent PWM channels. If the run at different > periods, one of them can only be used as either full-on or full-off > PWM. It would still be a PWM, but just with additional restrictions > on the duty cycle. > > 2) Use one channel as full PWM and the other channel as GPIO. > > 2) is a subset of 1) because we can implement 2) using 1) by treating > the full-on/full-off PWM as a GPIO pin. In my opinion, that makes the > first policy better because it is more flexible. > > > > And that's obviously subject to the kind of policy we want to implement. > > > I don't think it's necessarily a bad thing to give people the most > > > flexibility. If they know that one PWM channel is only ever going to be > > > full-on/full-off, then they can still use that other channel in whatever > > > way they want. If, on the other hand, we assume that the prescaler is > > > always going to be used we limit the flexibility even if we don't > > > necessarily have to. > > > > I think we agree here, just with different words. The only thing I doubt > > is: You wrote: "If they know $X, then they can still use that other > > channel in whatever way they want." Who is "they"? How can they know > > that $X is valid for someone else, or anyone else? Or is it enough that > > "they" know this about their own use? Now Clemens wants to improve the > > driver, does he need to consider "them" in the mainline driver > > implementation? If Clemens chooses one way or the other, will there be > > someone who then will produce a use case contradicting the implemented > > policy? How will you (or who will then) decide which use-case is more > > important? > > "They" refers to whoever writes the DT. Given the hardware restrictions, > the use-cases are already limited, so you are not going to get into a > situation where option 1) wouldn't work for any *supported* use-cases. > You can obviously always come up with a use-case where it wouldn't work, > but if that's not supported by the hardware, then obviously there's not > a thing we can do about it, as we already concluded. > > However, if you know that your hardware is restricted in this way, then > the hardware designers will already have ensured that the supported use- > cases don't conflict with the hardware. > > So if for example you want to support both a PWM fan and an LED with the > two outputs, then that's going to work because you can select a period > that works for both and run them both at the same period but at > different duty cycles. > > If you have some other setup where one use-case requires the PWM period > to be adjustable, then by definition with this hardware the second > channel can't be required to be adjustable as well. In that case you can > not rely on the second channel to be anything but full-on/full-off. But > that's still enough to implement a basic LED, so that's what the board > might support. > > Now you can obviously implement a simple LED using either a GPIO or a > PWM, so it doesn't really matter in that case which policy we choose > here. However, a GPIO is a special case of a PWM, so I think modelling > the second output as a full PWM is better policy because it gives you a > tiny bit more flexibility. > > So what I'm saying is that the hardware already dictates the set of > possible use-cases, so if anyone is going to use this in a way that > isn't supported, then it's okay to fail. > > > > Obviously if you want to use both channels at partial duty-cycles there > > > isn't much you can do and you really have to make sure they both run at > > > the same frequency/period. But that's usually something that you can > > > deal with by just choosing a period that works for both. > > > > > > And if that's not possible, well, then you better just use a different > > > PWM controller to begin with, because you just can't make it work. > > > > Yes. > > > > > > > > We can of course make this much simpler by assumung that gpio or on/off pwms > > > > > > are actually using the prescaler. But then we'd be limiting this chip's > > > > > > functionality. > > > > > > > > > > Yeah, this is obviously much simpler, but the cost is a bit high, in my > > > > > opinion. I'm fine with this alternative if there aren't any use-cases > > > > > where multiple outputs are actually used. > > > > > > > > This metric is wishy-washy; of course you can construct a use-case. I'd > > > > still go for this simpler option and assume the prescaler used if the > > > > PWM runs at 0% or 100%, but not when it is a GPIO. > > > > > > I don't understand what you're saying here. On one hand you seem to > > > object to what I was saying, but then you agree with it? > > > > > > And I'm not asking anyone to make up any artificial use-case. What I'm > > > saying is that if there aren't any existing use-cases that would break > > > if we assume a pre-scaler is used for full-on/full-off, then I'm okay > > > with making that assumption and simplifying the driver. If there were > > > use-cases, then that assumption would break existing users and that's > > > not something I'm willing to accept. > > > > > > Anything wrong with that metric in your opinion? > > > > The part I called wishy-washy is: "[....] if there aren't any use-cases > > where [...]". Who should decide what are (relevant) use-cases? We > > already agreed above that we talk about a hardware that doesn't allow us > > to consider it consisting of 2 independent channels and so we somehow > > have to limit their capabilities exposed by the PWM API. And whatever > > compromise we make, it's not hard to find a more or less realistic use > > case where the compromise hurts. So my interpretation of your words are: > > "I'm fine with this alternative if $somethingimpossible" which is not > > helpful. > > > > So yes, there is something wrong with your metric because it's IMHO > > impossible for a driver author to actually use it. > > Like I said, that's not what I was saying. I was merely saying that if > there aren't any use-cases that current users rely on that would be > broken by using this simpler implementation, then I'm okay with it, even > if it's less flexible than a more complicated implementation. It should > be possible to determine what the current users are by inspecting device > trees present in the kernel. Anything outside the kernel isn't something > we need to consider, as usual. I grepped for pca9685 and found no current in-kernel users of this driver. After reading your reasoning in this mail and rethinking the whole situation, I do no longer have any objections to the more complex solution. (Allowing 0% and 100% duty cycle channels with any period) I first thought it would be too confusing to block users from changing a duty cycle when in reality the period is the problem. However, if we log an error message, explaining that the periods have to match if duty cycles > 0 and < 100% are used, I think it's OK. Uwe, Sven: Do you have any objections? If not, I will prepare a v5 next week. Thanks, Clemens
On Thu, Dec 10, 2020 at 3:54 PM Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > After reading your reasoning in this mail and rethinking the whole > situation, I do no longer have any objections to the more complex > solution. (Allowing 0% and 100% duty cycle channels with any period) > > I first thought it would be too confusing to block users from changing a > duty cycle when in reality the period is the problem. > However, if we log an error message, explaining that the periods have to > match if duty cycles > 0 and < 100% are used, I think it's OK. > > Uwe, Sven: Do you have any objections? No objections, as long as the "fully flexible" solution doesn't get too complex. Complex code is hard to maintain and extend, but obviously that decision is for Thierry or Uwe to make. Thinking this through a little bit, I don't think the "fully flexible" solution has to be that complex. Keeping track of prescale-inuse may have to involve a bitmask and not a counter, such as here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pwm/pwm-pca9685.c?h=v5.10-rc7#n81 That way, channels can do set_bit() to mark that they use the prescaler, clear_bit() when giving up the prescaler, and if (bitmap_weight() <= 1) to check if they can make a prescale change. But this is theory - the only way to find out what's best, is to actually write the code... > If not, I will prepare a v5 next week. Looking forward to it, thank you !!
On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > > Like I said, that's not what I was saying. I was merely saying that if > > there aren't any use-cases that current users rely on that would be > > broken by using this simpler implementation, then I'm okay with it, even > > if it's less flexible than a more complicated implementation. It should > > be possible to determine what the current users are by inspecting device > > trees present in the kernel. Anything outside the kernel isn't something > > we need to consider, as usual. > > If "users in mainline" is the criteria that's a word. I didn't say "users in mainline", I said "use-cases". What I don't want to happen is for this change under discussion to break any existing use- cases of any existing users in the kernel. I said that we can determine what the existing users are by looking at which device trees use the compatible strings that the driver matches on. > So you agree we remove the following drivers?: > > - pwm-hibvt.c > Last driver specific change in Feb 2019, no mainline user > - pwm-sprd.c > Last driver specific change in Aug 2019, no mainline user No, that's an extrapolation of what I was saying above. Drivers with no apparent users are a separate topic, so don't conflate it with the issue at hand. While it's certainly unfortunate that these don't seem to be used, I see no reason why we should remove them. They don't create much of a maintenance burden, so I'm fine with keeping them in the hopes that users may still show up at some point. > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder > if it makes sense to check the machine.dts files if some of the PMWs are > completely unused. Do you consider status = "okay" a use that we have to > retain even if the node has no phandle? A PWM controller may be in use via sysfs even if it has no phandle. Thierry
Hello Thierry, On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote: > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote: > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > > > Like I said, that's not what I was saying. I was merely saying that if > > > there aren't any use-cases that current users rely on that would be > > > broken by using this simpler implementation, then I'm okay with it, even > > > if it's less flexible than a more complicated implementation. It should > > > be possible to determine what the current users are by inspecting device > > > trees present in the kernel. Anything outside the kernel isn't something > > > we need to consider, as usual. > > > > If "users in mainline" is the criteria that's a word. > > I didn't say "users in mainline", I said "use-cases". What I don't want > to happen is for this change under discussion to break any existing use- > cases of any existing users in the kernel. I said that we can determine > what the existing users are by looking at which device trees use the > compatible strings that the driver matches on. > > > So you agree we remove the following drivers?: > > > > - pwm-hibvt.c > > Last driver specific change in Feb 2019, no mainline user > > - pwm-sprd.c > > Last driver specific change in Aug 2019, no mainline user > > No, that's an extrapolation of what I was saying above. Drivers with no > apparent users are a separate topic, so don't conflate it with the issue > at hand. I cannot follow (and I think that's the problem between us and why those conflicts happen between us). For me it's a logic consequence of "Anything outside the kernel isn't something we need to consider, as usual." that drivers that are untouched for some period and have no mainline users can/should go away. (Is "extrapolation" as strong as "implication", or has it subjective interpretation added? My dictionary isn't accurate enough for that question.) But it seems there is something with my logic or you not saying exactly what you actually mean. (Did I miss any option? If yes it might be covered by a problem in my logic.) Having said that, even in the question at hand (i.e. what is the better compromise for mapping the inter-channel hardware limitations to software policy in the pac9685 driver) the idea "let's inspecting device trees present in the kernel" doesn't work, because for this driver there are none, too. (It might be used by a mainline machine via ACPI, but this is even less possible to consider for our judgements than a device tree with such a device and no user but the sysfs PWM interface.) > While it's certainly unfortunate that these don't seem to be used, I see > no reason why we should remove them. They don't create much of a > maintenance burden, so I'm fine with keeping them in the hopes that > users may still show up at some point. The problem I have with them is that I expect your voice of dissent when I find the time to improve the rounding behaviour of these drivers. My ultimate goal is to make the PWM framework a system where consumers can rely on a consistent behaviour of the API and a way to actually order what they need and get it. I'm not entirely sure we agree that we're not there yet. > > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder > > if it makes sense to check the machine.dts files if some of the PMWs are > > completely unused. Do you consider status = "okay" a use that we have to > > retain even if the node has no phandle? > > A PWM controller may be in use via sysfs even if it has no phandle. Yeah, I expected you will say that. (And I agree.) Best regards Uwe
On Fri, Dec 11, 2020 at 11:34:54AM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote: > > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote: > > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > > > > Like I said, that's not what I was saying. I was merely saying that if > > > > there aren't any use-cases that current users rely on that would be > > > > broken by using this simpler implementation, then I'm okay with it, even > > > > if it's less flexible than a more complicated implementation. It should > > > > be possible to determine what the current users are by inspecting device > > > > trees present in the kernel. Anything outside the kernel isn't something > > > > we need to consider, as usual. > > > > > > If "users in mainline" is the criteria that's a word. > > > > I didn't say "users in mainline", I said "use-cases". What I don't want > > to happen is for this change under discussion to break any existing use- > > cases of any existing users in the kernel. I said that we can determine > > what the existing users are by looking at which device trees use the > > compatible strings that the driver matches on. > > > > > So you agree we remove the following drivers?: > > > > > > - pwm-hibvt.c > > > Last driver specific change in Feb 2019, no mainline user > > > - pwm-sprd.c > > > Last driver specific change in Aug 2019, no mainline user > > > > No, that's an extrapolation of what I was saying above. Drivers with no > > apparent users are a separate topic, so don't conflate it with the issue > > at hand. > > I cannot follow (and I think that's the problem between us and why those > conflicts happen between us). For me it's a logic consequence of > "Anything outside the kernel isn't something we need to consider, as > usual." that drivers that are untouched for some period and have no > mainline users can/should go away. (Is "extrapolation" as strong as > "implication", or has it subjective interpretation added? My dictionary > isn't accurate enough for that question.) But it seems there is > something with my logic or you not saying exactly what you actually > mean. (Did I miss any option? If yes it might be covered by a problem in > my logic.) To me this is not as black and white as it seems to be for you. First I wasn't talking about unused drivers, but about use-cases that are not represented in mainline. Secondly I didn't say anything about removing support for use-cases that weren't in use upstream. All I said was that I didn't want any changes to regress existing use-cases. "Guessing" how that statement reflects on my thoughts about unused drivers is extrapolation. Your logic implication could've been correct, but in this case it wasn't because I consider a driver that was upstreamed to be part of the kernel, and people invested a fair amount of work to get it to that point, so I'm in no hurry to get rid of them. Instead, I prefer to give people the benefit of the doubt and assume that they had planned to get users upstream and for some reason just haven't gotten around to it. On the other hand, almost 18-24 months without activity is quite long. A compromise that works well for me is to keep drivers, even unused ones, as long as they're not getting in the way. > Having said that, even in the question at hand (i.e. what is the better > compromise for mapping the inter-channel hardware limitations to > software policy in the pac9685 driver) the idea "let's inspecting device > trees present in the kernel" doesn't work, because for this driver there > are none, too. (It might be used by a mainline machine via ACPI, but > this is even less possible to consider for our judgements than a device > tree with such a device and no user but the sysfs PWM interface.) Perhaps Clemens and Sven can shed some light into how this driver is being used. There clearly seem to be people interested in this driver, so why are there no consumers of this upstream. What's keeping people from upstreaming device trees that make use of this? > > While it's certainly unfortunate that these don't seem to be used, I see > > no reason why we should remove them. They don't create much of a > > maintenance burden, so I'm fine with keeping them in the hopes that > > users may still show up at some point. > > The problem I have with them is that I expect your voice of dissent when > I find the time to improve the rounding behaviour of these drivers. > My ultimate goal is to make the PWM framework a system where consumers > can rely on a consistent behaviour of the API and a way to actually > order what they need and get it. I'm not entirely sure we agree that > we're not there yet. I don't think we entirely agree that the framework is currently inconsistent. As you and others have shown in various threads, one way of rounding isn't necessarily always the best way. Some consumers expect a certain accuracy and don't care whether we over- or undershoot. Some controllers may give more accurate results when rounding up than when rounding down and for others rounding to nearest is the best option. A lot of consumers don't care all that much about rounding as long as the duty-cycle/period ratio is reasonably close. So there are a lot of variables in all of this and I don't think we can fix the rules so that they work for everyone. That's why I think we need to stick with the status quo that does work for pretty much everyone so far by being not overly strict and then extend that if we have to (i.e. if we come across a use-case where the current status quo doesn't work). > > > Most PWMs are added to cpu.dtsi files with status = "disabled", I wonder > > > if it makes sense to check the machine.dts files if some of the PMWs are > > > completely unused. Do you consider status = "okay" a use that we have to > > > retain even if the node has no phandle? > > > > A PWM controller may be in use via sysfs even if it has no phandle. > > Yeah, I expected you will say that. (And I agree.) sysfs on the PWM side does complicate things a little bit, but on the other hand it's not very different from other consumers. For any change at the API level (such as adding rounding modes, if we have to) we just need to make sure this can be represented using the sysfs interface as well. And by the way, as an additional argument against removing seemingly unused drivers: it's technically possible to instantiate a PCA9685 from userspace using sysfs, and there might be people relying on this to get their job done. Again, there are many shades of grey here and lots of unknowns, and extrapolation doesn't work very well under those circumstances. Thierry
On Mon, Dec 14, 2020 at 03:28:24PM +0100, Thierry Reding wrote: > On Fri, Dec 11, 2020 at 11:34:54AM +0100, Uwe Kleine-König wrote: > > Hello Thierry, > > > > On Fri, Dec 11, 2020 at 09:33:55AM +0100, Thierry Reding wrote: > > > On Thu, Dec 10, 2020 at 09:39:26PM +0100, Uwe Kleine-König wrote: > > > > On Thu, Dec 10, 2020 at 06:10:45PM +0100, Thierry Reding wrote: > > > > > Like I said, that's not what I was saying. I was merely saying that if > > > > > there aren't any use-cases that current users rely on that would be > > > > > broken by using this simpler implementation, then I'm okay with it, even > > > > > if it's less flexible than a more complicated implementation. It should > > > > > be possible to determine what the current users are by inspecting device > > > > > trees present in the kernel. Anything outside the kernel isn't something > > > > > we need to consider, as usual. > > > > > > > > If "users in mainline" is the criteria that's a word. > > > > > > I didn't say "users in mainline", I said "use-cases". What I don't want > > > to happen is for this change under discussion to break any existing use- > > > cases of any existing users in the kernel. I said that we can determine > > > what the existing users are by looking at which device trees use the > > > compatible strings that the driver matches on. > > > > > > > So you agree we remove the following drivers?: > > > > > > > > - pwm-hibvt.c > > > > Last driver specific change in Feb 2019, no mainline user > > > > - pwm-sprd.c > > > > Last driver specific change in Aug 2019, no mainline user > > > > > > No, that's an extrapolation of what I was saying above. Drivers with no > > > apparent users are a separate topic, so don't conflate it with the issue > > > at hand. > > > > I cannot follow (and I think that's the problem between us and why those > > conflicts happen between us). For me it's a logic consequence of > > "Anything outside the kernel isn't something we need to consider, as > > usual." that drivers that are untouched for some period and have no > > mainline users can/should go away. (Is "extrapolation" as strong as > > "implication", or has it subjective interpretation added? My dictionary > > isn't accurate enough for that question.) But it seems there is > > something with my logic or you not saying exactly what you actually > > mean. (Did I miss any option? If yes it might be covered by a problem in > > my logic.) > > To me this is not as black and white as it seems to be for you. First I > wasn't talking about unused drivers, but about use-cases that are not > represented in mainline. Secondly I didn't say anything about removing > support for use-cases that weren't in use upstream. All I said was that > I didn't want any changes to regress existing use-cases. > > "Guessing" how that statement reflects on my thoughts about unused > drivers is extrapolation. Your logic implication could've been correct, > but in this case it wasn't because I consider a driver that was > upstreamed to be part of the kernel, and people invested a fair amount > of work to get it to that point, so I'm in no hurry to get rid of them. > Instead, I prefer to give people the benefit of the doubt and assume > that they had planned to get users upstream and for some reason just > haven't gotten around to it. > > On the other hand, almost 18-24 months without activity is quite long. A > compromise that works well for me is to keep drivers, even unused ones, > as long as they're not getting in the way. > > > Having said that, even in the question at hand (i.e. what is the better > > compromise for mapping the inter-channel hardware limitations to > > software policy in the pac9685 driver) the idea "let's inspecting device > > trees present in the kernel" doesn't work, because for this driver there > > are none, too. (It might be used by a mainline machine via ACPI, but > > this is even less possible to consider for our judgements than a device > > tree with such a device and no user but the sysfs PWM interface.) > > Perhaps Clemens and Sven can shed some light into how this driver is > being used. There clearly seem to be people interested in this driver, > so why are there no consumers of this upstream. What's keeping people > from upstreaming device trees that make use of this? Our DT using the pca9685 is for an embedded board within a product and that board within is not sold alone. That's the reason why I did not upstream it yet, because I did not know if it is acceptable to upstream DTs for boards that are not really of great use to other people, because they can't (easily) get the hardware, unless they buy a big beer dispensing system. If that's not an issue then I am willig to upstream it of course. Clemens
Hi Thierry, On Mon, Dec 14, 2020 at 9:28 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > Perhaps Clemens and Sven can shed some light into how this driver is > being used. There clearly seem to be people interested in this driver, > so why are there no consumers of this upstream. What's keeping people > from upstreaming device trees that make use of this? > There are many reasons why a driver may not appear in the devicetree. In my specific case, I backported the PCA9685 driver to a 4.1 Android vendor kernel. This is too far behind to upstream. Also, the company regards the devicetree as a trade secret, which it is entitled to do, as devicetrees tend to be dual-licensed (GPL and MIT). More generally, I believe that the PCA9685 is quite popular in the Raspberry Pi world. Raspi devicetrees are not part of mainline, for various reasons that we don't need to get into here. Example: https://learn.adafruit.com/adafruit-16-channel-servo-driver-with-raspberry-pi
Hi Thierry, On Mon, Dec 14, 2020 at 9:28 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > > Perhaps Clemens and Sven can shed some light into how this driver is > being used. There clearly seem to be people interested in this driver, > so why are there no consumers of this upstream. What's keeping people > from upstreaming device trees that make use of this? > Also, there's this section in the driver: #ifdef CONFIG_ACPI static const struct acpi_device_id pca9685_acpi_ids[] = { { "INT3492", 0 }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(acpi, pca9685_acpi_ids); #endif Which means there might be arm64 or intel devices out there that define presence of this chip via an ACPI tree. Which exists only inside bioses and would not show up in any devicetree.
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 4a55dc18656c..0425e0bcb81e 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -47,11 +47,11 @@ #define PCA9685_ALL_LED_OFF_H 0xFD #define PCA9685_PRESCALE 0xFE +#define PCA9685_PRESCALE_DEF 0x1E /* => default frequency of ~200 Hz */ #define PCA9685_PRESCALE_MIN 0x03 /* => max. frequency of 1526 Hz */ #define PCA9685_PRESCALE_MAX 0xFF /* => min. frequency of 24 Hz */ #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 @@ -74,7 +74,6 @@ 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 +86,81 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip) return container_of(chip, struct pca9685, chip); } +static void pca9685_pwm_set_full_off(struct pca9685 *pca, int index, bool enable) +{ + unsigned int val = enable ? LED_FULL : 0; + + /* Note: The full OFF bit has the highest precedence */ + + if (index >= PCA9685_MAXCHAN) { + regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, val); + return; + } + regmap_update_bits(pca->regmap, LED_N_OFF_H(index), LED_FULL, val); +} + +static bool pca9685_pwm_is_full_off(struct pca9685 *pca, int index) +{ + unsigned int val = 0; + + if (index >= PCA9685_MAXCHAN) + return false; + + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); + return val & LED_FULL; +} + +static void pca9685_pwm_set_full_on(struct pca9685 *pca, int index, bool enable) +{ + unsigned int val = enable ? LED_FULL : 0; + + if (index >= PCA9685_MAXCHAN) { + regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, val); + return; + } + regmap_update_bits(pca->regmap, LED_N_ON_H(index), LED_FULL, val); +} + +static bool pca9685_pwm_is_full_on(struct pca9685 *pca, int index) +{ + unsigned int val = 0; + + if (index >= PCA9685_MAXCHAN) + return false; + + regmap_read(pca->regmap, LED_N_ON_H(index), &val); + return val & LED_FULL; +} + +static void pca9685_pwm_set_off_time(struct pca9685 *pca, int index, unsigned int off) +{ + int reg; + + /* Note: This function also clears the full OFF bit */ + + reg = index >= PCA9685_MAXCHAN ? + PCA9685_ALL_LED_OFF_L : LED_N_OFF_L(index); + regmap_write(pca->regmap, reg, off & 0xff); + + reg = index >= PCA9685_MAXCHAN ? + PCA9685_ALL_LED_OFF_H : LED_N_OFF_H(index); + regmap_write(pca->regmap, reg, (off >> 8) & 0xf); +} + +static unsigned int pca9685_pwm_off_time(struct pca9685 *pca, int index) +{ + unsigned int off, val = 0; + + if (index >= PCA9685_MAXCHAN) + return 0; + + regmap_read(pca->regmap, LED_N_OFF_H(index), &val); + off = (val & 0xf) << 8; + + regmap_read(pca->regmap, LED_N_OFF_L(index), &val); + return off | (val & 0xff); +} + #if IS_ENABLED(CONFIG_GPIOLIB) static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx) { @@ -138,34 +212,31 @@ 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_is_full_off(pca, offset); } 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); + if (value) { + pca9685_pwm_set_full_on(pca, offset, true); + /* Clear full OFF bit last */ + pca9685_pwm_set_full_off(pca, offset, false); + } else { + /* Set full OFF bit first */ + pca9685_pwm_set_full_off(pca, offset, true); + pca9685_pwm_set_full_on(pca, offset, false); + } } 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_full_off(pca, offset, true); pm_runtime_put(pca->chip.dev); pca9685_pwm_clear_inuse(pca, offset); } @@ -246,165 +317,98 @@ 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 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 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 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); + /* Calculate (chip-wide) period from prescale value */ + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * + (val + 1); - regmap_write(pca->regmap, reg, LED_FULL); + /* The (per-channel) polarity is fixed */ + state->polarity = PWM_POLARITY_NORMAL; - return 0; + 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; } - 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); + state->enabled = !pca9685_pwm_is_full_off(pca, pwm->hwpwm); - regmap_write(pca->regmap, reg, LED_FULL); - - return 0; + if (state->enabled && pca9685_pwm_is_full_on(pca, pwm->hwpwm)) { + state->duty_cycle = state->period; + return; } - 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; + duty = pca9685_pwm_off_time(pca, pwm->hwpwm) * state->period; + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; } -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) +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 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); + unsigned long long duty, prescale; + unsigned int val = 0; - regmap_write(pca->regmap, reg, 0); + if (state->polarity != PWM_POLARITY_NORMAL) + return -EOPNOTSUPP; - 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); - - /* - * 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); + prescale = DIV_ROUND_CLOSEST_ULL(PCA9685_OSC_CLOCK_MHZ * state->period, + PCA9685_COUNTER_RANGE * 1000) - 1; + if (prescale < PCA9685_PRESCALE_MIN || prescale > PCA9685_PRESCALE_MAX) { + dev_err(chip->dev, "prescaler not set: period out of bounds!\n"); + return -EINVAL; + } - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0); + 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); - return 0; -} + /* Change the chip-wide output frequency */ + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale); -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) -{ - struct pca9685 *pca = to_pca(chip); - unsigned int reg; + /* Wake the chip up */ + pca9685_set_sleep_mode(pca, false); + } - if (pwm->hwpwm >= PCA9685_MAXCHAN) - reg = PCA9685_ALL_LED_OFF_H; - else - reg = LED_N_OFF_H(pwm->hwpwm); + duty = PCA9685_COUNTER_RANGE * state->duty_cycle; + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period); - regmap_write(pca->regmap, reg, LED_FULL); + if (!state->enabled || duty < 1) { + /* Set full OFF bit first */ + pca9685_pwm_set_full_off(pca, pwm->hwpwm, true); + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); + return 0; + } - /* Clear the LED_OFF counter. */ - if (pwm->hwpwm >= PCA9685_MAXCHAN) - reg = PCA9685_ALL_LED_OFF_L; - else - reg = LED_N_OFF_L(pwm->hwpwm); + if (duty == PCA9685_COUNTER_RANGE) { + pca9685_pwm_set_full_on(pca, pwm->hwpwm, true); + /* Clear full OFF bit last */ + pca9685_pwm_set_full_off(pca, pwm->hwpwm, false); + return 0; + } - regmap_write(pca->regmap, reg, 0x0); + pca9685_pwm_set_off_time(pca, pwm->hwpwm, duty); + /* Clear full ON bit after OFF time was set */ + pca9685_pwm_set_full_on(pca, pwm->hwpwm, false); + return 0; } static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) @@ -422,15 +426,14 @@ 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_full_off(pca, pwm->hwpwm, true); 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, + .get_state = pca9685_pwm_get_state, + .apply = pca9685_pwm_apply, .request = pca9685_pwm_request, .free = pca9685_pwm_free, .owner = THIS_MODULE, @@ -461,7 +464,6 @@ static int pca9685_pwm_probe(struct i2c_client *client, ret); return ret; } - pca->period_ns = PCA9685_DEFAULT_PERIOD; i2c_set_clientdata(client, pca); @@ -505,14 +507,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 + * Always initialize with default prescale, but chip must be + * in sleep mode while changing prescaler. */ + pca9685_set_sleep_mode(pca, true); + regmap_write(pca->regmap, PCA9685_PRESCALE, PCA9685_PRESCALE_DEF); + pm_runtime_set_suspended(&client->dev); pm_runtime_enable(&client->dev); + if (!IS_ENABLED(CONFIG_PM)) { + /* Wake the chip up on non-PM environments */ + pca9685_set_sleep_mode(pca, false); + } + return 0; } @@ -524,7 +532,14 @@ static int pca9685_pwm_remove(struct i2c_client *client) ret = pwmchip_remove(&pca->chip); if (ret) return ret; + pm_runtime_disable(&client->dev); + + if (!IS_ENABLED(CONFIG_PM)) { + /* Put chip in sleep state on non-PM environments */ + pca9685_set_sleep_mode(pca, true); + } + return 0; }
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 - The prescale register is now set to the default value in probe. On systems without CONFIG_PM, the chip is woken up at probe time. 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. If one channel is reconfigured with new duty cycle and period, the others will keep the same relative duty cycle to period ratio as they had before, even though the per-chip / global frequency changed. (The PCA9685 has only one prescaler!) Note that although the datasheet mentions 200 Hz as default frequency when using the internal 25 MHz oscillator, the calculated period from the default prescaler register setting of 30 is 5079040ns. Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> --- 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 | 335 ++++++++++++++++++++------------------ 1 file changed, 175 insertions(+), 160 deletions(-)