Hi Martin,

Thanks for the patch.

On Saturday 17 September 2011 11:29:31 Martin Hostettler wrote:

The MT9M032 is a parallel 3MP sensor from Micron controlled through I2C.

According to the Aptina datasheet, it's an 1.6MP sensor :-)

You can remove this header.

+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+#include <media/mt9m032.h>
+
+#define MT9M032_CHIP_VERSION			0x00
+#define MT9M032_ROW_START			0x01
+#define MT9M032_COLUMN_START			0x02
+#define MT9M032_ROW_SIZE			0x03
+#define MT9M032_COLUMN_SIZE			0x04
+#define MT9M032_HBLANK				0x05
+#define MT9M032_VBLANK				0x06
+#define MT9M032_SHUTTER_WIDTH_HIGH		0x08
+#define MT9M032_SHUTTER_WIDTH_LOW		0x09
+#define MT9M032_PIX_CLK_CTRL			0x0a
+#define     MT9M032_PIX_CLK_CTRL_INV_PIXCLK	0x8000
+#define MT9M032_RESTART				0x0b
+#define MT9M032_RESET				0x0d
+#define MT9M032_PLL_CONFIG1			0x11
+#define     MT9M032_PLL_CONFIG1_OUTDIV_MASK	0x3f
+#define     MT9M032_PLL_CONFIG1_MUL_SHIFT	8
+#define MT9M032_READ_MODE1			0x1e
+#define MT9M032_READ_MODE2			0x20
+#define     MT9M032_READ_MODE2_VFLIP_SHIFT	15
+#define     MT9M032_READ_MODE2_HFLIP_SHIFT	14
+#define     MT9M032_READ_MODE2_ROW_BLC		0x40
+#define MT9M032_GAIN_GREEN1			0x2b
+#define MT9M032_GAIN_BLUE			0x2c
+#define MT9M032_GAIN_RED			0x2d
+#define MT9M032_GAIN_GREEN2			0x2e
+/* write only */
+#define MT9M032_GAIN_ALL			0x35
+#define     MT9M032_GAIN_DIGITAL_MASK		0x7f
+#define     MT9M032_GAIN_DIGITAL_SHIFT		8
+#define     MT9M032_GAIN_AMUL_SHIFT		6
+#define     MT9M032_GAIN_ANALOG_MASK		0x3f
+#define MT9M032_FORMATTER1			0x9e
+#define MT9M032_FORMATTER2			0x9f
+#define     MT9M032_FORMATTER2_DOUT_EN		0x1000
+#define     MT9M032_FORMATTER2_PIXCLK_EN	0x2000
+
+#define MT9M032_MAX_BLANKING_ROWS		0x7ff
+
+#define to_mt9m032(sd)	container_of(sd, struct mt9m032, subdev)
+#define to_dev(sensor)	&((struct i2c_client
*)v4l2_get_subdevdata(&sensor->subdev))->dev +
+struct mt9m032 {
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct mt9m032_platform_data *pdata;
+	struct v4l2_ctrl_handler ctrls;
+
+	bool streaming;
+
+	int pix_clock;
+
+	struct v4l2_mbus_framefmt format;	/* height and width always the same as
in crop */ +	struct v4l2_rect crop;
+	struct v4l2_fract frame_interval;
+
+	struct v4l2_ctrl *hflip, *vflip;
+};
+
+
+static int mt9m032_read_reg(struct mt9m032 *sensor, const u8 reg)

No need for the const keyword, this isn't a pointer :-)

+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+	s32 data = i2c_smbus_read_word_data(client, reg);
+
+	return data < 0 ? data : swab16(data);

You should use be16_to_cpu() here.

+}
+
+static int mt9m032_write_reg(struct mt9m032 *sensor, const u8 reg,
+		     const u16 data)

No need for const either.

+{
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+	return i2c_smbus_write_word_data(client, reg, swab16(data));

And cpu_to_be16() here.

+}
+
+

I'm not a big fan of several blank lines (you even use 3 of them in 
mt9m032_probe) to separate code blocks. If you want to divide the code (which 
I find good for readability), using comments usually works better. I 
personally use

/* ---------------------------------------------------------------------------
 * Foo Bar
 */

to split drivers in sections.

This is up to you.

+static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
+{
+	int effective_width;
+	u64 ns;
+
+	effective_width = width + 716; /* emperical value */
+	ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
+	dev_dbg(to_dev(sensor),	"MT9M032 line time: %llu ns\n", ns);
+	return ns;
+}
+
+static int mt9m032_update_timing(struct mt9m032 *sensor,
+				 struct v4l2_fract *interval,
+				 const struct v4l2_rect *crop)
+{
+	unsigned long row_time;
+	int additional_blanking_rows;
+	int min_blank;

Those can't be negative, what about using unsigned it ?

+
+	if (!interval)
+		interval = &sensor->frame_interval;

This will have the side effect of possibly modifying the frame interval stored 
in the mt9m032 structure. Is that on purpose ?

+	if (!crop)
+		crop = &sensor->crop;

If I'm not mistaken the function is always called with the crop parameter set 
to either NULL or &sensor->crop. I think the parameter could be removed.

+
+	row_time = mt9m032_row_time(sensor, crop->width);
+
+	additional_blanking_rows = div_u64(((u64)1000000000) *
interval->numerator, +	                                 
((u64)interval->denominator) * row_time) +	                           -
crop->height;
+
+	if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
+		/* hardware limits to 11 bit values */
+		interval->denominator = 1000;
+		interval->numerator = div_u64((crop->height + MT9M032_MAX_BLANKING_ROWS)
+		                              * ((u64)row_time) * interval->denominator
+					      , 1000000000);

You're exceeding the 80 columns limit anyway, the comma can be put at the end 
of the previous row.

+		additional_blanking_rows = div_u64(((u64)1000000000) *
interval->numerator,
+	                              ((u64)interval->denominator) * row_time)
+	                           - crop->height;
+	}
+	/* enforce minimal 1.6ms blanking time. */
+	min_blank = 1600000 / row_time;
+	additional_blanking_rows = clamp(additional_blanking_rows,
+	                                 min_blank, MT9M032_MAX_BLANKING_ROWS);
+
+	return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
+}
+
+static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
+				 const struct v4l2_rect *crop)
+{
+	int ret;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height - 1);
+	/* offsets compensate for black border */
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop->left + 16);
+	if (!ret)
+		ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top + 52);

I don't think the black rows/columns offsets should be added implicitly by the 
driver. Wouldn't it be beter to make them explicit ?

+	if (!ret)
+		ret = mt9m032_update_timing(sensor, NULL, crop);
+	return ret;
+}
+
+static int update_formatter2(struct mt9m032 *sensor, bool streaming)
+{
+	u16 reg_val =   MT9M032_FORMATTER2_DOUT_EN
+		      | 0x0070;  /* parts reserved! */
+				 /* possibly for changing to 14-bit mode */

Does the sensor support 14-bit output ?

+
+	if (streaming)
+		reg_val |= MT9M032_FORMATTER2_PIXCLK_EN;   /* pixclock enable */
+
+	return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
+}
+
+static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	int ret;
+
+	ret = update_formatter2(sensor, streaming);
+	if (!ret)
+		sensor->streaming = streaming;
+	return ret;
+}
+
+static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index != 0 || code->pad != 0)
+		return -EINVAL;
+	code->code = V4L2_MBUS_FMT_Y8_1X8;
+	return 0;
+}
+
+static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
+				   struct v4l2_subdev_fh *fh,
+				   struct v4l2_subdev_frame_size_enum *fse)
+{
+	if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad !=
0) +		return -EINVAL;
+
+	fse->min_width = 32;
+	fse->max_width = 1440;

Shouldn't that be 1472 ?

+	fse->min_height = 32;
+	fse->max_height = 1096;

You don't support binning/skipping, so I think min width/height should be 
equal to max width/height.

