Re: [PATCH v1 3/3] drm/panel: Add support for otm8009a panel driver

2017-07-06 Thread Philippe CORNU 


On 07/05/2017 11:28 AM, Andrzej Hajda wrote:
> On 04.07.2017 18:30, Philippe CORNU wrote:
>> This patch adds Orise Tech otm8009a 3.97" 480x800 TFT LCD
>> panel driver (MIPI-DSI video mode). The panel backlight is
>> managed through the DSI link. This panel driver is used in
>> several STM32 boards.
>>
>> Signed-off-by: Philippe CORNU 
>> ---
>>   drivers/gpu/drm/panel/Kconfig|   9 +
>>   drivers/gpu/drm/panel/Makefile   |   1 +
>>   drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 517 
>> +++
>>   3 files changed, 527 insertions(+)
>>   create mode 100755 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
>>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index d84a031..c1c9291 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -117,4 +117,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_ORISETECH_OTM8009A
>> +tristate "Orise Tech otm8009a 480p dsi 2dl video mode panel"
>> +depends on OF
>> +depends on DRM_MIPI_DSI
>> +depends on BACKLIGHT_CLASS_DEVICE
>> +help
>> +  Say Y here if you want to enable support for Orise Tech OTM8009A
>> +  480x800 DSI panel
>> +
>>   endmenu
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 9f6610d..ac798f3 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += 
>> panel-samsung-s6e8aa0.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_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
>> b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
>> new file mode 100755
>> index 000..7aefc09
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
>> @@ -0,0 +1,517 @@
>> +/*
>> + * Copyright (C) STMicroelectronics SA 2017
>> + *
>> + * Authors: Philippe Cornu 
>> + *  Yannick Fertre 
>> + *
>> + * License terms:  GNU General Public License (GPL), version 2
>> + */
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define DRV_NAME "orisetech_otm8009a"
>> +
>> +#define OTM8009A_BACKLIGHT_DEFAULT  240
>> +#define OTM8009A_BACKLIGHT_MAX  255
>> +
>> +struct otm8009a {
>> +struct device *dev;
>> +struct drm_panel panel;
>> +struct backlight_device *bl_dev;
>> +struct gpio_desc *reset_gpio;
>> +bool prepared;
>> +bool enabled;
>> +};
>> +
>> +static const struct drm_display_mode default_mode = {
>> +.clock = 32729,
>> +.hdisplay = 480,
>> +.hsync_start = 480 + 120,
>> +.hsync_end = 480 + 120 + 63,
>> +.htotal = 480 + 120 + 63 + 120,
>> +.vdisplay = 800,
>> +.vsync_start = 800 + 12,
>> +.vsync_end = 800 + 12 + 12,
>> +.vtotal = 800 + 12 + 12 + 12,
>> +.vrefresh = 50,
>> +.flags = 0,
>> +.width_mm = 52,
>> +.height_mm = 86,
>> +};
>> +
>> +static inline struct otm8009a *panel_to_otm8009a(struct drm_panel *panel)
>> +{
>> +return container_of(panel, struct otm8009a, panel);
>> +}
>> +
>> +static void otm8009a_dcs_write_buf(struct otm8009a *ctx, const void *data,
>> +   size_t len)
>> +{
>> +struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
>> +
>> +if (mipi_dsi_dcs_write_buffer(dsi, data, len) < 0)
>> +DRM_WARN("mipi dsi dcs write buffer failed");
> 
> New line at the end of string.
> 
> I see another way of fighting with error handling, its drawback is that
> failure does not prevent further execution.
> Since I am lost what error handling pattern is currently accepted in
> panels subsystem, I cannot advice if this should be changed.
> 

Hi Andrzef,

First of all, a big *THANK YOU* for this nice code review. Your comments 
are very relevant :-)

Regarding the error management, there are actually several solutions to 
deal with errors but I confess that for the moment none really convinced 
me... that is why I proposed here the *minimal solution of the trace*...

>> +}
>> +
>> +#define dcs_write_seq(seq...)   \
>> +do {\
>> +static const u8 d[] = { seq };  \
>> +otm8009a_dcs_write_buf(ctx, d, ARRAY_SIZE(d));  \
>> +} while (0)
> 
> It is up to your preferences, but I think "do {} while(0)" construct is
> kind of workaround and is obsoleted in favor of

