Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread David Lechner 

On 12/11/2017 03:18 PM, Noralf Trønnes wrote:


Den 10.12.2017 23.10, skrev David Lechner:

This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---



+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+    struct drm_crtc_state *crtc_state)
+{
+    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+    struct device *dev = tdev->drm->dev;
+    int ret;
+    u8 addr_mode;
+
+    DRM_DEBUG_KMS("\n");
+
+    mipi_dbi_hw_reset(mipi);
+
+    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+    if (ret) {
+    DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+    return;
+    }
+
+    msleep(150);
+
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+    msleep(500);
+
+    mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 
0x2c,

+ 0x2d);
+    mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+    mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+    mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+    mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+    mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+    mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+    mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+    switch (mipi->rotation) {
+    default:
+    addr_mode = ST7735R_MX | ST7735R_MY;
+    break;
+    case 90:
+    addr_mode = ST7735R_MX | ST7735R_MV;
+    break;
+    case 180:
+    addr_mode = 0;
+    break;
+    case 270:
+    addr_mode = ST7735R_MY | ST7735R_MV;
+    break;
+    }
+    mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+ MIPI_DCS_PIXEL_FMT_16BIT);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 
0x2f,

+ 0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+ 0x02, 0x10);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 
0x33,

+ 0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+ 0x03, 0x10);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+    msleep(100);
+
+    mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+    msleep(20);
+
+    mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+    .enable    = st7735r_pipe_enable,
+    .disable    = mipi_dbi_pipe_disable,
+    .update    = tinydrm_display_pipe_update,
+    .prepare_fb    = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+    TINYDRM_MODE(128, 160, 28, 35),
+};


This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.


Adafruit has a 1.44" 128x128px display [1] with the same controller, so 
the answer is no, the mode cannot be generic.


[1]: https://www.adafruit.com/product/2088


What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?


My first impression was that I didn't like it so much. But now, I 
actually think that it is a good idea. I think it is a good compromise 
between some boilerplate code in every driver and a driver with so many 
quirks that it is very difficult to work on. It may make sense to 
combine some very similar controllers, but as a rule of thumb, I think 
one driver per controller will work nicely.



I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...


I think as we add more drivers, the parts that get duplicated will 
become obvious candidates for helper functions to refactor, but I don't 
think trying to cram everything all into one driver is such a good idea.

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread David Lechner 

On 12/11/2017 03:18 PM, Noralf Trønnes wrote:


Den 10.12.2017 23.10, skrev David Lechner:

This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---



+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+    struct drm_crtc_state *crtc_state)
+{
+    struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+    struct device *dev = tdev->drm->dev;
+    int ret;
+    u8 addr_mode;
+
+    DRM_DEBUG_KMS("\n");
+
+    mipi_dbi_hw_reset(mipi);
+
+    ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+    if (ret) {
+    DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+    return;
+    }
+
+    msleep(150);
+
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+    msleep(500);
+
+    mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+    mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 
0x2c,

+ 0x2d);
+    mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+    mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+    mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+    mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+    mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+    mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+    mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+    mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+    switch (mipi->rotation) {
+    default:
+    addr_mode = ST7735R_MX | ST7735R_MY;
+    break;
+    case 90:
+    addr_mode = ST7735R_MX | ST7735R_MV;
+    break;
+    case 180:
+    addr_mode = 0;
+    break;
+    case 270:
+    addr_mode = ST7735R_MY | ST7735R_MV;
+    break;
+    }
+    mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+ MIPI_DCS_PIXEL_FMT_16BIT);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 
0x2f,

+ 0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+ 0x02, 0x10);
+    mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 
0x33,

+ 0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+ 0x03, 0x10);
+    mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+    msleep(100);
+
+    mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+    msleep(20);
+
+    mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+    .enable    = st7735r_pipe_enable,
+    .disable    = mipi_dbi_pipe_disable,
+    .update    = tinydrm_display_pipe_update,
+    .prepare_fb    = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+    TINYDRM_MODE(128, 160, 28, 35),
+};


This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.


