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

[Trevor Gamblin <tgamblin@baylibre.com>]

On 2025-02-10 14:03, Jonathan Cameron wrote:
> 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
Thank you!