[PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
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
diff --git a/drivers/media/common/tuners/tda18271-common.c
b/drivers/media/common/tuners/tda18271-common.c
index e1f6782..195b30e 100644
--- a/drivers/media/common/tuners/tda18271-common.c
+++ b/drivers/media/common/tuners/tda18271-common.c
@@ -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;
diff --git a/drivers/media/common/tuners/tda18271.h
b/drivers/media/common/tuners/tda18271.h
index d7fcc36..3abb221 100644
--- a/drivers/media/common/tuners/tda18271.h
+++ b/drivers/media/common/tuners/tda18271.h
@@ -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 {
diff --git a/drivers/media/video/tuner-core.c b/drivers/media/video/tuner-core.c
index fa406b9..1a047c5 100644
--- a/drivers/media/video/tuner-core.c
+++ b/drivers/media/video/tuner-core.c
@@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
wrote:
> 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
>
> diff --git a/drivers/media/common/tuners/tda18271-common.c
> b/drivers/media/common/tuners/tda18271-common.c
> index e1f6782..195b30e 100644
> --- a/drivers/media/common/tuners/tda18271-common.c
> +++ b/drivers/media/common/tuners/tda18271-common.c
> @@ -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;
> diff --git a/drivers/media/common/tuners/tda18271.h
> b/drivers/media/common/tuners/tda18271.h
> index d7fcc36..3abb221 100644
> --- a/drivers/media/common/tuners/tda18271.h
> +++ b/drivers/media/common/tuners/tda18271.h
> @@ -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 {
> diff --git a/drivers/media/video/tuner-core.c
> b/drivers/media/video/tuner-core.c
> index fa406b9..1a047c5 100644
> --- a/drivers/media/video/tuner-core.c
> +++ b/drivers/media/video/tuner-core.c
> @@ -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
>
>
>
Adding the maintainer for the 18271 driver to the CC.
Devin
--
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to major
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab wrote: > 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 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. Regards, Mike Krufky -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
Hi Michael,
Em 30-09-2010 15:52, Michael Krufky escreveu:
> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
> wrote:
>> 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
>
> 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.
Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
On Thu, Sep 30, 2010 at 3:12 PM, Mauro Carvalho Chehab
wrote:
> Hi Michael,
>
> Em 30-09-2010 15:52, Michael Krufky escreveu:
>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>> wrote:
>>> 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
>>
>> 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.
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?
Regards,
Mike Krufky
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
Em 30-09-2010 16:18, Michael Krufky escreveu:
> On Thu, Sep 30, 2010 at 3:12 PM, Mauro Carvalho Chehab
> wrote:
>> Hi Michael,
>>
>> Em 30-09-2010 15:52, Michael Krufky escreveu:
>>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>>> wrote:
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
>>>
>>> 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.
>
> 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.
Maybe the cx231xx have different limits per I2C bus. The device I'm currently
handling uses I2C channel 2 (instead of channel 1) for the tuner. The only
device on this bus is tda18271. The original driver uses just one byte for every
transactions. On my tests, _all_ I2C transactions to i2c channel #2 with
wLength > 4 bytes fail.
So, I'm pretty confident that it won't support more than 4 bytes of payload.
> 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.
Well, if the device doesn't support large transactions, what other options do
we have?
> I'd just like to review some other options and retest this before we
> merge. Is that okay with you?
Yeah, sure.
Cheers,
Mauro.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
On 09/30/2010 09:52 PM, Michael Krufky wrote: On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab wrote: 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 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. regards, Antti -- http://palosaari.fi/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
On Thu, Sep 30, 2010 at 6:00 PM, Antti Palosaari wrote: > On 09/30/2010 09:52 PM, Michael Krufky wrote: >> >> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab >> wrote: >>> >>> 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 >> >> 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. 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. Thanks for the input :-) Regards, Mike Krufky -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes
Em 30-09-2010 19:07, Michael Krufky escreveu: > On Thu, Sep 30, 2010 at 6:00 PM, Antti Palosaari wrote: >> On 09/30/2010 09:52 PM, Michael Krufky wrote: >>> >>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab >>> wrote: 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 >>> >>> 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. > > 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. Feel free to test. Just remember that all Hauppauge devices at Devin's polaris4 tree uses I2C Channel #1. This seems to be the default on all Conexant SDK also. The device I'm working with has the tuner at channel #2, and the ISDB-T demod at channel #1. So, the limits may differ. On my device, everything seem to work fine with max size = 4 (both analog and digital modes). Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
