RE: [PATCH 1/5] MT9P012: Add driver
Hi Felipe,
Sorry for the delay replying this...
Find my responses below.
-Original Message-
From: Felipe Balbi [mailto:felipe.ba...@nokia.com]
Sent: Wednesday, March 04, 2009 5:31 AM
To: Aguirre Rodriguez, Sergio Alberto
Cc: linux-media@vger.kernel.org; linux-o...@vger.kernel.org; Sakari Ailus;
Toivonen Tuukka.O (Nokia-D/Oulu); Doyu Hiroshi (Nokia-D/Helsinki);
DongSoo(Nathaniel) Kim; MiaoStanley; Nagalla, Hari; Hiremath, Vaibhav;
Lakhani, Amish; Menon, Nishanth
Subject: Re: [PATCH 1/5] MT9P012: Add driver
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_DETECTED1
+#define SENSOR_NOT_DETECTED0
these two should be unneeded...
Agreed, got rid of them...
+
+/**
+ * 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.
Done.
+};
+
+enum pixel_format {
+ RAWBAYER10
+};
+
+#define NUM_IMAGE_SIZES5
+#define NUM_PIXEL_FORMATS 1
+#define NUM_FPS2 /* 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.
Done.
+
+/* 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.
You're right. Done.
+static unsigned long xclk_current = MT9P012_XCLK_NOM_1;
why ??
Hmm, well. This is the xclk default value we use.
The driver basically uses 2 XCLK values to cover all the supported
framerates/framesizes.
Anyways, I've added a xclk_current field as part of struct mt9p012_sensor
instead. Removed this static var.
+static int
+find_vctrl(int id)
I guess it fits in one line...
Done. Fixed all similar cases.
+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
Re: [PATCH 1/5] MT9P012: Add driver
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_DETECTED1
+#define SENSOR_NOT_DETECTED0
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_SIZES5
+#define NUM_PIXEL_FORMATS 1
+#define NUM_FPS2 /* 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)
+
