[v4,1/2] dt-bindings: hwmon: Add ina3221 documentation

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

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