| Message ID | 20210406164140.81423-2-clemens.gruber@pqgruber.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v7,1/8] pwm: pca9685: Switch to atomic API | expand |
On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote: > Implements .get_state to read-out the current hardware state. > > The hardware readout may return slightly different values than those > that were set in apply due to the limited range of possible prescale and > counter register values. > > Also note that although the datasheet mentions 200 Hz as default > frequency when using the internal 25 MHz oscillator, the calculated > period from the default prescaler register setting of 30 is 5079040ns. > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > --- > Changes since v6: > - Added a comment regarding the division (Suggested by Uwe) > - Rebased > > drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > index 5a2ce97e71fd..d4474c5ff96f 100644 > --- a/drivers/pwm/pwm-pca9685.c > +++ b/drivers/pwm/pwm-pca9685.c > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > return 0; > } > > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct pca9685 *pca = to_pca(chip); > + unsigned long long duty; > + unsigned int val = 0; > + > + /* Calculate (chip-wide) period from prescale value */ > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > + /* > + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000. > + * The following calculation is therefore only a multiplication > + * and we are not losing precision. > + */ > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > + (val + 1); > + > + /* The (per-channel) polarity is fixed */ > + state->polarity = PWM_POLARITY_NORMAL; > + > + if (pwm->hwpwm >= PCA9685_MAXCHAN) { > + /* > + * The "all LEDs" channel does not support HW readout > + * Return 0 and disabled for backwards compatibility > + */ > + state->duty_cycle = 0; > + state->enabled = false; > + return; > + } > + > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > + > + state->enabled = !!duty; > + if (!state->enabled) { > + state->duty_cycle = 0; > + return; > + } else if (duty == PCA9685_COUNTER_RANGE) { > + state->duty_cycle = state->period; > + return; > + } > + > + duty *= state->period; > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; Given that with duty = 0 the chip is still "on" and changing the duty will first complete the currently running period, I'd model duty=0 as enabled. This also simplifies the code a bit, to something like: state->enabled = true; duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE); (I'm using round-up here assuming apply uses round-down to get idempotency. In the current patch set state this is wrong however.) > +} > + > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > { > struct pca9685 *pca = to_pca(chip); Best regards Uwe
On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote: > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote: > > Implements .get_state to read-out the current hardware state. > > > > The hardware readout may return slightly different values than those > > that were set in apply due to the limited range of possible prescale and > > counter register values. > > > > Also note that although the datasheet mentions 200 Hz as default > > frequency when using the internal 25 MHz oscillator, the calculated > > period from the default prescaler register setting of 30 is 5079040ns. > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > --- > > Changes since v6: > > - Added a comment regarding the division (Suggested by Uwe) > > - Rebased > > > > drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > index 5a2ce97e71fd..d4474c5ff96f 100644 > > --- a/drivers/pwm/pwm-pca9685.c > > +++ b/drivers/pwm/pwm-pca9685.c > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > return 0; > > } > > > > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct pca9685 *pca = to_pca(chip); > > + unsigned long long duty; > > + unsigned int val = 0; > > + > > + /* Calculate (chip-wide) period from prescale value */ > > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > > + /* > > + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000. > > + * The following calculation is therefore only a multiplication > > + * and we are not losing precision. > > + */ > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > > + (val + 1); > > + > > + /* The (per-channel) polarity is fixed */ > > + state->polarity = PWM_POLARITY_NORMAL; > > + > > + if (pwm->hwpwm >= PCA9685_MAXCHAN) { > > + /* > > + * The "all LEDs" channel does not support HW readout > > + * Return 0 and disabled for backwards compatibility > > + */ > > + state->duty_cycle = 0; > > + state->enabled = false; > > + return; > > + } > > + > > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > > + > > + state->enabled = !!duty; > > + if (!state->enabled) { > > + state->duty_cycle = 0; > > + return; > > + } else if (duty == PCA9685_COUNTER_RANGE) { > > + state->duty_cycle = state->period; > > + return; > > + } > > + > > + duty *= state->period; > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; > > Given that with duty = 0 the chip is still "on" and changing the duty > will first complete the currently running period, I'd model duty=0 as > enabled. This also simplifies the code a bit, to something like: > > > state->enabled = true; > duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE); > > (I'm using round-up here assuming apply uses round-down to get > idempotency. In the current patch set state this is wrong however.) So, in your opinion, every requested PWM of the pca9685 should always be enabled by default (from the PWM core viewpoint) ? And this wouldn't break the following because pwm_get_state does not actually read out the hw state: pwm_get_state -> enabled=true duty=0 pwm_apply_state -> enabled =false duty=0 pwm_get_state -> enabled=false duty=0 Thanks, Clemens
On Wed, Apr 07, 2021 at 09:33:20AM +0200, Clemens Gruber wrote: > On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote: > > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote: > > > Implements .get_state to read-out the current hardware state. > > > > > > The hardware readout may return slightly different values than those > > > that were set in apply due to the limited range of possible prescale and > > > counter register values. > > > > > > Also note that although the datasheet mentions 200 Hz as default > > > frequency when using the internal 25 MHz oscillator, the calculated > > > period from the default prescaler register setting of 30 is 5079040ns. > > > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > > --- > > > Changes since v6: > > > - Added a comment regarding the division (Suggested by Uwe) > > > - Rebased > > > > > > drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 46 insertions(+) > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > index 5a2ce97e71fd..d4474c5ff96f 100644 > > > --- a/drivers/pwm/pwm-pca9685.c > > > +++ b/drivers/pwm/pwm-pca9685.c > > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > return 0; > > > } > > > > > > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > > + struct pwm_state *state) > > > +{ > > > + struct pca9685 *pca = to_pca(chip); > > > + unsigned long long duty; > > > + unsigned int val = 0; > > > + > > > + /* Calculate (chip-wide) period from prescale value */ > > > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > > > + /* > > > + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000. > > > + * The following calculation is therefore only a multiplication > > > + * and we are not losing precision. > > > + */ > > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > > > + (val + 1); > > > + > > > + /* The (per-channel) polarity is fixed */ > > > + state->polarity = PWM_POLARITY_NORMAL; > > > + > > > + if (pwm->hwpwm >= PCA9685_MAXCHAN) { > > > + /* > > > + * The "all LEDs" channel does not support HW readout > > > + * Return 0 and disabled for backwards compatibility > > > + */ > > > + state->duty_cycle = 0; > > > + state->enabled = false; > > > + return; > > > + } > > > + > > > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > > > + > > > + state->enabled = !!duty; > > > + if (!state->enabled) { > > > + state->duty_cycle = 0; > > > + return; > > > + } else if (duty == PCA9685_COUNTER_RANGE) { > > > + state->duty_cycle = state->period; > > > + return; > > > + } > > > + > > > + duty *= state->period; > > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; > > > > Given that with duty = 0 the chip is still "on" and changing the duty > > will first complete the currently running period, I'd model duty=0 as > > enabled. This also simplifies the code a bit, to something like: > > > > > > state->enabled = true; > > duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > > state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE); > > > > (I'm using round-up here assuming apply uses round-down to get > > idempotency. In the current patch set state this is wrong however.) > > So, in your opinion, every requested PWM of the pca9685 should always be > enabled by default (from the PWM core viewpoint) ? > > And this wouldn't break the following because pwm_get_state does not > actually read out the hw state: > pwm_get_state -> enabled=true duty=0 > pwm_apply_state -> enabled =false duty=0 > pwm_get_state -> enabled=false duty=0 I don't see any breakage here. Either there is none or I failed to grasp where you see a problem. Best regards Uwe
On Wed, Apr 07, 2021 at 11:09:43AM +0200, Uwe Kleine-König wrote: > On Wed, Apr 07, 2021 at 09:33:20AM +0200, Clemens Gruber wrote: > > On Wed, Apr 07, 2021 at 07:31:35AM +0200, Uwe Kleine-König wrote: > > > On Tue, Apr 06, 2021 at 06:41:34PM +0200, Clemens Gruber wrote: > > > > Implements .get_state to read-out the current hardware state. > > > > > > > > The hardware readout may return slightly different values than those > > > > that were set in apply due to the limited range of possible prescale and > > > > counter register values. > > > > > > > > Also note that although the datasheet mentions 200 Hz as default > > > > frequency when using the internal 25 MHz oscillator, the calculated > > > > period from the default prescaler register setting of 30 is 5079040ns. > > > > > > > > Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> > > > > --- > > > > Changes since v6: > > > > - Added a comment regarding the division (Suggested by Uwe) > > > > - Rebased > > > > > > > > drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 46 insertions(+) > > > > > > > > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c > > > > index 5a2ce97e71fd..d4474c5ff96f 100644 > > > > --- a/drivers/pwm/pwm-pca9685.c > > > > +++ b/drivers/pwm/pwm-pca9685.c > > > > @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > return 0; > > > > } > > > > > > > > +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + struct pwm_state *state) > > > > +{ > > > > + struct pca9685 *pca = to_pca(chip); > > > > + unsigned long long duty; > > > > + unsigned int val = 0; > > > > + > > > > + /* Calculate (chip-wide) period from prescale value */ > > > > + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); > > > > + /* > > > > + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000. > > > > + * The following calculation is therefore only a multiplication > > > > + * and we are not losing precision. > > > > + */ > > > > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * > > > > + (val + 1); > > > > + > > > > + /* The (per-channel) polarity is fixed */ > > > > + state->polarity = PWM_POLARITY_NORMAL; > > > > + > > > > + if (pwm->hwpwm >= PCA9685_MAXCHAN) { > > > > + /* > > > > + * The "all LEDs" channel does not support HW readout > > > > + * Return 0 and disabled for backwards compatibility > > > > + */ > > > > + state->duty_cycle = 0; > > > > + state->enabled = false; > > > > + return; > > > > + } > > > > + > > > > + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > > > > + > > > > + state->enabled = !!duty; > > > > + if (!state->enabled) { > > > > + state->duty_cycle = 0; > > > > + return; > > > > + } else if (duty == PCA9685_COUNTER_RANGE) { > > > > + state->duty_cycle = state->period; > > > > + return; > > > > + } > > > > + > > > > + duty *= state->period; > > > > + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; > > > > > > Given that with duty = 0 the chip is still "on" and changing the duty > > > will first complete the currently running period, I'd model duty=0 as > > > enabled. This also simplifies the code a bit, to something like: > > > > > > > > > state->enabled = true; > > > duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); > > > state->duty_cycle = div_round_up(duty * state->period, PCA9685_COUNTER_RANGE); > > > > > > (I'm using round-up here assuming apply uses round-down to get > > > idempotency. In the current patch set state this is wrong however.) > > > > So, in your opinion, every requested PWM of the pca9685 should always be > > enabled by default (from the PWM core viewpoint) ? > > > > And this wouldn't break the following because pwm_get_state does not > > actually read out the hw state: > > pwm_get_state -> enabled=true duty=0 > > pwm_apply_state -> enabled =false duty=0 > > pwm_get_state -> enabled=false duty=0 > > I don't see any breakage here. Either there is none or I failed to grasp > where you see a problem. Me neither, I was just thinking out loud. Clemens
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c index 5a2ce97e71fd..d4474c5ff96f 100644 --- a/drivers/pwm/pwm-pca9685.c +++ b/drivers/pwm/pwm-pca9685.c @@ -333,6 +333,51 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, return 0; } +static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct pca9685 *pca = to_pca(chip); + unsigned long long duty; + unsigned int val = 0; + + /* Calculate (chip-wide) period from prescale value */ + regmap_read(pca->regmap, PCA9685_PRESCALE, &val); + /* + * PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000. + * The following calculation is therefore only a multiplication + * and we are not losing precision. + */ + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) * + (val + 1); + + /* The (per-channel) polarity is fixed */ + state->polarity = PWM_POLARITY_NORMAL; + + if (pwm->hwpwm >= PCA9685_MAXCHAN) { + /* + * The "all LEDs" channel does not support HW readout + * Return 0 and disabled for backwards compatibility + */ + state->duty_cycle = 0; + state->enabled = false; + return; + } + + duty = pca9685_pwm_get_duty(pca, pwm->hwpwm); + + state->enabled = !!duty; + if (!state->enabled) { + state->duty_cycle = 0; + return; + } else if (duty == PCA9685_COUNTER_RANGE) { + state->duty_cycle = state->period; + return; + } + + duty *= state->period; + state->duty_cycle = duty / PCA9685_COUNTER_RANGE; +} + static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) { struct pca9685 *pca = to_pca(chip); @@ -355,6 +400,7 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) static const struct pwm_ops pca9685_pwm_ops = { .apply = pca9685_pwm_apply, + .get_state = pca9685_pwm_get_state, .request = pca9685_pwm_request, .free = pca9685_pwm_free, .owner = THIS_MODULE,
Implements .get_state to read-out the current hardware state. The hardware readout may return slightly different values than those that were set in apply due to the limited range of possible prescale and counter register values. Also note that although the datasheet mentions 200 Hz as default frequency when using the internal 25 MHz oscillator, the calculated period from the default prescaler register setting of 30 is 5079040ns. Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com> --- Changes since v6: - Added a comment regarding the division (Suggested by Uwe) - Rebased drivers/pwm/pwm-pca9685.c | 46 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)