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
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
