diff mbox series

[v3,09/10] iio: adc: ad7124: Add error reporting during probe

Message ID 20241122113322.242875-21-u.kleine-koenig@baylibre.com (mailing list archive)
State New
Headers show
Series iio: adc: ad7124: Various fixes | expand

Commit Message

Uwe Kleine-König Nov. 22, 2024, 11:33 a.m. UTC 
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>
---
 drivers/iio/adc/ad7124.c | 78 +++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 36 deletions(-)

Comments

Trevor Gamblin Nov. 22, 2024, 4:44 p.m. UTC | #1 
On 2024-11-22 06:33, Uwe Kleine-König 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>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>
Andy Shevchenko Nov. 22, 2024, 8:25 p.m. UTC | #2 
On Fri, Nov 22, 2024 at 1:34 PM 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.

...

> +/* Only called during probe, so dev_err_probe() can be used */

It's a harmless comment, but I think dev_err_probe() name is good
enough to give such a hint.

...

> +/* Only called during probe, so dev_err_probe() can be used */

Ditto.

...

>         do {
>                 ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
>                 if (ret < 0)
> -                       return ret;
> +                       return dev_err_probe(&st->sd.spi->dev, ret, "Error reading status register\n");
>
>                 if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
>                         return 0;

>                 usleep_range(100, 2000);

Side note 1: fsleep() ?

>         } while (--timeout);

Side note 2: maybe using read_poll_timeout() from iopoll.h makes this
better looking?

...

>  static int ad7124_check_chip_id(struct ad7124_state *st)

>         ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
>         if (ret < 0)
> -               return ret;
> +               return dev_err_probe(&st->sd.spi->dev, ret,
> +                                    "Failure to read ID register\n");

Why not temporary for the struct device, will be the same LoCs now,
but might help in the future if more callers will need this parameter.

>
>         chip_id = AD7124_DEVICE_ID_GET(readval);
>         silicon_rev = AD7124_SILICON_REV_GET(readval);
>
> -       if (chip_id != st->chip_info->chip_id) {
> -               dev_err(&st->sd.spi->dev,
> -                       "Chip ID mismatch: expected %u, got %u\n",
> -                       st->chip_info->chip_id, chip_id);
> -               return -ENODEV;
> -       }
> +       if (chip_id != st->chip_info->chip_id)
> +               return dev_err_probe(&st->sd.spi->dev, -ENODEV,
> +                                    "Chip ID mismatch: expected %u, got %u\n",
> +                                    st->chip_info->chip_id, chip_id);
>
> -       if (silicon_rev == 0) {
> -               dev_err(&st->sd.spi->dev,
> -                       "Silicon revision empty. Chip may not be present\n");
> -               return -ENODEV;
> -       }
> +       if (silicon_rev == 0)
> +               return dev_err_probe(&st->sd.spi->dev, -ENODEV,
> +                                    "Silicon revision empty. Chip may not be present\n");
>
>         return 0;
>  }

...

>         ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
>         if (ret < 0)
> -               return ret;
> +               return dev_err_probe(dev, ret, "Failed to setup CONTROL register\n");
>
>         return ret;

Side note 3: return 0;

...

>         ret = ad7124_soft_reset(st);
>         if (ret < 0)

> +               /* ad7124_soft_reset() already emitted an error message */

To me it looks like an almost useless comment.

>                 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;

Ditto.
Uwe Kleine-König Nov. 25, 2024, 11:18 a.m. UTC | #3 
On Fri, Nov 22, 2024 at 10:25:43PM +0200, Andy Shevchenko wrote:
> On Fri, Nov 22, 2024 at 1:34 PM 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.
> 
> ...
> 
> > +/* Only called during probe, so dev_err_probe() can be used */
> 
> It's a harmless comment, but I think dev_err_probe() name is good
> enough to give such a hint.

Seems to be subjective. I guess I found already too many functions using
dev_err_probe() that are called also after .probe().

