[PATCH 1/3] ti_adc: Update with IIO map interface

[PATCH 1/3] ti_adc: Update with IIO map interface

[Pantelis Antoniou]

Add an IIO map interface that consumers can use.

Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
---
 drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 02a43c8..8595a90 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -20,8 +20,9 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
-#include <linux/io.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 #include <linux/platform_data/ti_am335x_adc.h>
@@ -29,6 +30,8 @@
 struct tiadc_device {
 	struct ti_tscadc_dev *mfd_tscadc;
 	int channels;
+	char *buf;
+	struct iio_map *map;
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
 }
 
-static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
+static int tiadc_channel_init(struct iio_dev *indio_dev,
+		struct tiadc_device *adc_dev)
 {
 	struct iio_chan_spec *chan_array;
-	int i;
-
-	indio_dev->num_channels = channels;
-	chan_array = kcalloc(indio_dev->num_channels,
-			sizeof(struct iio_chan_spec), GFP_KERNEL);
+	struct iio_chan_spec *chan;
+	char *s;
+	int i, len, size, ret;
+	int channels = adc_dev->channels;
 
+	size = channels * (sizeof(struct iio_chan_spec) + 6);
+	chan_array = kzalloc(size, GFP_KERNEL);
 	if (chan_array == NULL)
 		return -ENOMEM;
 
-	for (i = 0; i < (indio_dev->num_channels); i++) {
-		struct iio_chan_spec *chan = chan_array + i;
+	/* buffer space is after the array */
+	s = (char *)(chan_array + channels);
+	chan = chan_array;
+	for (i = 0; i < channels; i++, chan++, s += len + 1) {
+
+		len = sprintf(s, "AIN%d", i);
+
 		chan->type = IIO_VOLTAGE;
 		chan->indexed = 1;
 		chan->channel = i;
-		chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
+		chan->datasheet_name = s;
+		chan->scan_type.sign = 'u';
+		chan->scan_type.realbits = 12;
+		chan->scan_type.storagebits = 32;
+		chan->scan_type.shift = 0;
 	}
 
 	indio_dev->channels = chan_array;
+	indio_dev->num_channels = channels;
+
+	size = (channels + 1) * sizeof(struct iio_map);
+	adc_dev->map = kzalloc(size, GFP_KERNEL);
+	if (adc_dev->map == NULL) {
+		kfree(chan_array);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < indio_dev->num_channels; i++) {
+		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
+		adc_dev->map[i].consumer_dev_name = "any";
+		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
+	}
+	adc_dev->map[i].adc_channel_label = NULL;
+	adc_dev->map[i].consumer_dev_name = NULL;
+	adc_dev->map[i].consumer_channel = NULL;
+
+	ret = iio_map_array_register(indio_dev, adc_dev->map);
+	if (ret != 0) {
+		kfree(adc_dev->map);
+		kfree(chan_array);
+		return -ENOMEM;
+	}
 
 	return indio_dev->num_channels;
 }
@@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev)
 
 	tiadc_step_config(adc_dev);
 
-	err = tiadc_channel_init(indio_dev, adc_dev->channels);
+	err = tiadc_channel_init(indio_dev, adc_dev);
 	if (err < 0)
 		goto err_free_device;
 
-- 
1.7.12

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Lars-Peter Clausen]

On 11/01/2012 04:24 PM, Pantelis Antoniou wrote:
> Add an IIO map interface that consumers can use.

Hi,

Looks like you overlooked the review comments I had inline last time. I've
put them in again, see below.

