V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-common.c +34 -25

index e1f6782..195b30e 100644
@@ -193,20 +193,46 @@ int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
        unsigned char *regs = priv->tda18271_regs;
        unsigned char buf[TDA18271_NUM_REGS + 1];
        struct i2c_msg msg = { .addr = priv->i2c_props.addr, .flags = 0,
-                              .buf = buf, .len = len + 1 };
-       int i, ret;
+                              .buf = buf };
+       int i, ret = 1, max;
 
        BUG_ON((len == 0) || (idx + len > sizeof(buf)));
 
-       buf[0] = idx;
-       for (i = 1; i <= len; i++)
-               buf[i] = regs[idx - 1 + i];
+
+       switch (priv->small_i2c) {
+       case TDA18271_03_BYTE_CHUNK_INIT:
+               max = 3;
+               break;
+       case TDA18271_08_BYTE_CHUNK_INIT:
+               max = 8;
+               break;
+       case TDA18271_16_BYTE_CHUNK_INIT:
+               max = 16;
+               break;
+       case TDA18271_39_BYTE_CHUNK_INIT:
+       default:
+               max = 39;
+       }
 
        tda18271_i2c_gate_ctrl(fe, 1);
+       while (len) {
+               if (max > len)
+                       max = len;
 
-       /* write registers */
-       ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               buf[0] = idx;
+               for (i = 1; i <= max; i++)
+                       buf[i] = regs[idx - 1 + i];
 
+               msg.len = max + 1;
+
+               /* write registers */
+               ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
+               if (ret != 1)
+                       break;
+
+               idx += max;
+               len -= max;
+       }
        tda18271_i2c_gate_ctrl(fe, 0);
 
        if (ret != 1)

@@ -326,24 +352,7 @@ int tda18271_init_regs(struct dvb_frontend *fe)
        regs[R_EB22] = 0x48;
        regs[R_EB23] = 0xb0;
 
-       switch (priv->small_i2c) {
-       case TDA18271_08_BYTE_CHUNK_INIT:
-               tda18271_write_regs(fe, 0x00, 0x08);
-               tda18271_write_regs(fe, 0x08, 0x08);
-               tda18271_write_regs(fe, 0x10, 0x08);
-               tda18271_write_regs(fe, 0x18, 0x08);
-               tda18271_write_regs(fe, 0x20, 0x07);
-               break;
-       case TDA18271_16_BYTE_CHUNK_INIT:
-               tda18271_write_regs(fe, 0x00, 0x10);
-               tda18271_write_regs(fe, 0x10, 0x10);
-               tda18271_write_regs(fe, 0x20, 0x07);
-               break;
-       case TDA18271_39_BYTE_CHUNK_INIT:
-       default:
-               tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
-               break;
-       }
+       tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
 
        /* setup agc1 gain */
        regs[R_EB17] = 0x00;

drivers/media/common/tuners/tda18271.h +3 -2

