by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH v5 2/9] dt-bindings: adc: ad7173: add support for ad411x

On Mon, Jun 03, 2024 at 07:23:00PM +0300, Dumitru Ceclan via B4 Relay wrote:
> + common-mode-channel:
> + description:
> + This property is used for defining the negative input of a
> + single-ended or pseudo-differential voltage channel.
> +
> + Special inputs are valid as well.
> + minimum: 0
> + maximum: 31

I think this property, since it purports to be a common one due to the
lack of a vendor prefix, should be documented in adc.yaml.

> +
> adi,reference-select:
> description: |
> Select the reference source to use when converting on
> @@ -154,9 +213,31 @@ patternProperties:
> - avdd
> default: refout-avss
>
> + adi,current-channel:
> + description: |
> + Signal that the selected inputs are current channels.
> + Only available on AD4111 and AD4112.

> + type: boolean
Instead of boolean, use:
$ref: /schemas/types.yaml#/definitions/flag

Otherwise, I think this all looks okay.

Attachments:

(No filename) (1.04 kB)
signature.asc (235.00 B)
Download all attachments

2024-06-04 18:09:30

On 6/3/24 11:23 AM, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <[email protected]>
>
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
>
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
>
> - All ADCs have inputs with a precision voltage divider with a division
> ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> shunt resistor.
>
> Signed-off-by: Dumitru Ceclan <[email protected]>
> ---
> drivers/iio/adc/ad7173.c | 328 ++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 296 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index fb18acc83f39..45784e4bd27e 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -1,8 +1,9 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> - * AD717x family SPI ADC driver
> + * AD717x and AD411x family SPI ADC driver
> *
> * Supported devices:
> + * AD4111/AD4112/AD4114/AD4115/AD4116
> * AD7172-2/AD7172-4/AD7173-8/AD7175-2
> * AD7175-8/AD7176-2/AD7177-2
> *
> @@ -82,6 +83,11 @@
> #define AD7175_2_ID 0x0cd0
> #define AD7172_4_ID 0x2050
> #define AD7173_ID 0x30d0
> +#define AD4111_ID AD7173_ID
> +#define AD4112_ID AD7173_ID
> +#define AD4114_ID AD7173_ID
> +#define AD4116_ID 0x34d0
> +#define AD4115_ID 0x38d0
> #define AD7175_8_ID 0x3cd0
> #define AD7177_ID 0x4fd0
> #define AD7173_ID_MASK GENMASK(15, 4)
> @@ -112,6 +118,7 @@
>
> #define AD7173_GPO12_DATA(x) BIT((x) + 0)
> #define AD7173_GPO23_DATA(x) BIT((x) + 4)
> +#define AD4111_GPO01_DATA(x) BIT((x) + 6)
> #define AD7173_GPO_DATA(x) ((x) < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
>
> #define AD7173_INTERFACE_DATA_STAT BIT(6)
> @@ -130,26 +137,52 @@
> #define AD7173_VOLTAGE_INT_REF_uV 2500000
> #define AD7173_TEMP_SENSIIVITY_uV_per_C 477
> #define AD7177_ODR_START_VALUE 0x07
> +#define AD4111_SHUNT_RESISTOR_OHM 50
> +#define AD4111_DIVIDER_RATIO 10
> +#define AD4111_CURRENT_CHAN_CUTOFF 16
> +#define AD4111_VINCOM_INPUT 0X10

nit: odd to have upper-case X in 0x10

> +
> +/* pin < num_voltage_in is a normal voltage input */
> +/* pin >= num_voltage_in_div is a voltge input without a divider */

spelling: voltage

> +#define AD4111_IS_VINCOM_MISMATCH(pin1, pin2) ((pin1) == AD4111_VINCOM_INPUT && \
> + (pin2) < st->info->num_voltage_in && \
> + (pin2) >= st->info->num_voltage_in_div)
>
> #define AD7173_FILTER_ODR0_MASK GENMASK(5, 0)
> #define AD7173_MAX_CONFIGS 8
>
> +enum ad4111_current_channels {
> + AD4111_CURRENT_IN0P_IN0N,
> + AD4111_CURRENT_IN1P_IN1N,
> + AD4111_CURRENT_IN2P_IN2N,
> + AD4111_CURRENT_IN3P_IN3N,
> +};

Not sure this enum adds much since IN0 is 0, IN1 is 1, etc.
and it is only used as the index in the array initializer.

