| Message ID | 1349624323-15584-3-git-send-email-Julia.Lawall@lip6.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Am 07.10.2012 17:38, schrieb Julia Lawall: > From: Julia Lawall <Julia.Lawall@lip6.fr> > > 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 <Julia.Lawall@lip6.fr> > > --- > 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 = ®, > - }, { > - .addr = priv->cfg->i2c_addr, > - .flags = I2C_M_RD, > - .len = sizeof(buf), > - .buf = buf, > - } > + I2C_MSG_WRITE(priv->cfg->i2c_addr, ®, 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- 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
On Sun, 7 Oct 2012, walter harms wrote: > > > Am 07.10.2012 17:38, schrieb Julia Lawall: >> From: Julia Lawall <Julia.Lawall@lip6.fr> >> >> 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 <Julia.Lawall@lip6.fr> >> >> --- >> 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 = ®, >> - }, { >> - .addr = priv->cfg->i2c_addr, >> - .flags = I2C_M_RD, >> - .len = sizeof(buf), >> - .buf = buf, >> - } >> + I2C_MSG_WRITE(priv->cfg->i2c_addr, ®, 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 majordomo@vger.kernel.org >> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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
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 <Julia.Lawall@lip6.fr> >>> >>> 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 <Julia.Lawall@lip6.fr> >>> >>> --- >>> 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 = ®, >>> - }, { >>> - .addr = priv->cfg->i2c_addr, >>> - .flags = I2C_M_RD, >>> - .len = sizeof(buf), >>> - .buf = buf, >>> - } >>> + I2C_MSG_WRITE(priv->cfg->i2c_addr, ®, 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 majordomo@vger.kernel.org >>> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- 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
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 <Julia.Lawall@lip6.fr> >>>> >>>> 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 <Julia.Lawall@lip6.fr> >>>> >>>> --- >>>> 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 -- 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
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. -- 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
>> 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 -- 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
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. -- 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
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 -- 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
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 -- 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
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 -- 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
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 <Julia.Lawall@lip6.fr> >>> >>> 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 <Julia.Lawall@lip6.fr> >>> >>> --- >>> 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 -- 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
Em Sun, 07 Oct 2012 14:51:58 -0700 Joe Perches <joe@perches.com> 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 -- 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
On 08/10/12 12:56, Mauro Carvalho Chehab wrote: > Em Sun, 07 Oct 2012 14:51:58 -0700 > Joe Perches <joe@perches.com> 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, ®, 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 -- 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
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 -- 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
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 <joe@perches.com> 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, ®, 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
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
--
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
Em Mon, 8 Oct 2012 10:31:33 +0200 (CEST) Julia Lawall <julia.lawall@lip6.fr> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html Cheers, Mauro -- 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
Hi Julia, Em Mon, 8 Oct 2012 10:31:33 +0200 (CEST) Julia Lawall <julia.lawall@lip6.fr> 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? As we're discussing the macro names, etc, I'll just mark this patch series as RFC, at patchwork, removing it from my pending queue. Feel free to re-submit it later, after some agreement got reached. Thanks, Mauro -- 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
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 -- 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
Hi Julia, On Thu, 11 Oct 2012 08:45:43 +0200 (CEST), Julia Lawall wrote: > 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. Please note that I resigned from my position of i2c subsystem maintainer, so I will not handle this. If you think this is important, you'll have to resubmit and Wolfram will decide what he wants to do about it.
On Tue, 18 Dec 2012, Jean Delvare wrote: > Hi Julia, > > On Thu, 11 Oct 2012 08:45:43 +0200 (CEST), Julia Lawall wrote: > > 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. > > Please note that I resigned from my position of i2c subsystem > maintainer, so I will not handle this. If you think this is important, > you'll have to resubmit and Wolfram will decide what he wants to do > about it. OK, I had the impression that the conclusion was that the danger was greater than the benefit. If there is interest in it, since I think it does make the code more readable, I can pick it up again. 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
> > Please note that I resigned from my position of i2c subsystem > > maintainer, so I will not handle this. If you think this is important, > > you'll have to resubmit and Wolfram will decide what he wants to do > > about it. > > OK, I had the impression that the conclusion was that the danger was > greater than the benefit. If there is interest in it, since I think it > does make the code more readable, I can pick it up again. TBH, there are other things coming which have higher priority, so from my side, currently, there is not much interest right now. Thanks nonetheless, Wolfram
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 = ®, - }, { - .addr = priv->cfg->i2c_addr, - .flags = I2C_M_RD, - .len = sizeof(buf), - .buf = buf, - } + I2C_MSG_WRITE(priv->cfg->i2c_addr, ®, sizeof(reg)), + I2C_MSG_READ(priv->cfg->i2c_addr, buf, sizeof(buf)) }; ret = i2c_transfer(priv->i2c, msg, 2);