Re: [PATCHv15 3/7] video: add of helper for display timings/videomode

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 5997 bytes --]

Hi,

On 2012-11-26 11:07, Steffen Trumtrar wrote:
> This adds support for reading display timings from DT into a struct
> display_timings. The of_display_timing implementation supports multiple
> subnodes. All children are read into an array, that can be queried.
> 
> If no native mode is specified, the first subnode will be used.
> 
> For cases where the graphics driver knows there can be only one
> mode description or where the driver only supports one mode, a helper
> function of_get_videomode is added, that gets a struct videomode from DT.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Acked-by: Stephen Warren <swarren@nvidia.com>
> Reviewed-by: Thierry Reding <thierry.reding@avionic-design.de>
> Acked-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Thierry Reding <thierry.reding@avionic-design.de>
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../devicetree/bindings/video/display-timing.txt   |  107 ++++++++++
>  drivers/video/Kconfig                              |   15 ++
>  drivers/video/Makefile                             |    2 +
>  drivers/video/of_display_timing.c                  |  219 ++++++++++++++++++++
>  drivers/video/of_videomode.c                       |   54 +++++
>  include/linux/of_display_timing.h                  |   20 ++
>  include/linux/of_videomode.h                       |   18 ++
>  7 files changed, 435 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/display-timing.txt
>  create mode 100644 drivers/video/of_display_timing.c
>  create mode 100644 drivers/video/of_videomode.c
>  create mode 100644 include/linux/of_display_timing.h
>  create mode 100644 include/linux/of_videomode.h
> 
> diff --git a/Documentation/devicetree/bindings/video/display-timing.txt b/Documentation/devicetree/bindings/video/display-timing.txt
> new file mode 100644
> index 0000000..e238f27
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/display-timing.txt
> @@ -0,0 +1,107 @@
> +display-timing bindings
> +=======================
> +
> +display-timings node
> +--------------------
> +
> +required properties:
> + - none
> +
> +optional properties:
> + - native-mode: The native mode for the display, in case multiple modes are
> +		provided. When omitted, assume the first node is the native.
> +
> +timing 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-frequency: display clock in Hz
> +
> +optional properties:
> + - hsync-active: hsync pulse is active low/high/ignored
> + - vsync-active: vsync pulse is active low/high/ignored
> + - de-active: data-enable pulse is active low/high/ignored
> + - pixelclk-inverted: pixelclock is inverted (active on falling edge)/
> +				non-inverted (active on rising edge)/
> +				     ignored (ignore property)

I think hsync-active and vsync-active are clear, and commonly used, and
they are used for both drm and fb mode conversions in later patches.

de-active is not used in drm and fb mode conversions, but I think it's
also clear.

pixelclk-inverted is not used in the mode conversions. It's also a bit
unclear to me. What does it mean that pix clock is "active on rising
edge"? The pixel data is driven on rising edge? How about the sync
signals and DE, when are they driven? Does your HW have any settings
related to those?

OMAP has the invert pclk setting, but it also has a setting to define
when the sync signals are driven. The options are:
- syncs are driven on rising edge of pclk
- syncs are driven on falling edge of pclk
- syncs are driven on the opposite edge of pclk compared to the pixel data

For DE there's no setting, except the active high/low.

And if I'm not mistaken, if the optional properties are not defined,
they are not ignored, but left to the default 0. Which means active low,
or active on rising edge(?). I think it would be good to have a
"undefined" value for the properties.

> + - interlaced (bool): boolean to enable interlaced mode
> + - doublescan (bool): boolean to enable doublescan mode
> + - doubleclk (bool)

As I mentioned in the other mail, doubleclk is not used nor documented here.

> +All the optional properties that are not bool follow the following logic:
> +    <1>: high active
> +    <0>: low active
> +    omitted: not used on hardware
> +
> +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 native-mode can be specified.

I have some of the same concerns for this series than with the
interpreted power sequences (on fbdev): when you publish the DT
bindings, it's somewhat final version then, and fixing it later will be
difficult. Of course, video modes are much clearer than the power
sequences, so it's possible there won't be any problems with the DT
bindings.

However, I'd still feel safer if the series would be split to non-DT and
DT parts. The non-DT parts could be merged quite easily, and people
could start using them in their kernels. This should expose
bugs/problems related to the code.

The DT part could be merged later, when there's confidence that the
timings are good for all platforms.

Or, alternatively, all the non-common bindings (de-active, pck
invert,...) that are not used for fbdev or drm currently could be left
out for now. But I'd stil prefer merging it in two parts.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]