Re: [PATCH v4 6/6] iio: dac: ad3530r: Add support for AD3532R/AD3532

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The max_register parameter in the regmap config is incorrectly set to the base address of the highest channel, preventing reading and writing 16-bit values for the last channel.
--

commit 977833b9e38150088aceb9bef325ef263ed54eee
Author: Kim Seer Paller <kimseer.paller@analog.com>

iio: dac: ad3530r: Add support for AD3532R/AD3532
    
This commit adds support for the AD3532 and AD3532R 16-channel 16-bit voltage
output DACs. It adds a new register map, channel specifications, and powerdown
handlers while reusing shared table-driven configuration steps.

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> --- a/drivers/iio/dac/ad3530r.c
> +++ b/drivers/iio/dac/ad3530r.c
[ ... ]
> @@ -39,6 +40,25 @@
>  #define AD3531R_SW_LDAC_TRIG_A			0xDD
>  #define AD3531R_INPUT_CH			0xE3
>  
> +/* AD3532R/AD3532 bank 0 registers (channels 0-7) */
> +#define AD3532R_INTERFACE_CONFIG_A_0		0x1000
> +#define AD3532R_OUTPUT_OPERATING_MODE_0		0x1020
> +#define AD3532R_OUTPUT_OPERATING_MODE_1		0x1021
> +#define AD3532R_OUTPUT_CONTROL_0		0x102A
> +#define AD3532R_REFERENCE_CONTROL_0		0x103C
> +#define AD3532R_SW_LDAC_TRIG_0			0x10E5
> +#define AD3532R_INPUT_CH_0			0x10EB
> +
> +/* AD3532R/AD3532 bank 1 registers (channels 8-15) */
> +#define AD3532R_INTERFACE_CONFIG_A_1		0x3000
> +#define AD3532R_OUTPUT_OPERATING_MODE_2		0x3020
> +#define AD3532R_OUTPUT_OPERATING_MODE_3		0x3021
> +#define AD3532R_OUTPUT_CONTROL_1		0x302A
> +#define AD3532R_REFERENCE_CONTROL_1		0x303C
> +#define AD3532R_SW_LDAC_TRIG_1			0x30E5
> +#define AD3532R_INPUT_CH_1			0x30EB
> +#define AD3532R_MAX_REG_ADDR			0x30F9

[ ... ]

> @@ -369,12 +490,40 @@ static const unsigned int ad3531r_op_mode[] = {
>  	AD3530R_OUTPUT_OPERATING_MODE_0,
>  };
>  
> +static const unsigned int ad3532r_if_config[] = {
> +	AD3532R_INTERFACE_CONFIG_A_0,
> +	AD3532R_INTERFACE_CONFIG_A_1,
> +};
> +
[ ... ]
> +static const struct regmap_config ad3532r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD3532R_MAX_REG_ADDR,
> +};

[Severity: High]
Does this configuration prevent 16-bit reads and writes on the highest channel
(channel 15)?

Looking at ad3532r_input_ch_reg(), the input register for channel 15 resolves
to 0x30F9 (0x30EB + 2 * 7). Since ad3530r_dac_write() and ad3530r_read_raw()
both perform a 2-byte bulk regmap access starting at this address:

ad3530r_dac_write() {
    ...
    ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
                            &st->buf, sizeof(st->buf));
    ...
}

Regmap will check if the start address + length - 1 <= max_register. For
channel 15, this evaluates to 0x30FA <= 0x30F9, which fails the bounds check
and returns -EINVAL.

Should AD3532R_MAX_REG_ADDR be set to 0x30FA to accommodate the second byte of
the final channel's input register?

(Note that AD3530R_MAX_REG_ADDR might also have this exact same issue for
channel 7 in the pre-existing code).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-iio-ad3532r-support-v4-0-69d9a336f4e8@analog.com?part=6