Hi Jacopo,
Thank you for the patch.
On Fri, Feb 11, 2022 at 10:34:32AM +0100, Jacopo Mondi wrote:
> The OV5640 pixel array is composed as:
> - vertically: 16 dummy columns, 2592 valid ones and 16 dummy columns for
> a total of 2624 columns
> - horizontally: 8 optical black lines, 6 dummy ones, 1944 valid and 6
> dummies for a total of 1964 lines
>
> Adjust the analog crop rectangle in all modes to:
> - Skip the first 16 dummy columns
> - Skip the first 14 black/dummy lines
> - Pass the whole valid pixel array size to the ISP for all modes except
> 1024x768, 720p and 1080p which are obtained by cropping the valid pixel array.
>
> Adjust how timings is programmed to comply with the new definitions.
>
> Tested in RGB565, UYVY, RGB565 and RGB888 modes.
>
> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> ---
> drivers/media/i2c/ov5640.c | 120 +++++++++++++++++++++----------------
> 1 file changed, 68 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index f817f865ad16..232afd4d906a 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -626,14 +626,14 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
These macros are added in 17/23.
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + .left = 2,
> + .top = 4,
> .width = 640,
> .height = 480,
> },
> @@ -644,22 +644,23 @@ static const struct ov5640_mode_info ov5640_mode_init_data = {
> .max_fps = OV5640_30_FPS
> };
>
> -static const struct ov5640_mode_info
> -ov5640_mode_data[OV5640_NUM_MODES] = {
> +static const struct ov5640_mode_info ov5640_mode_data[OV5640_NUM_MODES] = {
> {
> /* 160x120 */
> .id = OV5640_MODE_QQVGA_160_120,
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 160,
> .height = 120,
> },
> @@ -674,14 +675,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 176,
> .height = 144,
> },
> @@ -696,14 +699,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 320,
> .height = 240,
> },
> @@ -718,14 +723,16 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_48M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> - .left = 16,
> - .top = 6,
> + /* Maintain a minimum digital crop processing margins. */
> + .left = 2,
> + .top = 4,
> .width = 640,
> .height = 480,
> },
> @@ -740,12 +747,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> + /* Maintain a minimum digital crop processing margins. */
> .left = 56,
> .top = 60,
> .width = 720,
> @@ -762,12 +771,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SUBSAMPLING,
> .pixel_rate = OV5640_PIXEL_RATE_96M,
> .analog_crop = {
> - .left = 0,
> - .top = 4,
> - .width = 2623,
> - .height = 1947,
> + /* Feed the full valid pixel array to the ISP. */
> + .left = OV5640_PIXEL_ARRAY_LEFT,
> + .top = OV5640_PIXEL_ARRAY_TOP,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> + /* Maintain a minimum digital crop processing margins. */
> .left = 56,
> .top = 6,
> .width = 720,
> @@ -786,8 +797,8 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .analog_crop = {
> .left = 0,
> .top = 4,
> - .width = 2623,
> - .height = 1947,
> + .width = OV5640_NATIVE_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .crop = {
> .left = 16,
> @@ -808,8 +819,8 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .analog_crop = {
> .left = 0,
> .top = 250,
> - .width = 2623,
> - .height = 1705,
> + .width = 2624,
> + .height = 1456,
> },
> .crop = {
> .left = 16,
> @@ -828,12 +839,14 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SCALING,
> .pixel_rate = OV5640_PIXEL_RATE_148M,
> .analog_crop = {
> + /* Crop the full valid pixel array in the center. */
> .left = 336,
> .top = 434,
> - .width = 2287,
> - .height = 1521,
> + .width = 1952,
> + .height = 1088,
> },
> .crop = {
> + /* Maintain a larger digital crop processing margins. */
> .left = 16,
> .top = 4,
> .width = 1920,
> @@ -850,16 +863,17 @@ ov5640_mode_data[OV5640_NUM_MODES] = {
> .dn_mode = SCALING,
> .pixel_rate = OV5640_PIXEL_RATE_168M,
> .analog_crop = {
> + /* Give more processing margin to full resolution. */
> .left = 0,
> .top = 0,
> - .width = 2623,
> - .height = 1951,
> + .width = OV5640_NATIVE_WIDTH,
> + .height = 1952,
> },
> .crop = {
> .left = 16,
> .top = 4,
> - .width = 2592,
> - .height = 1944,
> + .width = OV5640_PIXEL_ARRAY_WIDTH,
> + .height = OV5640_PIXEL_ARRAY_HEIGHT,
> },
> .htot = 2844,
> .vblank_def = 24,
> @@ -1384,11 +1398,13 @@ static int ov5640_set_timings(struct ov5640_dev *sensor,
> if (ret < 0)
> return ret;
>
> - ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HW, analog_crop->width);
> + ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_HW,
> + analog_crop->width + analog_crop->left - 1);
> if (ret < 0)
> return ret;
>
> - ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VH, analog_crop->height);
> + ret = ov5640_write_reg16(sensor, OV5640_REG_TIMING_VH,
> + analog_crop->height + analog_crop->top - 1);
I'd move this to 08/23 as mentioned in the review of that patch.
> if (ret < 0)
> return ret;
>
--
Regards,
Laurent Pinchart
