Re: [PATCH v6 09/17] iio: adc: ad7768: Configure channel sampling profiles

[Jonathan Cameron]

> At buffered capture setup, select the fastest compatible power mode for
> the enabled channel rates. Group channels into the two hardware mode
> profiles and program their decimation, mode selection, and data clock.
> 
> Signed-off-by: Janani Sunil <janani.sunil@xxxxxxxxxx>
There are a couple of sashiko comments on this.
I'm not sure what they are going on about, but please take a look
anyway!


>  
> +static u8 ad7768_channel_mode_mask(const struct ad7768_state *st, u8 ch)
> +{
> +	return BIT(ch) | ad7768_channel_mask(st, ch);

[Severity: Medium]
Does this bitwise OR inadvertently combine the logical channel index and the
physical channel mapping?

When this is applied to the AD7768_REG_CH_MODE_SEL register, it could result
in writing to reserved or unmapped hardware bits, which might lead to
unpredictable hardware state.

--

I'm not really sure what this code is trying to do so hard to comment
on whether sashiko is right or not.  If nothing else this code
needs a comment as it is takig the | of the raw position with one
that has been through the channel map.

> +}
> +
>  static u8 ad7768_all_standby_mask(const struct ad7768_state *st)
>  {
>  	return GENMASK(st->chip_info->num_channels - 1, 0);

> @@ -478,11 +544,66 @@ static int ad7768_update_scan_mode(struct iio_dev *indio_dev,
>  	if (st->clock_source == AD7768_CLOCK_SOURCE_XTAL)
>  		__clear_bit(st->chip_info->num_channels / 2, &standby_mask);
>  
> +	for_each_set_bit(c, scan_mask, st->chip_info->num_channels) {
> +		unsigned int mask;
> +		int mode;
> +
> +		mode = ad7768_find_matching_mode(mode_used, mode_freq,
> +						 st->ch_freq[c]);
> +		if (mode < 0)
> +			return dev_err_probe(dev, -EINVAL,
> +					     "Over %d channel modes required\n",
> +					     AD7768_NUM_CHANNEL_MODES);
> +
> +		mode_freq[mode] = st->ch_freq[c];
> +		mode_used[mode] = true;
> +
> +		mask = ad7768_channel_mode_mask(st, c);
> +		ret = regmap_assign_bits(st->regmap, AD7768_REG_CH_MODE_SEL,
> +					 mask, mode);

[Severity: Medium]
This applies the multi-bit mask to the hardware configuration register.

Could this configure unintended channels or interfere with device compatibility?
-
This is just the end of what happens with the above.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>