Re: [PATCH v6 10/12] iio: dac: ad5686: add control_sync() for single-channel devices

[Rodrigo Alencar]

On 26/05/05 01:35PM, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>
> 
> Create ad5310_control_sync() and ad5683_control_sync() functions that
> properly consume the mask definitions with FIELD_PREP(). This allows to
> reuse a function that updates the control register with cached values,
> without relying on confusing logic that depends on st->use_internal_vref,
> which is initialized earlier in ad5686_probe() because it is also
> applicable to the AD5686_REGMAP case, removing the need for the
> has_external_vref. Powerdown masks initialization is simplified as
> *_control_sync() masks outs any unused bits for the single-channel case.
> The change cleans up ad5686_write_dac_powerdown() and ad5686_probe(),
> organizing the code for feature extension, e.g. gain control support for
> single-channel devices.

...

> @@ -471,13 +490,12 @@ int ad5686_probe(struct device *dev,
>  	if (ret < 0 && ret != -ENODEV)
>  		return ret;
>  
> -	has_external_vref = ret != -ENODEV;
> -	st->vref_mv = has_external_vref ? ret / 1000 : st->chip_info->int_vref_mv;
> +	st->use_internal_vref = ret == -ENODEV;
> +	st->vref_mv = st->use_internal_vref ? st->chip_info->int_vref_mv : ret / 1000;
>  
> -	/* Initialize masks to all ones provided the max shift (last channel) */
> -	shift = ad5686_pd_mask_shift(&st->chip_info->channels[st->chip_info->num_channels - 1]);
> -	st->pwr_down_mask = GENMASK(shift + 1, 0);
> -	st->pwr_down_mode = GENMASK(shift + 1, 0);
> +	/* Initialize masks to all ones */
> +	st->pwr_down_mask = ~0;
> +	st->pwr_down_mode = ~0;

Sashiko's feedback:

	https://sashiko.dev/#/patchset/20260505-ad5686-fixes-v6-0-c2d5f7be32be%40analog.com?part=10

	Does initializing these masks to ~0 result in writing 1s to reserved
	hardware bits for multi-channel DACs?
	The initialization loop in the probe function only updates the bits for
	active channels. For devices with fewer than 8 channels per register, such
	as the 4-channel AD5686, the upper bits (8-31) remain set to 1.
	Earlier in ad5686_write_dac_powerdown(), the transmission payload for
	AD5686_REGMAP multi-channel devices is extracted using:
	    val = lower_16_bits(val);
	For a 4-channel device, this results in bits 8-15 being set to 1. When
	st->write() transmits this value to the AD5686_CMD_POWERDOWN_DAC register,
	it appears to violate the hardware specification which states that bits
	15-8 are reserved and must be written as 0.

In fact, DB15 to DB8 are don't care, so this is not a concern. That is also the
case for dual-channel devices, which requires unused bits to be one (gap between
channel 0 and channel 1).

...  

-- 
Kind regards,

Rodrigo Alencar