Received-SPF: pass (google.com: domain of linux-kernel+bounces-89774-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 3 Mar 2024 14:34:29 +0000
From: Jonathan Cameron <jic23@kernel.org>
To: Dumitru Ceclan <mitrutzceclan@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski
 <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>,
 David Lechner <dlechner@baylibre.com>, Ceclan Dumitru
 <dumitru.ceclan@analog.com>, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] iio: adc: ad7173: add support for additional
 models
Message-ID: <20240303143429.68fd5cd4@jic23-huawei>
In-Reply-To: <20240228135532.30761-3-mitrutzceclan@gmail.com>
References: <20240228135532.30761-1-mitrutzceclan@gmail.com>
	<20240228135532.30761-3-mitrutzceclan@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 28 Feb 2024 15:54:57 +0200
Dumitru Ceclan <mitrutzceclan@gmail.com> wrote:

> Add support for Analog Devices AD7172-2, AD7175-8, AD7177-2.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
Hi Dumitru

Some of the changes in here make it clear more inter chip variability
should be specified directly in your device_info structures, and less
(preferably none) use made of the id field + if statements inline.

Those ID fields should just be for matching, not for direct use to adjust
code flow.

Jonathan


> ---
>  drivers/iio/adc/ad7173.c | 82 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 74 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index b42fbe28a325..e60ecce20e08 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1,6 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0+
>  /*
> - * AD7172-2/AD7173-8/AD7175-2/AD7176-2 SPI ADC driver
> + * AD717x family SPI ADC driver
> + *
> + * Supported devices:
> + *  AD7172-2/AD7172-4/AD7173-8/AD7175-2
> + *  AD7175-8/AD7176-2/AD7177-2
> + *
>   * Copyright (C) 2015, 2024 Analog Devices, Inc.
>   */
>  
> @@ -61,10 +66,13 @@
>  #define AD7173_AIN_TEMP_POS	17
>  #define AD7173_AIN_TEMP_NEG	18
>  
> -#define AD7172_ID			0x00d0
> -#define AD7173_ID			0x30d0
> -#define AD7175_ID			0x0cd0
> +#define AD7172_2_ID			0x00d0
>  #define AD7176_ID			0x0c90
> +#define AD7175_2_ID			0x0cd0
> +#define AD7172_4_ID			0x2050
> +#define AD7173_ID			0x30d0
> +#define AD7175_8_ID			0x3cd0
> +#define AD7177_ID			0x4fd0
>  #define AD7173_ID_MASK			GENMASK(15, 4)
>  
>  #define AD7173_ADC_MODE_REF_EN		BIT(15)
> @@ -110,15 +118,19 @@
>  #define AD7173_SETUP_REF_SEL_EXT_REF	0x0
>  #define AD7173_VOLTAGE_INT_REF_uV	2500000
>  #define AD7173_TEMP_SENSIIVITY_uV_per_C	477
> +#define AD7177_ODR_START_VALUE		0x07
>  
>  #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
>  #define AD7173_MAX_CONFIGS		8
>  
..

>  
>  static const char *const ad7173_ref_sel_str[] = {
> @@ -656,7 +701,12 @@ static int ad7173_write_raw(struct iio_dev *indio_dev,
>  	switch (info) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		freq = val * MILLI + val2 / MILLI;
> -		for (i = 0; i < st->info->num_sinc5_data_rates - 1; i++)
> +		/*
> +		 * AD7177-2 has the filter values [0-6] marked as reserved
> +		 * datasheet page 58
> +		 */
> +		i = (st->info->id == AD7177_ID) ? AD7177_ODR_START_VALUE : 0;

Add an st->info->odr_start_value field. Can set it only for this part, but
if in future others turn up that needs similar it becomes very easy to support.

> +		for (; i < st->info->num_sinc5_data_rates - 1; i++)
>  			if (freq >= st->info->sinc5_data_rates[i])
>  				break;
>  
> @@ -908,8 +958,15 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>  		else
>  			ref_sel = ret;
>  
> +		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
> +		    st->info->id == AD7172_2_ID) {

As below.  You may well end up adding more devices here. Make it data instead by
adding  has_ref1 boolean to your st->info structure.

> +			fwnode_handle_put(child);
> +			return dev_err_probe(dev, -EINVAL,
> +				"Internal reference is not available on ad7172-2\n");
> +		}
> +
>  		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 &&
> -		    st->info->id != AD7173_ID) {
> +		    st->info->id != AD7173_ID && st->info->id != AD7172_2_ID) {
This is one of those cases that clearly shows why ID matching doesn't generalize well.
Better to have a flag in info that directly says if there is an external reference 2 
possible for each chip variant.  Otherwise this list just keeps on growing!

		   !st->info->has_ref2

>  			fwnode_handle_put(child);
>  			return dev_err_probe(dev, -EINVAL,
>  				"External reference 2 is only available on ad7173-8\n");