> >         ret = ad7124_soft_reset(st);
> >         if (ret < 0)
> 
> > +               /* ad7124_soft_reset() already emitted an error message */
> 
> To me it looks like an almost useless comment.

Same as above. If one of the first thoughts when you look at this code
is: "Huh, no error message in this exit code", it helps.

I ignored your side notes for now as they wouldn't affect this patch. I
made a note for later however.

Best regards
Uwe
Uwe Kleine-König Nov. 25, 2024, 11:20 a.m. UTC | #4 
On Fri, Nov 22, 2024 at 11:44:32AM -0500, Trevor Gamblin wrote:
> 
> On 2024-11-22 06:33, Uwe Kleine-König 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>
> Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

With the changes that Andy suggested I didn't add your tag yet. So if
you miss it in v4, *this time* it was a concious choice. :-)

Best regards and thanks,
Uwe
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index b17c3dbeaeba..fdbe2806bf11 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -360,20 +360,21 @@  static int ad7124_find_free_config_slot(struct ad7124_state *st)
 	return free_cfg_slot;
 }
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channel_config *cfg)
 {
 	unsigned int refsel = cfg->refsel;
+	struct device *dev = &st->sd.spi->dev;
 
 	switch (refsel) {
 	case AD7124_REFIN1:
 	case AD7124_REFIN2:
 	case AD7124_AVDD_REF:
-		if (IS_ERR(st->vref[refsel])) {
-			dev_err(&st->sd.spi->dev,
-				"Error, trying to use external voltage reference without a %s regulator.\n",
-				ad7124_ref_names[refsel]);
-			return PTR_ERR(st->vref[refsel]);
-		}
+		if (IS_ERR(st->vref[refsel]))
+			return dev_err_probe(dev, PTR_ERR(st->vref[refsel]),
+					     "Error, trying to use external voltage reference without a %s regulator.\n",
+					     ad7124_ref_names[refsel]);
+
 		cfg->vref_mv = regulator_get_voltage(st->vref[refsel]);
 		/* Conversion from uV to mV */
 		cfg->vref_mv /= 1000;
@@ -384,8 +385,7 @@  static int ad7124_init_config_vref(struct ad7124_state *st, struct ad7124_channe
 		st->adc_control |= AD7124_ADC_CTRL_REF_EN(1);
 		return 0;
 	default:
-		dev_err(&st->sd.spi->dev, "Invalid reference %d\n", refsel);
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Invalid reference %d\n", refsel);
 	}
 }
 
@@ -752,6 +752,7 @@  static const struct iio_info ad7124_info = {
 	.attrs = &ad7124_attrs_group,
 };
 
+/* Only called during probe, so dev_err_probe() can be used */
 static int ad7124_soft_reset(struct ad7124_state *st)
 {
 	unsigned int readval, timeout;
@@ -766,7 +767,7 @@  static int ad7124_soft_reset(struct ad7124_state *st)
 	do {
 		ret = ad_sd_read_reg(&st->sd, AD7124_STATUS, 1, &readval);
 		if (ret < 0)
-			return ret;
+			return dev_err_probe(&st->sd.spi->dev, ret, "Error reading status register\n");
 
 		if (!(readval & AD7124_STATUS_POR_FLAG_MSK))
 			return 0;
@@ -775,9 +776,7 @@  static int ad7124_soft_reset(struct ad7124_state *st)
 		usleep_range(100, 2000);
 	} while (--timeout);
 
-	dev_err(&st->sd.spi->dev, "Soft reset failed\n");
-
-	return -EIO;
+	return dev_err_probe(&st->sd.spi->dev, -EIO, "Soft reset failed\n");
 }
 
 static int ad7124_check_chip_id(struct ad7124_state *st)
@@ -787,23 +786,20 @@  static int ad7124_check_chip_id(struct ad7124_state *st)
 
 	ret = ad_sd_read_reg(&st->sd, AD7124_ID, 1, &readval);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(&st->sd.spi->dev, ret,
+				     "Failure to read ID register\n");
 
 	chip_id = AD7124_DEVICE_ID_GET(readval);
 	silicon_rev = AD7124_SILICON_REV_GET(readval);
 
-	if (chip_id != st->chip_info->chip_id) {
-		dev_err(&st->sd.spi->dev,
-			"Chip ID mismatch: expected %u, got %u\n",
-			st->chip_info->chip_id, chip_id);
-		return -ENODEV;
-	}
+	if (chip_id != st->chip_info->chip_id)
+		return dev_err_probe(&st->sd.spi->dev, -ENODEV,
+				     "Chip ID mismatch: expected %u, got %u\n",
+				     st->chip_info->chip_id, chip_id);
 
-	if (silicon_rev == 0) {
-		dev_err(&st->sd.spi->dev,
-			"Silicon revision empty. Chip may not be present\n");
-		return -ENODEV;
-	}
+	if (silicon_rev == 0)
+		return dev_err_probe(&st->sd.spi->dev, -ENODEV,
+				     "Silicon revision empty. Chip may not be present\n");
 
 	return 0;
 }
@@ -862,16 +858,18 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 	device_for_each_child_node_scoped(dev, child) {
 		ret = fwnode_property_read_u32(child, "reg", &channel);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse reg property of %pfwP\n", child);
 
 		if (channel >= indio_dev->num_channels)
 			return dev_err_probe(dev, -EINVAL,
-				"Channel index >= number of channels\n");
+					     "Channel index >= number of channels in %pfwP\n", child);
 
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, 2);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret,
+					     "Failed to parse diff-channels property of %pfwP\n", child);
 
 		if (!ad7124_valid_input_select(ain[0], st->chip_info) ||
 		    !ad7124_valid_input_select(ain[1], st->chip_info))
@@ -909,11 +907,12 @@  static int ad7124_parse_channel_config(struct iio_dev *indio_dev,
 static int ad7124_setup(struct ad7124_state *st)
 {
 	unsigned int fclk, power_mode;
+	struct device *dev = &st->sd.spi->dev;
 	int i, ret;
 
 	fclk = clk_get_rate(st->mclk);
 	if (!fclk)
-		return -EINVAL;
+		return dev_err_probe(dev, -EINVAL, "Failed to get mclk rate\n");
 
 	/* The power mode changes the master clock frequency */
 	power_mode = ad7124_find_closest_match(ad7124_master_clk_freq_hz,
@@ -922,7 +921,7 @@  static int ad7124_setup(struct ad7124_state *st)
 	if (fclk != ad7124_master_clk_freq_hz[power_mode]) {
 		ret = clk_set_rate(st->mclk, fclk);
 		if (ret)
-			return ret;
+			return dev_err_probe(dev, ret, "Failed to set mclk rate\n");
 	}
 
 	/* Set the power mode */
@@ -953,7 +952,7 @@  static int ad7124_setup(struct ad7124_state *st)
 
 	ret = ad_sd_write_reg(&st->sd, AD7124_ADC_CONTROL, 2, st->adc_control);
 	if (ret < 0)
-		return ret;
+		return dev_err_probe(dev, ret, "Failed to setup CONTROL register\n");
 
 	return ret;
 }
@@ -968,11 +967,12 @@  static int ad7124_probe(struct spi_device *spi)
 	const struct ad7124_chip_info *info;
 	struct ad7124_state *st;
 	struct iio_dev *indio_dev;
+	struct device *dev = &spi->dev;
 	int i, ret;
 
 	info = spi_get_device_match_data(spi);
 	if (!info)
-		return -ENODEV;
+		return dev_err_probe(dev, -ENODEV, "Failed to get match data\n");
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
 	if (!indio_dev)
@@ -1007,36 +1007,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 */
 		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[] = {