[PATCH v3 1/2] hwmon: (ads7828) driver cleanup

[PATCH v3 1/2] hwmon: (ads7828) driver cleanup

[Vivien Didelot]

* Remove module parameters, add a ads7828_platform_data;
* Move driver declaration to avoid adding function prototypes;
* Remove unused macros;
* Coding Style fixes.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/hwmon/ads7828           |  31 +++--
 drivers/hwmon/ads7828.c               | 216 +++++++++++++++++-----------------
 include/linux/platform_data/ads7828.h |  29 +++++
 3 files changed, 155 insertions(+), 121 deletions(-)
 create mode 100644 include/linux/platform_data/ads7828.h

diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
index 2bbebe6..b35668c 100644
--- a/Documentation/hwmon/ads7828
+++ b/Documentation/hwmon/ads7828
@@ -5,21 +5,32 @@ Supported chips:
   * Texas Instruments/Burr-Brown ADS7828
     Prefix: 'ads7828'
     Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
-    Datasheet: Publicly available at the Texas Instruments website :
+    Datasheet: Publicly available at the Texas Instruments website:
                http://focus.ti.com/lit/ds/symlink/ads7828.pdf
 
 Authors:
         Steve Hardy <shardy@redhat.com>
 
-Module Parameters
------------------
-
-* se_input: bool (default Y)
-  Single ended operation - set to N for differential mode
-* int_vref: bool (default Y)
-  Operate with the internal 2.5V reference - set to N for external reference
-* vref_mv: int (default 2500)
-  If using an external reference, set this to the reference voltage in mV
+Platform data
+-------------
+
+The ads7828 driver accepts an optional ads7828_platform_data structure (defined
+in include/linux/platform_data/ads7828.h). If no structure is provided, the
+configuration defaults to single ended operation and internal vref (2.5V).
+
+The structure fields are:
+
+* diff_input: bool
+  Differential operation - set to true for differential mode,
+  false for default single ended mode.
+* ext_vref: bool
+  External reference - set to true if it operates with an external reference,
+  false for default internal reference.
+* vref_mv: int
+  Voltage reference - if using an external reference, set this to the reference
+  voltage in mV, otherwise, it will default to the internal value (2500mV).
+  This value will be bounded with limits accepted by the chip, described in the
+  datasheet.
 
 Description
 -----------
diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index bf3fdf4..0a13bf8 100644
--- a/drivers/hwmon/ads7828.c
+++ b/drivers/hwmon/ads7828.c
@@ -6,7 +6,7 @@
  *
  * Written by Steve Hardy <shardy@redhat.com>
  *
- * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads7828.pdf
+ * For further information, see the Documentation/hwmon/ads7828 file.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -23,63 +23,48 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
-#include <linux/module.h>
-#include <linux/init.h>
-#include <linux/slab.h>
-#include <linux/jiffies.h>
-#include <linux/i2c.h>
+#include <linux/err.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
-#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/jiffies.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/platform_data/ads7828.h>
+#include <linux/slab.h>
 
 /* The ADS7828 registers */
-#define ADS7828_NCH 8 /* 8 channels of 12-bit A-D supported */
-#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
-#define ADS7828_CMD_SD_DIFF 0x00 /* Differential inputs */
-#define ADS7828_CMD_PD0 0x0 /* Power Down between A-D conversions */
-#define ADS7828_CMD_PD1 0x04 /* Internal ref OFF && A-D ON */
-#define ADS7828_CMD_PD2 0x08 /* Internal ref ON && A-D OFF */
-#define ADS7828_CMD_PD3 0x0C /* Internal ref ON && A-D ON */
-#define ADS7828_INT_VREF_MV 2500 /* Internal vref is 2.5V, 2500mV */
+#define ADS7828_NCH		8	/* 8 channels supported */
+#define ADS7828_CMD_SD_SE	0x80	/* Single ended inputs */
+#define ADS7828_CMD_PD1		0x04	/* Internal vref OFF && A/D ON */
+#define ADS7828_CMD_PD3		0x0C	/* Internal vref ON && A/D ON */
+#define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
+#define ADS7828_EXT_VREF_MV_MIN	50	/* External vref min value 0.05V */
+#define ADS7828_EXT_VREF_MV_MAX	5250	/* External vref max value 5.25V */
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
 	I2C_CLIENT_END };
 
-/* Module parameters */
-static bool se_input = 1; /* Default is SE, 0 == diff */
-static bool int_vref = 1; /* Default is internal ref ON */
-static int vref_mv = ADS7828_INT_VREF_MV; /* set if vref != 2.5V */
-module_param(se_input, bool, S_IRUGO);
-module_param(int_vref, bool, S_IRUGO);
-module_param(vref_mv, int, S_IRUGO);
-
-/* Global Variables */
-static u8 ads7828_cmd_byte; /* cmd byte without channel bits */
-static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
-
-/* Each client has this additional data */
+/* Client specific data */
 struct ads7828_data {
 	struct device *hwmon_dev;
-	struct mutex update_lock; /* mutex protect updates */
-	char valid; /* !=0 if following fields are valid */
-	unsigned long last_updated; /* In jiffies */
-	u16 adc_input[ADS7828_NCH]; /* ADS7828_NCH 12-bit samples */
+	struct mutex update_lock;	/* Mutex protecting updates */
+	unsigned long last_updated;	/* Last updated time (in jiffies) */
+	u16 adc_input[ADS7828_NCH];	/* ADS7828_NCH samples */
+	bool valid;			/* Validity flag */
+	bool diff_input;		/* Differential input */
+	bool ext_vref;			/* External voltage reference */
+	unsigned int vref_mv;		/* voltage reference value */
+	u8 cmd_byte;			/* Command byte without channel bits */
+	unsigned int lsb_resol;		/* Resolution of the ADC sample LSB */
 };
 
