ov772x: add edge contrl support

  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(-)
  

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