RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

(If you have a déjà vu: Yes, I tried to argue this case already in the
past, it's just that I'm just faced with a new use case that's broken
with the current state.)

Currently the idiom

	pwm_config(mypwm, value, period);
	if (!value)
		pwm_disable(mypwm);
	else
		pwm_enable(mypwm);

(and it's equivalent:

	state.duty_cycle = ...
	state.enable = state.duty_cycle ? true : false;
	pwm_apply_state(...);

) is quite present in the kernel.

In my eyes this is bad for two reasons:

 a) if the caller is supposed to call pwm_disable() after configuring the
    duty cycle to zero, then why doesn't pwm_config() contains this to
    unburden pwm-API users and so reduce open coding this assumption?

 b) it is actually wrong in at least two cases:
    - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
      and only after the current period is elapsed this is clocked into
      the hardware. So it is very likely that when pwm_disable() is
      called the period is still not over, and then the clk is disabled
      and the period never ends resulting in the pwm pin being frozen in
      whatever state it happend to be when the clk was stopped.
    - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
      correctly ensures the PWM pin to stay at 1. But then pwm_disable()
      puts it to 0. In the current case this implies that the backlight
      is fully enabled instead of off.

Both issues are hard (or at least ugly) to fix. The IMHO most sane
approach is to not let PWM-API-users care about power saving when they
still care about the output level and update the samantic of
pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
toggling, I don't care about the level of the output." in contrast to
the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
called before, stop the pin at (maybe inverted) low level (for most pwms
at least).".

This is in my eyes more clear, removes code duplication and power saving
can (and should) be implemented in the pwm backend drivers that have the
domain specific knowledge about the effects of pwm_disable and
eventually unwanted side effects when duty cycle is 0 and just do the
corresponding action if its safe. We could even put something like:

	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
		pwm_disable(pwm);

in pwm_config to help backend driver authors of "sane" PWMs.

I grepped in the kernel for callers of pwm_disable and adapted them for
this. I didn't verify all changes are correct, so take with a grain of
salt. The only fishy one is drivers/gpu/drm/i915/intel_panel.c in my
eyes, I'm not sure about drivers/pwm/pwm-lpc18xx-sct.c.

See below for my wip changes.

Looking forward to your comments.

Best regards
Uwe

diff --git a/arch/arm/mach-s3c24xx/mach-rx1950.c b/arch/arm/mach-s3c24xx/mach-rx1950.c
index 7f5a18fa305b..55db3b63add8 100644
--- a/arch/arm/mach-s3c24xx/mach-rx1950.c
+++ b/arch/arm/mach-s3c24xx/mach-rx1950.c
@@ -429,7 +429,6 @@ static void rx1950_lcd_power(int enable)
 		/* GPB1->OUTPUT, GPB1->0 */
 		gpio_direction_output(S3C2410_GPB(1), 0);
 		pwm_config(lcd_pwm, 0, LCD_PWM_PERIOD);
-		pwm_disable(lcd_pwm);
 
 		/* GPC0->0, GPC10->0 */
 		gpio_direction_output(S3C2410_GPC(0), 0);
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index b443278e569c..a2b0e5f74ac8 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -784,8 +784,6 @@ static void pwm_disable_backlight(const struct drm_connector_state *old_conn_sta
 
 	/* Disable the backlight */
 	pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
-	usleep_range(2000, 3000);
-	pwm_disable(panel->backlight.pwm);
 }
 
 void intel_panel_disable_backlight(const struct drm_connector_state *old_conn_state)
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7838af58f92d..0f1b597bb252 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -51,7 +51,7 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 	pwm_init_state(ctx->pwm, &state);
 	period = ctx->pwm->args.period;
 	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-	state.enabled = pwm ? true : false;
+	state.enabled = true;
 
 	ret = pwm_apply_state(ctx->pwm, &state);
 	if (!ret)
@@ -244,8 +244,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 						       ctx, pwm_fan_groups);
 	if (IS_ERR(hwmon)) {
 		dev_err(&pdev->dev, "Failed to register hwmon device\n");
-		ret = PTR_ERR(hwmon);
-		goto err_pwm_disable;
+		return PTR_ERR(hwmon);
 	}
 
 	ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx);
@@ -260,20 +259,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		if (IS_ERR(cdev)) {
 			dev_err(&pdev->dev,
 				"Failed to register pwm-fan as cooling device");
-			ret = PTR_ERR(cdev);
-			goto err_pwm_disable;
+			return PTR_ERR(cdev);
 		}
 		ctx->cdev = cdev;
 		thermal_cdev_update(cdev);
 	}
 
 	return 0;
-
-err_pwm_disable:
-	state.enabled = false;
-	pwm_apply_state(ctx->pwm, &state);
-
-	return ret;
 }
 
 static int pwm_fan_remove(struct platform_device *pdev)
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index df80c89ebe7f..755e13721a16 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -40,11 +40,6 @@ static void __led_pwm_set(struct led_pwm_data *led_dat)
 	int new_duty = led_dat->duty;
 
 	pwm_config(led_dat->pwm, new_duty, led_dat->period);
-
-	if (new_duty == 0)
-		pwm_disable(led_dat->pwm);
-	else
-		pwm_enable(led_dat->pwm);
 }
 
 static int led_pwm_set(struct led_classdev *led_cdev,
@@ -105,6 +100,8 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		return ret;
 	}
 
+	pwm_enable(led_data->pwm);
+
 	led_data->cdev.brightness_set_blocking = led_pwm_set;
 
 	/*

(This should better make use of pwm_state instead.)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..38f17c8b6364 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -864,6 +864,8 @@ void pwm_put(struct pwm_device *pwm)
 	if (!pwm)
 		return;
 
+	pwm_disable(pwm);
+
 	mutex_lock(&pwm_lock);
 
 	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index d7f5f7de030d..61125052ac3d 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -306,8 +306,6 @@ static void lpc18xx_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct lpc18xx_pwm_chip *lpc18xx_pwm = to_lpc18xx_pwm_chip(chip);
 	struct lpc18xx_pwm_data *lpc18xx_data = pwm_get_chip_data(pwm);
 
-	pwm_disable(pwm);
-	pwm_set_duty_cycle(pwm, 0);
 	clear_bit(lpc18xx_data->duty_event, &lpc18xx_pwm->event_map);
 }
 
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 2030a6b77a09..293abb823fde 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -165,12 +165,15 @@ static void lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 {
 	unsigned int period = pchip->pdata->pwm_period;
 	unsigned int duty = br * period / br_max;
+	struct pwm_state state;
 
-	pwm_config(pchip->pwmd, duty, period);
-	if (duty)
-		pwm_enable(pchip->pwmd);
-	else
-		pwm_disable(pchip->pwmd);
+	pwm_get_state(pchip->pwmd);
+
+	state.duty_cycle = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(pchip->pwmd, &state);
 }
 
 /* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index 73612485ed07..ba4e71b99c45 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -240,6 +240,7 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 	unsigned int period = lp->pdata->period_ns;
 	unsigned int duty = br * period / max_br;
 	struct pwm_device *pwm;
+	struct pwm_state state;
 
 	/* request pwm device with the consumer name */
 	if (!lp->pwm) {
@@ -248,19 +249,15 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 			return;
 
 		lp->pwm = pwm;
-
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pwm);
 	}
 
-	pwm_config(lp->pwm, duty, period);
-	if (duty)
-		pwm_enable(lp->pwm);
-	else
-		pwm_disable(lp->pwm);
+	pwm_get_state(lp->pwm, &state);
+
+	state.duty = duty;
+	state.period = period;
+	state.enabled = true;
+
+	pwm_apply_state(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 44ac5bde4e9d..e60831f3e29a 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -46,18 +46,42 @@ struct pwm_bl_data {
 	void			(*exit)(struct device *);
 };
 
+static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
+{
+	unsigned int lth = pb->lth_brightness;
+	u64 duty_cycle;
+
+	if (pb->levels)
+		duty_cycle = pb->levels[brightness];
+	else
+		duty_cycle = brightness;
+
+	duty_cycle *= pb->period - lth;
+	do_div(duty_cycle, pb->scale);
+
+	return duty_cycle + lth;
+}
+
 static void pwm_backlight_power_on(struct pwm_bl_data *pb, int brightness)
 {
 	int err;
+	int duty_cycle;
+	struct pwm_state state;
 
 	if (pb->enabled)
 		return;
 
+	duty_cycle = compute_duty_cycle(pb, brightness);
+
 	err = regulator_enable(pb->power_supply);
 	if (err < 0)
 		dev_err(pb->dev, "failed to enable power supply\n");
 
-	pwm_enable(pb->pwm);
+	pwm_get_state(pb->pwm, &state);
+	state.duty_cycle = duty_cycle;
+	state.period = pb->period;
+	state.enabled = true;
+	pwm_apply_state(pb->pwm, &state);
 
 	if (pb->post_pwm_on_delay)
 		msleep(pb->post_pwm_on_delay);
@@ -80,33 +104,15 @@ static void pwm_backlight_power_off(struct pwm_bl_data *pb)
 		msleep(pb->pwm_off_delay);
 
 	pwm_config(pb->pwm, 0, pb->period);
-	pwm_disable(pb->pwm);
 
 	regulator_disable(pb->power_supply);
 	pb->enabled = false;
 }
 
-static int compute_duty_cycle(struct pwm_bl_data *pb, int brightness)
-{
-	unsigned int lth = pb->lth_brightness;
-	u64 duty_cycle;
-
-	if (pb->levels)
-		duty_cycle = pb->levels[brightness];
-	else
-		duty_cycle = brightness;
-
-	duty_cycle *= pb->period - lth;
-	do_div(duty_cycle, pb->scale);
-
-	return duty_cycle + lth;
-}
-
 static int pwm_backlight_update_status(struct backlight_device *bl)
 {
 	struct pwm_bl_data *pb = bl_get_data(bl);
 	int brightness = bl->props.brightness;
-	int duty_cycle;
 
 	if (bl->props.power != FB_BLANK_UNBLANK ||
 	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
@@ -116,11 +122,9 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 	if (pb->notify)
 		brightness = pb->notify(pb->dev, brightness);
 
-	if (brightness > 0) {
-		duty_cycle = compute_duty_cycle(pb, brightness);
-		pwm_config(pb->pwm, duty_cycle, pb->period);
+	if (brightness > 0)
 		pwm_backlight_power_on(pb, brightness);
-	} else
+	else
 		pwm_backlight_power_off(pb);
 
 	if (pb->notify_after)
@@ -353,12 +357,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "got pwm for backlight\n");
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to
-	 * the atomic PWM API.
-	 */
-	pwm_apply_args(pb->pwm);
-
 	/*
 	 * The DT case will set the pwm_period_ns field to 0 and store the
 	 * period, parsed from the DT, in the PWM device. For the non-DT case,
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3784 bytes --]

On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> (If you have a déjà vu: Yes, I tried to argue this case already in the
> past, it's just that I'm just faced with a new use case that's broken
> with the current state.)
> 
> Currently the idiom
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm);
> 	else
> 		pwm_enable(mypwm);
> 
> (and it's equivalent:
> 
> 	state.duty_cycle = ...
> 	state.enable = state.duty_cycle ? true : false;
> 	pwm_apply_state(...);
> 
> ) is quite present in the kernel.
> 
> In my eyes this is bad for two reasons:
> 
>  a) if the caller is supposed to call pwm_disable() after configuring the
>     duty cycle to zero, then why doesn't pwm_config() contains this to
>     unburden pwm-API users and so reduce open coding this assumption?

That's because it's not true in all cases. pwm_disable() is not
equivalent to setting the duty cycle to zero. Many users don't care
about the difference, but that doesn't mean it should be assumed to
be always the case.

>  b) it is actually wrong in at least two cases:
>     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
>       and only after the current period is elapsed this is clocked into
>       the hardware. So it is very likely that when pwm_disable() is
>       called the period is still not over, and then the clk is disabled
>       and the period never ends resulting in the pwm pin being frozen in
>       whatever state it happend to be when the clk was stopped.

So that's a driver bug then. By definition, after the call to
pwm_config() completes, the hardware should be in the configured state.
If a device doesn't allow the programming to happen immediately, then it
is up to the driver to ensure it waits long enough for the settings to
have been properly applied.

>     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
>       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
>       puts it to 0. In the current case this implies that the backlight
>       is fully enabled instead of off.

Again, that's a driver bug. pwm_disable() isn't supposed to change the
pin polarity. It's not supposed to change the pin level.

> Both issues are hard (or at least ugly) to fix. The IMHO most sane
> approach is to not let PWM-API-users care about power saving when they
> still care about the output level and update the samantic of
> pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
> toggling, I don't care about the level of the output." in contrast to
> the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
> called before, stop the pin at (maybe inverted) low level (for most pwms
> at least).".

I don't see how you could make that work. In practically all cases a
pwm_disable() would have to be assumed to cause the pin level to become
undefined. That makes it pretty much useless.

> This is in my eyes more clear, removes code duplication and power saving
> can (and should) be implemented in the pwm backend drivers that have the
> domain specific knowledge about the effects of pwm_disable and
> eventually unwanted side effects when duty cycle is 0 and just do the
> corresponding action if its safe. We could even put something like:
> 
> 	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
> 		pwm_disable(pwm);
> 
> in pwm_config to help backend driver authors of "sane" PWMs.

I agree that this is simpler and clearer. However it also significantly
reduces the flexibility of the API, and I'm not sure that it is enough.
Again, yes, for many cases this is sufficient, but it doesn't fully
expose the capabilities of hardware.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > past, it's just that I'm just faced with a new use case that's broken
> > with the current state.)
> > 
> > Currently the idiom
> > 
> > 	pwm_config(mypwm, value, period);
> > 	if (!value)
> > 		pwm_disable(mypwm);
> > 	else
> > 		pwm_enable(mypwm);
> > 
> > (and it's equivalent:
> > 
> > 	state.duty_cycle = ...
> > 	state.enable = state.duty_cycle ? true : false;
> > 	pwm_apply_state(...);
> > 
> > ) is quite present in the kernel.
> > 
> > In my eyes this is bad for two reasons:
> > 
> >  a) if the caller is supposed to call pwm_disable() after configuring the
> >     duty cycle to zero, then why doesn't pwm_config() contains this to
> >     unburden pwm-API users and so reduce open coding this assumption?
> 
> That's because it's not true in all cases. pwm_disable() is not
> equivalent to setting the duty cycle to zero. Many users don't care
> about the difference, but that doesn't mean it should be assumed to
> be always the case.

I cannot follow you. You're saying that pwm_disable() is not the same as
duty-cycle := 0. That's different from what I said. I only talked about
pwm_disable() after config(pwm, duty=0, period).

Actually the current semantic of pwm_disable isn't clear to me. Is it:
"Freeze the pin in the current state"? If so, this isn't possible to
implement for some hardware I know. If it's like that, the pin state is
undefined after pwm_disable unless the duty-cycle was 0% or 100%
before, right?

> >  b) it is actually wrong in at least two cases:
> >     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
> >       and only after the current period is elapsed this is clocked into
> >       the hardware. So it is very likely that when pwm_disable() is
> >       called the period is still not over, and then the clk is disabled
> >       and the period never ends resulting in the pwm pin being frozen in
> >       whatever state it happend to be when the clk was stopped.
> 
> So that's a driver bug then. By definition, after the call to
> pwm_config() completes, the hardware should be in the configured state.
> If a device doesn't allow the programming to happen immediately, then it
> is up to the driver to ensure it waits long enough for the settings to
> have been properly applied.

Unfortunately there is no way (that I know of at least) to notice when a
period is over, so only an unconditional delay/sleep is the way out
here.
 
> >     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
> >       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
> >       puts it to 0. In the current case this implies that the backlight
> >       is fully enabled instead of off.
> 
> Again, that's a driver bug. pwm_disable() isn't supposed to change the
> pin polarity. It's not supposed to change the pin level.

So the disable callback must look as follows:

	def imx_pwm_disable(pwm):
	    state = pwm_get_state(...)
	    if state.polarity == PWM_POLARITY_INVERSED:
	        return

	    hw_access_to_disable_pwm()

(Alternatively the pin could be configured to gpio, the right level set
and the hw disable unconditionally.)

And actually because for an inverted polarity after setting the
duty_cycle to 100% it is possible to already disable the hardware in the
config callback. And for the users that config for duty=0 but don't call
pwm_disable() the symmetric case could be handled, too, so the config
callback optimally looks like:

	def imx_pwm_config(chip, duty, period):
	    state = pwm_get_state(...)
	    if (duty == 0 and not state.polarity == PWM_POLARITY_INVERSED) ||
	       (duty == period and state.polarity == PWM_POLARITY_INVERSED):
	        hw_access_to_disable_pwm()
	    else:
	        hw_access_to_config(duty, period)

The obvious downsides are:

  a) if pwm_disable() was called because the user doesn't need the pwm to
     toggle any more, it stays enabled.
  b) pwm_disable() after pwm_config() that allows to disable the
     hardware, is a noop

So it would be much saner from the backend implementations POV if the
PWM-API user doesn't call pwm_disable() if he cares about the output
level of the pin. And I cannot imagine any pwm implementation where this
requirement would make the implementation worse.

Unless I miss something currently there is no user-visible effect of
calling pwm_disable after duty was set to 0. So as an API-user I cannot
differentiate between

 - set the output to low; and
 - don't care about the pwm pin state, turn off and save power

All downsides could be fixed if the only way to set the output to low
would be

	pwm_config(pwm, 0, period);

and pwm_disable() would imply to turn off with no guarantees about the pin
state.

This would simplify some drivers but not complicate any.

This would make the API more expressive because it allows to
differentiate between "output low" and "power off".

The only downside I see is that all users must be reviewed and
eventually fixed for the slightly different semantic. Parts of that was
already done in my patch in the previous mail.

> > Both issues are hard (or at least ugly) to fix. The IMHO most sane
> > approach is to not let PWM-API-users care about power saving when they
> > still care about the output level and update the samantic of
> > pwm_disable() (and struct pwm_state::enabled == 0) to mean: "Stop
> > toggling, I don't care about the level of the output." in contrast to
> > the current semantic "Stop toggling. If pwm_config(mypwm, 0, period) was
> > called before, stop the pin at (maybe inverted) low level (for most pwms
> > at least).".
> 
> I don't see how you could make that work. In practically all cases a
> pwm_disable() would have to be assumed to cause the pin level to become
> undefined. That makes it pretty much useless.

Right, that's what I want. pwm_disable() should make no guarantees about
the pin level. So only call it if you don't care about the pin state. If
you care, only do

	pwm_config(pwm, duty=0, period);

and let the backend driver save power if it possible already in the
corresponding callback.

That is actually easier and more expressive! Currently the semantic of
pwm_disable() is ambiguous. The two possible meanings are:

   - save power and keep the pin state
   - save power

and the backend driver cannot know which one the caller actually wants.
So the only way is to assume the stronger one and keep the pin state
which results in more power usage for some hardware.

> > This is in my eyes more clear, removes code duplication and power saving
> > can (and should) be implemented in the pwm backend drivers that have the
> > domain specific knowledge about the effects of pwm_disable and
> > eventually unwanted side effects when duty cycle is 0 and just do the
> > corresponding action if its safe. We could even put something like:
> > 
> > 	if (pwm->flags & PWM_AUTO_DISABLE && pwm->period == 0)
> > 		pwm_disable(pwm);
> > 
> > in pwm_config to help backend driver authors of "sane" PWMs.
> 
> I agree that this is simpler and clearer. However it also significantly
> reduces the flexibility of the API, and I'm not sure that it is enough.
> Again, yes, for many cases this is sufficient, but it doesn't fully
> expose the capabilities of hardware.

Can you show me a use case that isn't possible to express with the
suggested change in semantic?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > I don't see how you could make that work. In practically all cases a
> > pwm_disable() would have to be assumed to cause the pin level to become
> > undefined. That makes it pretty much useless.
> 
> Right, that's what I want. pwm_disable() should make no guarantees about
> the pin level. [...]
>
> > I agree that this is simpler and clearer. However it also significantly
> > reduces the flexibility of the API, and I'm not sure that it is enough.
> > Again, yes, for many cases this is sufficient, but it doesn't fully
> > expose the capabilities of hardware.
> 
> Can you show me a use case that isn't possible to express with the
> suggested change in semantic?

To get forward here the only missing points are:

 a) Your claim that this simplification looses expressive power.
 b) Actually convert the users (assuming a) can be resolved)

I cannot find a usecase that cannot be handled with the suggested change
in semantic. So can I lure you in explaining what setup you have in
mind?

Best regards
Uwe

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

[PATCH 0/2] specify the pin state after pwm_disable as explicitly undefined

[Uwe Kleine-König]

Hello Thierry,

these would be the first two patches I'd suggest after you fail to find
an example that shows a disadvantage of my suggestion. :-)

Best regards
Uwe

Uwe Kleine-König (2):
  pwm: document the pin state after pwm_disable() to be undefined
  pwm: warn callers of pwm_apply_state() that expect a certain level
    after disabling

 Documentation/pwm.txt |  8 ++++++--
 drivers/pwm/core.c    | 12 ++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

-- 
2.18.0

[PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Uwe Kleine-König]

Contrarily to the common assumption that pwm_disable() stops the
output at the state where it just happens to be, this isn't what the
hardware does on some machines. For example on i.MX6 the pin goes to
0 level instead.

Note this doesn't reduce the expressiveness of the pwm-API because if
you want the PWM-Pin to stay at a given state, you are supposed to
configure it to 0% or 100% duty cycle without calling pwm_disable().
The backend driver is free to implement potential power savings
already when the duty cycle changes to one of these two cases so this
doesn't come at an increased runtime power cost (once the respective
drivers are fixed).

In return this allows to adhere to the API more easily for drivers that
currently cannot know if it's ok to disable the hardware on
pwm_disable() because the caller might or might not rely on the pin
stopping at a certain level.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 Documentation/pwm.txt | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 8fbf0aa3ba2d..36cbcd353e37 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -49,11 +49,15 @@ After being requested, a PWM has to be configured using::
 This API controls both the PWM period/duty_cycle config and the
 enable/disable state.
 
+Note that it is explicitly undefined what happens to the PWM pin when the
+pwm-device is disabled, even if the duty cycle was configured to 0% or 100%
+before. It might stay at the last configured state, it might go to 0 or High-Z.
+
 The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
 around pwm_apply_state() and should not be used if the user wants to change
 several parameter at once. For example, if you see pwm_config() and
-pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+pwm_enable() calls in the same function, this probably means you should switch
+to pwm_apply_state().
 
 The PWM user API also allows one to query the PWM state with pwm_get_state().
 
-- 
2.18.0

[PATCH 2/2] pwm: warn callers of pwm_apply_state() that expect a certain level after disabling

[Uwe Kleine-König]

If a pwm-API user changes the duty cycle to 0% or 100% and setting the
state to disabled, probably the old implicit behaviour is expected that
the pin state stays where it is after pwm_disable().

While there might be some users that trigger this warning as false
positive, it is probably good to have this anyhow at least until the
newly introduced undefined behaviour sunk in a bit.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..a076d58127ac 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -475,6 +475,18 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 	if (!memcmp(state, &pwm->state, sizeof(*state)))
 		return 0;
 
+	/*
+	 * If period, duty_cycle and/or polarity changes and the pwm should be
+	 * disabled the user might expect something that is explicitly undefined
+	 * (i.e. a certain state of the pin in disabled state).
+	 */
+	WARN_ONCE((state->period != pwm->state.period ||
+		   state->duty_cycle != pwm->state.duty_cycle ||
+		   state->polarity != pwm->state.polarity) &&
+		  (state->duty_cycle == 0 ||
+		   state->duty_cycle == state->period) &&
+		  state->enabled == false, "%s: config changed for disabled pin", pwm->label);
+
 	if (pwm->chip->ops->apply) {
 		err = pwm->chip->ops->apply(pwm->chip, pwm, state);
 		if (err)
-- 
2.18.0

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > I don't see how you could make that work. In practically all cases a
> > > pwm_disable() would have to be assumed to cause the pin level to become
> > > undefined. That makes it pretty much useless.
> > 
> > Right, that's what I want. pwm_disable() should make no guarantees about
> > the pin level. [...]
> >
> > > I agree that this is simpler and clearer. However it also significantly
> > > reduces the flexibility of the API, and I'm not sure that it is enough.
> > > Again, yes, for many cases this is sufficient, but it doesn't fully
> > > expose the capabilities of hardware.
> > 
> > Can you show me a use case that isn't possible to express with the
> > suggested change in semantic?
> 
> To get forward here the only missing points are:
> 
>  a) Your claim that this simplification looses expressive power.
>  b) Actually convert the users (assuming a) can be resolved)
> 
> I cannot find a usecase that cannot be handled with the suggested change
> in semantic. So can I lure you in explaining what setup you have in
> mind?

I'd really like to get forward here, so you feedback would be very
welcome. What is the use case you have in mind when writing "it also
significantly reduces the flexibility of the API, and I'm not sure that
it is enough"?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > I don't see how you could make that work. In practically all cases a
> > > > pwm_disable() would have to be assumed to cause the pin level to become
> > > > undefined. That makes it pretty much useless.
> > > 
> > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > the pin level. [...]
> > >
> > > > I agree that this is simpler and clearer. However it also significantly
> > > > reduces the flexibility of the API, and I'm not sure that it is enough.
> > > > Again, yes, for many cases this is sufficient, but it doesn't fully
> > > > expose the capabilities of hardware.
> > > 
> > > Can you show me a use case that isn't possible to express with the
> > > suggested change in semantic?
> > 
> > To get forward here the only missing points are:
> > 
> >  a) Your claim that this simplification looses expressive power.
> >  b) Actually convert the users (assuming a) can be resolved)
> > 
> > I cannot find a usecase that cannot be handled with the suggested change
> > in semantic. So can I lure you in explaining what setup you have in
> > mind?
> 
> I'd really like to get forward here, so you feedback would be very
> welcome. What is the use case you have in mind when writing "it also
> significantly reduces the flexibility of the API, and I'm not sure that
> it is enough"?

I'm a bit irritated that you don't continue to communicate here. On
August 9 I had the impression that we can discuss this topic to an end.
But as you stopped replying we unfortunately cannot :-|

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > I don't see how you could make that work. In practically all cases a
> > > > > pwm_disable() would have to be assumed to cause the pin level to become
> > > > > undefined. That makes it pretty much useless.
> > > > 
> > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > the pin level. [...]
> > > >
> > > > > I agree that this is simpler and clearer. However it also significantly
> > > > > reduces the flexibility of the API, and I'm not sure that it is enough.
> > > > > Again, yes, for many cases this is sufficient, but it doesn't fully
> > > > > expose the capabilities of hardware.
> > > > 
> > > > Can you show me a use case that isn't possible to express with the
> > > > suggested change in semantic?
> > > 
> > > To get forward here the only missing points are:
> > > 
> > >  a) Your claim that this simplification looses expressive power.
> > >  b) Actually convert the users (assuming a) can be resolved)
> > > 
> > > I cannot find a usecase that cannot be handled with the suggested change
> > > in semantic. So can I lure you in explaining what setup you have in
> > > mind?
> > 
> > I'd really like to get forward here, so you feedback would be very
> > welcome. What is the use case you have in mind when writing "it also
> > significantly reduces the flexibility of the API, and I'm not sure that
> > it is enough"?
> 
> I'm a bit irritated that you don't continue to communicate here. On
> August 9 I had the impression that we can discuss this topic to an end.
> But as you stopped replying we unfortunately cannot :-|

Seeing you reply on other topics I wonder what is the problem here. Can
you at least quickly confirm that you received my mails?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2536 bytes --]

On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > > I don't see how you could make that work. In practically all cases a
> > > > > > pwm_disable() would have to be assumed to cause the pin level to become
> > > > > > undefined. That makes it pretty much useless.
> > > > > 
> > > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > > the pin level. [...]
> > > > >
> > > > > > I agree that this is simpler and clearer. However it also significantly
> > > > > > reduces the flexibility of the API, and I'm not sure that it is enough.
> > > > > > Again, yes, for many cases this is sufficient, but it doesn't fully
> > > > > > expose the capabilities of hardware.
> > > > > 
> > > > > Can you show me a use case that isn't possible to express with the
> > > > > suggested change in semantic?
> > > > 
> > > > To get forward here the only missing points are:
> > > > 
> > > >  a) Your claim that this simplification looses expressive power.
> > > >  b) Actually convert the users (assuming a) can be resolved)
> > > > 
> > > > I cannot find a usecase that cannot be handled with the suggested change
> > > > in semantic. So can I lure you in explaining what setup you have in
> > > > mind?
> > > 
> > > I'd really like to get forward here, so you feedback would be very
> > > welcome. What is the use case you have in mind when writing "it also
> > > significantly reduces the flexibility of the API, and I'm not sure that
> > > it is enough"?
> > 
> > I'm a bit irritated that you don't continue to communicate here. On
> > August 9 I had the impression that we can discuss this topic to an end.
> > But as you stopped replying we unfortunately cannot :-|
> 
> Seeing you reply on other topics I wonder what is the problem here. Can
> you at least quickly confirm that you received my mails?

Confirming that I'm receiving your emails. Sorry, but I haven't found
the necessary time to look at the details here any closer to continue
the discussion in a meaningful way.

I will hopefully get around to it within the next couple of days.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Thu, Sep 27, 2018 at 10:29:55PM +0200, Thierry Reding wrote:
> On Thu, Sep 27, 2018 at 08:26:07PM +0200, Uwe Kleine-König wrote:
> > On Fri, Sep 21, 2018 at 06:02:30PM +0200, Uwe Kleine-König wrote:
> > > On Tue, Sep 04, 2018 at 10:37:24PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Aug 20, 2018 at 11:52:21AM +0200, Uwe Kleine-König wrote:
> > > > > On Thu, Aug 09, 2018 at 05:23:30PM +0200, Uwe Kleine-König wrote:
> > > > > > On Thu, Aug 09, 2018 at 01:30:54PM +0200, Thierry Reding wrote:
> > > > > > > I don't see how you could make that work. In practically all cases a
> > > > > > > pwm_disable() would have to be assumed to cause the pin level to become
> > > > > > > undefined. That makes it pretty much useless.
> > > > > > 
> > > > > > Right, that's what I want. pwm_disable() should make no guarantees about
> > > > > > the pin level. [...]
> > > > > >
> > > > > > > I agree that this is simpler and clearer. However it also significantly
> > > > > > > reduces the flexibility of the API, and I'm not sure that it is enough.
> > > > > > > Again, yes, for many cases this is sufficient, but it doesn't fully
> > > > > > > expose the capabilities of hardware.
> > > > > > 
> > > > > > Can you show me a use case that isn't possible to express with the
> > > > > > suggested change in semantic?
> > > > > 
> > > > > To get forward here the only missing points are:
> > > > > 
> > > > >  a) Your claim that this simplification looses expressive power.
> > > > >  b) Actually convert the users (assuming a) can be resolved)
> > > > > 
> > > > > I cannot find a usecase that cannot be handled with the suggested change
> > > > > in semantic. So can I lure you in explaining what setup you have in
> > > > > mind?
> > > > 
> > > > I'd really like to get forward here, so you feedback would be very
> > > > welcome. What is the use case you have in mind when writing "it also
> > > > significantly reduces the flexibility of the API, and I'm not sure that
> > > > it is enough"?
> > > 
> > > I'm a bit irritated that you don't continue to communicate here. On
> > > August 9 I had the impression that we can discuss this topic to an end.
> > > But as you stopped replying we unfortunately cannot :-|
> > 
> > Seeing you reply on other topics I wonder what is the problem here. Can
> > you at least quickly confirm that you received my mails?
> 
> Confirming that I'm receiving your emails. Sorry, but I haven't found
> the necessary time to look at the details here any closer to continue
> the discussion in a meaningful way.
> 
> I will hopefully get around to it within the next couple of days.

I would really like to continue this discussion before it is swapped out
of my mind again.

Did you find the time to consider my thoughts?

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1987 bytes --]

On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:
> Contrarily to the common assumption that pwm_disable() stops the
> output at the state where it just happens to be, this isn't what the
> hardware does on some machines. For example on i.MX6 the pin goes to
> 0 level instead.
> 
> Note this doesn't reduce the expressiveness of the pwm-API because if
> you want the PWM-Pin to stay at a given state, you are supposed to
> configure it to 0% or 100% duty cycle without calling pwm_disable().
> The backend driver is free to implement potential power savings
> already when the duty cycle changes to one of these two cases so this
> doesn't come at an increased runtime power cost (once the respective
> drivers are fixed).
> 
> In return this allows to adhere to the API more easily for drivers that
> currently cannot know if it's ok to disable the hardware on
> pwm_disable() because the caller might or might not rely on the pin
> stopping at a certain level.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  Documentation/pwm.txt | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)

I don't think this is correct. An API whose result is an undefined state
is useless in my opinion. So either we properly define what the state
should be after a disable operation, or we might as well just remove the
disable operation altogether.

From an API point of view I think it makes more sense to define the
state after disable to be inactive, that is high for normal PWM and low
for inversed PWM. The reason for this is that the disabled state is
effectively what the reset state of the PWM would be. If the PWM is
inverted, typically the board design has a pull-up somewhere to avoid a
backlight or LED from getting enabled by default. In all other cases a
PWM pin is likely just going to default to low when disabled.

This also matches what all other drivers, and users, assume.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3322 bytes --]

On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> (If you have a déjà vu: Yes, I tried to argue this case already in the
> past, it's just that I'm just faced with a new use case that's broken
> with the current state.)
> 
> Currently the idiom
> 
> 	pwm_config(mypwm, value, period);
> 	if (!value)
> 		pwm_disable(mypwm);
> 	else
> 		pwm_enable(mypwm);
> 
> (and it's equivalent:
> 
> 	state.duty_cycle = ...
> 	state.enable = state.duty_cycle ? true : false;
> 	pwm_apply_state(...);
> 
> ) is quite present in the kernel.
> 
> In my eyes this is bad for two reasons:
> 
>  a) if the caller is supposed to call pwm_disable() after configuring the
>     duty cycle to zero, then why doesn't pwm_config() contains this to
>     unburden pwm-API users and so reduce open coding this assumption?

Just to reiterate my thoughts on this: while the config/enable/disable
split may seem arbitrary for some use-cases (you may argue most
use-cases, and you'd probably be right), I think there's value in having
the additional flexibility to explicitly turn off the PWM. Also, duty
cycle of zero doesn't always mean "off". If your PWM is inverted, then
the duty cycle of zero actually means "on". And that of course also
still depends on other components on your board. You may have an
inverter somewhere, or you may actually be driving a circuit that
expects an inverted PWM.

So saying that duty-cycle of 0 equals disable is simplifying things too
much, in my opinion.

>  b) it is actually wrong in at least two cases:
>     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
>       and only after the current period is elapsed this is clocked into
>       the hardware. So it is very likely that when pwm_disable() is
>       called the period is still not over, and then the clk is disabled
>       and the period never ends resulting in the pwm pin being frozen in
>       whatever state it happend to be when the clk was stopped.
>     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
>       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
>       puts it to 0. In the current case this implies that the backlight
>       is fully enabled instead of off.

I think I've explained this in the past: these are i.MX specific issues
that you need to work around in the driver. It's also mostly orthogonal
to the API issue because consider the case where you want to unload the
PWM driver on your board. Your remove function would need disable all
PWM channels in order to properly clean up the device, but if you do
that, then your backlight would turn fully on, right?

On that note, I'm not sure I understand how this would even work, since
you should have the very same state on boot. I'm assuming here that the
PWM will be off at boot time by default, in which case, given that it's
disabled, it should cause the backlight to turn on. How do you solve
that problem? Or if it's not a problem at boot, why does it become a
problem on PWM disable or driver removal?

What you're currently proposing is unjustified at this point: you're
trying to work around an issue specific to i.MX by changing an API that
has worked fine for everyone else for a decade.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Uwe Kleine-König]

Hello Thierry,

thanks for taking time to reply in this thread.

On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:
> > Contrarily to the common assumption that pwm_disable() stops the
> > output at the state where it just happens to be, this isn't what the
> > hardware does on some machines. For example on i.MX6 the pin goes to
> > 0 level instead.
> > 
> > Note this doesn't reduce the expressiveness of the pwm-API because if
> > you want the PWM-Pin to stay at a given state, you are supposed to
> > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > The backend driver is free to implement potential power savings
> > already when the duty cycle changes to one of these two cases so this
> > doesn't come at an increased runtime power cost (once the respective
> > drivers are fixed).
> > 
> > In return this allows to adhere to the API more easily for drivers that
> > currently cannot know if it's ok to disable the hardware on
> > pwm_disable() because the caller might or might not rely on the pin
> > stopping at a certain level.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  Documentation/pwm.txt | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> I don't think this is correct. An API whose result is an undefined state
> is useless in my opinion. So either we properly define what the state
> should be after a disable operation, or we might as well just remove the
> disable operation altogether.

Explicitly not defining some details makes it easier for hardware
drivers to comply with the API. Sure it would be great if all hardware
would allow to implement a "stronger" API but this isn't how reality looks
like.

You say it's bad that .disable() should imply less than it did up to
now. I say .disable() should only imply that the PWM stops toggling, so
.disable has a single purpose that each hardware design should be able
to implement.
And this weaker requirement/result is still good enough to implement all
use cases. (You had doubts here in the past, but without an example. I
cannot imagine there is one.) In my eyes this makes the API better not
worse.

What we have now in the API is redundancy, which IMHO is worse. If I
want the pwm pin to stay low I can say:

	pwm_config(pwm, 0, 100);

or I can say:

	pwm_config(pwm, 0, 100);
	pwm_disable(pwm);

The expected result is the same. Now you could argue that not disabling
the pwm is a bug because it doesn't save some energy. But really this is
a weakness of the API. There are two ways to express "Set the pin to
constant 0" and if there is an opportunity to save some energy on a
certain hardware implementation, just calling pwm_config() with duty=0
should be enough to benefit from it. This makes the API easier to use
because there is a single command to get to the state the pwm user wants
the pwm to be in. (This is two advantages: A single way to reach the
desired state and only one function to call.)

> From an API point of view I think it makes more sense to define the
> state after disable to be inactive, that is high for normal PWM and low
> for inversed PWM. The reason for this is that the disabled state is
> effectively what the reset state of the PWM would be.

The problem is this is where theory and reality differ. On i.MX if the
pwm clock is disabled the pin goes to zero independent of being inverted
or not. And before pinmuxing is done the reset state of the PWMs I know
and work with is an input.

So the only way the PWM API could be implemented fulfilling your
conditions on i.MX is that .disable() is a noop. And the pwm clock must
not be disabled until pwm_put() is called.

So the semantic of .disable() currently is "disable the pwm clock if this
keeps the pin at the expected level". For some---maybe even
most---hardware implementations this if condition is a no-brainer, but
for others it is relevant.

I want to simplify the semantic of .disable to "disable the pwm clock".
This downside is that this exposes hardware details and we need to teach
the pwm API users that this might have a side effect that isn't expected
today. But the expressiveness of the API isn't restricted because
with calling only pwm_config(pwm, 0, 100) you can still get the intended
result.

> If the PWM is inverted, typically the board design has a pull-up
> somewhere to avoid a backlight or LED from getting enabled by default.

Experience shows that it's a bad idea to rely on the hardware guys to do
everything we software guys consider sane. If the API is only suitable
for typical scenarios that's a bad report. My suggestion doesn't solve
this completely, but it makes the behaviour in the "non-typical"
situations better without complicating stuff in the good cases. A
complete win.

> In all other cases a PWM pin is likely just going to default to low
> when disabled.

So you suggest to specify "calling pwm_disable() is likely to drive the
pin to zero"? For sure you don't as this is completely useless.

> This also matches what all other drivers, and users, assume.

I don't know the hardware details for all PWMs that have a Linux driver
but I'd be surprised if the PWMs in i.MX SoCs were the only hardware
implementations that don't comply with your idea of a PWM.

And users are fixable.

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > past, it's just that I'm just faced with a new use case that's broken
> > with the current state.)
> > 
> > Currently the idiom
> > 
> > 	pwm_config(mypwm, value, period);
> > 	if (!value)
> > 		pwm_disable(mypwm);
> > 	else
> > 		pwm_enable(mypwm);
> > 
> > (and it's equivalent:
> > 
> > 	state.duty_cycle = ...
> > 	state.enable = state.duty_cycle ? true : false;
> > 	pwm_apply_state(...);
> > 
> > ) is quite present in the kernel.
> > 
> > In my eyes this is bad for two reasons:
> > 
> >  a) if the caller is supposed to call pwm_disable() after configuring the
> >     duty cycle to zero, then why doesn't pwm_config() contains this to
> >     unburden pwm-API users and so reduce open coding this assumption?
> 
> Just to reiterate my thoughts on this: while the config/enable/disable
> split may seem arbitrary for some use-cases (you may argue most
> use-cases, and you'd probably be right), I think there's value in having
> the additional flexibility to explicitly turn off the PWM. Also, duty
> cycle of zero doesn't always mean "off". If your PWM is inverted, then
> the duty cycle of zero actually means "on". And that of course also
> still depends on other components on your board.

Did you notice you didn't argue against me here? We seem to agree that
calling disabling a PWM after setting the duty cycle to zero is bad.

> You may have an inverter somewhere, or you may actually be driving a
> circuit that expects an inverted PWM.
> 
> So saying that duty-cycle of 0 equals disable is simplifying things too
> much, in my opinion.

Full ack. So let's fix the users to not call pwm_disable after setting
the duty cycle to zero!

Really, only the driver knows when it's safe to disable the clock while
it's expected to keep the pin at a given state. So better don't expect a
certain state after pwm_disable().

> >  b) it is actually wrong in at least two cases:
> >     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
> >       and only after the current period is elapsed this is clocked into
> >       the hardware. So it is very likely that when pwm_disable() is
> >       called the period is still not over, and then the clk is disabled
> >       and the period never ends resulting in the pwm pin being frozen in
> >       whatever state it happend to be when the clk was stopped.
> >     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
> >       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
> >       puts it to 0. In the current case this implies that the backlight
> >       is fully enabled instead of off.
> 
> I think I've explained this in the past: these are i.MX specific issues
> that you need to work around in the driver.

I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
(in a sane way) be mapped by the current PWM API. That's because there
are assumptions in the API that are wrong for i.MX. Still the hardware
allows to implement all use cases. That's a sign that the API is bad and
is in need for generalisation.

> It's also mostly orthogonal
> to the API issue because consider the case where you want to unload the
> PWM driver on your board. Your remove function would need disable all
> PWM channels in order to properly clean up the device, but if you do
> that, then your backlight would turn fully on, right?

Right. I cannot fix all hardware problems in software. But with my
suggested change I can at least make it as good as possible in software
without having to resort to ugly hacks.
 
> On that note, I'm not sure I understand how this would even work, since
> you should have the very same state on boot. I'm assuming here that the
> PWM will be off at boot time by default, in which case, given that it's
> disabled, it should cause the backlight to turn on. How do you solve
> that problem? Or if it's not a problem at boot, why does it become a
> problem on PWM disable or driver removal?

Most of the time it's not a problem during boot because the pin is muxed
as input. But when the clock of the i.MX PWM is disabled it actively
drives a zero. And I already had cases where there really was a problem
during boot until the bootloader fixed the stuff in software. For these
cases I need patches ("hacks") that remove calls to pwm_disable in the
backlight driver to at least work around the issue during runtime, which
is the best that can be done in software.
 
> What you're currently proposing is unjustified at this point: you're
> trying to work around an issue specific to i.MX by changing an API that
> has worked fine for everyone else for a decade.

I don't agree here. The PWM API as it is now would make it necessary
that the imx driver becomes ugly. By changing the semantic of the API

 - the i.MX driver can be implemented nicely;
 - the other drivers for "saner" hardware can stay as they are;
 - the API users can still express all their needs;
 - the API stops having two ways to express the same needs;
 - most API users can be simplified because they can drop
     if (duty == 0) pwm_disable()
 - on i.MX the clock can be disabled when not needed saving some energy;

Regarding you claim that the API worked fine for a decade: I tried
already in 2013 to address this problem. And even independent of that,
that is not a good reason to not improve stuff.

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 8961 bytes --]

On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > > Hello Thierry,
> > > 
> > > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > > past, it's just that I'm just faced with a new use case that's broken
> > > with the current state.)
> > > 
> > > Currently the idiom
> > > 
> > > 	pwm_config(mypwm, value, period);
> > > 	if (!value)
> > > 		pwm_disable(mypwm);
> > > 	else
> > > 		pwm_enable(mypwm);
> > > 
> > > (and it's equivalent:
> > > 
> > > 	state.duty_cycle = ...
> > > 	state.enable = state.duty_cycle ? true : false;
> > > 	pwm_apply_state(...);
> > > 
> > > ) is quite present in the kernel.
> > > 
> > > In my eyes this is bad for two reasons:
> > > 
> > >  a) if the caller is supposed to call pwm_disable() after configuring the
> > >     duty cycle to zero, then why doesn't pwm_config() contains this to
> > >     unburden pwm-API users and so reduce open coding this assumption?
> > 
> > Just to reiterate my thoughts on this: while the config/enable/disable
> > split may seem arbitrary for some use-cases (you may argue most
> > use-cases, and you'd probably be right), I think there's value in having
> > the additional flexibility to explicitly turn off the PWM. Also, duty
> > cycle of zero doesn't always mean "off". If your PWM is inverted, then
> > the duty cycle of zero actually means "on". And that of course also
> > still depends on other components on your board.
> 
> Did you notice you didn't argue against me here? We seem to agree that
> calling disabling a PWM after setting the duty cycle to zero is bad.

I don't see why you think I agree with that. The only thing I'm saying
is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
same as the PWM being disabled.

> > You may have an inverter somewhere, or you may actually be driving a
> > circuit that expects an inverted PWM.
> > 
> > So saying that duty-cycle of 0 equals disable is simplifying things too
> > much, in my opinion.
> 
> Full ack. So let's fix the users to not call pwm_disable after setting
> the duty cycle to zero!

I don't understand why you come to that conclusion. It seems like we at
least agree on what the problem is and what potential issues there could
be with different setups.

The point that we don't agree with is how to fix this.

> Really, only the driver knows when it's safe to disable the clock while
> it's expected to keep the pin at a given state. So better don't expect a
> certain state after pwm_disable().

Yes, I agree that only the driver knows when it is safe to do so, but I
still fail to understand why we should change the API in order to
accomodate the specifics of a particular driver.

So we already know that currently all users expect that the pin state
after disable will be low (because they previously set duty cycle to 0,
which is the active equivalent of low). Given that stricter definition,
the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
state. I think the fix for that is for the i.MX PWM driver to make sure
the pin state doesn't actually change on ->disable(). If that means the
clock needs to remain on, then that's exactly what the driver should be
implementing.

> > >  b) it is actually wrong in at least two cases:
> > >     - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
> > >       and only after the current period is elapsed this is clocked into
> > >       the hardware. So it is very likely that when pwm_disable() is
> > >       called the period is still not over, and then the clk is disabled
> > >       and the period never ends resulting in the pwm pin being frozen in
> > >       whatever state it happend to be when the clk was stopped.
> > >     - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
> > >       correctly ensures the PWM pin to stay at 1. But then pwm_disable()
> > >       puts it to 0. In the current case this implies that the backlight
> > >       is fully enabled instead of off.
> > 
> > I think I've explained this in the past: these are i.MX specific issues
> > that you need to work around in the driver.
> 
> I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
> (in a sane way) be mapped by the current PWM API. That's because there
> are assumptions in the API that are wrong for i.MX. Still the hardware
> allows to implement all use cases. That's a sign that the API is bad and
> is in need for generalisation.

It's perfectly fine for the driver to make any of the PWM operations a
no-op if that's what it takes to make it conform with the API.

> > It's also mostly orthogonal
> > to the API issue because consider the case where you want to unload the
> > PWM driver on your board. Your remove function would need disable all
> > PWM channels in order to properly clean up the device, but if you do
> > that, then your backlight would turn fully on, right?
> 
> Right. I cannot fix all hardware problems in software. But with my
> suggested change I can at least make it as good as possible in software
> without having to resort to ugly hacks.
> 
> > On that note, I'm not sure I understand how this would even work, since
> > you should have the very same state on boot. I'm assuming here that the
> > PWM will be off at boot time by default, in which case, given that it's
> > disabled, it should cause the backlight to turn on. How do you solve
> > that problem? Or if it's not a problem at boot, why does it become a
> > problem on PWM disable or driver removal?
> 
> Most of the time it's not a problem during boot because the pin is muxed
> as input. But when the clock of the i.MX PWM is disabled it actively
> drives a zero. And I already had cases where there really was a problem
> during boot until the bootloader fixed the stuff in software. For these
> cases I need patches ("hacks") that remove calls to pwm_disable in the
> backlight driver to at least work around the issue during runtime, which
> is the best that can be done in software.

Are you aware of this:

	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987

This seems to address exactly the problem that you're describing. The
solution matches what I had expected a fix for this problem to look
like. Can you try it and see if that fixes the issue?

It looks as if that also fixes the shutdown problem, so it's better than
what you're proposing here yet it doesn't require any API change at all
to make it work.

Cc'ing Michal for visibility.

> > What you're currently proposing is unjustified at this point: you're
> > trying to work around an issue specific to i.MX by changing an API that
> > has worked fine for everyone else for a decade.
> 
> I don't agree here. The PWM API as it is now would make it necessary
> that the imx driver becomes ugly.

Beauty is very subjective. This doesn't count as an argument. Michal's
proposed solution looks perfectly fine.

> By changing the semantic of the API
> 
>  - the i.MX driver can be implemented nicely;
>  - the other drivers for "saner" hardware can stay as they are;
>  - the API users can still express all their needs;
>  - the API stops having two ways to express the same needs;

Again, duty-cycle = 0 is not the same as setting the state to disabled.
Practically they usually have the same effect, but that's just because
in the typical use-case we don't care about the subtle differences.

>  - most API users can be simplified because they can drop
>      if (duty == 0) pwm_disable()
>  - on i.MX the clock can be disabled when not needed saving some energy;

That's one reason why we have pwm_disable(). It's what you use to save
some energy if you don't need the PWM. Also, you claim that if the clock
is disabled the attached backlight will turn on at full brightness, so I
fail to see how this would work.

Also, you leave out the fact that even after all this work that touches
all users and all PWM drivers you still don't get fully correct
functionality on your devices because when the driver is removed or shut
down you run into the same problem.

> Regarding you claim that the API worked fine for a decade: I tried
> already in 2013 to address this problem. And even independent of that,
> that is not a good reason to not improve stuff.

My claim was that it has worked fine "for everyone else". I know that
this has been an issue on i.MX for a long time and I've done my best to
lay out why I think your proposal is not the right way to fix it. The
proposal from Michal shows that there is another way that properly fixes
the issue, so let's focus on getting that to work for your use-cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Vokáč Michal]

Ahoj,
thank you for bringing me to the discussion Thierry.

