Re: [PATCH V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver

From: Thierry Reding <thierry.reding@gmail.com>
To: Ajay Kumar <ajaykumar.rs@samsung.com>
Cc: linux-samsung-soc@vger.kernel.org,
	Vincent Palatin <vpalatin@chromium.org>,
	seanpaul@google.com, daniel.vetter@ffwll.ch, joshi@samsung.com,
	dri-devel@lists.freedesktop.org, ajaynumb@gmail.com,
	javier@dowhile0.org, prashanth.g@samsung.com
Subject: Re: [PATCH V5 11/12] Documentation: Add DT bindings for ps8622/ps8625 bridge driver
Date: Mon, 21 Jul 2014 09:06:29 +0200	[thread overview]
Message-ID: <20140721070627.GA8843@ulmo> (raw)
In-Reply-To: <1405629839-12086-12-git-send-email-ajaykumar.rs@samsung.com>

[-- Attachment #1.1: Type: text/plain, Size: 2474 bytes --]

On Fri, Jul 18, 2014 at 02:13:57AM +0530, Ajay Kumar wrote:
> From: Vincent Palatin <vpalatin@chromium.org>
> 
> Add DT binding documentation for ps8622/ps8625 bridge driver.
> 
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> ---
>  .../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 #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel