| Message ID | 1343660457-7238-1-git-send-email-alexg@meprolight.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <alexg@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 && chip_id != 0x1324) { > ret = -ENODEV; > dev_info(&client->dev, "No MT9V022 found, ID register 0x%x\n", > - data); > + chip_id); > goto ei2c; > } > > @@ -632,7 +639,7 @@ static int mt9v022_video_probe(struct i2c_client *client) > mt9v022->fmt = &mt9v022->fmts[0]; > > dev_info(&client->dev, "Detected a MT9V022 chip ID %x, %s sensor\n", > - data, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? > + chip_id, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? > "monochrome" : "colour"); > > ret = mt9v022_init(client); > @@ -728,7 +735,8 @@ static int mt9v022_s_mbus_config(struct v4l2_subdev *sd, > if (!(flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)) > pixclk |= 0x2; > > - ret = reg_write(client, MT9V022_PIXCLK_FV_LV, pixclk); > + ret = reg_write(client, (chip_id == 0x1324) ? MT9V024_PIXCLK_FV_LV : > + MT9V022_PIXCLK_FV_LV, pixclk); > if (ret < 0) > return ret; > > -- > 1.7.0.4 Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <alexg@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 */ > - 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 && chip_id != 0x1324) { > ret = -ENODEV; > dev_info(&client->dev, "No MT9V022 found, ID register 0x%x\n", > - data); > + chip_id); > goto ei2c; > } > > @@ -632,7 +639,7 @@ static int mt9v022_video_probe(struct i2c_client *client) > mt9v022->fmt = &mt9v022->fmts[0]; > > dev_info(&client->dev, "Detected a MT9V022 chip ID %x, %s sensor\n", > - data, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? > + chip_id, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? > "monochrome" : "colour"); > > ret = mt9v022_init(client); > @@ -728,7 +735,8 @@ static int mt9v022_s_mbus_config(struct v4l2_subdev *sd, > if (!(flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)) > pixclk |= 0x2; > > - ret = reg_write(client, MT9V022_PIXCLK_FV_LV, pixclk); > + ret = reg_write(client, (chip_id == 0x1324) ? MT9V024_PIXCLK_FV_LV : > + MT9V022_PIXCLK_FV_LV, pixclk); > if (ret < 0) > return ret; > > -- > 1.7.0.4 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; 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) ? + MT9V024_MAX_TOTAL_SHUTTER_WIDTH : + MT9V022_MAX_TOTAL_SHUTTER_WIDTH, 480); 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 && chip_id != 0x1324) { ret = -ENODEV; dev_info(&client->dev, "No MT9V022 found, ID register 0x%x\n", - data); + chip_id); goto ei2c; } @@ -632,7 +639,7 @@ static int mt9v022_video_probe(struct i2c_client *client) mt9v022->fmt = &mt9v022->fmts[0]; dev_info(&client->dev, "Detected a MT9V022 chip ID %x, %s sensor\n", - data, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? + chip_id, mt9v022->model == V4L2_IDENT_MT9V022IX7ATM ? "monochrome" : "colour"); ret = mt9v022_init(client); @@ -728,7 +735,8 @@ static int mt9v022_s_mbus_config(struct v4l2_subdev *sd, if (!(flags & V4L2_MBUS_VSYNC_ACTIVE_HIGH)) pixclk |= 0x2; - ret = reg_write(client, MT9V022_PIXCLK_FV_LV, pixclk); + ret = reg_write(client, (chip_id == 0x1324) ? MT9V024_PIXCLK_FV_LV : + MT9V022_PIXCLK_FV_LV, pixclk); if (ret < 0) return ret;
This patch has been successfully tested Signed-off-by: Alex Gershgorin <alexg@meprolight.com> --- drivers/media/video/Kconfig | 2 +- drivers/media/video/mt9v022.c | 28 ++++++++++++++++++---------- 2 files changed, 19 insertions(+), 11 deletions(-)