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_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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
