Re: [PATCH RFC v3] media: OF: add video sync endpoint property

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Sylwester,

Oops some how missed this mail, sorry for the late response.

On Sun, Jun 30, 2013 at 9:23 PM, Sylwester Nawrocki
<sylvester.nawrocki@xxxxxxxxx> wrote:
> Hi,
>
>
> On 06/22/2013 05:03 PM, Prabhakar Lad wrote:
>>
>> From: "Lad, Prabhakar"<prabhakar.csengg@xxxxxxxxx>
>>
>> This patch adds video sync properties as part of endpoint
>> properties and also support to parse them in the parser.
>>
>> Signed-off-by: Lad, Prabhakar<prabhakar.csengg@xxxxxxxxx>
>> Cc: Hans Verkuil<hans.verkuil@xxxxxxxxx>
>> Cc: Laurent Pinchart<laurent.pinchart@xxxxxxxxxxxxxxxx>
>> Cc: Mauro Carvalho Chehab<mchehab@xxxxxxxxxx>
>> Cc: Guennadi Liakhovetski<g.liakhovetski@xxxxxx>
>> Cc: Sylwester Nawrocki<s.nawrocki@xxxxxxxxxxx>
>> Cc: Sakari Ailus<sakari.ailus@xxxxxx>
>> Cc: Grant Likely<grant.likely@xxxxxxxxxxxx>
>> Cc: Rob Herring<rob.herring@xxxxxxxxxxx>
>> Cc: Rob Landley<rob@xxxxxxxxxxx>
>> Cc: devicetree-discuss@xxxxxxxxxxxxxxxx
>> Cc: linux-doc@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: davinci-linux-open-source@xxxxxxxxxxxxxxxxxxxx
>> Cc: Kyungmin Park<kyungmin.park@xxxxxxxxxxx>
>
>
> Do you really need such a long Cc list here ? I think it would be better
> to just add relevant e-mail addresses when sending the patch, otherwise
> when this patch is applied in this form all those addresses are going to
> be spammed with the patch management notifications, which might not be
> what some ones are really interested in.
>
>
Ok I'll take care of it in the next version.

>> ---
>>   This patch has 10 warnings for line over 80 characters
>>   for which I think can be ignored.
>>
>>   RFC v2 https://patchwork.kernel.org/patch/2578091/
>>   RFC V1 https://patchwork.kernel.org/patch/2572341/
>>   Changes for v3:
>>   1: Fixed review comments pointed by Laurent and Sylwester.
>>
>>   .../devicetree/bindings/media/video-interfaces.txt |    1 +
>>   drivers/media/v4l2-core/v4l2-of.c                  |   20
>> ++++++++++++++++++
>>   include/dt-bindings/media/video-interfaces.h       |   17
>> +++++++++++++++
>>   include/media/v4l2-mediabus.h                      |   22
>> +++++++++++---------
>>   4 files changed, 50 insertions(+), 10 deletions(-)
>>   create mode 100644 include/dt-bindings/media/video-interfaces.h
>>
>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> index e022d2d..2081278 100644
>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
>> @@ -101,6 +101,7 @@ Optional endpoint properties
>>     array contains only one entry.
>>   - clock-noncontinuous: a boolean property to allow MIPI CSI-2
>> non-continuous
>>     clock mode.
>> +- video-sync: property indicating to sync the video on a signal in RGB.
>>
>>
>>   Example
>> diff --git a/drivers/media/v4l2-core/v4l2-of.c
>> b/drivers/media/v4l2-core/v4l2-of.c
>> index aa59639..1a54530 100644
>> --- a/drivers/media/v4l2-core/v4l2-of.c
>> +++ b/drivers/media/v4l2-core/v4l2-of.c
>> @@ -100,6 +100,26 @@ static void v4l2_of_parse_parallel_bus(const struct
>> device_node *node,
>>         if (!of_property_read_u32(node, "data-shift",&v))
>>                 bus->data_shift = v;
>>
>> +       if (!of_property_read_u32(node, "video-sync",&v)) {
>> +               switch (v) {
>> +               case V4L2_MBUS_VIDEO_SEPARATE_SYNC:
>> +                       flags |= V4L2_MBUS_VIDEO_SEPARATE_SYNC;
>
>
> I'm not convinced all those video sync types is something that really
> belongs
> to the flags field. In my understanding this field is supposed to hold only
> the _signal polarity_ information.
>
>
Ok, so there should be a function say v4l2_of_parse_signal_polarity()
to get the polarity alone then.

>> +                       break;
>> +               case V4L2_MBUS_VIDEO_COMPOSITE_SYNC:
>> +                       flags |= V4L2_MBUS_VIDEO_COMPOSITE_SYNC;
>> +                       break;
>> +               case V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE:
>> +                       flags |= V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE;
>> +                       break;
>> +               case V4L2_MBUS_VIDEO_SYNC_ON_GREEN:
>> +                       flags |= V4L2_MBUS_VIDEO_SYNC_ON_GREEN;
>> +                       break;
>> +               case V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE:
>> +                       flags |= V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE;
>> +                       break;
>> +               }
>> +       }
>> +
>>         bus->flags = flags;
>>
>>   }
>> diff --git a/include/dt-bindings/media/video-interfaces.h
>> b/include/dt-bindings/media/video-interfaces.h
>> new file mode 100644
>> index 0000000..1a083dd
>> --- /dev/null
>> +++ b/include/dt-bindings/media/video-interfaces.h
>> @@ -0,0 +1,17 @@
>> +/*
>> + * This header provides constants for video interface.
>> + *
>> + */
>> +
>> +#ifndef _DT_BINDINGS_VIDEO_INTERFACES_H
>> +#define _DT_BINDINGS_VIDEO_INTERFACES_H
>> +
>> +#define V4L2_MBUS_VIDEO_SEPARATE_SYNC          (1<<  2)
>
>
> You should never be putting anything Linux specific in those dt-bindings
> headers. It just happens now include/dt-bindings is a part of the Linux
> kernel, but it could well be in some separate repository, or could be
> a part of the DT bindings specification, which is only specific to the
> hardware and doesn't depend on Linux OS at all. Thus V4L2_MBUS_ prefix
> shouldn't be used here.
>
>
Ok

