Re: [PATCH] pwm: cros-ec: Let cros_ec_pwm_get_state() return the last applied state

[Thierry Reding]

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@xxxxxxxxx>
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@xxxxxxxxx>
---
 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,
-- 
2.23.0

Attachment: signature.asc
Description: PGP signature