Patchwork [3/7] rtc: Convert struct i2c_msg initialization to C99 format

login
register
mail settings
Submitter Datta, Shubhrajyoti
Date Sept. 17, 2012, 1:58 p.m.
Message ID <1347890294-28467-4-git-send-email-shubhrajyoti@ti.com>
Download mbox | patch
Permalink /patch/184408/
State New
Headers show

Comments

Datta, Shubhrajyoti - Sept. 17, 2012, 1:58 p.m.
Convert the struct i2c_msg initialization to C99 format. This makes
    maintaining and editing the code simpler. Also helps once other fields
    like transferred are added in future.

Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
---
 drivers/rtc/rtc-isl1208.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Ryan Mallon - Sept. 18, 2012, 1:45 a.m.
On 17/09/12 23:58, Shubhrajyoti D wrote:
>     Convert the struct i2c_msg initialization to C99 format. This makes
>     maintaining and editing the code simpler. Also helps once other fields
>     like transferred are added in future.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/rtc/rtc-isl1208.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index dd2aeee..c3a76ae 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -68,9 +68,9 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
>  {
>  	u8 reg_addr[1] = { reg };
>  	struct i2c_msg msgs[2] = {
> -		{client->addr, 0, sizeof(reg_addr), reg_addr}
> +		{.addr = client->addr, .flags = 0, .len = sizeof(reg_addr), .buf = reg_addr}
>  		,

Putting the whole initialiser on one line is a bit ugly. Any reason not
to expand it over multiple lines as the previous patch (and majority of
other drivers) does?

~Ryan
Ryan Mallon - Sept. 18, 2012, 1:51 a.m.
On 17/09/12 23:58, Shubhrajyoti D wrote:
>     Convert the struct i2c_msg initialization to C99 format. This makes
>     maintaining and editing the code simpler. Also helps once other fields
>     like transferred are added in future.
> 
> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
>  drivers/rtc/rtc-isl1208.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
> index dd2aeee..c3a76ae 100644
> --- a/drivers/rtc/rtc-isl1208.c
> +++ b/drivers/rtc/rtc-isl1208.c
> @@ -68,9 +68,9 @@ isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
>  {
>  	u8 reg_addr[1] = { reg };
>  	struct i2c_msg msgs[2] = {
> -		{client->addr, 0, sizeof(reg_addr), reg_addr}
> +		{.addr = client->addr, .flags = 0, .len = sizeof(reg_addr), .buf = reg_addr}

Actually, I wonder if it is useful to have something like:

	#define I2C_WRITE(_addr, _buf, _len) {	\
		.addr	= _addr,		\
		.buf	= _buf,			\
		.len	= _len,			\
	}

	#define I2C_READ(_addr, _buf, _len) {	\
		.addr	= _addr,		\
		.buf	= _buf,			\
		.len	= _len,			\
		.flags	= I2C_M_RD,		\
	}

and then write this as:

	struct i2c_msg msgs[2] = {
		I2C_WRITE(client->addr, reg_addr, sizeof(reg_addr)),
		I2C_READ(client->addr, buf, len),
	};

~Ryan
Datta, Shubhrajyoti - Sept. 18, 2012, 5:38 a.m.
On Tuesday 18 September 2012 07:15 AM, Ryan Mallon wrote:
>>  		,
> Putting the whole initialiser on one line is a bit ugly. Any reason not
> to expand it over multiple lines as the previous patch (and majority of
> other drivers) does?
Will do that.
Datta, Shubhrajyoti - Sept. 18, 2012, 5:40 a.m.
On Tuesday 18 September 2012 07:21 AM, Ryan Mallon wrote:
> Actually, I wonder if it is useful to have something like:.
Read and write differ only in the flag also it will be a deviation from
what $SUBJECT
would warrant.  So could be a separate patch.
>
> 	#define I2C_WRITE(_addr, _buf, _len) {	\
> 		.addr	= _addr,		\
> 		.buf	= _buf,			\
> 		.len	= _len,			\
> 	}
>
> 	#define I2C_READ(_addr, _buf, _len) {	\
> 		.addr	= _addr,		\
> 		.buf	= _buf,			\
> 		.len	= _len,			\
> 		.flags	= I2C_M_RD,		\
> 	}
>
> and then write this as:
>
> 	struct i2c_msg msgs[2] = {
> 		I2C_WRITE(client->addr, reg_addr, sizeof(reg_addr)),
> 		I2C_READ(client->addr, buf, len),
> 	};
Ryan Mallon - Sept. 18, 2012, 5:47 a.m.
On 18/09/12 15:40, Shubhrajyoti wrote:
> On Tuesday 18 September 2012 07:21 AM, Ryan Mallon wrote:
>> Actually, I wonder if it is useful to have something like:.
> Read and write differ only in the flag also it will be a deviation from
> what $SUBJECT
> would warrant.  So could be a separate patch.

Sure, but I think it would help make the code even more readable than
just converting to C99 initialisers (especially since putting the C99
initialiser all on one line is hard to read), and there is little sense
in doing one clean up and then replacing it later with a different clean up.

If you are worried about the duplication of code for a single flag
difference you could always do:

  #define I2C_OP(_addr, _buf, _len, _flags)	\
	...

  #define I2C_WRITE(addr, buf, len) I2C_OP(addr, buf, len, 0)
  #define I2C_READ(addr, buf, len)  I2C_OP(addr, buf, len, I2C_M_RD)

~Ryan

>>
>> 	#define I2C_WRITE(_addr, _buf, _len) {	\
>> 		.addr	= _addr,		\
>> 		.buf	= _buf,			\
>> 		.len	= _len,			\
>> 	}
>>
>> 	#define I2C_READ(_addr, _buf, _len) {	\
>> 		.addr	= _addr,		\
>> 		.buf	= _buf,			\
>> 		.len	= _len,			\
>> 		.flags	= I2C_M_RD,		\
>> 	}
>>
>> and then write this as:
>>
>> 	struct i2c_msg msgs[2] = {
>> 		I2C_WRITE(client->addr, reg_addr, sizeof(reg_addr)),
>> 		I2C_READ(client->addr, buf, len),
>> 	};
>
Julia Lawall - Sept. 18, 2012, 5:53 a.m.
On Tue, 18 Sep 2012, Ryan Mallon wrote:

> On 18/09/12 15:40, Shubhrajyoti wrote:
>> On Tuesday 18 September 2012 07:21 AM, Ryan Mallon wrote:
>>> Actually, I wonder if it is useful to have something like:.
>> Read and write differ only in the flag also it will be a deviation from
>> what $SUBJECT
>> would warrant.  So could be a separate patch.
>
> Sure, but I think it would help make the code even more readable than
> just converting to C99 initialisers (especially since putting the C99
> initialiser all on one line is hard to read),

The one line thing is the fault of Coccinelle :)

julia

> and there is little sense
> in doing one clean up and then replacing it later with a different clean up.
>
> If you are worried about the duplication of code for a single flag
> difference you could always do:
>
>  #define I2C_OP(_addr, _buf, _len, _flags)	\
> 	...
>
>  #define I2C_WRITE(addr, buf, len) I2C_OP(addr, buf, len, 0)
>  #define I2C_READ(addr, buf, len)  I2C_OP(addr, buf, len, I2C_M_RD)
>
> ~Ryan
>
>>>
>>> 	#define I2C_WRITE(_addr, _buf, _len) {	\
>>> 		.addr	= _addr,		\
>>> 		.buf	= _buf,			\
>>> 		.len	= _len,			\
>>> 	}
>>>
>>> 	#define I2C_READ(_addr, _buf, _len) {	\
>>> 		.addr	= _addr,		\
>>> 		.buf	= _buf,			\
>>> 		.len	= _len,			\
>>> 		.flags	= I2C_M_RD,		\
>>> 	}
>>>
>>> and then write this as:
>>>
>>> 	struct i2c_msg msgs[2] = {
>>> 		I2C_WRITE(client->addr, reg_addr, sizeof(reg_addr)),
>>> 		I2C_READ(client->addr, buf, len),
>>> 	};
>>
>
>

Patch

diff --git a/drivers/rtc/rtc-isl1208.c b/drivers/rtc/rtc-isl1208.c
index dd2aeee..c3a76ae 100644
--- a/drivers/rtc/rtc-isl1208.c
+++ b/drivers/rtc/rtc-isl1208.c
@@ -68,9 +68,9 @@  isl1208_i2c_read_regs(struct i2c_client *client, u8 reg, u8 buf[],
 {
 	u8 reg_addr[1] = { reg };
 	struct i2c_msg msgs[2] = {
-		{client->addr, 0, sizeof(reg_addr), reg_addr}
+		{.addr = client->addr, .flags = 0, .len = sizeof(reg_addr), .buf = reg_addr}
 		,
-		{client->addr, I2C_M_RD, len, buf}
+		{.addr = client->addr, .flags = I2C_M_RD, .len = len, .buf = buf}
 	};
 	int ret;
 
@@ -90,7 +90,7 @@  isl1208_i2c_set_regs(struct i2c_client *client, u8 reg, u8 const buf[],
 {
 	u8 i2c_buf[ISL1208_REG_USR2 + 2];
 	struct i2c_msg msgs[1] = {
-		{client->addr, 0, len + 1, i2c_buf}
+		{.addr = client->addr, .flags = 0, .len = len + 1, .buf = i2c_buf}
 	};
 	int ret;