[PATCH v2 1/3] pwm: pwm-mediatek: Simplify error handling with dev_err_probe()

[PATCH v2 1/3] pwm: pwm-mediatek: Simplify error handling with dev_err_probe()

[AngeloGioacchino Del Regno]

Use dev_err_probe() to simplify handling errors in pwm_mediatek_probe().

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pwm/pwm-mediatek.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 0d4dd80e9f07..c7d5ca09a684 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -227,18 +227,14 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	pc->clk_top = devm_clk_get(&pdev->dev, "top");
-	if (IS_ERR(pc->clk_top)) {
-		dev_err(&pdev->dev, "clock: top fail: %ld\n",
-			PTR_ERR(pc->clk_top));
-		return PTR_ERR(pc->clk_top);
-	}
+	if (IS_ERR(pc->clk_top))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_top),
+				     "clock: top failed\n");
 
 	pc->clk_main = devm_clk_get(&pdev->dev, "main");
-	if (IS_ERR(pc->clk_main)) {
-		dev_err(&pdev->dev, "clock: main fail: %ld\n",
-			PTR_ERR(pc->clk_main));
-		return PTR_ERR(pc->clk_main);
-	}
+	if (IS_ERR(pc->clk_main))
+		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_main),
+				     "clock: main failed\n");
 
 	for (i = 0; i < pc->soc->num_pwms; i++) {
 		char name[8];
@@ -246,11 +242,9 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 		snprintf(name, sizeof(name), "pwm%d", i + 1);
 
 		pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
-		if (IS_ERR(pc->clk_pwms[i])) {
-			dev_err(&pdev->dev, "clock: %s fail: %ld\n",
-				name, PTR_ERR(pc->clk_pwms[i]));
-			return PTR_ERR(pc->clk_pwms[i]);
-		}
+		if (IS_ERR(pc->clk_pwms[i]))
+			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i]),
+					     "clock: %s failed\n", name);
 	}
 
 	pc->chip.dev = &pdev->dev;
@@ -258,10 +252,8 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 	pc->chip.npwm = pc->soc->num_pwms;
 
 	ret = devm_pwmchip_add(&pdev->dev, &pc->chip);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "pwmchip_add() failed\n");
 
 	return 0;
 }
-- 
2.33.1

[PATCH v2 2/3] pwm: pwm-mediatek: Allocate clk_pwms with devm_kmalloc_array

[AngeloGioacchino Del Regno]

Switch from devm_kcalloc to devm_kmalloc_array when allocating clk_pwms,
as this structure is being filled right after allocating it, hence
there is no need to zero it out beforehand.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pwm/pwm-mediatek.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index c7d5ca09a684..6b39f3d69e41 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -221,7 +221,7 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	pc->clk_pwms = devm_kcalloc(&pdev->dev, pc->soc->num_pwms,
+	pc->clk_pwms = devm_kmalloc_array(&pdev->dev, pc->soc->num_pwms,
 				    sizeof(*pc->clk_pwms), GFP_KERNEL);
 	if (!pc->clk_pwms)
 		return -ENOMEM;
-- 
2.33.1

[PATCH v2 3/3] pwm: pwm-mediatek: Beautify error messages text

[AngeloGioacchino Del Regno]

As a cherry-on-top cleanup, make error messages clearer to read
by changing instances of "clock: XXXX failed" to a more readable
"Failed to get XXXX clock". Also add "of" to unsupported period
error.

This is purely a cosmetic change; no "real" functional changes.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/pwm/pwm-mediatek.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 6b39f3d69e41..568b13a48717 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -146,7 +146,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (clkdiv > PWM_CLK_DIV_MAX) {
 		pwm_mediatek_clk_disable(chip, pwm);
-		dev_err(chip->dev, "period %d not supported\n", period_ns);
+		dev_err(chip->dev, "period of %d ns not supported\n", period_ns);
 		return -EINVAL;
 	}
 
@@ -229,12 +229,12 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 	pc->clk_top = devm_clk_get(&pdev->dev, "top");
 	if (IS_ERR(pc->clk_top))
 		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_top),
