[PATCH 0/11] introduce macros for i2c_msg initialization

[PATCH 0/11] introduce macros for i2c_msg initialization

[Julia Lawall]

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>

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

[Julia Lawall]

From: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>

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 <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>

---
 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

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

[Jean Delvare]

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

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

[Julia Lawall]

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

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

[Julia Lawall]

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

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

[Julia Lawall]

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

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

[Jean Delvare]

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.

-- 
Jean Delvare

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

[Julia Lawall]

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

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 772 bytes --]

> > 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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]