>> +#define V4L2_MBUS_VIDEO_COMPOSITE_SYNC         (1<<  3)
>> +#define V4L2_MBUS_VIDEO_SYNC_ON_COMPOSITE      (1<<  4)
>> +#define V4L2_MBUS_VIDEO_SYNC_ON_GREEN          (1<<  5)
>> +#define V4L2_MBUS_VIDEO_SYNC_ON_LUMINANCE      (1<<  6)
>> +
>> +#define V4L2_MBUS_VIDEO_INTERFACES_END         6
>> +
>> +#endif
>> diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h
>> index 83ae07e..a4676dd 100644
>> --- a/include/media/v4l2-mediabus.h
>> +++ b/include/media/v4l2-mediabus.h
>> @@ -11,6 +11,8 @@
>>   #ifndef V4L2_MEDIABUS_H
>>   #define V4L2_MEDIABUS_H
>>
>> +#include<dt-bindings/media/video-interfaces.h>
>> +
>>   #include<linux/v4l2-mediabus.h>
>>
>>   /* Parallel flags */
>> @@ -28,18 +30,18 @@
>>    * V4L2_MBUS_[HV]SYNC* flags should be also used for specifying
>>    * configuration of hardware that uses [HV]REF signals
>>    */
>> -#define V4L2_MBUS_HSYNC_ACTIVE_HIGH            (1<<  2)
>> -#define V4L2_MBUS_HSYNC_ACTIVE_LOW             (1<<  3)
>> -#define V4L2_MBUS_VSYNC_ACTIVE_HIGH            (1<<  4)
>> -#define V4L2_MBUS_VSYNC_ACTIVE_LOW             (1<<  5)
>> -#define V4L2_MBUS_PCLK_SAMPLE_RISING           (1<<  6)
>> -#define V4L2_MBUS_PCLK_SAMPLE_FALLING          (1<<  7)
>> -#define V4L2_MBUS_DATA_ACTIVE_HIGH             (1<<  8)
>> -#define V4L2_MBUS_DATA_ACTIVE_LOW              (1<<  9)
>> +#define V4L2_MBUS_HSYNC_ACTIVE_HIGH            (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 1))
>
>
> No, please don't do that. We shouldn't combine the DT and Linux kernel
> definitions like this.
>
>
Ok.

>> +#define V4L2_MBUS_HSYNC_ACTIVE_LOW             (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 2))
>> +#define V4L2_MBUS_VSYNC_ACTIVE_HIGH            (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 3))
>> +#define V4L2_MBUS_VSYNC_ACTIVE_LOW             (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 4))
>> +#define V4L2_MBUS_PCLK_SAMPLE_RISING           (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 5))
>> +#define V4L2_MBUS_PCLK_SAMPLE_FALLING          (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 6))
>> +#define V4L2_MBUS_DATA_ACTIVE_HIGH             (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 7))
>> +#define V4L2_MBUS_DATA_ACTIVE_LOW              (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 8))
>>
>>   /* FIELD = 0/1 - Field1 (odd)/Field2 (even) */
>> -#define V4L2_MBUS_FIELD_EVEN_HIGH              (1<<  10)
>> +#define V4L2_MBUS_FIELD_EVEN_HIGH              (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 9))
>>   /* FIELD = 1/0 - Field1 (odd)/Field2 (even) */
>> -#define V4L2_MBUS_FIELD_EVEN_LOW               (1<<  11)
>> +#define V4L2_MBUS_FIELD_EVEN_LOW               (1<<
>> (V4L2_MBUS_VIDEO_INTERFACES_END + 10))
>
>
> Please see my review of your 'media: i2c: tvp7002: add OF support" patch.
> AFAICS it should be sufficient to add only V4L2_MBUS_SOG_ACTIVE_{LOW,HIGH}
> flags and 'sync-on-green-active' DT property.
>
Ok.

Regards,
--Prabhakar Lad
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux