On Mon, Oct 29, 2018 at 2:52 PM Maxime Ripard maxime.ripard@bootlin.com wrote:

On Fri, Oct 26, 2018 at 08:13:31PM +0530, Jagan Teki wrote:

...

Add 10 bytes packet overhead for hblk where blank is set using a blanking packet like (4 bytes + 4 bytes + payload + 2 bytes)

This is according to BSP code from BPI-M64-bsp (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c) dsi_hblk = (ht-hspw)*dsi_pixel_bits[format]/8-(4+4+2);

So, add 10 bytes packet overhead for DSI hblk.

Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 596e560263bf..cf42be1f1ba1 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -461,7 +461,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, { struct mipi_dsi_device *device = dsi->device; unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;

• u16 hbp, hfp, hsa, hblk, vblk;
  

• u16 hbp, hfp, hsa, hblk_max, hblk, vblk;
  size_t bytes;
  u8 *buffer;
  
  

@@ -494,8 +494,13 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,

  /*
   * hblk seems to be the line + porches length.

•  * The blank is set using a blanking packet (4 bytes + 4 bytes +
  

•  * payload + 2 bytes). So minimal size is 10 bytes
   */
  

• hblk = (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp;
  

+#define HBLK_PACKET_OVERHEAD 10

• hblk_max = (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp;
  

• hblk_max -= HBLK_PACKET_OVERHEAD;
  

• hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, hblk_max);
  

I'd rather use the same convention than the other assignments done before in that function.

Yes, old code did use hsa. but it's added 10 bytes packet head instead of subtracting it.

hblk = mode->htotal * Bpp - hsa; => mode->htotal * Bpp - (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD); => (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp + HSA_PACKET_OVERHEAD;

And it should be (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp - HSA_PACKET_OVERHEAD;

This patch is simply doing the same by explicitly adding packet over ahead macro, which again used in hfp.

On Fri, Oct 26, 2018 at 08:13:33PM +0530, Jagan Teki wrote:

Increase the hfp packet overhead with another 10 bytes, the extra 10 bytes(which is hblk packet overhead) is adding for hfp packet overhead since hfp depends on hblk.

This is truely as per BSP code from BPI-M64-bsp. The real computation from BSP is (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c) dsi_hbp = (hbp-hspw)*dsi_pixel_bits[format]/8 - (4+2); dsi_hact = x * dsi_pixel_bits[format]/8; dsi_hblk = (ht-hspw)*dsi_pixel_bits[format]/8-(4+4+2); dsi_hfp = dsi_hblk - (4+dsi_hact+2) - (4+dsi_hbp+2);

Example, u32 fmt = dsi_pixel_bits[format]/8; => ((ht-hspw)*fmt - 10) - (6 + x * fmt) - (6 + (hbp-hspw)*fmt - 6) => (ht - hspw - x - (hbp - hspw)) * fmt - 16 => (ht - x - hbp) * fmt - 16 => (ht - x - (timmings->hor_total_time - timmings->hor_front_porch - x)

• fmt - 16

=> (timmings->hor_total_time - x - timmings->hor_total_time + timmings->hor_front_porch + x) * fmt - 16 => timmings->hor_front_porch * fmt - 16

So, increase the DSI hfp packet overhead by hblk packet overhead.

Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 6584b51736a9..20e330186b7f 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -461,7 +461,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, { struct mipi_dsi_device *device = dsi->device; unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;

• u16 hbp, hfp, hsa, hblk_max, hblk, vblk;

• u16 hbp, hfp_pkt_overhead, hfp, hsa, hblk_max, hblk, vblk; size_t bytes; u8 *buffer;

@@ -484,14 +484,6 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, hbp = max((unsigned int)HBP_PACKET_OVERHEAD, (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);

• /*

• * The frontporch is set using a blanking packet (4 bytes +
  

• * payload + 2 bytes). Its minimal size is therefore 6 bytes
  

• */
  

-#define HFP_PACKET_OVERHEAD 6

• hfp = max((unsigned int)HFP_PACKET_OVERHEAD,

•   (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD);
  

• /*
  • hblk seems to be the line + porches length.
  • The blank is set using a blanking packet (4 bytes + 4 bytes +

@@ -502,6 +494,18 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, hblk_max -= HBLK_PACKET_OVERHEAD; hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, hblk_max);

• /*

• * The frontporch is set using a blanking packet (4 bytes +
  

• * payload + 2 bytes). Its minimal size is therefore 6 bytes
  

• *
  

• * According to BSP code, extra 10 bytes(which is hblk packet overhead)
  

• * is adding for hfp packet overhead since hfp depends on hblk.
  

"According to the BSP code, another 10 bytes (the HBLK packet overhead) need to be added to the front porch overhead."

How has this been verified? Since we have registers to set all the parameters, this seems kind of weird.

Maxime

On Mon, Oct 29, 2018 at 07:57:56PM +0530, Jagan Teki wrote:

On Mon, Oct 29, 2018 at 2:58 PM Maxime Ripard maxime.ripard@bootlin.com wrote:

...

On Fri, Oct 26, 2018 at 08:13:33PM +0530, Jagan Teki wrote:

...

Increase the hfp packet overhead with another 10 bytes, the extra 10 bytes(which is hblk packet overhead) is adding for hfp packet overhead since hfp depends on hblk.

This is truely as per BSP code from BPI-M64-bsp. The real computation from BSP is (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c) dsi_hbp = (hbp-hspw)*dsi_pixel_bits[format]/8 - (4+2); dsi_hact = x * dsi_pixel_bits[format]/8; dsi_hblk = (ht-hspw)*dsi_pixel_bits[format]/8-(4+4+2); dsi_hfp = dsi_hblk - (4+dsi_hact+2) - (4+dsi_hbp+2);

Example, u32 fmt = dsi_pixel_bits[format]/8; => ((ht-hspw)*fmt - 10) - (6 + x * fmt) - (6 + (hbp-hspw)*fmt - 6) => (ht - hspw - x - (hbp - hspw)) * fmt - 16 => (ht - x - hbp) * fmt - 16 => (ht - x - (timmings->hor_total_time - timmings->hor_front_porch - x)

• fmt - 16

=> (timmings->hor_total_time - x - timmings->hor_total_time + timmings->hor_front_porch + x) * fmt - 16 => timmings->hor_front_porch * fmt - 16

So, increase the DSI hfp packet overhead by hblk packet overhead.

Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 6584b51736a9..20e330186b7f 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -461,7 +461,7 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, { struct mipi_dsi_device *device = dsi->device; unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;

• u16 hbp, hfp, hsa, hblk_max, hblk, vblk;
  

• u16 hbp, hfp_pkt_overhead, hfp, hsa, hblk_max, hblk, vblk;
  size_t bytes;
  u8 *buffer;
  
  

@@ -484,14 +484,6 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, hbp = max((unsigned int)HBP_PACKET_OVERHEAD, (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);

• /*
  

•  * The frontporch is set using a blanking packet (4 bytes +
  

•  * payload + 2 bytes). Its minimal size is therefore 6 bytes
  

•  */
  

-#define HFP_PACKET_OVERHEAD 6

• hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
  

•           (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD);
  

• /*
   * hblk seems to be the line + porches length.
   * The blank is set using a blanking packet (4 bytes + 4 bytes +
  

@@ -502,6 +494,18 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, hblk_max -= HBLK_PACKET_OVERHEAD; hblk = max((unsigned int)HBLK_PACKET_OVERHEAD, hblk_max);

• /*
  

•  * The frontporch is set using a blanking packet (4 bytes +
  

•  * payload + 2 bytes). Its minimal size is therefore 6 bytes
  

•  *
  

•  * According to BSP code, extra 10 bytes(which is hblk packet overhead)
  

•  * is adding for hfp packet overhead since hfp depends on hblk.
  

"According to the BSP code, another 10 bytes (the HBLK packet overhead) need to be added to the front porch overhead."

How has this been verified? Since we have registers to set all the parameters, this seems kind of weird.

Verified based on the BSP calculation which I explained in commit message and tested by validating these numbers with mainline vs bsp code.

So, not verified?

Did you have a panel that was broken due to this? Have you found that the timings output on the DSI bus were wrong using an oscilloscope or an analyzer? What makes you say that the Allwinner code is more correct than we are?

Maxime

On Fri, Oct 26, 2018 at 08:13:34PM +0530, Jagan Teki wrote:

Unlike hblk, the vblk timings should follow an equation to compute the desired value for lane 4 devices and rest of devices it would be 0.

BSP code from BPI-M64-bsp is computing vblk as for 4-lane devices (in drivers/video/sunxi/disp2/disp/de/lowlevel_sun50iw1/de_dsi.c) tmp = (ht*dsi_pixel_bits[format]/8)*vt-(4+dsi_hblk+2); dsi_vblk = (lane-tmp%lane);

So, update the vblk timing calculation accordingly.

Tested on 2-lane, 4-lane MIPI-DSI LCD panels.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 20e330186b7f..42bd7506abaf 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -511,8 +511,19 @@ static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi, * Allwinner BSP is using a rather convoluted calculation * there only for 4 lanes. However, using 0 (the !4 lanes * case) even with a 4 lanes screen seems to work...

Since you're adding the support for 2 lanes DSI displays, that should be removed.

• *
  

• * The vertical blank is set using a blanking packet (4 bytes +
  

• * payload + 2 bytes). Its minimal size is therefore 6 bytes
  

  */

• vblk = 0;

+#define VBLK_PACKET_OVERHEAD 6

• if (device->lanes == 4) {

• int tmp;
  

• tmp = (mode->htotal * Bpp) * mode->vtotal - (hblk + VBLK_PACKET_OVERHEAD);
  

• vblk =(device->lanes - tmp % device->lanes);
  

This generates a checkpatch warning. You should run all your patches through checkpatch before sending them.

Maxime

On Fri, Oct 26, 2018 at 08:13:35PM +0530, Jagan Teki wrote:

Some boards have VCC-DSI pin connected to voltage regulator which may not be turned on by default.

Add support for such boards by adding voltage regulator handling code to MIPI DSI driver.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 14 ++++++++++++++ drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 3 +++ 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 42bd7506abaf..bc57343592e0 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -949,6 +949,12 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,

dsi->drv = drv;

• ret = regulator_enable(dsi->regulator);
• if (ret) {

• dev_err(dev, "Failed to enable regulator\n");
  

• return ret;
  

• }

The regulator should be enabled only when the device is in use.

drm_encoder_helper_add(&dsi->encoder, &sun6i_dsi_enc_helper_funcs); ret = drm_encoder_init(drm, @@ -980,6 +986,7 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,

err_cleanup_connector: drm_encoder_cleanup(&dsi->encoder);

• regulator_disable(dsi->regulator); return ret;

}

@@ -989,6 +996,7 @@ static void sun6i_dsi_unbind(struct device *dev, struct device *master, struct sun6i_dsi *dsi = dev_get_drvdata(dev);

drm_panel_detach(dsi->panel);

• regulator_disable(dsi->regulator);

}

static const struct component_ops sun6i_dsi_ops = { @@ -1022,6 +1030,12 @@ static int sun6i_dsi_probe(struct platform_device *pdev) return PTR_ERR(base); }

• dsi->regulator = devm_regulator_get(dev, "vcc-dsi");
• if (IS_ERR(dsi->regulator)) {

• dev_err(dev, "Couldn't get regulator\n");
  

• return PTR_ERR(dsi->regulator);
  

• }

You haven't updated the DT bindings accordingly.

Maxime

On Mon, Oct 29, 2018 at 08:18:05PM +0530, Jagan Teki wrote:

On Mon, Oct 29, 2018 at 3:01 PM Maxime Ripard maxime.ripard@bootlin.com wrote:

...

On Fri, Oct 26, 2018 at 08:13:35PM +0530, Jagan Teki wrote:

...

Some boards have VCC-DSI pin connected to voltage regulator which may not be turned on by default.

Add support for such boards by adding voltage regulator handling code to MIPI DSI driver.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 14 ++++++++++++++ drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 3 +++ 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index 42bd7506abaf..bc57343592e0 100644 --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c @@ -949,6 +949,12 @@ static int sun6i_dsi_bind(struct device *dev, struct device *master,

  dsi->drv = drv;

• ret = regulator_enable(dsi->regulator);
  

• if (ret) {
  

•         dev_err(dev, "Failed to enable regulator\n");
  

•         return ret;
  

• }
  

The regulator should be enabled only when the device is in use.

True, but there is no device specific quirk needed because who ever not needed simply use dummy regulator [ 0.245821] sun6i-mipi-dsi 1ca0000.dsi: 1ca0000.dsi supply vcc-dsi not found, using dummy regulato r

even like HVCC in hdmi driver [1]

[1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/d...

That's not my point. You should enable the regulator only when the DSI bridge is being enabled, not when the driver has probed.

Maxime

On 26.10.2018 16:43, Jagan Teki wrote:

Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB bridge panel, which is available on same PCB with 24-bit RGB interface.

So, this patch adds DSI specific binding details on existing dt-bindings file.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

Changes for v3:

• Use existing binding doc and update dsi details

Changes for v2:

• none

.../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt index 35bc0c839f49..b7855dc7c66f 100644 --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt @@ -1,12 +1,39 @@ Banana Pi 7" (S070WV20-CT16) TFT LCD Panel

+S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.

+Depending on the variant, the PCB attached to the panel module either +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel +itself

As I understand this is display board, which contains 'pure' RGB panel S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. These are separate devices, just connected by vendor to simplify its assembly. Why don't you create then bridge driver for ICN6211 and RGB panel driver for S070WV20-CT16 - it looks more generic. Then you can describe both in dts and voila. Creating drivers for every combo of devices (panel + bridge), just because some vendor sells them together seems incorrect - we have devicetree for it.

Regards Andrzej

Required properties: -- compatible: should be "bananapi,s070wv20-ct16" +- compatible:

• for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
• for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"

+Required properties for RGB:

• power-supply: see ./panel-common.txt

+Required properties for MIPI-DSI to RGB: +- reg: for DSI virtual channel used by that screen +- avdd-supply: analog regulator dc1 switch +- dvdd-supply: 3v3 digital regulator +- reset-gpios: a GPIO phandle for the reset pin

Optional properties: -- enable-gpios: see ./simple-panel.txt +- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)

• backlight: see ./simple-panel.txt

This binding is compatible with the simple-panel binding, which is specified in ./simple-panel.txt.

+Example: +panel@0 {

• compatible = "bananapi,s070wv20-ct16-icn6211";
• reg = <0>;
• avdd-supply = <&reg_dc1sw>;
• dvdd-supply = <&reg_dldo1>;
• reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
• backlight = <&backlight_dsi>;

+};

On 31.10.2018 09:58, Chen-Yu Tsai wrote:

On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 26.10.2018 16:43, Jagan Teki wrote:

...

Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB bridge panel, which is available on same PCB with 24-bit RGB interface.

So, this patch adds DSI specific binding details on existing dt-bindings file.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

Changes for v3:

• Use existing binding doc and update dsi details

Changes for v2:

• none

.../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt index 35bc0c839f49..b7855dc7c66f 100644 --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt @@ -1,12 +1,39 @@ Banana Pi 7" (S070WV20-CT16) TFT LCD Panel

+S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.

+Depending on the variant, the PCB attached to the panel module either +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel +itself

As I understand this is display board, which contains 'pure' RGB panel S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. These are separate devices, just connected by vendor to simplify its assembly. Why don't you create then bridge driver for ICN6211 and RGB panel driver for S070WV20-CT16 - it looks more generic. Then you can describe both in dts and voila. Creating drivers for every combo of devices (panel + bridge), just because some vendor sells them together seems incorrect - we have devicetree for it.

Rob suggested this, and also the opposite: using the same "bananapi,s070wv20-ct16" compatible string for both types of connections, and have the driver deal with detecting the bus type.

The thing about the bridge chip is that there's no available datasheet that describes all the parts of the init sequence, in fact none at all. I managed to work out some bits, but the others remain a mystery and must be hard-coded to match the panel. That would work against having a generic bridge driver.

But it is common for many chips - 1st version of the driver is developed on one platform and it supports only one configuration, if next platform with the same cheap appears the driver is augmented if necessary.

Regards

Andrzej

ChenYu

...

Regards Andrzej

...

Required properties: -- compatible: should be "bananapi,s070wv20-ct16" +- compatible:

• for 24-bit RGB interface, use "bananapi,s070wv20-ct16"
• for ICN6211 MIPI-DSI to RGB bridge, use "bananapi,s070wv20-ct16-icn6211"

+Required properties for RGB:

• power-supply: see ./panel-common.txt

+Required properties for MIPI-DSI to RGB: +- reg: for DSI virtual channel used by that screen +- avdd-supply: analog regulator dc1 switch +- dvdd-supply: 3v3 digital regulator +- reset-gpios: a GPIO phandle for the reset pin

Optional properties: -- enable-gpios: see ./simple-panel.txt +- enable-gpios: see ./simple-panel.txt(not available in MIPI-DSI to RGB bridge)

• backlight: see ./simple-panel.txt

This binding is compatible with the simple-panel binding, which is specified in ./simple-panel.txt.

+Example: +panel@0 {

• compatible = "bananapi,s070wv20-ct16-icn6211";
  

• reg = <0>;
  

• avdd-supply = <&reg_dc1sw>;
  

• dvdd-supply = <&reg_dldo1>;
  

• reset-gpios = <&pio 3 6 GPIO_ACTIVE_HIGH>; /* PD6 */
  

• backlight = <&backlight_dsi>;
  

+};

On 06.11.2018 19:08, Jagan Teki wrote:

On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 31.10.2018 09:58, Chen-Yu Tsai wrote:

...

On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 26.10.2018 16:43, Jagan Teki wrote:

...

Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB bridge panel, which is available on same PCB with 24-bit RGB interface.

So, this patch adds DSI specific binding details on existing dt-bindings file.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

Changes for v3:

• Use existing binding doc and update dsi details

Changes for v2:

• none

.../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt index 35bc0c839f49..b7855dc7c66f 100644 --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt @@ -1,12 +1,39 @@ Banana Pi 7" (S070WV20-CT16) TFT LCD Panel

+S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.

+Depending on the variant, the PCB attached to the panel module either +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel +itself

As I understand this is display board, which contains 'pure' RGB panel S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. These are separate devices, just connected by vendor to simplify its assembly. Why don't you create then bridge driver for ICN6211 and RGB panel driver for S070WV20-CT16 - it looks more generic. Then you can describe both in dts and voila. Creating drivers for every combo of devices (panel + bridge), just because some vendor sells them together seems incorrect - we have devicetree for it.

Rob suggested this, and also the opposite: using the same "bananapi,s070wv20-ct16" compatible string for both types of connections, and have the driver deal with detecting the bus type.

The thing about the bridge chip is that there's no available datasheet that describes all the parts of the init sequence, in fact none at all. I managed to work out some bits, but the others remain a mystery and must be hard-coded to match the panel. That would work against having a generic bridge driver.

But it is common for many chips - 1st version of the driver is developed on one platform and it supports only one configuration, if next platform with the same cheap appears the driver is augmented if necessary.

At-least few of the commands from panel initialization code, the respective opcode data values are based on panel timings and even clock value is different in DSI. I think it look hard to try bridge driver for these restrictions, do you have any suggestions?

Where do you see an issue? Since panel is RGB it should have no initialization sequence (beside regulator/gpio power on/off), so the only thing to do is to figure out which regulators/gpios belongs to which component - with publicly available specs it should be doable.

The whole initialization sequence is for the bridge, so you put it into bridge driver, for starters it can be hardcoded.

Then you can:

1. Try to find other users of this ICN6211 chip and compare initialization sequences to guess purpose of registers.

2. Try to get specs of the chip (ask vendor, distributor, grep Internet).

3. Do nothing - if there will be other users of the bridge they will do this work.

Regards

Andrzej

On 10.11.2018 08:32, Jagan Teki wrote:

On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 06.11.2018 19:08, Jagan Teki wrote:

...

On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 31.10.2018 09:58, Chen-Yu Tsai wrote:

...

On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 26.10.2018 16:43, Jagan Teki wrote: > Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB > bridge panel, which is available on same PCB with 24-bit RGB interface. > > So, this patch adds DSI specific binding details on existing > dt-bindings file. > > Signed-off-by: Jagan Teki jagan@amarulasolutions.com > --- > Changes for v3: > - Use existing binding doc and update dsi details > Changes for v2: > - none > > .../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt > index 35bc0c839f49..b7855dc7c66f 100644 > --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt > +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt > @@ -1,12 +1,39 @@ > Banana Pi 7" (S070WV20-CT16) TFT LCD Panel > > +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface. > + > +Depending on the variant, the PCB attached to the panel module either > +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via > +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel > +itself As I understand this is display board, which contains 'pure' RGB panel S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. These are separate devices, just connected by vendor to simplify its assembly. Why don't you create then bridge driver for ICN6211 and RGB panel driver for S070WV20-CT16 - it looks more generic. Then you can describe both in dts and voila. Creating drivers for every combo of devices (panel + bridge), just because some vendor sells them together seems incorrect - we have devicetree for it.

Rob suggested this, and also the opposite: using the same "bananapi,s070wv20-ct16" compatible string for both types of connections, and have the driver deal with detecting the bus type.

The thing about the bridge chip is that there's no available datasheet that describes all the parts of the init sequence, in fact none at all. I managed to work out some bits, but the others remain a mystery and must be hard-coded to match the panel. That would work against having a generic bridge driver.

But it is common for many chips - 1st version of the driver is developed on one platform and it supports only one configuration, if next platform with the same cheap appears the driver is augmented if necessary.

At-least few of the commands from panel initialization code, the respective opcode data values are based on panel timings and even clock value is different in DSI. I think it look hard to try bridge driver for these restrictions, do you have any suggestions?

Where do you see an issue? Since panel is RGB it should have no initialization sequence (beside regulator/gpio power on/off), so the only thing to do is to figure out which regulators/gpios belongs to which component - with publicly available specs it should be doable.

The whole initialization sequence is for the bridge, so you put it into bridge driver, for starters it can be hardcoded.

Yes, I understand we can move regulators/gpio setup separately and though we hardcode the init sequence there is difference in clock for DSI(which I mentioned in previous mail). DSI panel can't work with clock used by RGB panel-simple.

If you mean pixel clock from timings in next patch it seems incorrect. Pixel clock should be always

htotal * vtotal * vrefresh, in case of drm_display_mode result should be divided by 1000 (as .clock is in kHz).

With timings provided there you have: 928*525*60 = 29232000

So pixel clock should be 29232, if other timings are correct. DSI clock is a different thing and it is private thing of DSI bridge/panel it should not be exposed via drm_display_mode.

Regards

Andrzej

...

Then you can:

1. Try to find other users of this ICN6211 chip and compare

initialization sequences to guess purpose of registers.

2. Try to get specs of the chip (ask vendor, distributor, grep Internet).

As we mentioned (even Chen-Yu), we are unable to find the proper spec for this panel, all we taken reference from AW BSP code.

...

3. Do nothing - if there will be other users of the bridge they will do

this work.

Don't know how we can go with generic bridge driver irrespective of these particular wrinkles, let me know if you have any suggestions.

On 18.11.2018 19:20, Jagan Teki wrote:

On Tue, Nov 13, 2018 at 1:26 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 10.11.2018 08:32, Jagan Teki wrote:

...

On Wed, Nov 7, 2018 at 2:41 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 06.11.2018 19:08, Jagan Teki wrote:

...

On Wed, Oct 31, 2018 at 2:45 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 31.10.2018 09:58, Chen-Yu Tsai wrote: > On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda a.hajda@samsung.com wrote: >> On 26.10.2018 16:43, Jagan Teki wrote: >>> Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB >>> bridge panel, which is available on same PCB with 24-bit RGB interface. >>> >>> So, this patch adds DSI specific binding details on existing >>> dt-bindings file. >>> >>> Signed-off-by: Jagan Teki jagan@amarulasolutions.com >>> --- >>> Changes for v3: >>> - Use existing binding doc and update dsi details >>> Changes for v2: >>> - none >>> >>> .../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- >>> 1 file changed, 29 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt >>> index 35bc0c839f49..b7855dc7c66f 100644 >>> --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt >>> +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt >>> @@ -1,12 +1,39 @@ >>> Banana Pi 7" (S070WV20-CT16) TFT LCD Panel >>> >>> +S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface. >>> + >>> +Depending on the variant, the PCB attached to the panel module either >>> +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via >>> +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel >>> +itself >> As I understand this is display board, which contains 'pure' RGB panel >> S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. >> These are separate devices, just connected by vendor to simplify its >> assembly. Why don't you create then bridge driver for ICN6211 and RGB >> panel driver for S070WV20-CT16 - it looks more generic. >> Then you can describe both in dts and voila. >> Creating drivers for every combo of devices (panel + bridge), just >> because some vendor sells them together seems incorrect - we have >> devicetree for it. > Rob suggested this, and also the opposite: using the same > "bananapi,s070wv20-ct16" > compatible string for both types of connections, and have the driver deal with > detecting the bus type. > > The thing about the bridge chip is that there's no available datasheet that > describes all the parts of the init sequence, in fact none at all. I managed > to work out some bits, but the others remain a mystery and must be hard-coded > to match the panel. That would work against having a generic bridge driver. But it is common for many chips - 1st version of the driver is developed on one platform and it supports only one configuration, if next platform with the same cheap appears the driver is augmented if necessary.

At-least few of the commands from panel initialization code, the respective opcode data values are based on panel timings and even clock value is different in DSI. I think it look hard to try bridge driver for these restrictions, do you have any suggestions?

Where do you see an issue? Since panel is RGB it should have no initialization sequence (beside regulator/gpio power on/off), so the only thing to do is to figure out which regulators/gpios belongs to which component - with publicly available specs it should be doable.

The whole initialization sequence is for the bridge, so you put it into bridge driver, for starters it can be hardcoded.

Yes, I understand we can move regulators/gpio setup separately and though we hardcode the init sequence there is difference in clock for DSI(which I mentioned in previous mail). DSI panel can't work with clock used by RGB panel-simple.

If you mean pixel clock from timings in next patch it seems incorrect. Pixel clock should be always

htotal * vtotal * vrefresh, in case of drm_display_mode result should be divided by 1000 (as .clock is in kHz).

With timings provided there you have: 928*525*60 = 29232000

So pixel clock should be 29232, if other timings are correct. DSI clock is a different thing and it is private thing of DSI bridge/panel it should not be exposed via drm_display_mode.

I understand your point, but in Allwinner case DSI clock which is named in tcon block can be computed using clock value from drm_display_mode from panel driver.

OK, it sounds like me telling you that 2+2=4 and you answering that you understand my point but in your case 2+2=5 fits better.

This is nothing about 'my point', this is simple fact that if you have 928*525 pixels (including hidden ones) with refresh rate 60Hz pixelclock is 29232kHz.

DE -> Mixer -> tcon-> MIPI <--> panel

So, for the value

• 55MHz panel clock the computed divider for DSI clock in tcon [1] is

6 which is proper and working divider

• 30MHz panel clock the computed divider for DSI clock in tcon [2] is

11 which can't work for this specific panel.

Please explain what exactly means "panel clock" and the divider here.

What rate reports modetest -v ...? For this and other panels.

Maybe we can figure out what should be fixed/adjusted.

Regards

Andrzej

I have verified other panels, the divider value work similar to this computation and those panels work accordingly. but this panel case the divider computation look weird, so we have used working and know clock. I'm thinking since this panel is of DSI to RGB bridge, so it may require some higher frequency to work but I've no document to confirm the same.

Hope this information help, let me know if you have any inputs.

[1] https://paste.ubuntu.com/p/drvzfHFMtY/ [2] https://paste.ubuntu.com/p/hz29CTJY2J/

On Wed, Oct 31, 2018 at 2:46 PM Julian Calaby julian.calaby@gmail.com wrote:

Hi Jagan,

On Wed, Oct 31, 2018 at 7:58 PM Chen-Yu Tsai wens@csie.org wrote:

...

On Wed, Oct 31, 2018 at 4:53 PM Andrzej Hajda a.hajda@samsung.com wrote:

...

On 26.10.2018 16:43, Jagan Teki wrote:

...

Bananapi S070WV20-CT16 ICN6211 is 800x480, 4-lane MIPI-DSI to RGB bridge panel, which is available on same PCB with 24-bit RGB interface.

So, this patch adds DSI specific binding details on existing dt-bindings file.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

Changes for v3:

• Use existing binding doc and update dsi details

Changes for v2:

• none

.../display/panel/bananapi,s070wv20-ct16.txt | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt index 35bc0c839f49..b7855dc7c66f 100644 --- a/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt +++ b/Documentation/devicetree/bindings/display/panel/bananapi,s070wv20-ct16.txt @@ -1,12 +1,39 @@ Banana Pi 7" (S070WV20-CT16) TFT LCD Panel

+S070WV20-CT16 is 7" 800x480 panel connected through a 24-bit RGB interface.

+Depending on the variant, the PCB attached to the panel module either +supports DSI, or DSI + 24-bit RGB. DSI is converted to 24-bit RGB via +an onboard ICN6211 MIPI DSI - RGB bridge chip, then fed to the panel +itself

As I understand this is display board, which contains 'pure' RGB panel S070WV20-CT16 and optionally ICN6211 DSI->RGB bridge. These are separate devices, just connected by vendor to simplify its assembly. Why don't you create then bridge driver for ICN6211 and RGB panel driver for S070WV20-CT16 - it looks more generic. Then you can describe both in dts and voila. Creating drivers for every combo of devices (panel + bridge), just because some vendor sells them together seems incorrect - we have devicetree for it.

Rob suggested this, and also the opposite: using the same "bananapi,s070wv20-ct16" compatible string for both types of connections, and have the driver deal with detecting the bus type.

The thing about the bridge chip is that there's no available datasheet that describes all the parts of the init sequence, in fact none at all. I managed to work out some bits, but the others remain a mystery and must be hard-coded to match the panel. That would work against having a generic bridge driver.

To me it seems logical that we'd model it as another step in the graph between the DSI component and the panel. It's conceivable that some other manufacturer will probably buy these for their panels and having a somewhat generic driver seems vaguely future proof to me.

As I see it, the weird init process belongs to the bridge chip and the timings belong to the panel and we shouldn't "confuse" them by giving them the same compatible.

But the problem here is due to lack of information about the panel, the init process command opcode data values seem to based on the panel timings values. This look weird and ie reason for going into separate panel driver with different compatible.

On Fri, Oct 26, 2018 at 08:13:39PM +0530, Jagan Teki wrote:

Techstar TS8550B MIPI DSI panel is 480x854, 2-lane MIPI DSI LCD panel. Add panel driver for it.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile | 1 + .../gpu/drm/panel/panel-techstar-ts8550b.c | 346 ++++++++++++++++++ 3 files changed, 356 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-techstar-ts8550b.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 20b88c275421..d0d4e60f5153 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -195,4 +195,13 @@ config DRM_PANEL_SITRONIX_ST7789V Say Y here if you want to enable support for the Sitronix ST7789V controller for 240x320 LCD panels

+config DRM_PANEL_TECHSTAR_TS8550B

• tristate "Techstar TS8550B MIPI-DSI panel driver"
• depends on OF
• depends on DRM_MIPI_DSI
• depends on BACKLIGHT_CLASS_DEVICE
• help

•  Say Y if you want to enable support for panels based on the
  

•  Techstar TS8550B MIPI-DSI interface.
  

endmenu diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 04696bb85218..88011f06edb8 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -20,3 +20,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o +obj-$(CONFIG_DRM_PANEL_TECHSTAR_TS8550B) += panel-techstar-ts8550b.o diff --git a/drivers/gpu/drm/panel/panel-techstar-ts8550b.c b/drivers/gpu/drm/panel/panel-techstar-ts8550b.c new file mode 100644 index 000000000000..8baca71595a7 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-techstar-ts8550b.c @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2019, Amarula Solutions.
• • Author: Jagan Teki jagan@amarulasolutions.com
• */

+#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/fb.h> +#include <linux/kernel.h> +#include <linux/module.h>

+#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>

+#include <drm/drm_mipi_dsi.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h>

+#include <video/mipi_display.h>

+struct ts8550b {

• struct drm_panel panel;
• struct mipi_dsi_device *dsi;
• struct backlight_device *backlight;
• struct regulator *dvdd;
• struct regulator *avdd;
• struct gpio_desc *reset;
• bool is_enabled;
• bool is_prepared;

+};

+static inline struct ts8550b *panel_to_ts8550b(struct drm_panel *panel) +{

• return container_of(panel, struct ts8550b, panel);

+}

+static inline int ts8550b_dcs_write_seq(struct ts8550b *ctx, const void *seq,

• 			size_t len)
  

+{

• return mipi_dsi_dcs_write_buffer(ctx->dsi, seq, len);

+};

+#define ts8550b_dcs_write_seq_static(ctx, seq...) \

• ({ \

• static const u8 d[] = { seq };			\
  

• ts8550b_dcs_write_seq(ctx, d, ARRAY_SIZE(d));	\
  

• })

+static void ts8550b_init_sequence(struct ts8550b *ctx)

This seems to be pretty "standard" MIPI-based display, utilizing bunch of manufacturer-specific commands during init sequence.

Standard MIPI commands fall between 0x00..0xAF and are defined in <video/mipi.h>

+{

• ts8550b_dcs_write_seq_static(ctx, 0x01, 0x00);

MIPI_DCS_SOFT_RESET

• msleep(200);
• ts8550b_dcs_write_seq_static(ctx, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x11);
• ts8550b_dcs_write_seq_static(ctx, 0xD1, 0x11);
• ts8550b_dcs_write_seq_static(ctx, 0x11, 0x00);

MIPI_DCS_EXIT_SLEEP_MODE

• msleep(200);
• ts8550b_dcs_write_seq_static(ctx, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x10);
• ts8550b_dcs_write_seq_static(ctx, 0xC0, 0xE9, 0x03);
• ts8550b_dcs_write_seq_static(ctx, 0xC1, 0x12, 0x02);
• ts8550b_dcs_write_seq_static(ctx, 0xC2, 0x07, 0x06);
• ts8550b_dcs_write_seq_static(ctx, 0xB0, 0x00, 0x0E, 0x15, 0x0F, 0x11,

• 		     0x08, 0x08, 0x08, 0x08, 0x23, 0x04, 0x13,
  

• 		     0x12, 0x2B, 0x34, 0x1F);
  

• ts8550b_dcs_write_seq_static(ctx, 0xB1, 0x00, 0x0E, 0x95, 0x0F, 0x13,

• 		     0x07, 0x09, 0x08, 0x08, 0x22, 0x04, 0x10,
  

• 		     0x0E, 0x2C, 0x34, 0x1F);
  

• ts8550b_dcs_write_seq_static(ctx, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x11);
• ts8550b_dcs_write_seq_static(ctx, 0xB0, 0x45);
• ts8550b_dcs_write_seq_static(ctx, 0xB1, 0x13);
• ts8550b_dcs_write_seq_static(ctx, 0xB2, 0x07);
• ts8550b_dcs_write_seq_static(ctx, 0xB3, 0x80);
• ts8550b_dcs_write_seq_static(ctx, 0xB5, 0x47);
• ts8550b_dcs_write_seq_static(ctx, 0xB7, 0x85);
• ts8550b_dcs_write_seq_static(ctx, 0xB8, 0x20);
• ts8550b_dcs_write_seq_static(ctx, 0xB9, 0x11);
• ts8550b_dcs_write_seq_static(ctx, 0xC1, 0x78);
• ts8550b_dcs_write_seq_static(ctx, 0xC2, 0x78);
• ts8550b_dcs_write_seq_static(ctx, 0xD0, 0x88);
• msleep(100);
• ts8550b_dcs_write_seq_static(ctx, 0xE0, 0x00, 0x00, 0x02);
• ts8550b_dcs_write_seq_static(ctx, 0xE1, 0x0B, 0x00, 0x0D, 0x00, 0x0C,

• 		     0x00, 0x0E, 0x00, 0x00, 0x44, 0x44);
  

• ts8550b_dcs_write_seq_static(ctx, 0xE2, 0x33, 0x33, 0x44, 0x44, 0x64,

• 		     0x00, 0x66, 0x00, 0x65, 0x00, 0x67, 0x00,
  

• 		     0x00);
  

• ts8550b_dcs_write_seq_static(ctx, 0xE3, 0x00, 0x00, 0x33, 0x33);
• ts8550b_dcs_write_seq_static(ctx, 0xE4, 0x44, 0x44);
• ts8550b_dcs_write_seq_static(ctx, 0xE5, 0x0C, 0x78, 0x3C, 0xA0, 0x0E,

• 		     0x78, 0x3C, 0xA0, 0x10, 0x78, 0x3C, 0xA0,
  

• 		     0x12, 0x78, 0x3C, 0xA0);
  

• ts8550b_dcs_write_seq_static(ctx, 0xE6, 0x00, 0x00, 0x33, 0x33);
• ts8550b_dcs_write_seq_static(ctx, 0xE7, 0x44, 0x44);
• ts8550b_dcs_write_seq_static(ctx, 0xE8, 0x0D, 0x78, 0x3C, 0xA0, 0x0F,

• 		     0x78, 0x3C, 0xA0, 0x11, 0x78, 0x3C, 0xA0,
  

• 		     0x13, 0x78, 0x3C, 0xA0);
  

• ts8550b_dcs_write_seq_static(ctx, 0xEB, 0x02, 0x02, 0x39, 0x39, 0xEE,

• 		     0x44, 0x00);
  

• ts8550b_dcs_write_seq_static(ctx, 0xEC, 0x00, 0x00);
• ts8550b_dcs_write_seq_static(ctx, 0xED, 0xFF, 0xF1, 0x04, 0x56, 0x72,

• 		     0x3F, 0xFF, 0xFF, 0xFF, 0xFF, 0xF3, 0x27,
  

• 		     0x65, 0x40, 0x1F, 0xFF);
  

• ts8550b_dcs_write_seq_static(ctx, 0xFF, 0x77, 0x01, 0x00, 0x00, 0x00);
• msleep(10);
• ts8550b_dcs_write_seq_static(ctx, 0x29, 0x00);

MIPI_DCS_SET_DISPLAY_ON

• msleep(200);

+}

+static int ts8550b_prepare(struct drm_panel *panel) +{

• struct ts8550b *ctx = panel_to_ts8550b(panel);
• int ret;
• if (ctx->is_prepared)

• return 0;
  

• gpiod_set_value(ctx->reset, 0);
• msleep(20);
• ret = regulator_enable(ctx->dvdd);
• if (ret)

• return ret;
  

• msleep(20);
• ret = regulator_enable(ctx->avdd);
• if (ret)

• return ret;
  

• msleep(20);
• gpiod_set_value(ctx->reset, 1);
• msleep(20);
• gpiod_set_value(ctx->reset, 0);
• msleep(30);
• gpiod_set_value(ctx->reset, 1);
• msleep(150);
• ts8550b_init_sequence(ctx);
• ctx->is_prepared = true;
• return 0;

+}

+static int ts8550b_enable(struct drm_panel *panel) +{

• struct ts8550b *ctx = panel_to_ts8550b(panel);
• if (ctx->is_enabled)

• return 0;
  

• msleep(120);
• backlight_enable(ctx->backlight);
• ctx->is_enabled = true;
• return 0;

+}

+static int ts8550b_disable(struct drm_panel *panel) +{

• struct ts8550b *ctx = panel_to_ts8550b(panel);
• if (!ctx->is_enabled)

• return 0;
  

• backlight_disable(ctx->backlight);
• ctx->is_enabled = false;
• return 0;

+}

+static int ts8550b_unprepare(struct drm_panel *panel) +{

• struct ts8550b *ctx = panel_to_ts8550b(panel);
• int ret;
• if (!ctx->is_prepared)

• return 0;
  

• ret = mipi_dsi_dcs_set_display_off(ctx->dsi);
• if (ret < 0)

• dev_err(panel->dev, "failed to set display off: %d\n", ret);
  

• ret = mipi_dsi_dcs_enter_sleep_mode(ctx->dsi);
• if (ret < 0)

• dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
  

• msleep(120);
• regulator_disable(ctx->dvdd);
• regulator_disable(ctx->avdd);
• gpiod_set_value(ctx->reset, 0);
• gpiod_set_value(ctx->reset, 1);
• gpiod_set_value(ctx->reset, 0);
• ctx->is_prepared = false;
• return 0;

+}

+static const struct drm_display_mode default_mode = {

• .clock = 27500,
• .vrefresh = 60,
• .hdisplay = 480,
• .hsync_start = 480 + 38,
• .hsync_end = 480 + 38 + 12,
• .htotal = 480 + 38 + 12 + 12,
• .vdisplay = 854,
• .vsync_start = 854 + 18,
• .vsync_end = 854 + 18 + 8,
• .vtotal = 854 + 18 + 8 + 4,

+};

+static int ts8550b_get_modes(struct drm_panel *panel) +{

• struct drm_connector *connector = panel->connector;
• struct ts8550b *ctx = panel_to_ts8550b(panel);
• struct drm_display_mode *mode;
• mode = drm_mode_duplicate(panel->drm, &default_mode);
• if (!mode) {

• dev_err(&ctx->dsi->dev, "failed to add mode %ux%ux@%u\n",
  

• 	default_mode.hdisplay, default_mode.vdisplay,
  

• 	default_mode.vrefresh);
  

• return -ENOMEM;
  

• }
• drm_mode_set_name(mode);
• mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
• drm_mode_probed_add(connector, mode);
• panel->connector->display_info.width_mm = 69;
• panel->connector->display_info.height_mm = 139;
• return 1;

+}

+static const struct drm_panel_funcs ts8550b_funcs = {

• .disable = ts8550b_disable,
• .unprepare = ts8550b_unprepare,
• .prepare = ts8550b_prepare,
• .enable = ts8550b_enable,
• .get_modes = ts8550b_get_modes,

+};

+static int ts8550b_dsi_probe(struct mipi_dsi_device *dsi) +{

• struct device_node *np;
• struct ts8550b *ctx;
• int ret;
• ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
• if (!ctx)

• return -ENOMEM;
  

• mipi_dsi_set_drvdata(dsi, ctx);
• ctx->dsi = dsi;
• drm_panel_init(&ctx->panel);
• ctx->panel.dev = &dsi->dev;
• ctx->panel.funcs = &ts8550b_funcs;
• ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
• if (IS_ERR(ctx->dvdd)) {

• dev_err(&dsi->dev, "Couldn't get dvdd regulator\n");
  

• return PTR_ERR(ctx->dvdd);
  

• }
• ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
• if (IS_ERR(ctx->avdd)) {

• dev_err(&dsi->dev, "Couldn't get avdd regulator\n");
  

• return PTR_ERR(ctx->avdd);
  

• }
• ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
• if (IS_ERR(ctx->reset)) {

• dev_err(&dsi->dev, "Couldn't get our reset GPIO\n");
  

• return PTR_ERR(ctx->reset);
  

• }
• np = of_parse_phandle(dsi->dev.of_node, "backlight", 0);
• if (np) {

• ctx->backlight = of_find_backlight_by_node(np);
  

• of_node_put(np);
  

• if (!ctx->backlight)
  

• 	return -EPROBE_DEFER;
  

• }
• ret = drm_panel_add(&ctx->panel);
• if (ret < 0)

• return ret;
  

• dsi->mode_flags = MIPI_DSI_MODE_VIDEO;
• dsi->format = MIPI_DSI_FMT_RGB888;
• dsi->lanes = 2;
• return mipi_dsi_attach(dsi);

+}

+static int ts8550b_dsi_remove(struct mipi_dsi_device *dsi) +{

• struct ts8550b *ctx = mipi_dsi_get_drvdata(dsi);
• mipi_dsi_detach(dsi);
• drm_panel_remove(&ctx->panel);
• if (ctx->backlight)

• put_device(&ctx->backlight->dev);
  

• return 0;

+}

+static const struct of_device_id ts8550b_of_match[] = {

• { .compatible = "techstar,ts8550b" },
• { }

+}; +MODULE_DEVICE_TABLE(of, ts8550b_of_match);

+static struct mipi_dsi_driver ts8550b_dsi_driver = {

• .probe = ts8550b_dsi_probe,
• .remove = ts8550b_dsi_remove,
• .driver = {

• .name		= "techstar-ts8550b",
  

• .of_match_table	= ts8550b_of_match,
  

• },

+}; +module_mipi_dsi_driver(ts8550b_dsi_driver);

+MODULE_AUTHOR("Jagan Teki jagan@amarulasolutions.com"); +MODULE_DESCRIPTION("Techstar TS8550B MIPI-DSI LCD Panel Driver");

+MODULE_LICENSE("GPL v2");

2.18.0.321.gffc6fa0e3

-- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. For more options, visit https://groups.google.com/d/optout.

On Sat, Oct 27, 2018 at 03:25:29PM +0530, Jagan Teki wrote:

On Fri, Oct 26, 2018 at 9:43 PM Priit Laes plaes@plaes.org wrote:

...

On Fri, Oct 26, 2018 at 08:13:39PM +0530, Jagan Teki wrote:

...

Techstar TS8550B MIPI DSI panel is 480x854, 2-lane MIPI DSI LCD panel. Add panel driver for it.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com Tested-by: Jagan Teki jagan@amarulasolutions.com

---

Changes for v3:

• new patch

Changes for v2:

• none

drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile | 1 + .../gpu/drm/panel/panel-techstar-ts8550b.c | 346 ++++++++++++++++++ 3 files changed, 356 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-techstar-ts8550b.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 20b88c275421..d0d4e60f5153 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -195,4 +195,13 @@ config DRM_PANEL_SITRONIX_ST7789V Say Y here if you want to enable support for the Sitronix ST7789V controller for 240x320 LCD panels

+config DRM_PANEL_TECHSTAR_TS8550B

• tristate "Techstar TS8550B MIPI-DSI panel driver"
  

• depends on OF
  

• depends on DRM_MIPI_DSI
  

• depends on BACKLIGHT_CLASS_DEVICE
  

• help
  

•   Say Y if you want to enable support for panels based on the
  

•   Techstar TS8550B MIPI-DSI interface.
  

endmenu diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 04696bb85218..88011f06edb8 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -20,3 +20,4 @@ obj-$(CONFIG_DRM_PANEL_SEIKO_43WVF1G) += panel-seiko-43wvf1g.o obj-$(CONFIG_DRM_PANEL_SHARP_LQ101R1SX01) += panel-sharp-lq101r1sx01.o obj-$(CONFIG_DRM_PANEL_SHARP_LS043T1LE01) += panel-sharp-ls043t1le01.o obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o +obj-$(CONFIG_DRM_PANEL_TECHSTAR_TS8550B) += panel-techstar-ts8550b.o diff --git a/drivers/gpu/drm/panel/panel-techstar-ts8550b.c b/drivers/gpu/drm/panel/panel-techstar-ts8550b.c new file mode 100644 index 000000000000..8baca71595a7 --- /dev/null +++ b/drivers/gpu/drm/panel/panel-techstar-ts8550b.c @@ -0,0 +1,346 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2019, Amarula Solutions.
• • Author: Jagan Teki jagan@amarulasolutions.com
• */

+#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/device.h> +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/fb.h> +#include <linux/kernel.h> +#include <linux/module.h>

+#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>

+#include <drm/drm_mipi_dsi.h> +#include <drm/drm_modes.h> +#include <drm/drm_panel.h>

+#include <video/mipi_display.h>

+struct ts8550b {

• struct drm_panel        panel;
  

• struct mipi_dsi_device  *dsi;
  

• struct backlight_device *backlight;
  

• struct regulator        *dvdd;
  

• struct regulator        *avdd;
  

• struct gpio_desc        *reset;
  

• bool                    is_enabled;
  

• bool                    is_prepared;
  

+};

+static inline struct ts8550b *panel_to_ts8550b(struct drm_panel *panel) +{

• return container_of(panel, struct ts8550b, panel);
  

+}

+static inline int ts8550b_dcs_write_seq(struct ts8550b *ctx, const void *seq,

•                                 size_t len)
  

+{

• return mipi_dsi_dcs_write_buffer(ctx->dsi, seq, len);
  

+};

+#define ts8550b_dcs_write_seq_static(ctx, seq...) \

• ({                                                      \
  

•         static const u8 d[] = { seq };                  \
  

•         ts8550b_dcs_write_seq(ctx, d, ARRAY_SIZE(d));   \
  

• })
  

+static void ts8550b_init_sequence(struct ts8550b *ctx)

This seems to be pretty "standard" MIPI-based display, utilizing bunch of manufacturer-specific commands during init sequence.

Standard MIPI commands fall between 0x00..0xAF and are defined in <video/mipi.h>

Where is this file? we have include/video/mipi_display.h with 0x00..0xA8. Not all commands in sequence is listed in this, may be I will update DCS commands macros and rest as-it-is. what do you think?

Yeah, include/video/mipi_display.h is the header file I was referring to.

Although those three commands I pointed out look like standard DCS command defines and the order makes sense too: SOFT_RESET -> magic1 -> EXIT_SLEEP_MODE -> magic2 -> SET_DISPLAY_ON

Until someone finds proper datasheet for this device, those magic1 and magic2 sequenses remain mystery...

Päikest, Priit :)