+
+	return 0;
+}
+
+/**
+ * __mt9m032_get_pad_crop() - get crop rect
+ * @sensor:	pointer to the sensor struct
+ * @fh:	filehandle for getting the try crop rect from
+ * @which:	select try or active crop rect
+ * Returns a pointer the current active or fh relative try crop rect
+ */
+static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
+						struct v4l2_subdev_fh *fh,
+						u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(fh, 0);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &sensor->crop;
+	default:
+		return NULL;
+	}
+}
+
+/**
+ * __mt9m032_get_pad_format() - get format
+ * @sensor:	pointer to the sensor struct
+ * @fh:	filehandle for getting the try format from
+ * @which:	select try or active format
+ * Returns a pointer the current active or fh relative try format
+ */
+static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032
*sensor, +							   struct v4l2_subdev_fh *fh,
+							   u32 which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_format(fh, 0);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &sensor->format;
+	default:
+		return NULL;
+	}
+}
+
+static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_mbus_framefmt *format;
+
+	if (fmt->pad != 0)
+		return -EINVAL;

fmt->pad is already validated by the core. Same comment for the set operation.

+	format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
+	if (format == NULL)
+		return -EINVAL;

fmt->which is validated by the core as well, so __mt9m032_get_pad_format() 
can't return NULL. Same for cropping.

+
+	fmt->format = *format;
+
+	return 0;
+}
+
+static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
+				  struct v4l2_subdev_fh *fh,
+				  struct v4l2_subdev_format *fmt)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	if (fmt->pad != 0)
+		return -EINVAL;
+
+
+	/*
+	 * fmt->format.colorspace, fmt->format.code and fmt->format.field are
ignored
+	 * and thus forced to fixed values by the get call below.
+	 *
+	 * fmt->format.width, fmt->format.height are forced to the values set via
crop
+	 */
+
+	return mt9m032_get_pad_format(subdev, fh, fmt);
+}
+
+static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct
v4l2_subdev_fh *fh, +			    struct v4l2_subdev_crop *crop)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_rect *curcrop;
+
+	if (crop->pad != 0)
+		return -EINVAL;
+	curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+	if (!curcrop)
+		return -EINVAL;
+
+	crop->rect = *curcrop;
+
+	return 0;
+}
+
+
+static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct
v4l2_subdev_fh *fh, +		     struct v4l2_subdev_crop *crop)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	struct v4l2_mbus_framefmt tmp_format;
+	struct v4l2_rect tmp_crop_rect;
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop_rect;
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	if (crop->pad != 0)
+		return -EINVAL;
+
+	format = __mt9m032_get_pad_format(sensor, fh, crop->which);
+	crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+	if (!format || !crop_rect)
+		return -EINVAL;
+	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		tmp_crop_rect = *crop_rect;
+		tmp_format = *format;
+		format = &tmp_format;
+		crop_rect = &tmp_crop_rect;
+	}
+
+	crop_rect->top = crop->rect.top & ~0x1;
+	crop_rect->left = crop->rect.left;
+	crop_rect->height = crop->rect.height;
+	crop_rect->width = crop->rect.width & ~1;

You should validate the crop rectangle here.

+	format->height = crop_rect->height;
+	format->width = crop_rect->width;
+
+	if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+		int ret = mt9m032_update_geom_timing(sensor, crop_rect);

As the format can't be changed during streaming, what about moving this to the 
s_stream handler ? You could then also remove the call from the probe() 
function.

+
+		if (!ret) {
+			sensor->crop = tmp_crop_rect;
+			sensor->format = tmp_format;
+		}
+		return ret;
+	}
+
+	return mt9m032_get_crop(subdev, fh, crop);

I don't think you need that.

+}
+
+static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
+				      struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+
+	fi->pad = 0;
+	memset(fi->reserved, 0, sizeof(fi->reserved));
+	fi->interval = sensor->frame_interval;
+
+	return 0;
+}
+
+static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
+				      struct v4l2_subdev_frame_interval *fi)
+{
+	struct mt9m032 *sensor = to_mt9m032(subdev);
+	int ret;
+
+	if (sensor->streaming)
+		return -EBUSY;
+
+	memset(fi->reserved, 0, sizeof(fi->reserved));
+
+	ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
+	if (!ret)
+		sensor->frame_interval = fi->interval;
+	return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9m032_g_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct mt9m032 *sensor = to_mt9m032(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+	int val;
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+		return -EINVAL;
+	if (reg->match.addr != client->addr)
+		return -ENODEV;

Do you think those checks should be kept when using the MC API ?

+	val = mt9m032_read_reg(sensor, reg->reg);
+	if (val < 0)
+		return -EIO;
+
+	reg->size = 2;
+	reg->val = (u64) val;

Is there a need for an explicit cast ?

+
+	return 0;
+}
+
+static int mt9m032_s_register(struct v4l2_subdev *sd,
+			      struct v4l2_dbg_register *reg)
+{
+	struct mt9m032 *sensor = to_mt9m032(sd);
+	struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+	if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+		return -EINVAL;
+
+	if (reg->match.addr != client->addr)
+		return -ENODEV;
+
+	if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
+		return -EIO;
+
+	return 0;
+}
+#endif
+
+static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
hflip) +{
+	int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
+		      | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
+		      | MT9M032_READ_MODE2_ROW_BLC
+		      | 0x0007;
+
+	return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
+}
+
+static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
+{
+	return update_read_mode2(sensor, sensor->vflip->cur.val, val);
+}
+
+static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
+{
+	return update_read_mode2(sensor, val, sensor->hflip->cur.val);
+}
+
+static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
+{
+	int shutter_width;
+	u16 high_val, low_val;
+	int ret;
+
+	/* shutter width is in row times */
+	shutter_width = (val * 1000) / mt9m032_row_time(sensor,
sensor->crop.width); +
+	high_val = (shutter_width >> 16) & 0xf;
+	low_val = shutter_width & 0xffff;
+
+	ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
+	if (!ret)
+		mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
+
+	return ret;
+}
+
+static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
+{
+	int digital_gain_val;	/* in 1/8th (0..127) */
+	int analog_mul;		/* 0 or 1 */
+	int analog_gain_val;	/* in 1/16th. (0..63) */
+	u16 reg_val;
+
+	digital_gain_val = 51; /* from setup example */
+
+	if (val < 63) {
+		analog_mul = 0;
+		analog_gain_val = val;
+	} else {
+		analog_mul = 1;
+		analog_gain_val = val / 2;
+	}
+
+	/* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
+	/* overall_gain = a_gain * (1 + digital_gain_val / 8) */
+
+	reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
MT9M032_GAIN_DIGITAL_SHIFT +		  | (analog_mul & 1) <<
MT9M032_GAIN_AMUL_SHIFT
+		  | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
+
+	return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
+}
+
+static int mt9m032_setup_pll(struct mt9m032 *sensor)
+{
+	struct mt9m032_platform_data* pdata = sensor->pdata;
+	u16 reg_pll1;
+	unsigned int pre_div;
+	int res, ret;
+
+	/* TODO: also support other pre-div values */
+	if (pdata->pll_pre_div != 6) {
+		dev_warn(to_dev(sensor),
+			"Unsupported PLL pre-divisor value %u, using default 6\n",
+			pdata->pll_pre_div);
+	}
+	pre_div = 6;

The mt9p031 driver also needs to setup a PLL. I think it's time to compute the 
PLL parameters at runtime instead of relying on board code.

We tend to use lower-case hex constants in the kernel.

This isn't needed anymore.

 	V4L2_IDENT_MT9M111		= 45007,
 	V4L2_IDENT_MT9M112		= 45008,
 	V4L2_IDENT_MT9V022IX7ATC	= 45010, /* No way to detect "normal" I77ATx */

-- 
Regards,

Laurent Pinchart