[PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[fabien.marteau]

Hello,

This is the driver of as1531 analog to digital chip we use on our ARMadeus platform.

[lm-sensors] [PATCH] hwmon: Driver for as1531,

[fabien.marteau]

Hello,

This is the driver of as1531 analog to digital chip we use on our ARMadeus platform.


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[fabien.marteau]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 9513 bytes --]

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


Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
---
 drivers/hwmon/Kconfig  |   10 ++
 drivers/hwmon/Makefile |    1 +
 drivers/hwmon/as1531.c |  297 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 308 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/as1531.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 50e40db..d2ba655 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -104,6 +104,16 @@ config SENSORS_ADCXX
 	  This driver can also be built as a module.  If so, the module
 	  will be called adcxx.
 
+config SENSORS_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 SENSORS_ADM1021
 	tristate "Analog Devices ADM1021 and compatibles"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 967d0ea..8a304b3 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
 obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
 obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
 obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
+obj-$(CONFIG_SENSORS_AS1531)	+= as1531.o
 obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
 obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
 obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
diff --git a/drivers/hwmon/as1531.c b/drivers/hwmon/as1531.c
new file mode 100644
index 0000000..3384663
--- /dev/null
+++ b/drivers/hwmon/as1531.c
@@ -0,0 +1,297 @@
+/*
+ * 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/init.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.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 {
+	struct device *hwmon_dev;
+	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 ssize_t as1531_read(struct device *dev,
+			struct device_attribute *devattr, char *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct as1531 *adc = dev_get_drvdata(&spi->dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	int status = 0;
+	int ret_value;
+	int32_t cmd;
+
+
+	if (mutex_lock_interruptible(&adc->lock))
+		return -ERESTARTSYS;
+
+	switch (attr->index) {
+	case 0:
+		cmd = AS1531_START_BIT | AS1531_CHAN0 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 1:
+		cmd = AS1531_START_BIT | AS1531_CHAN1 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 2:
+		cmd = AS1531_START_BIT | AS1531_CHAN2 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 3:
+		cmd = AS1531_START_BIT | AS1531_CHAN3 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 4:
+		cmd = AS1531_START_BIT | AS1531_CHAN4 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 5:
+		cmd = AS1531_START_BIT | AS1531_CHAN5 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 6:
+		cmd = AS1531_START_BIT | AS1531_CHAN6 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	case 7:
+		cmd = AS1531_START_BIT | AS1531_CHAN7 |
+		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
+		AS1531_POWER_NORMAL;
+		break;
+	default:
+		status = -EINVAL;
+		goto out;
+	}
+	status = as1531_message(spi, cmd, &ret_value);
+	if (status < 0)
+		goto out;
+
+	status = sprintf(buf, "%d\n", ret_value*2500/4096);
+out:
+	mutex_unlock(&adc->lock);
+	return status;
+}
+
+
+static ssize_t as1531_show_min(struct device *dev,
+		struct device_attribute *devattr, char *buf)
+{
+	return sprintf(buf, "0\n");
+}
+
+static ssize_t as1531_show_max(struct device *dev,
+		struct device_attribute *devattr, char *buf)
+{
+	return sprintf(buf, "%d\n", AS1531_MAX_VALUE);
+}
+
+static ssize_t as1531_show_name(struct device *dev, struct device_attribute
+				*devattr, char *buf)
+{
+	return sprintf(buf, "as1531\n");
+}
+
+static struct sensor_device_attribute as1531_input[] = {
+	SENSOR_ATTR(name, S_IRUGO, as1531_show_name, NULL, 0),
+	SENSOR_ATTR(in_min, S_IRUGO, as1531_show_min, NULL, 0),
+	SENSOR_ATTR(in_max, S_IRUGO, as1531_show_max, NULL, 0),
+	SENSOR_ATTR(in0_input, S_IRUGO, as1531_read, NULL, 0),
+	SENSOR_ATTR(in1_input, S_IRUGO, as1531_read, NULL, 1),
+	SENSOR_ATTR(in2_input, S_IRUGO, as1531_read, NULL, 2),
+	SENSOR_ATTR(in3_input, S_IRUGO, as1531_read, NULL, 3),
+	SENSOR_ATTR(in4_input, S_IRUGO, as1531_read, NULL, 4),
+	SENSOR_ATTR(in5_input, S_IRUGO, as1531_read, NULL, 5),
+	SENSOR_ATTR(in6_input, S_IRUGO, as1531_read, NULL, 6),
+	SENSOR_ATTR(in7_input, S_IRUGO, as1531_read, NULL, 7),
+};
+
+/*----------------------------------------------------------------------*/
+
+static int __devinit as1531_probe(struct spi_device *spi)
+{
+	struct as1531 *adc;
+	int status;
+	int i;
+
+	adc = kzalloc(sizeof(struct as1531), GFP_KERNEL);
+	if (adc == NULL)
+		return -ENOMEM;
+
+	mutex_init(&adc->lock);
+	mutex_lock(&adc->lock);
+
+	dev_set_drvdata(&spi->dev, adc);
+
+	for (i = 0; i < 11; i++) {
+		status = device_create_file(&spi->dev,
+						&as1531_input[i].dev_attr);
+		if (status < 0) {
+			dev_err(&spi->dev, "device_create_file failed.\n");
+			goto out_err;
+		}
+	}
+
+	adc->hwmon_dev = hwmon_device_register(&spi->dev);
+	if (IS_ERR(adc->hwmon_dev)) {
+		dev_err(&spi->dev, "hwmon_device_register failed.\n");
+		status = PTR_ERR(adc->hwmon_dev);
+		goto out_err;
+	}
+
+	mutex_unlock(&adc->lock);
+	return 0;
+
+out_err:
+	for (i--; i >= 0; i--)
+		device_remove_file(&spi->dev, &as1531_input[i].dev_attr);
+	dev_set_drvdata(&spi->dev, NULL);
+	mutex_unlock(&adc->lock);
+	kfree(adc);
+	return status;
+}
+
+static int __devexit as1531_remove(struct spi_device *spi)
+{
+	struct as1531 *adc = dev_get_drvdata(&spi->dev);
+	int i;
+
+	mutex_lock(&adc->lock);
+	hwmon_device_unregister(adc->hwmon_dev);
+	for (i = 0; i < 8; i++)
+		device_remove_file(&spi->dev, &as1531_input[i].dev_attr);
+
+	dev_set_drvdata(&spi->dev, NULL);
+	mutex_unlock(&adc->lock);
+	kfree(adc);
+
+	return 0;
+}
+
+/* SPI structures */
+
+static struct spi_driver as1531_driver = {
+	.driver = {
+		.name	= "as1531",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= as1531_probe,
+	.remove	= __devexit_p(as1531_remove),
+};
+
+/* Init module */
+
+static int __init init_as1531(void)
+{
+	return spi_register_driver(&as1531_driver);
+}
+
+static void __exit exit_as1531(void)
+{
+	spi_unregister_driver(&as1531_driver);
+}
+
+module_init(init_as1531);
+module_exit(exit_as1531);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabien Marteau <fabien.marteau@armadeus.com>");
+MODULE_DESCRIPTION("Driver for AS1531 ADC");
-- 
1.7.0.4

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Randy Dunlap]

On Mon, 16 May 2011 15:39:14 +0200 fabien.marteau@armadeus.com wrote:

> From: Fabien Marteau <fabien.marteau@armadeus.com>
> 
> 
> Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
> ---
>  drivers/hwmon/Kconfig  |   10 ++
>  drivers/hwmon/Makefile |    1 +
>  drivers/hwmon/as1531.c |  297 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 308 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/as1531.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 50e40db..d2ba655 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -104,6 +104,16 @@ config SENSORS_ADCXX
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adcxx.
>  
> +config SENSORS_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

is a 12? bits

what is the character after the "12", please?

Analog to digital converter

> +		provided by Austria-Microsystems company.
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called as1531.
> +
>  config SENSORS_ADM1021
>  	tristate "Analog Devices ADM1021 and compatibles"
>  	depends on I2C


> +/*----------------------------------------------------------------------*/
> +
> +static int __devinit as1531_probe(struct spi_device *spi)
> +{
> +	struct as1531 *adc;
> +	int status;
> +	int i;
> +
> +	adc = kzalloc(sizeof(struct as1531), GFP_KERNEL);
> +	if (adc == NULL)
> +		return -ENOMEM;
> +
> +	mutex_init(&adc->lock);
> +	mutex_lock(&adc->lock);
> +
> +	dev_set_drvdata(&spi->dev, adc);
> +
> +	for (i = 0; i < 11; i++) {

s/11/ARRAY_SIZE(as1531_input)/

> +		status = device_create_file(&spi->dev,
> +						&as1531_input[i].dev_attr);
> +		if (status < 0) {
> +			dev_err(&spi->dev, "device_create_file failed.\n");
> +			goto out_err;
> +		}
> +	}



---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Guenter Roeck]

