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. That's good (although it is generally better to have the codingsyle specific changes on a separate patch). However, in order to be CodingStyle compatible, you've 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 codingstyle's 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 don't like this either. If I haven't 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 don't 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 http://vger.kernel.org/majordomo-info.html
