Re: [PATCH v2 2/2] iio: dac: mcp47a1: add support for new device

On Tue, 28 Jul 2026 15:15:59 +0200
Joshua Crofts <[email protected]> wrote:

> On Mon, 27 Jul 2026 18:42:26 +0000
> [email protected] wrote:
> 
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Potential off-by-one in DAC scale calculation denominator.
> > --  
> 
> Responding to Sashiko's comment.
> 
> > --- Patch [2]: [PATCH v2 2/2] iio: dac: mcp47a1: add support for new device ---
> > commit 38bec2fbb020c3f749c52a26e5cc3e78e72eeaf5
> > Author: Joshua Crofts <[email protected]>
> > 
> > iio: dac: mcp47a1: add support for new device
> > 
> > The Microchip MCP47A1 is a 6-bit volatile Digital-to-Analog converter
> > which communicates via I2C.
> >   
> > > diff --git a/drivers/iio/dac/mcp47a1.c b/drivers/iio/dac/mcp47a1.c
> > > new file mode 100644
> > > index 0000000000000..140e93ff2ba01
> > > --- /dev/null
> > > +++ b/drivers/iio/dac/mcp47a1.c    
> > [ ... ]  
> > > +static int mcp47a1_read(struct iio_dev *indio_dev,
> > > +			struct iio_chan_spec const *chan,
> > > +			int *val, int *val2, long mask)
> > > +{
> > > +	struct mcp47a1_data *data = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	switch (mask) {    
> > [ ... ]  
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		*val = data->vref_mV;
> > > +		*val2 = MCP47A1_MAX_STEP;    
> > 
> > [Severity: Medium]
> > Does this code use the correct denominator for the scale calculation?
> > 
> > Since this is a 6-bit DAC, shouldn't the scale denominator be 64 (2^6)
> > instead of the maximum step value of 63? Using MCP47A1_MAX_STEP (63) here
> > introduces an off-by-one error that could cause userspace to calculate
> > output voltages with a ~1.5% LSB scaling error when reading the sysfs
> > attribute.  
> 
> Yes, the max amount of steps is 64, which should be passed here. I'll
> add a separate macro for this in v3.
I'd just use + 1 at this location given the clear relationship between
the maximum value that can be taken and the number of steps.

>