Re: [PATCH v4] media: v4l2-ctrl: add a helper function to add standard control with driver specific menu

2012-09-19 Thread Hans Verkuil
Hi Prabhakar,

I found some grammar issues, but also some (small) things that should be 
changed.


On Tue 18 September 2012 20:54:38 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar@ti.com
 
 Add helper function v4l2_ctrl_new_std_menu_items(), which adds
 a standard menu control, with driver specific menu.
 
 Signed-off-by: Lad, Prabhakar prabhakar@ti.com
 Signed-off-by: Manjunath Hadli manjunath.ha...@ti.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@infradead.org
 Cc: Hans de Goede hdego...@redhat.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Rob Landley r...@landley.net
 ---
 Changes for v4:
 1: Rather then adding a function to modify the menu, added a helper
function, that creates a new standard control with user specific
menu.
 
 Changes for v3:
 1: Fixed style/grammer issues as pointed by Hans.
Thanks Hans for providing the description.
 
 Changes for v2:
 1: Fixed review comments from Hans, to have return type as
void, add WARN_ON() for fail conditions, allow this fucntion
to modify the menu of custom controls.
 
  Documentation/video4linux/v4l2-controls.txt |   25 
  drivers/media/v4l2-core/v4l2-ctrls.c|   28 
 +++
  include/media/v4l2-ctrls.h  |   23 ++
  3 files changed, 76 insertions(+), 0 deletions(-)
 
 diff --git a/Documentation/video4linux/v4l2-controls.txt 
 b/Documentation/video4linux/v4l2-controls.txt
 index 43da22b..ad8e172 100644
 --- a/Documentation/video4linux/v4l2-controls.txt
 +++ b/Documentation/video4linux/v4l2-controls.txt
 @@ -136,11 +136,25 @@ Or alternatively for integer menu controls, by calling 
 v4l2_ctrl_new_int_menu:
   const struct v4l2_ctrl_ops *ops,
   u32 id, s32 max, s32 def, const s64 *qmenu_int);
  
 +Standard menu controls with driver specific menu are added by calling

with driver - with a driver

 +v4l2_ctrl_new_std_menu_items:
 +
 + struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(
 + struct v4l2_ctrl_handler *hdl,
 + const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
 + s32 skip_mask, s32 def, const char * const *qmenu_user);

