diff mbox series

[09/19] media: i2c: imx290: Simplify error handling when writing registers

Message ID 20220721083540.1525-10-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: imx290: Miscellaneous improvements | expand

Commit Message

Laurent Pinchart July 21, 2022, 8:35 a.m. UTC 
Error handling for register writes requires checking the error status of
every single write. This makes the code complex, or incorrect when the
checks are omitted. Simplify this by passing a pointer to an error code
to the imx290_write_reg() function, which allows writing multiple
registers in a row and only checking for errors at the end.

While at it, rename imx290_write_reg() to imx290_write() as there's
nothing else than registers to write, and rename imx290_read_reg()
accordingly.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx290.c | 86 ++++++++++++++------------------------
 1 file changed, 32 insertions(+), 54 deletions(-)

Comments

Alexander Stein July 21, 2022, 9:50 a.m. UTC | #1 
Am Donnerstag, 21. Juli 2022, 10:35:30 CEST schrieb Laurent Pinchart:
> Error handling for register writes requires checking the error status of
> every single write. This makes the code complex, or incorrect when the
> checks are omitted. Simplify this by passing a pointer to an error code
> to the imx290_write_reg() function, which allows writing multiple
> registers in a row and only checking for errors at the end.
> 
> While at it, rename imx290_write_reg() to imx290_write() as there's
> nothing else than registers to write, and rename imx290_read_reg()
> accordingly.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx290.c | 86 ++++++++++++++------------------------
>  1 file changed, 32 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> index 3f67c4d2417f..5b7f9027b50f 100644
> --- a/drivers/media/i2c/imx290.c
> +++ b/drivers/media/i2c/imx290.c
> @@ -367,7 +367,7 @@ static inline struct imx290 *to_imx290(struct
> v4l2_subdev *_sd) return container_of(_sd, struct imx290, sd);
>  }
> 
> -static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr,
> u32 *value) +static int __always_unused imx290_read(struct imx290 *imx290,
> u32 addr, u32 *value) {
>  	u8 data[3] = { 0, 0, 0 };
>  	int ret;
> @@ -385,17 +385,23 @@ static int __always_unused imx290_read_reg(struct
> imx290 *imx290, u32 addr, u32 return 0;
>  }
> 
> -static int imx290_write_reg(struct imx290 *imx290, u32 addr, u32 value)
> +static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int
> *err) {
>  	u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value >> 16 };
>  	int ret;
> 
> +	if (err && *err)
> +		return *err;
> +
>  	ret = regmap_raw_write(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
>  			       data, (addr >> IMX290_REG_SIZE_SHIFT) 
& 3);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: 
%d\n",
>  			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
>  			 addr & IMX290_REG_ADDR_MASK, ret);
> +		if (err)
> +			*err = ret;
> +	}
> 
>  	return ret;
>  }
> @@ -408,7 +414,7 @@ static int imx290_set_register_array(struct imx290
> *imx290, int ret;
> 
>  	for (i = 0; i < num_settings; ++i, ++settings) {
> -		ret = imx290_write_reg(imx290, settings->reg, settings-
>val);
> +		ret = imx290_write(imx290, settings->reg, settings->val, 
NULL);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -419,29 +425,16 @@ static int imx290_set_register_array(struct imx290
> *imx290, return 0;
>  }
> 
> -static int imx290_set_gain(struct imx290 *imx290, u32 value)
> -{
> -	int ret;
> -
> -	ret = imx290_write_reg(imx290, IMX290_GAIN, value);
> -	if (ret)
> -		dev_err(imx290->dev, "Unable to write gain\n");
> -
> -	return ret;
> -}
> -
>  /* Stop streaming */
>  static int imx290_stop_streaming(struct imx290 *imx290)
>  {
> -	int ret;
> +	int ret = 0;
> 
> -	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x01);
> -	if (ret < 0)
> -		return ret;
> +	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
> 
>  	msleep(30);
> 
> -	return imx290_write_reg(imx290, IMX290_XMSTA, 0x01);
> +	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
>  }
> 
>  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -456,25 +449,25 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> 
>  	switch (ctrl->id) {
>  	case V4L2_CID_GAIN:
> -		ret = imx290_set_gain(imx290, ctrl->val);
> +		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, 
NULL);
>  		break;
>  	case V4L2_CID_TEST_PATTERN:
>  		if (ctrl->val) {
> -			imx290_write_reg(imx290, IMX290_BLKLEVEL, 0);
> +			imx290_write(imx290, IMX290_BLKLEVEL, 0, 
&ret);
>  			usleep_range(10000, 11000);
> -			imx290_write_reg(imx290, IMX290_PGCTRL,
> -					 (u8)
(IMX290_PGCTRL_REGEN |
> -					 IMX290_PGCTRL_THRU |
> -					 
IMX290_PGCTRL_MODE(ctrl->val)));
> +			imx290_write(imx290, IMX290_PGCTRL,
> +				     (u8)(IMX290_PGCTRL_REGEN |
> +				     IMX290_PGCTRL_THRU |
> +				     IMX290_PGCTRL_MODE(ctrl-
>val)), &ret);
>  		} else {
> -			imx290_write_reg(imx290, IMX290_PGCTRL, 
0x00);
> +			imx290_write(imx290, IMX290_PGCTRL, 0x00, 
&ret);
>  			usleep_range(10000, 11000);
>  			if (imx290->bpp == 10)
> -				imx290_write_reg(imx290, 
IMX290_BLKLEVEL,
> -						 0x3c);
> +				imx290_write(imx290, 
IMX290_BLKLEVEL, 0x3c,
> +					     &ret);
>  			else /* 12 bits per pixel */
> -				imx290_write_reg(imx290, 
IMX290_BLKLEVEL,
> -						 0xf0);
> +				imx290_write(imx290, 
IMX290_BLKLEVEL, 0xf0,
> +					     &ret);
>  		}
>  		break;
>  	default:
> @@ -695,7 +688,8 @@ static int imx290_start_streaming(struct imx290 *imx290)
> return ret;
>  	}
> 
> -	ret = imx290_write_reg(imx290, IMX290_HMAX, imx290->current_mode-
>hmax);
> +	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
> +			   NULL);
>  	if (ret)
>  		return ret;
> 
> @@ -706,14 +700,12 @@ static int imx290_start_streaming(struct imx290
> *imx290) return ret;
>  	}
> 
> -	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x00);
> -	if (ret < 0)
> -		return ret;
> +	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
> 
>  	msleep(30);

