All of lore.kernel.org
 help / color / mirror / Atom feed
* [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
@ 2010-03-20 22:29 Paul Thomas
  2010-03-31 14:52 ` Jonathan Cameron
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Paul Thomas @ 2010-03-20 22:29 UTC (permalink / raw)
  To: lm-sensors

return -ENODEV if spi is ok, but you can't read OSC reg
removed most dev_dbg
passes checkpatch

Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
---
 drivers/hwmon/Kconfig   |    9 ++
 drivers/hwmon/Makefile  |    1 +
 drivers/hwmon/ads7871.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 263 insertions(+), 0 deletions(-)
 create mode 100644 drivers/hwmon/ads7871.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 68cf877..36a12d7 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -792,6 +792,15 @@ config SENSORS_ADS7828
 	  This driver can also be built as a module.  If so, the module
 	  will be called ads7828.
 
+config SENSORS_ADS7871
+	tristate "Texas Instruments ADS7871 A/D converter"
+	depends on SPI
+	help
+	  If you say yes here you get support for TI ADS7871 & ADS7870
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ads7871.
+
 config SENSORS_AMC6821
 	tristate "Texas Instruments AMC6821"
 	depends on I2C  && EXPERIMENTAL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 4bc215c..84e65a5 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
 obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
 obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
 obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
+obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
 obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
 obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
 obj-$(CONFIG_SENSORS_ADT7473)	+= adt7473.o
diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
new file mode 100644
index 0000000..4d0ae10
--- /dev/null
+++ b/drivers/hwmon/ads7871.c
@@ -0,0 +1,253 @@
+/*
+ *  ads7871 - driver for TI ADS7871 A/D converter
+ *
+ *  Copyright (c) 2010 Paul Thomas <pthomas8589@gmail.com>
+ *
+ *  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.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 or
+ *  later as publishhed by the Free Software Foundation.
+ *
+ *	You need to have something like this in struct spi_board_info
+ *	{
+ *		.modalias	= "ads7871",
+ *		.max_speed_hz	= 2*1000*1000,
+ *		.chip_select	= 0,
+ *		.bus_num	= 1,
+ *	},
+ */
+
+/*From figure 18 in the datasheet*/
+/*Register addresses*/
+#define REG_LS_BYTE	0 /*A/D Output Data, LS Byte*/
+#define REG_MS_BYTE	1 /*A/D Output Data, MS Byte*/
+#define REG_PGA_VALID	2 /*PGA Valid Register*/
+#define REG_AD_CONTROL	3 /*A/D Control Register*/
+#define REG_GAIN_MUX	4 /*Gain/Mux Register*/
+#define REG_IO_STATE	5 /*Digital I/O State Register*/
+#define REG_IO_CONTROL	6 /*Digital I/O Control Register*/
+#define REG_OSC_CONTROL	7 /*Rev/Oscillator Control Register*/
+#define REG_SER_CONTROL 24 /*Serial Interface Control Register*/
+#define REG_ID		31 /*ID Register*/
+
+/*From figure 17 in the datasheet
+* These bits get ORed with the address to form
+* the instruction byte */
+/*Instruction Bit masks*/
+#define INST_MODE_bm	(1<<7)
+#define INST_READ_bm	(1<<6)
+#define INST_16BIT_bm	(1<<5)
+
+/*From figure 18 in the datasheet*/
+/*bit masks for Rev/Oscillator Control Register*/
+#define MUX_CNV_bv	7
+#define MUX_CNV_bm	(1<<MUX_CNV_bv)
+#define MUX_M3_bm	(1<<3) /*M3 selects single ended*/
+#define MUX_G_bv	4 /*allows for reg = (gain << MUX_G_bv) | ...*/
+
+/*From figure 18 in the datasheet*/
+/*bit masks for Rev/Oscillator Control Register*/
+#define OSC_OSCR_bm	(1<<5)
+#define OSC_OSCE_bm	(1<<4)
+#define OSC_REFE_bm	(1<<3)
+#define OSC_BUFE_bm	(1<<2)
+#define OSC_R2V_bm	(1<<1)
+#define OSC_RBG_bm	(1<<0)
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+
+#define DEVICE_NAME 	"ads7871"
+
+struct ads7871_data {
+	struct device	*hwmon_dev;
+	struct mutex	update_lock;
+};
+
+static int ads7871_read_reg8(struct spi_device *spi, int reg)
+{
+	int ret;
+	reg = reg | INST_READ_bm;
+	ret = spi_w8r8(spi, reg);
+	return ret;
+}
+
+static int ads7871_read_reg16(struct spi_device *spi, int reg)
+{
+	int ret;
+	reg = reg | INST_READ_bm | INST_16BIT_bm;
+	ret = spi_w8r16(spi, reg);
+	return ret;
+}
+
+static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
+{
+	u8 tmp[2] = {reg, val};
+	return spi_write(spi, tmp, sizeof(tmp));
+}
+
+static ssize_t show_voltage(struct device *dev,
+		struct device_attribute *da, char *buf)
+{
+	struct spi_device *spi = to_spi_device(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	int ret, val, i = 0;
+	uint8_t channel, mux_cnv;
+
+	channel = attr->index;
+	/*TODO: add support for conversions
+	 *other than single ended with a gain of 1*/
+	/*MUX_M3_bm forces single ended*/
+	/*This is also where the gain of the PGA would be set*/
+	ads7871_write_reg8(spi, REG_GAIN_MUX,
+		(MUX_CNV_bm | MUX_M3_bm | channel));
+
+	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
+	mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
+	/*on 400MHz arm9 platform the conversion
+	 *is already done when we do this test*/
+	while ((i < 2) && mux_cnv) {
+		i++;
+		ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
+		mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
+		msleep_interruptible(1);
+	}
+
+	if (mux_cnv = 0) {
+		val = ads7871_read_reg16(spi, REG_LS_BYTE);
+		/*result in volts*10000 = (val/8192)*2.5*10000*/
+		val = ((val>>2) * 25000) / 8192;
+		return sprintf(buf, "%d\n", val);
+	} else {
+		return -1;
+	}
+}
+
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_voltage, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_voltage, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_voltage, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_voltage, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_voltage, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_voltage, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, show_voltage, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, show_voltage, NULL, 7);
+
+static struct attribute *ads7871_attributes[] = {
+	&sensor_dev_attr_in0_input.dev_attr.attr,
+	&sensor_dev_attr_in1_input.dev_attr.attr,
+	&sensor_dev_attr_in2_input.dev_attr.attr,
+	&sensor_dev_attr_in3_input.dev_attr.attr,
+	&sensor_dev_attr_in4_input.dev_attr.attr,
+	&sensor_dev_attr_in5_input.dev_attr.attr,
+	&sensor_dev_attr_in6_input.dev_attr.attr,
+	&sensor_dev_attr_in7_input.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group ads7871_group = {
+	.attrs = ads7871_attributes,
+};
+
+static int __devinit ads7871_probe(struct spi_device *spi)
+{
+	int status, ret, err = 0;
+	uint8_t val;
+	struct ads7871_data *pdata;
+
+	dev_dbg(&spi->dev, "probe\n");
+
+	pdata = kzalloc(sizeof(struct ads7871_data), GFP_KERNEL);
+	if (!pdata) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	status = sysfs_create_group(&spi->dev.kobj, &ads7871_group);
+	if (status < 0)
+		goto error_free;
+
+	pdata->hwmon_dev = hwmon_device_register(&spi->dev);
+	if (IS_ERR(pdata->hwmon_dev)) {
+		err = PTR_ERR(pdata->hwmon_dev);
+		goto error_remove;
+	}
+
+	spi_set_drvdata(spi, pdata);
+
+	/* Configure the SPI bus */
+	spi->mode = (SPI_MODE_0);
+	spi->bits_per_word = 8;
+	spi_setup(spi);
+
+	ads7871_write_reg8(spi, REG_SER_CONTROL, 0);
+	ads7871_write_reg8(spi, REG_AD_CONTROL, 0);
+
+	val = (OSC_OSCR_bm | OSC_OSCE_bm | OSC_REFE_bm | OSC_BUFE_bm);
+	ads7871_write_reg8(spi, REG_OSC_CONTROL, val);
+	ret = ads7871_read_reg8(spi, REG_OSC_CONTROL);
+
+	dev_dbg(&spi->dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
+	/*because there is no other error checking on an SPI bus
+	we need to make sure we really have a chip*/
+	if (val != ret) {
+		err = -ENODEV;
+		goto error_remove;
+	}
+
+	return 0;
+
+error_remove:
+	sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
+error_free:
+	kfree(pdata);
+exit:
+	return err;
+}
+
+static int __devexit ads7871_remove(struct spi_device *spi)
+{
+	struct ads7871_data *pdata = spi_get_drvdata(spi);
+
+	hwmon_device_unregister(pdata->hwmon_dev);
+	sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
+	kfree(pdata);
+	return 0;
+}
+
+static struct spi_driver ads7871_driver = {
+	.driver = {
+		.name = DEVICE_NAME,
+		.bus = &spi_bus_type,
+		.owner = THIS_MODULE,
+	},
+
+	.probe = ads7871_probe,
+	.remove = __devexit_p(ads7871_remove),
+};
+
+static int __init ads7871_init(void)
+{
+	return spi_register_driver(&ads7871_driver);
+}
+
+static void __exit ads7871_exit(void)
+{
+	spi_unregister_driver(&ads7871_driver);
+}
+
+module_init(ads7871_init);
+module_exit(ads7871_exit);
+
+MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
+MODULE_DESCRIPTION("TI ADS7871 A/D driver");
+MODULE_LICENSE("GPL");
-- 
1.6.2.5


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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
@ 2010-03-31 14:52 ` Jonathan Cameron
  2010-03-31 15:02 ` Paul Thomas
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2010-03-31 14:52 UTC (permalink / raw)
  To: lm-sensors

On 03/20/10 22:29, Paul Thomas wrote:
> return -ENODEV if spi is ok, but you can't read OSC reg
> removed most dev_dbg
> passes checkpatch

Now looks fine to me, though there is one bit of style that I'm
surprised gets past checkpatch....

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

There are a few things I'd like to see in addition, but
they can easily occur later in an incremental patch...
> 
> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
> ---
>  drivers/hwmon/Kconfig   |    9 ++
>  drivers/hwmon/Makefile  |    1 +
>  drivers/hwmon/ads7871.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 263 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/hwmon/ads7871.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 68cf877..36a12d7 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -792,6 +792,15 @@ config SENSORS_ADS7828
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called ads7828.
>  
> +config SENSORS_ADS7871
> +	tristate "Texas Instruments ADS7871 A/D converter"
> +	depends on SPI
> +	help
> +	  If you say yes here you get support for TI ADS7871 & ADS7870
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called ads7871.
> +
>  config SENSORS_AMC6821
>  	tristate "Texas Instruments AMC6821"
>  	depends on I2C  && EXPERIMENTAL
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 4bc215c..84e65a5 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -29,6 +29,7 @@ obj-$(CONFIG_SENSORS_ADM1029)	+= adm1029.o
>  obj-$(CONFIG_SENSORS_ADM1031)	+= adm1031.o
>  obj-$(CONFIG_SENSORS_ADM9240)	+= adm9240.o
>  obj-$(CONFIG_SENSORS_ADS7828)	+= ads7828.o
> +obj-$(CONFIG_SENSORS_ADS7871)	+= ads7871.o
>  obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
>  obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
>  obj-$(CONFIG_SENSORS_ADT7473)	+= adt7473.o
> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
> new file mode 100644
> index 0000000..4d0ae10
> --- /dev/null
> +++ b/drivers/hwmon/ads7871.c
> @@ -0,0 +1,253 @@
> +/*
> + *  ads7871 - driver for TI ADS7871 A/D converter
> + *
> + *  Copyright (c) 2010 Paul Thomas <pthomas8589@gmail.com>
> + *
> + *  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.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 or
> + *  later as publishhed by the Free Software Foundation.
> + *
> + *	You need to have something like this in struct spi_board_info
> + *	{
> + *		.modalias	= "ads7871",
> + *		.max_speed_hz	= 2*1000*1000,
> + *		.chip_select	= 0,
> + *		.bus_num	= 1,
> + *	},
> + */
> +
> +/*From figure 18 in the datasheet*/
> +/*Register addresses*/
> +#define REG_LS_BYTE	0 /*A/D Output Data, LS Byte*/
> +#define REG_MS_BYTE	1 /*A/D Output Data, MS Byte*/
> +#define REG_PGA_VALID	2 /*PGA Valid Register*/
> +#define REG_AD_CONTROL	3 /*A/D Control Register*/
> +#define REG_GAIN_MUX	4 /*Gain/Mux Register*/
> +#define REG_IO_STATE	5 /*Digital I/O State Register*/
> +#define REG_IO_CONTROL	6 /*Digital I/O Control Register*/
> +#define REG_OSC_CONTROL	7 /*Rev/Oscillator Control Register*/
> +#define REG_SER_CONTROL 24 /*Serial Interface Control Register*/
> +#define REG_ID		31 /*ID Register*/
> +
> +/*From figure 17 in the datasheet
> +* These bits get ORed with the address to form
> +* the instruction byte */
> +/*Instruction Bit masks*/
> +#define INST_MODE_bm	(1<<7)
> +#define INST_READ_bm	(1<<6)
> +#define INST_16BIT_bm	(1<<5)
> +
> +/*From figure 18 in the datasheet*/
> +/*bit masks for Rev/Oscillator Control Register*/
> +#define MUX_CNV_bv	7
> +#define MUX_CNV_bm	(1<<MUX_CNV_bv)
> +#define MUX_M3_bm	(1<<3) /*M3 selects single ended*/
> +#define MUX_G_bv	4 /*allows for reg = (gain << MUX_G_bv) | ...*/
> +
> +/*From figure 18 in the datasheet*/
> +/*bit masks for Rev/Oscillator Control Register*/
> +#define OSC_OSCR_bm	(1<<5)
> +#define OSC_OSCE_bm	(1<<4)
> +	dev_dbg(&spi->dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
> +#define OSC_REFE_bm	(1<<3)
> +#define OSC_BUFE_bm	(1<<2)
> +#define OSC_R2V_bm	(1<<1)
> +#define OSC_RBG_bm	(1<<0)
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/err.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +
> +#define DEVICE_NAME 	"ads7871"
> +
> +struct ads7871_data {
> +	struct device	*hwmon_dev;
> +	struct mutex	update_lock;
> +};
> +
> +static int ads7871_read_reg8(struct spi_device *spi, int reg)
> +{
> +	int ret;
> +	reg = reg | INST_READ_bm;
> +	ret = spi_w8r8(spi, reg);
> +	return ret;
> +}
> +
> +static int ads7871_read_reg16(struct spi_device *spi, int reg)
> +{
> +	int ret;
> +	reg = reg | INST_READ_bm | INST_16BIT_bm;
> +	ret = spi_w8r16(spi, reg);
> +	return ret;
> +}
> +
> +static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
> +{
> +	u8 tmp[2] = {reg, val};
> +	return spi_write(spi, tmp, sizeof(tmp));
> +}
> +
> +static ssize_t show_voltage(struct device *dev,
> +		struct device_attribute *da, char *buf)
> +{
> +	struct spi_device *spi = to_spi_device(dev);
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
> +	int ret, val, i = 0;
> +	uint8_t channel, mux_cnv;
> +
> +	channel = attr->index;
> +	/*TODO: add support for conversions
> +	 *other than single ended with a gain of 1*/
> +	/*MUX_M3_bm forces single ended*/
> +	/*This is also where the gain of the PGA would be set*/
> +	ads7871_write_reg8(spi, REG_GAIN_MUX,
> +		(MUX_CNV_bm | MUX_M3_bm | channel));
> +
> +	ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
> +	mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
> +	/*on 400MHz arm9 platform the conversion
> +	 *is already done when we do this test*/
> +	while ((i < 2) && mux_cnv) {
> +		i++;
> +		ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
> +		mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
> +		msleep_interruptible(1);
> +	}
> +
> +	if (mux_cnv = 0) {
> +		val = ads7871_read_reg16(spi, REG_LS_BYTE);
> +		/*result in volts*10000 = (val/8192)*2.5*10000*/
> +		val = ((val>>2) * 25000) / 8192;
> +		return sprintf(buf, "%d\n", val);
> +	} else {
This shouldn't have gotten through check patch.  You never have 
brackets round a single line like this.  Should be,

} else
	return -1;

> +		return -1;
> +	}
> +}
> +
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_voltage, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_voltage, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_voltage, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_voltage, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_voltage, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_voltage, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, show_voltage, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, show_voltage, NULL, 7);
> +
> +static struct attribute *ads7871_attributes[] = {
> +	&sensor_dev_attr_in0_input.dev_attr.attr,
> +	&sensor_dev_attr_in1_input.dev_attr.attr,
> +	&sensor_dev_attr_in2_input.dev_attr.attr,
> +	&sensor_dev_attr_in3_input.dev_attr.attr,
> +	&sensor_dev_attr_in4_input.dev_attr.attr,
> +	&sensor_dev_attr_in5_input.dev_attr.attr,
> +	&sensor_dev_attr_in6_input.dev_attr.attr,
> +	&sensor_dev_attr_in7_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group ads7871_group = {
> +	.attrs = ads7871_attributes,
> +};
> +
> +static int __devinit ads7871_probe(struct spi_device *spi)
> +{
> +	int status, ret, err = 0;
> +	uint8_t val;
> +	struct ads7871_data *pdata;
> +
There is still a lot of debug code in here that is useful for development
but probably doesn't want to be in the final driver.
> +	dev_dbg(&spi->dev, "probe\n");
> +
sizeof(*pdata) is neater.
> +	pdata = kzalloc(sizeof(struct ads7871_data), GFP_KERNEL);
> +	if (!pdata) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	status = sysfs_create_group(&spi->dev.kobj, &ads7871_group);
> +	if (status < 0)
> +		goto error_free;
> +
> +	pdata->hwmon_dev = hwmon_device_register(&spi->dev);
> +	if (IS_ERR(pdata->hwmon_dev)) {
> +		err = PTR_ERR(pdata->hwmon_dev);
> +		goto error_remove;
> +	}
> +
> +	spi_set_drvdata(spi, pdata);
> +
> +	/* Configure the SPI bus */
> +	spi->mode = (SPI_MODE_0);
> +	spi->bits_per_word = 8;
> +	spi_setup(spi);
> +
> +	ads7871_write_reg8(spi, REG_SER_CONTROL, 0);
> +	ads7871_write_reg8(spi, REG_AD_CONTROL, 0);
> +
> +	val = (OSC_OSCR_bm | OSC_OSCE_bm | OSC_REFE_bm | OSC_BUFE_bm);
> +	ads7871_write_reg8(spi, REG_OSC_CONTROL, val);
> +	ret = ads7871_read_reg8(spi, REG_OSC_CONTROL);
> +
> +	dev_dbg(&spi->dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
> +	/*because there is no other error checking on an SPI bus
> +	we need to make sure we really have a chip*/
> +	if (val != ret) {
> +		err = -ENODEV;
> +		goto error_remove;
> +	}
> +
> +	return 0;
> +
> +error_remove:
> +	sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
> +error_free:
> +	kfree(pdata);
> +exit:
> +	return err;
> +}
> +
> +static int __devexit ads7871_remove(struct spi_device *spi)
> +{
> +	struct ads7871_data *pdata = spi_get_drvdata(spi);
> +
> +	hwmon_device_unregister(pdata->hwmon_dev);
> +	sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
> +	kfree(pdata);
> +	return 0;
> +}
> +
> +static struct spi_driver ads7871_driver = {
> +	.driver = {
> +		.name = DEVICE_NAME,
> +		.bus = &spi_bus_type,
> +		.owner = THIS_MODULE,
> +	},
> +
> +	.probe = ads7871_probe,
> +	.remove = __devexit_p(ads7871_remove),
> +};

The driver supports two devices (as per description),
hence it would be nice to use a device table to allow
platform support code to specify which actually have.

> +
> +static int __init ads7871_init(void)
> +{
> +	return spi_register_driver(&ads7871_driver);
> +}
> +
> +static void __exit ads7871_exit(void)
> +{
> +	spi_unregister_driver(&ads7871_driver);
> +}
> +
> +module_init(ads7871_init);
> +module_exit(ads7871_exit);
> +
> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
> +MODULE_DESCRIPTION("TI ADS7871 A/D driver");
> +MODULE_LICENSE("GPL");


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
  2010-03-31 14:52 ` Jonathan Cameron
@ 2010-03-31 15:02 ` Paul Thomas
  2010-03-31 15:45 ` Jean Delvare
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Paul Thomas @ 2010-03-31 15:02 UTC (permalink / raw)
  To: lm-sensors

Great, just a couple of responses.

On Wed, Mar 31, 2010 at 7:52 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 03/20/10 22:29, Paul Thomas wrote:
>> return -ENODEV if spi is ok, but you can't read OSC reg
>> removed most dev_dbg
>> passes checkpatch
>
> Now looks fine to me, though there is one bit of style that I'm
> surprised gets past checkpatch....
>
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>
> There are a few things I'd like to see in addition, but
> they can easily occur later in an incremental patch...
>>
>> Signed-off-by: Paul Thomas <pthomas8589@gmail.com>
>> ---
>>  drivers/hwmon/Kconfig   |    9 ++
>>  drivers/hwmon/Makefile  |    1 +
>>  drivers/hwmon/ads7871.c |  253 +++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 263 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/hwmon/ads7871.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 68cf877..36a12d7 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -792,6 +792,15 @@ config SENSORS_ADS7828
>>         This driver can also be built as a module.  If so, the module
>>         will be called ads7828.
>>
>> +config SENSORS_ADS7871
>> +     tristate "Texas Instruments ADS7871 A/D converter"
>> +     depends on SPI
>> +     help
>> +       If you say yes here you get support for TI ADS7871 & ADS7870
>> +
>> +       This driver can also be built as a module.  If so, the module
>> +       will be called ads7871.
>> +
>>  config SENSORS_AMC6821
>>       tristate "Texas Instruments AMC6821"
>>       depends on I2C  && EXPERIMENTAL
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 4bc215c..84e65a5 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_SENSORS_ADM1029)       += adm1029.o
>>  obj-$(CONFIG_SENSORS_ADM1031)        += adm1031.o
>>  obj-$(CONFIG_SENSORS_ADM9240)        += adm9240.o
>>  obj-$(CONFIG_SENSORS_ADS7828)        += ads7828.o
>> +obj-$(CONFIG_SENSORS_ADS7871)        += ads7871.o
>>  obj-$(CONFIG_SENSORS_ADT7462)        += adt7462.o
>>  obj-$(CONFIG_SENSORS_ADT7470)        += adt7470.o
>>  obj-$(CONFIG_SENSORS_ADT7473)        += adt7473.o
>> diff --git a/drivers/hwmon/ads7871.c b/drivers/hwmon/ads7871.c
>> new file mode 100644
>> index 0000000..4d0ae10
>> --- /dev/null
>> +++ b/drivers/hwmon/ads7871.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + *  ads7871 - driver for TI ADS7871 A/D converter
>> + *
>> + *  Copyright (c) 2010 Paul Thomas <pthomas8589@gmail.com>
>> + *
>> + *  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.
>> + *
>> + *  This program is free software; you can redistribute it and/or modify
>> + *  it under the terms of the GNU General Public License version 2 or
>> + *  later as publishhed by the Free Software Foundation.
>> + *
>> + *   You need to have something like this in struct spi_board_info
>> + *   {
>> + *           .modalias       = "ads7871",
>> + *           .max_speed_hz   = 2*1000*1000,
>> + *           .chip_select    = 0,
>> + *           .bus_num        = 1,
>> + *   },
>> + */
>> +
>> +/*From figure 18 in the datasheet*/
>> +/*Register addresses*/
>> +#define REG_LS_BYTE  0 /*A/D Output Data, LS Byte*/
>> +#define REG_MS_BYTE  1 /*A/D Output Data, MS Byte*/
>> +#define REG_PGA_VALID        2 /*PGA Valid Register*/
>> +#define REG_AD_CONTROL       3 /*A/D Control Register*/
>> +#define REG_GAIN_MUX 4 /*Gain/Mux Register*/
>> +#define REG_IO_STATE 5 /*Digital I/O State Register*/
>> +#define REG_IO_CONTROL       6 /*Digital I/O Control Register*/
>> +#define REG_OSC_CONTROL      7 /*Rev/Oscillator Control Register*/
>> +#define REG_SER_CONTROL 24 /*Serial Interface Control Register*/
>> +#define REG_ID               31 /*ID Register*/
>> +
>> +/*From figure 17 in the datasheet
>> +* These bits get ORed with the address to form
>> +* the instruction byte */
>> +/*Instruction Bit masks*/
>> +#define INST_MODE_bm (1<<7)
>> +#define INST_READ_bm (1<<6)
>> +#define INST_16BIT_bm        (1<<5)
>> +
>> +/*From figure 18 in the datasheet*/
>> +/*bit masks for Rev/Oscillator Control Register*/
>> +#define MUX_CNV_bv   7
>> +#define MUX_CNV_bm   (1<<MUX_CNV_bv)
>> +#define MUX_M3_bm    (1<<3) /*M3 selects single ended*/
>> +#define MUX_G_bv     4 /*allows for reg = (gain << MUX_G_bv) | ...*/
>> +
>> +/*From figure 18 in the datasheet*/
>> +/*bit masks for Rev/Oscillator Control Register*/
>> +#define OSC_OSCR_bm  (1<<5)
>> +#define OSC_OSCE_bm  (1<<4)
>> +     dev_dbg(&spi->dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
>> +#define OSC_REFE_bm  (1<<3)
>> +#define OSC_BUFE_bm  (1<<2)
>> +#define OSC_R2V_bm   (1<<1)
>> +#define OSC_RBG_bm   (1<<0)
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/spi/spi.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/err.h>
>> +#include <linux/mutex.h>
>> +#include <linux/delay.h>
>> +
>> +#define DEVICE_NAME  "ads7871"
>> +
>> +struct ads7871_data {
>> +     struct device   *hwmon_dev;
>> +     struct mutex    update_lock;
>> +};
>> +
>> +static int ads7871_read_reg8(struct spi_device *spi, int reg)
>> +{
>> +     int ret;
>> +     reg = reg | INST_READ_bm;
>> +     ret = spi_w8r8(spi, reg);
>> +     return ret;
>> +}
>> +
>> +static int ads7871_read_reg16(struct spi_device *spi, int reg)
>> +{
>> +     int ret;
>> +     reg = reg | INST_READ_bm | INST_16BIT_bm;
>> +     ret = spi_w8r16(spi, reg);
>> +     return ret;
>> +}
>> +
>> +static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
>> +{
>> +     u8 tmp[2] = {reg, val};
>> +     return spi_write(spi, tmp, sizeof(tmp));
>> +}
>> +
>> +static ssize_t show_voltage(struct device *dev,
>> +             struct device_attribute *da, char *buf)
>> +{
>> +     struct spi_device *spi = to_spi_device(dev);
>> +     struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>> +     int ret, val, i = 0;
>> +     uint8_t channel, mux_cnv;
>> +
>> +     channel = attr->index;
>> +     /*TODO: add support for conversions
>> +      *other than single ended with a gain of 1*/
>> +     /*MUX_M3_bm forces single ended*/
>> +     /*This is also where the gain of the PGA would be set*/
>> +     ads7871_write_reg8(spi, REG_GAIN_MUX,
>> +             (MUX_CNV_bm | MUX_M3_bm | channel));
>> +
>> +     ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
>> +     mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
>> +     /*on 400MHz arm9 platform the conversion
>> +      *is already done when we do this test*/
>> +     while ((i < 2) && mux_cnv) {
>> +             i++;
>> +             ret = ads7871_read_reg8(spi, REG_GAIN_MUX);
>> +             mux_cnv = ((ret & MUX_CNV_bm)>>MUX_CNV_bv);
>> +             msleep_interruptible(1);
>> +     }
>> +
>> +     if (mux_cnv = 0) {
>> +             val = ads7871_read_reg16(spi, REG_LS_BYTE);
>> +             /*result in volts*10000 = (val/8192)*2.5*10000*/
>> +             val = ((val>>2) * 25000) / 8192;
>> +             return sprintf(buf, "%d\n", val);
>> +     } else {
> This shouldn't have gotten through check patch.  You never have
> brackets round a single line like this.  Should be,

I re-read CodingStyle when I was running checkpatch and saw that if
the branch is part of a muli line conditional statement (line 171 in
CodingStyle) then all sections should be braes should be used.

>
> } else
>        return -1;
>
>> +             return -1;
>> +     }
>> +}
>> +
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, show_voltage, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, show_voltage, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, show_voltage, NULL, 2);
>> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, show_voltage, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, show_voltage, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, show_voltage, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, show_voltage, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, show_voltage, NULL, 7);
>> +
>> +static struct attribute *ads7871_attributes[] = {
>> +     &sensor_dev_attr_in0_input.dev_attr.attr,
>> +     &sensor_dev_attr_in1_input.dev_attr.attr,
>> +     &sensor_dev_attr_in2_input.dev_attr.attr,
>> +     &sensor_dev_attr_in3_input.dev_attr.attr,
>> +     &sensor_dev_attr_in4_input.dev_attr.attr,
>> +     &sensor_dev_attr_in5_input.dev_attr.attr,
>> +     &sensor_dev_attr_in6_input.dev_attr.attr,
>> +     &sensor_dev_attr_in7_input.dev_attr.attr,
>> +     NULL
>> +};
>> +
>> +static const struct attribute_group ads7871_group = {
>> +     .attrs = ads7871_attributes,
>> +};
>> +
>> +static int __devinit ads7871_probe(struct spi_device *spi)
>> +{
>> +     int status, ret, err = 0;
>> +     uint8_t val;
>> +     struct ads7871_data *pdata;
>> +
> There is still a lot of debug code in here that is useful for development
> but probably doesn't want to be in the final driver.
I'm happy with what you think is best, but in other driver I've been
glad to have a little bit in demesg.

>> +     dev_dbg(&spi->dev, "probe\n");
>> +
> sizeof(*pdata) is neater.
>> +     pdata = kzalloc(sizeof(struct ads7871_data), GFP_KERNEL);
>> +     if (!pdata) {
>> +             err = -ENOMEM;
>> +             goto exit;
>> +     }
>> +
>> +     status = sysfs_create_group(&spi->dev.kobj, &ads7871_group);
>> +     if (status < 0)
>> +             goto error_free;
>> +
>> +     pdata->hwmon_dev = hwmon_device_register(&spi->dev);
>> +     if (IS_ERR(pdata->hwmon_dev)) {
>> +             err = PTR_ERR(pdata->hwmon_dev);
>> +             goto error_remove;
>> +     }
>> +
>> +     spi_set_drvdata(spi, pdata);
>> +
>> +     /* Configure the SPI bus */
>> +     spi->mode = (SPI_MODE_0);
>> +     spi->bits_per_word = 8;
>> +     spi_setup(spi);
>> +
>> +     ads7871_write_reg8(spi, REG_SER_CONTROL, 0);
>> +     ads7871_write_reg8(spi, REG_AD_CONTROL, 0);
>> +
>> +     val = (OSC_OSCR_bm | OSC_OSCE_bm | OSC_REFE_bm | OSC_BUFE_bm);
>> +     ads7871_write_reg8(spi, REG_OSC_CONTROL, val);
>> +     ret = ads7871_read_reg8(spi, REG_OSC_CONTROL);
>> +
>> +     dev_dbg(&spi->dev, "REG_OSC_CONTROL write:%x, read:%x\n", val, ret);
>> +     /*because there is no other error checking on an SPI bus
>> +     we need to make sure we really have a chip*/
>> +     if (val != ret) {
>> +             err = -ENODEV;
>> +             goto error_remove;
>> +     }
>> +
>> +     return 0;
>> +
>> +error_remove:
>> +     sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
>> +error_free:
>> +     kfree(pdata);
>> +exit:
>> +     return err;
>> +}
>> +
>> +static int __devexit ads7871_remove(struct spi_device *spi)
>> +{
>> +     struct ads7871_data *pdata = spi_get_drvdata(spi);
>> +
>> +     hwmon_device_unregister(pdata->hwmon_dev);
>> +     sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
>> +     kfree(pdata);
>> +     return 0;
>> +}
>> +
>> +static struct spi_driver ads7871_driver = {
>> +     .driver = {
>> +             .name = DEVICE_NAME,
>> +             .bus = &spi_bus_type,
>> +             .owner = THIS_MODULE,
>> +     },
>> +
>> +     .probe = ads7871_probe,
>> +     .remove = __devexit_p(ads7871_remove),
>> +};
>
> The driver supports two devices (as per description),
> hence it would be nice to use a device table to allow
> platform support code to specify which actually have.
>
>> +
>> +static int __init ads7871_init(void)
>> +{
>> +     return spi_register_driver(&ads7871_driver);
>> +}
>> +
>> +static void __exit ads7871_exit(void)
>> +{
>> +     spi_unregister_driver(&ads7871_driver);
>> +}
>> +
>> +module_init(ads7871_init);
>> +module_exit(ads7871_exit);
>> +
>> +MODULE_AUTHOR("Paul Thomas <pthomas8589@gmail.com>");
>> +MODULE_DESCRIPTION("TI ADS7871 A/D driver");
>> +MODULE_LICENSE("GPL");
>
>

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
  2010-03-31 14:52 ` Jonathan Cameron
  2010-03-31 15:02 ` Paul Thomas
@ 2010-03-31 15:45 ` Jean Delvare
  2010-03-31 15:51 ` Jonathan Cameron
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Jean Delvare @ 2010-03-31 15:45 UTC (permalink / raw)
  To: lm-sensors

Hi Jonathan,

On Wed, 31 Mar 2010 15:52:57 +0100, Jonathan Cameron wrote:
> There are a few things I'd like to see in addition, but
> they can easily occur later in an incremental patch...
> > (...)
> > +	if (mux_cnv = 0) {
> > +		val = ads7871_read_reg16(spi, REG_LS_BYTE);
> > +		/*result in volts*10000 = (val/8192)*2.5*10000*/
> > +		val = ((val>>2) * 25000) / 8192;
> > +		return sprintf(buf, "%d\n", val);
> > +	} else {
> This shouldn't have gotten through check patch.  You never have 
> brackets round a single line like this.  Should be,
> 
> } else
> 	return -1;

Some developers don't like it when the if branch has braces and the
else branch doesn't, or vice-versa. For this reason, checkpatch no
longer complains about single line branches using braces as long as at
least one other branch needs braces (which is the case here.)

> sizeof(*pdata) is neater.
> > +	pdata = kzalloc(sizeof(struct ads7871_data), GFP_KERNEL);

I beg to disagree. It's really a matter of personal taste. I prefer
sizeof(struct foo) because you can't accidentally ask for the size of
the pointer that way. In the end there is no consensus, which is why
checkpatch doesn't complain either way.

-- 
Jean Delvare

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
                   ` (2 preceding siblings ...)
  2010-03-31 15:45 ` Jean Delvare
@ 2010-03-31 15:51 ` Jonathan Cameron
  2010-03-31 15:55 ` Jonathan Cameron
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2010-03-31 15:51 UTC (permalink / raw)
  To: lm-sensors

On 03/31/10 16:45, Jean Delvare wrote:
> Hi Jonathan,
> 
> On Wed, 31 Mar 2010 15:52:57 +0100, Jonathan Cameron wrote:
>> There are a few things I'd like to see in addition, but
>> they can easily occur later in an incremental patch...
>>> (...)
>>> +	if (mux_cnv = 0) {
>>> +		val = ads7871_read_reg16(spi, REG_LS_BYTE);
>>> +		/*result in volts*10000 = (val/8192)*2.5*10000*/
>>> +		val = ((val>>2) * 25000) / 8192;
>>> +		return sprintf(buf, "%d\n", val);
>>> +	} else {
>> This shouldn't have gotten through check patch.  You never have 
>> brackets round a single line like this.  Should be,
>>
>> } else
>> 	return -1;
> 
> Some developers don't like it when the if branch has braces and the
> else branch doesn't, or vice-versa. For this reason, checkpatch no
> longer complains about single line branches using braces as long as at
> least one other branch needs braces (which is the case here.)
Fair enough. I'd missed that change.
> 
>> sizeof(*pdata) is neater.
>>> +	pdata = kzalloc(sizeof(struct ads7871_data), GFP_KERNEL);
> 
> I beg to disagree. It's really a matter of personal taste. I prefer
> sizeof(struct foo) because you can't accidentally ask for the size of
> the pointer that way. In the end there is no consensus, which is why
> checkpatch doesn't complain either way.
Also fair enough.  Not one that really bothers me!



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
                   ` (3 preceding siblings ...)
  2010-03-31 15:51 ` Jonathan Cameron
