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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
