diff mbox series

[v5,2/2] dt-bindings: Add Truly NT35597 panel bindings

Message ID 1533264572-6364-2-git-send-email-abhinavk@codeaurora.org (mailing list archive)
State Not Applicable, archived
Delegated to: Andy Gross
Headers show
Series [v5,1/2] drm/panel: Add support for Truly NT35597 panel | expand

Commit Message

Abhinav Kumar Aug. 3, 2018, 2:49 a.m. UTC
From: "abhinavk@codeaurora.org" <abhinavk@codeaurora.org>

Add the device tree bindings for Truly NT35597 panel.
This panel supports both single DSI and dual DSI.

However, this patch series supports only dual DSI.

Changes in v5:
  - None

Signed-off-by: Abhinav Kumar <abhinavk@codeaurora.org>
---
 .../devicetree/bindings/display/truly,nt35597.txt  | 69 ++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/truly,nt35597.txt

Comments

Linus Walleij Aug. 3, 2018, 11:20 a.m. UTC | #1
On Fri, Aug 3, 2018 at 4:49 AM Abhinav Kumar <abhinavk@codeaurora.org> wrote:

> From: "abhinavk@codeaurora.org" <abhinavk@codeaurora.org>
>
> Add the device tree bindings for Truly NT35597 panel.
> This panel supports both single DSI and dual DSI.

I do not think this is a panel but a panel driver that can be used
with several physical panels. Can you confirm?

I suspect this since the command sequence in the driver seems
to contain a command for setting up the actual resolution and
a bunch of clocking properties for the physical panel.

> +Required properties:
> +- compatible: should be "truly,nt35597"

As with eg ili9322 I think this should have dual compatible strings
identifying the system it is used with since the resolution, clocking
etc is apparently
actually configurable.

compatible:
  "truly,nt35597", "qcom,reference-design1-name-display";
  "truly,nt35597", "qcom,reference-design2-name-display";

Then you send the command setting up resolution etc only for that
one system.

> +- vdda-supply: phandle of the regulator that provides the supply voltage
> +  Power IC supply
> +- vdispp-supply: phandle of the regulator that provides the supply voltage
> +  for positive LCD bias
> +- vdispn-supply: phandle of the regulator that provides the supply voltage
> +  for negative LCD bias
> +- reset-gpios: phandle of gpio for reset line
> +  This should be 8mA, gpio can be configured using mux, pinctrl, pinctrl-names
> +- mode-gpios: phandle of the gpio for choosing the mode of the display
> +  for single DSI or Dual DSI
> +  This should be low for dual DSI and high for single DSI mode
> +- display-timings: Node for the Panel timings

I don't think this belongs in the device tree at all.

Provide the timings from the driver based on the compatible string
instead, as you actually send commands to set up a certain timing in
the display controller.

(See ili9322 driver for an example of how I do this.)

> +- ports: This device has two video ports driven by two DSIs. Their connections
> +  are modelled using the OF graph bindings specified in
> +  Documentation/devicetree/bindings/graph.txt.
> +  - port@0: DSI input port driven by master DSI
> +  - port@1: DSI input port driven by secondary DSI

The rest seems fine.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Abhinav Kumar Aug. 3, 2018, 9:31 p.m. UTC | #2
Hi Linus

Thanks for your valuable comments.

Yes, we agree with your comments here and will address them.

Some details below on how we will take care of it.

Thanks

Abhinav

On 2018-08-03 04:20, Linus Walleij wrote:
> On Fri, Aug 3, 2018 at 4:49 AM Abhinav Kumar <abhinavk@codeaurora.org> 
> wrote:
> 
>> From: "abhinavk@codeaurora.org" <abhinavk@codeaurora.org>
>> 
>> Add the device tree bindings for Truly NT35597 panel.
>> This panel supports both single DSI and dual DSI.
> 
> I do not think this is a panel but a panel driver that can be used
> with several physical panels. Can you confirm?
Yes, from the documentation I have I can see that this is a panel
driver and can support other panels/resolutions.
> 
> I suspect this since the command sequence in the driver seems
> to contain a command for setting up the actual resolution and
> a bunch of clocking properties for the physical panel.
> 
>> +Required properties:
>> +- compatible: should be "truly,nt35597"
> 
> As with eg ili9322 I think this should have dual compatible strings
> identifying the system it is used with since the resolution, clocking
> etc is apparently
> actually configurable.
> 
> compatible:
>   "truly,nt35597", "qcom,reference-design1-name-display";
>   "truly,nt35597", "qcom,reference-design2-name-display";
> 
> Then you send the command setting up resolution etc only for that
> one system.
> 
Yes, I agree we will can have dual compatible strings and we will pick
resolution/modes based on the compatible string similar to this:

https://github.com/torvalds/linux/blob/master/drivers/gpu/drm/panel/panel-ilitek-ili9322.c#L659

>> +- vdda-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  Power IC supply
>> +- vdispp-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for positive LCD bias
>> +- vdispn-supply: phandle of the regulator that provides the supply 
>> voltage
>> +  for negative LCD bias
>> +- reset-gpios: phandle of gpio for reset line
>> +  This should be 8mA, gpio can be configured using mux, pinctrl, 
>> pinctrl-names
>> +- mode-gpios: phandle of the gpio for choosing the mode of the 
>> display
>> +  for single DSI or Dual DSI
>> +  This should be low for dual DSI and high for single DSI mode
>> +- display-timings: Node for the Panel timings
> 
> I don't think this belongs in the device tree at all.
> 
> Provide the timings from the driver based on the compatible string
> instead, as you actually send commands to set up a certain timing in
> the display controller.
> 
> (See ili9322 driver for an example of how I do this.)

Yes, will follow the example of ili9322 and do the same.

> 
>> +- ports: This device has two video ports driven by two DSIs. Their 
>> connections
>> +  are modelled using the OF graph bindings specified in
>> +  Documentation/devicetree/bindings/graph.txt.
>> +  - port@0: DSI input port driven by master DSI
>> +  - port@1: DSI input port driven by secondary DSI
> 
> The rest seems fine.
> 
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/truly,nt35597.txt b/Documentation/devicetree/bindings/display/truly,nt35597.txt
new file mode 100644
index 0000000..5d297b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/truly,nt35597.txt
@@ -0,0 +1,69 @@ 
+Truly model NT35597 1440x2560 DSI Panel
+
+Required properties:
+- compatible: should be "truly,nt35597"
+- vdda-supply: phandle of the regulator that provides the supply voltage
+  Power IC supply
+- vdispp-supply: phandle of the regulator that provides the supply voltage
+  for positive LCD bias
+- vdispn-supply: phandle of the regulator that provides the supply voltage
+  for negative LCD bias
+- reset-gpios: phandle of gpio for reset line
+  This should be 8mA, gpio can be configured using mux, pinctrl, pinctrl-names
+- mode-gpios: phandle of the gpio for choosing the mode of the display
+  for single DSI or Dual DSI
+  This should be low for dual DSI and high for single DSI mode
+- display-timings: Node for the Panel timings
+- ports: This device has two video ports driven by two DSIs. Their connections
+  are modelled using the OF graph bindings specified in
+  Documentation/devicetree/bindings/graph.txt.
+  - port@0: DSI input port driven by master DSI
+  - port@1: DSI input port driven by secondary DSI
+
+Example:
+
+	dsi@ae94000 {
+		panel@0 {
+			compatible = "truly,nt35597";
+			reg = <0>;
+			vdda-supply = <&pm8998_l14>;
+			vdispp-supply = <&lab_regulator>;
+			vdispn-supply = <&ibb_regulator>;
+			pinctrl-names = "default", "suspend";
+			pinctrl-0 = <&dpu_dsi_active>;
+			pinctrl-1 = <&dpu_dsi_suspend>;
+
+			reset-gpios = <&tlmm 6 0>;
+			mode-gpios = <&tlmm 52 0>;
+			display-timings {
+				timing0: timing-0 {
+					clock-frequency = <268316138>;
+					hactive = <1440>;
+					vactive = <2560>;
+					hfront-porch = <200>;
+					hback-porch = <64>;
+					hsync-len = <32>;
+					vfront-porch = <8>;
+					vback-porch = <7>;
+					vsync-len = <1>;
+				};
+			};
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+				port@0 {
+					reg = <0>;
+					panel0_in: endpoint {
+						remote-endpoint = <&dsi0_out>;
+					};
+				};
+
+				port@1 {
+					reg = <1>;
+					panel1_in: endpoint {
+						remote-endpoint = <&dsi1_out>;
+					};
+				};
+			};
+		};
+	};