On Thursday 27 August 2009 08:36:29 Guennadi Liakhovetski wrote: > On Thu, 27 Aug 2009, Hans Verkuil wrote: > > > On Thursday 27 August 2009 00:44:13 Guennadi Liakhovetski wrote: > > > On Thu, 27 Aug 2009, Hans Verkuil wrote: > > > > > > > On Wednesday 26 August 2009 23:55:29 Guennadi Liakhovetski wrote: > > > > > > > > > > Hm, these are lines like > > > > > > > > > > subdev->grp_id = (__u32)icd; > > > > > > > > > > I think, grp_id should become unsigned long, Hans? > > > > > > > > Where and how is grp_id used? This seems to me to be an abuse of this field. > > > > It is really not meant to contain a pointer. > > > > > > > > I can't find any actual use of this field in this driver, so either I'm > > > > missing something or this is simply not needed at all. > > > > > > We discussed this before, here's an excerpt from your earlier reply to my > > > mail: > > > > > > <quote> > > > > In fact, what I actually need is to call a specific method, if it is > > > > implemented, from one specific subdevice, and get its error code - not > > > > from all and not until the first error. I am currently abusing your > > > > grp_id for this, but it might eventually be better to add such a wrapper. > > > > > > That's actually what grp_id is intended for (or one of the intended uses > > > at least). > > > </quote> > > > > I'm pretty sure I didn't realize at the time that you would attempt to store a > > pointer in grp_id. > > Yep, sorry, I didn't mention that. > > > > But, I think, since then I switched to using v4l2_subdev_call() > > > _everywhere_, so, it might well be, that this grp_id can go completely. > > > Not sure off the top of my head though. > > > > If you only use v4l2_subdev_call then you do not need to set grp_id at all. > > So these lines can be removed. > > Yes, certainly looks like you're right. So, would it be enough to push a > patch removing these two lines on top of the series? Notice, there are > currently no 64-bit platforms, that use soc-camera, so, this "bug" just > adds two compiler warnings for a couple of commits in an unpractical > configuration:-) Yes, please. It's dead code after all and dead code is bad. And so are unnecessary warnings in a daily build, BTW. Actually, I consider dead code the worst coding sin there is: bugs are easy: it clearly doesn't work, so you know the code is wrong. Missing code is equally easy: the code to handle a particular feature is simply not there. But dead code is deadly since it leads to questions like: Is it really not used or is it used somehow in some subtle manner? Was it intended for some essential feature that is still to be implemented? If so, do we still need to implement that or is it no longer needed? I've had to deal with things like that in the past where the original author was no longer available and I hated it every time that happened. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- 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