-				     "clock: top failed\n");
+				     "Failed to get top clock\n");
 
 	pc->clk_main = devm_clk_get(&pdev->dev, "main");
 	if (IS_ERR(pc->clk_main))
 		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_main),
-				     "clock: main failed\n");
+				     "Failed to get main clock\n");
 
 	for (i = 0; i < pc->soc->num_pwms; i++) {
 		char name[8];
@@ -244,7 +244,7 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 		pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
 		if (IS_ERR(pc->clk_pwms[i]))
 			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i]),
-					     "clock: %s failed\n", name);
+					     "Failed to get %s clock\n", name);
 	}
 
 	pc->chip.dev = &pdev->dev;
-- 
2.33.1

Re: [PATCH v2 3/3] pwm: pwm-mediatek: Beautify error messages text

[Macpaul Lin]

On 2/14/22 10:03 PM, AngeloGioacchino Del Regno wrote:
> As a cherry-on-top cleanup, make error messages clearer to read
> by changing instances of "clock: XXXX failed" to a more readable
> "Failed to get XXXX clock". Also add "of" to unsupported period
> error.
> 
> This is purely a cosmetic change; no "real" functional changes.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/pwm/pwm-mediatek.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 6b39f3d69e41..568b13a48717 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -146,7 +146,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   
>   	if (clkdiv > PWM_CLK_DIV_MAX) {
>   		pwm_mediatek_clk_disable(chip, pwm);
> -		dev_err(chip->dev, "period %d not supported\n", period_ns);
> +		dev_err(chip->dev, "period of %d ns not supported\n", period_ns);
>   		return -EINVAL;
>   	}
>   
> @@ -229,12 +229,12 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
>   	pc->clk_top = devm_clk_get(&pdev->dev, "top");
>   	if (IS_ERR(pc->clk_top))
>   		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_top),
> -				     "clock: top failed\n");
> +				     "Failed to get top clock\n");
>   
>   	pc->clk_main = devm_clk_get(&pdev->dev, "main");
>   	if (IS_ERR(pc->clk_main))
>   		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_main),
> -				     "clock: main failed\n");
> +				     "Failed to get main clock\n");
>   
>   	for (i = 0; i < pc->soc->num_pwms; i++) {
>   		char name[8];
> @@ -244,7 +244,7 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
>   		pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
>   		if (IS_ERR(pc->clk_pwms[i]))
>   			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i]),
> -					     "clock: %s failed\n", name);
> +					     "Failed to get %s clock\n", name);
>   	}
>   
>   	pc->chip.dev = &pdev->dev;
> 

The format of these debug messages "clock: top" or "clock: main" is  
meant to keep both human and machine's readability at the same time.
This kind of format is much more easier to parse by scripts, which the  
driver's category and sub nodes are separated by delimiters . If a fail  
log has been provided, the script could indicated where the issue might  
exists probably. Device vender, field application engineer, and driver  
maintainer could be able to write and use the error log parser before  
debugging.

I'm not sure if this kind of format will be better. Like, "Failed to get  
clock: %s".

If most people like this kind of solution ("Failed to get clock: %s\n"),  
then you can have the reviewed-by tag.
Thanks!

Reviewed-by: Macpaul Lin <macpaul.lin@mediatek.com>

Regards, :)
Macpaul Lin

Re: [PATCH v2 1/3] pwm: pwm-mediatek: Simplify error handling with dev_err_probe()

[Thierry Reding]

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

On Mon, Feb 14, 2022 at 03:03:37PM +0100, AngeloGioacchino Del Regno wrote:
> Use dev_err_probe() to simplify handling errors in pwm_mediatek_probe().
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/pwm/pwm-mediatek.c | 30 +++++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH v2 2/3] pwm: pwm-mediatek: Allocate clk_pwms with devm_kmalloc_array

