Hi Mauro
I think that your __i2c_transfer() solution is elegant for DVB-C frontend tuner reprogramming sleep cases:
TDA6650 programming manual, page 9 states:
"Main divider data are valid only if no new I2C-bus transmission
is started (START condition) during the computation
period of 50 μs."
http://www.datasheetcatalog.org/datasheet/philips/TDA6650.pdf
It states that after TDA6650 frontend programming, the whole I2C bus
(for any I2C transfer into any I2C device) must be quiet at least 50μs.
My main point for __i2c_transfer() elegance can be seen easilly
from the code examples below.
-----------------
An elegant way to solve only the 50μs TDA6650 sleep problem (not gate control issue),
is to solve it in tda665x.c's tda665x_set_state():
static int tda665x_set_state(struct dvb_frontend *fe,
enum tuner_param param,
struct tuner_state *tstate)
{
...
i2c_lock_adapter(state->i2c);
/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
err = __tda665x_write(state, buf, 5);
usleep(50);
i2c_unlock_adapter(state->i2c);
...
}
Ugly robust way to solveonly the 50μs TDA6650 sleep problem (not gate control issue,
is to teach I2C transfer code TDA6650's requirements:
-- warning: ugly --
saa7146_i2c_transfer(..) {
...
mutex_lock_interruptible(&dev->i2c_lock);
...
send_i2c_messages();
if (num == 5 && is_tda665())
usleep(50);
mutex_unlock(&dev->i2c_lock);
}
Here is a robust version with __i2c_transfer(). Main idea is to
protect the whole communicating with I2C Frontend device with I2C lock:
- Open I2C Gate so that you can talk to DVB Frontend Tuner.
- Send 5 bytes into DVB-C Frontend tuner.
- Quiesce I2C bus by sleeping 50μs to protect the Frontend Tuner message.
- Close I2C Gate so that DVB Frontend Tuner isn't reachable by I2C bus.
static int tda665x_set_state(struct dvb_frontend *fe,
enum tuner_param param,
struct tuner_state *tstate)
{
...
i2c_lock_adapter(state->i2c);
__open_i2c_gate(state);
/* Set params without taking I2C lock ( uses __i2c_transfer() ) */
err = __tda665x_write(state, buf, 5);
usleep(50);
__close_i2c_gate(state);
i2c_unlock_adapter(state->i2c);
...
}
-- Ugly solution for the same full problem in SAA7146 I2C transfer function itself (code with some unsolved problems) --
saa7146_i2c_transfer(..) {
...
mutex_lock_interruptible(&dev->i2c_lock);
if (is_tda665(addr))
send_msg_tda665_gate_open(dev);
...
send_i2c_messages();
if (num == 5 && is_tda665())
usleep(50);
if (is_tda665(addr))
send_msg_tda665_gate_close(dev);
mutex_unlock(&dev->i2c_lock);
}
Regards,
Marko Ristola
On 06/21/2012 10:10 PM, Mauro Carvalho Chehab wrote:
Em 21-06-2012 10:36, Mauro Carvalho Chehab escreveu:
The firmware blob may not be available when the driver probes.
Instead of blocking the whole kernel use request_firmware_nowait() and
continue without firmware.
This shouldn't be that bad on drx-k devices, as they all seem to have an
internal firmware. So, only the firmware update will take a little longer
to happen.
While thinking on converting another DVB frontend driver, I just realized
that a patch like that won't work fine.
As most of you know, there are _several_ I2C chips that don't tolerate any
usage of the I2C bus while a firmware is being loaded (I dunno if this is the
case of drx-k, but I won't doubt).
The current approach makes the device probe() logic is serialized. So, there's
no chance that two different I2C drivers to try to access the bus at the same
time, if the bridge driver is properly implemented.
However, now that firmware is loaded asynchronously, several other I2C drivers
may be trying to use the bus at the same time. So, events like IR (and CI) polling,
tuner get_status, etc can happen during a firmware transfer, causing the firmware
to not load properly.
A fix for that will require to lock the firmware load I2C traffic into a single
transaction.
So, the code that sends the firmware to the board need to be changed to something like:
static int DownloadMicrocode(struct drxk_state *state,
const u8 pMCImage[], u32 Length)
{
...
i2c_lock_adapter(state->i2c);
...
for (i = 0; i < nBlocks; i += 1) {
...
status = __i2c_transfer(state->i2c, ...);
...
}
i2c_unlock_adapter(state->i2c);
return status;
}
where __i2c_transfer() would be a variant of i2c_transfer() that won't take the
I2C lock.
Other drivers that load firmware at I2C and use request_firmware_nowait() will also
need a similar approach.
Jean,
What do you think?
Regards,
Mauro
Cc: Antti Palosaari <crope@xxxxxx>
Cc: Kay Sievers <kay@xxxxxxxxxx>
Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
---
drivers/media/dvb/frontends/drxk_hard.c | 109 +++++++++++++++++++------------
drivers/media/dvb/frontends/drxk_hard.h | 3 +
2 files changed, 72 insertions(+), 40 deletions(-)
diff --git a/drivers/media/dvb/frontends/drxk_hard.c b/drivers/media/dvb/frontends/drxk_hard.c
index 60b868f..4cb8d1e 100644
--- a/drivers/media/dvb/frontends/drxk_hard.c
+++ b/drivers/media/dvb/frontends/drxk_hard.c
@@ -5968,29 +5968,9 @@ error:
return status;
}
-static int load_microcode(struct drxk_state *state, const char *mc_name)
-{
- const struct firmware *fw = NULL;
- int err = 0;
-
- dprintk(1, "\n");
-
- err = request_firmware(&fw, mc_name, state->i2c->dev.parent);
- if (err < 0) {
- printk(KERN_ERR
- "drxk: Could not load firmware file %s.\n", mc_name);
- printk(KERN_INFO
- "drxk: Copy %s to your hotplug directory!\n", mc_name);
- return err;
- }
- err = DownloadMicrocode(state, fw->data, fw->size);
- release_firmware(fw);
- return err;
-}
-
static int init_drxk(struct drxk_state *state)
{
- int status = 0;
+ int status = 0, n = 0;
enum DRXPowerMode powerMode = DRXK_POWER_DOWN_OFDM;
u16 driverVersion;
@@ -6073,8 +6053,12 @@ static int init_drxk(struct drxk_state *state)
if (status < 0)
goto error;
- if (state->microcode_name)
- load_microcode(state, state->microcode_name);
+ if (state->fw) {
+ status = DownloadMicrocode(state, state->fw->data,
+ state->fw->size);
+ if (status < 0)
+ goto error;
+ }
/* disable token-ring bus through OFDM block for possible ucode upload */
status = write16(state, SIO_OFDM_SH_OFDM_RING_ENABLE__A, SIO_OFDM_SH_OFDM_RING_ENABLE_OFF);
@@ -6167,6 +6151,20 @@ static int init_drxk(struct drxk_state *state)
state->m_DrxkState = DRXK_POWERED_DOWN;
} else
state->m_DrxkState = DRXK_STOPPED;
+
+ /* Initialize the supported delivery systems */
+ n = 0;
+ if (state->m_hasDVBC) {
+ state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
+ state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
+ strlcat(state->frontend.ops.info.name, " DVB-C",
+ sizeof(state->frontend.ops.info.name));
+ }
+ if (state->m_hasDVBT) {
+ state->frontend.ops.delsys[n++] = SYS_DVBT;
+ strlcat(state->frontend.ops.info.name, " DVB-T",
+ sizeof(state->frontend.ops.info.name));
+ }
}
error:
if (status < 0)
@@ -6175,11 +6173,44 @@ error:
return status;
}
+static void load_firmware_cb(const struct firmware *fw,
+ void *context)
+{
+ struct drxk_state *state = context;
+
+ if (!fw) {
+ printk(KERN_ERR
+ "drxk: Could not load firmware file %s.\n",
+ state->microcode_name);
+ printk(KERN_INFO
+ "drxk: Copy %s to your hotplug directory!\n",
+ state->microcode_name);
+ state->microcode_name = NULL;
+
+ /*
+ * As firmware is now load asynchronous, it is not possible
+ * anymore to fail at frontend attach. We might silently
+ * return here, and hope that the driver won't crash.
+ * We might also change all DVB callbacks to return -ENODEV
+ * if the device is not initialized.
+ * As the DRX-K devices have their own internal firmware,
+ * let's just hope that it will match a firmware revision
+ * compatible with this driver and proceed.
+ */
+ }
+ state->fw = fw;
+
+ init_drxk(state);
+}
+
static void drxk_release(struct dvb_frontend *fe)
{
struct drxk_state *state = fe->demodulator_priv;
dprintk(1, "\n");
+ if (state->fw)
+ release_firmware(state->fw);
+
kfree(state);
}
@@ -6371,10 +6402,9 @@ static struct dvb_frontend_ops drxk_ops = {
struct dvb_frontend *drxk_attach(const struct drxk_config *config,
struct i2c_adapter *i2c)
{
- int n;
-
struct drxk_state *state = NULL;
u8 adr = config->adr;
+ int status;
dprintk(1, "\n");
state = kzalloc(sizeof(struct drxk_state), GFP_KERNEL);
@@ -6425,22 +6455,21 @@ struct dvb_frontend *drxk_attach(const struct drxk_config *config,
state->frontend.demodulator_priv = state;
init_state(state);
- if (init_drxk(state) < 0)
- goto error;
- /* Initialize the supported delivery systems */
- n = 0;
- if (state->m_hasDVBC) {
- state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_A;
- state->frontend.ops.delsys[n++] = SYS_DVBC_ANNEX_C;
- strlcat(state->frontend.ops.info.name, " DVB-C",
- sizeof(state->frontend.ops.info.name));
- }
- if (state->m_hasDVBT) {
- state->frontend.ops.delsys[n++] = SYS_DVBT;
- strlcat(state->frontend.ops.info.name, " DVB-T",
- sizeof(state->frontend.ops.info.name));
- }
+ /* Load firmware and initialize DRX-K */
+ if (state->microcode_name) {
+ status = request_firmware_nowait(THIS_MODULE, 1,
+ state->microcode_name,
+ state->i2c->dev.parent,
+ GFP_KERNEL,
+ state, load_firmware_cb);
+ if (status < 0) {
+ printk(KERN_ERR
+ "drxk: failed to request a firmware\n");
+ return NULL;
+ }
+ } else if (init_drxk(state) < 0)
+ goto error;
printk(KERN_INFO "drxk: frontend initialized.\n");
return &state->frontend;
diff --git a/drivers/media/dvb/frontends/drxk_hard.h b/drivers/media/dvb/frontends/drxk_hard.h
index 4bbf841..36677cd 100644
--- a/drivers/media/dvb/frontends/drxk_hard.h
+++ b/drivers/media/dvb/frontends/drxk_hard.h
@@ -338,7 +338,10 @@ struct drxk_state {
bool antenna_dvbt;
u16 antenna_gpio;
+ /* Firmware */
const char *microcode_name;
+ struct completion fw_wait_load;
+ const struct firmware *fw;
};
#define NEVER_LOCK 0
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
