* 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