Hi Jean, > -----Original Message----- > From: Jean Delvare [mailto:[EMAIL PROTECTED] ... > Note: you need to include a comment describing what your patch does, > as well as your Signed-off-by line. Here's a second review from me. > After that it will be up to Alessandro and the RTC folks. >
So I'm waiting for their comments. > > --- a/drivers/rtc/rtc-ds1307.c 2008-09-08 19:40:20.000000000 > +0200 > > +++ b/drivers/rtc/rtc-ds1307.c 2008-10-10 16:30:59.000000000 > +0200 > > @@ -17,8 +17,6 @@ > > #include <linux/rtc.h> > > #include <linux/bcd.h> > > > > - > > - > > Unrelated white space change, please revert. OK > > /* We can't determine type by probing, but if we expect pre-Linux > code > > * to have set the chip up as a clock (turning on the oscillator and > > * setting the date and time), Linux can ignore the non-clock > features. > > @@ -38,7 +36,6 @@ enum ds_type { > > // rs5c372 too? different address... > > }; > > > > - > > Unrelated white space change, please revert. OK > > /* RTC registers don't differ much, except for the century flag */ > > #define DS1307_REG_SECS 0x00 /* 00-59 */ > > # define DS1307_BIT_CH 0x80 > > @@ -85,14 +82,11 @@ enum ds_type { > > # define DS1337_BIT_A1I 0x01 > > #define DS1339_REG_TRICKLE 0x10 > > > > - > > - > > Unrelated white space change, please revert. OK > > struct ds1307 { > > u8 reg_addr; > > reg_addr is unused after your changes, so you should remove it as well. OK > > bool has_nvram; > > u8 regs[8]; > > enum ds_type type; > > - struct i2c_msg msg[2]; > > struct i2c_client *client; > > struct i2c_client dev; > > struct rtc_device *rtc; > > @@ -138,12 +132,9 @@ static int ds1307_get_time(struct device > > int tmp; > > > > /* read the RTC date and time registers all at once */ > > - ds1307->msg[1].flags = I2C_M_RD; > > - ds1307->msg[1].len = 7; > > - > > - tmp = i2c_transfer(to_i2c_adapter(ds1307->client->dev.parent), > > - ds1307->msg, 2); > > - if (tmp != 2) { > > + tmp = i2c_smbus_read_i2c_block_data(ds1307->client, > > + DS1307_REG_SECS, 7, ds1307->regs); > > + if (tmp != 7) { > > dev_err(dev, "%s error %d\n", "read", tmp); > > return -EIO; > > } > > @@ -181,8 +172,6 @@ static int ds1307_set_time(struct device { > > struct ds1307 *ds1307 = dev_get_drvdata(dev); > > int result; > > - int tmp; > > - u8 *buf = ds1307->regs; > > > > dev_dbg(dev, "%s secs=%d, mins=%d, " > > "hours=%d, mday=%d, mon=%d, year=%d, wday=%d\n", @@ > > -190,44 +179,41 @@ static int ds1307_set_time(struct device > > t->tm_hour, t->tm_mday, > > t->tm_mon, t->tm_year, t->tm_wday); > > > > - *buf++ = 0; /* first register addr */ > > - buf[DS1307_REG_SECS] = BIN2BCD(t->tm_sec); > > - buf[DS1307_REG_MIN] = BIN2BCD(t->tm_min); > > - buf[DS1307_REG_HOUR] = BIN2BCD(t->tm_hour); > > - buf[DS1307_REG_WDAY] = BIN2BCD(t->tm_wday + 1); > > - buf[DS1307_REG_MDAY] = BIN2BCD(t->tm_mday); > > - buf[DS1307_REG_MONTH] = BIN2BCD(t->tm_mon + 1); > > + ds1307->regs[DS1307_REG_SECS] = BIN2BCD(t->tm_sec); > > + ds1307->regs[DS1307_REG_MIN] = BIN2BCD(t->tm_min); > > + ds1307->regs[DS1307_REG_HOUR] = BIN2BCD(t->tm_hour); > > + ds1307->regs[DS1307_REG_WDAY] = BIN2BCD(t->tm_wday + 1); > > + ds1307->regs[DS1307_REG_MDAY] = BIN2BCD(t->tm_mday); > > + ds1307->regs[DS1307_REG_MONTH] = BIN2BCD(t->tm_mon + 1); > > This change makes the patch larger (and thus harder to review) with > almost no benefit. Same for many changes below... Why don't you keep > the buf pointer? Please keep in mind that your patch should do just > one thing and do it well. It was to avoid the usage of buf, but I can reverse it if you think it's clearer > > /* assume 20YY not 19YY */ > > - tmp = t->tm_year - 100; > > - buf[DS1307_REG_YEAR] = BIN2BCD(tmp); > > + ds1307->regs[DS1307_REG_YEAR] = BIN2BCD(t->tm_year - 100); > > > > switch (ds1307->type) { > > case ds_1337: > > case ds_1339: > > - buf[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY; > > + ds1307->regs[DS1307_REG_MONTH] |= DS1337_BIT_CENTURY; > > break; > > case ds_1340: > > - buf[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN > > - | DS1340_BIT_CENTURY; > > + ds1307->regs[DS1307_REG_HOUR] |= DS1340_BIT_CENTURY_EN > > + | DS1340_BIT_CENTURY; > > break; > > default: > > break; > > } > > > > - ds1307->msg[1].flags = 0; > > - ds1307->msg[1].len = 8; > > - > > dev_dbg(dev, "%s: %02x %02x %02x %02x %02x %02x %02x\n", > > - "write", buf[0], buf[1], buf[2], buf[3], > > - buf[4], buf[5], buf[6]); > > - > > - result = i2c_transfer(to_i2c_adapter(ds1307->client- > >dev.parent), > > - &ds1307->msg[1], 1); > > - if (result != 1) { > > - dev_err(dev, "%s error %d\n", "write", tmp); > > - return -EIO; > > + "write", ds1307->regs[0], ds1307->regs[1], > > + ds1307->regs[2], ds1307->regs[3], > > + ds1307->regs[4], ds1307->regs[5], ds1307->regs[6]); > > + > > + result = i2c_smbus_write_i2c_block_data(ds1307->client, > > + 0, 7, ds1307->regs); > > + if (result < 0) { > > + dev_err(dev, "%s error %d\n", "write", result); > > + return result; > > } > > + > > return 0; > > } > > > > @@ -246,7 +232,6 @@ ds1307_nvram_read(struct kobject *kobj, { > > struct i2c_client *client; > > struct ds1307 *ds1307; > > - struct i2c_msg msg[2]; > > int result; > > > > client = kobj_to_i2c_client(kobj); @@ -259,24 +244,13 @@ > > ds1307_nvram_read(struct kobject *kobj, > > if (unlikely(!count)) > > return count; > > > > - msg[0].addr = client->addr; > > - msg[0].flags = 0; > > - msg[0].len = 1; > > - msg[0].buf = buf; > > - > > - buf[0] = 8 + off; > > - > > - msg[1].addr = client->addr; > > - msg[1].flags = I2C_M_RD; > > - msg[1].len = count; > > - msg[1].buf = buf; > > - > > - result = i2c_transfer(to_i2c_adapter(client->dev.parent), msg, > 2); > > - if (result != 2) { > > - dev_err(&client->dev, "%s error %d\n", "nvram read", > result); > > - return -EIO; > > + result = i2c_smbus_read_i2c_block_data(client, 8 + off, count, > buf); > > + if (result < 0) { > > + dev_err(&client->dev, "%s error %d\n", "read", > > + result); > > Why did you change this error message for a less specific one? I think it's better to use the error value returned from i2c_smbus_read_i2c_block_data than to hard-code an error value. > > + return result; > > } > > - return count; > > + > > + return result; > > } > > Note that "return result" can be factored out. OK > > > > static ssize_t > > @@ -284,8 +258,7 @@ ds1307_nvram_write(struct kobject *kobj, > > char *buf, loff_t off, size_t count) { > > struct i2c_client *client; > > - u8 buffer[NVRAM_SIZE + 1]; > > - int ret; > > + int result; > > > > client = kobj_to_i2c_client(kobj); > > > > @@ -296,11 +269,12 @@ ds1307_nvram_write(struct kobject *kobj, > > if (unlikely(!count)) > > return count; > > > > - buffer[0] = 8 + off; > > - memcpy(buffer + 1, buf, count); > > - > > - ret = i2c_master_send(client, buffer, count + 1); > > - return (ret < 0) ? ret : (ret - 1); > > + result = i2c_smbus_write_i2c_block_data(client, 8 + off, count, > buf); > > + if (result < 0) { > > + dev_err(&client->dev, "%s error %d\n", "write", > result); > > I'd use "nvram write" in the error message. You're right, it's better. New version is attached for tests and comments -- Sébastien Barré Bureau d'étude - Développement SDEL Contrôle Commande D2A - Rue Nungesser et Coli 44860 Saint Aignan de Grand Lieu FRANCE Tél : +33(0)2 40 84 50 88 Fax : +33(0)2 40 84 51 10
patch-rtc-ds1307
Description: patch-rtc-ds1307
_______________________________________________ i2c mailing list i2c@lm-sensors.org http://lists.lm-sensors.org/mailman/listinfo/i2c