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