> +
> struct ad7173_device_info {
> const unsigned int *sinc5_data_rates;
> unsigned int num_sinc5_data_rates;
> unsigned int odr_start_value;
> + /*
> + * AD4116 has both inputs with a voltage divider and without.
> + * These inputs cannot be mixed in the channel configuration.
> + * Does not include the VINCOM input.
> + */
> + unsigned int num_voltage_in_div;
> unsigned int num_channels;
> unsigned int num_configs;
> - unsigned int num_inputs;
> + unsigned int num_voltage_in;
> unsigned int clock;
> unsigned int id;
> char *name;
> + bool has_current_inputs;
> + bool has_vincom_input;
> bool has_temp;
> /* ((AVDD1 − AVSS)/5) */
> bool has_common_input;
> bool has_input_buf;
> bool has_int_ref;
> bool has_ref2;
> + bool higher_gpio_bits;
> u8 num_gpios;
> };
>

...

> +static int ad4111_validate_current_ain(struct ad7173_state *st,
> + const unsigned int ain[AD7173_NO_AINS_PER_CHANNEL])
> +{
> + struct device *dev = &st->sd.spi->dev;
> +
> + if (!st->info->has_current_inputs)
> + return dev_err_probe(dev, -EINVAL,
> + "Model %s does not support current channels\n",
> + st->info->name);
> +
> + if (ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))

I guess OK for now, but could be nice to have num_current_inputs in chip_info.

