Re: [PATCH v2] media: v4l2-ctrls: add control for test pattern

[Prabhakar Lad]

Hi Sakari,

Thanks for the review.

On Fri, Sep 7, 2012 at 11:50 PM, Sakari Ailus <sakari.ailus@xxxxxx> wrote:
> Hi Prabhakar,
>
> Thanks for the patch!
>
>
> Prabhakar Lad wrote:
>>
>> From: Lad, Prabhakar <prabhakar.lad@xxxxxx>
>>
>> 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@xxxxxx>
>> Signed-off-by: Manjunath Hadli <manjunath.hadli@xxxxxx>
>> Cc: Sakari Ailus <sakari.ailus@xxxxxx>
>> Cc: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxx>
>> Cc: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
>> Cc: Hans de Goede <hdegoede@xxxxxxxxxx>
>> Cc: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>> Cc: Rob Landley <rob@xxxxxxxxxxx>
>> ---
>> 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@xxxxxx
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/