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

On Mon, 03 Aug 2026 17:44:40 +0000
[email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] Incorrect SMBus commands send extra bytes and corrupt the DAC hardware state.
> --
> 
> --- Patch [2]: [PATCH v3 2/2] iio: dac: mcp47a1: add support for new device ---
> commit f73263d5ea24798a693c0c4cfa738627794988ba
> Author: Joshua Crofts <[email protected]>
> 
> iio: dac: mcp47a1: add support for new device
> 
> This commit introduces a new driver for the Microchip MCP47A1 6-bit volatile
> DAC. It supports writing and reading the raw values and exposing the scale
> using I2C.
> 
> > diff --git a/drivers/iio/dac/mcp47a1.c b/drivers/iio/dac/mcp47a1.c
> > new file mode 100644
> > index 0000000000000..0bf994aa0e4f5
> > --- /dev/null
> > +++ b/drivers/iio/dac/mcp47a1.c  
> 
> [ ... ]
> 
> > +static int mcp47a1_write(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);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (!in_range(val, 0, MCP47A1_MAX_STEPS))
> > +			return -EINVAL;
> > +
> > +		return i2c_smbus_write_byte_data(data->client, MCP47A1_CMD_CODE,
> > +						 val);  
> 
> [Severity: Medium]
> Could this corrupt the DAC hardware state by sending an extra byte?
> 
> If the MCP47A1 expects a single byte without a command code, using
> i2c_smbus_write_byte_data() will send a 2-byte payload, writing 0x00 to the
> DAC before the actual value. This could potentially cause an output glitch.
> 
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +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_RAW:
> > +		ret = i2c_smbus_read_byte_data(data->client, MCP47A1_CMD_CODE);  
> 
> [Severity: Medium]
> Does this destructively reset the DAC on every read?
> 
> If the device does not expect a command code and interprets it as a data byte,
> calling i2c_smbus_read_byte_data() will perform a write of 0x00 before reading,
> destructively resetting the DAC output to 0.

No, this is a false positive. The MCP47A1 is SMBus compatible, it will interpret
the command as an instruction, not a data byte.

-- 
Kind regards,
Joshua Crofts