Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, On Monday 24 January 2011 21:45:39 mar...@neutronstar.dyndns.org wrote: > On Mon, Jan 24, 2011 at 12:32:12PM +0100, Laurent Pinchart wrote: > > On Thursday 20 January 2011 23:56:07 mar...@neutronstar.dyndns.org wrote: [snip] > > > >> +#define OFFSET_UNCHANGED 0x > > > >> +static int mt9m032_set_pad_geom(struct mt9m032 *sensor, > > > >> + struct v4l2_subdev_fh *fh, > > > >> + u32 which, u32 pad, > > > >> + s32 top, s32 left, s32 width, s32 > > > >> height) > > > >> +{ > > > >> + struct v4l2_mbus_framefmt tmp_format; > > > >> + struct v4l2_rect tmp_crop; > > > >> + struct v4l2_mbus_framefmt *format; > > > >> + struct v4l2_rect *crop; > > > >> + > > > >> + if (pad != 0) > > > >> + return -EINVAL; > > > >> + > > > >> + format = __mt9m032_get_pad_format(sensor, fh, which); > > > >> + crop = __mt9m032_get_pad_crop(sensor, fh, which); > > > >> + if (!format || !crop) > > > >> + return -EINVAL; > > > >> + if (which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > > >> + tmp_crop = *crop; > > > >> + tmp_format = *format; > > > >> + format = &tmp_format; > > > >> + crop = &tmp_crop; > > > >> + } > > > >> + > > > >> + if (top != OFFSET_UNCHANGED) > > > >> + crop->top = top & ~0x1; > > > >> + if (left != OFFSET_UNCHANGED) > > > >> + crop->left = left; > > > >> + crop->height = height; > > > >> + crop->width = width & ~1; > > > >> + > > > >> + format->height = crop->height; > > > >> + format->width = crop->width; > > > > > > > > This looks very weird to me. If your sensor doesn't include a scaler, > > > > it should support a single fixed format. Crop will then be used to > > > > select the crop rectangle. You're mixing the two for no obvious > > > > reason. > > > > > > I think i have to have both size and crop writable. So i wrote the code > > > to just have format width/height and crop width/height to be equal at > > > all times. So actually almost all code for crop setting and format are > > > shared. > > > > > > As you wrote in your recent mail this api isn't really intuitive and > > > i'm not really sure what's the right thing to do thus i just copied > > > the semantics from an existing driver with similar capable hardware. > > > > > > This code works nicely and media-ctl needs to be able to set the size > > > so that's the most logical i could come up with... > > > > See > > http://git.linuxtv.org/pinchartl/media.git?a=commitdiff;h=10affb3c5e0c8ae > > 74461c1b6a4ca6ed5251c27d8 for crop/format implementation for a sensor > > that supports cropping and binning. > > You basically say the set_format should just force the width and height of > the format to the croping rect's width and height if the sensor doesn't > support binning? That would of course be easy to implement. Yes, that's how I think it should be implemented. > Btw, i noticed MT9T001 does the register writes on crop->which == > V4L2_SUBDEV_FORMAT_TRY in mt9t001_set_crop... Looks like a tiny bug. I saw the bug a couple of weeks ago, I've fixed it and I'll push a new version when I'll have time to clean the code a little bit. -- 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] v4l: Add driver for Micron MT9M032 camera sensor
On Mon, Jan 24, 2011 at 12:32:12PM +0100, Laurent Pinchart wrote: > Hi Martin, > > On Thursday 20 January 2011 23:56:07 mar...@neutronstar.dyndns.org wrote: > > [snip] > > > >> +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int > > >> width) +{ > > >> +int effective_width; > > >> +u64 ns; > > >> +effective_width = width + 716; /* emperical value */ > > > > > > Where does it come from ? > > > > Like the comment says, it's just what the hardware seems to do from > > measureing framerates. Sadly i couldn't find anything exact anywhere... > > :-( Is the datasheet publicly available ? Only a very reduced version, that has nothing detailed afaik... > > [snip] > > > >> +row_time = mt9m032_row_time(sensor, crop->width); > > >> +do_div(ns, row_time); > > >> + > > >> +additional_blanking_rows = ns - crop->height; > > >> + > > >> +/* enforce minimal 1.6ms blanking time. */ > > >> +min_blank = 160 / row_time; > > >> +if (additional_blanking_rows < min_blank) > > >> +additional_blanking_rows = min_blank; > > > > > > You can use the min() macro. > > > > I'm pretty sure it's the max() one, but yes. > > > > >> +dev_dbg(to_dev(sensor), > > >> +"%s: V-blank %i\n", __func__, additional_blanking_rows); > > >> +if (additional_blanking_rows > 0x7ff) { > > >> +/* hardware limits 11 bit values */ > > >> +dev_warn(to_dev(sensor), > > >> +"mt9m032: frame rate too low.\n"); > > >> +additional_blanking_rows = 0x7ff; > > >> +} > > > > > > Or rather the clamp() macro. > > > > I think the error reporting reads more natual when doing the upper bound in > > the if. > > I would just do > > additional_blanking_rows = clamp(ns - crop->height, min_blank, 0x7ff); > > I don't think there's a need for any error reporting here. What you must do > instead is to limit the frame rate to hardware-acceptable values when the > user > tries to set it. Well when the value returned to userspace get gets clamped properly that should be enough, yes. > > [snip] > > > >> +static int update_formatter2(struct mt9m032 *sensor, bool streaming) > > >> +{ > > >> +struct i2c_client *client = > > >> v4l2_get_subdevdata(&sensor->subdev); > > >> + > > >> +u16 reg_val = 0x1000 /* Dout enable */ > > >> + | 0x0070; /* parts reserved! */ > > >> + /* possibly for changing to 14-bit > > >> mode */ > > >> + > > >> +if (streaming) > > >> +reg_val |= 0x2000; /* pixclock enable */ > > > > > > Please define constants at the beginning of the file (with the register > > > addresses) instead of using magic numbers. > > > > I'm using defines for all register numbers where i know the function > > reasonably well and explicit comments or variable names for all the bits i > > set in these registers. (And each register is only set in one function) > > > > I think that should be quite decent. Sadly from the material i have i have a > > lot of just undocumented pokeing at reserved bits to keep. For these cases i > > marked it in the code somehow are reserved and didn't do any defines for the > > register names because they would be useless. > > How did you get the information in the first place ? Pseudo code for the setup tasks. > > > Do you think this is acceptable? Or do i need to have a define for each > > known bit position the driver sets? > > Please define them. See > http://git.linuxtv.org/pinchartl/media.git?a=commitdiff;h=26e4a508f6e0fcb416e21bd29967ce6e2622abc7;hp=10affb3c5e0c8ae74461c1b6a4ca6ed5251c27d8#patch3 > Ok, i move everything where i'm confident that i know what it does to defines. > > What would i do with the undocumented bits? > > > > >> +#define OFFSET_UNCHANGED0x > > >> +static int mt9m032_set_pad_geom(struct mt9m032 *sensor, > > >> +struct v4l2_subdev_fh *fh, > > >> +u32 which, u32 pad, > > >> +s32 top, s32 left, s32 width, s32 > > >> height) > > >> +{ > > >> +struct v4l2_mbus_framefmt tmp_format; > > >> +struct v4l2_rect tmp_crop; > > >> +struct v4l2_mbus_framefmt *format; > > >> +struct v4l2_rect *crop; > > >> + > > >> +if (pad != 0) > > >> +return -EINVAL; > > >> + > > >> +format = __mt9m032_get_pad_format(sensor, fh, which); > > >> +crop = __mt9m032_get_pad_crop(sensor, fh, which); > > >> +if (!format || !crop) > > >> +return -EINVAL; > > >> +if (which == V4L2_SUBDEV_FORMAT_ACTIVE) { > > >> +tmp_crop = *crop; > > >> +tmp_format = *format; > > >> +format = &tmp_format; > > >> +crop = &tmp_crop; > > >> +} > > >> + > > >>
Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, On Thursday 20 January 2011 23:56:07 mar...@neutronstar.dyndns.org wrote: [snip] > >> +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int > >> width) +{ > >> + int effective_width; > >> + u64 ns; > >> + effective_width = width + 716; /* emperical value */ > > > > Where does it come from ? > > Like the comment says, it's just what the hardware seems to do from > measureing framerates. Sadly i couldn't find anything exact anywhere... :-( Is the datasheet publicly available ? [snip] > >> + row_time = mt9m032_row_time(sensor, crop->width); > >> + do_div(ns, row_time); > >> + > >> + additional_blanking_rows = ns - crop->height; > >> + > >> + /* enforce minimal 1.6ms blanking time. */ > >> + min_blank = 160 / row_time; > >> + if (additional_blanking_rows < min_blank) > >> + additional_blanking_rows = min_blank; > > > > You can use the min() macro. > > I'm pretty sure it's the max() one, but yes. > > >> + dev_dbg(to_dev(sensor), > >> + "%s: V-blank %i\n", __func__, additional_blanking_rows); > >> + if (additional_blanking_rows > 0x7ff) { > >> + /* hardware limits 11 bit values */ > >> + dev_warn(to_dev(sensor), > >> + "mt9m032: frame rate too low.\n"); > >> + additional_blanking_rows = 0x7ff; > >> + } > > > > Or rather the clamp() macro. > > I think the error reporting reads more natual when doing the upper bound in > the if. I would just do additional_blanking_rows = clamp(ns - crop->height, min_blank, 0x7ff); I don't think there's a need for any error reporting here. What you must do instead is to limit the frame rate to hardware-acceptable values when the user tries to set it. [snip] > >> +static int update_formatter2(struct mt9m032 *sensor, bool streaming) > >> +{ > >> + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev); > >> + > >> + u16 reg_val = 0x1000 /* Dout enable */ > >> +| 0x0070; /* parts reserved! */ > >> + /* possibly for changing to 14-bit mode */ > >> + > >> + if (streaming) > >> + reg_val |= 0x2000; /* pixclock enable */ > > > > Please define constants at the beginning of the file (with the register > > addresses) instead of using magic numbers. > > I'm using defines for all register numbers where i know the function > reasonably well and explicit comments or variable names for all the bits i > set in these registers. (And each register is only set in one function) > > I think that should be quite decent. Sadly from the material i have i have a > lot of just undocumented pokeing at reserved bits to keep. For these cases i > marked it in the code somehow are reserved and didn't do any defines for the > register names because they would be useless. How did you get the information in the first place ? > Do you think this is acceptable? Or do i need to have a define for each > known bit position the driver sets? Please define them. See http://git.linuxtv.org/pinchartl/media.git?a=commitdiff;h=26e4a508f6e0fcb416e21bd29967ce6e2622abc7;hp=10affb3c5e0c8ae74461c1b6a4ca6ed5251c27d8#patch3 > What would i do with the undocumented bits? > > >> +#define OFFSET_UNCHANGED 0x > >> +static int mt9m032_set_pad_geom(struct mt9m032 *sensor, > >> + struct v4l2_subdev_fh *fh, > >> + u32 which, u32 pad, > >> + s32 top, s32 left, s32 width, s32 height) > >> +{ > >> + struct v4l2_mbus_framefmt tmp_format; > >> + struct v4l2_rect tmp_crop; > >> + struct v4l2_mbus_framefmt *format; > >> + struct v4l2_rect *crop; > >> + > >> + if (pad != 0) > >> + return -EINVAL; > >> + > >> + format = __mt9m032_get_pad_format(sensor, fh, which); > >> + crop = __mt9m032_get_pad_crop(sensor, fh, which); > >> + if (!format || !crop) > >> + return -EINVAL; > >> + if (which == V4L2_SUBDEV_FORMAT_ACTIVE) { > >> + tmp_crop = *crop; > >> + tmp_format = *format; > >> + format = &tmp_format; > >> + crop = &tmp_crop; > >> + } > >> + > >> + if (top != OFFSET_UNCHANGED) > >> + crop->top = top & ~0x1; > >> + if (left != OFFSET_UNCHANGED) > >> + crop->left = left; > >> + crop->height = height; > >> + crop->width = width & ~1; > >> + > >> + format->height = crop->height; > >> + format->width = crop->width; > > > > This looks very weird to me. If your sensor doesn't include a scaler, it > > should support a single fixed format. Crop will then be used to select > > the crop rectangle. You're mixing the two for no obvious reason. > > I think i have to have both size and crop writable. So i wrote the code to > just have format width/height and crop width/height to be equal at all > times. So actually almost all code for crop setting and format are shared. > > As you wrote in your recent mail this api isn't really intuitive and i'm > not really sure what's the right thing to d
Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, On Wednesday 19 January 2011 20:12:14 mar...@neutronstar.dyndns.org wrote: > On Wed, Jan 19, 2011 at 12:05:10AM +0100, Hans Verkuil wrote: > > On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: [snip] > > > +#ifdef CONFIG_VIDEO_ADV_DEBUG > > > +static long mt9m032_ioctl(struct v4l2_subdev *sd, unsigned int cmd, > > > void *arg) +{ > > > + if (cmd == VIDIOC_DBG_G_REGISTER || cmd == VIDIOC_DBG_S_REGISTER) { > > > + struct v4l2_dbg_register *p = arg; > > > + > > > + if (!capable(CAP_SYS_ADMIN)) > > > + return -EPERM; > > > + > > > + if (cmd == VIDIOC_DBG_G_REGISTER) > > > + return v4l2_subdev_call(sd, core, g_register, p); > > > + else > > > + return v4l2_subdev_call(sd, core, s_register, p); > > > + } else { > > > + return -ENOIOCTLCMD; > > > + } > > > +} > > > > Huh? Ah, I get it. This is for when the user uses the subdev's device > > node directly. This is not good, the v4l2 framework should do translate > > this to g/s_register. The same should be done for g_chip_ident, I guess. > > I'm not sure what you are saying here. Should i move this to a patch for > v4l2-subdev.c to dispatch those ioctls for all subdevs? Yes, please do that. > I need these ioctls to work with the driver and last that i looked nothing > in the general framework or the omap3 ISP driver was forwarding there from > the video device node to the subdevice driver... -- 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] v4l: Add driver for Micron MT9M032 camera sensor
Hi Hans, On Wednesday 19 January 2011 08:23:18 Hans Verkuil wrote: > On Wednesday, January 19, 2011 00:50:35 Laurent Pinchart wrote: > > On Wednesday 19 January 2011 00:05:10 Hans Verkuil wrote: > > > On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: [snip] > > > > +#ifdef CONFIG_VIDEO_ADV_DEBUG > > > > +static long mt9m032_ioctl(struct v4l2_subdev *sd, unsigned int cmd, > > > > void *arg) +{ > > > > + if (cmd == VIDIOC_DBG_G_REGISTER || cmd == > > > > VIDIOC_DBG_S_REGISTER) { > > > > + struct v4l2_dbg_register *p = arg; > > > > + > > > > + if (!capable(CAP_SYS_ADMIN)) > > > > + return -EPERM; > > > > + > > > > + if (cmd == VIDIOC_DBG_G_REGISTER) > > > > + return v4l2_subdev_call(sd, core, g_register, > > > > p); > > > > + else > > > > + return v4l2_subdev_call(sd, core, s_register, > > > > p); > > > > + } else { > > > > + return -ENOIOCTLCMD; > > > > + } > > > > +} > > > > > > Huh? Ah, I get it. This is for when the user uses the subdev's device > > > node directly. This is not good, the v4l2 framework should do > > > translate this to g/s_register. > > > > Agreed. > > > > > The same should be done for g_chip_ident, I guess. > > > > I don't think we need g_chip_ident for subdev nodes, do we ? > > Why not? It makes the use of v4l2-dbg a bit easier if it is there. If you > provide g/s_register, then you should provide this one as well. Because v4l2_dbg_register::match is used to address a specific subdevice. When issuing the VIDIOC_DBG_[GS]_REGISTER ioctls on a subdev device node, the field isn't needed anymore. > I though of another one that should be handled in the framework: > VIDIOC_LOG_STATUS. > > That definitely should be handled as well. -- 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] v4l: Add driver for Micron MT9M032 camera sensor
Hi Laurent, >> + >> +#define MT9M032_CHIP_VERSION0x00 >> +#define MT9M032_ROW_START 0x01 >> +#define MT9M032_COLUMN_START0x02 >> +#define MT9M032_ROW_SIZE0x03 >> +#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_CTRL0x0A > > Kernel code usually uses lowercase hex constants. ok. >> +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width) >> +{ >> +int effective_width; >> +u64 ns; >> +effective_width = width + 716; /* emperical value */ > > Where does it come from ? Like the comment says, it's just what the hardware seems to do from measureing framerates. Sadly i couldn't find anything exact anywhere... >> +ns = 10ll * effective_width; >> +do_div(ns, sensor->pix_clock); > > Do you have high enough clock frequencies that you would loose precision by > dividing 1e9 by the clock first, and the multiplying it by the row length ? > If > so I would use div_u64(). Thanks for the hint to use div_u64, that's a bit nicer. To be honest, this is in a slow path, it uses SI units and with div_64 reads reasonably well and is what i want to express. I don't see to do something clever here just to save a few cycles and have it harder to reason about... [...] >> + >> +row_time = mt9m032_row_time(sensor, crop->width); >> +do_div(ns, row_time); >> + >> +additional_blanking_rows = ns - crop->height; >> + >> +/* enforce minimal 1.6ms blanking time. */ >> +min_blank = 160 / row_time; >> +if (additional_blanking_rows < min_blank) >> +additional_blanking_rows = min_blank; > > You can use the min() macro. I'm pretty sure it's the max() one, but yes. >> +dev_dbg(to_dev(sensor), >> +"%s: V-blank %i\n", __func__, additional_blanking_rows); >> +if (additional_blanking_rows > 0x7ff) { >> +/* hardware limits 11 bit values */ >> +dev_warn(to_dev(sensor), >> +"mt9m032: frame rate too low.\n"); >> +additional_blanking_rows = 0x7ff; >> +} > > Or rather the clamp() macro. I think the error reporting reads more natual when doing the upper bound in the if. >> +return mt9m032_write_reg(client, MT9M032_VBLANK, >> additional_blanking_rows); >> +} >> + >> +static int mt9m032_update_geom_timing(struct mt9m032 *sensor, >> + const struct v4l2_rect *crop) >> +{ >> +struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev); >> +int ret; >> + >> +if (!crop) >> +crop = &sensor->crop; > > I'd rather have the caller do this instead of magically working around a NULL > argument. Yes, on a second look at the current code you're right. >> +ret = mt9m032_write_reg(client, MT9M032_COLUMN_SIZE, crop->width - 1); >> +if (!ret) >> +mt9m032_write_reg(client, MT9M032_ROW_SIZE, crop->height - 1); > > Aren't you missing a ret = here (and below) ? Ouch. >> +static int update_formatter2(struct mt9m032 *sensor, bool streaming) >> +{ >> +struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev); >> + >> +u16 reg_val = 0x1000 /* Dout enable */ >> + | 0x0070; /* parts reserved! */ >> + /* possibly for changing to 14-bit mode */ >> + >> +if (streaming) >> +reg_val |= 0x2000; /* pixclock enable */ > > Please define constants at the beginning of the file (with the register > addresses) instead of using magic numbers. I'm using defines for all register numbers where i know the function reasonably well and explicit comments or variable names for all the bits i set in these registers. (And each register is only set in one function) I think that should be quite decent. Sadly from the material i have i have a lot of just undocumented pokeing at reserved bits to keep. For these cases i marked it in the code somehow are reserved and didn't do any defines for the register names because they would be useless. Do you think this is acceptable? Or do i need to have a define for each known bit position the driver sets? What would i do with the undocumented bits? >> +#define OFFSET_UNCHANGED0x >> +static int mt9m032_set_pad_geom(struct mt9m032 *sensor, >> +struct v4l2_subdev_fh *fh, >> +u32 which, u32 pad, >> +s32 top, s32 left, s32 width, s32 height) >> +{ >> +struct v4l2_mbus_framefmt tmp_format; >> +struct v4l2_rect tmp_crop; >> +struct v4l2_mbus_framefmt *format; >> +struct v4l2_rect *crop; >> + >> +if (pad != 0) >> +return -EINVAL; >> + >> +format = __mt9m032_get_pad_format(sensor, fh, which); >> +crop = __mt9m032_
Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
Hi Hans, Thanks for the quick review. I just noticed i didn't really understand the new control framework that well, could you maybe add a comment pointing to Documentation/video4linux/v4l2-controls.txt in the v4l2-ctrl.h header? I think that would help a lot. On Wed, Jan 19, 2011 at 12:05:10AM +0100, Hans Verkuil wrote: > Hi Martin, > > On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: > > The MT9M032 is a parallel 1284x812 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. > > Now, this is a truly bleeding edge driver :-) > > Pads aren't even merged yet! Yes, indeed. Blame the omap3 ISP driver for this :) > > Got some small comments: > > > Signed-off-by: Martin Hostettler > > --- > > drivers/media/video/Kconfig |7 + > > drivers/media/video/Makefile|1 + > > drivers/media/video/mt9m032.c | 834 > > +++ > > drivers/media/video/mt9m032.h | 38 ++ > > include/media/v4l2-chip-ident.h |1 + > > 5 files changed, 881 insertions(+), 0 deletions(-) > > create mode 100644 drivers/media/video/mt9m032.c > > create mode 100644 drivers/media/video/mt9m032.h > > > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > > index 9fad1a6..f2d5f80 100644 > > --- a/drivers/media/video/Kconfig > > +++ b/drivers/media/video/Kconfig > > @@ -773,6 +773,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 8f70b06..3e7299f 100644 > > --- a/drivers/media/video/Makefile > > +++ b/drivers/media/video/Makefile > > @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o > > obj-$(CONFIG_VIDEO_OV7670) += ov7670.o > > obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o > > obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o > > +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o > > obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c > > new file mode 100644 > > index 000..fe6af7b > > --- /dev/null > > +++ b/drivers/media/video/mt9m032.c > > @@ -0,0 +1,834 @@ > > +/* > > + * 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 > > +#include > > +#include > > + > > +#include "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_SIZE0x04 > > +#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_RESTART0x0B > > +#define MT9M032_RESET 0x0D > > +#define MT9M032_PLL_CONFIG10x11 > > +#define MT9M032_READ_MODE1 0x1E > > +#define MT9M032_READ_MODE2 0x20 > > +#define MT9M032_GAIN_GREEN10x2B > > +#define MT9M032_GAIN_BLUE 0x2C > > +#define MT9M032_GAIN_RED 0x2D > > +#define MT9M032_GAIN_GREEN20x2E > > +/* write only */ > > +#define MT9M032_GAIN_ALL 0x35 > > +#define MT9M032_F
Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
On Wednesday, January 19, 2011 00:50:35 Laurent Pinchart wrote: > Hi Hans and Martin, > > On Wednesday 19 January 2011 00:05:10 Hans Verkuil wrote: > > On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: > > [snip] > > > > + return mt9m032_write_reg(client, MT9M032_VBLANK, > > > additional_blanking_rows); > > > > I've found it easier to do the v4l2_subdev to i2c_client conversion at the > > lowest level: the read/write register functions. That way the conversion is > > done at only a few places, rather than at every place these read/write reg > > functions are called. Just my opinion, though. > > I agree with this. > > > > +#ifdef CONFIG_VIDEO_ADV_DEBUG > > > +static long mt9m032_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void > > > *arg) +{ > > > + if (cmd == VIDIOC_DBG_G_REGISTER || cmd == VIDIOC_DBG_S_REGISTER) { > > > + struct v4l2_dbg_register *p = arg; > > > + > > > + if (!capable(CAP_SYS_ADMIN)) > > > + return -EPERM; > > > + > > > + if (cmd == VIDIOC_DBG_G_REGISTER) > > > + return v4l2_subdev_call(sd, core, g_register, p); > > > + else > > > + return v4l2_subdev_call(sd, core, s_register, p); > > > + } else { > > > + return -ENOIOCTLCMD; > > > + } > > > +} > > > > Huh? Ah, I get it. This is for when the user uses the subdev's device node > > directly. This is not good, the v4l2 framework should do translate this to > > g/s_register. > > Agreed. > > > The same should be done for g_chip_ident, I guess. > > I don't think we need g_chip_ident for subdev nodes, do we ? Why not? It makes the use of v4l2-dbg a bit easier if it is there. If you provide g/s_register, then you should provide this one as well. I though of another one that should be handled in the framework: VIDIOC_LOG_STATUS. That definitely should be handled as well. Regards, Hans > > > > +#endif > > -- Hans Verkuil - video4linux developer - sponsored by Cisco -- 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] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, Thanks for the patch. On Tuesday 18 January 2011 23:18:42 Martin Hostettler wrote: > The MT9M032 is a parallel 1284x812 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 > --- > drivers/media/video/Kconfig |7 + > drivers/media/video/Makefile|1 + > drivers/media/video/mt9m032.c | 834 > drivers/media/video/mt9m032.h | 38 ++ > include/media/v4l2-chip-ident.h |1 + > 5 files changed, 881 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/mt9m032.c > create mode 100644 drivers/media/video/mt9m032.h > [snip] > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c > new file mode 100644 > index 000..fe6af7b > --- /dev/null > +++ b/drivers/media/video/mt9m032.c > @@ -0,0 +1,834 @@ > +/* > + * 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 > +#include > +#include > + > +#include "mt9m032.h" > + > +#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 Kernel code usually uses lowercase hex constants. > +#define MT9M032_RESTART 0x0B > +#define MT9M032_RESET0x0D > +#define MT9M032_PLL_CONFIG1 0x11 > +#define MT9M032_READ_MODE1 0x1E > +#define MT9M032_READ_MODE2 0x20 > +#define MT9M032_GAIN_GREEN1 0x2B > +#define MT9M032_GAIN_BLUE0x2C > +#define MT9M032_GAIN_RED 0x2D > +#define MT9M032_GAIN_GREEN2 0x2E > +/* write only */ > +#define MT9M032_GAIN_ALL 0x35 > +#define MT9M032_FORMATTER1 0x9E > +#define MT9M032_FORMATTER2 0x9F > + > +#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, *gain, *exposure; > +}; > + > +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width) > +{ > + int effective_width; > + u64 ns; > + effective_width = width + 716; /* emperical value */ Where does it come from ? > + ns = 10ll * effective_width; > + do_div(ns, sensor->pix_clock); Do you have high enough clock frequencies that you would loose precision by dividing 1e9 by the clock first, and the multiplying it by the row length ? If so I would use div_u64(). > + dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns); > + return ns; > +} > + > +static int mt9m032_update_timing(struct mt9m032 *sensor, > + const struct v4l2_fract *interval, > + const struct v4l2_rect *crop) > +{ > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev); > + u64 ns = 10; /* 1 sec */ > + unsigned long row_time; > + int additional_blanking_rows; > + int min_blank; > + > + if (!interval) > + interval = &sensor->frame_interval; > + if (!crop) > + crop = &sensor->crop; > + > + ns = ns * interval->num
Re: [PATCH] v4l: Add driver for Micron MT9M032 camera sensor
Hi Hans and Martin, On Wednesday 19 January 2011 00:05:10 Hans Verkuil wrote: > On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: [snip] > > + return mt9m032_write_reg(client, MT9M032_VBLANK, > > additional_blanking_rows); > > I've found it easier to do the v4l2_subdev to i2c_client conversion at the > lowest level: the read/write register functions. That way the conversion is > done at only a few places, rather than at every place these read/write reg > functions are called. Just my opinion, though. I agree with this. > > +#ifdef CONFIG_VIDEO_ADV_DEBUG > > +static long mt9m032_ioctl(struct v4l2_subdev *sd, unsigned int cmd, void > > *arg) +{ > > + if (cmd == VIDIOC_DBG_G_REGISTER || cmd == VIDIOC_DBG_S_REGISTER) { > > + struct v4l2_dbg_register *p = arg; > > + > > + if (!capable(CAP_SYS_ADMIN)) > > + return -EPERM; > > + > > + if (cmd == VIDIOC_DBG_G_REGISTER) > > + return v4l2_subdev_call(sd, core, g_register, p); > > + else > > + return v4l2_subdev_call(sd, core, s_register, p); > > + } else { > > + return -ENOIOCTLCMD; > > + } > > +} > > Huh? Ah, I get it. This is for when the user uses the subdev's device node > directly. This is not good, the v4l2 framework should do translate this to > g/s_register. Agreed. > The same should be done for g_chip_ident, I guess. I don't think we need g_chip_ident for subdev nodes, do we ? > > +#endif -- 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] v4l: Add driver for Micron MT9M032 camera sensor
Hi Martin, On Tuesday, January 18, 2011 23:18:42 Martin Hostettler wrote: > The MT9M032 is a parallel 1284x812 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. Now, this is a truly bleeding edge driver :-) Pads aren't even merged yet! Got some small comments: > Signed-off-by: Martin Hostettler > --- > drivers/media/video/Kconfig |7 + > drivers/media/video/Makefile|1 + > drivers/media/video/mt9m032.c | 834 > +++ > drivers/media/video/mt9m032.h | 38 ++ > include/media/v4l2-chip-ident.h |1 + > 5 files changed, 881 insertions(+), 0 deletions(-) > create mode 100644 drivers/media/video/mt9m032.c > create mode 100644 drivers/media/video/mt9m032.h > > diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig > index 9fad1a6..f2d5f80 100644 > --- a/drivers/media/video/Kconfig > +++ b/drivers/media/video/Kconfig > @@ -773,6 +773,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 8f70b06..3e7299f 100644 > --- a/drivers/media/video/Makefile > +++ b/drivers/media/video/Makefile > @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o > obj-$(CONFIG_VIDEO_OV7670) += ov7670.o > obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o > obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o > +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o > obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o > obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o > obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o > diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c > new file mode 100644 > index 000..fe6af7b > --- /dev/null > +++ b/drivers/media/video/mt9m032.c > @@ -0,0 +1,834 @@ > +/* > + * 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 > +#include > +#include > + > +#include "mt9m032.h" > + > +#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_RESTART 0x0B > +#define MT9M032_RESET0x0D > +#define MT9M032_PLL_CONFIG1 0x11 > +#define MT9M032_READ_MODE1 0x1E > +#define MT9M032_READ_MODE2 0x20 > +#define MT9M032_GAIN_GREEN1 0x2B > +#define MT9M032_GAIN_BLUE0x2C > +#define MT9M032_GAIN_RED 0x2D > +#define MT9M032_GAIN_GREEN2 0x2E > +/* write only */ > +#define MT9M032_GAIN_ALL 0x35 > +#define MT9M032_FORMATTER1 0x9E > +#define MT9M032_FORMATTER2 0x9F > + > +#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
[PATCH] v4l: Add driver for Micron MT9M032 camera sensor
The MT9M032 is a parallel 1284x812 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 --- drivers/media/video/Kconfig |7 + drivers/media/video/Makefile|1 + drivers/media/video/mt9m032.c | 834 +++ drivers/media/video/mt9m032.h | 38 ++ include/media/v4l2-chip-ident.h |1 + 5 files changed, 881 insertions(+), 0 deletions(-) create mode 100644 drivers/media/video/mt9m032.c create mode 100644 drivers/media/video/mt9m032.h diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig index 9fad1a6..f2d5f80 100644 --- a/drivers/media/video/Kconfig +++ b/drivers/media/video/Kconfig @@ -773,6 +773,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 8f70b06..3e7299f 100644 --- a/drivers/media/video/Makefile +++ b/drivers/media/video/Makefile @@ -70,6 +70,7 @@ obj-$(CONFIG_VIDEO_UPD64083) += upd64083.o obj-$(CONFIG_VIDEO_OV7670) += ov7670.o obj-$(CONFIG_VIDEO_TCM825X) += tcm825x.o obj-$(CONFIG_VIDEO_TVEEPROM) += tveeprom.o +obj-$(CONFIG_VIDEO_MT9M032) += mt9m032.o obj-$(CONFIG_VIDEO_MT9T001) += mt9t001.o obj-$(CONFIG_VIDEO_MT9V011) += mt9v011.o obj-$(CONFIG_VIDEO_MT9V032) += mt9v032.o diff --git a/drivers/media/video/mt9m032.c b/drivers/media/video/mt9m032.c new file mode 100644 index 000..fe6af7b --- /dev/null +++ b/drivers/media/video/mt9m032.c @@ -0,0 +1,834 @@ +/* + * 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 +#include +#include + +#include "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_SIZE0x04 +#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_RESTART0x0B +#define MT9M032_RESET 0x0D +#define MT9M032_PLL_CONFIG10x11 +#define MT9M032_READ_MODE1 0x1E +#define MT9M032_READ_MODE2 0x20 +#define MT9M032_GAIN_GREEN10x2B +#define MT9M032_GAIN_BLUE 0x2C +#define MT9M032_GAIN_RED 0x2D +#define MT9M032_GAIN_GREEN20x2E +/* write only */ +#define MT9M032_GAIN_ALL 0x35 +#define MT9M032_FORMATTER1 0x9E +#define MT9M032_FORMATTER2 0x9F + +#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, *gain, *exposure; +}; + + +static int mt9m032_read_reg(struct i2c_client *client, const u8 reg) +{ + s32 data = i2c_smbus_read_word_data(client, reg); + return data < 0 ? data : swab16(data); +} + +static int mt9m032_write_reg(struct i2c_client *client, const u8 reg, +const u16 data) +{ + return i2c_s