[PATCH v2 0/5] em28xx: i2c bug fixes and cleanups

[PATCH v2 0/5] em28xx: i2c bug fixes and cleanups

[Frank Schäfer]

This patch series contains some I2C bug fixes / cleanups / unifications and 
improvements for the em28xx driver, which I've made while working on adding 
support for the em25xx/em276x i2c bus B support and playing with the 
Terratec Cinergy 200 USB which I've got recently.

Patches 1 and 5 are cleanup/unification patches, patches 2, 3, 4 fix some bugs.
Patch 3 actually fixes 2 bugs, but separate commits didn't make sense, because 
more or less the whole function had to be rewritten.

Changelog v2:
- removed i2c address type/range check from patch 5 as requested by Antti Palosaari


Frank Schäfer (5):
  em28xx: clean up the data type mess of the i2c transfer function
    parameters
  em28xx: respect the message size constraints for i2c transfers
  em28xx: fix two severe bugs in function em2800_i2c_recv_bytes()
  em28xx: fix the i2c adapter functionality flags
  em28xx: fix+improve+unify i2c error handling, debug messages and code
    comments

 drivers/media/usb/em28xx/em28xx-core.c |    5 +-
 drivers/media/usb/em28xx/em28xx-i2c.c  |  276 +++++++++++++++++++-------------
 drivers/media/usb/em28xx/em28xx.h      |    2 +-
 3 Dateien geändert, 168 Zeilen hinzugefügt(+), 115 Zeilen entfernt(-)

-- 
1.7.10.4

[PATCH v2 1/5] em28xx: clean up the data type mess of the i2c transfer function parameters

