On Tue, May 17, 2011 at 01:33:52PM +0200, Laurent Pinchart wrote: > Hi Javier, > > Thanks for the patch. Sorry, but this laziness is getting beyond a joke... And the fact that apparantly no one is picking up on it other than me is also a joke. > > +static int mt9p031_power_on(struct mt9p031 *mt9p031) > > +{ > > + int ret; > > + > > + if (mt9p031->pdata->set_xclk) > > + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000); > > + /* turn on VDD_IO */ > > + ret = regulator_enable(mt9p031->reg_2v8); > > + if (ret) { > > + pr_err("Failed to enable 2.8v regulator: %d\n", ret); > > + return -1; And why all these 'return -1's? My guess is that this is plain laziness on the authors part. > > +static int mt9p031_set_params(struct i2c_client *client, > > + struct v4l2_rect *rect, u16 xskip, u16 yskip) > > set_params should apply the parameters, not change them. They should have > already been validated by the callers. > > > +{ ... > > +err: > > + return -1; And again... > > +} > > + > > +static int mt9p031_set_crop(struct v4l2_subdev *sd, > > + struct v4l2_subdev_fh *fh, > > + struct v4l2_subdev_crop *crop) > > +{ ... > > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > + ret = mt9p031_set_params(client, &rect, xskip, yskip); > > + if (ret < 0) > > + return ret; So this propagates the lazy 'return -1' all the way back to userspace. This is utter crap - really it is, and I'm getting sick and tired of telling people that they should not use 'return -1'. It's down right lazy and sloppy programming. I wish people would stop doing it. I wish people would review their own stuff for this _before_ posting it onto a mailing list, so I don't have to keep complaining about it. And I wish people reviewing drivers would also look for this as well and complain about it. 'return -1' is generally a big fat warning sign that the author is doing something wrong, and should _always_ be investigated and complained about. -- 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