On 10.10.2018 14:26, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
>> Hello Thierry,
>>
>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
>>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
>>>> Hello Thierry,
>>>>
>>>> (If you have a déjà vu: Yes, I tried to argue this case already in the
>>>> past, it's just that I'm just faced with a new use case that's broken
>>>> with the current state.)
>>>>
>>>> Currently the idiom
>>>>
>>>> 	pwm_config(mypwm, value, period);
>>>> 	if (!value)
>>>> 		pwm_disable(mypwm);
>>>> 	else
>>>> 		pwm_enable(mypwm);
>>>>
>>>> (and it's equivalent:
>>>>
>>>> 	state.duty_cycle = ...
>>>> 	state.enable = state.duty_cycle ? true : false;
>>>> 	pwm_apply_state(...);
>>>>
>>>> ) is quite present in the kernel.
>>>>
>>>> In my eyes this is bad for two reasons:
>>>>
>>>>   a) if the caller is supposed to call pwm_disable() after configuring the
>>>>      duty cycle to zero, then why doesn't pwm_config() contains this to
>>>>      unburden pwm-API users and so reduce open coding this assumption?
>>>
>>> Just to reiterate my thoughts on this: while the config/enable/disable
>>> split may seem arbitrary for some use-cases (you may argue most
>>> use-cases, and you'd probably be right), I think there's value in having
>>> the additional flexibility to explicitly turn off the PWM. Also, duty
>>> cycle of zero doesn't always mean "off". If your PWM is inverted, then
>>> the duty cycle of zero actually means "on". And that of course also
>>> still depends on other components on your board.
>>
>> Did you notice you didn't argue against me here? We seem to agree that
>> calling disabling a PWM after setting the duty cycle to zero is bad.
> 
> I don't see why you think I agree with that. The only thing I'm saying
> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> same as the PWM being disabled.
> 
>>> You may have an inverter somewhere, or you may actually be driving a
>>> circuit that expects an inverted PWM.
>>>
>>> So saying that duty-cycle of 0 equals disable is simplifying things too
>>> much, in my opinion.
>>
>> Full ack. So let's fix the users to not call pwm_disable after setting
>> the duty cycle to zero!
> 
> I don't understand why you come to that conclusion. It seems like we at
> least agree on what the problem is and what potential issues there could
> be with different setups.
> 
> The point that we don't agree with is how to fix this.
> 
>> Really, only the driver knows when it's safe to disable the clock while
>> it's expected to keep the pin at a given state. So better don't expect a
>> certain state after pwm_disable().
> 
> Yes, I agree that only the driver knows when it is safe to do so, but I
> still fail to understand why we should change the API in order to
> accomodate the specifics of a particular driver.
> 
> So we already know that currently all users expect that the pin state
> after disable will be low (because they previously set duty cycle to 0,
> which is the active equivalent of low). Given that stricter definition,
> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> state. I think the fix for that is for the i.MX PWM driver to make sure
> the pin state doesn't actually change on ->disable(). If that means the
> clock needs to remain on, then that's exactly what the driver should be
> implementing.
> 
>>>>   b) it is actually wrong in at least two cases:
>>>>      - On i.MX28 pwm_config(mypwm, 0, period) only programs a register
>>>>        and only after the current period is elapsed this is clocked into
>>>>        the hardware. So it is very likely that when pwm_disable() is
>>>>        called the period is still not over, and then the clk is disabled
>>>>        and the period never ends resulting in the pwm pin being frozen in
>>>>        whatever state it happend to be when the clk was stopped.
>>>>      - on i.MX6 if a pwm is inverted pwm_config(mypwm, 0, period)
>>>>        correctly ensures the PWM pin to stay at 1. But then pwm_disable()
>>>>        puts it to 0. In the current case this implies that the backlight
>>>>        is fully enabled instead of off.
>>>
>>> I think I've explained this in the past: these are i.MX specific issues
>>> that you need to work around in the driver.
>>
>> I'd say: The i.MX PWM doesn't match the idealised idea of a PWM that can
>> (in a sane way) be mapped by the current PWM API. That's because there
>> are assumptions in the API that are wrong for i.MX. Still the hardware
>> allows to implement all use cases. That's a sign that the API is bad and
>> is in need for generalisation.
> 
> It's perfectly fine for the driver to make any of the PWM operations a
> no-op if that's what it takes to make it conform with the API.
> 
>>> It's also mostly orthogonal
>>> to the API issue because consider the case where you want to unload the
>>> PWM driver on your board. Your remove function would need disable all
>>> PWM channels in order to properly clean up the device, but if you do
>>> that, then your backlight would turn fully on, right?
>>
>> Right. I cannot fix all hardware problems in software. But with my
>> suggested change I can at least make it as good as possible in software
>> without having to resort to ugly hacks.
>>
>>> On that note, I'm not sure I understand how this would even work, since
>>> you should have the very same state on boot. I'm assuming here that the
>>> PWM will be off at boot time by default, in which case, given that it's
>>> disabled, it should cause the backlight to turn on. How do you solve
>>> that problem? Or if it's not a problem at boot, why does it become a
>>> problem on PWM disable or driver removal?
>>
>> Most of the time it's not a problem during boot because the pin is muxed
>> as input. But when the clock of the i.MX PWM is disabled it actively
>> drives a zero. And I already had cases where there really was a problem
>> during boot until the bootloader fixed the stuff in software. For these
>> cases I need patches ("hacks") that remove calls to pwm_disable in the
>> backlight driver to at least work around the issue during runtime, which
>> is the best that can be done in software.
> 
> Are you aware of this:
> 
> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> 
> This seems to address exactly the problem that you're describing. The
> solution matches what I had expected a fix for this problem to look
> like. Can you try it and see if that fixes the issue?
>
> It looks as if that also fixes the shutdown problem, so it's better than
> what you're proposing here yet it doesn't require any API change at all
> to make it work.
> 
> Cc'ing Michal for visibility
 From my very brief skim of your previous discussion it looks like the
exact same problem. So I think it should hopefully solve Uwe's issue.

I would also like to bring attention to an other series I submitted
recently:

	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=68445

It implements the get_state() function that is call from PWM core.
It allows to read the actual state of the PWM HW at probe time.

So if you enable PWM in bootloader (I mean you configure it to produce
a PWM signal) and you also use it in Linux (just enabling the pwm chip
in DT is enough) you will end up with running PWM in userspace. And you
can read the actual period/duty cycle form sysfs.

And vice versa. If the PWM chip is disabled when Linux is booting,
you will end up with disabled PWM in userspace.

In combination with the RFC series this should produce undistorted
PWM signal from power-up to userspace for normal and inverted PWMs
for both cases: if you want it enabled from bootloader to userspace
or if you want it disabled from bootloader to userspace.

>>> What you're currently proposing is unjustified at this point: you're
>>> trying to work around an issue specific to i.MX by changing an API that
>>> has worked fine for everyone else for a decade.
>>
>> I don't agree here. The PWM API as it is now would make it necessary
>> that the imx driver becomes ugly.
> 
> Beauty is very subjective. This doesn't count as an argument. Michal's
> proposed solution looks perfectly fine.

Good to hear that, thanks ;)

>> By changing the semantic of the API
>>
>>   - the i.MX driver can be implemented nicely;
>>   - the other drivers for "saner" hardware can stay as they are;
>>   - the API users can still express all their needs;
>>   - the API stops having two ways to express the same needs;
> 
> Again, duty-cycle = 0 is not the same as setting the state to disabled.
> Practically they usually have the same effect, but that's just because
> in the typical use-case we don't care about the subtle differences.
> 
>>   - most API users can be simplified because they can drop
>>       if (duty == 0) pwm_disable()
>>   - on i.MX the clock can be disabled when not needed saving some energy;
> 
> That's one reason why we have pwm_disable(). It's what you use to save
> some energy if you don't need the PWM. Also, you claim that if the clock
> is disabled the attached backlight will turn on at full brightness, so I
> fail to see how this would work.
> 
> Also, you leave out the fact that even after all this work that touches
> all users and all PWM drivers you still don't get fully correct
> functionality on your devices because when the driver is removed or shut
> down you run into the same problem.
> 
>> Regarding you claim that the API worked fine for a decade: I tried
>> already in 2013 to address this problem. And even independent of that,
>> that is not a good reason to not improve stuff.
> 
> My claim was that it has worked fine "for everyone else". I know that
> this has been an issue on i.MX for a long time and I've done my best to
> lay out why I think your proposal is not the right way to fix it. The
> proposal from Michal shows that there is another way that properly fixes
> the issue, so let's focus on getting that to work for your use-cases.
> 
> Thierry
> 

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > > > Hello Thierry,
> > > > 
> > > > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > > > past, it's just that I'm just faced with a new use case that's broken
> > > > with the current state.)
> > > > 
> > > > Currently the idiom
> > > > 
> > > > 	pwm_config(mypwm, value, period);
> > > > 	if (!value)
> > > > 		pwm_disable(mypwm);
> > > > 	else
> > > > 		pwm_enable(mypwm);
> > > > 
> > > > (and it's equivalent:
> > > > 
> > > > 	state.duty_cycle = ...
> > > > 	state.enable = state.duty_cycle ? true : false;
> > > > 	pwm_apply_state(...);
> > > > 
> > > > ) is quite present in the kernel.
> > > > 
> > > > In my eyes this is bad for two reasons:
> > > > 
> > > >  a) if the caller is supposed to call pwm_disable() after configuring the
> > > >     duty cycle to zero, then why doesn't pwm_config() contains this to
> > > >     unburden pwm-API users and so reduce open coding this assumption?
> > > 
> > > Just to reiterate my thoughts on this: while the config/enable/disable
> > > split may seem arbitrary for some use-cases (you may argue most
> > > use-cases, and you'd probably be right), I think there's value in having
> > > the additional flexibility to explicitly turn off the PWM. Also, duty
> > > cycle of zero doesn't always mean "off". If your PWM is inverted, then
> > > the duty cycle of zero actually means "on". And that of course also
> > > still depends on other components on your board.
> > 
> > Did you notice you didn't argue against me here? We seem to agree that
> > calling disabling a PWM after setting the duty cycle to zero is bad.

You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
also don't seem to refuse to change pwm users that do:

	pwm_config(pwm, 0, 100);
	pwm_disable(pwm);

I interpreted your sentence as confirmation that this sequence isn't
correct in general.

Where is the difference you care about between duty=0 and disabled?

> I don't see why you think I agree with that. The only thing I'm saying
> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> same as the PWM being disabled.

What is in your view the current difference for the hardware behind the
PWM?

> > > You may have an inverter somewhere, or you may actually be driving a
> > > circuit that expects an inverted PWM.
> > > 
> > > So saying that duty-cycle of 0 equals disable is simplifying things too
> > > much, in my opinion.
> > 
> > Full ack. So let's fix the users to not call pwm_disable after setting
> > the duty cycle to zero!
> 
> I don't understand why you come to that conclusion. It seems like we at
> least agree on what the problem is and what potential issues there could
> be with different setups.
> 
> The point that we don't agree with is how to fix this.

ack.

> > Really, only the driver knows when it's safe to disable the clock while
> > it's expected to keep the pin at a given state. So better don't expect a
> > certain state after pwm_disable().
> 
> Yes, I agree that only the driver knows when it is safe to do so, but I
> still fail to understand why we should change the API in order to
> accomodate the specifics of a particular driver.

The incentive to change the API is a combination of:

 - The restriction to keep the pin driving at a certain level after
   disable isn't a natural and obvious property of the hardware.
   The designers of the imx-pwm for example did it differently.

 - The change removes ambiguity from from the demands on the hardware
   drivers.

 - The change doesn't make the API less expressive.

 - The change doesn't doesn't complicate anmy pwm hardware driver.

 - The change allows to simplify most pwm users.

 - The change simplifies the imx-pwm driver.

In my experience this is enough to justify such a change.

> So we already know that currently all users expect that the pin state
> after disable will be low (because they previously set duty cycle to 0,
> which is the active equivalent of low).

My theory is that most users don't expect this and/or don't make use of
it. As of 9dcd936c5312 we have 50 pwm drivers
($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
and I think there are a few false positive matches, pwm-sun4i.c for
example). I'd be willing to bet that as of now imx isn't the only driver
with this problem.

> Given that stricter definition,
> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> state. I think the fix for that is for the i.MX PWM driver to make sure
> the pin state doesn't actually change on ->disable(). If that means the
> clock needs to remain on, then that's exactly what the driver should be
> implementing.

Yes, if we keep the API "stricter" this is what should be done. This
doesn't justify to keep the API strict though.

> Are you aware of this:
> 
> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> 
> This seems to address exactly the problem that you're describing. The
> solution matches what I had expected a fix for this problem to look
> like. Can you try it and see if that fixes the issue?

Right. Apart from bugs in it (for example
http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
as output) this would solve the issue for sure. This doesn't void my
arguments to relax the PWM API though. Also look at the costs: The patch
adds 86 lines to the imx driver complicating it considerably. Each
machine that wants to profit of this change has to adapt it's device
tree to make the additional pinmuxing and gpio known.
To be fair, my change doesn't fix the gap between pinctrl being active
and the actual enablement of the pwm. But this could be better solved in
the pwm framework without touching any hardware driver with pinctrl
mechanism that are already in place. (Make use of the "init" pinctrl
state and only switch to "default" when the pwm is configured.)
There is nothing imx-pwm specific in hooking a gpio and adapting the
pinmuxing in it. So if you want to go this path, please implement it in
a generic way such that all pwm devices could benefit.

> It looks as if that also fixes the shutdown problem, so it's better than
> what you're proposing here yet it doesn't require any API change at all
> to make it work.

I don't care much about the shutdown problem. (Also, if the imx-pwm
driver is unbound, the gpio is freed, too, so it's only by chance if the
level is kept.)

> > > What you're currently proposing is unjustified at this point: you're
> > > trying to work around an issue specific to i.MX by changing an API that
> > > has worked fine for everyone else for a decade.
> > 
> > I don't agree here. The PWM API as it is now would make it necessary
> > that the imx driver becomes ugly.
> 
> Beauty is very subjective. This doesn't count as an argument. Michal's
> proposed solution looks perfectly fine.

In this case the opposite of ugly is "simple". This is a real argument.
And this is a reason to apply my idea. It simplifies stuff and solves
some problems.

A framework is good if it is general enough to cover all use cases for
its users and demands very little from it's implementors. Each callback
to implement by a hardware driver should be as simple as possible.
Implying "keep the pin level constant" in .disable makes it more
complicated for some drivers. So this is a bad restriction that should
better be dropped if there are no relvant downsides.

Sidenote: With the current capabilities of the pwm framework there is no
technical reason to expose to the hardware drivers that the pwm user uses
inverted logic. The framework could just apply

	duty = period - duty;

before calling the hardware specific .config callback and so allow to
simplify some drivers, reducing complexity to a single place.

> > By changing the semantic of the API
> > 
> >  - the i.MX driver can be implemented nicely;
> >  - the other drivers for "saner" hardware can stay as they are;
> >  - the API users can still express all their needs;
> >  - the API stops having two ways to express the same needs;
> 
> Again, duty-cycle = 0 is not the same as setting the state to disabled.
> 
> Practically they usually have the same effect, but that's just because
> in the typical use-case we don't care about the subtle differences.

I fail to see the subtle difference.

> >  - most API users can be simplified because they can drop
> >      if (duty == 0) pwm_disable()
> >  - on i.MX the clock can be disabled when not needed saving some energy;
> 
> That's one reason why we have pwm_disable(). It's what you use to save
> some energy if you don't need the PWM. Also, you claim that if the clock
> is disabled the attached backlight will turn on at full brightness, so I
> fail to see how this would work.

As long as you care about the backlight to be off, don't disable the
pwm. And if in this state the clk can be disabled, then this should be
done without an additional poke by the hardware driver which is the only
place that can know for sure that it is safe to do so. There is no good
reason to let the pwm user have to know that the currently configured
state might make some energy saving possible and impose the burden on
him to then call pwm_disable().

> Also, you leave out the fact that even after all this work that touches
> all users and all PWM drivers you still don't get fully correct
> functionality on your devices because when the driver is removed or shut
> down you run into the same problem.

It's ok for me that all bets are off when the driver is removed. I'm
happy when I fix the problem during active operation because on the
machine I currently care about the shutdown problem isn't relevant.
(Also as noted above, even Michal's patch doesn't solve the problem
formally.)

> > Regarding you claim that the API worked fine for a decade: I tried
> > already in 2013 to address this problem. And even independent of that,
> > that is not a good reason to not improve stuff.
> 
> My claim was that it has worked fine "for everyone else". I know that
> this has been an issue on i.MX for a long time and I've done my best to
> lay out why I think your proposal is not the right way to fix it.

I still fail to see a single technical reason to not evolve the API. I
read from your mails:

 - it has been like that for ages
 - it works for everyone but i.MX
 - it could be made working for i.MX by involving pinctrl and gpio in
   the hardware specific pwm driver.

Did I miss anything? None of these justifies to keep the status quo in
my eyes.

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 18621 bytes --]

On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > > > > Hello Thierry,
> > > > > 
> > > > > (If you have a déjà vu: Yes, I tried to argue this case already in the
> > > > > past, it's just that I'm just faced with a new use case that's broken
> > > > > with the current state.)
> > > > > 
> > > > > Currently the idiom
> > > > > 
> > > > > 	pwm_config(mypwm, value, period);
> > > > > 	if (!value)
> > > > > 		pwm_disable(mypwm);
> > > > > 	else
> > > > > 		pwm_enable(mypwm);
> > > > > 
> > > > > (and it's equivalent:
> > > > > 
> > > > > 	state.duty_cycle = ...
> > > > > 	state.enable = state.duty_cycle ? true : false;
> > > > > 	pwm_apply_state(...);
> > > > > 
> > > > > ) is quite present in the kernel.
> > > > > 
> > > > > In my eyes this is bad for two reasons:
> > > > > 
> > > > >  a) if the caller is supposed to call pwm_disable() after configuring the
> > > > >     duty cycle to zero, then why doesn't pwm_config() contains this to
> > > > >     unburden pwm-API users and so reduce open coding this assumption?
> > > > 
> > > > Just to reiterate my thoughts on this: while the config/enable/disable
> > > > split may seem arbitrary for some use-cases (you may argue most
> > > > use-cases, and you'd probably be right), I think there's value in having
> > > > the additional flexibility to explicitly turn off the PWM. Also, duty
> > > > cycle of zero doesn't always mean "off". If your PWM is inverted, then
> > > > the duty cycle of zero actually means "on". And that of course also
> > > > still depends on other components on your board.
> > > 
> > > Did you notice you didn't argue against me here? We seem to agree that
> > > calling disabling a PWM after setting the duty cycle to zero is bad.
> 
> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> also don't seem to refuse to change pwm users that do:
> 
> 	pwm_config(pwm, 0, 100);
> 	pwm_disable(pwm);
> 
> I interpreted your sentence as confirmation that this sequence isn't
> correct in general.

Yes, it is not correct in general, though it is correct in many cases.

> Where is the difference you care about between duty=0 and disabled?

This is described in the kerneldoc of the corresponding functions. In the
case of pwm_enable() it starts the output toggling, whereas pwm_enable()
disables the output toggling. So strictly by going according to the PWM
API a driver should only need to call pwm_config() if the duty cycle
actually changes. If all you do is disable and enable, then pwm_enable()
and pwm_disable() should be good enough. The implicit assumption is that
the configuration will persist across pwm_disable()/pwm_enable(). In
practice this doesn't matter that much because most users will be
setting the duty cycle and enable/disable at the same time. I can easily
imagine other use-cases where you would want to make the different. One
other difference could be that pwm_enable() and pwm_disable() require a
more involved sequence of operations than pwm_config() or vice versa, in
which cases you may want to avoid one or the other if possible.

In addition, we do expose these details to userspace via sysfs, so not
allowing an explicit disable after config(duty=0) would break that ABI.

> > I don't see why you think I agree with that. The only thing I'm saying
> > is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> > same as the PWM being disabled.
> 
> What is in your view the current difference for the hardware behind the
> PWM?

It depends on the driver implementation, but the kerneldoc is pretty
specific. I many cases I would expect pwm_disable() to allow a bit of
extra power to be saved because clocks could be turned off.

> > > > You may have an inverter somewhere, or you may actually be driving a
> > > > circuit that expects an inverted PWM.
> > > > 
> > > > So saying that duty-cycle of 0 equals disable is simplifying things too
> > > > much, in my opinion.
> > > 
> > > Full ack. So let's fix the users to not call pwm_disable after setting
> > > the duty cycle to zero!
> > 
> > I don't understand why you come to that conclusion. It seems like we at
> > least agree on what the problem is and what potential issues there could
> > be with different setups.
> > 
> > The point that we don't agree with is how to fix this.
> 
> ack.
> 
> > > Really, only the driver knows when it's safe to disable the clock while
> > > it's expected to keep the pin at a given state. So better don't expect a
> > > certain state after pwm_disable().
> > 
> > Yes, I agree that only the driver knows when it is safe to do so, but I
> > still fail to understand why we should change the API in order to
> > accomodate the specifics of a particular driver.
> 
> The incentive to change the API is a combination of:
> 
>  - The restriction to keep the pin driving at a certain level after
>    disable isn't a natural and obvious property of the hardware.
>    The designers of the imx-pwm for example did it differently.

I would disagree. A pin that you can't keep at a defined level if it
isn't actively driven is a fundamentally useless concept. You said
yourself that the hardware design is problematic because it causes the
pin to change unexpectedly when you disable the PWM. One could argue
that that's a bug in the hardware. However, given that there is a way to
avoid the unexpected change, there is a mechanism outside of PWM to deal
with this use-case. And that's also perfectly fine. Dealing with pins
that aren't actively driven is part of the job of a pinmux controller.

>  - The change removes ambiguity from from the demands on the hardware
>    drivers.

I don't think there's currently any ambiguity. We may not be stating
explicitly that the assumption is for a PWM to be "off" after
pwm_disable(), but it's certainly how everyone has interpreted it. Also
if there's an ambiguity, then that's what should be addressed.

>  - The change doesn't make the API less expressive.

Yes it does. You remove the distinction between duty cycle == 0 and
the PWM being disabled.

>  - The change doesn't doesn't complicate anmy pwm hardware driver.

True. It also doesn't simplify any PWM hardware driver.

>  - The change allows to simplify most pwm users.

Yes, the subset that doesn't care about the difference is marginally
simplified. Although if the API was more rigorous about the definition
of pwm_disable(), then PWM users would be even more simplified.

>  - The change simplifies the imx-pwm driver.

True. But you leave out that the change doesn't actually fix your
problem. If your PWM user goes away, it will still want to disable the
PWM because it is no longer in use. However, you don't want that to
mean that the backlight goes to full brightness again. If you unbind
the backlight driver, the backlight should remain off (or turn off if
it isn't already).

> In my experience this is enough to justify such a change.

I would consider the change if it was actually going to fix the issue
that you're seeing. As it is, all your proposal does is paper over one
specific symptom, and I'm sorry, but that's just not good enough.

> > So we already know that currently all users expect that the pin state
> > after disable will be low (because they previously set duty cycle to 0,
> > which is the active equivalent of low).
> 
> My theory is that most users don't expect this and/or don't make use of
> it. As of 9dcd936c5312 we have 50 pwm drivers
> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> and I think there are a few false positive matches, pwm-sun4i.c for
> example). I'd be willing to bet that as of now imx isn't the only driver
> with this problem.

We can speculate about that as much as we want. i.MX is the only one
that this issue has been reported against, so I'm going to have to
assume that it's working fine for all of the others. If it isn't then
people should come forward and report it.

> > Given that stricter definition,
> > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > state. I think the fix for that is for the i.MX PWM driver to make sure
> > the pin state doesn't actually change on ->disable(). If that means the
> > clock needs to remain on, then that's exactly what the driver should be
> > implementing.
> 
> Yes, if we keep the API "stricter" this is what should be done. This
> doesn't justify to keep the API strict though.

But an API that isn't strict is useless. If the behaviour of a PWM isn't
predictable, how can you use it to drive a backlight. If at any point it
could just go 100% and turn on the backlight to full brightness, how can
anyone use it for anything sensible?

> > Are you aware of this:
> > 
> > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > 
> > This seems to address exactly the problem that you're describing. The
> > solution matches what I had expected a fix for this problem to look
> > like. Can you try it and see if that fixes the issue?
> 
> Right. Apart from bugs in it (for example
> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> as output) this would solve the issue for sure. This doesn't void my
> arguments to relax the PWM API though. Also look at the costs: The patch
> adds 86 lines to the imx driver complicating it considerably.

Now you're just being dramatic. 86 lines of code is not a lot. And I
don't think it's complicated. I'm willing to carry the burden of those
86 lines of code if it fixes the issue for good.

>                                                               Each
> machine that wants to profit of this change has to adapt it's device
> tree to make the additional pinmuxing and gpio known.
> To be fair, my change doesn't fix the gap between pinctrl being active
> and the actual enablement of the pwm. But this could be better solved in
> the pwm framework without touching any hardware driver with pinctrl
> mechanism that are already in place. (Make use of the "init" pinctrl
> state and only switch to "default" when the pwm is configured.)
> There is nothing imx-pwm specific in hooking a gpio and adapting the
> pinmuxing in it. So if you want to go this path, please implement it in
> a generic way such that all pwm devices could benefit.

You're wrong. Judging by all the evidence that we have at this point,
this is exactly an imx-pwm specific issue, therefore the burden is on
the imx-pwm driver to implement a solution for it. If there ever is a
second driver that has this issue we can revisit moving this into the
core. I don't see any reason to do so at this point in time with the
information that we have.

> > It looks as if that also fixes the shutdown problem, so it's better than
> > what you're proposing here yet it doesn't require any API change at all
> > to make it work.
> 
> I don't care much about the shutdown problem. (Also, if the imx-pwm
> driver is unbound, the gpio is freed, too, so it's only by chance if the
> level is kept.)

Who else do you think would use the GPIO and change the level? If you've
got multiple concurrent users of the pin that's a whole other problem. I
also don't see why you wouldn't want to care about the shutdown problem.
It's annoying if you shut down the device and all of a sudden your
backlight comes back up again, perhaps in the middle of the display
controller shutting down and producing garbage on the screen.

> 
> > > > What you're currently proposing is unjustified at this point: you're
> > > > trying to work around an issue specific to i.MX by changing an API that
> > > > has worked fine for everyone else for a decade.
> > > 
> > > I don't agree here. The PWM API as it is now would make it necessary
> > > that the imx driver becomes ugly.
> > 
> > Beauty is very subjective. This doesn't count as an argument. Michal's
> > proposed solution looks perfectly fine.
> 
> In this case the opposite of ugly is "simple". This is a real argument.
> And this is a reason to apply my idea. It simplifies stuff and solves
> some problems.
> 
> A framework is good if it is general enough to cover all use cases for
> its users and demands very little from it's implementors. Each callback
> to implement by a hardware driver should be as simple as possible.
> Implying "keep the pin level constant" in .disable makes it more
> complicated for some drivers. So this is a bad restriction that should
> better be dropped if there are no relvant downsides.

Okay, so I'm not suggesting that pwm_disable() keep the pin level
constant. What I was proposing is that it be required to set the pin
level to "off" (whatever that means). And the reason for doing that is
because there are relevant downsides if we leave this undefined, as I
described at length above.

> Sidenote: With the current capabilities of the pwm framework there is no
> technical reason to expose to the hardware drivers that the pwm user uses
> inverted logic. The framework could just apply
> 
> 	duty = period - duty;
> 
> before calling the hardware specific .config callback and so allow to
> simplify some drivers, reducing complexity to a single place.

No, you're wrong. If you only consider the case of a backlight or an
LED, then yes, you could simplify it that way. However there are other
use-cases that require more fine-grained control and in those cases the
actual edges of the PWM are important.

Also, this has been discussed before and I have said elsewhere that I
have no objection to adding a helper that would simplify this for
consumers that don't care about the difference.

> > > By changing the semantic of the API
> > > 
> > >  - the i.MX driver can be implemented nicely;
> > >  - the other drivers for "saner" hardware can stay as they are;
> > >  - the API users can still express all their needs;
> > >  - the API stops having two ways to express the same needs;
> > 
> > Again, duty-cycle = 0 is not the same as setting the state to disabled.
> > 
> > Practically they usually have the same effect, but that's just because
> > in the typical use-case we don't care about the subtle differences.
> 
> I fail to see the subtle difference.

That's okay. I've tried my best to describe it to you in different ways.
If I still haven't been able to convey this, I don't know what else to
say.

> > >  - most API users can be simplified because they can drop
> > >      if (duty == 0) pwm_disable()
> > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > 
> > That's one reason why we have pwm_disable(). It's what you use to save
> > some energy if you don't need the PWM. Also, you claim that if the clock
> > is disabled the attached backlight will turn on at full brightness, so I
> > fail to see how this would work.
> 
> As long as you care about the backlight to be off, don't disable the
> pwm. And if in this state the clk can be disabled, then this should be
> done without an additional poke by the hardware driver which is the only
> place that can know for sure that it is safe to do so. There is no good
> reason to let the pwm user have to know that the currently configured
> state might make some energy saving possible and impose the burden on
> him to then call pwm_disable().

This doesn't have anything to do with consumers knowing about potential
energy savings. All we do is give the consumers an opportunity to say: I
need the PWM to be enabled because I want to use the PWM signal now or I
don't really need the PWM signal any more, just stop sending a signal.
This is merely a hint from the consumer. What the driver does with it is
ultimately up to the driver. If the driver wants to disable the clock
when the duty cycle goes to zero because it knows that it is safe to do
so, then by all means, feel free to do that.

> > Also, you leave out the fact that even after all this work that touches
> > all users and all PWM drivers you still don't get fully correct
> > functionality on your devices because when the driver is removed or shut
> > down you run into the same problem.
> 
> It's ok for me that all bets are off when the driver is removed.

Well, it's not okay for me. Especially if we can do better.

>                                                                  I'm
> happy when I fix the problem during active operation because on the
> machine I currently care about the shutdown problem isn't relevant.

Why is it not relevant? Surely at some point you will want to shut down
that machine. And even if not, imx-pwm is used by others and they do
seem to care about shutdown, so they should have a say in the matter as
well.

> (Also as noted above, even Michal's patch doesn't solve the problem
> formally.)

Now I'm confused, you said above that "Apart from bugs in it ... this
would solve the issue for sure". So which one is it?

> > > Regarding you claim that the API worked fine for a decade: I tried
> > > already in 2013 to address this problem. And even independent of that,
> > > that is not a good reason to not improve stuff.
> > 
> > My claim was that it has worked fine "for everyone else". I know that
> > this has been an issue on i.MX for a long time and I've done my best to
> > lay out why I think your proposal is not the right way to fix it.
> 
> I still fail to see a single technical reason to not evolve the API. I
> read from your mails:
> 
>  - it has been like that for ages
>  - it works for everyone but i.MX
>  - it could be made working for i.MX by involving pinctrl and gpio in
>    the hardware specific pwm driver.
> 
> Did I miss anything? None of these justifies to keep the status quo in
> my eyes.

I don't understand your logic here. You want to change the API, so the
burden is on you to convince me *why* it needs to be changed. So far you
have not been able to do that, so don't try to turn this around and make
me come up with reasons why it shouldn't change.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Vokáč Michal]

Uwe, Thierry,
just a few comments as I am not able to dive deeper into your discussion.

On 11.10.2018 14:00, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
>> Hello Thierry,
>>
>> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
>>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
>>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
>>>>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
>>>>>> Hello Thierry,
>>>>>>
>>>>>> (If you have a déjà vu: Yes, I tried to argue this case already in the
>>>>>> past, it's just that I'm just faced with a new use case that's broken
>>>>>> with the current state.)
>>>>>>
>>>>>> Currently the idiom
>>>>>>
>>>>>> 	pwm_config(mypwm, value, period);
>>>>>> 	if (!value)
>>>>>> 		pwm_disable(mypwm);
>>>>>> 	else
>>>>>> 		pwm_enable(mypwm);
>>>>>>
>>>>>> (and it's equivalent:
>>>>>>
>>>>>> 	state.duty_cycle = ...
>>>>>> 	state.enable = state.duty_cycle ? true : false;
>>>>>> 	pwm_apply_state(...);
>>>>>>
>>>>>> ) is quite present in the kernel.
>>>>>>
>>>>>> In my eyes this is bad for two reasons:
>>>>>>
>>>>>>   a) if the caller is supposed to call pwm_disable() after configuring the
>>>>>>      duty cycle to zero, then why doesn't pwm_config() contains this to
>>>>>>      unburden pwm-API users and so reduce open coding this assumption?
>>>>>
>>>>> Just to reiterate my thoughts on this: while the config/enable/disable
>>>>> split may seem arbitrary for some use-cases (you may argue most
>>>>> use-cases, and you'd probably be right), I think there's value in having
>>>>> the additional flexibility to explicitly turn off the PWM. Also, duty
>>>>> cycle of zero doesn't always mean "off". If your PWM is inverted, then
>>>>> the duty cycle of zero actually means "on". And that of course also
>>>>> still depends on other components on your board.
>>>>
>>>> Did you notice you didn't argue against me here? We seem to agree that
>>>> calling disabling a PWM after setting the duty cycle to zero is bad.
>>
>> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
>> also don't seem to refuse to change pwm users that do:
>>
>> 	pwm_config(pwm, 0, 100);
>> 	pwm_disable(pwm);
>>
>> I interpreted your sentence as confirmation that this sequence isn't
>> correct in general.
> 
> Yes, it is not correct in general, though it is correct in many cases.
> 
>> Where is the difference you care about between duty=0 and disabled?
> 
> This is described in the kerneldoc of the corresponding functions. In the
> case of pwm_enable() it starts the output toggling, whereas pwm_enable()
> disables the output toggling. So strictly by going according to the PWM
> API a driver should only need to call pwm_config() if the duty cycle
> actually changes. If all you do is disable and enable, then pwm_enable()
> and pwm_disable() should be good enough. The implicit assumption is that
> the configuration will persist across pwm_disable()/pwm_enable(). In
> practice this doesn't matter that much because most users will be
> setting the duty cycle and enable/disable at the same time. I can easily
> imagine other use-cases where you would want to make the different. One
> other difference could be that pwm_enable() and pwm_disable() require a
> more involved sequence of operations than pwm_config() or vice versa, in
> which cases you may want to avoid one or the other if possible.
> 
> In addition, we do expose these details to userspace via sysfs, so not
> allowing an explicit disable after config(duty=0) would break that ABI.
> 
>>> I don't see why you think I agree with that. The only thing I'm saying
>>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
>>> same as the PWM being disabled.
>>
>> What is in your view the current difference for the hardware behind the
>> PWM?
> 
> It depends on the driver implementation, but the kerneldoc is pretty
> specific. I many cases I would expect pwm_disable() to allow a bit of
> extra power to be saved because clocks could be turned off.
> 
>>>>> You may have an inverter somewhere, or you may actually be driving a
>>>>> circuit that expects an inverted PWM.
>>>>>
>>>>> So saying that duty-cycle of 0 equals disable is simplifying things too
>>>>> much, in my opinion.
>>>>
>>>> Full ack. So let's fix the users to not call pwm_disable after setting
>>>> the duty cycle to zero!
>>>
>>> I don't understand why you come to that conclusion. It seems like we at
>>> least agree on what the problem is and what potential issues there could
>>> be with different setups.
>>>
>>> The point that we don't agree with is how to fix this.
>>
>> ack.
>>
>>>> Really, only the driver knows when it's safe to disable the clock while
>>>> it's expected to keep the pin at a given state. So better don't expect a
>>>> certain state after pwm_disable().
>>>
>>> Yes, I agree that only the driver knows when it is safe to do so, but I
>>> still fail to understand why we should change the API in order to
>>> accomodate the specifics of a particular driver.
>>
>> The incentive to change the API is a combination of:
>>
>>   - The restriction to keep the pin driving at a certain level after
>>     disable isn't a natural and obvious property of the hardware.
>>     The designers of the imx-pwm for example did it differently.
> 
> I would disagree. A pin that you can't keep at a defined level if it
> isn't actively driven is a fundamentally useless concept. You said
> yourself that the hardware design is problematic because it causes the
> pin to change unexpectedly when you disable the PWM. One could argue
> that that's a bug in the hardware. However, given that there is a way to
> avoid the unexpected change, there is a mechanism outside of PWM to deal
> with this use-case. And that's also perfectly fine. Dealing with pins
> that aren't actively driven is part of the job of a pinmux controller.
> 
>>   - The change removes ambiguity from from the demands on the hardware
>>     drivers.
> 
> I don't think there's currently any ambiguity. We may not be stating
> explicitly that the assumption is for a PWM to be "off" after
> pwm_disable(), but it's certainly how everyone has interpreted it. Also
> if there's an ambiguity, then that's what should be addressed.
> 
>>   - The change doesn't make the API less expressive.
> 
> Yes it does. You remove the distinction between duty cycle == 0 and
> the PWM being disabled.
> 
>>   - The change doesn't doesn't complicate anmy pwm hardware driver.
> 
> True. It also doesn't simplify any PWM hardware driver.
> 
>>   - The change allows to simplify most pwm users.
> 
> Yes, the subset that doesn't care about the difference is marginally
> simplified. Although if the API was more rigorous about the definition
> of pwm_disable(), then PWM users would be even more simplified.
> 
>>   - The change simplifies the imx-pwm driver.
> 
> True. But you leave out that the change doesn't actually fix your
> problem. If your PWM user goes away, it will still want to disable the
> PWM because it is no longer in use. However, you don't want that to
> mean that the backlight goes to full brightness again. If you unbind
> the backlight driver, the backlight should remain off (or turn off if
> it isn't already).
> 
>> In my experience this is enough to justify such a change.
> 
> I would consider the change if it was actually going to fix the issue
> that you're seeing. As it is, all your proposal does is paper over one
> specific symptom, and I'm sorry, but that's just not good enough.
> 
>>> So we already know that currently all users expect that the pin state
>>> after disable will be low (because they previously set duty cycle to 0,
>>> which is the active equivalent of low).
>>
>> My theory is that most users don't expect this and/or don't make use of
>> it. As of 9dcd936c5312 we have 50 pwm drivers
>> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
>> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
>> and I think there are a few false positive matches, pwm-sun4i.c for
>> example). I'd be willing to bet that as of now imx isn't the only driver
>> with this problem.
> 
> We can speculate about that as much as we want. i.MX is the only one
> that this issue has been reported against, so I'm going to have to
> assume that it's working fine for all of the others. If it isn't then
> people should come forward and report it.
> 
>>> Given that stricter definition,
>>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
>>> state. I think the fix for that is for the i.MX PWM driver to make sure
>>> the pin state doesn't actually change on ->disable(). If that means the
>>> clock needs to remain on, then that's exactly what the driver should be
>>> implementing.
>>
>> Yes, if we keep the API "stricter" this is what should be done. This
>> doesn't justify to keep the API strict though.
> 
> But an API that isn't strict is useless. If the behaviour of a PWM isn't
> predictable, how can you use it to drive a backlight. If at any point it
> could just go 100% and turn on the backlight to full brightness, how can
> anyone use it for anything sensible?
> 
>>> Are you aware of this:
>>>
>>> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
>>>
>>> This seems to address exactly the problem that you're describing. The
>>> solution matches what I had expected a fix for this problem to look
>>> like. Can you try it and see if that fixes the issue?
>>
>> Right. Apart from bugs in it (for example
>> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
>> as output)

It is a RFC and the past discussion about it was mostly theoretical than
about technical details. So I thought that nobody would really expect
excellent technical solution at this point. I just tried to do my best
and tested dozens of variations of [enabled/disabled/inverted/non-
inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
affects current users and how it improves the situation.

Regarding the GPIO output configuration. Yes, the pin is configured as
input in the imx_pwm_init_pinctrl_info() function. At this stage you
do not really know how to configure the pin. Configuring it as input
is the safest option I think.

I thought that the pin is configured as output whenever gpiod_set_value()
is called. None of the other examples/usages in kernel I looked at used
the gpiod_direction_output(). So thanks for pointing that out, I can fix
that.

>> this would solve the issue for sure. This doesn't void my
>> arguments to relax the PWM API though. Also look at the costs: The patch
>> adds 86 lines to the imx driver complicating it considerably.
> 
> Now you're just being dramatic. 86 lines of code is not a lot. And I
> don't think it's complicated. I'm willing to carry the burden of those
> 86 lines of code if it fixes the issue for good.
> 
>>                                                                Each
>> machine that wants to profit of this change has to adapt it's device
>> tree to make the additional pinmuxing and gpio known.
>> To be fair, my change doesn't fix the gap between pinctrl being active
>> and the actual enablement of the pwm. But this could be better solved in
>> the pwm framework without touching any hardware driver with pinctrl
>> mechanism that are already in place. (Make use of the "init" pinctrl
>> state and only switch to "default" when the pwm is configured.)
>> There is nothing imx-pwm specific in hooking a gpio and adapting the
>> pinmuxing in it. So if you want to go this path, please implement it in
>> a generic way such that all pwm devices could benefit.
> 
> You're wrong. Judging by all the evidence that we have at this point,
> this is exactly an imx-pwm specific issue, therefore the burden is on
> the imx-pwm driver to implement a solution for it. If there ever is a
> second driver that has this issue we can revisit moving this into the
> core. I don't see any reason to do so at this point in time with the
> information that we have.
> 
>>> It looks as if that also fixes the shutdown problem, so it's better than
>>> what you're proposing here yet it doesn't require any API change at all
>>> to make it work.
>>
>> I don't care much about the shutdown problem. (Also, if the imx-pwm
>> driver is unbound, the gpio is freed, too, so it's only by chance if the
>> level is kept.)
> 
> Who else do you think would use the GPIO and change the level? If you've
> got multiple concurrent users of the pin that's a whole other problem. I
> also don't see why you wouldn't want to care about the shutdown problem.
> It's annoying if you shut down the device and all of a sudden your
> backlight comes back up again, perhaps in the middle of the display
> controller shutting down and producing garbage on the screen.
> 
>>
>>>>> What you're currently proposing is unjustified at this point: you're
>>>>> trying to work around an issue specific to i.MX by changing an API that
>>>>> has worked fine for everyone else for a decade.
>>>>
>>>> I don't agree here. The PWM API as it is now would make it necessary
>>>> that the imx driver becomes ugly.
>>>
>>> Beauty is very subjective. This doesn't count as an argument. Michal's
>>> proposed solution looks perfectly fine.
>>
>> In this case the opposite of ugly is "simple". This is a real argument.
>> And this is a reason to apply my idea. It simplifies stuff and solves
>> some problems.
>>
>> A framework is good if it is general enough to cover all use cases for
>> its users and demands very little from it's implementors. Each callback
>> to implement by a hardware driver should be as simple as possible.
>> Implying "keep the pin level constant" in .disable makes it more
>> complicated for some drivers. So this is a bad restriction that should
>> better be dropped if there are no relvant downsides.
> 
> Okay, so I'm not suggesting that pwm_disable() keep the pin level
> constant. What I was proposing is that it be required to set the pin
> level to "off" (whatever that means). And the reason for doing that is
> because there are relevant downsides if we leave this undefined, as I
> described at length above.
> 
>> Sidenote: With the current capabilities of the pwm framework there is no
>> technical reason to expose to the hardware drivers that the pwm user uses
>> inverted logic. The framework could just apply
>>
>> 	duty = period - duty;
>>

I prefer to utilize as much HW capabilities as possible. And it seems
like most PWM chips support HW output inversion (to some extent) so to
me it seems perfectly OK that that information can be propagated from
the upper SW levels to the lowest one.

>> before calling the hardware specific .config callback and so allow to
>> simplify some drivers, reducing complexity to a single place.
> 
> No, you're wrong. If you only consider the case of a backlight or an
> LED, then yes, you could simplify it that way. However there are other
> use-cases that require more fine-grained control and in those cases the
> actual edges of the PWM are important.
> 
> Also, this has been discussed before and I have said elsewhere that I
> have no objection to adding a helper that would simplify this for
> consumers that don't care about the difference.
> 
>>>> By changing the semantic of the API
>>>>
>>>>   - the i.MX driver can be implemented nicely;
>>>>   - the other drivers for "saner" hardware can stay as they are;
>>>>   - the API users can still express all their needs;
>>>>   - the API stops having two ways to express the same needs;
>>>
>>> Again, duty-cycle = 0 is not the same as setting the state to disabled.
>>>
>>> Practically they usually have the same effect, but that's just because
>>> in the typical use-case we don't care about the subtle differences.
>>
>> I fail to see the subtle difference.
> > That's okay. I've tried my best to describe it to you in different ways.
> If I still haven't been able to convey this, I don't know what else to
> say.
> 
>>>>   - most API users can be simplified because they can drop
>>>>       if (duty == 0) pwm_disable()
>>>>   - on i.MX the clock can be disabled when not needed saving some energy;
>>>
>>> That's one reason why we have pwm_disable(). It's what you use to save
>>> some energy if you don't need the PWM. Also, you claim that if the clock
>>> is disabled the attached backlight will turn on at full brightness, so I
>>> fail to see how this would work.
>>
>> As long as you care about the backlight to be off, don't disable the
>> pwm. And if in this state the clk can be disabled, then this should be
>> done without an additional poke by the hardware driver which is the only
>> place that can know for sure that it is safe to do so. There is no good
>> reason to let the pwm user have to know that the currently configured
>> state might make some energy saving possible and impose the burden on
>> him to then call pwm_disable().
> 
> This doesn't have anything to do with consumers knowing about potential
> energy savings. All we do is give the consumers an opportunity to say: I
> need the PWM to be enabled because I want to use the PWM signal now or I
> don't really need the PWM signal any more, just stop sending a signal.
> This is merely a hint from the consumer. What the driver does with it is
> ultimately up to the driver. If the driver wants to disable the clock
> when the duty cycle goes to zero because it knows that it is safe to do
> so, then by all means, feel free to do that.
> 
>>> Also, you leave out the fact that even after all this work that touches
>>> all users and all PWM drivers you still don't get fully correct
>>> functionality on your devices because when the driver is removed or shut
>>> down you run into the same problem.
>>
>> It's ok for me that all bets are off when the driver is removed.
> 
> Well, it's not okay for me. Especially if we can do better.
> 
>>                                                                   I'm
>> happy when I fix the problem during active operation because on the
>> machine I currently care about the shutdown problem isn't relevant.
> 
> Why is it not relevant? Surely at some point you will want to shut down
> that machine. And even if not, imx-pwm is used by others and they do
> seem to care about shutdown, so they should have a say in the matter as
> well.
> 
>> (Also as noted above, even Michal's patch doesn't solve the problem
>> formally.)
> 
> Now I'm confused, you said above that "Apart from bugs in it ... this
> would solve the issue for sure". So which one is it?
> 
>>>> Regarding you claim that the API worked fine for a decade: I tried
>>>> already in 2013 to address this problem. And even independent of that,
>>>> that is not a good reason to not improve stuff.
>>>
>>> My claim was that it has worked fine "for everyone else". I know that
>>> this has been an issue on i.MX for a long time and I've done my best to
>>> lay out why I think your proposal is not the right way to fix it.
>>
>> I still fail to see a single technical reason to not evolve the API. I
>> read from your mails:
>>
>>   - it has been like that for ages
>>   - it works for everyone but i.MX
>>   - it could be made working for i.MX by involving pinctrl and gpio in
>>     the hardware specific pwm driver.
>>
>> Did I miss anything? None of these justifies to keep the status quo in
>> my eyes.
> 
> I don't understand your logic here. You want to change the API, so the
> burden is on you to convince me *why* it needs to be changed. So far you
> have not been able to do that, so don't try to turn this around and make
> me come up with reasons why it shouldn't change.
> 
> Thierry
> 

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Thu, Oct 11, 2018 at 02:00:07PM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> > On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> > > On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> > > > On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> > > > > On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> > You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> > also don't seem to refuse to change pwm users that do:

Just noticed the above has a "n't" to much. I don't think you
misunderstood my point because of that though.
 
> > 	pwm_config(pwm, 0, 100);
> > 	pwm_disable(pwm);
> > 
> > I interpreted your sentence as confirmation that this sequence isn't
> > correct in general.
> 
> Yes, it is not correct in general, though it is correct in many cases.

Can you point out where it would not be correct?

> > Where is the difference you care about between duty=0 and disabled?
> 
> This is described in the kerneldoc of the corresponding functions. In the
> case of pwm_enable() it starts the output toggling, whereas pwm_enable()

s/pwm_enable()$/pwm_disable/ I assume.

> disables the output toggling. So strictly by going according to the PWM
> API a driver should only need to call pwm_config() if the duty cycle
> actually changes. If all you do is disable and enable, then pwm_enable()
> and pwm_disable() should be good enough.

Not sure we're looking on the same kerneldoc. pwm_disable only
specifies:

	pwm_disable() - stop a PWM output toggling

. If you call pwm_disable() for an i.MX device it doesn't toggle any
more. OK, after configuring the pwm as inverted and 0% duty (or not
inverted and 100% duty) you get an additional edge, but I do think it's
a bit bold to deduce that the pin should keep it's state from the above
specification.

> The implicit assumption is that the configuration will persist across
> pwm_disable()/pwm_enable(). In practice this doesn't matter that much
> because most users will be setting the duty cycle and enable/disable
> at the same time. I can easily imagine other use-cases where you would
> want to make the different. One other difference could be that
> pwm_enable() and pwm_disable() require a more involved sequence of
> operations than pwm_config() or vice versa, in which cases you may
> want to avoid one or the other if possible.

OK, A PWM user that has to switch between say 50% duty cycle and a
constant value can use pwm_enable() and pwm_disable() to change between
these states (after calling pwm_config(pwm, period / 2, period) once).

Similarly it can use

	pwm_config(pwm, 0, period);

and

	pwm_config(pwm, period / 2, period);

to toggle.

I confirm, with my suggestion the first alternative would go away. This
isn't a big loss though because the second stays around.

Now as you refuse to remove this first alternative there should be a
relevant semantic difference between the two. There are only a single
difference I see: With the first way (using pwm_enable() and
pwm_disable()) the PWM user doesn't control if the PWM stops at 0 or 1,
using the 2nd approach the PWM has to stop at 0.

Currently power consumption might be another difference, but you stated
below this wasn't relevant.

> In addition, we do expose these details to userspace via sysfs, so not
> allowing an explicit disable after config(duty=0) would break that ABI.

I wouldn't have a big problem with that even if the "strict" expectation
would be documented explicitly. The documentation for
/sys/class/pwm/pwmchipN/pwmX/enable only states:

	Enable/disable the PWM signal.

which I wouldn't consider to break with my suggestion.

> > What is in your view the current difference for the hardware behind the
> > PWM?
> 
> It depends on the driver implementation, but the kerneldoc is pretty
> specific. I many cases I would expect pwm_disable() to allow a bit of
> extra power to be saved because clocks could be turned off.

With my approach that expectation is given. With your's it is not in
every case because of the ambiguity of pwm_disable. On i.MX the clock
can only be disabled if the caller doesn't care about the resulting pin
state. The hardware driver however cannot know if this is the case and
so must keep the clock on even in the cases where it would be ok to
disable it.

> > > > Really, only the driver knows when it's safe to disable the clock while
> > > > it's expected to keep the pin at a given state. So better don't expect a
> > > > certain state after pwm_disable().
> > > 
> > > Yes, I agree that only the driver knows when it is safe to do so, but I
> > > still fail to understand why we should change the API in order to
> > > accomodate the specifics of a particular driver.
> > 
> > The incentive to change the API is a combination of:
> > 
> >  - The restriction to keep the pin driving at a certain level after
> >    disable isn't a natural and obvious property of the hardware.
> >    The designers of the imx-pwm for example did it differently.
> 
> I would disagree. A pin that you can't keep at a defined level if it
> isn't actively driven is a fundamentally useless concept. You said
> yourself that the hardware design is problematic because it causes the
> pin to change unexpectedly when you disable the PWM.

It's only unexpected if you are used to the strict API design.

And if you don't agree to this point, I'm missing ambition on your side.
If the strict API is only suitable for "sane" pwm hardware instead of
being general enough to also cover some of the "stranger" designs,
that's what I call useless.

> One could argue that that's a bug in the hardware. 

And one could argue further that it's a shortcoming of the PWM framework
that it is unable to handle it better, though it easily could.

> However, given that there is a way to avoid the unexpected change,
> there is a mechanism outside of PWM to deal with this use-case. And
> that's also perfectly fine. Dealing with pins that aren't actively
> driven is part of the job of a pinmux controller.
> 
> >  - The change removes ambiguity from from the demands on the hardware
> >    drivers.
> 
> I don't think there's currently any ambiguity. We may not be stating
> explicitly that the assumption is for a PWM to be "off" after
> pwm_disable(), but it's certainly how everyone has interpreted it. Also
> if there's an ambiguity, then that's what should be addressed.

"off" isn't a clear definition. For you it is (assuming I understood you
right) "stop immediately and keep driving the current level". For the
imx hardware designer it is, "stop toggling, output a 0". For others it
might be: "Stop driving the pin".

