[PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Antonio Ospite]

Hi,

here are a couple more of controls for the gspca ov534 subdriver.

In order to control the HUE value the sensor expects sin(HUE) and
cos(HUE) to be set so I decided to reuse the fixed point implementation
of sine and cosine from drivers/input/fixp-arith.h, see patches 2 and 3.

Dmitry, can the movement of fixp-arith.h in patch 2 go via the media
tree?  That should ease up the integration of patch 3 in this series
I think.

Jonathan, maybe fixp_sin() and fixp_cos() can be used in
drivers/media/video/ov7670.c too where currently ov7670_sine() and
ov7670_cosine() are defined, but I didn't want to send a patch I could
not test.

BTW What is the usual way to communicate these cross-subsystem stuff?
I CC-ed everybody only on the cover letter and on patches 2 and 3 which
are the ones concerning somehow both "input" and "media".

Thanks,
   Antonio


Antonio Ospite (3):
  [media] gspca - ov534: Add Saturation control
  Input: move drivers/input/fixp-arith.h to include/linux
  [media] gspca - ov534: Add Hue control

 drivers/input/ff-memless.c        |    3 +-
 drivers/input/fixp-arith.h        |   87 ------------------------------------
 drivers/media/video/gspca/ov534.c |   89 ++++++++++++++++++++++++++++++++++++-
 include/linux/fixp-arith.h        |   87 ++++++++++++++++++++++++++++++++++++
 4 files changed, 176 insertions(+), 90 deletions(-)
 delete mode 100644 drivers/input/fixp-arith.h
 create mode 100644 include/linux/fixp-arith.h

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[PATCH 2/3] Input: move drivers/input/fixp-arith.h to include/linux

[Antonio Ospite]

Move drivers/input/fixp-arith.h to include/linux so that the functions
defined there can be used by other subsystems, for instance some video
devices ISPs can control the output HUE value by setting registers for
sin(HUE) and cos(HUE).

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/input/ff-memless.c                    |    3 +--
 {drivers/input => include/linux}/fixp-arith.h |    0
 2 files changed, 1 insertion(+), 2 deletions(-)
 rename {drivers/input => include/linux}/fixp-arith.h (100%)

diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 117a59a..5f55885 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -31,8 +31,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/jiffies.h>
-
-#include "fixp-arith.h"
+#include <linux/fixp-arith.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Anssi Hannula <anssi.hannula@gmail.com>");
diff --git a/drivers/input/fixp-arith.h b/include/linux/fixp-arith.h
similarity index 100%
rename from drivers/input/fixp-arith.h
rename to include/linux/fixp-arith.h
-- 
1.7.10

Re: [PATCH 2/3] Input: move drivers/input/fixp-arith.h to include/linux

[Dmitry Torokhov]

On Mon, Apr 23, 2012 at 03:21:06PM +0200, Antonio Ospite wrote:
> Move drivers/input/fixp-arith.h to include/linux so that the functions
> defined there can be used by other subsystems, for instance some video
> devices ISPs can control the output HUE value by setting registers for
> sin(HUE) and cos(HUE).
> 
> Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/input/ff-memless.c                    |    3 +--
>  {drivers/input => include/linux}/fixp-arith.h |    0
>  2 files changed, 1 insertion(+), 2 deletions(-)
>  rename {drivers/input => include/linux}/fixp-arith.h (100%)
> 
> diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
> index 117a59a..5f55885 100644
> --- a/drivers/input/ff-memless.c
> +++ b/drivers/input/ff-memless.c
> @@ -31,8 +31,7 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/jiffies.h>
> -
> -#include "fixp-arith.h"
> +#include <linux/fixp-arith.h>
>  
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Anssi Hannula <anssi.hannula@gmail.com>");
> diff --git a/drivers/input/fixp-arith.h b/include/linux/fixp-arith.h
> similarity index 100%
> rename from drivers/input/fixp-arith.h
> rename to include/linux/fixp-arith.h
> -- 
> 1.7.10
> 

-- 
Dmitry

[PATCH 3/3] [media] gspca - ov534: Add Hue control

[Antonio Ospite]

Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
 drivers/media/video/gspca/ov534.c |   61 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/media/video/gspca/ov534.c b/drivers/media/video/gspca/ov534.c
index 45139b5..1a437b5 100644
--- a/drivers/media/video/gspca/ov534.c
+++ b/drivers/media/video/gspca/ov534.c
@@ -34,6 +34,8 @@
 
 #include "gspca.h"
 
