[1/2] drm/panel: Add DT bindings for Samsung S6D16D0

Message ID 20181008110006.24561-1-linus.walleij@linaro.org
State Superseded
Headers show
Series
  • [1/2] drm/panel: Add DT bindings for Samsung S6D16D0
Related show

Commit Message

Linus Walleij Oct. 8, 2018, 11 a.m.
This adds the device tree bindings for the Samsung S6D16D0
panel. This is a command mode only panel using DSI.

Cc: devicetree@vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../display/panel/samsung,s6d16d0.txt         | 30 +++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt

Comments

Sam Ravnborg Oct. 9, 2018, 7:26 p.m. | #1
On Mon, Oct 08, 2018 at 01:00:05PM +0200, Linus Walleij wrote:
> This adds the device tree bindings for the Samsung S6D16D0
> panel. This is a command mode only panel using DSI.
> 
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../display/panel/samsung,s6d16d0.txt         | 30 +++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt b/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt
> new file mode 100644
> index 000000000000..b94e366f451b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt
> @@ -0,0 +1,30 @@
> +Samsung S6D16D0 4" 864x480 AMOLED panel
> +
> +Required properties:
> +  - compatible: should be:
> +    "samsung,s6d16d0",
> +  - reg: the virtual channel number of a DSI peripheral
> +  - vdd1-supply: I/O voltage supply
> +  - reset-gpios: a GPIO spec for the reset pin (active low)
That it is active low is part of the GPIO part and should not
be mandated as part of the binding?

> +
> +The device node can contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +media/video-interfaces.txt. This node should describe panel's video bus.
> +
> +Example:
> +&dsi {
> +	...
> +
> +	panel@0 {
> +		compatible = "samsung,s6d16d0";
> +		reg = <0>;
> +		vdd1-supply = <&foo>;
> +		reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
This line specifies if ACTIVE HIGH or LOW.

	Sam
Linus Walleij Oct. 11, 2018, 8:48 a.m. | #2
On Tue, Oct 9, 2018 at 9:26 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> On Mon, Oct 08, 2018 at 01:00:05PM +0200, Linus Walleij wrote:

> > +  - reset-gpios: a GPIO spec for the reset pin (active low)
>
> That it is active low is part of the GPIO part and should not
> be mandated as part of the binding?

The fact that the consumer requires the line to be asserted
low is an electronic property of the component, i.e. the panel,
and it is just there explaining that if you set it to anything else than
active low, you are probably doing something wrong.

Should I specify "must be set as active low" instead to make
it clear?

> > +             reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
>
> This line specifies if ACTIVE HIGH or LOW.

Yep we always specify that on the consumer side. Or we should
always do that ... but since this panel is by defintion expecting
an active low signal, anything else would be an error and that
is what I am trying to say in the description.

Yours,
Linus Walleij
Andrzej Hajda Oct. 11, 2018, 8:59 a.m. | #3
On 11.10.2018 10:48, Linus Walleij wrote:
> On Tue, Oct 9, 2018 at 9:26 PM Sam Ravnborg <sam@ravnborg.org> wrote:
>> On Mon, Oct 08, 2018 at 01:00:05PM +0200, Linus Walleij wrote:
>>> +  - reset-gpios: a GPIO spec for the reset pin (active low)
>> That it is active low is part of the GPIO part and should not
>> be mandated as part of the binding?
> The fact that the consumer requires the line to be asserted
> low is an electronic property of the component, i.e. the panel,
> and it is just there explaining that if you set it to anything else than
> active low, you are probably doing something wrong.
>
> Should I specify "must be set as active low" instead to make
> it clear?

Common practice is as you wrote, I think it is OK.

Regards
Andrzej

>
>>> +             reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
>> This line specifies if ACTIVE HIGH or LOW.
> Yep we always specify that on the consumer side. Or we should
> always do that ... but since this panel is by defintion expecting
> an active low signal, anything else would be an error and that
> is what I am trying to say in the description.
>
> Yours,
> Linus Walleij
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt b/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt
new file mode 100644
index 000000000000..b94e366f451b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/samsung,s6d16d0.txt
@@ -0,0 +1,30 @@ 
+Samsung S6D16D0 4" 864x480 AMOLED panel
+
+Required properties:
+  - compatible: should be:
+    "samsung,s6d16d0",
+  - reg: the virtual channel number of a DSI peripheral
+  - vdd1-supply: I/O voltage supply
+  - reset-gpios: a GPIO spec for the reset pin (active low)
+
+The device node can contain one 'port' child node with one child
+'endpoint' node, according to the bindings defined in
+media/video-interfaces.txt. This node should describe panel's video bus.
+
+Example:
+&dsi {
+	...
+
+	panel@0 {
+		compatible = "samsung,s6d16d0";
+		reg = <0>;
+		vdd1-supply = <&foo>;
+		reset-gpios = <&foo_gpio 0 GPIO_ACTIVE_LOW>;
+
+		port {
+			panel_in: endpoint {
+				remote-endpoint = <&dsi_out>;
+			};
+		};
+	};
+};