Hello Liam Beguin,
The patch 379306506049: "iio: adc: ad7949: add vref selection
support" from Aug 15, 2021, leads to the following
Smatch static checker warning:
drivers/iio/adc/ad7949.c:387 ad7949_spi_probe()
error: 'ad7949_adc->vref' dereferencing possible ERR_PTR()
drivers/iio/adc/ad7949.c
309 static int ad7949_spi_probe(struct spi_device *spi)
310 {
311 u32 spi_ctrl_mask = spi->controller->bits_per_word_mask;
312 struct device *dev = &spi->dev;
313 const struct ad7949_adc_spec *spec;
314 struct ad7949_adc_chip *ad7949_adc;
315 struct iio_dev *indio_dev;
316 u32 tmp;
317 int ret;
318
319 indio_dev = devm_iio_device_alloc(dev, sizeof(*ad7949_adc));
320 if (!indio_dev) {
321 dev_err(dev, "can not allocate iio device\n");
322 return -ENOMEM;
323 }
324
325 indio_dev->info = &ad7949_spi_info;
326 indio_dev->name = spi_get_device_id(spi)->name;
327 indio_dev->modes = INDIO_DIRECT_MODE;
328 indio_dev->channels = ad7949_adc_channels;
329 spi_set_drvdata(spi, indio_dev);
330
331 ad7949_adc = iio_priv(indio_dev);
332 ad7949_adc->indio_dev = indio_dev;
333 ad7949_adc->spi = spi;
334
335 spec = &ad7949_adc_spec[spi_get_device_id(spi)->driver_data];
336 indio_dev->num_channels = spec->num_channels;
337 ad7949_adc->resolution = spec->resolution;
338
339 /* Set SPI bits per word */
340 if (spi_ctrl_mask & SPI_BPW_MASK(ad7949_adc->resolution)) {
341 spi->bits_per_word = ad7949_adc->resolution;
342 } else if (spi_ctrl_mask == SPI_BPW_MASK(16)) {
343 spi->bits_per_word = 16;
344 } else if (spi_ctrl_mask == SPI_BPW_MASK(8)) {
345 spi->bits_per_word = 8;
346 } else {
347 dev_err(dev, "unable to find common BPW with spi controller\n");
348 return -EINVAL;
349 }
350
351 /* Setup internal voltage reference */
352 tmp = 4096000;
353 device_property_read_u32(dev, "adi,internal-ref-microvolt", &tmp);
354
355 switch (tmp) {
356 case 2500000:
357 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_2500;
358 break;
359 case 4096000:
360 ad7949_adc->refsel = AD7949_CFG_VAL_REF_INT_4096;
361 break;
362 default:
363 dev_err(dev, "unsupported internal voltage reference\n");
364 return -EINVAL;
365 }
366
367 /* Setup external voltage reference, buffered? */
368 ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
Apparenty it's really rare to have an optional regulator? This function
is very tricky. It should return NULL if the option is not like all the
other optional features in the kernel. But the regulator code is really
not set up for not having a regulator. If it were then there would be a
lot of NULL checks in the regulator code. But since it's not then you
have to add them to the driver instead.
369 if (IS_ERR(ad7949_adc->vref)) {
370 ret = PTR_ERR(ad7949_adc->vref);
371 if (ret != -ENODEV)
372 return ret;
373 /* unbuffered? */
374 ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
375 if (IS_ERR(ad7949_adc->vref)) {
376 ret = PTR_ERR(ad7949_adc->vref);
377 if (ret != -ENODEV)
378 return ret;
Instead of returning NULL when the regulator is disabled it returns
-ENODEV. How do you differentiate from an -ENODEV which is caused by an
error vs an -ENODEV which is caused because the optional regulator is
disabled? You'll just have to hope that the errors are less common and
assume it means disabled.
You might be doubting that devm_regulator_get_optional() can return
-ENODEV on error? Look at the code and prepare your heart for sadness.
379 } else {
380 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP;
381 }
382 } else {
383 ad7949_adc->refsel = AD7949_CFG_VAL_REF_EXT_TEMP_BUF;
384 }
385
386 if (ad7949_adc->refsel & AD7949_CFG_VAL_REF_EXTERNAL) {
--> 387 ret = regulator_enable(ad7949_adc->vref);
^^^^^^^^^^^^^^^^
Every reference to ->vref will crash if the optional regulator is
disabled.
388 if (ret < 0) {
389 dev_err(dev, "fail to enable regulator\n");
390 return ret;
391 }
392
393 ret = devm_add_action_or_reset(dev, ad7949_disable_reg,
394 ad7949_adc->vref);
395 if (ret)
396 return ret;
397 }
398
399 mutex_init(&ad7949_adc->lock);
400
401 ret = ad7949_spi_init(ad7949_adc);
402 if (ret) {
403 dev_err(dev, "enable to init this device: %d\n", ret);
404 return ret;
405 }
406
407 ret = devm_iio_device_register(dev, indio_dev);
408 if (ret)
409 dev_err(dev, "fail to register iio device: %d\n", ret);
410
411 return ret;
412 }
regards,
dan carpenter
