On 2012-11-20 17:54, Steffen Trumtrar wrote:
> +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-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/non-inverted/ignored
Inverted related to what? And isn't this a bool? Pixel clock is either
normal (whatever that is), or inverted. It can't be "not used".
I guess normal case is "pixel data is driven on the rising edge of pixel
clock"? If that's common knowledge, I guess it doesn't need to be
mentioned. But I always have to verify from the documentation what
"normal" means on this particular panel/soc =).
> + - interlaced (bool)
> + - doublescan (bool)
> +
> +All the optional properties that are not bool follow the following logic:
> + <1>: high active
> + <0>: low active
> + omitted: not used on hardware
Perhaps it's obvious, but no harm being explicit: mention that the bool
properties are off is omitted.
And I didn't read the rest of the patches yet, so perhaps this is
already correct, but as I think this framework is usable without DT
also, the meanings of the fields in the structs should be explained in
the header files also in case they are not obvious.
> +Example:
> +
> + display-timings {
> + native-mode = <&timing0>;
> + timing0: 1920p24 {
This should still be 1080p24, not 1920p24 =).
> + /* 1920x1080p24 */
> + clock-frequency = <52000000>;
> + hactive = <1920>;
> + vactive = <1080>;
> + hfront-porch = <25>;
> + hback-porch = <25>;
> + hsync-len = <25>;
> + vback-porch = <2>;
> + vfront-porch = <2>;
> + vsync-len = <2>;
> + hsync-active = <1>;
> + };
> + };
> +
> diff --git a/include/linux/of_display_timings.h b/include/linux/of_display_timings.h
> new file mode 100644
> index 0000000..2b4fa0a
> --- /dev/null
> +++ b/include/linux/of_display_timings.h
> @@ -0,0 +1,20 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> + *
> + * display timings of helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_DISPLAY_TIMINGS_H
> +#define __LINUX_OF_DISPLAY_TIMINGS_H
> +
> +#include <linux/display_timing.h>
> +#include <linux/of.h>
No need to include these, just add "struct ...;".
> +#define OF_USE_NATIVE_MODE -1
> +
> +struct display_timings *of_get_display_timings(const struct device_node *np);
> +int of_display_timings_exists(const struct device_node *np);
> +
> +#endif
> diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
> new file mode 100644
> index 0000000..4de5fcc
> --- /dev/null
> +++ b/include/linux/of_videomode.h
> @@ -0,0 +1,18 @@
> +/*
> + * Copyright 2012 Steffen Trumtrar <s.trumtrar@xxxxxxxxxxxxxx>
> + *
> + * videomode of-helpers
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_OF_VIDEOMODE_H
> +#define __LINUX_OF_VIDEOMODE_H
> +
> +#include <linux/videomode.h>
> +#include <linux/of.h>
Same here.
> +int of_get_videomode(const struct device_node *np, struct videomode *vm,
> + int index);
> +
> +#endif /* __LINUX_OF_VIDEOMODE_H */
>
Tomi
Attachment:
signature.asc
Description: OpenPGP digital signature
