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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Bartlomiej Zolnierkiewicz]

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

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

[Guenter Roeck]

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)
>

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

[Bartlomiej Zolnierkiewicz]

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

Re: hwmon: pwm-fan: switch to new atomic PWM API

[Guenter Roeck]

On Mon, Apr 24, 2017 at 03:13:17PM +0200, 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).

I lost track where we are with this patch. The above series set did not make
it, as far as I can see. Is this patch truly dependent on that series, or
can it be applied separately ? It does apply cleanly.

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)

Re: hwmon: pwm-fan: switch to new atomic PWM API

[Bartlomiej Zolnierkiewicz]

On Friday, June 02, 2017 06:23:53 AM Guenter Roeck wrote:
> On Mon, Apr 24, 2017 at 03:13:17PM +0200, 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).
> 
> I lost track where we are with this patch. The above series set did not make
> it, as far as I can see. Is this patch truly dependent on that series, or
> can it be applied separately ? It does apply cleanly.

I think that it can be applied separately (the broken suspend/resume code
will continue to use the old API until the previous patchset is finally
merged and the code in question is removed altogether).

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

Re: hwmon: pwm-fan: switch to new atomic PWM API

[Guenter Roeck]

On Wed, Jun 07, 2017 at 03:38:44PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday, June 02, 2017 06:23:53 AM Guenter Roeck wrote:
> > On Mon, Apr 24, 2017 at 03:13:17PM +0200, 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).
> > 
> > I lost track where we are with this patch. The above series set did not make
> > it, as far as I can see. Is this patch truly dependent on that series, or
> > can it be applied separately ? It does apply cleanly.
> 
> I think that it can be applied separately (the broken suspend/resume code
> will continue to use the old API until the previous patchset is finally
> merged and the code in question is removed altogether).
> 

Ok. Applied to hwmon-next.

Thanks,
Guenter

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

[Thierry Reding]

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]