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
>
