| title: | Re PULL request http linuxtv org hg pb |
|
Em Wed, 20 May 2009 11:27:18 +0200 (CEST)
Patrick Boettcher <patrick.boettcher@xxxxxxx escreveu:
Hi Mauro,
please pull from my repository for hte following changesets:
Committed, thanks. Yet, I saw a few minor issues on one of your patch. Please
fix and submit me later a cleanup.
- Rewrote frontend-attach mechanism to gain noise-less deactivation of submodules
This patch do also a good CodingStyle fixes. Thats good (although it is
generally better to have the codingsyle specific changes on a separate patch).
However, in order to be CodingStyle compatible, youve sacrificed code
readability into a few cases. For example:
+ struct i2c_adapter *i2c_tuner
+ = s5h1420_get_tuner_i2c_adapter(fc- fe);
checkpatch.pl may not complain, but this is badly indented. Also, the backslash
is needed only on macros.
One alternative that would be correct, from codingstyles perpective would be:
+ struct i2c_adapter *i2c_tuner =
+ 5h1420_get_tuner_i2c_adapter(fc- fe);
The above is how indent tool would have enforced 80 cols. To be honest, I dont
like this either. If I havent any alternative, I would just violate 80 cols.
In this specific case, it is cleaner to break it into two statements:
struct i2c_adapter *i2c_tuner;
i2c_tuner = s5h1420_get_tuner_i2c_adapter(fc- fe);
The optimizer will likely generate the same assembler for both, but the later
is cleaner for humans.
+/* 4 MHz = 4000 KHz */
This comment is pointless. You should just remove it.
+ ops- tuner_ops.set_params
+ = skystar23_samsung_tbdu18132_tuner_set_params;
and
+ if (fc- fe != NULL)
+ fc- fe- ops.tuner_ops.set_params
+ = alps_tdee4_stv0297_tuner_set_params;
+ else
+ fc- fc_i2c_adap[0].no_base_addr = 0;
In the above case, the better for now is to just violate the 80 cols warning.
Unfortunately, we have the bad habit of using long names on some symbols, even
when not needed.
For example, instead of using alps_tdee4_stv0297_tuner_set_params, this name
would be equally understood:
alps_tdee4_stv0297_s_params;
Also, fc- fe- ops.tuner_ops.set_params could be, instead:
fc- fe- ops.tuner.s_params
I have to say that ops.tuner_ops is redundant.
+ if (params- frequency = 48000000 && params- frequency <= 154000000)
+ bs = 0x09;
+ if (params- frequency = 161000000 && params- frequency <= 439000000)
+ bs = 0x0a;
+ if (params- frequency = 447000000 && params- frequency <= 863000000)
+ bs = 0x08;
Just remove the backslash. You dont need they.
Cheers,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at rel="nofollow" vger.kernel.org/majordomo-info.html vger.kernel.org/majordomo-info.html
|
