On Tue, Sep 25, 2018 at 08:40:59PM -0700, Guenter Roeck wrote:

quoted

quoted

quoted

+2) child nodes
+  Required properties:
+  - reg: Must be 0, 1 or 2, corresponding to IN1, IN2 or IN3 port of INA3221

quoted

quoted

quoted

+  input@0 {
+          reg = <0x0>;
+          status = "disabled";

quoted

quoted

saying at the same time that I would like to see this work for other chips
as well.

quoted

quoted

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 :)

quoted

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