On Fri, Nov 22, 2024 at 1:34 PM Uwe Kleine-König
<u.kleine-koenig@xxxxxxxxxxxx> 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.
--
With Best Regards,
Andy Shevchenko
