From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail.kapsi.fi ([217.30.184.167]:35089 "EHLO mail.kapsi.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752991Ab2JBMt7 (ORCPT ); Tue, 2 Oct 2012 08:49:59 -0400 Message-ID: <506AE2DE.80306@iki.fi> Date: Tue, 02 Oct 2012 15:49:34 +0300 From: Antti Palosaari MIME-Version: 1.0 To: Michael Krufky CC: Mauro Carvalho Chehab , Devin Heitmueller , linux-media@vger.kernel.org Subject: Re: [PATCH RFC] em28xx: PCTV 520e switch tda18271 to tda18271c2dd References: <1349139145-22113-1-git-send-email-crope@iki.fi> <506ABA2B.3070908@iki.fi> <20121002080503.76869be7@redhat.com> <506ADE45.6000708@iki.fi> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: On 10/02/2012 03:42 PM, Michael Krufky wrote: > On Tue, Oct 2, 2012 at 8:29 AM, Antti Palosaari wrote: >> On 10/02/2012 02:05 PM, Mauro Carvalho Chehab wrote: >>> >>> Em Tue, 02 Oct 2012 12:55:55 +0300 >>> Antti Palosaari escreveu: >>> >>>> On 10/02/2012 06:17 AM, Michael Krufky wrote: >>>>> >>>>> On Mon, Oct 1, 2012 at 9:58 PM, Devin Heitmueller >>>>> wrote: >>>>>> >>>>>> On Mon, Oct 1, 2012 at 8:52 PM, Antti Palosaari wrote: >>>>>>> >>>>>>> New drxk firmware download does not work with tda18271. Actual >>>>>>> reason is more drxk driver than tda18271. Anyhow, tda18271c2dd >>>>>>> will work as it does not do as much I/O during attach than tda18271. >>>>>>> >>>>>>> Root of cause is tuner I/O during drx-k asynchronous firmware >>>>>>> download. request_firmware_nowait()... :-/ >>>>>> >>>>>> >>>>>> This seems like it's just changing the timing of the initialization >>>>>> process, which isn't really any better than the "msleep(2000)". It's >>>>>> just dumb luck that it happens to work on the developer's system. >>>>>> >>>>>> Don't get me wrong, I agree with Michael that this whole situation is >>>>>> ridiculous, but I don't see why swapping out the entire driver is a >>>>>> reasonable fix. >>>>> >>>>> >>>>> I just send out a patch entitled, "tda18271: prevent register access >>>>> during attach() if delay_cal is set" Antti, could you set >>>>> tda18271_config.delay_cal = 1 with this patch applied and see if it >>>>> solves your problem? >>>>> >>>>> Again, although this may solve the problem for this particular device, >>>>> the *real* problem is this asynchronous firmware download in the demod >>>>> driver. >>>>> >>>>> Nonetheless, Antti has been asking for this feature, to not allow >>>>> register access during attach, I was against it and I have my reasons, >>>>> but I believe that this patch is a fair compromise. >>>>> >>>>> After somebody can test it, I think we should merge this -- any >>>>> comments? >>>>> >>>>> http://patchwork.linuxtv.org/patch/14799/ >>>> >>>> >>>> I tested. It does not help. I also looked it more and it really bails >>>> out with error much earlier, in function where it reads chip ID. That >>>> makes me look the tda18271c2dd driver. >>> >>> >>> I saw Antti's logs: basically, tda18271_get_id() reads all registers at >>> the >>> chip during attach(), returning -EINVAL if tda18271_read_regs(fe) can't >>> read the value for R_ID register. >>> >>> Btw, why do you need to read 16 registers at once, instead of just reading >>> the needed register? read_extended and write operations are even more >>> evil: >>> they read/write the full set of 39 registers on each operation. That seems >>> to be overkill, especially on places like tda18271_get_id(), where >>> all the driver is doing is to check for the ID register. >>> >>> Worse than that, tda18271_get_id() doesn't even check if the read() >>> operation failed: it assumes that it will always work, letting the >>> switch(regs[R_ID]) to print a wrong message (device unknown) when >>> what actually failed where the 16 registers dump. >>> >>>> I found that for some reason >>>> these drivers uses different method for register read. tda18271 uses I2C >>>> transaction with 2 messages, write and read with REPEATED START >>>> condition. tda18271c2dd driver is just simple I2C read. So which one is >>>> correct? >>> >>> >>> That's due to the I2C locking schema: if you do two separate I2C >>> transfers, the I2C core will allow an event to happen between the >>> two operations. That typically causes troubles on read operations. >>> So, it is recommended to use just one i2c_transfer() call for read >>> operations that are mapped via a write and a read. >> >> >> I know rather well how I2C works. As many messages you put to single >> transfer those are aimed to do with REPEATED START condition without a STOP. >> And naturally, when you split to multiple transactions then there is STOP. >> STOP is send after the last I2C message in one transaction. >> >> But what I tried to say there was a different communication schema used >> between the drivers. Other must (quite likely) be wrong. There is no any >> mention about hacks. I don't see how that I2C locking you mention is >> related, as it is *one* I2C transfer in question for both cases. >> >> Here is difference what I mean: >> tda18271: START c0|Ack|00|Ack|START c1|data read|NAck|STOP >> tda18271cc: START c1|data read|NAck|STOP > > As per section 9.1 of the NXP TDA18271 datasheet (page 8) (download > from http://www.nxp.com/documents/data_sheet/TDA18271HD.pdf) > > 9.1 I2C-bus format, Write/Read mode > Remark: The I2C-bus read in the TDA18271HD must read the entire > I2C-bus map, with > required subaddress 00h. The number of bytes read is 16, or 39 in > extended register > mode; see Table 7. Reading write-only bits can return values that are > different from the > programmed values > > tda18271 is doing the right thing, tda18271cc is not. > > -Mike Krufky Yes, you are correct! Thanks. I will still test if it makes any difference. regards Antti -- http://palosaari.fi/