[PATCH] pwm: sti: fix error handling

[PATCH] pwm: sti: fix error handling

[Uwe Kleine-König]

This commit fixes several faults:

 - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
   by calling clk_put().
 - A clk that was prepared must be unprepared.
 - The .remove callback isn't supposed to call pwm_disable().
 - The necessary resources needed by the PWM must not be freed before
   pwmchip_remove() was called.

Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 37 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 1508616d794c..f89f8cbdfdfc 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	ret = clk_prepare(pc->pwm_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
-		return ret;
+		goto err_pwm_clk_prepare;
 	}
 
 skip_pwm:
@@ -615,13 +615,14 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	pc->cpt_clk = of_clk_get_by_name(dev->of_node, "capture");
 	if (IS_ERR(pc->cpt_clk)) {
 		dev_err(dev, "failed to get PWM capture clock\n");
-		return PTR_ERR(pc->cpt_clk);
+		ret = PTR_ERR(pc->cpt_clk);
+		goto err_cpt_clk_get;
 	}
 
 	ret = clk_prepare(pc->cpt_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
-		return ret;
+		goto err_cpt_clk_prepare;
 	}
 
 skip_cpt:
@@ -632,9 +633,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
-		clk_unprepare(pc->pwm_clk);
-		clk_unprepare(pc->cpt_clk);
-		return ret;
+		goto err_pwmchip_add;
 	}
 
 	for (i = 0; i < cdata->cpt_num_devs; i++) {
@@ -653,20 +652,46 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, pc);
 
 	return 0;
