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 > > > Signed-off-by: Uwe Kleine-König > > > --- > > > 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