On 26/08/2023 09:53, Krzysztof Kozlowski wrote:
On 25/08/2023 18:42, Ninad Malwade wrote:
Convert the TI INA3221 bindings from the free-form text format to
json-schema.
Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
Signed-off-by: Ninad Malwade <nmalwade@xxxxxxxxxx>
---
This is v2, so where is the changelog?
Indeed. This was the first time this patch was included with the patch
that extends the ina3221 driver. The original patch was here:
https://lore.kernel.org/lkml/20221108045243.24143-1-nmalwade@xxxxxxxxxx/
.../devicetree/bindings/hwmon/ina3221.txt | 54 ---------
.../devicetree/bindings/hwmon/ti,ina3221.yaml | 109 ++++++++++++++++++
2 files changed, 109 insertions(+), 54 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt
create mode 100644 Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
...
diff --git a/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
new file mode 100644
index 000000000000..0c6d41423d8c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/ti,ina3221.yaml
@@ -0,0 +1,109 @@
+# SPDX-License-Identifier: GPL-2.0-only
I assume you do not use standard license because of copying the description?
Probably just an oversight. I assume we can just update to be dual licensed?
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/ti,ina3221.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments INA3221 Current and Voltage Monitor
+
+maintainers:
+ - Jean Delvare <jdelvare@xxxxxxxx>
+ - Guenter Roeck <linux@xxxxxxxxxxxx>
+
+properties:
+ compatible:
+ const: ti,ina3221
+
+ reg:
+ maxItems: 1
+
+ ti,single-shot:
+ description: |
+ This chip has two power modes: single-shot (chip takes one measurement
+ and then shuts itself down) and continuous (chip takes continuous
+ measurements). The continuous mode is more reliable and suitable for
+ hardware monitor type device, but the single-shot mode is more power-
+ friendly and useful for battery-powered device which cares power
+ consumptions while still needs some measurements occasionally.
+
+ If this property is present, the single-shot mode will be used, instead
+ of the default continuous one for monitoring.
+ $ref: /schemas/types.yaml#/definitions/flag
+
+ "#address-cells":
+ description: Required only if a child node is present.
+ const: 1
+
+ "#size-cells":
+ description: Required only if a child node is present.
+ const: 0
+
+patternProperties:
+ "^input@[0-2]$":
+ description: The node contains optional child nodes for three channels.
+ Each child node describes the information of input source.
+ type: object
+ properties:
+ reg:
+ description: Must be 0, 1 and 2, corresponding to the IN1, IN2 or IN3
+ ports of the INA3221, respectively.
+ enum: [ 0, 1, 2 ]
+
+ label:
+ description: name of the input source
+
+ shunt-resistor-micro-ohms:
+ description: shunt resistor value in micro-Ohm
+
+ additionalProperties: false
This should be rather after type:object for readability.
OK
+
+ required:
+ - reg
+
+additionalProperties: false
And this please keep like in example schema, so after required:.
+
+required:
+ - compatible
+ - reg
+
+examples:
+ - |
+ #include <dt-bindings/clock/tegra186-clock.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/reset/tegra186-reset.h>
+
+ i2c@3160000 {
+ compatible = "nvidia,tegra186-i2c";
+ reg = <0x03160000 0x10000>;
+ interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&bpmp TEGRA186_CLK_I2C1>;
+ clock-names = "div-clk";
+ resets = <&bpmp TEGRA186_RESET_I2C1>;
+ reset-names = "i2c";
Drop all this. Not related, You only need i2c node with address/size-cells.
OK
+
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ina3221@40 {
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
+ compatible = "ti,ina3221";
+ reg = <0x40>;
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ input@0 {
+ reg = <0x0>;
+ status = "disabled";
Why is this node present? Binding said nodes are optional, so I assume
it can be just skipped. If all children must be there, then you should
actually require them in the binding (and mention it briefly in commit msg).
We can check this.
Thanks
Jon
--
nvpublic
