Re: [GIT PULL FOR 3.3] HDIC HD29L2 DMB-TH demodulator driver

[Antti Palosaari <crope@iki.fi>]

On 12/20/2011 05:26 PM, Mauro Carvalho Chehab wrote:
> On 20-12-2011 12:52, Antti Palosaari wrote:
>> On 12/20/2011 03:39 PM, Mauro Carvalho Chehab wrote:

>>>> +        break;
>>>> +    case 2:
>>>> +        str_constellation = "QAM16";
>>>> +        c->modulation = QAM_16;
>>>> +        break;
>>>> +    case 3:
>>>> +        str_constellation = "QAM32";
>>>> +        c->modulation = QAM_32;
>>>> +        break;
>>>> +    case 4:
>>>> +        str_constellation = "QAM64";
>>>> +        c->modulation = QAM_64;
>>>> +        break;
>>>
>>> Please, avoid magic numbers. Instead, use macros for each
>>> value.
>>
>> I disagree that. Those numbers are coming from demodulator
>> register value. Same way is used almost every driver that
>> supports reading current transmission params from the demod.
>
> There are drivers that don't code it well, but it is always preferred
> to use macros for register values. Good drivers have it.

I still disagree. Are we speaking same issue?

val = read_reg(rgister)
switch (val) {
case 2:
	c->modulation = QAM_16;
	break;
case 3:
	c->modulation = QAM_32;
	break;
}

Why I should define macros here?
Or do you mean I should define macros for the selecting correct bits 
from the register?

Anyhow, for me that piece of code looks very clear. And it is used 
similarly very many drivers.



>> After all as I see there is no big bugs. Those findings are mostly related
>> of missing DMB-TH API support (and was even commented clearly). And 1-2 CodingStyle issues.
>
> One issue is pure CodingStyle. The other no-API related aren't.
>
>> As there is still few other DMB-TH drivers having similar issues already in
>> the master I don't see why not to add that too. Anyhow, if you see that must
>> be put to staging until DMB-TH is defined to API it is OK for me.
>
> Please fix the non-API related issues. If you ack to provide us the API improvements
> for DMB for 3.4, and get rid of "auto_mode = true" for all cases, I'm
> ok on merging it after the fixes at drivers/media/dvb.

If I don't add DMB-TH support to API you will push that to the staging?

Adding those to API is not mission impossible. Interleaver is only new 
parameter and all the rest are just extending values. But my time is 
limited... and I really would like to finally got Anysee smart card 
reader integrated to USB serial first.

regards
Antti

-- 
http://palosaari.fi/