linux-kernel - Re: [PATCH v12 1/2] dt-bindings: adc: add AD7173

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: <20240118-lunar-anthem-31bf3b9b351d@spud>
Date: Thu, 18 Jan 2024 15:23:40 +0000
From: Conor Dooley <conor@...nel.org>
To: Dumitru Ceclan <mitrutzceclan@...il.com>
Cc: linus.walleij@...aro.org, brgl@...ev.pl, andy@...nel.org,
	linux-gpio@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>,
	Jonathan Cameron <jic23@...nel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Michael Walle <michael@...le.cc>,
	Andy Shevchenko <andy.shevchenko@...il.com>,
	Arnd Bergmann <arnd@...db.de>, ChiaEn Wu <chiaen_wu@...htek.com>,
	Niklas Schnelle <schnelle@...ux.ibm.com>,
	Leonard Göhrs <l.goehrs@...gutronix.de>,
	Mike Looijmans <mike.looijmans@...ic.nl>,
	Haibo Chen <haibo.chen@....com>,
	Hugo Villeneuve <hvilleneuve@...onoff.com>,
	David Lechner <dlechner@...libre.com>,
	Ceclan Dumitru <dumitru.ceclan@...log.com>,
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v12 1/2] dt-bindings: adc: add AD7173

On Thu, Jan 18, 2024 at 02:49:22PM +0200, Dumitru Ceclan wrote:
> The AD7173 family offer a complete integrated Sigma-Delta ADC solution
> which can be used in high precision, low noise single channel applications
> or higher speed multiplexed applications. The Sigma-Delta ADC is intended
> primarily for measurement of signals close to DC but also delivers
> outstanding performance with input bandwidths out to ~10kHz.
> 
> Signed-off-by: Dumitru Ceclan <mitrutzceclan@...il.com>
> ---
> 
> V11->V12
>  - Drop "binding", describe hardware in binding description
>  - Rename refin and refin2 to vref and vref2
>  - Add better description to external references to better show that the voltage
>     value that needs to be specified is the difference between the positive and
>     negative reference pins
>  - Add optional clocks properties
>  - Add optional adi,clock-select property
>  - Add option for second interrupt, error
>  - Add description to interrupts
> V10->V11
>  - Fix example warning: '#gpio-cells' is a dependency of 'gpio-controller'
>  - Add description to #gpio-cells property
> V9->V10
>  - Fix dt_binding_check type warning from adi,reference-select
> V8->v9
>  - Add gpio-controller and "#gpio-cells" properties
>  - Add missing avdd2 and iovdd supplies
>  - Add string type to reference-select
> V7->V8
>  - include missing fix from V6
> V6->V7 <no changes>
> V5->V6
>  - Moved global required property to proper placement
> V4 -> V5
>  - Use string enum instead of integers for "adi,reference-select"
>  - Fix conditional checking in regards to compatible
> V3 -> V4
>  - include supply attributes
>  - add channel attribute for selecting conversion reference
> V2 -> V3
>  - remove redundant descriptions
>  - use referenced 'bipolar' property
>  - remove newlines from example
> V1 -> V2 <no changes>
> 
>  .../bindings/iio/adc/adi,ad7173.yaml          | 242 ++++++++++++++++++
>  1 file changed, 242 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> new file mode 100644
> index 000000000000..4d0870cc014c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7173.yaml
> @@ -0,0 +1,242 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2023 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7173.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7173 ADC
> +
> +maintainers:
> +  - Ceclan Dumitru <dumitru.ceclan@...log.com>
> +
> +description: |
> +  Analog Devices AD717x ADC's:
> +  The AD717x family offer a complete integrated Sigma-Delta ADC solution which
> +  can be used in high precision, low noise single channel applications
> +  (Life Science measurements) or higher speed multiplexed applications
> +  (Factory Automation PLC Input modules). The Sigma-Delta ADC is intended
> +  primarily for measurement of signals close to DC but also delivers
> +  outstanding performance with input bandwidths out to ~10kHz.
> +
> +  Datasheets for supported chips:
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7172-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7173-8.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7175-2.pdf
> +    https://www.analog.com/media/en/technical-documentation/data-sheets/AD7176-2.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7172-2
> +      - adi,ad7173-8
> +      - adi,ad7175-2
> +      - adi,ad7176-2
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    minItems: 1
> +    description: |
> +      Ready interrupt: multiplexed with SPI data out. While SPI CS is low,
> +      can be used to indicate the completion of a conversion.
> +
> +      Error: The three error bits in the status register (ADC_ERROR, CRC_ERROR,
> +      and REG_ERROR) are OR'ed, inverted, and mapped to the ERROR pin. Therefore,
> +      the ERROR pin indicates that an error has occurred.

Instead of having a single description like this, you can create an
items list with two descriptions, one for each interrupt.

> +
> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: rdy
> +      - const: err

> +  clocks:
> +    maxItems: 1
> +    description: |
> +      Optional external clock source. Can include one clock source: external
> +      clock or external crystal.
> +
> +  clock-names:
> +    enum:
> +      - ext-clk
> +      - xtal
> +
> +  adi,clock-select:
> +    description: |
> +      Select the ADC clock source. Valid values are:
> +      int         : Internal oscillator
> +      int-out     : Internal oscillator with output on XTAL2 pin
> +      ext-clk     : External clock input on XTAL2 pin
> +      xtal        : External crystal on XTAL1 and XTAL2 pins
> +
> +    $ref: /schemas/types.yaml#/definitions/string
> +    enum:
> +      - int
> +      - int-out
> +      - ext-clk
> +      - xtal
> +    default: int

I am not a fan of properties like this one, that in my view reimplement
things that are supported by the regular clocks properties. I've got
some questions for you so I can understand whether or not this custom
property is required.

Whether or not the ext-clk or xtal is used is known based on
clock-names - why is the custom property required to determine that?
If neither of those clocks are present, then the internal clock would be
used. Choosing to use the internal clock if an external one is provided
sounds to me like a software policy decision made by the operating
system.

Finally, if the ADC has a clock output, why can that not be represented
by making the ADC a clock-controller?

Cheers,
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)