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

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

[Vivien Didelot]

* Remove unused macros;
* Point to the documentation;
* Coding Style fixes (Kernel Doc, spacing);
* Move driver declaration to avoid adding function prototypes.

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/hwmon/ads7828.c | 91 +++++++++++++++++++++++--------------------------
 1 file changed, 43 insertions(+), 48 deletions(-)

diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
index bf3fdf4..fb3010d 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
@@ -34,14 +34,12 @@
 #include <linux/mutex.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_SD_DIFF	0x00	/* Differential inputs */
+#define ADS7828_CMD_PD1		0x04	/* Internal ref OFF && A/D ON */
+#define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
+#define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
 
 /* Addresses to scan */
 static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
@@ -59,25 +57,26 @@ module_param(vref_mv, int, S_IRUGO);
 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 */
+/**
+ * struct ads7828_data - client specific data
+ * @hwmon_dev:		The hwmon device.
+ * @update_lock:	Mutex protecting updates.
+ * @valid:		Validity flag.
+ * @last_updated:	Last updated time (in jiffies).
+ * @adc_input:		ADS7828_NCH samples.
+ */
 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;
+	char valid;
+	unsigned long last_updated;
+	u16 adc_input[ADS7828_NCH];
 };
 
-/* 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)
 {
 	/* cmd byte C2,C1,C0 - see datasheet */
-	u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
+	u8 cmd = (((ch >> 1) | (ch & 0x01) << 2) << 4);
 	cmd |= ads7828_cmd_byte;
 	return cmd;
 }
@@ -120,9 +119,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
 		ads7828_lsb_resol)/1000);
 }
 
-#define in_reg(offset)\
-static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
-	NULL, offset)
+#define in_reg(offset) \
+static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in, NULL, offset)
 
 in_reg(0);
 in_reg(1);
@@ -158,25 +156,6 @@ static int ads7828_remove(struct i2c_client *client)
 	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)
@@ -247,13 +226,29 @@ exit:
 	return err;
 }
 
+static const struct i2c_device_id ads7828_ids[] = {
+	{ "ads7828", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, ads7828_ids);
+
+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)
 {
 	/* 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;
+	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
+	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
 
 	/* Calculate the LSB resolution */
 	ads7828_lsb_resol = (vref_mv*1000)/4096;
@@ -267,7 +262,7 @@ static void __exit sensors_ads7828_exit(void)
 }
 
 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);
-- 
1.7.11.4

[PATCH v2 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     | 58 ++++++++++++++++++++++++++++++++-------------
 3 files changed, 55 insertions(+), 22 deletions(-)

diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
index 2bbebe6..4366a87 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>
 
 Module Parameters
 -----------------
@@ -24,9 +31,10 @@ Module Parameters
 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 fb3010d..91fc629 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
@@ -41,6 +43,9 @@
 #define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
 #define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
 
+/* 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 };
@@ -53,9 +58,7 @@ 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 */
 
 /**
  * struct ads7828_data - client specific data
@@ -64,6 +67,8 @@ static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
  * @valid:		Validity flag.
  * @last_updated:	Last updated time (in jiffies).
  * @adc_input:		ADS7828_NCH samples.
+ * @lsb_resol:		Resolution of the A/D converter sample LSB.
+ * @read_channel:	Function used to read a channel.
  */
 struct ads7828_data {
 	struct device *hwmon_dev;
@@ -71,6 +76,8 @@ struct ads7828_data {
 	char valid;
 	unsigned long last_updated;
 	u16 adc_input[ADS7828_NCH];
+	unsigned int lsb_resol;
+	s32 (*read_channel)(const struct i2c_client *client, u8 command);
 };
 
 static inline u8 channel_cmd_byte(int ch)
@@ -96,8 +103,7 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
 
 		for (ch = 0; ch < ADS7828_NCH; ch++) {
 			u8 cmd = channel_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 = 1;
@@ -115,8 +121,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
 	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);
+	return sprintf(buf, "%d\n",
+		       (data->adc_input[attr->index] * data->lsb_resol) / 1000);
 }
 
 #define in_reg(offset) \
@@ -162,6 +168,7 @@ static int ads7828_detect(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = client->adapter;
 	int ch;
+	bool is_8bit = false;
 
 	/* Check we have a valid client */
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA))
@@ -172,20 +179,30 @@ 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++) {
 		u16 in_data;
 		u8 cmd = channel_cmd_byte(ch);
 		in_data = i2c_smbus_read_word_swapped(client, cmd);
 		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;
 }
