[PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

Hi,

This patch change all i2c access functions to SMBus access functions in order to use the ds1307 on SMBus.
I expect that using SMBus access functions is correct for all boards with i2C or SMBus adapter. Is it correct ?

I have tested it on my Geode LX board with a ds1307 device.

Please CC me your comments.
Thanks.

--- a/drivers/rtc/rtc-ds1307.c  2008-09-08 17:40:20.000000000 +0000
+++ b/drivers/rtc/rtc-ds1307.c  2008-10-07 13:21:57.000000000 +0000
@@ -92,7 +92,6 @@ struct ds1307 {
        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 +137,10 @@ 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) {
+       u8      *buf = ds1307->regs;
+       tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+                       DS1307_REG_SECS, 7, buf);
+       if (tmp != 7) {
                dev_err(dev, "%s error %d\n", "read", tmp);
                return -EIO;
        }
@@ -180,7 +177,6 @@ static int ds1307_get_time(struct device
 static int ds1307_set_time(struct device *dev, struct rtc_time *t)
 {
        struct ds1307   *ds1307 = dev_get_drvdata(dev);
-       int             result;
        int             tmp;
        u8              *buf = ds1307->regs;

@@ -190,7 +186,6 @@ 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);
@@ -215,16 +210,12 @@ static int ds1307_set_time(struct device
                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) {
+       tmp = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
+       if (tmp < 0) {
                dev_err(dev, "%s error %d\n", "write", tmp);
                return -EIO;
        }
@@ -246,8 +237,7 @@ ds1307_nvram_read(struct kobject *kobj,
 {
        struct i2c_client       *client;
        struct ds1307           *ds1307;
-       struct i2c_msg          msg[2];
-       int                     result;
+       int     tmp;

        client = kobj_to_i2c_client(kobj);
        ds1307 = i2c_get_clientdata(client);
@@ -259,24 +249,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);
+       tmp = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
+       if (tmp < 0) {
+               dev_err(&client->dev, "%s error %d\n", "read", tmp);
                return -EIO;
        }
-       return count;
+
+       return tmp;
 }

 static ssize_t
@@ -284,8 +263,8 @@ 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;
+       u8                      buffer[NVRAM_SIZE];
+       int                     tmp;

        client = kobj_to_i2c_client(kobj);

@@ -296,11 +275,14 @@ ds1307_nvram_write(struct kobject *kobj,
        if (unlikely(!count))
                return count;

-       buffer[0] = 8 + off;
-       memcpy(buffer + 1, buf, count);
+       memcpy(buffer, buf, count);

-       ret = i2c_master_send(client, buffer, count + 1);
-       return (ret < 0) ? ret : (ret - 1);
+       tmp = i2c_smbus_write_i2c_block_data(client, 8 + off, count, buffer);
+       if (tmp < 0) {
+               dev_err(&client->dev, "%s error %d\n", "write", tmp);
+               return -EIO;
+       }
+       return count;
 }

 static struct bin_attribute nvram = {
@@ -325,11 +307,15 @@ static int __devinit ds1307_probe(struct
        struct ds1307           *ds1307;
        int                     err = -ENODEV;
        int                     tmp;
+       u8                      *buf;
+
        const struct chip_desc  *chip = &chips[id->driver_data];
        struct i2c_adapter      *adapter = to_i2c_adapter(client->dev.parent);

        if (!i2c_check_functionality(adapter,
-                       I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+                       I2C_FUNC_SMBUS_WRITE_BYTE_DATA
+                       | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK
+                       | I2C_FUNC_SMBUS_READ_I2C_BLOCK))
                return -EIO;

        if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -338,35 +324,21 @@ static int __devinit ds1307_probe(struct
        ds1307->client = client;
        i2c_set_clientdata(client, ds1307);

-       ds1307->msg[0].addr = client->addr;
-       ds1307->msg[0].flags = 0;
-       ds1307->msg[0].len = 1;
-       ds1307->msg[0].buf = &ds1307->reg_addr;
-
-       ds1307->msg[1].addr = client->addr;
-       ds1307->msg[1].flags = I2C_M_RD;
-       ds1307->msg[1].len = sizeof(ds1307->regs);
-       ds1307->msg[1].buf = ds1307->regs;
-
        ds1307->type = id->driver_data;

        switch (ds1307->type) {
        case ds_1337:
        case ds_1339:
-               ds1307->reg_addr = DS1337_REG_CONTROL;
-               ds1307->msg[1].len = 2;
-
+               buf = &ds1307->regs[DS1337_REG_CONTROL];
                /* get registers that the "rtc" read below won't read... */
-               tmp = i2c_transfer(adapter, ds1307->msg, 2);
+               tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+                               DS1337_REG_CONTROL, 2, buf);
                if (tmp != 2) {
                        pr_debug("read error %d\n", tmp);
                        err = -EIO;
                        goto exit_free;
                }

-               ds1307->reg_addr = 0;
-               ds1307->msg[1].len = sizeof(ds1307->regs);
-
                /* oscillator off?  turn it on, so clock can tick. */
                if (ds1307->regs[0] & DS1337_BIT_nEOSC)
                        i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -385,9 +357,9 @@ static int __devinit ds1307_probe(struct

 read_rtc:
        /* read RTC registers */
-
-       tmp = i2c_transfer(adapter, ds1307->msg, 2);
-       if (tmp != 2) {
+       buf = ds1307->regs;
+       tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+       if (tmp != 8) {
                pr_debug("read error %d\n", tmp);
                err = -EIO;
                goto exit_free;

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[Jean Delvare]

Bonjour Sebastien,

On Thu, 9 Oct 2008 08:59:17 +0200, BARRE Sebastien wrote:
> Hi,
> 
> This patch change all i2c access functions to SMBus access
> functions in order to use the ds1307 on SMBus.
> I expect that using SMBus access functions is correct for all
> boards with i2C or SMBus adapter. Is it correct ?

Yes, this is correct, and I consider this kind of conversion a good
thing.

Unfortunately your e-mail client replaced all tabs with spaces in the
patch, so I can't apply it, which makes reviewing it much harder.
Here's a first pass anyway, but please fix that and resend the patch in
such a format that I (and other developers) can apply it.

Note that while this patch deals with I2C, it affects an RTC driver so
you should send it to the RTC subsystem maintainer (Alessandro, Cc'd)
and mailing list. You might also want to get the last developers who
touched the rtc-ds1307 driver to test your patch. I've Cc'd them as
well.

> 
> I have tested it on my Geode LX board with a ds1307 device.
> 
> Please CC me your comments.
> Thanks.
> 
> --- a/drivers/rtc/rtc-ds1307.c  2008-09-08 17:40:20.000000000 +0000
> +++ b/drivers/rtc/rtc-ds1307.c  2008-10-07 13:21:57.000000000 +0000
> @@ -92,7 +92,6 @@ struct ds1307 {
>         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 +137,10 @@ 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) {
> +       u8      *buf = ds1307->regs;

Please keep all variable declarations at the beginning of the function.
Not sure you really need a variable for that anyway, as you use it only
once.

> +       tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
> +                       DS1307_REG_SECS, 7, buf);
> +       if (tmp != 7) {
>                 dev_err(dev, "%s error %d\n", "read", tmp);
>                 return -EIO;
>         }
> @@ -180,7 +177,6 @@ static int ds1307_get_time(struct device
>  static int ds1307_set_time(struct device *dev, struct rtc_time *t)
>  {
>         struct ds1307   *ds1307 = dev_get_drvdata(dev);
> -       int             result;
>         int             tmp;
>         u8              *buf = ds1307->regs;
> 
> @@ -190,7 +186,6 @@ 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);
> @@ -215,16 +210,12 @@ static int ds1307_set_time(struct device
>                 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) {
> +       tmp = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
> +       if (tmp < 0) {
>                 dev_err(dev, "%s error %d\n", "write", tmp);
>                 return -EIO;
>         }
> @@ -246,8 +237,7 @@ ds1307_nvram_read(struct kobject *kobj,
>  {
>         struct i2c_client       *client;
>         struct ds1307           *ds1307;
> -       struct i2c_msg          msg[2];
> -       int                     result;
> +       int     tmp;

tmp is the worst variable name you can think of. Why not just keep
result?

> 
>         client = kobj_to_i2c_client(kobj);
>         ds1307 = i2c_get_clientdata(client);
> @@ -259,24 +249,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);
> +       tmp = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
> +       if (tmp < 0) {
> +               dev_err(&client->dev, "%s error %d\n", "read", tmp);
>                 return -EIO;
>         }
> -       return count;
> +
> +       return tmp;
>  }
> 
>  static ssize_t
> @@ -284,8 +263,8 @@ 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;
> +       u8                      buffer[NVRAM_SIZE];
> +       int                     tmp;

Ditto.

> 
>         client = kobj_to_i2c_client(kobj);
> 
> @@ -296,11 +275,14 @@ ds1307_nvram_write(struct kobject *kobj,
>         if (unlikely(!count))
>                 return count;
> 
> -       buffer[0] = 8 + off;
> -       memcpy(buffer + 1, buf, count);
> +       memcpy(buffer, buf, count);

As far as I can see, you no longer need this buffer at all. You can
simply pass buf to i2c_smbus_write_i2c_block_data() below.

> 
> -       ret = i2c_master_send(client, buffer, count + 1);
> -       return (ret < 0) ? ret : (ret - 1);
> +       tmp = i2c_smbus_write_i2c_block_data(client, 8 + off, count, buffer);
> +       if (tmp < 0) {
> +               dev_err(&client->dev, "%s error %d\n", "write", tmp);
> +               return -EIO;

Please do not hard-code error error values, return the values you
received from i2c_smbus_* functions instead.

> +       }
> +       return count;
>  }
> 
>  static struct bin_attribute nvram = {
> @@ -325,11 +307,15 @@ static int __devinit ds1307_probe(struct
>         struct ds1307           *ds1307;
>         int                     err = -ENODEV;
>         int                     tmp;
> +       u8                      *buf;
> +
>         const struct chip_desc  *chip = &chips[id->driver_data];
>         struct i2c_adapter      *adapter = to_i2c_adapter(client->dev.parent);
> 
>         if (!i2c_check_functionality(adapter,
> -                       I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> +                       I2C_FUNC_SMBUS_WRITE_BYTE_DATA
> +                       | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK
> +                       | I2C_FUNC_SMBUS_READ_I2C_BLOCK))

(I2C_FUNC_SMBUS_WRITE_I2C_BLOCK | I2C_FUNC_SMBUS_READ_I2C_BLOCK) has a
shorter form: I2C_FUNC_SMBUS_I2C_BLOCK.

>                 return -EIO;
> 
>         if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
> @@ -338,35 +324,21 @@ static int __devinit ds1307_probe(struct
>         ds1307->client = client;
>         i2c_set_clientdata(client, ds1307);
> 
> -       ds1307->msg[0].addr = client->addr;
> -       ds1307->msg[0].flags = 0;
> -       ds1307->msg[0].len = 1;
> -       ds1307->msg[0].buf = &ds1307->reg_addr;
> -
> -       ds1307->msg[1].addr = client->addr;
> -       ds1307->msg[1].flags = I2C_M_RD;
> -       ds1307->msg[1].len = sizeof(ds1307->regs);
> -       ds1307->msg[1].buf = ds1307->regs;
> -
>         ds1307->type = id->driver_data;
> 
>         switch (ds1307->type) {
>         case ds_1337:
>         case ds_1339:
> -               ds1307->reg_addr = DS1337_REG_CONTROL;
> -               ds1307->msg[1].len = 2;
> -
> +               buf = &ds1307->regs[DS1337_REG_CONTROL];
>                 /* get registers that the "rtc" read below won't read... */
> -               tmp = i2c_transfer(adapter, ds1307->msg, 2);
> +               tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
> +                               DS1337_REG_CONTROL, 2, buf);
>                 if (tmp != 2) {
>                         pr_debug("read error %d\n", tmp);
>                         err = -EIO;
>                         goto exit_free;
>                 }
> 
> -               ds1307->reg_addr = 0;
> -               ds1307->msg[1].len = sizeof(ds1307->regs);
> -
>                 /* oscillator off?  turn it on, so clock can tick. */
>                 if (ds1307->regs[0] & DS1337_BIT_nEOSC)
>                         i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> @@ -385,9 +357,9 @@ static int __devinit ds1307_probe(struct
> 
>  read_rtc:
>         /* read RTC registers */
> -
> -       tmp = i2c_transfer(adapter, ds1307->msg, 2);
> -       if (tmp != 2) {
> +       buf = ds1307->regs;
> +       tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
> +       if (tmp != 8) {
>                 pr_debug("read error %d\n", tmp);
>                 err = -EIO;
>                 goto exit_free;
> 

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

[-- Attachment #1: Type: text/plain, Size: 319 bytes --]

Thanks for your advices.
Fixed patch is in attachement to avoid tabs replacement.
Other comments are welcome.

--
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


[-- Attachment #2: patch-rtc-ds1307 --]
[-- Type: application/octet-stream, Size: 7582 bytes --]

--- 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>
 
-
-
 /* 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...
 };
 
-
 /* 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
 
-
-
 struct ds1307 {
 	u8			reg_addr;
 	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);
 
 	/* 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);
+		return result;
 	}
-	return count;
+
+	return result;
 }
 
 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);
+		return result;
+	}
+	return count;
 }
 
 static struct bin_attribute nvram = {
@@ -325,11 +299,14 @@ static int __devinit ds1307_probe(struct
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp;
+	u8			*buf;
+
 	const struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 
 	if (!i2c_check_functionality(adapter,
-			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+			I2C_FUNC_SMBUS_I2C_BLOCK))
 		return -EIO;
 
 	if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -338,35 +315,21 @@ static int __devinit ds1307_probe(struct
 	ds1307->client = client;
 	i2c_set_clientdata(client, ds1307);
 
-	ds1307->msg[0].addr = client->addr;
-	ds1307->msg[0].flags = 0;
-	ds1307->msg[0].len = 1;
-	ds1307->msg[0].buf = &ds1307->reg_addr;
-
-	ds1307->msg[1].addr = client->addr;
-	ds1307->msg[1].flags = I2C_M_RD;
-	ds1307->msg[1].len = sizeof(ds1307->regs);
-	ds1307->msg[1].buf = ds1307->regs;
-
 	ds1307->type = id->driver_data;
 
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
-		ds1307->reg_addr = DS1337_REG_CONTROL;
-		ds1307->msg[1].len = 2;
-
+		buf = &ds1307->regs[DS1337_REG_CONTROL];
 		/* get registers that the "rtc" read below won't read... */
-		tmp = i2c_transfer(adapter, ds1307->msg, 2);
+		tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+				DS1337_REG_CONTROL, 2, buf);
 		if (tmp != 2) {
 			pr_debug("read error %d\n", tmp);
 			err = -EIO;
 			goto exit_free;
 		}
 
-		ds1307->reg_addr = 0;
-		ds1307->msg[1].len = sizeof(ds1307->regs);
-
 		/* oscillator off?  turn it on, so clock can tick. */
 		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
 			i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -385,9 +348,9 @@ static int __devinit ds1307_probe(struct
 
 read_rtc:
 	/* read RTC registers */
-
-	tmp = i2c_transfer(adapter, ds1307->msg, 2);
-	if (tmp != 2) {
+	buf = ds1307->regs;
+	tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+	if (tmp != 8) {
 		pr_debug("read error %d\n", tmp);
 		err = -EIO;
 		goto exit_free;
@@ -430,7 +393,7 @@ read_rtc:
 		tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
 		if (tmp < 0) {
 			pr_debug("read error %d\n", tmp);
-			err = -EIO;
+			err = tmp;
 			goto exit_free;
 		}
 

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[Jean Delvare]

Hi Sebastien,

On Fri, 10 Oct 2008 15:46:11 +0200, BARRE Sebastien wrote:
> Thanks for your advices.
> Fixed patch is in attachement to avoid tabs replacement.
> Other comments are welcome.

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.

> --- 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.

>  /* 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.

>  /* 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.

>  struct ds1307 {
>  	u8			reg_addr;

reg_addr is unused after your changes, so you should remove it as well.

>  	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.

>  
>  	/* 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?

> +		return result;
>  	}
> -	return count;
> +
> +	return result;
>  }

Note that "return result" can be factored out.

>  
>  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.

> +		return result;
> +	}
> +	return count;
>  }
>  
>  static struct bin_attribute nvram = {
> @@ -325,11 +299,14 @@ static int __devinit ds1307_probe(struct
>  	struct ds1307		*ds1307;
>  	int			err = -ENODEV;
>  	int			tmp;
> +	u8			*buf;
> +
>  	const struct chip_desc	*chip = &chips[id->driver_data];
>  	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
>  
>  	if (!i2c_check_functionality(adapter,
> -			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
> +			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
> +			I2C_FUNC_SMBUS_I2C_BLOCK))
>  		return -EIO;
>  
>  	if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
> @@ -338,35 +315,21 @@ static int __devinit ds1307_probe(struct
>  	ds1307->client = client;
>  	i2c_set_clientdata(client, ds1307);
>  
> -	ds1307->msg[0].addr = client->addr;
> -	ds1307->msg[0].flags = 0;
> -	ds1307->msg[0].len = 1;
> -	ds1307->msg[0].buf = &ds1307->reg_addr;
> -
> -	ds1307->msg[1].addr = client->addr;
> -	ds1307->msg[1].flags = I2C_M_RD;
> -	ds1307->msg[1].len = sizeof(ds1307->regs);
> -	ds1307->msg[1].buf = ds1307->regs;
> -
>  	ds1307->type = id->driver_data;
>  
>  	switch (ds1307->type) {
>  	case ds_1337:
>  	case ds_1339:
> -		ds1307->reg_addr = DS1337_REG_CONTROL;
> -		ds1307->msg[1].len = 2;
> -
> +		buf = &ds1307->regs[DS1337_REG_CONTROL];
>  		/* get registers that the "rtc" read below won't read... */
> -		tmp = i2c_transfer(adapter, ds1307->msg, 2);
> +		tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
> +				DS1337_REG_CONTROL, 2, buf);
>  		if (tmp != 2) {
>  			pr_debug("read error %d\n", tmp);
>  			err = -EIO;
>  			goto exit_free;
>  		}
>  
> -		ds1307->reg_addr = 0;
> -		ds1307->msg[1].len = sizeof(ds1307->regs);
> -
>  		/* oscillator off?  turn it on, so clock can tick. */
>  		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
>  			i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
> @@ -385,9 +348,9 @@ static int __devinit ds1307_probe(struct
>  
>  read_rtc:
>  	/* read RTC registers */
> -
> -	tmp = i2c_transfer(adapter, ds1307->msg, 2);
> -	if (tmp != 2) {
> +	buf = ds1307->regs;
> +	tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
> +	if (tmp != 8) {
>  		pr_debug("read error %d\n", tmp);
>  		err = -EIO;
>  		goto exit_free;
> @@ -430,7 +393,7 @@ read_rtc:
>  		tmp = i2c_smbus_read_byte_data(client, DS1340_REG_FLAG);
>  		if (tmp < 0) {
>  			pr_debug("read error %d\n", tmp);
> -			err = -EIO;
> +			err = tmp;
>  			goto exit_free;
>  		}
>  
> 

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

[-- Attachment #1: Type: text/plain, Size: 8863 bytes --]

Hi Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org]
...
> 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

[-- Attachment #2: patch-rtc-ds1307 --]
[-- Type: application/octet-stream, Size: 5522 bytes --]

This patch change i2c access functions to SMBus access functions
in order to use the ds1307 with SMBus adapter.

Signed-off-by: Sebastien Barre <sbarre@sdelcc.com>

--- a/drivers/rtc/rtc-ds1307.c	2008-09-08 19:40:20.000000000 +0200
+++ b/drivers/rtc/rtc-ds1307.c	2008-10-14 14:22:12.000000000 +0200
@@ -88,11 +88,9 @@ enum ds_type {
 
 
 struct ds1307 {
-	u8			reg_addr;
 	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 +136,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;
 	}
@@ -190,7 +185,6 @@ 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);
@@ -215,18 +209,14 @@ static int ds1307_set_time(struct device
 		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;
+	result = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
+	if (result < 0) {
+		dev_err(dev, "%s error %d\n", "write", result);
+		return result;
 	}
 	return 0;
 }
@@ -246,7 +236,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 +248,10 @@ 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) {
+	result = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
+	if (result < 0)
 		dev_err(&client->dev, "%s error %d\n", "nvram read", result);
-		return -EIO;
-	}
-	return count;
+	return result;
 }
 
 static ssize_t
@@ -284,8 +259,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 +270,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", "nvram write", result);
+		return result;
+	}
+	return count;
 }
 
 static struct bin_attribute nvram = {
@@ -325,11 +300,13 @@ static int __devinit ds1307_probe(struct
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp;
+	u8			*buf;
 	const struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 
 	if (!i2c_check_functionality(adapter,
-			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+			I2C_FUNC_SMBUS_I2C_BLOCK))
 		return -EIO;
 
 	if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -338,35 +315,21 @@ static int __devinit ds1307_probe(struct
 	ds1307->client = client;
 	i2c_set_clientdata(client, ds1307);
 
-	ds1307->msg[0].addr = client->addr;
-	ds1307->msg[0].flags = 0;
-	ds1307->msg[0].len = 1;
-	ds1307->msg[0].buf = &ds1307->reg_addr;
-
-	ds1307->msg[1].addr = client->addr;
-	ds1307->msg[1].flags = I2C_M_RD;
-	ds1307->msg[1].len = sizeof(ds1307->regs);
-	ds1307->msg[1].buf = ds1307->regs;
-
 	ds1307->type = id->driver_data;
 
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
-		ds1307->reg_addr = DS1337_REG_CONTROL;
-		ds1307->msg[1].len = 2;
-
+		buf = &ds1307->regs[DS1337_REG_CONTROL];
 		/* get registers that the "rtc" read below won't read... */
-		tmp = i2c_transfer(adapter, ds1307->msg, 2);
+		tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+				DS1337_REG_CONTROL, 2, buf);
 		if (tmp != 2) {
 			pr_debug("read error %d\n", tmp);
 			err = -EIO;
 			goto exit_free;
 		}
 
-		ds1307->reg_addr = 0;
-		ds1307->msg[1].len = sizeof(ds1307->regs);
-
 		/* oscillator off?  turn it on, so clock can tick. */
 		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
 			i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -385,9 +348,9 @@ static int __devinit ds1307_probe(struct
 
 read_rtc:
 	/* read RTC registers */
-
-	tmp = i2c_transfer(adapter, ds1307->msg, 2);
-	if (tmp != 2) {
+	buf = ds1307->regs;
+	tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+	if (tmp != 8) {
 		pr_debug("read error %d\n", tmp);
 		err = -EIO;
 		goto exit_free;

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[Alessandro Zummo]

On Tue, 14 Oct 2008 15:15:10 +0200
BARRE Sebastien <sbarre-6lXSvc0s5hDQT0dZR+AlfA@public.gmane.org> wrote:

>  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.

 seems almost ok. please send your latest version to me and cc to
 the rtc-list for the final review. if it passed Jean's tests
 I'm confident it's quite ready for inclusion :)

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

> -----Original Message-----
> From: Alessandro Zummo [mailto:alessandro.zummo@towertech.it]
> Sent: Tuesday, October 14, 2008 3:22 PM
> To: BARRE Sebastien
> Cc: Jean Delvare; i2c@lm-sensors.org; frederic Rodo; David Brownell;
> Rodolfo Giometti
> Subject: Re: [i2c] [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility
>
> On Tue, 14 Oct 2008 15:15:10 +0200
> BARRE Sebastien <sbarre@sdelcc.com> wrote:
>
> >  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.
>
>  seems almost ok. please send your latest version to me and cc to
>  the rtc-list for the final review. if it passed Jean's tests
>  I'm confident it's quite ready for inclusion :)

Patch is attached to preserve tabs.

--
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

[-- Attachment #2: patch-rtc-ds1307 --]
[-- Type: application/octet-stream, Size: 5565 bytes --]

This patch change i2c access functions to SMBus access functions
in order to use the ds1307 with SMBus adapter.

Signed-off-by: Sebastien Barre <sbarre@sdelcc.com>
Acked-by: Jean Delvare <khali@linux-fr.org>
--- a/drivers/rtc/rtc-ds1307.c	2008-09-08 19:40:20.000000000 +0200
+++ b/drivers/rtc/rtc-ds1307.c	2008-10-14 14:22:12.000000000 +0200
@@ -88,11 +88,9 @@ enum ds_type {
 
 
 struct ds1307 {
-	u8			reg_addr;
 	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 +136,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;
 	}
@@ -190,7 +185,6 @@ 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);
@@ -215,18 +209,14 @@ static int ds1307_set_time(struct device
 		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;
+	result = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
+	if (result < 0) {
+		dev_err(dev, "%s error %d\n", "write", result);
+		return result;
 	}
 	return 0;
 }
@@ -246,7 +236,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 +248,10 @@ 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) {
+	result = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
+	if (result < 0)
 		dev_err(&client->dev, "%s error %d\n", "nvram read", result);
-		return -EIO;
-	}
-	return count;
+	return result;
 }
 
 static ssize_t
@@ -284,8 +259,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 +270,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", "nvram write", result);
+		return result;
+	}
+	return count;
 }
 
 static struct bin_attribute nvram = {
@@ -325,11 +300,13 @@ static int __devinit ds1307_probe(struct
 	struct ds1307		*ds1307;
 	int			err = -ENODEV;
 	int			tmp;
+	u8			*buf;
 	const struct chip_desc	*chip = &chips[id->driver_data];
 	struct i2c_adapter	*adapter = to_i2c_adapter(client->dev.parent);
 
 	if (!i2c_check_functionality(adapter,
-			I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+			I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+			I2C_FUNC_SMBUS_I2C_BLOCK))
 		return -EIO;
 
 	if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -338,35 +315,21 @@ static int __devinit ds1307_probe(struct
 	ds1307->client = client;
 	i2c_set_clientdata(client, ds1307);
 
-	ds1307->msg[0].addr = client->addr;
-	ds1307->msg[0].flags = 0;
-	ds1307->msg[0].len = 1;
-	ds1307->msg[0].buf = &ds1307->reg_addr;
-
-	ds1307->msg[1].addr = client->addr;
-	ds1307->msg[1].flags = I2C_M_RD;
-	ds1307->msg[1].len = sizeof(ds1307->regs);
-	ds1307->msg[1].buf = ds1307->regs;
-
 	ds1307->type = id->driver_data;
 
 	switch (ds1307->type) {
 	case ds_1337:
 	case ds_1339:
-		ds1307->reg_addr = DS1337_REG_CONTROL;
-		ds1307->msg[1].len = 2;
-
+		buf = &ds1307->regs[DS1337_REG_CONTROL];
 		/* get registers that the "rtc" read below won't read... */
-		tmp = i2c_transfer(adapter, ds1307->msg, 2);
+		tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+				DS1337_REG_CONTROL, 2, buf);
 		if (tmp != 2) {
 			pr_debug("read error %d\n", tmp);
 			err = -EIO;
 			goto exit_free;
 		}
 
-		ds1307->reg_addr = 0;
-		ds1307->msg[1].len = sizeof(ds1307->regs);
-
 		/* oscillator off?  turn it on, so clock can tick. */
 		if (ds1307->regs[0] & DS1337_BIT_nEOSC)
 			i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -385,9 +348,9 @@ static int __devinit ds1307_probe(struct
 
 read_rtc:
 	/* read RTC registers */
-
-	tmp = i2c_transfer(adapter, ds1307->msg, 2);
-	if (tmp != 2) {
+	buf = ds1307->regs;
+	tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+	if (tmp != 8) {
 		pr_debug("read error %d\n", tmp);
 		err = -EIO;
 		goto exit_free;

[-- Attachment #3: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[Alessandro Zummo]

On Tue, 14 Oct 2008 17:57:29 +0200
BARRE Sebastien <sbarre-6lXSvc0s5hDQT0dZR+AlfA@public.gmane.org> wrote:

> >  seems almost ok. please send your latest version to me and cc to
> >  the rtc-list for the final review. if it passed Jean's tests
> >  I'm confident it's quite ready for inclusion :)
> 
> Patch is attached to preserve tabs.

 bzzzt. wrong! patches must be submitted inline


-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

This patch change i2c access functions to SMBus access functions
in order to use the ds1307 with SMBus adapter.

Signed-off-by: Sebastien Barre <sbarre@sdelcc.com>
Acked-by: Jean Delvare <khali@linux-fr.org>

--- a/drivers/rtc/rtc-ds1307.c  2008-09-08 19:40:20.000000000 +0200
+++ b/drivers/rtc/rtc-ds1307.c  2008-10-14 14:22:12.000000000 +0200
@@ -88,11 +88,9 @@ enum ds_type {


 struct ds1307 {
-       u8                      reg_addr;
        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 +136,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;
        }
@@ -190,7 +185,6 @@ 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);
@@ -215,18 +209,14 @@ static int ds1307_set_time(struct device
                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;
+       result = i2c_smbus_write_i2c_block_data(ds1307->client, 0, 7, buf);
+       if (result < 0) {
+               dev_err(dev, "%s error %d\n", "write", result);
+               return result;
        }
        return 0;
 }
@@ -246,7 +236,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 +248,10 @@ 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) {
+       result = i2c_smbus_read_i2c_block_data(client, 8 + off, count, buf);
+       if (result < 0)
                dev_err(&client->dev, "%s error %d\n", "nvram read", result);
-               return -EIO;
-       }
-       return count;
+       return result;
 }

 static ssize_t
@@ -284,8 +259,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 +270,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", "nvram write", result);
+               return result;
+       }
+       return count;
 }

 static struct bin_attribute nvram = {
@@ -325,11 +300,13 @@ static int __devinit ds1307_probe(struct
        struct ds1307           *ds1307;
        int                     err = -ENODEV;
        int                     tmp;
+       u8                      *buf;
        const struct chip_desc  *chip = &chips[id->driver_data];
        struct i2c_adapter      *adapter = to_i2c_adapter(client->dev.parent);

        if (!i2c_check_functionality(adapter,
-                       I2C_FUNC_I2C | I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
+                       I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
+                       I2C_FUNC_SMBUS_I2C_BLOCK))
                return -EIO;

        if (!(ds1307 = kzalloc(sizeof(struct ds1307), GFP_KERNEL)))
@@ -338,35 +315,21 @@ static int __devinit ds1307_probe(struct
        ds1307->client = client;
        i2c_set_clientdata(client, ds1307);

-       ds1307->msg[0].addr = client->addr;
-       ds1307->msg[0].flags = 0;
-       ds1307->msg[0].len = 1;
-       ds1307->msg[0].buf = &ds1307->reg_addr;
-
-       ds1307->msg[1].addr = client->addr;
-       ds1307->msg[1].flags = I2C_M_RD;
-       ds1307->msg[1].len = sizeof(ds1307->regs);
-       ds1307->msg[1].buf = ds1307->regs;
-
        ds1307->type = id->driver_data;

        switch (ds1307->type) {
        case ds_1337:
        case ds_1339:
-               ds1307->reg_addr = DS1337_REG_CONTROL;
-               ds1307->msg[1].len = 2;
-
+               buf = &ds1307->regs[DS1337_REG_CONTROL];
                /* get registers that the "rtc" read below won't read... */
-               tmp = i2c_transfer(adapter, ds1307->msg, 2);
+               tmp = i2c_smbus_read_i2c_block_data(ds1307->client,
+                               DS1337_REG_CONTROL, 2, buf);
                if (tmp != 2) {
                        pr_debug("read error %d\n", tmp);
                        err = -EIO;
                        goto exit_free;
                }

-               ds1307->reg_addr = 0;
-               ds1307->msg[1].len = sizeof(ds1307->regs);
-
                /* oscillator off?  turn it on, so clock can tick. */
                if (ds1307->regs[0] & DS1337_BIT_nEOSC)
                        i2c_smbus_write_byte_data(client, DS1337_REG_CONTROL,
@@ -385,9 +348,9 @@ static int __devinit ds1307_probe(struct

 read_rtc:
        /* read RTC registers */
-
-       tmp = i2c_transfer(adapter, ds1307->msg, 2);
-       if (tmp != 2) {
+       buf = ds1307->regs;
+       tmp = i2c_smbus_read_i2c_block_data(ds1307->client, 0, 8, buf);
+       if (tmp != 8) {
                pr_debug("read error %d\n", tmp);
                err = -EIO;
                goto exit_free;

--
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



_______________________________________________
i2c mailing list
i2c@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[David Brownell]

On Tuesday 14 October 2008, Alessandro Zummo wrote:
> On Tue, 14 Oct 2008 17:57:29 +0200
> BARRE Sebastien <sbarre-6lXSvc0s5hDQT0dZR+AlfA@public.gmane.org> wrote:
> 
> > >  seems almost ok. please send your latest version to me and cc to
> > >  the rtc-list for the final review. if it passed Jean's tests
> > >  I'm confident it's quite ready for inclusion :)
> > 
> > Patch is attached to preserve tabs.
> 
>  bzzzt. wrong! patches must be submitted inline

Though that's not a hard-and-fast policy; when
the submitter can't get relevant mailers to avoid
mangling the patches, it's better to attach them
than otherwise.  However, see

  Documentation/email-clients.txt

More generally, this patch isn't much good since
it doesn't even *apply* ... on top of a pending
patch to the same driver, which has been in the
MM tree since late July or so.

My suggestion:  since that patch was recently
(last night) sent to Linus for merging, wait
until it's in mainline GIT.  Then refresh and
re-send the patch.

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[Jean Delvare]

On Tue, 14 Oct 2008 15:15:10 +0200, BARRE Sebastien wrote:
> New version is attached for tests and comments

Looks alright to me.

Acked-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[David Brownell]

On Tuesday 14 October 2008, Jean Delvare wrote:
> On Tue, 14 Oct 2008 15:15:10 +0200, BARRE Sebastien wrote:
> > New version is attached for tests and comments

Could I see it too?


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

> -----Original Message-----
> From: David Brownell [mailto:david-b@pacbell.net]
> Sent: Tuesday, October 14, 2008 5:47 PM
> To: Jean Delvare
> Cc: BARRE Sebastien; i2c@lm-sensors.org; Alessandro Zummo; frederic Rodo;
> Rodolfo Giometti
> Subject: Re: [i2c] [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility
>
> On Tuesday 14 October 2008, Jean Delvare wrote:
> > On Tue, 14 Oct 2008 15:15:10 +0200, BARRE Sebastien wrote:
> > > New version is attached for tests and comments
>
> Could I see it too?

Sorry David, you were in CC of this message but I received an error for your mail adress :

>[<00>] XMail bounce: Rcpt=[david-b@pacbell.net];Error=[553 5.3.0 flpi117 - m9EFx6OS014455, DNSBL:521< 81.252.86.87 >>_is_blocked.__For_information_see_http://att.net/blocks]
>
>
>[<01>] Error sending message [1223999945380.3097504688.2a381e.smtp4] from [vinci-energies.com].
>
>ID:        <SCDCBC5>
>Mail From: <sbarre@sdelcc.com>
>Rcpt To:   <david-b@pacbell.net>
>Server:    <ff-mx-vip4b.prodigy.net> [207.115.21.23]
>
>
>[<02>] The reason of the delivery failure was:
>
>553 5.3.0 flpi117 - m9EFx6OS014455, DNSBL:521< 81.252.86.87 >_is_blocked.__For_information_see_http://att.net/blocks

My mails are blocked by a server.

Jean, Could you forward this message to David.
Thanks.

--
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
_______________________________________________
i2c mailing list
i2c@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[David Brownell]

On Friday 10 October 2008, Jean Delvare wrote:
> Unfortunately your e-mail client replaced all tabs with spaces in the
> patch, so I can't apply it, which makes reviewing it much harder.
> Here's a first pass anyway, but please fix that and resend the patch in
> such a format that I (and other developers) can apply it.

... and test it.

At a quick glance, this looks like a sane conversion ... unlike the last
one proposed, which I had to NAK since it broke driver functionality by
trying to replace block transfers with non-equivalent byte-at-a-time ones.
(Clock updates between bytes, boom!)


> Note that while this patch deals with I2C, it affects an RTC driver so
> you should send it to the RTC subsystem maintainer (Alessandro, Cc'd)
> and mailing list. You might also want to get the last developers who
> touched the rtc-ds1307 driver to test your patch. I've Cc'd them as
> well.

Right.  As a rule, use GIT history to see who's maintaining code
when there's no MAINTAINERS entry.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 2.6.26.5] rtc-ds1307 : SMBus compatibility

[BARRE Sebastien]

> -----Original Message-----
> From: David Brownell [mailto:david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org]
...
> At a quick glance, this looks like a sane conversion ... unlike the
> last
> one proposed, which I had to NAK since it broke driver functionality by
> trying to replace block transfers with non-equivalent byte-at-a-time
> ones.
> (Clock updates between bytes, boom!)

Sorry, I don't understand what you talk about.
The i2c_smbus_read_i2c_block_data function do block transfer not byte-at-a-time one. Doesn't it ?