| 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: <20110217134203.0556a7ab@endymion.delvare>
Date: Thu, 17 Feb 2011 13:42:03 +0100
From: Jean Delvare <khali@...ux-fr.org>
To: Dirk Eibach <eibach@...ys.de>
Cc: linux-kernel@...r.kernel.org, guenter.roeck@...csson.com,
lm-sensors@...sensors.org, rdunlap@...otime.net,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: Add support for Texas Instruments ADS1015
Hi Dirk,
On Mon, 14 Feb 2011 14:21:50 +0100, Dirk Eibach wrote:
> Signed-off-by: Dirk Eibach <eibach@...ys.de>
> ---
> Changes since v1:
> - fixed/extended Documentation
> - removed unused register definitions
> - hardcoded PGA fullscale table size
> - made sure patch applies against v2.6.38-rc4
> - reordered functions to avoid forward declaration
> - results from i2c_smbus_read_word_data() are handled correctly
> - moved locking into ads1015_read_value()
> - removed unnecessray clearing of bit
> - proper error handling in ads1015_read_value()
> - use DIV_ROUND_CLOSEST for scaling result
> - removed detect()
Thanks for the quick update. Second review:
> (...)
> --- /dev/null
> +++ b/Documentation/hwmon/ads1015
> @@ -0,0 +1,33 @@
> +Kernel driver ads1015
> +=====================
> +
> +Supported chips:
> + * Texas Instruments ADS1015
> + Prefix: 'ads1015'
> + Addresses scanned: I2C 0x48, 0x49, 0x4a, 0x4b
With the detect function being gone, this is no longer true.
> + Datasheet: Publicly available at the Texas Instruments website :
> + http://focus.ti.com/lit/ds/symlink/ads1015.pdf
> +
> +Authors:
> + Dirk Eibach, Guntermann & Drunck GmbH <eibach@...ys.de>
> +
> +Description
> +-----------
> +
> +This driver implements support for the Texas Instruments ADS1015.
> +
> +This device is a 12-bit A-D converter with 4 inputs.
> +
> +The inputs can be used single ended or in certain differential combinations.
> +
> +On certain systems it makes sense to access absolute voltage values as well
> +as voltage differences. So all available combinations are made available by
> +8 "virtual" inputs:
> +in0: Voltage over AIN0 and AIN1.
> +in1: Voltage over AIN0 and AIN3.
> +in2: Voltage over AIN1 and AIN3.
> +in3: Voltage over AIN2 and AIN3.
> +in4: Voltage over AIN0 and GND.
> +in5: Voltage over AIN1 and GND.
> +in6: Voltage over AIN2 and GND.
> +in7: Voltage over AIN3 and GND.
I see you've updated the comment, presumably this is how you addressed
my concern about exposing all 8 input settings. I am really curious how
it can make sense to expose both direct and differential values
involving the same pins. The pcf8591 driver, which has to handle a
smiliar case, only exposes channels which make physical sense together
(it does so using a module parameter for historical reason, nowadays we
would use platform data for this.)
So I am still convinced that this part should be reworked. That being
said, you obviously know more than I do with regards to how you intend
to use the driver, so I'll leave you the last work on this.
> (...)
> --- /dev/null
> +++ b/drivers/hwmon/ads1015.c
> (...)
> +static int ads1015_read_value(struct i2c_client *client, unsigned int channel,
> + int *value)
> +{
> + u16 config;
> + s16 conversion;
> + unsigned int pga;
> + int fullscale;
> + unsigned int k;
> + struct ads1015_data *data = i2c_get_clientdata(client);
> + int res;
> +
> + mutex_lock(&data->update_lock);
> +
> + /* get fullscale voltage */
> + res = ads1015_read_reg(client, ADS1015_CONFIG);
> + if (res < 0)
> + goto err_unlock;
> + config = res;
> + pga = (config >> 9) & 0x0007;
> + fullscale = fullscale_table[pga];
> +
> + /* set channel and start single conversion */
> + config &= ~(0x0007 << 12);
> + config |= (1 << 15) | (1 << 8) | (channel & 0x0007) << 12;
> +
> + /* wait until conversion finished */
> + res = ads1015_write_reg(client, ADS1015_CONFIG, config);
> + if (res < 0)
> + goto err_unlock;
> + for (k = 0; k < 5; ++k) {
> + schedule_timeout(msecs_to_jiffies(1));
> + res = ads1015_read_reg(client, ADS1015_CONFIG);
> + if (res < 0)
> + goto err_unlock;
> + config = res;
> + if (config & (1 << 15))
> + break;
> + }
> + if (k == 5)
> + return -EIO;
You return with data->update_lock held.
> +
> + res = ads1015_read_reg(client, ADS1015_CONVERSION);
> + if (res < 0)
> + goto err_unlock;
> + conversion = res;
> +
> + mutex_unlock(&data->update_lock);
> +
> + *value = DIV_ROUND_CLOSEST(conversion * fullscale, 0x7ff0);
> +
> + return 0;
> +
> +err_unlock:
> + mutex_unlock(&data->update_lock);
> + return res;
> +}
> (...)
> +/* This is the driver that will be inserted */
> +static struct i2c_driver ads1015_driver = {
> + .class = I2C_CLASS_HWMON,
> + .driver = {
> + .name = "ads1015",
> + },
> + .probe = ads1015_probe,
> + .remove = ads1015_remove,
> + .id_table = ads1015_id,
> + .address_list = normal_i2c,
> +};
The only purpose of the address list is for the detect function, which
you just dropped. So you can remove the address list too. Same goes for
the class.
--
Jean Delvare
--
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