LinuxLists.cc - [PATCH 1/4] pwm: core: Support new usage

[permalink] [raw]

Subject: [PATCH 3/4] pwm: pca9685: Restrict period change for enabled PWMs

Previously, the last used PWM channel could change the global prescale
setting, even if other channels are already in use.

Fix it by only allowing the first enabled PWM to change the global
chip-wide prescale setting. If there is more than one channel in use,
the prescale settings resulting from the chosen periods must match.

GPIOs do not count as enabled PWMs as they are not using the prescaler
and can't change it.

Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 74 +++++++++++++++++++++++++++++++++------
1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 20dd579297e6..f70162114197 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -23,11 +23,11 @@
#include <linux/bitmap.h>

/*
- * Because the PCA9685 has only one prescaler per chip, changing the period of
- * one channel affects the period of all 16 PWM outputs!
- * However, the ratio between each configured duty cycle and the chip-wide
- * period remains constant, because the OFF time is set in proportion to the
- * counter range.
+ * Because the PCA9685 has only one prescaler per chip, only the first channel
+ * that is enabled is allowed to change the prescale register.
+ * PWM channels requested afterwards must use a period that results in the same
+ * prescale setting as the one set by the first requested channel.
+ * GPIOs do not count as enabled PWMs as they are not using the prescaler.
*/

#define PCA9685_MODE1 0x00
@@ -78,8 +78,9 @@
struct pca9685 {
struct pwm_chip chip;
struct regmap *regmap;
-#if IS_ENABLED(CONFIG_GPIOLIB)
struct mutex lock;
+ DECLARE_BITMAP(pwms_enabled, PCA9685_MAXCHAN + 1);
+#if IS_ENABLED(CONFIG_GPIOLIB)
struct gpio_chip gpio;
DECLARE_BITMAP(pwms_inuse, PCA9685_MAXCHAN + 1);
#endif
@@ -90,6 +91,22 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
return container_of(chip, struct pca9685, chip);
}

+/* This function is supposed to be called with the lock mutex held */
+static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
+{
+ /* No PWM enabled: Change allowed */
+ if (bitmap_empty(pca->pwms_enabled, PCA9685_MAXCHAN + 1))
+ return true;
+ /* More than one PWM enabled: Change not allowed */
+ if (bitmap_weight(pca->pwms_enabled, PCA9685_MAXCHAN + 1) > 1)
+ return false;
+ /*
+ * Only one PWM enabled: Change allowed if the PWM about to
+ * be changed is the one that is already enabled
+ */
+ return test_bit(channel, pca->pwms_enabled);
+}
+
/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
@@ -269,8 +286,6 @@ static int pca9685_pwm_gpio_probe(struct pca9685 *pca)
{
struct device *dev = pca->chip.dev;

- mutex_init(&pca->lock);
-
pca->gpio.label = dev_name(dev);
pca->gpio.parent = dev;
pca->gpio.request = pca9685_pwm_gpio_request;
@@ -314,8 +329,8 @@ static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
}
}

-static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
- const struct pwm_state *state)
+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, prescale;
@@ -338,6 +353,12 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,

regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
if (prescale != val) {
+ if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) {
+ dev_err(chip->dev,
+ "pwm not changed: periods of enabled pwms must match!\n");
+ return -EBUSY;
+ }
+
/*
* Putting the chip briefly into SLEEP mode
* at this point won't interfere with the
@@ -360,6 +381,25 @@ static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}

+static int pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+ const struct pwm_state *state)
+{
+ struct pca9685 *pca = to_pca(chip);
+ int ret;
+
+ mutex_lock(&pca->lock);
+ ret = __pca9685_pwm_apply(chip, pwm, state);
+ if (ret == 0) {
+ if (state->enabled)
+ set_bit(pwm->hwpwm, pca->pwms_enabled);
+ else
+ clear_bit(pwm->hwpwm, pca->pwms_enabled);
+ }
+ mutex_unlock(&pca->lock);
+
+ return ret;
+}
+
static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
@@ -401,6 +441,14 @@ static int pca9685_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)

if (pca9685_pwm_test_and_set_inuse(pca, pwm->hwpwm))
return -EBUSY;
+
+ if (pwm->hwpwm < PCA9685_MAXCHAN) {
+ /* PWMs - except the "all LEDs" channel - default to enabled */
+ mutex_lock(&pca->lock);
+ set_bit(pwm->hwpwm, pca->pwms_enabled);
+ mutex_unlock(&pca->lock);
+ }
+
pm_runtime_get_sync(chip->dev);

return 0;
@@ -410,7 +458,11 @@ static void pca9685_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct pca9685 *pca = to_pca(chip);

+ mutex_lock(&pca->lock);
pca9685_pwm_set_duty(pca, pwm->hwpwm, 0);
+ clear_bit(pwm->hwpwm, pca->pwms_enabled);
+ mutex_unlock(&pca->lock);
+
pm_runtime_put(chip->dev);
pca9685_pwm_clear_inuse(pca, pwm->hwpwm);
}
@@ -451,6 +503,8 @@ static int pca9685_pwm_probe(struct i2c_client *client,

i2c_set_clientdata(client, pca);

+ mutex_init(&pca->lock);
+
regmap_read(pca->regmap, PCA9685_MODE2, &reg);

if (device_property_read_bool(&client->dev, "invert"))
--
2.31.1

2021-05-07 17:10:20

[permalink] [raw]

Subject: [PATCH 4/4] pwm: pca9685: Add error messages for failed regmap calls

Regmap operations can fail if the underlying subsystem is not working
properly (e.g. hogged I2C bus, etc.)
As this is useful information for the user, print an error message if it
happens.
Let probe fail if the first regmap_read or the first regmap_write fails.

Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 83 ++++++++++++++++++++++++++++-----------
1 file changed, 59 insertions(+), 24 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index f70162114197..42ed770b432c 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -107,6 +107,30 @@ static bool pca9685_prescaler_can_change(struct pca9685 *pca, int channel)
return test_bit(channel, pca->pwms_enabled);
}

+static int pca9685_read_reg(struct pca9685 *pca, unsigned int reg, unsigned int *val)
+{
+ struct device *dev = pca->chip.dev;
+ int err;
+
+ err = regmap_read(pca->regmap, reg, val);
+ if (err)
+ dev_err(dev, "regmap_read of register 0x%x failed: %pe\n", reg, ERR_PTR(err));
+
+ return err;
+}
+
+static int pca9685_write_reg(struct pca9685 *pca, unsigned int reg, unsigned int val)
+{
+ struct device *dev = pca->chip.dev;
+ int err;
+
+ err = regmap_write(pca->regmap, reg, val);
+ if (err)
+ dev_err(dev, "regmap_write to register 0x%x failed: %pe\n", reg, ERR_PTR(err));
+
+ return err;
+}
+
/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
@@ -115,12 +139,12 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int

if (duty == 0) {
/* Set the full OFF bit, which has the highest precedence */
- regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ pca9685_write_reg(pca, REG_OFF_H(channel), LED_FULL);
return;
} 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);
+ pca9685_write_reg(pca, REG_ON_H(channel), LED_FULL);
+ pca9685_write_reg(pca, REG_OFF_H(channel), 0);
return;
}

@@ -140,11 +164,11 @@ static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int
off = (on + duty) % PCA9685_COUNTER_RANGE;

/* Set ON time (clears full ON bit) */
- regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
- regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+ pca9685_write_reg(pca, REG_ON_L(channel), on & 0xff);
+ pca9685_write_reg(pca, REG_ON_H(channel), (on >> 8) & 0xf);
/* Set OFF time (clears full OFF bit) */
- regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
- regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
+ pca9685_write_reg(pca, REG_OFF_L(channel), off & 0xff);
+ pca9685_write_reg(pca, REG_OFF_H(channel), (off >> 8) & 0xf);
}

static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
@@ -157,25 +181,25 @@ static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
return 0;
}

- regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+ pca9685_read_reg(pca, LED_N_OFF_H(channel), &off);
if (off & LED_FULL) {
/* Full OFF bit is set */
return 0;
}

- regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+ pca9685_read_reg(pca, LED_N_ON_H(channel), &on);
if (on & LED_FULL) {
/* Full ON bit is set */
return PCA9685_COUNTER_RANGE;
}

- regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ pca9685_read_reg(pca, LED_N_OFF_L(channel), &val);
off = ((off & 0xf) << 8) | (val & 0xff);
if (!pwm->state.usage_power)
return off;

/* Read ON register to calculate duty cycle of staggered output */
- if (regmap_read(pca->regmap, LED_N_ON_L(channel), &val)) {
+ if (pca9685_read_reg(pca, LED_N_ON_L(channel), &val)) {
/* Reset val to 0 in case reading LED_N_ON_L failed */
val = 0;
}
@@ -321,8 +345,15 @@ static inline int pca9685_pwm_gpio_probe(struct pca9685 *pca)

static void pca9685_set_sleep_mode(struct pca9685 *pca, bool enable)
{
- regmap_update_bits(pca->regmap, PCA9685_MODE1,
- MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+ struct device *dev = pca->chip.dev;
+ int err = regmap_update_bits(pca->regmap, PCA9685_MODE1,
+ MODE1_SLEEP, enable ? MODE1_SLEEP : 0);
+ if (err) {
+ dev_err(dev, "regmap_update_bits of register 0x%x failed: %pe\n",
+ PCA9685_MODE1, ERR_PTR(err));
+ return;
+ }
+
if (!enable) {
/* Wait 500us for the oscillator to be back up */
udelay(500);
@@ -351,7 +382,7 @@ static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}

- regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
if (prescale != val) {
if (!pca9685_prescaler_can_change(pca, pwm->hwpwm)) {
dev_err(chip->dev,
@@ -369,7 +400,7 @@ static int __pca9685_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
pca9685_set_sleep_mode(pca, true);

/* Change the chip-wide output frequency */
- regmap_write(pca->regmap, PCA9685_PRESCALE, prescale);
+ pca9685_write_reg(pca, PCA9685_PRESCALE, prescale);

/* Wake the chip up */
pca9685_set_sleep_mode(pca, false);
@@ -408,7 +439,7 @@ static void pca9685_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
unsigned int val = 0;

/* Calculate (chip-wide) period from prescale value */
- regmap_read(pca->regmap, PCA9685_PRESCALE, &val);
+ pca9685_read_reg(pca, PCA9685_PRESCALE, &val);
/*
* PCA9685_OSC_CLOCK_MHZ is 25, i.e. an integer divider of 1000.
* The following calculation is therefore only a multiplication
@@ -505,7 +536,9 @@ static int pca9685_pwm_probe(struct i2c_client *client,

mutex_init(&pca->lock);

- regmap_read(pca->regmap, PCA9685_MODE2, &reg);
+ ret = pca9685_read_reg(pca, PCA9685_MODE2, &reg);
+ if (ret)
+ return ret;

if (device_property_read_bool(&client->dev, "invert"))
reg |= MODE2_INVRT;
@@ -517,18 +550,20 @@ static int pca9685_pwm_probe(struct i2c_client *client,
else
reg |= MODE2_OUTDRV;

- regmap_write(pca->regmap, PCA9685_MODE2, reg);
+ ret = pca9685_write_reg(pca, PCA9685_MODE2, reg);
+ if (ret)
+ return ret;

/* Disable all LED ALLCALL and SUBx addresses to avoid bus collisions */
- regmap_read(pca->regmap, PCA9685_MODE1, &reg);
+ pca9685_read_reg(pca, PCA9685_MODE1, &reg);
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
- regmap_write(pca->regmap, PCA9685_MODE1, reg);
+ pca9685_write_reg(pca, PCA9685_MODE1, reg);

/* Reset OFF/ON registers to POR default */
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
- regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
- regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
- regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_L, LED_FULL);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_OFF_H, LED_FULL);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_ON_L, 0);
+ pca9685_write_reg(pca, PCA9685_ALL_LED_ON_H, 0);

pca->chip.ops = &pca9685_pwm_ops;
/* Add an extra channel for ALL_LED */
--
2.31.1

2021-05-07 17:25:13

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hello,

On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 5bb90af4997e..5a73251d28e3 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -54,12 +54,17 @@ enum {
> * @duty_cycle: PWM duty cycle (in nanoseconds)
> * @polarity: PWM polarity
> * @enabled: PWM enabled status
> + * @usage_power: If set, the PWM driver is only required to maintain the power
> + * output but has more freedom regarding signal form.
> + * If supported, the signal can be optimized, for example to
> + * improve EMI by phase shifting individual channels.
> */
> struct pwm_state {
> u64 period;
> u64 duty_cycle;
> enum pwm_polarity polarity;
> bool enabled;
> + bool usage_power;
> };
>
> /**

If we really want to support this usecase, I would prefer to not have it
in pwm_state because this concept is not a property of the wave form. So
for a driver it doesn't really make sense to set this flag in
.get_state().

Maybe it makes more sense to put this in a separate argument for a
variant of pwm_apply_state? something like:

int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);

and pwm_state_transition can then contain something like this usage
power concept and maybe also a sync flag that requests that the call
should only return when the new setting is active and maybe also a
complete_period flag that requests that the currently running period
must be completed before the requested setting is implemented.

OTOH the information "I only care about the relative duty cycle" is
relevant longer than during the transition to a new setting. (So if
there are two consumers and one specified to be only interested in the
relative duty cycle, the other might be allowed to change the common
period.)

Having said that, I don't like the proposed names. Neither "usage_power"
nor "pwm_apply_state_transition".

In a non-representative survey among two hardware engineers and one
software engineer who already contributed to PWM (!= me) I found out
that these three have an intuitive right understanding of
"allow-phase-shift" but have no idea what "usage-power" could mean.

On a side note: The hardware guys noted that it might make sense to
align the shifted phases. i.e. instead of shifting channel i by i *
period/16, it might be better to let the 2nd channel get active when the
first gets inactive. (i.e. try to keep the count of active channels
constant).

And as already mentioned earlier I still think we should define the
concept of "usage power" better. It should be clearly defined for a
driver author which setting they should pick for a given request. This
removes surprises for consumers and guessing for lowlevel driver
authors. Also a uniform policy brings conflicts better to light.
(Something like driver A does the right thing for consumer C and driver
B makes it right for D. But once D tries to use A things break. (And
then maybe A is changed to fit D, and C doesn't object because they
don't read the pwm list resulting in a breakage for C later.))

So in sum: I think this concept is too inchoate and we shouldn't apply
these patches. I would prefer to go for allow-phase-shift (if at all)
for now. There the concept is clear what is allowed (for a lowlevel
driver) resp. can be expected (for a consumer).

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (3.62 kB)
signature.asc (499.00 B)
Download all attachments

2021-05-07 17:34:59

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hi Uwe,

On Fri, May 07, 2021 at 05:03:17PM +0200, Uwe Kleine-K?nig wrote:
> Hello,
>
> On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 5bb90af4997e..5a73251d28e3 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -54,12 +54,17 @@ enum {
> > * @duty_cycle: PWM duty cycle (in nanoseconds)
> > * @polarity: PWM polarity
> > * @enabled: PWM enabled status
> > + * @usage_power: If set, the PWM driver is only required to maintain the power
> > + * output but has more freedom regarding signal form.
> > + * If supported, the signal can be optimized, for example to
> > + * improve EMI by phase shifting individual channels.
> > */
> > struct pwm_state {
> > u64 period;
> > u64 duty_cycle;
> > enum pwm_polarity polarity;
> > bool enabled;
> > + bool usage_power;
> > };
> >
> > /**
>
> If we really want to support this usecase, I would prefer to not have it
> in pwm_state because this concept is not a property of the wave form. So
> for a driver it doesn't really make sense to set this flag in
> .get_state().

It is related to the wave form in so far as it allows the driver to
change the wave form as long as the power output remains the same.

> Maybe it makes more sense to put this in a separate argument for a
> variant of pwm_apply_state? something like:
>
> int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);
>
> and pwm_state_transition can then contain something like this usage
> power concept and maybe also a sync flag that requests that the call
> should only return when the new setting is active and maybe also a
> complete_period flag that requests that the currently running period
> must be completed before the requested setting is implemented.
>
> OTOH the information "I only care about the relative duty cycle" is
> relevant longer than during the transition to a new setting. (So if
> there are two consumers and one specified to be only interested in the
> relative duty cycle, the other might be allowed to change the common
> period.)

As you said, usage_power does not only apply to one state transition.

> Having said that, I don't like the proposed names. Neither "usage_power"
> nor "pwm_apply_state_transition".
>
> In a non-representative survey among two hardware engineers and one
> software engineer who already contributed to PWM (!= me) I found out
> that these three have an intuitive right understanding of
> "allow-phase-shift" but have no idea what "usage-power" could mean.

One advantage of usage_power is that it is not limited to phase
shifting. Drivers could do other optimizations as long as the power
output remains the same.

> On a side note: The hardware guys noted that it might make sense to
> align the shifted phases. i.e. instead of shifting channel i by i *
> period/16, it might be better to let the 2nd channel get active when the
> first gets inactive. (i.e. try to keep the count of active channels
> constant).

I am not sure what you mean exactly, because the duty cycles of the
16 outputs are not necessarily the same and can all be active at the
same time. The idea is to spread the edges out as evenly as possible.
Shifting them by period/16 is the best way to smoothen the current
spikes in my opinion and the implementation is also very simple.

> And as already mentioned earlier I still think we should define the
> concept of "usage power" better. It should be clearly defined for a
> driver author which setting they should pick for a given request. This
> removes surprises for consumers and guessing for lowlevel driver
> authors. Also a uniform policy brings conflicts better to light.
> (Something like driver A does the right thing for consumer C and driver
> B makes it right for D. But once D tries to use A things break. (And
> then maybe A is changed to fit D, and C doesn't object because they
> don't read the pwm list resulting in a breakage for C later.))

I added documentation and comments to the header file as a first step
but we can always improve them.

Thanks,
Clemens

2021-05-07 17:51:49

[permalink] [raw]

Subject: [PATCH 2/4] pwm: pca9685: Support new usage_power setting in PWM state

If usage_power is set, the pca9685 driver will phase shift the
individual channels relative to their channel number. This improves EMI
because the enabled channels no longer turn on at the same time, while
still maintaining the configured duty cycle / power output.

Signed-off-by: Clemens Gruber <[email protected]>
---
drivers/pwm/pwm-pca9685.c | 61 +++++++++++++++++++++++++++++----------
1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/pwm-pca9685.c b/drivers/pwm/pwm-pca9685.c
index 7c9f174de64e..20dd579297e6 100644
--- a/drivers/pwm/pwm-pca9685.c
+++ b/drivers/pwm/pwm-pca9685.c
@@ -93,48 +93,77 @@ static inline struct pca9685 *to_pca(struct pwm_chip *chip)
/* Helper function to set the duty cycle ratio to duty/4096 (e.g. duty=2048 -> 50%) */
static void pca9685_pwm_set_duty(struct pca9685 *pca, int channel, unsigned int duty)
{
+ struct pwm_device *pwm = &pca->chip.pwms[channel];
+ unsigned int on, off;
+
if (duty == 0) {
/* Set the full OFF bit, which has the highest precedence */
regmap_write(pca->regmap, REG_OFF_H(channel), LED_FULL);
+ return;
} 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);
+ return;
}
+
+
+ if (pwm->state.usage_power && channel < PCA9685_MAXCHAN) {
+ /*
+ * If usage_power is set, the pca9685 driver will phase shift
+ * the individual channels relative to their channel number.
+ * This improves EMI because the enabled channels no longer
+ * turn on at the same time, while still maintaining the
+ * configured duty cycle / power output.
+ */
+ on = channel * PCA9685_COUNTER_RANGE / PCA9685_MAXCHAN;
+ } else
+ on = 0;
+
+ off = (on + duty) % PCA9685_COUNTER_RANGE;
+
+ /* Set ON time (clears full ON bit) */
+ regmap_write(pca->regmap, REG_ON_L(channel), on & 0xff);
+ regmap_write(pca->regmap, REG_ON_H(channel), (on >> 8) & 0xf);
+ /* Set OFF time (clears full OFF bit) */
+ regmap_write(pca->regmap, REG_OFF_L(channel), off & 0xff);
+ regmap_write(pca->regmap, REG_OFF_H(channel), (off >> 8) & 0xf);
}

static unsigned int pca9685_pwm_get_duty(struct pca9685 *pca, int channel)
{
- unsigned int off_h = 0, val = 0;
+ struct pwm_device *pwm = &pca->chip.pwms[channel];
+ unsigned int off = 0, on = 0, val = 0;

if (WARN_ON(channel >= PCA9685_MAXCHAN)) {
/* HW does not support reading state of "all LEDs" channel */
return 0;
}

- regmap_read(pca->regmap, LED_N_OFF_H(channel), &off_h);
- if (off_h & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_OFF_H(channel), &off);
+ if (off & LED_FULL) {
/* Full OFF bit is set */
return 0;
}

- regmap_read(pca->regmap, LED_N_ON_H(channel), &val);
- if (val & LED_FULL) {
+ regmap_read(pca->regmap, LED_N_ON_H(channel), &on);
+ if (on & LED_FULL) {
/* Full ON bit is set */
return PCA9685_COUNTER_RANGE;
}

- if (regmap_read(pca->regmap, LED_N_OFF_L(channel), &val)) {
- /* Reset val to 0 in case reading LED_N_OFF_L failed */
+ regmap_read(pca->regmap, LED_N_OFF_L(channel), &val);
+ off = ((off & 0xf) << 8) | (val & 0xff);
+ if (!pwm->state.usage_power)
+ return off;
+
+ /* Read ON register to calculate duty cycle of staggered output */
+ if (regmap_read(pca->regmap, LED_N_ON_L(channel), &val)) {
+ /* Reset val to 0 in case reading LED_N_ON_L failed */
val = 0;
}
- return ((off_h & 0xf) << 8) | (val & 0xff);
+ on = ((on & 0xf) << 8) | (val & 0xff);
+ return (off - on) & (PCA9685_COUNTER_RANGE - 1);
}

#if IS_ENABLED(CONFIG_GPIOLIB)
@@ -441,9 +470,11 @@ static int pca9685_pwm_probe(struct i2c_client *client,
reg &= ~(MODE1_ALLCALL | MODE1_SUB1 | MODE1_SUB2 | MODE1_SUB3);
regmap_write(pca->regmap, PCA9685_MODE1, reg);

- /* Reset OFF registers to POR default */
+ /* Reset OFF/ON registers to POR default */
regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_L, LED_FULL);
regmap_write(pca->regmap, PCA9685_ALL_LED_OFF_H, LED_FULL);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_L, 0);
+ regmap_write(pca->regmap, PCA9685_ALL_LED_ON_H, 0);

pca->chip.ops = &pca9685_pwm_ops;
/* Add an extra channel for ALL_LED */
--
2.31.1

2021-05-07 23:20:18

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hello Clemens,

On Fri, May 07, 2021 at 05:47:58PM +0200, Clemens Gruber wrote:
> On Fri, May 07, 2021 at 05:03:17PM +0200, Uwe Kleine-K?nig wrote:
> > On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index 5bb90af4997e..5a73251d28e3 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -54,12 +54,17 @@ enum {
> > > * @duty_cycle: PWM duty cycle (in nanoseconds)
> > > * @polarity: PWM polarity
> > > * @enabled: PWM enabled status
> > > + * @usage_power: If set, the PWM driver is only required to maintain the power
> > > + * output but has more freedom regarding signal form.
> > > + * If supported, the signal can be optimized, for example to
> > > + * improve EMI by phase shifting individual channels.
> > > */
> > > struct pwm_state {
> > > u64 period;
> > > u64 duty_cycle;
> > > enum pwm_polarity polarity;
> > > bool enabled;
> > > + bool usage_power;
> > > };
> > >
> > > /**
> >
> > If we really want to support this usecase, I would prefer to not have it
> > in pwm_state because this concept is not a property of the wave form. So
> > for a driver it doesn't really make sense to set this flag in
> > .get_state().
>
> It is related to the wave form in so far as it allows the driver to
> change the wave form as long as the power output remains the same.

Yes, the thing I wanted to express is: usage_power is a software thing.
Just from reading the hardware registers you can never know if
usage_power is set or not. So it is conceptually slightly different than
the other members of pwm_state which all are represented 1:1 in
hardware.

> > Maybe it makes more sense to put this in a separate argument for a
> > variant of pwm_apply_state? something like:
> >
> > int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);
> >
> > and pwm_state_transition can then contain something like this usage
> > power concept and maybe also a sync flag that requests that the call
> > should only return when the new setting is active and maybe also a
> > complete_period flag that requests that the currently running period
> > must be completed before the requested setting is implemented.
> >
> > OTOH the information "I only care about the relative duty cycle" is
> > relevant longer than during the transition to a new setting. (So if
> > there are two consumers and one specified to be only interested in the
> > relative duty cycle, the other might be allowed to change the common
> > period.)
>
> As you said, usage_power does not only apply to one state transition.
>
> > Having said that, I don't like the proposed names. Neither "usage_power"
> > nor "pwm_apply_state_transition".
> >
> > In a non-representative survey among two hardware engineers and one
> > software engineer who already contributed to PWM (!= me) I found out
> > that these three have an intuitive right understanding of
> > "allow-phase-shift" but have no idea what "usage-power" could mean.
>
> One advantage of usage_power is that it is not limited to phase
> shifting. Drivers could do other optimizations as long as the power
> output remains the same.

Freedom you give to the lowlevel driver might be a burden to the
consumer. I think it makes sense to split the concept into:

PWM_ALLOW_PHASE_SHIFT 1
PWM_SET_RELATIVE_DUTY 2
PWM_SET_POWER (PWM_ALLOW_PHASE_SHIFT | PWM_SET_RELATIVE_DUTY)

This way a consumer (e.g. a clock driver) who doesn't care about the
phase shift but wants a fixed period can specify that, and if a consumer
really only cares about the emitted power that's possible, too.

And given that your driver actually only implements a phase shift I
suggest not to generalize more than necessary here; also because the
semantic of usage-power isn't well defined. So this is something where I
agree to Thierry: Let's not solve a problem we don't have. (Though he
comes to a different conclusion here.)

> > On a side note: The hardware guys noted that it might make sense to
> > align the shifted phases. i.e. instead of shifting channel i by i *
> > period/16, it might be better to let the 2nd channel get active when the
> > first gets inactive. (i.e. try to keep the count of active channels
> > constant).
>
> I am not sure what you mean exactly, because the duty cycles of the
> 16 outputs are not necessarily the same and can all be active at the
> same time. The idea is to spread the edges out as evenly as possible.
> Shifting them by period/16 is the best way to smoothen the current
> spikes in my opinion and the implementation is also very simple.

Yes, the algorithm needed to satisfy this wish is more complicated. And
maybe it even isn't possible to implement it in a sane way, I didn't
think about it much. I just believed them that if you have two channels
that run at 50% it's better to have a phase shift of 50% between them
than 6.25%. Maybe it makes sense to align the start of channel #i+1 (if
allowed) to the end of channel #i to already get a better behaviour on
average than the fixed offset depending on the channel number.

> > And as already mentioned earlier I still think we should define the
> > concept of "usage power" better. It should be clearly defined for a
> > driver author which setting they should pick for a given request. This
> > removes surprises for consumers and guessing for lowlevel driver
> > authors. Also a uniform policy brings conflicts better to light.
> > (Something like driver A does the right thing for consumer C and driver
> > B makes it right for D. But once D tries to use A things break. (And
> > then maybe A is changed to fit D, and C doesn't object because they
> > don't read the pwm list resulting in a breakage for C later.))
>
> I added documentation and comments to the header file as a first step
> but we can always improve them.

In my book the documentation is inadequate because it doesn't define how
a driver should behave and so doesn't define what the consumer can
expect. With your description all settings I suggested in
https://lore.kernel.org/r/[email protected]
are allowed. I don't think this is a good idea.

And given that all people I talked to about "usage-power" were unable to
correctly guess its semantic, I'm convinced that we need a better name.
This is something you cannot outweigh with documentation -- most people
won't read it anyway.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (6.70 kB)
signature.asc (499.00 B)
Download all attachments

2021-05-31 17:29:50

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

On Sat, May 08, 2021 at 01:18:31AM +0200, Uwe Kleine-K?nig wrote:
> Hello Clemens,
>
> On Fri, May 07, 2021 at 05:47:58PM +0200, Clemens Gruber wrote:
> > On Fri, May 07, 2021 at 05:03:17PM +0200, Uwe Kleine-K?nig wrote:
> > > On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > > index 5bb90af4997e..5a73251d28e3 100644
> > > > --- a/include/linux/pwm.h
> > > > +++ b/include/linux/pwm.h
> > > > @@ -54,12 +54,17 @@ enum {
> > > > * @duty_cycle: PWM duty cycle (in nanoseconds)
> > > > * @polarity: PWM polarity
> > > > * @enabled: PWM enabled status
> > > > + * @usage_power: If set, the PWM driver is only required to maintain the power
> > > > + * output but has more freedom regarding signal form.
> > > > + * If supported, the signal can be optimized, for example to
> > > > + * improve EMI by phase shifting individual channels.
> > > > */
> > > > struct pwm_state {
> > > > u64 period;
> > > > u64 duty_cycle;
> > > > enum pwm_polarity polarity;
> > > > bool enabled;
> > > > + bool usage_power;
> > > > };
> > > >
> > > > /**
> > >
> > > If we really want to support this usecase, I would prefer to not have it
> > > in pwm_state because this concept is not a property of the wave form. So
> > > for a driver it doesn't really make sense to set this flag in
> > > .get_state().
> >
> > It is related to the wave form in so far as it allows the driver to
> > change the wave form as long as the power output remains the same.
>
> Yes, the thing I wanted to express is: usage_power is a software thing.
> Just from reading the hardware registers you can never know if
> usage_power is set or not. So it is conceptually slightly different than
> the other members of pwm_state which all are represented 1:1 in
> hardware.
>
> > > Maybe it makes more sense to put this in a separate argument for a
> > > variant of pwm_apply_state? something like:
> > >
> > > int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);
> > >
> > > and pwm_state_transition can then contain something like this usage
> > > power concept and maybe also a sync flag that requests that the call
> > > should only return when the new setting is active and maybe also a
> > > complete_period flag that requests that the currently running period
> > > must be completed before the requested setting is implemented.
> > >
> > > OTOH the information "I only care about the relative duty cycle" is
> > > relevant longer than during the transition to a new setting. (So if
> > > there are two consumers and one specified to be only interested in the
> > > relative duty cycle, the other might be allowed to change the common
> > > period.)
> >
> > As you said, usage_power does not only apply to one state transition.
> >
> > > Having said that, I don't like the proposed names. Neither "usage_power"
> > > nor "pwm_apply_state_transition".
> > >
> > > In a non-representative survey among two hardware engineers and one
> > > software engineer who already contributed to PWM (!= me) I found out
> > > that these three have an intuitive right understanding of
> > > "allow-phase-shift" but have no idea what "usage-power" could mean.
> >
> > One advantage of usage_power is that it is not limited to phase
> > shifting. Drivers could do other optimizations as long as the power
> > output remains the same.
>
> Freedom you give to the lowlevel driver might be a burden to the
> consumer. I think it makes sense to split the concept into:
>
> PWM_ALLOW_PHASE_SHIFT 1
> PWM_SET_RELATIVE_DUTY 2
> PWM_SET_POWER (PWM_ALLOW_PHASE_SHIFT | PWM_SET_RELATIVE_DUTY)
>
> This way a consumer (e.g. a clock driver) who doesn't care about the
> phase shift but wants a fixed period can specify that, and if a consumer
> really only cares about the emitted power that's possible, too.
>
> And given that your driver actually only implements a phase shift I
> suggest not to generalize more than necessary here; also because the
> semantic of usage-power isn't well defined. So this is something where I
> agree to Thierry: Let's not solve a problem we don't have. (Though he
> comes to a different conclusion here.)
>
> > > On a side note: The hardware guys noted that it might make sense to
> > > align the shifted phases. i.e. instead of shifting channel i by i *
> > > period/16, it might be better to let the 2nd channel get active when the
> > > first gets inactive. (i.e. try to keep the count of active channels
> > > constant).
> >
> > I am not sure what you mean exactly, because the duty cycles of the
> > 16 outputs are not necessarily the same and can all be active at the
> > same time. The idea is to spread the edges out as evenly as possible.
> > Shifting them by period/16 is the best way to smoothen the current
> > spikes in my opinion and the implementation is also very simple.
>
> Yes, the algorithm needed to satisfy this wish is more complicated. And
> maybe it even isn't possible to implement it in a sane way, I didn't
> think about it much. I just believed them that if you have two channels
> that run at 50% it's better to have a phase shift of 50% between them
> than 6.25%. Maybe it makes sense to align the start of channel #i+1 (if
> allowed) to the end of channel #i to already get a better behaviour on
> average than the fixed offset depending on the channel number.
>
> > > And as already mentioned earlier I still think we should define the
> > > concept of "usage power" better. It should be clearly defined for a
> > > driver author which setting they should pick for a given request. This
> > > removes surprises for consumers and guessing for lowlevel driver
> > > authors. Also a uniform policy brings conflicts better to light.
> > > (Something like driver A does the right thing for consumer C and driver
> > > B makes it right for D. But once D tries to use A things break. (And
> > > then maybe A is changed to fit D, and C doesn't object because they
> > > don't read the pwm list resulting in a breakage for C later.))
> >
> > I added documentation and comments to the header file as a first step
> > but we can always improve them.
>
> In my book the documentation is inadequate because it doesn't define how
> a driver should behave and so doesn't define what the consumer can
> expect. With your description all settings I suggested in
> https://lore.kernel.org/r/[email protected]
> are allowed. I don't think this is a good idea.
>
> And given that all people I talked to about "usage-power" were unable to
> correctly guess its semantic, I'm convinced that we need a better name.
> This is something you cannot outweigh with documentation -- most people
> won't read it anyway.

Thierry: Would be great to get your input on this.

Thanks,
Clemens

2021-06-04 09:47:34

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

On Fri, 7 May 2021 15:18:42 +0200, Clemens Gruber wrote:
> If usage_power is set, the PWM driver is only required to maintain
> the power output but has more freedom regarding signal form.
>
> If supported, the signal can be optimized, for example to
> improve EMI by phase shifting individual channels.

Applied, thanks!

[1/4] pwm: core: Support new usage_power setting in PWM state
commit: 9e40ee18a1dc1623a5368d6232aaed52fd29dada
[2/4] pwm: pca9685: Support new usage_power setting in PWM state
commit: ae16db1fd3a1b8d1713ba6af5cf27be32918d2b8
[3/4] pwm: pca9685: Restrict period change for enabled PWMs
commit: 6d6e7050276d40b5de97aa950d5d71057f2e2a25
[4/4] pwm: pca9685: Add error messages for failed regmap calls
commit: 79dd354fe1769ebec695dacfee007eafb1538d0c

Best regards,
--
Thierry Reding <[email protected]>

2021-06-04 09:51:12

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

On Mon, May 31, 2021 at 06:12:32PM +0200, Clemens Gruber wrote:
> On Sat, May 08, 2021 at 01:18:31AM +0200, Uwe Kleine-König wrote:
> > Hello Clemens,
> >
> > On Fri, May 07, 2021 at 05:47:58PM +0200, Clemens Gruber wrote:
> > > On Fri, May 07, 2021 at 05:03:17PM +0200, Uwe Kleine-König wrote:
> > > > On Fri, May 07, 2021 at 03:18:42PM +0200, Clemens Gruber wrote:
> > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > > > index 5bb90af4997e..5a73251d28e3 100644
> > > > > --- a/include/linux/pwm.h
> > > > > +++ b/include/linux/pwm.h
> > > > > @@ -54,12 +54,17 @@ enum {
> > > > > * @duty_cycle: PWM duty cycle (in nanoseconds)
> > > > > * @polarity: PWM polarity
> > > > > * @enabled: PWM enabled status
> > > > > + * @usage_power: If set, the PWM driver is only required to maintain the power
> > > > > + * output but has more freedom regarding signal form.
> > > > > + * If supported, the signal can be optimized, for example to
> > > > > + * improve EMI by phase shifting individual channels.
> > > > > */
> > > > > struct pwm_state {
> > > > > u64 period;
> > > > > u64 duty_cycle;
> > > > > enum pwm_polarity polarity;
> > > > > bool enabled;
> > > > > + bool usage_power;
> > > > > };
> > > > >
> > > > > /**
> > > >
> > > > If we really want to support this usecase, I would prefer to not have it
> > > > in pwm_state because this concept is not a property of the wave form. So
> > > > for a driver it doesn't really make sense to set this flag in
> > > > .get_state().
> > >
> > > It is related to the wave form in so far as it allows the driver to
> > > change the wave form as long as the power output remains the same.
> >
> > Yes, the thing I wanted to express is: usage_power is a software thing.
> > Just from reading the hardware registers you can never know if
> > usage_power is set or not. So it is conceptually slightly different than
> > the other members of pwm_state which all are represented 1:1 in
> > hardware.
> >
> > > > Maybe it makes more sense to put this in a separate argument for a
> > > > variant of pwm_apply_state? something like:
> > > >
> > > > int pwm_apply_state_transition(struct pwm_device *pwm, const struct pwm_state *state, const struct pwm_state_transition *transition);
> > > >
> > > > and pwm_state_transition can then contain something like this usage
> > > > power concept and maybe also a sync flag that requests that the call
> > > > should only return when the new setting is active and maybe also a
> > > > complete_period flag that requests that the currently running period
> > > > must be completed before the requested setting is implemented.
> > > >
> > > > OTOH the information "I only care about the relative duty cycle" is
> > > > relevant longer than during the transition to a new setting. (So if
> > > > there are two consumers and one specified to be only interested in the
> > > > relative duty cycle, the other might be allowed to change the common
> > > > period.)
> > >
> > > As you said, usage_power does not only apply to one state transition.
> > >
> > > > Having said that, I don't like the proposed names. Neither "usage_power"
> > > > nor "pwm_apply_state_transition".
> > > >
> > > > In a non-representative survey among two hardware engineers and one
> > > > software engineer who already contributed to PWM (!= me) I found out
> > > > that these three have an intuitive right understanding of
> > > > "allow-phase-shift" but have no idea what "usage-power" could mean.
> > >
> > > One advantage of usage_power is that it is not limited to phase
> > > shifting. Drivers could do other optimizations as long as the power
> > > output remains the same.
> >
> > Freedom you give to the lowlevel driver might be a burden to the
> > consumer. I think it makes sense to split the concept into:
> >
> > PWM_ALLOW_PHASE_SHIFT 1
> > PWM_SET_RELATIVE_DUTY 2
> > PWM_SET_POWER (PWM_ALLOW_PHASE_SHIFT | PWM_SET_RELATIVE_DUTY)
> >
> > This way a consumer (e.g. a clock driver) who doesn't care about the
> > phase shift but wants a fixed period can specify that, and if a consumer
> > really only cares about the emitted power that's possible, too.
> >
> > And given that your driver actually only implements a phase shift I
> > suggest not to generalize more than necessary here; also because the
> > semantic of usage-power isn't well defined. So this is something where I
> > agree to Thierry: Let's not solve a problem we don't have. (Though he
> > comes to a different conclusion here.)
> >
> > > > On a side note: The hardware guys noted that it might make sense to
> > > > align the shifted phases. i.e. instead of shifting channel i by i *
> > > > period/16, it might be better to let the 2nd channel get active when the
> > > > first gets inactive. (i.e. try to keep the count of active channels
> > > > constant).
> > >
> > > I am not sure what you mean exactly, because the duty cycles of the
> > > 16 outputs are not necessarily the same and can all be active at the
> > > same time. The idea is to spread the edges out as evenly as possible.
> > > Shifting them by period/16 is the best way to smoothen the current
> > > spikes in my opinion and the implementation is also very simple.
> >
> > Yes, the algorithm needed to satisfy this wish is more complicated. And
> > maybe it even isn't possible to implement it in a sane way, I didn't
> > think about it much. I just believed them that if you have two channels
> > that run at 50% it's better to have a phase shift of 50% between them
> > than 6.25%. Maybe it makes sense to align the start of channel #i+1 (if
> > allowed) to the end of channel #i to already get a better behaviour on
> > average than the fixed offset depending on the channel number.
> >
> > > > And as already mentioned earlier I still think we should define the
> > > > concept of "usage power" better. It should be clearly defined for a
> > > > driver author which setting they should pick for a given request. This
> > > > removes surprises for consumers and guessing for lowlevel driver
> > > > authors. Also a uniform policy brings conflicts better to light.
> > > > (Something like driver A does the right thing for consumer C and driver
> > > > B makes it right for D. But once D tries to use A things break. (And
> > > > then maybe A is changed to fit D, and C doesn't object because they
> > > > don't read the pwm list resulting in a breakage for C later.))
> > >
> > > I added documentation and comments to the header file as a first step
> > > but we can always improve them.
> >
> > In my book the documentation is inadequate because it doesn't define how
> > a driver should behave and so doesn't define what the consumer can
> > expect. With your description all settings I suggested in
> > https://lore.kernel.org/r/[email protected]
> > are allowed. I don't think this is a good idea.
> >
> > And given that all people I talked to about "usage-power" were unable to
> > correctly guess its semantic, I'm convinced that we need a better name.
> > This is something you cannot outweigh with documentation -- most people
> > won't read it anyway.
>
> Thierry: Would be great to get your input on this.

In the interest of making forward progress, I've applied this series.
The good thing is that with the current proposal none of this leaks into
ABI, so nothing is set in stone. If ever somebody comes up with better
names we can change them.

Thierry

Attachments:

(No filename) (7.48 kB)
signature.asc (849.00 B)
Download all attachments

2021-06-07 06:12:42

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hello Thierry,

On Fri, Jun 04, 2021 at 11:49:37AM +0200, Thierry Reding wrote:
> In the interest of making forward progress, I've applied this series.

I proposed a different approach that in contrast to usage_power:

- is well defined
(so driver authors and consumers know what to provide or expect resp.);
- has good name people understand without reading documentation;
- fully covers the problem Clemens want to address;
- fully covers what the only implementing lowlevel driver does; and
- is easy to implement based on the patches in this series

This is not what I call "forward progress". I take it personal that
after I pointed out technical shortcomings with this patch set and even
suggested a better concept, you didn't even made the effort to argue
but instead simply went on applying the patches.

Can you please point out what I'm missing?

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (1.04 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-07 14:43:06

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

On Mon, Jun 07, 2021 at 08:08:27AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Fri, Jun 04, 2021 at 11:49:37AM +0200, Thierry Reding wrote:
> > In the interest of making forward progress, I've applied this series.
>
> I proposed a different approach that in contrast to usage_power:
>
> - is well defined
> (so driver authors and consumers know what to provide or expect resp.);
> - has good name people understand without reading documentation;
> - fully covers the problem Clemens want to address;
> - fully covers what the only implementing lowlevel driver does; and
> - is easy to implement based on the patches in this series
>
> This is not what I call "forward progress". I take it personal that
> after I pointed out technical shortcomings with this patch set and even
> suggested a better concept, you didn't even made the effort to argue
> but instead simply went on applying the patches.

Forward progress doesn't always mean that everybody gets their way. And
this is nothing personal, so please don't take it that way.

I don't see where you pointed out technical shortcomings with this
patch, you merely pointed out that you don't like this solution and that
there might be a better way to implement it by expanding on the concepts
introduced in this patch series.

As I said, this is now no longer impacting ABI, so we can improve on
this further down the road if we choose to. However, I didn't see any
reason to postpone this any further. This is something that Clemens has
been working on for more than half a year and we've changed our minds
often enough. In my opinion this proposal is good enough.

Thierry

Attachments:

(No filename) (1.65 kB)
signature.asc (849.00 B)
Download all attachments

2021-06-07 20:42:56

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hello Thierry,

On Mon, Jun 07, 2021 at 04:40:14PM +0200, Thierry Reding wrote:
> On Mon, Jun 07, 2021 at 08:08:27AM +0200, Uwe Kleine-K?nig wrote:
> > Hello Thierry,
> >
> > On Fri, Jun 04, 2021 at 11:49:37AM +0200, Thierry Reding wrote:
> > > In the interest of making forward progress, I've applied this series.
> >
> > I proposed a different approach that in contrast to usage_power:
> >
> > - is well defined
> > (so driver authors and consumers know what to provide or expect resp.);
> > - has good name people understand without reading documentation;
> > - fully covers the problem Clemens want to address;
> > - fully covers what the only implementing lowlevel driver does; and
> > - is easy to implement based on the patches in this series
> >
> > This is not what I call "forward progress". I take it personal that
> > after I pointed out technical shortcomings with this patch set and even
> > suggested a better concept, you didn't even made the effort to argue
> > but instead simply went on applying the patches.
>
> Forward progress doesn't always mean that everybody gets their way.

Do you agree that the usage power flag introduced here isn't well
defined? If you think it is, tell me, what is the maximal and minimal
period a consumer has to expect when .period = 10000 ns is requested.
Assume you have a driver (think pwm-gpio) where a longer period means
more power savings. What is "the reasonable" period that the driver
should configure?

Do you agree that in contrast to usage-power allow-phase-shift is well
defined?

Did you ask people in your bubble what they expect from a usage power
flag for a PWM without first explaining what it does? Did you try to ask
an internet search engine what it suggests when searching for "PWM usage
power"?

Do you agree that allow-phase-shift is good enough to solve Clemens'
problem?

Either you give completely other answers than I do or you don't consider
it bad that consumers don't know what they can expect and that names are
unintuitive.

My problem is not that in the end a solution is picked that wasn't my
favourite. My problem is that I have the impression my arguments were
not considered but simply ignored.

> And this is nothing personal, so please don't take it that way.

If this isn't personal (which is great) then it's a decision that is (at
least for me) obviously wrong and ignoring the good arguments against
this choice without any relevant advantages compared to my suggested
solution. I have problems to not take this personal.

> I don't see where you pointed out technical shortcomings with this
> patch, you merely pointed out that you don't like this solution and that
> there might be a better way to implement it by expanding on the concepts
> introduced in this patch series.

Either you didn't read my mail at all, or you have a different idea
about what you consider a relevant argument. (Well, or you don't care
that your choice is bad. I hope this is only a theoretical possibility.)
Being well defined and having an intuitive name belong into this
"relevant" category in my book.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (3.26 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-09 20:43:51

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

Hello Thierry,

On Mon, Jun 07, 2021 at 08:51:58PM +0200, Uwe Kleine-K?nig wrote:
> My problem is not that in the end a solution is picked that wasn't my
> favourite. My problem is that I have the impression my arguments were
> not considered but simply ignored.

Another thing that annoys me is that there are currently ~20 open
patches by me in patchwork, most of them are easy to understand cleanups
and fixes, most of them are older than Clemens' series and most of them
are uncommented by you. And in this situation you apply the only
controversial series.

Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (740.00 B)
signature.asc (499.00 B)
Download all attachments

2021-06-10 13:12:54

[permalink] [raw]

Subject: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM state

On Wed, Jun 09, 2021 at 10:41:44PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
>
> On Mon, Jun 07, 2021 at 08:51:58PM +0200, Uwe Kleine-König wrote:
> > My problem is not that in the end a solution is picked that wasn't my
> > favourite. My problem is that I have the impression my arguments were
> > not considered but simply ignored.
>
> Another thing that annoys me is that there are currently ~20 open
> patches by me in patchwork, most of them are easy to understand cleanups
> and fixes, most of them are older than Clemens' series and most of them
> are uncommented by you. And in this situation you apply the only
> controversial series.

Clemens' series is actually older than those cleanups because it's been
in the works for many months now. And the reason why I'm prioritizing
Clemens' series is because it has broader impact and I want to make sure
it gets maximum soaking time in linux-next. Small fixes and cleanups are
less likely to break things, so I'm going to apply them after.

Thierry

Attachments:

(No filename) (1.01 kB)
signature.asc (849.00 B)
Download all attachments

2021-06-21 06:53:06

[permalink] [raw]

Subject: PWM-Patches for next merge window [Was: Re: [PATCH 1/4] pwm: core: Support new usage_power setting in PWM] state

Hello Thierry,

On Thu, Jun 10, 2021 at 03:11:40PM +0200, Thierry Reding wrote:
> On Wed, Jun 09, 2021 at 10:41:44PM +0200, Uwe Kleine-K?nig wrote:
> > On Mon, Jun 07, 2021 at 08:51:58PM +0200, Uwe Kleine-K?nig wrote:
> > > My problem is not that in the end a solution is picked that wasn't my
> > > favourite. My problem is that I have the impression my arguments were
> > > not considered but simply ignored.
> >
> > Another thing that annoys me is that there are currently ~20 open
> > patches by me in patchwork, most of them are easy to understand cleanups
> > and fixes, most of them are older than Clemens' series and most of them
> > are uncommented by you. And in this situation you apply the only
> > controversial series.
>
> Clemens' series is actually older than those cleanups because it's been
> in the works for many months now. And the reason why I'm prioritizing
> Clemens' series is because it has broader impact and I want to make sure
> it gets maximum soaking time in linux-next.

Hmm, I would have done that differently, but I don't expect that you
expected agreement from my side.

> Small fixes and cleanups are less likely to break things, so I'm going
> to apply them after.

I wonder what's the ideal delay for fixes and cleanups in your book.
We're two weeks after merging that high-impact patch series and Linus
just released -rc7.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |

Attachments:

(No filename) (1.54 kB)
signature.asc (499.00 B)
Download all attachments

2021-06-25 09:56:48