linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] media: v4l2-ctrls: add control for test pattern
@ 2012-09-07  8:41 Prabhakar Lad
  2012-09-07 18:20 ` Sakari Ailus
  0 siblings, 1 reply; 6+ messages in thread
From: Prabhakar Lad @ 2012-09-07  8:41 UTC (permalink / raw)
  To: LMML
  Cc: dlos, linux-kernel, Manjunath Hadli, Laurent Pinchart, linux-doc,
	Hans Verkuil, Sylwester Nawrocki, Lad, Prabhakar, Sakari Ailus,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park, Rob Landley

From: Lad, Prabhakar <prabhakar.lad@ti.com>

add V4L2_CID_TEST_PATTERN of type menu, which determines
the internal test pattern selected by the device.

Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
Cc: Sakari Ailus <sakari.ailus@iki.fi>
Cc: Hans Verkuil <hans.verkuil@cisco.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Rob Landley <rob@landley.net>
---
This patches has one checkpatch warning for line over
80 characters altough it can be avoided I have kept it
for consistency.

Changes for v2:
1: Included display devices in the description for test pattern
   as pointed by Hans.
2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
   pointed by Sylwester.
3: Removed the test patterns from menu as the are hardware specific
   as pointed by Sakari.

 Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
 include/linux/videodev2.h                    |    4 ++++
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index ad873ea..173934e 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
 	      </tbody>
 	    </entrytbl>
 	  </row>
+	  <row>
+	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry>
+	    <entry>menu</entry>
+	  </row>
+	  <row id="v4l2-test-pattern">
+	    <entry spanname="descr"> The Capture/Display/Sensors have the capability
+	    to generate internal test patterns and this are hardware specific. This
+	    test patterns are used to test a device is properly working and can generate
+	    the desired waveforms that it supports.</entry>
+	  </row>
+	  <row>
+	    <entrytbl spanname="descr" cols="2">
+	      <tbody valign="top">
+	        <row>
+	         <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
+	          <entry>Test pattern generation is disabled</entry>
+	        </row>
+	      </tbody>
+	    </entrytbl>
+	  </row>
 	<row><entry></entry></row>
 	</tbody>
       </tgroup>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 8f2f40b..d731422 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"Advanced",
 		NULL,
 	};
+	static const char * const test_pattern[] = {
+		"Disabled",
+		NULL,
+	};
 
 	switch (id) {
 	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
@@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		return jpeg_chroma_subsampling;
 	case V4L2_CID_DPCM_PREDICTOR:
 		return dpcm_predictor;
+	case V4L2_CID_TEST_PATTERN:
+		return test_pattern;
 
 	default:
 		return NULL;
@@ -740,6 +746,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_LINK_FREQ:		return "Link Frequency";
 	case V4L2_CID_PIXEL_RATE:		return "Pixel Rate";
 	case V4L2_CID_DPCM_PREDICTOR:		return "DPCM Predictor";
+	case V4L2_CID_TEST_PATTERN:		return "Test Pattern";
 
 	default:
 		return NULL;
@@ -841,6 +848,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_EXPOSURE_METERING:
 	case V4L2_CID_SCENE_MODE:
 	case V4L2_CID_DPCM_PREDICTOR:
+	case V4L2_CID_TEST_PATTERN:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
 	case V4L2_CID_LINK_FREQ:
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index ca9fb78..6a2d499 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -2005,6 +2005,10 @@ enum v4l2_dpcm_predictor {
 	V4L2_DPCM_PREDICTOR_SIMPLE	= 0,
 	V4L2_DPCM_PREDICTOR_ADVANCED	= 1,
 };
+#define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 4)
+enum v4l2_test_pattern {
+	V4L2_TEST_PATTERN_DISABLED		= 0,
+};
 
 /*
  *	T U N I N G
-- 
1.7.0.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern
  2012-09-07  8:41 [PATCH v2] media: v4l2-ctrls: add control for test pattern Prabhakar Lad
@ 2012-09-07 18:20 ` Sakari Ailus
  2012-09-07 19:09   ` Prabhakar Lad
  2012-09-08 11:11   ` Hans Verkuil
  0 siblings, 2 replies; 6+ messages in thread
From: Sakari Ailus @ 2012-09-07 18:20 UTC (permalink / raw)
  To: Prabhakar Lad
  Cc: LMML, dlos, linux-kernel, Manjunath Hadli, Laurent Pinchart,
	linux-doc, Hans Verkuil, Sylwester Nawrocki,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park, Rob Landley

Hi Prabhakar,

Thanks for the patch!

Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>
> add V4L2_CID_TEST_PATTERN of type menu, which determines
> the internal test pattern selected by the device.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> Cc: Sakari Ailus <sakari.ailus@iki.fi>
> Cc: Hans Verkuil <hans.verkuil@cisco.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Rob Landley <rob@landley.net>
> ---
> This patches has one checkpatch warning for line over
> 80 characters altough it can be avoided I have kept it
> for consistency.
>
> Changes for v2:
> 1: Included display devices in the description for test pattern
>     as pointed by Hans.
> 2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
>     pointed by Sylwester.
> 3: Removed the test patterns from menu as the are hardware specific
>     as pointed by Sakari.
>
>   Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++++++
>   drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
>   include/linux/videodev2.h                    |    4 ++++
>   3 files changed, 32 insertions(+), 0 deletions(-)
>
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index ad873ea..173934e 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
>   	      </tbody>
>   	    </entrytbl>
>   	  </row>
> +	  <row>
> +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry>
> +	    <entry>menu</entry>
> +	  </row>
> +	  <row id="v4l2-test-pattern">
> +	    <entry spanname="descr"> The Capture/Display/Sensors have the capability
> +	    to generate internal test patterns and this are hardware specific. This
> +	    test patterns are used to test a device is properly working and can generate
> +	    the desired waveforms that it supports.</entry>
> +	  </row>
> +	  <row>
> +	    <entrytbl spanname="descr" cols="2">
> +	      <tbody valign="top">
> +	        <row>
> +	         <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
> +	          <entry>Test pattern generation is disabled</entry>
> +	        </row>
> +	      </tbody>
> +	    </entrytbl>
> +	  </row>
>   	<row><entry></entry></row>
>   	</tbody>
>         </tgroup>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 8f2f40b..d731422 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>   		"Advanced",
>   		NULL,
>   	};
> +	static const char * const test_pattern[] = {
> +		"Disabled",
> +		NULL,
> +	};
>
>   	switch (id) {
>   	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> @@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>   		return jpeg_chroma_subsampling;
>   	case V4L2_CID_DPCM_PREDICTOR:
>   		return dpcm_predictor;
> +	case V4L2_CID_TEST_PATTERN:
> +		return test_pattern;

I think it's not necessary to define test_pattern (nor be prepared to 
return it) since the menu is going to be device specific. So the driver 
is responsible for all of the menu items. Such menus are created using 
v4l2_ctrl_new_custom() instead of v4l2_ctrl_new_std_menu().

Looks good to me otherwise.

Kind regards,