The ambiguity is that sometimes the PWM is disabled with the need to
keep the output driving at a certain level. And sometimes the PWM is
disabled and the output doesn't matter. In both cases pwm_disable() is
used and the driver cannot know in which of the two cases we are and if
it's ok to release the pin or not.

> >  - The change doesn't make the API less expressive.
> 
> Yes it does. You remove the distinction between duty cycle == 0 and
> the PWM being disabled.

Currently there is no distinction that is relevant for PWM users. (If
you don't agree, please provide an example.) In both cases the output is
constant 0 (assuming a non-inverted PWM).

The PWM user can still request that constant 0 with the laxer API. The
only difference in the strict API is that

	pwm_config(pwm, 0, 100)

doesn't disable the clock and

	pwm_disable(pwm)

does. There is no good reason however for a driver of a "sane" PWM to
not disable the clock in reply to pwm_config(pwm, 0, 100) though.

If you don't agree, can you show me a single PWM user that cares about
the distinction?

> >  - The change doesn't doesn't complicate any pwm hardware driver.
> 
> True. It also doesn't simplify any PWM hardware driver.

see "The change simplifies the imx-pwm driver." below that you
confirmed.

> >  - The change allows to simplify most pwm users.
> 
> Yes, the subset that doesn't care about the difference is marginally
> simplified. Although if the API was more rigorous about the definition
> of pwm_disable(), then PWM users would be even more simplified.

I cannot follow. How should/could the API be more rigorous to simplify
PWM users? Is this off-topic for the current discussion?

> >  - The change simplifies the imx-pwm driver.
> 
> True. But you leave out that the change doesn't actually fix your
> problem. If your PWM user goes away, it will still want to disable the
> PWM because it is no longer in use. However, you don't want that to
> mean that the backlight goes to full brightness again. If you unbind
> the backlight driver, the backlight should remain off (or turn off if
> it isn't already).

This advantage isn't about "solving my problems". It is about
simplification (as I wrote).

> > In my experience this is enough to justify such a change.
> 
> I would consider the change if it was actually going to fix the issue
> that you're seeing. As it is, all your proposal does is paper over one
> specific symptom, and I'm sorry, but that's just not good enough.

Correct, it doesn't completely fix the issue. It improves the situation
considerably though and has the advantages that you confirmed above.

> > > So we already know that currently all users expect that the pin state
> > > after disable will be low (because they previously set duty cycle to 0,
> > > which is the active equivalent of low).
> > 
> > My theory is that most users don't expect this and/or don't make use of
> > it. As of 9dcd936c5312 we have 50 pwm drivers
> > ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> > at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> > and I think there are a few false positive matches, pwm-sun4i.c for
> > example). I'd be willing to bet that as of now imx isn't the only driver
> > with this problem.
> 
> We can speculate about that as much as we want. i.MX is the only one
> that this issue has been reported against, so I'm going to have to
> assume that it's working fine for all of the others. If it isn't then
> people should come forward and report it.

You really assume that all the drivers that don't make use of
PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
you expect when the PWM user does:

	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })

? I'd say your assumption that all are working fine is treading on thin
ice. The effect maybe is only that the i.MX users use more corners of
the PWM API and/or are more open to report actual problems.

Also if there are two ways to implement something, one works for 49
different hardware implementations and one for 50 this is already a good
reason to stick to the latter.

> > > Given that stricter definition,
> > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > > state. I think the fix for that is for the i.MX PWM driver to make sure
> > > the pin state doesn't actually change on ->disable(). If that means the
> > > clock needs to remain on, then that's exactly what the driver should be
> > > implementing.
> > 
> > Yes, if we keep the API "stricter" this is what should be done. This
> > doesn't justify to keep the API strict though.
> 
> But an API that isn't strict is useless. If the behaviour of a PWM isn't
> predictable, how can you use it to drive a backlight. If at any point it
> could just go 100% and turn on the backlight to full brightness, how can
> anyone use it for anything sensible?

As long as I don't call pwm_disable() (or pwm_config(pwm, period,
period)) it must not go to 100%.

> > > Are you aware of this:
> > > 
> > > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > > 
> > > This seems to address exactly the problem that you're describing. The
> > > solution matches what I had expected a fix for this problem to look
> > > like. Can you try it and see if that fixes the issue?
> > 
> > Right. Apart from bugs in it (for example
> > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> > as output) this would solve the issue for sure. This doesn't void my
> > arguments to relax the PWM API though. Also look at the costs: The patch
> > adds 86 lines to the imx driver complicating it considerably.
> 
> Now you're just being dramatic. 86 lines of code is not a lot. And I
> don't think it's complicated. I'm willing to carry the burden of those
> 86 lines of code if it fixes the issue for good.

Right, the complexity doesn't come with the 86 lines of code. With my
approach the pwm-imx driver only has to care about its 6 registers. With
Michal's patch it suddenly has to care about gpio and pinmuxing.

> >                                                               Each
> > machine that wants to profit of this change has to adapt it's device
> > tree to make the additional pinmuxing and gpio known.
> > To be fair, my change doesn't fix the gap between pinctrl being active
> > and the actual enablement of the pwm. But this could be better solved in
> > the pwm framework without touching any hardware driver with pinctrl
> > mechanism that are already in place. (Make use of the "init" pinctrl
> > state and only switch to "default" when the pwm is configured.)
> > There is nothing imx-pwm specific in hooking a gpio and adapting the
> > pinmuxing in it. So if you want to go this path, please implement it in
> > a generic way such that all pwm devices could benefit.
> 
> You're wrong. Judging by all the evidence that we have at this point,
> this is exactly an imx-pwm specific issue, therefore the burden is on
> the imx-pwm driver to implement a solution for it.

You're the first maintainer I met with this attitude. Whenever I
implemented something in a driver specific way fixing an issue that
other drivers might have to, I was requested to find a generic solution
without the need to analyse if other drivers are affected or not.

There are some subsystems (e.g. tty/serial) where there is much stuff
implemented in each driver even though some things could well be
implemented in a generic way (e.g. rs485 handling). This is really a big
mess.

> > > It looks as if that also fixes the shutdown problem, so it's better than
> > > what you're proposing here yet it doesn't require any API change at all
> > > to make it work.
> > 
> > I don't care much about the shutdown problem. (Also, if the imx-pwm
> > driver is unbound, the gpio is freed, too, so it's only by chance if the
> > level is kept.)
> 
> Who else do you think would use the GPIO and change the level? If you've
> got multiple concurrent users of the pin that's a whole other problem. I
> also don't see why you wouldn't want to care about the shutdown problem.
> It's annoying if you shut down the device and all of a sudden your
> backlight comes back up again, perhaps in the middle of the display
> controller shutting down and producing garbage on the screen.

If someone on my machine is able to unbind the backlight driver making
the display flicker I have much more and graver problems than the
flickering display. And I didn't test (as I currently don't have an
affected device on my desk) but I think machine shutdown is no problem.

> > Sidenote: With the current capabilities of the pwm framework there is no
> > technical reason to expose to the hardware drivers that the pwm user uses
> > inverted logic. The framework could just apply
> > 
> > 	duty = period - duty;
> > 
> > before calling the hardware specific .config callback and so allow to
> > simplify some drivers, reducing complexity to a single place.
> 
> No, you're wrong. If you only consider the case of a backlight or an
> LED, then yes, you could simplify it that way. However there are other
> use-cases that require more fine-grained control and in those cases the
> actual edges of the PWM are important.

Can you please point out such a use case that is also possible to
implement with the current API?
 
> Also, this has been discussed before and I have said elsewhere that I
> have no objection to adding a helper that would simplify this for
> consumers that don't care about the difference.

I suggest a compromise below, maybe we can agree on that one.

> > > > By changing the semantic of the API
> > > > 
> > > >  - the i.MX driver can be implemented nicely;
> > > >  - the other drivers for "saner" hardware can stay as they are;
> > > >  - the API users can still express all their needs;
> > > >  - the API stops having two ways to express the same needs;
> > > 
> > > Again, duty-cycle = 0 is not the same as setting the state to disabled.
> > > 
> > > Practically they usually have the same effect, but that's just because
> > > in the typical use-case we don't care about the subtle differences.
> > 
> > I fail to see the subtle difference.
> 
> That's okay. I've tried my best to describe it to you in different ways.
> If I still haven't been able to convey this, I don't know what else to
> say.

I reread your explanation several times and fail to understand it. It
would be very helpful if you could come up with a scenario where it's
obvious that the distinction is needed. You're writing about "typical
use-cases" and "value in having the additional flexibility" without
providing a use-case that is atypical or actually need the flexibility.

Maybe I don't understand the semantic you're giving to the calls or
we're using the same words to describe different things. I try
to describe the semantic of pwm_disable of the strict API in my words,
can you please confirm or correct that?:

	pwm_disable stops toggling the PWM pin at whatever state the pin
	currently is. Given a duty cycle between 0% and 100% (exclusive)
	that means the pin stops at an undefined level. If the duty
	cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1
	respectively (and vice versa for an inverted one).

The semantic of pwm_enable in my understanding is:

	pwm_enable for an uninverted PWM starts with $duty_cycle
	nanoseconds at 0 and then is 1 for $period - $duty_cycle
	nanoseconds. Then repeat.
	For an inverted it is first 1 for $duty_cycle nanoseconds and
	then 0 for the rest of the period.

I think pwm_config is to be defined like this:

	pwm_config changes period and duty cycle without any
	specification about how the transition should be done.

All calls should be synchronous, that is should only return when the
change is actually "on the pin".

> > > >  - most API users can be simplified because they can drop
> > > >      if (duty == 0) pwm_disable()
> > > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > > 
> > > That's one reason why we have pwm_disable(). It's what you use to save
> > > some energy if you don't need the PWM. Also, you claim that if the clock
> > > is disabled the attached backlight will turn on at full brightness, so I
> > > fail to see how this would work.
> > 
> > As long as you care about the backlight to be off, don't disable the
> > pwm. And if in this state the clk can be disabled, then this should be
> > done without an additional poke by the hardware driver which is the only
> > place that can know for sure that it is safe to do so. There is no good
> > reason to let the pwm user have to know that the currently configured
> > state might make some energy saving possible and impose the burden on
> > him to then call pwm_disable().
> 
> This doesn't have anything to do with consumers knowing about potential
> energy savings. All we do is give the consumers an opportunity to say: I
> need the PWM to be enabled because I want to use the PWM signal now or I
> don't really need the PWM signal any more, just stop sending a signal.

There are two different degrees of "I don't need the PWM signal any
more". In the first case you still care about the output level and in
the other you don't. Currently the API doesn't give the PWM user the
possibility to specify which of the two are intended.

> This is merely a hint from the consumer. What the driver does with it is
> ultimately up to the driver. If the driver wants to disable the clock
> when the duty cycle goes to zero because it knows that it is safe to do
> so, then by all means, feel free to do that.

How should the driver know it is safe if the PWM user doesn't have a way
to tell if he cares about the output level?

> > > Also, you leave out the fact that even after all this work that touches
> > > all users and all PWM drivers you still don't get fully correct
> > > functionality on your devices because when the driver is removed or shut
> > > down you run into the same problem.
> > 
> > It's ok for me that all bets are off when the driver is removed.
> 
> Well, it's not okay for me. Especially if we can do better.

Fine, then lets give a way to PWM users to specify if we have to care
about the pin state after disable or not.

> >                                                                  I'm
> > happy when I fix the problem during active operation because on the
> > machine I currently care about the shutdown problem isn't relevant.
> 
> Why is it not relevant? Surely at some point you will want to shut down
> that machine.

Does a shutdown do anything with a running pwm software wise? The
leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls
pwm_free in some legacy configuration but otherwise does nothing that
would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm
convinced shutdown is fine here.

> And even if not, imx-pwm is used by others and they do
> seem to care about shutdown, so they should have a say in the matter as
> well.

Note that my suggestion doesn't make things worse for those users who
care. I'm used to that it's good enough if a patch improves the
situation for some users and doesn't make it worse for the others.

> > (Also as noted above, even Michal's patch doesn't solve the problem
> > formally.)
> 
> Now I'm confused, you said above that "Apart from bugs in it ... this
> would solve the issue for sure". So which one is it?

I'm sure that you must not assume that after

	gpio_direction_output(gpio, 0);
	gpio_free(gpio);

the line still drives the 0. In most cases this will be the case, but if
this makes the GPIO switch to being an input that should be correct
behaviour, too. That's why I wrote "formally" above.

Michal's patch solves the gap at startup and when the PWM user calls
disable() formally. At unbind when the gpio is freed it's "only" in
practise.

> > > > Regarding you claim that the API worked fine for a decade: I tried
> > > > already in 2013 to address this problem. And even independent of that,
> > > > that is not a good reason to not improve stuff.
> > > 
> > > My claim was that it has worked fine "for everyone else". I know that
> > > this has been an issue on i.MX for a long time and I've done my best to
> > > lay out why I think your proposal is not the right way to fix it.
> > 
> > I still fail to see a single technical reason to not evolve the API. I
> > read from your mails:
> > 
> >  - it has been like that for ages
> >  - it works for everyone but i.MX
> >  - it could be made working for i.MX by involving pinctrl and gpio in
> >    the hardware specific pwm driver.
> > 
> > Did I miss anything? None of these justifies to keep the status quo in
> > my eyes.
> 
> I don't understand your logic here. You want to change the API, so the
> burden is on you to convince me *why* it needs to be changed.

Changing the API isn't *needed*. But it's the simplest way to fix some
(not all) issues I see. Additionally it simplifies some things.

> So far you have not been able to do that, so don't try to turn this
> around and make me come up with reasons why it shouldn't change.

I listed several reasons to change the API, for some you confirmed them
to be valid. You listed some for keeping the API as is. As I'm
convinced my suggestion is good and your reasons are smaller (or even
bad), I try to argue against them.

The above question was my try to summarize your reasons and to make sure
the list is complete. I didn't try to turn anything around and if that
was your impression that's a misunderstanding.

With my current understanding the list of reasons to stick to the status
quo is:

 - it has been like that for ages
   (which is a poor reason to oppose to change);
 - it works for everyone but i.MX
   (which I doubt and with my replacement it works for all drivers and
   users as now including i.MX and maybe others that have the same issue
   without us knowing it currently);
 - it could be made working for i.MX by involving pinctrl and gpio in
   the hardware specific pwm driver (which is ridiculous because my
   solution is simpler); and
 - we're changing API
   (which I'd say is ok given that the specification isn't very specific
   and breaking the API for good reasons is not nice but acceptable IMHO)

In my eyes in the above list there is one valid reason to not change
(the last one). If you don't agree to change the userspace API we could
keep that constant and only change the kernel-internal API (which is a
bit ugly though).

The list of reasons in favour of a change are:

 - the pwm-imx driver can stay/become easier as with the stricter API;
 - ambiguity removed
   (hw drivers know in .disable if they can put the pin at a previously
   "forbidden" value)
 - users can be simplified by dropping some pwm_disable()

So now how can we come to a conclusion? Here are my suggestions for a
compromise:

What about adding a new callback that we could for example call
"disable_lax" that implements disabling the PWM without the guarantee to
put the pin in the "off" state?

We could also introduce a callback "freeze" (or disable_strict) that is
supposed to implement the "strict" disable. Then there is an objective
way to determine when all drivers are adapted to the updated model.

Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar
should already imply disabling clocks if possible without the need to
explicitly call pwm_disable() afterwards?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> Uwe, Thierry,
> just a few comments as I am not able to dive deeper into your discussion.
> 
> On 11.10.2018 14:00, Thierry Reding wrote:
> > On Thu, Oct 11, 2018 at 12:19:14PM +0200, Uwe Kleine-König wrote:
> >> Hello Thierry,
> >>
> >> On Wed, Oct 10, 2018 at 02:26:07PM +0200, Thierry Reding wrote:
> >>> On Tue, Oct 09, 2018 at 11:35:54AM +0200, Uwe Kleine-König wrote:
> >>>> On Tue, Oct 09, 2018 at 09:53:45AM +0200, Thierry Reding wrote:
> >>>>> On Mon, Aug 06, 2018 at 05:51:29PM +0200, Uwe Kleine-König wrote:
> >>>>>> Hello Thierry,
> >>>>>>
> >>>>>> (If you have a déjà vu: Yes, I tried to argue this case already in the
> >>>>>> past, it's just that I'm just faced with a new use case that's broken
> >>>>>> with the current state.)
> >>>>>>
> >>>>>> Currently the idiom
> >>>>>>
> >>>>>> 	pwm_config(mypwm, value, period);
> >>>>>> 	if (!value)
> >>>>>> 		pwm_disable(mypwm);
> >>>>>> 	else
> >>>>>> 		pwm_enable(mypwm);
> >>>>>>
> >>>>>> (and it's equivalent:
> >>>>>>
> >>>>>> 	state.duty_cycle = ...
> >>>>>> 	state.enable = state.duty_cycle ? true : false;
> >>>>>> 	pwm_apply_state(...);
> >>>>>>
> >>>>>> ) is quite present in the kernel.
> >>>>>>
> >>>>>> In my eyes this is bad for two reasons:
> >>>>>>
> >>>>>>   a) if the caller is supposed to call pwm_disable() after configuring the
> >>>>>>      duty cycle to zero, then why doesn't pwm_config() contains this to
> >>>>>>      unburden pwm-API users and so reduce open coding this assumption?
> >>>>>
> >>>>> Just to reiterate my thoughts on this: while the config/enable/disable
> >>>>> split may seem arbitrary for some use-cases (you may argue most
> >>>>> use-cases, and you'd probably be right), I think there's value in having
> >>>>> the additional flexibility to explicitly turn off the PWM. Also, duty
> >>>>> cycle of zero doesn't always mean "off". If your PWM is inverted, then
> >>>>> the duty cycle of zero actually means "on". And that of course also
> >>>>> still depends on other components on your board.
> >>>>
> >>>> Did you notice you didn't argue against me here? We seem to agree that
> >>>> calling disabling a PWM after setting the duty cycle to zero is bad.
> >>
> >> You wrote "Also, duty cycle of zero doesn't always mean \"off\"." but
> >> also don't seem to refuse to change pwm users that do:
> >>
> >> 	pwm_config(pwm, 0, 100);
> >> 	pwm_disable(pwm);
> >>
> >> I interpreted your sentence as confirmation that this sequence isn't
> >> correct in general.
> > 
> > Yes, it is not correct in general, though it is correct in many cases.
> > 
> >> Where is the difference you care about between duty=0 and disabled?
> > 
> > This is described in the kerneldoc of the corresponding functions. In the
> > case of pwm_enable() it starts the output toggling, whereas pwm_enable()
> > disables the output toggling. So strictly by going according to the PWM
> > API a driver should only need to call pwm_config() if the duty cycle
> > actually changes. If all you do is disable and enable, then pwm_enable()
> > and pwm_disable() should be good enough. The implicit assumption is that
> > the configuration will persist across pwm_disable()/pwm_enable(). In
> > practice this doesn't matter that much because most users will be
> > setting the duty cycle and enable/disable at the same time. I can easily
> > imagine other use-cases where you would want to make the different. One
> > other difference could be that pwm_enable() and pwm_disable() require a
> > more involved sequence of operations than pwm_config() or vice versa, in
> > which cases you may want to avoid one or the other if possible.
> > 
> > In addition, we do expose these details to userspace via sysfs, so not
> > allowing an explicit disable after config(duty=0) would break that ABI.
> > 
> >>> I don't see why you think I agree with that. The only thing I'm saying
> >>> is that pwm_config() shouldn't assume that a duty-cycle of 0 means the
> >>> same as the PWM being disabled.
> >>
> >> What is in your view the current difference for the hardware behind the
> >> PWM?
> > 
> > It depends on the driver implementation, but the kerneldoc is pretty
> > specific. I many cases I would expect pwm_disable() to allow a bit of
> > extra power to be saved because clocks could be turned off.
> > 
> >>>>> You may have an inverter somewhere, or you may actually be driving a
> >>>>> circuit that expects an inverted PWM.
> >>>>>
> >>>>> So saying that duty-cycle of 0 equals disable is simplifying things too
> >>>>> much, in my opinion.
> >>>>
> >>>> Full ack. So let's fix the users to not call pwm_disable after setting
> >>>> the duty cycle to zero!
> >>>
> >>> I don't understand why you come to that conclusion. It seems like we at
> >>> least agree on what the problem is and what potential issues there could
> >>> be with different setups.
> >>>
> >>> The point that we don't agree with is how to fix this.
> >>
> >> ack.
> >>
> >>>> Really, only the driver knows when it's safe to disable the clock while
> >>>> it's expected to keep the pin at a given state. So better don't expect a
> >>>> certain state after pwm_disable().
> >>>
> >>> Yes, I agree that only the driver knows when it is safe to do so, but I
> >>> still fail to understand why we should change the API in order to
> >>> accomodate the specifics of a particular driver.
> >>
> >> The incentive to change the API is a combination of:
> >>
> >>   - The restriction to keep the pin driving at a certain level after
> >>     disable isn't a natural and obvious property of the hardware.
> >>     The designers of the imx-pwm for example did it differently.
> > 
> > I would disagree. A pin that you can't keep at a defined level if it
> > isn't actively driven is a fundamentally useless concept. You said
> > yourself that the hardware design is problematic because it causes the
> > pin to change unexpectedly when you disable the PWM. One could argue
> > that that's a bug in the hardware. However, given that there is a way to
> > avoid the unexpected change, there is a mechanism outside of PWM to deal
> > with this use-case. And that's also perfectly fine. Dealing with pins
> > that aren't actively driven is part of the job of a pinmux controller.
> > 
> >>   - The change removes ambiguity from from the demands on the hardware
> >>     drivers.
> > 
> > I don't think there's currently any ambiguity. We may not be stating
> > explicitly that the assumption is for a PWM to be "off" after
> > pwm_disable(), but it's certainly how everyone has interpreted it. Also
> > if there's an ambiguity, then that's what should be addressed.
> > 
> >>   - The change doesn't make the API less expressive.
> > 
> > Yes it does. You remove the distinction between duty cycle == 0 and
> > the PWM being disabled.
> > 
> >>   - The change doesn't doesn't complicate anmy pwm hardware driver.
> > 
> > True. It also doesn't simplify any PWM hardware driver.
> > 
> >>   - The change allows to simplify most pwm users.
> > 
> > Yes, the subset that doesn't care about the difference is marginally
> > simplified. Although if the API was more rigorous about the definition
> > of pwm_disable(), then PWM users would be even more simplified.
> > 
> >>   - The change simplifies the imx-pwm driver.
> > 
> > True. But you leave out that the change doesn't actually fix your
> > problem. If your PWM user goes away, it will still want to disable the
> > PWM because it is no longer in use. However, you don't want that to
> > mean that the backlight goes to full brightness again. If you unbind
> > the backlight driver, the backlight should remain off (or turn off if
> > it isn't already).
> > 
> >> In my experience this is enough to justify such a change.
> > 
> > I would consider the change if it was actually going to fix the issue
> > that you're seeing. As it is, all your proposal does is paper over one
> > specific symptom, and I'm sorry, but that's just not good enough.
> > 
> >>> So we already know that currently all users expect that the pin state
> >>> after disable will be low (because they previously set duty cycle to 0,
> >>> which is the active equivalent of low).
> >>
> >> My theory is that most users don't expect this and/or don't make use of
> >> it. As of 9dcd936c5312 we have 50 pwm drivers
> >> ($(wc -l drivers/pwm/Makefile) - 3). At most 17 of them handle inversion
> >> at all. ($(git grep -l PWM_POLARITY_INVERSED drivers/pwm/pwm-* | wc -l),
> >> and I think there are a few false positive matches, pwm-sun4i.c for
> >> example). I'd be willing to bet that as of now imx isn't the only driver
> >> with this problem.
> > 
> > We can speculate about that as much as we want. i.MX is the only one
> > that this issue has been reported against, so I'm going to have to
> > assume that it's working fine for all of the others. If it isn't then
> > people should come forward and report it.
> > 
> >>> Given that stricter definition,
> >>> the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> >>> state. I think the fix for that is for the i.MX PWM driver to make sure
> >>> the pin state doesn't actually change on ->disable(). If that means the
> >>> clock needs to remain on, then that's exactly what the driver should be
> >>> implementing.
> >>
> >> Yes, if we keep the API "stricter" this is what should be done. This
> >> doesn't justify to keep the API strict though.
> > 
> > But an API that isn't strict is useless. If the behaviour of a PWM isn't
> > predictable, how can you use it to drive a backlight. If at any point it
> > could just go 100% and turn on the backlight to full brightness, how can
> > anyone use it for anything sensible?
> > 
> >>> Are you aware of this:
> >>>
> >>> 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> >>>
> >>> This seems to address exactly the problem that you're describing. The
> >>> solution matches what I had expected a fix for this problem to look
> >>> like. Can you try it and see if that fixes the issue?
> >>
> >> Right. Apart from bugs in it (for example
> >> http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> >> as output)
> 
> It is a RFC and the past discussion about it was mostly theoretical than
> about technical details. So I thought that nobody would really expect
> excellent technical solution at this point. I just tried to do my best
> and tested dozens of variations of [enabled/disabled/inverted/non-
> inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
> affects current users and how it improves the situation.

I replied to the mail and pointed out a few other things now. This is
easier than to talk about your patch in this discussion.
 
> Regarding the GPIO output configuration. Yes, the pin is configured as
> input in the imx_pwm_init_pinctrl_info() function. At this stage you
> do not really know how to configure the pin. Configuring it as input
> is the safest option I think.

Did it work without setting the gpio as output in your tests?

> I thought that the pin is configured as output whenever gpiod_set_value()
> is called. None of the other examples/usages in kernel I looked at used
> the gpiod_direction_output()

Maybe they use devm_gpio_request_one or similar that return your gpio's
direction already configured?

> >> Sidenote: With the current capabilities of the pwm framework there is no
> >> technical reason to expose to the hardware drivers that the pwm user uses
> >> inverted logic. The framework could just apply
> >>
> >> 	duty = period - duty;
> >>
> 
> I prefer to utilize as much HW capabilities as possible. And it seems
> like most PWM chips support HW output inversion (to some extent) so to
> me it seems perfectly OK that that information can be propagated from
> the upper SW levels to the lowest one.

If the hardware capability is useful I fully agree. But if inversion can
be accomplished by a chip that doesn't support inversion in hardware by
just using duty = period - duty (and so can be accomplished by a chip
that has inversion support without making use of it) then the drivers
shouldn't be confronted with it.

(I'm not sure if inversion is really relevant with the current status
quo, as the expectations are not documented in a place I'm aware of.)

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 16292 bytes --]

On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
[...]
> > We can speculate about that as much as we want. i.MX is the only one
> > that this issue has been reported against, so I'm going to have to
> > assume that it's working fine for all of the others. If it isn't then
> > people should come forward and report it.
> 
> You really assume that all the drivers that don't make use of
> PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
> you expect when the PWM user does:
> 
> 	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })
> 
> ? I'd say your assumption that all are working fine is treading on thin
> ice. The effect maybe is only that the i.MX users use more corners of
> the PWM API and/or are more open to report actual problems.

Look, it's unrealistic to expect me to actually test or verify all of
the different PWM controllers out there. The only thing I can go by is
by what the maintainers of the individual drivers tell me, or what has
been reported by users. So I'm going to assume that a driver works until
somebody reports that it is broken.

So again, if somebody notices that any of the other drivers are broken,
please do report it so that it can be fixed. Otherwise I don't have a
choice but assume that they still work.

> Also if there are two ways to implement something, one works for 49
> different hardware implementations and one for 50 this is already a good
> reason to stick to the latter.

This discussion isn't going anywhere, we're going in circles. I was not
suggesting that we don't find a solution for i.MX, I'm just saying that
changing the API is papering over some more fundamental issue on i.MX.
You can't magically fix it by changing the API, you have to get pinctrl
involved to properly solve this problem.

> > > > Given that stricter definition,
> > > > the i.MX PWM driver becomes buggy because it doesn't guarantee that pin
> > > > state. I think the fix for that is for the i.MX PWM driver to make sure
> > > > the pin state doesn't actually change on ->disable(). If that means the
> > > > clock needs to remain on, then that's exactly what the driver should be
> > > > implementing.
> > > 
> > > Yes, if we keep the API "stricter" this is what should be done. This
> > > doesn't justify to keep the API strict though.
> > 
> > But an API that isn't strict is useless. If the behaviour of a PWM isn't
> > predictable, how can you use it to drive a backlight. If at any point it
> > could just go 100% and turn on the backlight to full brightness, how can
> > anyone use it for anything sensible?
> 
> As long as I don't call pwm_disable() (or pwm_config(pwm, period,
> period)) it must not go to 100%.

I've been arguing that even if you call pwm_disable() it shouldn't go to
100%. So to summarize the problem for i.MX here: at reset time, the pin
driving the PWM is configured as GPIO and is configured as input with a
high impedance, so that it will be high on boot, prior to any software
running. This is reasonable in order to make sure that the backlight
stays off until it is properly initialized. What I've been trying to
convey is that when you do pwm_disable() the pin should go into exactly
that initial configuration to make sure the backlight doesn't behave in
an undefined way.

> > > > Are you aware of this:
> > > > 
> > > > 	http://patchwork.ozlabs.org/project/linux-pwm/list/?series=69987
> > > > 
> > > > This seems to address exactly the problem that you're describing. The
> > > > solution matches what I had expected a fix for this problem to look
> > > > like. Can you try it and see if that fixes the issue?
> > > 
> > > Right. Apart from bugs in it (for example
> > > http://patchwork.ozlabs.org/patch/981774/ does never configure the gpio
> > > as output) this would solve the issue for sure. This doesn't void my
> > > arguments to relax the PWM API though. Also look at the costs: The patch
> > > adds 86 lines to the imx driver complicating it considerably.
> > 
> > Now you're just being dramatic. 86 lines of code is not a lot. And I
> > don't think it's complicated. I'm willing to carry the burden of those
> > 86 lines of code if it fixes the issue for good.
> 
> Right, the complexity doesn't come with the 86 lines of code. With my
> approach the pwm-imx driver only has to care about its 6 registers. With
> Michal's patch it suddenly has to care about gpio and pinmuxing.

Why is that a bad thing? They are fundamentally entangled here, so why
do you think that's inappropriate?

> > >                                                               Each
> > > machine that wants to profit of this change has to adapt it's device
> > > tree to make the additional pinmuxing and gpio known.
> > > To be fair, my change doesn't fix the gap between pinctrl being active
> > > and the actual enablement of the pwm. But this could be better solved in
> > > the pwm framework without touching any hardware driver with pinctrl
> > > mechanism that are already in place. (Make use of the "init" pinctrl
> > > state and only switch to "default" when the pwm is configured.)
> > > There is nothing imx-pwm specific in hooking a gpio and adapting the
> > > pinmuxing in it. So if you want to go this path, please implement it in
> > > a generic way such that all pwm devices could benefit.
> > 
> > You're wrong. Judging by all the evidence that we have at this point,
> > this is exactly an imx-pwm specific issue, therefore the burden is on
> > the imx-pwm driver to implement a solution for it.
> 
> You're the first maintainer I met with this attitude. Whenever I
> implemented something in a driver specific way fixing an issue that
> other drivers might have to, I was requested to find a generic solution
> without the need to analyse if other drivers are affected or not.

Well, you should be happy. Dealing with this at a driver level is really
much easier than trying to come up with a generic solution. And maybe
you have met maintainers with a different attitude, so what? People have
different opinions and experiences. This shouldn't be a surprise.

In my opinion it's better to not try and generalize upfront. If you try
to create a generic solution if all you have is a single use-case, you
are bound to get it wrong. You're "generic" solution is likely to tailor
to the particular use-case and make it unusable for any others. So I try
not to attempt genericity until there are at least two or more use-cases
that can be the basis for a generic solution.

> There are some subsystems (e.g. tty/serial) where there is much stuff
> implemented in each driver even though some things could well be
> implemented in a generic way (e.g. rs485 handling). This is really a big
> mess.

One person's mess is merely another person's filing system.

> I reread your explanation several times and fail to understand it. It
> would be very helpful if you could come up with a scenario where it's
> obvious that the distinction is needed. You're writing about "typical
> use-cases" and "value in having the additional flexibility" without
> providing a use-case that is atypical or actually need the flexibility.
> 
> Maybe I don't understand the semantic you're giving to the calls or
> we're using the same words to describe different things. I try
> to describe the semantic of pwm_disable of the strict API in my words,
> can you please confirm or correct that?:
> 
> 	pwm_disable stops toggling the PWM pin at whatever state the pin
> 	currently is. Given a duty cycle between 0% and 100% (exclusive)
> 	that means the pin stops at an undefined level. If the duty
> 	cycle is 0% or 100% it stops (for an uninverted PWM) at 0 or 1
> 	respectively (and vice versa for an inverted one).

No, I don't think that's entirely correct. The current assumption by all
users is more that the pin would stop at the no power state, which would
be 0 for normal PWM and 1 for inverted.

This is what's reasonable for most or all cases. It's pretty much the
same as for a clock, or a regulator or any other number of devices.

> The semantic of pwm_enable in my understanding is:
> 
> 	pwm_enable for an uninverted PWM starts with $duty_cycle
> 	nanoseconds at 0 and then is 1 for $period - $duty_cycle
> 	nanoseconds. Then repeat.
> 	For an inverted it is first 1 for $duty_cycle nanoseconds and
> 	then 0 for the rest of the period.
> 
> I think pwm_config is to be defined like this:
> 
> 	pwm_config changes period and duty cycle without any
> 	specification about how the transition should be done.
> 
> All calls should be synchronous, that is should only return when the
> change is actually "on the pin".

Correct.

> > > > >  - most API users can be simplified because they can drop
> > > > >      if (duty == 0) pwm_disable()
> > > > >  - on i.MX the clock can be disabled when not needed saving some energy;
> > > > 
> > > > That's one reason why we have pwm_disable(). It's what you use to save
> > > > some energy if you don't need the PWM. Also, you claim that if the clock
> > > > is disabled the attached backlight will turn on at full brightness, so I
> > > > fail to see how this would work.
> > > 
> > > As long as you care about the backlight to be off, don't disable the
> > > pwm. And if in this state the clk can be disabled, then this should be
> > > done without an additional poke by the hardware driver which is the only
> > > place that can know for sure that it is safe to do so. There is no good
> > > reason to let the pwm user have to know that the currently configured
> > > state might make some energy saving possible and impose the burden on
> > > him to then call pwm_disable().
> > 
> > This doesn't have anything to do with consumers knowing about potential
> > energy savings. All we do is give the consumers an opportunity to say: I
> > need the PWM to be enabled because I want to use the PWM signal now or I
> > don't really need the PWM signal any more, just stop sending a signal.
> 
> There are two different degrees of "I don't need the PWM signal any
> more". In the first case you still care about the output level and in
> the other you don't. Currently the API doesn't give the PWM user the
> possibility to specify which of the two are intended.

I've said before that "don't care about the output level" is completely
useless. In most cases what you really want is the "no power" state and
that's what pwm_disable() should ensure. You're PWM could be controlling
a fan, and when you disable the PWM you want the fan to stop spinning,
right? You wouldn't want the fan to keep spinning after you've disabled
it.

This is something that most hardware engineers will also understand.
They tend to design their systems in such a way that the reset defaults
will have sensible effects. I think it's good in general to attempt, if
possible, to restore those reset defaults when you disable the device
that you're driving.

> > > happy when I fix the problem during active operation because on the
> > > machine I currently care about the shutdown problem isn't relevant.
> > 
> > Why is it not relevant? Surely at some point you will want to shut down
> > that machine.
> 
> Does a shutdown do anything with a running pwm software wise? The
> leds-pwm driver doesn't have a shutdown hook. The pwm_bl driver calls
> pwm_free in some legacy configuration but otherwise does nothing that
> would undo it's pwm_backlight_power_off() in the shutdown hook. So I'm
> convinced shutdown is fine here.

But pwm_backlight_power_off() will also end up calling pwm_disable(),
which by your description of the problem would fully light up the
backlight. That is, unless your driver implement ->apply(), but in that
case you wouldn't have a problem anyway.

> > And even if not, imx-pwm is used by others and they do
> > seem to care about shutdown, so they should have a say in the matter as
> > well.
> 
> Note that my suggestion doesn't make things worse for those users who
> care. I'm used to that it's good enough if a patch improves the
> situation for some users and doesn't make it worse for the others.
> 
> > > (Also as noted above, even Michal's patch doesn't solve the problem
> > > formally.)
> > 
> > Now I'm confused, you said above that "Apart from bugs in it ... this
> > would solve the issue for sure". So which one is it?
> 
> I'm sure that you must not assume that after
> 
> 	gpio_direction_output(gpio, 0);
> 	gpio_free(gpio);
> 
> the line still drives the 0. In most cases this will be the case, but if
> this makes the GPIO switch to being an input that should be correct
> behaviour, too. That's why I wrote "formally" above.
> 
> Michal's patch solves the gap at startup and when the PWM user calls
> disable() formally. At unbind when the gpio is freed it's "only" in
> practise.

That's why pinctrl is also involved. That should ensure that the correct
level is driven to the pin even if it isn't actively driven by either
GPIO or PWM.

> I listed several reasons to change the API, for some you confirmed them
> to be valid. You listed some for keeping the API as is. As I'm
> convinced my suggestion is good and your reasons are smaller (or even
> bad), I try to argue against them.
> 
> The above question was my try to summarize your reasons and to make sure
> the list is complete. I didn't try to turn anything around and if that
> was your impression that's a misunderstanding.
> 
> With my current understanding the list of reasons to stick to the status
> quo is:
> 
>  - it has been like that for ages
>    (which is a poor reason to oppose to change);
>  - it works for everyone but i.MX
>    (which I doubt and with my replacement it works for all drivers and
>    users as now including i.MX and maybe others that have the same issue
>    without us knowing it currently);
>  - it could be made working for i.MX by involving pinctrl and gpio in
>    the hardware specific pwm driver (which is ridiculous because my
>    solution is simpler); and
>  - we're changing API
>    (which I'd say is ok given that the specification isn't very specific
>    and breaking the API for good reasons is not nice but acceptable IMHO)
> 
> In my eyes in the above list there is one valid reason to not change
> (the last one). If you don't agree to change the userspace API we could
> keep that constant and only change the kernel-internal API (which is a
> bit ugly though).
> 
> The list of reasons in favour of a change are:
> 
>  - the pwm-imx driver can stay/become easier as with the stricter API;
>  - ambiguity removed
>    (hw drivers know in .disable if they can put the pin at a previously
>    "forbidden" value)
>  - users can be simplified by dropping some pwm_disable()
> 
> So now how can we come to a conclusion? Here are my suggestions for a
> compromise:
> 
> What about adding a new callback that we could for example call
> "disable_lax" that implements disabling the PWM without the guarantee to
> put the pin in the "off" state?
> 
> We could also introduce a callback "freeze" (or disable_strict) that is
> supposed to implement the "strict" disable. Then there is an objective
> way to determine when all drivers are adapted to the updated model.
> 
> Do you agree to my suggestion that pwm_config(pwm, 0, period) or similar
> should already imply disabling clocks if possible without the need to
> explicitly call pwm_disable() afterwards?

I don't think it has to be that explicit. Drivers should certainly feel
free to do so if it works and is reasonable.

To be honest, the whole discussion here is somewhat unproductive. You're
arguing about changing an API that's deprecated anyway, even if not
officially. We already introduced the atomic API a few years ago that is
meant to address most of these issues.

So instead of trying to "fix" a legacy API, have you considered moving
the i.MX driver to the atomic API? That should give you much better
control over when and how to program the registers.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
[...]
> > The semantic of pwm_enable in my understanding is:
> > 
> > 	pwm_enable for an uninverted PWM starts with $duty_cycle
> > 	nanoseconds at 0 and then is 1 for $period - $duty_cycle
> > 	nanoseconds. Then repeat.
> > 	For an inverted it is first 1 for $duty_cycle nanoseconds and
> > 	then 0 for the rest of the period.

Slight correction here: these are actually reversed. If you look at the
kerneldoc for enum pwm_polarity it explains what the expectations are.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1892 bytes --]

On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
[...]
> > >> Sidenote: With the current capabilities of the pwm framework there is no
> > >> technical reason to expose to the hardware drivers that the pwm user uses
> > >> inverted logic. The framework could just apply
> > >>
> > >> 	duty = period - duty;
> > >>
> > 
> > I prefer to utilize as much HW capabilities as possible. And it seems
> > like most PWM chips support HW output inversion (to some extent) so to
> > me it seems perfectly OK that that information can be propagated from
> > the upper SW levels to the lowest one.
> 
> If the hardware capability is useful I fully agree. But if inversion can
> be accomplished by a chip that doesn't support inversion in hardware by
> just using duty = period - duty (and so can be accomplished by a chip
> that has inversion support without making use of it) then the drivers
> shouldn't be confronted with it.

These two are orthogonal problems. If all you care about is the power
output of the PWM (as is the case for backlights, LEDs or fans, for
example), then inverted polarity has the same effect as duty = period -
duty.

However, the two don't result in identical waveforms. Again, a backlight
or LED doesn't care about the exact waveform, but there are other uses
for PWMs where these actually are important. I have admittedly never
used any of them myself, but this was brought to my attention a long
time ago when polarity support was introduced. I'm sure you can find the
discussions in some archives if you are inclined to do so.

> (I'm not sure if inversion is really relevant with the current status
> quo, as the expectations are not documented in a place I'm aware of.)

See the kerneldoc for enum pwm_polarity which does document this.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Vokáč Michal]

On 12.10.2018 11:45, Uwe Kleine-König wrote:
> On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
>> Uwe, Thierry,

[...]

>> It is a RFC and the past discussion about it was mostly theoretical than
>> about technical details. So I thought that nobody would really expect
>> excellent technical solution at this point. I just tried to do my best
>> and tested dozens of variations of [enabled/disabled/inverted/non-
>> inverted/ bootloader/device-tree/sysfs] configurations to be sure how it
>> affects current users and how it improves the situation.
> 
> I replied to the mail and pointed out a few other things now. This is
> easier than to talk about your patch in this discussion.

Ack.

>> Regarding the GPIO output configuration. Yes, the pin is configured as
>> input in the imx_pwm_init_pinctrl_info() function. At this stage you
>> do not really know how to configure the pin. Configuring it as input
>> is the safest option I think.
> 
> Did it work without setting the gpio as output in your tests?

Yes, I am using it like that for aprox. 2 months without issues.

>> I thought that the pin is configured as output whenever gpiod_set_value()
>> is called. None of the other examples/usages in kernel I looked at used
>> the gpiod_direction_output()
> 
> Maybe they use devm_gpio_request_one or similar that return your gpio's
> direction already configured?

Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib
functions.

Michal

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Michal,

On Fri, Oct 12, 2018 at 12:25:01PM +0000, Vokáč Michal wrote:
> >> Regarding the GPIO output configuration. Yes, the pin is configured as
> >> input in the imx_pwm_init_pinctrl_info() function. At this stage you
> >> do not really know how to configure the pin. Configuring it as input
> >> is the safest option I think.
> > 
> > Did it work without setting the gpio as output in your tests?
> 
> Yes, I am using it like that for aprox. 2 months without issues.

and you're sure the gpio is configured as output? Strange.
 
> >> I thought that the pin is configured as output whenever gpiod_set_value()
> >> is called. None of the other examples/usages in kernel I looked at used
> >> the gpiod_direction_output()
> > 
> > Maybe they use devm_gpio_request_one or similar that return your gpio's
> > direction already configured?
> 
> Nope. Examples I looked at (mainly I2C recovery) use only the gpiolib
> functions.

For example drivers/i2c/busses/i2c-imx.c uses devm_gpiod_get(&pdev->dev,
"scl", GPIOD_OUT_HIGH_OPEN_DRAIN); which configures as output (somewhat).

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Fri, Oct 12, 2018 at 11:53:49AM +0200, Thierry Reding wrote:
> On Thu, Oct 11, 2018 at 10:34:50PM +0200, Uwe Kleine-König wrote:
> [...]
> > > We can speculate about that as much as we want. i.MX is the only one
> > > that this issue has been reported against, so I'm going to have to
> > > assume that it's working fine for all of the others. If it isn't then
> > > people should come forward and report it.
> > 
> > You really assume that all the drivers that don't make use of
> > PWM_POLARITY_INVERSED (33 of 50 + imx at least, probably more) behave as
> > you expect when the PWM user does:
> > 
> > 	pwm_apply_state(pwm, { .period = 100; .duty_cycle=0; polarity=PWM_POLARITY_INVERSED; enabled=false; })
> > 
> > ? I'd say your assumption that all are working fine is treading on thin
> > ice. The effect maybe is only that the i.MX users use more corners of
> > the PWM API and/or are more open to report actual problems.
> 
> Look, it's unrealistic to expect me to actually test or verify all of
> the different PWM controllers out there. The only thing I can go by is
> by what the maintainers of the individual drivers tell me, or what has
> been reported by users. So I'm going to assume that a driver works until
> somebody reports that it is broken.

I didn't expect from you to test everything. I just pointed out that
your argument "I assume all drivers to behave correctly because there is
no report that any of them doesn't work" doesn't build on solid facts.

But lets make this explicit and official: I looked into all 48 pwm
drivers (at commit 6b3944e42e2e) and sorted them into categories:

a) not ported to pwm_state and no support .set_polarity:
   - pwm-ab8500.c
   - pwm-brcmstb.c
   - pwm-clps711x.c
   - pwm-crc.c
   - pwm-img.c
   - pwm-imx.c (1 of 2 pwm_ops in this driver)
   - pwm-lp3943.c
   - pwm-lpc32xx.c
   - pwm-mediatek.c
   - pwm-mtk-disp.c
   - pwm-mxs.c
   - pwm-pca9685.c
   - pwm-puv3.c
   - pwm-pxa.c
   - pwm-rcar.c
   - pwm-spear.c
   - pwm-sti.c
   - pwm-stmpe.c
   - pwm-twl-led.c
   - pwm-twl.c
   - pwm-tegra.c
  
b) supports inversion in hardware and pwm_state, polarity handling looks fine:
   - pwm-bcm-iproc.c
   - pwm-hibvt.c
   - pwm-rockchip.c
   - pwm-sun4i.c
   - pwm-zx.c

c) not ported to pwm_state, supports .set_polarity in hardware:
   - pwm-atmel-tcb.c
   - pwm-bcm2835.c
   - pwm-bcm-kona.c
   - pwm-berlin.c
   - pwm-ep93xx.c
   - pwm-fsl-ftm.c
   - pwm-lpc18xx-sct.c
   - pwm-jz4740.c
   - pwm-omap-dmtimer.c (not sure here, didn't find the actual implementation)
   - pwm-renesas-tpu.c
   - pwm-samsung.c
   - pwm-vt8500.c
   - pwm-tiehrpwm.c
   - pwm-tiecap.c

d) implements pwm_state, but ignores polarity:
   - pwm-cros-ec.c
   - pwm-lpss.c (with pwm-lpss-pci.c, pwm-lpss-platform.c)

e) supports state, but doesn't check .polarity on disable
   - pwm-atmel.c
   - pwm-atmel-hlcdc.c
   - pwm-imx.c (1 of 2 pwm_ops in this driver)
   - pwm-meson.c
   - pwm-stm32.c
   - pwm-stm32-lp.c

f) not actually a pwm driver (huh?):
   - pwm-tipwmss.c

So it's not as worse as I expected, mainly because some drivers don't
mention PWM_POLARITY_INVERSED but test for PWM_POLARITY_NORMAL.

The drivers in categories d) and e) are definitively broken and need
fixing. Some in the categories b) and c) might have the same problem as
imx (i.e. the output doesn't behave in pwm_disable(pwm) as in
pwm_config(pwm, 0, period)), but I'm unable to research that without the
respective hardware on my desk. (Reading the HW manual probably isn't
enough because most of them won't be explicit what happens in "off"
state. At least that's the case for the imx manual.)

> So again, if somebody notices that any of the other drivers are broken,
> please do report it so that it can be fixed. Otherwise I don't have a
> choice but assume that they still work.

done. :-)

> [...]

I deleted a big part of your mail and the repeating arguments I already
wrote in reply to it. I hope to work towards a compromise this way and
taking some heat out of the discussion.

> To be honest, the whole discussion here is somewhat unproductive.

I think the problem in this discussion is that there is a
misunderstanding between us. In my eyes the current state is:

I want to implement a change to the PWM API because I see a benefit in
it. I listed several advantages, some of them you accepted. You claimed
there was a disadvantage that makes the API less expressive. I tried to
understand that, asked several times for an example where the difference
you see actually matters but I don't get a reply to this question.

In this state I cannot invalidate your claim and I stay dissatisfied
because my incentive for a change is blocked by an argument I think is
void.

So I'll try to only discuss in the remaining part of this mail the
single issue that I think is key to our discussion:

You state there was a difference between

	pwm_disable(pwm);

and

	pwm_config(pwm, 0, period);

In my eyes they are equivalent. Both are expected to set the output to
inactive (that is 0 for an uninverted PWM, 1 for an inverted one).

