Re: [PATCH v2 1/2] iio: adc: ad4695: add offload-based oversampling support

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 13 Jan 2025 11:49:49 -0500
Trevor Gamblin <tgamblin@baylibre.com> wrote:

> On 2025-01-13 09:35, Nuno Sá wrote:
> > On Thu, 2025-01-09 at 13:47 -0500, Trevor Gamblin wrote:  
> >> Add support for the ad4695's oversampling feature when SPI offload is
> >> available. This allows the ad4695 to set oversampling ratios on a
> >> per-channel basis, raising the effective-number-of-bits from 16
> >> (OSR == 1) to 17 (4), 18 (16), or 19 (64) for a given sample (i.e. one
> >> full cycle through the auto-sequencer). The logic for reading and
> >> writing sampling frequency for a given channel is also adjusted based on
> >> the current oversampling ratio.
> >>
> >> The non-offload case isn't supported as there isn't a good way to
> >> trigger the CNV pin in this mode. Support could be added in the future
> >> if a use-case arises.
> >>
> >> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> >> ---  
> > LGTM, just one small thing inline... Either way:
> >
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> >  
> >>   drivers/iio/adc/ad4695.c | 333 ++++++++++++++++++++++++++++++++++++++++++----
> >> -
> >>   1 file changed, 303 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
> >> index c8cd73d19e86..0caaeaa310ed 100644
> >> --- a/drivers/iio/adc/ad4695.c
> >> +++ b/drivers/iio/adc/ad4695.c
> >> @@ -79,6 +79,7 @@
> >>   #define   AD4695_REG_CONFIG_IN_MODE			  BIT(6)
> >>   #define   AD4695_REG_CONFIG_IN_PAIR			  GENMASK(5, 4)
> >>   #define   AD4695_REG_CONFIG_IN_AINHIGHZ_EN		  BIT(3)
> >> +#define   AD4695_REG_CONFIG_IN_OSR_SET			  GENMASK(1, 0)
> >>   #define AD4695_REG_UPPER_IN(n)				(0x0040 | (2 * (n)))
> >>   #define AD4695_REG_LOWER_IN(n)				(0x0060 | (2 * (n)))
> >>   #define AD4695_REG_HYST_IN(n)				(0x0080 | (2 * (n)))
> >> @@ -127,6 +128,7 @@ struct ad4695_channel_config {
> >>   	bool bipolar;
> >>   	enum ad4695_in_pair pin_pairing;
> >>   	unsigned int common_mode_mv;
> >> +	unsigned int oversampling_ratio;
> >>   };
> >>     
> > ...
> >  
> >> +
> >> +static unsigned int ad4695_get_calibbias(int val, int val2, int osr)
> >> +{
> >> +	int val_calc, scale;
> >> +
> >> +	switch (osr) {
> >> +	case 4:
> >> +		scale = 4;
> >> +		break;
> >> +	case 16:
> >> +		scale = 2;
> >> +		break;
> >> +	case 64:
> >> +		scale = 1;
> >> +		break;
> >> +	default:
> >> +		scale = 8;
> >> +		break;
> >> +	}
> >> +
> >> +	val = clamp_t(int, val, S32_MIN / 8, S32_MAX / 8);
> >> +  
> > Why not clamp()? AFAICS, we have the same type on all the arguments. I also
> > think clamp*() macros got the same improvements as min/max() ones which means
> > that using the ones with explicit casts are not so often needed anymore. My
> > understanding is also that those macros are not that encouraged as it's easy to
> > go wrong with the casts.  
> I have no preference, this is just a recent habitual use of clamp_t(). If
> clamp() is preferred I can send a v3. Or maybe Jonathan can tweak it 
> when it is

I've left it as clamp_T for now.  We can always follow up with a series
to tidy these up general.

Series applied though a bit provisionally as the SPI offload set needed
a few tweaks that might get changed.

Pushed out as testing for now.

Thanks,

Jonathan

> 
> eventually applied?
> 
> - Trevor
> 
> >
> > - Nuno Sá