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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 10 Feb 2026 10:26:05 +0000
<Ariana.Lazar@microchip.com> wrote:

> 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@linaro.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.

Hi Ariana,

Just a reminder that this one still seems to be outstanding.
Maybe I missed a patch?

Thanks,

Jonathan

> 
> Best regards,
> Ariana
>