linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <thierry.reding@gmail.com>,
	Lee Jones <lee.jones@linaro.org>,
	linux-pwm@vger.kernel.org, kernel@pengutronix.de,
	linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent
Date: Tue, 29 Jun 2021 21:44:38 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2106292138100.1194476@ramsan.of.borg> (raw)
In-Reply-To: <20210501160943.108821-1-u.kleine-koenig@pengutronix.de>

[-- Attachment #1: Type: text/plain, Size: 6009 bytes --]

 	Hi Uwe,

On Sat, 1 May 2021, Uwe Kleine-König wrote:
> Without this change it can happen that if changing the polarity succeeded
> but changing duty_cycle and period failed pwm->state contains a mixture
> between the old and the requested state.
>
> So remember the initial state before starting to modify the configuration
> and restore it when one of the required callback fails.
>
> Compared to the previous implementation .disable() (if necessary) is called
> earlier to prevent a glitch.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks for your patch, which is now commit d7bff84fe7ed8c3b ("pwm:
Ensure for legacy drivers that pwm->state stays consistent") in
pwm/for-next.

This commit broke the backlight on the Atmark Techno Armadillo 800 EVA
board (arch/arm/boot/dts/r8a7740-armadillo800eva.dts), which now shows a
black screen.  Reverting the commit fixes the problem.

Do you have an idea what is wrong, and how to fix it?
Anything I can do to investigate?

Thanks!

> ---
> Hello,
>
> just a small optimisation: At the end of pwm_apply_legacy()
> state->enabled is known to be true, so simplify
>
> 	if (state->enabled && !pwm->state.enabled) {
>
> to
> 	if (!pwm->state.enabled) {
>
> Best regards
> Uwe
>
> drivers/pwm/core.c | 139 +++++++++++++++++++++++++--------------------
> 1 file changed, 78 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c4d5c0667137..57105deafb55 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -535,6 +535,71 @@ static void pwm_apply_state_debug(struct pwm_device *pwm,
> 	}
> }
>
> +static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> +			    const struct pwm_state *state)
> +{
> +	int err;
> +	struct pwm_state initial_state = pwm->state;
> +
> +	if (state->polarity != pwm->state.polarity) {
> +		if (!chip->ops->set_polarity) {
> +			err = -EINVAL;
> +			goto out_err;
> +		}
> +
> +		/*
> +		 * Changing the polarity of a running PWM is only allowed when
> +		 * the PWM driver implements ->apply().
> +		 */
> +		if (pwm->state.enabled) {
> +			chip->ops->disable(chip, pwm);
> +
> +			/*
> +			 * Update pwm->state already here in case
> +			 * .set_polarity() or another callback depend on that.
> +			 */
> +			pwm->state.enabled = false;
> +		}
> +
> +		err = chip->ops->set_polarity(chip, pwm,
> +					      state->polarity);
> +		if (err)
> +			goto out_err;
> +
> +		pwm->state.polarity = state->polarity;
> +	}
> +
> +	if (!state->enabled) {
> +		if (pwm->state.enabled)
> +			chip->ops->disable(chip, pwm);
> +		return 0;
> +	}
> +
> +	if (state->period != pwm->state.period ||
> +	    state->duty_cycle != pwm->state.duty_cycle) {
> +		err = chip->ops->config(pwm->chip, pwm,
> +					state->duty_cycle,
> +					state->period);
> +		if (err)
> +			goto out_err;
> +
> +		pwm->state.period = state->period;
> +		pwm->state.duty_cycle = state->duty_cycle;
> +	}
> +
> +	if (!pwm->state.enabled) {
> +		err = chip->ops->enable(chip, pwm);
> +		if (err)
> +			goto out_err;
> +	}
> +
> +	return 0;
> +
> +out_err:
> +	pwm->state = initial_state;
> +	return err;
> +}
> +
> /**
>  * pwm_apply_state() - atomically apply a new state to a PWM device
>  * @pwm: PWM device
> @@ -544,6 +609,8 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> {
> 	struct pwm_chip *chip;
> 	int err;
> +	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
> +		     const struct pwm_state *state);
>
> 	if (!pwm || !state || !state->period ||
> 	    state->duty_cycle > state->period)
> @@ -557,70 +624,20 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
> 	    state->enabled == pwm->state.enabled)
> 		return 0;
>
> -	if (chip->ops->apply) {
> -		err = chip->ops->apply(chip, pwm, state);
> -		if (err)
> -			return err;
> -
> -		trace_pwm_apply(pwm, state);
> -
> -		pwm->state = *state;
> -
> -		/*
> -		 * only do this after pwm->state was applied as some
> -		 * implementations of .get_state depend on this
> -		 */
> -		pwm_apply_state_debug(pwm, state);
> -	} else {
> -		/*
> -		 * FIXME: restore the initial state in case of error.
> -		 */
> -		if (state->polarity != pwm->state.polarity) {
> -			if (!chip->ops->set_polarity)
> -				return -EINVAL;
> -
> -			/*
> -			 * Changing the polarity of a running PWM is
> -			 * only allowed when the PWM driver implements
> -			 * ->apply().
> -			 */
> -			if (pwm->state.enabled) {
> -				chip->ops->disable(chip, pwm);
> -				pwm->state.enabled = false;
> -			}
> +	apply = chip->ops->apply ?: pwm_apply_legacy;
> +	err = apply(chip, pwm, state);
> +	if (err)
> +		return err;
>
> -			err = chip->ops->set_polarity(chip, pwm,
> -						      state->polarity);
> -			if (err)
> -				return err;
> +	trace_pwm_apply(pwm, state);
>
> -			pwm->state.polarity = state->polarity;
> -		}
> -
> -		if (state->period != pwm->state.period ||
> -		    state->duty_cycle != pwm->state.duty_cycle) {
> -			err = chip->ops->config(pwm->chip, pwm,
> -						state->duty_cycle,
> -						state->period);
> -			if (err)
> -				return err;
> +	pwm->state = *state;
>
> -			pwm->state.duty_cycle = state->duty_cycle;
> -			pwm->state.period = state->period;
> -		}
> -
> -		if (state->enabled != pwm->state.enabled) {
> -			if (state->enabled) {
> -				err = chip->ops->enable(chip, pwm);
> -				if (err)
> -					return err;
> -			} else {
> -				chip->ops->disable(chip, pwm);
> -			}
> -
> -			pwm->state.enabled = state->enabled;
> -		}
> -	}
> +	/*
> +	 * only do this after pwm->state was applied as some
> +	 * implementations of .get_state depend on this
> +	 */
> +	pwm_apply_state_debug(pwm, state);
>
> 	return 0;
> }
> -- 
> 2.30.2

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds

       reply	other threads:[~2021-06-29 19:44 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210411160451.1207799-1-u.kleine-koenig@pengutronix.de>
     [not found] ` <20210501160943.108821-1-u.kleine-koenig@pengutronix.de>
2021-06-29 19:44   ` Geert Uytterhoeven [this message]
2021-06-30  6:48     ` [PATCH v2] pwm: Ensure for legacy drivers that pwm->state stays consistent Uwe Kleine-König
2021-06-30 10:22       ` Geert Uytterhoeven
2021-06-30 16:21         ` Uwe Kleine-König
2021-06-30 17:08           ` Geert Uytterhoeven
2021-06-30 17:18           ` Thierry Reding
2021-06-29 19:52   ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.22.394.2106292138100.1194476@ramsan.of.borg \
    --to=geert@linux-m68k.org \
    --cc=kernel@pengutronix.de \
    --cc=lee.jones@linaro.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=thierry.reding@gmail.com \
    --cc=u.kleine-koenig@pengutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).