Message-ID: <20250504184227.177d1764@jic23-huawei>
Date: Sun, 4 May 2025 18:42:27 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <lars@...afoo.de>,
 <Michael.Hennerich@...log.com>, <dlechner@...libre.com>,
 <nuno.sa@...log.com>, <andy@...nel.org>, <robh@...nel.org>,
 <krzk+dt@...nel.org>, <conor+dt@...nel.org>, <marcelo.schmitt1@...il.com>
Subject: Re: [PATCH v2 7/7] iio: adc: ad4170: Add support for weigh scale
 and RTD sensors

On Mon, 28 Apr 2025 09:29:40 -0300
Marcelo Schmitt <marcelo.schmitt@...log.com> wrote:

> The AD4170 design has features to aid interfacing with weigh scale and RTD
> sensors that are expected to be setup with external circuitry for proper
> sensor operation. A key characteristic of those sensors is that the circuit
> they are in must be excited with a pair of signals. The external circuit
> can be excited either by voltage supply or by AD4170 excitation signals.
> The sensor can then be read through a different pair of lines that are
> connected to AD4170 ADC.
> 
> Configure AD4170 to handle external circuit sensors.
> 
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.com>
Hi Marcelo

A few comments inline.

Jonathan

> ---
> changes since v1:
> - Improved to support more than one external sensor connected.
> 
>  drivers/iio/adc/ad4170.c | 375 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 372 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad4170.c b/drivers/iio/adc/ad4170.c
> index 335b4194c7eb..9e4a9b524dd4 100644
> --- a/drivers/iio/adc/ad4170.c
> +++ b/drivers/iio/adc/ad4170.c
> @@ -62,6 +62,8 @@
>  #define AD4170_FILTER_FS_REG(x)				(0xC7 + 14 * (x))
>  #define AD4170_OFFSET_REG(x)				(0xCA + 14 * (x))
>  #define AD4170_GAIN_REG(x)				(0xCD + 14 * (x))
> +#define AD4170_V_BIAS_REG				0x135
> +#define AD4170_CURRENT_SRC_REG(x)			(0x139 + 2 * (x))
>  #define AD4170_GPIO_MODE_REG				0x191
>  #define AD4170_GPIO_OUTPUT_REG				0x193
>  #define AD4170_GPIO_INPUT_REG				0x195
> @@ -110,6 +112,10 @@
>  /* AD4170_FILTER_REG */
>  #define AD4170_FILTER_FILTER_TYPE_MSK			GENMASK(3, 0)
>  
> +/* AD4170_CURRENT_SRC_REG */
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_MSK		GENMASK(12, 8)
> +#define AD4170_CURRENT_SRC_I_OUT_VAL_MSK		GENMASK(2, 0)
> +
>  /* AD4170 register constants */
>  
>  /* AD4170_CLOCK_CTRL_REG constants */
> @@ -171,6 +177,21 @@
>  #define AD4170_FILTER_FILTER_TYPE_SINC5			0x4
>  #define AD4170_FILTER_FILTER_TYPE_SINC3			0x6
>  
> +/* AD4170_CURRENT_SRC_REG constants */

These seem a bit repetitive. Can we use some macros?

#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN(x) (x)
#define AD4170_CURRENT_SRC_I_OUT_PIN_GPIO(x) (x + 17)

> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN0		0
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN1		1
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN2		2
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN3		3
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN4		4
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN5		5
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN6		6
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN7		7
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_AIN8		8
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_GPIO0		17
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_GPIO1		18
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_GPIO2		19
> +#define AD4170_CURRENT_SRC_I_OUT_PIN_GPIO3		20
> +
>  /* Device properties and auxiliary constants */
>  
>  #define AD4170_NUM_ANALOG_PINS				9
> @@ -204,6 +225,15 @@
>  #define AD4170_PIN_UNASIGNED				0x00
>  #define AD4170_PIN_ANALOG_IN				0x01
>  #define AD4170_PIN_CURRENT_OUT				0x02
> +#define AD4170_PIN_VBIAS				0x04
> +
> +/* GPIO pin functions  */
> +#define AD4170_GPIO_UNASIGNED				0x00
> +#define AD4170_GPIO_AC_EXCITATION			0x02
> +#define AD4170_GPIO_OUTPUT				0x04
> +
> +/* Current source */
> +#define AD4170_CURRENT_SRC_DISABLED			0xFF
>  
>  static const unsigned int ad4170_reg_size[] = {
>  	[AD4170_CONFIG_A_REG] = 1,
> @@ -244,6 +274,8 @@ static const unsigned int ad4170_reg_size[] = {
>  	[AD4170_OFFSET_REG(5) ... AD4170_GAIN_REG(5)] = 3,
>  	[AD4170_OFFSET_REG(6) ... AD4170_GAIN_REG(6)] = 3,
>  	[AD4170_OFFSET_REG(7) ... AD4170_GAIN_REG(7)] = 3,
> +	[AD4170_V_BIAS_REG] = 2,
> +	[AD4170_CURRENT_SRC_REG(0) ... AD4170_CURRENT_SRC_REG(3)] = 2,
>  	[AD4170_GPIO_MODE_REG] = 2,
>  	[AD4170_GPIO_OUTPUT_REG] = 2,
>  	[AD4170_GPIO_INPUT_REG] = 2,
> @@ -305,6 +337,33 @@ static const unsigned int ad4170_sinc5_filt_fs_tbl[] = {
>  	1, 2, 4, 8, 12, 16, 20, 40, 48, 80, 100, 256,
>  };
>  
> +static const unsigned int ad4170_iout_pin_tbl[] = {
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN0,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN1,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN2,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN3,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN4,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN5,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN6,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN7,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_AIN8,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_GPIO0,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_GPIO1,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_GPIO2,
> +	AD4170_CURRENT_SRC_I_OUT_PIN_GPIO3,
> +};
> +
> +static const unsigned int ad4170_iout_current_ua_tbl[] = {
> +	0, 10, 50, 100, 250, 500, 1000, 1500

I think Andy got this, but trailing comma.

> +};
> +
> +enum ad4170_sensor_type {
> +	AD4170_WEIGH_SCALE_SENSOR = 0,
> +	AD4170_RTD_SENSOR = 1,
> +	AD4170_THERMOCOUPLE_SENSOR = 2,
> +	AD4170_ADC_SENSOR = 3,
> +};
> +
>  struct ad4170_chip_info {
>  	const char *name;
>  };
> @@ -378,6 +437,8 @@ struct ad4170_state {
>  	struct clk *ext_clk;
>  	struct clk_hw int_clk_hw;
>  	int pins_fn[AD4170_NUM_ANALOG_PINS];
> +	int gpio_fn[AD4170_NUM_GPIO_PINS];
> +	unsigned int cur_src_pins[AD4170_NUM_CURRENT_SRC];
>  	struct gpio_chip gpiochip;
>  	u32 int_pin_sel;
>  	int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
> @@ -925,6 +986,19 @@ static int ad4170_get_ain_voltage_uv(struct ad4170_state *st, int ain_n,
>  	struct device *dev = &st->spi->dev;
>  
>  	*ain_voltage = 0;
> +	/*
> +	 * The voltage bias (vbias) sets the common-mode voltage of the channel
> +	 * to (AVDD + AVSS)/2. If provided, AVSS supply provides the magnitude
> +	 * (absolute value) of the negative voltage supplied to the AVSS pin.
> +	 * So, we do AVDD - AVSS to compute the DC voltage generated by the bias
> +	 * voltage generator.
> +	 */
> +	if (st->pins_fn[ain_n] & AD4170_PIN_VBIAS) {
> +		*ain_voltage = (st->vrefs_uv[AD4170_AVDD_SUP]
> +				- st->vrefs_uv[AD4170_AVSS_SUP]) / 2;
> +		return 0;
> +	}
> +
>  	if (ain_n <= AD4170_CHAN_MAP_TEMP_SENSOR)
>  		return 0;
>  
> @@ -1742,6 +1816,266 @@ static int ad4170_gpio_init(struct iio_dev *indio_dev)
>  	return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev);
>  }
>  
> +static int _ad4170_find_table_index(const unsigned int *tbl, size_t len,
> +				    unsigned int val)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < len; i++)
> +		if (tbl[i] == val)
> +			return i;
I wonder if this is worth while vs the opencoded version

	for (i = 0; i < ARRAY_SIZE(ad4170_iout_pin_tbl); i++)
		if (ad4170_iout_pin_tbl[i] == val)
			break;

	if (i == ARRAY_SIZE(ad4170_iout_pin_tbl))
		return -EINVAL;

There is a bit more repetition and opportunity for missmatches
between which table etc, but it is pretty compact and you
only have two usecases of the function via the macro.

> +
> +	return -EINVAL;
> +}
> +
> +#define ad4170_find_table_index(table, val) \
> +	_ad4170_find_table_index(table, ARRAY_SIZE(table), val)
> +
> +static int ad4170_validate_excitation_pins(struct ad4170_state *st,
> +					   u32 *exc_pins, int num_exc_pins)
> +{
> +	struct device *dev = &st->spi->dev;
> +	int ret, i;
> +
> +	for (i = 0; i < num_exc_pins; i++) {
> +		unsigned int pin = exc_pins[i];
> +
> +		ret = ad4170_find_table_index(ad4170_iout_pin_tbl, pin);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret,
> +					     "Invalid excitation pin: %u\n",
> +					     pin);
> +
> +		if (pin <= AD4170_MAX_ANALOG_PINS) {
> +			if (st->pins_fn[pin] != AD4170_PIN_UNASIGNED)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "Pin %u already used with fn %u\n",
> +						     pin, st->pins_fn[pin]);
> +
> +			st->pins_fn[pin] |= AD4170_PIN_CURRENT_OUT;
> +		} else {
> +			unsigned int gpio = pin - AD4170_CURRENT_SRC_I_OUT_PIN_GPIO0;
> +
> +			if (st->gpio_fn[gpio] != AD4170_GPIO_UNASIGNED)
> +				return dev_err_probe(dev, -EINVAL,
> +						     "GPIO %u already used with fn %u\n",
> +						     gpio, st->gpio_fn[gpio]);
> +
> +			st->gpio_fn[gpio] |= AD4170_GPIO_AC_EXCITATION;
> +		}
> +	}
> +	return 0;
> +}
> +

> +
> +static int ad4170_setup_bridge(struct ad4170_state *st,
> +			       struct fwnode_handle *child,
> +			       struct ad4170_setup *setup, u32 *exc_pins,
> +			       int num_exc_pins, int exc_cur, bool ac_excited)
> +{
> +	int ret;
> +
> +	/*
> +	 * If a specific current is provided through
> +	 * adi,excitation-current-microamp, set excitation pins provided through
> +	 * adi,excitation-pins to AC excite the bridge circuit. Else, use
> +	 * predefined ACX1, ACX1 negated, ACX2, ACX2 negated signals to AC
> +	 * excite the bridge. Those signals are output on GPIO2, GPIO0, GPIO3,
> +	 * and GPIO1, respectively. If only two pins are specified for AC
> +	 * excitation, use ACX1 and ACX2.
> +	 *
> +	 * Also, to avoid any short-circuit condition when more than one channel
> +	 * is enabled, set GPIO2 and GPIO0 high, and set GPIO1 and GPIO3 low to
> +	 * DC excite the bridge whenever a channel without AC excitation is
> +	 * selected. That is needed because GPIO pins are controlled by the next
> +	 * highest priority GPIO function when a channel doesn't enable AC
> +	 * excitation. See datasheet Figure 113 Weigh Scale (AC Excitation) for
> +	 * an example circuit diagram.
> +	 */
> +	if (exc_cur == 0 && ac_excited) {
> +		if (num_exc_pins == 2) {
> +			setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_ADC_MSK, 0x3);
> +			ret = regmap_set_bits(st->regmap,
> +					      AD4170_GPIO_MODE_REG,
> +					      BIT(7) | BIT(5));
These are fields in a register with meaning that isn't obvious here.
Should have defines to give them a 'meaning'.
I'll guess this is setting two separate 1 bit values in 2 bit fields?
Those need defines that take the index of the gpio.


> +			if (ret)
> +				return ret;
> +
> +			ret = regmap_set_bits(st->regmap,
> +					      AD4170_GPIO_OUTPUT_REG,
> +					      BIT(3) | BIT(2));

This is a bit more obvious as those are the two pins we are using
but maybe a comment.

> +			if (ret)
> +				return ret;
> +
> +			st->gpio_fn[3] |= AD4170_GPIO_OUTPUT;
> +			st->gpio_fn[2] |= AD4170_GPIO_OUTPUT;
> +		} else {
> +			setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_ADC_MSK, 0x2);
> +			ret = regmap_set_bits(st->regmap,
> +					      AD4170_GPIO_MODE_REG,
> +					      BIT(7) | BIT(5) | BIT(3) | BIT(1));
As above. Looks like a bunch of 2 bit fields with value 1?
> +			if (ret)
> +				return ret;
> +
> +			ret = regmap_set_bits(st->regmap,
> +					      AD4170_GPIO_OUTPUT_REG,
> +					      BIT(3) | BIT(2) | BIT(1) | BIT(0));
> +			if (ret)
> +				return ret;
> +
> +			st->gpio_fn[3] |= AD4170_GPIO_OUTPUT;
> +			st->gpio_fn[2] |= AD4170_GPIO_OUTPUT;
> +			st->gpio_fn[1] |= AD4170_GPIO_OUTPUT;
> +			st->gpio_fn[0] |= AD4170_GPIO_OUTPUT;
> +		}
> +
> +		return 0;
> +	}
> +
> +	return ad4170_setup_current_src(st, child, setup, exc_pins,
> +					num_exc_pins, exc_cur, ac_excited);
> +}

Powered by blists - more mailing lists