Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1629532 > unrolled thread

[PATCH] hwmon: pwm-fan: switch to new atomic PWM API

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] hwmon: pwm-fan: switch to new atomic PWM API Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-04-24 15:20 +0200
    [PATCH] pwm: pwm-samsung: switch to new atomic PWM API Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-04-24 15:20 +0200
    Re: [PATCH] hwmon: pwm-fan: switch to new atomic PWM API Guenter Roeck <linux@roeck-us.net> - 2017-04-24 15:30 +0200
      Re: [PATCH] hwmon: pwm-fan: switch to new atomic PWM API Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-04-24 16:30 +0200

#1629532 — [PATCH] hwmon: pwm-fan: switch to new atomic PWM API

Switch pwm-fan driver to new atomic PWM API.

Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
Depends on "[PATCH v2 0/3] pwm: pwm-samsung: fix suspend/resume support"
patchset (https://www.spinics.net/lists/kernel/msg2495209.html).

 drivers/hwmon/pwm-fan.c | 68 +++++++++++++++++++------------------------------
 1 file changed, 26 insertions(+), 42 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 9dc40f3..4ac7700 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -40,31 +40,22 @@ struct pwm_fan_ctx {
 
 static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 {
-	struct pwm_args pargs;
-	unsigned long duty;
+	unsigned long period;
 	int ret = 0;
-
-	pwm_get_args(ctx->pwm, &pargs);
+	struct pwm_state state = { };
 
 	mutex_lock(&ctx->lock);
 	if (ctx->pwm_value == pwm)
 		goto exit_set_pwm_err;
 
-	duty = DIV_ROUND_UP(pwm * (pargs.period - 1), MAX_PWM);
-	ret = pwm_config(ctx->pwm, duty, pargs.period);
-	if (ret)
-		goto exit_set_pwm_err;
-
-	if (pwm == 0)
-		pwm_disable(ctx->pwm);
+	pwm_init_state(ctx->pwm, &state);
+	period = ctx->pwm->args.period;
+	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
+	state.enabled = pwm ? true : false;
 
-	if (ctx->pwm_value == 0) {
-		ret = pwm_enable(ctx->pwm);
-		if (ret)
-			goto exit_set_pwm_err;
-	}
-
-	ctx->pwm_value = pwm;
+	ret = pwm_apply_state(ctx->pwm, &state);
+	if (!ret)
+		ctx->pwm_value = pwm;
 exit_set_pwm_err:
 	mutex_unlock(&ctx->lock);
 	return ret;
@@ -218,10 +209,9 @@ static int pwm_fan_probe(struct platform_device *pdev)
 {
 	struct thermal_cooling_device *cdev;
 	struct pwm_fan_ctx *ctx;
-	struct pwm_args pargs;
 	struct device *hwmon;
-	int duty_cycle;
 	int ret;
+	struct pwm_state state = { };
 
 	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -237,28 +227,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ctx);
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(ctx->pwm);
-
-	/* Set duty cycle to maximum allowed */
-	pwm_get_args(ctx->pwm, &pargs);
-
-	duty_cycle = pargs.period - 1;
 	ctx->pwm_value = MAX_PWM;
 
-	ret = pwm_config(ctx->pwm, duty_cycle, pargs.period);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to configure PWM\n");
-		return ret;
-	}
+	/* Set duty cycle to maximum allowed and enable PWM output */
+	pwm_init_state(ctx->pwm, &state);
+	state.duty_cycle = ctx->pwm->args.period - 1;
+	state.enabled = true;
 
-	/* Enbale PWM output */
-	ret = pwm_enable(ctx->pwm);
+	ret = pwm_apply_state(ctx->pwm, &state);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to enable PWM\n");
+		dev_err(&pdev->dev, "Failed to configure PWM\n");
 		return ret;
 	}
 
@@ -266,8 +244,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
 						       ctx, pwm_fan_groups);
 	if (IS_ERR(hwmon)) {
 		dev_err(&pdev->dev, "Failed to register hwmon device\n");
-		pwm_disable(ctx->pwm);
-		return PTR_ERR(hwmon);
+		ret = PTR_ERR(hwmon);
+		goto err_pwm_disable;
 	}
 
 	ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx);
@@ -282,14 +260,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		if (IS_ERR(cdev)) {
 			dev_err(&pdev->dev,
 				"Failed to register pwm-fan as cooling device");
-			pwm_disable(ctx->pwm);
-			return PTR_ERR(cdev);
+			ret = PTR_ERR(cdev);
+			goto err_pwm_disable;
 		}
 		ctx->cdev = cdev;
 		thermal_cdev_update(cdev);
 	}
 
 	return 0;
+
+err_pwm_disable:
+	state.enabled = false;
+	pwm_apply_state(ctx->pwm, &state);
+
+	return ret;
 }
 
 static int pwm_fan_remove(struct platform_device *pdev)
-- 
1.9.1

[toc] | [next] | [standalone]

#1629538 — [PATCH] pwm: pwm-samsung: switch to new atomic PWM API

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@samsung.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(-)

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;
+	}
+
+	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,
 	.owner		= THIS_MODULE,
 };
 
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1629558

