Messages in this thread | | | Subject | Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state | From | Enric Balletbo i Serra <> | Date | Tue, 8 Oct 2019 18:33:15 +0200 |
| |
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 >
| |