| Message ID | 20180923041118.8743-2-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add an initial DT binding doc for ina3221 | expand |
On 09/22/2018 09:11 PM, Nicolin Chen wrote: > Texas Instruments INA3221 is a triple-channel shunt and bus > voltage monitor. This patch adds a DT binding doc for it. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > Changelog > v3->v4: > * Removed the attempt of putting labels in the node names > * Added a new optional label property in the child node > * Updated examples accordingly > v2->v3: > * Added a simple subject in the line 1 > * Fixed the shunt resistor value in the example > v1->v2: > * Dropped channel name properties > * Added child node definitions. > * * Added shunt resistor property in the child node > * * Added status property to indicate connection status > * * Changed to use child node name as the label of input source > > .../devicetree/bindings/hwmon/ina3221.txt | 42 +++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt > > diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt > new file mode 100644 > index 000000000000..7d90bfe34adb > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt > @@ -0,0 +1,42 @@ > +Texas Instruments INA3221 Device Tree Bindings > + > +1) ina3221 node > + Required properties: > + - compatible: Must be "ti,ina3221" > + - reg: I2C address > + > + = The node contains optional child nodes for three channels = > + = Each child node describes the information of input source = > + > + Example: > + > + ina3221@40 { > + compatible = "ti,ina3221"; > + reg = <0x40>; > + [ child node definitions... ] > + }; > + > +2) child nodes > + Required properties: > + - input-id: Must be 1, 2 or 3 > + > + Optional properties: > + - input-label: Name of the input source > + - shunt-resistor: Shunt resistor value in micro-Ohm > + - status: Should be "disabled" if no input source > + > + Example: > + > + input1 { > + input-id = <0x1>; We'll have to find a better name for this. Feel free to look up examples in the existing devicetree descriptions. The one that seems to be used most of the time to indicate a channel index or id is "reg". It should also start with 0 - there is no real reason for it to start with 1; it only makes the code more complex. > + status = "disabled"; > + }; > + input2 { > + input-id = <0x2>; > + shunt-resistor = <5000>; I would suggest shunt-resistor-micro-ohms as per Documentation/devicetree/bindings/property-units.txt. > + }; > + input3 { > + input-id = <0x3>; > + input-label = "VDD_5V"; > + shunt-resistor = <5000>; > + }; >
On Sat, Sep 22, 2018 at 10:19:42PM -0700, Guenter Roeck wrote: > >+2) child nodes > >+ Required properties: > >+ - input-id: Must be 1, 2 or 3 > >+ > >+ Optional properties: > >+ - input-label: Name of the input source > >+ - shunt-resistor: Shunt resistor value in micro-Ohm > >+ - status: Should be "disabled" if no input source > >+ > >+ Example: > >+ > >+ input1 { > >+ input-id = <0x1>; > > We'll have to find a better name for this. Feel free to look up examples in the > existing devicetree descriptions. The one that seems to be used most of the time > to indicate a channel index or id is "reg". It should also start with 0 - there > is no real reason for it to start with 1; it only makes the code more complex. The reason is that the port start from 1 in the datasheet. I don't mind using reg or count it from 0, will look up to see if I can find something solid; otherwise, I'll wait for binding doc maintainers' opinions before sending v5. > >+ status = "disabled"; > >+ }; > >+ input2 { > >+ input-id = <0x2>; > >+ shunt-resistor = <5000>; > > I would suggest shunt-resistor-micro-ohms as per > Documentation/devicetree/bindings/property-units.txt. Will change it. Thank you Nicolin
On 09/22/2018 10:31 PM, Nicolin Chen wrote: > On Sat, Sep 22, 2018 at 10:19:42PM -0700, Guenter Roeck wrote: >>> +2) child nodes >>> + Required properties: >>> + - input-id: Must be 1, 2 or 3 >>> + >>> + Optional properties: >>> + - input-label: Name of the input source >>> + - shunt-resistor: Shunt resistor value in micro-Ohm >>> + - status: Should be "disabled" if no input source >>> + >>> + Example: >>> + >>> + input1 { >>> + input-id = <0x1>; >> >> We'll have to find a better name for this. Feel free to look up examples in the >> existing devicetree descriptions. The one that seems to be used most of the time >> to indicate a channel index or id is "reg". It should also start with 0 - there >> is no real reason for it to start with 1; it only makes the code more complex. > > The reason is that the port start from 1 in the datasheet. > Maybe, but for me I'll want to have something that we can reuse for other chips. Having the index start with 0 for one chip and with 1 for another would be confusing. It is bad enough that we have in[0..n] for voltages and temp[1..n] for temperatures. I would not want to see the same in devicetree files, and much less so on a per-device basis. It is also pretty common to start channel numbers with 0 in devicetree files. > I don't mind using reg or count it from 0, will look up to > see if I can find something solid; otherwise, I'll wait for > binding doc maintainers' opinions before sending v5. > Sure, no problem. Guenter >>> + status = "disabled"; >>> + }; >>> + input2 { >>> + input-id = <0x2>; >>> + shunt-resistor = <5000>; >> >> I would suggest shunt-resistor-micro-ohms as per >> Documentation/devicetree/bindings/property-units.txt. > > Will change it. > > Thank you > Nicolin >
On Sat, Sep 22, 2018 at 10:45:49PM -0700, Guenter Roeck wrote: > On 09/22/2018 10:31 PM, Nicolin Chen wrote: > > On Sat, Sep 22, 2018 at 10:19:42PM -0700, Guenter Roeck wrote: > > > > +2) child nodes > > > > + Required properties: > > > > + - input-id: Must be 1, 2 or 3 > > > > + > > > > + Optional properties: > > > > + - input-label: Name of the input source > > > > + - shunt-resistor: Shunt resistor value in micro-Ohm > > > > + - status: Should be "disabled" if no input source > > > > + > > > > + Example: > > > > + > > > > + input1 { > > > > + input-id = <0x1>; > > > > > > We'll have to find a better name for this. Feel free to look up examples in the > > > existing devicetree descriptions. The one that seems to be used most of the time > > > to indicate a channel index or id is "reg". It should also start with 0 - there > > > is no real reason for it to start with 1; it only makes the code more complex. > > > > The reason is that the port start from 1 in the datasheet. > > > > Maybe, but for me I'll want to have something that we can reuse for other chips. > Having the index start with 0 for one chip and with 1 for another would be > confusing. It is bad enough that we have in[0..n] for voltages and temp[1..n] > for temperatures. I would not want to see the same in devicetree files, > and much less so on a per-device basis. It is also pretty common to start > channel numbers with 0 in devicetree files. Understood. I search a bit and saw most of "*-id" start from 0, although I cannot be sure whether their Datasheet/RM/schematics are counting from 0 or 1. And I also found a transposing example: Documentation/devicetree/bindings/serial/mrvl,pxa-ssp.txt Probably should be better to wait for doc maintainers' input. Thanks Nicolin
On 09/22/2018 11:01 PM, Nicolin Chen wrote: > On Sat, Sep 22, 2018 at 10:45:49PM -0700, Guenter Roeck wrote: >> On 09/22/2018 10:31 PM, Nicolin Chen wrote: >>> On Sat, Sep 22, 2018 at 10:19:42PM -0700, Guenter Roeck wrote: >>>>> +2) child nodes >>>>> + Required properties: >>>>> + - input-id: Must be 1, 2 or 3 >>>>> + >>>>> + Optional properties: >>>>> + - input-label: Name of the input source Just noticed the "input-" here. Please just use "label". >>>>> + - shunt-resistor: Shunt resistor value in micro-Ohm >>>>> + - status: Should be "disabled" if no input source >>>>> + >>>>> + Example: >>>>> + >>>>> + input1 { >>>>> + input-id = <0x1>; >>>> >>>> We'll have to find a better name for this. Feel free to look up examples in the >>>> existing devicetree descriptions. The one that seems to be used most of the time >>>> to indicate a channel index or id is "reg". It should also start with 0 - there >>>> is no real reason for it to start with 1; it only makes the code more complex. >>> >>> The reason is that the port start from 1 in the datasheet. >>> >> >> Maybe, but for me I'll want to have something that we can reuse for other chips. >> Having the index start with 0 for one chip and with 1 for another would be >> confusing. It is bad enough that we have in[0..n] for voltages and temp[1..n] >> for temperatures. I would not want to see the same in devicetree files, >> and much less so on a per-device basis. It is also pretty common to start >> channel numbers with 0 in devicetree files. > > Understood. I search a bit and saw most of "*-id" start from 0, > although I cannot be sure whether their Datasheet/RM/schematics > are counting from 0 or 1. > > And I also found a transposing example: > Documentation/devicetree/bindings/serial/mrvl,pxa-ssp.txt > You'll always find an example for anything in the kernel. In this case, it is for one specific chip. I'll want this to be reusable as template for _all_ hardware monitoring chips. Thanks, Guenter > Probably should be better to wait for doc maintainers' input. > > Thanks > Nicolin >
On Sat, Sep 22, 2018 at 11:36:22PM -0700, Guenter Roeck wrote: > >>>>>+ Optional properties: > >>>>>+ - input-label: Name of the input source > > Just noticed the "input-" here. Please just use "label". Will fix in v5. > >>>>>+ input1 { > >>>>>+ input-id = <0x1>; > >>>> > >>>>We'll have to find a better name for this. Feel free to look up examples in the > >>>>existing devicetree descriptions. The one that seems to be used most of the time > >>>>to indicate a channel index or id is "reg". It should also start with 0 - there > >>>>is no real reason for it to start with 1; it only makes the code more complex. > >>> > >>>The reason is that the port start from 1 in the datasheet. > >>> > >> > >>Maybe, but for me I'll want to have something that we can reuse for other chips. > >>Having the index start with 0 for one chip and with 1 for another would be > >>confusing. It is bad enough that we have in[0..n] for voltages and temp[1..n] > >>for temperatures. I would not want to see the same in devicetree files, > >>and much less so on a per-device basis. It is also pretty common to start > >>channel numbers with 0 in devicetree files. > > > >Understood. I search a bit and saw most of "*-id" start from 0, > >although I cannot be sure whether their Datasheet/RM/schematics > >are counting from 0 or 1. > > > >And I also found a transposing example: > > Documentation/devicetree/bindings/serial/mrvl,pxa-ssp.txt > > > > You'll always find an example for anything in the kernel. In this case, > it is for one specific chip. I'll want this to be reusable as template for > _all_ hardware monitoring chips. I see. Thanks Nicolin
diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt new file mode 100644 index 000000000000..7d90bfe34adb --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -0,0 +1,42 @@ +Texas Instruments INA3221 Device Tree Bindings + +1) ina3221 node + Required properties: + - compatible: Must be "ti,ina3221" + - reg: I2C address + + = The node contains optional child nodes for three channels = + = Each child node describes the information of input source = + + Example: + + ina3221@40 { + compatible = "ti,ina3221"; + reg = <0x40>; + [ child node definitions... ] + }; + +2) child nodes + Required properties: + - input-id: Must be 1, 2 or 3 + + Optional properties: + - input-label: Name of the input source + - shunt-resistor: Shunt resistor value in micro-Ohm + - status: Should be "disabled" if no input source + + Example: + + input1 { + input-id = <0x1>; + status = "disabled"; + }; + input2 { + input-id = <0x2>; + shunt-resistor = <5000>; + }; + input3 { + input-id = <0x3>; + input-label = "VDD_5V"; + shunt-resistor = <5000>; + };
Texas Instruments INA3221 is a triple-channel shunt and bus voltage monitor. This patch adds a DT binding doc for it. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- Changelog v3->v4: * Removed the attempt of putting labels in the node names * Added a new optional label property in the child node * Updated examples accordingly v2->v3: * Added a simple subject in the line 1 * Fixed the shunt resistor value in the example v1->v2: * Dropped channel name properties * Added child node definitions. * * Added shunt resistor property in the child node * * Added status property to indicate connection status * * Changed to use child node name as the label of input source .../devicetree/bindings/hwmon/ina3221.txt | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt