| Message ID | 20191008105417.16132-1-enric.balletbo@collabora.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state | expand |
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
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 >
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. 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. > > 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. > > - 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
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 >
On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > Hi Uwe, > > Adding Daniel and Lee to the discussion ... Thanks! > 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: > >>> 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). Looking at the pwm_bl code I wouldn't accept the above as it is but I'd almost certainly accept a patch to pwm_bl to move the PWM enable/disable out of both the power on/off functions so the duty-cycle/enable or disable can happen in one go within the update_status function. I don't think such a change would interfere with the power and enable sequencing needed by panels and it would therefore be a nice continuation of the work to convert over to the pwm_apply_state() API. None of the above has anything to do with what is right or wrong for the PWM API evolution. Of course, if this thread does conclude that it is OK the duty cycle of a disabled PWM to be retained for some drivers and not others then I'd hope to see some WARN_ON()s added to the PWM framework to help bring problems to the surface with all drivers. Daniel.
On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > > Hi Uwe, > > > > Adding Daniel and Lee to the discussion ... > > Thanks! > > > 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: > > >>> 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). > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > almost certainly accept a patch to pwm_bl to move the PWM enable/disable > out of both the power on/off functions so the duty-cycle/enable or > disable can happen in one go within the update_status function. I don't > think such a change would interfere with the power and enable sequencing > needed by panels and it would therefore be a nice continuation of the > work to convert over to the pwm_apply_state() API. OK for me. Enric, do you care enough to come up with a patch for pwm_bl? I'd expect that this alone should already fix your issue. > None of the above has anything to do with what is right or wrong for > the PWM API evolution. Of course, if this thread does conclude that it > is OK the duty cycle of a disabled PWM to be retained for some drivers > and not others then I'd hope to see some WARN_ON()s added to the PWM > framework to help bring problems to the surface with all drivers. I think it's not possible to add a reliable WARN_ON for that issue. It is quite expected that .get_state returns something that doesn't completely match the requested configuration. So if a consumer requests .duty_cycle = 1 .period = 100000000 .enabled = false pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware that has a concept of duty_cycle for disabled hardware. A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. Best regards Uwe
On 9/10/19 12:16, Uwe Kleine-König wrote: > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: >> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: >>> Hi Uwe, >>> >>> Adding Daniel and Lee to the discussion ... >> >> Thanks! >> >>> 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: >>>>>> 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). >> >> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd >> almost certainly accept a patch to pwm_bl to move the PWM enable/disable >> out of both the power on/off functions so the duty-cycle/enable or >> disable can happen in one go within the update_status function. I don't >> think such a change would interfere with the power and enable sequencing >> needed by panels and it would therefore be a nice continuation of the >> work to convert over to the pwm_apply_state() API. > > OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > I'd expect that this alone should already fix your issue. > Yes, I'll work on a proposal and send. Thanks you all. Regards, Enric >> None of the above has anything to do with what is right or wrong for >> the PWM API evolution. Of course, if this thread does conclude that it >> is OK the duty cycle of a disabled PWM to be retained for some drivers >> and not others then I'd hope to see some WARN_ON()s added to the PWM >> framework to help bring problems to the surface with all drivers. > > I think it's not possible to add a reliable WARN_ON for that issue. It > is quite expected that .get_state returns something that doesn't > completely match the requested configuration. So if a consumer requests > > .duty_cycle = 1 > .period = 100000000 > .enabled = false > > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > that has a concept of duty_cycle for disabled hardware. > > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. > > Best regards > Uwe >
On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > > > Hi Uwe, > > > > > > Adding Daniel and Lee to the discussion ... > > > > Thanks! > > > > > 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: > > > >>> 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). > > > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > > almost certainly accept a patch to pwm_bl to move the PWM enable/disable > > out of both the power on/off functions so the duty-cycle/enable or > > disable can happen in one go within the update_status function. I don't > > think such a change would interfere with the power and enable sequencing > > needed by panels and it would therefore be a nice continuation of the > > work to convert over to the pwm_apply_state() API. > > OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > I'd expect that this alone should already fix your issue. > > > None of the above has anything to do with what is right or wrong for > > the PWM API evolution. Of course, if this thread does conclude that it > > is OK the duty cycle of a disabled PWM to be retained for some drivers > > and not others then I'd hope to see some WARN_ON()s added to the PWM > > framework to help bring problems to the surface with all drivers. > > I think it's not possible to add a reliable WARN_ON for that issue. It > is quite expected that .get_state returns something that doesn't > completely match the requested configuration. So if a consumer requests > > .duty_cycle = 1 > .period = 100000000 > .enabled = false > > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > that has a concept of duty_cycle for disabled hardware. > > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. Isn't that intended to help identify "odd" PWM drivers rather than "odd" clients? Initially I was thinking that a WARN_ON() could be emitted when: 1. .duty_cycle is non-zero 2. .enabled is false 3. the PWM is not already enabled (#3 included to avoid too many false positives when disabling a PWM) A poisoning approach might be equally valid. If some drivers are permitted to "round" .duty_cycle to 0 when .enabled is false then the framework could get *all* drivers to behave in the same way by zeroing it out before calling the drivers apply method. It is not that big a deal but minimising the difference between driver behaviour should automatically reduce the difference in API usage by clients. Daniel.
On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote: > On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: > > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > > > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > > > > Hi Uwe, > > > > > > > > Adding Daniel and Lee to the discussion ... > > > > > > Thanks! > > > > > > > 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: > > > > >>> 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). > > > > > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > > > almost certainly accept a patch to pwm_bl to move the PWM enable/disable > > > out of both the power on/off functions so the duty-cycle/enable or > > > disable can happen in one go within the update_status function. I don't > > > think such a change would interfere with the power and enable sequencing > > > needed by panels and it would therefore be a nice continuation of the > > > work to convert over to the pwm_apply_state() API. > > > > OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > > I'd expect that this alone should already fix your issue. > > > > > None of the above has anything to do with what is right or wrong for > > > the PWM API evolution. Of course, if this thread does conclude that it > > > is OK the duty cycle of a disabled PWM to be retained for some drivers > > > and not others then I'd hope to see some WARN_ON()s added to the PWM > > > framework to help bring problems to the surface with all drivers. > > > > I think it's not possible to add a reliable WARN_ON for that issue. It > > is quite expected that .get_state returns something that doesn't > > completely match the requested configuration. So if a consumer requests > > > > .duty_cycle = 1 > > .period = 100000000 > > .enabled = false > > > > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > > that has a concept of duty_cycle for disabled hardware. > > > > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. > > Isn't that intended to help identify "odd" PWM drivers rather than "odd" > clients? > > Initially I was thinking that a WARN_ON() could be emitted when: > > 1. .duty_cycle is non-zero > 2. .enabled is false > 3. the PWM is not already enabled > > (#3 included to avoid too many false positives when disabling a PWM) I think I created a patch for that in the past, don't remember the details. > A poisoning approach might be equally valid. If some drivers are > permitted to "round" .duty_cycle to 0 when .enabled is false then the > framework could get *all* drivers to behave in the same way by > zeroing it out before calling the drivers apply method. It is not that > big a deal but minimising the difference between driver behaviour should > automatically reduce the difference in API usage by clients. I like it, but that breaks consumers that set .duty_cycle once during probe and then only do: pwm_get_state(pwm, &state); state.enabled = ... pwm_apply_state(pwm, &state); which is a common idiom. Best regards Uwe
On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote: > On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote: > > On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: > > > On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > > > > On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > > > > > Hi Uwe, > > > > > > > > > > Adding Daniel and Lee to the discussion ... > > > > > > > > Thanks! > > > > > > > > > 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: > > > > > >>> 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). > > > > > > > > Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > > > > almost certainly accept a patch to pwm_bl to move the PWM enable/disable > > > > out of both the power on/off functions so the duty-cycle/enable or > > > > disable can happen in one go within the update_status function. I don't > > > > think such a change would interfere with the power and enable sequencing > > > > needed by panels and it would therefore be a nice continuation of the > > > > work to convert over to the pwm_apply_state() API. > > > > > > OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > > > I'd expect that this alone should already fix your issue. > > > > > > > None of the above has anything to do with what is right or wrong for > > > > the PWM API evolution. Of course, if this thread does conclude that it > > > > is OK the duty cycle of a disabled PWM to be retained for some drivers > > > > and not others then I'd hope to see some WARN_ON()s added to the PWM > > > > framework to help bring problems to the surface with all drivers. > > > > > > I think it's not possible to add a reliable WARN_ON for that issue. It > > > is quite expected that .get_state returns something that doesn't > > > completely match the requested configuration. So if a consumer requests > > > > > > .duty_cycle = 1 > > > .period = 100000000 > > > .enabled = false > > > > > > pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > > > that has a concept of duty_cycle for disabled hardware. > > > > > > A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. > > > > Isn't that intended to help identify "odd" PWM drivers rather than "odd" > > clients? > > > > Initially I was thinking that a WARN_ON() could be emitted when: > > > > 1. .duty_cycle is non-zero > > 2. .enabled is false > > 3. the PWM is not already enabled > > > > (#3 included to avoid too many false positives when disabling a PWM) > > I think I created a patch for that in the past, don't remember the > details. > > > A poisoning approach might be equally valid. If some drivers are > > permitted to "round" .duty_cycle to 0 when .enabled is false then the > > framework could get *all* drivers to behave in the same way by > > zeroing it out before calling the drivers apply method. It is not that > > big a deal but minimising the difference between driver behaviour should > > automatically reduce the difference in API usage by clients. > > I like it, but that breaks consumers that set .duty_cycle once during > probe and then only do: > > pwm_get_state(pwm, &state); > state.enabled = ... > pwm_apply_state(pwm, &state); > > which is a common idiom. Sorry I must have missed something. That appears to be identical to what pwm_bl.c currently does, albeit for rather better reasons. If setting the duty cycle and then separately enabling it is a reasonable idiom then the cros-ec-pwm driver is a broken implementation of the API and needs to be fixed regardless of any changes to pwm_bl.c . Daniel.
Hi Uwe, Daniel, On 9/10/19 13:35, Daniel Thompson wrote: > On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote: >> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote: >>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: >>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: >>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: >>>>>> Hi Uwe, >>>>>> >>>>>> Adding Daniel and Lee to the discussion ... >>>>> >>>>> Thanks! >>>>> >>>>>> 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: >>>>>>>>> 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). >>>>> >>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd >>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable >>>>> out of both the power on/off functions so the duty-cycle/enable or >>>>> disable can happen in one go within the update_status function. I don't >>>>> think such a change would interfere with the power and enable sequencing >>>>> needed by panels and it would therefore be a nice continuation of the >>>>> work to convert over to the pwm_apply_state() API. >>>> >>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl? >>>> I'd expect that this alone should already fix your issue. >>>> >>>>> None of the above has anything to do with what is right or wrong for >>>>> the PWM API evolution. Of course, if this thread does conclude that it >>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers >>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM >>>>> framework to help bring problems to the surface with all drivers. >>>> >>>> I think it's not possible to add a reliable WARN_ON for that issue. It >>>> is quite expected that .get_state returns something that doesn't >>>> completely match the requested configuration. So if a consumer requests >>>> >>>> .duty_cycle = 1 >>>> .period = 100000000 >>>> .enabled = false >>>> >>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware >>>> that has a concept of duty_cycle for disabled hardware. >>>> >>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. >>> >>> Isn't that intended to help identify "odd" PWM drivers rather than "odd" >>> clients? >>> >>> Initially I was thinking that a WARN_ON() could be emitted when: >>> >>> 1. .duty_cycle is non-zero >>> 2. .enabled is false >>> 3. the PWM is not already enabled >>> >>> (#3 included to avoid too many false positives when disabling a PWM) >> >> I think I created a patch for that in the past, don't remember the >> details. >> >>> A poisoning approach might be equally valid. If some drivers are >>> permitted to "round" .duty_cycle to 0 when .enabled is false then the >>> framework could get *all* drivers to behave in the same way by >>> zeroing it out before calling the drivers apply method. It is not that >>> big a deal but minimising the difference between driver behaviour should >>> automatically reduce the difference in API usage by clients. >> >> I like it, but that breaks consumers that set .duty_cycle once during >> probe and then only do: >> >> pwm_get_state(pwm, &state); >> state.enabled = ... >> pwm_apply_state(pwm, &state); >> >> which is a common idiom. > > Sorry I must have missed something. That appears to be identical to > what pwm_bl.c currently does, albeit for rather better reasons. > > If setting the duty cycle and then separately enabling it is a > reasonable idiom then the cros-ec-pwm driver is a broken implementation > of the API and needs to be fixed regardless of any changes to pwm_bl.c . > I think that Daniel has reason here. Just come to my mind that, if the fix is not in the cros-ec-pwm driver we will also have the PWM sysfs user space interface broken for that driver, apart from future consumers. This turns on to my first approach, this patch. Uwe, You had some complains regarding is if I can only store enabled and duty_cycle instead of all the pwm_state, that should also work for me. If I solve that concern, might be this solution acceptable for you? Do you have more concerns? Regards, Enric > > Daniel. >
On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote: > Hi Uwe, Daniel, > > On 9/10/19 13:35, Daniel Thompson wrote: > > On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote: > >> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote: > >>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: > >>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > >>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > >>>>>> Hi Uwe, > >>>>>> > >>>>>> Adding Daniel and Lee to the discussion ... > >>>>> > >>>>> Thanks! > >>>>> > >>>>>> 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: > >>>>>>>>> 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). > >>>>> > >>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > >>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable > >>>>> out of both the power on/off functions so the duty-cycle/enable or > >>>>> disable can happen in one go within the update_status function. I don't > >>>>> think such a change would interfere with the power and enable sequencing > >>>>> needed by panels and it would therefore be a nice continuation of the > >>>>> work to convert over to the pwm_apply_state() API. > >>>> > >>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > >>>> I'd expect that this alone should already fix your issue. > >>>> > >>>>> None of the above has anything to do with what is right or wrong for > >>>>> the PWM API evolution. Of course, if this thread does conclude that it > >>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers > >>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM > >>>>> framework to help bring problems to the surface with all drivers. > >>>> > >>>> I think it's not possible to add a reliable WARN_ON for that issue. It > >>>> is quite expected that .get_state returns something that doesn't > >>>> completely match the requested configuration. So if a consumer requests > >>>> > >>>> .duty_cycle = 1 > >>>> .period = 100000000 > >>>> .enabled = false > >>>> > >>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > >>>> that has a concept of duty_cycle for disabled hardware. > >>>> > >>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. > >>> > >>> Isn't that intended to help identify "odd" PWM drivers rather than "odd" > >>> clients? > >>> > >>> Initially I was thinking that a WARN_ON() could be emitted when: > >>> > >>> 1. .duty_cycle is non-zero > >>> 2. .enabled is false > >>> 3. the PWM is not already enabled > >>> > >>> (#3 included to avoid too many false positives when disabling a PWM) > >> > >> I think I created a patch for that in the past, don't remember the > >> details. > >> > >>> A poisoning approach might be equally valid. If some drivers are > >>> permitted to "round" .duty_cycle to 0 when .enabled is false then the > >>> framework could get *all* drivers to behave in the same way by > >>> zeroing it out before calling the drivers apply method. It is not that > >>> big a deal but minimising the difference between driver behaviour should > >>> automatically reduce the difference in API usage by clients. > >> > >> I like it, but that breaks consumers that set .duty_cycle once during > >> probe and then only do: > >> > >> pwm_get_state(pwm, &state); > >> state.enabled = ... > >> pwm_apply_state(pwm, &state); > >> > >> which is a common idiom. > > > > Sorry I must have missed something. That appears to be identical to > > what pwm_bl.c currently does, albeit for rather better reasons. > > > > If setting the duty cycle and then separately enabling it is a > > reasonable idiom then the cros-ec-pwm driver is a broken implementation > > of the API and needs to be fixed regardless of any changes to pwm_bl.c . > > > > I think that Daniel has reason here. Just come to my mind that, if the fix is > not in the cros-ec-pwm driver we will also have the PWM sysfs user space > interface broken for that driver, apart from future consumers. > > This turns on to my first approach, this patch. Uwe, You had some complains > regarding is if I can only store enabled and duty_cycle instead of all the > pwm_state, that should also work for me. If I solve that concern, might be this > solution acceptable for you? Do you have more concerns? In my eyes we need a decision if "loosing" the value of .duty_cycle (and maybe .period) is considered ok. And if it's not we need to decide if we catch this in the pwm core or in each driver individually. My preference would be to not do it in each driver. Probably the best option is to do it in the core with an expressive code comment and then maybe make callers aware and issue a warning (if possible). But it's Thierry who has the last word on this. Best regards Uwe
On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote: > Hi Uwe, Daniel, > > On 9/10/19 13:35, Daniel Thompson wrote: > > On Wed, Oct 09, 2019 at 01:21:26PM +0200, Uwe Kleine-König wrote: > >> On Wed, Oct 09, 2019 at 11:42:36AM +0100, Daniel Thompson wrote: > >>> On Wed, Oct 09, 2019 at 12:16:37PM +0200, Uwe Kleine-König wrote: > >>>> On Wed, Oct 09, 2019 at 10:56:35AM +0100, Daniel Thompson wrote: > >>>>> On Wed, Oct 09, 2019 at 11:27:13AM +0200, Enric Balletbo i Serra wrote: > >>>>>> Hi Uwe, > >>>>>> > >>>>>> Adding Daniel and Lee to the discussion ... > >>>>> > >>>>> Thanks! > >>>>> > >>>>>> 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: > >>>>>>>>> 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). > >>>>> > >>>>> Looking at the pwm_bl code I wouldn't accept the above as it is but I'd > >>>>> almost certainly accept a patch to pwm_bl to move the PWM enable/disable > >>>>> out of both the power on/off functions so the duty-cycle/enable or > >>>>> disable can happen in one go within the update_status function. I don't > >>>>> think such a change would interfere with the power and enable sequencing > >>>>> needed by panels and it would therefore be a nice continuation of the > >>>>> work to convert over to the pwm_apply_state() API. > >>>> > >>>> OK for me. Enric, do you care enough to come up with a patch for pwm_bl? > >>>> I'd expect that this alone should already fix your issue. > >>>> > >>>>> None of the above has anything to do with what is right or wrong for > >>>>> the PWM API evolution. Of course, if this thread does conclude that it > >>>>> is OK the duty cycle of a disabled PWM to be retained for some drivers > >>>>> and not others then I'd hope to see some WARN_ON()s added to the PWM > >>>>> framework to help bring problems to the surface with all drivers. > >>>> > >>>> I think it's not possible to add a reliable WARN_ON for that issue. It > >>>> is quite expected that .get_state returns something that doesn't > >>>> completely match the requested configuration. So if a consumer requests > >>>> > >>>> .duty_cycle = 1 > >>>> .period = 100000000 > >>>> .enabled = false > >>>> > >>>> pwm_get_state possibly returns .duty_cycle = 0 even for drivers/hardware > >>>> that has a concept of duty_cycle for disabled hardware. > >>>> > >>>> A bit this is addressed in https://patchwork.ozlabs.org/patch/1147517/. > >>> > >>> Isn't that intended to help identify "odd" PWM drivers rather than "odd" > >>> clients? > >>> > >>> Initially I was thinking that a WARN_ON() could be emitted when: > >>> > >>> 1. .duty_cycle is non-zero > >>> 2. .enabled is false > >>> 3. the PWM is not already enabled > >>> > >>> (#3 included to avoid too many false positives when disabling a PWM) > >> > >> I think I created a patch for that in the past, don't remember the > >> details. > >> > >>> A poisoning approach might be equally valid. If some drivers are > >>> permitted to "round" .duty_cycle to 0 when .enabled is false then the > >>> framework could get *all* drivers to behave in the same way by > >>> zeroing it out before calling the drivers apply method. It is not that > >>> big a deal but minimising the difference between driver behaviour should > >>> automatically reduce the difference in API usage by clients. > >> > >> I like it, but that breaks consumers that set .duty_cycle once during > >> probe and then only do: > >> > >> pwm_get_state(pwm, &state); > >> state.enabled = ... > >> pwm_apply_state(pwm, &state); > >> > >> which is a common idiom. > > > > Sorry I must have missed something. That appears to be identical to > > what pwm_bl.c currently does, albeit for rather better reasons. > > > > If setting the duty cycle and then separately enabling it is a > > reasonable idiom then the cros-ec-pwm driver is a broken implementation > > of the API and needs to be fixed regardless of any changes to pwm_bl.c . > > > > I think that Daniel has reason here. Just come to my mind that, if the fix is > not in the cros-ec-pwm driver we will also have the PWM sysfs user space > interface broken for that driver, apart from future consumers. > > This turns on to my first approach, this patch. Uwe, You had some complains > regarding is if I can only store enabled and duty_cycle instead of all the > pwm_state, that should also work for me. If I solve that concern, might be this > solution acceptable for you? Do you have more concerns? There's been a similar report of pwm-backlight getting confused with the i.MX PWM driver. What ChromeOS EC and i.MX drivers both have in common is that they return a duty cycle of 0 when the PWM is off. For i.MX, the patch I propose caches the duty cycle and uses the cached value when appropriate. I think a similar patch (see below) would work for cros-ec. Can somebody give that a quick test to see whether it fixes the issue on ChromeOS hardware? Thierry --- >8 --- From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001 From: Thierry Reding <thierry.reding@gmail.com> Date: Thu, 17 Oct 2019 13:21:15 +0200 Subject: [PATCH] pwm: cros-ec: Cache duty cycle value The ChromeOS embedded controller doesn't differentiate between disabled and duty cycle being 0. In order not to potentially confuse consumers, cache the duty cycle and return the cached value instead of the real value when the PWM is disabled. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c index 89497448d217..09c08dee099e 100644 --- a/drivers/pwm/pwm-cros-ec.c +++ b/drivers/pwm/pwm-cros-ec.c @@ -25,11 +25,39 @@ struct cros_ec_pwm_device { struct pwm_chip chip; }; +/** + * struct cros_ec_pwm - per-PWM driver data + * @duty_cycle: cached duty cycle + */ +struct cros_ec_pwm { + u16 duty_cycle; +}; + static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) { return container_of(c, struct cros_ec_pwm_device, chip); } +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct cros_ec_pwm *channel; + + channel = kzalloc(sizeof(*channel), GFP_KERNEL); + if (!channel) + return -ENOMEM; + + pwm_set_chip_data(pwm, channel); + + return 0; +} + +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); + + kfree(channel); +} + static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) { struct { @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); - int duty_cycle; + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); + u16 duty_cycle; + int ret; /* The EC won't let us change the period */ if (state->period != EC_PWM_MAX_DUTY) @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, */ duty_cycle = state->enabled ? state->duty_cycle : 0; - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); + if (ret < 0) + return ret; + + channel->duty_cycle = state->duty_cycle; + + return 0; } static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); int ret; ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm); @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, 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; + /* + * Note that "disabled" and "duty cycle == 0" are treated the same. If + * the cached duty cycle is not zero, used the cached duty cycle. This + * ensures that the configured duty cycle is kept across a disable and + * enable operation and avoids potentially confusing consumers. + * + * For the case of the initial hardware readout, channel->duty_cycle + * will be 0 and the actual duty cycle read from the EC is used. + */ + if (ret == 0 && channel->duty_cycle > 0) + state->duty_cycle = channel->duty_cycle; + else + state->duty_cycle = ret; } static struct pwm_device * @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) } static const struct pwm_ops cros_ec_pwm_ops = { + .request = cros_ec_pwm_request, + .free = cros_ec_pwm_free, .get_state = cros_ec_pwm_get_state, .apply = cros_ec_pwm_apply, .owner = THIS_MODULE,
Hi Thierry, On 17/10/19 13:35, Thierry Reding wrote: > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote: [snip] > > --- >8 --- > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001 > From: Thierry Reding <thierry.reding@gmail.com> > Date: Thu, 17 Oct 2019 13:21:15 +0200 > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value > > The ChromeOS embedded controller doesn't differentiate between disabled > and duty cycle being 0. In order not to potentially confuse consumers, > cache the duty cycle and return the cached value instead of the real > value when the PWM is disabled. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 54 insertions(+), 4 deletions(-) > > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c > index 89497448d217..09c08dee099e 100644 > --- a/drivers/pwm/pwm-cros-ec.c > +++ b/drivers/pwm/pwm-cros-ec.c > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device { > struct pwm_chip chip; > }; > > +/** > + * struct cros_ec_pwm - per-PWM driver data > + * @duty_cycle: cached duty cycle > + */ > +struct cros_ec_pwm { > + u16 duty_cycle; > +}; > + > static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) > { > return container_of(c, struct cros_ec_pwm_device, chip); > } > > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct cros_ec_pwm *channel; > + > + channel = kzalloc(sizeof(*channel), GFP_KERNEL); > + if (!channel) > + return -ENOMEM; > + > + pwm_set_chip_data(pwm, channel); > + > + return 0; > +} > + > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > + > + kfree(channel); > +} > + > static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) > { > struct { > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > - int duty_cycle; > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > + u16 duty_cycle; > + int ret; > > /* The EC won't let us change the period */ > if (state->period != EC_PWM_MAX_DUTY) > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > */ > duty_cycle = state->enabled ? state->duty_cycle : 0; > > - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > + if (ret < 0) > + return ret; > + > + channel->duty_cycle = state->duty_cycle; > + > + return 0; > } > > static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_state *state) > { > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > int ret; > > ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm); > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > 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; > + /* > + * Note that "disabled" and "duty cycle == 0" are treated the same. If > + * the cached duty cycle is not zero, used the cached duty cycle. This > + * ensures that the configured duty cycle is kept across a disable and > + * enable operation and avoids potentially confusing consumers. > + * > + * For the case of the initial hardware readout, channel->duty_cycle > + * will be 0 and the actual duty cycle read from the EC is used. > + */ > + if (ret == 0 && channel->duty_cycle > 0) > + state->duty_cycle = channel->duty_cycle; > + else > + state->duty_cycle = ret; > } > > static struct pwm_device * > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) > } > > static const struct pwm_ops cros_ec_pwm_ops = { > + .request = cros_ec_pwm_request, > + .free = cros_ec_pwm_free, > .get_state = cros_ec_pwm_get_state, > .apply = cros_ec_pwm_apply, > .owner = THIS_MODULE, > I just tried your approach but I got a NULL pointer dereference while probe. I am just back from a week off but I'll be able to dig into it between today and tomorrow, just wanted to let you know that the patch doesn't works as is for me. [ 10.128455] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000 [ 10.141895] Mem abort info: [ 10.145090] ESR = 0x96000004 [ 10.148537] EC = 0x25: DABT (current EL), IL = 32 bits [ 10.154560] SET = 0, FnV = 0 [ 10.157986] EA = 0, S1PTW = 0 [ 10.161548] Data abort info: [ 10.164804] ISV = 0, ISS = 0x00000004 [ 10.169111] CM = 0, WnR = 0 [ 10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000 [ 10.179660] [0000000000000000] pgd=0000000000000000 [ 10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+) rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_ tables ipv6 nf_defrag_ipv6 [ 10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230 [ 10.179696] Hardware name: Google Kevin (DT) [ 10.179700] pstate: 60000005 (nZCv daif -PAN -UAO) [ 10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] [ 10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec] [ 10.247829] sp : ffff800012433810 [ 10.251531] x29: ffff800012433810 x28: 0000000200000026 [ 10.257469] x27: ffff800012433942 x26: ffff0000ef075010 [ 10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30 [ 10.269341] x23: 0000000000000000 x22: ffff0000ee273190 [ 10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270 [ 10.281214] x19: ffff800011bc98c8 x18: 0000000000000003 [ 10.287150] x17: 0000000000000007 x16: 000000000000000e [ 10.293088] x15: 0000000000000001 x14: 0000000000000019 [ 10.299024] x13: 0000000000000033 x12: 0000000000000018 [ 10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0 [ 10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA Objects: 31 [ 10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830 [ 10.310905] x7 : 000000000000011b x6 : 0000000000000001 [ 10.310908] x5 : 0000000000000000 x4 : 0000000000000000 [ 10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002 [ 10.310914] x1 : 0000000000000000 x0 : 0000000000000000 [ 10.310918] Call trace: [ 10.310931] cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] [ 10.310944] pwmchip_add_with_polarity+0x134/0x290 [ 10.363576] pwmchip_add+0x24/0x30 [ 10.367383] cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec] [ 10.373325] platform_drv_probe+0x58/0xa8 [ 10.377809] really_probe+0xe0/0x318 [ 10.381804] driver_probe_device+0x5c/0xf0 [ 10.386381] device_driver_attach+0x74/0x80 Thanks, Enric
On Mon, Oct 21, 2019 at 11:42:05AM +0200, Enric Balletbo i Serra wrote: > Hi Thierry, > On 17/10/19 13:35, Thierry Reding wrote: > > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote: > > [snip] > > > > > --- >8 --- > > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Thu, 17 Oct 2019 13:21:15 +0200 > > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value > > > > The ChromeOS embedded controller doesn't differentiate between disabled > > and duty cycle being 0. In order not to potentially confuse consumers, > > cache the duty cycle and return the cached value instead of the real > > value when the PWM is disabled. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c > > index 89497448d217..09c08dee099e 100644 > > --- a/drivers/pwm/pwm-cros-ec.c > > +++ b/drivers/pwm/pwm-cros-ec.c > > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device { > > struct pwm_chip chip; > > }; > > > > +/** > > + * struct cros_ec_pwm - per-PWM driver data > > + * @duty_cycle: cached duty cycle > > + */ > > +struct cros_ec_pwm { > > + u16 duty_cycle; > > +}; > > + > > static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) > > { > > return container_of(c, struct cros_ec_pwm_device, chip); > > } > > > > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct cros_ec_pwm *channel; > > + > > + channel = kzalloc(sizeof(*channel), GFP_KERNEL); > > + if (!channel) > > + return -ENOMEM; > > + > > + pwm_set_chip_data(pwm, channel); > > + > > + return 0; > > +} > > + > > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > + > > + kfree(channel); > > +} > > + > > static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) > > { > > struct { > > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > > - int duty_cycle; > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > + u16 duty_cycle; > > + int ret; > > > > /* The EC won't let us change the period */ > > if (state->period != EC_PWM_MAX_DUTY) > > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > */ > > duty_cycle = state->enabled ? state->duty_cycle : 0; > > > > - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > > + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > > + if (ret < 0) > > + return ret; > > + > > + channel->duty_cycle = state->duty_cycle; > > + > > + return 0; > > } > > > > static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > struct pwm_state *state) > > { > > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > int ret; > > > > ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm); > > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > 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; > > + /* > > + * Note that "disabled" and "duty cycle == 0" are treated the same. If > > + * the cached duty cycle is not zero, used the cached duty cycle. This > > + * ensures that the configured duty cycle is kept across a disable and > > + * enable operation and avoids potentially confusing consumers. > > + * > > + * For the case of the initial hardware readout, channel->duty_cycle > > + * will be 0 and the actual duty cycle read from the EC is used. > > + */ > > + if (ret == 0 && channel->duty_cycle > 0) > > + state->duty_cycle = channel->duty_cycle; > > + else > > + state->duty_cycle = ret; > > } > > > > static struct pwm_device * > > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) > > } > > > > static const struct pwm_ops cros_ec_pwm_ops = { > > + .request = cros_ec_pwm_request, > > + .free = cros_ec_pwm_free, > > .get_state = cros_ec_pwm_get_state, > > .apply = cros_ec_pwm_apply, > > .owner = THIS_MODULE, > > > > I just tried your approach but I got a NULL pointer dereference while probe. I > am just back from a week off but I'll be able to dig into it between today and > tomorrow, just wanted to let you know that the patch doesn't works as is for me. > > [ 10.128455] Unable to handle kernel NULL pointer dereference at virtual > address 0000000000000000 > [ 10.141895] Mem abort info: > > [ 10.145090] ESR = 0x96000004 > > [ 10.148537] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 10.154560] SET = 0, FnV = 0 > > [ 10.157986] EA = 0, S1PTW = 0 > > [ 10.161548] Data abort info: > > [ 10.164804] ISV = 0, ISS = 0x00000004 > > [ 10.169111] CM = 0, WnR = 0 > > [ 10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000 > > [ 10.179660] [0000000000000000] pgd=0000000000000000 > > [ 10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP > > [ 10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+) > rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_ > tables ipv6 nf_defrag_ipv6 > > [ 10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230 > > [ 10.179696] Hardware name: Google Kevin (DT) > > [ 10.179700] pstate: 60000005 (nZCv daif -PAN -UAO) > > [ 10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] > > [ 10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec] > > [ 10.247829] sp : ffff800012433810 > > [ 10.251531] x29: ffff800012433810 x28: 0000000200000026 > > [ 10.257469] x27: ffff800012433942 x26: ffff0000ef075010 > > [ 10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30 > > [ 10.269341] x23: 0000000000000000 x22: ffff0000ee273190 > [ 10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270 > [ 10.281214] x19: ffff800011bc98c8 x18: 0000000000000003 > [ 10.287150] x17: 0000000000000007 x16: 000000000000000e > [ 10.293088] x15: 0000000000000001 x14: 0000000000000019 > [ 10.299024] x13: 0000000000000033 x12: 0000000000000018 > [ 10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0 > [ 10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA > Objects: 31 > [ 10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830 > [ 10.310905] x7 : 000000000000011b x6 : 0000000000000001 > [ 10.310908] x5 : 0000000000000000 x4 : 0000000000000000 > [ 10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002 > [ 10.310914] x1 : 0000000000000000 x0 : 0000000000000000 > [ 10.310918] Call trace: > [ 10.310931] cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] > [ 10.310944] pwmchip_add_with_polarity+0x134/0x290 The problem is that .get_state is called from the core without requesting the pwm and so .request wasn't called before. I still think it's wrong to cache the duty-cycle in the driver, but if you still want to do it, .duty_cycle must go into the driver struct which however is very inefficient given that probably most of the machines don't actually have 255 PWMs. Best regards Uwe
On Mon, Oct 21, 2019 at 11:42:05AM +0200, Enric Balletbo i Serra wrote: > Hi Thierry, > On 17/10/19 13:35, Thierry Reding wrote: > > On Wed, Oct 09, 2019 at 03:47:43PM +0200, Enric Balletbo i Serra wrote: > > [snip] > > > > > --- >8 --- > > From 15245e5f0dc02af021451b098df93901c9f49373 Mon Sep 17 00:00:00 2001 > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Thu, 17 Oct 2019 13:21:15 +0200 > > Subject: [PATCH] pwm: cros-ec: Cache duty cycle value > > > > The ChromeOS embedded controller doesn't differentiate between disabled > > and duty cycle being 0. In order not to potentially confuse consumers, > > cache the duty cycle and return the cached value instead of the real > > value when the PWM is disabled. > > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-cros-ec.c | 58 ++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 54 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c > > index 89497448d217..09c08dee099e 100644 > > --- a/drivers/pwm/pwm-cros-ec.c > > +++ b/drivers/pwm/pwm-cros-ec.c > > @@ -25,11 +25,39 @@ struct cros_ec_pwm_device { > > struct pwm_chip chip; > > }; > > > > +/** > > + * struct cros_ec_pwm - per-PWM driver data > > + * @duty_cycle: cached duty cycle > > + */ > > +struct cros_ec_pwm { > > + u16 duty_cycle; > > +}; > > + > > static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) > > { > > return container_of(c, struct cros_ec_pwm_device, chip); > > } > > > > +static int cros_ec_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct cros_ec_pwm *channel; > > + > > + channel = kzalloc(sizeof(*channel), GFP_KERNEL); > > + if (!channel) > > + return -ENOMEM; > > + > > + pwm_set_chip_data(pwm, channel); > > + > > + return 0; > > +} > > + > > +static void cros_ec_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > > +{ > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > + > > + kfree(channel); > > +} > > + > > static int cros_ec_pwm_set_duty(struct cros_ec_device *ec, u8 index, u16 duty) > > { > > struct { > > @@ -96,7 +124,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > > - int duty_cycle; > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > + u16 duty_cycle; > > + int ret; > > > > /* The EC won't let us change the period */ > > if (state->period != EC_PWM_MAX_DUTY) > > @@ -108,13 +138,20 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > */ > > duty_cycle = state->enabled ? state->duty_cycle : 0; > > > > - return cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > > + ret = cros_ec_pwm_set_duty(ec_pwm->ec, pwm->hwpwm, duty_cycle); > > + if (ret < 0) > > + return ret; > > + > > + channel->duty_cycle = state->duty_cycle; > > + > > + return 0; > > } > > > > static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > struct pwm_state *state) > > { > > struct cros_ec_pwm_device *ec_pwm = pwm_to_cros_ec_pwm(chip); > > + struct cros_ec_pwm *channel = pwm_get_chip_data(pwm); > > int ret; > > > > ret = cros_ec_pwm_get_duty(ec_pwm->ec, pwm->hwpwm); > > @@ -126,8 +163,19 @@ static void cros_ec_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > 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; > > + /* > > + * Note that "disabled" and "duty cycle == 0" are treated the same. If > > + * the cached duty cycle is not zero, used the cached duty cycle. This > > + * ensures that the configured duty cycle is kept across a disable and > > + * enable operation and avoids potentially confusing consumers. > > + * > > + * For the case of the initial hardware readout, channel->duty_cycle > > + * will be 0 and the actual duty cycle read from the EC is used. > > + */ > > + if (ret == 0 && channel->duty_cycle > 0) > > + state->duty_cycle = channel->duty_cycle; > > + else > > + state->duty_cycle = ret; > > } > > > > static struct pwm_device * > > @@ -149,6 +197,8 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args) > > } > > > > static const struct pwm_ops cros_ec_pwm_ops = { > > + .request = cros_ec_pwm_request, > > + .free = cros_ec_pwm_free, > > .get_state = cros_ec_pwm_get_state, > > .apply = cros_ec_pwm_apply, > > .owner = THIS_MODULE, > > > > I just tried your approach but I got a NULL pointer dereference while probe. I > am just back from a week off but I'll be able to dig into it between today and > tomorrow, just wanted to let you know that the patch doesn't works as is for me. > > [ 10.128455] Unable to handle kernel NULL pointer dereference at virtual > address 0000000000000000 > [ 10.141895] Mem abort info: > > [ 10.145090] ESR = 0x96000004 > > [ 10.148537] EC = 0x25: DABT (current EL), IL = 32 bits > > [ 10.154560] SET = 0, FnV = 0 > > [ 10.157986] EA = 0, S1PTW = 0 > > [ 10.161548] Data abort info: > > [ 10.164804] ISV = 0, ISS = 0x00000004 > > [ 10.169111] CM = 0, WnR = 0 > > [ 10.172436] user pgtable: 4k pages, 48-bit VAs, pgdp=00000000ed44b000 > > [ 10.179660] [0000000000000000] pgd=0000000000000000 > > [ 10.179669] Internal error: Oops: 96000004 [#1] PREEMPT SMP > > [ 10.179673] Modules linked in: atmel_mxt_ts(+) rockchip_saradc pwm_cros_ec(+) > rockchip_thermal pcie_rockchip_host snd_soc_rl6231 ip_tables x_ > tables ipv6 nf_defrag_ipv6 > > [ 10.179694] CPU: 1 PID: 255 Comm: systemd-udevd Not tainted 5.4.0-rc4+ #230 > > [ 10.179696] Hardware name: Google Kevin (DT) > > [ 10.179700] pstate: 60000005 (nZCv daif -PAN -UAO) > > [ 10.179714] pc : cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] > > [ 10.179721] lr : cros_ec_pwm_get_state+0x80/0xf8 [pwm_cros_ec] > > [ 10.247829] sp : ffff800012433810 > > [ 10.251531] x29: ffff800012433810 x28: 0000000200000026 > > [ 10.257469] x27: ffff800012433942 x26: ffff0000ef075010 > > [ 10.263405] x25: ffff800011ca8508 x24: ffff800011e68e30 > > [ 10.269341] x23: 0000000000000000 x22: ffff0000ee273190 > [ 10.275278] x21: ffff0000ee2e3240 x20: ffff0000ee2e3270 > [ 10.281214] x19: ffff800011bc98c8 x18: 0000000000000003 > [ 10.287150] x17: 0000000000000007 x16: 000000000000000e > [ 10.293088] x15: 0000000000000001 x14: 0000000000000019 > [ 10.299024] x13: 0000000000000033 x12: 0000000000000018 > [ 10.304962] x11: 071c71c71c71c71c x10: 00000000000009d0 > [ 10.310379] atmel_mxt_ts 5-004a: Family: 164 Variant: 17 Firmware V2.0.AA > Objects: 31 > [ 10.310901] x9 : ffff800012433490 x8 : ffff0000edb81830 > [ 10.310905] x7 : 000000000000011b x6 : 0000000000000001 > [ 10.310908] x5 : 0000000000000000 x4 : 0000000000000000 > [ 10.310911] x3 : ffff0000edb80e00 x2 : 0000000000000002 > [ 10.310914] x1 : 0000000000000000 x0 : 0000000000000000 > [ 10.310918] Call trace: > [ 10.310931] cros_ec_pwm_get_state+0xcc/0xf8 [pwm_cros_ec] > [ 10.310944] pwmchip_add_with_polarity+0x134/0x290 > [ 10.363576] pwmchip_add+0x24/0x30 > [ 10.367383] cros_ec_pwm_probe+0x13c/0x1cc [pwm_cros_ec] > [ 10.373325] platform_drv_probe+0x58/0xa8 > [ 10.377809] really_probe+0xe0/0x318 > [ 10.381804] driver_probe_device+0x5c/0xf0 > [ 10.386381] device_driver_attach+0x74/0x80 Okay, this is caused by ->get_state() getting called during pwmchip_add_with_polarity(), which is before any of the PWMs can have been requested, which means ->request() has not been called yet and therefore the per-PWM state doesn't exist yet. There's a few ways to work around that (one would be to store the state within the per-chip structure (rather than per-PWM, another would be to defer calling the ->get_state() callback when the PWM is requested). Either way, given that we've got a number of other drivers with similar problems, I think at this point it's better to revert for now in v5.4 and if we do want to get this functionality back we need to start work on fixing up the problematic drivers. Thierry
diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c index 89497448d217..f750a3cf0c6c 100644 --- a/drivers/pwm/pwm-cros-ec.c +++ b/drivers/pwm/pwm-cros-ec.c @@ -18,11 +18,13 @@ * @dev: Device node * @ec: Pointer to EC device * @chip: PWM controller chip + * @state: Holds the last state applied */ struct cros_ec_pwm_device { struct device *dev; struct cros_ec_device *ec; struct pwm_chip chip; + struct pwm_state state; }; static inline struct cros_ec_pwm_device *pwm_to_cros_ec_pwm(struct pwm_chip *c) @@ -102,6 +104,9 @@ static int cros_ec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->period != EC_PWM_MAX_DUTY) return -EINVAL; + /* Store the new state */ + ec_pwm->state = *state; + /* * EC doesn't separate the concept of duty cycle and enabled, but * kernel does. Translate. @@ -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) { + 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; } static struct pwm_device *
For the cros-ec-pwm, "disabled" is the same as "duty cycle == 0", and is not possible to program a duty cycle while the device is disabled. However, the PWM API allows us to configure the "duty cycle" while the device is "disabled". But now, pwm_get_state() is returning the real hardware state instead of the last applied state, and this change of behavior, broke the display on my rk3399-gru-kevin and doesn't turn on anymore. Commit 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state") introduced this change of behavior. And, assuming that this is the right to do, workaround this problem for the cros-ec-pwm driver by reverting the mentioned commit at the lowlevel driver. With that patch applied pwm_get_state() will return only the programmed hardware duty cycle value if the PWM is enabled. When is disabled, will return the last applied duty_cycle value instead. That's not ideal, but definetely is better than don't implement .get_state(). Fixes: 01ccf903edd6 ("pwm: Let pwm_get_state() return the last implemented state") Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- drivers/pwm/pwm-cros-ec.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)