Re: [PATCH v2] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin,
On Tuesday 20 September 2011 22:42:23 mar...@neutronstar.dyndns.org wrote:
> On Mon, Sep 19, 2011 at 12:48:24AM +0200, Laurent Pinchart wrote:
> > On Saturday 17 September 2011 11:29:31 Martin Hostettler wrote:
[snip]
> > > +{
> > > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > > + s32 data = i2c_smbus_read_word_data(client, reg);
> > > +
> > > + return data < 0 ? data : swab16(data);
> >
> > You should use be16_to_cpu() here.
>
> No, i2c_smbus_read_word_data already has cpu to smbus endianness conversion
> built in. The hardware just does want it the other way around.
>
> At least that what i understand of drivers/i2c/i2c-core.c
> i2c_smbus_xfer_emulated
>
>1959 case I2C_SMBUS_WORD_DATA:
>1960 if (read_write == I2C_SMBUS_READ)
>1961 msg[1].len = 2;
>1962 else {
>1963 msg[0].len = 3;
>1964 msgbuf0[1] = data->word & 0xff;
>1965 msgbuf0[2] = data->word >> 8;
>1966 }
>1967 break;
>
>2063 case I2C_SMBUS_WORD_DATA:
>2064 case I2C_SMBUS_PROC_CALL:
>2065 data->word = msgbuf1[0] | (msgbuf1[1] <<
> 8); 2066 break;
>
> So it does it's own internal endianess handling.
My bad. Someone I won't name made me switch from swab16() to be16_to_cpu() in
a couple of other drivers, I think he will recognize himself :-) I'm at fault
for blindly doing it though.
[snip]
> > > +static int mt9m032_update_timing(struct mt9m032 *sensor,
> > > + struct v4l2_fract *interval,
> > > + const struct v4l2_rect *crop)
> > > +{
> > > + unsigned long row_time;
> > > + int additional_blanking_rows;
> > > + int min_blank;
> >
> > Those can't be negative, what about using unsigned it ?
>
> additional_blanking_rows can get negative during the calculation and then
> get clampted to min_blank. And having them both the signed keeps the
> cpu/compiler happy.
OK.
> > > +
> > > + if (!interval)
> > > + interval = &sensor->frame_interval;
> >
> > This will have the side effect of possibly modifying the frame interval
> > stored in the mt9m032 structure. Is that on purpose ?
>
> Yes, it adjusts the frame interval to a supported value for the given
> geometry.
>
> > > + if (!crop)
> > > + crop = &sensor->crop;
> >
> > If I'm not mistaken the function is always called with the crop parameter
> > set to either NULL or &sensor->crop. I think the parameter could be
> > removed.
>
> No it's called with a temporary from mt9m032_set_crop via
> mt9m032_update_geom_timing.
Right, my bad.
What bothers me is that mt9m032_update_geom_timing() and
mt9m032_update_timing() are used both to validate/modify the crop rectangle
and the frame interval, and to apply the settings to the hardware. I think it
would be cleaner if that was split in two parts.
> That codepath would overwrite the changes instantly after
> mt9m032_update_timing returns.
>
> Now trying to handle any errors in that path might not be a useful thing to
> do. I guess the results when mt9m032_write_reg fails will ultimatly be
> near random anyway...
[snip]
> > > + additional_blanking_rows = div_u64(((u64)10) *
> > > interval->numerator,
> > > + ((u64)interval->denominator) *
> > > row_time) + - crop->height;
> > > + }
> > > + /* enforce minimal 1.6ms blanking time. */
> > > + min_blank = 160 / row_time;
> > > + additional_blanking_rows = clamp(additional_blanking_rows,
> > > + min_blank,
> > > MT9M032_MAX_BLANKING_ROWS); +
> > > + return mt9m032_write_reg(sensor, MT9M032_VBLANK,
> > > additional_blanking_rows); +}
> > > +
> > > +static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
> > > + const struct v4l2_rect *crop)
> > > +{
> > > + int ret;
> > > +
> > > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width -
> > > 1); + if (!ret)
> > > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height
> > > -
1);
> > > + /* offsets compensate for black border */
> > > + if (!ret)
> > > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START,
> > > crop->left
+
> > > 16); +if (!ret)
> > > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_START, crop->top +
52);
> >
> > I don't think the black rows/columns offsets should be added implicitly
> > by the driver. Wouldn't it be beter to make them explicit ?
>
> Why? I consider them an implementation detail userspace shouldn't need to
> worry about...
Because userspace could be interesting in getting the black rows for instance.
> > > + if (!ret)
> > > + ret = mt9m032_update_timing(sensor, NULL, crop);
> > > + return ret;
> > > +}
> > >
Re: [PATCH v2] v4l: Add driver for Micron MT9M032 camera sensor
On Mon, Sep 19, 2011 at 12:48:24AM +0200, Laurent Pinchart wrote: > Hi Martin, > > Thanks for the patch. > > On Saturday 17 September 2011 11:29:31 Martin Hostettler wrote: > > The MT9M032 is a parallel 3MP sensor from Micron controlled through I2C. > > According to the Aptina datasheet, it's an 1.6MP sensor :-) Yes, of course. > > > 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 > > --- > > drivers/media/video/Kconfig |7 + > > drivers/media/video/Makefile|1 + > > drivers/media/video/mt9m032.c | 814 > > +++ include/media/mt9m032.h | > > 38 ++ > > include/media/v4l2-chip-ident.h |1 + > > 5 files changed, 861 insertions(+), 0 deletions(-) > > create mode 100644 drivers/media/video/mt9m032.c > > create mode 100644 include/media/mt9m032.h > > > > 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 f574dc0..41c6c12 100644 > > --- a/drivers/media/video/Kconfig > > +++ b/drivers/media/video/Kconfig > > @@ -798,6 +798,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 2723900..0d86830 100644 > > --- a/drivers/media/video/Makefile > > +++ b/drivers/media/video/Makefile > > @@ -74,6 +74,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 000..8a64193 > > --- /dev/null > > +++ b/drivers/media/video/mt9m032.c > > @@ -0,0 +1,814 @@ > > +/* > > + * Driver for MT9M032 CMOS Image Sensor from Micron > > + * > > + * Copyright (C) 2010-2011 Lund Engineering > > + * Contact: Gil Lund > > + * Author: Martin Hostettler > > + * > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include > > +#include > > You can remove this header. yes, another leftover. > > > +#include > > +#include > > +#include > > + > > +#include > > + > > +#define MT9M032_CHIP_VERSION 0x00 > > +#define MT9M032_ROW_START 0x01 > > +#define MT9M032_COLUMN_START 0x02 > > +#define MT9M032_ROW_SIZE 0x03 > > +#define MT9M032_COLUMN_SIZE0x04 > > +#define MT9M032_HBLANK 0x05 > > +#define MT9M032_VBLANK 0x06 > > +#define MT9M032_SHUTTER_WIDTH_HIGH 0x08 > > +#define MT9M032_SHUTTER_WIDTH_LOW 0x09 > > +#defi
Re: [PATCH v2] v4l: Add driver for Micron MT9M032 camera sensor
On Tue, 20 Sep 2011, Laurent Pinchart wrote: > Hi Guennadi, > > On Monday 19 September 2011 21:28:09 Guennadi Liakhovetski wrote: > > Hi Laurent > > > > just one question: > > > > On Mon, 19 Sep 2011, Laurent Pinchart wrote: > > > > diff --git a/drivers/media/video/mt9m032.c > > > > b/drivers/media/video/mt9m032.c new file mode 100644 > > > > index 000..8a64193 > > > > --- /dev/null > > > > +++ b/drivers/media/video/mt9m032.c > > > > @@ -0,0 +1,814 @@ > > > > [snip] > > > > > > +static int mt9m032_read_reg(struct mt9m032 *sensor, const u8 reg) > > > > > > No need for the const keyword, this isn't a pointer :-) > > > > I was actually wondering about these: of course it's not the same as using > > const for a pointer to tell the compiler, that this function will not > > change caller's data. But - doesn't using const for any local variable > > tell the compiler, that that _variable_ will not be modified in this > > function? Are there no optimisation possibilities, arising from that? > > I would expect the compiler to be smart enough to notice that the variable is > never assigned. Sure, but using "const" would allow the compiler to catch and complain about uses like foo(®); unless foo() is also declared as foo(const u8 *). But yes, for small obvious functions it makes no difference. > In practice, for such a small function, the generated code is > identical with and without the const keyword. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 v2] v4l: Add driver for Micron MT9M032 camera sensor
Hi Guennadi, On Monday 19 September 2011 21:28:09 Guennadi Liakhovetski wrote: > Hi Laurent > > just one question: > > On Mon, 19 Sep 2011, Laurent Pinchart wrote: > > > diff --git a/drivers/media/video/mt9m032.c > > > b/drivers/media/video/mt9m032.c new file mode 100644 > > > index 000..8a64193 > > > --- /dev/null > > > +++ b/drivers/media/video/mt9m032.c > > > @@ -0,0 +1,814 @@ > > [snip] > > > > +static int mt9m032_read_reg(struct mt9m032 *sensor, const u8 reg) > > > > No need for the const keyword, this isn't a pointer :-) > > I was actually wondering about these: of course it's not the same as using > const for a pointer to tell the compiler, that this function will not > change caller's data. But - doesn't using const for any local variable > tell the compiler, that that _variable_ will not be modified in this > function? Are there no optimisation possibilities, arising from that? I would expect the compiler to be smart enough to notice that the variable is never assigned. In practice, for such a small function, the generated code is identical with and without the const keyword. -- 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 v2] v4l: Add driver for Micron MT9M032 camera sensor
Hi Laurent just one question: On Mon, 19 Sep 2011, Laurent Pinchart wrote: > > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c > > new file mode 100644 > > index 000..8a64193 > > --- /dev/null > > +++ b/drivers/media/video/mt9m032.c > > @@ -0,0 +1,814 @@ [snip] > > +static int mt9m032_read_reg(struct mt9m032 *sensor, const u8 reg) > > No need for the const keyword, this isn't a pointer :-) I was actually wondering about these: of course it's not the same as using const for a pointer to tell the compiler, that this function will not change caller's data. But - doesn't using const for any local variable tell the compiler, that that _variable_ will not be modified in this function? Are there no optimisation possibilities, arising from that? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- 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 v2] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, Thanks for the patch. On Saturday 17 September 2011 11:29:31 Martin Hostettler wrote: > The MT9M032 is a parallel 3MP sensor from Micron controlled through I2C. According to the Aptina datasheet, it's an 1.6MP sensor :-) > 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 > --- > drivers/media/video/Kconfig |7 + > drivers/media/video/Makefile|1 + > drivers/media/video/mt9m032.c | 814 > +++ include/media/mt9m032.h | > 38 ++ > include/media/v4l2-chip-ident.h |1 + > 5 files changed, 861 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/mt9m032.c > create mode 100644 include/media/mt9m032.h > > 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 f574dc0..41c6c12 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -798,6 +798,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 2723900..0d86830 100644 > --- a/drivers/media/video/Makefile > +++ b/drivers/media/video/Makefile > @@ -74,6 +74,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 000..8a64193 > --- /dev/null > +++ b/drivers/media/video/mt9m032.c > @@ -0,0 +1,814 @@ > +/* > + * Driver for MT9M032 CMOS Image Sensor from Micron > + * > + * Copyright (C) 2010-2011 Lund Engineering > + * Contact: Gil Lund > + * Author: Martin Hostettler > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > +#include You can remove this header. > +#include > +#include > +#include > + > +#include > + > +#define MT9M032_CHIP_VERSION 0x00 > +#define MT9M032_ROW_START0x01 > +#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_LOW0x09 > +#define MT9M032_PIX_CLK_CTRL 0x0a > +#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000 > +#define MT9M032_RESTART 0x0b > +#define MT9M032_RESET0x0d > +#define MT9M032_PLL_CONFIG1 0x11 > +#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f > +#define MT9M032_PLL_
