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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Incorrect DAC scale/gain calculation due to hardcoding the output buffer gain based on the reference type rather than the chip variant.
- [High] The driver ignores the `clear-gpios` property defined in the DT binding, potentially leaving the DAC stuck in reset.
- [High] The LDAC GPIO is actively driven before the AVDD power supply is enabled, violating power sequencing constraints.
- [Low] Passing `int *val` directly to `regmap_read()` which expects an `unsigned int *` causes a compiler error.
--

--- Patch [2]: [PATCH v3 2/2] iio: dac: dac8163: Add driver for DAC8163 ---
commit 37b06ebefbaf9fd7f117618ad2ee0f83d7ce1b9c
Author: Lukas Metz <lukas.metz@gmx.net>

iio: dac: dac8163: Add driver for DAC8163

This patch adds a new IIO driver for the Texas Instruments DAC756x, DAC816x,
and DAC856x family of digital-to-analog converters. These devices are
low-power, voltage-output, dual-channel DACs with up to 16-bit resolution.

> diff --git a/drivers/iio/dac/ti-dac8163.c b/drivers/iio/dac/ti-dac8163.c
> new file mode 100644
> index 0000000000000..35af7828dc22e
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac8163.c
[ ... ]
> +static int dac8163_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct dac8163_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		ret = regmap_read(st->regmap,
> +				  FIELD_PREP(COMMAND_MASK, CMD_WRITE_UPDATE) |
> +				  FIELD_PREP(ADDRESS_MASK, chan->channel),
> +				  val);

[Severity: Low]
Does this cause a compiler error with modern compilers? It looks like val is
an int pointer, but regmap_read() expects an unsigned int pointer, which
might trigger an incompatible pointer types warning or error.

> +		if (ret)
> +			return ret;
> +		*val >>= chan->scan_type.shift;
> +		return IIO_VAL_INT;
> +	}
[ ... ]
> +static int dac8163_probe(struct spi_device *spi)
> +{
> +	const struct dac8163_chip_info *info;
> +	struct gpio_desc *ldac_gpio;
> +	struct iio_dev *indio_dev;
> +	struct dac8163_state *st;
> +	bool internal_reference;
> +	int ret;
[ ... ]
> +	// for now we keep the ldac pin asserted permanently so that the output
> +	// is updated immediately after a write to the channels raw attribute
> +	ldac_gpio = devm_gpiod_get_optional(&spi->dev, "ldac",
> +					    GPIOD_OUT_HIGH);

[Severity: High]
Does this driver need to handle the clear-gpios property defined in its DT
binding? If the hardware CLR pin is wired to a GPIO that defaults to the
active state, it seems the DAC might be left stuck in reset because the pin is
never deasserted.

[Severity: High]
Can driving the ldac GPIO high here back-power the unpowered IC? The GPIO is
driven to an active output state before the avdd regulator is enabled below,
which might forward-bias internal ESD protection diodes.

> +	if (IS_ERR(ldac_gpio))
> +		return dev_err_probe(&spi->dev, PTR_ERR(ldac_gpio),
> +				     "failed to get ldac gpio");
> +
> +	ret = devm_regulator_get_enable(&spi->dev, "avdd");
> +	if (ret < 0)
> +		return dev_err_probe(&spi->dev, ret,
> +				     "failed to get avdd voltage\n");
> +
> +	if (device_property_present(&spi->dev, "vrefin-supply")) {
> +		ret = devm_regulator_get_enable_read_voltage(&spi->dev,
> +							     "vrefin");
> +		if (ret < 0)
> +			return dev_err_probe(&spi->dev, ret,
> +					     "failed to get vrefin voltage\n");
> +
> +		st->vref_mV = ret / (MICRO / MILLI);
> +		internal_reference = false;
> +		st->gain = 1;
> +	} else {
> +		st->vref_mV = DAC8163_INTERNAL_REF_mV;
> +		internal_reference = true;
> +		st->gain = 2;
> +	}

[Severity: High]
Is this gain calculation correct? The code hardcodes the output buffer gain
based on whether the voltage reference is internal or external. It seems the
gain should be a fixed hardware property of the specific chip variant instead,
such as 1x for variant 2 models and 2x for variant 3 models.

> +
> +	ret = regmap_write(st->regmap, FIELD_PREP(COMMAND_MASK, CMD_SOFT_RST),
> +			   FULL_RESET);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802-dac8163-work-v3-0-3ecc7bc66d0d@gmx.net?part=2