Re: [PATCH v2] dt-bindings: iio: adc: add documentation for adi,ad7816

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] dt-bindings: iio: adc: missing maxItems on gpios and ad7818 busy constraint
- [Medium] dt-bindings: iio: adc: missing spi-peripheral-props.yaml ref in adi,ad7816
- [Low] dt-bindings: iio: adc: wrong IRQ trigger type in adi,ad7816 example
--

commit 172592494e92c865cd70820f7a6919506c5b2b75
Author: Tenglong Guo <admin@micronull.cn>

dt-bindings: iio: adc: add documentation for adi,ad7816

This commit adds a devicetree YAML binding schema for the Analog Devices
AD7816, AD7817, and AD7818 SPI ADC family.

> diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7816.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7816.yaml
> new file mode 100644
> index 0000000000000..d23ef9163e61e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7816.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/adc/adi,ad7816.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7816/AD7817/AD7818 ADC
> +
> +maintainers:
> +  - Tenglong Guo <admin@micronull.cn>
> +
> +description: |
> +  The AD7816/AD7817/AD7818 are 10-bit ADCs with on-chip temperature sensors.
> +  They use a SPI interface for communication.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad7816
> +      - adi,ad7817
> +      - adi,ad7818
> +
> +  reg:
> +    maxItems: 1
> +
> +  rdwr-gpios:
> +    description: GPIO connected to the RD/WR pin.
> +
> +  convert-gpios:
> +    description: GPIO connected to the CONVST pin.
> +
> +  busy-gpios:
> +    description: GPIO connected to the BUSY pin. Required for AD7816 and AD7817.
> +
> +  interrupts:
> +    maxItems: 1
> +    description: Optional interrupt for over-temperature indication.
> +
> +required:
> +  - compatible
> +  - reg
> +  - rdwr-gpios
> +  - convert-gpios
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad7816
> +              - adi,ad7817
> +    then:
> +      required:
> +        - busy-gpios

[Severity: Medium]
Should rdwr-gpios, convert-gpios, and busy-gpios specify maxItems: 1, and
should the allOf conditional block disallow busy-gpios on adi,ad7818?

In drivers/staging/iio/adc/ad7816.c, ad7816_probe() requests a single GPIO
descriptor for each line and only requests busy on AD7816 and AD7817:

drivers/staging/iio/adc/ad7816.c:ad7816_probe() {
	...
	chip->rdwr_pin = devm_gpiod_get(&spi_dev->dev, "rdwr", GPIOD_OUT_HIGH);
	...
	chip->convert_pin = devm_gpiod_get(&spi_dev->dev, "convert",
					   GPIOD_OUT_HIGH);
	...
	if (chip->id == ID_AD7816 || chip->id == ID_AD7817) {
		chip->busy_pin = devm_gpiod_get(&spi_dev->dev, "busy",
						GPIOD_IN);
	...
}

Without maxItems: 1 on rdwr-gpios, convert-gpios, and busy-gpios,
dt_binding_check fails meta-schema validation against meta-schemas/gpios.yaml.
Also, because the 8-pin AD7818 does not have a physical BUSY pin, adding an
else branch setting busy-gpios: false would prevent dtbs_check from accepting
busy-gpios on adi,ad7818 nodes.

> +
> +additionalProperties: false

[Severity: Medium]
Since the AD7816 family are SPI peripherals, does this binding need to
reference /schemas/spi/spi-peripheral-props.yaml# in allOf and use
unevaluatedProperties: false instead of additionalProperties: false?

Without referencing spi-peripheral-props.yaml# and switching to
unevaluatedProperties: false, device tree nodes using standard SPI properties
such as spi-max-frequency or spi-cpol will fail dtbs_check validation.

> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    spi {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        adc@0 {
> +            compatible = "adi,ad7816";
> +            reg = <0>;
> +            rdwr-gpios = <&gpio0 1 GPIO_ACTIVE_HIGH>;
> +            convert-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
> +            busy-gpios = <&gpio0 3 GPIO_ACTIVE_HIGH>;
> +            interrupts-extended = <&gpio0 4 IRQ_TYPE_EDGE_FALLING>;

[Severity: Low]
Should the example use IRQ_TYPE_LEVEL_LOW instead of IRQ_TYPE_EDGE_FALLING?

The Over-Temperature Indicator (OTI) output is active-low level, and
ad7816_probe() in drivers/staging/iio/adc/ad7816.c explicitly requests a
low-level trigger:

drivers/staging/iio/adc/ad7816.c:ad7816_probe() {
	...
	if (spi_dev->irq) {
		/* Only low trigger is supported in ad7816/7/8 */
		ret = devm_request_threaded_irq(&spi_dev->dev, spi_dev->irq,
						NULL,
						&ad7816_event_handler,
						IRQF_TRIGGER_LOW | IRQF_ONESHOT,
						indio_dev->name,
						indio_dev);
	...
}

> +        };
> +    };
> +...

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018104358.38255-1-wrmfwv@163.com?part=1