Re: [PATCH v1 3/3] drm/panel: Add support for otm8009a panel driver

2017-07-05 Thread Andrzej Hajda 
On 04.07.2017 18:30, Philippe CORNU wrote:
> This patch adds Orise Tech otm8009a 3.97" 480x800 TFT LCD
> panel driver (MIPI-DSI video mode). The panel backlight is
> managed through the DSI link. This panel driver is used in
> several STM32 boards.
>
> Signed-off-by: Philippe CORNU 
> ---
>  drivers/gpu/drm/panel/Kconfig|   9 +
>  drivers/gpu/drm/panel/Makefile   |   1 +
>  drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 517 
> +++
>  3 files changed, 527 insertions(+)
>  create mode 100755 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
>
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index d84a031..c1c9291 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -117,4 +117,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_ORISETECH_OTM8009A
> + tristate "Orise Tech otm8009a 480p dsi 2dl video mode panel"
> + depends on OF
> + depends on DRM_MIPI_DSI
> + depends on BACKLIGHT_CLASS_DEVICE
> + help
> +   Say Y here if you want to enable support for Orise Tech OTM8009A
> +   480x800 DSI panel
> +
>  endmenu
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index 9f6610d..ac798f3 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += 
> panel-samsung-s6e8aa0.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_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
> b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
> new file mode 100755
> index 000..7aefc09
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
> @@ -0,0 +1,517 @@
> +/*
> + * Copyright (C) STMicroelectronics SA 2017
> + *
> + * Authors: Philippe Cornu 
> + *  Yannick Fertre 
> + *
> + * License terms:  GNU General Public License (GPL), version 2
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DRV_NAME "orisetech_otm8009a"
> +
> +#define OTM8009A_BACKLIGHT_DEFAULT   240
> +#define OTM8009A_BACKLIGHT_MAX   255
> +
> +struct otm8009a {
> + struct device *dev;
> + struct drm_panel panel;
> + struct backlight_device *bl_dev;
> + struct gpio_desc *reset_gpio;
> + bool prepared;
> + bool enabled;
> +};
> +
> +static const struct drm_display_mode default_mode = {
> + .clock = 32729,
> + .hdisplay = 480,
> + .hsync_start = 480 + 120,
> + .hsync_end = 480 + 120 + 63,
> + .htotal = 480 + 120 + 63 + 120,
> + .vdisplay = 800,
> + .vsync_start = 800 + 12,
> + .vsync_end = 800 + 12 + 12,
> + .vtotal = 800 + 12 + 12 + 12,
> + .vrefresh = 50,
> + .flags = 0,
> + .width_mm = 52,
> + .height_mm = 86,
> +};
> +
> +static inline struct otm8009a *panel_to_otm8009a(struct drm_panel *panel)
> +{
> + return container_of(panel, struct otm8009a, panel);
> +}
> +
> +static void otm8009a_dcs_write_buf(struct otm8009a *ctx, const void *data,
> +size_t len)
> +{
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> +
> + if (mipi_dsi_dcs_write_buffer(dsi, data, len) < 0)
> + DRM_WARN("mipi dsi dcs write buffer failed");

New line at the end of string.

I see another way of fighting with error handling, its drawback is that
failure does not prevent further execution.
Since I am lost what error handling pattern is currently accepted in
panels subsystem, I cannot advice if this should be changed.

> +}
> +
> +#define dcs_write_seq(seq...)\
> + do {\
> + static const u8 d[] = { seq };  \
> + otm8009a_dcs_write_buf(ctx, d, ARRAY_SIZE(d));  \
> + } while (0)

It is up to your preferences, but I think "do {} while(0)" construct is
kind of workaround and is obsoleted in favor of ({ ... }) statement
expressions[1].
More serious issue is that you are using ctx implicitly, it should be
added as macro argument.

[1]: https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html

> +
> +static int otm8009a_init_sequence(struct otm8009a *ctx)
> +{
> + struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
> + int ret;
> +
> + /*
> +  * CMD2_ENA1: Enter in Command 2 mode, enable write function of
> +  * Command 2 & enable parameter shift

[PATCH v1 3/3] drm/panel: Add support for otm8009a panel driver

2017-07-04 Thread Philippe CORNU 
This patch adds Orise Tech otm8009a 3.97" 480x800 TFT LCD
panel driver (MIPI-DSI video mode). The panel backlight is
managed through the DSI link. This panel driver is used in
several STM32 boards.

Signed-off-by: Philippe CORNU 
---
 drivers/gpu/drm/panel/Kconfig|   9 +
 drivers/gpu/drm/panel/Makefile   |   1 +
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 517 +++
 3 files changed, 527 insertions(+)
 create mode 100755 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index d84a031..c1c9291 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -117,4 +117,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_ORISETECH_OTM8009A
+   tristate "Orise Tech otm8009a 480p dsi 2dl video mode panel"
+   depends on OF
+   depends on DRM_MIPI_DSI
+   depends on BACKLIGHT_CLASS_DEVICE
+   help
+ Say Y here if you want to enable support for Orise Tech OTM8009A
+ 480x800 DSI panel
+
 endmenu
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index 9f6610d..ac798f3 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_DRM_PANEL_SAMSUNG_S6E8AA0) += 
panel-samsung-s6e8aa0.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_ORISETECH_OTM8009A) += panel-orisetech-otm8009a.o
\ No newline at end of file
diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c 
b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
new file mode 100755
index 000..7aefc09
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -0,0 +1,517 @@
+/*
+ * Copyright (C) STMicroelectronics SA 2017
+ *
+ * Authors: Philippe Cornu 
+ *  Yannick Fertre 
+ *
+ * License terms:  GNU General Public License (GPL), version 2
+ */
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define DRV_NAME "orisetech_otm8009a"
+
+#define OTM8009A_BACKLIGHT_DEFAULT 240
+#define OTM8009A_BACKLIGHT_MAX 255
+
+struct otm8009a {
+   struct device *dev;
+   struct drm_panel panel;
+   struct backlight_device *bl_dev;
+   struct gpio_desc *reset_gpio;
+   bool prepared;
+   bool enabled;
+};
+
+static const struct drm_display_mode default_mode = {
+   .clock = 32729,
+   .hdisplay = 480,
+   .hsync_start = 480 + 120,
+   .hsync_end = 480 + 120 + 63,
+   .htotal = 480 + 120 + 63 + 120,
+   .vdisplay = 800,
+   .vsync_start = 800 + 12,
+   .vsync_end = 800 + 12 + 12,
+   .vtotal = 800 + 12 + 12 + 12,
+   .vrefresh = 50,
+   .flags = 0,
+   .width_mm = 52,
+   .height_mm = 86,
+};
+
+static inline struct otm8009a *panel_to_otm8009a(struct drm_panel *panel)
+{
+   return container_of(panel, struct otm8009a, panel);
+}
+
+static void otm8009a_dcs_write_buf(struct otm8009a *ctx, const void *data,
+  size_t len)
+{
+   struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+
+   if (mipi_dsi_dcs_write_buffer(dsi, data, len) < 0)
+   DRM_WARN("mipi dsi dcs write buffer failed");
+}
+
+#define dcs_write_seq(seq...)  \
+   do {\
+   static const u8 d[] = { seq };  \
+   otm8009a_dcs_write_buf(ctx, d, ARRAY_SIZE(d));  \
+   } while (0)
+
+static int otm8009a_init_sequence(struct otm8009a *ctx)
+{
+   struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
+   int ret;
+
+   /*
+* CMD2_ENA1: Enter in Command 2 mode, enable write function of
+* Command 2 & enable parameter shift function.
+* The 3 following sequences allow to enable ORISE command mode.
+*/
+   dcs_write_seq(0xFF, 0x80, 0x09, 0x01);
+   dcs_write_seq(0x00, 0x80);
+   dcs_write_seq(0xFF, 0x80, 0x09);
+
+   /*
+* Starting from here, address shift needs to be set before sending
+* a new command. You can find an example in the next sequence...
+* SD_PCH_CTRL (0xC480) Source Driver Precharge Control (SD_PT=GND)
+*/
+   dcs_write_seq(0x00, 0x80); /* address shift set to 0x80 */
+   dcs_write_seq(0xC4, 0x30); /* 0xC480 parameter 1 is 0x30 */
+   mdelay(10);
+
+   /* Not documented (0xC48A) */
+   dcs_write_seq(0x00, 0x8A);
+   dcs_write_seq(0xC4, 0x40);
+   mdelay(10);
+
+   /* PWR_CTRL4 (0xC5B0) Power Control Setting 4 for DC