Re: [PATCH v2 2/2] media: i2c: Add driver for onsemi MT9M114 camera sensor

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jun 14, 2022 at 04:02:16PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>    one more comment, mostly for the record if anyone else will
> encounter the same problem I found
> 
> On Fri, May 13, 2022 at 12:27:16PM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> 
> [snip]
> 
> > > +#define MT9M114_CAM_SENSOR_CONTROL_Y_READ_OUT_AVERAGE		(2 << 8)
> 
> The version of the datasheet I have documents this bit as "RESERVED"

Indeed, so does mine. I'll drop it.

> [snip]
> 
> > > +
> > > +static int mt9m114_configure(struct mt9m114 *sensor)
> > > +{
> > > +	u32 value;
> > > +	int ret = 0;
> > > +
> > > +	/*
> > > +	 * Pixel array crop and binning. The CAM_SENSOR_CFG_CPIPE_LAST_ROW
> > > +	 * register isn't clearly documented, but is always set to the number
> > > +	 * of output rows minus 4 in all example sensor modes.
> > > +	 */
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CFG_X_ADDR_START,
> > > +		      sensor->pa.crop.left, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CFG_Y_ADDR_START,
> > > +		      sensor->pa.crop.top, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CFG_X_ADDR_END,
> > > +		      sensor->pa.crop.width + sensor->pa.crop.left - 1, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CFG_Y_ADDR_END,
> > > +		      sensor->pa.crop.height + sensor->pa.crop.top - 1, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CFG_CPIPE_LAST_ROW,
> > > +		      sensor->pa.format.height - 4 - 1, &ret);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = mt9m114_read(sensor, MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
> > > +			   &value);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	value &= ~(MT9M114_CAM_SENSOR_CONTROL_X_READ_OUT_MASK |
> > > +		   MT9M114_CAM_SENSOR_CONTROL_Y_READ_OUT_MASK);
> > > +
> > > +	if (sensor->pa.crop.width != sensor->pa.format.width)
> > > +		value |= MT9M114_CAM_SENSOR_CONTROL_X_READ_OUT_SUMMING;
> > > +	if (sensor->pa.crop.height != sensor->pa.format.height)
> > > +		value |= MT9M114_CAM_SENSOR_CONTROL_Y_READ_OUT_SUMMING;
> 
> While applying 2x2 subsampling, I found SUMMING to mangle the color
> output possibly because the gains should be adjusted accordingly to
> the fact the SUMMING:
> "will add the charge or voltage values of the neighboring pixels
> together"
> 
> I have found the combination that works better for me out of the box
> to be:
> 
> 	if (sensor->pa.crop.width != sensor->pa.format.width)
> 		value |= MT9M114_CAM_SENSOR_CONTROL_X_READ_OUT_AVERAGE;
> 	if (sensor->pa.crop.height != sensor->pa.format.height)
> 		value |= MT9M114_CAM_SENSOR_CONTROL_Y_READ_OUT_SKIPPING;
> 
> Have you tested 2x2 binning with CSI-2 ?

I don't think I have. I'll give it a try.

> > > +
> > > +	mt9m114_write(sensor, MT9M114_CAM_SENSOR_CONTROL_READ_MODE, value,
> > > +		      &ret);
> > > +
> > > +	/*
> > > +	 * Color pipeline (IFP) cropping and scaling. Subtract 4 from the left
> > > +	 * and top coordinates to compensate for the lines and columns removed
> > > +	 * by demosaicing that are taken into account in the crop rectangle but
> > > +	 * not in the hardware.
> > > +	 */
> > > +	mt9m114_write(sensor, MT9M114_CAM_CROP_WINDOW_XOFFSET,
> > > +		      sensor->ifp.crop.left - 4, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_CROP_WINDOW_YOFFSET,
> > > +		      sensor->ifp.crop.top - 4, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_CROP_WINDOW_WIDTH,
> > > +		      sensor->ifp.crop.width, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_CROP_WINDOW_HEIGHT,
> > > +		      sensor->ifp.crop.height, &ret);
> > > +
> > > +	mt9m114_write(sensor, MT9M114_CAM_OUTPUT_WIDTH,
> > > +		      sensor->ifp.compose.width, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_OUTPUT_HEIGHT,
> > > +		      sensor->ifp.compose.height, &ret);
> > > +
> > > +	/* AWB and AE windows, use the full frame. */
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AWB_CLIP_WINDOW_XSTART, 0, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AWB_CLIP_WINDOW_YSTART, 0, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AWB_CLIP_WINDOW_XEND,
> > > +		      sensor->ifp.compose.width - 1, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AWB_CLIP_WINDOW_YEND,
> > > +		      sensor->ifp.compose.height - 1, &ret);
> > > +
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AE_INITIAL_WINDOW_XSTART,
> > > +		      0, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AE_INITIAL_WINDOW_YSTART,
> > > +		      0, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AE_INITIAL_WINDOW_XEND,
> > > +		      sensor->ifp.compose.width / 5 - 1, &ret);
> > > +	mt9m114_write(sensor, MT9M114_CAM_STAT_AE_INITIAL_WINDOW_YEND,
> > > +		      sensor->ifp.compose.height / 5 - 1, &ret);
> > > +
> > > +	mt9m114_write(sensor, MT9M114_CAM_CROP_CROPMODE,
> > > +		      MT9M114_CAM_CROP_MODE_AWB_AUTO_CROP_EN |
> > > +		      MT9M114_CAM_CROP_MODE_AE_AUTO_CROP_EN, &ret);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	/* Set the media bus code. */
> > > +	ret = mt9m114_read(sensor, MT9M114_CAM_OUTPUT_FORMAT, &value);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	value &= ~(MT9M114_CAM_OUTPUT_FORMAT_RGB_FORMAT_MASK |
> > > +		   MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_MASK |
> > > +		   MT9M114_CAM_OUTPUT_FORMAT_FORMAT_MASK |
> > > +		   MT9M114_CAM_OUTPUT_FORMAT_SWAP_BYTES |
> > > +		   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE);
> > > +	value |= sensor->ifp.info->output_format;
> > > +
> > > +	mt9m114_write(sensor, MT9M114_CAM_OUTPUT_FORMAT, value, &ret);
> > > +	return ret;
> > > +}

[snip]

-- 
Regards,

Laurent Pinchart