-- 
Sakari Ailus
sakari.ailus@iki.fi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern
  2012-09-07 18:20 ` Sakari Ailus
@ 2012-09-07 19:09   ` Prabhakar Lad
  2012-09-08 11:11   ` Hans Verkuil
  1 sibling, 0 replies; 6+ messages in thread
From: Prabhakar Lad @ 2012-09-07 19:09 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Prabhakar Lad, dlos, Rob Landley, linux-doc, linux-kernel,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park,
	Hans Verkuil, Sylwester Nawrocki, LMML

Hi Sakari,

Thanks for the review.

On Fri, Sep 7, 2012 at 11:50 PM, Sakari Ailus <sakari.ailus@iki.fi> wrote:
> Hi Prabhakar,
>
> Thanks for the patch!
>
>
> Prabhakar Lad wrote:
>>
>> From: Lad, Prabhakar <prabhakar.lad@ti.com>
>>
>> add V4L2_CID_TEST_PATTERN of type menu, which determines
>> the internal test pattern selected by the device.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
>> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
>> Cc: Sakari Ailus <sakari.ailus@iki.fi>
>> Cc: Hans Verkuil <hans.verkuil@cisco.com>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
>> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> Cc: Kyungmin Park <kyungmin.park@samsung.com>
>> Cc: Rob Landley <rob@landley.net>
>> ---
>> This patches has one checkpatch warning for line over
>> 80 characters altough it can be avoided I have kept it
>> for consistency.
>>
>> Changes for v2:
>> 1: Included display devices in the description for test pattern
>>     as pointed by Hans.
>> 2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
>>     pointed by Sylwester.
>> 3: Removed the test patterns from menu as the are hardware specific
>>     as pointed by Sakari.
>>
>>   Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++++++
>>   drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
>>   include/linux/videodev2.h                    |    4 ++++
>>   3 files changed, 32 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/DocBook/media/v4l/controls.xml
>> b/Documentation/DocBook/media/v4l/controls.xml
>> index ad873ea..173934e 100644
>> --- a/Documentation/DocBook/media/v4l/controls.xml
>> +++ b/Documentation/DocBook/media/v4l/controls.xml
>> @@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
>>               </tbody>
>>             </entrytbl>
>>           </row>
>> +         <row>
>> +           <entry
>> spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry>
>> +           <entry>menu</entry>
>> +         </row>
>> +         <row id="v4l2-test-pattern">
>> +           <entry spanname="descr"> The Capture/Display/Sensors have the
>> capability
>> +           to generate internal test patterns and this are hardware
>> specific. This
>> +           test patterns are used to test a device is properly working
>> and can generate
>> +           the desired waveforms that it supports.</entry>
>> +         </row>
>> +         <row>
>> +           <entrytbl spanname="descr" cols="2">
>> +             <tbody valign="top">
>> +               <row>
>> +
>> <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
>> +                 <entry>Test pattern generation is disabled</entry>
>> +               </row>
>> +             </tbody>
>> +           </entrytbl>
>> +         </row>
>>         <row><entry></entry></row>
>>         </tbody>
>>         </tgroup>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
>> b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 8f2f40b..d731422 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>                 "Advanced",
>>                 NULL,
>>         };
>> +       static const char * const test_pattern[] = {
>> +               "Disabled",
>> +               NULL,
>> +       };
>>
>>         switch (id) {
>>         case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
>> @@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>                 return jpeg_chroma_subsampling;
>>         case V4L2_CID_DPCM_PREDICTOR:
>>                 return dpcm_predictor;
>> +       case V4L2_CID_TEST_PATTERN:
>> +               return test_pattern;
>
>
> I think it's not necessary to define test_pattern (nor be prepared to return
> it) since the menu is going to be device specific. So the driver is
> responsible for all of the menu items. Such menus are created using
> v4l2_ctrl_new_custom() instead of v4l2_ctrl_new_std_menu().
>
Ok.

Regrads,
--Prabhakar Lad

> Looks good to me otherwise.
>
> Kind regards,
>
> --
> Sakari Ailus
> sakari.ailus@iki.fi
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@linux.davincidsp.com
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern
  2012-09-07 18:20 ` Sakari Ailus
  2012-09-07 19:09   ` Prabhakar Lad
@ 2012-09-08 11:11   ` Hans Verkuil
  2012-09-09  7:40     ` Sakari Ailus
  1 sibling, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2012-09-08 11:11 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Prabhakar Lad, LMML, dlos, linux-kernel, Manjunath Hadli,
	Laurent Pinchart, linux-doc, Hans Verkuil, Sylwester Nawrocki,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park, Rob Landley

On Fri September 7 2012 20:20:51 Sakari Ailus wrote:
> Hi Prabhakar,
> 
> Thanks for the patch!
> 
> Prabhakar Lad wrote:
> > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> >
> > add V4L2_CID_TEST_PATTERN of type menu, which determines
> > the internal test pattern selected by the device.
> >
> > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Rob Landley <rob@landley.net>
> > ---
> > This patches has one checkpatch warning for line over
> > 80 characters altough it can be avoided I have kept it
> > for consistency.
> >
> > Changes for v2:
> > 1: Included display devices in the description for test pattern
> >     as pointed by Hans.
> > 2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
> >     pointed by Sylwester.
> > 3: Removed the test patterns from menu as the are hardware specific
> >     as pointed by Sakari.
> >
> >   Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++++++
> >   drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
> >   include/linux/videodev2.h                    |    4 ++++
> >   3 files changed, 32 insertions(+), 0 deletions(-)
> >
> > diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> > index ad873ea..173934e 100644
> > --- a/Documentation/DocBook/media/v4l/controls.xml
> > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > @@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
> >   	      </tbody>
> >   	    </entrytbl>
> >   	  </row>
> > +	  <row>
> > +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry>
> > +	    <entry>menu</entry>
> > +	  </row>
> > +	  <row id="v4l2-test-pattern">
> > +	    <entry spanname="descr"> The Capture/Display/Sensors have the capability
> > +	    to generate internal test patterns and this are hardware specific. This
> > +	    test patterns are used to test a device is properly working and can generate
> > +	    the desired waveforms that it supports.</entry>
> > +	  </row>
> > +	  <row>
> > +	    <entrytbl spanname="descr" cols="2">
> > +	      <tbody valign="top">
> > +	        <row>
> > +	         <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
> > +	          <entry>Test pattern generation is disabled</entry>
> > +	        </row>
> > +	      </tbody>
> > +	    </entrytbl>
> > +	  </row>
> >   	<row><entry></entry></row>
> >   	</tbody>
> >         </tgroup>
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 8f2f40b..d731422 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >   		"Advanced",
> >   		NULL,
> >   	};
> > +	static const char * const test_pattern[] = {
> > +		"Disabled",
> > +		NULL,
> > +	};
> >
> >   	switch (id) {
> >   	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > @@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> >   		return jpeg_chroma_subsampling;
> >   	case V4L2_CID_DPCM_PREDICTOR:
> >   		return dpcm_predictor;
> > +	case V4L2_CID_TEST_PATTERN:
> > +		return test_pattern;
> 
> I think it's not necessary to define test_pattern (nor be prepared to 
> return it) since the menu is going to be device specific. So the driver 
> is responsible for all of the menu items. Such menus are created using 
> v4l2_ctrl_new_custom() instead of v4l2_ctrl_new_std_menu().
> 
> Looks good to me otherwise.

I would suggest that we *do* make this a standard control, but the menu consists
of just one item: "Disabled". After creating the control you can just set the
ctrl->qmenu pointer to the device-specific menu. I like using standard controls
because they guarantee standard naming and type conventions. They are also
easier to use in an application.

This would obvious require good documentation, both in the code and in the
spec.

BTW, see also this patch that needs to use the test pattern control:

http://www.spinics.net/lists/linux-media/msg52747.html

Regards,

	Hans

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern
  2012-09-08 11:11   ` Hans Verkuil
