On Tue, Apr 30, 2024 at 11:30 AM Alisa-Dariana Roman
<alisadariana@xxxxxxxxx> wrote:
>
> Unlike the other AD719Xs, AD7194 has configurable channels. The user can
> dynamically configure them in the devicetree.
>
> Also modify config AD7192 description for better scaling.
>
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@xxxxxxxxxx>
> ---
> drivers/iio/adc/Kconfig | 11 +++-
> drivers/iio/adc/ad7192.c | 129 +++++++++++++++++++++++++++++++++++++--
> 2 files changed, 133 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..74fecc284f1a 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -88,12 +88,17 @@ config AD7173
> called ad7173.
>
> config AD7192
> - tristate "Analog Devices AD7190 AD7192 AD7193 AD7195 ADC driver"
> + tristate "Analog Devices AD7192 and similar ADC driver"
> depends on SPI
> select AD_SIGMA_DELTA
> help
> - Say yes here to build support for Analog Devices AD7190,
> - AD7192, AD7193 or AD7195 SPI analog to digital converters (ADC).
> + Say yes here to build support for Analog Devices SPI analog to digital
> + converters (ADC):
> + - AD7190
> + - AD7192
> + - AD7193
> + - AD7194
> + - AD7195
> If unsure, say N (but it's safe to say "Y").
>
> To compile this driver as a module, choose M here: the
> diff --git a/drivers/iio/adc/ad7192.c b/drivers/iio/adc/ad7192.c
> index 3e797ff48086..0f6ecf953559 100644
> --- a/drivers/iio/adc/ad7192.c
> +++ b/drivers/iio/adc/ad7192.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * AD7190 AD7192 AD7193 AD7195 SPI ADC driver
> + * AD7192 and similar SPI ADC driver
> *
> * Copyright 2011-2015 Analog Devices Inc.
> */
> @@ -129,10 +129,21 @@
> #define AD7193_CH_AIN8 0x480 /* AIN7 - AINCOM */
> #define AD7193_CH_AINCOM 0x600 /* AINCOM - AINCOM */
>
> +#define AD7194_CH_POS(x) (((x) - 1) << 4)
> +#define AD7194_CH_NEG(x) ((x) - 1)
> +#define AD7194_CH(pos, neg) \
> + (((neg) == 0 ? BIT(10) : AD7194_CH_NEG(neg)) | AD7194_CH_POS(pos))
I think this needs a comment that BIT(10) is the CON18 flag for
pseudo-differential channels.
Also, it would probably be easier to understand if there were two
different macros, one for "single-channel" and one for "diff-channels"
rather than having neg==0 magically turn in to the pseudo flag.
> +#define AD7194_CH_TEMP 0x100 /* Temp sensor */
> +#define AD7194_CH_BASE_NR 2
> +#define AD7194_CH_AIN_START 1
> +#define AD7194_CH_AIN_NR 16
> +#define AD7194_CH_MAX_NR 272
> +
> /* ID Register Bit Designations (AD7192_REG_ID) */
> #define CHIPID_AD7190 0x4
> #define CHIPID_AD7192 0x0
> #define CHIPID_AD7193 0x2
> +#define CHIPID_AD7194 0x3
> #define CHIPID_AD7195 0x6
> #define AD7192_ID_MASK GENMASK(3, 0)
>
> @@ -170,6 +181,7 @@ enum {
> ID_AD7190,
> ID_AD7192,
> ID_AD7193,
> + ID_AD7194,
> ID_AD7195,
> };
>
> @@ -179,6 +191,7 @@ struct ad7192_chip_info {
> const struct iio_chan_spec *channels;
> u8 num_channels;
> const struct iio_info *info;
> + int (*parse_channels)(struct iio_dev *indio_dev);
> };
>
> struct ad7192_state {
> @@ -932,6 +945,15 @@ static const struct iio_info ad7192_info = {
> .update_scan_mode = ad7192_update_scan_mode,
> };
>
> +static const struct iio_info ad7194_info = {
> + .read_raw = ad7192_read_raw,
> + .write_raw = ad7192_write_raw,
> + .write_raw_get_fmt = ad7192_write_raw_get_fmt,
> + .read_avail = ad7192_read_avail,
> + .validate_trigger = ad_sd_validate_trigger,
> + .update_scan_mode = ad7192_update_scan_mode,
It looks like ad7192_update_scan_mode() won't work on ad7194 since the
channels are specified via DT for this chip and could be anything.
Each scan_index no longer corresponds a specific bit in
AD7192_REG_CONF so it would end up with some random configuration.
> +};
> +
The rest looks fine other than what Andy mentioned already.
