Re: [bug report] iio: dac: adding support for Microchip MCP47FEB02
From: Jonathan Cameron
Date: Sun Mar 01 2026 - 07:32:10 EST
On Tue, 10 Feb 2026 10:26:05 +0000
<Ariana.Lazar@xxxxxxxxxxxxx> 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@xxxxxxxxxx> 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
>