[PATCH 0/2] staging:iio:adc:as1531 driver port from hwmon driver.

[PATCH 0/2] staging:iio:adc:as1531 driver port from hwmon driver.

[Jonathan Cameron]

Dear All,

This is a quick initial port of the as1531 driver Fabien submitted
to the lm-sensors mailing list.

Based on top of a large series I just sent to Greg KH and posted to
linux-iio. That has a few other dependencies, so might be easier
to just to wait for them all to be in a standard tree (linux-next
or mainline).

Dropped from hwmon driver - max and min values.

Stuff to add at some point (i.e. when needed)

1) Differential channels - trivial to do, just add a few more entries to
the channels array and some signed bits in read_raw

2) Buffered read support - straight forward case where all channels have
to be explicitly requested anyway.

3) Other devices trivially added:
as1530 is directly compatible.
as1532/1533 look like 4 channel equivalents (add iio_chan_spec arrays).

The other similarly numbered parts are different enough to need their
own drivers.

Fabien, as more of the code in the first patch is yours than mine,
I've put you as author of that one.

The second is mostly stuff you'd have gotten in review to the hwmon
driver anyway.


Fabien Marteau (1):
  staging:iio:adc: as1531 driver initial conversion from hwmon
    submission.

Jonathan Cameron (1):
  staging:iio:adc:as1351 general cleanup and conversion to standard
    functions.

 drivers/staging/iio/adc/Kconfig  |   10 ++
 drivers/staging/iio/adc/Makefile |    2 +
 drivers/staging/iio/adc/as1531.c |  168 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 180 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/adc/as1531.c

-- 
1.7.3.4

[PATCH V2 0/2] staging:iio:adc:as1531 driver port from hwmon driver.

[Jonathan Cameron]

V2: Fix the various issues Guenter found.

Original message -

Dear All,

This is a quick initial port of the as1531 driver Fabien submitted
to the lm-sensors mailing list.

Based on top of a large series I just sent to Greg KH and posted to
linux-iio. That has a few other dependencies, so might be easier
to just to wait for them all to be in a standard tree (linux-next
or mainline).

Dropped from hwmon driver - max and min values.

Stuff to add at some point (i.e. when needed)

1) Differential channels - trivial to do, just add a few more entries to
the channels array and some signed bits in read_raw

2) Buffered read support - straight forward case where all channels have
to be explicitly requested anyway.

3) Other devices trivially added:
as1530 is directly compatible.
as1532/1533 look like 4 channel equivalents (add iio_chan_spec arrays).

The other similarly numbered parts are different enough to need their
own drivers.

Fabien, as more of the code in the first patch is yours than mine,
I've put you as author of that one.

The second is mostly stuff you'd have gotten in review to the hwmon
driver anyway.

Jonathan

Fabien Marteau (1):
  staging:iio:adc: as1531 driver initial conversion from hwmon
    submission.

Jonathan Cameron (1):
  staging:iio:adc:as1351 general cleanup and conversion to standard
    functions.

 drivers/staging/iio/adc/Kconfig  |   10 +++
 drivers/staging/iio/adc/Makefile |    2 +
 drivers/staging/iio/adc/as1531.c |  166 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/adc/as1531.c

-- 
1.7.3.4

Re: [PATCH V2 0/2] staging:iio:adc:as1531 driver port from hwmon driver.

[Fabien Marteau]

Dear Jonathan,

Thanks for all the work you done to re-write this driver under iio. I
will try to find time this week to test this new version under my platform.

It was just a little tricky hwmon driver, it's really nice that you
spend time to write a good driver.

Fabien M

On 18/05/2011 18:16, Jonathan Cameron wrote:
> V2: Fix the various issues Guenter found.
> 
> Original message -
> 
> Dear All,
> 
> This is a quick initial port of the as1531 driver Fabien submitted
> to the lm-sensors mailing list.
> 
> Based on top of a large series I just sent to Greg KH and posted to
> linux-iio. That has a few other dependencies, so might be easier
> to just to wait for them all to be in a standard tree (linux-next
> or mainline).
> 
> Dropped from hwmon driver - max and min values.
> 
> Stuff to add at some point (i.e. when needed)
> 
> 1) Differential channels - trivial to do, just add a few more entries to
> the channels array and some signed bits in read_raw
> 
> 2) Buffered read support - straight forward case where all channels have
> to be explicitly requested anyway.
> 
> 3) Other devices trivially added:
> as1530 is directly compatible.
> as1532/1533 look like 4 channel equivalents (add iio_chan_spec arrays).
> 
> The other similarly numbered parts are different enough to need their
> own drivers.
> 
> Fabien, as more of the code in the first patch is yours than mine,
> I've put you as author of that one.
> 
> The second is mostly stuff you'd have gotten in review to the hwmon
> driver anyway.
> 
> Jonathan
> 
> Fabien Marteau (1):
>   staging:iio:adc: as1531 driver initial conversion from hwmon
>     submission.
> 
> Jonathan Cameron (1):
>   staging:iio:adc:as1351 general cleanup and conversion to standard
>     functions.
> 
>  drivers/staging/iio/adc/Kconfig  |   10 +++
>  drivers/staging/iio/adc/Makefile |    2 +
>  drivers/staging/iio/adc/as1531.c |  166 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/adc/as1531.c
> 

