linux-kernel - Re: [PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

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: <d9e258cc-fa3a-4c6b-a58a-193711d1d9ec@baylibre.com>
Date: Wed, 22 Jan 2025 15:11:25 -0600
From: David Lechner <dlechner@...libre.com>
To: Alisa-Dariana Roman <alisadariana@...il.com>,
 Alisa-Dariana Roman <alisa.roman@...log.com>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-gpio@...r.kernel.org
Cc: Lars-Peter Clausen <lars@...afoo.de>,
 Michael Hennerich <Michael.Hennerich@...log.com>,
 Jonathan Cameron <jic23@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Linus Walleij <linus.walleij@...aro.org>,
 Bartosz Golaszewski <brgl@...ev.pl>
Subject: Re: [PATCH v2 1/2] dt-bindings: iio: adc: add AD7191

On 1/22/25 7:20 AM, Alisa-Dariana Roman wrote:
> AD7191 is a pin-programmable, ultralow noise 24-bit sigma-delta ADC
> designed for precision bridge sensor measurements. It features two
> differential analog input channels, selectable output rates,
> programmable gain, internal temperature sensor and simultaneous
> 50Hz/60Hz rejection.
> 
> Signed-off-by: Alisa-Dariana Roman <alisa.roman@...log.com>
> ---
>  .../bindings/iio/adc/adi,ad7191.yaml          | 175 ++++++++++++++++++
>  MAINTAINERS                                   |   7 +
>  2 files changed, 182 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> new file mode 100644
> index 000000000000..c0a6bed7a9cb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7191.yaml
> @@ -0,0 +1,175 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2025 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7191.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7191 ADC device driver
> +
> +maintainers:
> +  - Alisa-Dariana Roman <alisa.roman@...log.com>
> +
> +description: |
> +  Bindings for the Analog Devices AD7191 ADC device. Datasheet can be
> +  found here:
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD7191.pdf

If we are not going to have a powerdown-gpios, I think we should mention in the
description that it is expected that the PDOWN pin is connected to the SPI
controller chip select.

> +
> +properties:

...

> +
> +  clksel-gpios:

Do we really need this one? I don't think we have a chip yet that wants to
change the clock at runtime. We don't have this for any of the other similar
ADI sigma delta chips that have already been upstreamed.

If there is an evaluation board where the pin is wired to a GPIO, we can just
use gpio-hog to select the correct state.

> +    description: |
> +      Clock source selection pin (internal or external). Should be defined if
> +      clksel-config is absent.
> +    maxItems: 1
> +
> +  adi,clksel-state:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: |
> +      Should be present if CLKSEL is pin-strapped. 0 selects an external clock,
> +      1 selects the internal clock. If defined, clksel-gpios must be absent.
> +    enum: [0, 1]

I don't think we need this one either. If the clocks property is present, then
we can assume that CLKSEL is going to be hardwired to indicate an external
clock and if the clocks property is absent, then we know it must be hardwired
to select the internal clock.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - avdd-supply
> +  - dvdd-supply
> +  - vref-supply
> +  - spi-cpol
> +  - spi-cpha
> +  - temp-gpios
> +  - chan-gpios
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +  - if:
> +      required:
> +        - adi,odr-state
> +    then:
> +      properties:
> +        odr-gpios: false
> +    else:
> +      required:
> +        - odr-gpios

I think we could simplify these with:

- oneOf:
  - required:
     - adi,odr-state
  - required:
     - odr-gpios

> +  - if:
> +      required:
> +        - adi,pga-state
> +    then:
> +      properties:
> +        pga-gpios: false
> +    else:
> +      required:
> +        - pga-gpios
> +  - if:
> +      required:
> +        - adi,clksel-state
> +    then:
> +      properties:
> +        clksel-gpios: false
> +    else:
> +      required:
> +        - clksel-gpios
> +
> +unevaluatedProperties: false
> +