| Message ID | uab7c249a.wl%morimoto.kuninori@renesas.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Mon, 23 Mar 2009, Kuninori Morimoto wrote: > > Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> > --- > This patch is 1st step for extra settings > > drivers/media/video/ov772x.c | 34 ++++++++++++++++++++++++++++++++++ > include/media/ov772x.h | 25 +++++++++++++++++++++++++ > 2 files changed, 59 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c > index 34c9819..a951327 100644 > --- a/drivers/media/video/ov772x.c > +++ b/drivers/media/video/ov772x.c > @@ -358,6 +358,15 @@ > #define VOSZ_VGA 0xF0 > #define VOSZ_QVGA 0x78 > > +/* EDGE CTRL > + * see alse > + * ov772x.h :: for Edge ctrl > + */ > +#define EDGE0CTRL(param) (((param) >> 24) & 0x1F) > +#define EDGE1CTRL(param) (((param) >> 16) & 0x0F) > +#define EDGE2CTRL(param) (((param) >> 8) & 0xFF) > +#define EDGE3CTRL(param) (((param) >> 0) & 0xFF) > + > /* > * ID > */ > @@ -816,6 +825,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height, > ov772x_reset(priv->client); > > /* > + * set Edge Ctrl if platform has edgectrl > + */ > + if (priv->info->edgectrl & OV772X_EDGECTRL_ENABLE) { > + ret = ov772x_mask_set(priv->client, > + EDGE0, 0x1F, EDGE0CTRL(priv->info->edgectrl)); > + if (ret < 0) > + goto ov772x_set_fmt_error; > + > + ret = ov772x_mask_set(priv->client, > + EDGE1, 0x0F, EDGE1CTRL(priv->info->edgectrl)); > + if (ret < 0) > + goto ov772x_set_fmt_error; > + > + ret = ov772x_mask_set(priv->client, > + EDGE2, 0xFF, EDGE2CTRL(priv->info->edgectrl)); > + if (ret < 0) > + goto ov772x_set_fmt_error; > + > + ret = ov772x_mask_set(priv->client, > + EDGE3, 0xFF, EDGE3CTRL(priv->info->edgectrl)); > + if (ret < 0) > + goto ov772x_set_fmt_error; > + } > + > + /* > * set size format > */ > ret = ov772x_write_array(priv->client, priv->win->regs); > diff --git a/include/media/ov772x.h b/include/media/ov772x.h > index 57db48d..5b083dc 100644 > --- a/include/media/ov772x.h > +++ b/include/media/ov772x.h > @@ -17,9 +17,34 @@ > #define OV772X_FLAG_VFLIP 0x00000001 /* Vertical flip image */ > #define OV772X_FLAG_HFLIP 0x00000002 /* Horizontal flip image */ > > +/* > + * for Edge ctrl > + * > + * strength : (for EDGE0) Edge enhancement strength control > + * threshold : (for EDGE1) Edge enhancement threshold control > + * low : (for EDGE2) Edge enhancement strength Low point control > + * high : (for EDGE3) Edge enhancement strength High point control > + * > + * Meaning of edgectrl bit > + * > + * Exx0 0000 xxxx 1111 2222 2222 3333 3333 > + * > + * E: use edgectrl or not (OV772X_EDGECTRL_ENABLE) > + * 0: for Edge0 ctrl > + * 1: for Edge1 ctrl > + * 2: for Edge2 ctrl > + * 3: for Edge3 ctrl > + */ > +#define OV772X_EDGECTRL_ENABLE 0x80000000 > +#define OV772X_EDGECTRL(strength, threshold, low, high) \ > + (OV772X_EDGECTRL_ENABLE | \ > + (strength & 0x1F) << 24 | (threshold & 0x0F) << 16 | \ > + (low & 0xFF) << 8 | (high & 0xFF) << 0) > + > struct ov772x_camera_info { > unsigned long buswidth; > unsigned long flags; > + unsigned long edgectrl; Wouldn't it be easier to use unsigned char edge_strength; unsigned char edge_threshold; unsigned char edge_low; unsigned char edge_high; and thus avoid all that shifting? > struct soc_camera_link link; > }; > > -- > 1.5.6.3 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 checking > Wouldn't it be easier to use > > unsigned char edge_strength; > unsigned char edge_threshold; > unsigned char edge_low; > unsigned char edge_high; > > and thus avoid all that shifting? Yes. it is easier to use. But, driver should judge whether to use this setting or not. Because this setting is optional. Because user setting might be 0, we can not judge it like this. if (edge_xxx) ov772x_mask_set( xxxx ) So, we can use un-used bit to judge whether to use it. and edge_strength and edge_threshold have un-used bit. But edge_low and edge_high doesn't have un-used bit. Another way to judge it is to use pointer or to add another variable. But I don't like these style. What do you think about this ? 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 Mon, 23 Mar 2009, morimoto.kuninori@renesas.com wrote: > Dear Guennadi > > Thank you for checking > > > Wouldn't it be easier to use > > > > unsigned char edge_strength; > > unsigned char edge_threshold; > > unsigned char edge_low; > > unsigned char edge_high; > > > > and thus avoid all that shifting? > > Yes. it is easier to use. > But, driver should judge whether to use this setting or not. > Because this setting is optional. > > Because user setting might be 0, > we can not judge it like this. > if (edge_xxx) > ov772x_mask_set( xxxx ) > > So, we can use un-used bit to judge whether to use it. > and edge_strength and edge_threshold have un-used bit. > But edge_low and edge_high doesn't have un-used bit. > > Another way to judge it is to use pointer or to add another variable. > But I don't like these style. > What do you think about this ? Is edge_strength == 0 a useful edge configuration? Cannot you use it as a test whether to set all edge parameters or not? If you cannot, well, just do the same as what you have done with 32-bits - use one bit in strength as "edge enable" - just exactly in the same way as in your patch. Like if (edge_strength & EDGE_ENABLE) { set_strength; set_threshold; set_low; set_high; } Thanks Guennadi --- Guennadi Liakhovetski -- 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 > Is edge_strength == 0 a useful edge configuration? Cannot you use it as a > test whether to set all edge parameters or not? If you cannot, well, just > do the same as what you have done with 32-bits - use one bit in strength > as "edge enable" - just exactly in the same way as in your patch. Like > > if (edge_strength & EDGE_ENABLE) { > set_strength; > set_threshold; > set_low; > set_high; > } Hmm.. edge_threshold has 4 un-used bit. we can use it for judge. And sorry, I don't like this style unsigned char edge_strength; unsigned char edge_threshold; unsigned char edge_low; unsigned char edge_high; I will create new struct ov772x_edge for it. 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 34c9819..a951327 100644 --- a/drivers/media/video/ov772x.c +++ b/drivers/media/video/ov772x.c @@ -358,6 +358,15 @@ #define VOSZ_VGA 0xF0 #define VOSZ_QVGA 0x78 +/* EDGE CTRL + * see alse + * ov772x.h :: for Edge ctrl + */ +#define EDGE0CTRL(param) (((param) >> 24) & 0x1F) +#define EDGE1CTRL(param) (((param) >> 16) & 0x0F) +#define EDGE2CTRL(param) (((param) >> 8) & 0xFF) +#define EDGE3CTRL(param) (((param) >> 0) & 0xFF) + /* * ID */ @@ -816,6 +825,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height, ov772x_reset(priv->client); /* + * set Edge Ctrl if platform has edgectrl + */ + if (priv->info->edgectrl & OV772X_EDGECTRL_ENABLE) { + ret = ov772x_mask_set(priv->client, + EDGE0, 0x1F, EDGE0CTRL(priv->info->edgectrl)); + if (ret < 0) + goto ov772x_set_fmt_error; + + ret = ov772x_mask_set(priv->client, + EDGE1, 0x0F, EDGE1CTRL(priv->info->edgectrl)); + if (ret < 0) + goto ov772x_set_fmt_error; + + ret = ov772x_mask_set(priv->client, + EDGE2, 0xFF, EDGE2CTRL(priv->info->edgectrl)); + if (ret < 0) + goto ov772x_set_fmt_error; + + ret = ov772x_mask_set(priv->client, + EDGE3, 0xFF, EDGE3CTRL(priv->info->edgectrl)); + if (ret < 0) + goto ov772x_set_fmt_error; + } + + /* * set size format */ ret = ov772x_write_array(priv->client, priv->win->regs); diff --git a/include/media/ov772x.h b/include/media/ov772x.h index 57db48d..5b083dc 100644 --- a/include/media/ov772x.h +++ b/include/media/ov772x.h @@ -17,9 +17,34 @@ #define OV772X_FLAG_VFLIP 0x00000001 /* Vertical flip image */ #define OV772X_FLAG_HFLIP 0x00000002 /* Horizontal flip image */ +/* + * for Edge ctrl + * + * strength : (for EDGE0) Edge enhancement strength control + * threshold : (for EDGE1) Edge enhancement threshold control + * low : (for EDGE2) Edge enhancement strength Low point control + * high : (for EDGE3) Edge enhancement strength High point control + * + * Meaning of edgectrl bit + * + * Exx0 0000 xxxx 1111 2222 2222 3333 3333 + * + * E: use edgectrl or not (OV772X_EDGECTRL_ENABLE) + * 0: for Edge0 ctrl + * 1: for Edge1 ctrl + * 2: for Edge2 ctrl + * 3: for Edge3 ctrl + */ +#define OV772X_EDGECTRL_ENABLE 0x80000000 +#define OV772X_EDGECTRL(strength, threshold, low, high) \ + (OV772X_EDGECTRL_ENABLE | \ + (strength & 0x1F) << 24 | (threshold & 0x0F) << 16 | \ + (low & 0xFF) << 8 | (high & 0xFF) << 0) + struct ov772x_camera_info { unsigned long buswidth; unsigned long flags; + unsigned long edgectrl; struct soc_camera_link link; };
Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com> --- This patch is 1st step for extra settings drivers/media/video/ov772x.c | 34 ++++++++++++++++++++++++++++++++++ include/media/ov772x.h | 25 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 0 deletions(-)