index d7fcc36..3abb221 100644
@@ -80,8 +80,9 @@ enum tda18271_output_options {
 
  enum tda18271_small_i2c {
        TDA18271_39_BYTE_CHUNK_INIT = 0,
-       TDA18271_16_BYTE_CHUNK_INIT = 1,
-       TDA18271_08_BYTE_CHUNK_INIT = 2,
+       TDA18271_16_BYTE_CHUNK_INIT = 16,
+       TDA18271_08_BYTE_CHUNK_INIT = 8,
+       TDA18271_03_BYTE_CHUNK_INIT = 3,
  };
 
  struct tda18271_config {

drivers/media/video/tuner-core.c +1 -2

index fa406b9..1a047c5 100644
@@ -428,7 +428,7 @@ static void set_type(struct i2c_client *c, unsigned int type,
        {
                struct tda18271_config cfg = {
                        .config = t->config,
-                       .small_i2c = TDA18271_08_BYTE_CHUNK_INIT,
+                       .small_i2c = TDA18271_03_BYTE_CHUNK_INIT,
                };
 
                if (!dvb_attach(tda18271_attach, &t->fe, t->i2c->addr,
--

1.7.1

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported. [   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272 [   82.600530] tda18271 4-0060: destroying instance

Print:
[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +1 -2

index 7955e49..77e3642 100644
@@ -1177,7 +1177,7 @@ static int tda18271_get_id(struct dvb_frontend *fe)
                break;
        }
 
-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s (%i) detected @ %d-%04x%s\n", name, regs[R_ID] & 0x7f,
                 i2c_adapter_id(priv->i2c_props.adap),
                 priv->i2c_props.addr,
                 (0 == ret) ? "" : ", device not supported.");
--

1.7.1

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-common.c +7 -0

index 195b30e..7ba3ba3 100644
@@ -549,6 +549,13 @@ int tda18271_calc_main_pll(struct dvb_frontend *fe, u32 freq)
        regs[R_MD1]   = 0x7f & (div >> 16);
        regs[R_MD2]   = 0xff & (div >> 8);
        regs[R_MD3]   = 0xff & div;
+
+       if (tda18271_debug & DBG_REG) {
+               tda_reg("MAIN_DIV_BYTE_1    = 0x%02x\n", 0xff & regs[R_MD1]);
+               tda_reg("MAIN_DIV_BYTE_2    = 0x%02x\n", 0xff & regs[R_MD2]);
+               tda_reg("MAIN_DIV_BYTE_3    = 0x%02x\n", 0xff & regs[R_MD3]);
+       }
+

 fail:
       return ret;
 }
--
1.7.1

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported. [   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272 [   82.600530] tda18271 4-0060: destroying instance

Print:
[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +0 -0

index 7955e49..77e3642 100644

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-common.c +7 -0

index 195b30e..7ba3ba3 100644
@@ -549,6 +549,13 @@ int tda18271_calc_main_pll(struct dvb_frontend *fe, u32 freq)
        regs[R_MD1]   = 0x7f & (div >> 16);
        regs[R_MD2]   = 0xff & (div >> 8);
        regs[R_MD3]   = 0xff & div;
+
+       if (tda18271_debug & DBG_REG) {
+               tda_reg("MAIN_DIV_BYTE_1    = 0x%02x\n", 0xff & regs[R_MD1]);
+               tda_reg("MAIN_DIV_BYTE_2    = 0x%02x\n", 0xff & regs[R_MD2]);
+               tda_reg("MAIN_DIV_BYTE_3    = 0x%02x\n", 0xff & regs[R_MD3]);
+       }
+

 fail:
       return ret;
 }

Quoted 10 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

This looks to me as if it is working around a problem on the i2c master. I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver. It seems to be a band-aid to cover up a problem in the i2c master device driver code.

Quoted 23 lines · Mauro Carvalho Chehab

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported.
[   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272
[   82.600530] tda18271 4-0060: destroying instance

Print:

[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +0 -0

index 7955e49..77e3642 100644
@@ -1177,7 +1177,7 @@ static int tda18271_get_id(struct dvb_frontend *fe)
               break;
       }
 

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1183

Quoted 5 lines

drivers/media/common/tuners/tda18271-fe.c +1 -1

-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s (%i) detected @ %d-%04x%s\n", name, regs[R_ID] & 0x7f,
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr,
                (0 == ret) ? "" : ", device not supported.");

A patch like this is fine for testing, but I see no reason for merging this into the kernel. Can you provide an explaination as per why this would be useful? In general, if you see, "Unknown device detected @ X-00YY, device not supported." then it means that this is not a tda182x1.

Quoted 18 lines · Mauro Carvalho Chehab

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-common.c +7 -0

index 195b30e..7ba3ba3 100644
@@ -549,6 +549,13 @@ int tda18271_calc_main_pll(struct dvb_frontend *fe, u32 freq)
       regs[R_MD1]   = 0x7f & (div >> 16);
       regs[R_MD2]   = 0xff & (div >> 8);
       regs[R_MD3]   = 0xff & div;
+
+       if (tda18271_debug & DBG_REG) {
+               tda_reg("MAIN_DIV_BYTE_1    = 0x%02x\n", 0xff & regs[R_MD1]);
+               tda_reg("MAIN_DIV_BYTE_2    = 0x%02x\n", 0xff & regs[R_MD2]);
+               tda_reg("MAIN_DIV_BYTE_3    = 0x%02x\n", 0xff & regs[R_MD3]);
+       }
+
 fail:

drivers/media/common/tuners/tda18271-common.c int tda18271_calc_main_pll(struct dvb_frontend *fe, u32 freq) · line 561

Quoted 2 lines

drivers/media/common/tuners/tda18271-common.c +0 -0

       return ret;
 }

I would actually prefer NOT to merge this - it is redundant. When DBG_REG is enabled, the driver will dump the contents of all registers, including MD1, MD2 and MD3. With this patch applied, it would dump this data twice. I do not believe this is useful at all.

Regards,

Mike Krufky

Hi Michael,

Em 30-09-2010 15:52, Michael Krufky escreveu:

Quoted 13 lines

Quoted 10 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 8 lines

This looks to me as if it is working around a problem on the i2c master.  I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver.  It seems to be a band-aid to cover up a problem in the i2c master device driver code.

In the specific case of cx231xx the hardware don't support any I2C transactions with more than 4 bytes. It is common to find this kind of limit on other types of hardware as well.

In the specific case of tda18271, the workaround for tda18271 is already there:

enum tda18271_small_i2c {
       TDA18271_39_BYTE_CHUNK_INIT = 0,
       TDA18271_16_BYTE_CHUNK_INIT = 1,
       TDA18271_08_BYTE_CHUNK_INIT = 2,
};
(from upstream tda18271.h header)

Yet, it is currently broken. In thesis, if you use small_i2c, it should limit the transactions size to a certain value, but, if you try to limit to 8, you'll still see some transactions with size=11, since the code that honors small_i2c only works for the initial dump of the 39 registers, as there are some cases where writes to EP3 registers are done with:
       tda18271_write_regs(fe, R_EP3, 11);

and the current code for tda18271_write_regs don't check it.

So, if there's a code there that allows limiting small_i2c:
       1) this code should work for all transactions, not only to the initial init;
       2) if there is such code, why not allow specifying a max size of 4 bytes?

Another aspect is that doing such kind or restriction at the i2c adapter would require a very ugly logic, as some devices like tda18271 have only 8 bits registers per i2c address, and a write (or read) with length > 1 byte update/read the next consecutive registers, while other devices like xc3028 have one single I2C address for multi-byte operations like updating the firmware.

If this logic would be moved into the bridge drivers, they would need to have some ugly tests, checking for each specific i2c sub-driver at their core drivers.

Em 30-09-2010 15:57, Michael Krufky escreveu:

Quoted 25 lines

Quoted 23 lines · Mauro Carvalho Chehab

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported. [   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272 [   82.600530] tda18271 4-0060: destroying instance

Print:
[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +0 -0

index 7955e49..77e3642 100644
@@ -1177,7 +1177,7 @@ static int tda18271_get_id(struct dvb_frontend *fe)
                break;
        }
 

Quoted 11 lines

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1183

Quoted 5 lines

drivers/media/common/tuners/tda18271-fe.c +1 -1

-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s (%i) detected @ %d-%04x%s\n", name, regs[R_ID] & 0x7f,
                 i2c_adapter_id(priv->i2c_props.adap),
                 priv->i2c_props.addr,
                 (0 == ret) ? "" : ", device not supported.");

A patch like this is fine for testing, but I see no reason for merging this into the kernel.  Can you provide an explaination as per why this would be useful?  In general, if you see, "Unknown device detected @ X-00YY, device not supported." then it means that this is not a tda182x1.

cx231xx have 4 I2C buses. The device I'm working with have the tuner at the wrong chip. As it doesn't support 0 byte transactions, if you try to read from the wrong i2c, it will just return 0 to all read requests.

So, this kind of message can be very useful if someone sends us a report about a new device. The changes are small and are printed only in the case of errors, where people will likely try to reach the developers. So, I think it is a good idea to have it mainstream.

Quoted 51 lines · Mauro Carvalho Chehab

Hi Michael,

Em 30-09-2010 15:52, Michael Krufky escreveu:

Quoted 13 lines

Quoted 10 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Quoted 8 lines

This looks to me as if it is working around a problem on the i2c master. I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver. It seems to be a band-aid to cover up a problem in the i2c master device driver code.

In the specific case of cx231xx the hardware don't support any I2C transactions with more than 4 bytes. It is common to find this kind of limit on other types of hardware as well.

In the specific case of tda18271, the workaround for tda18271 is already there:

enum tda18271_small_i2c {
       TDA18271_39_BYTE_CHUNK_INIT = 0,
       TDA18271_16_BYTE_CHUNK_INIT = 1,
       TDA18271_08_BYTE_CHUNK_INIT = 2,
};

(from upstream tda18271.h header)

Yet, it is currently broken. In thesis, if you use small_i2c, it should limit the transactions size to a certain value, but, if you try to limit to 8, you'll still see some transactions with size=11, since the code that honors small_i2c only works for the initial dump of the 39 registers, as there are some cases where writes to EP3 registers are done with:

       tda18271_write_regs(fe, R_EP3, 11);

and the current code for tda18271_write_regs don't check it.

So, if there's a code there that allows limiting small_i2c:
1) this code should work for all transactions, not only to the initial init;
2) if there is such code, why not allow specifying a max size of 4 bytes?

Quoted 8 lines

Another aspect is that doing such kind or restriction at the i2c adapter would require a very ugly logic, as some devices like tda18271 have only 8 bits registers per i2c address, and a write (or read) with length > 1 byte update/read the next consecutive registers, while other devices like xc3028 have one single I2C address for multi-byte operations like updating the firmware.

If this logic would be moved into the bridge drivers, they would need to have some ugly tests, checking for each specific i2c sub-driver at their core drivers.

Hmm... If you don't mind, would you allow me to think about this a bit, and run some tests of my own?

I have already seen the tda18271 work properly on the cx231xx with 8 byte transactions, the issue that I saw was actually a 12-byte transaction limit, so the 11 byte transfer didn't cause a problem. I'd like to test this again myself using the cx231xx device that I have on hand.

However, this transaction in particular:

tda18271_write_regs(fe, R_EP3, 11);

...is not supposed to be broken down to smaller transaction -- this particular transaction is supposed to be a one shot change to all 11 regs at once. Straying from this could result in unpredictable behavior.

I'd just like to review some other options and retest this before we merge. Is that okay with you?

Quoted 38 lines · Mauro Carvalho Chehab

Em 30-09-2010 15:57, Michael Krufky escreveu:

Quoted 25 lines

Quoted 23 lines · Mauro Carvalho Chehab

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported.
[   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272
[   82.600530] tda18271 4-0060: destroying instance

Print:

[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +0 -0

index 7955e49..77e3642 100644
@@ -1177,7 +1177,7 @@ static int tda18271_get_id(struct dvb_frontend *fe)
               break;
       }
 

Quoted 11 lines

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1183

Quoted 5 lines

drivers/media/common/tuners/tda18271-fe.c +1 -1

-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s (%i) detected @ %d-%04x%s\n", name, regs[R_ID] & 0x7f,
                i2c_adapter_id(priv->i2c_props.adap),
                priv->i2c_props.addr,
                (0 == ret) ? "" : ", device not supported.");

A patch like this is fine for testing, but I see no reason for merging this into the kernel. Can you provide an explaination as per why this would be useful? In general, if you see, "Unknown device detected @ X-00YY, device not supported." then it means that this is not a tda182x1.

Quoted 7 lines

cx231xx have 4 I2C buses. The device I'm working with have the tuner at the wrong chip. As it doesn't support 0 byte transactions, if you try to read from the wrong i2c, it will just return 0 to all read requests.

So, this kind of message can be very useful if someone sends us a report about a new device. The changes are small and are printed only in the case of errors, where people will likely try to reach the developers. So, I think it is a good idea to have it mainstream.

Mauro,

I think that's a reasonable explanation. Would you be open to reworking the patch such that the register contents only show up if the device is not recognized? (when ret < 0) . In the case where the device is correctly identified (ret == 0), I'd rather preserve the original successful detection message, and not see the ID register contents.

Mauro,

I think that's a reasonable explanation. Would you be open to reworking the patch such that the register contents only show up if the device is not recognized? (when ret < 0) . In the case where the device is correctly identified (ret == 0), I'd rather preserve the original successful detection message, and not see the ID register contents.

Em 30-09-2010 16:27, Michael Krufky escreveu:

Quoted 8 lines

Mauro,

I think that's a reasonable explanation.  Would you be open to reworking the patch such that the register contents only show up if the device is not recognized?  (when ret < 0) . In the case where the device is correctly identified (ret == 0), I'd rather preserve the original successful detection message, and not see the ID register contents.

Patch enclosed.

---

[PATCH] V4L/DVB: tda18271: Add some hint about what tda18217 reg ID returned

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported. [   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272 [   82.600530] tda18271 4-0060: destroying instance

Print:
[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

This helps to detect when the driver is using the wrong I2C bus (or have the i2g gate switch pointing to the wrong place), on devices like cx231xx that just return 0 on reads to a non-existent i2c device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +0 -1

index 7955e49..3db8727 100644
@@ -1156,7 +1156,6 @@ static int tda18271_get_id(struct dvb_frontend *fe)
        struct tda18271_priv *priv = fe->tuner_priv;
        unsigned char *regs = priv->tda18271_regs;
        char *name;
-       int ret = 0;
 
        mutex_lock(&priv->lock);
        tda18271_read_regs(fe);

@@ -1172,17 +1171,18 @@ static int tda18271_get_id(struct dvb_frontend *fe)
                priv->id = TDA18271HDC2;
                break;

       default:
-               name = "Unknown device";
-               ret = -EINVAL;
-               break;
+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID],
+                        i2c_adapter_id(priv->i2c_props.adap),
+                        priv->i2c_props.addr);
+               return -EINVAL;
       }