On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
> From: Fabien Marteau <fabien.marteau@armadeus.com>
> 
> 
Some description, such as "Dhis driver adds support for xxx" would be nice.

Also, I wonder if this driver belongs into hwmon in the first place. It is
a generic ADC chip with high conversion rate. iio would probably be more
appropriate and also much better in supporting high speed readings.

> Signed-off-by: Fabien Marteau <fabien.marteau@armadeus.com>
> ---
>  drivers/hwmon/Kconfig  |   10 ++
>  drivers/hwmon/Makefile |    1 +
>  drivers/hwmon/as1531.c |  297 ++++++++++++++++++++++++++++++++++++++++++++++++

Documentation/hwmon/as1531 is missing.
Do you plan to add yourself into MAINTAINERS ?

>  3 files changed, 308 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/as1531.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 50e40db..d2ba655 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -104,6 +104,16 @@ config SENSORS_ADCXX
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called adcxx.
>  
> +config SENSORS_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.
> +
Should be inserted in alphabetical order. Should depend on EXPERIMENTAL.

>  config SENSORS_ADM1021
>  	tristate "Analog Devices ADM1021 and compatibles"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 967d0ea..8a304b3 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_SENSORS_ABITUGURU3)+= abituguru3.o
>  obj-$(CONFIG_SENSORS_AD7414)	+= ad7414.o
>  obj-$(CONFIG_SENSORS_AD7418)	+= ad7418.o
>  obj-$(CONFIG_SENSORS_ADCXX)	+= adcxx.o
> +obj-$(CONFIG_SENSORS_AS1531)	+= as1531.o

Insert in alphabetical order.

>  obj-$(CONFIG_SENSORS_ADM1021)	+= adm1021.o
>  obj-$(CONFIG_SENSORS_ADM1025)	+= adm1025.o
>  obj-$(CONFIG_SENSORS_ADM1026)	+= adm1026.o
> diff --git a/drivers/hwmon/as1531.c b/drivers/hwmon/as1531.c
> new file mode 100644
> index 0000000..3384663
> --- /dev/null
> +++ b/drivers/hwmon/as1531.c
> @@ -0,0 +1,297 @@
> +/*
> + * 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/init.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/sysfs.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.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 {
> +	struct device *hwmon_dev;
> +	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];
> +
Not that it matters much, but the buffer sizes seem to be a bit on the large side.

> +	cmd_send = cmd;
> +
> +	spi_message_init(&message);
> +	memset(x, 0, sizeof x);
> +	memset(buf, 0, sizeof(buf));
> +	memset(buf_read, 0, sizeof(buf_read));
> +
Are those memsets really needed ?

> +	for (i = 0; i < 8; i++) {
> +		buf[i] = ((cmd_send & 0x80)>>7);

Missing spaces. Wonder why checkpatch doesn't complain about this anymore.

> +		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);

More missing spaces.

> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t as1531_read(struct device *dev,
> +			struct device_attribute *devattr, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct as1531 *adc = dev_get_drvdata(&spi->dev);
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	int status = 0;
> +	int ret_value;
> +	int32_t cmd;
> +
> +
Extra blank line.

> +	if (mutex_lock_interruptible(&adc->lock))
> +		return -ERESTARTSYS;
> +
> +	switch (attr->index) {
> +	case 0:
> +		cmd = AS1531_START_BIT | AS1531_CHAN0 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 1:
> +		cmd = AS1531_START_BIT | AS1531_CHAN1 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 2:
> +		cmd = AS1531_START_BIT | AS1531_CHAN2 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 3:
> +		cmd = AS1531_START_BIT | AS1531_CHAN3 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 4:
> +		cmd = AS1531_START_BIT | AS1531_CHAN4 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 5:
> +		cmd = AS1531_START_BIT | AS1531_CHAN5 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 6:
> +		cmd = AS1531_START_BIT | AS1531_CHAN6 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	case 7:
> +		cmd = AS1531_START_BIT | AS1531_CHAN7 |
> +		AS1531_RANGE_0_TO_VREF | AS1531_MODE_COM |
> +		AS1531_POWER_NORMAL;
> +		break;
> +	default:
> +		status = -EINVAL;
> +		goto out;
> +	}

The lock is really only needed here.

> +	status = as1531_message(spi, cmd, &ret_value);
> +	if (status < 0)
> +		goto out;
> +
> +	status = sprintf(buf, "%d\n", ret_value*2500/4096);

Missing spaces. The conversion seems to be arbitrary. You might want to return the raw value 
and convert via sensors.conf.

> +out:
> +	mutex_unlock(&adc->lock);
> +	return status;
> +}
> +
> +
> +static ssize_t as1531_show_min(struct device *dev,
> +		struct device_attribute *devattr, char *buf)
> +{
> +	return sprintf(buf, "0\n");
> +}
> +
> +static ssize_t as1531_show_max(struct device *dev,
> +		struct device_attribute *devattr, char *buf)
> +{
> +	return sprintf(buf, "%d\n", AS1531_MAX_VALUE);
> +}
> +

Not sure if reporting constants makes much sense. Opinions, anyone ?
Besides, AS1531_MAX_VALUE is as arbitrary as the conversion function above.

> +static ssize_t as1531_show_name(struct device *dev, struct device_attribute
> +				*devattr, char *buf)
> +{
> +	return sprintf(buf, "as1531\n");
> +}
> +
> +static struct sensor_device_attribute as1531_input[] = {
> +	SENSOR_ATTR(name, S_IRUGO, as1531_show_name, NULL, 0),
> +	SENSOR_ATTR(in_min, S_IRUGO, as1531_show_min, NULL, 0),
> +	SENSOR_ATTR(in_max, S_IRUGO, as1531_show_max, NULL, 0),

Non-standard attributes. Even if there is only one set of values reported per chip, 
reporting it as non-standard attribute doesn't provide much value. Please define 
per-sensor attributes.

> +	SENSOR_ATTR(in0_input, S_IRUGO, as1531_read, NULL, 0),
> +	SENSOR_ATTR(in1_input, S_IRUGO, as1531_read, NULL, 1),
> +	SENSOR_ATTR(in2_input, S_IRUGO, as1531_read, NULL, 2),
> +	SENSOR_ATTR(in3_input, S_IRUGO, as1531_read, NULL, 3),
> +	SENSOR_ATTR(in4_input, S_IRUGO, as1531_read, NULL, 4),
> +	SENSOR_ATTR(in5_input, S_IRUGO, as1531_read, NULL, 5),
> +	SENSOR_ATTR(in6_input, S_IRUGO, as1531_read, NULL, 6),
> +	SENSOR_ATTR(in7_input, S_IRUGO, as1531_read, NULL, 7),
> +};
> +
> +/*----------------------------------------------------------------------*/
> +
> +static int __devinit as1531_probe(struct spi_device *spi)
> +{
> +	struct as1531 *adc;
> +	int status;
> +	int i;
> +
> +	adc = kzalloc(sizeof(struct as1531), GFP_KERNEL);
> +	if (adc == NULL)
> +		return -ENOMEM;
> +
> +	mutex_init(&adc->lock);
> +	mutex_lock(&adc->lock);

Is that lock here really needed ? Should not be necessary.

> +
> +	dev_set_drvdata(&spi->dev, adc);
> +
> +	for (i = 0; i < 11; i++) {
> +		status = device_create_file(&spi->dev,
> +						&as1531_input[i].dev_attr);
> +		if (status < 0) {
> +			dev_err(&spi->dev, "device_create_file failed.\n");
> +			goto out_err;
> +		}
> +	}
> +
> +	adc->hwmon_dev = hwmon_device_register(&spi->dev);
> +	if (IS_ERR(adc->hwmon_dev)) {
> +		dev_err(&spi->dev, "hwmon_device_register failed.\n");
> +		status = PTR_ERR(adc->hwmon_dev);
> +		goto out_err;
> +	}
> +
> +	mutex_unlock(&adc->lock);
> +	return 0;
> +
> +out_err:
> +	for (i--; i >= 0; i--)
> +		device_remove_file(&spi->dev, &as1531_input[i].dev_attr);
> +	dev_set_drvdata(&spi->dev, NULL);
> +	mutex_unlock(&adc->lock);
> +	kfree(adc);
> +	return status;
> +}
> +
> +static int __devexit as1531_remove(struct spi_device *spi)
> +{
> +	struct as1531 *adc = dev_get_drvdata(&spi->dev);
> +	int i;
> +
> +	mutex_lock(&adc->lock);

Lock should not be needed here. If anything, it provides a false sense 
of security: Anything else waiting on it will be surprised to notice that 
the mutex (and memory) it is waiting on has been freed.
In other words, if this lock is really needed, you are in big trouble.

> +	hwmon_device_unregister(adc->hwmon_dev);
> +	for (i = 0; i < 8; i++)
> +		device_remove_file(&spi->dev, &as1531_input[i].dev_attr);
> +
> +	dev_set_drvdata(&spi->dev, NULL);
> +	mutex_unlock(&adc->lock);
> +	kfree(adc);
> +
> +	return 0;
> +}
> +
> +/* SPI structures */
> +
> +static struct spi_driver as1531_driver = {
> +	.driver = {
> +		.name	= "as1531",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe	= as1531_probe,
> +	.remove	= __devexit_p(as1531_remove),
> +};
> +
> +/* Init module */
> +
> +static int __init init_as1531(void)
> +{
> +	return spi_register_driver(&as1531_driver);
> +}
> +
> +static void __exit exit_as1531(void)
> +{
> +	spi_unregister_driver(&as1531_driver);
> +}
> +
> +module_init(init_as1531);
> +module_exit(exit_as1531);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Fabien Marteau <fabien.marteau@armadeus.com>");
> +MODULE_DESCRIPTION("Driver for AS1531 ADC");
> -- 
> 1.7.0.4
> 

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Fabien Marteau]

