On Fri, Sep 28, 2012 at 2:31 PM, Antti Palosaari <crope@xxxxxx> wrote:
> Hello,
> Did not fix the issue. Problem remains same. With the sleep + that patch it
> works still.
>
> On 09/28/2012 06:04 PM, Mauro Carvalho Chehab wrote:
>>
>> The tda18271 datasheet says:
>>
>> "The image rejection calibration and RF tracking filter
>> calibration must be launched exactly as described in the
>> flowchart, otherwise bad calibration or even blocking of the
>> TDA18211HD can result making it impossible to communicate
>> via the I2C-bus."
>>
>> (yeah, tda18271 refers there to tda18211 - likely a typo at their
>> datasheets)
>
>
> tda18211 is just same than tda18271 but without a analog.
>
>> That likely explains why sometimes tda18271 stops answering. That
>> is now happening more often on designs with drx-k chips, as the
>> firmware is now loaded asyncrousnly there.
>>
>> While the above text doesn't explicitly tell that the I2C bus
>> couldn't be used by other devices during such initialization,
>> that seems to be a requirement there.
>>
>> So, let's explicitly use the I2C lock there, avoiding I2C bus
>> share during those critical moments.
>>
>> Compile-tested only. Please test.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>> ---
>> drivers/media/tuners/tda18271-common.c | 104
>> ++++++++++++++++++++++-----------
>> 1 file changed, 71 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/media/tuners/tda18271-common.c
>> b/drivers/media/tuners/tda18271-common.c
>> index 221171e..18c77af 100644
>> --- a/drivers/media/tuners/tda18271-common.c
>> +++ b/drivers/media/tuners/tda18271-common.c
>> @@ -187,7 +187,8 @@ int tda18271_read_extended(struct dvb_frontend *fe)
>> return (ret == 2 ? 0 : ret);
>> }
>>
>> -int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
>> +static int __tda18271_write_regs(struct dvb_frontend *fe, int idx, int
>> len,
>> + bool lock_i2c)
>> {
>> struct tda18271_priv *priv = fe->tuner_priv;
>> unsigned char *regs = priv->tda18271_regs;
>> @@ -198,7 +199,6 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
>> idx, int len)
>>
>> BUG_ON((len == 0) || (idx + len > sizeof(buf)));
>>
>> -
>> switch (priv->small_i2c) {
>> case TDA18271_03_BYTE_CHUNK_INIT:
>> max = 3;
>> @@ -214,7 +214,19 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
>> idx, int len)
>> max = 39;
>> }
>>
>> - tda18271_i2c_gate_ctrl(fe, 1);
>> +
>> + /*
>> + * If lock_i2c is true, it will take the I2C bus for tda18271
>> private
>> + * usage during the entire write ops, as otherwise, bad things
>> could
>> + * happen.
>> + * During device init, several write operations will happen. So,
>> + * tda18271_init_regs controls the I2C lock directly,
>> + * disabling lock_i2c here.
>> + */
>> + if (lock_i2c) {
>> + tda18271_i2c_gate_ctrl(fe, 1);
>> + i2c_lock_adapter(priv->i2c_props.adap);
>> + }
>> while (len) {
>> if (max > len)
>> max = len;
>> @@ -226,14 +238,17 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
>> idx, int len)
>> msg.len = max + 1;
>>
>> /* write registers */
>> - ret = i2c_transfer(priv->i2c_props.adap, &msg, 1);
>> + ret = __i2c_transfer(priv->i2c_props.adap, &msg, 1);
>> if (ret != 1)
>> break;
>>
>> idx += max;
>> len -= max;
>> }
>> - tda18271_i2c_gate_ctrl(fe, 0);
>> + if (lock_i2c) {
>> + i2c_unlock_adapter(priv->i2c_props.adap);
>> + tda18271_i2c_gate_ctrl(fe, 0);
>> + }
>>
>> if (ret != 1)
>> tda_err("ERROR: idx = 0x%x, len = %d, "
>> @@ -242,10 +257,16 @@ int tda18271_write_regs(struct dvb_frontend *fe, int
>> idx, int len)
>> return (ret == 1 ? 0 : ret);
>> }
>>
>> +int tda18271_write_regs(struct dvb_frontend *fe, int idx, int len)
>> +{
>> + return __tda18271_write_regs(fe, idx, len, true);
>> +}
>> +
>>
>> /*---------------------------------------------------------------------*/
>>
>> -int tda18271_charge_pump_source(struct dvb_frontend *fe,
>> - enum tda18271_pll pll, int force)
>> +static int __tda18271_charge_pump_source(struct dvb_frontend *fe,
>> + enum tda18271_pll pll, int force,
>> + bool lock_i2c)
>> {
>> struct tda18271_priv *priv = fe->tuner_priv;
>> unsigned char *regs = priv->tda18271_regs;
>> @@ -255,9 +276,16 @@ int tda18271_charge_pump_source(struct dvb_frontend
>> *fe,
>> regs[r_cp] &= ~0x20;
>> regs[r_cp] |= ((force & 1) << 5);
>>
>> - return tda18271_write_regs(fe, r_cp, 1);
>> + return __tda18271_write_regs(fe, r_cp, 1, lock_i2c);
>> +}
>> +
>> +int tda18271_charge_pump_source(struct dvb_frontend *fe,
>> + enum tda18271_pll pll, int force)
>> +{
>> + return __tda18271_charge_pump_source(fe, pll, force, true);
>> }
>>
>> +
>> int tda18271_init_regs(struct dvb_frontend *fe)
>> {
>> struct tda18271_priv *priv = fe->tuner_priv;
>> @@ -267,6 +295,13 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> i2c_adapter_id(priv->i2c_props.adap),
>> priv->i2c_props.addr);
>>
>> + /*
>> + * Don't let any other I2C transfer to happen at adapter during
>> init,
>> + * as those could cause bad things
>> + */
>> + tda18271_i2c_gate_ctrl(fe, 1);
>> + i2c_lock_adapter(priv->i2c_props.adap);
>> +
>> /* initialize registers */
>> switch (priv->id) {
>> case TDA18271HDC1:
>> @@ -352,28 +387,28 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_EB22] = 0x48;
>> regs[R_EB23] = 0xb0;
>>
>> - tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS);
>> + __tda18271_write_regs(fe, 0x00, TDA18271_NUM_REGS, false);
>>
>> /* setup agc1 gain */
>> regs[R_EB17] = 0x00;
>> - tda18271_write_regs(fe, R_EB17, 1);
>> + __tda18271_write_regs(fe, R_EB17, 1, false);
>> regs[R_EB17] = 0x03;
>> - tda18271_write_regs(fe, R_EB17, 1);
>> + __tda18271_write_regs(fe, R_EB17, 1, false);
>> regs[R_EB17] = 0x43;
>> - tda18271_write_regs(fe, R_EB17, 1);
>> + __tda18271_write_regs(fe, R_EB17, 1, false);
>> regs[R_EB17] = 0x4c;
>> - tda18271_write_regs(fe, R_EB17, 1);
>> + __tda18271_write_regs(fe, R_EB17, 1, false);
>>
>> /* setup agc2 gain */
>> if ((priv->id) == TDA18271HDC1) {
>> regs[R_EB20] = 0xa0;
>> - tda18271_write_regs(fe, R_EB20, 1);
>> + __tda18271_write_regs(fe, R_EB20, 1, false);
>> regs[R_EB20] = 0xa7;
>> - tda18271_write_regs(fe, R_EB20, 1);
>> + __tda18271_write_regs(fe, R_EB20, 1, false);
>> regs[R_EB20] = 0xe7;
>> - tda18271_write_regs(fe, R_EB20, 1);
>> + __tda18271_write_regs(fe, R_EB20, 1, false);
>> regs[R_EB20] = 0xec;
>> - tda18271_write_regs(fe, R_EB20, 1);
>> + __tda18271_write_regs(fe, R_EB20, 1, false);
>> }
>>
>> /* image rejection calibration */
>> @@ -391,21 +426,21 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_MD2] = 0x08;
>> regs[R_MD3] = 0x00;
>>
>> - tda18271_write_regs(fe, R_EP3, 11);
>> + __tda18271_write_regs(fe, R_EP3, 11, false);
>>
>> if ((priv->id) == TDA18271HDC2) {
>> /* main pll cp source on */
>> - tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1);
>> + __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 1,
>> false);
>> msleep(1);
>>
>> /* main pll cp source off */
>> - tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0);
>> + __tda18271_charge_pump_source(fe, TDA18271_MAIN_PLL, 0,
>> false);
>> }
>>
>> msleep(5); /* pll locking */
>>
>> /* launch detector */
>> - tda18271_write_regs(fe, R_EP1, 1);
>> + __tda18271_write_regs(fe, R_EP1, 1, false);
>> msleep(5); /* wanted low measurement */
>>
>> regs[R_EP5] = 0x85;
>> @@ -413,11 +448,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_CD1] = 0x66;
>> regs[R_CD2] = 0x70;
>>
>> - tda18271_write_regs(fe, R_EP3, 7);
>> + __tda18271_write_regs(fe, R_EP3, 7, false);
>> msleep(5); /* pll locking */
>>
>> /* launch optimization algorithm */
>> - tda18271_write_regs(fe, R_EP2, 1);
>> + __tda18271_write_regs(fe, R_EP2, 1, false);
>> msleep(30); /* image low optimization completion */
>>
>> /* mid-band */
>> @@ -428,11 +463,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_MD1] = 0x73;
>> regs[R_MD2] = 0x1a;
>>
>> - tda18271_write_regs(fe, R_EP3, 11);
>> + __tda18271_write_regs(fe, R_EP3, 11, false);
>> msleep(5); /* pll locking */
>>
>> /* launch detector */
>> - tda18271_write_regs(fe, R_EP1, 1);
>> + __tda18271_write_regs(fe, R_EP1, 1, false);
>> msleep(5); /* wanted mid measurement */
>>
>> regs[R_EP5] = 0x86;
>> @@ -440,11 +475,11 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_CD1] = 0x66;
>> regs[R_CD2] = 0xa0;
>>
>> - tda18271_write_regs(fe, R_EP3, 7);
>> + __tda18271_write_regs(fe, R_EP3, 7, false);
>> msleep(5); /* pll locking */
>>
>> /* launch optimization algorithm */
>> - tda18271_write_regs(fe, R_EP2, 1);
>> + __tda18271_write_regs(fe, R_EP2, 1, false);
>> msleep(30); /* image mid optimization completion */
>>
>> /* high-band */
>> @@ -456,30 +491,33 @@ int tda18271_init_regs(struct dvb_frontend *fe)
>> regs[R_MD1] = 0x71;
>> regs[R_MD2] = 0xcd;
>>
>> - tda18271_write_regs(fe, R_EP3, 11);
>> + __tda18271_write_regs(fe, R_EP3, 11, false);
>> msleep(5); /* pll locking */
>>
>> /* launch detector */
>> - tda18271_write_regs(fe, R_EP1, 1);
>> + __tda18271_write_regs(fe, R_EP1, 1, false);
>> msleep(5); /* wanted high measurement */
>>
>> regs[R_EP5] = 0x87;
>> regs[R_CD1] = 0x65;
>> regs[R_CD2] = 0x50;
>>
>> - tda18271_write_regs(fe, R_EP3, 7);
>> + __tda18271_write_regs(fe, R_EP3, 7, false);
>> msleep(5); /* pll locking */
>>
>> /* launch optimization algorithm */
>> - tda18271_write_regs(fe, R_EP2, 1);
>> + __tda18271_write_regs(fe, R_EP2, 1, false);
>> msleep(30); /* image high optimization completion */
>>
>> /* return to normal mode */
>> regs[R_EP4] = 0x64;
>> - tda18271_write_regs(fe, R_EP4, 1);
>> + __tda18271_write_regs(fe, R_EP4, 1, false);
>>
>> /* synchronize */
>> - tda18271_write_regs(fe, R_EP1, 1);
>> + __tda18271_write_regs(fe, R_EP1, 1, false);
>> +
>> + i2c_unlock_adapter(priv->i2c_props.adap);
>> + tda18271_i2c_gate_ctrl(fe, 0);
>>
>> return 0;
>> }
>>
>
>
> --
> http://palosaari.fi/
I have to NACK this particular patch -- I saw Mauro's email about the
locked i2c -- it's a great idea. In fact, it is the original use case
for adding this functionality into i2c, I just never got around to
implementing it in the tda18271 driver. However, it shouldnt be
around *all* i2c transactions -- it should only lock the i2c bus
during the *critical* section. Please wait for me to send a new patch
for testing. I'll try to get to it before the end of the weekend.
(hopefully tonight or tomorrow morning)
-Mike
--
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
