linux-kernel - Re: [bug report] iio: dac: adding support for Microchip MCP47FEB02

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <16614d1f875aa1e9923375dd3de0897de17791f5.camel@microchip.com>
Date: Tue, 10 Feb 2026 10:26:05 +0000
From: <Ariana.Lazar@...rochip.com>
To: <andy.shevchenko@...il.com>, <dan.carpenter@...aro.org>
CC: <andriy.shevchenko@...el.com>, <nuno.sa@...log.com>,
	<dlechner@...libre.com>, <linux-iio@...r.kernel.org>, <andy@...nel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [bug report] iio: dac: adding support for Microchip MCP47FEB02

On Fri, 2026-02-06 at 17:57 +0200, Andy Shevchenko wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On Fri, Feb 6, 2026 at 5:32 PM Dan Carpenter
> <dan.carpenter@...aro.org> wrote:
> > On Fri, Feb 06, 2026 at 05:14:53PM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 06, 2026 at 05:33:26PM +0300, Dan Carpenter wrote:
> > > > On Fri, Feb 06, 2026 at 04:04:07PM +0200, Andy Shevchenko
> > > > wrote:
> > > > > > drivers/iio/dac/mcp47feb02.c
> > > > > >     712 static int mcp47feb02_init_scales_avail(struct
> > > > > > mcp47feb02_data *data, int vdd_mV,
> > > > > >     713                                         int
> > > > > > vref_mV, int vref1_mV)
> > > > > >     714 {
> > > > > >     715         struct device *dev =
> > > > > > regmap_get_device(data->regmap);
> > > > > >     716         int tmp_vref;
> > > > > >     717
> > > > > >     718         mcp47feb02_init_scale(data,
> > > > > > MCP47FEB02_SCALE_VDD, vdd_mV, data->scale);
> > > > > >     719
> > > > > >     720         if (data->use_vref)
> > > > > >     721                 tmp_vref = vref_mV;
> > > > > >     722         else
> > > > > >     723                 tmp_vref =
> > > > > > MCP47FEB02_INTERNAL_BAND_GAP_mV;
> > > > > >     724
> > > > > >     725         mcp47feb02_init_scale(data,
> > > > > > MCP47FEB02_SCALE_GAIN_X1, tmp_vref, data->scale);
> > > > > >     726         mcp47feb02_init_scale(data,
> > > > > > MCP47FEB02_SCALE_GAIN_X2, tmp_vref * 2, data->scale);
> > > > > >     727
> > > > > >     728         if (data->phys_channels >= 4) {
> > > > > >     729                 mcp47feb02_init_scale(data,
> > > > > > MCP47FEB02_SCALE_VDD, vdd_mV, data->scale_1);
> > > > > >     730
> > > > > >     731                 if (data->use_vref1 && vref1_mV <=
> > > > > > 0)
> > > > > > --> 732                         return dev_err_probe(dev,
> > > > > > vref1_mV, "Invalid voltage for Vref1\n");
> > > > > >                                                          
> > > > > > ^^^^^^^^
> > > > > > vref1_mV is not a valid error code.
> > > > > 
> > > > > Why not? When it's negative I believe the above statement is
> > > > > not true.
> > > > 
> > > > I saw this as just sanity checking the input.  vref1_mV is
> > > > never
> > > > actually negative.  I don't know if
> > > > devm_regulator_get_enable_read_voltage()
> > > > can return less than one millivolt.
> > > 
> > >  * In cases where the supply is not strictly required, callers
> > > can check for
> > >  * -ENODEV error and handle it accordingly.
> > >  *
> > >  * Returns: voltage in microvolts on success, or an negative
> > > error number on failure.
> > > 
> > > What did I miss?
> > > 
> > 
> > drivers/iio/dac/mcp47feb02.c
> >   1157          if (chip_features->have_ext_vref1) {
> >   1158                  ret =
> > devm_regulator_get_enable_read_voltage(dev, "vref1");
> >   1159                  if (ret > 0) {
> >   1160                          vref1_mV = ret / MILLI;
> > 
> > Potentially, if ret is in the 1-999 range then vref1_mV could be
> > zero,
> > but it can't be negative.
> 
> I see, thanks!
> 
> So, it means that the validation should be moved here on ret < 0 and
> ret < 1000 (if positive).
> 
> >   1161                          data->use_vref1 = true;
> >   1162                  } else {
> >   1163                          dev_dbg(dev, "using internal band
> > gap as voltage reference 1.\n");
> >   1164                          dev_dbg(dev, "Vref1 is
> > unavailable.\n");
> 
> But... ret < 0  is checked here.
> Hence the only one left is the range [0..999].
> 
> >   1165                  }
> >   1166          }
> >   1167
> >   1168          ret = mcp47feb02_init_ctrl_regs(data);
> >   1169          if (ret)
> >   1170                  return dev_err_probe(dev, ret, "Error
> > initialising vref register\n");
> >   1171
> >   1172          ret = mcp47feb02_init_ch_scales(data, vdd_mV,
> > vref_mV, vref1_mV);
> >                                                                    
> >     ^^^^^^^^
> > 
> >   1173          if (ret)
> >   1174                  return ret;
> 
> 
> --
> With Best Regards,
> Andy Shevchenko


Hello Dan and Andy,

Thank you for bringing to my attention this bug. I fixed it by storing
voltages
in microvolts instead of millivolts in order to avoid the [1, 999]
case.
I removed dividing by MILLI from the probe function and kept the
computation of
the scale values only in init_scale function.

I will send a follow on patch.

Best regards,
Ariana