Hi Grant,
On Sun, 13 May 2007 01:43:42 -0600, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@xxxxxxxxxxxx>
> ---
>
> Here is the 3rd iteration with the following changes:
> - Modified to be an i2c "new style" driver based on David Brownell's work
Can we see the corresponding architecture patch (device declaration)?
> - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
> reads instead of a single byte at a time.
>
> drivers/i2c/chips/ds1682.c | 270 ++++++++++++++++++++++++++++++++++++++++++++
> 1 files changed, 270 insertions(+), 0 deletions(-)
Where have the changes to Kconfig and Makefile gone? Without them, this
patch is a no-op.
BTW, the config option shouldn't be named SENSORS_DS1682 as your
original patch had - this device isn't a sensor. It should be just
"DS1682".
j>
>
> diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
> new file mode 100644
> index 0000000..0a4a89e
> --- /dev/null
> +++ b/drivers/i2c/chips/ds1682.c
> @@ -0,0 +1,270 @@
> +/*
> + * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
> + *
> + * Written by: Grant Likely <grant.likely@xxxxxxxxxxxx>
> + *
> + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> + * Copyright (C) 2005 James Chapman <jchapman@xxxxxxxxxxx>
> + * Copyright (C) 2000 Russell King
> + *
> + * Derived from ds1337 real time clock device driver
Technically, this is the ds1337 driver on which James and Russell have
a copyright, not yours. I don't think there is anything left from the
code you originally copied from, BTW.
> + * Simple register attributes
> + */
> +SENSOR_DEVICE_ATTR_2(config, S_IRUGO, ds1682_show, NULL, DS1682_REG_CONFIG, 1);
Bug! The last two arguments are swapped.
Also, I don't see the point of exporting this value to user-space, as
the format is proprietary.
> +SENSOR_DEVICE_ATTR_2(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> + 4, DS1682_REG_ELAPSED);
> +SENSOR_DEVICE_ATTR_2(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> + 4, DS1682_REG_ALARM);
> +SENSOR_DEVICE_ATTR_2(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> + 2, DS1682_REG_EVT_CNTR);
> +
> +static const struct attribute_group ds1682_group = {
> + .attrs = (struct attribute *[]) {
> + &sensor_dev_attr_config.dev_attr.attr,
> + &sensor_dev_attr_elapsed_time.dev_attr.attr,
> + &sensor_dev_attr_alarm_time.dev_attr.attr,
> + &sensor_dev_attr_event_count.dev_attr.attr,
> + NULL,
> + },
> +};
I'm not sure if all compilers support this.
> +static ssize_t ds1682_eeprom_read(struct kobject *kobj, char *buf, loff_t off,
> + size_t count)
> +{
> + struct i2c_client *client = kobj_to_i2c_client(kobj);
> + int rc;
> +
> + dev_dbg(&client->dev, "ds1682_eeprom_read(p=%p, off=%lli, c=%zi)\n",
> + buf, off, count);
> +
> + if (off > DS1682_EEPROM_SIZE)
Should be ">=", otherwise you might end up doing a 0-length I2C block
read, which is bad.
> +
> +/*
> + * Called when a device is found at the ds1682 address
> + */
This comment is no longer true.
> +static int ds1682_probe(struct i2c_client *client)
> +{
> + int err = 0;
> +
> + if (client->addr != 0x6b) {
> + dev_err(&client->dev, "%x is not a valid address\n",
> + client->addr);
> + return -ENODEV;
> + }
Why check this? You should trust the person who wrote the device
definition in the platform code. The only reason why the older i2c chip
drivers did address checks is because that was one preliminary
device identification step. With the new model, you don't need to
identify the device, you already know what's there as per
architecture-level device declaration.
Not checking the address even has a benefit: if a compatible device
exists (today or in the future) with a different address, we don't need
to patch the driver to support it.
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_BYTE_DATA |
> + I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
This no longer reflects what the driver is using.
> + dev_err(&client->dev, "i2c bus does not support the ds1682\n");
> + goto exit;
> + }
> +
> + if (sysfs_create_group(&client->dev.kobj, &ds1682_group))
Missing "err = ".