iio: adc: Add support for AD4170 series of ADCs
Subsystems (3)
PATCH 1/7 dt-bindings: iio: adc: Add AD4170
In reply to Marcelo SchmittAdd device tree documentation for AD4170 and similar sigma-delta ADCs. The AD4170 is a 24-bit, multichannel, sigma-delta ADC.
---
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.
.../bindings/iio/adc/adi,ad4170.yaml | 527 ++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 534 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +527 -0
@@ -0,0 +1,527 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4170 and similar Analog to Digital Converters + +maintainers: + - Marcelo Schmitt <[email protected]> + +description: | + Analog Devices AD4170 series of Sigma-delta Analog to Digital Converters. + Specifications can be found at: + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4190-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4195-4.pdf + +$ref: /schemas/spi/spi-peripheral-props.yaml# + +$defs: + sensor-node: + type: object + description: | + Common properties of external sensor circuitry connected to the ADC. + + properties: + reg: + description: + Channel number. Connects the sensor to the channel with this number + of the device. + minimum: 1 + maximum: 16 + + diff-channels: + $ref: /schemas/types.yaml#/definitions/uint32-array + maxItems: 2 + minItems: 2 + description: | + ADC analog input pins to which the sensor circuit is connected. + The first value specifies the positive input pin, the second + specifies the negative input pin. See adc.yaml for details. + + bipolar: + $ref: /schemas/types.yaml#/definitions/flag + description: If provided, the channel is to be used in bipolar mode. + + adi,sensor-type: + description: Type of sensor connected to the device. + $ref: /schemas/types.yaml#/definitions/uint8 + + adi,ac-excited: + type: boolean + description: | + Whether the external circuit has to be AC or DC excited. + + adi,excitation-pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + ADC pins used for external circuit excitation. Some applications + require optimum matching between excitation currents. Using excitation + current pairs minimizes the excitation current mismatch and the + excitation current drift matching on the ADC. Must describe either 1 + or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the first external reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2, 3] + default: 0 + + required: + - reg + - diff-channels + - bipolar + - adi,sensor-type + - adi,excitation-pins + - adi,reference-select + +properties: + compatible: + enum: + - adi,ad4170 + - adi,ad4190 + - adi,ad4195 + + avss-supply: + description: + Referece voltage supply for AVSS. If provided, describes the magnitude + (absolute value) of the negative voltage supplied to the AVSS pin. Since + AVSS must be −2.625V minimum and 0V maximum, the declared supply voltage + must be between 0 and 2.65V. If not provided, AVSS is assumed to be at + system ground (0V). + + avdd-supply: + description: + A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD). + + iovdd-supply: + description: 1.7V to 5.25V reference supply to the serial interface (IOVDD). + + refin1p-supply: + description: REFIN+ supply that can be used as reference for conversion. + + refin1n-supply: + description: REFIN- supply that can be used as reference for conversion. If + provided, describes the magnitude (absolute value) of the negative voltage + supplied to the REFIN- pin. + + refin2p-supply: + description: REFIN2+ supply that can be used as reference for conversion. + + refin2n-supply: + description: REFIN2- supply that can be used as reference for conversion. If + provided, describes the magnitude (absolute value) of the negative voltage + supplied to the REFIN2- pin. + + spi-cpol: true + + spi-cpha: true + + interrupts: + maxItems: 1 + + interrupt-names: + description: | + Specify which pin should be configured as Data Ready interrupt. + Default if not supplied is sdo. + enum: + - sdo + - dig_aux1 + + clocks: + maxItems: 1 + description: + Optional external clock source. Can specify either an external clock or + external crystal. + + clock-names: + enum: + - ext-clk + - xtal + + '#clock-cells': + const: 0 + + gpio-controller: true + + "#gpio-cells": + const: 2 + description: | + The first cell is for the GPIO number: 0 to 3. + The second cell takes standard GPIO flags. + + ldac-gpios: + description: + GPIO connected to DIG_AUX2 pin to be used as LDAC toggle to control the + transfer of data from the DAC_INPUT_A register to the DAC. + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +patternProperties: + "^channel@[0-9a-f]$": + $ref: adc.yaml + type: object + unevaluatedProperties: false + description: | + Represents the external channels which are connected to the ADC. + + properties: + reg: + description: | + The channel number. + items: + minimum: 0 + maximum: 15 + + diff-channels: + description: | + This property is used for defining the inputs of a differential + voltage channel. The first value is the positive input and the second + value is the negative input of the channel. + + Besides the analog input pins AIN0 to AIN8, there are special inputs + that can be selected with the following values: + 17: Internal temperature sensor + 18: (AVDD-AVSS)/5 + 19: (IOVDD-DGND)/5 + 20: DAC output + 21: ALDO + 22: DLDO + 23: AVSS + 24: DGND + 25: REFIN+ + 26: REFIN- + 27: REFIN2+ + 28: REFIN2- + 29: REFOUT + For the internal temperature sensor, use the input number for both + inputs (i.e. diff-channels = <17 17>). + items: + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20, 21, 22, 23, 24, 25, + 26, 27, 28, 29] + + single-channel: true + + common-mode-channel: true + + bipolar: true + + adi,sensor-type: + description: Sensor type for direct ADC sensors. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 0 + + adi,buffered-positive: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the positive voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC + combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on positive reference inputs (REFIN+, + REFIN2+) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled). + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,buffered-negative: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the negative voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC + combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on negative reference inputs (REFIN-, + REFIN2-) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled). + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the internal reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2, 3] + default: 2 + + required: + - reg + + allOf: + - oneOf: + - required: [single-channel] + properties: + diff-channels: false + - required: [diff-channels] + properties: + single-channel: false + common-mode-channel: false + + "^weighscale@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: Weigh scale sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 1 + + adi,excitation-pins: + description: | + ADC pins to use for weigh scale bridge circuit excitation. Must + describe either 1 or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 4 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. If not provided and + adi,ac-excited is true, use predefined ACX1, ACX1 negated, ACX2, and + ACX2 negated signals to AC excite the weigh scale bridge. Those + singals are output on GPIO2, GPIO0, GPIO3, and GPIO1, respectively. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + + adi,power-down-switch-pin: + description: | + Number of the GPIO used as power-down switch for the bridge circuit. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1] + + "^thermocouple@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: Thermocouple sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 2 + + adi,excitation-pins: + description: | + ADC pins to use for bridge circuit excitation. Must describe either 1 + or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 4 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. If not provided and + adi,ac-excited is true, use predefined ACX1, ACX1 negated, ACX2, and + ACX2 negated signals to AC excite the bridge circuit. Those singals + are output on GPIO2, GPIO0, GPIO3, and GPIO1, respectively. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + + adi,vbias: + type: boolean + description: | + For unbiased thermocouple applications, the voltage generated by the + thermocouple must be biased around some DC voltage. When present, this + property specifies a bias voltage of (AVDD + AVSS)/2 to be applied as + common-mode voltage for the sensor. + + adi,power-down-switch-pin: + description: | + Number of the GPIO used as power-down switch for the bridge circuit. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1] + + "^rtd@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: RTD sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 3 + + adi,excitation-pins: + description: | + ADC pins to use for RTD circuit excitation. Must describe a pair of + pins. E.g. <0 1>; <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 2 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: true + + required: + - adi,excitation-current-microamp + +required: + - compatible + - reg + - avdd-supply + - iovdd-supply + - spi-cpol + - spi-cpha + +allOf: + # Some devices don't have integrated DAC + - if: + properties: + compatible: + contains: + enum: + - adi,ad4190 + - adi,ad4195 + then: + properties: + ldac-gpios: false + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4170"; + reg = <0>; + spi-max-frequency = <20000000>; + spi-cpol; + spi-cpha; + avdd-supply = <&avdd>; + iovdd-supply = <&iovdd>; + interrupt-parent = <&gpio_in>; + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; + #address-cells = <1>; + #size-cells = <0>; + + // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range + // Differential bipolar. If AVSS < 0V, differential true bipolar + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN2 with respect to DGND throughout AVDD/DGND input range + // Pseudo-differential unipolar (fig. 2a) + channel@1 { + reg = <1>; + single-channel = <2>; + common-mode-channel = <24>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range + // Pseudo-differential bipolar (fig. 2b) + channel@2 { + reg = <2>; + bipolar; + single-channel = <3>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range + // Pseudo-differential bipolar (fig. 2c) + channel@3 { + reg = <3>; + bipolar; + single-channel = <4>; + common-mode-channel = <24>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN5 with respect to 2.5V throughout AVDD/AVSS input range + // Pseudo-differential unipolar (AD4170 datasheet page 46 example) + channel@4 { + reg = <4>; + single-channel = <5>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN6 with respect to 2.5V throughout REFIN+/REFIN- input range + // Pseudo-differential bipolar + channel@5 { + reg = <5>; + bipolar; + single-channel = <6>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <0>; + }; + // Weigh scale sensor + weighscale@6 { + reg = <6>; + bipolar; + diff-channels = <7 8>; + adi,sensor-type = /bits/ 8 <1>; + adi,ac-excited; + adi,excitation-pins = <17 18>, <19 20>; + adi,reference-select = /bits/ 8 <0>; + }; + }; + }; +... +
MAINTAINERS +7 -0
@@ -1337,6 +1337,13 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 F: Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml F: drivers/iio/adc/ad4130.c +ANALOG DEVICES INC AD4170 DRIVER +M: Marcelo Schmitt <[email protected]> +L: [email protected] +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml + ANALOG DEVICES INC AD4695 DRIVER M: Michael Hennerich <[email protected]> M: Nuno Sá <[email protected]>
Signature · 1 line
2.47.2
PATCH 2/7 iio: adc: Add basic support for AD4170
In reply to Marcelo SchmittFrom: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.
--- MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 16 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad4170.c | 1950 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 1968 insertions(+) create mode 100644 drivers/iio/adc/ad4170.c
MAINTAINERS +1 -0
@@ -1343,6 +1343,7 @@ L: [email protected] S: Supported W: https://ez.analog.com/linux-software-drivers F: Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml +F: drivers/iio/adc/ad4170.c ANALOG DEVICES INC AD4695 DRIVER M: Michael Hennerich <[email protected]>
drivers/iio/adc/Kconfig +16 -0
@@ -70,6 +70,22 @@ config AD4130 To compile this driver as a module, choose M here: the module will be called ad4130. + +config AD4170 + tristate "Analog Device AD4170 ADC Driver" + depends on SPI + depends on GPIOLIB + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select REGMAP_SPI + depends on COMMON_CLK + help + Say yes here to build support for Analog Devices AD4170 SPI analog + to digital converters (ADC). + + To compile this driver as a module, choose M here: the module will be + called ad4170. + config AD4695 tristate "Analog Device AD4695 ADC Driver" depends on SPI
drivers/iio/adc/Makefile +1 -0
@@ -11,6 +11,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.o obj-$(CONFIG_AD4000) += ad4000.o obj-$(CONFIG_AD4030) += ad4030.o obj-$(CONFIG_AD4130) += ad4130.o +obj-$(CONFIG_AD4170) += ad4170.o obj-$(CONFIG_AD4695) += ad4695.o obj-$(CONFIG_AD4851) += ad4851.o obj-$(CONFIG_AD7091R) += ad7091r-base.o
drivers/iio/adc/ad4170.c new file +1950 -0
1,950 lines not on this page · raw message
Signature · 1 line
2.47.2
PATCH 3/7 iio: adc: ad4170: Add support for buffered data capture
In reply to Marcelo SchmittExtend the AD4170 driver to allow buffered data capture in continuous read mode. In continuous read mode, the chip skips the instruction phase and outputs just ADC sample data, enabling faster sample rates to be reached. The internal channel sequencer always starts sampling from channel 0 and channel 0 must be enabled if more than one channel is selected for data capture. The scan mask validation callback checks the aforementioned condition is met.
--- drivers/iio/adc/ad4170.c | 170 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 169 insertions(+), 1 deletion(-)
drivers/iio/adc/ad4170.c +169 -1
@@ -10,10 +10,12 @@#include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/iio/buffer.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/iio/trigger.h> #include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/kernel.h>
@@ -323,6 +325,16 @@ struct ad4170_state {int sps_tbl[ARRAY_SIZE (ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE ][2 ];struct completion completion;struct iio_trigger *trig; + +struct spi_transfer xfer; +struct spi_message msg; +__be32 bounce_buffer[AD4170_MAX_CHANNELS ]; +/* +* DMA (thus cache coherency maintenance) requires the transfer buffers +* to live in their own cache lines. +*/ +__be32 rx_buf__aligned(IIO_DMA_MINALIGN) ; +u8 tx_buf[2 ]; };static void ad4170_fill_sps_tbl (struct ad4170_state *st)
@@ -882,6 +894,7 @@ static const struct iio_chan_spec ad4170_channel_template = { .scan_type = { .realbits = 24, .storagebits = 32, + .shift = 8, .endianness = IIO_BE, }, };
@@ -1480,11 +1493,29 @@ static int ad4170_write_raw_get_fmt(struct iio_dev *indio_dev, } } +static int ad4170_update_scan_mode (struct iio_dev *indio_dev, +const unsigned long *active_scan_mask) +{ +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int chan_index; +int ret; + +guard (mutex)(&st->lock); + +iio_for_each_active_channel (indio_dev, chan_index) { + ret =ad4170_set_channel_enable (st, chan_index, true); +if (ret) +return ret; + } +return 0 ; +} +static const struct iio_info ad4170_info = { .read_raw = ad4170_read_raw, .read_avail = ad4170_read_avail, .write_raw = ad4170_write_raw, .write_raw_get_fmt = ad4170_write_raw_get_fmt, + .update_scan_mode = ad4170_update_scan_mode, .debugfs_reg_access = ad4170_debugfs_reg_access, };
@@ -1759,6 +1790,130 @@ static int ad4170_initial_config(struct iio_dev *indio_dev) AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK); } +static int ad4170_prepare_spi_message (struct ad4170_state *st) +{ +/* +* Continuous data register read is enabled on buffer postenable so +* no instruction phase is needed meaning we don't need to send the +* register address to read data. Transfer only needs the read buffer. +*/ + st->xfer.rx_buf = &st->rx_buf; + st->xfer.len =BITS_TO_BYTES (ad4170_channel_template.scan_type.realbits); + +spi_message_init_with_transfers (&st->msg, &st->xfer,1 ); + +return devm_spi_optimize_message (&st->spi->dev, st->spi, &st->msg); +} + +static int ad4170_buffer_postenable (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + + ret =ad4170_set_mode (st,AD4170_ADC_CTRL_MODE_CONT ); +if (ret <0 ) +return ret; + +/* +* Enables continuous data register read. +* This enables continuous read of the ADC Data register. The ADC must +* be in a continuous conversion mode. +*/ +return regmap_update_bits (st->regmap16,AD4170_ADC_CTRL_REG , +AD4170_ADC_CTRL_CONT_READ_MSK , +FIELD_PREP (AD4170_ADC_CTRL_CONT_READ_MSK , +AD4170_ADC_CTRL_CONT_READ_ENABLE )); +} + +static int ad4170_buffer_predisable (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret, i; + +/* +* To exit continuous read, write 0xA5 to the ADC during the first 8 +* SCLKs of the ADC data read. +*/ + st->tx_buf[0 ] =AD4170_ADC_CTRL_CONT_READ_EXIT ; + st->tx_buf[1 ] =0 ; + ret =spi_write (st->spi, st->tx_buf,2 ); +if (ret) +return ret; + + ret =regmap_update_bits (st->regmap16,AD4170_ADC_CTRL_REG , +AD4170_ADC_CTRL_CONT_READ_MSK , +FIELD_PREP (AD4170_ADC_CTRL_CONT_READ_MSK , +AD4170_ADC_CTRL_CONT_READ_DISABLE )); +if (ret) +return ret; + + ret =ad4170_set_mode (st,AD4170_ADC_CTRL_MODE_IDLE ); +if (ret) +return ret; + +/* +* The ADC sequences through all the enabled channels (see datasheet +* page 95). That can lead to incorrect channel being read if a +* single-shot read (or buffered read with different active_scan_mask) +* is done after buffer disable. Disable all channels so only requested +* channels will be read. +*/ +for (i =0 ; i < indio_dev->num_channels; i++) { + ret =ad4170_set_channel_enable (st, i, false); +if (ret) +return ret; + } +return ret; +} + +static bool ad4170_validate_scan_mask (struct iio_dev *indio_dev, +const unsigned long *scan_mask) +{ +unsigned int masklength =iio_get_masklength (indio_dev); +unsigned long first, next; + +/* +* The channel sequencer cycles through the enabled channels in +* sequential order, from channel 0 to channel 15, bypassing disabled +* channels. When more than one channel is enabled, channel 0 must +* always be enabled. See datasheet channel_en register description at +* page 95. +*/ + first =find_next_bit (scan_mask, masklength,0 ); + next =find_next_bit (scan_mask, masklength, first +1 ); +if (next < masklength) +return test_bit (0 , scan_mask); + +return true; +} + +static const struct iio_buffer_setup_ops ad4170_buffer_ops = { + .postenable = ad4170_buffer_postenable, + .predisable = ad4170_buffer_predisable, + .validate_scan_mask = ad4170_validate_scan_mask, +}; + +static irqreturn_t ad4170_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ad4170_state *st =iio_priv (indio_dev); +int i, ret; + +iio_for_each_active_channel (indio_dev, i) { + ret =spi_sync (st->spi, &st->msg); +if (ret) + goto err_out; + + st->bounce_buffer[i] = st->rx_buf; + } + +iio_push_to_buffers (indio_dev, st->bounce_buffer); +err_out: +iio_trigger_notify_done (indio_dev->trig); +return IRQ_HANDLED ; +} +static const struct iio_trigger_ops ad4170_trigger_ops = { .validate_device = iio_trigger_validate_own_device, };
@@ -1768,7 +1923,10 @@ static irqreturn_t ad4170_irq_handler(int irq, void *dev_id)struct iio_dev *indio_dev = dev_id;struct ad4170_state *st =iio_priv (indio_dev); -complete (&st->completion); +if (iio_buffer_enabled (indio_dev)) +iio_trigger_poll (st->trig); +else +complete (&st->completion);return IRQ_HANDLED ; };
@@ -1915,6 +2073,16 @@ static int ad4170_probe(struct spi_device *spi)return dev_err_probe (dev, ret,"Failed to setup trigger\n" ); } + ret =ad4170_prepare_spi_message (st); +if (ret) +return dev_err_probe (dev, ret,"Failed to prepare SPI message\n" ); + + ret =devm_iio_triggered_buffer_setup (dev, indio_dev,NULL , + &ad4170_trigger_handler, + &ad4170_buffer_ops); +if (ret) +return dev_err_probe (dev, ret,"Failed to setup read buffer\n" ); +return devm_iio_device_register (dev, indio_dev); }
Signature · 1 line
2.47.2
PATCH 4/7 iio: adc: ad4170: Add clock provider support
In reply to Marcelo SchmittThe AD4170 chip can use an externally supplied clock at the XTAL2 pin, or an external crystal connected to the XTAL1 and XTAL2 pins. Alternatively, the AD4170 can provide it's 16 MHz internal clock at the XTAL2 pin. Extend the AD4170 driver so it effectively uses the provided external clock, if any, or supplies it's own clock as a clock provider.
--- drivers/iio/adc/ad4170.c | 135 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 134 insertions(+), 1 deletion(-)
drivers/iio/adc/ad4170.c +134 -1
@@ -7,6 +7,8 @@#include <linux/bitfield.h> #include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/delay.h> #include <linux/device.h> #include <linux/err.h>
@@ -62,6 +64,7 @@#define AD4170_DATA_16B_STATUS_REG 0x1A#define AD4170_DATA_24B_REG 0x1E#define AD4170_PIN_MUXING_REG 0x69 +#define AD4170_CLOCK_CTRL_REG 0x6B#define AD4170_ADC_CTRL_REG 0x71#define AD4170_CHAN_EN_REG 0x79#define AD4170_CHAN_SETUP_REG (x) (0x81 + 4 * (x))
@@ -89,6 +92,9 @@#define AD4170_PIN_MUXING_DIG_AUX1_CTRL_MSK GENMASK(5, 4)#define AD4170_PIN_MUXING_SYNC_CTRL_MSK GENMASK(3, 2) +/* AD4170_CLOCK_CTRL_REG */ +#define AD4170_CLOCK_CTRL_CLOCKSEL_MSK GENMASK(1, 0) +/* AD4170_ADC_CTRL_REG */ #define AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK BIT(7)#define AD4170_ADC_CTRL_CONT_READ_MSK GENMASK(5, 4)
@@ -121,6 +127,12 @@/* AD4170 register constants */ +/* AD4170_CLOCK_CTRL_REG constants */ +#define AD4170_CLOCK_CTRL_CLOCKSEL_INT 0x0 +#define AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT 0x1 +#define AD4170_CLOCK_CTRL_CLOCKSEL_EXT 0x2 +#define AD4170_CLOCK_CTRL_CLOCKSEL_EXT_XTAL 0x3 +/* AD4170_CHAN_MAP_REG constants */ #define AD4170_CHAN_MAP_AIN0 0#define AD4170_CHAN_MAP_AIN1 1
@@ -238,6 +250,10 @@ enum ad4170_regulator {AD4170_MAX_SUP }; +static const char *const ad4170_clk_sel[] = { +"ext-clk" ,"xtal" +}; +enum ad4170_int_pin_sel {AD4170_INT_PIN_SDO ,AD4170_INT_PIN_DIG_AUX1 ,
@@ -320,6 +336,9 @@ struct ad4170_state {struct ad4170_chan_info chan_infos[AD4170_MAX_CHANNELS ];struct ad4170_setup_info setup_infos[AD4170_MAX_SETUPS ];u32 mclk_hz; +unsigned int clock_ctrl; +struct clk *ext_clk; +struct clk_hw int_clk_hw;int pins_fn[AD4170_NUM_ANALOG_PINS ];u32 int_pin_sel;int sps_tbl[ARRAY_SIZE (ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE ][2 ];
@@ -1693,13 +1712,127 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev)return 0 ; } +static struct ad4170_state *clk_hw_to_ad4170 (struct clk_hw *hw) +{ + return container_of(hw, struct ad4170_state, int_clk_hw); +} + +static unsigned long ad4170_sel_clk (struct ad4170_state *st, +unsigned int clk_sel) +{ + st->clock_ctrl &= ~AD4170_CLOCK_CTRL_CLOCKSEL_MSK ; + st->clock_ctrl |=FIELD_PREP (AD4170_CLOCK_CTRL_CLOCKSEL_MSK , clk_sel); +return regmap_write (st->regmap16,AD4170_CLOCK_CTRL_REG , st->clock_ctrl); +} + +static unsigned long ad4170_clk_recalc_rate (struct clk_hw *hw, +unsigned long parent_rate) +{ +return AD4170_INT_CLOCK_16MHZ ; +} + +static int ad4170_clk_output_is_enabled (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); +u32 clk_sel; + + clk_sel =FIELD_GET (AD4170_CLOCK_CTRL_CLOCKSEL_MSK , st->clock_ctrl); +return clk_sel ==AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT ; +} + +static int ad4170_clk_output_prepare (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); + +return ad4170_sel_clk (st,AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT ); +} + +static void ad4170_clk_output_unprepare (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); + +ad4170_sel_clk (st,AD4170_CLOCK_CTRL_CLOCKSEL_INT ); +} + +static const struct clk_ops ad4170_int_clk_ops = { + .recalc_rate = ad4170_clk_recalc_rate, + .is_enabled = ad4170_clk_output_is_enabled, + .prepare = ad4170_clk_output_prepare, + .unprepare = ad4170_clk_output_unprepare, +}; + +static int ad4170_register_clk_provider (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +struct device *dev = indio_dev->dev.parent; +struct fwnode_handle *fwnode =dev_fwnode (dev); +struct clk_init_data init = {}; +int ret; + +if (!IS_ENABLED (CONFIG_COMMON_CLK )) +return 0 ; + + init.name =fwnode_get_name (fwnode); + init.ops = &ad4170_int_clk_ops; + + st->int_clk_hw.init = &init; + ret =devm_clk_hw_register (dev, &st->int_clk_hw); +if (ret) +return ret; + +return devm_of_clk_add_hw_provider (dev, of_clk_hw_simple_get, + &st->int_clk_hw); +} + +static int ad4170_clock_select (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +struct device *dev = &st->spi->dev; +int ret; + + st->mclk_hz =AD4170_INT_CLOCK_16MHZ ; + ret =device_property_match_property_string (dev,"clock-names" , + ad4170_clk_sel, +ARRAY_SIZE (ad4170_clk_sel)); +if (ret <0 ) { +/* Use internal clock reference */ + st->clock_ctrl |=FIELD_PREP (AD4170_CLOCK_CTRL_CLOCKSEL_MSK , +AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT ); +return ad4170_register_clk_provider (indio_dev); + } + +/* Use external clock reference */ + st->ext_clk =devm_clk_get_enabled (dev, ad4170_clk_sel[ret]); +if (IS_ERR (st->ext_clk)) +return dev_err_probe (dev,PTR_ERR (st->ext_clk), +"Failed to get external clock\n" ); + + st->clock_ctrl |=FIELD_PREP (AD4170_CLOCK_CTRL_CLOCKSEL_MSK , +AD4170_CLOCK_CTRL_CLOCKSEL_EXT + ret); + + st->mclk_hz =clk_get_rate (st->ext_clk); +if (st->mclk_hz <AD4170_EXT_CLOCK_MHZ_MIN || + st->mclk_hz >AD4170_EXT_CLOCK_MHZ_MAX ) { +return dev_err_probe (dev, -EINVAL , +"Invalid external clock frequency %u\n" , + st->mclk_hz); + } +return 0 ; +} +static int ad4170_parse_firmware (struct iio_dev *indio_dev) {struct ad4170_state *st =iio_priv (indio_dev);struct device *dev = &st->spi->dev;int reg_data, ret, i; - st->mclk_hz =AD4170_INT_CLOCK_16MHZ ; + ret =ad4170_clock_select (indio_dev); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to setup device clock\n" ); + + ret =regmap_write (st->regmap16,AD4170_CLOCK_CTRL_REG , st->clock_ctrl); +if (ret) +return ret;for (i =0 ; i <AD4170_NUM_ANALOG_PINS ; i++) st->pins_fn[i] =AD4170_PIN_UNASIGNED ;
Signature · 1 line
2.47.2
PATCH 5/7 iio: adc: ad4170: Add GPIO controller support
In reply to Marcelo SchmittThe AD4170 has four multifunctional pins that can be used as GPIOs. The GPIO functionality can be accessed when the AD4170 chip is not busy performing continuous data capture or handling any other register read/write request. Also, the AD4170 does not provide any interrupt based on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
Implement gpio_chip callbacks so to make AD4170 GPIO pins controllable through the gpiochip interface.
--- drivers/iio/adc/ad4170.c | 167 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 166 insertions(+), 1 deletion(-)
drivers/iio/adc/ad4170.c +166 -1
@@ -12,6 +12,7 @@#include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/gpio/driver.h> #include <linux/iio/buffer.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h>
@@ -79,6 +80,7 @@#define AD4170_FIR_CTRL 0x141#define AD4170_COEFF_DATA_REG 0x14A#define AD4170_COEFF_ADDR_REG 0x14C +#define AD4170_GPIO_MODE_REG 0x191#define AD4170_GPIO_OUTPUT_REG 0x193#define AD4170_GPIO_INPUT_REG 0x195
@@ -189,6 +191,7 @@/* Device properties and auxiliary constants */ #define AD4170_NUM_ANALOG_PINS 9 +#define AD4170_NUM_GPIO_PINS 4#define AD4170_MAX_CHANNELS 16#define AD4170_MAX_ANALOG_PINS 8#define AD4170_MAX_SETUPS 8
@@ -340,6 +343,7 @@ struct ad4170_state {struct clk *ext_clk;struct clk_hw int_clk_hw;int pins_fn[AD4170_NUM_ANALOG_PINS ]; +struct gpio_chip gpiochip;u32 int_pin_sel;int sps_tbl[ARRAY_SIZE (ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE ][2 ];struct completion completion;
@@ -1553,6 +1557,156 @@ static int ad4170_soft_reset(struct ad4170_state *st)return 0 ; } +static int ad4170_gpio_get (struct gpio_chip *gc,unsigned int offset) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_read (st->regmap16,AD4170_GPIO_MODE_REG , &val); +if (ret) + goto err_release; + +/* +* If the GPIO is configured as an input, read the current value from +* AD4170_GPIO_INPUT_REG. Otherwise, read the input value from +* AD4170_GPIO_OUTPUT_REG. +*/ +if (val &BIT (offset *2 )) + ret =regmap_read (st->regmap16,AD4170_GPIO_INPUT_REG , &val); +else + ret =regmap_read (st->regmap16,AD4170_GPIO_OUTPUT_REG , &val); +if (ret) + goto err_release; + + ret = !!(val &BIT (offset)); +err_release: +iio_device_release_direct (indio_dev); + +return ret; +} + +static int ad4170_gpio_set (struct gpio_chip *gc,unsigned int offset,int value) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_read (st->regmap16,AD4170_GPIO_MODE_REG , &val); +if (ret) + goto err_release; + +if (val &BIT (offset *2 +1 )) + ret =regmap_update_bits (st->regmap16,AD4170_GPIO_OUTPUT_REG , +BIT (offset), value << offset); + +err_release: +iio_device_release_direct (indio_dev); +return ret; +} + +static int ad4170_gpio_get_direction (struct gpio_chip *gc,unsigned int offset) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_read (st->regmap16,AD4170_GPIO_MODE_REG , &val); +if (ret) + goto err_release; + +if (val &BIT (offset *2 +1 )) + ret =GPIO_LINE_DIRECTION_OUT ; +else + ret =GPIO_LINE_DIRECTION_IN ; + +err_release: +iio_device_release_direct (indio_dev); + +return ret; +} + +static int ad4170_gpio_direction_input (struct gpio_chip *gc,unsigned int offset) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_clear_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 +1 )); +if (ret) + goto err_release; + + ret =regmap_set_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 )); + +err_release: +iio_device_release_direct (indio_dev); + +return ret; +} + +static int ad4170_gpio_direction_output (struct gpio_chip *gc, +unsigned int offset,int value) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_clear_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 )); +if (ret) + goto err_release; + + ret =regmap_set_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 +1 )); + +err_release: +iio_device_release_direct (indio_dev); + +ad4170_gpio_set (gc, offset, value); +return ret; +} + +static int ad4170_gpio_init (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); + + st->gpiochip = (struct gpio_chip ) { + .label ="ad4170_gpios" , + .base =-1 , + .ngpio =4 , + .parent = &st->spi->dev, + .can_sleep = true, + .get_direction = ad4170_gpio_get_direction, + .direction_input = ad4170_gpio_direction_input, + .direction_output = ad4170_gpio_direction_output, + .get = ad4170_gpio_get, + .set_rv = ad4170_gpio_set, + .owner =THIS_MODULE , + }; + +return devm_gpiochip_add_data (&st->spi->dev, &st->gpiochip, indio_dev); +} +static int ad4170_parse_reference (struct ad4170_state *st,struct fwnode_handle *child,struct ad4170_setup *setup)
@@ -1855,7 +2009,18 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)if (ret)return ret; -return ad4170_parse_channels (indio_dev); + ret =ad4170_parse_channels (indio_dev); +if (ret) +return ret; + +/* Only create a GPIO chip if flagged for it */ +if (device_property_read_bool (&st->spi->dev,"gpio-controller" )) { + ret =ad4170_gpio_init (indio_dev); +if (ret <0 ) +return ret; + } + +return 0 ; }static int ad4170_initial_config (struct iio_dev *indio_dev)
Signature · 1 line
2.47.2
PATCH 6/7 iio: adc: ad4170: Add support for internal temperature sensor
In reply to Marcelo SchmittThe AD4170 has an internal temperature sensor that can be read using the ADC. Whenever possible, configure an IIO channel to provide the chip's temperature.
--- drivers/iio/adc/ad4170.c | 72 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 69 insertions(+), 3 deletions(-)
drivers/iio/adc/ad4170.c +69 -3
@@ -922,6 +922,27 @@ static const struct iio_chan_spec ad4170_channel_template = { }, }; +static const struct iio_chan_spec ad4170_temp_channel_template = { + .type =IIO_TEMP , + .indexed =0 , + .channel =17 , + .channel2 =17 , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ) | +BIT (IIO_CHAN_INFO_OFFSET ) | +BIT (IIO_CHAN_INFO_CALIBSCALE ) | +BIT (IIO_CHAN_INFO_CALIBBIAS ) | +BIT (IIO_CHAN_INFO_SAMP_FREQ ), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_SAMP_FREQ ), + .scan_type = { + .sign ='s' , + .realbits =24 , + .storagebits =32 , + .shift =8 , + .endianness =IIO_BE , + }, +}; +/* * Receives the number of a multiplexed AD4170 input (ain_n), and stores the * voltage (in µV) of the specified input into ain_voltage. If the input number
@@ -1209,9 +1230,27 @@ static int ad4170_read_raw(struct iio_dev *indio_dev,return ret;case IIO_CHAN_INFO_SCALE : pga =FIELD_GET (AD4170_AFE_PGA_GAIN_MSK , setup->afe); - *val = chan_info->scale_tbl[pga][0 ]; - *val2 = chan_info->scale_tbl[pga][1 ]; -return IIO_VAL_INT_PLUS_NANO ; +switch (chan->type) { +case IIO_VOLTAGE : + *val = chan_info->scale_tbl[pga][0 ]; + *val2 = chan_info->scale_tbl[pga][1 ]; +return IIO_VAL_INT_PLUS_NANO ; + +case IIO_TEMP : +/* +* The scale_tbl converts output codes to mV units so +* multiply by MILLI to make the factor convert to µV. +* Then, apply the temperature sensor change sensitivity +* of 477 μV/K. Finally, multiply the result by MILLI +* again to comply with milli degrees Celsius IIO ABI. +*/ + *val =0 ;/* The scale integer part is always 0. */ + *val2 =DIV_ROUND_CLOSEST (chan_info->scale_tbl[pga][1 ] *MILLI , +477 ) *MILLI ; +return IIO_VAL_INT_PLUS_NANO ; +default : +return -EINVAL ; + }case IIO_CHAN_INFO_OFFSET : pga =FIELD_GET (AD4170_AFE_PGA_GAIN_MSK , setup->afe); *val = chan_info->offset_tbl[pga];
@@ -1855,12 +1894,39 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev)if (num_channels >AD4170_MAX_CHANNELS )return dev_err_probe (dev, -EINVAL ,"Too many channels\n" ); +/* Add one for temperature */ + num_channels =min (num_channels +1 ,AD4170_MAX_CHANNELS ); +device_for_each_child_node_scoped (dev, child) { ret =ad4170_parse_channel_node (indio_dev, child, chan_num++);if (ret)return ret; } +/* +* Add internal temperature sensor channel if the maximum number of +* channels has not been reached. +*/ +if (num_channels <AD4170_MAX_CHANNELS ) { +struct ad4170_setup *setup = &st->chan_infos[chan_num].setup; + + st->chans[chan_num] = ad4170_temp_channel_template; + st->chans[chan_num].address = chan_num; + st->chans[chan_num].scan_index = chan_num; + + st->chan_infos[chan_num].setup_num =AD4170_INVALID_SETUP ; + st->chan_infos[chan_num].initialized = true; + + setup->afe |=FIELD_PREP (AD4170_AFE_REF_SELECT_MSK , +AD4170_REF_AVDD ); + + ret =ad4170_get_input_range (st, &st->chans[chan_num], chan_num, +AD4170_REF_AVDD ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Invalid input config\n" ); + + st->chan_infos[chan_num].input_range_uv = ret; + } indio_dev->num_channels = num_channels; indio_dev->channels = st->chans;return 0 ;
Signature · 1 line
2.47.2
PATCH 7/7 iio: adc: ad4170: Add support for weigh scale and RTD sensors
In reply to Marcelo SchmittThe 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.
--- drivers/iio/adc/ad4170.c | 341 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 338 insertions(+), 3 deletions(-)
drivers/iio/adc/ad4170.c +338 -3
@@ -77,6 +77,7 @@#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_FIR_CTRL 0x141#define AD4170_COEFF_DATA_REG 0x14A#define AD4170_COEFF_ADDR_REG 0x14C
@@ -127,6 +128,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 */
@@ -188,6 +193,21 @@#define AD4170_FILTER_FILTER_TYPE_SINC5 0x4#define AD4170_FILTER_FILTER_TYPE_SINC3 0x6 +/* AD4170_CURRENT_SRC_REG constants */ +#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
@@ -222,6 +242,12 @@#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 0x04enum ad4170_ref_buf {AD4170_REF_BUF_PRE ,/* Pre-charge referrence buffer */
@@ -278,6 +304,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 +}; + +enum ad4170_sensor_type { +AD4170_ADC_SENSOR =0 , +AD4170_WEIGH_SCALE_SENSOR =1 , +AD4170_THERMOCOUPLE_SENSOR =2 , +AD4170_RTD_SENSOR =3 , +}; +static const char *const ad4170_chip_names[] = {"ad4170" , "ad4190",
@@ -343,6 +396,7 @@ 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 ];struct gpio_chip gpiochip;u32 int_pin_sel;int sps_tbl[ARRAY_SIZE (ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE ][2 ];
@@ -956,6 +1010,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 ;
@@ -1746,6 +1813,242 @@ 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; + +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_rtd (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 current_src, ret, i; + +for (i =0 ; i < num_exc_pins; i++) { +unsigned int pin = exc_pins[i]; + + current_src |=FIELD_PREP (AD4170_CURRENT_SRC_I_OUT_PIN_MSK , pin); + current_src |=FIELD_PREP (AD4170_CURRENT_SRC_I_OUT_VAL_MSK , exc_cur); + + ret =regmap_write (st->regmap16,AD4170_CURRENT_SRC_REG (i), + current_src); +if (ret) +return ret; + } + +if (ac_excited) + setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_IEXC_MSK , + num_exc_pins ==2 ?0x2 :0x3 ); + +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 current_src, ret, i; + +if (!ac_excited) +return 0 ; + +/* +* 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. See AD4170 datasheet for instructions +* on how to setup the bridge circuit. +* +* 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 ) { +if (num_exc_pins ==2 ) { + setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_ADC_MSK ,0x3 ); + ret =regmap_set_bits (st->regmap16, +AD4170_GPIO_MODE_REG , +BIT (7 ) |BIT (5 )); +if (ret) +return ret; + + ret =regmap_set_bits (st->regmap16, +AD4170_GPIO_OUTPUT_REG , +BIT (3 ) |BIT (2 )); +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->regmap16, +AD4170_GPIO_MODE_REG , +BIT (7 ) |BIT (5 ) |BIT (3 ) |BIT (1 )); +if (ret) +return ret; + + ret =regmap_set_bits (st->regmap16, +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 ; + } +for (i =0 ; i < num_exc_pins; i++) { +unsigned int pin = exc_pins[i]; + + current_src |=FIELD_PREP (AD4170_CURRENT_SRC_I_OUT_PIN_MSK , pin); + current_src |=FIELD_PREP (AD4170_CURRENT_SRC_I_OUT_VAL_MSK , exc_cur); + + ret =regmap_write (st->regmap16,AD4170_CURRENT_SRC_REG (i), + current_src); +if (ret) +return ret; + } + + setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_IEXC_MSK , + num_exc_pins ==2 ?0x2 :0x3 ); + +return 0 ; +} + +static int ad4170_parse_external_sensor (struct ad4170_state *st, +struct fwnode_handle *child, +struct ad4170_setup *setup, +struct iio_chan_spec *chan,u8 s_type) +{ +unsigned int num_exc_pins, exc_cur, reg_val; +struct device *dev = &st->spi->dev; +u32 pins[2 ], exc_pins[4 ]; +bool ac_excited, vbias; +int ret; + + ret =fwnode_property_read_u32_array (child,"diff-channels" , pins, +ARRAY_SIZE (pins)); +if (ret) +return dev_err_probe (dev, ret, +"Failed to read sensor diff-channels\n" ); + + chan->differential = true; + chan->channel = pins[0 ]; + chan->channel2 = pins[1 ]; + + ac_excited =fwnode_property_read_bool (child,"adi,ac-excited" ); + + num_exc_pins =fwnode_property_count_u32 (child,"adi,excitation-pins" ); +if (num_exc_pins !=2 && num_exc_pins !=4 ) +return dev_err_probe (dev, -EINVAL , +"Invalid number of excitation pins\n" ); + + ret =fwnode_property_read_u32_array (child,"adi,excitation-pins" , + exc_pins, num_exc_pins); +if (ret) +return dev_err_probe (dev, ret, +"Failed to read adi,excitation-pins\n" ); + + ret =ad4170_validate_excitation_pins (st, exc_pins, num_exc_pins); +if (ret) +return ret; + + exc_cur =0 ; + ret =fwnode_property_read_u32 (child,"adi,excitation-current-microamp" , + &exc_cur); +if (ret && s_type ==AD4170_RTD_SENSOR ) +return dev_err_probe (dev, ret, +"Failed to read adi,excitation-current-microamp\n" ); + + ret =ad4170_find_table_index (ad4170_iout_current_ua_tbl, exc_cur); +if (ret <0 ) +return dev_err_probe (dev, ret, +"Invalid excitation current: %uuA\n" , + exc_cur); + +/* Get the excitation current configuration value */ + exc_cur = ret; + +if (s_type ==AD4170_THERMOCOUPLE_SENSOR ) { + vbias =fwnode_property_read_bool (child,"adi,vbias" ); +if (vbias) { + st->pins_fn[chan->channel2] |=AD4170_PIN_VBIAS ; + reg_val =BIT (chan->channel2); +return regmap_write (st->regmap16,AD4170_V_BIAS_REG , + reg_val); + } + } +if (s_type ==AD4170_WEIGH_SCALE_SENSOR || + s_type ==AD4170_THERMOCOUPLE_SENSOR ) { + ret =ad4170_setup_bridge (st, child, setup, exc_pins, + num_exc_pins, exc_cur, ac_excited); + }else { + ret =ad4170_setup_rtd (st, child, setup, exc_pins, num_exc_pins, + exc_cur, ac_excited); + } +return ret; +} +static int ad4170_parse_reference (struct ad4170_state *st,struct fwnode_handle *child,struct ad4170_setup *setup)
@@ -1827,6 +2130,7 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev,struct ad4170_state *st =iio_priv (indio_dev);struct device *dev = &st->spi->dev;struct ad4170_chan_info *chan_info; +u8 s_type =AD4170_ADC_SENSOR ;struct ad4170_setup *setup;struct iio_chan_spec *chan;unsigned int ch_reg;
@@ -1857,10 +2161,34 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev,if (ret)return ret; - ret =ad4170_parse_adc_channel_type (dev, child, chan); -if (ret <0 ) -return ret; + ret =fwnode_property_read_u8 (child,"adi,sensor-type" , &s_type); +if (!ret) { +if (s_type >AD4170_RTD_SENSOR ) +return dev_err_probe (dev, ret, +"Invalid adi,sensor-type: %u\n" , + s_type); + } +switch (s_type) { +case AD4170_ADC_SENSOR : + ret =ad4170_parse_adc_channel_type (dev, child, chan); +if (ret <0 ) +return ret; +break ; +case AD4170_WEIGH_SCALE_SENSOR : + fallthrough; +case AD4170_THERMOCOUPLE_SENSOR : + fallthrough; +case AD4170_RTD_SENSOR : + ret =ad4170_parse_external_sensor (st, child, setup, chan, + s_type); +if (ret <0 ) +return ret; + +break ; +default : +return -EINVAL ; + } bipolar =fwnode_property_read_bool (child,"bipolar" ); setup->afe |=FIELD_PREP (AD4170_AFE_BIPOLAR_MSK , bipolar);if (bipolar)
@@ -2057,6 +2385,9 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)for (i =0 ; i <AD4170_NUM_ANALOG_PINS ; i++) st->pins_fn[i] =AD4170_PIN_UNASIGNED ; +for (i =0 ; i <AD4170_NUM_GPIO_PINS ; i++) + st->gpio_fn[i] =AD4170_GPIO_UNASIGNED ; +/* On power on, device defaults to using SDO pin for data ready signal */ st->int_pin_sel =AD4170_INT_PIN_SDO ; ret = device_property_match_property_string(dev, "interrupt-names",
@@ -2081,6 +2412,10 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)/* Only create a GPIO chip if flagged for it */ if (device_property_read_bool (&st->spi->dev,"gpio-controller" )) { +for (i =0 ; i <AD4170_NUM_GPIO_PINS ; i++) +if (st->gpio_fn[i] !=AD4170_GPIO_UNASIGNED ) +return 0 ; + ret =ad4170_gpio_init (indio_dev);if (ret <0 )return ret;
Signature · 1 line
2.47.2
cover iio: adc: Add support for AD4170 series of ADCs
This patch set adds support for Analog Devices AD4170 and similar sigma-delta ADCs.
I've arranged the patches so that the first ones comprise changes more typical of IIO device drivers while the last one has the most uncommon changes. I believe to have applied all suggestions to the RFC version.
Patch 1 adds device tree documentation for the parts. Patch 2 adds basic device support.
Patch 3 adds support for buffered ADC reading.
Patch 4 adds clock provider support
Patch 5 adds GPIO controller support.
Patch 6 adds internal temperature sensor support.
Patch 7 adds support for external RTD and bridge circuit sensors.
For context, an initial version of the ad4170 driver was developed by Ana-Maria Cusco. I then picked that up, created dt docs, and did several improvements to the driver.
This first version has so many changes compared to the RFC version that it's not worth listing them all. The most significant one, though, are listed below.
Change log RFC -> v1
[IIO driver changes]
- Split off extra features into smaller additional patches.
- No longer pushing timestamp data to buffers.
- Used iio_for_each_active_channel() to iterate over channels in trigger handler.
- Correctly implemented incomplete/broken usage of SPI optimized messages.
- No longer setting interrupt direction in driver.
- Register names, register field names, and names of constants for register fields now follow an established pattern.
- Data ready interrupt config now set according to interrupt-names dt property.
- Added scan_mask to validate chan 0 is enabled when multiple chans are enabled.
- Added a comment explaining the reason to disable all channel at buffer_predisable().
- Complemented comment about channel 0 being required to be enabled when more than one channel is enabled.
- New GPIO controller support patch.
- New internal temperature sensor support patch.
There are some places where I used find_closest() to match/verify values from dt. On the last patch, I added ad4170_find_table_index() to avoid find_closest() but may use something else if there are any generic helper for such thing.
[device tree changes]
- Dropped adi,ad4170.h. All dt documentation is now in adi,ad4170.yaml.
- Described external bridge circuits and RTD sensors as specialized channels with specific properties for excitation control.
- Bridge circuit excitation properties are now sensor-node only.
- Added compatibles for 2 more similar chips.
- Added ldac-gpios.
- Dropped adi,dig-aux1, DIG_AUX1 now set according to Data Ready interrupt choice.
- Dropped adi,dig-aux2, DIG_AUX2 now set according to LDAC GPIO presence.
- Dropped adi,sync-option, SYNC_CTRL will only of be set for multi-device sync.
- Dropped adi,burnout-current-nanoamp since open wire detection should be a runtime config.
- Complemented adi,buffered-pos/neg description with possible reason to not enable them.
- adi,gpion-power-down-switch renamed to adi,power-down-switch-pin is now a sensor-node only prop.
About the bridge power-down-switch pins, it was mentioned that maybe they could be described as regulators that the bridge circuit would consume. One complication of that approach is that the AD4170 powerdown switches close to AVSS/GND and that doesn't help determining regulator maximum voltage. It may also be desired to control the power switches at runtime to save power by closing the power down switches when the bridge would not be in use. Because of that, I have removed pw-down switch props from the dt-binding.
Not sure what to do about the various possible multiplexed inputs to the ADC. Even though the IIO driver doesn't handle all of them, I'm keeping those since dt-bindings are intended to be OS agnostic and we are expected to make bindings complete.
Ana-Maria Cusco (1):
iio: adc: Add basic support for AD4170
Marcelo Schmitt (6):
dt-bindings: iio: adc: Add AD4170
iio: adc: ad4170: Add support for buffered data capture
iio: adc: ad4170: Add clock provider support
iio: adc: ad4170: Add GPIO controller support
iio: adc: ad4170: Add support for internal temperature sensor
iio: adc: ad4170: Add support for weigh scale and RTD sensors
.../bindings/iio/adc/adi,ad4170.yaml | 527 +++ MAINTAINERS | 8 + drivers/iio/adc/Kconfig | 16 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/ad4170.c | 2817 +++++++++++++++++ 5 files changed, 3369 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml create mode 100644 drivers/iio/adc/ad4170.c
base-commit: 1c2409fe38d5c19015d69851d15ba543d1911932
Signature · 1 line
2.47.2
Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Marcelo SchmittHi Marecelo,
First, superficial look...
Quoted 106 lines · Marcelo Schmitt
From: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.Signed-off-by Ana-Maria Cusco <[email protected]>Co-developed-by Marcelo Schmitt <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 16 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad4170.c | 1950 ++++++++++++++++++++++++++++++++++++++4 files changed, 1968 insertions(+)create mode 100644
drivers/iio/adc/ad4170.cMAINTAINERS +0
-0@@ -1343,6 +1343,7 @@ L: [email protected]S: Supported
W: https://ez.analog.com/linux-software-drivers
F:Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml
+F:drivers/iio/adc/ad4170.c
ANALOG DEVICES INC AD4695 DRIVER
M: Michael Hennerich <[email protected]>drivers/iio/adc/Kconfig +0
-0@@ -70,6 +70,22 @@ config AD4130To compile this driver as a module, choose M here: the module will be called ad4130.
drivers/iio/adc/Kconfig +16
-0+ +config AD4170 + tristate "Analog Device AD4170 ADC Driver" + depends on SPI + depends on GPIOLIB + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER + select REGMAP_SPI + depends on COMMON_CLK + help + Say yes here to build support for Analog Devices AD4170 SPI analog + to digital converters (ADC). + + To compile this driver as a module, choose M here: the module will be + called ad4170. +config AD4695
tristate "Analog Device AD4695 ADC Driver" depends on SPIdrivers/iio/adc/Makefile +0
-0@@ -11,6 +11,7 @@ obj-$(CONFIG_AD_SIGMA_DELTA) += ad_sigma_delta.oobj-$(CONFIG_AD4000) += ad4000.o
obj-$(CONFIG_AD4030) += ad4030.o
obj-$(CONFIG_AD4130) += ad4130.o
+obj-$(CONFIG_AD4170) += ad4170.o
obj-$(CONFIG_AD4695) += ad4695.o
obj-$(CONFIG_AD4851) += ad4851.o
obj-$(CONFIG_AD7091R) += ad7091r-base.odrivers/iio/adc/ad4170.c new file +29
-0@@ -0,0 +1,1950 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* +* Copyright (C) 2024 Analog Devices, Inc. +* Author: Ana-Maria Cusco <[email protected]> +* Author: Marcelo Schmitt <[email protected]> +*/ + +#include <linux/bitfield.h> +#include <linux/bitops.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/trigger.h> +#include <linux/iio/trigger_consumer.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/kernel.h> +#include <linux/math64.h> +#include <linux/module.h> +#include <linux/property.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/spi/spi.h> +#include <linux/unaligned.h> +#include <linux/units.h> +#include <linux/util_macros.h> +
...
Quoted 9 lines
drivers/iio/adc/ad4170.c new file +9
-0+ +static int ad4170_read_sample (struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int settling_time_ms, ret; + +guard (mutex)(&st->lock); +/*
drivers/iio/adc/ad4170.c line 1119
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+* The ADC sequences through all enabled channels. That can lead to +* incorrect channel being sampled if a previous read would have left a +* different channel enabled. Thus, always enable and disable the +* channel on single-shot read. +*/ + ret =ad4170_set_channel_enable (st, chan->address, true); +if (ret) +return ret; + +reinit_completion (&st->completion);
I would do the above right before wait_for_completion_timeout()...
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +4
-0+ + ret =ad4170_set_mode (st,AD4170_ADC_CTRL_MODE_SINGLE ); +if (ret) + goto err_disable;
...
Quoted 15 lines
drivers/iio/adc/ad4170.c new file +14
-0+ +static int ad4170_set_pga (struct ad4170_state *st, + struct iio_chan_spec const *chan, int val, int val2) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +unsigned int old_pga =FIELD_GET (AD4170_AFE_PGA_GAIN_MSK , setup->afe); +unsigned int pga; +int ret =0 ; + +for (pga =0 ; pga <AD4170_NUM_PGA_OPTIONS ; pga++) { +if (val == chan_info->scale_tbl[pga][0 ] && + val2 == chan_info->scale_tbl[pga][1]) +break ;
drivers/iio/adc/ad4170.c line 1339
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ } + +if (pga ==AD4170_NUM_PGA_OPTIONS ) +return -EINVAL ; + +if (pga == old_pga) +return 0 ; + + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK ; + setup->afe |=FIELD_PREP (AD4170_AFE_PGA_GAIN_MSK , pga);
ditto...
Quoted 37 lines
drivers/iio/adc/ad4170.c new file +37
-0+ +guard (mutex)(&st->lock); + ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) { + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK ; + setup->afe |=FIELD_PREP (AD4170_AFE_PGA_GAIN_MSK , old_pga); + } + +return ret; +} + +static int ad4170_set_channel_freq (struct ad4170_state *st, + struct iio_chan_spec const *chan, int val, + int val2) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +enum ad4170_filter_type f_type =__ad4170_get_filter_type (setup->filter); +int filt_fs_tbl_size, i, ret =0 ; +unsigned int old_filter_fs; + +switch (f_type) { +case AD4170_SINC5_AVG : + fallthrough; +case AD4170_SINC3 : + filt_fs_tbl_size =ARRAY_SIZE (ad4170_sinc3_filt_fs_tbl); +break ; +case AD4170_SINC5 : + filt_fs_tbl_size =ARRAY_SIZE (ad4170_sinc5_filt_fs_tbl); +break ; + } + +for (i =0 ; i < filt_fs_tbl_size; i++) { +if (st->sps_tbl[f_type][i][0 ] == val && + st->sps_tbl[f_type][i][1] == val2) +break ; + }
drivers/iio/adc/ad4170.c line 1386
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+if (i >= filt_fs_tbl_size) +return -EINVAL ; + + old_filter_fs = setup->filter_fs; +if (f_type ==AD4170_SINC5 ) + setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i]; +else + setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i]; + +guard (mutex)(&st->lock);
Shouldn't the lock also protect the 'setup' struct?
Quoted 121 lines
drivers/iio/adc/ad4170.c new file +121
-0+ ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) + setup->filter_fs = old_filter_fs; + +return ret; +} + +static int ad4170_set_calib_offset (struct ad4170_state *st, + struct iio_chan_spec const *chan, int val) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +u32 old_offset; +int ret; + + old_offset = setup->offset; + setup->offset = val; + +guard (mutex)(&st->lock); + ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) + setup->offset = old_offset; + +return ret; +} + +static int ad4170_set_calib_gain (struct ad4170_state *st, +struct iio_chan_spec const *chan,int val) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +u32 old_gain; +int ret; + + old_gain = setup->gain; + setup->gain = val; + +guard (mutex)(&st->lock); + ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) + setup->gain = old_gain; + +return ret; +} + +static int __ad4170_write_raw (struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long info) +{ +struct ad4170_state *st =iio_priv (indio_dev); + +switch (info) { +case IIO_CHAN_INFO_SCALE : +return ad4170_set_pga (st, chan, val, val2); +case IIO_CHAN_INFO_SAMP_FREQ : +return ad4170_set_channel_freq (st, chan, val, val2); +case IIO_CHAN_INFO_CALIBBIAS : +return ad4170_set_calib_offset (st, chan, val); +case IIO_CHAN_INFO_CALIBSCALE : +return ad4170_set_calib_gain (st, chan, val); +default : +return -EINVAL ; + } +} + +static int ad4170_write_raw (struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long info) +{ +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =__ad4170_write_raw (indio_dev, chan, val, val2, info); +iio_device_release_direct (indio_dev); +return ret; +} + +static int ad4170_write_raw_get_fmt (struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + long info) +{ +switch (info) { +case IIO_CHAN_INFO_SCALE : +return IIO_VAL_INT_PLUS_NANO ; +case IIO_CHAN_INFO_SAMP_FREQ : +return IIO_VAL_INT_PLUS_MICRO ; +case IIO_CHAN_INFO_CALIBBIAS : +case IIO_CHAN_INFO_CALIBSCALE : +return IIO_VAL_INT ; +default : +return -EINVAL ; + } +} + +static const struct iio_info ad4170_info = { + .read_raw = ad4170_read_raw, + .read_avail = ad4170_read_avail, + .write_raw = ad4170_write_raw, + .write_raw_get_fmt = ad4170_write_raw_get_fmt, + .debugfs_reg_access = ad4170_debugfs_reg_access, +}; + +static int ad4170_soft_reset (struct ad4170_state *st) +{ +int ret; + + ret =regmap_write (st->regmap8,AD4170_CONFIG_A_REG , + AD4170_SW_RESET_MSK); +if (ret) +return ret; + +/* AD4170-4 requires 1 ms between reset and any register access. */ +fsleep (MILLI ); + +return 0 ; +} + +static int ad4170_parse_reference(struct ad4170_state *st, + struct fwnode_handle *child,
drivers/iio/adc/ad4170.c line 1517
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ struct ad4170_setup *setup) +{ +struct device *dev = &st->spi->dev; +int ret; +u8 aux; + +/* Positive reference buffer setup */ + aux =AD4170_REF_BUF_PRE ;/* Default to have precharge buffer enabled. */ + ret =fwnode_property_read_u8 (child,"adi,buffered-positive" , &aux); +if (ret) {
Shouldn't this be if (!ret)?
drivers/iio/adc/ad4170.c line 1528
Quoted 12 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (aux <AD4170_REF_BUF_PRE || aux >AD4170_REF_BUF_BYPASS ) + return dev_err_probe(dev, -EINVAL, + "Invalid adi,buffered-positive:%u\n",
drivers/iio/adc/ad4170.c new file +8
-0+ aux); + } + setup->afe |=FIELD_PREP (AD4170_AFE_REF_BUF_P_MSK , aux); + +/* Negative reference buffer setup */ + aux =AD4170_REF_BUF_PRE ;/* Default to have precharge buffer enabled. */ + ret =fwnode_property_read_u8 (child,"adi,buffered-negative" , &aux); +if (ret) {
ditto
Quoted 45 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (aux <AD4170_REF_BUF_PRE || aux >AD4170_REF_BUF_BYPASS ) + return dev_err_probe(dev, -EINVAL, + "Invalid adi,buffered-negative:%u\n",
drivers/iio/adc/ad4170.c new file +40
-0+ aux); + } + setup->afe |=FIELD_PREP (AD4170_AFE_REF_BUF_M_MSK , aux); + +/* Voltage reference selection */ + aux =AD4170_REF_REFOUT ;/* Default reference selection. */ +fwnode_property_read_u8 (child,"adi,reference-select" , &aux); +if (aux >AD4170_REF_AVDD ) +return dev_err_probe (dev, -EINVAL , + "Invalid reference selected %u\n", aux); + setup->afe |=FIELD_PREP (AD4170_AFE_REF_SELECT_MSK , aux); + +return 0 ; +} + +static int ad4170_parse_adc_channel_type (struct device *dev, +struct fwnode_handle *child, +struct iio_chan_spec *chan) +{ +u32 pins[2 ]; +int ret; + + ret =fwnode_property_read_u32_array (child,"diff-channels" , pins, + ARRAY_SIZE(pins)); +if (!ret) { + chan->differential = true; + chan->channel = pins[0 ]; + chan->channel2 = pins[1 ]; +return 0 ; + } + ret =fwnode_property_read_u32 (child,"single-channel" , &pins[0 ]); +if (!ret) { + chan->differential = false; + chan->channel = pins[0 ]; + + ret =fwnode_property_read_u32 (child,"common-mode-channel" , + &pins[1]); +if (ret) + return dev_err_probe(dev, ret, + "single-ended channels must define common-mode-channel\n");
drivers/iio/adc/ad4170.c line 1561
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +4
-0+ + chan->channel2 = pins[1 ]; +return 0 ; + }
Kind of a nitpick but for the above I would flip the logic. First check for errors in the single-channel case and then you can have one less of level of indentation...
Quoted 12 lines
drivers/iio/adc/ad4170.c new file +12
-0+return dev_err_probe (dev, ret, +"Channel must define one of diff-channels or single-channel.\n" ); +} + +static int ad4170_parse_channel_node (struct iio_dev *indio_dev, + struct fwnode_handle *child, + unsigned int chan_num) +{ +struct ad4170_state *st =iio_priv (indio_dev); +struct device *dev = &st->spi->dev; +struct ad4170_chan_info *chan_info; +struct ad4170_setup *setup;
drivers/iio/adc/ad4170.c line 1597
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+struct iio_chan_spec *chan; +unsigned int ch_reg; +u8 ref_select; +bool bipolar; +int ret; + + ret =fwnode_property_read_u32 (child,"reg" , &ch_reg); +if (ret) +return ret; +
Could also deserve a log message?
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (ch_reg >=AD4170_MAX_CHANNELS ) +return dev_err_probe (dev, -EINVAL , + "Channel idx greater than no of channels\n");
...
Quoted 15 lines
drivers/iio/adc/ad4170.c new file +14
-0+ +static int ad4170_trigger_setup (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + + st->trig =devm_iio_trigger_alloc (indio_dev->dev.parent,"%s-trig%d" , + indio_dev->name, + iio_device_id(indio_dev)); +if (!st->trig) +return -ENOMEM ; + + st->trig->ops = &ad4170_trigger_ops; + st->trig->dev.parent = indio_dev->dev.parent;
drivers/iio/adc/ad4170.c line 1798
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ +iio_trigger_set_drvdata (st->trig, indio_dev); + ret =devm_iio_trigger_register (indio_dev->dev.parent, st->trig); +if (ret) +return ret; + + indio_dev->trig =iio_trigger_get (st->trig); + +return request_irq (st->spi->irq, &ad4170_irq_handler,IRQF_ONESHOT , + indio_dev->name, indio_dev);
devm_request_irq()...
Quoted 30 lines
drivers/iio/adc/ad4170.c new file +30
-0+} + +static int ad4170_regulator_setup (struct ad4170_state *st) +{ +struct device *dev = &st->spi->dev; +int ret; + +/* Required regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get AVDD voltage.\n" ); + + st->vrefs_uv[AD4170_AVDD_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"iovdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get IOVDD voltage.\n" ); + + st->vrefs_uv[AD4170_IOVDD_SUP ] = ret; + +/* Optional regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avss" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get AVSS voltage.\n" ); + +/* Assume AVSS at GND (0V) if not provided */ + st->vrefs_uv[AD4170_AVSS_SUP ] = ret == -ENODEV ?0 : -ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1p" ); +if (ret <0 && ret != -ENODEV )
drivers/iio/adc/ad4170.c line 1838
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+return dev_err_probe (dev, ret,"Failed to get REFIN+ voltage.\n" ); + + st->vrefs_uv[AD4170_REFIN1P_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1n" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN1N_SUP ] = ret == -ENODEV ? -ENODEV : -ret;
Maybe to early for me but the comment does not make it clear to me why the negation? Won't the regulator return a negative voltage?
Quoted 23 lines
drivers/iio/adc/ad4170.c new file +4
-0+ + ret =devm_regulator_get_enable_read_voltage (dev,"refin2p" ); +if (ret <0 && ret != -ENODEV ) + return dev_err_probe(dev, ret, "Failed to get REFIN2+voltage.\n");
drivers/iio/adc/ad4170.c new file +6
-0+ + st->vrefs_uv[AD4170_REFIN2P_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin2n" ); +if (ret <0 && ret != -ENODEV ) + return dev_err_probe(dev, ret, "Failed to get REFIN2-voltage.\n");
drivers/iio/adc/ad4170.c new file +11
-0+ +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN2N_SUP ] = ret == -ENODEV ? -ENODEV : -ret; + +return 0 ; +} + +static int ad4170_probe (struct spi_device *spi) +{ +struct device *dev = &spi->dev; +struct iio_dev *indio_dev;
drivers/iio/adc/ad4170.c line 1869
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+struct ad4170_state *st; +const char *dev_name; +int ret; + + indio_dev =devm_iio_device_alloc (&spi->dev,sizeof (*st)); +if (!indio_dev) +return -ENOMEM ; + + st =iio_priv (indio_dev); +devm_mutex_init (dev, &st->lock);
check for errors...
Quoted 14 lines
drivers/iio/adc/ad4170.c new file +14
-0+ + dev_name =spi_get_device_match_data (spi); +if (!dev_name) +return -EINVAL ; + + indio_dev->name = dev_name; + indio_dev->info = &ad4170_info; + + st->spi = spi; + st->regmap8 =devm_regmap_init_spi (spi, &ad4170_regmap8_config); +if (IS_ERR (st->regmap8)) +return dev_err_probe (dev,PTR_ERR (st->regmap8), + "Failed to initialize regmap8\n"); +
drivers/iio/adc/ad4170.c line 1893
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ st->regmap16 =devm_regmap_init_spi (spi, &ad4170_regmap16_config); +if (IS_ERR (st->regmap16)) +return dev_err_probe (dev,PTR_ERR (st->regmap16), + "Failed to initialize regmap16\n"); + + st->regmap24 =devm_regmap_init_spi (spi, &ad4170_regmap24_config); +if (IS_ERR (st->regmap24)) +return dev_err_probe (dev,PTR_ERR (st->regmap24), + "Failed to initialize regmap24\n"); +
Hmm, interesting idea... but I would expect an explanation on why can't we have bulk reads for the 16 and 24 bit cases? Without it, I have to ask why not?
Quoted 22 lines
drivers/iio/adc/ad4170.c new file +22
-0+ ret =ad4170_regulator_setup (st); +if (ret) +return ret; + + ret =ad4170_soft_reset (st); +if (ret) +return ret; + + ret =ad4170_parse_firmware (indio_dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to parse firmware\n" ); + + ret =ad4170_initial_config (indio_dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to setup device\n" ); + +init_completion (&st->completion); + +if (spi->irq) { + ret =ad4170_trigger_setup (indio_dev); +if (ret) + return dev_err_probe(dev, ret, "Failed to setup
Quoted 1 line
trigger\n");
Typically it's better to log the errors inside ad4170_trigger_setup() unless you use it outside probe.
- Nuno Sá
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +3
-0+MODULE_AUTHOR ("Marcelo Schmitt <[email protected]>" ); +MODULE_DESCRIPTION ("Analog Devices AD4170 SPI driver" ); +MODULE_LICENSE ("GPL" );
Re: [PATCH v1 3/7] iio: adc: ad4170: Add support for buffered data capture
In reply to Marcelo SchmittQuoted 63 lines · Marcelo Schmitt
Extend the AD4170 driver to allow buffered data capture in continuous read mode. In continuous read mode, the chip skips the instruction phase and outputs just ADC sample data, enabling faster sample rates to be reached. The internal channel sequencer always starts sampling from channel 0 and channel 0 must be enabled if more than one channel is selected for data capture. The scan mask validation callback checks the aforementioned condition is met.
Signed-off-by Marcelo Schmitt <[email protected]>---
drivers/iio/adc/ad4170.c | 170 ++++++++++++++++++++++++++++++++++++++-1 file changed, 169 insertions(+), 1 deletion(-)drivers/iio/adc/ad4170.c +2
-0@@ -10,10 +10,12 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/iio/buffer.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> #include <linux/iio/trigger.h> #include <linux/iio/trigger_consumer.h> +#include <linux/iio/triggered_buffer.h> #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/kernel.h>@@ -323,6 +325,16 @@ struct ad4170_state {int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2]; struct completion completion;
structiio_trigger*trig;drivers/iio/adc/ad4170.c +6
-0+ +struct spi_transfer xfer; +struct spi_message msg; +__be32 bounce_buffer[AD4170_MAX_CHANNELS ]; +/* +* DMA (thus cache coherency maintenance) requires the transfer buffers
drivers/iio/adc/ad4170.c +4
-0+* to live in their own cache lines. +*/ +__be32 rx_buf__aligned(IIO_DMA_MINALIGN) ; +u8 tx_buf[2 ];};
static voidad4170_fill_sps_tbl(struct ad4170_state *st)drivers/iio/adc/ad4170.c +1
-0@@ -882,6 +894,7 @@ static const struct iio_chan_spec ad4170_channel_template = { .scan_type = { .realbits = 24, .storagebits = 32, + .shift = 8, .endianness = IIO_BE, },};
drivers/iio/adc/ad4170.c +0
-0@@ -1480,11 +1493,29 @@ static int ad4170_write_raw_get_fmt(struct iio_dev *indio_dev, } }
drivers/iio/adc/ad4170.c static int ad4170_write_raw_get_fmt(struct iio_dev *indio_dev, · line 1504
Quoted 9 lines
drivers/iio/adc/ad4170.c +9
-0+static int ad4170_update_scan_mode (struct iio_dev *indio_dev, + const unsigned long *active_scan_mask) +{ +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int chan_index; +int ret; + +guard (mutex)(&st->lock); +
Do we need this? In theory we should be fine since you do iio_device_claim_direct() in ad4170_read_sample().
Otherwise I would expect the same locking rules in postenable() and predisable().
Quoted 65 lines
drivers/iio/adc/ad4170.c +8
-0+iio_for_each_active_channel (indio_dev, chan_index) { + ret =ad4170_set_channel_enable (st, chan_index, true); +if (ret) +return ret; + } +return 0 ; +} +static const struct
iio_infoad4170_info= { .read_raw =ad4170_read_raw, .read_avail =ad4170_read_avail, .write_raw =ad4170_write_raw, .write_raw_get_fmt =ad4170_write_raw_get_fmt,drivers/iio/adc/ad4170.c +1
-0+ .update_scan_mode = ad4170_update_scan_mode,.debugfs_reg_access =
ad4170_debugfs_reg_access, };drivers/iio/adc/ad4170.c +0
-0@@ -1759,6 +1790,130 @@ static int ad4170_initial_config(struct iio_dev *indio_dev) AD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSK); }drivers/iio/adc/ad4170.c +6
-0+static int ad4170_prepare_spi_message (struct ad4170_state *st) +{ +/* +* Continuous data register read is enabled on buffer postenable so +* no instruction phase is needed meaning we don't need to send the +* register address to read data. Transfer only needs the read buffer.
drivers/iio/adc/ad4170.c +3
-0+*/ + st->xfer.rx_buf = &st->rx_buf; + st->xfer.len =
BITS_TO_BYTES(ad4170_channel_template.scan_type.realbits);drivers/iio/adc/ad4170.c +18
-0+ +spi_message_init_with_transfers (&st->msg, &st->xfer,1 ); + +return devm_spi_optimize_message (&st->spi->dev, st->spi, &st->msg); +} + +static int ad4170_buffer_postenable (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + + ret =ad4170_set_mode (st,AD4170_ADC_CTRL_MODE_CONT ); +if (ret <0 ) +return ret; + +/* +* Enables continuous data register read. +* This enables continuous read of the ADC Data register. The ADC must
drivers/iio/adc/ad4170.c +6
-0+* be in a continuous conversion mode. +*/ + return regmap_update_bits(st->regmap16, AD4170_ADC_CTRL_REG, + AD4170_ADC_CTRL_CONT_READ_MSK, + FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK, +
AD4170_ADC_CTRL_CONT_READ_ENABLE));drivers/iio/adc/ad4170.c +6
-0+} + +static int ad4170_buffer_predisable (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret, i;
drivers/iio/adc/ad4170.c static int ad4170_initial_config(struct iio_dev *indio_dev) · line 1841
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+ +/* +* To exit continuous read, write 0xA5 to the ADC during the first 8 +* SCLKs of the ADC data read. +*/ + st->tx_buf[0 ] =AD4170_ADC_CTRL_CONT_READ_EXIT ; + st->tx_buf[1 ] =0 ; + ret =spi_write (st->spi, st->tx_buf,2 ); +if (ret) +return ret;
Couldn't we still use regmap? Like regmap_write(st->regmap8, AD4170_ADC_CTRL_CONT_READ_EXIT, 0)?
Likely fails on the register ranges and not sure it's any better... Still not a fan of mixing "raw" bus read/writes and regmap().
Quoted 42 lines
drivers/iio/adc/ad4170.c +5
-0+ + ret = regmap_update_bits(st->regmap16, AD4170_ADC_CTRL_REG, + AD4170_ADC_CTRL_CONT_READ_MSK, + FIELD_PREP(AD4170_ADC_CTRL_CONT_READ_MSK, +
AD4170_ADC_CTRL_CONT_READ_DISABLE));drivers/iio/adc/ad4170.c +11
-0+if (ret) +return ret; + + ret = ad4170_set_mode(st, AD4170_ADC_CTRL_MODE_IDLE); +if (ret) +return ret; + +/* +* The ADC sequences through all the enabled channels (see datasheet +* page 95). That can lead to incorrect channel being read if a +* single-shot read (or buffered read with different
active_scan_mask)drivers/iio/adc/ad4170.c +1
-0+* is done after buffer disable. Disable all channels so only requested
drivers/iio/adc/ad4170.c +21
-0+* channels will be read. +*/ +for (i =0 ; i < indio_dev->num_channels; i++) { + ret =ad4170_set_channel_enable (st, i, false); +if (ret) +return ret; + } +return ret; +} + +static bool ad4170_validate_scan_mask (struct iio_dev *indio_dev, + const unsigned long *scan_mask) +{ +unsigned int masklength =iio_get_masklength (indio_dev); +unsigned long first, next; + +/* +* The channel sequencer cycles through the enabled channels in +* sequential order, from channel 0 to channel 15, bypassing disabled +* channels. When more than one channel is enabled, channel 0 must +* always be enabled. See datasheet channel_en register description at
drivers/iio/adc/ad4170.c static int ad4170_initial_config(struct iio_dev *indio_dev) · line 1885
Quoted 6 lines
drivers/iio/adc/ad4170.c +6
-0+* page 95. +*/ + first =find_next_bit (scan_mask, masklength,0 ); + next =find_next_bit (scan_mask, masklength, first +1 ); +if (next < masklength) +return test_bit (0 , scan_mask);
Hmm, maybe this is simpler?
if (bitmap_weight(scan_mask, masklength) > 1) return test_bit(0, scan_mask); return true;
- Nuno Sá
Re: [PATCH v1 4/7] iio: adc: ad4170: Add clock provider support
In reply to Marcelo SchmittQuoted 8 lines · Marcelo Schmitt
The AD4170 chip can use an externally supplied clock at the XTAL2 pin, or an external crystal connected to the XTAL1 and XTAL2 pins. Alternatively, the AD4170 can provide it's 16 MHz internal clock at the XTAL2 pin. Extend the AD4170 driver so it effectively uses the provided external clock, if any, or supplies it's own clock as a clock provider.
Signed-off-by Marcelo Schmitt <[email protected]>---
Just one minor note, with it:
Quoted 127 lines
drivers/iio/adc/ad4170.c | 135 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 134 insertions(+), 1 deletion(-)drivers/iio/adc/ad4170.c +0
-0@@ -7,6 +7,8 @@
#include <linux/bitfield.h>
#include <linux/bitops.h>drivers/iio/adc/ad4170.c +2
-0+#include <linux/clk.h> +#include <linux/clk-provider.h> #include <
linux/delay.h>
#include <linux/device.h>
#include <linux/err.h>drivers/iio/adc/ad4170.c +1
-0@@ -62,6 +64,7 @@ #define AD4170_DATA_16B_STATUS_REG 0x1A #define AD4170_DATA_24B_REG 0x1E #define AD4170_PIN_MUXING_REG 0x69 +#define AD4170_CLOCK_CTRL_REG 0x6B #define AD4170_ADC_CTRL_REG 0x71 #define AD4170_CHAN_EN_REG 0x79 #define AD4170_CHAN_SETUP_REG(x) (0x81 + 4 * (x))@@ -89,6 +92,9 @@ #define AD4170_PIN_MUXING_DIG_AUX1_CTRL_MSK GENMASK(5, 4) #define AD4170_PIN_MUXING_SYNC_CTRL_MSK GENMASK(3, 2)drivers/iio/adc/ad4170.c +3
-0+/* AD4170_CLOCK_CTRL_REG */ +#define AD4170_CLOCK_CTRL_CLOCKSEL_MSK GENMASK(1, 0) +/*
AD4170_ADC_CTRL_REG*/
#defineAD4170_ADC_CTRL_MULTI_DATA_REG_SEL_MSKBIT(7)
#defineAD4170_ADC_CTRL_CONT_READ_MSKGENMASK(5, 4)drivers/iio/adc/ad4170.c +0
-0@@ -121,6 +127,12 @@
/* AD4170 register constants */drivers/iio/adc/ad4170.c +6
-0+/* AD4170_CLOCK_CTRL_REG constants */ +#define AD4170_CLOCK_CTRL_CLOCKSEL_INT 0x0 +#define AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT 0x1 +#define AD4170_CLOCK_CTRL_CLOCKSEL_EXT 0x2 +#define AD4170_CLOCK_CTRL_CLOCKSEL_EXT_XTAL 0x3 +/*
AD4170_CHAN_MAP_REGconstants */
#defineAD4170_CHAN_MAP_AIN00
#defineAD4170_CHAN_MAP_AIN11drivers/iio/adc/ad4170.c +0
-0@@ -238,6 +250,10 @@ enum ad4170_regulator {
AD4170_MAX_SUP
};drivers/iio/adc/ad4170.c +4
-0+static const char *const ad4170_clk_sel[] = { +"ext-clk" ,"xtal" +}; +enum
ad4170_int_pin_sel{
AD4170_INT_PIN_SDO,
AD4170_INT_PIN_DIG_AUX1,drivers/iio/adc/ad4170.c +3
-0@@ -320,6 +336,9 @@ struct ad4170_state { struct ad4170_chan_info chan_infos[AD4170_MAX_CHANNELS]; struct ad4170_setup_info setup_infos[AD4170_MAX_SETUPS]; u32 mclk_hz; +unsigned int clock_ctrl; +struct clk *ext_clk; +struct clk_hw int_clk_hw; int pins_fn[AD4170_NUM_ANALOG_PINS]; u32 int_pin_sel;int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
drivers/iio/adc/ad4170.c +0
-0@@ -1693,13 +1712,127 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev) return 0; }drivers/iio/adc/ad4170.c +10
-0+static struct ad4170_state *clk_hw_to_ad4170 (struct clk_hw *hw) +{ + return container_of(hw, struct ad4170_state, int_clk_hw); +} + +static unsigned long ad4170_sel_clk (struct ad4170_state *st, + unsigned int clk_sel) +{ + st->clock_ctrl &= ~AD4170_CLOCK_CTRL_CLOCKSEL_MSK ; + st->clock_ctrl |= FIELD_PREP(AD4170_CLOCK_CTRL_CLOCKSEL_MSK,
clk_sel);drivers/iio/adc/ad4170.c +1
-0+ return regmap_write(st->regmap16, AD4170_CLOCK_CTRL_REG, st-Quoted 1 line
clock_ctrl);drivers/iio/adc/ad4170.c +40
-0+} + +static unsigned long ad4170_clk_recalc_rate (struct clk_hw *hw, + unsigned long parent_rate) +{ +return AD4170_INT_CLOCK_16MHZ ; +} + +static int ad4170_clk_output_is_enabled (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); +u32 clk_sel; + + clk_sel =FIELD_GET (AD4170_CLOCK_CTRL_CLOCKSEL_MSK , st->clock_ctrl); +return clk_sel ==AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT ; +} + +static int ad4170_clk_output_prepare (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); + +return ad4170_sel_clk (st,AD4170_CLOCK_CTRL_CLOCKSEL_INT_OUT ); +} + +static void ad4170_clk_output_unprepare (struct clk_hw *hw) +{ +struct ad4170_state *st =clk_hw_to_ad4170 (hw); + +ad4170_sel_clk (st,AD4170_CLOCK_CTRL_CLOCKSEL_INT ); +} + +static const struct clk_ops ad4170_int_clk_ops = { + .recalc_rate = ad4170_clk_recalc_rate, + .is_enabled = ad4170_clk_output_is_enabled, + .prepare = ad4170_clk_output_prepare, + .unprepare = ad4170_clk_output_unprepare, +}; + +static int ad4170_register_clk_provider (struct iio_dev *indio_dev) +{
drivers/iio/adc/ad4170.c static int ad4170_parse_channels(struct iio_dev *indio_dev) · line 1775
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+struct ad4170_state *st =iio_priv (indio_dev); +struct device *dev = indio_dev->dev.parent; +struct fwnode_handle *fwnode =dev_fwnode (dev); +struct clk_init_data init = {}; +int ret; + +if (!IS_ENABLED (CONFIG_COMMON_CLK )) +return 0 ; + + init.name =fwnode_get_name (fwnode);
Maybe allow for clock-output-names? See:
https://elixir.bootlin.com/linux/v6.13.7/source/drivers/iio/frequency/adf4350.c#L467
- Nuno Sá
Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support
In reply to Marcelo SchmittQuoted 11 lines · Marcelo Schmitt
The AD4170 has four multifunctional pins that can be used as GPIOs. The GPIO functionality can be accessed when the AD4170 chip is not busy performing continuous data capture or handling any other register read/write request. Also, the AD4170 does not provide any interrupt based on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
Implement
gpio_chipcallbacks so to make AD4170 GPIO pins controllable through the gpiochip interface.Signed-off-by Marcelo Schmitt <[email protected]>---
Just some doubts, see below...
Quoted 84 lines
drivers/iio/adc/ad4170.c | 167 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 166 insertions(+), 1 deletion(-)drivers/iio/adc/ad4170.c +2
-0@@ -12,6 +12,7 @@ #include <linux/delay.h> #include <linux/device.h> #include <linux/err.h> +#include <linux/gpio/driver.h> #include <linux/iio/buffer.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h>@@ -79,6 +80,7 @@ #define AD4170_FIR_CTRL 0x141 #define AD4170_COEFF_DATA_REG 0x14A #define AD4170_COEFF_ADDR_REG 0x14C +#define AD4170_GPIO_MODE_REG 0x191 #define AD4170_GPIO_OUTPUT_REG 0x193 #define AD4170_GPIO_INPUT_REG 0x195drivers/iio/adc/ad4170.c +0
-0@@ -189,6 +191,7 @@ /* Device properties and auxiliary constants */
#defineAD4170_NUM_ANALOG_PINS9
+#defineAD4170_NUM_GPIO_PINS4
#defineAD4170_MAX_CHANNELS16
#defineAD4170_MAX_ANALOG_PINS8
#defineAD4170_MAX_SETUPS8drivers/iio/adc/ad4170.c +1
-0@@ -340,6 +343,7 @@ struct ad4170_state { struct clk *ext_clk; struct clk_hw int_clk_hw; int pins_fn[AD4170_NUM_ANALOG_PINS]; +struct gpio_chip gpiochip; u32 int_pin_sel;int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2]; struct completion completion;
drivers/iio/adc/ad4170.c +0
-0@@ -1553,6 +1557,156 @@ static int ad4170_soft_reset(struct ad4170_state *st) return 0; }drivers/iio/adc/ad4170.c +23
-0+static int ad4170_gpio_get (struct gpio_chip *gc,unsigned int offset) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_read (st->regmap16,AD4170_GPIO_MODE_REG , &val); +if (ret) + goto err_release; + +/* +* If the GPIO is configured as an input, read the current value from +* AD4170_GPIO_INPUT_REG. Otherwise, read the input value from +* AD4170_GPIO_OUTPUT_REG. +*/ +if (val &BIT (offset *2 )) + ret =regmap_read (st->regmap16,AD4170_GPIO_INPUT_REG , &val); +else + ret = regmap_read(st->regmap16, AD4170_GPIO_OUTPUT_REG,&val);
drivers/iio/adc/ad4170.c +11
-0+if (ret) + goto err_release; + + ret = !!(val &BIT (offset)); +err_release: +iio_device_release_direct (indio_dev); + +return ret; +} + +static int ad4170_gpio_set(struct gpio_chip *gc, unsigned int offset, intvalue)
drivers/iio/adc/ad4170.c +4
-0+{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val;
drivers/iio/adc/ad4170.c static int ad4170_soft_reset(struct ad4170_state *st) · line 1607
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_read (st->regmap16,AD4170_GPIO_MODE_REG , &val); +if (ret) + goto err_release; + +if (val &BIT (offset *2 +1 ))
Why do we need this? Are we checking if it's a GPO? If so, we should return -EPERM in case we have a GPI?
Quoted 11 lines
drivers/iio/adc/ad4170.c +1
-0+ ret = regmap_update_bits(st->regmap16,
AD4170_GPIO_OUTPUT_REG,drivers/iio/adc/ad4170.c +8
-0+ BIT(offset), value << offset); + +err_release: +iio_device_release_direct (indio_dev); +return ret; +} + +static int ad4170_gpio_get_direction(struct gpio_chip *gc, unsigned intoffset)
drivers/iio/adc/ad4170.c static int ad4170_soft_reset(struct ad4170_state *st) · line 1569
Quoted 9 lines
drivers/iio/adc/ad4170.c +9
-0+{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; +
This claim_direct() makes me wonder if there's any overlap between the GPIO func and normal readings? Like, imagine a consumer requests a gpio and no buffering is happening so all is good. However, there's nothing stopping us for enabling buffering afterwards, right? Wouldn't that be an issue? If there are shared pins, I can see this also being an issue even for single shot reading... Otherwise, I wonder why we have this iio_device_claim_direct() calls? Is it just for using the internal IIO lock?
At this point, I did not checked the datasheet so I can be completely misunderstanding things...
- Nuno Sá
Re: [PATCH v1 6/7] iio: adc: ad4170: Add support for internal temperature sensor
In reply to Marcelo SchmittQuoted 6 lines · Marcelo Schmitt
The AD4170 has an internal temperature sensor that can be read using the ADC. Whenever possible, configure an IIO channel to provide the chip's temperature.
Signed-off-by Marcelo Schmitt <[email protected]>---
One minor nit... Otherwise looks good:
Quoted 60 lines
drivers/iio/adc/ad4170.c | 72 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 3 deletions(-)drivers/iio/adc/ad4170.c +0
-0@@ -922,6 +922,27 @@ static const struct iio_chan_spec ad4170_channel_template = { }, };drivers/iio/adc/ad4170.c +21
-0+static const struct iio_chan_spec ad4170_temp_channel_template = { + .type =IIO_TEMP , + .indexed =0 , + .channel =17 , + .channel2 =17 , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | + BIT(IIO_CHAN_INFO_SCALE) | + BIT(IIO_CHAN_INFO_OFFSET) | + BIT(IIO_CHAN_INFO_CALIBSCALE) | + BIT(IIO_CHAN_INFO_CALIBBIAS) | + BIT(IIO_CHAN_INFO_SAMP_FREQ), + .info_mask_separate_available =BIT (IIO_CHAN_INFO_SAMP_FREQ ), + .scan_type = { + .sign ='s' , + .realbits =24 , + .storagebits =32 , + .shift =8 , + .endianness =IIO_BE , + }, +}; +/*
* Receives the number of a multiplexed AD4170 input (ain_n), and stores the
* voltage (in µV) of the specified input intoain_voltage. If the input numberdrivers/iio/adc/ad4170.c +0
-0@@ -1209,9 +1230,27 @@ static int ad4170_read_raw(struct iio_dev *indio_dev, return ret;case
IIO_CHAN_INFO_SCALE:
pga =FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe);drivers/iio/adc/ad4170.c +10
-3- *val = chan_info->scale_tbl[pga][0 ]; - *val2 = chan_info->scale_tbl[pga][1 ]; -return IIO_VAL_INT_PLUS_NANO ; +switch (chan->type) { +case IIO_VOLTAGE : + *val = chan_info->scale_tbl[pga][0 ]; + *val2 = chan_info->scale_tbl[pga][1 ]; +return IIO_VAL_INT_PLUS_NANO ; + +case IIO_TEMP : +/* +* The scale_tbl converts output codes to mV units so +* multiply by MILLI to make the factor convert to µV.
drivers/iio/adc/ad4170.c +1
-0+* Then, apply the temperature sensor change sensitivity
drivers/iio/adc/ad4170.c +2
-0+* of 477 μV/K. Finally, multiply the result by MILLI +* again to comply with milli degrees Celsius IIO ABI.
drivers/iio/adc/ad4170.c static int ad4170_read_raw(struct iio_dev *indio_dev, · line 1247
Quoted 2 lines
drivers/iio/adc/ad4170.c +2
-0+*/ + *val =0 ;/* The scale integer part is always 0. */
Hmm this comment does not add much...
Quoted 54 lines
drivers/iio/adc/ad4170.c +1
-0+ *val2 = DIV_ROUND_CLOSEST(chan_info-Quoted 1 line
scale_tbl[pga][1] * MILLI,
drivers/iio/adc/ad4170.c +5
-0+ 477) * MILLI; +return IIO_VAL_INT_PLUS_NANO ; +default : +return -EINVAL ; + }case
IIO_CHAN_INFO_OFFSET:
pga =FIELD_GET(AD4170_AFE_PGA_GAIN_MSK, setup->afe); *val = chan_info->offset_tbl[pga];drivers/iio/adc/ad4170.c +0
-0@@ -1855,12 +1894,39 @@ static int ad4170_parse_channels(struct iio_dev *indio_dev) if (num_channels > AD4170_MAX_CHANNELS) return dev_err_probe(dev, -EINVAL, "Too many channels\n");drivers/iio/adc/ad4170.c +3
-0+/* Add one for temperature */ + num_channels =min (num_channels +1 ,AD4170_MAX_CHANNELS ); +
device_for_each_child_node_scoped(dev, child){ ret =ad4170_parse_channel_node(indio_dev, child, chan_num++);
if (ret)
return ret;
}drivers/iio/adc/ad4170.c +18
-0+/* +* Add internal temperature sensor channel if the maximum number of +* channels has not been reached. +*/ +if (num_channels <AD4170_MAX_CHANNELS ) { +struct ad4170_setup *setup = &st->chan_infos[chan_num].setup; + + st->chans[chan_num] = ad4170_temp_channel_template; + st->chans[chan_num].address = chan_num; + st->chans[chan_num].scan_index = chan_num; + + st->chan_infos[chan_num].setup_num =AD4170_INVALID_SETUP ; + st->chan_infos[chan_num].initialized = true; + + setup->afe |=FIELD_PREP (AD4170_AFE_REF_SELECT_MSK , +AD4170_REF_AVDD ); + + ret = ad4170_get_input_range(st, &st->chans[chan_num],
chan_num,drivers/iio/adc/ad4170.c +3
-0+ AD4170_REF_AVDD); +if (ret <0 ) + return dev_err_probe(dev, ret, "Invalid inputconfig\n");
drivers/iio/adc/ad4170.c +3
-0+ + st->chan_infos[chan_num].input_range_uv = ret; + }
indio_dev->num_channels=num_channels;
indio_dev->channels=st->chans;
return 0;
Re: [PATCH v1 7/7] iio: adc: ad4170: Add support for weigh scale and RTD sensors
In reply to Marcelo SchmittQuoted 59 lines · Marcelo Schmitt
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 <[email protected]>---
drivers/iio/adc/ad4170.c | 341 ++++++++++++++++++++++++++++++++++++++-1 file changed, 338 insertions(+), 3 deletions(-)drivers/iio/adc/ad4170.c +1
-0@@ -77,6 +77,7 @@ #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_FIR_CTRL 0x141 #define AD4170_COEFF_DATA_REG 0x14A #define AD4170_COEFF_ADDR_REG 0x14C@@ -127,6 +128,10 @@ /* AD4170_FILTER_REG */ #define AD4170_FILTER_FILTER_TYPE_MSK GENMASK(3, 0)drivers/iio/adc/ad4170.c +4
-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_REGconstants */drivers/iio/adc/ad4170.c +0
-0@@ -188,6 +193,21 @@ #define AD4170_FILTER_FILTER_TYPE_SINC5 0x4 #define AD4170_FILTER_FILTER_TYPE_SINC3 0x6drivers/iio/adc/ad4170.c +15
-0+/* AD4170_CURRENT_SRC_REG constants */ +#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 */
...
Quoted 158 lines
#define
AD4170_NUM_ANALOG_PINS9drivers/iio/adc/ad4170.c +6
-0@@ -222,6 +242,12 @@ #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
enumad4170_ref_buf{
AD4170_REF_BUF_PRE, /* Pre-charge referrence buffer */drivers/iio/adc/ad4170.c +0
-0@@ -278,6 +304,33 @@ static const unsigned int ad4170_sinc5_filt_fs_tbl[] = {1, 2, 4, 8, 12, 16, 20, 40, 48, 80, 100, 256 };
drivers/iio/adc/ad4170.c +27
-0+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 +}; + +enum ad4170_sensor_type { +AD4170_ADC_SENSOR =0 , +AD4170_WEIGH_SCALE_SENSOR =1 , +AD4170_THERMOCOUPLE_SENSOR =2 , +AD4170_RTD_SENSOR =3 , +}; +static const char * const ad4170_chip_names[] = { "ad4170",
"ad4190",drivers/iio/adc/ad4170.c +1
-0@@ -343,6 +396,7 @@ 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 ]; struct gpio_chip gpiochip; u32 int_pin_sel;int sps_tbl[ARRAY_SIZE(ad4170_filt_names)][AD4170_MAX_FS_TBL_SIZE][2];
drivers/iio/adc/ad4170.c +0
-0@@ -956,6 +1010,19 @@ static int ad4170_get_ain_voltage_uv(struct ad4170_state *st, int ain_n, struct device *dev = &st->spi->dev;
*ain_voltage= 0;drivers/iio/adc/ad4170.c +2
-0+/* +* The voltage bias (vbias) sets the common-mode voltage of the channel
drivers/iio/adc/ad4170.c +1
-0+* to (AVDD + AVSS)/2. If provided, AVSS supply provides the magnitude
drivers/iio/adc/ad4170.c +2
-0+* (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
drivers/iio/adc/ad4170.c +8
-0+* 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;drivers/iio/adc/ad4170.c +0
-0@@ -1746,6 +1813,242 @@ static int ad4170_gpio_init(struct iio_dev *indio_dev) return devm_gpiochip_add_data(&st->spi->dev, &st->gpiochip, indio_dev); }drivers/iio/adc/ad4170.c +34
-0+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; + +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 usedwith fn %u\n",
drivers/iio/adc/ad4170.c +5
-0+ 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;drivers/iio/adc/ad4170.c +4
-0+ +if (st->gpio_fn[gpio] !=AD4170_GPIO_UNASIGNED ) + return dev_err_probe(dev, -EINVAL, + "GPIO %u already usedwith fn %u\n",
drivers/iio/adc/ad4170.c +1
-0+ gpio, st-Quoted 1 line
gpio_fn[gpio]);
drivers/iio/adc/ad4170.c +18
-0+ + st->gpio_fn[gpio] =AD4170_GPIO_AC_EXCITATION ; + } + } +return 0 ; +} + +static int ad4170_setup_rtd (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 current_src, ret, i; + +for (i =0 ; i < num_exc_pins; i++) { +unsigned int pin = exc_pins[i]; + + current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_PIN_MSK,pin);
drivers/iio/adc/ad4170.c +1
-0+ current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_VAL_MSK,
exc_cur);drivers/iio/adc/ad4170.c +3
-0+ + ret =regmap_write (st->regmap16,AD4170_CURRENT_SRC_REG (i), + current_src);
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1891
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+if (ret) +return ret; + } + +if (ac_excited) + setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_IEXC_MSK , + num_exc_pins == 2 ? 0x2 : 0x3); + +return 0 ; +}
In the above I do not see any explicit GPIO configuration which makes me wonder if having the RTD is mutual exclusive with having GPIOs?
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1901
Quoted 11 lines
drivers/iio/adc/ad4170.c +5
-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)drivers/iio/adc/ad4170.c +5
-0+{ +int current_src, ret, i; + +if (!ac_excited) +return 0 ;
Same as above, if !ac_excited, can't we use the GPIOs? because ad4170_validate_excitation_pins() just unconditionally sets AD4170_GPIO_AC_EXCITATION. Or maybe this DT property is only adding complexity... See below
Quoted 58 lines
drivers/iio/adc/ad4170.c +4
-0+ +/* +* If a specific current is provided through +* adi,excitation-current-microamp, set excitation pins provided through
drivers/iio/adc/ad4170.c +3
-0+* 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,
drivers/iio/adc/ad4170.c +2
-0+* and GPIO1, respectively. If only two pins are specified for AC +* excitation, use ACX1 and ACX2. See AD4170 datasheet for instructions
drivers/iio/adc/ad4170.c +3
-0+* on how to setup the bridge circuit. +* +* Also, to avoid any short-circuit condition when more than one channel
drivers/iio/adc/ad4170.c +1
-0+* is enabled, set GPIO2 and GPIO0 high, and set GPIO1 and GPIO3 low to
drivers/iio/adc/ad4170.c +2
-0+* DC excite the bridge whenever a channel without AC excitation is +* selected. That is needed because GPIO pins are controlled by the next
drivers/iio/adc/ad4170.c +2
-0+* highest priority GPIO function when a channel doesn't enable AC +* excitation. See datasheet Figure 113 Weigh Scale (AC Excitation) for
drivers/iio/adc/ad4170.c +5
-0+* an example circuit diagram. +*/ +if (exc_cur ==0 ) { +if (num_exc_pins ==2 ) { + setup->misc |= FIELD_PREP(AD4170_MISC_CHOP_ADC_MSK,
0x3);drivers/iio/adc/ad4170.c +16
-0+ ret =regmap_set_bits (st->regmap16, + AD4170_GPIO_MODE_REG, + BIT(7) | BIT(5)); +if (ret) +return ret; + + ret =regmap_set_bits (st->regmap16, + AD4170_GPIO_OUTPUT_REG, + BIT(3) | BIT(2)); +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);drivers/iio/adc/ad4170.c +3
-0+ ret = regmap_set_bits(st->regmap16, + AD4170_GPIO_MODE_REG, + BIT(7) | BIT(5) | BIT(3) |BIT(1));
drivers/iio/adc/ad4170.c +6
-0+if (ret) +return ret; + + ret = regmap_set_bits(st->regmap16, + AD4170_GPIO_OUTPUT_REG, + BIT(3) | BIT(2) | BIT(1) |BIT(0));
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1955
Quoted 7 lines
drivers/iio/adc/ad4170.c +7
-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 ;
Not sure if you gain much with having the funcs OR'ed like this... If I'm not missing nothing it's only about logging in ad4170_validate_excitation_pins()? It's up to you but I would consider using bitmaps (unsigned long) for this and just test the bits.
Quoted 51 lines
drivers/iio/adc/ad4170.c +8
-0+ } + +return 0 ; + } +for (i =0 ; i < num_exc_pins; i++) { +unsigned int pin = exc_pins[i]; + + current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_PIN_MSK,pin);
drivers/iio/adc/ad4170.c +1
-0+ current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_VAL_MSK,
exc_cur);drivers/iio/adc/ad4170.c +17
-0+ + ret =regmap_write (st->regmap16,AD4170_CURRENT_SRC_REG (i), + current_src); +if (ret) +return ret; + } + + setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_IEXC_MSK , + num_exc_pins == 2 ? 0x2 : 0x3); + +return 0 ; +} + +static int ad4170_parse_external_sensor (struct ad4170_state *st, +struct fwnode_handle *child, +struct ad4170_setup *setup, + struct iio_chan_spec *chan, u8
s_type)drivers/iio/adc/ad4170.c +12
-0+{ +unsigned int num_exc_pins, exc_cur, reg_val; +struct device *dev = &st->spi->dev; +u32 pins[2 ], exc_pins[4 ]; +bool ac_excited, vbias; +int ret; + + ret =fwnode_property_read_u32_array (child,"diff-channels" , pins, + ARRAY_SIZE(pins)); +if (ret) + return dev_err_probe(dev, ret, + "Failed to read sensor diff-channels\n");
drivers/iio/adc/ad4170.c +8
-0+ + chan->differential = true; + chan->channel = pins[0 ]; + chan->channel2 = pins[1 ]; + + ac_excited =fwnode_property_read_bool (child,"adi,ac-excited" ); + + num_exc_pins = fwnode_property_count_u32(child, "adi,excitation-pins");
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 2004
Quoted 3 lines
drivers/iio/adc/ad4170.c +3
-0+if (num_exc_pins !=2 && num_exc_pins !=4 ) +return dev_err_probe (dev, -EINVAL , + "Invalid number of excitation pins\n");
Can't we assume that a valid num_exc_pins property means ac_excited = true? Because that looks to be the logic in ad4170_validate_excitation_pins().
Quoted 30 lines
drivers/iio/adc/ad4170.c +13
-0+ + ret =fwnode_property_read_u32_array (child,"adi,excitation-pins" , + exc_pins, num_exc_pins); +if (ret) +return dev_err_probe (dev, ret, + "Failed to read adi,excitation-pins\n"); + + ret =ad4170_validate_excitation_pins (st, exc_pins, num_exc_pins); +if (ret) +return ret; + + exc_cur =0 ; + ret = fwnode_property_read_u32(child, "adi,excitation-current-microamp",
drivers/iio/adc/ad4170.c +4
-0+ &exc_cur); +if (ret && s_type ==AD4170_RTD_SENSOR ) + return dev_err_probe(dev, ret, + "Failed to read adi,excitation-current-microamp\n");
drivers/iio/adc/ad4170.c +11
-0+ + ret =ad4170_find_table_index (ad4170_iout_current_ua_tbl, exc_cur); +if (ret <0 ) +return dev_err_probe (dev, ret, + "Invalid excitation current: %uuA\n", + exc_cur); + +/* Get the excitation current configuration value */ + exc_cur = ret; + +if (s_type ==AD4170_THERMOCOUPLE_SENSOR ) {
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 2042
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+ vbias =fwnode_property_read_bool (child,"adi,vbias" ); +if (vbias) { + st->pins_fn[chan->channel2] |=AD4170_PIN_VBIAS ; + reg_val =BIT (chan->channel2); +return regmap_write (st->regmap16,AD4170_V_BIAS_REG , + reg_val); + } + } +if (s_type ==AD4170_WEIGH_SCALE_SENSOR || + s_type == AD4170_THERMOCOUPLE_SENSOR) {
I guess you have this because of indentation levels but is it that bad if we do
if (s_typ == AD4170_THERMOCOUPLE_SENSOR && fwnode_property_read_bool(child, "adi,vbias") {
...
}Quoted 78 lines
drivers/iio/adc/ad4170.c +4
-0+ ret =ad4170_setup_bridge (st, child, setup, exc_pins, + num_exc_pins, exc_cur, ac_excited); + }else { + ret = ad4170_setup_rtd(st, child, setup, exc_pins,
num_exc_pins,drivers/iio/adc/ad4170.c +5
-0+ exc_cur, ac_excited); + } +return ret; +} +static int
ad4170_parse_reference(struct ad4170_state *st, struct fwnode_handle *child, struct ad4170_setup *setup)drivers/iio/adc/ad4170.c +1
-0@@ -1827,6 +2130,7 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev, struct ad4170_state *st = iio_priv(indio_dev); struct device *dev = &st->spi->dev; struct ad4170_chan_info *chan_info; +u8 s_type =AD4170_ADC_SENSOR ; struct ad4170_setup *setup; struct iio_chan_spec *chan;unsigned int
ch_reg;drivers/iio/adc/ad4170.c +0
-0@@ -1857,10 +2161,34 @@ static int ad4170_parse_channel_node(struct iio_dev *indio_dev, if (ret) return ret;drivers/iio/adc/ad4170.c +12
-3- ret =ad4170_parse_adc_channel_type (dev, child, chan); -if (ret <0 ) -return ret; + ret =fwnode_property_read_u8 (child,"adi,sensor-type" , &s_type); +if (!ret) { +if (s_type >AD4170_RTD_SENSOR ) +return dev_err_probe (dev, ret, + "Invalid adi,sensor-type: %u\n", + s_type); + } +switch (s_type) { +case AD4170_ADC_SENSOR : + ret =ad4170_parse_adc_channel_type (dev, child, chan); +if (ret <0 ) +return ret;drivers/iio/adc/ad4170.c +15
-0+break ; +case AD4170_WEIGH_SCALE_SENSOR : + fallthrough; +case AD4170_THERMOCOUPLE_SENSOR : + fallthrough; +case AD4170_RTD_SENSOR : + ret =ad4170_parse_external_sensor (st, child, setup, chan, + s_type); +if (ret <0 ) +return ret; + +break ; +default : +return -EINVAL ; + }bipolar =
fwnode_property_read_bool(child, "bipolar");setup->afe|=FIELD_PREP(AD4170_AFE_BIPOLAR_MSK, bipolar); if (bipolar)drivers/iio/adc/ad4170.c +0
-0@@ -2057,6 +2385,9 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev) for (i = 0; i < AD4170_NUM_ANALOG_PINS; i++) st->pins_fn[i] = AD4170_PIN_UNASIGNED;drivers/iio/adc/ad4170.c +3
-0+for (i =0 ; i <AD4170_NUM_GPIO_PINS ; i++) + st->gpio_fn[i] =AD4170_GPIO_UNASIGNED ; +/* On power on, device defaults to using SDO pin for data ready signal */
st->int_pin_sel=AD4170_INT_PIN_SDO;
ret = device_property_match_property_string(dev, "interrupt-names",drivers/iio/adc/ad4170.c +0
-0@@ -2081,6 +2412,10 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
/* Only create a GPIO chip if flagged for it */
if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
drivers/iio/adc/ad4170.c static int ad4170_parse_firmware(struct iio_dev *indio_dev) · line 2417
Quoted 3 lines
drivers/iio/adc/ad4170.c +3
-0+for (i =0 ; i <AD4170_NUM_GPIO_PINS ; i++) +if (st->gpio_fn[i] !=AD4170_GPIO_UNASIGNED ) +return 0 ;
I think you could improve this... You're taking an all or nothing approach. IIUC, we can have cases where only two GPIOs are in use which means we could use the other 2? There the gpiochio init_valid_mask() call that you could potentially use.
- Nuno Sá
Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Nuno SáHi Nuno, thank you for your review.
I've already applied most of your suggestions.
Also providing answers to some questions inline.
Thanks
Quoted 18 lines · Nuno Sá
Hi Marecelo,
First, superficial look...
Quoted 13 lines · Marcelo Schmitt
From: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.Signed-off-by Ana-Maria Cusco <[email protected]>Co-developed-by Marcelo Schmitt <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>---
...
Quoted 7 lines
drivers/iio/adc/ad4170.c line 1119
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ ret =ad4170_set_channel_enable (st, chan->address, true); +if (ret) +return ret; + +reinit_completion (&st->completion);I would do the above right before
wait_for_completion_timeout()...
Ack. Changed accordingly.
...
Quoted 6 lines
drivers/iio/adc/ad4170.c line 1339
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +3
-0+ + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK ; + setup->afe |=FIELD_PREP (AD4170_AFE_PGA_GAIN_MSK , pga);ditto...
Ack.
...
Quoted 4 lines
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +4
-0+static int ad4170_set_channel_freq (struct ad4170_state *st, + struct iio_chan_spec const *chan, int val, + int val2) +{
...
Quoted 10 lines
drivers/iio/adc/ad4170.c line 1386
Quoted 8 lines
drivers/iio/adc/ad4170.c new file +8
-0+ + old_filter_fs = setup->filter_fs; +if (f_type ==AD4170_SINC5 ) + setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i]; +else + setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i]; + +guard (mutex)(&st->lock);Shouldn't the lock also protect the 'setup' struct?
Good catch. Changed to acquire the lock before reading and updating the setup in all places.
...
Quoted 6 lines
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6
-0+static int ad4170_parse_reference (struct ad4170_state *st, + struct fwnode_handle *child, + struct ad4170_setup *setup) +{ +struct device *dev = &st->spi->dev; +int ret;
Quoted 9 lines
drivers/iio/adc/ad4170.c line 1517
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6
-0+u8 aux; + +/* Positive reference buffer setup */ + aux =AD4170_REF_BUF_PRE ;/* Default to have precharge buffer enabled. */ + ret =fwnode_property_read_u8 (child,"adi,buffered-positive" , &aux); +if (ret) {Shouldn't this be if (!ret)?
Yes, fixed for all off adi,buffered-positive/negative, adi,reference-select. Thanks.
Quoted 6 lines
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (aux <AD4170_REF_BUF_PRE || aux >AD4170_REF_BUF_BYPASS ) + return dev_err_probe(dev, -EINVAL, + "Invalid adi,buffered-positive:%u\n",
(quoted diff) +2
-0+ aux); + }
Quoted 8 lines
drivers/iio/adc/ad4170.c line 1528
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6
-0+ setup->afe |=FIELD_PREP (AD4170_AFE_REF_BUF_P_MSK , aux); + +/* Negative reference buffer setup */ + aux =AD4170_REF_BUF_PRE ;/* Default to have precharge buffer enabled. */ + ret =fwnode_property_read_u8 (child,"adi,buffered-negative" , &aux); +if (ret) {ditto
Ack
Quoted 9 lines
Quoted 9 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (aux <AD4170_REF_BUF_PRE || aux >AD4170_REF_BUF_BYPASS ) + return dev_err_probe(dev, -EINVAL, + "Invalid adi,buffered-negative:%u\n",
drivers/iio/adc/ad4170.c new file +4
-0+ aux); + } + setup->afe |=FIELD_PREP (AD4170_AFE_REF_BUF_M_MSK , aux); +
...
Quoted 26 lines
Quoted 26 lines
drivers/iio/adc/ad4170.c new file +26
-0+ +static int ad4170_parse_adc_channel_type (struct device *dev, +struct fwnode_handle *child, +struct iio_chan_spec *chan) +{ +u32 pins[2 ]; +int ret; + + ret =fwnode_property_read_u32_array (child,"diff-channels" , pins, + ARRAY_SIZE(pins)); +if (!ret) { + chan->differential = true; + chan->channel = pins[0 ]; + chan->channel2 = pins[1 ]; +return 0 ; + } + ret =fwnode_property_read_u32 (child,"single-channel" , &pins[0 ]); +if (!ret) { + chan->differential = false; + chan->channel = pins[0 ]; + + ret =fwnode_property_read_u32 (child,"common-mode-channel" , + &pins[1]); +if (ret) + return dev_err_probe(dev, ret, + "single-ended channels must define common-mode-
Quoted 8 lines
drivers/iio/adc/ad4170.c line 1561
Quoted 5 lines
channel\n");
drivers/iio/adc/ad4170.c new file +4
-0+ + chan->channel2 = pins[1 ]; +return 0 ; + }Kind of a nitpick but for the above I would flip the logic. First check for errors in the single-channel case and then you can have one less of level of indentation...
Okay, will do it.
Quoted 5 lines
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +4
-0+return dev_err_probe (dev, ret, +"Channel must define one of diff-channels or single-channel.\n" ); +} +
...
Quoted 7 lines
drivers/iio/adc/ad4170.c line 1597
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ + ret =fwnode_property_read_u32 (child,"reg" , &ch_reg); +if (ret) +return ret; +Could also deserve a log message?
Sure, added.
Quoted 7 lines
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +3
-0+if (ch_reg >=AD4170_MAX_CHANNELS ) +return dev_err_probe (dev, -EINVAL , + "Channel idx greater than no of channels\n");...
...
Quoted 6 lines
drivers/iio/adc/ad4170.c line 1798
Quoted 3 lines
drivers/iio/adc/ad4170.c new file +3
-0+ +return request_irq (st->spi->irq, &ad4170_irq_handler,IRQF_ONESHOT , + indio_dev->name, indio_dev);
devm_request_irq()...
Done.
Quoted 35 lines
Quoted 35 lines
drivers/iio/adc/ad4170.c new file +35
-0+} + +static int ad4170_regulator_setup (struct ad4170_state *st) +{ +struct device *dev = &st->spi->dev; +int ret; + +/* Required regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get AVDD voltage.\n" ); + + st->vrefs_uv[AD4170_AVDD_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"iovdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get IOVDD voltage.\n" ); + + st->vrefs_uv[AD4170_IOVDD_SUP ] = ret; + +/* Optional regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avss" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get AVSS voltage.\n" ); + +/* Assume AVSS at GND (0V) if not provided */ + st->vrefs_uv[AD4170_AVSS_SUP ] = ret == -ENODEV ?0 : -ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1p" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN+ voltage.\n" ); + + st->vrefs_uv[AD4170_REFIN1P_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1n" );
Quoted 8 lines
drivers/iio/adc/ad4170.c line 1838
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN1N_SUP ] = ret == -ENODEV ? -ENODEV : -ret;Maybe to early for me but the comment does not make it clear to me why the negation? Won't the regulator return a negative voltage?
devm_regulator_get_enable_read_voltage(), regulator_get_voltage(), and anything about reading the regulator voltage returns either a positive voltage value or a negative error code. I couldn't find out how to read a negative voltage with regulator API. So, for now, this is making the simplifying assumption that the negative end of external reference supplies is always below GND level (even though they could be positive).
...
Quoted 6 lines
drivers/iio/adc/ad4170.c line 1869
Quoted 3 lines
drivers/iio/adc/ad4170.c new file +3
-0+ + st =iio_priv (indio_dev); +devm_mutex_init (dev, &st->lock);check for errors...
Ack, done.
Quoted 1 line
Quoted 1 line
+
...
Quoted 8 lines
drivers/iio/adc/ad4170.c line 1893
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ st->regmap24 =devm_regmap_init_spi (spi, &ad4170_regmap24_config); +if (IS_ERR (st->regmap24)) +return dev_err_probe (dev,PTR_ERR (st->regmap24), + "Failed to initialize regmap24\n"); +Hmm, interesting idea... but I would expect an explanation on why can't we have bulk reads for the 16 and 24 bit cases? Without it, I have to ask why not?
Not sure how regmap_bulk_read() handles it from a quick look but CS can't be taken high between multibyte register access. Anyway, will check out if that can be used here.
Quoted 1 line
...
Quoted 9 lines
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +5
-0+ +if (spi->irq) { + ret =ad4170_trigger_setup (indio_dev); +if (ret) + return dev_err_probe(dev, ret, "Failed to setuptrigger\n");
Typically it's better to log the errors inside
ad4170_trigger_setup()unless you use it outside probe.
Ack.
Quoted 3 lines
- Nuno Sá
Quoted 1 line
Thanks,
Marcelo
Rob Herring
maintainerRe: [PATCH v1 1/7] dt-bindings: iio: adc: Add AD4170
In reply to Marcelo SchmittQuoted 38 lines · Marcelo Schmitt
Add device tree documentation for AD4170 and similar sigma-delta ADCs. The AD4170 is a 24-bit, multichannel, sigma-delta ADC.
Signed-off-by Marcelo Schmitt <[email protected]>---
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..../bindings/iio/adc/adi,ad4170.yaml | 527 ++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 534 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yamlDocumentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +14
-0@@ -0,0 +1,527 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4170 and similar Analog to Digital Converters + +maintainers: + - Marcelo Schmitt <[email protected]> + +description: | + Analog Devices AD4170 series of Sigma-delta Analog to Digital Converters. + Specifications can be found at:
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 24
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4190-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4195-4.pdf + +$ref: /schemas/spi/spi-peripheral-props.yaml# + +$defs: + sensor-node: + type: object + description: |
Don't need '|' if no formatting.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 36
Quoted 12 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +12
-0+ Common properties of external sensor circuitry connected to the ADC. + + properties: + reg: + description: + Channel number. Connects the sensor to the channel with this number + of the device. + minimum: 1 + maximum: 16 + + diff-channels: + $ref: /schemas/types.yaml#/definitions/uint32-array
Already has a type in adc.yaml which needs to be referenced for 'sensor-node'.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 39
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +3
-0+ maxItems: 2 + minItems: 2 + description: |
Don't need '|'. And a few more places...
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 45
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +6
-0+ ADC analog input pins to which the sensor circuit is connected. + The first value specifies the positive input pin, the second + specifies the negative input pin. See adc.yaml for details. + + bipolar: + $ref: /schemas/types.yaml#/definitions/flag
Already has a type.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 54
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +9
-0+ description: If provided, the channel is to be used in bipolar mode. + + adi,sensor-type: + description: Type of sensor connected to the device. + $ref: /schemas/types.yaml#/definitions/uint8 + + adi,ac-excited: + type: boolean + description: |
Don't need '|'
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 64
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ Whether the external circuit has to be AC or DC excited. + + adi,excitation-pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + ADC pins used for external circuit excitation. Some applications + require optimum matching between excitation currents. Using excitation + current pairs minimizes the excitation current mismatch and the + excitation current drift matching on the ADC. Must describe either 1 + or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>.
This looks like a uint32-matrix instead:
items:
minItems: 2
maxItems: 2
items:
maximum: <max pin number>?
Quoted 68 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +68
-0+ + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the first external reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2, 3] + default: 0 + + required: + - reg + - diff-channels + - bipolar + - adi,sensor-type + - adi,excitation-pins + - adi,reference-select + +properties: + compatible: + enum: + - adi,ad4170 + - adi,ad4190 + - adi,ad4195 + + avss-supply: + description: + Referece voltage supply for AVSS. If provided, describes the magnitude + (absolute value) of the negative voltage supplied to the AVSS pin. Since + AVSS must be −2.625V minimum and 0V maximum, the declared supply voltage + must be between 0 and 2.65V. If not provided, AVSS is assumed to be at + system ground (0V). + + avdd-supply: + description: + A supply of 4.75V to 5.25V relative to AVSS that powers the chip (AVDD). + + iovdd-supply: + description: 1.7V to 5.25V reference supply to the serial interface (IOVDD). + + refin1p-supply: + description: REFIN+ supply that can be used as reference for conversion. + + refin1n-supply: + description: REFIN- supply that can be used as reference for conversion. If + provided, describes the magnitude (absolute value) of the negative voltage + supplied to the REFIN- pin. + + refin2p-supply: + description: REFIN2+ supply that can be used as reference for conversion. + + refin2n-supply: + description: REFIN2- supply that can be used as reference for conversion. If + provided, describes the magnitude (absolute value) of the negative voltage + supplied to the REFIN2- pin. + + spi-cpol: true
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 142
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ + spi-cpha: true + + interrupts: + maxItems: 1 + + interrupt-names: + description: | + Specify which pin should be configured as Data Ready interrupt. + Default if not supplied is sdo.
default: sdo
And drop the sentence.
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +4
-0+ enum: + - sdo + - dig_aux1 +
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 156
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ clocks: + maxItems: 1 + description: + Optional external clock source. Can specify either an external clock or + external crystal. + + clock-names: + enum: + - ext-clk + - xtal
default?
Quoted 27 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +27
-0+ + '#clock-cells': + const: 0 + + gpio-controller: true + + "#gpio-cells": + const: 2 + description: | + The first cell is for the GPIO number: 0 to 3. + The second cell takes standard GPIO flags. + + ldac-gpios: + description: + GPIO connected to DIG_AUX2 pin to be used as LDAC toggle to control the + transfer of data from the DAC_INPUT_A register to the DAC. + maxItems: 1 + + '#address-cells': + const: 1 + + '#size-cells': + const: 0 + +patternProperties: + "^channel@[0-9a-f]$": + $ref: adc.yaml
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 193
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ type: object + unevaluatedProperties: false + description: | + Represents the external channels which are connected to the ADC. + + properties: + reg: + description: | + The channel number. + items:
Drop 'items' to imply there's only 1 entry.
Quoted 19 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +19
-0+ minimum: 0 + maximum: 15 + + diff-channels: + description: | + This property is used for defining the inputs of a differential + voltage channel. The first value is the positive input and the second + value is the negative input of the channel. + + Besides the analog input pins AIN0 to AIN8, there are special inputs + that can be selected with the following values: + 17: Internal temperature sensor + 18: (AVDD-AVSS)/5 + 19: (IOVDD-DGND)/5 + 20: DAC output + 21: ALDO + 22: DLDO + 23: AVSS + 24: DGND
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 222
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ 25: REFIN+ + 26: REFIN- + 27: REFIN2+ + 28: REFIN2- + 29: REFOUT + For the internal temperature sensor, use the input number for both + inputs (i.e. diff-channels = <17 17>). + items: + enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20, 21, 22, 23, 24, 25, + 26, 27, 28, 29]
These constraints don't apply to other cases of diff-channels?
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 233
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +11
-0+ + single-channel: true + + common-mode-channel: true + + bipolar: true + + adi,sensor-type: + description: Sensor type for direct ADC sensors. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 0
Since this is optional and only one possible value, you don't need it...
Quoted 270 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +270
-0+ + adi,buffered-positive: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the positive voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC + combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on positive reference inputs (REFIN+, + REFIN2+) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled). + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,buffered-negative: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the negative voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC + combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on negative reference inputs (REFIN-, + REFIN2-) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled). + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the internal reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2, 3] + default: 2 + + required: + - reg + + allOf: + - oneOf: + - required: [single-channel] + properties: + diff-channels: false + - required: [diff-channels] + properties: + single-channel: false + common-mode-channel: false + + "^weighscale@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: Weigh scale sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 1 + + adi,excitation-pins: + description: | + ADC pins to use for weigh scale bridge circuit excitation. Must + describe either 1 or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 4 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. If not provided and + adi,ac-excited is true, use predefined ACX1, ACX1 negated, ACX2, and + ACX2 negated signals to AC excite the weigh scale bridge. Those + singals are output on GPIO2, GPIO0, GPIO3, and GPIO1, respectively. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + + adi,power-down-switch-pin: + description: | + Number of the GPIO used as power-down switch for the bridge circuit. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1] + + "^thermocouple@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: Thermocouple sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 2 + + adi,excitation-pins: + description: | + ADC pins to use for bridge circuit excitation. Must describe either 1 + or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 4 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. If not provided and + adi,ac-excited is true, use predefined ACX1, ACX1 negated, ACX2, and + ACX2 negated signals to AC excite the bridge circuit. Those singals + are output on GPIO2, GPIO0, GPIO3, and GPIO1, respectively. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + + adi,vbias: + type: boolean + description: | + For unbiased thermocouple applications, the voltage generated by the + thermocouple must be biased around some DC voltage. When present, this + property specifies a bias voltage of (AVDD + AVSS)/2 to be applied as + common-mode voltage for the sensor. + + adi,power-down-switch-pin: + description: | + Number of the GPIO used as power-down switch for the bridge circuit. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1] + + "^rtd@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: RTD sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 3 + + adi,excitation-pins: + description: | + ADC pins to use for RTD circuit excitation. Must describe a pair of + pins. E.g. <0 1>; <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 2 + items: + minimum: 0 + maximum: 20 + + adi,excitation-current-microamp: true + + required: + - adi,excitation-current-microamp + +required: + - compatible + - reg + - avdd-supply + - iovdd-supply + - spi-cpol + - spi-cpha + +allOf: + # Some devices don't have integrated DAC + - if: + properties: + compatible: + contains: + enum: + - adi,ad4190 + - adi,ad4195 + then: + properties: + ldac-gpios: false + +unevaluatedProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4170"; + reg = <0>; + spi-max-frequency = <20000000>; + spi-cpol; + spi-cpha; + avdd-supply = <&avdd>; + iovdd-supply = <&iovdd>; + interrupt-parent = <&gpio_in>; + interrupts = <0 IRQ_TYPE_EDGE_FALLING>; + #address-cells = <1>; + #size-cells = <0>; + + // Sample AIN0 with respect to AIN1 throughout AVDD/AVSS input range + // Differential bipolar. If AVSS < 0V, differential true bipolar + channel@0 { + reg = <0>; + bipolar; + diff-channels = <0 1>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN2 with respect to DGND throughout AVDD/DGND input range + // Pseudo-differential unipolar (fig. 2a) + channel@1 { + reg = <1>; + single-channel = <2>; + common-mode-channel = <24>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN3 with respect to 2.5V throughout AVDD/AVSS input range + // Pseudo-differential bipolar (fig. 2b) + channel@2 { + reg = <2>; + bipolar; + single-channel = <3>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN4 with respect to DGND throughout AVDD/AVSS input range + // Pseudo-differential bipolar (fig. 2c) + channel@3 { + reg = <3>; + bipolar; + single-channel = <4>; + common-mode-channel = <24>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + }; + // Sample AIN5 with respect to 2.5V throughout AVDD/AVSS input range + // Pseudo-differential unipolar (AD4170 datasheet page 46 example) + channel@4 { + reg = <4>; + single-channel = <5>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <3>; + };
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 513
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ // Sample AIN6 with respect to 2.5V throughout REFIN+/REFIN- input range + // Pseudo-differential bipolar + channel@5 { + reg = <5>; + bipolar; + single-channel = <6>; + common-mode-channel = <29>; + adi,sensor-type = /bits/ 8 <0>; + adi,reference-select = /bits/ 8 <0>; + };
Can you try to have every child node type in the example rather than multiple cases of 'channel' nodes? We want every property defined in schemas in the examples if possible (but not creating every possible iteration).
Quoted 35 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +14
-0+ // Weigh scale sensor + weighscale@6 { + reg = <6>; + bipolar; + diff-channels = <7 8>; + adi,sensor-type = /bits/ 8 <1>; + adi,ac-excited; + adi,excitation-pins = <17 18>, <19 20>; + adi,reference-select = /bits/ 8 <0>; + }; + }; + }; +... +MAINTAINERS +7
-0@@ -1337,6 +1337,13 @@ F: Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130 F: Documentation/devicetree/bindings/iio/adc/adi,ad4130.yaml F: drivers/iio/adc/ad4130.c +ANALOG DEVICES INC AD4170 DRIVER +M: Marcelo Schmitt <[email protected]> +L: [email protected] +S: Supported +W: https://ez.analog.com/linux-software-drivers +F: Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml + ANALOG DEVICES INC AD4695 DRIVER M: Michael Hennerich <[email protected]> M: Nuno Sá <[email protected]>Signature · 1 line
2.47.2
Jonathan Cameron
maintainerRe: [PATCH v1 1/7] dt-bindings: iio: adc: Add AD4170
In reply to Marcelo SchmittQuoted 49 lines · On Wed, 9 Apr 2025 09:24:18 -0300 Marcelo Schmitt <[email protected]>…
Add device tree documentation for AD4170 and similar sigma-delta ADCs. The AD4170 is a 24-bit, multichannel, sigma-delta ADC.
Signed-off-by Marcelo Schmitt <[email protected]>---
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..../bindings/iio/adc/adi,ad4170.yaml | 527 ++++++++++++++++++ MAINTAINERS | 7 + 2 files changed, 534 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad4170.yamlDocumentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +25
-0@@ -0,0 +1,527 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/adc/adi,ad4170.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Analog Devices AD4170 and similar Analog to Digital Converters + +maintainers: + - Marcelo Schmitt <[email protected]> + +description: | + Analog Devices AD4170 series of Sigma-delta Analog to Digital Converters. + Specifications can be found at: + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4170-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4190-4.pdf + https://www.analog.com/media/en/technical-documentation/data-sheets/ad4195-4.pdf + +$ref: /schemas/spi/spi-peripheral-props.yaml# + +$defs: + sensor-node: + type: object + description: | + Common properties of external sensor circuitry connected to the ADC.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 35
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ + properties: + reg: + description: + Channel number. Connects the sensor to the channel with this number + of the device. + minimum: 1 + maximum: 16 + + diff-channels:
Maybe add some comments to the file. I was very confused on what this def is for, but now I think it's just an adaption of an ADC channel to a specific sensor type.
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +7
-0+ $ref: /schemas/types.yaml#/definitions/uint32-array + maxItems: 2 + minItems: 2 + description: | + ADC analog input pins to which the sensor circuit is connected. + The first value specifies the positive input pin, the second + specifies the negative input pin. See adc.yaml for details.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 52
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ + bipolar: + $ref: /schemas/types.yaml#/definitions/flag + description: If provided, the channel is to be used in bipolar mode. + + adi,sensor-type: + description: Type of sensor connected to the device. + $ref: /schemas/types.yaml#/definitions/uint8 + + adi,ac-excited:
All the existing excitation related bindings are
adi,excitation-*
Maybe adi,excitation-ac for this one?
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 55
Quoted 3 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +3
-0+ type: boolean + description: | + Whether the external circuit has to be AC or DC excited.
Hmm. The concept of a DC excitation source confused me a bit, but I guess that's just a current source.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 66
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +11
-0+ + adi,excitation-pins: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: | + ADC pins used for external circuit excitation. Some applications + require optimum matching between excitation currents. Using excitation + current pairs minimizes the excitation current mismatch and the + excitation current drift matching on the ADC. Must describe either 1 + or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + + adi,excitation-current-microamp:
We have an existing rtd specific version of this but I guess it can be used for other things with this chip so fair enough to generalize it.
Quoted 16 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +16
-0+ description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the first external reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 92
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ enum: [0, 1, 2, 3] + default: 0 + + required: + - reg + - diff-channels + - bipolar + - adi,sensor-type + - adi,excitation-pins + - adi,reference-select
If it is required, why specify defaults above?
Quoted 49 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +48
-0+ +properties: + compatible: + enum: + - adi,ad4170 + - adi,ad4190 + - adi,ad4195 + +patternProperties: + "^channel@[0-9a-f]$": + $ref: adc.yaml + type: object + unevaluatedProperties: false + description: | + Represents the external channels which are connected to the ADC. + + properties: + reg: + description: | + The channel number. + items: + minimum: 0 + maximum: 15 + + diff-channels: + description: | + This property is used for defining the inputs of a differential + voltage channel. The first value is the positive input and the second + value is the negative input of the channel. + + Besides the analog input pins AIN0 to AIN8, there are special inputs + that can be selected with the following values: + 17: Internal temperature sensor + 18: (AVDD-AVSS)/5 + 19: (IOVDD-DGND)/5 + 20: DAC output + 21: ALDO + 22: DLDO + 23: AVSS + 24: DGND + 25: REFIN+ + 26: REFIN- + 27: REFIN2+ + 28: REFIN2- + 29: REFOUT + For the internal temperature sensor, use the input number for both + inputs (i.e. diff-channels = <17 17>). + items:
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 230
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ enum: [0, 1, 2, 3, 4, 5, 6, 7, 8, 17, 18, 19, 20, 21, 22, 23, 24, 25, + 26, 27, 28, 29] + + single-channel: true + + common-mode-channel: true + + bipolar: true + + adi,sensor-type:
This wants a fuller description somewhere in the binding. What are the possible types? Also why do we need them here if the sensor-nodes refer to channels?
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +9
-0+ description: Sensor type for direct ADC sensors. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 0 + + adi,buffered-positive: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the positive voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 249
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on positive reference inputs (REFIN+, + REFIN2+) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled).
I'm not seeing a reason (from this description) to ever pick 0. Maybe talk about what the precharge buffer alone gets you.
Quoted 71 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +71
-0+ $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,buffered-negative: + description: | + Enable precharge buffer, full buffer, or skip reference buffering of + the negative voltage reference. Because the output impedance of the + source driving the voltage reference inputs may be dynamic, RC + combinations of those inputs can cause DC gain errors if the reference + inputs go unbuffered into the ADC. Enable reference buffering if the + provided reference source has dynamic high impedance output. Note the + absolute voltage allowed on negative reference inputs (REFIN-, + REFIN2-) is from AVSS − 50 mV to AVDD + 50 mV when the reference + buffers are disabled but narrows to AVSS to AVDD when reference + buffering is enabled or in precharge mode. + 0: Reference precharge buffer. + 1: Full Buffer. + 2: Bypass reference buffers (buffering disabled). + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2] + default: 0 + + adi,reference-select: + description: | + Select the reference source to use when converting on the specific + channel. Valid values are: + 0: Differential reference voltage REFIN+ - REFIN−. + 1: Differential reference voltage REFIN2+ - REFIN2−. + 2: Internal 2.5V referece (REFOUT) relative to AVSS. + 3: Analog supply voltage (AVDD) relative AVSS. + If this field is left empty, the internal reference is selected. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1, 2, 3] + default: 2 + + required: + - reg + + allOf: + - oneOf: + - required: [single-channel] + properties: + diff-channels: false + - required: [diff-channels] + properties: + single-channel: false + common-mode-channel: false + + "^weighscale@": + $ref: '#/$defs/sensor-node' + unevaluatedProperties: false + + properties: + diff-channels: true + bipolar: true + + adi,sensor-type: + description: Weigh scale sensor. + $ref: /schemas/types.yaml#/definitions/uint8 + const: 1 + + adi,excitation-pins: + description: | + ADC pins to use for weigh scale bridge circuit excitation. Must + describe either 1 or 2 pairs of pins. E.g. <0 1>; <2 3>; <0 1>, <2 3>. + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 2 + maxItems: 4 + items: + minimum: 0
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 330
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +10
-0+ maximum: 20 + + adi,excitation-current-microamp: + description: | + Excitation current in microamperes to be output to each excitation pin + specified by adi,excitation-pins property. If not provided and + adi,ac-excited is true, use predefined ACX1, ACX1 negated, ACX2, and + ACX2 negated signals to AC excite the weigh scale bridge. Those + singals are output on GPIO2, GPIO0, GPIO3, and GPIO1, respectively. + enum: [0, 10, 50, 100, 250, 500, 1000, 1500]
Why the repeat? This description is already in the sensor-node definition.
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml line 337
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4170.yaml new file +7
-0+ + adi,power-down-switch-pin: + description: | + Number of the GPIO used as power-down switch for the bridge circuit. + $ref: /schemas/types.yaml#/definitions/uint8 + enum: [0, 1] +
In general, can we reduce repetition of docs across the sensor types so we don't repeat anything?
J
Jonathan Cameron
maintainerRe: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Marcelo SchmittQuoted 34 lines · On Fri, 11 Apr 2025 12:38:35 -0300 Marcelo Schmitt…
Hi Nuno, thank you for your review.
I've already applied most of your suggestions.
Also providing answers to some questions inline.Thanks
Quoted 18 lines · Nuno Sá
Hi Marecelo,
First, superficial look...
Quoted 13 lines · Marcelo Schmitt
From: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.Signed-off-by Ana-Maria Cusco <[email protected]>Co-developed-by Marcelo Schmitt <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>---
...
Quoted 7 lines
drivers/iio/adc/ad4170.c line 1119
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ ret =ad4170_set_channel_enable (st, chan->address, true); +if (ret) +return ret; + +reinit_completion (&st->completion);I would do the above right before
wait_for_completion_timeout()...
Quoted 1 line
Ack. Changed accordingly.
Hi Marcelo,
A general efficiency comment. If you agree with a comment, no need to reply. Better to crop that bit of the thread out and reply only to those parts where there is more to add.
Cuts down on what reviewers need to read! We assume anything you don't comment on is something you are happy with changing as suggested!
I normally only point this out to people who are sending a lot of code as it's just not worth it for one time contributors. So bad luck you get me being fussier ;)
Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/ad4170.c line 1838
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6
-0+ ret =devm_regulator_get_enable_read_voltage (dev,"refin1n" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN1N_SUP ] = ret == -ENODEV ? -ENODEV : -ret;Maybe to early for me but the comment does not make it clear to me why the negation? Won't the regulator return a negative voltage?
Quoted 6 lines
devm_regulator_get_enable_read_voltage(),regulator_get_voltage(), and anything about reading the regulator voltage returns either a positive voltage value or a negative error code. I couldn't find out how to read a negative voltage with regulator API. So, for now, this is making the simplifying assumption that the negative end of external reference supplies is always below GND level (even though they could be positive).
Hmm. We went around this a long time back but I can't remember what the outcome was...
https://lore.kernel.org/linux-iio/544AC56F16B56944AEC3BD4E3D59177137546EF3FC@LIMKCMBX1.ad.analog.com/
looks like the thread.
Take a look at dac/ad5791.c for example of a negative reference
Michael might remember more of that discussion than I do!
Jonathan
Jonathan Cameron
maintainerRe: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Marcelo SchmittQuoted 9 lines · On Wed, 9 Apr 2025 09:24:35 -0300 Marcelo Schmitt <[email protected]>…
From: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.
Quoted 3 lines
Signed-off-by Ana-Maria Cusco <[email protected]>Co-developed-by Marcelo Schmitt <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>
Hi.
Clearly this is a massive driver, even with it broken up like this. So it might take a few cycles to review enough that we don't find new things :(
I can't think of a good way to split it up further though
Jonathan
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +0
-0
drivers/iio/adc/ad4170.c line 185
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +9
-0+#define AD4170_NUM_ANALOG_PINS 9 +#define AD4170_MAX_CHANNELS 16 +#define AD4170_MAX_ANALOG_PINS 8 +#define AD4170_MAX_SETUPS 8 +#define AD4170_INVALID_SETUP 9 +#define AD4170_NUM_CURRENT_SRC 4 +#define AD4170_DEFAULT_SAMP_RATE (125 * KILO) + +#define AD4170_INT_REF_2_5V (2500 * MILLI)
As with the clocks. Sometimes just having the value inline is clearer.
Also units of this are not clear from name (uvolts it seems)
drivers/iio/adc/ad4170.c line 190
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ +/* Internal and external clock properties */ +#define AD4170_INT_CLOCK_16MHZ (16 * MEGA) +#define AD4170_EXT_CLOCK_MHZ_MIN (1 * MEGA) +#define AD4170_EXT_CLOCK_MHZ_MAX (17 * MEGA)
Do these benefit from being defines given the are real numbers, not magic ones?
drivers/iio/adc/ad4170.c line 257
Quoted 4 lines
drivers/iio/adc/ad4170.c new file +4
-0+#define AD4170_MAX_FS_TBL_SIZE ARRAY_SIZE(ad4170_sinc3_filt_fs_tbl) + +static const unsigned int ad4170_sinc5_filt_fs_tbl[] = { +1 ,2 ,4 ,8 ,12 ,16 ,20 ,40 ,48 ,80 ,100 ,256
Add the trailing comma. Only time to not have those is for terminating / null entries. It is a slightly odd convention given we won't see more elements in an array like this one but it has been like that a long time.
Similar for other cases.
Quoted 11 lines
drivers/iio/adc/ad4170.c new file +8
-0+}; + +static int ad4170_read_sample (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int *val) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int settling_time_ms, ret; +
drivers/iio/adc/ad4170.c line 1117
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+guard (mutex)(&st->lock); +/* +* The ADC sequences through all enabled channels. That can lead to +* incorrect channel being sampled if a previous read would have left a +* different channel enabled. Thus, always enable and disable the +* channel on single-shot read. +*/ + ret =ad4170_set_channel_enable (st, chan->address, true); +if (ret) +return ret;
Consider factoring out from here...
Quoted 19 lines
drivers/iio/adc/ad4170.c new file +19
-0+ +reinit_completion (&st->completion); + + ret =ad4170_set_mode (st,AD4170_ADC_CTRL_MODE_SINGLE ); +if (ret) + goto err_disable; + +/* +* When a channel is manually selected by the user, the ADC needs an +* extra time to provide the first stable conversion. The ADC settling +* time depends on the filter type, filter frequency, and ADC clock +* frequency (see datasheet page 53). The maximum settling time among +* all filter configurations is 6291164 / fCLK. Use that formula to wait +* for sufficient time whatever the filter configuration may be. +*/ + settling_time_ms =DIV_ROUND_UP (6291164 *MILLI , st->mclk_hz); + ret =wait_for_completion_timeout (&st->completion, +msecs_to_jiffies (settling_time_ms)); +if (!ret)
drivers/iio/adc/ad4170.c line 1146
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+dev_dbg (&st->spi->dev, +"No Data Ready signal. Reading after delay.\n" ); + + ret =regmap_read (st->regmap24,AD4170_DATA_24B_REG , val); +if (ret) + goto err_disable; + +if (chan->scan_type.sign =='s' ) + *val =sign_extend32 (*val, chan->scan_type.realbits -1 ); +
to here as a helper. That way you can do direct returns from the helper.
drivers/iio/adc/ad4170.c line 1148
Quoted 2 lines
drivers/iio/adc/ad4170.c new file +2
-0+err_disable: + ret =ad4170_set_channel_enable (st, chan->address, false);
Avoid overwriting the first error code.
Quoted 16 lines
drivers/iio/adc/ad4170.c new file +15
-0+if (ret) +return ret; + +return IIO_VAL_INT ; +} +static int ad4170_read_avail (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +const int **vals,int *type,int *length, +long info) +{ +struct ad4170_state *st =iio_priv (indio_dev); +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +enum ad4170_filter_type f_type; + +switch (info) {
drivers/iio/adc/ad4170.c line 1297
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+case IIO_CHAN_INFO_SCALE : + *vals = (int *)chan_info->scale_tbl; + *length =ARRAY_SIZE (chan_info->scale_tbl) *2 ; + *type =IIO_VAL_INT_PLUS_NANO ; +return IIO_AVAIL_LIST ; +case IIO_CHAN_INFO_SAMP_FREQ : + f_type =ad4170_get_filter_type (indio_dev, chan); +switch (f_type) { +case AD4170_SINC5_AVG : + fallthrough;
No need for fallthrough here.
case A:
case B:
is fine without .
Quoted 17 lines
drivers/iio/adc/ad4170.c new file +17
-0+case AD4170_SINC3 : + *vals = (int *)st->sps_tbl[f_type]; + *length =ARRAY_SIZE (ad4170_sinc3_filt_fs_tbl) *2 ; +break ; +case AD4170_SINC5 : + *vals = (int *)st->sps_tbl[f_type]; + *length =ARRAY_SIZE (ad4170_sinc5_filt_fs_tbl) *2 ; +break ; +default : +return -EINVAL ; + } + *type =IIO_VAL_INT_PLUS_MICRO ; + +return IIO_AVAIL_LIST ; +default : +return -EINVAL ; + }
drivers/iio/adc/ad4170.c line 1324
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+} + +static int ad4170_set_pga (struct ad4170_state *st, +struct iio_chan_spec const *chan,int val,int val2) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +unsigned int old_pga =FIELD_GET (AD4170_AFE_PGA_GAIN_MSK , setup->afe); +unsigned int pga; +int ret =0 ;
Looks like ret is always set.
Quoted 24 lines
drivers/iio/adc/ad4170.c new file +24
-0+ +for (pga =0 ; pga <AD4170_NUM_PGA_OPTIONS ; pga++) { +if (val == chan_info->scale_tbl[pga][0 ] && + val2 == chan_info->scale_tbl[pga][1 ]) +break ; + } + +if (pga ==AD4170_NUM_PGA_OPTIONS ) +return -EINVAL ; + +if (pga == old_pga) +return 0 ; + + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK ; + setup->afe |=FIELD_PREP (AD4170_AFE_PGA_GAIN_MSK , pga); + +guard (mutex)(&st->lock); + ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) { + setup->afe &= ~AD4170_AFE_PGA_GAIN_MSK ; + setup->afe |=FIELD_PREP (AD4170_AFE_PGA_GAIN_MSK , old_pga); + } + +return ret;
drivers/iio/adc/ad4170.c line 1358
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+} + +static int ad4170_set_channel_freq (struct ad4170_state *st, +struct iio_chan_spec const *chan,int val, +int val2) +{ +struct ad4170_chan_info *chan_info = &st->chan_infos[chan->address]; +struct ad4170_setup *setup = &chan_info->setup; +enum ad4170_filter_type f_type =__ad4170_get_filter_type (setup->filter); +int filt_fs_tbl_size, i, ret =0 ;
Don't mix declarations with assignments on a line with others that don't assign Also, I think ret is always set anwyay.
Quoted 61 lines
drivers/iio/adc/ad4170.c new file +58
-0+unsigned int old_filter_fs; + +switch (f_type) { +case AD4170_SINC5_AVG : + fallthrough; +case AD4170_SINC3 : + filt_fs_tbl_size =ARRAY_SIZE (ad4170_sinc3_filt_fs_tbl); +break ; +case AD4170_SINC5 : + filt_fs_tbl_size =ARRAY_SIZE (ad4170_sinc5_filt_fs_tbl); +break ; + } + +for (i =0 ; i < filt_fs_tbl_size; i++) { +if (st->sps_tbl[f_type][i][0 ] == val && + st->sps_tbl[f_type][i][1 ] == val2) +break ; + } +if (i >= filt_fs_tbl_size) +return -EINVAL ; + + old_filter_fs = setup->filter_fs; +if (f_type ==AD4170_SINC5 ) + setup->filter_fs = ad4170_sinc5_filt_fs_tbl[i]; +else + setup->filter_fs = ad4170_sinc3_filt_fs_tbl[i]; + +guard (mutex)(&st->lock); + ret =ad4170_write_channel_setup (st, chan->address, false); +if (ret) + setup->filter_fs = old_filter_fs; + +return ret; +} + +static irqreturn_t ad4170_irq_handler (int irq,void *dev_id) +{ +struct iio_dev *indio_dev = dev_id; +struct ad4170_state *st =iio_priv (indio_dev); + +complete (&st->completion); + +return IRQ_HANDLED ; +}; + +static int ad4170_trigger_setup (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + + st->trig =devm_iio_trigger_alloc (indio_dev->dev.parent,"%s-trig%d" , + indio_dev->name, +iio_device_id (indio_dev)); +if (!st->trig) +return -ENOMEM ; + + st->trig->ops = &ad4170_trigger_ops; + st->trig->dev.parent = indio_dev->dev.parent;
drivers/iio/adc/ad4170.c
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ +iio_trigger_set_drvdata (st->trig, indio_dev); + ret =devm_iio_trigger_register (indio_dev->dev.parent, st->trig); +if (ret) +return ret; + + indio_dev->trig =iio_trigger_get (st->trig); + +return request_irq (st->spi->irq, &ad4170_irq_handler,IRQF_ONESHOT , + indio_dev->name, indio_dev);
This seems unusual. If it's a trigger, why not calling iio_trigger_poll() etc? This doesn't seem to be a trigger at all. Maybe introduce it only with buffered capture, so in the next patch. Here all you need is the interrupt registration.
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+} + +static int ad4170_regulator_setup (struct ad4170_state *st) +{ +struct device *dev = &st->spi->dev;
drivers/iio/adc/ad4170.c line 1813
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+int ret; + +/* Required regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get AVDD voltage.\n" ); + + st->vrefs_uv[AD4170_AVDD_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"iovdd" );
If no channel uses this reference is it not optional? Maybe not worth the complexity of handling that. We have sometime bothered to do so in the past by first figuring out which references are in use, then trying to get the appropriate regulators with small changes for cases like this where it needs to be enabled but we might not need the voltage.
Quoted 61 lines
drivers/iio/adc/ad4170.c new file +61
-0+if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get IOVDD voltage.\n" ); + + st->vrefs_uv[AD4170_IOVDD_SUP ] = ret; + +/* Optional regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avss" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get AVSS voltage.\n" ); + +/* Assume AVSS at GND (0V) if not provided */ + st->vrefs_uv[AD4170_AVSS_SUP ] = ret == -ENODEV ?0 : -ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1p" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN+ voltage.\n" ); + + st->vrefs_uv[AD4170_REFIN1P_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin1n" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN1N_SUP ] = ret == -ENODEV ? -ENODEV : -ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin2p" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN2+ voltage.\n" ); + + st->vrefs_uv[AD4170_REFIN2P_SUP ] = ret; + + ret =devm_regulator_get_enable_read_voltage (dev,"refin2n" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN2- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN2N_SUP ] = ret == -ENODEV ? -ENODEV : -ret; + +return 0 ; +} + +static int ad4170_probe (struct spi_device *spi) +{ +struct device *dev = &spi->dev; +struct iio_dev *indio_dev; +struct ad4170_state *st; +const char *dev_name; +int ret; + + indio_dev =devm_iio_device_alloc (&spi->dev,sizeof (*st)); +if (!indio_dev) +return -ENOMEM ; + + st =iio_priv (indio_dev); +devm_mutex_init (dev, &st->lock); + + dev_name =spi_get_device_match_data (spi); +if (!dev_name) +return -EINVAL ; +
drivers/iio/adc/ad4170.c line 1884
Quoted 10 lines
drivers/iio/adc/ad4170.c new file +10
-0+ indio_dev->name = dev_name; + indio_dev->info = &ad4170_info; + + st->spi = spi; + st->regmap8 =devm_regmap_init_spi (spi, &ad4170_regmap8_config); +if (IS_ERR (st->regmap8)) +return dev_err_probe (dev,PTR_ERR (st->regmap8), +"Failed to initialize regmap8\n" ); + + st->regmap16 =devm_regmap_init_spi (spi, &ad4170_regmap16_config);
I see Nuno suggested bulk reads for these. I'd be surprised if you can't make that work better than this complex multiple regmap case. We normallly only do this if they are genuinely variable sized registers. Here they are defined as sets of 8 bit registers.
drivers/iio/adc/ad4170.c line 1922
Quoted 11 lines
drivers/iio/adc/ad4170.c new file +10
-0+if (IS_ERR (st->regmap16)) +return dev_err_probe (dev,PTR_ERR (st->regmap16), +"Failed to initialize regmap16\n" ); + + +return devm_iio_device_register (dev, indio_dev); +} + +static const struct spi_device_id ad4170_id_table[] = { + {"ad4170" , (kernel_ulong_t )ad4170_chip_names[0 ] },
I'd be surprised if we never see anything different between these other than the names. I would just define a structure that for now just contains the const char *name
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +5
-0+ { "ad4190", (kernel_ulong_t)ad4170_chip_names[1] }, + { "ad4195", (kernel_ulong_t)ad4170_chip_names[2] }, + { } +}; +MODULE_DEVICE_TABLE (spi, ad4170_id_table);
Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Jonathan CameronQuoted 10 lines · Jonathan Cameron
On Fri, 11 Apr 2025 12:38:35 -0300
Marcelo Schmitt <[email protected]> wrote:A general efficiency comment. If you agree with a comment, no need to reply. Better to crop that bit of the thread out and reply only to those parts where there is more to add.
Cuts down on what reviewers need to read! We assume anything you don't comment on is something you are happy with changing as suggested!
+ 1000!
Quoted 3 lines
I normally only point this out to people who are sending a lot of code as it's just not worth it for one time contributors. So bad luck you get me being fussier ;)
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Jonathan CameronHi Jonathan,
Thank you for reviewing this set.
Clarifying some bits inline. Will apply all other suggested changes.
Thanks,
Marcelo
Quoted 12 lines · Jonathan Cameron
Quoted 9 lines · On Wed, 9 Apr 2025 09:24:35 -0300 Marcelo Schmitt <[email protected]>…
From: Ana-Maria Cusco <[email protected]>
Add support for the AD4170 ADC with the following features:
- Single-shot read.
- Analog front end PGA configuration.
- Digital filter and sampling frequency configuration.
- Calibration gain and offset configuration.
- Differential and pseudo-differential input configuration.
Quoted 8 lines
Quoted 3 lines
Signed-off-by Ana-Maria Cusco <[email protected]>Co-developed-by Marcelo Schmitt <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>Hi.
Clearly this is a massive driver, even with it broken up like this. So it might take a few cycles to review enough that we don't find new things :(
No worries. Yeah, I tried my best to make it concise but this is still not small piece of code.
Quoted 12 lines
I can't think of a good way to split it up further though
Jonathan
Quoted 5 lines
drivers/iio/adc/ad4170.c new file +0
-0
...
Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/ad4170.c new file +11
-0+static int ad4170_regulator_setup (struct ad4170_state *st) +{ +struct device *dev = &st->spi->dev; +int ret; + +/* Required regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get AVDD voltage.\n" ); + + st->vrefs_uv[AD4170_AVDD_SUP ] = ret;
Quoted 8 lines
drivers/iio/adc/ad4170.c line 1813
Quoted 2 lines
drivers/iio/adc/ad4170.c new file +2
-0+ + ret =devm_regulator_get_enable_read_voltage (dev,"iovdd" );If no channel uses this reference is it not optional? Maybe not worth the complexity of handling that. We have sometime bothered to do so in the past by first figuring out which references are in use, then trying to get the appropriate regulators with small changes for cases like this where it needs to be enabled but we might not need the voltage.
We can set the channel multiplexer to use IOVDD reference as diff chan negative input. Similar thing can be done for the other reference supplies. I think the examples in dt-binding don't use IOVDD but they could. Since the driver is supporting other regulators, maybe support IOVDD too?
Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/ad4170.c new file +10
-0+if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get IOVDD voltage.\n" ); + + st->vrefs_uv[AD4170_IOVDD_SUP ] = ret; + +/* Optional regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avss" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get AVSS voltage.\n" ); +
Re: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Jonathan CameronQuoted 17 lines
Quoted 10 lines
Quoted 9 lines
drivers/iio/adc/ad4170.c line 1838
Quoted 6 lines
drivers/iio/adc/ad4170.c new file +6
-0+ ret =devm_regulator_get_enable_read_voltage (dev,"refin1n" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get REFIN- voltage.\n" ); + +/* Negative supplies are assumed to provide negative voltage */ + st->vrefs_uv[AD4170_REFIN1N_SUP ] = ret == -ENODEV ? -ENODEV : -ret;Maybe to early for me but the comment does not make it clear to me why the negation? Won't the regulator return a negative voltage?
Quoted 6 lines
devm_regulator_get_enable_read_voltage(),regulator_get_voltage(), and anything about reading the regulator voltage returns either a positive voltage value or a negative error code. I couldn't find out how to read a negative voltage with regulator API. So, for now, this is making the simplifying assumption that the negative end of external reference supplies is always below GND level (even though they could be positive).
Quoted 5 lines
Hmm. We went around this a long time back but I can't remember what the outcome was...
https://lore.kernel.org/linux-iio/544AC56F16B56944AEC3BD4E3D59177137546EF3FC@LIMKCMBX1.ad.analog.com/
looks like the thread.Take a look at
dac/ad5791.cfor example of a negative reference
AD5791 references are always either only negative (V_REFNF, V_REFNS) or only positive (V_REFPS, V_REFPF) so the driver is fine with only adding a negative sign to the voltage obtained from the negative supply.
AD4170 external references can be set to a negative voltage in some setups, or to positive voltages on other setups. They're also constrained on the resulting nominal reference voltage (REFIN+ − REFIN−) which is typically 2.5V but can range from 1V minmum AVDD maximum. From that perspective, maybe this could be abstracted as a differential voltage supply? Though, the multiplexer in the chip can bet set to connect individual REFIN+, REFIN−, ..., REFIN2- supplies ends to ADC analog inputs so an API like get_voltage(reg, &voltage)) would make it easier to handle those configurations.
From the mentioned thread, I see no hard objection to extending the regulator framework in that sense so I'll start working on something to provide better support for these cases where we have negative voltage.
Thanks,
Marcelo
Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support
In reply to Nuno Sá...
Quoted 12 lines
drivers/iio/adc/ad4170.c static int ad4170_soft_reset(struct ad4170_state *st) · line 1569
Quoted 11 lines
+static int
ad4170_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)drivers/iio/adc/ad4170.c +9
-0+{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +unsigned int val; +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; +
Quoted 7 lines
This
claim_direct()makes me wonder if there's any overlap between the GPIO func and normal readings? Like, imagine a consumer requests a gpio and no buffering is happening so all is good. However, there's nothing stopping us for enabling buffering afterwards, right? Wouldn't that be an issue? If there are shared pins, I can see this also being an issue even for single shot reading... Otherwise, I wonder why we have thisiio_device_claim_direct()calls? Is it just for using the internal IIO lock?
We need read/write to AD4170 registers to configure/set/read GPIOs so the intent of claiming direct mode here is to avoid that from happening while we are doing something else (e.g. a buffered capture). I'm now considering to also lock on the state mutex to prevent concurrent run with the single-shot read routine.
Thanks,
Marcelo
Re: [PATCH v1 5/7] iio: adc: ad4170: Add GPIO controller support
In reply to Marcelo SchmittQuoted 8 lines · Marcelo Schmitt
The AD4170 has four multifunctional pins that can be used as GPIOs. The GPIO functionality can be accessed when the AD4170 chip is not busy performing continuous data capture or handling any other register read/write request. Also, the AD4170 does not provide any interrupt based on GPIO pin states so AD4170 GPIOs can't be used as interrupt sources.
Implement
gpio_chipcallbacks so to make AD4170 GPIO pins controllable
callbacks to
Quoted 1 line
through the gpiochip interface.
...
Quoted 12 lines
drivers/iio/adc/ad4170.c +12
-0+static int ad4170_gpio_direction_output (struct gpio_chip *gc, +unsigned int offset,int value) +{ +struct iio_dev *indio_dev =gpiochip_get_data (gc); +struct ad4170_state *st =iio_priv (indio_dev); +int ret; + +if (!iio_device_claim_direct (indio_dev)) +return -EBUSY ; + + ret =regmap_clear_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 ));
drivers/iio/adc/ad4170.c static int ad4170_soft_reset(struct ad4170_state *st) · line 1685
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+if (ret) + goto err_release; + + ret =regmap_set_bits (st->regmap16,AD4170_GPIO_MODE_REG , +BIT (offset *2 +1 )); + +err_release: +iio_device_release_direct (indio_dev); + +ad4170_gpio_set (gc, offset, value);
This is incorrect ordering, you will have glitches. Can you set the value beforehands? Or is it broken hardware?
Quoted 2 lines
(quoted diff) +2
-0+ return ret; +}
...
Quoted 7 lines
drivers/iio/adc/ad4170.c +6
-0+static int ad4170_gpio_init (struct iio_dev *indio_dev) +{ +struct ad4170_state *st =iio_priv (indio_dev); + st->gpiochip = (struct gpio_chip ) { + .label ="ad4170_gpios" , + .base =-1 ,
drivers/iio/adc/ad4170.c static int ad4170_soft_reset(struct ad4170_state *st) · line 1705
Quoted 10 lines
drivers/iio/adc/ad4170.c +10
-0+ .ngpio = 4, + .parent = &st->spi->dev, + .can_sleep = true, + .get_direction = ad4170_gpio_get_direction, + .direction_input = ad4170_gpio_direction_input, + .direction_output = ad4170_gpio_direction_output, + .get = ad4170_gpio_get, + .set_rv = ad4170_gpio_set, + .owner = THIS_MODULE, + };
I think it would be better to have it field by field initialised.
Quoted 2 lines
drivers/iio/adc/ad4170.c +2
-0+return devm_gpiochip_add_data (&st->spi->dev, &st->gpiochip, indio_dev); +}
...
drivers/iio/adc/ad4170.c static int ad4170_parse_firmware(struct iio_dev *indio_dev) · line 2019
Quoted 4 lines
drivers/iio/adc/ad4170.c +4
-0+/* Only create a GPIO chip if flagged for it */ +if (device_property_read_bool (&st->spi->dev,"gpio-controller" )) { + ret =ad4170_gpio_init (indio_dev); +if (ret <0 )
< 0 ? What is the meaning of the positive values that you expect from this function?
Quoted 2 lines
(quoted diff) +2
-0+ return ret; + }
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v1 7/7] iio: adc: ad4170: Add support for weigh scale and RTD sensors
In reply to Nuno Sá...
Quoted 22 lines
Quoted 22 lines
drivers/iio/adc/ad4170.c +11
-0+static int ad4170_setup_rtd (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 current_src, ret, i; + +for (i =0 ; i < num_exc_pins; i++) { +unsigned int pin = exc_pins[i]; + + current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_PIN_MSK,pin);
(quoted diff) +1
-0+ current_src |= FIELD_PREP(AD4170_CURRENT_SRC_I_OUT_VAL_MSK,
exc_cur);drivers/iio/adc/ad4170.c +8
-0+ + ret =regmap_write (st->regmap16,AD4170_CURRENT_SRC_REG (i), + current_src); +if (ret) +return ret; + } + +if (ac_excited)
Quoted 8 lines
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1891
Quoted 5 lines
drivers/iio/adc/ad4170.c +5
-0+ setup->misc |=FIELD_PREP (AD4170_MISC_CHOP_IEXC_MSK , + num_exc_pins == 2 ? 0x2 : 0x3); + +return 0 ; +}In the above I do not see any explicit GPIO configuration which makes me wonder if having the RTD is mutual exclusive with having GPIOs?
For RTD sensors, it's recommended to use a different excitation mechanism which is configured in CURRENT_SOURCE and MISCELLANEOUS registers. The current source for RTD sensor excitation could come from a GPIO, but only implemented support for outputting the current from an analog pin. Will handle the case of using GPIOs in v2.
Quoted 17 lines
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1901
Quoted 12 lines
drivers/iio/adc/ad4170.c +5
-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)drivers/iio/adc/ad4170.c +5
-0+{ +int current_src, ret, i; + +if (!ac_excited) +return 0 ;Same as above, if !ac_excited, can't we use the GPIOs? because
ad4170_validate_excitation_pins()just unconditionally setsAD4170_GPIO_AC_EXCITATION. Or maybe this DT property is only adding complexity... See below
I see, maybe AD4170_GPIO_EXCITATION would better describe the function.
...
Quoted 11 lines
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 1955
Quoted 5 lines
drivers/iio/adc/ad4170.c +5
-0+ + 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 ;Not sure if you gain much with having the funcs OR'ed like this... If I'm not missing nothing it's only about logging in
ad4170_validate_excitation_pins()? It's up to you but I would consider using bitmaps (unsigned long) for this and just test the bits.
Ah, yes, a bitmap is what was trying to do but misimplemented it. There are 4 possible functions for AD4170 GPIOs.
(1) Power-down switches (currently not supported);
(2) External sensor excitation;
(3) GPIO;
(4) CHANNEL_TO_GPIO (not eager to support that).
If a GPIO is used for external circuit excitation, we don't want to export it as a GPIO. If the GPIO were set a power-down switch we would also not expose it.
...
Quoted 11 lines
drivers/iio/adc/ad4170.c static int ad4170_gpio_init(struct iio_dev *indio_dev) · line 2004
Quoted 8 lines
drivers/iio/adc/ad4170.c +4
-0+ + ac_excited =fwnode_property_read_bool (child,"adi,ac-excited" ); + + num_exc_pins = fwnode_property_count_u32(child, "adi,excitation-pins");
drivers/iio/adc/ad4170.c +3
-0+if (num_exc_pins !=2 && num_exc_pins !=4 ) +return dev_err_probe (dev, -EINVAL , + "Invalid number of excitation pins\n");Can't we assume that a valid
num_exc_pinsproperty meansac_excited= true? Because that looks to be the logic inad4170_validate_excitation_pins().
Unfortunately, no. The user may want and set AD4170 to DC excite the external circuit. They may want to use the same set of excitation pins, but not enable channel chop or output current chop (leading to DC excitation). Well, we may choose to not support the DC case in the driver I guess, but since this is already fairly complicated, why not going an step further to support it fully?
Quoted 8 lines
Quoted 8 lines
drivers/iio/adc/ad4170.c +7
-0+ + ret =fwnode_property_read_u32_array (child,"adi,excitation-pins" , + exc_pins, num_exc_pins); +if (ret) +return dev_err_probe (dev, ret, + "Failed to read adi,excitation-pins\n"); +
...
Quoted 5 lines
Quoted 5 lines
(quoted diff) +0
-0@@ -2081,6 +2412,10 @@ static int ad4170_parse_firmware(struct iio_dev *indio_dev)
/* Only create a GPIO chip if flagged for it */
if (device_property_read_bool(&st->spi->dev, "gpio-controller")) {
Quoted 8 lines
drivers/iio/adc/ad4170.c static int ad4170_parse_firmware(struct iio_dev *indio_dev) · line 2417
Quoted 3 lines
drivers/iio/adc/ad4170.c +3
-0+for (i =0 ; i <AD4170_NUM_GPIO_PINS ; i++) +if (st->gpio_fn[i] !=AD4170_GPIO_UNASIGNED ) +return 0 ;I think you could improve this... You're taking an all or nothing approach. IIUC, we can have cases where only two GPIOs are in use which means we could use the other 2? There the gpiochio
init_valid_mask()call that you could potentially use.
Isn't gpiochio init_valid_mask() only to distinguish between GPIOs that can (or cannot) be used as interrupts? Not sure AD4170 GPIOs can be used for interrupts at all (think they can't) so didn't implement init_valid_mask().
Thanks,
Marcelo
Re: [PATCH v1 7/7] iio: adc: ad4170: Add support for weigh scale and RTD sensors
On Mon, Apr 14, 2025 at 6:37 PM Marcelo Schmitt
<[email protected]> wrote:
...
Quoted 8 lines
Quoted 4 lines
I think you could improve this... You're taking an all or nothing approach. IIUC, we can have cases where only two GPIOs are in use which means we could use the other 2? There the gpiochio
init_valid_mask()call that you could potentially use.Isn't gpiochio
init_valid_mask()only to distinguish between GPIOs that can (or cannot) be used as interrupts? Not sure AD4170 GPIOs can be used for interrupts at all (think they can't) so didn't implementinit_valid_mask().
There are two masks, one per GPIO, and one per GPIO-as-IRQ. I agree that it's better to use valid mask and not reinvent a wheel.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH v1 2/7] iio: adc: Add basic support for AD4170
In reply to Marcelo SchmittQuoted 22 lines
...
Quoted 11 lines
Quoted 11 lines
drivers/iio/adc/ad4170.c new file +11
-0+static int ad4170_regulator_setup (struct ad4170_state *st) +{ +struct device *dev = &st->spi->dev; +int ret; + +/* Required regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avdd" ); +if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get AVDD voltage.\n" ); + + st->vrefs_uv[AD4170_AVDD_SUP ] = ret;Quoted 8 lines
drivers/iio/adc/ad4170.c line 1813
Quoted 2 lines
drivers/iio/adc/ad4170.c new file +2
-0+ + ret =devm_regulator_get_enable_read_voltage (dev,"iovdd" );If no channel uses this reference is it not optional? Maybe not worth the complexity of handling that. We have sometime bothered to do so in the past by first figuring out which references are in use, then trying to get the appropriate regulators with small changes for cases like this where it needs to be enabled but we might not need the voltage.
Quoted 4 lines
We can set the channel multiplexer to use IOVDD reference as diff chan negative input. Similar thing can be done for the other reference supplies. I think the examples in dt-binding don't use IOVDD but they could. Since the driver is supporting other regulators, maybe support IOVDD too?
That's fine. In a few drivers where we have muxes that can use many different regulators, we allow for regulators to not be provided if no channels use them. It may not be worth the effort. If anyone comes along later with a board wired that way then we can relax the requirement at that point.
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/iio/adc/ad4170.c new file +10
-0+if (ret <0 ) +return dev_err_probe (dev, ret,"Failed to get IOVDD voltage.\n" ); + + st->vrefs_uv[AD4170_IOVDD_SUP ] = ret; + +/* Optional regulators */ + ret =devm_regulator_get_enable_read_voltage (dev,"avss" ); +if (ret <0 && ret != -ENODEV ) +return dev_err_probe (dev, ret,"Failed to get AVSS voltage.\n" ); +