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@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?
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@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
