* (no subject)
@ 2011-05-17 9:28 ` Javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-media
Cc: g.liakhovetski, laurent.pinchart, carlighting, beagleboard,
linux-arm-kernel
This series of patches provides support for Aptina mt9p031 sensor on Beagleboard xM.
It has been tested using media-ctl and yavta.
^ permalink raw reply [flat|nested] 26+ messages in thread
* No subject
@ 2011-05-17 9:28 ` Javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-arm-kernel
This series of patches provides support for Aptina mt9p031 sensor on Beagleboard xM.
It has been tested using media-ctl and yavta.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 9:28 ` No subject Javier Martin
@ 2011-05-17 9:28 ` Javier Martin
-1 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-media
Cc: g.liakhovetski, laurent.pinchart, carlighting, beagleboard,
linux-arm-kernel, Javier Martin
It has been tested in beagleboard xM, using LI-5M03 module.
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
drivers/media/video/Kconfig | 8 +
drivers/media/video/Makefile | 1 +
drivers/media/video/mt9p031.c | 773 +++++++++++++++++++++++++++++++++++++++
include/media/mt9p031.h | 11 +
include/media/v4l2-chip-ident.h | 1 +
5 files changed, 794 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/mt9p031.c
create mode 100644 include/media/mt9p031.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 00f51dd..32df8bd 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -329,6 +329,14 @@ config VIDEO_OV7670
OV7670 VGA camera. It currently only works with the M88ALP01
controller.
+config VIDEO_MT9P031
+ tristate "Micron MT9P031 support"
+ depends on I2C && VIDEO_V4L2
+ ---help---
+ This driver supports MT9P031 cameras from Micron
+ This is a Video4Linux2 sensor-level driver for the Micron
+ mt0p031 5 Mpixel camera.
+
config VIDEO_MT9V011
tristate "Micron mt9v011 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index ace5d8b..912b29b 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
+obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o
diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
new file mode 100644
index 0000000..850cfec
--- /dev/null
+++ b/drivers/media/video/mt9p031.c
@@ -0,0 +1,773 @@
+/*
+ * Driver for MT9P031 CMOS Image Sensor from Aptina
+ *
+ * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
+ *
+ * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * Based on the MT9V032 driver and Bastian Hecht's code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <linux/videodev2.h>
+
+#include <media/mt9p031.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION 0x00
+#define MT9P031_CHIP_VERSION_VALUE 0x1801
+#define MT9P031_ROW_START 0x01
+#define MT9P031_ROW_START_SKIP 54
+#define MT9P031_COLUMN_START 0x02
+#define MT9P031_COLUMN_START_SKIP 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_RESET 0x0d
+#define MT9P031_RESET_ENABLE 1
+#define MT9P031_RESET_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_MAX_HEIGHT 1944
+#define MT9P031_MAX_WIDTH 2592
+#define MT9P031_MIN_HEIGHT 2
+#define MT9P031_MIN_WIDTH 18
+
+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;
+ int model; /* V4L2_IDENT_MT9P031* codes from v4l2-chip-ident.h */
+ u16 xskip;
+ u16 yskip;
+ struct regulator *reg_1v8, *reg_2v8;
+};
+
+static struct mt9p031 *to_mt9p031(const struct i2c_client *client)
+{
+ return container_of(i2c_get_clientdata(client), struct mt9p031, subdev);
+}
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+ s32 data = i2c_smbus_read_word_data(client, reg);
+ return data < 0 ? data : swab16(data);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ return i2c_smbus_write_word_data(client, reg, swab16(data));
+}
+
+static int reg_set(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ int ret;
+
+ ret = reg_read(client, reg);
+ if (ret < 0)
+ return ret;
+ return reg_write(client, reg, ret | data);
+}
+
+static int reg_clear(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ int ret;
+
+ ret = reg_read(client, reg);
+ if (ret < 0)
+ return ret;
+ return reg_write(client, reg, ret & ~data);
+}
+
+static int mt9p031_idle(struct i2c_client *client)
+{
+ int ret;
+
+ /* Disable chip output, synchronous option update */
+ ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_ENABLE);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_DISABLE);
+ if (ret < 0)
+ goto err;
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ if (ret < 0)
+ goto err;
+ return 0;
+err:
+ return -EIO;
+}
+
+static int mt9p031_power_on(struct mt9p031 *mt9p031)
+{
+ int ret;
+
+ if (mt9p031->pdata->set_xclk)
+ mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
+ /* turn on VDD_IO */
+ ret = regulator_enable(mt9p031->reg_2v8);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ return -1;
+ }
+ return 0;
+}
+
+static void mt9p031_power_off(struct mt9p031 *mt9p031)
+{
+ if (mt9p031->pdata->set_xclk)
+ mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+ regulator_disable(mt9p031->reg_2v8);
+}
+
+static int mt9p031_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ if (code->pad || code->index)
+ return -EINVAL;
+
+ code->code = mt9p031->format.code;
+
+ return 0;
+}
+
+static struct v4l2_mbus_framefmt *mt9p031_get_pad_format(
+ struct mt9p031 *mt9p031,
+ struct v4l2_subdev_fh *fh,
+ unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(fh, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &mt9p031->format;
+ default:
+ return NULL;
+ }
+}
+
+static struct v4l2_rect *mt9p031_get_pad_crop(struct mt9p031 *mt9p031,
+ struct v4l2_subdev_fh *fh, unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(fh, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &mt9p031->rect;
+ default:
+ return NULL;
+ }
+}
+
+static int mt9p031_get_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_rect *rect = mt9p031_get_pad_crop(mt9p031, fh, crop->pad,
+ crop->which);
+ if (!rect)
+ return -EINVAL;
+
+ crop->rect = *rect;
+
+ return 0;
+}
+
+static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
+{
+ unsigned int skip;
+
+ if (source - source / 4 < *target) {
+ *target = source;
+ return 1;
+ }
+
+ skip = DIV_ROUND_CLOSEST(source, *target);
+ if (skip > max_skip)
+ skip = max_skip;
+ *target = 2 * DIV_ROUND_UP(source, 2 * skip);
+
+ return skip;
+}
+
+static int mt9p031_set_params(struct i2c_client *client,
+ struct v4l2_rect *rect, u16 xskip, u16 yskip)
+{
+ struct mt9p031 *mt9p031 = to_mt9p031(client);
+ int ret;
+ u16 xbin, ybin;
+ const u16 hblank = MT9P031_H_BLANKING_VALUE,
+ vblank = MT9P031_V_BLANKING_VALUE;
+
+ /*
+ * TODO: Attention! When implementing horizontal flipping, adjust
+ * alignment according to R2 "Column Start" description in the datasheet
+ */
+ if (xskip & 1) {
+ xbin = 1;
+ rect->left &= ~3;
+ } else if (xskip & 2) {
+ xbin = 2;
+ rect->left &= ~7;
+ } else {
+ xbin = 4;
+ rect->left &= ~15;
+ }
+
+ ybin = min(yskip, (u16)4);
+
+ rect->top &= ~1;
+
+ /* Disable register update, reconfigure atomically */
+ ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_SYN);
+ if (ret < 0)
+ goto err;
+
+ dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
+ xskip, yskip, rect->width, rect->height, rect->left, rect->top);
+
+ /* Blanking and start values - default... */
+ ret = reg_write(client, MT9P031_H_BLANKING, hblank);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_V_BLANKING, vblank);
+ if (ret < 0)
+ goto err;
+
+ if (yskip != mt9p031->yskip || xskip != mt9p031->xskip) {
+ /* Binning, skipping */
+ ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
+ ((xbin - 1) << 4) | (xskip - 1));
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
+ ((ybin - 1) << 4) | (yskip - 1));
+ if (ret < 0)
+ goto err;
+ }
+ dev_dbg(&client->dev, "new physical left %u, top %u\n",
+ rect->left, rect->top);
+
+ ret = reg_write(client, MT9P031_COLUMN_START,
+ rect->left + MT9P031_COLUMN_START_SKIP);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_ROW_START,
+ rect->top + MT9P031_ROW_START_SKIP);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_WINDOW_WIDTH,
+ rect->width - 1);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
+ rect->height - 1);
+ if (ret < 0)
+ goto err;
+
+ /* Re-enable register update, commit all changes */
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_SYN);
+ if (ret < 0)
+ goto err;
+
+ mt9p031->xskip = xskip;
+ mt9p031->yskip = yskip;
+ return ret;
+err:
+ return -1;
+}
+
+static int mt9p031_set_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_mbus_framefmt *f;
+ struct v4l2_rect *c;
+ struct v4l2_rect rect;
+ u16 xskip, yskip;
+ s32 width, height;
+ int ret;
+
+ pr_info("%s(%ux%u@%u:%u : %u)\n", __func__,
+ crop->rect.width, crop->rect.height,
+ crop->rect.left, crop->rect.top, crop->which);
+
+ /*
+ * Clamp the crop rectangle boundaries and align them to a multiple of 2
+ * pixels.
+ */
+ rect.width = ALIGN(clamp(crop->rect.width,
+ MT9P031_MIN_WIDTH, MT9P031_MAX_WIDTH), 2);
+ rect.height = ALIGN(clamp(crop->rect.height,
+ MT9P031_MIN_HEIGHT, MT9P031_MAX_HEIGHT), 2);
+ rect.left = ALIGN(clamp(crop->rect.left,
+ 0, MT9P031_MAX_WIDTH - rect.width), 2);
+ rect.top = ALIGN(clamp(crop->rect.top,
+ 0, MT9P031_MAX_HEIGHT - rect.height), 2);
+
+ c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
+
+ if (rect.width != c->width || rect.height != c->height) {
+ /*
+ * Reset the output image size if the crop rectangle size has
+ * been modified.
+ */
+ f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
+ crop->which);
+ width = f->width;
+ height = f->height;
+
+ xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
+ yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
+ } else {
+ xskip = mt9p031->xskip;
+ yskip = mt9p031->yskip;
+ f = NULL;
+ }
+
+ if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ ret = mt9p031_set_params(client, &rect, xskip, yskip);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (f) {
+ f->width = width;
+ f->height = height;
+ }
+
+ *c = rect;
+ crop->rect = rect;
+
+ return 0;
+}
+
+static int mt9p031_get_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ fmt->format =
+ *mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+ return 0;
+}
+
+static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
+ s32 max_skip, s32 max)
+{
+ unsigned int skip;
+
+ if (*source - *source / 4 < target) {
+ *source = target;
+ return 1;
+ }
+
+ skip = min(max, *source + target / 2) / target;
+ if (skip > max_skip)
+ skip = max_skip;
+ *source = target * skip;
+
+ return skip;
+}
+
+static int mt9p031_fmt_validate(struct v4l2_subdev *sd,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_mbus_framefmt *format = &fmt->format;
+
+ if (format->code != mt9p031->format.code || fmt->pad)
+ return -EINVAL;
+
+ format->colorspace = V4L2_COLORSPACE_SRGB;
+ format->width = clamp_t(int, ALIGN(format->width, 2), 2,
+ MT9P031_MAX_WIDTH);
+ format->height = clamp_t(int, ALIGN(format->height, 2), 2,
+ MT9P031_MAX_HEIGHT);
+ format->field = V4L2_FIELD_NONE;
+
+ return 0;
+}
+
+static int mt9p031_set_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct v4l2_subdev_format sdf = *fmt;
+ struct v4l2_mbus_framefmt *f, *format = &sdf.format;
+ struct v4l2_rect *c, rect;
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ u16 xskip, yskip;
+ int ret;
+
+ ret = mt9p031_fmt_validate(sd, &sdf);
+ if (ret < 0)
+ return ret;
+
+ f = mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+
+ if (f->width != format->width || f->height != format->height) {
+ c = mt9p031_get_pad_crop(mt9p031, fh, fmt->pad, fmt->which);
+
+ rect.width = c->width;
+ rect.height = c->height;
+
+ xskip = mt9p031_skip_for_scale(&rect.width, format->width, 7,
+ MT9P031_MAX_WIDTH);
+ if (rect.width + c->left > MT9P031_MAX_WIDTH)
+ rect.left = (MT9P031_MAX_WIDTH - rect.width) / 2;
+ else
+ rect.left = c->left;
+ yskip = mt9p031_skip_for_scale(&rect.height, format->height, 8,
+ MT9P031_MAX_HEIGHT);
+ if (rect.height + c->top > MT9P031_MAX_HEIGHT)
+ rect.top = (MT9P031_MAX_HEIGHT - rect.height) / 2;
+ else
+ rect.top = c->top;
+ } else {
+ xskip = mt9p031->xskip;
+ yskip = mt9p031->yskip;
+ c = NULL;
+ }
+
+ pr_info("%s(%ux%u : %u)\n", __func__,
+ format->width, format->height, fmt->which);
+
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ /* mt9p031_set_params() doesn't change width and height */
+ ret = mt9p031_set_params(client, &rect, xskip, yskip);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (c)
+ *c = rect;
+
+ *f = *format;
+ fmt->format = *format;
+
+ return 0;
+}
+
+static int mt9p031_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ int ret;
+
+ if (enable) {
+ /* Switch to master "normal" mode */
+ ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ } else {
+ /* Stop sensor readout */
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ }
+ if (ret < 0)
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * Interface active, can use i2c. If it fails, it can indeed mean, that
+ * this wasn't our capture interface, so, we wait for the right one
+ */
+static int mt9p031_video_probe(struct i2c_client *client)
+{
+ struct mt9p031 *mt9p031 = to_mt9p031(client);
+ s32 data;
+ int ret;
+
+ /* Read out the chip version register */
+ data = reg_read(client, MT9P031_CHIP_VERSION);
+
+ switch (data) {
+ case MT9P031_CHIP_VERSION_VALUE:
+ mt9p031->model = V4L2_IDENT_MT9P031;
+ break;
+ default:
+ dev_err(&client->dev,
+ "No MT9P031 chip detected, register read %x\n", data);
+ return -ENODEV;
+ }
+
+ dev_info(&client->dev, "Detected a MT9P031 chip ID %x\n", data);
+
+ ret = mt9p031_idle(client);
+ if (ret < 0)
+ dev_err(&client->dev, "Failed to initialise the camera\n");
+
+ return ret;
+}
+
+static int mt9p031_registered(struct v4l2_subdev *sd)
+{
+ struct mt9p031 *mt9p031;
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ mt9p031_power_off(mt9p031);
+ return 0;
+}
+
+static int mt9p031_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct mt9p031 *mt9p031;
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ return mt9p031_power_on(mt9p031);
+}
+
+static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct mt9p031 *mt9p031;
+
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ mt9p031_power_off(mt9p031);
+ return 0;
+}
+
+static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
+{
+ struct mt9p031 *mt9p031;
+ int ret = 0;
+
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ if (on) {
+ ret = mt9p031_power_on(mt9p031);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ goto out;
+ }
+ } else {
+ mt9p031_power_off(mt9p031);
+ }
+out:
+ return ret;
+}
+
+static struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
+ .s_power = mt9p031_set_power,
+};
+
+static struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
+ .s_stream = mt9p031_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops mt9p031_subdev_pad_ops = {
+ .enum_mbus_code = mt9p031_enum_mbus_code,
+ .get_fmt = mt9p031_get_format,
+ .set_fmt = mt9p031_set_format,
+ .get_crop = mt9p031_get_crop,
+ .set_crop = mt9p031_set_crop,
+};
+
+static struct v4l2_subdev_ops mt9p031_subdev_ops = {
+ .core = &mt9p031_subdev_core_ops,
+ .video = &mt9p031_subdev_video_ops,
+ .pad = &mt9p031_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
+ .registered = mt9p031_registered,
+ .open = mt9p031_open,
+ .close = mt9p031_close,
+};
+
+static int mt9p031_probe(struct i2c_client *client,
+ const struct i2c_device_id *did)
+{
+ struct mt9p031 *mt9p031;
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct mt9p031_platform_data *pdata = client->dev.platform_data;
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ int ret;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_warn(&adapter->dev,
+ "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+ return -EIO;
+ }
+
+ mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
+ if (!mt9p031)
+ return -ENOMEM;
+
+ v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
+ mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
+
+ mt9p031->pdata = pdata;
+ mt9p031->rect.left = 0/*MT9P031_COLUMN_SKIP*/;
+ mt9p031->rect.top = 0/*MT9P031_ROW_SKIP*/;
+ mt9p031->rect.width = MT9P031_MAX_WIDTH;
+ mt9p031->rect.height = MT9P031_MAX_HEIGHT;
+
+ mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
+
+ mt9p031->format.width = MT9P031_MAX_WIDTH;
+ mt9p031->format.height = MT9P031_MAX_HEIGHT;
+ mt9p031->format.field = V4L2_FIELD_NONE;
+ mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+ /* mt9p031_idle() will reset the chip to default. */
+
+ mt9p031->xskip = 1;
+ mt9p031->yskip = 1;
+
+ mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
+ if (IS_ERR(mt9p031->reg_1v8)) {
+ ret = PTR_ERR(mt9p031->reg_1v8);
+ pr_err("Failed 1.8v regulator: %d\n", ret);
+ goto e1v8;
+ }
+
+ mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
+ if (IS_ERR(mt9p031->reg_2v8)) {
+ ret = PTR_ERR(mt9p031->reg_2v8);
+ pr_err("Failed 2.8v regulator: %d\n", ret);
+ goto e2v8;
+ }
+
+ ret = mt9p031_power_on(mt9p031);
+ if (ret) {
+ pr_err("Failed to power on device: %d\n", ret);
+ goto pwron;
+ }
+ /* turn on VDD_IO */
+ ret = regulator_enable(mt9p031->reg_2v8);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ goto e2v8en;
+ }
+
+ if (pdata->reset)
+ pdata->reset(sd, 1);
+
+ msleep(50);
+
+ if (pdata->reset)
+ pdata->reset(sd, 0);
+
+ msleep(50);
+
+ ret = mt9p031_video_probe(client);
+ if (ret)
+ goto evprobe;
+
+ mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
+ if (ret)
+ goto evprobe;
+
+ mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ return ret;
+
+evprobe:
+ regulator_disable(mt9p031->reg_2v8);
+e2v8en:
+ mt9p031_power_off(mt9p031);
+pwron:
+ regulator_put(mt9p031->reg_2v8);
+e2v8:
+ regulator_put(mt9p031->reg_1v8);
+e1v8:
+ kfree(mt9p031);
+ return ret;
+}
+
+static int mt9p031_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ v4l2_device_unregister_subdev(sd);
+ media_entity_cleanup(&sd->entity);
+ regulator_disable(mt9p031->reg_2v8);
+ regulator_put(mt9p031->reg_2v8);
+ regulator_put(mt9p031->reg_1v8);
+ kfree(mt9p031);
+
+ return 0;
+}
+
+static const struct i2c_device_id mt9p031_id[] = {
+ { "mt9p031", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mt9p031_id);
+
+static struct i2c_driver mt9p031_i2c_driver = {
+ .driver = {
+ .name = "mt9p031",
+ },
+ .probe = mt9p031_probe,
+ .remove = mt9p031_remove,
+ .id_table = mt9p031_id,
+};
+
+static int __init mt9p031_mod_init(void)
+{
+ return i2c_add_driver(&mt9p031_i2c_driver);
+}
+
+static void __exit mt9p031_mod_exit(void)
+{
+ i2c_del_driver(&mt9p031_i2c_driver);
+}
+
+module_init(mt9p031_mod_init);
+module_exit(mt9p031_mod_exit);
+
+MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
+MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
new file mode 100644
index 0000000..ad37eb3
--- /dev/null
+++ b/include/media/mt9p031.h
@@ -0,0 +1,11 @@
+#ifndef MT9P031_H
+#define MT9P031_H
+
+struct v4l2_subdev;
+
+struct mt9p031_platform_data {
+ int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
+ int (*reset)(struct v4l2_subdev *subdev, int active);
+};
+
+#endif
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index b3edb67..97919f2 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -297,6 +297,7 @@ enum {
V4L2_IDENT_MT9T112 = 45022,
V4L2_IDENT_MT9V111 = 45031,
V4L2_IDENT_MT9V112 = 45032,
+ V4L2_IDENT_MT9P031 = 45033,
/* HV7131R CMOS sensor: just ident 46000 */
V4L2_IDENT_HV7131R = 46000,
--
1.7.0.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 9:28 ` Javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-arm-kernel
It has been tested in beagleboard xM, using LI-5M03 module.
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
drivers/media/video/Kconfig | 8 +
drivers/media/video/Makefile | 1 +
drivers/media/video/mt9p031.c | 773 +++++++++++++++++++++++++++++++++++++++
include/media/mt9p031.h | 11 +
include/media/v4l2-chip-ident.h | 1 +
5 files changed, 794 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/mt9p031.c
create mode 100644 include/media/mt9p031.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 00f51dd..32df8bd 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -329,6 +329,14 @@ config VIDEO_OV7670
OV7670 VGA camera. It currently only works with the M88ALP01
controller.
+config VIDEO_MT9P031
+ tristate "Micron MT9P031 support"
+ depends on I2C && VIDEO_V4L2
+ ---help---
+ This driver supports MT9P031 cameras from Micron
+ This is a Video4Linux2 sensor-level driver for the Micron
+ mt0p031 5 Mpixel camera.
+
config VIDEO_MT9V011
tristate "Micron mt9v011 sensor support"
depends on I2C && VIDEO_V4L2
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index ace5d8b..912b29b 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o
obj-$(CONFIG_VIDEO_OV7670) += ov7670.o
obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o
obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o
+obj-$(CONFIG_VIDEO_MT9P031) += mt9p031.o
obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o
obj-$(CONFIG_VIDEO_SR030PC30) += sr030pc30.o
obj-$(CONFIG_VIDEO_NOON010PC30) += noon010pc30.o
diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
new file mode 100644
index 0000000..850cfec
--- /dev/null
+++ b/drivers/media/video/mt9p031.c
@@ -0,0 +1,773 @@
+/*
+ * Driver for MT9P031 CMOS Image Sensor from Aptina
+ *
+ * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
+ *
+ * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
+ *
+ * Based on the MT9V032 driver and Bastian Hecht's code.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/log2.h>
+#include <linux/pm.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <media/v4l2-subdev.h>
+#include <linux/videodev2.h>
+
+#include <media/mt9p031.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/v4l2-subdev.h>
+#include <media/v4l2-device.h>
+
+/* mt9p031 selected register addresses */
+#define MT9P031_CHIP_VERSION 0x00
+#define MT9P031_CHIP_VERSION_VALUE 0x1801
+#define MT9P031_ROW_START 0x01
+#define MT9P031_ROW_START_SKIP 54
+#define MT9P031_COLUMN_START 0x02
+#define MT9P031_COLUMN_START_SKIP 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_RESET 0x0d
+#define MT9P031_RESET_ENABLE 1
+#define MT9P031_RESET_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_MAX_HEIGHT 1944
+#define MT9P031_MAX_WIDTH 2592
+#define MT9P031_MIN_HEIGHT 2
+#define MT9P031_MIN_WIDTH 18
+
+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;
+ int model; /* V4L2_IDENT_MT9P031* codes from v4l2-chip-ident.h */
+ u16 xskip;
+ u16 yskip;
+ struct regulator *reg_1v8, *reg_2v8;
+};
+
+static struct mt9p031 *to_mt9p031(const struct i2c_client *client)
+{
+ return container_of(i2c_get_clientdata(client), struct mt9p031, subdev);
+}
+
+static int reg_read(struct i2c_client *client, const u8 reg)
+{
+ s32 data = i2c_smbus_read_word_data(client, reg);
+ return data < 0 ? data : swab16(data);
+}
+
+static int reg_write(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ return i2c_smbus_write_word_data(client, reg, swab16(data));
+}
+
+static int reg_set(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ int ret;
+
+ ret = reg_read(client, reg);
+ if (ret < 0)
+ return ret;
+ return reg_write(client, reg, ret | data);
+}
+
+static int reg_clear(struct i2c_client *client, const u8 reg,
+ const u16 data)
+{
+ int ret;
+
+ ret = reg_read(client, reg);
+ if (ret < 0)
+ return ret;
+ return reg_write(client, reg, ret & ~data);
+}
+
+static int mt9p031_idle(struct i2c_client *client)
+{
+ int ret;
+
+ /* Disable chip output, synchronous option update */
+ ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_ENABLE);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_DISABLE);
+ if (ret < 0)
+ goto err;
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ if (ret < 0)
+ goto err;
+ return 0;
+err:
+ return -EIO;
+}
+
+static int mt9p031_power_on(struct mt9p031 *mt9p031)
+{
+ int ret;
+
+ if (mt9p031->pdata->set_xclk)
+ mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
+ /* turn on VDD_IO */
+ ret = regulator_enable(mt9p031->reg_2v8);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ return -1;
+ }
+ return 0;
+}
+
+static void mt9p031_power_off(struct mt9p031 *mt9p031)
+{
+ if (mt9p031->pdata->set_xclk)
+ mt9p031->pdata->set_xclk(&mt9p031->subdev, 0);
+ regulator_disable(mt9p031->reg_2v8);
+}
+
+static int mt9p031_enum_mbus_code(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ if (code->pad || code->index)
+ return -EINVAL;
+
+ code->code = mt9p031->format.code;
+
+ return 0;
+}
+
+static struct v4l2_mbus_framefmt *mt9p031_get_pad_format(
+ struct mt9p031 *mt9p031,
+ struct v4l2_subdev_fh *fh,
+ unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(fh, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &mt9p031->format;
+ default:
+ return NULL;
+ }
+}
+
+static struct v4l2_rect *mt9p031_get_pad_crop(struct mt9p031 *mt9p031,
+ struct v4l2_subdev_fh *fh, unsigned int pad, u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(fh, pad);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &mt9p031->rect;
+ default:
+ return NULL;
+ }
+}
+
+static int mt9p031_get_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_rect *rect = mt9p031_get_pad_crop(mt9p031, fh, crop->pad,
+ crop->which);
+ if (!rect)
+ return -EINVAL;
+
+ crop->rect = *rect;
+
+ return 0;
+}
+
+static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
+{
+ unsigned int skip;
+
+ if (source - source / 4 < *target) {
+ *target = source;
+ return 1;
+ }
+
+ skip = DIV_ROUND_CLOSEST(source, *target);
+ if (skip > max_skip)
+ skip = max_skip;
+ *target = 2 * DIV_ROUND_UP(source, 2 * skip);
+
+ return skip;
+}
+
+static int mt9p031_set_params(struct i2c_client *client,
+ struct v4l2_rect *rect, u16 xskip, u16 yskip)
+{
+ struct mt9p031 *mt9p031 = to_mt9p031(client);
+ int ret;
+ u16 xbin, ybin;
+ const u16 hblank = MT9P031_H_BLANKING_VALUE,
+ vblank = MT9P031_V_BLANKING_VALUE;
+
+ /*
+ * TODO: Attention! When implementing horizontal flipping, adjust
+ * alignment according to R2 "Column Start" description in the datasheet
+ */
+ if (xskip & 1) {
+ xbin = 1;
+ rect->left &= ~3;
+ } else if (xskip & 2) {
+ xbin = 2;
+ rect->left &= ~7;
+ } else {
+ xbin = 4;
+ rect->left &= ~15;
+ }
+
+ ybin = min(yskip, (u16)4);
+
+ rect->top &= ~1;
+
+ /* Disable register update, reconfigure atomically */
+ ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_SYN);
+ if (ret < 0)
+ goto err;
+
+ dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
+ xskip, yskip, rect->width, rect->height, rect->left, rect->top);
+
+ /* Blanking and start values - default... */
+ ret = reg_write(client, MT9P031_H_BLANKING, hblank);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_V_BLANKING, vblank);
+ if (ret < 0)
+ goto err;
+
+ if (yskip != mt9p031->yskip || xskip != mt9p031->xskip) {
+ /* Binning, skipping */
+ ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
+ ((xbin - 1) << 4) | (xskip - 1));
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
+ ((ybin - 1) << 4) | (yskip - 1));
+ if (ret < 0)
+ goto err;
+ }
+ dev_dbg(&client->dev, "new physical left %u, top %u\n",
+ rect->left, rect->top);
+
+ ret = reg_write(client, MT9P031_COLUMN_START,
+ rect->left + MT9P031_COLUMN_START_SKIP);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_ROW_START,
+ rect->top + MT9P031_ROW_START_SKIP);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_WINDOW_WIDTH,
+ rect->width - 1);
+ if (ret < 0)
+ goto err;
+ ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
+ rect->height - 1);
+ if (ret < 0)
+ goto err;
+
+ /* Re-enable register update, commit all changes */
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_SYN);
+ if (ret < 0)
+ goto err;
+
+ mt9p031->xskip = xskip;
+ mt9p031->yskip = yskip;
+ return ret;
+err:
+ return -1;
+}
+
+static int mt9p031_set_crop(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_mbus_framefmt *f;
+ struct v4l2_rect *c;
+ struct v4l2_rect rect;
+ u16 xskip, yskip;
+ s32 width, height;
+ int ret;
+
+ pr_info("%s(%ux%u@%u:%u : %u)\n", __func__,
+ crop->rect.width, crop->rect.height,
+ crop->rect.left, crop->rect.top, crop->which);
+
+ /*
+ * Clamp the crop rectangle boundaries and align them to a multiple of 2
+ * pixels.
+ */
+ rect.width = ALIGN(clamp(crop->rect.width,
+ MT9P031_MIN_WIDTH, MT9P031_MAX_WIDTH), 2);
+ rect.height = ALIGN(clamp(crop->rect.height,
+ MT9P031_MIN_HEIGHT, MT9P031_MAX_HEIGHT), 2);
+ rect.left = ALIGN(clamp(crop->rect.left,
+ 0, MT9P031_MAX_WIDTH - rect.width), 2);
+ rect.top = ALIGN(clamp(crop->rect.top,
+ 0, MT9P031_MAX_HEIGHT - rect.height), 2);
+
+ c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
+
+ if (rect.width != c->width || rect.height != c->height) {
+ /*
+ * Reset the output image size if the crop rectangle size has
+ * been modified.
+ */
+ f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
+ crop->which);
+ width = f->width;
+ height = f->height;
+
+ xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
+ yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
+ } else {
+ xskip = mt9p031->xskip;
+ yskip = mt9p031->yskip;
+ f = NULL;
+ }
+
+ if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ ret = mt9p031_set_params(client, &rect, xskip, yskip);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (f) {
+ f->width = width;
+ f->height = height;
+ }
+
+ *c = rect;
+ crop->rect = rect;
+
+ return 0;
+}
+
+static int mt9p031_get_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ fmt->format =
+ *mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+ return 0;
+}
+
+static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
+ s32 max_skip, s32 max)
+{
+ unsigned int skip;
+
+ if (*source - *source / 4 < target) {
+ *source = target;
+ return 1;
+ }
+
+ skip = min(max, *source + target / 2) / target;
+ if (skip > max_skip)
+ skip = max_skip;
+ *source = target * skip;
+
+ return skip;
+}
+
+static int mt9p031_fmt_validate(struct v4l2_subdev *sd,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ struct v4l2_mbus_framefmt *format = &fmt->format;
+
+ if (format->code != mt9p031->format.code || fmt->pad)
+ return -EINVAL;
+
+ format->colorspace = V4L2_COLORSPACE_SRGB;
+ format->width = clamp_t(int, ALIGN(format->width, 2), 2,
+ MT9P031_MAX_WIDTH);
+ format->height = clamp_t(int, ALIGN(format->height, 2), 2,
+ MT9P031_MAX_HEIGHT);
+ format->field = V4L2_FIELD_NONE;
+
+ return 0;
+}
+
+static int mt9p031_set_format(struct v4l2_subdev *sd,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ struct v4l2_subdev_format sdf = *fmt;
+ struct v4l2_mbus_framefmt *f, *format = &sdf.format;
+ struct v4l2_rect *c, rect;
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+ u16 xskip, yskip;
+ int ret;
+
+ ret = mt9p031_fmt_validate(sd, &sdf);
+ if (ret < 0)
+ return ret;
+
+ f = mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
+
+ if (f->width != format->width || f->height != format->height) {
+ c = mt9p031_get_pad_crop(mt9p031, fh, fmt->pad, fmt->which);
+
+ rect.width = c->width;
+ rect.height = c->height;
+
+ xskip = mt9p031_skip_for_scale(&rect.width, format->width, 7,
+ MT9P031_MAX_WIDTH);
+ if (rect.width + c->left > MT9P031_MAX_WIDTH)
+ rect.left = (MT9P031_MAX_WIDTH - rect.width) / 2;
+ else
+ rect.left = c->left;
+ yskip = mt9p031_skip_for_scale(&rect.height, format->height, 8,
+ MT9P031_MAX_HEIGHT);
+ if (rect.height + c->top > MT9P031_MAX_HEIGHT)
+ rect.top = (MT9P031_MAX_HEIGHT - rect.height) / 2;
+ else
+ rect.top = c->top;
+ } else {
+ xskip = mt9p031->xskip;
+ yskip = mt9p031->yskip;
+ c = NULL;
+ }
+
+ pr_info("%s(%ux%u : %u)\n", __func__,
+ format->width, format->height, fmt->which);
+
+ if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ /* mt9p031_set_params() doesn't change width and height */
+ ret = mt9p031_set_params(client, &rect, xskip, yskip);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (c)
+ *c = rect;
+
+ *f = *format;
+ fmt->format = *format;
+
+ return 0;
+}
+
+static int mt9p031_s_stream(struct v4l2_subdev *sd, int enable)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(sd);
+ int ret;
+
+ if (enable) {
+ /* Switch to master "normal" mode */
+ ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ } else {
+ /* Stop sensor readout */
+ ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
+ MT9P031_OUTPUT_CONTROL_CEN);
+ }
+ if (ret < 0)
+ return -EIO;
+
+ return 0;
+}
+
+/*
+ * Interface active, can use i2c. If it fails, it can indeed mean, that
+ * this wasn't our capture interface, so, we wait for the right one
+ */
+static int mt9p031_video_probe(struct i2c_client *client)
+{
+ struct mt9p031 *mt9p031 = to_mt9p031(client);
+ s32 data;
+ int ret;
+
+ /* Read out the chip version register */
+ data = reg_read(client, MT9P031_CHIP_VERSION);
+
+ switch (data) {
+ case MT9P031_CHIP_VERSION_VALUE:
+ mt9p031->model = V4L2_IDENT_MT9P031;
+ break;
+ default:
+ dev_err(&client->dev,
+ "No MT9P031 chip detected, register read %x\n", data);
+ return -ENODEV;
+ }
+
+ dev_info(&client->dev, "Detected a MT9P031 chip ID %x\n", data);
+
+ ret = mt9p031_idle(client);
+ if (ret < 0)
+ dev_err(&client->dev, "Failed to initialise the camera\n");
+
+ return ret;
+}
+
+static int mt9p031_registered(struct v4l2_subdev *sd)
+{
+ struct mt9p031 *mt9p031;
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ mt9p031_power_off(mt9p031);
+ return 0;
+}
+
+static int mt9p031_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct mt9p031 *mt9p031;
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ return mt9p031_power_on(mt9p031);
+}
+
+static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
+{
+ struct mt9p031 *mt9p031;
+
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ mt9p031_power_off(mt9p031);
+ return 0;
+}
+
+static int mt9p031_set_power(struct v4l2_subdev *sd, int on)
+{
+ struct mt9p031 *mt9p031;
+ int ret = 0;
+
+ mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ if (on) {
+ ret = mt9p031_power_on(mt9p031);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ goto out;
+ }
+ } else {
+ mt9p031_power_off(mt9p031);
+ }
+out:
+ return ret;
+}
+
+static struct v4l2_subdev_core_ops mt9p031_subdev_core_ops = {
+ .s_power = mt9p031_set_power,
+};
+
+static struct v4l2_subdev_video_ops mt9p031_subdev_video_ops = {
+ .s_stream = mt9p031_s_stream,
+};
+
+static struct v4l2_subdev_pad_ops mt9p031_subdev_pad_ops = {
+ .enum_mbus_code = mt9p031_enum_mbus_code,
+ .get_fmt = mt9p031_get_format,
+ .set_fmt = mt9p031_set_format,
+ .get_crop = mt9p031_get_crop,
+ .set_crop = mt9p031_set_crop,
+};
+
+static struct v4l2_subdev_ops mt9p031_subdev_ops = {
+ .core = &mt9p031_subdev_core_ops,
+ .video = &mt9p031_subdev_video_ops,
+ .pad = &mt9p031_subdev_pad_ops,
+};
+
+static const struct v4l2_subdev_internal_ops mt9p031_subdev_internal_ops = {
+ .registered = mt9p031_registered,
+ .open = mt9p031_open,
+ .close = mt9p031_close,
+};
+
+static int mt9p031_probe(struct i2c_client *client,
+ const struct i2c_device_id *did)
+{
+ struct mt9p031 *mt9p031;
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct mt9p031_platform_data *pdata = client->dev.platform_data;
+ struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
+ int ret;
+
+ if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_warn(&adapter->dev,
+ "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+ return -EIO;
+ }
+
+ mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
+ if (!mt9p031)
+ return -ENOMEM;
+
+ v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
+ mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
+
+ mt9p031->pdata = pdata;
+ mt9p031->rect.left = 0/*MT9P031_COLUMN_SKIP*/;
+ mt9p031->rect.top = 0/*MT9P031_ROW_SKIP*/;
+ mt9p031->rect.width = MT9P031_MAX_WIDTH;
+ mt9p031->rect.height = MT9P031_MAX_HEIGHT;
+
+ mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
+
+ mt9p031->format.width = MT9P031_MAX_WIDTH;
+ mt9p031->format.height = MT9P031_MAX_HEIGHT;
+ mt9p031->format.field = V4L2_FIELD_NONE;
+ mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+ /* mt9p031_idle() will reset the chip to default. */
+
+ mt9p031->xskip = 1;
+ mt9p031->yskip = 1;
+
+ mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
+ if (IS_ERR(mt9p031->reg_1v8)) {
+ ret = PTR_ERR(mt9p031->reg_1v8);
+ pr_err("Failed 1.8v regulator: %d\n", ret);
+ goto e1v8;
+ }
+
+ mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
+ if (IS_ERR(mt9p031->reg_2v8)) {
+ ret = PTR_ERR(mt9p031->reg_2v8);
+ pr_err("Failed 2.8v regulator: %d\n", ret);
+ goto e2v8;
+ }
+
+ ret = mt9p031_power_on(mt9p031);
+ if (ret) {
+ pr_err("Failed to power on device: %d\n", ret);
+ goto pwron;
+ }
+ /* turn on VDD_IO */
+ ret = regulator_enable(mt9p031->reg_2v8);
+ if (ret) {
+ pr_err("Failed to enable 2.8v regulator: %d\n", ret);
+ goto e2v8en;
+ }
+
+ if (pdata->reset)
+ pdata->reset(sd, 1);
+
+ msleep(50);
+
+ if (pdata->reset)
+ pdata->reset(sd, 0);
+
+ msleep(50);
+
+ ret = mt9p031_video_probe(client);
+ if (ret)
+ goto evprobe;
+
+ mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
+ if (ret)
+ goto evprobe;
+
+ mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+ return ret;
+
+evprobe:
+ regulator_disable(mt9p031->reg_2v8);
+e2v8en:
+ mt9p031_power_off(mt9p031);
+pwron:
+ regulator_put(mt9p031->reg_2v8);
+e2v8:
+ regulator_put(mt9p031->reg_1v8);
+e1v8:
+ kfree(mt9p031);
+ return ret;
+}
+
+static int mt9p031_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *sd = i2c_get_clientdata(client);
+ struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
+
+ v4l2_device_unregister_subdev(sd);
+ media_entity_cleanup(&sd->entity);
+ regulator_disable(mt9p031->reg_2v8);
+ regulator_put(mt9p031->reg_2v8);
+ regulator_put(mt9p031->reg_1v8);
+ kfree(mt9p031);
+
+ return 0;
+}
+
+static const struct i2c_device_id mt9p031_id[] = {
+ { "mt9p031", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mt9p031_id);
+
+static struct i2c_driver mt9p031_i2c_driver = {
+ .driver = {
+ .name = "mt9p031",
+ },
+ .probe = mt9p031_probe,
+ .remove = mt9p031_remove,
+ .id_table = mt9p031_id,
+};
+
+static int __init mt9p031_mod_init(void)
+{
+ return i2c_add_driver(&mt9p031_i2c_driver);
+}
+
+static void __exit mt9p031_mod_exit(void)
+{
+ i2c_del_driver(&mt9p031_i2c_driver);
+}
+
+module_init(mt9p031_mod_init);
+module_exit(mt9p031_mod_exit);
+
+MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
+MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9p031.h b/include/media/mt9p031.h
new file mode 100644
index 0000000..ad37eb3
--- /dev/null
+++ b/include/media/mt9p031.h
@@ -0,0 +1,11 @@
+#ifndef MT9P031_H
+#define MT9P031_H
+
+struct v4l2_subdev;
+
+struct mt9p031_platform_data {
+ int (*set_xclk)(struct v4l2_subdev *subdev, int hz);
+ int (*reset)(struct v4l2_subdev *subdev, int active);
+};
+
+#endif
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index b3edb67..97919f2 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -297,6 +297,7 @@ enum {
V4L2_IDENT_MT9T112 = 45022,
V4L2_IDENT_MT9V111 = 45031,
V4L2_IDENT_MT9V112 = 45032,
+ V4L2_IDENT_MT9P031 = 45033,
/* HV7131R CMOS sensor: just ident 46000 */
V4L2_IDENT_HV7131R = 46000,
--
1.7.0.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
2011-05-17 9:28 ` No subject Javier Martin
@ 2011-05-17 9:28 ` Javier Martin
-1 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-media
Cc: g.liakhovetski, laurent.pinchart, carlighting, beagleboard,
linux-arm-kernel, Javier Martin
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
arch/arm/mach-omap2/board-omap3beagle.c | 130 ++++++++++++++++++++++++++++++-
arch/arm/mach-omap2/devices.c | 2 +-
2 files changed, 127 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 33007fd..ccd72dc 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -24,15 +24,20 @@
#include <linux/input.h>
#include <linux/gpio_keys.h>
#include <linux/opp.h>
+#include <linux/i2c.h>
+#include <linux/mm.h>
+#include <linux/videodev2.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
#include <linux/mtd/nand.h>
#include <linux/mmc/host.h>
-
+#include <linux/gpio.h>
#include <linux/regulator/machine.h>
#include <linux/i2c/twl.h>
+#include <media/mt9p031.h>
+
#include <mach/hardware.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -47,12 +52,18 @@
#include <plat/nand.h>
#include <plat/usb.h>
#include <plat/omap_device.h>
+#include <plat/i2c.h>
#include "mux.h"
#include "hsmmc.h"
#include "timer-gp.h"
#include "pm.h"
+#include "devices.h"
+#include "../../../drivers/media/video/omap3isp/isp.h"
+#include "../../../drivers/media/video/omap3isp/ispreg.h"
+#define MT9P031_RESET_GPIO 98
+#define MT9P031_XCLK ISP_XCLK_A
#define NAND_BLOCK_SIZE SZ_128K
/*
@@ -273,6 +284,44 @@ static struct regulator_consumer_supply beagle_vsim_supply = {
static struct gpio_led gpio_leds[];
+static struct regulator_consumer_supply beagle_vaux3_supply = {
+ .supply = "cam_1v8",
+};
+
+static struct regulator_consumer_supply beagle_vaux4_supply = {
+ .supply = "cam_2v8",
+};
+
+/* VAUX3 for CAM_1V8 */
+static struct regulator_init_data beagle_vaux3 = {
+ .constraints = {
+ .min_uV = 1800000,
+ .max_uV = 1800000,
+ .apply_uV = true,
+ .valid_modes_mask = REGULATOR_MODE_NORMAL
+ | REGULATOR_MODE_STANDBY,
+ .valid_ops_mask = REGULATOR_CHANGE_MODE
+ | REGULATOR_CHANGE_STATUS,
+ },
+ .num_consumer_supplies = 1,
+ .consumer_supplies = &beagle_vaux3_supply,
+};
+
+/* VAUX4 for CAM_2V8 */
+static struct regulator_init_data beagle_vaux4 = {
+ .constraints = {
+ .min_uV = 1800000,
+ .max_uV = 1800000,
+ .apply_uV = true,
+ .valid_modes_mask = REGULATOR_MODE_NORMAL
+ | REGULATOR_MODE_STANDBY,
+ .valid_ops_mask = REGULATOR_CHANGE_MODE
+ | REGULATOR_CHANGE_STATUS,
+ },
+ .num_consumer_supplies = 1,
+ .consumer_supplies = &beagle_vaux4_supply,
+};
+
static int beagle_twl_gpio_setup(struct device *dev,
unsigned gpio, unsigned ngpio)
{
@@ -309,6 +358,15 @@ static int beagle_twl_gpio_setup(struct device *dev,
pr_err("%s: unable to configure EHCI_nOC\n", __func__);
}
+ if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
+ /*
+ * Power on camera interface - only on pre-production, not
+ * needed on production boards
+ */
+ gpio_request(gpio + 2, "CAM_EN");
+ gpio_direction_output(gpio + 2, 1);
+ }
+
/*
* TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active
* high / others active low)
@@ -451,6 +509,8 @@ static struct twl4030_platform_data beagle_twldata = {
.vsim = &beagle_vsim,
.vdac = &beagle_vdac,
.vpll2 = &beagle_vpll2,
+ .vaux3 = &beagle_vaux3,
+ .vaux4 = &beagle_vaux4,
};
static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
@@ -463,15 +523,16 @@ static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
};
static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
- {
- I2C_BOARD_INFO("eeprom", 0x50),
- },
+ {
+ I2C_BOARD_INFO("eeprom", 0x50),
+ },
};
static int __init omap3_beagle_i2c_init(void)
{
omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo,
ARRAY_SIZE(beagle_i2c_boardinfo));
+ omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */
/* Bus 3 is attached to the DVI port where devices like the pico DLP
* projector don't work reliably with 400kHz */
omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
@@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
return;
}
+extern struct platform_device omap3isp_device;
+
+static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
+{
+ struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
+ int ret;
+
+ ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
+ return 0;
+}
+
+static int beagle_cam_reset(struct v4l2_subdev *subdev, int active)
+{
+ /* Set RESET_BAR to !active */
+ gpio_set_value(MT9P031_RESET_GPIO, !active);
+
+ return 0;
+}
+
+static struct mt9p031_platform_data beagle_mt9p031_platform_data = {
+ .set_xclk = beagle_cam_set_xclk,
+ .reset = beagle_cam_reset,
+};
+
+static struct i2c_board_info mt9p031_camera_i2c_device = {
+ I2C_BOARD_INFO("mt9p031", 0x48),
+ .platform_data = &beagle_mt9p031_platform_data,
+};
+
+static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = {
+ {
+ .board_info = &mt9p031_camera_i2c_device,
+ .i2c_adapter_id = 2,
+ },
+ { NULL, 0, },
+};
+
+static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = {
+ {
+ .subdevs = mt9p031_camera_subdevs,
+ .interface = ISP_INTERFACE_PARALLEL,
+ .bus = {
+ .parallel = {
+ .data_lane_shift = 0,
+ .clk_pol = 1,
+ .bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
+ }
+ },
+ },
+ { },
+};
+
+static struct isp_platform_data beagle_isp_platform_data = {
+ .subdevs = beagle_camera_subdevs,
+};
+
static void __init omap3_beagle_init(void)
{
omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
@@ -679,6 +796,11 @@ static void __init omap3_beagle_init(void)
beagle_display_init();
beagle_opp_init();
+
+ /* Enable camera */
+ gpio_request(MT9P031_RESET_GPIO, "cam_rst");
+ gpio_direction_output(MT9P031_RESET_GPIO, 0);
+ omap3_init_camera(&beagle_isp_platform_data);
}
MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 7b85585..ae5ba25 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -200,7 +200,7 @@ static struct resource omap3isp_resources[] = {
}
};
-static struct platform_device omap3isp_device = {
+struct platform_device omap3isp_device = {
.name = "omap3isp",
.id = -1,
.num_resources = ARRAY_SIZE(omap3isp_resources),
--
1.7.0.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
@ 2011-05-17 9:28 ` Javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: Javier Martin @ 2011-05-17 9:28 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
arch/arm/mach-omap2/board-omap3beagle.c | 130 ++++++++++++++++++++++++++++++-
arch/arm/mach-omap2/devices.c | 2 +-
2 files changed, 127 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 33007fd..ccd72dc 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -24,15 +24,20 @@
#include <linux/input.h>
#include <linux/gpio_keys.h>
#include <linux/opp.h>
+#include <linux/i2c.h>
+#include <linux/mm.h>
+#include <linux/videodev2.h>
#include <linux/mtd/mtd.h>
#include <linux/mtd/partitions.h>
#include <linux/mtd/nand.h>
#include <linux/mmc/host.h>
-
+#include <linux/gpio.h>
#include <linux/regulator/machine.h>
#include <linux/i2c/twl.h>
+#include <media/mt9p031.h>
+
#include <mach/hardware.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -47,12 +52,18 @@
#include <plat/nand.h>
#include <plat/usb.h>
#include <plat/omap_device.h>
+#include <plat/i2c.h>
#include "mux.h"
#include "hsmmc.h"
#include "timer-gp.h"
#include "pm.h"
+#include "devices.h"
+#include "../../../drivers/media/video/omap3isp/isp.h"
+#include "../../../drivers/media/video/omap3isp/ispreg.h"
+#define MT9P031_RESET_GPIO 98
+#define MT9P031_XCLK ISP_XCLK_A
#define NAND_BLOCK_SIZE SZ_128K
/*
@@ -273,6 +284,44 @@ static struct regulator_consumer_supply beagle_vsim_supply = {
static struct gpio_led gpio_leds[];
+static struct regulator_consumer_supply beagle_vaux3_supply = {
+ .supply = "cam_1v8",
+};
+
+static struct regulator_consumer_supply beagle_vaux4_supply = {
+ .supply = "cam_2v8",
+};
+
+/* VAUX3 for CAM_1V8 */
+static struct regulator_init_data beagle_vaux3 = {
+ .constraints = {
+ .min_uV = 1800000,
+ .max_uV = 1800000,
+ .apply_uV = true,
+ .valid_modes_mask = REGULATOR_MODE_NORMAL
+ | REGULATOR_MODE_STANDBY,
+ .valid_ops_mask = REGULATOR_CHANGE_MODE
+ | REGULATOR_CHANGE_STATUS,
+ },
+ .num_consumer_supplies = 1,
+ .consumer_supplies = &beagle_vaux3_supply,
+};
+
+/* VAUX4 for CAM_2V8 */
+static struct regulator_init_data beagle_vaux4 = {
+ .constraints = {
+ .min_uV = 1800000,
+ .max_uV = 1800000,
+ .apply_uV = true,
+ .valid_modes_mask = REGULATOR_MODE_NORMAL
+ | REGULATOR_MODE_STANDBY,
+ .valid_ops_mask = REGULATOR_CHANGE_MODE
+ | REGULATOR_CHANGE_STATUS,
+ },
+ .num_consumer_supplies = 1,
+ .consumer_supplies = &beagle_vaux4_supply,
+};
+
static int beagle_twl_gpio_setup(struct device *dev,
unsigned gpio, unsigned ngpio)
{
@@ -309,6 +358,15 @@ static int beagle_twl_gpio_setup(struct device *dev,
pr_err("%s: unable to configure EHCI_nOC\n", __func__);
}
+ if (omap3_beagle_get_rev() == OMAP3BEAGLE_BOARD_XM) {
+ /*
+ * Power on camera interface - only on pre-production, not
+ * needed on production boards
+ */
+ gpio_request(gpio + 2, "CAM_EN");
+ gpio_direction_output(gpio + 2, 1);
+ }
+
/*
* TWL4030_GPIO_MAX + 0 == ledA, EHCI nEN_USB_PWR (out, XM active
* high / others active low)
@@ -451,6 +509,8 @@ static struct twl4030_platform_data beagle_twldata = {
.vsim = &beagle_vsim,
.vdac = &beagle_vdac,
.vpll2 = &beagle_vpll2,
+ .vaux3 = &beagle_vaux3,
+ .vaux4 = &beagle_vaux4,
};
static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
@@ -463,15 +523,16 @@ static struct i2c_board_info __initdata beagle_i2c_boardinfo[] = {
};
static struct i2c_board_info __initdata beagle_i2c_eeprom[] = {
- {
- I2C_BOARD_INFO("eeprom", 0x50),
- },
+ {
+ I2C_BOARD_INFO("eeprom", 0x50),
+ },
};
static int __init omap3_beagle_i2c_init(void)
{
omap_register_i2c_bus(1, 2600, beagle_i2c_boardinfo,
ARRAY_SIZE(beagle_i2c_boardinfo));
+ omap_register_i2c_bus(2, 100, NULL, 0); /* Enable I2C2 for camera */
/* Bus 3 is attached to the DVI port where devices like the pico DLP
* projector don't work reliably with 400kHz */
omap_register_i2c_bus(3, 100, beagle_i2c_eeprom, ARRAY_SIZE(beagle_i2c_eeprom));
@@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
return;
}
+extern struct platform_device omap3isp_device;
+
+static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
+{
+ struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
+ int ret;
+
+ ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
+ return 0;
+}
+
+static int beagle_cam_reset(struct v4l2_subdev *subdev, int active)
+{
+ /* Set RESET_BAR to !active */
+ gpio_set_value(MT9P031_RESET_GPIO, !active);
+
+ return 0;
+}
+
+static struct mt9p031_platform_data beagle_mt9p031_platform_data = {
+ .set_xclk = beagle_cam_set_xclk,
+ .reset = beagle_cam_reset,
+};
+
+static struct i2c_board_info mt9p031_camera_i2c_device = {
+ I2C_BOARD_INFO("mt9p031", 0x48),
+ .platform_data = &beagle_mt9p031_platform_data,
+};
+
+static struct isp_subdev_i2c_board_info mt9p031_camera_subdevs[] = {
+ {
+ .board_info = &mt9p031_camera_i2c_device,
+ .i2c_adapter_id = 2,
+ },
+ { NULL, 0, },
+};
+
+static struct isp_v4l2_subdevs_group beagle_camera_subdevs[] = {
+ {
+ .subdevs = mt9p031_camera_subdevs,
+ .interface = ISP_INTERFACE_PARALLEL,
+ .bus = {
+ .parallel = {
+ .data_lane_shift = 0,
+ .clk_pol = 1,
+ .bridge = ISPCTRL_PAR_BRIDGE_DISABLE,
+ }
+ },
+ },
+ { },
+};
+
+static struct isp_platform_data beagle_isp_platform_data = {
+ .subdevs = beagle_camera_subdevs,
+};
+
static void __init omap3_beagle_init(void)
{
omap3_mux_init(board_mux, OMAP_PACKAGE_CBB);
@@ -679,6 +796,11 @@ static void __init omap3_beagle_init(void)
beagle_display_init();
beagle_opp_init();
+
+ /* Enable camera */
+ gpio_request(MT9P031_RESET_GPIO, "cam_rst");
+ gpio_direction_output(MT9P031_RESET_GPIO, 0);
+ omap3_init_camera(&beagle_isp_platform_data);
}
MACHINE_START(OMAP3_BEAGLE, "OMAP3 Beagle Board")
diff --git a/arch/arm/mach-omap2/devices.c b/arch/arm/mach-omap2/devices.c
index 7b85585..ae5ba25 100644
--- a/arch/arm/mach-omap2/devices.c
+++ b/arch/arm/mach-omap2/devices.c
@@ -200,7 +200,7 @@ static struct resource omap3isp_resources[] = {
}
};
-static struct platform_device omap3isp_device = {
+struct platform_device omap3isp_device = {
.name = "omap3isp",
.id = -1,
.num_resources = ARRAY_SIZE(omap3isp_resources),
--
1.7.0.4
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 9:28 ` Javier Martin
@ 2011-05-17 11:33 ` Laurent Pinchart
-1 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-17 11:33 UTC (permalink / raw)
To: Javier Martin
Cc: linux-media, g.liakhovetski, carlighting, beagleboard, linux-arm-kernel
Hi Javier,
Thanks for the patch.
On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
> It has been tested in beagleboard xM, using LI-5M03 module.
>
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
[snip]
> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> new file mode 100644
> index 0000000..850cfec
> --- /dev/null
> +++ b/drivers/media/video/mt9p031.c
> @@ -0,0 +1,773 @@
> +/*
> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> + *
> + * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
> + *
> + * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * Based on the MT9V032 driver and Bastian Hecht's code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/log2.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/mt9p031.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +
> +/* mt9p031 selected register addresses */
> +#define MT9P031_CHIP_VERSION 0x00
> +#define MT9P031_CHIP_VERSION_VALUE 0x1801
> +#define MT9P031_ROW_START 0x01
> +#define MT9P031_ROW_START_SKIP 54
> +#define MT9P031_COLUMN_START 0x02
> +#define MT9P031_COLUMN_START_SKIP 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_RESET 0x0d
> +#define MT9P031_RESET_ENABLE 1
> +#define MT9P031_RESET_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_MAX_HEIGHT 1944
> +#define MT9P031_MAX_WIDTH 2592
> +#define MT9P031_MIN_HEIGHT 2
> +#define MT9P031_MIN_WIDTH 18
> +
> +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;
> + int model; /* V4L2_IDENT_MT9P031* codes from v4l2-chip-ident.h */
model is assigned by never read, you can remove it.
> + u16 xskip;
> + u16 yskip;
> + struct regulator *reg_1v8, *reg_2v8;
Please split this on two lines.
You never enable the 1.8V regulator, is that intentional ?
> +};
[snip]
> +static int reg_set(struct i2c_client *client, const u8 reg,
> + const u16 data)
> +{
> + int ret;
> +
> + ret = reg_read(client, reg);
> + if (ret < 0)
> + return ret;
> + return reg_write(client, reg, ret | data);
> +}
> +
> +static int reg_clear(struct i2c_client *client, const u8 reg,
> + const u16 data)
> +{
> + int ret;
> +
> + ret = reg_read(client, reg);
> + if (ret < 0)
> + return ret;
> + return reg_write(client, reg, ret & ~data);
> +}
I still think you should use a shadow copy of the register in the mt9p031
structure instead of reading the value back from the hardware. As these two
functions are only used to handle the output control register, you could
create an mt9p031_set_output_control(struct mt9p031 *mt9p031, u16 clear, u16
set) function instead.
> +static int mt9p031_idle(struct i2c_client *client)
This function resets the chip, what about calling it mt9p031_reset() instead ?
> +{
> + int ret;
> +
> + /* Disable chip output, synchronous option update */
> + ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_ENABLE);
> + if (ret < 0)
> + goto err;
You can return -EIO directly.
> + ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_DISABLE);
> + if (ret < 0)
> + goto err;
Here too.
> + ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_CEN);
> + if (ret < 0)
> + goto err;
And here too.
> + return 0;
> +err:
> + return -EIO;
> +}
[snip]
> +
> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> +{
> + int ret;
> +
> + if (mt9p031->pdata->set_xclk)
> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> + /* turn on VDD_IO */
> + ret = regulator_enable(mt9p031->reg_2v8);
> + if (ret) {
> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> + return -1;
> + }
I would enable the regulator first. As a general rule, chips should be powered
up before their I/Os are actively driven.
You need to restore registers here, otherwise all controls set by the user
will not be applied to the device.
> + return 0;
> +}
[snip]
> +static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
> +{
> + unsigned int skip;
> +
> + if (source - source / 4 < *target) {
> + *target = source;
> + return 1;
> + }
> +
> + skip = DIV_ROUND_CLOSEST(source, *target);
> + if (skip > max_skip)
> + skip = max_skip;
> + *target = 2 * DIV_ROUND_UP(source, 2 * skip);
> +
> + return skip;
> +}
> +
> +static int mt9p031_set_params(struct i2c_client *client,
> + struct v4l2_rect *rect, u16 xskip, u16 yskip)
set_params should apply the parameters, not change them. They should have
already been validated by the callers.
> +{
> + struct mt9p031 *mt9p031 = to_mt9p031(client);
> + int ret;
> + u16 xbin, ybin;
> + const u16 hblank = MT9P031_H_BLANKING_VALUE,
> + vblank = MT9P031_V_BLANKING_VALUE;
> +
> + /*
> + * TODO: Attention! When implementing horizontal flipping, adjust
> + * alignment according to R2 "Column Start" description in the datasheet
> + */
> + if (xskip & 1) {
> + xbin = 1;
> + rect->left &= ~3;
> + } else if (xskip & 2) {
> + xbin = 2;
> + rect->left &= ~7;
> + } else {
> + xbin = 4;
> + rect->left &= ~15;
> + }
> +
> + ybin = min(yskip, (u16)4);
> +
> + rect->top &= ~1;
> +
> + /* Disable register update, reconfigure atomically */
> + ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_SYN);
> + if (ret < 0)
> + goto err;
> +
> + dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
> + xskip, yskip, rect->width, rect->height, rect->left, rect->top);
> +
> + /* Blanking and start values - default... */
> + ret = reg_write(client, MT9P031_H_BLANKING, hblank);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_V_BLANKING, vblank);
> + if (ret < 0)
> + goto err;
> +
> + if (yskip != mt9p031->yskip || xskip != mt9p031->xskip) {
> + /* Binning, skipping */
> + ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
> + ((xbin - 1) << 4) | (xskip - 1));
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
> + ((ybin - 1) << 4) | (yskip - 1));
> + if (ret < 0)
> + goto err;
> + }
> + dev_dbg(&client->dev, "new physical left %u, top %u\n",
> + rect->left, rect->top);
> +
> + ret = reg_write(client, MT9P031_COLUMN_START,
> + rect->left + MT9P031_COLUMN_START_SKIP);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_ROW_START,
> + rect->top + MT9P031_ROW_START_SKIP);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_WINDOW_WIDTH,
> + rect->width - 1);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
> + rect->height - 1);
> + if (ret < 0)
> + goto err;
> +
> + /* Re-enable register update, commit all changes */
> + ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_SYN);
> + if (ret < 0)
> + goto err;
> +
> + mt9p031->xskip = xskip;
> + mt9p031->yskip = yskip;
> + return ret;
> +err:
> + return -1;
> +}
> +
> +static int mt9p031_set_crop(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_crop *crop)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> + struct v4l2_mbus_framefmt *f;
> + struct v4l2_rect *c;
> + struct v4l2_rect rect;
> + u16 xskip, yskip;
> + s32 width, height;
> + int ret;
> +
> + pr_info("%s(%ux%u@%u:%u : %u)\n", __func__,
> + crop->rect.width, crop->rect.height,
> + crop->rect.left, crop->rect.top, crop->which);
> +
> + /*
> + * Clamp the crop rectangle boundaries and align them to a multiple of 2
> + * pixels.
> + */
> + rect.width = ALIGN(clamp(crop->rect.width,
> + MT9P031_MIN_WIDTH, MT9P031_MAX_WIDTH), 2);
> + rect.height = ALIGN(clamp(crop->rect.height,
> + MT9P031_MIN_HEIGHT, MT9P031_MAX_HEIGHT), 2);
> + rect.left = ALIGN(clamp(crop->rect.left,
> + 0, MT9P031_MAX_WIDTH - rect.width), 2);
> + rect.top = ALIGN(clamp(crop->rect.top,
> + 0, MT9P031_MAX_HEIGHT - rect.height), 2);
> +
> + c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
> +
> + if (rect.width != c->width || rect.height != c->height) {
> + /*
> + * Reset the output image size if the crop rectangle size has
> + * been modified.
> + */
> + f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
> + crop->which);
> + width = f->width;
> + height = f->height;
> +
> + xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
> + yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
> + } else {
> + xskip = mt9p031->xskip;
> + yskip = mt9p031->yskip;
> + f = NULL;
> + }
> +
> + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (f) {
> + f->width = width;
> + f->height = height;
> + }
> +
> + *c = rect;
> + crop->rect = rect;
> +
> + return 0;
> +}
[snip]
> +static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
> + s32 max_skip, s32 max)
> +{
> + unsigned int skip;
> +
> + if (*source - *source / 4 < target) {
> + *source = target;
> + return 1;
> + }
> +
> + skip = min(max, *source + target / 2) / target;
> + if (skip > max_skip)
> + skip = max_skip;
> + *source = target * skip;
> +
> + return skip;
> +}
[snip]
> +static int mt9p031_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct v4l2_subdev_format sdf = *fmt;
> + struct v4l2_mbus_framefmt *f, *format = &sdf.format;
> + struct v4l2_rect *c, rect;
> + struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> + u16 xskip, yskip;
> + int ret;
> +
> + ret = mt9p031_fmt_validate(sd, &sdf);
> + if (ret < 0)
> + return ret;
> +
> + f = mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
> +
> + if (f->width != format->width || f->height != format->height) {
If width and height are identical to the current value, you can return
immediately.
> + c = mt9p031_get_pad_crop(mt9p031, fh, fmt->pad, fmt->which);
> +
> + rect.width = c->width;
> + rect.height = c->height;
> +
> + xskip = mt9p031_skip_for_scale(&rect.width, format->width, 7,
> + MT9P031_MAX_WIDTH);
> + if (rect.width + c->left > MT9P031_MAX_WIDTH)
> + rect.left = (MT9P031_MAX_WIDTH - rect.width) / 2;
> + else
> + rect.left = c->left;
> + yskip = mt9p031_skip_for_scale(&rect.height, format->height, 8,
> + MT9P031_MAX_HEIGHT);
> + if (rect.height + c->top > MT9P031_MAX_HEIGHT)
> + rect.top = (MT9P031_MAX_HEIGHT - rect.height) / 2;
> + else
> + rect.top = c->top;
> + } else {
> + xskip = mt9p031->xskip;
> + yskip = mt9p031->yskip;
> + c = NULL;
> + }
> +
> + pr_info("%s(%ux%u : %u)\n", __func__,
> + format->width, format->height, fmt->which);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + /* mt9p031_set_params() doesn't change width and height */
> + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (c)
> + *c = rect;
> +
> + *f = *format;
> + fmt->format = *format;
> +
> + return 0;
> +}
[snip]
> +static int mt9p031_registered(struct v4l2_subdev *sd)
> +{
> + struct mt9p031 *mt9p031;
> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> +
> + mt9p031_power_off(mt9p031);
What's that for ?
> + return 0;
> +}
[snip]
> +static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh) +{
> + struct mt9p031 *mt9p031;
> +
> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> +
> + mt9p031_power_off(mt9p031);
You need to reference count power handling, otherwise closing the subdev file
handle will turn the power off, even if the sensor is part of a pipeline
actively streaming video. Have a look at the mt9v032 driver to see how that's
done.
> + return 0;
> +}
[snip]
> +static int mt9p031_probe(struct i2c_client *client,
> + const struct i2c_device_id *did)
> +{
> + struct mt9p031 *mt9p031;
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct mt9p031_platform_data *pdata = client->dev.platform_data;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + int ret;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_warn(&adapter->dev,
> + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> + return -EIO;
> + }
> +
> + mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
> + if (!mt9p031)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
> + mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
> +
> + mt9p031->pdata = pdata;
> + mt9p031->rect.left = 0/*MT9P031_COLUMN_SKIP*/;
> + mt9p031->rect.top = 0/*MT9P031_ROW_SKIP*/;
No commented out code please.
> + mt9p031->rect.width = MT9P031_MAX_WIDTH;
> + mt9p031->rect.height = MT9P031_MAX_HEIGHT;
> +
> + mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
> +
> + mt9p031->format.width = MT9P031_MAX_WIDTH;
> + mt9p031->format.height = MT9P031_MAX_HEIGHT;
> + mt9p031->format.field = V4L2_FIELD_NONE;
> + mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
> +
> + /* mt9p031_idle() will reset the chip to default. */
There's no mt9p032_idle() call here.
> +
> + mt9p031->xskip = 1;
> + mt9p031->yskip = 1;
> +
> + mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
> + if (IS_ERR(mt9p031->reg_1v8)) {
> + ret = PTR_ERR(mt9p031->reg_1v8);
> + pr_err("Failed 1.8v regulator: %d\n", ret);
> + goto e1v8;
> + }
> +
> + mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
> + if (IS_ERR(mt9p031->reg_2v8)) {
> + ret = PTR_ERR(mt9p031->reg_2v8);
> + pr_err("Failed 2.8v regulator: %d\n", ret);
> + goto e2v8;
> + }
> +
> + ret = mt9p031_power_on(mt9p031);
> + if (ret) {
> + pr_err("Failed to power on device: %d\n", ret);
> + goto pwron;
> + }
> + /* turn on VDD_IO */
> + ret = regulator_enable(mt9p031->reg_2v8);
> + if (ret) {
> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> + goto e2v8en;
> + }
The regulator is enabled by mt9p032_power_on(), there's no need to enable it a
second time herE.
> +
> + if (pdata->reset)
> + pdata->reset(sd, 1);
> +
> + msleep(50);
> +
> + if (pdata->reset)
> + pdata->reset(sd, 0);
> +
> + msleep(50);
Why do you need to reset the chip here ? If resetting it is required after
powering it up, you should do it every time the power is turned on, not just
here.
> + ret = mt9p031_video_probe(client);
You have a set_xclk callback to board code, so I assume the chip can be driven
by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3 ISP from board
code, you need to get hold of the OMAP3 ISP device pointer. Your next patch
exports omap3isp_device, but I'm not sure that's the way to go. One option is
struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
but that requires the subdev to be registered before the function can be
called. In that case you would need to move the probe code to the registered
subdev internal function.
A clean solution is needed in the long run, preferably not involving board
code at all. It would be nice if the OMAP3 ISP driver could export XCLKA/XCLKB
as generic clock objects.
> + if (ret)
> + goto evprobe;
> +
> + mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
> + if (ret)
> + goto evprobe;
> +
> + mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> + return ret;
> +
> +evprobe:
> + regulator_disable(mt9p031->reg_2v8);
> +e2v8en:
> + mt9p031_power_off(mt9p031);
> +pwron:
> + regulator_put(mt9p031->reg_2v8);
> +e2v8:
> + regulator_put(mt9p031->reg_1v8);
> +e1v8:
> + kfree(mt9p031);
> + return ret;
> +}
[snip]
> +MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
You mention Micron in the Kconfig file, maybe you could replace it with
Aptina.
> +MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
> +MODULE_LICENSE("GPL v2");
[snip]
> diff --git a/include/media/v4l2-chip-ident.h
> b/include/media/v4l2-chip-ident.h index b3edb67..97919f2 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -297,6 +297,7 @@ enum {
> V4L2_IDENT_MT9T112 = 45022,
> V4L2_IDENT_MT9V111 = 45031,
> V4L2_IDENT_MT9V112 = 45032,
> + V4L2_IDENT_MT9P031 = 45033,
This constant will become unused, you can remove it as well.
>
> /* HV7131R CMOS sensor: just ident 46000 */
> V4L2_IDENT_HV7131R = 46000,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 11:33 ` Laurent Pinchart
0 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-17 11:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Javier,
Thanks for the patch.
On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
> It has been tested in beagleboard xM, using LI-5M03 module.
>
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
[snip]
> diff --git a/drivers/media/video/mt9p031.c b/drivers/media/video/mt9p031.c
> new file mode 100644
> index 0000000..850cfec
> --- /dev/null
> +++ b/drivers/media/video/mt9p031.c
> @@ -0,0 +1,773 @@
> +/*
> + * Driver for MT9P031 CMOS Image Sensor from Aptina
> + *
> + * Copyright (C) 2011, Javier Martin <javier.martin@vista-silicon.com>
> + *
> + * Copyright (C) 2011, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> + *
> + * Based on the MT9V032 driver and Bastian Hecht's code.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/log2.h>
> +#include <linux/pm.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <media/v4l2-subdev.h>
> +#include <linux/videodev2.h>
> +
> +#include <media/mt9p031.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/v4l2-subdev.h>
> +#include <media/v4l2-device.h>
> +
> +/* mt9p031 selected register addresses */
> +#define MT9P031_CHIP_VERSION 0x00
> +#define MT9P031_CHIP_VERSION_VALUE 0x1801
> +#define MT9P031_ROW_START 0x01
> +#define MT9P031_ROW_START_SKIP 54
> +#define MT9P031_COLUMN_START 0x02
> +#define MT9P031_COLUMN_START_SKIP 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_RESET 0x0d
> +#define MT9P031_RESET_ENABLE 1
> +#define MT9P031_RESET_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_MAX_HEIGHT 1944
> +#define MT9P031_MAX_WIDTH 2592
> +#define MT9P031_MIN_HEIGHT 2
> +#define MT9P031_MIN_WIDTH 18
> +
> +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;
> + int model; /* V4L2_IDENT_MT9P031* codes from v4l2-chip-ident.h */
model is assigned by never read, you can remove it.
> + u16 xskip;
> + u16 yskip;
> + struct regulator *reg_1v8, *reg_2v8;
Please split this on two lines.
You never enable the 1.8V regulator, is that intentional ?
> +};
[snip]
> +static int reg_set(struct i2c_client *client, const u8 reg,
> + const u16 data)
> +{
> + int ret;
> +
> + ret = reg_read(client, reg);
> + if (ret < 0)
> + return ret;
> + return reg_write(client, reg, ret | data);
> +}
> +
> +static int reg_clear(struct i2c_client *client, const u8 reg,
> + const u16 data)
> +{
> + int ret;
> +
> + ret = reg_read(client, reg);
> + if (ret < 0)
> + return ret;
> + return reg_write(client, reg, ret & ~data);
> +}
I still think you should use a shadow copy of the register in the mt9p031
structure instead of reading the value back from the hardware. As these two
functions are only used to handle the output control register, you could
create an mt9p031_set_output_control(struct mt9p031 *mt9p031, u16 clear, u16
set) function instead.
> +static int mt9p031_idle(struct i2c_client *client)
This function resets the chip, what about calling it mt9p031_reset() instead ?
> +{
> + int ret;
> +
> + /* Disable chip output, synchronous option update */
> + ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_ENABLE);
> + if (ret < 0)
> + goto err;
You can return -EIO directly.
> + ret = reg_write(client, MT9P031_RESET, MT9P031_RESET_DISABLE);
> + if (ret < 0)
> + goto err;
Here too.
> + ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_CEN);
> + if (ret < 0)
> + goto err;
And here too.
> + return 0;
> +err:
> + return -EIO;
> +}
[snip]
> +
> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> +{
> + int ret;
> +
> + if (mt9p031->pdata->set_xclk)
> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> + /* turn on VDD_IO */
> + ret = regulator_enable(mt9p031->reg_2v8);
> + if (ret) {
> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> + return -1;
> + }
I would enable the regulator first. As a general rule, chips should be powered
up before their I/Os are actively driven.
You need to restore registers here, otherwise all controls set by the user
will not be applied to the device.
> + return 0;
> +}
[snip]
> +static u16 mt9p031_skip_for_crop(s32 source, s32 *target, s32 max_skip)
> +{
> + unsigned int skip;
> +
> + if (source - source / 4 < *target) {
> + *target = source;
> + return 1;
> + }
> +
> + skip = DIV_ROUND_CLOSEST(source, *target);
> + if (skip > max_skip)
> + skip = max_skip;
> + *target = 2 * DIV_ROUND_UP(source, 2 * skip);
> +
> + return skip;
> +}
> +
> +static int mt9p031_set_params(struct i2c_client *client,
> + struct v4l2_rect *rect, u16 xskip, u16 yskip)
set_params should apply the parameters, not change them. They should have
already been validated by the callers.
> +{
> + struct mt9p031 *mt9p031 = to_mt9p031(client);
> + int ret;
> + u16 xbin, ybin;
> + const u16 hblank = MT9P031_H_BLANKING_VALUE,
> + vblank = MT9P031_V_BLANKING_VALUE;
> +
> + /*
> + * TODO: Attention! When implementing horizontal flipping, adjust
> + * alignment according to R2 "Column Start" description in the datasheet
> + */
> + if (xskip & 1) {
> + xbin = 1;
> + rect->left &= ~3;
> + } else if (xskip & 2) {
> + xbin = 2;
> + rect->left &= ~7;
> + } else {
> + xbin = 4;
> + rect->left &= ~15;
> + }
> +
> + ybin = min(yskip, (u16)4);
> +
> + rect->top &= ~1;
> +
> + /* Disable register update, reconfigure atomically */
> + ret = reg_set(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_SYN);
> + if (ret < 0)
> + goto err;
> +
> + dev_dbg(&client->dev, "skip %u:%u, rect %ux%u@%u:%u\n",
> + xskip, yskip, rect->width, rect->height, rect->left, rect->top);
> +
> + /* Blanking and start values - default... */
> + ret = reg_write(client, MT9P031_H_BLANKING, hblank);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_V_BLANKING, vblank);
> + if (ret < 0)
> + goto err;
> +
> + if (yskip != mt9p031->yskip || xskip != mt9p031->xskip) {
> + /* Binning, skipping */
> + ret = reg_write(client, MT9P031_COLUMN_ADDRESS_MODE,
> + ((xbin - 1) << 4) | (xskip - 1));
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_ROW_ADDRESS_MODE,
> + ((ybin - 1) << 4) | (yskip - 1));
> + if (ret < 0)
> + goto err;
> + }
> + dev_dbg(&client->dev, "new physical left %u, top %u\n",
> + rect->left, rect->top);
> +
> + ret = reg_write(client, MT9P031_COLUMN_START,
> + rect->left + MT9P031_COLUMN_START_SKIP);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_ROW_START,
> + rect->top + MT9P031_ROW_START_SKIP);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_WINDOW_WIDTH,
> + rect->width - 1);
> + if (ret < 0)
> + goto err;
> + ret = reg_write(client, MT9P031_WINDOW_HEIGHT,
> + rect->height - 1);
> + if (ret < 0)
> + goto err;
> +
> + /* Re-enable register update, commit all changes */
> + ret = reg_clear(client, MT9P031_OUTPUT_CONTROL,
> + MT9P031_OUTPUT_CONTROL_SYN);
> + if (ret < 0)
> + goto err;
> +
> + mt9p031->xskip = xskip;
> + mt9p031->yskip = yskip;
> + return ret;
> +err:
> + return -1;
> +}
> +
> +static int mt9p031_set_crop(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_crop *crop)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> + struct v4l2_mbus_framefmt *f;
> + struct v4l2_rect *c;
> + struct v4l2_rect rect;
> + u16 xskip, yskip;
> + s32 width, height;
> + int ret;
> +
> + pr_info("%s(%ux%u@%u:%u : %u)\n", __func__,
> + crop->rect.width, crop->rect.height,
> + crop->rect.left, crop->rect.top, crop->which);
> +
> + /*
> + * Clamp the crop rectangle boundaries and align them to a multiple of 2
> + * pixels.
> + */
> + rect.width = ALIGN(clamp(crop->rect.width,
> + MT9P031_MIN_WIDTH, MT9P031_MAX_WIDTH), 2);
> + rect.height = ALIGN(clamp(crop->rect.height,
> + MT9P031_MIN_HEIGHT, MT9P031_MAX_HEIGHT), 2);
> + rect.left = ALIGN(clamp(crop->rect.left,
> + 0, MT9P031_MAX_WIDTH - rect.width), 2);
> + rect.top = ALIGN(clamp(crop->rect.top,
> + 0, MT9P031_MAX_HEIGHT - rect.height), 2);
> +
> + c = mt9p031_get_pad_crop(mt9p031, fh, crop->pad, crop->which);
> +
> + if (rect.width != c->width || rect.height != c->height) {
> + /*
> + * Reset the output image size if the crop rectangle size has
> + * been modified.
> + */
> + f = mt9p031_get_pad_format(mt9p031, fh, crop->pad,
> + crop->which);
> + width = f->width;
> + height = f->height;
> +
> + xskip = mt9p031_skip_for_crop(rect.width, &width, 7);
> + yskip = mt9p031_skip_for_crop(rect.height, &height, 8);
> + } else {
> + xskip = mt9p031->xskip;
> + yskip = mt9p031->yskip;
> + f = NULL;
> + }
> +
> + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (f) {
> + f->width = width;
> + f->height = height;
> + }
> +
> + *c = rect;
> + crop->rect = rect;
> +
> + return 0;
> +}
[snip]
> +static u16 mt9p031_skip_for_scale(s32 *source, s32 target,
> + s32 max_skip, s32 max)
> +{
> + unsigned int skip;
> +
> + if (*source - *source / 4 < target) {
> + *source = target;
> + return 1;
> + }
> +
> + skip = min(max, *source + target / 2) / target;
> + if (skip > max_skip)
> + skip = max_skip;
> + *source = target * skip;
> +
> + return skip;
> +}
[snip]
> +static int mt9p031_set_format(struct v4l2_subdev *sd,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(sd);
> + struct v4l2_subdev_format sdf = *fmt;
> + struct v4l2_mbus_framefmt *f, *format = &sdf.format;
> + struct v4l2_rect *c, rect;
> + struct mt9p031 *mt9p031 = container_of(sd, struct mt9p031, subdev);
> + u16 xskip, yskip;
> + int ret;
> +
> + ret = mt9p031_fmt_validate(sd, &sdf);
> + if (ret < 0)
> + return ret;
> +
> + f = mt9p031_get_pad_format(mt9p031, fh, fmt->pad, fmt->which);
> +
> + if (f->width != format->width || f->height != format->height) {
If width and height are identical to the current value, you can return
immediately.
> + c = mt9p031_get_pad_crop(mt9p031, fh, fmt->pad, fmt->which);
> +
> + rect.width = c->width;
> + rect.height = c->height;
> +
> + xskip = mt9p031_skip_for_scale(&rect.width, format->width, 7,
> + MT9P031_MAX_WIDTH);
> + if (rect.width + c->left > MT9P031_MAX_WIDTH)
> + rect.left = (MT9P031_MAX_WIDTH - rect.width) / 2;
> + else
> + rect.left = c->left;
> + yskip = mt9p031_skip_for_scale(&rect.height, format->height, 8,
> + MT9P031_MAX_HEIGHT);
> + if (rect.height + c->top > MT9P031_MAX_HEIGHT)
> + rect.top = (MT9P031_MAX_HEIGHT - rect.height) / 2;
> + else
> + rect.top = c->top;
> + } else {
> + xskip = mt9p031->xskip;
> + yskip = mt9p031->yskip;
> + c = NULL;
> + }
> +
> + pr_info("%s(%ux%u : %u)\n", __func__,
> + format->width, format->height, fmt->which);
> +
> + if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + /* mt9p031_set_params() doesn't change width and height */
> + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (c)
> + *c = rect;
> +
> + *f = *format;
> + fmt->format = *format;
> +
> + return 0;
> +}
[snip]
> +static int mt9p031_registered(struct v4l2_subdev *sd)
> +{
> + struct mt9p031 *mt9p031;
> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> +
> + mt9p031_power_off(mt9p031);
What's that for ?
> + return 0;
> +}
[snip]
> +static int mt9p031_close(struct v4l2_subdev *sd, struct v4l2_subdev_fh
> *fh) +{
> + struct mt9p031 *mt9p031;
> +
> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> +
> + mt9p031_power_off(mt9p031);
You need to reference count power handling, otherwise closing the subdev file
handle will turn the power off, even if the sensor is part of a pipeline
actively streaming video. Have a look at the mt9v032 driver to see how that's
done.
> + return 0;
> +}
[snip]
> +static int mt9p031_probe(struct i2c_client *client,
> + const struct i2c_device_id *did)
> +{
> + struct mt9p031 *mt9p031;
> + struct v4l2_subdev *sd = i2c_get_clientdata(client);
> + struct mt9p031_platform_data *pdata = client->dev.platform_data;
> + struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> + int ret;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
> + dev_warn(&adapter->dev,
> + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> + return -EIO;
> + }
> +
> + mt9p031 = kzalloc(sizeof(struct mt9p031), GFP_KERNEL);
> + if (!mt9p031)
> + return -ENOMEM;
> +
> + v4l2_i2c_subdev_init(&mt9p031->subdev, client, &mt9p031_subdev_ops);
> + mt9p031->subdev.internal_ops = &mt9p031_subdev_internal_ops;
> +
> + mt9p031->pdata = pdata;
> + mt9p031->rect.left = 0/*MT9P031_COLUMN_SKIP*/;
> + mt9p031->rect.top = 0/*MT9P031_ROW_SKIP*/;
No commented out code please.
> + mt9p031->rect.width = MT9P031_MAX_WIDTH;
> + mt9p031->rect.height = MT9P031_MAX_HEIGHT;
> +
> + mt9p031->format.code = V4L2_MBUS_FMT_SGRBG12_1X12;
> +
> + mt9p031->format.width = MT9P031_MAX_WIDTH;
> + mt9p031->format.height = MT9P031_MAX_HEIGHT;
> + mt9p031->format.field = V4L2_FIELD_NONE;
> + mt9p031->format.colorspace = V4L2_COLORSPACE_SRGB;
> +
> + /* mt9p031_idle() will reset the chip to default. */
There's no mt9p032_idle() call here.
> +
> + mt9p031->xskip = 1;
> + mt9p031->yskip = 1;
> +
> + mt9p031->reg_1v8 = regulator_get(NULL, "cam_1v8");
> + if (IS_ERR(mt9p031->reg_1v8)) {
> + ret = PTR_ERR(mt9p031->reg_1v8);
> + pr_err("Failed 1.8v regulator: %d\n", ret);
> + goto e1v8;
> + }
> +
> + mt9p031->reg_2v8 = regulator_get(NULL, "cam_2v8");
> + if (IS_ERR(mt9p031->reg_2v8)) {
> + ret = PTR_ERR(mt9p031->reg_2v8);
> + pr_err("Failed 2.8v regulator: %d\n", ret);
> + goto e2v8;
> + }
> +
> + ret = mt9p031_power_on(mt9p031);
> + if (ret) {
> + pr_err("Failed to power on device: %d\n", ret);
> + goto pwron;
> + }
> + /* turn on VDD_IO */
> + ret = regulator_enable(mt9p031->reg_2v8);
> + if (ret) {
> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> + goto e2v8en;
> + }
The regulator is enabled by mt9p032_power_on(), there's no need to enable it a
second time herE.
> +
> + if (pdata->reset)
> + pdata->reset(sd, 1);
> +
> + msleep(50);
> +
> + if (pdata->reset)
> + pdata->reset(sd, 0);
> +
> + msleep(50);
Why do you need to reset the chip here ? If resetting it is required after
powering it up, you should do it every time the power is turned on, not just
here.
> + ret = mt9p031_video_probe(client);
You have a set_xclk callback to board code, so I assume the chip can be driven
by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3 ISP from board
code, you need to get hold of the OMAP3 ISP device pointer. Your next patch
exports omap3isp_device, but I'm not sure that's the way to go. One option is
struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
but that requires the subdev to be registered before the function can be
called. In that case you would need to move the probe code to the registered
subdev internal function.
A clean solution is needed in the long run, preferably not involving board
code at all. It would be nice if the OMAP3 ISP driver could export XCLKA/XCLKB
as generic clock objects.
> + if (ret)
> + goto evprobe;
> +
> + mt9p031->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_init(&mt9p031->subdev.entity, 1, &mt9p031->pad, 0);
> + if (ret)
> + goto evprobe;
> +
> + mt9p031->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> + return ret;
> +
> +evprobe:
> + regulator_disable(mt9p031->reg_2v8);
> +e2v8en:
> + mt9p031_power_off(mt9p031);
> +pwron:
> + regulator_put(mt9p031->reg_2v8);
> +e2v8:
> + regulator_put(mt9p031->reg_1v8);
> +e1v8:
> + kfree(mt9p031);
> + return ret;
> +}
[snip]
> +MODULE_DESCRIPTION("Aptina MT9P031 Camera driver");
You mention Micron in the Kconfig file, maybe you could replace it with
Aptina.
> +MODULE_AUTHOR("Bastian Hecht <hechtb@gmail.com>");
> +MODULE_LICENSE("GPL v2");
[snip]
> diff --git a/include/media/v4l2-chip-ident.h
> b/include/media/v4l2-chip-ident.h index b3edb67..97919f2 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -297,6 +297,7 @@ enum {
> V4L2_IDENT_MT9T112 = 45022,
> V4L2_IDENT_MT9V111 = 45031,
> V4L2_IDENT_MT9V112 = 45032,
> + V4L2_IDENT_MT9P031 = 45033,
This constant will become unused, you can remove it as well.
>
> /* HV7131R CMOS sensor: just ident 46000 */
> V4L2_IDENT_HV7131R = 46000,
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 11:33 ` Laurent Pinchart
@ 2011-05-17 11:47 ` Guennadi Liakhovetski
-1 siblings, 0 replies; 26+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-17 11:47 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Javier Martin, linux-media, carlighting, beagleboard, linux-arm-kernel
Hi Laurent
Thanks for your review! Javier, if you like, you can wait a couple of days
until I find some time to review the driver, or you can submit a version,
addressing Laurent's points, but be prepared to have to do another one;)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 11:47 ` Guennadi Liakhovetski
0 siblings, 0 replies; 26+ messages in thread
From: Guennadi Liakhovetski @ 2011-05-17 11:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Laurent
Thanks for your review! Javier, if you like, you can wait a couple of days
until I find some time to review the driver, or you can submit a version,
addressing Laurent's points, but be prepared to have to do another one;)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 11:47 ` Guennadi Liakhovetski
@ 2011-05-17 11:59 ` javier Martin
-1 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-17 11:59 UTC (permalink / raw)
To: Guennadi Liakhovetski
Cc: Laurent Pinchart, linux-media, carlighting, beagleboard,
linux-arm-kernel
On 17 May 2011 13:47, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Laurent
>
> Thanks for your review! Javier, if you like, you can wait a couple of days
> until I find some time to review the driver, or you can submit a version,
> addressing Laurent's points, but be prepared to have to do another one;)
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
OK, I think I'll wait to have Guennadi's review too.
Thank you both.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 11:59 ` javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-17 11:59 UTC (permalink / raw)
To: linux-arm-kernel
On 17 May 2011 13:47, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> Hi Laurent
>
> Thanks for your review! Javier, if you like, you can wait a couple of days
> until I find some time to review the driver, or you can submit a version,
> addressing Laurent's points, but be prepared to have to do another one;)
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
>
OK, I think I'll wait to have Guennadi's review too.
Thank you both.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 11:59 ` javier Martin
@ 2011-05-17 17:14 ` Ivan Nazarenko
-1 siblings, 0 replies; 26+ messages in thread
From: Ivan Nazarenko @ 2011-05-17 17:14 UTC (permalink / raw)
To: javier Martin, linux-media, beagleboard; +Cc: linux-arm-kernel
Javier,
I have been using the aptina patch (https://github.com/Aptina/BeagleBoard-xM) on beagleboard while waiting linux-media solve this mt9p031 issue. Now that you have something working, I would like to try it - but I would like to know what is the clock rate you actually drove the sensor.
Reviewing your path, I suppose it is 54MHz, so you would be achieving some 10 full 5MPix frames/s from the sensor. Is that correct? (the aptina patch delivers less than 4 fps).
Regards,
Ivan
On Tuesday, May 17, 2011 08:59:04 javier Martin wrote:
> On 17 May 2011 13:47, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Laurent
> >
> > Thanks for your review! Javier, if you like, you can wait a couple of days
> > until I find some time to review the driver, or you can submit a version,
> > addressing Laurent's points, but be prepared to have to do another one;)
> >
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> >
>
> OK, I think I'll wait to have Guennadi's review too.
> Thank you both.
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 17:14 ` Ivan Nazarenko
0 siblings, 0 replies; 26+ messages in thread
From: Ivan Nazarenko @ 2011-05-17 17:14 UTC (permalink / raw)
To: linux-arm-kernel
Javier,
I have been using the aptina patch (https://github.com/Aptina/BeagleBoard-xM) on beagleboard while waiting linux-media solve this mt9p031 issue. Now that you have something working, I would like to try it - but I would like to know what is the clock rate you actually drove the sensor.
Reviewing your path, I suppose it is 54MHz, so you would be achieving some 10 full 5MPix frames/s from the sensor. Is that correct? (the aptina patch delivers less than 4 fps).
Regards,
Ivan
On Tuesday, May 17, 2011 08:59:04 javier Martin wrote:
> On 17 May 2011 13:47, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > Hi Laurent
> >
> > Thanks for your review! Javier, if you like, you can wait a couple of days
> > until I find some time to review the driver, or you can submit a version,
> > addressing Laurent's points, but be prepared to have to do another one;)
> >
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> >
>
> OK, I think I'll wait to have Guennadi's review too.
> Thank you both.
>
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
2011-05-17 9:28 ` Javier Martin
@ 2011-05-17 23:08 ` Russell King - ARM Linux
-1 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2011-05-17 23:08 UTC (permalink / raw)
To: Javier Martin
Cc: linux-media, beagleboard, carlighting, g.liakhovetski,
linux-arm-kernel, laurent.pinchart
On Tue, May 17, 2011 at 11:28:48AM +0200, Javier Martin wrote:
> +#include "devices.h"
> +#include "../../../drivers/media/video/omap3isp/isp.h"
> +#include "../../../drivers/media/video/omap3isp/ispreg.h"
This suggests that there's something very wrong with what's going on;
it suggests that you're trying to access driver internals which should
be handled via some better means. And it looks like it's this:
> @@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
> return;
> }
>
> +extern struct platform_device omap3isp_device;
> +
> +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
> +{
> + struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
> + int ret;
> +
> + ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
> + return 0;
> +}
That really needs fixing in a different way.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
@ 2011-05-17 23:08 ` Russell King - ARM Linux
0 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2011-05-17 23:08 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, May 17, 2011 at 11:28:48AM +0200, Javier Martin wrote:
> +#include "devices.h"
> +#include "../../../drivers/media/video/omap3isp/isp.h"
> +#include "../../../drivers/media/video/omap3isp/ispreg.h"
This suggests that there's something very wrong with what's going on;
it suggests that you're trying to access driver internals which should
be handled via some better means. And it looks like it's this:
> @@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
> return;
> }
>
> +extern struct platform_device omap3isp_device;
> +
> +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
> +{
> + struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
> + int ret;
> +
> + ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
> + return 0;
> +}
That really needs fixing in a different way.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 11:33 ` Laurent Pinchart
@ 2011-05-17 23:18 ` Russell King - ARM Linux
-1 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2011-05-17 23:18 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Javier Martin, beagleboard, carlighting, g.liakhovetski,
linux-arm-kernel, linux-media
On Tue, May 17, 2011 at 01:33:52PM +0200, Laurent Pinchart wrote:
> Hi Javier,
>
> Thanks for the patch.
Sorry, but this laziness is getting beyond a joke... And the fact that
apparantly no one is picking up on it other than me is also a joke.
> > +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> > +{
> > + int ret;
> > +
> > + if (mt9p031->pdata->set_xclk)
> > + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> > + /* turn on VDD_IO */
> > + ret = regulator_enable(mt9p031->reg_2v8);
> > + if (ret) {
> > + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> > + return -1;
And why all these 'return -1's? My guess is that this is plain laziness
on the authors part.
> > +static int mt9p031_set_params(struct i2c_client *client,
> > + struct v4l2_rect *rect, u16 xskip, u16 yskip)
>
> set_params should apply the parameters, not change them. They should have
> already been validated by the callers.
>
> > +{
...
> > +err:
> > + return -1;
And again...
> > +}
> > +
> > +static int mt9p031_set_crop(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_crop *crop)
> > +{
...
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> > + if (ret < 0)
> > + return ret;
So this propagates the lazy 'return -1' all the way back to userspace.
This is utter crap - really it is, and I'm getting sick and tired of
telling people that they should not use 'return -1'. It's down right
lazy and sloppy programming.
I wish people would stop doing it. I wish people would review their own
stuff for this _before_ posting it onto a mailing list, so I don't have
to keep complaining about it. And I wish people reviewing drivers would
also look for this as well and complain about it.
'return -1' is generally a big fat warning sign that the author is doing
something wrong, and should _always_ be investigated and complained about.
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-17 23:18 ` Russell King - ARM Linux
0 siblings, 0 replies; 26+ messages in thread
From: Russell King - ARM Linux @ 2011-05-17 23:18 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, May 17, 2011 at 01:33:52PM +0200, Laurent Pinchart wrote:
> Hi Javier,
>
> Thanks for the patch.
Sorry, but this laziness is getting beyond a joke... And the fact that
apparantly no one is picking up on it other than me is also a joke.
> > +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> > +{
> > + int ret;
> > +
> > + if (mt9p031->pdata->set_xclk)
> > + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> > + /* turn on VDD_IO */
> > + ret = regulator_enable(mt9p031->reg_2v8);
> > + if (ret) {
> > + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> > + return -1;
And why all these 'return -1's? My guess is that this is plain laziness
on the authors part.
> > +static int mt9p031_set_params(struct i2c_client *client,
> > + struct v4l2_rect *rect, u16 xskip, u16 yskip)
>
> set_params should apply the parameters, not change them. They should have
> already been validated by the callers.
>
> > +{
...
> > +err:
> > + return -1;
And again...
> > +}
> > +
> > +static int mt9p031_set_crop(struct v4l2_subdev *sd,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_crop *crop)
> > +{
...
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + ret = mt9p031_set_params(client, &rect, xskip, yskip);
> > + if (ret < 0)
> > + return ret;
So this propagates the lazy 'return -1' all the way back to userspace.
This is utter crap - really it is, and I'm getting sick and tired of
telling people that they should not use 'return -1'. It's down right
lazy and sloppy programming.
I wish people would stop doing it. I wish people would review their own
stuff for this _before_ posting it onto a mailing list, so I don't have
to keep complaining about it. And I wish people reviewing drivers would
also look for this as well and complain about it.
'return -1' is generally a big fat warning sign that the author is doing
something wrong, and should _always_ be investigated and complained about.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
2011-05-17 23:08 ` Russell King - ARM Linux
@ 2011-05-18 7:50 ` Laurent Pinchart
-1 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-18 7:50 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Javier Martin, linux-media, beagleboard, carlighting,
g.liakhovetski, linux-arm-kernel
Hi Russell,
On Wednesday 18 May 2011 01:08:21 Russell King - ARM Linux wrote:
> On Tue, May 17, 2011 at 11:28:48AM +0200, Javier Martin wrote:
> > +#include "devices.h"
> > +#include "../../../drivers/media/video/omap3isp/isp.h"
> > +#include "../../../drivers/media/video/omap3isp/ispreg.h"
>
> This suggests that there's something very wrong with what's going on;
> it suggests that you're trying to access driver internals which should
> be handled via some better means. And it looks like it's this:
> > @@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
> >
> > return;
> >
> > }
> >
> > +extern struct platform_device omap3isp_device;
> > +
> > +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
> > +{
> > + struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
> > + int ret;
> > +
> > + ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
> > + return 0;
> > +}
>
> That really needs fixing in a different way.
I plan to look into whether I can expose the OMAP3 ISP clocks through the
Linux clock framework.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module).
@ 2011-05-18 7:50 ` Laurent Pinchart
0 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-18 7:50 UTC (permalink / raw)
To: linux-arm-kernel
Hi Russell,
On Wednesday 18 May 2011 01:08:21 Russell King - ARM Linux wrote:
> On Tue, May 17, 2011 at 11:28:48AM +0200, Javier Martin wrote:
> > +#include "devices.h"
> > +#include "../../../drivers/media/video/omap3isp/isp.h"
> > +#include "../../../drivers/media/video/omap3isp/ispreg.h"
>
> This suggests that there's something very wrong with what's going on;
> it suggests that you're trying to access driver internals which should
> be handled via some better means. And it looks like it's this:
> > @@ -654,6 +715,62 @@ static void __init beagle_opp_init(void)
> >
> > return;
> >
> > }
> >
> > +extern struct platform_device omap3isp_device;
> > +
> > +static int beagle_cam_set_xclk(struct v4l2_subdev *subdev, int hz)
> > +{
> > + struct isp_device *isp = platform_get_drvdata(&omap3isp_device);
> > + int ret;
> > +
> > + ret = isp->platform_cb.set_xclk(isp, hz, MT9P031_XCLK);
> > + return 0;
> > +}
>
> That really needs fixing in a different way.
I plan to look into whether I can expose the OMAP3 ISP clocks through the
Linux clock framework.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 11:33 ` Laurent Pinchart
@ 2011-05-18 9:10 ` javier Martin
-1 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-18 9:10 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, g.liakhovetski, carlighting, beagleboard, linux-arm-kernel
Hi Laurent,
I've already fixed almost every issue you pointed out.
However, I still have got some doubts that I hope you can clarify.
On 17 May 2011 13:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> Thanks for the patch.
>
> On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
>> It has been tested in beagleboard xM, using LI-5M03 module.
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>>
>> +
>> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
>> +{
>> + int ret;
>> +
>> + if (mt9p031->pdata->set_xclk)
>> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
>> + /* turn on VDD_IO */
>> + ret = regulator_enable(mt9p031->reg_2v8);
>> + if (ret) {
>> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
>> + return -1;
>> + }
>
>I would enable the regulator first. As a general rule, chips should be powered
>up before their I/Os are actively driven.
>
>You need to restore registers here, otherwise all controls set by the user
>will not be applied to the device.
It's my mistake. This driver uses two regulators: 1,8 and 2,8 V
respectively. 2,8V regulator powers analog part and I/O whereas 1,8V
one powers the core. What I failed to do was keeping 1,8V regulator
always powered on, so that register configuration was not lost, and
power 2,8V regulator on and off as needed since it does not affect
register values. However, I messed it all up.
Ideally I would have to power 1,8V regulator on and off too. However,
as you wisely pointed out, registers should be restored in that case.
How am I supposed to keep track of register values? Are there any
helper functions I can use for that purpose or must I create a custom
register cache? Do you know any driver that uses this technique?
> [snip]
>> +static int mt9p031_set_params(struct i2c_client *client,
>> + struct v4l2_rect *rect, u16 xskip, u16 yskip)
>
> set_params should apply the parameters, not change them. They should have
> already been validated by the callers.
"mt9p031_set_params()" function is used by "mt9p031_set_crop()" and
"mt9p031_set_format()", as you have correctly stated, these functions
shouldn' apply parameters but only change them.
I've checked mt9v032 driver and it is as you said. The question is,
where should these parameters get applied then?
>> +static int mt9p031_registered(struct v4l2_subdev *sd)
>> +{
>> + struct mt9p031 *mt9p031;
>> + mt9p031 = container_of(sd, struct mt9p031, subdev);
>> +
>> + mt9p031_power_off(mt9p031);
>
> What's that for ?
>
>> + return 0;
>> +}
Since "mt9p031_power_off()" and "mt9p031_power_on()" functions
disable/enable the 2,8V regulator which powers I/O, it must be powered
on during probe and after registering, it can be safely powered off.
>
> You have a set_xclk callback to board code, so I assume the chip can be driven
> by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3 ISP from board
> code, you need to get hold of the OMAP3 ISP device pointer. Your next patch
> exports omap3isp_device, but I'm not sure that's the way to go. One option is
>
> struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
>
> but that requires the subdev to be registered before the function can be
> called. In that case you would need to move the probe code to the registered
> subdev internal function.
>
Yes, I tried using that function but it didn't work because subdev
hadn't been registeret yet.
> A clean solution is needed in the long run, preferably not involving board
> code at all. It would be nice if the OMAP3 ISP driver could export XCLKA/XCLKB
> as generic clock objects.
So, what should I do in order to submit the driver to mainline?
Do you want me to move the probe code to registered callback?
Thank you.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-18 9:10 ` javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-18 9:10 UTC (permalink / raw)
To: linux-arm-kernel
Hi Laurent,
I've already fixed almost every issue you pointed out.
However, I still have got some doubts that I hope you can clarify.
On 17 May 2011 13:33, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Javier,
>
> Thanks for the patch.
>
> On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
>> It has been tested in beagleboard xM, using LI-5M03 module.
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>>
>> +
>> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
>> +{
>> + int ret;
>> +
>> + if (mt9p031->pdata->set_xclk)
>> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
>> + /* turn on VDD_IO */
>> + ret = regulator_enable(mt9p031->reg_2v8);
>> + if (ret) {
>> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
>> + return -1;
>> + }
>
>I would enable the regulator first. As a general rule, chips should be powered
>up before their I/Os are actively driven.
>
>You need to restore registers here, otherwise all controls set by the user
>will not be applied to the device.
It's my mistake. This driver uses two regulators: 1,8 and 2,8 V
respectively. 2,8V regulator powers analog part and I/O whereas 1,8V
one powers the core. What I failed to do was keeping 1,8V regulator
always powered on, so that register configuration was not lost, and
power 2,8V regulator on and off as needed since it does not affect
register values. However, I messed it all up.
Ideally I would have to power 1,8V regulator on and off too. However,
as you wisely pointed out, registers should be restored in that case.
How am I supposed to keep track of register values? Are there any
helper functions I can use for that purpose or must I create a custom
register cache? Do you know any driver that uses this technique?
> [snip]
>> +static int mt9p031_set_params(struct i2c_client *client,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? struct v4l2_rect *rect, u16 xskip, u16 yskip)
>
> set_params should apply the parameters, not change them. They should have
> already been validated by the callers.
"mt9p031_set_params()" function is used by "mt9p031_set_crop()" and
"mt9p031_set_format()", as you have correctly stated, these functions
shouldn' apply parameters but only change them.
I've checked mt9v032 driver and it is as you said. The question is,
where should these parameters get applied then?
>> +static int mt9p031_registered(struct v4l2_subdev *sd)
>> +{
>> + ? ? struct mt9p031 *mt9p031;
>> + ? ? mt9p031 = container_of(sd, struct mt9p031, subdev);
>> +
>> + ? ? mt9p031_power_off(mt9p031);
>
> What's that for ?
>
>> + ? ? return 0;
>> +}
Since "mt9p031_power_off()" and "mt9p031_power_on()" functions
disable/enable the 2,8V regulator which powers I/O, it must be powered
on during probe and after registering, it can be safely powered off.
>
> You have a set_xclk callback to board code, so I assume the chip can be driven
> by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3 ISP from board
> code, you need to get hold of the OMAP3 ISP device pointer. Your next patch
> exports omap3isp_device, but I'm not sure that's the way to go. One option is
>
> struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
>
> but that requires the subdev to be registered before the function can be
> called. In that case you would need to move the probe code to the registered
> subdev internal function.
>
Yes, I tried using that function but it didn't work because subdev
hadn't been registeret yet.
> A clean solution is needed in the long run, preferably not involving board
> code at all. It would be nice if the OMAP3 ISP driver could export XCLKA/XCLKB
> as generic clock objects.
So, what should I do in order to submit the driver to mainline?
Do you want me to move the probe code to registered callback?
Thank you.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-17 17:14 ` Ivan Nazarenko
@ 2011-05-18 10:34 ` javier Martin
-1 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-18 10:34 UTC (permalink / raw)
To: Ivan Nazarenko; +Cc: linux-media, beagleboard, linux-arm-kernel
Hi Ivan,
On 17 May 2011 19:14, Ivan Nazarenko <ivan.nazarenko@gmail.com> wrote:
> Javier,
>
> I have been using the aptina patch (https://github.com/Aptina/BeagleBoard-xM) on beagleboard while waiting linux-media solve this mt9p031 issue. Now that you have something working, I would like to try it - but I would like to know what is the clock rate you actually drove the sensor.
>
> Reviewing your path, I suppose it is 54MHz, so you would be achieving some 10 full 5MPix frames/s from the sensor. Is that correct? (the aptina patch delivers less than 4 fps).
>
Yes, you are right. Whereas clock rate is set to 54MHz, with my
oscilloscope I have measured 57 MHz.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-18 10:34 ` javier Martin
0 siblings, 0 replies; 26+ messages in thread
From: javier Martin @ 2011-05-18 10:34 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ivan,
On 17 May 2011 19:14, Ivan Nazarenko <ivan.nazarenko@gmail.com> wrote:
> Javier,
>
> I have been using the aptina patch (https://github.com/Aptina/BeagleBoard-xM) on beagleboard while waiting linux-media solve this mt9p031 issue. Now that you have something working, I would like to try it - but I would like to know what is the clock rate you actually drove the sensor.
>
> Reviewing your path, I suppose it is 54MHz, so you would be achieving some 10 full 5MPix frames/s from the sensor. Is that correct? (the aptina patch delivers less than 4 fps).
>
Yes, you are right. Whereas clock rate is set to 54MHz, with my
oscilloscope I have measured 57 MHz.
--
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
2011-05-18 9:10 ` javier Martin
@ 2011-05-18 14:23 ` Laurent Pinchart
-1 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-18 14:23 UTC (permalink / raw)
To: javier Martin
Cc: linux-media, g.liakhovetski, carlighting, beagleboard, linux-arm-kernel
Hi Javier,
On Wednesday 18 May 2011 11:10:03 javier Martin wrote:
> Hi Laurent,
> I've already fixed almost every issue you pointed out.
> However, I still have got some doubts that I hope you can clarify.
>
> On 17 May 2011 13:33, Laurent Pinchart wrote:
> > On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
> >> It has been tested in beagleboard xM, using LI-5M03 module.
> >>
> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >>
> >> +
> >> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> >> +{
> >> + int ret;
> >> +
> >> + if (mt9p031->pdata->set_xclk)
> >> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> >> + /* turn on VDD_IO */
> >> + ret = regulator_enable(mt9p031->reg_2v8);
> >> + if (ret) {
> >> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> >> + return -1;
> >> + }
> >
> >I would enable the regulator first. As a general rule, chips should be
> >powered up before their I/Os are actively driven.
> >
> >You need to restore registers here, otherwise all controls set by the user
> >will not be applied to the device.
>
> It's my mistake. This driver uses two regulators: 1,8 and 2,8 V
> respectively. 2,8V regulator powers analog part and I/O whereas 1,8V
> one powers the core. What I failed to do was keeping 1,8V regulator
> always powered on, so that register configuration was not lost, and
> power 2,8V regulator on and off as needed since it does not affect
> register values. However, I messed it all up.
>
> Ideally I would have to power 1,8V regulator on and off too. However,
> as you wisely pointed out, registers should be restored in that case.
> How am I supposed to keep track of register values? Are there any
> helper functions I can use for that purpose or must I create a custom
> register cache? Do you know any driver that uses this technique?
You can either keep track of register contents manually, or use the control
framework to restore the value of all controls by calling
v4l2_ctrl_handler_setup(). That's what the mt9v032 driver does in its power on
handler. You might have to restore a couple of registers manually if they're
not handled through controls.
> > [snip]
> >
> >> +static int mt9p031_set_params(struct i2c_client *client,
> >> + struct v4l2_rect *rect, u16 xskip, u16
> >> yskip)
> >
> > set_params should apply the parameters, not change them. They should have
> > already been validated by the callers.
>
> "mt9p031_set_params()" function is used by "mt9p031_set_crop()" and
> "mt9p031_set_format()", as you have correctly stated, these functions
> shouldn' apply parameters but only change them.
> I've checked mt9v032 driver and it is as you said. The question is,
> where should these parameters get applied then?
The mt9v032 driver applies those parameters at stream on time. The downside is
that it doesn't support resolution or crop changes at runtime. While changing
resolution at runtime might not make much sense in most cases, changing the
crop rectangle (providing its size stays the same) is a useful feature.
You can handle resolution changes at runtime, or you can choose to only set
the resolution at stream on time. In both cases, the mt9p031_set_params()
function should only apply parameters to the device, not change their value on
the driver side. That was the purpose of my original comment.
> >> +static int mt9p031_registered(struct v4l2_subdev *sd)
> >> +{
> >> + struct mt9p031 *mt9p031;
> >> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> >> +
> >> + mt9p031_power_off(mt9p031);
> >
> > What's that for ?
> >
> >> + return 0;
> >> +}
>
> Since "mt9p031_power_off()" and "mt9p031_power_on()" functions
> disable/enable the 2,8V regulator which powers I/O, it must be powered
> on during probe and after registering, it can be safely powered off.
Can't you just power it off at the end of your probe function, instead of
doing it here ?
> > You have a set_xclk callback to board code, so I assume the chip can be
> > driven by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3
> > ISP from board code, you need to get hold of the OMAP3 ISP device
> > pointer. Your next patch exports omap3isp_device, but I'm not sure
> > that's the way to go. One option is
> >
> > struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
> >
> > but that requires the subdev to be registered before the function can be
> > called. In that case you would need to move the probe code to the
> > registered subdev internal function.
>
> Yes, I tried using that function but it didn't work because subdev
> hadn't been registeret yet.
>
> > A clean solution is needed in the long run, preferably not involving
> > board code at all. It would be nice if the OMAP3 ISP driver could export
> > XCLKA/XCLKB as generic clock objects.
>
> So, what should I do in order to submit the driver to mainline?
> Do you want me to move the probe code to registered callback?
You should move the part that powers the device up and reads registers to the
registered callback. The board code should then use v4l2_dev_to_isp_device()
as showed above. This isn't a perfect solution, but at least it will be
consistent with other sensor drivers. When the OMAP3 ISP clocks will be
available through the clock framework, we will fix board code (and possibly
sensor drivers).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] mt9p031: Add mt9p031 sensor driver.
@ 2011-05-18 14:23 ` Laurent Pinchart
0 siblings, 0 replies; 26+ messages in thread
From: Laurent Pinchart @ 2011-05-18 14:23 UTC (permalink / raw)
To: linux-arm-kernel
Hi Javier,
On Wednesday 18 May 2011 11:10:03 javier Martin wrote:
> Hi Laurent,
> I've already fixed almost every issue you pointed out.
> However, I still have got some doubts that I hope you can clarify.
>
> On 17 May 2011 13:33, Laurent Pinchart wrote:
> > On Tuesday 17 May 2011 11:28:47 Javier Martin wrote:
> >> It has been tested in beagleboard xM, using LI-5M03 module.
> >>
> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >>
> >> +
> >> +static int mt9p031_power_on(struct mt9p031 *mt9p031)
> >> +{
> >> + int ret;
> >> +
> >> + if (mt9p031->pdata->set_xclk)
> >> + mt9p031->pdata->set_xclk(&mt9p031->subdev, 54000000);
> >> + /* turn on VDD_IO */
> >> + ret = regulator_enable(mt9p031->reg_2v8);
> >> + if (ret) {
> >> + pr_err("Failed to enable 2.8v regulator: %d\n", ret);
> >> + return -1;
> >> + }
> >
> >I would enable the regulator first. As a general rule, chips should be
> >powered up before their I/Os are actively driven.
> >
> >You need to restore registers here, otherwise all controls set by the user
> >will not be applied to the device.
>
> It's my mistake. This driver uses two regulators: 1,8 and 2,8 V
> respectively. 2,8V regulator powers analog part and I/O whereas 1,8V
> one powers the core. What I failed to do was keeping 1,8V regulator
> always powered on, so that register configuration was not lost, and
> power 2,8V regulator on and off as needed since it does not affect
> register values. However, I messed it all up.
>
> Ideally I would have to power 1,8V regulator on and off too. However,
> as you wisely pointed out, registers should be restored in that case.
> How am I supposed to keep track of register values? Are there any
> helper functions I can use for that purpose or must I create a custom
> register cache? Do you know any driver that uses this technique?
You can either keep track of register contents manually, or use the control
framework to restore the value of all controls by calling
v4l2_ctrl_handler_setup(). That's what the mt9v032 driver does in its power on
handler. You might have to restore a couple of registers manually if they're
not handled through controls.
> > [snip]
> >
> >> +static int mt9p031_set_params(struct i2c_client *client,
> >> + struct v4l2_rect *rect, u16 xskip, u16
> >> yskip)
> >
> > set_params should apply the parameters, not change them. They should have
> > already been validated by the callers.
>
> "mt9p031_set_params()" function is used by "mt9p031_set_crop()" and
> "mt9p031_set_format()", as you have correctly stated, these functions
> shouldn' apply parameters but only change them.
> I've checked mt9v032 driver and it is as you said. The question is,
> where should these parameters get applied then?
The mt9v032 driver applies those parameters at stream on time. The downside is
that it doesn't support resolution or crop changes at runtime. While changing
resolution at runtime might not make much sense in most cases, changing the
crop rectangle (providing its size stays the same) is a useful feature.
You can handle resolution changes at runtime, or you can choose to only set
the resolution at stream on time. In both cases, the mt9p031_set_params()
function should only apply parameters to the device, not change their value on
the driver side. That was the purpose of my original comment.
> >> +static int mt9p031_registered(struct v4l2_subdev *sd)
> >> +{
> >> + struct mt9p031 *mt9p031;
> >> + mt9p031 = container_of(sd, struct mt9p031, subdev);
> >> +
> >> + mt9p031_power_off(mt9p031);
> >
> > What's that for ?
> >
> >> + return 0;
> >> +}
>
> Since "mt9p031_power_off()" and "mt9p031_power_on()" functions
> disable/enable the 2,8V regulator which powers I/O, it must be powered
> on during probe and after registering, it can be safely powered off.
Can't you just power it off at the end of your probe function, instead of
doing it here ?
> > You have a set_xclk callback to board code, so I assume the chip can be
> > driven by one of the OMAP3 ISP XCLK signals. To call back to the OMAP3
> > ISP from board code, you need to get hold of the OMAP3 ISP device
> > pointer. Your next patch exports omap3isp_device, but I'm not sure
> > that's the way to go. One option is
> >
> > struct isp_device *isp = v4l2_dev_to_isp_device(subdev->v4l2_dev);
> >
> > but that requires the subdev to be registered before the function can be
> > called. In that case you would need to move the probe code to the
> > registered subdev internal function.
>
> Yes, I tried using that function but it didn't work because subdev
> hadn't been registeret yet.
>
> > A clean solution is needed in the long run, preferably not involving
> > board code at all. It would be nice if the OMAP3 ISP driver could export
> > XCLKA/XCLKB as generic clock objects.
>
> So, what should I do in order to submit the driver to mainline?
> Do you want me to move the probe code to registered callback?
You should move the part that powers the device up and reads registers to the
registered callback. The board code should then use v4l2_dev_to_isp_device()
as showed above. This isn't a perfect solution, but at least it will be
consistent with other sensor drivers. When the OMAP3 ISP clocks will be
available through the clock framework, we will fix board code (and possibly
sensor drivers).
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2011-05-18 14:23 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-17 9:28 Javier Martin
2011-05-17 9:28 ` No subject Javier Martin
2011-05-17 9:28 ` [PATCH 1/2] mt9p031: Add mt9p031 sensor driver Javier Martin
2011-05-17 9:28 ` Javier Martin
2011-05-17 11:33 ` Laurent Pinchart
2011-05-17 11:33 ` Laurent Pinchart
2011-05-17 11:47 ` Guennadi Liakhovetski
2011-05-17 11:47 ` Guennadi Liakhovetski
2011-05-17 11:59 ` javier Martin
2011-05-17 11:59 ` javier Martin
2011-05-17 17:14 ` Ivan Nazarenko
2011-05-17 17:14 ` Ivan Nazarenko
2011-05-18 10:34 ` javier Martin
2011-05-18 10:34 ` javier Martin
2011-05-17 23:18 ` Russell King - ARM Linux
2011-05-17 23:18 ` Russell King - ARM Linux
2011-05-18 9:10 ` javier Martin
2011-05-18 9:10 ` javier Martin
2011-05-18 14:23 ` Laurent Pinchart
2011-05-18 14:23 ` Laurent Pinchart
2011-05-17 9:28 ` [PATCH 2/2] OMAP3BEAGLE: Add support for mt9p031 sensor (LI-5M03 module) Javier Martin
2011-05-17 9:28 ` Javier Martin
2011-05-17 23:08 ` Russell King - ARM Linux
2011-05-17 23:08 ` Russell King - ARM Linux
2011-05-18 7:50 ` Laurent Pinchart
2011-05-18 7:50 ` Laurent Pinchart
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.