[Thierry Reding]

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

On Mon, Feb 14, 2022 at 03:03:38PM +0100, AngeloGioacchino Del Regno wrote:
> Switch from devm_kcalloc to devm_kmalloc_array when allocating clk_pwms,
> as this structure is being filled right after allocating it, hence
> there is no need to zero it out beforehand.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/pwm/pwm-mediatek.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

Thierry

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

Re: [PATCH v2 3/3] pwm: pwm-mediatek: Beautify error messages text

[Thierry Reding]

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

On Tue, Feb 15, 2022 at 02:47:33PM +0800, Macpaul Lin wrote:
> On 2/14/22 10:03 PM, AngeloGioacchino Del Regno wrote:
> > As a cherry-on-top cleanup, make error messages clearer to read
> > by changing instances of "clock: XXXX failed" to a more readable
> > "Failed to get XXXX clock". Also add "of" to unsupported period
> > error.
> > 
> > This is purely a cosmetic change; no "real" functional changes.
> > 
> > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> >   drivers/pwm/pwm-mediatek.c | 8 ++++----
> >   1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> > index 6b39f3d69e41..568b13a48717 100644
> > --- a/drivers/pwm/pwm-mediatek.c
> > +++ b/drivers/pwm/pwm-mediatek.c
> > @@ -146,7 +146,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >   	if (clkdiv > PWM_CLK_DIV_MAX) {
> >   		pwm_mediatek_clk_disable(chip, pwm);
> > -		dev_err(chip->dev, "period %d not supported\n", period_ns);
> > +		dev_err(chip->dev, "period of %d ns not supported\n", period_ns);
> >   		return -EINVAL;
> >   	}
> > @@ -229,12 +229,12 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
> >   	pc->clk_top = devm_clk_get(&pdev->dev, "top");
> >   	if (IS_ERR(pc->clk_top))
> >   		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_top),
> > -				     "clock: top failed\n");
> > +				     "Failed to get top clock\n");
> >   	pc->clk_main = devm_clk_get(&pdev->dev, "main");
> >   	if (IS_ERR(pc->clk_main))
> >   		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_main),
> > -				     "clock: main failed\n");
> > +				     "Failed to get main clock\n");
> >   	for (i = 0; i < pc->soc->num_pwms; i++) {
> >   		char name[8];
> > @@ -244,7 +244,7 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
> >   		pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
> >   		if (IS_ERR(pc->clk_pwms[i]))
> >   			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i]),
> > -					     "clock: %s failed\n", name);
> > +					     "Failed to get %s clock\n", name);
> >   	}
> >   	pc->chip.dev = &pdev->dev;
> > 
> 
> The format of these debug messages "clock: top" or "clock: main" is meant to
> keep both human and machine's readability at the same time.
> This kind of format is much more easier to parse by scripts, which the
> driver's category and sub nodes are separated by delimiters . If a fail log
> has been provided, the script could indicated where the issue might exists
> probably. Device vender, field application engineer, and driver maintainer
> could be able to write and use the error log parser before debugging.

Does such a script truly exist? Given that most error messages don't
follow this format, I would expect it to be only very marginally useful.
Typically the prefix that the dev_printk() variants add is enough
information to deduct where the error happens, at which point it's back
to good old grep to find the matching string to localize exactly.

> I'm not sure if this kind of format will be better. Like, "Failed to get
> clock: %s".
> 
> If most people like this kind of solution ("Failed to get clock: %s\n"),
> then you can have the reviewed-by tag.
> Thanks!
> 
> Reviewed-by: Macpaul Lin <macpaul.lin@mediatek.com>

I'm going to assume that the scriptability is a theoretical argument, so
I'll take this. Let me know if you do rely on the exact format and I can
drop this again.

Thierry

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