[PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[Mauro Carvalho Chehab]

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 <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-417.c b/drivers/media/video/cx231xx/cx231xx-417.c
index 402e109..ddaa437 100644
--- a/drivers/media/video/cx231xx/cx231xx-417.c
+++ b/drivers/media/video/cx231xx/cx231xx-417.c
@@ -1044,7 +1044,7 @@ static int cx231xx_load_firmware(struct cx231xx *dev)
 	/* transfer to the chip */
 	dprintk(2, "Loading firmware to GPIO...\n");
 	p_fw_data = (u32 *)firmware->data;
-	dprintk(2, "firmware->size=%d\n", firmware->size);
+	dprintk(2, "firmware->size=%zd\n", firmware->size);
 	for (transfer_size = 0; transfer_size < firmware->size;
 		 transfer_size += 4) {
 		fw_data = *p_fw_data;
diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index b4eda90..ab9fbf8 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -1605,7 +1605,7 @@ void cx231xx_set_DIF_bandpass(struct cx231xx *dev, u32 if_freq,
 	if_freq = 16000000;
     }
 
-    cx231xx_info("Enter IF=%d\n",
+    cx231xx_info("Enter IF=%zd\n",
 		 sizeof(Dif_set_array)/sizeof(struct dif_settings));
     for (i = 0; i < sizeof(Dif_set_array)/sizeof(struct dif_settings); i++) {
 	if (Dif_set_array[i].if_freq == if_freq) {
-- 
1.7.1

Re: [PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[Mauro Carvalho Chehab]

Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:
> 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 <mchehab@redhat.com>

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

Thanks,
Mauro.

Re: [PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[Devin Heitmueller]

On Thu, Oct 7, 2010 at 5:48 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:
>> 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 <mchehab@redhat.com>
>
> 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).

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.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[Mauro Carvalho Chehab]

Em 07-10-2010 19:04, Devin Heitmueller escreveu:
> On Thu, Oct 7, 2010 at 5:48 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:
>>> 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 <mchehab@redhat.com>
>>
>> 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).
> 
> 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.

Ok, thanks!

> 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'm c/c Douglas on this email. Douglas had an ugent issue that required him
to travel abroad. He just returned yesterday night. Let's hope he'll find some
time to sync -hg with -git and backport those patches.

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

Ok.

Cheers,
Mauro

Re: [PATCH 01/10] V4L/DVB: cx231xx: remove a printk warning at -avcore and at -417

[Mauro Carvalho Chehab]

Em 07-10-2010 19:04, Devin Heitmueller escreveu:
> On Thu, Oct 7, 2010 at 5:48 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 28-09-2010 15:46, Mauro Carvalho Chehab escreveu:
>>> 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 <mchehab@redhat.com>
>>
>> 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).
> 
> 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.

Applied, thanks! Thanks to the compilation tests I do here, I discovered some
duplicated symbols between cx23885 and cx231xx-417. I just fixed the errors.
Patches were post to the ML and were also applied to the tree, to avoid compilation
breakages.

Cheers,
Mauro.

[PATCH 02/10] V4L/DVB: cx231xx: fix Kconfig dependencies

[Mauro Carvalho Chehab]

ERROR: "cx2341x_mpeg_ctrls" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_fill_defaults" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_log_status" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_ctrl_get_menu" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_update" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_ctrl_query" [drivers/media/video/cx231xx/cx231xx.ko] undefined!
ERROR: "cx2341x_ext_ctrls" [drivers/media/video/cx231xx/cx231xx.ko] undefined!

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/Kconfig b/drivers/media/video/cx231xx/Kconfig
index 5ac7ece..bb04914 100644
--- a/drivers/media/video/cx231xx/Kconfig
+++ b/drivers/media/video/cx231xx/Kconfig
@@ -6,6 +6,7 @@ config VIDEO_CX231XX
 	depends on VIDEO_IR
 	select VIDEOBUF_VMALLOC
 	select VIDEO_CX25840
+	select VIDEO_CX2341X
 
 	---help---
 	  This is a video4linux driver for Conexant 231xx USB based TV cards.
-- 
1.7.1

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

[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 <mchehab@redhat.com>

diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
index 7955e49..77e3642 100644
--- a/drivers/media/common/tuners/tda18271-fe.c
+++ b/drivers/media/common/tuners/tda18271-fe.c
@@ -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

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

[Devin Heitmueller]

On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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 <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
> index 7955e49..77e3642 100644
> --- a/drivers/media/common/tuners/tda18271-fe.c
> +++ b/drivers/media/common/tuners/tda18271-fe.c
> @@ -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
>
>
>

Adding the maintainer for the 18271 driver to the CC.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

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

[Michael Krufky]

On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> 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 <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
> index 7955e49..77e3642 100644
> --- a/drivers/media/common/tuners/tda18271-fe.c
> +++ b/drivers/media/common/tuners/tda18271-fe.c
> @@ -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.");

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.

Regards,

Mike Krufky

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

[Mauro Carvalho Chehab]

Em 30-09-2010 15:57, Michael Krufky escreveu:
> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> 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 <mchehab@redhat.com>
>>
>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>> index 7955e49..77e3642 100644
>> --- a/drivers/media/common/tuners/tda18271-fe.c
>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>> @@ -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.");
> 
> 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.

Cheers,
Mauro

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

[Michael Krufky]

On Thu, Sep 30, 2010 at 3:16 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 30-09-2010 15:57, Michael Krufky escreveu:
>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> wrote:
>>> 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 <mchehab@redhat.com>
>>>
>>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>>> index 7955e49..77e3642 100644
>>> --- a/drivers/media/common/tuners/tda18271-fe.c
>>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>>> @@ -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.");
>>
>> 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.

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.

Thanks & regards,

Mike Krufky

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

[Mauro Carvalho Chehab]

Em 30-09-2010 16:27, Michael Krufky escreveu:
> On Thu, Sep 30, 2010 at 3:16 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 30-09-2010 15:57, Michael Krufky escreveu:
>>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>>> <mchehab@redhat.com> wrote:
>>>> 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 <mchehab@redhat.com>
>>>>
>>>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>>>> index 7955e49..77e3642 100644
>>>> --- a/drivers/media/common/tuners/tda18271-fe.c
>>>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>>>> @@ -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.");
>>>
>>> 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.
> 
> 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.

Ok, sure. I'll do it.

Thanks,
Mauro.

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

[Mauro Carvalho Chehab]

Em 30-09-2010 16:27, Michael Krufky escreveu:
> 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 <mchehab@redhat.com>

diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
index 7955e49..3db8727 100644
--- a/drivers/media/common/tuners/tda18271-fe.c
+++ b/drivers/media/common/tuners/tda18271-fe.c
@@ -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,

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

[Michael Krufky]

On Thu, Sep 30, 2010 at 4:26 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 30-09-2010 16:27, Michael Krufky escreveu:
>> 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 <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
> index 7955e49..3db8727 100644
> --- a/drivers/media/common/tuners/tda18271-fe.c
> +++ b/drivers/media/common/tuners/tda18271-fe.c
> @@ -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,
>
>

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


> +               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:


> +               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 <mkrufky@kernellabs.com>

Thank you.

Regards,

Mike Krufky

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

[Mauro Carvalho Chehab]

Em 30-09-2010 18:03, Michael Krufky escreveu:
> On Thu, Sep 30, 2010 at 4:26 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Em 30-09-2010 16:27, Michael Krufky escreveu:
>>> 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 <mchehab@redhat.com>
>>
>> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
>> index 7955e49..3db8727 100644
>> --- a/drivers/media/common/tuners/tda18271-fe.c
>> +++ b/drivers/media/common/tuners/tda18271-fe.c
>> @@ -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,
>>
>>
> 
> This looks good to me, although you could concatenate these lines together:
> 
> 
>> +               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:
> 
> 
>> +               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.

All parameters after the format string don't fit on 80 cols. I did a small optimization on that.
> 
> Anyway,
> 
> Reviewed-by: Michael Krufky <mkrufky@kernellabs.com>

Thanks, committed.

Cheers,
Mauro.

[PATCH 04/10] V4L/DVB: cx231xx: properly implement URB control messages log

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-core.c b/drivers/media/video/cx231xx/cx231xx-core.c
index 5406ff2..983b120 100644
--- a/drivers/media/video/cx231xx/cx231xx-core.c
+++ b/drivers/media/video/cx231xx/cx231xx-core.c
@@ -47,11 +47,6 @@ static unsigned int reg_debug;
 module_param(reg_debug, int, 0644);
 MODULE_PARM_DESC(reg_debug, "enable debug messages [URB reg]");
 
-#define cx231xx_regdbg(fmt, arg...) do {\
-	if (reg_debug) \
-		printk(KERN_INFO "%s %s :"fmt, \
-			 dev->name, __func__ , ##arg); } while (0)
-
 static int alt = CX231XX_PINOUT;
 module_param(alt, int, 0644);
 MODULE_PARM_DESC(alt, "alternate setting to use for video endpoint");
@@ -240,6 +235,66 @@ int cx231xx_send_usb_command(struct cx231xx_i2c *i2c_bus,
 EXPORT_SYMBOL_GPL(cx231xx_send_usb_command);
 
 /*
+ * Sends/Receives URB control messages, assuring to use a kalloced buffer
+ * for all operations (dev->urb_buf), to avoid using stacked buffers, as
+ * they aren't safe for usage with USB, due to DMA restrictions.
+ * Also implements the debug code for control URB's.
+ */
+static int __usb_control_msg(struct cx231xx *dev, unsigned int pipe,
+	__u8 request, __u8 requesttype, __u16 value, __u16 index,
+	void *data, __u16 size, int timeout)
+{
+	int rc, i;
+
+	if (reg_debug) {
+		printk(KERN_DEBUG "%s: (pipe 0x%08x): "
+				"%s:  %02x %02x %02x %02x %02x %02x %02x %02x ",
+				dev->name,
+				pipe,
+				(requesttype & USB_DIR_IN) ? "IN" : "OUT",
+				requesttype,
+				request,
+				value & 0xff, value >> 8,
+				index & 0xff, index >> 8,
+				size & 0xff, size >> 8);
+		if (!(requesttype & USB_DIR_IN)) {
+			printk(KERN_CONT ">>>");
+			for (i = 0; i < size; i++)
+				printk(KERN_CONT " %02x",
+				       ((unsigned char *)data)[i]);
+		}
+	}
+
+	/* Do the real call to usb_control_msg */
+	mutex_lock(&dev->ctrl_urb_lock);
+	if (!(requesttype & USB_DIR_IN) && size)
+		memcpy(dev->urb_buf, data, size);
+	rc = usb_control_msg(dev->udev, pipe, request, requesttype, value,
+			     index, dev->urb_buf, size, timeout);
+	if ((requesttype & USB_DIR_IN) && size)
+		memcpy(data, dev->urb_buf, size);
+	mutex_unlock(&dev->ctrl_urb_lock);
+
+	if (reg_debug) {
+		if (unlikely(rc < 0)) {
+			printk(KERN_CONT "FAILED!\n");
+			return rc;
+		}
+
+		if ((requesttype & USB_DIR_IN)) {
+			printk(KERN_CONT "<<<");
+			for (i = 0; i < size; i++)
+				printk(KERN_CONT " %02x",
+				       ((unsigned char *)data)[i]);
+		}
+		printk(KERN_CONT "\n");
+	}
+
+	return rc;
+}
+
+
+/*
  * cx231xx_read_ctrl_reg()
  * reads data from the usb device specifying bRequest and wValue
  */
@@ -276,39 +331,9 @@ int cx231xx_read_ctrl_reg(struct cx231xx *dev, u8 req, u16 reg,
 	if (val == 0xFF)
 		return -EINVAL;
 
-	if (reg_debug) {
-		cx231xx_isocdbg("(pipe 0x%08x): "
-				"IN:  %02x %02x %02x %02x %02x %02x %02x %02x ",
-				pipe,
-				USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-				req, 0, val,
-				reg & 0xff, reg >> 8, len & 0xff, len >> 8);
-	}
-
-	mutex_lock(&dev->ctrl_urb_lock);
-	ret = usb_control_msg(dev->udev, pipe, req,
+	ret = __usb_control_msg(dev, pipe, req,
 			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-			      val, reg, dev->urb_buf, len, HZ);
-	if (ret < 0) {
-		cx231xx_isocdbg(" failed!\n");
-		mutex_unlock(&dev->ctrl_urb_lock);
-		return ret;
-	}
-
-	if (len)
-		memcpy(buf, dev->urb_buf, len);
-
-	mutex_unlock(&dev->ctrl_urb_lock);
-
-	if (reg_debug) {
-		int byte;
-
-		cx231xx_isocdbg("<<<");
-		for (byte = 0; byte < len; byte++)
-			cx231xx_isocdbg(" %02x", (unsigned char)buf[byte]);
-		cx231xx_isocdbg("\n");
-	}
-
+			      val, reg, buf, len, HZ);
 	return ret;
 }
 
@@ -331,28 +356,10 @@ int cx231xx_send_vendor_cmd(struct cx231xx *dev,
 	else
 		pipe = usb_sndctrlpipe(dev->udev, 0);
 
-	if (reg_debug) {
-		int byte;
-
-		cx231xx_isocdbg("(pipe 0x%08x): "
-				"OUT: %02x %02x %02x %04x %04x %04x >>>",
-				pipe,
-				ven_req->
-				direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-				ven_req->bRequest, 0, ven_req->wValue,
-				ven_req->wIndex, ven_req->wLength);
-
-		for (byte = 0; byte < ven_req->wLength; byte++)
-			cx231xx_isocdbg(" %02x",
-					(unsigned char)ven_req->pBuff[byte]);
-		cx231xx_isocdbg("\n");
-	}
-
-
-/*
-If the cx23102 read more than 4 bytes with i2c bus,
-need chop to 4 byte per request
-*/
+	/*
+	 * If the cx23102 read more than 4 bytes with i2c bus,
+	 * need chop to 4 byte per request
+	 */
 	if ((ven_req->wLength > 4) && ((ven_req->bRequest == 0x4) ||
 					(ven_req->bRequest == 0x5) ||
 					(ven_req->bRequest == 0x6))) {
@@ -362,71 +369,39 @@ need chop to 4 byte per request
 
 		unsend_size = ven_req->wLength;
 
-		mutex_lock(&dev->ctrl_urb_lock);
-		/* the first package*/
+		/* the first package */
 		ven_req->wValue = ven_req->wValue & 0xFFFB;
 		ven_req->wValue = (ven_req->wValue & 0xFFBD) | 0x2;
-		/*printk(KERN_INFO " !!!!! 0x%x 0x%x 0x%x 0x%x \n",
-			ven_req->bRequest,
-			ven_req->direction | USB_TYPE_VENDOR |
-			USB_RECIP_DEVICE,ven_req->wValue,ven_req->wIndex);*/
-		ret = usb_control_msg(dev->udev, pipe, ven_req->bRequest,
-			ven_req->
-			direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		ret = __usb_control_msg(dev, pipe, ven_req->bRequest,
+			ven_req->direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 			ven_req->wValue, ven_req->wIndex, pdata,
 			0x0004, HZ);
 		unsend_size = unsend_size - 4;
-		mutex_unlock(&dev->ctrl_urb_lock);
 
-		/* the middle package*/
+		/* the middle package */
 		ven_req->wValue = (ven_req->wValue & 0xFFBD) | 0x42;
 		while (unsend_size - 4 > 0) {
 			pdata = pdata + 4;
-			/*printk(KERN_INFO " !!!!! 0x%x 0x%x 0x%x 0x%x \n",
+			ret = __usb_control_msg(dev, pipe,
 				ven_req->bRequest,
-				ven_req->direction | USB_TYPE_VENDOR |
-				USB_RECIP_DEVICE,
-				ven_req->wValue,ven_req->wIndex);*/
-			mutex_lock(&dev->ctrl_urb_lock);
-			ret = usb_control_msg(dev->udev, pipe,
-				 ven_req->bRequest,
-				ven_req->
-				direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				ven_req->direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 				ven_req->wValue, ven_req->wIndex, pdata,
 				0x0004, HZ);
-			mutex_unlock(&dev->ctrl_urb_lock);
 			unsend_size = unsend_size - 4;
 		}
 
-
-		/* the last package*/
+		/* the last package */
 		ven_req->wValue = (ven_req->wValue & 0xFFBD) | 0x40;
 		pdata = pdata + 4;
-		/*printk(KERN_INFO " !!!!! 0x%x 0x%x 0x%x 0x%x \n",
-			ven_req->bRequest,
-			ven_req->direction | USB_TYPE_VENDOR |
-			USB_RECIP_DEVICE,ven_req->wValue,ven_req->wIndex);*/
-		mutex_lock(&dev->ctrl_urb_lock);
-		ret = usb_control_msg(dev->udev, pipe, ven_req->bRequest,
-			ven_req->
-			direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		ret = __usb_control_msg(dev, pipe, ven_req->bRequest,
+			ven_req->direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 			ven_req->wValue, ven_req->wIndex, pdata,
 			unsend_size, HZ);
-		mutex_unlock(&dev->ctrl_urb_lock);
-		/*printk(KERN_INFO " @@@@@ temp_buffer[0]=0x%x 0x%x 0x%x 0x%x
-			  0x%x 0x%x\n",ven_req->pBuff[0],ven_req->pBuff[1],
-			ven_req->pBuff[2], ven_req->pBuff[3],ven_req->pBuff[4],
-			ven_req->pBuff[5]);*/
-
 	} else {
-		mutex_lock(&dev->ctrl_urb_lock);
-		ret = usb_control_msg(dev->udev, pipe, ven_req->bRequest,
-				ven_req->
-				direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+		ret = __usb_control_msg(dev, pipe, ven_req->bRequest,
+				ven_req->direction | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 				ven_req->wValue, ven_req->wIndex,
-				 ven_req->pBuff, ven_req->wLength, HZ);
-		mutex_unlock(&dev->ctrl_urb_lock);
-
+				ven_req->pBuff, ven_req->wLength, HZ);
 	}
 
 	return ret;
@@ -484,12 +459,9 @@ int cx231xx_write_ctrl_reg(struct cx231xx *dev, u8 req, u16 reg, char *buf,
 		cx231xx_isocdbg("\n");
 	}
 
-	mutex_lock(&dev->ctrl_urb_lock);
-	memcpy(dev->urb_buf, buf, len);
-	ret = usb_control_msg(dev->udev, pipe, req,
+	ret = __usb_control_msg(dev, pipe, req,
 			      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
-			      val, reg, dev->urb_buf, len, HZ);
-	mutex_unlock(&dev->ctrl_urb_lock);
+			      val, reg, buf, len, HZ);
 
 	return ret;
 }
-- 
1.7.1

[PATCH 06/10] V4L/DVB: cx231xx: better handle the master port enable command

[Mauro Carvalho Chehab]

Improves the logic, for it to be clearer and to avoid having
board-dependent config there.

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-avcore.c b/drivers/media/video/cx231xx/cx231xx-avcore.c
index ab9fbf8..2d773b3 100644
--- a/drivers/media/video/cx231xx/cx231xx-avcore.c
+++ b/drivers/media/video/cx231xx/cx231xx-avcore.c
@@ -1268,36 +1268,39 @@ int cx231xx_set_agc_analog_digital_mux_select(struct cx231xx *dev,
 	return status;
 }
 
-int cx231xx_enable_i2c_for_tuner(struct cx231xx *dev, u8 I2CIndex)
+int cx231xx_enable_i2c_port_3(struct cx231xx *dev, bool is_port_3)
 {
 	u8 value[4] = { 0, 0, 0, 0 };
 	int status = 0;
-
-	cx231xx_info("Changing the i2c port for tuner to %d\n", I2CIndex);
+	bool current_is_port_3;
 
 	status = cx231xx_read_ctrl_reg(dev, VRT_GET_REGISTER,
 				       PWR_CTL_EN, value, 4);
 	if (status < 0)
 		return status;
 
-	if (I2CIndex == I2C_1) {
-		if (value[0] & I2C_DEMOD_EN) {
-			value[0] &= ~I2C_DEMOD_EN;
-			status = cx231xx_write_ctrl_reg(dev, VRT_SET_REGISTER,
-						   PWR_CTL_EN, value, 4);
-		}
-	} else {
-		if (!(value[0] & I2C_DEMOD_EN)) {
-			value[0] |= I2C_DEMOD_EN;
-			status = cx231xx_write_ctrl_reg(dev, VRT_SET_REGISTER,
-						   PWR_CTL_EN, value, 4);
-		}
-	}
+	current_is_port_3 = value[0] & I2C_DEMOD_EN ? true : false;
+
+	/* Just return, if already using the right port */
+	if (current_is_port_3 == is_port_3)
+		return 0;
+
+	if (is_port_3)
+		value[0] |= I2C_DEMOD_EN;
+	else
+		value[0] &= ~I2C_DEMOD_EN;
+
+	cx231xx_info("Changing the i2c master port to %d\n",
+		     is_port_3 ?  3 : 1);
+
+	status = cx231xx_write_ctrl_reg(dev, VRT_SET_REGISTER,
+					PWR_CTL_EN, value, 4);
 
 	return status;
 
 }
-EXPORT_SYMBOL_GPL(cx231xx_enable_i2c_for_tuner);
+EXPORT_SYMBOL_GPL(cx231xx_enable_i2c_port_3);
+
 void update_HH_register_after_set_DIF(struct cx231xx *dev)
 {
 /*
@@ -2324,26 +2327,16 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
 			msleep(PWR_SLEEP_INTERVAL);
 		}
 
-		if ((dev->model == CX231XX_BOARD_CNXT_CARRAERA) ||
-		    (dev->model == CX231XX_BOARD_CNXT_RDE_250) ||
-		    (dev->model == CX231XX_BOARD_CNXT_SHELBY) ||
-		    (dev->model == CX231XX_BOARD_CNXT_RDU_250)) {
-			/* tuner path to channel 1 from port 3 */
-			cx231xx_enable_i2c_for_tuner(dev, I2C_3);
+		if (dev->board.tuner_type != TUNER_ABSENT) {
+			/* Enable tuner */
+			cx231xx_enable_i2c_port_3(dev, true);
 
 			/* reset the Tuner */
-			cx231xx_gpio_set(dev, dev->board.tuner_gpio);
+			if (dev->board.tuner_gpio)
+				cx231xx_gpio_set(dev, dev->board.tuner_gpio);
 
 			if (dev->cx231xx_reset_analog_tuner)
 				dev->cx231xx_reset_analog_tuner(dev);
-		} else if ((dev->model == CX231XX_BOARD_CNXT_RDE_253S) ||
-			   (dev->model == CX231XX_BOARD_CNXT_VIDEO_GRABBER) ||
-			   (dev->model == CX231XX_BOARD_CNXT_RDU_253S) ||
-			   (dev->model == CX231XX_BOARD_HAUPPAUGE_EXETER)) {
-			/* tuner path to channel 1 from port 3 */
-			cx231xx_enable_i2c_for_tuner(dev, I2C_3);
-			if (dev->cx231xx_reset_analog_tuner)
-				dev->cx231xx_reset_analog_tuner(dev);
 		}
 
 		break;
@@ -2401,33 +2394,23 @@ int cx231xx_set_power_mode(struct cx231xx *dev, enum AV_MODE mode)
 			msleep(PWR_SLEEP_INTERVAL);
 		}
 
-		if ((dev->model == CX231XX_BOARD_CNXT_CARRAERA) ||
-		    (dev->model == CX231XX_BOARD_CNXT_RDE_250) ||
-		    (dev->model == CX231XX_BOARD_CNXT_SHELBY) ||
-		    (dev->model == CX231XX_BOARD_CNXT_RDU_250)) {
-			/* tuner path to channel 1 from port 3 */
-			cx231xx_enable_i2c_for_tuner(dev, I2C_3);
+		if (dev->board.tuner_type != TUNER_ABSENT) {
+			/*
+			 * Enable tuner
+			 *	Hauppauge Exeter seems to need to do something different!
+			 */
+			if (dev->model == CX231XX_BOARD_HAUPPAUGE_EXETER)
+				cx231xx_enable_i2c_port_3(dev, false);
+			else
+				cx231xx_enable_i2c_port_3(dev, true);
 
 			/* reset the Tuner */
-			cx231xx_gpio_set(dev, dev->board.tuner_gpio);
-
-			if (dev->cx231xx_reset_analog_tuner)
-				dev->cx231xx_reset_analog_tuner(dev);
-		} else if ((dev->model == CX231XX_BOARD_CNXT_RDE_253S) ||
-		    (dev->model == CX231XX_BOARD_CNXT_VIDEO_GRABBER) ||
-		    (dev->model == CX231XX_BOARD_CNXT_RDU_253S)) {
-			/* tuner path to channel 1 from port 3 */
-			cx231xx_enable_i2c_for_tuner(dev, I2C_3);
-			if (dev->cx231xx_reset_analog_tuner)
-				dev->cx231xx_reset_analog_tuner(dev);
-		} else if (dev->model == CX231XX_BOARD_HAUPPAUGE_EXETER) {
-			/* tuner path to channel 1 from port 1 ?? */
-			cx231xx_enable_i2c_for_tuner(dev, I2C_1);
+			if (dev->board.tuner_gpio)
+				cx231xx_gpio_set(dev, dev->board.tuner_gpio);
 
 			if (dev->cx231xx_reset_analog_tuner)
 				dev->cx231xx_reset_analog_tuner(dev);
 		}
-
 		break;
 
 	default:
diff --git a/drivers/media/video/cx231xx/cx231xx-core.c b/drivers/media/video/cx231xx/cx231xx-core.c
index 983b120..4af46fc 100644
--- a/drivers/media/video/cx231xx/cx231xx-core.c
+++ b/drivers/media/video/cx231xx/cx231xx-core.c
@@ -1401,9 +1401,7 @@ int cx231xx_dev_init(struct cx231xx *dev)
 		cx231xx_set_alt_setting(dev, INDEX_TS1, 0);
 
 	/* set the I2C master port to 3 on channel 1 */
-	if (dev->model != CX231XX_BOARD_CNXT_VIDEO_GRABBER &&
-	    dev->model != CX231XX_BOARD_HAUPPAUGE_USBLIVE2)
-		errCode = cx231xx_enable_i2c_for_tuner(dev, I2C_3);
+	errCode = cx231xx_enable_i2c_port_3(dev, true);
 
 	return errCode;
 }
diff --git a/drivers/media/video/cx231xx/cx231xx-dvb.c b/drivers/media/video/cx231xx/cx231xx-dvb.c
index 4efd3d3..5feb3ee 100644
--- a/drivers/media/video/cx231xx/cx231xx-dvb.c
+++ b/drivers/media/video/cx231xx/cx231xx-dvb.c
@@ -235,11 +235,11 @@ static int start_streaming(struct cx231xx_dvb *dvb)
 
 	if (dev->USE_ISO) {
 		cx231xx_info("DVB transfer mode is ISO.\n");
-mutex_lock(&dev->i2c_lock);
-		cx231xx_enable_i2c_for_tuner(dev, I2C_1);
+		mutex_lock(&dev->i2c_lock);
+		cx231xx_enable_i2c_port_3(dev, false);
 		cx231xx_set_alt_setting(dev, INDEX_TS1, 4);
-		cx231xx_enable_i2c_for_tuner(dev, I2C_3);
-mutex_unlock(&dev->i2c_lock);
+		cx231xx_enable_i2c_port_3(dev, true);
+		mutex_unlock(&dev->i2c_lock);
 		rc = cx231xx_set_mode(dev, CX231XX_DIGITAL_MODE);
 		if (rc < 0)
 			return rc;
diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h
index 5ffdd36..b4859a0 100644
--- a/drivers/media/video/cx231xx/cx231xx.h
+++ b/drivers/media/video/cx231xx/cx231xx.h
@@ -929,7 +929,7 @@ int cx231xx_power_suspend(struct cx231xx *dev);
 int cx231xx_init_ctrl_pin_status(struct cx231xx *dev);
 int cx231xx_set_agc_analog_digital_mux_select(struct cx231xx *dev,
 					      u8 analog_or_digital);
-int cx231xx_enable_i2c_for_tuner(struct cx231xx *dev, u8 I2CIndex);
+int cx231xx_enable_i2c_port_3(struct cx231xx *dev, bool is_port_3);
 
 /* video audio decoder related functions */
 void video_mux(struct cx231xx *dev, int index);
-- 
1.7.1

[PATCH 05/10] V4L/DVB: cx231xx: properly use the right tuner i2c address

[Mauro Carvalho Chehab]

The driver has a field to indicate what bus is used by tuner and
by demod. However, this field were never used. On Pixelview,
it uses I2C 2 for tuner, instead of I2C 1.

	drivers/media/video/cx231xx/cx231xx-cards.c

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c b/drivers/media/video/cx231xx/cx231xx-cards.c
index b516068..8e088db 100644
--- a/drivers/media/video/cx231xx/cx231xx-cards.c
+++ b/drivers/media/video/cx231xx/cx231xx-cards.c
@@ -569,7 +569,7 @@ void cx231xx_card_setup(struct cx231xx *dev)
 	/* Initialize the tuner */
 	if (dev->board.tuner_type != TUNER_ABSENT) {
 		dev->sd_tuner = v4l2_i2c_new_subdev(&dev->v4l2_dev,
-						    &dev->i2c_bus[1].i2c_adap,
+						    &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 						    "tuner", "tuner",
 						    dev->tuner_addr, NULL);
 		if (dev->sd_tuner == NULL)
diff --git a/drivers/media/video/cx231xx/cx231xx-dvb.c b/drivers/media/video/cx231xx/cx231xx-dvb.c
index 879eacb..4efd3d3 100644
--- a/drivers/media/video/cx231xx/cx231xx-dvb.c
+++ b/drivers/media/video/cx231xx/cx231xx-dvb.c
@@ -347,7 +347,7 @@ static int attach_xc5000(u8 addr, struct cx231xx *dev)
 	struct xc5000_config cfg;
 
 	memset(&cfg, 0, sizeof(cfg));
-	cfg.i2c_adap = &dev->i2c_bus[1].i2c_adap;
+	cfg.i2c_adap = &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap;
 	cfg.i2c_addr = addr;
 
 	if (!dev->dvb->frontend) {
@@ -573,7 +573,7 @@ static int dvb_init(struct cx231xx *dev)
 
 		dev->dvb->frontend = dvb_attach(s5h1432_attach,
 					&dvico_s5h1432_config,
-					&dev->i2c_bus[2].i2c_adap);
+					&dev->i2c_bus[dev->board.demod_i2c_master].i2c_adap);
 
 		if (dev->dvb->frontend == NULL) {
 			printk(DRIVER_NAME
@@ -586,7 +586,7 @@ static int dvb_init(struct cx231xx *dev)
 		dvb->frontend->callback = cx231xx_tuner_callback;
 
 		if (!dvb_attach(xc5000_attach, dev->dvb->frontend,
-			       &dev->i2c_bus[1].i2c_adap,
+			       &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 			       &cnxt_rde250_tunerconfig)) {
 			result = -EINVAL;
 			goto out_free;
@@ -598,7 +598,7 @@ static int dvb_init(struct cx231xx *dev)
 
 		dev->dvb->frontend = dvb_attach(s5h1411_attach,
 					       &xc5000_s5h1411_config,
-					       &dev->i2c_bus[2].i2c_adap);
+					       &dev->i2c_bus[dev->board.demod_i2c_master].i2c_adap);
 
 		if (dev->dvb->frontend == NULL) {
 			printk(DRIVER_NAME
@@ -611,7 +611,7 @@ static int dvb_init(struct cx231xx *dev)
 		dvb->frontend->callback = cx231xx_tuner_callback;
 
 		if (!dvb_attach(xc5000_attach, dev->dvb->frontend,
-			       &dev->i2c_bus[1].i2c_adap,
+			       &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 			       &cnxt_rdu250_tunerconfig)) {
 			result = -EINVAL;
 			goto out_free;
@@ -621,7 +621,7 @@ static int dvb_init(struct cx231xx *dev)
 
 		dev->dvb->frontend = dvb_attach(s5h1432_attach,
 					&dvico_s5h1432_config,
-					&dev->i2c_bus[2].i2c_adap);
+					&dev->i2c_bus[dev->board.demod_i2c_master].i2c_adap);
 
 		if (dev->dvb->frontend == NULL) {
 			printk(DRIVER_NAME
@@ -634,7 +634,7 @@ static int dvb_init(struct cx231xx *dev)
 		dvb->frontend->callback = cx231xx_tuner_callback;
 
 		if (!dvb_attach(tda18271_attach, dev->dvb->frontend,
-			       0x60, &dev->i2c_bus[1].i2c_adap,
+			       0x60, &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 			       &cnxt_rde253s_tunerconfig)) {
 			result = -EINVAL;
 			goto out_free;
@@ -644,7 +644,7 @@ static int dvb_init(struct cx231xx *dev)
 
 		dev->dvb->frontend = dvb_attach(s5h1411_attach,
 					       &tda18271_s5h1411_config,
-					       &dev->i2c_bus[2].i2c_adap);
+					       &dev->i2c_bus[dev->board.demod_i2c_master].i2c_adap);
 
 		if (dev->dvb->frontend == NULL) {
 			printk(DRIVER_NAME
@@ -657,7 +657,7 @@ static int dvb_init(struct cx231xx *dev)
 		dvb->frontend->callback = cx231xx_tuner_callback;
 
 		if (!dvb_attach(tda18271_attach, dev->dvb->frontend,
-			       0x60, &dev->i2c_bus[1].i2c_adap,
+			       0x60, &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 			       &cnxt_rde253s_tunerconfig)) {
 			result = -EINVAL;
 			goto out_free;
@@ -666,11 +666,11 @@ static int dvb_init(struct cx231xx *dev)
 	case CX231XX_BOARD_HAUPPAUGE_EXETER:
 
 		printk(KERN_INFO "%s: looking for tuner / demod on i2c bus: %d\n",
-		       __func__, i2c_adapter_id(&dev->i2c_bus[1].i2c_adap));
+		       __func__, i2c_adapter_id(&dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap));
 
 		dev->dvb->frontend = dvb_attach(lgdt3305_attach,
 						&hcw_lgdt3305_config,
-						&dev->i2c_bus[1].i2c_adap);
+						&dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap);
 
 		if (dev->dvb->frontend == NULL) {
 			printk(DRIVER_NAME
@@ -683,7 +683,7 @@ static int dvb_init(struct cx231xx *dev)
 		dvb->frontend->callback = cx231xx_tuner_callback;
 
 		dvb_attach(tda18271_attach, dev->dvb->frontend,
-			   0x60, &dev->i2c_bus[1].i2c_adap,
+			   0x60, &dev->i2c_bus[dev->board.tuner_i2c_master].i2c_adap,
 			   &hcw_tda18271_config);
 		break;
 
-- 
1.7.1

[PATCH 07/10] V4L/DVB: cx231xx: Only change gpio direction when needed

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-cards.c b/drivers/media/video/cx231xx/cx231xx-cards.c
index 8e088db..28f77a7 100644
--- a/drivers/media/video/cx231xx/cx231xx-cards.c
+++ b/drivers/media/video/cx231xx/cx231xx-cards.c
@@ -495,10 +495,11 @@ void cx231xx_pre_card_setup(struct cx231xx *dev)
 	if (dev->board.tuner_gpio) {
 		cx231xx_set_gpio_direction(dev, dev->board.tuner_gpio->bit, 1);
 		cx231xx_set_gpio_value(dev, dev->board.tuner_gpio->bit, 1);
+	}
+	if (dev->board.tuner_sif_gpio >= 0)
 		cx231xx_set_gpio_direction(dev, dev->board.tuner_sif_gpio, 1);
 
-		/* request some modules if any required */
-	}
+	/* request some modules if any required */
 
 	/* set the mode to Analog mode initially */
 	cx231xx_set_mode(dev, CX231XX_ANALOG_MODE);
diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h
index b4859a0..d079433 100644
--- a/drivers/media/video/cx231xx/cx231xx.h
+++ b/drivers/media/video/cx231xx/cx231xx.h
@@ -333,9 +333,10 @@ struct cx231xx_board {
 	struct cx231xx_reg_seq *dvb_gpio;
 	struct cx231xx_reg_seq *suspend_gpio;
 	struct cx231xx_reg_seq *tuner_gpio;
-	u8 tuner_sif_gpio;
-	u8 tuner_scl_gpio;
-	u8 tuner_sda_gpio;
+		/* Negative means don't use it */
+	s8 tuner_sif_gpio;
+	s8 tuner_scl_gpio;
+	s8 tuner_sda_gpio;
 
 	/* PIN ctrl */
 	u32 ctl_pin_status_mask;
-- 
1.7.1

[PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[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 <mchehab@redhat.com>

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

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Devin Heitmueller]

On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 <mchehab@redhat.com>
>
> 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

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Michael Krufky]

On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> 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 <mchehab@redhat.com>

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

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Mauro Carvalho Chehab]

Hi Michael,

Em 30-09-2010 15:52, Michael Krufky escreveu:
> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> 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 <mchehab@redhat.com>
> 
> 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.

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Michael Krufky]

On Thu, Sep 30, 2010 at 3:12 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Hi Michael,
>
> Em 30-09-2010 15:52, Michael Krufky escreveu:
>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>> <mchehab@redhat.com> 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 <mchehab@redhat.com>
>>
>> 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

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Mauro Carvalho Chehab]

Em 30-09-2010 16:18, Michael Krufky escreveu:
> On Thu, Sep 30, 2010 at 3:12 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Hi Michael,
>>
>> Em 30-09-2010 15:52, Michael Krufky escreveu:
>>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>>> <mchehab@redhat.com> 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 <mchehab@redhat.com>
>>>
>>> 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.

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Antti Palosaari]

On 09/30/2010 09:52 PM, Michael Krufky wrote:
> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com>  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<mchehab@redhat.com>
>
> 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/

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Michael Krufky]

On Thu, Sep 30, 2010 at 6:00 PM, Antti Palosaari <crope@iki.fi> wrote:
> On 09/30/2010 09:52 PM, Michael Krufky wrote:
>>
>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>> <mchehab@redhat.com>  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<mchehab@redhat.com>
>>
>> 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

Re: [PATCH 08/10] V4L/DVB: tda18271: allow restricting max out to 4 bytes

[Mauro Carvalho Chehab]

Em 30-09-2010 19:07, Michael Krufky escreveu:
> On Thu, Sep 30, 2010 at 6:00 PM, Antti Palosaari <crope@iki.fi> wrote:
>> On 09/30/2010 09:52 PM, Michael Krufky wrote:
>>>
>>> On Tue, Sep 28, 2010 at 2:46 PM, Mauro Carvalho Chehab
>>> <mchehab@redhat.com>  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<mchehab@redhat.com>
>>>
>>> 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

[PATCH 09/10] V4L/DVB: tda18271: Add debug message with frequency divisor

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/common/tuners/tda18271-common.c b/drivers/media/common/tuners/tda18271-common.c
index 195b30e..7ba3ba3 100644
--- a/drivers/media/common/tuners/tda18271-common.c
+++ b/drivers/media/common/tuners/tda18271-common.c
@@ -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

Re: [PATCH 09/10] V4L/DVB: tda18271: Add debug message with frequency divisor

[Devin Heitmueller]

On Tue, Sep 28, 2010 at 2:47 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-common.c b/drivers/media/common/tuners/tda18271-common.c
> index 195b30e..7ba3ba3 100644
> --- a/drivers/media/common/tuners/tda18271-common.c
> +++ b/drivers/media/common/tuners/tda18271-common.c
> @@ -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
>
>
>

Adding the maintainer for the 18271 driver to the CC.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: [PATCH 09/10] V4L/DVB: tda18271: Add debug message with frequency divisor

[Michael Krufky]

On Tue, Sep 28, 2010 at 2:47 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-common.c b/drivers/media/common/tuners/tda18271-common.c
> index 195b30e..7ba3ba3 100644
> --- a/drivers/media/common/tuners/tda18271-common.c
> +++ b/drivers/media/common/tuners/tda18271-common.c
> @@ -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;
>  }


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

Re: [PATCH 09/10] V4L/DVB: tda18271: Add debug message with frequency divisor

[Mauro Carvalho Chehab]

Em 30-09-2010 16:03, Michael Krufky escreveu:
> On Tue, Sep 28, 2010 at 2:47 PM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>>
>> diff --git a/drivers/media/common/tuners/tda18271-common.c b/drivers/media/common/tuners/tda18271-common.c
>> index 195b30e..7ba3ba3 100644
>> --- a/drivers/media/common/tuners/tda18271-common.c
>> +++ b/drivers/media/common/tuners/tda18271-common.c
>> @@ -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;
>>  }
> 
> 
> 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.

Ok.

> 
> Regards,
> 
> Mike Krufky

[PATCH 10/10] V4L/DVB: cx231xx-audio: fix some locking issues

[Mauro Carvalho Chehab]

Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>

diff --git a/drivers/media/video/cx231xx/cx231xx-audio.c b/drivers/media/video/cx231xx/cx231xx-audio.c
index 6ac418c..30d13c1 100644
--- a/drivers/media/video/cx231xx/cx231xx-audio.c
+++ b/drivers/media/video/cx231xx/cx231xx-audio.c
@@ -124,6 +124,9 @@ static void cx231xx_audio_isocirq(struct urb *urb)
 		break;
 	}
 
+	if (atomic_read(&dev->stream_started) == 0)
+		return;
+
 	if (dev->adev.capture_pcm_substream) {
 		substream = dev->adev.capture_pcm_substream;
 		runtime = substream->runtime;
@@ -206,6 +209,9 @@ static void cx231xx_audio_bulkirq(struct urb *urb)
 		break;
 	}
 
+	if (atomic_read(&dev->stream_started) == 0)
+		return;
+
 	if (dev->adev.capture_pcm_substream) {
 		substream = dev->adev.capture_pcm_substream;
 		runtime = substream->runtime;
@@ -370,35 +376,6 @@ static int cx231xx_init_audio_bulk(struct cx231xx *dev)
 	return errCode;
 }
 
-
-static int cx231xx_cmd(struct cx231xx *dev, int cmd, int arg)
-{
-	dprintk("%s transfer\n", (dev->adev.capture_stream == STREAM_ON) ?
-		"stop" : "start");
-
-	switch (cmd) {
-	case CX231XX_CAPTURE_STREAM_EN:
-		if (dev->adev.capture_stream == STREAM_OFF && arg == 1) {
-			dev->adev.capture_stream = STREAM_ON;
-			if (is_fw_load(dev) == 0)
-				cx25840_call(dev, core, load_fw);
-			if (dev->USE_ISO)
-				cx231xx_init_audio_isoc(dev);
-			else
-				cx231xx_init_audio_bulk(dev);
-		} else if (dev->adev.capture_stream == STREAM_ON && arg == 0) {
-			dev->adev.capture_stream = STREAM_OFF;
-			cx231xx_isoc_audio_deinit(dev);
-		} else {
-			cx231xx_errdev("An underrun very likely occurred. "
-				       "Ignoring it.\n");
-		}
-		return 0;
-	default:
-		return -EINVAL;
-	}
-}
-
 static int snd_pcm_alloc_vmalloc_buffer(struct snd_pcm_substream *subs,
 					size_t size)
 {
@@ -460,22 +437,24 @@ static int snd_cx231xx_capture_open(struct snd_pcm_substream *substream)
 
 	/* set alternate setting for audio interface */
 	/* 1 - 48000 samples per sec */
+	mutex_lock(&dev->lock);
 	if (dev->USE_ISO)
 		ret = cx231xx_set_alt_setting(dev, INDEX_AUDIO, 1);
 	else
 		ret = cx231xx_set_alt_setting(dev, INDEX_AUDIO, 0);
+	mutex_unlock(&dev->lock);
 	if (ret < 0) {
 		cx231xx_errdev("failed to set alternate setting !\n");
 
 		return ret;
 	}
 
-	/* inform hardware to start streaming */
-	ret = cx231xx_capture_start(dev, 1, Audio);
-
 	runtime->hw = snd_cx231xx_hw_capture;
 
 	mutex_lock(&dev->lock);
+	/* inform hardware to start streaming */
+	ret = cx231xx_capture_start(dev, 1, Audio);
+
 	dev->adev.users++;
 	mutex_unlock(&dev->lock);
 
@@ -493,7 +472,8 @@ static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
 
 	dprintk("closing device\n");
 
-	/* inform hardware to start streaming */
+	/* inform hardware to stop streaming */
+	mutex_lock(&dev->lock);
 	ret = cx231xx_capture_start(dev, 0, Audio);
 
 	/* set alternate setting for audio interface */
@@ -502,11 +482,11 @@ static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
 	if (ret < 0) {
 		cx231xx_errdev("failed to set alternate setting !\n");
 
+		mutex_unlock(&dev->lock);
 		return ret;
 	}
 
 	dev->mute = 1;
-	mutex_lock(&dev->lock);
 	dev->adev.users--;
 	mutex_unlock(&dev->lock);
 
@@ -515,7 +495,10 @@ static int snd_cx231xx_pcm_close(struct snd_pcm_substream *substream)
 		dprintk("disabling audio stream!\n");
 		dev->adev.shutdown = 0;
 		dprintk("released lock\n");
-		cx231xx_cmd(dev, CX231XX_CAPTURE_STREAM_EN, 0);
+		if (atomic_read(&dev->stream_started) > 0) {
+			atomic_set(&dev->stream_started, 0);
+			schedule_work(&dev->wq_trigger);
+		}
 	}
 	return 0;
 }
@@ -546,8 +529,10 @@ static int snd_cx231xx_hw_capture_free(struct snd_pcm_substream *substream)
 
 	dprintk("Stop capture, if needed\n");
 
-	if (dev->adev.capture_stream == STREAM_ON)
-		cx231xx_cmd(dev, CX231XX_CAPTURE_STREAM_EN, CX231XX_STOP_AUDIO);
+	if (atomic_read(&dev->stream_started) > 0) {
+		atomic_set(&dev->stream_started, 0);
+		schedule_work(&dev->wq_trigger);
+	}
 
 	return 0;
 }
@@ -562,32 +547,46 @@ static int snd_cx231xx_prepare(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+static void audio_trigger(struct work_struct *work)
+{
+	struct cx231xx *dev = container_of(work, struct cx231xx, wq_trigger);
+
+	if (atomic_read(&dev->stream_started)) {
+		dprintk("starting capture");
+		if (is_fw_load(dev) == 0)
+			cx25840_call(dev, core, load_fw);
+		if (dev->USE_ISO)
+			cx231xx_init_audio_isoc(dev);
+		else
+			cx231xx_init_audio_bulk(dev);
+	} else {
+		dprintk("stopping capture");
+		cx231xx_isoc_audio_deinit(dev);
+	}
+}
+
 static int snd_cx231xx_capture_trigger(struct snd_pcm_substream *substream,
 				       int cmd)
 {
 	struct cx231xx *dev = snd_pcm_substream_chip(substream);
 	int retval;
 
-	dprintk("Should %s capture\n", (cmd == SNDRV_PCM_TRIGGER_START) ?
-		"start" : "stop");
-
 	spin_lock(&dev->adev.slock);
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		cx231xx_cmd(dev, CX231XX_CAPTURE_STREAM_EN,
-			    CX231XX_START_AUDIO);
-		retval = 0;
+		atomic_set(&dev->stream_started, 1);
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		cx231xx_cmd(dev, CX231XX_CAPTURE_STREAM_EN, CX231XX_STOP_AUDIO);
-		retval = 0;
+		atomic_set(&dev->stream_started, 0);
 		break;
 	default:
 		retval = -EINVAL;
 	}
-
 	spin_unlock(&dev->adev.slock);
-	return retval;
+
+	schedule_work(&dev->wq_trigger);
+
+	return 0;
 }
 
 static snd_pcm_uframes_t snd_cx231xx_capture_pointer(struct snd_pcm_substream
@@ -668,6 +667,8 @@ static int cx231xx_audio_init(struct cx231xx *dev)
 	strcpy(card->shortname, "Cx231xx Audio");
 	strcpy(card->longname, "Conexant cx231xx Audio");
 
+	INIT_WORK(&dev->wq_trigger, audio_trigger);
+
 	err = snd_card_register(card);
 	if (err < 0) {
 		snd_card_free(card);
diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h
index d079433..f9cdc01 100644
--- a/drivers/media/video/cx231xx/cx231xx.h
+++ b/drivers/media/video/cx231xx/cx231xx.h
@@ -27,6 +27,7 @@
 #include <linux/ioctl.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
+#include <linux/workqueue.h>
 #include <linux/mutex.h>
 
 #include <media/cx2341x.h>
@@ -387,9 +388,6 @@ enum AUDIO_INPUT {
 #define CX231XX_AUDIO_BUFS              5
 #define CX231XX_NUM_AUDIO_PACKETS       16
 #define CX231XX_ISO_NUM_AUDIO_PACKETS	64
-#define CX231XX_CAPTURE_STREAM_EN       1
-#define CX231XX_STOP_AUDIO              0
-#define CX231XX_START_AUDIO             1
 
 /* cx231xx extensions */
 #define CX231XX_AUDIO                   0x10
@@ -407,7 +405,6 @@ struct cx231xx_audio {
 	struct snd_card *sndcard;
 
 	int users, shutdown;
-	enum cx231xx_stream_state capture_stream;
 	/* locks */
 	spinlock_t slock;
 
@@ -624,6 +621,9 @@ struct cx231xx {
 
 	struct cx231xx_IR *ir;
 
+	struct work_struct wq_trigger;		/* Trigger to start/stop audio for alsa module */
+	atomic_t	   stream_started;	/* stream should be running if true */
+
 	struct list_head devlist;
 
 	int tuner_type;		/* type of the tuner */
-- 
1.7.1