[PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

For the cros-ec-pwm, "disabled" is the same as "duty cycle == 0", and is
not possible to program a duty cycle while the device is disabled. However,
the PWM API allows us to configure the "duty cycle" while the device is
"disabled". But now, pwm_get_state() is returning the real hardware state
instead of the last applied state, and this change of behavior, broke
the display on my rk3399-gru-kevin and doesn't turn on anymore.

Commit 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented
state") introduced this change of behavior. And, assuming that this is
the right to do, workaround this problem for the cros-ec-pwm driver by
reverting the mentioned commit at the lowlevel driver.

With that patch applied pwm_get_state() will return only the programmed
hardware duty cycle value if the PWM is enabled. When is disabled, will
return the last applied duty_cycle value instead. That's not ideal, but
definetely is better than don't implement .get_state().

Fixes: 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state")
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---

 drivers/pwm/pwm-cros-ec.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 89497448d217..f750a3cf0c6c 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -18,11 +18,13 @@
  * @dev: Device node
  * @ec: Pointer to EC device
  * @chip: PWM controller chip
+ * @state: Holds the last state applied
  */
 struct cros_ec_pwm_device {
 	struct device *dev;
 	struct cros_ec_device *ec;
 	struct pwm_chip chip;
+	struct pwm_state state;
 };
 
 static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
@@ -102,6 +104,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (state->period != EC_PWM_MAX_DUTY)
 		return -EINVAL;
 
+	/* Store the new state */
+	ec_pwm->state = *state;
+
 	/*
 	 * EC doesn't separate the concept of duty cycle and enabled, but
 	 * kernel does. Translate.
@@ -117,17 +122,28 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
 	int ret;
 
-	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
-	if (ret < 0) {
-		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
-		return;
+	/*
+	 * As there is no way for this hardware to separate the concept of
+	 * duty cycle and enabled, but the PWM API does, let return the last
+	 * applied state when the PWM is disabled and only return the real
+	 * hardware value when the PWM is enabled. Otherwise, a user of this
+	 * driver, can get confused because won't be able to program a duty
+	 * cycle while the PWM is disabled.
+	 */
+	state->enabled = ec_pwm->state.enabled;
+	if (state->enabled) {
+		ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
+		if (ret < 0) {
+			dev_err(chip->dev, "error getting initial duty: %d\n",
+				ret);
+			return;
+		}
+		state->duty_cycle = ret;
+	} else {
+		state->duty_cycle = ec_pwm->state.duty_cycle;
 	}
 
-	state->enabled = (ret > 0);
 	state->period = EC_PWM_MAX_DUTY;
-
-	/* Note that "disabled" and "duty cycle == 0" are treated the same */
-	state->duty_cycle = ret;
 }
 
 static struct pwm_device *
-- 
2.20.1

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

Hello Enric,

