Message ID | 1368710287-8741-1-git-send-email-prabhakar.csengg@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi Laurent/Sylwester, On Thu, May 16, 2013 at 6:48 PM, Lad Prabhakar <prabhakar.csengg@gmail.com> wrote: > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > This patch adds "sync-on-green" property as part of > endpoint properties and also support to parse them in the parser. > If you are ok with the patch can I send non RFC version of it ? Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Prabhakar, On 05/16/2013 03:18 PM, Lad Prabhakar wrote: > From: Lad, Prabhakar <prabhakar.csengg@gmail.com> > > This patch adds "sync-on-green" property as part of > endpoint properties and also support to parse them in the parser. > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt > @@ -101,6 +101,8 @@ Optional endpoint properties > array contains only one entry. > - clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous > clock mode. > +-sync-on-green: a boolean property indicating to sync with the green signal in > + RGB. Are you sure this is what you need for the TVP7002 chip ? I think we should differentiate between analog and digital signals and the related device's configuration. AFAIU for the analog part there can be various video sychronisation methods, i.e. ways in which the synchronisation signals are transmitted along side the video component (RGB or luma/chroma) signals. According to [1] (presumably not most reliable source of information) there are following methods of transmitting sync signals: - Separate sync - Composite sync - Sync-on-green (SOG) - Sync-on-luminance - Sync-on-composite And all these seem to refer to analog video signal. From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's datasheet ([2], p. 52) and your initial TVP7002 patches it looks like what you want is to specify polarity of the SOGOUT signal, so the processor that receives this signal can properly interpret it, is it correct ? If so then wouldn't it be more appropriate to define e.g. 'sog-active' property and media bus flags: V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH ? And for synchronisation method on the analog part we could perhaps define 'component-sync' or similar property that would enumerate all possible synchronisation methods. We might as well use separate boolean properties, but I'm a bit concerned about the increasing number of properties that need to be parsed for each parallel video bus "endpoint". Anyway I'm not sure if we would already have use cases for the 'component-sync' property. [1] http://en.wikipedia.org/wiki/Component_video_sync [2] http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=tvp7002&fileType=pdf > Example > diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c > index aa59639..b51d61f 100644 > --- a/drivers/media/v4l2-core/v4l2-of.c > +++ b/drivers/media/v4l2-core/v4l2-of.c > @@ -100,6 +100,9 @@ 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_get_property(node, "sync-on-green",&v)) > + flags |= V4L2_MBUS_SYNC_ON_GREEN; > + > bus->flags = flags; > > } > diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h > index 83ae07e..cf2c66f9 100644 > --- a/include/media/v4l2-mediabus.h > +++ b/include/media/v4l2-mediabus.h > @@ -40,6 +40,7 @@ > #define V4L2_MBUS_FIELD_EVEN_HIGH (1<< 10) > /* FIELD = 1/0 - Field1 (odd)/Field2 (even) */ > #define V4L2_MBUS_FIELD_EVEN_LOW (1<< 11) > +#define V4L2_MBUS_SYNC_ON_GREEN (1<< 12) -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On Fri, May 24, 2013 at 4:41 PM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > Prabhakar, > > > On 05/16/2013 03:18 PM, Lad Prabhakar wrote: >> >> From: Lad, Prabhakar <prabhakar.csengg@gmail.com> >> >> This patch adds "sync-on-green" property as part of >> endpoint properties and also support to parse them in the parser. > > >> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt >> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt >> @@ -101,6 +101,8 @@ Optional endpoint properties >> array contains only one entry. >> - clock-noncontinuous: a boolean property to allow MIPI CSI-2 >> non-continuous >> clock mode. >> +-sync-on-green: a boolean property indicating to sync with the green >> signal in >> + RGB. > > > Are you sure this is what you need for the TVP7002 chip ? > Yes > I think we should differentiate between analog and digital signals and the > related > device's configuration. AFAIU for the analog part there can be various video > sychronisation methods, i.e. ways in which the synchronisation signals are > transmitted along side the video component (RGB or luma/chroma) signals. > According > to [1] (presumably not most reliable source of information) there are > following > methods of transmitting sync signals: > > - Separate sync > - Composite sync > - Sync-on-green (SOG) > - Sync-on-luminance > - Sync-on-composite > > And all these seem to refer to analog video signal. > I was about to add all these but as per Laurent mentioned we can add this whenever there is a need of it. > From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's > datasheet > ([2], p. 52) and your initial TVP7002 patches it looks like what you want is > to > specify polarity of the SOGOUT signal, so the processor that receives this > signal > can properly interpret it, is it correct ? > Yes > If so then wouldn't it be more appropriate to define e.g. 'sog-active' > property > and media bus flags: > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH > ? > Agreed I'll add these flags. > And for synchronisation method on the analog part we could perhaps define > 'component-sync' or similar property that would enumerate all possible > synchronisation methods. We might as well use separate boolean properties, > but I'm a bit concerned about the increasing number of properties that need > to be parsed for each parallel video bus "endpoint". > I am not clear on it can please elaborate more on this. Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 05/25/2013 11:17 AM, Prabhakar Lad wrote: >> From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's >> > datasheet >> > ([2], p. 52) and your initial TVP7002 patches it looks like what you want is >> > to >> > specify polarity of the SOGOUT signal, so the processor that receives this >> > signal >> > can properly interpret it, is it correct ? >> > > Yes >> > If so then wouldn't it be more appropriate to define e.g. 'sog-active' >> > property >> > and media bus flags: >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH >> > ? >> > > Agreed I'll add these flags. > >> > And for synchronisation method on the analog part we could perhaps define >> > 'component-sync' or similar property that would enumerate all possible >> > synchronisation methods. We might as well use separate boolean properties, >> > but I'm a bit concerned about the increasing number of properties that need >> > to be parsed for each parallel video bus "endpoint". >> > > I am not clear on it can please elaborate more on this. I thought about two possible options: 1. single property 'component-sync' or 'video-sync' that would have values: #define VIDEO_SEPARATE_SYNC 0x01 #define VIDEO_COMPOSITE_SYNC 0x02 #define VIDEO_SYNC_ON_COMPOSITE 0x04 #define VIDEO_SYNC_ON_GREEN 0x08 #define VIDEO_SYNC_ON_LUMINANCE 0x10 And we could put these definitions into a separate header, e.g. <dt-bindings/video-interfaces.h> Then in a device tree source file one could have, e.g. video-sync = <VIDEO_SYNC_ON_GREEN>; 2. Separate boolean property for each video sync type, e.g. "video-composite-sync" "video-sync-on-composite" "video-sync-on-green" "video-sync-on-luminance" Separate sync, with separate VSYNC, HSYNC lines, would be the default, when none of the above is specified and 'vsync-active', 'hsync-active' properties are present. However, I suppose the better would be to deduce the video synchronisation method from the sync signal polarity flags. Then, for instance, when an endpoint node contains "composite-sync-active" property the parser would determine the "composite sync" synchronisation type is used. Thus it might make sense to have only following integer properties (added as needed): composite-sync-active sync-on-green-active sync-on-comp-active sync-on-luma-active This would allow to specify polarity of each signal and at the same time the parsing code could derive synchronisation type. A new field could be added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled within v4l2_of_parse_endpoint(). What do you think ? Thanks, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On Sat, May 25, 2013 at 7:41 PM, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > Hi, > > > On 05/25/2013 11:17 AM, Prabhakar Lad wrote: >>> >>> From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's >>> > datasheet >>> > ([2], p. 52) and your initial TVP7002 patches it looks like what you >>> > want is >>> > to >>> > specify polarity of the SOGOUT signal, so the processor that receives >>> > this >>> > signal >>> > can properly interpret it, is it correct ? >>> > >> >> Yes >>> >>> > If so then wouldn't it be more appropriate to define e.g. 'sog-active' >>> > property >>> > and media bus flags: >>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW >>> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH >>> > ? >>> > >> >> Agreed I'll add these flags. >> >>> > And for synchronisation method on the analog part we could perhaps >>> > define >>> > 'component-sync' or similar property that would enumerate all possible >>> > synchronisation methods. We might as well use separate boolean >>> > properties, >>> > but I'm a bit concerned about the increasing number of properties that >>> > need >>> > to be parsed for each parallel video bus "endpoint". >>> > >> >> I am not clear on it can please elaborate more on this. > > > I thought about two possible options: > > 1. single property 'component-sync' or 'video-sync' that would have values: > > #define VIDEO_SEPARATE_SYNC 0x01 > #define VIDEO_COMPOSITE_SYNC 0x02 > #define VIDEO_SYNC_ON_COMPOSITE 0x04 > #define VIDEO_SYNC_ON_GREEN 0x08 > #define VIDEO_SYNC_ON_LUMINANCE 0x10 > > And we could put these definitions into a separate header, e.g. > <dt-bindings/video-interfaces.h> > > Then in a device tree source file one could have, e.g. > > video-sync = <VIDEO_SYNC_ON_GREEN>; > > > 2. Separate boolean property for each video sync type, e.g. > > "video-composite-sync" > "video-sync-on-composite" > "video-sync-on-green" > "video-sync-on-luminance" > > Separate sync, with separate VSYNC, HSYNC lines, would be the default, when > none of the above is specified and 'vsync-active', 'hsync-active' properties > are present. > > However, I suppose the better would be to deduce the video synchronisation > method from the sync signal polarity flags. Then, for instance, when an > endpoint node contains "composite-sync-active" property the parser would > determine the "composite sync" synchronisation type is used. > > Thus it might make sense to have only following integer properties (added > as needed): > > composite-sync-active > sync-on-green-active > sync-on-comp-active > sync-on-luma-active > > This would allow to specify polarity of each signal and at the same time > the parsing code could derive synchronisation type. A new field could be > added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled > within v4l2_of_parse_endpoint(). > I am OK with this option. and I hope you meant "struct v4l2_of_bus_parallel" instead of " struct v4l2_of_parallel_bus" and to fill sync_type within v4l2_of_parse_parallel_bus() and not in v4l2_of_parse_endpoint(). Regards, --Prabhakar Lad -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/25/2013 04:26 PM, Prabhakar Lad wrote: >> Thus it might make sense to have only following integer properties (added >> > as needed): >> > >> > composite-sync-active >> > sync-on-green-active >> > sync-on-comp-active >> > sync-on-luma-active >> > >> > This would allow to specify polarity of each signal and at the same time >> > the parsing code could derive synchronisation type. A new field could be >> > added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled >> > within v4l2_of_parse_endpoint(). >> > > I am OK with this option. and I hope you meant "struct > v4l2_of_bus_parallel" instead > of " struct v4l2_of_parallel_bus" and to fill sync_type within > v4l2_of_parse_parallel_bus() > and not in v4l2_of_parse_endpoint(). Yes, that's what I meant, sorry for this confusion. Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sylwester, On Saturday 25 May 2013 16:11:52 Sylwester Nawrocki wrote: > On 05/25/2013 11:17 AM, Prabhakar Lad wrote: > >> > From looking at Figure 8 "TVP7002 Application Example" in the TVP7002's > >> > datasheet ([2], p. 52) and your initial TVP7002 patches it looks like > >> > what you want is to specify polarity of the SOGOUT signal, so the > >> > processor that receives this signal can properly interpret it, is it > >> > correct ? > > > > Yes > > > >> > If so then wouldn't it be more appropriate to define e.g. 'sog-active' > >> > property and media bus flags: > >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_LOW > >> > V4L2_MBUS_SYNC_ON_GREEN_ACTIVE_HIGH > >> > > >> > ? > > > > Agreed I'll add these flags. > > > >> > And for synchronisation method on the analog part we could perhaps > >> > define 'component-sync' or similar property that would enumerate all > >> > possible synchronisation methods. We might as well use separate > >> > boolean properties, but I'm a bit concerned about the increasing > >> > number of properties that need to be parsed for each parallel video > >> > bus "endpoint". > > > > I am not clear on it can please elaborate more on this. > > I thought about two possible options: > > 1. single property 'component-sync' or 'video-sync' that would have values: > > #define VIDEO_SEPARATE_SYNC 0x01 > #define VIDEO_COMPOSITE_SYNC 0x02 > #define VIDEO_SYNC_ON_COMPOSITE 0x04 > #define VIDEO_SYNC_ON_GREEN 0x08 > #define VIDEO_SYNC_ON_LUMINANCE 0x10 > > And we could put these definitions into a separate header, e.g. > <dt-bindings/video-interfaces.h> > > Then in a device tree source file one could have, e.g. > > video-sync = <VIDEO_SYNC_ON_GREEN>; > > > 2. Separate boolean property for each video sync type, e.g. > > "video-composite-sync" > "video-sync-on-composite" > "video-sync-on-green" > "video-sync-on-luminance" > > Separate sync, with separate VSYNC, HSYNC lines, would be the default, when > none of the above is specified and 'vsync-active', 'hsync-active' properties > are present. I prefer 1. over 2. > However, I suppose the better would be to deduce the video synchronisation > method from the sync signal polarity flags. Then, for instance, when an > endpoint node contains "composite-sync-active" property the parser would > determine the "composite sync" synchronisation type is used. > > Thus it might make sense to have only following integer properties (added > as needed): > > composite-sync-active > sync-on-green-active > sync-on-comp-active > sync-on-luma-active > > This would allow to specify polarity of each signal and at the same time > the parsing code could derive synchronisation type. A new field could be > added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled > within v4l2_of_parse_endpoint(). > > What do you think ? My gut feeling is that we should have separate properties for the video sync type and the synchronization signals polarities. We could have a chip that supports sync-on-green on the analog (input) side and outputs separate hsync and vsync signals only on the digital (output) side. There would be no sync- on-green polarity in that case.
On 05/30/2013 05:21 AM, Laurent Pinchart wrote: > Hi Sylwester, > > On Saturday 25 May 2013 16:11:52 Sylwester Nawrocki wrote: >> On 05/25/2013 11:17 AM, Prabhakar Lad wrote: [...] >>>>> And for synchronisation method on the analog part we could perhaps >>>>> define 'component-sync' or similar property that would enumerate all >>>>> possible synchronisation methods. We might as well use separate >>>>> boolean properties, but I'm a bit concerned about the increasing >>>>> number of properties that need to be parsed for each parallel video >>>>> bus "endpoint". >>> >>> I am not clear on it can please elaborate more on this. >> >> I thought about two possible options: >> >> 1. single property 'component-sync' or 'video-sync' that would have values: >> >> #define VIDEO_SEPARATE_SYNC 0x01 >> #define VIDEO_COMPOSITE_SYNC 0x02 >> #define VIDEO_SYNC_ON_COMPOSITE 0x04 >> #define VIDEO_SYNC_ON_GREEN 0x08 >> #define VIDEO_SYNC_ON_LUMINANCE 0x10 >> >> And we could put these definitions into a separate header, e.g. >> <dt-bindings/video-interfaces.h> >> >> Then in a device tree source file one could have, e.g. >> >> video-sync = <VIDEO_SYNC_ON_GREEN>; >> >> >> 2. Separate boolean property for each video sync type, e.g. >> >> "video-composite-sync" >> "video-sync-on-composite" >> "video-sync-on-green" >> "video-sync-on-luminance" >> >> Separate sync, with separate VSYNC, HSYNC lines, would be the default, when >> none of the above is specified and 'vsync-active', 'hsync-active' properties >> are present. > > I prefer 1. over 2. > >> However, I suppose the better would be to deduce the video synchronisation >> method from the sync signal polarity flags. Then, for instance, when an >> endpoint node contains "composite-sync-active" property the parser would >> determine the "composite sync" synchronisation type is used. >> >> Thus it might make sense to have only following integer properties (added >> as needed): >> >> composite-sync-active >> sync-on-green-active >> sync-on-comp-active >> sync-on-luma-active >> >> This would allow to specify polarity of each signal and at the same time >> the parsing code could derive synchronisation type. A new field could be >> added to struct v4l2_of_parallel_bus, e.g. sync_type and it would be filled >> within v4l2_of_parse_endpoint(). >> >> What do you think ? > > My gut feeling is that we should have separate properties for the video sync > type and the synchronization signals polarities. We could have a chip that > supports sync-on-green on the analog (input) side and outputs separate hsync > and vsync signals only on the digital (output) side. There would be no sync- > on-green polarity in that case. Yes, agreed. I've had some doubts that using single DT property for defining really 2 distinct H/W properties like this might not be flexible enough. The option 1. seems most correct then. Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt index e022d2d..f91821f 100644 --- a/Documentation/devicetree/bindings/media/video-interfaces.txt +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt @@ -101,6 +101,8 @@ Optional endpoint properties array contains only one entry. - clock-noncontinuous: a boolean property to allow MIPI CSI-2 non-continuous clock mode. +-sync-on-green: a boolean property indicating to sync with the green signal in + RGB. Example diff --git a/drivers/media/v4l2-core/v4l2-of.c b/drivers/media/v4l2-core/v4l2-of.c index aa59639..b51d61f 100644 --- a/drivers/media/v4l2-core/v4l2-of.c +++ b/drivers/media/v4l2-core/v4l2-of.c @@ -100,6 +100,9 @@ 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_get_property(node, "sync-on-green", &v)) + flags |= V4L2_MBUS_SYNC_ON_GREEN; + bus->flags = flags; } diff --git a/include/media/v4l2-mediabus.h b/include/media/v4l2-mediabus.h index 83ae07e..cf2c66f9 100644 --- a/include/media/v4l2-mediabus.h +++ b/include/media/v4l2-mediabus.h @@ -40,6 +40,7 @@ #define V4L2_MBUS_FIELD_EVEN_HIGH (1 << 10) /* FIELD = 1/0 - Field1 (odd)/Field2 (even) */ #define V4L2_MBUS_FIELD_EVEN_LOW (1 << 11) +#define V4L2_MBUS_SYNC_ON_GREEN (1 << 12) /* Serial flags */ /* How many lanes the client can use */