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

[Sabau, Radu bogdan]

> -----Original Message-----
> From: David Lechner <dlechner@xxxxxxxxxxxx>
> Sent: Monday, March 16, 2026 5:51 PM
> To: Sabau, Radu bogdan <Radu.Sabau@xxxxxxxxxx>; Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxx>
> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>; Hennerich, Michael
> <Michael.Hennerich@xxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>;
> Sa, Nuno <Nuno.Sa@xxxxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxx>;
> Rob Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>;
> Conor Dooley <conor+dt@xxxxxxxxxx>; Uwe Kleine-König
> <ukleinek@xxxxxxxxxx>; Liam Girdwood <lgirdwood@xxxxxxxxx>; Mark Brown
> <broonie@xxxxxxxxxx>; Linus Walleij <linusw@xxxxxxxxxx>; Bartosz
> Golaszewski <brgl@xxxxxxxxxx>; Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>;
> linux-iio@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-pwm@xxxxxxxxxxxxxxx; linux-
> gpio@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 2/4] iio: adc: ad4691: add initial driver for AD4691
> family
> 
> [External]
> 
> On 3/16/26 10:29 AM, Sabau, Radu bogdan wrote:
> >
> >
> >> -----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?
> 
> Since we already have a custom read/write functions to handle different
> register sizes and the chip can read more than one consecutive register
> at once, can we just call this a single register and add a special case
> to ad4691_reg_read/write() to handle it? Then we can just do a regular
> regmap_read/write() functions to access it as a single 16-bit value.

This sounds even better! I will have this in the next version!