[PATCH] hwmon: (ads1015) Add devicetree documentation

[Grant Likely]

On Thu, Mar 03, 2011 at 02:25:49PM +0100, Wolfram Sang wrote:
> > > Hmm, device tree bindings should be OS-neutral, sysfs is not.
> > 
> > Why do we document this in the Linux kernel tree then?
> 
> To describe which bindings Linux supports.
> 
> > > with the active channels. Then again, what is the drawback of exporting
> > > all channels?
> > 
> > Performance and user-friendliness. libsensors-based applications will
> > read all available attributes by default, and each reading takes time.
> > Letting the platform declare how the inputs are used allows for a sane
> > output for "sensors" and other similar tools out of the box, without
> > the user having to tinker with ignore statements in configuration files
> > to discard the nonsensical values.
> 
> OK, that's fine I'd say.
> 
> > > Is there another hwmon-driver doing so (couldn't find one)?
> > 
> > If "doing so" means "letting the user define how the ADC inputs are
> > used", then yes, the pcf8591 driver does something similar, except that
> > it uses a module parameter for the setting, for historical reasons.
> > Platform-provided, per-device data is better in my opinion.
> 
> OK. The thing is you can't map platform_data 1:1 to bindings, because
> most are very specific to the Linux-driver. Do you think something like
> "active-channels" would be sufficent for those other hwmon devices, too?
> (I still do not like "exported-channels", because there is no need to
> export the channels for the OS. The devicetree is primarily a hardware
> description language) Or maybe we go specific and say "ads1015,channel1
> = 1"? Maybe somebody knows of a similar chips as a reference?

Yes, Wolfram's correct.  The focus should be on what the connections
actually are instead of what the OS should attempt to do with them.
ie. give the active channels actual names for what they do, or use a
phandle to reference them from another node.  The driver can then make
a decision based on whether or not a channel has a configuration
provided.

>From the little information I have, I'd recommend something like:

sensor at 49 {
	compatible = "ti,ads1015"
	reg = <0x49>;

	// Each child node (one node per channel) has an address with
	// no range
	#address-cells = <1>;
	#size-cells = <0>;

	adc at 2 {
		ti,measurement = "cpu voltage";
		reg = <2>;
	};

	adc at 4 {
		ti,measurement = "base voltage";
		reg = <4>;
	};
};

However, after taking a little look at the data sheet, this binding
might end up being a little naive for what the part can do.  It might
make more sense to do something like:

sensor at 49 {
	compatible = "ti,ads1015"
	reg = <0x49>;

	// Each child node (one node per channel) has an address with
	// no range
	#address-cells = <1>;
	#size-cells = <0>;

	measurement at 0 {
		reg = <0>;	// This reg value no longer reflects
				// the chip, it's just a handle for
				// logical measurement channels
		ti,measurement = "cpu voltage";
		ti,multiplexer = <2>; // AIN0 vs. AIN3
		ti,gain = <5>; // +/-0.256V
		ti,inverse-polarity;
	};

	measurement at 1 {
		reg = <1>;
		ti,measurement = "base voltage";
		ti,multiplexer = <4>; // AIN0 vs. GND
		ti,gain = <2>; // +/-2.048V
	};
};

...which splits it up by logical measurement configurations instead of
only the multiplexer setting.

It's more verbose than a single exported-channels property, but it is
flexible enough that it can be easily extended with extra
configuration data per channel.

g.