Re: [PATCH] mt9v022: Add support for mt9v024
Hi Alex
Thanks for the patch, comments below.
On Mon, 30 Jul 2012, Alex Gershgorin wrote:
This patch has been successfully tested
Signed-off-by: Alex Gershgorin al...@meprolight.com
---
drivers/media/video/Kconfig |2 +-
drivers/media/video/mt9v022.c | 28 ++--
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..38d6944 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -1013,7 +1013,7 @@ config SOC_CAMERA_MT9T112
This driver supports MT9T112 cameras from Aptina.
config SOC_CAMERA_MT9V022
- tristate mt9v022 support
+ tristate mt9v022 and mt9v024 support
depends on SOC_CAMERA I2C
select GPIO_PCA953X if MT9V022_PCA9536_SWITCH
help
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index bf63417..d2c1ab1 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -26,7 +26,7 @@
* The platform has to define ctruct i2c_board_info objects and link to them
* from struct soc_camera_link
*/
-
+static s32 chip_id;
No, this should be per instance. Please, add it to struct mt9v022.
static char *sensor_type;
module_param(sensor_type, charp, S_IRUGO);
MODULE_PARM_DESC(sensor_type, Sensor type: \colour\ or \monochrome\);
@@ -57,6 +57,10 @@ MODULE_PARM_DESC(sensor_type, Sensor type: \colour\ or
\monochrome\);
#define MT9V022_AEC_AGC_ENABLE 0xAF
#define MT9V022_MAX_TOTAL_SHUTTER_WIDTH 0xBD
+/* mt9v024 partial list register addresses changes with respect to mt9v022 */
+#define MT9V024_PIXCLK_FV_LV 0x72
+#define MT9V024_MAX_TOTAL_SHUTTER_WIDTH 0xAD
+
/* Progressive scan, master, defaults */
#define MT9V022_CHIP_CONTROL_DEFAULT 0x188
@@ -185,7 +189,9 @@ static int mt9v022_init(struct i2c_client *client)
if (!ret)
ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH, 480);
if (!ret)
- ret = reg_write(client, MT9V022_MAX_TOTAL_SHUTTER_WIDTH, 480);
+ ret = reg_write(client, (chip_id == 0x1324) ?
I would use a macro something like
#define is_mt9v024(p) (p-chip_id == 0x1324)
same everywhere below
+ MT9V024_MAX_TOTAL_SHUTTER_WIDTH :
+ MT9V022_MAX_TOTAL_SHUTTER_WIDTH, 480);
Hm, with just 2 registers different it almost isn't worth it, but still...
(1) if someone uses this driver as a template, or (2) if we add more
sensors or more registers, whose addresses also are different, I think, we
better do it properly. How about
/* only registers with different addresses on different mt9v02x sensors */
struct mt9v02x_register {
u8 max_total_shutter_width;
u8 pixclk_fv_lv;
};
static const struct mt9v02x_register mt9v022_register = {
.max_total_shutter_width= MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
.pixclk_fv_lv = MT9V022_PIXCLK_FV_LV,
};
static const struct mt9v02x_register mt9v024_register = {
.max_total_shutter_width= MT9V024_MAX_TOTAL_SHUTTER_WIDTH,
.pixclk_fv_lv = MT9V024_PIXCLK_FV_LV,
};
struct mt9v022 {
...
const struct mt9v02x_register *reg;
...
};
and then in this case just do
+ ret = reg_write(client, mt9v022-reg-max_total_shutter_width,
480);
etc.?
if (!ret)
/* default - auto */
ret = reg_clear(client, MT9V022_BLACK_LEVEL_CALIB_CTRL, 1);
@@ -238,8 +244,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct
v4l2_crop *a)
ret = reg_read(client, MT9V022_AEC_AGC_ENABLE);
if (ret = 0) {
if (ret 1) /* Autoexposure */
- ret = reg_write(client, MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
- rect.height + mt9v022-y_skip_top + 43);
+ ret = reg_write(client, (chip_id == 0x1324) ?
+ MT9V024_MAX_TOTAL_SHUTTER_WIDTH :
+ MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
+ rect.height + mt9v022-y_skip_top + 43);
else
ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
rect.height + mt9v022-y_skip_top + 43);
@@ -566,18 +574,17 @@ static int mt9v022_video_probe(struct i2c_client
*client)
{
struct mt9v022 *mt9v022 = to_mt9v022(client);
struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
- s32 data;
int ret;
unsigned long flags;
/* Read out the chip version register */
- data = reg_read(client, MT9V022_CHIP_VERSION);
+ chip_id = reg_read(client, MT9V022_CHIP_VERSION);
/* must be 0x1311 or 0x1313 */
- if (data != 0x1311 data != 0x1313) {
+ if (chip_id != 0x1311 chip_id != 0x1313
RE: [PATCH] mt9v022: Add support for mt9v024
Hi Guennadi,
Thanks for the patch, comments below.
Thanks for you comments.
On Mon, 30 Jul 2012, Alex Gershgorin wrote:
This patch has been successfully tested
Signed-off-by: Alex Gershgorin al...@meprolight.com
---
drivers/media/video/Kconfig |2 +-
drivers/media/video/mt9v022.c | 28 ++--
2 files changed, 19 insertions(+), 11 deletions(-)
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index 99937c9..38d6944 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -1013,7 +1013,7 @@ config SOC_CAMERA_MT9T112
This driver supports MT9T112 cameras from Aptina.
config SOC_CAMERA_MT9V022
- tristate mt9v022 support
+ tristate mt9v022 and mt9v024 support
depends on SOC_CAMERA I2C
select GPIO_PCA953X if MT9V022_PCA9536_SWITCH
help
diff --git a/drivers/media/video/mt9v022.c b/drivers/media/video/mt9v022.c
index bf63417..d2c1ab1 100644
--- a/drivers/media/video/mt9v022.c
+++ b/drivers/media/video/mt9v022.c
@@ -26,7 +26,7 @@
* The platform has to define ctruct i2c_board_info objects and link to them
* from struct soc_camera_link
*/
-
+static s32 chip_id;
No, this should be per instance. Please, add it to struct mt9v022.
static char *sensor_type;
module_param(sensor_type, charp, S_IRUGO);
MODULE_PARM_DESC(sensor_type, Sensor type: \colour\ or \monochrome\);
@@ -57,6 +57,10 @@ MODULE_PARM_DESC(sensor_type, Sensor type: \colour\ or
\monochrome\);
#define MT9V022_AEC_AGC_ENABLE 0xAF
#define MT9V022_MAX_TOTAL_SHUTTER_WIDTH 0xBD
+/* mt9v024 partial list register addresses changes with respect to mt9v022 */
+#define MT9V024_PIXCLK_FV_LV 0x72
+#define MT9V024_MAX_TOTAL_SHUTTER_WIDTH 0xAD
+
/* Progressive scan, master, defaults */
#define MT9V022_CHIP_CONTROL_DEFAULT 0x188
@@ -185,7 +189,9 @@ static int mt9v022_init(struct i2c_client *client)
if (!ret)
ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH, 480);
if (!ret)
- ret = reg_write(client, MT9V022_MAX_TOTAL_SHUTTER_WIDTH, 480);
+ ret = reg_write(client, (chip_id == 0x1324) ?
I would use a macro something like
#define is_mt9v024(p) (p-chip_id == 0x1324)
same everywhere below
+ MT9V024_MAX_TOTAL_SHUTTER_WIDTH :
+ MT9V022_MAX_TOTAL_SHUTTER_WIDTH, 480);
Hm, with just 2 registers different it almost isn't worth it, but still...
There is little more than two registers and also added new registers
if you're interested I can send you the document.
(1) if someone uses this driver as a template, or (2) if we add more
sensors or more registers, whose addresses also are different, I think, we
better do it properly. How about
/* only registers with different addresses on different mt9v02x sensors */
struct mt9v02x_register {
u8 max_total_shutter_width;
u8 pixclk_fv_lv;
};
static const struct mt9v02x_register mt9v022_register = {
.max_total_shutter_width= MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
.pixclk_fv_lv = MT9V022_PIXCLK_FV_LV,
};
static const struct mt9v02x_register mt9v024_register = {
.max_total_shutter_width= MT9V024_MAX_TOTAL_SHUTTER_WIDTH,
.pixclk_fv_lv = MT9V024_PIXCLK_FV_LV,
};
struct mt9v022 {
...
const struct mt9v02x_register *reg;
...
};
and then in this case just do
+ ret = reg_write(client,
mt9v022-reg-max_total_shutter_width, 480);
etc.?
I accept your corrections and suggestions in the near future
I will send an updated version of this patch.
Regards,
Alex
if (!ret)
/* default - auto */
ret = reg_clear(client, MT9V022_BLACK_LEVEL_CALIB_CTRL, 1);
@@ -238,8 +244,10 @@ static int mt9v022_s_crop(struct v4l2_subdev *sd, struct
v4l2_crop *a)
ret = reg_read(client, MT9V022_AEC_AGC_ENABLE);
if (ret = 0) {
if (ret 1) /* Autoexposure */
- ret = reg_write(client, MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
- rect.height + mt9v022-y_skip_top + 43);
+ ret = reg_write(client, (chip_id == 0x1324) ?
+ MT9V024_MAX_TOTAL_SHUTTER_WIDTH :
+ MT9V022_MAX_TOTAL_SHUTTER_WIDTH,
+ rect.height + mt9v022-y_skip_top + 43);
else
ret = reg_write(client, MT9V022_TOTAL_SHUTTER_WIDTH,
rect.height + mt9v022-y_skip_top + 43);
@@ -566,18 +574,17 @@ static int mt9v022_video_probe(struct i2c_client
*client)
{
struct mt9v022 *mt9v022 = to_mt9v022(client);
struct soc_camera_link *icl = soc_camera_i2c_to_link(client);
- s32 data;
int ret;
unsigned long flags;
/* Read out the chip version register */