+
+err_pwmchip_add:
+
+	if (cdata->cpt_num_devs) {
+		clk_unprepare(pc->cpt_clk);
+err_cpt_clk_prepare:
+
+		clk_put(pc->cpt_clk);
+	}
+err_cpt_clk_get:
+
+	if (cdata->pwm_num_devs) {
+		clk_unprepare(pc->pwm_clk);
+err_pwm_clk_prepare:
+
+		clk_put(pc->pwm_clk);
+	}
+	return ret;
 }
 
 static int sti_pwm_remove(struct platform_device *pdev)
 {
 	struct sti_pwm_chip *pc = platform_get_drvdata(pdev);
-	unsigned int i;
+	int ret;
+
+	ret = pwmchip_remove(&pc->chip);
+	if (ret)
+		return ret;
 
-	for (i = 0; i < pc->cdata->pwm_num_devs; i++)
-		pwm_disable(&pc->chip.pwms[i]);
+	if (pc->cdata->cpt_num_devs) {
+		clk_unprepare(pc->cpt_clk);
+		clk_put(pc->cpt_clk);
+	}
 
-	clk_unprepare(pc->pwm_clk);
-	clk_unprepare(pc->cpt_clk);
+	if (pc->cdata->pwm_num_devs) {
+		clk_unprepare(pc->pwm_clk);
+		clk_put(pc->pwm_clk);
+	}
 
-	return pwmchip_remove(&pc->chip);
+	return 0;
 }
 
 static const struct of_device_id sti_pwm_of_match[] = {
-- 
2.28.0

Re: [PATCH] pwm: sti: fix error handling

[Lee Jones]

On Tue, 13 Oct 2020, Uwe Kleine-König wrote:

> This commit fixes several faults:
> 
>  - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
>    by calling clk_put().
>  - A clk that was prepared must be unprepared.
>  - The .remove callback isn't supposed to call pwm_disable().
>  - The necessary resources needed by the PWM must not be freed before
>    pwmchip_remove() was called.
> 
> Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
>  1 file changed, 37 insertions(+), 12 deletions(-)

Sorry for the delay, this ended up in spam.

> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 1508616d794c..f89f8cbdfdfc 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	ret = clk_prepare(pc->pwm_clk);
>  	if (ret) {
>  		dev_err(dev, "failed to prepare clock\n");
> -		return ret;
> +		goto err_pwm_clk_prepare;

I would prefer these to indicate the intention, rather than were they
were called from.  So err_put_cpt_clk for this one, etc.

>  	}
>  
>  skip_pwm:
> @@ -615,13 +615,14 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	pc->cpt_clk = of_clk_get_by_name(dev->of_node, "capture");
>  	if (IS_ERR(pc->cpt_clk)) {
>  		dev_err(dev, "failed to get PWM capture clock\n");
> -		return PTR_ERR(pc->cpt_clk);
> +		ret = PTR_ERR(pc->cpt_clk);
> +		goto err_cpt_clk_get;

err_unprepare_pwm_clk, etc.

>  	}
>  
>  	ret = clk_prepare(pc->cpt_clk);
>  	if (ret) {
>  		dev_err(dev, "failed to prepare clock\n");
> -		return ret;
> +		goto err_cpt_clk_prepare;
>  	}
>  
>  skip_cpt:
> @@ -632,9 +633,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  
>  	ret = pwmchip_add(&pc->chip);
>  	if (ret < 0) {
> -		clk_unprepare(pc->pwm_clk);
> -		clk_unprepare(pc->cpt_clk);
> -		return ret;
> +		goto err_pwmchip_add;
>  	}
>  
>  	for (i = 0; i < cdata->cpt_num_devs; i++) {
> @@ -653,20 +652,46 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, pc);
>  
>  	return 0;
> +
> +err_pwmchip_add:
> +
> +	if (cdata->cpt_num_devs) {

No need for the evaluations, clk_*() calls are NULL safe.

> +		clk_unprepare(pc->cpt_clk);
> +err_cpt_clk_prepare:
> +
> +		clk_put(pc->cpt_clk);
> +	}
> +err_cpt_clk_get:
> +
> +	if (cdata->pwm_num_devs) {
> +		clk_unprepare(pc->pwm_clk);
> +err_pwm_clk_prepare:
> +
> +		clk_put(pc->pwm_clk);
> +	}
> +	return ret;
>  }
>  
>  static int sti_pwm_remove(struct platform_device *pdev)
>  {
>  	struct sti_pwm_chip *pc = platform_get_drvdata(pdev);
> -	unsigned int i;
> +	int ret;
> +
> +	ret = pwmchip_remove(&pc->chip);
> +	if (ret)
> +		return ret;
>  
> -	for (i = 0; i < pc->cdata->pwm_num_devs; i++)
> -		pwm_disable(&pc->chip.pwms[i]);
> +	if (pc->cdata->cpt_num_devs) {
> +		clk_unprepare(pc->cpt_clk);
> +		clk_put(pc->cpt_clk);
> +	}
>  
> -	clk_unprepare(pc->pwm_clk);
> -	clk_unprepare(pc->cpt_clk);
> +	if (pc->cdata->pwm_num_devs) {
> +		clk_unprepare(pc->pwm_clk);
> +		clk_put(pc->pwm_clk);
> +	}

Again, no need for the evaluations, just call clk_{unprepare,put}.

> -	return pwmchip_remove(&pc->chip);
> +	return 0;
>  }
>  
>  static const struct of_device_id sti_pwm_of_match[] = {

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] pwm: sti: fix error handling

[Uwe Kleine-König]

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

Hello Lee,

