* [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
@ 2011-12-14 1:20 Martin Hostettler
2011-12-14 1:55 ` Marek Vasut
0 siblings, 1 reply; 11+ messages in thread
From: Martin Hostettler @ 2011-12-14 1:20 UTC (permalink / raw)
To: Laurent Pinchart, Hans Verkuil; +Cc: linux-media, Martin Hostettler
The MT9M032 is a parallel 1.6MP sensor from Micron controlled through I2C.
The driver creates a V4L2 subdevice. It currently supports cropping, gain,
exposure and v/h flipping controls in monochrome mode with an
external pixel clock.
Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
---
drivers/media/video/Kconfig | 7 +
drivers/media/video/Makefile | 1 +
drivers/media/video/mt9m032.c | 819 +++++++++++++++++++++++++++++++++++++++++
include/media/mt9m032.h | 38 ++
4 files changed, 865 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/mt9m032.c
create mode 100644 include/media/mt9m032.h
Changes in V3
* rebased to current mainline
* removed unneeded includes
* remove all translation code to hide black or active boundry sensor pixels. Userspace now needs to select crop in original device coordinates.
* remove useless consts, casts and defines
* changed enum_frame_size to return min == max
* removed duplicated input validation
* validate crop rectangle on set_crop
Changes in V2
* ported to current mainline
* Moved dispatching for subdev ioctls VIDIOC_DBG_G_REGISTER and VIDIOC_DBG_S_REGISTER into v4l2-subdev
* Removed VIDIOC_DBG_G_CHIP_IDENT support
* moved header to media/
* Fixed missing error handling
* lowercase hex constants
* moved v4l2_get_subdevdata to register access helpers
* use div_u64 instead of do_div
* moved all know register values into #define:ed constants
* Fixed error reporting, used clamp instead of open coding.
* lots of style fixes.
* add try_ctrl to make sure user space sees rounded values
* Fixed some problem in control framework usage.
* Fixed set_format to force width and height setup via crop. Simplyfied code.
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index b303a3f..cf05d9b 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -820,6 +820,13 @@ config SOC_CAMERA_MT9M001
This driver supports MT9M001 cameras from Micron, monochrome
and colour models.
+config VIDEO_MT9M032
+ tristate "MT9M032 camera sensor support"
+ depends on I2C && VIDEO_V4L2
+ help
+ This driver supports MT9M032 cameras from Micron, monochrome
+ models only.
+
config SOC_CAMERA_MT9M111
tristate "mt9m111, mt9m112 and mt9m131 support"
depends on SOC_CAMERA && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 117f9c4..39c7455 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o
obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o
obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o
+obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o
obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
obj-$(CONFIG_SOC_CAMERA_MT9T112) += mt9t112.o
diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
new file mode 100644
index 0000000..b4159c7
--- /dev/null
+++ b/drivers/media/video/mt9m032.c
@@ -0,0 +1,819 @@
+/*
+ * Driver for MT9M032 CMOS Image Sensor from Micron
+ *
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <gwlund@lundeng.com>
+ * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/v4l2-mediabus.h>
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-subdev.h>
+
+#include <media/mt9m032.h>
+
+#define MT9M032_CHIP_VERSION 0x00
+#define MT9M032_ROW_START 0x01
+#define MT9M032_COLUMN_START 0x02
+#define MT9M032_ROW_SIZE 0x03
+#define MT9M032_COLUMN_SIZE 0x04
+#define MT9M032_HBLANK 0x05
+#define MT9M032_VBLANK 0x06
+#define MT9M032_SHUTTER_WIDTH_HIGH 0x08
+#define MT9M032_SHUTTER_WIDTH_LOW 0x09
+#define MT9M032_PIX_CLK_CTRL 0x0a
+#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000
+#define MT9M032_RESTART 0x0b
+#define MT9M032_RESET 0x0d
+#define MT9M032_PLL_CONFIG1 0x11
+#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
+#define MT9M032_PLL_CONFIG1_MUL_SHIFT 8
+#define MT9M032_READ_MODE1 0x1e
+#define MT9M032_READ_MODE2 0x20
+#define MT9M032_READ_MODE2_VFLIP_SHIFT 15
+#define MT9M032_READ_MODE2_HFLIP_SHIFT 14
+#define MT9M032_READ_MODE2_ROW_BLC 0x40
+#define MT9M032_GAIN_GREEN1 0x2b
+#define MT9M032_GAIN_BLUE 0x2c
+#define MT9M032_GAIN_RED 0x2d
+#define MT9M032_GAIN_GREEN2 0x2e
+/* write only */
+#define MT9M032_GAIN_ALL 0x35
+#define MT9M032_GAIN_DIGITAL_MASK 0x7f
+#define MT9M032_GAIN_DIGITAL_SHIFT 8
+#define MT9M032_GAIN_AMUL_SHIFT 6
+#define MT9M032_GAIN_ANALOG_MASK 0x3f
+#define MT9M032_FORMATTER1 0x9e
+#define MT9M032_FORMATTER2 0x9f
+#define MT9M032_FORMATTER2_DOUT_EN 0x1000
+#define MT9M032_FORMATTER2_PIXCLK_EN 0x2000
+
+#define MT9M032_MAX_BLANKING_ROWS 0x7ff
+
+
+/*
+ * width and height include active boundry and black parts
+ *
+ * column 0- 15 active boundry
+ * column 16-1455 image
+ * column 1456-1471 active boundry
+ * column 1472-1599 black
+ *
+ * row 0- 51 black
+ * row 53- 59 active boundry
+ * row 60-1139 image
+ * row 1140-1147 active boundry
+ * row 1148-1151 black
+ */
+#define MT9M032_WIDTH 1600
+#define MT9M032_HEIGHT 1152
+#define MT9M032_MINIMALSIZE 32
+
+#define to_mt9m032(sd) container_of(sd, struct mt9m032, subdev)
+#define to_dev(sensor) &((struct i2c_client *)v4l2_get_subdevdata(&sensor->subdev))->dev
+
+struct mt9m032 {
+ struct v4l2_subdev subdev;
+ struct media_pad pad;
+ struct mt9m032_platform_data *pdata;
+ struct v4l2_ctrl_handler ctrls;
+
+ bool streaming;
+
+ int pix_clock;
+
+ struct v4l2_mbus_framefmt format; /* height and width always the same as in crop */
+ struct v4l2_rect crop;
+ struct v4l2_fract frame_interval;
+
+ struct v4l2_ctrl *hflip, *vflip;
+};
+
+
+static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+ s32 data = i2c_smbus_read_word_data(client, reg);
+
+ return data < 0 ? data : swab16(data);
+}
+
+static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
+ const u16 data)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+ return i2c_smbus_write_word_data(client, reg, swab16(data));
+}
+
+
+static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
+{
+ int effective_width;
+ u64 ns;
+
+ effective_width = width + 716; /* emperical value */
+ ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
+ dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns);
+ return ns;
+}
+
+static int mt9m032_update_timing(struct mt9m032 *sensor,
+ struct v4l2_fract *interval,
+ const struct v4l2_rect *crop)
+{
+ unsigned long row_time;
+ int additional_blanking_rows;
+ int min_blank;
+
+ if (!interval)
+ interval = &sensor->frame_interval;
+ if (!crop)
+ crop = &sensor->crop;
+
+ row_time = mt9m032_row_time(sensor, crop->width);
+
+ additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
+ ((u64)interval->denominator) * row_time)
+ - crop->height;
+
+ if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
+ /* hardware limits to 11 bit values */
+ interval->denominator = 1000;
+ interval->numerator = div_u64((crop->height + MT9M032_MAX_BLANKING_ROWS)
+ * ((u64)row_time) * interval->denominator,
+ 1000000000);
+ additional_blanking_rows = div_u64(((u64)1000000000) * interval->numerator,
+ ((u64)interval->denominator) * row_time)
+ - crop->height;
+ }
+ /* enforce minimal 1.6ms blanking time. */
+ min_blank = 1600000 / row_time;
+ additional_blanking_rows = clamp(additional_blanking_rows,
+ min_blank, MT9M032_MAX_BLANKING_ROWS);
+
+ return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
+}
+
+static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
+ const struct v4l2_rect *crop)
+{
+ int ret;
+
+ ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height - 1);
+ /* offsets compensate for black border */
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop->left);
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
+ if (!ret)
+ ret = mt9m032_update_timing(sensor, NULL, crop);
+ return ret;
+}
+
+static int update_formatter2(struct mt9m032 *sensor, bool streaming)
+{
+ u16 reg_val = MT9M032_FORMATTER2_DOUT_EN
+ | 0x0070; /* parts reserved! */
+ /* possibly for changing to 14-bit mode */
+
+ if (streaming)
+ reg_val |= MT9M032_FORMATTER2_PIXCLK_EN; /* pixclock enable */
+
+ return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
+}
+
+static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+ int ret;
+
+ ret = update_formatter2(sensor, streaming);
+ if (!ret)
+ sensor->streaming = streaming;
+ return ret;
+}
+
+static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_mbus_code_enum *code)
+{
+ if (code->index != 0 || code->pad != 0)
+ return -EINVAL;
+ code->code = V4L2_MBUS_FMT_Y8_1X8;
+ return 0;
+}
+
+static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_frame_size_enum *fse)
+{
+ if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad != 0)
+ return -EINVAL;
+
+ fse->min_width = MT9M032_WIDTH;
+ fse->max_width = MT9M032_WIDTH;
+ fse->min_height = MT9M032_HEIGHT;
+ fse->max_height = MT9M032_HEIGHT;
+
+ return 0;
+}
+
+/**
+ * __mt9m032_get_pad_crop() - get crop rect
+ * @sensor: pointer to the sensor struct
+ * @fh: filehandle for getting the try crop rect from
+ * @which: select try or active crop rect
+ * Returns a pointer the current active or fh relative try crop rect
+ */
+static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
+ struct v4l2_subdev_fh *fh,
+ u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(fh, 0);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &sensor->crop;
+ default:
+ return NULL;
+ }
+}
+
+/**
+ * __mt9m032_get_pad_format() - get format
+ * @sensor: pointer to the sensor struct
+ * @fh: filehandle for getting the try format from
+ * @which: select try or active format
+ * Returns a pointer the current active or fh relative try format
+ */
+static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032 *sensor,
+ struct v4l2_subdev_fh *fh,
+ u32 which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_format(fh, 0);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return &sensor->format;
+ default:
+ return NULL;
+ }
+}
+
+static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+ struct v4l2_mbus_framefmt *format;
+
+ format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
+
+ fmt->format = *format;
+
+ return 0;
+}
+
+static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_format *fmt)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+
+ if (sensor->streaming)
+ return -EBUSY;
+
+
+ /*
+ * fmt->format.colorspace, fmt->format.code and fmt->format.field are ignored
+ * and thus forced to fixed values by the get call below.
+ *
+ * fmt->format.width, fmt->format.height are forced to the values set via crop
+ */
+
+ return mt9m032_get_pad_format(subdev, fh, fmt);
+}
+
+static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+ struct v4l2_rect *curcrop;
+
+ curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+
+ crop->rect = *curcrop;
+
+ return 0;
+}
+
+static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct v4l2_subdev_fh *fh,
+ struct v4l2_subdev_crop *crop)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+ struct v4l2_mbus_framefmt tmp_format;
+ struct v4l2_rect tmp_crop_rect;
+ struct v4l2_mbus_framefmt *format;
+ struct v4l2_rect *crop_rect;
+
+ if (sensor->streaming)
+ return -EBUSY;
+
+ format = __mt9m032_get_pad_format(sensor, fh, crop->which);
+ crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
+ if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ tmp_crop_rect = *crop_rect;
+ tmp_format = *format;
+ format = &tmp_format;
+ crop_rect = &tmp_crop_rect;
+ }
+
+ crop_rect->top = clamp(crop->rect.top, 0,
+ MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
+ crop_rect->left = clamp(crop->rect.left, 0,
+ MT9M032_WIDTH - MT9M032_MINIMALSIZE);
+ crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
+ MT9M032_HEIGHT - crop_rect->top);
+ crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
+ MT9M032_WIDTH - crop_rect->left) & ~1;
+
+ format->height = crop_rect->height;
+ format->width = crop_rect->width;
+
+ if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
+ int ret = mt9m032_update_geom_timing(sensor, crop_rect);
+
+ if (!ret) {
+ sensor->crop = tmp_crop_rect;
+ sensor->format = tmp_format;
+ }
+ return ret;
+ }
+
+ return 0;
+}
+
+static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+
+ fi->pad = 0;
+ memset(fi->reserved, 0, sizeof(fi->reserved));
+ fi->interval = sensor->frame_interval;
+
+ return 0;
+}
+
+static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_frame_interval *fi)
+{
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+ int ret;
+
+ if (sensor->streaming)
+ return -EBUSY;
+
+ memset(fi->reserved, 0, sizeof(fi->reserved));
+
+ ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
+ if (!ret)
+ sensor->frame_interval = fi->interval;
+ return ret;
+}
+
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+static int mt9m032_g_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ struct mt9m032 *sensor = to_mt9m032(sd);
+ struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+ int val;
+
+ if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+ return -EINVAL;
+ if (reg->match.addr != client->addr)
+ return -ENODEV;
+
+ val = mt9m032_read_reg(sensor, reg->reg);
+ if (val < 0)
+ return -EIO;
+
+ reg->size = 2;
+ reg->val = val;
+
+ return 0;
+}
+
+static int mt9m032_s_register(struct v4l2_subdev *sd,
+ struct v4l2_dbg_register *reg)
+{
+ struct mt9m032 *sensor = to_mt9m032(sd);
+ struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
+
+ if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
+ return -EINVAL;
+
+ if (reg->match.addr != client->addr)
+ return -ENODEV;
+
+ if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
+ return -EIO;
+
+ return 0;
+}
+#endif
+
+static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool hflip)
+{
+ int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
+ | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
+ | MT9M032_READ_MODE2_ROW_BLC
+ | 0x0007;
+
+ return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
+}
+
+static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
+{
+ return update_read_mode2(sensor, sensor->vflip->cur.val, val);
+}
+
+static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
+{
+ return update_read_mode2(sensor, val, sensor->hflip->cur.val);
+}
+
+static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
+{
+ int shutter_width;
+ u16 high_val, low_val;
+ int ret;
+
+ /* shutter width is in row times */
+ shutter_width = (val * 1000) / mt9m032_row_time(sensor, sensor->crop.width);
+
+ high_val = (shutter_width >> 16) & 0xf;
+ low_val = shutter_width & 0xffff;
+
+ ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
+ if (!ret)
+ mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
+
+ return ret;
+}
+
+static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
+{
+ int digital_gain_val; /* in 1/8th (0..127) */
+ int analog_mul; /* 0 or 1 */
+ int analog_gain_val; /* in 1/16th. (0..63) */
+ u16 reg_val;
+
+ digital_gain_val = 51; /* from setup example */
+
+ if (val < 63) {
+ analog_mul = 0;
+ analog_gain_val = val;
+ } else {
+ analog_mul = 1;
+ analog_gain_val = val / 2;
+ }
+
+ /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
+ /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
+
+ reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) << MT9M032_GAIN_DIGITAL_SHIFT
+ | (analog_mul & 1) << MT9M032_GAIN_AMUL_SHIFT
+ | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
+
+ return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
+}
+
+static int mt9m032_setup_pll(struct mt9m032 *sensor)
+{
+ struct mt9m032_platform_data* pdata = sensor->pdata;
+ u16 reg_pll1;
+ unsigned int pre_div;
+ int res, ret;
+
+ /* TODO: also support other pre-div values */
+ if (pdata->pll_pre_div != 6) {
+ dev_warn(to_dev(sensor),
+ "Unsupported PLL pre-divisor value %u, using default 6\n",
+ pdata->pll_pre_div);
+ }
+ pre_div = 6;
+
+ sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
+ (pre_div * pdata->pll_out_div);
+
+ reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
+ | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
+
+ ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as clock source */
+
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
+ /* more reserved, Continuous */
+ /* Master Mode */
+ if (!ret)
+ res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
+
+ if (!ret)
+ ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
+ /* Set 14-bit mode, select 7 divider */
+
+ return ret;
+}
+
+static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
+{
+ if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
+ /* round because of multiplier used for values >= 63 */
+ ctrl->val &= ~1;
+ }
+
+ return 0;
+}
+
+static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032, ctrls);
+
+ switch (ctrl->id) {
+ case V4L2_CID_GAIN:
+ return mt9m032_set_gain(sensor, ctrl->val);
+
+ case V4L2_CID_HFLIP:
+ return mt9m032_set_hflip(sensor, ctrl->val);
+
+ case V4L2_CID_VFLIP:
+ return mt9m032_set_vflip(sensor, ctrl->val);
+
+ case V4L2_CID_EXPOSURE:
+ return mt9m032_set_exposure(sensor, ctrl->val);
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
+ .s_stream = mt9m032_s_stream,
+ .g_frame_interval = mt9m032_get_frame_interval,
+ .s_frame_interval = mt9m032_set_frame_interval,
+};
+
+static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
+ .s_ctrl = mt9m032_set_ctrl,
+ .try_ctrl = mt9m032_try_ctrl,
+};
+
+
+static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .g_register = mt9m032_g_register,
+ .s_register = mt9m032_s_register,
+#endif
+};
+
+static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
+ .enum_mbus_code = mt9m032_enum_mbus_code,
+ .enum_frame_size = mt9m032_enum_frame_size,
+ .get_fmt = mt9m032_get_pad_format,
+ .set_fmt = mt9m032_set_pad_format,
+ .set_crop = mt9m032_set_crop,
+ .get_crop = mt9m032_get_crop,
+};
+
+static const struct v4l2_subdev_ops mt9m032_ops = {
+ .core = &mt9m032_core_ops,
+ .video = &mt9m032_video_ops,
+ .pad = &mt9m032_pad_ops,
+};
+
+static int mt9m032_probe(struct i2c_client *client,
+ const struct i2c_device_id *devid)
+{
+ struct mt9m032 *sensor;
+ int chip_version;
+ int res, ret;
+
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) {
+ dev_warn(&client->adapter->dev,
+ "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
+ return -EIO;
+ }
+
+ if (!client->dev.platform_data)
+ return -ENODEV;
+
+ sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
+ if (sensor == NULL)
+ return -ENOMEM;
+
+ sensor->pdata = client->dev.platform_data;
+
+ v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
+ sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+
+
+ /*
+ * This driver was developed with a camera module with seperate external
+ * pix clock. For setups which use the clock from the camera interface
+ * the code will need to be extended with the appropriate platform
+ * callback to setup the clock.
+ */
+ chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
+ if (0x1402 == chip_version) {
+ dev_info(&client->dev, "mt9m032: detected sensor.\n");
+ } else {
+ dev_warn(&client->dev, "mt9m032: error: detected unsupported chip version 0x%x\n",
+ chip_version);
+ ret = -ENODEV;
+ goto free_sensor;
+ }
+
+
+
+ sensor->frame_interval.numerator = 1;
+ sensor->frame_interval.denominator = 30;
+
+ sensor->crop.left = 416;
+ sensor->crop.top = 360;
+ sensor->crop.width = 640;
+ sensor->crop.height = 480;
+
+ sensor->format.width = sensor->crop.width;
+ sensor->format.height = sensor->crop.height;
+ sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
+ sensor->format.field = V4L2_FIELD_NONE;
+ sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
+
+ v4l2_ctrl_handler_init(&sensor->ctrls, 4);
+
+ v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+ V4L2_CID_GAIN, 0, 127, 1, 64);
+
+ sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+ V4L2_CID_HFLIP, 0, 1, 1, 0);
+ sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+ V4L2_CID_VFLIP, 0, 1, 1, 0);
+ v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
+ V4L2_CID_EXPOSURE, 0, 8000, 1, 1700); /* 1.7ms */
+
+
+ if (sensor->ctrls.error) {
+ ret = sensor->ctrls.error;
+ dev_err(&client->dev, "control initialization error %d\n", ret);
+ goto free_ctrl;
+ }
+
+ sensor->subdev.ctrl_handler = &sensor->ctrls;
+ sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
+ ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
+ if (ret < 0)
+ goto free_ctrl;
+
+ ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1); /* reset on */
+ if (ret < 0)
+ goto free_ctrl;
+ mt9m032_write_reg(sensor, MT9M032_RESET, 0); /* reset off */
+ if (ret < 0)
+ goto free_ctrl;
+
+ ret = mt9m032_setup_pll(sensor);
+ if (ret < 0)
+ goto free_ctrl;
+ msleep(10);
+
+ v4l2_ctrl_handler_setup(&sensor->ctrls);
+
+ /* SIZE */
+ ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
+ if (ret < 0)
+ goto free_ctrl;
+
+ ret = mt9m032_write_reg(sensor, 0x41, 0x0000); /* reserved !!! */
+ if (ret < 0)
+ goto free_ctrl;
+ ret = mt9m032_write_reg(sensor, 0x42, 0x0003); /* reserved !!! */
+ if (ret < 0)
+ goto free_ctrl;
+ ret = mt9m032_write_reg(sensor, 0x43, 0x0003); /* reserved !!! */
+ if (ret < 0)
+ goto free_ctrl;
+ ret = mt9m032_write_reg(sensor, 0x7f, 0x0000); /* reserved !!! */
+ if (ret < 0)
+ goto free_ctrl;
+ if (sensor->pdata->invert_pixclock) {
+ mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL, MT9M032_PIX_CLK_CTRL_INV_PIXCLK);
+ if (ret < 0)
+ goto free_ctrl;
+ }
+
+ res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
+
+ ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
+ if (ret < 0)
+ goto free_ctrl;
+ msleep(100);
+ ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
+ if (ret < 0)
+ goto free_ctrl;
+ msleep(100);
+ ret = update_formatter2(sensor, false);
+ if (ret < 0)
+ goto free_ctrl;
+
+ return ret;
+
+free_ctrl:
+ v4l2_ctrl_handler_free(&sensor->ctrls);
+
+free_sensor:
+ kfree(sensor);
+ return ret;
+}
+
+static int mt9m032_remove(struct i2c_client *client)
+{
+ struct v4l2_subdev *subdev = i2c_get_clientdata(client);
+ struct mt9m032 *sensor = to_mt9m032(subdev);
+
+ v4l2_device_unregister_subdev(&sensor->subdev);
+ v4l2_ctrl_handler_free(&sensor->ctrls);
+ media_entity_cleanup(&sensor->subdev.entity);
+ kfree(sensor);
+ return 0;
+}
+
+static const struct i2c_device_id mt9m032_id_table[] = {
+ {MT9M032_NAME, 0},
+ {}
+};
+
+MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
+
+static struct i2c_driver mt9m032_i2c_driver = {
+ .driver = {
+ .name = MT9M032_NAME,
+ },
+ .probe = mt9m032_probe,
+ .remove = mt9m032_remove,
+ .id_table = mt9m032_id_table,
+};
+
+static int __init mt9m032_init(void)
+{
+ int rval;
+
+ rval = i2c_add_driver(&mt9m032_i2c_driver);
+ if (rval)
+ pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
+
+ return rval;
+}
+
+static void mt9m032_exit(void)
+{
+ i2c_del_driver(&mt9m032_i2c_driver);
+}
+
+module_init(mt9m032_init);
+module_exit(mt9m032_exit);
+
+MODULE_AUTHOR("Martin Hostettler");
+MODULE_DESCRIPTION("MT9M032 camera sensor driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
new file mode 100644
index 0000000..94cefc5
--- /dev/null
+++ b/include/media/mt9m032.h
@@ -0,0 +1,38 @@
+/*
+ * Driver for MT9M032 CMOS Image Sensor from Micron
+ *
+ * Copyright (C) 2010-2011 Lund Engineering
+ * Contact: Gil Lund <gwlund@lundeng.com>
+ * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ *
+ */
+
+#ifndef MT9M032_H
+#define MT9M032_H
+
+#define MT9M032_NAME "mt9m032"
+#define MT9M032_I2C_ADDR (0xb8 >> 1)
+
+struct mt9m032_platform_data {
+ u32 ext_clock;
+ u32 pll_pre_div;
+ u32 pll_mul;
+ u32 pll_out_div;
+ int invert_pixclock;
+
+};
+#endif /* MT9M032_H */
--
1.7.2.5
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 1:20 [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor Martin Hostettler
@ 2011-12-14 1:55 ` Marek Vasut
2011-12-14 7:14 ` martin
2011-12-14 21:44 ` Guennadi Liakhovetski
0 siblings, 2 replies; 11+ messages in thread
From: Marek Vasut @ 2011-12-14 1:55 UTC (permalink / raw)
To: Martin Hostettler; +Cc: Laurent Pinchart, Hans Verkuil, linux-media
Dear Martin Hostettler,
> The MT9M032 is a parallel 1.6MP sensor from Micron controlled through I2C.
>
> The driver creates a V4L2 subdevice. It currently supports cropping, gain,
> exposure and v/h flipping controls in monochrome mode with an
> external pixel clock.
>
> Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> ---
> drivers/media/video/Kconfig | 7 +
> drivers/media/video/Makefile | 1 +
> drivers/media/video/mt9m032.c | 819
> +++++++++++++++++++++++++++++++++++++++++ include/media/mt9m032.h |
> 38 ++
> 4 files changed, 865 insertions(+), 0 deletions(-)
> create mode 100644 drivers/media/video/mt9m032.c
> create mode 100644 include/media/mt9m032.h
>
> Changes in V3
> * rebased to current mainline
> * removed unneeded includes
> * remove all translation code to hide black or active boundry sensor
> pixels. Userspace now needs to select crop in original device coordinates.
> * remove useless consts, casts and defines
> * changed enum_frame_size to return min == max
> * removed duplicated input validation
> * validate crop rectangle on set_crop
>
> Changes in V2
> * ported to current mainline
> * Moved dispatching for subdev ioctls VIDIOC_DBG_G_REGISTER and
> VIDIOC_DBG_S_REGISTER into v4l2-subdev * Removed VIDIOC_DBG_G_CHIP_IDENT
> support
> * moved header to media/
> * Fixed missing error handling
> * lowercase hex constants
> * moved v4l2_get_subdevdata to register access helpers
> * use div_u64 instead of do_div
> * moved all know register values into #define:ed constants
> * Fixed error reporting, used clamp instead of open coding.
> * lots of style fixes.
> * add try_ctrl to make sure user space sees rounded values
> * Fixed some problem in control framework usage.
> * Fixed set_format to force width and height setup via crop. Simplyfied
> code.
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index b303a3f..cf05d9b 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -820,6 +820,13 @@ config SOC_CAMERA_MT9M001
> This driver supports MT9M001 cameras from Micron, monochrome
> and colour models.
>
> +config VIDEO_MT9M032
> + tristate "MT9M032 camera sensor support"
> + depends on I2C && VIDEO_V4L2
> + help
> + This driver supports MT9M032 cameras from Micron, monochrome
> + models only.
> +
> config SOC_CAMERA_MT9M111
> tristate "mt9m111, mt9m112 and mt9m131 support"
> depends on SOC_CAMERA && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 117f9c4..39c7455 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o
>
> obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o
> obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o
> +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
> obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o
> obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
> obj-$(CONFIG_SOC_CAMERA_MT9T112) += mt9t112.o
> diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> new file mode 100644
> index 0000000..b4159c7
> --- /dev/null
> +++ b/drivers/media/video/mt9m032.c
> @@ -0,0 +1,819 @@
> +/*
> + * Driver for MT9M032 CMOS Image Sensor from Micron
> + *
> + * Copyright (C) 2010-2011 Lund Engineering
> + * Contact: Gil Lund <gwlund@lundeng.com>
> + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/v4l2-mediabus.h>
> +
> +#include <media/media-entity.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-subdev.h>
> +
> +#include <media/mt9m032.h>
> +
> +#define MT9M032_CHIP_VERSION 0x00
> +#define MT9M032_ROW_START 0x01
> +#define MT9M032_COLUMN_START 0x02
> +#define MT9M032_ROW_SIZE 0x03
> +#define MT9M032_COLUMN_SIZE 0x04
> +#define MT9M032_HBLANK 0x05
> +#define MT9M032_VBLANK 0x06
> +#define MT9M032_SHUTTER_WIDTH_HIGH 0x08
> +#define MT9M032_SHUTTER_WIDTH_LOW 0x09
> +#define MT9M032_PIX_CLK_CTRL 0x0a
> +#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000
> +#define MT9M032_RESTART 0x0b
> +#define MT9M032_RESET 0x0d
> +#define MT9M032_PLL_CONFIG1 0x11
> +#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
> +#define MT9M032_PLL_CONFIG1_MUL_SHIFT 8
> +#define MT9M032_READ_MODE1 0x1e
> +#define MT9M032_READ_MODE2 0x20
> +#define MT9M032_READ_MODE2_VFLIP_SHIFT 15
> +#define MT9M032_READ_MODE2_HFLIP_SHIFT 14
> +#define MT9M032_READ_MODE2_ROW_BLC 0x40
> +#define MT9M032_GAIN_GREEN1 0x2b
> +#define MT9M032_GAIN_BLUE 0x2c
> +#define MT9M032_GAIN_RED 0x2d
> +#define MT9M032_GAIN_GREEN2 0x2e
> +/* write only */
> +#define MT9M032_GAIN_ALL 0x35
> +#define MT9M032_GAIN_DIGITAL_MASK 0x7f
> +#define MT9M032_GAIN_DIGITAL_SHIFT 8
> +#define MT9M032_GAIN_AMUL_SHIFT 6
> +#define MT9M032_GAIN_ANALOG_MASK 0x3f
> +#define MT9M032_FORMATTER1 0x9e
> +#define MT9M032_FORMATTER2 0x9f
> +#define MT9M032_FORMATTER2_DOUT_EN 0x1000
> +#define MT9M032_FORMATTER2_PIXCLK_EN 0x2000
> +
> +#define MT9M032_MAX_BLANKING_ROWS 0x7ff
Fix the alignment here please.
> +
> +
> +/*
> + * width and height include active boundry and black parts
> + *
> + * column 0- 15 active boundry
> + * column 16-1455 image
> + * column 1456-1471 active boundry
> + * column 1472-1599 black
> + *
> + * row 0- 51 black
> + * row 53- 59 active boundry
> + * row 60-1139 image
> + * row 1140-1147 active boundry
> + * row 1148-1151 black
> + */
> +#define MT9M032_WIDTH 1600
> +#define MT9M032_HEIGHT 1152
> +#define MT9M032_MINIMALSIZE 32
> +
> +#define to_mt9m032(sd) container_of(sd, struct mt9m032, subdev)
> +#define to_dev(sensor) &((struct i2c_client
> *)v4l2_get_subdevdata(&sensor->subdev))->dev +
> +struct mt9m032 {
> + struct v4l2_subdev subdev;
> + struct media_pad pad;
> + struct mt9m032_platform_data *pdata;
> + struct v4l2_ctrl_handler ctrls;
> +
> + bool streaming;
> +
> + int pix_clock;
> +
> + struct v4l2_mbus_framefmt format; /* height and width always the
same as
> in crop */ + struct v4l2_rect crop;
> + struct v4l2_fract frame_interval;
> +
> + struct v4l2_ctrl *hflip, *vflip;
> +};
> +
> +
> +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> + s32 data = i2c_smbus_read_word_data(client, reg);
> +
> + return data < 0 ? data : swab16(data);
Uhm ... why ?
> +}
> +
> +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> + const u16 data)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> +
> + return i2c_smbus_write_word_data(client, reg, swab16(data));
> +}
> +
> +
> +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
> +{
> + int effective_width;
> + u64 ns;
> +
> + effective_width = width + 716; /* emperical value */
> + ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
Magic number, please fix globally.
> + dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns);
> + return ns;
> +}
> +
> +static int mt9m032_update_timing(struct mt9m032 *sensor,
> + struct v4l2_fract *interval,
> + const struct v4l2_rect *crop)
> +{
> + unsigned long row_time;
> + int additional_blanking_rows;
> + int min_blank;
> +
> + if (!interval)
> + interval = &sensor->frame_interval;
> + if (!crop)
> + crop = &sensor->crop;
> +
> + row_time = mt9m032_row_time(sensor, crop->width);
> +
> + additional_blanking_rows = div_u64(((u64)1000000000) *
> interval->numerator, +
> ((u64)interval->denominator) * row_time) + -
> crop->height;
> +
> + if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
> + /* hardware limits to 11 bit values */
> + interval->denominator = 1000;
> + interval->numerator = div_u64((crop->height +
MT9M032_MAX_BLANKING_ROWS)
> + * ((u64)row_time) * interval-
>denominator,
> + 1000000000);
> + additional_blanking_rows = div_u64(((u64)1000000000) *
100000<many zeroes>UL or ULL might work better btw?
> interval->numerator, +
> ((u64)interval->denominator) * row_time) + -
> crop->height;
> + }
> + /* enforce minimal 1.6ms blanking time. */
> + min_blank = 1600000 / row_time;
> + additional_blanking_rows = clamp(additional_blanking_rows,
> + min_blank, MT9M032_MAX_BLANKING_ROWS);
> +
> + return mt9m032_write_reg(sensor, MT9M032_VBLANK,
> additional_blanking_rows); +}
> +
> +static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
> + const struct v4l2_rect *crop)
> +{
> + int ret;
> +
> + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height -
1);
> + /* offsets compensate for black border */
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop-
>left);
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
> + if (!ret)
> + ret = mt9m032_update_timing(sensor, NULL, crop);
> + return ret;
> +}
> +
> +static int update_formatter2(struct mt9m032 *sensor, bool streaming)
> +{
> + u16 reg_val = MT9M032_FORMATTER2_DOUT_EN
> + | 0x0070; /* parts reserved! */
> + /* possibly for changing to 14-bit mode */
MAGIC!
> +
> + if (streaming)
> + reg_val |= MT9M032_FORMATTER2_PIXCLK_EN; /* pixclock enable */
> +
> + return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
> +}
> +
> +static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> + int ret;
> +
> + ret = update_formatter2(sensor, streaming);
> + if (!ret)
> + sensor->streaming = streaming;
> + return ret;
> +}
> +
> +static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_mbus_code_enum *code)
> +{
> + if (code->index != 0 || code->pad != 0)
Parenthsis missing ? () || () ?
> + return -EINVAL;
> + code->code = V4L2_MBUS_FMT_Y8_1X8;
> + return 0;
> +}
> +
> +static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad !=
> 0) + return -EINVAL;
> +
> + fse->min_width = MT9M032_WIDTH;
> + fse->max_width = MT9M032_WIDTH;
> + fse->min_height = MT9M032_HEIGHT;
> + fse->max_height = MT9M032_HEIGHT;
> +
> + return 0;
> +}
> +
> +/**
> + * __mt9m032_get_pad_crop() - get crop rect
> + * @sensor: pointer to the sensor struct
> + * @fh: filehandle for getting the try crop rect from
> + * @which: select try or active crop rect
> + * Returns a pointer the current active or fh relative try crop rect
> + */
> +static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
> + struct v4l2_subdev_fh *fh,
> + u32 which)
Do NOT use __function, they might (even though in this case unlikely) be used by
compiler.
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_crop(fh, 0);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &sensor->crop;
> + default:
> + return NULL;
> + }
> +}
> +
> +/**
> + * __mt9m032_get_pad_format() - get format
> + * @sensor: pointer to the sensor struct
> + * @fh: filehandle for getting the try format from
> + * @which: select try or active format
> + * Returns a pointer the current active or fh relative try format
> + */
> +static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032
> *sensor, + struct
v4l2_subdev_fh *fh,
> + u32 which)
> +{
> + switch (which) {
> + case V4L2_SUBDEV_FORMAT_TRY:
> + return v4l2_subdev_get_try_format(fh, 0);
> + case V4L2_SUBDEV_FORMAT_ACTIVE:
> + return &sensor->format;
> + default:
> + return NULL;
> + }
> +}
> +
> +static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> + struct v4l2_mbus_framefmt *format;
> +
> + format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
> +
> + fmt->format = *format;
> +
> + return 0;
> +}
> +
> +static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_fh *fh,
> + struct v4l2_subdev_format *fmt)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> +
> + /*
> + * fmt->format.colorspace, fmt->format.code and fmt->format.field are
> ignored + * and thus forced to fixed values by the get call below.
> + *
> + * fmt->format.width, fmt->format.height are forced to the values set
via
> crop + */
> +
> + return mt9m032_get_pad_format(subdev, fh, fmt);
> +}
> +
> +static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct
> v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> + struct v4l2_rect *curcrop;
> +
> + curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> +
> + crop->rect = *curcrop;
> +
> + return 0;
> +}
> +
> +static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct
> v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> + struct v4l2_mbus_framefmt tmp_format;
> + struct v4l2_rect tmp_crop_rect;
> + struct v4l2_mbus_framefmt *format;
> + struct v4l2_rect *crop_rect;
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> + format = __mt9m032_get_pad_format(sensor, fh, crop->which);
> + crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + tmp_crop_rect = *crop_rect;
> + tmp_format = *format;
> + format = &tmp_format;
> + crop_rect = &tmp_crop_rect;
> + }
> +
> + crop_rect->top = clamp(crop->rect.top, 0,
> + MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
> + crop_rect->left = clamp(crop->rect.left, 0,
> + MT9M032_WIDTH - MT9M032_MINIMALSIZE);
> + crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
> + MT9M032_HEIGHT - crop_rect->top);
> + crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
> + MT9M032_WIDTH - crop_rect->left) & ~1;
> +
> + format->height = crop_rect->height;
> + format->width = crop_rect->width;
> +
> + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> + int ret = mt9m032_update_geom_timing(sensor, crop_rect);
> +
> + if (!ret) {
> + sensor->crop = tmp_crop_rect;
> + sensor->format = tmp_format;
> + }
> + return ret;
return ret below and set ret = 0 at the begining
> + }
> +
> + return 0;
> +}
> +
> +static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> +
> + fi->pad = 0;
> + memset(fi->reserved, 0, sizeof(fi->reserved));
> + fi->interval = sensor->frame_interval;
> +
> + return 0;
> +}
> +
> +static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_frame_interval *fi)
> +{
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> + int ret;
> +
> + if (sensor->streaming)
> + return -EBUSY;
> +
> + memset(fi->reserved, 0, sizeof(fi->reserved));
> +
> + ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
> + if (!ret)
> + sensor->frame_interval = fi->interval;
> + return ret;
> +}
> +
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> +static int mt9m032_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct mt9m032 *sensor = to_mt9m032(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> + int val;
> +
> + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> + return -EINVAL;
> + if (reg->match.addr != client->addr)
> + return -ENODEV;
> +
> + val = mt9m032_read_reg(sensor, reg->reg);
> + if (val < 0)
> + return -EIO;
> +
> + reg->size = 2;
> + reg->val = val;
> +
> + return 0;
> +}
> +
> +static int mt9m032_s_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
> +{
> + struct mt9m032 *sensor = to_mt9m032(sd);
> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> +
> + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> + return -EINVAL;
> +
> + if (reg->match.addr != client->addr)
> + return -ENODEV;
> +
> + if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
> + return -EIO;
> +
> + return 0;
> +}
> +#endif
> +
> +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> hflip) +{
> + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
!!(bool) ? hmm ...
> + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> + | MT9M032_READ_MODE2_ROW_BLC
> + | 0x0007;
> +
> + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> +}
> +
> +static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
> +{
> + return update_read_mode2(sensor, sensor->vflip->cur.val, val);
> +}
> +
> +static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
> +{
> + return update_read_mode2(sensor, val, sensor->hflip->cur.val);
> +}
> +
> +static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
> +{
> + int shutter_width;
> + u16 high_val, low_val;
> + int ret;
> +
> + /* shutter width is in row times */
> + shutter_width = (val * 1000) / mt9m032_row_time(sensor,
> sensor->crop.width); +
> + high_val = (shutter_width >> 16) & 0xf;
> + low_val = shutter_width & 0xffff;
> +
> + ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
> + if (!ret)
> + mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
You're not checking return value here ?
> +
> + return ret;
> +}
> +
> +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> +{
> + int digital_gain_val; /* in 1/8th (0..127) */
> + int analog_mul; /* 0 or 1 */
> + int analog_gain_val; /* in 1/16th. (0..63) */
> + u16 reg_val;
> +
> + digital_gain_val = 51; /* from setup example */
> +
> + if (val < 63) {
> + analog_mul = 0;
> + analog_gain_val = val;
> + } else {
> + analog_mul = 1;
> + analog_gain_val = val / 2;
> + }
> +
> + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> +
> + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> MT9M032_GAIN_AMUL_SHIFT
> + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> +
> + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> +}
> +
> +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> +{
> + struct mt9m032_platform_data* pdata = sensor->pdata;
> + u16 reg_pll1;
> + unsigned int pre_div;
> + int res, ret;
> +
> + /* TODO: also support other pre-div values */
> + if (pdata->pll_pre_div != 6) {
> + dev_warn(to_dev(sensor),
> + "Unsupported PLL pre-divisor value %u, using default
6\n",
> + pdata->pll_pre_div);
> + }
> + pre_div = 6;
> +
> + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> + (pre_div * pdata->pll_out_div);
> +
> + reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> + | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> +
> + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
clock
> source */ +
urm ... magic ... black magic ...
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> + /* more reserved,
Continuous */
> + /* Master Mode */
> + if (!ret)
> + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> +
> + if (!ret)
> + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> + /* Set 14-bit mode, select 7 divider */
> +
> + return ret;
> +}
> +
> +static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
> + /* round because of multiplier used for values >= 63 */
> + ctrl->val &= ~1;
> + }
> +
> + return 0;
> +}
> +
> +static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032,
> ctrls); +
> + switch (ctrl->id) {
> + case V4L2_CID_GAIN:
> + return mt9m032_set_gain(sensor, ctrl->val);
> +
> + case V4L2_CID_HFLIP:
> + return mt9m032_set_hflip(sensor, ctrl->val);
> +
> + case V4L2_CID_VFLIP:
> + return mt9m032_set_vflip(sensor, ctrl->val);
> +
> + case V4L2_CID_EXPOSURE:
> + return mt9m032_set_exposure(sensor, ctrl->val);
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
> + .s_stream = mt9m032_s_stream,
> + .g_frame_interval = mt9m032_get_frame_interval,
> + .s_frame_interval = mt9m032_set_frame_interval,
> +};
> +
> +static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
> + .s_ctrl = mt9m032_set_ctrl,
> + .try_ctrl = mt9m032_try_ctrl,
> +};
> +
> +
> +static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
> +#ifdef CONFIG_VIDEO_ADV_DEBUG
> + .g_register = mt9m032_g_register,
> + .s_register = mt9m032_s_register,
> +#endif
> +};
> +
> +static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
> + .enum_mbus_code = mt9m032_enum_mbus_code,
> + .enum_frame_size = mt9m032_enum_frame_size,
> + .get_fmt = mt9m032_get_pad_format,
> + .set_fmt = mt9m032_set_pad_format,
> + .set_crop = mt9m032_set_crop,
> + .get_crop = mt9m032_get_crop,
> +};
> +
> +static const struct v4l2_subdev_ops mt9m032_ops = {
> + .core = &mt9m032_core_ops,
> + .video = &mt9m032_video_ops,
> + .pad = &mt9m032_pad_ops,
> +};
> +
> +static int mt9m032_probe(struct i2c_client *client,
> + const struct i2c_device_id *devid)
> +{
> + struct mt9m032 *sensor;
> + int chip_version;
> + int res, ret;
> +
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> { + dev_warn(&client->adapter->dev,
> + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> + return -EIO;
> + }
> +
> + if (!client->dev.platform_data)
> + return -ENODEV;
> +
> + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> + if (sensor == NULL)
> + return -ENOMEM;
> +
> + sensor->pdata = client->dev.platform_data;
> +
> + v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
> + sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> +
> +
> + /*
> + * This driver was developed with a camera module with seperate external
> + * pix clock. For setups which use the clock from the camera interface
> + * the code will need to be extended with the appropriate platform
> + * callback to setup the clock.
> + */
> + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> + if (0x1402 == chip_version) {
So I suspect this can also cast fireballs ? Also, why do you use Yoda conditions
here ? Please run checkpatch.pl on this too just to be sure.
> + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> + } else {
> + dev_warn(&client->dev, "mt9m032: error: detected unsupported
chip
> version 0x%x\n", + chip_version);
> + ret = -ENODEV;
> + goto free_sensor;
> + }
> +
> +
> +
> + sensor->frame_interval.numerator = 1;
> + sensor->frame_interval.denominator = 30;
> +
> + sensor->crop.left = 416;
> + sensor->crop.top = 360;
> + sensor->crop.width = 640;
> + sensor->crop.height = 480;
> +
> + sensor->format.width = sensor->crop.width;
> + sensor->format.height = sensor->crop.height;
> + sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
> + sensor->format.field = V4L2_FIELD_NONE;
> + sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
> +
> + v4l2_ctrl_handler_init(&sensor->ctrls, 4);
> +
> + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> + V4L2_CID_GAIN, 0, 127, 1, 64);
> +
> + sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> + V4L2_CID_HFLIP, 0, 1, 1, 0);
> + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> + V4L2_CID_VFLIP, 0, 1, 1, 0);
> + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> + V4L2_CID_EXPOSURE, 0, 8000, 1, 1700); /* 1.7ms */
> +
> +
> + if (sensor->ctrls.error) {
> + ret = sensor->ctrls.error;
> + dev_err(&client->dev, "control initialization error %d\n", ret);
> + goto free_ctrl;
> + }
> +
> + sensor->subdev.ctrl_handler = &sensor->ctrls;
> + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> + ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
> + if (ret < 0)
> + goto free_ctrl;
> +
> + ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1); /* reset on */
> + if (ret < 0)
> + goto free_ctrl;
> + mt9m032_write_reg(sensor, MT9M032_RESET, 0); /* reset off */
> + if (ret < 0)
> + goto free_ctrl;
> +
> + ret = mt9m032_setup_pll(sensor);
> + if (ret < 0)
> + goto free_ctrl;
> + msleep(10);
> +
> + v4l2_ctrl_handler_setup(&sensor->ctrls);
> +
> + /* SIZE */
> + ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
> + if (ret < 0)
> + goto free_ctrl;
> +
> + ret = mt9m032_write_reg(sensor, 0x41, 0x0000); /* reserved !!! */
> + if (ret < 0)
> + goto free_ctrl;
> + ret = mt9m032_write_reg(sensor, 0x42, 0x0003); /* reserved !!! */
> + if (ret < 0)
> + goto free_ctrl;
> + ret = mt9m032_write_reg(sensor, 0x43, 0x0003); /* reserved !!! */
> + if (ret < 0)
> + goto free_ctrl;
> + ret = mt9m032_write_reg(sensor, 0x7f, 0x0000); /* reserved !!! */
Ok, now I feel like reading a grimoire and learing how to cast iceblast. ;-)
> + if (ret < 0)
> + goto free_ctrl;
> + if (sensor->pdata->invert_pixclock) {
> + mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL,
> MT9M032_PIX_CLK_CTRL_INV_PIXCLK); + if (ret < 0)
> + goto free_ctrl;
> + }
> +
> + res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
> +
> + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
> + if (ret < 0)
> + goto free_ctrl;
> + msleep(100);
> + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
> + if (ret < 0)
> + goto free_ctrl;
> + msleep(100);
> + ret = update_formatter2(sensor, false);
> + if (ret < 0)
> + goto free_ctrl;
> +
> + return ret;
> +
> +free_ctrl:
> + v4l2_ctrl_handler_free(&sensor->ctrls);
> +
> +free_sensor:
> + kfree(sensor);
> + return ret;
> +}
> +
> +static int mt9m032_remove(struct i2c_client *client)
> +{
> + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> + struct mt9m032 *sensor = to_mt9m032(subdev);
> +
> + v4l2_device_unregister_subdev(&sensor->subdev);
> + v4l2_ctrl_handler_free(&sensor->ctrls);
> + media_entity_cleanup(&sensor->subdev.entity);
> + kfree(sensor);
> + return 0;
> +}
> +
> +static const struct i2c_device_id mt9m032_id_table[] = {
> + {MT9M032_NAME, 0},
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
> +
> +static struct i2c_driver mt9m032_i2c_driver = {
> + .driver = {
> + .name = MT9M032_NAME,
> + },
> + .probe = mt9m032_probe,
> + .remove = mt9m032_remove,
> + .id_table = mt9m032_id_table,
> +};
> +
> +static int __init mt9m032_init(void)
> +{
> + int rval;
> +
> + rval = i2c_add_driver(&mt9m032_i2c_driver);
> + if (rval)
> + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> +
> + return rval;
> +}
> +
> +static void mt9m032_exit(void)
> +{
> + i2c_del_driver(&mt9m032_i2c_driver);
> +}
> +
> +module_init(mt9m032_init);
> +module_exit(mt9m032_exit);
> +
> +MODULE_AUTHOR("Martin Hostettler");
> +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> new file mode 100644
> index 0000000..94cefc5
> --- /dev/null
> +++ b/include/media/mt9m032.h
> @@ -0,0 +1,38 @@
> +/*
> + * Driver for MT9M032 CMOS Image Sensor from Micron
> + *
> + * Copyright (C) 2010-2011 Lund Engineering
> + * Contact: Gil Lund <gwlund@lundeng.com>
> + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + *
> + */
> +
> +#ifndef MT9M032_H
> +#define MT9M032_H
> +
> +#define MT9M032_NAME "mt9m032"
> +#define MT9M032_I2C_ADDR (0xb8 >> 1)
Pass in platform data, this is likely changable.
> +
> +struct mt9m032_platform_data {
> + u32 ext_clock;
> + u32 pll_pre_div;
> + u32 pll_mul;
> + u32 pll_out_div;
> + int invert_pixclock;
> +
> +};
> +#endif /* MT9M032_H */
Cheers
M
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 1:55 ` Marek Vasut
@ 2011-12-14 7:14 ` martin
2011-12-14 13:49 ` Laurent Pinchart
2011-12-14 21:44 ` Guennadi Liakhovetski
1 sibling, 1 reply; 11+ messages in thread
From: martin @ 2011-12-14 7:14 UTC (permalink / raw)
To: Marek Vasut; +Cc: Laurent Pinchart, Hans Verkuil, linux-media
On Wed, Dec 14, 2011 at 02:55:31AM +0100, Marek Vasut wrote:
> Dear Martin Hostettler,
>
> > The MT9M032 is a parallel 1.6MP sensor from Micron controlled through I2C.
> >
> > The driver creates a V4L2 subdevice. It currently supports cropping, gain,
> > exposure and v/h flipping controls in monochrome mode with an
> > external pixel clock.
> >
> > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > ---
> > drivers/media/video/Kconfig | 7 +
> > drivers/media/video/Makefile | 1 +
> > drivers/media/video/mt9m032.c | 819
> > +++++++++++++++++++++++++++++++++++++++++ include/media/mt9m032.h |
> > 38 ++
> > 4 files changed, 865 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/media/video/mt9m032.c
> > create mode 100644 include/media/mt9m032.h
> >
> > Changes in V3
> > * rebased to current mainline
> > * removed unneeded includes
> > * remove all translation code to hide black or active boundry sensor
> > pixels. Userspace now needs to select crop in original device coordinates.
> > * remove useless consts, casts and defines
> > * changed enum_frame_size to return min == max
> > * removed duplicated input validation
> > * validate crop rectangle on set_crop
> >
> > Changes in V2
> > * ported to current mainline
> > * Moved dispatching for subdev ioctls VIDIOC_DBG_G_REGISTER and
> > VIDIOC_DBG_S_REGISTER into v4l2-subdev * Removed VIDIOC_DBG_G_CHIP_IDENT
> > support
> > * moved header to media/
> > * Fixed missing error handling
> > * lowercase hex constants
> > * moved v4l2_get_subdevdata to register access helpers
> > * use div_u64 instead of do_div
> > * moved all know register values into #define:ed constants
> > * Fixed error reporting, used clamp instead of open coding.
> > * lots of style fixes.
> > * add try_ctrl to make sure user space sees rounded values
> > * Fixed some problem in control framework usage.
> > * Fixed set_format to force width and height setup via crop. Simplyfied
> > code.
> >
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index b303a3f..cf05d9b 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -820,6 +820,13 @@ config SOC_CAMERA_MT9M001
> > This driver supports MT9M001 cameras from Micron, monochrome
> > and colour models.
> >
> > +config VIDEO_MT9M032
> > + tristate "MT9M032 camera sensor support"
> > + depends on I2C && VIDEO_V4L2
> > + help
> > + This driver supports MT9M032 cameras from Micron, monochrome
> > + models only.
> > +
> > config SOC_CAMERA_MT9M111
> > tristate "mt9m111, mt9m112 and mt9m131 support"
> > depends on SOC_CAMERA && I2C
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index 117f9c4..39c7455 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o
> >
> > obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o
> > obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o
> > +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
> > obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o
> > obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
> > obj-$(CONFIG_SOC_CAMERA_MT9T112) += mt9t112.o
> > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> > new file mode 100644
> > index 0000000..b4159c7
> > --- /dev/null
> > +++ b/drivers/media/video/mt9m032.c
> > @@ -0,0 +1,819 @@
> > +/*
> > + * Driver for MT9M032 CMOS Image Sensor from Micron
> > + *
> > + * Copyright (C) 2010-2011 Lund Engineering
> > + * Contact: Gil Lund <gwlund@lundeng.com>
> > + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/v4l2-mediabus.h>
> > +
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include <media/mt9m032.h>
> > +
> > +#define MT9M032_CHIP_VERSION 0x00
> > +#define MT9M032_ROW_START 0x01
> > +#define MT9M032_COLUMN_START 0x02
> > +#define MT9M032_ROW_SIZE 0x03
> > +#define MT9M032_COLUMN_SIZE 0x04
> > +#define MT9M032_HBLANK 0x05
> > +#define MT9M032_VBLANK 0x06
> > +#define MT9M032_SHUTTER_WIDTH_HIGH 0x08
> > +#define MT9M032_SHUTTER_WIDTH_LOW 0x09
> > +#define MT9M032_PIX_CLK_CTRL 0x0a
> > +#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000
> > +#define MT9M032_RESTART 0x0b
> > +#define MT9M032_RESET 0x0d
> > +#define MT9M032_PLL_CONFIG1 0x11
> > +#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
> > +#define MT9M032_PLL_CONFIG1_MUL_SHIFT 8
> > +#define MT9M032_READ_MODE1 0x1e
> > +#define MT9M032_READ_MODE2 0x20
> > +#define MT9M032_READ_MODE2_VFLIP_SHIFT 15
> > +#define MT9M032_READ_MODE2_HFLIP_SHIFT 14
> > +#define MT9M032_READ_MODE2_ROW_BLC 0x40
> > +#define MT9M032_GAIN_GREEN1 0x2b
> > +#define MT9M032_GAIN_BLUE 0x2c
> > +#define MT9M032_GAIN_RED 0x2d
> > +#define MT9M032_GAIN_GREEN2 0x2e
> > +/* write only */
> > +#define MT9M032_GAIN_ALL 0x35
> > +#define MT9M032_GAIN_DIGITAL_MASK 0x7f
> > +#define MT9M032_GAIN_DIGITAL_SHIFT 8
> > +#define MT9M032_GAIN_AMUL_SHIFT 6
> > +#define MT9M032_GAIN_ANALOG_MASK 0x3f
> > +#define MT9M032_FORMATTER1 0x9e
> > +#define MT9M032_FORMATTER2 0x9f
> > +#define MT9M032_FORMATTER2_DOUT_EN 0x1000
> > +#define MT9M032_FORMATTER2_PIXCLK_EN 0x2000
> > +
> > +#define MT9M032_MAX_BLANKING_ROWS 0x7ff
>
> Fix the alignment here please.
#define REGISTER
#define REGISTER_FIELD
I think this is quite readable and with tabwidth 8 everything else is aligned
in one line in the actual code.
>
> > +
> > +
> > +/*
> > + * width and height include active boundry and black parts
> > + *
> > + * column 0- 15 active boundry
> > + * column 16-1455 image
> > + * column 1456-1471 active boundry
> > + * column 1472-1599 black
> > + *
> > + * row 0- 51 black
> > + * row 53- 59 active boundry
> > + * row 60-1139 image
> > + * row 1140-1147 active boundry
> > + * row 1148-1151 black
> > + */
> > +#define MT9M032_WIDTH 1600
> > +#define MT9M032_HEIGHT 1152
> > +#define MT9M032_MINIMALSIZE 32
> > +
> > +#define to_mt9m032(sd) container_of(sd, struct mt9m032, subdev)
> > +#define to_dev(sensor) &((struct i2c_client
> > *)v4l2_get_subdevdata(&sensor->subdev))->dev +
> > +struct mt9m032 {
> > + struct v4l2_subdev subdev;
> > + struct media_pad pad;
> > + struct mt9m032_platform_data *pdata;
> > + struct v4l2_ctrl_handler ctrls;
> > +
> > + bool streaming;
> > +
> > + int pix_clock;
> > +
> > + struct v4l2_mbus_framefmt format; /* height and width always the
> same as
> > in crop */ + struct v4l2_rect crop;
> > + struct v4l2_fract frame_interval;
> > +
> > + struct v4l2_ctrl *hflip, *vflip;
> > +};
> > +
> > +
> > +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > + s32 data = i2c_smbus_read_word_data(client, reg);
> > +
> > + return data < 0 ? data : swab16(data);
>
> Uhm ... why ?
error codes are not swab16-ed, data values are. Hardware needs swapping of
data values.
>
> > +}
> > +
> > +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> > + const u16 data)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + return i2c_smbus_write_word_data(client, reg, swab16(data));
> > +}
> > +
> > +
> > +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
> > +{
> > + int effective_width;
> > + u64 ns;
> > +
> > + effective_width = width + 716; /* emperical value */
> > + ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
>
> Magic number, please fix globally.
I don't see a point in moving SI-bases like 10^9 into constants.
The rest are as magical to me as they are to you. Sadly there's a
lot that just comes from sample bringup sequences or like the 716
that are just results from trying to fit results and expectations.
There's not much i can do to give them a sane name, so i prefer not
to confuse things. (I could use LINE_FUDGE_OFFSET or some such, but
that doesn't really help)
>
> > + dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns);
> > + return ns;
> > +}
> > +
> > +static int mt9m032_update_timing(struct mt9m032 *sensor,
> > + struct v4l2_fract *interval,
> > + const struct v4l2_rect *crop)
> > +{
> > + unsigned long row_time;
> > + int additional_blanking_rows;
> > + int min_blank;
> > +
> > + if (!interval)
> > + interval = &sensor->frame_interval;
> > + if (!crop)
> > + crop = &sensor->crop;
> > +
> > + row_time = mt9m032_row_time(sensor, crop->width);
> > +
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
> > interval->numerator, +
> > ((u64)interval->denominator) * row_time) + -
> > crop->height;
> > +
> > + if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
> > + /* hardware limits to 11 bit values */
> > + interval->denominator = 1000;
> > + interval->numerator = div_u64((crop->height +
> MT9M032_MAX_BLANKING_ROWS)
> > + * ((u64)row_time) * interval-
> >denominator,
> > + 1000000000);
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
>
> 100000<many zeroes>UL or ULL might work better btw?
This works fine.
>
> > interval->numerator, +
> > ((u64)interval->denominator) * row_time) + -
> > crop->height;
> > + }
> > + /* enforce minimal 1.6ms blanking time. */
> > + min_blank = 1600000 / row_time;
> > + additional_blanking_rows = clamp(additional_blanking_rows,
> > + min_blank, MT9M032_MAX_BLANKING_ROWS);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_VBLANK,
> > additional_blanking_rows); +}
> > +
> > +static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
> > + const struct v4l2_rect *crop)
> > +{
> > + int ret;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height -
> 1);
> > + /* offsets compensate for black border */
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop-
> >left);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
> > + if (!ret)
> > + ret = mt9m032_update_timing(sensor, NULL, crop);
> > + return ret;
> > +}
> > +
> > +static int update_formatter2(struct mt9m032 *sensor, bool streaming)
> > +{
> > + u16 reg_val = MT9M032_FORMATTER2_DOUT_EN
> > + | 0x0070; /* parts reserved! */
> > + /* possibly for changing to 14-bit mode */
>
> MAGIC!
Yes, unless i find a better datasheet i can't do anything about this.
>
> > +
> > + if (streaming)
> > + reg_val |= MT9M032_FORMATTER2_PIXCLK_EN; /* pixclock enable */
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
> > +}
> > +
> > +static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + ret = update_formatter2(sensor, streaming);
> > + if (!ret)
> > + sensor->streaming = streaming;
> > + return ret;
> > +}
> > +
> > +static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + if (code->index != 0 || code->pad != 0)
>
> Parenthsis missing ? () || () ?
!= binds stronger that || so this the Parenthsis would be noise.
Anyone in kernel land who really get's confused by this?
>
> > + return -EINVAL;
> > + code->code = V4L2_MBUS_FMT_Y8_1X8;
> > + return 0;
> > +}
> > +
> > +static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad !=
> > 0) + return -EINVAL;
> > +
> > + fse->min_width = MT9M032_WIDTH;
> > + fse->max_width = MT9M032_WIDTH;
> > + fse->min_height = MT9M032_HEIGHT;
> > + fse->max_height = MT9M032_HEIGHT;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_crop() - get crop rect
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try crop rect from
> > + * @which: select try or active crop rect
> > + * Returns a pointer the current active or fh relative try crop rect
> > + */
> > +static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
> > + struct v4l2_subdev_fh *fh,
> > + u32 which)
>
> Do NOT use __function, they might (even though in this case unlikely) be used by
> compiler.
This is the usual coding style. Lots of drivers use this naming convention.
e.g.
grep "int __" drivers/media/video/*.c | wc -l
138
Documentation/CodingStyle says nothing about this, and as it's standard kernel
practice, i believe this is ok.
And it's pretty safe too, "__mt9m032" is a prefix that's unlikely to be used
in a way they would conflict with verbose v4l2 function names.
>
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_crop(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->crop;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_format() - get format
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try format from
> > + * @which: select try or active format
> > + * Returns a pointer the current active or fh relative try format
> > + */
> > +static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032
> > *sensor, + struct
> v4l2_subdev_fh *fh,
> > + u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_format(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->format;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt *format;
> > +
> > + format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
> > +
> > + fmt->format = *format;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > +
> > + /*
> > + * fmt->format.colorspace, fmt->format.code and fmt->format.field are
> > ignored + * and thus forced to fixed values by the get call below.
> > + *
> > + * fmt->format.width, fmt->format.height are forced to the values set
> via
> > crop + */
> > +
> > + return mt9m032_get_pad_format(subdev, fh, fmt);
> > +}
> > +
> > +static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_rect *curcrop;
> > +
> > + curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > +
> > + crop->rect = *curcrop;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt tmp_format;
> > + struct v4l2_rect tmp_crop_rect;
> > + struct v4l2_mbus_framefmt *format;
> > + struct v4l2_rect *crop_rect;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + format = __mt9m032_get_pad_format(sensor, fh, crop->which);
> > + crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + tmp_crop_rect = *crop_rect;
> > + tmp_format = *format;
> > + format = &tmp_format;
> > + crop_rect = &tmp_crop_rect;
> > + }
> > +
> > + crop_rect->top = clamp(crop->rect.top, 0,
> > + MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
> > + crop_rect->left = clamp(crop->rect.left, 0,
> > + MT9M032_WIDTH - MT9M032_MINIMALSIZE);
> > + crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
> > + MT9M032_HEIGHT - crop_rect->top);
> > + crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
> > + MT9M032_WIDTH - crop_rect->left) & ~1;
> > +
> > + format->height = crop_rect->height;
> > + format->width = crop_rect->width;
> > +
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int ret = mt9m032_update_geom_timing(sensor, crop_rect);
> > +
> > + if (!ret) {
> > + sensor->crop = tmp_crop_rect;
> > + sensor->format = tmp_format;
> > + }
> > + return ret;
>
> return ret below and set ret = 0 at the begining
Yes, that would be more consistant.
>
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + fi->pad = 0;
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > + fi->interval = sensor->frame_interval;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > +
> > + ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
> > + if (!ret)
> > + sensor->frame_interval = fi->interval;
> > + return ret;
> > +}
> > +
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +static int mt9m032_g_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > + int val;
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
> > +
> > + val = mt9m032_read_reg(sensor, reg->reg);
> > + if (val < 0)
> > + return -EIO;
> > +
> > + reg->size = 2;
> > + reg->val = val;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_s_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > +
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
> > +
> > + if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > hflip) +{
> > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
>
> !!(bool) ? hmm ...
a true bool can be any value except 0. !!bool is guaranteed to be either
0 or 1 and this suitable for use in bitshifting to set a specific bit.
>
> > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > + | MT9M032_READ_MODE2_ROW_BLC
> > + | 0x0007;
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > +}
> > +
> > +static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, sensor->vflip->cur.val, val);
> > +}
> > +
> > +static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, val, sensor->hflip->cur.val);
> > +}
> > +
> > +static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
> > +{
> > + int shutter_width;
> > + u16 high_val, low_val;
> > + int ret;
> > +
> > + /* shutter width is in row times */
> > + shutter_width = (val * 1000) / mt9m032_row_time(sensor,
> > sensor->crop.width); +
> > + high_val = (shutter_width >> 16) & 0xf;
> > + low_val = shutter_width & 0xffff;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
> > + if (!ret)
> > + mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
>
> You're not checking return value here ?
Yes, i should add that.
>
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > +{
> > + int digital_gain_val; /* in 1/8th (0..127) */
> > + int analog_mul; /* 0 or 1 */
> > + int analog_gain_val; /* in 1/16th. (0..63) */
> > + u16 reg_val;
> > +
> > + digital_gain_val = 51; /* from setup example */
> > +
> > + if (val < 63) {
> > + analog_mul = 0;
> > + analog_gain_val = val;
> > + } else {
> > + analog_mul = 1;
> > + analog_gain_val = val / 2;
> > + }
> > +
> > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > +
> > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > MT9M032_GAIN_AMUL_SHIFT
> > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > +}
> > +
> > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > +{
> > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > + u16 reg_pll1;
> > + unsigned int pre_div;
> > + int res, ret;
> > +
> > + /* TODO: also support other pre-div values */
> > + if (pdata->pll_pre_div != 6) {
> > + dev_warn(to_dev(sensor),
> > + "Unsupported PLL pre-divisor value %u, using default
> 6\n",
> > + pdata->pll_pre_div);
> > + }
> > + pre_div = 6;
> > +
> > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > + (pre_div * pdata->pll_out_div);
> > +
> > + reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> > + | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> clock
> > source */ +
>
> urm ... magic ... black magic ...
yes, indeed. But i don't have any more information.
The datasheet nicely states "reserved" and in the bringup section
"poke in these magical values, please". :/ Not even a register name.
>
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > + /* more reserved,
> Continuous */
> > + /* Master Mode */
> > + if (!ret)
> > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > +
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > + /* Set 14-bit mode, select 7 divider */
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
> > + /* round because of multiplier used for values >= 63 */
> > + ctrl->val &= ~1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032,
> > ctrls); +
> > + switch (ctrl->id) {
> > + case V4L2_CID_GAIN:
> > + return mt9m032_set_gain(sensor, ctrl->val);
> > +
> > + case V4L2_CID_HFLIP:
> > + return mt9m032_set_hflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_VFLIP:
> > + return mt9m032_set_vflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_EXPOSURE:
> > + return mt9m032_set_exposure(sensor, ctrl->val);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
> > + .s_stream = mt9m032_s_stream,
> > + .g_frame_interval = mt9m032_get_frame_interval,
> > + .s_frame_interval = mt9m032_set_frame_interval,
> > +};
> > +
> > +static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
> > + .s_ctrl = mt9m032_set_ctrl,
> > + .try_ctrl = mt9m032_try_ctrl,
> > +};
> > +
> > +
> > +static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > + .g_register = mt9m032_g_register,
> > + .s_register = mt9m032_s_register,
> > +#endif
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
> > + .enum_mbus_code = mt9m032_enum_mbus_code,
> > + .enum_frame_size = mt9m032_enum_frame_size,
> > + .get_fmt = mt9m032_get_pad_format,
> > + .set_fmt = mt9m032_set_pad_format,
> > + .set_crop = mt9m032_set_crop,
> > + .get_crop = mt9m032_get_crop,
> > +};
> > +
> > +static const struct v4l2_subdev_ops mt9m032_ops = {
> > + .core = &mt9m032_core_ops,
> > + .video = &mt9m032_video_ops,
> > + .pad = &mt9m032_pad_ops,
> > +};
> > +
> > +static int mt9m032_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid)
> > +{
> > + struct mt9m032 *sensor;
> > + int chip_version;
> > + int res, ret;
> > +
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > { + dev_warn(&client->adapter->dev,
> > + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > + return -EIO;
> > + }
> > +
> > + if (!client->dev.platform_data)
> > + return -ENODEV;
> > +
> > + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> > + if (sensor == NULL)
> > + return -ENOMEM;
> > +
> > + sensor->pdata = client->dev.platform_data;
> > +
> > + v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
> > + sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +
> > + /*
> > + * This driver was developed with a camera module with seperate external
> > + * pix clock. For setups which use the clock from the camera interface
> > + * the code will need to be extended with the appropriate platform
> > + * callback to setup the clock.
> > + */
> > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > + if (0x1402 == chip_version) {
>
> So I suspect this can also cast fireballs ? Also, why do you use Yoda conditions
> here ? Please run checkpatch.pl on this too just to be sure.
No fireball was visible when looking at the hardware.
>
> > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > + } else {
> > + dev_warn(&client->dev, "mt9m032: error: detected unsupported
> chip
> > version 0x%x\n", + chip_version);
> > + ret = -ENODEV;
> > + goto free_sensor;
> > + }
> > +
> > +
> > +
> > + sensor->frame_interval.numerator = 1;
> > + sensor->frame_interval.denominator = 30;
> > +
> > + sensor->crop.left = 416;
> > + sensor->crop.top = 360;
> > + sensor->crop.width = 640;
> > + sensor->crop.height = 480;
> > +
> > + sensor->format.width = sensor->crop.width;
> > + sensor->format.height = sensor->crop.height;
> > + sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
> > + sensor->format.field = V4L2_FIELD_NONE;
> > + sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
> > +
> > + v4l2_ctrl_handler_init(&sensor->ctrls, 4);
> > +
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_GAIN, 0, 127, 1, 64);
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_EXPOSURE, 0, 8000, 1, 1700); /* 1.7ms */
> > +
> > +
> > + if (sensor->ctrls.error) {
> > + ret = sensor->ctrls.error;
> > + dev_err(&client->dev, "control initialization error %d\n", ret);
> > + goto free_ctrl;
> > + }
> > +
> > + sensor->subdev.ctrl_handler = &sensor->ctrls;
> > + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > + ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1); /* reset on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + mt9m032_write_reg(sensor, MT9M032_RESET, 0); /* reset off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_setup_pll(sensor);
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(10);
> > +
> > + v4l2_ctrl_handler_setup(&sensor->ctrls);
> > +
> > + /* SIZE */
> > + ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, 0x41, 0x0000); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x42, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x43, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x7f, 0x0000); /* reserved !!! */
>
> Ok, now I feel like reading a grimoire and learing how to cast iceblast. ;-)
Just image you had to actually write drivers with the level of undocumented
magic value poking in the offical documentation...
>
> > + if (ret < 0)
> > + goto free_ctrl;
> > + if (sensor->pdata->invert_pixclock) {
> > + mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL,
> > MT9M032_PIX_CLK_CTRL_INV_PIXCLK); + if (ret < 0)
> > + goto free_ctrl;
> > + }
> > +
> > + res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = update_formatter2(sensor, false);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + return ret;
> > +
> > +free_ctrl:
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > +
> > +free_sensor:
> > + kfree(sensor);
> > + return ret;
> > +}
> > +
> > +static int mt9m032_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + v4l2_device_unregister_subdev(&sensor->subdev);
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > + media_entity_cleanup(&sensor->subdev.entity);
> > + kfree(sensor);
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id mt9m032_id_table[] = {
> > + {MT9M032_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
> > +
> > +static struct i2c_driver mt9m032_i2c_driver = {
> > + .driver = {
> > + .name = MT9M032_NAME,
> > + },
> > + .probe = mt9m032_probe,
> > + .remove = mt9m032_remove,
> > + .id_table = mt9m032_id_table,
> > +};
> > +
> > +static int __init mt9m032_init(void)
> > +{
> > + int rval;
> > +
> > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > + if (rval)
> > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > +
> > + return rval;
> > +}
> > +
> > +static void mt9m032_exit(void)
> > +{
> > + i2c_del_driver(&mt9m032_i2c_driver);
> > +}
> > +
> > +module_init(mt9m032_init);
> > +module_exit(mt9m032_exit);
> > +
> > +MODULE_AUTHOR("Martin Hostettler");
> > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> > new file mode 100644
> > index 0000000..94cefc5
> > --- /dev/null
> > +++ b/include/media/mt9m032.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Driver for MT9M032 CMOS Image Sensor from Micron
> > + *
> > + * Copyright (C) 2010-2011 Lund Engineering
> > + * Contact: Gil Lund <gwlund@lundeng.com>
> > + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#ifndef MT9M032_H
> > +#define MT9M032_H
> > +
> > +#define MT9M032_NAME "mt9m032"
> > +#define MT9M032_I2C_ADDR (0xb8 >> 1)
>
> Pass in platform data, this is likely changable.
Not selectable in hardware. The sensor doen't have any known
provision to change the i2c address. I think that they would have
documented id selection pins...
>
> > +
> > +struct mt9m032_platform_data {
> > + u32 ext_clock;
> > + u32 pll_pre_div;
> > + u32 pll_mul;
> > + u32 pll_out_div;
> > + int invert_pixclock;
> > +
> > +};
> > +#endif /* MT9M032_H */
>
> Cheers
> M
- Martin Hostettler
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 7:14 ` martin
@ 2011-12-14 13:49 ` Laurent Pinchart
2011-12-14 18:58 ` martin
0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2011-12-14 13:49 UTC (permalink / raw)
To: martin; +Cc: Marek Vasut, Hans Verkuil, linux-media
Hi Martin,
On Wednesday 14 December 2011 08:14:01 martin@neutronstar.dyndns.org wrote:
> On Wed, Dec 14, 2011 at 02:55:31AM +0100, Marek Vasut wrote:
> > > The MT9M032 is a parallel 1.6MP sensor from Micron controlled through
> > > I2C.
> > >
> > > The driver creates a V4L2 subdevice. It currently supports cropping,
> > > gain, exposure and v/h flipping controls in monochrome mode with an
> > > external pixel clock.
> > >
> > > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > > ---
[snip]
> > > diff --git a/drivers/media/video/mt9m032.c
> > > b/drivers/media/video/mt9m032.c new file mode 100644
> > > index 0000000..b4159c7
> > > --- /dev/null
> > > +++ b/drivers/media/video/mt9m032.c
> > > @@ -0,0 +1,819 @@
[snip]
> > > +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > + s32 data = i2c_smbus_read_word_data(client, reg);
> > > +
> > > + return data < 0 ? data : swab16(data);
> >
> > Uhm ... why ?
>
> error codes are not swab16-ed, data values are. Hardware needs swapping of
> data values.
You can use i2c_smbus_read_word_swapped() and i2c_smbus_write_word_swapped().
> > > +}
> > > +
> > > +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> > > + const u16 data)
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > +
> > > + return i2c_smbus_write_word_data(client, reg, swab16(data));
> > > +}
[snip]
> > > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > > hflip) +{
> > > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
> >
> > !!(bool) ? hmm ...
>
> a true bool can be any value except 0. !!bool is guaranteed to be either
> 0 or 1 and this suitable for use in bitshifting to set a specific bit.
I'd use
int reg_val = (vflip ? MT9M032_READ_MODE2_VFLIP : 0)
| (hflip ? MT9M032_READ_MODE2_HFLIP : 0)
...
but that might just be me.
BTW, what about using fixed-size types for register values ? u16 would make
sense here.
> > > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > > + | MT9M032_READ_MODE2_ROW_BLC
> > > + | 0x0007;
> > > +
> > > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > > +}
[snip]
> > > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > > +{
> > > + int digital_gain_val; /* in 1/8th (0..127) */
> > > + int analog_mul; /* 0 or 1 */
> > > + int analog_gain_val; /* in 1/16th. (0..63) */
> > > + u16 reg_val;
> > > +
> > > + digital_gain_val = 51; /* from setup example */
> > > +
> > > + if (val < 63) {
> > > + analog_mul = 0;
> > > + analog_gain_val = val;
> > > + } else {
> > > + analog_mul = 1;
> > > + analog_gain_val = val / 2;
> > > + }
> > > +
> > > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > > +
> > > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > > MT9M032_GAIN_AMUL_SHIFT
> > > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > > +
> > > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > > +}
Lot's of Aptina sensors have a similar gain setup with two or three stages. Do
you think it would be worth it creating a function that could be shared
between them ?
> > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > +{
> > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > + u16 reg_pll1;
> > > + unsigned int pre_div;
> > > + int res, ret;
> > > +
> > > + /* TODO: also support other pre-div values */
I might already have mentioned this, but wouldn't it be time to work a on real
PLL setup code that compute the pre-divisor, multiplier and output divisor
dynamically from the input and output clock frequencies ?
> > > + if (pdata->pll_pre_div != 6) {
> > > + dev_warn(to_dev(sensor),
> > > + "Unsupported PLL pre-divisor value %u, using default
> >
> > 6\n",
> >
> > > + pdata->pll_pre_div);
> > > + }
> > > + pre_div = 6;
> > > +
> > > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > > + (pre_div * pdata->pll_out_div);
> > > +
> > > + reg_pll1 = ((pdata->pll_out_div - 1) &
> > > MT9M032_PLL_CONFIG1_OUTDIV_MASK) + | pdata->pll_mul <<
> > > MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > > +
> > > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > > + if (!ret)
> > > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> >
> > clock
> >
> > > source */ +
> >
> > urm ... magic ... black magic ...
>
> yes, indeed. But i don't have any more information.
> The datasheet nicely states "reserved" and in the bringup section
> "poke in these magical values, please". :/ Not even a register name.
What version of the datasheet do you have ? According to the MT9P031
datasheet, register 0x10 is called PLL Control and bits 0 and 1 are described
as
1 Use PLL
When set, use the PLL output as the system clock. When clear, use EXTCLK as
the system clock.
0 Power PLL
When set, the PLL is powered. When clear, it is not powered.
The other bits are not documented. The mt9p031 driver has
#define MT9P031_PLL_CONTROL 0x10
#define MT9P031_PLL_CONTROL_PWROFF 0x0050
#define MT9P031_PLL_CONTROL_PWRON 0x0051
#define MT9P031_PLL_CONTROL_USEPLL 0x0052
which you could reuse.
> > > + if (!ret)
> > > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > > + /* more reserved, Continuous */
> > > + /* Master Mode */
> > > + if (!ret)
> > > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > > +
> > > + if (!ret)
> > > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > > + /* Set 14-bit mode, select 7 divider */
> > > +
> > > + return ret;
> > > +}
[snip]
> > > + /*
> > > +* This driver was developed with a camera module with seperate external
> > > +* pix clock. For setups which use the clock from the camera interface
> > > +* the code will need to be extended with the appropriate platform
> > > +* callback to setup the clock.
> > > + */
> > > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > > + if (0x1402 == chip_version) {
> >
> > So I suspect this can also cast fireballs ? Also, why do you use Yoda
> > conditions here ? Please run checkpatch.pl on this too just to be sure.
>
> No fireball was visible when looking at the hardware.
#define MT9M032_CHIP_VERSION_VALUE 0x1402
is an option (not sure if it prevents fireballs though).
> > > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > > + } else {
> > > + dev_warn(&client->dev, "mt9m032: error: detected unsupported
> > chip version 0x%x\n",
> > > + chip_version);
> > > + ret = -ENODEV;
> > > + goto free_sensor;
> > > + }
[snip]
> > > +static int __init mt9m032_init(void)
> > > +{
> > > + int rval;
> > > +
> > > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > > + if (rval)
> > > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > > +
> > > + return rval;
> > > +}
> > > +
> > > +static void mt9m032_exit(void)
> > > +{
> > > + i2c_del_driver(&mt9m032_i2c_driver);
> > > +}
> > > +
> > > +module_init(mt9m032_init);
> > > +module_exit(mt9m032_exit);
v3.3 will have a very handy module_i2c_driver() macro. See
https://lkml.org/lkml/2011/11/17/36
> > > +MODULE_AUTHOR("Martin Hostettler");
> > > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > > +MODULE_LICENSE("GPL v2");
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 13:49 ` Laurent Pinchart
@ 2011-12-14 18:58 ` martin
2011-12-14 22:11 ` Sakari Ailus
2011-12-19 10:43 ` Laurent Pinchart
0 siblings, 2 replies; 11+ messages in thread
From: martin @ 2011-12-14 18:58 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Marek Vasut, Hans Verkuil, linux-media
On Wed, Dec 14, 2011 at 02:49:05PM +0100, Laurent Pinchart wrote:
> Hi Martin,
>
> On Wednesday 14 December 2011 08:14:01 martin@neutronstar.dyndns.org wrote:
> > On Wed, Dec 14, 2011 at 02:55:31AM +0100, Marek Vasut wrote:
> > > > The MT9M032 is a parallel 1.6MP sensor from Micron controlled through
> > > > I2C.
> > > >
> > > > The driver creates a V4L2 subdevice. It currently supports cropping,
> > > > gain, exposure and v/h flipping controls in monochrome mode with an
> > > > external pixel clock.
> > > >
> > > > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > > > ---
>
> [snip]
>
> > > > diff --git a/drivers/media/video/mt9m032.c
> > > > b/drivers/media/video/mt9m032.c new file mode 100644
> > > > index 0000000..b4159c7
> > > > --- /dev/null
> > > > +++ b/drivers/media/video/mt9m032.c
> > > > @@ -0,0 +1,819 @@
>
> [snip]
>
> > > > +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > > + s32 data = i2c_smbus_read_word_data(client, reg);
> > > > +
> > > > + return data < 0 ? data : swab16(data);
> > >
> > > Uhm ... why ?
> >
> > error codes are not swab16-ed, data values are. Hardware needs swapping of
> > data values.
>
> You can use i2c_smbus_read_word_swapped() and i2c_smbus_write_word_swapped().
Yes, i didn't notice that they have been added. Nicer to use them.
>
> > > > +}
> > > > +
> > > > +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> > > > + const u16 data)
> > > > +{
> > > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > > +
> > > > + return i2c_smbus_write_word_data(client, reg, swab16(data));
> > > > +}
>
> [snip]
>
> > > > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > > > hflip) +{
> > > > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
> > >
> > > !!(bool) ? hmm ...
> >
> > a true bool can be any value except 0. !!bool is guaranteed to be either
> > 0 or 1 and this suitable for use in bitshifting to set a specific bit.
>
> I'd use
>
> int reg_val = (vflip ? MT9M032_READ_MODE2_VFLIP : 0)
> | (hflip ? MT9M032_READ_MODE2_HFLIP : 0)
> ...
>
> but that might just be me.
I like shifts more, easy to check in the datasheet.
>
> BTW, what about using fixed-size types for register values ? u16 would make
> sense here.
I don't think i would make a difference here.
>
> > > > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > > > + | MT9M032_READ_MODE2_ROW_BLC
> > > > + | 0x0007;
> > > > +
> > > > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > > > +}
>
> [snip]
>
> > > > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > > > +{
> > > > + int digital_gain_val; /* in 1/8th (0..127) */
> > > > + int analog_mul; /* 0 or 1 */
> > > > + int analog_gain_val; /* in 1/16th. (0..63) */
> > > > + u16 reg_val;
> > > > +
> > > > + digital_gain_val = 51; /* from setup example */
> > > > +
> > > > + if (val < 63) {
> > > > + analog_mul = 0;
> > > > + analog_gain_val = val;
> > > > + } else {
> > > > + analog_mul = 1;
> > > > + analog_gain_val = val / 2;
> > > > + }
> > > > +
> > > > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > > > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > > > +
> > > > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > > > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > > > MT9M032_GAIN_AMUL_SHIFT
> > > > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > > > +
> > > > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > > > +}
>
> Lot's of Aptina sensors have a similar gain setup with two or three stages. Do
> you think it would be worth it creating a function that could be shared
> between them ?
I don't have time to work on such an extension in a meaningful way.
I'm not sure what's the best way to use this kind of staged gain hardware
best anyway.
>
> > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > +{
> > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > + u16 reg_pll1;
> > > > + unsigned int pre_div;
> > > > + int res, ret;
> > > > +
> > > > + /* TODO: also support other pre-div values */
>
> I might already have mentioned this, but wouldn't it be time to work a on real
> PLL setup code that compute the pre-divisor, multiplier and output divisor
> dynamically from the input and output clock frequencies ?
I'm not sure what the implications for quality and stability of such a
generic setup would be. My gut feeling is most users go with known working
hardcoded values.
Also in the datasheet i have access to, this is totally underdocumented.
>
> > > > + if (pdata->pll_pre_div != 6) {
> > > > + dev_warn(to_dev(sensor),
> > > > + "Unsupported PLL pre-divisor value %u, using default
> > >
> > > 6\n",
> > >
> > > > + pdata->pll_pre_div);
> > > > + }
> > > > + pre_div = 6;
> > > > +
> > > > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > > > + (pre_div * pdata->pll_out_div);
> > > > +
> > > > + reg_pll1 = ((pdata->pll_out_div - 1) &
> > > > MT9M032_PLL_CONFIG1_OUTDIV_MASK) + | pdata->pll_mul <<
> > > > MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > > > +
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> > >
> > > clock
> > >
> > > > source */ +
> > >
> > > urm ... magic ... black magic ...
> >
> > yes, indeed. But i don't have any more information.
> > The datasheet nicely states "reserved" and in the bringup section
> > "poke in these magical values, please". :/ Not even a register name.
>
> What version of the datasheet do you have ? According to the MT9P031
Rev. B 10/07 EN
> datasheet, register 0x10 is called PLL Control and bits 0 and 1 are described
> as
>
> 1 Use PLL
> When set, use the PLL output as the system clock. When clear, use EXTCLK as
> the system clock.
> 0 Power PLL
> When set, the PLL is powered. When clear, it is not powered.
>
> The other bits are not documented. The mt9p031 driver has
>
> #define MT9P031_PLL_CONTROL 0x10
> #define MT9P031_PLL_CONTROL_PWROFF 0x0050
> #define MT9P031_PLL_CONTROL_PWRON 0x0051
> #define MT9P031_PLL_CONTROL_USEPLL 0x0052
>
> which you could reuse.
Yes, it seems close enough. But i'm a bit uneasy about mixing from
different datasheets. Would i keep the original names so everybody can see
that it's from a data sheet of a similar but different hardware?
It would be
MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL
then witch resolves to a slightly odd 0x0051 | 0x0052.
>
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > > > + /* more reserved, Continuous */
> > > > + /* Master Mode */
> > > > + if (!ret)
> > > > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > > > +
> > > > + if (!ret)
> > > > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > > > + /* Set 14-bit mode, select 7 divider */
> > > > +
> > > > + return ret;
> > > > +}
>
>
> [snip]
>
> > > > + /*
> > > > +* This driver was developed with a camera module with seperate external
> > > > +* pix clock. For setups which use the clock from the camera interface
> > > > +* the code will need to be extended with the appropriate platform
> > > > +* callback to setup the clock.
> > > > + */
> > > > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > > > + if (0x1402 == chip_version) {
> > >
> > > So I suspect this can also cast fireballs ? Also, why do you use Yoda
> > > conditions here ? Please run checkpatch.pl on this too just to be sure.
> >
> > No fireball was visible when looking at the hardware.
>
> #define MT9M032_CHIP_VERSION_VALUE 0x1402
>
> is an option (not sure if it prevents fireballs though).
I don't think it will prevent any burns or fires.
But i'll add it anyway, it's clear enough.
>
> > > > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > > > + } else {
> > > > + dev_warn(&client->dev, "mt9m032: error: detected unsupported
> > > chip version 0x%x\n",
> > > > + chip_version);
> > > > + ret = -ENODEV;
> > > > + goto free_sensor;
> > > > + }
>
> [snip]
>
> > > > +static int __init mt9m032_init(void)
> > > > +{
> > > > + int rval;
> > > > +
> > > > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > > > + if (rval)
> > > > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > > > +
> > > > + return rval;
> > > > +}
> > > > +
> > > > +static void mt9m032_exit(void)
> > > > +{
> > > > + i2c_del_driver(&mt9m032_i2c_driver);
> > > > +}
> > > > +
> > > > +module_init(mt9m032_init);
> > > > +module_exit(mt9m032_exit);
>
> v3.3 will have a very handy module_i2c_driver() macro. See
> https://lkml.org/lkml/2011/11/17/36
If you think i should do that i can. Looses a bit verboseness in case of
errors, but i don't think it matters.
For me both ways are ok.
>
> > > > +MODULE_AUTHOR("Martin Hostettler");
> > > > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > > > +MODULE_LICENSE("GPL v2");
>
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 1:55 ` Marek Vasut
2011-12-14 7:14 ` martin
@ 2011-12-14 21:44 ` Guennadi Liakhovetski
1 sibling, 0 replies; 11+ messages in thread
From: Guennadi Liakhovetski @ 2011-12-14 21:44 UTC (permalink / raw)
To: Marek Vasut
Cc: Martin Hostettler, Laurent Pinchart, Hans Verkuil,
Linux Media Mailing List
On Wed, 14 Dec 2011, Marek Vasut wrote:
> Dear Martin Hostettler,
>
> > The MT9M032 is a parallel 1.6MP sensor from Micron controlled through I2C.
> >
> > The driver creates a V4L2 subdevice. It currently supports cropping, gain,
> > exposure and v/h flipping controls in monochrome mode with an
> > external pixel clock.
> >
> > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > ---
> > drivers/media/video/Kconfig | 7 +
> > drivers/media/video/Makefile | 1 +
> > drivers/media/video/mt9m032.c | 819
> > +++++++++++++++++++++++++++++++++++++++++ include/media/mt9m032.h |
> > 38 ++
> > 4 files changed, 865 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/media/video/mt9m032.c
> > create mode 100644 include/media/mt9m032.h
> >
> > Changes in V3
> > * rebased to current mainline
> > * removed unneeded includes
> > * remove all translation code to hide black or active boundry sensor
> > pixels. Userspace now needs to select crop in original device coordinates.
> > * remove useless consts, casts and defines
> > * changed enum_frame_size to return min == max
> > * removed duplicated input validation
> > * validate crop rectangle on set_crop
> >
> > Changes in V2
> > * ported to current mainline
> > * Moved dispatching for subdev ioctls VIDIOC_DBG_G_REGISTER and
> > VIDIOC_DBG_S_REGISTER into v4l2-subdev * Removed VIDIOC_DBG_G_CHIP_IDENT
> > support
> > * moved header to media/
> > * Fixed missing error handling
> > * lowercase hex constants
> > * moved v4l2_get_subdevdata to register access helpers
> > * use div_u64 instead of do_div
> > * moved all know register values into #define:ed constants
> > * Fixed error reporting, used clamp instead of open coding.
> > * lots of style fixes.
> > * add try_ctrl to make sure user space sees rounded values
> > * Fixed some problem in control framework usage.
> > * Fixed set_format to force width and height setup via crop. Simplyfied
> > code.
> >
> > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> > index b303a3f..cf05d9b 100644
> > --- a/drivers/media/video/Kconfig
> > +++ b/drivers/media/video/Kconfig
> > @@ -820,6 +820,13 @@ config SOC_CAMERA_MT9M001
> > This driver supports MT9M001 cameras from Micron, monochrome
> > and colour models.
> >
> > +config VIDEO_MT9M032
> > + tristate "MT9M032 camera sensor support"
> > + depends on I2C && VIDEO_V4L2
> > + help
> > + This driver supports MT9M032 cameras from Micron, monochrome
> > + models only.
> > +
> > config SOC_CAMERA_MT9M111
> > tristate "mt9m111, mt9m112 and mt9m131 support"
> > depends on SOC_CAMERA && I2C
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index 117f9c4..39c7455 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -77,6 +77,7 @@ obj-$(CONFIG_VIDEO_ADP1653) += adp1653.o
> >
> > obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o
> > obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o
> > +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
> > obj-$(CONFIG_SOC_CAMERA_MT9M111) += mt9m111.o
> > obj-$(CONFIG_SOC_CAMERA_MT9T031) += mt9t031.o
> > obj-$(CONFIG_SOC_CAMERA_MT9T112) += mt9t112.o
> > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c
> > new file mode 100644
> > index 0000000..b4159c7
> > --- /dev/null
> > +++ b/drivers/media/video/mt9m032.c
> > @@ -0,0 +1,819 @@
> > +/*
> > + * Driver for MT9M032 CMOS Image Sensor from Micron
> > + *
> > + * Copyright (C) 2010-2011 Lund Engineering
> > + * Contact: Gil Lund <gwlund@lundeng.com>
> > + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/v4l2-mediabus.h>
> > +
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include <media/mt9m032.h>
> > +
> > +#define MT9M032_CHIP_VERSION 0x00
> > +#define MT9M032_ROW_START 0x01
> > +#define MT9M032_COLUMN_START 0x02
> > +#define MT9M032_ROW_SIZE 0x03
> > +#define MT9M032_COLUMN_SIZE 0x04
> > +#define MT9M032_HBLANK 0x05
> > +#define MT9M032_VBLANK 0x06
> > +#define MT9M032_SHUTTER_WIDTH_HIGH 0x08
> > +#define MT9M032_SHUTTER_WIDTH_LOW 0x09
> > +#define MT9M032_PIX_CLK_CTRL 0x0a
> > +#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000
> > +#define MT9M032_RESTART 0x0b
> > +#define MT9M032_RESET 0x0d
> > +#define MT9M032_PLL_CONFIG1 0x11
> > +#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
> > +#define MT9M032_PLL_CONFIG1_MUL_SHIFT 8
> > +#define MT9M032_READ_MODE1 0x1e
> > +#define MT9M032_READ_MODE2 0x20
> > +#define MT9M032_READ_MODE2_VFLIP_SHIFT 15
> > +#define MT9M032_READ_MODE2_HFLIP_SHIFT 14
> > +#define MT9M032_READ_MODE2_ROW_BLC 0x40
> > +#define MT9M032_GAIN_GREEN1 0x2b
> > +#define MT9M032_GAIN_BLUE 0x2c
> > +#define MT9M032_GAIN_RED 0x2d
> > +#define MT9M032_GAIN_GREEN2 0x2e
> > +/* write only */
> > +#define MT9M032_GAIN_ALL 0x35
> > +#define MT9M032_GAIN_DIGITAL_MASK 0x7f
> > +#define MT9M032_GAIN_DIGITAL_SHIFT 8
> > +#define MT9M032_GAIN_AMUL_SHIFT 6
> > +#define MT9M032_GAIN_ANALOG_MASK 0x3f
> > +#define MT9M032_FORMATTER1 0x9e
> > +#define MT9M032_FORMATTER2 0x9f
> > +#define MT9M032_FORMATTER2_DOUT_EN 0x1000
> > +#define MT9M032_FORMATTER2_PIXCLK_EN 0x2000
> > +
> > +#define MT9M032_MAX_BLANKING_ROWS 0x7ff
>
> Fix the alignment here please.
>
> > +
> > +
> > +/*
> > + * width and height include active boundry and black parts
> > + *
> > + * column 0- 15 active boundry
> > + * column 16-1455 image
> > + * column 1456-1471 active boundry
> > + * column 1472-1599 black
> > + *
> > + * row 0- 51 black
> > + * row 53- 59 active boundry
> > + * row 60-1139 image
> > + * row 1140-1147 active boundry
> > + * row 1148-1151 black
> > + */
> > +#define MT9M032_WIDTH 1600
> > +#define MT9M032_HEIGHT 1152
> > +#define MT9M032_MINIMALSIZE 32
> > +
> > +#define to_mt9m032(sd) container_of(sd, struct mt9m032, subdev)
> > +#define to_dev(sensor) &((struct i2c_client
> > *)v4l2_get_subdevdata(&sensor->subdev))->dev +
> > +struct mt9m032 {
> > + struct v4l2_subdev subdev;
> > + struct media_pad pad;
> > + struct mt9m032_platform_data *pdata;
> > + struct v4l2_ctrl_handler ctrls;
> > +
> > + bool streaming;
> > +
> > + int pix_clock;
> > +
> > + struct v4l2_mbus_framefmt format; /* height and width always the
> same as
> > in crop */ + struct v4l2_rect crop;
> > + struct v4l2_fract frame_interval;
> > +
> > + struct v4l2_ctrl *hflip, *vflip;
> > +};
> > +
> > +
> > +static int mt9m032_read_reg(struct mt9m032 *sensor, u8 reg)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > + s32 data = i2c_smbus_read_word_data(client, reg);
> > +
> > + return data < 0 ? data : swab16(data);
>
> Uhm ... why ?
Because that's how that function is documented:
* This executes the SMBus "read word" protocol, returning negative errno
* else a 16-bit unsigned "word" received from the device.
A better possibility, however, would be to use the recently added
i2c_smbus_read_word_data_swapped(), but I still don't see it even in next,
so, the above should be fine.
> > +}
> > +
> > +static int mt9m032_write_reg(struct mt9m032 *sensor, u8 reg,
> > + const u16 data)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + return i2c_smbus_write_word_data(client, reg, swab16(data));
> > +}
> > +
> > +
> > +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
> > +{
> > + int effective_width;
> > + u64 ns;
> > +
> > + effective_width = width + 716; /* emperical value */
> > + ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
>
> Magic number, please fix globally.
NSEC_PER_SEC could be used.
> > + dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns);
> > + return ns;
> > +}
> > +
> > +static int mt9m032_update_timing(struct mt9m032 *sensor,
> > + struct v4l2_fract *interval,
> > + const struct v4l2_rect *crop)
> > +{
> > + unsigned long row_time;
> > + int additional_blanking_rows;
> > + int min_blank;
> > +
> > + if (!interval)
> > + interval = &sensor->frame_interval;
> > + if (!crop)
> > + crop = &sensor->crop;
> > +
> > + row_time = mt9m032_row_time(sensor, crop->width);
> > +
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
> > interval->numerator, +
> > ((u64)interval->denominator) * row_time) + -
> > crop->height;
> > +
> > + if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
> > + /* hardware limits to 11 bit values */
> > + interval->denominator = 1000;
> > + interval->numerator = div_u64((crop->height +
> MT9M032_MAX_BLANKING_ROWS)
> > + * ((u64)row_time) * interval-
> >denominator,
> > + 1000000000);
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
>
> 100000<many zeroes>UL or ULL might work better btw?
>
> > interval->numerator, +
> > ((u64)interval->denominator) * row_time) + -
> > crop->height;
> > + }
> > + /* enforce minimal 1.6ms blanking time. */
> > + min_blank = 1600000 / row_time;
> > + additional_blanking_rows = clamp(additional_blanking_rows,
> > + min_blank, MT9M032_MAX_BLANKING_ROWS);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_VBLANK,
> > additional_blanking_rows); +}
> > +
> > +static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
> > + const struct v4l2_rect *crop)
> > +{
> > + int ret;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height -
> 1);
> > + /* offsets compensate for black border */
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop-
> >left);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top);
> > + if (!ret)
> > + ret = mt9m032_update_timing(sensor, NULL, crop);
> > + return ret;
> > +}
> > +
> > +static int update_formatter2(struct mt9m032 *sensor, bool streaming)
> > +{
> > + u16 reg_val = MT9M032_FORMATTER2_DOUT_EN
> > + | 0x0070; /* parts reserved! */
> > + /* possibly for changing to 14-bit mode */
>
> MAGIC!
>
> > +
> > + if (streaming)
> > + reg_val |= MT9M032_FORMATTER2_PIXCLK_EN; /* pixclock enable */
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
> > +}
> > +
> > +static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + ret = update_formatter2(sensor, streaming);
> > + if (!ret)
> > + sensor->streaming = streaming;
> > + return ret;
> > +}
> > +
> > +static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + if (code->index != 0 || code->pad != 0)
>
> Parenthsis missing ? () || () ?
No! The above is perfectly fine.
> > + return -EINVAL;
> > + code->code = V4L2_MBUS_FMT_Y8_1X8;
> > + return 0;
> > +}
> > +
> > +static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad !=
> > 0) + return -EINVAL;
> > +
> > + fse->min_width = MT9M032_WIDTH;
> > + fse->max_width = MT9M032_WIDTH;
> > + fse->min_height = MT9M032_HEIGHT;
> > + fse->max_height = MT9M032_HEIGHT;
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_crop() - get crop rect
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try crop rect from
> > + * @which: select try or active crop rect
> > + * Returns a pointer the current active or fh relative try crop rect
> > + */
> > +static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
> > + struct v4l2_subdev_fh *fh,
> > + u32 which)
>
> Do NOT use __function, they might (even though in this case unlikely) be used by
> compiler.
Don't see a problem with that, should be ok to keep.
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_crop(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->crop;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_format() - get format
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try format from
> > + * @which: select try or active format
> > + * Returns a pointer the current active or fh relative try format
> > + */
> > +static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032
> > *sensor, + struct
> v4l2_subdev_fh *fh,
> > + u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_format(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->format;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt *format;
> > +
> > + format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
> > +
> > + fmt->format = *format;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > +
> > + /*
> > + * fmt->format.colorspace, fmt->format.code and fmt->format.field are
> > ignored + * and thus forced to fixed values by the get call below.
> > + *
> > + * fmt->format.width, fmt->format.height are forced to the values set
> via
> > crop + */
> > +
> > + return mt9m032_get_pad_format(subdev, fh, fmt);
> > +}
> > +
> > +static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_rect *curcrop;
> > +
> > + curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > +
> > + crop->rect = *curcrop;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt tmp_format;
> > + struct v4l2_rect tmp_crop_rect;
> > + struct v4l2_mbus_framefmt *format;
> > + struct v4l2_rect *crop_rect;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + format = __mt9m032_get_pad_format(sensor, fh, crop->which);
> > + crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + tmp_crop_rect = *crop_rect;
> > + tmp_format = *format;
> > + format = &tmp_format;
> > + crop_rect = &tmp_crop_rect;
> > + }
> > +
> > + crop_rect->top = clamp(crop->rect.top, 0,
> > + MT9M032_HEIGHT - MT9M032_MINIMALSIZE) & ~1;
> > + crop_rect->left = clamp(crop->rect.left, 0,
> > + MT9M032_WIDTH - MT9M032_MINIMALSIZE);
> > + crop_rect->height = clamp(crop->rect.height, MT9M032_MINIMALSIZE,
> > + MT9M032_HEIGHT - crop_rect->top);
> > + crop_rect->width = clamp(crop->rect.width, MT9M032_MINIMALSIZE,
> > + MT9M032_WIDTH - crop_rect->left) & ~1;
> > +
> > + format->height = crop_rect->height;
> > + format->width = crop_rect->width;
> > +
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int ret = mt9m032_update_geom_timing(sensor, crop_rect);
> > +
> > + if (!ret) {
> > + sensor->crop = tmp_crop_rect;
> > + sensor->format = tmp_format;
> > + }
> > + return ret;
>
> return ret below and set ret = 0 at the begining
Why? Fon't see a reason for this.
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + fi->pad = 0;
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > + fi->interval = sensor->frame_interval;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > +
> > + ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
> > + if (!ret)
> > + sensor->frame_interval = fi->interval;
> > + return ret;
> > +}
> > +
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +static int mt9m032_g_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > + int val;
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
> > +
> > + val = mt9m032_read_reg(sensor, reg->reg);
> > + if (val < 0)
> > + return -EIO;
> > +
> > + reg->size = 2;
> > + reg->val = val;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_s_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > +
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
> > +
> > + if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > hflip) +{
> > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
>
> !!(bool) ? hmm ...
>
> > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > + | MT9M032_READ_MODE2_ROW_BLC
> > + | 0x0007;
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > +}
> > +
> > +static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, sensor->vflip->cur.val, val);
> > +}
> > +
> > +static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, val, sensor->hflip->cur.val);
> > +}
> > +
> > +static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
> > +{
> > + int shutter_width;
> > + u16 high_val, low_val;
> > + int ret;
> > +
> > + /* shutter width is in row times */
> > + shutter_width = (val * 1000) / mt9m032_row_time(sensor,
> > sensor->crop.width); +
> > + high_val = (shutter_width >> 16) & 0xf;
> > + low_val = shutter_width & 0xffff;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
> > + if (!ret)
> > + mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
>
> You're not checking return value here ?
>
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > +{
> > + int digital_gain_val; /* in 1/8th (0..127) */
> > + int analog_mul; /* 0 or 1 */
> > + int analog_gain_val; /* in 1/16th. (0..63) */
> > + u16 reg_val;
> > +
> > + digital_gain_val = 51; /* from setup example */
> > +
> > + if (val < 63) {
> > + analog_mul = 0;
> > + analog_gain_val = val;
> > + } else {
> > + analog_mul = 1;
> > + analog_gain_val = val / 2;
> > + }
> > +
> > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > +
> > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > MT9M032_GAIN_AMUL_SHIFT
> > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > +}
> > +
> > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > +{
> > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > + u16 reg_pll1;
> > + unsigned int pre_div;
> > + int res, ret;
> > +
> > + /* TODO: also support other pre-div values */
> > + if (pdata->pll_pre_div != 6) {
> > + dev_warn(to_dev(sensor),
> > + "Unsupported PLL pre-divisor value %u, using default
> 6\n",
> > + pdata->pll_pre_div);
> > + }
> > + pre_div = 6;
> > +
> > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > + (pre_div * pdata->pll_out_div);
> > +
> > + reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> > + | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> clock
> > source */ +
>
> urm ... magic ... black magic ...
Commented constants are no worse than named macros IMHO.
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > + /* more reserved,
> Continuous */
> > + /* Master Mode */
> > + if (!ret)
> > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > +
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > + /* Set 14-bit mode, select 7 divider */
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
> > + /* round because of multiplier used for values >= 63 */
> > + ctrl->val &= ~1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032,
> > ctrls); +
> > + switch (ctrl->id) {
> > + case V4L2_CID_GAIN:
> > + return mt9m032_set_gain(sensor, ctrl->val);
> > +
> > + case V4L2_CID_HFLIP:
> > + return mt9m032_set_hflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_VFLIP:
> > + return mt9m032_set_vflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_EXPOSURE:
> > + return mt9m032_set_exposure(sensor, ctrl->val);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
> > + .s_stream = mt9m032_s_stream,
> > + .g_frame_interval = mt9m032_get_frame_interval,
> > + .s_frame_interval = mt9m032_set_frame_interval,
> > +};
> > +
> > +static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
> > + .s_ctrl = mt9m032_set_ctrl,
> > + .try_ctrl = mt9m032_try_ctrl,
> > +};
> > +
> > +
> > +static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > + .g_register = mt9m032_g_register,
> > + .s_register = mt9m032_s_register,
> > +#endif
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
> > + .enum_mbus_code = mt9m032_enum_mbus_code,
> > + .enum_frame_size = mt9m032_enum_frame_size,
> > + .get_fmt = mt9m032_get_pad_format,
> > + .set_fmt = mt9m032_set_pad_format,
> > + .set_crop = mt9m032_set_crop,
> > + .get_crop = mt9m032_get_crop,
> > +};
> > +
> > +static const struct v4l2_subdev_ops mt9m032_ops = {
> > + .core = &mt9m032_core_ops,
> > + .video = &mt9m032_video_ops,
> > + .pad = &mt9m032_pad_ops,
> > +};
> > +
> > +static int mt9m032_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid)
> > +{
> > + struct mt9m032 *sensor;
> > + int chip_version;
> > + int res, ret;
> > +
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > { + dev_warn(&client->adapter->dev,
> > + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > + return -EIO;
> > + }
> > +
> > + if (!client->dev.platform_data)
> > + return -ENODEV;
> > +
> > + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> > + if (sensor == NULL)
> > + return -ENOMEM;
> > +
> > + sensor->pdata = client->dev.platform_data;
> > +
> > + v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
> > + sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +
> > + /*
> > + * This driver was developed with a camera module with seperate external
> > + * pix clock. For setups which use the clock from the camera interface
> > + * the code will need to be extended with the appropriate platform
> > + * callback to setup the clock.
> > + */
> > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > + if (0x1402 == chip_version) {
>
> So I suspect this can also cast fireballs ? Also, why do you use Yoda conditions
> here ? Please run checkpatch.pl on this too just to be sure.
Not sure what is the problem, that this comment is trying to describe.
> > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > + } else {
> > + dev_warn(&client->dev, "mt9m032: error: detected unsupported
> chip
> > version 0x%x\n", + chip_version);
> > + ret = -ENODEV;
> > + goto free_sensor;
> > + }
> > +
> > +
> > +
> > + sensor->frame_interval.numerator = 1;
> > + sensor->frame_interval.denominator = 30;
> > +
> > + sensor->crop.left = 416;
> > + sensor->crop.top = 360;
> > + sensor->crop.width = 640;
> > + sensor->crop.height = 480;
> > +
> > + sensor->format.width = sensor->crop.width;
> > + sensor->format.height = sensor->crop.height;
> > + sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
> > + sensor->format.field = V4L2_FIELD_NONE;
> > + sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
> > +
> > + v4l2_ctrl_handler_init(&sensor->ctrls, 4);
> > +
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_GAIN, 0, 127, 1, 64);
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_EXPOSURE, 0, 8000, 1, 1700); /* 1.7ms */
> > +
> > +
> > + if (sensor->ctrls.error) {
> > + ret = sensor->ctrls.error;
> > + dev_err(&client->dev, "control initialization error %d\n", ret);
> > + goto free_ctrl;
> > + }
> > +
> > + sensor->subdev.ctrl_handler = &sensor->ctrls;
> > + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > + ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1); /* reset on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + mt9m032_write_reg(sensor, MT9M032_RESET, 0); /* reset off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_setup_pll(sensor);
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(10);
> > +
> > + v4l2_ctrl_handler_setup(&sensor->ctrls);
> > +
> > + /* SIZE */
> > + ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, 0x41, 0x0000); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x42, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x43, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x7f, 0x0000); /* reserved !!! */
>
> Ok, now I feel like reading a grimoire and learing how to cast iceblast. ;-)
No need, it suffices to write a couple of drivers for undocumented
hardware, where you only have to rely on some examples from the vendor.
> > + if (ret < 0)
> > + goto free_ctrl;
> > + if (sensor->pdata->invert_pixclock) {
> > + mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL,
> > MT9M032_PIX_CLK_CTRL_INV_PIXCLK); + if (ret < 0)
> > + goto free_ctrl;
> > + }
> > +
> > + res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = update_formatter2(sensor, false);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + return ret;
> > +
> > +free_ctrl:
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > +
> > +free_sensor:
> > + kfree(sensor);
> > + return ret;
> > +}
> > +
> > +static int mt9m032_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + v4l2_device_unregister_subdev(&sensor->subdev);
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > + media_entity_cleanup(&sensor->subdev.entity);
> > + kfree(sensor);
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id mt9m032_id_table[] = {
> > + {MT9M032_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
> > +
> > +static struct i2c_driver mt9m032_i2c_driver = {
> > + .driver = {
> > + .name = MT9M032_NAME,
> > + },
> > + .probe = mt9m032_probe,
> > + .remove = mt9m032_remove,
> > + .id_table = mt9m032_id_table,
> > +};
> > +
> > +static int __init mt9m032_init(void)
> > +{
> > + int rval;
> > +
> > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > + if (rval)
> > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > +
> > + return rval;
> > +}
> > +
> > +static void mt9m032_exit(void)
> > +{
> > + i2c_del_driver(&mt9m032_i2c_driver);
> > +}
> > +
> > +module_init(mt9m032_init);
> > +module_exit(mt9m032_exit);
> > +
> > +MODULE_AUTHOR("Martin Hostettler");
> > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> > new file mode 100644
> > index 0000000..94cefc5
> > --- /dev/null
> > +++ b/include/media/mt9m032.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Driver for MT9M032 CMOS Image Sensor from Micron
> > + *
> > + * Copyright (C) 2010-2011 Lund Engineering
> > + * Contact: Gil Lund <gwlund@lundeng.com>
> > + * Author: Martin Hostettler <martin@neutronstar.dyndns.org>
> > + *
> > + * 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.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#ifndef MT9M032_H
> > +#define MT9M032_H
> > +
> > +#define MT9M032_NAME "mt9m032"
> > +#define MT9M032_I2C_ADDR (0xb8 >> 1)
>
> Pass in platform data, this is likely changable.
I would just drop this. It is unused.
> > +
> > +struct mt9m032_platform_data {
> > + u32 ext_clock;
> > + u32 pll_pre_div;
> > + u32 pll_mul;
> > + u32 pll_out_div;
> > + int invert_pixclock;
> > +
> > +};
> > +#endif /* MT9M032_H */
>
> Cheers
> M
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 18:58 ` martin
@ 2011-12-14 22:11 ` Sakari Ailus
2011-12-17 9:50 ` martin
2011-12-19 10:43 ` Laurent Pinchart
1 sibling, 1 reply; 11+ messages in thread
From: Sakari Ailus @ 2011-12-14 22:11 UTC (permalink / raw)
To: martin; +Cc: Laurent Pinchart, Marek Vasut, Hans Verkuil, linux-media
Hi Martin,
On Wed, Dec 14, 2011 at 07:58:45PM +0100, martin@neutronstar.dyndns.org wrote:
...
> > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > +{
> > > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > + u16 reg_pll1;
> > > > > + unsigned int pre_div;
> > > > > + int res, ret;
> > > > > +
> > > > > + /* TODO: also support other pre-div values */
> >
> > I might already have mentioned this, but wouldn't it be time to work a on real
> > PLL setup code that compute the pre-divisor, multiplier and output divisor
> > dynamically from the input and output clock frequencies ?
>
> I'm not sure what the implications for quality and stability of such a
> generic setup would be. My gut feeling is most users go with known working
> hardcoded values.
You'd get a lot better control of the sensor as a bonus in doing so. Also,
you could program the sensor properly suitable for the host it is connected
to, achieving optimal maximum frame rates for it.
These values tend to be relatively board / bridge dependent. On one board
some frequencies might not be usable even if they do not exceed the maximum
for the bridge.
Please also see this:
<URL:http://www.mail-archive.com/linux-media@vger.kernel.org/msg39798.html>
> Also in the datasheet i have access to, this is totally underdocumented.
That's unfortunate. Laurent, is yours the same?
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 22:11 ` Sakari Ailus
@ 2011-12-17 9:50 ` martin
2011-12-19 10:47 ` Laurent Pinchart
2011-12-19 21:43 ` Sakari Ailus
0 siblings, 2 replies; 11+ messages in thread
From: martin @ 2011-12-17 9:50 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Laurent Pinchart, Marek Vasut, Hans Verkuil, linux-media
On Thu, Dec 15, 2011 at 12:11:13AM +0200, Sakari Ailus wrote:
> Hi Martin,
>
> On Wed, Dec 14, 2011 at 07:58:45PM +0100, martin@neutronstar.dyndns.org wrote:
> ...
> > > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > > +{
> > > > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > > + u16 reg_pll1;
> > > > > > + unsigned int pre_div;
> > > > > > + int res, ret;
> > > > > > +
> > > > > > + /* TODO: also support other pre-div values */
> > >
> > > I might already have mentioned this, but wouldn't it be time to work a on real
> > > PLL setup code that compute the pre-divisor, multiplier and output divisor
> > > dynamically from the input and output clock frequencies ?
> >
> > I'm not sure what the implications for quality and stability of such a
> > generic setup would be. My gut feeling is most users go with known working
> > hardcoded values.
>
> You'd get a lot better control of the sensor as a bonus in doing so. Also,
> you could program the sensor properly suitable for the host it is connected
> to, achieving optimal maximum frame rates for it.
>
> These values tend to be relatively board / bridge dependent. On one board
> some frequencies might not be usable even if they do not exceed the maximum
> for the bridge.
Yes, that's why i have exported almost all of the pll details i'm reasonanly sure
that they are settable in the platform data. I think this gives maximum
flexibility in the board configuration. Maybe something with less values to fiddle
with in the normal case would be better. But not really by a large amount.
static struct mt9m032_platform_data mt9m032_platform_data = {
.ext_clock = 13500000,
.pll_pre_div = 6,
.pll_mul = 120,
.pll_out_div = 5,
.invert_pixclock = 1,
};
I think what Laurent was talking about was moving to a setup where the board
doesn't need to specify the exact details. i.e. have pll_pre_div, pll_mul and
pll_out_div rolled into one. I'm not sure that's something that should be done.
And this driver does feel like it had it's share of tracking in development
interfaces.
>
> Please also see this:
>
> <URL:http://www.mail-archive.com/linux-media@vger.kernel.org/msg39798.html>
Sounds sensible for the future. But let's not hold this driver until agreement is
reached about the details?
- Martin
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-14 18:58 ` martin
2011-12-14 22:11 ` Sakari Ailus
@ 2011-12-19 10:43 ` Laurent Pinchart
1 sibling, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2011-12-19 10:43 UTC (permalink / raw)
To: martin; +Cc: Marek Vasut, Hans Verkuil, linux-media
Hi Martin,
On Wednesday 14 December 2011 19:58:45 Martin Hostettler wrote:
> On Wed, Dec 14, 2011 at 02:49:05PM +0100, Laurent Pinchart wrote:
> > On Wednesday 14 December 2011 08:14:01 Martin Hostettler wrote:
> > > On Wed, Dec 14, 2011 at 02:55:31AM +0100, Marek Vasut wrote:
> > > > > The MT9M032 is a parallel 1.6MP sensor from Micron controlled
> > > > > through I2C.
> > > > >
> > > > > The driver creates a V4L2 subdevice. It currently supports
> > > > > cropping, gain, exposure and v/h flipping controls in monochrome
> > > > > mode with an external pixel clock.
> > > > >
> > > > > Signed-off-by: Martin Hostettler <martin@neutronstar.dyndns.org>
> > > > > ---
[snip]
> > > > > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip,
> > > > > bool hflip) +{
> > > > > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
> > > >
> > > > !!(bool) ? hmm ...
> > >
> > > a true bool can be any value except 0. !!bool is guaranteed to be
> > > either 0 or 1 and this suitable for use in bitshifting to set a
> > > specific bit.
> >
> > I'd use
> >
> > int reg_val = (vflip ? MT9M032_READ_MODE2_VFLIP : 0)
> >
> > | (hflip ? MT9M032_READ_MODE2_HFLIP : 0)
> >
> > ...
> >
> > but that might just be me.
>
> I like shifts more, easy to check in the datasheet.
That's a matter of taste I guess. I personnally find the second option more
readable.
> > BTW, what about using fixed-size types for register values ? u16 would
> > make sense here.
>
> I don't think i would make a difference here.
It won't make much of a difference, but it makes the code clearer in my
opinion (otherwise I wouldn't have proposed it :-)).
> > > > > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > > > > + | MT9M032_READ_MODE2_ROW_BLC
> > > > > + | 0x0007;
> > > > > +
> > > > > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > > > > +}
[snip]
> > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > +{
> > > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > + u16 reg_pll1;
> > > > > + unsigned int pre_div;
> > > > > + int res, ret;
> > > > > +
> > > > > + /* TODO: also support other pre-div values */
> >
> > I might already have mentioned this, but wouldn't it be time to work a on
> > real PLL setup code that compute the pre-divisor, multiplier and output
> > divisor dynamically from the input and output clock frequencies ?
>
> I'm not sure what the implications for quality and stability of such a
> generic setup would be. My gut feeling is most users go with known working
> hardcoded values.
That won't be future-proof. We will need DT bindings in the near future, and
PLL parameters won't be there. That's one of the reasons why they should be
computed automatically.
Regarding quality and stability, let's just write code that produces PLL
parameters values identical to the ones you would otherwise hardcode manually
:-)
> Also in the datasheet i have access to, this is totally underdocumented.
I'm pretty sure the MT9P031 documentation applies here.
> > > > > + if (pdata->pll_pre_div != 6) {
> > > > > + dev_warn(to_dev(sensor),
> > > > > + "Unsupported PLL pre-divisor value %u, using default
> > > >
> > > > 6\n",
> > > >
> > > > > + pdata->pll_pre_div);
> > > > > + }
> > > > > + pre_div = 6;
> > > > > +
> > > > > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > > > > + (pre_div * pdata->pll_out_div);
> > > > > +
> > > > > + reg_pll1 = ((pdata->pll_out_div - 1) &
> > > > > MT9M032_PLL_CONFIG1_OUTDIV_MASK) + | pdata->pll_mul <<
> > > > > MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > > > > +
> > > > > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > > > > + if (!ret)
> > > > > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as
> > > >
> > > > clock
> > > >
> > > > > source */ +
> > > >
> > > > urm ... magic ... black magic ...
> > >
> > > yes, indeed. But i don't have any more information.
> > > The datasheet nicely states "reserved" and in the bringup section
> > > "poke in these magical values, please". :/ Not even a register name.
> >
> > What version of the datasheet do you have ? According to the MT9P031
>
> Rev. B 10/07 EN
I'll see if I can get something newer.
> > datasheet, register 0x10 is called PLL Control and bits 0 and 1 are
> > described as
> >
> > 1 Use PLL
> > When set, use the PLL output as the system clock. When clear, use EXTCLK
> > as the system clock.
> > 0 Power PLL
> > When set, the PLL is powered. When clear, it is not powered.
> >
> > The other bits are not documented. The mt9p031 driver has
> >
> > #define MT9P031_PLL_CONTROL 0x10
> > #define MT9P031_PLL_CONTROL_PWROFF 0x0050
> > #define MT9P031_PLL_CONTROL_PWRON 0x0051
> > #define MT9P031_PLL_CONTROL_USEPLL 0x0052
> >
> > which you could reuse.
>
> Yes, it seems close enough. But i'm a bit uneasy about mixing from
> different datasheets. Would i keep the original names so everybody can see
> that it's from a data sheet of a similar but different hardware?
I'd use MT9M032, with a comment right above the definitions stating that they
come from the MT9P031 documentation.
> It would be
> MT9P031_PLL_CONTROL_PWRON | MT9P031_PLL_CONTROL_USEPLL
> then witch resolves to a slightly odd 0x0051 | 0x0052.
Yes it's a bit odd, but that's the best I've been able to find :-)
> > > > > + if (!ret)
> > > > > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > > > > + /* more reserved, Continuous */
> > > > > + /* Master Mode */
> > > > > + if (!ret)
> > > > > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > > > > +
> > > > > + if (!ret)
> > > > > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > > > > + /* Set 14-bit mode, select 7 divider */
> > > > > +
> > > > > + return ret;
> > > > > +}
[snip]
> > > > > +static int __init mt9m032_init(void)
> > > > > +{
> > > > > + int rval;
> > > > > +
> > > > > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > > > > + if (rval)
> > > > > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > > > > +
> > > > > + return rval;
> > > > > +}
> > > > > +
> > > > > +static void mt9m032_exit(void)
> > > > > +{
> > > > > + i2c_del_driver(&mt9m032_i2c_driver);
> > > > > +}
> > > > > +
> > > > > +module_init(mt9m032_init);
> > > > > +module_exit(mt9m032_exit);
> >
> > v3.3 will have a very handy module_i2c_driver() macro. See
> > https://lkml.org/lkml/2011/11/17/36
>
> If you think i should do that i can. Looses a bit verboseness in case of
> errors, but i don't think it matters.
>
> For me both ways are ok.
Many drivers have been moved to module_*_driver(), I think that's the
preferred way.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-17 9:50 ` martin
@ 2011-12-19 10:47 ` Laurent Pinchart
2011-12-19 21:43 ` Sakari Ailus
1 sibling, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2011-12-19 10:47 UTC (permalink / raw)
To: martin; +Cc: Sakari Ailus, Marek Vasut, Hans Verkuil, linux-media
Hi Martin,
On Saturday 17 December 2011 10:50:51 martin@neutronstar.dyndns.org wrote:
> On Thu, Dec 15, 2011 at 12:11:13AM +0200, Sakari Ailus wrote:
> > On Wed, Dec 14, 2011 at 07:58:45PM +0100, martin@neutronstar.dyndns.org
> > wrote: ...
> >
> > > > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > > > +{
> > > > > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > > > + u16 reg_pll1;
> > > > > > > + unsigned int pre_div;
> > > > > > > + int res, ret;
> > > > > > > +
> > > > > > > + /* TODO: also support other pre-div values */
> > > >
> > > > I might already have mentioned this, but wouldn't it be time to work
> > > > a on real PLL setup code that compute the pre-divisor, multiplier
> > > > and output divisor dynamically from the input and output clock
> > > > frequencies ?
> > >
> > > I'm not sure what the implications for quality and stability of such a
> > > generic setup would be. My gut feeling is most users go with known
> > > working hardcoded values.
> >
> > You'd get a lot better control of the sensor as a bonus in doing so.
> > Also, you could program the sensor properly suitable for the host it is
> > connected to, achieving optimal maximum frame rates for it.
> >
> > These values tend to be relatively board / bridge dependent. On one board
> > some frequencies might not be usable even if they do not exceed the
> > maximum for the bridge.
>
> Yes, that's why i have exported almost all of the pll details i'm
> reasonanly sure that they are settable in the platform data. I think this
> gives maximum flexibility in the board configuration. Maybe something with
> less values to fiddle with in the normal case would be better. But not
> really by a large amount.
>
> static struct mt9m032_platform_data mt9m032_platform_data = {
> .ext_clock = 13500000,
> .pll_pre_div = 6,
> .pll_mul = 120,
> .pll_out_div = 5,
> .invert_pixclock = 1,
> };
>
> I think what Laurent was talking about was moving to a setup where the
> board doesn't need to specify the exact details. i.e. have pll_pre_div,
> pll_mul and pll_out_div rolled into one. I'm not sure that's something
> that should be done.
That's what I was referring to, yes. The idea would be to pass the input
(external) clock frequency to the driver, as well as the desired pixel clock.
Both are properties of the board hardware, so they will be included in DT
bindings. The PLL parameters are not properties of the board hardware, so they
should be computed automatically.
> And this driver does feel like it had it's share of tracking in development
> interfaces.
>
> > Please also see this:
> >
> > <URL:http://www.mail-archive.com/linux-media@vger.kernel.org/msg39798.htm
> > l>
>
> Sounds sensible for the future. But let's not hold this driver until
> agreement is reached about the details?
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor
2011-12-17 9:50 ` martin
2011-12-19 10:47 ` Laurent Pinchart
@ 2011-12-19 21:43 ` Sakari Ailus
1 sibling, 0 replies; 11+ messages in thread
From: Sakari Ailus @ 2011-12-19 21:43 UTC (permalink / raw)
To: martin; +Cc: Laurent Pinchart, Marek Vasut, Hans Verkuil, linux-media
Hi Martin,
On Sat, Dec 17, 2011 at 10:50:51AM +0100, martin@neutronstar.dyndns.org wrote:
> On Thu, Dec 15, 2011 at 12:11:13AM +0200, Sakari Ailus wrote:
> > Hi Martin,
> >
> > On Wed, Dec 14, 2011 at 07:58:45PM +0100, martin@neutronstar.dyndns.org wrote:
> > ...
> > > > > > > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > > > > > > +{
> > > > > > > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > > > > > > + u16 reg_pll1;
> > > > > > > + unsigned int pre_div;
> > > > > > > + int res, ret;
> > > > > > > +
> > > > > > > + /* TODO: also support other pre-div values */
> > > >
> > > > I might already have mentioned this, but wouldn't it be time to work a on real
> > > > PLL setup code that compute the pre-divisor, multiplier and output divisor
> > > > dynamically from the input and output clock frequencies ?
> > >
> > > I'm not sure what the implications for quality and stability of such a
> > > generic setup would be. My gut feeling is most users go with known working
> > > hardcoded values.
> >
> > You'd get a lot better control of the sensor as a bonus in doing so. Also,
> > you could program the sensor properly suitable for the host it is connected
> > to, achieving optimal maximum frame rates for it.
> >
> > These values tend to be relatively board / bridge dependent. On one board
> > some frequencies might not be usable even if they do not exceed the maximum
> > for the bridge.
>
> Yes, that's why i have exported almost all of the pll details i'm reasonanly sure
> that they are settable in the platform data. I think this gives maximum
> flexibility in the board configuration. Maybe something with less values to fiddle
> with in the normal case would be better. But not really by a large amount.
I originally thought it was part of the driver; that's how it is in most of
the drivers currently. But these values also depend on the use cases, how
you want to use the sensor essentially.
A concrete example is the OMAP 3 ISP. The CSI-2 receiver's speed is 200 Mp/s
while the rest of the ISP is only 100 Mp/s. Capturing a high-resolution
still photo is best done by reading the sensor's pixel array as fast as
possible --- 200 Mp/s.
Such images must be then processed through the ISP from memory to memory.
> static struct mt9m032_platform_data mt9m032_platform_data = {
> .ext_clock = 13500000,
> .pll_pre_div = 6,
> .pll_mul = 120,
> .pll_out_div = 5,
> .invert_pixclock = 1,
> };
>
> I think what Laurent was talking about was moving to a setup where the
> board doesn't need to specify the exact details. i.e. have pll_pre_div,
> pll_mul and pll_out_div rolled into one. I'm not sure that's something
> that should be done.
Consider the above example. The answer is "yes". Still, the user should not
have to deal with the internal clock tree of the hardware, and I don't see a
reason why it would be needed.
> And this driver does feel like it had it's share of tracking in development
> interfaces.
>
> >
> > Please also see this:
> >
> > <URL:http://www.mail-archive.com/linux-media@vger.kernel.org/msg39798.html>
>
> Sounds sensible for the future. But let's not hold this driver until
> agreement is reached about the details?
I assume the spec is public for this sensor? Albeit specifying the PLL
configuration in the platform data is not ideal, it is still indefinitely
better than specifying it in the driver itself.
Calculating the PLL parameters based on higher level concept of link
frequency would still be preferred, I think. It can be done now, even if the
link frequency isn't currently selectable by the user.
Regards,
--
Sakari Ailus
e-mail: sakari.ailus@iki.fi jabber/XMPP/Gmail: sailus@retiisi.org.uk
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-12-19 21:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-12-14 1:20 [PATCH v3] v4l: Add driver for Micron MT9M032 camera sensor Martin Hostettler
2011-12-14 1:55 ` Marek Vasut
2011-12-14 7:14 ` martin
2011-12-14 13:49 ` Laurent Pinchart
2011-12-14 18:58 ` martin
2011-12-14 22:11 ` Sakari Ailus
2011-12-17 9:50 ` martin
2011-12-19 10:47 ` Laurent Pinchart
2011-12-19 21:43 ` Sakari Ailus
2011-12-19 10:43 ` Laurent Pinchart
2011-12-14 21:44 ` Guennadi Liakhovetski
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.