| Message ID | 1347890294-28467-4-git-send-email-shubhrajyoti@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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.
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), > };
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), >> }; >
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), >>> }; >> > >
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;
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(-)