Hi Paul,
Driver itself looks fine, but there is a fair bit of code style
cleanup needing doing. Was fair enough when you sent early version
before which checkpatch wouldn't like, but now you are moving
towards a clean driver, please fix everything the script throws
up.
Note I reviewed this in reverse as it's easier to read drivers that way
hence comments might not work properly starting at the top.
Given I did the last review against the datasheet, this time
I've assumed all is fine from the what it is doing point of view.
Jonathan
> ---
> drivers/hwmon/Kconfig | 11 ++-
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/ads7871.c | 275 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 286 insertions(+), 1 deletions(-)
> create mode 100644 drivers/hwmon/ads7871.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 68cf877..f08d59f 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -169,7 +169,7 @@ config SENSORS_ADM9240
>
> This driver can also be built as a module. If so, the module
> will be called adm9240.
*cough* I think you've just inserted a white space error.
> -
> +
> config SENSORS_ADT7462
> tristate "Analog Devices ADT7462"
> depends on I2C && EXPERIMENTAL
> @@ -791,6 +791,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"
> 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..e1ae01f
> --- /dev/null
> +++ b/drivers/hwmon/ads7871.c
> @@ -0,0 +1,275 @@
> +/*
> + * ads7871 - driver for TI ADS7871 A/D converter
> + *
> + * Copyright (c) 2010 Paul Thomas <pthomas8589@xxxxxxxxx>
> + *
> + * 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,
> + * },
> + */
> +
Why is this still here?
> +#define DEBUG 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) | ...*/
> +
Turn these into a nice pretty multiline comment... I think checkpatch
will throw a wobbly about these.
/*
* From figure 18 in the datasheet
* Bit masks for re....
*/
> +/*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);
>From what I recall, spi_w8r8 etc return errors themselves. Pass this on
don't apply your own interpretation as you've just taken a possibly
specific error and converted it to a generic one.
> + if (ret < 0)
> + return -EINVAL;
> +
> + 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);
> + if (ret < 0)
> + return -EINVAL;
> +
> + return ret;
> +}
> +
> +static int ads7871_write_reg8(struct spi_device *spi, int reg, u8 val)
> +{
Cleaner to have
u8 tmp[] = {reg, val};
> + u8 tmp[2];
> + tmp[0] = reg;
> + tmp[1] = 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;
> + dev_dbg(&spi->dev, "setup REG_GAIN_MUX: channel = %d, ", channel);
> + /*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);
> + }
> + dev_dbg(&spi->dev, "ret = %x, mux_cnv = %d, i = %d\n\n", ret, mux_cnv, i);
> +
> + 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;
> +}
> +
Up to you, but maybe a local macro to do these? They are all very similar!
> +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;
> +
Get most of this debugging info out. Rather handy when writing
the driver, but not going to give info of much use chasing problems
now it is written.
> + dev_dbg(&spi->dev, "ads7871_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)
> + {
> + dev_dbg(&spi->dev, "status(sysfs_create_group) = %d\n", status);
> + 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);
> +
Style issues here. Please run whole patch through scripts/checkpatch.pl and fix
all the errors / warnings.
> + if (IS_ERR(pdata->hwmon_dev))
> + {
> + dev_dbg(&spi->dev, "hwmon_device_register: IS_ERR\n");
> + }
> +
> + /* 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)
> + {
Please use a standard error. -ENODEV perhaps?
> + err = -1;
> + 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);
> +
Not liking the triple new line! For that matter, probably not a
worth leaving this in anyway. It is trivial to tell if a device
has been removed or not without it.
> + dev_dbg(&spi->dev, "ads7871_remove\n\n\n");
> + hwmon_device_unregister(pdata->hwmon_dev);
> + sysfs_remove_group(&spi->dev.kobj, &ads7871_group);
(nitpick) Conventionally a newline before simple returns like this.
Looks like you aren't freeing 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@xxxxxxxxx>");
> +MODULE_DESCRIPTION("TI ADS7871 A/D driver");
> +MODULE_LICENSE("GPL");
_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