@ 2010-03-31 15:55 ` Jonathan Cameron
  2010-04-05 18:57 ` Paul Thomas
  2010-04-06 12:16 ` Jonathan Cameron
  6 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2010-03-31 15:55 UTC (permalink / raw)
  To: lm-sensors

On 03/31/10 16:02, Paul Thomas wrote:
> Great, just a couple of responses.
> 

...

>>> +static const struct attribute_group ads7871_group = {
>>> +     .attrs = ads7871_attributes,
>>> +};
>>> +
>>> +static int __devinit ads7871_probe(struct spi_device *spi)
>>> +{
>>> +     int status, ret, err = 0;
>>> +     uint8_t val;
>>> +     struct ads7871_data *pdata;
>>> +
>> There is still a lot of debug code in here that is useful for development
>> but probably doesn't want to be in the final driver.
> I'm happy with what you think is best, but in other driver I've been
> glad to have a little bit in demesg.
Fair enough.  How much you report tends to be a personal decision, so
it's fine with me as is.

Jonathan

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
                   ` (4 preceding siblings ...)
  2010-03-31 15:55 ` Jonathan Cameron
@ 2010-04-05 18:57 ` Paul Thomas
  2010-04-06 12:16 ` Jonathan Cameron
  6 siblings, 0 replies; 8+ messages in thread
From: Paul Thomas @ 2010-04-05 18:57 UTC (permalink / raw)
  To: lm-sensors

On Wed, Mar 31, 2010 at 8:55 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
> On 03/31/10 16:02, Paul Thomas wrote:
>> Great, just a couple of responses.
>>
>
> ...
>
>>>> +static const struct attribute_group ads7871_group = {
>>>> +     .attrs = ads7871_attributes,
>>>> +};
>>>> +
>>>> +static int __devinit ads7871_probe(struct spi_device *spi)
>>>> +{
>>>> +     int status, ret, err = 0;
>>>> +     uint8_t val;
>>>> +     struct ads7871_data *pdata;
>>>> +
>>> There is still a lot of debug code in here that is useful for development
>>> but probably doesn't want to be in the final driver.
>> I'm happy with what you think is best, but in other driver I've been
>> glad to have a little bit in demesg.
> Fair enough.  How much you report tends to be a personal decision, so
> it's fine with me as is.
>
> Jonathan
>

Is there a staging git tree for hwmon? I see this:
http://lm-sensors.org/kernel, but both branches look old. Should I cc
another mailing list with this patch? I'm just trying to understand
how the flow between hwmon and the upstream kernel stuff works.

thanks,
Paul

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lm-sensors] [PATCH] V3, TI ads7871 a/d converter,
  2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
                   ` (5 preceding siblings ...)
  2010-04-05 18:57 ` Paul Thomas
@ 2010-04-06 12:16 ` Jonathan Cameron
  6 siblings, 0 replies; 8+ messages in thread
