| 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
| ||
|
Date: Thu, 17 Feb 2022 16:44:42 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>
To: chegbeli <ciprian.hegbeli@...log.com>, jic23@...nel.org,
robh+dt@...nel.org, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] dt-bindings: iio: add ADE9078
On 17/02/2022 14:51, chegbeli wrote:
> Added device tree bindings for the ADE9078
>
> Signed-off-by: chegbeli <ciprian.hegbeli@...log.com>
> ---
> .../bindings/iio/meter/adi,ade9078.yaml | 153 ++++++++++++++++++
> include/dt-bindings/iio/meter/adi,ade9078.h | 21 +++
> 2 files changed, 174 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
> create mode 100644 include/dt-bindings/iio/meter/adi,ade9078.h
>
> diff --git a/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
> new file mode 100644
> index 000000000000..e27d52e06e32
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/meter/adi,ade9078.yaml
> @@ -0,0 +1,153 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +# Copyright 2021 Analog Devices Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/bindings/iio/addac/adi,ade9078.yaml#
Did you test your schema with dt_binding_check? This should fail.
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADE9078 High Performance, Polyphase Energy Metering driver
> +
> +mainterners:
> + -Ciprian Hegbeli <ciprian.hegbeli@...log.com>
Space after '-'.
> +
> +description: |
> + The ADE9078 1 is a highly accurate, fully integrated energy
> + metering device. Interfacing with both current transformer
> + (CT) and Rogowski coil sensors, the ADE9078 enables users to
> + develop a 3-phase metrology platform, which achieves high
> + performance for Class 1 up to Class 0.2 meters.
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ade9078
> +
> + reg:
> + maxItems: 1
> +
> + '#address-cells':
> + const: 1
> +
> + '#size-cells':
> + const: 0
> +
> + spi-max-frequency:
> + maximum: 1000000
> +
> + interrupts:
> + maxItems: 2
> +
> + reset-gpios:
> + description: |
> + Must be the device tree identifier of the RESET pin. As the line is
> + active low, it should be marked GPIO_ACTIVE_LOW.
> + maxItems: 1
> +
> + interrupt-names:
> + description: |
> + Names to be attributed to the interrupts of the device. Should be "irq0"
> + or "irq1"
Skip description but instead list items ( - const: irq0 ...)
> +
> + adi,wf-cap-sel:
> + description: |
> + This bit selects whether the waveform buffer is filled with resampled
> + data or fixed data rate data
> + 0 - WF_RESAMPLED_DATA
> + 1 - WF_FIXED_DATA_RATE
> + maxItems: 1
> + minimum: 0
> + maximum: 1
1. You need a type definition.
2. This looks like bool.
3. maxItems seems wrong here.
4. Do not describe registers and their bits but a feature of the device.
This applies to fields below as well.
> +
> + adi,wf-mode:
> + description: |
> + Fixed data rate waveforms filling and trigger based modes.
> + 0 - WFB_FULL_MODE (Stop when waveform buffer is full)
> + 1 - WFB_EN_TRIG_MODE (Continuous fill—stop only on enabled trigger events)
> + 2 - WFB_CENTER_EN_TRIG_MODE (Continuous filling—center capture around enabled trigger events)
> + 3 - WFB_SVAE_EN_TRIG_MODE (Continuous fill—save event address of enabled trigger events)
> + maxItems: 1
> + minimum: 0
> + maximum: 3
Everything above + this looks like an enum, so use enum, instead of min/max.
> +
> + adi,wf-src:
> + description: |
> + Waveform buffer source and DREADY, data ready update rate, selection.
> + 0 - WFB_SRC_SINC4 (Sinc4 output, at 16 kSPS)
> + 1 - Reserved
> + 2 - WFB_SRC_SINC4_IIR_LPF (Sinc4 + IIR LPF output, at 4 kSPS)
> + 3 - WFB_SRC_DSP (Current and voltage channel waveform samples,processed by the DSP
> + (xI_PCF, xV_PCF) at 4 kSPS)
> + maxItems: 1
> + minimum: 0
> + maximum: 3
> +
> + adi,wf-in-en:
> + description: |
> + This setting determines whether the IN waveform samples are read out of
> + the waveform buffer through SPI.
> + 0 - WFB_IN_DISABLE
> + 1 - WFB_IN_EN
> + maxItems: 1
> + minimum: 0
> + maximum: 1
Also bool.
> +
> + required:
> + - compatible
> + - reg
> + - reset-gpios
> + - interrupts
> + - interrupt-names
> + - adi,wf-cap-sel
> + - adi,wf-mode
> + - adi,wf-src
> + - adi,wf-in-en
> +
> +patternProperties:
> + "^phase@[0-3]$":
> + type: object
> + description: |
> + Represents the external phases which are externally connected. Each phase
> + has a current, voltage and power component
> +
> + properties:
> + reg:
> + description: |
> + The phase represented by a number
> + 0 - Phase A
> + 1 - unused
> + 2 - Phase B
> + 3 - unused
> + 4 - Phase C
> + maxItems: 1
> + minimum: 0
> + maximum: 4
> +
> + required:
> + - reg
> +
> +examples:
> + - |
> + ade9078@0 {
Generic node name. Please pick something appropriate. Maybe "meter"?
> + compatible = "adi,ade9078";
You have entirely broken indentation here. Use four spaces for DTS example.
> + reg = <0>;
> + spi-max-frequency = <7000000>;
> +
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reset-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
> + interrupts = <2 IRQ_TYPE_EDGE_FALLING>, <3 IRQ_TYPE_EDGE_FALLING>;
You clearly did not test it... this won't work without headers.
> + interrupt-names = "irq0", "irq1";
> + interrupt-parent = <&gpio>;
Best regards,
Krzysztof
Powered by blists - more mailing lists