> 
> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++--------
>  1 file changed, 49 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 02a43c8..8595a90 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -20,8 +20,9 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/platform_device.h>
> -#include <linux/io.h>
>  #include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  #include <linux/platform_data/ti_am335x_adc.h>
> @@ -29,6 +30,8 @@
>  struct tiadc_device {
>  	struct ti_tscadc_dev *mfd_tscadc;
>  	int channels;
> +	char *buf;

buf doesn't seem to be used anywhere

> +	struct iio_map *map;
>  };
>  
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>  	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>  }
>  
> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> +static int tiadc_channel_init(struct iio_dev *indio_dev,
> +		struct tiadc_device *adc_dev)
>  {
>  	struct iio_chan_spec *chan_array;
> -	int i;
> -
> -	indio_dev->num_channels = channels;
> -	chan_array = kcalloc(indio_dev->num_channels,
> -			sizeof(struct iio_chan_spec), GFP_KERNEL);
> +	struct iio_chan_spec *chan;
> +	char *s;
> +	int i, len, size, ret;
> +	int channels = adc_dev->channels;
>  
> +	size = channels * (sizeof(struct iio_chan_spec) + 6);
> +	chan_array = kzalloc(size, GFP_KERNEL);
>  	if (chan_array == NULL)
>  		return -ENOMEM;
>  
> -	for (i = 0; i < (indio_dev->num_channels); i++) {
> -		struct iio_chan_spec *chan = chan_array + i;
> +	/* buffer space is after the array */
> +	s = (char *)(chan_array + channels);
> +	chan = chan_array;
> +	for (i = 0; i < channels; i++, chan++, s += len + 1) {
> +
> +		len = sprintf(s, "AIN%d", i);
> +
>  		chan->type = IIO_VOLTAGE;
>  		chan->indexed = 1;
>  		chan->channel = i;
> -		chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
> +		chan->datasheet_name = s;
> +		chan->scan_type.sign = 'u';
> +		chan->scan_type.realbits = 12;
> +		chan->scan_type.storagebits = 32;
> +		chan->scan_type.shift = 0;

The scan type assignment should go in a separate patch if possible.

>  	}
>  
>  	indio_dev->channels = chan_array;
> +	indio_dev->num_channels = channels;
> +
> +	size = (channels + 1) * sizeof(struct iio_map);
> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
> +	if (adc_dev->map == NULL) {
> +		kfree(chan_array);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < indio_dev->num_channels; i++) {
> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
> +		adc_dev->map[i].consumer_dev_name = "any";
> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
> +	}
> +	adc_dev->map[i].adc_channel_label = NULL;
> +	adc_dev->map[i].consumer_dev_name = NULL;
> +	adc_dev->map[i].consumer_channel = NULL;

The map should be passed in via platform data or similar. All the fields of
the map depend on the specific user, so you can't use a generic map. In fact
if we were able to use a generic map, we wouldn't need a map at all.

> +
> +	ret = iio_map_array_register(indio_dev, adc_dev->map);
> +	if (ret != 0) {
> +		kfree(adc_dev->map);
> +		kfree(chan_array);
> +		return -ENOMEM;
> +	}
>  
>  	return indio_dev->num_channels;
>  }
> @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev)
>  
>  	tiadc_step_config(adc_dev);
>  
> -	err = tiadc_channel_init(indio_dev, adc_dev->channels);
> +	err = tiadc_channel_init(indio_dev, adc_dev);
>  	if (err < 0)
>  		goto err_free_device;
>  

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Pantelis Antoniou]

On Oct 31, 2012, at 7:52 PM, Lars-Peter Clausen wrote:

> On 11/01/2012 04:24 PM, Pantelis Antoniou wrote:
>> Add an IIO map interface that consumers can use.
> 
> Hi,
> 
> Looks like you overlooked the review comments I had inline last time. I've
> put them in again, see below.
> 
>> 
>> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
>> ---
>> drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++--------
>> 1 file changed, 49 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 02a43c8..8595a90 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -20,8 +20,9 @@
>> #include <linux/slab.h>
>> #include <linux/interrupt.h>
>> #include <linux/platform_device.h>
>> -#include <linux/io.h>
>> #include <linux/iio/iio.h>
>> +#include <linux/iio/machine.h>
>> +#include <linux/iio/driver.h>
>> 
>> #include <linux/mfd/ti_am335x_tscadc.h>
>> #include <linux/platform_data/ti_am335x_adc.h>
>> @@ -29,6 +30,8 @@
>> struct tiadc_device {
>> 	struct ti_tscadc_dev *mfd_tscadc;
>> 	int channels;
>> +	char *buf;
> 
> buf doesn't seem to be used anywhere
> 

Duh

>> +	struct iio_map *map;
>> };
>> 
>> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>> @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>> 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>> }
>> 
>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>> +static int tiadc_channel_init(struct iio_dev *indio_dev,
>> +		struct tiadc_device *adc_dev)
>> {
>> 	struct iio_chan_spec *chan_array;
>> -	int i;
>> -
>> -	indio_dev->num_channels = channels;
>> -	chan_array = kcalloc(indio_dev->num_channels,
>> -			sizeof(struct iio_chan_spec), GFP_KERNEL);
>> +	struct iio_chan_spec *chan;
>> +	char *s;
>> +	int i, len, size, ret;
>> +	int channels = adc_dev->channels;
>> 
>> +	size = channels * (sizeof(struct iio_chan_spec) + 6);
>> +	chan_array = kzalloc(size, GFP_KERNEL);
>> 	if (chan_array == NULL)
>> 		return -ENOMEM;
>> 
>> -	for (i = 0; i < (indio_dev->num_channels); i++) {
>> -		struct iio_chan_spec *chan = chan_array + i;
>> +	/* buffer space is after the array */
>> +	s = (char *)(chan_array + channels);
>> +	chan = chan_array;
>> +	for (i = 0; i < channels; i++, chan++, s += len + 1) {
>> +
>> +		len = sprintf(s, "AIN%d", i);
>> +
>> 		chan->type = IIO_VOLTAGE;
>> 		chan->indexed = 1;
>> 		chan->channel = i;
>> -		chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
>> +		chan->datasheet_name = s;
>> +		chan->scan_type.sign = 'u';
>> +		chan->scan_type.realbits = 12;
>> +		chan->scan_type.storagebits = 32;
>> +		chan->scan_type.shift = 0;
> 
> The scan type assignment should go in a separate patch if possible.

ok.

> 
>> 	}
>> 
>> 	indio_dev->channels = chan_array;
>> +	indio_dev->num_channels = channels;
>> +
>> +	size = (channels + 1) * sizeof(struct iio_map);
>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>> +	if (adc_dev->map == NULL) {
>> +		kfree(chan_array);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>> +		adc_dev->map[i].consumer_dev_name = "any";
>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>> +	}
>> +	adc_dev->map[i].adc_channel_label = NULL;
>> +	adc_dev->map[i].consumer_dev_name = NULL;
>> +	adc_dev->map[i].consumer_channel = NULL;
> 
> The map should be passed in via platform data or similar. All the fields of
> the map depend on the specific user, so you can't use a generic map. In fact
> if we were able to use a generic map, we wouldn't need a map at all.

There's no platform data in the board I'm using. It's board-generic using
device tree only.

> 
>> +
>> +	ret = iio_map_array_register(indio_dev, adc_dev->map);
>> +	if (ret != 0) {
>> +		kfree(adc_dev->map);
>> +		kfree(chan_array);
>> +		return -ENOMEM;
>> +	}
>> 
>> 	return indio_dev->num_channels;
>> }
>> @@ -168,7 +206,7 @@ static int __devinit tiadc_probe(struct platform_device *pdev)
>> 
>> 	tiadc_step_config(adc_dev);
>> 
>> -	err = tiadc_channel_init(indio_dev, adc_dev->channels);
>> +	err = tiadc_channel_init(indio_dev, adc_dev);
>> 	if (err < 0)
>> 		goto err_free_device;
>> 
> 

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Lars-Peter Clausen]

On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
> [...]
>>> 	}
>>>
>>> 	indio_dev->channels = chan_array;
>>> +	indio_dev->num_channels = channels;
>>> +
>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>> +	if (adc_dev->map == NULL) {
>>> +		kfree(chan_array);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>> +	}
>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>> +	adc_dev->map[i].consumer_channel = NULL;
>>
>> The map should be passed in via platform data or similar. All the fields of
>> the map depend on the specific user, so you can't use a generic map. In fact
>> if we were able to use a generic map, we wouldn't need a map at all.
> 
> There's no platform data in the board I'm using. It's board-generic using
> device tree only.

That's the 'or similar' ;) Unfortunately we do not have a device tree
binding for IIO yet. But I think we should aim at a interface similar like
we have in other subsystems like the clk, regulator or dma framework.

- Lars

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Pantelis Antoniou]

On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:

> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
>> [...]
>>>> 	}
>>>> 
>>>> 	indio_dev->channels = chan_array;
>>>> +	indio_dev->num_channels = channels;
>>>> +
>>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>>> +	if (adc_dev->map == NULL) {
>>>> +		kfree(chan_array);
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +
>>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>>> +	}
>>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>>> +	adc_dev->map[i].consumer_channel = NULL;
>>> 
>>> The map should be passed in via platform data or similar. All the fields of
>>> the map depend on the specific user, so you can't use a generic map. In fact
>>> if we were able to use a generic map, we wouldn't need a map at all.
>> 
>> There's no platform data in the board I'm using. It's board-generic using
>> device tree only.
> 
> That's the 'or similar' ;) Unfortunately we do not have a device tree
> binding for IIO yet. But I think we should aim at a interface similar like
> we have in other subsystems like the clk, regulator or dma framework.
> 
> - Lars