-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s detected @ %d-%04x\n", name, i2c_adapter_id(priv->i2c_props.adap),
-                priv->i2c_props.addr,
-                (0 == ret) ? "" : ", device not supported.");
+                priv->i2c_props.addr);

-       return ret;
+       return 0;
 }

 static int tda18271_setup_configuration(struct dvb_frontend *fe,

+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID],
+                        i2c_adapter_id(priv->i2c_props.adap),
+                        priv->i2c_props.addr);
+               return -EINVAL;

+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID], i2c_adapter_id(priv->i2c_props.adap), priv->i2c_props.addr);
+               return -EINVAL;

Quoted 10 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab<[email protected]>

This looks to me as if it is working around a problem on the i2c master. I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver. It seems to be a band-aid to cover up a problem in the i2c master device driver code.

Quoted 15 lines · Michael Krufky

Quoted 11 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab<[email protected]>

Quoted 8 lines

This looks to me as if it is working around a problem on the i2c master.  I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver.  It seems to be a band-aid to cover up a problem in the i2c master device driver code.

Yes it is I2C provider limitation, but I think almost all I2C adapters have some limit. I suggest to set param for each tuner and demod driver which splits reads and writes to len adapter can handle. I did that for tda18218 write.

But there is one major point you don't see. It is not simple to add this splitting limit to the provider. Provider does not have knowledge which is meaning of bytes it transfers to the bus. Without knowledge it breaks functionality surely in some point. There is commonly seen 1, 2 and 4 byte register address and same for register values. Also some chips like to send data as register-value pairs.

Quoted 30 lines · Michael Krufky

Quoted 15 lines

Quoted 11 lines · Mauro Carvalho Chehab

By default, tda18271 tries to optimize I2C bus by updating all registers at the same time. Unfortunately, some devices doesn't support it.

The current logic has a problem when small_i2c is equal to 8, since there are some transfers using 11 + 1 bytes.

Fix the problem by enforcing the max size at the right place, and allows reducing it to max = 3 + 1.

Signed-off-by Mauro Carvalho Chehab<[email protected]>

Quoted 8 lines

This looks to me as if it is working around a problem on the i2c master. I believe that a fix like this really belongs in the i2c master driver, it should be able to break the i2c transactions down into transactions that the i2c master can handle.

I wouldn't want to merge this without a better explanation of why it is necessary in the tda18271 driver. It seems to be a band-aid to cover up a problem in the i2c master device driver code.

Yes it is I2C provider limitation, but I think almost all I2C adapters have some limit. I suggest to set param for each tuner and demod driver which splits reads and writes to len adapter can handle. I did that for tda18218 write.

Quoted 6 lines

But there is one major point you don't see. It is not simple to add this splitting limit to the provider. Provider does not have knowledge which is meaning of bytes it transfers to the bus. Without knowledge it breaks functionality surely in some point. There is commonly seen 1, 2 and 4 byte register address and same for register values. Also some chips like to send data as register-value pairs.

Yes, I understand. We will likely merge Mauro's patch, I just want to test it on my own hardware first, and I'd like to verify the cx231xx i2c xfer limit issue that Mauro is reporting. I'll try to get back to this early next week, or hopefully over this weekend.

Quoted 65 lines · Mauro Carvalho Chehab

Em 30-09-2010 16:27, Michael Krufky escreveu:

Quoted 8 lines

Mauro,

I think that's a reasonable explanation. Would you be open to reworking the patch such that the register contents only show up if the device is not recognized? (when ret < 0) . In the case where the device is correctly identified (ret == 0), I'd rather preserve the original successful detection message, and not see the ID register contents.

Patch enclosed.

---

[PATCH] V4L/DVB: tda18271: Add some hint about what tda18217 reg ID returned

Instead of doing:

[   82.581639] tda18271 4-0060: creating new instance
[   82.588411] Unknown device detected @ 4-0060, device not supported.
[   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272
[   82.600530] tda18271 4-0060: destroying instance

Print:

[  468.740392] Unknown device (0) detected @ 4-0060, device not supported.

for the error message, to help detecting what's going wrong with the device.

This helps to detect when the driver is using the wrong I2C bus (or have the i2g gate switch pointing to the wrong place), on devices like cx231xx that just return 0 on reads to a non-existent i2c device.

Signed-off-by Mauro Carvalho Chehab <[email protected]>

drivers/media/common/tuners/tda18271-fe.c +6 -5

index 7955e49..3db8727 100644
@@ -1156,7 +1156,6 @@ static int tda18271_get_id(struct dvb_frontend *fe)
       struct tda18271_priv *priv = fe->tuner_priv;
       unsigned char *regs = priv->tda18271_regs;
       char *name;
-       int ret = 0;
 
       mutex_lock(&priv->lock);
       tda18271_read_regs(fe);

@@ -1172,17 +1171,18 @@ static int tda18271_get_id(struct dvb_frontend *fe)
               priv->id = TDA18271HDC2;
               break;
       default:
-               name = "Unknown device";
-               ret = -EINVAL;
-               break;
+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID],
+                        i2c_adapter_id(priv->i2c_props.adap),
+                        priv->i2c_props.addr);
+               return -EINVAL;
       }
 