[Frank Schäfer]

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   28 +++++++++++-----------------
 1 Datei geändert, 11 Zeilen hinzugefügt(+), 17 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 1683bd9..44533e4 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -53,12 +53,11 @@ do {							\
  * em2800_i2c_send_max4()
  * send up to 4 bytes to the i2c device
  */
-static int em2800_i2c_send_max4(struct em28xx *dev, unsigned char addr,
-				char *buf, int len)
+static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	int write_timeout;
-	unsigned char b2[6];
+	u8 b2[6];
 	BUG_ON(len < 1 || len > 4);
 	b2[5] = 0x80 + len - 1;
 	b2[4] = addr;
@@ -89,15 +88,13 @@ static int em2800_i2c_send_max4(struct em28xx *dev, unsigned char addr,
 /*
  * em2800_i2c_send_bytes()
  */
-static int em2800_i2c_send_bytes(void *data, unsigned char addr, char *buf,
-				 short len)
+static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
-	char *bufPtr = buf;
+	u8 *bufPtr = buf;
 	int ret;
 	int wrcount = 0;
 	int count;
 	int maxLen = 4;
-	struct em28xx *dev = (struct em28xx *)data;
 	while (len > 0) {
 		count = (len > maxLen) ? maxLen : len;
 		ret = em2800_i2c_send_max4(dev, addr, bufPtr, count);
@@ -115,9 +112,9 @@ static int em2800_i2c_send_bytes(void *data, unsigned char addr, char *buf,
  * em2800_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
-static int em2800_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
+static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 {
-	char msg;
+	u8 msg;
 	int ret;
 	int write_timeout;
 	msg = addr;
@@ -150,8 +147,7 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
  * em2800_i2c_recv_bytes()
  * read from the i2c device
  */
-static int em2800_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
-				 char *buf, int len)
+static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	/* check for the device and set i2c read address */
@@ -174,11 +170,10 @@ static int em2800_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
 /*
  * em28xx_i2c_send_bytes()
  */
-static int em28xx_i2c_send_bytes(void *data, unsigned char addr, char *buf,
-				 short len, int stop)
+static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
+				 u16 len, int stop)
 {
 	int wrcount = 0;
-	struct em28xx *dev = (struct em28xx *)data;
 	int write_timeout, ret;
 
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
@@ -199,8 +194,7 @@ static int em28xx_i2c_send_bytes(void *data, unsigned char addr, char *buf,
  * em28xx_i2c_recv_bytes()
  * read a byte from the i2c device
  */
-static int em28xx_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
-				 char *buf, int len)
+static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 {
 	int ret;
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
@@ -217,7 +211,7 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, unsigned char addr,
  * em28xx_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
-static int em28xx_i2c_check_for_device(struct em28xx *dev, unsigned char addr)
+static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
 {
 	int ret;
 
-- 
1.7.10.4

[PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

The em2800 can transfer up to 4 bytes per i2c message.
All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.

I2C adapters should never split messages transferred via the I2C subsystem
into multiple message transfers, because the result will almost always NOT be
the same as when the whole data is transferred to the I2C client in a single
message.
If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
should be returned.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |   44 ++++++++++++++-------------------
 1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 44533e4..c508c12 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -50,14 +50,18 @@ do {							\
 } while (0)
 
 /*
- * em2800_i2c_send_max4()
- * send up to 4 bytes to the i2c device
+ * em2800_i2c_send_bytes()
+ * send up to 4 bytes to the em2800 i2c device
  */
-static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
+static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
 	int write_timeout;
 	u8 b2[6];
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
 	BUG_ON(len < 1 || len > 4);
 	b2[5] = 0x80 + len - 1;
 	b2[4] = addr;
@@ -86,29 +90,6 @@ static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 }
 
 /*
- * em2800_i2c_send_bytes()
- */
-static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
-{
-	u8 *bufPtr = buf;
-	int ret;
-	int wrcount = 0;
-	int count;
-	int maxLen = 4;
-	while (len > 0) {
-		count = (len > maxLen) ? maxLen : len;
-		ret = em2800_i2c_send_max4(dev, addr, bufPtr, count);
-		if (ret > 0) {
-			len -= count;
-			bufPtr += count;
-			wrcount += count;
-		} else
-			return (ret < 0) ? ret : -EFAULT;
-	}
-	return wrcount;
-}
-
-/*
  * em2800_i2c_check_for_device()
  * check if there is a i2c_device at the supplied address
  */
@@ -150,6 +131,10 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
 	int ret;
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
 	/* check for the device and set i2c read address */
 	ret = em2800_i2c_check_for_device(dev, addr);
 	if (ret) {
@@ -176,6 +161,9 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	int wrcount = 0;
 	int write_timeout, ret;
 
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
 
 	/* Seems to be required after a write */
@@ -197,6 +185,10 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 {
 	int ret;
+
+	if (len < 1 || len > 64)
+		return -EOPNOTSUPP;
+
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
 	if (ret < 0) {
 		em28xx_warn("reading i2c device failed (error=%i)\n", ret);
-- 
1.7.10.4

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Mauro Carvalho Chehab]

Em Sun, 16 Dec 2012 19:23:28 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> The em2800 can transfer up to 4 bytes per i2c message.
> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.
> 
> I2C adapters should never split messages transferred via the I2C subsystem
> into multiple message transfers, because the result will almost always NOT be
> the same as when the whole data is transferred to the I2C client in a single
> message.
> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
> should be returned.
> 
> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> ---
>  drivers/media/usb/em28xx/em28xx-i2c.c |   44 ++++++++++++++-------------------
>  1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)
> 
> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> index 44533e4..c508c12 100644
> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> @@ -50,14 +50,18 @@ do {							\
>  } while (0)
>  
>  /*
> - * em2800_i2c_send_max4()
> - * send up to 4 bytes to the i2c device
> + * em2800_i2c_send_bytes()
> + * send up to 4 bytes to the em2800 i2c device
>   */
> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
> +static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>  {
>  	int ret;
>  	int write_timeout;
>  	u8 b2[6];
> +
> +	if (len < 1 || len > 4)
> +		return -EOPNOTSUPP;
> +

Except if you actually tested it with all em2800 devices, I think that
this change just broke it for em2800.

Maybe Sascha could review this patch series on his em2800 devices.

Those devices are limited, and just like other devices (cx231xx for example),
the I2C bus need to split long messages, otherwise the I2C devices will
fail.

Btw, there was already a long discussion with regards to splitting long
I2C messages at the I2C bus or at the I2C adapters. The decision was
to do it at the I2C bus logic, as it is simpler than making a code
at each I2C client for them to properly handle -EOPNOTSUPP and implement
a fallback logic to reduce the transfer window until reach what's
supported by the device.

So, for now, I won't apply this patch series (except for patch 1, with
is obviously correct).

Cheers,
Mauro

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
> Em Sun, 16 Dec 2012 19:23:28 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> The em2800 can transfer up to 4 bytes per i2c message.
>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.
>>
>> I2C adapters should never split messages transferred via the I2C subsystem
>> into multiple message transfers, because the result will almost always NOT be
>> the same as when the whole data is transferred to the I2C client in a single
>> message.
>> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
>> should be returned.
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-i2c.c |   44 ++++++++++++++-------------------
>>  1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>> index 44533e4..c508c12 100644
>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>> @@ -50,14 +50,18 @@ do {							\
>>  } while (0)
>>  
>>  /*
>> - * em2800_i2c_send_max4()
>> - * send up to 4 bytes to the i2c device
>> + * em2800_i2c_send_bytes()
>> + * send up to 4 bytes to the em2800 i2c device
>>   */
>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>> +static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>>  {
>>  	int ret;
>>  	int write_timeout;
>>  	u8 b2[6];
>> +
>> +	if (len < 1 || len > 4)
>> +		return -EOPNOTSUPP;
>> +
> Except if you actually tested it with all em2800 devices, I think that
> this change just broke it for em2800.

Yes, I have tested it. ;)
But it should be obvious that splitting messages doesn't work, because
the first byte of each message is treated as register address:

Let's say we want to transfer the 8 bytes 11 22 33 44 55 66 77 88 to
registers A0-A7 of a client.
The correspondig i2c message transferred via the i2c subsystem is A0 11
22 33 44 55 66 77 88
The current in em28xx splits this message into 3 messages:

message 1: A0 11 22 33
message 2: 44 55 66 77
message 3: 88

The result is, that only the first 3 bytes are transferred correctly.
The data bytes 44 and 88 are interpreted as register index and get lost,
data bytes 55 66 77 end up in registers 0x44, 0x45 and 0x46 instead of
0xA4 to 0xA6.
Ouch !

> Maybe Sascha could review this patch series on his em2800 devices.

Comments / reviews are appreciated.

> Those devices are limited, and just like other devices (cx231xx for example),
> the I2C bus need to split long messages, otherwise the I2C devices will
> fail.

I2C adapters are supposed to fail with -EOPNOTSUPP if the message length
exceeds their capabilities.
Drivers must be able to handle this error, otherwise they have to be fixed.

> Btw, there was already a long discussion with regards to splitting long
> I2C messages at the I2C bus or at the I2C adapters. The decision was
> to do it at the I2C bus logic, as it is simpler than making a code
> at each I2C client for them to properly handle -EOPNOTSUPP and implement
> a fallback logic to reduce the transfer window until reach what's
> supported by the device.

While letting the i2c bus layer split messages sounds like the right
thing to do, it is hard to realize that in practice.
The reason is, that the needed algorithm depends on the capabilities and
behavior of the i2c adapter _and_ the connected i2c client.
The three main parameters are:
- message size limits
- client register width
- automatic register index incrementation

I don't know what has been discussed in past, but I talked to Jean
Delvare about the message size constraints a few weeks ago.
He told me that it doesn't make sense to try to handle this at the i2c
subsystem level. The parameters can be different for reading and
writing, adapter and client and things are getting complicated quickly.

Regards,
Frank

> So, for now, I won't apply this patch series (except for patch 1, with
> is obviously correct).
>
> Cheers,
> Mauro

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Mauro Carvalho Chehab]

Em Sun, 23 Dec 2012 14:58:12 +0100
Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:

> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
> > Em Sun, 16 Dec 2012 19:23:28 +0100
> > Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >
> >> The em2800 can transfer up to 4 bytes per i2c message.
> >> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.
> >>
> >> I2C adapters should never split messages transferred via the I2C subsystem
> >> into multiple message transfers, because the result will almost always NOT be
> >> the same as when the whole data is transferred to the I2C client in a single
> >> message.
> >> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
> >> should be returned.
> >>
> >> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >> ---
> >>  drivers/media/usb/em28xx/em28xx-i2c.c |   44 ++++++++++++++-------------------
> >>  1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)
> >>
> >> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
> >> index 44533e4..c508c12 100644
> >> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >> @@ -50,14 +50,18 @@ do {							\
> >>  } while (0)
> >>  
> >>  /*
> >> - * em2800_i2c_send_max4()
> >> - * send up to 4 bytes to the i2c device
> >> + * em2800_i2c_send_bytes()
> >> + * send up to 4 bytes to the em2800 i2c device
> >>   */
> >> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
> >> +static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
> >>  {
> >>  	int ret;
> >>  	int write_timeout;
> >>  	u8 b2[6];
> >> +
> >> +	if (len < 1 || len > 4)
> >> +		return -EOPNOTSUPP;
> >> +
> > Except if you actually tested it with all em2800 devices, I think that
> > this change just broke it for em2800.
> 
> Yes, I have tested it. ;)
> But it should be obvious that splitting messages doesn't work, because
> the first byte of each message is treated as register address:
> 
> Let's say we want to transfer the 8 bytes 11 22 33 44 55 66 77 88 to
> registers A0-A7 of a client.
> The correspondig i2c message transferred via the i2c subsystem is A0 11
> 22 33 44 55 66 77 88
> The current in em28xx splits this message into 3 messages:
> 
> message 1: A0 11 22 33
> message 2: 44 55 66 77
> message 3: 88
> 
> The result is, that only the first 3 bytes are transferred correctly.
> The data bytes 44 and 88 are interpreted as register index and get lost,
> data bytes 55 66 77 end up in registers 0x44, 0x45 and 0x46 instead of
> 0xA4 to 0xA6.
> Ouch !

I see your point, but there are devices that allow sending a message
splitted into smaller URB data and do the right thing when the I2C
transfer actually happen. The cx231xx is one of such devices: it has
4 I2C buses internally; a few of them have a limit of 4 bytes per
transfer. Even so, manufacturers use this limited buses, plugging
I2C devices there that require I2C transfers with more than 4 bytes.

One of the tricks used on such devices to write long messages is
to disable I2C stop on the first I2C block, and to disable I2C
start+address data on the next messages. Only the I2C bus can do
such things, and such logic avoids to push down to all I2C clients
a complex logic to split messages there.

I'm not sure if this is the case of em2800 though, as I don't have
any device with it.

In any case, before dropping it, we should be sure that this won't
break any supported device. Probably the worse case are the TV-based
em2800 boards, with tuners, as they may need to transfer more than 4
bytes via I2C.

> 
> > Maybe Sascha could review this patch series on his em2800 devices.
> 
> Comments / reviews are appreciated.

Let's give him some time to review, before merging it.

> > Those devices are limited, and just like other devices (cx231xx for example),
> > the I2C bus need to split long messages, otherwise the I2C devices will
> > fail.
> 
> I2C adapters are supposed to fail with -EOPNOTSUPP if the message length
> exceeds their capabilities.
> Drivers must be able to handle this error, otherwise they have to be fixed.

Currently, afaikt, no V4L2 I2C client knows how to handle it. Ok, returning
-EOPNOTSUPP if the I2C data came from userspace makes sense.

> > Btw, there was already a long discussion with regards to splitting long
> > I2C messages at the I2C bus or at the I2C adapters. The decision was
> > to do it at the I2C bus logic, as it is simpler than making a code
> > at each I2C client for them to properly handle -EOPNOTSUPP and implement
> > a fallback logic to reduce the transfer window until reach what's
> > supported by the device.
> 
> While letting the i2c bus layer split messages sounds like the right
> thing to do, it is hard to realize that in practice.
> The reason is, that the needed algorithm depends on the capabilities and
> behavior of the i2c adapter _and_ the connected i2c client.
> The three main parameters are:
> - message size limits
> - client register width
> - automatic register index incrementation
> 
> I don't know what has been discussed in past,

You'll need to dig into the ML archives. This is a recurrent theme, and,
we have implementations doing I2C split at bus (most cases) and a few
ones doing it at the client side.

> but I talked to Jean
> Delvare about the message size constraints a few weeks ago.
> He told me that it doesn't make sense to try to handle this at the i2c
> subsystem level. The parameters can be different for reading and
> writing, adapter and client and things are getting complicated quickly.

Jean's opinion is to push it to I2C clients (and we actually do it on a
few cases), but as I explained before, there are several drivers where
this is better done at the I2C bus driver, as the I2C implementation 
allows doing it easily at bus level by playing with I2C STOP bits/I2C 
start bits.

We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't
tell the max size of the I2C messages. Adding a complex split logic
for every driver is not a common practice, as just a few I2C bus bridge
drivers suffer from very strict limits.

Also, clients that split I2C messages don't actually handle -EOPNOTSUPP.
Instead, they have an init parameter telling the maximum size of the
I2C messages accepted by the bus.

The logic there is complex, and may require an additional logic at the
bus side, in order to warrant that no I2C stop/start bits will be sent
in the middle of a message, or otherwise the device will fail[1].

So, it is generally simpler and more effective to just do it at the bus
side.

[1] not all I2C client drivers have an address sub-address. Also, several
of them require a long I2C message to be handled at once, otherwise the
device fails.

Regards,
Mauro

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
> Em Sun, 23 Dec 2012 14:58:12 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> The em2800 can transfer up to 4 bytes per i2c message.
>>>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes per message.
>>>>
>>>> I2C adapters should never split messages transferred via the I2C subsystem
>>>> into multiple message transfers, because the result will almost always NOT be
>>>> the same as when the whole data is transferred to the I2C client in a single
>>>> message.
>>>> If the message size exceeds the capabilities of the I2C adapter, -EOPNOTSUPP
>>>> should be returned.
>>>>
>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>> ---
>>>>  drivers/media/usb/em28xx/em28xx-i2c.c |   44 ++++++++++++++-------------------
>>>>  1 Datei geändert, 18 Zeilen hinzugefügt(+), 26 Zeilen entfernt(-)
>>>>
>>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> index 44533e4..c508c12 100644
>>>> --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>>> @@ -50,14 +50,18 @@ do {							\
>>>>  } while (0)
>>>>  
>>>>  /*
>>>> - * em2800_i2c_send_max4()
>>>> - * send up to 4 bytes to the i2c device
>>>> + * em2800_i2c_send_bytes()
>>>> + * send up to 4 bytes to the em2800 i2c device
>>>>   */
>>>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>>>> +static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
>>>>  {
>>>>  	int ret;
>>>>  	int write_timeout;
>>>>  	u8 b2[6];
>>>> +
>>>> +	if (len < 1 || len > 4)
>>>> +		return -EOPNOTSUPP;
>>>> +
>>> Except if you actually tested it with all em2800 devices, I think that
>>> this change just broke it for em2800.
>> Yes, I have tested it. ;)
>> But it should be obvious that splitting messages doesn't work, because
>> the first byte of each message is treated as register address:
>>
>> Let's say we want to transfer the 8 bytes 11 22 33 44 55 66 77 88 to
>> registers A0-A7 of a client.
>> The correspondig i2c message transferred via the i2c subsystem is A0 11
>> 22 33 44 55 66 77 88
>> The current in em28xx splits this message into 3 messages:
>>
>> message 1: A0 11 22 33
>> message 2: 44 55 66 77
>> message 3: 88
>>
>> The result is, that only the first 3 bytes are transferred correctly.
>> The data bytes 44 and 88 are interpreted as register index and get lost,
>> data bytes 55 66 77 end up in registers 0x44, 0x45 and 0x46 instead of
>> 0xA4 to 0xA6.
>> Ouch !
> I see your point, but there are devices that allow sending a message
> splitted into smaller URB data and do the right thing when the I2C
> transfer actually happen. The cx231xx is one of such devices: it has
> 4 I2C buses internally; a few of them have a limit of 4 bytes per
> transfer. Even so, manufacturers use this limited buses, plugging
> I2C devices there that require I2C transfers with more than 4 bytes.
>
> One of the tricks used on such devices to write long messages is
> to disable I2C stop on the first I2C block, and to disable I2C
> start+address data on the next messages. Only the I2C bus can do
> such things, and such logic avoids to push down to all I2C clients
> a complex logic to split messages there.

Yes, if the i2c adapter supports this feature, there is no problem with
splitting messages.

> I'm not sure if this is the case of em2800 though, as I don't have
> any device with it.

I'm not aware of a possibility to do this with the em2800.

> In any case, before dropping it, we should be sure that this won't
> break any supported device. Probably the worse case are the TV-based
> em2800 boards, with tuners, as they may need to transfer more than 4
> bytes via I2C.

I agree.

We currently have 6 boards with the em2800. Used i2c clients are

TDA9887
SAA711X
TUNER_PHILIPS_FCV1236D
TUNER_LG_PAL_NEW_TAPC
TUNER_LG_TALN
and in case of the Terratec Cinergy 200 USB an external IR receiver IC

None of these clients tries to transfer more than 4 bytes at once.
Terratec Cinergy 200 USB has also been tested by me.

Anyway, even if one of those client drivers would writes more than 4
bytes at once to the device, it wouldn't work with the current code.

>>> Maybe Sascha could review this patch series on his em2800 devices.
>> Comments / reviews are appreciated.
> Let's give him some time to review, before merging it.

Sure.

>>> Those devices are limited, and just like other devices (cx231xx for example),
>>> the I2C bus need to split long messages, otherwise the I2C devices will
>>> fail.
>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message length
>> exceeds their capabilities.
>> Drivers must be able to handle this error, otherwise they have to be fixed.
> Currently, afaikt, no V4L2 I2C client knows how to handle it.

Maybe. Fortunately, it seems to cause no trouble.

>  Ok, returning
> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>
>>> Btw, there was already a long discussion with regards to splitting long
>>> I2C messages at the I2C bus or at the I2C adapters. The decision was
>>> to do it at the I2C bus logic, as it is simpler than making a code
>>> at each I2C client for them to properly handle -EOPNOTSUPP and implement
>>> a fallback logic to reduce the transfer window until reach what's
>>> supported by the device.
>> While letting the i2c bus layer split messages sounds like the right
>> thing to do, it is hard to realize that in practice.
>> The reason is, that the needed algorithm depends on the capabilities and
>> behavior of the i2c adapter _and_ the connected i2c client.
>> The three main parameters are:
>> - message size limits
>> - client register width
>> - automatic register index incrementation
>>
>> I don't know what has been discussed in past,
> You'll need to dig into the ML archives. This is a recurrent theme, and,
> we have implementations doing I2C split at bus (most cases) and a few
> ones doing it at the client side.

Yeah, I also have a working implementation of i2c block read/write
emulation in my experimental code. ;)

>> but I talked to Jean
>> Delvare about the message size constraints a few weeks ago.
>> He told me that it doesn't make sense to try to handle this at the i2c
>> subsystem level. The parameters can be different for reading and
>> writing, adapter and client and things are getting complicated quickly.
> Jean's opinion is to push it to I2C clients (and we actually do it on a
> few cases), but as I explained before, there are several drivers where
> this is better done at the I2C bus driver, as the I2C implementation 
> allows doing it easily at bus level by playing with I2C STOP bits/I2C 
> start bits.
>
> We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't
> tell the max size of the I2C messages. Adding a complex split logic
> for every driver is not a common practice, as just a few I2C bus bridge
> drivers suffer from very strict limits.

Yes, and even with those who have such a strict limit, it is usually not
exceeded because the clients are too 'simple'. ;)

