Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Sat, Oct 20, 2012 at 09:59:51PM +0200, Thierry Reding wrote:
> On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote:
> [...]
> > diff --git a/include/linux/of_display_timings.h
> > b/include/linux/of_display_timings.h
> [...]
> > +struct display_timings {
> > + unsigned int num_timings;
> > + unsigned int default_timing;
> > +
> > + struct signal_timing **timings;
> > +};
> > +
> > +struct timing_entry {
> > + u32 min;
> > + u32 typ;
> > + u32 max;
> > +};
> > +
> > +struct signal_timing {
>
> I'm slightly confused by the naming here. signal_timing seems overly
> generic in this context. Is there any reason why this isn't called
> display_timing or even display_mode?
>
You are right. I actually already changed that, for the same reasons.
It will be called display_timing in the next version, as I think that's what it
really
is.
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote:
[...]
> diff --git a/include/linux/of_display_timings.h
> b/include/linux/of_display_timings.h
[...]
> +struct display_timings {
> + unsigned int num_timings;
> + unsigned int default_timing;
> +
> + struct signal_timing **timings;
> +};
> +
> +struct timing_entry {
> + u32 min;
> + u32 typ;
> + u32 max;
> +};
> +
> +struct signal_timing {
I'm slightly confused by the naming here. signal_timing seems overly
generic in this context. Is there any reason why this isn't called
display_timing or even display_mode?
pgp4IEEZTAHtO.pgp
Description: PGP signature
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 04, 2012 at 07:59:19PM +0200, Steffen Trumtrar wrote: [...] > diff --git a/include/linux/of_display_timings.h > b/include/linux/of_display_timings.h > new file mode 100644 > index 000..1ad719e > --- /dev/null > +++ b/include/linux/of_display_timings.h > @@ -0,0 +1,85 @@ > +/* > + * Copyright 2012 Steffen Trumtrar > + * > + * description of display timings > + * > + * This file is released under the GPLv2 > + */ > + > +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H > +#define __LINUX_OF_DISPLAY_TIMINGS_H This file needs to include linux/slab.h because it uses kfree() in the inline functions. Alternatively I think I'd rather see the inline functions moved out of the header, with the exception of the signal_timing_get_value() function perhaps. Moreover there should be a forward declaration of struct display_node to avoid the need to include linux/of.h. Thierry pgpgdeqMpeoZ4.pgp Description: PGP signature
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 11, 2012 at 09:31:18PM +0200, Thierry Reding wrote:
> On Mon, Oct 08, 2012 at 09:49:21AM +0200, Steffen Trumtrar wrote:
> > On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> > > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> [...]
> > > > +
> > > > + disp->num_timings = 0;
> > > > +
> > > > + for_each_child_of_node(timings_np, entry) {
> > > > + disp->num_timings++;
> > > > + }
> > >
> > > No need for { }
> > >
> >
> > Okay.
>
> Or you could just use of_get_child_count().
>
> Thierry
Ah, very nice. That's definitely better. Didn't know about that function.
Thanks,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Mon, Oct 08, 2012 at 09:49:21AM +0200, Steffen Trumtrar wrote:
> On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> > On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
[...]
> > > +
> > > + disp->num_timings = 0;
> > > +
> > > + for_each_child_of_node(timings_np, entry) {
> > > + disp->num_timings++;
> > > + }
> >
> > No need for { }
> >
>
> Okay.
Or you could just use of_get_child_count().
Thierry
pgpdhdUsgoKig.pgp
Description: PGP signature
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi Stephen, On Monday 08 October 2012 10:10:31 Stephen Warren wrote: > On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote: > > On Fri, 5 Oct 2012, Stephen Warren wrote: > >> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: > >>> Hi Steffen > >>> > >>> Sorry for chiming in so late in the game, but I've long been wanting to > >>> have a look at this and compare with what we do for V4L2, so, this seems > >>> a great opportunity to me:-) > >>> > >>> On Thu, 4 Oct 2012, Steffen Trumtrar wrote: > diff --git > a/Documentation/devicetree/bindings/video/display-timings.txt > b/Documentation/devicetree/bindings/video/display-timings.txt > > +timings-subnode > +--- > + > +required properties: > + - hactive, vactive: Display resolution > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing > parameters + in pixels > + vfront-porch, vback-porch, vsync-len: Vertical display timing > parameters in + lines > + - clock: displayclock in Hz > >>> > >>> You're going to hate me for this, but eventually we want to actually > >>> reference clock objects in our DT bindings. For now, even if you don't > >>> want to actually add clock phandles and stuff here, I think, using the > >>> standard "clock-frequency" property would be much better! > >> > >> In a definition of a display timing, we will never need to use the clock > >> binding; the clock binding would be used by the HW module that is > >> generating a timing, not by the timing definition itself. > > > > You mean clock consumer bindings will be in the display device DT node? > > And the display-timings node will be its child? > > Yes > > ... > > + - interlaced (bool) > >>> > >>> Is "interlaced" a property of the hardware, i.e. of the board? Can the > >>> same display controller on one board require interlaced data and on > >>> another board - progressive? > >> > >> Interlace is a property of a display mode. It's quite possible for a > >> particular display controller to switch between interlace and > >> progressive output at run-time. For example, reconfiguring the output > >> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to > >> a built-in LCD display, you're probably always going to be driving the > >> single mode required by the panel, and that mode will likely always be > >> progressive. However, since this binding attempts to describe any > >> display timing, I think we still need this property per mode. > > > > But why do you need this in the DT then at all? > > Because the driver for the display controller has no idea what display > or panel will be connected to it. That's right, but with the current common panel framework (WIP, I plan to resume working on it this week) the display controller will be able to query the panel for bus configuration parameters (or the other way around, the panel driver will set the display controller bus configuration). The panel driver will still need to receive timing information from DT, but it will be possible to communicate display bus configuration parameters between the panel driver and the display controller driver. This being said, I agree that interlaced is a property of the display mode, not of the display bus (even though the display bus will then be configured for interlaced data). > > If it's fixed, as required per display controller, then its driver will > > know it. If it's runtime configurable, then it's a purely software > > parameter and doesn't depend on the board? > > interlace-vs-progressive isn't "fixed, as required per display > controller", but is a property of the mode being sent by the display > controller, and the requirements for that mode are driven by the > panel/display connected to the display controller, not the display > controller, in general. > > ... > > >>> BTW, I'm not very familiar with display interfaces, but for interlaced > >>> you probably sometimes use a field signal, whose polarity you also want > >>> to specify here? We use a "field-even-active" integer property for it. > >> > >> I think that's a property of the display controller itself, rather than > >> an individual mode, although I'm not 100% certain. My assertion is that > >> the physical interface that the display controller is driving will > >> determine whether embedded or separate sync is used, and in the separate > >> sync case, how the field signal is defined, and that all interlace modes > >> driven over that interface will use the same field signal definition. > > > > In general, I might be misunderstanding something, but don't we have to > > distinguish between 2 types of information about display timings: (1) is > > defined by the display controller requirements, is known to the display > > driver and doesn't need to be present in timings DT. We did have some of > > these parameters in board data previously, because we didn't have proper > > display controller dri
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/7/2012 11:01 PM, Tomi Valkeinen wrote: > On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote: > >> In general, I might be misunderstanding something, but don't we have to >> distinguish between 2 types of information about display timings: (1) is >> defined by the display controller requirements, is known to the display >> driver and doesn't need to be present in timings DT. We did have some of >> these parameters in board data previously, because we didn't have proper >> display controller drivers... (2) is board specific configuration, and is >> such it has to be present in DT. >> >> In that way, doesn't "interlaced" belong to type (1) and thus doesn't need >> to be present in DT? > > As I see it, this DT data is about the display (most commonly LCD > panel), i.e. what video mode(s) the panel supports. If things were done > my way, the panel's supported timings would be defined in the driver for > the panel, and DT would be left to describe board specific data, but > this approach has its benefits. > > Thus, if you connect an interlaced panel to your board, Do interlaced panels exist? I have never seen one. you need to tell > the display controller that this panel requires interlace signal. Also, > pixel clock source doesn't make sense in this context, as this doesn't > describe the actual used configuration, but only what the panel > supports. > > Of course, if this is about describing the hardware, the default-mode > property doesn't really fit in... > > Tomi > > > > ___ > devicetree-discuss mailing list > devicetree-disc...@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 06:20 AM, Tomi Valkeinen wrote: > On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote: >> On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote: >>> On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski >>> wrote: ... >>> Of course, if this is about describing the hardware, the >>> default-mode property doesn't really fit in... >> >> Maybe we should rename it to native-mode then ? > > Hmm, right, if it means native mode, then it is describing the > hardware. But would it make sense to require that the native mode > is the first mode in the list, then? This would make the separate > default-mode/native-mode property not needed. I'm not sure if device-tree guarantees that the nodes enumerate in a specific order. If it does, then that may be a reasonable solution. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/08/2012 02:25 AM, Guennadi Liakhovetski wrote: > On Fri, 5 Oct 2012, Stephen Warren wrote: > >> On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: >>> Hi Steffen >>> >>> Sorry for chiming in so late in the game, but I've long been wanting to >>> have a look at this and compare with what we do for V4L2, so, this seems a >>> great opportunity to me:-) >>> >>> On Thu, 4 Oct 2012, Steffen Trumtrar wrote: >> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt b/Documentation/devicetree/bindings/video/display-timings.txt >> +timings-subnode +--- + +required properties: + - hactive, vactive: Display resolution + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters + in pixels + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in + lines + - clock: displayclock in Hz >>> >>> You're going to hate me for this, but eventually we want to actually >>> reference clock objects in our DT bindings. For now, even if you don't >>> want to actually add clock phandles and stuff here, I think, using the >>> standard "clock-frequency" property would be much better! >> >> In a definition of a display timing, we will never need to use the clock >> binding; the clock binding would be used by the HW module that is >> generating a timing, not by the timing definition itself. > > You mean clock consumer bindings will be in the display device DT node? > And the display-timings node will be its child? Yes ... + - interlaced (bool) >>> >>> Is "interlaced" a property of the hardware, i.e. of the board? Can the >>> same display controller on one board require interlaced data and on >>> another board - progressive? >> >> Interlace is a property of a display mode. It's quite possible for a >> particular display controller to switch between interlace and >> progressive output at run-time. For example, reconfiguring the output >> between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to >> a built-in LCD display, you're probably always going to be driving the >> single mode required by the panel, and that mode will likely always be >> progressive. However, since this binding attempts to describe any >> display timing, I think we still need this property per mode. > > But why do you need this in the DT then at all? Because the driver for the display controller has no idea what display or panel will be connected to it. > If it's fixed, as required > per display controller, then its driver will know it. If it's runtime > configurable, then it's a purely software parameter and doesn't depend on > the board? interlace-vs-progressive isn't "fixed, as required per display controller", but is a property of the mode being sent by the display controller, and the requirements for that mode are driven by the panel/display connected to the display controller, not the display controller, in general. ... >>> BTW, I'm not very familiar with display >>> interfaces, but for interlaced you probably sometimes use a field signal, >>> whose polarity you also want to specify here? We use a "field-even-active" >>> integer property for it. >> >> I think that's a property of the display controller itself, rather than >> an individual mode, although I'm not 100% certain. My assertion is that >> the physical interface that the display controller is driving will >> determine whether embedded or separate sync is used, and in the separate >> sync case, how the field signal is defined, and that all interlace modes >> driven over that interface will use the same field signal definition. > > In general, I might be misunderstanding something, but don't we have to > distinguish between 2 types of information about display timings: (1) is > defined by the display controller requirements, is known to the display > driver and doesn't need to be present in timings DT. We did have some of > these parameters in board data previously, because we didn't have proper > display controller drivers... Yes, there probably is data of that kind, but the display mode timings binding is only address standardized display timings information, not controller-specific information, and hence doesn't cover this case. > (2) is board specific configuration, and is > such it has to be present in DT. Certainly, yes. > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need > to be present in DT? I don't believe so. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Mon, 2012-10-08 at 14:04 +0200, Laurent Pinchart wrote: > Hi Tomi, > > On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote: > > On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote: > > > In general, I might be misunderstanding something, but don't we have to > > > distinguish between 2 types of information about display timings: (1) is > > > defined by the display controller requirements, is known to the display > > > driver and doesn't need to be present in timings DT. We did have some of > > > these parameters in board data previously, because we didn't have proper > > > display controller drivers... (2) is board specific configuration, and is > > > such it has to be present in DT. > > > > > > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need > > > to be present in DT? > > > > As I see it, this DT data is about the display (most commonly LCD > > panel), i.e. what video mode(s) the panel supports. If things were done > > my way, the panel's supported timings would be defined in the driver for > > the panel, and DT would be left to describe board specific data, but > > this approach has its benefits. > > What about dumb DPI panels ? They will all be supported by a single driver, > would you have the driver contain information about all known DPI panels ? DT > seems a good solution to me in this case. Yes, I would have a table in the driver for all the devices it supports, which would describe the device specific parameters. But I don't have a problem with DT solution. Both methods have their pros and cons, and perhaps DT based solution is more practical. > For complex panels where the driver will support a single (or very few) model > I agree that specifying the timings in DT isn't needed. > > > Thus, if you connect an interlaced panel to your board, you need to tell > > the display controller that this panel requires interlace signal. Also, > > pixel clock source doesn't make sense in this context, as this doesn't > > describe the actual used configuration, but only what the panel > > supports. > > > > Of course, if this is about describing the hardware, the default-mode > > property doesn't really fit in... > > Maybe we should rename it to native-mode then ? Hmm, right, if it means native mode, then it is describing the hardware. But would it make sense to require that the native mode is the first mode in the list, then? This would make the separate default-mode/native-mode property not needed. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi Tomi, On Monday 08 October 2012 12:01:18 Tomi Valkeinen wrote: > On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote: > > In general, I might be misunderstanding something, but don't we have to > > distinguish between 2 types of information about display timings: (1) is > > defined by the display controller requirements, is known to the display > > driver and doesn't need to be present in timings DT. We did have some of > > these parameters in board data previously, because we didn't have proper > > display controller drivers... (2) is board specific configuration, and is > > such it has to be present in DT. > > > > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need > > to be present in DT? > > As I see it, this DT data is about the display (most commonly LCD > panel), i.e. what video mode(s) the panel supports. If things were done > my way, the panel's supported timings would be defined in the driver for > the panel, and DT would be left to describe board specific data, but > this approach has its benefits. What about dumb DPI panels ? They will all be supported by a single driver, would you have the driver contain information about all known DPI panels ? DT seems a good solution to me in this case. For complex panels where the driver will support a single (or very few) model I agree that specifying the timings in DT isn't needed. > Thus, if you connect an interlaced panel to your board, you need to tell > the display controller that this panel requires interlace signal. Also, > pixel clock source doesn't make sense in this context, as this doesn't > describe the actual used configuration, but only what the panel > supports. > > Of course, if this is about describing the hardware, the default-mode > property doesn't really fit in... Maybe we should rename it to native-mode then ? -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Mon, 2012-10-08 at 10:25 +0200, Guennadi Liakhovetski wrote: > In general, I might be misunderstanding something, but don't we have to > distinguish between 2 types of information about display timings: (1) is > defined by the display controller requirements, is known to the display > driver and doesn't need to be present in timings DT. We did have some of > these parameters in board data previously, because we didn't have proper > display controller drivers... (2) is board specific configuration, and is > such it has to be present in DT. > > In that way, doesn't "interlaced" belong to type (1) and thus doesn't need > to be present in DT? As I see it, this DT data is about the display (most commonly LCD panel), i.e. what video mode(s) the panel supports. If things were done my way, the panel's supported timings would be defined in the driver for the panel, and DT would be left to describe board specific data, but this approach has its benefits. Thus, if you connect an interlaced panel to your board, you need to tell the display controller that this panel requires interlace signal. Also, pixel clock source doesn't make sense in this context, as this doesn't describe the actual used configuration, but only what the panel supports. Of course, if this is about describing the hardware, the default-mode property doesn't really fit in... Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Fri, 5 Oct 2012, Stephen Warren wrote: > On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: > > Hi Steffen > > > > Sorry for chiming in so late in the game, but I've long been wanting to > > have a look at this and compare with what we do for V4L2, so, this seems a > > great opportunity to me:-) > > > > On Thu, 4 Oct 2012, Steffen Trumtrar wrote: > > >> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > >> b/Documentation/devicetree/bindings/video/display-timings.txt > > >> +timings-subnode > >> +--- > >> + > >> +required properties: > >> + - hactive, vactive: Display resolution > >> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing > >> parameters > >> + in pixels > >> + vfront-porch, vback-porch, vsync-len: Vertical display timing > >> parameters in > >> + lines > >> + - clock: displayclock in Hz > > > > You're going to hate me for this, but eventually we want to actually > > reference clock objects in our DT bindings. For now, even if you don't > > want to actually add clock phandles and stuff here, I think, using the > > standard "clock-frequency" property would be much better! > > In a definition of a display timing, we will never need to use the clock > binding; the clock binding would be used by the HW module that is > generating a timing, not by the timing definition itself. You mean clock consumer bindings will be in the display device DT node? And the display-timings node will be its child? > That said, your comment about renaming the property to avoid any kind of > conceptual conflict is still quite valid. This is bike-shedding, but > "pixel-clock" might be more in line with typical video mode terminology, > although there's certainly preference in DT for using the generic term > clock-frequency that you proposed. Either is fine by me. > > >> +optional properties: > >> + - hsync-active-high (bool): Hsync pulse is active high > >> + - vsync-active-high (bool): Vsync pulse is active high > > > > For the above two we also considered using bool properties but eventually > > settled down with integer ones: > > > > - hsync-active = <1> > > > > for active-high and 0 for active low. This has the added advantage of > > being able to omit this property in the .dts, which then doesn't mean, > > that the polarity is active low, but rather, that the hsync line is not > > used on this hardware. So, maybe it would be good to use the same binding > > here too? > > I agree. This also covers the case where analog display connectors often > use polarity to differentiate similar modes, yet digital connectors > often always use a fixed polarity since the receiving device can > "measure" the signal in more complete ways. > > If the board HW inverts these lines, the same property names can exist > in the display controller itself, and the two values XORd together to > yield the final output polarity. > > >> + - de-active-high (bool): Data-Enable pulse is active high > >> + - pixelclk-inverted (bool): pixelclock is inverted > > > > We don't (yet) have a de-active property in V4L, don't know whether we'll > > ever have to distingsuish between what some datasheets call "HREF" and > > HSYNC in DT, but maybe similarly to the above an integer would be > > preferred. As for pixclk, we call the property "pclk-sample" and it's also > > an integer. > > Thinking about this more: de-active-high is likely to be a > board-specific property and hence something in the display controller, > not in the mode definition? > > >> + - interlaced (bool) > > > > Is "interlaced" a property of the hardware, i.e. of the board? Can the > > same display controller on one board require interlaced data and on > > another board - progressive? > > Interlace is a property of a display mode. It's quite possible for a > particular display controller to switch between interlace and > progressive output at run-time. For example, reconfiguring the output > between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to > a built-in LCD display, you're probably always going to be driving the > single mode required by the panel, and that mode will likely always be > progressive. However, since this binding attempts to describe any > display timing, I think we still need this property per mode. But why do you need this in the DT then at all? If it's fixed, as required per display controller, then its driver will know it. If it's runtime configurable, then it's a purely software parameter and doesn't depend on the board? > > BTW, I'm not very familiar with display > > interfaces, but for interlaced you probably sometimes use a field signal, > > whose polarity you also want to specify here? We use a "field-even-active" > > integer property for it. > > I think that's a property of the display controller itself, rather than > an individual mode, although I'm not 100% certain. My assertion is that > the physical interface that the display cont
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi,
On Mon, Oct 08, 2012 at 10:07:45AM +0300, Tomi Valkeinen wrote:
> Hi,
>
> On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> > Signed-off-by: Steffen Trumtrar
> > ---
> > .../devicetree/bindings/video/display-timings.txt | 222
> >
> > drivers/of/Kconfig |5 +
> > drivers/of/Makefile|1 +
> > drivers/of/of_display_timings.c| 183
> > include/linux/of_display_timings.h | 85
> > 5 files changed, 496 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/video/display-timings.txt
> > create mode 100644 drivers/of/of_display_timings.c
> > create mode 100644 include/linux/of_display_timings.h
> >
> > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt
> > b/Documentation/devicetree/bindings/video/display-timings.txt
> > new file mode 100644
> > index 000..45e39bd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> > @@ -0,0 +1,222 @@
> > +display-timings bindings
> > +==
> > +
> > +display-timings-node
> > +
> > +
> > +required properties:
> > + - none
> > +
> > +optional properties:
> > + - default-timing: the default timing value
> > +
> > +timings-subnode
> > +---
> > +
> > +required properties:
> > + - hactive, vactive: Display resolution
> > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing
> > parameters
> > + in pixels
> > + vfront-porch, vback-porch, vsync-len: Vertical display timing
> > parameters in
> > + lines
> > + - clock: displayclock in Hz
> > +
> > +optional properties:
> > + - hsync-active-high (bool): Hsync pulse is active high
> > + - vsync-active-high (bool): Vsync pulse is active high
> > + - de-active-high (bool): Data-Enable pulse is active high
> > + - pixelclk-inverted (bool): pixelclock is inverted
> > + - interlaced (bool)
> > + - doublescan (bool)
>
> I think bool should be generally used for things that are on/off, like
> interlace. For hsync-active-high & others I'd rather have 0/1 values as
> others already suggested.
>
> > +There are different ways of describing the capabilities of a display. The
> > devicetree
> > +representation corresponds to the one commonly found in datasheets for
> > displays.
> > +If a display supports multiple signal timings, the default-timing can be
> > specified.
> > +
> > +The parameters are defined as
> > +
> > +struct signal_timing
> > +===
> > +
> > +
> > +--+-+--+---+
> > + | |↑| |
> >|
> > + | ||vback_porch | |
> >|
> > + | |↓| |
> >|
> > +
> > +--###--+---+
> > + | #↑# |
> >|
> > + | #|# |
> >|
> > + | hback #|# hfront |
> > hsync |
> > + | porch #| hactive # porch |
> > len |
> > +
> > |<>#<---+--->#<>|<->|
> > + | #|# |
> >|
> > + | #|vactive # |
> >|
> > + | #|# |
> >|
> > + | #↓# |
> >|
> > +
> > +--###--+---+
> > + | |↑| |
> >|
> > + | ||vfront_porch| |
> >|
> > + | |↓| |
> >|
> > +
> > +--+-+--+---+
> > + | |↑| |
> >|
> > + | ||vsync_len | |
> >|
> > + | |↓| |
> >|
> > +
> > +--+-+--+---+
> > +
> > +
> > +Example:
> > +
> > + display-timings {
> > + default-timing = <&timing0>;
> > + timing0: 1920p24 {
> > + /* 1920x1080p24 */
>
> I think this is commonly called 1080p24.
>
Oops. Yes, you are right.
> > + clock = <5200>;
> > + hac
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Sun, Oct 07, 2012 at 03:38:25PM +0200, Laurent Pinchart wrote:
> Hi Steffen,
>
> On Friday 05 October 2012 18:38:58 Steffen Trumtrar wrote:
> > On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> > > On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > > > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> > > >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > > ...
> > >
> > > >>> + for_each_child_of_node(timings_np, entry) {
> > > >>> + struct signal_timing *st;
> > > >>> +
> > > >>> + st = of_get_display_timing(entry);
> > > >>> +
> > > >>> + if (!st)
> > > >>> + continue;
> > > >>
> > > >> I wonder if that shouldn't be an error?
> > > >
> > > > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit
> > > > quiet in case of a faulty spec, that is right.
> > >
> > > I did mean return an error; if we try to parse something and can't,
> > > shouldn't we return an error?
> > >
> > > I suppose it may be possible to limp on and use whatever subset of modes
> > > could be parsed and drop the others, which is what this code does, but
> > > the code after the loop would definitely return an error if zero timings
> > > were parseable.
> >
> > If a display supports multiple modes, I think it is better to have a working
> > mode (even if it is not the preferred one) than have none at all.
> > If there is no mode at all, that should be an error, right.
>
> If we fail completely in case of an error, DT writers will notice their bugs.
> If we ignore errors silently they won't, and we'll end up with buggy DTs (or,
> to be accurate, even more buggy DTs :-)). I'd rather fail completely in the
> first implementation and add workarounds later only if we need to.
>
Okay, that is two against one. And if you say it like this, Stephen and you are
right I guess. Fail completely it is then.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Mon, 2012-10-08 at 10:07 +0300, Tomi Valkeinen wrote: > Hi, > I don't see you setting disp->default_timing to OF_DEFAULT_TIMING in > case there's no default_timing found. > > Or, at least I presume OF_DEFAULT_TIMING is meant to mark non-existing > default timing. The name OF_DEFAULT_TIMING is not very descriptive to > me. Ah, I see now from the second patch how this is meant to be used. So if there's no default timing in DT data, disp->default_timing is 0, meaning the first entry. And the caller of of_get_videomode() will use OF_DEFAULT_TIMING as index to get the default mode. So I think it's ok. Tomi signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi,
On Thu, 2012-10-04 at 19:59 +0200, Steffen Trumtrar wrote:
> Signed-off-by: Steffen Trumtrar
> ---
> .../devicetree/bindings/video/display-timings.txt | 222
>
> drivers/of/Kconfig |5 +
> drivers/of/Makefile|1 +
> drivers/of/of_display_timings.c| 183
> include/linux/of_display_timings.h | 85
> 5 files changed, 496 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/video/display-timings.txt
> create mode 100644 drivers/of/of_display_timings.c
> create mode 100644 include/linux/of_display_timings.h
>
> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt
> b/Documentation/devicetree/bindings/video/display-timings.txt
> new file mode 100644
> index 000..45e39bd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timings.txt
> @@ -0,0 +1,222 @@
> +display-timings bindings
> +==
> +
> +display-timings-node
> +
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - default-timing: the default timing value
> +
> +timings-subnode
> +---
> +
> +required properties:
> + - hactive, vactive: Display resolution
> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
> + in pixels
> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters
> in
> + lines
> + - clock: displayclock in Hz
> +
> +optional properties:
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - de-active-high (bool): Data-Enable pulse is active high
> + - pixelclk-inverted (bool): pixelclock is inverted
> + - interlaced (bool)
> + - doublescan (bool)
I think bool should be generally used for things that are on/off, like
interlace. For hsync-active-high & others I'd rather have 0/1 values as
others already suggested.
> +There are different ways of describing the capabilities of a display. The
> devicetree
> +representation corresponds to the one commonly found in datasheets for
> displays.
> +If a display supports multiple signal timings, the default-timing can be
> specified.
> +
> +The parameters are defined as
> +
> +struct signal_timing
> +===
> +
> +
> +--+-+--+---+
> + | |↑| |
> |
> + | ||vback_porch | |
> |
> + | |↓| |
> |
> +
> +--###--+---+
> + | #↑# |
> |
> + | #|# |
> |
> + | hback #|# hfront |
> hsync |
> + | porch #| hactive # porch | len
> |
> +
> |<>#<---+--->#<>|<->|
> + | #|# |
> |
> + | #|vactive # |
> |
> + | #|# |
> |
> + | #↓# |
> |
> +
> +--###--+---+
> + | |↑| |
> |
> + | ||vfront_porch| |
> |
> + | |↓| |
> |
> +
> +--+-+--+---+
> + | |↑| |
> |
> + | ||vsync_len | |
> |
> + | |↓| |
> |
> +
> +--+-+--+---+
> +
> +
> +Example:
> +
> + display-timings {
> + default-timing = <&timing0>;
> + timing0: 1920p24 {
> + /* 1920x1080p24 */
I think this is commonly called 1080p24.
> + clock = <5200>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hfront-porch = <25>;
> + hback-porch = <25>;
> + hsync-len = <25>;
> + vback-porch = <2>;
> + vfront-porch = <2>;
> + vsync-len = <2>;
> + hsync-active
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi Steffen,
On Friday 05 October 2012 18:38:58 Steffen Trumtrar wrote:
> On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> > On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> > >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> > ...
> >
> > >>> + for_each_child_of_node(timings_np, entry) {
> > >>> + struct signal_timing *st;
> > >>> +
> > >>> + st = of_get_display_timing(entry);
> > >>> +
> > >>> + if (!st)
> > >>> + continue;
> > >>
> > >> I wonder if that shouldn't be an error?
> > >
> > > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit
> > > quiet in case of a faulty spec, that is right.
> >
> > I did mean return an error; if we try to parse something and can't,
> > shouldn't we return an error?
> >
> > I suppose it may be possible to limp on and use whatever subset of modes
> > could be parsed and drop the others, which is what this code does, but
> > the code after the loop would definitely return an error if zero timings
> > were parseable.
>
> If a display supports multiple modes, I think it is better to have a working
> mode (even if it is not the preferred one) than have none at all.
> If there is no mode at all, that should be an error, right.
If we fail completely in case of an error, DT writers will notice their bugs.
If we ignore errors silently they won't, and we'll end up with buggy DTs (or,
to be accurate, even more buggy DTs :-)). I'd rather fail completely in the
first implementation and add workarounds later only if we need to.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Fri, Oct 05, 2012 at 10:21:37AM -0600, Stephen Warren wrote:
> On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> > On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
> >> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
> ...
> >>> + for_each_child_of_node(timings_np, entry) {
> >>> + struct signal_timing *st;
> >>> +
> >>> + st = of_get_display_timing(entry);
> >>> +
> >>> + if (!st)
> >>> + continue;
> >>
> >> I wonder if that shouldn't be an error?
> >
> > In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet
> > in
> > case of a faulty spec, that is right.
>
> I did mean return an error; if we try to parse something and can't,
> shouldn't we return an error?
>
> I suppose it may be possible to limp on and use whatever subset of modes
> could be parsed and drop the others, which is what this code does, but
> the code after the loop would definitely return an error if zero timings
> were parseable.
If a display supports multiple modes, I think it is better to have a working
mode (even if it is not the preferred one) than have none at all.
If there is no mode at all, that should be an error, right.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 04, 2012 at 11:35:35PM +0200, Guennadi Liakhovetski wrote: > Hi Steffen > > Sorry for chiming in so late in the game, but I've long been wanting to > have a look at this and compare with what we do for V4L2, so, this seems a > great opportunity to me:-) > > On Thu, 4 Oct 2012, Steffen Trumtrar wrote: > > > Signed-off-by: Steffen Trumtrar > > --- > > .../devicetree/bindings/video/display-timings.txt | 222 > > > > drivers/of/Kconfig |5 + > > drivers/of/Makefile|1 + > > drivers/of/of_display_timings.c| 183 > > include/linux/of_display_timings.h | 85 > > 5 files changed, 496 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/video/display-timings.txt > > create mode 100644 drivers/of/of_display_timings.c > > create mode 100644 include/linux/of_display_timings.h > > > > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > > b/Documentation/devicetree/bindings/video/display-timings.txt > > new file mode 100644 > > index 000..45e39bd > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/video/display-timings.txt > > @@ -0,0 +1,222 @@ > > +display-timings bindings > > +== > > + > > +display-timings-node > > + > > + > > +required properties: > > + - none > > + > > +optional properties: > > + - default-timing: the default timing value > > + > > +timings-subnode > > +--- > > + > > +required properties: > > + - hactive, vactive: Display resolution > > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing > > parameters > > + in pixels > > + vfront-porch, vback-porch, vsync-len: Vertical display timing > > parameters in > > + lines > > + - clock: displayclock in Hz > > You're going to hate me for this, but eventually we want to actually > reference clock objects in our DT bindings. For now, even if you don't > want to actually add clock phandles and stuff here, I think, using the > standard "clock-frequency" property would be much better! > Well, that shouldn't be a big deal, the "clock-frequency" property I mean :-) > > + > > +optional properties: > > + - hsync-active-high (bool): Hsync pulse is active high > > + - vsync-active-high (bool): Vsync pulse is active high > > For the above two we also considered using bool properties but eventually > settled down with integer ones: > > - hsync-active = <1> > > for active-high and 0 for active low. This has the added advantage of > being able to omit this property in the .dts, which then doesn't mean, > that the polarity is active low, but rather, that the hsync line is not > used on this hardware. So, maybe it would be good to use the same binding > here too? > Never really thought about it that way. But the argument sounds convincing. > > + - de-active-high (bool): Data-Enable pulse is active high > > + - pixelclk-inverted (bool): pixelclock is inverted > > We don't (yet) have a de-active property in V4L, don't know whether we'll > ever have to distingsuish between what some datasheets call "HREF" and > HSYNC in DT, but maybe similarly to the above an integer would be > preferred. As for pixclk, we call the property "pclk-sample" and it's also > an integer. > > > + - interlaced (bool) > > Is "interlaced" a property of the hardware, i.e. of the board? Can the > same display controller on one board require interlaced data and on > another board - progressive? BTW, I'm not very familiar with display > interfaces, but for interlaced you probably sometimes use a field signal, > whose polarity you also want to specify here? We use a "field-even-active" > integer property for it. > I don't really know about that; have to collect some info first. > Thanks > Guennadi Thank you. Regards, Steffen -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/05/2012 10:16 AM, Steffen Trumtrar wrote:
> On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote:
>> On 10/04/2012 11:59 AM, Steffen Trumtrar wrote:
...
>>> + for_each_child_of_node(timings_np, entry) {
>>> + struct signal_timing *st;
>>> +
>>> + st = of_get_display_timing(entry);
>>> +
>>> + if (!st)
>>> + continue;
>>
>> I wonder if that shouldn't be an error?
>
> In the sense of a pr_err not a -EINVAL I presume?! It is a little bit quiet in
> case of a faulty spec, that is right.
I did mean return an error; if we try to parse something and can't,
shouldn't we return an error?
I suppose it may be possible to limp on and use whatever subset of modes
could be parsed and drop the others, which is what this code does, but
the code after the loop would definitely return an error if zero timings
were parseable.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 03:35 PM, Guennadi Liakhovetski wrote: > Hi Steffen > > Sorry for chiming in so late in the game, but I've long been wanting to > have a look at this and compare with what we do for V4L2, so, this seems a > great opportunity to me:-) > > On Thu, 4 Oct 2012, Steffen Trumtrar wrote: >> diff --git a/Documentation/devicetree/bindings/video/display-timings.txt >> b/Documentation/devicetree/bindings/video/display-timings.txt >> +timings-subnode >> +--- >> + >> +required properties: >> + - hactive, vactive: Display resolution >> + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing >> parameters >> + in pixels >> + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters >> in >> + lines >> + - clock: displayclock in Hz > > You're going to hate me for this, but eventually we want to actually > reference clock objects in our DT bindings. For now, even if you don't > want to actually add clock phandles and stuff here, I think, using the > standard "clock-frequency" property would be much better! In a definition of a display timing, we will never need to use the clock binding; the clock binding would be used by the HW module that is generating a timing, not by the timing definition itself. That said, your comment about renaming the property to avoid any kind of conceptual conflict is still quite valid. This is bike-shedding, but "pixel-clock" might be more in line with typical video mode terminology, although there's certainly preference in DT for using the generic term clock-frequency that you proposed. Either is fine by me. >> +optional properties: >> + - hsync-active-high (bool): Hsync pulse is active high >> + - vsync-active-high (bool): Vsync pulse is active high > > For the above two we also considered using bool properties but eventually > settled down with integer ones: > > - hsync-active = <1> > > for active-high and 0 for active low. This has the added advantage of > being able to omit this property in the .dts, which then doesn't mean, > that the polarity is active low, but rather, that the hsync line is not > used on this hardware. So, maybe it would be good to use the same binding > here too? I agree. This also covers the case where analog display connectors often use polarity to differentiate similar modes, yet digital connectors often always use a fixed polarity since the receiving device can "measure" the signal in more complete ways. If the board HW inverts these lines, the same property names can exist in the display controller itself, and the two values XORd together to yield the final output polarity. >> + - de-active-high (bool): Data-Enable pulse is active high >> + - pixelclk-inverted (bool): pixelclock is inverted > > We don't (yet) have a de-active property in V4L, don't know whether we'll > ever have to distingsuish between what some datasheets call "HREF" and > HSYNC in DT, but maybe similarly to the above an integer would be > preferred. As for pixclk, we call the property "pclk-sample" and it's also > an integer. Thinking about this more: de-active-high is likely to be a board-specific property and hence something in the display controller, not in the mode definition? >> + - interlaced (bool) > > Is "interlaced" a property of the hardware, i.e. of the board? Can the > same display controller on one board require interlaced data and on > another board - progressive? Interlace is a property of a display mode. It's quite possible for a particular display controller to switch between interlace and progressive output at run-time. For example, reconfiguring the output between 480i, 720p, 1080i, 1080p modes. Admittedly, if you're talking to a built-in LCD display, you're probably always going to be driving the single mode required by the panel, and that mode will likely always be progressive. However, since this binding attempts to describe any display timing, I think we still need this property per mode. > BTW, I'm not very familiar with display > interfaces, but for interlaced you probably sometimes use a field signal, > whose polarity you also want to specify here? We use a "field-even-active" > integer property for it. I think that's a property of the display controller itself, rather than an individual mode, although I'm not 100% certain. My assertion is that the physical interface that the display controller is driving will determine whether embedded or separate sync is used, and in the separate sync case, how the field signal is defined, and that all interlace modes driven over that interface will use the same field signal definition. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 04, 2012 at 12:47:16PM -0600, Stephen Warren wrote: > On 10/04/2012 11:59 AM, Steffen Trumtrar wrote: > > A patch description would be useful for something like this. > Yes. Shame on me. > > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > > b/Documentation/devicetree/bindings/video/display-timings.txt > > new file mode 100644 > ... > > +Usage in backend > > + > > Everything before that point in the file looks fine to me. > \o/ > Everything after this point in the file seems to be Linux-specific > implementation details. Does it really belong in the DT binding > documentation, rather than some Linux-specific documentation file? > I guess you are right about that. > > +struct drm_display_mode > > +=== > > + > > + > > +--+-+--+---+ > > + | | | | > >| ↑ > > + | | | | > >| | > > + | | | | > >| | > > + > > +--###--+---+ > > | > > I suspect the entire horizontal box above (and the entire vertical box > all the way down the left-hand side) should be on the bottom/right > instead of top/left. The reason I think this is because all of > vsync_start, vsync_end, vdisplay have to be referenced to some known > point, which is usually zero or the start of the timing definition, /or/ > there would be some value indicating the size of the top marging/porch > in order to say where those other values are referenced to. > > > + | # ↑ ↑ ↑# | > >| | > > + | # | | |# | > >| | > > + | # | | |# | > >| | > > + | # | | |# | > >| | > > + | # | | |# | > >| | > > + | # | | | hdisplay # | > >| | > > + | #<--++---># | > >| | > > + | # | | |# | > >| | > > + | # |vsync_start |# | > >| | > > + | # | | |# | > >| | > > + | # | |vsync_end |# | > >| | > > + | # | | |vdisplay# | > >| | > > + | # | | |# | > >| | > > + | # | | |# | > >| | > > + | # | | |# | > >| | vtotal > > + | # | | |# | > >| | > > + | # | | | hsync_start# | > >| | > > + | #<--+-+--+-->| > >| | > > + | # | | |# | > >| | > > + | # | | | hsync_end # | > >| | > > + | > > #<--+-+--+-->| | > > + | # | | ↓# | > >| | > > + > > +--|#|--+---+ > > | > > + | | | | | | > >| | > > + | | | | | | > >| | > > + | | ↓ | | | > >| | > > + > > +--+-+---+--+---+ > > | > > + | | | | | > >| | > > + | | | | | > >| | > > + | | ↓ | | > >| ↓ > > + > > +--+-+--+---+ > > + htotal > > + > > <-> > > > diff --git a/drivers/of/of_display_timings.c > > b/drivers/of/of_display_timings.c > > > +static int parse_property(struct device_node *np, char *name, > > + struct tim
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On Thu, Oct 04, 2012 at 11:35:35PM +0200, Guennadi Liakhovetski wrote: > > +optional properties: > > + - hsync-active-high (bool): Hsync pulse is active high > > + - vsync-active-high (bool): Vsync pulse is active high > > For the above two we also considered using bool properties but eventually > settled down with integer ones: > > - hsync-active = <1> > > for active-high and 0 for active low. This has the added advantage of > being able to omit this property in the .dts, which then doesn't mean, > that the polarity is active low, but rather, that the hsync line is not > used on this hardware. So, maybe it would be good to use the same binding > here too? Philipp, this is the same argumentation as we discussed yesterday for the dual-link LVDS option, so that one could be modelled in a similar way. rsc -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
Hi Steffen Sorry for chiming in so late in the game, but I've long been wanting to have a look at this and compare with what we do for V4L2, so, this seems a great opportunity to me:-) On Thu, 4 Oct 2012, Steffen Trumtrar wrote: > Signed-off-by: Steffen Trumtrar > --- > .../devicetree/bindings/video/display-timings.txt | 222 > > drivers/of/Kconfig |5 + > drivers/of/Makefile|1 + > drivers/of/of_display_timings.c| 183 > include/linux/of_display_timings.h | 85 > 5 files changed, 496 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/video/display-timings.txt > create mode 100644 drivers/of/of_display_timings.c > create mode 100644 include/linux/of_display_timings.h > > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > b/Documentation/devicetree/bindings/video/display-timings.txt > new file mode 100644 > index 000..45e39bd > --- /dev/null > +++ b/Documentation/devicetree/bindings/video/display-timings.txt > @@ -0,0 +1,222 @@ > +display-timings bindings > +== > + > +display-timings-node > + > + > +required properties: > + - none > + > +optional properties: > + - default-timing: the default timing value > + > +timings-subnode > +--- > + > +required properties: > + - hactive, vactive: Display resolution > + - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters > + in pixels > + vfront-porch, vback-porch, vsync-len: Vertical display timing parameters > in > + lines > + - clock: displayclock in Hz You're going to hate me for this, but eventually we want to actually reference clock objects in our DT bindings. For now, even if you don't want to actually add clock phandles and stuff here, I think, using the standard "clock-frequency" property would be much better! > + > +optional properties: > + - hsync-active-high (bool): Hsync pulse is active high > + - vsync-active-high (bool): Vsync pulse is active high For the above two we also considered using bool properties but eventually settled down with integer ones: - hsync-active = <1> for active-high and 0 for active low. This has the added advantage of being able to omit this property in the .dts, which then doesn't mean, that the polarity is active low, but rather, that the hsync line is not used on this hardware. So, maybe it would be good to use the same binding here too? > + - de-active-high (bool): Data-Enable pulse is active high > + - pixelclk-inverted (bool): pixelclock is inverted We don't (yet) have a de-active property in V4L, don't know whether we'll ever have to distingsuish between what some datasheets call "HREF" and HSYNC in DT, but maybe similarly to the above an integer would be preferred. As for pixclk, we call the property "pclk-sample" and it's also an integer. > + - interlaced (bool) Is "interlaced" a property of the hardware, i.e. of the board? Can the same display controller on one board require interlaced data and on another board - progressive? BTW, I'm not very familiar with display interfaces, but for interlaced you probably sometimes use a field signal, whose polarity you also want to specify here? We use a "field-even-active" integer property for it. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2 v6] of: add helper to parse display timings
On 10/04/2012 11:59 AM, Steffen Trumtrar wrote: A patch description would be useful for something like this. > diff --git a/Documentation/devicetree/bindings/video/display-timings.txt > b/Documentation/devicetree/bindings/video/display-timings.txt > new file mode 100644 ... > +Usage in backend > + Everything before that point in the file looks fine to me. Everything after this point in the file seems to be Linux-specific implementation details. Does it really belong in the DT binding documentation, rather than some Linux-specific documentation file? > +struct drm_display_mode > +=== > + > + > +--+-+--+---+ > + | | | | > | ↑ > + | | | | > | | > + | | | | > | | > + > +--###--+---+ > | I suspect the entire horizontal box above (and the entire vertical box all the way down the left-hand side) should be on the bottom/right instead of top/left. The reason I think this is because all of vsync_start, vsync_end, vdisplay have to be referenced to some known point, which is usually zero or the start of the timing definition, /or/ there would be some value indicating the size of the top marging/porch in order to say where those other values are referenced to. > + | # ↑ ↑ ↑# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | | hdisplay # | > | | > + | #<--++---># | > | | > + | # | | |# | > | | > + | # |vsync_start |# | > | | > + | # | | |# | > | | > + | # | |vsync_end |# | > | | > + | # | | |vdisplay# | > | | > + | # | | |# | > | | > + | # | | |# | > | | > + | # | | |# | > | | vtotal > + | # | | |# | > | | > + | # | | | hsync_start# | > | | > + | #<--+-+--+-->| > | | > + | # | | |# | > | | > + | # | | | hsync_end # | > | | > + | > #<--+-+--+-->| | > + | # | | ↓# | > | | > + > +--|#|--+---+ > | > + | | | | | | > | | > + | | | | | | > | | > + | | ↓ | | | > | | > + > +--+-+---+--+---+ > | > + | | | | | > | | > + | | | | | > | | > + | | ↓ | | > | ↓ > + > +--+-+--+---+ > + htotal > + > <-> > diff --git a/drivers/of/of_display_timings.c b/drivers/of/of_display_timings.c > +static int parse_property(struct device_node *np, char *name, > + struct timing_entry *result) > + if (cells == 1) > + ret = of_property_read_u32_array(np, name, &result->typ, cells); Should that branch not just set result->min/max to typ as well? Presumably it'd prevent any code that interprets struct timing_entry from having to check if those values were 0 or not? > + else if (cells == 3) > +
[PATCH 1/2 v6] of: add helper to parse display timings
Signed-off-by: Steffen Trumtrar
---
.../devicetree/bindings/video/display-timings.txt | 222
drivers/of/Kconfig |5 +
drivers/of/Makefile|1 +
drivers/of/of_display_timings.c| 183
include/linux/of_display_timings.h | 85
5 files changed, 496 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/display-timings.txt
create mode 100644 drivers/of/of_display_timings.c
create mode 100644 include/linux/of_display_timings.h
diff --git a/Documentation/devicetree/bindings/video/display-timings.txt
b/Documentation/devicetree/bindings/video/display-timings.txt
new file mode 100644
index 000..45e39bd
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/display-timings.txt
@@ -0,0 +1,222 @@
+display-timings bindings
+==
+
+display-timings-node
+
+
+required properties:
+ - none
+
+optional properties:
+ - default-timing: the default timing value
+
+timings-subnode
+---
+
+required properties:
+ - hactive, vactive: Display resolution
+ - hfront-porch, hback-porch, hsync-len: Horizontal Display timing parameters
+ in pixels
+ vfront-porch, vback-porch, vsync-len: Vertical display timing parameters in
+ lines
+ - clock: displayclock in Hz
+
+optional properties:
+ - hsync-active-high (bool): Hsync pulse is active high
+ - vsync-active-high (bool): Vsync pulse is active high
+ - de-active-high (bool): Data-Enable pulse is active high
+ - pixelclk-inverted (bool): pixelclock is inverted
+ - interlaced (bool)
+ - doublescan (bool)
+
+There are different ways of describing the capabilities of a display. The
devicetree
+representation corresponds to the one commonly found in datasheets for
displays.
+If a display supports multiple signal timings, the default-timing can be
specified.
+
+The parameters are defined as
+
+struct signal_timing
+===
+
+ +--+-+--+---+
+ | |↑| | |
+ | ||vback_porch | | |
+ | |↓| | |
+ +--###--+---+
+ | #↑# | |
+ | #|# | |
+ | hback #|# hfront | hsync |
+ | porch #| hactive # porch | len |
+ |<>#<---+--->#<>|<->|
+ | #|# | |
+ | #|vactive # | |
+ | #|# | |
+ | #↓# | |
+ +--###--+---+
+ | |↑| | |
+ | ||vfront_porch| | |
+ | |↓| | |
+ +--+-+--+---+
+ | |↑| | |
+ | ||vsync_len | | |
+ | |↓| | |
+ +--+-+--+---+
+
+
+Example:
+
+ display-timings {
+ default-timing = <&timing0>;
+ timing0: 1920p24 {
+ /* 1920x1080p24 */
+ clock = <5200>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hfront-porch = <25>;
+ hback-porch = <25>;
+ hsync-len = <25>;
+ vback-porch = <2>;
+ vfront-porch = <2>;
+ vsync-len = <2>;
+ hsync-active-high;
+ };
+ };
+
+Every property also supports the use of ranges, so the commonly used datasheet
+description with -tuples can be used.
+
+Example:
+
+ timing1: timing {
+ /* 1920x1080p24 */
+ clock = <14850>;
+ hactive = <1920>;
+ vactive = <1080>;
+ hsync-len = <0 44 60>;
+ hfront-porch = <80 88 95>;
+ hback-porch = <100 148 160>;
+ vfront-porch = <0 4 6>;
+ vback-por
