On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> The imx219_set_binning() function sets registers based on the bpp value,
> which is computed in imx219_set_framefmt(). As both functions are called
> from the same place consecutively, and set registers based on the
> selected mode, merge them together to simplify the code.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/i2c/imx219.c | 43 +++++++++-----------------------------
> 1 file changed, 10 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 9490dcba42ab..3a0b082d9ee0 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -618,6 +618,7 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> {
> const struct imx219_mode *mode = imx219->mode;
> unsigned int bpp;
> + u16 bin_mode;
> int ret = 0;
>
> switch (format->code) {
> @@ -648,6 +649,15 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> mode->crop.top - IMX219_PIXEL_ARRAY_TOP + mode->crop.height - 1,
> &ret);
>
> + if (!imx219->mode->binning)
> + bin_mode = IMX219_BINNING_NONE;
> + else if (bpp == 8)
> + bin_mode = IMX219_BINNING_2X2_ANALOG;
> + else
> + bin_mode = IMX219_BINNING_2X2;
> +
> + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret);
It feels a little weird using u16 for bin_mode because you happen to
know that the underlying register is 16 bit, and then are relying on
the compiler to extend it to u64 for cci_write. Is it better to use
the native u64 for cci_write, or just use unsigned int?
Either way:
Reviewed-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> +
> cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE,
> format->width, &ret);
> cci_write(imx219->regmap, IMX219_REG_Y_OUTPUT_SIZE,
> @@ -665,32 +675,6 @@ static int imx219_set_framefmt(struct imx219 *imx219,
> return ret;
> }
>
> -static int imx219_set_binning(struct imx219 *imx219,
> - const struct v4l2_mbus_framefmt *format)
> -{
> - if (!imx219->mode->binning)
> - return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
> - IMX219_BINNING_NONE, NULL);
> -
> - switch (format->code) {
> - case MEDIA_BUS_FMT_SRGGB8_1X8:
> - case MEDIA_BUS_FMT_SGRBG8_1X8:
> - case MEDIA_BUS_FMT_SGBRG8_1X8:
> - case MEDIA_BUS_FMT_SBGGR8_1X8:
> - return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
> - IMX219_BINNING_2X2_ANALOG, NULL);
> -
> - case MEDIA_BUS_FMT_SRGGB10_1X10:
> - case MEDIA_BUS_FMT_SGRBG10_1X10:
> - case MEDIA_BUS_FMT_SGBRG10_1X10:
> - case MEDIA_BUS_FMT_SBGGR10_1X10:
> - return cci_write(imx219->regmap, IMX219_REG_BINNING_MODE,
> - IMX219_BINNING_2X2, NULL);
> - }
> -
> - return -EINVAL;
> -}
> -
> static int imx219_get_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_selection *sel)
> @@ -764,13 +748,6 @@ static int imx219_start_streaming(struct imx219 *imx219,
> goto err_rpm_put;
> }
>
> - ret = imx219_set_binning(imx219, format);
> - if (ret) {
> - dev_err(&client->dev, "%s failed to set binning: %d\n",
> - __func__, ret);
> - goto err_rpm_put;
> - }
> -
> /* Apply customized values from user */
> ret = __v4l2_ctrl_handler_setup(imx219->sd.ctrl_handler);
> if (ret)
> --
> Regards,
>
> Laurent Pinchart
>
