Re: [PATCH v5 1/2] dt-bindings: hwmon: Add ina3221 documentation

[Nicolin Chen]

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