-/* Function declaration - necessary due to function dependencies */
-static int ads7828_detect(struct i2c_client *client,
-			  struct i2c_board_info *info);
-static int ads7828_probe(struct i2c_client *client,
-			 const struct i2c_device_id *id);
-
-static inline u8 channel_cmd_byte(int ch)
+/* Command byte C2,C1,C0 - see datasheet */
+static inline u8 ads7828_cmd_byte(u8 cmd, int ch)
 {
-	/* cmd byte C2,C1,C0 - see datasheet */
-	u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
-	cmd |= ads7828_cmd_byte;
-	return cmd;
+	return cmd | (((ch >> 1) | (ch & 0x01) << 2) << 4);
 }
 
 /* Update data for the device (all 8 channels) */
@@ -96,12 +81,12 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
 		dev_dbg(&client->dev, "Starting ads7828 update\n");
 
 		for (ch = 0; ch < ADS7828_NCH; ch++) {
-			u8 cmd = channel_cmd_byte(ch);
+			u8 cmd = ads7828_cmd_byte(data->cmd_byte, ch);
 			data->adc_input[ch] =
 				i2c_smbus_read_word_swapped(client, cmd);
 		}
 		data->last_updated = jiffies;
-		data->valid = 1;
+		data->valid = true;
 	}
 
 	mutex_unlock(&data->update_lock);
@@ -110,28 +95,25 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
 }
 
 /* sysfs callback function */
-static ssize_t show_in(struct device *dev, struct device_attribute *da,
-	char *buf)
+static ssize_t ads7828_show_in(struct device *dev, struct device_attribute *da,
+			       char *buf)
 {
 	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
 	struct ads7828_data *data = ads7828_update_device(dev);
-	/* Print value (in mV as specified in sysfs-interface documentation) */
-	return sprintf(buf, "%d\n", (data->adc_input[attr->index] *
-		ads7828_lsb_resol)/1000);
-}
+	unsigned int value = DIV_ROUND_CLOSEST(data->adc_input[attr->index] *
+					       data->lsb_resol, 1000);
 
-#define in_reg(offset)\
-static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
-	NULL, offset)
+	return sprintf(buf, "%d\n", value);
+}
 