So in the meantime no-one can use IIO ADC in any OF only platform.

In the meantime, this is pretty reasonable IMO. This is only for a specific 
board with known channel mappings.

I'm not out to fix IIO, I'm out to fix a single board.

Regards

-- Pantelis --
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/3] ti_adc: Update with IIO map interface

[Lars-Peter Clausen]

On 10/31/2012 07:12 PM, Pantelis Antoniou wrote:
> 
> On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:
> 
>> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
>>> [...]
>>>>> 	}
>>>>>
>>>>> 	indio_dev->channels = chan_array;
>>>>> +	indio_dev->num_channels = channels;
>>>>> +
>>>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>>>> +	if (adc_dev->map == NULL) {
>>>>> +		kfree(chan_array);
>>>>> +		return -ENOMEM;
>>>>> +	}
>>>>> +
>>>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>>>> +	}
>>>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>>>> +	adc_dev->map[i].consumer_channel = NULL;
>>>>
>>>> The map should be passed in via platform data or similar. All the fields of
>>>> the map depend on the specific user, so you can't use a generic map. In fact
>>>> if we were able to use a generic map, we wouldn't need a map at all.
>>>
>>> There's no platform data in the board I'm using. It's board-generic using
>>> device tree only.
>>
>> That's the 'or similar' ;) Unfortunately we do not have a device tree
>> binding for IIO yet. But I think we should aim at a interface similar like
>> we have in other subsystems like the clk, regulator or dma framework.
>>
>> - Lars
> 
> So in the meantime no-one can use IIO ADC in any OF only platform.

Yes, nobody can use it until somebody implements it. So far nobody needed
it, so that's why it hasn't been implemented yet. The whole in kernel
consumer API for IIO is still very young and only a very few drivers support
it yet.

> 
> In the meantime, this is pretty reasonable IMO. This is only for a specific 
> board with known channel mappings.

Unfortunately it is not. It is adding a device specific hack to a generic
driver and it is also completely misusing the API.

> 
> I'm not out to fix IIO, I'm out to fix a single board.
> 

It's not about fixing IIO, it's about extending IIO to be able to serve your
needs. See, the issue is if everybody would work around the lack of DT
bindings we'll never have DT bindings for IIO, so the right thing to do is
to implement them instead of working around the lack of.

- Lars

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Pantelis Antoniou]

On Oct 31, 2012, at 8:36 PM, Lars-Peter Clausen wrote:

> On 10/31/2012 07:12 PM, Pantelis Antoniou wrote:
>> 
>> On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:
>> 
>>> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
>>>> [...]
>>>>>> 	}
>>>>>> 
>>>>>> 	indio_dev->channels = chan_array;
>>>>>> +	indio_dev->num_channels = channels;
>>>>>> +
>>>>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>>>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>>>>> +	if (adc_dev->map == NULL) {
>>>>>> +		kfree(chan_array);
>>>>>> +		return -ENOMEM;
>>>>>> +	}
>>>>>> +
>>>>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>>>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>>>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>>>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>>>>> +	}
>>>>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>>>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>>>>> +	adc_dev->map[i].consumer_channel = NULL;
>>>>> 
>>>>> The map should be passed in via platform data or similar. All the fields of
>>>>> the map depend on the specific user, so you can't use a generic map. In fact
>>>>> if we were able to use a generic map, we wouldn't need a map at all.
>>>> 
>>>> There's no platform data in the board I'm using. It's board-generic using
>>>> device tree only.
>>> 
>>> That's the 'or similar' ;) Unfortunately we do not have a device tree
>>> binding for IIO yet. But I think we should aim at a interface similar like
>>> we have in other subsystems like the clk, regulator or dma framework.
>>> 
>>> - Lars
>> 
>> So in the meantime no-one can use IIO ADC in any OF only platform.
> 
> Yes, nobody can use it until somebody implements it. So far nobody needed
> it, so that's why it hasn't been implemented yet. The whole in kernel
> consumer API for IIO is still very young and only a very few drivers support
> it yet.
> 
>> 
>> In the meantime, this is pretty reasonable IMO. This is only for a specific 
>> board with known channel mappings.
> 
> Unfortunately it is not. It is adding a device specific hack to a generic
> driver and it is also completely misusing the API.
> 
>> 
>> I'm not out to fix IIO, I'm out to fix a single board.
>> 
> 
> It's not about fixing IIO, it's about extending IIO to be able to serve your
> needs. See, the issue is if everybody would work around the lack of DT
> bindings we'll never have DT bindings for IIO, so the right thing to do is
> to implement them instead of working around the lack of.
> 
> - Lars

