Re: [RFC 1/2] dvb-core: add generic helper function for I2C register

[Antti Palosaari <crope@iki.fi>]

Hello,
I compared all I2C-client drivers I have done and here are the results:
name = name of driver module
reg = reg addr len (bytes)
val = reg val len (bytes)
auto = auto increment
other = register banks, etc.

name       reg  val auto   other
qt1010       1    1    ?
af9013       2    1    Y
ec100        1    1    ?
tda18218     1    1    Y
tua9001      1    2    ?
tda18212     1    1    Y
cxd2820r     1    1    ?   bank
tda10071     1    1    Y
a8293        not relevant, only one control byte
rtl2830      1    1    Y   bank
rtl2832      1    1    Y   bank
af9033       3*   1    Y   *bank/mailbox
<noname>     2    1    Y

As we can see I2C msg structure where is address first ans after that 
payload is quite de Facto. There was only one driver which didn't meet 
that condition, it is LNB-controller which uses only one byte.

tda10071 driver has most typical register read and write routines and 
size of those are 70 LOC, including rd_reg, rd_regs, wr_reg, wr_regs, 
excluding bit based register functions.
12 drivers, ca. 70 LOC per driver makes 840 LOC of less code. And you 
can save even more if generalize bit register access functions too 
(commonly: wr_reg_mask, rd_reg_mask, wr_reg_bits, rd_reg_bits).

More comments below.

On 11/09/2011 12:37 PM, Jean Delvare wrote:
> If code is duplicated, then something should indeed be done about it.
> But preferably after analyzing properly what the helper functions
> should look like, and for this you'll have to look at "all" drivers
> that could benefit from it. At the moment only the tda18218 driver was
> reported to need it, that's not enough to generalize.
>
> You should take a look at drivers/misc/eeprom/at24.c, it contains
> fairly complete transfer functions which cover the various EEPROM
> types. Non-EEPROM devices could behave differently, but this would
> still seem to be a good start for any I2C device using block transfers.
> It was once proposed that these functions could make their way into
> i2c-core or a generic i2c helper function.
>
> Both at24 and Antti's proposal share the idea of storing information
> about the device capabilities (max block read and write lengths, but we
> could also put there alignment requirements or support for repeated
> start condition.) in a private structure. If we generalize the
> functions then this information would have to be stored in struct
> i2c_client and possibly struct i2c_adapter (or struct i2c_algorithm) so
> that the function can automatically find out the right sequence of
> commands for the adapter/slave combination.
>
> Speaking of struct i2c_client, I seem to remember that the dvb
> subsystem doesn't use it much at the moment. This might be an issue if
> you intend to get the generic code into i2c-core, as most helper
> functions rely on a valid i2c_client structure by design.

As we have now some kind of understanding what is needed, could you 
start direct planning? I am ready to implement some basic stuff that I 
see most benefit (listed below).

The functions I see most important are:
wr_regs
rd_regs
wr_reg
rg_reg
wr_reg_mask
wr_reg_bits
rd_reg_bits


regards
Antti
-- 
http://palosaari.fi/