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

Andy Shevchenko
2026-02-06T15:57:52Z

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