Re: [PATCH 1/4] support for DVBSky dvb-s2 usb: add some config andset_voltage for m88ds3103

[Antti Palosaari]

Moikka!

On 08/07/2014 12:31 PM, nibble.max wrote:
>>> @@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>>> *fe)
>>>
>>> priv->delivery_system = c->delivery_system;
>>>
>>> +   if (priv->cfg->start_ctrl) {
>>> +   for (len = 0; len < 30 ; len++) {
>>> +   m88ds3103_read_status(fe, &status);
>>> +   if (status & FE_HAS_LOCK) {
>>> +   priv->cfg->start_ctrl(fe);
>>> +   break;
>>> +   }
>>> +   msleep(20);
>>> +   }
>>> +   }
>>> +
>>
>> What is idea of that start_ctrl logic? It looks ugly. Why it is needed?
>> What is wrong with default DVB-core implementation? You should not need
>> to poll demod lock here and then call streaming control callback from
>> USB driver. If you implement .streaming_ctrl() callback to DVB USB
>> driver, it is called automatically for you.
> It is nothing with streaming_ctrl of DVB USB driver.
> It relates with the hardware chip problem.
> The m88ds3103 will not output ts clock when it powers up at the first time.
> It starts to output ts clock as soon as it locks the signal.
> But the slave fifo of Cypress usb chip really need this clock to work.
> If there is no this clock, the salve fifo will stop.
> start_ctrl logic is used to restart the salve fifo when the ts clock is valid.

OK. Then we have to find out some solution... Is there anyone who has a
nice idea how to signal USB interface driver when demod gains a lock?
Sure USB driver could poll read_status() too, but it does not sound good
solution neither.

How about overriding FE .read_status() callback. It is called all the
time by DVB-core when frontend is open. Hook .read_status() to USB
interface driver, then call original .read_status() (implemented by
m88ds3103 driver), and after each call check if status is LOCKED or NOT
LOCKED. When status changes from NOT LOCKED to LOCKED call that board
specific routine which restarts TS FIFO. No change for demod driver
needed at all.

regards
Antto

-- 
http://palosaari.fi/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Re: [PATCH 1/4] support for DVBSky dvb-s2 usb: add some config andset_voltage for m88ds3103

[nibble.max]

