dvb-core: add generic helper function for I2C register
RFC 1/2 dvb-core: add generic helper function for I2C register
Function that splits and sends most typical I2C register write.
--- drivers/media/dvb/dvb-core/Makefile | 2 +- drivers/media/dvb/dvb-core/dvb_generic.c | 48 ++++++++++++++++++++++++++++++ drivers/media/dvb/dvb-core/dvb_generic.h | 21 +++++++++++++ 3 files changed, 70 insertions(+), 1 deletions(-) create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.c create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.h
drivers/media/dvb/dvb-core/Makefile +1 -1
@@ -6,6 +6,6 @@ dvb-net-$(CONFIG_DVB_NET) := dvb_net.o dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - $(dvb-net-y) dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o dvb_generic.o obj-$(CONFIG_DVB_CORE) += dvb-core.o
drivers/media/dvb/dvb-core/dvb_generic.c new file +48 -0
48 lines not on this page · raw message
drivers/media/dvb/dvb-core/dvb_generic.h new file +21 -0
@@ -0,0 +1,21 @@ +#ifndef DVB_GENERIC_H +#define DVB_GENERIC_H + +#define DVB_GENERIC_LOG_PREFIX "dvb_generic" +#define warn(f, arg...) \ + printk(KERN_WARNING DVB_GENERIC_LOG_PREFIX": " f "\n", ## arg) + +struct dvb_i2c_cfg { +struct i2c_adapter *adapter; +u8 addr; +/* TODO: reg_addr_len; as now use one byte */ +/* TODO: reg_val_len; as now use one byte */ +u8 max_wr; +u8 max_rd; +}; + +extern int dvb_wr_regs (struct dvb_i2c_cfg *,u8 ,u8 *,int ); +extern int dvb_wr_reg (struct dvb_i2c_cfg *,u8 ,u8 ); + +#endif +
Signature · 1 line
1.7.4.4
Quoted 110 lines · Antti Palosaari
Function that splits and sends most typical I2C register write.
Signed-off-by Antti Palosaari <[email protected]>---
drivers/media/dvb/dvb-core/Makefile | 2 +-drivers/media/dvb/dvb-core/dvb_generic.c| 48
++++++++++++++++++++++++++++++
drivers/media/dvb/dvb-core/dvb_generic.h | 21 +++++++++++++3 files changed, 70 insertions(+), 1 deletions(-)create mode 100644
drivers/media/dvb/dvb-core/dvb_generic.ccreate mode 100644drivers/media/dvb/dvb-core/dvb_generic.hdrivers/media/dvb/dvb-core/Makefile +1
-1@@ -6,6 +6,6 @@ dvb-net-$(CONFIG_DVB_NET) := dvb_net.o dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - $(dvb-net-y) dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o dvb_generic.oobj-$(CONFIG_DVB_CORE) += dvb-core.o
drivers/media/dvb/dvb-core/dvb_generic.c new file +27
-0@@ -0,0 +1,48 @@ +#include <linux/i2c.h> +#include "dvb_generic.h" + +/* write multiple registers */ +int dvb_wr_regs (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 *val,int len_tot) +{ +#define REG_ADDR_LEN 1 +#define REG_VAL_LEN 1 + int ret, len_cur, len_rem, len_max; + u8 buf[i2c_cfg->max_wr]; + struct i2c_msg msg[1] = { + { + .addr = i2c_cfg->addr, + .flags = 0, + .buf = buf, + } + }; + + len_max = i2c_cfg->max_wr - REG_ADDR_LEN; + for (len_rem = len_tot; len_rem > 0; len_rem -= len_max) { + len_cur = len_rem; + if (len_cur > len_max) + len_cur = len_max; + + msg[0].len = len_cur + REG_ADDR_LEN; + buf[0] = reg; + memcpy(&buf[REG_ADDR_LEN], &val[len_tot - len_rem],
len_cur);drivers/media/dvb/dvb-core/dvb_generic.c new file +21
-0+ + ret = i2c_transfer(i2c_cfg->adapter, msg, 1); + if (ret != 1) { + warn("i2c wr failed=%d reg=%02x len=%d", + ret, reg, len_cur); + return -EREMOTEIO; + } + reg += len_cur; + } + + return 0; +} +EXPORT_SYMBOL (dvb_wr_regs); + +/* write single register */ +int dvb_wr_reg (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 val) +{ + return dvb_wr_regs(i2c_cfg, reg, &val, 1); +} +EXPORT_SYMBOL (dvb_wr_reg); +drivers/media/dvb/dvb-core/dvb_generic.h new file +21
-1@@ -0,0 +1,21 @@ +#ifndef DVB_GENERIC_H +#define DVB_GENERIC_H + +#define DVB_GENERIC_LOG_PREFIX "dvb_generic" +#define warn(f, arg...) \ + printk(KERN_WARNING DVB_GENERIC_LOG_PREFIX": " f "\n", ## arg) + +struct dvb_i2c_cfg { + struct i2c_adapter *adapter; + u8 addr; + /* TODO: reg_addr_len; as now use one byte */ + /* TODO: reg_val_len; as now use one byte */ + u8 max_wr; + u8 max_rd; +}; + +extern int dvb_wr_regs (struct dvb_i2c_cfg *,u8 ,u8 *,int ); +extern int dvb_wr_reg (struct dvb_i2c_cfg *,u8 ,u8 ); + +#endif + --
Quoted 2 lines
1.7.4.4
Without reviewing the actual code here, there actually isn't any reason for this to be a part of dvb-core. It can be its own module.
Meanwhile, if there is need for this, don't you think it should be implemented in a generic way for use by other, NON-dvb drivers?
-Mike
Em 08-11-2011 21:54, Antti Palosaari escreveu:
Quoted 53 lines
Function that splits and sends most typical I2C register write.
Signed-off-by Antti Palosaari <[email protected]>--- drivers/media/dvb/dvb-core/Makefile | 2 +- drivers/media/dvb/dvb-core/dvb_generic.c | 48 ++++++++++++++++++++++++++++++ drivers/media/dvb/dvb-core/dvb_generic.h | 21 +++++++++++++ 3 files changed, 70 insertions(+), 1 deletions(-) create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.c create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.hdrivers/media/dvb/dvb-core/Makefile +1
-1@@ -6,6 +6,6 @@ dvb-net-$(CONFIG_DVB_NET) := dvb_net.o dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - $(dvb-net-y) dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o dvb_generic.o obj-$(CONFIG_DVB_CORE) += dvb-core.odrivers/media/dvb/dvb-core/dvb_generic.c new file +24
-0@@ -0,0 +1,48 @@ +#include <linux/i2c.h> +#include "dvb_generic.h" + +/* write multiple registers */ +int dvb_wr_regs (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 *val,int len_tot) +{ +#define REG_ADDR_LEN 1 +#define REG_VAL_LEN 1 +int ret, len_cur, len_rem, len_max; +u8 buf[i2c_cfg->max_wr]; +struct i2c_msg msg[1 ] = { + { + .addr = i2c_cfg->addr, + .flags =0 , + .buf = buf, + } + }; + + len_max = i2c_cfg->max_wr -REG_ADDR_LEN ; +for (len_rem = len_tot; len_rem >0 ; len_rem -= len_max) { + len_cur = len_rem; +if (len_cur > len_max) + len_cur = len_max; +
drivers/media/dvb/dvb-core/dvb_generic.c line 34
Quoted 10 lines
drivers/media/dvb/dvb-core/dvb_generic.c new file +10
-0+ msg[0 ].len = len_cur +REG_ADDR_LEN ; + buf[0 ] = reg; +memcpy (&buf[REG_ADDR_LEN ], &val[len_tot - len_rem], len_cur); + + ret =i2c_transfer (i2c_cfg->adapter, msg,1 ); +if (ret !=1 ) { +warn ("i2c wr failed=%d reg=%02x len=%d" , + ret, reg, len_cur); +return -EREMOTEIO ; + }
Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = {
{
.addr = i2c_cfg->addr,
.flags = 0,
.buf = buf,
},
{
.addr = i2c_cfg->addr,
.flags = 0,
.buf = buf2,
}
};
ret = i2c_transfer(i2c_cfg->adapter, msg, 2);Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
I like the idea of having some functions to help handling those cases where a single transaction needs to be split into several messages.
Yet, I agree with Michael: I would add such logic inside the I2C subsystem, and being sure that the lock is kept during the entire I2C operation.
Jean,
Thoughts?
Quoted 41 lines
drivers/media/dvb/dvb-core/dvb_generic.c new file +14
-0+ reg += len_cur; + } + +return 0 ; +} +EXPORT_SYMBOL (dvb_wr_regs); + +/* write single register */ +int dvb_wr_reg (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 val) +{ +return dvb_wr_regs (i2c_cfg, reg, &val,1 ); +} +EXPORT_SYMBOL (dvb_wr_reg); +drivers/media/dvb/dvb-core/dvb_generic.h new file +21
-0@@ -0,0 +1,21 @@ +#ifndef DVB_GENERIC_H +#define DVB_GENERIC_H + +#define DVB_GENERIC_LOG_PREFIX "dvb_generic" +#define warn(f, arg...) \ + printk(KERN_WARNING DVB_GENERIC_LOG_PREFIX": " f "\n", ## arg) + +struct dvb_i2c_cfg { +struct i2c_adapter *adapter; +u8 addr; +/* TODO: reg_addr_len; as now use one byte */ +/* TODO: reg_val_len; as now use one byte */ +u8 max_wr; +u8 max_rd; +}; + +extern int dvb_wr_regs (struct dvb_i2c_cfg *,u8 ,u8 *,int ); +extern int dvb_wr_reg (struct dvb_i2c_cfg *,u8 ,u8 ); + +#endif +
Quoted 83 lines · Mauro Carvalho Chehab
Em 08-11-2011 21:54, Antti Palosaari escreveu:
Quoted 53 lines
Function that splits and sends most typical I2C register write.
Signed-off-by Antti Palosaari <[email protected]>--- drivers/media/dvb/dvb-core/Makefile | 2 +- drivers/media/dvb/dvb-core/dvb_generic.c | 48 ++++++++++++++++++++++++++++++ drivers/media/dvb/dvb-core/dvb_generic.h | 21 +++++++++++++ 3 files changed, 70 insertions(+), 1 deletions(-) create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.c create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.hdrivers/media/dvb/dvb-core/Makefile +1
-1@@ -6,6 +6,6 @@ dvb-net-$(CONFIG_DVB_NET) := dvb_net.o dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - $(dvb-net-y) dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o dvb_generic.o obj-$(CONFIG_DVB_CORE) += dvb-core.odrivers/media/dvb/dvb-core/dvb_generic.c new file +24
-0@@ -0,0 +1,48 @@ +#include <linux/i2c.h> +#include "dvb_generic.h" + +/* write multiple registers */ +int dvb_wr_regs (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 *val,int len_tot) +{ +#define REG_ADDR_LEN 1 +#define REG_VAL_LEN 1 +int ret, len_cur, len_rem, len_max; +u8 buf[i2c_cfg->max_wr]; +struct i2c_msg msg[1 ] = { + { + .addr = i2c_cfg->addr, + .flags =0 , + .buf = buf, + } + }; + + len_max = i2c_cfg->max_wr -REG_ADDR_LEN ; +for (len_rem = len_tot; len_rem >0 ; len_rem -= len_max) { + len_cur = len_rem; +if (len_cur > len_max) + len_cur = len_max; +drivers/media/dvb/dvb-core/dvb_generic.c line 34
Quoted 10 lines
drivers/media/dvb/dvb-core/dvb_generic.c new file +10
-0+ msg[0 ].len = len_cur +REG_ADDR_LEN ; + buf[0 ] = reg; +memcpy (&buf[REG_ADDR_LEN ], &val[len_tot - len_rem], len_cur); + + ret =i2c_transfer (i2c_cfg->adapter, msg,1 ); +if (ret !=1 ) { +warn ("i2c wr failed=%d reg=%02x len=%d" , + ret, reg, len_cur); +return -EREMOTEIO ; + }Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = { { .addr = i2c_cfg->addr, .flags = 0, .buf = buf, }, { .addr = i2c_cfg->addr, .flags = 0, .buf = buf2, } }; ret = i2c_transfer(i2c_cfg->adapter, msg, 2);
Quoted 2 lines
Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
This is correct, but this isn't the only difference. The second difference is that, with the code above, a repeated-start condition is used between both messages, instead of a stop condition followed by a start condition. While ideally all controllers, all controller drivers and all slaves would support that, I don't think this is true in practice.
Also note that preventing others from accessing the bus during the transaction might be desirable sometimes, but this isn't always the case. A large data transfer over I2C can take a significant amount of time, during which smaller signaling I2C transfers would be blocked. Sometimes latency is important too. I think it would be wrong to hard-code the latency vs. throughput/continuity decision in the helper functions.
Quoted 8 lines
I like the idea of having some functions to help handling those cases where a single transaction needs to be split into several messages.
Yet, I agree with Michael: I would add such logic inside the I2C subsystem, and being sure that the lock is kept during the entire I2C operation.
Jean,
Thoughts?
I agree that it makes some sense. We recently added helper functions for swapped word reads, to avoid code duplication amongst device drivers. This would follow a similar logic.
However you should bear in mind that different I2C devices have different expectations and requirements. Some do automatic register address increment, some don't. Some support arbitrary read/write length and alignment, some don't. It is common that write constraints differ from read constraints. So you won't possibly come up with universal I2C read and write functions. There is a reason why it was originally decided to only provide the low-level transfer functions in i2c-core and leave the rest up to individual device drivers.
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.
Signature · 1 line
Jean Delvare
Quoted 6 lines · Jean Delvare
Quoted 4 lines · Mauro Carvalho Chehab
ret = i2c_transfer(i2c_cfg->adapter, msg, 2);Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
Quoted 6 lines
This is correct, but this isn't the only difference. The second difference is that, with the code above, a repeated-start condition is used between both messages, instead of a stop condition followed by a start condition. While ideally all controllers, all controller drivers and all slaves would support that, I don't think this is true in practice.
I agree, as we just replied same time to that message :)
Quoted 4 lines
I agree that it makes some sense. We recently added helper functions for swapped word reads, to avoid code duplication amongst device drivers. This would follow a similar logic.
Quoted 8 lines
However you should bear in mind that different I2C devices have different expectations and requirements. Some do automatic register address increment, some don't. Some support arbitrary read/write length and alignment, some don't. It is common that write constraints differ from read constraints. So you won't possibly come up with universal I2C read and write functions. There is a reason why it was originally decided to only provide the low-level transfer functions in i2c-core and leave the rest up to individual device drivers.
So true. Some chips allow even configure auto increment or not and even more.
But I take that most common behaviour way. It is never possible to support all I2C access formats chip vendors will define. But that first reg then values with auto increment is very common, maybe over 90% cases.
Quoted 5 lines
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.
Only?, I think you missed part of my first mail I mentioned 7 cases from *my* drivers where splitting is needed. Actually, I just remember one more, it is AF9015 & AF9033. And those are for the splitting only, same function can be used maybe 90% of current tuner and demod drivers we have.
Quoted 16 lines
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_clientand possibly structi2c_adapter(or structi2c_algorithm) so that the function can automatically find out the right sequence of commands for the adapter/slave combination.
Quoted 4 lines
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 validi2c_clientstructure by design.
I will check those later today, have to go back lecture.
regards
Antti
Signature · 1 line
http://palosaari.fi/
Em 09-11-2011 08:37, Jean Delvare escreveu:
Quoted 87 lines · Mauro Carvalho Chehab
Quoted 83 lines
Em 08-11-2011 21:54, Antti Palosaari escreveu:
Quoted 53 lines
Function that splits and sends most typical I2C register write.
Signed-off-by Antti Palosaari <[email protected]>--- drivers/media/dvb/dvb-core/Makefile | 2 +- drivers/media/dvb/dvb-core/dvb_generic.c | 48 ++++++++++++++++++++++++++++++ drivers/media/dvb/dvb-core/dvb_generic.h | 21 +++++++++++++ 3 files changed, 70 insertions(+), 1 deletions(-) create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.c create mode 100644 drivers/media/dvb/dvb-core/dvb_generic.hdrivers/media/dvb/dvb-core/Makefile +1
-1@@ -6,6 +6,6 @@ dvb-net-$(CONFIG_DVB_NET) := dvb_net.o dvb-core-objs := dvbdev.o dmxdev.o dvb_demux.o dvb_filter.o \ dvb_ca_en50221.o dvb_frontend.o \ - $(dvb-net-y) dvb_ringbuffer.o dvb_math.o + $(dvb-net-y) dvb_ringbuffer.o dvb_math.o dvb_generic.o obj-$(CONFIG_DVB_CORE) += dvb-core.odrivers/media/dvb/dvb-core/dvb_generic.c new file +24
-0@@ -0,0 +1,48 @@ +#include <linux/i2c.h> +#include "dvb_generic.h" + +/* write multiple registers */ +int dvb_wr_regs (struct dvb_i2c_cfg *i2c_cfg,u8 reg,u8 *val,int len_tot) +{ +#define REG_ADDR_LEN 1 +#define REG_VAL_LEN 1 +int ret, len_cur, len_rem, len_max; +u8 buf[i2c_cfg->max_wr]; +struct i2c_msg msg[1 ] = { + { + .addr = i2c_cfg->addr, + .flags =0 , + .buf = buf, + } + }; + + len_max = i2c_cfg->max_wr -REG_ADDR_LEN ; +for (len_rem = len_tot; len_rem >0 ; len_rem -= len_max) { + len_cur = len_rem; +if (len_cur > len_max) + len_cur = len_max; +drivers/media/dvb/dvb-core/dvb_generic.c line 34
Quoted 10 lines
drivers/media/dvb/dvb-core/dvb_generic.c new file +10
-0+ msg[0 ].len = len_cur +REG_ADDR_LEN ; + buf[0 ] = reg; +memcpy (&buf[REG_ADDR_LEN ], &val[len_tot - len_rem], len_cur); + + ret =i2c_transfer (i2c_cfg->adapter, msg,1 ); +if (ret !=1 ) { +warn ("i2c wr failed=%d reg=%02x len=%d" , + ret, reg, len_cur); +return -EREMOTEIO ; + }Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = { { .addr = i2c_cfg->addr, .flags = 0, .buf = buf, }, { .addr = i2c_cfg->addr, .flags = 0, .buf = buf2, } }; ret = i2c_transfer(i2c_cfg->adapter, msg, 2);Quoted 2 lines
Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
Quoted 6 lines
This is correct, but this isn't the only difference. The second difference is that, with the code above, a repeated-start condition is used between both messages, instead of a stop condition followed by a start condition. While ideally all controllers, all controller drivers and all slaves would support that, I don't think this is true in practice.
True. On most cases, repeated-start works fine on media devices, but I bet there are some chips that don't support it on a few boards.
Quoted 7 lines
Also note that preventing others from accessing the bus during the transaction might be desirable sometimes, but this isn't always the case. A large data transfer over I2C can take a significant amount of time, during which smaller signaling I2C transfers would be blocked. Sometimes latency is important too. I think it would be wrong to hard-code the latency vs. throughput/continuity decision in the helper functions.
What happens in practice, at least on media devices, is that, when a large I2C transaction is broken, and some other transaction takes the bus, some sort of corruption happens. We've seen such broken behavior on several drivers, during firmware uploads, during eeprom access, during tuner initialization, and even when reading a single register value at a sub-address specified by a previous write.
So, at least on media, this is not a matter of latency vs throughput, but a matter of work/not work.
When there's no need to join I2C transactions, the better is to let the driver do several calls to the I2C xfer functions. All media drivers I know do that: they only use long I2C transfers when it is a hardware requirement for doing that.
Quoted 22 lines
Quoted 8 lines
I like the idea of having some functions to help handling those cases where a single transaction needs to be split into several messages.
Yet, I agree with Michael: I would add such logic inside the I2C subsystem, and being sure that the lock is kept during the entire I2C operation.
Jean,
Thoughts?I agree that it makes some sense. We recently added helper functions for swapped word reads, to avoid code duplication amongst device drivers. This would follow a similar logic.
However you should bear in mind that different I2C devices have different expectations and requirements. Some do automatic register address increment, some don't. Some support arbitrary read/write length and alignment, some don't. It is common that write constraints differ from read constraints. So you won't possibly come up with universal I2C read and write functions. There is a reason why it was originally decided to only provide the low-level transfer functions in i2c-core and leave the rest up to individual device drivers.
Quoted 5 lines
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.
This is a need for several drivers. There are two major types of devices here:
1) devices that need a write + read in order to get some data
from the a register sub-address;
2) devices that need to receive a large I2C block at the same
time.
I dunno tda18218, but tda18271 is one example of the latter case: The datasheet explicitly requires to fill a block of 18 registers at the same time, in order to initialize the device. A fail on fulfilling this requirement causes sub-optimal tuning, or even failures, so, drivers like cx231xx (that allows only 5 bytes for I2C payloads) need to split such transaction into multiple I2C messages.
Quoted 16 lines
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_clientand possibly structi2c_adapter(or structi2c_algorithm) so that the function can automatically find out the right sequence of commands for the adapter/slave combination.
Quoted 5 lines
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 validi2c_clientstructure by design.
Yes, DVB uses the low level I2C ops. I don't see any reason why not changing it to use struct i2c_client (well, except that such change would require lots of changes and tests).
Regards,
Mauro
Quoted 9 lines · Mauro Carvalho Chehab
Em 09-11-2011 08:37, Jean Delvare escreveu:
Quoted 4 lines
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 validi2c_clientstructure by design.Yes, DVB uses the low level I2C ops. I don't see any reason why not changing it to use struct
i2c_client(well, except that such change would require lots of changes and tests).
The "lots of changes and tests" part is indeed the problem. Furthermore I clearly remember discussing this with Michael a couple years ago (during the i2c device driver model rework) and telling him I would never force DVB to make use of i2c_client if they did not want to. I gave my word, taking it back now would be unfair. Well at least the new i2c model would make it possible, this wasn't the case before, but this is such a huge amount of work that I certainly don't want to push this on anyone.
Signature · 1 line
Jean Delvare
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.
Quoted 22 lines · Jean Delvare
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_clientand possibly structi2c_adapter(or structi2c_algorithm) so that the function can automatically find out the right sequence of commands for the adapter/slave combination.
Quoted 4 lines
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 validi2c_clientstructure 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_regsrd_regswr_regrg_regwr_reg_maskwr_reg_bitsrd_reg_bits
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 18 lines · Mauro Carvalho Chehab
Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = { { .addr = i2c_cfg->addr, .flags = 0, .buf = buf, }, { .addr = i2c_cfg->addr, .flags = 0, .buf = buf2, } }; ret = i2c_transfer(i2c_cfg->adapter, msg, 2);
Quoted 2 lines
Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
In my understanding adding more messages than one means those should be handled as one I2C transaction using REPEATED START.
I see one big problem here, it is our adapters. I think again, for the experience I have, most of our I2C-adapters can do only 3 different types of I2C xfers;
* I2C write
* I2C write + I2C read (combined with REPEATED START)
* I2C read (I suspect many adapters does not support that)
That means, I2C REPEATED writes are not possible.
Quoted 8 lines
I like the idea of having some functions to help handling those cases where a single transaction needs to be split into several messages.
Yet, I agree with Michael: I would add such logic inside the I2C subsystem, and being sure that the lock is kept during the entire I2C operation.
Jean,
Thoughts?
regards
Antti
Signature · 1 line
http://palosaari.fi/
Quoted 22 lines · Antti Palosaari
Quoted 18 lines · Mauro Carvalho Chehab
Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = { { .addr = i2c_cfg->addr, .flags = 0, .buf = buf, }, { .addr = i2c_cfg->addr, .flags = 0, .buf = buf2, } }; ret = i2c_transfer(i2c_cfg->adapter, msg, 2);Quoted 2 lines
Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
Quoted 9 lines
In my understanding adding more messages than one means those should be handled as one I2C transaction using REPEATED START.
I see one big problem here, it is our adapters. I think again, for the experience I have, most of our I2C-adapters can do only 3 different types of I2C xfers;
* I2C write
* I2C write + I2C read (combined with REPEATED START)
* I2C read (I suspect many adapters does not support that) That means, I2C REPEATED writes are not possible.
Also, some adapters _or slaves_ won't support more than one repeated start in a given transaction, so splitting block reads in continuous chunks won't always work either. Which makes some sense if you think about it: if both the slave and the controller supported larger blocks then there would be no need to split the transfer into multiple messages in the first place.
Signature · 1 line
Jean Delvare
Quoted 33 lines · Jean Delvare
Quoted 22 lines · Antti Palosaari
Quoted 18 lines · Mauro Carvalho Chehab
Due to the way I2C locks are bound, doing something like the above and something like:
struct i2c_msg msg[2] = { { .addr = i2c_cfg->addr, .flags = 0, .buf = buf, }, { .addr = i2c_cfg->addr, .flags = 0, .buf = buf2, } }; ret = i2c_transfer(i2c_cfg->adapter, msg, 2);Quoted 2 lines
Produces a different result. In the latter case, I2C core avoids having any other transaction in the middle of the 2 messages.
Quoted 9 lines
In my understanding adding more messages than one means those should be handled as one I2C transaction using REPEATED START.
I see one big problem here, it is our adapters. I think again, for the experience I have, most of our I2C-adapters can do only 3 different types of I2C xfers;
* I2C write
* I2C write + I2C read (combined with REPEATED START)
* I2C read (I suspect many adapters does not support that) That means, I2C REPEATED writes are not possible.
Quoted 7 lines
Also, some adapters
_orslaves_won't support more than one repeated start in a given transaction, so splitting block reads in continuous chunks won't always work either. Which makes some sense if you think about it: if both the slave and the controller supported larger blocks then there would be no need to split the transfer into multiple messages in the first place.
Yes, I can immediately think of the stv0288 which can receive up to 108 bytes continuous write of its register map, but using the lme2510c controller won't write more than 16, probably beyond the limit of the firmwares buffer.
I think mostly, you are at the mercy of the controller firmware and not really the host i2c controller abilities.
Regards
Malcolm
Hi Antti,
As an additional note, it just occurred to me that what you are working on is somewhat related to Mark Brown's regmap. Look in drivers/base/regmap and see if maybe you can reuse and/or extend Mark's approach.
Signature · 1 line
Jean Delvare