The only differences I see are:

 - hardware driver details
   Some driver might not disable some resources after pwm_config and so
   failing to save some power. You stated however this is not the
   relevant issue.
 - after pwm_config(pwm, 0, period) pwm_enable(pwm) doesn't restore the old
   duty cycle.
   So with my suggested change the PWM framework stops caching the duty
   cycle. This appears too tiny for a reason to me to actually matter.

I assume I failed to list the detail you consider relevant. Can you
please make an effort to describe a use-case where the difference you
care about actually matters?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello Thierry,

On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> [...]
> > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > >> inverted logic. The framework could just apply
> > > >>
> > > >> 	duty = period - duty;
> > > >>
> > > 
> > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > like most PWM chips support HW output inversion (to some extent) so to
> > > me it seems perfectly OK that that information can be propagated from
> > > the upper SW levels to the lowest one.
> > 
> > If the hardware capability is useful I fully agree. But if inversion can
> > be accomplished by a chip that doesn't support inversion in hardware by
> > just using duty = period - duty (and so can be accomplished by a chip
> > that has inversion support without making use of it) then the drivers
> > shouldn't be confronted with it.
> 
> These two are orthogonal problems. If all you care about is the power
> output of the PWM (as is the case for backlights, LEDs or fans, for
> example), then inverted polarity has the same effect as duty = period -
> duty.
> 
> However, the two don't result in identical waveforms.

OK, let's find the difference then. Consider a pwm that is off at start,
then it's configured to run at 66% duty cycle at time A and then it's
disabled again at time B.

The resulting wave form is:

______________       __       __       __       _____________________
              \_____/  \_____/  \_____/  \_____/
            A                                B
              ^        ^        ^        ^

With the ^ markers pointing out where the periods started.

Correct?

Now with the duty = period - duty trick the wave would look as follows:


_________________       __       __       __       __________________
                 \_____/  \_____/  \_____/  \_____/
            A                                B
              ^        ^        ^        ^

Apart from a shift I cannot see any difference. I confirm that this
might be bad if there are two (or more) PWMs that are expected to run in
sync, but given that the current API doesn't provide the possibility to
map such a use case this is irrelevant.

Another difference I confirm there might be is that this might result in
one more (or less) periods depending on timing and the capabilities of
the hardware. Is this bad? I'd say it isn't.

What am I missing?

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello,

adding the author of the commit that introduced the .set_polarity
callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
some value to this discussion.

On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > [...]
> > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > >> inverted logic. The framework could just apply
> > > > >>
> > > > >> 	duty = period - duty;
> > > > >>
> > > > 
> > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > me it seems perfectly OK that that information can be propagated from
> > > > the upper SW levels to the lowest one.
> > > 
> > > If the hardware capability is useful I fully agree. But if inversion can
> > > be accomplished by a chip that doesn't support inversion in hardware by
> > > just using duty = period - duty (and so can be accomplished by a chip
> > > that has inversion support without making use of it) then the drivers
> > > shouldn't be confronted with it.
> > 
> > These two are orthogonal problems. If all you care about is the power
> > output of the PWM (as is the case for backlights, LEDs or fans, for
> > example), then inverted polarity has the same effect as duty = period -
> > duty.
> > 
> > However, the two don't result in identical waveforms.
> 
> OK, let's find the difference then. Consider a pwm that is off at start,
> then it's configured to run at 66% duty cycle at time A and then it's
> disabled again at time B.
> 
> The resulting wave form is:
> 
> ______________       __       __       __       _____________________
>               \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> With the ^ markers pointing out where the periods started.
> 
> Correct?
> 
> Now with the duty = period - duty trick the wave would look as follows:
> 
> 
> _________________       __       __       __       __________________
>                  \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> Apart from a shift I cannot see any difference. I confirm that this
> might be bad if there are two (or more) PWMs that are expected to run in
> sync, but given that the current API doesn't provide the possibility to
> map such a use case this is irrelevant.
> 
> Another difference I confirm there might be is that this might result in
> one more (or less) periods depending on timing and the capabilities of
> the hardware. Is this bad? I'd say it isn't.
> 
> What am I missing?

The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions:

    A practical example where [changing polarity] can prove useful is to
    simulate inversion of the duty cycle. While inversion of polarity
    and duty cycle are not exactly the same, the differences for most
    use-cases are negligible.

This doesn't help me to understand why the polarity stuff is useful
though. Does this log say that the motivation to support changing
polarity was to simulate "inversion of the duty cycle"? What exactly is
"inversion of the duty cycle"? Just

	duty_cycle = period - duty_cycle;

? Why does this have to be simulated, the API was able to do this
without any simulation before?

Is there a single use-case that can be done with polarity support in the
API that isn't possible without it? I cannot imagine there is any but
would like to learn and understand why this is valuable.

Currently there is no user of pwm_set_polarity(), I suggest to remove it
to not give users an incentive to still rely on the non-atomic API.

Also among the users of pwm_apply_state there is none which makes use of
the polarity setting.

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

Hello,

On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote:
> adding the author of the commit that introduced the .set_polarity
> callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
> some value to this discussion.

there is no insight to be expected from that, the email address doesn't
exist any more.

Best regards
Uwe

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

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5449 bytes --]

On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > [...]
> > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > >> inverted logic. The framework could just apply
> > > > >>
> > > > >> 	duty = period - duty;
> > > > >>
> > > > 
> > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > me it seems perfectly OK that that information can be propagated from
> > > > the upper SW levels to the lowest one.
> > > 
> > > If the hardware capability is useful I fully agree. But if inversion can
> > > be accomplished by a chip that doesn't support inversion in hardware by
> > > just using duty = period - duty (and so can be accomplished by a chip
> > > that has inversion support without making use of it) then the drivers
> > > shouldn't be confronted with it.
> > 
> > These two are orthogonal problems. If all you care about is the power
> > output of the PWM (as is the case for backlights, LEDs or fans, for
> > example), then inverted polarity has the same effect as duty = period -
> > duty.
> > 
> > However, the two don't result in identical waveforms.
> 
> OK, let's find the difference then. Consider a pwm that is off at start,
> then it's configured to run at 66% duty cycle at time A and then it's
> disabled again at time B.
> 
> The resulting wave form is:
> 
> ______________       __       __       __       _____________________
>               \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> With the ^ markers pointing out where the periods started.
> 
> Correct?
> 
> Now with the duty = period - duty trick the wave would look as follows:
> 
> 
> _________________       __       __       __       __________________
>                  \_____/  \_____/  \_____/  \_____/
>             A                                B
>               ^        ^        ^        ^
> 
> Apart from a shift I cannot see any difference. I confirm that this
> might be bad if there are two (or more) PWMs that are expected to run in
> sync, but given that the current API doesn't provide the possibility to
> map such a use case this is irrelevant.
> 
> Another difference I confirm there might be is that this might result in
> one more (or less) periods depending on timing and the capabilities of
> the hardware. Is this bad? I'd say it isn't.
> 
> What am I missing?

I don't think that's entirely correct. For normal PWM with 66% duty
cycle you'd get something like this:

     ____    ____    ____
    /    \__/    \__/    \__   (1)
    ^       ^       ^

For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33%
duty cycle) you'd get this:

     __      __      __
    /  \____/  \____/  \____   (2)
    ^       ^       ^

For a truly inversed PWM (duty cycle = 33%), it would look like this:

          __      __      __
    \____/  \____/  \____/     (3)
    ^       ^       ^

If you emulate inversion of the truly inversed PWM, you'd get this:

        ____    ____    ____
    \__/    \__/    \__/       (4)
    ^       ^       ^

As you can see, all of the above are slightly different. As you can see,
and you already noted that, the emulated inversion (2) and the true
inversion (3) are almost identical, except that they have a phase shift
that is equivalent to the duty cycle. The same is true of (1) and (4).
The phase shift doesn't affect the power output of the signal, so (1)
and (4) as well as (2) and (3) have the same power output. This is the
reason why the difference doesn't matter for backlight or LEDs, because
brightness is proportional to the power output.

However, you can also see that (2) and (3) are different in how the
period starts. (2) starts with a rising edge while (3) starts with a
falling edge.

Now, for any use-cases where the only thing we care about is the power
output, my opinion is that consumers should implement the inversion. If
the hardware is capable of inversion, then by all means they should use
that in the implementation. But, a lot of hardware is not capable of
doing the inversion, so they can still fall back to emulating the
inversion. Either way, the important point here is that the consumer
driver (pwm-backlight, leds-pwm, ...) knows that only power output is
relevant, so it is the consumer drivers that should make the decision.

On the other hand, other use-cases may rely on the exact shape of the
PWM signal, so if the edges are important, then they need to rely on the
hardware inversion for proper functionality. My understanding is that we
don't have any of those users in the kernel, but my recollection is that
people use the sysfs interfaces to make use of this. Like I said
elsewhere, if I remember correctly, people use this as a way of
synchronizing motors using PWM pulses. My knowledge of those use-cases
stops there because I've never used PWMs like that.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5118 bytes --]

On Mon, Oct 15, 2018 at 10:14:21AM +0200, Uwe Kleine-König wrote:
> Hello,
> 
> adding the author of the commit that introduced the .set_polarity
> callback (0aa0869c3c9b10338dd92a20fa4a9b6959f177b5), maybe he can add
> some value to this discussion.
> 
> On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > > [...]
> > > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > > >> inverted logic. The framework could just apply
> > > > > >>
> > > > > >> 	duty = period - duty;
> > > > > >>
> > > > > 
> > > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > > me it seems perfectly OK that that information can be propagated from
> > > > > the upper SW levels to the lowest one.
> > > > 
> > > > If the hardware capability is useful I fully agree. But if inversion can
> > > > be accomplished by a chip that doesn't support inversion in hardware by
> > > > just using duty = period - duty (and so can be accomplished by a chip
> > > > that has inversion support without making use of it) then the drivers
> > > > shouldn't be confronted with it.
> > > 
> > > These two are orthogonal problems. If all you care about is the power
> > > output of the PWM (as is the case for backlights, LEDs or fans, for
> > > example), then inverted polarity has the same effect as duty = period -
> > > duty.
> > > 
> > > However, the two don't result in identical waveforms.
> > 
> > OK, let's find the difference then. Consider a pwm that is off at start,
> > then it's configured to run at 66% duty cycle at time A and then it's
> > disabled again at time B.
> > 
> > The resulting wave form is:
> > 
> > ______________       __       __       __       _____________________
> >               \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > With the ^ markers pointing out where the periods started.
> > 
> > Correct?
> > 
> > Now with the duty = period - duty trick the wave would look as follows:
> > 
> > 
> > _________________       __       __       __       __________________
> >                  \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > Apart from a shift I cannot see any difference. I confirm that this
> > might be bad if there are two (or more) PWMs that are expected to run in
> > sync, but given that the current API doesn't provide the possibility to
> > map such a use case this is irrelevant.
> > 
> > Another difference I confirm there might be is that this might result in
> > one more (or less) periods depending on timing and the capabilities of
> > the hardware. Is this bad? I'd say it isn't.
> > 
> > What am I missing?
> 
> The commit log of 0aa0869c3c9b10338dd92a20fa4a9b6959f177b5 mentions:
> 
>     A practical example where [changing polarity] can prove useful is to
>     simulate inversion of the duty cycle. While inversion of polarity
>     and duty cycle are not exactly the same, the differences for most
>     use-cases are negligible.
> 
> This doesn't help me to understand why the polarity stuff is useful
> though. Does this log say that the motivation to support changing
> polarity was to simulate "inversion of the duty cycle"? What exactly is
> "inversion of the duty cycle"? Just
> 
> 	duty_cycle = period - duty_cycle;
> 
> ? Why does this have to be simulated, the API was able to do this
> without any simulation before?
> 
> Is there a single use-case that can be done with polarity support in the
> API that isn't possible without it? I cannot imagine there is any but
> would like to learn and understand why this is valuable.

See my earlier mail which will hopefully answer these questions.

> Currently there is no user of pwm_set_polarity(), I suggest to remove it
> to not give users an incentive to still rely on the non-atomic API.

Yeah, that's probably a good idea. The long-term plan was always to get
rid of the legacy API, but I haven't gotten around to that yet.

> Also among the users of pwm_apply_state there is none which makes use of
> the polarity setting.

Well, this is primarily because our current users don't care much about
the polarity setting, so they just go with whatever initial setting they
get (usually via pwm_get_args()). These correspond to whatever was
specified in the flags of the PWM specifier in DT or in the lookup table
for non-DT devices (see for example of_pwm_xlate_with_flags()).

There is one user that allows changing the polarity, and that's sysfs.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: RFC: don't let drivers issue pwm_disable after pwm_config(pwm, 0, period)

[Uwe Kleine-König]

On Mon, Oct 15, 2018 at 11:22:13AM +0200, Thierry Reding wrote:
> On Sun, Oct 14, 2018 at 01:34:00PM +0200, Uwe Kleine-König wrote:
> > Hello Thierry,
> > 
> > On Fri, Oct 12, 2018 at 12:11:43PM +0200, Thierry Reding wrote:
> > > On Fri, Oct 12, 2018 at 11:45:57AM +0200, Uwe Kleine-König wrote:
> > > > On Thu, Oct 11, 2018 at 01:15:44PM +0000, Vokáč Michal wrote:
> > > [...]
> > > > > >> Sidenote: With the current capabilities of the pwm framework there is no
> > > > > >> technical reason to expose to the hardware drivers that the pwm user uses
> > > > > >> inverted logic. The framework could just apply
> > > > > >>
> > > > > >> 	duty = period - duty;
> > > > > >>
> > > > > 
> > > > > I prefer to utilize as much HW capabilities as possible. And it seems
> > > > > like most PWM chips support HW output inversion (to some extent) so to
> > > > > me it seems perfectly OK that that information can be propagated from
> > > > > the upper SW levels to the lowest one.
> > > > 
> > > > If the hardware capability is useful I fully agree. But if inversion can
> > > > be accomplished by a chip that doesn't support inversion in hardware by
> > > > just using duty = period - duty (and so can be accomplished by a chip
> > > > that has inversion support without making use of it) then the drivers
> > > > shouldn't be confronted with it.
> > > 
> > > These two are orthogonal problems. If all you care about is the power
> > > output of the PWM (as is the case for backlights, LEDs or fans, for
> > > example), then inverted polarity has the same effect as duty = period -
> > > duty.
> > > 
> > > However, the two don't result in identical waveforms.
> > 
> > OK, let's find the difference then. Consider a pwm that is off at start,
> > then it's configured to run at 66% duty cycle at time A and then it's
> > disabled again at time B.
> > 
> > The resulting wave form is:
> > 
> > ______________       __       __       __       _____________________
> >               \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > With the ^ markers pointing out where the periods started.
> > 
> > Correct?
> > 
> > Now with the duty = period - duty trick the wave would look as follows:
> > 
> > 
> > _________________       __       __       __       __________________
> >                  \_____/  \_____/  \_____/  \_____/
> >             A                                B
> >               ^        ^        ^        ^
> > 
> > Apart from a shift I cannot see any difference. I confirm that this
> > might be bad if there are two (or more) PWMs that are expected to run in
> > sync, but given that the current API doesn't provide the possibility to
> > map such a use case this is irrelevant.
> > 
> > Another difference I confirm there might be is that this might result in
> > one more (or less) periods depending on timing and the capabilities of
> > the hardware. Is this bad? I'd say it isn't.
> > 
> > What am I missing?
> 
> I don't think that's entirely correct. For normal PWM with 66% duty
> cycle you'd get something like this:
> 
>      ____    ____    ____
>     /    \__/    \__/    \__   (1)
>     ^       ^       ^
> 
> For "emulated" inversion (duty cycle = period - duty cycle, i.e. 33%
> duty cycle) you'd get this:
> 
>      __      __      __
>     /  \____/  \____/  \____   (2)
>     ^       ^       ^
> 
> For a truly inversed PWM (duty cycle = 33%), it would look like this:
> 
>           __      __      __
>     \____/  \____/  \____/     (3)
>     ^       ^       ^
> 
> If you emulate inversion of the truly inversed PWM, you'd get this:
> 
>         ____    ____    ____
>     \__/    \__/    \__/       (4)
>     ^       ^       ^
> 
> As you can see, all of the above are slightly different. As you can see,
> and you already noted that, the emulated inversion (2) and the true
> inversion (3) are almost identical, except that they have a phase shift
> that is equivalent to the duty cycle. The same is true of (1) and (4).
> The phase shift doesn't affect the power output of the signal, so (1)
> and (4) as well as (2) and (3) have the same power output. This is the
> reason why the difference doesn't matter for backlight or LEDs, because
> brightness is proportional to the power output.
> 
> However, you can also see that (2) and (3) are different in how the
> period starts. (2) starts with a rising edge while (3) starts with a
> falling edge.
> 
> Now, for any use-cases where the only thing we care about is the power
> output, my opinion is that consumers should implement the inversion. If
> the hardware is capable of inversion, then by all means they should use
> that in the implementation. But, a lot of hardware is not capable of
> doing the inversion, so they can still fall back to emulating the
> inversion. Either way, the important point here is that the consumer
> driver (pwm-backlight, leds-pwm, ...) knows that only power output is
> relevant, so it is the consumer drivers that should make the decision.
> 
> On the other hand, other use-cases may rely on the exact shape of the
> PWM signal, so if the edges are important, then they need to rely on the
> hardware inversion for proper functionality. My understanding is that we
> don't have any of those users in the kernel, but my recollection is that
> people use the sysfs interfaces to make use of this.

Your figures suggest that the first edge is different, too, but that's
wrong because you didn't take the idle state into account that was
active before the first period. If the output was already high before
there cannot be a raising edge. When this is corrected in your figures
they look as follows:

1) uninverted 
       ____    ____    ____
   ___/    \__/    \__/    \__
      ^       ^       ^

2) emulated inversion

   ______      __      __
         \____/  \____/  \____
      ^       ^       ^

3) hw inversion:
    __      __      __
      \____/  \____/  \____/
      ^       ^       ^

4) hw + emulated inversion:

          ____    ____    ____
   ______/    \__/    \__/       (4)
      ^       ^       ^

So the order of the edges is identical for hardware inversion and
software inversion.

The only difference is that with hardware inversion the first edge might
come earlier. But with the current PWM API you cannot make use of this
"advantage" because there is no way for the user to sync the period of
the PWM to anything else (e.g. another PWM). Also typically for a motor
control you want two PWMs running with a phase shift like:

         ___     ___     ___
    ____/   \___/   \___/
        ^  ___  ^  ___  ^
    ______/   \___/   \___/
          ^       ^       ^

So independant of inversion or not the periods of the two signals must
be shifted by (say) $period/2. How do you achive that? I'd say the best
you can do is:

	local_irq_disable();
	pwm_enable(pwm1);
	delay(period / 2);
	pwm_enable(pwm2);
	local_irq_enable();

Now given that it can take some time until pwm_enable() returns, this is
poor timing and you'd have to reduce the delay (by experimenting). If
you want to do that from userspace via sysfs you cannot even disable
irqs and the timing depends on how many ethernet packages are processed
between your two syscalls.

Trying this without additional hardware capabilities (e.g. a counter
shared by two PWMs) is broken. As the PWM API doesn't expose such
capabilities changing how inversion is implemented won't break any user
that isn't already broken before.

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Boris Brezillon]

Hi Thierry,

On Tue, 9 Oct 2018 11:04:01 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello Thierry,
> 
> thanks for taking time to reply in this thread.
> 
> On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > Contrarily to the common assumption that pwm_disable() stops the
> > > output at the state where it just happens to be, this isn't what the
> > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > 0 level instead.
> > > 
> > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > The backend driver is free to implement potential power savings
> > > already when the duty cycle changes to one of these two cases so this
> > > doesn't come at an increased runtime power cost (once the respective
> > > drivers are fixed).
> > > 
> > > In return this allows to adhere to the API more easily for drivers that
> > > currently cannot know if it's ok to disable the hardware on
> > > pwm_disable() because the caller might or might not rely on the pin
> > > stopping at a certain level.
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > >  Documentation/pwm.txt | 8 ++++++--
> > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > 
> > I don't think this is correct. An API whose result is an undefined state
> > is useless in my opinion. So either we properly define what the state
> > should be after a disable operation, or we might as well just remove the
> > disable operation altogether.  
> 
> Explicitly not defining some details makes it easier for hardware
> drivers to comply with the API. Sure it would be great if all hardware
> would allow to implement a "stronger" API but this isn't how reality looks
> like.
> 
> You say it's bad that .disable() should imply less than it did up to
> now. I say .disable() should only imply that the PWM stops toggling, so
> .disable has a single purpose that each hardware design should be able
> to implement.
> And this weaker requirement/result is still good enough to implement all
> use cases. (You had doubts here in the past, but without an example. I
> cannot imagine there is one.) In my eyes this makes the API better not
> worse.
> 
> What we have now in the API is redundancy, which IMHO is worse. If I
> want the pwm pin to stay low I can say:
> 
> 	pwm_config(pwm, 0, 100);
> 
> or I can say:
> 
> 	pwm_config(pwm, 0, 100);
> 	pwm_disable(pwm);
> 
> The expected result is the same. Now you could argue that not disabling
> the pwm is a bug because it doesn't save some energy. But really this is
> a weakness of the API. There are two ways to express "Set the pin to
> constant 0" and if there is an opportunity to save some energy on a
> certain hardware implementation, just calling pwm_config() with duty=0
> should be enough to benefit from it. This makes the API easier to use
> because there is a single command to get to the state the pwm user wants
> the pwm to be in. (This is two advantages: A single way to reach the
> desired state and only one function to call.)

I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
duty=100%" logic should, IMHO, be a HW-specific optimization. And if you
look at existing drivers, it shouldn't be that hard to patch those that
support this optimization since they most of the time have a separate
disable function which could be called from the their config function
if needed.
On the other hand, if we do it the other way around, and expect
pwm_disable() to keep the pin in the state it was after setting a duty
of 0, it becomes more complicated (impossible?) for PWM blocks that do
not support specifying a default pin state when the PWM is disabled.

Regards,

Boris

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Uwe Kleine-König]

On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> On Tue, 9 Oct 2018 11:04:01 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Hello Thierry,
> > 
> > thanks for taking time to reply in this thread.
> > 
> > On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > > Contrarily to the common assumption that pwm_disable() stops the
> > > > output at the state where it just happens to be, this isn't what the
> > > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > > 0 level instead.
> > > > 
> > > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > > The backend driver is free to implement potential power savings
> > > > already when the duty cycle changes to one of these two cases so this
> > > > doesn't come at an increased runtime power cost (once the respective
> > > > drivers are fixed).
> > > > 
> > > > In return this allows to adhere to the API more easily for drivers that
> > > > currently cannot know if it's ok to disable the hardware on
> > > > pwm_disable() because the caller might or might not rely on the pin
> > > > stopping at a certain level.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  Documentation/pwm.txt | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > > 
> > > I don't think this is correct. An API whose result is an undefined state
> > > is useless in my opinion. So either we properly define what the state
> > > should be after a disable operation, or we might as well just remove the
> > > disable operation altogether.  
> > 
> > Explicitly not defining some details makes it easier for hardware
> > drivers to comply with the API. Sure it would be great if all hardware
> > would allow to implement a "stronger" API but this isn't how reality looks
> > like.
> > 
> > You say it's bad that .disable() should imply less than it did up to
> > now. I say .disable() should only imply that the PWM stops toggling, so
> > .disable has a single purpose that each hardware design should be able
> > to implement.
> > And this weaker requirement/result is still good enough to implement all
> > use cases. (You had doubts here in the past, but without an example. I
> > cannot imagine there is one.) In my eyes this makes the API better not
> > worse.
> > 
> > What we have now in the API is redundancy, which IMHO is worse. If I
> > want the pwm pin to stay low I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 
> > or I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 	pwm_disable(pwm);
> > 
> > The expected result is the same. Now you could argue that not disabling
> > the pwm is a bug because it doesn't save some energy. But really this is
> > a weakness of the API. There are two ways to express "Set the pin to
> > constant 0" and if there is an opportunity to save some energy on a
> > certain hardware implementation, just calling pwm_config() with duty=0
> > should be enough to benefit from it. This makes the API easier to use
> > because there is a single command to get to the state the pwm user wants
> > the pwm to be in. (This is two advantages: A single way to reach the
> > desired state and only one function to call.)
> 
> I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> duty=100%" logic should, IMHO, be a HW-specific optimization. And if you
> look at existing drivers, it shouldn't be that hard to patch those that
> support this optimization since they most of the time have a separate
> disable function which could be called from the their config function
> if needed.
> On the other hand, if we do it the other way around, and expect
> pwm_disable() to keep the pin in the state it was after setting a duty
> of 0, it becomes more complicated (impossible?) for PWM blocks that do
> not support specifying a default pin state when the PWM is disabled.

Of course you could argue (as Thierry does) that .disable should be a
noop then. Thinking that further .disable could be a noop for every hw
driver then and we could remove pwm_disable() completely (together with
.enabled in pwm_state).

Best regards
Uwe

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

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Uwe Kleine-König]

Hello Boris,

