On Wed, Sep 22, 2021 at 01:10:52AM -0400, Liam Beguin wrote:
> >
> > 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.
>
> I see.. So far, I've only used fixed-regulators to provide a constant
> voltage reference here, and I guess those are quite unlikely to fail.
>
> > You might be doubting that devm_regulator_get_optional() can return
> > -ENODEV on error? Look at the code and prepare your heart for sadness.
>
> Thanks for the warning, I see what you meant now.
>
> I wasn't able to use smatch to reproduce the error with the following:
>
> make O=builds/smatch CHECK="~/dev/smatch/smatch -p=kernel" C=1 Image.gz
>
> Would you have any pointer for that?
This requires building the cross function Database:
~/dev/smatch/smatch_scripts/build_kernel_data.sh
The command takes 5 hours to run so here is a short cut which just
builds the minimum two files:
~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/core.c | tee out
~/dev/smatch/smatch_data/db/create_db.sh -p=kernel out
~/dev/smatch/smatch_scripts/kchecker --info drivers/regulator/devres.c | tee out
~/dev/smatch/smatch_data/db/reload_partial.sh out
~/dev/smatch/smatch_scripts/kchecker --spammy drivers/iio/adc/ad7949.c
>
> Anyway, I believe the following would address the error you mentioned.
>
> -- >8 --
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> index 44bb5fde83de..3613f4e55e1c 100644
> --- a/drivers/iio/adc/ad7949.c
> +++ b/drivers/iio/adc/ad7949.c
> @@ -368,12 +368,14 @@ static int ad7949_spi_probe(struct spi_device *spi)
> ad7949_adc->vref = devm_regulator_get_optional(dev, "vrefin");
> if (IS_ERR(ad7949_adc->vref)) {
> ret = PTR_ERR(ad7949_adc->vref);
> + ad7949_adc->vref = NULL;
This is not required because it will just be reassigned in a couple
lines.
> if (ret != -ENODEV)
> return ret;
> /* unbuffered? */
> ad7949_adc->vref = devm_regulator_get_optional(dev, "vref");
> if (IS_ERR(ad7949_adc->vref)) {
> ret = PTR_ERR(ad7949_adc->vref);
> + ad7949_adc->vref = NULL;
But this also won't work. Passing a NULL to regulator_enable() will
cause an Oops. All the reference to ->vref need checks. :/
> if (ret != -ENODEV)
> return ret;
> } else {
regards,
dan carpenter