Hi Guenter,

Thanks for the review.

On 16/05/2011 17:39, Guenter Roeck wrote:
> On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
>> From: Fabien Marteau <fabien.marteau@armadeus.com>
>>
>>
> Some description, such as "Dhis driver adds support for xxx" would be nice.
> 
> Also, I wonder if this driver belongs into hwmon in the first place. It is
> a generic ADC chip with high conversion rate. iio would probably be more
> appropriate and also much better in supporting high speed readings.
I provided this driver "as is" because it's a driver that work well on
our platform. I thought that iio was not stable enough driver framework
to be used.
I can rewrite it under iio framework but I have no time for this moment
to do that. You think it's better to wait for an iio driver or to
continue commiting this ?

regards,
Fabien M

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Jonathan Cameron]

On 05/17/11 08:06, Fabien Marteau wrote:
> Hi Guenter,
> 
> Thanks for the review.
> 
> On 16/05/2011 17:39, Guenter Roeck wrote:
>> On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
>>> From: Fabien Marteau <fabien.marteau@armadeus.com>
>>>
>>>
>> Some description, such as "Dhis driver adds support for xxx" would be nice.
>>
>> Also, I wonder if this driver belongs into hwmon in the first place. It is
>> a generic ADC chip with high conversion rate. iio would probably be more
>> appropriate and also much better in supporting high speed readings.
> I provided this driver "as is" because it's a driver that work well on
> our platform. I thought that iio was not stable enough driver framework
> to be used.
> I can rewrite it under iio framework but I have no time for this moment
> to do that. You think it's better to wait for an iio driver or to
> continue commiting this ?
I'd say that if you primary use is hwmon, put it there for now and we can think
about moving it at a later date depending on how people are actually using it.
Guenter, would that be ok for you?

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Jonathan Cameron]