> Also, clients that split I2C messages don't actually handle -EOPNOTSUPP.
> Instead, they have an init parameter telling the maximum size of the
> I2C messages accepted by the bus.
>
> The logic there is complex, and may require an additional logic at the
> bus side, in order to warrant that no I2C stop/start bits will be sent
> in the middle of a message, or otherwise the device will fail[1].
>
> So, it is generally simpler and more effective to just do it at the bus
> side.

Maybe. I have no opinion yet.
My feeling is, that this should be handled by the i2c subsystem as much
as possible, but
a) it's complex due to the described reasons
b) I have no complete concept yet
c) the i2c people seem to be not very interested
d) there is lots of other stuff with a higher priority on my TODO list


The reason why I started looking into this is, that the em2765 in the
SpeedLink VAD Laplace webcam (and likely all similar chip variants, e.g.
em2760, em277x, em25xx) can transfer only 1 byte per message to/from the
sensor client when using the 2nd i2c bus. I don't know where this
restriction comes from, possible explanations are:
- because that the 2nd bus is realized using GPIO pins
- because the OV2640 uses SCCB
Sensor probing for example includes reading 16 bit VID and VID from the
client, which means that we require 2 instead of 1 i2c transfers.
Sure, for this special case we could live with it, especially because
the OV2640 driver uses single byte transfers only...

But that's a separate issue, we can talk about later in a different tread.

This patch just removes some code which is currently not working (and
appearantly not needed).
So I think it should be applied.

Regards,
Frank

> [1] not all I2C client drivers have an address sub-address. Also, several
> of them require a long I2C message to be handled at once, otherwise the
> device fails.
>
> Regards,
> Mauro

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Antti Palosaari]

On 12/24/2012 01:09 PM, Frank Schäfer wrote:
> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>> Em Sun, 23 Dec 2012 14:58:12 +0100
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>>>> Those devices are limited, and just like other devices (cx231xx for example),
>>>> the I2C bus need to split long messages, otherwise the I2C devices will
>>>> fail.
>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message length
>>> exceeds their capabilities.
>>> Drivers must be able to handle this error, otherwise they have to be fixed.
>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>
> Maybe. Fortunately, it seems to cause no trouble.
>
>>   Ok, returning
>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>
>>>> Btw, there was already a long discussion with regards to splitting long
>>>> I2C messages at the I2C bus or at the I2C adapters. The decision was
>>>> to do it at the I2C bus logic, as it is simpler than making a code
>>>> at each I2C client for them to properly handle -EOPNOTSUPP and implement
>>>> a fallback logic to reduce the transfer window until reach what's
>>>> supported by the device.
>>> While letting the i2c bus layer split messages sounds like the right
>>> thing to do, it is hard to realize that in practice.
>>> The reason is, that the needed algorithm depends on the capabilities and
>>> behavior of the i2c adapter _and_ the connected i2c client.
>>> The three main parameters are:
>>> - message size limits
>>> - client register width
>>> - automatic register index incrementation
>>>
>>> I don't know what has been discussed in past,
>> You'll need to dig into the ML archives. This is a recurrent theme, and,
>> we have implementations doing I2C split at bus (most cases) and a few
>> ones doing it at the client side.
>
> Yeah, I also have a working implementation of i2c block read/write
> emulation in my experimental code. ;)
>
>>> but I talked to Jean
>>> Delvare about the message size constraints a few weeks ago.
>>> He told me that it doesn't make sense to try to handle this at the i2c
>>> subsystem level. The parameters can be different for reading and
>>> writing, adapter and client and things are getting complicated quickly.
>> Jean's opinion is to push it to I2C clients (and we actually do it on a
>> few cases), but as I explained before, there are several drivers where
>> this is better done at the I2C bus driver, as the I2C implementation
>> allows doing it easily at bus level by playing with I2C STOP bits/I2C
>> start bits.
>>
>> We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't
>> tell the max size of the I2C messages. Adding a complex split logic
>> for every driver is not a common practice, as just a few I2C bus bridge
>> drivers suffer from very strict limits.
>
> Yes, and even with those who have such a strict limit, it is usually not
> exceeded because the clients are too 'simple'. ;)
>
>> Also, clients that split I2C messages don't actually handle -EOPNOTSUPP.
>> Instead, they have an init parameter telling the maximum size of the
>> I2C messages accepted by the bus.
>>
>> The logic there is complex, and may require an additional logic at the
>> bus side, in order to warrant that no I2C stop/start bits will be sent
>> in the middle of a message, or otherwise the device will fail[1].
>>
>> So, it is generally simpler and more effective to just do it at the bus
>> side.
>
> Maybe. I have no opinion yet.
> My feeling is, that this should be handled by the i2c subsystem as much
> as possible, but
> a) it's complex due to the described reasons
> b) I have no complete concept yet
> c) the i2c people seem to be not very interested
> d) there is lots of other stuff with a higher priority on my TODO list

Maybe you already have seen, but I did some initial stuff year or two 
ago for implementing that but left it unimplemented as there was so much 
stuff to check and discuss in order to agree correct solution.

http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html

There is regmap which maybe could do stuff like that, I am not sure as I 
never tested it. At least it could do some stuff top of I2C bus.

Also I heavily disagree you what goes to I2C subsystem integration. That 
is clearly stuff which resides top of I2C bus and it is *not bus 
dependent*. There is many other buses too having similar splitting logic 
like SPI?

> The reason why I started looking into this is, that the em2765 in the
> SpeedLink VAD Laplace webcam (and likely all similar chip variants, e.g.
> em2760, em277x, em25xx) can transfer only 1 byte per message to/from the
> sensor client when using the 2nd i2c bus. I don't know where this
> restriction comes from, possible explanations are:
> - because that the 2nd bus is realized using GPIO pins

Just to mention, there is existing framework for GPIO I2C.

> - because the OV2640 uses SCCB

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Hi Antti,

Am 02.01.2013 20:29, schrieb Antti Palosaari:
> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>>>> Those devices are limited, and just like other devices (cx231xx
>>>>> for example),
>>>>> the I2C bus need to split long messages, otherwise the I2C devices
>>>>> will
>>>>> fail.
>>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message
>>>> length
>>>> exceeds their capabilities.
>>>> Drivers must be able to handle this error, otherwise they have to
>>>> be fixed.
>>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>>
>> Maybe. Fortunately, it seems to cause no trouble.
>>
>>>   Ok, returning
>>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>>
>>>>> Btw, there was already a long discussion with regards to splitting
>>>>> long
>>>>> I2C messages at the I2C bus or at the I2C adapters. The decision was
>>>>> to do it at the I2C bus logic, as it is simpler than making a code
>>>>> at each I2C client for them to properly handle -EOPNOTSUPP and
>>>>> implement
>>>>> a fallback logic to reduce the transfer window until reach what's
>>>>> supported by the device.
>>>> While letting the i2c bus layer split messages sounds like the right
>>>> thing to do, it is hard to realize that in practice.
>>>> The reason is, that the needed algorithm depends on the
>>>> capabilities and
>>>> behavior of the i2c adapter _and_ the connected i2c client.
>>>> The three main parameters are:
>>>> - message size limits
>>>> - client register width
>>>> - automatic register index incrementation
>>>>
>>>> I don't know what has been discussed in past,
>>> You'll need to dig into the ML archives. This is a recurrent theme,
>>> and,
>>> we have implementations doing I2C split at bus (most cases) and a few
>>> ones doing it at the client side.
>>
>> Yeah, I also have a working implementation of i2c block read/write
>> emulation in my experimental code. ;)
>>
>>>> but I talked to Jean
>>>> Delvare about the message size constraints a few weeks ago.
>>>> He told me that it doesn't make sense to try to handle this at the i2c
>>>> subsystem level. The parameters can be different for reading and
>>>> writing, adapter and client and things are getting complicated
>>>> quickly.
>>> Jean's opinion is to push it to I2C clients (and we actually do it on a
>>> few cases), but as I explained before, there are several drivers where
>>> this is better done at the I2C bus driver, as the I2C implementation
>>> allows doing it easily at bus level by playing with I2C STOP bits/I2C
>>> start bits.
>>>
>>> We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't
>>> tell the max size of the I2C messages. Adding a complex split logic
>>> for every driver is not a common practice, as just a few I2C bus bridge
>>> drivers suffer from very strict limits.
>>
>> Yes, and even with those who have such a strict limit, it is usually not
>> exceeded because the clients are too 'simple'. ;)
>>
>>> Also, clients that split I2C messages don't actually handle
>>> -EOPNOTSUPP.
>>> Instead, they have an init parameter telling the maximum size of the
>>> I2C messages accepted by the bus.
>>>
>>> The logic there is complex, and may require an additional logic at the
>>> bus side, in order to warrant that no I2C stop/start bits will be sent
>>> in the middle of a message, or otherwise the device will fail[1].
>>>
>>> So, it is generally simpler and more effective to just do it at the bus
>>> side.
>>
>> Maybe. I have no opinion yet.
>> My feeling is, that this should be handled by the i2c subsystem as much
>> as possible, but
>> a) it's complex due to the described reasons
>> b) I have no complete concept yet
>> c) the i2c people seem to be not very interested
>> d) there is lots of other stuff with a higher priority on my TODO list
>
> Maybe you already have seen, but I did some initial stuff year or two
> ago for implementing that but left it unimplemented as there was so
> much stuff to check and discuss in order to agree correct solution.
>
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>
> There is regmap which maybe could do stuff like that, I am not sure as
> I never tested it. At least it could do some stuff top of I2C bus.

Yes, I've read this discussion, but didn't have time to take a deeper
look into the regmap stuff yet.

For the em28xx driver itself, there is no real need for i2c block
read/write emulation at the moment. We could save only a few lines.
I'm also burried with lots of other stuff at the moment which has a
higher priority for me.

Please note that the whole discussion has nothing to do with this patch.
It just removes code which isn't and has never been working.

>
> Also I heavily disagree you what goes to I2C subsystem integration.
> That is clearly stuff which resides top of I2C bus and it is *not bus
> dependent*. There is many other buses too having similar splitting
> logic like SPI?
>

I don't understand you. In which points do we disagree ??


Regards,
Frank

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Antti Palosaari]

