linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 0/9] Add support for AD411x
@ 2024-06-06 16:07 Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 1/9] dt-bindings: iio: adc: Add common-mode-channel property Dumitru Ceclan via B4 Relay
                   ` (8 more replies)
  0 siblings, 9 replies; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan, Conor Dooley,
	Jonathan Cameron, Nuno Sa

This patch series adds support for the Analog Devices AD4111, AD4112,
 AD4114, AD4115, AD4116 within the existing AD7173 driver.

  The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications. They integrate an analog front end suitable for processing
fully differential/single-ended and bipolar voltage inputs.

Particularities of the models:
- All ADCs have inputs with a precision voltage divider with a division
ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
shunt resistor.

Discussions from this patch series have concluded with:
-Datasheets mention single-ended and pseudo differential capabilities by
 the means of connecting the negative input of a differential pair (IN-)
 to a constant voltage supply and letting the positive input fluctuate.
 This is not a special operating mode, it is a capability of the
 differential channels to also measure such signals.

-Single-ended and pseudo differential do not need any specific
 configuration and cannot be differentiated from differential usage by
 the driver side =>
	offer adi,channel-type attribute to flag the usage of the channel

-VINCOM is described as a dedicated pin for single-ended channels but as
 seen in AD4116, it is a normal input connected to the cross-point
 multiplexer (VIN10, VINCOM (single-ended or differential pair)).
 This does not mean full functionality in any configuration:
 AD4111:"If any two voltage inputs are paired in a configuration other
 than what is described in this data sheet, the accuracy of the device
 cannot be guaranteed".

-ADCIN15 input pin from AD4116 is specified as the dedicated pin for
 pseudo-differential but from the datasheet it results that this pin is
 also able to measure single-ended and fully differential channels
 ("ADCIN11, ADCIN15. (pseudo differential or differential pair)";
  "An example is to connect the ADCIN15 pin externally to the AVSS
   pin in a single-ended configuration")

 As such, detecting the type of usage of a channel is not possible and
will be the responsibility of the user to specify.
 If the user has connected a non 0V (in regards to AVSS) supply to
the negative input pin of a channel in a pseudo differential
configuration, the offset of the measurement from AVSS will not be known
from the driver and will need to be measured by other means.

Datasheets:
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf

This series depends on patches:
(iio: adc: ad7173: Use device_for_each_child_node_scoped() to simplify error paths.)
https://lore.kernel.org/all/20240330190849.1321065-6-jic23@kernel.org
(dt-bindings: iio: adc: Add single-channel property)
https://lore.kernel.org/linux-iio/20240514120222.56488-5-alisa.roman@analog.com/

And patch series:
(AD7173 fixes)
https://lore.kernel.org/all/20240521-ad7173-fixes-v1-0-8161cc7f3ad1@analog.com/

Signed-off-by: Dumitru Ceclan <mitrutzceclan@gmail.com>
---
Changes in v6:
dt-bindings: iio: adc: Add common-mode-channel property
- pick up rb tag

dt-bindings: adc: ad7173: add support for ad411x
- use ref flag instead of type: boolean
- document (AVDD-AVSS)/5 as a power supply monitoring option

iio: adc: ad_sigma_delta: add disable_one callback
iio: adc: ad7173: refactor channel configuration parsing
iio: adc: ad7173: refactor ain and vref selection
<no changes>

iio: adc: ad7173: add support for special inputs
- rename (AVDD-AVSS)/5 from common input to pow_mon
- restrict (AVDD-AVSS)/5 to only be correctly paired as
  (AVDD-AVSS)/5+ 0 to ain0 (AVSS-AVDD)/5- to ain1

iio: adc: ad7173: refactor device info structs
- add space before '}' in device tables

iio: adc: ad7173: document sampling frequency behaviour
<no changes>

iio: adc: ad7173: Add support for AD411x devices
- fix typo
- drop current channels enum
- describe channel inputs array with comments
- check for vincom input support when setting special_input
- use fwnode_property_read_u32 instead of array for single value reads
- drop remnant of previous version is_current_chan value setting
- set ain value directly for current channels
- add space before '}' in device tables

- Link to v5: https://lore.kernel.org/r/20240603-ad4111-v5-0-9a9c54d9ac78@analog.com

Changes in v5:
dt-bindings: iio: adc: Add common-mode-channel property
- create patch

dt-bindings: adc: ad7173: add support for ad411x
- remove "adi" from common-mode-channel property

iio: adc: ad_sigma_delta: add disable_one callback
- create patch

iio: adc: ad7173: refactor channel configuration parsing
<no changes>

iio: adc: ad7173: refactor ain and vref selection
- change function header to send ain[0] and ain[1] as two arguments
- drop loop
- drop reviewed-by tag

iio: adc: ad7173: add support for special inputs
- unroll the loop in the ain validation function
- drop reviewed-by tag

iio: adc: ad7173: refactor device info structs
<no changes>

iio: adc: ad7173: document sampling frequency behaviour
- create patch

iio: adc: ad7173: Add support for AD411x devices
- rename VINCOM define from wildcard to specific models
- sampling frequency comment: "to" -> "for"
- don't set ain[1] for a second time in channel_parse, set
  chan->channel2 value directly if the channel is a current channel
- revert white space change in channel config parse
- unroll the loop in the ain validation function
- use new common-mode-channel property name
- remove sampling frequency comment as included in a previous patch

- Link to v4: https://lore.kernel.org/r/20240531-ad4111-v4-0-64607301c057@analog.com

Changes in v4:
dt-bindings: adc: ad7173: add support for ad411x
- remove "adi,channel-type"
- add "adi,common-mode-input" and "adi,current-channel" to use
  single-channel with both single-ended/pseudo-differential and current
  channels
- add restrictions to patternProperties channel to restrict presence of
  both diff-channels and single-channel
   and
  both "adi,current-channel" and "adi,common-mode-input"
- update diff-channels description
- update commit message to current state of the binding
- add a second example to exemplify single-ended and current channels

iio: adc: ad7173: refactor channel configuration parsing
- picked up reviewed-by tag

iio: adc: ad7173: refactor ain and vref selection
- Moved reference error message from validate_reference() to
  ad7173_get_ref_voltage_milli()
- Changed from dev_err_probe to dev_err as function can be reached from
  non probe paths
- added AD7173_NO_AINS_PER_CHANNEL to remove ambiguity when using the
  size of the ain array

iio: adc: ad7173: add support for special inputs
- picked up reviewed-by tag

iio: adc: ad7173: refactor device info structs
- create patch

iio: adc: ad7173: Add support for AD411x devices
- separate chip id defines for ad411<1,2,4>
- fix device_info typos: voltage, VINCOM
- rename num_voltage_inputs and num_voltage_inputs_with_divider to *_voltage_in*
- subtract ch->cfg.bipolar directly
- change to const ain argument in *_validate_current_ain()
- change parsing to new dt structure for current and single-ended channels
- drop adi,channel-type
- refactor device info structs as the previous patch

iio: adc: ad7173: Reduce device info struct size
- remove patch as suggested by David Lechner as it would increase binary
  size more than the savings done in RAM

- Link to v3: https://lore.kernel.org/r/20240527-ad4111-v3-0-7e9eddbbd3eb@analog.com

Changes in v3:

iio: adc: ad7173: fix buffers enablement for ad7176-2
iio: adc: ad7173: Add ad7173_device_info names
iio: adc: ad7173: Remove index from temp channel
- Remove patches, send in separate series

dt-bindings: adc: ad7173: add support for ad411x
- fix VINCOM typo
- switch current channel definition to use single-channel
- remove pseudo-differential from adi,channel-type, specify that
  single-ended will be used for that case as well
- remove diff-channels from required, already defined in adc.yaml
- update constraints to not permit single-channel for models without
  current inputs

iio: adc: ad7173: refactor channel configuration parsing
- remove blank line from commit message

iio: adc: ad7173: refactor ain and vref selection
- remove blank space from commit message

iio: adc: ad7173: add support for special inputs
<no changes>

iio: adc: ad7173: Add support for AD411x devices
- remove pseudo diff channel type
- change current channels dt parsing to single-channel
- change module description from wild-card to "and similar"
- add comment to document HW behavior when multiple channels are enabled
  in buffered reading mode

iio: adc: ad7173: Reduce device info struct size
<no changes>

- Link to v2: https://lore.kernel.org/r/20240514-ad4111-v2-0-29be6a55efb5@analog.com

Changes in v2:

dt-bindings: adc: ad7173: add support for ad411x
- Add constraint for missing avdd2-supply on ad411x
- Change support for current channels to selecting the actual
   diff-channels input values and activating the
   adi,current-channel property
- Add constraint for adi,current-channel
- Add adi,channel-type to be able to differentiante in the driver
   between single-ended, pseudo-differential and differential channels.
- Update diff-channels description to decribe inputs beside the AINs

iio: adc: ad7173: fix buffers enablement for ad7176-2
- Specify ".has_input_buf = false" for AD7176-2
- Drop fixes tag, specify that configuration bits are read only

iio: adc: ad7173: refactor channel configuration parsing
- Add Link and Suggested-by in commit message

iio: adc: ad7173: refactor ain and vref selection
- Improve commit message to express commit purpose
- Refactor line wrappings due to reduced indent
- Change AINs check to a loop

iio: adc: ad7173: add support for special inputs
- Create patch

iio: adc: ad7173: Add ad7173_device_info names
- Create patch

iio: adc: ad7173: Remove index from temp channel
- Justify in commit message userspace breakage
- Remove index from the correct channel template

iio: adc: ad7173: Add support for AD411x devices
- Add missing validation for VCOM and inputs with voltage divider
- Add missing validation for AD4116 low level inputs
- Add missing ad7173_device_info names
- Add support for setting differential flag depending on the channel type
- Change current channel validation to use actual pin values
- Combine multiple chipID reg values in a single define
		(AD7173_AD4111_AD4112_AD4114_ID)
- Rename num_inputs and num_inputs_with_divider to include voltage
- Add comment to specify that num_voltage_inputs_with_divider does not
   include the VCOM pin.
- Change break to direct returns where possible in switch cases
- Add fix for ad411x gpio's

iio: adc: ad7173: Reduce device info struct size
- Create patch

- Link to v1: https://lore.kernel.org/r/20240401-ad4111-v1-0-34618a9cc502@analog.com

---
Dumitru Ceclan (9):
      dt-bindings: iio: adc: Add common-mode-channel property
      dt-bindings: adc: ad7173: add support for ad411x
      iio: adc: ad_sigma_delta: add disable_one callback
      iio: adc: ad7173: refactor channel configuration parsing
      iio: adc: ad7173: refactor ain and vref selection
      iio: adc: ad7173: add support for special inputs
      iio: adc: ad7173: refactor device info structs
      iio: adc: ad7173: document sampling frequency behaviour
      iio: adc: ad7173: Add support for AD411x devices

 Documentation/devicetree/bindings/iio/adc/adc.yaml |  11 +
 .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 194 +++++-
 drivers/iio/adc/ad7124.c                           |  14 +-
 drivers/iio/adc/ad7173.c                           | 688 +++++++++++++++------
 drivers/iio/adc/ad_sigma_delta.c                   |   6 +
 include/linux/iio/adc/ad_sigma_delta.h             |  14 +
 6 files changed, 731 insertions(+), 196 deletions(-)
---
base-commit: 02b664413a44903ef349bb70a3f1842cbcee9616
change-id: 20240312-ad4111-7eeb34eb4a5f

Best regards,
-- 
Dumitru Ceclan <dumitru.ceclan@analog.com>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH v6 1/9] dt-bindings: iio: adc: Add common-mode-channel property
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x Dumitru Ceclan via B4 Relay
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan, Conor Dooley

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

There are ADCs that are differential but support to measure single-ended
signals on the same channels by connecting a constant voltage to the
negative input pin.

This property allows to properly define a single-ended channel that
requires two inputs to be specified. Software can use the presence of
this property to mark the channel as not differential.

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 Documentation/devicetree/bindings/iio/adc/adc.yaml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/adc.yaml b/Documentation/devicetree/bindings/iio/adc/adc.yaml
index 0a77592f7388..8e7835cf36fd 100644
--- a/Documentation/devicetree/bindings/iio/adc/adc.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adc.yaml
@@ -46,6 +46,17 @@ properties:
       differential channels). If this and diff-channels are not present reg
       shall be used instead.
 
+  common-mode-channel:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      Some ADCs have differential input pins that can be used to measure
+      single-ended or pseudo-differential inputs. This property can be used
+      in addition to single-channel to signal software that this channel is
+      not differential but still specify two inputs.
+
+      The input pair is specified by setting single-channel to the positive
+      input pin and common-mode-channel to the negative pin.
+
   settling-time-us:
     description:
       Time between enabling the channel and first stable readings.

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 1/9] dt-bindings: iio: adc: Add common-mode-channel property Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-06 16:38   ` Conor Dooley
  2024-06-06 16:07 ` [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback Dumitru Ceclan via B4 Relay
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.

AD411x family ADCs support a VCOM pin. The purpose of this pin is to
offer a dedicated common-mode voltage input for single-ended channels.
This pin is specified as supporting a differential channel with VIN10 on
model AD4116.

AD4111/AD4112 support current channels. Support is implemented using
single-channel and "adi,current-channel".

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 .../devicetree/bindings/iio/adc/adi,ad7173.yaml    | 194 ++++++++++++++++++++-
 1 file changed, 192 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
index ea6cfcd0aff4..17c5d39cc2c1 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
@@ -19,7 +19,18 @@ description: |
   primarily for measurement of signals close to DC but also delivers
   outstanding performance with input bandwidths out to ~10kHz.
 
+  Analog Devices AD411x ADC's:
+  The AD411X family encompasses a series of low power, low noise, 24-bit,
+  sigma-delta analog-to-digital converters that offer a versatile range of
+  specifications. They integrate an analog front end suitable for processing
+  fully differential/single-ended and bipolar voltage inputs.
+
   Datasheets for supported chips:
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4111.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4112.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4114.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4115.pdf
+    https://www.analog.com/media/en/technical-documentation/data-sheets/AD4116.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-4.pdf
     https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
@@ -31,6 +42,11 @@ description: |
 properties:
   compatible:
     enum:
+      - adi,ad4111
+      - adi,ad4112
+      - adi,ad4114
+      - adi,ad4115
+      - adi,ad4116
       - adi,ad7172-2
       - adi,ad7172-4
       - adi,ad7173-8
@@ -129,10 +145,56 @@ patternProperties:
         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.
+
+          Family AD411x supports a dedicated VINCOM voltage input.
+          To select it set the second channel to 16.
+            (VIN2, VINCOM) -> diff-channels = <2 16>
+
+          There are special values that can be selected besides the voltage
+          analog inputs:
+            21: REF+
+            22: REF−
+
+          Supported only by AD7172-2, AD7172-4, AD7175-2, AD7175-8, AD7177-2,
+          must be paired together and can be used to monitor the power supply
+          of the ADC:
+            19: ((AVDD1 − AVSS)/5)+
+            20: ((AVDD1 − AVSS)/5)−
+
         items:
           minimum: 0
           maximum: 31
 
+      single-channel:
+        description: |
+          This property is used for defining a current channel or the positive
+          input of a voltage channel (single-ended or pseudo-differential).
+
+          Models AD4111 and AD4112 support current channels.
+            Example: (IIN2+, IIN2−) -> single-channel = <2>
+          To correctly configure a current channel set the "adi,current-channel"
+          property to true.
+
+          To configure a single-ended/pseudo-differential channel set the
+          "common-mode-channel" property to the desired negative voltage input.
+
+          When used as a voltage channel, special inputs are valid as well.
+        minimum: 0
+        maximum: 31
+
+      common-mode-channel:
+        description:
+          This property is used for defining the negative input of a
+          single-ended or pseudo-differential voltage channel.
+
+          Special inputs are valid as well.
+        minimum: 0
+        maximum: 31
+
       adi,reference-select:
         description: |
           Select the reference source to use when converting on
@@ -154,9 +216,31 @@ patternProperties:
           - avdd
         default: refout-avss
 
+      adi,current-channel:
+        $ref: /schemas/types.yaml#/definitions/flag
+        description: |
+          Signal that the selected inputs are current channels.
+          Only available on AD4111 and AD4112.
+
     required:
       - reg
-      - diff-channels
+
+    allOf:
+      - oneOf:
+          - required: [single-channel]
+            properties:
+              diff-channels: false
+          - required: [diff-channels]
+            properties:
+              single-channel: false
+              adi,current-channel: false
+              common-mode-channel: false
+
+      - if:
+          required: [common-mode-channel]
+        then:
+          properties:
+            adi,current-channel: false
 
 required:
   - compatible
@@ -166,7 +250,6 @@ allOf:
   - $ref: /schemas/spi/spi-peripheral-props.yaml#
 
   # Only ad7172-4, ad7173-8 and ad7175-8 support vref2
-  # Other models have [0-3] channel registers
   - if:
       properties:
         compatible:
@@ -187,6 +270,37 @@ allOf:
                 - vref
                 - refout-avss
                 - avdd
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad4114
+              - adi,ad4115
+              - adi,ad4116
+              - adi,ad7173-8
+              - adi,ad7175-8
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            reg:
+              maximum: 15
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad7172-2
+              - adi,ad7175-2
+              - adi,ad7176-2
+              - adi,ad7177-2
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
             reg:
               maximum: 3
 
@@ -210,6 +324,34 @@ allOf:
           required:
             - adi,reference-select
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad4111
+              - adi,ad4112
+              - adi,ad4114
+              - adi,ad4115
+              - adi,ad4116
+    then:
+      properties:
+        avdd2-supply: false
+
+  - if:
+      properties:
+        compatible:
+          not:
+            contains:
+              enum:
+                - adi,ad4111
+                - adi,ad4112
+    then:
+      patternProperties:
+        "^channel@[0-9a-f]$":
+          properties:
+            adi,current-channel: false
+
   - if:
       anyOf:
         - required: [clock-names]
@@ -221,6 +363,7 @@ allOf:
 unevaluatedProperties: false
 
 examples:
+  # Example AD7173-8 with external reference connected to REF+/REF-:
   - |
     #include <dt-bindings/gpio/gpio.h>
     #include <dt-bindings/interrupt-controller/irq.h>
@@ -277,3 +420,50 @@ examples:
         };
       };
     };
+
+  # Example AD4111 with current channel and single-ended channel:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+       adc@0 {
+        compatible = "adi,ad4111";
+        reg = <0>;
+
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        interrupts = <25 IRQ_TYPE_EDGE_FALLING>;
+        interrupt-names = "rdy";
+        interrupt-parent = <&gpio>;
+        spi-max-frequency = <5000000>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        #clock-cells = <0>;
+
+        channel@0 {
+          reg = <0>;
+          bipolar;
+          diff-channels = <4 5>;
+        };
+
+        // Single ended channel VIN2/VINCOM
+        channel@1 {
+          reg = <1>;
+          bipolar;
+          single-channel = <2>;
+          common-mode-channel = <16>;
+        };
+
+        // Current channel IN2+/IN2-
+        channel@2 {
+          reg = <2>;
+          single-channel = <2>;
+          adi,current-channel;
+        };
+      };
+    };

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 1/9] dt-bindings: iio: adc: Add common-mode-channel property Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:02   ` Nuno Sá
  2024-06-06 16:07 ` [PATCH v6 4/9] iio: adc: ad7173: refactor channel configuration parsing Dumitru Ceclan via B4 Relay
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Sigma delta ADCs with a sequencer need to disable the previously enabled
channel when reading using ad_sigma_delta_single_conversion(). This was
done manually in drivers for devices with sequencers.

This patch implements handling of single channel disabling after a
single conversion.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7124.c               | 14 ++++++++------
 drivers/iio/adc/ad7173.c               | 11 ++++++-----
 drivers/iio/adc/ad_sigma_delta.c       |  6 ++++++
 include/linux/iio/adc/ad_sigma_delta.h | 14 ++++++++++++++
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ad7124.c b/drivers/iio/adc/ad7124.c
index e7b1d517d3de..3beed78496c5 100644
--- a/drivers/iio/adc/ad7124.c
+++ b/drivers/iio/adc/ad7124.c
@@ -555,10 +555,18 @@ static int ad7124_disable_all(struct ad_sigma_delta *sd)
 	return 0;
 }
 
+static int ad7124_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
+{
+	struct ad7124_state *st = container_of(sd, struct ad7124_state, sd);
+
+	return ad7124_spi_write_mask(st, AD7124_CHANNEL(chan), AD7124_CHANNEL_EN_MSK, 0, 2);
+}
+
 static const struct ad_sigma_delta_info ad7124_sigma_delta_info = {
 	.set_channel = ad7124_set_channel,
 	.append_status = ad7124_append_status,
 	.disable_all = ad7124_disable_all,
+	.disable_one = ad7124_disable_one,
 	.set_mode = ad7124_set_mode,
 	.has_registers = true,
 	.addr_shift = 0,
@@ -582,12 +590,6 @@ static int ad7124_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		/* After the conversion is performed, disable the channel */
-		ret = ad_sd_write_reg(&st->sd, AD7124_CHANNEL(chan->address), 2,
-				      st->channels[chan->address].ain | AD7124_CHANNEL_EN(0));
-		if (ret < 0)
-			return ret;
-
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		mutex_lock(&st->cfgs_lock);
diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 638e2468efbf..f3088e8b4b8b 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -569,10 +569,16 @@ static int ad7173_disable_all(struct ad_sigma_delta *sd)
 	return 0;
 }
 
+static int ad7173_disable_one(struct ad_sigma_delta *sd, unsigned int chan)
+{
+	return ad_sd_write_reg(sd, AD7173_REG_CH(chan), 2, 0);
+}
+
 static struct ad_sigma_delta_info ad7173_sigma_delta_info = {
 	.set_channel = ad7173_set_channel,
 	.append_status = ad7173_append_status,
 	.disable_all = ad7173_disable_all,
+	.disable_one = ad7173_disable_one,
 	.set_mode = ad7173_set_mode,
 	.has_registers = true,
 	.addr_shift = 0,
@@ -668,11 +674,6 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 		if (ret < 0)
 			return ret;
 
-		/* disable channel after single conversion */
-		ret = ad_sd_write_reg(&st->sd, AD7173_REG_CH(chan->address), 2, 0);
-		if (ret < 0)
-			return ret;
-
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_TEMP) {
diff --git a/drivers/iio/adc/ad_sigma_delta.c b/drivers/iio/adc/ad_sigma_delta.c
index 97a05f325df7..ec34b3d1336f 100644
--- a/drivers/iio/adc/ad_sigma_delta.c
+++ b/drivers/iio/adc/ad_sigma_delta.c
@@ -321,6 +321,7 @@ int ad_sigma_delta_single_conversion(struct iio_dev *indio_dev,
 
 	sigma_delta->keep_cs_asserted = false;
 	ad_sigma_delta_set_mode(sigma_delta, AD_SD_MODE_IDLE);
+	ad_sigma_delta_disable_one(sigma_delta, chan->address);
 	sigma_delta->bus_locked = false;
 	spi_bus_unlock(sigma_delta->spi->controller);
 	iio_device_release_direct_mode(indio_dev);
@@ -671,6 +672,11 @@ int ad_sd_init(struct ad_sigma_delta *sigma_delta, struct iio_dev *indio_dev,
 			dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_all().\n");
 			return -EINVAL;
 		}
+
+		if (!info->disable_one) {
+			dev_err(&spi->dev, "ad_sigma_delta_info lacks disable_one().\n");
+			return -EINVAL;
+		}
 	}
 
 	if (info->irq_line)
diff --git a/include/linux/iio/adc/ad_sigma_delta.h b/include/linux/iio/adc/ad_sigma_delta.h
index 383614ebd760..f8c1d2505940 100644
--- a/include/linux/iio/adc/ad_sigma_delta.h
+++ b/include/linux/iio/adc/ad_sigma_delta.h
@@ -37,6 +37,10 @@ struct iio_dev;
  * @append_status: Will be called to enable status append at the end of the sample, may be NULL.
  * @set_mode: Will be called to select the current mode, may be NULL.
  * @disable_all: Will be called to disable all channels, may be NULL.
+ * @disable_one: Will be called to disable a single channel after
+ *		ad_sigma_delta_single_conversion(), may be NULL.
+ *		Usage of this callback expects iio_chan_spec.address to contain
+ *		the value required for the driver to identify the channel.
  * @postprocess_sample: Is called for each sampled data word, can be used to
  *		modify or drop the sample data, it, may be NULL.
  * @has_registers: true if the device has writable and readable registers, false
@@ -55,6 +59,7 @@ struct ad_sigma_delta_info {
 	int (*append_status)(struct ad_sigma_delta *, bool append);
 	int (*set_mode)(struct ad_sigma_delta *, enum ad_sigma_delta_mode mode);
 	int (*disable_all)(struct ad_sigma_delta *);
+	int (*disable_one)(struct ad_sigma_delta *, unsigned int chan);
 	int (*postprocess_sample)(struct ad_sigma_delta *, unsigned int raw_sample);
 	bool has_registers;
 	unsigned int addr_shift;
@@ -140,6 +145,15 @@ static inline int ad_sigma_delta_disable_all(struct ad_sigma_delta *sd)
 	return 0;
 }
 
+static inline int ad_sigma_delta_disable_one(struct ad_sigma_delta *sd,
+					     unsigned int chan)
+{
+	if (sd->info->disable_one)
+		return sd->info->disable_one(sd, chan);
+
+	return 0;
+}
+
 static inline int ad_sigma_delta_set_mode(struct ad_sigma_delta *sd,
 	unsigned int mode)
 {

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 4/9] iio: adc: ad7173: refactor channel configuration parsing
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (2 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection Dumitru Ceclan via B4 Relay
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan,
	Jonathan Cameron, Nuno Sa

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Move configurations regarding number of channels from
*_fw_parse_device_config to *_fw_parse_channel_config.

Suggested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Link: https://lore.kernel.org/all/20240303162148.3ad91aa2@jic23-huawei/
Reviewed-by: David Lechner <dlechner@baylibre.com>
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index f3088e8b4b8b..8631f218b69e 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -913,7 +913,23 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
 	unsigned int ain[2], chan_index = 0;
-	int ref_sel, ret;
+	int ref_sel, ret, num_channels;
+
+	num_channels = device_get_child_node_count(dev);
+
+	if (st->info->has_temp)
+		num_channels++;
+
+	if (num_channels == 0)
+		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
+
+	if (num_channels > st->info->num_channels)
+		return dev_err_probe(dev, -EINVAL,
+			"Too many channels specified. Maximum is %d, not including temperature channel if supported.\n",
+			st->info->num_channels);
+
+	indio_dev->num_channels = num_channels;
+	st->num_channels = num_channels;
 
 	chan_arr = devm_kcalloc(dev, sizeof(*indio_dev->channels),
 				st->num_channels, GFP_KERNEL);
@@ -1008,7 +1024,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_state *st = iio_priv(indio_dev);
 	struct device *dev = indio_dev->dev.parent;
-	unsigned int num_channels;
 	int ret;
 
 	st->regulators[0].supply = ad7173_ref_sel_str[AD7173_SETUP_REF_SEL_EXT_REF];
@@ -1067,16 +1082,6 @@ static int ad7173_fw_parse_device_config(struct iio_dev *indio_dev)
 
 	ad7173_sigma_delta_info.irq_line = ret;
 
-	num_channels = device_get_child_node_count(dev);
-
-	if (st->info->has_temp)
-		num_channels++;
-
-	if (num_channels == 0)
-		return dev_err_probe(dev, -ENODATA, "No channels specified\n");
-	indio_dev->num_channels = num_channels;
-	st->num_channels = num_channels;
-
 	return ad7173_fw_parse_channel_config(indio_dev);
 }
 

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (3 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 4/9] iio: adc: ad7173: refactor channel configuration parsing Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:04   ` Nuno Sá
  2024-06-06 16:07 ` [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs Dumitru Ceclan via B4 Relay
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Move validation of analog inputs and reference voltage selection to
separate functions to reduce the size of the channel config parsing
function and improve readability.
Add defines for the number of analog inputs in a channel.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8631f218b69e..4040edbd1c32 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -60,6 +60,7 @@
 #define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
 #define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
 
+#define AD7173_NO_AINS_PER_CHANNEL	2
 #define AD7173_CH_ADDRESS(pos, neg) \
 	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
 	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
@@ -629,6 +630,7 @@ static int ad7173_setup(struct iio_dev *indio_dev)
 static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
 						 u8 reference_select)
 {
+	struct device *dev = &st->sd.spi->dev;
 	int vref;
 
 	switch (reference_select) {
@@ -652,9 +654,11 @@ static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
 		return -EINVAL;
 	}
 
-	if (vref < 0)
+	if (vref < 0) {
+		dev_err(dev, "Cannot use reference %u. Error:%d\n",
+			reference_select, vref);
 		return vref;
-
+	}
 	return vref / (MICRO / MILLI);
 }
 
@@ -906,13 +910,47 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
 					   &st->int_clk_hw);
 }
 
+static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
+					      unsigned int ain0, unsigned int ain1)
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	if (ain0 >= st->info->num_inputs ||
+	    ain1 >= st->info->num_inputs)
+		return dev_err_probe(dev, -EINVAL,
+				     "Input pin number out of range for pair (%d %d).\n",
+				     ain0, ain1);
+
+	return 0;
+}
+
+static int ad7173_validate_reference(struct ad7173_state *st, int ref_sel)
+{
+	struct device *dev = &st->sd.spi->dev;
+	int ret;
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF && !st->info->has_int_ref)
+		return dev_err_probe(dev, -EINVAL,
+			"Internal reference is not available on current model.\n");
+
+	if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
+		return dev_err_probe(dev, -EINVAL,
+			"External reference 2 is not available on current model.\n");
+
+	ret = ad7173_get_ref_voltage_milli(st, ref_sel);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 {
 	struct ad7173_channel *chans_st_arr, *chan_st_priv;
 	struct ad7173_state *st = iio_priv(indio_dev);
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
-	unsigned int ain[2], chan_index = 0;
+	unsigned int ain[AD7173_NO_AINS_PER_CHANNEL], chan_index = 0;
 	int ref_sel, ret, num_channels;
 
 	num_channels = device_get_child_node_count(dev);
@@ -966,11 +1004,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		if (ret)
 			return ret;
 
-		if (ain[0] >= st->info->num_inputs ||
-		    ain[1] >= st->info->num_inputs)
-			return dev_err_probe(dev, -EINVAL,
-				"Input pin number out of range for pair (%d %d).\n",
-				ain[0], ain[1]);
+		ret = ad7173_validate_voltage_ain_inputs(st, ain[0], ain[1]);
+		if (ret)
+			return ret;
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -981,19 +1017,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		else
 			ref_sel = ret;
 
-		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF &&
-		    !st->info->has_int_ref)
-			return dev_err_probe(dev, -EINVAL,
-				"Internal reference is not available on current model.\n");
-
-		if (ref_sel == AD7173_SETUP_REF_SEL_EXT_REF2 && !st->info->has_ref2)
-			return dev_err_probe(dev, -EINVAL,
-				"External reference 2 is not available on current model.\n");
-
-		ret = ad7173_get_ref_voltage_milli(st, ref_sel);
-		if (ret < 0)
-			return dev_err_probe(dev, ret,
-					     "Cannot use reference %u\n", ref_sel);
+		ret = ad7173_validate_reference(st, ref_sel);
+		if (ret)
+			return ret;
 
 		if (ref_sel == AD7173_SETUP_REF_SEL_INT_REF)
 			st->adc_mode |= AD7173_ADC_MODE_REF_EN;

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (4 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:06   ` Nuno Sá
  2024-06-06 16:07 ` [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs Dumitru Ceclan via B4 Relay
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

 Add support for selecting REF+ and REF- inputs on all models.
 Add support for selecting ((AVDD1 − AVSS)/5) inputs
  on supported models.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 4040edbd1c32..d16fa081a285 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -66,6 +66,13 @@
 	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
 #define AD7173_AIN_TEMP_POS	17
 #define AD7173_AIN_TEMP_NEG	18
+#define AD7173_AIN_POW_MON_POS	19
+#define AD7173_AIN_POW_MON_NEG	20
+#define AD7173_AIN_REF_POS	21
+#define AD7173_AIN_REF_NEG	22
+
+#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
+					(x) == AD7173_AIN_REF_NEG)
 
 #define AD7172_2_ID			0x00d0
 #define AD7175_ID			0x0cd0
@@ -146,6 +153,8 @@ struct ad7173_device_info {
 	unsigned int id;
 	char *name;
 	bool has_temp;
+	/* ((AVDD1 − AVSS)/5) */
+	bool has_pow_supply_monitoring;
 	bool has_input_buf;
 	bool has_int_ref;
 	bool has_ref2;
@@ -216,6 +225,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -230,6 +240,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -245,6 +256,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = false,
 		.clock = 2 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7173_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
@@ -259,6 +271,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -274,6 +287,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_input_buf = true,
 		.has_int_ref = true,
 		.has_ref2 = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -288,6 +302,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = false,
 		.has_input_buf = false,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = false,
 		.clock = 16 * HZ_PER_MHZ,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
 		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
@@ -302,6 +317,7 @@ static const struct ad7173_device_info ad7173_device_info[] = {
 		.has_temp = true,
 		.has_input_buf = true,
 		.has_int_ref = true,
+		.has_pow_supply_monitoring = true,
 		.clock = 16 * HZ_PER_MHZ,
 		.odr_start_value = AD7177_ODR_START_VALUE,
 		.sinc5_data_rates = ad7175_sinc5_data_rates,
@@ -914,9 +930,18 @@ static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 					      unsigned int ain0, unsigned int ain1)
 {
 	struct device *dev = &st->sd.spi->dev;
+	bool special_input0, special_input1;
+
+	/* (AVDD1-AVSS)/5 power supply monitoring */
+	if (ain0 == AD7173_AIN_POW_MON_POS && ain1 == AD7173_AIN_POW_MON_NEG &&
+	    st->info->has_pow_supply_monitoring)
+		return 0;
+
+	special_input0 = AD7173_IS_REF_INPUT(ain0);
+	special_input1 = AD7173_IS_REF_INPUT(ain1);
 
-	if (ain0 >= st->info->num_inputs ||
-	    ain1 >= st->info->num_inputs)
+	if ((ain0 >= st->info->num_inputs && !special_input0) ||
+	    (ain1 >= st->info->num_inputs && !special_input1))
 		return dev_err_probe(dev, -EINVAL,
 				     "Input pin number out of range for pair (%d %d).\n",
 				     ain0, ain1);

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (5 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:08   ` Nuno Sá
  2024-06-06 16:07 ` [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour Dumitru Ceclan via B4 Relay
  2024-06-06 16:07 ` [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices Dumitru Ceclan via B4 Relay
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Drop array of device info structs and use individual structs for all;
drop models enum as no longer needed. This improves readability as the
structs are pointed directly.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 267 ++++++++++++++++++++++-------------------------
 1 file changed, 127 insertions(+), 140 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index d16fa081a285..8d008186cd6e 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -132,16 +132,6 @@
 #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
 #define AD7173_MAX_CONFIGS		8
 
-enum ad7173_ids {
-	ID_AD7172_2,
-	ID_AD7172_4,
-	ID_AD7173_8,
-	ID_AD7175_2,
-	ID_AD7175_8,
-	ID_AD7176_2,
-	ID_AD7177_2,
-};
-
 struct ad7173_device_info {
 	const unsigned int *sinc5_data_rates;
 	unsigned int num_sinc5_data_rates;
@@ -214,115 +204,119 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
 	5000,					/* 20    */
 };
 
-static const struct ad7173_device_info ad7173_device_info[] = {
-	[ID_AD7172_2] = {
-		.name = "ad7172-2",
-		.id = AD7172_2_ID,
-		.num_inputs = 5,
-		.num_channels = 4,
-		.num_configs = 4,
-		.num_gpios = 2,
-		.has_temp = true,
-		.has_input_buf = true,
-		.has_int_ref = true,
-		.has_pow_supply_monitoring = true,
-		.clock = 2 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7173_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-	},
-	[ID_AD7172_4] = {
-		.name = "ad7172-4",
-		.id = AD7172_4_ID,
-		.num_inputs = 9,
-		.num_channels = 8,
-		.num_configs = 8,
-		.num_gpios = 4,
-		.has_temp = false,
-		.has_input_buf = true,
-		.has_ref2 = true,
-		.has_pow_supply_monitoring = true,
-		.clock = 2 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7173_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-	},
-	[ID_AD7173_8] = {
-		.name = "ad7173-8",
-		.id = AD7173_ID,
-		.num_inputs = 17,
-		.num_channels = 16,
-		.num_configs = 8,
-		.num_gpios = 4,
-		.has_temp = true,
-		.has_input_buf = true,
-		.has_int_ref = true,
-		.has_ref2 = true,
-		.has_pow_supply_monitoring = false,
-		.clock = 2 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7173_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
-	},
-	[ID_AD7175_2] = {
-		.name = "ad7175-2",
-		.id = AD7175_2_ID,
-		.num_inputs = 5,
-		.num_channels = 4,
-		.num_configs = 4,
-		.num_gpios = 2,
-		.has_temp = true,
-		.has_input_buf = true,
-		.has_int_ref = true,
-		.has_pow_supply_monitoring = true,
-		.clock = 16 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7175_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-	},
-	[ID_AD7175_8] = {
-		.name = "ad7175-8",
-		.id = AD7175_8_ID,
-		.num_inputs = 17,
-		.num_channels = 16,
-		.num_configs = 8,
-		.num_gpios = 4,
-		.has_temp = true,
-		.has_input_buf = true,
-		.has_int_ref = true,
-		.has_ref2 = true,
-		.has_pow_supply_monitoring = true,
-		.clock = 16 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7175_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-	},
-	[ID_AD7176_2] = {
-		.name = "ad7176-2",
-		.id = AD7176_ID,
-		.num_inputs = 5,
-		.num_channels = 4,
-		.num_configs = 4,
-		.num_gpios = 2,
-		.has_temp = false,
-		.has_input_buf = false,
-		.has_int_ref = true,
-		.has_pow_supply_monitoring = false,
-		.clock = 16 * HZ_PER_MHZ,
-		.sinc5_data_rates = ad7175_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-	},
-	[ID_AD7177_2] = {
-		.name = "ad7177-2",
-		.id = AD7177_ID,
-		.num_inputs = 5,
-		.num_channels = 4,
-		.num_configs = 4,
-		.num_gpios = 2,
-		.has_temp = true,
-		.has_input_buf = true,
-		.has_int_ref = true,
-		.has_pow_supply_monitoring = true,
-		.clock = 16 * HZ_PER_MHZ,
-		.odr_start_value = AD7177_ODR_START_VALUE,
-		.sinc5_data_rates = ad7175_sinc5_data_rates,
-		.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
-	},
+static const struct ad7173_device_info ad7172_2_device_info = {
+	.name = "ad7172-2",
+	.id = AD7172_2_ID,
+	.num_inputs = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7172_4_device_info = {
+	.name = "ad7172-4",
+	.id = AD7172_4_ID,
+	.num_inputs = 9,
+	.num_channels = 8,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_temp = false,
+	.has_input_buf = true,
+	.has_ref2 = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7173_8_device_info = {
+	.name = "ad7173-8",
+	.id = AD7173_ID,
+	.num_inputs = 17,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_ref2 = true,
+	.has_pow_supply_monitoring = false,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7175_2_device_info = {
+	.name = "ad7175-2",
+	.id = AD7175_2_ID,
+	.num_inputs = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7175_8_device_info = {
+	.name = "ad7175-8",
+	.id = AD7175_8_ID,
+	.num_inputs = 17,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_gpios = 4,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_ref2 = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7176_2_device_info = {
+	.name = "ad7176-2",
+	.id = AD7176_ID,
+	.num_inputs = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = false,
+	.has_input_buf = false,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = false,
+	.clock = 16 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad7177_2_device_info = {
+	.name = "ad7177-2",
+	.id = AD7177_ID,
+	.num_inputs = 5,
+	.num_channels = 4,
+	.num_configs = 4,
+	.num_gpios = 2,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.has_pow_supply_monitoring = true,
+	.clock = 16 * HZ_PER_MHZ,
+	.odr_start_value = AD7177_ODR_START_VALUE,
+	.sinc5_data_rates = ad7175_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7175_sinc5_data_rates),
 };
 
 static const char *const ad7173_ref_sel_str[] = {
@@ -1192,32 +1186,25 @@ static int ad7173_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id ad7173_of_match[] = {
-	{ .compatible = "adi,ad7172-2",
-	  .data = &ad7173_device_info[ID_AD7172_2]},
-	{ .compatible = "adi,ad7172-4",
-	  .data = &ad7173_device_info[ID_AD7172_4]},
-	{ .compatible = "adi,ad7173-8",
-	  .data = &ad7173_device_info[ID_AD7173_8]},
-	{ .compatible = "adi,ad7175-2",
-	  .data = &ad7173_device_info[ID_AD7175_2]},
-	{ .compatible = "adi,ad7175-8",
-	  .data = &ad7173_device_info[ID_AD7175_8]},
-	{ .compatible = "adi,ad7176-2",
-	  .data = &ad7173_device_info[ID_AD7176_2]},
-	{ .compatible = "adi,ad7177-2",
-	  .data = &ad7173_device_info[ID_AD7177_2]},
+	{ .compatible = "adi,ad7172-2", .data = &ad7172_2_device_info },
+	{ .compatible = "adi,ad7172-4", .data = &ad7172_4_device_info },
+	{ .compatible = "adi,ad7173-8", .data = &ad7173_8_device_info },
+	{ .compatible = "adi,ad7175-2", .data = &ad7175_2_device_info },
+	{ .compatible = "adi,ad7175-8", .data = &ad7175_8_device_info },
+	{ .compatible = "adi,ad7176-2", .data = &ad7176_2_device_info },
+	{ .compatible = "adi,ad7177-2", .data = &ad7177_2_device_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ad7173_of_match);
 
 static const struct spi_device_id ad7173_id_table[] = {
-	{ "ad7172-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_2]},
-	{ "ad7172-4", (kernel_ulong_t)&ad7173_device_info[ID_AD7172_4]},
-	{ "ad7173-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7173_8]},
-	{ "ad7175-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_2]},
-	{ "ad7175-8", (kernel_ulong_t)&ad7173_device_info[ID_AD7175_8]},
-	{ "ad7176-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7176_2]},
-	{ "ad7177-2", (kernel_ulong_t)&ad7173_device_info[ID_AD7177_2]},
+	{ "ad7172-2", (kernel_ulong_t)&ad7172_2_device_info },
+	{ "ad7172-4", (kernel_ulong_t)&ad7172_4_device_info },
+	{ "ad7173-8", (kernel_ulong_t)&ad7173_8_device_info },
+	{ "ad7175-2", (kernel_ulong_t)&ad7175_2_device_info },
+	{ "ad7175-8", (kernel_ulong_t)&ad7175_8_device_info },
+	{ "ad7176-2", (kernel_ulong_t)&ad7176_2_device_info },
+	{ "ad7177-2", (kernel_ulong_t)&ad7177_2_device_info },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ad7173_id_table);

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (6 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:09   ` Nuno Sá
  2024-06-06 16:07 ` [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices Dumitru Ceclan via B4 Relay
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

The ADCs supported by this driver feature a sequencer that read in a
loop all the enabled chanels. When setting the individual sampling
frequency for each channel and enabling multiple channels, the effective
of each channel will be lower than the actual set value. Document this
behaviour in a comment.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 8d008186cd6e..58da5717fd36 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -740,6 +740,21 @@ static int ad7173_write_raw(struct iio_dev *indio_dev,
 		return ret;
 
 	switch (info) {
+	/*
+	 * This attribute sets the sampling frequency for each channel individually.
+	 * There are no issues for raw or buffered reads of an individual channel.
+	 *
+	 * When multiple channels are enabled in buffered mode, the effective
+	 * sampling rate of a channel is lowered in correlation to the number
+	 * of channels enabled and the sampling rate of the other channels.
+	 *
+	 * Example: 3 channels enabled with rates CH1:6211sps CH2,CH3:10sps
+	 * While the reading of CH1 takes only 0.16ms, the reading of CH2 and CH3
+	 * will take 100ms each.
+	 *
+	 * This will cause the reading of CH1 to be actually done once every
+	 * 200.16ms, an effective rate of 4.99sps.
+	 */
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		freq = val * MILLI + val2 / MILLI;
 		for (i = st->info->odr_start_value; i < st->info->num_sinc5_data_rates - 1; i++)

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices
  2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
                   ` (7 preceding siblings ...)
  2024-06-06 16:07 ` [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:07 ` Dumitru Ceclan via B4 Relay
  2024-06-07  9:20   ` Nuno Sá
  8 siblings, 1 reply; 25+ messages in thread
From: Dumitru Ceclan via B4 Relay @ 2024-06-06 16:07 UTC (permalink / raw)
  To: Ceclan Dumitru
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

From: Dumitru Ceclan <dumitru.ceclan@analog.com>

Add support for AD4111/AD4112/AD4114/AD4115/AD4116.

The AD411X family encompasses a series of low power, low noise, 24-bit,
sigma-delta analog-to-digital converters that offer a versatile range of
specifications.

This family of ADCs integrates an analog front end suitable for processing
both fully differential and single-ended, bipolar voltage inputs
addressing a wide array of industrial and instrumentation requirements.

- All ADCs have inputs with a precision voltage divider with a division
  ratio of 10.
- AD4116 has 5 low level inputs without a voltage divider.
- AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
  shunt resistor.

Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
---
 drivers/iio/adc/ad7173.c | 317 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 285 insertions(+), 32 deletions(-)

diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
index 58da5717fd36..cfcd12447e24 100644
--- a/drivers/iio/adc/ad7173.c
+++ b/drivers/iio/adc/ad7173.c
@@ -1,8 +1,9 @@
 // SPDX-License-Identifier: GPL-2.0+
 /*
- * AD717x family SPI ADC driver
+ * AD717x and AD411x family SPI ADC driver
  *
  * Supported devices:
+ *  AD4111/AD4112/AD4114/AD4115/AD4116
  *  AD7172-2/AD7172-4/AD7173-8/AD7175-2
  *  AD7175-8/AD7176-2/AD7177-2
  *
@@ -80,6 +81,11 @@
 #define AD7175_2_ID			0x0cd0
 #define AD7172_4_ID			0x2050
 #define AD7173_ID			0x30d0
+#define AD4111_ID			AD7173_ID
+#define AD4112_ID			AD7173_ID
+#define AD4114_ID			AD7173_ID
+#define AD4116_ID			0x34d0
+#define AD4115_ID			0x38d0
 #define AD7175_8_ID			0x3cd0
 #define AD7177_ID			0x4fd0
 #define AD7173_ID_MASK			GENMASK(15, 4)
@@ -110,6 +116,7 @@
 
 #define AD7173_GPO12_DATA(x)	BIT((x) + 0)
 #define AD7173_GPO23_DATA(x)	BIT((x) + 4)
+#define AD4111_GPO01_DATA(x)	BIT((x) + 6)
 #define AD7173_GPO_DATA(x)	((x) < 2 ? AD7173_GPO12_DATA(x) : AD7173_GPO23_DATA(x))
 
 #define AD7173_INTERFACE_DATA_STAT	BIT(6)
@@ -128,6 +135,16 @@
 #define AD7173_VOLTAGE_INT_REF_uV	2500000
 #define AD7173_TEMP_SENSIIVITY_uV_per_C	477
 #define AD7177_ODR_START_VALUE		0x07
+#define AD4111_SHUNT_RESISTOR_OHM	50
+#define AD4111_DIVIDER_RATIO		10
+#define AD4111_CURRENT_CHAN_CUTOFF	16
+#define AD4111_VINCOM_INPUT		0x10
+
+/* pin <  num_voltage_in is a normal voltage input */
+/* pin >= num_voltage_in_div is a voltage input without a divider */
+#define AD4111_IS_VINCOM_MISMATCH(pin1, pin2) ((pin1) == AD4111_VINCOM_INPUT && \
+					       (pin2) < st->info->num_voltage_in && \
+					       (pin2) >= st->info->num_voltage_in_div)
 
 #define AD7173_FILTER_ODR0_MASK		GENMASK(5, 0)
 #define AD7173_MAX_CONFIGS		8
@@ -136,18 +153,27 @@ struct ad7173_device_info {
 	const unsigned int *sinc5_data_rates;
 	unsigned int num_sinc5_data_rates;
 	unsigned int odr_start_value;
+	/*
+	 * AD4116 has both inputs with a voltage divider and without.
+	 * These inputs cannot be mixed in the channel configuration.
+	 * Does not include the VINCOM input.
+	 */
+	unsigned int num_voltage_in_div;
 	unsigned int num_channels;
 	unsigned int num_configs;
-	unsigned int num_inputs;
+	unsigned int num_voltage_in;
 	unsigned int clock;
 	unsigned int id;
 	char *name;
+	bool has_current_inputs;
+	bool has_vincom_input;
 	bool has_temp;
 	/* ((AVDD1 − AVSS)/5) */
 	bool has_pow_supply_monitoring;
 	bool has_input_buf;
 	bool has_int_ref;
 	bool has_ref2;
+	bool higher_gpio_bits;
 	u8 num_gpios;
 };
 
@@ -189,6 +215,24 @@ struct ad7173_state {
 #endif
 };
 
+static unsigned int ad4115_sinc5_data_rates[] = {
+	24845000, 24845000, 20725000, 20725000,	/*  0-3  */
+	15564000, 13841000, 10390000, 10390000,	/*  4-7  */
+	4994000,  2499000,  1000000,  500000,	/*  8-11 */
+	395500,   200000,   100000,   59890,	/* 12-15 */
+	49920,    20000,    16660,    10000,	/* 16-19 */
+	5000,	  2500,     2500,		/* 20-22 */
+};
+
+static unsigned int ad4116_sinc5_data_rates[] = {
+	12422360, 12422360, 12422360, 12422360,	/*  0-3  */
+	10362690, 10362690, 7782100,  6290530,	/*  4-7  */
+	5194800,  2496900,  1007600,  499900,	/*  8-11 */
+	390600,	  200300,   100000,   59750,	/* 12-15 */
+	49840,	  20000,    16650,    10000,	/* 16-19 */
+	5000,	  2500,	    1250,		/* 20-22 */
+};
+
 static const unsigned int ad7173_sinc5_data_rates[] = {
 	6211000, 6211000, 6211000, 6211000, 6211000, 6211000, 5181000, 4444000,	/*  0-7  */
 	3115000, 2597000, 1007000, 503800,  381000,  200300,  100500,  59520,	/*  8-15 */
@@ -204,13 +248,114 @@ static const unsigned int ad7175_sinc5_data_rates[] = {
 	5000,					/* 20    */
 };
 
+static unsigned int ad4111_current_channel_config[] = {
+	/* Ain sel: pos        neg    */
+	0x1E8, /* 15:IIN0+    8:IIN0− */
+	0x1C9, /* 14:IIN1+    9:IIN1− */
+	0x1AA, /* 13:IIN2+   10:IIN2− */
+	0x18B, /* 12:IIN3+   11:IIN3− */
+};
+
+static const struct ad7173_device_info ad4111_device_info = {
+	.name = "ad4111",
+	.id = AD4111_ID,
+	.num_voltage_in_div = 8,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 8,
+	.num_gpios = 2,
+	.higher_gpio_bits = true,
+	.has_temp = true,
+	.has_vincom_input = true,
+	.has_input_buf = true,
+	.has_current_inputs = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4112_device_info = {
+	.name = "ad4112",
+	.id = AD4112_ID,
+	.num_voltage_in_div = 8,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 8,
+	.num_gpios = 2,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_current_inputs = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4114_device_info = {
+	.name = "ad4114",
+	.id = AD4114_ID,
+	.num_voltage_in_div = 16,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 2 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad7173_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4115_device_info = {
+	.name = "ad4115",
+	.id = AD4115_ID,
+	.num_voltage_in_div = 16,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 8 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad4115_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad4115_sinc5_data_rates),
+};
+
+static const struct ad7173_device_info ad4116_device_info = {
+	.name = "ad4116",
+	.id = AD4116_ID,
+	.num_voltage_in_div = 11,
+	.num_channels = 16,
+	.num_configs = 8,
+	.num_voltage_in = 16,
+	.num_gpios = 4,
+	.higher_gpio_bits = true,
+	.has_vincom_input = true,
+	.has_temp = true,
+	.has_input_buf = true,
+	.has_int_ref = true,
+	.clock = 4 * HZ_PER_MHZ,
+	.sinc5_data_rates = ad4116_sinc5_data_rates,
+	.num_sinc5_data_rates = ARRAY_SIZE(ad4116_sinc5_data_rates),
+};
+
 static const struct ad7173_device_info ad7172_2_device_info = {
 	.name = "ad7172-2",
 	.id = AD7172_2_ID,
-	.num_inputs = 5,
+	.num_voltage_in = 5,
 	.num_channels = 4,
 	.num_configs = 4,
 	.num_gpios = 2,
+	.higher_gpio_bits = false,
 	.has_temp = true,
 	.has_input_buf = true,
 	.has_int_ref = true,
@@ -223,10 +368,11 @@ static const struct ad7173_device_info ad7172_2_device_info = {
 static const struct ad7173_device_info ad7172_4_device_info = {
 	.name = "ad7172-4",
 	.id = AD7172_4_ID,
-	.num_inputs = 9,
+	.num_voltage_in = 9,
 	.num_channels = 8,
 	.num_configs = 8,
 	.num_gpios = 4,
+	.higher_gpio_bits = false,
 	.has_temp = false,
 	.has_input_buf = true,
 	.has_ref2 = true,
@@ -239,10 +385,11 @@ static const struct ad7173_device_info ad7172_4_device_info = {
 static const struct ad7173_device_info ad7173_8_device_info = {
 	.name = "ad7173-8",
 	.id = AD7173_ID,
-	.num_inputs = 17,
+	.num_voltage_in = 17,
 	.num_channels = 16,
 	.num_configs = 8,
 	.num_gpios = 4,
+	.higher_gpio_bits = false,
 	.has_temp = true,
 	.has_input_buf = true,
 	.has_int_ref = true,
@@ -256,10 +403,11 @@ static const struct ad7173_device_info ad7173_8_device_info = {
 static const struct ad7173_device_info ad7175_2_device_info = {
 	.name = "ad7175-2",
 	.id = AD7175_2_ID,
-	.num_inputs = 5,
+	.num_voltage_in = 5,
 	.num_channels = 4,
 	.num_configs = 4,
 	.num_gpios = 2,
+	.higher_gpio_bits = false,
 	.has_temp = true,
 	.has_input_buf = true,
 	.has_int_ref = true,
@@ -272,10 +420,11 @@ static const struct ad7173_device_info ad7175_2_device_info = {
 static const struct ad7173_device_info ad7175_8_device_info = {
 	.name = "ad7175-8",
 	.id = AD7175_8_ID,
-	.num_inputs = 17,
+	.num_voltage_in = 17,
 	.num_channels = 16,
 	.num_configs = 8,
 	.num_gpios = 4,
+	.higher_gpio_bits = false,
 	.has_temp = true,
 	.has_input_buf = true,
 	.has_int_ref = true,
@@ -289,10 +438,11 @@ static const struct ad7173_device_info ad7175_8_device_info = {
 static const struct ad7173_device_info ad7176_2_device_info = {
 	.name = "ad7176-2",
 	.id = AD7176_ID,
-	.num_inputs = 5,
+	.num_voltage_in = 5,
 	.num_channels = 4,
 	.num_configs = 4,
 	.num_gpios = 2,
+	.higher_gpio_bits = false,
 	.has_temp = false,
 	.has_input_buf = false,
 	.has_int_ref = true,
@@ -305,10 +455,11 @@ static const struct ad7173_device_info ad7176_2_device_info = {
 static const struct ad7173_device_info ad7177_2_device_info = {
 	.name = "ad7177-2",
 	.id = AD7177_ID,
-	.num_inputs = 5,
+	.num_voltage_in = 5,
 	.num_channels = 4,
 	.num_configs = 4,
 	.num_gpios = 2,
+	.higher_gpio_bits = false,
 	.has_temp = true,
 	.has_input_buf = true,
 	.has_int_ref = true,
@@ -358,6 +509,15 @@ static int ad7173_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
 	return 0;
 }
 
+static int ad4111_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset, unsigned int *reg,
+			     unsigned int *mask)
+{
+	*mask = AD4111_GPO01_DATA(offset);
+	*reg = base;
+	return 0;
+}
+
 static void ad7173_gpio_disable(void *data)
 {
 	struct ad7173_state *st = data;
@@ -390,7 +550,10 @@ static int ad7173_gpio_init(struct ad7173_state *st)
 	gpio_regmap.regmap = st->reg_gpiocon_regmap;
 	gpio_regmap.ngpio = st->info->num_gpios;
 	gpio_regmap.reg_set_base = AD7173_REG_GPIO;
-	gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
+	if (st->info->higher_gpio_bits)
+		gpio_regmap.reg_mask_xlate = ad4111_mask_xlate;
+	else
+		gpio_regmap.reg_mask_xlate = ad7173_mask_xlate;
 
 	st->gpio_regmap = devm_gpio_regmap_register(dev, &gpio_regmap);
 	ret = PTR_ERR_OR_ZERO(st->gpio_regmap);
@@ -690,18 +853,33 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if (chan->type == IIO_TEMP) {
+
+		switch (chan->type) {
+		case IIO_TEMP:
 			temp = AD7173_VOLTAGE_INT_REF_uV * MILLI;
 			temp /= AD7173_TEMP_SENSIIVITY_uV_per_C;
 			*val = temp;
 			*val2 = chan->scan_type.realbits;
-		} else {
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_VOLTAGE:
 			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
 			*val2 = chan->scan_type.realbits - !!(ch->cfg.bipolar);
+
+			if (chan->channel < st->info->num_voltage_in_div)
+				*val *= AD4111_DIVIDER_RATIO;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		case IIO_CURRENT:
+			*val = ad7173_get_ref_voltage_milli(st, ch->cfg.ref_sel);
+			*val /= AD4111_SHUNT_RESISTOR_OHM;
+			*val2 = chan->scan_type.realbits - ch->cfg.bipolar;
+			return IIO_VAL_FRACTIONAL_LOG2;
+		default:
+			return -EINVAL;
 		}
-		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_OFFSET:
-		if (chan->type == IIO_TEMP) {
+
+		switch (chan->type) {
+		case IIO_TEMP:
 			/* 0 Kelvin -> raw sample */
 			temp   = -ABSOLUTE_ZERO_MILLICELSIUS;
 			temp  *= AD7173_TEMP_SENSIIVITY_uV_per_C;
@@ -710,10 +888,14 @@ static int ad7173_read_raw(struct iio_dev *indio_dev,
 						       AD7173_VOLTAGE_INT_REF_uV *
 						       MILLI);
 			*val   = -temp;
-		} else {
+			return IIO_VAL_INT;
+		case IIO_VOLTAGE:
+		case IIO_CURRENT:
 			*val = -BIT(chan->scan_type.realbits - 1);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
 		}
-		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		reg = st->channels[chan->address].cfg.odr;
 
@@ -935,6 +1117,23 @@ static int ad7173_register_clk_provider(struct iio_dev *indio_dev)
 					   &st->int_clk_hw);
 }
 
+static int ad4111_validate_current_ain(struct ad7173_state *st,
+				       const unsigned int ain[AD7173_NO_AINS_PER_CHANNEL])
+{
+	struct device *dev = &st->sd.spi->dev;
+
+	if (!st->info->has_current_inputs)
+		return dev_err_probe(dev, -EINVAL,
+			"Model %s does not support current channels\n",
+			st->info->name);
+
+	if (ain[0] >= ARRAY_SIZE(ad4111_current_channel_config))
+		return dev_err_probe(dev, -EINVAL,
+			"For current channels single-channel must be <[0-3]>\n");
+
+	return 0;
+}
+
 static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 					      unsigned int ain0, unsigned int ain1)
 {
@@ -946,15 +1145,30 @@ static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
 	    st->info->has_pow_supply_monitoring)
 		return 0;
 
-	special_input0 = AD7173_IS_REF_INPUT(ain0);
-	special_input1 = AD7173_IS_REF_INPUT(ain1);
+	special_input0 = AD7173_IS_REF_INPUT(ain0) ||
+			 (ain0 == AD4111_VINCOM_INPUT && st->info->has_vincom_input);
+	special_input1 = AD7173_IS_REF_INPUT(ain1) ||
+			 (ain1 == AD4111_VINCOM_INPUT && st->info->has_vincom_input);
+
+	if (st->info->has_vincom_input)
+		if (AD4111_IS_VINCOM_MISMATCH(ain0, ain1) ||
+		    AD4111_IS_VINCOM_MISMATCH(ain1, ain0))
+			return dev_err_probe(dev, -EINVAL,
+				"VINCOM must be paired with inputs having divider.\n");
 
-	if ((ain0 >= st->info->num_inputs && !special_input0) ||
-	    (ain1 >= st->info->num_inputs && !special_input1))
+	if ((ain0 >= st->info->num_voltage_in && !special_input0) ||
+	    (ain1 >= st->info->num_voltage_in && !special_input1))
 		return dev_err_probe(dev, -EINVAL,
 				     "Input pin number out of range for pair (%d %d).\n",
 				     ain0, ain1);
 
+	if (!special_input0 && !special_input1 &&
+	    ((ain0 >= st->info->num_voltage_in_div) !=
+	     (ain1 >= st->info->num_voltage_in_div)))
+		return dev_err_probe(dev, -EINVAL,
+			"Both inputs must either have a voltage divider or not have: (%d %d).\n",
+			ain0, ain1);
+
 	return 0;
 }
 
@@ -985,7 +1199,7 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 	struct device *dev = indio_dev->dev.parent;
 	struct iio_chan_spec *chan_arr, *chan;
 	unsigned int ain[AD7173_NO_AINS_PER_CHANNEL], chan_index = 0;
-	int ref_sel, ret, num_channels;
+	int ref_sel, ret, is_current_chan, num_channels;
 
 	num_channels = device_get_child_node_count(dev);
 
@@ -1032,15 +1246,38 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 
 	device_for_each_child_node_scoped(dev, child) {
 		chan = &chan_arr[chan_index];
+		*chan = ad7173_channel_template;
 		chan_st_priv = &chans_st_arr[chan_index];
 		ret = fwnode_property_read_u32_array(child, "diff-channels",
 						     ain, ARRAY_SIZE(ain));
-		if (ret)
-			return ret;
+		if (ret) {
+			ret = fwnode_property_read_u32(child, "single-channel",
+						       ain);
+			if (ret)
+				return dev_err_probe(dev, ret,
+					"Channel must define one of diff-channels or single-channel.\n");
 
-		ret = ad7173_validate_voltage_ain_inputs(st, ain[0], ain[1]);
-		if (ret)
-			return ret;
+			is_current_chan = fwnode_property_read_bool(child, "adi,current-channel");
+		} else {
+			chan->differential = true;
+		}
+
+		if (is_current_chan) {
+			ret = ad4111_validate_current_ain(st, ain);
+			if (ret)
+				return ret;
+		} else {
+			if (!chan->differential) {
+				ret = fwnode_property_read_u32(child,
+					"common-mode-channel", ain + 1);
+				if (ret)
+					return dev_err_probe(dev, ret,
+						"common-mode-channel must be defined for single-ended channels.\n");
+			}
+			ret = ad7173_validate_voltage_ain_inputs(st, ain[0], ain[1]);
+			if (ret)
+				return ret;
+		}
 
 		ret = fwnode_property_match_property_string(child,
 							    "adi,reference-select",
@@ -1059,14 +1296,9 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 			st->adc_mode |= AD7173_ADC_MODE_REF_EN;
 		chan_st_priv->cfg.ref_sel = ref_sel;
 
-		*chan = ad7173_channel_template;
 		chan->address = chan_index;
 		chan->scan_index = chan_index;
 		chan->channel = ain[0];
-		chan->channel2 = ain[1];
-		chan->differential = true;
-
-		chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
 		chan_st_priv->chan_reg = chan_index;
 		chan_st_priv->cfg.input_buf = st->info->has_input_buf;
 		chan_st_priv->cfg.odr = 0;
@@ -1075,6 +1307,17 @@ static int ad7173_fw_parse_channel_config(struct iio_dev *indio_dev)
 		if (chan_st_priv->cfg.bipolar)
 			chan->info_mask_separate |= BIT(IIO_CHAN_INFO_OFFSET);
 
+		if (is_current_chan) {
+			chan->type = IIO_CURRENT;
+			chan->differential = false;
+			chan->channel2 = 0;
+			chan_st_priv->ain = ad4111_current_channel_config[ain[0]];
+		} else {
+			chan_st_priv->cfg.input_buf = st->info->has_input_buf;
+			chan->channel2 = ain[1];
+			chan_st_priv->ain = AD7173_CH_ADDRESS(ain[0], ain[1]);
+		}
+
 		chan_index++;
 	}
 	return 0;
@@ -1201,6 +1444,11 @@ static int ad7173_probe(struct spi_device *spi)
 }
 
 static const struct of_device_id ad7173_of_match[] = {
+	{ .compatible = "ad4111",	.data = &ad4111_device_info },
+	{ .compatible = "ad4112",	.data = &ad4112_device_info },
+	{ .compatible = "ad4114",	.data = &ad4114_device_info },
+	{ .compatible = "ad4115",	.data = &ad4115_device_info },
+	{ .compatible = "ad4116",	.data = &ad4116_device_info },
 	{ .compatible = "adi,ad7172-2", .data = &ad7172_2_device_info },
 	{ .compatible = "adi,ad7172-4", .data = &ad7172_4_device_info },
 	{ .compatible = "adi,ad7173-8", .data = &ad7173_8_device_info },
@@ -1213,6 +1461,11 @@ static const struct of_device_id ad7173_of_match[] = {
 MODULE_DEVICE_TABLE(of, ad7173_of_match);
 
 static const struct spi_device_id ad7173_id_table[] = {
+	{ "ad4111",   (kernel_ulong_t)&ad4111_device_info },
+	{ "ad4112",   (kernel_ulong_t)&ad4112_device_info },
+	{ "ad4114",   (kernel_ulong_t)&ad4114_device_info },
+	{ "ad4115",   (kernel_ulong_t)&ad4115_device_info },
+	{ "ad4116",   (kernel_ulong_t)&ad4116_device_info },
 	{ "ad7172-2", (kernel_ulong_t)&ad7172_2_device_info },
 	{ "ad7172-4", (kernel_ulong_t)&ad7172_4_device_info },
 	{ "ad7173-8", (kernel_ulong_t)&ad7173_8_device_info },
@@ -1237,5 +1490,5 @@ module_spi_driver(ad7173_driver);
 MODULE_IMPORT_NS(IIO_AD_SIGMA_DELTA);
 MODULE_AUTHOR("Lars-Peter Clausen <lars@metafo.de>");
 MODULE_AUTHOR("Dumitru Ceclan <dumitru.ceclan@analog.com>");
-MODULE_DESCRIPTION("Analog Devices AD7172/AD7173/AD7175/AD7176 ADC driver");
+MODULE_DESCRIPTION("Analog Devices AD7173 and similar ADC driver");
 MODULE_LICENSE("GPL");

-- 
2.43.0



^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x
  2024-06-06 16:07 ` [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x Dumitru Ceclan via B4 Relay
@ 2024-06-06 16:38   ` Conor Dooley
  0 siblings, 0 replies; 25+ messages in thread
From: Conor Dooley @ 2024-06-06 16:38 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

[-- Attachment #1: Type: text/plain, Size: 700 bytes --]

On Thu, Jun 06, 2024 at 07:07:41PM +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Add support for: AD4111, AD4112, AD4114, AD4115, AD4116.
> 
> AD411x family ADCs support a VCOM pin. The purpose of this pin is to
> offer a dedicated common-mode voltage input for single-ended channels.
> This pin is specified as supporting a differential channel with VIN10 on
> model AD4116.
> 
> AD4111/AD4112 support current channels. Support is implemented using
> single-channel and "adi,current-channel".
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback
  2024-06-06 16:07 ` [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:02   ` Nuno Sá
  2024-06-07  9:29     ` Ceclan, Dumitru
  0 siblings, 1 reply; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:02 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Sigma delta ADCs with a sequencer need to disable the previously enabled
> channel when reading using ad_sigma_delta_single_conversion(). This was
> done manually in drivers for devices with sequencers.
> 
> This patch implements handling of single channel disabling after a
> single conversion.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

You could have this done in separate patches... Oh well, this is simple enough
that I don't care much.

Reviewed-by: Nuno Sa <nuno.sa@analog.com>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection
  2024-06-06 16:07 ` [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:04   ` Nuno Sá
  2024-06-07  9:37     ` Ceclan, Dumitru
  0 siblings, 1 reply; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:04 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Move validation of analog inputs and reference voltage selection to
> separate functions to reduce the size of the channel config parsing
> function and improve readability.
> Add defines for the number of analog inputs in a channel.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++--------------
> -
>  1 file changed, 47 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 8631f218b69e..4040edbd1c32 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -60,6 +60,7 @@
>  #define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
>  #define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
>  
> +#define AD7173_NO_AINS_PER_CHANNEL	2
>  #define AD7173_CH_ADDRESS(pos, neg) \
>  	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
> @@ -629,6 +630,7 @@ static int ad7173_setup(struct iio_dev *indio_dev)
>  static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
>  						 u8 reference_select)
>  {
> +	struct device *dev = &st->sd.spi->dev;
>  	int vref;
>  
>  	switch (reference_select) {
> @@ -652,9 +654,11 @@ static unsigned int ad7173_get_ref_voltage_milli(struct
> ad7173_state *st,
>  		return -EINVAL;
>  	}
>  
> -	if (vref < 0)
> +	if (vref < 0) {
> +		dev_err(dev, "Cannot use reference %u. Error:%d\n",
> +			reference_select, vref);
>  		return vref;
> -
> +	}
>  	return vref / (MICRO / MILLI);
>  }

unrelated?

- Nuno Sá


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs
  2024-06-06 16:07 ` [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:06   ` Nuno Sá
  2024-06-07  9:34     ` Ceclan, Dumitru
  0 siblings, 1 reply; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:06 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
>  Add support for selecting REF+ and REF- inputs on all models.
>  Add support for selecting ((AVDD1 − AVSS)/5) inputs
>   on supported models.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 4040edbd1c32..d16fa081a285 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> @@ -66,6 +66,13 @@
>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>  #define AD7173_AIN_TEMP_POS	17
>  #define AD7173_AIN_TEMP_NEG	18
> +#define AD7173_AIN_POW_MON_POS	19
> +#define AD7173_AIN_POW_MON_NEG	20
> +#define AD7173_AIN_REF_POS	21
> +#define AD7173_AIN_REF_NEG	22
> +
> +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
> +					(x) == AD7173_AIN_REF_NEG)
>  
>  #define AD7172_2_ID			0x00d0
>  #define AD7175_ID			0x0cd0
> @@ -146,6 +153,8 @@ struct ad7173_device_info {
>  	unsigned int id;
>  	char *name;
>  	bool has_temp;
> +	/* ((AVDD1 − AVSS)/5) */
> +	bool has_pow_supply_monitoring;
>  	bool has_input_buf;
>  	bool has_int_ref;
>  	bool has_ref2;
> @@ -216,6 +225,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_temp = true,
>  		.has_input_buf = true,
>  		.has_int_ref = true,
> +		.has_pow_supply_monitoring = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -230,6 +240,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_temp = false,
>  		.has_input_buf = true,
>  		.has_ref2 = true,
> +		.has_pow_supply_monitoring = true,
>  		.clock = 2 * HZ_PER_MHZ,
>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
> @@ -245,6 +256,7 @@ static const struct ad7173_device_info
> ad7173_device_info[] = {
>  		.has_input_buf = true,
>  		.has_int_ref = true,
>  		.has_ref2 = true,
> +		.has_pow_supply_monitoring = false,

No need to set the 'false' cases...


- Nuno Sá

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs
  2024-06-06 16:07 ` [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:08   ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:08 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Drop array of device info structs and use individual structs for all;
> drop models enum as no longer needed. This improves readability as the
> structs are pointed directly.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour
  2024-06-06 16:07 ` [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:09   ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:09 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> The ADCs supported by this driver feature a sequencer that read in a
> loop all the enabled chanels. When setting the individual sampling
> frequency for each channel and enabling multiple channels, the effective
> of each channel will be lower than the actual set value. Document this
> behaviour in a comment.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---

Reviewed-by: Nuno Sa <nuno.sa@analog.com>



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices
  2024-06-06 16:07 ` [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices Dumitru Ceclan via B4 Relay
@ 2024-06-07  9:20   ` Nuno Sá
  2024-06-07  9:41     ` Ceclan, Dumitru
  0 siblings, 1 reply; 25+ messages in thread
From: Nuno Sá @ 2024-06-07  9:20 UTC (permalink / raw)
  To: dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel, Dumitru Ceclan

On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> 
> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> 
> The AD411X family encompasses a series of low power, low noise, 24-bit,
> sigma-delta analog-to-digital converters that offer a versatile range of
> specifications.
> 
> This family of ADCs integrates an analog front end suitable for processing
> both fully differential and single-ended, bipolar voltage inputs
> addressing a wide array of industrial and instrumentation requirements.
> 
> - All ADCs have inputs with a precision voltage divider with a division
>   ratio of 10.
> - AD4116 has 5 low level inputs without a voltage divider.
> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
>   shunt resistor.
> 
> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> ---
>  drivers/iio/adc/ad7173.c | 317 ++++++++++++++++++++++++++++++++++++++++++----
> -
>  1 file changed, 285 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> index 58da5717fd36..cfcd12447e24 100644
> --- a/drivers/iio/adc/ad7173.c
> +++ b/drivers/iio/adc/ad7173.c
> 
...

>  static const struct ad7173_device_info ad7172_2_device_info = {
>  	.name = "ad7172-2",
>  	.id = AD7172_2_ID,
> -	.num_inputs = 5,
> +	.num_voltage_in = 5,
>  	.num_channels = 4,
>  	.num_configs = 4,
>  	.num_gpios = 2,
> +	.higher_gpio_bits = false,

No need to explicitly set to 'false'. Ditto for the other places...

...

> 
>  static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
>  					      unsigned int ain0, unsigned int
> ain1)
>  {
> @@ -946,15 +1145,30 @@ static int ad7173_validate_voltage_ain_inputs(struct
> ad7173_state *st,
>  	    st->info->has_pow_supply_monitoring)
>  		return 0;
>  
> -	special_input0 = AD7173_IS_REF_INPUT(ain0);
> -	special_input1 = AD7173_IS_REF_INPUT(ain1);
> +	special_input0 = AD7173_IS_REF_INPUT(ain0) ||
> +			 (ain0 == AD4111_VINCOM_INPUT && st->info-
> >has_vincom_input);
> +	special_input1 = AD7173_IS_REF_INPUT(ain1) ||
> +			 (ain1 == AD4111_VINCOM_INPUT && st->info-
> >has_vincom_input);
> +

Wondering... can ain1 (or ain0) be AD4111_VINCOM_INPUT and !st->info-
>has_vincom_input? Would that actually be acceptable? It would assume it's not
so we should check that right? Or am I missing something?

- Nuno Sá


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback
  2024-06-07  9:02   ` Nuno Sá
@ 2024-06-07  9:29     ` Ceclan, Dumitru
  2024-06-07 10:16       ` Nuno Sá
  0 siblings, 1 reply; 25+ messages in thread
From: Ceclan, Dumitru @ 2024-06-07  9:29 UTC (permalink / raw)
  To: Nuno Sá, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On 07/06/2024 12:02, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Sigma delta ADCs with a sequencer need to disable the previously enabled
>> channel when reading using ad_sigma_delta_single_conversion(). This was
>> done manually in drivers for devices with sequencers.
>>
>> This patch implements handling of single channel disabling after a
>> single conversion.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
> 
> You could have this done in separate patches... Oh well, this is simple enough
> that I don't care much.
> 
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> 

Separate patches would break driver functionality then fix it.
The drivers would not probe as disable_one() callback is missing.

This would have been alright?


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs
  2024-06-07  9:06   ` Nuno Sá
@ 2024-06-07  9:34     ` Ceclan, Dumitru
  2024-06-07 10:29       ` Nuno Sá
  0 siblings, 1 reply; 25+ messages in thread
From: Ceclan, Dumitru @ 2024-06-07  9:34 UTC (permalink / raw)
  To: Nuno Sá, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On 07/06/2024 12:06, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>>  Add support for selecting REF+ and REF- inputs on all models.
>>  Add support for selecting ((AVDD1 − AVSS)/5) inputs
>>   on supported models.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
>>  1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 4040edbd1c32..d16fa081a285 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -66,6 +66,13 @@
>>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>>  #define AD7173_AIN_TEMP_POS	17
>>  #define AD7173_AIN_TEMP_NEG	18
>> +#define AD7173_AIN_POW_MON_POS	19
>> +#define AD7173_AIN_POW_MON_NEG	20
>> +#define AD7173_AIN_REF_POS	21
>> +#define AD7173_AIN_REF_NEG	22
>> +
>> +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
>> +					(x) == AD7173_AIN_REF_NEG)
>>  
>>  #define AD7172_2_ID			0x00d0
>>  #define AD7175_ID			0x0cd0
>> @@ -146,6 +153,8 @@ struct ad7173_device_info {
>>  	unsigned int id;
>>  	char *name;
>>  	bool has_temp;
>> +	/* ((AVDD1 − AVSS)/5) */
>> +	bool has_pow_supply_monitoring;
>>  	bool has_input_buf;
>>  	bool has_int_ref;
>>  	bool has_ref2;
>> @@ -216,6 +225,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_temp = true,
>>  		.has_input_buf = true,
>>  		.has_int_ref = true,
>> +		.has_pow_supply_monitoring = true,
>>  		.clock = 2 * HZ_PER_MHZ,
>>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> @@ -230,6 +240,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_temp = false,
>>  		.has_input_buf = true,
>>  		.has_ref2 = true,
>> +		.has_pow_supply_monitoring = true,
>>  		.clock = 2 * HZ_PER_MHZ,
>>  		.sinc5_data_rates = ad7173_sinc5_data_rates,
>>  		.num_sinc5_data_rates = ARRAY_SIZE(ad7173_sinc5_data_rates),
>> @@ -245,6 +256,7 @@ static const struct ad7173_device_info
>> ad7173_device_info[] = {
>>  		.has_input_buf = true,
>>  		.has_int_ref = true,
>>  		.has_ref2 = true,
>> +		.has_pow_supply_monitoring = false,
> 
> No need to set the 'false' cases...
> 
> 
> - Nuno Sá

This was suggested by David Lechner to ensure consistency with has_temp
regarding another field, I considered that it would apply here as well.
https://lore.kernel.org/all/CAMknhBGaJxXvsQ8cZkgDsKLVjOY5y2pzox-99hdOCrUaoZdsxg@mail.gmail.com/

This would also increase visibility towards what features does a specific
model support as it is clearly stated with "= false" rather than looking
for what fields are not set within the struct.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection
  2024-06-07  9:04   ` Nuno Sá
@ 2024-06-07  9:37     ` Ceclan, Dumitru
  2024-06-07 10:24       ` Nuno Sá
  0 siblings, 1 reply; 25+ messages in thread
From: Ceclan, Dumitru @ 2024-06-07  9:37 UTC (permalink / raw)
  To: Nuno Sá, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On 07/06/2024 12:04, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Move validation of analog inputs and reference voltage selection to
>> separate functions to reduce the size of the channel config parsing
>> function and improve readability.
>> Add defines for the number of analog inputs in a channel.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++--------------
>> -
>>  1 file changed, 47 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 8631f218b69e..4040edbd1c32 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>> @@ -60,6 +60,7 @@
>>  #define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
>>  #define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
>>  
>> +#define AD7173_NO_AINS_PER_CHANNEL	2
>>  #define AD7173_CH_ADDRESS(pos, neg) \
>>  	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
>>  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
>> @@ -629,6 +630,7 @@ static int ad7173_setup(struct iio_dev *indio_dev)
>>  static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
>>  						 u8 reference_select)
>>  {
>> +	struct device *dev = &st->sd.spi->dev;
>>  	int vref;
>>  
>>  	switch (reference_select) {
>> @@ -652,9 +654,11 @@ static unsigned int ad7173_get_ref_voltage_milli(struct
>> ad7173_state *st,
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (vref < 0)
>> +	if (vref < 0) {
>> +		dev_err(dev, "Cannot use reference %u. Error:%d\n",
>> +			reference_select, vref);
>>  		return vref;
>> -
>> +	}
>>  	return vref / (MICRO / MILLI);
>>  }
> 
> unrelated?
> 
> - Nuno Sá
> 

Hmm, maybe I misunderstood "Any error log needed should be done inside ad7173_get_ref_voltage_milli()"
https://lore.kernel.org/all/71452f6882efe6a181d477914488617d28a38e2f.camel@gmail.com/

This change should be in a different patch or should it not've been done
this way?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices
  2024-06-07  9:20   ` Nuno Sá
@ 2024-06-07  9:41     ` Ceclan, Dumitru
  2024-06-07 10:39       ` Nuno Sá
  0 siblings, 1 reply; 25+ messages in thread
From: Ceclan, Dumitru @ 2024-06-07  9:41 UTC (permalink / raw)
  To: Nuno Sá, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On 07/06/2024 12:20, Nuno Sá wrote:
> On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
>> From: Dumitru Ceclan <dumitru.ceclan@analog.com>
>>
>> Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
>>
>> The AD411X family encompasses a series of low power, low noise, 24-bit,
>> sigma-delta analog-to-digital converters that offer a versatile range of
>> specifications.
>>
>> This family of ADCs integrates an analog front end suitable for processing
>> both fully differential and single-ended, bipolar voltage inputs
>> addressing a wide array of industrial and instrumentation requirements.
>>
>> - All ADCs have inputs with a precision voltage divider with a division
>>   ratio of 10.
>> - AD4116 has 5 low level inputs without a voltage divider.
>> - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
>>   shunt resistor.
>>
>> Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
>> ---
>>  drivers/iio/adc/ad7173.c | 317 ++++++++++++++++++++++++++++++++++++++++++----
>> -
>>  1 file changed, 285 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
>> index 58da5717fd36..cfcd12447e24 100644
>> --- a/drivers/iio/adc/ad7173.c
>> +++ b/drivers/iio/adc/ad7173.c
>>
> ...
> 
>>  static const struct ad7173_device_info ad7172_2_device_info = {
>>  	.name = "ad7172-2",
>>  	.id = AD7172_2_ID,
>> -	.num_inputs = 5,
>> +	.num_voltage_in = 5,
>>  	.num_channels = 4,
>>  	.num_configs = 4,
>>  	.num_gpios = 2,
>> +	.higher_gpio_bits = false,
> 
> No need to explicitly set to 'false'. Ditto for the other places...
> 
> ...
> 
>>
>>  static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
>>  					      unsigned int ain0, unsigned int
>> ain1)
>>  {
>> @@ -946,15 +1145,30 @@ static int ad7173_validate_voltage_ain_inputs(struct
>> ad7173_state *st,
>>  	    st->info->has_pow_supply_monitoring)
>>  		return 0;
>>  
>> -	special_input0 = AD7173_IS_REF_INPUT(ain0);
>> -	special_input1 = AD7173_IS_REF_INPUT(ain1);
>> +	special_input0 = AD7173_IS_REF_INPUT(ain0) ||
>> +			 (ain0 == AD4111_VINCOM_INPUT && st->info-
>>> has_vincom_input);
>> +	special_input1 = AD7173_IS_REF_INPUT(ain1) ||
>> +			 (ain1 == AD4111_VINCOM_INPUT && st->info-
>>> has_vincom_input);
>> +
> 
> Wondering... can ain1 (or ain0) be AD4111_VINCOM_INPUT and !st->info-
>> has_vincom_input? Would that actually be acceptable? It would assume it's not
> so we should check that right? Or am I missing something?
> 
> - Nuno Sá
> 

It will fail when we check for the number of voltage inputs:
(ain0 >= st->info->num_voltage_in && !special_input0) 
as special_input will not be true if has_vincom_input is false

Indeed this check is a bit hidden, should it be more explicit?


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback
  2024-06-07  9:29     ` Ceclan, Dumitru
@ 2024-06-07 10:16       ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07 10:16 UTC (permalink / raw)
  To: Ceclan, Dumitru, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On Fri, 2024-06-07 at 12:29 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:02, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > > Sigma delta ADCs with a sequencer need to disable the previously enabled
> > > channel when reading using ad_sigma_delta_single_conversion(). This was
> > > done manually in drivers for devices with sequencers.
> > > 
> > > This patch implements handling of single channel disabling after a
> > > single conversion.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > 
> > You could have this done in separate patches... Oh well, this is simple
> > enough
> > that I don't care much.
> > 
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> > 
> 
> Separate patches would break driver functionality then fix it.
> The drivers would not probe as disable_one() callback is missing.
> 
> This would have been alright?
> 

No, it's not... I mean, you could also only enforce the disable_one() presence
after updating all users but I agree this is simple enough to go in one patch.

- Nuno Sá



^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection
  2024-06-07  9:37     ` Ceclan, Dumitru
@ 2024-06-07 10:24       ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07 10:24 UTC (permalink / raw)
  To: Ceclan, Dumitru, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On Fri, 2024-06-07 at 12:37 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:04, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > > Move validation of analog inputs and reference voltage selection to
> > > separate functions to reduce the size of the channel config parsing
> > > function and improve readability.
> > > Add defines for the number of analog inputs in a channel.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 68 +++++++++++++++++++++++++++++++++----------
> > > ----
> > > -
> > >  1 file changed, 47 insertions(+), 21 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 8631f218b69e..4040edbd1c32 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > @@ -60,6 +60,7 @@
> > >  #define AD7173_CH_SETUP_AINPOS_MASK	GENMASK(9, 5)
> > >  #define AD7173_CH_SETUP_AINNEG_MASK	GENMASK(4, 0)
> > >  
> > > +#define AD7173_NO_AINS_PER_CHANNEL	2
> > >  #define AD7173_CH_ADDRESS(pos, neg) \
> > >  	(FIELD_PREP(AD7173_CH_SETUP_AINPOS_MASK, pos) | \
> > >  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
> > > @@ -629,6 +630,7 @@ static int ad7173_setup(struct iio_dev *indio_dev)
> > >  static unsigned int ad7173_get_ref_voltage_milli(struct ad7173_state *st,
> > >  						 u8 reference_select)
> > >  {
> > > +	struct device *dev = &st->sd.spi->dev;
> > >  	int vref;
> > >  
> > >  	switch (reference_select) {
> > > @@ -652,9 +654,11 @@ static unsigned int
> > > ad7173_get_ref_voltage_milli(struct
> > > ad7173_state *st,
> > >  		return -EINVAL;
> > >  	}
> > >  
> > > -	if (vref < 0)
> > > +	if (vref < 0) {
> > > +		dev_err(dev, "Cannot use reference %u. Error:%d\n",
> > > +			reference_select, vref);
> > >  		return vref;
> > > -
> > > +	}
> > >  	return vref / (MICRO / MILLI);
> > >  }
> > 
> > unrelated?
> > 
> > - Nuno Sá
> > 
> 
> Hmm, maybe I misunderstood "Any error log needed should be done inside
> ad7173_get_ref_voltage_milli()"
> https://lore.kernel.org/all/71452f6882efe6a181d477914488617d28a38e2f.camel@gmail.com/
> 
> This change should be in a different patch or should it not've been done
> this way?

Ohh right... Mentioning this particular log change in the commit message would
avoid my question. Also, note that you're still doing:

ret = ad7173_get_ref_voltage_milli(st, ref_sel);
if (ret < 0)
	return ret;

instead of:

return ad7173_get_ref_voltage_milli(...)

which defeats the purpose of having the log inside
ad7173_get_ref_voltage_milli() 

But after stepping back I see ad7173_get_ref_voltage_milli() is also being used
in a non probe path. Hence, doing the log out of the function using
dev_err_probe() may be reason enough to keep things as you had before my
comments. Will leave that up to you (sorry for the noise).

- Nuno Sá
 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs
  2024-06-07  9:34     ` Ceclan, Dumitru
@ 2024-06-07 10:29       ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07 10:29 UTC (permalink / raw)
  To: Ceclan, Dumitru, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On Fri, 2024-06-07 at 12:34 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:06, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > >  Add support for selecting REF+ and REF- inputs on all models.
> > >  Add support for selecting ((AVDD1 − AVSS)/5) inputs
> > >   on supported models.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 29 +++++++++++++++++++++++++++--
> > >  1 file changed, 27 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 4040edbd1c32..d16fa081a285 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > @@ -66,6 +66,13 @@
> > >  	 FIELD_PREP(AD7173_CH_SETUP_AINNEG_MASK, neg))
> > >  #define AD7173_AIN_TEMP_POS	17
> > >  #define AD7173_AIN_TEMP_NEG	18
> > > +#define AD7173_AIN_POW_MON_POS	19
> > > +#define AD7173_AIN_POW_MON_NEG	20
> > > +#define AD7173_AIN_REF_POS	21
> > > +#define AD7173_AIN_REF_NEG	22
> > > +
> > > +#define AD7173_IS_REF_INPUT(x)		((x) == AD7173_AIN_REF_POS || \
> > > +					(x) == AD7173_AIN_REF_NEG)
> > >  
> > >  #define AD7172_2_ID			0x00d0
> > >  #define AD7175_ID			0x0cd0
> > > @@ -146,6 +153,8 @@ struct ad7173_device_info {
> > >  	unsigned int id;
> > >  	char *name;
> > >  	bool has_temp;
> > > +	/* ((AVDD1 − AVSS)/5) */
> > > +	bool has_pow_supply_monitoring;
> > >  	bool has_input_buf;
> > >  	bool has_int_ref;
> > >  	bool has_ref2;
> > > @@ -216,6 +225,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_temp = true,
> > >  		.has_input_buf = true,
> > >  		.has_int_ref = true,
> > > +		.has_pow_supply_monitoring = true,
> > >  		.clock = 2 * HZ_PER_MHZ,
> > >  		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > >  		.num_sinc5_data_rates =
> > > ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > @@ -230,6 +240,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_temp = false,
> > >  		.has_input_buf = true,
> > >  		.has_ref2 = true,
> > > +		.has_pow_supply_monitoring = true,
> > >  		.clock = 2 * HZ_PER_MHZ,
> > >  		.sinc5_data_rates = ad7173_sinc5_data_rates,
> > >  		.num_sinc5_data_rates =
> > > ARRAY_SIZE(ad7173_sinc5_data_rates),
> > > @@ -245,6 +256,7 @@ static const struct ad7173_device_info
> > > ad7173_device_info[] = {
> > >  		.has_input_buf = true,
> > >  		.has_int_ref = true,
> > >  		.has_ref2 = true,
> > > +		.has_pow_supply_monitoring = false,
> > 
> > No need to set the 'false' cases...
> > 
> > 
> > - Nuno Sá
> 
> This was suggested by David Lechner to ensure consistency with has_temp
> regarding another field, I considered that it would apply here as well.
> https://lore.kernel.org/all/CAMknhBGaJxXvsQ8cZkgDsKLVjOY5y2pzox-99hdOCrUaoZdsxg@mail.gmail.com/
> 

Well, I would argue that the has_temp flag being set to 0 is also unneeded and
can be removed (in another patch).
> This would also increase visibility towards what features does a specific
> model support as it is clearly stated with "= false" rather than looking
> for what fields are not set within the struct.

IMO, the omission of the flag is already pretty clear that the feature is not
available. Typically we don't initialize things that do not need to be
initialized (less LOC)...

- Nuno Sá


^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices
  2024-06-07  9:41     ` Ceclan, Dumitru
@ 2024-06-07 10:39       ` Nuno Sá
  0 siblings, 0 replies; 25+ messages in thread
From: Nuno Sá @ 2024-06-07 10:39 UTC (permalink / raw)
  To: Ceclan, Dumitru, dumitru.ceclan
  Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
	Rob Herring, Krzysztof Kozlowski, Conor Dooley, David Lechner,
	linux-iio, devicetree, linux-kernel

On Fri, 2024-06-07 at 12:41 +0300, Ceclan, Dumitru wrote:
> On 07/06/2024 12:20, Nuno Sá wrote:
> > On Thu, 2024-06-06 at 19:07 +0300, Dumitru Ceclan via B4 Relay wrote:
> > > From: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > 
> > > Add support for AD4111/AD4112/AD4114/AD4115/AD4116.
> > > 
> > > The AD411X family encompasses a series of low power, low noise, 24-bit,
> > > sigma-delta analog-to-digital converters that offer a versatile range of
> > > specifications.
> > > 
> > > This family of ADCs integrates an analog front end suitable for processing
> > > both fully differential and single-ended, bipolar voltage inputs
> > > addressing a wide array of industrial and instrumentation requirements.
> > > 
> > > - All ADCs have inputs with a precision voltage divider with a division
> > >   ratio of 10.
> > > - AD4116 has 5 low level inputs without a voltage divider.
> > > - AD4111 and AD4112 support current inputs (0 mA to 20 mA) using a 50ohm
> > >   shunt resistor.
> > > 
> > > Signed-off-by: Dumitru Ceclan <dumitru.ceclan@analog.com>
> > > ---
> > >  drivers/iio/adc/ad7173.c | 317
> > > ++++++++++++++++++++++++++++++++++++++++++----
> > > -
> > >  1 file changed, 285 insertions(+), 32 deletions(-)
> > > 
> > > diff --git a/drivers/iio/adc/ad7173.c b/drivers/iio/adc/ad7173.c
> > > index 58da5717fd36..cfcd12447e24 100644
> > > --- a/drivers/iio/adc/ad7173.c
> > > +++ b/drivers/iio/adc/ad7173.c
> > > 
> > ...
> > 
> > >  static const struct ad7173_device_info ad7172_2_device_info = {
> > >  	.name = "ad7172-2",
> > >  	.id = AD7172_2_ID,
> > > -	.num_inputs = 5,
> > > +	.num_voltage_in = 5,
> > >  	.num_channels = 4,
> > >  	.num_configs = 4,
> > >  	.num_gpios = 2,
> > > +	.higher_gpio_bits = false,
> > 
> > No need to explicitly set to 'false'. Ditto for the other places...
> > 
> > ...
> > 
> > > 
> > >  static int ad7173_validate_voltage_ain_inputs(struct ad7173_state *st,
> > >  					      unsigned int ain0, unsigned
> > > int
> > > ain1)
> > >  {
> > > @@ -946,15 +1145,30 @@ static int
> > > ad7173_validate_voltage_ain_inputs(struct
> > > ad7173_state *st,
> > >  	    st->info->has_pow_supply_monitoring)
> > >  		return 0;
> > >  
> > > -	special_input0 = AD7173_IS_REF_INPUT(ain0);
> > > -	special_input1 = AD7173_IS_REF_INPUT(ain1);
> > > +	special_input0 = AD7173_IS_REF_INPUT(ain0) ||
> > > +			 (ain0 == AD4111_VINCOM_INPUT && st->info-
> > > > has_vincom_input);
> > > +	special_input1 = AD7173_IS_REF_INPUT(ain1) ||
> > > +			 (ain1 == AD4111_VINCOM_INPUT && st->info-
> > > > has_vincom_input);
> > > +
> > 
> > Wondering... can ain1 (or ain0) be AD4111_VINCOM_INPUT and !st->info-
> > > has_vincom_input? Would that actually be acceptable? It would assume it's
> > > not
> > so we should check that right? Or am I missing something?
> > 
> > - Nuno Sá
> > 
> 
> It will fail when we check for the number of voltage inputs:
> (ain0 >= st->info->num_voltage_in && !special_input0) 
> as special_input will not be true if has_vincom_input is false
> 
> Indeed this check is a bit hidden, should it be more explicit?
> 

Hmm I see... Up to you. I guess I was not paying enough attention to 
st->info->num_voltage_in and to the fact that VINCOM and REFs are not counted by
it.

OTOH, yes, an explicit check could make sense because the log you output:

"Input pin number out of range for pair..."

It's really not mentioning the real problem (or it's a very hidden message IOW).
having something like

if (ain0 == AD4111_VINCOM_INPUT && !st->info-has_vincom_input)
	return dev_err_probe(dev, -EINVAL, "VINCOM not supported for %s\n",
			part_name);

would be something way easier to understand :)

- Nuno Sá

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2024-06-07 10:35 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-06 16:07 [PATCH v6 0/9] Add support for AD411x Dumitru Ceclan via B4 Relay
2024-06-06 16:07 ` [PATCH v6 1/9] dt-bindings: iio: adc: Add common-mode-channel property Dumitru Ceclan via B4 Relay
2024-06-06 16:07 ` [PATCH v6 2/9] dt-bindings: adc: ad7173: add support for ad411x Dumitru Ceclan via B4 Relay
2024-06-06 16:38   ` Conor Dooley
2024-06-06 16:07 ` [PATCH v6 3/9] iio: adc: ad_sigma_delta: add disable_one callback Dumitru Ceclan via B4 Relay
2024-06-07  9:02   ` Nuno Sá
2024-06-07  9:29     ` Ceclan, Dumitru
2024-06-07 10:16       ` Nuno Sá
2024-06-06 16:07 ` [PATCH v6 4/9] iio: adc: ad7173: refactor channel configuration parsing Dumitru Ceclan via B4 Relay
2024-06-06 16:07 ` [PATCH v6 5/9] iio: adc: ad7173: refactor ain and vref selection Dumitru Ceclan via B4 Relay
2024-06-07  9:04   ` Nuno Sá
2024-06-07  9:37     ` Ceclan, Dumitru
2024-06-07 10:24       ` Nuno Sá
2024-06-06 16:07 ` [PATCH v6 6/9] iio: adc: ad7173: add support for special inputs Dumitru Ceclan via B4 Relay
2024-06-07  9:06   ` Nuno Sá
2024-06-07  9:34     ` Ceclan, Dumitru
2024-06-07 10:29       ` Nuno Sá
2024-06-06 16:07 ` [PATCH v6 7/9] iio: adc: ad7173: refactor device info structs Dumitru Ceclan via B4 Relay
2024-06-07  9:08   ` Nuno Sá
2024-06-06 16:07 ` [PATCH v6 8/9] iio: adc: ad7173: document sampling frequency behaviour Dumitru Ceclan via B4 Relay
2024-06-07  9:09   ` Nuno Sá
2024-06-06 16:07 ` [PATCH v6 9/9] iio: adc: ad7173: Add support for AD411x devices Dumitru Ceclan via B4 Relay
2024-06-07  9:20   ` Nuno Sá
2024-06-07  9:41     ` Ceclan, Dumitru
2024-06-07 10:39       ` Nuno Sá

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).