On 05/17/11 10:25, Jonathan Cameron wrote:
> On 05/17/11 08:06, Fabien Marteau wrote:
>> Hi Guenter,
>>
>> Thanks for the review.
>>
>> On 16/05/2011 17:39, Guenter Roeck wrote:
>>> On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
>>>> From: Fabien Marteau <fabien.marteau@armadeus.com>
>>>>
>>>>
>>> Some description, such as "Dhis driver adds support for xxx" would be nice.
>>>
>>> Also, I wonder if this driver belongs into hwmon in the first place. It is
>>> a generic ADC chip with high conversion rate. iio would probably be more
>>> appropriate and also much better in supporting high speed readings.
>> I provided this driver "as is" because it's a driver that work well on
>> our platform. I thought that iio was not stable enough driver framework
>> to be used.
>> I can rewrite it under iio framework but I have no time for this moment
>> to do that. You think it's better to wait for an iio driver or to
>> continue commiting this ?
> I'd say that if you primary use is hwmon, put it there for now and we can think
> about moving it at a later date depending on how people are actually using it.
> Guenter, would that be ok for you?
Having actually taken a look at the code, it's straight forward, so if you
are using it as a general purpose adc then I'm happy to port to IIO
sometimes soonish...  Will need some testing at somepoint though.

Jonathan

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Fabien Marteau]

On 17/05/2011 11:34, Jonathan Cameron wrote:
> On 05/17/11 10:25, Jonathan Cameron wrote:
>> On 05/17/11 08:06, Fabien Marteau wrote:
>>> Hi Guenter,
>>>
>>> Thanks for the review.
>>>
>>> On 16/05/2011 17:39, Guenter Roeck wrote:
>>>> On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
>>>>> From: Fabien Marteau <fabien.marteau@armadeus.com>
>>>>>
>>>>>
>>>> Some description, such as "Dhis driver adds support for xxx" would be nice.
>>>>
>>>> Also, I wonder if this driver belongs into hwmon in the first place. It is
>>>> a generic ADC chip with high conversion rate. iio would probably be more
>>>> appropriate and also much better in supporting high speed readings.
>>> I provided this driver "as is" because it's a driver that work well on
>>> our platform. I thought that iio was not stable enough driver framework
>>> to be used.
>>> I can rewrite it under iio framework but I have no time for this moment
>>> to do that. You think it's better to wait for an iio driver or to
>>> continue commiting this ?
>> I'd say that if you primary use is hwmon, put it there for now and we can think
>> about moving it at a later date depending on how people are actually using it.
>> Guenter, would that be ok for you?
> Having actually taken a look at the code, it's straight forward, so if you
> are using it as a general purpose adc then I'm happy to port to IIO
> sometimes soonish...  Will need some testing at somepoint though.
I've got it on my platform, then I can test it without problem if you want.

Fabien
> 
> Jonathan
> 

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Guenter Roeck]

On Tue, May 17, 2011 at 05:25:01AM -0400, Jonathan Cameron wrote:
> On 05/17/11 08:06, Fabien Marteau wrote:
> > Hi Guenter,
> > 
> > Thanks for the review.
> > 
> > On 16/05/2011 17:39, Guenter Roeck wrote:
> >> On Mon, May 16, 2011 at 09:39:14AM -0400, fabien.marteau@armadeus.com wrote:
> >>> From: Fabien Marteau <fabien.marteau@armadeus.com>
> >>>
> >>>
> >> Some description, such as "Dhis driver adds support for xxx" would be nice.
> >>
> >> Also, I wonder if this driver belongs into hwmon in the first place. It is
> >> a generic ADC chip with high conversion rate. iio would probably be more
> >> appropriate and also much better in supporting high speed readings.
> > I provided this driver "as is" because it's a driver that work well on
> > our platform. I thought that iio was not stable enough driver framework
> > to be used.
> > I can rewrite it under iio framework but I have no time for this moment
> > to do that. You think it's better to wait for an iio driver or to
> > continue commiting this ?
> I'd say that if you primary use is hwmon, put it there for now and we can think
> about moving it at a later date depending on how people are actually using it.
> Guenter, would that be ok for you?

Yes, that would be ok.

Thanks,
Guenter

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Jonathan Cameron]

On 05/16/11 14:39, fabien.marteau@armadeus.com wrote:
> From: Fabien Marteau <fabien.marteau@armadeus.com>
> 
Had a few minutes so done a quick port to IIO with minimal code changes.
Now looking at cleaning things up so actually reading the code rather
than cut and paste.  The message is somewhat 'novel'. 

...

So we have a spi device that only talks in 1 bit words?
1) I'm amazed any spi controllers support that.
2) Doesn't look necessary from the data sheet google furnished me with.

Looks like it will quite happily work with 8 bit transfers. Suggested replacement code below.
> +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_message(struct spi_device *spi, int cmd, u16 *ret_value)
{
	int status;
	u8 cmd_send = cmd;
	struct spi_message message;
	struct spi_transfer x[2] = {
		{
			.len = 1,
			/* this should be default anyway - so could drop */
			.bits_per_word = 8,
			/* This should be set in board config really */
			.speed_hz = AS1531_SPI_SPEED,
			.rx_buf = &cmd_send,
		}, {
			.len = 2,
			.bits_per_word = 8,
			.speed_hz = AS1531_SPI_SPEED,
			.tx_buf = ret_value,
		}
	};

	spi_message_init(&message);
	spi_message_add_tail(&x[0], &message);
	spi_message_add_tail(&x[1], &message);

	status = spi_sync(spi, &message);
	if (status < 0)
		return status;

	*ret_value = (be16_to_cpup(ret_value) >> 2) & 0xFFF;

	return 0;
}

Obviously also requires changing the type of the element passed
as ret_value.  Could set the bits_per_word of the second transfer
to 16 and avoid the endianness conversion, but I'm not sure how
many spi masters support that.

Jonathan

Re: [PATCH] hwmon: Driver for as1531, Austria-Microsystem Analog to Digital Converter.

[Jonathan Cameron]

On 05/18/11 14:09, Jonathan Cameron wrote:
> On 05/16/11 14:39, fabien.marteau@armadeus.com wrote:
>> From: Fabien Marteau <fabien.marteau@armadeus.com>
>>
> Had a few minutes so done a quick port to IIO with minimal code changes.
> Now looking at cleaning things up so actually reading the code rather
> than cut and paste.  The message is somewhat 'novel'. 
> 
> ...
> 
> So we have a spi device that only talks in 1 bit words?
> 1) I'm amazed any spi controllers support that.
> 2) Doesn't look necessary from the data sheet google furnished me with.
> 
> Looks like it will quite happily work with 8 bit transfers. Suggested replacement code below.
>> +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;
>> +}

Or skip the below and use spi_w8r16. This isn't a fast capture route anyway.
> static int as1531_message(struct spi_device *spi, int cmd, u16 *ret_value)
> {
> 	int status;
> 	u8 cmd_send = cmd;
> 	struct spi_message message;
> 	struct spi_transfer x[2] = {
> 		{
> 			.len = 1,
> 			/* this should be default anyway - so could drop */
> 			.bits_per_word = 8,
> 			/* This should be set in board config really */
> 			.speed_hz = AS1531_SPI_SPEED,
> 			.rx_buf = &cmd_send,
> 		}, {
> 			.len = 2,
> 			.bits_per_word = 8,
> 			.speed_hz = AS1531_SPI_SPEED,
> 			.tx_buf = ret_value,
> 		}
> 	};
> 
> 	spi_message_init(&message);
> 	spi_message_add_tail(&x[0], &message);
> 	spi_message_add_tail(&x[1], &message);
> 
> 	status = spi_sync(spi, &message);
> 	if (status < 0)
> 		return status;
> 
> 	*ret_value = (be16_to_cpup(ret_value) >> 2) & 0xFFF;
> 
> 	return 0;
> }
> 
> Obviously also requires changing the type of the element passed
> as ret_value.  Could set the bits_per_word of the second transfer
> to 16 and avoid the endianness conversion, but I'm not sure how
> many spi masters support that.
> 
> Jonathan