On 01/02/2013 11:12 PM, Frank Schäfer wrote:
> Hi Antti,
>
> Am 02.01.2013 20:29, schrieb Antti Palosaari:
>> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>
>>>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>
>>>>>> Those devices are limited, and just like other devices (cx231xx
>>>>>> for example),
>>>>>> the I2C bus need to split long messages, otherwise the I2C devices
>>>>>> will
>>>>>> fail.
>>>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message
>>>>> length
>>>>> exceeds their capabilities.
>>>>> Drivers must be able to handle this error, otherwise they have to
>>>>> be fixed.
>>>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>>>
>>> Maybe. Fortunately, it seems to cause no trouble.
>>>
>>>>    Ok, returning
>>>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>>>
>>>>>> Btw, there was already a long discussion with regards to splitting
>>>>>> long
>>>>>> I2C messages at the I2C bus or at the I2C adapters. The decision was
>>>>>> to do it at the I2C bus logic, as it is simpler than making a code
>>>>>> at each I2C client for them to properly handle -EOPNOTSUPP and
>>>>>> implement
>>>>>> a fallback logic to reduce the transfer window until reach what's
>>>>>> supported by the device.
>>>>> While letting the i2c bus layer split messages sounds like the right
>>>>> thing to do, it is hard to realize that in practice.
>>>>> The reason is, that the needed algorithm depends on the
>>>>> capabilities and
>>>>> behavior of the i2c adapter _and_ the connected i2c client.
>>>>> The three main parameters are:
>>>>> - message size limits
>>>>> - client register width
>>>>> - automatic register index incrementation
>>>>>
>>>>> I don't know what has been discussed in past,
>>>> You'll need to dig into the ML archives. This is a recurrent theme,
>>>> and,
>>>> we have implementations doing I2C split at bus (most cases) and a few
>>>> ones doing it at the client side.
>>>
>>> Yeah, I also have a working implementation of i2c block read/write
>>> emulation in my experimental code. ;)
>>>
>>>>> but I talked to Jean
>>>>> Delvare about the message size constraints a few weeks ago.
>>>>> He told me that it doesn't make sense to try to handle this at the i2c
>>>>> subsystem level. The parameters can be different for reading and
>>>>> writing, adapter and client and things are getting complicated
>>>>> quickly.
>>>> Jean's opinion is to push it to I2C clients (and we actually do it on a
>>>> few cases), but as I explained before, there are several drivers where
>>>> this is better done at the I2C bus driver, as the I2C implementation
>>>> allows doing it easily at bus level by playing with I2C STOP bits/I2C
>>>> start bits.
>>>>
>>>> We simply have too much I2C clients, and -EOPNOTSUPP error code doesn't
>>>> tell the max size of the I2C messages. Adding a complex split logic
>>>> for every driver is not a common practice, as just a few I2C bus bridge
>>>> drivers suffer from very strict limits.
>>>
>>> Yes, and even with those who have such a strict limit, it is usually not
>>> exceeded because the clients are too 'simple'. ;)
>>>
>>>> Also, clients that split I2C messages don't actually handle
>>>> -EOPNOTSUPP.
>>>> Instead, they have an init parameter telling the maximum size of the
>>>> I2C messages accepted by the bus.
>>>>
>>>> The logic there is complex, and may require an additional logic at the
>>>> bus side, in order to warrant that no I2C stop/start bits will be sent
>>>> in the middle of a message, or otherwise the device will fail[1].
>>>>
>>>> So, it is generally simpler and more effective to just do it at the bus
>>>> side.
>>>
>>> Maybe. I have no opinion yet.
>>> My feeling is, that this should be handled by the i2c subsystem as much
>>> as possible, but
>>> a) it's complex due to the described reasons
>>> b) I have no complete concept yet
>>> c) the i2c people seem to be not very interested
>>> d) there is lots of other stuff with a higher priority on my TODO list
>>
>> Maybe you already have seen, but I did some initial stuff year or two
>> ago for implementing that but left it unimplemented as there was so
>> much stuff to check and discuss in order to agree correct solution.
>>
>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>>
>> There is regmap which maybe could do stuff like that, I am not sure as
>> I never tested it. At least it could do some stuff top of I2C bus.
>
> Yes, I've read this discussion, but didn't have time to take a deeper
> look into the regmap stuff yet.
>
> For the em28xx driver itself, there is no real need for i2c block
> read/write emulation at the moment. We could save only a few lines.
> I'm also burried with lots of other stuff at the moment which has a
> higher priority for me.
>
> Please note that the whole discussion has nothing to do with this patch.
> It just removes code which isn't and has never been working.
>
>>
>> Also I heavily disagree you what goes to I2C subsystem integration.
>> That is clearly stuff which resides top of I2C bus and it is *not bus
>> dependent*. There is many other buses too having similar splitting
>> logic like SPI?
>>
>
> I don't understand you. In which points do we disagree ??

"My feeling is, that this should be handled by the i2c subsystem as much 
as possible"

Antti


-- 
http://palosaari.fi/

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 02.01.2013 22:15, schrieb Antti Palosaari:
> On 01/02/2013 11:12 PM, Frank Schäfer wrote:
>> Hi Antti,
>>
>> Am 02.01.2013 20:29, schrieb Antti Palosaari:
>>> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>>>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>
>>>>>>> Those devices are limited, and just like other devices (cx231xx
>>>>>>> for example),
>>>>>>> the I2C bus need to split long messages, otherwise the I2C devices
>>>>>>> will
>>>>>>> fail.
>>>>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message
>>>>>> length
>>>>>> exceeds their capabilities.
>>>>>> Drivers must be able to handle this error, otherwise they have to
>>>>>> be fixed.
>>>>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>>>>
>>>> Maybe. Fortunately, it seems to cause no trouble.
>>>>
>>>>>    Ok, returning
>>>>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>>>>
>>>>>>> Btw, there was already a long discussion with regards to splitting
>>>>>>> long
>>>>>>> I2C messages at the I2C bus or at the I2C adapters. The decision
>>>>>>> was
>>>>>>> to do it at the I2C bus logic, as it is simpler than making a code
>>>>>>> at each I2C client for them to properly handle -EOPNOTSUPP and
>>>>>>> implement
>>>>>>> a fallback logic to reduce the transfer window until reach what's
>>>>>>> supported by the device.
>>>>>> While letting the i2c bus layer split messages sounds like the right
>>>>>> thing to do, it is hard to realize that in practice.
>>>>>> The reason is, that the needed algorithm depends on the
>>>>>> capabilities and
>>>>>> behavior of the i2c adapter _and_ the connected i2c client.
>>>>>> The three main parameters are:
>>>>>> - message size limits
>>>>>> - client register width
>>>>>> - automatic register index incrementation
>>>>>>
>>>>>> I don't know what has been discussed in past,
>>>>> You'll need to dig into the ML archives. This is a recurrent theme,
>>>>> and,
>>>>> we have implementations doing I2C split at bus (most cases) and a few
>>>>> ones doing it at the client side.
>>>>
>>>> Yeah, I also have a working implementation of i2c block read/write
>>>> emulation in my experimental code. ;)
>>>>
>>>>>> but I talked to Jean
>>>>>> Delvare about the message size constraints a few weeks ago.
>>>>>> He told me that it doesn't make sense to try to handle this at
>>>>>> the i2c
>>>>>> subsystem level. The parameters can be different for reading and
>>>>>> writing, adapter and client and things are getting complicated
>>>>>> quickly.
>>>>> Jean's opinion is to push it to I2C clients (and we actually do it
>>>>> on a
>>>>> few cases), but as I explained before, there are several drivers
>>>>> where
>>>>> this is better done at the I2C bus driver, as the I2C implementation
>>>>> allows doing it easily at bus level by playing with I2C STOP bits/I2C
>>>>> start bits.
>>>>>
>>>>> We simply have too much I2C clients, and -EOPNOTSUPP error code
>>>>> doesn't
>>>>> tell the max size of the I2C messages. Adding a complex split logic
>>>>> for every driver is not a common practice, as just a few I2C bus
>>>>> bridge
>>>>> drivers suffer from very strict limits.
>>>>
>>>> Yes, and even with those who have such a strict limit, it is
>>>> usually not
>>>> exceeded because the clients are too 'simple'. ;)
>>>>
>>>>> Also, clients that split I2C messages don't actually handle
>>>>> -EOPNOTSUPP.
>>>>> Instead, they have an init parameter telling the maximum size of the
>>>>> I2C messages accepted by the bus.
>>>>>
>>>>> The logic there is complex, and may require an additional logic at
>>>>> the
>>>>> bus side, in order to warrant that no I2C stop/start bits will be
>>>>> sent
>>>>> in the middle of a message, or otherwise the device will fail[1].
>>>>>
>>>>> So, it is generally simpler and more effective to just do it at
>>>>> the bus
>>>>> side.
>>>>
>>>> Maybe. I have no opinion yet.
>>>> My feeling is, that this should be handled by the i2c subsystem as
>>>> much
>>>> as possible, but
>>>> a) it's complex due to the described reasons
>>>> b) I have no complete concept yet
>>>> c) the i2c people seem to be not very interested
>>>> d) there is lots of other stuff with a higher priority on my TODO list
>>>
>>> Maybe you already have seen, but I did some initial stuff year or two
>>> ago for implementing that but left it unimplemented as there was so
>>> much stuff to check and discuss in order to agree correct solution.
>>>
>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>>>
>>> There is regmap which maybe could do stuff like that, I am not sure as
>>> I never tested it. At least it could do some stuff top of I2C bus.
>>
>> Yes, I've read this discussion, but didn't have time to take a deeper
>> look into the regmap stuff yet.
>>
>> For the em28xx driver itself, there is no real need for i2c block
>> read/write emulation at the moment. We could save only a few lines.
>> I'm also burried with lots of other stuff at the moment which has a
>> higher priority for me.
>>
>> Please note that the whole discussion has nothing to do with this patch.
>> It just removes code which isn't and has never been working.
>>
>>>
>>> Also I heavily disagree you what goes to I2C subsystem integration.
>>> That is clearly stuff which resides top of I2C bus and it is *not bus
>>> dependent*. There is many other buses too having similar splitting
>>> logic like SPI?
>>>
>>
>> I don't understand you. In which points do we disagree ??
>
> "My feeling is, that this should be handled by the i2c subsystem as
> much as possible"

Maybe I should have said "as much as makes sense" ;)
To be more precise: I think it's always good to have as much common code
as possible. And of course the complexity of the code must be justified
by it's benefits.
Do you agree ?

Frank
>
> Antti
>
>

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Antti Palosaari]

