[bug report] iio: adc: ad7949: add vref selection support

[Dan Carpenter <dan.carpenter@oracle.com>]

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