-in_reg(0);
-in_reg(1);
-in_reg(2);
-in_reg(3);
-in_reg(4);
-in_reg(5);
-in_reg(6);
-in_reg(7);
+static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ads7828_show_in, NULL, 0);
+static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ads7828_show_in, NULL, 1);
+static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, ads7828_show_in, NULL, 2);
+static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, ads7828_show_in, NULL, 3);
+static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, ads7828_show_in, NULL, 4);
+static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, ads7828_show_in, NULL, 5);
+static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, ads7828_show_in, NULL, 6);
+static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, ads7828_show_in, NULL, 7);
 
 static struct attribute *ads7828_attributes[] = {
 	&sensor_dev_attr_in0_input.dev_attr.attr,
@@ -152,36 +134,20 @@ static const struct attribute_group ads7828_group = {
 static int ads7828_remove(struct i2c_client *client)
 {
 	struct ads7828_data *data = i2c_get_clientdata(client);
+
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
-	kfree(i2c_get_clientdata(client));
+	i2c_set_clientdata(client, NULL);
+
 	return 0;
 }
 
-static const struct i2c_device_id ads7828_id[] = {
-	{ "ads7828", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(i2c, ads7828_id);
-
-/* This is the driver that will be inserted */
-static struct i2c_driver ads7828_driver = {
-	.class = I2C_CLASS_HWMON,
-	.driver = {
-		.name = "ads7828",
-	},
-	.probe = ads7828_probe,
-	.remove = ads7828_remove,
-	.id_table = ads7828_id,
-	.detect = ads7828_detect,
-	.address_list = normal_i2c,
-};
-
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int ads7828_detect(struct i2c_client *client,
 			  struct i2c_board_info *info)
 {
 	struct i2c_adapter *adapter = client->adapter;
+	u8 default_cmd_byte = ADS7828_CMD_SD_SE | ADS7828_CMD_PD3;
 	int ch;
 
 	/* Check we have a valid client */
@@ -196,9 +162,12 @@ static int ads7828_detect(struct i2c_client *client,
 	 * - Check the top 4 bits of each result are not set (12 data bits)
 	 */
 	for (ch = 0; ch < ADS7828_NCH; ch++) {
-		u16 in_data;
-		u8 cmd = channel_cmd_byte(ch);
-		in_data = i2c_smbus_read_word_swapped(client, cmd);
+		u8 cmd = ads7828_cmd_byte(default_cmd_byte, ch);
+		u16 in_data = i2c_smbus_read_word_swapped(client, cmd);
+
+		if (in_data < 0)
+			return -ENODEV;
+
 		if (in_data & 0xF000) {
 			pr_debug("%s : Doesn't look like an ads7828 device\n",
 				 __func__);
@@ -214,61 +183,86 @@ static int ads7828_detect(struct i2c_client *client,
 static int ads7828_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
-	struct ads7828_data *data;
 	int err;
-
-	data = kzalloc(sizeof(struct ads7828_data), GFP_KERNEL);
-	if (!data) {
-		err = -ENOMEM;
-		goto exit;
+	struct ads7828_data *data;
+	struct ads7828_platform_data *pdata = client->dev.platform_data;
+
+	data = devm_kzalloc(&client->dev, sizeof(struct ads7828_data),
+			    GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	if (pdata) {
+		data->diff_input = pdata->diff_input;
+		data->ext_vref = pdata->ext_vref;
+		if (data->ext_vref)
+			data->vref_mv = pdata->vref_mv;
 	}
 
+	/* Bound Vref with min/max values if it was provided */
+	if (data->vref_mv)
+		data->vref_mv = SENSORS_LIMIT(data->vref_mv,
+					      ADS7828_EXT_VREF_MV_MIN,
+					      ADS7828_EXT_VREF_MV_MAX);
+	else
+		data->vref_mv = ADS7828_INT_VREF_MV;
+
+	data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 4096);
+
+	data->cmd_byte = data->ext_vref ? ADS7828_CMD_PD1 : ADS7828_CMD_PD3;
+	if (!data->diff_input)
+		data->cmd_byte |= ADS7828_CMD_SD_SE;
+
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
-	/* Register sysfs hooks */
 	err = sysfs_create_group(&client->dev.kobj, &ads7828_group);
 	if (err)
-		goto exit_free;
+		return err;
 
 	data->hwmon_dev = hwmon_device_register(&client->dev);
 	if (IS_ERR(data->hwmon_dev)) {
 		err = PTR_ERR(data->hwmon_dev);
-		goto exit_remove;
+		goto error;
 	}
 
 	return 0;
 
-exit_remove:
+error:
 	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
-exit_free:
-	kfree(data);
-exit:
 	return err;
 }
 
-static int __init sensors_ads7828_init(void)
-{
-	/* Initialize the command byte according to module parameters */
-	ads7828_cmd_byte = se_input ?
-		ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
-	ads7828_cmd_byte |= int_vref ?
-		ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
+static const struct i2c_device_id ads7828_ids[] = {
+	{ "ads7828", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ads7828_ids);
 
-	/* Calculate the LSB resolution */
-	ads7828_lsb_resol = (vref_mv*1000)/4096;
+static struct i2c_driver ads7828_driver = {
+	.class = I2C_CLASS_HWMON,
+	.driver = {
+		.name = "ads7828",
+	},
+	.address_list = normal_i2c,
+	.detect = ads7828_detect,
+	.probe = ads7828_probe,
+	.remove = ads7828_remove,
+	.id_table = ads7828_ids,
+};
 
+static int __init sensors_ads7828_init(void)
+{
 	return i2c_add_driver(&ads7828_driver);
 }
+module_init(sensors_ads7828_init);
 
 static void __exit sensors_ads7828_exit(void)
 {
 	i2c_del_driver(&ads7828_driver);
 }
+module_exit(sensors_ads7828_exit);
 
 MODULE_AUTHOR("Steve Hardy <shardy@redhat.com>");
-MODULE_DESCRIPTION("ADS7828 driver");
+MODULE_DESCRIPTION("Driver for TI ADS7828 A/D converter");
 MODULE_LICENSE("GPL");
-
-module_init(sensors_ads7828_init);
-module_exit(sensors_ads7828_exit);
diff --git a/include/linux/platform_data/ads7828.h b/include/linux/platform_data/ads7828.h
new file mode 100644
index 0000000..3245f45
--- /dev/null
+++ b/include/linux/platform_data/ads7828.h
@@ -0,0 +1,29 @@
+/*
+ * TI ADS7828 A/D Converter platform data definition
+ *
+ * Copyright (c) 2012 Savoir-faire Linux Inc.
+ *          Vivien Didelot <vivien.didelot@savoirfairelinux.com>
+ *
+ * For further information, see the Documentation/hwmon/ads7828 file.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _PDATA_ADS7828_H
+#define _PDATA_ADS7828_H
+
+/**
+ * struct ads7828_platform_data - optional ADS7828 connectivity info
+ * @diff_input:		Differential input mode.
+ * @ext_vref:		Use an external voltage reference.
+ * @vref_mv:		Voltage reference value, if external.
+ */
+struct ads7828_platform_data {
+	bool diff_input;
+	bool ext_vref;
+	unsigned int vref_mv;
+};
+
+#endif /* _PDATA_ADS7828_H */
-- 
1.7.11.4

[PATCH v3 2/2] hwmon: (ads7828) add support for ADS7830

[Vivien Didelot]

From: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>

The ADS7830 device is almost the same as the ADS7828,
except that it does 8-bit sampling, instead of 12-bit.
This patch extends the ads7828 driver to support this chip.

Signed-off-by: Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 Documentation/hwmon/ads7828 | 12 ++++++++++--
 drivers/hwmon/Kconfig       |  7 ++++---
 drivers/hwmon/ads7828.c     | 45 ++++++++++++++++++++++++++++++++++-----------
 3 files changed, 48 insertions(+), 16 deletions(-)

diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
index b35668c..51eab52 100644
--- a/Documentation/hwmon/ads7828
+++ b/Documentation/hwmon/ads7828
@@ -8,8 +8,15 @@ Supported chips:
     Datasheet: Publicly available at the Texas Instruments website:
                http://focus.ti.com/lit/ds/symlink/ads7828.pdf
 
+  * Texas Instruments ADS7830
+    Prefix: 'ads7830'
+    Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
+    Datasheet: Publicly available at the Texas Instruments website:
+               http://focus.ti.com/lit/ds/symlink/ads7830.pdf
+
 Authors:
         Steve Hardy <shardy@redhat.com>
+        Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
 
 Platform data
 -------------
@@ -35,9 +42,10 @@ The structure fields are:
 Description
 -----------
 
-This driver implements support for the Texas Instruments ADS7828.
+This driver implements support for the Texas Instruments ADS7828 and ADS7830.
 
-This device is a 12-bit 8-channel A-D converter.
+The ADS7828 device is a 12-bit 8-channel A/D converter, while the ADS7830 does
+8-bit sampling.
 
 It can operate in single ended mode (8 +ve inputs) or in differential mode,
 where 4 differential pairs can be measured.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 83e3e9d..960c8c5 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1060,11 +1060,12 @@ config SENSORS_ADS1015
 	  will be called ads1015.
 
 config SENSORS_ADS7828
-	tristate "Texas Instruments ADS7828"
+	tristate "Texas Instruments ADS7828 and compatibles"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADS7828
-	  12-bit 8-channel ADC device.
+	  If you say yes here you get support for Texas Instruments ADS7828 and
+	  ADS7830 8-channel A/D converters. ADS7828 resolution is 12-bit, while
+	  it is 8-bit on ADS7830.
 
 	  This driver can also be built as a module.  If so, the module
 	  will be called ads7828.
diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index 0a13bf8..809c065 100644
--- a/drivers/hwmon/ads7828.c
+++ b/drivers/hwmon/ads7828.c
@@ -1,11 +1,13 @@
 /*
- * ads7828.c - lm_sensors driver for ads7828 12-bit 8-channel ADC
+ * ads7828.c - driver for TI ADS7828 8-channel A/D converter and compatibles
  * (C) 2007 EADS Astrium
  *
  * This driver is based on the lm75 and other lm_sensors/hwmon drivers
  *
  * Written by Steve Hardy <shardy@redhat.com>
  *
+ * ADS7830 support, by Guillaume Roguez <guillaume.roguez@savoirfairelinux.com>
+ *
  * For further information, see the Documentation/hwmon/ads7828 file.
  *
  * This program is free software; you can redistribute it and/or modify
@@ -43,6 +45,9 @@
 #define ADS7828_EXT_VREF_MV_MIN	50	/* External vref min value 0.05V */
 #define ADS7828_EXT_VREF_MV_MAX	5250	/* External vref max value 5.25V */
 
+/* List of supported devices */
+enum ads7828_chips { ads7828, ads7830 };
+
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
 	I2C_CLIENT_END };
@@ -59,6 +64,7 @@ struct ads7828_data {
 	unsigned int vref_mv;		/* voltage reference value */
 	u8 cmd_byte;			/* Command byte without channel bits */
 	unsigned int lsb_resol;		/* Resolution of the ADC sample LSB */
+	s32 (*read_channel)(const struct i2c_client *client, u8 command);
 };
 
 /* Command byte C2,C1,C0 - see datasheet */
@@ -82,8 +88,7 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
 
 		for (ch = 0; ch < ADS7828_NCH; ch++) {
 			u8 cmd = ads7828_cmd_byte(data->cmd_byte, ch);
-			data->adc_input[ch] =
-				i2c_smbus_read_word_swapped(client, cmd);
+			data->adc_input[ch] = data->read_channel(client, cmd);
 		}
 		data->last_updated = jiffies;
 		data->valid = true;
@@ -148,6 +153,7 @@ static int ads7828_detect(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = client->adapter;
 	u8 default_cmd_byte = ADS7828_CMD_SD_SE | ADS7828_CMD_PD3;
+	bool is_8bit = false;
 	int ch;
 
 	/* Check we have a valid client */
@@ -159,7 +165,9 @@ static int ads7828_detect(struct i2c_client *client,
 	 * dedicated register so attempt to sanity check using knowledge of
 	 * the chip
 	 * - Read from the 8 channel addresses
-	 * - Check the top 4 bits of each result are not set (12 data bits)
+	 * - Check the top 4 bits of each result:
+	 *   - They should not be set in case of 12-bit samples
+	 *   - The two bytes should be equal in case of 8-bit samples
 	 */
 	for (ch = 0; ch < ADS7828_NCH; ch++) {
 		u8 cmd = ads7828_cmd_byte(default_cmd_byte, ch);
@@ -169,13 +177,20 @@ static int ads7828_detect(struct i2c_client *client,
 			return -ENODEV;
 
 		if (in_data & 0xF000) {
-			pr_debug("%s : Doesn't look like an ads7828 device\n",
-				 __func__);
-			return -ENODEV;
+			if ((in_data >> 8) == (in_data & 0xFF)) {
+				/* Seems to be an ADS7830 (8-bit sample) */
+				is_8bit = true;
+			} else {
+				dev_dbg(&client->dev, "doesn't look like an ADS7828 compatible device\n");
+				return -ENODEV;
+			}
 		}
 	}
 
-	strlcpy(info->type, "ads7828", I2C_NAME_SIZE);
+	if (is_8bit)
+		strlcpy(info->type, "ads7830", I2C_NAME_SIZE);
+	else
+		strlcpy(info->type, "ads7828", I2C_NAME_SIZE);
 
 	return 0;
 }
@@ -207,7 +222,14 @@ static int ads7828_probe(struct i2c_client *client,
 	else
 		data->vref_mv = ADS7828_INT_VREF_MV;
 
-	data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 4096);
+	/* ADS7828 uses 12-bit samples, while ADS7830 is 8-bit */
+	if (id->driver_data == ads7828) {
+		data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 4096);
+		data->read_channel = i2c_smbus_read_word_swapped;
+	} else {
+		data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 256);
+		data->read_channel = i2c_smbus_read_byte_data;
+	}
 
 	data->cmd_byte = data->ext_vref ? ADS7828_CMD_PD1 : ADS7828_CMD_PD3;
 	if (!data->diff_input)
@@ -234,7 +256,8 @@ error:
 }
 
 static const struct i2c_device_id ads7828_ids[] = {
-	{ "ads7828", 0 },
+	{ "ads7828", ads7828 },
+	{ "ads7830", ads7830 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ads7828_ids);
@@ -264,5 +287,5 @@ static void __exit sensors_ads7828_exit(void)
 module_exit(sensors_ads7828_exit);
 
 MODULE_AUTHOR("Steve Hardy <shardy@redhat.com>");
-MODULE_DESCRIPTION("Driver for TI ADS7828 A/D converter");
+MODULE_DESCRIPTION("Driver for TI ADS7828 A/D converter and compatibles");
 MODULE_LICENSE("GPL");
-- 
1.7.11.4

Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup

[Guenter Roeck]

On Tue, Oct 02, 2012 at 06:10:02PM -0400, Vivien Didelot wrote:
> * Remove module parameters, add a ads7828_platform_data;
> * Move driver declaration to avoid adding function prototypes;
> * Remove unused macros;
> * Coding Style fixes.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Hi Vivien,

nice cleanup. Couple of minor comments below.

> ---
>  Documentation/hwmon/ads7828           |  31 +++--
>  drivers/hwmon/ads7828.c               | 216 +++++++++++++++++-----------------
>  include/linux/platform_data/ads7828.h |  29 +++++
>  3 files changed, 155 insertions(+), 121 deletions(-)
>  create mode 100644 include/linux/platform_data/ads7828.h
> 
> diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
> index 2bbebe6..b35668c 100644
> --- a/Documentation/hwmon/ads7828
> +++ b/Documentation/hwmon/ads7828
> @@ -5,21 +5,32 @@ Supported chips:
>    * Texas Instruments/Burr-Brown ADS7828
>      Prefix: 'ads7828'
>      Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
> -    Datasheet: Publicly available at the Texas Instruments website :
> +    Datasheet: Publicly available at the Texas Instruments website:
>                 http://focus.ti.com/lit/ds/symlink/ads7828.pdf
>  
>  Authors:
>          Steve Hardy <shardy@redhat.com>
>  
> -Module Parameters
> ------------------
> -
> -* se_input: bool (default Y)
> -  Single ended operation - set to N for differential mode
> -* int_vref: bool (default Y)
> -  Operate with the internal 2.5V reference - set to N for external reference
> -* vref_mv: int (default 2500)
> -  If using an external reference, set this to the reference voltage in mV
> +Platform data
> +-------------
> +
> +The ads7828 driver accepts an optional ads7828_platform_data structure (defined
> +in include/linux/platform_data/ads7828.h). If no structure is provided, the
> +configuration defaults to single ended operation and internal vref (2.5V).
> +
> +The structure fields are:
> +
> +* diff_input: bool
> +  Differential operation - set to true for differential mode,
> +  false for default single ended mode.
> +* ext_vref: bool
> +  External reference - set to true if it operates with an external reference,
> +  false for default internal reference.
> +* vref_mv: int
> +  Voltage reference - if using an external reference, set this to the reference
> +  voltage in mV, otherwise, it will default to the internal value (2500mV).
> +  This value will be bounded with limits accepted by the chip, described in the
> +  datasheet.
>  
>  Description
>  -----------
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index bf3fdf4..0a13bf8 100644
> --- a/drivers/hwmon/ads7828.c
> +++ b/drivers/hwmon/ads7828.c
> @@ -6,7 +6,7 @@
>   *
>   * Written by Steve Hardy <shardy@redhat.com>
>   *
> - * Datasheet available at: http://focus.ti.com/lit/ds/symlink/ads7828.pdf
> + * For further information, see the Documentation/hwmon/ads7828 file.
>   *
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License as published by
> @@ -23,63 +23,48 @@
>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> -#include <linux/module.h>
> -#include <linux/init.h>
> -#include <linux/slab.h>
> -#include <linux/jiffies.h>
> -#include <linux/i2c.h>
> +#include <linux/err.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> -#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/platform_data/ads7828.h>
> +#include <linux/slab.h>
>  
>  /* The ADS7828 registers */
> -#define ADS7828_NCH 8 /* 8 channels of 12-bit A-D supported */
> -#define ADS7828_CMD_SD_SE 0x80 /* Single ended inputs */
> -#define ADS7828_CMD_SD_DIFF 0x00 /* Differential inputs */
> -#define ADS7828_CMD_PD0 0x0 /* Power Down between A-D conversions */
> -#define ADS7828_CMD_PD1 0x04 /* Internal ref OFF && A-D ON */
> -#define ADS7828_CMD_PD2 0x08 /* Internal ref ON && A-D OFF */
> -#define ADS7828_CMD_PD3 0x0C /* Internal ref ON && A-D ON */
> -#define ADS7828_INT_VREF_MV 2500 /* Internal vref is 2.5V, 2500mV */
> +#define ADS7828_NCH		8	/* 8 channels supported */
> +#define ADS7828_CMD_SD_SE	0x80	/* Single ended inputs */
> +#define ADS7828_CMD_PD1		0x04	/* Internal vref OFF && A/D ON */
> +#define ADS7828_CMD_PD3		0x0C	/* Internal vref ON && A/D ON */
> +#define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
> +#define ADS7828_EXT_VREF_MV_MIN	50	/* External vref min value 0.05V */
> +#define ADS7828_EXT_VREF_MV_MAX	5250	/* External vref max value 5.25V */
>  
>  /* Addresses to scan */
>  static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
>  	I2C_CLIENT_END };
>  
> -/* Module parameters */
> -static bool se_input = 1; /* Default is SE, 0 == diff */
> -static bool int_vref = 1; /* Default is internal ref ON */
> -static int vref_mv = ADS7828_INT_VREF_MV; /* set if vref != 2.5V */
> -module_param(se_input, bool, S_IRUGO);
> -module_param(int_vref, bool, S_IRUGO);
> -module_param(vref_mv, int, S_IRUGO);
> -
> -/* Global Variables */
> -static u8 ads7828_cmd_byte; /* cmd byte without channel bits */
> -static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
> -
> -/* Each client has this additional data */
> +/* Client specific data */
>  struct ads7828_data {
>  	struct device *hwmon_dev;
> -	struct mutex update_lock; /* mutex protect updates */
> -	char valid; /* !=0 if following fields are valid */
> -	unsigned long last_updated; /* In jiffies */
> -	u16 adc_input[ADS7828_NCH]; /* ADS7828_NCH 12-bit samples */
> +	struct mutex update_lock;	/* Mutex protecting updates */
> +	unsigned long last_updated;	/* Last updated time (in jiffies) */
> +	u16 adc_input[ADS7828_NCH];	/* ADS7828_NCH samples */
> +	bool valid;			/* Validity flag */
> +	bool diff_input;		/* Differential input */
> +	bool ext_vref;			/* External voltage reference */
> +	unsigned int vref_mv;		/* voltage reference value */
> +	u8 cmd_byte;			/* Command byte without channel bits */
> +	unsigned int lsb_resol;		/* Resolution of the ADC sample LSB */
>  };
>  
> -/* Function declaration - necessary due to function dependencies */
> -static int ads7828_detect(struct i2c_client *client,
> -			  struct i2c_board_info *info);
> -static int ads7828_probe(struct i2c_client *client,
> -			 const struct i2c_device_id *id);
> -
> -static inline u8 channel_cmd_byte(int ch)
> +/* Command byte C2,C1,C0 - see datasheet */
> +static inline u8 ads7828_cmd_byte(u8 cmd, int ch)
>  {
> -	/* cmd byte C2,C1,C0 - see datasheet */
> -	u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> -	cmd |= ads7828_cmd_byte;
> -	return cmd;
> +	return cmd | (((ch >> 1) | (ch & 0x01) << 2) << 4);
>  }
>  
>  /* Update data for the device (all 8 channels) */
> @@ -96,12 +81,12 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
>  		dev_dbg(&client->dev, "Starting ads7828 update\n");
>  
>  		for (ch = 0; ch < ADS7828_NCH; ch++) {
> -			u8 cmd = channel_cmd_byte(ch);
> +			u8 cmd = ads7828_cmd_byte(data->cmd_byte, ch);
>  			data->adc_input[ch] =
>  				i2c_smbus_read_word_swapped(client, cmd);
>  		}
>  		data->last_updated = jiffies;
> -		data->valid = 1;
> +		data->valid = true;
>  	}
>  
>  	mutex_unlock(&data->update_lock);
> @@ -110,28 +95,25 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
>  }
>  
>  /* sysfs callback function */
> -static ssize_t show_in(struct device *dev, struct device_attribute *da,
> -	char *buf)
> +static ssize_t ads7828_show_in(struct device *dev, struct device_attribute *da,
> +			       char *buf)
>  {
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
>  	struct ads7828_data *data = ads7828_update_device(dev);
> -	/* Print value (in mV as specified in sysfs-interface documentation) */
> -	return sprintf(buf, "%d\n", (data->adc_input[attr->index] *
> -		ads7828_lsb_resol)/1000);
> -}
> +	unsigned int value = DIV_ROUND_CLOSEST(data->adc_input[attr->index] *
> +					       data->lsb_resol, 1000);
>  
> -#define in_reg(offset)\
> -static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> -	NULL, offset)
> +	return sprintf(buf, "%d\n", value);
> +}
>  
> -in_reg(0);
> -in_reg(1);
> -in_reg(2);
> -in_reg(3);
> -in_reg(4);
> -in_reg(5);
> -in_reg(6);
> -in_reg(7);
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, ads7828_show_in, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, ads7828_show_in, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, ads7828_show_in, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, ads7828_show_in, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, ads7828_show_in, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, ads7828_show_in, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, ads7828_show_in, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, ads7828_show_in, NULL, 7);
>  
>  static struct attribute *ads7828_attributes[] = {
>  	&sensor_dev_attr_in0_input.dev_attr.attr,
> @@ -152,36 +134,20 @@ static const struct attribute_group ads7828_group = {
>  static int ads7828_remove(struct i2c_client *client)
>  {
>  	struct ads7828_data *data = i2c_get_clientdata(client);
> +
>  	hwmon_device_unregister(data->hwmon_dev);
>  	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
> -	kfree(i2c_get_clientdata(client));
> +	i2c_set_clientdata(client, NULL);

i2c_set_clientdata(client, NULL) is not necessary. The framework does that for you.

> +
>  	return 0;
>  }
>  
> -static const struct i2c_device_id ads7828_id[] = {
> -	{ "ads7828", 0 },
> -	{ }
> -};
> -MODULE_DEVICE_TABLE(i2c, ads7828_id);
> -
> -/* This is the driver that will be inserted */
> -static struct i2c_driver ads7828_driver = {
> -	.class = I2C_CLASS_HWMON,
> -	.driver = {
> -		.name = "ads7828",
> -	},
> -	.probe = ads7828_probe,
> -	.remove = ads7828_remove,
> -	.id_table = ads7828_id,
> -	.detect = ads7828_detect,
> -	.address_list = normal_i2c,
> -};
> -
>  /* Return 0 if detection is successful, -ENODEV otherwise */
>  static int ads7828_detect(struct i2c_client *client,
>  			  struct i2c_board_info *info)
>  {
>  	struct i2c_adapter *adapter = client->adapter;
> +	u8 default_cmd_byte = ADS7828_CMD_SD_SE | ADS7828_CMD_PD3;
>  	int ch;
>  
>  	/* Check we have a valid client */
> @@ -196,9 +162,12 @@ static int ads7828_detect(struct i2c_client *client,
>  	 * - Check the top 4 bits of each result are not set (12 data bits)
>  	 */
>  	for (ch = 0; ch < ADS7828_NCH; ch++) {
> -		u16 in_data;
> -		u8 cmd = channel_cmd_byte(ch);
> -		in_data = i2c_smbus_read_word_swapped(client, cmd);
> +		u8 cmd = ads7828_cmd_byte(default_cmd_byte, ch);
> +		u16 in_data = i2c_smbus_read_word_swapped(client, cmd);
> +
> +		if (in_data < 0)
> +			return -ENODEV;
> +
>  		if (in_data & 0xF000) {
>  			pr_debug("%s : Doesn't look like an ads7828 device\n",
>  				 __func__);
> @@ -214,61 +183,86 @@ static int ads7828_detect(struct i2c_client *client,
>  static int ads7828_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> -	struct ads7828_data *data;
>  	int err;
> -
> -	data = kzalloc(sizeof(struct ads7828_data), GFP_KERNEL);
> -	if (!data) {
> -		err = -ENOMEM;
> -		goto exit;
> +	struct ads7828_data *data;
> +	struct ads7828_platform_data *pdata = client->dev.platform_data;
> +
> +	data = devm_kzalloc(&client->dev, sizeof(struct ads7828_data),
> +			    GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
The above change (use of devm_kzalloc) is in the latest upstream code already.
Please reparent.

> +	if (pdata) {
> +		data->diff_input = pdata->diff_input;
> +		data->ext_vref = pdata->ext_vref;
> +		if (data->ext_vref)
> +			data->vref_mv = pdata->vref_mv;
>  	}
>  
> +	/* Bound Vref with min/max values if it was provided */
> +	if (data->vref_mv)
> +		data->vref_mv = SENSORS_LIMIT(data->vref_mv,
> +					      ADS7828_EXT_VREF_MV_MIN,
> +					      ADS7828_EXT_VREF_MV_MAX);
> +	else
> +		data->vref_mv = ADS7828_INT_VREF_MV;
> +
> +	data->lsb_resol = DIV_ROUND_CLOSEST(data->vref_mv * 1000, 4096);
> +
> +	data->cmd_byte = data->ext_vref ? ADS7828_CMD_PD1 : ADS7828_CMD_PD3;
> +	if (!data->diff_input)
> +		data->cmd_byte |= ADS7828_CMD_SD_SE;
> +
>  	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
>  
> -	/* Register sysfs hooks */
>  	err = sysfs_create_group(&client->dev.kobj, &ads7828_group);
>  	if (err)
> -		goto exit_free;
> +		return err;
>  
>  	data->hwmon_dev = hwmon_device_register(&client->dev);
>  	if (IS_ERR(data->hwmon_dev)) {
>  		err = PTR_ERR(data->hwmon_dev);
> -		goto exit_remove;
> +		goto error;
>  	}
>  
>  	return 0;
>  
> -exit_remove:
> +error:
>  	sysfs_remove_group(&client->dev.kobj, &ads7828_group);
> -exit_free:
> -	kfree(data);
> -exit:
>  	return err;
>  }
>  
> -static int __init sensors_ads7828_init(void)
> -{
> -	/* Initialize the command byte according to module parameters */
> -	ads7828_cmd_byte = se_input ?
> -		ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
> -	ads7828_cmd_byte |= int_vref ?
> -		ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
> +static const struct i2c_device_id ads7828_ids[] = {
> +	{ "ads7828", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ads7828_ids);
>  
> -	/* Calculate the LSB resolution */
> -	ads7828_lsb_resol = (vref_mv*1000)/4096;
> +static struct i2c_driver ads7828_driver = {
> +	.class = I2C_CLASS_HWMON,
> +	.driver = {
> +		.name = "ads7828",
> +	},
> +	.address_list = normal_i2c,
> +	.detect = ads7828_detect,
> +	.probe = ads7828_probe,
> +	.remove = ads7828_remove,
> +	.id_table = ads7828_ids,
> +};
>  
> +static int __init sensors_ads7828_init(void)
> +{
>  	return i2c_add_driver(&ads7828_driver);
>  }
> +module_init(sensors_ads7828_init);
>  
>  static void __exit sensors_ads7828_exit(void)
>  {
>  	i2c_del_driver(&ads7828_driver);
>  }
> +module_exit(sensors_ads7828_exit);
>  
With the cleanup, you can now use the module_i2c_driver macro.

>  MODULE_AUTHOR("Steve Hardy <shardy@redhat.com>");
> -MODULE_DESCRIPTION("ADS7828 driver");
> +MODULE_DESCRIPTION("Driver for TI ADS7828 A/D converter");
>  MODULE_LICENSE("GPL");
> -
> -module_init(sensors_ads7828_init);
> -module_exit(sensors_ads7828_exit);
> diff --git a/include/linux/platform_data/ads7828.h b/include/linux/platform_data/ads7828.h
> new file mode 100644
> index 0000000..3245f45
> --- /dev/null
> +++ b/include/linux/platform_data/ads7828.h
> @@ -0,0 +1,29 @@
> +/*
> + * TI ADS7828 A/D Converter platform data definition
> + *
> + * Copyright (c) 2012 Savoir-faire Linux Inc.
> + *          Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> + *
> + * For further information, see the Documentation/hwmon/ads7828 file.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef _PDATA_ADS7828_H
> +#define _PDATA_ADS7828_H
> +
> +/**
> + * struct ads7828_platform_data - optional ADS7828 connectivity info
> + * @diff_input:		Differential input mode.
> + * @ext_vref:		Use an external voltage reference.
> + * @vref_mv:		Voltage reference value, if external.
> + */
> +struct ads7828_platform_data {
> +	bool diff_input;
> +	bool ext_vref;
> +	unsigned int vref_mv;
> +};
> +
> +#endif /* _PDATA_ADS7828_H */
> -- 
> 1.7.11.4
> 
> 

Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup

[Vivien Didelot]

Hi Guenter,

Some of those changes weren't in the mainline tree a few days ago.
I'll cherry-pick them and send a update very soon :-)

Thanks for the tips,
Vivien

Re: [PATCH v3 1/2] hwmon: (ads7828) driver cleanup

[Guenter Roeck]

On Tue, Oct 02, 2012 at 10:28:13PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> 
> Some of those changes weren't in the mainline tree a few days ago.
> I'll cherry-pick them and send a update very soon :-)
> 
True, but they have been in -next for the last two months or so ...

Guenter