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

[<Ariana.Lazar@microchip.com>]

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().

Best regards,
Ariana