Re: [PATCH v2 2/7] iio: adc: Add basic support for AD4170

Marcelo Schmitt
2025-05-12T13:22:33Z

...
>
> > +#define AD4170_EXT_CLOCK_MHZ_MIN (1 * MEGA)
> > +#define AD4170_EXT_CLOCK_MHZ_MAX (17 * MEGA)
>
> Ditto. But are you sure that it's 17 and not 16?

The maximum external clock rates shown in the datasheet are 17 MHz.
The minimum clock low/high times are 27.6 ns, resulting in roughly 18 MHz.
So, the specifications indicate that 17 MHz clock would be okay to operate AD4170.

...
> ...
>
> > + nv = (u64)chan_info->input_range_uv * NANO;
> > + lshift = (pga >> 3 & 1); /* handle cases 8 and 9 */
>
> lshift = !!(pga & BIT(3));
>
> ?
>
> > + rshift = precision_bits - bipolar + (pga & 0x7) - lshift;
>
> GENMASK() ?

rshift = precision_bits - bipolar + (pga & GENMASK(2, 0)) - lshift;
?

> ...
>
> This was really hard to review and I am sure I missed something.
>
> Please, split this to 3+ patches (basic + feature1 + feature2 + ...).

Sorry about that. This part is so flexible that even basic ADC channel support
becomes quite complex due to the dynamic setup configurations and possible
combinations of input type (differential / pseudo-diff) and PGA. For v3, I split
calib gain, offset, and samp freq into separate patches. The PGA, though, is
harder to disentangle from the rest of the base driver.

Thanks,
Marcelo