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 | Wed, 9 Oct 2019 11:27:13 +0200 |
| |
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 >
| |