On 01/02/2013 11:29 PM, Frank Schäfer wrote:
> Am 02.01.2013 22:15, schrieb Antti Palosaari:
>> On 01/02/2013 11:12 PM, Frank Schäfer wrote:
>>> Hi Antti,
>>>
>>> Am 02.01.2013 20:29, schrieb Antti Palosaari:
>>>> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>>>>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>>>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>>
>>>>>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>>>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>
>>>>>>>> Those devices are limited, and just like other devices (cx231xx
>>>>>>>> for example),
>>>>>>>> the I2C bus need to split long messages, otherwise the I2C devices
>>>>>>>> will
>>>>>>>> fail.
>>>>>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message
>>>>>>> length
>>>>>>> exceeds their capabilities.
>>>>>>> Drivers must be able to handle this error, otherwise they have to
>>>>>>> be fixed.
>>>>>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>>>>>
>>>>> Maybe. Fortunately, it seems to cause no trouble.
>>>>>
>>>>>>     Ok, returning
>>>>>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>>>>>
>>>>>>>> Btw, there was already a long discussion with regards to splitting
>>>>>>>> long
>>>>>>>> I2C messages at the I2C bus or at the I2C adapters. The decision
>>>>>>>> was
>>>>>>>> to do it at the I2C bus logic, as it is simpler than making a code
>>>>>>>> at each I2C client for them to properly handle -EOPNOTSUPP and
>>>>>>>> implement
>>>>>>>> a fallback logic to reduce the transfer window until reach what's
>>>>>>>> supported by the device.
>>>>>>> While letting the i2c bus layer split messages sounds like the right
>>>>>>> thing to do, it is hard to realize that in practice.
>>>>>>> The reason is, that the needed algorithm depends on the
>>>>>>> capabilities and
>>>>>>> behavior of the i2c adapter _and_ the connected i2c client.
>>>>>>> The three main parameters are:
>>>>>>> - message size limits
>>>>>>> - client register width
>>>>>>> - automatic register index incrementation
>>>>>>>
>>>>>>> I don't know what has been discussed in past,
>>>>>> You'll need to dig into the ML archives. This is a recurrent theme,
>>>>>> and,
>>>>>> we have implementations doing I2C split at bus (most cases) and a few
>>>>>> ones doing it at the client side.
>>>>>
>>>>> Yeah, I also have a working implementation of i2c block read/write
>>>>> emulation in my experimental code. ;)
>>>>>
>>>>>>> but I talked to Jean
>>>>>>> Delvare about the message size constraints a few weeks ago.
>>>>>>> He told me that it doesn't make sense to try to handle this at
>>>>>>> the i2c
>>>>>>> subsystem level. The parameters can be different for reading and
>>>>>>> writing, adapter and client and things are getting complicated
>>>>>>> quickly.
>>>>>> Jean's opinion is to push it to I2C clients (and we actually do it
>>>>>> on a
>>>>>> few cases), but as I explained before, there are several drivers
>>>>>> where
>>>>>> this is better done at the I2C bus driver, as the I2C implementation
>>>>>> allows doing it easily at bus level by playing with I2C STOP bits/I2C
>>>>>> start bits.
>>>>>>
>>>>>> We simply have too much I2C clients, and -EOPNOTSUPP error code
>>>>>> doesn't
>>>>>> tell the max size of the I2C messages. Adding a complex split logic
>>>>>> for every driver is not a common practice, as just a few I2C bus
>>>>>> bridge
>>>>>> drivers suffer from very strict limits.
>>>>>
>>>>> Yes, and even with those who have such a strict limit, it is
>>>>> usually not
>>>>> exceeded because the clients are too 'simple'. ;)
>>>>>
>>>>>> Also, clients that split I2C messages don't actually handle
>>>>>> -EOPNOTSUPP.
>>>>>> Instead, they have an init parameter telling the maximum size of the
>>>>>> I2C messages accepted by the bus.
>>>>>>
>>>>>> The logic there is complex, and may require an additional logic at
>>>>>> the
>>>>>> bus side, in order to warrant that no I2C stop/start bits will be
>>>>>> sent
>>>>>> in the middle of a message, or otherwise the device will fail[1].
>>>>>>
>>>>>> So, it is generally simpler and more effective to just do it at
>>>>>> the bus
>>>>>> side.
>>>>>
>>>>> Maybe. I have no opinion yet.
>>>>> My feeling is, that this should be handled by the i2c subsystem as
>>>>> much
>>>>> as possible, but
>>>>> a) it's complex due to the described reasons
>>>>> b) I have no complete concept yet
>>>>> c) the i2c people seem to be not very interested
>>>>> d) there is lots of other stuff with a higher priority on my TODO list
>>>>
>>>> Maybe you already have seen, but I did some initial stuff year or two
>>>> ago for implementing that but left it unimplemented as there was so
>>>> much stuff to check and discuss in order to agree correct solution.
>>>>
>>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>>>>
>>>> There is regmap which maybe could do stuff like that, I am not sure as
>>>> I never tested it. At least it could do some stuff top of I2C bus.
>>>
>>> Yes, I've read this discussion, but didn't have time to take a deeper
>>> look into the regmap stuff yet.
>>>
>>> For the em28xx driver itself, there is no real need for i2c block
>>> read/write emulation at the moment. We could save only a few lines.
>>> I'm also burried with lots of other stuff at the moment which has a
>>> higher priority for me.
>>>
>>> Please note that the whole discussion has nothing to do with this patch.
>>> It just removes code which isn't and has never been working.
>>>
>>>>
>>>> Also I heavily disagree you what goes to I2C subsystem integration.
>>>> That is clearly stuff which resides top of I2C bus and it is *not bus
>>>> dependent*. There is many other buses too having similar splitting
>>>> logic like SPI?
>>>>
>>>
>>> I don't understand you. In which points do we disagree ??
>>
>> "My feeling is, that this should be handled by the i2c subsystem as
>> much as possible"
>
> Maybe I should have said "as much as makes sense" ;)
> To be more precise: I think it's always good to have as much common code
> as possible. And of course the complexity of the code must be justified
> by it's benefits.
> Do you agree ?

Common code is of course what it should be. Integrating that splitting 
logic to I2C subsystem is not common in the meaning as there is other 
buses than I2C as well. That splitting logic is something which is not 
that much bus specific.

Antti

-- 
http://palosaari.fi/

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 02.01.2013 22:40, schrieb Antti Palosaari:
> On 01/02/2013 11:29 PM, Frank Schäfer wrote:
>> Am 02.01.2013 22:15, schrieb Antti Palosaari:
>>> On 01/02/2013 11:12 PM, Frank Schäfer wrote:
>>>> Hi Antti,
>>>>
>>>> Am 02.01.2013 20:29, schrieb Antti Palosaari:
>>>>> On 12/24/2012 01:09 PM, Frank Schäfer wrote:
>>>>>> Am 23.12.2012 15:46, schrieb Mauro Carvalho Chehab:
>>>>>>> Em Sun, 23 Dec 2012 14:58:12 +0100
>>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>>>
>>>>>>>> Am 23.12.2012 01:07, schrieb Mauro Carvalho Chehab:
>>>>>>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>>>>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>>>
>>>>>>>>> Those devices are limited, and just like other devices (cx231xx
>>>>>>>>> for example),
>>>>>>>>> the I2C bus need to split long messages, otherwise the I2C
>>>>>>>>> devices
>>>>>>>>> will
>>>>>>>>> fail.
>>>>>>>> I2C adapters are supposed to fail with -EOPNOTSUPP if the message
>>>>>>>> length
>>>>>>>> exceeds their capabilities.
>>>>>>>> Drivers must be able to handle this error, otherwise they have to
>>>>>>>> be fixed.
>>>>>>> Currently, afaikt, no V4L2 I2C client knows how to handle it.
>>>>>>
>>>>>> Maybe. Fortunately, it seems to cause no trouble.
>>>>>>
>>>>>>>     Ok, returning
>>>>>>> -EOPNOTSUPP if the I2C data came from userspace makes sense.
>>>>>>>
>>>>>>>>> Btw, there was already a long discussion with regards to
>>>>>>>>> splitting
>>>>>>>>> long
>>>>>>>>> I2C messages at the I2C bus or at the I2C adapters. The decision
>>>>>>>>> was
>>>>>>>>> to do it at the I2C bus logic, as it is simpler than making a
>>>>>>>>> code
>>>>>>>>> at each I2C client for them to properly handle -EOPNOTSUPP and
>>>>>>>>> implement
>>>>>>>>> a fallback logic to reduce the transfer window until reach what's
>>>>>>>>> supported by the device.
>>>>>>>> While letting the i2c bus layer split messages sounds like the
>>>>>>>> right
>>>>>>>> thing to do, it is hard to realize that in practice.
>>>>>>>> The reason is, that the needed algorithm depends on the
>>>>>>>> capabilities and
>>>>>>>> behavior of the i2c adapter _and_ the connected i2c client.
>>>>>>>> The three main parameters are:
>>>>>>>> - message size limits
>>>>>>>> - client register width
>>>>>>>> - automatic register index incrementation
>>>>>>>>
>>>>>>>> I don't know what has been discussed in past,
>>>>>>> You'll need to dig into the ML archives. This is a recurrent theme,
>>>>>>> and,
>>>>>>> we have implementations doing I2C split at bus (most cases) and
>>>>>>> a few
>>>>>>> ones doing it at the client side.
>>>>>>
>>>>>> Yeah, I also have a working implementation of i2c block read/write
>>>>>> emulation in my experimental code. ;)
>>>>>>
>>>>>>>> but I talked to Jean
>>>>>>>> Delvare about the message size constraints a few weeks ago.
>>>>>>>> He told me that it doesn't make sense to try to handle this at
>>>>>>>> the i2c
>>>>>>>> subsystem level. The parameters can be different for reading and
>>>>>>>> writing, adapter and client and things are getting complicated
>>>>>>>> quickly.
>>>>>>> Jean's opinion is to push it to I2C clients (and we actually do it
>>>>>>> on a
>>>>>>> few cases), but as I explained before, there are several drivers
>>>>>>> where
>>>>>>> this is better done at the I2C bus driver, as the I2C
>>>>>>> implementation
>>>>>>> allows doing it easily at bus level by playing with I2C STOP
>>>>>>> bits/I2C
>>>>>>> start bits.
>>>>>>>
>>>>>>> We simply have too much I2C clients, and -EOPNOTSUPP error code
>>>>>>> doesn't
>>>>>>> tell the max size of the I2C messages. Adding a complex split logic
>>>>>>> for every driver is not a common practice, as just a few I2C bus
>>>>>>> bridge
>>>>>>> drivers suffer from very strict limits.
>>>>>>
>>>>>> Yes, and even with those who have such a strict limit, it is
>>>>>> usually not
>>>>>> exceeded because the clients are too 'simple'. ;)
>>>>>>
>>>>>>> Also, clients that split I2C messages don't actually handle
>>>>>>> -EOPNOTSUPP.
>>>>>>> Instead, they have an init parameter telling the maximum size of
>>>>>>> the
>>>>>>> I2C messages accepted by the bus.
>>>>>>>
>>>>>>> The logic there is complex, and may require an additional logic at
>>>>>>> the
>>>>>>> bus side, in order to warrant that no I2C stop/start bits will be
>>>>>>> sent
>>>>>>> in the middle of a message, or otherwise the device will fail[1].
>>>>>>>
>>>>>>> So, it is generally simpler and more effective to just do it at
>>>>>>> the bus
>>>>>>> side.
>>>>>>
>>>>>> Maybe. I have no opinion yet.
>>>>>> My feeling is, that this should be handled by the i2c subsystem as
>>>>>> much
>>>>>> as possible, but
>>>>>> a) it's complex due to the described reasons
>>>>>> b) I have no complete concept yet
>>>>>> c) the i2c people seem to be not very interested
>>>>>> d) there is lots of other stuff with a higher priority on my TODO
>>>>>> list
>>>>>
>>>>> Maybe you already have seen, but I did some initial stuff year or two
>>>>> ago for implementing that but left it unimplemented as there was so
>>>>> much stuff to check and discuss in order to agree correct solution.
>>>>>
>>>>> http://www.mail-archive.com/linux-media@vger.kernel.org/msg38840.html
>>>>>
>>>>> There is regmap which maybe could do stuff like that, I am not
>>>>> sure as
>>>>> I never tested it. At least it could do some stuff top of I2C bus.
>>>>
>>>> Yes, I've read this discussion, but didn't have time to take a deeper
>>>> look into the regmap stuff yet.
>>>>
>>>> For the em28xx driver itself, there is no real need for i2c block
>>>> read/write emulation at the moment. We could save only a few lines.
>>>> I'm also burried with lots of other stuff at the moment which has a
>>>> higher priority for me.
>>>>
>>>> Please note that the whole discussion has nothing to do with this
>>>> patch.
>>>> It just removes code which isn't and has never been working.
>>>>
>>>>>
>>>>> Also I heavily disagree you what goes to I2C subsystem integration.
>>>>> That is clearly stuff which resides top of I2C bus and it is *not bus
>>>>> dependent*. There is many other buses too having similar splitting
>>>>> logic like SPI?
>>>>>
>>>>
>>>> I don't understand you. In which points do we disagree ??
>>>
>>> "My feeling is, that this should be handled by the i2c subsystem as
>>> much as possible"
>>
>> Maybe I should have said "as much as makes sense" ;)
>> To be more precise: I think it's always good to have as much common code
>> as possible. And of course the complexity of the code must be justified
>> by it's benefits.
>> Do you agree ?
>
> Common code is of course what it should be. Integrating that splitting
> logic to I2C subsystem is not common in the meaning as there is other
> buses than I2C as well. That splitting logic is something which is not
> that much bus specific.

