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 <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 0ed1013737e3..fb026a25fb61 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -333,6 +333,46 @@ 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);
+ 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 +395,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,
--
2.31.1
Hello Clemens,
On Mon, Mar 29, 2021 at 02:57:02PM +0200, 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.
This is fine and for most hardware that's not preventable. My
requirement here is that
.get_state(pwm, &state);
.apply_state(pwm, &state);
doesn't yield any changes.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
On Mon, Mar 29, 2021 at 05:51:40PM +0200, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Mon, Mar 29, 2021 at 02:57:02PM +0200, 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.
>
> This is fine and for most hardware that's not preventable. My
> requirement here is that
>
> .get_state(pwm, &state);
> .apply_state(pwm, &state);
>
> doesn't yield any changes.
Yes, that would not change anything.
Thanks,
Clemens
On Mon, Mar 29, 2021 at 02:57:02PM +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 <[email protected]>
> ---
> drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 0ed1013737e3..fb026a25fb61 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -333,6 +333,46 @@ 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);
> + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> + (val + 1);
As we have PCA9685_OSC_CLOCK_MHZ = 25 this is an integer calculation
without loss of precision. It might be worth to point that out in a
comment. (Otherwise doing the division at the end might be more
sensible.)
> + /* 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;
.apply uses ROUND_CLOSEST to calculate duty from state->duty_cycle,
still using / here (instead of ROUND_CLOSEST), but again as
PCA9685_OSC_CLOCK_MHZ is 25 this calculation doesn't suffer from
rounding errors. So if you feed the state returned here into .apply
again, there is (as I want it) no change.
The only annoyance is that if PCA9685_PRESCALE holds a value smaller
than 3, .apply() will fail. Not sure there is any saner way to handle
this.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Mar 29, 2021 at 06:54:29PM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Mar 29, 2021 at 02:57:02PM +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 <[email protected]>
> > ---
> > drivers/pwm/pwm-pca9685.c | 41 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 41 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 0ed1013737e3..fb026a25fb61 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -333,6 +333,46 @@ 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);
> > + state->period = (PCA9685_COUNTER_RANGE * 1000 / PCA9685_OSC_CLOCK_MHZ) *
> > + (val + 1);
>
> As we have PCA9685_OSC_CLOCK_MHZ = 25 this is an integer calculation
> without loss of precision. It might be worth to point that out in a
> comment. (Otherwise doing the division at the end might be more
> sensible.)
What comment do you have in mind?
/* 1 integer multiplication (without loss of precision) at runtime */ ?
>
> > + /* 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;
>
> .apply uses ROUND_CLOSEST to calculate duty from state->duty_cycle,
> still using / here (instead of ROUND_CLOSEST), but again as
> PCA9685_OSC_CLOCK_MHZ is 25 this calculation doesn't suffer from
> rounding errors. So if you feed the state returned here into .apply
> again, there is (as I want it) no change.
>
> The only annoyance is that if PCA9685_PRESCALE holds a value smaller
> than 3, .apply() will fail. Not sure there is any saner way to handle
> this.
According to the datasheet, "The hardware forces a minimum value that
can be loaded into the PRE_SCALE register at '3'", so there should never
be anything below 3 in that register.
Thanks for your review!
Clemens
On Mon, Mar 29, 2021 at 07:11:53PM +0200, Clemens Gruber wrote:
> On Mon, Mar 29, 2021 at 06:54:29PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Mar 29, 2021 at 02:57:02PM +0200, Clemens Gruber wrote:
> > > [...]
> > > + /* 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);
> >
> > As we have PCA9685_OSC_CLOCK_MHZ = 25 this is an integer calculation
> > without loss of precision. It might be worth to point that out in a
> > comment. (Otherwise doing the division at the end might be more
> > sensible.)
>
> What comment do you have in mind?
> /* 1 integer multiplication (without loss of precision) at runtime */ ?
Something like:
/*
* PCA9685_OSC_CLOCK_MHZ is 25 and so an integer divider of
* 1000. So the calculation here is only a multiplication and
* we're not loosing precision.
*/
> > > + /* 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;
> >
> > .apply uses ROUND_CLOSEST to calculate duty from state->duty_cycle,
> > still using / here (instead of ROUND_CLOSEST), but again as
> > PCA9685_OSC_CLOCK_MHZ is 25 this calculation doesn't suffer from
> > rounding errors. So if you feed the state returned here into .apply
> > again, there is (as I want it) no change.
> >
> > The only annoyance is that if PCA9685_PRESCALE holds a value smaller
> > than 3, .apply() will fail. Not sure there is any saner way to handle
> > this.
>
> According to the datasheet, "The hardware forces a minimum value that
> can be loaded into the PRE_SCALE register at '3'", so there should never
> be anything below 3 in that register.
Did you verify that the register reads back a 3 if you write a lower
value into the register?
Maybe the most defensive way would be:
+ regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ /*
+ * According to the datasheet, the hardware forces a minimum
+ * value that can be loaded is 3, so if we read something lower
+ * assume that the hardware actually implemented a 3.
+ */
+ if (val < 3)
+ val = 3;
+ state->period = ...
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |