Message ID | 1349624323-15584-4-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Julia, On Sun, 7 Oct 2012 17:38:33 +0200, Julia Lawall wrote: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > 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 = ®, .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,
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 <Julia.Lawall@lip6.fr> > > > > 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 = ®, .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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {