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