On Fri, Nov 06, 2020 at 08:29:14AM +0000, Lee Jones wrote:
> On Tue, 13 Oct 2020, Uwe Kleine-König wrote:
> 
> > This commit fixes several faults:
> > 
> >  - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
> >    by calling clk_put().
> >  - A clk that was prepared must be unprepared.
> >  - The .remove callback isn't supposed to call pwm_disable().
> >  - The necessary resources needed by the PWM must not be freed before
> >    pwmchip_remove() was called.
> > 
> > Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > ---
> >  drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 37 insertions(+), 12 deletions(-)
> 
> Sorry for the delay, this ended up in spam.
> 
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > index 1508616d794c..f89f8cbdfdfc 100644
> > --- a/drivers/pwm/pwm-sti.c
> > +++ b/drivers/pwm/pwm-sti.c
> > @@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> >  	ret = clk_prepare(pc->pwm_clk);
> >  	if (ret) {
> >  		dev_err(dev, "failed to prepare clock\n");
> > -		return ret;
> > +		goto err_pwm_clk_prepare;
> 
> I would prefer these to indicate the intention, rather than were they
> were called from.  So err_put_cpt_clk for this one, etc.

This might be subjective, but I like it better the way I did it. My
pattern is:

	ret = get_resource_A();
	if (ret)
		goto err_A;

	ret = get_resource_B();
	if (ret)
		goto err_B;

	...

	put_resource_B();
err_B:
	
	put_resource_A();
err_A:

	return ret;

This way just looking at on get_resource_$X block it is obvious that the
picked label is right and in the error handling blocks that's obvious,
too.

However with the (admittedly more common) style you prefer it is:

	ret = get_resource_A();
	if (ret)
		goto return_ret; // or just: return ret

	ret = get_resource_B();
	if (ret)
		goto put_A;

	...

put_B:
	put_resource_B();

put_A:
	put_resource_A();

return_ret:
	return ret;

You have to check the previous block to see that put_A is right for
the error path of get_resource_B(). In this trivial example you have to
look back 6 instead of 2 lines. For more complex stuff it tends to be
3 lines for my approach (one more for the error message, and so still in
the same logical block) but might be considerably bigger for the common
approach. The usual amount of context in patches is 3 lines. And if you
add another resource allocation between A and B you have to adapt the
error path in B which is somewhat unrelated. So a patch adding A2 looks
for my approach:

@@ ...
 	if (ret)
 		goto err_A;
 
+	ret = get_resource_A2();
+	if (ret)
+		goto err_A2;
+
 	ret = get_resource_B();
 	if (ret)
 		goto err_B;
@@ ...
 	put_resource_B();
 err_B:
 	
+	put_resource_A2();
+err_A2:
+
 	put_resource_A()
 err_A:
 
Here you see that the right error label is used in the new error path
and that it is placed correctly between err_B and err_A.

For your preferred approach the patch looks as follows:

@@ ...
 	if (ret)
 		goto return_ret;
 
+	ret = get_resource_A2();
+	if (ret)
+		goto put_A;
+
 	ret = get_resource_B();
 	if (ret)
-		goto put_A;
+		goto put_A2;
 
 	...
@@ ...
 put_B: 
 	put_resource_B();
 
+put_A2:
+	put_resource_A2;
+
 put_A:
 	put_resource_A();
 
Note you cannot see by just looking at the patch that goto put_A is
right. (Well, if you assume that the old code is correct see that just
before put_A B is freed which matches what just happens after your new
get_resource_A2, but that's considerably more complicated.) Also you
have to modify the goto for B.

This is in my eyes ugly enough to justify my preference.

> > +err_pwmchip_add:
> > +
> > +	if (cdata->cpt_num_devs) {
> 
> No need for the evaluations, clk_*() calls are NULL safe.

Yes, I added that to match the allocation pattern. I don't feel strong
here. (But as a side note: I don't like the gotos in .probe. They are
not used to do (plain) error handling and so they complicate following
the code flow.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH] pwm: sti: fix error handling

[Lee Jones]

On Fri, 06 Nov 2020, Uwe Kleine-König wrote:

> Hello Lee,
> 
> On Fri, Nov 06, 2020 at 08:29:14AM +0000, Lee Jones wrote:
> > On Tue, 13 Oct 2020, Uwe Kleine-König wrote:
> > 
> > > This commit fixes several faults:
> > > 
> > >  - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
> > >    by calling clk_put().
> > >  - A clk that was prepared must be unprepared.
> > >  - The .remove callback isn't supposed to call pwm_disable().
> > >  - The necessary resources needed by the PWM must not be freed before
> > >    pwmchip_remove() was called.
> > > 
> > > Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
> > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > ---
> > >  drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 37 insertions(+), 12 deletions(-)
> > 
> > Sorry for the delay, this ended up in spam.
> > 
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > index 1508616d794c..f89f8cbdfdfc 100644
> > > --- a/drivers/pwm/pwm-sti.c
> > > +++ b/drivers/pwm/pwm-sti.c
> > > @@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > >  	ret = clk_prepare(pc->pwm_clk);
> > >  	if (ret) {
> > >  		dev_err(dev, "failed to prepare clock\n");
> > > -		return ret;
> > > +		goto err_pwm_clk_prepare;
> > 
> > I would prefer these to indicate the intention, rather than were they
> > were called from.  So err_put_cpt_clk for this one, etc.
> 
> This might be subjective, but I like it better the way I did it. My
> pattern is:
> 
> 	ret = get_resource_A();
> 	if (ret)
> 		goto err_A;
> 
> 	ret = get_resource_B();
> 	if (ret)
> 		goto err_B;
> 
> 	...
> 
> 	put_resource_B();
> err_B:
> 	
> 	put_resource_A();
> err_A:
> 
> 	return ret;
> 
> This way just looking at on get_resource_$X block it is obvious that the
> picked label is right and in the error handling blocks that's obvious,
> too.
> 
> However with the (admittedly more common) style you prefer it is:
> 
> 	ret = get_resource_A();
> 	if (ret)
> 		goto return_ret; // or just: return ret
> 
> 	ret = get_resource_B();
> 	if (ret)
> 		goto put_A;
> 
> 	...
> 
> put_B:
> 	put_resource_B();
> 
> put_A:
> 	put_resource_A();
> 
> return_ret:
> 	return ret;
> 
> You have to check the previous block to see that put_A is right for
> the error path of get_resource_B(). In this trivial example you have to
> look back 6 instead of 2 lines. For more complex stuff it tends to be
> 3 lines for my approach (one more for the error message, and so still in
> the same logical block) but might be considerably bigger for the common
> approach. The usual amount of context in patches is 3 lines. And if you
> add another resource allocation between A and B you have to adapt the
> error path in B which is somewhat unrelated. So a patch adding A2 looks
> for my approach:
> 
> @@ ...
>  	if (ret)
>  		goto err_A;
>  
> +	ret = get_resource_A2();
> +	if (ret)
> +		goto err_A2;
> +
>  	ret = get_resource_B();
>  	if (ret)
>  		goto err_B;
> @@ ...
>  	put_resource_B();
>  err_B:
>  	
> +	put_resource_A2();
> +err_A2:
> +
>  	put_resource_A()
>  err_A:
>  
> Here you see that the right error label is used in the new error path
> and that it is placed correctly between err_B and err_A.
> 
> For your preferred approach the patch looks as follows:
> 
> @@ ...
>  	if (ret)
>  		goto return_ret;
>  
> +	ret = get_resource_A2();
> +	if (ret)
> +		goto put_A;
> +
>  	ret = get_resource_B();
>  	if (ret)
> -		goto put_A;
> +		goto put_A2;
>  
>  	...
> @@ ...
>  put_B: 
>  	put_resource_B();
>  
> +put_A2:
> +	put_resource_A2;
> +
>  put_A:
>  	put_resource_A();
>  
> Note you cannot see by just looking at the patch that goto put_A is
> right. (Well, if you assume that the old code is correct see that just
> before put_A B is freed which matches what just happens after your new
> get_resource_A2, but that's considerably more complicated.) Also you
> have to modify the goto for B.
> 
> This is in my eyes ugly enough to justify my preference.

Wow, you sure put a lot of effort into that. :)

I'll leave it up to ST and Thierry to have the final say.

> > > +err_pwmchip_add:
> > > +
> > > +	if (cdata->cpt_num_devs) {
> > 
> > No need for the evaluations, clk_*() calls are NULL safe.
> 
> Yes, I added that to match the allocation pattern. I don't feel strong

Right, but my point is, you don't have to.

It's added complexity/lines for no clear value.

I would personally simplify it all and remove them.

It also saves the ugliness of goto'ing into the middle of an if().

> here. (But as a side note: I don't like the gotos in .probe. They are
> not used to do (plain) error handling and so they complicate following
> the code flow.)

I think error handling is where gotos excel personally.
-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH] pwm: sti: fix error handling

[Thierry Reding]

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

On Fri, Nov 06, 2020 at 10:29:08AM +0000, Lee Jones wrote:
> On Fri, 06 Nov 2020, Uwe Kleine-König wrote:
> 
> > Hello Lee,
> > 
> > On Fri, Nov 06, 2020 at 08:29:14AM +0000, Lee Jones wrote:
> > > On Tue, 13 Oct 2020, Uwe Kleine-König wrote:
> > > 
> > > > This commit fixes several faults:
> > > > 
> > > >  - Iff a clk was returned by of_clk_get_by_name() it must be dereferenced
> > > >    by calling clk_put().
> > > >  - A clk that was prepared must be unprepared.
> > > >  - The .remove callback isn't supposed to call pwm_disable().
> > > >  - The necessary resources needed by the PWM must not be freed before
> > > >    pwmchip_remove() was called.
> > > > 
> > > > Fixes: 378fe115d19d ("pwm: sti: Add new driver for ST's PWM IP")
> > > > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> > > > ---
> > > >  drivers/pwm/pwm-sti.c | 49 ++++++++++++++++++++++++++++++++-----------
> > > >  1 file changed, 37 insertions(+), 12 deletions(-)
> > > 
> > > Sorry for the delay, this ended up in spam.
> > > 
> > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > index 1508616d794c..f89f8cbdfdfc 100644
> > > > --- a/drivers/pwm/pwm-sti.c
> > > > +++ b/drivers/pwm/pwm-sti.c
> > > > @@ -605,7 +605,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> > > >  	ret = clk_prepare(pc->pwm_clk);
> > > >  	if (ret) {
> > > >  		dev_err(dev, "failed to prepare clock\n");
> > > > -		return ret;
> > > > +		goto err_pwm_clk_prepare;
> > > 
> > > I would prefer these to indicate the intention, rather than were they
> > > were called from.  So err_put_cpt_clk for this one, etc.
> > 
> > This might be subjective, but I like it better the way I did it. My
> > pattern is:
> > 
> > 	ret = get_resource_A();
> > 	if (ret)
> > 		goto err_A;
> > 
> > 	ret = get_resource_B();
> > 	if (ret)
> > 		goto err_B;
> > 
> > 	...
> > 
> > 	put_resource_B();
> > err_B:
> > 	
> > 	put_resource_A();
> > err_A:
> > 
> > 	return ret;
> > 
> > This way just looking at on get_resource_$X block it is obvious that the
> > picked label is right and in the error handling blocks that's obvious,
> > too.
> > 
> > However with the (admittedly more common) style you prefer it is:
> > 
> > 	ret = get_resource_A();
> > 	if (ret)
> > 		goto return_ret; // or just: return ret
> > 
> > 	ret = get_resource_B();
> > 	if (ret)
> > 		goto put_A;
> > 
> > 	...
> > 
> > put_B:
> > 	put_resource_B();
> > 
> > put_A:
> > 	put_resource_A();
> > 
> > return_ret:
> > 	return ret;
> > 
> > You have to check the previous block to see that put_A is right for
> > the error path of get_resource_B(). In this trivial example you have to
> > look back 6 instead of 2 lines. For more complex stuff it tends to be
> > 3 lines for my approach (one more for the error message, and so still in
> > the same logical block) but might be considerably bigger for the common
> > approach. The usual amount of context in patches is 3 lines. And if you
> > add another resource allocation between A and B you have to adapt the
> > error path in B which is somewhat unrelated. So a patch adding A2 looks
> > for my approach:
> > 
> > @@ ...
> >  	if (ret)
> >  		goto err_A;
> >  
> > +	ret = get_resource_A2();
> > +	if (ret)
> > +		goto err_A2;
> > +
> >  	ret = get_resource_B();
> >  	if (ret)
> >  		goto err_B;
> > @@ ...
> >  	put_resource_B();
> >  err_B:
> >  	
> > +	put_resource_A2();
> > +err_A2:
> > +
> >  	put_resource_A()
> >  err_A:
> >  
> > Here you see that the right error label is used in the new error path
> > and that it is placed correctly between err_B and err_A.
> > 
> > For your preferred approach the patch looks as follows:
> > 
> > @@ ...
> >  	if (ret)
> >  		goto return_ret;
> >  
> > +	ret = get_resource_A2();
> > +	if (ret)
> > +		goto put_A;
> > +
> >  	ret = get_resource_B();
> >  	if (ret)
> > -		goto put_A;
> > +		goto put_A2;
> >  
> >  	...
> > @@ ...
> >  put_B: 
> >  	put_resource_B();
> >  
> > +put_A2:
> > +	put_resource_A2;
> > +
> >  put_A:
> >  	put_resource_A();
> >  
> > Note you cannot see by just looking at the patch that goto put_A is
> > right. (Well, if you assume that the old code is correct see that just
> > before put_A B is freed which matches what just happens after your new
> > get_resource_A2, but that's considerably more complicated.) Also you
> > have to modify the goto for B.
> > 
> > This is in my eyes ugly enough to justify my preference.
> 
> Wow, you sure put a lot of effort into that. :)
> 
> I'll leave it up to ST and Thierry to have the final say.

I agree with Lee on this one, so I've applied the patch and touched up
the label names while at it.

Thierry

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

Re: [PATCH] pwm: sti: fix error handling

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 391 bytes --]

Hello Thierry,

On 11/11/20 8:28 PM, Thierry Reding wrote:
> I agree with Lee on this one, so I've applied the patch and touched up
> the label names while at it.

Have you tried to follow my reasoning? I consider it irritating that you 
overrule the preference I justified in such detail by applying a patch 
with me as author and the error handling variant I opposed to.

Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sti: fix error handling

[Thierry Reding]

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

On Wed, Nov 11, 2020 at 08:43:18PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On 11/11/20 8:28 PM, Thierry Reding wrote:
> > I agree with Lee on this one, so I've applied the patch and touched up
> > the label names while at it.
> 
> Have you tried to follow my reasoning?

Yes I did, and I happen to disagree.

>                                        I consider it irritating that you
> overrule the preference I justified in such detail by applying a patch with
> me as author and the error handling variant I opposed to.

Well, it was either this or have you respin the patch. I didn't think
you'd mind, so I did that work myself but still wanted to credit you for
fixing this in the first place (the names of the labels really don't
mean much for authorship in my opinion).

If you prefer I can just remove the patch and you can do this yourself.

Thierry

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

Re: [PATCH] pwm: sti: fix error handling

[Uwe Kleine-König]

[-- Attachment #1.1.1: Type: text/plain, Size: 1180 bytes --]

Hello Thierry,

[dropped Ajit Pal Singh from Cc: as the address doesn't seem to exist 
any more]

On 11/11/20 8:52 PM, Thierry Reding wrote:
> On Wed, Nov 11, 2020 at 08:43:18PM +0100, Uwe Kleine-König wrote:
>> On 11/11/20 8:28 PM, Thierry Reding wrote:
>>> I agree with Lee on this one, so I've applied the patch and touched up
>>> the label names while at it.
>>
>> Have you tried to follow my reasoning?
> 
> Yes I did, and I happen to disagree.
> 
>>                                         I consider it irritating that you
>> overrule the preference I justified in such detail by applying a patch with
>> me as author and the error handling variant I opposed to.
> 
> Well, it was either this or have you respin the patch. I didn't think
> you'd mind, so I did that work myself but still wanted to credit you for
> fixing this in the first place (the names of the labels really don't
> mean much for authorship in my opinion).
> 
> If you prefer I can just remove the patch and you can do this yourself.

I sent the version of the patch that does it the way I want. So yes, 
please drop the modified patch from your queue.

Best regards
Uwe

[-- Attachment #1.1.2: OpenPGP_0xE2DCDD9132669BD6.asc --]
[-- Type: application/pgp-keys, Size: 152729 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] pwm: sti: fix error handling

[Thierry Reding]

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

On Wed, Nov 11, 2020 at 09:39:51PM +0100, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> [dropped Ajit Pal Singh from Cc: as the address doesn't seem to exist any
> more]
> 
> On 11/11/20 8:52 PM, Thierry Reding wrote:
> > On Wed, Nov 11, 2020 at 08:43:18PM +0100, Uwe Kleine-König wrote:
> > > On 11/11/20 8:28 PM, Thierry Reding wrote:
> > > > I agree with Lee on this one, so I've applied the patch and touched up
> > > > the label names while at it.
> > > 
> > > Have you tried to follow my reasoning?
> > 
> > Yes I did, and I happen to disagree.
> > 
> > >                                         I consider it irritating that you
> > > overrule the preference I justified in such detail by applying a patch with
> > > me as author and the error handling variant I opposed to.
> > 
> > Well, it was either this or have you respin the patch. I didn't think
> > you'd mind, so I did that work myself but still wanted to credit you for
> > fixing this in the first place (the names of the labels really don't
> > mean much for authorship in my opinion).
> > 
> > If you prefer I can just remove the patch and you can do this yourself.
> 
> I sent the version of the patch that does it the way I want. So yes, please
> drop the modified patch from your queue.

Alrighty then.

Thierry

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