linux-mips
[Top] [All Lists]

Re: [i2c] [PATCH 3/4] RTC: add Xicor 1241 driver

To: Mark Zhan <rongkai.zhan@windriver.com>
Subject: Re: [i2c] [PATCH 3/4] RTC: add Xicor 1241 driver
From: Jean Delvare <khali@linux-fr.org>
Date: Mon, 1 Oct 2007 15:59:38 +0200
Cc: i2c@lm-sensors.org, linux-mips@linux-mips.org, rtc-linux@googlegroups.com, a.zummo@towertech.it, ralf@linux-mips.org
In-reply-to: <46FF7279.3020102@windriver.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <46FF7279.3020102@windriver.com>
Sender: linux-mips-bounce@linux-mips.org
Hi Mark,

On Sun, 30 Sep 2007 17:55:05 +0800, Mark Zhan wrote:
> This patch add the Xicor 1241 RTC driver which is used in
> MIPS Sibyte 1250/1480 boards.

So this chip is using two-byte register addressing, which isn't
compatible with SMBus. Which explains why the register reads and writes
in your driver look strange. I don't think it's quite correct.

> +/*
> + * Register Offset
> + */
> +#define X1241_SEC    0x30            /* Seconds */
> +#define X1241_MIN    0x31            /* Minutes */
> +#define X1241_HOUR   0x32            /* Hours */
> +#define X1241_MDAY   0x33            /* Day of month */
> +#define X1241_MON    0x34            /* Month */
> +#define X1241_YEAR   0x35            /* Year */
> +#define X1241_WDAY   0x36            /* Day of Week */
> +#define X1241_Y2K    0x37            /* Year 2K */
> +#define X1241_SR     0x3F            /* Status register */
> +
> +DEFINE_SPINLOCK(xicor1241_lock);
> +
> +static int xicor1241_get_time(struct device *dev, struct rtc_time *tm)
> +{
> +     struct i2c_client *client = to_i2c_client(dev);
> +     unsigned int y2k, year, mon, mday, wday, hour, min, sec;
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(&xicor1241_lock, flags);
> +
> +     i2c_smbus_write_byte_data(client, X1241_SEC, X1241_SEC);

If I read the datasheet properly, this should be:

        i2c_smbus_write_byte_data(client, 0, X1241_SEC);

The SC register is at 0x0030, not 0x3030.

> +     sec = i2c_smbus_read_byte_data(client, X1241_SEC);
> +     min = i2c_smbus_read_byte_data(client, X1241_MIN);
> +     hour = i2c_smbus_read_byte_data(client, X1241_HOUR);
> +     mday = i2c_smbus_read_byte_data(client, X1241_MDAY);
> +     mon = i2c_smbus_read_byte_data(client, X1241_MON);
> +     year = i2c_smbus_read_byte_data(client, X1241_YEAR);
> +     wday = i2c_smbus_read_byte_data(client, X1241_WDAY);
> +     y2k = i2c_smbus_read_byte_data(client, X1241_Y2K);

You are using the "Current Address Read" mode here, right? If so, you
aren't supposed to send any address information at all, i.e. you should
use i2c_smbus_read_byte() instead of i2c_smbus_read_byte_data(). You
are probably just lucky that the chip ignores the extra byte you're
sending.

> (...)
> +static int xicor1241_set_time(struct device *dev, struct rtc_time *tm)
> +{
> (...)
> +     /* unlock writes to the CCR */
> +     i2c_smbus_write_word_data(client, X1241_SR,
> +                     (X1241_SR_WEL << 8) | X1241_SR);
> +     i2c_smbus_write_word_data(client, X1241_SR,
> +                     ((X1241_SR_WEL | X1241_SR_RWEL) << 8) | X1241_SR);
> +
> +     i2c_smbus_write_word_data(client, X1241_SEC, (sec << 8) | X1241_SEC);
> +     i2c_smbus_write_word_data(client, X1241_MIN, (min << 8) | X1241_MIN);
> +     i2c_smbus_write_word_data(client, X1241_HOUR, (hour << 8) | X1241_HOUR);
> +     i2c_smbus_write_word_data(client, X1241_MDAY, (mday << 8) | X1241_MDAY);
> +     i2c_smbus_write_word_data(client, X1241_WDAY, (wday << 8) | X1241_WDAY);
> +     i2c_smbus_write_word_data(client, X1241_MON, (mon << 8) | X1241_MON);
> +     i2c_smbus_write_word_data(client, X1241_YEAR, (year << 8) | X1241_YEAR);
> +     i2c_smbus_write_word_data(client, X1241_Y2K, (y2k << 8) | X1241_Y2K);
> +
> +     i2c_smbus_write_word_data(client, X1241_SR, (0 << 8) | X1241_SR);

Here again I am surprised. I expected:

        i2c_smbus_write_word_data(client, 0, (sec << 8) | X1241_SEC);

So that you write to register 0x0030, not 0x3030. Same for all the
other register writes. Or am I misreading the datasheet?

> +
> +     spin_unlock_irqrestore(&xicor1241_lock, flags);
> +     return 0;
> +}
> +
> +static const struct rtc_class_ops xicor1241_rtc_ops = {
> +     .read_time = xicor1241_get_time,
> +     .set_time  = xicor1241_set_time,
> +};
> +
> +static int __devinit xicor1241_rtc_probe(struct i2c_client *client)
> +{
> +     struct rtc_device *rtc;
> +
> +     if (!i2c_check_functionality(client->adapter, 
> I2C_FUNC_SMBUS_BYTE_DATA)) {
> +             dev_dbg(&client->dev, "I2C adapter function check failure!\n");
> +             return -ENODEV;
> +     }

This check isn't sufficient, you must check for
I2C_FUNC_SMBUS_WRITE_WORD_DATA as well, and possibly
I2C_FUNC_SMBUS_READ_BYTE if my comment above is correct.

-- 
Jean Delvare

<Prev in Thread] Current Thread [Next in Thread>
  • Re: [i2c] [PATCH 3/4] RTC: add Xicor 1241 driver, Jean Delvare <=