On Mon, Dec 12, 2011 at 6:13 PM, Andreas Oberritter <obi@xxxxxxxxxxx> wrote:
> On 12.12.2011 05:28, Manu Abraham wrote:
>> On Sat, Dec 10, 2011 at 5:17 PM, Antti Palosaari <crope@xxxxxx> wrote:
>>> On 12/10/2011 06:44 AM, Manu Abraham wrote:
>>>>
>>>> static int cxd2820r_set_frontend(struct dvb_frontend *fe,
>>>
>>> [...]
>>>>
>>>> + switch (c->delivery_system) {
>>>> + case SYS_DVBT:
>>>> + ret = cxd2820r_init_t(fe);
>>>
>>>
>>>> + ret = cxd2820r_set_frontend_t(fe, p);
>>>
>>>
>>>
>>> Anyhow, I don't now like idea you have put .init() calls to .set_frontend().
>>> Could you move .init() happen in .init() callback as it was earlier?
>>
>> This was there in the earlier patch as well. Maybe you have a
>> new issue now ? ;-)
>>
>> ok.
>>
>> The argument what you make doesn't hold well, Why ?
>>
>> int cxd2820r_init_t(struct dvb_frontend *fe)
>> {
>> ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>> }
>>
>>
>> int cxd2820r_init_c(struct dvb_frontend *fe)
>> {
>> ret = cxd2820r_wr_reg(priv, 0x00085, 0x07);
>> }
>>
>>
>> Now, you might like to point that, the Base I2C address location
>> is different comparing DVB-T/DVBT2 to DVB-C
>>
>> So, If you have the init as in earlier with a common init, then you
>> will likely init the wrong device at .init(), as init is called open().
>> So, this might result in an additional register write, which could
>> be avoided altogether. One register access is not definitely
>> something to brag about, but is definitely a small incremental
>> difference. Other than that this register write doesn't do anything
>> more than an ADC_START. So starting the ADC at init doesn't
>> make sense. But does so when you want to select the right ADC.
>> So definitely, this change is an improvement. Also, you can
>> compare the time taken for the device to tune now. It is quite
>> a lot faster compared to without this patch. So you or any other
>> user should be happy. :-)
>>
>>
>> I don't think that in any way, the init should be used at init as
>> you say, which sounds pretty much incorrect.
>
> Maybe the function names should be modified to avoid confusion with the
> init driver callback.
On another tangential thought, Is it really worth to wrap that single
register write with another function name ?
instead of the current usage; ie,
ret = cxd2820r_wr_reg(priv, 0x00085, 0x07); /* Start ADC */
within set_frontend()
in set_frontend(), another thing that's wrapped up similarly is
the set_frontend() within the search() callback, which causes
another set of confusions within the driver.
Regards,
Manu
--
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
