[v3] ov772x: add edge contrl support
Commit Message
Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
---
v2 -> v3
o use edgectrl.strength for judge
Sorry for my miss-understanding.
This patch use edgectrl.strength for judgement.
And the explain in the comment has all.
My datasheet doesn't have details more than this explain.
drivers/media/video/ov772x.c | 25 +++++++++++++++++++++++++
include/media/ov772x.h | 21 +++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
Comments
On Tue, 24 Mar 2009, Kuninori Morimoto wrote:
> Signed-off-by: Kuninori Morimoto <morimoto.kuninori@renesas.com>
> ---
> v2 -> v3
> o use edgectrl.strength for judge
>
> Sorry for my miss-understanding.
> This patch use edgectrl.strength for judgement.
> And the explain in the comment has all.
> My datasheet doesn't have details more than this explain.
>
> drivers/media/video/ov772x.c | 25 +++++++++++++++++++++++++
> include/media/ov772x.h | 21 +++++++++++++++++++++
> 2 files changed, 46 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/ov772x.c b/drivers/media/video/ov772x.c
> index 34c9819..3226c43 100644
> --- a/drivers/media/video/ov772x.c
> +++ b/drivers/media/video/ov772x.c
> @@ -816,6 +816,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height,
> ov772x_reset(priv->client);
>
> /*
> + * set Edge Ctrl
> + */
> + if (priv->info->edgectrl.strength) {
> + ret = ov772x_mask_set(priv->client, EDGE0, 0x1F,
> + priv->info->edgectrl.strength);
> + if (ret < 0)
> + goto ov772x_set_fmt_error;
Whatever this "edge" does, isn't it so, that "threshold," "upper," and
"lower" parameters configure it and "strength" actually enforces the
changes? Say, if strength == 0, edge control is off, and as soon as you
switch it to != 0, it is on with all the configured parameters? If my
guess is right, wouldn't it make sense to first configure parameters and
then set strength? If you agree, I'll just swap them when committing, so,
you don't have to resend. Just please either confirm that you agree, or
explain why I am wrong.
> +
> + ret = ov772x_mask_set(priv->client, EDGE1, 0x0F,
> + priv->info->edgectrl.threshold);
> + if (ret < 0)
> + goto ov772x_set_fmt_error;
> +
> + ret = ov772x_mask_set(priv->client, EDGE2, 0xFF,
> + priv->info->edgectrl.upper);
> + if (ret < 0)
> + goto ov772x_set_fmt_error;
> +
> + ret = ov772x_mask_set(priv->client, EDGE3, 0xFF,
> + priv->info->edgectrl.lower);
> + 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..cfdd80e 100644
> --- a/include/media/ov772x.h
> +++ b/include/media/ov772x.h
> @@ -17,10 +17,31 @@
> #define OV772X_FLAG_VFLIP 0x00000001 /* Vertical flip image */
> #define OV772X_FLAG_HFLIP 0x00000002 /* Horizontal flip image */
>
> +/*
> + * for Edge ctrl
> + */
> +struct ov772x_edge_ctrl {
> + unsigned char strength; /* strength control */
> + unsigned char threshold; /* threshold detection */
> + unsigned char upper; /* strength upper limit */
> + unsigned char lower; /* strength lower limit */
> +};
> +
> +#define OV772X_EDGECTRL(s, t, u, l) \
> + {.strength = (s & 0x1F),\
> + .threshold = (t & 0x0F),\
> + .upper = (u & 0xFF),\
> + .lower = (l & 0xFF),\
> + }
If you don't mind, I'll reformat this slightly to
+#define OV772X_EDGECTRL(s, t, u, l) \
+{ \
+ .strength = (s & 0x1F), \
+ .threshold = (t & 0x0F), \
+ .upper = (u & 0xFF), \
+ .lower = (l & 0xFF), \
+}
when applying, ok?
> +
> +/*
> + * ov772x camera info
> + */
> struct ov772x_camera_info {
> unsigned long buswidth;
> unsigned long flags;
> struct soc_camera_link link;
> + struct ov772x_edge_ctrl edgectrl;
> };
>
> #endif /* __OV772X_H__ */
> --
> 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 patch
> Whatever this "edge" does, isn't it so, that "threshold," "upper," and
> "lower" parameters configure it and "strength" actually enforces the
> changes? Say, if strength == 0, edge control is off, and as soon as you
> switch it to != 0, it is on with all the configured parameters? If my
> guess is right, wouldn't it make sense to first configure parameters and
> then set strength? If you agree, I'll just swap them when committing, so,
> you don't have to resend. Just please either confirm that you agree, or
> explain why I am wrong.
I don't know detail of these register's order.
Because my datasheet doesn't have detail explain.
For example, does "strength" actually enforce
all of configured parameters change ?
So, I tried to test whether these register setting
order were important.
It seems to be independent apparently respectively.
For example, I can change only "upper" register and
the setting was effective.
So, I will ask to maker about these register's behavior.
Because it seems to relate to other register.
So, please ignore this patch until I get answer. sorry.
> +#define OV772X_EDGECTRL(s, t, u, l) \
> +{ \
> + .strength = (s & 0x1F), \
> + .threshold = (t & 0x0F), \
> + .upper = (u & 0xFF), \
> + .lower = (l & 0xFF), \
> +}
I will fix this in next =)
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
Hello Morimoto-san
On Wed, 25 Mar 2009, morimoto.kuninori@renesas.com wrote:
> > Whatever this "edge" does, isn't it so, that "threshold," "upper," and
> > "lower" parameters configure it and "strength" actually enforces the
> > changes? Say, if strength == 0, edge control is off, and as soon as you
> > switch it to != 0, it is on with all the configured parameters? If my
> > guess is right, wouldn't it make sense to first configure parameters and
> > then set strength? If you agree, I'll just swap them when committing, so,
> > you don't have to resend. Just please either confirm that you agree, or
> > explain why I am wrong.
>
> I don't know detail of these register's order.
> Because my datasheet doesn't have detail explain.
>
> For example, does "strength" actually enforce
> all of configured parameters change ?
>
> So, I tried to test whether these register setting
> order were important.
>
> It seems to be independent apparently respectively.
> For example, I can change only "upper" register and
> the setting was effective.
So, you _do_ know what that parameter does - if you can verify what effect
it produces on the camera? So, that's just what I was asking - what do
these settings do? What changes do you observe when you manipulate them?
And this your observation actually suffices to me to preserve your
original order of register writes. If documentation says nothing about it,
and as long as it works - we can keep it.
> So, I will ask to maker about these register's behavior.
> Because it seems to relate to other register.
> So, please ignore this patch until I get answer. sorry.
Well, I don't think we have to wait for an answer for too long. If they
don't reply within 1-2 days, let's just take the patch as is (with the
single minor correction I proposed).
> > +#define OV772X_EDGECTRL(s, t, u, l) \
> > +{ \
> > + .strength = (s & 0x1F), \
> > + .threshold = (t & 0x0F), \
> > + .upper = (u & 0xFF), \
> > + .lower = (l & 0xFF), \
> > +}
>
> I will fix this in next =)
That's up to you. This is a minor formatting correction, which I can do
myself when merging. But if you like, you can send an update, sure.
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
@@ -816,6 +816,31 @@ static int ov772x_set_params(struct ov772x_priv *priv, u32 width, u32 height,
ov772x_reset(priv->client);
/*
+ * set Edge Ctrl
+ */
+ if (priv->info->edgectrl.strength) {
+ ret = ov772x_mask_set(priv->client, EDGE0, 0x1F,
+ priv->info->edgectrl.strength);
+ if (ret < 0)
+ goto ov772x_set_fmt_error;
+
+ ret = ov772x_mask_set(priv->client, EDGE1, 0x0F,
+ priv->info->edgectrl.threshold);
+ if (ret < 0)
+ goto ov772x_set_fmt_error;
+
+ ret = ov772x_mask_set(priv->client, EDGE2, 0xFF,
+ priv->info->edgectrl.upper);
+ if (ret < 0)
+ goto ov772x_set_fmt_error;
+
+ ret = ov772x_mask_set(priv->client, EDGE3, 0xFF,
+ priv->info->edgectrl.lower);
+ if (ret < 0)
+ goto ov772x_set_fmt_error;
+ }
+
+ /*
* set size format
*/
ret = ov772x_write_array(priv->client, priv->win->regs);
@@ -17,10 +17,31 @@
#define OV772X_FLAG_VFLIP 0x00000001 /* Vertical flip image */
#define OV772X_FLAG_HFLIP 0x00000002 /* Horizontal flip image */
+/*
+ * for Edge ctrl
+ */
+struct ov772x_edge_ctrl {
+ unsigned char strength; /* strength control */
+ unsigned char threshold; /* threshold detection */
+ unsigned char upper; /* strength upper limit */
+ unsigned char lower; /* strength lower limit */
+};
+
+#define OV772X_EDGECTRL(s, t, u, l) \
+ {.strength = (s & 0x1F),\
+ .threshold = (t & 0x0F),\
+ .upper = (u & 0xFF),\
+ .lower = (l & 0xFF),\
+ }
+
+/*
+ * ov772x camera info
+ */
struct ov772x_camera_info {
unsigned long buswidth;
unsigned long flags;
struct soc_camera_link link;
+ struct ov772x_edge_ctrl edgectrl;
};
#endif /* __OV772X_H__ */