Adafruit has a 1.44" 128x128px display [1] with the same controller, so 
the answer is no, the mode cannot be generic.


[1]: https://www.adafruit.com/product/2088


What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?


My first impression was that I didn't like it so much. But now, I 
actually think that it is a good idea. I think it is a good compromise 
between some boilerplate code in every driver and a driver with so many 
quirks that it is very difficult to work on. It may make sense to 
combine some very similar controllers, but as a rule of thumb, I think 
one driver per controller will work nicely.



I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...


I think as we add more drivers, the parts that get duplicated will 
become obvious candidates for helper functions to refactor, but I don't 
think trying to cram everything all into one driver is such a good idea.

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread Noralf Trønnes 


Den 10.12.2017 23.10, skrev David Lechner:

This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---



+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+   struct drm_crtc_state *crtc_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   struct device *dev = tdev->drm->dev;
+   int ret;
+   u8 addr_mode;
+
+   DRM_DEBUG_KMS("\n");
+
+   mipi_dbi_hw_reset(mipi);
+
+   ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+   if (ret) {
+   DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+   return;
+   }
+
+   msleep(150);
+
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+   msleep(500);
+
+   mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+0x2d);
+   mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+   mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+   mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+   mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+   mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+   mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+   mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+   switch (mipi->rotation) {
+   default:
+   addr_mode = ST7735R_MX | ST7735R_MY;
+   break;
+   case 90:
+   addr_mode = ST7735R_MX | ST7735R_MV;
+   break;
+   case 180:
+   addr_mode = 0;
+   break;
+   case 270:
+   addr_mode = ST7735R_MY | ST7735R_MV;
+   break;
+   }
+   mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+   mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+MIPI_DCS_PIXEL_FMT_16BIT);
+   mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 0x2f,
+0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+0x02, 0x10);
+   mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 0x33,
+0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+0x03, 0x10);
+   mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+   msleep(100);
+
+   mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+   msleep(20);
+
+   mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+   .enable = st7735r_pipe_enable,
+   .disable= mipi_dbi_pipe_disable,
+   .update = tinydrm_display_pipe_update,
+   .prepare_fb = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+   TINYDRM_MODE(128, 160, 28, 35),
+};


This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.

What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?
I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...

Noralf.

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread Noralf Trønnes 


Den 10.12.2017 23.10, skrev David Lechner:

This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---



