[RFC v2 0/4] iio: Devicetree support

[RFC v2 0/4] 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 first two patches are actually updates to the MAX1363 driver. The first
patch updates the driver to use devm_ functions, and the second patch adds
support for an external reference voltage.

Patch 3 changes the first parameter to iio_channel_get() to be the pointer to
the consumer device instead of the consumer device name.

Patch 4 adds basic OF support to the IIO subsystem. Support is modeled after
the clock subsystem.

The patches are now based on the to togreg branch of iio kernel repository.
Several patches were already applied and are no longer part of the patch set.

Guenter

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

[PATCH 1/4] iio: max1363: Use devm_ functions whereever possible to allocate resources

[Guenter Roeck]

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

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index ef868c9..1353fda 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -1410,8 +1410,9 @@ static int max1363_alloc_scan_masks(struct iio_dev *indio_dev)
 	unsigned long *masks;
 	int i;
 
-	masks = kzalloc(BITS_TO_LONGS(MAX1363_MAX_CHANNELS)*sizeof(long)*
-			  (st->chip_info->num_modes + 1), GFP_KERNEL);
+	masks = devm_kzalloc(&indio_dev->dev,
+			BITS_TO_LONGS(MAX1363_MAX_CHANNELS) * sizeof(long) *
+			(st->chip_info->num_modes + 1), GFP_KERNEL);
 	if (!masks)
 		return -ENOMEM;
 
@@ -1546,7 +1547,7 @@ static int max1363_probe(struct i2c_client *client,
 
 	st = iio_priv(indio_dev);
 
-	st->reg = regulator_get(&client->dev, "vcc");
+	st->reg = devm_regulator_get(&client->dev, "vcc");
 	if (IS_ERR(st->reg)) {
 		ret = PTR_ERR(st->reg);
 		goto error_unregister_map;
@@ -1554,7 +1555,7 @@ static int max1363_probe(struct i2c_client *client,
 
 	ret = regulator_enable(st->reg);
 	if (ret)
-		goto error_put_reg;
+		goto error_unregister_map;
 
 	/* this is only used for device removal purposes */
 	i2c_set_clientdata(client, indio_dev);
@@ -1575,15 +1576,15 @@ static int max1363_probe(struct i2c_client *client,
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	ret = max1363_initial_setup(st);
 	if (ret < 0)
-		goto error_free_available_scan_masks;
+		goto error_disable_reg;
 
 	ret = iio_triggered_buffer_setup(indio_dev, NULL,
 		&max1363_trigger_handler, &max1363_buffered_setup_ops);
 	if (ret)
-		goto error_free_available_scan_masks;
+		goto error_disable_reg;
 
 	if (client->irq) {
-		ret = request_threaded_irq(st->client->irq,
+		ret = devm_request_threaded_irq(&client->dev, st->client->irq,
 					   NULL,
 					   &max1363_event_handler,
 					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
@@ -1596,20 +1597,14 @@ static int max1363_probe(struct i2c_client *client,
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
-		goto error_free_irq;
+		goto error_uninit_buffer;
 
 	return 0;
-error_free_irq:
-	if (client->irq)
-		free_irq(st->client->irq, indio_dev);
+
 error_uninit_buffer:
 	iio_triggered_buffer_cleanup(indio_dev);
-error_free_available_scan_masks:
-	kfree(indio_dev->available_scan_masks);
 error_disable_reg:
 	regulator_disable(st->reg);
-error_put_reg:
-	regulator_put(st->reg);
 error_unregister_map:
 	iio_map_array_unregister(indio_dev);
 error_free_device:
@@ -1624,12 +1619,8 @@ static int max1363_remove(struct i2c_client *client)
 	struct max1363_state *st = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
-	if (client->irq)
-		free_irq(st->client->irq, indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
-	kfree(indio_dev->available_scan_masks);
 	regulator_disable(st->reg);
-	regulator_put(st->reg);
 	iio_map_array_unregister(indio_dev);
 	iio_device_free(indio_dev);
 
-- 
1.7.9.7

Re: [PATCH 1/4] iio: max1363: Use devm_ functions whereever possible to allocate resources

[Jonathan Cameron]

On 02/03/2013 12:59 AM, Guenter Roeck wrote:
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied to togreg branch of iio.git.

Note I'll probably rebase the togreg branch if / when Greg has pulled
last pull request (sent a few mins ago) so as to get a directly
applied fix for this driver.

> ---
>  drivers/iio/adc/max1363.c |   29 ++++++++++-------------------
>  1 file changed, 10 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index ef868c9..1353fda 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -1410,8 +1410,9 @@ static int max1363_alloc_scan_masks(struct iio_dev *indio_dev)
>  	unsigned long *masks;
>  	int i;
>  
> -	masks = kzalloc(BITS_TO_LONGS(MAX1363_MAX_CHANNELS)*sizeof(long)*
> -			  (st->chip_info->num_modes + 1), GFP_KERNEL);
> +	masks = devm_kzalloc(&indio_dev->dev,
> +			BITS_TO_LONGS(MAX1363_MAX_CHANNELS) * sizeof(long) *
> +			(st->chip_info->num_modes + 1), GFP_KERNEL);
>  	if (!masks)
>  		return -ENOMEM;
>  
> @@ -1546,7 +1547,7 @@ static int max1363_probe(struct i2c_client *client,
>  
>  	st = iio_priv(indio_dev);
>  
> -	st->reg = regulator_get(&client->dev, "vcc");
> +	st->reg = devm_regulator_get(&client->dev, "vcc");
>  	if (IS_ERR(st->reg)) {
>  		ret = PTR_ERR(st->reg);
>  		goto error_unregister_map;
> @@ -1554,7 +1555,7 @@ static int max1363_probe(struct i2c_client *client,
>  
>  	ret = regulator_enable(st->reg);
>  	if (ret)
> -		goto error_put_reg;
> +		goto error_unregister_map;
>  
>  	/* this is only used for device removal purposes */
>  	i2c_set_clientdata(client, indio_dev);
> @@ -1575,15 +1576,15 @@ static int max1363_probe(struct i2c_client *client,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	ret = max1363_initial_setup(st);
>  	if (ret < 0)
> -		goto error_free_available_scan_masks;
> +		goto error_disable_reg;
>  
>  	ret = iio_triggered_buffer_setup(indio_dev, NULL,
>  		&max1363_trigger_handler, &max1363_buffered_setup_ops);
>  	if (ret)
> -		goto error_free_available_scan_masks;
> +		goto error_disable_reg;
>  
>  	if (client->irq) {
> -		ret = request_threaded_irq(st->client->irq,
> +		ret = devm_request_threaded_irq(&client->dev, st->client->irq,
>  					   NULL,
>  					   &max1363_event_handler,
>  					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> @@ -1596,20 +1597,14 @@ static int max1363_probe(struct i2c_client *client,
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0)
> -		goto error_free_irq;
> +		goto error_uninit_buffer;
>  
>  	return 0;
> -error_free_irq:
> -	if (client->irq)
> -		free_irq(st->client->irq, indio_dev);
> +
>  error_uninit_buffer:
>  	iio_triggered_buffer_cleanup(indio_dev);
> -error_free_available_scan_masks:
> -	kfree(indio_dev->available_scan_masks);
>  error_disable_reg:
>  	regulator_disable(st->reg);
> -error_put_reg:
> -	regulator_put(st->reg);
>  error_unregister_map:
>  	iio_map_array_unregister(indio_dev);
>  error_free_device:
> @@ -1624,12 +1619,8 @@ static int max1363_remove(struct i2c_client *client)
>  	struct max1363_state *st = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> -	if (client->irq)
> -		free_irq(st->client->irq, indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> -	kfree(indio_dev->available_scan_masks);
>  	regulator_disable(st->reg);
> -	regulator_put(st->reg);
>  	iio_map_array_unregister(indio_dev);
>  	iio_device_free(indio_dev);
>  
> 

Re: [PATCH 1/4] iio: max1363: Use devm_ functions whereever possible to allocate resources

[Guenter Roeck]

On Sun, Feb 03, 2013 at 12:10:32PM +0000, Jonathan Cameron wrote:
> On 02/03/2013 12:59 AM, Guenter Roeck wrote:
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> Applied to togreg branch of iio.git.
> 
> Note I'll probably rebase the togreg branch if / when Greg has pulled
> last pull request (sent a few mins ago) so as to get a directly
> applied fix for this driver.
> 
No problem.

There is some dead code in the max1363 driver.

drivers/iio/adc/max1363.c:1492:12: warning: 'max1363_register_buffered_funcs_and_init' defined but not used [-Wunused-function]
drivers/iio/adc/max1363.c:1527:13: warning: 'max1363_buffer_cleanup' defined but not used [-Wunused-function]

I thought I had seen a note on the iio mailing list that you wanted to submit a patch for it,
but I didn't see it yet. Or maybe my memory is wrong, or you were talking about
something else.

Want me to submit a patch (assuming the fix is simply to remove the above functions) ?

Thanks,
Guenter

Re: [PATCH 1/4] iio: max1363: Use devm_ functions whereever possible to allocate resources

[Jonathan Cameron]

On 02/03/2013 05:18 PM, Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 12:10:32PM +0000, Jonathan Cameron wrote:
>> On 02/03/2013 12:59 AM, Guenter Roeck wrote:
>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>> Applied to togreg branch of iio.git.
>>
>> Note I'll probably rebase the togreg branch if / when Greg has pulled
>> last pull request (sent a few mins ago) so as to get a directly
>> applied fix for this driver.
>>
> No problem.
> 
> There is some dead code in the max1363 driver.
> 
> drivers/iio/adc/max1363.c:1492:12: warning: 'max1363_register_buffered_funcs_and_init' defined but not used [-Wunused-function]
> drivers/iio/adc/max1363.c:1527:13: warning: 'max1363_buffer_cleanup' defined but not used [-Wunused-function]
> 
> I thought I had seen a note on the iio mailing list that you wanted to submit a patch for it,

Went out yesterday as [PATCH] iio:max1363 remove some functions left after merge

> but I didn't see it yet. Or maybe my memory is wrong, or you were talking about
> something else.

It's gone to Greg who added it staging-next yesterday (rather than throw the iio tree).
I actually intended to get his Ack on it (as the merge that caused it was I think
his), but he applied it directly. Anyhow, that's the reason staging-next with that patch
will differ from iio.git and hence why I'll rebase once the current pull request is
dealt with.

> 
> Want me to submit a patch (assuming the fix is simply to remove the above functions) ?

All sorted if via an indirect route.

> 
> Thanks,
> Guenter
> --
> 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: [PATCH 1/4] iio: max1363: Use devm_ functions whereever possible to allocate resources

[Guenter Roeck]

On Sun, Feb 03, 2013 at 06:02:57PM +0000, Jonathan Cameron wrote:
> On 02/03/2013 05:18 PM, Guenter Roeck wrote:
> > On Sun, Feb 03, 2013 at 12:10:32PM +0000, Jonathan Cameron wrote:
> >> On 02/03/2013 12:59 AM, Guenter Roeck wrote:
> >>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >> Applied to togreg branch of iio.git.
> >>
> >> Note I'll probably rebase the togreg branch if / when Greg has pulled
> >> last pull request (sent a few mins ago) so as to get a directly
> >> applied fix for this driver.
> >>
> > No problem.
> > 
> > There is some dead code in the max1363 driver.
> > 
> > drivers/iio/adc/max1363.c:1492:12: warning: 'max1363_register_buffered_funcs_and_init' defined but not used [-Wunused-function]
> > drivers/iio/adc/max1363.c:1527:13: warning: 'max1363_buffer_cleanup' defined but not used [-Wunused-function]
> > 
> > I thought I had seen a note on the iio mailing list that you wanted to submit a patch for it,
> 
> Went out yesterday as [PATCH] iio:max1363 remove some functions left after merge
> 
> > but I didn't see it yet. Or maybe my memory is wrong, or you were talking about
> > something else.
> 
> It's gone to Greg who added it staging-next yesterday (rather than throw the iio tree).
> I actually intended to get his Ack on it (as the merge that caused it was I think
> his), but he applied it directly. Anyhow, that's the reason staging-next with that patch
> will differ from iio.git and hence why I'll rebase once the current pull request is
> dealt with.
> 
> > 
> > Want me to submit a patch (assuming the fix is simply to remove the above functions) ?
> 
> All sorted if via an indirect route.
> 
Great, thanks.

Guenter

[PATCH v2 2/4] iio/adc: (max1363) Add support for external reference voltage

[Guenter Roeck]

Implement external reference voltage as regulator named "vref".

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
v2: Use regulator API to specify vref instead of creating new devicetree
    bindings.
    Keep reference voltage internally in uV, as this is the scale provided
    by the regulator subsystem. We need the value in uV anyway, so that does
    make some sense.

 drivers/iio/adc/max1363.c |   52 +++++++++++++++++++++++++++++++++------------
 1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
index 1353fda..cac73d7 100644
--- a/drivers/iio/adc/max1363.c
+++ b/drivers/iio/adc/max1363.c
@@ -163,6 +163,8 @@ struct max1363_chip_info {
  * @mask_low:		bitmask for enabled low thresholds
  * @thresh_high:	high threshold values
  * @thresh_low:		low threshold values
+ * @vref:		Reference voltage regulator
+ * @vref_uv:		Actual (external or internal) reference voltage
  */
 struct max1363_state {
 	struct i2c_client		*client;
@@ -182,6 +184,8 @@ struct max1363_state {
 	/* 4x unipolar first then the fours bipolar ones */
 	s16				thresh_high[8];
 	s16				thresh_low[8];
+	struct regulator		*vref;
+	u32				vref_uv;
 };
 
 #define MAX1363_MODE_SINGLE(_num, _mask) {				\
@@ -393,6 +397,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);
@@ -400,16 +406,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_uv >> st->chip_info->bits;
+		*val = scale_uv / 1000;
+		*val2 = (scale_uv % 1000) * 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -1390,12 +1390,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)
+		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];
@@ -1533,6 +1537,7 @@ static int max1363_probe(struct i2c_client *client,
 	int ret;
 	struct max1363_state *st;
 	struct iio_dev *indio_dev;
+	struct regulator *vref;
 
 	indio_dev = iio_device_alloc(sizeof(struct max1363_state));
 	if (indio_dev == NULL) {
@@ -1563,6 +1568,23 @@ static int max1363_probe(struct i2c_client *client,
 	st->chip_info = &max1363_chip_info_tbl[id->driver_data];
 	st->client = client;
 
+	st->vref_uv = st->chip_info->int_vref_mv * 1000;
+	vref = devm_regulator_get(&client->dev, "vref");
+	if (!IS_ERR(vref)) {
+		int vref_uv;
+
+		ret = regulator_enable(vref);
+		if (ret)
+			goto error_disable_reg;
+		st->vref = vref;
+		vref_uv = regulator_get_voltage(vref);
+		if (vref_uv <= 0) {
+			ret = -EINVAL;
+			goto error_disable_reg;
+		}
+		st->vref_uv = vref_uv;
+	}
+
 	ret = max1363_alloc_scan_masks(indio_dev);
 	if (ret)
 		goto error_disable_reg;
@@ -1604,6 +1626,8 @@ static int max1363_probe(struct i2c_client *client,
 error_uninit_buffer:
 	iio_triggered_buffer_cleanup(indio_dev);
 error_disable_reg:
+	if (st->vref)
+		regulator_disable(st->vref);
 	regulator_disable(st->reg);
 error_unregister_map:
 	iio_map_array_unregister(indio_dev);
@@ -1620,6 +1644,8 @@ static int max1363_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
+	if (st->vref)
+		regulator_disable(st->vref);
 	regulator_disable(st->reg);
 	iio_map_array_unregister(indio_dev);
 	iio_device_free(indio_dev);
-- 
1.7.9.7

Re: [PATCH v2 2/4] iio/adc: (max1363) Add support for external reference voltage

[Jonathan Cameron]

On 02/03/2013 12:59 AM, Guenter Roeck wrote:
> Implement external reference voltage as regulator named "vref".
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Applied to togreg branch of iio.git.
Thanks,

The remaining two patches will have sit a while.
Patch 3 effects stuff outside IIO so I'll wait to see if anyone
screams that it is going to cause merge nightmares!

Patch 4 is clearly still under discussion though a consensus seems
to have more or less been reached.

Thanks again for this work Guenter. I like the way one of my oldest
drivers is getting cleaned up along the way without me having to
do anything much ;)

> ---
> v2: Use regulator API to specify vref instead of creating new devicetree
>     bindings.
>     Keep reference voltage internally in uV, as this is the scale provided
>     by the regulator subsystem. We need the value in uV anyway, so that does
>     make some sense.
> 
>  drivers/iio/adc/max1363.c |   52 +++++++++++++++++++++++++++++++++------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/adc/max1363.c b/drivers/iio/adc/max1363.c
> index 1353fda..cac73d7 100644
> --- a/drivers/iio/adc/max1363.c
> +++ b/drivers/iio/adc/max1363.c
> @@ -163,6 +163,8 @@ struct max1363_chip_info {
>   * @mask_low:		bitmask for enabled low thresholds
>   * @thresh_high:	high threshold values
>   * @thresh_low:		low threshold values
> + * @vref:		Reference voltage regulator
> + * @vref_uv:		Actual (external or internal) reference voltage
>   */
>  struct max1363_state {
>  	struct i2c_client		*client;
> @@ -182,6 +184,8 @@ struct max1363_state {
>  	/* 4x unipolar first then the fours bipolar ones */
>  	s16				thresh_high[8];
>  	s16				thresh_low[8];
> +	struct regulator		*vref;
> +	u32				vref_uv;
>  };
>  
>  #define MAX1363_MODE_SINGLE(_num, _mask) {				\
> @@ -393,6 +397,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);
> @@ -400,16 +406,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_uv >> st->chip_info->bits;
> +		*val = scale_uv / 1000;
> +		*val2 = (scale_uv % 1000) * 1000;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -1390,12 +1390,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)
> +		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];
> @@ -1533,6 +1537,7 @@ static int max1363_probe(struct i2c_client *client,
>  	int ret;
>  	struct max1363_state *st;
>  	struct iio_dev *indio_dev;
> +	struct regulator *vref;
>  
>  	indio_dev = iio_device_alloc(sizeof(struct max1363_state));
>  	if (indio_dev == NULL) {
> @@ -1563,6 +1568,23 @@ static int max1363_probe(struct i2c_client *client,
>  	st->chip_info = &max1363_chip_info_tbl[id->driver_data];
>  	st->client = client;
>  
> +	st->vref_uv = st->chip_info->int_vref_mv * 1000;
> +	vref = devm_regulator_get(&client->dev, "vref");
> +	if (!IS_ERR(vref)) {
> +		int vref_uv;
> +
> +		ret = regulator_enable(vref);
> +		if (ret)
> +			goto error_disable_reg;
> +		st->vref = vref;
> +		vref_uv = regulator_get_voltage(vref);
> +		if (vref_uv <= 0) {
> +			ret = -EINVAL;
> +			goto error_disable_reg;
> +		}
> +		st->vref_uv = vref_uv;
> +	}
> +
>  	ret = max1363_alloc_scan_masks(indio_dev);
>  	if (ret)
>  		goto error_disable_reg;
> @@ -1604,6 +1626,8 @@ static int max1363_probe(struct i2c_client *client,
>  error_uninit_buffer:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  error_disable_reg:
> +	if (st->vref)
> +		regulator_disable(st->vref);
>  	regulator_disable(st->reg);
>  error_unregister_map:
>  	iio_map_array_unregister(indio_dev);
> @@ -1620,6 +1644,8 @@ static int max1363_remove(struct i2c_client *client)
>  
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> +	if (st->vref)
> +		regulator_disable(st->vref);
>  	regulator_disable(st->reg);
>  	iio_map_array_unregister(indio_dev);
>  	iio_device_free(indio_dev);
> 

[PATCH RFC 3/4] iio: Update iio_channel_get API to use consumer device pointer as argument

[Guenter Roeck]

For iio_channel_get to work with OF based configurations, it needs the
consumer device pointer instead of the consumer device name as argument.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 drivers/extcon/extcon-adc-jack.c    |    3 +--
 drivers/iio/inkern.c                |   11 ++++++++++-
 drivers/power/generic-adc-battery.c |    4 ++--
 drivers/power/lp8788-charger.c      |    8 ++++----
 include/linux/iio/consumer.h        |    5 +++--
 5 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/drivers/extcon/extcon-adc-jack.c b/drivers/extcon/extcon-adc-jack.c
index eda2a1a..d0233cd 100644
--- a/drivers/extcon/extcon-adc-jack.c
+++ b/drivers/extcon/extcon-adc-jack.c
@@ -135,8 +135,7 @@ static int adc_jack_probe(struct platform_device *pdev)
 		;
 	data->num_conditions = i;
 
-	data->chan = iio_channel_get(dev_name(&pdev->dev),
-			pdata->consumer_channel);
+	data->chan = iio_channel_get(&pdev->dev, pdata->consumer_channel);
 	if (IS_ERR(data->chan)) {
 		err = PTR_ERR(data->chan);
 		goto out;
diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index c42aba6..b289915 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -93,7 +93,8 @@ static const struct iio_chan_spec
 }
 
 
-struct iio_channel *iio_channel_get(const char *name, const char *channel_name)
+static struct iio_channel *iio_channel_get_sys(const char *name,
+					       const char *channel_name)
 {
 	struct iio_map_internal *c_i = NULL, *c = NULL;
 	struct iio_channel *channel;
@@ -144,6 +145,14 @@ error_no_mem:
 	iio_device_put(c->indio_dev);
 	return ERR_PTR(err);
 }
+
+struct iio_channel *iio_channel_get(struct device *dev,
+				    const char *channel_name)
+{
+	const char *name = dev ? dev_name(dev) : NULL;
+
+	return iio_channel_get_sys(name, channel_name);
+}
 EXPORT_SYMBOL_GPL(iio_channel_get);
 
 void iio_channel_release(struct iio_channel *channel)
diff --git a/drivers/power/generic-adc-battery.c b/drivers/power/generic-adc-battery.c
index 32ce17e..42733c4 100644
--- a/drivers/power/generic-adc-battery.c
+++ b/drivers/power/generic-adc-battery.c
@@ -287,8 +287,8 @@ static int gab_probe(struct platform_device *pdev)
 	 * based on the channel supported by consumer device.
 	 */
 	for (chan = 0; chan < ARRAY_SIZE(gab_chan_name); chan++) {
-		adc_bat->channel[chan] = iio_channel_get(dev_name(&pdev->dev),
-						gab_chan_name[chan]);
+		adc_bat->channel[chan] = iio_channel_get(&pdev->dev,
+							 gab_chan_name[chan]);
 		if (IS_ERR(adc_bat->channel[chan])) {
 			ret = PTR_ERR(adc_bat->channel[chan]);
 		} else {
diff --git a/drivers/power/lp8788-charger.c b/drivers/power/lp8788-charger.c
index 22b6407c..2788908 100644
--- a/drivers/power/lp8788-charger.c
+++ b/drivers/power/lp8788-charger.c
@@ -580,7 +580,7 @@ static void lp8788_irq_unregister(struct platform_device *pdev,
 	}
 }
 
-static void lp8788_setup_adc_channel(const char *consumer_name,
+static void lp8788_setup_adc_channel(struct device *dev,
 				struct lp8788_charger *pchg)
 {
 	struct lp8788_charger_platform_data *pdata = pchg->pdata;
@@ -590,11 +590,11 @@ static void lp8788_setup_adc_channel(const char *consumer_name,
 		return;
 
 	/* ADC channel for battery voltage */
-	chan = iio_channel_get(consumer_name, pdata->adc_vbatt);
+	chan = iio_channel_get(dev, pdata->adc_vbatt);
 	pchg->chan[LP8788_VBATT] = IS_ERR(chan) ? NULL : chan;
 
 	/* ADC channel for battery temperature */
-	chan = iio_channel_get(consumer_name, pdata->adc_batt_temp);
+	chan = iio_channel_get(dev, pdata->adc_batt_temp);
 	pchg->chan[LP8788_BATT_TEMP] = IS_ERR(chan) ? NULL : chan;
 }
 
@@ -704,7 +704,7 @@ static int lp8788_charger_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	lp8788_setup_adc_channel(pdev->name, pchg);
+	lp8788_setup_adc_channel(&pdev->dev, pchg);
 
 	ret = lp8788_psy_register(pdev, pchg);
 	if (ret)
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index a85787a..833926c 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -31,14 +31,15 @@ struct iio_channel {
 
 /**
  * iio_channel_get() - get description of all that is needed to access channel.
- * @name:		Unique name of the device as provided in the iio_map
+ * @dev:		Pointer to consumer device. Device name must match
+ *			the name of the device as provided in the iio_map
  *			with which the desired provider to consumer mapping
  *			was registered.
  * @consumer_channel:	Unique name to identify the channel on the consumer
  *			side. This typically describes the channels use within
  *			the consumer. E.g. 'battery_voltage'
  */
-struct iio_channel *iio_channel_get(const char *name,
+struct iio_channel *iio_channel_get(struct device *dev,
 				    const char *consumer_channel);
 
 /**
-- 
1.7.9.7

[PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

Provide bindings and parse OF data during initialization.

Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
- Documentation update per feedback
- Dropped io-channel-output-names from the bindings document. The property is
  not used in the code, and it is not entirely clear what it would be used for.
  If there is a need for it, we can add it back in later on.
- Don't export OF specific API calls
- For OF support, no longer depend on iio_map
- Add #ifdef CONFIG_OF where appropriate, and ensure that the code still builds
  if it is not selected.
- Change iio_channel_get to take device pointer as argument instead of device
  name. Retain old API as of_iio_channel_get_sys.
- iio_channel_get now works for both OF and non-OF configurations.

 .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
 drivers/iio/inkern.c                               |  186 ++++++++++++++++++++
 2 files changed, 262 insertions(+)
 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..58df5f6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
@@ -0,0 +1,76 @@
+This binding is a work-in-progress. It is derived from clock bindings,
+and based on suggestions from Lars-Peter Clausen [1].
+
+Sources of IIO channels can be represented by any node in the device
+tree.  Those nodes are designated as IIO providers.  IIO consumer
+nodes use a phandle and IIO specifier pair to connect IIO provider
+outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
+specifier is an array of one or more cells identifying the IIO
+output on a device.  The length of an IIO specifier is defined by the
+value of a #io-channel-cells property in the 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.
+
+For example:
+
+    adc: adc@35 {
+	compatible = "maxim,max1139";
+	reg = <0x35>;
+        #io-channel-cells = <1>;
+    };
+
+==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 b289915..d48f2a8 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"
@@ -92,6 +93,179 @@ static const struct iio_chan_spec
 	return chan;
 }
 
+#ifdef CONFIG_OF
+
+static int iio_dev_node_match(struct device *dev, void *data)
+{
+	return !strcmp(dev->type->name, "iio_device") && dev->of_node == data;
+}
+
+static struct iio_channel *of_iio_channel_get(struct device_node *np, int index)
+{
+	struct iio_channel *channel;
+	struct device *idev;
+	struct iio_dev *indio_dev;
+	int err;
+	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);
+
+	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
+			       iio_dev_node_match);
+	of_node_put(iiospec.np);
+	if (idev == NULL)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	indio_dev = dev_to_iio_dev(idev);
+
+	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
+	if (channel == NULL) {
+		err = -ENOMEM;
+		goto err_no_mem;
+	}
+
+	channel->indio_dev = indio_dev;
+	index = iiospec.args_count ? iiospec.args[0] : 0;
+	if (index >= indio_dev->num_channels) {
+		err = -EINVAL;
+		goto err_no_channel;
+	}
+	channel->channel = &indio_dev->channels[index];
+
+	return channel;
+
+err_no_channel:
+	kfree(channel);
+err_no_mem:
+	iio_device_put(indio_dev);
+	return ERR_PTR(err);
+}
+
+static struct iio_channel *of_iio_channel_get_by_name(struct device_node *np,
+						      const char *name)
+{
+	struct iio_channel *chan = ERR_PTR(-ENOENT);
+
+	/* Walk up the tree of devices looking for a matching iio channel */
+	while (np) {
+		int index = 0;
+
+		/*
+		 * For named iio channels, first look up the name in the
+		 * "io-channel-names" property.  If it cannot be found, the
+		 * index will be an error code, and of_iio_channel_get()
+		 * will fail.
+		 */
+		if (name)
+			index = of_property_match_string(np, "io-channel-names",
+							 name);
+		chan = of_iio_channel_get(np, index);
+		if (!IS_ERR(chan))
+			break;
+		else if (name && index >= 0) {
+			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
+				np->full_name, name ? name : "", index);
+			return chan;
+		}
+
+		/*
+		 * No matching IIO channel found on this node.
+		 * If the parent node has a "io-channel-ranges" property,
+		 * then we can try one of its channels.
+		 */
+		np = np->parent;
+		if (np && !of_get_property(np, "io-channel-ranges", NULL))
+			break;
+	}
+	return chan;
+}
+
+static struct iio_channel *of_iio_channel_get_all(struct device *dev)
+{
+	struct iio_channel *chans;
+	int i, mapind, nummaps = 0;
+	int ret;
+
+	do {
+		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)	/* no error, return NULL to search map table */
+		return NULL;
+
+	/* NULL terminated array to save passing size */
+	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
+	if (chans == NULL) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	/* Search for OF matches */
+	for (mapind = 0; mapind < nummaps; mapind++) {
+		struct device *idev;
+		struct iio_dev *indio_dev;
+		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;
+
+		idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
+				       iio_dev_node_match);
+		of_node_put(iiospec.np);
+		if (idev == NULL) {
+			ret = -EPROBE_DEFER;
+			goto error_free_chans;
+		}
+		indio_dev = dev_to_iio_dev(idev);
+		channel = iiospec.args_count ? iiospec.args[0] : 0;
+		if (channel >= indio_dev->num_channels) {
+			ret = -EINVAL;
+			goto error_free_chans;
+		}
+		chans[mapind].indio_dev = indio_dev;
+		chans[mapind].channel = &indio_dev->channels[channel];
+	}
+	return chans;
+
+error_free_chans:
+	for (i = 0; i < mapind; i++)
+		iio_device_put(chans[i].indio_dev);
+	kfree(chans);
+error:
+	return ERR_PTR(ret);
+}
+
+#else /* CONFIG_OF */
+
+static inline struct iio_channel *
+of_iio_channel_get_by_name(struct device_node *np, const char *name)
+{
+	return ERR_PTR(-ENOENT);
+}
+
+static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
+{
+	return NULL;
+}
+
+#endif /* CONFIG_OF */
 
 static struct iio_channel *iio_channel_get_sys(const char *name,
 					       const char *channel_name)
@@ -150,7 +324,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
 				    const char *channel_name)
 {
 	const char *name = dev ? dev_name(dev) : NULL;
+	struct iio_channel *channel;
 
+	if (dev) {
+		channel = of_iio_channel_get_by_name(dev->of_node,
+						     channel_name);
+		if (!IS_ERR(channel))
+			return channel;
+	}
 	return iio_channel_get_sys(name, channel_name);
 }
 EXPORT_SYMBOL_GPL(iio_channel_get);
@@ -173,6 +354,11 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
 
 	if (dev == NULL)
 		return ERR_PTR(-EINVAL);
+
+	chans = of_iio_channel_get_all(dev);
+	if (chans)
+		return chans;
+
 	name = dev_name(dev);
 
 	mutex_lock(&iio_map_list_lock);
-- 
1.7.9.7

Re: [PATCH v2 4/4] iio: Add OF support

[Tomasz Figa]

Hi Guenter,

Some comments inline.

On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> Provide bindings and parse OF data during initialization.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
> - Documentation update per feedback
> - Dropped io-channel-output-names from the bindings document. The
> property is not used in the code, and it is not entirely clear what it
> would be used for. If there is a need for it, we can add it back in
> later on.
> - Don't export OF specific API calls
> - For OF support, no longer depend on iio_map
> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
> builds if it is not selected.
> - Change iio_channel_get to take device pointer as argument instead of
> device name. Retain old API as of_iio_channel_get_sys.
> - iio_channel_get now works for both OF and non-OF configurations.
> 
>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
>  drivers/iio/inkern.c                               |  186
> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>  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..58df5f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,76 @@
> +This binding is a work-in-progress. It is derived from clock bindings,
> +and based on suggestions from Lars-Peter Clausen [1].
> +
> +Sources of IIO channels can be represented by any node in the device
> +tree.  Those nodes are designated as IIO providers.  IIO consumer
> +nodes use a phandle and IIO specifier pair to connect IIO provider
> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> +specifier is an array of one or more cells identifying the IIO
> +output on a device.  The length of an IIO specifier is defined by the
> +value of a #io-channel-cells property in the 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.
> +
> +For example:
> +
> +    adc: adc@35 {
> +	compatible = "maxim,max1139";
> +	reg = <0x35>;
> +        #io-channel-cells = <1>;
> +    };
> +
> +==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 b289915..d48f2a8 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"
> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>  	return chan;
>  }
> 
> +#ifdef CONFIG_OF
> +
> +static int iio_dev_node_match(struct device *dev, void *data)
> +{
> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node == 
data;

Hmm, do you need to check type name here? One device node should rather 
represent only one device, making node an unique identifier.

It this is meant to be a sanity check, it could be done one time after 
finding the device.

> +}
> +
> +static struct iio_channel *of_iio_channel_get(struct device_node *np,
> int index) +{
> +	struct iio_channel *channel;
> +	struct device *idev;
> +	struct iio_dev *indio_dev;
> +	int err;
> +	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);
> +
> +	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> +			       iio_dev_node_match);
> +	of_node_put(iiospec.np);
> +	if (idev == NULL)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	indio_dev = dev_to_iio_dev(idev);
> +
> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> +	if (channel == NULL) {
> +		err = -ENOMEM;
> +		goto err_no_mem;
> +	}
> +
> +	channel->indio_dev = indio_dev;
> +	index = iiospec.args_count ? iiospec.args[0] : 0;

What happens here with remaining phandle arguments?

I'm not sure if such use case is needed for iio, but other subsystems give 
the possibility of specifying custom xlate callback translating from a 
custom specifier into channel number. (e.g. drivers/gpio/gpiolib-of.c)

Best regards,
Tomasz

> +	if (index >= indio_dev->num_channels) {
> +		err = -EINVAL;
> +		goto err_no_channel;
> +	}
> +	channel->channel = &indio_dev->channels[index];
> +
> +	return channel;
> +
> +err_no_channel:
> +	kfree(channel);
> +err_no_mem:
> +	iio_device_put(indio_dev);
> +	return ERR_PTR(err);
> +}
> +
> +static struct iio_channel *of_iio_channel_get_by_name(struct
> device_node *np, +						      const char 
*name)
> +{
> +	struct iio_channel *chan = ERR_PTR(-ENOENT);
> +
> +	/* Walk up the tree of devices looking for a matching iio channel */
> +	while (np) {
> +		int index = 0;
> +
> +		/*
> +		 * For named iio channels, first look up the name in the
> +		 * "io-channel-names" property.  If it cannot be found, the
> +		 * index will be an error code, and of_iio_channel_get()
> +		 * will fail.
> +		 */
> +		if (name)
> +			index = of_property_match_string(np, "io-channel-names",
> +							 name);
> +		chan = of_iio_channel_get(np, index);
> +		if (!IS_ERR(chan))
> +			break;
> +		else if (name && index >= 0) {
> +			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
> +				np->full_name, name ? name : "", index);
> +			return chan;
> +		}
> +
> +		/*
> +		 * No matching IIO channel found on this node.
> +		 * If the parent node has a "io-channel-ranges" property,
> +		 * then we can try one of its channels.
> +		 */
> +		np = np->parent;
> +		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> +			break;
> +	}
> +	return chan;
> +}
> +
> +static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> +{
> +	struct iio_channel *chans;
> +	int i, mapind, nummaps = 0;
> +	int ret;
> +
> +	do {
> +		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)	/* no error, return NULL to search map table */
> +		return NULL;
> +
> +	/* NULL terminated array to save passing size */
> +	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
> +	if (chans == NULL) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	/* Search for OF matches */
> +	for (mapind = 0; mapind < nummaps; mapind++) {
> +		struct device *idev;
> +		struct iio_dev *indio_dev;
> +		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;
> +
> +		idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> +				       iio_dev_node_match);
> +		of_node_put(iiospec.np);
> +		if (idev == NULL) {
> +			ret = -EPROBE_DEFER;
> +			goto error_free_chans;
> +		}
> +		indio_dev = dev_to_iio_dev(idev);
> +		channel = iiospec.args_count ? iiospec.args[0] : 0;
> +		if (channel >= indio_dev->num_channels) {
> +			ret = -EINVAL;
> +			goto error_free_chans;
> +		}
> +		chans[mapind].indio_dev = indio_dev;
> +		chans[mapind].channel = &indio_dev->channels[channel];
> +	}
> +	return chans;
> +
> +error_free_chans:
> +	for (i = 0; i < mapind; i++)
> +		iio_device_put(chans[i].indio_dev);
> +	kfree(chans);
> +error:
> +	return ERR_PTR(ret);
> +}
> +
> +#else /* CONFIG_OF */
> +
> +static inline struct iio_channel *
> +of_iio_channel_get_by_name(struct device_node *np, const char *name)
> +{
> +	return ERR_PTR(-ENOENT);
> +}
> +
> +static inline struct iio_channel *of_iio_channel_get_all(struct device
> *dev) +{
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_OF */
> 
>  static struct iio_channel *iio_channel_get_sys(const char *name,
>  					       const char *channel_name)
> @@ -150,7 +324,14 @@ struct iio_channel *iio_channel_get(struct device
> *dev, const char *channel_name)
>  {
>  	const char *name = dev ? dev_name(dev) : NULL;
> +	struct iio_channel *channel;
> 
> +	if (dev) {
> +		channel = of_iio_channel_get_by_name(dev->of_node,
> +						     channel_name);
> +		if (!IS_ERR(channel))
> +			return channel;
> +	}
>  	return iio_channel_get_sys(name, channel_name);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_get);
> @@ -173,6 +354,11 @@ struct iio_channel *iio_channel_get_all(struct
> device *dev)
> 
>  	if (dev == NULL)
>  		return ERR_PTR(-EINVAL);
> +
> +	chans = of_iio_channel_get_all(dev);
> +	if (chans)
> +		return chans;
> +
>  	name = dev_name(dev);
> 
>  	mutex_lock(&iio_map_list_lock);

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> Hi Guenter,
> 
> Some comments inline.
> 
> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> > Provide bindings and parse OF data during initialization.
> > 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > ---
> > - Documentation update per feedback
> > - Dropped io-channel-output-names from the bindings document. The
> > property is not used in the code, and it is not entirely clear what it
> > would be used for. If there is a need for it, we can add it back in
> > later on.
> > - Don't export OF specific API calls
> > - For OF support, no longer depend on iio_map
> > - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
> > builds if it is not selected.
> > - Change iio_channel_get to take device pointer as argument instead of
> > device name. Retain old API as of_iio_channel_get_sys.
> > - iio_channel_get now works for both OF and non-OF configurations.
> > 
> >  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
> >  drivers/iio/inkern.c                               |  186
> > ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> >  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..58df5f6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -0,0 +1,76 @@
> > +This binding is a work-in-progress. It is derived from clock bindings,
> > +and based on suggestions from Lars-Peter Clausen [1].
> > +
> > +Sources of IIO channels can be represented by any node in the device
> > +tree.  Those nodes are designated as IIO providers.  IIO consumer
> > +nodes use a phandle and IIO specifier pair to connect IIO provider
> > +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > +specifier is an array of one or more cells identifying the IIO
> > +output on a device.  The length of an IIO specifier is defined by the
> > +value of a #io-channel-cells property in the 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.
> > +
> > +For example:
> > +
> > +    adc: adc@35 {
> > +	compatible = "maxim,max1139";
> > +	reg = <0x35>;
> > +        #io-channel-cells = <1>;
> > +    };
> > +
> > +==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 b289915..d48f2a8 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"
> > @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> >  	return chan;
> >  }
> > 
> > +#ifdef CONFIG_OF
> > +
> > +static int iio_dev_node_match(struct device *dev, void *data)
> > +{
> > +	return !strcmp(dev->type->name, "iio_device") && dev->of_node == 
> data;
> 
> Hmm, do you need to check type name here? One device node should rather 
> represent only one device, making node an unique identifier.
> 
> It this is meant to be a sanity check, it could be done one time after 
> finding the device.
> 
Hi Tomasz,

This is what Lars had suggested earlier:

> 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.

Is it really needed, or in other words would it be sufficient to check if
of_node and data match each other ? Your reasoning makes sense to me, and I had
thought about it as well, but I don't really know, and I don't know how I could
test it and guarantee correctness either. I'll be happy to take the strcmp() out
if someone tells me that it is definitely not needed ...

> > +}
> > +
> > +static struct iio_channel *of_iio_channel_get(struct device_node *np,
> > int index) +{
> > +	struct iio_channel *channel;
> > +	struct device *idev;
> > +	struct iio_dev *indio_dev;
> > +	int err;
> > +	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);
> > +
> > +	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> > +			       iio_dev_node_match);
> > +	of_node_put(iiospec.np);
> > +	if (idev == NULL)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	indio_dev = dev_to_iio_dev(idev);
> > +
> > +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> > +	if (channel == NULL) {
> > +		err = -ENOMEM;
> > +		goto err_no_mem;
> > +	}
> > +
> > +	channel->indio_dev = indio_dev;
> > +	index = iiospec.args_count ? iiospec.args[0] : 0;
> 
> What happens here with remaining phandle arguments?
> 
> I'm not sure if such use case is needed for iio, but other subsystems give 
> the possibility of specifying custom xlate callback translating from a 
> custom specifier into channel number. (e.g. drivers/gpio/gpiolib-of.c)
> 
I don't have a use case, and I hesitate to implement something that may
never be used (and I can not really test it either). And without a use
case we would not even know if the implementation makes sense or not.

It should be possible to add additional functionality later if needed.

Thanks,
Guenter

> Best regards,
> Tomasz
> 
> > +	if (index >= indio_dev->num_channels) {
> > +		err = -EINVAL;
> > +		goto err_no_channel;
> > +	}
> > +	channel->channel = &indio_dev->channels[index];
> > +
> > +	return channel;
> > +
> > +err_no_channel:
> > +	kfree(channel);
> > +err_no_mem:
> > +	iio_device_put(indio_dev);
> > +	return ERR_PTR(err);
> > +}
> > +
> > +static struct iio_channel *of_iio_channel_get_by_name(struct
> > device_node *np, +						      const char 
> *name)
> > +{
> > +	struct iio_channel *chan = ERR_PTR(-ENOENT);
> > +
> > +	/* Walk up the tree of devices looking for a matching iio channel */
> > +	while (np) {
> > +		int index = 0;
> > +
> > +		/*
> > +		 * For named iio channels, first look up the name in the
> > +		 * "io-channel-names" property.  If it cannot be found, the
> > +		 * index will be an error code, and of_iio_channel_get()
> > +		 * will fail.
> > +		 */
> > +		if (name)
> > +			index = of_property_match_string(np, "io-channel-names",
> > +							 name);
> > +		chan = of_iio_channel_get(np, index);
> > +		if (!IS_ERR(chan))
> > +			break;
> > +		else if (name && index >= 0) {
> > +			pr_err("ERROR: could not get IIO channel %s:%s(%i)\n",
> > +				np->full_name, name ? name : "", index);
> > +			return chan;
> > +		}
> > +
> > +		/*
> > +		 * No matching IIO channel found on this node.
> > +		 * If the parent node has a "io-channel-ranges" property,
> > +		 * then we can try one of its channels.
> > +		 */
> > +		np = np->parent;
> > +		if (np && !of_get_property(np, "io-channel-ranges", NULL))
> > +			break;
> > +	}
> > +	return chan;
> > +}
> > +
> > +static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> > +{
> > +	struct iio_channel *chans;
> > +	int i, mapind, nummaps = 0;
> > +	int ret;
> > +
> > +	do {
> > +		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)	/* no error, return NULL to search map table */
> > +		return NULL;
> > +
> > +	/* NULL terminated array to save passing size */
> > +	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
> > +	if (chans == NULL) {
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	/* Search for OF matches */
> > +	for (mapind = 0; mapind < nummaps; mapind++) {
> > +		struct device *idev;
> > +		struct iio_dev *indio_dev;
> > +		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;
> > +
> > +		idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> > +				       iio_dev_node_match);
> > +		of_node_put(iiospec.np);
> > +		if (idev == NULL) {
> > +			ret = -EPROBE_DEFER;
> > +			goto error_free_chans;
> > +		}
> > +		indio_dev = dev_to_iio_dev(idev);
> > +		channel = iiospec.args_count ? iiospec.args[0] : 0;
> > +		if (channel >= indio_dev->num_channels) {
> > +			ret = -EINVAL;
> > +			goto error_free_chans;
> > +		}
> > +		chans[mapind].indio_dev = indio_dev;
> > +		chans[mapind].channel = &indio_dev->channels[channel];
> > +	}
> > +	return chans;
> > +
> > +error_free_chans:
> > +	for (i = 0; i < mapind; i++)
> > +		iio_device_put(chans[i].indio_dev);
> > +	kfree(chans);
> > +error:
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +#else /* CONFIG_OF */
> > +
> > +static inline struct iio_channel *
> > +of_iio_channel_get_by_name(struct device_node *np, const char *name)
> > +{
> > +	return ERR_PTR(-ENOENT);
> > +}
> > +
> > +static inline struct iio_channel *of_iio_channel_get_all(struct device
> > *dev) +{
> > +	return NULL;
> > +}
> > +
> > +#endif /* CONFIG_OF */
> > 
> >  static struct iio_channel *iio_channel_get_sys(const char *name,
> >  					       const char *channel_name)
> > @@ -150,7 +324,14 @@ struct iio_channel *iio_channel_get(struct device
> > *dev, const char *channel_name)
> >  {
> >  	const char *name = dev ? dev_name(dev) : NULL;
> > +	struct iio_channel *channel;
> > 
> > +	if (dev) {
> > +		channel = of_iio_channel_get_by_name(dev->of_node,
> > +						     channel_name);
> > +		if (!IS_ERR(channel))
> > +			return channel;
> > +	}
> >  	return iio_channel_get_sys(name, channel_name);
> >  }
> >  EXPORT_SYMBOL_GPL(iio_channel_get);
> > @@ -173,6 +354,11 @@ struct iio_channel *iio_channel_get_all(struct
> > device *dev)
> > 
> >  	if (dev == NULL)
> >  		return ERR_PTR(-EINVAL);
> > +
> > +	chans = of_iio_channel_get_all(dev);
> > +	if (chans)
> > +		return chans;
> > +
> >  	name = dev_name(dev);
> > 
> >  	mutex_lock(&iio_map_list_lock);
> 

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>> Hi Guenter,
>>
>> Some comments inline.
>>
>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>> Provide bindings and parse OF data during initialization.
>>>
>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>> ---
>>> - Documentation update per feedback
>>> - Dropped io-channel-output-names from the bindings document. The
>>> property is not used in the code, and it is not entirely clear what it
>>> would be used for. If there is a need for it, we can add it back in
>>> later on.
>>> - Don't export OF specific API calls
>>> - For OF support, no longer depend on iio_map
>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
>>> builds if it is not selected.
>>> - Change iio_channel_get to take device pointer as argument instead of
>>> device name. Retain old API as of_iio_channel_get_sys.
>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>
>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
>>>  drivers/iio/inkern.c                               |  186
>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>  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..58df5f6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>> @@ -0,0 +1,76 @@
>>> +This binding is a work-in-progress. It is derived from clock bindings,
>>> +and based on suggestions from Lars-Peter Clausen [1].
>>> +
>>> +Sources of IIO channels can be represented by any node in the device
>>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>> +specifier is an array of one or more cells identifying the IIO
>>> +output on a device.  The length of an IIO specifier is defined by the
>>> +value of a #io-channel-cells property in the 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.
>>> +
>>> +For example:
>>> +
>>> +    adc: adc@35 {
>>> +	compatible = "maxim,max1139";
>>> +	reg = <0x35>;
>>> +        #io-channel-cells = <1>;
>>> +    };
>>> +
>>> +==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 b289915..d48f2a8 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"
>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>  	return chan;
>>>  }
>>>
>>> +#ifdef CONFIG_OF
>>> +
>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>> +{
>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node == 
>> data;
>>
>> Hmm, do you need to check type name here? One device node should rather 
>> represent only one device, making node an unique identifier.
>>
>> It this is meant to be a sanity check, it could be done one time after 
>> finding the device.
>>
> Hi Tomasz,
> 
> This is what Lars had suggested earlier:
> 
>> 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.
> 
> Is it really needed, or in other words would it be sufficient to check if
> of_node and data match each other ? Your reasoning makes sense to me, and I had
> thought about it as well, but I don't really know, and I don't know how I could
> test it and guarantee correctness either. I'll be happy to take the strcmp() out
> if someone tells me that it is definitely not needed ...

A IIO trigger and a IIO device may have the same of_node, e.g. if they both
belong to the same physical device. But you don't need to do the strcmp just
compare dev->type to iio_dev_type i.e. dev->type == &iio_dev_type. Although it
doesn't really matter in practice first check for the of_node then check for
the type, since the of_node will only match for a few devices at most, the type
will match for quite a few.


> 
>>> +}
>>> +
>>> +static struct iio_channel *of_iio_channel_get(struct device_node *np,
>>> int index) +{
>>> +	struct iio_channel *channel;
>>> +	struct device *idev;
>>> +	struct iio_dev *indio_dev;
>>> +	int err;
>>> +	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);
>>> +
>>> +	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
>>> +			       iio_dev_node_match);
>>> +	of_node_put(iiospec.np);
>>> +	if (idev == NULL)
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> +	indio_dev = dev_to_iio_dev(idev);
>>> +
>>> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
>>> +	if (channel == NULL) {
>>> +		err = -ENOMEM;
>>> +		goto err_no_mem;
>>> +	}
>>> +
>>> +	channel->indio_dev = indio_dev;
>>> +	index = iiospec.args_count ? iiospec.args[0] : 0;
>>
>> What happens here with remaining phandle arguments?
>>
>> I'm not sure if such use case is needed for iio, but other subsystems give 
>> the possibility of specifying custom xlate callback translating from a 
>> custom specifier into channel number. (e.g. drivers/gpio/gpiolib-of.c)
>>
> I don't have a use case, and I hesitate to implement something that may
> never be used (and I can not really test it either). And without a use
> case we would not even know if the implementation makes sense or not.
> 
> It should be possible to add additional functionality later if needed.
> 

Agreed, if it turns out somebody needs it, we can easily add it later on.

- Lars

Re: [PATCH v2 4/4] iio: Add OF support

[Tomasz Figa]

On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> >> Hi Guenter,
> >> 
> >> Some comments inline.
> >> 
> >> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> >>> Provide bindings and parse OF data during initialization.
> >>> 
> >>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >>> ---
> >>> - Documentation update per feedback
> >>> - Dropped io-channel-output-names from the bindings document. The
> >>> property is not used in the code, and it is not entirely clear what
> >>> it
> >>> would be used for. If there is a need for it, we can add it back in
> >>> later on.
> >>> - Don't export OF specific API calls
> >>> - For OF support, no longer depend on iio_map
> >>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code
> >>> still builds if it is not selected.
> >>> - Change iio_channel_get to take device pointer as argument instead
> >>> of
> >>> device name. Retain old API as of_iio_channel_get_sys.
> >>> - iio_channel_get now works for both OF and non-OF configurations.
> >>> 
> >>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
> >>>  drivers/iio/inkern.c                               |  186
> >>> 
> >>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> >>> 
> >>>  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..58df5f6
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> >>> @@ -0,0 +1,76 @@
> >>> +This binding is a work-in-progress. It is derived from clock
> >>> bindings,
> >>> +and based on suggestions from Lars-Peter Clausen [1].
> >>> +
> >>> +Sources of IIO channels can be represented by any node in the
> >>> device
> >>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
> >>> +nodes use a phandle and IIO specifier pair to connect IIO provider
> >>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> >>> +specifier is an array of one or more cells identifying the IIO
> >>> +output on a device.  The length of an IIO specifier is defined by
> >>> the
> >>> +value of a #io-channel-cells property in the 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.
> >>> +
> >>> +For example:
> >>> +
> >>> +    adc: adc@35 {
> >>> +	compatible = "maxim,max1139";
> >>> +	reg = <0x35>;
> >>> +        #io-channel-cells = <1>;
> >>> +    };
> >>> +
> >>> +==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 b289915..d48f2a8 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"
> >>> 
> >>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> >>> 
> >>>  	return chan;
> >>>  
> >>>  }
> >>> 
> >>> +#ifdef CONFIG_OF
> >>> +
> >>> +static int iio_dev_node_match(struct device *dev, void *data)
> >>> +{
> >>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node 
==
> >> 
> >> data;
> >> 
> >> Hmm, do you need to check type name here? One device node should
> >> rather
> >> represent only one device, making node an unique identifier.
> >> 
> >> It this is meant to be a sanity check, it could be done one time
> >> after
> >> finding the device.
> > 
> > Hi Tomasz,
> > 
> > This is what Lars had suggested earlier:
> >> 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.
> > 
> > Is it really needed, or in other words would it be sufficient to check
> > if of_node and data match each other ? Your reasoning makes sense to
> > me, and I had thought about it as well, but I don't really know, and
> > I don't know how I could test it and guarantee correctness either.
> > I'll be happy to take the strcmp() out if someone tells me that it is
> > definitely not needed ...
> 
> A IIO trigger and a IIO device may have the same of_node, e.g. if they
> both belong to the same physical device. But you don't need to do the
> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> &iio_dev_type. Although it doesn't really matter in practice first
> check for the of_node then check for the type, since the of_node will
> only match for a few devices at most, the type will match for quite a
> few.

I must disagree.

If you have two IIO devices provided by one physical device, then in 
device tree they should be represented as follows:

	phys-dev@12345678 {
		compatible = "some-physical-device";
		/* ... */

		my_trig: iio-trigger {
			/* ... */
		};

		my_dev: iio-device {
			/* ... */
		};
	};

Notice that phys-dev works here as an IIO bus on which its IIO devices are 
available. This is related to the convention that single OF device node 
represents single device, which would be violated otherwise.

> >>> +}
> >>> +
> >>> +static struct iio_channel *of_iio_channel_get(struct device_node
> >>> *np,
> >>> int index) +{
> >>> +	struct iio_channel *channel;
> >>> +	struct device *idev;
> >>> +	struct iio_dev *indio_dev;
> >>> +	int err;
> >>> +	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);
> >>> +
> >>> +	idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> >>> +			       iio_dev_node_match);
> >>> +	of_node_put(iiospec.np);
> >>> +	if (idev == NULL)
> >>> +		return ERR_PTR(-EPROBE_DEFER);
> >>> +
> >>> +	indio_dev = dev_to_iio_dev(idev);
> >>> +
> >>> +	channel = kzalloc(sizeof(*channel), GFP_KERNEL);
> >>> +	if (channel == NULL) {
> >>> +		err = -ENOMEM;
> >>> +		goto err_no_mem;
> >>> +	}
> >>> +
> >>> +	channel->indio_dev = indio_dev;
> >>> +	index = iiospec.args_count ? iiospec.args[0] : 0;
> >> 
> >> What happens here with remaining phandle arguments?
> >> 
> >> I'm not sure if such use case is needed for iio, but other subsystems
> >> give the possibility of specifying custom xlate callback translating
> >> from a custom specifier into channel number. (e.g.
> >> drivers/gpio/gpiolib-of.c)> 
> > I don't have a use case, and I hesitate to implement something that
> > may
> > never be used (and I can not really test it either). And without a use
> > case we would not even know if the implementation makes sense or not.
> > 
> > It should be possible to add additional functionality later if needed.
> 
> Agreed, if it turns out somebody needs it, we can easily add it later
> on.

OK. I'm not much into IIO, so my suggestion was based entirely on what 
other subsystems do.

Best regards,
Tomasz

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 12:52 PM, Tomasz Figa wrote:
> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>> Hi Guenter,
>>>>
>>>> Some comments inline.
>>>>
>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>> Provide bindings and parse OF data during initialization.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>> ---
>>>>> - Documentation update per feedback
>>>>> - Dropped io-channel-output-names from the bindings document. The
>>>>> property is not used in the code, and it is not entirely clear what
>>>>> it
>>>>> would be used for. If there is a need for it, we can add it back in
>>>>> later on.
>>>>> - Don't export OF specific API calls
>>>>> - For OF support, no longer depend on iio_map
>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code
>>>>> still builds if it is not selected.
>>>>> - Change iio_channel_get to take device pointer as argument instead
>>>>> of
>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>>>
>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
>>>>>  drivers/iio/inkern.c                               |  186
>>>>>
>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>
>>>>>  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..58df5f6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,76 @@
>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>> bindings,
>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>> +
>>>>> +Sources of IIO channels can be represented by any node in the
>>>>> device
>>>>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
>>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>> +output on a device.  The length of an IIO specifier is defined by
>>>>> the
>>>>> +value of a #io-channel-cells property in the 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.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +    adc: adc@35 {
>>>>> +	compatible = "maxim,max1139";
>>>>> +	reg = <0x35>;
>>>>> +        #io-channel-cells = <1>;
>>>>> +    };
>>>>> +
>>>>> +==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 b289915..d48f2a8 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"
>>>>>
>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>
>>>>>  	return chan;
>>>>>  
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_OF
>>>>> +
>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>> +{
>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node 
> ==
>>>>
>>>> data;
>>>>
>>>> Hmm, do you need to check type name here? One device node should
>>>> rather
>>>> represent only one device, making node an unique identifier.
>>>>
>>>> It this is meant to be a sanity check, it could be done one time
>>>> after
>>>> finding the device.
>>>
>>> Hi Tomasz,
>>>
>>> This is what Lars had suggested earlier:
>>>> 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.
>>>
>>> Is it really needed, or in other words would it be sufficient to check
>>> if of_node and data match each other ? Your reasoning makes sense to
>>> me, and I had thought about it as well, but I don't really know, and
>>> I don't know how I could test it and guarantee correctness either.
>>> I'll be happy to take the strcmp() out if someone tells me that it is
>>> definitely not needed ...
>>
>> A IIO trigger and a IIO device may have the same of_node, e.g. if they
>> both belong to the same physical device. But you don't need to do the
>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
>> &iio_dev_type. Although it doesn't really matter in practice first
>> check for the of_node then check for the type, since the of_node will
>> only match for a few devices at most, the type will match for quite a
>> few.
> 
> I must disagree.
> 
> If you have two IIO devices provided by one physical device, then in 
> device tree they should be represented as follows:
> 
> 	phys-dev@12345678 {
> 		compatible = "some-physical-device";
> 		/* ... */
> 
> 		my_trig: iio-trigger {
> 			/* ... */
> 		};
> 
> 		my_dev: iio-device {
> 			/* ... */
> 		};
> 	};
> 
> Notice that phys-dev works here as an IIO bus on which its IIO devices are 
> available. This is related to the convention that single OF device node 
> represents single device, which would be violated otherwise.

But we do some_dev->of_node = some_other_dev->of_node, all over the place. How
is that different?

- Lars

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> > On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > > On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > >> Hi Guenter,
> > >> 
> > >> Some comments inline.
> > >> 
> > >> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> > >>> Provide bindings and parse OF data during initialization.
> > >>> 
> > >>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > >>> ---
> > >>> - Documentation update per feedback
> > >>> - Dropped io-channel-output-names from the bindings document. The
> > >>> property is not used in the code, and it is not entirely clear what
> > >>> it
> > >>> would be used for. If there is a need for it, we can add it back in
> > >>> later on.
> > >>> - Don't export OF specific API calls
> > >>> - For OF support, no longer depend on iio_map
> > >>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code
> > >>> still builds if it is not selected.
> > >>> - Change iio_channel_get to take device pointer as argument instead
> > >>> of
> > >>> device name. Retain old API as of_iio_channel_get_sys.
> > >>> - iio_channel_get now works for both OF and non-OF configurations.
> > >>> 
> > >>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
> > >>>  drivers/iio/inkern.c                               |  186
> > >>> 
> > >>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > >>> 
> > >>>  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..58df5f6
> > >>> --- /dev/null
> > >>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > >>> @@ -0,0 +1,76 @@
> > >>> +This binding is a work-in-progress. It is derived from clock
> > >>> bindings,
> > >>> +and based on suggestions from Lars-Peter Clausen [1].
> > >>> +
> > >>> +Sources of IIO channels can be represented by any node in the
> > >>> device
> > >>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
> > >>> +nodes use a phandle and IIO specifier pair to connect IIO provider
> > >>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > >>> +specifier is an array of one or more cells identifying the IIO
> > >>> +output on a device.  The length of an IIO specifier is defined by
> > >>> the
> > >>> +value of a #io-channel-cells property in the 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.
> > >>> +
> > >>> +For example:
> > >>> +
> > >>> +    adc: adc@35 {
> > >>> +	compatible = "maxim,max1139";
> > >>> +	reg = <0x35>;
> > >>> +        #io-channel-cells = <1>;
> > >>> +    };
> > >>> +
> > >>> +==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 b289915..d48f2a8 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"
> > >>> 
> > >>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > >>> 
> > >>>  	return chan;
> > >>>  
> > >>>  }
> > >>> 
> > >>> +#ifdef CONFIG_OF
> > >>> +
> > >>> +static int iio_dev_node_match(struct device *dev, void *data)
> > >>> +{
> > >>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node 
> ==
> > >> 
> > >> data;
> > >> 
> > >> Hmm, do you need to check type name here? One device node should
> > >> rather
> > >> represent only one device, making node an unique identifier.
> > >> 
> > >> It this is meant to be a sanity check, it could be done one time
> > >> after
> > >> finding the device.
> > > 
> > > Hi Tomasz,
> > > 
> > > This is what Lars had suggested earlier:
> > >> 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.
> > > 
> > > Is it really needed, or in other words would it be sufficient to check
> > > if of_node and data match each other ? Your reasoning makes sense to
> > > me, and I had thought about it as well, but I don't really know, and
> > > I don't know how I could test it and guarantee correctness either.
> > > I'll be happy to take the strcmp() out if someone tells me that it is
> > > definitely not needed ...
> > 
> > A IIO trigger and a IIO device may have the same of_node, e.g. if they
> > both belong to the same physical device. But you don't need to do the
> > strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> > &iio_dev_type. Although it doesn't really matter in practice first
> > check for the of_node then check for the type, since the of_node will
> > only match for a few devices at most, the type will match for quite a
> > few.
> 
> I must disagree.
> 
> If you have two IIO devices provided by one physical device, then in 
> device tree they should be represented as follows:
> 
> 	phys-dev@12345678 {
> 		compatible = "some-physical-device";
> 		/* ... */
> 
> 		my_trig: iio-trigger {
> 			/* ... */
> 		};
> 
> 		my_dev: iio-device {
> 			/* ... */
> 		};
> 	};
> 
> Notice that phys-dev works here as an IIO bus on which its IIO devices are 
> available. This is related to the convention that single OF device node 
> represents single device, which would be violated otherwise.
> 
Right now the iio device is a child of the physical device, and I am simply
passing of_node on to it. guess you are saying that is not correct ?

If so, what would be the correct approach ? Something like the following ?

	voltage-sensor@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		vcc-supply = <&reg_3p3v>;
		vref-supply = <&reg_3p3v>;

		max1139-iio: iio-device {
			device_type = "iio_device";
			#io-channel-cells = <1>;
		};
	};

and in the driver probe function:

	if (parent->of_node)
		iio_dev->dev.of_node = of_find_node_by_type(parent->of_node, "iio_device");

Another option would be to use of_find_compatible_node() and something like
			compatible = "iio-device";
in the iio-device node.

Which one is preferred ?

Thanks,
Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Tomasz Figa]

On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> > On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> > > On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > > > On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > > >> Hi Guenter,
> > > >> 
> > > >> Some comments inline.
> > > >> 
> > > >> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> > > >>> Provide bindings and parse OF data during initialization.
> > > >>> 
> > > >>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > >>> ---
> > > >>> - Documentation update per feedback
> > > >>> - Dropped io-channel-output-names from the bindings document.
> > > >>> The
> > > >>> property is not used in the code, and it is not entirely clear
> > > >>> what
> > > >>> it
> > > >>> would be used for. If there is a need for it, we can add it back
> > > >>> in
> > > >>> later on.
> > > >>> - Don't export OF specific API calls
> > > >>> - For OF support, no longer depend on iio_map
> > > >>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
> > > >>> code
> > > >>> still builds if it is not selected.
> > > >>> - Change iio_channel_get to take device pointer as argument
> > > >>> instead
> > > >>> of
> > > >>> device name. Retain old API as of_iio_channel_get_sys.
> > > >>> - iio_channel_get now works for both OF and non-OF
> > > >>> configurations.
> > > >>> 
> > > >>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> > > >>>  ++++++++
> > > >>>  drivers/iio/inkern.c                               |  186
> > > >>> 
> > > >>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > > >>> 
> > > >>>  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..58df5f6
> > > >>> --- /dev/null
> > > >>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > > >>> @@ -0,0 +1,76 @@
> > > >>> +This binding is a work-in-progress. It is derived from clock
> > > >>> bindings,
> > > >>> +and based on suggestions from Lars-Peter Clausen [1].
> > > >>> +
> > > >>> +Sources of IIO channels can be represented by any node in the
> > > >>> device
> > > >>> +tree.  Those nodes are designated as IIO providers.  IIO
> > > >>> consumer
> > > >>> +nodes use a phandle and IIO specifier pair to connect IIO
> > > >>> provider
> > > >>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > > >>> +specifier is an array of one or more cells identifying the IIO
> > > >>> +output on a device.  The length of an IIO specifier is defined
> > > >>> by
> > > >>> the
> > > >>> +value of a #io-channel-cells property in the 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.
> > > >>> +
> > > >>> +For example:
> > > >>> +
> > > >>> +    adc: adc@35 {
> > > >>> +	compatible = "maxim,max1139";
> > > >>> +	reg = <0x35>;
> > > >>> +        #io-channel-cells = <1>;
> > > >>> +    };
> > > >>> +
> > > >>> +==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 b289915..d48f2a8 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"
> > > >>> 
> > > >>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > > >>> 
> > > >>>  	return chan;
> > > >>>  
> > > >>>  }
> > > >>> 
> > > >>> +#ifdef CONFIG_OF
> > > >>> +
> > > >>> +static int iio_dev_node_match(struct device *dev, void *data)
> > > >>> +{
> > > >>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node
> > 
> > ==
> > 
> > > >> data;
> > > >> 
> > > >> Hmm, do you need to check type name here? One device node should
> > > >> rather
> > > >> represent only one device, making node an unique identifier.
> > > >> 
> > > >> It this is meant to be a sanity check, it could be done one time
> > > >> after
> > > >> finding the device.
> > > > 
> > > > Hi Tomasz,
> > > > 
> > > > This is what Lars had suggested earlier:
> > > >> 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.
> > > > 
> > > > Is it really needed, or in other words would it be sufficient to
> > > > check
> > > > if of_node and data match each other ? Your reasoning makes sense
> > > > to
> > > > me, and I had thought about it as well, but I don't really know,
> > > > and
> > > > I don't know how I could test it and guarantee correctness either.
> > > > I'll be happy to take the strcmp() out if someone tells me that it
> > > > is
> > > > definitely not needed ...
> > > 
> > > A IIO trigger and a IIO device may have the same of_node, e.g. if
> > > they
> > > both belong to the same physical device. But you don't need to do
> > > the
> > > strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> > > &iio_dev_type. Although it doesn't really matter in practice first
> > > check for the of_node then check for the type, since the of_node
> > > will
> > > only match for a few devices at most, the type will match for quite
> > > a
> > > few.
> > 
> > I must disagree.
> > 
> > If you have two IIO devices provided by one physical device, then in
> > 
> > device tree they should be represented as follows:
> > 	phys-dev@12345678 {
> > 	
> > 		compatible = "some-physical-device";
> > 		/* ... */
> > 		
> > 		my_trig: iio-trigger {
> > 		
> > 			/* ... */
> > 		
> > 		};
> > 		
> > 		my_dev: iio-device {
> > 		
> > 			/* ... */
> > 		
> > 		};
> > 	
> > 	};
> > 
> > Notice that phys-dev works here as an IIO bus on which its IIO devices
> > are available. This is related to the convention that single OF
> > device node represents single device, which would be violated
> > otherwise.
> 
> Right now the iio device is a child of the physical device, and I am
> simply passing of_node on to it. guess you are saying that is not
> correct ?
> 
> If so, what would be the correct approach ? Something like the following
> ?
> 
> 	voltage-sensor@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		vcc-supply = <&reg_3p3v>;
> 		vref-supply = <&reg_3p3v>;
> 
> 		max1139-iio: iio-device {
> 			device_type = "iio_device";
> 			#io-channel-cells = <1>;
> 		};
> 	};
> 
> and in the driver probe function:
> 
> 	if (parent->of_node)
> 		iio_dev->dev.of_node = of_find_node_by_type(parent->of_node,
> "iio_device");
> 
> Another option would be to use of_find_compatible_node() and something
> like compatible = "iio-device";
> in the iio-device node.

A device node is defined as a node having compatible property. Other nodes 
should be seen as helper nodes, which do not represent devices (although 
they all have struct device_node in Linux).

Also, AFAIK, device_type is a deprecated property used by some legacy 
PowerPC machines and for current machines only compatible should be used.

So I guess the approach with compatible would be appropriate here.

However for physical devices providing only a single IIO device it might 
be better to allow simpler specification, like:

 	max1139-iio: voltage-sensor@35 {
 		compatible = "maxim,max1139", "iio_device";
 		reg = <0x35>;
 		vcc-supply = <&reg_3p3v>;
 		vref-supply = <&reg_3p3v>;
		device_type = "iio_device";
 		#io-channel-cells = <1>;
 	};

The node would be parseable using the same code as for subnodes (first try 
to parse current node and if it fails - no iio_device in compatible list - 
parse child nodes), but you could simplify specification of simple 
devices.

Also keep in mind that of_find_compatible_node() and friends iterate over 
the whole flattened device tree (starting from the specified node), not 
just over child nodes of specified node. To iterate over child nodes 
for_each_child_of_node must be used.

The code would then be something like this:

	struct device_node *np;

	ret = iio_of_parse_node(dev->of_node);
	if (!ret)
		return 0;

	for_each_child_of_node(dev->of_node, np)
		iio_of_parse_node(np);

With the check for compatible property inside iio_of_parse_node().

Best regards,
Tomasz

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>>>> Hi Guenter,
>>>>>>
>>>>>> Some comments inline.
>>>>>>
>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>>>> Provide bindings and parse OF data during initialization.
>>>>>>>
>>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>>>> ---
>>>>>>> - Documentation update per feedback
>>>>>>> - Dropped io-channel-output-names from the bindings document.
>>>>>>> The
>>>>>>> property is not used in the code, and it is not entirely clear
>>>>>>> what
>>>>>>> it
>>>>>>> would be used for. If there is a need for it, we can add it back
>>>>>>> in
>>>>>>> later on.
>>>>>>> - Don't export OF specific API calls
>>>>>>> - For OF support, no longer depend on iio_map
>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
>>>>>>> code
>>>>>>> still builds if it is not selected.
>>>>>>> - Change iio_channel_get to take device pointer as argument
>>>>>>> instead
>>>>>>> of
>>>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>>>> - iio_channel_get now works for both OF and non-OF
>>>>>>> configurations.
>>>>>>>
>>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
>>>>>>>  ++++++++
>>>>>>>  drivers/iio/inkern.c                               |  186
>>>>>>>
>>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>>>
>>>>>>>  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..58df5f6
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>>> @@ -0,0 +1,76 @@
>>>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>>>> bindings,
>>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>>>> +
>>>>>>> +Sources of IIO channels can be represented by any node in the
>>>>>>> device
>>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
>>>>>>> consumer
>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
>>>>>>> provider
>>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>>>> +output on a device.  The length of an IIO specifier is defined
>>>>>>> by
>>>>>>> the
>>>>>>> +value of a #io-channel-cells property in the 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.
>>>>>>> +
>>>>>>> +For example:
>>>>>>> +
>>>>>>> +    adc: adc@35 {
>>>>>>> +	compatible = "maxim,max1139";
>>>>>>> +	reg = <0x35>;
>>>>>>> +        #io-channel-cells = <1>;
>>>>>>> +    };
>>>>>>> +
>>>>>>> +==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 b289915..d48f2a8 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"
>>>>>>>
>>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>>>
>>>>>>>  	return chan;
>>>>>>>  
>>>>>>>  }
>>>>>>>
>>>>>>> +#ifdef CONFIG_OF
>>>>>>> +
>>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>>>> +{
>>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node
>>>
>>> ==
>>>
>>>>>> data;
>>>>>>
>>>>>> Hmm, do you need to check type name here? One device node should
>>>>>> rather
>>>>>> represent only one device, making node an unique identifier.
>>>>>>
>>>>>> It this is meant to be a sanity check, it could be done one time
>>>>>> after
>>>>>> finding the device.
>>>>>
>>>>> Hi Tomasz,
>>>>>
>>>>> This is what Lars had suggested earlier:
>>>>>> 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.
>>>>>
>>>>> Is it really needed, or in other words would it be sufficient to
>>>>> check
>>>>> if of_node and data match each other ? Your reasoning makes sense
>>>>> to
>>>>> me, and I had thought about it as well, but I don't really know,
>>>>> and
>>>>> I don't know how I could test it and guarantee correctness either.
>>>>> I'll be happy to take the strcmp() out if someone tells me that it
>>>>> is
>>>>> definitely not needed ...
>>>>
>>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
>>>> they
>>>> both belong to the same physical device. But you don't need to do
>>>> the
>>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
>>>> &iio_dev_type. Although it doesn't really matter in practice first
>>>> check for the of_node then check for the type, since the of_node
>>>> will
>>>> only match for a few devices at most, the type will match for quite
>>>> a
>>>> few.
>>>
>>> I must disagree.
>>>
>>> If you have two IIO devices provided by one physical device, then in
>>>
>>> device tree they should be represented as follows:
>>> 	phys-dev@12345678 {
>>> 	
>>> 		compatible = "some-physical-device";
>>> 		/* ... */
>>> 		
>>> 		my_trig: iio-trigger {
>>> 		
>>> 			/* ... */
>>> 		
>>> 		};
>>> 		
>>> 		my_dev: iio-device {
>>> 		
>>> 			/* ... */
>>> 		
>>> 		};
>>> 	
>>> 	};
>>>
>>> Notice that phys-dev works here as an IIO bus on which its IIO devices
>>> are available. This is related to the convention that single OF
>>> device node represents single device, which would be violated
>>> otherwise.
>>
>> Right now the iio device is a child of the physical device, and I am
>> simply passing of_node on to it. guess you are saying that is not
>> correct ?
>>
>> If so, what would be the correct approach ? Something like the following
>> ?
>>
>> 	voltage-sensor@35 {
>> 		compatible = "maxim,max1139";
>> 		reg = <0x35>;
>> 		vcc-supply = <&reg_3p3v>;
>> 		vref-supply = <&reg_3p3v>;
>>
>> 		max1139-iio: iio-device {
>> 			device_type = "iio_device";
>> 			#io-channel-cells = <1>;
>> 		};
>> 	};
>>
>> and in the driver probe function:
>>
>> 	if (parent->of_node)
>> 		iio_dev->dev.of_node = of_find_node_by_type(parent->of_node,
>> "iio_device");
>>
>> Another option would be to use of_find_compatible_node() and something
>> like compatible = "iio-device";
>> in the iio-device node.
> 
> A device node is defined as a node having compatible property. Other nodes 
> should be seen as helper nodes, which do not represent devices (although 
> they all have struct device_node in Linux).
> 
> Also, AFAIK, device_type is a deprecated property used by some legacy 
> PowerPC machines and for current machines only compatible should be used.
> 
> So I guess the approach with compatible would be appropriate here.
> 
> However for physical devices providing only a single IIO device it might 
> be better to allow simpler specification, like:
> 
>  	max1139-iio: voltage-sensor@35 {
>  		compatible = "maxim,max1139", "iio_device";

I don't think this makes a lot of sense. First of all iio_device an artificial
Linux term, while the device tree should describe the hardware. Secondly there
is no generic iio driver which could match on a node with a "iio_device"
compability string and stuff would just work. I mean we don't do

compatible = "atmel,at91sam9260-i2c", "i2c-master";

or similar either.


>  		reg = <0x35>;
>  		vcc-supply = <&reg_3p3v>;
>  		vref-supply = <&reg_3p3v>;
> 		device_type = "iio_device";
>  		#io-channel-cells = <1>;
>  	};
> 

Re: [PATCH v2 4/4] iio: Add OF support

[Jonathan Cameron]

On 02/03/2013 06:55 PM, Lars-Peter Clausen wrote:
> On 02/03/2013 06:30 PM, Tomasz Figa wrote:
>> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
>>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
>>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>>>>> Hi Guenter,
>>>>>>>
>>>>>>> Some comments inline.
>>>>>>>
>>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>>>>> Provide bindings and parse OF data during initialization.
>>>>>>>>
>>>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>>>>> ---
>>>>>>>> - Documentation update per feedback
>>>>>>>> - Dropped io-channel-output-names from the bindings document.
>>>>>>>> The
>>>>>>>> property is not used in the code, and it is not entirely clear
>>>>>>>> what
>>>>>>>> it
>>>>>>>> would be used for. If there is a need for it, we can add it back
>>>>>>>> in
>>>>>>>> later on.
>>>>>>>> - Don't export OF specific API calls
>>>>>>>> - For OF support, no longer depend on iio_map
>>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
>>>>>>>> code
>>>>>>>> still builds if it is not selected.
>>>>>>>> - Change iio_channel_get to take device pointer as argument
>>>>>>>> instead
>>>>>>>> of
>>>>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>>>>> - iio_channel_get now works for both OF and non-OF
>>>>>>>> configurations.
>>>>>>>>
>>>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
>>>>>>>>  ++++++++
>>>>>>>>  drivers/iio/inkern.c                               |  186
>>>>>>>>
>>>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>>>>
>>>>>>>>  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..58df5f6
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>>>> @@ -0,0 +1,76 @@
>>>>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>>>>> bindings,
>>>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>>>>> +
>>>>>>>> +Sources of IIO channels can be represented by any node in the
>>>>>>>> device
>>>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
>>>>>>>> consumer
>>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
>>>>>>>> provider
>>>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>>>>> +output on a device.  The length of an IIO specifier is defined
>>>>>>>> by
>>>>>>>> the
>>>>>>>> +value of a #io-channel-cells property in the 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.
>>>>>>>> +
>>>>>>>> +For example:
>>>>>>>> +
>>>>>>>> +    adc: adc@35 {
>>>>>>>> +	compatible = "maxim,max1139";
>>>>>>>> +	reg = <0x35>;
>>>>>>>> +        #io-channel-cells = <1>;
>>>>>>>> +    };
>>>>>>>> +
>>>>>>>> +==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 b289915..d48f2a8 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"
>>>>>>>>
>>>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>>>>
>>>>>>>>  	return chan;
>>>>>>>>  
>>>>>>>>  }
>>>>>>>>
>>>>>>>> +#ifdef CONFIG_OF
>>>>>>>> +
>>>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>>>>> +{
>>>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node
>>>>
>>>> ==
>>>>
>>>>>>> data;
>>>>>>>
>>>>>>> Hmm, do you need to check type name here? One device node should
>>>>>>> rather
>>>>>>> represent only one device, making node an unique identifier.
>>>>>>>
>>>>>>> It this is meant to be a sanity check, it could be done one time
>>>>>>> after
>>>>>>> finding the device.
>>>>>>
>>>>>> Hi Tomasz,
>>>>>>
>>>>>> This is what Lars had suggested earlier:
>>>>>>> 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.
>>>>>>
>>>>>> Is it really needed, or in other words would it be sufficient to
>>>>>> check
>>>>>> if of_node and data match each other ? Your reasoning makes sense
>>>>>> to
>>>>>> me, and I had thought about it as well, but I don't really know,
>>>>>> and
>>>>>> I don't know how I could test it and guarantee correctness either.
>>>>>> I'll be happy to take the strcmp() out if someone tells me that it
>>>>>> is
>>>>>> definitely not needed ...
>>>>>
>>>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
>>>>> they
>>>>> both belong to the same physical device. But you don't need to do
>>>>> the
>>>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
>>>>> &iio_dev_type. Although it doesn't really matter in practice first
>>>>> check for the of_node then check for the type, since the of_node
>>>>> will
>>>>> only match for a few devices at most, the type will match for quite
>>>>> a
>>>>> few.
>>>>
>>>> I must disagree.
>>>>
>>>> If you have two IIO devices provided by one physical device, then in
>>>>
>>>> device tree they should be represented as follows:
>>>> 	phys-dev@12345678 {
>>>> 	
>>>> 		compatible = "some-physical-device";
>>>> 		/* ... */
>>>> 		
>>>> 		my_trig: iio-trigger {
>>>> 		
>>>> 			/* ... */
>>>> 		
>>>> 		};
>>>> 		
>>>> 		my_dev: iio-device {
>>>> 		
>>>> 			/* ... */
>>>> 		
>>>> 		};
>>>> 	
>>>> 	};
>>>>
>>>> Notice that phys-dev works here as an IIO bus on which its IIO devices
>>>> are available. This is related to the convention that single OF
>>>> device node represents single device, which would be violated
>>>> otherwise.
>>>
>>> Right now the iio device is a child of the physical device, and I am
>>> simply passing of_node on to it. guess you are saying that is not
>>> correct ?
>>>
>>> If so, what would be the correct approach ? Something like the following
>>> ?
>>>
>>> 	voltage-sensor@35 {
>>> 		compatible = "maxim,max1139";
>>> 		reg = <0x35>;
>>> 		vcc-supply = <&reg_3p3v>;
>>> 		vref-supply = <&reg_3p3v>;
>>>
>>> 		max1139-iio: iio-device {
>>> 			device_type = "iio_device";
>>> 			#io-channel-cells = <1>;
>>> 		};
>>> 	};
>>>
>>> and in the driver probe function:
>>>
>>> 	if (parent->of_node)
>>> 		iio_dev->dev.of_node = of_find_node_by_type(parent->of_node,
>>> "iio_device");
>>>
>>> Another option would be to use of_find_compatible_node() and something
>>> like compatible = "iio-device";
>>> in the iio-device node.
>>
>> A device node is defined as a node having compatible property. Other nodes 
>> should be seen as helper nodes, which do not represent devices (although 
>> they all have struct device_node in Linux).
>>
>> Also, AFAIK, device_type is a deprecated property used by some legacy 
>> PowerPC machines and for current machines only compatible should be used.
>>
>> So I guess the approach with compatible would be appropriate here.
>>
>> However for physical devices providing only a single IIO device it might 
>> be better to allow simpler specification, like:
>>
>>  	max1139-iio: voltage-sensor@35 {
>>  		compatible = "maxim,max1139", "iio_device";
> 
> I don't think this makes a lot of sense. First of all iio_device an artificial
> Linux term, while the device tree should describe the hardware. Secondly there
> is no generic iio driver which could match on a node with a "iio_device"
> compability string and stuff would just work. I mean we don't do
> 
> compatible = "atmel,at91sam9260-i2c", "i2c-master";
> 
> or similar either.
Lars, what you say here doesn't cover one 'interesting' case.

An iio_device has one aspect in which it is not artificial. It represents
a group of channels that may be sampled at 'one instance' on a given device.

We do have devices out there (no drivers as yet) where it will make sense
to have multiple iio_device instances for one 'chip'.  This is typically
where we have multiple sampling rates for the different devices within
a package.  The mxs-lradc has 8 different 'slots' and any subset of channels
can be assigned to any device.  So it may at some point be controlled
via a driver registering 8 iio_device instances.

Now this might justify child nodes for the iio_device.  I'm really not
sure, but thought I'd throw it in there.  Note it is probably only
really relevant when we have clients using the interrupt driven interface
rather than iio_hwmon and similar which use a polled read.  Right now
the polled and interrupt driven consumer interfaces are mutually exclusive

Now arguably we only want to pull this trick of multiple iio devices to
simplify the core handling of these 'interesting' bits of hardware, but
it does have a real meaning in hardware none the less.

Just thought I'd confuse things even further ;)

Jonathan
> 
> 
>>  		reg = <0x35>;
>>  		vcc-supply = <&reg_3p3v>;
>>  		vref-supply = <&reg_3p3v>;
>> 		device_type = "iio_device";
>>  		#io-channel-cells = <1>;
>>  	};
>>
> --
> 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: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 09:58 PM, Jonathan Cameron wrote:
> On 02/03/2013 06:55 PM, Lars-Peter Clausen wrote:
>> On 02/03/2013 06:30 PM, Tomasz Figa wrote:
>>> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
>>>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
>>>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>>>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>>>>>> Hi Guenter,
>>>>>>>>
>>>>>>>> Some comments inline.
>>>>>>>>
>>>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>>>>>> Provide bindings and parse OF data during initialization.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>> - Documentation update per feedback
>>>>>>>>> - Dropped io-channel-output-names from the bindings document.
>>>>>>>>> The
>>>>>>>>> property is not used in the code, and it is not entirely clear
>>>>>>>>> what
>>>>>>>>> it
>>>>>>>>> would be used for. If there is a need for it, we can add it back
>>>>>>>>> in
>>>>>>>>> later on.
>>>>>>>>> - Don't export OF specific API calls
>>>>>>>>> - For OF support, no longer depend on iio_map
>>>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
>>>>>>>>> code
>>>>>>>>> still builds if it is not selected.
>>>>>>>>> - Change iio_channel_get to take device pointer as argument
>>>>>>>>> instead
>>>>>>>>> of
>>>>>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>>>>>> - iio_channel_get now works for both OF and non-OF
>>>>>>>>> configurations.
>>>>>>>>>
>>>>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
>>>>>>>>>  ++++++++
>>>>>>>>>  drivers/iio/inkern.c                               |  186
>>>>>>>>>
>>>>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>>>>>
>>>>>>>>>  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..58df5f6
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>>>>> @@ -0,0 +1,76 @@
>>>>>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>>>>>> bindings,
>>>>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>>>>>> +
>>>>>>>>> +Sources of IIO channels can be represented by any node in the
>>>>>>>>> device
>>>>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
>>>>>>>>> consumer
>>>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
>>>>>>>>> provider
>>>>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>>>>>> +output on a device.  The length of an IIO specifier is defined
>>>>>>>>> by
>>>>>>>>> the
>>>>>>>>> +value of a #io-channel-cells property in the 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.
>>>>>>>>> +
>>>>>>>>> +For example:
>>>>>>>>> +
>>>>>>>>> +    adc: adc@35 {
>>>>>>>>> +	compatible = "maxim,max1139";
>>>>>>>>> +	reg = <0x35>;
>>>>>>>>> +        #io-channel-cells = <1>;
>>>>>>>>> +    };
>>>>>>>>> +
>>>>>>>>> +==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 b289915..d48f2a8 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"
>>>>>>>>>
>>>>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>>>>>
>>>>>>>>>  	return chan;
>>>>>>>>>  
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +#ifdef CONFIG_OF
>>>>>>>>> +
>>>>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>>>>>> +{
>>>>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node
>>>>>
>>>>> ==
>>>>>
>>>>>>>> data;
>>>>>>>>
>>>>>>>> Hmm, do you need to check type name here? One device node should
>>>>>>>> rather
>>>>>>>> represent only one device, making node an unique identifier.
>>>>>>>>
>>>>>>>> It this is meant to be a sanity check, it could be done one time
>>>>>>>> after
>>>>>>>> finding the device.
>>>>>>>
>>>>>>> Hi Tomasz,
>>>>>>>
>>>>>>> This is what Lars had suggested earlier:
>>>>>>>> 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.
>>>>>>>
>>>>>>> Is it really needed, or in other words would it be sufficient to
>>>>>>> check
>>>>>>> if of_node and data match each other ? Your reasoning makes sense
>>>>>>> to
>>>>>>> me, and I had thought about it as well, but I don't really know,
>>>>>>> and
>>>>>>> I don't know how I could test it and guarantee correctness either.
>>>>>>> I'll be happy to take the strcmp() out if someone tells me that it
>>>>>>> is
>>>>>>> definitely not needed ...
>>>>>>
>>>>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
>>>>>> they
>>>>>> both belong to the same physical device. But you don't need to do
>>>>>> the
>>>>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
>>>>>> &iio_dev_type. Although it doesn't really matter in practice first
>>>>>> check for the of_node then check for the type, since the of_node
>>>>>> will
>>>>>> only match for a few devices at most, the type will match for quite
>>>>>> a
>>>>>> few.
>>>>>
>>>>> I must disagree.
>>>>>
>>>>> If you have two IIO devices provided by one physical device, then in
>>>>>
>>>>> device tree they should be represented as follows:
>>>>> 	phys-dev@12345678 {
>>>>> 	
>>>>> 		compatible = "some-physical-device";
>>>>> 		/* ... */
>>>>> 		
>>>>> 		my_trig: iio-trigger {
>>>>> 		
>>>>> 			/* ... */
>>>>> 		
>>>>> 		};
>>>>> 		
>>>>> 		my_dev: iio-device {
>>>>> 		
>>>>> 			/* ... */
>>>>> 		
>>>>> 		};
>>>>> 	
>>>>> 	};
>>>>>
>>>>> Notice that phys-dev works here as an IIO bus on which its IIO devices
>>>>> are available. This is related to the convention that single OF
>>>>> device node represents single device, which would be violated
>>>>> otherwise.
>>>>
>>>> Right now the iio device is a child of the physical device, and I am
>>>> simply passing of_node on to it. guess you are saying that is not
>>>> correct ?
>>>>
>>>> If so, what would be the correct approach ? Something like the following
>>>> ?
>>>>
>>>> 	voltage-sensor@35 {
>>>> 		compatible = "maxim,max1139";
>>>> 		reg = <0x35>;
>>>> 		vcc-supply = <&reg_3p3v>;
>>>> 		vref-supply = <&reg_3p3v>;
>>>>
>>>> 		max1139-iio: iio-device {
>>>> 			device_type = "iio_device";
>>>> 			#io-channel-cells = <1>;
>>>> 		};
>>>> 	};
>>>>
>>>> and in the driver probe function:
>>>>
>>>> 	if (parent->of_node)
>>>> 		iio_dev->dev.of_node = of_find_node_by_type(parent->of_node,
>>>> "iio_device");
>>>>
>>>> Another option would be to use of_find_compatible_node() and something
>>>> like compatible = "iio-device";
>>>> in the iio-device node.
>>>
>>> A device node is defined as a node having compatible property. Other nodes 
>>> should be seen as helper nodes, which do not represent devices (although 
>>> they all have struct device_node in Linux).
>>>
>>> Also, AFAIK, device_type is a deprecated property used by some legacy 
>>> PowerPC machines and for current machines only compatible should be used.
>>>
>>> So I guess the approach with compatible would be appropriate here.
>>>
>>> However for physical devices providing only a single IIO device it might 
>>> be better to allow simpler specification, like:
>>>
>>>  	max1139-iio: voltage-sensor@35 {
>>>  		compatible = "maxim,max1139", "iio_device";
>>
>> I don't think this makes a lot of sense. First of all iio_device an artificial
>> Linux term, while the device tree should describe the hardware. Secondly there
>> is no generic iio driver which could match on a node with a "iio_device"
>> compability string and stuff would just work. I mean we don't do
>>
>> compatible = "atmel,at91sam9260-i2c", "i2c-master";
>>
>> or similar either.
> Lars, what you say here doesn't cover one 'interesting' case.
> 
> An iio_device has one aspect in which it is not artificial. It represents
> a group of channels that may be sampled at 'one instance' on a given device.

I primarily meant the term IIO.

- Lars

Re: [PATCH v2 4/4] iio: Add OF support

[Tomasz Figa]

On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
> On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> >>>>>> Hi Guenter,
> >>>>>> 
> >>>>>> Some comments inline.
> >>>>>> 
> >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> >>>>>>> Provide bindings and parse OF data during initialization.
> >>>>>>> 
> >>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >>>>>>> ---
> >>>>>>> - Documentation update per feedback
> >>>>>>> - Dropped io-channel-output-names from the bindings document.
> >>>>>>> The
> >>>>>>> property is not used in the code, and it is not entirely clear
> >>>>>>> what
> >>>>>>> it
> >>>>>>> would be used for. If there is a need for it, we can add it back
> >>>>>>> in
> >>>>>>> later on.
> >>>>>>> - Don't export OF specific API calls
> >>>>>>> - For OF support, no longer depend on iio_map
> >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
> >>>>>>> code
> >>>>>>> still builds if it is not selected.
> >>>>>>> - Change iio_channel_get to take device pointer as argument
> >>>>>>> instead
> >>>>>>> of
> >>>>>>> device name. Retain old API as of_iio_channel_get_sys.
> >>>>>>> - iio_channel_get now works for both OF and non-OF
> >>>>>>> configurations.
> >>>>>>> 
> >>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> >>>>>>>  ++++++++
> >>>>>>>  drivers/iio/inkern.c                               |  186
> >>>>>>> 
> >>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> >>>>>>> 
> >>>>>>>  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..58df5f6
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> >>>>>>> @@ -0,0 +1,76 @@
> >>>>>>> +This binding is a work-in-progress. It is derived from clock
> >>>>>>> bindings,
> >>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
> >>>>>>> +
> >>>>>>> +Sources of IIO channels can be represented by any node in the
> >>>>>>> device
> >>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
> >>>>>>> consumer
> >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
> >>>>>>> provider
> >>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> >>>>>>> +specifier is an array of one or more cells identifying the IIO
> >>>>>>> +output on a device.  The length of an IIO specifier is defined
> >>>>>>> by
> >>>>>>> the
> >>>>>>> +value of a #io-channel-cells property in the 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.
> >>>>>>> +
> >>>>>>> +For example:
> >>>>>>> +
> >>>>>>> +    adc: adc@35 {
> >>>>>>> +	compatible = "maxim,max1139";
> >>>>>>> +	reg = <0x35>;
> >>>>>>> +        #io-channel-cells = <1>;
> >>>>>>> +    };
> >>>>>>> +
> >>>>>>> +==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 b289915..d48f2a8 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"
> >>>>>>> 
> >>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> >>>>>>> 
> >>>>>>>  	return chan;
> >>>>>>>  
> >>>>>>>  }
> >>>>>>> 
> >>>>>>> +#ifdef CONFIG_OF
> >>>>>>> +
> >>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
> >>>>>>> +{
> >>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
>of_node
> >>> 
> >>> ==
> >>> 
> >>>>>> data;
> >>>>>> 
> >>>>>> Hmm, do you need to check type name here? One device node should
> >>>>>> rather
> >>>>>> represent only one device, making node an unique identifier.
> >>>>>> 
> >>>>>> It this is meant to be a sanity check, it could be done one time
> >>>>>> after
> >>>>>> finding the device.
> >>>>> 
> >>>>> Hi Tomasz,
> >>>>> 
> >>>>> This is what Lars had suggested earlier:
> >>>>>> 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.
> >>>>> 
> >>>>> Is it really needed, or in other words would it be sufficient to
> >>>>> check
> >>>>> if of_node and data match each other ? Your reasoning makes sense
> >>>>> to
> >>>>> me, and I had thought about it as well, but I don't really know,
> >>>>> and
> >>>>> I don't know how I could test it and guarantee correctness either.
> >>>>> I'll be happy to take the strcmp() out if someone tells me that it
> >>>>> is
> >>>>> definitely not needed ...
> >>>> 
> >>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
> >>>> they
> >>>> both belong to the same physical device. But you don't need to do
> >>>> the
> >>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> >>>> &iio_dev_type. Although it doesn't really matter in practice first
> >>>> check for the of_node then check for the type, since the of_node
> >>>> will
> >>>> only match for a few devices at most, the type will match for quite
> >>>> a
> >>>> few.
> >>> 
> >>> I must disagree.
> >>> 
> >>> If you have two IIO devices provided by one physical device, then in
> >>> 
> >>> device tree they should be represented as follows:
> >>> 	phys-dev@12345678 {
> >>> 	
> >>> 		compatible = "some-physical-device";
> >>> 		/* ... */
> >>> 		
> >>> 		my_trig: iio-trigger {
> >>> 		
> >>> 			/* ... */
> >>> 		
> >>> 		};
> >>> 		
> >>> 		my_dev: iio-device {
> >>> 		
> >>> 			/* ... */
> >>> 		
> >>> 		};
> >>> 	
> >>> 	};
> >>> 
> >>> Notice that phys-dev works here as an IIO bus on which its IIO
> >>> devices
> >>> are available. This is related to the convention that single OF
> >>> device node represents single device, which would be violated
> >>> otherwise.
> >> 
> >> Right now the iio device is a child of the physical device, and I am
> >> simply passing of_node on to it. guess you are saying that is not
> >> correct ?
> >> 
> >> If so, what would be the correct approach ? Something like the
> >> following ?
> >> 
> >> 	voltage-sensor@35 {
> >> 	
> >> 		compatible = "maxim,max1139";
> >> 		reg = <0x35>;
> >> 		vcc-supply = <&reg_3p3v>;
> >> 		vref-supply = <&reg_3p3v>;
> >> 		
> >> 		max1139-iio: iio-device {
> >> 		
> >> 			device_type = "iio_device";
> >> 			#io-channel-cells = <1>;
> >> 		
> >> 		};
> >> 	
> >> 	};
> >> 
> >> and in the driver probe function:
> >> 	if (parent->of_node)
> >> 	
> >> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
>of_node,
> >> 
> >> "iio_device");
> >> 
> >> Another option would be to use of_find_compatible_node() and
> >> something
> >> like compatible = "iio-device";
> >> in the iio-device node.
> > 
> > A device node is defined as a node having compatible property. Other
> > nodes should be seen as helper nodes, which do not represent devices
> > (although they all have struct device_node in Linux).
> > 
> > Also, AFAIK, device_type is a deprecated property used by some legacy
> > PowerPC machines and for current machines only compatible should be
> > used.
> > 
> > So I guess the approach with compatible would be appropriate here.
> > 
> > However for physical devices providing only a single IIO device it
> > might> 
> > be better to allow simpler specification, like:
> >  	max1139-iio: voltage-sensor@35 {
> >  	
> >  		compatible = "maxim,max1139", "iio_device";
> 
> I don't think this makes a lot of sense. First of all iio_device an
> artificial Linux term, while the device tree should describe the
> hardware.

Well, if you look at an iio_device as a subdevice of a physical device 
then it should make a bit more sense. (See nodes of GPIO/pinctrl pin banks 
or regulators of a PMIC chip.)

> Secondly there is no generic iio driver which could match on
> a node with a "iio_device" compability string and stuff would just
> work. I mean we don't do
> 
> compatible = "atmel,at91sam9260-i2c", "i2c-master";
> 
> or similar either.

Right. We don't need the other compatible for simple devices with single 
subdevice. This is implied by the driver registering a single IIO driver 
using the node of physical device.

> >  		reg = <0x35>;
> >  		vcc-supply = <&reg_3p3v>;
> >  		vref-supply = <&reg_3p3v>;
> > 		
> > 		device_type = "iio_device";

Also we don't need this device_type. Basically we don't need to specify 
whether given node is an iio_device or an iio_trigger. It's up to the 
driver to register the node as a device or a trigger by setting dev.of_node 
field properly.

So my suggestion would be to make the bindings as following. For single 
subdevice:

	voltage-sensor@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		vcc-supply = <&reg_3p3v>;
		vref-supply = <&reg_3p3v>;
		#io-channel-cells = <1>;
	};

For multiple subdevices:

	voltage-sensor@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		vcc-supply = <&reg_3p3v>;
		vref-supply = <&reg_3p3v>;

		subdevice1 {
			/* Subdevice specific data */
			#io-channel-cells = <1>;
		};

		subdevice2 {
			/* Subdevice specific data */
			#io-channel-cells = <1>;
		}
	};

Best regards,
Tomasz

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote:
> On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
> > On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> > > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> > >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> > >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> > >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > >>>>>> Hi Guenter,
> > >>>>>> 
> > >>>>>> Some comments inline.
> > >>>>>> 
> > >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> > >>>>>>> Provide bindings and parse OF data during initialization.
> > >>>>>>> 
> > >>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > >>>>>>> ---
> > >>>>>>> - Documentation update per feedback
> > >>>>>>> - Dropped io-channel-output-names from the bindings document.
> > >>>>>>> The
> > >>>>>>> property is not used in the code, and it is not entirely clear
> > >>>>>>> what
> > >>>>>>> it
> > >>>>>>> would be used for. If there is a need for it, we can add it back
> > >>>>>>> in
> > >>>>>>> later on.
> > >>>>>>> - Don't export OF specific API calls
> > >>>>>>> - For OF support, no longer depend on iio_map
> > >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
> > >>>>>>> code
> > >>>>>>> still builds if it is not selected.
> > >>>>>>> - Change iio_channel_get to take device pointer as argument
> > >>>>>>> instead
> > >>>>>>> of
> > >>>>>>> device name. Retain old API as of_iio_channel_get_sys.
> > >>>>>>> - iio_channel_get now works for both OF and non-OF
> > >>>>>>> configurations.
> > >>>>>>> 
> > >>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> > >>>>>>>  ++++++++
> > >>>>>>>  drivers/iio/inkern.c                               |  186
> > >>>>>>> 
> > >>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > >>>>>>> 
> > >>>>>>>  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..58df5f6
> > >>>>>>> --- /dev/null
> > >>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > >>>>>>> @@ -0,0 +1,76 @@
> > >>>>>>> +This binding is a work-in-progress. It is derived from clock
> > >>>>>>> bindings,
> > >>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
> > >>>>>>> +
> > >>>>>>> +Sources of IIO channels can be represented by any node in the
> > >>>>>>> device
> > >>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
> > >>>>>>> consumer
> > >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
> > >>>>>>> provider
> > >>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > >>>>>>> +specifier is an array of one or more cells identifying the IIO
> > >>>>>>> +output on a device.  The length of an IIO specifier is defined
> > >>>>>>> by
> > >>>>>>> the
> > >>>>>>> +value of a #io-channel-cells property in the 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.
> > >>>>>>> +
> > >>>>>>> +For example:
> > >>>>>>> +
> > >>>>>>> +    adc: adc@35 {
> > >>>>>>> +	compatible = "maxim,max1139";
> > >>>>>>> +	reg = <0x35>;
> > >>>>>>> +        #io-channel-cells = <1>;
> > >>>>>>> +    };
> > >>>>>>> +
> > >>>>>>> +==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 b289915..d48f2a8 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"
> > >>>>>>> 
> > >>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > >>>>>>> 
> > >>>>>>>  	return chan;
> > >>>>>>>  
> > >>>>>>>  }
> > >>>>>>> 
> > >>>>>>> +#ifdef CONFIG_OF
> > >>>>>>> +
> > >>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
> > >>>>>>> +{
> > >>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
> >of_node
> > >>> 
> > >>> ==
> > >>> 
> > >>>>>> data;
> > >>>>>> 
> > >>>>>> Hmm, do you need to check type name here? One device node should
> > >>>>>> rather
> > >>>>>> represent only one device, making node an unique identifier.
> > >>>>>> 
> > >>>>>> It this is meant to be a sanity check, it could be done one time
> > >>>>>> after
> > >>>>>> finding the device.
> > >>>>> 
> > >>>>> Hi Tomasz,
> > >>>>> 
> > >>>>> This is what Lars had suggested earlier:
> > >>>>>> 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.
> > >>>>> 
> > >>>>> Is it really needed, or in other words would it be sufficient to
> > >>>>> check
> > >>>>> if of_node and data match each other ? Your reasoning makes sense
> > >>>>> to
> > >>>>> me, and I had thought about it as well, but I don't really know,
> > >>>>> and
> > >>>>> I don't know how I could test it and guarantee correctness either.
> > >>>>> I'll be happy to take the strcmp() out if someone tells me that it
> > >>>>> is
> > >>>>> definitely not needed ...
> > >>>> 
> > >>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
> > >>>> they
> > >>>> both belong to the same physical device. But you don't need to do
> > >>>> the
> > >>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> > >>>> &iio_dev_type. Although it doesn't really matter in practice first
> > >>>> check for the of_node then check for the type, since the of_node
> > >>>> will
> > >>>> only match for a few devices at most, the type will match for quite
> > >>>> a
> > >>>> few.
> > >>> 
> > >>> I must disagree.
> > >>> 
> > >>> If you have two IIO devices provided by one physical device, then in
> > >>> 
> > >>> device tree they should be represented as follows:
> > >>> 	phys-dev@12345678 {
> > >>> 	
> > >>> 		compatible = "some-physical-device";
> > >>> 		/* ... */
> > >>> 		
> > >>> 		my_trig: iio-trigger {
> > >>> 		
> > >>> 			/* ... */
> > >>> 		
> > >>> 		};
> > >>> 		
> > >>> 		my_dev: iio-device {
> > >>> 		
> > >>> 			/* ... */
> > >>> 		
> > >>> 		};
> > >>> 	
> > >>> 	};
> > >>> 
> > >>> Notice that phys-dev works here as an IIO bus on which its IIO
> > >>> devices
> > >>> are available. This is related to the convention that single OF
> > >>> device node represents single device, which would be violated
> > >>> otherwise.
> > >> 
> > >> Right now the iio device is a child of the physical device, and I am
> > >> simply passing of_node on to it. guess you are saying that is not
> > >> correct ?
> > >> 
> > >> If so, what would be the correct approach ? Something like the
> > >> following ?
> > >> 
> > >> 	voltage-sensor@35 {
> > >> 	
> > >> 		compatible = "maxim,max1139";
> > >> 		reg = <0x35>;
> > >> 		vcc-supply = <&reg_3p3v>;
> > >> 		vref-supply = <&reg_3p3v>;
> > >> 		
> > >> 		max1139-iio: iio-device {
> > >> 		
> > >> 			device_type = "iio_device";
> > >> 			#io-channel-cells = <1>;
> > >> 		
> > >> 		};
> > >> 	
> > >> 	};
> > >> 
> > >> and in the driver probe function:
> > >> 	if (parent->of_node)
> > >> 	
> > >> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
> >of_node,
> > >> 
> > >> "iio_device");
> > >> 
> > >> Another option would be to use of_find_compatible_node() and
> > >> something
> > >> like compatible = "iio-device";
> > >> in the iio-device node.
> > > 
> > > A device node is defined as a node having compatible property. Other
> > > nodes should be seen as helper nodes, which do not represent devices
> > > (although they all have struct device_node in Linux).
> > > 
> > > Also, AFAIK, device_type is a deprecated property used by some legacy
> > > PowerPC machines and for current machines only compatible should be
> > > used.
> > > 
> > > So I guess the approach with compatible would be appropriate here.
> > > 
> > > However for physical devices providing only a single IIO device it
> > > might> 
> > > be better to allow simpler specification, like:
> > >  	max1139-iio: voltage-sensor@35 {
> > >  	
> > >  		compatible = "maxim,max1139", "iio_device";
> > 
> > I don't think this makes a lot of sense. First of all iio_device an
> > artificial Linux term, while the device tree should describe the
> > hardware.
> 
> Well, if you look at an iio_device as a subdevice of a physical device 
> then it should make a bit more sense. (See nodes of GPIO/pinctrl pin banks 
> or regulators of a PMIC chip.)
> 
> > Secondly there is no generic iio driver which could match on
> > a node with a "iio_device" compability string and stuff would just
> > work. I mean we don't do
> > 
> > compatible = "atmel,at91sam9260-i2c", "i2c-master";
> > 
> > or similar either.
> 
> Right. We don't need the other compatible for simple devices with single 
> subdevice. This is implied by the driver registering a single IIO driver 
> using the node of physical device.
> 
> > >  		reg = <0x35>;
> > >  		vcc-supply = <&reg_3p3v>;
> > >  		vref-supply = <&reg_3p3v>;
> > > 		
> > > 		device_type = "iio_device";
> 
> Also we don't need this device_type. Basically we don't need to specify 
> whether given node is an iio_device or an iio_trigger. It's up to the 
> driver to register the node as a device or a trigger by setting dev.of_node 
> field properly.
> 
> So my suggestion would be to make the bindings as following. For single 
> subdevice:
> 
> 	voltage-sensor@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		vcc-supply = <&reg_3p3v>;
> 		vref-supply = <&reg_3p3v>;
> 		#io-channel-cells = <1>;
> 	};
> 
> For multiple subdevices:
> 
> 	voltage-sensor@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		vcc-supply = <&reg_3p3v>;
> 		vref-supply = <&reg_3p3v>;
> 
> 		subdevice1 {
> 			/* Subdevice specific data */
> 			#io-channel-cells = <1>;
> 		};
> 
> 		subdevice2 {
> 			/* Subdevice specific data */
> 			#io-channel-cells = <1>;
> 		}

Please provide an example how to parse that. Obviously now I can not look for
"compatible" anymore. Sure, I can use of_get_child_by_name, but that means
the sub-device names would have to be well defined. Or I could use
of_find_node_by_name, but then I would need something like 

 	voltage-sensor@35 {
 		compatible = "maxim,max1139";
		iio-device;
 		reg = <0x35>;
 		vcc-supply = <&reg_3p3v>;
 		vref-supply = <&reg_3p3v>;
 		#io-channel-cells = <1>;
 	};

Thanks,
Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/04/2013 06:12 PM, Guenter Roeck wrote:
> On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote:
>> On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
>>> On 02/03/2013 06:30 PM, Tomasz Figa wrote:
>>>> On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
>>>>> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
>>>>>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
>>>>>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>>>>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>>>>>>> Hi Guenter,
>>>>>>>>>
>>>>>>>>> Some comments inline.
>>>>>>>>>
>>>>>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>>>>>>> Provide bindings and parse OF data during initialization.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>>>>>>> ---
>>>>>>>>>> - Documentation update per feedback
>>>>>>>>>> - Dropped io-channel-output-names from the bindings document.
>>>>>>>>>> The
>>>>>>>>>> property is not used in the code, and it is not entirely clear
>>>>>>>>>> what
>>>>>>>>>> it
>>>>>>>>>> would be used for. If there is a need for it, we can add it back
>>>>>>>>>> in
>>>>>>>>>> later on.
>>>>>>>>>> - Don't export OF specific API calls
>>>>>>>>>> - For OF support, no longer depend on iio_map
>>>>>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
>>>>>>>>>> code
>>>>>>>>>> still builds if it is not selected.
>>>>>>>>>> - Change iio_channel_get to take device pointer as argument
>>>>>>>>>> instead
>>>>>>>>>> of
>>>>>>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>>>>>>> - iio_channel_get now works for both OF and non-OF
>>>>>>>>>> configurations.
>>>>>>>>>>
>>>>>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
>>>>>>>>>>  ++++++++
>>>>>>>>>>  drivers/iio/inkern.c                               |  186
>>>>>>>>>>
>>>>>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>>>>>>
>>>>>>>>>>  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..58df5f6
>>>>>>>>>> --- /dev/null
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>>>>>>> @@ -0,0 +1,76 @@
>>>>>>>>>> +This binding is a work-in-progress. It is derived from clock
>>>>>>>>>> bindings,
>>>>>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>>>>>>> +
>>>>>>>>>> +Sources of IIO channels can be represented by any node in the
>>>>>>>>>> device
>>>>>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
>>>>>>>>>> consumer
>>>>>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
>>>>>>>>>> provider
>>>>>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>>>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>>>>>>> +output on a device.  The length of an IIO specifier is defined
>>>>>>>>>> by
>>>>>>>>>> the
>>>>>>>>>> +value of a #io-channel-cells property in the 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.
>>>>>>>>>> +
>>>>>>>>>> +For example:
>>>>>>>>>> +
>>>>>>>>>> +    adc: adc@35 {
>>>>>>>>>> +	compatible = "maxim,max1139";
>>>>>>>>>> +	reg = <0x35>;
>>>>>>>>>> +        #io-channel-cells = <1>;
>>>>>>>>>> +    };
>>>>>>>>>> +
>>>>>>>>>> +==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 b289915..d48f2a8 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"
>>>>>>>>>>
>>>>>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>>>>>>
>>>>>>>>>>  	return chan;
>>>>>>>>>>  
>>>>>>>>>>  }
>>>>>>>>>>
>>>>>>>>>> +#ifdef CONFIG_OF
>>>>>>>>>> +
>>>>>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>>>>>>> +{
>>>>>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
>>> of_node
>>>>>>
>>>>>> ==
>>>>>>
>>>>>>>>> data;
>>>>>>>>>
>>>>>>>>> Hmm, do you need to check type name here? One device node should
>>>>>>>>> rather
>>>>>>>>> represent only one device, making node an unique identifier.
>>>>>>>>>
>>>>>>>>> It this is meant to be a sanity check, it could be done one time
>>>>>>>>> after
>>>>>>>>> finding the device.
>>>>>>>>
>>>>>>>> Hi Tomasz,
>>>>>>>>
>>>>>>>> This is what Lars had suggested earlier:
>>>>>>>>> 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.
>>>>>>>>
>>>>>>>> Is it really needed, or in other words would it be sufficient to
>>>>>>>> check
>>>>>>>> if of_node and data match each other ? Your reasoning makes sense
>>>>>>>> to
>>>>>>>> me, and I had thought about it as well, but I don't really know,
>>>>>>>> and
>>>>>>>> I don't know how I could test it and guarantee correctness either.
>>>>>>>> I'll be happy to take the strcmp() out if someone tells me that it
>>>>>>>> is
>>>>>>>> definitely not needed ...
>>>>>>>
>>>>>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
>>>>>>> they
>>>>>>> both belong to the same physical device. But you don't need to do
>>>>>>> the
>>>>>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
>>>>>>> &iio_dev_type. Although it doesn't really matter in practice first
>>>>>>> check for the of_node then check for the type, since the of_node
>>>>>>> will
>>>>>>> only match for a few devices at most, the type will match for quite
>>>>>>> a
>>>>>>> few.
>>>>>>
>>>>>> I must disagree.
>>>>>>
>>>>>> If you have two IIO devices provided by one physical device, then in
>>>>>>
>>>>>> device tree they should be represented as follows:
>>>>>> 	phys-dev@12345678 {
>>>>>> 	
>>>>>> 		compatible = "some-physical-device";
>>>>>> 		/* ... */
>>>>>> 		
>>>>>> 		my_trig: iio-trigger {
>>>>>> 		
>>>>>> 			/* ... */
>>>>>> 		
>>>>>> 		};
>>>>>> 		
>>>>>> 		my_dev: iio-device {
>>>>>> 		
>>>>>> 			/* ... */
>>>>>> 		
>>>>>> 		};
>>>>>> 	
>>>>>> 	};
>>>>>>
>>>>>> Notice that phys-dev works here as an IIO bus on which its IIO
>>>>>> devices
>>>>>> are available. This is related to the convention that single OF
>>>>>> device node represents single device, which would be violated
>>>>>> otherwise.
>>>>>
>>>>> Right now the iio device is a child of the physical device, and I am
>>>>> simply passing of_node on to it. guess you are saying that is not
>>>>> correct ?
>>>>>
>>>>> If so, what would be the correct approach ? Something like the
>>>>> following ?
>>>>>
>>>>> 	voltage-sensor@35 {
>>>>> 	
>>>>> 		compatible = "maxim,max1139";
>>>>> 		reg = <0x35>;
>>>>> 		vcc-supply = <&reg_3p3v>;
>>>>> 		vref-supply = <&reg_3p3v>;
>>>>> 		
>>>>> 		max1139-iio: iio-device {
>>>>> 		
>>>>> 			device_type = "iio_device";
>>>>> 			#io-channel-cells = <1>;
>>>>> 		
>>>>> 		};
>>>>> 	
>>>>> 	};
>>>>>
>>>>> and in the driver probe function:
>>>>> 	if (parent->of_node)
>>>>> 	
>>>>> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
>>> of_node,
>>>>>
>>>>> "iio_device");
>>>>>
>>>>> Another option would be to use of_find_compatible_node() and
>>>>> something
>>>>> like compatible = "iio-device";
>>>>> in the iio-device node.
>>>>
>>>> A device node is defined as a node having compatible property. Other
>>>> nodes should be seen as helper nodes, which do not represent devices
>>>> (although they all have struct device_node in Linux).
>>>>
>>>> Also, AFAIK, device_type is a deprecated property used by some legacy
>>>> PowerPC machines and for current machines only compatible should be
>>>> used.
>>>>
>>>> So I guess the approach with compatible would be appropriate here.
>>>>
>>>> However for physical devices providing only a single IIO device it
>>>> might> 
>>>> be better to allow simpler specification, like:
>>>>  	max1139-iio: voltage-sensor@35 {
>>>>  	
>>>>  		compatible = "maxim,max1139", "iio_device";
>>>
>>> I don't think this makes a lot of sense. First of all iio_device an
>>> artificial Linux term, while the device tree should describe the
>>> hardware.
>>
>> Well, if you look at an iio_device as a subdevice of a physical device 
>> then it should make a bit more sense. (See nodes of GPIO/pinctrl pin banks 
>> or regulators of a PMIC chip.)
>>
>>> Secondly there is no generic iio driver which could match on
>>> a node with a "iio_device" compability string and stuff would just
>>> work. I mean we don't do
>>>
>>> compatible = "atmel,at91sam9260-i2c", "i2c-master";
>>>
>>> or similar either.
>>
>> Right. We don't need the other compatible for simple devices with single 
>> subdevice. This is implied by the driver registering a single IIO driver 
>> using the node of physical device.
>>
>>>>  		reg = <0x35>;
>>>>  		vcc-supply = <&reg_3p3v>;
>>>>  		vref-supply = <&reg_3p3v>;
>>>> 		
>>>> 		device_type = "iio_device";
>>
>> Also we don't need this device_type. Basically we don't need to specify 
>> whether given node is an iio_device or an iio_trigger. It's up to the 
>> driver to register the node as a device or a trigger by setting dev.of_node 
>> field properly.
>>
>> So my suggestion would be to make the bindings as following. For single 
>> subdevice:
>>
>> 	voltage-sensor@35 {
>> 		compatible = "maxim,max1139";
>> 		reg = <0x35>;
>> 		vcc-supply = <&reg_3p3v>;
>> 		vref-supply = <&reg_3p3v>;
>> 		#io-channel-cells = <1>;
>> 	};
>>
>> For multiple subdevices:
>>
>> 	voltage-sensor@35 {
>> 		compatible = "maxim,max1139";
>> 		reg = <0x35>;
>> 		vcc-supply = <&reg_3p3v>;
>> 		vref-supply = <&reg_3p3v>;
>>
>> 		subdevice1 {
>> 			/* Subdevice specific data */
>> 			#io-channel-cells = <1>;
>> 		};
>>
>> 		subdevice2 {
>> 			/* Subdevice specific data */
>> 			#io-channel-cells = <1>;
>> 		}
> 
> Please provide an example how to parse that. Obviously now I can not look for
> "compatible" anymore. Sure, I can use of_get_child_by_name, but that means
> the sub-device names would have to be well defined. Or I could use
> of_find_node_by_name, but then I would need something like

Do you actually need to parse this in the generic code? It would be up to
the device driver to parse this and hook up the of_node to the of_node of
the iio device. Your code should just work fine already with multiple
subdevices.

- Lars

> 
>  	voltage-sensor@35 {
>  		compatible = "maxim,max1139";
> 		iio-device;
>  		reg = <0x35>;
>  		vcc-supply = <&reg_3p3v>;
>  		vref-supply = <&reg_3p3v>;
>  		#io-channel-cells = <1>;
>  	};
> 
> Thanks,
> Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Mon, Feb 04, 2013 at 09:12:14AM -0800, Guenter Roeck wrote:
> On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote:
> > On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
> > > On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> > > > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> > > >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> > > >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen wrote:
> > > >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > > >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > > >>>>>> Hi Guenter,
> > > >>>>>> 
> > > >>>>>> Some comments inline.
> > > >>>>>> 
> > > >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> > > >>>>>>> Provide bindings and parse OF data during initialization.
> > > >>>>>>> 
> > > >>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > >>>>>>> ---
> > > >>>>>>> - Documentation update per feedback
> > > >>>>>>> - Dropped io-channel-output-names from the bindings document.
> > > >>>>>>> The
> > > >>>>>>> property is not used in the code, and it is not entirely clear
> > > >>>>>>> what
> > > >>>>>>> it
> > > >>>>>>> would be used for. If there is a need for it, we can add it back
> > > >>>>>>> in
> > > >>>>>>> later on.
> > > >>>>>>> - Don't export OF specific API calls
> > > >>>>>>> - For OF support, no longer depend on iio_map
> > > >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the
> > > >>>>>>> code
> > > >>>>>>> still builds if it is not selected.
> > > >>>>>>> - Change iio_channel_get to take device pointer as argument
> > > >>>>>>> instead
> > > >>>>>>> of
> > > >>>>>>> device name. Retain old API as of_iio_channel_get_sys.
> > > >>>>>>> - iio_channel_get now works for both OF and non-OF
> > > >>>>>>> configurations.
> > > >>>>>>> 
> > > >>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> > > >>>>>>>  ++++++++
> > > >>>>>>>  drivers/iio/inkern.c                               |  186
> > > >>>>>>> 
> > > >>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > > >>>>>>> 
> > > >>>>>>>  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..58df5f6
> > > >>>>>>> --- /dev/null
> > > >>>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > > >>>>>>> @@ -0,0 +1,76 @@
> > > >>>>>>> +This binding is a work-in-progress. It is derived from clock
> > > >>>>>>> bindings,
> > > >>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
> > > >>>>>>> +
> > > >>>>>>> +Sources of IIO channels can be represented by any node in the
> > > >>>>>>> device
> > > >>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
> > > >>>>>>> consumer
> > > >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
> > > >>>>>>> provider
> > > >>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > > >>>>>>> +specifier is an array of one or more cells identifying the IIO
> > > >>>>>>> +output on a device.  The length of an IIO specifier is defined
> > > >>>>>>> by
> > > >>>>>>> the
> > > >>>>>>> +value of a #io-channel-cells property in the 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.
> > > >>>>>>> +
> > > >>>>>>> +For example:
> > > >>>>>>> +
> > > >>>>>>> +    adc: adc@35 {
> > > >>>>>>> +	compatible = "maxim,max1139";
> > > >>>>>>> +	reg = <0x35>;
> > > >>>>>>> +        #io-channel-cells = <1>;
> > > >>>>>>> +    };
> > > >>>>>>> +
> > > >>>>>>> +==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 b289915..d48f2a8 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"
> > > >>>>>>> 
> > > >>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > > >>>>>>> 
> > > >>>>>>>  	return chan;
> > > >>>>>>>  
> > > >>>>>>>  }
> > > >>>>>>> 
> > > >>>>>>> +#ifdef CONFIG_OF
> > > >>>>>>> +
> > > >>>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
> > > >>>>>>> +{
> > > >>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
> > >of_node
> > > >>> 
> > > >>> ==
> > > >>> 
> > > >>>>>> data;
> > > >>>>>> 
> > > >>>>>> Hmm, do you need to check type name here? One device node should
> > > >>>>>> rather
> > > >>>>>> represent only one device, making node an unique identifier.
> > > >>>>>> 
> > > >>>>>> It this is meant to be a sanity check, it could be done one time
> > > >>>>>> after
> > > >>>>>> finding the device.
> > > >>>>> 
> > > >>>>> Hi Tomasz,
> > > >>>>> 
> > > >>>>> This is what Lars had suggested earlier:
> > > >>>>>> 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.
> > > >>>>> 
> > > >>>>> Is it really needed, or in other words would it be sufficient to
> > > >>>>> check
> > > >>>>> if of_node and data match each other ? Your reasoning makes sense
> > > >>>>> to
> > > >>>>> me, and I had thought about it as well, but I don't really know,
> > > >>>>> and
> > > >>>>> I don't know how I could test it and guarantee correctness either.
> > > >>>>> I'll be happy to take the strcmp() out if someone tells me that it
> > > >>>>> is
> > > >>>>> definitely not needed ...
> > > >>>> 
> > > >>>> A IIO trigger and a IIO device may have the same of_node, e.g. if
> > > >>>> they
> > > >>>> both belong to the same physical device. But you don't need to do
> > > >>>> the
> > > >>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type ==
> > > >>>> &iio_dev_type. Although it doesn't really matter in practice first
> > > >>>> check for the of_node then check for the type, since the of_node
> > > >>>> will
> > > >>>> only match for a few devices at most, the type will match for quite
> > > >>>> a
> > > >>>> few.
> > > >>> 
> > > >>> I must disagree.
> > > >>> 
> > > >>> If you have two IIO devices provided by one physical device, then in
> > > >>> 
> > > >>> device tree they should be represented as follows:
> > > >>> 	phys-dev@12345678 {
> > > >>> 	
> > > >>> 		compatible = "some-physical-device";
> > > >>> 		/* ... */
> > > >>> 		
> > > >>> 		my_trig: iio-trigger {
> > > >>> 		
> > > >>> 			/* ... */
> > > >>> 		
> > > >>> 		};
> > > >>> 		
> > > >>> 		my_dev: iio-device {
> > > >>> 		
> > > >>> 			/* ... */
> > > >>> 		
> > > >>> 		};
> > > >>> 	
> > > >>> 	};
> > > >>> 
> > > >>> Notice that phys-dev works here as an IIO bus on which its IIO
> > > >>> devices
> > > >>> are available. This is related to the convention that single OF
> > > >>> device node represents single device, which would be violated
> > > >>> otherwise.
> > > >> 
> > > >> Right now the iio device is a child of the physical device, and I am
> > > >> simply passing of_node on to it. guess you are saying that is not
> > > >> correct ?
> > > >> 
> > > >> If so, what would be the correct approach ? Something like the
> > > >> following ?
> > > >> 
> > > >> 	voltage-sensor@35 {
> > > >> 	
> > > >> 		compatible = "maxim,max1139";
> > > >> 		reg = <0x35>;
> > > >> 		vcc-supply = <&reg_3p3v>;
> > > >> 		vref-supply = <&reg_3p3v>;
> > > >> 		
> > > >> 		max1139-iio: iio-device {
> > > >> 		
> > > >> 			device_type = "iio_device";
> > > >> 			#io-channel-cells = <1>;
> > > >> 		
> > > >> 		};
> > > >> 	
> > > >> 	};
> > > >> 
> > > >> and in the driver probe function:
> > > >> 	if (parent->of_node)
> > > >> 	
> > > >> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
> > >of_node,
> > > >> 
> > > >> "iio_device");
> > > >> 
> > > >> Another option would be to use of_find_compatible_node() and
> > > >> something
> > > >> like compatible = "iio-device";
> > > >> in the iio-device node.
> > > > 
> > > > A device node is defined as a node having compatible property. Other
> > > > nodes should be seen as helper nodes, which do not represent devices
> > > > (although they all have struct device_node in Linux).
> > > > 
> > > > Also, AFAIK, device_type is a deprecated property used by some legacy
> > > > PowerPC machines and for current machines only compatible should be
> > > > used.
> > > > 
> > > > So I guess the approach with compatible would be appropriate here.
> > > > 
> > > > However for physical devices providing only a single IIO device it
> > > > might> 
> > > > be better to allow simpler specification, like:
> > > >  	max1139-iio: voltage-sensor@35 {
> > > >  	
> > > >  		compatible = "maxim,max1139", "iio_device";
> > > 
> > > I don't think this makes a lot of sense. First of all iio_device an
> > > artificial Linux term, while the device tree should describe the
> > > hardware.
> > 
> > Well, if you look at an iio_device as a subdevice of a physical device 
> > then it should make a bit more sense. (See nodes of GPIO/pinctrl pin banks 
> > or regulators of a PMIC chip.)
> > 
> > > Secondly there is no generic iio driver which could match on
> > > a node with a "iio_device" compability string and stuff would just
> > > work. I mean we don't do
> > > 
> > > compatible = "atmel,at91sam9260-i2c", "i2c-master";
> > > 
> > > or similar either.
> > 
> > Right. We don't need the other compatible for simple devices with single 
> > subdevice. This is implied by the driver registering a single IIO driver 
> > using the node of physical device.
> > 
> > > >  		reg = <0x35>;
> > > >  		vcc-supply = <&reg_3p3v>;
> > > >  		vref-supply = <&reg_3p3v>;
> > > > 		
> > > > 		device_type = "iio_device";
> > 
> > Also we don't need this device_type. Basically we don't need to specify 
> > whether given node is an iio_device or an iio_trigger. It's up to the 
> > driver to register the node as a device or a trigger by setting dev.of_node 
> > field properly.
> > 
> > So my suggestion would be to make the bindings as following. For single 
> > subdevice:
> > 
> > 	voltage-sensor@35 {
> > 		compatible = "maxim,max1139";
> > 		reg = <0x35>;
> > 		vcc-supply = <&reg_3p3v>;
> > 		vref-supply = <&reg_3p3v>;
> > 		#io-channel-cells = <1>;
> > 	};
> > 
> > For multiple subdevices:
> > 
> > 	voltage-sensor@35 {
> > 		compatible = "maxim,max1139";
> > 		reg = <0x35>;
> > 		vcc-supply = <&reg_3p3v>;
> > 		vref-supply = <&reg_3p3v>;
> > 
> > 		subdevice1 {
> > 			/* Subdevice specific data */
> > 			#io-channel-cells = <1>;
> > 		};
> > 
> > 		subdevice2 {
> > 			/* Subdevice specific data */
> > 			#io-channel-cells = <1>;
> > 		}
> 
> Please provide an example how to parse that. Obviously now I can not look for
> "compatible" anymore. Sure, I can use of_get_child_by_name, but that means
> the sub-device names would have to be well defined. Or I could use
> of_find_node_by_name, but then I would need something like 
> 
>  	voltage-sensor@35 {
>  		compatible = "maxim,max1139";
> 		iio-device;
>  		reg = <0x35>;
>  		vcc-supply = <&reg_3p3v>;
>  		vref-supply = <&reg_3p3v>;
>  		#io-channel-cells = <1>;
>  	};

Never mind. My brain is really too flu-foggy to do anything.

Looks like I can use of_find_node_by_name if subdevice1 and subdevice2 have well defined
names such as iio-device or iio-trigger. It might even be possible to encode
multiple iio subdevices in names such as iio-device@0 and iio-device@1. But that
would (or not ?) mean that the names should be something like the following for
consistency.

	iio-device@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		vcc-supply = <&reg_3p3v>;
		vref-supply = <&reg_3p3v>;
		#io-channel-cells = <1>;
	};

For multiple subdevices:

	voltage-sensor@35 {
		compatible = "maxim,max1139";
		reg = <0x35>;
		vcc-supply = <&reg_3p3v>;
		vref-supply = <&reg_3p3v>;

		iio-device {
			/* Subdevice specific data */
			#io-channel-cells = <1>;
		};

		iio-trigger {
			/* Subdevice specific data */
			#io-channel-cells = <1>;
		}

Does that make sense ?

Thanks,
Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Tomasz Figa]

On Monday 04 of February 2013 09:51:34 Guenter Roeck wrote:
> On Mon, Feb 04, 2013 at 09:12:14AM -0800, Guenter Roeck wrote:
> > On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote:
> > > On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
> > > > On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> > > > > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> > > > >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> > > > >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen 
wrote:
> > > > >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > > > >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > > > >>>>>> Hi Guenter,
> > > > >>>>>> 
> > > > >>>>>> Some comments inline.
> > > > >>>>>> 
> > > > >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck 
wrote:
> > > > >>>>>>> Provide bindings and parse OF data during initialization.
> > > > >>>>>>> 
> > > > >>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > > >>>>>>> ---
> > > > >>>>>>> - Documentation update per feedback
> > > > >>>>>>> - Dropped io-channel-output-names from the bindings
> > > > >>>>>>> document.
> > > > >>>>>>> The
> > > > >>>>>>> property is not used in the code, and it is not entirely
> > > > >>>>>>> clear
> > > > >>>>>>> what
> > > > >>>>>>> it
> > > > >>>>>>> would be used for. If there is a need for it, we can add
> > > > >>>>>>> it back
> > > > >>>>>>> in
> > > > >>>>>>> later on.
> > > > >>>>>>> - Don't export OF specific API calls
> > > > >>>>>>> - For OF support, no longer depend on iio_map
> > > > >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that
> > > > >>>>>>> the
> > > > >>>>>>> code
> > > > >>>>>>> still builds if it is not selected.
> > > > >>>>>>> - Change iio_channel_get to take device pointer as
> > > > >>>>>>> argument
> > > > >>>>>>> instead
> > > > >>>>>>> of
> > > > >>>>>>> device name. Retain old API as of_iio_channel_get_sys.
> > > > >>>>>>> - iio_channel_get now works for both OF and non-OF
> > > > >>>>>>> configurations.
> > > > >>>>>>> 
> > > > >>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> > > > >>>>>>>  ++++++++
> > > > >>>>>>>  drivers/iio/inkern.c                               |  186
> > > > >>>>>>> 
> > > > >>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > > > >>>>>>> 
> > > > >>>>>>>  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..58df5f6
> > > > >>>>>>> --- /dev/null
> > > > >>>>>>> +++
> > > > >>>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > > > >>>>>>> @@ -0,0 +1,76 @@
> > > > >>>>>>> +This binding is a work-in-progress. It is derived from
> > > > >>>>>>> clock
> > > > >>>>>>> bindings,
> > > > >>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
> > > > >>>>>>> +
> > > > >>>>>>> +Sources of IIO channels can be represented by any node in
> > > > >>>>>>> the
> > > > >>>>>>> device
> > > > >>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
> > > > >>>>>>> consumer
> > > > >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
> > > > >>>>>>> provider
> > > > >>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers,
> > > > >>>>>>> an IIO
> > > > >>>>>>> +specifier is an array of one or more cells identifying
> > > > >>>>>>> the IIO
> > > > >>>>>>> +output on a device.  The length of an IIO specifier is
> > > > >>>>>>> defined
> > > > >>>>>>> by
> > > > >>>>>>> the
> > > > >>>>>>> +value of a #io-channel-cells property in the 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.
> > > > >>>>>>> +
> > > > >>>>>>> +For example:
> > > > >>>>>>> +
> > > > >>>>>>> +    adc: adc@35 {
> > > > >>>>>>> +	compatible = "maxim,max1139";
> > > > >>>>>>> +	reg = <0x35>;
> > > > >>>>>>> +        #io-channel-cells = <1>;
> > > > >>>>>>> +    };
> > > > >>>>>>> +
> > > > >>>>>>> +==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 b289915..d48f2a8 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"
> > > > >>>>>>> 
> > > > >>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > > > >>>>>>> 
> > > > >>>>>>>  	return chan;
> > > > >>>>>>>  
> > > > >>>>>>>  }
> > > > >>>>>>> 
> > > > >>>>>>> +#ifdef CONFIG_OF
> > > > >>>>>>> +
> > > > >>>>>>> +static int iio_dev_node_match(struct device *dev, void
> > > > >>>>>>> *data)
> > > > >>>>>>> +{
> > > > >>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
> > > >
> > > >of_node
> > > >
> > > > >>> ==
> > > > >>> 
> > > > >>>>>> data;
> > > > >>>>>> 
> > > > >>>>>> Hmm, do you need to check type name here? One device node
> > > > >>>>>> should
> > > > >>>>>> rather
> > > > >>>>>> represent only one device, making node an unique
> > > > >>>>>> identifier.
> > > > >>>>>> 
> > > > >>>>>> It this is meant to be a sanity check, it could be done one
> > > > >>>>>> time
> > > > >>>>>> after
> > > > >>>>>> finding the device.
> > > > >>>>> 
> > > > >>>>> Hi Tomasz,
> > > > >>>>> 
> > > > >>>>> This is what Lars had suggested earlier:
> > > > >>>>>> 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.
> > > > >>>>> 
> > > > >>>>> Is it really needed, or in other words would it be
> > > > >>>>> sufficient to
> > > > >>>>> check
> > > > >>>>> if of_node and data match each other ? Your reasoning makes
> > > > >>>>> sense
> > > > >>>>> to
> > > > >>>>> me, and I had thought about it as well, but I don't really
> > > > >>>>> know,
> > > > >>>>> and
> > > > >>>>> I don't know how I could test it and guarantee correctness
> > > > >>>>> either.
> > > > >>>>> I'll be happy to take the strcmp() out if someone tells me
> > > > >>>>> that it
> > > > >>>>> is
> > > > >>>>> definitely not needed ...
> > > > >>>> 
> > > > >>>> A IIO trigger and a IIO device may have the same of_node,
> > > > >>>> e.g. if
> > > > >>>> they
> > > > >>>> both belong to the same physical device. But you don't need
> > > > >>>> to do
> > > > >>>> the
> > > > >>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type
> > > > >>>> ==
> > > > >>>> &iio_dev_type. Although it doesn't really matter in practice
> > > > >>>> first
> > > > >>>> check for the of_node then check for the type, since the
> > > > >>>> of_node
> > > > >>>> will
> > > > >>>> only match for a few devices at most, the type will match for
> > > > >>>> quite
> > > > >>>> a
> > > > >>>> few.
> > > > >>> 
> > > > >>> I must disagree.
> > > > >>> 
> > > > >>> If you have two IIO devices provided by one physical device,
> > > > >>> then in
> > > > >>> 
> > > > >>> device tree they should be represented as follows:
> > > > >>> 	phys-dev@12345678 {
> > > > >>> 	
> > > > >>> 		compatible = "some-physical-device";
> > > > >>> 		/* ... */
> > > > >>> 		
> > > > >>> 		my_trig: iio-trigger {
> > > > >>> 		
> > > > >>> 			/* ... */
> > > > >>> 		
> > > > >>> 		};
> > > > >>> 		
> > > > >>> 		my_dev: iio-device {
> > > > >>> 		
> > > > >>> 			/* ... */
> > > > >>> 		
> > > > >>> 		};
> > > > >>> 	
> > > > >>> 	};
> > > > >>> 
> > > > >>> Notice that phys-dev works here as an IIO bus on which its IIO
> > > > >>> devices
> > > > >>> are available. This is related to the convention that single
> > > > >>> OF
> > > > >>> device node represents single device, which would be violated
> > > > >>> otherwise.
> > > > >> 
> > > > >> Right now the iio device is a child of the physical device, and
> > > > >> I am
> > > > >> simply passing of_node on to it. guess you are saying that is
> > > > >> not
> > > > >> correct ?
> > > > >> 
> > > > >> If so, what would be the correct approach ? Something like the
> > > > >> following ?
> > > > >> 
> > > > >> 	voltage-sensor@35 {
> > > > >> 	
> > > > >> 		compatible = "maxim,max1139";
> > > > >> 		reg = <0x35>;
> > > > >> 		vcc-supply = <&reg_3p3v>;
> > > > >> 		vref-supply = <&reg_3p3v>;
> > > > >> 		
> > > > >> 		max1139-iio: iio-device {
> > > > >> 		
> > > > >> 			device_type = "iio_device";
> > > > >> 			#io-channel-cells = <1>;
> > > > >> 		
> > > > >> 		};
> > > > >> 	
> > > > >> 	};
> > > > >> 
> > > > >> and in the driver probe function:
> > > > >> 	if (parent->of_node)
> > > > >> 	
> > > > >> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
> > > >
> > > >of_node,
> > > >
> > > > >> "iio_device");
> > > > >> 
> > > > >> Another option would be to use of_find_compatible_node() and
> > > > >> something
> > > > >> like compatible = "iio-device";
> > > > >> in the iio-device node.
> > > > > 
> > > > > A device node is defined as a node having compatible property.
> > > > > Other
> > > > > nodes should be seen as helper nodes, which do not represent
> > > > > devices
> > > > > (although they all have struct device_node in Linux).
> > > > > 
> > > > > Also, AFAIK, device_type is a deprecated property used by some
> > > > > legacy
> > > > > PowerPC machines and for current machines only compatible should
> > > > > be
> > > > > used.
> > > > > 
> > > > > So I guess the approach with compatible would be appropriate
> > > > > here.
> > > > > 
> > > > > However for physical devices providing only a single IIO device
> > > > > it
> > > > > might>
> > > > > 
> > > > > be better to allow simpler specification, like:
> > > > >  	max1139-iio: voltage-sensor@35 {
> > > > >  	
> > > > >  		compatible = "maxim,max1139", "iio_device";
> > > > 
> > > > I don't think this makes a lot of sense. First of all iio_device
> > > > an
> > > > artificial Linux term, while the device tree should describe the
> > > > hardware.
> > > 
> > > Well, if you look at an iio_device as a subdevice of a physical
> > > device
> > > then it should make a bit more sense. (See nodes of GPIO/pinctrl pin
> > > banks or regulators of a PMIC chip.)
> > > 
> > > > Secondly there is no generic iio driver which could match on
> > > > a node with a "iio_device" compability string and stuff would just
> > > > work. I mean we don't do
> > > > 
> > > > compatible = "atmel,at91sam9260-i2c", "i2c-master";
> > > > 
> > > > or similar either.
> > > 
> > > Right. We don't need the other compatible for simple devices with
> > > single subdevice. This is implied by the driver registering a
> > > single IIO driver using the node of physical device.
> > > 
> > > > >  		reg = <0x35>;
> > > > >  		vcc-supply = <&reg_3p3v>;
> > > > >  		vref-supply = <&reg_3p3v>;
> > > > > 		
> > > > > 		device_type = "iio_device";
> > > 
> > > Also we don't need this device_type. Basically we don't need to
> > > specify
> > > whether given node is an iio_device or an iio_trigger. It's up to
> > > the
> > > driver to register the node as a device or a trigger by setting
> > > dev.of_node field properly.
> > > 
> > > So my suggestion would be to make the bindings as following. For
> > > single
> > > 
> > > subdevice:
> > > 	voltage-sensor@35 {
> > > 	
> > > 		compatible = "maxim,max1139";
> > > 		reg = <0x35>;
> > > 		vcc-supply = <&reg_3p3v>;
> > > 		vref-supply = <&reg_3p3v>;
> > > 		#io-channel-cells = <1>;
> > > 	
> > > 	};
> > > 
> > > For multiple subdevices:
> > > 	voltage-sensor@35 {
> > > 	
> > > 		compatible = "maxim,max1139";
> > > 		reg = <0x35>;
> > > 		vcc-supply = <&reg_3p3v>;
> > > 		vref-supply = <&reg_3p3v>;
> > > 		
> > > 		subdevice1 {
> > > 		
> > > 			/* Subdevice specific data */
> > > 			#io-channel-cells = <1>;
> > > 		
> > > 		};
> > > 		
> > > 		subdevice2 {
> > > 		
> > > 			/* Subdevice specific data */
> > > 			#io-channel-cells = <1>;
> > > 		
> > > 		}
> > 
> > Please provide an example how to parse that. Obviously now I can not
> > look for "compatible" anymore. Sure, I can use of_get_child_by_name,
> > but that means the sub-device names would have to be well defined. Or
> > I could use of_find_node_by_name, but then I would need something
> > like
> > 
> >  	voltage-sensor@35 {
> >  	
> >  		compatible = "maxim,max1139";
> > 		
> > 		iio-device;
> > 		
> >  		reg = <0x35>;
> >  		vcc-supply = <&reg_3p3v>;
> >  		vref-supply = <&reg_3p3v>;
> >  		#io-channel-cells = <1>;
> >  	
> >  	};
> 
> Never mind. My brain is really too flu-foggy to do anything.
> 
> Looks like I can use of_find_node_by_name if subdevice1 and subdevice2
> have well defined names such as iio-device or iio-trigger. It might
> even be possible to encode multiple iio subdevices in names such as
> iio-device@0 and iio-device@1. But that would (or not ?) mean that the
> names should be something like the following for consistency.
> 
> 	iio-device@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		vcc-supply = <&reg_3p3v>;
> 		vref-supply = <&reg_3p3v>;
> 		#io-channel-cells = <1>;
> 	};
> 
> For multiple subdevices:
> 
> 	voltage-sensor@35 {
> 		compatible = "maxim,max1139";
> 		reg = <0x35>;
> 		vcc-supply = <&reg_3p3v>;
> 		vref-supply = <&reg_3p3v>;
> 
> 		iio-device {
> 			/* Subdevice specific data */
> 			#io-channel-cells = <1>;
> 		};
> 
> 		iio-trigger {
> 			/* Subdevice specific data */
> 			#io-channel-cells = <1>;
> 		}
> 
> Does that make sense ?

Do you need to parse this from generic code at all?

A particular IIO device driver gets a pointer to voltage-sensor@35 node in 
its dev.of_node. In case of a simple device without multiple subdevices, 
the driver will set the of_node field of iio_device.dev (or 
iio_trigger.dev) field and register the iio_device using 
iio_device_register() (or iio_trigger_register()).

In case of multiple subdevices, the driver (not generic code) will iterate 
over its child nodes and registers iio_devices and/or iio_triggers with 
appropriate nodes set in iio_device/iio_trigger structs.

All the generic code should do is parsing the phandles in client nodes, 
looking up to which devices they belong and translating phandle arguments 
to channel numbers.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Mon, Feb 04, 2013 at 07:00:55PM +0100, Tomasz Figa wrote:
> On Monday 04 of February 2013 09:51:34 Guenter Roeck wrote:
> > On Mon, Feb 04, 2013 at 09:12:14AM -0800, Guenter Roeck wrote:
> > > On Mon, Feb 04, 2013 at 12:14:52AM +0100, Tomasz Figa wrote:
> > > > On Sunday 03 of February 2013 19:55:47 Lars-Peter Clausen wrote:
> > > > > On 02/03/2013 06:30 PM, Tomasz Figa wrote:
> > > > > > On Sunday 03 of February 2013 09:01:07 Guenter Roeck wrote:
> > > > > >> On Sun, Feb 03, 2013 at 12:52:40PM +0100, Tomasz Figa wrote:
> > > > > >>> On Sunday 03 of February 2013 12:29:23 Lars-Peter Clausen 
> wrote:
> > > > > >>>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > > > > >>>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> > > > > >>>>>> Hi Guenter,
> > > > > >>>>>> 
> > > > > >>>>>> Some comments inline.
> > > > > >>>>>> 
> > > > > >>>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck 
> wrote:
> > > > > >>>>>>> Provide bindings and parse OF data during initialization.
> > > > > >>>>>>> 
> > > > > >>>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > > > > >>>>>>> ---
> > > > > >>>>>>> - Documentation update per feedback
> > > > > >>>>>>> - Dropped io-channel-output-names from the bindings
> > > > > >>>>>>> document.
> > > > > >>>>>>> The
> > > > > >>>>>>> property is not used in the code, and it is not entirely
> > > > > >>>>>>> clear
> > > > > >>>>>>> what
> > > > > >>>>>>> it
> > > > > >>>>>>> would be used for. If there is a need for it, we can add
> > > > > >>>>>>> it back
> > > > > >>>>>>> in
> > > > > >>>>>>> later on.
> > > > > >>>>>>> - Don't export OF specific API calls
> > > > > >>>>>>> - For OF support, no longer depend on iio_map
> > > > > >>>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that
> > > > > >>>>>>> the
> > > > > >>>>>>> code
> > > > > >>>>>>> still builds if it is not selected.
> > > > > >>>>>>> - Change iio_channel_get to take device pointer as
> > > > > >>>>>>> argument
> > > > > >>>>>>> instead
> > > > > >>>>>>> of
> > > > > >>>>>>> device name. Retain old API as of_iio_channel_get_sys.
> > > > > >>>>>>> - iio_channel_get now works for both OF and non-OF
> > > > > >>>>>>> configurations.
> > > > > >>>>>>> 
> > > > > >>>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76
> > > > > >>>>>>>  ++++++++
> > > > > >>>>>>>  drivers/iio/inkern.c                               |  186
> > > > > >>>>>>> 
> > > > > >>>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> > > > > >>>>>>> 
> > > > > >>>>>>>  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..58df5f6
> > > > > >>>>>>> --- /dev/null
> > > > > >>>>>>> +++
> > > > > >>>>>>> b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > > > > >>>>>>> @@ -0,0 +1,76 @@
> > > > > >>>>>>> +This binding is a work-in-progress. It is derived from
> > > > > >>>>>>> clock
> > > > > >>>>>>> bindings,
> > > > > >>>>>>> +and based on suggestions from Lars-Peter Clausen [1].
> > > > > >>>>>>> +
> > > > > >>>>>>> +Sources of IIO channels can be represented by any node in
> > > > > >>>>>>> the
> > > > > >>>>>>> device
> > > > > >>>>>>> +tree.  Those nodes are designated as IIO providers.  IIO
> > > > > >>>>>>> consumer
> > > > > >>>>>>> +nodes use a phandle and IIO specifier pair to connect IIO
> > > > > >>>>>>> provider
> > > > > >>>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers,
> > > > > >>>>>>> an IIO
> > > > > >>>>>>> +specifier is an array of one or more cells identifying
> > > > > >>>>>>> the IIO
> > > > > >>>>>>> +output on a device.  The length of an IIO specifier is
> > > > > >>>>>>> defined
> > > > > >>>>>>> by
> > > > > >>>>>>> the
> > > > > >>>>>>> +value of a #io-channel-cells property in the 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.
> > > > > >>>>>>> +
> > > > > >>>>>>> +For example:
> > > > > >>>>>>> +
> > > > > >>>>>>> +    adc: adc@35 {
> > > > > >>>>>>> +	compatible = "maxim,max1139";
> > > > > >>>>>>> +	reg = <0x35>;
> > > > > >>>>>>> +        #io-channel-cells = <1>;
> > > > > >>>>>>> +    };
> > > > > >>>>>>> +
> > > > > >>>>>>> +==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 b289915..d48f2a8 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"
> > > > > >>>>>>> 
> > > > > >>>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> > > > > >>>>>>> 
> > > > > >>>>>>>  	return chan;
> > > > > >>>>>>>  
> > > > > >>>>>>>  }
> > > > > >>>>>>> 
> > > > > >>>>>>> +#ifdef CONFIG_OF
> > > > > >>>>>>> +
> > > > > >>>>>>> +static int iio_dev_node_match(struct device *dev, void
> > > > > >>>>>>> *data)
> > > > > >>>>>>> +{
> > > > > >>>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev-
> > > > >
> > > > >of_node
> > > > >
> > > > > >>> ==
> > > > > >>> 
> > > > > >>>>>> data;
> > > > > >>>>>> 
> > > > > >>>>>> Hmm, do you need to check type name here? One device node
> > > > > >>>>>> should
> > > > > >>>>>> rather
> > > > > >>>>>> represent only one device, making node an unique
> > > > > >>>>>> identifier.
> > > > > >>>>>> 
> > > > > >>>>>> It this is meant to be a sanity check, it could be done one
> > > > > >>>>>> time
> > > > > >>>>>> after
> > > > > >>>>>> finding the device.
> > > > > >>>>> 
> > > > > >>>>> Hi Tomasz,
> > > > > >>>>> 
> > > > > >>>>> This is what Lars had suggested earlier:
> > > > > >>>>>> 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.
> > > > > >>>>> 
> > > > > >>>>> Is it really needed, or in other words would it be
> > > > > >>>>> sufficient to
> > > > > >>>>> check
> > > > > >>>>> if of_node and data match each other ? Your reasoning makes
> > > > > >>>>> sense
> > > > > >>>>> to
> > > > > >>>>> me, and I had thought about it as well, but I don't really
> > > > > >>>>> know,
> > > > > >>>>> and
> > > > > >>>>> I don't know how I could test it and guarantee correctness
> > > > > >>>>> either.
> > > > > >>>>> I'll be happy to take the strcmp() out if someone tells me
> > > > > >>>>> that it
> > > > > >>>>> is
> > > > > >>>>> definitely not needed ...
> > > > > >>>> 
> > > > > >>>> A IIO trigger and a IIO device may have the same of_node,
> > > > > >>>> e.g. if
> > > > > >>>> they
> > > > > >>>> both belong to the same physical device. But you don't need
> > > > > >>>> to do
> > > > > >>>> the
> > > > > >>>> strcmp just compare dev->type to iio_dev_type i.e. dev->type
> > > > > >>>> ==
> > > > > >>>> &iio_dev_type. Although it doesn't really matter in practice
> > > > > >>>> first
> > > > > >>>> check for the of_node then check for the type, since the
> > > > > >>>> of_node
> > > > > >>>> will
> > > > > >>>> only match for a few devices at most, the type will match for
> > > > > >>>> quite
> > > > > >>>> a
> > > > > >>>> few.
> > > > > >>> 
> > > > > >>> I must disagree.
> > > > > >>> 
> > > > > >>> If you have two IIO devices provided by one physical device,
> > > > > >>> then in
> > > > > >>> 
> > > > > >>> device tree they should be represented as follows:
> > > > > >>> 	phys-dev@12345678 {
> > > > > >>> 	
> > > > > >>> 		compatible = "some-physical-device";
> > > > > >>> 		/* ... */
> > > > > >>> 		
> > > > > >>> 		my_trig: iio-trigger {
> > > > > >>> 		
> > > > > >>> 			/* ... */
> > > > > >>> 		
> > > > > >>> 		};
> > > > > >>> 		
> > > > > >>> 		my_dev: iio-device {
> > > > > >>> 		
> > > > > >>> 			/* ... */
> > > > > >>> 		
> > > > > >>> 		};
> > > > > >>> 	
> > > > > >>> 	};
> > > > > >>> 
> > > > > >>> Notice that phys-dev works here as an IIO bus on which its IIO
> > > > > >>> devices
> > > > > >>> are available. This is related to the convention that single
> > > > > >>> OF
> > > > > >>> device node represents single device, which would be violated
> > > > > >>> otherwise.
> > > > > >> 
> > > > > >> Right now the iio device is a child of the physical device, and
> > > > > >> I am
> > > > > >> simply passing of_node on to it. guess you are saying that is
> > > > > >> not
> > > > > >> correct ?
> > > > > >> 
> > > > > >> If so, what would be the correct approach ? Something like the
> > > > > >> following ?
> > > > > >> 
> > > > > >> 	voltage-sensor@35 {
> > > > > >> 	
> > > > > >> 		compatible = "maxim,max1139";
> > > > > >> 		reg = <0x35>;
> > > > > >> 		vcc-supply = <&reg_3p3v>;
> > > > > >> 		vref-supply = <&reg_3p3v>;
> > > > > >> 		
> > > > > >> 		max1139-iio: iio-device {
> > > > > >> 		
> > > > > >> 			device_type = "iio_device";
> > > > > >> 			#io-channel-cells = <1>;
> > > > > >> 		
> > > > > >> 		};
> > > > > >> 	
> > > > > >> 	};
> > > > > >> 
> > > > > >> and in the driver probe function:
> > > > > >> 	if (parent->of_node)
> > > > > >> 	
> > > > > >> 		iio_dev->dev.of_node = of_find_node_by_type(parent-
> > > > >
> > > > >of_node,
> > > > >
> > > > > >> "iio_device");
> > > > > >> 
> > > > > >> Another option would be to use of_find_compatible_node() and
> > > > > >> something
> > > > > >> like compatible = "iio-device";
> > > > > >> in the iio-device node.
> > > > > > 
> > > > > > A device node is defined as a node having compatible property.
> > > > > > Other
> > > > > > nodes should be seen as helper nodes, which do not represent
> > > > > > devices
> > > > > > (although they all have struct device_node in Linux).
> > > > > > 
> > > > > > Also, AFAIK, device_type is a deprecated property used by some
> > > > > > legacy
> > > > > > PowerPC machines and for current machines only compatible should
> > > > > > be
> > > > > > used.
> > > > > > 
> > > > > > So I guess the approach with compatible would be appropriate
> > > > > > here.
> > > > > > 
> > > > > > However for physical devices providing only a single IIO device
> > > > > > it
> > > > > > might>
> > > > > > 
> > > > > > be better to allow simpler specification, like:
> > > > > >  	max1139-iio: voltage-sensor@35 {
> > > > > >  	
> > > > > >  		compatible = "maxim,max1139", "iio_device";
> > > > > 
> > > > > I don't think this makes a lot of sense. First of all iio_device
> > > > > an
> > > > > artificial Linux term, while the device tree should describe the
> > > > > hardware.
> > > > 
> > > > Well, if you look at an iio_device as a subdevice of a physical
> > > > device
> > > > then it should make a bit more sense. (See nodes of GPIO/pinctrl pin
> > > > banks or regulators of a PMIC chip.)
> > > > 
> > > > > Secondly there is no generic iio driver which could match on
> > > > > a node with a "iio_device" compability string and stuff would just
> > > > > work. I mean we don't do
> > > > > 
> > > > > compatible = "atmel,at91sam9260-i2c", "i2c-master";
> > > > > 
> > > > > or similar either.
> > > > 
> > > > Right. We don't need the other compatible for simple devices with
> > > > single subdevice. This is implied by the driver registering a
> > > > single IIO driver using the node of physical device.
> > > > 
> > > > > >  		reg = <0x35>;
> > > > > >  		vcc-supply = <&reg_3p3v>;
> > > > > >  		vref-supply = <&reg_3p3v>;
> > > > > > 		
> > > > > > 		device_type = "iio_device";
> > > > 
> > > > Also we don't need this device_type. Basically we don't need to
> > > > specify
> > > > whether given node is an iio_device or an iio_trigger. It's up to
> > > > the
> > > > driver to register the node as a device or a trigger by setting
> > > > dev.of_node field properly.
> > > > 
> > > > So my suggestion would be to make the bindings as following. For
> > > > single
> > > > 
> > > > subdevice:
> > > > 	voltage-sensor@35 {
> > > > 	
> > > > 		compatible = "maxim,max1139";
> > > > 		reg = <0x35>;
> > > > 		vcc-supply = <&reg_3p3v>;
> > > > 		vref-supply = <&reg_3p3v>;
> > > > 		#io-channel-cells = <1>;
> > > > 	
> > > > 	};
> > > > 
> > > > For multiple subdevices:
> > > > 	voltage-sensor@35 {
> > > > 	
> > > > 		compatible = "maxim,max1139";
> > > > 		reg = <0x35>;
> > > > 		vcc-supply = <&reg_3p3v>;
> > > > 		vref-supply = <&reg_3p3v>;
> > > > 		
> > > > 		subdevice1 {
> > > > 		
> > > > 			/* Subdevice specific data */
> > > > 			#io-channel-cells = <1>;
> > > > 		
> > > > 		};
> > > > 		
> > > > 		subdevice2 {
> > > > 		
> > > > 			/* Subdevice specific data */
> > > > 			#io-channel-cells = <1>;
> > > > 		
> > > > 		}
> > > 
> > > Please provide an example how to parse that. Obviously now I can not
> > > look for "compatible" anymore. Sure, I can use of_get_child_by_name,
> > > but that means the sub-device names would have to be well defined. Or
> > > I could use of_find_node_by_name, but then I would need something
> > > like
> > > 
> > >  	voltage-sensor@35 {
> > >  	
> > >  		compatible = "maxim,max1139";
> > > 		
> > > 		iio-device;
> > > 		
> > >  		reg = <0x35>;
> > >  		vcc-supply = <&reg_3p3v>;
> > >  		vref-supply = <&reg_3p3v>;
> > >  		#io-channel-cells = <1>;
> > >  	
> > >  	};
> > 
> > Never mind. My brain is really too flu-foggy to do anything.
> > 
> > Looks like I can use of_find_node_by_name if subdevice1 and subdevice2
> > have well defined names such as iio-device or iio-trigger. It might
> > even be possible to encode multiple iio subdevices in names such as
> > iio-device@0 and iio-device@1. But that would (or not ?) mean that the
> > names should be something like the following for consistency.
> > 
> > 	iio-device@35 {
> > 		compatible = "maxim,max1139";
> > 		reg = <0x35>;
> > 		vcc-supply = <&reg_3p3v>;
> > 		vref-supply = <&reg_3p3v>;
> > 		#io-channel-cells = <1>;
> > 	};
> > 
> > For multiple subdevices:
> > 
> > 	voltage-sensor@35 {
> > 		compatible = "maxim,max1139";
> > 		reg = <0x35>;
> > 		vcc-supply = <&reg_3p3v>;
> > 		vref-supply = <&reg_3p3v>;
> > 
> > 		iio-device {
> > 			/* Subdevice specific data */
> > 			#io-channel-cells = <1>;
> > 		};
> > 
> > 		iio-trigger {
> > 			/* Subdevice specific data */
> > 			#io-channel-cells = <1>;
> > 		}
> > 
> > Does that make sense ?
> 
> Do you need to parse this from generic code at all?
> 
> A particular IIO device driver gets a pointer to voltage-sensor@35 node in 
> its dev.of_node. In case of a simple device without multiple subdevices, 
> the driver will set the of_node field of iio_device.dev (or 
> iio_trigger.dev) field and register the iio_device using 
> iio_device_register() (or iio_trigger_register()).
> 
> In case of multiple subdevices, the driver (not generic code) will iterate 
> over its child nodes and registers iio_devices and/or iio_triggers with 
> appropriate nodes set in iio_device/iio_trigger structs.
> 
> All the generic code should do is parsing the phandles in client nodes, 
> looking up to which devices they belong and translating phandle arguments 
> to channel numbers.
> 
Ok, I think now I understand (or at least a bit better). That would also solve
the problem mentioned by Jonathan, where a driver for a single chip registers
multiple IIO devices.

Thanks,
Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 12:29:23PM +0100, Lars-Peter Clausen wrote:
> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
> > On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
> >> Hi Guenter,
> >>
> >> Some comments inline.
> >>
> >> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
> >>> Provide bindings and parse OF data during initialization.
> >>>
> >>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> >>> ---
> >>> - Documentation update per feedback
> >>> - Dropped io-channel-output-names from the bindings document. The
> >>> property is not used in the code, and it is not entirely clear what it
> >>> would be used for. If there is a need for it, we can add it back in
> >>> later on.
> >>> - Don't export OF specific API calls
> >>> - For OF support, no longer depend on iio_map
> >>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
> >>> builds if it is not selected.
> >>> - Change iio_channel_get to take device pointer as argument instead of
> >>> device name. Retain old API as of_iio_channel_get_sys.
> >>> - iio_channel_get now works for both OF and non-OF configurations.
> >>>
> >>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
> >>>  drivers/iio/inkern.c                               |  186
> >>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
> >>>  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..58df5f6
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> >>> @@ -0,0 +1,76 @@
> >>> +This binding is a work-in-progress. It is derived from clock bindings,
> >>> +and based on suggestions from Lars-Peter Clausen [1].
> >>> +
> >>> +Sources of IIO channels can be represented by any node in the device
> >>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
> >>> +nodes use a phandle and IIO specifier pair to connect IIO provider
> >>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> >>> +specifier is an array of one or more cells identifying the IIO
> >>> +output on a device.  The length of an IIO specifier is defined by the
> >>> +value of a #io-channel-cells property in the 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.
> >>> +
> >>> +For example:
> >>> +
> >>> +    adc: adc@35 {
> >>> +	compatible = "maxim,max1139";
> >>> +	reg = <0x35>;
> >>> +        #io-channel-cells = <1>;
> >>> +    };
> >>> +
> >>> +==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 b289915..d48f2a8 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"
> >>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
> >>>  	return chan;
> >>>  }
> >>>
> >>> +#ifdef CONFIG_OF
> >>> +
> >>> +static int iio_dev_node_match(struct device *dev, void *data)
> >>> +{
> >>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node == 
> >> data;
> >>
> >> Hmm, do you need to check type name here? One device node should rather 
> >> represent only one device, making node an unique identifier.
> >>
> >> It this is meant to be a sanity check, it could be done one time after 
> >> finding the device.
> >>
> > Hi Tomasz,
> > 
> > This is what Lars had suggested earlier:
> > 
> >> 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.
> > 
> > Is it really needed, or in other words would it be sufficient to check if
> > of_node and data match each other ? Your reasoning makes sense to me, and I had
> > thought about it as well, but I don't really know, and I don't know how I could
> > test it and guarantee correctness either. I'll be happy to take the strcmp() out
> > if someone tells me that it is definitely not needed ...
> 
> A IIO trigger and a IIO device may have the same of_node, e.g. if they both
> belong to the same physical device. But you don't need to do the strcmp just
> compare dev->type to iio_dev_type i.e. dev->type == &iio_dev_type. Although it
> doesn't really matter in practice first check for the of_node then check for
> the type, since the of_node will only match for a few devices at most, the type
> will match for quite a few.
> 
iio_dev_type is defined as static variable in industrialio-core.c, which can be
loaded as module. To use it in inkern.c, I would have to move it there and make
it global. I can do that, but is it worth it just to safe the strcmp ?

Thanks,
Guenter

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 05:31 PM, Guenter Roeck wrote:
> On Sun, Feb 03, 2013 at 12:29:23PM +0100, Lars-Peter Clausen wrote:
>> On 02/03/2013 03:06 AM, Guenter Roeck wrote:
>>> On Sun, Feb 03, 2013 at 02:30:24AM +0100, Tomasz Figa wrote:
>>>> Hi Guenter,
>>>>
>>>> Some comments inline.
>>>>
>>>> On Saturday 02 of February 2013 16:59:40 Guenter Roeck wrote:
>>>>> Provide bindings and parse OF data during initialization.
>>>>>
>>>>> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
>>>>> ---
>>>>> - Documentation update per feedback
>>>>> - Dropped io-channel-output-names from the bindings document. The
>>>>> property is not used in the code, and it is not entirely clear what it
>>>>> would be used for. If there is a need for it, we can add it back in
>>>>> later on.
>>>>> - Don't export OF specific API calls
>>>>> - For OF support, no longer depend on iio_map
>>>>> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still
>>>>> builds if it is not selected.
>>>>> - Change iio_channel_get to take device pointer as argument instead of
>>>>> device name. Retain old API as of_iio_channel_get_sys.
>>>>> - iio_channel_get now works for both OF and non-OF configurations.
>>>>>
>>>>>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
>>>>>  drivers/iio/inkern.c                               |  186
>>>>> ++++++++++++++++++++ 2 files changed, 262 insertions(+)
>>>>>  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..58df5f6
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
>>>>> @@ -0,0 +1,76 @@
>>>>> +This binding is a work-in-progress. It is derived from clock bindings,
>>>>> +and based on suggestions from Lars-Peter Clausen [1].
>>>>> +
>>>>> +Sources of IIO channels can be represented by any node in the device
>>>>> +tree.  Those nodes are designated as IIO providers.  IIO consumer
>>>>> +nodes use a phandle and IIO specifier pair to connect IIO provider
>>>>> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
>>>>> +specifier is an array of one or more cells identifying the IIO
>>>>> +output on a device.  The length of an IIO specifier is defined by the
>>>>> +value of a #io-channel-cells property in the 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.
>>>>> +
>>>>> +For example:
>>>>> +
>>>>> +    adc: adc@35 {
>>>>> +	compatible = "maxim,max1139";
>>>>> +	reg = <0x35>;
>>>>> +        #io-channel-cells = <1>;
>>>>> +    };
>>>>> +
>>>>> +==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 b289915..d48f2a8 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"
>>>>> @@ -92,6 +93,179 @@ static const struct iio_chan_spec
>>>>>  	return chan;
>>>>>  }
>>>>>
>>>>> +#ifdef CONFIG_OF
>>>>> +
>>>>> +static int iio_dev_node_match(struct device *dev, void *data)
>>>>> +{
>>>>> +	return !strcmp(dev->type->name, "iio_device") && dev->of_node == 
>>>> data;
>>>>
>>>> Hmm, do you need to check type name here? One device node should rather 
>>>> represent only one device, making node an unique identifier.
>>>>
>>>> It this is meant to be a sanity check, it could be done one time after 
>>>> finding the device.
>>>>
>>> Hi Tomasz,
>>>
>>> This is what Lars had suggested earlier:
>>>
>>>> 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.
>>>
>>> Is it really needed, or in other words would it be sufficient to check if
>>> of_node and data match each other ? Your reasoning makes sense to me, and I had
>>> thought about it as well, but I don't really know, and I don't know how I could
>>> test it and guarantee correctness either. I'll be happy to take the strcmp() out
>>> if someone tells me that it is definitely not needed ...
>>
>> A IIO trigger and a IIO device may have the same of_node, e.g. if they both
>> belong to the same physical device. But you don't need to do the strcmp just
>> compare dev->type to iio_dev_type i.e. dev->type == &iio_dev_type. Although it
>> doesn't really matter in practice first check for the of_node then check for
>> the type, since the of_node will only match for a few devices at most, the type
>> will match for quite a few.
>>
> iio_dev_type is defined as static variable in industrialio-core.c, which can be
> loaded as module. To use it in inkern.c, I would have to move it there and make
> it global. I can do that, but is it worth it just to safe the strcmp ?

inkern.c and industrialio-core.c are in the same compilation unit as far as I
can see:

industrialio-y := industrialio-core.o industrialio-event.o inkern.o

So I don't see a problem there, just make it non-static and a declaration for
it to iio-core.h. Or maybe even better add a of_find_iio_device_by_node() function.

- Lars

Re: [PATCH v2 4/4] iio: Add OF support

[Lars-Peter Clausen]

On 02/03/2013 01:59 AM, Guenter Roeck wrote:
> Provide bindings and parse OF data during initialization.
> 
> Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> ---
> - Documentation update per feedback
> - Dropped io-channel-output-names from the bindings document. The property is
>   not used in the code, and it is not entirely clear what it would be used for.
>   If there is a need for it, we can add it back in later on.
> - Don't export OF specific API calls
> - For OF support, no longer depend on iio_map
> - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still builds
>   if it is not selected.
> - Change iio_channel_get to take device pointer as argument instead of device
>   name. Retain old API as of_iio_channel_get_sys.
> - iio_channel_get now works for both OF and non-OF configurations.
> 

>From my point of view this looks good in general now, Just a few comments.

>  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
>  drivers/iio/inkern.c                               |  186 ++++++++++++++++++++
>  2 files changed, 262 insertions(+)
>  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..58df5f6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> @@ -0,0 +1,76 @@
> +This binding is a work-in-progress. It is derived from clock bindings,
> +and based on suggestions from Lars-Peter Clausen [1].
> +
> +Sources of IIO channels can be represented by any node in the device
> +tree.  Those nodes are designated as IIO providers.  IIO consumer
> +nodes use a phandle and IIO specifier pair to connect IIO provider
> +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> +specifier is an array of one or more cells identifying the IIO
> +output on a device.  The length of an IIO specifier is defined by the
> +value of a #io-channel-cells property in the clock provider node.
> +

Is the extra space at the begining of each sentence on purpose?

> +[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.
> +
> +For example:
> +
> +    adc: adc@35 {
> +	compatible = "maxim,max1139";
> +	reg = <0x35>;
> +        #io-channel-cells = <1>;

You are mixing tabs and spaces here for indention.

> +    };
> +
> +==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>;

I'm not sure how much sense this example makes, since you can only request
those channels which have a name.

> +		io-channel-names = "vcc", "vdd", "vref", "1.2V";
> +	};
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index b289915..d48f2a8 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -10,6 +10,7 @@
[...]

+
> +static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> +{
> +	struct iio_channel *chans;
> +	int i, mapind, nummaps = 0;
> +	int ret;
> +
> +	do {
> +		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)	/* no error, return NULL to search map table */
> +		return NULL;
> +
> +	/* NULL terminated array to save passing size */
> +	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);

I think using kcalloc makes sense here.

> +	if (chans == NULL) {
> +		ret = -ENOMEM;
> +		goto error;
> +	}
> +
> +	/* Search for OF matches */
> +	for (mapind = 0; mapind < nummaps; mapind++) {
> +		struct device *idev;
> +		struct iio_dev *indio_dev;
> +		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;
> +
> +		idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> +				       iio_dev_node_match);
> +		of_node_put(iiospec.np);
> +		if (idev == NULL) {
> +			ret = -EPROBE_DEFER;
> +			goto error_free_chans;
> +		}
> +		indio_dev = dev_to_iio_dev(idev);
> +		channel = iiospec.args_count ? iiospec.args[0] : 0;
> +		if (channel >= indio_dev->num_channels) {
> +			ret = -EINVAL;
> +			goto error_free_chans;
> +		}

Hm, I wonder if we can share some code with of_iio_get_channel here, since it
is pretty much the same code. Maybe put the whole parse_phandle and device
lookup in a heler function.

> +		chans[mapind].indio_dev = indio_dev;
> +		chans[mapind].channel = &indio_dev->channels[channel];
> +	}
> +	return chans;
> +
> +error_free_chans:
> +	for (i = 0; i < mapind; i++)
> +		iio_device_put(chans[i].indio_dev);
> +	kfree(chans);
> +error:
> +	return ERR_PTR(ret);
> +}
> +
> +#else /* CONFIG_OF */
> +
> +static inline struct iio_channel *
> +of_iio_channel_get_by_name(struct device_node *np, const char *name)
> +{
> +	return ERR_PTR(-ENOENT);
> +}
> +
> +static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +#endif /* CONFIG_OF */
>  
>  static struct iio_channel *iio_channel_get_sys(const char *name,
>  					       const char *channel_name)
> @@ -150,7 +324,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
>  				    const char *channel_name)
>  {
>  	const char *name = dev ? dev_name(dev) : NULL;
> +	struct iio_channel *channel;
>  
> +	if (dev) {
> +		channel = of_iio_channel_get_by_name(dev->of_node,
> +						     channel_name);
> +		if (!IS_ERR(channel))

Hm, I wonder if we should use the same semantics as for of_iio_channel_get_all
here. NULL means there is no channel use the map lookup and error code means
there is a channel, but there was an error requesting it. As it is right now
for probe deferral won't work, since EPROBE_DEFER is not passed on.


> +			return channel;
> +	}
>  	return iio_channel_get_sys(name, channel_name);
>  }
>  EXPORT_SYMBOL_GPL(iio_channel_get);
> @@ -173,6 +354,11 @@ struct iio_channel *iio_channel_get_all(struct device *dev)
>  
>  	if (dev == NULL)
>  		return ERR_PTR(-EINVAL);
> +
> +	chans = of_iio_channel_get_all(dev);
> +	if (chans)
> +		return chans;
> +
>  	name = dev_name(dev);
>  
>  	mutex_lock(&iio_map_list_lock);

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 03:17:57PM +0100, Lars-Peter Clausen wrote:
> On 02/03/2013 01:59 AM, Guenter Roeck wrote:
> > Provide bindings and parse OF data during initialization.
> > 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> > ---
> > - Documentation update per feedback
> > - Dropped io-channel-output-names from the bindings document. The property is
> >   not used in the code, and it is not entirely clear what it would be used for.
> >   If there is a need for it, we can add it back in later on.
> > - Don't export OF specific API calls
> > - For OF support, no longer depend on iio_map
> > - Add #ifdef CONFIG_OF where appropriate, and ensure that the code still builds
> >   if it is not selected.
> > - Change iio_channel_get to take device pointer as argument instead of device
> >   name. Retain old API as of_iio_channel_get_sys.
> > - iio_channel_get now works for both OF and non-OF configurations.
> > 
> 
> From my point of view this looks good in general now, Just a few comments.
> 
> >  .../devicetree/bindings/iio/iio-bindings.txt       |   76 ++++++++
> >  drivers/iio/inkern.c                               |  186 ++++++++++++++++++++
> >  2 files changed, 262 insertions(+)
> >  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..58df5f6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/iio-bindings.txt
> > @@ -0,0 +1,76 @@
> > +This binding is a work-in-progress. It is derived from clock bindings,
> > +and based on suggestions from Lars-Peter Clausen [1].
> > +
> > +Sources of IIO channels can be represented by any node in the device
> > +tree.  Those nodes are designated as IIO providers.  IIO consumer
> > +nodes use a phandle and IIO specifier pair to connect IIO provider
> > +outputs to IIO inputs.  Similar to the gpio specifiers, an IIO
> > +specifier is an array of one or more cells identifying the IIO
> > +output on a device.  The length of an IIO specifier is defined by the
> > +value of a #io-channel-cells property in the clock provider node.
> > +
> 
> Is the extra space at the begining of each sentence on purpose?
> 
Copied from clock code, and used throughout documentation here or there.
Not my style, really, and I'll likely forget to do it if/when I add text myself,
so I'll change it.

[ ... ]

> > +	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>;
> 
> I'm not sure how much sense this example makes, since you can only request
> those channels which have a name.
> 
It does - of_iio_channel_get_all() returns all channels even if there are no names.

> [...]
> 
> +
> > +static struct iio_channel *of_iio_channel_get_all(struct device *dev)
> > +{
> > +	struct iio_channel *chans;
> > +	int i, mapind, nummaps = 0;
> > +	int ret;
> > +
> > +	do {
> > +		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)	/* no error, return NULL to search map table */
> > +		return NULL;
> > +
> > +	/* NULL terminated array to save passing size */
> > +	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
> 
> I think using kcalloc makes sense here.
> 
that would leave chan->data uninitialized, and I would have to initialize it
explicitly. also, if additional fields are ever added, we would risk having
uninitialized fields. Using kzalloc avoids a potential future error case, so I
would prefer to keep it.

> > +	if (chans == NULL) {
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	/* Search for OF matches */
> > +	for (mapind = 0; mapind < nummaps; mapind++) {
> > +		struct device *idev;
> > +		struct iio_dev *indio_dev;
> > +		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;
> > +
> > +		idev = bus_find_device(&iio_bus_type, NULL, iiospec.np,
> > +				       iio_dev_node_match);
> > +		of_node_put(iiospec.np);
> > +		if (idev == NULL) {
> > +			ret = -EPROBE_DEFER;
> > +			goto error_free_chans;
> > +		}
> > +		indio_dev = dev_to_iio_dev(idev);
> > +		channel = iiospec.args_count ? iiospec.args[0] : 0;
> > +		if (channel >= indio_dev->num_channels) {
> > +			ret = -EINVAL;
> > +			goto error_free_chans;
> > +		}
> 
> Hm, I wonder if we can share some code with of_iio_get_channel here, since it
> is pretty much the same code. Maybe put the whole parse_phandle and device
> lookup in a heler function.
> 
Done; introduced __of_iio_get_channel to be called by both functions.

> > +		chans[mapind].indio_dev = indio_dev;
> > +		chans[mapind].channel = &indio_dev->channels[channel];
> > +	}
> > +	return chans;
> > +
> > +error_free_chans:
> > +	for (i = 0; i < mapind; i++)
> > +		iio_device_put(chans[i].indio_dev);
> > +	kfree(chans);
> > +error:
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +#else /* CONFIG_OF */
> > +
> > +static inline struct iio_channel *
> > +of_iio_channel_get_by_name(struct device_node *np, const char *name)
> > +{
> > +	return ERR_PTR(-ENOENT);
> > +}
> > +
> > +static inline struct iio_channel *of_iio_channel_get_all(struct device *dev)
> > +{
> > +	return NULL;
> > +}
> > +
> > +#endif /* CONFIG_OF */
> >  
> >  static struct iio_channel *iio_channel_get_sys(const char *name,
> >  					       const char *channel_name)
> > @@ -150,7 +324,14 @@ struct iio_channel *iio_channel_get(struct device *dev,
> >  				    const char *channel_name)
> >  {
> >  	const char *name = dev ? dev_name(dev) : NULL;
> > +	struct iio_channel *channel;
> >  
> > +	if (dev) {
> > +		channel = of_iio_channel_get_by_name(dev->of_node,
> > +						     channel_name);
> > +		if (!IS_ERR(channel))
> 
> Hm, I wonder if we should use the same semantics as for of_iio_channel_get_all
> here. NULL means there is no channel use the map lookup and error code means
> there is a channel, but there was an error requesting it. As it is right now
> for probe deferral won't work, since EPROBE_DEFER is not passed on.
> 
Yes, you are right. I'll try, and also write some test code to actually test it
to make sure that it works as intended.

Re: [PATCH v2 4/4] iio: Add OF support

[Guenter Roeck]

On Sun, Feb 03, 2013 at 08:22:13AM -0800, Guenter Roeck wrote:
[ ... ]

> > > +
> > > +	/* NULL terminated array to save passing size */
> > > +	chans = kzalloc(sizeof(*chans)*(nummaps + 1), GFP_KERNEL);
> > 
> > I think using kcalloc makes sense here.
> > 
> that would leave chan->data uninitialized, and I would have to initialize it
> explicitly. also, if additional fields are ever added, we would risk having
> uninitialized fields. Using kzalloc avoids a potential future error case, so I
> would prefer to keep it.
> 
Please ignore this one.

Looks like my brain was too flu-foggy to realize that kcalloc does clear the
memory.

Guenter