Re: [PATCH 1/2] iio: dac: dac8163: Add driver for DAC8163

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The SPI transfer buffer `tx` is allocated on the stack, which is not DMA-safe and violates SPI subsystem requirements.
- [High] The pointer returned by `spi_get_device_match_data()` is unconditionally dereferenced without a NULL check.
- [Medium] The IIO scale attribute calculation ignores the hardware gain when the internal reference is used, returning an incorrect scale factor.
--

commit db89a9af22e26ad0341eb42f1659ed8a2fad0e51
Author: Lukas Metz <lukas.metz@gmx.net>

iio: dac: dac8163: Add driver for DAC8163

This commit adds a new driver for the Texas Instruments DAC756x, DAC816x,
and DAC856x series devices. These are low-power, voltage-output,
dual-channel DACs with up to 16 bits of resolution.

> diff --git a/drivers/iio/dac/ti-dac8163.c b/drivers/iio/dac/ti-dac8163.c
> new file mode 100644
> index 0000000000000..84a9dfb5347d6
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac8163.c

[ ... ]

> +static int dacxx6x_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	struct dacxx6x_state *st;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		st = iio_priv(indio_dev);
> +		*val = st->vref_uv / MILLI; /* vref in mV */
> +		*val2 = chan->scan_type.realbits;

[Severity: Medium]
Does this scale calculation account for the hardware gain when using the
internal reference?

When st->internal_ref is true, the internal reference is 2.5V, but these
devices typically have a default internal gain of 2, making the full-scale
voltage 5V.

Should the gain multiplier be factored into the vref_uv calculation before
exposing the scale to userspace?

> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int dacxx6x_write_reg(struct dacxx6x_state *st, int reg, int addr,
> +			     unsigned int val)
> +{
> +	u8 tx[3];
> +
> +	tx[0] = COMMAND_SET(reg, addr);
> +	tx[1] = (val >> 8) & 0xff;
> +	tx[2] = val & 0xff;
> +
> +	return spi_write(st->spi, tx, sizeof(tx));

[Severity: High]
Is it safe to pass a stack-allocated buffer to spi_write() here?

Since spi_write() maps the provided buffer for DMA, passing a stack
variable can lead to invalid physical address mappings or memory corruption,
especially on systems with CONFIG_VMAP_STACK enabled.

Could the tx buffer be moved into the dacxx6x_state structure, protected by
the lock, and properly cacheline-aligned to ensure it is DMA-safe?

> +}

[ ... ]

> +static int dacxx6x_probe(struct spi_device *spi)
> +{

[ ... ]

> +	ret = dacxx6x_write_reg(st, CMD_SEL_REFERENCE, 0,
> +				FIELD_PREP(VOLTAGE_REFERENCE_MASK, st->internal_ref));
> +
> +	if (ret < 0)
> +		goto err;
> +
> +	info = spi_get_device_match_data(spi);
> +
> +	indio_dev->name = info->name;

[Severity: High]
Can spi_get_device_match_data() return NULL here if the device is
instantiated from userspace via sysfs (using driver_override and bind)
with a name that doesn't match the ID tables?

If it does return NULL, the subsequent dereference of info->name will cause
a kernel oops. Is a NULL check needed before accessing info?

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &dacxx6x_iio_info;
> +	indio_dev->channels = info->channels;
> +	indio_dev->num_channels = 2;
> +
> +	ret = iio_device_register(indio_dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-dac8163-work-v1-0-5b508158faa0@gmx.net?part=1