Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Ugh.  Well that was a rather sloppy patch on my part.  I'll fix up and resubmit.

On 5/14/07, Jean Delvare <[email protected]> wrote:
Hi Grant,

On Sun, 13 May 2007 01:43:42 -0600, Grant Likely wrote:
> Signed-off-by: Grant Likely <[email protected]>
> ---
>
> 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)?

Yes, I'll include it in the commit comment.  It is for a board that
isn't in mainline (yet) so a seperate patch doesn't make much sense.


> - 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.

Oops, eaten by stgit (or rather; eaten by my inexperience with stgit.


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".

Done.

BTW, I've just left the driver in drivers/i2c/chips.  It seems
sufficiently different from an rtc (and the interface is totally
difference) that putting it in drivers/rtc seems wrong.  If you
disagree, I can change this.


>
> 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 <[email protected]>
> + *
> + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> + * Copyright (C) 2005 James Chapman <[email protected]>
> + * Copyright (C) 2000 Russell King
> + *
> + * Derived from ds1337 real time clock device driver
j>
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.

ok, fixed.  I like to err on the side of caution when it comes to
copyright notices.

> + * 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.

You're right.  Removed.

> +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.

I think it should be okay.  It is used abundantly in the
arch/ppc/syslib/*_devices.c code, and that code is compiled with a lot
of different compiler versions.

> +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.

Right, good catch

> +
> +/*
> + * 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.

Me being paranoid a guess.  Removed.


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 = ".

Gah!

Thanks for the comments,
g.

--
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
[email protected]
(403) 399-0195
-
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]
  Powered by Linux