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 <martin@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > 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 0000000..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 <gwlund@xxxxxxxxxxx>
> > + * Author: Martin Hostettler <martin@xxxxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + */
> > +
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/math64.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/v4l2-mediabus.h>
> > +
> > +#include <media/media-entity.h>
> > +#include <media/v4l2-chip-ident.h>
>
> You can remove this header.
yes, another leftover.
>
> > +#include <media/v4l2-ctrls.h>
> > +#include <media/v4l2-device.h>
> > +#include <media/v4l2-subdev.h>
> > +
> > +#include <media/mt9m032.h>
> > +
> > +#define MT9M032_CHIP_VERSION 0x00
> > +#define MT9M032_ROW_START 0x01
> > +#define MT9M032_COLUMN_START 0x02
> > +#define MT9M032_ROW_SIZE 0x03
> > +#define MT9M032_COLUMN_SIZE 0x04
> > +#define MT9M032_HBLANK 0x05
> > +#define MT9M032_VBLANK 0x06
> > +#define MT9M032_SHUTTER_WIDTH_HIGH 0x08
> > +#define MT9M032_SHUTTER_WIDTH_LOW 0x09
> > +#define MT9M032_PIX_CLK_CTRL 0x0a
> > +#define MT9M032_PIX_CLK_CTRL_INV_PIXCLK 0x8000
> > +#define MT9M032_RESTART 0x0b
> > +#define MT9M032_RESET 0x0d
> > +#define MT9M032_PLL_CONFIG1 0x11
> > +#define MT9M032_PLL_CONFIG1_OUTDIV_MASK 0x3f
> > +#define MT9M032_PLL_CONFIG1_MUL_SHIFT 8
> > +#define MT9M032_READ_MODE1 0x1e
> > +#define MT9M032_READ_MODE2 0x20
> > +#define MT9M032_READ_MODE2_VFLIP_SHIFT 15
> > +#define MT9M032_READ_MODE2_HFLIP_SHIFT 14
> > +#define MT9M032_READ_MODE2_ROW_BLC 0x40
> > +#define MT9M032_GAIN_GREEN1 0x2b
> > +#define MT9M032_GAIN_BLUE 0x2c
> > +#define MT9M032_GAIN_RED 0x2d
> > +#define MT9M032_GAIN_GREEN2 0x2e
> > +/* write only */
> > +#define MT9M032_GAIN_ALL 0x35
> > +#define MT9M032_GAIN_DIGITAL_MASK 0x7f
> > +#define MT9M032_GAIN_DIGITAL_SHIFT 8
> > +#define MT9M032_GAIN_AMUL_SHIFT 6
> > +#define MT9M032_GAIN_ANALOG_MASK 0x3f
> > +#define MT9M032_FORMATTER1 0x9e
> > +#define MT9M032_FORMATTER2 0x9f
> > +#define MT9M032_FORMATTER2_DOUT_EN 0x1000
> > +#define MT9M032_FORMATTER2_PIXCLK_EN 0x2000
> > +
> > +#define MT9M032_MAX_BLANKING_ROWS 0x7ff
> > +
> > +#define to_mt9m032(sd) container_of(sd, struct mt9m032, subdev)
> > +#define to_dev(sensor) &((struct i2c_client
> > *)v4l2_get_subdevdata(&sensor->subdev))->dev +
> > +struct mt9m032 {
> > + struct v4l2_subdev subdev;
> > + struct media_pad pad;
> > + struct mt9m032_platform_data *pdata;
> > + struct v4l2_ctrl_handler ctrls;
> > +
> > + bool streaming;
> > +
> > + int pix_clock;
> > +
> > + struct v4l2_mbus_framefmt format; /* height and width always the same as
> > in crop */ + struct v4l2_rect crop;
> > + struct v4l2_fract frame_interval;
> > +
> > + struct v4l2_ctrl *hflip, *vflip;
> > +};
> > +
> > +
> > +static int mt9m032_read_reg(struct mt9m032 *sensor, const u8 reg)
>
> No need for the const keyword, this isn't a pointer :-)
It makes
reg = 4; a compile time error ;)
But yes, it's not really of much use. But lot's of these i2c register access
helpers have that const. It seems i have cargo culted that here.
NB: Maybe that code shouldn't be duplicated that much.
>
> > +{
> > + 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.
>
> > +}
> > +
> > +static int mt9m032_write_reg(struct mt9m032 *sensor, const u8 reg,
> > + const u16 data)
>
> No need for const either.
>
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + return i2c_smbus_write_word_data(client, reg, swab16(data));
>
> And cpu_to_be16() here.
see above.
>
> > +}
> > +
> > +
>
> I'm not a big fan of several blank lines (you even use 3 of them in
> mt9m032_probe) to separate code blocks. If you want to divide the code (which
> I find good for readability), using comments usually works better. I
> personally use
>
> /* ---------------------------------------------------------------------------
> * Foo Bar
> */
>
> to split drivers in sections.
>
> This is up to you.
I happen to really like blank lines. I removed a lot already because kernel folk
seems to be really like them all neat and minimal.
>
> > +static unsigned long mt9m032_row_time(struct mt9m032 *sensor, int width)
> > +{
> > + int effective_width;
> > + u64 ns;
> > +
> > + effective_width = width + 716; /* emperical value */
> > + ns = div_u64(((u64)1000000000) * effective_width, sensor->pix_clock);
> > + dev_dbg(to_dev(sensor), "MT9M032 line time: %llu ns\n", ns);
> > + return ns;
> > +}
> > +
> > +static int mt9m032_update_timing(struct mt9m032 *sensor,
> > + struct v4l2_fract *interval,
> > + const struct v4l2_rect *crop)
> > +{
> > + unsigned long row_time;
> > + int additional_blanking_rows;
> > + int min_blank;
>
> 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.
>
> > +
> > + 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. 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...
> > +
> > + row_time = mt9m032_row_time(sensor, crop->width);
> > +
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
> > interval->numerator, +
> > ((u64)interval->denominator) * row_time) + -
> > crop->height;
> > +
> > + if (additional_blanking_rows > MT9M032_MAX_BLANKING_ROWS) {
> > + /* hardware limits to 11 bit values */
> > + interval->denominator = 1000;
> > + interval->numerator = div_u64((crop->height + MT9M032_MAX_BLANKING_ROWS)
> > + * ((u64)row_time) * interval->denominator
> > + , 1000000000);
>
> You're exceeding the 80 columns limit anyway, the comma can be put at the end
> of the previous row.
yes
>
> > + additional_blanking_rows = div_u64(((u64)1000000000) *
> > interval->numerator,
> > + ((u64)interval->denominator) * row_time)
> > + - crop->height;
> > + }
> > + /* enforce minimal 1.6ms blanking time. */
> > + min_blank = 1600000 / row_time;
> > + additional_blanking_rows = clamp(additional_blanking_rows,
> > + min_blank, MT9M032_MAX_BLANKING_ROWS);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_VBLANK, additional_blanking_rows);
> > +}
> > +
> > +static int mt9m032_update_geom_timing(struct mt9m032 *sensor,
> > + const struct v4l2_rect *crop)
> > +{
> > + int ret;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_SIZE, crop->width - 1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_ROW_SIZE, crop->height - 1);
> > + /* offsets compensate for black border */
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_COLUMN_START, crop->left + 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...
>
> > + if (!ret)
> > + ret = mt9m032_update_timing(sensor, NULL, crop);
> > + return ret;
> > +}
> > +
> > +static int update_formatter2(struct mt9m032 *sensor, bool streaming)
> > +{
> > + u16 reg_val = MT9M032_FORMATTER2_DOUT_EN
> > + | 0x0070; /* parts reserved! */
> > + /* possibly for changing to 14-bit mode */
>
> Does the sensor support 14-bit output ?
Something with PLL, *muble* *muble*
Basicly here i tried to resuce possible information from a comment on a sample
bring-up code. Maybe it's just noise in the data sheet. Or maybe someone will
someday understand it. Hard to say with those sample sequences full of access to
undocumented bits with comments that don't really help.
>
> > +
> > + if (streaming)
> > + reg_val |= MT9M032_FORMATTER2_PIXCLK_EN; /* pixclock enable */
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_FORMATTER2, reg_val);
> > +}
> > +
> > +static int mt9m032_s_stream(struct v4l2_subdev *subdev, int streaming)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + ret = update_formatter2(sensor, streaming);
> > + if (!ret)
> > + sensor->streaming = streaming;
> > + return ret;
> > +}
> > +
> > +static int mt9m032_enum_mbus_code(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_mbus_code_enum *code)
> > +{
> > + if (code->index != 0 || code->pad != 0)
> > + return -EINVAL;
> > + code->code = V4L2_MBUS_FMT_Y8_1X8;
> > + return 0;
> > +}
> > +
> > +static int mt9m032_enum_frame_size(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_frame_size_enum *fse)
> > +{
> > + if (fse->index != 0 || fse->code != V4L2_MBUS_FMT_Y8_1X8 || fse->pad !=
> > 0) + return -EINVAL;
> > +
> > + fse->min_width = 32;
> > + fse->max_width = 1440;
>
> Shouldn't that be 1472 ?
This driver consistently hides black pixels from userspace. And the active image
is 1440x1080 according to the datasheet.
>
> > + fse->min_height = 32;
> > + fse->max_height = 1096;
>
> You don't support binning/skipping, so I think min width/height should be
> equal to max width/height.
But it supports cropping, so the size of the data processed is variable, isn't it?
>
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_crop() - get crop rect
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try crop rect from
> > + * @which: select try or active crop rect
> > + * Returns a pointer the current active or fh relative try crop rect
> > + */
> > +static struct v4l2_rect *__mt9m032_get_pad_crop(struct mt9m032 *sensor,
> > + struct v4l2_subdev_fh *fh,
> > + u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_crop(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->crop;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +/**
> > + * __mt9m032_get_pad_format() - get format
> > + * @sensor: pointer to the sensor struct
> > + * @fh: filehandle for getting the try format from
> > + * @which: select try or active format
> > + * Returns a pointer the current active or fh relative try format
> > + */
> > +static struct v4l2_mbus_framefmt *__mt9m032_get_pad_format(struct mt9m032
> > *sensor, + struct v4l2_subdev_fh *fh,
> > + u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_format(fh, 0);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &sensor->format;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +static int mt9m032_get_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt *format;
> > +
> > + if (fmt->pad != 0)
> > + return -EINVAL;
>
> fmt->pad is already validated by the core. Same comment for the set operation.
Nice, removed for all {get,set}_{format,crop}.
>
> > + format = __mt9m032_get_pad_format(sensor, fh, fmt->which);
> > + if (format == NULL)
> > + return -EINVAL;
>
> fmt->which is validated by the core as well, so __mt9m032_get_pad_format()
> can't return NULL. Same for cropping.
I guess if fh->try_* isn't set it wouldn't be NULL, so yes i'll remove
this too.
>
> > +
> > + fmt->format = *format;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_pad_format(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_fh *fh,
> > + struct v4l2_subdev_format *fmt)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + if (fmt->pad != 0)
> > + return -EINVAL;
> > +
> > +
> > + /*
> > + * fmt->format.colorspace, fmt->format.code and fmt->format.field are
> > ignored
> > + * and thus forced to fixed values by the get call below.
> > + *
> > + * fmt->format.width, fmt->format.height are forced to the values set via
> > crop
> > + */
> > +
> > + return mt9m032_get_pad_format(subdev, fh, fmt);
> > +}
> > +
> > +static int mt9m032_get_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_rect *curcrop;
> > +
> > + if (crop->pad != 0)
> > + return -EINVAL;
> > + curcrop = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > + if (!curcrop)
> > + return -EINVAL;
> > +
> > + crop->rect = *curcrop;
> > +
> > + return 0;
> > +}
> > +
> > +
> > +static int mt9m032_set_crop(struct v4l2_subdev *subdev, struct
> > v4l2_subdev_fh *fh, + struct v4l2_subdev_crop *crop)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + struct v4l2_mbus_framefmt tmp_format;
> > + struct v4l2_rect tmp_crop_rect;
> > + struct v4l2_mbus_framefmt *format;
> > + struct v4l2_rect *crop_rect;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + if (crop->pad != 0)
> > + return -EINVAL;
> > +
> > + format = __mt9m032_get_pad_format(sensor, fh, crop->which);
> > + crop_rect = __mt9m032_get_pad_crop(sensor, fh, crop->which);
> > + if (!format || !crop_rect)
> > + return -EINVAL;
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + tmp_crop_rect = *crop_rect;
> > + tmp_format = *format;
> > + format = &tmp_format;
> > + crop_rect = &tmp_crop_rect;
> > + }
> > +
> > + crop_rect->top = crop->rect.top & ~0x1;
> > + crop_rect->left = crop->rect.left;
> > + crop_rect->height = crop->rect.height;
> > + crop_rect->width = crop->rect.width & ~1;
>
> You should validate the crop rectangle here.
Yes, that can't harm, can it?
>
> > + format->height = crop_rect->height;
> > + format->width = crop_rect->width;
> > +
> > + if (crop->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > + int ret = mt9m032_update_geom_timing(sensor, crop_rect);
>
> As the format can't be changed during streaming, what about moving this to the
> s_stream handler ? You could then also remove the call from the probe()
> function.
I'd rather not. The driver should change the minimal registers needed for each
operation, to keep the option to do things with the generic register set ioctls.
(Not my idea, but worth to argue over)
>
> > +
> > + if (!ret) {
> > + sensor->crop = tmp_crop_rect;
> > + sensor->format = tmp_format;
> > + }
> > + return ret;
> > + }
> > +
> > + return mt9m032_get_crop(subdev, fh, crop);
>
> I don't think you need that.
I generally like the idea of having the setters return exactly what the getters
return explicitly. Just a case of programmer doesn't trust himself.
>
> > +}
> > +
> > +static int mt9m032_get_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + fi->pad = 0;
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > + fi->interval = sensor->frame_interval;
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_frame_interval(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_frame_interval *fi)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > + int ret;
> > +
> > + if (sensor->streaming)
> > + return -EBUSY;
> > +
> > + memset(fi->reserved, 0, sizeof(fi->reserved));
> > +
> > + ret = mt9m032_update_timing(sensor, &fi->interval, NULL);
> > + if (!ret)
> > + sensor->frame_interval = fi->interval;
> > + return ret;
> > +}
> > +
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > +static int mt9m032_g_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > + int val;
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
>
> Do you think those checks should be kept when using the MC API ?
I think they are useful enough to prevent accidentially poking the wrong
hardware. Also it's always good to force userspace to initialize
everything.
>
> > + val = mt9m032_read_reg(sensor, reg->reg);
> > + if (val < 0)
> > + return -EIO;
> > +
> > + reg->size = 2;
> > + reg->val = (u64) val;
>
> Is there a need for an explicit cast ?
prevents sign extension, iirc. val ist signed because mt9m032_read_reg
multiplexes error codes in negative values.
>
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_s_register(struct v4l2_subdev *sd,
> > + struct v4l2_dbg_register *reg)
> > +{
> > + struct mt9m032 *sensor = to_mt9m032(sd);
> > + struct i2c_client *client = v4l2_get_subdevdata(&sensor->subdev);
> > +
> > + if (reg->match.type != V4L2_CHIP_MATCH_I2C_ADDR || reg->reg > 0xff)
> > + return -EINVAL;
> > +
> > + if (reg->match.addr != client->addr)
> > + return -ENODEV;
> > +
> > + if (mt9m032_write_reg(sensor, reg->reg, reg->val) < 0)
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int update_read_mode2(struct mt9m032 *sensor, bool vflip, bool
> > hflip) +{
> > + int reg_val = (!!vflip) << MT9M032_READ_MODE2_VFLIP_SHIFT
> > + | (!!hflip) << MT9M032_READ_MODE2_HFLIP_SHIFT
> > + | MT9M032_READ_MODE2_ROW_BLC
> > + | 0x0007;
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_READ_MODE2, reg_val);
> > +}
> > +
> > +static int mt9m032_set_hflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, sensor->vflip->cur.val, val);
> > +}
> > +
> > +static int mt9m032_set_vflip(struct mt9m032 *sensor, s32 val)
> > +{
> > + return update_read_mode2(sensor, val, sensor->hflip->cur.val);
> > +}
> > +
> > +static int mt9m032_set_exposure(struct mt9m032 *sensor, s32 val)
> > +{
> > + int shutter_width;
> > + u16 high_val, low_val;
> > + int ret;
> > +
> > + /* shutter width is in row times */
> > + shutter_width = (val * 1000) / mt9m032_row_time(sensor,
> > sensor->crop.width); +
> > + high_val = (shutter_width >> 16) & 0xf;
> > + low_val = shutter_width & 0xffff;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_HIGH, high_val);
> > + if (!ret)
> > + mt9m032_write_reg(sensor, MT9M032_SHUTTER_WIDTH_LOW, low_val);
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_set_gain(struct mt9m032 *sensor, s32 val)
> > +{
> > + int digital_gain_val; /* in 1/8th (0..127) */
> > + int analog_mul; /* 0 or 1 */
> > + int analog_gain_val; /* in 1/16th. (0..63) */
> > + u16 reg_val;
> > +
> > + digital_gain_val = 51; /* from setup example */
> > +
> > + if (val < 63) {
> > + analog_mul = 0;
> > + analog_gain_val = val;
> > + } else {
> > + analog_mul = 1;
> > + analog_gain_val = val / 2;
> > + }
> > +
> > + /* a_gain = (1+analog_mul) + (analog_gain_val+1)/16 */
> > + /* overall_gain = a_gain * (1 + digital_gain_val / 8) */
> > +
> > + reg_val = (digital_gain_val & MT9M032_GAIN_DIGITAL_MASK) <<
> > MT9M032_GAIN_DIGITAL_SHIFT + | (analog_mul & 1) <<
> > MT9M032_GAIN_AMUL_SHIFT
> > + | (analog_gain_val & MT9M032_GAIN_ANALOG_MASK);
> > +
> > + return mt9m032_write_reg(sensor, MT9M032_GAIN_ALL, reg_val);
> > +}
> > +
> > +static int mt9m032_setup_pll(struct mt9m032 *sensor)
> > +{
> > + struct mt9m032_platform_data* pdata = sensor->pdata;
> > + u16 reg_pll1;
> > + unsigned int pre_div;
> > + int res, ret;
> > +
> > + /* TODO: also support other pre-div values */
> > + if (pdata->pll_pre_div != 6) {
> > + dev_warn(to_dev(sensor),
> > + "Unsupported PLL pre-divisor value %u, using default 6\n",
> > + pdata->pll_pre_div);
> > + }
> > + pre_div = 6;
>
> The mt9p031 driver also needs to setup a PLL. I think it's time to compute the
> PLL parameters at runtime instead of relying on board code.
Problem here is that i can't really understand the general case PLL setup from the
datasheet. So it's hard to really do it fully dynamic.
>
> > +
> > + sensor->pix_clock = pdata->ext_clock * pdata->pll_mul /
> > + (pre_div * pdata->pll_out_div);
> > +
> > + reg_pll1 = ((pdata->pll_out_div - 1) & MT9M032_PLL_CONFIG1_OUTDIV_MASK)
> > + | pdata->pll_mul << MT9M032_PLL_CONFIG1_MUL_SHIFT;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_PLL_CONFIG1, reg_pll1);
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, 0x10, 0x53); /* Select PLL as clock
> > source */ +
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_READ_MODE1, 0x8006);
> > + /* more reserved, Continuous */
> > + /* Master Mode */
> > + if (!ret)
> > + res = mt9m032_read_reg(sensor, MT9M032_READ_MODE1);
> > +
> > + if (!ret)
> > + ret = mt9m032_write_reg(sensor, MT9M032_FORMATTER1, 0x111e);
> > + /* Set 14-bit mode, select 7 divider */
> > +
> > + return ret;
> > +}
> > +
> > +static int mt9m032_try_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + if (ctrl->id == V4L2_CID_GAIN && ctrl->val >= 63) {
> > + /* round because of multiplier used for values >= 63 */
> > + ctrl->val &= ~1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int mt9m032_set_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > + struct mt9m032 *sensor = container_of(ctrl->handler, struct mt9m032,
> > ctrls); +
> > + switch (ctrl->id) {
> > + case V4L2_CID_GAIN:
> > + return mt9m032_set_gain(sensor, ctrl->val);
> > +
> > + case V4L2_CID_HFLIP:
> > + return mt9m032_set_hflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_VFLIP:
> > + return mt9m032_set_vflip(sensor, ctrl->val);
> > +
> > + case V4L2_CID_EXPOSURE:
> > + return mt9m032_set_exposure(sensor, ctrl->val);
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +}
> > +
> > +static const struct v4l2_subdev_video_ops mt9m032_video_ops = {
> > + .s_stream = mt9m032_s_stream,
> > + .g_frame_interval = mt9m032_get_frame_interval,
> > + .s_frame_interval = mt9m032_set_frame_interval,
> > +};
> > +
> > +static struct v4l2_ctrl_ops mt9m032_ctrl_ops = {
> > + .s_ctrl = mt9m032_set_ctrl,
> > + .try_ctrl = mt9m032_try_ctrl,
> > +};
> > +
> > +
> > +static const struct v4l2_subdev_core_ops mt9m032_core_ops = {
> > +#ifdef CONFIG_VIDEO_ADV_DEBUG
> > + .g_register = mt9m032_g_register,
> > + .s_register = mt9m032_s_register,
> > +#endif
> > +};
> > +
> > +static const struct v4l2_subdev_pad_ops mt9m032_pad_ops = {
> > + .enum_mbus_code = mt9m032_enum_mbus_code,
> > + .enum_frame_size = mt9m032_enum_frame_size,
> > + .get_fmt = mt9m032_get_pad_format,
> > + .set_fmt = mt9m032_set_pad_format,
> > + .set_crop = mt9m032_set_crop,
> > + .get_crop = mt9m032_get_crop,
> > +};
> > +
> > +static const struct v4l2_subdev_ops mt9m032_ops = {
> > + .core = &mt9m032_core_ops,
> > + .video = &mt9m032_video_ops,
> > + .pad = &mt9m032_pad_ops,
> > +};
> > +
> > +static int mt9m032_probe(struct i2c_client *client,
> > + const struct i2c_device_id *devid)
> > +{
> > + struct mt9m032 *sensor;
> > + int chip_version;
> > + int res, ret;
> > +
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA))
> > { + dev_warn(&client->adapter->dev,
> > + "I2C-Adapter doesn't support I2C_FUNC_SMBUS_WORD\n");
> > + return -EIO;
> > + }
> > +
> > + if (!client->dev.platform_data)
> > + return -ENODEV;
> > +
> > + sensor = kzalloc(sizeof(*sensor), GFP_KERNEL);
> > + if (sensor == NULL)
> > + return -ENOMEM;
> > +
> > + sensor->pdata = client->dev.platform_data;
> > +
> > + v4l2_i2c_subdev_init(&sensor->subdev, client, &mt9m032_ops);
> > + sensor->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > +
> > +
> > + /*
> > + * This driver was developed with a camera module with seperate external
> > + * pix clock. For setups which use the clock from the camera interface
> > + * the code will need to be extended with the appropriate platform
> > + * callback to setup the clock.
> > + */
> > + chip_version = mt9m032_read_reg(sensor, MT9M032_CHIP_VERSION);
> > + if (0x1402 == chip_version) {
> > + dev_info(&client->dev, "mt9m032: detected sensor.\n");
> > + } else {
> > + dev_warn(&client->dev, "mt9m032: error: detected unsupported chip
> > version 0x%x\n", + chip_version);
> > + ret = -ENODEV;
> > + goto free_sensor;
> > + }
> > +
> > +
> > +
> > + sensor->frame_interval.numerator = 1;
> > + sensor->frame_interval.denominator = 30;
> > +
> > + sensor->crop.left = 416;
> > + sensor->crop.top = 360;
> > + sensor->crop.width = 640;
> > + sensor->crop.height = 480;
> > +
> > + sensor->format.width = sensor->crop.width;
> > + sensor->format.height = sensor->crop.height;
> > + sensor->format.code = V4L2_MBUS_FMT_Y8_1X8;
> > + sensor->format.field = V4L2_FIELD_NONE;
> > + sensor->format.colorspace = V4L2_COLORSPACE_SRGB;
> > +
> > + v4l2_ctrl_handler_init(&sensor->ctrls, 4);
> > +
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_GAIN, 0, 127, 1, 64);
> > +
> > + sensor->hflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + sensor->vflip = v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > + v4l2_ctrl_new_std(&sensor->ctrls, &mt9m032_ctrl_ops,
> > + V4L2_CID_EXPOSURE, 0, 8000, 1, 1700); /* 1.7ms */
> > +
> > +
> > + if (sensor->ctrls.error) {
> > + ret = sensor->ctrls.error;
> > + dev_err(&client->dev, "control initialization error %d\n", ret);
> > + goto free_ctrl;
> > + }
> > +
> > + sensor->subdev.ctrl_handler = &sensor->ctrls;
> > + sensor->pad.flags = MEDIA_PAD_FL_SOURCE;
> > + ret = media_entity_init(&sensor->subdev.entity, 1, &sensor->pad, 0);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESET, 1); /* reset on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + mt9m032_write_reg(sensor, MT9M032_RESET, 0); /* reset off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_setup_pll(sensor);
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(10);
> > +
> > + v4l2_ctrl_handler_setup(&sensor->ctrls);
> > +
> > + /* SIZE */
> > + ret = mt9m032_update_geom_timing(sensor, &sensor->crop);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + ret = mt9m032_write_reg(sensor, 0x41, 0x0000); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x42, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x43, 0x0003); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + ret = mt9m032_write_reg(sensor, 0x7f, 0x0000); /* reserved !!! */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + if (sensor->pdata->invert_pixclock) {
> > + mt9m032_write_reg(sensor, MT9M032_PIX_CLK_CTRL,
> > MT9M032_PIX_CLK_CTRL_INV_PIXCLK); + if (ret < 0)
> > + goto free_ctrl;
> > + }
> > +
> > + res = mt9m032_read_reg(sensor, MT9M032_PIX_CLK_CTRL);
> > +
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 1); /* Restart on */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = mt9m032_write_reg(sensor, MT9M032_RESTART, 0); /* Restart off */
> > + if (ret < 0)
> > + goto free_ctrl;
> > + msleep(100);
> > + ret = update_formatter2(sensor, false);
> > + if (ret < 0)
> > + goto free_ctrl;
> > +
> > + return ret;
> > +
> > +free_ctrl:
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > +
> > +free_sensor:
> > + kfree(sensor);
> > + return ret;
> > +}
> > +
> > +static int mt9m032_remove(struct i2c_client *client)
> > +{
> > + struct v4l2_subdev *subdev = i2c_get_clientdata(client);
> > + struct mt9m032 *sensor = to_mt9m032(subdev);
> > +
> > + v4l2_device_unregister_subdev(&sensor->subdev);
> > + v4l2_ctrl_handler_free(&sensor->ctrls);
> > + media_entity_cleanup(&sensor->subdev.entity);
> > + kfree(sensor);
> > + return 0;
> > +}
> > +
> > +static const struct i2c_device_id mt9m032_id_table[] = {
> > + {MT9M032_NAME, 0},
> > + {}
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, mt9m032_id_table);
> > +
> > +static struct i2c_driver mt9m032_i2c_driver = {
> > + .driver = {
> > + .name = MT9M032_NAME,
> > + },
> > + .probe = mt9m032_probe,
> > + .remove = mt9m032_remove,
> > + .id_table = mt9m032_id_table,
> > +};
> > +
> > +static int __init mt9m032_init(void)
> > +{
> > + int rval;
> > +
> > + rval = i2c_add_driver(&mt9m032_i2c_driver);
> > + if (rval)
> > + pr_err("%s: failed registering " MT9M032_NAME "\n", __func__);
> > +
> > + return rval;
> > +}
> > +
> > +static void mt9m032_exit(void)
> > +{
> > + i2c_del_driver(&mt9m032_i2c_driver);
> > +}
> > +
> > +module_init(mt9m032_init);
> > +module_exit(mt9m032_exit);
> > +
> > +MODULE_AUTHOR("Martin Hostettler");
> > +MODULE_DESCRIPTION("MT9M032 camera sensor driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/media/mt9m032.h b/include/media/mt9m032.h
> > new file mode 100644
> > index 0000000..a473af4
> > --- /dev/null
> > +++ b/include/media/mt9m032.h
> > @@ -0,0 +1,38 @@
> > +/*
> > + * Driver for MT9M032 CMOS Image Sensor from Micron
> > + *
> > + * Copyright (C) 2010-2011 Lund Engineering
> > + * Contact: Gil Lund <gwlund@xxxxxxxxxxx>
> > + * Author: Martin Hostettler <martin@xxxxxxxxxxxxxxxxxxxxxx>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > + * General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> > + * 02110-1301 USA
> > + *
> > + */
> > +
> > +#ifndef MT9M032_H
> > +#define MT9M032_H
> > +
> > +#define MT9M032_NAME "mt9m032"
> > +#define MT9M032_I2C_ADDR (0xB8 >> 1)
>
> We tend to use lower-case hex constants in the kernel.
can do.
>
> > +
> > +struct mt9m032_platform_data {
> > + u32 ext_clock;
> > + u32 pll_pre_div;
> > + u32 pll_mul;
> > + u32 pll_out_div;
> > + int invert_pixclock;
> > +
> > +};
> > +#endif /* MT9M032_H */
> > diff --git a/include/media/v4l2-chip-ident.h
> > b/include/media/v4l2-chip-ident.h index 63fd9d3..384967f 100644
> > --- a/include/media/v4l2-chip-ident.h
> > +++ b/include/media/v4l2-chip-ident.h
> > @@ -290,6 +290,7 @@ enum {
> > /* Micron CMOS sensor chips: 45000-45099 */
> > V4L2_IDENT_MT9M001C12ST = 45000,
> > V4L2_IDENT_MT9M001C12STM = 45005,
> > + V4L2_IDENT_MT9M032 = 45006,
>
> This isn't needed anymore.
gone
>
> > V4L2_IDENT_MT9M111 = 45007,
> > V4L2_IDENT_MT9M112 = 45008,
> > V4L2_IDENT_MT9V022IX7ATC = 45010, /* No way to detect "normal" I77ATx */
>
> --
> Regards,
>
> Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