Moikka!
Thanks for your review.
>Moikka!
>
>That patch contains (too) many changes:
>1) TS clock config option
>2) TS clock polarity config option
>3) start_ctrl() callback
>4) set_voltage implementation
>5) set_voltage() callback
>
>Generally I am fine with 1, 2 and 4.
>
>When you do that many different logical changes for existing driver, you
>should split it is one patch per change.
>
>Rest of the comments are between the code.
>
>
>On 08/06/2014 07:27 AM, nibble.max wrote:
>> Add some config parameters and set_voltage function for m88ds3103.
>> 
>> Signed-off-by: Nibble Max 
>> ---
>>   drivers/media/dvb-frontends/m88ds3103.c | 91 
>> +++--
>>   drivers/media/dvb-frontends/m88ds3103.h | 37 --
>>   2 files changed, 96 insertions(+), 32 deletions(-)
>> 
>> diff --git a/drivers/media/dvb-frontends/m88ds3103.c 
>> b/drivers/media/dvb-frontends/m88ds3103.c
>> index dfe0c2f..df2f89c 100644
>> --- a/drivers/media/dvb-frontends/m88ds3103.c
>> +++ b/drivers/media/dvb-frontends/m88ds3103.c
>> @@ -247,8 +247,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>  u8 u8tmp, u8tmp1, u8tmp2;
>>  u8 buf[2];
>>  u16 u16tmp, divide_ratio;
>> -u32 tuner_frequency, target_mclk, ts_clk;
>> +u32 tuner_frequency, target_mclk;
>>  s32 s32tmp;
>> +fe_status_t status;
>>  dev_dbg(&priv->i2c->dev,
>>  "%s: delivery_system=%d modulation=%d frequency=%d 
>> symbol_rate=%d inversion=%d pilot=%d rolloff=%d\n",
>>  __func__, c->delivery_system,
>> @@ -316,9 +317,6 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>  target_mclk = 144000;
>>  break;
>>  case M88DS3103_TS_PARALLEL:
>> -case M88DS3103_TS_PARALLEL_12:
>> -case M88DS3103_TS_PARALLEL_16:
>> -case M88DS3103_TS_PARALLEL_19_2:
>>  case M88DS3103_TS_CI:
>>  if (c->symbol_rate < 1800)
>>  target_mclk = 96000;
>> @@ -352,33 +350,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>  switch (priv->cfg->ts_mode) {
>>  case M88DS3103_TS_SERIAL:
>>  u8tmp1 = 0x00;
>> -ts_clk = 0;
>> -u8tmp = 0x46;
>> +u8tmp = 0x06;
>>  break;
>>  case M88DS3103_TS_SERIAL_D7:
>>  u8tmp1 = 0x20;
>> -ts_clk = 0;
>> -u8tmp = 0x46;
>> +u8tmp = 0x06;
>>  break;
>>  case M88DS3103_TS_PARALLEL:
>> -ts_clk = 24000;
>> -u8tmp = 0x42;
>> -break;
>> -case M88DS3103_TS_PARALLEL_12:
>> -ts_clk = 12000;
>> -u8tmp = 0x42;
>> -break;
>> -case M88DS3103_TS_PARALLEL_16:
>> -ts_clk = 16000;
>> -u8tmp = 0x42;
>> -break;
>> -case M88DS3103_TS_PARALLEL_19_2:
>> -ts_clk = 19200;
>> -u8tmp = 0x42;
>> +u8tmp = 0x02;
>>  break;
>>  case M88DS3103_TS_CI:
>> -ts_clk = 6000;
>> -u8tmp = 0x43;
>> +u8tmp = 0x03;
>>  break;
>>  default:
>>  dev_dbg(&priv->i2c->dev, "%s: invalid ts_mode\n", __func__);
>> @@ -386,6 +368,9 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>  goto err;
>>  }
>>   
>> +if (priv->cfg->ts_clk_pol)
>> +u8tmp |= 0x40;
>> +
>>  /* TS mode */
>>  ret = m88ds3103_wr_reg(priv, 0xfd, u8tmp);
>>  if (ret)
>> @@ -399,8 +384,8 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>  goto err;
>>  }
>>   
>> -if (ts_clk) {
>> -divide_ratio = DIV_ROUND_UP(target_mclk, ts_clk);
>> +if (priv->cfg->ts_clk) {
>> +divide_ratio = DIV_ROUND_UP(target_mclk, priv->cfg->ts_clk);
>>  u8tmp1 = divide_ratio / 2;
>>  u8tmp2 = DIV_ROUND_UP(divide_ratio, 2);
>>  } else {
>> @@ -411,7 +396,7 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>   
>>  dev_dbg(&priv->i2c->dev,
>>  "%s: target_mclk=%d ts_clk=%d divide_ratio=%d\n",
>> -__func__, target_mclk, ts_clk, divide_ratio);
>> +__func__, target_mclk, priv->cfg->ts_clk, divide_ratio);
>>   
>>  u8tmp1--;
>>  u8tmp2--;
>> @@ -523,6 +508,17 @@ static int m88ds3103_set_frontend(struct dvb_frontend 
>> *fe)
>>   
>>  priv->delivery_system = c->delivery_system;
>>   
>> +if (priv->cfg->start_ctrl) {
>> +for (len = 0; len < 30 ; len++) {
>> +m88ds3103_read_status(fe, &status);
>> +if (status & FE_HAS_LOCK) {
>> +priv->cfg->start_ctrl(fe);
>> +break;
>> +}
>> +msleep(20);
>> +}
>> +}
>> +
>
>What