+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+   struct drm_crtc_state *crtc_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   struct device *dev = tdev->drm->dev;
+   int ret;
+   u8 addr_mode;
+
+   DRM_DEBUG_KMS("\n");
+
+   mipi_dbi_hw_reset(mipi);
+
+   ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+   if (ret) {
+   DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+   return;
+   }
+
+   msleep(150);
+
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+   msleep(500);
+
+   mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+0x2d);
+   mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+   mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+   mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+   mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+   mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+   mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+   mipi_dbi_command(mipi, ST7735R_VMCTR1, 0x0e);
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+   switch (mipi->rotation) {
+   default:
+   addr_mode = ST7735R_MX | ST7735R_MY;
+   break;
+   case 90:
+   addr_mode = ST7735R_MX | ST7735R_MV;
+   break;
+   case 180:
+   addr_mode = 0;
+   break;
+   case 270:
+   addr_mode = ST7735R_MY | ST7735R_MV;
+   break;
+   }
+   mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+   mipi_dbi_command(mipi, MIPI_DCS_SET_PIXEL_FORMAT,
+MIPI_DCS_PIXEL_FMT_16BIT);
+   mipi_dbi_command(mipi, ST7735R_GAMCTRP1, 0x0f, 0x1a, 0x0f, 0x18, 0x2f,
+0x28, 0x20, 0x22, 0x1f, 0x1b, 0x23, 0x37, 0x00, 0x07,
+0x02, 0x10);
+   mipi_dbi_command(mipi, ST7735R_GAMCTRN1, 0x0f, 0x1b, 0x0f, 0x17, 0x33,
+0x2c, 0x29, 0x2e, 0x30, 0x30, 0x39, 0x3f, 0x00, 0x07,
+0x03, 0x10);
+   mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+   msleep(100);
+
+   mipi_dbi_command(mipi, MIPI_DCS_ENTER_NORMAL_MODE);
+
+   msleep(20);
+
+   mipi_dbi_pipe_enable(pipe, crtc_state);
+}
+
+static const struct drm_simple_display_pipe_funcs st7735r_pipe_funcs = {
+   .enable = st7735r_pipe_enable,
+   .disable= mipi_dbi_pipe_disable,
+   .update = tinydrm_display_pipe_update,
+   .prepare_fb = tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7735r_mode = {
+   TINYDRM_MODE(128, 160, 28, 35),
+};


This naming talk has made me realise that these should be named after
the panel since they're not generic controller functions.
Maybe the mode can be generic, not sure if the physical size can/will
differ, the resolution is very likely to be the same for all.

What's your view on my descision to split the MIPI DBI compatible
controllers into per controller drivers instead of having one driver
that supports them all?
I've been afraid that it will be a very big file with macro definitions
per controller and enable functions per panel.

This driver has 15 macros and ~80 panel specific lines.
With one panel supported, half the driver is boilerplate.
The mi0283qt panel (MIPI DBI compatible) is in the same ballpark.

Adding helpers for panel reset/exit_sleep, for MIPI_DCS_SET_ADDRESS_MODE
and for display_on/enter_normal/flush could cut it down some more if we
made one driver to rule them all. Maybe the enable function per panel
could be cut in half that way.

I'm starting to wonder if one driver isn't such a bad solution after all...

Noralf.

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread Philippe Ombredanne 
David,

On Sun, Dec 10, 2017 at 11:10 PM, David Lechner  wrote:
> This adds a new driver for Sitronix ST7735R display panels.
>
> This has been tested using an Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner 
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -0,0 +1,219 @@
> +/*
> + * DRM driver for Sitronix ST7735R panels
> + *
> + * Copyright 2017 David Lechner 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

Have you considered using the new SPDX ids? Check the doc patches from
Thomas for details.

This could come out this way if you are using the C++ comment style
all the way, a style that you should at least use for the license id
as requested and commented by Linus:

> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2017 David Lechner 
> +// DRM driver for Sitronix ST7735R panels

-- 
Cordially
Philippe Ombredanne

Re: [PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-11 Thread Philippe Ombredanne 
David,

On Sun, Dec 10, 2017 at 11:10 PM, David Lechner  wrote:
> This adds a new driver for Sitronix ST7735R display panels.
>
> This has been tested using an Adafruit 1.8" TFT.
>
> Signed-off-by: David Lechner 
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7735r.c
> @@ -0,0 +1,219 @@
> +/*
> + * DRM driver for Sitronix ST7735R panels
> + *
> + * Copyright 2017 David Lechner 
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */

Have you considered using the new SPDX ids? Check the doc patches from
Thomas for details.

This could come out this way if you are using the C++ comment style
all the way, a style that you should at least use for the license id
as requested and commented by Linus:

> +// SPDX-License-Identifier: GPL-2.0+
> +// Copyright 2017 David Lechner 
> +// DRM driver for Sitronix ST7735R panels

-- 
Cordially
Philippe Ombredanne

[PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-10 Thread David Lechner 
This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---

v2 changes:
* Change delay from 10ms to 20ms to avoid checkpatch warning
* Use mipi_dbi_pipe_enable()/mipi_dbi_pipe_disable() to reduce duplicated code
* Rebase on drm-misc-next (tinydrm_lastclose => drm_fb_helper_lastclose)
* Use mipi_dbi_debugfs_init
* Add mipi->read_commands = NULL; since this display is write-only


 MAINTAINERS   |   6 ++
 drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
 drivers/gpu/drm/tinydrm/Makefile  |   1 +
 drivers/gpu/drm/tinydrm/st7735r.c | 219 ++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/st7735r.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0e49099..2ce17f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4551,6 +4551,12 @@ S:   Maintained
 F: drivers/gpu/drm/tinydrm/st7586.c
 F: Documentation/devicetree/bindings/display/st7586.txt
 
+DRM DRIVER FOR SITRONIX ST7735R PANELS
+M: David Lechner 
+S: Maintained
+F: drivers/gpu/drm/tinydrm/st7735r.c
+F: Documentation/devicetree/bindings/display/st7735r.txt
+
 DRM DRIVER FOR TDFX VIDEO CARDS
 S: Orphan / Obsolete
 F: drivers/gpu/drm/tdfx/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 90c5bd5..b0e567d 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -52,3 +52,13 @@ config TINYDRM_ST7586
  * LEGO MINDSTORMS EV3
 
  If M is selected the module will be called st7586.
+
+config TINYDRM_ST7735R
+   tristate "DRM support for Sitronix ST7735R display panels"
+   depends on DRM_TINYDRM && SPI
+   select TINYDRM_MIPI_DBI
+   help
+ DRM driver Sitronix ST7735R with one of the following LCDs:
+ * JD-T18003-T01 1.8" 128x160 TFT
+
+ If M is selected the module will be called st7735r.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 8aeee53..49a1119 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_TINYDRM_ILI9225)   += ili9225.o
 obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)  += repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)   += st7586.o
