| Message ID | 20240401-ad4111-v1-6-34618a9cc502@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for AD411x | expand |
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > 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 <dumitru.ceclan@analog.com> > --- > drivers/iio/adc/ad7173.c | 224 ++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 210 insertions(+), 14 deletions(-) > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c > index 9526585e6929..ac32bd7dbd1e 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 > * > @@ -72,6 +73,11 @@ > #define AD7175_2_ID 0x0cd0 > #define AD7172_4_ID 0x2050 > #define AD7173_ID 0x30d0 > +#define AD4111_ID 0x30d0 > +#define AD4112_ID 0x30d0 > +#define AD4114_ID 0x30d0 It might make it a bit more obvious that not all chips have a unique ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather than introducing multiple macros with the same value. Or leave it as AD7173_ID to keep it short and add a comment where it is used with 411x chips in ad7173_device_info[]. > +#define AD4116_ID 0x34d0 > +#define AD4115_ID 0x38d0 > #define AD7175_8_ID 0x3cd0 > #define AD7177_ID 0x4fd0 > #define AD7173_ID_MASK GENMASK(15, 4) > @@ -120,11 +126,20 @@ > #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 AD411X_VCOM_INPUT 0X10 > +#define AD4111_CURRENT_CHAN_CUTOFF 16 > > #define AD7173_FILTER_ODR0_MASK GENMASK(5, 0) > #define AD7173_MAX_CONFIGS 8 > > enum ad7173_ids { > + ID_AD4111, > + ID_AD4112, > + ID_AD4114, > + ID_AD4115, > + ID_AD4116, > ID_AD7172_2, > ID_AD7172_4, > ID_AD7173_8, > @@ -134,16 +149,26 @@ enum ad7173_ids { > ID_AD7177_2, > }; > > +enum ad4111_current_channels { > + AD4111_CURRENT_IN0P_IN0N, > + AD4111_CURRENT_IN1P_IN1N, > + AD4111_CURRENT_IN2P_IN2N, > + AD4111_CURRENT_IN3P_IN3N, > +}; > + > struct ad7173_device_info { > const unsigned int *sinc5_data_rates; > unsigned int num_sinc5_data_rates; > unsigned int odr_start_value; > + unsigned int num_inputs_with_divider; > unsigned int num_channels; > unsigned int num_configs; > unsigned int num_inputs; Probably a good idea to change num_inputs to num_voltage_inputs so it isn't confused with the total number of inputs. Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider. Also could use a comment to make it clear if num_voltage_inputs includes num_voltage_inputs_with_divider or not. And that it doesn't include VINCOM. Probably also need some flag here to differentiate ADCINxx voltage inputs on AD4116. > unsigned int clock; > unsigned int id; > char *name; > + bool has_current_inputs; Maybe more future-proof to have num_current_inputs instead of bool? > + bool has_vcom; For consistency: has_vcom_input > bool has_temp; > bool has_input_buf; > bool has_int_ref; > @@ -189,6 +214,24 @@ struct ad7173_state { > #endif > }; > > +static unsigned int ad4115_sinc5_data_rates[] = { > + 24845000, 24845000, 20725000, 20725000, /* 0-3 */ > + 15564000, 13841000, 10390000, 10390000, /* 4-7 */ > + 4994000, 2499000, 1000000, 500000, /* 8-11 */ > + 395500, 200000, 100000, 59890, /* 12-15 */ > + 49920, 20000, 16660, 10000, /* 16-19 */ > + 5000, 2500, 2500, /* 20-22 */ > +}; > + > +static unsigned int ad4116_sinc5_data_rates[] = { > + 12422360, 12422360, 12422360, 12422360, /* 0-3 */ > + 10362690, 10362690, 7782100, 6290530, /* 4-7 */ > + 5194800, 2496900, 1007600, 499900, /* 8-11 */ > + 390600, 200300, 100000, 59750, /* 12-15 */ > + 49840, 20000, 16650, 10000, /* 16-19 */ > + 5000, 2500, 1250, /* 20-22 */ > +}; > + > static const unsigned int ad7173_sinc5_data_rates[] = { > 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */ > 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */ > @@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = { > 5000, /* 20 */ > }; > > +static unsigned int ad4111_current_channel_config[] = { > + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8, > + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9, > + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA, > + [AD4111_CURRENT_IN3P_IN3N] = 0x18B, > +}; As mentioned in the DT bindings review, it would make more sense to just use the datasheet numbers for the current input channels in the diff-channels DT property, then we don't need this lookup table. > + > static const struct ad7173_device_info ad7173_device_info[] = { > + [ID_AD4111] = { > + .id = AD4111_ID, > + .num_inputs_with_divider = 8, > + .num_channels = 16, > + .num_configs = 8, > + .num_inputs = 8, > + .num_gpios = 2, > + .has_temp = true, > + .has_vcom = true, > + .has_input_buf = true, > + .has_current_inputs = true, > + .has_int_ref = true, > + .clock = 2 * HZ_PER_MHZ, > + .sinc5_data_rates = ad7173_sinc5_data_rates, > + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), > + }, > + [ID_AD4112] = { > + .id = AD4112_ID, > + .num_inputs_with_divider = 8, > + .num_channels = 16, > + .num_configs = 8, > + .num_inputs = 8, > + .num_gpios = 2, > + .has_vcom = true, > + .has_temp = true, > + .has_input_buf = true, > + .has_current_inputs = true, > + .has_int_ref = true, > + .clock = 2 * HZ_PER_MHZ, > + .sinc5_data_rates = ad7173_sinc5_data_rates, > + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), > + }, > + [ID_AD4114] = { > + .id = AD4114_ID, > + .num_inputs_with_divider = 16, > + .num_channels = 16, > + .num_configs = 8, > + .num_inputs = 16, > + .num_gpios = 4, > + .has_vcom = true, > + .has_temp = true, > + .has_input_buf = true, > + .has_int_ref = true, > + .clock = 2 * HZ_PER_MHZ, > + .sinc5_data_rates = ad7173_sinc5_data_rates, > + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), > + }, > + [ID_AD4115] = { > + .id = AD4115_ID, > + .num_inputs_with_divider = 16, > + .num_channels = 16, > + .num_configs = 8, > + .num_inputs = 16, > + .num_gpios = 4, > + .has_vcom = true, > + .has_temp = true, > + .has_input_buf = true, > + .has_int_ref = true, > + .clock = 8 * HZ_PER_MHZ, > + .sinc5_data_rates = ad4115_sinc5_data_rates, > + .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates), > + }, > + [ID_AD4116] = { > + .id = AD4116_ID, > + .num_inputs_with_divider = 11, > + .num_channels = 16, > + .num_configs = 8, > + .num_inputs = 16, > + .num_gpios = 4, > + .has_vcom = true, > + .has_temp = true, > + .has_input_buf = true, > + .has_int_ref = true, > + .clock = 4 * HZ_PER_MHZ, > + .sinc5_data_rates = ad4116_sinc5_data_rates, > + .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates), > + }, > [ID_AD7172_2] = { > .name = "ad7172-2", > .id = AD7172_2_ID, > @@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev, > > return IIO_VAL_INT; > case IIO_CHAN_INFO_SCALE: > - if (chan->type == IIO_TEMP) { > + > + switch (chan->type) { > + case IIO_TEMP: > temp = AD7173_VOLTAGE_INT_REF_uV * MILLI; > temp /= AD7173_TEMP_SENSIIVITY_uV_per_C; > *val = temp; > *val2 = chan->scan_type.realbits; > - } else { > + break; can we just return here instead of break? > + case IIO_VOLTAGE: > *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > + > + if (chan->channel < st->info->num_inputs_with_divider) > + *val *= AD4111_DIVIDER_RATIO; > + break; same: return here > + case IIO_CURRENT: > + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > + *val /= AD4111_SHUNT_RESISTOR_OHM; > + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); Static analysis tools like to complain about using bool as int. Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway. > + break; return here > + default: > + return -EINVAL; > } > return IIO_VAL_FRACTIONAL_LOG2; > case IIO_CHAN_INFO_OFFSET: > - if (chan->type == IIO_TEMP) { > + > + switch (chan->type) { > + case IIO_TEMP: > /* 0 Kelvin -> raw sample */ > temp = -ABSOLUTE_ZERO_MILLICELSIUS; > temp *= AD7173_TEMP_SENSIIVITY_uV_per_C; > @@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev, > AD7173_VOLTAGE_INT_REF_uV * > MILLI); > *val = -temp; > - } else { > + break; return ... > + case IIO_VOLTAGE: > + case IIO_CURRENT: > *val = -BIT(chan->scan_type.realbits - 1); Expecting a special case here, at least when ADCIN15 is configured for pseudo-differential inputs. > + break; return ... > + default: > + return -EINVAL; > } > return IIO_VAL_INT; > case IIO_CHAN_INFO_SAMP_FREQ: > @@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev) > &st->int_clk_hw); > } > > +static int ad4111_validate_current_ain(struct ad7173_state *st, > + unsigned int ain[2]) > +{ > + struct device *dev = &st->sd.spi->dev; > + > + if (!st->info->has_current_inputs) > + return dev_err_probe(dev, -EINVAL, > + "Reg values equal to or higher than %d are restricted to models with current channels.\n", > + AD4111_CURRENT_CHAN_CUTOFF); > + > + if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config)) > + return dev_err_probe(dev, -EINVAL, > + "For current channel diff-channels must be <[0-%d],0>\n", > + ARRAY_SIZE(ad4111_current_channel_config) - 1); > + > + return 0; > +} > + > static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, > unsigned int ain[2]) > { > @@ -951,7 +1117,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[2], chan_index = 0; > - int ref_sel, ret, num_channels; > + int ref_sel, ret, reg, num_channels; > > num_channels = device_get_child_node_count(dev); > > @@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) > if (ret) > return ret; > > - ret = ad7173_validate_voltage_ain_inputs(st, ain); > + ret = fwnode_property_read_u32(child, "reg", ®); > if (ret) > return ret; > > + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { As mentioned in the DT bindings review, using reg to determine if an a channel is a current input or voltage input seems wrong/fragile. We should be able to use the has_current_inputs flag and the input numbers instead to determine the type of input. > + ret = ad4111_validate_current_ain(st, ain); > + if (ret) > + return ret; > + } else { > + ret = ad7173_validate_voltage_ain_inputs(st, ain); > + if (ret) > + return ret; > + } > + > ret = fwnode_property_match_property_string(child, > "adi,reference-select", > ad7173_ref_sel_str, > @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) > *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]); > + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { > + chan->type = IIO_CURRENT; > + chan->channel = ain[0]; > + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; > + } else { > + chan->channel = ain[0]; > + chan->channel2 = ain[1]; > + chan->differential = true; Expecting chan->differential = false when ADCIN15 is configured for pseudo-differential inputs. Also, perhaps missed in previous reviews, I would expect chan->differential = false when channels are used as single-ended. > + > + chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]); > + chan_st_priv->cfg.input_buf = st->info->has_input_buf; > + } > + > chan_st_priv->chan_reg = chan_index; > - chan_st_priv->cfg.input_buf = st->info->has_input_buf; > chan_st_priv->cfg.odr = 0; > - > chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar"); > if (chan_st_priv->cfg.bipolar) > chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET); > @@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi) > } > > static const struct of_device_id ad7173_of_match[] = { > + { .compatible = "ad4111", > + .data = &ad7173_device_info[ID_AD4111]}, > + { .compatible = "ad4112", > + .data = &ad7173_device_info[ID_AD4112]}, > + { .compatible = "ad4114", > + .data = &ad7173_device_info[ID_AD4114]}, > + { .compatible = "ad4115", > + .data = &ad7173_device_info[ID_AD4115]}, > { .compatible = "adi,ad7172-2", > .data = &ad7173_device_info[ID_AD7172_2]}, > { .compatible = "adi,ad7172-4", > @@ -1186,6 +1377,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)&ad7173_device_info[ID_AD4111]}, > + { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]}, > + { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]}, > + { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]}, > + { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]}, > { "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]}, > { "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]}, > { "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]}, > @@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver); > MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA); > MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>"); > MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>"); > -MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver"); > +MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver"); > MODULE_LICENSE("GPL"); > > -- > 2.43.0 > >
On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > 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 <dumitru.ceclan@analog.com> > --- ... > @@ -951,7 +1117,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[2], chan_index = 0; > - int ref_sel, ret, num_channels; > + int ref_sel, ret, reg, num_channels; > > num_channels = device_get_child_node_count(dev); > Another thing that is missing in this function both for the chips being added here and the existing chips are channels for _all_ possible inputs. The driver is adding a fixed input channel for the temperature sensor, as it should. But all of the chips also have a similar input channel configuration that measures the reference voltage. Currently, there doesn't seem to be a way to make use of this feature. I would expect a hard-coded voltage input channel that is always added for this reference voltage similar to the temperature channel. The ad717x chips (except AD7173-8 and AD7176-2) also have a common mode voltage input ("((AVDD1 − AVSS)/5)") that could work the same. In the case of the ad717x chips though, it looks like these channels are not "fixed" like they are in ad411x. It looks like these inputs can be mixed and matched with AINx inputs and/or each other as differential pairs. So if that is actually the case, I would expect the DT bindings for ad717x to look like adi,ad4130.yaml where these additional input sources are listed in the diff-channels property instead of having hard-coded channels in the driver like I have suggested above. But, as always, fixes for ad717x should be in a separate patch series. Still, I think adding a hard-coded channel for the reference voltage input for ad411x chips in this patch makes sense.
On Mon, Apr 1, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote: > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > > > > From: Dumitru Ceclan <dumitru.ceclan@analog.com> > > > > Add support for AD4111/AD4112/AD4114/AD4115/AD4116. > > ... > > @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) > > *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]); > > + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { > > + chan->type = IIO_CURRENT; > > + chan->channel = ain[0]; > > + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; > > + } else { > > + chan->channel = ain[0]; > > + chan->channel2 = ain[1]; > > + chan->differential = true; > > Expecting chan->differential = false when ADCIN15 is configured for > pseudo-differential inputs. > > Also, perhaps missed in previous reviews, I would expect > chan->differential = false when channels are used as single-ended. > After sleeping on it, I came to the concision that these parts are probably too complex to try to worry about differential vs. pseudo-differential/single-ended (what the datasheet calls single-ended is really pseudo-differential). So I take back my comments about expecting differential = false in those cases.
On 02/04/2024 00:53, David Lechner wrote: > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >> >> 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 <dumitru.ceclan@analog.com> >> --- > > ... > >> @@ -951,7 +1117,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[2], chan_index = 0; >> - int ref_sel, ret, num_channels; >> + int ref_sel, ret, reg, num_channels; >> >> num_channels = device_get_child_node_count(dev); >> > > Another thing that is missing in this function both for the chips > being added here and the existing chips are channels for _all_ > possible inputs. The driver is adding a fixed input channel for the > temperature sensor, as it should. But all of the chips also have a > similar input channel configuration that measures the reference > voltage. Currently, there doesn't seem to be a way to make use of this > feature. I would expect a hard-coded voltage input channel that is > always added for this reference voltage similar to the temperature > channel. > AD7173-8: Channel input configs: AINPOS0: REF+ 10101: 21 AINNEG0: REF- 10110: 22 For the user to define the REF measurement channel: diff-channels = <21 22>; So it is possible from the binding side. It would just need support from the driver as currently any value above the stated number of inputs is rejected. Maybe document this in a comment like you suggested below. I really do not agree with using up channels without letting the user decide. I can accept to dedicate one for the temp where applicable but more than that and it feels like we are restricting the usage too much. > The ad717x chips (except AD7173-8 and AD7176-2) also have a common > mode voltage input ("((AVDD1 − AVSS)/5)") that could work the same. > Again, would be resolved if I added support from the driver. > In the case of the ad717x chips though, it looks like these channels > are not "fixed" like they are in ad411x. It looks like these inputs > can be mixed and matched with AINx inputs and/or each other as > differential pairs. So if that is actually the case, I would expect > the DT bindings for ad717x to look like adi,ad4130.yaml where these > additional input sources are listed in the diff-channels property > instead of having hard-coded channels in the driver like I have > suggested above. > Yep, agree. > But, as always, fixes for ad717x should be in a separate patch series. > Still, I think adding a hard-coded channel for the reference voltage > input for ad411x chips in this patch makes sense. As stated above, not comfortable with using up channels with hard-coded values.
On 01/04/2024 22:45, David Lechner wrote: > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: >> >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> >> ... >> #define AD7175_2_ID 0x0cd0 >> #define AD7172_4_ID 0x2050 >> #define AD7173_ID 0x30d0 >> +#define AD4111_ID 0x30d0 >> +#define AD4112_ID 0x30d0 >> +#define AD4114_ID 0x30d0 > > It might make it a bit more obvious that not all chips have a unique > ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather > than introducing multiple macros with the same value. > > Or leave it as AD7173_ID to keep it short and add a comment where it > is used with 411x chips in ad7173_device_info[]. > Sure >> +#define AD4116_ID 0x34d0 >> +#define AD4115_ID 0x38d0 >> #define AD7175_8_ID 0x3cd0 >> #define AD7177_ID 0x4fd0 >> #define AD7173_ID_MASK GENMASK(15, 4) ... >> struct ad7173_device_info { >> const unsigned int *sinc5_data_rates; >> unsigned int num_sinc5_data_rates; >> unsigned int odr_start_value; >> + unsigned int num_inputs_with_divider; >> unsigned int num_channels; >> unsigned int num_configs; >> unsigned int num_inputs; > > Probably a good idea to change num_inputs to num_voltage_inputs so it > isn't confused with the total number of inputs. > > Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider. > > Also could use a comment to make it clear if num_voltage_inputs > includes num_voltage_inputs_with_divider or not. And that it doesn't > include VINCOM. > Alright for these 3 statements above. > Probably also need some flag here to differentiate ADCINxx voltage > inputs on AD4116. > That is the purpose of num_inputs_with_divider. Mangled some changes when splitting into individual patches. Will include in V2. " if (ain[1] == AD411X_VCOM_INPUT && ain[0] >= st->info->num_inputs_with_divider) return dev_err_probe(dev, -EINVAL, "VCOM must be paired with inputs having divider.\n"); " ... >> >> +static unsigned int ad4111_current_channel_config[] = { >> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8, >> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9, >> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA, >> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B, >> +}; > > As mentioned in the DT bindings review, it would make more sense to > just use the datasheet numbers for the current input channels in the > diff-channels DT property, then we don't need this lookup table. > Yet, the datasheet does not specify the numbers, just a single bitfield for each pair. It is too much of a churn to need to decode that bitfield into individual values when the user just wants to select a single pair. ... >> + case IIO_CURRENT: >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); >> + *val /= AD4111_SHUNT_RESISTOR_OHM; >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > > Static analysis tools like to complain about using bool as int. > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway. > Maybe it does not apply here, but i followed this advice: Andy Shevchenko V1 of AD7173 (named initially ad717x) " > + return (bool)(value & mask); This is weird. You have int which you get from bool, wouldn't be better to use !!(...) as other GPIO drivers do? " >> + case IIO_CURRENT: >> *val = -BIT(chan->scan_type.realbits - 1); > > Expecting a special case here, at least when ADCIN15 is configured for > pseudo-differential inputs. > And what configuration would that be? The only configurable part is the BI_UNIPOLARx bit in the channel register, which is addressed here. There seems to be a confusion similar to what we had with single-ended channels. The ADC is differential. Pseudo-differential in this datasheet just means that you wired a fixed voltage(higher than 0) to the negative analog input. Which you can also do on the other inputs with a divider. ... >> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]); >> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { >> + chan->type = IIO_CURRENT; >> + chan->channel = ain[0]; >> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; >> + } else { >> + chan->channel = ain[0]; >> + chan->channel2 = ain[1]; >> + chan->differential = true; > > Expecting chan->differential = false when ADCIN15 is configured for > pseudo-differential inputs. > > Also, perhaps missed in previous reviews, I would expect > chan->differential = false when channels are used as single-ended. > Why? Also, how would one detect if you are using single-ended channels? The ADC is still differential. Single ended is represented as connecting AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate. In the IIO framework the only difference this makes is in the naming of the channel: voltage0-voltage1 vs just voltage0 All channels are differential. Pseudo differential: still differential.
On 02/04/2024 17:00, David Lechner wrote: > On Mon, Apr 1, 2024 at 2:45 PM David Lechner <dlechner@baylibre.com> wrote: >> >> On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay >> <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: ... >>> *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]); >>> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { >>> + chan->type = IIO_CURRENT; >>> + chan->channel = ain[0]; >>> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; >>> + } else { >>> + chan->channel = ain[0]; >>> + chan->channel2 = ain[1]; >>> + chan->differential = true; >> >> Expecting chan->differential = false when ADCIN15 is configured for >> pseudo-differential inputs. >> >> Also, perhaps missed in previous reviews, I would expect >> chan->differential = false when channels are used as single-ended. >> > > After sleeping on it, I came to the concision that these parts are > probably too complex to try to worry about differential vs. > pseudo-differential/single-ended (what the datasheet calls > single-ended is really pseudo-differential). > > So I take back my comments about expecting differential = false in those cases. Alrighty then
On Wed, Apr 3, 2024 at 4:53 AM Ceclan, Dumitru <mitrutzceclan@gmail.com> wrote: > > > On 01/04/2024 22:45, David Lechner wrote: > > On Mon, Apr 1, 2024 at 10:10 AM Dumitru Ceclan via B4 Relay > > <devnull+dumitru.ceclan.analog.com@kernel.org> wrote: > >> > >> From: Dumitru Ceclan <dumitru.ceclan@analog.com> > >> > > ... > > >> #define AD7175_2_ID 0x0cd0 > >> #define AD7172_4_ID 0x2050 > >> #define AD7173_ID 0x30d0 > >> +#define AD4111_ID 0x30d0 > >> +#define AD4112_ID 0x30d0 > >> +#define AD4114_ID 0x30d0 > > > > It might make it a bit more obvious that not all chips have a unique > > ID if we rename AD7173_ID to AD7173_AD4111_AD4112_AD4114_ID rather > > than introducing multiple macros with the same value. > > > > Or leave it as AD7173_ID to keep it short and add a comment where it > > is used with 411x chips in ad7173_device_info[]. > > > > Sure > > >> +#define AD4116_ID 0x34d0 > >> +#define AD4115_ID 0x38d0 > >> #define AD7175_8_ID 0x3cd0 > >> #define AD7177_ID 0x4fd0 > >> #define AD7173_ID_MASK GENMASK(15, 4) > > ... > > >> struct ad7173_device_info { > >> const unsigned int *sinc5_data_rates; > >> unsigned int num_sinc5_data_rates; > >> unsigned int odr_start_value; > >> + unsigned int num_inputs_with_divider; > >> unsigned int num_channels; > >> unsigned int num_configs; > >> unsigned int num_inputs; > > > > Probably a good idea to change num_inputs to num_voltage_inputs so it > > isn't confused with the total number of inputs. > > > > Similarly num_voltage_inputs_with_divider instead of num_inputs_with_divider. > > > > Also could use a comment to make it clear if num_voltage_inputs > > includes num_voltage_inputs_with_divider or not. And that it doesn't > > include VINCOM. > > > > Alright for these 3 statements above. > > > Probably also need some flag here to differentiate ADCINxx voltage > > inputs on AD4116. > > > > That is the purpose of num_inputs_with_divider. Mangled some changes > when splitting into individual patches. Will include in V2. > " > if (ain[1] == AD411X_VCOM_INPUT && > ain[0] >= st->info->num_inputs_with_divider) > return dev_err_probe(dev, -EINVAL, > "VCOM must be paired with inputs having divider.\n"); > " > > ... > > >> > >> +static unsigned int ad4111_current_channel_config[] = { > >> + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8, > >> + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9, > >> + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA, > >> + [AD4111_CURRENT_IN3P_IN3N] = 0x18B, > >> +}; > > > > As mentioned in the DT bindings review, it would make more sense to > > just use the datasheet numbers for the current input channels in the > > diff-channels DT property, then we don't need this lookup table. > > > Yet, the datasheet does not specify the numbers, just a single bitfield > for each pair. It is too much of a churn to need to decode that bitfield > into individual values when the user just wants to select a single pair. > > ... > > >> + case IIO_CURRENT: > >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > >> + *val /= AD4111_SHUNT_RESISTOR_OHM; > >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > > > > Static analysis tools like to complain about using bool as int. > > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway. > > > Maybe it does not apply here, but i followed this advice: > > Andy Shevchenko V1 of AD7173 (named initially ad717x) > " > > + return (bool)(value & mask); > > This is weird. You have int which you get from bool, wouldn't be better > to use > !!(...) as other GPIO drivers do? As long as the build bots don't complain, there isn't a reason to change it. It is just a matter of personal preference at that point. I got a sparse warning for something like this recently [1], but maybe that case was just because it was inside of a FIELD_PREP() using it as bit logic instead of addition and we won't get any warnings here. [1]: https://lore.kernel.org/linux-iio/20240129195611.701611-3-dlechner@baylibre.com/ > > " > > > >> + case IIO_CURRENT: > >> *val = -BIT(chan->scan_type.realbits - 1); > > > > Expecting a special case here, at least when ADCIN15 is configured for > > pseudo-differential inputs. > > > > And what configuration would that be? > The only configurable part is the BI_UNIPOLARx bit in the channel > register, which is addressed here. > > There seems to be a confusion similar to what we had with single-ended > channels. The ADC is differential. Pseudo-differential in this datasheet > just means that you wired a fixed voltage(higher than 0) to the negative > analog input. > > Which you can also do on the other inputs with a divider. > As discussed elsewhere, you can disregard this suggestion. > ... > > >> - chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]); > >> + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { > >> + chan->type = IIO_CURRENT; > >> + chan->channel = ain[0]; > >> + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; > >> + } else { > >> + chan->channel = ain[0]; > >> + chan->channel2 = ain[1]; > >> + chan->differential = true; > > > > Expecting chan->differential = false when ADCIN15 is configured for > > pseudo-differential inputs. > > > > Also, perhaps missed in previous reviews, I would expect > > chan->differential = false when channels are used as single-ended. > > > Why? > Also, how would one detect if you are using single-ended channels? > > The ADC is still differential. Single ended is represented as connecting > AVSS(or another fixed voltage) and only letting the AIN+ input to fluctuate. > > In the IIO framework the only difference this makes is in the naming of > the channel: > voltage0-voltage1 vs just voltage0 > > All channels are differential. Pseudo differential: still differential. In the discussions on the AD7380 patch series, we came to the conclusion that pseduo-differential is technically not differential in the context of the .differential flag in IIO. But as mentioned in my follow up, for this driver it is going to make things far simpler if we just ignore that and treat pseudo-differential the same as fully differential.
> > > > >> + case IIO_CURRENT: > > >> + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); > > >> + *val /= AD4111_SHUNT_RESISTOR_OHM; > > >> + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); > > > > > > Static analysis tools like to complain about using bool as int. > > > Probably more clear to write it as (ch->cfg.bipolar ? 1 : 0) anyway. > > > > > Maybe it does not apply here, but i followed this advice: > > > > Andy Shevchenko V1 of AD7173 (named initially ad717x) > > " > > > + return (bool)(value & mask); > > > > This is weird. You have int which you get from bool, wouldn't be better > > to use > > !!(...) as other GPIO drivers do? > > As long as the build bots don't complain, there isn't a reason to > change it. It is just a matter of personal preference at that point. > > I got a sparse warning for something like this recently [1], but maybe > that case was just because it was inside of a FIELD_PREP() using it as > bit logic instead of addition and we won't get any warnings here. > > [1]: https://lore.kernel.org/linux-iio/20240129195611.701611-3-dlechner@baylibre.com/ It was common to use !! for a number of years, but then it got a comment from Linus Torvalds making reasonable point that it isn't easy to read, so slight preference these days is for a ternary.
On 01/04/2024 22:45, David Lechner wrote: >> unsigned int clock; >> unsigned int id; >> char *name; >> + bool has_current_inputs; > Maybe more future-proof to have num_current_inputs instead of bool? At first I agreed with this, but the way that the current inputs are mapped to reg values does not really offer a way to extend them without changing completely the numbering scheme. If that happens, changing this field will be the least bit to need changing.
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c index 9526585e6929..ac32bd7dbd1e 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 * @@ -72,6 +73,11 @@ #define AD7175_2_ID 0x0cd0 #define AD7172_4_ID 0x2050 #define AD7173_ID 0x30d0 +#define AD4111_ID 0x30d0 +#define AD4112_ID 0x30d0 +#define AD4114_ID 0x30d0 +#define AD4116_ID 0x34d0 +#define AD4115_ID 0x38d0 #define AD7175_8_ID 0x3cd0 #define AD7177_ID 0x4fd0 #define AD7173_ID_MASK GENMASK(15, 4) @@ -120,11 +126,20 @@ #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 AD411X_VCOM_INPUT 0X10 +#define AD4111_CURRENT_CHAN_CUTOFF 16 #define AD7173_FILTER_ODR0_MASK GENMASK(5, 0) #define AD7173_MAX_CONFIGS 8 enum ad7173_ids { + ID_AD4111, + ID_AD4112, + ID_AD4114, + ID_AD4115, + ID_AD4116, ID_AD7172_2, ID_AD7172_4, ID_AD7173_8, @@ -134,16 +149,26 @@ enum ad7173_ids { ID_AD7177_2, }; +enum ad4111_current_channels { + AD4111_CURRENT_IN0P_IN0N, + AD4111_CURRENT_IN1P_IN1N, + AD4111_CURRENT_IN2P_IN2N, + AD4111_CURRENT_IN3P_IN3N, +}; + struct ad7173_device_info { const unsigned int *sinc5_data_rates; unsigned int num_sinc5_data_rates; unsigned int odr_start_value; + unsigned int num_inputs_with_divider; unsigned int num_channels; unsigned int num_configs; unsigned int num_inputs; unsigned int clock; unsigned int id; char *name; + bool has_current_inputs; + bool has_vcom; bool has_temp; bool has_input_buf; bool has_int_ref; @@ -189,6 +214,24 @@ struct ad7173_state { #endif }; +static unsigned int ad4115_sinc5_data_rates[] = { + 24845000, 24845000, 20725000, 20725000, /* 0-3 */ + 15564000, 13841000, 10390000, 10390000, /* 4-7 */ + 4994000, 2499000, 1000000, 500000, /* 8-11 */ + 395500, 200000, 100000, 59890, /* 12-15 */ + 49920, 20000, 16660, 10000, /* 16-19 */ + 5000, 2500, 2500, /* 20-22 */ +}; + +static unsigned int ad4116_sinc5_data_rates[] = { + 12422360, 12422360, 12422360, 12422360, /* 0-3 */ + 10362690, 10362690, 7782100, 6290530, /* 4-7 */ + 5194800, 2496900, 1007600, 499900, /* 8-11 */ + 390600, 200300, 100000, 59750, /* 12-15 */ + 49840, 20000, 16650, 10000, /* 16-19 */ + 5000, 2500, 1250, /* 20-22 */ +}; + static const unsigned int ad7173_sinc5_data_rates[] = { 6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000, /* 0-7 */ 3115000, 2597000, 1007000, 503800, 381000, 200300, 100500, 59520, /* 8-15 */ @@ -204,7 +247,91 @@ static const unsigned int ad7175_sinc5_data_rates[] = { 5000, /* 20 */ }; +static unsigned int ad4111_current_channel_config[] = { + [AD4111_CURRENT_IN0P_IN0N] = 0x1E8, + [AD4111_CURRENT_IN1P_IN1N] = 0x1C9, + [AD4111_CURRENT_IN2P_IN2N] = 0x1AA, + [AD4111_CURRENT_IN3P_IN3N] = 0x18B, +}; + static const struct ad7173_device_info ad7173_device_info[] = { + [ID_AD4111] = { + .id = AD4111_ID, + .num_inputs_with_divider = 8, + .num_channels = 16, + .num_configs = 8, + .num_inputs = 8, + .num_gpios = 2, + .has_temp = true, + .has_vcom = true, + .has_input_buf = true, + .has_current_inputs = true, + .has_int_ref = true, + .clock = 2 * HZ_PER_MHZ, + .sinc5_data_rates = ad7173_sinc5_data_rates, + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), + }, + [ID_AD4112] = { + .id = AD4112_ID, + .num_inputs_with_divider = 8, + .num_channels = 16, + .num_configs = 8, + .num_inputs = 8, + .num_gpios = 2, + .has_vcom = true, + .has_temp = true, + .has_input_buf = true, + .has_current_inputs = true, + .has_int_ref = true, + .clock = 2 * HZ_PER_MHZ, + .sinc5_data_rates = ad7173_sinc5_data_rates, + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), + }, + [ID_AD4114] = { + .id = AD4114_ID, + .num_inputs_with_divider = 16, + .num_channels = 16, + .num_configs = 8, + .num_inputs = 16, + .num_gpios = 4, + .has_vcom = true, + .has_temp = true, + .has_input_buf = true, + .has_int_ref = true, + .clock = 2 * HZ_PER_MHZ, + .sinc5_data_rates = ad7173_sinc5_data_rates, + .num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates), + }, + [ID_AD4115] = { + .id = AD4115_ID, + .num_inputs_with_divider = 16, + .num_channels = 16, + .num_configs = 8, + .num_inputs = 16, + .num_gpios = 4, + .has_vcom = true, + .has_temp = true, + .has_input_buf = true, + .has_int_ref = true, + .clock = 8 * HZ_PER_MHZ, + .sinc5_data_rates = ad4115_sinc5_data_rates, + .num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates), + }, + [ID_AD4116] = { + .id = AD4116_ID, + .num_inputs_with_divider = 11, + .num_channels = 16, + .num_configs = 8, + .num_inputs = 16, + .num_gpios = 4, + .has_vcom = true, + .has_temp = true, + .has_input_buf = true, + .has_int_ref = true, + .clock = 4 * HZ_PER_MHZ, + .sinc5_data_rates = ad4116_sinc5_data_rates, + .num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates), + }, [ID_AD7172_2] = { .name = "ad7172-2", .id = AD7172_2_ID, @@ -670,18 +797,34 @@ static int ad7173_read_raw(struct iio_dev *indio_dev, return IIO_VAL_INT; case IIO_CHAN_INFO_SCALE: - if (chan->type == IIO_TEMP) { + + switch (chan->type) { + case IIO_TEMP: temp = AD7173_VOLTAGE_INT_REF_uV * MILLI; temp /= AD7173_TEMP_SENSIIVITY_uV_per_C; *val = temp; *val2 = chan->scan_type.realbits; - } else { + break; + case IIO_VOLTAGE: *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); + + if (chan->channel < st->info->num_inputs_with_divider) + *val *= AD4111_DIVIDER_RATIO; + break; + case IIO_CURRENT: + *val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel); + *val /= AD4111_SHUNT_RESISTOR_OHM; + *val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar); + break; + default: + return -EINVAL; } return IIO_VAL_FRACTIONAL_LOG2; case IIO_CHAN_INFO_OFFSET: - if (chan->type == IIO_TEMP) { + + switch (chan->type) { + case IIO_TEMP: /* 0 Kelvin -> raw sample */ temp = -ABSOLUTE_ZERO_MILLICELSIUS; temp *= AD7173_TEMP_SENSIIVITY_uV_per_C; @@ -690,8 +833,13 @@ static int ad7173_read_raw(struct iio_dev *indio_dev, AD7173_VOLTAGE_INT_REF_uV * MILLI); *val = -temp; - } else { + break; + case IIO_VOLTAGE: + case IIO_CURRENT: *val = -BIT(chan->scan_type.realbits - 1); + break; + default: + return -EINVAL; } return IIO_VAL_INT; case IIO_CHAN_INFO_SAMP_FREQ: @@ -909,6 +1057,24 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev) &st->int_clk_hw); } +static int ad4111_validate_current_ain(struct ad7173_state *st, + unsigned int ain[2]) +{ + struct device *dev = &st->sd.spi->dev; + + if (!st->info->has_current_inputs) + return dev_err_probe(dev, -EINVAL, + "Reg values equal to or higher than %d are restricted to models with current channels.\n", + AD4111_CURRENT_CHAN_CUTOFF); + + if (ain[1] != 0 && ain[0] >= ARRAY_SIZE(ad4111_current_channel_config)) + return dev_err_probe(dev, -EINVAL, + "For current channel diff-channels must be <[0-%d],0>\n", + ARRAY_SIZE(ad4111_current_channel_config) - 1); + + return 0; +} + static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st, unsigned int ain[2]) { @@ -951,7 +1117,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[2], chan_index = 0; - int ref_sel, ret, num_channels; + int ref_sel, ret, reg, num_channels; num_channels = device_get_child_node_count(dev); @@ -1004,10 +1170,20 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) if (ret) return ret; - ret = ad7173_validate_voltage_ain_inputs(st, ain); + ret = fwnode_property_read_u32(child, "reg", ®); if (ret) return ret; + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { + ret = ad4111_validate_current_ain(st, ain); + if (ret) + return ret; + } else { + ret = ad7173_validate_voltage_ain_inputs(st, ain); + if (ret) + return ret; + } + ret = fwnode_property_match_property_string(child, "adi,reference-select", ad7173_ref_sel_str, @@ -1028,15 +1204,22 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev) *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]); + if (reg >= AD4111_CURRENT_CHAN_CUTOFF) { + chan->type = IIO_CURRENT; + chan->channel = ain[0]; + chan_st_priv->ain = ad4111_current_channel_config[ain[0]]; + } else { + 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->cfg.input_buf = st->info->has_input_buf; + } + chan_st_priv->chan_reg = chan_index; - chan_st_priv->cfg.input_buf = st->info->has_input_buf; chan_st_priv->cfg.odr = 0; - chan_st_priv->cfg.bipolar = fwnode_property_read_bool(child, "bipolar"); if (chan_st_priv->cfg.bipolar) chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET); @@ -1167,6 +1350,14 @@ static int ad7173_probe(struct spi_device *spi) } static const struct of_device_id ad7173_of_match[] = { + { .compatible = "ad4111", + .data = &ad7173_device_info[ID_AD4111]}, + { .compatible = "ad4112", + .data = &ad7173_device_info[ID_AD4112]}, + { .compatible = "ad4114", + .data = &ad7173_device_info[ID_AD4114]}, + { .compatible = "ad4115", + .data = &ad7173_device_info[ID_AD4115]}, { .compatible = "adi,ad7172-2", .data = &ad7173_device_info[ID_AD7172_2]}, { .compatible = "adi,ad7172-4", @@ -1186,6 +1377,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)&ad7173_device_info[ID_AD4111]}, + { "ad4112", (kernel_ulong_t)&ad7173_device_info[ID_AD4112]}, + { "ad4114", (kernel_ulong_t)&ad7173_device_info[ID_AD4114]}, + { "ad4115", (kernel_ulong_t)&ad7173_device_info[ID_AD4115]}, + { "ad4116", (kernel_ulong_t)&ad7173_device_info[ID_AD4116]}, { "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]}, { "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]}, { "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]}, @@ -1210,5 +1406,5 @@ module_spi_driver(ad7173_driver); MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA); MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>"); MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>"); -MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver"); +MODULE_DESCRIPTION("Analog Devices AD717x and AD411x ADC driver"); MODULE_LICENSE("GPL");