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

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.

> +		if (ret < 0)
> +			return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2