Re: [PATCH v4 09/10] iio: adc: ad7124: Add error reporting during probe

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 27 Nov 2024 15:59:38 +0100
Uwe Kleine-König <u.kleine-koenig@baylibre.com> wrote:

> A driver that silently fails to probe is annoying and hard to debug. So
> add messages in the error paths of the probe function.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Probably worth calling out in this patch description that you also
replace some dev_err() calls with dev_err_probe()

One comment inline.

Thanks,

Jonathan

> @@ -1007,36 +1008,42 @@ static int ad7124_probe(struct spi_device *spi)
>  
>  		ret = regulator_enable(st->vref[i]);
>  		if (ret)
> -			return ret;
> +			return dev_err_probe(dev, ret, "Failed to enable regulator #%d\n", i);
>  
>  		ret = devm_add_action_or_reset(&spi->dev, ad7124_reg_disable,
>  					       st->vref[i]);
>  		if (ret)
> -			return ret;
> +			return dev_err_probe(dev, ret, "Failed to register disable handler for regulator #%d\n", i);
>  	}
>  
>  	st->mclk = devm_clk_get_enabled(&spi->dev, "mclk");
>  	if (IS_ERR(st->mclk))
> -		return PTR_ERR(st->mclk);
> +		return dev_err_probe(dev, PTR_ERR(st->mclk), "Failed to get mclk\n");
>  
>  	ret = ad7124_soft_reset(st);
>  	if (ret < 0)
> +		/* ad7124_soft_reset() already emitted an error message */
I'd not bother adding these already emitted comments.
The only time we'd care is if someone else comes along and adds them. Hopefully we'd
catch that anyway in review, but even if don't it wouldn't matter a lot.
>  		return ret;
>  
>  	ret = ad7124_check_chip_id(st);
>  	if (ret)
> +		/* ad7124_check_chip_id() already emitted an error message */
>  		return ret;
>  
>  	ret = ad7124_setup(st);
>  	if (ret < 0)
> +		/* ad7124_setup() already emitted an error message */
>  		return ret;
>  
>  	ret = devm_ad_sd_setup_buffer_and_trigger(&spi->dev, indio_dev);
>  	if (ret < 0)
> -		return ret;
> +		return dev_err_probe(dev, ret, "Failed to setup triggers\n");
>  
> -	return devm_iio_device_register(&spi->dev, indio_dev);
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
>  
> +	return 0;
>  }
>  
>  static const struct of_device_id ad7124_of_match[] = {