@@ -202,6 +219,15 @@ static int ads7828_probe(struct i2c_client *client,
 		goto exit;
 	}
 
+	/* ADS7828 uses 12-bit samples, while ADS7830 is 8-bit */
+	if (id->driver_data == ads7828) {
+		data->read_channel = i2c_smbus_read_word_swapped;
+		data->lsb_resol = (vref_mv * 1000) / 4096;
+	} else {
+		data->read_channel = i2c_smbus_read_byte_data;
+		data->lsb_resol = (vref_mv * 1000) / 256;
+	}
+
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);
 
@@ -227,7 +253,8 @@ exit:
 }
 
 static const struct i2c_device_id ads7828_ids[] = {
-	{ "ads7828", 0 },
+	{ "ads7828", ads7828 },
+	{ "ads7830", ads7830 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ads7828_ids);
@@ -250,9 +277,6 @@ static int __init sensors_ads7828_init(void)
 	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
 	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
 
-	/* Calculate the LSB resolution */
-	ads7828_lsb_resol = (vref_mv*1000)/4096;
-
 	return i2c_add_driver(&ads7828_driver);
 }
 
@@ -262,7 +286,7 @@ static void __exit sensors_ads7828_exit(void)
 }
 
 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");
 
 module_init(sensors_ads7828_init);
-- 
1.7.11.4

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

[Guenter Roeck]

On Mon, Oct 01, 2012 at 07:16:24PM -0400, Vivien Didelot wrote:
> 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>

Hi Guillaume,
Hi Vivien,

couple of comments below.

> ---
>  Documentation/hwmon/ads7828 | 12 ++++++++--
>  drivers/hwmon/Kconfig       |  7 +++---
>  drivers/hwmon/ads7828.c     | 58 ++++++++++++++++++++++++++++++++-------------
>  3 files changed, 55 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
> index 2bbebe6..4366a87 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>
>  
>  Module Parameters
>  -----------------
> @@ -24,9 +31,10 @@ Module Parameters
>  Description
>  -----------
>  
> -This driver implements support for the Texas Instruments ADS7828.
> +This driver implements support for the Texas Instruments ADS7828, and ADS7830.
>  

s/,//

> -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 fb3010d..91fc629 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
> @@ -41,6 +43,9 @@
>  #define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
>  #define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
>  
> +/* 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 };
> @@ -53,9 +58,7 @@ 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 */

At some point we may have to look into the configuration ... using module
parameters doesn't seem to be right here. Should be platform data or device
tree. But that is for later ... just something to keep in mind.

> -static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
>  
>  /**
>   * struct ads7828_data - client specific data
> @@ -64,6 +67,8 @@ static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
>   * @valid:		Validity flag.
>   * @last_updated:	Last updated time (in jiffies).
>   * @adc_input:		ADS7828_NCH samples.
> + * @lsb_resol:		Resolution of the A/D converter sample LSB.
> + * @read_channel:	Function used to read a channel.
>   */
>  struct ads7828_data {
>  	struct device *hwmon_dev;
> @@ -71,6 +76,8 @@ struct ads7828_data {
>  	char valid;
>  	unsigned long last_updated;
>  	u16 adc_input[ADS7828_NCH];
> +	unsigned int lsb_resol;
> +	s32 (*read_channel)(const struct i2c_client *client, u8 command);
>  };
>  
>  static inline u8 channel_cmd_byte(int ch)
> @@ -96,8 +103,7 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
>  
>  		for (ch = 0; ch < ADS7828_NCH; ch++) {
>  			u8 cmd = channel_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 = 1;
> @@ -115,8 +121,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
>  	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);
> +	return sprintf(buf, "%d\n",
> +		       (data->adc_input[attr->index] * data->lsb_resol) / 1000);
>  }
>  
>  #define in_reg(offset) \
> @@ -162,6 +168,7 @@ static int ads7828_detect(struct i2c_client *client,
>  {
>  	struct i2c_adapter *adapter = client->adapter;
>  	int ch;
> +	bool is_8bit = false;
>  
>  	/* Check we have a valid client */
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA))
> @@ -172,20 +179,30 @@ 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++) {
>  		u16 in_data;
>  		u8 cmd = channel_cmd_byte(ch);
>  		in_data = i2c_smbus_read_word_swapped(client, cmd);

What if it is < 0, ie if there was a read error since the device does not exist ?

in_data should be defined as int, and you should check for an error and
abort if there is one (previously that was covered in the "& 0xF000", but that
is no longer the case).

>  		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");

WARNING: quoted string split across lines
#190: FILE: drivers/hwmon/ads7828.c:196:
+                               dev_dbg(&client->dev, "doesn't look like an "
+                                       "ADS7828 compatible device\n");

Better:
				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;
>  }
> @@ -202,6 +219,15 @@ static int ads7828_probe(struct i2c_client *client,
>  		goto exit;
>  	}
>  
> +	/* ADS7828 uses 12-bit samples, while ADS7830 is 8-bit */
> +	if (id->driver_data == ads7828) {
> +		data->read_channel = i2c_smbus_read_word_swapped;
> +		data->lsb_resol = (vref_mv * 1000) / 4096;
> +	} else {
> +		data->read_channel = i2c_smbus_read_byte_data;
> +		data->lsb_resol = (vref_mv * 1000) / 256;

Just wondering ... does that introduce a rounding error ?
Would DIV_ROUND_CLOSEST be better ?

> +	}
> +
>  	i2c_set_clientdata(client, data);
>  	mutex_init(&data->update_lock);
>  
> @@ -227,7 +253,8 @@ exit:
>  }
>  
>  static const struct i2c_device_id ads7828_ids[] = {
> -	{ "ads7828", 0 },
> +	{ "ads7828", ads7828 },
> +	{ "ads7830", ads7830 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, ads7828_ids);
> @@ -250,9 +277,6 @@ static int __init sensors_ads7828_init(void)
>  	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
>  	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
>  
> -	/* Calculate the LSB resolution */
> -	ads7828_lsb_resol = (vref_mv*1000)/4096;
> -
>  	return i2c_add_driver(&ads7828_driver);
>  }
>  
> @@ -262,7 +286,7 @@ static void __exit sensors_ads7828_exit(void)
>  }
>  
>  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");
>  
>  module_init(sensors_ads7828_init);
> -- 
> 1.7.11.4
> 
> 

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

[Vivien Didelot]

Hi Guenter,

On Mon, 2012-10-01 at 18:20 -0700, Guenter Roeck wrote:
> On Mon, Oct 01, 2012 at 07:16:24PM -0400, Vivien Didelot wrote:
> > 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>
> 
> Hi Guillaume,
> Hi Vivien,
> 
> couple of comments below.
> 
> > ---
> >  Documentation/hwmon/ads7828 | 12 ++++++++--
> >  drivers/hwmon/Kconfig       |  7 +++---
> >  drivers/hwmon/ads7828.c     | 58 ++++++++++++++++++++++++++++++++-------------
> >  3 files changed, 55 insertions(+), 22 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/ads7828 b/Documentation/hwmon/ads7828
> > index 2bbebe6..4366a87 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>
> >  
> >  Module Parameters
> >  -----------------
> > @@ -24,9 +31,10 @@ Module Parameters
> >  Description
> >  -----------
> >  
> > -This driver implements support for the Texas Instruments ADS7828.
> > +This driver implements support for the Texas Instruments ADS7828, and ADS7830.
> >  
> 
> s/,//
Sounds like an abuse of the serial comma :-)
> 
> > -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 fb3010d..91fc629 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
> > @@ -41,6 +43,9 @@
> >  #define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
> >  #define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
> >  
> > +/* 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 };
> > @@ -53,9 +58,7 @@ 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 */
> 
> At some point we may have to look into the configuration ... using module
> parameters doesn't seem to be right here. Should be platform data or device
> tree. But that is for later ... just something to keep in mind.
> 
> > -static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
> >  
> >  /**
> >   * struct ads7828_data - client specific data
> > @@ -64,6 +67,8 @@ static unsigned int ads7828_lsb_resol; /* resolution of the ADC sample lsb */
> >   * @valid:		Validity flag.
> >   * @last_updated:	Last updated time (in jiffies).
> >   * @adc_input:		ADS7828_NCH samples.
> > + * @lsb_resol:		Resolution of the A/D converter sample LSB.
> > + * @read_channel:	Function used to read a channel.
> >   */
> >  struct ads7828_data {
> >  	struct device *hwmon_dev;
> > @@ -71,6 +76,8 @@ struct ads7828_data {
> >  	char valid;
> >  	unsigned long last_updated;
> >  	u16 adc_input[ADS7828_NCH];
> > +	unsigned int lsb_resol;
> > +	s32 (*read_channel)(const struct i2c_client *client, u8 command);
> >  };
> >  
> >  static inline u8 channel_cmd_byte(int ch)
> > @@ -96,8 +103,7 @@ static struct ads7828_data *ads7828_update_device(struct device *dev)
> >  
> >  		for (ch = 0; ch < ADS7828_NCH; ch++) {
> >  			u8 cmd = channel_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 = 1;
> > @@ -115,8 +121,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
> >  	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);
> > +	return sprintf(buf, "%d\n",
> > +		       (data->adc_input[attr->index] * data->lsb_resol) / 1000);
> >  }
> >  
> >  #define in_reg(offset) \
> > @@ -162,6 +168,7 @@ static int ads7828_detect(struct i2c_client *client,
> >  {
> >  	struct i2c_adapter *adapter = client->adapter;
> >  	int ch;
> > +	bool is_8bit = false;
> >  
> >  	/* Check we have a valid client */
> >  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_READ_WORD_DATA))
> > @@ -172,20 +179,30 @@ 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++) {
> >  		u16 in_data;
> >  		u8 cmd = channel_cmd_byte(ch);
> >  		in_data = i2c_smbus_read_word_swapped(client, cmd);
> 
> What if it is < 0, ie if there was a read error since the device does not exist ?
> 
> in_data should be defined as int, and you should check for an error and
> abort if there is one (previously that was covered in the "& 0xF000", but that
> is no longer the case).
Good catch.
> 
> >  		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");
> 
> WARNING: quoted string split across lines
> #190: FILE: drivers/hwmon/ads7828.c:196:
> +                               dev_dbg(&client->dev, "doesn't look like an "
> +                                       "ADS7828 compatible device\n");
Hum ok, so the reason for that is that it breaks the ability to grep a
string... Makes sense.
> 
> Better:
> 				dev_dbg(&client->dev,
> 					"doesn't look like an ADS7828 compatible device\n");
This exceeds 80 chars, but I'll find a shorter message.
> 
> > +				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;
> >  }
> > @@ -202,6 +219,15 @@ static int ads7828_probe(struct i2c_client *client,
> >  		goto exit;
> >  	}
> >  
> > +	/* ADS7828 uses 12-bit samples, while ADS7830 is 8-bit */
> > +	if (id->driver_data == ads7828) {
> > +		data->read_channel = i2c_smbus_read_word_swapped;
> > +		data->lsb_resol = (vref_mv * 1000) / 4096;
> > +	} else {
> > +		data->read_channel = i2c_smbus_read_byte_data;
> > +		data->lsb_resol = (vref_mv * 1000) / 256;
> 
> Just wondering ... does that introduce a rounding error ?
> Would DIV_ROUND_CLOSEST be better ?
Since it is still a module parameter, yes, it will be safer to use
DIV_ROUND_CLOSEST.
> 
> > +	}
> > +
> >  	i2c_set_clientdata(client, data);
> >  	mutex_init(&data->update_lock);
> >  
> > @@ -227,7 +253,8 @@ exit:
> >  }
> >  
> >  static const struct i2c_device_id ads7828_ids[] = {
> > -	{ "ads7828", 0 },
> > +	{ "ads7828", ads7828 },
> > +	{ "ads7830", ads7830 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, ads7828_ids);
> > @@ -250,9 +277,6 @@ static int __init sensors_ads7828_init(void)
> >  	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
> >  	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
> >  
> > -	/* Calculate the LSB resolution */
> > -	ads7828_lsb_resol = (vref_mv*1000)/4096;
> > -
> >  	return i2c_add_driver(&ads7828_driver);
> >  }
> >  
> > @@ -262,7 +286,7 @@ static void __exit sensors_ads7828_exit(void)
> >  }
> >  
> >  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");
> >  
> >  module_init(sensors_ads7828_init);
> > -- 
> > 1.7.11.4
> > 
> > 
Thanks,
Vivien

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

[Guenter Roeck]

On Mon, Oct 01, 2012 at 11:28:21PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> 
Hi Vivien,

[ ... ]

