[RFC 00/11] staging/iio: Devicetree support

[RFC 00/11] staging/iio: Devicetree support

[Guenter Roeck]

This patch set adds basic device tree support to the IIO subsystem. It is the
result of the [1] and [2] discussions. The patch set is based on v3.8-rc5.

Patches 1 to 7 are either bug fixes or cleanup patches and can at least in
theory be applied immediately, per the subsystem maintainer's choice.

Patch 8 and 9 modify the parameters to iio_channel_get_all_cb(),
iio_channel_get_all(), and iio_map_array_unregister() to prepare for the
subsequent patches. If and how those changes are acceptable is TBD.
Obviously the changes make sense to me, but if anyone has a better solution
please let me know.

Patch 10 adds basic OF support to the IIO subsystem. Support is modeled after
the clock subsystem. Obviously this will need some cleanup, and we'll have to
decide if the approach is acceptable and which OF specific functions will be
required. But it is at least a starting point for the discussion.

Patch 11 adds basic device-tree configuration to the MAX1363 driver;
I needed that for my target system which uses an external reference voltage.

For reference, here is a real dts file snippet:

	max1139: voltage-sensor@35 {
        	compatible = "maxim,max1139";
		reg = <0x35>;
		vref = <3300>;
		#io-channel-cells = <1>;
	};
	...
	iio_hwmon {
		compatible = "iio-hwmon";
		io-channels = <&max1139 0>, <&max1139 1>, <&max1139 2>,
			<&max1139 3>, <&max1139 4>, <&max1139 5>,
			<&max1139 6>, <&max1139 7>, <&max1139 8>,
			<&max1139 9>, <&max1139 10>, <&max1139 11>;
	};

This is the resulting output from the "sensors" command:

iio_hwmon-isa-000a
Adapter: ISA adapter
in1:          +0.00 V
in2:          +3.12 V
in3:          +1.64 V
in4:          +2.50 V
in5:          +3.29 V
in6:          +1.51 V
in7:          +3.30 V
in8:          +3.30 V
in9:          +1.05 V
in10:         +3.30 V
in11:         +1.65 V
in12:         +1.65 V

Probe deferral works quite nicely:

[   17.267086] platform iio_hwmon.10: Driver iio_hwmon requests probe deferral

Guenter

---
[1] http://marc.info/?l=linux-iio&m=135902119507483&w=2
[2] http://marc.info/?l=lm-sensors&m=135375101529918&w=1

[PATCH 01/11] staging/iio: (iio_hwmon) Use devm_kzalloc

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/staging/iio/iio_hwmon.c |   70 ++++++++++++---------------------------
 1 file changed, 22 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
index c7a5f97..5456272 100644
--- a/drivers/staging/iio/iio_hwmon.c
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -55,63 +55,47 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 	return sprintf(buf, "%d\n", result);
 }
 
-static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
-{
-	int i;
-	struct sensor_device_attribute *a;
-	for (i = 0; i < st->num_channels; i++)
-		if (st->attrs[i]) {
-			a = to_sensor_dev_attr(
-				container_of(st->attrs[i],
-					     struct device_attribute,
-					     attr));
-			kfree(a);
-		}
-}
-
 static int iio_hwmon_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct iio_hwmon_state *st;
 	struct sensor_device_attribute *a;
 	int ret, i;
 	int in_i = 1, temp_i = 1, curr_i = 1;
 	enum iio_chan_type type;
 
-	st = kzalloc(sizeof(*st), GFP_KERNEL);
-	if (st == NULL) {
-		ret = -ENOMEM;
-		goto error_ret;
-	}
+	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
+	if (st == NULL)
+		return -ENOMEM;
 
-	st->channels = iio_channel_get_all(dev_name(&pdev->dev));
-	if (IS_ERR(st->channels)) {
-		ret = PTR_ERR(st->channels);
-		goto error_free_state;
-	}
+	st->channels = iio_channel_get_all(dev_name(dev));
+	if (IS_ERR(st->channels))
+		return PTR_ERR(st->channels);
 
 	/* count how many attributes we have */
 	while (st->channels[st->num_channels].indio_dev)
 		st->num_channels++;
 
-	st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
-			    GFP_KERNEL);
+	st->attrs = devm_kzalloc(dev,
+				 sizeof(st->attrs) * (st->num_channels + 1),
+				 GFP_KERNEL);
 	if (st->attrs == NULL) {
 		ret = -ENOMEM;
 		goto error_release_channels;
 	}
