lkml.org 
[lkml]   [2011]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@gdsys.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@gdsys.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


\
 
 \ /
  Last update: 2011-02-17 13:45    [from the cache]
©2003-2011 Jasper Spaans