-       tda_info("%s detected @ %d-%04x%s\n", name,
+       tda_info("%s detected @ %d-%04x\n", name,

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1188

Quoted 10 lines

drivers/media/common/tuners/tda18271-fe.c +2 -3

                i2c_adapter_id(priv->i2c_props.adap),
-                priv->i2c_props.addr,
-                (0 == ret) ? "" : ", device not supported.");
+                priv->i2c_props.addr);
 
-       return ret;
+       return 0;
 }
 
 static int tda18271_setup_configuration(struct dvb_frontend *fe,

This looks good to me, although you could concatenate these lines together:

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1178

Quoted 5 lines

drivers/media/common/tuners/tda18271-fe.c +5 -0

+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID],
+                        i2c_adapter_id(priv->i2c_props.adap),
+                        priv->i2c_props.addr);
+               return -EINVAL;

to be more like this:

drivers/media/common/tuners/tda18271-fe.c static int tda18271_get_id(struct dvb_frontend *fe) · line 1178

Quoted 3 lines

drivers/media/common/tuners/tda18271-fe.c +3 -0

+               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
+                        regs[R_ID], i2c_adapter_id(priv->i2c_props.adap), priv->i2c_props.addr);
+               return -EINVAL;

...that is, if it fits within an 80-char line.

Anyway,

Reviewed-by Michael Krufky <[email protected]>

drivers/media/video/cx231xx/cx231xx-avcore.c:1608: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’ drivers/media/video/cx231xx/cx231xx-417.c:1047: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’

Signed-off-by Mauro Carvalho Chehab <[email protected]>

Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:

Quoted 4 lines

drivers/media/video/cx231xx/cx231xx-avcore.c:1608: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’ drivers/media/video/cx231xx/cx231xx-417.c:1047: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’

Signed-off-by Mauro Carvalho Chehab <[email protected]>

