| Message ID | 1374111202-23288-1-git-send-email-ljalvs@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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(+)