On Tue, Oct 08, 2019 at 12:54:17PM +0200, Enric Balletbo i Serra wrote:
> @@ -117,17 +122,28 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
>  	int ret;
>  
> -	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> -	if (ret < 0) {
> -		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
> -		return;
> +	/*
> +	 * As there is no way for this hardware to separate the concept of
> +	 * duty cycle and enabled, but the PWM API does, let return the last
> +	 * applied state when the PWM is disabled and only return the real
> +	 * hardware value when the PWM is enabled. Otherwise, a user of this
> +	 * driver, can get confused because won't be able to program a duty
> +	 * cycle while the PWM is disabled.
> +	 */
> +	state->enabled = ec_pwm->state.enabled;

> +	if (state->enabled) {

As part of registration of the pwm .get_state is called. In this case
.apply wasn't called before and so state->enabled is probably 0. So this
breaks reporting the initial state ...

> +		ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> +		if (ret < 0) {
> +			dev_err(chip->dev, "error getting initial duty: %d\n",
> +				ret);
> +			return;
> +		}
> +		state->duty_cycle = ret;
> +	} else {
> +		state->duty_cycle = ec_pwm->state.duty_cycle;
>  	}
>  
> -	state->enabled = (ret > 0);
>  	state->period = EC_PWM_MAX_DUTY;
> -
> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> -	state->duty_cycle = ret;

A few thoughts to your approach here ...:

 - Would it make sense to only store duty_cycle and enabled in the
   driver struct?

 - Which driver is the consumer of your pwm? If I understand correctly
   the following sequence is the bad one:

	state.period = P;
	state.duty_cycle = D;
	state.enabled = 0;
   	pwm_apply_state(pwm, &state);

	...

	pwm_get_state(pwm, &state);
	state.enabled = 1;
   	pwm_apply_state(pwm, &state);

   Before my patch there was an implicit promise in the PWM framework
   that the last pwm_apply_state has .duty_cycle = D (and .period = P).
   Is this worthwile, or should we instead declare this as
   non-guaranteed and fix the caller?

 - If this is a more or less common property that hardware doesn't know
   the concept of "disabled" maybe it would make sense to drop this from
   the PWM framework, too. (This is a question that I discussed some
   time ago already with Thierry, but without an result. The key
   question is: What is the difference between "disabled" and
   "duty_cycle = 0" in general and does any consumer care about it.)

 - A softer variant of the above: Should pwm_get_state() anticipate that
   with .enabled = 0 the duty_cycle (and maybe also period) is
   unreliable and cache that for callers?

Unrelated to the patch in question I noticed that the cros-ec-pwm driver
doesn't handle polarity. We need

	state->polarity = PWM_POLARITY_NORMAL;

in cros_ec_pwm_get_state() and

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -ERANGE;

in cros_ec_pwm_apply(). (Not sure -ERANGE is the right value, I think
there is no global rule in force that tells the right value though.)

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

Hi Uwe,

Thanks for the quick reply.

On 8/10/19 16:34, Uwe Kleine-König wrote:
> Hello Enric,
> 
> On Tue, Oct 08, 2019 at 12:54:17PM +0200, Enric Balletbo i Serra wrote:
>> @@ -117,17 +122,28 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
>>  	int ret;
>>  
>> -	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
>> -	if (ret < 0) {
>> -		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
>> -		return;
>> +	/*
>> +	 * As there is no way for this hardware to separate the concept of
>> +	 * duty cycle and enabled, but the PWM API does, let return the last
>> +	 * applied state when the PWM is disabled and only return the real
>> +	 * hardware value when the PWM is enabled. Otherwise, a user of this
>> +	 * driver, can get confused because won't be able to program a duty
>> +	 * cycle while the PWM is disabled.
>> +	 */
>> +	state->enabled = ec_pwm->state.enabled;
> 
>> +	if (state->enabled) {
> 
> As part of registration of the pwm .get_state is called. In this case
> .apply wasn't called before and so state->enabled is probably 0. So this
> breaks reporting the initial state ...
> 
>> +		ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
>> +		if (ret < 0) {
>> +			dev_err(chip->dev, "error getting initial duty: %d\n",
>> +				ret);
>> +			return;
>> +		}
>> +		state->duty_cycle = ret;
>> +	} else {
>> +		state->duty_cycle = ec_pwm->state.duty_cycle;
>>  	}
>>  
>> -	state->enabled = (ret > 0);
>>  	state->period = EC_PWM_MAX_DUTY;
>> -
>> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
>> -	state->duty_cycle = ret;
> 
> A few thoughts to your approach here ...:
> 
>  - Would it make sense to only store duty_cycle and enabled in the
>    driver struct?
> 

Yes, in fact, my first approach (that I didn't send) was only storing enabled
and duty cycle. For some reason I ended storing the full pwm_state struct, but I
guess is not really needed.


>  - Which driver is the consumer of your pwm? If I understand correctly
>    the following sequence is the bad one:
> 

The consumer is the pwm_bl driver. Actually I'n trying to identify other consumers.

> 	state.period = P;
> 	state.duty_cycle = D;
> 	state.enabled = 0;
>    	pwm_apply_state(pwm, &state);
> 
> 	...
> 
> 	pwm_get_state(pwm, &state);
> 	state.enabled = 1;
>    	pwm_apply_state(pwm, &state);
> 

Yes that's the sequence.

>    Before my patch there was an implicit promise in the PWM framework
>    that the last pwm_apply_state has .duty_cycle = D (and .period = P).
>    Is this worthwile, or should we instead declare this as
>    non-guaranteed and fix the caller?
> 

pwm_bl is compliant with this, the problem in the pwm-cros-ec driver is when you
set the duty_cycle but enable is 0.

>  - If this is a more or less common property that hardware doesn't know
>    the concept of "disabled" maybe it would make sense to drop this from
>    the PWM framework, too. (This is a question that I discussed some
>    time ago already with Thierry, but without an result. The key
>    question is: What is the difference between "disabled" and
>    "duty_cycle = 0" in general and does any consumer care about it.)
> 

Good question, I don't really know all consumer requirements, but AFAIK, usually
when you want to program duty_cycle to 0 you also want to disable the PWM. At
least for the backlight case doesn't make sense program first the duty_cycle and
then enable the PWM, is implicit, if duty_cycle is 0 the PWM is disabled, if
duty_cycle > 0 the PWM is enabled.

>  - A softer variant of the above: Should pwm_get_state() anticipate that
>    with .enabled = 0 the duty_cycle (and maybe also period) is
>    unreliable and cache that for callers?
> 

Sorry, when you say pwm_get_state(), you mean the core call or the lowlevel
driver call?

> Unrelated to the patch in question I noticed that the cros-ec-pwm driver
> doesn't handle polarity. We need
> 
> 	state->polarity = PWM_POLARITY_NORMAL;
> 
> in cros_ec_pwm_get_state() and
> 
> 	if (state->polarity != PWM_POLARITY_NORMAL)
> 		return -ERANGE;
> 
> in cros_ec_pwm_apply(). (Not sure -ERANGE is the right value, I think
> there is no global rule in force that tells the right value though.)
> 

Nice catch, thanks, I'll send a patch to fix this.

Thanks,
 Enric

> Best regards
> Uwe
> 

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> Hi Uwe,
> 
> Thanks for the quick reply.
> 
> On 8/10/19 16:34, Uwe Kleine-König wrote:
> > Hello Enric,
> > 
> > On Tue, Oct 08, 2019 at 12:54:17PM +0200, Enric Balletbo i Serra wrote:
> >> @@ -117,17 +122,28 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> >>  	int ret;
> >>  
> >> -	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> >> -	if (ret < 0) {
> >> -		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
> >> -		return;
> >> +	/*
> >> +	 * As there is no way for this hardware to separate the concept of
> >> +	 * duty cycle and enabled, but the PWM API does, let return the last
> >> +	 * applied state when the PWM is disabled and only return the real
> >> +	 * hardware value when the PWM is enabled. Otherwise, a user of this
> >> +	 * driver, can get confused because won't be able to program a duty
> >> +	 * cycle while the PWM is disabled.
> >> +	 */
> >> +	state->enabled = ec_pwm->state.enabled;
> > 
> >> +	if (state->enabled) {
> > 
> > As part of registration of the pwm .get_state is called. In this case
> > .apply wasn't called before and so state->enabled is probably 0. So this
> > breaks reporting the initial state ...
> > 
> >> +		ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> >> +		if (ret < 0) {
> >> +			dev_err(chip->dev, "error getting initial duty: %d\n",
> >> +				ret);
> >> +			return;
> >> +		}
> >> +		state->duty_cycle = ret;
> >> +	} else {
> >> +		state->duty_cycle = ec_pwm->state.duty_cycle;
> >>  	}
> >>  
> >> -	state->enabled = (ret > 0);
> >>  	state->period = EC_PWM_MAX_DUTY;
> >> -
> >> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> >> -	state->duty_cycle = ret;
> > 
> > A few thoughts to your approach here ...:
> > 
> >  - Would it make sense to only store duty_cycle and enabled in the
> >    driver struct?
> > 
> 
> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> guess is not really needed.
> 
> 
> >  - Which driver is the consumer of your pwm? If I understand correctly
> >    the following sequence is the bad one:
> > 
> 
> The consumer is the pwm_bl driver. Actually I'n trying to identify
> other consumers.

Ah, I see why I missed to identify the problem back when I checked this
driver. The problem is not that .duty_cycle isn't set but there .enabled
isn't set. So maybe we just want:

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 2201b8c78641..0468c6ee4448 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
        if (brightness > 0) {
                pwm_get_state(pb->pwm, &state);
                state.duty_cycle = compute_duty_cycle(pb, brightness);
+               state.enabled = true;
                pwm_apply_state(pb->pwm, &state);
                pwm_backlight_power_on(pb);
        } else

? On a side note: It's IMHO strange that pwm_backlight_power_on
reconfigures the PWM once more.

> > 	state.period = P;
> > 	state.duty_cycle = D;
> > 	state.enabled = 0;
> >    	pwm_apply_state(pwm, &state);
> > 
> > 	...
> > 
> > 	pwm_get_state(pwm, &state);
> > 	state.enabled = 1;
> >    	pwm_apply_state(pwm, &state);
> > 
> 
> Yes that's the sequence.
> 
> >    Before my patch there was an implicit promise in the PWM framework
> >    that the last pwm_apply_state has .duty_cycle = D (and .period = P).
> >    Is this worthwile, or should we instead declare this as
> >    non-guaranteed and fix the caller?
> > 
> 
> pwm_bl is compliant with this, the problem in the pwm-cros-ec driver is when you
> set the duty_cycle but enable is 0.

pwm_bl *relies* on this behaviour. The question is: Is this a valid
assumption to rely on (for consumers) resp. to guarantee (for the PWM
framework)? I'm not sure it is because each PWM that doesn't know the
concept of "disabled" (not sure how many there are) needs some effort to
simulate it (by caching duty_cycle and period on disable).

Dropping this promise and fix pwm_bl (and maybe other consumers that
rely on it) is my preferred solution.
 
> >  - If this is a more or less common property that hardware doesn't know
> >    the concept of "disabled" maybe it would make sense to drop this from
> >    the PWM framework, too. (This is a question that I discussed some
> >    time ago already with Thierry, but without an result. The key
> >    question is: What is the difference between "disabled" and
> >    "duty_cycle = 0" in general and does any consumer care about it.)
> > 
> 
> Good question, I don't really know all consumer requirements, but AFAIK, usually
> when you want to program duty_cycle to 0 you also want to disable the PWM.

Note that hardware designers are "creative" and "disable the PWM" has
different semantics for different PWMs. Some PWMs just stop the output
at the level that it happens to be in, some stop in the inactive level,
some stop at 0, some stop driving the pin. Currently the intended
semantic of a disabled PWM is that it drives the inactive level (but it
might be smart and stop driving if there is a pull in the right
direction). I see no benefit of this semantic as it can also be
accomplished by setting .duty_cycle = 0, .period = $something_small.
Thierry doesn't agree and I fail to understand his reasoning.

> At least for the backlight case doesn't make sense program first the
> duty_cycle and then enable the PWM, is implicit, if duty_cycle is 0
> the PWM is disabled, if duty_cycle > 0 the PWM is enabled.

Yeah, that's my conclusion of above, too. After all the pwm_apply_state
function is there for being able to go from one state to each other
state with a single function call.

> >  - A softer variant of the above: Should pwm_get_state() anticipate that
> >    with .enabled = 0 the duty_cycle (and maybe also period) is
> >    unreliable and cache that for callers?
> 
> Sorry, when you say pwm_get_state(), you mean the core call or the lowlevel
> driver call?

The suggestion is to do what you do in the driver (i.e. remember
duty_cycle and in the general case also period) in the framework
instead and fix the problem for all lowlevel drivers that behave similar
to the implementation in question. i.e. don't rely on .duty_cycle and
.period having a sensible value after .get_state() if the PWM is off.
This is IMHO the second best option.

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

Hi Uwe,

Adding Daniel and Lee to the discussion ...


On 8/10/19 22:31, Uwe Kleine-König wrote:
> On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
>> Hi Uwe,
>>
>> Thanks for the quick reply.
>>
>> On 8/10/19 16:34, Uwe Kleine-König wrote:
>>> Hello Enric,
>>>
>>> On Tue, Oct 08, 2019 at 12:54:17PM +0200, Enric Balletbo i Serra wrote:
>>>> @@ -117,17 +122,28 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>>>>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
>>>>  	int ret;
>>>>  
>>>> -	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
>>>> -	if (ret < 0) {
>>>> -		dev_err(chip->dev, "error getting initial duty: %d\n", ret);
>>>> -		return;
>>>> +	/*
>>>> +	 * As there is no way for this hardware to separate the concept of
>>>> +	 * duty cycle and enabled, but the PWM API does, let return the last
>>>> +	 * applied state when the PWM is disabled and only return the real
>>>> +	 * hardware value when the PWM is enabled. Otherwise, a user of this
>>>> +	 * driver, can get confused because won't be able to program a duty
>>>> +	 * cycle while the PWM is disabled.
>>>> +	 */
>>>> +	state->enabled = ec_pwm->state.enabled;
>>>
>>>> +	if (state->enabled) {
>>>
>>> As part of registration of the pwm .get_state is called. In this case
>>> .apply wasn't called before and so state->enabled is probably 0. So this
>>> breaks reporting the initial state ...
>>>
>>>> +		ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
>>>> +		if (ret < 0) {
>>>> +			dev_err(chip->dev, "error getting initial duty: %d\n",
>>>> +				ret);
>>>> +			return;
>>>> +		}
>>>> +		state->duty_cycle = ret;
>>>> +	} else {
>>>> +		state->duty_cycle = ec_pwm->state.duty_cycle;
>>>>  	}
>>>>  
>>>> -	state->enabled = (ret > 0);
>>>>  	state->period = EC_PWM_MAX_DUTY;
>>>> -
>>>> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
>>>> -	state->duty_cycle = ret;
>>>
>>> A few thoughts to your approach here ...:
>>>
>>>  - Would it make sense to only store duty_cycle and enabled in the
>>>    driver struct?
>>>
>>
>> Yes, in fact, my first approach (that I didn't send) was only storing enabled
>> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
>> guess is not really needed.
>>
>>
>>>  - Which driver is the consumer of your pwm? If I understand correctly
>>>    the following sequence is the bad one:
>>>
>>
>> The consumer is the pwm_bl driver. Actually I'n trying to identify
>> other consumers.
> 

So far, the pwm_bl driver is the only consumer of cros-ec-pwm.

> Ah, I see why I missed to identify the problem back when I checked this
> driver. The problem is not that .duty_cycle isn't set but there .enabled
> isn't set. So maybe we just want:
> 
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 2201b8c78641..0468c6ee4448 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>         if (brightness > 0) {
>                 pwm_get_state(pb->pwm, &state);
>                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> +               state.enabled = true;
>                 pwm_apply_state(pb->pwm, &state);
>                 pwm_backlight_power_on(pb);
>         } else
> 
> ? On a side note: It's IMHO strange that pwm_backlight_power_on
> reconfigures the PWM once more.
> 

Looking again to the pwm_bl code, now, I am not sure this is correct (although
it probably solves the problem for me).

Current behaviour is:

* If brightness > 0 and pwm_bl is disabled

   pwm_get_state(pb->pwm, &state);
   state.duty_cycle = compute_duty_cycle(pb, brightness);
   pwm_apply_state(pb->pwm, &state);
   pwm_backlight_power_on(pb);
      regulator_enable(pb->power_supply);
      state.enabled = true;
      pwm_apply_state(pb->pwm, &state);

* If brightness > 0 and pwm_bl is already enabled

   pwm_get_state(pb->pwm, &state);
   state.duty_cycle = compute_duty_cycle(pb, brightness);
   pwm_apply_state(pb->pwm, &state);

The sequence:'first' set duty_cycle and 'second' enable the PWM makes some kind
of sense because there is a regulator_enable in the middle of the power on sequence.

To work for me I need to submit state.enabled && state.duty_cycle atomically. So
I thin that solving the problem at lowlevel driver (aka cros-ec-pwm) makes more
sense. At the end, is really a problem of the lowlevel driver, and the PWM
framework is enough flexible which is fine.

Note: I did a quick look at different PWM drivers that implement .get_state()
and looks like the cros-ec-pwm is the only driver that has this restriction.

>>> 	state.period = P;
>>> 	state.duty_cycle = D;
>>> 	state.enabled = 0;
>>>    	pwm_apply_state(pwm, &state);
>>>
>>> 	...
>>>
>>> 	pwm_get_state(pwm, &state);
>>> 	state.enabled = 1;
>>>    	pwm_apply_state(pwm, &state);
>>>
>>
>> Yes that's the sequence.
>>
>>>    Before my patch there was an implicit promise in the PWM framework
>>>    that the last pwm_apply_state has .duty_cycle = D (and .period = P).
>>>    Is this worthwile, or should we instead declare this as
>>>    non-guaranteed and fix the caller?
>>>
>>
>> pwm_bl is compliant with this, the problem in the pwm-cros-ec driver is when you
>> set the duty_cycle but enable is 0.
> 
> pwm_bl *relies* on this behaviour. The question is: Is this a valid
> assumption to rely on (for consumers) resp. to guarantee (for the PWM
> framework)? I'm not sure it is because each PWM that doesn't know the
> concept of "disabled" (not sure how many there are) needs some effort to
> simulate it (by caching duty_cycle and period on disable).
> 
> Dropping this promise and fix pwm_bl (and maybe other consumers that
> rely on it) is my preferred solution.
>  
>>>  - If this is a more or less common property that hardware doesn't know
>>>    the concept of "disabled" maybe it would make sense to drop this from
>>>    the PWM framework, too. (This is a question that I discussed some
>>>    time ago already with Thierry, but without an result. The key
>>>    question is: What is the difference between "disabled" and
>>>    "duty_cycle = 0" in general and does any consumer care about it.)
>>>
>>
>> Good question, I don't really know all consumer requirements, but AFAIK, usually
>> when you want to program duty_cycle to 0 you also want to disable the PWM.
> 
> Note that hardware designers are "creative" and "disable the PWM" has
> different semantics for different PWMs. Some PWMs just stop the output
> at the level that it happens to be in, some stop in the inactive level,
> some stop at 0, some stop driving the pin. Currently the intended
> semantic of a disabled PWM is that it drives the inactive level (but it
> might be smart and stop driving if there is a pull in the right
> direction). I see no benefit of this semantic as it can also be
> accomplished by setting .duty_cycle = 0, .period = $something_small.
> Thierry doesn't agree and I fail to understand his reasoning.
> 
>> At least for the backlight case doesn't make sense program first the
>> duty_cycle and then enable the PWM, is implicit, if duty_cycle is 0
>> the PWM is disabled, if duty_cycle > 0 the PWM is enabled.
> 
> Yeah, that's my conclusion of above, too. After all the pwm_apply_state
> function is there for being able to go from one state to each other
> state with a single function call.
> 

Looking at the code again cahnged my point of view on this, see my comment above.

>>>  - A softer variant of the above: Should pwm_get_state() anticipate that
>>>    with .enabled = 0 the duty_cycle (and maybe also period) is
>>>    unreliable and cache that for callers?
>>
>> Sorry, when you say pwm_get_state(), you mean the core call or the lowlevel
>> driver call?
> 
> The suggestion is to do what you do in the driver (i.e. remember
> duty_cycle and in the general case also period) in the framework
> instead and fix the problem for all lowlevel drivers that behave similar
> to the implementation in question. i.e. don't rely on .duty_cycle and
> .period having a sensible value after .get_state() if the PWM is off.
> This is IMHO the second best option.
> 
> Best regards
> Uwe
> 

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Daniel Thompson]

On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> Hi Uwe,
> 
> Adding Daniel and Lee to the discussion ...

Thanks!

> On 8/10/19 22:31, Uwe Kleine-König wrote:
> > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> >>> A few thoughts to your approach here ...:
> >>>
> >>>  - Would it make sense to only store duty_cycle and enabled in the
> >>>    driver struct?
> >>>
> >>
> >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> >> guess is not really needed.
> >>
> >>
> >>>  - Which driver is the consumer of your pwm? If I understand correctly
> >>>    the following sequence is the bad one:
> >>>
> >>
> >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> >> other consumers.
> > 
> 
> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> 
> > Ah, I see why I missed to identify the problem back when I checked this
> > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > isn't set. So maybe we just want:
> > 
> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > index 2201b8c78641..0468c6ee4448 100644
> > --- a/drivers/video/backlight/pwm_bl.c
> > +++ b/drivers/video/backlight/pwm_bl.c
> > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> >         if (brightness > 0) {
> >                 pwm_get_state(pb->pwm, &state);
> >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > +               state.enabled = true;
> >                 pwm_apply_state(pb->pwm, &state);
> >                 pwm_backlight_power_on(pb);
> >         } else
> > 
> > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > reconfigures the PWM once more.
> > 
> 
> Looking again to the pwm_bl code, now, I am not sure this is correct (although
> it probably solves the problem for me).

Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
almost certainly accept a patch to pwm_bl to move the PWM enable/disable
out of both the power on/off functions so the duty-cycle/enable or
disable can happen in one go within the update_status function. I don't
think such a change would interfere with the power and enable sequencing
needed by panels and it would therefore be a nice continuation of the
work to convert over to the pwm_apply_state() API.

None of the above has anything to do with what is right or wrong for
the PWM API evolution. Of course, if this thread does conclude that it
is OK the duty cycle of a disabled PWM to be retained for some drivers
and not others then I'd hope to see some WARN_ON()s added to the PWM
framework to help bring problems to the surface with all drivers.


Daniel.

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> > Hi Uwe,
> > 
> > Adding Daniel and Lee to the discussion ...
> 
> Thanks!
> 
> > On 8/10/19 22:31, Uwe Kleine-König wrote:
> > > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> > >>> A few thoughts to your approach here ...:
> > >>>
> > >>>  - Would it make sense to only store duty_cycle and enabled in the
> > >>>    driver struct?
> > >>>
> > >>
> > >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> > >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> > >> guess is not really needed.
> > >>
> > >>
> > >>>  - Which driver is the consumer of your pwm? If I understand correctly
> > >>>    the following sequence is the bad one:
> > >>>
> > >>
> > >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> > >> other consumers.
> > > 
> > 
> > So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> > 
> > > Ah, I see why I missed to identify the problem back when I checked this
> > > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > > isn't set. So maybe we just want:
> > > 
> > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > index 2201b8c78641..0468c6ee4448 100644
> > > --- a/drivers/video/backlight/pwm_bl.c
> > > +++ b/drivers/video/backlight/pwm_bl.c
> > > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> > >         if (brightness > 0) {
> > >                 pwm_get_state(pb->pwm, &state);
> > >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > > +               state.enabled = true;
> > >                 pwm_apply_state(pb->pwm, &state);
> > >                 pwm_backlight_power_on(pb);
> > >         } else
> > > 
> > > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > > reconfigures the PWM once more.
> > > 
> > 
> > Looking again to the pwm_bl code, now, I am not sure this is correct (although
> > it probably solves the problem for me).
> 
> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> out of both the power on/off functions so the duty-cycle/enable or
> disable can happen in one go within the update_status function. I don't
> think such a change would interfere with the power and enable sequencing
> needed by panels and it would therefore be a nice continuation of the
> work to convert over to the pwm_apply_state() API.

OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
I'd expect that this alone should already fix your issue.
 
> None of the above has anything to do with what is right or wrong for
> the PWM API evolution. Of course, if this thread does conclude that it
> is OK the duty cycle of a disabled PWM to be retained for some drivers
> and not others then I'd hope to see some WARN_ON()s added to the PWM
> framework to help bring problems to the surface with all drivers.

I think it's not possible to add a reliable WARN_ON for that issue. It
is quite expected that .get_state returns something that doesn't
completely match the requested configuration. So if a consumer requests

	.duty_cycle = 1
	.period = 100000000
	.enabled = false

pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
that has a concept of duty_cycle for disabled hardware.

A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

On 9/10/19 12:16, Uwe Kleine-König wrote:
> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
>>> Hi Uwe,
>>>
>>> Adding Daniel and Lee to the discussion ...
>>
>> Thanks!
>>
>>> On 8/10/19 22:31, Uwe Kleine-König wrote:
>>>> On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
>>>>>> A few thoughts to your approach here ...:
>>>>>>
>>>>>>  - Would it make sense to only store duty_cycle and enabled in the
>>>>>>    driver struct?
>>>>>>
>>>>>
>>>>> Yes, in fact, my first approach (that I didn't send) was only storing enabled
>>>>> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
>>>>> guess is not really needed.
>>>>>
>>>>>
>>>>>>  - Which driver is the consumer of your pwm? If I understand correctly
>>>>>>    the following sequence is the bad one:
>>>>>>
>>>>>
>>>>> The consumer is the pwm_bl driver. Actually I'n trying to identify
>>>>> other consumers.
>>>>
>>>
>>> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
>>>
>>>> Ah, I see why I missed to identify the problem back when I checked this
>>>> driver. The problem is not that .duty_cycle isn't set but there .enabled
>>>> isn't set. So maybe we just want:
>>>>
>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>>> index 2201b8c78641..0468c6ee4448 100644
>>>> --- a/drivers/video/backlight/pwm_bl.c
>>>> +++ b/drivers/video/backlight/pwm_bl.c
>>>> @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>>>>         if (brightness > 0) {
>>>>                 pwm_get_state(pb->pwm, &state);
>>>>                 state.duty_cycle = compute_duty_cycle(pb, brightness);
>>>> +               state.enabled = true;
>>>>                 pwm_apply_state(pb->pwm, &state);
>>>>                 pwm_backlight_power_on(pb);
>>>>         } else
>>>>
>>>> ? On a side note: It's IMHO strange that pwm_backlight_power_on
>>>> reconfigures the PWM once more.
>>>>
>>>
>>> Looking again to the pwm_bl code, now, I am not sure this is correct (although
>>> it probably solves the problem for me).
>>
>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable
>> out of both the power on/off functions so the duty-cycle/enable or
>> disable can happen in one go within the update_status function. I don't
>> think such a change would interfere with the power and enable sequencing
>> needed by panels and it would therefore be a nice continuation of the
>> work to convert over to the pwm_apply_state() API.
> 
> OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> I'd expect that this alone should already fix your issue.
>  

Yes, I'll work on a proposal and send. Thanks you all.

Regards,
 Enric

>> None of the above has anything to do with what is right or wrong for
>> the PWM API evolution. Of course, if this thread does conclude that it
>> is OK the duty cycle of a disabled PWM to be retained for some drivers
>> and not others then I'd hope to see some WARN_ON()s added to the PWM
>> framework to help bring problems to the surface with all drivers.
> 
> I think it's not possible to add a reliable WARN_ON for that issue. It
> is quite expected that .get_state returns something that doesn't
> completely match the requested configuration. So if a consumer requests
> 
> 	.duty_cycle = 1
> 	.period = 100000000
> 	.enabled = false
> 
> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> that has a concept of duty_cycle for disabled hardware.
> 
> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
> 
> Best regards
> Uwe
> 

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Daniel Thompson]

On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> > > Hi Uwe,
> > > 
> > > Adding Daniel and Lee to the discussion ...
> > 
> > Thanks!
> > 
> > > On 8/10/19 22:31, Uwe Kleine-König wrote:
> > > > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> > > >>> A few thoughts to your approach here ...:
> > > >>>
> > > >>>  - Would it make sense to only store duty_cycle and enabled in the
> > > >>>    driver struct?
> > > >>>
> > > >>
> > > >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> > > >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> > > >> guess is not really needed.
> > > >>
> > > >>
> > > >>>  - Which driver is the consumer of your pwm? If I understand correctly
> > > >>>    the following sequence is the bad one:
> > > >>>
> > > >>
> > > >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> > > >> other consumers.
> > > > 
> > > 
> > > So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> > > 
> > > > Ah, I see why I missed to identify the problem back when I checked this
> > > > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > > > isn't set. So maybe we just want:
> > > > 
> > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > index 2201b8c78641..0468c6ee4448 100644
> > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> > > >         if (brightness > 0) {
> > > >                 pwm_get_state(pb->pwm, &state);
> > > >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > > > +               state.enabled = true;
> > > >                 pwm_apply_state(pb->pwm, &state);
> > > >                 pwm_backlight_power_on(pb);
> > > >         } else
> > > > 
> > > > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > > > reconfigures the PWM once more.
> > > > 
> > > 
> > > Looking again to the pwm_bl code, now, I am not sure this is correct (although
> > > it probably solves the problem for me).
> > 
> > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> > almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> > out of both the power on/off functions so the duty-cycle/enable or
> > disable can happen in one go within the update_status function. I don't
> > think such a change would interfere with the power and enable sequencing
> > needed by panels and it would therefore be a nice continuation of the
> > work to convert over to the pwm_apply_state() API.
> 
> OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> I'd expect that this alone should already fix your issue.
>  
> > None of the above has anything to do with what is right or wrong for
> > the PWM API evolution. Of course, if this thread does conclude that it
> > is OK the duty cycle of a disabled PWM to be retained for some drivers
> > and not others then I'd hope to see some WARN_ON()s added to the PWM
> > framework to help bring problems to the surface with all drivers.
> 
> I think it's not possible to add a reliable WARN_ON for that issue. It
> is quite expected that .get_state returns something that doesn't
> completely match the requested configuration. So if a consumer requests
> 
> 	.duty_cycle = 1
> 	.period = 100000000
> 	.enabled = false
> 
> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> that has a concept of duty_cycle for disabled hardware.
> 
> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.

Isn't that intended to help identify "odd" PWM drivers rather than "odd"
clients?

Initially I was thinking that a WARN_ON() could be emitted when:

1. .duty_cycle is non-zero
2. .enabled is false
3. the PWM is not already enabled

(#3 included to avoid too many false positives when disabling a PWM)

A poisoning approach might be equally valid. If some drivers are
permitted to "round" .duty_cycle to 0 when .enabled is false then the
framework could get *all* drivers to behave in the same way by
zeroing it out before calling the drivers apply method. It is not that
big a deal but minimising the difference between driver behaviour should
automatically reduce the difference in API usage by clients.


Daniel.

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote:
> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
> > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> > > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> > > > Hi Uwe,
> > > > 
> > > > Adding Daniel and Lee to the discussion ...
> > > 
> > > Thanks!
> > > 
> > > > On 8/10/19 22:31, Uwe Kleine-König wrote:
> > > > > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> > > > >>> A few thoughts to your approach here ...:
> > > > >>>
> > > > >>>  - Would it make sense to only store duty_cycle and enabled in the
> > > > >>>    driver struct?
> > > > >>>
> > > > >>
> > > > >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> > > > >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> > > > >> guess is not really needed.
> > > > >>
> > > > >>
> > > > >>>  - Which driver is the consumer of your pwm? If I understand correctly
> > > > >>>    the following sequence is the bad one:
> > > > >>>
> > > > >>
> > > > >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> > > > >> other consumers.
> > > > > 
> > > > 
> > > > So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> > > > 
> > > > > Ah, I see why I missed to identify the problem back when I checked this
> > > > > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > > > > isn't set. So maybe we just want:
> > > > > 
> > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > index 2201b8c78641..0468c6ee4448 100644
> > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> > > > >         if (brightness > 0) {
> > > > >                 pwm_get_state(pb->pwm, &state);
> > > > >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > > > > +               state.enabled = true;
> > > > >                 pwm_apply_state(pb->pwm, &state);
> > > > >                 pwm_backlight_power_on(pb);
> > > > >         } else
> > > > > 
> > > > > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > > > > reconfigures the PWM once more.
> > > > > 
> > > > 
> > > > Looking again to the pwm_bl code, now, I am not sure this is correct (although
> > > > it probably solves the problem for me).
> > > 
> > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> > > almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> > > out of both the power on/off functions so the duty-cycle/enable or
> > > disable can happen in one go within the update_status function. I don't
> > > think such a change would interfere with the power and enable sequencing
> > > needed by panels and it would therefore be a nice continuation of the
> > > work to convert over to the pwm_apply_state() API.
> > 
> > OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> > I'd expect that this alone should already fix your issue.
> >  
> > > None of the above has anything to do with what is right or wrong for
> > > the PWM API evolution. Of course, if this thread does conclude that it
> > > is OK the duty cycle of a disabled PWM to be retained for some drivers
> > > and not others then I'd hope to see some WARN_ON()s added to the PWM
> > > framework to help bring problems to the surface with all drivers.
> > 
> > I think it's not possible to add a reliable WARN_ON for that issue. It
> > is quite expected that .get_state returns something that doesn't
> > completely match the requested configuration. So if a consumer requests
> > 
> > 	.duty_cycle = 1
> > 	.period = 100000000
> > 	.enabled = false
> > 
> > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> > that has a concept of duty_cycle for disabled hardware.
> > 
> > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
> 
> Isn't that intended to help identify "odd" PWM drivers rather than "odd"
> clients?
> 
> Initially I was thinking that a WARN_ON() could be emitted when:
> 
> 1. .duty_cycle is non-zero
> 2. .enabled is false
> 3. the PWM is not already enabled
> 
> (#3 included to avoid too many false positives when disabling a PWM)

I think I created a patch for that in the past, don't remember the
details.

> A poisoning approach might be equally valid. If some drivers are
> permitted to "round" .duty_cycle to 0 when .enabled is false then the
> framework could get *all* drivers to behave in the same way by
> zeroing it out before calling the drivers apply method. It is not that
> big a deal but minimising the difference between driver behaviour should
> automatically reduce the difference in API usage by clients.

I like it, but that breaks consumers that set .duty_cycle once during
probe and then only do:

	pwm_get_state(pwm, &state);
	state.enabled = ...
	pwm_apply_state(pwm, &state);

which is a common idiom.

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Daniel Thompson]

On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote:
> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote:
> > On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
> > > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> > > > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> > > > > Hi Uwe,
> > > > > 
> > > > > Adding Daniel and Lee to the discussion ...
> > > > 
> > > > Thanks!
> > > > 
> > > > > On 8/10/19 22:31, Uwe Kleine-König wrote:
> > > > > > On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> > > > > >>> A few thoughts to your approach here ...:
> > > > > >>>
> > > > > >>>  - Would it make sense to only store duty_cycle and enabled in the
> > > > > >>>    driver struct?
> > > > > >>>
> > > > > >>
> > > > > >> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> > > > > >> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> > > > > >> guess is not really needed.
> > > > > >>
> > > > > >>
> > > > > >>>  - Which driver is the consumer of your pwm? If I understand correctly
> > > > > >>>    the following sequence is the bad one:
> > > > > >>>
> > > > > >>
> > > > > >> The consumer is the pwm_bl driver. Actually I'n trying to identify
> > > > > >> other consumers.
> > > > > > 
> > > > > 
> > > > > So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> > > > > 
> > > > > > Ah, I see why I missed to identify the problem back when I checked this
> > > > > > driver. The problem is not that .duty_cycle isn't set but there .enabled
> > > > > > isn't set. So maybe we just want:
> > > > > > 
> > > > > > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> > > > > > index 2201b8c78641..0468c6ee4448 100644
> > > > > > --- a/drivers/video/backlight/pwm_bl.c
> > > > > > +++ b/drivers/video/backlight/pwm_bl.c
> > > > > > @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> > > > > >         if (brightness > 0) {
> > > > > >                 pwm_get_state(pb->pwm, &state);
> > > > > >                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> > > > > > +               state.enabled = true;
> > > > > >                 pwm_apply_state(pb->pwm, &state);
> > > > > >                 pwm_backlight_power_on(pb);
> > > > > >         } else
> > > > > > 
> > > > > > ? On a side note: It's IMHO strange that pwm_backlight_power_on
> > > > > > reconfigures the PWM once more.
> > > > > > 
> > > > > 
> > > > > Looking again to the pwm_bl code, now, I am not sure this is correct (although
> > > > > it probably solves the problem for me).
> > > > 
> > > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> > > > almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> > > > out of both the power on/off functions so the duty-cycle/enable or
> > > > disable can happen in one go within the update_status function. I don't
> > > > think such a change would interfere with the power and enable sequencing
> > > > needed by panels and it would therefore be a nice continuation of the
> > > > work to convert over to the pwm_apply_state() API.
> > > 
> > > OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> > > I'd expect that this alone should already fix your issue.
> > >  
> > > > None of the above has anything to do with what is right or wrong for
> > > > the PWM API evolution. Of course, if this thread does conclude that it
> > > > is OK the duty cycle of a disabled PWM to be retained for some drivers
> > > > and not others then I'd hope to see some WARN_ON()s added to the PWM
> > > > framework to help bring problems to the surface with all drivers.
> > > 
> > > I think it's not possible to add a reliable WARN_ON for that issue. It
> > > is quite expected that .get_state returns something that doesn't
> > > completely match the requested configuration. So if a consumer requests
> > > 
> > > 	.duty_cycle = 1
> > > 	.period = 100000000
> > > 	.enabled = false
> > > 
> > > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> > > that has a concept of duty_cycle for disabled hardware.
> > > 
> > > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
> > 
> > Isn't that intended to help identify "odd" PWM drivers rather than "odd"
> > clients?
> > 
> > Initially I was thinking that a WARN_ON() could be emitted when:
> > 
> > 1. .duty_cycle is non-zero
> > 2. .enabled is false
> > 3. the PWM is not already enabled
> > 
> > (#3 included to avoid too many false positives when disabling a PWM)
> 
> I think I created a patch for that in the past, don't remember the
> details.
> 
> > A poisoning approach might be equally valid. If some drivers are
> > permitted to "round" .duty_cycle to 0 when .enabled is false then the
> > framework could get *all* drivers to behave in the same way by
> > zeroing it out before calling the drivers apply method. It is not that
> > big a deal but minimising the difference between driver behaviour should
> > automatically reduce the difference in API usage by clients.
> 
> I like it, but that breaks consumers that set .duty_cycle once during
> probe and then only do:
> 
> 	pwm_get_state(pwm, &state);
> 	state.enabled = ...
> 	pwm_apply_state(pwm, &state);
> 
> which is a common idiom.

Sorry I must have missed something. That appears to be identical to
what pwm_bl.c currently does, albeit for rather better reasons.

If setting the duty cycle and then separately enabling it is a
reasonable idiom then the cros-ec-pwm driver is a broken implementation
of the API and needs to be fixed regardless of any changes to pwm_bl.c .


Daniel.

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

Hi Uwe, Daniel,

On 9/10/19 13:35, Daniel Thompson wrote:
> On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote:
>> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote:
>>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
>>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
>>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
>>>>>> Hi Uwe,
>>>>>>
>>>>>> Adding Daniel and Lee to the discussion ...
>>>>>
>>>>> Thanks!
>>>>>
>>>>>> On 8/10/19 22:31, Uwe Kleine-König wrote:
>>>>>>> On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
>>>>>>>>> A few thoughts to your approach here ...:
>>>>>>>>>
>>>>>>>>>  - Would it make sense to only store duty_cycle and enabled in the
>>>>>>>>>    driver struct?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yes, in fact, my first approach (that I didn't send) was only storing enabled
>>>>>>>> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
>>>>>>>> guess is not really needed.
>>>>>>>>
>>>>>>>>
>>>>>>>>>  - Which driver is the consumer of your pwm? If I understand correctly
>>>>>>>>>    the following sequence is the bad one:
>>>>>>>>>
>>>>>>>>
>>>>>>>> The consumer is the pwm_bl driver. Actually I'n trying to identify
>>>>>>>> other consumers.
>>>>>>>
>>>>>>
>>>>>> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
>>>>>>
>>>>>>> Ah, I see why I missed to identify the problem back when I checked this
>>>>>>> driver. The problem is not that .duty_cycle isn't set but there .enabled
>>>>>>> isn't set. So maybe we just want:
>>>>>>>
>>>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
>>>>>>> index 2201b8c78641..0468c6ee4448 100644
>>>>>>> --- a/drivers/video/backlight/pwm_bl.c
>>>>>>> +++ b/drivers/video/backlight/pwm_bl.c
>>>>>>> @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>>>>>>>         if (brightness > 0) {
>>>>>>>                 pwm_get_state(pb->pwm, &state);
>>>>>>>                 state.duty_cycle = compute_duty_cycle(pb, brightness);
>>>>>>> +               state.enabled = true;
>>>>>>>                 pwm_apply_state(pb->pwm, &state);
>>>>>>>                 pwm_backlight_power_on(pb);
>>>>>>>         } else
>>>>>>>
>>>>>>> ? On a side note: It's IMHO strange that pwm_backlight_power_on
>>>>>>> reconfigures the PWM once more.
>>>>>>>
>>>>>>
>>>>>> Looking again to the pwm_bl code, now, I am not sure this is correct (although
>>>>>> it probably solves the problem for me).
>>>>>
>>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
>>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable
>>>>> out of both the power on/off functions so the duty-cycle/enable or
>>>>> disable can happen in one go within the update_status function. I don't
>>>>> think such a change would interfere with the power and enable sequencing
>>>>> needed by panels and it would therefore be a nice continuation of the
>>>>> work to convert over to the pwm_apply_state() API.
>>>>
>>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
>>>> I'd expect that this alone should already fix your issue.
>>>>  
>>>>> None of the above has anything to do with what is right or wrong for
>>>>> the PWM API evolution. Of course, if this thread does conclude that it
>>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers
>>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM
>>>>> framework to help bring problems to the surface with all drivers.
>>>>
>>>> I think it's not possible to add a reliable WARN_ON for that issue. It
>>>> is quite expected that .get_state returns something that doesn't
>>>> completely match the requested configuration. So if a consumer requests
>>>>
>>>> 	.duty_cycle = 1
>>>> 	.period = 100000000
>>>> 	.enabled = false
>>>>
>>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
>>>> that has a concept of duty_cycle for disabled hardware.
>>>>
>>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
>>>
>>> Isn't that intended to help identify "odd" PWM drivers rather than "odd"
>>> clients?
>>>
>>> Initially I was thinking that a WARN_ON() could be emitted when:
>>>
>>> 1. .duty_cycle is non-zero
>>> 2. .enabled is false
>>> 3. the PWM is not already enabled
>>>
>>> (#3 included to avoid too many false positives when disabling a PWM)
>>
>> I think I created a patch for that in the past, don't remember the
>> details.
>>
>>> A poisoning approach might be equally valid. If some drivers are
>>> permitted to "round" .duty_cycle to 0 when .enabled is false then the
>>> framework could get *all* drivers to behave in the same way by
>>> zeroing it out before calling the drivers apply method. It is not that
>>> big a deal but minimising the difference between driver behaviour should
>>> automatically reduce the difference in API usage by clients.
>>
>> I like it, but that breaks consumers that set .duty_cycle once during
>> probe and then only do:
>>
>> 	pwm_get_state(pwm, &state);
>> 	state.enabled = ...
>> 	pwm_apply_state(pwm, &state);
>>
>> which is a common idiom.
> 
> Sorry I must have missed something. That appears to be identical to
> what pwm_bl.c currently does, albeit for rather better reasons.
> 
> If setting the duty cycle and then separately enabling it is a
> reasonable idiom then the cros-ec-pwm driver is a broken implementation
> of the API and needs to be fixed regardless of any changes to pwm_bl.c .
> 

I think that Daniel has reason here. Just come to my mind that, if the fix is
not in the cros-ec-pwm driver we will also have the PWM sysfs user space
interface broken for that driver, apart from future consumers.

This turns on to my first approach, this patch. Uwe, You had some complains
regarding is if I can only store enabled and duty_cycle instead of all the
pwm_state, that should also work for me. If I solve that concern, might be this
solution acceptable for you? Do you have more concerns?

Regards,
 Enric

> 
> Daniel.
> 

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:
> Hi Uwe, Daniel,
> 
> On 9/10/19 13:35, Daniel Thompson wrote:
> > On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote:
> >> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote:
> >>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
> >>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> >>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> >>>>>> Hi Uwe,
> >>>>>>
> >>>>>> Adding Daniel and Lee to the discussion ...
> >>>>>
> >>>>> Thanks!
> >>>>>
> >>>>>> On 8/10/19 22:31, Uwe Kleine-König wrote:
> >>>>>>> On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> >>>>>>>>> A few thoughts to your approach here ...:
> >>>>>>>>>
> >>>>>>>>>  - Would it make sense to only store duty_cycle and enabled in the
> >>>>>>>>>    driver struct?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> >>>>>>>> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> >>>>>>>> guess is not really needed.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>  - Which driver is the consumer of your pwm? If I understand correctly
> >>>>>>>>>    the following sequence is the bad one:
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The consumer is the pwm_bl driver. Actually I'n trying to identify
> >>>>>>>> other consumers.
> >>>>>>>
> >>>>>>
> >>>>>> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> >>>>>>
> >>>>>>> Ah, I see why I missed to identify the problem back when I checked this
> >>>>>>> driver. The problem is not that .duty_cycle isn't set but there .enabled
> >>>>>>> isn't set. So maybe we just want:
> >>>>>>>
> >>>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> >>>>>>> index 2201b8c78641..0468c6ee4448 100644
> >>>>>>> --- a/drivers/video/backlight/pwm_bl.c
> >>>>>>> +++ b/drivers/video/backlight/pwm_bl.c
> >>>>>>> @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> >>>>>>>         if (brightness > 0) {
> >>>>>>>                 pwm_get_state(pb->pwm, &state);
> >>>>>>>                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> >>>>>>> +               state.enabled = true;
> >>>>>>>                 pwm_apply_state(pb->pwm, &state);
> >>>>>>>                 pwm_backlight_power_on(pb);
> >>>>>>>         } else
> >>>>>>>
> >>>>>>> ? On a side note: It's IMHO strange that pwm_backlight_power_on
> >>>>>>> reconfigures the PWM once more.
> >>>>>>>
> >>>>>>
> >>>>>> Looking again to the pwm_bl code, now, I am not sure this is correct (although
> >>>>>> it probably solves the problem for me).
> >>>>>
> >>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> >>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> >>>>> out of both the power on/off functions so the duty-cycle/enable or
> >>>>> disable can happen in one go within the update_status function. I don't
> >>>>> think such a change would interfere with the power and enable sequencing
> >>>>> needed by panels and it would therefore be a nice continuation of the
> >>>>> work to convert over to the pwm_apply_state() API.
> >>>>
> >>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> >>>> I'd expect that this alone should already fix your issue.
> >>>>  
> >>>>> None of the above has anything to do with what is right or wrong for
> >>>>> the PWM API evolution. Of course, if this thread does conclude that it
> >>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers
> >>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM
> >>>>> framework to help bring problems to the surface with all drivers.
> >>>>
> >>>> I think it's not possible to add a reliable WARN_ON for that issue. It
> >>>> is quite expected that .get_state returns something that doesn't
> >>>> completely match the requested configuration. So if a consumer requests
> >>>>
> >>>> 	.duty_cycle = 1
> >>>> 	.period = 100000000
> >>>> 	.enabled = false
> >>>>
> >>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> >>>> that has a concept of duty_cycle for disabled hardware.
> >>>>
> >>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
> >>>
> >>> Isn't that intended to help identify "odd" PWM drivers rather than "odd"
> >>> clients?
> >>>
> >>> Initially I was thinking that a WARN_ON() could be emitted when:
> >>>
> >>> 1. .duty_cycle is non-zero
> >>> 2. .enabled is false
> >>> 3. the PWM is not already enabled
> >>>
> >>> (#3 included to avoid too many false positives when disabling a PWM)
> >>
> >> I think I created a patch for that in the past, don't remember the
> >> details.
> >>
> >>> A poisoning approach might be equally valid. If some drivers are
> >>> permitted to "round" .duty_cycle to 0 when .enabled is false then the
> >>> framework could get *all* drivers to behave in the same way by
> >>> zeroing it out before calling the drivers apply method. It is not that
> >>> big a deal but minimising the difference between driver behaviour should
> >>> automatically reduce the difference in API usage by clients.
> >>
> >> I like it, but that breaks consumers that set .duty_cycle once during
> >> probe and then only do:
> >>
> >> 	pwm_get_state(pwm, &state);
> >> 	state.enabled = ...
> >> 	pwm_apply_state(pwm, &state);
> >>
> >> which is a common idiom.
> > 
> > Sorry I must have missed something. That appears to be identical to
> > what pwm_bl.c currently does, albeit for rather better reasons.
> > 
> > If setting the duty cycle and then separately enabling it is a
> > reasonable idiom then the cros-ec-pwm driver is a broken implementation
> > of the API and needs to be fixed regardless of any changes to pwm_bl.c .
> > 
> 
> I think that Daniel has reason here. Just come to my mind that, if the fix is
> not in the cros-ec-pwm driver we will also have the PWM sysfs user space
> interface broken for that driver, apart from future consumers.
> 
> This turns on to my first approach, this patch. Uwe, You had some complains
> regarding is if I can only store enabled and duty_cycle instead of all the
> pwm_state, that should also work for me. If I solve that concern, might be this
> solution acceptable for you? Do you have more concerns?

In my eyes we need a decision if "loosing" the value of .duty_cycle (and
maybe .period) is considered ok. And if it's not we need to decide if we
catch this in the pwm core or in each driver individually. My preference
would be to not do it in each driver. Probably the best option is to do
it in the core with an expressive code comment and then maybe make
callers aware and issue a warning (if possible).

But it's Thierry who has the last word on this.

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Thierry Reding]

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

On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:
> Hi Uwe, Daniel,
> 
> On 9/10/19 13:35, Daniel Thompson wrote:
> > On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote:
> >> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote:
> >>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote:
> >>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote:
> >>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote:
> >>>>>> Hi Uwe,
> >>>>>>
> >>>>>> Adding Daniel and Lee to the discussion ...
> >>>>>
> >>>>> Thanks!
> >>>>>
> >>>>>> On 8/10/19 22:31, Uwe Kleine-König wrote:
> >>>>>>> On Tue, Oct 08, 2019 at 06:33:15PM +0200, Enric Balletbo i Serra wrote:
> >>>>>>>>> A few thoughts to your approach here ...:
> >>>>>>>>>
> >>>>>>>>>  - Would it make sense to only store duty_cycle and enabled in the
> >>>>>>>>>    driver struct?
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Yes, in fact, my first approach (that I didn't send) was only storing enabled
> >>>>>>>> and duty cycle. For some reason I ended storing the full pwm_state struct, but I
> >>>>>>>> guess is not really needed.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>>  - Which driver is the consumer of your pwm? If I understand correctly
> >>>>>>>>>    the following sequence is the bad one:
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> The consumer is the pwm_bl driver. Actually I'n trying to identify
> >>>>>>>> other consumers.
> >>>>>>>
> >>>>>>
> >>>>>> So far, the pwm_bl driver is the only consumer of cros-ec-pwm.
> >>>>>>
> >>>>>>> Ah, I see why I missed to identify the problem back when I checked this
> >>>>>>> driver. The problem is not that .duty_cycle isn't set but there .enabled
> >>>>>>> isn't set. So maybe we just want:
> >>>>>>>
> >>>>>>> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> >>>>>>> index 2201b8c78641..0468c6ee4448 100644
> >>>>>>> --- a/drivers/video/backlight/pwm_bl.c
> >>>>>>> +++ b/drivers/video/backlight/pwm_bl.c
> >>>>>>> @@ -123,6 +123,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
> >>>>>>>         if (brightness > 0) {
> >>>>>>>                 pwm_get_state(pb->pwm, &state);
> >>>>>>>                 state.duty_cycle = compute_duty_cycle(pb, brightness);
> >>>>>>> +               state.enabled = true;
> >>>>>>>                 pwm_apply_state(pb->pwm, &state);
> >>>>>>>                 pwm_backlight_power_on(pb);
> >>>>>>>         } else
> >>>>>>>
> >>>>>>> ? On a side note: It's IMHO strange that pwm_backlight_power_on
> >>>>>>> reconfigures the PWM once more.
> >>>>>>>
> >>>>>>
> >>>>>> Looking again to the pwm_bl code, now, I am not sure this is correct (although
> >>>>>> it probably solves the problem for me).
> >>>>>
> >>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd
> >>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable
> >>>>> out of both the power on/off functions so the duty-cycle/enable or
> >>>>> disable can happen in one go within the update_status function. I don't
> >>>>> think such a change would interfere with the power and enable sequencing
> >>>>> needed by panels and it would therefore be a nice continuation of the
> >>>>> work to convert over to the pwm_apply_state() API.
> >>>>
> >>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl?
> >>>> I'd expect that this alone should already fix your issue.
> >>>>  
> >>>>> None of the above has anything to do with what is right or wrong for
> >>>>> the PWM API evolution. Of course, if this thread does conclude that it
> >>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers
> >>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM
> >>>>> framework to help bring problems to the surface with all drivers.
> >>>>
> >>>> I think it's not possible to add a reliable WARN_ON for that issue. It
> >>>> is quite expected that .get_state returns something that doesn't
> >>>> completely match the requested configuration. So if a consumer requests
> >>>>
> >>>> 	.duty_cycle = 1
> >>>> 	.period = 100000000
> >>>> 	.enabled = false
> >>>>
> >>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware
> >>>> that has a concept of duty_cycle for disabled hardware.
> >>>>
> >>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/.
> >>>
> >>> Isn't that intended to help identify "odd" PWM drivers rather than "odd"
> >>> clients?
> >>>
> >>> Initially I was thinking that a WARN_ON() could be emitted when:
> >>>
> >>> 1. .duty_cycle is non-zero
> >>> 2. .enabled is false
> >>> 3. the PWM is not already enabled
> >>>
> >>> (#3 included to avoid too many false positives when disabling a PWM)
> >>
> >> I think I created a patch for that in the past, don't remember the
> >> details.
> >>
> >>> A poisoning approach might be equally valid. If some drivers are
> >>> permitted to "round" .duty_cycle to 0 when .enabled is false then the
> >>> framework could get *all* drivers to behave in the same way by
> >>> zeroing it out before calling the drivers apply method. It is not that
> >>> big a deal but minimising the difference between driver behaviour should
> >>> automatically reduce the difference in API usage by clients.
> >>
> >> I like it, but that breaks consumers that set .duty_cycle once during
> >> probe and then only do:
> >>
> >> 	pwm_get_state(pwm, &state);
> >> 	state.enabled = ...
> >> 	pwm_apply_state(pwm, &state);
> >>
> >> which is a common idiom.
> > 
> > Sorry I must have missed something. That appears to be identical to
> > what pwm_bl.c currently does, albeit for rather better reasons.
> > 
> > If setting the duty cycle and then separately enabling it is a
> > reasonable idiom then the cros-ec-pwm driver is a broken implementation
> > of the API and needs to be fixed regardless of any changes to pwm_bl.c .
> > 
> 
> I think that Daniel has reason here. Just come to my mind that, if the fix is
> not in the cros-ec-pwm driver we will also have the PWM sysfs user space
> interface broken for that driver, apart from future consumers.
> 
> This turns on to my first approach, this patch. Uwe, You had some complains
> regarding is if I can only store enabled and duty_cycle instead of all the
> pwm_state, that should also work for me. If I solve that concern, might be this
> solution acceptable for you? Do you have more concerns?

There's been a similar report of pwm-backlight getting confused with the
i.MX PWM driver. What ChromeOS EC and i.MX drivers both have in common
is that they return a duty cycle of 0 when the PWM is off. For i.MX, the
patch I propose caches the duty cycle and uses the cached value when
appropriate. I think a similar patch (see below) would work for cros-ec.

Can somebody give that a quick test to see whether it fixes the issue on
ChromeOS hardware?

Thierry

--- >8 ---
From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
From: Thierry Reding <thierry.reding@gmail.com>
Date: Thu, 17 Oct 2019 13:21:15 +0200
Subject: [PATCH] pwm: cros-ec: Cache duty cycle value

The ChromeOS embedded controller doesn't differentiate between disabled
and duty cycle being 0. In order not to potentially confuse consumers,
cache the duty cycle and return the cached value instead of the real
value when the PWM is disabled.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
 drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 89497448d217..09c08dee099e 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
 	struct pwm_chip chip;
 };
 
+/**
+ * struct cros_ec_pwm - per-PWM driver data
+ * @duty_cycle: cached duty cycle
+ */
+struct cros_ec_pwm {
+	u16 duty_cycle;
+};
+
 static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
 {
 	return container_of(c, struct cros_ec_pwm_device, chip);
 }
 
+static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct cros_ec_pwm *channel;
+
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
+	if (!channel)
+		return -ENOMEM;
+
+	pwm_set_chip_data(pwm, channel);
+
+	return 0;
+}
+
+static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
+
+	kfree(channel);
+}
+
 static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
 {
 	struct {
@@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			     const struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
-	int duty_cycle;
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
+	u16 duty_cycle;
+	int ret;
 
 	/* The EC won't let us change the period */
 	if (state->period != EC_PWM_MAX_DUTY)
@@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 */
 	duty_cycle = state->enabled ? state->duty_cycle : 0;
 
-	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
+	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
+	if (ret < 0)
+		return ret;
+
+	channel->duty_cycle = state->duty_cycle;
+
+	return 0;
 }
 
 static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 				  struct pwm_state *state)
 {
 	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
+	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
 	int ret;
 
 	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
@@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 	state->enabled = (ret > 0);
 	state->period = EC_PWM_MAX_DUTY;
 
-	/* Note that "disabled" and "duty cycle == 0" are treated the same */
-	state->duty_cycle = ret;
+	/*
+	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
+	 * the cached duty cycle is not zero, used the cached duty cycle. This
+	 * ensures that the configured duty cycle is kept across a disable and
+	 * enable operation and avoids potentially confusing consumers.
+	 *
+	 * For the case of the initial hardware readout, channel->duty_cycle
+	 * will be 0 and the actual duty cycle read from the EC is used.
+	 */
+	if (ret == 0 && channel->duty_cycle > 0)
+		state->duty_cycle = channel->duty_cycle;
+	else
+		state->duty_cycle = ret;
 }
 
 static struct pwm_device *
@@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 }
 
 static const struct pwm_ops cros_ec_pwm_ops = {
+	.request = cros_ec_pwm_request,
+	.free = cros_ec_pwm_free,
 	.get_state	= cros_ec_pwm_get_state,
 	.apply		= cros_ec_pwm_apply,
 	.owner		= THIS_MODULE,
-- 
2.23.0


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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Enric Balletbo i Serra]

Hi Thierry,
On 17/10/19 13:35, Thierry Reding wrote:
> On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:

[snip]

> 
> --- >8 ---
> From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Thu, 17 Oct 2019 13:21:15 +0200
> Subject: [PATCH] pwm: cros-ec: Cache duty cycle value
> 
> The ChromeOS embedded controller doesn't differentiate between disabled
> and duty cycle being 0. In order not to potentially confuse consumers,
> cache the duty cycle and return the cached value instead of the real
> value when the PWM is disabled.
> 
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
>  drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
>  1 file changed, 54 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 89497448d217..09c08dee099e 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
>  	struct pwm_chip chip;
>  };
>  
> +/**
> + * struct cros_ec_pwm - per-PWM driver data
> + * @duty_cycle: cached duty cycle
> + */
> +struct cros_ec_pwm {
> +	u16 duty_cycle;
> +};
> +
>  static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
>  {
>  	return container_of(c, struct cros_ec_pwm_device, chip);
>  }
>  
> +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct cros_ec_pwm *channel;
> +
> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> +	if (!channel)
> +		return -ENOMEM;
> +
> +	pwm_set_chip_data(pwm, channel);
> +
> +	return 0;
> +}
> +
> +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> +
> +	kfree(channel);
> +}
> +
>  static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
>  {
>  	struct {
> @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			     const struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> -	int duty_cycle;
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> +	u16 duty_cycle;
> +	int ret;
>  
>  	/* The EC won't let us change the period */
>  	if (state->period != EC_PWM_MAX_DUTY)
> @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	duty_cycle = state->enabled ? state->duty_cycle : 0;
>  
> -	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> +	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> +	if (ret < 0)
> +		return ret;
> +
> +	channel->duty_cycle = state->duty_cycle;
> +
> +	return 0;
>  }
>  
>  static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  				  struct pwm_state *state)
>  {
>  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
>  	int ret;
>  
>  	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>  	state->enabled = (ret > 0);
>  	state->period = EC_PWM_MAX_DUTY;
>  
> -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> -	state->duty_cycle = ret;
> +	/*
> +	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
> +	 * the cached duty cycle is not zero, used the cached duty cycle. This
> +	 * ensures that the configured duty cycle is kept across a disable and
> +	 * enable operation and avoids potentially confusing consumers.
> +	 *
> +	 * For the case of the initial hardware readout, channel->duty_cycle
> +	 * will be 0 and the actual duty cycle read from the EC is used.
> +	 */
> +	if (ret == 0 && channel->duty_cycle > 0)
> +		state->duty_cycle = channel->duty_cycle;
> +	else
> +		state->duty_cycle = ret;
>  }
>  
>  static struct pwm_device *
> @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
>  }
>  
>  static const struct pwm_ops cros_ec_pwm_ops = {
> +	.request = cros_ec_pwm_request,
> +	.free = cros_ec_pwm_free,
>  	.get_state	= cros_ec_pwm_get_state,
>  	.apply		= cros_ec_pwm_apply,
>  	.owner		= THIS_MODULE,
> 

I just tried your approach but I got a NULL pointer dereference while probe. I
am just back from a week off but I'll be able to dig into it between today and
tomorrow, just wanted to let you know that the patch doesn't works as is for me.

[   10.128455] Unable to handle kernel NULL pointer dereference at virtual
address 0000000000000000
[   10.141895] Mem abort info:

[   10.145090]   ESR = 0x96000004

[   10.148537]   EC = 0x25: DABT (current EL), IL = 32 bits

[   10.154560]   SET = 0, FnV = 0

[   10.157986]   EA = 0, S1PTW = 0

[   10.161548] Data abort info:

[   10.164804]   ISV = 0, ISS = 0x00000004

[   10.169111]   CM = 0, WnR = 0

[   10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000

[   10.179660] [0000000000000000] pgd=0000000000000000

[   10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP

[   10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+)
rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_
tables ipv6 nf_defrag_ipv6

[   10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230

[   10.179696] Hardware name: Google Kevin (DT)

[   10.179700] pstate: 60000005 (nZCv daif -PAN -UAO)

[   10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]

[   10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec]

[   10.247829] sp : ffff800012433810

[   10.251531] x29: ffff800012433810 x28: 0000000200000026

[   10.257469] x27: ffff800012433942 x26: ffff0000ef075010

[   10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30

[   10.269341] x23: 0000000000000000 x22: ffff0000ee273190
[   10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270
[   10.281214] x19: ffff800011bc98c8 x18: 0000000000000003
[   10.287150] x17: 0000000000000007 x16: 000000000000000e
[   10.293088] x15: 0000000000000001 x14: 0000000000000019
[   10.299024] x13: 0000000000000033 x12: 0000000000000018
[   10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0
[   10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA
Objects: 31
[   10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830
[   10.310905] x7 : 000000000000011b x6 : 0000000000000001
[   10.310908] x5 : 0000000000000000 x4 : 0000000000000000
[   10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002
[   10.310914] x1 : 0000000000000000 x0 : 0000000000000000
[   10.310918] Call trace:
[   10.310931]  cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
[   10.310944]  pwmchip_add_with_polarity+0x134/0x290
[   10.363576]  pwmchip_add+0x24/0x30
[   10.367383]  cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec]
[   10.373325]  platform_drv_probe+0x58/0xa8
[   10.377809]  really_probe+0xe0/0x318
[   10.381804]  driver_probe_device+0x5c/0xf0
[   10.386381]  device_driver_attach+0x74/0x80

Thanks,
 Enric

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Uwe Kleine-König]

On Mon, Oct 21, 2019 at 11:42:05AM +0200, Enric Balletbo i Serra wrote:
> Hi Thierry,
> On 17/10/19 13:35, Thierry Reding wrote:
> > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:
> 
> [snip]
> 
> > 
> > --- >8 ---
> > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Date: Thu, 17 Oct 2019 13:21:15 +0200
> > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value
> > 
> > The ChromeOS embedded controller doesn't differentiate between disabled
> > and duty cycle being 0. In order not to potentially confuse consumers,
> > cache the duty cycle and return the cached value instead of the real
> > value when the PWM is disabled.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> > index 89497448d217..09c08dee099e 100644
> > --- a/drivers/pwm/pwm-cros-ec.c
> > +++ b/drivers/pwm/pwm-cros-ec.c
> > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
> >  	struct pwm_chip chip;
> >  };
> >  
> > +/**
> > + * struct cros_ec_pwm - per-PWM driver data
> > + * @duty_cycle: cached duty cycle
> > + */
> > +struct cros_ec_pwm {
> > +	u16 duty_cycle;
> > +};
> > +
> >  static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
> >  {
> >  	return container_of(c, struct cros_ec_pwm_device, chip);
> >  }
> >  
> > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct cros_ec_pwm *channel;
> > +
> > +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > +	if (!channel)
> > +		return -ENOMEM;
> > +
> > +	pwm_set_chip_data(pwm, channel);
> > +
> > +	return 0;
> > +}
> > +
> > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > +
> > +	kfree(channel);
> > +}
> > +
> >  static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
> >  {
> >  	struct {
> > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			     const struct pwm_state *state)
> >  {
> >  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > -	int duty_cycle;
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > +	u16 duty_cycle;
> > +	int ret;
> >  
> >  	/* The EC won't let us change the period */
> >  	if (state->period != EC_PWM_MAX_DUTY)
> > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 */
> >  	duty_cycle = state->enabled ? state->duty_cycle : 0;
> >  
> > -	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > +	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	channel->duty_cycle = state->duty_cycle;
> > +
> > +	return 0;
> >  }
> >  
> >  static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  				  struct pwm_state *state)
> >  {
> >  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> >  	int ret;
> >  
> >  	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	state->enabled = (ret > 0);
> >  	state->period = EC_PWM_MAX_DUTY;
> >  
> > -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> > -	state->duty_cycle = ret;
> > +	/*
> > +	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
> > +	 * the cached duty cycle is not zero, used the cached duty cycle. This
> > +	 * ensures that the configured duty cycle is kept across a disable and
> > +	 * enable operation and avoids potentially confusing consumers.
> > +	 *
> > +	 * For the case of the initial hardware readout, channel->duty_cycle
> > +	 * will be 0 and the actual duty cycle read from the EC is used.
> > +	 */
> > +	if (ret == 0 && channel->duty_cycle > 0)
> > +		state->duty_cycle = channel->duty_cycle;
> > +	else
> > +		state->duty_cycle = ret;
> >  }
> >  
> >  static struct pwm_device *
> > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> >  }
> >  
> >  static const struct pwm_ops cros_ec_pwm_ops = {
> > +	.request = cros_ec_pwm_request,
> > +	.free = cros_ec_pwm_free,
> >  	.get_state	= cros_ec_pwm_get_state,
> >  	.apply		= cros_ec_pwm_apply,
> >  	.owner		= THIS_MODULE,
> > 
> 
> I just tried your approach but I got a NULL pointer dereference while probe. I
> am just back from a week off but I'll be able to dig into it between today and
> tomorrow, just wanted to let you know that the patch doesn't works as is for me.
> 
> [   10.128455] Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
> [   10.141895] Mem abort info:
> 
> [   10.145090]   ESR = 0x96000004
> 
> [   10.148537]   EC = 0x25: DABT (current EL), IL = 32 bits
> 
> [   10.154560]   SET = 0, FnV = 0
> 
> [   10.157986]   EA = 0, S1PTW = 0
> 
> [   10.161548] Data abort info:
> 
> [   10.164804]   ISV = 0, ISS = 0x00000004
> 
> [   10.169111]   CM = 0, WnR = 0
> 
> [   10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000
> 
> [   10.179660] [0000000000000000] pgd=0000000000000000
> 
> [   10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> 
> [   10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+)
> rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_
> tables ipv6 nf_defrag_ipv6
> 
> [   10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230
> 
> [   10.179696] Hardware name: Google Kevin (DT)
> 
> [   10.179700] pstate: 60000005 (nZCv daif -PAN -UAO)
> 
> [   10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
> 
> [   10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec]
> 
> [   10.247829] sp : ffff800012433810
> 
> [   10.251531] x29: ffff800012433810 x28: 0000000200000026
> 
> [   10.257469] x27: ffff800012433942 x26: ffff0000ef075010
> 
> [   10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30
> 
> [   10.269341] x23: 0000000000000000 x22: ffff0000ee273190
> [   10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270
> [   10.281214] x19: ffff800011bc98c8 x18: 0000000000000003
> [   10.287150] x17: 0000000000000007 x16: 000000000000000e
> [   10.293088] x15: 0000000000000001 x14: 0000000000000019
> [   10.299024] x13: 0000000000000033 x12: 0000000000000018
> [   10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0
> [   10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA
> Objects: 31
> [   10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830
> [   10.310905] x7 : 000000000000011b x6 : 0000000000000001
> [   10.310908] x5 : 0000000000000000 x4 : 0000000000000000
> [   10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002
> [   10.310914] x1 : 0000000000000000 x0 : 0000000000000000
> [   10.310918] Call trace:
> [   10.310931]  cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
> [   10.310944]  pwmchip_add_with_polarity+0x134/0x290

The problem is that .get_state is called from the core without
requesting the pwm and so .request wasn't called before.

I still think it's wrong to cache the duty-cycle in the driver, but if
you still want to do it, .duty_cycle must go into the driver struct
which however is very inefficient given that probably most of the
machines don't actually have 255 PWMs.

Best regards
Uwe

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

Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Thierry Reding]

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

On Mon, Oct 21, 2019 at 11:42:05AM +0200, Enric Balletbo i Serra wrote:
> Hi Thierry,
> On 17/10/19 13:35, Thierry Reding wrote:
> > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote:
> 
> [snip]
> 
> > 
> > --- >8 ---
> > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Date: Thu, 17 Oct 2019 13:21:15 +0200
> > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value
> > 
> > The ChromeOS embedded controller doesn't differentiate between disabled
> > and duty cycle being 0. In order not to potentially confuse consumers,
> > cache the duty cycle and return the cached value instead of the real
> > value when the PWM is disabled.
> > 
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> >  drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 54 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> > index 89497448d217..09c08dee099e 100644
> > --- a/drivers/pwm/pwm-cros-ec.c
> > +++ b/drivers/pwm/pwm-cros-ec.c
> > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device {
> >  	struct pwm_chip chip;
> >  };
> >  
> > +/**
> > + * struct cros_ec_pwm - per-PWM driver data
> > + * @duty_cycle: cached duty cycle
> > + */
> > +struct cros_ec_pwm {
> > +	u16 duty_cycle;
> > +};
> > +
> >  static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c)
> >  {
> >  	return container_of(c, struct cros_ec_pwm_device, chip);
> >  }
> >  
> > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct cros_ec_pwm *channel;
> > +
> > +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > +	if (!channel)
> > +		return -ENOMEM;
> > +
> > +	pwm_set_chip_data(pwm, channel);
> > +
> > +	return 0;
> > +}
> > +
> > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
> > +{
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > +
> > +	kfree(channel);
> > +}
> > +
> >  static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty)
> >  {
> >  	struct {
> > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			     const struct pwm_state *state)
> >  {
> >  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > -	int duty_cycle;
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> > +	u16 duty_cycle;
> > +	int ret;
> >  
> >  	/* The EC won't let us change the period */
> >  	if (state->period != EC_PWM_MAX_DUTY)
> > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	 */
> >  	duty_cycle = state->enabled ? state->duty_cycle : 0;
> >  
> > -	return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > +	ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	channel->duty_cycle = state->duty_cycle;
> > +
> > +	return 0;
> >  }
> >  
> >  static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  				  struct pwm_state *state)
> >  {
> >  	struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip);
> > +	struct cros_ec_pwm *channel = pwm_get_chip_data(pwm);
> >  	int ret;
> >  
> >  	ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm);
> > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	state->enabled = (ret > 0);
> >  	state->period = EC_PWM_MAX_DUTY;
> >  
> > -	/* Note that "disabled" and "duty cycle == 0" are treated the same */
> > -	state->duty_cycle = ret;
> > +	/*
> > +	 * Note that "disabled" and "duty cycle == 0" are treated the same. If
> > +	 * the cached duty cycle is not zero, used the cached duty cycle. This
> > +	 * ensures that the configured duty cycle is kept across a disable and
> > +	 * enable operation and avoids potentially confusing consumers.
> > +	 *
> > +	 * For the case of the initial hardware readout, channel->duty_cycle
> > +	 * will be 0 and the actual duty cycle read from the EC is used.
> > +	 */
> > +	if (ret == 0 && channel->duty_cycle > 0)
> > +		state->duty_cycle = channel->duty_cycle;
> > +	else
> > +		state->duty_cycle = ret;
> >  }
> >  
> >  static struct pwm_device *
> > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
> >  }
> >  
> >  static const struct pwm_ops cros_ec_pwm_ops = {
> > +	.request = cros_ec_pwm_request,
> > +	.free = cros_ec_pwm_free,
> >  	.get_state	= cros_ec_pwm_get_state,
> >  	.apply		= cros_ec_pwm_apply,
> >  	.owner		= THIS_MODULE,
> > 
> 
> I just tried your approach but I got a NULL pointer dereference while probe. I
> am just back from a week off but I'll be able to dig into it between today and
> tomorrow, just wanted to let you know that the patch doesn't works as is for me.
> 
> [   10.128455] Unable to handle kernel NULL pointer dereference at virtual
> address 0000000000000000
> [   10.141895] Mem abort info:
> 
> [   10.145090]   ESR = 0x96000004
> 
> [   10.148537]   EC = 0x25: DABT (current EL), IL = 32 bits
> 
> [   10.154560]   SET = 0, FnV = 0
> 
> [   10.157986]   EA = 0, S1PTW = 0
> 
> [   10.161548] Data abort info:
> 
> [   10.164804]   ISV = 0, ISS = 0x00000004
> 
> [   10.169111]   CM = 0, WnR = 0
> 
> [   10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000
> 
> [   10.179660] [0000000000000000] pgd=0000000000000000
> 
> [   10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP
> 
> [   10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+)
> rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_
> tables ipv6 nf_defrag_ipv6
> 
> [   10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230
> 
> [   10.179696] Hardware name: Google Kevin (DT)
> 
> [   10.179700] pstate: 60000005 (nZCv daif -PAN -UAO)
> 
> [   10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
> 
> [   10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec]
> 
> [   10.247829] sp : ffff800012433810
> 
> [   10.251531] x29: ffff800012433810 x28: 0000000200000026
> 
> [   10.257469] x27: ffff800012433942 x26: ffff0000ef075010
> 
> [   10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30
> 
> [   10.269341] x23: 0000000000000000 x22: ffff0000ee273190
> [   10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270
> [   10.281214] x19: ffff800011bc98c8 x18: 0000000000000003
> [   10.287150] x17: 0000000000000007 x16: 000000000000000e
> [   10.293088] x15: 0000000000000001 x14: 0000000000000019
> [   10.299024] x13: 0000000000000033 x12: 0000000000000018
> [   10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0
> [   10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA
> Objects: 31
> [   10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830
> [   10.310905] x7 : 000000000000011b x6 : 0000000000000001
> [   10.310908] x5 : 0000000000000000 x4 : 0000000000000000
> [   10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002
> [   10.310914] x1 : 0000000000000000 x0 : 0000000000000000
> [   10.310918] Call trace:
> [   10.310931]  cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec]
> [   10.310944]  pwmchip_add_with_polarity+0x134/0x290
> [   10.363576]  pwmchip_add+0x24/0x30
> [   10.367383]  cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec]
> [   10.373325]  platform_drv_probe+0x58/0xa8
> [   10.377809]  really_probe+0xe0/0x318
> [   10.381804]  driver_probe_device+0x5c/0xf0
> [   10.386381]  device_driver_attach+0x74/0x80

Okay, this is caused by ->get_state() getting called during
pwmchip_add_with_polarity(), which is before any of the PWMs can have
been requested, which means ->request() has not been called yet and
therefore the per-PWM state doesn't exist yet. There's a few ways to
work around that (one would be to store the state within the per-chip
structure (rather than per-PWM, another would be to defer calling the
->get_state() callback when the PWM is requested).

Either way, given that we've got a number of other drivers with similar
problems, I think at this point it's better to revert for now in v5.4
and if we do want to get this functionality back we need to start work
on fixing up the problematic drivers.

Thierry

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