> + return dev_err_probe(dev, -EINVAL,
> + "For current channels single-channel must be <[0-3]>\n");
> +
> + return 0;
> +}
> +
> static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> unsigned int ain0, unsigned int ain1)
> {
> @@ -944,16 +1149,31 @@ static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> bool special_input0, special_input1;
>
> special_input0 = AD7173_IS_REF_INPUT(ain0) ||
> - (AD7173_IS_COM_INPUT(ain0) && st->info->has_common_input);
> + (AD7173_IS_COM_INPUT(ain0) && st->info->has_common_input) ||
> + ain0 == AD4111_VINCOM_INPUT;
> special_input1 = AD7173_IS_REF_INPUT(ain1) ||
> - (AD7173_IS_COM_INPUT(ain1) && st->info->has_common_input);
> + (AD7173_IS_COM_INPUT(ain1) && st->info->has_common_input) ||
> + ain1 == AD4111_VINCOM_INPUT;

Do we also need to check has_vincom_input here? Otherwise out of range
check below might succeed for chips that don't have this pin.

>
> - if ((ain0 >= st->info->num_inputs && !special_input0) ||
> - (ain1 >= st->info->num_inputs && !special_input1))
> + if (st->info->has_vincom_input)
> + if (AD4111_IS_VINCOM_MISMATCH(ain0, ain1) ||
> + AD4111_IS_VINCOM_MISMATCH(ain1, ain0))
> + return dev_err_probe(dev, -EINVAL,
> + "VINCOM must be paired with inputs having divider.\n");
> +
> + if ((ain0 >= st->info->num_voltage_in && !special_input0) ||
> + (ain1 >= st->info->num_voltage_in && !special_input1))
> return dev_err_probe(dev, -EINVAL,
> "Input pin number out of range for pair (%d %d).\n",
> ain0, ain1);
>
> + if (!special_input0 && !special_input1 &&
> + ((ain0 >= st->info->num_voltage_in_div) !=
> + (ain1 >= st->info->num_voltage_in_div)))
> + return dev_err_probe(dev, -EINVAL,
> + "Both inputs must either have a voltage divider or not have: (%d %d).\n",
> + ain0, ain1);
> +
> return 0;
> }
>
> @@ -984,7 +1204,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> struct device *dev = indio_dev->dev.parent;
> struct iio_chan_spec *chan_arr, *chan;
> unsigned int ain[AD7173_NO_AINS_PER_CHANNEL], chan_index = 0;
> - int ref_sel, ret, num_channels;
> + int ref_sel, ret, is_current_chan, num_channels;
>
> num_channels = device_get_child_node_count(dev);
>
> @@ -1031,15 +1251,40 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
>
> device_for_each_child_node_scoped(dev, child) {
> chan = &chan_arr[chan_index];
> + *chan = ad7173_channel_template;
> chan_st_priv = &chans_st_arr[chan_index];
> ret = fwnode_property_read_u32_array(child, "diff-channels",
> ain, ARRAY_SIZE(ain));
> - if (ret)
> - return ret;
> + if (ret) {
> + ret = fwnode_property_read_u32_array(child, "single-channel",
> + ain, 1);

Should this just be fwnode_property_read_u32()?

> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Channel must define one of diff-channels or single-channel.\n");
>
> - ret = ad7173_validate_voltage_ain_inputs(st, ain[0], ain[1]);
> - if (ret)
> - return ret;
> + is_current_chan = fwnode_property_read_bool(child, "adi,current-channel");
> + } else {
> + chan->differential = true;
> + }
> +
> + if (is_current_chan) {
> + ret = ad4111_validate_current_ain(st, ain);
> + if (ret)
> + return ret;
> + is_current_chan = true;

is_current_chan is already set, so this is redundant.

> + } else {
> + if (!chan->differential) {
> + ret = fwnode_property_read_u32_array(child,
> + "common-mode-channel", ain + 1, 1);

And here fwnode_property_read_u32()?

> + if (ret)
> + return dev_err_probe(dev, ret,
> + "common-mode-channel must be defined for single-ended channels.\n");
> + }
> + ret = ad7173_validate_voltage_ain_inputs(st, ain[0], ain[1]);
> + if (ret)
> + return ret;
> + is_current_chan = false;

Same here, reduandant is_current_chan.

> + }
>
> ret = fwnode_property_match_property_string(child,
> "adi,reference-select",
> @@ -1058,14 +1303,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> st->adc_mode |= AD7173_ADC_MODE_REF_EN;
> chan_st_priv->cfg.ref_sel = ref_sel;
>
> - *chan = ad7173_channel_template;
> chan->address = chan_index;
> chan->scan_index = chan_index;
> chan->channel = ain[0];
> - chan->channel2 = ain[1];
> - chan->differential = true;
> -
> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> chan_st_priv->chan_reg = chan_index;
> chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> chan_st_priv->cfg.odr = 0;
> @@ -1074,6 +1314,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
> if (chan_st_priv->cfg.bipolar)
> chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
>
> + if (is_current_chan) {
> + chan->type = IIO_CURRENT;
> + chan->differential = false;
> + chan->channel2 = 0;
> + ain[1] = FIELD_GET(AD7173_CH_SETUP_AINNEG_MASK,
> + ad4111_current_channel_config[ain[0]]);
> + ain[0] = FIELD_GET(AD7173_CH_SETUP_AINPOS_MASK,
> + ad4111_current_channel_config[ain[0]]);

Changing the meaning of ain here makes the code harder to understand.

How about just:

chan_st_priv->ain = ad4111_current_channel_config[ain[0]];

here and mode the chan_st_priv->ain = below inside the else?

> + } else {
> + chan_st_priv->cfg.input_buf = st->info->has_input_buf;
> + chan->channel2 = ain[1];
> + }
> + chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
> +
> chan_index++;
> }
> return 0;
> @@ -1200,6 +1454,11 @@ static int ad7173_probe(struct spi_device *spi)
> }
>
> static const struct of_device_id ad7173_of_match[] = {
> + { .compatible = "ad4111", .data = &ad4111_device_info},
> + { .compatible = "ad4112", .data = &ad4112_device_info},
> + { .compatible = "ad4114", .data = &ad4114_device_info},
> + { .compatible = "ad4115", .data = &ad4115_device_info},
> + { .compatible = "ad4116", .data = &ad4116_device_info},
> { .compatible = "adi,ad7172-2", .data = &ad7172_2_device_info},
> { .compatible = "adi,ad7172-4", .data = &ad7172_4_device_info},
> { .compatible = "adi,ad7173-8", .data = &ad7173_8_device_info},
> @@ -1212,6 +1471,11 @@ static const struct of_device_id ad7173_of_match[] = {
> MODULE_DEVICE_TABLE(of, ad7173_of_match);
>
> static const struct spi_device_id ad7173_id_table[] = {
> + { "ad4111", (kernel_ulong_t)&ad4111_device_info},
> + { "ad4112", (kernel_ulong_t)&ad4112_device_info},
> + { "ad4114", (kernel_ulong_t)&ad4114_device_info},
> + { "ad4115", (kernel_ulong_t)&ad4115_device_info},
> + { "ad4116", (kernel_ulong_t)&ad4116_device_info},
> { "ad7172-2", (kernel_ulong_t)&ad7172_2_device_info},
> { "ad7172-4", (kernel_ulong_t)&ad7172_4_device_info},
> { "ad7173-8", (kernel_ulong_t)&ad7173_8_device_info},

nit: Same as in the other patch, space before }

> @@ -1236,5 +1500,5 @@ module_spi_driver(ad7173_driver);
> MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
> MODULE_AUTHOR("Lars-Peter Clausen <[email protected]>");
> MODULE_AUTHOR("Dumitru Ceclan <[email protected]>");
> -MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
> +MODULE_DESCRIPTION("Analog Devices AD7173 and similar ADC driver");
> MODULE_LICENSE("GPL");
>