Re: [PATCH v3 5/5] v4l: Add driver for Micron MT9M032 camera sensor
Hi Sakari,
On Thursday 08 March 2012 19:17:46 Sakari Ailus wrote:
On Wed, Mar 07, 2012 at 12:31:34PM +0100, Laurent Pinchart wrote:
[snip]
+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));
I'm not quite sure these should be touched.
Why not ? Do you think this could cause a regression in the future when
the fields won't be reserved anymore ?
The user is responsible for setting those fields to zero. If we set them to
zero for them they will start relying on that. At some point that might not
hold true anymore.
Thinking about it some more, applications should set the reserved fields to 0,
or first issue a get call and modify the fields it's interested in, keeping
the reserved fields at their default value. I'll remove the memset here.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] v4l: Add driver for Micron MT9M032 camera sensor
Hi Laurent,
On Wed, Mar 07, 2012 at 12:31:34PM +0100, Laurent Pinchart wrote:
...
+static u32 mt9m032_row_time(struct mt9m032 *sensor, unsigned int width)
+{
+ unsigned int effective_width;
+ u32 ns;
+
+ effective_width = width + 716; /* emperical value */
Why empirical value? This should be directly related to image width
(before binning) and horizontal blanking.
Ask Martin :-)
I don't have access to the documentation nor the hardware, so I'd rather keep
the value as-is for now.
Ok. Let's keep this one then.
+ ns = div_u64(10ULL * effective_width, sensor-pix_clock);
+ dev_dbg(to_dev(sensor), MT9M032 line time: %u ns\n, ns);
+ return ns;
+}
[snip]
+static int mt9m032_setup_pll(struct mt9m032 *sensor)
+{
+ static const struct aptina_pll_limits limits = {
+ .ext_clock_min = 800,
+ .ext_clock_max = 1650,
+ .int_clock_min = 200,
+ .int_clock_max = 2400,
+ .out_clock_min = 32200,
+ .out_clock_max = 69300,
+ .pix_clock_max = 9900,
+ .n_min = 1,
+ .n_max = 64,
+ .m_min = 16,
+ .m_max = 255,
+ .p1_min = 1,
+ .p1_max = 128,
+ };
+
+ struct i2c_client *client = v4l2_get_subdevdata(sensor-subdev);
+ struct mt9m032_platform_data *pdata = sensor-pdata;
+ struct aptina_pll pll;
+ int ret;
+
+ pll.ext_clock = pdata-ext_clock;
+ pll.pix_clock = pdata-pix_clock;
You could initialise these in the declaration.
Yes, but I would find that less readable :-)
Ok.
...
+ rect.width = min(rect.width, MT9M032_PIXEL_ARRAY_WIDTH - rect.left);
+ rect.height = min(rect.height, MT9M032_PIXEL_ARRAY_HEIGHT -
rect.top);
+
+ __crop = __mt9m032_get_pad_crop(sensor, fh, crop-which);
+
+ if (rect.width != __crop-width || rect.height != __crop-height) {
+ /* Reset the output image size if the crop rectangle size has
+ * been modified.
+ */
+ format = __mt9m032_get_pad_format(sensor, fh, crop-which);
+ format-width = rect.width;
+ format-height = rect.height;
I think you can do this unconditionally.
I could, but I hope to add binning/skipping support to this driver soon. The
check will be needed then.
Sounds fine for me.
+ }
+
+ *__crop = rect;
+ crop-rect = rect;
+
+ if (crop-which != V4L2_SUBDEV_FORMAT_ACTIVE)
+ return 0;
+
+ return mt9m032_update_geom_timing(sensor);
+}
[snip]
+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));
I'm not quite sure these should be touched.
Why not ? Do you think this could cause a regression in the future when the
fields won't be reserved anymore ?
The user is responsible for setting those fields to zero. If we set them to
zero for them they will start relying on that. At some point that might not
hold true anymore.
+ ret = mt9m032_update_timing(sensor, fi-interval);
+ if (!ret)
+ sensor-frame_interval = fi-interval;
+
+ return ret;
+}
+static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
+{
+ struct mt9m032 *sensor =
+ container_of(ctrl-handler, struct mt9m032, ctrls);
+ struct i2c_client *client = v4l2_get_subdevdata(sensor-subdev);
+ int ret;
+
+ switch (ctrl-id) {
+ case V4L2_CID_GAIN:
+ return mt9m032_set_gain(sensor, ctrl-val);
The gain control only touches analogue gain. Shouldn't you use
V4L2_CID_ANALOGUE_GAIN (or something alike) instead?
If there was such a control in mainline, sure ;-)
I plan to revisit controls in the various Aptina sensor drivers I maintain in
the near future. Analog/digital gains will be on my to-do list.
Fine for me. Let's think these after 3.4 merge window.
Cheers,
--
Sakari Ailus
e-mail: sakari.ai...@iki.fi jabber/XMPP/Gmail: sai...@retiisi.org.uk
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] v4l: Add driver for Micron MT9M032 camera sensor
Hi Sakari,
On Wednesday 07 March 2012 01:16:33 Sakari Ailus wrote:
Hi Laurent,
Thanks for the patch.
I have a few comments below. Just one fairly general question: locking.
Shouldn't you serialise accesses to sensor registers and your data,
possibly by using a mutex?
I guess I should, yes... :-) will fix.
Laurent Pinchart wrote:
From: Martin Hostettler mar...@neutronstar.dyndns.org
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 mar...@neutronstar.dyndns.org
[Lots of clean up, fixes and enhancements]
Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com
[snip]
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index d1304e1..8e037e9 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_VIDEO_AS3645A) += as3645a.o
obj-$(CONFIG_SOC_CAMERA_IMX074)+= imx074.o
obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o
+obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o
I would put this among the other subdev sensor drivers instead.
Good point.
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 000..c3d69aa
--- /dev/null
+++ b/drivers/media/video/mt9m032.c
[snip]
+static u32 mt9m032_row_time(struct mt9m032 *sensor, unsigned int width)
+{
+ unsigned int effective_width;
+ u32 ns;
+
+ effective_width = width + 716; /* emperical value */
Why empirical value? This should be directly related to image width
(before binning) and horizontal blanking.
Ask Martin :-)
I don't have access to the documentation nor the hardware, so I'd rather keep
the value as-is for now.
+ ns = div_u64(10ULL * effective_width, sensor-pix_clock);
+ dev_dbg(to_dev(sensor), MT9M032 line time: %u ns\n, ns);
+ return ns;
+}
[snip]
+static int mt9m032_setup_pll(struct mt9m032 *sensor)
+{
+ static const struct aptina_pll_limits limits = {
+ .ext_clock_min = 800,
+ .ext_clock_max = 1650,
+ .int_clock_min = 200,
+ .int_clock_max = 2400,
+ .out_clock_min = 32200,
+ .out_clock_max = 69300,
+ .pix_clock_max = 9900,
+ .n_min = 1,
+ .n_max = 64,
+ .m_min = 16,
+ .m_max = 255,
+ .p1_min = 1,
+ .p1_max = 128,
+ };
+
+ struct i2c_client *client = v4l2_get_subdevdata(sensor-subdev);
+ struct mt9m032_platform_data *pdata = sensor-pdata;
+ struct aptina_pll pll;
+ int ret;
+
+ pll.ext_clock = pdata-ext_clock;
+ pll.pix_clock = pdata-pix_clock;
You could initialise these in the declaration.
Yes, but I would find that less readable :-)
+ ret = aptina_pll_calculate(client-dev, limits, pll);
+ if (ret 0)
+ return ret;
+
+ sensor-pix_clock = pll.pix_clock;
+
+ ret = mt9m032_write(client, MT9M032_PLL_CONFIG1,
+ (pll.m MT9M032_PLL_CONFIG1_MUL_SHIFT)
+ | (pll.p1 - 1));
+ if (!ret)
+ ret = mt9m032_write(client, MT9P031_PLL_CONFIG2, pll.n - 1);
+ if (!ret)
+ ret = mt9m032_write(client, MT9P031_PLL_CONTROL,
+ MT9P031_PLL_CONTROL_PWRON |
+ MT9P031_PLL_CONTROL_USEPLL);
+ if (!ret) /* more reserved, Continuous, Master Mode */
+ ret = mt9m032_write(client, MT9M032_READ_MODE1, 0x8006);
+ if (!ret) /* Set 14-bit mode, select 7 divider */
+ ret = mt9m032_write(client, MT9M032_FORMATTER1, 0x111e);
I hope the datasheet is public. :-)
If there is one in a known URL it wouldn't hurt to refer to it, I think.
It's not public, and I don't have access to it (yet at least). I would have
modified this otherwise.
+ return ret;
+}
[snip]
+/**
+ * __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,
+ enum v4l2_subdev_format_whence which)
+{
+ switch (which) {
+ case V4L2_SUBDEV_FORMAT_TRY:
+ return v4l2_subdev_get_try_crop(fh, 0);
+ case V4L2_SUBDEV_FORMAT_ACTIVE:
+ return sensor-crop;
+
[PATCH v3 5/5] v4l: Add driver for Micron MT9M032 camera sensor
From: Martin Hostettler mar...@neutronstar.dyndns.org 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 mar...@neutronstar.dyndns.org [Lots of clean up, fixes and enhancements] Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com --- drivers/media/video/Kconfig |8 + drivers/media/video/Makefile |1 + drivers/media/video/mt9m032.c | 825 + include/media/mt9m032.h | 36 ++ 4 files changed, 870 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/mt9m032.c create mode 100644 include/media/mt9m032.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 666836d..2611708 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -947,6 +947,14 @@ 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 + select VIDEO_APTINA_PLL + 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 d1304e1..8e037e9 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -82,6 +82,7 @@ obj-$(CONFIG_VIDEO_AS3645A) += as3645a.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 000..c3d69aa --- /dev/null +++ b/drivers/media/video/mt9m032.c @@ -0,0 +1,825 @@ +/* + * Driver for MT9M032 CMOS Image Sensor from Micron + * + * Copyright (C) 2010-2011 Lund Engineering + * Contact: Gil Lund gwl...@lundeng.com + * Author: Martin Hostettler mar...@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/mt9m032.h +#include media/v4l2-ctrls.h +#include media/v4l2-device.h +#include media/v4l2-subdev.h + +#include aptina-pll.h + +/* + * width and height include active boundary and black parts + * + * column0- 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 + * row1140-1147 active boundry + * row1148-1151 black + */ + +#define MT9M032_PIXEL_ARRAY_WIDTH 1600 +#define MT9M032_PIXEL_ARRAY_HEIGHT 1152 + +#define MT9M032_CHIP_VERSION 0x00 +#defineMT9M032_CHIP_VERSION_VALUE 0x1402 +#define MT9M032_ROW_START 0x01 +#defineMT9M032_ROW_START_MIN 0 +#defineMT9M032_ROW_START_MAX 1152 +#defineMT9M032_ROW_START_DEF 60 +#define MT9M032_COLUMN_START 0x02 +#defineMT9M032_COLUMN_START_MIN0 +#defineMT9M032_COLUMN_START_MAX1600 +#defineMT9M032_COLUMN_START_DEF16 +#define MT9M032_ROW_SIZE 0x03 +#defineMT9M032_ROW_SIZE_MIN32 +#defineMT9M032_ROW_SIZE_MAX1152 +#defineMT9M032_ROW_SIZE_DEF1080 +#define MT9M032_COLUMN_SIZE0x04 +#define
Re: [PATCH v3 5/5] v4l: Add driver for Micron MT9M032 camera sensor
Hi Laurent, Thanks for the patch. I have a few comments below. Just one fairly general question: locking. Shouldn't you serialise accesses to sensor registers and your data, possibly by using a mutex? Laurent Pinchart wrote: From: Martin Hostettler mar...@neutronstar.dyndns.org 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 mar...@neutronstar.dyndns.org [Lots of clean up, fixes and enhancements] Signed-off-by: Laurent Pinchart laurent.pinch...@ideasonboard.com --- drivers/media/video/Kconfig |8 + drivers/media/video/Makefile |1 + drivers/media/video/mt9m032.c | 825 + include/media/mt9m032.h | 36 ++ 4 files changed, 870 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/mt9m032.c create mode 100644 include/media/mt9m032.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 666836d..2611708 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -947,6 +947,14 @@ 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 + select VIDEO_APTINA_PLL + 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 d1304e1..8e037e9 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -82,6 +82,7 @@ obj-$(CONFIG_VIDEO_AS3645A) += as3645a.o obj-$(CONFIG_SOC_CAMERA_IMX074) += imx074.o obj-$(CONFIG_SOC_CAMERA_MT9M001) += mt9m001.o +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o I would put this among the other subdev sensor drivers instead. 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 000..c3d69aa --- /dev/null +++ b/drivers/media/video/mt9m032.c @@ -0,0 +1,825 @@ +/* + * Driver for MT9M032 CMOS Image Sensor from Micron + * + * Copyright (C) 2010-2011 Lund Engineering + * Contact: Gil Lund gwl...@lundeng.com + * Author: Martin Hostettler mar...@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/mt9m032.h +#include media/v4l2-ctrls.h +#include media/v4l2-device.h +#include media/v4l2-subdev.h + +#include aptina-pll.h + +/* + * width and height include active boundary and black parts + * + * column0- 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 + * row1140-1147 active boundry + * row1148-1151 black + */ + +#define MT9M032_PIXEL_ARRAY_WIDTH1600 +#define MT9M032_PIXEL_ARRAY_HEIGHT 1152 + +#define MT9M032_CHIP_VERSION 0x00 +#define MT9M032_CHIP_VERSION_VALUE 0x1402 +#define MT9M032_ROW_START0x01 +#define MT9M032_ROW_START_MIN 0 +#define MT9M032_ROW_START_MAX 1152 +#define MT9M032_ROW_START_DEF 60 +#define MT9M032_COLUMN_START 0x02 +#define MT9M032_COLUMN_START_MIN0 +#define MT9M032_COLUMN_START_MAX1600 +#define MT9M032_COLUMN_START_DEF
