Re: [[RFC]DPU PATCH] drm/bridge: add sn65dsi86 bridge driver support

2018-04-10 Thread Andrzej Hajda 
Hi,


On 09.04.2018 12:46, Sandeep Panda wrote:
> Add support for TI's sn65dsi86 dsi2edp bridge chip.
> The chip converts DSI transmitted signal to eDP signal,
> which is fed to the connected eDP panel.

Add info about control bus. It looks like the chip can be controlled via
i2c, dsi or both, am I right?
In such case it should be mentioned, and described which way of control
is implemented.

>
> Signed-off-by: Sandeep Panda 
> ---
>  .../bindings/display/bridge/ti,sn65dsi86.txt   |   53 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c  | 1146 
> 
>  2 files changed, 1199 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

Please split bindings and the driver into separate patches.

>
> diff --git 
> a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt 
> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> new file mode 100644
> index 000..5794770
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -0,0 +1,53 @@
> +SN65DSI86 DSItoeDP bridge chip
DSI to eDP
> +--
> +
> +This is the binding for Texas Instruments SN65DSI86 bridge.
> +
> +Required properties:
> +
> +- compatible: Must be "ti,sn65dsi86"
> +- reg:   Main I2C slave ID (for I2C host 
> driver)

reg: the I2C address of the chip, must be 0x28 or 0x29 (or whatever
datasheet says)

> +- sn,irq-gpio:   Main IRQ gpio mapping
> +- sn,enable-gpio Main reset gpio mapping

You can remove 'sn,' prefixes and should add 's' suffixes: irq-gpios,
enable-gpios.
Why are you using 'Main' everywhere? It does not look correct.
Please also rephrase descriptions, look at existing bindings, for
similar props.

Another thing is that you are using irq-gpio just to provide interrupt,
in such case please use
interrupts/interrupts-extended property.

> +
> +Required nodes:
> +
> +This device has two video ports. Their connections are modelled using the OF
> +graph bindings specified in Documentation/devicetree/bindings/graph.txt.
> +
> +- Video port 0 for DSI input
> +- Video port 1 for eDP output
> +
> +Example
> +---
> +
> +vga-bridge {

edp-bridge@39

> + compatible = "ti,sn65dsi86";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0x39>;
> +
> + sn,irq-gpio = < 32 0>;
> + sn,enable-gpio = < 33 0>;

Replace zeros with apropriate macros.

> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> +
> + vga_bridge_in: endpoint {
edp_bridge_in

> + remote-endpoint = <_out>;
> + };
> + };
> +
> + port@1 {
> + reg = <1>;
> +
> + vga_bridge_out: endpoint {
edp_bridge_out
> + remote-endpoint = <_in>;
> + };
> + };
> + };
> +}

The driver access multiple properties not defined in bindings, please
either remove them from the driver either add to bindings.
It is hard to guess if it is something to remove or just undocumented. I
see no point in detailed review of the driver until it will be clear.
Random things I have spotted looking at the code:
- please use gpiod API instead of obsolete one,
- with SPDX, license text is redundant,
- fortunately datasheet is available, please define and use macros
instead of magic numbers, for example in sn65dsi86_reg_cfg,

Regards
Andrzej

> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> new file mode 100644
> index 000..ff226b9
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,1146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +struct sn65dsi86_reg_cfg {
> + u8 reg;
> + u8 val;
> + int sleep_in_ms;
> +};
> +
> +struct

[[RFC]DPU PATCH] drm/bridge: add sn65dsi86 bridge driver support

2018-04-10 Thread Sandeep Panda 
Add support for TI's sn65dsi86 dsi2edp bridge chip.
The chip converts DSI transmitted signal to eDP signal,
which is fed to the connected eDP panel.

Signed-off-by: Sandeep Panda 
---
 .../bindings/display/bridge/ti,sn65dsi86.txt   |   53 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c  | 1146 
 2 files changed, 1199 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt 
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
new file mode 100644
index 000..5794770
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
@@ -0,0 +1,53 @@
+SN65DSI86 DSItoeDP bridge chip
+--
+
+This is the binding for Texas Instruments SN65DSI86 bridge.
+
+Required properties:
+
+- compatible: Must be "ti,sn65dsi86"
+- reg: Main I2C slave ID (for I2C host driver)
+- sn,irq-gpio: Main IRQ gpio mapping
+- sn,enable-gpio   Main reset gpio mapping
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using the OF
+graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for DSI input
+- Video port 1 for eDP output
+
+Example
+---
+
+vga-bridge {
+   compatible = "ti,sn65dsi86";
+   #address-cells = <1>;
+   #size-cells = <0>;
+   reg = <0x39>;
+
+   sn,irq-gpio = < 32 0>;
+   sn,enable-gpio = < 33 0>;
+
+   ports {
+   #address-cells = <1>;
+   #size-cells = <0>;
+
+   port@0 {
+   reg = <0>;
+
+   vga_bridge_in: endpoint {
+   remote-endpoint = <_out>;
+   };
+   };
+
+   port@1 {
+   reg = <1>;
+
+   vga_bridge_out: endpoint {
+   remote-endpoint = <_in>;
+   };
+   };
+   };
+}
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
new file mode 100644
index 000..ff226b9
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -0,0 +1,1146 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+struct sn65dsi86_reg_cfg {
+   u8 reg;
+   u8 val;
+   int sleep_in_ms;
+};
+
+struct sn65dsi86_video_cfg {
+   u32 h_active;
+   u32 h_front_porch;
+   u32 h_pulse_width;
+   u32 h_back_porch;
+   bool h_polarity;
+   u32 v_active;
+   u32 v_front_porch;
+   u32 v_pulse_width;
+   u32 v_back_porch;
+   bool v_polarity;
+   u32 pclk_khz;
+   u32 num_of_lanes;
+};
+
+struct sn65dsi86_gpios {
+   u32 irq_gpio;
+   u32 enable_gpio;
+   u32 i2c_sda;
+   u32 i2c_scl;
+   u32 panel_bias_en;
+   u32 panel_bklt_en;
+   u32 panel_bklt_ctrl;
+};
+
+struct sn65dsi86_reg_cfg reg_cfg_proto_0[] = {
+   {0x0A, 0x02, 0x0},  /* REFCLK 19.2MHz */
+   {0x10, 0x26, 0x14}, /* DSI lanes */
+   {0x12, 0x7B, 0x0},  /* DSIA CLK FREQ 309.03MHz */
+   {0x5A, 0x05, 0x0},  /* enhanced framing and ASSR */
+   {0x93, 0x30, 0x0},  /* 4 DP lanes no SSC */
+   {0x94, 0x80, 0x0},  /* HBR */
+   {0x0D, 0x01, 0x0},  /* PLL ENABLE */
+   {0x95, 0x00, 0x0},  /* POST-Cursor2 0dB */
+   {0x64, 0x01, 0x0},  /* WriteDPCD Register 0x0010A in Sink */
+   {0x74, 0x00, 0x0},
+   {0x75, 0x01, 0x0},
+   {0x76, 0x0A, 0x0},
+   {0x77, 0x01, 0x0},
+   {0x78, 0x81, 0x14},
+   {0x96, 0x0A, 0x14}, /* Semi-Auto TRAIN */
+   {0xFF, 0x14, 0x0},
+   {0x20, 0x70, 0x0},  /* CHA_ACTIVE_LINE_LENGTH */
+   {0x21, 0x08, 0x0},
+   {0x24, 0xA0, 0x0},  /* CHA_VERTICAL_DISPLAY_SIZE */
+   {0x25, 0x05, 0x0},
+   {0x2C, 0x20, 0x0},  /* CHA_HSYNC_PULSE_WIDTH */
+   {0x2D, 0x80, 0x0},
+   {0x30, 0x0A, 0x0},  /* CHA_VSYNC_PULSE_WIDTH */
+