| Message ID | 20230821223001.28480-7-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | cff09e76bf7940e2a82d003d80ca89824005008d |
| Headers | show |
| Series | [v2,01/18] media: i2c: imx219: Convert to CCI register access helpers | expand |
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 >
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 --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)
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(-)