Re: [PATCH v3] iio: dac: mcp4821: add configurable gain support

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 12 Apr 2026 13:35:45 -0500
David Lechner <dlechner@baylibre.com> wrote:

> On 4/12/26 10:23 AM, Nikhil Gautam wrote:
> > Hi David,
> > 
> > Thanks a lot for the detailed review and suggestions.
> > 
> > I understand the issues you pointed out. I will resend the next revision
> > as a new thread instead of replying, to avoid breaking tooling.
> > 
> > I will also split the unrelated changes into separate patches, starting
> > with moving the state initialization, and handle spelling fixes
> > independently.
> > 
> > Regarding the gain handling, your suggestion to use an integer value
> > instead of a boolean makes sense. I will update the implementation to
> > store the gain directly and simplify the logic accordingly.
> >   
> 
> To save time, you don't need to comment on things you agree with. It
> makes it hard to find any important parts where you might be asking
> for additional feedback.
> 
> And trim out irrelevant quoted bits when replying. It saves time too.
> 
> And please don't top post.
> 
> >>> +static const int mcp4821_gain_avail[] = {1, 2};  
> >> The attribute is the scale attribute, not gain, so these need to
> >> be: { MCP4821_VREF_MV, 2 * MCP4821_VREF_MV } combined with...
> >>  
> >>>   -    return 0;
> >>> +static int mcp4821_read_avail(struct iio_dev *indio_dev,
> >>> +                  struct iio_chan_spec const *chan,
> >>> +                  const int **vals, int *type, int *length,
> >>> +                  long info)
> >>> +{
> >>> +    switch (info) {
> >>> +    case IIO_CHAN_INFO_SCALE:
> >>> +        *vals = mcp4821_gain_avail;
> >>> +        *type = IIO_VAL_INT;  
> >> ... IIO_VAL_FRACTIONAL_LOG2.
> >>
> >> The idea is that the scale_available attribute should return the same
> >> values as the scale attribute.  
> 
> Instead, reply inline where the context makes sense like this:
> 
> > I am thinking to return (2048/4096) Values based on gain selected
> > instead (1/2) Values in scale_available, so that user will have clear picture which gain
> > to be selected, else for (1/2) Values he always has to refer the driver.
> >   
> 
> We have to follow the IIO ABI of what these attribute mean so that all drivers
> work the same. We can't make up new rules for this driver.
> 
> For a 12-bit chip, `cat iio\:device0/out_voltage_scale_available` for this
> driver should return `0.5 1` since the scale is either 2048 / (1 << 12)
> or 4096 / (1 << 12).
> 
> These are exactly the values that must be written to
> `iio\:device0/out_voltage_scale_available` to change the scale. And then
> `cat iio\:device0/out_voltage_scale_available` must read back exactly what
> was written.

out_voltage_scale was intended here I think as _available attributes
are read only.


> 
>