Re: [PATCH v2 4/4] iio: dac: ad3530r: add support for AD5710R/AD5711R

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The `powerdown_mode` sysfs attribute is exposed with write permissions despite being read-only, causing write attempts to fail unconditionally with `-EINVAL`.
--

--- Patch [4]: [PATCH v2 4/4] iio: dac: ad3530r: add support for AD5710R/AD5711R ---
commit 3c7f6e84bbb3d2178089f97875a1f7dc3599d417
Author: Kim Seer Paller <kimseer.paller@analog.com>

iio: dac: ad3530r: add support for AD5710R/AD5711R

This patch adds support for the AD5710R/AD5711R 8-channel configurable 
IDAC/VDAC parts. The new devices are folded into the ad3530r driver as 
they share the same register map and access model.

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> index 97483534f8c33..935b3a832d8e9 100644
> --- a/drivers/iio/dac/ad3530r.c
> +++ b/drivers/iio/dac/ad3530r.c

[ ... ]

> @@ -194,6 +210,26 @@ static const struct iio_enum ad3532r_powerdown_mode_enum = {
>  	.set = ad3530r_set_powerdown_mode,
>  };
>  
> +static int ad5710r_get_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(st->regmap, AD5710R_CHN_VMODE_EN, &val);
> +	if (ret)
> +		return ret;
> +
> +	return !(val & AD5710R_CHN_VMODE_EN_BIT(chan->channel));
> +}
> +
> +static const struct iio_enum ad5710r_powerdown_mode_enum = {
> +	.items = ad5710r_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ad5710r_powerdown_modes),
> +	.get = ad5710r_get_powerdown_mode,
> +};
> +

[ ... ]

> @@ -419,6 +506,17 @@ static const struct iio_chan_spec_ext_info ad3532r_ext_info[] = {
>  	{ }
>  };
>  
> +static const struct iio_chan_spec_ext_info ad5710r_ext_info[] = {
> +	{
> +		.name = "powerdown",
> +		.shared = IIO_SEPARATE,
> +		.read = ad5710r_get_dac_powerdown,
> +		.write = ad5710r_set_dac_powerdown,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad5710r_powerdown_mode_enum),

[Severity: Low]
Does using the IIO_ENUM macro here incorrectly expose the attribute as writable?

Since ad5710r_powerdown_mode_enum lacks a .set callback, any writes to the
powerdown_mode sysfs attribute will fail unconditionally with -EINVAL. 

However, the IIO_ENUM macro automatically assigns iio_enum_write to the .write 
callback, which grants write permissions in sysfs. Should this attribute be 
manually defined in the ext_info array without a .write callback to properly 
enforce read-only semantics at the VFS layer?

> +	{ }
> +};
> +
>  #define AD3530R_CHAN(_chan, _ext_info)				\
>  {								\
>  	.type = IIO_VOLTAGE,					\

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-iio-ad5710r-upstream-v2-0-324949dc72da@analog.com?part=4