Re: [PATCH 2/2] iio: dac: add support for Microchip MCP48FEB02

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 24 Apr 2026 08:01:17 +0000
<Ariana.Lazar@microchip.com> wrote:

> Hi Jonathan,
> 
> > > +
> > > +static int mcp48feb02_init_ctrl_regs(struct mcp48feb02_data *data)
> > > +{
> > > +     unsigned int i, vref_ch, gain_ch, pd_ch;
> > > +     int ret;
> > > +
> > > +     ret = regmap_read(data->regmap, MCP48FEB02_VREF_REG_ADDR,
> > > &vref_ch);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_read(data->regmap,
> > > MCP48FEB02_GAIN_CTRL_STATUS_REG_ADDR, &gain_ch);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_read(data->regmap,
> > > MCP48FEB02_POWER_DOWN_REG_ADDR, &pd_ch);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     gain_ch = gain_ch & MCP48FEB02_GAIN_BITS_MASK;
> > > +     for_each_set_bit(i, &data->active_channels_mask, data-  
> > > >phys_channels) {  
> > > +             struct device *dev = regmap_get_device(data->regmap);
> > > +             unsigned int pd_tmp;
> > > +
> > > +             data->chdata[i].ref_mode = (vref_ch >> (2 * i)) &
> > > MCP48FEB02_DAC_CTRL_MASK;
> > > +             data->chdata[i].use_2x_gain = (gain_ch >> i)  &
> > > MCP48FEB02_GAIN_BIT_MASK;
> > > +
> > > +             /*
> > > +              * Inform the user that the current voltage reference
> > > read from the volatile
> > > +              * register of the chip is different from the one
> > > specified in the device tree.
> > > +              * Considering that the user cannot have an external
> > > voltage reference connected
> > > +              * to the pin and select the internal Band Gap at the
> > > same time, in order to avoid
> > > +              * miscofiguring the reference voltage, the volatile
> > > register will not be written.  
> > 
> > Spell check comments.  misconfiguring
> >   
> > > +              * In order to overwrite the setting from volatile
> > > register with the one from the
> > > +              * device tree, the user needs to write the chosen
> > > scale.  
> > 
> > I'm a little unsure of why we need this extra gate on updating things
> > to match
> > the device tree provided config.  Why should the volatile register at
> > this point
> > match what DT says?  If it does seems to me we should be noisier
> > about it than dev_dbg()
> > 
> >   
> > > +              */
> > > +             switch (data->chdata[i].ref_mode) {
> > > +             case MCP48FEB02_INTERNAL_BAND_GAP:
> > > +                     if (data->phys_channels >= 4 && (i % 2) &&
> > > data->use_vref1) {
> > > +                             dev_dbg(dev, "ch[%u]: was configured
> > > to use internal band gap", i);
> > > +                             dev_dbg(dev, "ch[%u]: reference
> > > voltage set to VREF1", i);
> > > +                             break;
> > > +                     }
> > > +                     if ((data->phys_channels < 4 || (data-  
> > > >phys_channels >= 4 && !(i % 2))) &&  
> > > +                         data->use_vref) {
> > > +                             dev_dbg(dev, "ch[%u]: was configured
> > > to use internal band gap", i);
> > > +                             dev_dbg(dev, "ch[%u]: reference
> > > voltage set to VREF", i);
> > > +                             break;
> > > +                     }  
> >   
> 
> 
> The device restores its EEPROM configuration (or writes default values
> for the part numbers without EEPROM) into the volatile registers at
> startup, so during probe we may have a valid Vref/gain state that does
> not match current DT. The driver should report any mismatch to the user
> because the Vref/gain selection is changed only through the scale
> attribute of each channel. The DT only describes available resources on
> the board for determining the available scales.
> 
> I can update the messages to describe the behavior better and use
> dev_info() instead of dev_dbg().

The statement above about it not being possible to use the internal band gap
if a vref is wired leaves me with more questions about this.

I'm a bit concerned about cases like:

Vref is wired and eeprom is set for internal reference.   Should we
be very careful to not enable vref until that situation is resolved?
It might be on anyway but we can at least make sure we are reponsible
for turning it on.

Maybe the chip has sufficient protective elements to cope with that
though obviously it won't give sensible output whilst this is true.

If all those are fine, dev_info() makes sense to me.
I wonder if we should return -EBUSY for attempts to read the voltage
back whilst in this state as well?  Might provide some additional
indication something is mismatched and we don't expect the device to work
correctly.

Jonathan


> Best regards,
> Ariana
> 
>