Well you introduce a hard 30ms delay when the i2c transfer above fails, but I 
guess that's negligible.

>  	/* Start streaming */
> -	return imx290_write_reg(imx290, IMX290_XMSTA, 0x00);
> +	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
>  }
> 
>  static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> @@ -772,27 +764,13 @@ static int imx290_set_data_lanes(struct imx290
> *imx290) * validated in probe itself
>  		 */
>  		dev_err(imx290->dev, "Lane configuration not 
supported\n");
> -		ret = -EINVAL;
> -		goto exit;
> +		return -EINVAL;
>  	}
> 
> -	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
> -	if (ret) {
> -		dev_err(imx290->dev, "Error setting Physical Lane number 
register\n");
> -		goto exit;
> -	}
> -
> -	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
> -	if (ret) {
> -		dev_err(imx290->dev, "Error setting CSI Lane mode 
register\n");
> -		goto exit;
> -	}
> -
> -	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
> -	if (ret)
> -		dev_err(imx290->dev, "Error setting FR/FDG SEL 
register\n");
> +	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
> +	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
> +	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
> 
> -exit:
>  	return ret;
>  }

Reviewed-by: Alexander Stein <alexander.stein@ew.tq-group.com>
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
index 3f67c4d2417f..5b7f9027b50f 100644
--- a/drivers/media/i2c/imx290.c
+++ b/drivers/media/i2c/imx290.c
@@ -367,7 +367,7 @@  static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
 	return container_of(_sd, struct imx290, sd);
 }
 
-static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr, u32 *value)
+static int __always_unused imx290_read(struct imx290 *imx290, u32 addr, u32 *value)
 {
 	u8 data[3] = { 0, 0, 0 };
 	int ret;
@@ -385,17 +385,23 @@  static int __always_unused imx290_read_reg(struct imx290 *imx290, u32 addr, u32
 	return 0;
 }
 
-static int imx290_write_reg(struct imx290 *imx290, u32 addr, u32 value)
+static int imx290_write(struct imx290 *imx290, u32 addr, u32 value, int *err)
 {
 	u8 data[3] = { value & 0xff, (value >> 8) & 0xff, value >> 16 };
 	int ret;
 
+	if (err && *err)
+		return *err;
+
 	ret = regmap_raw_write(imx290->regmap, addr & IMX290_REG_ADDR_MASK,
 			       data, (addr >> IMX290_REG_SIZE_SHIFT) & 3);
-	if (ret < 0)
+	if (ret < 0) {
 		dev_err(imx290->dev, "%u-bit write to 0x%04x failed: %d\n",
 			 ((addr >> IMX290_REG_SIZE_SHIFT) & 3) * 8,
 			 addr & IMX290_REG_ADDR_MASK, ret);
+		if (err)
+			*err = ret;
+	}
 
 	return ret;
 }
@@ -408,7 +414,7 @@  static int imx290_set_register_array(struct imx290 *imx290,
 	int ret;
 
 	for (i = 0; i < num_settings; ++i, ++settings) {
-		ret = imx290_write_reg(imx290, settings->reg, settings->val);
+		ret = imx290_write(imx290, settings->reg, settings->val, NULL);
 		if (ret < 0)
 			return ret;
 	}
@@ -419,29 +425,16 @@  static int imx290_set_register_array(struct imx290 *imx290,
 	return 0;
 }
 
-static int imx290_set_gain(struct imx290 *imx290, u32 value)
-{
-	int ret;
-
-	ret = imx290_write_reg(imx290, IMX290_GAIN, value);
-	if (ret)
-		dev_err(imx290->dev, "Unable to write gain\n");
-
-	return ret;
-}
-
 /* Stop streaming */
 static int imx290_stop_streaming(struct imx290 *imx290)
 {
-	int ret;
+	int ret = 0;
 
-	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x01);
-	if (ret < 0)
-		return ret;
+	imx290_write(imx290, IMX290_STANDBY, 0x01, &ret);
 
 	msleep(30);
 
-	return imx290_write_reg(imx290, IMX290_XMSTA, 0x01);
+	return imx290_write(imx290, IMX290_XMSTA, 0x01, &ret);
 }
 
 static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -456,25 +449,25 @@  static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_GAIN:
-		ret = imx290_set_gain(imx290, ctrl->val);
+		ret = imx290_write(imx290, IMX290_GAIN, ctrl->val, NULL);
 		break;
 	case V4L2_CID_TEST_PATTERN:
 		if (ctrl->val) {
-			imx290_write_reg(imx290, IMX290_BLKLEVEL, 0);
+			imx290_write(imx290, IMX290_BLKLEVEL, 0, &ret);
 			usleep_range(10000, 11000);
-			imx290_write_reg(imx290, IMX290_PGCTRL,
-					 (u8)(IMX290_PGCTRL_REGEN |
-					 IMX290_PGCTRL_THRU |
-					 IMX290_PGCTRL_MODE(ctrl->val)));
+			imx290_write(imx290, IMX290_PGCTRL,
+				     (u8)(IMX290_PGCTRL_REGEN |
+				     IMX290_PGCTRL_THRU |
+				     IMX290_PGCTRL_MODE(ctrl->val)), &ret);
 		} else {
-			imx290_write_reg(imx290, IMX290_PGCTRL, 0x00);
+			imx290_write(imx290, IMX290_PGCTRL, 0x00, &ret);
 			usleep_range(10000, 11000);
 			if (imx290->bpp == 10)
-				imx290_write_reg(imx290, IMX290_BLKLEVEL,
-						 0x3c);
+				imx290_write(imx290, IMX290_BLKLEVEL, 0x3c,
+					     &ret);
 			else /* 12 bits per pixel */
-				imx290_write_reg(imx290, IMX290_BLKLEVEL,
-						 0xf0);
+				imx290_write(imx290, IMX290_BLKLEVEL, 0xf0,
+					     &ret);
 		}
 		break;
 	default:
@@ -695,7 +688,8 @@  static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_HMAX, imx290->current_mode->hmax);
+	ret = imx290_write(imx290, IMX290_HMAX, imx290->current_mode->hmax,
+			   NULL);
 	if (ret)
 		return ret;
 
