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