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@xxxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
