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

[Enric Balletbo i Serra <enric.balletbo@collabora.com>]

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
>