[PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Ariana Lazar]

Initialize vref1_uV variable to 0 before calling
mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
uninitialized value when have_ext_vref1 is false.

Fixes: dd154646d292 ("iio: dac: mcp47feb02: Fix Vref validation [1-999] case")
Reported-by: Dan Carpenter <error27@gmail.com>
Closes: https://lore.kernel.org/all/adiPnla0M5EzvgD-@stanley.mountain/
Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
---
 drivers/iio/dac/mcp47feb02.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
index faccb804a5ed548088aaf83266b16ed45a92916c..efd4847913e46a4fa1b671aa9c5bd3e1430406b7 100644
--- a/drivers/iio/dac/mcp47feb02.c
+++ b/drivers/iio/dac/mcp47feb02.c
@@ -1095,9 +1095,10 @@ static int mcp47feb02_probe(struct i2c_client *client)
 {
 	const struct mcp47feb02_features *chip_features;
 	struct device *dev = &client->dev;
+	int vdd_uV, vref_uV, vref1_uV;
 	struct mcp47feb02_data *data;
 	struct iio_dev *indio_dev;
-	int vref1_uV, vref_uV, vdd_uV, ret;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
 	if (!indio_dev)
@@ -1146,13 +1147,13 @@ static int mcp47feb02_probe(struct i2c_client *client)
 		dev_dbg(dev, "Vref is unavailable.\n");
 	}
 
