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