OK, OK,

I see the point. It's just that I'm under the gun for more pressing matters ATM.
Can at least get a small write-up about how the bindings should look like?

There's absolutely nothing, not even a hint of one out there in the intertubes,
on how the channel mapping should look like.

Regards

-- Pantelis

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Lars-Peter Clausen]

On 10/31/2012 07:43 PM, Pantelis Antoniou wrote:
> 
> On Oct 31, 2012, at 8:36 PM, Lars-Peter Clausen wrote:
> 
>> On 10/31/2012 07:12 PM, Pantelis Antoniou wrote:
>>>
>>> On Oct 31, 2012, at 8:07 PM, Lars-Peter Clausen wrote:
>>>
>>>> On 10/31/2012 06:55 PM, Pantelis Antoniou wrote:
>>>>> [...]
>>>>>>> 	}
>>>>>>>
>>>>>>> 	indio_dev->channels = chan_array;
>>>>>>> +	indio_dev->num_channels = channels;
>>>>>>> +
>>>>>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>>>>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>>>>>> +	if (adc_dev->map == NULL) {
>>>>>>> +		kfree(chan_array);
>>>>>>> +		return -ENOMEM;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>>>>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>>>>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>>>>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>>>>>> +	}
>>>>>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>>>>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>>>>>> +	adc_dev->map[i].consumer_channel = NULL;
>>>>>>
>>>>>> The map should be passed in via platform data or similar. All the fields of
>>>>>> the map depend on the specific user, so you can't use a generic map. In fact
>>>>>> if we were able to use a generic map, we wouldn't need a map at all.
>>>>>
>>>>> There's no platform data in the board I'm using. It's board-generic using
>>>>> device tree only.
>>>>
>>>> That's the 'or similar' ;) Unfortunately we do not have a device tree
>>>> binding for IIO yet. But I think we should aim at a interface similar like
>>>> we have in other subsystems like the clk, regulator or dma framework.
>>>>
>>>> - Lars
>>>
>>> So in the meantime no-one can use IIO ADC in any OF only platform.
>>
>> Yes, nobody can use it until somebody implements it. So far nobody needed
>> it, so that's why it hasn't been implemented yet. The whole in kernel
>> consumer API for IIO is still very young and only a very few drivers support
>> it yet.
>>
>>>
>>> In the meantime, this is pretty reasonable IMO. This is only for a specific 
>>> board with known channel mappings.
>>
>> Unfortunately it is not. It is adding a device specific hack to a generic
>> driver and it is also completely misusing the API.
>>
>>>
>>> I'm not out to fix IIO, I'm out to fix a single board.
>>>
>>
>> It's not about fixing IIO, it's about extending IIO to be able to serve your
>> needs. See, the issue is if everybody would work around the lack of DT
>> bindings we'll never have DT bindings for IIO, so the right thing to do is
>> to implement them instead of working around the lack of.
>>
>> - Lars
> 
> OK, OK,
> 

ok :)

> I see the point. It's just that I'm under the gun for more pressing matters ATM.
> Can at least get a small write-up about how the bindings should look like?
> 
> There's absolutely nothing, not even a hint of one out there in the intertubes,
> on how the channel mapping should look like.

Again, that's because nobody probably has given this much though yet. As I said
the in-kernel consumer framework is still young and so far only a tiny fraction
of the drivers support it. If you want to I can try to give this some though
and come up with a small proof of concept, but this would have to wait until
next week, since I have a few other things on my desk as well.

I think a good start might be to take a closer look at the clk dt bindings
(Documentation/devicetree/bindings/clock/clock-bindings.txt).

- Lars

Re: [PATCH 1/3] ti_adc: Update with IIO map interface

[Porter, Matt]

On Oct 31, 2012, at 1:55 PM, Pantelis Antoniou wrote:

> 
> On Oct 31, 2012, at 7:52 PM, Lars-Peter Clausen wrote:
> 
>> On 11/01/2012 04:24 PM, Pantelis Antoniou wrote:
>>> Add an IIO map interface that consumers can use.
>> 
>> Hi,
>> 
>> Looks like you overlooked the review comments I had inline last time. I've
>> put them in again, see below.
>> 
>>> 
>>> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>>> ---
>>> drivers/iio/adc/ti_am335x_adc.c | 60 +++++++++++++++++++++++++++++++++--------
>>> 1 file changed, 49 insertions(+), 11 deletions(-)
>>> 
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index 02a43c8..8595a90 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>>> @@ -20,8 +20,9 @@
>>> #include <linux/slab.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/platform_device.h>
>>> -#include <linux/io.h>
>>> #include <linux/iio/iio.h>
>>> +#include <linux/iio/machine.h>
>>> +#include <linux/iio/driver.h>
>>> 
>>> #include <linux/mfd/ti_am335x_tscadc.h>
>>> #include <linux/platform_data/ti_am335x_adc.h>
>>> @@ -29,6 +30,8 @@
>>> struct tiadc_device {
>>> 	struct ti_tscadc_dev *mfd_tscadc;
>>> 	int channels;
>>> +	char *buf;
>> 
>> buf doesn't seem to be used anywhere
>> 
> 
> Duh
> 
>>> +	struct iio_map *map;
>>> };
>>> 
>>> static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>>> @@ -72,27 +75,62 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
>>> 	tiadc_writel(adc_dev, REG_SE, STPENB_STEPENB);
>>> }
>>> 
>>> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>>> +static int tiadc_channel_init(struct iio_dev *indio_dev,
>>> +		struct tiadc_device *adc_dev)
>>> {
>>> 	struct iio_chan_spec *chan_array;
>>> -	int i;
>>> -
>>> -	indio_dev->num_channels = channels;
>>> -	chan_array = kcalloc(indio_dev->num_channels,
>>> -			sizeof(struct iio_chan_spec), GFP_KERNEL);
>>> +	struct iio_chan_spec *chan;
>>> +	char *s;
>>> +	int i, len, size, ret;
>>> +	int channels = adc_dev->channels;
>>> 
>>> +	size = channels * (sizeof(struct iio_chan_spec) + 6);
>>> +	chan_array = kzalloc(size, GFP_KERNEL);
>>> 	if (chan_array == NULL)
>>> 		return -ENOMEM;
>>> 
>>> -	for (i = 0; i < (indio_dev->num_channels); i++) {
>>> -		struct iio_chan_spec *chan = chan_array + i;
>>> +	/* buffer space is after the array */
>>> +	s = (char *)(chan_array + channels);
>>> +	chan = chan_array;
>>> +	for (i = 0; i < channels; i++, chan++, s += len + 1) {
>>> +
>>> +		len = sprintf(s, "AIN%d", i);
>>> +
>>> 		chan->type = IIO_VOLTAGE;
>>> 		chan->indexed = 1;
>>> 		chan->channel = i;
>>> -		chan->info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT;
>>> +		chan->datasheet_name = s;
>>> +		chan->scan_type.sign = 'u';
>>> +		chan->scan_type.realbits = 12;
>>> +		chan->scan_type.storagebits = 32;
>>> +		chan->scan_type.shift = 0;
>> 
>> The scan type assignment should go in a separate patch if possible.
> 
> ok.
> 
>> 
>>> 	}
>>> 
>>> 	indio_dev->channels = chan_array;
>>> +	indio_dev->num_channels = channels;
>>> +
>>> +	size = (channels + 1) * sizeof(struct iio_map);
>>> +	adc_dev->map = kzalloc(size, GFP_KERNEL);
>>> +	if (adc_dev->map == NULL) {
>>> +		kfree(chan_array);
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	for (i = 0; i < indio_dev->num_channels; i++) {
>>> +		adc_dev->map[i].adc_channel_label = chan_array[i].datasheet_name;
>>> +		adc_dev->map[i].consumer_dev_name = "any";
>>> +		adc_dev->map[i].consumer_channel = chan_array[i].datasheet_name;
>>> +	}
>>> +	adc_dev->map[i].adc_channel_label = NULL;
>>> +	adc_dev->map[i].consumer_dev_name = NULL;
>>> +	adc_dev->map[i].consumer_channel = NULL;
>> 
>> The map should be passed in via platform data or similar. All the fields of
>> the map depend on the specific user, so you can't use a generic map. In fact
>> if we were able to use a generic map, we wouldn't need a map at all.
> 
> There's no platform data in the board I'm using. It's board-generic using
> device tree only.

How about a DT binding for the map?

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

[PATCH 2/3] ti_tscadc: Match mfd sub devices to regmap interface

[Pantelis Antoniou]

The MFD parent device now uses a regmap, instead of direct
memory access. Use the same method in the sub devices to avoid
nasty surprises.

Please not that this driver can't really deal with the case of the regmap
call failing in anyway. So that's why there's no error handling.

I think it's best to patch this up, until the driver maintainers deal
with this properly.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/iio/adc/ti_am335x_adc.c           | 10 ++++++++--
 drivers/input/touchscreen/ti_am335x_tsc.c |  9 +++++++--
 2 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 8595a90..44806f9 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -23,7 +23,9 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/machine.h>
 #include <linux/iio/driver.h>
+#include <linux/regmap.h>
 
+#include <linux/io.h>
 #include <linux/mfd/ti_am335x_tscadc.h>
 #include <linux/platform_data/ti_am335x_adc.h>
 
@@ -36,13 +38,17 @@ struct tiadc_device {
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
 {
-	return readl(adc->mfd_tscadc->tscadc_base + reg);
+	unsigned int val;
+
+	val = (unsigned int)-1;
+	regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val);
+	return val;
 }
 
 static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
 					unsigned int val)
 {
-	writel(val, adc->mfd_tscadc->tscadc_base + reg);
+	regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
 static void tiadc_step_config(struct tiadc_device *adc_dev)
diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
index 7a26810..5723957 100644
--- a/drivers/input/touchscreen/ti_am335x_tsc.c
+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
@@ -26,6 +26,7 @@
 #include <linux/io.h>
 #include <linux/input/ti_am335x_tsc.h>
 #include <linux/delay.h>
+#include <linux/regmap.h>
 
 #include <linux/mfd/ti_am335x_tscadc.h>
 
@@ -64,13 +65,17 @@ struct titsc {
 
 static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
 {
-	return readl(ts->mfd_tscadc->tscadc_base + reg);
+	unsigned int val;
+
+	val = (unsigned int)-1;
+	regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val);
+	return val;
 }
 
 static void titsc_writel(struct titsc *tsc, unsigned int reg,
 					unsigned int val)
 {
-	writel(val, tsc->mfd_tscadc->tscadc_base + reg);
+	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
 }
 
 /*
-- 
1.7.12

Re: [PATCH 2/3] ti_tscadc: Match mfd sub devices to regmap interface

[Jonathan Cameron]

On 11/01/2012 03:24 PM, Pantelis Antoniou wrote:
> The MFD parent device now uses a regmap, instead of direct
> memory access. Use the same method in the sub devices to avoid
> nasty surprises.
> 
> Please not that this driver can't really deal with the case of the regmap
> call failing in anyway. So that's why there's no error handling.
> 
> I think it's best to patch this up, until the driver maintainers deal
> with this properly.
This should be split in two as it's touching two different drivers in
different subsystems and may merge through them.

> 
> Signed-off-by: Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> ---
>  drivers/iio/adc/ti_am335x_adc.c           | 10 ++++++++--
>  drivers/input/touchscreen/ti_am335x_tsc.c |  9 +++++++--
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 8595a90..44806f9 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -23,7 +23,9 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/machine.h>
>  #include <linux/iio/driver.h>
> +#include <linux/regmap.h>
>  
> +#include <linux/io.h>
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  #include <linux/platform_data/ti_am335x_adc.h>
>  
> @@ -36,13 +38,17 @@ struct tiadc_device {
>  
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>  {
> -	return readl(adc->mfd_tscadc->tscadc_base + reg);
> +	unsigned int val;
> +
> +	val = (unsigned int)-1;
> +	regmap_read(adc->mfd_tscadc->regmap_tscadc, reg, &val);
> +	return val;
>  }
>  
>  static void tiadc_writel(struct tiadc_device *adc, unsigned int reg,
>  					unsigned int val)
>  {
> -	writel(val, adc->mfd_tscadc->tscadc_base + reg);
> +	regmap_write(adc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
>  static void tiadc_step_config(struct tiadc_device *adc_dev)
> diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> index 7a26810..5723957 100644
> --- a/drivers/input/touchscreen/ti_am335x_tsc.c
> +++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> @@ -26,6 +26,7 @@
>  #include <linux/io.h>
>  #include <linux/input/ti_am335x_tsc.h>
>  #include <linux/delay.h>
> +#include <linux/regmap.h>
>  
>  #include <linux/mfd/ti_am335x_tscadc.h>
>  
> @@ -64,13 +65,17 @@ struct titsc {
>  
>  static unsigned int titsc_readl(struct titsc *ts, unsigned int reg)
>  {
> -	return readl(ts->mfd_tscadc->tscadc_base + reg);
> +	unsigned int val;
> +
> +	val = (unsigned int)-1;
> +	regmap_read(ts->mfd_tscadc->regmap_tscadc, reg, &val);
> +	return val;
>  }
>  
>  static void titsc_writel(struct titsc *tsc, unsigned int reg,
>  					unsigned int val)
>  {
> -	writel(val, tsc->mfd_tscadc->tscadc_base + reg);
> +	regmap_write(tsc->mfd_tscadc->regmap_tscadc, reg, val);
>  }
>  
>  /*
> 

[PATCH 3/3] ti_tscadc: Deal with non activation of all the devices.

[Pantelis Antoniou]

It's common not for both the touchscreen & adc to be activated
at the same time. Deal with this case.

Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com>
---
 drivers/mfd/ti_am335x_tscadc.c       | 34 +++++++++++++++++++++++-----------
 include/linux/mfd/ti_am335x_tscadc.h |  8 +++-----
 2 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index e947dd8..97eb4f7 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -68,7 +68,7 @@ static	int __devinit ti_tscadc_probe(struct platform_device *pdev)
 	int			irq;
 	int			err, ctrl;
 	int			clk_value, clock_rate;
-	int			tsc_wires, adc_channels = 0, total_channels;
+	int			tsc_wires = 0, adc_channels = 0, total_channels;
 
 	if (!pdata) {
 		dev_err(&pdev->dev, "Could not find platform data\n");
@@ -78,7 +78,9 @@ static	int __devinit ti_tscadc_probe(struct platform_device *pdev)
 	if (pdata->adc_init)
 		adc_channels = pdata->adc_init->adc_channels;
 
-	tsc_wires = pdata->tsc_init->wires;
+	if (pdata->tsc_init)
+		tsc_wires = pdata->tsc_init->wires;
+
 	total_channels = tsc_wires + adc_channels;
 
 	if (total_channels > 8) {
@@ -176,20 +178,30 @@ static	int __devinit ti_tscadc_probe(struct platform_device *pdev)
 	ctrl |= CNTRLREG_TSCSSENB;
 	tscadc_writel(tscadc, REG_CTRL, ctrl);
 
+	tscadc->used_cells = 0;
+	tscadc->tsc_cell = -1;
+	tscadc->adc_cell = -1;
+
 	/* TSC Cell */
-	cell = &tscadc->cells[TSC_CELL];
-	cell->name = "tsc";
-	cell->platform_data = tscadc;
-	cell->pdata_size = sizeof(*tscadc);
+	if (tsc_wires > 0) {
+		tscadc->tsc_cell = tscadc->used_cells;
+		cell = &tscadc->cells[tscadc->used_cells++];
+		cell->name = "tsc";
+		cell->platform_data = tscadc;
+		cell->pdata_size = sizeof(*tscadc);
+	}
 
 	/* ADC Cell */
-	cell = &tscadc->cells[ADC_CELL];
-	cell->name = "tiadc";
-	cell->platform_data = tscadc;
-	cell->pdata_size = sizeof(*tscadc);
+	if (adc_channels > 0) {
+		tscadc->adc_cell = tscadc->used_cells;
+		cell = &tscadc->cells[tscadc->used_cells++];
+		cell->name = "tiadc";
+		cell->platform_data = tscadc;
+		cell->pdata_size = sizeof(*tscadc);
+	}
 
 	err = mfd_add_devices(&pdev->dev, pdev->id, tscadc->cells,
-			TSCADC_CELLS, NULL, 0, NULL);
+			tscadc->used_cells, NULL, 0, NULL);
 	if (err < 0)
 		goto err_disable_clk;
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index 9624fea..50a245f 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -128,11 +128,6 @@
 
 #define TSCADC_CELLS		2
 
-enum tscadc_cells {
-	TSC_CELL,
-	ADC_CELL,
-};
-
 struct mfd_tscadc_board {
 	struct tsc_data *tsc_init;
 	struct adc_data *adc_init;
@@ -143,6 +138,9 @@ struct ti_tscadc_dev {
 	struct regmap *regmap_tscadc;
 	void __iomem *tscadc_base;
 	int irq;
+	int used_cells;	/* 0-2 */
+	int tsc_cell;	/* -1 if not used */
+	int adc_cell;	/* -1 if not used */
 	struct mfd_cell cells[TSCADC_CELLS];
 
 	/* tsc device */
-- 
1.7.12