[PATCH][RFC] Add mt9p031 sensor support.

Hi Javier,

Thanks for the patch. Here's a review of the power handling code.

On Tuesday 24 May 2011 16:30:43 Javier Martin wrote:

This RFC includes a power management implementation that causes
the sensor to show images with horizontal artifacts (usually
monochrome lines that appear on the image randomly).

Signed-off-by: Javier Martin <redacted>

[snip]

[snip]

This header is not needed anymore.

+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+
+#define MT9P031_PIXCLK_FREQ			54000000
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION			0x00
+#define		MT9P031_CHIP_VERSION_VALUE	0x1801
+#define MT9P031_ROW_START			0x01
+#define		MT9P031_ROW_START_DEF		54
+#define MT9P031_COLUMN_START			0x02
+#define		MT9P031_COLUMN_START_DEF	16
+#define MT9P031_WINDOW_HEIGHT			0x03
+#define MT9P031_WINDOW_WIDTH			0x04
+#define MT9P031_H_BLANKING			0x05
+#define		MT9P031_H_BLANKING_VALUE	0
+#define MT9P031_V_BLANKING			0x06
+#define		MT9P031_V_BLANKING_VALUE	25
+#define MT9P031_OUTPUT_CONTROL			0x07
+#define		MT9P031_OUTPUT_CONTROL_CEN	2
+#define		MT9P031_OUTPUT_CONTROL_SYN	1
+#define MT9P031_SHUTTER_WIDTH_UPPER		0x08
+#define MT9P031_SHUTTER_WIDTH			0x09
+#define MT9P031_PIXEL_CLOCK_CONTROL		0x0a
+#define MT9P031_FRAME_RESTART			0x0b
+#define MT9P031_SHUTTER_DELAY			0x0c
+#define MT9P031_RST				0x0d
+#define		MT9P031_RST_ENABLE		1
+#define		MT9P031_RST_DISABLE		0
+#define MT9P031_READ_MODE_1			0x1e
+#define MT9P031_READ_MODE_2			0x20
+#define		MT9P031_READ_MODE_2_ROW_MIR	0x8000
+#define		MT9P031_READ_MODE_2_COL_MIR	0x4000
+#define MT9P031_ROW_ADDRESS_MODE		0x22
+#define MT9P031_COLUMN_ADDRESS_MODE		0x23
+#define MT9P031_GLOBAL_GAIN			0x35
+
+#define MT9P031_WINDOW_HEIGHT_MAX		1944
+#define MT9P031_WINDOW_WIDTH_MAX		2592
+#define MT9P031_WINDOW_HEIGHT_MIN		2
+#define MT9P031_WINDOW_WIDTH_MIN		18

Can you move those 4 constants right below MT9P031_WINDOW_HEIGHT and 
MT9P031_WINDOW_WIDTH ? The max values are not correct, according to the 
datasheet they should be 2005 and 2751. You can define *_DEF constants for the 
default width and height.

+struct mt9p031 {
+	struct v4l2_subdev subdev;
+	struct media_pad pad;
+	struct v4l2_rect rect;	/* Sensor window */
+	struct v4l2_mbus_framefmt format;
+	struct mt9p031_platform_data *pdata;
+	struct mutex power_lock; /* lock to protect power_count */
+	int power_count;
+	u16 xskip;
+	u16 yskip;
+	/* cache register values */
+	u16 output_control;
+	u16 h_blanking;
+	u16 v_blanking;
+	u16 column_address_mode;
+	u16 row_address_mode;
+	u16 column_start;
+	u16 row_start;
+	u16 window_width;
+	u16 window_height;
+	struct regulator *reg_1v8;
+	struct regulator *reg_2v8;
+};

[snip]

+static int restore_registers(struct i2c_client *client)
+{
+	int ret;
+	struct mt9p031 *mt9p031 = to_mt9p031(client);
+
+	/* Disable register update, reconfigure atomically */
+	ret = mt9p031_set_output_control(mt9p031, 0,
+					MT9P031_OUTPUT_CONTROL_SYN);
+	if (ret < 0)
+		return ret;
+
+	/* Blanking and start values - default... */
+	ret = reg_write(client, MT9P031_H_BLANKING, mt9p031->h_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_V_BLANKING, mt9p031->v_blanking);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
+				mt9p031->column_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
+				mt9p031->row_address_mode);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_COLUMN_START,
+				mt9p031->column_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_ROW_START,
+				mt9p031->row_start);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_WIDTH,
+				mt9p031->window_width);
+	if (ret < 0)
+		return ret;
+
+	ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
+				mt9p031->window_height);
+	if (ret < 0)
+		return ret;

All those registers will be written to in mt9p031_s_stream(), there's no need 
to restore them when powering the chip up. You can remove this function 
completely, as well as the register cache.

+	/* Re-enable register update, commit all changes */
+	ret = mt9p031_set_output_control(mt9p031,
+					MT9P031_OUTPUT_CONTROL_SYN, 0);
+	if (ret < 0)
+		return ret;
+	return 0;
+}

[snip]

+static int mt9p031_power_on(struct mt9p031 *mt9p031)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	/* Ensure RESET_BAR is low */
+	if (mt9p031->pdata->reset)
+		mt9p031->pdata->reset(&mt9p031->subdev, 1);
+	/* turn on digital supply first */
+	ret = regulator_enable(mt9p031->reg_1v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 1.8v regulator: %d\n", ret);
+		goto err_1v8;

You can return ret immediately.

+	}

According to the datasheet (see figure 31) you need a delay of minimum 1ms 
here. regulator_enable() probably doesn't wait for the power to stabilize 
before returning (correct me if I'm wrong), so a slightly longer delay would 
be safer.

+	/* now turn on analog supply */
+	ret = regulator_enable(mt9p031->reg_2v8);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to enable 2.8v regulator: %d\n", ret);
+		goto err_rst;
+	}

+	/* Now RESET_BAR must be high */
+	if (mt9p031->pdata->reset)

Similarly, you need to wait for the 2.8V power supply to stabilize before de-
asserting the reset signal. A short delay is probably required. You can put it 
inside the 'if', as the delay is not needed if the reset signal can't be 
controlled.

+		mt9p031->pdata->reset(&mt9p031->subdev, 0);
+
+	if (mt9p031->pdata->set_xclk)
+		mt9p031->pdata->set_xclk(&mt9p031->subdev, MT9P031_PIXCLK_FREQ);
+
+	/* soft reset */
+	ret = mt9p031_reset(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to reset the camera\n");
+		goto err_rst;
+	}
+
+	ret = restore_registers(client);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to restore registers\n");
+		goto err_rst;
+	}
+
+	return 0;
+err_rst:
+	regulator_disable(mt9p031->reg_1v8);

You should disable both regulators here, and only the 1.8V regulator if 
enabling the 2.8V regulator fails.

+err_1v8:
+	return ret;
+
+}

[snip]

+static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	int ret = 0;
+
+	mutex_lock(&mt9p031->power_lock);
+
+	/*
+	 * If the power count is modified from 0 to != 0 or from != 0 to 0,
+	 * update the power state.
+	 */
+	if (mt9p031->power_count == !on) {
+		if (on) {
+			ret = mt9p031_power_on(mt9p031);
+			if (ret) {
+				dev_err(mt9p031->subdev.v4l2_dev->dev,
+				"Failed to enable 2.8v regulator: %d\n", ret);

This message isn't correct anymore, as mt9p031_power_on() now handles two 
regulators. As mt9p031_power_on() already prints an error message in the case 
of failure, you can remove this message.

+				goto out;
+			}
+		} else {
+			mt9p031_power_off(mt9p031);
+		}
+	}
+
+	/* Update the power count. */
+	mt9p031->power_count += on ? 1 : -1;
+	WARN_ON(mt9p031->power_count < 0);
+
+out:
+	mutex_unlock(&mt9p031->power_lock);
+	return ret;
+}
+
+static int mt9p031_registered(struct v4l2_subdev *sd)
+{
+	struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+	struct i2c_client *client = v4l2_get_subdevdata(&mt9p031->subdev);
+	int ret;
+
+	ret = mt9p031_set_power(&mt9p031->subdev, 1);
+	if (ret) {
+		dev_err(&client->dev,
+			"Failed to power on device: %d\n", ret);
+		goto err_pwron;

You can return ret directly.

+	}
+
+	ret = mt9p031_video_probe(client);
+	if (ret)
+		goto err_evprobe;
+

Code from here

+	mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
+	ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
+	if (ret)
+		goto err_evprobe;
+
+	mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;

to here can be moved to the probe() function, it will simplify 
mt9p031_registered(). The function will become so small that you can directly 
inline mt9p031_video_probe() inside it.

+	mt9p031_set_power(&mt9p031->subdev, 0);
+
+	return 0;
+err_evprobe:
+	mt9p031_set_power(&mt9p031->subdev, 0);
+err_pwron:
+	return ret;
+}

-- 
Regards,

Laurent Pinchart