linux-kernel - Re: [PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8d6b39c8-a00c-4440-8451-70eac566c544@baylibre.com>
Date: Wed, 19 Jun 2024 14:57:18 -0500
From: David Lechner <dlechner@...libre.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>, broonie@...nel.org,
 lars@...afoo.de, Michael.Hennerich@...log.com, jic23@...nel.org,
 robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 nuno.sa@...log.com, marcelo.schmitt1@...il.com
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-spi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/6] dt-bindings: iio: adc: Add AD4000

On 6/18/24 6:12 PM, Marcelo Schmitt wrote:
> Add device tree documentation for AD4000 series of ADC devices.
> 
...

> +  adi,spi-mode:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum: [ single, chain ]
> +    description: |
> +      This property indicates the SPI wiring configuration.
> +
> +      When this property is omitted, it is assumed that the device is using what
> +      the datasheet calls "4-wire mode". This is the conventional SPI mode used
> +      when there are multiple devices on the same bus. In this mode, the CNV
> +      line is used to initiate the conversion and the SDI line is connected to
> +      CS on the SPI controller.
> +
> +      When this property is present, it indicates that the device is using one
> +      of the following alternative wiring configurations:
> +
> +      * single: The datasheet calls this "3-wire mode". (NOTE: The datasheet's
> +        definition of 3-wire mode is NOT at all related to the standard
> +        spi-3wire property!) This mode is often used when the ADC is the only
> +        device on the bus. In this mode, SDI is connected to MOSI or to VIO, and
> +        the CNV line can be connected to the CS line of the SPI controller or to
> +        a GPIO, in which case the CS line of the controller is unused.
> +      * chain: The datasheet calls this "chain mode". This mode is used to save
> +        on wiring when multiple ADCs are used. In this mode, the SDI line of
> +        one chip is tied to the SDO of the next chip in the chain and the SDI of
> +        the last chip in the chain is tied to GND. Only the first chip in the
> +        chain is connected to the SPI bus. The CNV line of all chips are tied
> +        together. The CS line of the SPI controller can be used as the CNV line
> +        only if it is active high.
> +

After reviewing the driver and going back and looking at the diagrams in [1] again,
I think we are missing a wiring mode here. What the driver is calling "single/3-wire"
mode is actually using 4 wires and is the wiring mode that I suggested should be
the default since that is the only wiring configuration where we can read/write
registers.

[1]: https://lore.kernel.org/linux-iio/87058695-a1a6-4e68-87c5-accdb8451bf4@baylibre.com/

So to recap, this is what I suggest we should do:

default unnamed mode:

  * Wiring:
      ADC    HOST
      ---    ----
      CNV    CS (or GPIO)
      SDI    SDO
      SDO    SDI
      SCLK   SCLK
  * Requires SPI controller with SPI_MOSI_IDLE_HIGH/LOW capability
  * Can read/write registers
  * Can do "3-wire mode"-style reads (turbo and not turbo)
    * Requires SPI_MOSI_IDLE_HIGH
  * Can do "4-wire mode"-style reads (turbo and not turbo)
    * Requires SPI_MOSI_IDLE_HIGH, SPI_CS_HIGH (or no CS and cnv-gpios)
  * Can do "daisy-chain mode"-style reads
    * Requires SPI_MOSI_IDLE_LOW, SPI_CS_HIGH (or no CS and cnv-gpios)
  * #daisy-chained-devices is optional

"single" mode:

  * Wiring: same as default except ADC SDI is hard-wired to logic high.
  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "3-wire mode"-style reads (turbo and not turbo)
  * #daisy-chained-devices is forbidden
  * Use case: save one wire, works with any SPI controller

"multi" mode:

  * Wiring:
      ADC    HOST
      ---    ----
      CNV    GPIO
      SDI    CSn
      SDO    SDI
      SCLK   SCLK

  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "4-wire mode"-style reads (not turbo)
  * #daisy-chained-devices is forbidden
  * Use case: multiple ADCs can share one CNV trigger

"chain" mode:

  * Wiring: same as default except ADC SDI is hard-wired to logic low.
  * Cannot read/write registers.
  * Doesn't require special SPI controller (no SPI_MOSI_IDLE_HIGH/LOW)
  * Can do "daisy-chain mode"-style reads (requires CS high or cnv-gpios)
  * #daisy-chained-devices is required
  * Use case: save one wire, works with any SPI controller

---

To put it more simply in the bindings though, really this property
is just describing how the SDI pin is wired. (CNV pin wiring can be
inferred from this property and presence or absence of cnv-gpios
property.) So maybe better would be:

  adi,sdi-pin:
    $ref: /schemas/types.yaml#/definitions/string
    enum: [ high, low, cs ]
    description:
       Describes how the ADC SDI pin is wired. When this property is
       omitted, ADC SDI is connected to host SDO. "high" indicates
       that the ADC SDI pin is hard-wired to logic high (VIO).
       "low" indicates that it is hard-wired low (GND). "cs" indicates
       that the ADC SDI pin is connected to the host CS line.

And put a note about the specialized SPI controller requirements in
the main description.