Hi,
not looking at v4l2 part since it's not my area...
On Tue, Mar 03, 2009 at 09:44:14PM +0100, ext Aguirre Rodriguez, Sergio Alberto
wrote:
> +#define SENSOR_DETECTED 1
> +#define SENSOR_NOT_DETECTED 0
these two should be unneeded...
> +
> +/**
> + * struct mt9p012_reg - mt9p012 register format
> + * @length: length of the register
> + * @reg: 16-bit offset to register
> + * @val: 8/16/32-bit register value
> + *
> + * Define a structure for MT9P012 register initialization values
> + */
> +struct mt9p012_reg {
> + u16 length;
> + u16 reg;
> + u32 val;
> +};
> +
> +enum image_size {
> + BIN4XSCALE,
> + BIN4X,
> + BIN2X,
> + THREE_MP,
> + FIVE_MP
you probably wanna prefix these with MT9P012_ to avoid namespace
conflicts.
> +};
> +
> +enum pixel_format {
> + RAWBAYER10
> +};
> +
> +#define NUM_IMAGE_SIZES 5
> +#define NUM_PIXEL_FORMATS 1
> +#define NUM_FPS 2 /* 2 ranges */
> +#define FPS_LOW_RANGE 0
> +#define FPS_HIGH_RANGE 1
> +
> +/**
> + * struct capture_size - image capture size information
> + * @width: image width in pixels
> + * @height: image height in pixels
> + */
> +struct capture_size {
> + unsigned long width;
> + unsigned long height;
> +};
> +
> +/**
> + * struct mt9p012_pll_settings - struct for storage of sensor pll values
> + * @vt_pix_clk_div: vertical pixel clock divider
> + * @vt_sys_clk_div: veritcal system clock divider
> + * @pre_pll_div: pre pll divider
> + * @fine_int_tm: fine resolution interval time
> + * @frame_lines: number of lines in frame
> + * @line_len: number of pixels in line
> + * @min_pll: minimum pll multiplier
> + * @max_pll: maximum pll multiplier
> + */
> +struct mt9p012_pll_settings {
> + u16 vt_pix_clk_div;
> + u16 vt_sys_clk_div;
> + u16 pre_pll_div;
> +
> + u16 fine_int_tm;
> + u16 frame_lines;
> + u16 line_len;
> +
> + u16 min_pll;
> + u16 max_pll;
> +};
> +
> +/*
> + * Array of image sizes supported by MT9P012. These must be ordered from
> + * smallest image size to largest.
> + */
> +const static struct capture_size mt9p012_sizes[] = {
> + { 216, 162 }, /* 4X BINNING+SCALING */
> + { 648, 486 }, /* 4X BINNING */
> + { 1296, 972 }, /* 2X BINNING */
> + { 2048, 1536}, /* 3 MP */
> + { 2592, 1944}, /* 5 MP */
> +};
> +
> +/* PLL settings for MT9P012 */
> +enum mt9p012_pll_type {
> + PLL_5MP = 0,
> + PLL_3MP,
> + PLL_1296_15FPS,
> + PLL_1296_30FPS,
> + PLL_648_15FPS,
> + PLL_648_30FPS,
> + PLL_216_15FPS,
> + PLL_216_30FPS
> +};
missing tabs, fix identation.
> +
> +/* Debug functions */
> +static int debug;
> +module_param(debug, bool, 0644);
> +MODULE_PARM_DESC(debug, "Debug level (0-1)");
if it's a bool it's not debug level, it's debug on/off switch :-p
> +static struct mt9p012_sensor mt9p012;
> +static struct i2c_driver mt9p012sensor_i2c_driver;
unneeded.
> +static unsigned long xclk_current = MT9P012_XCLK_NOM_1;
why ??
> +static int
> +find_vctrl(int id)
I guess it fits in one line...
> +static int
> +mt9p012_read_reg(struct i2c_client *client, u16 data_length, u16 reg, u32
> *val)
> +{
> + int err;
> + struct i2c_msg msg[1];
> + unsigned char data[4];
> +
> + if (!client->adapter)
> + return -ENODEV;
> + if (data_length != MT9P012_8BIT && data_length != MT9P012_16BIT
> + && data_length != MT9P012_32BIT)
> + return -EINVAL;
> +
> + msg->addr = client->addr;
> + msg->flags = 0;
> + msg->len = 2;
> + msg->buf = data;
> +
> + /* high byte goes out first */
> + data[0] = (u8) (reg >> 8);;
> + data[1] = (u8) (reg & 0xff);
> + err = i2c_transfer(client->adapter, msg, 1);
> + if (err >= 0) {
> + msg->len = data_length;
> + msg->flags = I2C_M_RD;
> + err = i2c_transfer(client->adapter, msg, 1);
> + }
> + if (err >= 0) {
> + *val = 0;
> + /* high byte comes first */
> + if (data_length == MT9P012_8BIT)
> + *val = data[0];
> + else if (data_length == MT9P012_16BIT)
> + *val = data[1] + (data[0] << 8);
> + else
> + *val = data[3] + (data[2] << 8) +
> + (data[1] << 16) + (data[0] << 24);
> + return 0;
> + }
> + dev_err(&client->dev, "read from offset 0x%x error %d", reg, err);
doesn't this chip support smbus ?? It would be a lot simpler if it
does... :-s
> +static int ioctl_s_power(struct v4l2_int_device *s, enum v4l2_power on)
> +{
> + struct mt9p012_sensor *sensor = s->priv;
> + struct i2c_client *c = sensor->i2c_client;
> + int rval;
> +
> + if ((on == V4L2_POWER_STANDBY) && (sensor->state == SENSOR_DETECTED))
> + mt9p012_write_regs(c, stream_off_list);
> +
> + if (on != V4L2_POWER_ON)
> + sensor->pdata->set_xclk(0);
> + else
> + sensor->pdata->set_xclk(xclk_current);
I guess this should be clk_enable() and clk_disabled() calls.
> +
> + rval = sensor->pdata->power_set(on);
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to set the power state: " DRIVER_NAME
> + " sensor\n");
dev_err() should already hold the driver name. This could be changed to:
dev_err(&c->dev, "Unable to set the power state, err %d\n"), rval);
> + sensor->pdata->set_xclk(0);
> + return rval;
> + }
> +
> + if ((on == V4L2_POWER_ON) && (sensor->state == SENSOR_DETECTED))
> + mt9p012_configure(s);
> +
> + if ((on == V4L2_POWER_ON) && (sensor->state == SENSOR_NOT_DETECTED)) {
> + rval = mt9p012_detect(c);
this should be called during probe() and if it fails you bail out...
otherwise the device will always be available, I guess...
> + if (rval < 0) {
> + dev_err(&c->dev, "Unable to detect " DRIVER_NAME
> + " sensor\n");
> + sensor->state = SENSOR_NOT_DETECTED;
> + return rval;
> + }
> + sensor->state = SENSOR_DETECTED;
> + sensor->ver = rval;
> + pr_info(DRIVER_NAME " chip version 0x%02x detected\n",
> + sensor->ver);
no pr_info, use dev_dbg();
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * ioctl_init - V4L2 sensor interface handler for VIDIOC_INT_INIT
> + * @s: pointer to standard V4L2 device structure
> + *
> + * Initialize the sensor device (call mt9p012_configure())
> + */
> +static int ioctl_init(struct v4l2_int_device *s)
> +{
> + return 0;
> +}
> +
> +/**
> + * ioctl_dev_exit - V4L2 sensor interface handler for vidioc_int_dev_exit_num
> + * @s: pointer to standard V4L2 device structure
> + *
> + * Delinitialise the dev. at slave detach. The complement of ioctl_dev_init.
> + */
> +static int ioctl_dev_exit(struct v4l2_int_device *s)
> +{
> + return 0;
> +}
> +
> +/**
> + * ioctl_dev_init - V4L2 sensor interface handler for vidioc_int_dev_init_num
> + * @s: pointer to standard V4L2 device structure
> + *
> + * Initialise the device when slave attaches to the master. Returns 0 if
> + * mt9p012 device could be found, otherwise returns appropriate error.
> + */
> +static int ioctl_dev_init(struct v4l2_int_device *s)
> +{
> + return 0;
> +}
> +/**
> + * ioctl_enum_framesizes - V4L2 sensor if handler for
> vidioc_int_enum_framesizes
> + * @s: pointer to standard V4L2 device structure
> + * @frms: pointer to standard V4L2 framesizes enumeration structure
> + *
> + * Returns possible framesizes depending on choosen pixel format
> + **/
> +static int ioctl_enum_framesizes(struct v4l2_int_device *s,
> + struct v4l2_frmsizeenum *frms)
> +{
> + int ifmt;
> +
> + for (ifmt = 0; ifmt < NUM_CAPTURE_FORMATS; ifmt++) {
> + if (frms->pixel_format == mt9p012_formats[ifmt].pixelformat)
> + break;
> + }
> + /* Is requested pixelformat not found on sensor? */
> + if (ifmt == NUM_CAPTURE_FORMATS)
> + return -EINVAL;
> +
> + /* Do we already reached all discrete framesizes? */
> + if (frms->index >= 5)
> + return -EINVAL;
> +
> + frms->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + frms->discrete.width = mt9p012_sizes[frms->index].width;
> + frms->discrete.height = mt9p012_sizes[frms->index].height;
> +
> + return 0;
> +}
> +
> +const struct v4l2_fract mt9p012_frameintervals[] = {
> + { .numerator = 1, .denominator = 11 },
> + { .numerator = 1, .denominator = 15 },
> + { .numerator = 1, .denominator = 20 },
> + { .numerator = 1, .denominator = 25 },
> + { .numerator = 1, .denominator = 30 },
> +};
> +
> +static int ioctl_enum_frameintervals(struct v4l2_int_device *s,
> + struct v4l2_frmivalenum *frmi)
> +{
> + int ifmt;
> +
> + for (ifmt = 0; ifmt < NUM_CAPTURE_FORMATS; ifmt++) {
> + if (frmi->pixel_format == mt9p012_formats[ifmt].pixelformat)
> + break;
> + }
> + /* Is requested pixelformat not found on sensor? */
> + if (ifmt == NUM_CAPTURE_FORMATS)
> + return -EINVAL;
> +
> + /* Do we already reached all discrete framesizes? */
> +
> + if (((frmi->width == mt9p012_sizes[4].width) &&
> + (frmi->height == mt9p012_sizes[4].height)) ||
> + ((frmi->width == mt9p012_sizes[3].width) &&
> + (frmi->height == mt9p012_sizes[3].height))) {
> + /* FIXME: The only frameinterval supported by 5MP and 3MP
> + * capture sizes is 1/11 fps
> + */
> + if (frmi->index != 0)
> + return -EINVAL;
> + } else {
> + if (frmi->index >= 5)
> + return -EINVAL;
> + }
> +
> + frmi->type = V4L2_FRMSIZE_TYPE_DISCRETE;
> + frmi->discrete.numerator =
> + mt9p012_frameintervals[frmi->index].numerator;
> + frmi->discrete.denominator =
> +
> mt9p012_frameintervals[frmi->index].denominator;
> +
> + return 0;
> +}
> +
> +static struct v4l2_int_ioctl_desc mt9p012_ioctl_desc[] = {
> + { .num = vidioc_int_enum_framesizes_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_enum_framesizes },
> + { .num = vidioc_int_enum_frameintervals_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_enum_frameintervals },
> + { .num = vidioc_int_dev_init_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_dev_init },
> + { .num = vidioc_int_dev_exit_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_dev_exit },
> + { .num = vidioc_int_s_power_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_power },
> + { .num = vidioc_int_g_priv_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_priv },
> + { .num = vidioc_int_init_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_init },
> + { .num = vidioc_int_enum_fmt_cap_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_enum_fmt_cap },
> + { .num = vidioc_int_try_fmt_cap_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_try_fmt_cap },
> + { .num = vidioc_int_g_fmt_cap_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_fmt_cap },
> + { .num = vidioc_int_s_fmt_cap_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_fmt_cap },
> + { .num = vidioc_int_g_parm_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_parm },
> + { .num = vidioc_int_s_parm_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_parm },
> + { .num = vidioc_int_queryctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_queryctrl },
> + { .num = vidioc_int_g_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_g_ctrl },
> + { .num = vidioc_int_s_ctrl_num,
> + .func = (v4l2_int_ioctl_func *)ioctl_s_ctrl },
> +};
> +
> +static struct v4l2_int_slave mt9p012_slave = {
> + .ioctls = mt9p012_ioctl_desc,
> + .num_ioctls = ARRAY_SIZE(mt9p012_ioctl_desc),
> +};
> +
> +static struct v4l2_int_device mt9p012_int_device = {
> + .module = THIS_MODULE,
> + .name = DRIVER_NAME,
> + .priv = &mt9p012,
> + .type = v4l2_int_type_slave,
> + .u = {
> + .slave = &mt9p012_slave,
> + },
please tabify this.
> +};
> +
> +/**
> + * mt9p012_probe - sensor driver i2c probe handler
> + * @client: i2c driver client device structure
> + *
> + * Register sensor as an i2c client device and V4L2
> + * device.
> + */
> +static int
> +mt9p012_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> + struct mt9p012_sensor *sensor = &mt9p012;
you should kzalloc(sensor) during probe() and be sure to kfree() in the
error case and on remove().
> + int err;
> +
> + if (i2c_get_clientdata(client))
> + return -EBUSY;
> +
> + sensor->pdata = client->dev.platform_data;
it's not a good practice to hold the complete pdata. You should have
something like:
struct mt9p012_platform_data *pdata = client->dev.platorm_data;
if (!pdata) {
dev_err(&client->dev, "no pdata\n";
return -EINVAL
}
sensor->power_set = pdata->power_set;
sensor->... = pdata->...
> +
> + if (!sensor->pdata) {
> + dev_err(&client->dev, "no platform data?\n");
> + return -ENODEV;
why no dev ?? the device seems to exist...
> + }
> +
> + sensor->v4l2_int_device = &mt9p012_int_device;
> + sensor->i2c_client = client;
You don't wanna hold client, you just need dev. From dev you can fecth
the i2c client pointer again by:
sensor->dev = &client->dev;
...
client = to_i2c_client(sensor->dev);
> + i2c_set_clientdata(client, sensor);
> +
> + /* Make the default capture format QCIF V4L2_PIX_FMT_SGRBG10 */
> + sensor->pix.width = MT9P012_VIDEO_WIDTH_4X_BINN_SCALED;
> + sensor->pix.height = MT9P012_VIDEO_WIDTH_4X_BINN_SCALED;
> + sensor->pix.pixelformat = V4L2_PIX_FMT_SGRBG10;
> +
> + err = v4l2_int_device_register(sensor->v4l2_int_device);
> + if (err)
> + i2c_set_clientdata(client, NULL);
> +
> + return err;
> +}
> +
> +/**
> + * mt9p012_remove - sensor driver i2c remove handler
> + * @client: i2c driver client device structure
> + *
> + * Unregister sensor as an i2c client device and V4L2
> + * device. Complement of mt9p012_probe().
> + */
> +static int __exit
> +mt9p012_remove(struct i2c_client *client)
you can't do it, remove __exit from here. i2c drivers can't sit in
__init or __exit only sections.
> +{
> + struct mt9p012_sensor *sensor = i2c_get_clientdata(client);
> +
> + if (!client->adapter)
> + return -ENODEV; /* our client isn't attached */
this won't happen, if it does, fix your driver :-p
> +
> + v4l2_int_device_unregister(sensor->v4l2_int_device);
> + i2c_set_clientdata(client, NULL);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id mt9p012_id[] = {
> + { DRIVER_NAME, 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, mt9p012_id);
> +
> +static struct i2c_driver mt9p012sensor_i2c_driver = {
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + },
> + .probe = mt9p012_probe,
> + .remove = __exit_p(mt9p012_remove),
remove __exit_p()
> + .id_table = mt9p012_id,
> +};
> +
> +static struct mt9p012_sensor mt9p012 = {
> + .timeperframe = {
> + .numerator = 1,
> + .denominator = 15,
> + },
> + .state = SENSOR_NOT_DETECTED,
> +};
> +
> +/**
> + * mt9p012sensor_init - sensor driver module_init handler
> + *
> + * Registers driver as an i2c client driver. Returns 0 on success,
> + * error code otherwise.
> + */
> +static int __init mt9p012sensor_init(void)
> +{
> + return i2c_add_driver(&mt9p012sensor_i2c_driver);
> +}
> +module_init(mt9p012sensor_init);
> +
> +/**
> + * mt9p012sensor_cleanup - sensor driver module_exit handler
> + *
> + * Unregisters/deletes driver as an i2c client driver.
> + * Complement of mt9p012sensor_init.
> + */
> +static void __exit mt9p012sensor_cleanup(void)
> +{
> + i2c_del_driver(&mt9p012sensor_i2c_driver);
> +}
> +module_exit(mt9p012sensor_cleanup);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("mt9p012 camera sensor driver");
> diff --git a/drivers/media/video/mt9p012_regs.h
> b/drivers/media/video/mt9p012_regs.h
> new file mode 100644
> index 0000000..70f6ee7
> --- /dev/null
> +++ b/drivers/media/video/mt9p012_regs.h
> @@ -0,0 +1,74 @@
> +/*
> + * drivers/media/video/mt9p012_regs.h
> + *
> + * Register definitions for the MT9P012 camera sensor.
> + *
> + * Author:
> + * Sameer Venkatraman <same...@ti.com>
> + * Sergio Aguirre <saagui...@ti.com>
> + * Martinez Leonides
> + *
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
no reason to add this. It's only used in the sibbling C file, so move
these there.
> diff --git a/include/media/mt9p012.h b/include/media/mt9p012.h
> new file mode 100644
> index 0000000..13a9745
> --- /dev/null
> +++ b/include/media/mt9p012.h
> @@ -0,0 +1,37 @@
> +/*
> + * drivers/media/video/mt9p012.h
This path is wrong and nobody uses it anymore, it should be in form:
mt9p012.h - Register definitions for the MT9P012 camera sensor
> + *
> + * Register definitions for the MT9P012 camera sensor.
> + *
> + * Author:
> + * Sameer Venkatraman <same...@ti.com>
> + * Martinez Leonides
> + *
> + *
> + * Copyright (C) 2008 Texas Instruments.
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#ifndef MT9P012_H
> +#define MT9P012_H
> +
> +
> +#define MT9P012_I2C_ADDR 0x10
> +
> +/**
> + * struct mt9p012_platform_data - platform data values and access functions
> + * @power_set: Power state access function, zero is off, non-zero is on.
> + * @default_regs: Default registers written after power-on or reset.
> + * @ifparm: Interface parameters access function
> + * @priv_data_set: device private data (pointer) access function
> + */
> +struct mt9p012_platform_data {
> + int (*power_set)(enum v4l2_power power);
> + u32 (*set_xclk)(u32 xclkfreq);
You shouldn't need this function, should be using clk fw.
--
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
