Hi Grant,
On Mon, 7 May 2007 14:45:42 -0600, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@xxxxxxxxxxxx>
> ---
> drivers/i2c/chips/Kconfig | 10 ++
> drivers/i2c/chips/Makefile | 1 +
> drivers/i2c/chips/ds1682.c | 363 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 374 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/chips/ds1682.c
>
Please fix the following warnings:
drivers/i2c/chips/ds1682.c: In function 'ds1682_store':
drivers/i2c/chips/ds1682.c:129: warning: format '%i' expects type 'int', but argument 5 has type 'size_t'
drivers/i2c/chips/ds1682.c: In function 'ds1682_user_data_store':
drivers/i2c/chips/ds1682.c:220: warning: format '%i' expects type 'int', but argument 4 has type 'size_t'
> + help
> + If you say yes here you get support for Dallas Semiconductor
> + DS1682 Total Elapsed Time Recorder
> +
> + This driver can also be built as a module. If so, the module
> + will be called ds1682.
> +
I know this isn't exactly a RTC chip, but this is still related with
timekeeping, so wouldn't it be better placed under drivers/rtc?
Alessandro, what do you think?
> +static unsigned short normal_i2c[] = { DS1682_ADDR, I2C_CLIENT_END };
> +
> +I2C_CLIENT_INSMOD;
> +
> +/*
> + * Low level chip access functions
> + */
> +static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
> +{
> + u8 *bytes = buf;
> + int val;
> +
> + while (len--) {
> + val = i2c_smbus_read_byte_data(client, reg++);
> + if (val < 0)
> + return -EIO;
> + *bytes++ = val;
> + }
> + return 0;
> +}
Did you check if the device supports I2C block reads? This would be
much faster, and might also solve consistency issues. Are you certain
that the above brings you back bytes which all belong to the same
"time measurement"? Does the device latch the register values on the
first read?
I find it strange that you use an I2C block write when writing values
to the chip, but individual byte reads in the other direction.
> +static ssize_t ds1682_store(struct device *dev,
> + struct device_attribute *attr, const char *buf,
> + size_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + int reg;
> + int regsize;
> + char *endp;
> + u32 val;
> + int rc;
> +
> + /* Sanitize the input */
> + reg = ds1682_attr_to_reg(attr, ®size);
> +
> + if ((reg < 0) || (count >= 16) || (buf[count] != '\0')) {
How could buf[count] not be '\0'? As far as I know this is guaranteed
by the underlying sysfs code. I also don't see the point of checking
the value of count.
> +
> +/*
> + * Called when a device is found at the ds1682 address
> + */
> +static struct i2c_driver ds1682_driver;
> +static int ds1682_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> + struct i2c_client *new_client;
Please rename this to just "client". This "new_client" name is a
disease :(
> + if (!(new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + new_client->addr = address;
> + new_client->adapter = adapter;
> + new_client->driver = &ds1682_driver;
> + new_client->flags = 0;
Not needed, the kzalloc above did it for you.
> +
> + /* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */
The "kind" value has nothing to do with the address. It tells you if
the user asked (through a module parameter) for the device
detection/identification to be skipped. Which brings me to the point
that your driver does no device identification at all. Fortunately,
address 0x6b isn't very popular, but I would still like to see some
detection code. Is there a way to identify the DS1682? I know that such
devices usually (unfortunately) don't have an identification register,
but maybe there are unused configuration bits which can be checked? Or
you can check the number of registers?
> +
> + return 0;
> +
> + exit_attr_userdata:
Please use a single space for label indentation.
> +static struct i2c_driver ds1682_driver = {
> + .driver = {
> + .name = "ds1682",
> + },
Indentation.
The code is quite nice otherwise, good work!