Re: [PATCH v12 2/6] video: add of helper for videomode

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

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

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@pengutronix.de>
> + *
> + * 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@pengutronix.de>
> + *
> + * 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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]