Re: [PATCH v2 3/3] iio: adc: ad7192: Add fast settling support

[Jonathan Cameron]

On Wed, 20 Sep 2023 03:33:42 +0300
alisadariana@xxxxxxxxx wrote:

> From: Alisa-Dariana Roman <alisadariana@xxxxxxxxx>
> 
> Add fast settling mode support for AD7193.
> 
> Add fast_settling_average_factor attribute. Expose to user the current
> value of the fast settling average factor. User can change the value by
> writing a new one.
> 
> Add fast_settling_average_factor_available attribute. Expose to user
> possible values for the fast settling average factor.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---
>  .../ABI/testing/sysfs-bus-iio-adc-ad7192      |  18 +++
>  drivers/iio/adc/ad7192.c                      | 128 ++++++++++++++++--
>  2 files changed, 134 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7192 b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7192
> index f8315202c8f0..5790adbb1cc1 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7192
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-ad7192
> @@ -19,6 +19,24 @@ Description:
>  		the bridge can be disconnected (when it is not being used
>  		using the bridge_switch_en attribute.
>  
> +What:		/sys/bus/iio/devices/iio:deviceX/fast_settling_average_factor
> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		This attribute, if available, is used to activate or deactivate
> +		fast settling mode and set the value of the average factor to
> +		1, 2, 8 or 16. If the average factor is set to 1, the fast
> +		settling mode is disabled. The data from the sinc filter is
> +		averaged by chosen value. The averaging reduces the output data
> +		rate for a given FS word, however, the rms noise improves.

Trivial but RMS as it's an acronym.

> +
> +What:		/sys/bus/iio/devices/iio:deviceX/fast_settling_average_factor_available
> +KernelVersion:
> +Contact:	linux-iio@xxxxxxxxxxxxxxx
> +Description:
> +		Reading returns a list with the possible values for the fast
> +		settling average factor: 1, 2, 8, 16.
> +
>  What:		/sys/bus/iio/devices/iio:deviceX/in_voltagex_sys_calibration
>  KernelVersion:
>  Contact:	linux-iio@xxxxxxxxxxxxxxx
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index eed3de02c26d..8987b78865f3 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -60,6 +60,8 @@
>  #define AD7192_MODE_SEL_MASK	GENMASK(23, 21) /* Operation Mode Select Mask */
>  #define AD7192_MODE_STA_MASK	BIT(20) /* Status Register transmission Mask */
>  #define AD7192_MODE_CLKSRC_MASK	GENMASK(19, 18) /* Clock Source Select Mask */
> +#define AD7192_MODE_AVG_MASK	GENMASK(17, 16)
> +		  /* Fast Settling Filter Average Select Mask (AD7193 only) */
>  #define AD7192_MODE_SINC3	BIT(15) /* SINC3 Filter Select */
>  #define AD7192_MODE_ENPAR	BIT(13) /* Parity Enable */
>  #define AD7192_MODE_CLKDIV	BIT(12) /* Clock divide by 2 (AD7190/2 only)*/
> @@ -182,6 +184,7 @@ struct ad7192_state {
>  	u32				mode;
>  	u32				conf;
>  	u32				scale_avail[8][2];
> +	u8				avg_avail[4];
>  	u8				gpocon;
>  	u8				clock_sel;
>  	struct mutex			lock;	/* protect sensor state */
> @@ -459,6 +462,13 @@ static int ad7192_setup(struct iio_dev *indio_dev, struct device_node *np)
>  		st->scale_avail[i][0] = scale_uv;
>  	}
>  
> +	if (st->chip_info->chip_id == CHIPID_AD7193) {

Hmm. This does match with local style, but I'd have preferred if the driver
put all this information in the chip_info structure rather than scattered in code
thoughout the driver.  That would be a bigger and mostly unrelated cleanup however
and this doesn't make the situation any worse really so I'm fine with this
patch as it stands.

Jonathan


> +		st->avg_avail[0] = 1;
> +		st->avg_avail[1] = 2;
> +		st->avg_avail[2] = 8;
> +		st->avg_avail[3] = 16;
> +	}
> +
>  	return 0;
>  }
>