On Thu, Jul 19, 2012 at 3:41 PM, Ismael Luceno <ismael.luceno@xxxxxxxxx> wrote: > On Thu, 19 Jul 2012 10:25:09 -0300 > Ezequiel Garcia <elezegarcia@xxxxxxxxx> wrote: >> On Wed, Jul 18, 2012 at 7:26 PM, Ismael Luceno >> <ismael.luceno@xxxxxxxxx> wrote: >> > On Thu, Jun 21, 2012 at 4:52 PM, Ezequiel Garcia >> > <elezegarcia@xxxxxxxxx> wrote: >> >> This patch moves video_nr module parameter to core.c >> >> and then passes that parameter as an argument to functions >> >> that need it. >> >> This way we avoid the extern declaration and parameter >> >> dependencies are better exposed. >> > <...> >> > >> > NACK. >> > >> > The changes to video_nr are supposed to be preserved. >> >> Mmm, I'm sorry but I don't see any functionality change in this patch, >> just a cleanup. >> >> What do you mean by "changes to video_nr are supposed to be >> preserved"? > > It is modified by solo_enc_alloc, which is called multiple times by > solo_enc_v4l2_init. Mmm, I see what you mean. Sorry for not noticing that :-( However, I still think that extern int is really not needed and should be cleaned up. Using global variables is not a nice practice, and using this extern hides dependencies, i.e. hides who is using video_nr. (For instance, I failed to see such dependency) Perhaps, you could consider passing the int as a pointer, so that solo_enc_alloc() can modify it. However, since it's your driver, you have access to the hardware, etc. I won't push any further this issue. Plus, this little issue is not the one preventing from moving out of staging. Feel free to nack any other of the patches, though I think this one was the only one non-trivial. Thanks for reviewing, Ezequiel. -- 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
