On Mon, 03 Dec 2007 17:59:59 +0000 Steve Hardy <[email protected]> wrote:
> Hi,
>
> Here is a patch against 2.6.23.9 which adds support for the
> Burr-Brown/Texas-Instruments
> ADS7828 12-bit 8-channel A-D converter.
>
> The chip is used for voltage monitoring on the COTS processor card I am
> currently working with.
>
> The driver simply outputs the current input voltages (in mv as specified
> in the lm-sensors sysfs interface documentation). Any scaling required for
> a specific board is handled by user-space. Hopefully this makes the driver
> generic enough to be generally useful.
>
> The driver is basically a simple rehash of existing code - I used lm75 as
> the starting point, with some inspiration from other existing drivers.
>
> ...
>
> diff -uprN -X linux-2.6.23.9/Documentation/dontdiff
> linux-2.6.23.9/drivers/hwmon/Kconfig
> linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig
> --- linux-2.6.23.9/drivers/hwmon/Kconfig 2007-11-26
> 17:51:43.000000000 +0000
> +++ linux-2.6.23.9-ads7828/drivers/hwmon/Kconfig 2007-11-28
Your email client is wordwrapping the text and it replaces tabs with
spaces. It will need a resend, please.
> @@ -530,6 +530,16 @@ config SENSORS_THMC50
> This driver can also be built as a module. If so, the module
> will be called thmc50.
>
> +config SENSORS_ADS7828
> + tristate "Texas Instruments ADS7828"
> + depends on I2C && EXPERIMENTAL
I wouldn't bother with EXPERIMENTAL personally. It seems a farily
pointless thing.
> linux-2.6.23.9-ads7828/drivers/hwmon/ads7828.c
Please prepare and test patches against the latest Linus tree, from git or
from ftp://ftp.kernel.org/pub/linux/kernel/v2.6/snapshots
Usually it's OK to develop a new driver against the latest stable release,
but we regularly change interfaces and if we did, this driver won't even
compile.
> +static int ads7828_attach_adapter(struct i2c_adapter *adapter);
> +static int ads7828_detect(struct i2c_adapter *adapter, int address, int
> kind);
> +static void ads7828_init_client(struct i2c_client *client);
> +static int ads7828_detach_client(struct i2c_client *client);
> +static struct ads7828_data *ads7828_update_device(struct device *dev);
> +static u16 ads7828_read_value(struct i2c_client *client, u8 reg);
I do dislike all these forward declarations, but they're all needed here
give the order of the functions. Maybe from my Pascal-on-pdp11 days..
> +/* This is the driver that will be inserted */
> +static struct i2c_driver ads7828_driver = {
> + .driver = {
> + .name = "ads7828",
> + },
> + .id = I2C_DRIVERID_ADS7828,
> + .attach_adapter = ads7828_attach_adapter,
> + .detach_client = ads7828_detach_client,
> +};
> +
> +static ssize_t show_in(struct device *dev, struct device_attribute *da,
> + char *buf)
> +{
> + 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 sysctl-interface
> documentation) */
> + return sprintf(buf, "%d\n", (data->adc_input[attr->index] *
> + ads7828_lsb_resol)/1000);
> +}
> +
> +#define in_reg(offset)\
> +static SENSOR_DEVICE_ATTR(in##offset##_input, S_IRUGO, show_in,\
> + NULL, offset);
> +
> +in_reg(0);
> +in_reg(1);
> +in_reg(2);
> +in_reg(3);
> +in_reg(4);
> +in_reg(5);
> +in_reg(6);
> +in_reg(7);
> +
> +static int ads7828_attach_adapter(struct i2c_adapter *adapter)
> +{
> + if (!(adapter->class & I2C_CLASS_HWMON))
> + return 0;
Can this happen?
> + return i2c_probe(adapter, &addr_data, ads7828_detect);
> +}
> +
> +static struct attribute *ads7828_attributes[] = {
> + &sensor_dev_attr_in0_input.dev_attr.attr,
> + &sensor_dev_attr_in1_input.dev_attr.attr,
> + &sensor_dev_attr_in2_input.dev_attr.attr,
> + &sensor_dev_attr_in3_input.dev_attr.attr,
> + &sensor_dev_attr_in4_input.dev_attr.attr,
> + &sensor_dev_attr_in5_input.dev_attr.attr,
> + &sensor_dev_attr_in6_input.dev_attr.attr,
> + &sensor_dev_attr_in7_input.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group ads7828_group = {
> + .attrs = ads7828_attributes,
> +};
> +
> +/* This function is called by i2c_detect */
> +static int ads7828_detect(struct i2c_adapter *adapter, int address, int
> kind)
> +{
> + struct i2c_client *new_client;
> + struct ads7828_data *data;
> + int err = 0, ch = 0;
> + const char *name = "";
> + u8 cmd;
> + u16 in_data;
> +
> + /* Check we have a valid client */
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_WORD_DATA))
> + goto exit;
> +
> + /* OK. For now, we presume we have a valid client. We now create the
> + client structure, even though we cannot fill it completely yet.
> + But it allows us to access ads7828_read_value. */
> + data = kmalloc(sizeof(struct ads7828_data), GFP_KERNEL);
> + if (!data)
> + err = -ENOMEM;
> + goto exit;
> + }
> + memset(data, 0, sizeof(struct ads7828_data));
Use kzalloc() above, remove the memset.
> + new_client = &data->client;
> + i2c_set_clientdata(new_client, data);
> + new_client->addr = address;
> + new_client->adapter = adapter;
> + new_client->driver = &ads7828_driver;
> + new_client->flags = 0;
> +
> + /* Perform local initialisation */
> + ads7828_init_client(new_client);
> +
> + /* Now, we do the remaining detection. There is no identification
> + 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)
> + */
> + if (kind < 0) {
> + for (ch = 0; ch < ADS7828_NCH; ch++) {
> + /* cmd byte C2,C1,C0 - see datasheet */
> + cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> + cmd |= ads7828_cmd_byte;
> + in_data = ads7828_read_value(new_client, cmd);
> + if (in_data & 0xF000) {
> + printk(KERN_DEBUG
> + "%s : Doesn't look like an ads7828 device\n",
> + __FUNCTION__);
> + goto exit_free;
> + }
> + }
> + }
> +
> + /* Determine the chip type - only one kind supported! */
> + if (kind <= 0)
> + kind = ads7828;
> +
> + if (kind == ads7828)
> + name = "ads7828";
> +
> + /* Fill in the remaining client fields, put it into the global list */
> + strlcpy(new_client->name, name, I2C_NAME_SIZE);
> + data->valid = 0;
> + mutex_init(&data->update_lock);
> +
> + /* Tell the I2C layer a new client has arrived */
> + err = i2c_attach_client(new_client);
> + if (err)
> + goto exit_free;
> +
> + /* Register sysfs hooks */
> + err = sysfs_create_group(&new_client->dev.kobj, &ads7828_group);
> + if (err)
> + goto exit_detach;
> +
> + data->class_dev = hwmon_device_register(&new_client->dev);
> + if (IS_ERR(data->class_dev)) {
> + err = PTR_ERR(data->class_dev);
> + goto exit_remove;
> + }
> +
> + return 0;
> +
> +exit_remove:
> + sysfs_remove_group(&new_client->dev.kobj, &ads7828_group);
> +exit_detach:
> + i2c_detach_client(new_client);
> +exit_free:
> + kfree(data);
> +exit:
> + return err;
> +}
>
> ...
>
> +static struct ads7828_data *ads7828_update_device(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct ads7828_data *data = i2c_get_clientdata(client);
> + unsigned int cmd, ch;
> +
> + mutex_lock(&data->update_lock); /* LOCK */
I don't think that comment adds a lot of value ;)
> + if (time_after(jiffies, data->last_updated + HZ + HZ / 2)
> + || !data->valid) {
> + dev_dbg(&client->dev, "Starting ads7828 update\n");
> +
> + for (ch = 0; ch < ADS7828_NCH; ch++) {
> + /* cmd byte C2,C1,C0 - see datasheet */
> + cmd = (((ch>>1) | (ch&0x01)<<2)<<4);
> + cmd |= ads7828_cmd_byte;
> + data->adc_input[ch] = ads7828_read_value(client, cmd);
> + }
> + data->last_updated = jiffies;
> + data->valid = 1;
> + }
> +
> + mutex_unlock(&data->update_lock); /* UNLOCK */
But that one's great!
> + return data;
> +}
> +
> +static int __init sensors_ads7828_init(void)
> +{
> + return i2c_add_driver(&ads7828_driver);
> +}
> +
> +static void __exit sensors_ads7828_exit(void)
> +{
> + i2c_del_driver(&ads7828_driver);
> +}
> +
> +MODULE_AUTHOR("Steve Hardy <[email protected]");
> +MODULE_DESCRIPTION("ADS7828 driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(sensors_ads7828_init);
> +module_exit(sensors_ads7828_exit);
> diff -uprN -X linux-2.6.23.9/Documentation/dontdiff
> linux-2.6.23.9/include/linux/i2c-id.h
> linux-2.6.23.9-ads7828/include/linux/i2c-id.h
> --- linux-2.6.23.9/include/linux/i2c-id.h 2007-11-26
> 17:51:43.000000000 +0000
> +++ linux-2.6.23.9-ads7828/include/linux/i2c-id.h 2007-11-28
> 10:02:02.000000000 +0000
> @@ -161,6 +161,7 @@
> #define I2C_DRIVERID_FSCHER 1046
> #define I2C_DRIVERID_W83L785TS 1047
> #define I2C_DRIVERID_OV7670 1048 /* Omnivision 7670 camera */
> +#define I2C_DRIVERID_ADS7828 1049
It all looks reasonable. I'd have applied it if it weren't for the
mailer-mangling.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