+
 	for (i = 0; i < st->num_channels; i++) {
-		a = kzalloc(sizeof(*a), GFP_KERNEL);
+		a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
 		if (a == NULL) {
 			ret = -ENOMEM;
-			goto error_free_attrs;
+			goto error_release_channels;
 		}
 
 		sysfs_attr_init(&a->dev_attr.attr);
 		ret = iio_get_channel_type(&st->channels[i], &type);
-		if (ret < 0) {
-			kfree(a);
-			goto error_free_attrs;
-		}
+		if (ret < 0)
+			goto error_release_channels;
+
 		switch (type) {
 		case IIO_VOLTAGE:
 			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
@@ -130,13 +114,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 			break;
 		default:
 			ret = -EINVAL;
-			kfree(a);
-			goto error_free_attrs;
+			goto error_release_channels;
 		}
 		if (a->dev_attr.attr.name == NULL) {
-			kfree(a);
 			ret = -ENOMEM;
-			goto error_free_attrs;
+			goto error_release_channels;
 		}
 		a->dev_attr.show = iio_hwmon_read_val;
 		a->dev_attr.attr.mode = S_IRUGO;
@@ -146,11 +128,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 
 	st->attr_group.attrs = st->attrs;
 	platform_set_drvdata(pdev, st);
-	ret = sysfs_create_group(&pdev->dev.kobj, &st->attr_group);
+	ret = sysfs_create_group(&dev->kobj, &st->attr_group);
 	if (ret < 0)
-		goto error_free_attrs;
+		goto error_release_channels;
 
-	st->hwmon_dev = hwmon_device_register(&pdev->dev);
+	st->hwmon_dev = hwmon_device_register(dev);
 	if (IS_ERR(st->hwmon_dev)) {
 		ret = PTR_ERR(st->hwmon_dev);
 		goto error_remove_group;
@@ -158,15 +140,9 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 	return 0;
 
 error_remove_group:
-	sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
-error_free_attrs:
-	iio_hwmon_free_attrs(st);
-	kfree(st->attrs);
+	sysfs_remove_group(&dev->kobj, &st->attr_group);
 error_release_channels:
 	iio_channel_release_all(st->channels);
-error_free_state:
-	kfree(st);
-error_ret:
 	return ret;
 }
 
@@ -176,8 +152,6 @@ static int iio_hwmon_remove(struct platform_device *pdev)
 
 	hwmon_device_unregister(st->hwmon_dev);
 	sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
-	iio_hwmon_free_attrs(st);
-	kfree(st->attrs);
 	iio_channel_release_all(st->channels);
 
 	return 0;
-- 
1.7.9.7

Re: [PATCH 01/11] staging/iio: (iio_hwmon) Use devm_kzalloc

[Jonathan Cameron]

On 01/31/2013 09:42 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
This crossed with a little fix patch that changes one of the kzallocs
below.  The same trivial fix applies here so I've applied this with the obvious change.

Thanks,
> ---
>  drivers/staging/iio/iio_hwmon.c |   70 ++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
> index c7a5f97..5456272 100644
> --- a/drivers/staging/iio/iio_hwmon.c
> +++ b/drivers/staging/iio/iio_hwmon.c
> @@ -55,63 +55,47 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>  	return sprintf(buf, "%d\n", result);
>  }
>  
> -static void iio_hwmon_free_attrs(struct iio_hwmon_state *st)
> -{
> -	int i;
> -	struct sensor_device_attribute *a;
> -	for (i = 0; i < st->num_channels; i++)
> -		if (st->attrs[i]) {
> -			a = to_sensor_dev_attr(
> -				container_of(st->attrs[i],
> -					     struct device_attribute,
> -					     attr));
> -			kfree(a);
> -		}
> -}
> -
>  static int iio_hwmon_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct iio_hwmon_state *st;
>  	struct sensor_device_attribute *a;
>  	int ret, i;
>  	int in_i = 1, temp_i = 1, curr_i = 1;
>  	enum iio_chan_type type;
>  
> -	st = kzalloc(sizeof(*st), GFP_KERNEL);
> -	if (st == NULL) {
> -		ret = -ENOMEM;
> -		goto error_ret;
> -	}
> +	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
> +	if (st == NULL)
> +		return -ENOMEM;
>  
> -	st->channels = iio_channel_get_all(dev_name(&pdev->dev));
> -	if (IS_ERR(st->channels)) {
> -		ret = PTR_ERR(st->channels);
> -		goto error_free_state;
> -	}
> +	st->channels = iio_channel_get_all(dev_name(dev));
> +	if (IS_ERR(st->channels))
> +		return PTR_ERR(st->channels);
>  
>  	/* count how many attributes we have */
>  	while (st->channels[st->num_channels].indio_dev)
>  		st->num_channels++;
>  
> -	st->attrs = kzalloc(sizeof(st->attrs) * (st->num_channels + 1),
this has changed to kzalloc(sizeof(*st->attrs) * (st->num_channels + 1),
so the following changed to equivalent.

> -			    GFP_KERNEL);
> +	st->attrs = devm_kzalloc(dev,
> +				 sizeof(st->attrs) * (st->num_channels + 1),
> +				 GFP_KERNEL);
>  	if (st->attrs == NULL) {
>  		ret = -ENOMEM;
>  		goto error_release_channels;
>  	}
> +
>  	for (i = 0; i < st->num_channels; i++) {
> -		a = kzalloc(sizeof(*a), GFP_KERNEL);
> +		a = devm_kzalloc(dev, sizeof(*a), GFP_KERNEL);
>  		if (a == NULL) {
>  			ret = -ENOMEM;
> -			goto error_free_attrs;
> +			goto error_release_channels;
>  		}
>  
>  		sysfs_attr_init(&a->dev_attr.attr);
>  		ret = iio_get_channel_type(&st->channels[i], &type);
> -		if (ret < 0) {
> -			kfree(a);
> -			goto error_free_attrs;
> -		}
> +		if (ret < 0)
> +			goto error_release_channels;
> +
>  		switch (type) {
>  		case IIO_VOLTAGE:
>  			a->dev_attr.attr.name = kasprintf(GFP_KERNEL,
> @@ -130,13 +114,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  			break;
>  		default:
>  			ret = -EINVAL;
> -			kfree(a);
> -			goto error_free_attrs;
> +			goto error_release_channels;
>  		}
>  		if (a->dev_attr.attr.name == NULL) {
> -			kfree(a);
>  			ret = -ENOMEM;
> -			goto error_free_attrs;
> +			goto error_release_channels;
>  		}
>  		a->dev_attr.show = iio_hwmon_read_val;
>  		a->dev_attr.attr.mode = S_IRUGO;
> @@ -146,11 +128,11 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  
>  	st->attr_group.attrs = st->attrs;
>  	platform_set_drvdata(pdev, st);
> -	ret = sysfs_create_group(&pdev->dev.kobj, &st->attr_group);
> +	ret = sysfs_create_group(&dev->kobj, &st->attr_group);
>  	if (ret < 0)
> -		goto error_free_attrs;
> +		goto error_release_channels;
>  
> -	st->hwmon_dev = hwmon_device_register(&pdev->dev);
> +	st->hwmon_dev = hwmon_device_register(dev);
>  	if (IS_ERR(st->hwmon_dev)) {
>  		ret = PTR_ERR(st->hwmon_dev);
>  		goto error_remove_group;
> @@ -158,15 +140,9 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  	return 0;
>  
>  error_remove_group:
> -	sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
> -error_free_attrs:
> -	iio_hwmon_free_attrs(st);
> -	kfree(st->attrs);
> +	sysfs_remove_group(&dev->kobj, &st->attr_group);
>  error_release_channels:
>  	iio_channel_release_all(st->channels);
> -error_free_state:
> -	kfree(st);
> -error_ret:
>  	return ret;
>  }
>  
> @@ -176,8 +152,6 @@ static int iio_hwmon_remove(struct platform_device *pdev)
>  
>  	hwmon_device_unregister(st->hwmon_dev);
>  	sysfs_remove_group(&pdev->dev.kobj, &st->attr_group);
> -	iio_hwmon_free_attrs(st);
> -	kfree(st->attrs);
>  	iio_channel_release_all(st->channels);
>  
>  	return 0;
> 

[PATCH 02/11] staging/iio: (iio_hwmon) Add support for sysfs name attribute

[Guenter Roeck]

The 'name' attribute is mandatory for hwmon devices.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/staging/iio/iio_hwmon.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
index 5456272..7f260ea 100644
--- a/drivers/staging/iio/iio_hwmon.c
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -55,6 +55,14 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
 	return sprintf(buf, "%d\n", result);
 }
 
+static ssize_t show_name(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	return sprintf(buf, "iio_hwmon\n");
+}
+
+static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
+
 static int iio_hwmon_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -77,7 +85,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		st->num_channels++;
 
 	st->attrs = devm_kzalloc(dev,
-				 sizeof(st->attrs) * (st->num_channels + 1),
+				 sizeof(st->attrs) * (st->num_channels + 2),
 				 GFP_KERNEL);
 	if (st->attrs == NULL) {
 		ret = -ENOMEM;
@@ -125,7 +133,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 		a->index = i;
 		st->attrs[i] = &a->dev_attr.attr;
 	}
-
+	st->attrs[st->num_channels] = &dev_attr_name.attr;
 	st->attr_group.attrs = st->attrs;
 	platform_set_drvdata(pdev, st);
 	ret = sysfs_create_group(&dev->kobj, &st->attr_group);
-- 
1.7.9.7

Re: [PATCH 02/11] staging/iio: (iio_hwmon) Add support for sysfs name attribute

[Jonathan Cameron]

On 01/31/2013 09:42 PM, Guenter Roeck wrote:
> The 'name' attribute is mandatory for hwmon devices.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Wow, that one character patch these crossed with manages to effect
this one as well.  Applied with the obvious tiny fix.

Thanks,
> ---
>  drivers/staging/iio/iio_hwmon.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
> index 5456272..7f260ea 100644
> --- a/drivers/staging/iio/iio_hwmon.c
> +++ b/drivers/staging/iio/iio_hwmon.c
> @@ -55,6 +55,14 @@ static ssize_t iio_hwmon_read_val(struct device *dev,
>  	return sprintf(buf, "%d\n", result);
>  }
>  
> +static ssize_t show_name(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	return sprintf(buf, "iio_hwmon\n");
> +}
> +
> +static DEVICE_ATTR(name, S_IRUGO, show_name, NULL);
> +
>  static int iio_hwmon_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -77,7 +85,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		st->num_channels++;
>  
>  	st->attrs = devm_kzalloc(dev,
> -				 sizeof(st->attrs) * (st->num_channels + 1),
> +				 sizeof(st->attrs) * (st->num_channels + 2),
>  				 GFP_KERNEL);
>  	if (st->attrs == NULL) {
>  		ret = -ENOMEM;
> @@ -125,7 +133,7 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  		a->index = i;
>  		st->attrs[i] = &a->dev_attr.attr;
>  	}
> -
> +	st->attrs[st->num_channels] = &dev_attr_name.attr;
>  	st->attr_group.attrs = st->attrs;
>  	platform_set_drvdata(pdev, st);
>  	ret = sysfs_create_group(&dev->kobj, &st->attr_group);
> 

[PATCH 03/11] staging/iio: (iio_hwmon) Basic devicetree support

[Guenter Roeck]

Add 'iio-hwmon' OF compatibility table entry to enable OF matches.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/staging/iio/iio_hwmon.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
index 7f260ea..6a5bc5f 100644
--- a/drivers/staging/iio/iio_hwmon.c
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -165,10 +165,16 @@ static int iio_hwmon_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static struct of_device_id iio_hwmon_of_match[] = {
+	{ .compatible = "iio-hwmon", },
+	{ }
+};
+
 static struct platform_driver __refdata iio_hwmon_driver = {
 	.driver = {
 		.name = "iio_hwmon",
 		.owner = THIS_MODULE,
+		.of_match_table = iio_hwmon_of_match,
 	},
 	.probe = iio_hwmon_probe,
 	.remove = iio_hwmon_remove,
-- 
1.7.9.7

Re: [PATCH 03/11] staging/iio: (iio_hwmon) Basic devicetree support

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Add 'iio-hwmon' OF compatibility table entry to enable OF matches.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied to togreg branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git

Thanks,
> ---
>  drivers/staging/iio/iio_hwmon.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
> index 7f260ea..6a5bc5f 100644
> --- a/drivers/staging/iio/iio_hwmon.c
> +++ b/drivers/staging/iio/iio_hwmon.c
> @@ -165,10 +165,16 @@ static int iio_hwmon_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static struct of_device_id iio_hwmon_of_match[] = {
> +	{ .compatible = "iio-hwmon", },
> +	{ }
> +};
> +
>  static struct platform_driver __refdata iio_hwmon_driver = {
>  	.driver = {
>  		.name = "iio_hwmon",
>  		.owner = THIS_MODULE,
> +		.of_match_table = iio_hwmon_of_match,
>  	},
>  	.probe = iio_hwmon_probe,
>  	.remove = iio_hwmon_remove,
> 

[PATCH 04/11] iio/adc: (lp8788) Provide OF node information to iio device

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/adc/lp8788_adc.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
index 72955e4..e5293d0 100644
--- a/drivers/iio/adc/lp8788_adc.c
+++ b/drivers/iio/adc/lp8788_adc.c
@@ -208,6 +208,7 @@ static int lp8788_adc_probe(struct platform_device *pdev)
 	adc->lp = lp;
 	platform_set_drvdata(pdev, indio_dev);
 
+	indio_dev->dev.of_node = pdev->dev.of_node;
 	ret = lp8788_iio_map_register(indio_dev, lp->pdata, adc);
 	if (ret)
 		goto err_iio_map;
-- 
1.7.9.7

Re: [PATCH 04/11] iio/adc: (lp8788) Provide OF node information to iio device

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
applied to togreg branch of
git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git

> ---
>  drivers/iio/adc/lp8788_adc.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
> index 72955e4..e5293d0 100644
> --- a/drivers/iio/adc/lp8788_adc.c
> +++ b/drivers/iio/adc/lp8788_adc.c
> @@ -208,6 +208,7 @@ static int lp8788_adc_probe(struct platform_device *pdev)
>  	adc->lp = lp;
>  	platform_set_drvdata(pdev, indio_dev);
>  
> +	indio_dev->dev.of_node = pdev->dev.of_node;
>  	ret = lp8788_iio_map_register(indio_dev, lp->pdata, adc);
>  	if (ret)
>  		goto err_iio_map;
> 

[PATCH 05/11] iio/adc: (max1363) Provide OF node information to iio device

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/adc/max1363.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 03b25b3..ca8bbc1 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -1538,6 +1538,7 @@ static int max1363_probe(struct i2c_client *client,
 		goto error_out;
 	}
 
+	indio_dev->dev.of_node = client->dev.of_node;
 	ret = iio_map_array_register(indio_dev, client->dev.platform_data);
 	if (ret < 0)
 		goto error_free_device;
-- 
1.7.9.7

Re: [PATCH 05/11] iio/adc: (max1363) Provide OF node information to iio device

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>

Applied to togreg branch of iio.git

A previous merge seems to have gone wrong in this driver and some
removed functions have reappeared. I'll post a patch for that in a few mins.

> ---
>  drivers/iio/adc/max1363.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 03b25b3..ca8bbc1 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -1538,6 +1538,7 @@ static int max1363_probe(struct i2c_client *client,
>  		goto error_out;
>  	}
>  
> +	indio_dev->dev.of_node = client->dev.of_node;
>  	ret = iio_map_array_register(indio_dev, client->dev.platform_data);
>  	if (ret < 0)
>  		goto error_free_device;
> 

[PATCH 06/11] iio/adc: (max1363) Remove duplicate code

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/adc/max1363.c |    2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index ca8bbc1..2c773b4 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -1572,8 +1572,6 @@ static int max1363_probe(struct i2c_client *client,
 	indio_dev->num_channels = st->chip_info->num_channels;
 	indio_dev->info = st->chip_info->info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
-	indio_dev->channels = st->chip_info->channels;
-	indio_dev->num_channels = st->chip_info->num_channels;
 	ret = max1363_initial_setup(st);
 	if (ret < 0)
 		goto error_free_available_scan_masks;
-- 
1.7.9.7

Re: [PATCH 06/11] iio/adc: (max1363) Remove duplicate code

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied to togreg branch of iio.git
> ---
>  drivers/iio/adc/max1363.c |    2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index ca8bbc1..2c773b4 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -1572,8 +1572,6 @@ static int max1363_probe(struct i2c_client *client,
>  	indio_dev->num_channels = st->chip_info->num_channels;
>  	indio_dev->info = st->chip_info->info;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> -	indio_dev->channels = st->chip_info->channels;
> -	indio_dev->num_channels = st->chip_info->num_channels;
>  	ret = max1363_initial_setup(st);
>  	if (ret < 0)
>  		goto error_free_available_scan_masks;
> 

[PATCH 07/11] iio/adc: (max1363) Fix data conversion problems

[Guenter Roeck]

For chips with more than 8 bit ADC resolution, received data was always
masked against 0xfff, ie with a 12 bit mask. This can result in bad data
for chips with 10 bit resolution if those chips have higher bits set
(seen with MAX1139).

The receive buffer was defined as char array. This could result in
unintentional sign extensions if the upper bit in a received byte
was set. Since the chip is configured for unipolar mode, we never
have to handle negative values, and sign extensions are never needed.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/adc/max1363.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 2c773b4..fdc8be9 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -334,7 +334,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 {
 	int ret = 0;
 	s32 data;
-	char rxbuf[2];
+	u8 rxbuf[2];
 	struct max1363_state *st = iio_priv(indio_dev);
 	struct i2c_client *client = st->client;
 
@@ -366,7 +366,8 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
 			ret = data;
 			goto error_ret;
 		}
-		data = (s32)(rxbuf[1]) | ((s32)(rxbuf[0] & 0x0F)) << 8;
+		data = (rxbuf[1] | rxbuf[0] << 8) &
+		  ((1 << st->chip_info->bits) - 1);
 	} else {
 		/* Get reading */
 		data = i2c_master_recv(client, rxbuf, 1);
-- 
1.7.9.7

Re: [PATCH 07/11] iio/adc: (max1363) Fix data conversion problems

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> For chips with more than 8 bit ADC resolution, received data was always
> masked against 0xfff, ie with a 12 bit mask. This can result in bad data
> for chips with 10 bit resolution if those chips have higher bits set
> (seen with MAX1139).
> 
> The receive buffer was defined as char array. This could result in
> unintentional sign extensions if the upper bit in a received byte
> was set. Since the chip is configured for unipolar mode, we never
> have to handle negative values, and sign extensions are never needed.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied to togreg branch of iio.git
> ---
>  drivers/iio/adc/max1363.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 2c773b4..fdc8be9 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -334,7 +334,7 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  {
>  	int ret = 0;
>  	s32 data;
> -	char rxbuf[2];
> +	u8 rxbuf[2];
>  	struct max1363_state *st = iio_priv(indio_dev);
>  	struct i2c_client *client = st->client;
>  
> @@ -366,7 +366,8 @@ static int max1363_read_single_chan(struct iio_dev *indio_dev,
>  			ret = data;
>  			goto error_ret;
>  		}
> -		data = (s32)(rxbuf[1]) | ((s32)(rxbuf[0] & 0x0F)) << 8;
> +		data = (rxbuf[1] | rxbuf[0] << 8) &
> +		  ((1 << st->chip_info->bits) - 1);
>  	} else {
>  		/* Get reading */
>  		data = i2c_master_recv(client, rxbuf, 1);
> 

[RFC 08/11] iio: Update iio_channel_get_all and iio_channel_get_all_cb API

[Guenter Roeck]

Pass device pointer instead of device name as parameter to iio_channel_get_all
and iio_channel_get_all_cb. This will enable us to use OF information to
retrieve consumer channel information.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/buffer_cb.c         |    4 ++--
 drivers/iio/inkern.c            |    6 ++++--
 drivers/staging/iio/iio_hwmon.c |   10 +++++++---
 include/linux/iio/consumer.h    |    8 ++++----
 4 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
index 4d40e24..9201022 100644
--- a/drivers/iio/buffer_cb.c
+++ b/drivers/iio/buffer_cb.c
@@ -25,7 +25,7 @@ static struct iio_buffer_access_funcs iio_cb_access = {
 	.store_to = &iio_buffer_cb_store_to,
 };
 
-struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
+struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 					     int (*cb)(u8 *data,
 						       void *private),
 					     void *private)
@@ -46,7 +46,7 @@ struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
 	cb_buff->buffer.access = &iio_cb_access;
 	INIT_LIST_HEAD(&cb_buff->buffer.demux_list);
 
-	cb_buff->channels = iio_channel_get_all(name);
+	cb_buff->channels = iio_channel_get_all(dev);
 	if (IS_ERR(cb_buff->channels)) {
 		ret = PTR_ERR(cb_buff->channels);
 		goto error_free_cb_buff;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index d55e98f..58d0ffe 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -167,16 +167,18 @@ void iio_channel_release(struct iio_channel *channel)
 }
 EXPORT_SYMBOL_GPL(iio_channel_release);
 
-struct iio_channel *iio_channel_get_all(const char *name)
+struct iio_channel *iio_channel_get_all(struct device *dev)
 {
+	const char *name;
 	struct iio_channel *chans;
 	struct iio_map_internal *c = NULL;
 	int nummaps = 0;
 	int mapind = 0;
 	int i, ret;
 
-	if (name == NULL)
+	if (dev == NULL)
 		return ERR_PTR(-EINVAL);
+	name = dev_name(dev);
 
 	mutex_lock(&iio_map_list_lock);
 	/* first count the matching maps */
diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
index 6a5bc5f..a4fce64 100644
--- a/drivers/staging/iio/iio_hwmon.c
+++ b/drivers/staging/iio/iio_hwmon.c
@@ -71,14 +71,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
 	int ret, i;
 	int in_i = 1, temp_i = 1, curr_i = 1;
 	enum iio_chan_type type;
+	struct iio_channel *channels;
+	const __be32 *list;
+
+	channels = iio_channel_get_all(dev);
+	if (IS_ERR(channels))
+		return PTR_ERR(channels);
 
 	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
 		return -ENOMEM;
 
-	st->channels = iio_channel_get_all(dev_name(dev));
-	if (IS_ERR(st->channels))
-		return PTR_ERR(st->channels);
+	st->channels = channels;
 
 	/* count how many attributes we have */
 	while (st->channels[st->num_channels].indio_dev)
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 16c35ac..6c44167 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -48,14 +48,14 @@ void iio_channel_release(struct iio_channel *chan);
 
 /**
  * iio_channel_get_all() - get all channels associated with a client
- * @name:		name of consumer device.
+ * @dev:		Pointer to consumer device.
  *
  * Returns an array of iio_channel structures terminated with one with
  * null iio_dev pointer.
  * This function is used by fairly generic consumers to get all the
  * channels registered as having this consumer.
  */
-struct iio_channel *iio_channel_get_all(const char *name);
+struct iio_channel *iio_channel_get_all(struct device *dev);
 
 /**
  * iio_channel_release_all() - reverse iio_channel_get_all
@@ -66,7 +66,7 @@ void iio_channel_release_all(struct iio_channel *chan);
 struct iio_cb_buffer;
 /**
  * iio_channel_get_all_cb() - register callback for triggered capture
- * @name:		Name of client device.
+ * @dev:		Pointer to client device.
  * @cb:			Callback function.
  * @private:		Private data passed to callback.
  *
@@ -74,7 +74,7 @@ struct iio_cb_buffer;
  * So if the channels requested come from different devices this will
  * fail.
  */
-struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
+struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
 					     int (*cb)(u8 *data,
 						       void *private),
 					     void *private);
-- 
1.7.9.7

Re: [RFC 08/11] iio: Update iio_channel_get_all and iio_channel_get_all_cb API

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Pass device pointer instead of device name as parameter to iio_channel_get_all
> and iio_channel_get_all_cb. This will enable us to use OF information to
> retrieve consumer channel information.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied with unused 'lists' variable dropped.
> ---
>  drivers/iio/buffer_cb.c         |    4 ++--
>  drivers/iio/inkern.c            |    6 ++++--
>  drivers/staging/iio/iio_hwmon.c |   10 +++++++---
>  include/linux/iio/consumer.h    |    8 ++++----
>  4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/buffer_cb.c b/drivers/iio/buffer_cb.c
> index 4d40e24..9201022 100644
> --- a/drivers/iio/buffer_cb.c
> +++ b/drivers/iio/buffer_cb.c
> @@ -25,7 +25,7 @@ static struct iio_buffer_access_funcs iio_cb_access = {
>  	.store_to = &iio_buffer_cb_store_to,
>  };
>  
> -struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
> +struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  					     int (*cb)(u8 *data,
>  						       void *private),
>  					     void *private)
> @@ -46,7 +46,7 @@ struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
>  	cb_buff->buffer.access = &iio_cb_access;
>  	INIT_LIST_HEAD(&cb_buff->buffer.demux_list);
>  
> -	cb_buff->channels = iio_channel_get_all(name);
> +	cb_buff->channels = iio_channel_get_all(dev);
>  	if (IS_ERR(cb_buff->channels)) {
>  		ret = PTR_ERR(cb_buff->channels);
>  		goto error_free_cb_buff;
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index d55e98f..58d0ffe 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -167,16 +167,18 @@ void iio_channel_release(struct iio_channel *channel)
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_release);
>  
> -struct iio_channel *iio_channel_get_all(const char *name)
> +struct iio_channel *iio_channel_get_all(struct device *dev)
>  {
> +	const char *name;
>  	struct iio_channel *chans;
>  	struct iio_map_internal *c = NULL;
>  	int nummaps = 0;
>  	int mapind = 0;
>  	int i, ret;
>  
> -	if (name == NULL)
> +	if (dev == NULL)
>  		return ERR_PTR(-EINVAL);
> +	name = dev_name(dev);
>  
>  	mutex_lock(&iio_map_list_lock);
>  	/* first count the matching maps */
> diff --git a/drivers/staging/iio/iio_hwmon.c b/drivers/staging/iio/iio_hwmon.c
> index 6a5bc5f..a4fce64 100644
> --- a/drivers/staging/iio/iio_hwmon.c
> +++ b/drivers/staging/iio/iio_hwmon.c
> @@ -71,14 +71,18 @@ static int iio_hwmon_probe(struct platform_device *pdev)
>  	int ret, i;
>  	int in_i = 1, temp_i = 1, curr_i = 1;
>  	enum iio_chan_type type;
> +	struct iio_channel *channels;
> +	const __be32 *list;
> +
> +	channels = iio_channel_get_all(dev);
> +	if (IS_ERR(channels))
> +		return PTR_ERR(channels);
>  
>  	st = devm_kzalloc(dev, sizeof(*st), GFP_KERNEL);
>  	if (st == NULL)
>  		return -ENOMEM;
>  
> -	st->channels = iio_channel_get_all(dev_name(dev));
> -	if (IS_ERR(st->channels))
> -		return PTR_ERR(st->channels);
> +	st->channels = channels;
>  
>  	/* count how many attributes we have */
>  	while (st->channels[st->num_channels].indio_dev)
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 16c35ac..6c44167 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -48,14 +48,14 @@ void iio_channel_release(struct iio_channel *chan);
>  
>  /**
>   * iio_channel_get_all() - get all channels associated with a client
> - * @name:		name of consumer device.
> + * @dev:		Pointer to consumer device.
>   *
>   * Returns an array of iio_channel structures terminated with one with
>   * null iio_dev pointer.
>   * This function is used by fairly generic consumers to get all the
>   * channels registered as having this consumer.
>   */
> -struct iio_channel *iio_channel_get_all(const char *name);
> +struct iio_channel *iio_channel_get_all(struct device *dev);
>  
>  /**
>   * iio_channel_release_all() - reverse iio_channel_get_all
> @@ -66,7 +66,7 @@ void iio_channel_release_all(struct iio_channel *chan);
>  struct iio_cb_buffer;
>  /**
>   * iio_channel_get_all_cb() - register callback for triggered capture
> - * @name:		Name of client device.
> + * @dev:		Pointer to client device.
>   * @cb:			Callback function.
>   * @private:		Private data passed to callback.
>   *
> @@ -74,7 +74,7 @@ struct iio_cb_buffer;
>   * So if the channels requested come from different devices this will
>   * fail.
>   */
> -struct iio_cb_buffer *iio_channel_get_all_cb(const char *name,
> +struct iio_cb_buffer *iio_channel_get_all_cb(struct device *dev,
>  					     int (*cb)(u8 *data,
>  						       void *private),
>  					     void *private);
> 

[RFC 09/11] iio: Simplify iio_map_array_unregister API

[Guenter Roeck]

Instead of requiring the map to unregister, simply unregister all map entries
associated with the given iio device. This simplifies map removal and also works
for maps generated through devicetree.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/iio/adc/lp8788_adc.c |   11 ++---------
 drivers/iio/adc/max1363.c    |    4 ++--
 drivers/iio/inkern.c         |   36 +++++++++++-------------------------
 include/linux/iio/driver.h   |    9 +++------
 4 files changed, 18 insertions(+), 42 deletions(-)

diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
index e5293d0..d6d509b 100644
--- a/drivers/iio/adc/lp8788_adc.c
+++ b/drivers/iio/adc/lp8788_adc.c
@@ -187,12 +187,6 @@ static int lp8788_iio_map_register(struct iio_dev *indio_dev,
 	return 0;
 }
 
-static inline void lp8788_iio_map_unregister(struct iio_dev *indio_dev,
-				struct lp8788_adc *adc)
-{
-	iio_map_array_unregister(indio_dev, adc->map);
-}
-
 static int lp8788_adc_probe(struct platform_device *pdev)
 {
 	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
@@ -231,7 +225,7 @@ static int lp8788_adc_probe(struct platform_device *pdev)
 	return 0;
 
 err_iio_device:
-	lp8788_iio_map_unregister(indio_dev, adc);
+	iio_map_array_unregister(indio_dev);
 err_iio_map:
 	iio_device_free(indio_dev);
 	return ret;
@@ -240,10 +234,9 @@ err_iio_map:
 static int lp8788_adc_remove(struct platform_device *pdev)
 {
 	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct lp8788_adc *adc = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	lp8788_iio_map_unregister(indio_dev, adc);
+	iio_map_array_unregister(indio_dev);
 	iio_device_free(indio_dev);
 
 	return 0;
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index fdc8be9..51165d6 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -1618,7 +1618,7 @@ error_disable_reg:
 error_put_reg:
 	regulator_put(st->reg);
 error_unregister_map:
-	iio_map_array_unregister(indio_dev, client->dev.platform_data);
+	iio_map_array_unregister(indio_dev);
 error_free_device:
 	iio_device_free(indio_dev);
 error_out:
@@ -1638,7 +1638,7 @@ static int max1363_remove(struct i2c_client *client)
 	kfree(indio_dev->available_scan_masks);
 	regulator_disable(st->reg);
 	regulator_put(st->reg);
-	iio_map_array_unregister(indio_dev, client->dev.platform_data);
+	iio_map_array_unregister(indio_dev);
 	iio_device_free(indio_dev);
 
 	return 0;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 58d0ffe..c42aba6 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -54,39 +54,25 @@ error_ret:
 EXPORT_SYMBOL_GPL(iio_map_array_register);
 
 
-/* Assumes the exact same array (e.g. memory locations)
- * used at unregistration as used at registration rather than
- * more complex checking of contents.
+/*
+ * Remove all map entries associated with the given iio device
  */
-int iio_map_array_unregister(struct iio_dev *indio_dev,
-			     struct iio_map *maps)
+int iio_map_array_unregister(struct iio_dev *indio_dev)
 {
-	int i = 0, ret = 0;
-	bool found_it;
+	int ret = -ENODEV;
 	struct iio_map_internal *mapi;
-
-	if (maps == NULL)
-		return 0;
+	struct list_head *pos, *tmp;
 
 	mutex_lock(&iio_map_list_lock);
-	while (maps[i].consumer_dev_name != NULL) {
-		found_it = false;
-		list_for_each_entry(mapi, &iio_map_list, l)
-			if (&maps[i] == mapi->map) {
-				list_del(&mapi->l);
-				kfree(mapi);
-				found_it = true;
-				break;
-			}
-		if (!found_it) {
-			ret = -ENODEV;
-			goto error_ret;
+	list_for_each_safe(pos, tmp, &iio_map_list) {
+		mapi = list_entry(pos, struct iio_map_internal, l);
+		if (indio_dev == mapi->indio_dev) {
+			list_del(&mapi->l);
+			kfree(mapi);
+			ret = 0;
 		}
-		i++;
 	}
-error_ret:
 	mutex_unlock(&iio_map_list_lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iio_map_array_unregister);
diff --git a/include/linux/iio/driver.h b/include/linux/iio/driver.h
index a4f8b2e..7dfb10e 100644
--- a/include/linux/iio/driver.h
+++ b/include/linux/iio/driver.h
@@ -22,13 +22,10 @@ int iio_map_array_register(struct iio_dev *indio_dev,
 			   struct iio_map *map);
 
 /**
- * iio_map_array_unregister() - tell the core to remove consumer mappings
+ * iio_map_array_unregister() - tell the core to remove consumer mappings for
+ *				the given provider device
  * @indio_dev:	provider device
- * @map:	array of mappings to remove. Note these must have same memory
- *		addresses as those originally added not just equal parameter
- *		values.
  */
-int iio_map_array_unregister(struct iio_dev *indio_dev,
-			     struct iio_map *map);
+int iio_map_array_unregister(struct iio_dev *indio_dev);
 
 #endif
-- 
1.7.9.7

Re: [RFC 09/11] iio: Simplify iio_map_array_unregister API

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Instead of requiring the map to unregister, simply unregister all map entries
> associated with the given iio device. This simplifies map removal and also works
> for maps generated through devicetree.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Sensible change. No idea why this never occured to me at the time ;)

Applied to togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/lp8788_adc.c |   11 ++---------
>  drivers/iio/adc/max1363.c    |    4 ++--
>  drivers/iio/inkern.c         |   36 +++++++++++-------------------------
>  include/linux/iio/driver.h   |    9 +++------
>  4 files changed, 18 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/iio/adc/lp8788_adc.c b/drivers/iio/adc/lp8788_adc.c
> index e5293d0..d6d509b 100644
> --- a/drivers/iio/adc/lp8788_adc.c
> +++ b/drivers/iio/adc/lp8788_adc.c
> @@ -187,12 +187,6 @@ static int lp8788_iio_map_register(struct iio_dev *indio_dev,
>  	return 0;
>  }
>  
> -static inline void lp8788_iio_map_unregister(struct iio_dev *indio_dev,
> -				struct lp8788_adc *adc)
> -{
> -	iio_map_array_unregister(indio_dev, adc->map);
> -}
> -
>  static int lp8788_adc_probe(struct platform_device *pdev)
>  {
>  	struct lp8788 *lp = dev_get_drvdata(pdev->dev.parent);
> @@ -231,7 +225,7 @@ static int lp8788_adc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_iio_device:
> -	lp8788_iio_map_unregister(indio_dev, adc);
> +	iio_map_array_unregister(indio_dev);
>  err_iio_map:
>  	iio_device_free(indio_dev);
>  	return ret;
> @@ -240,10 +234,9 @@ err_iio_map:
>  static int lp8788_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct lp8788_adc *adc = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> -	lp8788_iio_map_unregister(indio_dev, adc);
> +	iio_map_array_unregister(indio_dev);
>  	iio_device_free(indio_dev);
>  
>  	return 0;
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index fdc8be9..51165d6 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -1618,7 +1618,7 @@ error_disable_reg:
>  error_put_reg:
>  	regulator_put(st->reg);
>  error_unregister_map:
> -	iio_map_array_unregister(indio_dev, client->dev.platform_data);
> +	iio_map_array_unregister(indio_dev);
>  error_free_device:
>  	iio_device_free(indio_dev);
>  error_out:
> @@ -1638,7 +1638,7 @@ static int max1363_remove(struct i2c_client *client)
>  	kfree(indio_dev->available_scan_masks);
>  	regulator_disable(st->reg);
>  	regulator_put(st->reg);
> -	iio_map_array_unregister(indio_dev, client->dev.platform_data);
> +	iio_map_array_unregister(indio_dev);
>  	iio_device_free(indio_dev);
>  
>  	return 0;
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 58d0ffe..c42aba6 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -54,39 +54,25 @@ error_ret:
>  EXPORT_SYMBOL_GPL(iio_map_array_register);
>  
>  
> -/* Assumes the exact same array (e.g. memory locations)
> - * used at unregistration as used at registration rather than
> - * more complex checking of contents.
> +/*
> + * Remove all map entries associated with the given iio device
>   */
> -int iio_map_array_unregister(struct iio_dev *indio_dev,
> -			     struct iio_map *maps)
> +int iio_map_array_unregister(struct iio_dev *indio_dev)
>  {
> -	int i = 0, ret = 0;
> -	bool found_it;
> +	int ret = -ENODEV;
>  	struct iio_map_internal *mapi;
> -
> -	if (maps == NULL)
> -		return 0;
> +	struct list_head *pos, *tmp;
>  
>  	mutex_lock(&iio_map_list_lock);
> -	while (maps[i].consumer_dev_name != NULL) {
> -		found_it = false;
> -		list_for_each_entry(mapi, &iio_map_list, l)
> -			if (&maps[i] == mapi->map) {
> -				list_del(&mapi->l);
> -				kfree(mapi);
> -				found_it = true;
> -				break;
> -			}
> -		if (!found_it) {
> -			ret = -ENODEV;
> -			goto error_ret;
> +	list_for_each_safe(pos, tmp, &iio_map_list) {
> +		mapi = list_entry(pos, struct iio_map_internal, l);
> +		if (indio_dev == mapi->indio_dev) {
> +			list_del(&mapi->l);
> +			kfree(mapi);
> +			ret = 0;
>  		}
> -		i++;
>  	}
> -error_ret:
>  	mutex_unlock(&iio_map_list_lock);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(iio_map_array_unregister);
> diff --git a/include/linux/iio/driver.h b/include/linux/iio/driver.h
> index a4f8b2e..7dfb10e 100644
> --- a/include/linux/iio/driver.h
> +++ b/include/linux/iio/driver.h
> @@ -22,13 +22,10 @@ int iio_map_array_register(struct iio_dev *indio_dev,
>  			   struct iio_map *map);
>  
>  /**
> - * iio_map_array_unregister() - tell the core to remove consumer mappings
> + * iio_map_array_unregister() - tell the core to remove consumer mappings for
> + *				the given provider device
>   * @indio_dev:	provider device
> - * @map:	array of mappings to remove. Note these must have same memory
> - *		addresses as those originally added not just equal parameter
> - *		values.
>   */
> -int iio_map_array_unregister(struct iio_dev *indio_dev,
> -			     struct iio_map *map);
> +int iio_map_array_unregister(struct iio_dev *indio_dev);
>  
>  #endif
> 

[RFC 10/11] iio: Add OF support

[Guenter Roeck]

Provide bindings, new API access functions, and parse OF data
during initialization.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
 drivers/iio/inkern.c                               |  241 ++++++++++++++++----
 include/linux/iio/consumer.h                       |    8 +
 3 files changed, 299 insertions(+), 47 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..0f51c95
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
@@ -0,0 +1,97 @@
+This binding is a work-in-progress, and are based on clock bindings and
+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 more 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 clock 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.
+
+Optional properties:
+io-channel-output-names:
+		    Recommended to be a list of strings of IIO output signal
+		    names indexed by the first cell in the IIO specifier.
+		    However, the meaning of io-channel-output-names is domain
+		    specific to the IIO provider, and is only provided to
+		    encourage using the same meaning for the majority of IIO
+		    providers.  This format may not work for IIO providers
+		    using a complex IIO specifier format.  In those cases it
+		    is recommended to omit this property and create a binding
+		    specific names property.
+
+		    IIO consumer nodes must never directly reference
+		    the provider's io-channel-output-names property.
+
+For example:
+
+    adc: adc@35 {
+	compatible = "maxim,max1139";
+	reg = <0x35>;
+        #io-channel-cells = <1>;
+        io-channel-output-names = "adc1", "adc2";
+    };
+
+- this node defines a device with two named IIO outputs, the first named
+  "adc1" and the second named "adc2".  Consumer nodes always reference
+  IIO channels by index. The names should reflect the IIO output signal
+  names for the device.
+
+==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 #clock-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
+		IIO channels from this node. Useful for bus nodes to provide
+		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";
+	};
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c42aba6..af80a18 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -10,6 +10,7 @@
 #include <linux/export.h>
 #include <linux/slab.h>
 #include <linux/mutex.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include "iio_core.h"
@@ -26,13 +27,55 @@ struct iio_map_internal {
 static LIST_HEAD(iio_map_list);
 static DEFINE_MUTEX(iio_map_list_lock);
 
+static struct iio_map *iio_map_of_init(struct iio_dev *idev)
+{
+	struct device_node *np = idev->dev.of_node;
+	struct iio_map *maps;
+	u32 cells, channels = 1;
+	int names, i, ret;
+
+	ret = of_property_read_u32(np, "#io-channel-cells", &cells);
+	if (ret < 0 || cells > 1)
+		return ERR_PTR(-EINVAL);
+
+	names = of_property_count_strings(np, "io-channel-output-names");
+	if (names < 0)
+		names = 0;
+
+	if (channels < names)
+		channels = names;
+
+	maps = devm_kzalloc(&idev->dev, sizeof(*maps) * (channels + 1),
+			    GFP_KERNEL);
+	if (maps == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < channels; i++) {
+		if (i < names) {
+			ret = of_property_read_string_index(np,
+				    "io-channel-output-names", i,
+				    &maps[i].consumer_channel);
+			if (ret < 0)
+				return ERR_PTR(ret);
+		}
+		/* Use dummy for consumer device names */
+		maps[i].consumer_dev_name = "of";
+	}
+	return maps;
+}
+
 int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
 {
 	int i = 0, ret = 0;
 	struct iio_map_internal *mapi;
 
-	if (maps == NULL)
-		return 0;
+	if (maps == NULL) {
+		maps = iio_map_of_init(indio_dev);
+		if (IS_ERR(maps))
+			return PTR_ERR(maps);
+		if (maps == NULL)
+			return 0;
+	}
 
 	mutex_lock(&iio_map_list_lock);
 	while (maps[i].consumer_dev_name != NULL) {
@@ -92,30 +135,14 @@ static const struct iio_chan_spec
 	return chan;
 }
 
-
-struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
+struct iio_channel *iio_channel_get_common(struct iio_map_internal *c,
+					   int index)
 {
-	struct iio_map_internal *c_i = NULL, *c = NULL;
 	struct iio_channel *channel;
 	int err;
 
-	if (name == NULL && channel_name == NULL)
-		return ERR_PTR(-ENODEV);
-
-	/* first find matching entry the channel map */
-	mutex_lock(&iio_map_list_lock);
-	list_for_each_entry(c_i, &iio_map_list, l) {
-		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
-		    (channel_name &&
-		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
-			continue;
-		c = c_i;
-		iio_device_get(c->indio_dev);
-		break;
-	}
-	mutex_unlock(&iio_map_list_lock);
 	if (c == NULL)
-		return ERR_PTR(-ENODEV);
+		return ERR_PTR(-EPROBE_DEFER);
 
 	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
 	if (channel == NULL) {
@@ -125,7 +152,13 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
 
 	channel->indio_dev = c->indio_dev;
 
-	if (c->map->adc_channel_label) {
+	if (index >= 0) {
+		if (index >= c->indio_dev->num_channels) {
+			err = -EINVAL;
+			goto error_no_chan;
+		}
+		channel->channel = &c->indio_dev->channels[index];
+	} else if (c->map->adc_channel_label) {
 		channel->channel =
 			iio_chan_spec_from_name(channel->indio_dev,
 						c->map->adc_channel_label);
@@ -144,6 +177,60 @@ error_no_mem:
 	iio_device_put(c->indio_dev);
 	return ERR_PTR(err);
 }
+
+struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
+{
+	struct iio_map_internal *c_i, *c = NULL;
+	int err, map_index;
+	struct of_phandle_args iiospec;
+
+	if (index < 0)
+		return ERR_PTR(-EINVAL);
+
+	err = of_parse_phandle_with_args(np, "io-channels",
+					 "#io-channel-cells",
+					 index, &iiospec);
+	if (err)
+		return ERR_PTR(err);
+
+	map_index = iiospec.args_count ? iiospec.args[0] : 0;
+
+	mutex_lock(&iio_map_list_lock);
+	list_for_each_entry(c_i, &iio_map_list, l) {
+		if (iiospec.np == c_i->indio_dev->dev.of_node) {
+			c = c_i;
+			iio_device_get(c->indio_dev);
+			break;
+		}
+	}
+	of_node_put(iiospec.np);
+	mutex_unlock(&iio_map_list_lock);
+	return iio_channel_get_common(c, index);
+}
+EXPORT_SYMBOL_GPL(of_iio_channel_get);
+
+struct iio_channel *iio_channel_get(const char *name,
+				    const char *channel_name)
+{
+	struct iio_map_internal *c_i = NULL, *c = NULL;
+
+	if (name == NULL && channel_name == NULL)
+		return ERR_PTR(-ENODEV);
+
+	/* first find matching entry the channel map */
+	mutex_lock(&iio_map_list_lock);
+	list_for_each_entry(c_i, &iio_map_list, l) {
+		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
+		    (channel_name && strcmp(channel_name,
+					    c_i->map->consumer_channel) != 0))
+			continue;
+		c = c_i;
+		iio_device_get(c->indio_dev);
+		break;
+	}
+	mutex_unlock(&iio_map_list_lock);
+	return iio_channel_get_common(c, -1);
+}
 EXPORT_SYMBOL_GPL(iio_channel_get);
 
 void iio_channel_release(struct iio_channel *channel)
@@ -159,24 +246,43 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
 	struct iio_channel *chans;
 	struct iio_map_internal *c = NULL;
 	int nummaps = 0;
-	int mapind = 0;
+	int mapind;
 	int i, ret;
 
 	if (dev == NULL)
 		return ERR_PTR(-EINVAL);
+
 	name = dev_name(dev);
 
 	mutex_lock(&iio_map_list_lock);
 	/* first count the matching maps */
-	list_for_each_entry(c, &iio_map_list, l)
-		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
-			continue;
-		else
-			nummaps++;
-
-	if (nummaps == 0) {
-		ret = -ENODEV;
-		goto error_ret;
+	if (dev->of_node) {
+		nummaps = 0;
+		do {
+			int ret;
+
+			ret = of_parse_phandle_with_args(dev->of_node,
+							 "io-channels",
+							 "#io-channel-cells",
+							 nummaps, NULL);
+			if (ret < 0)
+				break;
+		} while (++nummaps);
+
+		if (nummaps == 0) {
+			ret = -ENODEV;
+			goto error_ret;
+		}
+	} else {
+		list_for_each_entry(c, &iio_map_list, l)
+			if (strcmp(name, c->map->consumer_dev_name) != 0)
+				continue;
+			else
+				nummaps++;
+		if (nummaps == 0) {
+			ret = -EPROBE_DEFER;
+			goto error_ret;
+		}
 	}
 
 	/* NULL terminated array to save passing size */
@@ -187,24 +293,65 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
 	}
 
 	/* for each map fill in the chans element */
-	list_for_each_entry(c, &iio_map_list, l) {
-		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
-			continue;
-		chans[mapind].indio_dev = c->indio_dev;
-		chans[mapind].data = c->map->consumer_data;
-		chans[mapind].channel =
-			iio_chan_spec_from_name(chans[mapind].indio_dev,
+	if (dev->of_node) {
+		/*
+		 * Device-tree based mapping, search for OF matches.
+		 */
+		for (mapind = 0; mapind < nummaps; mapind++) {
+			int channel;
+			struct of_phandle_args iiospec;
+
+			ret = of_parse_phandle_with_args(dev->of_node,
+							 "io-channels",
+							 "#io-channel-cells",
+							 mapind, &iiospec);
+			if (ret)
+				goto error_free_chans;
+			channel = iiospec.args_count ? iiospec.args[0] : 0;
+			ret = -EPROBE_DEFER;
+			list_for_each_entry(c, &iio_map_list, l) {
+				if (iiospec.np == c->indio_dev->dev.of_node) {
+					ret = 0;
+					break;
+				}
+			}
+			if (ret)
+				goto error_free_chans;
+			if (channel >= c->indio_dev->num_channels) {
+				ret = -EINVAL;
+				goto error_free_chans;
+			}
+			chans[mapind].indio_dev = c->indio_dev;
+			chans[mapind].data = c->map->consumer_data;
+			chans[mapind].channel =
+			  &c->indio_dev->channels[channel];
+			iio_device_get(c->indio_dev);
+		}
+	} else {
+		/*
+		 * Platform data based mapping, search for consumer
+		 * device name.
+		 */
+		mapind = 0;
+		list_for_each_entry(c, &iio_map_list, l) {
+			if (strcmp(name, c->map->consumer_dev_name) != 0)
+				continue;
+			chans[mapind].indio_dev = c->indio_dev;
+			chans[mapind].data = c->map->consumer_data;
+			chans[mapind].channel =
+				iio_chan_spec_from_name(c->indio_dev,
 						c->map->adc_channel_label);
-		if (chans[mapind].channel == NULL) {
-			ret = -EINVAL;
+			if (chans[mapind].channel == NULL) {
+				ret = -EINVAL;
+				goto error_free_chans;
+			}
+			iio_device_get(c->indio_dev);
+			mapind++;
+		}
+		if (mapind < nummaps) {
+			ret = -EPROBE_DEFER;
 			goto error_free_chans;
 		}
-		iio_device_get(chans[mapind].indio_dev);
-		mapind++;
-	}
-	if (mapind == 0) {
-		ret = -ENODEV;
-		goto error_free_chans;
 	}
 	mutex_unlock(&iio_map_list_lock);
 
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 6c44167..a347c15 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -41,6 +41,14 @@ struct iio_channel *iio_channel_get(const char *name,
 				    const char *consumer_channel);
 
 /**
+ * of_iio_channel_get() - get description of all that is needed to access
+ *			channel from OF node pointer.
+ * @np:			Consumer's OF node pointer.
+ * @index:		Index pointing to the io-channels entry in np.
+ */
+struct iio_channel *of_iio_channel_get(struct device_node *np, int index);
+
+/**
  * iio_channel_release() - release channels obtained via iio_channel_get
  * @chan:		The channel to be released.
  */
-- 
1.7.9.7

Re: [RFC 10/11] iio: Add OF support

[Lars-Peter Clausen]

013 10:43 PM, Guenter Roeck wrote:
> Provide bindings, new API access functions, and parse OF data
> during initialization.
> 

Hi Guenter,

Thanks for taking care of this.

I'd prefer to have only one iio_get_channel which handles both the dt and the
non-dt case. Otherwise we'll soon have constructs like

if (dev->of_node)
	chan = of_iio_get_channel(dev->of_node, 1);
else
	chan = iio_get_channel(dev_name(dev), "vcc");

appearing all over the place in drivers code. I'd keep the actual
implementation pretty close to what the clk framework does. E.g. take a look at
of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via the
iio_map lookup for the devicetree case, just loop over all IIO devices and
match by of_node. This should allow us to simplify the code quite a bit.

- Lars

On 01/31/2

> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>  include/linux/iio/consumer.h                       |    8 +
>  3 files changed, 299 insertions(+), 47 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..0f51c95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,97 @@
> +This binding is a work-in-progress, and are based on clock bindings and
> +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 more 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 clock 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.
> +
> +Optional properties:
> +io-channel-output-names:
> +		    Recommended to be a list of strings of IIO output signal
> +		    names indexed by the first cell in the IIO specifier.
> +		    However, the meaning of io-channel-output-names is domain
> +		    specific to the IIO provider, and is only provided to
> +		    encourage using the same meaning for the majority of IIO
> +		    providers.  This format may not work for IIO providers
> +		    using a complex IIO specifier format.  In those cases it
> +		    is recommended to omit this property and create a binding
> +		    specific names property.
> +
> +		    IIO consumer nodes must never directly reference
> +		    the provider's io-channel-output-names property.
> +
> +For example:
> +
> +    adc: adc@35 {
> +	compatible = "maxim,max1139";
> +	reg = <0x35>;
> +        #io-channel-cells = <1>;
> +        io-channel-output-names = "adc1", "adc2";
> +    };
> +
> +- this node defines a device with two named IIO outputs, the first named
> +  "adc1" and the second named "adc2".  Consumer nodes always reference
> +  IIO channels by index. The names should reflect the IIO output signal
> +  names for the device.
> +
> +==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 #clock-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
> +		IIO channels from this node. Useful for bus nodes to provide
> +		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";
> +	};
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index c42aba6..af80a18 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -10,6 +10,7 @@
>  #include <linux/export.h>
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> @@ -26,13 +27,55 @@ struct iio_map_internal {
>  static LIST_HEAD(iio_map_list);
>  static DEFINE_MUTEX(iio_map_list_lock);
>  
> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
> +{

To be honest I find this whole function a bit confusing. If we are using
devicetree, we shouldn't need to register iio_map. Maybe just skip over
io-channel-output-names for now, until we figure out if we really need this and
what for.

> +	struct device_node *np = idev->dev.of_node;
> +	struct iio_map *maps;
> +	u32 cells, channels = 1;
> +	int names, i, ret;
> +
> +	ret = of_property_read_u32(np, "#io-channel-cells", &cells);
> +	if (ret < 0 || cells > 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	names = of_property_count_strings(np, "io-channel-output-names");
> +	if (names < 0)
> +		names = 0;
> +
> +	if (channels < names)
> +		channels = names;
> +
> +	maps = devm_kzalloc(&idev->dev, sizeof(*maps) * (channels + 1),
> +			    GFP_KERNEL);
> +	if (maps == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < channels; i++) {
> +		if (i < names) {
> +			ret = of_property_read_string_index(np,
> +				    "io-channel-output-names", i,
> +				    &maps[i].consumer_channel);
> +			if (ret < 0)
> +				return ERR_PTR(ret);
> +		}
> +		/* Use dummy for consumer device names */
> +		maps[i].consumer_dev_name = "of";
> +	}
> +	return maps;
> +}
> +
>  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  {
>  	int i = 0, ret = 0;
>  	struct iio_map_internal *mapi;
>  
> -	if (maps == NULL)
> -		return 0;
> +	if (maps == NULL) {
> +		maps = iio_map_of_init(indio_dev);
> +		if (IS_ERR(maps))
> +			return PTR_ERR(maps);
> +		if (maps == NULL)
> +			return 0;
> +	}
>  
>  	mutex_lock(&iio_map_list_lock);
>  	while (maps[i].consumer_dev_name != NULL) {
> @@ -92,30 +135,14 @@ static const struct iio_chan_spec
>  	return chan;
>  }
>  
> -
> -struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
> +struct iio_channel *iio_channel_get_common(struct iio_map_internal *c,
> +					   int index)
>  {
> -	struct iio_map_internal *c_i = NULL, *c = NULL;
>  	struct iio_channel *channel;
>  	int err;
>  
> -	if (name == NULL && channel_name == NULL)
> -		return ERR_PTR(-ENODEV);
> -
> -	/* first find matching entry the channel map */
> -	mutex_lock(&iio_map_list_lock);
> -	list_for_each_entry(c_i, &iio_map_list, l) {
> -		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> -		    (channel_name &&
> -		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
> -			continue;
> -		c = c_i;
> -		iio_device_get(c->indio_dev);
> -		break;
> -	}
> -	mutex_unlock(&iio_map_list_lock);
>  	if (c == NULL)
> -		return ERR_PTR(-ENODEV);
> +		return ERR_PTR(-EPROBE_DEFER);
>  
>  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>  	if (channel == NULL) {
> @@ -125,7 +152,13 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>  
>  	channel->indio_dev = c->indio_dev;
>  
> -	if (c->map->adc_channel_label) {
> +	if (index >= 0) {
> +		if (index >= c->indio_dev->num_channels) {
> +			err = -EINVAL;
> +			goto error_no_chan;
> +		}
> +		channel->channel = &c->indio_dev->channels[index];
> +	} else if (c->map->adc_channel_label) {
>  		channel->channel =
>  			iio_chan_spec_from_name(channel->indio_dev,
>  						c->map->adc_channel_label);
> @@ -144,6 +177,60 @@ error_no_mem:
>  	iio_device_put(c->indio_dev);
>  	return ERR_PTR(err);
>  }
> +
> +struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> +{
> +	struct iio_map_internal *c_i, *c = NULL;
> +	int err, map_index;
> +	struct of_phandle_args iiospec;
> +
> +	if (index < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	err = of_parse_phandle_with_args(np, "io-channels",
> +					 "#io-channel-cells",
> +					 index, &iiospec);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	map_index = iiospec.args_count ? iiospec.args[0] : 0;
> +
> +	mutex_lock(&iio_map_list_lock);
> +	list_for_each_entry(c_i, &iio_map_list, l) {
> +		if (iiospec.np == c_i->indio_dev->dev.of_node) {
> +			c = c_i;
> +			iio_device_get(c->indio_dev);
> +			break;
> +		}
> +	}
> +	of_node_put(iiospec.np);
> +	mutex_unlock(&iio_map_list_lock);
> +	return iio_channel_get_common(c, index);
> +}
> +EXPORT_SYMBOL_GPL(of_iio_channel_get);
> +
> +struct iio_channel *iio_channel_get(const char *name,
> +				    const char *channel_name)
> +{
> +	struct iio_map_internal *c_i = NULL, *c = NULL;
> +
> +	if (name == NULL && channel_name == NULL)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* first find matching entry the channel map */
> +	mutex_lock(&iio_map_list_lock);
> +	list_for_each_entry(c_i, &iio_map_list, l) {
> +		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> +		    (channel_name && strcmp(channel_name,
> +					    c_i->map->consumer_channel) != 0))
> +			continue;
> +		c = c_i;
> +		iio_device_get(c->indio_dev);
> +		break;
> +	}
> +	mutex_unlock(&iio_map_list_lock);
> +	return iio_channel_get_common(c, -1);
> +}
>  EXPORT_SYMBOL_GPL(iio_channel_get);
>  
>  void iio_channel_release(struct iio_channel *channel)
> @@ -159,24 +246,43 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  	struct iio_channel *chans;
>  	struct iio_map_internal *c = NULL;
>  	int nummaps = 0;
> -	int mapind = 0;
> +	int mapind;
>  	int i, ret;
>  
>  	if (dev == NULL)
>  		return ERR_PTR(-EINVAL);
> +
>  	name = dev_name(dev);
>  
>  	mutex_lock(&iio_map_list_lock);
>  	/* first count the matching maps */
> -	list_for_each_entry(c, &iio_map_list, l)
> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> -			continue;
> -		else
> -			nummaps++;
> -
> -	if (nummaps == 0) {
> -		ret = -ENODEV;
> -		goto error_ret;
> +	if (dev->of_node) {
> +		nummaps = 0;
> +		do {
> +			int ret;
> +
> +			ret = of_parse_phandle_with_args(dev->of_node,
> +							 "io-channels",
> +							 "#io-channel-cells",
> +							 nummaps, NULL);
> +			if (ret < 0)
> +				break;
> +		} while (++nummaps);
> +
> +		if (nummaps == 0) {
> +			ret = -ENODEV;
> +			goto error_ret;
> +		}
> +	} else {
> +		list_for_each_entry(c, &iio_map_list, l)
> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> +				continue;
> +			else
> +				nummaps++;
> +		if (nummaps == 0) {
> +			ret = -EPROBE_DEFER;
> +			goto error_ret;
> +		}
>  	}
>  
>  	/* NULL terminated array to save passing size */
> @@ -187,24 +293,65 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  	}
>  
>  	/* for each map fill in the chans element */
> -	list_for_each_entry(c, &iio_map_list, l) {
> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> -			continue;
> -		chans[mapind].indio_dev = c->indio_dev;
> -		chans[mapind].data = c->map->consumer_data;
> -		chans[mapind].channel =
> -			iio_chan_spec_from_name(chans[mapind].indio_dev,
> +	if (dev->of_node) {
> +		/*
> +		 * Device-tree based mapping, search for OF matches.
> +		 */
> +		for (mapind = 0; mapind < nummaps; mapind++) {
> +			int channel;
> +			struct of_phandle_args iiospec;
> +
> +			ret = of_parse_phandle_with_args(dev->of_node,
> +							 "io-channels",
> +							 "#io-channel-cells",
> +							 mapind, &iiospec);
> +			if (ret)
> +				goto error_free_chans;
> +			channel = iiospec.args_count ? iiospec.args[0] : 0;
> +			ret = -EPROBE_DEFER;
> +			list_for_each_entry(c, &iio_map_list, l) {
> +				if (iiospec.np == c->indio_dev->dev.of_node) {
> +					ret = 0;
> +					break;
> +				}
> +			}
> +			if (ret)
> +				goto error_free_chans;
> +			if (channel >= c->indio_dev->num_channels) {
> +				ret = -EINVAL;
> +				goto error_free_chans;
> +			}
> +			chans[mapind].indio_dev = c->indio_dev;
> +			chans[mapind].data = c->map->consumer_data;
> +			chans[mapind].channel =
> +			  &c->indio_dev->channels[channel];
> +			iio_device_get(c->indio_dev);
> +		}
> +	} else {
> +		/*
> +		 * Platform data based mapping, search for consumer
> +		 * device name.
> +		 */
> +		mapind = 0;
> +		list_for_each_entry(c, &iio_map_list, l) {
> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> +				continue;
> +			chans[mapind].indio_dev = c->indio_dev;
> +			chans[mapind].data = c->map->consumer_data;
> +			chans[mapind].channel =
> +				iio_chan_spec_from_name(c->indio_dev,
>  						c->map->adc_channel_label);
> -		if (chans[mapind].channel == NULL) {
> -			ret = -EINVAL;
> +			if (chans[mapind].channel == NULL) {
> +				ret = -EINVAL;
> +				goto error_free_chans;
> +			}
> +			iio_device_get(c->indio_dev);
> +			mapind++;
> +		}
> +		if (mapind < nummaps) {
> +			ret = -EPROBE_DEFER;
>  			goto error_free_chans;
>  		}
> -		iio_device_get(chans[mapind].indio_dev);
> -		mapind++;
> -	}
> -	if (mapind == 0) {
> -		ret = -ENODEV;
> -		goto error_free_chans;
>  	}
>  	mutex_unlock(&iio_map_list_lock);
>  
[...]

Re: [RFC 10/11] iio: Add OF support

[Guenter Roeck]

On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
> 013 10:43 PM, Guenter Roeck wrote:
> > Provide bindings, new API access functions, and parse OF data
> > during initialization.
> > 
> 
> Hi Guenter,
> 
> Thanks for taking care of this.
> 
> I'd prefer to have only one iio_get_channel which handles both the dt and the
> non-dt case. Otherwise we'll soon have constructs like
> 
> if (dev->of_node)
> 	chan = of_iio_get_channel(dev->of_node, 1);
> else
> 	chan = iio_get_channel(dev_name(dev), "vcc");
> 
> appearing all over the place in drivers code. I'd keep the actual
> implementation pretty close to what the clk framework does. E.g. take a look at
> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via the
> iio_map lookup for the devicetree case, just loop over all IIO devices and
> match by of_node. This should allow us to simplify the code quite a bit.
> 
Yes, I just could not figure out how to loop over the list of iio devices. That
is why I hijacked iio_map which does provide a list.

Any hints ?

Thanks,
Guenter

> - Lars
> 
> On 01/31/2
> 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > ---
> >  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
> >  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
> >  include/linux/iio/consumer.h                       |    8 +
> >  3 files changed, 299 insertions(+), 47 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..0f51c95
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -0,0 +1,97 @@
> > +This binding is a work-in-progress, and are based on clock bindings and
> > +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 more 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 clock 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.
> > +
> > +Optional properties:
> > +io-channel-output-names:
> > +		    Recommended to be a list of strings of IIO output signal
> > +		    names indexed by the first cell in the IIO specifier.
> > +		    However, the meaning of io-channel-output-names is domain
> > +		    specific to the IIO provider, and is only provided to
> > +		    encourage using the same meaning for the majority of IIO
> > +		    providers.  This format may not work for IIO providers
> > +		    using a complex IIO specifier format.  In those cases it
> > +		    is recommended to omit this property and create a binding
> > +		    specific names property.
> > +
> > +		    IIO consumer nodes must never directly reference
> > +		    the provider's io-channel-output-names property.
> > +
> > +For example:
> > +
> > +    adc: adc@35 {
> > +	compatible = "maxim,max1139";
> > +	reg = <0x35>;
> > +        #io-channel-cells = <1>;
> > +        io-channel-output-names = "adc1", "adc2";
> > +    };
> > +
> > +- this node defines a device with two named IIO outputs, the first named
> > +  "adc1" and the second named "adc2".  Consumer nodes always reference
> > +  IIO channels by index. The names should reflect the IIO output signal
> > +  names for the device.
> > +
> > +==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 #clock-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
> > +		IIO channels from this node. Useful for bus nodes to provide
> > +		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";
> > +	};
> > diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> > index c42aba6..af80a18 100644
> > --- a/drivers/iio/inkern.c
> > +++ b/drivers/iio/inkern.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/export.h>
> >  #include <linux/slab.h>
> >  #include <linux/mutex.h>
> > +#include <linux/of.h>
> >  
> >  #include <linux/iio/iio.h>
> >  #include "iio_core.h"
> > @@ -26,13 +27,55 @@ struct iio_map_internal {
> >  static LIST_HEAD(iio_map_list);
> >  static DEFINE_MUTEX(iio_map_list_lock);
> >  
> > +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
> > +{
> 
> To be honest I find this whole function a bit confusing. If we are using
> devicetree, we shouldn't need to register iio_map. Maybe just skip over
> io-channel-output-names for now, until we figure out if we really need this and
> what for.
> 
> > +	struct device_node *np = idev->dev.of_node;
> > +	struct iio_map *maps;
> > +	u32 cells, channels = 1;
> > +	int names, i, ret;
> > +
> > +	ret = of_property_read_u32(np, "#io-channel-cells", &cells);
> > +	if (ret < 0 || cells > 1)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	names = of_property_count_strings(np, "io-channel-output-names");
> > +	if (names < 0)
> > +		names = 0;
> > +
> > +	if (channels < names)
> > +		channels = names;
> > +
> > +	maps = devm_kzalloc(&idev->dev, sizeof(*maps) * (channels + 1),
> > +			    GFP_KERNEL);
> > +	if (maps == NULL)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	for (i = 0; i < channels; i++) {
> > +		if (i < names) {
> > +			ret = of_property_read_string_index(np,
> > +				    "io-channel-output-names", i,
> > +				    &maps[i].consumer_channel);
> > +			if (ret < 0)
> > +				return ERR_PTR(ret);
> > +		}
> > +		/* Use dummy for consumer device names */
> > +		maps[i].consumer_dev_name = "of";
> > +	}
> > +	return maps;
> > +}
> > +
> >  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
> >  {
> >  	int i = 0, ret = 0;
> >  	struct iio_map_internal *mapi;
> >  
> > -	if (maps == NULL)
> > -		return 0;
> > +	if (maps == NULL) {
> > +		maps = iio_map_of_init(indio_dev);
> > +		if (IS_ERR(maps))
> > +			return PTR_ERR(maps);
> > +		if (maps == NULL)
> > +			return 0;
> > +	}
> >  
> >  	mutex_lock(&iio_map_list_lock);
> >  	while (maps[i].consumer_dev_name != NULL) {
> > @@ -92,30 +135,14 @@ static const struct iio_chan_spec
> >  	return chan;
> >  }
> >  
> > -
> > -struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
> > +struct iio_channel *iio_channel_get_common(struct iio_map_internal *c,
> > +					   int index)
> >  {
> > -	struct iio_map_internal *c_i = NULL, *c = NULL;
> >  	struct iio_channel *channel;
> >  	int err;
> >  
> > -	if (name == NULL && channel_name == NULL)
> > -		return ERR_PTR(-ENODEV);
> > -
> > -	/* first find matching entry the channel map */
> > -	mutex_lock(&iio_map_list_lock);
> > -	list_for_each_entry(c_i, &iio_map_list, l) {
> > -		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> > -		    (channel_name &&
> > -		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
> > -			continue;
> > -		c = c_i;
> > -		iio_device_get(c->indio_dev);
> > -		break;
> > -	}
> > -	mutex_unlock(&iio_map_list_lock);
> >  	if (c == NULL)
> > -		return ERR_PTR(-ENODEV);
> > +		return ERR_PTR(-EPROBE_DEFER);
> >  
> >  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> >  	if (channel == NULL) {
> > @@ -125,7 +152,13 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
> >  
> >  	channel->indio_dev = c->indio_dev;
> >  
> > -	if (c->map->adc_channel_label) {
> > +	if (index >= 0) {
> > +		if (index >= c->indio_dev->num_channels) {
> > +			err = -EINVAL;
> > +			goto error_no_chan;
> > +		}
> > +		channel->channel = &c->indio_dev->channels[index];
> > +	} else if (c->map->adc_channel_label) {
> >  		channel->channel =
> >  			iio_chan_spec_from_name(channel->indio_dev,
> >  						c->map->adc_channel_label);
> > @@ -144,6 +177,60 @@ error_no_mem:
> >  	iio_device_put(c->indio_dev);
> >  	return ERR_PTR(err);
> >  }
> > +
> > +struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> > +{
> > +	struct iio_map_internal *c_i, *c = NULL;
> > +	int err, map_index;
> > +	struct of_phandle_args iiospec;
> > +
> > +	if (index < 0)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	err = of_parse_phandle_with_args(np, "io-channels",
> > +					 "#io-channel-cells",
> > +					 index, &iiospec);
> > +	if (err)
> > +		return ERR_PTR(err);
> > +
> > +	map_index = iiospec.args_count ? iiospec.args[0] : 0;
> > +
> > +	mutex_lock(&iio_map_list_lock);
> > +	list_for_each_entry(c_i, &iio_map_list, l) {
> > +		if (iiospec.np == c_i->indio_dev->dev.of_node) {
> > +			c = c_i;
> > +			iio_device_get(c->indio_dev);
> > +			break;
> > +		}
> > +	}
> > +	of_node_put(iiospec.np);
> > +	mutex_unlock(&iio_map_list_lock);
> > +	return iio_channel_get_common(c, index);
> > +}
> > +EXPORT_SYMBOL_GPL(of_iio_channel_get);
> > +
> > +struct iio_channel *iio_channel_get(const char *name,
> > +				    const char *channel_name)
> > +{
> > +	struct iio_map_internal *c_i = NULL, *c = NULL;
> > +
> > +	if (name == NULL && channel_name == NULL)
> > +		return ERR_PTR(-ENODEV);
> > +
> > +	/* first find matching entry the channel map */
> > +	mutex_lock(&iio_map_list_lock);
> > +	list_for_each_entry(c_i, &iio_map_list, l) {
> > +		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> > +		    (channel_name && strcmp(channel_name,
> > +					    c_i->map->consumer_channel) != 0))
> > +			continue;
> > +		c = c_i;
> > +		iio_device_get(c->indio_dev);
> > +		break;
> > +	}
> > +	mutex_unlock(&iio_map_list_lock);
> > +	return iio_channel_get_common(c, -1);
> > +}
> >  EXPORT_SYMBOL_GPL(iio_channel_get);
> >  
> >  void iio_channel_release(struct iio_channel *channel)
> > @@ -159,24 +246,43 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  	struct iio_channel *chans;
> >  	struct iio_map_internal *c = NULL;
> >  	int nummaps = 0;
> > -	int mapind = 0;
> > +	int mapind;
> >  	int i, ret;
> >  
> >  	if (dev == NULL)
> >  		return ERR_PTR(-EINVAL);
> > +
> >  	name = dev_name(dev);
> >  
> >  	mutex_lock(&iio_map_list_lock);
> >  	/* first count the matching maps */
> > -	list_for_each_entry(c, &iio_map_list, l)
> > -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> > -			continue;
> > -		else
> > -			nummaps++;
> > -
> > -	if (nummaps == 0) {
> > -		ret = -ENODEV;
> > -		goto error_ret;
> > +	if (dev->of_node) {
> > +		nummaps = 0;
> > +		do {
> > +			int ret;
> > +
> > +			ret = of_parse_phandle_with_args(dev->of_node,
> > +							 "io-channels",
> > +							 "#io-channel-cells",
> > +							 nummaps, NULL);
> > +			if (ret < 0)
> > +				break;
> > +		} while (++nummaps);
> > +
> > +		if (nummaps == 0) {
> > +			ret = -ENODEV;
> > +			goto error_ret;
> > +		}
> > +	} else {
> > +		list_for_each_entry(c, &iio_map_list, l)
> > +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> > +				continue;
> > +			else
> > +				nummaps++;
> > +		if (nummaps == 0) {
> > +			ret = -EPROBE_DEFER;
> > +			goto error_ret;
> > +		}
> >  	}
> >  
> >  	/* NULL terminated array to save passing size */
> > @@ -187,24 +293,65 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
> >  	}
> >  
> >  	/* for each map fill in the chans element */
> > -	list_for_each_entry(c, &iio_map_list, l) {
> > -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> > -			continue;
> > -		chans[mapind].indio_dev = c->indio_dev;
> > -		chans[mapind].data = c->map->consumer_data;
> > -		chans[mapind].channel =
> > -			iio_chan_spec_from_name(chans[mapind].indio_dev,
> > +	if (dev->of_node) {
> > +		/*
> > +		 * Device-tree based mapping, search for OF matches.
> > +		 */
> > +		for (mapind = 0; mapind < nummaps; mapind++) {
> > +			int channel;
> > +			struct of_phandle_args iiospec;
> > +
> > +			ret = of_parse_phandle_with_args(dev->of_node,
> > +							 "io-channels",
> > +							 "#io-channel-cells",
> > +							 mapind, &iiospec);
> > +			if (ret)
> > +				goto error_free_chans;
> > +			channel = iiospec.args_count ? iiospec.args[0] : 0;
> > +			ret = -EPROBE_DEFER;
> > +			list_for_each_entry(c, &iio_map_list, l) {
> > +				if (iiospec.np == c->indio_dev->dev.of_node) {
> > +					ret = 0;
> > +					break;
> > +				}
> > +			}
> > +			if (ret)
> > +				goto error_free_chans;
> > +			if (channel >= c->indio_dev->num_channels) {
> > +				ret = -EINVAL;
> > +				goto error_free_chans;
> > +			}
> > +			chans[mapind].indio_dev = c->indio_dev;
> > +			chans[mapind].data = c->map->consumer_data;
> > +			chans[mapind].channel =
> > +			  &c->indio_dev->channels[channel];
> > +			iio_device_get(c->indio_dev);
> > +		}
> > +	} else {
> > +		/*
> > +		 * Platform data based mapping, search for consumer
> > +		 * device name.
> > +		 */
> > +		mapind = 0;
> > +		list_for_each_entry(c, &iio_map_list, l) {
> > +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> > +				continue;
> > +			chans[mapind].indio_dev = c->indio_dev;
> > +			chans[mapind].data = c->map->consumer_data;
> > +			chans[mapind].channel =
> > +				iio_chan_spec_from_name(c->indio_dev,
> >  						c->map->adc_channel_label);
> > -		if (chans[mapind].channel == NULL) {
> > -			ret = -EINVAL;
> > +			if (chans[mapind].channel == NULL) {
> > +				ret = -EINVAL;
> > +				goto error_free_chans;
> > +			}
> > +			iio_device_get(c->indio_dev);
> > +			mapind++;
> > +		}
> > +		if (mapind < nummaps) {
> > +			ret = -EPROBE_DEFER;
> >  			goto error_free_chans;
> >  		}
> > -		iio_device_get(chans[mapind].indio_dev);
> > -		mapind++;
> > -	}
> > -	if (mapind == 0) {
> > -		ret = -ENODEV;
> > -		goto error_free_chans;
> >  	}
> >  	mutex_unlock(&iio_map_list_lock);
> >  
> [...]
> 
> 

Re: [RFC 10/11] iio: Add OF support

[Lars-Peter Clausen]

On 02/01/2013 03:33 PM, Guenter Roeck wrote:
> On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
>> 013 10:43 PM, Guenter Roeck wrote:
>>> Provide bindings, new API access functions, and parse OF data
>>> during initialization.
>>>
>>
>> Hi Guenter,
>>
>> Thanks for taking care of this.
>>
>> I'd prefer to have only one iio_get_channel which handles both the dt and the
>> non-dt case. Otherwise we'll soon have constructs like
>>
>> if (dev->of_node)
>> 	chan = of_iio_get_channel(dev->of_node, 1);
>> else
>> 	chan = iio_get_channel(dev_name(dev), "vcc");
>>
>> appearing all over the place in drivers code. I'd keep the actual
>> implementation pretty close to what the clk framework does. E.g. take a look at
>> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via the
>> iio_map lookup for the devicetree case, just loop over all IIO devices and
>> match by of_node. This should allow us to simplify the code quite a bit.
>>
> Yes, I just could not figure out how to loop over the list of iio devices. That
> is why I hijacked iio_map which does provide a list.
> 
> Any hints ?

Yes, use bus_find_device on iio_bus_type. A nice example how to use this to
lookup device by of node is of_find_i2c_device_by_node. For IIO you also need
to make sure that dev->type is iio_dev_type, since both devices and triggers
are registered on the same bus.

> 
> Thanks,
> Guenter
> 
>> - Lars
>>
>> On 01/31/2
>>
>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>>>  include/linux/iio/consumer.h                       |    8 +
>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> @@ -0,0 +1,97 @@
>>> +This binding is a work-in-progress, and are based on clock bindings and
>>> +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 more 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 clock 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.
>>> +
>>> +Optional properties:
>>> +io-channel-output-names:
>>> +		    Recommended to be a list of strings of IIO output signal
>>> +		    names indexed by the first cell in the IIO specifier.
>>> +		    However, the meaning of io-channel-output-names is domain
>>> +		    specific to the IIO provider, and is only provided to
>>> +		    encourage using the same meaning for the majority of IIO
>>> +		    providers.  This format may not work for IIO providers
>>> +		    using a complex IIO specifier format.  In those cases it
>>> +		    is recommended to omit this property and create a binding
>>> +		    specific names property.
>>> +
>>> +		    IIO consumer nodes must never directly reference
>>> +		    the provider's io-channel-output-names property.
>>> +
>>> +For example:
>>> +
>>> +    adc: adc@35 {
>>> +	compatible = "maxim,max1139";
>>> +	reg = <0x35>;
>>> +        #io-channel-cells = <1>;
>>> +        io-channel-output-names = "adc1", "adc2";
>>> +    };
>>> +
>>> +- this node defines a device with two named IIO outputs, the first named
>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
>>> +  IIO channels by index. The names should reflect the IIO output signal
>>> +  names for the device.
>>> +
>>> +==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 #clock-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
>>> +		IIO channels from this node. Useful for bus nodes to provide
>>> +		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";
>>> +	};
>>> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
>>> index c42aba6..af80a18 100644
>>> --- a/drivers/iio/inkern.c
>>> +++ b/drivers/iio/inkern.c
>>> @@ -10,6 +10,7 @@
>>>  #include <linux/export.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/mutex.h>
>>> +#include <linux/of.h>
>>>  
>>>  #include <linux/iio/iio.h>
>>>  #include "iio_core.h"
>>> @@ -26,13 +27,55 @@ struct iio_map_internal {
>>>  static LIST_HEAD(iio_map_list);
>>>  static DEFINE_MUTEX(iio_map_list_lock);
>>>  
>>> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
>>> +{
>>
>> To be honest I find this whole function a bit confusing. If we are using
>> devicetree, we shouldn't need to register iio_map. Maybe just skip over
>> io-channel-output-names for now, until we figure out if we really need this and
>> what for.
>>
>>> +	struct device_node *np = idev->dev.of_node;
>>> +	struct iio_map *maps;
>>> +	u32 cells, channels = 1;
>>> +	int names, i, ret;
>>> +
>>> +	ret = of_property_read_u32(np, "#io-channel-cells", &cells);
>>> +	if (ret < 0 || cells > 1)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	names = of_property_count_strings(np, "io-channel-output-names");
>>> +	if (names < 0)
>>> +		names = 0;
>>> +
>>> +	if (channels < names)
>>> +		channels = names;
>>> +
>>> +	maps = devm_kzalloc(&idev->dev, sizeof(*maps) * (channels + 1),
>>> +			    GFP_KERNEL);
>>> +	if (maps == NULL)
>>> +		return ERR_PTR(-ENOMEM);
>>> +
>>> +	for (i = 0; i < channels; i++) {
>>> +		if (i < names) {
>>> +			ret = of_property_read_string_index(np,
>>> +				    "io-channel-output-names", i,
>>> +				    &maps[i].consumer_channel);
>>> +			if (ret < 0)
>>> +				return ERR_PTR(ret);
>>> +		}
>>> +		/* Use dummy for consumer device names */
>>> +		maps[i].consumer_dev_name = "of";
>>> +	}
>>> +	return maps;
>>> +}
>>> +
>>>  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>>>  {
>>>  	int i = 0, ret = 0;
>>>  	struct iio_map_internal *mapi;
>>>  
>>> -	if (maps == NULL)
>>> -		return 0;
>>> +	if (maps == NULL) {
>>> +		maps = iio_map_of_init(indio_dev);
>>> +		if (IS_ERR(maps))
>>> +			return PTR_ERR(maps);
>>> +		if (maps == NULL)
>>> +			return 0;
>>> +	}
>>>  
>>>  	mutex_lock(&iio_map_list_lock);
>>>  	while (maps[i].consumer_dev_name != NULL) {
>>> @@ -92,30 +135,14 @@ static const struct iio_chan_spec
>>>  	return chan;
>>>  }
>>>  
>>> -
>>> -struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>>> +struct iio_channel *iio_channel_get_common(struct iio_map_internal *c,
>>> +					   int index)
>>>  {
>>> -	struct iio_map_internal *c_i = NULL, *c = NULL;
>>>  	struct iio_channel *channel;
>>>  	int err;
>>>  
>>> -	if (name == NULL && channel_name == NULL)
>>> -		return ERR_PTR(-ENODEV);
>>> -
>>> -	/* first find matching entry the channel map */
>>> -	mutex_lock(&iio_map_list_lock);
>>> -	list_for_each_entry(c_i, &iio_map_list, l) {
>>> -		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
>>> -		    (channel_name &&
>>> -		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
>>> -			continue;
>>> -		c = c_i;
>>> -		iio_device_get(c->indio_dev);
>>> -		break;
>>> -	}
>>> -	mutex_unlock(&iio_map_list_lock);
>>>  	if (c == NULL)
>>> -		return ERR_PTR(-ENODEV);
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>>  
>>>  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>>>  	if (channel == NULL) {
>>> @@ -125,7 +152,13 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>>>  
>>>  	channel->indio_dev = c->indio_dev;
>>>  
>>> -	if (c->map->adc_channel_label) {
>>> +	if (index >= 0) {
>>> +		if (index >= c->indio_dev->num_channels) {
>>> +			err = -EINVAL;
>>> +			goto error_no_chan;
>>> +		}
>>> +		channel->channel = &c->indio_dev->channels[index];
>>> +	} else if (c->map->adc_channel_label) {
>>>  		channel->channel =
>>>  			iio_chan_spec_from_name(channel->indio_dev,
>>>  						c->map->adc_channel_label);
>>> @@ -144,6 +177,60 @@ error_no_mem:
>>>  	iio_device_put(c->indio_dev);
>>>  	return ERR_PTR(err);
>>>  }
>>> +
>>> +struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
>>> +{
>>> +	struct iio_map_internal *c_i, *c = NULL;
>>> +	int err, map_index;
>>> +	struct of_phandle_args iiospec;
>>> +
>>> +	if (index < 0)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	err = of_parse_phandle_with_args(np, "io-channels",
>>> +					 "#io-channel-cells",
>>> +					 index, &iiospec);
>>> +	if (err)
>>> +		return ERR_PTR(err);
>>> +
>>> +	map_index = iiospec.args_count ? iiospec.args[0] : 0;
>>> +
>>> +	mutex_lock(&iio_map_list_lock);
>>> +	list_for_each_entry(c_i, &iio_map_list, l) {
>>> +		if (iiospec.np == c_i->indio_dev->dev.of_node) {
>>> +			c = c_i;
>>> +			iio_device_get(c->indio_dev);
>>> +			break;
>>> +		}
>>> +	}
>>> +	of_node_put(iiospec.np);
>>> +	mutex_unlock(&iio_map_list_lock);
>>> +	return iio_channel_get_common(c, index);
>>> +}
>>> +EXPORT_SYMBOL_GPL(of_iio_channel_get);
>>> +
>>> +struct iio_channel *iio_channel_get(const char *name,
>>> +				    const char *channel_name)
>>> +{
>>> +	struct iio_map_internal *c_i = NULL, *c = NULL;
>>> +
>>> +	if (name == NULL && channel_name == NULL)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	/* first find matching entry the channel map */
>>> +	mutex_lock(&iio_map_list_lock);
>>> +	list_for_each_entry(c_i, &iio_map_list, l) {
>>> +		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
>>> +		    (channel_name && strcmp(channel_name,
>>> +					    c_i->map->consumer_channel) != 0))
>>> +			continue;
>>> +		c = c_i;
>>> +		iio_device_get(c->indio_dev);
>>> +		break;
>>> +	}
>>> +	mutex_unlock(&iio_map_list_lock);
>>> +	return iio_channel_get_common(c, -1);
>>> +}
>>>  EXPORT_SYMBOL_GPL(iio_channel_get);
>>>  
>>>  void iio_channel_release(struct iio_channel *channel)
>>> @@ -159,24 +246,43 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>>>  	struct iio_channel *chans;
>>>  	struct iio_map_internal *c = NULL;
>>>  	int nummaps = 0;
>>> -	int mapind = 0;
>>> +	int mapind;
>>>  	int i, ret;
>>>  
>>>  	if (dev == NULL)
>>>  		return ERR_PTR(-EINVAL);
>>> +
>>>  	name = dev_name(dev);
>>>  
>>>  	mutex_lock(&iio_map_list_lock);
>>>  	/* first count the matching maps */
>>> -	list_for_each_entry(c, &iio_map_list, l)
>>> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
>>> -			continue;
>>> -		else
>>> -			nummaps++;
>>> -
>>> -	if (nummaps == 0) {
>>> -		ret = -ENODEV;
>>> -		goto error_ret;
>>> +	if (dev->of_node) {
>>> +		nummaps = 0;
>>> +		do {
>>> +			int ret;
>>> +
>>> +			ret = of_parse_phandle_with_args(dev->of_node,
>>> +							 "io-channels",
>>> +							 "#io-channel-cells",
>>> +							 nummaps, NULL);
>>> +			if (ret < 0)
>>> +				break;
>>> +		} while (++nummaps);
>>> +
>>> +		if (nummaps == 0) {
>>> +			ret = -ENODEV;
>>> +			goto error_ret;
>>> +		}
>>> +	} else {
>>> +		list_for_each_entry(c, &iio_map_list, l)
>>> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
>>> +				continue;
>>> +			else
>>> +				nummaps++;
>>> +		if (nummaps == 0) {
>>> +			ret = -EPROBE_DEFER;
>>> +			goto error_ret;
>>> +		}
>>>  	}
>>>  
>>>  	/* NULL terminated array to save passing size */
>>> @@ -187,24 +293,65 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>>>  	}
>>>  
>>>  	/* for each map fill in the chans element */
>>> -	list_for_each_entry(c, &iio_map_list, l) {
>>> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
>>> -			continue;
>>> -		chans[mapind].indio_dev = c->indio_dev;
>>> -		chans[mapind].data = c->map->consumer_data;
>>> -		chans[mapind].channel =
>>> -			iio_chan_spec_from_name(chans[mapind].indio_dev,
>>> +	if (dev->of_node) {
>>> +		/*
>>> +		 * Device-tree based mapping, search for OF matches.
>>> +		 */
>>> +		for (mapind = 0; mapind < nummaps; mapind++) {
>>> +			int channel;
>>> +			struct of_phandle_args iiospec;
>>> +
>>> +			ret = of_parse_phandle_with_args(dev->of_node,
>>> +							 "io-channels",
>>> +							 "#io-channel-cells",
>>> +							 mapind, &iiospec);
>>> +			if (ret)
>>> +				goto error_free_chans;
>>> +			channel = iiospec.args_count ? iiospec.args[0] : 0;
>>> +			ret = -EPROBE_DEFER;
>>> +			list_for_each_entry(c, &iio_map_list, l) {
>>> +				if (iiospec.np == c->indio_dev->dev.of_node) {
>>> +					ret = 0;
>>> +					break;
>>> +				}
>>> +			}
>>> +			if (ret)
>>> +				goto error_free_chans;
>>> +			if (channel >= c->indio_dev->num_channels) {
>>> +				ret = -EINVAL;
>>> +				goto error_free_chans;
>>> +			}
>>> +			chans[mapind].indio_dev = c->indio_dev;
>>> +			chans[mapind].data = c->map->consumer_data;
>>> +			chans[mapind].channel =
>>> +			  &c->indio_dev->channels[channel];
>>> +			iio_device_get(c->indio_dev);
>>> +		}
>>> +	} else {
>>> +		/*
>>> +		 * Platform data based mapping, search for consumer
>>> +		 * device name.
>>> +		 */
>>> +		mapind = 0;
>>> +		list_for_each_entry(c, &iio_map_list, l) {
>>> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
>>> +				continue;
>>> +			chans[mapind].indio_dev = c->indio_dev;
>>> +			chans[mapind].data = c->map->consumer_data;
>>> +			chans[mapind].channel =
>>> +				iio_chan_spec_from_name(c->indio_dev,
>>>  						c->map->adc_channel_label);
>>> -		if (chans[mapind].channel == NULL) {
>>> -			ret = -EINVAL;
>>> +			if (chans[mapind].channel == NULL) {
>>> +				ret = -EINVAL;
>>> +				goto error_free_chans;
>>> +			}
>>> +			iio_device_get(c->indio_dev);
>>> +			mapind++;
>>> +		}
>>> +		if (mapind < nummaps) {
>>> +			ret = -EPROBE_DEFER;
>>>  			goto error_free_chans;
>>>  		}
>>> -		iio_device_get(chans[mapind].indio_dev);
>>> -		mapind++;
>>> -	}
>>> -	if (mapind == 0) {
>>> -		ret = -ENODEV;
>>> -		goto error_free_chans;
>>>  	}
>>>  	mutex_unlock(&iio_map_list_lock);
>>>  
>> [...]
>>
>>

Re: [RFC 10/11] iio: Add OF support

[Guenter Roeck]

On Fri, Feb 01, 2013 at 03:59:17PM +0100, Lars-Peter Clausen wrote:
> On 02/01/2013 03:33 PM, Guenter Roeck wrote:
> > On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
> >> 013 10:43 PM, Guenter Roeck wrote:
> >>> Provide bindings, new API access functions, and parse OF data
> >>> during initialization.
> >>>
> >>
> >> Hi Guenter,
> >>
> >> Thanks for taking care of this.
> >>
> >> I'd prefer to have only one iio_get_channel which handles both the dt and the
> >> non-dt case. Otherwise we'll soon have constructs like
> >>
> >> if (dev->of_node)
> >> 	chan = of_iio_get_channel(dev->of_node, 1);
> >> else
> >> 	chan = iio_get_channel(dev_name(dev), "vcc");
> >>

Clock code has of_clk_get_by_name(node *, name), clk_get(dev, name), and
of_clk_get(node *, index). Right now of_iio_get_channel matches of_clk_get,
and iio_get_channel matches clk_get.

Question is really how we want to API to look like. I am open to suggestions.

> >> appearing all over the place in drivers code. I'd keep the actual
> >> implementation pretty close to what the clk framework does. E.g. take a look at
> >> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour via the
> >> iio_map lookup for the devicetree case, just loop over all IIO devices and
> >> match by of_node. This should allow us to simplify the code quite a bit.
> >>
> > Yes, I just could not figure out how to loop over the list of iio devices. That
> > is why I hijacked iio_map which does provide a list.
> > 
> > Any hints ?
> 
> Yes, use bus_find_device on iio_bus_type. A nice example how to use this to
> lookup device by of node is of_find_i2c_device_by_node. For IIO you also need
> to make sure that dev->type is iio_dev_type, since both devices and triggers
> are registered on the same bus.
> 
Great, that works nicely, and, yes, the code is now much simpler.

> >>> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
> >>> +{
> >>
> >> To be honest I find this whole function a bit confusing. If we are using
> >> devicetree, we shouldn't need to register iio_map. Maybe just skip over
> >> io-channel-output-names for now, until we figure out if we really need this and
> >> what for.
> >>
Correct, I don't need this anymore after implementing the above.

Thanks,
Guenter

Re: [RFC 10/11] iio: Add OF support

[Lars-Peter Clausen]

On 02/01/2013 08:42 PM, Guenter Roeck wrote:
> On Fri, Feb 01, 2013 at 03:59:17PM +0100, Lars-Peter Clausen wrote:
>> On 02/01/2013 03:33 PM, Guenter Roeck wrote:
>>> On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
>>>> 013 10:43 PM, Guenter Roeck wrote:
>>>>> Provide bindings, new API access functions, and parse OF data
>>>>> during initialization.
>>>>>
>>>>
>>>> Hi Guenter,
>>>>
>>>> Thanks for taking care of this.
>>>>
>>>> I'd prefer to have only one iio_get_channel which handles both the dt and the
>>>> non-dt case. Otherwise we'll soon have constructs like
>>>>
>>>> if (dev->of_node)
>>>> 	chan = of_iio_get_channel(dev->of_node, 1);
>>>> else
>>>> 	chan = iio_get_channel(dev_name(dev), "vcc");
>>>>
>
> Clock code has of_clk_get_by_name(node *, name), clk_get(dev, name), and
> of_clk_get(node *, index). Right now of_iio_get_channel matches of_clk_get,
> and iio_get_channel matches clk_get.
>
> Question is really how we want to API to look like. I am open to suggestions.

I'm not necessarily against having a separate of_iio_get_channe function, but
I'm not sure if we really need it, maybe use it internally as a helper and if
we really need it in some driver make it public and export it. clk_get first
calls of_clk_get_by_name, and only if didn't find a clk it falls back to the
map based lookup. I think iio_get_channel should behave the similar. of_clk_get
is kind of just a helper function for of_clk_get_by_name, not sure if we need
it as a separate function in IIO. If we find out we need it we can probably
still add it later.

- Lars

>
>>>> appearing all over the place in drivers code. I'd keep the actual
>>>> implementation pretty close to what the clk framework does. E.g. take a
look at
>>>> of_clk_get_by_name() and clk_get() in clkdev.c. And don't take a detour
via the
>>>> iio_map lookup for the devicetree case, just loop over all IIO devices and
>>>> match by of_node. This should allow us to simplify the code quite a bit.
>>>>
>>> Yes, I just could not figure out how to loop over the list of iio devices. That
>>> is why I hijacked iio_map which does provide a list.
>>>
>>> Any hints ?
>>
>> Yes, use bus_find_device on iio_bus_type. A nice example how to use this to
>> lookup device by of node is of_find_i2c_device_by_node. For IIO you also need
>> to make sure that dev->type is iio_dev_type, since both devices and triggers
>> are registered on the same bus.
>>
> Great, that works nicely, and, yes, the code is now much simpler.
>
>>>>> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
>>>>> +{
>>>>
>>>> To be honest I find this whole function a bit confusing. If we are using
>>>> devicetree, we shouldn't need to register iio_map. Maybe just skip over
>>>> io-channel-output-names for now, until we figure out if we really need
this and
>>>> what for.
>>>>
> Correct, I don't need this anymore after implementing the above.
>

Re: [RFC 10/11] iio: Add OF support

[Guenter Roeck]

On Sat, Feb 02, 2013 at 03:37:46PM +0100, Lars-Peter Clausen wrote:
> On 02/01/2013 08:42 PM, Guenter Roeck wrote:
> > On Fri, Feb 01, 2013 at 03:59:17PM +0100, Lars-Peter Clausen wrote:
> >> On 02/01/2013 03:33 PM, Guenter Roeck wrote:
> >>> On Fri, Feb 01, 2013 at 12:58:02PM +0100, Lars-Peter Clausen wrote:
> >>>> 013 10:43 PM, Guenter Roeck wrote:
> >>>>> Provide bindings, new API access functions, and parse OF data
> >>>>> during initialization.
> >>>>>
> >>>>
> >>>> Hi Guenter,
> >>>>
> >>>> Thanks for taking care of this.
> >>>>
> >>>> I'd prefer to have only one iio_get_channel which handles both the dt and the
> >>>> non-dt case. Otherwise we'll soon have constructs like
> >>>>
> >>>> if (dev->of_node)
> >>>> 	chan = of_iio_get_channel(dev->of_node, 1);
> >>>> else
> >>>> 	chan = iio_get_channel(dev_name(dev), "vcc");
> >>>>
> >
> > Clock code has of_clk_get_by_name(node *, name), clk_get(dev, name), and
> > of_clk_get(node *, index). Right now of_iio_get_channel matches of_clk_get,
> > and iio_get_channel matches clk_get.
> >
> > Question is really how we want to API to look like. I am open to suggestions.
> 
> I'm not necessarily against having a separate of_iio_get_channe function, but
> I'm not sure if we really need it, maybe use it internally as a helper and if
> we really need it in some driver make it public and export it. clk_get first
> calls of_clk_get_by_name, and only if didn't find a clk it falls back to the
> map based lookup. I think iio_get_channel should behave the similar. of_clk_get
> is kind of just a helper function for of_clk_get_by_name, not sure if we need
> it as a separate function in IIO. If we find out we need it we can probably
> still add it later.
> 
You are right. If I modify iio_get_channel to take the device pointer as
argument, it should work for both OF and non-OF with the approach you outlined
above.

Thanks,
Guenter

Re: [RFC 10/11] iio: Add OF support

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Provide bindings, new API access functions, and parse OF data
> during initialization.
> 
Firstly thanks for working on this Guenter, it's been a big hole
for a while largely because non of our largest developers were
actually using development platforms with device tree support.

Given my knowledge of device tree is based on the odd article
and looking at similar sets of bindings this morning, my comments
are likely to be somewhat superficial and uninformed ;)

Mostly on this one I'll take a back seat and let those who
know this stuff better come to a consensus.

Jonathan

> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>  include/linux/iio/consumer.h                       |    8 +
>  3 files changed, 299 insertions(+), 47 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..0f51c95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,97 @@
> +This binding is a work-in-progress, and are based on clock bindings and
> +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 more 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 clock 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.
> +
> +Optional properties:
> +io-channel-output-names:
> +		    Recommended to be a list of strings of IIO output signal
> +		    names indexed by the first cell in the IIO specifier.
> +		    However, the meaning of io-channel-output-names is domain
> +		    specific to the IIO provider, and is only provided to
> +		    encourage using the same meaning for the majority of IIO
> +		    providers.  This format may not work for IIO providers
> +		    using a complex IIO specifier format.  In those cases it
> +		    is recommended to omit this property and create a binding
> +		    specific names property.
> +
> +		    IIO consumer nodes must never directly reference
> +		    the provider's io-channel-output-names property.
> +
> +For example:
> +
> +    adc: adc@35 {
> +	compatible = "maxim,max1139";
> +	reg = <0x35>;
> +        #io-channel-cells = <1>;
> +        io-channel-output-names = "adc1", "adc2";
> +    };
> +
> +- this node defines a device with two named IIO outputs, the first named
> +  "adc1" and the second named "adc2".  Consumer nodes always reference
> +  IIO channels by index. The names should reflect the IIO output signal
> +  names for the device.
> +
> +==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 #clock-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
> +		IIO channels from this node. Useful for bus nodes to provide
> +		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";
Having different numbers of channels and channel names seems
unusual... Deliberate or you got bored making up channel names?

Why use indexed values for <&adc 0> etc rather than the output
channel names on adc?  For the iio_map stuff we initialy used
indexes but got a lot of responses that it was a silly idea and
naming was much more consistent and easy to follow.

Is there a fundamental reason for it here?

(note I don't mind either way as this seems more compact and cleaner
in some ways)

> +	};
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index c42aba6..af80a18 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -10,6 +10,7 @@
>  #include <linux/export.h>
>  #include <linux/slab.h>
>  #include <linux/mutex.h>
> +#include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
>  #include "iio_core.h"
> @@ -26,13 +27,55 @@ struct iio_map_internal {
>  static LIST_HEAD(iio_map_list);
>  static DEFINE_MUTEX(iio_map_list_lock);
>  
> +static struct iio_map *iio_map_of_init(struct iio_dev *idev)
> +{
> +	struct device_node *np = idev->dev.of_node;
> +	struct iio_map *maps;
> +	u32 cells, channels = 1;
> +	int names, i, ret;
> +
> +	ret = of_property_read_u32(np, "#io-channel-cells", &cells);
> +	if (ret < 0 || cells > 1)
> +		return ERR_PTR(-EINVAL);
> +
> +	names = of_property_count_strings(np, "io-channel-output-names");
> +	if (names < 0)
> +		names = 0;
> +
> +	if (channels < names)
> +		channels = names;
> +
> +	maps = devm_kzalloc(&idev->dev, sizeof(*maps) * (channels + 1),
> +			    GFP_KERNEL);
> +	if (maps == NULL)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < channels; i++) {
> +		if (i < names) {
> +			ret = of_property_read_string_index(np,
> +				    "io-channel-output-names", i,
> +				    &maps[i].consumer_channel);
> +			if (ret < 0)
> +				return ERR_PTR(ret);
> +		}
> +		/* Use dummy for consumer device names */
> +		maps[i].consumer_dev_name = "of";
> +	}
> +	return maps;
> +}
> +
>  int iio_map_array_register(struct iio_dev *indio_dev, struct iio_map *maps)
>  {
>  	int i = 0, ret = 0;
>  	struct iio_map_internal *mapi;
>  
> -	if (maps == NULL)
> -		return 0;
> +	if (maps == NULL) {
> +		maps = iio_map_of_init(indio_dev);
> +		if (IS_ERR(maps))
> +			return PTR_ERR(maps);
> +		if (maps == NULL)
> +			return 0;
> +	}
>  
>  	mutex_lock(&iio_map_list_lock);
>  	while (maps[i].consumer_dev_name != NULL) {
> @@ -92,30 +135,14 @@ static const struct iio_chan_spec
>  	return chan;
>  }
>  
> -
> -struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
> +struct iio_channel *iio_channel_get_common(struct iio_map_internal *c,
> +					   int index)
>  {
> -	struct iio_map_internal *c_i = NULL, *c = NULL;
>  	struct iio_channel *channel;
>  	int err;
>  
> -	if (name == NULL && channel_name == NULL)
> -		return ERR_PTR(-ENODEV);
> -
> -	/* first find matching entry the channel map */
> -	mutex_lock(&iio_map_list_lock);
> -	list_for_each_entry(c_i, &iio_map_list, l) {
> -		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> -		    (channel_name &&
> -		     strcmp(channel_name, c_i->map->consumer_channel) != 0))
> -			continue;
> -		c = c_i;
> -		iio_device_get(c->indio_dev);
> -		break;
> -	}
> -	mutex_unlock(&iio_map_list_lock);
>  	if (c == NULL)
> -		return ERR_PTR(-ENODEV);
> +		return ERR_PTR(-EPROBE_DEFER);
>  
>  	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>  	if (channel == NULL) {
> @@ -125,7 +152,13 @@ struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
>  
>  	channel->indio_dev = c->indio_dev;
>  
> -	if (c->map->adc_channel_label) {
> +	if (index >= 0) {
> +		if (index >= c->indio_dev->num_channels) {
> +			err = -EINVAL;
> +			goto error_no_chan;
> +		}
> +		channel->channel = &c->indio_dev->channels[index];
> +	} else if (c->map->adc_channel_label) {
>  		channel->channel =
>  			iio_chan_spec_from_name(channel->indio_dev,
>  						c->map->adc_channel_label);
> @@ -144,6 +177,60 @@ error_no_mem:
>  	iio_device_put(c->indio_dev);
>  	return ERR_PTR(err);
>  }
> +
> +struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
> +{
> +	struct iio_map_internal *c_i, *c = NULL;
> +	int err, map_index;
> +	struct of_phandle_args iiospec;
> +
> +	if (index < 0)
> +		return ERR_PTR(-EINVAL);
> +
> +	err = of_parse_phandle_with_args(np, "io-channels",
> +					 "#io-channel-cells",
> +					 index, &iiospec);
> +	if (err)
> +		return ERR_PTR(err);
> +
> +	map_index = iiospec.args_count ? iiospec.args[0] : 0;
> +
> +	mutex_lock(&iio_map_list_lock);
> +	list_for_each_entry(c_i, &iio_map_list, l) {
> +		if (iiospec.np == c_i->indio_dev->dev.of_node) {
> +			c = c_i;
> +			iio_device_get(c->indio_dev);
> +			break;
> +		}
> +	}
> +	of_node_put(iiospec.np);
> +	mutex_unlock(&iio_map_list_lock);
> +	return iio_channel_get_common(c, index);
> +}
> +EXPORT_SYMBOL_GPL(of_iio_channel_get);
> +
> +struct iio_channel *iio_channel_get(const char *name,
> +				    const char *channel_name)
> +{
> +	struct iio_map_internal *c_i = NULL, *c = NULL;
> +
> +	if (name == NULL && channel_name == NULL)
> +		return ERR_PTR(-ENODEV);
> +
> +	/* first find matching entry the channel map */
> +	mutex_lock(&iio_map_list_lock);
> +	list_for_each_entry(c_i, &iio_map_list, l) {
> +		if ((name && strcmp(name, c_i->map->consumer_dev_name) != 0) ||
> +		    (channel_name && strcmp(channel_name,
> +					    c_i->map->consumer_channel) != 0))
> +			continue;
> +		c = c_i;
> +		iio_device_get(c->indio_dev);
> +		break;
> +	}
> +	mutex_unlock(&iio_map_list_lock);
> +	return iio_channel_get_common(c, -1);
> +}
>  EXPORT_SYMBOL_GPL(iio_channel_get);
>  
>  void iio_channel_release(struct iio_channel *channel)
> @@ -159,24 +246,43 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  	struct iio_channel *chans;
>  	struct iio_map_internal *c = NULL;
>  	int nummaps = 0;
> -	int mapind = 0;
> +	int mapind;
>  	int i, ret;
>  
>  	if (dev == NULL)
>  		return ERR_PTR(-EINVAL);
> +
>  	name = dev_name(dev);
>  
>  	mutex_lock(&iio_map_list_lock);
>  	/* first count the matching maps */
> -	list_for_each_entry(c, &iio_map_list, l)
> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> -			continue;
> -		else
> -			nummaps++;
> -
> -	if (nummaps == 0) {
> -		ret = -ENODEV;
> -		goto error_ret;
> +	if (dev->of_node) {
> +		nummaps = 0;
> +		do {
> +			int ret;
> +
> +			ret = of_parse_phandle_with_args(dev->of_node,
> +							 "io-channels",
> +							 "#io-channel-cells",
> +							 nummaps, NULL);
> +			if (ret < 0)
> +				break;
> +		} while (++nummaps);
> +
> +		if (nummaps == 0) {
> +			ret = -ENODEV;
> +			goto error_ret;
> +		}
> +	} else {
> +		list_for_each_entry(c, &iio_map_list, l)
> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> +				continue;
> +			else
> +				nummaps++;
> +		if (nummaps == 0) {
> +			ret = -EPROBE_DEFER;
> +			goto error_ret;
> +		}
>  	}
>  
>  	/* NULL terminated array to save passing size */
> @@ -187,24 +293,65 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  	}
>  
>  	/* for each map fill in the chans element */
> -	list_for_each_entry(c, &iio_map_list, l) {
> -		if (name && strcmp(name, c->map->consumer_dev_name) != 0)
> -			continue;
> -		chans[mapind].indio_dev = c->indio_dev;
> -		chans[mapind].data = c->map->consumer_data;
> -		chans[mapind].channel =
> -			iio_chan_spec_from_name(chans[mapind].indio_dev,
> +	if (dev->of_node) {
> +		/*
> +		 * Device-tree based mapping, search for OF matches.
> +		 */
> +		for (mapind = 0; mapind < nummaps; mapind++) {
> +			int channel;
> +			struct of_phandle_args iiospec;
> +
> +			ret = of_parse_phandle_with_args(dev->of_node,
> +							 "io-channels",
> +							 "#io-channel-cells",
> +							 mapind, &iiospec);
> +			if (ret)
> +				goto error_free_chans;
> +			channel = iiospec.args_count ? iiospec.args[0] : 0;
> +			ret = -EPROBE_DEFER;
> +			list_for_each_entry(c, &iio_map_list, l) {
> +				if (iiospec.np == c->indio_dev->dev.of_node) {
> +					ret = 0;
> +					break;
> +				}
> +			}
> +			if (ret)
> +				goto error_free_chans;
> +			if (channel >= c->indio_dev->num_channels) {
> +				ret = -EINVAL;
> +				goto error_free_chans;
> +			}
> +			chans[mapind].indio_dev = c->indio_dev;
> +			chans[mapind].data = c->map->consumer_data;
> +			chans[mapind].channel =
> +			  &c->indio_dev->channels[channel];
> +			iio_device_get(c->indio_dev);
> +		}
> +	} else {
> +		/*
> +		 * Platform data based mapping, search for consumer
> +		 * device name.
> +		 */
> +		mapind = 0;
> +		list_for_each_entry(c, &iio_map_list, l) {
> +			if (strcmp(name, c->map->consumer_dev_name) != 0)
> +				continue;
> +			chans[mapind].indio_dev = c->indio_dev;
> +			chans[mapind].data = c->map->consumer_data;
> +			chans[mapind].channel =
> +				iio_chan_spec_from_name(c->indio_dev,
>  						c->map->adc_channel_label);
> -		if (chans[mapind].channel == NULL) {
> -			ret = -EINVAL;
> +			if (chans[mapind].channel == NULL) {
> +				ret = -EINVAL;
> +				goto error_free_chans;
> +			}
> +			iio_device_get(c->indio_dev);
> +			mapind++;
> +		}
> +		if (mapind < nummaps) {
> +			ret = -EPROBE_DEFER;
>  			goto error_free_chans;
>  		}
> -		iio_device_get(chans[mapind].indio_dev);
> -		mapind++;
> -	}
> -	if (mapind == 0) {
> -		ret = -ENODEV;
> -		goto error_free_chans;
>  	}
>  	mutex_unlock(&iio_map_list_lock);
>  
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 6c44167..a347c15 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -41,6 +41,14 @@ struct iio_channel *iio_channel_get(const char *name,
>  				    const char *consumer_channel);
>  
>  /**
> + * of_iio_channel_get() - get description of all that is needed to access
> + *			channel from OF node pointer.
> + * @np:			Consumer's OF node pointer.
> + * @index:		Index pointing to the io-channels entry in np.
> + */
> +struct iio_channel *of_iio_channel_get(struct device_node *np, int index);
> +
> +/**
>   * iio_channel_release() - release channels obtained via iio_channel_get
>   * @chan:		The channel to be released.
>   */
> 

Re: [RFC 10/11] iio: Add OF support

[Guenter Roeck]

On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> > Provide bindings, new API access functions, and parse OF data
> > during initialization.
> > 
> Firstly thanks for working on this Guenter, it's been a big hole
> for a while largely because non of our largest developers were
> actually using development platforms with device tree support.
> 
> Given my knowledge of device tree is based on the odd article
> and looking at similar sets of bindings this morning, my comments
> are likely to be somewhat superficial and uninformed ;)
> 
> Mostly on this one I'll take a back seat and let those who
> know this stuff better come to a consensus.
> 
> Jonathan
> 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > ---
> >  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
> >  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
> >  include/linux/iio/consumer.h                       |    8 +
> >  3 files changed, 299 insertions(+), 47 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..0f51c95
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -0,0 +1,97 @@
> > +This binding is a work-in-progress, and are based on clock bindings and
> > +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 more 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 clock 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.
> > +
> > +Optional properties:
> > +io-channel-output-names:
> > +		    Recommended to be a list of strings of IIO output signal
> > +		    names indexed by the first cell in the IIO specifier.
> > +		    However, the meaning of io-channel-output-names is domain
> > +		    specific to the IIO provider, and is only provided to
> > +		    encourage using the same meaning for the majority of IIO
> > +		    providers.  This format may not work for IIO providers
> > +		    using a complex IIO specifier format.  In those cases it
> > +		    is recommended to omit this property and create a binding
> > +		    specific names property.
> > +
> > +		    IIO consumer nodes must never directly reference
> > +		    the provider's io-channel-output-names property.
> > +
> > +For example:
> > +
> > +    adc: adc@35 {
> > +	compatible = "maxim,max1139";
> > +	reg = <0x35>;
> > +        #io-channel-cells = <1>;
> > +        io-channel-output-names = "adc1", "adc2";
> > +    };
> > +
> > +- this node defines a device with two named IIO outputs, the first named
> > +  "adc1" and the second named "adc2".  Consumer nodes always reference
> > +  IIO channels by index. The names should reflect the IIO output signal
> > +  names for the device.
> > +
> > +==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 #clock-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
> > +		IIO channels from this node. Useful for bus nodes to provide
> > +		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";
> Having different numbers of channels and channel names seems
> unusual... Deliberate or you got bored making up channel names?
> 
> Why use indexed values for <&adc 0> etc rather than the output
> channel names on adc?  For the iio_map stuff we initialy used
> indexes but got a lot of responses that it was a silly idea and
> naming was much more consistent and easy to follow.
> 
> Is there a fundamental reason for it here?
> 
> (note I don't mind either way as this seems more compact and cleaner
> in some ways)
> 

It follows the structure used by clocks, which uses the provided name(s) to
calculate an index into io-channels. This way, the provider does not have to
provide the mapping, the consumer does not have to know the io-channel index,
and the consumer code can call something like

	channel = iio_get_channel(dev, "vcc");

In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".

This works for both platform data and OF data (though platform data will
still need provider-based mapping, at least for now).

This lets the code use a static name (eg "vcc"), and the mapping to the actual
provider happens through devicetree. Since the name is only used locally and
consumer driver specific, there is no need to define globally unique names.

With this approach, the io channel map is not needed at all for the OF case.
I had used it in this version of the patch set, but got rid of it now.

Actually, provider based mapping doesn't even work. If the consumer is
instantiated before the provider, the mapping doesn't exist yet, and the
call to iio_channel_get_all will fail. There is no way to prevent this,
as providers can come online at any time and there is no means to enforce that
all providers are already active by the time the consumers are instantiated.
Even if a mapping exists, there is no way to know if it is complete, if a
consumer is mapped to multiple providers.

With the consumer based mapping, iio_channel_get_all 'knows' that not all
requested providers are available and can return -EPROBEDEFER in that case.

As a side effect, we can also use the names - if provided - as channel
labels in iio_hwmon.

Note this will require the iio_get_channel API to change from taking the
consumer device name to taking the consumer device pointer as argument.
This will enable it to work for both OF and non-OF cases, should address Lars'
concerns about duplicate API functions, and synchronize the code to match how
the clock framework works.

Thanks,
Guenter

Re: [RFC 10/11] iio: Add OF support

[Jonathan Cameron]

On 02/02/2013 04:10 PM, Guenter Roeck wrote:
> On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>> Provide bindings, new API access functions, and parse OF data
>>> during initialization.
>>>
>> Firstly thanks for working on this Guenter, it's been a big hole
>> for a while largely because non of our largest developers were
>> actually using development platforms with device tree support.
>>
>> Given my knowledge of device tree is based on the odd article
>> and looking at similar sets of bindings this morning, my comments
>> are likely to be somewhat superficial and uninformed ;)
>>
>> Mostly on this one I'll take a back seat and let those who
>> know this stuff better come to a consensus.
>>
>> Jonathan
>>
>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>>>  include/linux/iio/consumer.h                       |    8 +
>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> @@ -0,0 +1,97 @@
>>> +This binding is a work-in-progress, and are based on clock bindings and
>>> +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 more 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 clock 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.
>>> +
>>> +Optional properties:
>>> +io-channel-output-names:
>>> +		    Recommended to be a list of strings of IIO output signal
>>> +		    names indexed by the first cell in the IIO specifier.
>>> +		    However, the meaning of io-channel-output-names is domain
>>> +		    specific to the IIO provider, and is only provided to
>>> +		    encourage using the same meaning for the majority of IIO
>>> +		    providers.  This format may not work for IIO providers
>>> +		    using a complex IIO specifier format.  In those cases it
>>> +		    is recommended to omit this property and create a binding
>>> +		    specific names property.
>>> +
>>> +		    IIO consumer nodes must never directly reference
>>> +		    the provider's io-channel-output-names property.
>>> +
>>> +For example:
>>> +
>>> +    adc: adc@35 {
>>> +	compatible = "maxim,max1139";
>>> +	reg = <0x35>;
>>> +        #io-channel-cells = <1>;
>>> +        io-channel-output-names = "adc1", "adc2";
>>> +    };
>>> +
>>> +- this node defines a device with two named IIO outputs, the first named
>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
>>> +  IIO channels by index. The names should reflect the IIO output signal
>>> +  names for the device.
>>> +
>>> +==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 #clock-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
>>> +		IIO channels from this node. Useful for bus nodes to provide
>>> +		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";
>> Having different numbers of channels and channel names seems
>> unusual... Deliberate or you got bored making up channel names?
>>
>> Why use indexed values for <&adc 0> etc rather than the output
>> channel names on adc?  For the iio_map stuff we initialy used
>> indexes but got a lot of responses that it was a silly idea and
>> naming was much more consistent and easy to follow.
>>
>> Is there a fundamental reason for it here?
>>
>> (note I don't mind either way as this seems more compact and cleaner
>> in some ways)
>>
> 
> It follows the structure used by clocks, which uses the provided name(s) to
> calculate an index into io-channels. This way, the provider does not have to
> provide the mapping, the consumer does not have to know the io-channel index,
> and the consumer code can call something like
> 
> 	channel = iio_get_channel(dev, "vcc");
> 
> In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".
> 
> This works for both platform data and OF data (though platform data will
> still need provider-based mapping, at least for now).
> 
> This lets the code use a static name (eg "vcc"), and the mapping to the actual
> provider happens through devicetree. Since the name is only used locally and
> consumer driver specific, there is no need to define globally unique names.
> 
> With this approach, the io channel map is not needed at all for the OF case.
> I had used it in this version of the patch set, but got rid of it now.
> 
> Actually, provider based mapping doesn't even work. If the consumer is
> instantiated before the provider, the mapping doesn't exist yet, and the
> call to iio_channel_get_all will fail. There is no way to prevent this,
> as providers can come online at any time and there is no means to enforce that
> all providers are already active by the time the consumers are instantiated.
> Even if a mapping exists, there is no way to know if it is complete, if a
> consumer is mapped to multiple providers.
> 
> With the consumer based mapping, iio_channel_get_all 'knows' that not all
> requested providers are available and can return -EPROBEDEFER in that case.
Thanks. That makes sense.  At the moment iio_hwmon is the only case that
does a 'get all'. Clearly things are easier when the driver is requesting a
specific set and we can do the back off much more easily.

> 
> As a side effect, we can also use the names - if provided - as channel
> labels in iio_hwmon.
> 
> Note this will require the iio_get_channel API to change from taking the
> consumer device name to taking the consumer device pointer as argument.
> This will enable it to work for both OF and non-OF cases, should address Lars'
> concerns about duplicate API functions, and synchronize the code to match how
> the clock framework works.

Agreed, doing this gives us a cleaner syntax as well.  Note there are other
users of that function in tree so be sure to get them all!

> 
> Thanks,
> Guenter
Thanks for the explanation.  What I was actually suggesting was something
like:

adc: max1139@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		#io-channel-cells = <1>;
		io-channel-output-names = "adc1", "adc2", "adc3"				
	};

iio_hwmon {
	compatible = "iio-hwmon";
	io-channels = <&adc "adc1">, <&adc "adc2">, <&adc "adc3">,
	io-channel-names = "vcc", "vdd", "vref";
}

Having taken a look at the available syntax, those <> pairs have
to be unsigned integers?  Hence the additional level of indirection?

(sorry, I'm getting you to give me a tutorial on device tree syntax rather
than the actual issue here!)

I guess it was desirable to keep the syntax relatively simple but that occasionally
adds the requirement for a bit of indirection.



Jonathan

> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 10/11] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
>> On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>>> Provide bindings, new API access functions, and parse OF data
>>>> during initialization.
>>>>
>>> Firstly thanks for working on this Guenter, it's been a big hole
>>> for a while largely because non of our largest developers were
>>> actually using development platforms with device tree support.
>>>
>>> Given my knowledge of device tree is based on the odd article
>>> and looking at similar sets of bindings this morning, my comments
>>> are likely to be somewhat superficial and uninformed ;)
>>>
>>> Mostly on this one I'll take a back seat and let those who
>>> know this stuff better come to a consensus.
>>>
>>> Jonathan
>>>
>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>> ---
>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>>>>  include/linux/iio/consumer.h                       |    8 +
>>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>> @@ -0,0 +1,97 @@
>>>> +This binding is a work-in-progress, and are based on clock bindings and
>>>> +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 more 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 clock 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.
>>>> +
>>>> +Optional properties:
>>>> +io-channel-output-names:
>>>> +		    Recommended to be a list of strings of IIO output signal
>>>> +		    names indexed by the first cell in the IIO specifier.
>>>> +		    However, the meaning of io-channel-output-names is domain
>>>> +		    specific to the IIO provider, and is only provided to
>>>> +		    encourage using the same meaning for the majority of IIO
>>>> +		    providers.  This format may not work for IIO providers
>>>> +		    using a complex IIO specifier format.  In those cases it
>>>> +		    is recommended to omit this property and create a binding
>>>> +		    specific names property.
>>>> +
>>>> +		    IIO consumer nodes must never directly reference
>>>> +		    the provider's io-channel-output-names property.
>>>> +
>>>> +For example:
>>>> +
>>>> +    adc: adc@35 {
>>>> +	compatible = "maxim,max1139";
>>>> +	reg = <0x35>;
>>>> +        #io-channel-cells = <1>;
>>>> +        io-channel-output-names = "adc1", "adc2";
>>>> +    };
>>>> +
>>>> +- this node defines a device with two named IIO outputs, the first named
>>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
>>>> +  IIO channels by index. The names should reflect the IIO output signal
>>>> +  names for the device.
>>>> +
>>>> +==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 #clock-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
>>>> +		IIO channels from this node. Useful for bus nodes to provide
>>>> +		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";
>>> Having different numbers of channels and channel names seems
>>> unusual... Deliberate or you got bored making up channel names?
>>>
>>> Why use indexed values for <&adc 0> etc rather than the output
>>> channel names on adc?  For the iio_map stuff we initialy used
>>> indexes but got a lot of responses that it was a silly idea and
>>> naming was much more consistent and easy to follow.
>>>
>>> Is there a fundamental reason for it here?
>>>
>>> (note I don't mind either way as this seems more compact and cleaner
>>> in some ways)
>>>
>>
>> It follows the structure used by clocks, which uses the provided name(s) to
>> calculate an index into io-channels. This way, the provider does not have to
>> provide the mapping, the consumer does not have to know the io-channel index,
>> and the consumer code can call something like
>>
>> 	channel = iio_get_channel(dev, "vcc");
>>
>> In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".
>>
>> This works for both platform data and OF data (though platform data will
>> still need provider-based mapping, at least for now).
>>
>> This lets the code use a static name (eg "vcc"), and the mapping to the actual
>> provider happens through devicetree. Since the name is only used locally and
>> consumer driver specific, there is no need to define globally unique names.
>>
>> With this approach, the io channel map is not needed at all for the OF case.
>> I had used it in this version of the patch set, but got rid of it now.
>>
>> Actually, provider based mapping doesn't even work. If the consumer is
>> instantiated before the provider, the mapping doesn't exist yet, and the
>> call to iio_channel_get_all will fail. There is no way to prevent this,
>> as providers can come online at any time and there is no means to enforce that
>> all providers are already active by the time the consumers are instantiated.
>> Even if a mapping exists, there is no way to know if it is complete, if a
>> consumer is mapped to multiple providers.
>>
>> With the consumer based mapping, iio_channel_get_all 'knows' that not all
>> requested providers are available and can return -EPROBEDEFER in that case.
> Thanks. That makes sense.  At the moment iio_hwmon is the only case that
> does a 'get all'. Clearly things are easier when the driver is requesting a
> specific set and we can do the back off much more easily.
> 
>>
>> As a side effect, we can also use the names - if provided - as channel
>> labels in iio_hwmon.
>>
>> Note this will require the iio_get_channel API to change from taking the
>> consumer device name to taking the consumer device pointer as argument.
>> This will enable it to work for both OF and non-OF cases, should address Lars'
>> concerns about duplicate API functions, and synchronize the code to match how
>> the clock framework works.
> 
> Agreed, doing this gives us a cleaner syntax as well.  Note there are other
> users of that function in tree so be sure to get them all!
> 
>>
>> Thanks,
>> Guenter
> Thanks for the explanation.  What I was actually suggesting was something
> like:
> 
> adc: max1139@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		#io-channel-cells = <1>;
> 		io-channel-output-names = "adc1", "adc2", "adc3"				
> 	};
> 
> iio_hwmon {
> 	compatible = "iio-hwmon";
> 	io-channels = <&adc "adc1">, <&adc "adc2">, <&adc "adc3">,
> 	io-channel-names = "vcc", "vdd", "vref";
> }
> 
> Having taken a look at the available syntax, those <> pairs have
> to be unsigned integers?  Hence the additional level of indirection?

Yea, I think mixing phandles and strings simply doesn't work, due how
devicetree stores things.

> 
> (sorry, I'm getting you to give me a tutorial on device tree syntax rather
> than the actual issue here!)
> 
> I guess it was desirable to keep the syntax relatively simple but that occasionally
> adds the requirement for a bit of indirection.
> 
> 
> 
> Jonathan

Re: [RFC 10/11] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 12:47 PM, Lars-Peter Clausen wrote:
> On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
>> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
>>> On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
>>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>>>> Provide bindings, new API access functions, and parse OF data
>>>>> during initialization.
>>>>>
>>>> Firstly thanks for working on this Guenter, it's been a big hole
>>>> for a while largely because non of our largest developers were
>>>> actually using development platforms with device tree support.
>>>>
>>>> Given my knowledge of device tree is based on the odd article
>>>> and looking at similar sets of bindings this morning, my comments
>>>> are likely to be somewhat superficial and uninformed ;)
>>>>
>>>> Mostly on this one I'll take a back seat and let those who
>>>> know this stuff better come to a consensus.
>>>>
>>>> Jonathan
>>>>
>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>> ---
>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>>>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>>>>>  include/linux/iio/consumer.h                       |    8 +
>>>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,97 @@
>>>>> +This binding is a work-in-progress, and are based on clock bindings and
>>>>> +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 more 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 clock 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.
>>>>> +
>>>>> +Optional properties:
>>>>> +io-channel-output-names:
>>>>> +		    Recommended to be a list of strings of IIO output signal
>>>>> +		    names indexed by the first cell in the IIO specifier.
>>>>> +		    However, the meaning of io-channel-output-names is domain
>>>>> +		    specific to the IIO provider, and is only provided to
>>>>> +		    encourage using the same meaning for the majority of IIO
>>>>> +		    providers.  This format may not work for IIO providers
>>>>> +		    using a complex IIO specifier format.  In those cases it
>>>>> +		    is recommended to omit this property and create a binding
>>>>> +		    specific names property.
>>>>> +
>>>>> +		    IIO consumer nodes must never directly reference
>>>>> +		    the provider's io-channel-output-names property.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +    adc: adc@35 {
>>>>> +	compatible = "maxim,max1139";
>>>>> +	reg = <0x35>;
>>>>> +        #io-channel-cells = <1>;
>>>>> +        io-channel-output-names = "adc1", "adc2";
>>>>> +    };
>>>>> +
>>>>> +- this node defines a device with two named IIO outputs, the first named
>>>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
>>>>> +  IIO channels by index. The names should reflect the IIO output signal
>>>>> +  names for the device.
>>>>> +
>>>>> +==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 #clock-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
>>>>> +		IIO channels from this node. Useful for bus nodes to provide
>>>>> +		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";
>>>> Having different numbers of channels and channel names seems
>>>> unusual... Deliberate or you got bored making up channel names?
>>>>
>>>> Why use indexed values for <&adc 0> etc rather than the output
>>>> channel names on adc?  For the iio_map stuff we initialy used
>>>> indexes but got a lot of responses that it was a silly idea and
>>>> naming was much more consistent and easy to follow.
>>>>
>>>> Is there a fundamental reason for it here?
>>>>
>>>> (note I don't mind either way as this seems more compact and cleaner
>>>> in some ways)
>>>>
>>>
>>> It follows the structure used by clocks, which uses the provided name(s) to
>>> calculate an index into io-channels. This way, the provider does not have to
>>> provide the mapping, the consumer does not have to know the io-channel index,
>>> and the consumer code can call something like
>>>
>>> 	channel = iio_get_channel(dev, "vcc");
>>>
>>> In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".
>>>
>>> This works for both platform data and OF data (though platform data will
>>> still need provider-based mapping, at least for now).
>>>
>>> This lets the code use a static name (eg "vcc"), and the mapping to the actual
>>> provider happens through devicetree. Since the name is only used locally and
>>> consumer driver specific, there is no need to define globally unique names.
>>>
>>> With this approach, the io channel map is not needed at all for the OF case.
>>> I had used it in this version of the patch set, but got rid of it now.
>>>
>>> Actually, provider based mapping doesn't even work. If the consumer is
>>> instantiated before the provider, the mapping doesn't exist yet, and the
>>> call to iio_channel_get_all will fail. There is no way to prevent this,
>>> as providers can come online at any time and there is no means to enforce that
>>> all providers are already active by the time the consumers are instantiated.
>>> Even if a mapping exists, there is no way to know if it is complete, if a
>>> consumer is mapped to multiple providers.
>>>
>>> With the consumer based mapping, iio_channel_get_all 'knows' that not all
>>> requested providers are available and can return -EPROBEDEFER in that case.
>> Thanks. That makes sense.  At the moment iio_hwmon is the only case that
>> does a 'get all'. Clearly things are easier when the driver is requesting a
>> specific set and we can do the back off much more easily.
>>
>>>
>>> As a side effect, we can also use the names - if provided - as channel
>>> labels in iio_hwmon.
>>>
>>> Note this will require the iio_get_channel API to change from taking the
>>> consumer device name to taking the consumer device pointer as argument.
>>> This will enable it to work for both OF and non-OF cases, should address Lars'
>>> concerns about duplicate API functions, and synchronize the code to match how
>>> the clock framework works.
>>
>> Agreed, doing this gives us a cleaner syntax as well.  Note there are other
>> users of that function in tree so be sure to get them all!
>>
>>>
>>> Thanks,
>>> Guenter
>> Thanks for the explanation.  What I was actually suggesting was something
>> like:
>>
>> adc: max1139@35 {
>> 		compatible = "maxim,max1139";
>> 		reg = <0x35>;
>> 		#io-channel-cells = <1>;
>> 		io-channel-output-names = "adc1", "adc2", "adc3"				
>> 	};
>>
>> iio_hwmon {
>> 	compatible = "iio-hwmon";
>> 	io-channels = <&adc "adc1">, <&adc "adc2">, <&adc "adc3">,
>> 	io-channel-names = "vcc", "vdd", "vref";
>> }
>>
>> Having taken a look at the available syntax, those <> pairs have
>> to be unsigned integers?  Hence the additional level of indirection?
> 
> Yea, I think mixing phandles and strings simply doesn't work, due how
> devicetree stores things.
> 

Another possibility would beto do things the way the regulator framework does
an have each channel as a subnode to the converter devices eg.


adc: max1139@35 {
 		compatible = "maxim,max1139";
 		reg = <0x35>;
		adc0: adc@0 {
			reg = <0>;
		};
		adc1: adc@1 {
			reg = <1>;
		};
		adc2: adc@2 {
			reg = <2>;
		};

 	};


iio_hwmon {
 	compatible = "iio-hwmon";
 	io-channels = <&adc0>, <&adc1>, <&adc2>;
 	io-channel-names = "vcc", "vdd", "vref";
};

But I'm not sure how much sense this makes for IIO.

- Lars

>>
>> (sorry, I'm getting you to give me a tutorial on device tree syntax rather
>> than the actual issue here!)
>>
>> I guess it was desirable to keep the syntax relatively simple but that occasionally
>> adds the requirement for a bit of indirection.
>>
>>
>>
>> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 10/11] iio: Add OF support

[Jonathan Cameron]

On 02/03/2013 11:52 AM, Lars-Peter Clausen wrote:
> On 02/03/2013 12:47 PM, Lars-Peter Clausen wrote:
>> On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
>>> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
>>>> On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
>>>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
>>>>>> Provide bindings, new API access functions, and parse OF data
>>>>>> during initialization.
>>>>>>
>>>>> Firstly thanks for working on this Guenter, it's been a big hole
>>>>> for a while largely because non of our largest developers were
>>>>> actually using development platforms with device tree support.
>>>>>
>>>>> Given my knowledge of device tree is based on the odd article
>>>>> and looking at similar sets of bindings this morning, my comments
>>>>> are likely to be somewhat superficial and uninformed ;)
>>>>>
>>>>> Mostly on this one I'll take a back seat and let those who
>>>>> know this stuff better come to a consensus.
>>>>>
>>>>> Jonathan
>>>>>
>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>>> ---
>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
>>>>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
>>>>>>  include/linux/iio/consumer.h                       |    8 +
>>>>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>> @@ -0,0 +1,97 @@
>>>>>> +This binding is a work-in-progress, and are based on clock bindings and
>>>>>> +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 more 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 clock 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.
>>>>>> +
>>>>>> +Optional properties:
>>>>>> +io-channel-output-names:
>>>>>> +		    Recommended to be a list of strings of IIO output signal
>>>>>> +		    names indexed by the first cell in the IIO specifier.
>>>>>> +		    However, the meaning of io-channel-output-names is domain
>>>>>> +		    specific to the IIO provider, and is only provided to
>>>>>> +		    encourage using the same meaning for the majority of IIO
>>>>>> +		    providers.  This format may not work for IIO providers
>>>>>> +		    using a complex IIO specifier format.  In those cases it
>>>>>> +		    is recommended to omit this property and create a binding
>>>>>> +		    specific names property.
>>>>>> +
>>>>>> +		    IIO consumer nodes must never directly reference
>>>>>> +		    the provider's io-channel-output-names property.
>>>>>> +
>>>>>> +For example:
>>>>>> +
>>>>>> +    adc: adc@35 {
>>>>>> +	compatible = "maxim,max1139";
>>>>>> +	reg = <0x35>;
>>>>>> +        #io-channel-cells = <1>;
>>>>>> +        io-channel-output-names = "adc1", "adc2";
>>>>>> +    };
>>>>>> +
>>>>>> +- this node defines a device with two named IIO outputs, the first named
>>>>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
>>>>>> +  IIO channels by index. The names should reflect the IIO output signal
>>>>>> +  names for the device.
>>>>>> +
>>>>>> +==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 #clock-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
>>>>>> +		IIO channels from this node. Useful for bus nodes to provide
>>>>>> +		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";
>>>>> Having different numbers of channels and channel names seems
>>>>> unusual... Deliberate or you got bored making up channel names?
>>>>>
>>>>> Why use indexed values for <&adc 0> etc rather than the output
>>>>> channel names on adc?  For the iio_map stuff we initialy used
>>>>> indexes but got a lot of responses that it was a silly idea and
>>>>> naming was much more consistent and easy to follow.
>>>>>
>>>>> Is there a fundamental reason for it here?
>>>>>
>>>>> (note I don't mind either way as this seems more compact and cleaner
>>>>> in some ways)
>>>>>
>>>>
>>>> It follows the structure used by clocks, which uses the provided name(s) to
>>>> calculate an index into io-channels. This way, the provider does not have to
>>>> provide the mapping, the consumer does not have to know the io-channel index,
>>>> and the consumer code can call something like
>>>>
>>>> 	channel = iio_get_channel(dev, "vcc");
>>>>
>>>> In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".
>>>>
>>>> This works for both platform data and OF data (though platform data will
>>>> still need provider-based mapping, at least for now).
>>>>
>>>> This lets the code use a static name (eg "vcc"), and the mapping to the actual
>>>> provider happens through devicetree. Since the name is only used locally and
>>>> consumer driver specific, there is no need to define globally unique names.
>>>>
>>>> With this approach, the io channel map is not needed at all for the OF case.
>>>> I had used it in this version of the patch set, but got rid of it now.
>>>>
>>>> Actually, provider based mapping doesn't even work. If the consumer is
>>>> instantiated before the provider, the mapping doesn't exist yet, and the
>>>> call to iio_channel_get_all will fail. There is no way to prevent this,
>>>> as providers can come online at any time and there is no means to enforce that
>>>> all providers are already active by the time the consumers are instantiated.
>>>> Even if a mapping exists, there is no way to know if it is complete, if a
>>>> consumer is mapped to multiple providers.
>>>>
>>>> With the consumer based mapping, iio_channel_get_all 'knows' that not all
>>>> requested providers are available and can return -EPROBEDEFER in that case.
>>> Thanks. That makes sense.  At the moment iio_hwmon is the only case that
>>> does a 'get all'. Clearly things are easier when the driver is requesting a
>>> specific set and we can do the back off much more easily.
>>>
>>>>
>>>> As a side effect, we can also use the names - if provided - as channel
>>>> labels in iio_hwmon.
>>>>
>>>> Note this will require the iio_get_channel API to change from taking the
>>>> consumer device name to taking the consumer device pointer as argument.
>>>> This will enable it to work for both OF and non-OF cases, should address Lars'
>>>> concerns about duplicate API functions, and synchronize the code to match how
>>>> the clock framework works.
>>>
>>> Agreed, doing this gives us a cleaner syntax as well.  Note there are other
>>> users of that function in tree so be sure to get them all!
>>>
>>>>
>>>> Thanks,
>>>> Guenter
>>> Thanks for the explanation.  What I was actually suggesting was something
>>> like:
>>>
>>> adc: max1139@35 {
>>> 		compatible = "maxim,max1139";
>>> 		reg = <0x35>;
>>> 		#io-channel-cells = <1>;
>>> 		io-channel-output-names = "adc1", "adc2", "adc3"				
>>> 	};
>>>
>>> iio_hwmon {
>>> 	compatible = "iio-hwmon";
>>> 	io-channels = <&adc "adc1">, <&adc "adc2">, <&adc "adc3">,
>>> 	io-channel-names = "vcc", "vdd", "vref";
>>> }
>>>
>>> Having taken a look at the available syntax, those <> pairs have
>>> to be unsigned integers?  Hence the additional level of indirection?
>>
>> Yea, I think mixing phandles and strings simply doesn't work, due how
>> devicetree stores things.
>>
> 
> Another possibility would beto do things the way the regulator framework does
> an have each channel as a subnode to the converter devices eg.
> 
> 
> adc: max1139@35 {
>  		compatible = "maxim,max1139";
>  		reg = <0x35>;
> 		adc0: adc@0 {
> 			reg = <0>;
> 		};
> 		adc1: adc@1 {
> 			reg = <1>;
> 		};
> 		adc2: adc@2 {
> 			reg = <2>;
> 		};
> 
>  	};
> 
> 
> iio_hwmon {
>  	compatible = "iio-hwmon";
>  	io-channels = <&adc0>, <&adc1>, <&adc2>;
>  	io-channel-names = "vcc", "vdd", "vref";
> };
> 
> But I'm not sure how much sense this makes for IIO.
> 
The original approach is cleaner to my mind despite the indirection
(which is an indirection because often the channel indexing is non obvious
for a given device)

Lets just 'strongly' encourage the presences of io-channel-names and
io-channel-output-names even when not used by the particular drivers.
(though as Guenter said, they have uses even when not directly 'necessary')

Jonathan

> - Lars
> 
>>>
>>> (sorry, I'm getting you to give me a tutorial on device tree syntax rather
>>> than the actual issue here!)
>>>
>>> I guess it was desirable to keep the syntax relatively simple but that occasionally
>>> adds the requirement for a bit of indirection.
>>>
>>>
>>>
>>> Jonathan
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [RFC 10/11] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 11:57:53AM +0000, Jonathan Cameron wrote:
> On 02/03/2013 11:52 AM, Lars-Peter Clausen wrote:
> > On 02/03/2013 12:47 PM, Lars-Peter Clausen wrote:
> >> On 02/03/2013 12:39 PM, Jonathan Cameron wrote:
> >>> On 02/02/2013 04:10 PM, Guenter Roeck wrote:
> >>>> On Sat, Feb 02, 2013 at 10:29:02AM +0000, Jonathan Cameron wrote:
> >>>>> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> >>>>>> Provide bindings, new API access functions, and parse OF data
> >>>>>> during initialization.
> >>>>>>
> >>>>> Firstly thanks for working on this Guenter, it's been a big hole
> >>>>> for a while largely because non of our largest developers were
> >>>>> actually using development platforms with device tree support.
> >>>>>
> >>>>> Given my knowledge of device tree is based on the odd article
> >>>>> and looking at similar sets of bindings this morning, my comments
> >>>>> are likely to be somewhat superficial and uninformed ;)
> >>>>>
> >>>>> Mostly on this one I'll take a back seat and let those who
> >>>>> know this stuff better come to a consensus.
> >>>>>
> >>>>> Jonathan
> >>>>>
> >>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   97 ++++++++
> >>>>>>  drivers/iio/inkern.c                               |  241 ++++++++++++++++----
> >>>>>>  include/linux/iio/consumer.h                       |    8 +
> >>>>>>  3 files changed, 299 insertions(+), 47 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..0f51c95
> >>>>>> --- /dev/null
> >>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> >>>>>> @@ -0,0 +1,97 @@
> >>>>>> +This binding is a work-in-progress, and are based on clock bindings and
> >>>>>> +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 more 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 clock 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.
> >>>>>> +
> >>>>>> +Optional properties:
> >>>>>> +io-channel-output-names:
> >>>>>> +		    Recommended to be a list of strings of IIO output signal
> >>>>>> +		    names indexed by the first cell in the IIO specifier.
> >>>>>> +		    However, the meaning of io-channel-output-names is domain
> >>>>>> +		    specific to the IIO provider, and is only provided to
> >>>>>> +		    encourage using the same meaning for the majority of IIO
> >>>>>> +		    providers.  This format may not work for IIO providers
> >>>>>> +		    using a complex IIO specifier format.  In those cases it
> >>>>>> +		    is recommended to omit this property and create a binding
> >>>>>> +		    specific names property.
> >>>>>> +
> >>>>>> +		    IIO consumer nodes must never directly reference
> >>>>>> +		    the provider's io-channel-output-names property.
> >>>>>> +
> >>>>>> +For example:
> >>>>>> +
> >>>>>> +    adc: adc@35 {
> >>>>>> +	compatible = "maxim,max1139";
> >>>>>> +	reg = <0x35>;
> >>>>>> +        #io-channel-cells = <1>;
> >>>>>> +        io-channel-output-names = "adc1", "adc2";
> >>>>>> +    };
> >>>>>> +
> >>>>>> +- this node defines a device with two named IIO outputs, the first named
> >>>>>> +  "adc1" and the second named "adc2".  Consumer nodes always reference
> >>>>>> +  IIO channels by index. The names should reflect the IIO output signal
> >>>>>> +  names for the device.
> >>>>>> +
> >>>>>> +==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 #clock-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
> >>>>>> +		IIO channels from this node. Useful for bus nodes to provide
> >>>>>> +		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";
> >>>>> Having different numbers of channels and channel names seems
> >>>>> unusual... Deliberate or you got bored making up channel names?
> >>>>>
> >>>>> Why use indexed values for <&adc 0> etc rather than the output
> >>>>> channel names on adc?  For the iio_map stuff we initialy used
> >>>>> indexes but got a lot of responses that it was a silly idea and
> >>>>> naming was much more consistent and easy to follow.
> >>>>>
> >>>>> Is there a fundamental reason for it here?
> >>>>>
> >>>>> (note I don't mind either way as this seems more compact and cleaner
> >>>>> in some ways)
> >>>>>
> >>>>
> >>>> It follows the structure used by clocks, which uses the provided name(s) to
> >>>> calculate an index into io-channels. This way, the provider does not have to
> >>>> provide the mapping, the consumer does not have to know the io-channel index,
> >>>> and the consumer code can call something like
> >>>>
> >>>> 	channel = iio_get_channel(dev, "vcc");
> >>>>
> >>>> In the above example, "vcc" will map to "<&adc, 0>", and "vref" to "<&adc, 2>".
> >>>>
> >>>> This works for both platform data and OF data (though platform data will
> >>>> still need provider-based mapping, at least for now).
> >>>>
> >>>> This lets the code use a static name (eg "vcc"), and the mapping to the actual
> >>>> provider happens through devicetree. Since the name is only used locally and
> >>>> consumer driver specific, there is no need to define globally unique names.
> >>>>
> >>>> With this approach, the io channel map is not needed at all for the OF case.
> >>>> I had used it in this version of the patch set, but got rid of it now.
> >>>>
> >>>> Actually, provider based mapping doesn't even work. If the consumer is
> >>>> instantiated before the provider, the mapping doesn't exist yet, and the
> >>>> call to iio_channel_get_all will fail. There is no way to prevent this,
> >>>> as providers can come online at any time and there is no means to enforce that
> >>>> all providers are already active by the time the consumers are instantiated.
> >>>> Even if a mapping exists, there is no way to know if it is complete, if a
> >>>> consumer is mapped to multiple providers.
> >>>>
> >>>> With the consumer based mapping, iio_channel_get_all 'knows' that not all
> >>>> requested providers are available and can return -EPROBEDEFER in that case.
> >>> Thanks. That makes sense.  At the moment iio_hwmon is the only case that
> >>> does a 'get all'. Clearly things are easier when the driver is requesting a
> >>> specific set and we can do the back off much more easily.
> >>>
> >>>>
> >>>> As a side effect, we can also use the names - if provided - as channel
> >>>> labels in iio_hwmon.
> >>>>
> >>>> Note this will require the iio_get_channel API to change from taking the
> >>>> consumer device name to taking the consumer device pointer as argument.
> >>>> This will enable it to work for both OF and non-OF cases, should address Lars'
> >>>> concerns about duplicate API functions, and synchronize the code to match how
> >>>> the clock framework works.
> >>>
> >>> Agreed, doing this gives us a cleaner syntax as well.  Note there are other
> >>> users of that function in tree so be sure to get them all!
> >>>
> >>>>
> >>>> Thanks,
> >>>> Guenter
> >>> Thanks for the explanation.  What I was actually suggesting was something
> >>> like:
> >>>
> >>> adc: max1139@35 {
> >>> 		compatible = "maxim,max1139";
> >>> 		reg = <0x35>;
> >>> 		#io-channel-cells = <1>;
> >>> 		io-channel-output-names = "adc1", "adc2", "adc3"				
> >>> 	};
> >>>
> >>> iio_hwmon {
> >>> 	compatible = "iio-hwmon";
> >>> 	io-channels = <&adc "adc1">, <&adc "adc2">, <&adc "adc3">,
> >>> 	io-channel-names = "vcc", "vdd", "vref";
> >>> }
> >>>
> >>> Having taken a look at the available syntax, those <> pairs have
> >>> to be unsigned integers?  Hence the additional level of indirection?
> >>
> >> Yea, I think mixing phandles and strings simply doesn't work, due how
> >> devicetree stores things.
> >>
> > 
> > Another possibility would beto do things the way the regulator framework does
> > an have each channel as a subnode to the converter devices eg.
> > 
> > 
> > adc: max1139@35 {
> >  		compatible = "maxim,max1139";
> >  		reg = <0x35>;
> > 		adc0: adc@0 {
> > 			reg = <0>;
> > 		};
> > 		adc1: adc@1 {
> > 			reg = <1>;
> > 		};
> > 		adc2: adc@2 {
> > 			reg = <2>;
> > 		};
> > 
> >  	};
> > 
> > 
> > iio_hwmon {
> >  	compatible = "iio-hwmon";
> >  	io-channels = <&adc0>, <&adc1>, <&adc2>;
> >  	io-channel-names = "vcc", "vdd", "vref";
> > };
> > 
> > But I'm not sure how much sense this makes for IIO.
> > 
> The original approach is cleaner to my mind despite the indirection
> (which is an indirection because often the channel indexing is non obvious
> for a given device)
> 
+1

> Lets just 'strongly' encourage the presences of io-channel-names and
> io-channel-output-names even when not used by the particular drivers.
> (though as Guenter said, they have uses even when not directly 'necessary')
> 
Agreed. Right now I dropped io-channel-output-names, but after looking into the
clock code it seems it provides similar info as the mapping code. There is some
difference, as it doesn't associate consumer devices with the name, but there is
still the list of names mapped to the clock. I'll look into it some more to see
if and how we can use it.

Thanks,
Guenter

[RFC 11/11] iio/adc: (max1363) Add basic OF bindings and external vref support

[Guenter Roeck]

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 Documentation/devicetree/bindings/iio/max1363.txt |   54 +++++++++++++++++++++
 drivers/iio/adc/max1363.c                         |   54 ++++++++++++++++-----
 2 files changed, 95 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/max1363.txt

diff --git a/Documentation/devicetree/bindings/iio/max1363.txt b/Documentation/devicetree/bindings/iio/max1363.txt
new file mode 100644
index 0000000..6d22861
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/max1363.txt
@@ -0,0 +1,54 @@
+Device Tree bindings for MAX1363 and compatible ADC controllers
+
+This binding uses the common IIO binding[1].
+
+[1] Documentation/devicetree/bindings/iio/iio-bindings.txt
+
+Required properties:
+
+- compatible, shall be one of the following:
+	"maxim,max1361"
+	"maxim,max1362"
+	"maxim,max1363"
+	"maxim,max1364"
+	"maxim,max1036"
+	"maxim,max1037"
+	"maxim,max1038"
+	"maxim,max1039"
+	"maxim,max1136"
+	"maxim,max1137"
+	"maxim,max1138"
+	"maxim,max1139"
+	"maxim,max1236"
+	"maxim,max1237"
+	"maxim,max1238"
+	"maxim,max1239"
+	"maxim,max11600"
+	"maxim,max11601"
+	"maxim,max11602"
+	"maxim,max11603"
+	"maxim,max11604"
+	"maxim,max11605"
+	"maxim,max11606"
+	"maxim,max11607"
+	"maxim,max11608"
+	"maxim,max11609"
+	"maxim,max11610"
+	"maxim,max11611"
+	"maxim,max11612"
+	"maxim,max11613"
+	"maxim,max11614"
+	"maxim,max11615"
+	"maxim,max11616"
+	"maxim,max11617"
+
+- reg: shall be the I2C device address
+
+Required properties for IIO bindings:
+- #io-channel-cells: from common IIO bindings; shall be set to 1.
+
+Optional properties:
+- vref: Reference voltage in mV. If the provided reference voltage matches
+	the internal reference voltage, the internal reference voltage is used.
+	Otherwise it is assumed that an external reference voltage is used,
+	and the chip is programmed accordingly.
diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 51165d6..cf49a20 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -31,6 +31,7 @@
 #include <linux/slab.h>
 #include <linux/err.h>
 #include <linux/module.h>
+#include <linux/of.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -162,6 +163,7 @@ struct max1363_chip_info {
  * @mask_low:		bitmask for enabled low thresholds
  * @thresh_high:	high threshold values
  * @thresh_low:		low threshold values
+ * @vref_mv:		Actual (external or internal) reference voltage
  */
 struct max1363_state {
 	struct i2c_client		*client;
@@ -181,6 +183,7 @@ struct max1363_state {
 	/* 4x unipolar first then the fours bipolar ones */
 	s16				thresh_high[8];
 	s16				thresh_low[8];
+	u16				vref_mv;
 };
 
 #define MAX1363_MODE_SINGLE(_num, _mask) {				\
@@ -392,6 +395,8 @@ static int max1363_read_raw(struct iio_dev *indio_dev,
 {
 	struct max1363_state *st = iio_priv(indio_dev);
 	int ret;
+	unsigned long scale_uv;
+
 	switch (m) {
 	case IIO_CHAN_INFO_RAW:
 		ret = max1363_read_single_chan(indio_dev, chan, val, m);
@@ -399,16 +404,10 @@ static int max1363_read_raw(struct iio_dev *indio_dev,
 			return ret;
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
-		if ((1 << (st->chip_info->bits + 1)) >
-		    st->chip_info->int_vref_mv) {
-			*val = 0;
-			*val2 = 500000;
-			return IIO_VAL_INT_PLUS_MICRO;
-		} else {
-			*val = (st->chip_info->int_vref_mv)
-				>> st->chip_info->bits;
-			return IIO_VAL_INT;
-		}
+		scale_uv = (st->vref_mv * 1000) >> st->chip_info->bits;
+		*val = scale_uv / 1000;
+		*val2 = (scale_uv % 1000) * 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -1389,12 +1388,16 @@ static const struct max1363_chip_info max1363_chip_info_tbl[] = {
 
 static int max1363_initial_setup(struct max1363_state *st)
 {
-	st->setupbyte = MAX1363_SETUP_AIN3_IS_AIN3_REF_IS_VDD
-		| MAX1363_SETUP_POWER_UP_INT_REF
-		| MAX1363_SETUP_INT_CLOCK
+	st->setupbyte = MAX1363_SETUP_INT_CLOCK
 		| MAX1363_SETUP_UNIPOLAR
 		| MAX1363_SETUP_NORESET;
 
+	if (st->vref_mv != st->chip_info->int_vref_mv)
+		st->setupbyte |= MAX1363_SETUP_AIN3_IS_REF_EXT_TO_REF;
+	else
+		st->setupbyte |= MAX1363_SETUP_POWER_UP_INT_REF
+		  | MAX1363_SETUP_AIN3_IS_AIN3_REF_IS_INT;
+
 	/* Set scan mode writes the config anyway so wait until then*/
 	st->setupbyte = MAX1363_SETUP_BYTE(st->setupbyte);
 	st->current_mode = &max1363_mode_table[st->chip_info->default_mode];
@@ -1526,6 +1529,26 @@ static void max1363_buffer_cleanup(struct iio_dev *indio_dev)
 	iio_kfifo_free(indio_dev->buffer);
 }
 
+#ifdef CONFIG_OF
+static int max1363_parse_of(struct device *dev, struct max1363_state *st)
+{
+	struct device_node *np = dev->of_node;
+	u32 vref_mv;
+
+	if (!of_property_read_u32(np, "vref", &vref_mv)) {
+		if (vref_mv == 0 || vref_mv > USHRT_MAX)
+			return -EINVAL;
+		st->vref_mv = vref_mv;
+	}
+	return 0;
+}
+#else
+static int max1363_parse_of(struct device *dev, struct max1363_state *st)
+{
+	return 0;
+}
+#endif
+
 static int max1363_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1562,6 +1585,11 @@ static int max1363_probe(struct i2c_client *client,
 	st->chip_info = &max1363_chip_info_tbl[id->driver_data];
 	st->client = client;
 
+	st->vref_mv = st->chip_info->int_vref_mv;
+	ret = max1363_parse_of(&client->dev, st);
+	if (ret)
+		goto error_disable_reg;
+
 	ret = max1363_alloc_scan_masks(indio_dev);
 	if (ret)
 		goto error_disable_reg;
-- 
1.7.9.7

Re: [RFC 11/11] iio/adc: (max1363) Add basic OF bindings and external vref support

[Jonathan Cameron]

On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Mostly fine.  Comments below are on the fact I'd prefer
a reference voltage coming from a regulator than being
a bit of platform data.
> ---
>  Documentation/devicetree/bindings/iio/max1363.txt |   54 +++++++++++++++++++++
>  drivers/iio/adc/max1363.c                         |   54 ++++++++++++++++-----
>  2 files changed, 95 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/max1363.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/max1363.txt b/Documentation/devicetree/bindings/iio/max1363.txt
> new file mode 100644
> index 0000000..6d22861
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/max1363.txt
> @@ -0,0 +1,54 @@
> +Device Tree bindings for MAX1363 and compatible ADC controllers
> +
> +This binding uses the common IIO binding[1].
> +
> +[1] Documentation/devicetree/bindings/iio/iio-bindings.txt
> +
> +Required properties:
> +
> +- compatible, shall be one of the following:
> +	"maxim,max1361"
> +	"maxim,max1362"
> +	"maxim,max1363"
> +	"maxim,max1364"
> +	"maxim,max1036"
> +	"maxim,max1037"
> +	"maxim,max1038"
> +	"maxim,max1039"
> +	"maxim,max1136"
> +	"maxim,max1137"
> +	"maxim,max1138"
> +	"maxim,max1139"
> +	"maxim,max1236"
> +	"maxim,max1237"
> +	"maxim,max1238"
> +	"maxim,max1239"
> +	"maxim,max11600"
> +	"maxim,max11601"
> +	"maxim,max11602"
> +	"maxim,max11603"
> +	"maxim,max11604"
> +	"maxim,max11605"
> +	"maxim,max11606"
> +	"maxim,max11607"
> +	"maxim,max11608"
> +	"maxim,max11609"
> +	"maxim,max11610"
> +	"maxim,max11611"
> +	"maxim,max11612"
> +	"maxim,max11613"
> +	"maxim,max11614"
> +	"maxim,max11615"
> +	"maxim,max11616"
> +	"maxim,max11617"
> +
> +- reg: shall be the I2C device address
> +
> +Required properties for IIO bindings:
> +- #io-channel-cells: from common IIO bindings; shall be set to 1.
> +
> +Optional properties:
> +- vref: Reference voltage in mV. If the provided reference voltage matches
> +	the internal reference voltage, the internal reference voltage is used.
> +	Otherwise it is assumed that an external reference voltage is used,
> +	and the chip is programmed accordingly.

Why not use a regulator? It has a nice device tree map and if it's just a fixed
voltage, we have the fixed regulator driver for them.  This is pretty common
throughout IIO (unsuprisingly) and we've been generally getting with platform
data that does this in favour of regulators.  Back when we started out, the
regulators framework was new so providing an alternative was pretty much
required.  Now it's pretty universal.


> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 51165d6..cf49a20 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -31,6 +31,7 @@
>  #include <linux/slab.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -162,6 +163,7 @@ struct max1363_chip_info {
>   * @mask_low:		bitmask for enabled low thresholds
>   * @thresh_high:	high threshold values
>   * @thresh_low:		low threshold values
> + * @vref_mv:		Actual (external or internal) reference voltage
>   */
>  struct max1363_state {
>  	struct i2c_client		*client;
> @@ -181,6 +183,7 @@ struct max1363_state {
>  	/* 4x unipolar first then the fours bipolar ones */
>  	s16				thresh_high[8];
>  	s16				thresh_low[8];
> +	u16				vref_mv;
>  };
>  
>  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> @@ -392,6 +395,8 @@ static int max1363_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct max1363_state *st = iio_priv(indio_dev);
>  	int ret;
> +	unsigned long scale_uv;
> +
>  	switch (m) {
>  	case IIO_CHAN_INFO_RAW:
>  		ret = max1363_read_single_chan(indio_dev, chan, val, m);
> @@ -399,16 +404,10 @@ static int max1363_read_raw(struct iio_dev *indio_dev,
>  			return ret;
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
> -		if ((1 << (st->chip_info->bits + 1)) >
> -		    st->chip_info->int_vref_mv) {
> -			*val = 0;
> -			*val2 = 500000;
> -			return IIO_VAL_INT_PLUS_MICRO;
> -		} else {
> -			*val = (st->chip_info->int_vref_mv)
> -				>> st->chip_info->bits;
> -			return IIO_VAL_INT;
> -		}
> +		scale_uv = (st->vref_mv * 1000) >> st->chip_info->bits;
> +		*val = scale_uv / 1000;
> +		*val2 = (scale_uv % 1000) * 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1389,12 +1388,16 @@ static const struct max1363_chip_info max1363_chip_info_tbl[] = {
>  
>  static int max1363_initial_setup(struct max1363_state *st)
>  {
> -	st->setupbyte = MAX1363_SETUP_AIN3_IS_AIN3_REF_IS_VDD
> -		| MAX1363_SETUP_POWER_UP_INT_REF
> -		| MAX1363_SETUP_INT_CLOCK
> +	st->setupbyte = MAX1363_SETUP_INT_CLOCK
>  		| MAX1363_SETUP_UNIPOLAR
>  		| MAX1363_SETUP_NORESET;
>  
> +	if (st->vref_mv != st->chip_info->int_vref_mv)
> +		st->setupbyte |= MAX1363_SETUP_AIN3_IS_REF_EXT_TO_REF;
> +	else
> +		st->setupbyte |= MAX1363_SETUP_POWER_UP_INT_REF
> +		  | MAX1363_SETUP_AIN3_IS_AIN3_REF_IS_INT;
> +
>  	/* Set scan mode writes the config anyway so wait until then*/
>  	st->setupbyte = MAX1363_SETUP_BYTE(st->setupbyte);
>  	st->current_mode = &max1363_mode_table[st->chip_info->default_mode];
> @@ -1526,6 +1529,26 @@ static void max1363_buffer_cleanup(struct iio_dev *indio_dev)
>  	iio_kfifo_free(indio_dev->buffer);
>  }
>  
> +#ifdef CONFIG_OF
> +static int max1363_parse_of(struct device *dev, struct max1363_state *st)
> +{
> +	struct device_node *np = dev->of_node;
> +	u32 vref_mv;
> +
> +	if (!of_property_read_u32(np, "vref", &vref_mv)) {
> +		if (vref_mv == 0 || vref_mv > USHRT_MAX)
> +			return -EINVAL;
> +		st->vref_mv = vref_mv;
> +	}
> +	return 0;
> +}
> +#else
> +static int max1363_parse_of(struct device *dev, struct max1363_state *st)
> +{
> +	return 0;
> +}
> +#endif
> +
>  static int max1363_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -1562,6 +1585,11 @@ static int max1363_probe(struct i2c_client *client,
>  	st->chip_info = &max1363_chip_info_tbl[id->driver_data];
>  	st->client = client;
>  
> +	st->vref_mv = st->chip_info->int_vref_mv;
> +	ret = max1363_parse_of(&client->dev, st);
> +	if (ret)
> +		goto error_disable_reg;
> +
>  	ret = max1363_alloc_scan_masks(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;
> 

Re: [RFC 11/11] iio/adc: (max1363) Add basic OF bindings and external vref support

[Guenter Roeck]

On Sat, Feb 02, 2013 at 10:33:12AM +0000, Jonathan Cameron wrote:
> On 01/31/2013 09:43 PM, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Mostly fine.  Comments below are on the fact I'd prefer
> a reference voltage coming from a regulator than being
> a bit of platform data.
> > ---
> >  Documentation/devicetree/bindings/iio/max1363.txt |   54 +++++++++++++++++++++
> >  drivers/iio/adc/max1363.c                         |   54 ++++++++++++++++-----
> >  2 files changed, 95 insertions(+), 13 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/iio/max1363.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/max1363.txt b/Documentation/devicetree/bindings/iio/max1363.txt
> > new file mode 100644
> > index 0000000..6d22861
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/max1363.txt
> > @@ -0,0 +1,54 @@
> > +Device Tree bindings for MAX1363 and compatible ADC controllers
> > +
> > +This binding uses the common IIO binding[1].
> > +
> > +[1] Documentation/devicetree/bindings/iio/iio-bindings.txt
> > +
> > +Required properties:
> > +
> > +- compatible, shall be one of the following:
> > +	"maxim,max1361"
> > +	"maxim,max1362"
> > +	"maxim,max1363"
> > +	"maxim,max1364"
> > +	"maxim,max1036"
> > +	"maxim,max1037"
> > +	"maxim,max1038"
> > +	"maxim,max1039"
> > +	"maxim,max1136"
> > +	"maxim,max1137"
> > +	"maxim,max1138"
> > +	"maxim,max1139"
> > +	"maxim,max1236"
> > +	"maxim,max1237"
> > +	"maxim,max1238"
> > +	"maxim,max1239"
> > +	"maxim,max11600"
> > +	"maxim,max11601"
> > +	"maxim,max11602"
> > +	"maxim,max11603"
> > +	"maxim,max11604"
> > +	"maxim,max11605"
> > +	"maxim,max11606"
> > +	"maxim,max11607"
> > +	"maxim,max11608"
> > +	"maxim,max11609"
> > +	"maxim,max11610"
> > +	"maxim,max11611"
> > +	"maxim,max11612"
> > +	"maxim,max11613"
> > +	"maxim,max11614"
> > +	"maxim,max11615"
> > +	"maxim,max11616"
> > +	"maxim,max11617"
> > +
> > +- reg: shall be the I2C device address
> > +
> > +Required properties for IIO bindings:
> > +- #io-channel-cells: from common IIO bindings; shall be set to 1.
> > +
> > +Optional properties:
> > +- vref: Reference voltage in mV. If the provided reference voltage matches
> > +	the internal reference voltage, the internal reference voltage is used.
> > +	Otherwise it is assumed that an external reference voltage is used,
> > +	and the chip is programmed accordingly.
> 
> Why not use a regulator? It has a nice device tree map and if it's just a fixed
> voltage, we have the fixed regulator driver for them.  This is pretty common
> throughout IIO (unsuprisingly) and we've been generally getting with platform
> data that does this in favour of regulators.  Back when we started out, the
> regulators framework was new so providing an alternative was pretty much
> required.  Now it's pretty universal.
> 
Makes sense' I'll look into it.

Thanks,
Guenter