On 04/24/2017 06:13 AM, Bartlomiej Zolnierkiewicz wrote:
> Switch pwm-fan driver to new atomic PWM API.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
> Depends on "[PATCH v2 0/3] pwm: pwm-samsung: fix suspend/resume support"
> patchset (https://www.spinics.net/lists/kernel/msg2495209.html).
>

Why would this driver, which does not depend on any Samsung code,
depend on a Samsung specific fix ?

Was this tested with hardware ?

Thanks,
Guenter

>  drivers/hwmon/pwm-fan.c | 68 +++++++++++++++++++------------------------------
>  1 file changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 9dc40f3..4ac7700 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -40,31 +40,22 @@ struct pwm_fan_ctx {
>
>  static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  {
> -	struct pwm_args pargs;
> -	unsigned long duty;
> +	unsigned long period;
>  	int ret = 0;
> -
> -	pwm_get_args(ctx->pwm, &pargs);
> +	struct pwm_state state = { };
>
>  	mutex_lock(&ctx->lock);
>  	if (ctx->pwm_value == pwm)
>  		goto exit_set_pwm_err;
>
> -	duty = DIV_ROUND_UP(pwm * (pargs.period - 1), MAX_PWM);
> -	ret = pwm_config(ctx->pwm, duty, pargs.period);
> -	if (ret)
> -		goto exit_set_pwm_err;
> -
> -	if (pwm == 0)
> -		pwm_disable(ctx->pwm);
> +	pwm_init_state(ctx->pwm, &state);
> +	period = ctx->pwm->args.period;
> +	state.duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> +	state.enabled = pwm ? true : false;
>
> -	if (ctx->pwm_value == 0) {
> -		ret = pwm_enable(ctx->pwm);
> -		if (ret)
> -			goto exit_set_pwm_err;
> -	}
> -
> -	ctx->pwm_value = pwm;
> +	ret = pwm_apply_state(ctx->pwm, &state);
> +	if (!ret)
> +		ctx->pwm_value = pwm;
>  exit_set_pwm_err:
>  	mutex_unlock(&ctx->lock);
>  	return ret;
> @@ -218,10 +209,9 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  {
>  	struct thermal_cooling_device *cdev;
>  	struct pwm_fan_ctx *ctx;
> -	struct pwm_args pargs;
>  	struct device *hwmon;
> -	int duty_cycle;
>  	int ret;
> +	struct pwm_state state = { };
>
>  	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
> @@ -237,28 +227,16 @@ static int pwm_fan_probe(struct platform_device *pdev)
>
>  	platform_set_drvdata(pdev, ctx);
>
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to the
> -	 * atomic PWM API.
> -	 */
> -	pwm_apply_args(ctx->pwm);
> -
> -	/* Set duty cycle to maximum allowed */
> -	pwm_get_args(ctx->pwm, &pargs);
> -
> -	duty_cycle = pargs.period - 1;
>  	ctx->pwm_value = MAX_PWM;
>
> -	ret = pwm_config(ctx->pwm, duty_cycle, pargs.period);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Failed to configure PWM\n");
> -		return ret;
> -	}
> +	/* Set duty cycle to maximum allowed and enable PWM output */
> +	pwm_init_state(ctx->pwm, &state);
> +	state.duty_cycle = ctx->pwm->args.period - 1;
> +	state.enabled = true;
>
> -	/* Enbale PWM output */
> -	ret = pwm_enable(ctx->pwm);
> +	ret = pwm_apply_state(ctx->pwm, &state);
>  	if (ret) {
> -		dev_err(&pdev->dev, "Failed to enable PWM\n");
> +		dev_err(&pdev->dev, "Failed to configure PWM\n");
>  		return ret;
>  	}
>
> @@ -266,8 +244,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  						       ctx, pwm_fan_groups);
>  	if (IS_ERR(hwmon)) {
>  		dev_err(&pdev->dev, "Failed to register hwmon device\n");
> -		pwm_disable(ctx->pwm);
> -		return PTR_ERR(hwmon);
> +		ret = PTR_ERR(hwmon);
> +		goto err_pwm_disable;
>  	}
>
>  	ret = pwm_fan_of_get_cooling_data(&pdev->dev, ctx);
> @@ -282,14 +260,20 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  		if (IS_ERR(cdev)) {
>  			dev_err(&pdev->dev,
>  				"Failed to register pwm-fan as cooling device");
> -			pwm_disable(ctx->pwm);
> -			return PTR_ERR(cdev);
> +			ret = PTR_ERR(cdev);
> +			goto err_pwm_disable;
>  		}
>  		ctx->cdev = cdev;
>  		thermal_cdev_update(cdev);
>  	}
>
>  	return 0;
> +
> +err_pwm_disable:
> +	state.enabled = false;
> +	pwm_apply_state(ctx->pwm, &state);
> +
> +	return ret;
>  }
>
>  static int pwm_fan_remove(struct platform_device *pdev)
>

[toc] | [prev] | [next] | [standalone]

#1629619

Hi,

On Monday, April 24, 2017 06:26:12 AM Guenter Roeck wrote:
> On 04/24/2017 06:13 AM, Bartlomiej Zolnierkiewicz wrote:
> > Switch pwm-fan driver to new atomic PWM API.
> >
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > ---
> > Depends on "[PATCH v2 0/3] pwm: pwm-samsung: fix suspend/resume support"
> > patchset (https://www.spinics.net/lists/kernel/msg2495209.html).
> >
> 
> Why would this driver, which does not depend on any Samsung code,
> depend on a Samsung specific fix ?

The above patchset contains pwm-fan specific patch:
"[PATCH v2 3/3] hwmon: pwm-fan: remove no longer needed suspend/resume code"
(https://www.spinics.net/lists/kernel/msg2495210.html).

It seems that the patchset name itself could be better,
sorry for this.

> Was this tested with hardware ?

Yes, on Exynos5422 based Odroid-XU3 board.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web