@@ -706,14 +700,12 @@  static int imx290_start_streaming(struct imx290 *imx290)
 		return ret;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_STANDBY, 0x00);
-	if (ret < 0)
-		return ret;
+	imx290_write(imx290, IMX290_STANDBY, 0x00, &ret);
 
 	msleep(30);
 
 	/* Start streaming */
-	return imx290_write_reg(imx290, IMX290_XMSTA, 0x00);
+	return imx290_write(imx290, IMX290_XMSTA, 0x00, &ret);
 }
 
 static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
@@ -772,27 +764,13 @@  static int imx290_set_data_lanes(struct imx290 *imx290)
 		 * validated in probe itself
 		 */
 		dev_err(imx290->dev, "Lane configuration not supported\n");
-		ret = -EINVAL;
-		goto exit;
+		return -EINVAL;
 	}
 
-	ret = imx290_write_reg(imx290, IMX290_PHY_LANE_NUM, laneval);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting Physical Lane number register\n");
-		goto exit;
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_CSI_LANE_MODE, laneval);
-	if (ret) {
-		dev_err(imx290->dev, "Error setting CSI Lane mode register\n");
-		goto exit;
-	}
-
-	ret = imx290_write_reg(imx290, IMX290_FR_FDG_SEL, frsel);
-	if (ret)
-		dev_err(imx290->dev, "Error setting FR/FDG SEL register\n");
+	imx290_write(imx290, IMX290_PHY_LANE_NUM, laneval, &ret);
+	imx290_write(imx290, IMX290_CSI_LANE_MODE, laneval, &ret);
+	imx290_write(imx290, IMX290_FR_FDG_SEL, frsel, &ret);
 
-exit:
 	return ret;
 }