From: Jonathan Cameron @ 2010-04-06 12:16 UTC (permalink / raw)
  To: lm-sensors

On 04/05/10 19:57, Paul Thomas wrote:
> On Wed, Mar 31, 2010 at 8:55 AM, Jonathan Cameron <jic23@cam.ac.uk> wrote:
>> On 03/31/10 16:02, Paul Thomas wrote:
>>> Great, just a couple of responses.
>>>
>>
>> ...
>>
>>>>> +static const struct attribute_group ads7871_group = {
>>>>> +     .attrs = ads7871_attributes,
>>>>> +};
>>>>> +
>>>>> +static int __devinit ads7871_probe(struct spi_device *spi)
>>>>> +{
>>>>> +     int status, ret, err = 0;
>>>>> +     uint8_t val;
>>>>> +     struct ads7871_data *pdata;
>>>>> +
>>>> There is still a lot of debug code in here that is useful for development
>>>> but probably doesn't want to be in the final driver.
>>> I'm happy with what you think is best, but in other driver I've been
>>> glad to have a little bit in demesg.
>> Fair enough.  How much you report tends to be a personal decision, so
>> it's fine with me as is.
>>
>> Jonathan
>>
> 
> Is there a staging git tree for hwmon? I see this:
> http://lm-sensors.org/kernel, but both branches look old. Should I cc
> another mailing list with this patch? I'm just trying to understand
> how the flow between hwmon and the upstream kernel stuff works.

Sadly hwmon is without a specific maintainer at the moment.  Andrew
Morton is handling some patches in his role as maintainer of last
resort and I think Jean still takes some patches in through his tree,
though they may be mostly work on drivers he has a particular interest
in.

Jonathan






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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2010-04-06 12:16 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-20 22:29 [lm-sensors] [PATCH] V3, TI ads7871 a/d converter, Paul Thomas
2010-03-31 14:52 ` Jonathan Cameron
2010-03-31 15:02 ` Paul Thomas
2010-03-31 15:45 ` Jean Delvare
2010-03-31 15:51 ` Jonathan Cameron
2010-03-31 15:55 ` Jonathan Cameron
2010-04-05 18:57 ` Paul Thomas
2010-04-06 12:16 ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.