[1/5] mt9m111: set inital return values to zero

Commit Message

Michael Grzeschik July 12, 2011, 3:39 p.m. UTC

Signed-off-by: Michael Grzeschik <[e-mail]>
---
 drivers/media/video/mt9m111.c |   32 ++++++++++++++++----------------
 1 files changed, 16 insertions(+), 16 deletions(-)

Comments

Laurent Pinchart July 14, 2011, 3:25 p.m. UTC | #1

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).

Guennadi Liakhovetski July 17, 2011, 4:52 p.m. UTC | #2

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

Patch

diff --git a/drivers/media/video/mt9m111.c b/drivers/media/video/mt9m111.c
index ebebed9..f10dcf0 100644
--- a/drivers/media/video/mt9m111.c
+++ b/drivers/media/video/mt9m111.c
@@ -190,7 +190,7 @@  static struct mt9m111 *to_mt9m111(const struct i2c_client *client)
 
 static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 {
-	int ret;
+	int ret = 0;
 	u16 page;
 	static int lastpage = -1;	/* PageMap cache value */
 
@@ -208,7 +208,7 @@  static int reg_page_map_set(struct i2c_client *client, const u16 reg)
 
 static int mt9m111_reg_read(struct i2c_client *client, const u16 reg)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_page_map_set(client, reg);
 	if (!ret)
@@ -221,7 +221,7 @@  static int mt9m111_reg_read(struct i2c_client *client, const u16 reg)
 static int mt9m111_reg_write(struct i2c_client *client, const u16 reg,
 			     const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_page_map_set(client, reg);
 	if (!ret)
@@ -234,7 +234,7 @@  static int mt9m111_reg_write(struct i2c_client *client, const u16 reg,
 static int mt9m111_reg_set(struct i2c_client *client, const u16 reg,
 			   const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = mt9m111_reg_read(client, reg);
 	if (ret >= 0)
@@ -245,7 +245,7 @@  static int mt9m111_reg_set(struct i2c_client *client, const u16 reg,
 static int mt9m111_reg_clear(struct i2c_client *client, const u16 reg,
 			     const u16 data)
 {
-	int ret;
+	int ret = 0;
 
 	ret = mt9m111_reg_read(client, reg);
 	return mt9m111_reg_write(client, reg, ret & ~data);
@@ -387,7 +387,7 @@  static int mt9m111_setfmt_yuv(struct i2c_client *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)
@@ -397,7 +397,7 @@  static int mt9m111_enable(struct i2c_client *client)
 
 static int mt9m111_reset(struct i2c_client *client)
 {
-	int ret;
+	int ret = 0;
 
 	ret = reg_set(RESET, MT9M111_RESET_RESET_MODE);
 	if (!ret)
@@ -452,7 +452,7 @@  static int mt9m111_s_crop(struct v4l2_subdev *sd, struct v4l2_crop *a)
 	struct v4l2_rect rect = a->c;
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	dev_dbg(&client->dev, "%s left=%d, top=%d, width=%d, height=%d\n",
 		__func__, rect.left, rect.top, rect.width, rect.height);
@@ -568,7 +568,7 @@  static int mt9m111_s_fmt(struct v4l2_subdev *sd,
 		.width	= mf->width,
 		.height	= mf->height,
 	};
-	int ret;
+	int ret = 0;
 
 	fmt = mt9m111_find_datafmt(mf->code, mt9m111_colour_fmts,
 				   ARRAY_SIZE(mt9m111_colour_fmts));
@@ -741,7 +741,7 @@  static struct soc_camera_ops mt9m111_ops = {
 static int mt9m111_set_flip(struct i2c_client *client, int flip, int mask)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (mt9m111->context == HIGHPOWER) {
 		if (flip)
@@ -791,7 +791,7 @@  static int mt9m111_set_global_gain(struct i2c_client *client, int gain)
 static int mt9m111_set_autoexposure(struct i2c_client *client, int on)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (on)
 		ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOEXPO_EN);
@@ -807,7 +807,7 @@  static int mt9m111_set_autoexposure(struct i2c_client *client, int on)
 static int mt9m111_set_autowhitebalance(struct i2c_client *client, int on)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	if (on)
 		ret = reg_set(OPER_MODE_CTRL, MT9M111_OPMODE_AUTOWHITEBAL_EN);
@@ -868,7 +868,7 @@  static int mt9m111_s_ctrl(struct v4l2_subdev *sd, struct v4l2_control *ctrl)
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	const struct v4l2_queryctrl *qctrl;
-	int ret;
+	int ret = 0;
 
 	qctrl = soc_camera_find_qctrl(&mt9m111_ops, ctrl->id);
 	if (!qctrl)
@@ -945,7 +945,7 @@  static int mt9m111_resume(struct soc_camera_device *icd)
 static int mt9m111_init(struct i2c_client *client)
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
-	int ret;
+	int ret = 0;
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(client);
@@ -969,7 +969,7 @@  static int mt9m111_video_probe(struct soc_camera_device *icd,
 {
 	struct mt9m111 *mt9m111 = to_mt9m111(client);
 	s32 data;
-	int ret;
+	int ret = 0;
 
 	/*
 	 * We must have a parent by now. And it cannot be a wrong one.
@@ -1053,7 +1053,7 @@  static int mt9m111_probe(struct i2c_client *client,
 	struct soc_camera_device *icd = client->dev.platform_data;
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct soc_camera_link *icl;
-	int ret;
+	int ret = 0;
 
 	if (!icd) {
 		dev_err(&client->dev, "mt9m111: soc-camera data missing!\n");