Message-ID: <528646bc0705141029w397b11cejec17c770d64d7ce5@mail.gmail.com>
Date:	Mon, 14 May 2007 11:29:04 -0600
From:	"Grant Likely" <grant.likely@...retlab.ca>
To:	"Jean Delvare" <khali@...ux-fr.org>
Cc:	i2c@...sensors.org, linux-kernel@...r.kernel.org,
	"Alessandro Zummo" <alessandro.zummo@...ertech.it>
Subject: Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

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

On 5/14/07, Jean Delvare <khali@...ux-fr.org> wrote:
> Hi Grant,
>
> On Sun, 13 May 2007 01:43:42 -0600, Grant Likely wrote:
> > Signed-off-by: Grant Likely <grant.likely@...retlab.ca>
> > ---
> >
> > 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 <grant.likely@...retlab.ca>
> > + *
> > + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> > + * Copyright (C) 2005 James Chapman <jchapman@...alix.com>
> > + * 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.
grant.likely@...retlab.ca
(403) 399-0195
-
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