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@xxxxxxxxxxx> 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_MSGREAD/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 -- http://palosaari.fi/ -- To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
