On Tue, 9 Oct 2012, Jean Delvare wrote:

Hi Julia,

On Sun,  7 Oct 2012 17:38:30 +0200, Julia Lawall wrote:

quoted

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