I would recommend that qmenu_user is just renamed to qmenu.
The _user suffix suggests that this is a userspace-provided menu, which
is not the case.

 +
  These functions are typically called right after the v4l2_ctrl_handler_init:
  
   static const s64 exp_bias_qmenu[] = {
  -2, -1, 0, 1, 2
   };
 + static const char * const test_pattern[] = {
 + Disabled,
 + Vertical Bars,
 + Solid Black,
 + Solid White,
 + };
  
   v4l2_ctrl_handler_init(foo-ctrl_handler, nr_of_controls);
   v4l2_ctrl_new_std(foo-ctrl_handler, foo_ctrl_ops,
 @@ -156,6 +170,9 @@ These functions are typically called right after the 
 v4l2_ctrl_handler_init:
   ARRAY_SIZE(exp_bias_qmenu) - 1,
   ARRAY_SIZE(exp_bias_qmenu) / 2 - 1,
   exp_bias_qmenu);
 + v4l2_ctrl_new_std_menu_items(foo-ctrl_handler, foo_ctrl_ops,
 + V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
 + 0, test_pattern);
   ...
   if (foo-ctrl_handler.error) {
   int err = foo-ctrl_handler.error;
 @@ -185,6 +202,14 @@ v4l2_ctrl_new_std_menu in that it doesn't have the mask 
 argument and takes
  as the last argument an array of signed 64-bit integers that form an exact
  menu item list.
  
 +The v4l2_ctrl_new_std_menu_items funtion is very similar as

funtion - function
similar as - similar to

 +v4l2_ctrl_new_std_menu but takes a extra parameter qmenu_user, which is

a extra - an extra

 +driver specific menu but yet a standard menu control.

the driver specific menu for an otherwise standard menu control.

 +A good example for this control is the test pattern control for
 +capture/display/sensors devices that have the capability to generate test
 +patterns. These test patterns are hardware specific, so the contents of the
 +menu will vary from device to device.
 +
  Note that if something fails, the function will return NULL or an error and
  set ctrl_handler-error to the error code. If ctrl_handler-error was already
  set, then it will just return and do nothing. This is also true for
 diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
 b/drivers/media/v4l2-core/v4l2-ctrls.c
 index d731422..9ac1b75 100644
 --- a/drivers/media/v4l2-core/v4l2-ctrls.c
 +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
 @@ -1649,6 +1649,34 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct 
 v4l2_ctrl_handler *hdl,
  }
  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
  
 +/* Helper function for 

Re: [PATCH v4] media: v4l2-ctrl: add a helper function to add standard control with driver specific menu

2012-09-19 Thread Prabhakar Lad
Hi Hans,

Thanks for the review.

On Wed, Sep 19, 2012 at 1:23 PM, Hans Verkuil hverk...@xs4all.nl wrote:
 Hi Prabhakar,

 I found some grammar issues, but also some (small) things that should be 
 changed.


 On Tue 18 September 2012 20:54:38 Prabhakar Lad wrote:
 From: Lad, Prabhakar prabhakar@ti.com

 Add helper function v4l2_ctrl_new_std_menu_items(), which adds
 a standard menu control, with driver specific menu.

 Signed-off-by: Lad, Prabhakar prabhakar@ti.com
 Signed-off-by: Manjunath Hadli manjunath.ha...@ti.com
 Cc: Hans Verkuil hans.verk...@cisco.com
 Cc: Sakari Ailus sakari.ai...@iki.fi
 Cc: Sylwester Nawrocki s.nawro...@samsung.com
 Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
 Cc: Mauro Carvalho Chehab mche...@infradead.org
 Cc: Hans de Goede hdego...@redhat.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
 Cc: Rob Landley r...@landley.net
 ---
 Changes for v4:
 1: Rather then adding a function to modify the menu, added a helper
function, that creates a new standard control with user specific
menu.

 Changes for v3:
 1: Fixed style/grammer issues as pointed by Hans.
Thanks Hans for providing the description.

 Changes for v2:
 1: Fixed review comments from Hans, to have return type as
void, add WARN_ON() for fail conditions, allow this fucntion
to modify the menu of custom controls.

  Documentation/video4linux/v4l2-controls.txt |   25 
  drivers/media/v4l2-core/v4l2-ctrls.c|   28 
 +++
  include/media/v4l2-ctrls.h  |   23 ++
  3 files changed, 76 insertions(+), 0 deletions(-)

 diff --git a/Documentation/video4linux/v4l2-controls.txt 
 b/Documentation/video4linux/v4l2-controls.txt
 index 43da22b..ad8e172 100644
 --- a/Documentation/video4linux/v4l2-controls.txt
 +++ b/Documentation/video4linux/v4l2-controls.txt
 @@ -136,11 +136,25 @@ Or alternatively for integer menu controls, by calling 
 v4l2_ctrl_new_int_menu:
   const struct v4l2_ctrl_ops *ops,
   u32 id, s32 max, s32 def, const s64 *qmenu_int);

 +Standard menu controls with driver specific menu are added by calling

 with driver - with a driver

Ok.

 +v4l2_ctrl_new_std_menu_items:
 +
 + struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(
 + struct v4l2_ctrl_handler *hdl,
 + const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
 + s32 skip_mask, s32 def, const char * const *qmenu_user);

 I would recommend that qmenu_user is just renamed to qmenu.
 The _user suffix suggests that this is a userspace-provided menu, which
 is not the case.

Ok.

 +
  These functions are typically called right after the v4l2_ctrl_handler_init:

   static const s64 exp_bias_qmenu[] = {
  -2, -1, 0, 1, 2
   };
 + static const char * const test_pattern[] = {
 + Disabled,
 + Vertical Bars,
 + Solid Black,
 + Solid White,
 + };

   v4l2_ctrl_handler_init(foo-ctrl_handler, nr_of_controls);
   v4l2_ctrl_new_std(foo-ctrl_handler, foo_ctrl_ops,
 @@ -156,6 +170,9 @@ These functions are typically called right after the 
 v4l2_ctrl_handler_init:
   ARRAY_SIZE(exp_bias_qmenu) - 1,
   ARRAY_SIZE(exp_bias_qmenu) / 2 - 1,
   exp_bias_qmenu);
 + v4l2_ctrl_new_std_menu_items(foo-ctrl_handler, foo_ctrl_ops,
 + V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
 + 0, test_pattern);
   ...
   if (foo-ctrl_handler.error) {
   int err = foo-ctrl_handler.error;
 @@ -185,6 +202,14 @@ v4l2_ctrl_new_std_menu in that it doesn't have the mask 
 argument and takes
  as the last argument an array of signed 64-bit integers that form an exact
  menu item list.

 +The v4l2_ctrl_new_std_menu_items funtion is very similar as

 funtion - function
 similar as - similar to

Ok.
 +v4l2_ctrl_new_std_menu but takes a extra parameter qmenu_user, which is

 a extra - an extra

 +driver specific menu but yet a standard menu control.

 the driver specific menu for an otherwise standard menu control.

Ok.

 +A good example for this control is the test pattern control for
 +capture/display/sensors devices that have the capability to generate test
 +patterns. These test patterns are hardware specific, so the contents of the
 +menu will vary from device to device.
 +
  Note that if something fails, the function will return NULL or an error and
  set ctrl_handler-error to the error code. If ctrl_handler-error was 
 already
  set, then it will just return and do nothing. This is also true for
 diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
 b/drivers/media/v4l2-core/v4l2-ctrls.c
 index d731422..9ac1b75 100644
 --- a/drivers/media/v4l2-core/v4l2-ctrls.c
 +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
 @@ -1649,6 +1649,34 @@ struct v4l2_ctrl 

[PATCH v4] media: v4l2-ctrl: add a helper function to add standard control with driver specific menu

2012-09-18 Thread Prabhakar Lad
From: Lad, Prabhakar prabhakar@ti.com

Add helper function v4l2_ctrl_new_std_menu_items(), which adds
a standard menu control, with driver specific menu.

Signed-off-by: Lad, Prabhakar prabhakar@ti.com
Signed-off-by: Manjunath Hadli manjunath.ha...@ti.com
Cc: Hans Verkuil hans.verk...@cisco.com
Cc: Sakari Ailus sakari.ai...@iki.fi
Cc: Sylwester Nawrocki s.nawro...@samsung.com
Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Hans de Goede hdego...@redhat.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Guennadi Liakhovetski g.liakhovet...@gmx.de
Cc: Rob Landley r...@landley.net
---
Changes for v4:
1: Rather then adding a function to modify the menu, added a helper
   function, that creates a new standard control with user specific
   menu.

Changes for v3:
1: Fixed style/grammer issues as pointed by Hans.
   Thanks Hans for providing the description.

Changes for v2:
1: Fixed review comments from Hans, to have return type as
   void, add WARN_ON() for fail conditions, allow this fucntion
   to modify the menu of custom controls.

 Documentation/video4linux/v4l2-controls.txt |   25 
 drivers/media/v4l2-core/v4l2-ctrls.c|   28 +++
 include/media/v4l2-ctrls.h  |   23 ++
 3 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt 
b/Documentation/video4linux/v4l2-controls.txt
index 43da22b..ad8e172 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -136,11 +136,25 @@ Or alternatively for integer menu controls, by calling 
v4l2_ctrl_new_int_menu:
const struct v4l2_ctrl_ops *ops,
u32 id, s32 max, s32 def, const s64 *qmenu_int);
 
+Standard menu controls with driver specific menu are added by calling
+v4l2_ctrl_new_std_menu_items:
+
+   struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(
+   struct v4l2_ctrl_handler *hdl,
+   const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
+   s32 skip_mask, s32 def, const char * const *qmenu_user);
+
 These functions are typically called right after the v4l2_ctrl_handler_init:
 
static const s64 exp_bias_qmenu[] = {
   -2, -1, 0, 1, 2
};
+   static const char * const test_pattern[] = {
+   Disabled,
+   Vertical Bars,
+   Solid Black,
+   Solid White,
+   };
 
v4l2_ctrl_handler_init(foo-ctrl_handler, nr_of_controls);
v4l2_ctrl_new_std(foo-ctrl_handler, foo_ctrl_ops,
@@ -156,6 +170,9 @@ These functions are typically called right after the 
v4l2_ctrl_handler_init:
ARRAY_SIZE(exp_bias_qmenu) - 1,
ARRAY_SIZE(exp_bias_qmenu) / 2 - 1,
exp_bias_qmenu);
+   v4l2_ctrl_new_std_menu_items(foo-ctrl_handler, foo_ctrl_ops,
+   V4L2_CID_TEST_PATTERN, ARRAY_SIZE(test_pattern) - 1, 0,
+   0, test_pattern);
...
if (foo-ctrl_handler.error) {
int err = foo-ctrl_handler.error;
@@ -185,6 +202,14 @@ v4l2_ctrl_new_std_menu in that it doesn't have the mask 
argument and takes
 as the last argument an array of signed 64-bit integers that form an exact
 menu item list.
 
+The v4l2_ctrl_new_std_menu_items funtion is very similar as
+v4l2_ctrl_new_std_menu but takes a extra parameter qmenu_user, which is
+driver specific menu but yet a standard menu control.
+A good example for this control is the test pattern control for
+capture/display/sensors devices that have the capability to generate test
+patterns. These test patterns are hardware specific, so the contents of the
+menu will vary from device to device.
+
 Note that if something fails, the function will return NULL or an error and
 set ctrl_handler-error to the error code. If ctrl_handler-error was already
 set, then it will just return and do nothing. This is also true for
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c 
b/drivers/media/v4l2-core/v4l2-ctrls.c
index d731422..9ac1b75 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1649,6 +1649,34 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct 
v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
 
+/* Helper function for standard menu controls with user defined menu */
+struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
+   const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
+   s32 mask, s32 def, const char * const *qmenu_user)
+{
+   const char * const *qmenu = v4l2_ctrl_get_menu(id);
+   const char *name;
+   enum v4l2_ctrl_type type;
+   s32 min;
+   s32 step;
+   u32 flags;
+
+   if (!qmenu) {
+   handler_set_err(hdl, -EINVAL);
+