+obj-$(CONFIG_TINYDRM_ST7735R)  += st7735r.o
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c 
b/drivers/gpu/drm/tinydrm/st7735r.c
new file mode 100644
index 000..0f5c295
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -0,0 +1,219 @@
+/*
+ * DRM driver for Sitronix ST7735R panels
+ *
+ * Copyright 2017 David Lechner 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define ST7735R_FRMCTR10xb1
+#define ST7735R_FRMCTR20xb2
+#define ST7735R_FRMCTR30xb3
+#define ST7735R_INVCTR 0xb4
+#define ST7735R_PWCTR1 0xc0
+#define ST7735R_PWCTR2 0xc1
+#define ST7735R_PWCTR3 0xc2
+#define ST7735R_PWCTR4 0xc3
+#define ST7735R_PWCTR5 0xc4
+#define ST7735R_VMCTR1 0xc5
+#define ST7735R_GAMCTRP1   0xe0
+#define ST7735R_GAMCTRN1   0xe1
+
+#define ST7735R_MY BIT(7)
+#define ST7735R_MX BIT(6)
+#define ST7735R_MV BIT(5)
+
+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+   struct drm_crtc_state *crtc_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   struct device *dev = tdev->drm->dev;
+   int ret;
+   u8 addr_mode;
+
+   DRM_DEBUG_KMS("\n");
+
+   mipi_dbi_hw_reset(mipi);
+
+   ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+   if (ret) {
+   DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+   return;
+   }
+
+   msleep(150);
+
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+   msleep(500);
+
+   mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+0x2d);
+   mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+   mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+   mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+   mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+   mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+

[PATCH v2 2/2] drm/tinydrm: add driver for ST7735R panels

2017-12-10 Thread David Lechner 
This adds a new driver for Sitronix ST7735R display panels.

This has been tested using an Adafruit 1.8" TFT.

Signed-off-by: David Lechner 
---

v2 changes:
* Change delay from 10ms to 20ms to avoid checkpatch warning
* Use mipi_dbi_pipe_enable()/mipi_dbi_pipe_disable() to reduce duplicated code
* Rebase on drm-misc-next (tinydrm_lastclose => drm_fb_helper_lastclose)
* Use mipi_dbi_debugfs_init
* Add mipi->read_commands = NULL; since this display is write-only


 MAINTAINERS   |   6 ++
 drivers/gpu/drm/tinydrm/Kconfig   |  10 ++
 drivers/gpu/drm/tinydrm/Makefile  |   1 +
 drivers/gpu/drm/tinydrm/st7735r.c | 219 ++
 4 files changed, 236 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/st7735r.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0e49099..2ce17f5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4551,6 +4551,12 @@ S:   Maintained
 F: drivers/gpu/drm/tinydrm/st7586.c
 F: Documentation/devicetree/bindings/display/st7586.txt
 
+DRM DRIVER FOR SITRONIX ST7735R PANELS
+M: David Lechner 
+S: Maintained
+F: drivers/gpu/drm/tinydrm/st7735r.c
+F: Documentation/devicetree/bindings/display/st7735r.txt
+
 DRM DRIVER FOR TDFX VIDEO CARDS
 S: Orphan / Obsolete
 F: drivers/gpu/drm/tdfx/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index 90c5bd5..b0e567d 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -52,3 +52,13 @@ config TINYDRM_ST7586
  * LEGO MINDSTORMS EV3
 
  If M is selected the module will be called st7586.
+
+config TINYDRM_ST7735R
+   tristate "DRM support for Sitronix ST7735R display panels"
+   depends on DRM_TINYDRM && SPI
+   select TINYDRM_MIPI_DBI
+   help
+ DRM driver Sitronix ST7735R with one of the following LCDs:
+ * JD-T18003-T01 1.8" 128x160 TFT
+
+ If M is selected the module will be called st7735r.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 8aeee53..49a1119 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_TINYDRM_ILI9225)   += ili9225.o
 obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)  += repaper.o
 obj-$(CONFIG_TINYDRM_ST7586)   += st7586.o
+obj-$(CONFIG_TINYDRM_ST7735R)  += st7735r.o
diff --git a/drivers/gpu/drm/tinydrm/st7735r.c 
b/drivers/gpu/drm/tinydrm/st7735r.c
new file mode 100644
index 000..0f5c295
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/st7735r.c
@@ -0,0 +1,219 @@
+/*
+ * DRM driver for Sitronix ST7735R panels
+ *
+ * Copyright 2017 David Lechner 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+#include 
+#include 
+
+#define ST7735R_FRMCTR10xb1
+#define ST7735R_FRMCTR20xb2
+#define ST7735R_FRMCTR30xb3
+#define ST7735R_INVCTR 0xb4
+#define ST7735R_PWCTR1 0xc0
+#define ST7735R_PWCTR2 0xc1
+#define ST7735R_PWCTR3 0xc2
+#define ST7735R_PWCTR4 0xc3
+#define ST7735R_PWCTR5 0xc4
+#define ST7735R_VMCTR1 0xc5
+#define ST7735R_GAMCTRP1   0xe0
+#define ST7735R_GAMCTRN1   0xe1
+
+#define ST7735R_MY BIT(7)
+#define ST7735R_MX BIT(6)
+#define ST7735R_MV BIT(5)
+
+static void st7735r_pipe_enable(struct drm_simple_display_pipe *pipe,
+   struct drm_crtc_state *crtc_state)
+{
+   struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+   struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+   struct device *dev = tdev->drm->dev;
+   int ret;
+   u8 addr_mode;
+
+   DRM_DEBUG_KMS("\n");
+
+   mipi_dbi_hw_reset(mipi);
+
+   ret = mipi_dbi_command(mipi, MIPI_DCS_SOFT_RESET);
+   if (ret) {
+   DRM_DEV_ERROR(dev, "Error sending command %d\n", ret);
+   return;
+   }
+
+   msleep(150);
+
+   mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+   msleep(500);
+
+   mipi_dbi_command(mipi, ST7735R_FRMCTR1, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR2, 0x01, 0x2c, 0x2d);
+   mipi_dbi_command(mipi, ST7735R_FRMCTR3, 0x01, 0x2c, 0x2d, 0x01, 0x2c,
+0x2d);
+   mipi_dbi_command(mipi, ST7735R_INVCTR, 0x07);
+   mipi_dbi_command(mipi, ST7735R_PWCTR1, 0xa2, 0x02, 0x84);
+   mipi_dbi_command(mipi, ST7735R_PWCTR2, 0xc5);
+   mipi_dbi_command(mipi, ST7735R_PWCTR3, 0x0a, 0x00);
+   mipi_dbi_command(mipi, ST7735R_PWCTR4, 0x8a, 0x2a);
+   mipi_dbi_command(mipi, ST7735R_PWCTR5, 0x8a, 0xee);
+