Ah !
Yes, maybe it makes sense to place this even on a higher level/layer.
Maybe regmap is already what we are looking for (or can be extended).

Frank

>
> Antti
>

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Sascha Sommer]

Hello,

Am Sat, 22 Dec 2012 22:07:46 -0200
schrieb Mauro Carvalho Chehab <mchehab@redhat.com>:

> Em Sun, 16 Dec 2012 19:23:28 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> 
> > The em2800 can transfer up to 4 bytes per i2c message.
> > All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
> > per message.
> > 
> > I2C adapters should never split messages transferred via the I2C
> > subsystem into multiple message transfers, because the result will
> > almost always NOT be the same as when the whole data is transferred
> > to the I2C client in a single message.
> > If the message size exceeds the capabilities of the I2C adapter,
> > -EOPNOTSUPP should be returned.
> > 
> > Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> > ---
> >  drivers/media/usb/em28xx/em28xx-i2c.c |   44
> > ++++++++++++++------------------- 1 Datei geändert, 18 Zeilen
> > hinzugefügt(+), 26 Zeilen entfernt(-)
> > 
> > diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
> > b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
> > 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> > +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> > @@ -50,14 +50,18 @@ do
> > {							\ } while
> > (0) 
> >  /*
> > - * em2800_i2c_send_max4()
> > - * send up to 4 bytes to the i2c device
> > + * em2800_i2c_send_bytes()
> > + * send up to 4 bytes to the em2800 i2c device
> >   */
> > -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
> > *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
> > *dev, u8 addr, u8 *buf, u16 len) {
> >  	int ret;
> >  	int write_timeout;
> >  	u8 b2[6];
> > +
> > +	if (len < 1 || len > 4)
> > +		return -EOPNOTSUPP;
> > +
> 
> Except if you actually tested it with all em2800 devices, I think that
> this change just broke it for em2800.
> 
> Maybe Sascha could review this patch series on his em2800 devices.
> 
> Those devices are limited, and just like other devices (cx231xx for
> example), the I2C bus need to split long messages, otherwise the I2C
> devices will fail.
> 

The only device that I own is the Terratec Cinergy 200 USB.
Unfortunately I left it in my parents house so I won't be able to
test the patch within the next two weeks. I don't know if any of the
other devices ever transfered more than 4 bytes but as far as I
remember the windows driver of the cinergy 200 usb did not do this.
The traces obtained from it were the only information I had during
development. On first sight, the splitting code looks wrong.

Regards

Sascha

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Hi Sascha,

Am 02.01.2013 21:45, schrieb Sascha Sommer:
> Hello,
>
> Am Sat, 22 Dec 2012 22:07:46 -0200
> schrieb Mauro Carvalho Chehab <mchehab@redhat.com>:
>
>> Em Sun, 16 Dec 2012 19:23:28 +0100
>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>
>>> The em2800 can transfer up to 4 bytes per i2c message.
>>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
>>> per message.
>>>
>>> I2C adapters should never split messages transferred via the I2C
>>> subsystem into multiple message transfers, because the result will
>>> almost always NOT be the same as when the whole data is transferred
>>> to the I2C client in a single message.
>>> If the message size exceeds the capabilities of the I2C adapter,
>>> -EOPNOTSUPP should be returned.
>>>
>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>> ---
>>>  drivers/media/usb/em28xx/em28xx-i2c.c |   44
>>> ++++++++++++++------------------- 1 Datei geändert, 18 Zeilen
>>> hinzugefügt(+), 26 Zeilen entfernt(-)
>>>
>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
>>> 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>> @@ -50,14 +50,18 @@ do
>>> {							\ } while
>>> (0) 
>>>  /*
>>> - * em2800_i2c_send_max4()
>>> - * send up to 4 bytes to the i2c device
>>> + * em2800_i2c_send_bytes()
>>> + * send up to 4 bytes to the em2800 i2c device
>>>   */
>>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
>>> *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
>>> *dev, u8 addr, u8 *buf, u16 len) {
>>>  	int ret;
>>>  	int write_timeout;
>>>  	u8 b2[6];
>>> +
>>> +	if (len < 1 || len > 4)
>>> +		return -EOPNOTSUPP;
>>> +
>> Except if you actually tested it with all em2800 devices, I think that
>> this change just broke it for em2800.
>>
>> Maybe Sascha could review this patch series on his em2800 devices.
>>
>> Those devices are limited, and just like other devices (cx231xx for
>> example), the I2C bus need to split long messages, otherwise the I2C
>> devices will fail.
>>
> The only device that I own is the Terratec Cinergy 200 USB.
> Unfortunately I left it in my parents house so I won't be able to
> test the patch within the next two weeks. I don't know if any of the
> other devices ever transfered more than 4 bytes but as far as I
> remember the windows driver of the cinergy 200 usb did not do this.
> The traces obtained from it were the only information I had during
> development. On first sight, the splitting code looks wrong.

Thanks for your reply !
I have a Terratec Cinergy 200 USB, too, and I used this device for
testing the code.
You are right, the Windows driver never transfers more than 4 bytes
(verified with USB-logs).
Do you know if there is something like a control flag for non-stopping
i2c transfers ?

Maybe you also noticed the following tread:
http://www.spinics.net/lists/linux-media/msg57442.html

Do you remember any details about your device ?
One thing not mentioned in this tread is, that there seem to be multiple
chip IDs for the EM2800.
The em28xx only knows about ID=7 and I assume that's what you device
uses. But the chip in my device uses ID=4...

Regards,
Frank

> Regards
>
> Sascha
>
>

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Sascha Sommer]

Hello Frank,

Am Wed, 02 Jan 2013 22:25:10 +0100
schrieb Frank Schäfer <fschaefer.oss@googlemail.com>:

