| Message ID | 20180925225930.31886-2-nicoleotsuka@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add an initial DT binding doc for ina3221 | expand |
Hi Nicolin, On 09/25/2018 03:59 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 > v4->v5: > * Replaced "input-id" with "reg" and added address-cells and size-cells > * Replaced "input-label" with "label" > * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms" > 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 | 49 +++++++++++++++++++ > 1 file changed, 49 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..e17a897f4803 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt > @@ -0,0 +1,49 @@ > +Texas Instruments INA3221 Device Tree Bindings > + > +1) ina3221 node > + Required properties: > + - compatible: Must be "ti,ina3221" > + - reg: I2C address > + > + Optional properties: > + = 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. > + > + Example: > + > + ina3221@40 { > + compatible = "ti,ina3221"; > + reg = <0x40>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + [ child node definitions... ] > + }; > + > +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 > + - status: Should be "disabled" if no input source > + > + Example: > + > + input@0 { > + reg = <0x0>; > + status = "disabled"; I kind of feel embarrassed that I asked for the reg change ... especially while saying at the same time that I would like to see this work for other chips as well. Other chips have different kinds of sensors. Voltage, current, power, temperature, and others. Whatever we come up with should support that. I see two possibilities right now. We can stick with reg and add a "type" property, or we can make the index something like {voltage,current,power,temperature,humidity}-{id,index} I personally prefer "type", but I don't really have a strong opinion. What do you think ? Or maybe we should really wait for feedback from Rob. Thanks, Guenter > + }; > + input@1 { > + reg = <0x1>; > + shunt-resistor-micro-ohms = <5000>; > + }; > + input@2 { > + reg = <0x2>; > + label = "VDD_5V"; > + shunt-resistor-micro-ohms = <5000>; > + }; >
Hello Guenter, On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote: > > +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 > > + - status: Should be "disabled" if no input source > > + > > + Example: > > + > > + input@0 { > > + reg = <0x0>; > > + status = "disabled"; > > I kind of feel embarrassed that I asked for the reg change ... especially while > saying at the same time that I would like to see this work for other chips > as well. Well, though I didn't mention it, yet I changed it to "reg" is more likely an agreement than a compromise: I searched in the mail list and then found this mail (a year ago though): https://www.spinics.net/lists/kernel/msg2455439.html I feel it very similar to my case. So rather than betting Rob won't tell me the same, changing to "reg" may reduce a turnaround time :) > Other chips have different kinds of sensors. Voltage, current, power, temperature, > and others. Whatever we come up with should support that. > > I see two possibilities right now. We can stick with reg and add a "type" property, > or we can make the index something like > {voltage,current,power,temperature,humidity}-{id,index} One small concern is a case of being multi-type. For example, I saw ina2xx driver having voltage, current and power at the same time... > I personally prefer "type", but I don't really have a strong opinion. > What do you think ? I also like this over "reg" -- "reg" requires two extra properties, and itself sounds slightly unnatural to me for situations like this one where we don't use it as a register address, although I know it is convenient and common to use. > Or maybe we should really wait for feedback from Rob. Personally I don't mind it all to change the doc and code and then send a v6. But eventually we'll still need the final Acked-by from Rob right? Then I guess it's the only option. By the way, I have two ina3221 hwmon patches that rebase upon this binding series. And I'd like to send them out to go through review first, but I am not sure if you'd be okay for it -- I don't really like to change their rebase order as it might mess up something. Thanks Nicolin
On 09/25/2018 08:08 PM, Nicolin Chen wrote: > Hello Guenter, > > On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote: > >>> +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 >>> + - status: Should be "disabled" if no input source >>> + >>> + Example: >>> + >>> + input@0 { >>> + reg = <0x0>; >>> + status = "disabled"; >> >> I kind of feel embarrassed that I asked for the reg change ... especially while >> saying at the same time that I would like to see this work for other chips >> as well. > > Well, though I didn't mention it, yet I changed it to "reg" is more > likely an agreement than a compromise: I searched in the mail list > and then found this mail (a year ago though): > https://www.spinics.net/lists/kernel/msg2455439.html > > I feel it very similar to my case. So rather than betting Rob won't > tell me the same, changing to "reg" may reduce a turnaround time :) > >> Other chips have different kinds of sensors. Voltage, current, power, temperature, >> and others. Whatever we come up with should support that. >> >> I see two possibilities right now. We can stick with reg and add a "type" property, >> or we can make the index something like >> {voltage,current,power,temperature,humidity}-{id,index} > > One small concern is a case of being multi-type. For example, I saw > ina2xx driver having voltage, current and power at the same time... > Yes, with that we would have something like voltage@0 { type = "voltage"; reg = <0>; }; current@0 { type = "current"; reg = <0>; }; ... or voltage@0 { voltage-id = <0>; }; current@0 { current-id = <0>; }; ... >> I personally prefer "type", but I don't really have a strong opinion. >> What do you think ? > > I also like this over "reg" -- "reg" requires two extra properties, > and itself sounds slightly unnatural to me for situations like this > one where we don't use it as a register address, although I know it > is convenient and common to use. > With "type", we would still need two properties. reg = <0>; type = "voltage"; and type could be optional or not required for a chip only supporting a single sensor type (like the ina3221). This would be equivalent to, say, voltage-index = <0>; when using a single property. With the "reg" approach, we would be ok for now - however, I would like to get feedback from Rob if introducing a "type" property will be acceptable when the time comes to do so. >> Or maybe we should really wait for feedback from Rob. > > Personally I don't mind it all to change the doc and code and then > send a v6. But eventually we'll still need the final Acked-by from > Rob right? Then I guess it's the only option. > Yes. At this point I'd rather have input from Rob than moving forward with v6. > By the way, I have two ina3221 hwmon patches that rebase upon this > binding series. And I'd like to send them out to go through review > first, but I am not sure if you'd be okay for it -- I don't really > like to change their rebase order as it might mess up something. > Are those bug fixes or further enhancements ? For enhancements, it is your call when to send them; I am fine either way. If they are bug fixes, they should come first so we can apply them to -stable. Thanks, Guenter
On Tue, Sep 25, 2018 at 08:40:59PM -0700, Guenter Roeck wrote: > >>>+2) child nodes > >>>+ Required properties: > >>>+ - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221 > >>>+ input@0 { > >>>+ reg = <0x0>; > >>>+ status = "disabled"; > >>saying at the same time that I would like to see this work for other chips > >>as well. > >>Other chips have different kinds of sensors. Voltage, current, power, temperature, > >>and others. Whatever we come up with should support that. > >> > >>I see two possibilities right now. We can stick with reg and add a "type" property, > >>or we can make the index something like > >> {voltage,current,power,temperature,humidity}-{id,index} > > > >One small concern is a case of being multi-type. For example, I saw > >ina2xx driver having voltage, current and power at the same time... > > > Yes, with that we would have something like > > voltage@0 { > type = "voltage"; > reg = <0>; > }; > current@0 { > type = "current"; > reg = <0>; > or > voltage@0 { > voltage-id = <0>; > }; > current@0 { > current-id = <0>; I see the point now. So this could be a good binding for different types of sensor input sources. Then the hwmon device driver side'd also get a hint from DT, or potentially have further common helper functions or structures being defined in the core driver. Yet I feel that we could still have something more obvious to tell which exact port that the input source links to. From my point of view, neither "type-id" nor "type + reg" nor "reg" only perfectly tells the port connection information. It somehow feels like that we are assigning an ID or even an address to an input source; Yes, we describe them in the doc, but by reading the device node itself without knowing the famous "reg", it's a bit hard to tell. And not to justify for my way, but both the "input-id" in the v2 and the "pwm-port" in the aspeed patch sound relatively straightforward. If we could make something similar to regulator bindings, probably it would make more sense. I know the standalone voltage node might be odd though, just trying to express my concern. source0: voltage0 { type = "voltage"; lable = "VDD_5V"; shunt-resistor-micro-ohm = <5000>; /* status = "disabled"; */ }; ina3221@40 { port1 = <&source0>; /* port2 = <&source1>; */ }; > With "type", we would still need two properties. > > reg = <0>; > type = "voltage"; > > and type could be optional or not required for a chip only supporting > a single sensor type (like the ina3221). > > This would be equivalent to, say, > voltage-index = <0>; > when using a single property. > > With the "reg" approach, we would be ok for now - however, I would like > to get feedback from Rob if introducing a "type" property will be > acceptable when the time comes to do so. OK. Let's see how Rob replies. If he strongly feels "reg" is still essential, at least this patch can be Acked first -- reduces turn around time :) > >By the way, I have two ina3221 hwmon patches that rebase upon this > >binding series. And I'd like to send them out to go through review > >first, but I am not sure if you'd be okay for it -- I don't really > >like to change their rebase order as it might mess up something. > > > Are those bug fixes or further enhancements ? For enhancements, > it is your call when to send them; I am fine either way. If they are > bug fixes, they should come first so we can apply them to -stable. Understood. They are adding new sysfs nodes, like in[123]_enable as you suggested during the previous review. I'll send them soon. Thanks Nicolin
On Tue, Sep 25, 2018 at 06:52:29PM -0700, Guenter Roeck wrote: > Hi Nicolin, > > On 09/25/2018 03:59 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 > > v4->v5: > > * Replaced "input-id" with "reg" and added address-cells and size-cells > > * Replaced "input-label" with "label" > > * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms" > > 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 | 49 +++++++++++++++++++ > > 1 file changed, 49 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..e17a897f4803 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt > > @@ -0,0 +1,49 @@ > > +Texas Instruments INA3221 Device Tree Bindings > > + > > +1) ina3221 node > > + Required properties: > > + - compatible: Must be "ti,ina3221" > > + - reg: I2C address > > + > > + Optional properties: > > + = 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. > > + > > + Example: > > + > > + ina3221@40 { > > + compatible = "ti,ina3221"; > > + reg = <0x40>; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + [ child node definitions... ] > > + }; > > + > > +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 > > + - status: Should be "disabled" if no input source > > + > > + Example: > > + > > + input@0 { > > + reg = <0x0>; > > + status = "disabled"; > > I kind of feel embarrassed that I asked for the reg change ... especially while > saying at the same time that I would like to see this work for other chips > as well. > > Other chips have different kinds of sensors. Voltage, current, power, temperature, > and others. Whatever we come up with should support that. > > I see two possibilities right now. We can stick with reg and add a "type" property, > or we can make the index something like > {voltage,current,power,temperature,humidity}-{id,index} > > I personally prefer "type", but I don't really have a strong opinion. > What do you think ? Or maybe we should really wait for feedback from Rob. reg and 'type' is my preference though just 'type' may be too vague. For this case I don't think we need it if there's only one possible type as the driver should know that. Rob
On Tue, Sep 25, 2018 at 03:59:29PM -0700, 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 > v4->v5: > * Replaced "input-id" with "reg" and added address-cells and size-cells > * Replaced "input-label" with "label" > * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms" > 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 | 49 +++++++++++++++++++ > 1 file changed, 49 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..e17a897f4803 > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt > @@ -0,0 +1,49 @@ > +Texas Instruments INA3221 Device Tree Bindings > + > +1) ina3221 node > + Required properties: > + - compatible: Must be "ti,ina3221" > + - reg: I2C address > + > + Optional properties: > + = 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. > + > + Example: > + > + ina3221@40 { > + compatible = "ti,ina3221"; > + reg = <0x40>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + [ child node definitions... ] > + }; > + > +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 > + - status: Should be "disabled" if no input source Don't need to explicitly list status. > + > + Example: > + > + 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>; > + }; Please combine the examples into one complete example. Rob
On Thu, Sep 27, 2018 at 12:44:13PM -0500, Rob Herring wrote: > > +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 > > + - status: Should be "disabled" if no input source > > Don't need to explicitly list status. Will remove in v6. > > + > > + Example: > > + > > + 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>; > > + }; > > Please combine the examples into one complete example. Will merge them in v6. Thank you Nicolin
diff --git a/Documentation/devicetree/bindings/hwmon/ina3221.txt b/Documentation/devicetree/bindings/hwmon/ina3221.txt new file mode 100644 index 000000000000..e17a897f4803 --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/ina3221.txt @@ -0,0 +1,49 @@ +Texas Instruments INA3221 Device Tree Bindings + +1) ina3221 node + Required properties: + - compatible: Must be "ti,ina3221" + - reg: I2C address + + Optional properties: + = 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. + + Example: + + ina3221@40 { + compatible = "ti,ina3221"; + reg = <0x40>; + #address-cells = <1>; + #size-cells = <0>; + + [ child node definitions... ] + }; + +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 + - status: Should be "disabled" if no input source + + Example: + + 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>; + };
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 v4->v5: * Replaced "input-id" with "reg" and added address-cells and size-cells * Replaced "input-label" with "label" * Replaced "shunt-resistor" with "shunt-resistor-micro-ohms" 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 | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/ina3221.txt