[PATCH 1/2] staging:iio:adc: as1531 driver initial conversion from hwmon submission.

[Jonathan Cameron]

From: Fabien Marteau <fabien.marteau@armadeus.com>

V2: Fix double mutex unlock and whitespace (Guenter Roeck).
Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/adc/Kconfig  |   10 ++
 drivers/staging/iio/adc/Makefile |    2 +
 drivers/staging/iio/adc/as1531.c |  209 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 221 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/Kconfig
index 8c751c4..4af730a 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -168,6 +168,16 @@ config ADT7410
 	  Say yes here to build support for Analog Devices ADT7410
 	  temperature sensors.
 
+config AS1531
+tristate "Austria Microsystems AS1531 Analog to Digital Converter"
+	depends on SPI_MASTER
+	help
+	  If you say yes here you get support for Austria Microsystems AS1531.
+		AS1531 is a 12 bits Analog to digitals converter with 8 channels
+		provided by Austria-Microsystems company.
+	  This driver can also be built as a module.  If so, the module
+	  will be called as1531.
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc/Makefile
index 1d9b3f5..027b94e2 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -39,3 +39,5 @@ obj-$(CONFIG_AD7816) += ad7816.o
 obj-$(CONFIG_ADT75) += adt75.o
 obj-$(CONFIG_ADT7310) += adt7310.o
 obj-$(CONFIG_ADT7410) += adt7410.o
