linux-mips
[Top] [All Lists]

Re: [PATCH 2/4] RTC: make m41t80 driver can work with the SMBus adapters

To: rongkai.zhan@windriver.com
Subject: Re: [PATCH 2/4] RTC: make m41t80 driver can work with the SMBus adapters
From: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
Date: Tue, 02 Oct 2007 00:06:16 +0900 (JST)
Cc: i2c@lm-sensors.org, linux-mips@linux-mips.org, rtc-linux@googlegroups.com, ralf@linux-mips.org, a.zummo@towertech.it
In-reply-to: <46FF726E.4020200@windriver.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <46FF726E.4020200@windriver.com>
Sender: linux-mips-bounce@linux-mips.org
On Sun, 30 Sep 2007 17:54:54 +0800, Mark Zhan <rongkai.zhan@windriver.com> 
wrote:
> This patch makes m41t80 RTC driver also can work with the SMBus adapters,
> which doesn't i2c_transfer() method.
> 
> Signed-off-by: Mark Zhan <rongkai.zhan@windriver.com>

As Jean already said, your mailer corrupted your patch.
Also, please keep in mind the 80 column rule.

> +static int m41t80_i2c_read(struct i2c_client *client, struct i2c_msg *msgs, 
> int num)
> +{
> +     int i, rc;
> +     u8 dt_addr = msgs[0].buf[0];
> +
> +     if (num < 2)
> +             return -1;
> +
> +     if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
> +             i2c_transfer(client->adapter, msgs, 2) < 0) {
> +             dev_err(&client->dev, "read error\n");
> +             return -EIO;
> +     } else {
> +             for (i = 0; i < msgs[1].len; i++) {
> +                     rc = i2c_smbus_read_byte_data(client, dt_addr + i);
> +                     if (rc < 0)
> +                             return -EIO;
> +                     msgs[1].buf[i] = (u8)rc;
> +             }
> +     }
> +
> +     return 0;
> +}

You must ensure time values are consistent.  The RTC might update its
time data between each I2C transfer.

It seems the original rtc_m41t81.c:m41t81_get_time() tries to solve
this issue by reading sec/min in loop, but it would not be enough.
For example, if the function was called at 23:59:59, the sec (and min)
might wrap just before reading the hour, then you might get 00:59:59.

The old m41t00 i2c driver once did it right with smbus, but current
m41t00 driver (and rtc-m41t80 driver) dropped that feature a while
ago.  You can see the old code at:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/i2c/chips/m41t00.c;hb=e931b8d8a428f87e6ea488d2fd80007bb66b3ea8

> +static int m41t80_i2c_write(struct i2c_client *client, struct i2c_msg *msg)
> +{
> +     int i, rc;
> +     u8 *wbuf = msg->buf;
> +     u8 *buf = wbuf + 1;
> +
> +     if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
> +         i2c_transfer(client->adapter, msg, 1) < 0) {
> +             dev_err(&client->dev, "write error\n");
> +             return -EIO;
> +     } else {
> +             for (i = 0; i < msg[0].len - 1; i++) {
> +                     rc = i2c_smbus_write_byte_data(client, wbuf[0]+i, 
> buf[i]);
> +                     if (rc < 0)
> +                             return -EIO;
> +             }
> +     }
> +
> +     return 0;
> +}

Writing to the RTC by multiple I2C transfers might have same
consistency problem.  But it would not be a real problem if you wrote
the sec register first.  Here is a comment in
rtc_m41t81.c:m41t81_set_time():

        /*
         * Note the write order matters as it ensures the correctness.
         * When we write sec, 10th sec is clear.  It is reasonable to
         * believe we should finish writing min within a second.
         */

I think this comment is worth to import.

Other parts looks good for me.

---
Atsushi Nemoto

<Prev in Thread] Current Thread [Next in Thread>