| Message ID | u63irl9dx.wl%morimoto.kuninori@renesas.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Tue, 3 Mar 2009, Kuninori Morimoto wrote: > This patch add support extra register settings for platform. > For instance, platform comes to be able to use the > special setting like lens. > > Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> > --- [snip] > @@ -815,6 +808,14 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height, > */ > ov772x_reset(priv->client); > > + /* set extra setting */ > + if (priv->info->extra) { > + ret = ov772x_write_array(priv->client, > + priv->info->extra); > + if (ret < 0) > + goto ov772x_set_fmt_error; > + } > + > /* > * set size format > */ Hm, cannot say this patch makes me specifically happy. This means we let the user directly arbitrarily configure our registers. I don't seem to have a datasheet for ov772x, so, I cannot judge what registers are required for lens configuration, and how many meaningful settings there can be. For instance, maybe there are only two variants like lens-configuration-A and lens-configuration-B? Then I would just add respective flags to platform data. If there are really many variants, maybe we can let user-space configure them using VIDIOC_DBG_S_REGISTER? Can you maybe explain to me at least approximately what those lens settings are doing? Are there any sane defaults that would reasonably work with all lenses? In the very worst case, if we decide - no, this is very platform specific, and it has to be done in the kernel (why?), I would prefer adding elements like u32 LENS_CONFIG_1; u32 LENS_CONFIG_2; ... rather than allowing the platform to arbitrarily mangle our registers? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- 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
Dear Guennadi > can be. For instance, maybe there are only two variants like > lens-configuration-A and lens-configuration-B? Then I would just add > respective flags to platform data. If there are really many variants, > maybe we can let user-space configure them using VIDIOC_DBG_S_REGISTER? > Can you maybe explain to me at least approximately what those lens > settings are doing? well... useing VIDIOC_DBG_S_REGISTER is not good idea for me. Because we have to add CONFIG_VIDEO_ADY_DEBUG option which is for debug. ap325 lens setting have o a lot of common control register setting o AGC/AEC/BLC/DSP/AWB setting o Banding filter o Y/G channel average value o color value a lot of register will be set. like this +static const struct regval_list ov7725_lens [] = { + { 0x09, 0x00 }, { 0x0D, 0x61 }, { 0x0E, 0xD5 }, { 0x0F, 0xC5 }, + { 0x10, 0x25 }, { 0x11, 0x01 }, { 0x13, 0xEF }, { 0x14, 0x41 }, + { 0x22, 0x7F }, { 0x23, 0x03 }, { 0x24, 0x40 }, { 0x25, 0x30 }, + { 0x26, 0x82 }, { 0x2F, 0x35 }, { 0x37, 0x81 }, { 0x39, 0x6C }, + { 0x3A, 0x8C }, { 0x3B, 0xBC }, { 0x3C, 0xC0 }, { 0x3D, 0x03 }, + { 0x40, 0xE8 }, { 0x41, 0x00 }, { 0x42, 0x7F }, { 0x49, 0x00 }, + { 0x4A, 0x00 }, { 0x4B, 0x00 }, { 0x4C, 0x00 }, { 0x4D, 0x09 }, + { 0x60, 0x00 }, { 0x61, 0x05 }, { 0x63, 0xE0 }, { 0x64, 0xFF }, + { 0x65, 0x20 }, { 0x66, 0x00 }, { 0x69, 0x9E }, { 0x6B, 0x2D }, + { 0x6C, 0x09 }, { 0x6E, 0x72 }, { 0x6F, 0x4D }, { 0x70, 0x12 }, + { 0x71, 0xBF }, { 0x72, 0x0D }, { 0x73, 0x12 }, { 0x74, 0x12 }, + { 0x76, 0x00 }, { 0x77, 0x3A }, { 0x78, 0x23 }, { 0x79, 0x22 }, + { 0x7A, 0x41 }, { 0x7E, 0x04 }, { 0x7F, 0x0E }, { 0x80, 0x20 }, + { 0x81, 0x43 }, { 0x82, 0x53 }, { 0x83, 0x61 }, { 0x84, 0x6D }, + { 0x85, 0x76 }, { 0x86, 0x7E }, { 0x87, 0x86 }, { 0x88, 0x94 }, + { 0x89, 0xA1 }, { 0x8A, 0xC5 }, { 0x8E, 0x03 }, { 0x8F, 0x02 }, + { 0x90, 0x05 }, { 0x91, 0x01 }, { 0x92, 0x03 }, { 0x93, 0x00 }, + { 0x94, 0x7A }, { 0x95, 0x75 }, { 0x96, 0x05 }, { 0x97, 0x22 }, + { 0x98, 0x63 }, { 0x99, 0x85 }, { 0x9A, 0x1E }, { 0x9B, 0x08 }, + { 0x9C, 0x20 }, { 0x9E, 0x00 }, { 0x9F, 0xF8 }, { 0xA0, 0x02 }, + { 0xA1, 0x50 }, { 0xA6, 0x04 }, { 0xA7, 0x30 }, { 0xA8, 0x30 }, + { 0xAA, 0x00 }, ENDMARKER, +}; Best regards -- Kuninori Morimoto -- 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
On Tue, 3 Mar 2009, morimoto.kuninori@renesas.com wrote: > > can be. For instance, maybe there are only two variants like > > lens-configuration-A and lens-configuration-B? Then I would just add > > respective flags to platform data. If there are really many variants, > > maybe we can let user-space configure them using VIDIOC_DBG_S_REGISTER? > > Can you maybe explain to me at least approximately what those lens > > settings are doing? > > well... useing VIDIOC_DBG_S_REGISTER is not good idea for me. > Because we have to add CONFIG_VIDEO_ADY_DEBUG option which is for debug. > > ap325 lens setting have > > o a lot of common control register setting > o AGC/AEC/BLC/DSP/AWB setting > o Banding filter > o Y/G channel average value > o color value > > a lot of register will be set. > like this > > +static const struct regval_list ov7725_lens [] = { > + { 0x09, 0x00 }, { 0x0D, 0x61 }, { 0x0E, 0xD5 }, { 0x0F, 0xC5 }, > + { 0x10, 0x25 }, { 0x11, 0x01 }, { 0x13, 0xEF }, { 0x14, 0x41 }, > + { 0x22, 0x7F }, { 0x23, 0x03 }, { 0x24, 0x40 }, { 0x25, 0x30 }, > + { 0x26, 0x82 }, { 0x2F, 0x35 }, { 0x37, 0x81 }, { 0x39, 0x6C }, > + { 0x3A, 0x8C }, { 0x3B, 0xBC }, { 0x3C, 0xC0 }, { 0x3D, 0x03 }, > + { 0x40, 0xE8 }, { 0x41, 0x00 }, { 0x42, 0x7F }, { 0x49, 0x00 }, > + { 0x4A, 0x00 }, { 0x4B, 0x00 }, { 0x4C, 0x00 }, { 0x4D, 0x09 }, > + { 0x60, 0x00 }, { 0x61, 0x05 }, { 0x63, 0xE0 }, { 0x64, 0xFF }, > + { 0x65, 0x20 }, { 0x66, 0x00 }, { 0x69, 0x9E }, { 0x6B, 0x2D }, > + { 0x6C, 0x09 }, { 0x6E, 0x72 }, { 0x6F, 0x4D }, { 0x70, 0x12 }, > + { 0x71, 0xBF }, { 0x72, 0x0D }, { 0x73, 0x12 }, { 0x74, 0x12 }, > + { 0x76, 0x00 }, { 0x77, 0x3A }, { 0x78, 0x23 }, { 0x79, 0x22 }, > + { 0x7A, 0x41 }, { 0x7E, 0x04 }, { 0x7F, 0x0E }, { 0x80, 0x20 }, > + { 0x81, 0x43 }, { 0x82, 0x53 }, { 0x83, 0x61 }, { 0x84, 0x6D }, > + { 0x85, 0x76 }, { 0x86, 0x7E }, { 0x87, 0x86 }, { 0x88, 0x94 }, > + { 0x89, 0xA1 }, { 0x8A, 0xC5 }, { 0x8E, 0x03 }, { 0x8F, 0x02 }, > + { 0x90, 0x05 }, { 0x91, 0x01 }, { 0x92, 0x03 }, { 0x93, 0x00 }, > + { 0x94, 0x7A }, { 0x95, 0x75 }, { 0x96, 0x05 }, { 0x97, 0x22 }, > + { 0x98, 0x63 }, { 0x99, 0x85 }, { 0x9A, 0x1E }, { 0x9B, 0x08 }, > + { 0x9C, 0x20 }, { 0x9E, 0x00 }, { 0x9F, 0xF8 }, { 0xA0, 0x02 }, > + { 0xA1, 0x50 }, { 0xA6, 0x04 }, { 0xA7, 0x30 }, { 0xA8, 0x30 }, > + { 0xAA, 0x00 }, ENDMARKER, > +}; Ok, this is indeed a lot, still, we should do this properly. After a discussion with Hans on IRC the general conclusion was "noone outside of the device driver shall even know device registers." I think, we shall split this huge array in at least 3 groups: 1. default, that's also valid for other setups with this chip. as you describe this, this set might be empty... 2. settings, for which controls exist, or can be meaningfully added. For example, there are controls for gain, exposure, auto white balance,... 3. a configuration struct with meaningfully named _and_ documented fields. I.e., plese, do not name fields like "r17" or similar:-) This becomes even more important in the absence of a publicly available datasheet. Also, the struct field -- register relationship doesn't have to be one-to-one. I.e., might well be that one field affects several registers, or several fields affect one register. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- 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
Dear Guennadi Thank you for comment > Ok, this is indeed a lot, still, we should do this properly. After a > discussion with Hans on IRC the general conclusion was "noone outside of > the device driver shall even know device registers." I think, we shall > split this huge array in at least 3 groups: > > 1. default, that's also valid for other setups with this chip. as you > describe this, this set might be empty... > > 2. settings, for which controls exist, or can be meaningfully added. For > example, there are controls for gain, exposure, auto white balance,... > > 3. a configuration struct with meaningfully named _and_ documented fields. > I.e., plese, do not name fields like "r17" or similar:-) This becomes even > more important in the absence of a publicly available datasheet. Also, the > struct field -- register relationship doesn't have to be one-to-one. I.e., > might well be that one field affects several registers, or several fields > affect one register. Hmm. maybe I could understand. Following is a (very) rough composition. Is "defaults" so important ? Normal ov772x driver do default settings in init time. And sorry. I'm very busy now. So I will try to this problem in future. ---------- rough composition ------------- struct ov772x_AWB_gain { unsigned char blue; /* blue channel gain */ unsigned char red; /* red channel gain */ unsigned char green; /* green channel gain */ }; struct ov772x_Average_level { unsigned char ub; /* U/B Average lebel */ unsigned char ygb; /* Y/Gb Average lebel */ unsigned char vr; /* V/R Average lebel */ }; ... a lot of more ... struct ov772x_extra_settings { /* 1 */ const struct regval_list defaults; /* maybe empty */ /* 2 */ const struct ov772x_AWB_gain awb; /* for control */ const struct ov772x_Average_level lebel; /* for control */ ... }; Best regards -- Kuninori Morimoto -- 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/ov772x.c b/drivers/media/video/ov772x.c index 84b0fc1..f07d558 100644 --- a/drivers/media/video/ov772x.c +++ b/drivers/media/video/ov772x.c @@ -368,11 +368,6 @@ /* * struct */ -struct regval_list { - unsigned char reg_num; - unsigned char value; -}; - struct ov772x_color_format { char *name; __u32 fourcc; @@ -400,8 +395,6 @@ struct ov772x_priv { unsigned int flag_hflip:1; }; -#define ENDMARKER { 0xff, 0xff } - /* * register setting for window size */ @@ -815,6 +808,14 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height, */ ov772x_reset(priv->client); + /* set extra setting */ + if (priv->info->extra) { + ret = ov772x_write_array(priv->client, + priv->info->extra); + if (ret < 0) + goto ov772x_set_fmt_error; + } + /* * set size format */ diff --git a/include/media/ov772x.h b/include/media/ov772x.h index 57db48d..8a20a1e 100644 --- a/include/media/ov772x.h +++ b/include/media/ov772x.h @@ -13,6 +13,12 @@ #include <media/soc_camera.h> +#define ENDMARKER { 0xff, 0xff } +struct regval_list { + unsigned char reg_num; + unsigned char value; +}; + /* for flags */ #define OV772X_FLAG_VFLIP 0x00000001 /* Vertical flip image */ #define OV772X_FLAG_HFLIP 0x00000002 /* Horizontal flip image */ @@ -21,6 +27,7 @@ struct ov772x_camera_info { unsigned long buswidth; unsigned long flags; struct soc_camera_link link; + const struct regval_list *extra; }; #endif /* __OV772X_H__ */
This patch add support extra register settings for platform. For instance, platform comes to be able to use the special setting like lens. Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> --- Thank you Magnus for your nice comment. drivers/media/video/ov772x.c | 15 ++++++++------- include/media/ov772x.h | 7 +++++++ 2 files changed, 15 insertions(+), 7 deletions(-)