| Message ID | 1436414792-9716-9-git-send-email-crope@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Thu, 9 Jul 2015 07:06:29 +0300 Antti Palosaari <crope@iki.fi> escreveu: > Use jiffies to set timeout for firmware command status polling. > It is more elegant solution than poll X times with sleep. > > Shorten timeout to 30ms as all commands seems to be executed under > 10ms. > > Signed-off-by: Antti Palosaari <crope@iki.fi> > --- > drivers/media/dvb-frontends/tda10071.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/media/dvb-frontends/tda10071.c b/drivers/media/dvb-frontends/tda10071.c > index 6226b57..c1507cc 100644 > --- a/drivers/media/dvb-frontends/tda10071.c > +++ b/drivers/media/dvb-frontends/tda10071.c > @@ -53,8 +53,9 @@ static int tda10071_cmd_execute(struct tda10071_dev *dev, > struct tda10071_cmd *cmd) > { > struct i2c_client *client = dev->client; > - int ret, i; > + int ret; > unsigned int uitmp; > + unsigned long timeout; > > if (!dev->warm) { > ret = -EFAULT; > @@ -72,17 +73,19 @@ static int tda10071_cmd_execute(struct tda10071_dev *dev, > goto error; > > /* 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. I'll skip this patch from the git pull request, as, from your description, this is just a cleanup patch. So, it shouldn't affect the other patches at the series. > } > > - dev_dbg(&client->dev, "loop=%d\n", i); > + dev_dbg(&client->dev, "cmd execution took %u ms\n", > + jiffies_to_msecs(jiffies) - > + (jiffies_to_msecs(timeout) - CMD_EXECUTE_TIMEOUT)); > > - if (i == 0) { > + if (uitmp) { > ret = -ETIMEDOUT; > goto error; > } -- 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 08/11/2015 01:20 PM, Mauro Carvalho Chehab wrote: > Em Thu, 9 Jul 2015 07:06:29 +0300 > Antti Palosaari <crope@iki.fi> 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. 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. 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
Em Tue, 11 Aug 2015 18:18:39 +0300 Antti Palosaari <crope@iki.fi> escreveu: > On 08/11/2015 01:20 PM, Mauro Carvalho Chehab wrote: > > Em Thu, 9 Jul 2015 07:06:29 +0300 > > Antti Palosaari <crope@iki.fi> 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 > -- 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/dvb-frontends/tda10071.c b/drivers/media/dvb-frontends/tda10071.c index 6226b57..c1507cc 100644 --- a/drivers/media/dvb-frontends/tda10071.c +++ b/drivers/media/dvb-frontends/tda10071.c @@ -53,8 +53,9 @@ static int tda10071_cmd_execute(struct tda10071_dev *dev, struct tda10071_cmd *cmd) { struct i2c_client *client = dev->client; - int ret, i; + int ret; unsigned int uitmp; + unsigned long timeout; if (!dev->warm) { ret = -EFAULT; @@ -72,17 +73,19 @@ static int tda10071_cmd_execute(struct tda10071_dev *dev, goto error; /* 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); } - dev_dbg(&client->dev, "loop=%d\n", i); + dev_dbg(&client->dev, "cmd execution took %u ms\n", + jiffies_to_msecs(jiffies) - + (jiffies_to_msecs(timeout) - CMD_EXECUTE_TIMEOUT)); - if (i == 0) { + if (uitmp) { ret = -ETIMEDOUT; goto error; }
Use jiffies to set timeout for firmware command status polling. It is more elegant solution than poll X times with sleep. Shorten timeout to 30ms as all commands seems to be executed under 10ms. Signed-off-by: Antti Palosaari <crope@iki.fi> --- drivers/media/dvb-frontends/tda10071.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)