| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1349148501.5876.22.camel@trivette>
Date: Mon, 01 Oct 2012 23:28:21 -0400
From: Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: lm-sensors@...sensors.org,
Guillaume Roguez <guillaume.roguez@...oirfairelinux.com>,
Jean Delvare <khali@...ux-fr.org>,
linux-kernel@...r.kernel.org, Steve Hardy <shardy@...hat.com>
Subject: Re: [PATCH v2 2/2] hwmon: (ads7828) add support for ADS7830
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@...oirfairelinux.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@...oirfairelinux.com>
> > Signed-off-by: Vivien Didelot <vivien.didelot@...oirfairelinux.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@...hat.com>
> > + Guillaume Roguez <guillaume.roguez@...oirfairelinux.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@...hat.com>
> > *
> > + * ADS7830 support by Guillaume Roguez <guillaume.roguez@...oirfairelinux.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@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists