[PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Luis Alves]

Hi,

This i2c init should stop the interrupt storm that happens in some cards when the IR receiver in enabled.
It works perfectly in my TBS6981.

It would be good to test in other problematic cards.

In this patch I've added the IR init to the TeVii S470/S471 (and some others that fall in the same case statment).
Other cards but these that suffer the same issue should also be tested.

Give feedback!

Regards,
Luis



Signed-off-by: Luis Alves <ljalvs@gmail.com>
---
 drivers/media/pci/cx23885/cx23885-cards.c |   22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/media/pci/cx23885/cx23885-cards.c b/drivers/media/pci/cx23885/cx23885-cards.c
index 7e923f8..89ce132 100644
--- a/drivers/media/pci/cx23885/cx23885-cards.c
+++ b/drivers/media/pci/cx23885/cx23885-cards.c
@@ -1081,6 +1081,27 @@ int cx23885_tuner_callback(void *priv, int component, int command, int arg)
 	return 0;
 }
 
+void cx23885_ir_setup(struct cx23885_dev *dev)
+{
+	struct i2c_msg msg;
+	char buffer[2];
+
+	/* this should stop the IR interrupt
+	   storm that happens in some cards */
+	msg.addr = 0x4c;
+	msg.flags = 0;
+	msg.len = 2;
+	msg.buf = buffer;
+
+	buffer[0] = 0x1f;
+	buffer[1] = 0x80;
+	i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+
+	buffer[0] = 0x23;
+	buffer[1] = 0x80;
+	i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
+}
+
 void cx23885_gpio_setup(struct cx23885_dev *dev)
 {
 	switch (dev->board) {
@@ -1664,6 +1685,7 @@ void cx23885_card_setup(struct cx23885_dev *dev)
 		ts1->gen_ctrl_val  = 0x5; /* Parallel */
 		ts1->ts_clk_en_val = 0x1; /* Enable TS_CLK */
 		ts1->src_sel_val   = CX23885_SRC_SEL_PARALLEL_MPEG_VIDEO;
+		cx23885_ir_setup(dev);
 		break;
 	case CX23885_BOARD_NETUP_DUAL_DVBS2_CI:
 	case CX23885_BOARD_NETUP_DUAL_DVB_T_C_CI_RF:
-- 
1.7.9.5

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Devin Heitmueller]

On Wed, Jul 17, 2013 at 9:33 PM, Luis Alves <ljalvs@gmail.com> wrote:
> Hi,
>
> This i2c init should stop the interrupt storm that happens in some cards when the IR receiver in enabled.
> It works perfectly in my TBS6981.

What is at I2C address 0x4c?  Might be useful to have a comment in
there explaining what this patch actually does.  This assumes you
know/understand what it does - if you don't then a comment saying "I
don't know why this is needed but my board doesn't work right without
it" is just as valuable.

> It would be good to test in other problematic cards.
>
> In this patch I've added the IR init to the TeVii S470/S471 (and some others that fall in the same case statment).
> Other cards but these that suffer the same issue should also be tested.

Without fully understanding the nature of this patch and what cards
that it actually effects, it may make sense to move your board into a
separate case statement.  Generally it's bad form to make changes like
against other cards without any testing against those cards (otherwise
you can introduce regressions).  Stick it in its own case statement,
and users of the other boards can move their cards into that case
statement *after* it's actually validated.

Devin

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

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Antti Palosaari]

On 07/18/2013 04:58 AM, Devin Heitmueller wrote:
> On Wed, Jul 17, 2013 at 9:33 PM, Luis Alves <ljalvs@gmail.com> wrote:
>> Hi,
>>
>> This i2c init should stop the interrupt storm that happens in some cards when the IR receiver in enabled.
>> It works perfectly in my TBS6981.
>
> What is at I2C address 0x4c?  Might be useful to have a comment in
> there explaining what this patch actually does.  This assumes you
> know/understand what it does - if you don't then a comment saying "I
> don't know why this is needed but my board doesn't work right without
> it" is just as valuable.
>
>> It would be good to test in other problematic cards.
>>
>> In this patch I've added the IR init to the TeVii S470/S471 (and some others that fall in the same case statment).
>> Other cards but these that suffer the same issue should also be tested.
>
> Without fully understanding the nature of this patch and what cards
> that it actually effects, it may make sense to move your board into a
> separate case statement.  Generally it's bad form to make changes like
> against other cards without any testing against those cards (otherwise
> you can introduce regressions).  Stick it in its own case statement,
> and users of the other boards can move their cards into that case
> statement *after* it's actually validated.
>
> Devin
>

