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
Signed-off-by: Clemens Gruber <[email protected]>
---
Changes since v4:
- Patches split up
- Use a single set_duty function
- Improve readability / new macros
- Added a patch to restrict prescale changes to the first user
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 | 253 +++++++++++++-------------------------
1 file changed, 87 insertions(+), 166 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 4a55dc18656c..1b5b5fb93b43 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -51,7 +51,6 @@
#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
@@ -71,10 +70,14 @@
#define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
#define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
+#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
+#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
+#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
+#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
+
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 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}
+static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
+{
+ if (duty == 0) {
+ /* Set the full OFF bit, which has the highest precedence */
+ regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ } else if (duty >= PCA9685_COUNTER_RANGE) {
+ /* Set the full ON bit and clear the full OFF bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
+ regmap_write(pca->regmap, REG_OFF_H(channel), 0);
+ } else {
+ /* Set OFF time (clears the full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
+ /* Clear the full ON bit */
+ regmap_write(pca->regmap, REG_ON_H(channel), 0);
+ }
+}
+
+static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
+{
+ unsigned int off_h, val;
+
+ if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
+ /* Hardware readout not supported for "all LEDs" channel */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
+ if (off_h & LED_FULL) {
+ /* Full OFF bit is set */
+ return 0;
+ }
+
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
+ if (val & LED_FULL) {
+ /* Full ON bit is set */
+ return PCA9685_COUNTER_RANGE;
+ }
+
+ regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ return ((off_h & 0xf) << 8) | (val & 0xff);
+}
+
#if IS_ENABLED(CONFIG_GPIOLIB)
static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
{
@@ -138,34 +184,23 @@ 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_get_duty(pca, offset) != 0;
}
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);
+ pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
}
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_duty(pca, offset, 0);
pm_runtime_put(pca->chip.dev);
pca9685_pwm_clear_inuse(pca, offset);
}
@@ -246,167 +281,56 @@ 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 int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
{
struct pca9685 *pca = to_pca(chip);
- unsigned long long duty;
- 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 long long duty, prescale;
+ 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);
+ if (state->polarity != PWM_POLARITY_NORMAL)
+ return -EOPNOTSUPP;
- regmap_write(pca->regmap, reg, LED_FULL);
-
- return 0;
+ 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;
}
- 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);
-
- regmap_write(pca->regmap, reg, LED_FULL);
+ duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
+ duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
+ if (!state->enabled || duty < 1) {
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+ return 0;
+ } else if (duty == PCA9685_COUNTER_RANGE) {
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
return 0;
}
- 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;
-}
-
-static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- 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);
-
- regmap_write(pca->regmap, reg, 0);
-
- 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);
+ 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);
- /*
- * 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);
+ /* Change the chip-wide output frequency */
+ regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
- regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
+ /* Wake the chip up */
+ pca9685_set_sleep_mode(pca, false);
+ }
+ pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
return 0;
}
-static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
- struct pca9685 *pca = to_pca(chip);
- unsigned int reg;
-
- if (pwm->hwpwm >= PCA9685_MAXCHAN)
- reg = PCA9685_ALL_LED_OFF_H;
- else
- reg = LED_N_OFF_H(pwm->hwpwm);
-
- regmap_write(pca->regmap, reg, LED_FULL);
-
- /* Clear the LED_OFF counter. */
- 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);
-}
-
static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);
@@ -422,15 +346,13 @@ 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_duty(pca, pwm->hwpwm, 0);
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,
+ .apply = pca9685_pwm_apply,
.request = pca9685_pwm_request,
.free = pca9685_pwm_free,
.owner = THIS_MODULE,
@@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
ret);
return ret;
}
- pca->period_ns = PCA9685_DEFAULT_PERIOD;
i2c_set_clientdata(client, pca);
--
2.29.2
The chip does not come out of POR in active state but in sleep state.
To be sure (in case the bootloader woke it up) we force it to sleep in
probe.
On kernels without CONFIG_PM, we wake the chip in .probe and put it to
sleep in .remove.
Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index b3398963c0ff..7b14447f3c05 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -467,14 +467,19 @@ 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
+ * The chip comes out of power-up in the sleep state,
+ * but force it to sleep in case it was woken up before
*/
+ pca9685_set_sleep_mode(pca, true);
+ 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;
}
@@ -486,7 +491,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
Hi Clemens, this looks compact, simple and neat. I like it a lot !!
Few very minor nits below.
On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
<[email protected]> 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
>
> Signed-off-by: Clemens Gruber <[email protected]>
> ---
> Changes since v4:
> - Patches split up
> - Use a single set_duty function
> - Improve readability / new macros
> - Added a patch to restrict prescale changes to the first user
>
> 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 | 253 +++++++++++++-------------------------
> 1 file changed, 87 insertions(+), 166 deletions(-)
>
> diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> index 4a55dc18656c..1b5b5fb93b43 100644
> --- a/drivers/pwm/pwm-pca9685.c
> +++ b/drivers/pwm/pwm-pca9685.c
> @@ -51,7 +51,6 @@
> #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
> @@ -71,10 +70,14 @@
> #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
>
> +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
> +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
> +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
> +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
Yes !!
> +
> 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 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> return container_of(chip, struct pca9685, chip);
> }
>
> +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> +{
Add brief function documentation to clarify that 'duty' sets the duty cycle
_ratio_ to 'duty/4096' on ? I.e.
duty == 2048 => duty cycle ratio = 2048/4096 = 50% on
duty == 4096 => duty cycle ratio = 4096/4086 = 100% on etc
> + if (duty == 0) {
> + /* Set the full OFF bit, which has the highest precedence */
> + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> + } else if (duty >= PCA9685_COUNTER_RANGE) {
> + /* Set the full ON bit and clear the full OFF bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> + } else {
> + /* Set OFF time (clears the full OFF bit) */
> + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> + /* Clear the full ON bit */
> + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> + }
> +}
> +
> +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> +{
> + unsigned int off_h, val;
> +
> + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> + /* Hardware readout not supported for "all LEDs" channel */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> + if (off_h & LED_FULL) {
I believe this may trigger bots which are monitoring patches on LKML:
if regmap_read() somehow fails, off_h will be used uninitialized.
Prevent by initializing off_h and val?
> + /* Full OFF bit is set */
> + return 0;
> + }
> +
> + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> + if (val & LED_FULL) {
> + /* Full ON bit is set */
> + return PCA9685_COUNTER_RANGE;
> + }
> +
> + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> + return ((off_h & 0xf) << 8) | (val & 0xff);
> +}
> +
> #if IS_ENABLED(CONFIG_GPIOLIB)
> static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> {
> @@ -138,34 +184,23 @@ 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_get_duty(pca, offset) != 0;
> }
>
> 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);
> + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> }
>
> 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_duty(pca, offset, 0);
> pm_runtime_put(pca->chip.dev);
> pca9685_pwm_clear_inuse(pca, offset);
> }
> @@ -246,167 +281,56 @@ 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 int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> {
> struct pca9685 *pca = to_pca(chip);
> - unsigned long long duty;
> - 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 long long duty, prescale;
> + 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);
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EOPNOTSUPP;
>
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - return 0;
> + 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");
would "pwm not changed: period out of bounds" be a clearer error message here?
> + return -EINVAL;
> }
>
> - 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);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
>
> + if (!state->enabled || duty < 1) {
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> + return 0;
> + } else if (duty == PCA9685_COUNTER_RANGE) {
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> return 0;
> }
>
> - 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;
> -}
> -
> -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - 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);
> -
> - regmap_write(pca->regmap, reg, 0);
> -
> - 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);
> + 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);
>
> - /*
> - * 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);
> + /* Change the chip-wide output frequency */
> + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
>
> - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> + /* Wake the chip up */
> + pca9685_set_sleep_mode(pca, false);
> + }
>
> + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> return 0;
> }
>
> -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> - struct pca9685 *pca = to_pca(chip);
> - unsigned int reg;
> -
> - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> - reg = PCA9685_ALL_LED_OFF_H;
> - else
> - reg = LED_N_OFF_H(pwm->hwpwm);
> -
> - regmap_write(pca->regmap, reg, LED_FULL);
> -
> - /* Clear the LED_OFF counter. */
> - 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);
> -}
> -
> static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> {
> struct pca9685 *pca = to_pca(chip);
> @@ -422,15 +346,13 @@ 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_duty(pca, pwm->hwpwm, 0);
> 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,
> + .apply = pca9685_pwm_apply,
> .request = pca9685_pwm_request,
> .free = pca9685_pwm_free,
> .owner = THIS_MODULE,
> @@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> ret);
> return ret;
> }
> - pca->period_ns = PCA9685_DEFAULT_PERIOD;
>
> i2c_set_clientdata(client, pca);
>
> --
> 2.29.2
>
Hi Sven,
On Wed, Dec 16, 2020 at 10:58:07PM -0500, Sven Van Asbroeck wrote:
> Hi Clemens, this looks compact, simple and neat. I like it a lot !!
Thanks!
>
> Few very minor nits below.
>
> On Wed, Dec 16, 2020 at 7:53 AM Clemens Gruber
> <[email protected]> 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
> >
> > Signed-off-by: Clemens Gruber <[email protected]>
> > ---
> > Changes since v4:
> > - Patches split up
> > - Use a single set_duty function
> > - Improve readability / new macros
> > - Added a patch to restrict prescale changes to the first user
> >
> > 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 | 253 +++++++++++++-------------------------
> > 1 file changed, 87 insertions(+), 166 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
> > index 4a55dc18656c..1b5b5fb93b43 100644
> > --- a/drivers/pwm/pwm-pca9685.c
> > +++ b/drivers/pwm/pwm-pca9685.c
> > @@ -51,7 +51,6 @@
> > #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
> > @@ -71,10 +70,14 @@
> > #define LED_N_OFF_H(N) (PCA9685_LEDX_OFF_H + (4 * (N)))
> > #define LED_N_OFF_L(N) (PCA9685_LEDX_OFF_L + (4 * (N)))
> >
> > +#define REG_ON_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_H : LED_N_ON_H((C)))
> > +#define REG_ON_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_ON_L : LED_N_ON_L((C)))
> > +#define REG_OFF_H(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_H : LED_N_OFF_H((C)))
> > +#define REG_OFF_L(C) ((C) >= PCA9685_MAXCHAN ? PCA9685_ALL_LED_OFF_L : LED_N_OFF_L((C)))
>
> Yes !!
>
> > +
> > 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 +90,49 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
> > return container_of(chip, struct pca9685, chip);
> > }
> >
> > +static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
> > +{
>
> Add brief function documentation to clarify that 'duty' sets the duty cycle
> _ratio_ to 'duty/4096' on ? I.e.
> duty == 2048 => duty cycle ratio = 2048/4096 = 50% on
> duty == 4096 => duty cycle ratio = 4096/4086 = 100% on etc
Will do.
>
> > + if (duty == 0) {
> > + /* Set the full OFF bit, which has the highest precedence */
> > + regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
> > + } else if (duty >= PCA9685_COUNTER_RANGE) {
> > + /* Set the full ON bit and clear the full OFF bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), LED_FULL);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), 0);
> > + } else {
> > + /* Set OFF time (clears the full OFF bit) */
> > + regmap_write(pca->regmap, REG_OFF_L(channel), duty & 0xff);
> > + regmap_write(pca->regmap, REG_OFF_H(channel), (duty >> 8) & 0xf);
> > + /* Clear the full ON bit */
> > + regmap_write(pca->regmap, REG_ON_H(channel), 0);
> > + }
> > +}
> > +
> > +static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
> > +{
> > + unsigned int off_h, val;
> > +
> > + if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
> > + /* Hardware readout not supported for "all LEDs" channel */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
> > + if (off_h & LED_FULL) {
>
> I believe this may trigger bots which are monitoring patches on LKML:
> if regmap_read() somehow fails, off_h will be used uninitialized.
>
> Prevent by initializing off_h and val?
I can initialize the values to 0 of course and check the file for other
places with missing initializations.
Or would it be better to check the return codes of regmap_read/write in
such cases? I'm not sure.
>
> > + /* Full OFF bit is set */
> > + return 0;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
> > + if (val & LED_FULL) {
> > + /* Full ON bit is set */
> > + return PCA9685_COUNTER_RANGE;
> > + }
> > +
> > + regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
> > + return ((off_h & 0xf) << 8) | (val & 0xff);
> > +}
> > +
> > #if IS_ENABLED(CONFIG_GPIOLIB)
> > static bool pca9685_pwm_test_and_set_inuse(struct pca9685 *pca, int pwm_idx)
> > {
> > @@ -138,34 +184,23 @@ 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_get_duty(pca, offset) != 0;
> > }
> >
> > 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);
> > + pca9685_pwm_set_duty(pca, offset, value ? PCA9685_COUNTER_RANGE : 0);
> > }
> >
> > 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_duty(pca, offset, 0);
> > pm_runtime_put(pca->chip.dev);
> > pca9685_pwm_clear_inuse(pca, offset);
> > }
> > @@ -246,167 +281,56 @@ 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 int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > - unsigned long long duty;
> > - 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 long long duty, prescale;
> > + 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);
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EOPNOTSUPP;
> >
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - return 0;
> > + 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");
>
> would "pwm not changed: period out of bounds" be a clearer error message here?
Yes, I think so. I'll change it in the next version.
>
> > + return -EINVAL;
> > }
> >
> > - 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);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > + duty = PCA9685_COUNTER_RANGE * state->duty_cycle;
> > + duty = DIV_ROUND_CLOSEST_ULL(duty, state->period);
> >
> > + if (!state->enabled || duty < 1) {
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
> > + return 0;
> > + } else if (duty == PCA9685_COUNTER_RANGE) {
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > return 0;
> > }
> >
> > - 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;
> > -}
> > -
> > -static int pca9685_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - 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);
> > -
> > - regmap_write(pca->regmap, reg, 0);
> > -
> > - 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);
> > + 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);
> >
> > - /*
> > - * 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);
> > + /* Change the chip-wide output frequency */
> > + regmap_write(pca->regmap, PCA9685_PRESCALE, (int)prescale);
> >
> > - regmap_update_bits(pca->regmap, reg, LED_FULL, 0x0);
> > + /* Wake the chip up */
> > + pca9685_set_sleep_mode(pca, false);
> > + }
> >
> > + pca9685_pwm_set_duty(pca, pwm->hwpwm, duty);
> > return 0;
> > }
> >
> > -static void pca9685_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > -{
> > - struct pca9685 *pca = to_pca(chip);
> > - unsigned int reg;
> > -
> > - if (pwm->hwpwm >= PCA9685_MAXCHAN)
> > - reg = PCA9685_ALL_LED_OFF_H;
> > - else
> > - reg = LED_N_OFF_H(pwm->hwpwm);
> > -
> > - regmap_write(pca->regmap, reg, LED_FULL);
> > -
> > - /* Clear the LED_OFF counter. */
> > - 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);
> > -}
> > -
> > static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > {
> > struct pca9685 *pca = to_pca(chip);
> > @@ -422,15 +346,13 @@ 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_duty(pca, pwm->hwpwm, 0);
> > 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,
> > + .apply = pca9685_pwm_apply,
> > .request = pca9685_pwm_request,
> > .free = pca9685_pwm_free,
> > .owner = THIS_MODULE,
> > @@ -461,7 +383,6 @@ static int pca9685_pwm_probe(struct i2c_client *client,
> > ret);
> > return ret;
> > }
> > - pca->period_ns = PCA9685_DEFAULT_PERIOD;
> >
> > i2c_set_clientdata(client, pca);
> >
> > --
> > 2.29.2
> >
Thanks,
Clemens
On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
<[email protected]> wrote:
>
> I can initialize the values to 0 of course and check the file for other
> places with missing initializations.
>
> Or would it be better to check the return codes of regmap_read/write in
> such cases? I'm not sure.
I think that checking the regmap_read/write return values is overkill
in this driver. These functions can't realistically fail, except if the i2c
bus is bad, i.e. h/w failure or intermittency. And that's an externality
which I believe we can ignore.
Maybe Thierry or Uwe have further insights here.
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <[email protected]> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
>
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.
>
> Maybe Thierry or Uwe have further insights here.
I'm a fan of full checking, but I'm not sure what's Thierry's position
on that.
My reasoning is: If the bus is bad and a request to modify the PWM fails
because of that, the PWM consumer probably wants to know.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Clemens,
On Tue, Dec 15, 2020 at 10:22:22PM +0100, Clemens Gruber wrote:
> + if (state->polarity != PWM_POLARITY_NORMAL)
> + return -EOPNOTSUPP;
We agreed on -EINVAL for that one since 2b1c1a5d5148.
Other than that the patch looks ok (but note I only looked quickly).
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
On Mon, Mar 01, 2021 at 10:41:15PM +0100, Uwe Kleine-K?nig wrote:
> On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> > <[email protected]> wrote:
> > >
> > > I can initialize the values to 0 of course and check the file for other
> > > places with missing initializations.
> > >
> > > Or would it be better to check the return codes of regmap_read/write in
> > > such cases? I'm not sure.
> >
> > I think that checking the regmap_read/write return values is overkill
> > in this driver. These functions can't realistically fail, except if the i2c
> > bus is bad, i.e. h/w failure or intermittency. And that's an externality
> > which I believe we can ignore.
> >
> > Maybe Thierry or Uwe have further insights here.
>
> I'm a fan of full checking, but I'm not sure what's Thierry's position
> on that.
>
> My reasoning is: If the bus is bad and a request to modify the PWM fails
> because of that, the PWM consumer probably wants to know.
I see. Then I'd suggest that we postpone adding these checks until we
get a response from Thierry and if he agrees with you, we could add
these checks in a separate patch series?
Thanks,
Clemens
Hi Uwe,
On Mon, Mar 01, 2021 at 10:44:07PM +0100, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Tue, Dec 15, 2020 at 10:22:22PM +0100, Clemens Gruber wrote:
> > + if (state->polarity != PWM_POLARITY_NORMAL)
> > + return -EOPNOTSUPP;
>
> We agreed on -EINVAL for that one since 2b1c1a5d5148.
>
> Other than that the patch looks ok (but note I only looked quickly).
Thanks for looking over it. This will be -EINVAL in the next revision.
Best regards,
Clemens
On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> <[email protected]> wrote:
> >
> > I can initialize the values to 0 of course and check the file for other
> > places with missing initializations.
> >
> > Or would it be better to check the return codes of regmap_read/write in
> > such cases? I'm not sure.
>
> I think that checking the regmap_read/write return values is overkill
> in this driver. These functions can't realistically fail, except if the i2c
> bus is bad, i.e. h/w failure or intermittency. And that's an externality
> which I believe we can ignore.
I think there are (rare) occasions where it's fine to not check for
errors, i.e. if you definitively know that calls can't fail. However,
given that this uses regmap and you don't really know what's backing
this, I think it's always better to err on the side of caution and
properly check the return values.
The fact that this can be externally caused is actually a reason why
we shouldn't be ignoring any errors. If there's a chip that's hogging
the I2C bus or if you've even just mistyped the I2C client's address
in DT, it's better if the PWM driver tells you with an error message
than if it is silently ignoring the errors and keeps you guessing at
why the PWM isn't behaving the way it should.
Granted, the error code isn't always going to pinpoint exactly what's
going wrong, but for serious errors often the I2C bus driver will let
you know with an extra error message. However, it's much easier to go
looking for that error message if the PWM driver lets you know that
something went wrong.
Please just add full checking of regmap operations.
Thierry
Hi Thierry,
On Mon, Mar 22, 2021 at 08:58:35AM +0100, Thierry Reding wrote:
> On Thu, Dec 17, 2020 at 12:10:10PM -0500, Sven Van Asbroeck wrote:
> > On Thu, Dec 17, 2020 at 11:48 AM Clemens Gruber
> > <[email protected]> wrote:
> > >
> > > I can initialize the values to 0 of course and check the file for other
> > > places with missing initializations.
> > >
> > > Or would it be better to check the return codes of regmap_read/write in
> > > such cases? I'm not sure.
> >
> > I think that checking the regmap_read/write return values is overkill
> > in this driver. These functions can't realistically fail, except if the i2c
> > bus is bad, i.e. h/w failure or intermittency. And that's an externality
> > which I believe we can ignore.
>
> I think there are (rare) occasions where it's fine to not check for
> errors, i.e. if you definitively know that calls can't fail. However,
> given that this uses regmap and you don't really know what's backing
> this, I think it's always better to err on the side of caution and
> properly check the return values.
>
> The fact that this can be externally caused is actually a reason why
> we shouldn't be ignoring any errors. If there's a chip that's hogging
> the I2C bus or if you've even just mistyped the I2C client's address
> in DT, it's better if the PWM driver tells you with an error message
> than if it is silently ignoring the errors and keeps you guessing at
> why the PWM isn't behaving the way it should.
>
> Granted, the error code isn't always going to pinpoint exactly what's
> going wrong, but for serious errors often the I2C bus driver will let
> you know with an extra error message. However, it's much easier to go
> looking for that error message if the PWM driver lets you know that
> something went wrong.
>
> Please just add full checking of regmap operations.
OK, I will create a separate patch adding these checks in the next
series.
This will lead to > 20 additional dev_err statements, let me know if I
should instead just return the error code and not add dev_err's for
every failed regmap operation.
Clemens