On Thu, 14 Jul 2011, Laurent Pinchart wrote:
> Hi Michael,
>
> There's no need to set initial return values to zero if they're assigned to in
> all code paths.
>
> [snip]
>
> > *client) static int mt9m111_enable(struct i2c_client *client)
> > {
> > struct mt9m111 *mt9m111 = to_mt9m111(client);
> > - int ret;
> > + int ret = 0;
> >
> > ret = reg_set(RESET, MT9M111_RESET_CHIP_ENABLE);
> > if (!ret)
>
> This is a clear example, ret will never be used uninitialized. Initializing it
> to 0 would be a waste of resources (although in this case it will probably be
> optimized out by the compiler).
Seconded. When I wrote:
> > +static int mt9m111_reg_mask(struct i2c_client *client, const u16 reg,
> > + const u16 data, const u16 mask)
> > +{
> > + int ret;
> > +
> > + ret = mt9m111_reg_read(client, reg);
> > + return mt9m111_reg_write(client, reg, (ret & ~mask) | data);
>
> Ok, I feel ashamed, that I have accepted this driver in this form... It is
> full of such buggy error handling instances, and this adds just one
> more... So, I would very appreciate if you could clean them up - before
> this patch, and handle this error properly too, otherwise I might do this
> myself some time... And, just noticed - "static int lastpage" from
> reg_page_map_set() must be moved into struct mt9m111, if this driver shall
> be able to handle more than one sensor simultaneously, at least in
> principle...
I didn't mean to init all return codes to 0. I meant, before using a
result of a reg_read(), you have to check it for error. I.e.,
+ ret = mt9m111_reg_read(client, reg);
+ if (ret >= 0)
+ ret = mt9m111_reg_write(client, reg, (ret & ~mask) | data);
+ return ret;
In principle, after the updated version of your patch "mt9m111: rewrite
set_pixfmt" all errors, returned by reg_read(), reg_write() and reg_mask()
are checked, even if some of them I would do a bit differently. E.g., I
would propagate the error code instead of replacing it with -EIO, etc. But
in principle all error cases are handled, so, we can live with that for
now. I'm dropping this patch.
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
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
