Re: [PATCH v2 1/3] pwm: tegra: Make use of dev_err_probe()

[Thierry Reding <thierry.reding@kernel.org>]

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

On Fri, Sep 18, 2026 at 04:33:45PM +0200, Uwe Kleine-König wrote:
> Usage of dev_err_probe() is more compact than dev_err()'s, emits the
> error code and handles -ENOMEM and -EPROBE_DEFER properly. Benefit from
> these improvements.
> 
> Also add a few messages in error paths that lacked an output before.
> 
> Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/pwm-tegra.c | 52 ++++++++++++++++++++++++-----------------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-tegra.c b/drivers/pwm/pwm-tegra.c
> index 5cdbe120ba2d..efb7ab60f602 100644
> --- a/drivers/pwm/pwm-tegra.c
> +++ b/drivers/pwm/pwm-tegra.c
> @@ -316,14 +316,15 @@ static const struct pwm_ops tegra_pwm_ops = {
>  
>  static int tegra_pwm_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct pwm_chip *chip;
>  	struct tegra_pwm_chip *pc;
>  	const struct tegra_pwm_soc *soc;
>  	int ret;
>  
> -	soc = of_device_get_match_data(&pdev->dev);
> +	soc = of_device_get_match_data(dev);
>  
> -	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_channels, sizeof(*pc));
> +	chip = devm_pwmchip_alloc(dev, soc->num_channels, sizeof(*pc));
>  	if (IS_ERR(chip))
>  		return PTR_ERR(chip);
>  	pc = to_tegra_pwm_chip(chip);
> @@ -331,28 +332,39 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  	pc->soc = soc;
>  
>  	pc->regs = devm_platform_ioremap_resource(pdev, 0);
> -	if (IS_ERR(pc->regs))
> +	if (IS_ERR(pc->regs)) {
> +		/*
> +		 * devm_platform_ioremap_resource() already emits an error
> +		 * message with CONFIG_HAS_IOMEM, so don't emit another message
> +		 * here.
> +		 */

Seems a bit counter-productive to leave comments like this. Function
comments should document what the function does and then people should
read those comments. Then we don't need to mention it every time we call
these functions.

>  		return PTR_ERR(pc->regs);
> +	}
>  
>  	platform_set_drvdata(pdev, chip);
>  
> -	pc->clk = devm_clk_get(&pdev->dev, NULL);
> +	pc->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(pc->clk))
> -		return PTR_ERR(pc->clk);
> +		return dev_err_probe(dev, PTR_ERR(pc->clk), "Failed to get clock\n");
>  
> -	ret = devm_tegra_core_dev_init_opp_table_common(&pdev->dev);
> -	if (ret)
> +	ret = devm_tegra_core_dev_init_opp_table_common(dev);
> +	if (ret) {
> +		/*
> +		 * devm_tegra_core_dev_init_opp_table_common() emits an error
> +		 * message most of the time, so don't add another.
> +		 */

Same here.

> @@ -385,17 +395,17 @@ static int tegra_pwm_probe(struct platform_device *pdev)
>  
>  	ret = pwmchip_add(chip);
>  	if (ret < 0) {
> -		dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret);
> +		dev_err_probe(dev, ret, "Adding pwmchip failed\n");

This stands out as very different from other error messages, so maybe
change this to something like "Failed to add PWM chip" for consistency?

Thierry

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