From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from lists.s-osg.org ([54.187.51.154]:57197 "EHLO lists.s-osg.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755206AbbHKPur (ORCPT ); Tue, 11 Aug 2015 11:50:47 -0400 Date: Tue, 11 Aug 2015 12:50:42 -0300 From: Mauro Carvalho Chehab To: Antti Palosaari Cc: linux-media@vger.kernel.org Subject: Re: [PATCH 09/12] tda10071: use jiffies when poll firmware status Message-ID: <20150811125042.594967d6@recife.lan> In-Reply-To: <55CA124F.9080507@iki.fi> References: <1436414792-9716-1-git-send-email-crope@iki.fi> <1436414792-9716-9-git-send-email-crope@iki.fi> <20150811072055.55eeb0d4@recife.lan> <55CA124F.9080507@iki.fi> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org List-ID: Em Tue, 11 Aug 2015 18:18:39 +0300 Antti Palosaari escreveu: > On 08/11/2015 01:20 PM, Mauro Carvalho Chehab wrote: > > Em Thu, 9 Jul 2015 07:06:29 +0300 > > Antti Palosaari escreveu: > > > >> Use jiffies to set timeout for firmware command status polling. > >> It is more elegant solution than poll X times with sleep. > > >> /* wait cmd execution terminate */ > >> - for (i = 1000, uitmp = 1; i && uitmp; i--) { > >> + #define CMD_EXECUTE_TIMEOUT 30 > >> + timeout = jiffies + msecs_to_jiffies(CMD_EXECUTE_TIMEOUT); > >> + for (uitmp = 1; !time_after(jiffies, timeout) && uitmp;) { > >> ret = regmap_read(dev->regmap, 0x1f, &uitmp); > >> if (ret) > >> goto error; > >> - > >> - usleep_range(200, 5000); > > > > Hmm... removing the usleep() doesn't sound a good idea. You'll be > > flooding the I2C bus with read commands and spending CPU cycles > > for 30ms spending more power than the previous code. That doesn't > > sound more "elegant solution than poll X times with sleep" for me. > > > > So, I would keep the usleep_range() here and add a better > > comment on the patch description. > > First of all, polling firmware ready status is very common for chips > having firmware. And there is 2 ways to implement it: > 1) poll N times in a loop using X sleep, timeout = N * X > 2) poll in a loop using jiffies as a timeout > > IMHO 2 is more elegant solution and I have started using it recently. Yes, (2) is more elegant. > What you now propose is add some throttle in order to slow down polling > interval to reduce I2C I/O. Yes sure less I/O is better, but downside is > that it makes some unneeded extra delay to code path. Usually these sort > firmware ready polling ends rather quickly, in a loop or two. If only few interactions is needed, then OK. Please add a comment then, explaining that. > > Sure it eats some extra CPU cycles, but I think extra control messages > are about nothing compared to I/O used for data streaming. > > Which kind of throttle delay you think is suitable for polling command > status over I2C bus? > > regards > Antti >