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: 6272 bytes --]

On Mon, Sep 21, 2026 at 02:46:03PM +0200, Uwe Kleine-König wrote:
> Hello Thierry,
> 
> On Mon, Sep 21, 2026 at 11:38:41AM +0200, Thierry Reding wrote:
> > 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.
> 
> I often deal with bug reports by users where things fail without an
> error message[1]. So a usual thing I do is checking probe (and other)
> functions for silent error paths. As I fail to follow the (continously
> changing) set of functions that emit an error message, this comment is
> very useful for a me at least and I'd wish others would add such
> comments, too. (An IMHO fine thing here would be to let no generic
> resource getter function emit an error message, but that ship has
> sailed.)

I seem to remember that we discussed both options at the time and the
general concensus was that these functions should provide canonical
error messages since their whole purpose was to remove boilerplate and
about a quarter or so of the boilerplate was the error message, with the
added issue that error messages were all over the place. All of these
callsites are going to print an message for these errors, so might as
well add standard messages for these types of situations and save a
bunch of text. I think overall it's a win, but it comes at the cost of
people having to know that these already print errors.

> I'd be open for a shorter marker, that might even be machine-parsable.

I don't know if an extra marker would be all that helpful over a
comment. The beauty of the current solution is that the error handling
is reduced to just the check and the return value, everything else is
encapsulated into the helper.

There are semantic patches that check for these situations, but I
suspect not everyone runs those. I don't know if we can somehow make the
compiler warn about these situations.

> > >  		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?
> 
> Fine for me, will fix in the next submission.
> 
> Thanks for your feedback,
> Uwe
> 
> [1] last instance was just today, where on a bananapi USB didn't work
>     and /sys/kernel/debug/devices_deferred ended up containing:
> 
> 	1c13000.usb     platform: supplier 1c13400.phy not ready
> 	1c1c000.usb     platform: supplier 1c13400.phy not ready
> 	1c14400.usb     platform: supplier 1c13400.phy not ready
> 	1c13400.phy     platform: supplier axp20x-usb-power-supply not ready
> 	1c14000.usb     platform: supplier 1c13400.phy not ready
> 	1c1c400.usb     platform: supplier 1c13400.phy not ready
> 	axp20x-usb-power-supply
> 
>     It would be so easy[2] to add a useful debugging hint here
> 
> [2] https://lore.kernel.org/all/b699f8251afed736af454a981630926e45eb7da7.1789983244.git.ukleinek@debian.org/

I see. Yeah, I find it increasingly difficult to remember all the
context of a function call, whether it is that it prints an error
message or needs some lock to be held, sleeps or not. Sashiko has
been really good about pointing some of those things out.

Thierry

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