OK, I just updated my tree with the patches that Mkrufky acked. It basically contains the same patches from my previous post, plus the patches that Palash sent, and Devin/Mkrufky patches from polaris4 tree, rebased over the top of kernel v2.6.36-rc7 (this makes easier for me to test and to merge).

The patches are at:
       http://git.linuxtv.org/mchehab/cx231xx.git

Sri already sent his ack for the first series of the patches.

The tree contains two extra patches:

1) a cx231xx large CodingStyle fix patch:
       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=eacd1a7749ae45d1f2f5782c013b863ff480746d

It basically solves the issues that checkpatch.pl complained on this series of patches;

2) a cx231xx-417 gcc warning fix:
       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=ca3a6a8c2a4819702e93b9612c4a6d90474ea9b5

Devin,

Would it be ok for you if I merge them on my main tree? They're needed for one board I'm working with (a Pixelview SBTVD Hybrid - that supports both analog and full-seg ISDB-T).

Quoted 27 lines · Mauro Carvalho Chehab

Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:

Quoted 4 lines

drivers/media/video/cx231xx/cx231xx-avcore.c:1608: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’ drivers/media/video/cx231xx/cx231xx-417.c:1047: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’

Signed-off-by Mauro Carvalho Chehab <[email protected]>

OK, I just updated my tree with the patches that Mkrufky acked. It basically contains the same patches from my previous post, plus the patches that Palash sent, and Devin/Mkrufky patches from polaris4 tree, rebased over the top of kernel v2.6.36-rc7 (this makes easier for me to test and to merge).

The patches are at:

       http://git.linuxtv.org/mchehab/cx231xx.git

Sri already sent his ack for the first series of the patches.

The tree contains two extra patches:

1) a cx231xx large CodingStyle fix patch:

       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=eacd1a7749ae45d1f2f5782c013b863ff480746d

It basically solves the issues that checkpatch.pl complained on this series of patches;

2) a cx231xx-417 gcc warning fix:

       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=ca3a6a8c2a4819702e93b9612c4a6d90474ea9b5

Quoted 5 lines

Devin,

Would it be ok for you if I merge them on my main tree? They're needed for one board I'm working with (a Pixelview SBTVD Hybrid - that supports both analog and full-seg ISDB-T).

Yeah, I've got additional fixes which aren't on that tree yet, but I don't see any reason why what's there cannot be merged.

It would be helpful if you could get Douglas to merge both sets of patches (mine and yours) to the hg backport tree as well, so I can continue development without requiring the bleeding edge kernel (all the work going on is for an embedded target which is running a relatively old kernel).

I've got another couple dozen patches and I'm willing to continue pushing this stuff upstream, but you need to meet me halfway here by not making the bleeding edge kernel a requirement for this work.

Quoted 27 lines · Mauro Carvalho Chehab

Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:

Quoted 4 lines

drivers/media/video/cx231xx/cx231xx-avcore.c:1608: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘long unsigned int’ drivers/media/video/cx231xx/cx231xx-417.c:1047: warning: format ‘%d’ expects type ‘int’, but argument 3 has type ‘size_t’

Signed-off-by Mauro Carvalho Chehab <[email protected]>

OK, I just updated my tree with the patches that Mkrufky acked. It basically contains the same patches from my previous post, plus the patches that Palash sent, and Devin/Mkrufky patches from polaris4 tree, rebased over the top of kernel v2.6.36-rc7 (this makes easier for me to test and to merge).

The patches are at:

       http://git.linuxtv.org/mchehab/cx231xx.git

Sri already sent his ack for the first series of the patches.

The tree contains two extra patches:

1) a cx231xx large CodingStyle fix patch:

       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=eacd1a7749ae45d1f2f5782c013b863ff480746d

It basically solves the issues that checkpatch.pl complained on this series of patches;

2) a cx231xx-417 gcc warning fix:

       http://git.linuxtv.org/mchehab/cx231xx.git?a=commit;h=ca3a6a8c2a4819702e93b9612c4a6d90474ea9b5

Quoted 5 lines

Devin,

Would it be ok for you if I merge them on my main tree? They're needed for one board I'm working with (a Pixelview SBTVD Hybrid - that supports both analog and full-seg ISDB-T).

Yeah, I've got additional fixes which aren't on that tree yet, but I don't see any reason why what's there cannot be merged.