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

[Mauro Carvalho Chehab]

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


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.


Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/