On Fri, Jul 18, 2014 at 02:13:57AM +0530, Ajay Kumar wrote:
> From: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
>
> Add DT binding documentation for ps8622/ps8625 bridge driver.
>
> Signed-off-by: Vincent Palatin <vpalatin@xxxxxxxxxxxx>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>
> ---
> .../devicetree/bindings/drm/bridge/ps8622.txt | 21 ++++++++++++++++++++
This is the wrong directory. Bindings are supposed to be OS agnostic,
but DRM is (mostly) Linux-specific. video/bridge would be a better
subdirectory for this. Somebody really ought to be moving out the
existing bindings in the drm subdirectory to a more proper location.
> 1 file changed, 21 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/drm/bridge/ps8622.txt
>
> diff --git a/Documentation/devicetree/bindings/drm/bridge/ps8622.txt b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> new file mode 100644
> index 0000000..1d154ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/drm/bridge/ps8622.txt
> @@ -0,0 +1,21 @@
> +ps8622-bridge bindings
> +
> +Required properties:
> + - compatible: "parade,ps8622" or "parade,ps8625"
Documentation/devicetree/bindings/vendor-prefixes.txt doesn't contain an
entry for parade yet.
> + - reg: first i2c address of the bridge
> + - sleep-gpios: OF device-tree gpio specification
> + - reset-gpios: OF device-tree gpio specification
> +
> +Optional properties:
> + - lane-count: number of DP lanes to use
> + - use-external-pwm: backlight will be controlled by an external PWM
In case of an external backlight, don't you still need a way to control
it? Perhaps instead of using this boolean property you should make this
take a phandle to the real backlight? Like so:
backlight {
compatible = "pwm-backlight";
...
}
bridge@48 {
compatible = "parade,ps8622";
...
backlight = <&/backlight>;
}
Then you can simply look up the backlight device when that property
exists and instantiate the built-in backlight otherwise.
> +
> +Example:
> + ps8622-bridge@48 {
> + compatible = "parade,ps8622";
> + reg = <0x48>;
> + sleep-gpios = <&gpc3 6 1 0 0>;
> + reset-gpios = <&gpc3 1 1 0 0>;
> + display-timings = <&lcd_display_timings>;
Why is this specifying display-timings? It's not mentioned in the
binding above and I don't see why the bridge would need to provide
one since it's really a property of the panel.
Thierry
Attachment:
pgptIOwWLFPgx.pgp
Description: PGP signature
