RE: [PATCH v3 2/4] iio: adc: ad4691: add initial driver for AD4691 family

From: Sabau, Radu bogdan

Date: Mon Mar 16 2026 - 11:39:43 EST




> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Sent: Friday, March 13, 2026 12:58 PM
> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>
>
> > + u32 acc_mask[2] = { mask & 0xFF, mask >> 8 };
>
> This looks quite wrong. Is it for sure like two 32-bit stances per each mask
> byte? If not, this should be __le16 acc_mask = cpu_to_le16(~BIT(...));
>

Hi Andy,

Each acc_mask has its own register, therefore the u32 acc_mask[2] is
intentional - since the regmap is configured with val_bits=32 - the 4-byte
stride matches what regmap reads. However, I understand how this
can be confusing for anyone reading the code, therefore I propose
two ways for this :

1. Keep regmap_bulk_write and add a comment above acc_mask explaining
why u32 is used, although these register values are 8 bits.
2. Switch to regmap_multi_reg_write, which takes explicit (reg, value) pairs
and sidesteps the ambiguity entirely.

Do you have a preference?

...

> > + if (!rst)
> > + return 0;
>
> Is this required? I mean if reset APIs are NULL-aware, this will be just 300 µs
> sleep.
>

You are right about this. Also, I will implement a software reset in this case
as per David's suggestion.

Thanks,
Radu