+	vref1_uV = 0;
 	if (chip_features->have_ext_vref1) {
 		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
 		if (ret > 0) {
 			vref1_uV = ret;
 			data->use_vref1 = true;
 		} else {
-			vref1_uV = 0;
 			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
 			dev_dbg(dev, "Vref1 is unavailable.\n");
 		}

---
base-commit: 51e7665ab81f02adc80a1219c260ee678e9c6eb8
change-id: 20260414-mcp47feb02-fix4-614de9334f22

Best regards,
-- 
Ariana Lazar <ariana.lazar@microchip.com>

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:
> Initialize vref1_uV variable to 0 before calling
> mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
> uninitialized value when have_ext_vref1 is false.

...

> +	vref1_uV = 0;
>  	if (chip_features->have_ext_vref1) {

I'm wondering what will happen if we do the below unconditionally?

>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");

If we have no regulator, we get a dummy one, right? What is the voltage will
be? 0?

>  		if (ret > 0) {
>  			vref1_uV = ret;
>  			data->use_vref1 = true;
>  		} else {
> -			vref1_uV = 0;
>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
>  			dev_dbg(dev, "Vref1 is unavailable.\n");
>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[David Lechner]

On 4/14/26 7:48 AM, Andy Shevchenko wrote:
> On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:
>> Initialize vref1_uV variable to 0 before calling
>> mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
>> uninitialized value when have_ext_vref1 is false.
> 
> ...
> 
>> +	vref1_uV = 0;
>>  	if (chip_features->have_ext_vref1) {
> 
> I'm wondering what will happen if we do the below unconditionally?
> 
>>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> 
> If we have no regulator, we get a dummy one, right? What is the voltage will
> be? 0?

It will fail with -EINVAL because dummy regulator doesn't have a
voltage specified anywhere.

> 
>>  		if (ret > 0) {
>>  			vref1_uV = ret;
>>  			data->use_vref1 = true;
>>  		} else {
>> -			vref1_uV = 0;
>>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
>>  			dev_dbg(dev, "Vref1 is unavailable.\n");
>>  		}
> 

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Ariana.Lazar]

Hi Andy,

On Tue, 2026-04-14 at 15:48 +0300, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:
> > Initialize vref1_uV variable to 0 before calling
> > mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing
> > an
> > uninitialized value when have_ext_vref1 is false.
> 
> ...
> 
> > +     vref1_uV = 0;
> >       if (chip_features->have_ext_vref1) {
> 
> I'm wondering what will happen if we do the below unconditionally?
> 
> >               ret = devm_regulator_get_enable_read_voltage(dev,
> > "vref1");
> 
> If we have no regulator, we get a dummy one, right? What is the
> voltage will
> be? 0?
> 
> >               if (ret > 0) {
> >                       vref1_uV = ret;
> >                       data->use_vref1 = true;
> >               } else {
> > -                     vref1_uV = 0;
> >                       dev_dbg(dev, "using internal band gap as
> > voltage reference 1.\n");
> >                       dev_dbg(dev, "Vref1 is unavailable.\n");
> >               }

Thank you for the review.

This is a safety check to ensure the devicetree matches the available
hardware. If Vref1 was selected in devicetree but unavailable in
hardware, the scales MCP47FEB02_SCALE_GAIN_X1 and
MCP47FEB02_SCALE_GAIN_X2 and also voltage readings would be incorrect
for the channels that use Vref1.

I did something similiar to what you have suggested in the first patch
I have submitted for this driver and checking first was recommended.

https://lore.kernel.org/all/20250927185324.2f9e8061@jic23-huawei/

Best regards,
Ariana

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[David Lechner]

On 4/14/26 7:33 AM, Ariana Lazar wrote:
> Initialize vref1_uV variable to 0 before calling
> mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
> uninitialized value when have_ext_vref1 is false.
> 
> Fixes: dd154646d292 ("iio: dac: mcp47feb02: Fix Vref validation [1-999] case")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/all/adiPnla0M5EzvgD-@stanley.mountain/
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
>  drivers/iio/dac/mcp47feb02.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index faccb804a5ed548088aaf83266b16ed45a92916c..efd4847913e46a4fa1b671aa9c5bd3e1430406b7 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -1095,9 +1095,10 @@ static int mcp47feb02_probe(struct i2c_client *client)
>  {
>  	const struct mcp47feb02_features *chip_features;
>  	struct device *dev = &client->dev;
> +	int vdd_uV, vref_uV, vref1_uV;
>  	struct mcp47feb02_data *data;
>  	struct iio_dev *indio_dev;
> -	int vref1_uV, vref_uV, vdd_uV, ret;
> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -1146,13 +1147,13 @@ static int mcp47feb02_probe(struct i2c_client *client)
>  		dev_dbg(dev, "Vref is unavailable.\n");
>  	}
>  
> +	vref1_uV = 0;
>  	if (chip_features->have_ext_vref1) {
>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");

I don't think ignoring all errors here is a good idea. For example, what
if it returned -EPROBE_DEFER? Then we could end up with the wrong reference
voltage instead of trying again later.

So probably something like this would be better (also fixes uninitialized
variable):

	if (chip_features->have_ext_vref1 &&
	    device_property_present(dev, "vref1")) {
		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
		if (ret < 0)
			return ret;

		vref1_uV = ret;
		data->use_vref1 = true;
	} else {
		vref1_uV = 0;
		dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
	}


>  		if (ret > 0) {
>  			vref1_uV = ret;
>  			data->use_vref1 = true;
>  		} else {
> -			vref1_uV = 0;
>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
>  			dev_dbg(dev, "Vref1 is unavailable.\n");
>  		}
> 
> ---
> base-commit: 51e7665ab81f02adc80a1219c260ee678e9c6eb8
> change-id: 20260414-mcp47feb02-fix4-614de9334f22
> 
> Best regards,

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 08:21:00AM -0500, David Lechner wrote:
> On 4/14/26 7:48 AM, Andy Shevchenko wrote:
> > On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:
> >> Initialize vref1_uV variable to 0 before calling
> >> mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
> >> uninitialized value when have_ext_vref1 is false.

...

> >> +	vref1_uV = 0;
> >>  	if (chip_features->have_ext_vref1) {
> > 
> > I'm wondering what will happen if we do the below unconditionally?
> > 
> >>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> > 
> > If we have no regulator, we get a dummy one, right? What is the voltage will
> > be? 0?
> 
> It will fail with -EINVAL because dummy regulator doesn't have a
> voltage specified anywhere.

Okay, and we have exactly a branch to assign 0 to the value with even useful
debug message which will answer to two questions at once: Do we have it? Yes,
since we see the debug message. Have we got it? No, as it's written in the
debug message.

That said, I think the better fix is just to drop the outer conditional.

> >>  		if (ret > 0) {
> >>  			vref1_uV = ret;
> >>  			data->use_vref1 = true;
> >>  		} else {
> >> -			vref1_uV = 0;
> >>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> >>  			dev_dbg(dev, "Vref1 is unavailable.\n");
> >>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 07:18:50PM +0300, Andy Shevchenko wrote:
> On Tue, Apr 14, 2026 at 08:21:00AM -0500, David Lechner wrote:
> > On 4/14/26 7:48 AM, Andy Shevchenko wrote:
> > > On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:

...

> > >> +	vref1_uV = 0;
> > >>  	if (chip_features->have_ext_vref1) {
> > > 
> > > I'm wondering what will happen if we do the below unconditionally?
> > > 
> > >>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> > > 
> > > If we have no regulator, we get a dummy one, right? What is the voltage will
> > > be? 0?
> > 
> > It will fail with -EINVAL because dummy regulator doesn't have a
> > voltage specified anywhere.
> 
> Okay, and we have exactly a branch to assign 0 to the value with even useful
> debug message which will answer to two questions at once: Do we have it? Yes,

I meant "No" here as well.

> since we see the debug message. Have we got it? No, as it's written in the
> debug message.
> 
> That said, I think the better fix is just to drop the outer conditional.
> 
> > >>  		if (ret > 0) {
> > >>  			vref1_uV = ret;
> > >>  			data->use_vref1 = true;
> > >>  		} else {
> > >> -			vref1_uV = 0;
> > >>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> > >>  			dev_dbg(dev, "Vref1 is unavailable.\n");
> > >>  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 02:26:45PM +0000, Ariana.Lazar@microchip.com wrote:
> On Tue, 2026-04-14 at 15:48 +0300, Andy Shevchenko wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > know the content is safe

You should get rid of this message. It's incompatible with OSS development
process.

> > On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:

...

> > > +     vref1_uV = 0;
> > >       if (chip_features->have_ext_vref1) {
> > 
> > I'm wondering what will happen if we do the below unconditionally?
> > 
> > >               ret = devm_regulator_get_enable_read_voltage(dev,
> > > "vref1");
> > 
> > If we have no regulator, we get a dummy one, right? What is the
> > voltage will
> > be? 0?
> > 
> > >               if (ret > 0) {
> > >                       vref1_uV = ret;
> > >                       data->use_vref1 = true;
> > >               } else {
> > > -                     vref1_uV = 0;
> > >                       dev_dbg(dev, "using internal band gap as
> > > voltage reference 1.\n");
> > >                       dev_dbg(dev, "Vref1 is unavailable.\n");
> > >               }
> 
> Thank you for the review.
> 
> This is a safety check to ensure the devicetree matches the available
> hardware. If Vref1 was selected in devicetree but unavailable in
> hardware, the scales MCP47FEB02_SCALE_GAIN_X1 and
> MCP47FEB02_SCALE_GAIN_X2 and also voltage readings would be incorrect
> for the channels that use Vref1.

I didn't get how. What I recommend is to do regulator request unconditionally.

> I did something similiar to what you have suggested in the first patch
> I have submitted for this driver and checking first was recommended.
> 
> https://lore.kernel.org/all/20250927185324.2f9e8061@jic23-huawei/

I briefly read that. The check was there, Jonathan just asked to modify
the check itself IIUC, i.o.w. the semantics of the check was commented
and not the check presence in the first place.

Did I get it wrong? Jonathan, can we get rid of the check and ask for
regulator unconditionally? (Maybe it would be good to print an error
code in the debug message to be sure why it failed to get the regulator
or its voltage.)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Andy Shevchenko]

On Tue, Apr 14, 2026 at 10:32:11AM -0500, David Lechner wrote:
> On 4/14/26 7:33 AM, Ariana Lazar wrote:

...

> > +	vref1_uV = 0;
> >  	if (chip_features->have_ext_vref1) {
> >  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> 
> I don't think ignoring all errors here is a good idea. For example, what
> if it returned -EPROBE_DEFER? Then we could end up with the wrong reference
> voltage instead of trying again later.
> 
> So probably something like this would be better (also fixes uninitialized
> variable):
> 
> 	if (chip_features->have_ext_vref1 &&
> 	    device_property_present(dev, "vref1")) {
> 		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
> 		if (ret < 0)
> 			return ret;
> 
> 		vref1_uV = ret;
> 		data->use_vref1 = true;
> 	} else {
> 		vref1_uV = 0;
> 		dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> 	}

Thanks for the suggestion, I agree also with this alternative.

> >  		if (ret > 0) {
> >  			vref1_uV = ret;
> >  			data->use_vref1 = true;
> >  		} else {
> > -			vref1_uV = 0;
> >  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
> >  			dev_dbg(dev, "Vref1 is unavailable.\n");
> >  		}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Jonathan Cameron]

On Tue, 14 Apr 2026 15:33:38 +0300
Ariana Lazar <ariana.lazar@microchip.com> wrote:

> Initialize vref1_uV variable to 0 before calling
> mcp47feb02_init_ch_scales() in mcp47feb02_probe() to avoid passing an
> uninitialized value when have_ext_vref1 is false.
> 
> Fixes: dd154646d292 ("iio: dac: mcp47feb02: Fix Vref validation [1-999] case")
> Reported-by: Dan Carpenter <error27@gmail.com>
> Closes: https://lore.kernel.org/all/adiPnla0M5EzvgD-@stanley.mountain/
> Signed-off-by: Ariana Lazar <ariana.lazar@microchip.com>
> ---
>  drivers/iio/dac/mcp47feb02.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp47feb02.c b/drivers/iio/dac/mcp47feb02.c
> index faccb804a5ed548088aaf83266b16ed45a92916c..efd4847913e46a4fa1b671aa9c5bd3e1430406b7 100644
> --- a/drivers/iio/dac/mcp47feb02.c
> +++ b/drivers/iio/dac/mcp47feb02.c
> @@ -1095,9 +1095,10 @@ static int mcp47feb02_probe(struct i2c_client *client)
>  {
>  	const struct mcp47feb02_features *chip_features;
>  	struct device *dev = &client->dev;
> +	int vdd_uV, vref_uV, vref1_uV;
>  	struct mcp47feb02_data *data;
>  	struct iio_dev *indio_dev;
> -	int vref1_uV, vref_uV, vdd_uV, ret;
I haven't caught up with the rest of the discussion yet, but this change
seems to be superficial style stuff. Doesn't belong in a fix patch.

Thanks,

Jonathan

> +	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -1146,13 +1147,13 @@ static int mcp47feb02_probe(struct i2c_client *client)
>  		dev_dbg(dev, "Vref is unavailable.\n");
>  	}
>  
> +	vref1_uV = 0;
>  	if (chip_features->have_ext_vref1) {
>  		ret = devm_regulator_get_enable_read_voltage(dev, "vref1");
>  		if (ret > 0) {
>  			vref1_uV = ret;
>  			data->use_vref1 = true;
>  		} else {
> -			vref1_uV = 0;
>  			dev_dbg(dev, "using internal band gap as voltage reference 1.\n");
>  			dev_dbg(dev, "Vref1 is unavailable.\n");
>  		}
> 
> ---
> base-commit: 51e7665ab81f02adc80a1219c260ee678e9c6eb8
> change-id: 20260414-mcp47feb02-fix4-614de9334f22
> 
> Best regards,

Re: [PATCH] iio: dac: Fix passing uninitialized vref1_uV for no Vref1 case

[Jonathan Cameron]

On Tue, 14 Apr 2026 19:26:24 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Tue, Apr 14, 2026 at 02:26:45PM +0000, Ariana.Lazar@microchip.com wrote:
> > On Tue, 2026-04-14 at 15:48 +0300, Andy Shevchenko wrote:  
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you
> > > know the content is safe  
> 
> You should get rid of this message. It's incompatible with OSS development
> process.
> 
> > > On Tue, Apr 14, 2026 at 03:33:38PM +0300, Ariana Lazar wrote:  
> 
> ...
> 
> > > > +     vref1_uV = 0;
> > > >       if (chip_features->have_ext_vref1) {  
> > > 
> > > I'm wondering what will happen if we do the below unconditionally?
> > >   
> > > >               ret = devm_regulator_get_enable_read_voltage(dev,
> > > > "vref1");  
> > > 
> > > If we have no regulator, we get a dummy one, right? What is the
> > > voltage will
> > > be? 0?
> > >   
> > > >               if (ret > 0) {
> > > >                       vref1_uV = ret;
> > > >                       data->use_vref1 = true;
> > > >               } else {
> > > > -                     vref1_uV = 0;
> > > >                       dev_dbg(dev, "using internal band gap as
> > > > voltage reference 1.\n");
> > > >                       dev_dbg(dev, "Vref1 is unavailable.\n");
> > > >               }  
> > 
> > Thank you for the review.
> > 
> > This is a safety check to ensure the devicetree matches the available
> > hardware. If Vref1 was selected in devicetree but unavailable in
> > hardware, the scales MCP47FEB02_SCALE_GAIN_X1 and
> > MCP47FEB02_SCALE_GAIN_X2 and also voltage readings would be incorrect
> > for the channels that use Vref1.  
> 
> I didn't get how. What I recommend is to do regulator request unconditionally.
> 
> > I did something similiar to what you have suggested in the first patch
> > I have submitted for this driver and checking first was recommended.
> > 
> > https://lore.kernel.org/all/20250927185324.2f9e8061@jic23-huawei/  
> 
> I briefly read that. The check was there, Jonathan just asked to modify
> the check itself IIUC, i.o.w. the semantics of the check was commented
> and not the check presence in the first place.
> 
> Did I get it wrong? Jonathan, can we get rid of the check and ask for
> regulator unconditionally? (Maybe it would be good to print an error
> code in the debug message to be sure why it failed to get the regulator
> or its voltage.)
> 
Whilst it might be harmless I'm not keen to get a regulator at all if it
doesn't exist.  Sure we'll see an error and the code will work (assuming
the non existent regulator isn't in the DT!) but we'll also get a debug
message that is at best misleading.  Vref1 would indeed be unavailable
because the specific device doesn't have a pin to connect it to!

So I think the outer guard still makes sense. 

There is a reasonable question of why anything is using the value that
isn't initialized, and hence why the original bug exists?
I'm thinking it probably doesn't in practice, but that the
relationship between the flag and channel count etc is sufficiently complex
the compiler can't tell that.  So my assumption is this 'bug' is
a false positive.  We still want to tidy it up anyway on basis
it's not unreasonable that we are triggering static analysis warnings
on it!

J