| Message ID | 20230913135638.26277-18-laurent.pinchart@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: i2c: imx219: Miscellaneous cleanups and improvements | expand |
Hi Laurent On Wed, Sep 13, 2023 at 04:56:35PM +0300, Laurent Pinchart wrote: > The IMX219 has distinct binning registers for the horizontal and > vertical directions. Calculate their value and write them separately. > Do you expect different binning factors in horizontal and vertical directions ? > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/i2c/imx219.c | 39 ++++++++++++++++++++++++++------------ > 1 file changed, 27 insertions(+), 12 deletions(-) > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > index 6bfdceaf5044..bf1c2a1dad95 100644 > --- a/drivers/media/i2c/imx219.c > +++ b/drivers/media/i2c/imx219.c > @@ -90,10 +90,11 @@ > #define IMX219_REG_ORIENTATION CCI_REG8(0x0172) > > /* Binning Mode */ > -#define IMX219_REG_BINNING_MODE CCI_REG16(0x0174) > -#define IMX219_BINNING_NONE 0x0000 > -#define IMX219_BINNING_2X2 0x0101 > -#define IMX219_BINNING_2X2_ANALOG 0x0303 > +#define IMX219_REG_BINNING_MODE_H CCI_REG8(0x0174) > +#define IMX219_REG_BINNING_MODE_V CCI_REG8(0x0175) > +#define IMX219_BINNING_NONE 0x00 > +#define IMX219_BINNING_X2 0x01 > +#define IMX219_BINNING_X2_ANALOG 0x03 > > #define IMX219_REG_CSI_DATA_FORMAT_A CCI_REG16(0x018c) > > @@ -615,7 +616,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, > const struct v4l2_mbus_framefmt *format; > const struct v4l2_rect *crop; > unsigned int bpp; > - u64 bin_mode; > + u64 bin_h, bin_v; > int ret = 0; > > format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0); > @@ -647,14 +648,28 @@ static int imx219_set_framefmt(struct imx219 *imx219, > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > - if (format->width == crop->width && format->height == crop->height) > - bin_mode = IMX219_BINNING_NONE; > - else if (bpp == 8) > - bin_mode = IMX219_BINNING_2X2_ANALOG; > - else > - bin_mode = IMX219_BINNING_2X2; > + switch (crop->width / format->width) { > + case 1: > + default: With the currently supported modes nothing but 1 or 2 can be obtained. I would have kept default: out, or used it to WARN developers adding new modes they have to support other binning factors (4x is the only not supported one). A minor though. > + bin_h = IMX219_BINNING_NONE; > + break; > + case 2: > + bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > + break; > + } > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret); > + switch (crop->height / format->height) { > + case 1: > + default: > + bin_v = IMX219_BINNING_NONE; > + break; > + case 2: > + bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > + break; > + } > + > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); With the currently supported mode, this could have been a single switch(). Doesn't hurt though Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, > format->width, &ret); > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, On Wed, Sep 13, 2023 at 04:54:56PM +0200, Jacopo Mondi wrote: > On Wed, Sep 13, 2023 at 04:56:35PM +0300, Laurent Pinchart wrote: > > The IMX219 has distinct binning registers for the horizontal and > > vertical directions. Calculate their value and write them separately. > > Do you expect different binning factors in horizontal and vertical > directions ? It should work, should someone decide they need to do so. I thus see no reason to disallow it. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > drivers/media/i2c/imx219.c | 39 ++++++++++++++++++++++++++------------ > > 1 file changed, 27 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c > > index 6bfdceaf5044..bf1c2a1dad95 100644 > > --- a/drivers/media/i2c/imx219.c > > +++ b/drivers/media/i2c/imx219.c > > @@ -90,10 +90,11 @@ > > #define IMX219_REG_ORIENTATION CCI_REG8(0x0172) > > > > /* Binning Mode */ > > -#define IMX219_REG_BINNING_MODE CCI_REG16(0x0174) > > -#define IMX219_BINNING_NONE 0x0000 > > -#define IMX219_BINNING_2X2 0x0101 > > -#define IMX219_BINNING_2X2_ANALOG 0x0303 > > +#define IMX219_REG_BINNING_MODE_H CCI_REG8(0x0174) > > +#define IMX219_REG_BINNING_MODE_V CCI_REG8(0x0175) > > +#define IMX219_BINNING_NONE 0x00 > > +#define IMX219_BINNING_X2 0x01 > > +#define IMX219_BINNING_X2_ANALOG 0x03 > > > > #define IMX219_REG_CSI_DATA_FORMAT_A CCI_REG16(0x018c) > > > > @@ -615,7 +616,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > const struct v4l2_mbus_framefmt *format; > > const struct v4l2_rect *crop; > > unsigned int bpp; > > - u64 bin_mode; > > + u64 bin_h, bin_v; > > int ret = 0; > > > > format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0); > > @@ -647,14 +648,28 @@ static int imx219_set_framefmt(struct imx219 *imx219, > > cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, > > crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); > > > > - if (format->width == crop->width && format->height == crop->height) > > - bin_mode = IMX219_BINNING_NONE; > > - else if (bpp == 8) > > - bin_mode = IMX219_BINNING_2X2_ANALOG; > > - else > > - bin_mode = IMX219_BINNING_2X2; > > + switch (crop->width / format->width) { > > + case 1: > > + default: > > With the currently supported modes nothing but 1 or 2 can be obtained. > I would have kept default: out, or used it to WARN developers adding > new modes they have to support other binning factors (4x is the only > not supported one). A minor though. The crop rectangle gets removed from the mode data in a latter patch, so potential for errors will disappear :-) > > + bin_h = IMX219_BINNING_NONE; > > + break; > > + case 2: > > + bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > + break; > > + } > > > > - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret); > > + switch (crop->height / format->height) { > > + case 1: > > + default: > > + bin_v = IMX219_BINNING_NONE; > > + break; > > + case 2: > > + bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; > > + break; > > + } > > + > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); > > + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); > > With the currently supported mode, this could have been a single > switch(). Doesn't hurt though > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, > > format->width, &ret);
diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c index 6bfdceaf5044..bf1c2a1dad95 100644 --- a/drivers/media/i2c/imx219.c +++ b/drivers/media/i2c/imx219.c @@ -90,10 +90,11 @@ #define IMX219_REG_ORIENTATION CCI_REG8(0x0172) /* Binning Mode */ -#define IMX219_REG_BINNING_MODE CCI_REG16(0x0174) -#define IMX219_BINNING_NONE 0x0000 -#define IMX219_BINNING_2X2 0x0101 -#define IMX219_BINNING_2X2_ANALOG 0x0303 +#define IMX219_REG_BINNING_MODE_H CCI_REG8(0x0174) +#define IMX219_REG_BINNING_MODE_V CCI_REG8(0x0175) +#define IMX219_BINNING_NONE 0x00 +#define IMX219_BINNING_X2 0x01 +#define IMX219_BINNING_X2_ANALOG 0x03 #define IMX219_REG_CSI_DATA_FORMAT_A CCI_REG16(0x018c) @@ -615,7 +616,7 @@ static int imx219_set_framefmt(struct imx219 *imx219, const struct v4l2_mbus_framefmt *format; const struct v4l2_rect *crop; unsigned int bpp; - u64 bin_mode; + u64 bin_h, bin_v; int ret = 0; format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0); @@ -647,14 +648,28 @@ static int imx219_set_framefmt(struct imx219 *imx219, cci_write(imx219->regmap, IMX219_REG_Y_ADD_END_A, crop->top - IMX219_PIXEL_ARRAY_TOP + crop->height - 1, &ret); - if (format->width == crop->width && format->height == crop->height) - bin_mode = IMX219_BINNING_NONE; - else if (bpp == 8) - bin_mode = IMX219_BINNING_2X2_ANALOG; - else - bin_mode = IMX219_BINNING_2X2; + switch (crop->width / format->width) { + case 1: + default: + bin_h = IMX219_BINNING_NONE; + break; + case 2: + bin_h = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; + break; + } - cci_write(imx219->regmap, IMX219_REG_BINNING_MODE, bin_mode, &ret); + switch (crop->height / format->height) { + case 1: + default: + bin_v = IMX219_BINNING_NONE; + break; + case 2: + bin_v = bpp == 8 ? IMX219_BINNING_X2_ANALOG : IMX219_BINNING_X2; + break; + } + + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_H, bin_h, &ret); + cci_write(imx219->regmap, IMX219_REG_BINNING_MODE_V, bin_v, &ret); cci_write(imx219->regmap, IMX219_REG_X_OUTPUT_SIZE, format->width, &ret);
The IMX219 has distinct binning registers for the horizontal and vertical directions. Calculate their value and write them separately. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- drivers/media/i2c/imx219.c | 39 ++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-)