hmm, I looked again the cx23885 driver.

0x4c == [0x98 >> 1] = "flatiron" == some internal block of the chip

There is routine which dumps registers out, 0x00 - 0x23
cx23885_flatiron_dump()

There is also existing routine to write those Flatiron registers. So, 
that direct I2C access could be shorten to:
cx23885_flatiron_write(dev, 0x1f, 0x80);
cx23885_flatiron_write(dev, 0x23, 0x80);


Unfortunately these two register names are not defined. Something clock 
or interrupt related likely.

regards
Antti

-- 
http://palosaari.fi/

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Devin Heitmueller]

On Wed, Jul 17, 2013 at 10:15 PM, Antti Palosaari <crope@iki.fi> wrote:
> hmm, I looked again the cx23885 driver.
>
> 0x4c == [0x98 >> 1] = "flatiron" == some internal block of the chip

Yeah, ok.  Pretty sure Flatiron is the codename for the ADC for the SIF.

> There is routine which dumps registers out, 0x00 - 0x23
> cx23885_flatiron_dump()
>
> There is also existing routine to write those Flatiron registers. So, that
> direct I2C access could be shorten to:
> cx23885_flatiron_write(dev, 0x1f, 0x80);
> cx23885_flatiron_write(dev, 0x23, 0x80);

Yeah, the internal register routines should be used to avoid confusion.

> Unfortunately these two register names are not defined. Something clock or
> interrupt related likely.

Strange.  The ADC output is usually tied directly to the Merlin.  I
wonder why it would ever generate interrupts.

No easy answers here.  WIll probably have to take a closer look at the
datasheet, or just ask Andy.

Devin

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

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Andy Walls]

On Wed, 2013-07-17 at 22:41 -0400, Devin Heitmueller wrote:
> On Wed, Jul 17, 2013 at 10:15 PM, Antti Palosaari <crope@iki.fi> wrote:
> > hmm, I looked again the cx23885 driver.
> >
> > 0x4c == [0x98 >> 1] = "flatiron" == some internal block of the chip
> 
> Yeah, ok.  Pretty sure Flatiron is the codename for the ADC for the SIF.
> 
> > There is routine which dumps registers out, 0x00 - 0x23
> > cx23885_flatiron_dump()
> >
> > There is also existing routine to write those Flatiron registers. So, that
> > direct I2C access could be shorten to:
> > cx23885_flatiron_write(dev, 0x1f, 0x80);
> > cx23885_flatiron_write(dev, 0x23, 0x80);
> 
> Yeah, the internal register routines should be used to avoid confusion.
> 
> > Unfortunately these two register names are not defined. Something clock or
> > interrupt related likely.
> 
> Strange.  The ADC output is usually tied directly to the Merlin.  I
> wonder why it would ever generate interrupts.

The CX2310[012] datasheet has a very short description of these Flatiron
registers.

Apparently the Flatiron genereates an interrupt after the built-in self
test for each of its left and right channels has completed.

Apparently Conexant wire-OR'ed the Flatiron's interrupt output with the
interrupt output of the CX23885 A/V core.



> No easy answers here.  WIll probably have to take a closer look at the
> datasheet, or just ask Andy.

The I2C writes clear the interrupt status of the built in self test
status interrupt for the left and right channels respectively.

It would be best to do this after any spurious A/V core interrupt is
detected from a CX23885.  Since they are I2C writes, they have to be
done in a non-IRQ context, as are the IR unit manipulations.

Regards,
Andy

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Konstantin Dimitrov]

On Thu, Jul 18, 2013 at 4:33 AM, Luis Alves <ljalvs@gmail.com> wrote:

> Signed-off-by: Luis Alves <ljalvs@gmail.com>
> ---
>  drivers/media/pci/cx23885/cx23885-cards.c |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/drivers/media/pci/cx23885/cx23885-cards.c b/drivers/media/pci/cx23885/cx23885-cards.c
> index 7e923f8..89ce132 100644
> --- a/drivers/media/pci/cx23885/cx23885-cards.c
> +++ b/drivers/media/pci/cx23885/cx23885-cards.c
> @@ -1081,6 +1081,27 @@ int cx23885_tuner_callback(void *priv, int component, int command, int arg)
>         return 0;
>  }
>
> +void cx23885_ir_setup(struct cx23885_dev *dev)
> +{
> +       struct i2c_msg msg;
> +       char buffer[2];
> +
> +       /* this should stop the IR interrupt
> +          storm that happens in some cards */
> +       msg.addr = 0x4c;
> +       msg.flags = 0;
> +       msg.len = 2;
> +       msg.buf = buffer;
> +
> +       buffer[0] = 0x1f;
> +       buffer[1] = 0x80;
> +       i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
> +
> +       buffer[0] = 0x23;
> +       buffer[1] = 0x80;
> +       i2c_transfer(&dev->i2c_bus[2].i2c_adap, &msg, 1);
> +}
> +

hi All,

i didn't want to interfere thus far for that series of related
patches, but because it starts to become a little ridiculous -
actually that's what happens when you copy&paste someone else's work
without having any understanding how and why that code (even very
simple as code) was invented (but it's not that simple to invent it
though) - in this particular case - that same code you can track back
to several years ago when drivers for TBS 698x cards was released by
me. so, for whatever reason that code wasn't up-streamed by me - lack
of time, NDAs, etc. and i don't mind that be up-streamed by someone
who wants to do it (after all it was released under GPL as patch to
GPL code), what i find as highly inappropriate when the author of the
code is perfectly known and it's known to who submit the above patch,
at least as a courtesy, if you wish, the original author of the code
to be included to CC - what about if i'm not subscribed to linux-media
or even if i missed to spot the email as part of linux-media and i as
the one who mind it have something in mind. so, i must say that i
totally agree with questions and concerns Devin Heitmueller
<dheitmueller@kernellabs.com> raised in regards to that code - when
it's highly specific to particular design and who submit it doesn't
really know what it does and those you know are not allow to say, it's
just increase the risk to pollute the mainline drivers rather then
improve them and that's why it needs at least to be put in right place
- not affect boards for which it's not intended. also, i think when
what that code does is not clear it should be a comment from where it
comes and the same if it wasn't open-source, but made with clean-room
reverse-engineering - that again should be mentioned, because it
implies you don't understand, at least fully, the work and you can't
really maintain what you submit as patches in case of problems.

best regards,
konstantin

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Luis Alves]

Hi Konstantin,

It was not my intention to send this piece of code as a patch to be
upstreamed. My apologies for that misunderstanding.
My intention was just to send something for people to try and see if
it solves the interrupt spam in their cards.
I should have sent it just as a normal email to the list.

You are right that I don't fully understand what those registers
control because unfortunately there is no public documentation
available (even for end of life products). But Andy Walls seem to have
a very good explanation.

I just disagree about knowing the author of this code... I had no clue
it was you, all I knew is that it came from tbs under GPL.
But if you say you are, I believe you and give you all the credit...

To be honest I just want my tbs card to work as it should.

Regards,
Luis

Re: [PATCH] cx23885: Fix interrupt storm that happens in some cards when IR is enabled.

[Luis Alves]

Sorry if I wasn't clear, but this patch is not intended to be merged
in the main tree (as it is).
I've sent it so that people facing this interrupt storm when IR is
enabled can test it in their cards (I only have the TBS6981 to test
and it works).
Probably I should have just sent a mail with a code sample...

About what it does, I don't have a clue! I just know that it does
silence the interrupt spam.
My best guess is that the IR interrupt line is shared with the ADC
interrupt line and maybe the ADC is generating an end-of-conversion
interrupt by default.
And touching this register can be disabling the ADC interrupts - or
powering down the ADC - or just disabling the ADC clock.

It would be valuable for other people that have this issues in their
cards to test and then make a proper patch to the cx23885.

If this doesn't work with other cards, then I'll just add those two
lines to be specific to my card init code.

Thanks and Regards,
Luis