Re: [PATCH v4 2/2] iio: Add OF support

On Wednesday 06 of February 2013 12:05:13 Guenter Roeck wrote:

On Wed, Feb 06, 2013 at 07:37:37PM +0000, Jonathan Cameron wrote:

quoted

On 02/06/2013 06:29 PM, Guenter Roeck wrote:

quoted

Provide bindings and parse OF data during initialization.

Signed-off-by: Guenter Roeck <redacted>

looks good to me.  Couple of little queries inline.

quoted

---
v4:
- Fixed wrong parameter to dummy of_iio_channel_get_by_name if
CONFIG_OF is> > 
  undefined, and wrong return value.

- Initialize indio_dev->of_node in iio_device_register if the
calling driver> > 
  neglected to do it.

v3:
- Cleaned up documentation (formatting, left-over clock references)
- Updated bindings description to permit sub-devices
- When searching for iio devices, use the pointer to the iio device
type instead> > 
  of strcmp. Rename iio_dev_type to iio_device_type (to match other
  device types) and make it global for that purpose. Check the OF
  node first, then the device type, as the node is less likely to
  match.

- Move the common code in of_iio_channel_get and
of_iio_channel_get_all to> > 
  __of_iio_channel_get.

- Return NULL from of_iio_channel_get_by_name if nothing is found,
or

  an error if there is a problem with consistency or if the provider
  device is not yet available.

- In iio_channel_get, return if of_iio_channel_get_by_name() returns
a channel> > 
  or an error, and continue otherwise.

v2:
- Rebased to iio/togreg
- Documentation update per feedback
- Dropped io-channel-output-names from the bindings document. The
property is> > 
  not used in the code, and it is not entirely clear what it would
  be used for. If there is a need for it, we can add it back in
  later on.

- Don't export OF specific API calls
- For OF support, no longer depend on iio_map
- Add #ifdef CONFIG_OF where appropriate, and ensure that the code
still builds> > 
  if it is not selected.

- Change iio_channel_get to take device pointer as argument instead
of device> > 
  name. Retain old API as of_iio_channel_get_sys.

- iio_channel_get now works for both OF and non-OF configurations
- Use regulator to get vref for max1363 driver.

 drivers/iio/iio_core.h                             |    1 +
 drivers/iio/industrialio-core.c                    |    8 +-
 drivers/iio/inkern.c                               |  171
 ++++++++++++++++++++ 4 files changed, 268 insertions(+), 2
 deletions(-)
 create mode 100644
 Documentation/devicetree/bindings/iio/iio-bindings.txt> > 

diff --git a/Documentation/devicetree/bindings/iio/iio-bindings.txt

b/Documentation/devicetree/bindings/iio/iio-bindings.txt new file
mode 100644
index 0000000..2475c2e

--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt

@@ -0,0 +1,90 @@
+This binding is a work-in-progress. It is derived from clock

bindings,
+and based on suggestions from Lars-Peter Clausen [1].
+
+Sources of IIO channels can be represented by any node in the
device
+tree. Those nodes are designated as IIO providers. IIO consumer
+nodes use a phandle and IIO specifier pair to connect IIO provider
+outputs to IIO inputs. Similar to the gpio specifiers, an IIO
+specifier is an array of one or more cells identifying the IIO
+output on a device. The length of an IIO specifier is defined by
the
+value of a #io-channel-cells property in the IIO provider node.
+
+[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
+
+==IIO providers==
+
+Required properties:
+#io-channel-cells: Number of cells in an IIO specifier; Typically 0
for nodes +		   with a single IIO output and 1 for nodes with
multiple
+		   IIO outputs.
+
+Example for a simple configuration with no trigger:
+
+	adc: voltage-sensor@35 {
+		compatible = "maxim,max1139";
+		reg = <0x35>;
+		#io-channel-cells = <1>;
+	};
+
+Example for a configuration with trigger:
+
+	adc@35 {
+		compatible = "maxim,max1139";
+		reg = <0x35>;
+
+		adc: iio-device {
+			#io-channel-cells = <1>;
+		};
+		trigger: iio-trigger {
+			#io-channel-cells = <1>;

Why would the trigger have channel-cells?
So far we don't have any device tree stuff for the triggers.

This comes from one of the examples passed around earlier.

Presumably, when triggers are supported through devicetree, you would
have more than one per device, meaning you need the same logic as for
devices.

Though on the other side that isn't supported yet - I don't have
hardware to test, so I can not implement - or, rather, test -
devicetree based trigger support. I can take the entire thing out if
you prefer - I don't really have a strong opinion.

quoted

quoted

+		};
+	};
+
+==IIO consumers==
+
+Required properties:
+io-channels:	List of phandle and IIO specifier pairs, one pair
+		for each IIO input to the device. Note: if the
+		IIO provider specifies '0' for #io-channel-cells,
+		then only the phandle portion of the pair will appear.
+
+Optional properties:
+io-channel-names:
+		List of IIO input name strings sorted in the same
+		order as the io-channels property. Consumers drivers
+		will use io-channel-names to match IIO input names
+		with IIO specifiers.
+io-channel-ranges:
+		Empty property indicating that child nodes can inherit 

named

quoted

quoted

+		IIO channels from this node. Useful for bus nodes to 

provide

quoted

quoted

+		and IIO channel to their children.
+
+For example:
+
+	device {
+		io-channels = <&adc 1>, <&ref 0>;
+		io-channel-names = "vcc", "vdd";
+	};
+
+This represents a device with two IIO inputs, named "vcc" and
"vdd".
+The vcc channel is connected to output 1 of the &adc device, and
the
+vdd channel is connected to output 0 of the &ref device.
+
+==Example==
+
+	adc: max1139@35 {
+		compatible = "maxim,max1139";
+		reg = <0x35>;
+		#io-channel-cells = <1>;
+	};
+
+	...
+
+	iio_hwmon {
+		compatible = "iio-hwmon";
+		io-channels = <&adc 0>, <&adc 1>, <&adc 2>,
+			<&adc 3>, <&adc 4>, <&adc 5>,
+			<&adc 6>, <&adc 7>, <&adc 8>,
+			<&adc 9>, <&adc 10>, <&adc 11>;
+		io-channel-names = "vcc", "vdd", "vref", "1.2V";

This example still seems a little odd. Why give one where there are
more channels than names?

I just didn't have a better idea. Do you know of another iio consumer I
could use as example, one which doesn't use the get_all API ?

quoted

quoted

+	};

diff --git a/drivers/iio/iio_core.h b/drivers/iio/iio_core.h
index f652e6a..05c1b74 100644
--- a/drivers/iio/iio_core.h
+++ b/drivers/iio/iio_core.h

@@ -18,6 +18,7 @@

 struct iio_chan_spec;
 struct iio_dev;

+extern struct device_type iio_device_type;

 int __iio_add_chan_devattr(const char *postfix,
 
 			   struct iio_chan_spec const *chan,

diff --git a/drivers/iio/industrialio-core.c

b/drivers/iio/industrialio-core.c index 8848f16..6d8b027 100644

--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c

@@ -847,7 +847,7 @@ static void iio_dev_release(struct device

*device)

 	kfree(indio_dev);
 
 }

-static struct device_type iio_dev_type = {
+struct device_type iio_device_type = {

 	.name = "iio_device",
 	.release = iio_dev_release,
 
 };

@@ -869,7 +869,7 @@ struct iio_dev *iio_device_alloc(int

sizeof_priv)

 	if (dev) {
 	
 		dev->dev.groups = dev->groups;

-		dev->dev.type = &iio_dev_type;
+		dev->dev.type = &iio_device_type;

 		dev->dev.bus = &iio_bus_type;
 		device_initialize(&dev->dev);
 		dev_set_drvdata(&dev->dev, (void *)dev);

@@ -960,6 +960,10 @@ int iio_device_register(struct iio_dev

*indio_dev)

 {
 
 	int ret;

+	/* If the calling driver did not initialize of_node, do it here 

*/

quoted

quoted

+	if (!indio_dev->dev.of_node && indio_dev->dev.parent)
+		indio_dev->dev.of_node = indio_dev->dev.parent->of_node;
+

I guess that's sensible, though I'd be inclined to check it and throw
a wobbly if it isn't defined.  That way we have one 'right' place to
do it.

I had to look that one up :). "throw a wobbly" = "reached the end of
rational thought and action" or "to become suddenly very agitated or
angry". Is that about right ?

I assume you mean the case where indio_dev->dev.of_node is NULL ?
Problem is that this can hapen anytime (and always if CONFIG_OF is
undefined). I'd be more concerned if indio_dev->dev.parent is NULL,
which happens with a couple of drivers if I interpret the code
correctly. Otherwise it only matters if the node should really point to
a sub-node and not to the parent's node itself - but presumably
programmers should detect that while writing the driver.

Alternative would be to set of_node in all calling drivers.
Unfortunately, there are about one hundred callers, so that would be a
substantial effort.

Not sure myself what the right approach is here - that is why I asked
for feedback about it in v3. I am open to suggestions.

I'm not sure if I understand the point, so following might be just some 
random thoughts:

There is no need to check for of_node. Case where there is no of_node 
attached to the device is fine, as lots of systems are being booted 
without device tree support.

Getting of_node from parent device seems reasonable to me, as this will 
automatically handle all the simple cases where iio_device node would be 
equal to physical device node.

Best regards,
Tomasz