| Message ID | 20230825164249.22860-2-nmalwade@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: ina3221: Add selective summation support | expand |
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@nvidia.com> > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > --- This is v2, so where is the changelog? > .../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? > +%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@suse.com> > + - Guenter Roeck <linux@roeck-us.net> > + > +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. > + > + 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. > + > + #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). > + }; Best regards, Krzysztof
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@nvidia.com> >> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> >> --- > > 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@nvidia.com/ > >> .../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@suse.com> >> + - Guenter Roeck <linux@roeck-us.net> >> + >> +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
On 29/08/2023 14:46, Jon Hunter wrote: > it 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? checkpatch would complain, which means it was not run? Dual license, please, as asked by checkpatch. Best regards, Krzysztof
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@nvidia.com> >> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> ... >> + 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). I have taken a look at this and if the 'input@0' is omitted above the driver still enables it. It only disables it or marks as disconnected if the node is present but no enabled. So we can mark these as required. Is there a better way to mark them as required apart from listing all input channels under required? Jon
On Fri, Sep 01, 2023 at 05:34:00PM +0100, Jon Hunter wrote: > > 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@nvidia.com> > > > Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> > > ... > > > > + 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). > > > I have taken a look at this and if the 'input@0' is omitted above the driver > still enables it. It only disables it or marks as disconnected if the node > is present but no enabled. So we can mark these as required. > > Is there a better way to mark them as required apart from listing all input > channels under required? > Channels should by default be enabled because they are enabled by default in the chip. Requiring that all nodes be listed in devicetree just to select the default behavior would be overkill. Jusat because the chip has the ability to disable channels, that should really not be made the default. Guenter
On 01/09/2023 17:44, Guenter Roeck wrote: > On Fri, Sep 01, 2023 at 05:34:00PM +0100, Jon Hunter wrote: >> >> 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@nvidia.com> >>>> Signed-off-by: Ninad Malwade <nmalwade@nvidia.com> >> >> ... >> >>>> + 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). >> >> >> I have taken a look at this and if the 'input@0' is omitted above the driver >> still enables it. It only disables it or marks as disconnected if the node >> is present but no enabled. So we can mark these as required. >> >> Is there a better way to mark them as required apart from listing all input >> channels under required? >> > > Channels should by default be enabled because they are enabled by default > in the chip. Requiring that all nodes be listed in devicetree just to > select the default behavior would be overkill. > > Jusat because the chip has the ability to disable channels, that should > really not be made the default. Yes and that is fine. What we are trying to sort out here is what should the example contain in the dt-binding doc? The original example from the text binding doc was replicated to the yaml version and based upon the above that seems like a valid example. What I could do is add an extra comment under the 'input' property that "Input channels default to enabled in the chip. Unless channels are explicitly disabled in device-tree, input channels will be enabled". Jon
diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt deleted file mode 100644 index fa63b6171407..000000000000 --- a/Documentation/devicetree/bindings/hwmon/ina3221.txt +++ /dev/null @@ -1,54 +0,0 @@ -Texas Instruments INA3221 Device Tree Bindings - -1) ina3221 node - Required properties: - - compatible: Must be "ti,ina3221" - - reg: I2C address - - Optional properties: - - ti,single-shot: 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. - - = The node contains optional child nodes for three channels = - = Each child node describes the information of input source = - - - #address-cells: Required only if a child node is present. Must be 1. - - #size-cells: Required only if a child node is present. Must be 0. - -2) child nodes - Required properties: - - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221 - - Optional properties: - - label: Name of the input source - - shunt-resistor-micro-ohms: Shunt resistor value in micro-Ohm - -Example: - -ina3221@40 { - compatible = "ti,ina3221"; - reg = <0x40>; - #address-cells = <1>; - #size-cells = <0>; - - input@0 { - reg = <0x0>; - status = "disabled"; - }; - input@1 { - reg = <0x1>; - shunt-resistor-micro-ohms = <5000>; - }; - input@2 { - reg = <0x2>; - label = "VDD_5V"; - shunt-resistor-micro-ohms = <5000>; - }; -}; 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 +%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@suse.com> + - Guenter Roeck <linux@roeck-us.net> + +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 + + required: + - reg + +additionalProperties: false + +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"; + + #address-cells = <1>; + #size-cells = <0>; + + ina3221@40 { + compatible = "ti,ina3221"; + reg = <0x40>; + #address-cells = <1>; + #size-cells = <0>; + + input@0 { + reg = <0x0>; + status = "disabled"; + }; + + input@1 { + reg = <0x1>; + shunt-resistor-micro-ohms = <5000>; + }; + + input@2 { + reg = <0x2>; + label = "VDD_5V"; + shunt-resistor-micro-ohms = <5000>; + }; + }; + };