On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> On Tue, 9 Oct 2018 11:04:01 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Hello Thierry,
> > 
> > thanks for taking time to reply in this thread.
> > 
> > On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > > Contrarily to the common assumption that pwm_disable() stops the
> > > > output at the state where it just happens to be, this isn't what the
> > > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > > 0 level instead.
> > > > 
> > > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > > The backend driver is free to implement potential power savings
> > > > already when the duty cycle changes to one of these two cases so this
> > > > doesn't come at an increased runtime power cost (once the respective
> > > > drivers are fixed).
> > > > 
> > > > In return this allows to adhere to the API more easily for drivers that
> > > > currently cannot know if it's ok to disable the hardware on
> > > > pwm_disable() because the caller might or might not rely on the pin
> > > > stopping at a certain level.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  Documentation/pwm.txt | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > > 
> > > I don't think this is correct. An API whose result is an undefined state
> > > is useless in my opinion. So either we properly define what the state
> > > should be after a disable operation, or we might as well just remove the
> > > disable operation altogether.  
> > 
> > Explicitly not defining some details makes it easier for hardware
> > drivers to comply with the API. Sure it would be great if all hardware
> > would allow to implement a "stronger" API but this isn't how reality looks
> > like.
> > 
> > You say it's bad that .disable() should imply less than it did up to
> > now. I say .disable() should only imply that the PWM stops toggling, so
> > .disable has a single purpose that each hardware design should be able
> > to implement.
> > And this weaker requirement/result is still good enough to implement all
> > use cases. (You had doubts here in the past, but without an example. I
> > cannot imagine there is one.) In my eyes this makes the API better not
> > worse.
> > 
> > What we have now in the API is redundancy, which IMHO is worse. If I
> > want the pwm pin to stay low I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 
> > or I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 	pwm_disable(pwm);
> > 
> > The expected result is the same. Now you could argue that not disabling
> > the pwm is a bug because it doesn't save some energy. But really this is
> > a weakness of the API. There are two ways to express "Set the pin to
> > constant 0" and if there is an opportunity to save some energy on a
> > certain hardware implementation, just calling pwm_config() with duty=0
> > should be enough to benefit from it. This makes the API easier to use
> > because there is a single command to get to the state the pwm user wants
> > the pwm to be in. (This is two advantages: A single way to reach the
> > desired state and only one function to call.)
> 
> I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> duty=100%" logic should, IMHO, be a HW-specific optimization.

Note that according to Thierry the behaviour of

	pwm_config(pwm, 100, 100);
	pwm_disable(pwm);

(and similarily of

	pwm_apply_state({.period = 100, .duty_cycle = 100, .enabled = false});

) should (assuming an uninverted pwm) result in the output stopping at 0
(i.e. it's inactive level which is the same as after pwm_config(pwm, 0, 100)).

So there isn't anything special about "disable PWM on duty=0% or
duty=100%". "disable PWM" in Thierry's eyes is always "stop at inactive
level" independant of the previously configured duty cycle.

You talking about the specific cases 0% and 100% makes me believe that
you are an example (BTW, I'm another) that makes Thierry's claim

	The current assumption by all users is more that [after
	pwm_disable()] the pin would stop at the no power state, which
	would be 0 for normal PWM and 1 for inverted.

wrong. (See mail with Message-ID: 20181012095349.GA9162@ulmo in this
thread.)
 
To get forward here: Thierry, can you please confirm that there is no
semantical difference between "pwm_disable()" and "pwm_config(pwm, 0,
period)". (Or alternatively come up with a use case where a difference
really matters. I'm sure this is impossible though.)

Then I can start simplifying API users and adapt the pwm
hardware drivers. When this is done there will be no user left of
pwm_disable and struct pwm_state::enabled because nobody needs it with
the current semantic. After that we can either rip the concept of
pwm_disable or shift it's semantic to my initial suggestion of "no
guarantees about the output level" and use that in the few drivers that
really don't care about the output level[1] and so give the hardware the
possibility to disable the clock even if that results in something
different than "inactive".

As a first step I'd suggest to explicitly state the expectations on
hardware driver implementations and pwm-API users to get all affected
parties in line. These are in my eyes:

 - configuring the duty cycle should (if possible) not result in
   glitches, so the currently running period should be completed and
   after that a new period with the new configuration should start.
   (Is this reasonable to expect this from all implementations? If not
   we should maybe introduce a way to let the users know?)

 - The function to configure the duty cycle should only return when the
   period with the new configuration actually started.

If the concept of pwm_disable() stays, we should document the expected
behaviour of the pin. Something like:

 - After disabling the pwm you must not make any assumptions about the
   pin level. It might be low, or inactive, or even high-z. So don't
   disable the pwm if you for example care about a display backlight to
   stay off.

If desired we might even keep the behaviour for the sysfs implementation
that disable there for historic reasons keeps the semantic of
duty-cycle=0.

Further things that could be done (eventually after discussing they are
really sensible) are:

 - for pwms that can be programmed now for the next period implement the
   necessary sleep in the PWM-Framework (mxs, atmel, sun4i at least);
 - abstract inversed pwms in the framework instead of each hw driver;
 - let hw drivers correct *state on apply
   (if the user requests period=1ms and the driver can only provide
   multiples of 0.6667 ms because the input clock is fixed at 1500 Hz)

Best regards
Uwe

[1] the only example I know is the pwm-backlight driver that could
    disable the pwm without the inactive level guarantee when the
    enable-gpio is inactive.
-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 9384 bytes --]

On Thu, Oct 18, 2018 at 10:44:05AM +0200, Uwe Kleine-König wrote:
> Hello Boris,
> 
> On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> > On Tue, 9 Oct 2018 11:04:01 +0200
> > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> > 
> > > Hello Thierry,
> > > 
> > > thanks for taking time to reply in this thread.
> > > 
> > > On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > > > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > > > Contrarily to the common assumption that pwm_disable() stops the
> > > > > output at the state where it just happens to be, this isn't what the
> > > > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > > > 0 level instead.
> > > > > 
> > > > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > > > The backend driver is free to implement potential power savings
> > > > > already when the duty cycle changes to one of these two cases so this
> > > > > doesn't come at an increased runtime power cost (once the respective
> > > > > drivers are fixed).
> > > > > 
> > > > > In return this allows to adhere to the API more easily for drivers that
> > > > > currently cannot know if it's ok to disable the hardware on
> > > > > pwm_disable() because the caller might or might not rely on the pin
> > > > > stopping at a certain level.
> > > > > 
> > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > > ---
> > > > >  Documentation/pwm.txt | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > > > 
> > > > I don't think this is correct. An API whose result is an undefined state
> > > > is useless in my opinion. So either we properly define what the state
> > > > should be after a disable operation, or we might as well just remove the
> > > > disable operation altogether.  
> > > 
> > > Explicitly not defining some details makes it easier for hardware
> > > drivers to comply with the API. Sure it would be great if all hardware
> > > would allow to implement a "stronger" API but this isn't how reality looks
> > > like.
> > > 
> > > You say it's bad that .disable() should imply less than it did up to
> > > now. I say .disable() should only imply that the PWM stops toggling, so
> > > .disable has a single purpose that each hardware design should be able
> > > to implement.
> > > And this weaker requirement/result is still good enough to implement all
> > > use cases. (You had doubts here in the past, but without an example. I
> > > cannot imagine there is one.) In my eyes this makes the API better not
> > > worse.
> > > 
> > > What we have now in the API is redundancy, which IMHO is worse. If I
> > > want the pwm pin to stay low I can say:
> > > 
> > > 	pwm_config(pwm, 0, 100);
> > > 
> > > or I can say:
> > > 
> > > 	pwm_config(pwm, 0, 100);
> > > 	pwm_disable(pwm);
> > > 
> > > The expected result is the same. Now you could argue that not disabling
> > > the pwm is a bug because it doesn't save some energy. But really this is
> > > a weakness of the API. There are two ways to express "Set the pin to
> > > constant 0" and if there is an opportunity to save some energy on a
> > > certain hardware implementation, just calling pwm_config() with duty=0
> > > should be enough to benefit from it. This makes the API easier to use
> > > because there is a single command to get to the state the pwm user wants
> > > the pwm to be in. (This is two advantages: A single way to reach the
> > > desired state and only one function to call.)
> > 
> > I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> > duty=100%" logic should, IMHO, be a HW-specific optimization.
> 
> Note that according to Thierry the behaviour of
> 
> 	pwm_config(pwm, 100, 100);
> 	pwm_disable(pwm);
> 
> (and similarily of
> 
> 	pwm_apply_state({.period = 100, .duty_cycle = 100, .enabled = false});
> 
> ) should (assuming an uninverted pwm) result in the output stopping at 0
> (i.e. it's inactive level which is the same as after pwm_config(pwm, 0, 100)).
> 
> So there isn't anything special about "disable PWM on duty=0% or
> duty=100%". "disable PWM" in Thierry's eyes is always "stop at inactive
> level" independant of the previously configured duty cycle.
> 
> You talking about the specific cases 0% and 100% makes me believe that
> you are an example (BTW, I'm another) that makes Thierry's claim
> 
> 	The current assumption by all users is more that [after
> 	pwm_disable()] the pin would stop at the no power state, which
> 	would be 0 for normal PWM and 1 for inverted.
> 
> wrong. (See mail with Message-ID: 20181012095349.GA9162@ulmo in this
> thread.)

Let's be specific here: the vast majority of PWM consumers currently
are pwm-backlight, leds-pwm and pwm-fan. All of them call pwm_disable()
when the devices are turned off (backlight and LED turn off, fan stops)
which is equivalent to duty-cycle = 0.

> To get forward here: Thierry, can you please confirm that there is no
> semantical difference between "pwm_disable()" and "pwm_config(pwm, 0,
> period)". (Or alternatively come up with a use case where a difference
> really matters. I'm sure this is impossible though.)

And here I was hoping that we were making progress on this. Now you're
taking us back to square one...

> Then I can start simplifying API users and adapt the pwm
> hardware drivers. When this is done there will be no user left of
> pwm_disable and struct pwm_state::enabled because nobody needs it with
> the current semantic. After that we can either rip the concept of
> pwm_disable or shift it's semantic to my initial suggestion of "no
> guarantees about the output level" and use that in the few drivers that
> really don't care about the output level[1] and so give the hardware the
> possibility to disable the clock even if that results in something
> different than "inactive".

I have repeatedly told you that not making any guarantees is completely
useless. I can't think of a reason why any PWM consumer would ever want
to have the pin go into an undefined state.

> As a first step I'd suggest to explicitly state the expectations on
> hardware driver implementations and pwm-API users to get all affected
> parties in line. These are in my eyes:
> 
>  - configuring the duty cycle should (if possible) not result in
>    glitches, so the currently running period should be completed and
>    after that a new period with the new configuration should start.
>    (Is this reasonable to expect this from all implementations? If not
>    we should maybe introduce a way to let the users know?)
> 
>  - The function to configure the duty cycle should only return when the
>    period with the new configuration actually started.

Yes, those two are reasonable expectations in my opinion.

> If the concept of pwm_disable() stays, we should document the expected
> behaviour of the pin. Something like:
> 
>  - After disabling the pwm you must not make any assumptions about the
>    pin level. It might be low, or inactive, or even high-z. So don't
>    disable the pwm if you for example care about a display backlight to
>    stay off.

Like I said multiple times, that's completely useless. Can you please
provide an example where a consumer would possible want that behavior?

> If desired we might even keep the behaviour for the sysfs implementation
> that disable there for historic reasons keeps the semantic of
> duty-cycle=0.
> 
> Further things that could be done (eventually after discussing they are
> really sensible) are:
> 
>  - for pwms that can be programmed now for the next period implement the
>    necessary sleep in the PWM-Framework (mxs, atmel, sun4i at least);

I fail to see the point in this. It's incredibly trivial to do this in
the drivers. And a sleep is perhaps the simplest way to implement this
and could technically be made generic and handled in the core, but the
hardware could just as well have a mechanism to signal the beginning of
the next period in some bit in a register. So why would you want to add
code to the core that's highly device dependent? What do you gain from
it? If we already have the expectations documented that ->config() must
only return after the settings have been applied, then why move some of
the responsibility back into the core?

>  - abstract inversed pwms in the framework instead of each hw driver;

What exactly are you propsing here? We've had this discussion just a
couple of days ago and I already mentioned why the duty cycle inversion
is not the same thing as the signal inversion.

>  - let hw drivers correct *state on apply
>    (if the user requests period=1ms and the driver can only provide
>    multiples of 0.6667 ms because the input clock is fixed at 1500 Hz)

That's not a very good idea. If the user requests a configuration that
can't be met we should return an error, not silently apply something
that we think is good enough and hope that the user noticed that and
tries a different configuration instead.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 6337 bytes --]

On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> Hi Thierry,
> 
> On Tue, 9 Oct 2018 11:04:01 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Hello Thierry,
> > 
> > thanks for taking time to reply in this thread.
> > 
> > On Tue, Oct 09, 2018 at 09:34:07AM +0200, Thierry Reding wrote:
> > > On Mon, Aug 20, 2018 at 04:43:56PM +0200, Uwe Kleine-König wrote:  
> > > > Contrarily to the common assumption that pwm_disable() stops the
> > > > output at the state where it just happens to be, this isn't what the
> > > > hardware does on some machines. For example on i.MX6 the pin goes to
> > > > 0 level instead.
> > > > 
> > > > Note this doesn't reduce the expressiveness of the pwm-API because if
> > > > you want the PWM-Pin to stay at a given state, you are supposed to
> > > > configure it to 0% or 100% duty cycle without calling pwm_disable().
> > > > The backend driver is free to implement potential power savings
> > > > already when the duty cycle changes to one of these two cases so this
> > > > doesn't come at an increased runtime power cost (once the respective
> > > > drivers are fixed).
> > > > 
> > > > In return this allows to adhere to the API more easily for drivers that
> > > > currently cannot know if it's ok to disable the hardware on
> > > > pwm_disable() because the caller might or might not rely on the pin
> > > > stopping at a certain level.
> > > > 
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > >  Documentation/pwm.txt | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)  
> > > 
> > > I don't think this is correct. An API whose result is an undefined state
> > > is useless in my opinion. So either we properly define what the state
> > > should be after a disable operation, or we might as well just remove the
> > > disable operation altogether.  
> > 
> > Explicitly not defining some details makes it easier for hardware
> > drivers to comply with the API. Sure it would be great if all hardware
> > would allow to implement a "stronger" API but this isn't how reality looks
> > like.
> > 
> > You say it's bad that .disable() should imply less than it did up to
> > now. I say .disable() should only imply that the PWM stops toggling, so
> > .disable has a single purpose that each hardware design should be able
> > to implement.
> > And this weaker requirement/result is still good enough to implement all
> > use cases. (You had doubts here in the past, but without an example. I
> > cannot imagine there is one.) In my eyes this makes the API better not
> > worse.
> > 
> > What we have now in the API is redundancy, which IMHO is worse. If I
> > want the pwm pin to stay low I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 
> > or I can say:
> > 
> > 	pwm_config(pwm, 0, 100);
> > 	pwm_disable(pwm);
> > 
> > The expected result is the same. Now you could argue that not disabling
> > the pwm is a bug because it doesn't save some energy. But really this is
> > a weakness of the API. There are two ways to express "Set the pin to
> > constant 0" and if there is an opportunity to save some energy on a
> > certain hardware implementation, just calling pwm_config() with duty=0
> > should be enough to benefit from it. This makes the API easier to use
> > because there is a single command to get to the state the pwm user wants
> > the pwm to be in. (This is two advantages: A single way to reach the
> > desired state and only one function to call.)
> 
> I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> duty=100%" logic should, IMHO, be a HW-specific optimization. And if you
> look at existing drivers, it shouldn't be that hard to patch those that
> support this optimization since they most of the time have a separate
> disable function which could be called from the their config function
> if needed.

There's nothing in the API that prevents you from doing so and I never
said that this couldn't be done in pwm_config(). If you know it's safe
to disable the PWM if the duty cycle goes to zero, then by all means
do it.

That doesn't mean we necessarily have to go and change the API. There
may be drivers that prefer to do it in pwm_disable() and I don't see a
reason why we shouldn't be able to have both.

Quite frankly I think this whole discussion is moot. We already have a
different API (atomic) that was created specifically to solve these
kinds of issues. So if there's any driver issues with the legacy API,
then those drivers should be moved to the new API. All the public-facing
APIs already use those if available anyway. I don't see the point in
rev'ing an API that's deprecated anyway.

Also, the whole discussion here started because Uwe reported that the
i.MX driver wasn't expecting according to the assumptions from PWM
consumers (i.e. the PWM pin goes low for inverted PWM). So really no
amount of API changes is going to magically fix that problem.

> On the other hand, if we do it the other way around, and expect
> pwm_disable() to keep the pin in the state it was after setting a duty
> of 0, it becomes more complicated (impossible?) for PWM blocks that do
> not support specifying a default pin state when the PWM is disabled.

I think that'd be an unrealistic expectation. If you look at your
typical hardware design, engineers will have put it together in a way
that with the (hardware) defaults, your device will behave normally.
Those hardware defaults, or at least something as close to it as
possible, are what you should be programming your hardware blocks with
before you relinquish control in the driver. For pretty much all the
hardware devices that I'm familiar with, the "off" or "no power" state
is that hardware default.

In other words, when you boot a device the fans, backlights and LEDs
will be off by default. Therefore I think pwm_disable() should put the
PWM back into that state. That's evidently not what is happening for
i.MX currently, otherwise Uwe wouldn't be seeing that problem.

Furthermore, there is another patch currently under review that does
attempt to fix this in a proper way. So I just don't understand why we
keep going around in circles here.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] pwm: document the pin state after pwm_disable() to be undefined

[Uwe Kleine-König]

Hello Thierry,

On Thu, Oct 18, 2018 at 04:44:14PM +0200, Thierry Reding wrote:
> On Thu, Oct 18, 2018 at 10:44:05AM +0200, Uwe Kleine-König wrote:
> > On Tue, Oct 16, 2018 at 09:44:17AM +0200, Boris Brezillon wrote:
> > > I kinda agree with Uwe on this point. The "disable PWM on duty=0% or
> > > duty=100%" logic should, IMHO, be a HW-specific optimization.
> > 
> > Note that according to Thierry the behaviour of
> > 
> > 	pwm_config(pwm, 100, 100);
> > 	pwm_disable(pwm);
> > 
> > (and similarily of
> > 
> > 	pwm_apply_state({.period = 100, .duty_cycle = 100, .enabled = false});
> > 
> > ) should (assuming an uninverted pwm) result in the output stopping at 0
> > (i.e. it's inactive level which is the same as after pwm_config(pwm, 0, 100)).
> > 
> > So there isn't anything special about "disable PWM on duty=0% or
> > duty=100%". "disable PWM" in Thierry's eyes is always "stop at inactive
> > level" independant of the previously configured duty cycle.
> > 
> > You talking about the specific cases 0% and 100% makes me believe that
> > you are an example (BTW, I'm another) that makes Thierry's claim
> > 
> > 	The current assumption by all users is more that [after
> > 	pwm_disable()] the pin would stop at the no power state, which
> > 	would be 0 for normal PWM and 1 for inverted.
> > 
> > wrong. (See mail with Message-ID: 20181012095349.GA9162@ulmo in this
> > thread.)
> 
> Let's be specific here: the vast majority of PWM consumers currently
> are pwm-backlight, leds-pwm and pwm-fan. All of them call pwm_disable()
> when the devices are turned off (backlight and LED turn off, fan stops)
> which is equivalent to duty-cycle = 0.

Ah, you're talking about drivers making use of the PWM API, I thought
we're talking about the people working with PWMs.

The pwm-backlight driver only calls pwm_disable() after
pwm_config(pb->pwm, 0, pb->period). Same for leds-pwm. So I'd claim
their expectation is that pwm_disable() freezes the pwm, not that it
stops at idle. pwm-fan is strange, mixing the old API with
pwm_apply_state, assuming the maximal allowed duty cycle is period - 1,
so I'm not taking that one into account to argue.

> > To get forward here: Thierry, can you please confirm that there is no
> > semantical difference between "pwm_disable()" and "pwm_config(pwm, 0,
> > period)". (Or alternatively come up with a use case where a difference
> > really matters. I'm sure this is impossible though.)
> 
> And here I was hoping that we were making progress on this. Now you're
> taking us back to square one...

You wrote above that pwm_disable is equivalent to duty-cycle = 0. So we
are at an agreement finally it seems.

And yes, I asked you again for an example because "there is a difference
and some might care and use that difference" isn't an argument that is
getting us further. If the statement is true it would be more
cooperative to explain the difference and why it matters. As is it only
thwarts my efforts without a possibility to effectively accept or refute
it.

> > Then I can start simplifying API users and adapt the pwm
> > hardware drivers. When this is done there will be no user left of
> > pwm_disable and struct pwm_state::enabled because nobody needs it with
> > the current semantic. After that we can either rip the concept of
> > pwm_disable or shift it's semantic to my initial suggestion of "no
> > guarantees about the output level" and use that in the few drivers that
> > really don't care about the output level[1] and so give the hardware the
> > possibility to disable the clock even if that results in something
> > different than "inactive".
> 
> I have repeatedly told you that not making any guarantees is completely
> useless. I can't think of a reason why any PWM consumer would ever want
> to have the pin go into an undefined state.

No driver *wants* the pin go to an undefined state. But it might
indicate that doing so is acceptable without interfering with its
intentions. The example I had was a display that has both an enable GPIO
and a PWM for backlight. When the driver disables the display using the
GPIO the state of the PWM doesn't matter and it could call pwm_disable()
with the semantic I want pwm_disable to have.

> > As a first step I'd suggest to explicitly state the expectations on
> > hardware driver implementations and pwm-API users to get all affected
> > parties in line. These are in my eyes:
> > 
> >  - configuring the duty cycle should (if possible) not result in
> >    glitches, so the currently running period should be completed and
> >    after that a new period with the new configuration should start.
> >    (Is this reasonable to expect this from all implementations? If not
> >    we should maybe introduce a way to let the users know?)
> > 
> >  - The function to configure the duty cycle should only return when the
> >    period with the new configuration actually started.
> 
> Yes, those two are reasonable expectations in my opinion.
> 
> > If the concept of pwm_disable() stays, we should document the expected
> > behaviour of the pin. Something like:
> > 
> >  - After disabling the pwm you must not make any assumptions about the
> >    pin level. It might be low, or inactive, or even high-z. So don't
> >    disable the pwm if you for example care about a display backlight to
> >    stay off.
> 
> Like I said multiple times, that's completely useless. Can you please
> provide an example where a consumer would possible want that behavior?

see above.

Do you think it's sensible to keep pwm_disable() with the current semantic
(i.e. same result as pwm_config(pwm, 0, period))? If not, what is in
your eyes the best that should be done with the findings of this thread?

> > If desired we might even keep the behaviour for the sysfs implementation
> > that disable there for historic reasons keeps the semantic of
> > duty-cycle=0.
> > 
> > Further things that could be done (eventually after discussing they are
> > really sensible) are:
> > 
> >  - for pwms that can be programmed now for the next period implement the
> >    necessary sleep in the PWM-Framework (mxs, atmel, sun4i at least);
> 
> I fail to see the point in this. It's incredibly trivial to do this in
> the drivers.

It's better to have code that handles identical situations for several
drivers in a single place. Even if the code in question is simple.
Having only the stuff in a hardware driver that cannot be shared
simplifies understanding what it does, increases code coverage,
simplifies identifying further stuff that can be factored into the core.
And if you later notice that the simple stuff needs an adaption because
it isn't that simple after all or should cover another use case, you
only touch a single place. Then testing this change on a single
implementation gives more confidence that the change is also right for
the other affected drivers than when you fix each individual instance
individually.

> And a sleep is perhaps the simplest way to implement this
> and could technically be made generic and handled in the core, but the
> hardware could just as well have a mechanism to signal the beginning of
> the next period in some bit in a register.

I agree, if the hardware has a means to detect when the new period
started, that should be used instead of the generic sleep. That's why
the sleep in the core should only be used by the others.

> So why would you want to add code to the core that's highly device
> dependent?

I want to add code to the core that is generic enough to be used by
several drivers. For that to be useful it's not necessary that all
drivers benefit from that.

> What do you gain from it? If we already have the expectations
> documented that ->config() must only return after the settings have
> been applied, then why move some of the responsibility back into the
> core?

To simplify the hardware drivers and not solve the same problem (even if
it's easy) in a single place. And note, I want pwm_config to return only
after the settings are active, I didn't talk about the callback that hw
drivers have to implement. That's because ideally the PWM framework
isn't only a shim layer between the hardware and PWM users but it
implements things that are useful for both sides. So the things that a
caller of pwm_config should be able to rely on doesn't necessarily need
to be inherited to the implementers of the .config callback.

> >  - abstract inversed pwms in the framework instead of each hw driver;
> 
> What exactly are you propsing here? We've had this discussion just a
> couple of days ago and I already mentioned why the duty cycle inversion
> is not the same thing as the signal inversion.

Right, you claimed there is a difference. I argued that nobody can make
use of that difference because it's only a shift and as the period
borders are not communicated back to the PWM user there is nothing that
can be synced to it. Can you contradict that?

Also, when changing the polarity glitches can happen, even more so when
the PWM is disabled. Take for example an uninverted PWM running at 50%
that is then inverted and disabled at A and then enabled again at B:
     ____      ____            ____
    /    \____/    \____/\____/
    ^         ^         ^^
                      A  B

It might result in a peek which depending on the use case might be bad.

> >  - let hw drivers correct *state on apply
> >    (if the user requests period=1ms and the driver can only provide
> >    multiples of 0.6667 ms because the input clock is fixed at 1500 Hz)
> 
> That's not a very good idea. If the user requests a configuration that
> can't be met we should return an error, not silently apply something
> that we think is good enough and hope that the user noticed that and
> tries a different configuration instead.

I don't care much how this is done. The clk API explicitly states that
clk_set_rate might not exactly set the desired rate and provides a
function (clk_round_rate) to prophesy the result. That's what I had in
mind and what might make sense for PWM, too. After all most drivers have
to implement rounding already now if the granularity of period and duty
isn't a divisor of 1 ns.

Best regards
Uwe

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