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

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

[Antti Palosaari]

Function that splits and sends most typical I2C register write.

Signed-off-by: Antti Palosaari <crope@iki.fi>
---
  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

diff --git a/drivers/media/dvb/dvb-core/Makefile 
b/drivers/media/dvb/dvb-core/Makefile
index 8f22bcd..230584a 100644
--- a/drivers/media/dvb/dvb-core/Makefile
+++ b/drivers/media/dvb/dvb-core/Makefile
@@ -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
diff --git a/drivers/media/dvb/dvb-core/dvb_generic.c 
b/drivers/media/dvb/dvb-core/dvb_generic.c
new file mode 100644
index 0000000..002bd24
--- /dev/null
+++ b/drivers/media/dvb/dvb-core/dvb_generic.c
@@ -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);
+
+		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);
+
diff --git a/drivers/media/dvb/dvb-core/dvb_generic.h 
b/drivers/media/dvb/dvb-core/dvb_generic.h
new file mode 100644
index 0000000..7a140ab
--- /dev/null
+++ b/drivers/media/dvb/dvb-core/dvb_generic.h
@@ -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
+
-- 
1.7.4.4

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

[Michael Krufky]

On Tue, Nov 8, 2011 at 6:54 PM, Antti Palosaari <crope@iki.fi> wrote:
> Function that splits and sends most typical I2C register write.
>
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  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
>
> diff --git a/drivers/media/dvb/dvb-core/Makefile
> b/drivers/media/dvb/dvb-core/Makefile
> index 8f22bcd..230584a 100644
> --- a/drivers/media/dvb/dvb-core/Makefile
> +++ b/drivers/media/dvb/dvb-core/Makefile
> @@ -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
> diff --git a/drivers/media/dvb/dvb-core/dvb_generic.c
> b/drivers/media/dvb/dvb-core/dvb_generic.c
> new file mode 100644
> index 0000000..002bd24
> --- /dev/null
> +++ b/drivers/media/dvb/dvb-core/dvb_generic.c
> @@ -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);
> +
> +               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);
> +
> diff --git a/drivers/media/dvb/dvb-core/dvb_generic.h
> b/drivers/media/dvb/dvb-core/dvb_generic.h
> new file mode 100644
> index 0000000..7a140ab
> --- /dev/null
> +++ b/drivers/media/dvb/dvb-core/dvb_generic.h
> @@ -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
> +
> --
> 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

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

[Mauro Carvalho Chehab]

Em 08-11-2011 21:54, Antti Palosaari escreveu:
> Function that splits and sends most typical I2C register write.
> 
> Signed-off-by: Antti Palosaari <crope@iki.fi>
> ---
>  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
> 
> diff --git a/drivers/media/dvb/dvb-core/Makefile b/drivers/media/dvb/dvb-core/Makefile
> index 8f22bcd..230584a 100644
> --- a/drivers/media/dvb/dvb-core/Makefile
> +++ b/drivers/media/dvb/dvb-core/Makefile
> @@ -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
> diff --git a/drivers/media/dvb/dvb-core/dvb_generic.c b/drivers/media/dvb/dvb-core/dvb_generic.c
> new file mode 100644
> index 0000000..002bd24
> --- /dev/null
> +++ b/drivers/media/dvb/dvb-core/dvb_generic.c
> @@ -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);
> +
> +        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?

> +        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);
> +
> diff --git a/drivers/media/dvb/dvb-core/dvb_generic.h b/drivers/media/dvb/dvb-core/dvb_generic.h
> new file mode 100644
> index 0000000..7a140ab
> --- /dev/null
> +++ b/drivers/media/dvb/dvb-core/dvb_generic.h
> @@ -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
> +

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

[Jean Delvare]

On Wed, 09 Nov 2011 07:56:13 -0200, Mauro Carvalho Chehab wrote:
> Em 08-11-2011 21:54, Antti Palosaari escreveu:
> > Function that splits and sends most typical I2C register write.
> > 
> > Signed-off-by: Antti Palosaari <crope@iki.fi>
> > ---
> >  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
> > 
> > diff --git a/drivers/media/dvb/dvb-core/Makefile b/drivers/media/dvb/dvb-core/Makefile
> > index 8f22bcd..230584a 100644
> > --- a/drivers/media/dvb/dvb-core/Makefile
> > +++ b/drivers/media/dvb/dvb-core/Makefile
> > @@ -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
> > diff --git a/drivers/media/dvb/dvb-core/dvb_generic.c b/drivers/media/dvb/dvb-core/dvb_generic.c
> > new file mode 100644
> > index 0000000..002bd24
> > --- /dev/null
> > +++ b/drivers/media/dvb/dvb-core/dvb_generic.c
> > @@ -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);
> > +
> > +        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.

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.

> 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.

-- 
Jean Delvare

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

[Antti Palosaari]

On 11/09/2011 12:37 PM, Jean Delvare wrote:
> On Wed, 09 Nov 2011 07:56:13 -0200, Mauro Carvalho Chehab wrote:
>>      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.
>
> 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 :)

> 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.

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.

> 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.

> 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.

I will check those later today, have to go back lecture.

regards
Antti

-- 
http://palosaari.fi/

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

[Mauro Carvalho Chehab]

Em 09-11-2011 08:37, Jean Delvare escreveu:
> On Wed, 09 Nov 2011 07:56:13 -0200, Mauro Carvalho Chehab wrote:
>> Em 08-11-2011 21:54, Antti Palosaari escreveu:
>>> Function that splits and sends most typical I2C register write.
>>>
>>> Signed-off-by: Antti Palosaari <crope@iki.fi>
>>> ---
>>>  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
>>>
>>> diff --git a/drivers/media/dvb/dvb-core/Makefile b/drivers/media/dvb/dvb-core/Makefile
>>> index 8f22bcd..230584a 100644
>>> --- a/drivers/media/dvb/dvb-core/Makefile
>>> +++ b/drivers/media/dvb/dvb-core/Makefile
>>> @@ -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
>>> diff --git a/drivers/media/dvb/dvb-core/dvb_generic.c b/drivers/media/dvb/dvb-core/dvb_generic.c
>>> new file mode 100644
>>> index 0000000..002bd24
>>> --- /dev/null
>>> +++ b/drivers/media/dvb/dvb-core/dvb_generic.c
>>> @@ -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);
>>> +
>>> +        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.
> 
> 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.

> 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.

>> 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.

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.

> 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.
> 

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

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

[Jean Delvare]

On Wed, 09 Nov 2011 10:02:08 -0200, Mauro Carvalho Chehab wrote:
> Em 09-11-2011 08:37, Jean Delvare escreveu:
> > 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.
> 
> 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.

-- 
Jean Delvare

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

[Antti Palosaari]

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/

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

[Antti Palosaari]

On 11/09/2011 11:56 AM, Mauro Carvalho Chehab wrote:
> 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.

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.

> 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

-- 
http://palosaari.fi/

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

[Jean Delvare]

On Wed, 09 Nov 2011 12:41:36 +0200, Antti Palosaari wrote:
> On 11/09/2011 11:56 AM, Mauro Carvalho Chehab wrote:
> > 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.
> 
> 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.

-- 
Jean Delvare

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

[Malcolm Priestley]

On 09/11/11 10:52, Jean Delvare wrote:
> On Wed, 09 Nov 2011 12:41:36 +0200, Antti Palosaari wrote:
>> On 11/09/2011 11:56 AM, Mauro Carvalho Chehab wrote:
>>> 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.
>>
>> 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.
>

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

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

[Jean Delvare]

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.

-- 
Jean Delvare