Message ID | 20120403110503.392c8432@milhouse (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1SEzfy-00070a-5G for patchwork@linuxtv.org; Tue, 03 Apr 2012 11:05:18 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.75/mailfrontend-2) with esmtp for <patchwork@linuxtv.org> id 1SEzfx-0001WW-Hs; Tue, 03 Apr 2012 11:05:17 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751559Ab2DCJFN (ORCPT <rfc822;patchwork@linuxtv.org>); Tue, 3 Apr 2012 05:05:13 -0400 Received: from bues.ch ([80.190.117.144]:45916 "EHLO bues.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750797Ab2DCJFM (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 3 Apr 2012 05:05:12 -0400 Received: by bues.ch with esmtpsa (Exim 4.72) (envelope-from <m@bues.ch>) id 1SEzfq-00042c-AR; Tue, 03 Apr 2012 11:05:10 +0200 Date: Tue, 3 Apr 2012 11:05:03 +0200 From: Michael =?UTF-8?B?QsO8c2No?= <m@bues.ch> To: Antti Palosaari <crope@iki.fi> Cc: linux-media <linux-media@vger.kernel.org> Subject: [PATCH] fc0011: Reduce number of retries Message-ID: <20120403110503.392c8432@milhouse> X-Mailer: Claws Mail 3.8.0 (GTK+ 2.24.10; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: multipart/signed; micalg=PGP-SHA1; boundary="Sig_/MAjqN74ROLgNqcL3rvD7w7D"; protocol="application/pgp-signature" Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 5.6.1.2065439, Antispam-Engine: 2.7.2.376379, Antispam-Data: 2012.4.3.85715 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' HTML_00_01 0.05, HTML_00_10 0.05, MIME_LOWER_CASE 0.05, MSGID_ADDED_BY_MTA 0.05, BODYTEXTP_SIZE_3000_LESS 0, BODY_SIZE_2000_2999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, INVALID_MSGID_NO_FQDN 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __CT 0, __CTYPE_HAS_BOUNDARY 0, __CTYPE_MULTIPART 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __MIME_VERSION 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Michael Büsch
April 3, 2012, 9:05 a.m. UTC
Now that i2c transfers are fixed, 3 retries are enough.
Signed-off-by: Michael Buesch <m@bues.ch>
---
Comments
Hi, On 04/03/2012 12:05 PM, Michael Büsch wrote: > Now that i2c transfers are fixed, 3 retries are enough. > > Signed-off-by: Michael Buesch<m@bues.ch> > > --- > > Index: linux/drivers/media/common/tuners/fc0011.c > =================================================================== > --- linux.orig/drivers/media/common/tuners/fc0011.c 2012-04-03 08:48:39.000000000 +0200 > +++ linux/drivers/media/common/tuners/fc0011.c 2012-04-03 10:44:07.243418827 +0200 > @@ -314,7 +314,7 @@ > if (err) > return err; > vco_retries = 0; > - while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 6) { > + while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 3) { Do we need to retry at all? I2C core layer is responsible to retry is xfer() fails. If failure is propagated to driver I'd assume: - I2C is still buggy by not return -EAGAIN on arbitrary error - I2C xfer failed for real. Look this piece of code from i2c-core.c: int i2c_transfer() { ... /* Retry automatically on arbitration loss */ orig_jiffies = jiffies; for (ret = 0, try = 0; try <= adap->retries; try++) { ret = adap->algo->master_xfer(adap, msgs, num); if (ret != -EAGAIN) break; if (time_after(jiffies, orig_jiffies + adap->timeout)) break; } ... } BR, David > /* Reset the tuner and try again */ > err = fe->callback(priv->i2c, DVB_FRONTEND_COMPONENT_TUNER, > FC0011_FE_CALLBACK_RESET, priv->addr); > > -- 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 Tue, 03 Apr 2012 12:58:16 +0300 David Cohen <david.a.cohen@linux.intel.com> wrote: > > - while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 6) { > > + while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 3) { > > Do we need to retry at all? It is not an i2c retry. It retries the whole device configuration operation after resetting it. I shouldn't have mentioned i2c in the commit log, because this really only was a side effect.
On 04/03/2012 01:07 PM, Michael Büsch wrote: > On Tue, 03 Apr 2012 12:58:16 +0300 > David Cohen<david.a.cohen@linux.intel.com> wrote: >>> - while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 6) { >>> + while (!(vco_cal& FC11_VCOCAL_OK)&& vco_retries< 3) { >> >> Do we need to retry at all? > > It is not an i2c retry. It retries the whole device configuration operation > after resetting it. > I shouldn't have mentioned i2c in the commit log, because this really only was > a side effect. > Hm, got it. :) -- 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 03.04.2012 12:05, Michael Büsch wrote: > Now that i2c transfers are fixed, 3 retries are enough. > > Signed-off-by: Michael Buesch<m@bues.ch> Applied, thanks! http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/af9035_experimental I think I will update original af9035 PULL request soon for the same level as af9035_experimental is currently. regards Antti
On Tue, 03 Apr 2012 18:24:20 +0300 Antti Palosaari <crope@iki.fi> wrote: > On 03.04.2012 12:05, Michael Büsch wrote: > > Now that i2c transfers are fixed, 3 retries are enough. > > > > Signed-off-by: Michael Buesch<m@bues.ch> > > Applied, thanks! > http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/af9035_experimental > > I think I will update original af9035 PULL request soon for the same > level as af9035_experimental is currently. That's great. The driver really works well for me. On another thing: The af9035 driver doesn't look multi-device safe. There are lots of static variables around that keep device state. So it looks like this will blow up if multiple devices are present in the system. Unlikely, but still... . Are there any plans to fix this up? If not, I'll probably take a look at this. But don't hold your breath.
On 03.04.2012 18:33, Michael Büsch wrote: > On Tue, 03 Apr 2012 18:24:20 +0300 > Antti Palosaari<crope@iki.fi> wrote: > >> On 03.04.2012 12:05, Michael Büsch wrote: >>> Now that i2c transfers are fixed, 3 retries are enough. >>> >>> Signed-off-by: Michael Buesch<m@bues.ch> >> >> Applied, thanks! >> http://git.linuxtv.org/anttip/media_tree.git/shortlog/refs/heads/af9035_experimental >> >> I think I will update original af9035 PULL request soon for the same >> level as af9035_experimental is currently. > > That's great. The driver really works well for me. > > On another thing: > The af9035 driver doesn't look multi-device safe. There are lots of static > variables around that keep device state. So it looks like this will > blow up if multiple devices are present in the system. Unlikely, but still... . > Are there any plans to fix this up? > If not, I'll probably take a look at this. But don't hold your breath. That's true and same applies for many other DVB USB drivers. Main reason for current hackish situation is DVB USB core limits. For example priv is not available until frontend attach etc. It "just" works even a little bit luck. Good example is that sequence counter, if you have multiple devices it runs wrongly as all increases same counter. But as a firmware does not care sequence numbers it still works. Remote controller is other big problem - coming from same limitations. And that is not first time these are spoken :) I have thought to redesign whole DVB USB framework, but as I am too busy always I haven't done that. Feel free to start fixing. regards Antti
On 03.04.2012 18:33, Michael Büsch wrote: > On another thing: > The af9035 driver doesn't look multi-device safe. There are lots of static > variables around that keep device state. So it looks like this will > blow up if multiple devices are present in the system. Unlikely, but still... . > Are there any plans to fix this up? > If not, I'll probably take a look at this. But don't hold your breath. I fixed what was possible by moving af9033 and af9035 configurations for the state. That at least resolves most significant issues - like your fc0011 tuner callback. But there is still some stuff in "static struct dvb_usb_device_properties" which cannot be fixed. Like dynamic remote configuration, dual mode, etc. I am going to make RFC for those maybe even this week after some analysis is done. regards Antti
On Mon, 07 May 2012 21:53:23 +0300 Antti Palosaari <crope@iki.fi> wrote: > On 03.04.2012 18:33, Michael Büsch wrote: > > On another thing: > > The af9035 driver doesn't look multi-device safe. There are lots of static > > variables around that keep device state. So it looks like this will > > blow up if multiple devices are present in the system. Unlikely, but still... . > > Are there any plans to fix this up? > > If not, I'll probably take a look at this. But don't hold your breath. > > I fixed what was possible by moving af9033 and af9035 configurations for > the state. That at least resolves most significant issues - like your > fc0011 tuner callback. Cool, thanks a lot! > But there is still some stuff in "static struct > dvb_usb_device_properties" which cannot be fixed. Like dynamic remote > configuration, dual mode, etc. I am going to make RFC for those maybe > even this week after some analysis is done. Thanks. I'm currently lacking the time to do this kind of intrusive changes, so I'm happy to see you looking into this.
Index: linux/drivers/media/common/tuners/fc0011.c =================================================================== --- linux.orig/drivers/media/common/tuners/fc0011.c 2012-04-03 08:48:39.000000000 +0200 +++ linux/drivers/media/common/tuners/fc0011.c 2012-04-03 10:44:07.243418827 +0200 @@ -314,7 +314,7 @@ if (err) return err; vco_retries = 0; - while (!(vco_cal & FC11_VCOCAL_OK) && vco_retries < 6) { + while (!(vco_cal & FC11_VCOCAL_OK) && vco_retries < 3) { /* Reset the tuner and try again */ err = fe->callback(priv->i2c, DVB_FRONTEND_COMPONENT_TUNER, FC0011_FE_CALLBACK_RESET, priv->addr);