From: Hans Verkuil <hverkuil@xs4all.nl>
To: "edubezval@gmail.com" <edubezval@gmail.com>
Cc: Dinesh Ram <dinram@cisco.com>,
Linux-Media <linux-media@vger.kernel.org>,
dinesh.ram@cern.ch
Subject: Re: [PATCH 2/6] si4713 : Modified i2c driver to handle cases where interrupts are not used
Date: Sun, 01 Sep 2013 13:00:48 +0200 [thread overview]
Message-ID: <52231E60.80308@xs4all.nl> (raw)
In-Reply-To: <CAC-25o_rpw3w1vBomYfYGZpr=bRV=FG8yrqruy-wgEZtcCXLCg@mail.gmail.com>
On 08/31/2013 01:32 PM, edubezval@gmail.com wrote:
> On Fri, Aug 30, 2013 at 7:28 AM, Dinesh Ram <dinram@cisco.com> wrote:
>> Checks have been introduced at several places in the code to test if an interrupt is set or not.
>> For devices which do not use the interrupt, to get a valid response, within a specified timeout,
>> the device is polled instead.
>>
>> Signed-off-by: Dinesh Ram <dinram@cisco.com>
>> ---
>> drivers/media/radio/si4713/si4713.c | 110 ++++++++++++++++++++----------------
>> drivers/media/radio/si4713/si4713.h | 1 +
>> 2 files changed, 63 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/media/radio/si4713/si4713.c b/drivers/media/radio/si4713/si4713.c
>> index ac727e3..55c4d27 100644
>> --- a/drivers/media/radio/si4713/si4713.c
>> +++ b/drivers/media/radio/si4713/si4713.c
>> @@ -27,7 +27,6 @@
>> #include <linux/i2c.h>
>> #include <linux/slab.h>
>> #include <linux/gpio.h>
>> -#include <linux/regulator/consumer.h>
>> #include <linux/module.h>
>> #include <media/v4l2-device.h>
>> #include <media/v4l2-ioctl.h>
>> @@ -213,6 +212,7 @@ static int si4713_send_command(struct si4713_device *sdev, const u8 command,
>> u8 response[], const int respn, const int usecs)
>> {
>> struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
>> + unsigned long until_jiffies;
>> u8 data1[MAX_ARGS + 1];
>> int err;
>>
>> @@ -228,30 +228,39 @@ static int si4713_send_command(struct si4713_device *sdev, const u8 command,
>> if (err != argn + 1) {
>> v4l2_err(&sdev->sd, "Error while sending command 0x%02x\n",
>> command);
>> - return (err > 0) ? -EIO : err;
>> + return err < 0 ? err : -EIO;
>> }
>>
>> + until_jiffies = jiffies + usecs_to_jiffies(usecs) + 1;
>> +
>> /* Wait response from interrupt */
>> - if (!wait_for_completion_timeout(&sdev->work,
>> + if (client->irq) {
>> + if (!wait_for_completion_timeout(&sdev->work,
>> usecs_to_jiffies(usecs) + 1))
>> - v4l2_warn(&sdev->sd,
>> + v4l2_warn(&sdev->sd,
>> "(%s) Device took too much time to answer.\n",
>> __func__);
>> -
>> - /* Then get the response */
>> - err = i2c_master_recv(client, response, respn);
>> - if (err != respn) {
>> - v4l2_err(&sdev->sd,
>> - "Error while reading response for command 0x%02x\n",
>> - command);
>> - return (err > 0) ? -EIO : err;
>> }
>>
>> - DBG_BUFFER(&sdev->sd, "Response", response, respn);
>> - if (check_command_failed(response[0]))
>> - return -EBUSY;
>> + do {
>> + err = i2c_master_recv(client, response, respn);
>> + if (err != respn) {
>> + v4l2_err(&sdev->sd,
>> + "Error %d while reading response for command 0x%02x\n",
>> + err, command);
>> + return err < 0 ? err : -EIO;
>> + }
>>
>> - return 0;
>> + DBG_BUFFER(&sdev->sd, "Response", response, respn);
>> + if (!check_command_failed(response[0]))
>> + return 0;
>> +
>> + if (client->irq)
>> + return -EBUSY;
>> + msleep(1);
>> + } while (jiffies <= until_jiffies);
>> +
>> + return -EBUSY;
>> }
>>
>> /*
>> @@ -344,14 +353,15 @@ static int si4713_write_property(struct si4713_device *sdev, u16 prop, u16 val)
>> */
>> static int si4713_powerup(struct si4713_device *sdev)
>> {
>> + struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
>> int err;
>> u8 resp[SI4713_PWUP_NRESP];
>> /*
>> * .First byte = Enabled interrupts and boot function
>> * .Second byte = Input operation mode
>> */
>> - const u8 args[SI4713_PWUP_NARGS] = {
>> - SI4713_PWUP_CTSIEN | SI4713_PWUP_GPO2OEN | SI4713_PWUP_FUNC_TX,
>> + u8 args[SI4713_PWUP_NARGS] = {
>> + SI4713_PWUP_GPO2OEN | SI4713_PWUP_FUNC_TX,
>> SI4713_PWUP_OPMOD_ANALOG,
>> };
>>
>> @@ -369,18 +379,22 @@ static int si4713_powerup(struct si4713_device *sdev)
>> gpio_set_value(sdev->gpio_reset, 1);
>> }
>>
>> + if (client->irq)
>> + args[0] |= SI4713_PWUP_CTSIEN;
>> +
>> err = si4713_send_command(sdev, SI4713_CMD_POWER_UP,
>> args, ARRAY_SIZE(args),
>> resp, ARRAY_SIZE(resp),
>> TIMEOUT_POWER_UP);
>> -
>> +
>> if (!err) {
>> v4l2_dbg(1, debug, &sdev->sd, "Powerup response: 0x%02x\n",
>> resp[0]);
>> v4l2_dbg(1, debug, &sdev->sd, "Device in power up mode\n");
>> sdev->power_state = POWER_ON;
>>
>> - err = si4713_write_property(sdev, SI4713_GPO_IEN,
>> + if (client->irq)
>> + err = si4713_write_property(sdev, SI4713_GPO_IEN,
>> SI4713_STC_INT | SI4713_CTS);
>> } else {
>> if (gpio_is_valid(sdev->gpio_reset))
>> @@ -447,7 +461,7 @@ static int si4713_checkrev(struct si4713_device *sdev)
>> if (rval < 0)
>> return rval;
>>
>> - if (resp[1] == SI4713_PRODUCT_NUMBER) {
>> + if (resp[1] == SI4713_PRODUCT_NUMBER) {
>> v4l2_info(&sdev->sd, "chip found @ 0x%02x (%s)\n",
>> client->addr << 1, client->adapter->name);
>> } else {
>> @@ -465,33 +479,34 @@ static int si4713_checkrev(struct si4713_device *sdev)
>> */
>> static int si4713_wait_stc(struct si4713_device *sdev, const int usecs)
>> {
>> - int err;
>> + struct i2c_client *client = v4l2_get_subdevdata(&sdev->sd);
>> u8 resp[SI4713_GET_STATUS_NRESP];
>> -
>> - /* Wait response from STC interrupt */
>> - if (!wait_for_completion_timeout(&sdev->work,
>> - usecs_to_jiffies(usecs) + 1))
>> - v4l2_warn(&sdev->sd,
>> - "%s: device took too much time to answer (%d usec).\n",
>> - __func__, usecs);
>> -
>> - /* Clear status bits */
>> - err = si4713_send_command(sdev, SI4713_CMD_GET_INT_STATUS,
>> - NULL, 0,
>> - resp, ARRAY_SIZE(resp),
>> - DEFAULT_TIMEOUT);
>> -
>> - if (err < 0)
>> - goto exit;
>> -
>> - v4l2_dbg(1, debug, &sdev->sd,
>> - "%s: status bits: 0x%02x\n", __func__, resp[0]);
>> -
>> - if (!(resp[0] & SI4713_STC_INT))
>> - err = -EIO;
>> -
>> -exit:
>> - return err;
>> + unsigned long start_jiffies = jiffies;
>> + int err;
>> +
>> + if (client->irq &&
>> + !wait_for_completion_timeout(&sdev->work, usecs_to_jiffies(usecs) + 1))
>> + v4l2_warn(&sdev->sd,
>> + "(%s) Device took too much time to answer.\n", __func__);
>> +
>> + for (;;) {
>> + /* Clear status bits */
>> + err = si4713_send_command(sdev, SI4713_CMD_GET_INT_STATUS,
>> + NULL, 0,
>> + resp, ARRAY_SIZE(resp),
>> + DEFAULT_TIMEOUT);
>> +
>> + if (err >= 0) {
>> + v4l2_dbg(1, debug, &sdev->sd,
>> + "%s: status bits: 0x%02x\n", __func__, resp[0]);
>> +
>> + if (resp[0] & SI4713_STC_INT)
>> + return 0;
>> + }
>> + if (jiffies_to_usecs(jiffies - start_jiffies) > usecs)
>> + return -EIO;
>> + msleep(3);
>> + }
>> }
>>
>> /*
>> @@ -1024,7 +1039,6 @@ static int si4713_initialize(struct si4713_device *sdev)
>> if (rval < 0)
>> return rval;
>>
>> -
>> sdev->frequency = DEFAULT_FREQUENCY;
>> sdev->stereo = 1;
>> sdev->tune_rnl = DEFAULT_TUNE_RNL;
>> diff --git a/drivers/media/radio/si4713/si4713.h b/drivers/media/radio/si4713/si4713.h
>> index c274e1f..dc0ce66 100644
>> --- a/drivers/media/radio/si4713/si4713.h
>> +++ b/drivers/media/radio/si4713/si4713.h
>> @@ -15,6 +15,7 @@
>> #ifndef SI4713_I2C_H
>> #define SI4713_I2C_H
>>
>> +#include <linux/regulator/consumer.h>
>
> Forgot to ask, why do you need to have regulator/consumer.h in this
> header to add polling mode to this driver.
The header include was moved from the C-source to the header since the header
needs it. This header will also be included by the USB driver and it is silly
if the USB driver would have to include consumer.h when the si4713.h should do it.
If you want it can be split off to a separate patch.
Regards,
Hans
next prev parent reply other threads:[~2013-09-01 11:01 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-30 11:28 [PATCH 0/6] si4713 : USB driver Dinesh Ram
2013-08-30 11:28 ` [PATCH 1/6] si4713 : Reorganized drivers/media/radio directory Dinesh Ram
2013-08-30 11:28 ` [PATCH 2/6] si4713 : Modified i2c driver to handle cases where interrupts are not used Dinesh Ram
2013-08-31 11:31 ` edubezval
2013-09-01 10:57 ` Hans Verkuil
2013-09-01 14:45 ` edubezval
2013-09-02 7:11 ` Hans Verkuil
2013-09-02 10:29 ` Dinesh Ram
2013-09-03 12:50 ` edubezval
2013-09-30 13:07 ` Hans Verkuil
2013-09-30 14:32 ` Dinesh Ram
[not found] ` <1378046534.45961.YahooMailNeo@web190905.mail.sg3.yahoo.com>
2013-09-01 14:51 ` edubezval
2013-08-31 11:32 ` edubezval
2013-09-01 11:00 ` Hans Verkuil [this message]
2013-09-01 14:47 ` edubezval
2013-08-30 11:28 ` [PATCH 3/6] si4713 : Bug fix for si4713_tx_tune_power() method in the i2c driver Dinesh Ram
2013-08-31 11:49 ` edubezval
2013-09-01 11:04 ` Hans Verkuil
2013-09-01 14:57 ` edubezval
2013-09-02 6:59 ` Hans Verkuil
2013-09-02 7:15 ` Hans Verkuil
2013-08-30 11:28 ` [PATCH 4/6] si4713 : HID blacklist Si4713 USB development board Dinesh Ram
2013-08-30 11:47 ` Jiri Kosina
2013-09-02 9:23 ` Jiri Kosina
2013-08-30 11:28 ` [PATCH 5/6] si4713 : Added the USB driver for Si4713 Dinesh Ram
2013-09-02 3:13 ` edubezval
2013-08-30 11:28 ` [PATCH 6/6] si4713 : Added MAINTAINERS entry for radio-usb-si4713 driver Dinesh Ram
2013-08-30 12:07 ` Hans Verkuil
2013-08-31 11:38 ` [PATCH 1/6] si4713 : Reorganized drivers/media/radio directory edubezval
2013-08-30 12:09 ` [PATCH 0/6] si4713 : USB driver Hans Verkuil
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52231E60.80308@xs4all.nl \
--to=hverkuil@xs4all.nl \
--cc=dinesh.ram@cern.ch \
--cc=dinram@cisco.com \
--cc=edubezval@gmail.com \
--cc=linux-media@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox