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

On Sat, 1 Aug 2026 03:30:13 +0100
Jonathan Cameron <[email protected]> wrote:

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

Good to know, for some reason I thought that this type of stuff was
discouraged (MACRO_NAME + x would seem messy in most cases).

-- 
Kind regards,
Joshua Crofts