+
+obj-$(CONFIG_AS1531) += as1531.o
\ No newline at end of file
diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc/as1531.c
new file mode 100644
index 0000000..1724c39
--- /dev/null
+++ b/drivers/staging/iio/adc/as1531.c
@@ -0,0 +1,209 @@
+/*
+ * as1531.c
+ *
+ * Driver for Austria-Microsystem Analog to Digital Converter.
+ *
+ * Copyright (c) 2010, 2011 Fabien Marteau <fabien.marteau@armadeus.com>
+ * Driver based on Marc Pignat <marc.pignat@hevs.ch> adcxx.c driver.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "adc.h"
+
+#define AS1531_SPI_SPEED 64941
+#define AS1531_MAX_VALUE 2500
+
+#define AS1531_START_BIT	(0x80)
+#define AS1531_CHAN0		(0<<4)
+#define AS1531_CHAN1		(4<<4)
+#define AS1531_CHAN2		(1<<4)
+#define AS1531_CHAN3		(5<<4)
+#define AS1531_CHAN4		(2<<4)
+#define AS1531_CHAN5		(6<<4)
+#define AS1531_CHAN6		(3<<4)
+#define AS1531_CHAN7		(7<<4)
+
+#define AS1531_RANGE_0_TO_VREF			(1<<3)
+#define AS1531_RANGE_HALFVREF_TO_HALFVREF	(0<<3)
+
+#define AS1531_MODE_COM		(1<<2)
+#define AS1531_MODE_DIFF	(0<<2)
+
+#define AS1531_POWER_DOWN		0x0
+#define AS1531_POWER_REDUCED		0x1
+#define AS1531_POWER_REDUCED_BIS	0x2
+#define AS1531_POWER_NORMAL		0x3
+
+struct as1531_state {
+	struct spi_device *spi;
+	struct mutex lock;
+};
+
+static int as1531_message(struct spi_device *spi, int cmd, int *ret_value)
+{
+	struct spi_message	message;
+	struct spi_transfer	x[1];
+	int status, i;
+	u8	cmd_send;
+	unsigned char buf[64];
+	unsigned char buf_read[64];
+
+	cmd_send = cmd;
+
+	spi_message_init(&message);
+	memset(x, 0, sizeof x);
+	memset(buf, 0, sizeof(buf));
+	memset(buf_read, 0, sizeof(buf_read));
+
+	for (i = 0; i < 8; i++) {
+		buf[i] = ((cmd_send & 0x80)>>7);
+		cmd_send = cmd_send << 1;
+	}
+
+	x[0].tx_buf = buf;
+	x[0].len = 24;
+	x[0].rx_buf = buf_read;
+	x[0].speed_hz = AS1531_SPI_SPEED;
+	x[0].bits_per_word = 1;
+	spi_message_add_tail(&x[0], &message);
+
+	status = spi_sync(spi, &message);
+	if (status < 0)
+		return status;
+
+	*ret_value = buf_read[11] & 0x01;
+	for (i = 12; i < 23 ; i++) {
+		*ret_value = *ret_value << 1;
+		*ret_value = *ret_value | (buf_read[i]&0x01);
+	}
+
+	return 0;
+}
+
+static int as1531_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+	int status = 0;
+	int ret_value = 0;
+	struct as1531_state *st = iio_priv(indio_dev);
+
+	if (mutex_lock_interruptible(&st->lock))
+		return -ERESTARTSYS;
+
+	status = as1531_message(st->spi,
+				AS1531_START_BIT | chan->address |
+				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+				AS1531_POWER_NORMAL,
+				&ret_value);
+	mutex_unlock(&st->lock);
+	if (status < 0)
+		return status;
+
+	*val = ret_value*2500/4096;
+
+	return IIO_VAL_INT;
+}
+
+static const struct iio_chan_spec as1531_channels[] = {
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 0, 1, 0, AS1531_CHAN0, 0, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 1, 1, 0, AS1531_CHAN1, 1, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 2, 1, 0, AS1531_CHAN2, 2, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 3, 1, 0, AS1531_CHAN3, 3, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 4, 1, 0, AS1531_CHAN4, 4, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 5, 1, 0, AS1531_CHAN5, 5, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 6, 1, 0, AS1531_CHAN6, 6, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 7, 1, 0, AS1531_CHAN7, 7, {}, 0),
+};
+
+static const struct iio_info as1531_info = {
+	.read_raw = &as1531_read_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int __devinit as1531_probe(struct spi_device *spi)
+{
+	int ret;
+	struct as1531_state *st;
+	struct iio_dev *indio_dev = iio_allocate_device(sizeof(*st));
+
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	mutex_init(&st->lock);
+	st->spi = spi;
+
+	spi_set_drvdata(spi, indio_dev);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->info = &as1531_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = as1531_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as1531_channels);
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		iio_free_device(indio_dev);
+	return ret;
+}
+
+static int as1531_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+	return 0;
+}
+
+static const struct spi_device_id as1531_id[] = {
+	{"as1531", 0},
+	{}
+};
+
+static struct spi_driver as1531_driver = {
+	.driver = {
+		.name	= "as1531",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= as1531_probe,
+	.remove	= __devexit_p(as1531_remove),
+	.id_table = as1531_id,
+};
+
+static int __init init_as1531(void)
+{
+	return spi_register_driver(&as1531_driver);
+}
+module_init(init_as1531);
+
+static void __exit exit_as1531(void)
+{
+	spi_unregister_driver(&as1531_driver);
+}
+module_exit(exit_as1531);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabien Marteau <fabien.marteau@armadeus.com>");
+MODULE_DESCRIPTION("Driver for AS1531 ADC");
-- 
1.7.3.4

Re: [PATCH 1/2] staging:iio:adc: as1531 driver initial conversion from hwmon submission.

[Guenter Roeck]

On Wed, May 18, 2011 at 12:16:59PM -0400, Jonathan Cameron wrote:
> From: Fabien Marteau <fabien.marteau@armadeus.com>
>=20
> V2: Fix double mutex unlock and whitespace (Guenter Roeck).

Should be below ---

> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

and nitpick below, otherwise=20

Revewed-by: Guenter Roeck <guenter.roeck@ericsson.com>

> ---
>  drivers/staging/iio/adc/Kconfig  |   10 ++
>  drivers/staging/iio/adc/Makefile |    2 +
>  drivers/staging/iio/adc/as1531.c |  209 ++++++++++++++++++++++++++++++=
++++++++
>  3 files changed, 221 insertions(+), 0 deletions(-)
>=20
> diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/=
Kconfig
> index 8c751c4..4af730a 100644
> --- a/drivers/staging/iio/adc/Kconfig
> +++ b/drivers/staging/iio/adc/Kconfig
> @@ -168,6 +168,16 @@ config ADT7410
>  	  Say yes here to build support for Analog Devices ADT7410
>  	  temperature sensors.
> =20
> +config AS1531
> +tristate "Austria Microsystems AS1531 Analog to Digital Converter"
> +	depends on SPI_MASTER
> +	help
> +	  If you say yes here you get support for Austria Microsystems AS1531.
> +		AS1531 is a 12=A0bits Analog to digitals converter with 8 channels
> +		provided by Austria-Microsystems company.
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called as1531.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc=
/Makefile
> index 1d9b3f5..027b94e2 100644
> --- a/drivers/staging/iio/adc/Makefile
> +++ b/drivers/staging/iio/adc/Makefile
> @@ -39,3 +39,5 @@ obj-$(CONFIG_AD7816) +=3D ad7816.o
>  obj-$(CONFIG_ADT75) +=3D adt75.o
>  obj-$(CONFIG_ADT7310) +=3D adt7310.o
>  obj-$(CONFIG_ADT7410) +=3D adt7410.o
> +
> +obj-$(CONFIG_AS1531) +=3D as1531.o
> \ No newline at end of file

What does this mean ? Is there a missing newline ?

Thanks,
Guenter

[PATCH 2/2] staging:iio:adc:as1351 general cleanup and conversion to standard functions.

[Jonathan Cameron]

V2: Fix the precedence issue Guenter Roeck pointed out.

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/adc/as1531.c |   59 +++++--------------------------------
 1 files changed, 8 insertions(+), 51 deletions(-)

diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc/as1531.c
index 1724c39..24a1ac0 100644
--- a/drivers/staging/iio/adc/as1531.c
+++ b/drivers/staging/iio/adc/as1531.c
@@ -58,47 +58,6 @@ struct as1531_state {
 	struct mutex lock;
 };
 
-static int as1531_message(struct spi_device *spi, int cmd, int *ret_value)
-{
-	struct spi_message	message;
-	struct spi_transfer	x[1];
-	int status, i;
-	u8	cmd_send;
-	unsigned char buf[64];
-	unsigned char buf_read[64];
-
-	cmd_send = cmd;
-
-	spi_message_init(&message);
-	memset(x, 0, sizeof x);
-	memset(buf, 0, sizeof(buf));
-	memset(buf_read, 0, sizeof(buf_read));
-
-	for (i = 0; i < 8; i++) {
-		buf[i] = ((cmd_send & 0x80)>>7);
-		cmd_send = cmd_send << 1;
-	}
-
-	x[0].tx_buf = buf;
-	x[0].len = 24;
-	x[0].rx_buf = buf_read;
-	x[0].speed_hz = AS1531_SPI_SPEED;
-	x[0].bits_per_word = 1;
-	spi_message_add_tail(&x[0], &message);
-
-	status = spi_sync(spi, &message);
-	if (status < 0)
-		return status;
-
-	*ret_value = buf_read[11] & 0x01;
-	for (i = 12; i < 23 ; i++) {
-		*ret_value = *ret_value << 1;
-		*ret_value = *ret_value | (buf_read[i]&0x01);
-	}
-
-	return 0;
-}
-
 static int as1531_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val,
@@ -106,22 +65,22 @@ static int as1531_read_raw(struct iio_dev *indio_dev,
 			   long m)
 {
 	int status = 0;
-	int ret_value = 0;
+	u16 ret_value = 0;
 	struct as1531_state *st = iio_priv(indio_dev);
 
 	if (mutex_lock_interruptible(&st->lock))
 		return -ERESTARTSYS;
 
-	status = as1531_message(st->spi,
-				AS1531_START_BIT | chan->address |
-				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
-				AS1531_POWER_NORMAL,
-				&ret_value);
+	status = spi_w8r16(st->spi,
+			   AS1531_START_BIT | chan->address |
+			   AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+			   AS1531_POWER_NORMAL);
 	mutex_unlock(&st->lock);
 	if (status < 0)
 		return status;
 
-	*val = ret_value*2500/4096;
+	ret_value = status;
+	*val = ((be16_to_cpu(ret_value) >> 2) & 0xFFF)*2500/4096;
 
 	return IIO_VAL_INT;
 }
@@ -171,9 +130,7 @@ static int __devinit as1531_probe(struct spi_device *spi)
 
 static int as1531_remove(struct spi_device *spi)
 {
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-
-	iio_device_unregister(indio_dev);
+	iio_device_unregister(spi_get_drvdata(spi));
 	return 0;
 }
 
-- 
1.7.3.4

Re: [PATCH 2/2] staging:iio:adc:as1351 general cleanup and conversion to standard functions.

[Guenter Roeck]

On Wed, May 18, 2011 at 12:17:00PM -0400, Jonathan Cameron wrote:
> V2: Fix the precedence issue Guenter Roeck pointed out.
> 
Should be below ---, unless you want it in the commit log.

> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

Nitpick below, otherwise

Reviewed-by: Guenter Roeck <guenter.roeck@ericsson.com>

> ---
>  drivers/staging/iio/adc/as1531.c |   59 +++++--------------------------------
>  1 files changed, 8 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc/as1531.c
> index 1724c39..24a1ac0 100644
> --- a/drivers/staging/iio/adc/as1531.c
> +++ b/drivers/staging/iio/adc/as1531.c
> @@ -58,47 +58,6 @@ struct as1531_state {
>  	struct mutex lock;
>  };
>  
> -static int as1531_message(struct spi_device *spi, int cmd, int *ret_value)
> -{
> -	struct spi_message	message;
> -	struct spi_transfer	x[1];
> -	int status, i;
> -	u8	cmd_send;
> -	unsigned char buf[64];
> -	unsigned char buf_read[64];
> -
> -	cmd_send = cmd;
> -
> -	spi_message_init(&message);
> -	memset(x, 0, sizeof x);
> -	memset(buf, 0, sizeof(buf));
> -	memset(buf_read, 0, sizeof(buf_read));
> -
> -	for (i = 0; i < 8; i++) {
> -		buf[i] = ((cmd_send & 0x80)>>7);
> -		cmd_send = cmd_send << 1;
> -	}
> -
> -	x[0].tx_buf = buf;
> -	x[0].len = 24;
> -	x[0].rx_buf = buf_read;
> -	x[0].speed_hz = AS1531_SPI_SPEED;
> -	x[0].bits_per_word = 1;
> -	spi_message_add_tail(&x[0], &message);
> -
> -	status = spi_sync(spi, &message);
> -	if (status < 0)
> -		return status;
> -
> -	*ret_value = buf_read[11] & 0x01;
> -	for (i = 12; i < 23 ; i++) {
> -		*ret_value = *ret_value << 1;
> -		*ret_value = *ret_value | (buf_read[i]&0x01);
> -	}
> -
> -	return 0;
> -}
> -
>  static int as1531_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val,
> @@ -106,22 +65,22 @@ static int as1531_read_raw(struct iio_dev *indio_dev,
>  			   long m)
>  {
>  	int status = 0;
> -	int ret_value = 0;
> +	u16 ret_value = 0;
>  	struct as1531_state *st = iio_priv(indio_dev);
>  
>  	if (mutex_lock_interruptible(&st->lock))
>  		return -ERESTARTSYS;
>  
> -	status = as1531_message(st->spi,
> -				AS1531_START_BIT | chan->address |
> -				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> -				AS1531_POWER_NORMAL,
> -				&ret_value);
> +	status = spi_w8r16(st->spi,
> +			   AS1531_START_BIT | chan->address |
> +			   AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +			   AS1531_POWER_NORMAL);
>  	mutex_unlock(&st->lock);
>  	if (status < 0)
>  		return status;
>  
> -	*val = ret_value*2500/4096;
> +	ret_value = status;
> +	*val = ((be16_to_cpu(ret_value) >> 2) & 0xFFF)*2500/4096;
>  
Even though checkpatch doesn't complain anymore (why ?), the rule to put spaces
around * and / still applies:

Use one space around (on each side of) most binary and ternary operators,
such as any of these:

        =  +  -  <  >  *  /  %  |  &  ^  <=  >=  ==  !=  ?  :

Thanks,
Guenter

Re: [PATCH 2/2] staging:iio:adc:as1351 general cleanup and conversion to standard functions.

[Jonathan Cameron]

On 05/18/11 17:35, Guenter Roeck wrote:
> On Wed, May 18, 2011 at 12:17:00PM -0400, Jonathan Cameron wrote:
>> V2: Fix the precedence issue Guenter Roeck pointed out.
>>
> Should be below ---, unless you want it in the commit log.
> 
>> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> Nitpick below, otherwise
> 
> Reviewed-by: Guenter Roeck <guenter.roeck@ericsson.com>
Thanks,

Fixed up as suggested.  I'll wait for Fabien's test results
before doing any more with this driver.

[PATCH 1/2] staging:iio:adc: as1531 driver initial conversion from hwmon submission.

[Jonathan Cameron]

=46rom: Fabien Marteau <fabien.marteau@armadeus.com>

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/adc/Kconfig  |   10 ++
 drivers/staging/iio/adc/Makefile |    2 +
 drivers/staging/iio/adc/as1531.c |  212 ++++++++++++++++++++++++++++++=
++++++++
 3 files changed, 224 insertions(+), 0 deletions(-)

diff --git a/drivers/staging/iio/adc/Kconfig b/drivers/staging/iio/adc/=
Kconfig
index 8c751c4..4af730a 100644
--- a/drivers/staging/iio/adc/Kconfig
+++ b/drivers/staging/iio/adc/Kconfig
@@ -168,6 +168,16 @@ config ADT7410
 	  Say yes here to build support for Analog Devices ADT7410
 	  temperature sensors.
=20
+config AS1531
+tristate "Austria Microsystems AS1531 Analog to Digital Converter"
+	depends on SPI_MASTER
+	help
+	  If you say yes here you get support for Austria Microsystems AS1531=
=2E
+		AS1531 is a 12=C2=A0bits Analog to digitals converter with 8 channel=
s
+		provided by Austria-Microsystems company.
+	  This driver can also be built as a module.  If so, the module
+	  will be called as1531.
+
 config MAX1363
 	tristate "Maxim max1363 ADC driver"
 	depends on I2C
diff --git a/drivers/staging/iio/adc/Makefile b/drivers/staging/iio/adc=
/Makefile
index 1d9b3f5..027b94e2 100644
--- a/drivers/staging/iio/adc/Makefile
+++ b/drivers/staging/iio/adc/Makefile
@@ -39,3 +39,5 @@ obj-$(CONFIG_AD7816) +=3D ad7816.o
 obj-$(CONFIG_ADT75) +=3D adt75.o
 obj-$(CONFIG_ADT7310) +=3D adt7310.o
 obj-$(CONFIG_ADT7410) +=3D adt7410.o
+
+obj-$(CONFIG_AS1531) +=3D as1531.o
\ No newline at end of file
diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc=
/as1531.c
new file mode 100644
index 0000000..7222549
--- /dev/null
+++ b/drivers/staging/iio/adc/as1531.c
@@ -0,0 +1,212 @@
+/*
+ * as1531.c
+ *
+ * Driver for Austria-Microsystem Analog to Digital Converter.
+ *
+ * Copyright (c) 2010, 2011 Fabien Marteau <fabien.marteau@armadeus.co=
m>
+ * Driver based on Marc Pignat <marc.pignat@hevs.ch> adcxx.c driver.
+ *
+ * This program is free software; you can redistribute it and/or modif=
y
+ * it under the terms of the GNU General Public License as published b=
y
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/sysfs.h>
+#include <linux/spi/spi.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "adc.h"
+
+#define AS1531_SPI_SPEED 64941
+#define AS1531_MAX_VALUE 2500
+
+#define AS1531_START_BIT	(0x80)
+#define AS1531_CHAN0		(0<<4)
+#define AS1531_CHAN1		(4<<4)
+#define AS1531_CHAN2		(1<<4)
+#define AS1531_CHAN3		(5<<4)
+#define AS1531_CHAN4		(2<<4)
+#define AS1531_CHAN5		(6<<4)
+#define AS1531_CHAN6		(3<<4)
+#define AS1531_CHAN7		(7<<4)
+
+#define AS1531_RANGE_0_TO_VREF			(1<<3)
+#define AS1531_RANGE_HALFVREF_TO_HALFVREF	(0<<3)
+
+#define AS1531_MODE_COM		(1<<2)
+#define AS1531_MODE_DIFF	(0<<2)
+
+#define AS1531_POWER_DOWN		0x0
+#define AS1531_POWER_REDUCED		0x1
+#define AS1531_POWER_REDUCED_BIS	0x2
+#define AS1531_POWER_NORMAL		0x3
+
+struct as1531_state {
+	struct spi_device *spi;
+	struct mutex lock;
+};
+
+static int as1531_message(struct spi_device *spi, int cmd, int *ret_va=
lue)
+{
+	struct spi_message	message;
+	struct spi_transfer	x[1];
+	int status, i;
+	u8	cmd_send;
+	unsigned char buf[64];
+	unsigned char buf_read[64];
+
+	cmd_send =3D cmd;
+
+	spi_message_init(&message);
+	memset(x, 0, sizeof x);
+	memset(buf, 0, sizeof(buf));
+	memset(buf_read, 0, sizeof(buf_read));
+
+	for (i =3D 0; i < 8; i++) {
+		buf[i] =3D ((cmd_send & 0x80)>>7);
+		cmd_send =3D cmd_send << 1;
+	}
+
+	x[0].tx_buf =3D buf;
+	x[0].len =3D 24;
+	x[0].rx_buf =3D buf_read;
+	x[0].speed_hz =3D AS1531_SPI_SPEED;
+	x[0].bits_per_word =3D 1;
+	spi_message_add_tail(&x[0], &message);
+
+	status =3D spi_sync(spi, &message);
+	if (status < 0)
+		return status;
+
+	*ret_value =3D buf_read[11] & 0x01;
+	for (i =3D 12; i < 23 ; i++) {
+		*ret_value =3D *ret_value << 1;
+		*ret_value =3D *ret_value | (buf_read[i]&0x01);
+	}
+
+	return 0;
+}
+
+static int as1531_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val,
+			   int *val2,
+			   long m)
+{
+
+	int status =3D 0;
+	int ret_value =3D 0;
+	struct as1531_state *st =3D iio_priv(indio_dev);
+	if (mutex_lock_interruptible(&st->lock))
+		return -ERESTARTSYS;
+
+	status =3D as1531_message(st->spi,
+				AS1531_START_BIT | chan->address |
+				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+				AS1531_POWER_NORMAL,
+				&ret_value);
+	mutex_unlock(&st->lock);
+	if (status < 0)
+		goto out;
+=09
+	*val =3D ret_value*2500/4096;
+
+	return IIO_VAL_INT;
+out:
+	mutex_unlock(&st->lock);
+	return status;
+}
+
+static const struct iio_chan_spec as1531_channels[] =3D {
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 0, 1, 0, AS1531_CHAN0, 0, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 1, 1, 0, AS1531_CHAN1, 1, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 2, 1, 0, AS1531_CHAN2, 2, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 3, 1, 0, AS1531_CHAN3, 3, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 4, 1, 0, AS1531_CHAN4, 4, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 5, 1, 0, AS1531_CHAN5, 5, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 6, 1, 0, AS1531_CHAN6, 6, {}, 0),
+	IIO_CHAN(IIO_IN, 0, 1, 1, NULL, 7, 1, 0, AS1531_CHAN7, 7, {}, 0),
+};
+
+static const struct iio_info as1531_info =3D {
+	.read_raw =3D &as1531_read_raw,
+	.driver_module =3D THIS_MODULE,
+};
+
+static int __devinit as1531_probe(struct spi_device *spi)
+{
+	int ret;
+	struct as1531_state *st;
+	struct iio_dev *indio_dev =3D iio_allocate_device(sizeof(*st));
+
+	if (indio_dev =3D=3D NULL)
+		return -ENOMEM;
+
+	st =3D iio_priv(indio_dev);
+	mutex_init(&st->lock);
+	st->spi =3D spi;
+
+	spi_set_drvdata(spi, indio_dev);
+	indio_dev->dev.parent =3D &spi->dev;
+	indio_dev->name =3D spi_get_device_id(spi)->name;
+	indio_dev->info =3D &as1531_info;
+	indio_dev->modes =3D INDIO_DIRECT_MODE;
+	indio_dev->channels =3D as1531_channels;
+	indio_dev->num_channels =3D ARRAY_SIZE(as1531_channels);
+
+	ret =3D iio_device_register(indio_dev);
+	if (ret < 0)
+		iio_free_device(indio_dev);
+	return ret;
+}
+
+static int as1531_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev =3D spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+	return 0;
+}
+
+static const struct spi_device_id as1531_id[] =3D {
+	{"as1531", 0},
+	{}
+};
+
+static struct spi_driver as1531_driver =3D {
+	.driver =3D {
+		.name	=3D "as1531",
+		.owner	=3D THIS_MODULE,
+	},
+	.probe	=3D as1531_probe,
+	.remove	=3D __devexit_p(as1531_remove),
+	.id_table =3D as1531_id,
+};
+
+static int __init init_as1531(void)
+{
+	return spi_register_driver(&as1531_driver);
+}
+module_init(init_as1531);
+
+static void __exit exit_as1531(void)
+{
+	spi_unregister_driver(&as1531_driver);
+}
+module_exit(exit_as1531);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabien Marteau <fabien.marteau@armadeus.com>");
+MODULE_DESCRIPTION("Driver for AS1531 ADC");
--=20
1.7.3.4

Re: [PATCH 1/2] staging:iio:adc: as1531 driver initial conversion from hwmon submission.

[Guenter Roeck]

On Wed, May 18, 2011 at 11:39:31AM -0400, Jonathan Cameron wrote:
> From: Fabien Marteau <fabien.marteau@armadeus.com>
> 
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>

Hi Jonathan,

nice job. Thanks a lot for the effort.

[ ... ]

> +static int as1531_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val,
> +			   int *val2,
> +			   long m)
> +{
> +
Extra blank line

> +	int status = 0;
> +	int ret_value = 0;
> +	struct as1531_state *st = iio_priv(indio_dev);

Move it to here, maybe ?

> +	if (mutex_lock_interruptible(&st->lock))
> +		return -ERESTARTSYS;
> +
> +	status = as1531_message(st->spi,
> +				AS1531_START_BIT | chan->address |
> +				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +				AS1531_POWER_NORMAL,
> +				&ret_value);
> +	mutex_unlock(&st->lock);
> +	if (status < 0)
> +		goto out;
> +	
> +	*val = ret_value*2500/4096;
> +
> +	return IIO_VAL_INT;
> +out:
> +	mutex_unlock(&st->lock);

Mutex was unlocked above already. Maybe just return status above ?

Thanks,
Guenter 

Re: [PATCH 1/2] staging:iio:adc: as1531 driver initial conversion from hwmon submission.

[Jonathan Cameron]

On 05/18/11 17:02, Guenter Roeck wrote:
> On Wed, May 18, 2011 at 11:39:31AM -0400, Jonathan Cameron wrote:
>> From: Fabien Marteau <fabien.marteau-d2DlULPkwbNWk0Htik3J/w@public.gmane.org>
>>
>> Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> 
> Hi Jonathan,
> 
> nice job. Thanks a lot for the effort.
> 
> [ ... ]
> 
>> +static int as1531_read_raw(struct iio_dev *indio_dev,
>> +			   struct iio_chan_spec const *chan,
>> +			   int *val,
>> +			   int *val2,
>> +			   long m)
>> +{
>> +
> Extra blank line
> 
>> +	int status = 0;
>> +	int ret_value = 0;
>> +	struct as1531_state *st = iio_priv(indio_dev);
> 
> Move it to here, maybe ?
> 
>> +	if (mutex_lock_interruptible(&st->lock))
>> +		return -ERESTARTSYS;
>> +
>> +	status = as1531_message(st->spi,
>> +				AS1531_START_BIT | chan->address |
>> +				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
>> +				AS1531_POWER_NORMAL,
>> +				&ret_value);
>> +	mutex_unlock(&st->lock);
>> +	if (status < 0)
>> +		goto out;
>> +	
>> +	*val = ret_value*2500/4096;
>> +
>> +	return IIO_VAL_INT;
>> +out:
>> +	mutex_unlock(&st->lock);
> 
> Mutex was unlocked above already. Maybe just return status above ?
Hmm.. Sometimes working without testing shows just how bad first versions
of code really can be ;)

Thanks for the other review as well.  Have fixed up both.


Thanks.

Jonathan 

[PATCH 2/2] staging:iio:adc:as1351 general cleanup and conversion to standard functions.

[Jonathan Cameron]

Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
---
 drivers/staging/iio/adc/as1531.c |   60 +++++--------------------------------
 1 files changed, 8 insertions(+), 52 deletions(-)

diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc/as1531.c
index 7222549..30119ce 100644
--- a/drivers/staging/iio/adc/as1531.c
+++ b/drivers/staging/iio/adc/as1531.c
@@ -58,47 +58,6 @@ struct as1531_state {
 	struct mutex lock;
 };
 
-static int as1531_message(struct spi_device *spi, int cmd, int *ret_value)
-{
-	struct spi_message	message;
-	struct spi_transfer	x[1];
-	int status, i;
-	u8	cmd_send;
-	unsigned char buf[64];
-	unsigned char buf_read[64];
-
-	cmd_send = cmd;
-
-	spi_message_init(&message);
-	memset(x, 0, sizeof x);
-	memset(buf, 0, sizeof(buf));
-	memset(buf_read, 0, sizeof(buf_read));
-
-	for (i = 0; i < 8; i++) {
-		buf[i] = ((cmd_send & 0x80)>>7);
-		cmd_send = cmd_send << 1;
-	}
-
-	x[0].tx_buf = buf;
-	x[0].len = 24;
-	x[0].rx_buf = buf_read;
-	x[0].speed_hz = AS1531_SPI_SPEED;
-	x[0].bits_per_word = 1;
-	spi_message_add_tail(&x[0], &message);
-
-	status = spi_sync(spi, &message);
-	if (status < 0)
-		return status;
-
-	*ret_value = buf_read[11] & 0x01;
-	for (i = 12; i < 23 ; i++) {
-		*ret_value = *ret_value << 1;
-		*ret_value = *ret_value | (buf_read[i]&0x01);
-	}
-
-	return 0;
-}
-
 static int as1531_read_raw(struct iio_dev *indio_dev,
 			   struct iio_chan_spec const *chan,
 			   int *val,
@@ -107,21 +66,20 @@ static int as1531_read_raw(struct iio_dev *indio_dev,
 {
 
 	int status = 0;
-	int ret_value = 0;
+	u16 ret_value = 0;
 	struct as1531_state *st = iio_priv(indio_dev);
 	if (mutex_lock_interruptible(&st->lock))
 		return -ERESTARTSYS;
 
-	status = as1531_message(st->spi,
-				AS1531_START_BIT | chan->address |
-				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
-				AS1531_POWER_NORMAL,
-				&ret_value);
+	status = spi_w8r16(st->spi,
+			   AS1531_START_BIT | chan->address |
+			   AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+			   AS1531_POWER_NORMAL);
 	mutex_unlock(&st->lock);
 	if (status < 0)
 		goto out;
-	
-	*val = ret_value*2500/4096;
+	ret_value = status;
+	*val = (be16_to_cpu(ret_value) >>2)&0xFFF*2500/4096;
 
 	return IIO_VAL_INT;
 out:
@@ -174,9 +132,7 @@ static int __devinit as1531_probe(struct spi_device *spi)
 
 static int as1531_remove(struct spi_device *spi)
 {
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-
-	iio_device_unregister(indio_dev);
+	iio_device_unregister(spi_get_drvdata(spi));
 	return 0;
 }
 
-- 
1.7.3.4

Re: [PATCH 2/2] staging:iio:adc:as1351 general cleanup and conversion to standard functions.

[Guenter Roeck]

Hi Jonathan,

On Wed, May 18, 2011 at 11:39:32AM -0400, Jonathan Cameron wrote:
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> ---
>  drivers/staging/iio/adc/as1531.c |   60 +++++--------------------------------
>  1 files changed, 8 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/as1531.c b/drivers/staging/iio/adc/as1531.c
> index 7222549..30119ce 100644
> --- a/drivers/staging/iio/adc/as1531.c
> +++ b/drivers/staging/iio/adc/as1531.c
> @@ -58,47 +58,6 @@ struct as1531_state {
>  	struct mutex lock;
>  };
>  
> -static int as1531_message(struct spi_device *spi, int cmd, int *ret_value)
> -{
> -	struct spi_message	message;
> -	struct spi_transfer	x[1];
> -	int status, i;
> -	u8	cmd_send;
> -	unsigned char buf[64];
> -	unsigned char buf_read[64];
> -
> -	cmd_send = cmd;
> -
> -	spi_message_init(&message);
> -	memset(x, 0, sizeof x);
> -	memset(buf, 0, sizeof(buf));
> -	memset(buf_read, 0, sizeof(buf_read));
> -
> -	for (i = 0; i < 8; i++) {
> -		buf[i] = ((cmd_send & 0x80)>>7);
> -		cmd_send = cmd_send << 1;
> -	}
> -
> -	x[0].tx_buf = buf;
> -	x[0].len = 24;
> -	x[0].rx_buf = buf_read;
> -	x[0].speed_hz = AS1531_SPI_SPEED;
> -	x[0].bits_per_word = 1;
> -	spi_message_add_tail(&x[0], &message);
> -
> -	status = spi_sync(spi, &message);
> -	if (status < 0)
> -		return status;
> -
> -	*ret_value = buf_read[11] & 0x01;
> -	for (i = 12; i < 23 ; i++) {
> -		*ret_value = *ret_value << 1;
> -		*ret_value = *ret_value | (buf_read[i]&0x01);
> -	}
> -
> -	return 0;
> -}
> -
>  static int as1531_read_raw(struct iio_dev *indio_dev,
>  			   struct iio_chan_spec const *chan,
>  			   int *val,
> @@ -107,21 +66,20 @@ static int as1531_read_raw(struct iio_dev *indio_dev,
>  {
>  
>  	int status = 0;
> -	int ret_value = 0;
> +	u16 ret_value = 0;
>  	struct as1531_state *st = iio_priv(indio_dev);
>  	if (mutex_lock_interruptible(&st->lock))
>  		return -ERESTARTSYS;
>  
> -	status = as1531_message(st->spi,
> -				AS1531_START_BIT | chan->address |
> -				AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> -				AS1531_POWER_NORMAL,
> -				&ret_value);
> +	status = spi_w8r16(st->spi,
> +			   AS1531_START_BIT | chan->address |
> +			   AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +			   AS1531_POWER_NORMAL);
>  	mutex_unlock(&st->lock);
>  	if (status < 0)
>  		goto out;
> -	
> -	*val = ret_value*2500/4096;
> +	ret_value = status;
> +	*val = (be16_to_cpu(ret_value) >>2)&0xFFF*2500/4096;

checkpatch does complain about this one ... oddly enough, though, only about the missing space
between >> and 2.

& has lower precedence than * and /, so I am not sure if the code here does
what you expect it to do.

Thanks,
Guenter