2012-10-07 15:39:39

by Julia Lawall

Subject: [PATCH 0/11] introduce macros for i2c_msg initialization

This patch set introduces some macros for describing how an i2c_msg is
being initialized. There are three macros: I2C_MSG_READ, for a read
message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other
kind of message, which is expected to be very rarely used.

Some i2c_msg initializations have been updated accordingly using the
following semantic patch:

// <smpl>
@r@
field list[n] ds;
type T;
identifier i;
@@

struct i2c_msg {
ds
T i;
...
};

@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@

struct i2c_msg nm = {
is,
- a
+ .i = a
,...
};

@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@

struct i2c_msg nm[...] = { ..., {
is,
- a
+ .i = a
,...}, ...};

@@
initializer list[r.n] is;
expression a;
identifier nm;
identifier r.i;
@@

struct i2c_msg nm[] = { ..., {
is,
- a
+ .i = a
,...}, ...};

// --------------------------------------------------------------------
// ensure everyone has all fields, pointer case first

@rt@
type T;
identifier i;
@@

struct i2c_msg {
...
T *i;
...
};

@t1@
expression e;
identifier nm,rt.i;
position p;
@@

struct i2c_msg nm = {@p
.i = e,
};

@@
identifier nm,rt.i;
position p!= t1.p;
@@

struct i2c_msg nm = {@p
+ .i = NULL,
...
};

@t2@
expression e;
identifier nm,rt.i;
position p;
@@

struct i2c_msg nm[] = { ..., {@p
.i = e,
}, ...};

@@
identifier nm,rt.i;
position p!= t2.p;
@@

struct i2c_msg nm[] = { ..., {@p
+ .i = NULL,
...
}, ...};

@t3@
expression e;
identifier nm,rt.i;
position p;
@@

struct i2c_msg nm[...] = { ..., {@p
.i = e,
}, ...};

@@
identifier nm,rt.i;
position p!= t3.p;
@@

struct i2c_msg nm[...] = { ..., {@p
+ .i = NULL,
...
}, ...};

// ---------------------------------

@f1@
expression e;
identifier nm,r.i;
position p;
@@

struct i2c_msg nm = {@p
.i = e,
};

@@
identifier nm,r.i;
position p!= f1.p;
@@

struct i2c_msg nm = {@p
+ .i = 0,
...
};

@f2@
expression e;
identifier nm,r.i;
position p;
@@

struct i2c_msg nm[] = { ..., {@p
.i = e,
}, ...};

@@
identifier nm,r.i;
position p!= f2.p;
@@

struct i2c_msg nm[] = { ..., {@p
+ .i = 0,
...
}, ...};

@f3@
expression e;
identifier nm,r.i;
position p;
@@

struct i2c_msg nm[...] = { ..., {@p
.i = e,
}, ...};

@@
identifier nm,r.i;
position p!= f3.p;
@@

struct i2c_msg nm[...] = { ..., {@p
+ .i = 0,
...
}, ...};

// --------------------------------------------------------------------

@@
constant c;
identifier x;
type T,T1;
T[] b;
@@

struct i2c_msg x =
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
};

@@
constant c;
identifier x;
type T,T1;
T[] b;
@@

struct i2c_msg x[...] = {...,
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};

@@
constant c;
identifier x;
type T,T1;
T[] b;
@@

struct i2c_msg x[] = {...,
{ .buf = \((T1)b\|(T1)(&b)\|(T1)(&b[0])\), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};

@@
constant c;
identifier x;
type T1;
expression b;
@@

struct i2c_msg x =
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
};

@@
constant c;
identifier x;
type T1;
expression b;
@@

struct i2c_msg x[...] = {...,
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};

@@
constant c;
identifier x;
type T1;
expression b;
@@

struct i2c_msg x[] = {...,
{ .buf = (T1)(&b), .len =
(
0
|
sizeof (...)
|
- c
+ sizeof(b)
)
} ,...};

// --------------------------------------------------------------------

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

// has to come before the next rule, which matcher fewer fields
@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;

// --------------------------------------------------------------------

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
,...};

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
,...};

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x[] = {...,
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
,...};

// --------------------------------------------------------------------

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
,...};

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
,...};

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x[...] = {...,
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
,...};

// --------------------------------------------------------------------
// got everything?

@check1@
identifier nm;
position p;
@@

struct i2c_msg nm@p = {...};

@script:python@
p << check1.p;
@@

cocci.print_main("",p)

@check2@
identifier nm;
position p;
@@

struct i2c_msg nm@p [] = {...,{...},...};

@script:python@
p << check2.p;
@@

cocci.print_main("",p)

@check3@
identifier nm;
position p;
@@

struct i2c_msg nm@p [...] = {...,{...},...};

@script:python@
p << check3.p;
@@

cocci.print_main("",p)
// </smpl>

2012-10-07 15:39:25

by Julia Lawall

Subject: [PATCH 1/13] include/linux/i2c.h: introduce macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

This patch introduces some macros for describing how an i2c_msg is being
initialized. There are three macros: I2C_MSG_READ, for a read message,
I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other kind of
message, which is expected to be very rarely used.

Signed-off-by: Julia Lawall <[email protected]>

---
include/linux/i2c.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 94aed0c..885ebea 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -556,6 +556,12 @@ struct i2c_msg {
__u8 *buf; /* pointer to msg data */
};

+#define I2C_MSG_OP(_addr, _buf, _len, _flags) \
+ { .addr = _addr, .buf = _buf, .len = _len, .flags = _flags }
+
+#define I2C_MSG_WRITE(addr, buf, len) I2C_MSG_OP(addr, buf, len, 0)
+#define I2C_MSG_READ(addr, buf, len) I2C_MSG_OP(addr, buf, len, I2C_M_RD)
+
/* To determine what functionality is present */

#define I2C_FUNC_I2C 0x00000001

2012-10-07 15:39:56

by Julia Lawall

Subject: [PATCH 4/13] drivers/media/tuners/tda18212.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

