[PATCH 0/3] staging:iio:adis16130 bits and bobs.

[PATCH 0/3] staging:iio:adis16130 bits and bobs.

[Jonathan Cameron]

Hi All,

Had a few bored moments so thought I'd take a quick look at this driver.
The read function is rather odd to say the least, Lars/Michael could one of
you take a quick look at this.  Right now it does an spi sync that I think
should read the data and follows it with an additional read.  I can't
immediately see what the read is for.

Also note the introduction of IIO_INT_PLUS_PICO which is going to be common
with 24 bit plus devices.  For now I've ignored the write case as this driver
doesn't support it, but it will be needed if for example 24/16 bit options
are both supported for this driver.

Note to my mind this device is far enough away from the other adis parts
in interface to justify it's own driver.

Also could someone check my scale/offset calcs are right. It's Sunday
morning and I'm not feeling all that awake ;)

Thanks,

Jonathan 

Jonathan Cameron (3):
  iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.
  staging:iio:gyro:adis16130 drop unused list.h header.
  staging:iio:gyro:adis16130 add offset and scale info mask elements.

 drivers/iio/industrialio-core.c           |  5 +++
 drivers/staging/iio/gyro/adis16130_core.c | 58 +++++++++++++++++++++++++------
 include/linux/iio/types.h                 |  1 +
 3 files changed, 53 insertions(+), 11 deletions(-)

-- 
1.8.2.1

[PATCH 1/3] iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.

[Jonathan Cameron]

For now not added to the write_raw handling as no usecase yet exists
and it would involve a more substantial reworking of the string
processing code to handle it cleanly.

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/iio/industrialio-core.c | 5 +++++
 include/linux/iio/types.h       | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index e145931..40ef037 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -393,6 +393,11 @@ static ssize_t iio_read_channel_info(struct device *dev,
 			return sprintf(buf, "-%d.%09u\n", val, -val2);
 		else
 			return sprintf(buf, "%d.%09u\n", val, val2);
+	case IIO_VAL_INT_PLUS_PICO:
+		if (val2 < 0)
+			return sprintf(buf, "-%d.%012u\n", val, -val2);
+		else
+			return sprintf(buf, "-%d.%012u\n", val, val2);
 	case IIO_VAL_FRACTIONAL:
 		tmp = div_s64((s64)val * 1000000000LL, val2);
 		val2 = do_div(tmp, 1000000000LL);
diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
index 88bf0f0..27e78a6 100644
--- a/include/linux/iio/types.h
+++ b/include/linux/iio/types.h
@@ -58,6 +58,7 @@ enum iio_modifier {
 #define IIO_VAL_INT_PLUS_MICRO 2
 #define IIO_VAL_INT_PLUS_NANO 3
 #define IIO_VAL_INT_PLUS_MICRO_DB 4
+#define IIO_VAL_INT_PLUS_PICO 5
 #define IIO_VAL_FRACTIONAL 10
 #define IIO_VAL_FRACTIONAL_LOG2 11
 
-- 
1.8.2.1

[PATCH 2/3] staging:iio:gyro:adis16130 drop unused list.h header.

[Jonathan Cameron]

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/gyro/adis16130_core.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/iio/gyro/adis16130_core.c b/drivers/staging/iio/gyro/adis16130_core.c
index 531b803..628671a 100644
--- a/drivers/staging/iio/gyro/adis16130_core.c
+++ b/drivers/staging/iio/gyro/adis16130_core.c
@@ -13,7 +13,6 @@
 #include <linux/spi/spi.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
-#include <linux/list.h>
 #include <linux/module.h>
 
 #include <linux/iio/iio.h>
-- 
1.8.2.1

[PATCH 3/3] staging:iio:gyro:adis16130 add offset and scale info mask elements.

[Jonathan Cameron]

Signed-off-by: Jonathan Cameron <jic23@kernel.org>
---
 drivers/staging/iio/gyro/adis16130_core.c | 57 +++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/gyro/adis16130_core.c b/drivers/staging/iio/gyro/adis16130_core.c
index 628671a..4c44afc 100644
--- a/drivers/staging/iio/gyro/adis16130_core.c
+++ b/drivers/staging/iio/gyro/adis16130_core.c
@@ -84,14 +84,47 @@ static int adis16130_read_raw(struct iio_dev *indio_dev,
 	int ret;
 	u32 temp;
 
-	/* Take the iio_dev status lock */
-	mutex_lock(&indio_dev->mlock);
-	ret =  adis16130_spi_read(indio_dev, chan->address, &temp);
-	mutex_unlock(&indio_dev->mlock);
-	if (ret)
-		return ret;
-	*val = temp;
-	return IIO_VAL_INT;
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		/* Take the iio_dev status lock */
+		mutex_lock(&indio_dev->mlock);
+		ret =  adis16130_spi_read(indio_dev, chan->address, &temp);
+		mutex_unlock(&indio_dev->mlock);
+		if (ret)
+			return ret;
+		*val = temp;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			/* -2^23 */
+			*val = -8388608;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			/* -2^23 + 25*14093 */
+			*val = -8036233;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			/* 2pi/(360*23488) */
+			*val = 0;
+			*val2 = 743072;
+			return IIO_VAL_INT_PLUS_PICO;
+		case IIO_TEMP:
+			/* 1/(14093*1000) */
+			*val = 0;
+			*val2 = 70957;
+			return IIO_VAL_INT_PLUS_PICO;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
 }
 
 static const struct iio_chan_spec adis16130_channels[] = {
@@ -99,13 +132,17 @@ static const struct iio_chan_spec adis16130_channels[] = {
 		.type = IIO_ANGL_VEL,
 		.modified = 1,
 		.channel2 = IIO_MOD_Z,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_OFFSET) |
+		BIT(IIO_CHAN_INFO_SCALE),
 		.address = ADIS16130_RATEDATA,
 	}, {
 		.type = IIO_TEMP,
 		.indexed = 1,
 		.channel = 0,
-		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_OFFSET) |
+		BIT(IIO_CHAN_INFO_SCALE),
 		.address = ADIS16130_TEMPDATA,
 	}
 };
-- 
1.8.2.1

Re: [PATCH 0/3] staging:iio:adis16130 bits and bobs.

[Lars-Peter Clausen]

On 04/14/2013 01:23 PM, Jonathan Cameron wrote:
> Hi All,
> 
> Had a few bored moments so thought I'd take a quick look at this driver.
> The read function is rather odd to say the least, Lars/Michael could one of
> you take a quick look at this.  Right now it does an spi sync that I think
> should read the data and follows it with an additional read.  I can't
> immediately see what the read is for.

Yea I noticed that too, I think that read was introduced by accident during
some refactoring. See
https://github.com/lclausen-adi/linux-2.6/commit/1889f3a5d291ad57a5faed83652c465de29d740f

Unfortunately I couldn't get my hands on a adis16030 board yet to test those
changes otherwise I'd already submitted them.

> 
> Also note the introduction of IIO_INT_PLUS_PICO which is going to be common
> with 24 bit plus devices.  For now I've ignored the write case as this driver
> doesn't support it, but it will be needed if for example 24/16 bit options
> are both supported for this driver.
> 
> Note to my mind this device is far enough away from the other adis parts
> in interface to justify it's own driver.

Yes.

> 
> Also could someone check my scale/offset calcs are right. It's Sunday
> morning and I'm not feeling all that awake ;)
> 

For temp scale you seem to be off by a factor of 1000000 (well or maybe
I'm), otherwise I got the same.

> Thanks,
> 
> Jonathan 
> 
> Jonathan Cameron (3):
>   iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.
>   staging:iio:gyro:adis16130 drop unused list.h header.
>   staging:iio:gyro:adis16130 add offset and scale info mask elements.
> 
>  drivers/iio/industrialio-core.c           |  5 +++
>  drivers/staging/iio/gyro/adis16130_core.c | 58 +++++++++++++++++++++++++------
>  include/linux/iio/types.h                 |  1 +
>  3 files changed, 53 insertions(+), 11 deletions(-)
> 

Re: [PATCH 1/3] iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.

[Lars-Peter Clausen]

On 04/14/2013 01:23 PM, Jonathan Cameron wrote:
> For now not added to the write_raw handling as no usecase yet exists
> and it would involve a more substantial reworking of the string
> processing code to handle it cleanly.
> 
> Signed-off-by: Jonathan Cameron <jic23@kernel.org>
> ---
>  drivers/iio/industrialio-core.c | 5 +++++
>  include/linux/iio/types.h       | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index e145931..40ef037 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -393,6 +393,11 @@ static ssize_t iio_read_channel_info(struct device *dev,
>  			return sprintf(buf, "-%d.%09u\n", val, -val2);
>  		else
>  			return sprintf(buf, "%d.%09u\n", val, val2);
> +	case IIO_VAL_INT_PLUS_PICO:
> +		if (val2 < 0)
> +			return sprintf(buf, "-%d.%012u\n", val, -val2);
> +		else
> +			return sprintf(buf, "-%d.%012u\n", val, val2);

This one shouldn't have the minus sign.

>  	case IIO_VAL_FRACTIONAL:
>  		tmp = div_s64((s64)val * 1000000000LL, val2);
>  		val2 = do_div(tmp, 1000000000LL);
> diff --git a/include/linux/iio/types.h b/include/linux/iio/types.h
> index 88bf0f0..27e78a6 100644
> --- a/include/linux/iio/types.h
> +++ b/include/linux/iio/types.h
> @@ -58,6 +58,7 @@ enum iio_modifier {
>  #define IIO_VAL_INT_PLUS_MICRO 2
>  #define IIO_VAL_INT_PLUS_NANO 3
>  #define IIO_VAL_INT_PLUS_MICRO_DB 4
> +#define IIO_VAL_INT_PLUS_PICO 5
>  #define IIO_VAL_FRACTIONAL 10
>  #define IIO_VAL_FRACTIONAL_LOG2 11
>  

Re: [PATCH 0/3] staging:iio:adis16130 bits and bobs.

[Jonathan Cameron]

On 04/14/2013 01:20 PM, Lars-Peter Clausen wrote:
> On 04/14/2013 01:23 PM, Jonathan Cameron wrote:
>> Hi All,
>>
>> Had a few bored moments so thought I'd take a quick look at this driver.
>> The read function is rather odd to say the least, Lars/Michael could one of
>> you take a quick look at this.  Right now it does an spi sync that I think
>> should read the data and follows it with an additional read.  I can't
>> immediately see what the read is for.
> 
> Yea I noticed that too, I think that read was introduced by accident during
> some refactoring. See
> https://github.com/lclausen-adi/linux-2.6/commit/1889f3a5d291ad57a5faed83652c465de29d740f
oops.
> 
> Unfortunately I couldn't get my hands on a adis16030 board yet to test those
> changes otherwise I'd already submitted them.
I did wonder why this one had slipped through your net.

Even untested, I'd be tempted to apply your patch.  What is there is pretty
obviously garbage whereas yours is probably right!  I guess it depends on time
scales for getting the part.

> 
>>
>> Also note the introduction of IIO_INT_PLUS_PICO which is going to be common
>> with 24 bit plus devices.  For now I've ignored the write case as this driver
>> doesn't support it, but it will be needed if for example 24/16 bit options
>> are both supported for this driver.
>>
>> Note to my mind this device is far enough away from the other adis parts
>> in interface to justify it's own driver.
> 
> Yes.
> 
>>
>> Also could someone check my scale/offset calcs are right. It's Sunday
>> morning and I'm not feeling all that awake ;)
>>
> 
> For temp scale you seem to be off by a factor of 1000000 (well or maybe
> I'm), otherwise I got the same.
oops :)  Divide vs multiply...

Will post a v2 in a few mins for what we had here and let you handle
the interesting read as you like.
> 
>> Thanks,
>>
>> Jonathan 
>>
>> Jonathan Cameron (3):
>>   iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.
>>   staging:iio:gyro:adis16130 drop unused list.h header.
>>   staging:iio:gyro:adis16130 add offset and scale info mask elements.
>>
>>  drivers/iio/industrialio-core.c           |  5 +++
>>  drivers/staging/iio/gyro/adis16130_core.c | 58 +++++++++++++++++++++++++------
>>  include/linux/iio/types.h                 |  1 +
>>  3 files changed, 53 insertions(+), 11 deletions(-)
>>
> 

Re: [PATCH 0/3] staging:iio:adis16130 bits and bobs.

[Jonathan Cameron]

On 04/14/2013 06:53 PM, Jonathan Cameron wrote:
> On 04/14/2013 01:20 PM, Lars-Peter Clausen wrote:
>> On 04/14/2013 01:23 PM, Jonathan Cameron wrote:
>>> Hi All,
>>>
>>> Had a few bored moments so thought I'd take a quick look at this driver.
>>> The read function is rather odd to say the least, Lars/Michael could one of
>>> you take a quick look at this.  Right now it does an spi sync that I think
>>> should read the data and follows it with an additional read.  I can't
>>> immediately see what the read is for.
>>
>> Yea I noticed that too, I think that read was introduced by accident during
>> some refactoring. See
>> https://github.com/lclausen-adi/linux-2.6/commit/1889f3a5d291ad57a5faed83652c465de29d740f
> oops.
>>
>> Unfortunately I couldn't get my hands on a adis16030 board yet to test those
>> changes otherwise I'd already submitted them.
> I did wonder why this one had slipped through your net.
> 
> Even untested, I'd be tempted to apply your patch.  What is there is pretty
> obviously garbage whereas yours is probably right!  I guess it depends on time
> scales for getting the part.
> 
>>
>>>
>>> Also note the introduction of IIO_INT_PLUS_PICO which is going to be common
>>> with 24 bit plus devices.  For now I've ignored the write case as this driver
>>> doesn't support it, but it will be needed if for example 24/16 bit options
>>> are both supported for this driver.
>>>
>>> Note to my mind this device is far enough away from the other adis parts
>>> in interface to justify it's own driver.
>>
>> Yes.
>>
>>>
>>> Also could someone check my scale/offset calcs are right. It's Sunday
>>> morning and I'm not feeling all that awake ;)
>>>
>>
>> For temp scale you seem to be off by a factor of 1000000 (well or maybe
>> I'm), otherwise I got the same.
> oops :)  Divide vs multiply...
> 
> Will post a v2 in a few mins for what we had here and let you handle
> the interesting read as you like.

Actually seeing as you have similar patches queued up in your tree I'll
leave it all for you and take the lazy way out ;)

*continues looking about in the tree you referenced to see what other nuggets
you are working on :)*

Jonathan
>>
>>> Thanks,
>>>
>>> Jonathan 
>>>
>>> Jonathan Cameron (3):
>>>   iio:Add an IIO_VAL_INTO_PLUS_PICO return type for read_raw callbacks.
>>>   staging:iio:gyro:adis16130 drop unused list.h header.
>>>   staging:iio:gyro:adis16130 add offset and scale info mask elements.
>>>
>>>  drivers/iio/industrialio-core.c           |  5 +++
>>>  drivers/staging/iio/gyro/adis16130_core.c | 58 +++++++++++++++++++++++++------
>>>  include/linux/iio/types.h                 |  1 +
>>>  3 files changed, 53 insertions(+), 11 deletions(-)
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/3] staging:iio:adis16130 bits and bobs.

[Lars-Peter Clausen]

On 04/14/2013 07:56 PM, Jonathan Cameron wrote:
> On 04/14/2013 06:53 PM, Jonathan Cameron wrote:
>> On 04/14/2013 01:20 PM, Lars-Peter Clausen wrote:
>>> On 04/14/2013 01:23 PM, Jonathan Cameron wrote:
>>>> Hi All,
>>>>
>>>> Had a few bored moments so thought I'd take a quick look at this driver.
>>>> The read function is rather odd to say the least, Lars/Michael could one of
>>>> you take a quick look at this.  Right now it does an spi sync that I think
>>>> should read the data and follows it with an additional read.  I can't
>>>> immediately see what the read is for.
>>>
>>> Yea I noticed that too, I think that read was introduced by accident during
>>> some refactoring. See
>>> https://github.com/lclausen-adi/linux-2.6/commit/1889f3a5d291ad57a5faed83652c465de29d740f
>> oops.
>>>
>>> Unfortunately I couldn't get my hands on a adis16030 board yet to test those
>>> changes otherwise I'd already submitted them.
>> I did wonder why this one had slipped through your net.
>>
>> Even untested, I'd be tempted to apply your patch.  What is there is pretty
>> obviously garbage whereas yours is probably right!  I guess it depends on time
>> scales for getting the part.
>>
>>>
>>>>
>>>> Also note the introduction of IIO_INT_PLUS_PICO which is going to be common
>>>> with 24 bit plus devices.  For now I've ignored the write case as this driver
>>>> doesn't support it, but it will be needed if for example 24/16 bit options
>>>> are both supported for this driver.
>>>>
>>>> Note to my mind this device is far enough away from the other adis parts
>>>> in interface to justify it's own driver.
>>>
>>> Yes.
>>>
>>>>
>>>> Also could someone check my scale/offset calcs are right. It's Sunday
>>>> morning and I'm not feeling all that awake ;)
>>>>
>>>
>>> For temp scale you seem to be off by a factor of 1000000 (well or maybe
>>> I'm), otherwise I got the same.
>> oops :)  Divide vs multiply...
>>
>> Will post a v2 in a few mins for what we had here and let you handle
>> the interesting read as you like.
> 
> Actually seeing as you have similar patches queued up in your tree I'll
> leave it all for you and take the lazy way out ;)
> 
> *continues looking about in the tree you referenced to see what other nuggets
> you are working on :)*

Nothing really in there, it's more of a junkyard ;)

- Lars