> Hi Sascha,
> 
> Am 02.01.2013 21:45, schrieb Sascha Sommer:
> > Hello,
> >
> > Am Sat, 22 Dec 2012 22:07:46 -0200
> > schrieb Mauro Carvalho Chehab <mchehab@redhat.com>:
> >
> >> Em Sun, 16 Dec 2012 19:23:28 +0100
> >> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
> >>
> >>> The em2800 can transfer up to 4 bytes per i2c message.
> >>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
> >>> per message.
> >>>
> >>> I2C adapters should never split messages transferred via the I2C
> >>> subsystem into multiple message transfers, because the result will
> >>> almost always NOT be the same as when the whole data is
> >>> transferred to the I2C client in a single message.
> >>> If the message size exceeds the capabilities of the I2C adapter,
> >>> -EOPNOTSUPP should be returned.
> >>>
> >>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
> >>> ---
> >>>  drivers/media/usb/em28xx/em28xx-i2c.c |   44
> >>> ++++++++++++++------------------- 1 Datei geändert, 18 Zeilen
> >>> hinzugefügt(+), 26 Zeilen entfernt(-)
> >>>
> >>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
> >>> 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
> >>> @@ -50,14 +50,18 @@ do
> >>> {							\ } while
> >>> (0) 
> >>>  /*
> >>> - * em2800_i2c_send_max4()
> >>> - * send up to 4 bytes to the i2c device
> >>> + * em2800_i2c_send_bytes()
> >>> + * send up to 4 bytes to the em2800 i2c device
> >>>   */
> >>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
> >>> *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
> >>> *dev, u8 addr, u8 *buf, u16 len) {
> >>>  	int ret;
> >>>  	int write_timeout;
> >>>  	u8 b2[6];
> >>> +
> >>> +	if (len < 1 || len > 4)
> >>> +		return -EOPNOTSUPP;
> >>> +
> >> Except if you actually tested it with all em2800 devices, I think
> >> that this change just broke it for em2800.
> >>
> >> Maybe Sascha could review this patch series on his em2800 devices.
> >>
> >> Those devices are limited, and just like other devices (cx231xx for
> >> example), the I2C bus need to split long messages, otherwise the
> >> I2C devices will fail.
> >>
> > The only device that I own is the Terratec Cinergy 200 USB.
> > Unfortunately I left it in my parents house so I won't be able to
> > test the patch within the next two weeks. I don't know if any of the
> > other devices ever transfered more than 4 bytes but as far as I
> > remember the windows driver of the cinergy 200 usb did not do this.
> > The traces obtained from it were the only information I had during
> > development. On first sight, the splitting code looks wrong.
> 
> Thanks for your reply !
> I have a Terratec Cinergy 200 USB, too, and I used this device for
> testing the code.
> You are right, the Windows driver never transfers more than 4 bytes
> (verified with USB-logs).
> Do you know if there is something like a control flag for non-stopping
> i2c transfers ?

I never encountered such a flag.

> 
> Maybe you also noticed the following tread:
> http://www.spinics.net/lists/linux-media/msg57442.html
> 
> Do you remember any details about your device ?

The description in this thread also matches my device.
Maybe an additional hint is that it can't capture at full
resolution because the USB packet size is to small. Therefore
the em28xx driver somewhere limits it to 360x576 for PAL (Some commits
broke this code in the past)

> One thing not mentioned in this tread is, that there seem to be
> multiple chip IDs for the EM2800.
> The em28xx only knows about ID=7 and I assume that's what you device
> uses. But the chip in my device uses ID=4...

I checked some of my old mails. The original driver used the
chip id for device detection because some users had different devices
with different ids but this might have been a coincidence. 

Judging from the patch below my card uses chip id 4, too.

---
25/drivers/media/video/em28xx/em28xx-cards.c~v4l-786-chip-id-removed-since-it-isn-t-required
Fri Nov  4 16:20:36 2005 +++
25-akpm/drivers/media/video/em28xx/em28xx-cards.c	Fri Nov  4
16:20:37 2005 @@ -160,7 +160,6 @@ struct em2820_board em2820_boards[] =
{ }, [EM2800_BOARD_TERRATEC_CINERGY_200] = { .name         = "Terratec
Cinergy 200 USB",
-		.chip_id      = 0x4,
 		.is_em2800    = 1,
 		.vchannels    = 3,
 		.norm         = VIDEO_MODE_PAL,
@@ -184,7 +183,6 @@ struct em2820_board em2820_boards[] = {
 	},
 	[EM2800_BOARD_LEADTEK_WINFAST_USBII] = {
 		.name         = "Leadtek Winfast USB II",
-		.chip_id      = 0x2,
 		.is_em2800    = 1,
 		.vchannels    = 3,
 		.norm         = VIDEO_MODE_PAL,
@@ -208,7 +206,6 @@ struct em2820_board em2820_boards[] = {
 	},
 	[EM2800_BOARD_KWORLD_USB2800] = {
 		.name         = "Kworld USB2800",
-		.chip_id      = 0x7,
 		.is_em2800    = 1,
 		.vchannels    = 3,
 		.norm         = VIDEO_MODE_PAL,


Regards

Sascha

Re: [PATCH v2 2/5] em28xx: respect the message size constraints for i2c transfers

[Frank Schäfer]

Am 02.01.2013 23:21, schrieb Sascha Sommer:
> Hello Frank,
>
> Am Wed, 02 Jan 2013 22:25:10 +0100
> schrieb Frank Schäfer <fschaefer.oss@googlemail.com>:
>
>> Hi Sascha,
>>
>> Am 02.01.2013 21:45, schrieb Sascha Sommer:
>>> Hello,
>>>
>>> Am Sat, 22 Dec 2012 22:07:46 -0200
>>> schrieb Mauro Carvalho Chehab <mchehab@redhat.com>:
>>>
>>>> Em Sun, 16 Dec 2012 19:23:28 +0100
>>>> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>>>>
>>>>> The em2800 can transfer up to 4 bytes per i2c message.
>>>>> All other em25xx/em27xx/28xx chips can transfer at least 64 bytes
>>>>> per message.
>>>>>
>>>>> I2C adapters should never split messages transferred via the I2C
>>>>> subsystem into multiple message transfers, because the result will
>>>>> almost always NOT be the same as when the whole data is
>>>>> transferred to the I2C client in a single message.
>>>>> If the message size exceeds the capabilities of the I2C adapter,
>>>>> -EOPNOTSUPP should be returned.
>>>>>
>>>>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>>>>> ---
>>>>>  drivers/media/usb/em28xx/em28xx-i2c.c |   44
>>>>> ++++++++++++++------------------- 1 Datei geändert, 18 Zeilen
>>>>> hinzugefügt(+), 26 Zeilen entfernt(-)
>>>>>
>>>>> diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c
>>>>> b/drivers/media/usb/em28xx/em28xx-i2c.c index 44533e4..c508c12
>>>>> 100644 --- a/drivers/media/usb/em28xx/em28xx-i2c.c
>>>>> +++ b/drivers/media/usb/em28xx/em28xx-i2c.c
>>>>> @@ -50,14 +50,18 @@ do
>>>>> {							\ } while
>>>>> (0) 
>>>>>  /*
>>>>> - * em2800_i2c_send_max4()
>>>>> - * send up to 4 bytes to the i2c device
>>>>> + * em2800_i2c_send_bytes()
>>>>> + * send up to 4 bytes to the em2800 i2c device
>>>>>   */
>>>>> -static int em2800_i2c_send_max4(struct em28xx *dev, u8 addr, u8
>>>>> *buf, u16 len) +static int em2800_i2c_send_bytes(struct em28xx
>>>>> *dev, u8 addr, u8 *buf, u16 len) {
>>>>>  	int ret;
>>>>>  	int write_timeout;
>>>>>  	u8 b2[6];
>>>>> +
>>>>> +	if (len < 1 || len > 4)
>>>>> +		return -EOPNOTSUPP;
>>>>> +
>>>> Except if you actually tested it with all em2800 devices, I think
>>>> that this change just broke it for em2800.
>>>>
>>>> Maybe Sascha could review this patch series on his em2800 devices.
>>>>
>>>> Those devices are limited, and just like other devices (cx231xx for
>>>> example), the I2C bus need to split long messages, otherwise the
>>>> I2C devices will fail.
>>>>
>>> The only device that I own is the Terratec Cinergy 200 USB.
>>> Unfortunately I left it in my parents house so I won't be able to
>>> test the patch within the next two weeks. I don't know if any of the
>>> other devices ever transfered more than 4 bytes but as far as I
>>> remember the windows driver of the cinergy 200 usb did not do this.
>>> The traces obtained from it were the only information I had during
>>> development. On first sight, the splitting code looks wrong.
>> Thanks for your reply !
>> I have a Terratec Cinergy 200 USB, too, and I used this device for
>> testing the code.
>> You are right, the Windows driver never transfers more than 4 bytes
>> (verified with USB-logs).
>> Do you know if there is something like a control flag for non-stopping
>> i2c transfers ?
> I never encountered such a flag.

Ok. So there is no way to realize block writes > 4 bytes without
considering address and register width of the i2c client.

>
>> Maybe you also noticed the following tread:
>> http://www.spinics.net/lists/linux-media/msg57442.html
>>
>> Do you remember any details about your device ?
> The description in this thread also matches my device.

Ok.

> Maybe an additional hint is that it can't capture at full
> resolution because the USB packet size is to small. 

Interesting. Would be interesting to test it with USB bulk transfers
(which has been added recently).

> Therefore
> the em28xx driver somewhere limits it to 360x576 for PAL (Some commits
> broke this code in the past)

Yeah, I noticed that something is wrong with the resolutions
qv4l shows two resolutions: 720x576 and 360x288.
The first one seems to be 320x576 in reality and the latter doesn't work.
And there seems to be a problem with RGB565 LE and the 8bit bayer
formats. Not sure if the em2800 really supports them. Could be qv4l or
libv4l issues, too.

I didn't have time yet to look at this. It's on my TODO list but has low
priority.

>
>> One thing not mentioned in this tread is, that there seem to be
>> multiple chip IDs for the EM2800.
>> The em28xx only knows about ID=7 and I assume that's what you device
>> uses. But the chip in my device uses ID=4...
> I checked some of my old mails. The original driver used the
> chip id for device detection because some users had different devices
> with different ids but this might have been a coincidence. 
>
> Judging from the patch below my card uses chip id 4, too.
>
> ---
> 25/drivers/media/video/em28xx/em28xx-cards.c~v4l-786-chip-id-removed-since-it-isn-t-required
> Fri Nov  4 16:20:36 2005 +++
> 25-akpm/drivers/media/video/em28xx/em28xx-cards.c	Fri Nov  4
> 16:20:37 2005 @@ -160,7 +160,6 @@ struct em2820_board em2820_boards[] =
> { }, [EM2800_BOARD_TERRATEC_CINERGY_200] = { .name         = "Terratec
> Cinergy 200 USB",
> -		.chip_id      = 0x4,
>  		.is_em2800    = 1,
>  		.vchannels    = 3,
>  		.norm         = VIDEO_MODE_PAL,
> @@ -184,7 +183,6 @@ struct em2820_board em2820_boards[] = {
>  	},
>  	[EM2800_BOARD_LEADTEK_WINFAST_USBII] = {
>  		.name         = "Leadtek Winfast USB II",
> -		.chip_id      = 0x2,
>  		.is_em2800    = 1,
>  		.vchannels    = 3,
>  		.norm         = VIDEO_MODE_PAL,
> @@ -208,7 +206,6 @@ struct em2820_board em2820_boards[] = {
>  	},
>  	[EM2800_BOARD_KWORLD_USB2800] = {
>  		.name         = "Kworld USB2800",
> -		.chip_id      = 0x7,
>  		.is_em2800    = 1,
>  		.vchannels    = 3,
>  		.norm         = VIDEO_MODE_PAL,
>

Interesting, thanks !

Regards,
Frank

> Regards
>
> Sascha
>

[PATCH v2 3/5] em28xx: fix two severe bugs in function em2800_i2c_recv_bytes()

[Frank Schäfer]

Function em2800_i2c_recv_bytes() has 2 severe bugs:
1) It does not wait for the i2c read to complete before reading the received message content from the bridge registers.
2) Reading more than 1 byte doesn't work

The former can result in data corruption, the latter always does.

The rewritten code also superseds the content of function
em2800_i2c_check_for_device().

Tested with device "Terratec Cinergy 200 USB".

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |  104 ++++++++++++++++++---------------
 drivers/media/usb/em28xx/em28xx.h     |    2 +-
 2 Dateien geändert, 58 Zeilen hinzugefügt(+), 48 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index c508c12..940ff4d 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -73,12 +73,14 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 	if (len > 3)
 		b2[0] = buf[3];
 
+	/* trigger write */
 	ret = dev->em28xx_write_regs(dev, 4 - len, &b2[4 - len], 2 + len);
 	if (ret != 2 + len) {
 		em28xx_warn("writing to i2c device failed (error=%i)\n", ret);
 		return -EIO;
 	}
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
+	/* wait for completion */
+	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
 		if (ret == 0x80 + len - 1)
@@ -90,66 +92,74 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 }
 
 /*
- * em2800_i2c_check_for_device()
- * check if there is a i2c_device at the supplied address
+ * em2800_i2c_recv_bytes()
+ * read up to 4 bytes from the em2800 i2c device
  */
-static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
+static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 {
-	u8 msg;
+	u8 buf2[4];
 	int ret;
-	int write_timeout;
-	msg = addr;
-	ret = dev->em28xx_write_regs(dev, 0x04, &msg, 1);
-	if (ret < 0) {
-		em28xx_warn("setting i2c device address failed (error=%i)\n",
-			    ret);
-		return ret;
-	}
-	msg = 0x84;
-	ret = dev->em28xx_write_regs(dev, 0x05, &msg, 1);
-	if (ret < 0) {
-		em28xx_warn("preparing i2c read failed (error=%i)\n", ret);
-		return ret;
+	int read_timeout;
+	int i;
+
+	if (len < 1 || len > 4)
+		return -EOPNOTSUPP;
+
+	/* trigger read */
+	buf2[1] = 0x84 + len - 1;
+	buf2[0] = addr;
+	ret = dev->em28xx_write_regs(dev, 0x04, buf2, 2);
+	if (ret != 2) {
+		em28xx_warn("failed to trigger read from i2c address 0x%x "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
 	}
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
-	     write_timeout -= 5) {
-		unsigned reg = dev->em28xx_read_reg(dev, 0x5);
 
-		if (reg == 0x94)
+	/* wait for completion */
+	for (read_timeout = EM2800_I2C_XFER_TIMEOUT; read_timeout > 0;
+	     read_timeout -= 5) {
+		ret = dev->em28xx_read_reg(dev, 0x05);
+		if (ret == 0x84 + len - 1) {
+			break;
+		} else if (ret == 0x94 + len - 1) {
 			return -ENODEV;
-		else if (reg == 0x84)
-			return 0;
+		} else if (ret < 0) {
+			em28xx_warn("failed to get i2c transfer status from "
+				    "bridge register (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
 	}
-	return -ENODEV;
+	if (ret != 0x84 + len - 1)
+		em28xx_warn("read from i2c device at 0x%x timed out\n", addr);
+
+	/* get the received message */
+	ret = dev->em28xx_read_reg_req_len(dev, 0x00, 4-len, buf2, len);
+	if (ret != len) {
+		em28xx_warn("reading from i2c device at 0x%x failed: "
+			    "couldn't get the received message from the bridge "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
+	}
+	for (i=0; i<len; i++)
+		buf[i] = buf2[len-1-i];
+
+	return ret;
 }
 
 /*
- * em2800_i2c_recv_bytes()
- * read from the i2c device
+ * em2800_i2c_check_for_device()
+ * check if there is an i2c device at the supplied address
  */
-static int em2800_i2c_recv_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
+static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 {
+	u8 buf;
 	int ret;
 
-	if (len < 1 || len > 4)
-		return -EOPNOTSUPP;
-
-	/* check for the device and set i2c read address */
-	ret = em2800_i2c_check_for_device(dev, addr);
-	if (ret) {
-		em28xx_warn
-		    ("preparing read at i2c address 0x%x failed (error=%i)\n",
-		     addr, ret);
-		return ret;
-	}
-	ret = dev->em28xx_read_reg_req_len(dev, 0x0, 0x3, buf, len);
-	if (ret < 0) {
-		em28xx_warn("reading from i2c device at 0x%x failed (error=%i)",
-			    addr, ret);
-		return ret;
-	}
-	return ret;
+	ret = em2800_i2c_recv_bytes(dev, addr, &buf, 1);
+	if (ret == 1)
+		return 0;
+	return (ret < 0) ? ret : -EIO;
 }
 
 /*
@@ -167,7 +177,7 @@ static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
 
 	/* Seems to be required after a write */
-	for (write_timeout = EM2800_I2C_WRITE_TIMEOUT; write_timeout > 0;
+	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
 		if (!ret)
diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
index 062841e..dc89aaf 100644
--- a/drivers/media/usb/em28xx/em28xx.h
+++ b/drivers/media/usb/em28xx/em28xx.h
@@ -195,7 +195,7 @@
 */
 
 /* time in msecs to wait for i2c writes to finish */
-#define EM2800_I2C_WRITE_TIMEOUT 20
+#define EM2800_I2C_XFER_TIMEOUT		20
 
 enum em28xx_mode {
 	EM28XX_SUSPEND,
-- 
1.7.10.4

[PATCH v2 4/5] em28xx: fix the i2c adapter functionality flags

[Frank Schäfer]

I2C_FUNC_SMBUS_EMUL includes flag I2C_FUNC_SMBUS_WRITE_BLOCK_DATA which signals
that up to 31 data bytes can be written to the ic2 client.
But the EM2800 supports only i2c messages with max. 4 data bytes.
I2C_FUNC_IC2 should be set if a master_xfer fucntion pointer is provided in
struct i2c_algorithm.

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-i2c.c |    6 +++++-
 1 Datei geändert, 5 Zeilen hinzugefügt(+), 1 Zeile entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 940ff4d..7118535 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -445,7 +445,11 @@ static int em28xx_i2c_eeprom(struct em28xx *dev, unsigned char *eedata, int len)
  */
 static u32 functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_SMBUS_EMUL;
+	struct em28xx *dev = adap->algo_data;
+	u32 func_flags = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	if (dev->board.is_em2800)
+		func_flags &= ~I2C_FUNC_SMBUS_WRITE_BLOCK_DATA;
+	return func_flags;
 }
 
 static struct i2c_algorithm em28xx_algo = {
-- 
1.7.10.4

[PATCH v2 5/5] em28xx: fix+improve+unify i2c error handling, debug messages and code comments

[Frank Schäfer]

- do not pass USB specific error codes to userspace/i2c-subsystem
- unify the returned error codes and make them compliant with
  the i2c subsystem spec
- check number of actually transferred bytes (via USB) everywehere
- fix/improve debug messages
- improve code comments

Changelog v2:
- removed i2c address type/range check as requested by Antti Palosaari

Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
---
 drivers/media/usb/em28xx/em28xx-core.c |    5 +-
 drivers/media/usb/em28xx/em28xx-i2c.c  |  112 +++++++++++++++++++++++---------
 2 Dateien geändert, 85 Zeilen hinzugefügt(+), 32 Zeilen entfernt(-)

diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
index c78d38b..cd808bf 100644
--- a/drivers/media/usb/em28xx/em28xx-core.c
+++ b/drivers/media/usb/em28xx/em28xx-core.c
@@ -101,7 +101,7 @@ int em28xx_read_reg_req_len(struct em28xx *dev, u8 req, u16 reg,
 		if (reg_debug)
 			printk(" failed!\n");
 		mutex_unlock(&dev->ctrl_urb_lock);
-		return ret;
+		return usb_translate_errors(ret);
 	}
 
 	if (len)
@@ -182,6 +182,9 @@ int em28xx_write_regs_req(struct em28xx *dev, u8 req, u16 reg, char *buf,
 			      0x0000, reg, dev->urb_buf, len, HZ);
 	mutex_unlock(&dev->ctrl_urb_lock);
 
+	if (ret < 0)
+		return usb_translate_errors(ret);
+
 	if (dev->wait_after_write)
 		msleep(dev->wait_after_write);
 
diff --git a/drivers/media/usb/em28xx/em28xx-i2c.c b/drivers/media/usb/em28xx/em28xx-i2c.c
index 7118535..c57db43 100644
--- a/drivers/media/usb/em28xx/em28xx-i2c.c
+++ b/drivers/media/usb/em28xx/em28xx-i2c.c
@@ -76,18 +76,26 @@ static int em2800_i2c_send_bytes(struct em28xx *dev, u8 addr, u8 *buf, u16 len)
 	/* trigger write */
 	ret = dev->em28xx_write_regs(dev, 4 - len, &b2[4 - len], 2 + len);
 	if (ret != 2 + len) {
-		em28xx_warn("writing to i2c device failed (error=%i)\n", ret);
-		return -EIO;
+		em28xx_warn("failed to trigger write to i2c address 0x%x "
+			    "(error=%i)\n", addr, ret);
+		return (ret < 0) ? ret : -EIO;
 	}
 	/* wait for completion */
 	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
-		if (ret == 0x80 + len - 1)
+		if (ret == 0x80 + len - 1) {
 			return len;
+		} else if (ret == 0x94 + len - 1) {
+			return -ENODEV;
+		} else if (ret < 0) {
+			em28xx_warn("failed to get i2c transfer status from "
+				    "bridge register (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
 	}
-	em28xx_warn("i2c write timed out\n");
+	em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
 	return -EIO;
 }
 
@@ -168,24 +176,46 @@ static int em2800_i2c_check_for_device(struct em28xx *dev, u8 addr)
 static int em28xx_i2c_send_bytes(struct em28xx *dev, u16 addr, u8 *buf,
 				 u16 len, int stop)
 {
-	int wrcount = 0;
 	int write_timeout, ret;
 
 	if (len < 1 || len > 64)
 		return -EOPNOTSUPP;
 
-	wrcount = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
+	/* Write to i2c device */
+	ret = dev->em28xx_write_regs_req(dev, stop ? 2 : 3, addr, buf, len);
+	if (ret != len) {
+		if (ret < 0) {
+			em28xx_warn("writing to i2c device at 0x%x failed "
+				    "(error=%i)\n", addr, ret);
+			return ret;
+		} else {
+			em28xx_warn("%i bytes write to i2c device at 0x%x "
+				    "requested, but %i bytes written\n",
+				    len, addr, ret);
+			return -EIO;
+		}
+	}
 
-	/* Seems to be required after a write */
+	/* Check success of the i2c operation */
 	for (write_timeout = EM2800_I2C_XFER_TIMEOUT; write_timeout > 0;
 	     write_timeout -= 5) {
 		ret = dev->em28xx_read_reg(dev, 0x05);
-		if (!ret)
-			break;
+		if (ret == 0) { /* success */
+			return len;
+		} else if (ret == 0x10) {
+			return -ENODEV;
+		} else if (ret < 0) {
+			em28xx_warn("failed to read i2c transfer status from "
+				    "bridge (error=%i)\n", ret);
+			return ret;
+		}
 		msleep(5);
+		/* NOTE: do we really have to wait for success ?
+		   Never seen anything else than 0x00 or 0x10
+		   (even with high payload) ...			*/
 	}
-
-	return wrcount;
+	em28xx_warn("write to i2c device at 0x%x timed out\n", addr);
+	return -EIO;
 }
 
 /*
@@ -199,14 +229,37 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 	if (len < 1 || len > 64)
 		return -EOPNOTSUPP;
 
+	/* Read data from i2c device */
 	ret = dev->em28xx_read_reg_req_len(dev, 2, addr, buf, len);
+	if (ret != len) {
+		if (ret < 0) {
+			em28xx_warn("reading from i2c device at 0x%x failed "
+				    "(error=%i)\n", addr, ret);
+			return ret;
+		} else {
+			em28xx_warn("%i bytes requested from i2c device at "
+				    "0x%x, but %i bytes received\n",
+				    len, addr, ret);
+			return -EIO;
+		}
+	}
+
+	/* Check success of the i2c operation */
+	ret = dev->em28xx_read_reg(dev, 0x05);
 	if (ret < 0) {
-		em28xx_warn("reading i2c device failed (error=%i)\n", ret);
+		em28xx_warn("failed to read i2c transfer status from "
+			    "bridge (error=%i)\n", ret);
 		return ret;
 	}
-	if (dev->em28xx_read_reg(dev, 0x5) != 0)
-		return -ENODEV;
-	return ret;
+	if (ret > 0) {
+		if (ret == 0x10) {
+			return -ENODEV;
+		} else {
+			em28xx_warn("unknown i2c error (status=%i)\n", ret);
+			return -EIO;
+		}
+	}
+	return len;
 }
 
 /*
@@ -216,15 +269,12 @@ static int em28xx_i2c_recv_bytes(struct em28xx *dev, u16 addr, u8 *buf, u16 len)
 static int em28xx_i2c_check_for_device(struct em28xx *dev, u16 addr)
 {
 	int ret;
+	u8 buf;
 
-	ret = dev->em28xx_read_reg_req(dev, 2, addr);
-	if (ret < 0) {
-		em28xx_warn("reading from i2c device failed (error=%i)\n", ret);
-		return ret;
-	}
-	if (dev->em28xx_read_reg(dev, 0x5) != 0)
-		return -ENODEV;
-	return 0;
+	ret = em28xx_i2c_recv_bytes(dev, addr, &buf, 1);
+	if (ret == 1)
+		return 0;
+	return (ret < 0) ? ret : -EIO;
 }
 
 /*
@@ -249,11 +299,11 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 				rc = em2800_i2c_check_for_device(dev, addr);
 			else
 				rc = em28xx_i2c_check_for_device(dev, addr);
-			if (rc < 0) {
-				dprintk2(2, " no device\n");
+			if (rc == -ENODEV) {
+				if (i2c_debug >= 2)
+					printk(" no device\n");
 				return rc;
 			}
-
 		} else if (msgs[i].flags & I2C_M_RD) {
 			/* read bytes */
 			if (dev->board.is_em2800)
@@ -284,16 +334,16 @@ static int em28xx_i2c_xfer(struct i2c_adapter *i2c_adap,
 							   msgs[i].len,
 							   i == num - 1);
 		}
-		if (rc < 0)
-			goto err;
+		if (rc < 0) {
+			if (i2c_debug >= 2)
+				printk(" ERROR: %i\n", rc);
+			return rc;
+		}
 		if (i2c_debug >= 2)
 			printk("\n");
 	}
 
 	return num;
-err:
-	dprintk2(2, " ERROR: %i\n", rc);
-	return rc;
 }
 
 /* based on linux/sunrpc/svcauth.h and linux/hash.h
-- 
1.7.10.4