In the second initialization, a length expressed as an explicit constant is
also re-expressed as the size of the buffer (reg).

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/tda18212.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/media/tuners/tda18212.c b/drivers/media/tuners/tda18212.c
index 5d9f028..fb810ce 100644
--- a/drivers/media/tuners/tda18212.c
+++ b/drivers/media/tuners/tda18212.c
@@ -34,12 +34,7 @@ static int tda18212_wr_regs(struct tda18212_priv *priv, u8 reg, u8 *val,
int ret;
u8 buf[len+1];
struct i2c_msg msg[1] = {
- {
- .addr = priv->cfg->i2c_address,
- .flags = 0,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_address, buf, sizeof(buf))
};

buf[0] = reg;
@@ -63,17 +58,8 @@ static int tda18212_rd_regs(struct tda18212_priv *priv, u8 reg, u8 *val,
int ret;
u8 buf[len];
struct i2c_msg msg[2] = {
- {
- .addr = priv->cfg->i2c_address,
- .flags = 0,
- .len = 1,
- .buf = &reg,
- }, {
- .addr = priv->cfg->i2c_address,
- .flags = I2C_M_RD,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_address, buf, sizeof(buf))
};

ret = i2c_transfer(priv->i2c, msg, 2);

2012-10-07 15:40:04

by Julia Lawall

Subject: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the buffer, when this is possible.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/qt1010.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/media/tuners/qt1010.c b/drivers/media/tuners/qt1010.c
index bc419f8..37ff254 100644
--- a/drivers/media/tuners/qt1010.c
+++ b/drivers/media/tuners/qt1010.c
@@ -25,10 +25,8 @@
static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->cfg->i2c_address,
- .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->cfg->i2c_address,
- .flags = I2C_M_RD, .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {
@@ -43,8 +41,8 @@ static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val)
static int qt1010_writereg(struct qt1010_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = { reg, val };
- struct i2c_msg msg = { .addr = priv->cfg->i2c_address,
- .flags = 0, .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
+ sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
dev_warn(&priv->i2c->dev, "%s: i2c wr failed reg=%02x\n",

2012-10-07 15:40:13

by Julia Lawall

Subject: [PATCH 6/13] drivers/media/tuners/tda18271-common.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---

drivers/media/tuners/tda18271-common.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/media/tuners/tda18271-common.c b/drivers/media/tuners/tda18271-common.c
index 221171e..d05ed53 100644
--- a/drivers/media/tuners/tda18271-common.c
+++ b/drivers/media/tuners/tda18271-common.c
@@ -125,10 +125,8 @@ int tda18271_read_regs(struct dvb_frontend *fe)
unsigned char buf = 0x00;
int ret;
struct i2c_msg msg[] = {
- { .addr = priv->i2c_props.addr, .flags = 0,
- .buf = &buf, .len = 1 },
- { .addr = priv->i2c_props.addr, .flags = I2C_M_RD,
- .buf = regs, .len = 16 }
+ I2C_MSG_WRITE(priv->i2c_props.addr, &buf, sizeof(buf)),
+ I2C_MSG_READ(priv->i2c_props.addr, regs, 16)
};

tda18271_i2c_gate_ctrl(fe, 1);
@@ -155,10 +153,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
unsigned char buf = 0x00;
int ret, i;
struct i2c_msg msg[] = {
- { .addr = priv->i2c_props.addr, .flags = 0,
- .buf = &buf, .len = 1 },
- { .addr = priv->i2c_props.addr, .flags = I2C_M_RD,
- .buf = regdump, .len = TDA18271_NUM_REGS }
+ I2C_MSG_WRITE(priv->i2c_props.addr, &buf, sizeof(buf)),
+ I2C_MSG_READ(priv->i2c_props.addr, regdump, sizeof(regdump))
};

tda18271_i2c_gate_ctrl(fe, 1);
@@ -192,8 +188,7 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
struct tda18271_priv *priv = fe->tuner_priv;
unsigned char *regs = priv->tda18271_regs;
unsigned char buf[TDA18271_NUM_REGS + 1];
- struct i2c_msg msg = { .addr = priv->i2c_props.addr, .flags = 0,
- .buf = buf };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, 0);
int i, ret = 1, max;

BUG_ON((len == 0) || (idx + len > sizeof(buf)));

2012-10-07 15:40:22

by Julia Lawall

Subject: [PATCH 7/13] drivers/media/tuners/tua9001.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/tua9001.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/tuners/tua9001.c b/drivers/media/tuners/tua9001.c
index 3896684..a9e7e91 100644
--- a/drivers/media/tuners/tua9001.c
+++ b/drivers/media/tuners/tua9001.c
@@ -27,12 +27,7 @@ static int tua9001_wr_reg(struct tua9001_priv *priv, u8 reg, u16 val)
int ret;
u8 buf[3] = { reg, (val >> 8) & 0xff, (val >> 0) & 0xff };
struct i2c_msg msg[1] = {
- {
- .addr = priv->cfg->i2c_addr,
- .flags = 0,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
};

ret = i2c_transfer(priv->i2c, msg, 1);

2012-10-07 15:40:30

by Julia Lawall

Subject: [PATCH 11/13] drivers/media/tuners/tda18218.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/tda18218.c | 19 +++----------------
1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/media/tuners/tda18218.c b/drivers/media/tuners/tda18218.c
index 1819853..c0c2fef 100644
--- a/drivers/media/tuners/tda18218.c
+++ b/drivers/media/tuners/tda18218.c
@@ -26,11 +26,7 @@ static int tda18218_wr_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len)
int ret = 0, len2, remaining;
u8 buf[1 + len];
struct i2c_msg msg[1] = {
- {
- .addr = priv->cfg->i2c_address,
- .flags = 0,
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_address, buf, 0)
};

for (remaining = len; remaining > 0;
@@ -65,17 +61,8 @@ static int tda18218_rd_regs(struct tda18218_priv *priv, u8 reg, u8 *val, u8 len)
int ret;
u8 buf[reg+len]; /* we must start read always from reg 0x00 */
struct i2c_msg msg[2] = {
- {
- .addr = priv->cfg->i2c_address,
- .flags = 0,
- .len = 1,
- .buf = "\x00",
- }, {
- .addr = priv->cfg->i2c_address,
- .flags = I2C_M_RD,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_address, "\x00", 1),
+ I2C_MSG_READ(priv->cfg->i2c_address, buf, sizeof(buf))
};

ret = i2c_transfer(priv->i2c, msg, 2);

2012-10-07 15:40:34

by Julia Lawall

Subject: [PATCH 10/13] drivers/media/tuners/tda8290.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the bufferin each case.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/tda8290.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/media/tuners/tda8290.c b/drivers/media/tuners/tda8290.c
index 8c48521..83ecee2 100644
--- a/drivers/media/tuners/tda8290.c
+++ b/drivers/media/tuners/tda8290.c
@@ -463,7 +463,8 @@ static void tda8290_standby(struct dvb_frontend *fe)
unsigned char cb1[] = { 0x30, 0xD0 };
unsigned char tda8290_standby[] = { 0x00, 0x02 };
unsigned char tda8290_agc_tri[] = { 0x02, 0x20 };
- struct i2c_msg msg = {.addr = priv->tda827x_addr, .flags=0, .buf=cb1, .len = 2};
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->tda827x_addr, cb1,
+ sizeof(cb1));

tda8290_i2c_bridge(fe, 1);
if (priv->ver & TDA8275A)
@@ -532,8 +533,8 @@ static void tda8290_init_tuner(struct dvb_frontend *fe)
0x3F, 0x2A, 0x04, 0xFF, 0x00, 0x00, 0x40 };
unsigned char tda8275a_init[] = { 0x00, 0x00, 0x00, 0x00, 0xdC, 0x05, 0x8b,
0x0c, 0x04, 0x20, 0xFF, 0x00, 0x00, 0x4b };
- struct i2c_msg msg = {.addr = priv->tda827x_addr, .flags=0,
- .buf=tda8275_init, .len = 14};
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->tda827x_addr, tda8275_init,
+ sizeof(tda8275_init));
if (priv->ver & TDA8275A)
msg.buf = tda8275a_init;

@@ -569,7 +570,7 @@ static int tda829x_find_tuner(struct dvb_frontend *fe)
int i, ret, tuners_found;
u32 tuner_addrs;
u8 data;
- struct i2c_msg msg = { .flags = I2C_M_RD, .buf = &data, .len = 1 };
+ struct i2c_msg msg = I2C_MSG_READ(0, &data, sizeof(data));

if (!analog_ops->i2c_gate_ctrl) {
printk(KERN_ERR "tda8290: no gate control were provided!\n");
@@ -658,8 +659,8 @@ static int tda8290_probe(struct tuner_i2c_props *i2c_props)
#define TDA8290_ID 0x89
u8 reg = 0x1f, id;
struct i2c_msg msg_read[] = {
- { .addr = i2c_props->addr, .flags = 0, .len = 1, .buf = &reg },
- { .addr = i2c_props->addr, .flags = I2C_M_RD, .len = 1, .buf = &id },
+ I2C_MSG_WRITE(i2c_props->addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(i2c_props->addr, &id, sizeof(id)),
};

/* detect tda8290 */
@@ -685,8 +686,8 @@ static int tda8295_probe(struct tuner_i2c_props *i2c_props)
#define TDA8295C2_ID 0x8b
u8 reg = 0x2f, id;
struct i2c_msg msg_read[] = {
- { .addr = i2c_props->addr, .flags = 0, .len = 1, .buf = &reg },
- { .addr = i2c_props->addr, .flags = I2C_M_RD, .len = 1, .buf = &id },
+ I2C_MSG_WRITE(i2c_props->addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(i2c_props->addr, &id, sizeof(id)),
};

/* detect tda8295 */

2012-10-07 15:40:41

by Julia Lawall

Subject: [PATCH 8/13] drivers/media/tuners/fc2580.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the buffer in each case.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/fc2580.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/media/tuners/fc2580.c b/drivers/media/tuners/fc2580.c
index aff39ae..d0c0ff1 100644
--- a/drivers/media/tuners/fc2580.c
+++ b/drivers/media/tuners/fc2580.c
@@ -45,12 +45,7 @@ static int fc2580_wr_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len)
int ret;
u8 buf[1 + len];
struct i2c_msg msg[1] = {
- {
- .addr = priv->cfg->i2c_addr,
- .flags = 0,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
};

buf[0] = reg;
@@ -73,17 +68,8 @@ static int fc2580_rd_regs(struct fc2580_priv *priv, u8 reg, u8 *val, int len)
int ret;
u8 buf[len];
struct i2c_msg msg[2] = {
- {
- .addr = priv->cfg->i2c_addr,
- .flags = 0,
- .len = 1,
- .buf = &reg,
- }, {
- .addr = priv->cfg->i2c_addr,
- .flags = I2C_M_RD,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf))
};

ret = i2c_transfer(priv->i2c, msg, 2);

2012-10-07 15:40:49

by Julia Lawall

Subject: [PATCH 12/13] drivers/media/tuners/max2165.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the buffer, when this is possible.

The second case is simplified to use simple variables rather than arrays.
The variable b0 is dropped completely, and the variable reg that it
contains is used instead. The variable b1 is replaced by a u8-typed
variable named buf (the name used earlier in the file). The uses of b1 are
then adjusted accordingly.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---

drivers/media/tuners/max2165.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/media/tuners/max2165.c b/drivers/media/tuners/max2165.c
index ba84936..6638617 100644
--- a/drivers/media/tuners/max2165.c
+++ b/drivers/media/tuners/max2165.c
@@ -47,7 +47,7 @@ static int max2165_write_reg(struct max2165_priv *priv, u8 reg, u8 data)
{
int ret;
u8 buf[] = { reg, data };
- struct i2c_msg msg = { .flags = 0, .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));

msg.addr = priv->config->i2c_address;

@@ -68,11 +68,10 @@ static int max2165_read_reg(struct max2165_priv *priv, u8 reg, u8 *p_data)
int ret;
u8 dev_addr = priv->config->i2c_address;

- u8 b0[] = { reg };
- u8 b1[] = { 0 };
+ u8 buf;
struct i2c_msg msg[] = {
- { .addr = dev_addr, .flags = 0, .buf = b0, .len = 1 },
- { .addr = dev_addr, .flags = I2C_M_RD, .buf = b1, .len = 1 },
+ I2C_MSG_WRITE(dev_addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(dev_addr, &buf, sizeof(buf)),
};

ret = i2c_transfer(priv->i2c, msg, 2);
@@ -81,10 +80,10 @@ static int max2165_read_reg(struct max2165_priv *priv, u8 reg, u8 *p_data)
return -EIO;
}

- *p_data = b1[0];
+ *p_data = buf;
if (debug >= 2)
dprintk("%s: reg=0x%02X, data=0x%02X\n",
- __func__, reg, b1[0]);
+ __func__, reg, buf);
return 0;
}

2012-10-07 15:40:55

by Julia Lawall

Subject: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the buffer in each case.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/fc0011.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
index e488254..5dbba98 100644
--- a/drivers/media/tuners/fc0011.c
+++ b/drivers/media/tuners/fc0011.c
@@ -80,8 +80,7 @@ struct fc0011_priv {
static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = { reg, val };
- struct i2c_msg msg = { .addr = priv->addr,
- .flags = 0, .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
dev_err(&priv->i2c->dev,
@@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 reg, u8 *val)
{
u8 dummy;
struct i2c_msg msg[2] = {
- { .addr = priv->addr,
- .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->addr,
- .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
+ I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {

2012-10-07 15:41:01

by Julia Lawall

Subject: [PATCH 5/13] drivers/media/tuners: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

A length expressed as an explicit constant is also re-expressed as the size
of the buffer, when this is possible.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/fc0012.c | 8 +++-----
drivers/media/tuners/fc0013.c | 8 +++-----
drivers/media/tuners/mc44s803.c | 8 +++-----
drivers/media/tuners/mt2060.c | 13 +++++--------
drivers/media/tuners/mt2063.c | 23 ++++++-----------------
drivers/media/tuners/mt2131.c | 13 +++++--------
drivers/media/tuners/mt2266.c | 13 +++++--------
drivers/media/tuners/mxl5005s.c | 8 ++++----
drivers/media/tuners/tda827x.c | 29 +++++++++++------------------
drivers/media/tuners/tuner-i2c.h | 12 ++++--------
drivers/media/tuners/tuner-simple.c | 5 +----
drivers/media/tuners/xc4000.c | 9 +++------
drivers/media/tuners/xc5000.c | 9 +++------
13 files changed, 56 insertions(+), 102 deletions(-)

diff --git a/drivers/media/tuners/fc0012.c b/drivers/media/tuners/fc0012.c
index 308135a..01dac7e 100644
--- a/drivers/media/tuners/fc0012.c
+++ b/drivers/media/tuners/fc0012.c
@@ -24,9 +24,7 @@
static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = {reg, val};
- struct i2c_msg msg = {
- .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
@@ -38,8 +36,8 @@ static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
static int fc0012_readreg(struct fc0012_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->addr, val, 1),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {
diff --git a/drivers/media/tuners/fc0013.c b/drivers/media/tuners/fc0013.c
index bd8f0f1..174f0b0 100644
--- a/drivers/media/tuners/fc0013.c
+++ b/drivers/media/tuners/fc0013.c
@@ -27,9 +27,7 @@
static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = {reg, val};
- struct i2c_msg msg = {
- .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
@@ -41,8 +39,8 @@ static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
static int fc0013_readreg(struct fc0013_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->addr, val, 1),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {
diff --git a/drivers/media/tuners/mc44s803.c b/drivers/media/tuners/mc44s803.c
index f1b7640..df47e03 100644
--- a/drivers/media/tuners/mc44s803.c
+++ b/drivers/media/tuners/mc44s803.c
@@ -37,9 +37,8 @@
static int mc44s803_writereg(struct mc44s803_priv *priv, u32 val)
{
u8 buf[3];
- struct i2c_msg msg = {
- .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 3
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
+ sizeof(buf));

buf[0] = (val & 0xff0000) >> 16;
buf[1] = (val & 0xff00) >> 8;
@@ -59,8 +58,7 @@ static int mc44s803_readreg(struct mc44s803_priv *priv, u8 reg, u32 *val)
u8 buf[3];
int ret;
struct i2c_msg msg[] = {
- { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
- .buf = buf, .len = 3 },
+ I2C_MSG_READ(priv->cfg->i2c_address, buf, sizeof(buf)),
};

wval = MC44S803_REG_SM(MC44S803_REG_DATAREG, MC44S803_ADDR) |
diff --git a/drivers/media/tuners/mt2060.c b/drivers/media/tuners/mt2060.c
index 13381de..5fb2e77 100644
--- a/drivers/media/tuners/mt2060.c
+++ b/drivers/media/tuners/mt2060.c
@@ -42,8 +42,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {
@@ -57,9 +57,8 @@ static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = { reg, val };
- struct i2c_msg msg = {
- .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
+ sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "mt2060 I2C write failed\n");
@@ -71,9 +70,7 @@ static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
// Writes a set of consecutive registers
static int mt2060_writeregs(struct mt2060_priv *priv,u8 *buf, u8 len)
{
- struct i2c_msg msg = {
- .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "mt2060 I2C write failed (len=%i)\n",(int)len);
return -EREMOTEIO;
diff --git a/drivers/media/tuners/mt2063.c b/drivers/media/tuners/mt2063.c
index 0ed9091..c29f6f6 100644
--- a/drivers/media/tuners/mt2063.c
+++ b/drivers/media/tuners/mt2063.c
@@ -250,12 +250,8 @@ static u32 mt2063_write(struct mt2063_state *state, u8 reg, u8 *data, u32 len)
struct dvb_frontend *fe = state->frontend;
int ret;
u8 buf[60];
- struct i2c_msg msg = {
- .addr = state->config->tuner_address,
- .flags = 0,
- .buf = buf,
- .len = len + 1
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(state->config->tuner_address, buf,
+ len + 1);

dprintk(2, "\n");

@@ -313,17 +309,10 @@ static u32 mt2063_read(struct mt2063_state *state,
for (i = 0; i < cnt; i++) {
u8 b0[] = { subAddress + i };
struct i2c_msg msg[] = {
- {
- .addr = state->config->tuner_address,
- .flags = 0,
- .buf = b0,
- .len = 1
- }, {
- .addr = state->config->tuner_address,
- .flags = I2C_M_RD,
- .buf = pData + i,
- .len = 1
- }
+ I2C_MSG_WRITE(state->config->tuner_address, b0,
+ sizeof(b0)),
+ I2C_MSG_READ(state->config->tuner_address,
+ pData + i, 1)
};

status = i2c_transfer(state->i2c, msg, 2);
diff --git a/drivers/media/tuners/mt2131.c b/drivers/media/tuners/mt2131.c
index f83b0c1..a154901 100644
--- a/drivers/media/tuners/mt2131.c
+++ b/drivers/media/tuners/mt2131.c
@@ -53,10 +53,8 @@ static u8 mt2131_config2[] = {
static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->cfg->i2c_address, .flags = 0,
- .buf = &reg, .len = 1 },
- { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
- .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
};

if (i2c_transfer(priv->i2c, msg, 2) != 2) {
@@ -69,8 +67,8 @@ static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = { reg, val };
- struct i2c_msg msg = { .addr = priv->cfg->i2c_address, .flags = 0,
- .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
+ sizeof(buf));

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "mt2131 I2C write failed\n");
@@ -81,8 +79,7 @@ static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)

static int mt2131_writeregs(struct mt2131_priv *priv,u8 *buf, u8 len)
{
- struct i2c_msg msg = { .addr = priv->cfg->i2c_address,
- .flags = 0, .buf = buf, .len = len };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);

if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "mt2131 I2C write failed (len=%i)\n",
diff --git a/drivers/media/tuners/mt2266.c b/drivers/media/tuners/mt2266.c
index bca4d75..a714728 100644
--- a/drivers/media/tuners/mt2266.c
+++ b/drivers/media/tuners/mt2266.c
@@ -57,8 +57,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
{
struct i2c_msg msg[2] = {
- { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
- { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
+ I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
};
if (i2c_transfer(priv->i2c, msg, 2) != 2) {
printk(KERN_WARNING "MT2266 I2C read failed\n");
@@ -71,9 +71,8 @@ static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
{
u8 buf[2] = { reg, val };
- struct i2c_msg msg = {
- .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
+ sizeof(buf));
if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "MT2266 I2C write failed\n");
return -EREMOTEIO;
@@ -84,9 +83,7 @@ static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
// Writes a set of consecutive registers
static int mt2266_writeregs(struct mt2266_priv *priv,u8 *buf, u8 len)
{
- struct i2c_msg msg = {
- .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
printk(KERN_WARNING "MT2266 I2C write failed (len=%i)\n",(int)len);
return -EREMOTEIO;
diff --git a/drivers/media/tuners/mxl5005s.c b/drivers/media/tuners/mxl5005s.c
index b473b76..c913322 100644
--- a/drivers/media/tuners/mxl5005s.c
+++ b/drivers/media/tuners/mxl5005s.c
@@ -3848,8 +3848,8 @@ static int mxl5005s_reset(struct dvb_frontend *fe)
int ret = 0;

u8 buf[2] = { 0xff, 0x00 };
- struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
- .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
+ sizeof(buf));

dprintk(2, "%s()\n", __func__);

@@ -3874,8 +3874,8 @@ static int mxl5005s_writereg(struct dvb_frontend *fe, u8 reg, u8 val, int latch)
{
struct mxl5005s_state *state = fe->tuner_priv;
u8 buf[3] = { reg, val, MXL5005S_LATCH_BYTE };
- struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
- .buf = buf, .len = 3 };
+ struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
+ sizeof(buf));

if (latch == 0)
msg.len = 2;
diff --git a/drivers/media/tuners/tda827x.c b/drivers/media/tuners/tda827x.c
index a0d1762..15a4802 100644
--- a/drivers/media/tuners/tda827x.c
+++ b/drivers/media/tuners/tda827x.c
@@ -159,8 +159,7 @@ static int tda827xo_set_params(struct dvb_frontend *fe)
u8 buf[14];
int rc;

- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = buf, .len = sizeof(buf) };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
int i, tuner_freq, if_freq;
u32 N;

@@ -233,8 +232,7 @@ static int tda827xo_sleep(struct dvb_frontend *fe)
{
struct tda827x_priv *priv = fe->tuner_priv;
static u8 buf[] = { 0x30, 0xd0 };
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = buf, .len = sizeof(buf) };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));

dprintk("%s:\n", __func__);
tuner_transfer(fe, &msg, 1);
@@ -255,7 +253,7 @@ static int tda827xo_set_analog_params(struct dvb_frontend *fe,
u32 N;
int i;
struct tda827x_priv *priv = fe->tuner_priv;
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0 };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, NULL, 0);
unsigned int freq = params->frequency;

tda827x_set_std(fe, params);
@@ -335,8 +333,7 @@ static void tda827xo_agcf(struct dvb_frontend *fe)
{
struct tda827x_priv *priv = fe->tuner_priv;
unsigned char data[] = { 0x80, 0x0c };
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = data, .len = 2};
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));

tuner_transfer(fe, &msg, 1);
}
@@ -445,8 +442,7 @@ static int tda827xa_sleep(struct dvb_frontend *fe)
{
struct tda827x_priv *priv = fe->tuner_priv;
static u8 buf[] = { 0x30, 0x90 };
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = buf, .len = sizeof(buf) };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));

dprintk("%s:\n", __func__);

@@ -465,7 +461,7 @@ static void tda827xa_lna_gain(struct dvb_frontend *fe, int high,
unsigned char buf[] = {0x22, 0x01};
int arg;
int gp_func;
- struct i2c_msg msg = { .flags = 0, .buf = buf, .len = sizeof(buf) };
+ struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));

if (NULL == priv->cfg) {
dprintk("tda827x_config not defined, cannot set LNA gain!\n");
@@ -518,8 +514,7 @@ static int tda827xa_set_params(struct dvb_frontend *fe)
struct tda827xa_data *frequency_map = tda827xa_dvbt;
u8 buf[11];

- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = buf, .len = sizeof(buf) };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));

int i, tuner_freq, if_freq, rc;
u32 N;
@@ -665,8 +660,8 @@ static int tda827xa_set_analog_params(struct dvb_frontend *fe,
u32 N;
int i;
struct tda827x_priv *priv = fe->tuner_priv;
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
- .buf = tuner_reg, .len = sizeof(tuner_reg) };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, tuner_reg,
+ sizeof(tuner_reg));
unsigned int freq = params->frequency;

tda827x_set_std(fe, params);
@@ -760,8 +755,7 @@ static void tda827xa_agcf(struct dvb_frontend *fe)
{
struct tda827x_priv *priv = fe->tuner_priv;
unsigned char data[] = {0x80, 0x2c};
- struct i2c_msg msg = {.addr = priv->i2c_addr, .flags = 0,
- .buf = data, .len = 2};
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));
tuner_transfer(fe, &msg, 1);
}

@@ -855,8 +849,7 @@ static int tda827x_probe_version(struct dvb_frontend *fe)
u8 data;
int rc;
struct tda827x_priv *priv = fe->tuner_priv;
- struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = I2C_M_RD,
- .buf = &data, .len = 1 };
+ struct i2c_msg msg = I2C_MSG_READ(priv->i2c_addr, &data, sizeof(data));

rc = tuner_transfer(fe, &msg, 1);

diff --git a/drivers/media/tuners/tuner-i2c.h b/drivers/media/tuners/tuner-i2c.h
index 18f0056..a8a6da6 100644
--- a/drivers/media/tuners/tuner-i2c.h
+++ b/drivers/media/tuners/tuner-i2c.h
@@ -35,8 +35,7 @@ struct tuner_i2c_props {

static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf, int len)
{
- struct i2c_msg msg = { .addr = props->addr, .flags = 0,
- .buf = buf, .len = len };
+ struct i2c_msg msg = I2C_MSG_WRITE(props->addr, buf, len);
int ret = i2c_transfer(props->adap, &msg, 1);

return (ret == 1) ? len : ret;
@@ -44,8 +43,7 @@ static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf,

static inline int tuner_i2c_xfer_recv(struct tuner_i2c_props *props, char *buf, int len)
{
- struct i2c_msg msg = { .addr = props->addr, .flags = I2C_M_RD,
- .buf = buf, .len = len };
+ struct i2c_msg msg = I2C_MSG_READ(props->addr, buf, len);
int ret = i2c_transfer(props->adap, &msg, 1);

return (ret == 1) ? len : ret;
@@ -55,10 +53,8 @@ static inline int tuner_i2c_xfer_send_recv(struct tuner_i2c_props *props,
char *obuf, int olen,
char *ibuf, int ilen)
{
- struct i2c_msg msg[2] = { { .addr = props->addr, .flags = 0,
- .buf = obuf, .len = olen },
- { .addr = props->addr, .flags = I2C_M_RD,
- .buf = ibuf, .len = ilen } };
+ struct i2c_msg msg[2] = { I2C_MSG_WRITE(props->addr, obuf, olen),
+ I2C_MSG_READ(props->addr, ibuf, ilen) };
int ret = i2c_transfer(props->adap, msg, 2);

return (ret == 2) ? ilen : ret;
diff --git a/drivers/media/tuners/tuner-simple.c b/drivers/media/tuners/tuner-simple.c
index 39e7e58..df5ba78 100644
--- a/drivers/media/tuners/tuner-simple.c
+++ b/drivers/media/tuners/tuner-simple.c
@@ -1065,10 +1065,7 @@ struct dvb_frontend *simple_tuner_attach(struct dvb_frontend *fe,
*/
if (i2c_adap != NULL) {
u8 b[1];
- struct i2c_msg msg = {
- .addr = i2c_addr, .flags = I2C_M_RD,
- .buf = b, .len = 1,
- };
+ struct i2c_msg msg = I2C_MSG_READ(i2c_addr, b, sizeof(b));

if (fe->ops.i2c_gate_ctrl)
fe->ops.i2c_gate_ctrl(fe, 1);
diff --git a/drivers/media/tuners/xc4000.c b/drivers/media/tuners/xc4000.c
index 4937712..7b65b6b 100644
--- a/drivers/media/tuners/xc4000.c
+++ b/drivers/media/tuners/xc4000.c
@@ -256,8 +256,7 @@ static void xc_debug_dump(struct xc4000_priv *priv);

static int xc_send_i2c_data(struct xc4000_priv *priv, u8 *buf, int len)
{
- struct i2c_msg msg = { .addr = priv->i2c_props.addr,
- .flags = 0, .buf = buf, .len = len };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);
if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
if (priv->ignore_i2c_write_errors == 0) {
printk(KERN_ERR "xc4000: I2C write failed (len=%i)\n",
@@ -550,10 +549,8 @@ static int xc4000_readreg(struct xc4000_priv *priv, u16 reg, u16 *val)
u8 buf[2] = { reg >> 8, reg & 0xff };
u8 bval[2] = { 0, 0 };
struct i2c_msg msg[2] = {
- { .addr = priv->i2c_props.addr,
- .flags = 0, .buf = &buf[0], .len = 2 },
- { .addr = priv->i2c_props.addr,
- .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
+ I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
+ I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),
};

if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {
diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
index dc93cf3..33b6b1e 100644
--- a/drivers/media/tuners/xc5000.c
+++ b/drivers/media/tuners/xc5000.c
@@ -253,8 +253,7 @@ static int xc5000_TunerReset(struct dvb_frontend *fe);

static int xc_send_i2c_data(struct xc5000_priv *priv, u8 *buf, int len)
{
- struct i2c_msg msg = { .addr = priv->i2c_props.addr,
- .flags = 0, .buf = buf, .len = len };
+ struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);

if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
printk(KERN_ERR "xc5000: I2C write failed (len=%i)\n", len);
@@ -285,10 +284,8 @@ static int xc5000_readreg(struct xc5000_priv *priv, u16 reg, u16 *val)
u8 buf[2] = { reg >> 8, reg & 0xff };
u8 bval[2] = { 0, 0 };
struct i2c_msg msg[2] = {
- { .addr = priv->i2c_props.addr,
- .flags = 0, .buf = &buf[0], .len = 2 },
- { .addr = priv->i2c_props.addr,
- .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
+ I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
+ I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),
};

if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {

2012-10-07 15:42:50

by Julia Lawall

Subject: [PATCH 2/13] drivers/media/tuners/mxl5007t.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

In each case, a length expressed as an explicit constant is also
re-expressed as the size of the buffer, when this is possible.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/mxl5007t.c | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/media/tuners/mxl5007t.c b/drivers/media/tuners/mxl5007t.c
index 69e453e..c0c28be 100644
--- a/drivers/media/tuners/mxl5007t.c
+++ b/drivers/media/tuners/mxl5007t.c
@@ -464,8 +464,8 @@ reg_pair_t *mxl5007t_calc_rf_tune_regs(struct mxl5007t_state *state,
static int mxl5007t_write_reg(struct mxl5007t_state *state, u8 reg, u8 val)
{
u8 buf[] = { reg, val };
- struct i2c_msg msg = { .addr = state->i2c_props.addr, .flags = 0,
- .buf = buf, .len = 2 };
+ struct i2c_msg msg = I2C_MSG_WRITE(state->i2c_props.addr, buf,
+ sizeof(buf));
int ret;

ret = i2c_transfer(state->i2c_props.adap, &msg, 1);
@@ -494,10 +494,8 @@ static int mxl5007t_read_reg(struct mxl5007t_state *state, u8 reg, u8 *val)
{
u8 buf[2] = { 0xfb, reg };
struct i2c_msg msg[] = {
- { .addr = state->i2c_props.addr, .flags = 0,
- .buf = buf, .len = 2 },
- { .addr = state->i2c_props.addr, .flags = I2C_M_RD,
- .buf = val, .len = 1 },
+ I2C_MSG_WRITE(state->i2c_props.addr, buf, sizeof(buf)),
+ I2C_MSG_READ(state->i2c_props.addr, val, 1),
};
int ret;

@@ -512,10 +510,8 @@ static int mxl5007t_read_reg(struct mxl5007t_state *state, u8 reg, u8 *val)
static int mxl5007t_soft_reset(struct mxl5007t_state *state)
{
u8 d = 0xff;
- struct i2c_msg msg = {
- .addr = state->i2c_props.addr, .flags = 0,
- .buf = &d, .len = 1
- };
+ struct i2c_msg msg = I2C_MSG_WRITE(state->i2c_props.addr, &d,
+ sizeof(d));
int ret = i2c_transfer(state->i2c_props.adap, &msg, 1);

if (ret != 1) {

2012-10-07 15:43:00

by Julia Lawall

Subject: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

From: Julia Lawall <[email protected]>

Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

In the second i2c_msg structure, a length expressed as an explicit constant
is also re-expressed as the size of the buffer, reg.

A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
+ I2C_MSG_READ(a,b,c)
;

@@
expression a,b,c;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = 0}
+ I2C_MSG_WRITE(a,b,c)
;

@@
expression a,b,c,d;
identifier x;
@@

struct i2c_msg x =
- {.addr = a, .buf = b, .len = c, .flags = d}
+ I2C_MSG_OP(a,b,c,d)
;
// </smpl>

Signed-off-by: Julia Lawall <[email protected]>

---
drivers/media/tuners/e4000.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 1b33ed3..8f182fc 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
int ret;
u8 buf[1 + len];
struct i2c_msg msg[1] = {
- {
- .addr = priv->cfg->i2c_addr,
- .flags = 0,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
};

buf[0] = reg;
@@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
int ret;
u8 buf[len];
struct i2c_msg msg[2] = {
- {
- .addr = priv->cfg->i2c_addr,
- .flags = 0,
- .len = 1,
- .buf = &reg,
- }, {
- .addr = priv->cfg->i2c_addr,
- .flags = I2C_M_RD,
- .len = sizeof(buf),
- .buf = buf,
- }
+ I2C_MSG_WRITE(priv->cfg->i2c_addr, &reg, sizeof(reg)),
+ I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf))
};

ret = i2c_transfer(priv->i2c, msg, 2);

2012-10-07 16:34:07

by walter harms

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

Am 07.10.2012 17:38, schrieb Julia Lawall:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> In the second i2c_msg structure, a length expressed as an explicit constant
> is also re-expressed as the size of the buffer, reg.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> + I2C_MSG_READ(a,b,c)
> ;
>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = 0}
> + I2C_MSG_WRITE(a,b,c)
> ;
>
> @@
> expression a,b,c,d;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = d}
> + I2C_MSG_OP(a,b,c,d)
> ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/media/tuners/e4000.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
> index 1b33ed3..8f182fc 100644
> --- a/drivers/media/tuners/e4000.c
> +++ b/drivers/media/tuners/e4000.c
> @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
> int ret;
> u8 buf[1 + len];
> struct i2c_msg msg[1] = {
> - {
> - .addr = priv->cfg->i2c_addr,
> - .flags = 0,
> - .len = sizeof(buf),
> - .buf = buf,
> - }
> + I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
> };
>

Any reason why struct i2c_msg is an array ?

re,
wh

> buf[0] = reg;
> @@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
> int ret;
> u8 buf[len];
> struct i2c_msg msg[2] = {
> - {
> - .addr = priv->cfg->i2c_addr,
> - .flags = 0,
> - .len = 1,
> - .buf = &reg,
> - }, {
> - .addr = priv->cfg->i2c_addr,
> - .flags = I2C_M_RD,
> - .len = sizeof(buf),
> - .buf = buf,
> - }
> + I2C_MSG_WRITE(priv->cfg->i2c_addr, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf))
> };
>
> ret = i2c_transfer(priv->i2c, msg, 2);
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

2012-10-07 16:44:03

by walter harms

Subject: Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

Am 07.10.2012 17:38, schrieb Julia Lawall:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> A length expressed as an explicit constant is also re-expressed as the size
> of the buffer in each case.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> + I2C_MSG_READ(a,b,c)
> ;
>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = 0}
> + I2C_MSG_WRITE(a,b,c)
> ;
>
> @@
> expression a,b,c,d;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = d}
> + I2C_MSG_OP(a,b,c,d)
> ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/media/tuners/fc0011.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
> index e488254..5dbba98 100644
> --- a/drivers/media/tuners/fc0011.c
> +++ b/drivers/media/tuners/fc0011.c
> @@ -80,8 +80,7 @@ struct fc0011_priv {
> static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = { reg, val };
> - struct i2c_msg msg = { .addr = priv->addr,
> - .flags = 0, .buf = buf, .len = 2 };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> dev_err(&priv->i2c->dev,
> @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 reg, u8 *val)
> {
> u8 dummy;
> struct i2c_msg msg[2] = {
> - { .addr = priv->addr,
> - .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->addr,
> - .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
> };
>

This dummy looks strange, can it be that this is used uninitialised ?

re,
wh


> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>

2012-10-07 16:44:26

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012, walter harms wrote:

>
>
> Am 07.10.2012 17:38, schrieb Julia Lawall:
>> From: Julia Lawall <[email protected]>
>>
>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>
>> In the second i2c_msg structure, a length expressed as an explicit constant
>> is also re-expressed as the size of the buffer, reg.
>>
>> A simplified version of the semantic patch that makes this change is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>> + I2C_MSG_READ(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>> + I2C_MSG_WRITE(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c,d;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = d}
>> + I2C_MSG_OP(a,b,c,d)
>> ;
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/media/tuners/e4000.c | 20 +++-----------------
>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>> index 1b33ed3..8f182fc 100644
>> --- a/drivers/media/tuners/e4000.c
>> +++ b/drivers/media/tuners/e4000.c
>> @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>> int ret;
>> u8 buf[1 + len];
>> struct i2c_msg msg[1] = {
>> - {
>> - .addr = priv->cfg->i2c_addr,
>> - .flags = 0,
>> - .len = sizeof(buf),
>> - .buf = buf,
>> - }
>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
>> };
>>
>
> Any reason why struct i2c_msg is an array ?

I assumed that it looked more harmonious with the other uses of
i2c_transfer, which takes as arguments an array and the number of
elements.

But there are some files that instead use i2c_transfer(priv->i2c, &msg, 1).
I can change them all to do that if that is preferred. But maybe I
will wait a little bit to see if there are other issues to address at
the same time.

thanks,
julia

>
> re,
> wh
>
>> buf[0] = reg;
>> @@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv, u8 reg, u8 *val, int len)
>> int ret;
>> u8 buf[len];
>> struct i2c_msg msg[2] = {
>> - {
>> - .addr = priv->cfg->i2c_addr,
>> - .flags = 0,
>> - .len = 1,
>> - .buf = &reg,
>> - }, {
>> - .addr = priv->cfg->i2c_addr,
>> - .flags = I2C_M_RD,
>> - .len = sizeof(buf),
>> - .buf = buf,
>> - }
>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf))
>> };
>>
>> ret = i2c_transfer(priv->i2c, msg, 2);
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

2012-10-07 16:51:05

by Julia Lawall

Subject: Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012, walter harms wrote:

>
>
> Am 07.10.2012 17:38, schrieb Julia Lawall:
>> From: Julia Lawall <[email protected]>
>>
>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>
>> A length expressed as an explicit constant is also re-expressed as the size
>> of the buffer in each case.
>>
>> A simplified version of the semantic patch that makes this change is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>> + I2C_MSG_READ(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>> + I2C_MSG_WRITE(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c,d;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = d}
>> + I2C_MSG_OP(a,b,c,d)
>> ;
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/media/tuners/fc0011.c | 9 +++------
>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/tuners/fc0011.c b/drivers/media/tuners/fc0011.c
>> index e488254..5dbba98 100644
>> --- a/drivers/media/tuners/fc0011.c
>> +++ b/drivers/media/tuners/fc0011.c
>> @@ -80,8 +80,7 @@ struct fc0011_priv {
>> static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = { reg, val };
>> - struct i2c_msg msg = { .addr = priv->addr,
>> - .flags = 0, .buf = buf, .len = 2 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> dev_err(&priv->i2c->dev,
>> @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 reg, u8 *val)
>> {
>> u8 dummy;
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->addr,
>> - .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->addr,
>> - .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
>> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
>> };
>>
>
> This dummy looks strange, can it be that this is used uninitialised ?

I'm not sure to understand the question. The read, when it happens in
i2c_transfer will initialize dummy. On the other hand, I don't know what
i2c_transfer does when the buffer is NULL and the size is 1. It does not
look very elegant at least.

julia

2012-10-07 16:54:53

by walter harms

Subject: Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

Am 07.10.2012 18:50, schrieb Julia Lawall:
> On Sun, 7 Oct 2012, walter harms wrote:
>
>>
>>
>> Am 07.10.2012 17:38, schrieb Julia Lawall:
>>> From: Julia Lawall <[email protected]>
>>>
>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>>
>>> A length expressed as an explicit constant is also re-expressed as
>>> the size
>>> of the buffer in each case.
>>>
>>> A simplified version of the semantic patch that makes this change is as
>>> follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>>> + I2C_MSG_READ(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>>> + I2C_MSG_WRITE(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c,d;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = d}
>>> + I2C_MSG_OP(a,b,c,d)
>>> ;
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <[email protected]>
>>>
>>> ---
>>> drivers/media/tuners/fc0011.c | 9 +++------
>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/fc0011.c
>>> b/drivers/media/tuners/fc0011.c
>>> index e488254..5dbba98 100644
>>> --- a/drivers/media/tuners/fc0011.c
>>> +++ b/drivers/media/tuners/fc0011.c
>>> @@ -80,8 +80,7 @@ struct fc0011_priv {
>>> static int fc0011_writereg(struct fc0011_priv *priv, u8 reg, u8 val)
>>> {
>>> u8 buf[2] = { reg, val };
>>> - struct i2c_msg msg = { .addr = priv->addr,
>>> - .flags = 0, .buf = buf, .len = 2 };
>>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>>>
>>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>>> dev_err(&priv->i2c->dev,
>>> @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv
>>> *priv, u8 reg, u8 *val)
>>> {
>>> u8 dummy;
>>> struct i2c_msg msg[2] = {
>>> - { .addr = priv->addr,
>>> - .flags = 0, .buf = &reg, .len = 1 },
>>> - { .addr = priv->addr,
>>> - .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
>>> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
>>> + I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
>>> };
>>>
>>
>> This dummy looks strange, can it be that this is used uninitialised ?
>
> I'm not sure to understand the question. The read, when it happens in
> i2c_transfer will initialize dummy. On the other hand, I don't know
> what i2c_transfer does when the buffer is NULL and the size is 1. It
> does not look very elegant at least.
>

mea culpa, i mixed read and write

re,
wh

> julia
>
>

2012-10-07 17:13:35

by walter harms

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

Am 07.10.2012 18:44, schrieb Julia Lawall:
> On Sun, 7 Oct 2012, walter harms wrote:
>
>>
>>
>> Am 07.10.2012 17:38, schrieb Julia Lawall:
>>> From: Julia Lawall <[email protected]>
>>>
>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>>
>>> In the second i2c_msg structure, a length expressed as an explicit
>>> constant
>>> is also re-expressed as the size of the buffer, reg.
>>>
>>> A simplified version of the semantic patch that makes this change is as
>>> follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>>> + I2C_MSG_READ(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>>> + I2C_MSG_WRITE(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c,d;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = d}
>>> + I2C_MSG_OP(a,b,c,d)
>>> ;
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <[email protected]>
>>>
>>> ---
>>> drivers/media/tuners/e4000.c | 20 +++-----------------
>>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>> index 1b33ed3..8f182fc 100644
>>> --- a/drivers/media/tuners/e4000.c
>>> +++ b/drivers/media/tuners/e4000.c
>>> @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv,
>>> u8 reg, u8 *val, int len)
>>> int ret;
>>> u8 buf[1 + len];
>>> struct i2c_msg msg[1] = {
>>> - {
>>> - .addr = priv->cfg->i2c_addr,
>>> - .flags = 0,
>>> - .len = sizeof(buf),
>>> - .buf = buf,
>>> - }
>>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
>>> };
>>>
>>
>> Any reason why struct i2c_msg is an array ?
>
> I assumed that it looked more harmonious with the other uses of
> i2c_transfer, which takes as arguments an array and the number of elements.
>
> But there are some files that instead use i2c_transfer(priv->i2c, &msg, 1).
> I can change them all to do that if that is preferred. But maybe I will
> wait a little bit to see if there are other issues to address at the
> same time.
>
> thanks,
> julia
>

Hi Julia,
please be aware i am not the maintainer only a distant watcher :)

do you really thing that a macro is appropriate here ? I feel uneasy about it
but i can not offer an other solution.

nothing to worry about,
just my 2 cents.

re,
wh


>>
>> re,
>> wh
>>
>>> buf[0] = reg;
>>> @@ -54,17 +49,8 @@ static int e4000_rd_regs(struct e4000_priv *priv,
>>> u8 reg, u8 *val, int len)
>>> int ret;
>>> u8 buf[len];
>>> struct i2c_msg msg[2] = {
>>> - {
>>> - .addr = priv->cfg->i2c_addr,
>>> - .flags = 0,
>>> - .len = 1,
>>> - .buf = &reg,
>>> - }, {
>>> - .addr = priv->cfg->i2c_addr,
>>> - .flags = I2C_M_RD,
>>> - .len = sizeof(buf),
>>> - .buf = buf,
>>> - }
>>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, &reg, sizeof(reg)),
>>> + I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf))
>>> };
>>>
>>> ret = i2c_transfer(priv->i2c, msg, 2);
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe
>>> kernel-janitors" in
>>> the body of a message to [email protected]
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe
>> kernel-janitors" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>
>

2012-10-07 17:19:10

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012, walter harms wrote:

>
>
> Am 07.10.2012 18:44, schrieb Julia Lawall:
>> On Sun, 7 Oct 2012, walter harms wrote:
>>
>>>
>>>
>>> Am 07.10.2012 17:38, schrieb Julia Lawall:
>>>> From: Julia Lawall <[email protected]>
>>>>
>>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>>>
>>>> In the second i2c_msg structure, a length expressed as an explicit
>>>> constant
>>>> is also re-expressed as the size of the buffer, reg.
>>>>
>>>> A simplified version of the semantic patch that makes this change is as
>>>> follows: (http://coccinelle.lip6.fr/)
>>>>
>>>> // <smpl>
>>>> @@
>>>> expression a,b,c;
>>>> identifier x;
>>>> @@
>>>>
>>>> struct i2c_msg x =
>>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>>>> + I2C_MSG_READ(a,b,c)
>>>> ;
>>>>
>>>> @@
>>>> expression a,b,c;
>>>> identifier x;
>>>> @@
>>>>
>>>> struct i2c_msg x =
>>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>>>> + I2C_MSG_WRITE(a,b,c)
>>>> ;
>>>>
>>>> @@
>>>> expression a,b,c,d;
>>>> identifier x;
>>>> @@
>>>>
>>>> struct i2c_msg x =
>>>> - {.addr = a, .buf = b, .len = c, .flags = d}
>>>> + I2C_MSG_OP(a,b,c,d)
>>>> ;
>>>> // </smpl>
>>>>
>>>> Signed-off-by: Julia Lawall <[email protected]>
>>>>
>>>> ---
>>>> drivers/media/tuners/e4000.c | 20 +++-----------------
>>>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>>> index 1b33ed3..8f182fc 100644
>>>> --- a/drivers/media/tuners/e4000.c
>>>> +++ b/drivers/media/tuners/e4000.c
>>>> @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv,
>>>> u8 reg, u8 *val, int len)
>>>> int ret;
>>>> u8 buf[1 + len];
>>>> struct i2c_msg msg[1] = {
>>>> - {
>>>> - .addr = priv->cfg->i2c_addr,
>>>> - .flags = 0,
>>>> - .len = sizeof(buf),
>>>> - .buf = buf,
>>>> - }
>>>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
>>>> };
>>>>
>>>
>>> Any reason why struct i2c_msg is an array ?
>>
>> I assumed that it looked more harmonious with the other uses of
>> i2c_transfer, which takes as arguments an array and the number of elements.
>>
>> But there are some files that instead use i2c_transfer(priv->i2c, &msg, 1).
>> I can change them all to do that if that is preferred. But maybe I will
>> wait a little bit to see if there are other issues to address at the
>> same time.
>>
>> thanks,
>> julia
>>
>
> Hi Julia,
> please be aware i am not the maintainer only a distant watcher :)
>
> do you really thing that a macro is appropriate here ? I feel uneasy about it
> but i can not offer an other solution.

Some people thought that it would be nice to have the macros rather than
the inlined field initializations, especially since there is no flag for
write. A separate question is whether an array of one element is useful,
or whether one should systematically use & on a simple variable of the
structure type. I'm open to suggestions about either point.

julia

2012-10-07 18:16:30

by Joe Perches

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 2012-10-07 at 19:18 +0200, Julia Lawall wrote:
> On Sun, 7 Oct 2012, walter harms wrote:
> > Am 07.10.2012 18:44, schrieb Julia Lawall:
> >> On Sun, 7 Oct 2012, walter harms wrote:
> >>> Am 07.10.2012 17:38, schrieb Julia Lawall:
> >>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
> >>>> struct i2c_msg x =
> >>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> >>>> + I2C_MSG_READ(a,b,c)
[]
> >>>> struct i2c_msg x =
> >>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
> >>>> + I2C_MSG_WRITE(a,b,c)
[]
> > do you really thing that a macro is appropriate here ? I feel uneasy about it
> > but i can not offer an other solution.

I think the macros are fine.

> Some people thought that it would be nice to have the macros rather than
> the inlined field initializations, especially since there is no flag for
> write. A separate question is whether an array of one element is useful,
> or whether one should systematically use & on a simple variable of the
> structure type. I'm open to suggestions about either point.

I think the macro naming is not great.

Maybe add DEFINE_/DECLARE_/_INIT or something other than an action
name type to the macro names.

I think the consistency is better if all the references are done
as arrays, even for single entry arrays.

It's all quibbling in any case.

2012-10-07 18:27:18

by Michael Büsch

Subject: Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012 18:50:31 +0200 (CEST)
Julia Lawall <[email protected]> wrote:

> >> @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 reg, u8 *val)
> >> {
> >> u8 dummy;
> >> struct i2c_msg msg[2] = {
> >> - { .addr = priv->addr,
> >> - .flags = 0, .buf = &reg, .len = 1 },
> >> - { .addr = priv->addr,
> >> - .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
> >> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
> >> + I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
> >> };
> >>
> >
> > This dummy looks strange, can it be that this is used uninitialised ?
>
> I'm not sure to understand the question. The read, when it happens in
> i2c_transfer will initialize dummy. On the other hand, I don't know what
> i2c_transfer does when the buffer is NULL and the size is 1. It does not
> look very elegant at least.

Well its use case is if you only care about the side effects and not the actual data
returned by the read operation.

--
Greetings, Michael.

PGP encryption is encouraged / 908D8B0E

Attachments:

2012-10-07 18:56:31

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

>> Some people thought that it would be nice to have the macros rather than
>> the inlined field initializations, especially since there is no flag for
>> write. A separate question is whether an array of one element is useful,
>> or whether one should systematically use & on a simple variable of the
>> structure type. I'm open to suggestions about either point.
>
> I think the macro naming is not great.
>
> Maybe add DEFINE_/DECLARE_/_INIT or something other than an action
> name type to the macro names.

DEFINE and DECLARE usually have a declared variable as an argument, which
is not the case here.

These macros are like the macros PCI_DEVICE and PCI_DEVICE_CLASS.

Are READ and WRITE the action names? They are really the important
information in this case.

> I think the consistency is better if all the references are done
> as arrays, even for single entry arrays.

Is it worth creating arrays where &msg is used? Or would it be better to
leave that aspect as it is?

julia

2012-10-07 21:39:35

by Joe Perches

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 2012-10-07 at 20:56 +0200, Julia Lawall wrote:
> >> Some people thought that it would be nice to have the macros rather than
> >> the inlined field initializations, especially since there is no flag for
> >> write. A separate question is whether an array of one element is useful,
> >> or whether one should systematically use & on a simple variable of the
> >> structure type. I'm open to suggestions about either point.
> >
> > I think the macro naming is not great.
> >
> > Maybe add DEFINE_/DECLARE_/_INIT or something other than an action
> > name type to the macro names.
>
> DEFINE and DECLARE usually have a declared variable as an argument, which
> is not the case here.
>
> These macros are like the macros PCI_DEVICE and PCI_DEVICE_CLASS.

I understand that.

> Are READ and WRITE the action names? They are really the important
> information in this case.

Yes, most (all?) uses of _READ and _WRITE macros actually
perform some I/O.

> > I think the consistency is better if all the references are done
> > as arrays, even for single entry arrays.
>
> Is it worth creating arrays where &msg is used? Or would it be better to
> leave that aspect as it is?

Reasonable arguments can be made either way.

2012-10-07 21:44:10

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012, Joe Perches wrote:

> On Sun, 2012-10-07 at 20:56 +0200, Julia Lawall wrote:
>>>> Some people thought that it would be nice to have the macros rather than
>>>> the inlined field initializations, especially since there is no flag for
>>>> write. A separate question is whether an array of one element is useful,
>>>> or whether one should systematically use & on a simple variable of the
>>>> structure type. I'm open to suggestions about either point.
>>>
>>> I think the macro naming is not great.
>>>
>>> Maybe add DEFINE_/DECLARE_/_INIT or something other than an action
>>> name type to the macro names.
>>
>> DEFINE and DECLARE usually have a declared variable as an argument, which
>> is not the case here.
>>
>> These macros are like the macros PCI_DEVICE and PCI_DEVICE_CLASS.
>
> I understand that.
>
>> Are READ and WRITE the action names? They are really the important
>> information in this case.
>
> Yes, most (all?) uses of _READ and _WRITE macros actually
> perform some I/O.

I2C_MSG_READ_DATA?
I2C_MSG_READ_INFO?
I2C_MSG_READ_INIT?
I2C_MSG_PREPARE_READ?

julia

2012-10-07 21:50:07

by Ryan Mallon

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On 08/10/12 08:39, Joe Perches wrote:
> On Sun, 2012-10-07 at 20:56 +0200, Julia Lawall wrote:
>>>> Some people thought that it would be nice to have the macros rather than
>>>> the inlined field initializations, especially since there is no flag for
>>>> write. A separate question is whether an array of one element is useful,
>>>> or whether one should systematically use & on a simple variable of the
>>>> structure type. I'm open to suggestions about either point.
>>>
>>> I think the macro naming is not great.
>>>
>>> Maybe add DEFINE_/DECLARE_/_INIT or something other than an action
>>> name type to the macro names.
>>
>> DEFINE and DECLARE usually have a declared variable as an argument, which
>> is not the case here.
>>
>> These macros are like the macros PCI_DEVICE and PCI_DEVICE_CLASS.
>
> I understand that.
>
>> Are READ and WRITE the action names? They are really the important
>> information in this case.
>
> Yes, most (all?) uses of _READ and _WRITE macros actually
> perform some I/O.

Well, they are describing an IO operation even if they don't perform it
directly. What else would you call them? I think the macro names are
fine as is.

~Ryan

2012-10-07 21:52:04

by Joe Perches

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 2012-10-07 at 23:43 +0200, Julia Lawall wrote:
> On Sun, 7 Oct 2012, Joe Perches wrote:
> >> Are READ and WRITE the action names? They are really the important
> >> information in this case.
> >
> > Yes, most (all?) uses of _READ and _WRITE macros actually
> > perform some I/O.
>
> I2C_MSG_READ_DATA?
> I2C_MSG_READ_INFO?
> I2C_MSG_READ_INIT?
> I2C_MSG_PREPARE_READ?

Dunno, naming is hard. Maybe:

I2C_INPUT_MSG
I2C_OUTPUT_MSG
I2C_OP_MSG

cheers, Joe

2012-10-07 21:52:46

by Ryan Mallon

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On 08/10/12 03:44, Julia Lawall wrote:
> On Sun, 7 Oct 2012, walter harms wrote:
>
>>
>>
>> Am 07.10.2012 17:38, schrieb Julia Lawall:
>>> From: Julia Lawall <[email protected]>
>>>
>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>>
>>> In the second i2c_msg structure, a length expressed as an explicit
>>> constant
>>> is also re-expressed as the size of the buffer, reg.
>>>
>>> A simplified version of the semantic patch that makes this change is as
>>> follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>>> + I2C_MSG_READ(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>>> + I2C_MSG_WRITE(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c,d;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = d}
>>> + I2C_MSG_OP(a,b,c,d)
>>> ;
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <[email protected]>
>>>
>>> ---
>>> drivers/media/tuners/e4000.c | 20 +++-----------------
>>> 1 file changed, 3 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
>>> index 1b33ed3..8f182fc 100644
>>> --- a/drivers/media/tuners/e4000.c
>>> +++ b/drivers/media/tuners/e4000.c
>>> @@ -26,12 +26,7 @@ static int e4000_wr_regs(struct e4000_priv *priv,
>>> u8 reg, u8 *val, int len)
>>> int ret;
>>> u8 buf[1 + len];
>>> struct i2c_msg msg[1] = {
>>> - {
>>> - .addr = priv->cfg->i2c_addr,
>>> - .flags = 0,
>>> - .len = sizeof(buf),
>>> - .buf = buf,
>>> - }
>>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, buf, sizeof(buf))
>>> };
>>>
>>
>> Any reason why struct i2c_msg is an array ?
>
> I assumed that it looked more harmonious with the other uses of
> i2c_transfer, which takes as arguments an array and the number of elements.
>
> But there are some files that instead use i2c_transfer(priv->i2c, &msg, 1).
> I can change them all to do that if that is preferred. But maybe I will
> wait a little bit to see if there are other issues to address at the
> same time.

This is probably a good thing to do, but the initial patch series should
just do the conversion to the macros. Too many additional changes runs
the risk of introducing bugs and making bisection difficult.

~Ryan

2012-10-07 21:55:51

by Ryan Mallon

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

On 08/10/12 02:38, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> A length expressed as an explicit constant is also re-expressed as the size
> of the buffer, when this is possible.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> + I2C_MSG_READ(a,b,c)
> ;
>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = 0}
> + I2C_MSG_WRITE(a,b,c)
> ;
>
> @@
> expression a,b,c,d;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = d}
> + I2C_MSG_OP(a,b,c,d)
> ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/media/tuners/qt1010.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/tuners/qt1010.c b/drivers/media/tuners/qt1010.c
> index bc419f8..37ff254 100644
> --- a/drivers/media/tuners/qt1010.c
> +++ b/drivers/media/tuners/qt1010.c
> @@ -25,10 +25,8 @@
> static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->cfg->i2c_address,
> - .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->cfg->i2c_address,
> - .flags = I2C_M_RD, .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),

This is a bit inconsistent. For single length values we should either
consistently use sizeof(val) or 1. This has both.

~Ryan

2012-10-07 22:06:04

by Ryan Mallon

Subject: Re: [PATCH 5/13] drivers/media/tuners: use macros for i2c_msg initialization

On 08/10/12 02:38, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> A length expressed as an explicit constant is also re-expressed as the size
> of the buffer, when this is possible.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> + I2C_MSG_READ(a,b,c)
> ;
>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = 0}
> + I2C_MSG_WRITE(a,b,c)
> ;
>
> @@
> expression a,b,c,d;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = d}
> + I2C_MSG_OP(a,b,c,d)
> ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
> drivers/media/tuners/fc0012.c | 8 +++-----
> drivers/media/tuners/fc0013.c | 8 +++-----
> drivers/media/tuners/mc44s803.c | 8 +++-----
> drivers/media/tuners/mt2060.c | 13 +++++--------
> drivers/media/tuners/mt2063.c | 23 ++++++-----------------
> drivers/media/tuners/mt2131.c | 13 +++++--------
> drivers/media/tuners/mt2266.c | 13 +++++--------
> drivers/media/tuners/mxl5005s.c | 8 ++++----
> drivers/media/tuners/tda827x.c | 29 +++++++++++------------------
> drivers/media/tuners/tuner-i2c.h | 12 ++++--------
> drivers/media/tuners/tuner-simple.c | 5 +----
> drivers/media/tuners/xc4000.c | 9 +++------
> drivers/media/tuners/xc5000.c | 9 +++------
> 13 files changed, 56 insertions(+), 102 deletions(-)
>
> diff --git a/drivers/media/tuners/fc0012.c b/drivers/media/tuners/fc0012.c
> index 308135a..01dac7e 100644
> --- a/drivers/media/tuners/fc0012.c
> +++ b/drivers/media/tuners/fc0012.c
> @@ -24,9 +24,7 @@
> static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = {reg, val};
> - struct i2c_msg msg = {
> - .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
> @@ -38,8 +36,8 @@ static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
> static int fc0012_readreg(struct fc0012_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->addr, val, 1),
> };
>
> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
> diff --git a/drivers/media/tuners/fc0013.c b/drivers/media/tuners/fc0013.c
> index bd8f0f1..174f0b0 100644
> --- a/drivers/media/tuners/fc0013.c
> +++ b/drivers/media/tuners/fc0013.c
> @@ -27,9 +27,7 @@
> static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = {reg, val};
> - struct i2c_msg msg = {
> - .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
> @@ -41,8 +39,8 @@ static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
> static int fc0013_readreg(struct fc0013_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->addr, val, 1),
> };
>
> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
> diff --git a/drivers/media/tuners/mc44s803.c b/drivers/media/tuners/mc44s803.c
> index f1b7640..df47e03 100644
> --- a/drivers/media/tuners/mc44s803.c
> +++ b/drivers/media/tuners/mc44s803.c
> @@ -37,9 +37,8 @@
> static int mc44s803_writereg(struct mc44s803_priv *priv, u32 val)
> {
> u8 buf[3];
> - struct i2c_msg msg = {
> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 3
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
> + sizeof(buf));
>
> buf[0] = (val & 0xff0000) >> 16;
> buf[1] = (val & 0xff00) >> 8;
> @@ -59,8 +58,7 @@ static int mc44s803_readreg(struct mc44s803_priv *priv, u8 reg, u32 *val)
> u8 buf[3];
> int ret;
> struct i2c_msg msg[] = {
> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
> - .buf = buf, .len = 3 },
> + I2C_MSG_READ(priv->cfg->i2c_address, buf, sizeof(buf)),
> };
>
> wval = MC44S803_REG_SM(MC44S803_REG_DATAREG, MC44S803_ADDR) |
> diff --git a/drivers/media/tuners/mt2060.c b/drivers/media/tuners/mt2060.c
> index 13381de..5fb2e77 100644
> --- a/drivers/media/tuners/mt2060.c
> +++ b/drivers/media/tuners/mt2060.c
> @@ -42,8 +42,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
> static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),

Same here, should consistently use either sizeof(val) or 1 for single
length items.

> };
>
> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
> @@ -57,9 +57,8 @@ static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
> static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = { reg, val };
> - struct i2c_msg msg = {
> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
> + sizeof(buf));
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "mt2060 I2C write failed\n");
> @@ -71,9 +70,7 @@ static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
> // Writes a set of consecutive registers
> static int mt2060_writeregs(struct mt2060_priv *priv,u8 *buf, u8 len)
> {
> - struct i2c_msg msg = {
> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "mt2060 I2C write failed (len=%i)\n",(int)len);
> return -EREMOTEIO;
> diff --git a/drivers/media/tuners/mt2063.c b/drivers/media/tuners/mt2063.c
> index 0ed9091..c29f6f6 100644
> --- a/drivers/media/tuners/mt2063.c
> +++ b/drivers/media/tuners/mt2063.c
> @@ -250,12 +250,8 @@ static u32 mt2063_write(struct mt2063_state *state, u8 reg, u8 *data, u32 len)
> struct dvb_frontend *fe = state->frontend;
> int ret;
> u8 buf[60];
> - struct i2c_msg msg = {
> - .addr = state->config->tuner_address,
> - .flags = 0,
> - .buf = buf,
> - .len = len + 1
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->tuner_address, buf,
> + len + 1);
>
> dprintk(2, "\n");
>
> @@ -313,17 +309,10 @@ static u32 mt2063_read(struct mt2063_state *state,
> for (i = 0; i < cnt; i++) {
> u8 b0[] = { subAddress + i };
> struct i2c_msg msg[] = {
> - {
> - .addr = state->config->tuner_address,
> - .flags = 0,
> - .buf = b0,
> - .len = 1
> - }, {
> - .addr = state->config->tuner_address,
> - .flags = I2C_M_RD,
> - .buf = pData + i,
> - .len = 1
> - }
> + I2C_MSG_WRITE(state->config->tuner_address, b0,
> + sizeof(b0)),
> + I2C_MSG_READ(state->config->tuner_address,
> + pData + i, 1)

Same here. The b0 "array" is pretty ugly, could easily be fixed in a
follow up patch.

> };
>
> status = i2c_transfer(state->i2c, msg, 2);
> diff --git a/drivers/media/tuners/mt2131.c b/drivers/media/tuners/mt2131.c
> index f83b0c1..a154901 100644
> --- a/drivers/media/tuners/mt2131.c
> +++ b/drivers/media/tuners/mt2131.c
> @@ -53,10 +53,8 @@ static u8 mt2131_config2[] = {
> static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->cfg->i2c_address, .flags = 0,
> - .buf = &reg, .len = 1 },
> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
> - .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
> };
>
> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
> @@ -69,8 +67,8 @@ static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
> static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = { reg, val };
> - struct i2c_msg msg = { .addr = priv->cfg->i2c_address, .flags = 0,
> - .buf = buf, .len = 2 };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
> + sizeof(buf));
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "mt2131 I2C write failed\n");
> @@ -81,8 +79,7 @@ static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)
>
> static int mt2131_writeregs(struct mt2131_priv *priv,u8 *buf, u8 len)
> {
> - struct i2c_msg msg = { .addr = priv->cfg->i2c_address,
> - .flags = 0, .buf = buf, .len = len };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
>
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "mt2131 I2C write failed (len=%i)\n",
> diff --git a/drivers/media/tuners/mt2266.c b/drivers/media/tuners/mt2266.c
> index bca4d75..a714728 100644
> --- a/drivers/media/tuners/mt2266.c
> +++ b/drivers/media/tuners/mt2266.c
> @@ -57,8 +57,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
> static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
> {
> struct i2c_msg msg[2] = {
> - { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),

And again.

> };
> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
> printk(KERN_WARNING "MT2266 I2C read failed\n");
> @@ -71,9 +71,8 @@ static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
> static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
> {
> u8 buf[2] = { reg, val };
> - struct i2c_msg msg = {
> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
> + sizeof(buf));
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "MT2266 I2C write failed\n");
> return -EREMOTEIO;
> @@ -84,9 +83,7 @@ static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
> // Writes a set of consecutive registers
> static int mt2266_writeregs(struct mt2266_priv *priv,u8 *buf, u8 len)
> {
> - struct i2c_msg msg = {
> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
> - };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
> printk(KERN_WARNING "MT2266 I2C write failed (len=%i)\n",(int)len);
> return -EREMOTEIO;
> diff --git a/drivers/media/tuners/mxl5005s.c b/drivers/media/tuners/mxl5005s.c
> index b473b76..c913322 100644
> --- a/drivers/media/tuners/mxl5005s.c
> +++ b/drivers/media/tuners/mxl5005s.c
> @@ -3848,8 +3848,8 @@ static int mxl5005s_reset(struct dvb_frontend *fe)
> int ret = 0;
>
> u8 buf[2] = { 0xff, 0x00 };
> - struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
> - .buf = buf, .len = 2 };
> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
> + sizeof(buf));
>
> dprintk(2, "%s()\n", __func__);
>
> @@ -3874,8 +3874,8 @@ static int mxl5005s_writereg(struct dvb_frontend *fe, u8 reg, u8 val, int latch)
> {
> struct mxl5005s_state *state = fe->tuner_priv;
> u8 buf[3] = { reg, val, MXL5005S_LATCH_BYTE };
> - struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
> - .buf = buf, .len = 3 };
> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
> + sizeof(buf));
>
> if (latch == 0)
> msg.len = 2;
> diff --git a/drivers/media/tuners/tda827x.c b/drivers/media/tuners/tda827x.c
> index a0d1762..15a4802 100644
> --- a/drivers/media/tuners/tda827x.c
> +++ b/drivers/media/tuners/tda827x.c
> @@ -159,8 +159,7 @@ static int tda827xo_set_params(struct dvb_frontend *fe)
> u8 buf[14];
> int rc;
>
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = buf, .len = sizeof(buf) };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
> int i, tuner_freq, if_freq;
> u32 N;
>
> @@ -233,8 +232,7 @@ static int tda827xo_sleep(struct dvb_frontend *fe)
> {
> struct tda827x_priv *priv = fe->tuner_priv;
> static u8 buf[] = { 0x30, 0xd0 };
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = buf, .len = sizeof(buf) };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>
> dprintk("%s:\n", __func__);
> tuner_transfer(fe, &msg, 1);
> @@ -255,7 +253,7 @@ static int tda827xo_set_analog_params(struct dvb_frontend *fe,
> u32 N;
> int i;
> struct tda827x_priv *priv = fe->tuner_priv;
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0 };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, NULL, 0);
> unsigned int freq = params->frequency;
>
> tda827x_set_std(fe, params);
> @@ -335,8 +333,7 @@ static void tda827xo_agcf(struct dvb_frontend *fe)
> {
> struct tda827x_priv *priv = fe->tuner_priv;
> unsigned char data[] = { 0x80, 0x0c };
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = data, .len = 2};
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));
>
> tuner_transfer(fe, &msg, 1);
> }
> @@ -445,8 +442,7 @@ static int tda827xa_sleep(struct dvb_frontend *fe)
> {
> struct tda827x_priv *priv = fe->tuner_priv;
> static u8 buf[] = { 0x30, 0x90 };
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = buf, .len = sizeof(buf) };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>
> dprintk("%s:\n", __func__);
>
> @@ -465,7 +461,7 @@ static void tda827xa_lna_gain(struct dvb_frontend *fe, int high,
> unsigned char buf[] = {0x22, 0x01};
> int arg;
> int gp_func;
> - struct i2c_msg msg = { .flags = 0, .buf = buf, .len = sizeof(buf) };
> + struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));
>
> if (NULL == priv->cfg) {
> dprintk("tda827x_config not defined, cannot set LNA gain!\n");
> @@ -518,8 +514,7 @@ static int tda827xa_set_params(struct dvb_frontend *fe)
> struct tda827xa_data *frequency_map = tda827xa_dvbt;
> u8 buf[11];
>
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = buf, .len = sizeof(buf) };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>
> int i, tuner_freq, if_freq, rc;
> u32 N;
> @@ -665,8 +660,8 @@ static int tda827xa_set_analog_params(struct dvb_frontend *fe,
> u32 N;
> int i;
> struct tda827x_priv *priv = fe->tuner_priv;
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
> - .buf = tuner_reg, .len = sizeof(tuner_reg) };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, tuner_reg,
> + sizeof(tuner_reg));
> unsigned int freq = params->frequency;
>
> tda827x_set_std(fe, params);
> @@ -760,8 +755,7 @@ static void tda827xa_agcf(struct dvb_frontend *fe)
> {
> struct tda827x_priv *priv = fe->tuner_priv;
> unsigned char data[] = {0x80, 0x2c};
> - struct i2c_msg msg = {.addr = priv->i2c_addr, .flags = 0,
> - .buf = data, .len = 2};
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));
> tuner_transfer(fe, &msg, 1);
> }
>
> @@ -855,8 +849,7 @@ static int tda827x_probe_version(struct dvb_frontend *fe)
> u8 data;
> int rc;
> struct tda827x_priv *priv = fe->tuner_priv;
> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = I2C_M_RD,
> - .buf = &data, .len = 1 };
> + struct i2c_msg msg = I2C_MSG_READ(priv->i2c_addr, &data, sizeof(data));
>
> rc = tuner_transfer(fe, &msg, 1);
>
> diff --git a/drivers/media/tuners/tuner-i2c.h b/drivers/media/tuners/tuner-i2c.h
> index 18f0056..a8a6da6 100644
> --- a/drivers/media/tuners/tuner-i2c.h
> +++ b/drivers/media/tuners/tuner-i2c.h
> @@ -35,8 +35,7 @@ struct tuner_i2c_props {
>
> static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf, int len)
> {
> - struct i2c_msg msg = { .addr = props->addr, .flags = 0,
> - .buf = buf, .len = len };
> + struct i2c_msg msg = I2C_MSG_WRITE(props->addr, buf, len);
> int ret = i2c_transfer(props->adap, &msg, 1);
>
> return (ret == 1) ? len : ret;
> @@ -44,8 +43,7 @@ static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf,
>
> static inline int tuner_i2c_xfer_recv(struct tuner_i2c_props *props, char *buf, int len)
> {
> - struct i2c_msg msg = { .addr = props->addr, .flags = I2C_M_RD,
> - .buf = buf, .len = len };
> + struct i2c_msg msg = I2C_MSG_READ(props->addr, buf, len);
> int ret = i2c_transfer(props->adap, &msg, 1);
>
> return (ret == 1) ? len : ret;
> @@ -55,10 +53,8 @@ static inline int tuner_i2c_xfer_send_recv(struct tuner_i2c_props *props,
> char *obuf, int olen,
> char *ibuf, int ilen)
> {
> - struct i2c_msg msg[2] = { { .addr = props->addr, .flags = 0,
> - .buf = obuf, .len = olen },
> - { .addr = props->addr, .flags = I2C_M_RD,
> - .buf = ibuf, .len = ilen } };
> + struct i2c_msg msg[2] = { I2C_MSG_WRITE(props->addr, obuf, olen),
> + I2C_MSG_READ(props->addr, ibuf, ilen) };

Consistent coding style with other initialisers would be:

struct i2c_msg msg[2] = {
I2C_MSG_WRITE(props->addr, obuf, olen),
I2C_MSG_READ(props->addr, ibuf, ilen),
};

> int ret = i2c_transfer(props->adap, msg, 2);
>
> return (ret == 2) ? ilen : ret;
> diff --git a/drivers/media/tuners/tuner-simple.c b/drivers/media/tuners/tuner-simple.c
> index 39e7e58..df5ba78 100644
> --- a/drivers/media/tuners/tuner-simple.c
> +++ b/drivers/media/tuners/tuner-simple.c
> @@ -1065,10 +1065,7 @@ struct dvb_frontend *simple_tuner_attach(struct dvb_frontend *fe,
> */
> if (i2c_adap != NULL) {
> u8 b[1];
> - struct i2c_msg msg = {
> - .addr = i2c_addr, .flags = I2C_M_RD,
> - .buf = b, .len = 1,
> - };
> + struct i2c_msg msg = I2C_MSG_READ(i2c_addr, b, sizeof(b));

More silly "arrays" :-/. Can be fixed later.

>
> if (fe->ops.i2c_gate_ctrl)
> fe->ops.i2c_gate_ctrl(fe, 1);
> diff --git a/drivers/media/tuners/xc4000.c b/drivers/media/tuners/xc4000.c
> index 4937712..7b65b6b 100644
> --- a/drivers/media/tuners/xc4000.c
> +++ b/drivers/media/tuners/xc4000.c
> @@ -256,8 +256,7 @@ static void xc_debug_dump(struct xc4000_priv *priv);
>
> static int xc_send_i2c_data(struct xc4000_priv *priv, u8 *buf, int len)
> {
> - struct i2c_msg msg = { .addr = priv->i2c_props.addr,
> - .flags = 0, .buf = buf, .len = len };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);
> if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
> if (priv->ignore_i2c_write_errors == 0) {
> printk(KERN_ERR "xc4000: I2C write failed (len=%i)\n",
> @@ -550,10 +549,8 @@ static int xc4000_readreg(struct xc4000_priv *priv, u16 reg, u16 *val)
> u8 buf[2] = { reg >> 8, reg & 0xff };
> u8 bval[2] = { 0, 0 };
> struct i2c_msg msg[2] = {
> - { .addr = priv->i2c_props.addr,
> - .flags = 0, .buf = &buf[0], .len = 2 },
> - { .addr = priv->i2c_props.addr,
> - .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
> + I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
> + I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),

&buf[0] == buf. Might as well fix this now, the way it is written is
just silly.

> };
>
> if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {
> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
> index dc93cf3..33b6b1e 100644
> --- a/drivers/media/tuners/xc5000.c
> +++ b/drivers/media/tuners/xc5000.c
> @@ -253,8 +253,7 @@ static int xc5000_TunerReset(struct dvb_frontend *fe);
>
> static int xc_send_i2c_data(struct xc5000_priv *priv, u8 *buf, int len)
> {
> - struct i2c_msg msg = { .addr = priv->i2c_props.addr,
> - .flags = 0, .buf = buf, .len = len };
> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);
>
> if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
> printk(KERN_ERR "xc5000: I2C write failed (len=%i)\n", len);
> @@ -285,10 +284,8 @@ static int xc5000_readreg(struct xc5000_priv *priv, u16 reg, u16 *val)
> u8 buf[2] = { reg >> 8, reg & 0xff };
> u8 bval[2] = { 0, 0 };
> struct i2c_msg msg[2] = {
> - { .addr = priv->i2c_props.addr,
> - .flags = 0, .buf = &buf[0], .len = 2 },
> - { .addr = priv->i2c_props.addr,
> - .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
> + I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
> + I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),

Same here, change &buf[0] to buf.

> };
>
> if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {
>

~Ryan

2012-10-07 22:14:33

by Ryan Mallon

Subject: Re: [PATCH 12/13] drivers/media/tuners/max2165.c: use macros for i2c_msg initialization

On 08/10/12 02:38, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> A length expressed as an explicit constant is also re-expressed as the size
> of the buffer, when this is possible.
>
> The second case is simplified to use simple variables rather than arrays.
> The variable b0 is dropped completely, and the variable reg that it
> contains is used instead. The variable b1 is replaced by a u8-typed
> variable named buf (the name used earlier in the file). The uses of b1 are
> then adjusted accordingly.
>
> A simplified version of the semantic patch that makes this change is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
> + I2C_MSG_READ(a,b,c)
> ;
>
> @@
> expression a,b,c;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = 0}
> + I2C_MSG_WRITE(a,b,c)
> ;
>
> @@
> expression a,b,c,d;
> identifier x;
> @@
>
> struct i2c_msg x =
> - {.addr = a, .buf = b, .len = c, .flags = d}
> + I2C_MSG_OP(a,b,c,d)
> ;
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
> ---
>
> drivers/media/tuners/max2165.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/tuners/max2165.c b/drivers/media/tuners/max2165.c
> index ba84936..6638617 100644
> --- a/drivers/media/tuners/max2165.c
> +++ b/drivers/media/tuners/max2165.c
> @@ -47,7 +47,7 @@ static int max2165_write_reg(struct max2165_priv *priv, u8 reg, u8 data)
> {
> int ret;
> u8 buf[] = { reg, data };
> - struct i2c_msg msg = { .flags = 0, .buf = buf, .len = 2 };
> + struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));
>
> msg.addr = priv->config->i2c_address;
>
> @@ -68,11 +68,10 @@ static int max2165_read_reg(struct max2165_priv *priv, u8 reg, u8 *p_data)
> int ret;
> u8 dev_addr = priv->config->i2c_address;
>
> - u8 b0[] = { reg };
> - u8 b1[] = { 0 };
> + u8 buf;
> struct i2c_msg msg[] = {
> - { .addr = dev_addr, .flags = 0, .buf = b0, .len = 1 },
> - { .addr = dev_addr, .flags = I2C_M_RD, .buf = b1, .len = 1 },
> + I2C_MSG_WRITE(dev_addr, &reg, sizeof(reg)),
> + I2C_MSG_READ(dev_addr, &buf, sizeof(buf)),
> };

Not sure if the array changes should be done here or as a separate
patch. Some of the other patches also have cases where single index
arrays (both buffers and messages) could be converted. Should either
convert all or none of them. I think its probably best to do as a
separate series on top of this though.

~Ryan

>
> ret = i2c_transfer(priv->i2c, msg, 2);
> @@ -81,10 +80,10 @@ static int max2165_read_reg(struct max2165_priv *priv, u8 reg, u8 *p_data)
> return -EIO;
> }
>
> - *p_data = b1[0];
> + *p_data = buf;
> if (debug >= 2)
> dprintk("%s: reg=0x%02X, data=0x%02X\n",
> - __func__, reg, b1[0]);
> + __func__, reg, buf);
> return 0;
> }
>
>

2012-10-08 01:57:05

by Mauro Carvalho Chehab

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

Em Sun, 07 Oct 2012 14:51:58 -0700
Joe Perches <[email protected]> escreveu:

> On Sun, 2012-10-07 at 23:43 +0200, Julia Lawall wrote:
> > On Sun, 7 Oct 2012, Joe Perches wrote:
> > >> Are READ and WRITE the action names? They are really the important
> > >> information in this case.
> > >
> > > Yes, most (all?) uses of _READ and _WRITE macros actually
> > > perform some I/O.
> >
> > I2C_MSG_READ_DATA?
> > I2C_MSG_READ_INFO?
> > I2C_MSG_READ_INIT?
> > I2C_MSG_PREPARE_READ?
>
> Dunno, naming is hard. Maybe:
>
> I2C_INPUT_MSG
> I2C_OUTPUT_MSG
> I2C_OP_MSG

READ/WRITE sounds better, IMHO, as it will generally match with the
function names (they're generally named like foo_i2c_read or foo_reg_read).
I think none of them uses input or output. Btw, with I2C buses, a
register read is coded as a write ops, that sets the register's sub-address,
followed by a read ops from that (and subsequent) registers.

I'm afraid that using INPUT/OUTPUT will sound confusing.

So, IMHO, I2C_READ_MSG and I2C_WRITE_MSG sounds better.

Btw, as the WRITE + READ operation is very common (I think it is
much more common than a simple READ msg), it could make sense to have
just one macro for it, like:

INIT_I2C_READ_SUBADDR() that would take both write and read values.

I also don't like the I2C_MSG_OP. The operations there are always
read or write.

So, IMHO, the better would be to code the read and write message init message
as something similar to:

#define DECLARE_I2C_MSG_READ(_msg, _addr, _buf, _len, _flags) \
struct i2c_msg _msg[1] = { \
{.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
}

#define DECLARE_I2C_MSG_WRITE(_msg, _addr, _buf, _len, _flags) \
struct i2c_msg _msg[1] = { \
{.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) & ~I2C_M_RD } \
}

#define DECLARE_I2C_MSG_READ_SUBADDR(_msg, _addr, _subaddr, _sublen,_subflags, _buf,_len, _flags) \
struct i2c_msg _msg[2] = { \
{.addr = _addr, .buf = _subbuf, .len = _sublen, .flags = (_subflags) & ~I2C_M_RD } \
{.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
}


Notes:

1) in the case of DECLARE_I2C_MSG_READ_SUBADDR(), I'm almost sure that, in all cases, the
first message will always have buffer size equal to 1. If so, you can simplify the number
of arguments there.

2) It could make sense to have, in fact, two versions for each, one with _FLAGS and another one
without. On most cases, the one without flags are used.

3) I bet that most of the cases where 2 messages are used, the first message has length equal
to one, and it uses a fixed u8 constant with the I2C sub-address. So, maybe it would be nice
to have a variant for this case.


Cheers,
Mauro

2012-10-08 02:11:54

by Ryan Mallon

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On 08/10/12 12:56, Mauro Carvalho Chehab wrote:
> Em Sun, 07 Oct 2012 14:51:58 -0700
> Joe Perches <[email protected]> escreveu:
>
>> On Sun, 2012-10-07 at 23:43 +0200, Julia Lawall wrote:
>>> On Sun, 7 Oct 2012, Joe Perches wrote:
>>>>> Are READ and WRITE the action names? They are really the important
>>>>> information in this case.
>>>>
>>>> Yes, most (all?) uses of _READ and _WRITE macros actually
>>>> perform some I/O.
>>>
>>> I2C_MSG_READ_DATA?
>>> I2C_MSG_READ_INFO?
>>> I2C_MSG_READ_INIT?
>>> I2C_MSG_PREPARE_READ?
>>
>> Dunno, naming is hard. Maybe:
>>
>> I2C_INPUT_MSG
>> I2C_OUTPUT_MSG
>> I2C_OP_MSG
>
> READ/WRITE sounds better, IMHO, as it will generally match with the
> function names (they're generally named like foo_i2c_read or foo_reg_read).
> I think none of them uses input or output. Btw, with I2C buses, a
> register read is coded as a write ops, that sets the register's sub-address,
> followed by a read ops from that (and subsequent) registers.
>
> I'm afraid that using INPUT/OUTPUT will sound confusing.
>
> So, IMHO, I2C_READ_MSG and I2C_WRITE_MSG sounds better.
>
> Btw, as the WRITE + READ operation is very common (I think it is
> much more common than a simple READ msg), it could make sense to have
> just one macro for it, like:
>
> INIT_I2C_READ_SUBADDR() that would take both write and read values.
>
> I also don't like the I2C_MSG_OP. The operations there are always
> read or write.
>
> So, IMHO, the better would be to code the read and write message init message
> as something similar to:
>
> #define DECLARE_I2C_MSG_READ(_msg, _addr, _buf, _len, _flags) \
> struct i2c_msg _msg[1] = { \
> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
> }
>
> #define DECLARE_I2C_MSG_WRITE(_msg, _addr, _buf, _len, _flags) \
> struct i2c_msg _msg[1] = { \
> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) & ~I2C_M_RD } \
> }
>
> #define DECLARE_I2C_MSG_READ_SUBADDR(_msg, _addr, _subaddr, _sublen,_subflags, _buf,_len, _flags) \
> struct i2c_msg _msg[2] = { \
> {.addr = _addr, .buf = _subbuf, .len = _sublen, .flags = (_subflags) & ~I2C_M_RD } \
> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
> }

I think this is probably more confusing, not less. The macro takes 8
arguments, and in many cases will wrap onto two or more lines. The large
number of arguments also makes it difficult for a casual reader to
determine exactly what it does. In comparison:

I2C_MSG_WRITE(info->i2c_addr, &reg, 1);
I2C_MSG_READ(info->i2c_addr, buf, sizeof(buf));

is fairly self-explanatory, especially for someone familiar with i2c,
without having to look up the macro definitions.

> Notes:
>
> 1) in the case of DECLARE_I2C_MSG_READ_SUBADDR(), I'm almost sure that, in all cases, the
> first message will always have buffer size equal to 1. If so, you can simplify the number
> of arguments there.
>
> 2) It could make sense to have, in fact, two versions for each, one with _FLAGS and another one
> without. On most cases, the one without flags are used.
>
> 3) I bet that most of the cases where 2 messages are used, the first message has length equal
> to one, and it uses a fixed u8 constant with the I2C sub-address. So, maybe it would be nice
> to have a variant for this case.

That ends up with a whole bunch of variants of the macro for something
which should be very simple. The proposal has three macros, and the
I2C_MSG_OP macro is only required for a one or two corner cases where
non-standard flags are used.

~Ryan

2012-10-08 05:04:25

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012, Joe Perches wrote:

> On Sun, 2012-10-07 at 23:43 +0200, Julia Lawall wrote:
>> On Sun, 7 Oct 2012, Joe Perches wrote:
>>>> Are READ and WRITE the action names? They are really the important
>>>> information in this case.
>>>
>>> Yes, most (all?) uses of _READ and _WRITE macros actually
>>> perform some I/O.
>>
>> I2C_MSG_READ_DATA?
>> I2C_MSG_READ_INFO?
>> I2C_MSG_READ_INIT?
>> I2C_MSG_PREPARE_READ?
>
> Dunno, naming is hard. Maybe:
>
> I2C_INPUT_MSG
> I2C_OUTPUT_MSG
> I2C_OP_MSG

The current terminology, however, is READ, not INPUT (.flags = I2C_M_RD).

julia

2012-10-08 05:05:47

by Julia Lawall

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

On Mon, 8 Oct 2012, Ryan Mallon wrote:

> On 08/10/12 02:38, Julia Lawall wrote:
>> From: Julia Lawall <[email protected]>
>>
>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>
>> A length expressed as an explicit constant is also re-expressed as the size
>> of the buffer, when this is possible.
>>
>> A simplified version of the semantic patch that makes this change is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>> + I2C_MSG_READ(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>> + I2C_MSG_WRITE(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c,d;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = d}
>> + I2C_MSG_OP(a,b,c,d)
>> ;
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/media/tuners/qt1010.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/tuners/qt1010.c b/drivers/media/tuners/qt1010.c
>> index bc419f8..37ff254 100644
>> --- a/drivers/media/tuners/qt1010.c
>> +++ b/drivers/media/tuners/qt1010.c
>> @@ -25,10 +25,8 @@
>> static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->cfg->i2c_address,
>> - .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->cfg->i2c_address,
>> - .flags = I2C_M_RD, .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
>
> This is a bit inconsistent. For single length values we should either
> consistently use sizeof(val) or 1. This has both.

val is a pointer. It does not have size 1.

julia

2012-10-08 05:12:39

by Julia Lawall

Subject: Re: [PATCH 5/13] drivers/media/tuners: use macros for i2c_msg initialization

As far as I can see, the comments on this are:

&b[0] should be just b
There should be a newline before the elements of a multi-element array
Some one-element array buffers should just be variables
sizeof(val) should be used

I will redo the patches to include the first two, but not the others.

I will send another set of patches for the third one. It seems like the
conclusion is that the buffer should always be a variable if it can, but
the message should always be an array for uniformity in the call to
i2c_transfer.

I believe that the fourth one is not correct. The variable in question is
a pointer, so sizeof would give the wrong result. If it is preferred, I
could not use sizeof for the other structure in the same code, but it
seems just a little safer to use it.

thanks,
julia


On Mon, 8 Oct 2012, Ryan Mallon wrote:

> On 08/10/12 02:38, Julia Lawall wrote:
>> From: Julia Lawall <[email protected]>
>>
>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>
>> A length expressed as an explicit constant is also re-expressed as the size
>> of the buffer, when this is possible.
>>
>> A simplified version of the semantic patch that makes this change is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>> + I2C_MSG_READ(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>> + I2C_MSG_WRITE(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c,d;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = d}
>> + I2C_MSG_OP(a,b,c,d)
>> ;
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>> drivers/media/tuners/fc0012.c | 8 +++-----
>> drivers/media/tuners/fc0013.c | 8 +++-----
>> drivers/media/tuners/mc44s803.c | 8 +++-----
>> drivers/media/tuners/mt2060.c | 13 +++++--------
>> drivers/media/tuners/mt2063.c | 23 ++++++-----------------
>> drivers/media/tuners/mt2131.c | 13 +++++--------
>> drivers/media/tuners/mt2266.c | 13 +++++--------
>> drivers/media/tuners/mxl5005s.c | 8 ++++----
>> drivers/media/tuners/tda827x.c | 29 +++++++++++------------------
>> drivers/media/tuners/tuner-i2c.h | 12 ++++--------
>> drivers/media/tuners/tuner-simple.c | 5 +----
>> drivers/media/tuners/xc4000.c | 9 +++------
>> drivers/media/tuners/xc5000.c | 9 +++------
>> 13 files changed, 56 insertions(+), 102 deletions(-)
>>
>> diff --git a/drivers/media/tuners/fc0012.c b/drivers/media/tuners/fc0012.c
>> index 308135a..01dac7e 100644
>> --- a/drivers/media/tuners/fc0012.c
>> +++ b/drivers/media/tuners/fc0012.c
>> @@ -24,9 +24,7 @@
>> static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = {reg, val};
>> - struct i2c_msg msg = {
>> - .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
>> @@ -38,8 +36,8 @@ static int fc0012_writereg(struct fc0012_priv *priv, u8 reg, u8 val)
>> static int fc0012_readreg(struct fc0012_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->addr, val, 1),
>> };
>>
>> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>> diff --git a/drivers/media/tuners/fc0013.c b/drivers/media/tuners/fc0013.c
>> index bd8f0f1..174f0b0 100644
>> --- a/drivers/media/tuners/fc0013.c
>> +++ b/drivers/media/tuners/fc0013.c
>> @@ -27,9 +27,7 @@
>> static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = {reg, val};
>> - struct i2c_msg msg = {
>> - .addr = priv->addr, .flags = 0, .buf = buf, .len = 2
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->addr, buf, sizeof(buf));
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> err("I2C write reg failed, reg: %02x, val: %02x", reg, val);
>> @@ -41,8 +39,8 @@ static int fc0013_writereg(struct fc0013_priv *priv, u8 reg, u8 val)
>> static int fc0013_readreg(struct fc0013_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->addr, .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->addr, val, 1),
>> };
>>
>> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>> diff --git a/drivers/media/tuners/mc44s803.c b/drivers/media/tuners/mc44s803.c
>> index f1b7640..df47e03 100644
>> --- a/drivers/media/tuners/mc44s803.c
>> +++ b/drivers/media/tuners/mc44s803.c
>> @@ -37,9 +37,8 @@
>> static int mc44s803_writereg(struct mc44s803_priv *priv, u32 val)
>> {
>> u8 buf[3];
>> - struct i2c_msg msg = {
>> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 3
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
>> + sizeof(buf));
>>
>> buf[0] = (val & 0xff0000) >> 16;
>> buf[1] = (val & 0xff00) >> 8;
>> @@ -59,8 +58,7 @@ static int mc44s803_readreg(struct mc44s803_priv *priv, u8 reg, u32 *val)
>> u8 buf[3];
>> int ret;
>> struct i2c_msg msg[] = {
>> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
>> - .buf = buf, .len = 3 },
>> + I2C_MSG_READ(priv->cfg->i2c_address, buf, sizeof(buf)),
>> };
>>
>> wval = MC44S803_REG_SM(MC44S803_REG_DATAREG, MC44S803_ADDR) |
>> diff --git a/drivers/media/tuners/mt2060.c b/drivers/media/tuners/mt2060.c
>> index 13381de..5fb2e77 100644
>> --- a/drivers/media/tuners/mt2060.c
>> +++ b/drivers/media/tuners/mt2060.c
>> @@ -42,8 +42,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
>> static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
>
> Same here, should consistently use either sizeof(val) or 1 for single
> length items.
>
>> };
>>
>> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>> @@ -57,9 +57,8 @@ static int mt2060_readreg(struct mt2060_priv *priv, u8 reg, u8 *val)
>> static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = { reg, val };
>> - struct i2c_msg msg = {
>> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
>> + sizeof(buf));
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "mt2060 I2C write failed\n");
>> @@ -71,9 +70,7 @@ static int mt2060_writereg(struct mt2060_priv *priv, u8 reg, u8 val)
>> // Writes a set of consecutive registers
>> static int mt2060_writeregs(struct mt2060_priv *priv,u8 *buf, u8 len)
>> {
>> - struct i2c_msg msg = {
>> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "mt2060 I2C write failed (len=%i)\n",(int)len);
>> return -EREMOTEIO;
>> diff --git a/drivers/media/tuners/mt2063.c b/drivers/media/tuners/mt2063.c
>> index 0ed9091..c29f6f6 100644
>> --- a/drivers/media/tuners/mt2063.c
>> +++ b/drivers/media/tuners/mt2063.c
>> @@ -250,12 +250,8 @@ static u32 mt2063_write(struct mt2063_state *state, u8 reg, u8 *data, u32 len)
>> struct dvb_frontend *fe = state->frontend;
>> int ret;
>> u8 buf[60];
>> - struct i2c_msg msg = {
>> - .addr = state->config->tuner_address,
>> - .flags = 0,
>> - .buf = buf,
>> - .len = len + 1
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->tuner_address, buf,
>> + len + 1);
>>
>> dprintk(2, "\n");
>>
>> @@ -313,17 +309,10 @@ static u32 mt2063_read(struct mt2063_state *state,
>> for (i = 0; i < cnt; i++) {
>> u8 b0[] = { subAddress + i };
>> struct i2c_msg msg[] = {
>> - {
>> - .addr = state->config->tuner_address,
>> - .flags = 0,
>> - .buf = b0,
>> - .len = 1
>> - }, {
>> - .addr = state->config->tuner_address,
>> - .flags = I2C_M_RD,
>> - .buf = pData + i,
>> - .len = 1
>> - }
>> + I2C_MSG_WRITE(state->config->tuner_address, b0,
>> + sizeof(b0)),
>> + I2C_MSG_READ(state->config->tuner_address,
>> + pData + i, 1)
>
> Same here. The b0 "array" is pretty ugly, could easily be fixed in a
> follow up patch.
>
>> };
>>
>> status = i2c_transfer(state->i2c, msg, 2);
>> diff --git a/drivers/media/tuners/mt2131.c b/drivers/media/tuners/mt2131.c
>> index f83b0c1..a154901 100644
>> --- a/drivers/media/tuners/mt2131.c
>> +++ b/drivers/media/tuners/mt2131.c
>> @@ -53,10 +53,8 @@ static u8 mt2131_config2[] = {
>> static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->cfg->i2c_address, .flags = 0,
>> - .buf = &reg, .len = 1 },
>> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD,
>> - .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
>> };
>>
>> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>> @@ -69,8 +67,8 @@ static int mt2131_readreg(struct mt2131_priv *priv, u8 reg, u8 *val)
>> static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = { reg, val };
>> - struct i2c_msg msg = { .addr = priv->cfg->i2c_address, .flags = 0,
>> - .buf = buf, .len = 2 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
>> + sizeof(buf));
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "mt2131 I2C write failed\n");
>> @@ -81,8 +79,7 @@ static int mt2131_writereg(struct mt2131_priv *priv, u8 reg, u8 val)
>>
>> static int mt2131_writeregs(struct mt2131_priv *priv,u8 *buf, u8 len)
>> {
>> - struct i2c_msg msg = { .addr = priv->cfg->i2c_address,
>> - .flags = 0, .buf = buf, .len = len };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
>>
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "mt2131 I2C write failed (len=%i)\n",
>> diff --git a/drivers/media/tuners/mt2266.c b/drivers/media/tuners/mt2266.c
>> index bca4d75..a714728 100644
>> --- a/drivers/media/tuners/mt2266.c
>> +++ b/drivers/media/tuners/mt2266.c
>> @@ -57,8 +57,8 @@ MODULE_PARM_DESC(debug, "Turn on/off debugging (default:off).");
>> static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
>> {
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->cfg->i2c_address, .flags = 0, .buf = &reg, .len = 1 },
>> - { .addr = priv->cfg->i2c_address, .flags = I2C_M_RD, .buf = val, .len = 1 },
>> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
>> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
>
> And again.
>
>> };
>> if (i2c_transfer(priv->i2c, msg, 2) != 2) {
>> printk(KERN_WARNING "MT2266 I2C read failed\n");
>> @@ -71,9 +71,8 @@ static int mt2266_readreg(struct mt2266_priv *priv, u8 reg, u8 *val)
>> static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
>> {
>> u8 buf[2] = { reg, val };
>> - struct i2c_msg msg = {
>> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = 2
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf,
>> + sizeof(buf));
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "MT2266 I2C write failed\n");
>> return -EREMOTEIO;
>> @@ -84,9 +83,7 @@ static int mt2266_writereg(struct mt2266_priv *priv, u8 reg, u8 val)
>> // Writes a set of consecutive registers
>> static int mt2266_writeregs(struct mt2266_priv *priv,u8 *buf, u8 len)
>> {
>> - struct i2c_msg msg = {
>> - .addr = priv->cfg->i2c_address, .flags = 0, .buf = buf, .len = len
>> - };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->cfg->i2c_address, buf, len);
>> if (i2c_transfer(priv->i2c, &msg, 1) != 1) {
>> printk(KERN_WARNING "MT2266 I2C write failed (len=%i)\n",(int)len);
>> return -EREMOTEIO;
>> diff --git a/drivers/media/tuners/mxl5005s.c b/drivers/media/tuners/mxl5005s.c
>> index b473b76..c913322 100644
>> --- a/drivers/media/tuners/mxl5005s.c
>> +++ b/drivers/media/tuners/mxl5005s.c
>> @@ -3848,8 +3848,8 @@ static int mxl5005s_reset(struct dvb_frontend *fe)
>> int ret = 0;
>>
>> u8 buf[2] = { 0xff, 0x00 };
>> - struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
>> - .buf = buf, .len = 2 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
>> + sizeof(buf));
>>
>> dprintk(2, "%s()\n", __func__);
>>
>> @@ -3874,8 +3874,8 @@ static int mxl5005s_writereg(struct dvb_frontend *fe, u8 reg, u8 val, int latch)
>> {
>> struct mxl5005s_state *state = fe->tuner_priv;
>> u8 buf[3] = { reg, val, MXL5005S_LATCH_BYTE };
>> - struct i2c_msg msg = { .addr = state->config->i2c_address, .flags = 0,
>> - .buf = buf, .len = 3 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(state->config->i2c_address, buf,
>> + sizeof(buf));
>>
>> if (latch == 0)
>> msg.len = 2;
>> diff --git a/drivers/media/tuners/tda827x.c b/drivers/media/tuners/tda827x.c
>> index a0d1762..15a4802 100644
>> --- a/drivers/media/tuners/tda827x.c
>> +++ b/drivers/media/tuners/tda827x.c
>> @@ -159,8 +159,7 @@ static int tda827xo_set_params(struct dvb_frontend *fe)
>> u8 buf[14];
>> int rc;
>>
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = buf, .len = sizeof(buf) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>> int i, tuner_freq, if_freq;
>> u32 N;
>>
>> @@ -233,8 +232,7 @@ static int tda827xo_sleep(struct dvb_frontend *fe)
>> {
>> struct tda827x_priv *priv = fe->tuner_priv;
>> static u8 buf[] = { 0x30, 0xd0 };
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = buf, .len = sizeof(buf) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>>
>> dprintk("%s:\n", __func__);
>> tuner_transfer(fe, &msg, 1);
>> @@ -255,7 +253,7 @@ static int tda827xo_set_analog_params(struct dvb_frontend *fe,
>> u32 N;
>> int i;
>> struct tda827x_priv *priv = fe->tuner_priv;
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, NULL, 0);
>> unsigned int freq = params->frequency;
>>
>> tda827x_set_std(fe, params);
>> @@ -335,8 +333,7 @@ static void tda827xo_agcf(struct dvb_frontend *fe)
>> {
>> struct tda827x_priv *priv = fe->tuner_priv;
>> unsigned char data[] = { 0x80, 0x0c };
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = data, .len = 2};
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));
>>
>> tuner_transfer(fe, &msg, 1);
>> }
>> @@ -445,8 +442,7 @@ static int tda827xa_sleep(struct dvb_frontend *fe)
>> {
>> struct tda827x_priv *priv = fe->tuner_priv;
>> static u8 buf[] = { 0x30, 0x90 };
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = buf, .len = sizeof(buf) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>>
>> dprintk("%s:\n", __func__);
>>
>> @@ -465,7 +461,7 @@ static void tda827xa_lna_gain(struct dvb_frontend *fe, int high,
>> unsigned char buf[] = {0x22, 0x01};
>> int arg;
>> int gp_func;
>> - struct i2c_msg msg = { .flags = 0, .buf = buf, .len = sizeof(buf) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));
>>
>> if (NULL == priv->cfg) {
>> dprintk("tda827x_config not defined, cannot set LNA gain!\n");
>> @@ -518,8 +514,7 @@ static int tda827xa_set_params(struct dvb_frontend *fe)
>> struct tda827xa_data *frequency_map = tda827xa_dvbt;
>> u8 buf[11];
>>
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = buf, .len = sizeof(buf) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, buf, sizeof(buf));
>>
>> int i, tuner_freq, if_freq, rc;
>> u32 N;
>> @@ -665,8 +660,8 @@ static int tda827xa_set_analog_params(struct dvb_frontend *fe,
>> u32 N;
>> int i;
>> struct tda827x_priv *priv = fe->tuner_priv;
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = 0,
>> - .buf = tuner_reg, .len = sizeof(tuner_reg) };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, tuner_reg,
>> + sizeof(tuner_reg));
>> unsigned int freq = params->frequency;
>>
>> tda827x_set_std(fe, params);
>> @@ -760,8 +755,7 @@ static void tda827xa_agcf(struct dvb_frontend *fe)
>> {
>> struct tda827x_priv *priv = fe->tuner_priv;
>> unsigned char data[] = {0x80, 0x2c};
>> - struct i2c_msg msg = {.addr = priv->i2c_addr, .flags = 0,
>> - .buf = data, .len = 2};
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_addr, data, sizeof(data));
>> tuner_transfer(fe, &msg, 1);
>> }
>>
>> @@ -855,8 +849,7 @@ static int tda827x_probe_version(struct dvb_frontend *fe)
>> u8 data;
>> int rc;
>> struct tda827x_priv *priv = fe->tuner_priv;
>> - struct i2c_msg msg = { .addr = priv->i2c_addr, .flags = I2C_M_RD,
>> - .buf = &data, .len = 1 };
>> + struct i2c_msg msg = I2C_MSG_READ(priv->i2c_addr, &data, sizeof(data));
>>
>> rc = tuner_transfer(fe, &msg, 1);
>>
>> diff --git a/drivers/media/tuners/tuner-i2c.h b/drivers/media/tuners/tuner-i2c.h
>> index 18f0056..a8a6da6 100644
>> --- a/drivers/media/tuners/tuner-i2c.h
>> +++ b/drivers/media/tuners/tuner-i2c.h
>> @@ -35,8 +35,7 @@ struct tuner_i2c_props {
>>
>> static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf, int len)
>> {
>> - struct i2c_msg msg = { .addr = props->addr, .flags = 0,
>> - .buf = buf, .len = len };
>> + struct i2c_msg msg = I2C_MSG_WRITE(props->addr, buf, len);
>> int ret = i2c_transfer(props->adap, &msg, 1);
>>
>> return (ret == 1) ? len : ret;
>> @@ -44,8 +43,7 @@ static inline int tuner_i2c_xfer_send(struct tuner_i2c_props *props, char *buf,
>>
>> static inline int tuner_i2c_xfer_recv(struct tuner_i2c_props *props, char *buf, int len)
>> {
>> - struct i2c_msg msg = { .addr = props->addr, .flags = I2C_M_RD,
>> - .buf = buf, .len = len };
>> + struct i2c_msg msg = I2C_MSG_READ(props->addr, buf, len);
>> int ret = i2c_transfer(props->adap, &msg, 1);
>>
>> return (ret == 1) ? len : ret;
>> @@ -55,10 +53,8 @@ static inline int tuner_i2c_xfer_send_recv(struct tuner_i2c_props *props,
>> char *obuf, int olen,
>> char *ibuf, int ilen)
>> {
>> - struct i2c_msg msg[2] = { { .addr = props->addr, .flags = 0,
>> - .buf = obuf, .len = olen },
>> - { .addr = props->addr, .flags = I2C_M_RD,
>> - .buf = ibuf, .len = ilen } };
>> + struct i2c_msg msg[2] = { I2C_MSG_WRITE(props->addr, obuf, olen),
>> + I2C_MSG_READ(props->addr, ibuf, ilen) };
>
> Consistent coding style with other initialisers would be:
>
> struct i2c_msg msg[2] = {
> I2C_MSG_WRITE(props->addr, obuf, olen),
> I2C_MSG_READ(props->addr, ibuf, ilen),
> };
>
>> int ret = i2c_transfer(props->adap, msg, 2);
>>
>> return (ret == 2) ? ilen : ret;
>> diff --git a/drivers/media/tuners/tuner-simple.c b/drivers/media/tuners/tuner-simple.c
>> index 39e7e58..df5ba78 100644
>> --- a/drivers/media/tuners/tuner-simple.c
>> +++ b/drivers/media/tuners/tuner-simple.c
>> @@ -1065,10 +1065,7 @@ struct dvb_frontend *simple_tuner_attach(struct dvb_frontend *fe,
>> */
>> if (i2c_adap != NULL) {
>> u8 b[1];
>> - struct i2c_msg msg = {
>> - .addr = i2c_addr, .flags = I2C_M_RD,
>> - .buf = b, .len = 1,
>> - };
>> + struct i2c_msg msg = I2C_MSG_READ(i2c_addr, b, sizeof(b));
>
> More silly "arrays" :-/. Can be fixed later.
>
>>
>> if (fe->ops.i2c_gate_ctrl)
>> fe->ops.i2c_gate_ctrl(fe, 1);
>> diff --git a/drivers/media/tuners/xc4000.c b/drivers/media/tuners/xc4000.c
>> index 4937712..7b65b6b 100644
>> --- a/drivers/media/tuners/xc4000.c
>> +++ b/drivers/media/tuners/xc4000.c
>> @@ -256,8 +256,7 @@ static void xc_debug_dump(struct xc4000_priv *priv);
>>
>> static int xc_send_i2c_data(struct xc4000_priv *priv, u8 *buf, int len)
>> {
>> - struct i2c_msg msg = { .addr = priv->i2c_props.addr,
>> - .flags = 0, .buf = buf, .len = len };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);
>> if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
>> if (priv->ignore_i2c_write_errors == 0) {
>> printk(KERN_ERR "xc4000: I2C write failed (len=%i)\n",
>> @@ -550,10 +549,8 @@ static int xc4000_readreg(struct xc4000_priv *priv, u16 reg, u16 *val)
>> u8 buf[2] = { reg >> 8, reg & 0xff };
>> u8 bval[2] = { 0, 0 };
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->i2c_props.addr,
>> - .flags = 0, .buf = &buf[0], .len = 2 },
>> - { .addr = priv->i2c_props.addr,
>> - .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
>> + I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
>> + I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),
>
> &buf[0] == buf. Might as well fix this now, the way it is written is
> just silly.
>
>> };
>>
>> if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {
>> diff --git a/drivers/media/tuners/xc5000.c b/drivers/media/tuners/xc5000.c
>> index dc93cf3..33b6b1e 100644
>> --- a/drivers/media/tuners/xc5000.c
>> +++ b/drivers/media/tuners/xc5000.c
>> @@ -253,8 +253,7 @@ static int xc5000_TunerReset(struct dvb_frontend *fe);
>>
>> static int xc_send_i2c_data(struct xc5000_priv *priv, u8 *buf, int len)
>> {
>> - struct i2c_msg msg = { .addr = priv->i2c_props.addr,
>> - .flags = 0, .buf = buf, .len = len };
>> + struct i2c_msg msg = I2C_MSG_WRITE(priv->i2c_props.addr, buf, len);
>>
>> if (i2c_transfer(priv->i2c_props.adap, &msg, 1) != 1) {
>> printk(KERN_ERR "xc5000: I2C write failed (len=%i)\n", len);
>> @@ -285,10 +284,8 @@ static int xc5000_readreg(struct xc5000_priv *priv, u16 reg, u16 *val)
>> u8 buf[2] = { reg >> 8, reg & 0xff };
>> u8 bval[2] = { 0, 0 };
>> struct i2c_msg msg[2] = {
>> - { .addr = priv->i2c_props.addr,
>> - .flags = 0, .buf = &buf[0], .len = 2 },
>> - { .addr = priv->i2c_props.addr,
>> - .flags = I2C_M_RD, .buf = &bval[0], .len = 2 },
>> + I2C_MSG_WRITE(priv->i2c_props.addr, &buf[0], sizeof(buf)),
>> + I2C_MSG_READ(priv->i2c_props.addr, &bval[0], sizeof(bval)),
>
> Same here, change &buf[0] to buf.
>
>> };
>>
>> if (i2c_transfer(priv->i2c_props.adap, msg, 2) != 2) {
>>
>
> ~Ryan
>
>

2012-10-08 05:13:19

by Julia Lawall

Subject: Re: [PATCH 12/13] drivers/media/tuners/max2165.c: use macros for i2c_msg initialization

On Mon, 8 Oct 2012, Ryan Mallon wrote:

> On 08/10/12 02:38, Julia Lawall wrote:
>> From: Julia Lawall <[email protected]>
>>
>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>
>> A length expressed as an explicit constant is also re-expressed as the size
>> of the buffer, when this is possible.
>>
>> The second case is simplified to use simple variables rather than arrays.
>> The variable b0 is dropped completely, and the variable reg that it
>> contains is used instead. The variable b1 is replaced by a u8-typed
>> variable named buf (the name used earlier in the file). The uses of b1 are
>> then adjusted accordingly.
>>
>> A simplified version of the semantic patch that makes this change is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>> + I2C_MSG_READ(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>> + I2C_MSG_WRITE(a,b,c)
>> ;
>>
>> @@
>> expression a,b,c,d;
>> identifier x;
>> @@
>>
>> struct i2c_msg x =
>> - {.addr = a, .buf = b, .len = c, .flags = d}
>> + I2C_MSG_OP(a,b,c,d)
>> ;
>> // </smpl>
>>
>> Signed-off-by: Julia Lawall <[email protected]>
>>
>> ---
>>
>> drivers/media/tuners/max2165.c | 13 ++++++-------
>> 1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/tuners/max2165.c b/drivers/media/tuners/max2165.c
>> index ba84936..6638617 100644
>> --- a/drivers/media/tuners/max2165.c
>> +++ b/drivers/media/tuners/max2165.c
>> @@ -47,7 +47,7 @@ static int max2165_write_reg(struct max2165_priv *priv, u8 reg, u8 data)
>> {
>> int ret;
>> u8 buf[] = { reg, data };
>> - struct i2c_msg msg = { .flags = 0, .buf = buf, .len = 2 };
>> + struct i2c_msg msg = I2C_MSG_WRITE(0, buf, sizeof(buf));
>>
>> msg.addr = priv->config->i2c_address;
>>
>> @@ -68,11 +68,10 @@ static int max2165_read_reg(struct max2165_priv *priv, u8 reg, u8 *p_data)
>> int ret;
>> u8 dev_addr = priv->config->i2c_address;
>>
>> - u8 b0[] = { reg };
>> - u8 b1[] = { 0 };
>> + u8 buf;
>> struct i2c_msg msg[] = {
>> - { .addr = dev_addr, .flags = 0, .buf = b0, .len = 1 },
>> - { .addr = dev_addr, .flags = I2C_M_RD, .buf = b1, .len = 1 },
>> + I2C_MSG_WRITE(dev_addr, &reg, sizeof(reg)),
>> + I2C_MSG_READ(dev_addr, &buf, sizeof(buf)),
>> };
>
> Not sure if the array changes should be done here or as a separate
> patch. Some of the other patches also have cases where single index
> arrays (both buffers and messages) could be converted. Should either
> convert all or none of them. I think its probably best to do as a
> separate series on top of this though.

OK, I will do it that way.

thanks,
julia

2012-10-08 05:14:00

by Ryan Mallon

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

On 08/10/12 16:05, Julia Lawall wrote:
> On Mon, 8 Oct 2012, Ryan Mallon wrote:
>
>> On 08/10/12 02:38, Julia Lawall wrote:
>>> From: Julia Lawall <[email protected]>
>>>
>>> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>>>
>>> A length expressed as an explicit constant is also re-expressed as
>>> the size
>>> of the buffer, when this is possible.
>>>
>>> A simplified version of the semantic patch that makes this change is as
>>> follows: (http://coccinelle.lip6.fr/)
>>>
>>> // <smpl>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = I2C_M_RD}
>>> + I2C_MSG_READ(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = 0}
>>> + I2C_MSG_WRITE(a,b,c)
>>> ;
>>>
>>> @@
>>> expression a,b,c,d;
>>> identifier x;
>>> @@
>>>
>>> struct i2c_msg x =
>>> - {.addr = a, .buf = b, .len = c, .flags = d}
>>> + I2C_MSG_OP(a,b,c,d)
>>> ;
>>> // </smpl>
>>>
>>> Signed-off-by: Julia Lawall <[email protected]>
>>>
>>> ---
>>> drivers/media/tuners/qt1010.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/tuners/qt1010.c
>>> b/drivers/media/tuners/qt1010.c
>>> index bc419f8..37ff254 100644
>>> --- a/drivers/media/tuners/qt1010.c
>>> +++ b/drivers/media/tuners/qt1010.c
>>> @@ -25,10 +25,8 @@
>>> static int qt1010_readreg(struct qt1010_priv *priv, u8 reg, u8 *val)
>>> {
>>> struct i2c_msg msg[2] = {
>>> - { .addr = priv->cfg->i2c_address,
>>> - .flags = 0, .buf = &reg, .len = 1 },
>>> - { .addr = priv->cfg->i2c_address,
>>> - .flags = I2C_M_RD, .buf = val, .len = 1 },
>>> + I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
>>> + I2C_MSG_READ(priv->cfg->i2c_address, val, 1),
>>
>> This is a bit inconsistent. For single length values we should either
>> consistently use sizeof(val) or 1. This has both.
>
> val is a pointer. It does not have size 1.

Sorry, I mean either:

I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
I2C_MSG_READ(priv->cfg->i2c_address, val, sizeof(*val)),

or:

I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, 1),
I2C_MSG_READ(priv->cfg->i2c_address, val, 1),

~Ryan

2012-10-08 05:24:15

by Julia Lawall

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

> Sorry, I mean either:
>
> I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> I2C_MSG_READ(priv->cfg->i2c_address, val, sizeof(*val)),

Of course. Sorry for not having seen that. I can do that.

julia

2012-10-08 07:54:36

by Antti Palosaari

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

On 10/08/2012 05:11 AM, Ryan Mallon wrote:
> On 08/10/12 12:56, Mauro Carvalho Chehab wrote:
>> Em Sun, 07 Oct 2012 14:51:58 -0700
>> Joe Perches <[email protected]> escreveu:
>>
>>> On Sun, 2012-10-07 at 23:43 +0200, Julia Lawall wrote:
>>>> On Sun, 7 Oct 2012, Joe Perches wrote:
>>>>>> Are READ and WRITE the action names? They are really the important
>>>>>> information in this case.
>>>>>
>>>>> Yes, most (all?) uses of _READ and _WRITE macros actually
>>>>> perform some I/O.
>>>>
>>>> I2C_MSG_READ_DATA?
>>>> I2C_MSG_READ_INFO?
>>>> I2C_MSG_READ_INIT?
>>>> I2C_MSG_PREPARE_READ?
>>>
>>> Dunno, naming is hard. Maybe:
>>>
>>> I2C_INPUT_MSG
>>> I2C_OUTPUT_MSG
>>> I2C_OP_MSG
>>
>> READ/WRITE sounds better, IMHO, as it will generally match with the
>> function names (they're generally named like foo_i2c_read or foo_reg_read).
>> I think none of them uses input or output. Btw, with I2C buses, a
>> register read is coded as a write ops, that sets the register's sub-address,
>> followed by a read ops from that (and subsequent) registers.
>>
>> I'm afraid that using INPUT/OUTPUT will sound confusing.
>>
>> So, IMHO, I2C_READ_MSG and I2C_WRITE_MSG sounds better.
>>
>> Btw, as the WRITE + READ operation is very common (I think it is
>> much more common than a simple READ msg), it could make sense to have
>> just one macro for it, like:
>>
>> INIT_I2C_READ_SUBADDR() that would take both write and read values.
>>
>> I also don't like the I2C_MSG_OP. The operations there are always
>> read or write.
>>
>> So, IMHO, the better would be to code the read and write message init message
>> as something similar to:
>>
>> #define DECLARE_I2C_MSG_READ(_msg, _addr, _buf, _len, _flags) \
>> struct i2c_msg _msg[1] = { \
>> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
>> }
>>
>> #define DECLARE_I2C_MSG_WRITE(_msg, _addr, _buf, _len, _flags) \
>> struct i2c_msg _msg[1] = { \
>> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) & ~I2C_M_RD } \
>> }
>>
>> #define DECLARE_I2C_MSG_READ_SUBADDR(_msg, _addr, _subaddr, _sublen,_subflags, _buf,_len, _flags) \
>> struct i2c_msg _msg[2] = { \
>> {.addr = _addr, .buf = _subbuf, .len = _sublen, .flags = (_subflags) & ~I2C_M_RD } \
>> {.addr = _addr, .buf = _buf, .len = _len, .flags = (_flags) | I2C_M_RD } \
>> }
>
> I think this is probably more confusing, not less. The macro takes 8
> arguments, and in many cases will wrap onto two or more lines. The large
> number of arguments also makes it difficult for a casual reader to
> determine exactly what it does. In comparison:
>
> I2C_MSG_WRITE(info->i2c_addr, &reg, 1);
> I2C_MSG_READ(info->i2c_addr, buf, sizeof(buf));
>
> is fairly self-explanatory, especially for someone familiar with i2c,
> without having to look up the macro definitions.

I agree that.

>
>> Notes:
>>
>> 1) in the case of DECLARE_I2C_MSG_READ_SUBADDR(), I'm almost sure that, in all cases, the
>> first message will always have buffer size equal to 1. If so, you can simplify the number
>> of arguments there.
>>
>> 2) It could make sense to have, in fact, two versions for each, one with _FLAGS and another one
>> without. On most cases, the one without flags are used.
>>
>> 3) I bet that most of the cases where 2 messages are used, the first message has length equal
>> to one, and it uses a fixed u8 constant with the I2C sub-address. So, maybe it would be nice
>> to have a variant for this case.
>
> That ends up with a whole bunch of variants of the macro for something
> which should be very simple. The proposal has three macros, and the
> I2C_MSG_OP macro is only required for a one or two corner cases where
> non-standard flags are used.

I did some study around one year back to generalize these I2C access
routines, especially due to splitting logic needed very often. At that
point it come out there is regmap library which could do things like
that. I never looked it more carefully and thus I am not sure if it does
or not. Anyway, these I2C routines for register access are similar from
driver to driver and generalizing those could be very nice.

http://thread.gmane.org/gmane.linux.drivers.video-input-infrastructure/40186/focus=40204

regards
Antti

--
http://palosaari.fi/

2012-10-08 08:31:37

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

I found only 15 uses of I2C_MSG_OP, out of 653 uses of one of the three
macros. Since I2C_MSG_OP has the complete set of flags, I think it should
be OK?

One of the uses, in drivers/media/i2c/adv7604.c, is as follows:

struct i2c_msg msg[2] = { { client->addr, 0, 1, msgbuf0 },
{ client->addr, 0 | I2C_M_RD, len, msgbuf1 }

I'm not sure what was intended, but I guess the second structure is
supposed to only do a read?

julia

2012-10-09 12:06:57

by Jean Delvare

Subject: Re: [PATCH 9/13] drivers/media/tuners/fc0011.c: use macros for i2c_msg initialization

On Sun, 7 Oct 2012 18:50:31 +0200 (CEST), Julia Lawall wrote:
> On Sun, 7 Oct 2012, walter harms wrote:
> > Am 07.10.2012 17:38, schrieb Julia Lawall:
> >> @@ -97,10 +96,8 @@ static int fc0011_readreg(struct fc0011_priv *priv, u8 reg, u8 *val)
> >> {
> >> u8 dummy;
> >> struct i2c_msg msg[2] = {
> >> - { .addr = priv->addr,
> >> - .flags = 0, .buf = &reg, .len = 1 },
> >> - { .addr = priv->addr,
> >> - .flags = I2C_M_RD, .buf = val ? : &dummy, .len = 1 },
> >> + I2C_MSG_WRITE(priv->addr, &reg, sizeof(reg)),
> >> + I2C_MSG_READ(priv->addr, val ? : &dummy, sizeof(dummy)),
> >> };
> >>
> >
> > This dummy looks strange, can it be that this is used uninitialised ?
>
> I'm not sure to understand the question. The read, when it happens in
> i2c_transfer will initialize dummy. On the other hand, I don't know what
> i2c_transfer does when the buffer is NULL and the size is 1. It does not
> look very elegant at least.

i2c_transfer() itself won't care, it just passes the request down to the
underlying i2c bus driver. Most driver implementations will assume
proper buffer addresses as soon as size > 0, so passing NULL instead
would crash them. In short, don't do that.

--
Jean Delvare

2012-10-09 12:12:38

by Jean Delvare

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

Hi Julia,

On Mon, 8 Oct 2012 07:24:11 +0200 (CEST), Julia Lawall wrote:
> > Sorry, I mean either:
> >
> > I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> > I2C_MSG_READ(priv->cfg->i2c_address, val, sizeof(*val)),
>
> Of course. Sorry for not having seen that. I can do that.

Eek, no, you can't, not in the general case at least. sizeof(*val) will
return the size of the _first_ element of the destination buffer, which
has nothing to do with the length of that buffer (which in turn might
be rightfully longer than the read length for this specific message.)

--
Jean Delvare

2012-10-09 12:30:50

by Jean Delvare

Subject: Re: [PATCH 2/13] drivers/media/tuners/mxl5007t.c: use macros for i2c_msg initialization

Hi Julia,

On Sun, 7 Oct 2012 17:38:33 +0200, Julia Lawall wrote:
> From: Julia Lawall <[email protected]>
>
> Introduce use of I2c_MSG_READ/WRITE/OP, for readability.

Next time you send this patch set, please Cc me on every post so that I
don't have to hunt for them on lkml.org.

> In each case, a length expressed as an explicit constant is also
> re-expressed as the size of the buffer, when this is possible.

This is conceptually wrong, please don't do that. It is perfectly valid
to use a buffer which is larger than the message being written or read.
When exchanging multiple messages, it is actually quite common to
declare only 2 buffers and reuse them:

char reg;
char val[2];

struct i2c_msg msg[2] = {
{ .addr = addr, .flags = 0, .buf = &reg, .len = 1 },
{ .addr = addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
};

reg = 0x04;
i2c_transfer(i2c_adap, msg, 2);
/* Do stuff with val */

reg = 0x06;
msg[1].len = 2;
i2c_transfer(i2c_adap, msg, 2);
/* Do stuff with val */

Your conversion would read 2 bytes from register 0x04 instead of 1 in
the example above.

I am not opposed to the idea of i2c_msg initialization helper macros,
but please don't mix that with actual code changes which could have bad
side effects.

Thanks,
--
Jean Delvare

2012-10-09 12:50:06

by Julia Lawall

Subject: Re: [PATCH 2/13] drivers/media/tuners/mxl5007t.c: use macros for i2c_msg initialization

On Tue, 9 Oct 2012, Jean Delvare wrote:

> Hi Julia,
>
> On Sun, 7 Oct 2012 17:38:33 +0200, Julia Lawall wrote:
> > From: Julia Lawall <[email protected]>
> >
> > Introduce use of I2c_MSG_READ/WRITE/OP, for readability.
>
> Next time you send this patch set, please Cc me on every post so that I
> don't have to hunt for them on lkml.org.

OK.

> > In each case, a length expressed as an explicit constant is also
> > re-expressed as the size of the buffer, when this is possible.
>
> This is conceptually wrong, please don't do that. It is perfectly valid
> to use a buffer which is larger than the message being written or read.
> When exchanging multiple messages, it is actually quite common to
> declare only 2 buffers and reuse them:
>
> char reg;
> char val[2];
>
> struct i2c_msg msg[2] = {
> { .addr = addr, .flags = 0, .buf = &reg, .len = 1 },
> { .addr = addr, .flags = I2C_M_RD, .buf = val, .len = 1 },
> };
>
> reg = 0x04;
> i2c_transfer(i2c_adap, msg, 2);
> /* Do stuff with val */
>
> reg = 0x06;
> msg[1].len = 2;
> i2c_transfer(i2c_adap, msg, 2);
> /* Do stuff with val */
>
> Your conversion would read 2 bytes from register 0x04 instead of 1 in
> the example above.
>
> I am not opposed to the idea of i2c_msg initialization helper macros,
> but please don't mix that with actual code changes which could have bad
> side effects.

I at least tried to check in every case that the result of sizeof is the
same as the value that is present. But I can leave the constants as is,
if that seems better.

Thanks for the feedback.

julia

2012-10-09 12:51:36

by Julia Lawall

Subject: Re: [PATCH 3/13] drivers/media/tuners/qt1010.c: use macros for i2c_msg initialization

On Tue, 9 Oct 2012, Jean Delvare wrote:

> Hi Julia,
>
> On Mon, 8 Oct 2012 07:24:11 +0200 (CEST), Julia Lawall wrote:
> > > Sorry, I mean either:
> > >
> > > I2C_MSG_WRITE(priv->cfg->i2c_address, &reg, sizeof(reg)),
> > > I2C_MSG_READ(priv->cfg->i2c_address, val, sizeof(*val)),
> >
> > Of course. Sorry for not having seen that. I can do that.
>
> Eek, no, you can't, not in the general case at least. sizeof(*val) will
> return the size of the _first_ element of the destination buffer, which
> has nothing to do with the length of that buffer (which in turn might
> be rightfully longer than the read length for this specific message.)

I was actually only going to do it when the size was 1 and the type was
u8 *. But your other email suggests that converting to sizeof is just not
a good idea at all. So I will drop that part of the rule.

julia

2012-10-09 15:33:12

by Jean Delvare

Subject: Re: [PATCH 0/11] introduce macros for i2c_msg initialization

Hi Julia,

On Sun, 7 Oct 2012 17:38:30 +0200, Julia Lawall wrote:
> This patch set introduces some macros for describing how an i2c_msg is
> being initialized. There are three macros: I2C_MSG_READ, for a read
> message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other
> kind of message, which is expected to be very rarely used.

"Some other kind of message" is actually messages which need extra
flags. They are still read or write messages.

OK, I've read the whole series now and grepped the kernel tree so I
have a better overview. There are a lot more occurrences than what you
converted. I presume the conversions were just an example and you leave
the rest up to the relevant maintainers (e.g. me) if they are
interested?

Given the huge number of affected drivers (a quick grep suggests 230
drivers and more than 300 occurrences), we'd better think twice before
going on as it will be intrusive and hard to change afterward.

So my first question will be: what is your goal with this change? Are
you only trying to save a few lines of source code? Or do you expect to
actually fix/prevent bugs by introducing these macros? Or something
else?

I admit I am not completely convinced by the benefit at the moment. A
number of these drivers should be using i2c_smbus_*() functions instead
of i2c_transfer() for improved compatibility, or i2c_master_send/recv()
for single message transfers (383 occurrences!), so making
i2c_transfer() easier to use isn't at the top of my priority list. And
I see the extra work for the pre-processor, so we need a good reason
for doing that.

--
Jean Delvare

2012-10-09 15:43:42

by Julia Lawall

Subject: Re: [PATCH 0/11] introduce macros for i2c_msg initialization

On Tue, 9 Oct 2012, Jean Delvare wrote:

> Hi Julia,
>
> On Sun, 7 Oct 2012 17:38:30 +0200, Julia Lawall wrote:
> > This patch set introduces some macros for describing how an i2c_msg is
> > being initialized. There are three macros: I2C_MSG_READ, for a read
> > message, I2C_MSG_WRITE, for a write message, and I2C_MSG_OP, for some other
> > kind of message, which is expected to be very rarely used.
>
> "Some other kind of message" is actually messages which need extra
> flags. They are still read or write messages.

I agree. We could also have a read with extra options macro and a write
with extra options macro. That would give four macros, which is not too
much more than three.

> OK, I've read the whole series now and grepped the kernel tree so I
> have a better overview. There are a lot more occurrences than what you
> converted. I presume the conversions were just an example and you leave
> the rest up to the relevant maintainers (e.g. me) if they are
> interested?

I would be happy to do the rest, or at least to do more. I just didn't
want to do 600+ cases before knowing how others felt about the various
changes. Actually, now that we seem to have decided to make fewer changes
at once, I could probably work more quickly. So far, I have been
comparing the results after running cpp, as well as checking that the
sizeof transformation is correct, which is a bit slow.

> Given the huge number of affected drivers (a quick grep suggests 230
> drivers and more than 300 occurrences), we'd better think twice before
> going on as it will be intrusive and hard to change afterward.
>
> So my first question will be: what is your goal with this change? Are
> you only trying to save a few lines of source code? Or do you expect to
> actually fix/prevent bugs by introducing these macros? Or something
> else?

The main goal just seems to be to provide something that is more readable.

> I admit I am not completely convinced by the benefit at the moment. A
> number of these drivers should be using i2c_smbus_*() functions instead
> of i2c_transfer() for improved compatibility, or i2c_master_send/recv()
> for single message transfers (383 occurrences!), so making
> i2c_transfer() easier to use isn't at the top of my priority list. And
> I see the extra work for the pre-processor, so we need a good reason
> for doing that.

OK, if it doesn't seem like a good idea, it is no problem to drop the idea
completely. It does seem a bit nicer to have writing indicated as WRITE
rather than as 0, but that might not be a big enough benefit to justify
making changes.

julia

2012-10-09 23:33:05

by Mauro Carvalho Chehab

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

Em Mon, 8 Oct 2012 10:31:33 +0200 (CEST)
Julia Lawall <[email protected]> escreveu:

> I found only 15 uses of I2C_MSG_OP, out of 653 uses of one of the three
> macros. Since I2C_MSG_OP has the complete set of flags, I think it should
> be OK?
>
> One of the uses, in drivers/media/i2c/adv7604.c, is as follows:
>
> struct i2c_msg msg[2] = { { client->addr, 0, 1, msgbuf0 },
> { client->addr, 0 | I2C_M_RD, len, msgbuf1 }
>
> I'm not sure what was intended, but I guess the second structure is
> supposed to only do a read?

Yes, this is just typical I2C register read I2C messsage. The first line
specifies what register should be read (the content of msgbuf0), with is
a one char value, and the second line stores the registers contents at
msgbuf1.

This is exactly what I said before: this is a typical situation:

Just one macro could be used for that, with 4 parameters:

I2C_MSG_READ_SUBADDR(addr, sub_addr, len, buf);

Almost all of those I2C messages will fall on 3 cases only:
- a read msg (3 parameters, 1 msg);
- a write message (3 parameters, 1 msg);
- a write subaddr followed by a read (4 parameters, 2 msgs).

You'll find very few exceptions to it, where additional I2C flags are
needed, or several different transactions were grouped together, due
to the I2C locking or in order to use I2C repeat-start mode[1].

In a matter of fact, as the maintainer, I prefer to fully see the entire
I2C message for those exceptions, as those other usages require more care
while reviewing/merging.


[1] very, very few media i2c bus drivers implement any other flags except
for I2C_M_RD. That's why it is so rare to see them there.

>
> julia
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html




Cheers,
Mauro

2012-10-11 06:45:49

by Julia Lawall

Subject: Re: [PATCH 13/13] drivers/media/tuners/e4000.c: use macros for i2c_msg initialization

I found 6 cases where there are more than 2 messages in the array. I
didn't check how many cases where there are two messages but there is
something other than one read and one write.

Perhaps a reasonable option would be to use

I2C_MSG_READ
I2C_MSG_WRITE
I2C_MSG_READ_OP
I2C_MSG_WRITE_OP

The last two are for the few cases where more flags are specified. As
compared to the original proposal of I2C_MSG_OP, these keep the READ or
WRITE idea in the macro name. The additional argument to the OP macros
would be or'd with the read or write (nothing to do in this case) flags as
appropriate.

Mauro proposed INIT_I2C_READ_SUBADDR for the very common case where a
message array has one read and one write. I think that putting one
I2C_MSG_READ and one I2C_MSG_WRITE in this case is readable enough, and
avoids the need to do something special for the cases that don't match the
expectations of INIT_I2C_READ_SUBADDR.

I propose not to do anything for the moment either for sizes or for
message or buffer arrays that contain only one element.

julia

2012-10-22 09:18:35

by Julia Lawall

Subject: Re: [PATCH 0/11] introduce macros for i2c_msg initialization

I have been looking at this again, with the macros

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

#define I2C_MSG_WRITE(addr, buf, len) \
I2C_MSG_OP(addr, buf, len, 0)
#define I2C_MSG_READ(addr, buf, len) \
I2C_MSG_OP(addr, buf, len, I2C_M_RD)
#define I2C_MSG_WRITE_OP(addr, buf, len, op) \
I2C_MSG_OP(addr, buf, len, 0 | op)
#define I2C_MSG_READ_OP(addr, buf, len, op) \
I2C_MSG_OP(addr, buf, len, I2C_M_RD | op)

and the tuners files as a random first example. This time I haven't made
any adjustments for arrays of size 1.

The following file is a bit unfortunate, because a single structure is
mostly used for writing, but sometimes used for reading, in the function
tda827xa_set_params. That is, there are explicit assignments to
msg.flags.

drivers/media/tuners/tda827x.c

Currently, this is done in 8 files across the entire kernel. Would it be
a problem to use a READ or WRITE macro to initialize such a structure,
give that the type is going to change? Maybe the I2C_MSG_OP macro could
be used, with an explicit flag argument?

julia