linux-kernel - Re: [PATCH] pwm: pwm-samsung: switch to new atomic PWM API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20170821082948.GR18996@ulmo>
Date:   Mon, 21 Aug 2017 10:29:48 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
Cc:     Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Kamil Debski <kamil@...as.org>,
        Tomasz Figa <tomasz.figa@...il.com>, linux-pwm@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH] pwm: pwm-samsung: switch to new atomic PWM API

On Mon, Apr 24, 2017 at 03:13:18PM +0200, Bartlomiej Zolnierkiewicz wrote:
> Switch pwm-samsung driver to new atomic PWM API.
> 
> This is an initial conversion (based on the PWM core's
> pwm_apply_state() implementation) which can be improved
> later.
> 
> There should be no functional changes caused by this
> patch.
> 
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>
> ---
> Depends on "[PATCH v2 0/3] pwm: pwm-samsung: fix suspend/resume support"
> patchset (https://www.spinics.net/lists/kernel/msg2495209.html).
> 
>  drivers/pwm/pwm-samsung.c | 59 ++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 14 deletions(-)

Sorry, it looks like I never reviewed this.

> diff --git a/drivers/pwm/pwm-samsung.c b/drivers/pwm/pwm-samsung.c
> index 062f2cf..09868a9 100644
> --- a/drivers/pwm/pwm-samsung.c
> +++ b/drivers/pwm/pwm-samsung.c
> @@ -241,7 +241,7 @@ static void pwm_samsung_free(struct pwm_chip *chip, struct pwm_device *pwm)
>  	pwm_set_chip_data(pwm, NULL);
>  }
>  
> -static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +static void pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct samsung_pwm_chip *our_chip = to_samsung_pwm_chip(chip);
>  	unsigned int tcon_chan = to_tcon_channel(pwm->hwpwm);
> @@ -263,8 +263,6 @@ static int pwm_samsung_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	our_chip->disabled_mask &= ~BIT(pwm->hwpwm);
>  
>  	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
> -
> -	return 0;
>  }
>  
>  static void pwm_samsung_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -385,12 +383,6 @@ static int __pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> -static int pwm_samsung_config(struct pwm_chip *chip, struct pwm_device *pwm,
> -			      int duty_ns, int period_ns)
> -{
> -	return __pwm_samsung_config(chip, pwm, duty_ns, period_ns, false);
> -}
> -
>  static void pwm_samsung_set_invert(struct samsung_pwm_chip *chip,
>  				   unsigned int channel, bool invert)
>  {
> @@ -415,7 +407,7 @@ static void pwm_samsung_set_invert(struct samsung_pwm_chip *chip,
>  	spin_unlock_irqrestore(&samsung_pwm_lock, flags);
>  }
>  
> -static int pwm_samsung_set_polarity(struct pwm_chip *chip,
> +static void pwm_samsung_set_polarity(struct pwm_chip *chip,
>  				    struct pwm_device *pwm,
>  				    enum pwm_polarity polarity)
>  {
> @@ -424,6 +416,48 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip,
>  
>  	/* Inverted means normal in the hardware. */
>  	pwm_samsung_set_invert(our_chip, pwm->hwpwm, invert);
> +}
> +
> +static int pwm_samsung_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     struct pwm_state *state)
> +{
> +	int err;
> +
> +	/*
> +	 * FIXME: restore the initial state in case of error.
> +	 */
> +	if (state->polarity != pwm->state.polarity) {
> +		if (pwm->state.enabled) {
> +			pwm_samsung_disable(pwm->chip, pwm);
> +			pwm->state.enabled = false;
> +		}
> +
> +		pwm_samsung_set_polarity(pwm->chip, pwm,
> +					 state->polarity);
> +
> +		pwm->state.polarity = state->polarity;
> +	}

The whole point, or at least much of it, of the atomic API is that you
can perform all computations before writing any registers, exactly so
that you can avoid having to restore the initial state. Otherwise we're
no better than the legacy API, really.

> +	if (state->period != pwm->state.period ||
> +	    state->duty_cycle != pwm->state.duty_cycle) {
> +		err = __pwm_samsung_config(pwm->chip, pwm,
> +					   state->duty_cycle,
> +					   state->period, false);
> +		if (err)
> +			return err;
> +
> +		pwm->state.duty_cycle = state->duty_cycle;
> +		pwm->state.period = state->period;
> +	}
> +
> +	if (state->enabled != pwm->state.enabled) {
> +		if (state->enabled)
> +			pwm_samsung_enable(pwm->chip, pwm);
> +		else
> +			pwm_samsung_disable(pwm->chip, pwm);
> +
> +		pwm->state.enabled = state->enabled;
> +	}
>  
>  	return 0;
>  }
> @@ -431,10 +465,7 @@ static int pwm_samsung_set_polarity(struct pwm_chip *chip,
>  static const struct pwm_ops pwm_samsung_ops = {
>  	.request	= pwm_samsung_request,
>  	.free		= pwm_samsung_free,
> -	.enable		= pwm_samsung_enable,
> -	.disable	= pwm_samsung_disable,
> -	.config		= pwm_samsung_config,
> -	.set_polarity	= pwm_samsung_set_polarity,
> +	.apply		= pwm_samsung_apply,

You should always provide a .get_state() implementation as well to go
along with .apply().

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)