> > > +			} else {
> > > +				dev_dbg(&client->dev, "doesn't look like an "
> > > +					"ADS7828 compatible device\n");
> > 
> > WARNING: quoted string split across lines
> > #190: FILE: drivers/hwmon/ads7828.c:196:
> > +                               dev_dbg(&client->dev, "doesn't look like an "
> > +                                       "ADS7828 compatible device\n");
> Hum ok, so the reason for that is that it breaks the ability to grep a
> string... Makes sense.
> > 
> > Better:
> > 				dev_dbg(&client->dev,
> > 					"doesn't look like an ADS7828 compatible device\n");
> This exceeds 80 chars, but I'll find a shorter message.

That is ok nowadays if it is due to a quoted string.

Guenter 

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

[Guenter Roeck]

On Mon, Oct 01, 2012 at 07:16:23PM -0400, Vivien Didelot wrote:
> * Remove unused macros;
> * Point to the documentation;
> * Coding Style fixes (Kernel Doc, spacing);
> * Move driver declaration to avoid adding function prototypes.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Hi Vivien,

> ---
>  drivers/hwmon/ads7828.c | 91 +++++++++++++++++++++++--------------------------
>  1 file changed, 43 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> index bf3fdf4..fb3010d 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
> @@ -34,14 +34,12 @@
>  #include <linux/mutex.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_SD_DIFF	0x00	/* Differential inputs */
> +#define ADS7828_CMD_PD1		0x04	/* Internal ref OFF && A/D ON */
> +#define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
> +#define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
>  
>  /* Addresses to scan */
>  static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> @@ -59,25 +57,26 @@ module_param(vref_mv, int, S_IRUGO);
>  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 */
> +/**
> + * struct ads7828_data - client specific data
> + * @hwmon_dev:		The hwmon device.
> + * @update_lock:	Mutex protecting updates.
> + * @valid:		Validity flag.
> + * @last_updated:	Last updated time (in jiffies).
> + * @adc_input:		ADS7828_NCH samples.
> + */
This isn't really an externally visible API, so I wonder if it provides value to
document it this way. No strong opinion, just wondering.

>  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;
> +	char valid;
> +	unsigned long last_updated;
> +	u16 adc_input[ADS7828_NCH];
>  };
>  
> -/* 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)
>  {
>  	/* cmd byte C2,C1,C0 - see datasheet */
> -	u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> +	u8 cmd = (((ch >> 1) | (ch & 0x01) << 2) << 4);
>  	cmd |= ads7828_cmd_byte;
>  	return cmd;
>  }
> @@ -120,9 +119,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
>  		ads7828_lsb_resol)/1000);

Can you fix this one as well since you are at it ? There is another one in sensors_ads7828_init().
[ Wonder why checkpatch doesn't complain about it ]

>  }
>  
> -#define in_reg(offset)\
> -static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> -	NULL, offset)
> +#define in_reg(offset) \
> +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in, NULL, offset)
>  
This causes a checkpatch error - checkpatch doesn't like the multi-line macros.

>  in_reg(0);
>  in_reg(1);

Since it seems to be hardly worth it anyway (yes, I know there are 8 of them),
would be great if you can just get rid of the macro and just use
	static SENSOR_DEVICE_ATTR(in[1-8]_input, ...)
instead.

> @@ -158,25 +156,6 @@ static int ads7828_remove(struct i2c_client *client)
>  	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)
> @@ -247,13 +226,29 @@ exit:
>  	return err;
>  }
>  
> +static const struct i2c_device_id ads7828_ids[] = {
> +	{ "ads7828", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ads7828_ids);
> +
> +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)
>  {
>  	/* 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;
> +	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
> +	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
>  
>  	/* Calculate the LSB resolution */
>  	ads7828_lsb_resol = (vref_mv*1000)/4096;
> @@ -267,7 +262,7 @@ static void __exit sensors_ads7828_exit(void)
>  }
>  
>  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);
> -- 
> 1.7.11.4
> 
> 

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

[Vivien Didelot]

Hi Guenter,

On Mon, 2012-10-01 at 18:07 -0700, Guenter Roeck wrote:
> On Mon, Oct 01, 2012 at 07:16:23PM -0400, Vivien Didelot wrote:
> > * Remove unused macros;
> > * Point to the documentation;
> > * Coding Style fixes (Kernel Doc, spacing);
> > * Move driver declaration to avoid adding function prototypes.
> > 
> > Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
> 
> Hi Vivien,
> 
> > ---
> >  drivers/hwmon/ads7828.c | 91 +++++++++++++++++++++++--------------------------
> >  1 file changed, 43 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/hwmon/ads7828.c b/drivers/hwmon/ads7828.c
> > index bf3fdf4..fb3010d 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
> > @@ -34,14 +34,12 @@
> >  #include <linux/mutex.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_SD_DIFF	0x00	/* Differential inputs */
> > +#define ADS7828_CMD_PD1		0x04	/* Internal ref OFF && A/D ON */
> > +#define ADS7828_CMD_PD3		0x0C	/* Internal ref ON && A/D ON */
> > +#define ADS7828_INT_VREF_MV	2500	/* Internal vref is 2.5V, 2500mV */
> >  
> >  /* Addresses to scan */
> >  static const unsigned short normal_i2c[] = { 0x48, 0x49, 0x4a, 0x4b,
> > @@ -59,25 +57,26 @@ module_param(vref_mv, int, S_IRUGO);
> >  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 */
> > +/**
> > + * struct ads7828_data - client specific data
> > + * @hwmon_dev:		The hwmon device.
> > + * @update_lock:	Mutex protecting updates.
> > + * @valid:		Validity flag.
> > + * @last_updated:	Last updated time (in jiffies).
> > + * @adc_input:		ADS7828_NCH samples.
> > + */
> This isn't really an externally visible API, so I wonder if it provides value to
> document it this way. No strong opinion, just wondering.
I found the version below a bit cluttered, that's why I used the
KernelDoc notation. Would you prefer something else, like right-aligned
comments?
> 
> >  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;
> > +	char valid;
> > +	unsigned long last_updated;
> > +	u16 adc_input[ADS7828_NCH];
> >  };
> >  
> > -/* 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)
> >  {
> >  	/* cmd byte C2,C1,C0 - see datasheet */
> > -	u8 cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> > +	u8 cmd = (((ch >> 1) | (ch & 0x01) << 2) << 4);
> >  	cmd |= ads7828_cmd_byte;
> >  	return cmd;
> >  }
> > @@ -120,9 +119,8 @@ static ssize_t show_in(struct device *dev, struct device_attribute *da,
> >  		ads7828_lsb_resol)/1000);
> 
> Can you fix this one as well since you are at it ? There is another one in sensors_ads7828_init().
> [ Wonder why checkpatch doesn't complain about it ]
Sure.
> 
> >  }
> >  
> > -#define in_reg(offset)\
> > -static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> > -	NULL, offset)
> > +#define in_reg(offset) \
> > +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in, NULL, offset)
> >  
> This causes a checkpatch error - checkpatch doesn't like the multi-line macros.
My bad, I'm on a 2.6.37 box, I didn't checkout the upstream version of
checkpatch.pl too.
> 
> >  in_reg(0);
> >  in_reg(1);
> 
> Since it seems to be hardly worth it anyway (yes, I know there are 8 of them),
> would be great if you can just get rid of the macro and just use
> 	static SENSOR_DEVICE_ATTR(in[1-8]_input, ...)
> instead.
Ok.
> 
> > @@ -158,25 +156,6 @@ static int ads7828_remove(struct i2c_client *client)
> >  	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)
> > @@ -247,13 +226,29 @@ exit:
> >  	return err;
> >  }
> >  
> > +static const struct i2c_device_id ads7828_ids[] = {
> > +	{ "ads7828", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, ads7828_ids);
> > +
> > +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)
> >  {
> >  	/* 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;
> > +	ads7828_cmd_byte = se_input ? ADS7828_CMD_SD_SE : ADS7828_CMD_SD_DIFF;
> > +	ads7828_cmd_byte |= int_vref ? ADS7828_CMD_PD3 : ADS7828_CMD_PD1;
> >  
> >  	/* Calculate the LSB resolution */
> >  	ads7828_lsb_resol = (vref_mv*1000)/4096;
> > @@ -267,7 +262,7 @@ static void __exit sensors_ads7828_exit(void)
> >  }
> >  
> >  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);
> > -- 
> > 1.7.11.4
> > 
> > 

Thanks for your comments,
Vivien

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

[Guenter Roeck]

On Mon, Oct 01, 2012 at 10:16:07PM -0400, Vivien Didelot wrote:
> Hi Guenter,
> 
[ ... ]
> > >  
> > > -/* Each client has this additional data */
> > > +/**
> > > + * struct ads7828_data - client specific data
> > > + * @hwmon_dev:		The hwmon device.
> > > + * @update_lock:	Mutex protecting updates.
> > > + * @valid:		Validity flag.
> > > + * @last_updated:	Last updated time (in jiffies).
> > > + * @adc_input:		ADS7828_NCH samples.
> > > + */
> > This isn't really an externally visible API, so I wonder if it provides value to
> > document it this way. No strong opinion, just wondering.
> I found the version below a bit cluttered, that's why I used the
> KernelDoc notation. Would you prefer something else, like right-aligned
> comments?

Tab aligned, maybe ? Not sure if that works out, though.

Guenter