@ 2012-09-09  7:40     ` Sakari Ailus
  2012-09-13  1:04       ` Laurent Pinchart
  0 siblings, 1 reply; 6+ messages in thread
From: Sakari Ailus @ 2012-09-09  7:40 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: Prabhakar Lad, LMML, dlos, linux-kernel, Manjunath Hadli,
	Laurent Pinchart, linux-doc, Hans Verkuil, Sylwester Nawrocki,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park, Rob Landley

Hi Hans,

On Sat, Sep 08, 2012 at 01:11:04PM +0200, Hans Verkuil wrote:
> On Fri September 7 2012 20:20:51 Sakari Ailus wrote:
> > Hi Prabhakar,
> > 
> > Thanks for the patch!
> > 
> > Prabhakar Lad wrote:
> > > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> > >
> > > add V4L2_CID_TEST_PATTERN of type menu, which determines
> > > the internal test pattern selected by the device.
> > >
> > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Rob Landley <rob@landley.net>
> > > ---
> > > This patches has one checkpatch warning for line over
> > > 80 characters altough it can be avoided I have kept it
> > > for consistency.
> > >
> > > Changes for v2:
> > > 1: Included display devices in the description for test pattern
> > >     as pointed by Hans.
> > > 2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
> > >     pointed by Sylwester.
> > > 3: Removed the test patterns from menu as the are hardware specific
> > >     as pointed by Sakari.
> > >
> > >   Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++++++
> > >   drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
> > >   include/linux/videodev2.h                    |    4 ++++
> > >   3 files changed, 32 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> > > index ad873ea..173934e 100644
> > > --- a/Documentation/DocBook/media/v4l/controls.xml
> > > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > > @@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
> > >   	      </tbody>
> > >   	    </entrytbl>
> > >   	  </row>
> > > +	  <row>
> > > +	    <entry spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry>
> > > +	    <entry>menu</entry>
> > > +	  </row>
> > > +	  <row id="v4l2-test-pattern">
> > > +	    <entry spanname="descr"> The Capture/Display/Sensors have the capability
> > > +	    to generate internal test patterns and this are hardware specific. This
> > > +	    test patterns are used to test a device is properly working and can generate
> > > +	    the desired waveforms that it supports.</entry>
> > > +	  </row>
> > > +	  <row>
> > > +	    <entrytbl spanname="descr" cols="2">
> > > +	      <tbody valign="top">
> > > +	        <row>
> > > +	         <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
> > > +	          <entry>Test pattern generation is disabled</entry>
> > > +	        </row>
> > > +	      </tbody>
> > > +	    </entrytbl>
> > > +	  </row>
> > >   	<row><entry></entry></row>
> > >   	</tbody>
> > >         </tgroup>
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index 8f2f40b..d731422 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >   		"Advanced",
> > >   		NULL,
> > >   	};
> > > +	static const char * const test_pattern[] = {
> > > +		"Disabled",
> > > +		NULL,
> > > +	};
> > >
> > >   	switch (id) {
> > >   	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > > @@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > >   		return jpeg_chroma_subsampling;
> > >   	case V4L2_CID_DPCM_PREDICTOR:
> > >   		return dpcm_predictor;
> > > +	case V4L2_CID_TEST_PATTERN:
> > > +		return test_pattern;
> > 
> > I think it's not necessary to define test_pattern (nor be prepared to 
> > return it) since the menu is going to be device specific. So the driver 
> > is responsible for all of the menu items. Such menus are created using 
> > v4l2_ctrl_new_custom() instead of v4l2_ctrl_new_std_menu().
> > 
> > Looks good to me otherwise.
> 
> I would suggest that we *do* make this a standard control, but the menu consists
> of just one item: "Disabled". After creating the control you can just set the
> ctrl->qmenu pointer to the device-specific menu. I like using standard controls
> because they guarantee standard naming and type conventions. They are also
> easier to use in an application.

That's not quite enough. Also menu_skip_mask and max also need to be
replaced. In a general case min as well. It's easy to do mistakes in that
--- how about a separate function for doing that? It'd be also nice to be
able to use the existing standardised menu item strings, but that's just an
extra plus.

However I think it's beyond this patch, which I think then is be fine w/o
modifications. So on my behalf,

Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

Cheers,

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern
  2012-09-09  7:40     ` Sakari Ailus
@ 2012-09-13  1:04       ` Laurent Pinchart
  0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2012-09-13  1:04 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Hans Verkuil, Prabhakar Lad, LMML, dlos, linux-kernel,
	Manjunath Hadli, linux-doc, Hans Verkuil, Sylwester Nawrocki,
	Mauro Carvalho Chehab, Hans de Goede, Kyungmin Park, Rob Landley

Hi Sakari,

On Sunday 09 September 2012 10:40:17 Sakari Ailus wrote:
> On Sat, Sep 08, 2012 at 01:11:04PM +0200, Hans Verkuil wrote:
> > On Fri September 7 2012 20:20:51 Sakari Ailus wrote:
> > > Prabhakar Lad wrote:
> > > > From: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > > 
> > > > add V4L2_CID_TEST_PATTERN of type menu, which determines
> > > > the internal test pattern selected by the device.
> > > > 
> > > > Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> > > > Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> > > > Cc: Sakari Ailus <sakari.ailus@iki.fi>
> > > > Cc: Hans Verkuil <hans.verkuil@cisco.com>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Cc: Mauro Carvalho Chehab <mchehab@infradead.org>
> > > > Cc: Sylwester Nawrocki <s.nawrocki@samsung.com>
> > > > Cc: Hans de Goede <hdegoede@redhat.com>
> > > > Cc: Kyungmin Park <kyungmin.park@samsung.com>
> > > > Cc: Rob Landley <rob@landley.net>
> > > > ---
> > > > This patches has one checkpatch warning for line over
> > > > 80 characters altough it can be avoided I have kept it
> > > > for consistency.
> > > > 
> > > > Changes for v2:
> > > > 1: Included display devices in the description for test pattern
> > > >     as pointed by Hans.
> > > > 
> > > > 2: In the menu replaced 'Test Pattern Disabled' by 'Disabled' as
> > > >     pointed by Sylwester.
> > > > 
> > > > 3: Removed the test patterns from menu as the are hardware specific
> > > >     as pointed by Sakari.
> > > >   
> > > >   Documentation/DocBook/media/v4l/controls.xml |   20 ++++++++++++++++
> > > >   drivers/media/v4l2-core/v4l2-ctrls.c         |    8 ++++++++
> > > >   include/linux/videodev2.h                    |    4 ++++
> > > >   3 files changed, 32 insertions(+), 0 deletions(-)
> > > > 
> > > > diff --git a/Documentation/DocBook/media/v4l/controls.xml
> > > > b/Documentation/DocBook/media/v4l/controls.xml index ad873ea..173934e
> > > > 100644
> > > > --- a/Documentation/DocBook/media/v4l/controls.xml
> > > > +++ b/Documentation/DocBook/media/v4l/controls.xml
> > > > @@ -4311,6 +4311,26 @@ interface and may change in the future.</para>
> > > >   	      </tbody>
> > > >   	    </entrytbl>
> > > >   	  </row>
> > > > +	  <row>
> > > > +	    <entry
> > > > spanname="id"><constant>V4L2_CID_TEST_PATTERN</constant></entry> +	  
> > > >  <entry>menu</entry>
> > > > +	  </row>
> > > > +	  <row id="v4l2-test-pattern">
> > > > +	    <entry spanname="descr"> The Capture/Display/Sensors have the
> > > > capability
> > > > +	    to generate internal test patterns and this are hardware
> > > > specific. This
> > > > +	    test patterns are used to test a device is properly working and
> > > > can generate
> > > > +	    the desired waveforms that
> > > > it supports.</entry>
> > > > +	  </row>
> > > > +	  <row>
> > > > +	    <entrytbl spanname="descr" cols="2">
> > > > +	      <tbody valign="top">
> > > > +	        <row>
> > > > +	        
> > > > <entry><constant>V4L2_TEST_PATTERN_DISABLED</constant></entry>
> > > > +	          <entry>Test pattern generation is disabled</entry>
> > > > +	        </row>
> > > > +	      </tbody>
> > > > +	    </entrytbl>
> > > > +	  </row>
> > > >   	<row><entry></entry></row>
> > > >   	</tbody>
> > > >         </tgroup>
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > b/drivers/media/v4l2-core/v4l2-ctrls.c index 8f2f40b..d731422 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > @@ -430,6 +430,10 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > > >   		"Advanced",
> > > >   		NULL,
> > > >   	};
> > > > +	static const char * const test_pattern[] = {
> > > > +		"Disabled",
> > > > +		NULL,
> > > > +	};
> > > > 
> > > >   	switch (id) {
> > > >   	case V4L2_CID_MPEG_AUDIO_SAMPLING_FREQ:
> > > > @@ -509,6 +513,8 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
> > > >   		return jpeg_chroma_subsampling;
> > > >   	case V4L2_CID_DPCM_PREDICTOR:
> > > >   		return dpcm_predictor;
> > > > +	case V4L2_CID_TEST_PATTERN:
> > > > +		return test_pattern;
> > > 
> > > I think it's not necessary to define test_pattern (nor be prepared to
> > > return it) since the menu is going to be device specific. So the driver
> > > is responsible for all of the menu items. Such menus are created using
> > > v4l2_ctrl_new_custom() instead of v4l2_ctrl_new_std_menu().
> > > 
> > > Looks good to me otherwise.
> > 
> > I would suggest that we *do* make this a standard control, but the menu
> > consists of just one item: "Disabled". After creating the control you can
> > just set the ctrl->qmenu pointer to the device-specific menu. I like
> > using standard controls because they guarantee standard naming and type
> > conventions. They are also easier to use in an application.
> 
> That's not quite enough. Also menu_skip_mask and max also need to be
> replaced. In a general case min as well. It's easy to do mistakes in that
> --- how about a separate function for doing that? It'd be also nice to be
> able to use the existing standardised menu item strings, but that's just an
> extra plus.

Agreed. A way to create a standard menu control with driver-supplied menu 
items would be a good addition to the control framework API.

> However I think it's beyond this patch, which I think then is be fine w/o
> modifications. So on my behalf,
> 
> Acked-by: Sakari Ailus <sakari.ailus@iki.fi>

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-09-13 10:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-07  8:41 [PATCH v2] media: v4l2-ctrls: add control for test pattern Prabhakar Lad
2012-09-07 18:20 ` Sakari Ailus
2012-09-07 19:09   ` Prabhakar Lad
2012-09-08 11:11   ` Hans Verkuil
2012-09-09  7:40     ` Sakari Ailus
2012-09-13  1:04       ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).