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/  |