+#include <linux/fixp-arith.h>
+
 #define OV534_REG_ADDRESS	0xf1	/* sensor address */
 #define OV534_REG_SUBADDR	0xf2
 #define OV534_REG_WRITE		0xf3
@@ -53,6 +55,7 @@ MODULE_LICENSE("GPL");
 
 /* controls */
 enum e_ctrl {
+	HUE,
 	SATURATION,
 	BRIGHTNESS,
 	CONTRAST,
@@ -88,6 +91,7 @@ enum sensors {
 };
 
 /* V4L2 controls supported by the driver */
+static void sethue(struct gspca_dev *gspca_dev);
 static void setsaturation(struct gspca_dev *gspca_dev);
 static void setbrightness(struct gspca_dev *gspca_dev);
 static void setcontrast(struct gspca_dev *gspca_dev);
@@ -105,6 +109,18 @@ static int sd_start(struct gspca_dev *gspca_dev);
 static void sd_stopN(struct gspca_dev *gspca_dev);
 
 static const struct ctrl sd_ctrls[] = {
+[HUE] = {
+		{
+			.id      = V4L2_CID_HUE,
+			.type    = V4L2_CTRL_TYPE_INTEGER,
+			.name    = "Hue",
+			.minimum = -90,
+			.maximum = 90,
+			.step    = 1,
+			.default_value = 0,
+		},
+		.set_control = sethue
+	},
 [SATURATION] = {
 		{
 			.id      = V4L2_CID_SATURATION,
@@ -698,7 +714,7 @@ static const u8 sensor_init_772x[][2] = {
 	{ 0x9c, 0x20 },
 	{ 0x9e, 0x81 },
 
-	{ 0xa6, 0x06 },
+	{ 0xa6, 0x07 },
 	{ 0x7e, 0x0c },
 	{ 0x7f, 0x16 },
 	{ 0x80, 0x2a },
@@ -969,6 +985,48 @@ static void set_frame_rate(struct gspca_dev *gspca_dev)
 	PDEBUG(D_PROBE, "frame_rate: %d", r->fps);
 }
 
+static void sethue(struct gspca_dev *gspca_dev)
+{
+	struct sd *sd = (struct sd *) gspca_dev;
+	int val;
+
+	val = sd->ctrls[HUE].val;
+	if (sd->sensor == SENSOR_OV767x) {
+		/* TBD */
+	} else {
+		s16 huesin;
+		s16 huecos;
+
+		/* fixp_sin and fixp_cos accept only positive values, while
+		 * our val is between -90 and 90
+		 */
+		val += 360;
+
+		/* According to the datasheet the registers expect HUESIN and
+		 * HUECOS to be the result of the trigonometric functions,
+		 * scaled by 0x80.
+		 *
+		 * The 0x100 here represents the maximun absolute value
+		 * returned byt fixp_sin and fixp_cos, so the scaling will
+		 * consider the result like in the interval [-1.0, 1.0].
+		 */
+		huesin = fixp_sin(val) * 0x80 / 0x100;
+		huecos = fixp_cos(val) * 0x80 / 0x100;
+
+		if (huesin < 0) {
+			sccb_reg_write(gspca_dev, 0xab,
+				sccb_reg_read(gspca_dev, 0xab) | 0x2);
+			huesin = -huesin;
+		} else {
+			sccb_reg_write(gspca_dev, 0xab,
+				sccb_reg_read(gspca_dev, 0xab) & ~0x2);
+
+		}
+		sccb_reg_write(gspca_dev, 0xa9, (u8)huecos);
+		sccb_reg_write(gspca_dev, 0xaa, (u8)huesin);
+	}
+}
+
 static void setsaturation(struct gspca_dev *gspca_dev)
 {
 	struct sd *sd = (struct sd *) gspca_dev;
@@ -1337,6 +1395,7 @@ static int sd_start(struct gspca_dev *gspca_dev)
 	if (!(gspca_dev->ctrl_dis & (1 << GAIN)))
 		setgain(gspca_dev);
 	setexposure(gspca_dev);
+	sethue(gspca_dev);
 	setsaturation(gspca_dev);
 	setbrightness(gspca_dev);
 	setcontrast(gspca_dev);
-- 
1.7.10

Re: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 309 bytes --]

If you "reply to all" please fix Johann Deneux's address, I mis-wrote
it, sorry.

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Dmitry Torokhov]

On Mon, Apr 23, 2012 at 03:21:04PM +0200, Antonio Ospite wrote:
> Hi,
> 
> here are a couple more of controls for the gspca ov534 subdriver.
> 
> In order to control the HUE value the sensor expects sin(HUE) and
> cos(HUE) to be set so I decided to reuse the fixed point implementation
> of sine and cosine from drivers/input/fixp-arith.h, see patches 2 and 3.
> 
> Dmitry, can the movement of fixp-arith.h in patch 2 go via the media
> tree?  That should ease up the integration of patch 3 in this series
> I think.

Yep, this is fine with me.

> 
> Jonathan, maybe fixp_sin() and fixp_cos() can be used in
> drivers/media/video/ov7670.c too where currently ov7670_sine() and
> ov7670_cosine() are defined, but I didn't want to send a patch I could
> not test.
> 
> BTW What is the usual way to communicate these cross-subsystem stuff?
> I CC-ed everybody only on the cover letter and on patches 2 and 3 which
> are the ones concerning somehow both "input" and "media".
> 
> Thanks,
>    Antonio
> 
> 
> Antonio Ospite (3):
>   [media] gspca - ov534: Add Saturation control
>   Input: move drivers/input/fixp-arith.h to include/linux
>   [media] gspca - ov534: Add Hue control
> 
>  drivers/input/ff-memless.c        |    3 +-
>  drivers/input/fixp-arith.h        |   87 ------------------------------------
>  drivers/media/video/gspca/ov534.c |   89 ++++++++++++++++++++++++++++++++++++-
>  include/linux/fixp-arith.h        |   87 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 176 insertions(+), 90 deletions(-)
>  delete mode 100644 drivers/input/fixp-arith.h
>  create mode 100644 include/linux/fixp-arith.h
> 
> -- 
> Antonio Ospite
> http://ao2.it
> 
> A: Because it messes up the order in which people normally read text.
>    See http://en.wikipedia.org/wiki/Posting_style
> Q: Why is top-posting such a bad thing?

-- 
Dmitry

Re: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Jonathan Corbet]

On Mon, 23 Apr 2012 15:21:04 +0200
Antonio Ospite <ospite@studenti.unina.it> wrote:

> Jonathan, maybe fixp_sin() and fixp_cos() can be used in
> drivers/media/video/ov7670.c too where currently ov7670_sine() and
> ov7670_cosine() are defined, but I didn't want to send a patch I could
> not test.

Seems like a good idea.  No reason to have multiple such hacks in the
kernel; I'll look at dumping the ov7670 version when I get a chance.  That
may not be all that soon, though; life is a bit challenging at the moment.

One concern is that if we're going to add users to fixp-arith.h, some of
it should maybe go to a C file.  Otherwise we'll create duplicated copies
of the cos_table array for each user.  I'm not sure the functions need to
be inline either; nobody expects cos() to be blindingly fast.

Thanks,

jon

Re: [PATCH 0/3] gspca - ov534: saturation and hue (using fixp-arith.h)

[Antonio Ospite]

[-- Attachment #1: Type: text/plain, Size: 1403 bytes --]

On Mon, 23 Apr 2012 14:16:25 -0600
Jonathan Corbet <corbet@lwn.net> wrote:

> On Mon, 23 Apr 2012 15:21:04 +0200
> Antonio Ospite <ospite@studenti.unina.it> wrote:
> 
> > Jonathan, maybe fixp_sin() and fixp_cos() can be used in
> > drivers/media/video/ov7670.c too where currently ov7670_sine() and
> > ov7670_cosine() are defined, but I didn't want to send a patch I could
> > not test.
> 
> Seems like a good idea.  No reason to have multiple such hacks in the
> kernel; I'll look at dumping the ov7670 version when I get a chance.  That
> may not be all that soon, though; life is a bit challenging at the moment.
> 
> One concern is that if we're going to add users to fixp-arith.h, some of
> it should maybe go to a C file.  Otherwise we'll create duplicated copies
> of the cos_table array for each user.  I'm not sure the functions need to
> be inline either; nobody expects cos() to be blindingly fast.
> 

Jonathan, does lib/fixp-arith.c sound OK to you? I'll put that on my
TODO, unless Johann wans to do some more rework; in any case I think we
can still merge this patchset like it is right now.

Jean-Francois will you take care of that?

Thanks,
   Antonio

-- 
Antonio Ospite
http://ao2.it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]