diff mbox series

[v2,06/18] media: i2c: imx219: Merge format and binning setting functions

Message ID 20230821223001.28480-7-laurent.pinchart@ideasonboard.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: imx219: Miscellaneous cleanups and improvements | expand

Commit Message

Laurent Pinchart Aug. 21, 2023, 10:29 p.m. UTC 
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@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 43 +++++++++-----------------------------
 1 file changed, 10 insertions(+), 33 deletions(-)

Comments

Dave Stevenson Aug. 22, 2023, 4:29 p.m. UTC | #1 
On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> 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@ideasonboard.com>
> ---
>  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@raspberrypi.com>

> +
>         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
>
Laurent Pinchart Aug. 23, 2023, 9:25 a.m. UTC | #2 
Hi Dave,

On Tue, Aug 22, 2023 at 05:29:44PM +0100, Dave Stevenson wrote:
> On Mon, 21 Aug 2023 at 23:30, Laurent Pinchart 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@ideasonboard.com>
> > ---
> >  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?

I don't think it makes a major difference. I'll switch to u64 (and may
further rework this by splitting horizontal and vertical binning).

> Either way:
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> > +
> >         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)
diff mbox series

Patch

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);
+
 	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)