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@xxxxxxxxxxxxxxxxxxxx>
> > 
> > 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@xxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> > +        Guillaume Roguez <guillaume.roguez@xxxxxxxxxxxxxxxxxxxx>
> >  
> >  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@xxxxxxxxxx>
> >   *
> > + * ADS7830 support by Guillaume Roguez <guillaume.roguez@xxxxxxxxxxxxxxxxxxxx>
> > + *
> >   * 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@xxxxxxxxxx>");
> > -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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/