[lm-sensors] [PATCH] V3, TI ads7871 a/d converter, formatting cleanup

Paul Thomas pthomas8589 at gmail.com
Wed Mar 31 17:02:58 CEST 2010


Great, just a couple of responses.

On Wed, Mar 31, 2010 at 7:52 AM, Jonathan Cameron <jic23 at 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 at 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 at 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 at 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 at gmail.com>");
>> +MODULE_DESCRIPTION("TI ADS7871 A/D driver");
>> +MODULE_LICENSE("GPL");
>
>




More information about the lm-sensors mailing list