[PATCH v3 0/6] Support for LEGO MINDSTORMS EV3 LCD display

[PATCH v3 0/6] Support for LEGO MINDSTORMS EV3 LCD display

[David Lechner]

The goal of this series is to get the built-in LCD of the LEGO MINDSTORMS EV3
working.

v2 changes:
* Wrote a new driver for ST7586 instead of combining it with existing drivers
* Don't touch MIPI DBI code (other than the patch suggested by Noralf)
* New defconfig patch

v3 changes:
* New patch to generalize tinydrm_xrgb8888_to_gray8() so that it can be reused.
* Device tree bindings in separate patch.
* Fixed incorrect device tree binding pin descriptions.
* Added MAINTAINERS entry for drivers/gpu/drm/tinydrm/st7586.c.
* Removed "mipi_dbi_" from function names in st7586.c.
* Moved init and fini to pipe_enable and pipe_disable ops.
* Dropped RGB565 format.
* Made adjustments for the fact the controller cannot be read via SPI.
* Dropped st7586.h - values moved into st7586.c.

David Lechner (6):
  drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()
  drm/tinydrm: generalize tinydrm_xrgb8888_to_gray8()
  dt-bindings: add binding for Sitronix ST7586 display panels
  drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD
  ARM: dts: da850-lego-ev3: Add node for LCD display
  ARM: davinci_all_defconfig: enable tinydrm and ST7586

 .../bindings/display/sitronix,st7586.txt           |  26 ++
 MAINTAINERS                                        |   6 +
 arch/arm/boot/dts/da850-lego-ev3.dts               |  24 ++
 arch/arm/configs/davinci_all_defconfig             |   2 +
 drivers/gpu/drm/tinydrm/Kconfig                    |  10 +
 drivers/gpu/drm/tinydrm/Makefile                   |   1 +
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c     |  35 +-
 drivers/gpu/drm/tinydrm/mi0283qt.c                 |   8 +-
 drivers/gpu/drm/tinydrm/mipi-dbi.c                 |  17 +-
 drivers/gpu/drm/tinydrm/repaper.c                  |  21 +-
 drivers/gpu/drm/tinydrm/st7586.c                   | 466 +++++++++++++++++++++
 include/drm/tinydrm/mipi-dbi.h                     |   6 +-
 include/drm/tinydrm/tinydrm-helpers.h              |   3 +-
 13 files changed, 582 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7586.txt
 create mode 100644 drivers/gpu/drm/tinydrm/st7586.c

-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/6] drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()

[David Lechner]

This removes the call to mipi_dbi_init() from mipi_dbi_spi_init() so that
drivers can have a driver-specific implementation if needed.

Suggested-by: Noralf Trønnes <noralf@tronnes.org>
Signed-off-by: David Lechner <david@lechnology.com>
Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/gpu/drm/tinydrm/mi0283qt.c |  8 ++++++--
 drivers/gpu/drm/tinydrm/mipi-dbi.c | 17 +++++------------
 include/drm/tinydrm/mipi-dbi.h     |  6 +-----
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
index 482ff1c3..7e5bb7d 100644
--- a/drivers/gpu/drm/tinydrm/mi0283qt.c
+++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
@@ -195,8 +195,12 @@ static int mi0283qt_probe(struct spi_device *spi)
 
 	device_property_read_u32(dev, "rotation", &rotation);
 
-	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
-				&mi0283qt_driver, &mi0283qt_mode, rotation);
+	ret = mipi_dbi_spi_init(spi, mipi, dc);
+	if (ret)
+		return ret;
+
+	ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
+			    &mi0283qt_driver, &mi0283qt_mode, rotation);
 	if (ret)
 		return ret;
 
diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
index e10fa4b..cba9784 100644
--- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
+++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
@@ -777,15 +777,12 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
 /**
  * mipi_dbi_spi_init - Initialize MIPI DBI SPI interfaced controller
  * @spi: SPI device
- * @dc: D/C gpio (optional)
  * @mipi: &mipi_dbi structure to initialize
- * @pipe_funcs: Display pipe functions
- * @driver: DRM driver
- * @mode: Display mode
- * @rotation: Initial rotation in degrees Counter Clock Wise
+ * @dc: D/C gpio (optional)
  *
  * This function sets &mipi_dbi->command, enables &mipi->read_commands for the
- * usual read commands and initializes @mipi using mipi_dbi_init().
+ * usual read commands. It should be followed by a call to mipi_dbi_init() or
+ * a driver-specific init.
  *
  * If @dc is set, a Type C Option 3 interface is assumed, if not
  * Type C Option 1.
@@ -800,11 +797,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
  * Zero on success, negative error code on failure.
  */
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
-		      struct gpio_desc *dc,
-		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
-		      struct drm_driver *driver,
-		      const struct drm_display_mode *mode,
-		      unsigned int rotation)
+		      struct gpio_desc *dc)
 {
 	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
 	struct device *dev = &spi->dev;
@@ -850,7 +843,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
 			return -ENOMEM;
 	}
 
-	return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation);
+	return 0;
 }
 EXPORT_SYMBOL(mipi_dbi_spi_init);
 
diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
index d137b16..83346dd 100644
--- a/include/drm/tinydrm/mipi-dbi.h
+++ b/include/drm/tinydrm/mipi-dbi.h
@@ -62,11 +62,7 @@ mipi_dbi_from_tinydrm(struct tinydrm_device *tdev)
 }
 
 int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
-		      struct gpio_desc *dc,
-		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
-		      struct drm_driver *driver,
-		      const struct drm_display_mode *mode,
-		      unsigned int rotation);
+		      struct gpio_desc *dc);
 int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
 		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
 		  struct drm_driver *driver,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/6] drm/tinydrm: remove call to mipi_dbi_init() from mipi_dbi_spi_init()

[Noralf Trønnes]

Den 04.08.2017 00.33, skrev David Lechner:
> This removes the call to mipi_dbi_init() from mipi_dbi_spi_init() so that
> drivers can have a driver-specific implementation if needed.
>
> Suggested-by: Noralf Trønnes <noralf@tronnes.org>
> Signed-off-by: David Lechner <david@lechnology.com>
> Reviewed-by: Noralf Trønnes <noralf@tronnes.org>
> ---

Thanks, applied to drm-misc.

Noralf.

>   drivers/gpu/drm/tinydrm/mi0283qt.c |  8 ++++++--
>   drivers/gpu/drm/tinydrm/mipi-dbi.c | 17 +++++------------
>   include/drm/tinydrm/mipi-dbi.h     |  6 +-----
>   3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/mi0283qt.c b/drivers/gpu/drm/tinydrm/mi0283qt.c
> index 482ff1c3..7e5bb7d 100644
> --- a/drivers/gpu/drm/tinydrm/mi0283qt.c
> +++ b/drivers/gpu/drm/tinydrm/mi0283qt.c
> @@ -195,8 +195,12 @@ static int mi0283qt_probe(struct spi_device *spi)
>   
>   	device_property_read_u32(dev, "rotation", &rotation);
>   
> -	ret = mipi_dbi_spi_init(spi, mipi, dc, &mi0283qt_pipe_funcs,
> -				&mi0283qt_driver, &mi0283qt_mode, rotation);
> +	ret = mipi_dbi_spi_init(spi, mipi, dc);
> +	if (ret)
> +		return ret;
> +
> +	ret = mipi_dbi_init(&spi->dev, mipi, &mi0283qt_pipe_funcs,
> +			    &mi0283qt_driver, &mi0283qt_mode, rotation);
>   	if (ret)
>   		return ret;
>   
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index e10fa4b..cba9784 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -777,15 +777,12 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>   /**
>    * mipi_dbi_spi_init - Initialize MIPI DBI SPI interfaced controller
>    * @spi: SPI device
> - * @dc: D/C gpio (optional)
>    * @mipi: &mipi_dbi structure to initialize
> - * @pipe_funcs: Display pipe functions
> - * @driver: DRM driver
> - * @mode: Display mode
> - * @rotation: Initial rotation in degrees Counter Clock Wise
> + * @dc: D/C gpio (optional)
>    *
>    * This function sets &mipi_dbi->command, enables &mipi->read_commands for the
> - * usual read commands and initializes @mipi using mipi_dbi_init().
> + * usual read commands. It should be followed by a call to mipi_dbi_init() or
> + * a driver-specific init.
>    *
>    * If @dc is set, a Type C Option 3 interface is assumed, if not
>    * Type C Option 1.
> @@ -800,11 +797,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>    * Zero on success, negative error code on failure.
>    */
>   int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
> -		      struct gpio_desc *dc,
> -		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
> -		      struct drm_driver *driver,
> -		      const struct drm_display_mode *mode,
> -		      unsigned int rotation)
> +		      struct gpio_desc *dc)
>   {
>   	size_t tx_size = tinydrm_spi_max_transfer_size(spi, 0);
>   	struct device *dev = &spi->dev;
> @@ -850,7 +843,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
>   			return -ENOMEM;
>   	}
>   
> -	return mipi_dbi_init(dev, mipi, pipe_funcs, driver, mode, rotation);
> +	return 0;
>   }
>   EXPORT_SYMBOL(mipi_dbi_spi_init);
>   
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index d137b16..83346dd 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -62,11 +62,7 @@ mipi_dbi_from_tinydrm(struct tinydrm_device *tdev)
>   }
>   
>   int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi,
> -		      struct gpio_desc *dc,
> -		      const struct drm_simple_display_pipe_funcs *pipe_funcs,
> -		      struct drm_driver *driver,
> -		      const struct drm_display_mode *mode,
> -		      unsigned int rotation);
> +		      struct gpio_desc *dc);
>   int mipi_dbi_init(struct device *dev, struct mipi_dbi *mipi,
>   		  const struct drm_simple_display_pipe_funcs *pipe_funcs,
>   		  struct drm_driver *driver,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 2/6] drm/tinydrm: generalize tinydrm_xrgb8888_to_gray8()

[David Lechner]

This adds parameters for vaddr and clip to tinydrm_xrgb8888_to_gray8() to
make it more generic.

dma_buf_{begin,end}_cpu_access() are moved out to the repaper driver.

Signed-off-by: David Lechner <david@lechnology.com>
---
 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 35 ++++++++++----------------
 drivers/gpu/drm/tinydrm/repaper.c              | 21 +++++++++++++++-
 include/drm/tinydrm/tinydrm-helpers.h          |  3 ++-
 3 files changed, 35 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
index 75808bb..5915ba8 100644
--- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
+++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
@@ -185,7 +185,9 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
 /**
  * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
  * @dst: 8-bit grayscale destination buffer
+ * @vaddr: XRGB8888 source buffer
  * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
  *
  * Drm doesn't have native monochrome or grayscale support.
  * Such drivers can announce the commonly supported XR24 format to userspace
@@ -199,12 +201,11 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
  * Returns:
  * Zero on success, negative error code on failure.
  */
-int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
+int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
+			      struct drm_clip_rect *clip)
 {
-	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
-	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
-	unsigned int x, y, pitch = fb->pitches[0];
-	int ret = 0;
+	unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
+	unsigned int x, y;
 	void *buf;
 	u32 *src;
 
@@ -214,22 +215,16 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
 	 * The cma memory is write-combined so reads are uncached.
 	 * Speed up by fetching one line at a time.
 	 */
-	buf = kmalloc(pitch, GFP_KERNEL);
+	buf = kmalloc(len, GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
-	if (import_attach) {
-		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
-					       DMA_FROM_DEVICE);
-		if (ret)
-			goto err_free;
-	}
-
-	for (y = 0; y < fb->height; y++) {
-		src = cma_obj->vaddr + (y * pitch);
-		memcpy(buf, src, pitch);
+	for (y = clip->y1; y < clip->y2; y++) {
+		src = vaddr + (y * fb->pitches[0]);
+		src += clip->x1;
+		memcpy(buf, src, len);
 		src = buf;
-		for (x = 0; x < fb->width; x++) {
+		for (x = clip->x1; x < clip->x2; x++) {
 			u8 r = (*src & 0x00ff0000) >> 16;
 			u8 g = (*src & 0x0000ff00) >> 8;
 			u8 b =  *src & 0x000000ff;
@@ -240,13 +235,9 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
 		}
 	}
 
-	if (import_attach)
-		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
-					     DMA_FROM_DEVICE);
-err_free:
 	kfree(buf);
 
-	return ret;
+	return 0;
 }
 EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
 
diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
index 3343d3f..d34cd9b 100644
--- a/drivers/gpu/drm/tinydrm/repaper.c
+++ b/drivers/gpu/drm/tinydrm/repaper.c
@@ -18,6 +18,7 @@
  */
 
 #include <linux/delay.h>
+#include <linux/dma-buf.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -525,11 +526,20 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
 			    struct drm_clip_rect *clips,
 			    unsigned int num_clips)
 {
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
 	struct tinydrm_device *tdev = fb->dev->dev_private;
 	struct repaper_epd *epd = epd_from_tinydrm(tdev);
+	struct drm_clip_rect clip;
 	u8 *buf = NULL;
 	int ret = 0;
 
+	/* repaper can't do partial updates */
+	clip.x1 = 0;
+	clip.x2 = fb->width;
+	clip.y1 = 0;
+	clip.y2 = fb->height;
+
 	mutex_lock(&tdev->dirty_lock);
 
 	if (!epd->enabled)
@@ -550,7 +560,16 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
 		goto out_unlock;
 	}
 
-	ret = tinydrm_xrgb8888_to_gray8(buf, fb);
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
+					       DMA_FROM_DEVICE);
+		if (ret)
+			goto out_unlock;
+	}
+
+	ret = tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
+	if (import_attach)
+		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
 	if (ret)
 		goto out_unlock;
 
diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
index a6c387f..9e13ef5 100644
--- a/include/drm/tinydrm/tinydrm-helpers.h
+++ b/include/drm/tinydrm/tinydrm-helpers.h
@@ -43,7 +43,8 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
 void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
 				struct drm_framebuffer *fb,
 				struct drm_clip_rect *clip, bool swap);
-int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb);
+int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
+			      struct drm_clip_rect *clip);
 
 struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
 int tinydrm_enable_backlight(struct backlight_device *backlight);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/6] drm/tinydrm: generalize tinydrm_xrgb8888_to_gray8()

[Noralf Trønnes]

Den 04.08.2017 00.33, skrev David Lechner:
> This adds parameters for vaddr and clip to tinydrm_xrgb8888_to_gray8() to
> make it more generic.
>
> dma_buf_{begin,end}_cpu_access() are moved out to the repaper driver.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 35 ++++++++++----------------
>   drivers/gpu/drm/tinydrm/repaper.c              | 21 +++++++++++++++-
>   include/drm/tinydrm/tinydrm-helpers.h          |  3 ++-
>   3 files changed, 35 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> index 75808bb..5915ba8 100644
> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
> @@ -185,7 +185,9 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>   /**
>    * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>    * @dst: 8-bit grayscale destination buffer
> + * @vaddr: XRGB8888 source buffer
>    * @fb: DRM framebuffer
> + * @clip: Clip rectangle area to copy
>    *
>    * Drm doesn't have native monochrome or grayscale support.
>    * Such drivers can announce the commonly supported XR24 format to userspace
> @@ -199,12 +201,11 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>    * Returns:
>    * Zero on success, negative error code on failure.
>    */
> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> +			      struct drm_clip_rect *clip)
>   {
> -	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> -	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
> -	unsigned int x, y, pitch = fb->pitches[0];
> -	int ret = 0;
> +	unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
> +	unsigned int x, y;
>   	void *buf;
>   	u32 *src;
>   
> @@ -214,22 +215,16 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
>   	 * The cma memory is write-combined so reads are uncached.
>   	 * Speed up by fetching one line at a time.
>   	 */
> -	buf = kmalloc(pitch, GFP_KERNEL);
> +	buf = kmalloc(len, GFP_KERNEL);
>   	if (!buf)
>   		return -ENOMEM;
>   
> -	if (import_attach) {
> -		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> -					       DMA_FROM_DEVICE);
> -		if (ret)
> -			goto err_free;
> -	}
> -
> -	for (y = 0; y < fb->height; y++) {
> -		src = cma_obj->vaddr + (y * pitch);
> -		memcpy(buf, src, pitch);
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		src = vaddr + (y * fb->pitches[0]);
> +		src += clip->x1;
> +		memcpy(buf, src, len);
>   		src = buf;
> -		for (x = 0; x < fb->width; x++) {
> +		for (x = clip->x1; x < clip->x2; x++) {
>   			u8 r = (*src & 0x00ff0000) >> 16;
>   			u8 g = (*src & 0x0000ff00) >> 8;
>   			u8 b =  *src & 0x000000ff;
> @@ -240,13 +235,9 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
>   		}
>   	}
>   
> -	if (import_attach)
> -		ret = dma_buf_end_cpu_access(import_attach->dmabuf,
> -					     DMA_FROM_DEVICE);
> -err_free:
>   	kfree(buf);
>   
> -	return ret;
> +	return 0;
>   }
>   EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>   
> diff --git a/drivers/gpu/drm/tinydrm/repaper.c b/drivers/gpu/drm/tinydrm/repaper.c
> index 3343d3f..d34cd9b 100644
> --- a/drivers/gpu/drm/tinydrm/repaper.c
> +++ b/drivers/gpu/drm/tinydrm/repaper.c
> @@ -18,6 +18,7 @@
>    */
>   
>   #include <linux/delay.h>
> +#include <linux/dma-buf.h>
>   #include <linux/gpio/consumer.h>
>   #include <linux/module.h>
>   #include <linux/of_device.h>
> @@ -525,11 +526,20 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
>   			    struct drm_clip_rect *clips,
>   			    unsigned int num_clips)
>   {
> +	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> +	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
>   	struct tinydrm_device *tdev = fb->dev->dev_private;
>   	struct repaper_epd *epd = epd_from_tinydrm(tdev);
> +	struct drm_clip_rect clip;
>   	u8 *buf = NULL;
>   	int ret = 0;
>   
> +	/* repaper can't do partial updates */
> +	clip.x1 = 0;
> +	clip.x2 = fb->width;
> +	clip.y1 = 0;
> +	clip.y2 = fb->height;
> +
>   	mutex_lock(&tdev->dirty_lock);
>   
>   	if (!epd->enabled)
> @@ -550,7 +560,16 @@ static int repaper_fb_dirty(struct drm_framebuffer *fb,
>   		goto out_unlock;
>   	}
>   
> -	ret = tinydrm_xrgb8888_to_gray8(buf, fb);
> +	if (import_attach) {
> +		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> +					       DMA_FROM_DEVICE);
> +		if (ret)
> +			goto out_unlock;
> +	}
> +
> +	ret = tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
> +	if (import_attach)
> +		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);

I think we can make tinydrm_xrgb8888_to_gray8() return void like the
other copy functions. We loose the error if the line buffer can't be
allocated, but that's no big deal. If we can't allocate <4k of memory,
then we have much bigger problems elsewhere.
This simplifies returning an error from dma_buf_end_cpu_access().
The error is propagated to userspace who called the dirty ioctl.

When I have switched from cma to shmem buffers backing the framebuffer
(ongoing work), we don't need the line buffer copy to speed up the
uncached reads from write combined memory mappings. But we will
probably need it on buffers imported with PRIME, since we don't know
the memory mapping. At least I don't think we can find that out.

Noralf.

>   	if (ret)
>   		goto out_unlock;
>   
> diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h
> index a6c387f..9e13ef5 100644
> --- a/include/drm/tinydrm/tinydrm-helpers.h
> +++ b/include/drm/tinydrm/tinydrm-helpers.h
> @@ -43,7 +43,8 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct drm_framebuffer *fb,
>   void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>   				struct drm_framebuffer *fb,
>   				struct drm_clip_rect *clip, bool swap);
> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb);
> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct drm_framebuffer *fb,
> +			      struct drm_clip_rect *clip);
>   
>   struct backlight_device *tinydrm_of_find_backlight(struct device *dev);
>   int tinydrm_enable_backlight(struct backlight_device *backlight);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 2/6] drm/tinydrm: generalize tinydrm_xrgb8888_to_gray8()

[Noralf Trønnes]

Den 04.08.2017 09.27, skrev Noralf Trønnes:
>
> Den 04.08.2017 00.33, skrev David Lechner:
>> This adds parameters for vaddr and clip to 
>> tinydrm_xrgb8888_to_gray8() to
>> make it more generic.
>>
>> dma_buf_{begin,end}_cpu_access() are moved out to the repaper driver.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c | 35 
>> ++++++++++----------------
>>   drivers/gpu/drm/tinydrm/repaper.c              | 21 +++++++++++++++-
>>   include/drm/tinydrm/tinydrm-helpers.h          |  3 ++-
>>   3 files changed, 35 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c 
>> b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> index 75808bb..5915ba8 100644
>> --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c
>> @@ -185,7 +185,9 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>   /**
>>    * tinydrm_xrgb8888_to_gray8 - Convert XRGB8888 to grayscale
>>    * @dst: 8-bit grayscale destination buffer
>> + * @vaddr: XRGB8888 source buffer
>>    * @fb: DRM framebuffer
>> + * @clip: Clip rectangle area to copy
>>    *
>>    * Drm doesn't have native monochrome or grayscale support.
>>    * Such drivers can announce the commonly supported XR24 format to 
>> userspace
>> @@ -199,12 +201,11 @@ EXPORT_SYMBOL(tinydrm_xrgb8888_to_rgb565);
>>    * Returns:
>>    * Zero on success, negative error code on failure.
>>    */
>> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb)
>> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct 
>> drm_framebuffer *fb,
>> +                  struct drm_clip_rect *clip)
>>   {
>> -    struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> -    struct dma_buf_attachment *import_attach = 
>> cma_obj->base.import_attach;
>> -    unsigned int x, y, pitch = fb->pitches[0];
>> -    int ret = 0;
>> +    unsigned int len = (clip->x2 - clip->x1) * sizeof(u32);
>> +    unsigned int x, y;
>>       void *buf;
>>       u32 *src;
>>   @@ -214,22 +215,16 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct 
>> drm_framebuffer *fb)
>>        * The cma memory is write-combined so reads are uncached.
>>        * Speed up by fetching one line at a time.
>>        */
>> -    buf = kmalloc(pitch, GFP_KERNEL);
>> +    buf = kmalloc(len, GFP_KERNEL);
>>       if (!buf)
>>           return -ENOMEM;
>>   -    if (import_attach) {
>> -        ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> -                           DMA_FROM_DEVICE);
>> -        if (ret)
>> -            goto err_free;
>> -    }
>> -
>> -    for (y = 0; y < fb->height; y++) {
>> -        src = cma_obj->vaddr + (y * pitch);
>> -        memcpy(buf, src, pitch);
>> +    for (y = clip->y1; y < clip->y2; y++) {
>> +        src = vaddr + (y * fb->pitches[0]);
>> +        src += clip->x1;
>> +        memcpy(buf, src, len);
>>           src = buf;
>> -        for (x = 0; x < fb->width; x++) {
>> +        for (x = clip->x1; x < clip->x2; x++) {
>>               u8 r = (*src & 0x00ff0000) >> 16;
>>               u8 g = (*src & 0x0000ff00) >> 8;
>>               u8 b =  *src & 0x000000ff;
>> @@ -240,13 +235,9 @@ int tinydrm_xrgb8888_to_gray8(u8 *dst, struct 
>> drm_framebuffer *fb)
>>           }
>>       }
>>   -    if (import_attach)
>> -        ret = dma_buf_end_cpu_access(import_attach->dmabuf,
>> -                         DMA_FROM_DEVICE);
>> -err_free:
>>       kfree(buf);
>>   -    return ret;
>> +    return 0;
>>   }
>>   EXPORT_SYMBOL(tinydrm_xrgb8888_to_gray8);
>>   diff --git a/drivers/gpu/drm/tinydrm/repaper.c 
>> b/drivers/gpu/drm/tinydrm/repaper.c
>> index 3343d3f..d34cd9b 100644
>> --- a/drivers/gpu/drm/tinydrm/repaper.c
>> +++ b/drivers/gpu/drm/tinydrm/repaper.c
>> @@ -18,6 +18,7 @@
>>    */
>>     #include <linux/delay.h>
>> +#include <linux/dma-buf.h>
>>   #include <linux/gpio/consumer.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> @@ -525,11 +526,20 @@ static int repaper_fb_dirty(struct 
>> drm_framebuffer *fb,
>>                   struct drm_clip_rect *clips,
>>                   unsigned int num_clips)
>>   {
>> +    struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> +    struct dma_buf_attachment *import_attach = 
>> cma_obj->base.import_attach;
>>       struct tinydrm_device *tdev = fb->dev->dev_private;
>>       struct repaper_epd *epd = epd_from_tinydrm(tdev);
>> +    struct drm_clip_rect clip;
>>       u8 *buf = NULL;
>>       int ret = 0;
>>   +    /* repaper can't do partial updates */
>> +    clip.x1 = 0;
>> +    clip.x2 = fb->width;
>> +    clip.y1 = 0;
>> +    clip.y2 = fb->height;
>> +
>>       mutex_lock(&tdev->dirty_lock);
>>         if (!epd->enabled)
>> @@ -550,7 +560,16 @@ static int repaper_fb_dirty(struct 
>> drm_framebuffer *fb,
>>           goto out_unlock;
>>       }
>>   -    ret = tinydrm_xrgb8888_to_gray8(buf, fb);
>> +    if (import_attach) {
>> +        ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> +                           DMA_FROM_DEVICE);
>> +        if (ret)
>> +            goto out_unlock;
>> +    }
>> +
>> +    ret = tinydrm_xrgb8888_to_gray8(buf, cma_obj->vaddr, fb, &clip);
>> +    if (import_attach)
>> +        dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
>
> I think we can make tinydrm_xrgb8888_to_gray8() return void like the
> other copy functions. We loose the error if the line buffer can't be
> allocated, but that's no big deal. If we can't allocate <4k of memory,
> then we have much bigger problems elsewhere.
> This simplifies returning an error from dma_buf_end_cpu_access().
> The error is propagated to userspace who called the dirty ioctl.
>
> When I have switched from cma to shmem buffers backing the framebuffer
> (ongoing work), we don't need the line buffer copy to speed up the
> uncached reads from write combined memory mappings. But we will
> probably need it on buffers imported with PRIME, since we don't know
> the memory mapping. At least I don't think we can find that out.
>

I realised that we can just do a slow copy, if we can't allocate a buffer.
I will sort that out in all the copy functions when we switch to shmem.
So please make tinydrm_xrgb8888_to_gray8() void.

> Noralf.
>
>>       if (ret)
>>           goto out_unlock;
>>   diff --git a/include/drm/tinydrm/tinydrm-helpers.h 
>> b/include/drm/tinydrm/tinydrm-helpers.h
>> index a6c387f..9e13ef5 100644
>> --- a/include/drm/tinydrm/tinydrm-helpers.h
>> +++ b/include/drm/tinydrm/tinydrm-helpers.h
>> @@ -43,7 +43,8 @@ void tinydrm_swab16(u16 *dst, void *vaddr, struct 
>> drm_framebuffer *fb,
>>   void tinydrm_xrgb8888_to_rgb565(u16 *dst, void *vaddr,
>>                   struct drm_framebuffer *fb,
>>                   struct drm_clip_rect *clip, bool swap);
>> -int tinydrm_xrgb8888_to_gray8(u8 *dst, struct drm_framebuffer *fb);
>> +int tinydrm_xrgb8888_to_gray8(u8 *dst, void *vaddr, struct 
>> drm_framebuffer *fb,
>> +                  struct drm_clip_rect *clip);
>>     struct backlight_device *tinydrm_of_find_backlight(struct device 
>> *dev);
>>   int tinydrm_enable_backlight(struct backlight_device *backlight);
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[David Lechner]

This adds a new binding for Sitronix ST7586 display panels.

Using lego as the vendor prefix in the compatible string because the display
panel I am working with is an integral part of the LEGO MINDSTORMS EV3.

Signed-off-by: David Lechner <david@lechnology.com>
---
 .../bindings/display/sitronix,st7586.txt           | 26 ++++++++++++++++++++++
 1 file changed, 26 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7586.txt

diff --git a/Documentation/devicetree/bindings/display/sitronix,st7586.txt b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
new file mode 100644
index 0000000..dfb0b7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
@@ -0,0 +1,26 @@
+Sitronix ST7586 display panel
+
+Required properties:
+- compatible:	"lego,ev3-lcd".
+
+The node for this driver must be a child node of a SPI controller, hence
+all mandatory properties described in ../spi/spi-bus.txt must be specified.
+
+Optional properties:
+- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
+		the panel interface operation mode (IF[3:1] pins):
+		- present: IF=011 4-wire 8-bit data serial interface
+		- absent:  IF=010 3-wire 9-bit data serial interface
+- reset-gpios:	Reset pin
+- power-supply:	A regulator node for the supply voltage.
+- backlight:	phandle of the backlight device attached to the panel
+- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
+
+Example:
+	display@0{
+		compatible = "lego,ev3-lcd";
+		reg = <0>;
+		spi-max-frequency = <10000000>;
+		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+	};
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[Noralf Trønnes]

Den 04.08.2017 00.33, skrev David Lechner:
> This adds a new binding for Sitronix ST7586 display panels.
>
> Using lego as the vendor prefix in the compatible string because the display
> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.

Is this display available outside of this Lego part?
If not you can remove the properties you don't need for this particular
display setup. Another st7586 display with a different panel would need
a different initialization sequence and compatible string, so we can
add properties when/if that happens.

Noralf.

> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>   .../bindings/display/sitronix,st7586.txt           | 26 ++++++++++++++++++++++
>   1 file changed, 26 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/display/sitronix,st7586.txt
>
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7586.txt b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> new file mode 100644
> index 0000000..dfb0b7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible:	"lego,ev3-lcd".
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in ../spi/spi-bus.txt must be specified.
> +
> +Optional properties:
> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> +		the panel interface operation mode (IF[3:1] pins):
> +		- present: IF=011 4-wire 8-bit data serial interface
> +		- absent:  IF=010 3-wire 9-bit data serial interface
> +- reset-gpios:	Reset pin
> +- power-supply:	A regulator node for the supply voltage.
> +- backlight:	phandle of the backlight device attached to the panel
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> +
> +Example:
> +	display@0{
> +		compatible = "lego,ev3-lcd";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +	};

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[David Lechner]

On 08/04/2017 04:48 AM, Noralf Trønnes wrote:
> 
> Den 04.08.2017 00.33, skrev David Lechner:
>> This adds a new binding for Sitronix ST7586 display panels.
>>
>> Using lego as the vendor prefix in the compatible string because the 
>> display
>> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.
> 
> Is this display available outside of this Lego part?
No, it is not.

> If not you can remove the properties you don't need for this particular
> display setup. Another st7586 display with a different panel would need
> a different initialization sequence and compatible string, so we can
> add properties when/if that happens.

OK. so I will drop power-supply and backlight.

Should I remove these from the driver as well? There are some panels out 
there that could use them.[1]

[1]: 
http://www.buydisplay.com/download/manual/ERC240160-1_Series_Datasheet.pdf

> 
> Noralf.
> 
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   .../bindings/display/sitronix,st7586.txt           | 26 
>> ++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>   create mode 100644 
>> Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>
>> diff --git 
>> a/Documentation/devicetree/bindings/display/sitronix,st7586.txt 
>> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> new file mode 100644
>> index 0000000..dfb0b7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> @@ -0,0 +1,26 @@
>> +Sitronix ST7586 display panel
>> +
>> +Required properties:
>> +- compatible:    "lego,ev3-lcd".
>> +
>> +The node for this driver must be a child node of a SPI controller, hence
>> +all mandatory properties described in ../spi/spi-bus.txt must be 
>> specified.
>> +
>> +Optional properties:
>> +- dc-gpios:    D/C pin. The presence/absence of this GPIO determines
>> +        the panel interface operation mode (IF[3:1] pins):
>> +        - present: IF=011 4-wire 8-bit data serial interface
>> +        - absent:  IF=010 3-wire 9-bit data serial interface
>> +- reset-gpios:    Reset pin
>> +- power-supply:    A regulator node for the supply voltage.
>> +- backlight:    phandle of the backlight device attached to the panel
>> +- rotation:    panel rotation in degrees counter clockwise 
>> (0,90,180,270)
>> +
>> +Example:
>> +    display@0{
>> +        compatible = "lego,ev3-lcd";
>> +        reg = <0>;
>> +        spi-max-frequency = <10000000>;
>> +        dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
>> +        reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
>> +    };
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[Noralf Trønnes]

Den 04.08.2017 18.51, skrev David Lechner:
> On 08/04/2017 04:48 AM, Noralf Trønnes wrote:
>>
>> Den 04.08.2017 00.33, skrev David Lechner:
>>> This adds a new binding for Sitronix ST7586 display panels.
>>>
>>> Using lego as the vendor prefix in the compatible string because the 
>>> display
>>> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.
>>
>> Is this display available outside of this Lego part?
> No, it is not.
>
>> If not you can remove the properties you don't need for this particular
>> display setup. Another st7586 display with a different panel would need
>> a different initialization sequence and compatible string, so we can
>> add properties when/if that happens.
>
> OK. so I will drop power-supply and backlight.
>

And the dc/reset gpios should be required properties since your display
won't work without them (or rather they're hardwired that way).

> Should I remove these from the driver as well? There are some panels 
> out there that could use them.[1]
>
> [1]: 
> http://www.buydisplay.com/download/manual/ERC240160-1_Series_Datasheet.pdf
>

Yes, because that display won't work with the driver as-is, so it's
really dead code. If someone wants to add support for that display,
they'll add the necessary code. When maintaining code for 10-20 years,
it's important to not add code that 'might' be used in the future.
Let the future take care of itself and keep the codebase slim :-)

Noralf.

>>
>> Noralf.
>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> ---
>>>   .../bindings/display/sitronix,st7586.txt           | 26 
>>> ++++++++++++++++++++++
>>>   1 file changed, 26 insertions(+)
>>>   create mode 100644 
>>> Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>>
>>> diff --git 
>>> a/Documentation/devicetree/bindings/display/sitronix,st7586.txt 
>>> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>> new file mode 100644
>>> index 0000000..dfb0b7b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>> @@ -0,0 +1,26 @@
>>> +Sitronix ST7586 display panel
>>> +
>>> +Required properties:
>>> +- compatible:    "lego,ev3-lcd".
>>> +
>>> +The node for this driver must be a child node of a SPI controller, 
>>> hence
>>> +all mandatory properties described in ../spi/spi-bus.txt must be 
>>> specified.
>>> +
>>> +Optional properties:
>>> +- dc-gpios:    D/C pin. The presence/absence of this GPIO determines
>>> +        the panel interface operation mode (IF[3:1] pins):
>>> +        - present: IF=011 4-wire 8-bit data serial interface
>>> +        - absent:  IF=010 3-wire 9-bit data serial interface
>>> +- reset-gpios:    Reset pin
>>> +- power-supply:    A regulator node for the supply voltage.
>>> +- backlight:    phandle of the backlight device attached to the panel
>>> +- rotation:    panel rotation in degrees counter clockwise 
>>> (0,90,180,270)
>>> +
>>> +Example:
>>> +    display@0{
>>> +        compatible = "lego,ev3-lcd";
>>> +        reg = <0>;
>>> +        spi-max-frequency = <10000000>;
>>> +        dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
>>> +        reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
>>> +    };
>>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Thursday 03 Aug 2017 17:33:47 David Lechner wrote:
> This adds a new binding for Sitronix ST7586 display panels.
> 
> Using lego as the vendor prefix in the compatible string because the display
> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.
> 
> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
> ---
>  .../bindings/display/sitronix,st7586.txt           | 26 +++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/display/sitronix,st7586.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt new file
> mode 100644
> index 0000000..dfb0b7b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> @@ -0,0 +1,26 @@
> +Sitronix ST7586 display panel
> +
> +Required properties:
> +- compatible:	"lego,ev3-lcd".
> +
> +The node for this driver must be a child node of a SPI controller, hence
> +all mandatory properties described in ../spi/spi-bus.txt must be specified.
> +
> +Optional properties:
> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> +		the panel interface operation mode (IF[3:1] pins):
> +		- present: IF=011 4-wire 8-bit data serial interface
> +		- absent:  IF=010 3-wire 9-bit data serial interface

How does this work ? Do you have a single GPIO on your system connected to 
IF[1], with IF[3:2] hardwired to 01 ?

> +- reset-gpios:	Reset pin
> +- power-supply:	A regulator node for the supply voltage.
> +- backlight:	phandle of the backlight device attached to the panel
> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)

Please use the OF graph DT bindings (a.k.a. ports) to describe the connection 
between the panel and its source.

> +Example:
> +	display@0{
> +		compatible = "lego,ev3-lcd";
> +		reg = <0>;
> +		spi-max-frequency = <10000000>;
> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> +	};

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[David Lechner]

On 08/04/2017 09:54 AM, Laurent Pinchart wrote:
> Hi David,
> 
> Thank you for the patch.
> 
> On Thursday 03 Aug 2017 17:33:47 David Lechner wrote:
>> This adds a new binding for Sitronix ST7586 display panels.
>>
>> Using lego as the vendor prefix in the compatible string because the display
>> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   .../bindings/display/sitronix,st7586.txt           | 26 +++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt new file
>> mode 100644
>> index 0000000..dfb0b7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> @@ -0,0 +1,26 @@
>> +Sitronix ST7586 display panel
>> +
>> +Required properties:
>> +- compatible:	"lego,ev3-lcd".
>> +
>> +The node for this driver must be a child node of a SPI controller, hence
>> +all mandatory properties described in ../spi/spi-bus.txt must be specified.
>> +
>> +Optional properties:
>> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
>> +		the panel interface operation mode (IF[3:1] pins):
>> +		- present: IF=011 4-wire 8-bit data serial interface
>> +		- absent:  IF=010 3-wire 9-bit data serial interface
> 
> How does this work ? Do you have a single GPIO on your system connected to
> IF[1], with IF[3:2] hardwired to 01 ?

LEGO has not made the internals of the display publicly available, so I 
cannot say for sure. But I assume that IF[3:1] is hardwired to 011. This 
causes pin D1 to assigned to the signal A0, which is what we are calling 
the dc gpio here.

If IF[3:1] were hardwired to 010, then pin D1 would be not not used and 
there would be no A0 signal.

So, basically, we can infer the state of IF[3:1] by the fact that we 
have a dc pin or not.

> 
>> +- reset-gpios:	Reset pin
>> +- power-supply:	A regulator node for the supply voltage.
>> +- backlight:	phandle of the backlight device attached to the panel
>> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> 
> Please use the OF graph DT bindings (a.k.a. ports) to describe the connection
> between the panel and its source.

I am afraid that I do not understand this request. What would the source 
of the panel be? There is nothing like a SoC LCD controller that is 
driving this panel.

> 
>> +Example:
>> +	display@0{
>> +		compatible = "lego,ev3-lcd";
>> +		reg = <0>;
>> +		spi-max-frequency = <10000000>;
>> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
>> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
>> +	};
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[Laurent Pinchart]

Hi David,

On Friday 04 Aug 2017 10:51:37 David Lechner wrote:
> On 08/04/2017 09:54 AM, Laurent Pinchart wrote:
> > On Thursday 03 Aug 2017 17:33:47 David Lechner wrote:
> >> This adds a new binding for Sitronix ST7586 display panels.
> >> 
> >> Using lego as the vendor prefix in the compatible string because the
> >> display panel I am working with is an integral part of the LEGO
> >> MINDSTORMS EV3.
> >> 
> >> Signed-off-by: David Lechner <david@lechnology.com>
> >> ---
> >> 
> >>   .../bindings/display/sitronix,st7586.txt           | 26
> >>   +++++++++++++++++++
> >>   1 file changed, 26 insertions(+)
> >>   create mode 100644
> >> 
> >> Documentation/devicetree/bindings/display/sitronix,st7586.txt
> >> 
> >> diff --git
> >> a/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> >> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt new file
> >> mode 100644
> >> index 0000000..dfb0b7b
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
> >> @@ -0,0 +1,26 @@
> >> +Sitronix ST7586 display panel
> >> +
> >> +Required properties:
> >> +- compatible:	"lego,ev3-lcd".
> >> +
> >> +The node for this driver must be a child node of a SPI controller, hence
> >> +all mandatory properties described in ../spi/spi-bus.txt must be
> >> specified. +
> >> +Optional properties:
> >> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
> >> +		the panel interface operation mode (IF[3:1] pins):
> >> +		- present: IF=011 4-wire 8-bit data serial interface
> >> +		- absent:  IF=010 3-wire 9-bit data serial interface
> > 
> > How does this work ? Do you have a single GPIO on your system connected to
> > IF[1], with IF[3:2] hardwired to 01 ?
> 
> LEGO has not made the internals of the display publicly available, so I
> cannot say for sure. But I assume that IF[3:1] is hardwired to 011. This
> causes pin D1 to assigned to the signal A0, which is what we are calling
> the dc gpio here.
> 
> If IF[3:1] were hardwired to 010, then pin D1 would be not not used and
> there would be no A0 signal.
> 
> So, basically, we can infer the state of IF[3:1] by the fact that we
> have a dc pin or not.

OK, now I understand what you mean. Maybe you should phrase it a bit 
differently to make it clearer ? How about

dc-gpios: Specified or the GPIO connected to the panel's D/C pin (also called 
A0). The property is required when the panel operates in 4-wire mode (IF[3:1] 
= 011) and prohibited when the panel operates in 3-wire mode (IF[3:1] = 010).

By the way, if the signal is named A0, why don't you call the property a0-
gpios ?

> >> +- reset-gpios:	Reset pin
> >> +- power-supply:	A regulator node for the supply voltage.
> >> +- backlight:	phandle of the backlight device attached to the panel
> >> +- rotation:	panel rotation in degrees counter clockwise 
(0,90,180,270)
> > 
> > Please use the OF graph DT bindings (a.k.a. ports) to describe the
> > connection between the panel and its source.
> 
> I am afraid that I do not understand this request. What would the source
> of the panel be? There is nothing like a SoC LCD controller that is
> driving this panel.

My bad, I should have read the panel datasheet before replying :-S Please 
ignore this comment.

> >> +Example:
> >> +	display@0{
> >> +		compatible = "lego,ev3-lcd";
> >> +		reg = <0>;
> >> +		spi-max-frequency = <10000000>;
> >> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
> >> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
> >> +	};

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[David Lechner]

On 08/04/2017 02:39 PM, Laurent Pinchart wrote:
> Hi David,
> 
> On Friday 04 Aug 2017 10:51:37 David Lechner wrote:
>> On 08/04/2017 09:54 AM, Laurent Pinchart wrote:
>>> On Thursday 03 Aug 2017 17:33:47 David Lechner wrote:
>>>> This adds a new binding for Sitronix ST7586 display panels.
>>>>
>>>> Using lego as the vendor prefix in the compatible string because the
>>>> display panel I am working with is an integral part of the LEGO
>>>> MINDSTORMS EV3.
>>>>
>>>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>>>> ---
>>>>
>>>>    .../bindings/display/sitronix,st7586.txt           | 26
>>>>    +++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>    create mode 100644
>>>>
>>>> Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>>> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt new file
>>>> mode 100644
>>>> index 0000000..dfb0b7b
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>>> @@ -0,0 +1,26 @@
>>>> +Sitronix ST7586 display panel
>>>> +
>>>> +Required properties:
>>>> +- compatible:	"lego,ev3-lcd".
>>>> +
>>>> +The node for this driver must be a child node of a SPI controller, hence
>>>> +all mandatory properties described in ../spi/spi-bus.txt must be
>>>> specified. +
>>>> +Optional properties:
>>>> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
>>>> +		the panel interface operation mode (IF[3:1] pins):
>>>> +		- present: IF=011 4-wire 8-bit data serial interface
>>>> +		- absent:  IF=010 3-wire 9-bit data serial interface
>>>
>>> How does this work ? Do you have a single GPIO on your system connected to
>>> IF[1], with IF[3:2] hardwired to 01 ?
>>
>> LEGO has not made the internals of the display publicly available, so I
>> cannot say for sure. But I assume that IF[3:1] is hardwired to 011. This
>> causes pin D1 to assigned to the signal A0, which is what we are calling
>> the dc gpio here.
>>
>> If IF[3:1] were hardwired to 010, then pin D1 would be not not used and
>> there would be no A0 signal.
>>
>> So, basically, we can infer the state of IF[3:1] by the fact that we
>> have a dc pin or not.
> 
> OK, now I understand what you mean. Maybe you should phrase it a bit
> differently to make it clearer ? How about
> 
> dc-gpios: Specified or the GPIO connected to the panel's D/C pin (also called
> A0). The property is required when the panel operates in 4-wire mode (IF[3:1]
> = 011) and prohibited when the panel operates in 3-wire mode (IF[3:1] = 010).

Yes, this is more clear. Thank you for the suggestion.

> 
> By the way, if the signal is named A0, why don't you call the property a0-
> gpios ?

I consider "dc-gpios" to be a generic name since it is used by many 
different panels. But I would be OK with calling it "a0-gpios" as well. 
It will just require more explanation that this is the A0 *signal* and 
not the A0 *pin*. The actual pin used is labeled *D1* on the controller.

On the other hand, it is labeled as A0 on LEGO's schematic, so perhaps 
a0 is better.

> 
>>>> +- reset-gpios:	Reset pin
>>>> +- power-supply:	A regulator node for the supply voltage.
>>>> +- backlight:	phandle of the backlight device attached to the panel
>>>> +- rotation:	panel rotation in degrees counter clockwise
> (0,90,180,270)
>>>
>>> Please use the OF graph DT bindings (a.k.a. ports) to describe the
>>> connection between the panel and its source.
>>
>> I am afraid that I do not understand this request. What would the source
>> of the panel be? There is nothing like a SoC LCD controller that is
>> driving this panel.
> 
> My bad, I should have read the panel datasheet before replying :-S Please
> ignore this comment.
> 
>>>> +Example:
>>>> +	display@0{
>>>> +		compatible = "lego,ev3-lcd";
>>>> +		reg = <0>;
>>>> +		spi-max-frequency = <10000000>;
>>>> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
>>>> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
>>>> +	};
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 3/6] dt-bindings: add binding for Sitronix ST7586 display panels

[Noralf Trønnes]

Hi Laurent,


Den 04.08.2017 16.54, skrev Laurent Pinchart:
> Hi David,
>
> Thank you for the patch.
>
> On Thursday 03 Aug 2017 17:33:47 David Lechner wrote:
>> This adds a new binding for Sitronix ST7586 display panels.
>>
>> Using lego as the vendor prefix in the compatible string because the display
>> panel I am working with is an integral part of the LEGO MINDSTORMS EV3.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>   .../bindings/display/sitronix,st7586.txt           | 26 +++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>   create mode 100644
>> Documentation/devicetree/bindings/display/sitronix,st7586.txt
>>
>> diff --git a/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> b/Documentation/devicetree/bindings/display/sitronix,st7586.txt new file
>> mode 100644
>> index 0000000..dfb0b7b
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/sitronix,st7586.txt
>> @@ -0,0 +1,26 @@
>> +Sitronix ST7586 display panel
>> +
>> +Required properties:
>> +- compatible:	"lego,ev3-lcd".
>> +
>> +The node for this driver must be a child node of a SPI controller, hence
>> +all mandatory properties described in ../spi/spi-bus.txt must be specified.
>> +
>> +Optional properties:
>> +- dc-gpios:	D/C pin. The presence/absence of this GPIO determines
>> +		the panel interface operation mode (IF[3:1] pins):
>> +		- present: IF=011 4-wire 8-bit data serial interface
>> +		- absent:  IF=010 3-wire 9-bit data serial interface
> How does this work ? Do you have a single GPIO on your system connected to
> IF[1], with IF[3:2] hardwired to 01 ?
>
>> +- reset-gpios:	Reset pin
>> +- power-supply:	A regulator node for the supply voltage.
>> +- backlight:	phandle of the backlight device attached to the panel
>> +- rotation:	panel rotation in degrees counter clockwise (0,90,180,270)
> Please use the OF graph DT bindings (a.k.a. ports) to describe the connection
> between the panel and its source.

This display has a controller with onboard memory that is scanned out
to the panel. The controller is connected, in this instance, through SPI.
Both initialization and pixel data is transferred over SPI. It resembles
the MIPI DCS/DBI standard except that it misses some of the commands and
has non standard formats: 2-bit greyscale and monochrome. MIPI DBI only
supports rgb formats (3, 8, 12, 16 and 18-bits). So it isn't a drm panel
in the sense as one connected through MIPI DSI or MIPI DPI.

MIPI DBI has 3 interface types:
- 8/9/16/18 bit parallel bus + Data/Command signal (8080 or motorola bus)
- 8/16 bit SPI + D/C signal
- 9 bit SPI (D/C as first bit)

Noralf.

>> +Example:
>> +	display@0{
>> +		compatible = "lego,ev3-lcd";
>> +		reg = <0>;
>> +		spi-max-frequency = <10000000>;
>> +		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
>> +		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
>> +	};

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 4/6] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[David Lechner]

LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
module for the ST7586 controller with parameters for the LEGO MINDSTORMS
EV3 LCD display.

Signed-off-by: David Lechner <david@lechnology.com>
---
 MAINTAINERS                      |   6 +
 drivers/gpu/drm/tinydrm/Kconfig  |  10 +
 drivers/gpu/drm/tinydrm/Makefile |   1 +
 drivers/gpu/drm/tinydrm/st7586.c | 466 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 483 insertions(+)
 create mode 100644 drivers/gpu/drm/tinydrm/st7586.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a1e772e..9643f95 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4365,6 +4365,12 @@ S:	Maintained
 F:	drivers/gpu/drm/tinydrm/repaper.c
 F:	Documentation/devicetree/bindings/display/repaper.txt
 
+DRM DRIVER FOR SITRONIX ST7586 PANELS
+M:	David Lechner <david@lechnology.com>
+S:	Maintained
+F:	drivers/gpu/drm/tinydrm/st7586.c
+F:	Documentation/devicetree/bindings/display/st7586.txt
+
 DRM DRIVER FOR RAGE 128 VIDEO CARDS
 S:	Orphan / Obsolete
 F:	drivers/gpu/drm/r128/
diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
index f17c3ca..2e790e7 100644
--- a/drivers/gpu/drm/tinydrm/Kconfig
+++ b/drivers/gpu/drm/tinydrm/Kconfig
@@ -32,3 +32,13 @@ config TINYDRM_REPAPER
 	  2.71" TFT EPD Panel (E2271CS021)
 
 	  If M is selected the module will be called repaper.
+
+config TINYDRM_ST7586
+	tristate "DRM support for Sitronix ST7586 display panels"
+	depends on DRM_TINYDRM && SPI
+	select TINYDRM_MIPI_DBI
+	help
+	  DRM driver for the following Sitronix ST7586 panels:
+	  * LEGO MINDSTORMS EV3
+
+	  If M is selected the module will be called st7586.
diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
index 95bb4d4..0c184bd 100644
--- a/drivers/gpu/drm/tinydrm/Makefile
+++ b/drivers/gpu/drm/tinydrm/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
 # Displays
 obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
 obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
+obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
new file mode 100644
index 0000000..11e226d
--- /dev/null
+++ b/drivers/gpu/drm/tinydrm/st7586.c
@@ -0,0 +1,466 @@
+/*
+ * DRM driver for Sitronix ST7586 panels
+ *
+ * Copyright 2017 David Lechner <david@lechnology.com>
+ *
+ * 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 <linux/delay.h>
+#include <linux/dma-buf.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <video/mipi_display.h>
+
+#include <drm/tinydrm/mipi-dbi.h>
+#include <drm/tinydrm/tinydrm-helpers.h>
+
+/* controller-specific commands */
+#define ST7586_DISP_MODE_GRAY	0x38
+#define ST7586_DISP_MODE_MONO	0x39
+#define ST7586_ENABLE_DDRAM	0x3a
+#define ST7586_SET_DISP_DUTY	0xb0
+#define ST7586_SET_PART_DISP	0xb4
+#define ST7586_SET_NLINE_INV	0xb5
+#define ST7586_SET_VOP		0xc0
+#define ST7586_SET_BIAS_SYSTEM	0xc3
+#define ST7586_SET_BOOST_LEVEL	0xc4
+#define ST7586_SET_VOP_OFFSET	0xc7
+#define ST7586_ENABLE_ANALOG	0xd0
+#define ST7586_AUTO_READ_CTRL	0xd7
+#define ST7586_OTP_RW_CTRL	0xe0
+#define ST7586_OTP_CTRL_OUT	0xe1
+#define ST7586_OTP_READ		0xe3
+
+#define ST7586_DISP_CTRL_MX	BIT(6)
+#define ST7586_DISP_CTRL_MY	BIT(7)
+
+static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
+				       struct drm_framebuffer *fb,
+				       struct drm_clip_rect *clip)
+{
+	size_t len = (clip->x2 - clip->x1) * (clip->y2 - clip->y1);
+	unsigned int x, y;
+	u8 *src, *buf, val;
+
+	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
+	clip->x1 = clip->x1 / 3 * 3;
+	clip->x2 = (clip->x2 + 2) / 3 * 3;
+
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	tinydrm_xrgb8888_to_gray8(buf, vaddr, fb, clip);
+	src = buf;
+
+	for (y = clip->y1; y < clip->y2; y++) {
+		for (x = clip->x1; x < clip->x2; x += 3) {
+			val = *src++ & 0xc0;
+			if (val & 0xc0)
+				val |= 0x20;
+			val |= (*src++ & 0xc0) >> 3;
+			if (val & 0x18)
+				val |= 0x04;
+			val |= *src++ >> 6;
+			*dst++ = ~val;
+		}
+	}
+
+	/* now adjust the clip so it applies to dst */
+	clip->x1 /= 3;
+	clip->x2 /= 3;
+
+	kfree(buf);
+}
+
+static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
+			   struct drm_clip_rect *clip)
+{
+	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
+	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
+	struct drm_format_name_buf format_name;
+	void *src = cma_obj->vaddr;
+	int ret = 0;
+
+	if (import_attach) {
+		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
+					       DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+	}
+
+	switch (fb->format->format) {
+	case DRM_FORMAT_XRGB8888:
+		st7586_xrgb8888_to_gray332(dst, src, fb, clip);
+		break;
+	default:
+		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
+			     drm_get_format_name(fb->format->format,
+						 &format_name));
+		ret = -EINVAL;
+		break;
+	}
+
+	if (import_attach)
+		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
+
+	return ret;
+}
+
+static int st7586_fb_dirty(struct drm_framebuffer *fb,
+			   struct drm_file *file_priv, unsigned int flags,
+			   unsigned int color, struct drm_clip_rect *clips,
+			   unsigned int num_clips)
+{
+	struct tinydrm_device *tdev = fb->dev->dev_private;
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+	struct drm_clip_rect clip;
+	int ret = 0;
+
+	mutex_lock(&tdev->dirty_lock);
+
+	if (!mipi->enabled)
+		goto out_unlock;
+
+	/* fbdev can flush even when we're not interested */
+	if (tdev->pipe.plane.fb != fb)
+		goto out_unlock;
+
+	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
+			    fb->height);
+
+	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
+		  clip.x1, clip.x2, clip.y1, clip.y2);
+
+	ret = st7586_buf_copy(mipi->tx_buf, fb, &clip);
+	if (ret)
+		goto out_unlock;
+
+	/* NB: st7586_buf_copy() modifies clip */
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
+			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
+			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
+			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
+			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
+
+	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
+				   (u8 *)mipi->tx_buf,
+				   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
+
+out_unlock:
+	mutex_unlock(&tdev->dirty_lock);
+
+	if (ret)
+		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
+			     ret);
+
+	return ret;
+}
+
+static const struct drm_framebuffer_funcs st7586_fb_funcs = {
+	.destroy	= drm_fb_cma_destroy,
+	.create_handle	= drm_fb_cma_create_handle,
+	.dirty		= st7586_fb_dirty,
+};
+
+void st7586_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 drm_framebuffer *fb = pipe->plane.fb;
+	struct device *dev = tdev->drm->dev;
+	int ret;
+	u8 addr_mode;
+
+	DRM_DEBUG_KMS("\n");
+
+	ret = regulator_enable(mipi->regulator);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulator %d\n", ret);
+		return;
+	}
+
+	mipi_dbi_hw_reset(mipi);
+	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
+	if (ret) {
+		dev_err(dev, "Error sending command %d\n", ret);
+		regulator_disable(mipi->regulator);
+		return;
+	}
+
+	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
+
+	msleep(10);
+
+	mipi_dbi_command(mipi, ST7586_OTP_READ);
+
+	msleep(20);
+
+	mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+
+	msleep(50);
+
+	mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
+	mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
+	mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
+	mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
+	mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
+	mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
+	mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
+	mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
+
+	switch (mipi->rotation) {
+	default:
+		addr_mode = 0x00;
+		break;
+	case 90:
+		addr_mode = ST7586_DISP_CTRL_MY;
+		break;
+	case 180:
+		addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
+		break;
+	case 270:
+		addr_mode = ST7586_DISP_CTRL_MX;
+		break;
+	}
+	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
+
+	mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
+	mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
+	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
+
+	msleep(100);
+
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
+
+	mipi->enabled = true;
+
+	if (fb)
+		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
+
+	tinydrm_enable_backlight(mipi->backlight);
+}
+
+static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
+{
+	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
+	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
+
+	DRM_DEBUG_KMS("\n");
+
+	if (!mipi->enabled)
+		return;
+
+	mipi->enabled = false;
+	tinydrm_disable_backlight(mipi->backlight);
+	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
+	regulator_disable(mipi->regulator);
+}
+
+static const u32 st7586_formats[] = {
+	DRM_FORMAT_XRGB8888,
+};
+
+static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
+		const struct drm_simple_display_pipe_funcs *pipe_funcs,
+		struct drm_driver *driver, const struct drm_display_mode *mode,
+		unsigned int rotation)
+{
+	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
+	struct tinydrm_device *tdev = &mipi->tinydrm;
+	int ret;
+
+	mutex_init(&mipi->cmdlock);
+
+	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
+	if (!mipi->tx_buf)
+		return -ENOMEM;
+
+	ret = devm_tinydrm_init(dev, tdev, &st7586_fb_funcs, driver);
+	if (ret)
+		return ret;
+
+	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
+					DRM_MODE_CONNECTOR_VIRTUAL,
+					st7586_formats,
+					ARRAY_SIZE(st7586_formats),
+					mode, rotation);
+	if (ret)
+		return ret;
+
+	tdev->drm->mode_config.preferred_depth = 32;
+	mipi->rotation = rotation;
+
+	drm_mode_config_reset(tdev->drm);
+
+	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
+		      tdev->drm->mode_config.preferred_depth, rotation);
+
+	return 0;
+}
+
+static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
+	.enable		= st7586_pipe_enable,
+	.disable	= st7586_pipe_disable,
+	.update		= tinydrm_display_pipe_update,
+	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
+};
+
+static const struct drm_display_mode st7586_mode = {
+	TINYDRM_MODE(178, 128, 37, 27),
+};
+
+DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
+
+static struct drm_driver st7586_driver = {
+	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
+				  DRIVER_ATOMIC,
+	.fops			= &st7586_fops,
+	TINYDRM_GEM_DRIVER_OPS,
+	.lastclose		= tinydrm_lastclose,
+	.debugfs_init		= mipi_dbi_debugfs_init,
+	.name			= "st7586",
+	.desc			= "Sitronix ST7586",
+	.date			= "20170801",
+	.major			= 1,
+	.minor			= 0,
+};
+
+static const struct of_device_id st7586_of_match[] = {
+	{ .compatible = "lego,ev3-lcd" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, st7586_of_match);
+
+static const struct spi_device_id st7586_id[] = {
+	{ "ev3-lcd", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, st7586_id);
+
+static int st7586_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct tinydrm_device *tdev;
+	struct mipi_dbi *mipi;
+	struct gpio_desc *dc;
+	u32 rotation = 0;
+	int ret;
+
+	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
+	if (!mipi)
+		return -ENOMEM;
+
+	mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
+	if (IS_ERR(mipi->reset)) {
+		dev_err(dev, "Failed to get gpio 'reset'\n");
+		return PTR_ERR(mipi->reset);
+	}
+
+	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
+	if (IS_ERR(dc)) {
+		dev_err(dev, "Failed to get gpio 'dc'\n");
+		return PTR_ERR(dc);
+	}
+
+	mipi->regulator = devm_regulator_get(dev, "power");
+	if (IS_ERR(mipi->regulator))
+		return PTR_ERR(mipi->regulator);
+
+	mipi->backlight = tinydrm_of_find_backlight(dev);
+	if (IS_ERR(mipi->backlight))
+		return PTR_ERR(mipi->backlight);
+
+	device_property_read_u32(dev, "rotation", &rotation);
+
+	ret = mipi_dbi_spi_init(spi, mipi, dc);
+	if (ret)
+		return ret;
+
+	/* Cannot read from this controller via SPI */
+	mipi->read_commands = NULL;
+
+	/*
+	 * we are using 8-bit data, so we are not actually swapping anything,
+	 * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
+	 * right thing and not use 16-bit transfers (which results in swapped
+	 * bytes on little-endian systems and causes out of order data to be
+	 * sent to the display).
+	 */
+	mipi->swap_bytes = true;
+
+	ret = st7586_init(&spi->dev, mipi, &st7586_pipe_funcs, &st7586_driver,
+			  &st7586_mode, rotation);
+	if (ret)
+		return ret;
+
+	tdev = &mipi->tinydrm;
+
+	ret = devm_tinydrm_register(tdev);
+	if (ret)
+		return ret;
+
+	spi_set_drvdata(spi, mipi);
+
+	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
+			 tdev->drm->driver->name, dev_name(dev),
+			 spi->max_speed_hz / 1000000,
+			 tdev->drm->primary->index);
+
+	return 0;
+}
+
+static void st7586_shutdown(struct spi_device *spi)
+{
+	struct mipi_dbi *mipi = spi_get_drvdata(spi);
+
+	tinydrm_shutdown(&mipi->tinydrm);
+}
+
+static int __maybe_unused st7586_pm_suspend(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+
+	return tinydrm_suspend(&mipi->tinydrm);
+}
+
+static int __maybe_unused st7586_pm_resume(struct device *dev)
+{
+	struct mipi_dbi *mipi = dev_get_drvdata(dev);
+
+	return tinydrm_resume(&mipi->tinydrm);
+}
+
+static const struct dev_pm_ops st7586_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
+};
+
+static struct spi_driver st7586_spi_driver = {
+	.driver = {
+		.name = "st7586",
+		.owner = THIS_MODULE,
+		.of_match_table = st7586_of_match,
+		.pm = &st7586_pm_ops,
+	},
+	.id_table = st7586_id,
+	.probe = st7586_probe,
+	.shutdown = st7586_shutdown,
+};
+module_spi_driver(st7586_spi_driver);
+
+MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
+MODULE_AUTHOR("David Lechner <david@lechnology.com>");
+MODULE_LICENSE("GPL");
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 4/6] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Noralf Trønnes]

Den 04.08.2017 00.33, skrev David Lechner:
> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
> module for the ST7586 controller with parameters for the LEGO MINDSTORMS
> EV3 LCD display.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>   MAINTAINERS                      |   6 +
>   drivers/gpu/drm/tinydrm/Kconfig  |  10 +
>   drivers/gpu/drm/tinydrm/Makefile |   1 +
>   drivers/gpu/drm/tinydrm/st7586.c | 466 +++++++++++++++++++++++++++++++++++++++
>   4 files changed, 483 insertions(+)
>   create mode 100644 drivers/gpu/drm/tinydrm/st7586.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a1e772e..9643f95 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4365,6 +4365,12 @@ S:	Maintained
>   F:	drivers/gpu/drm/tinydrm/repaper.c
>   F:	Documentation/devicetree/bindings/display/repaper.txt
>   
> +DRM DRIVER FOR SITRONIX ST7586 PANELS
> +M:	David Lechner <david@lechnology.com>
> +S:	Maintained
> +F:	drivers/gpu/drm/tinydrm/st7586.c
> +F:	Documentation/devicetree/bindings/display/st7586.txt
> +

This file is sorted alphabetically, so it should go after SIS.
It seems REPAPER ended up in the wrong place after the conflict
resolution following Linus's cleanup.

>   DRM DRIVER FOR RAGE 128 VIDEO CARDS
>   S:	Orphan / Obsolete
>   F:	drivers/gpu/drm/r128/
> diff --git a/drivers/gpu/drm/tinydrm/Kconfig b/drivers/gpu/drm/tinydrm/Kconfig
> index f17c3ca..2e790e7 100644
> --- a/drivers/gpu/drm/tinydrm/Kconfig
> +++ b/drivers/gpu/drm/tinydrm/Kconfig
> @@ -32,3 +32,13 @@ config TINYDRM_REPAPER
>   	  2.71" TFT EPD Panel (E2271CS021)
>   
>   	  If M is selected the module will be called repaper.
> +
> +config TINYDRM_ST7586
> +	tristate "DRM support for Sitronix ST7586 display panels"
> +	depends on DRM_TINYDRM && SPI
> +	select TINYDRM_MIPI_DBI
> +	help
> +	  DRM driver for the following Sitronix ST7586 panels:
> +	  * LEGO MINDSTORMS EV3
> +
> +	  If M is selected the module will be called st7586.
> diff --git a/drivers/gpu/drm/tinydrm/Makefile b/drivers/gpu/drm/tinydrm/Makefile
> index 95bb4d4..0c184bd 100644
> --- a/drivers/gpu/drm/tinydrm/Makefile
> +++ b/drivers/gpu/drm/tinydrm/Makefile
> @@ -6,3 +6,4 @@ obj-$(CONFIG_TINYDRM_MIPI_DBI)		+= mipi-dbi.o
>   # Displays
>   obj-$(CONFIG_TINYDRM_MI0283QT)		+= mi0283qt.o
>   obj-$(CONFIG_TINYDRM_REPAPER)		+= repaper.o
> +obj-$(CONFIG_TINYDRM_ST7586)		+= st7586.o
> diff --git a/drivers/gpu/drm/tinydrm/st7586.c b/drivers/gpu/drm/tinydrm/st7586.c
> new file mode 100644
> index 0000000..11e226d
> --- /dev/null
> +++ b/drivers/gpu/drm/tinydrm/st7586.c
> @@ -0,0 +1,466 @@
> +/*
> + * DRM driver for Sitronix ST7586 panels
> + *
> + * Copyright 2017 David Lechner <david@lechnology.com>
> + *
> + * 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 <linux/delay.h>
> +#include <linux/dma-buf.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <video/mipi_display.h>
> +
> +#include <drm/tinydrm/mipi-dbi.h>
> +#include <drm/tinydrm/tinydrm-helpers.h>
> +
> +/* controller-specific commands */
> +#define ST7586_DISP_MODE_GRAY	0x38
> +#define ST7586_DISP_MODE_MONO	0x39
> +#define ST7586_ENABLE_DDRAM	0x3a
> +#define ST7586_SET_DISP_DUTY	0xb0
> +#define ST7586_SET_PART_DISP	0xb4
> +#define ST7586_SET_NLINE_INV	0xb5
> +#define ST7586_SET_VOP		0xc0
> +#define ST7586_SET_BIAS_SYSTEM	0xc3
> +#define ST7586_SET_BOOST_LEVEL	0xc4
> +#define ST7586_SET_VOP_OFFSET	0xc7
> +#define ST7586_ENABLE_ANALOG	0xd0
> +#define ST7586_AUTO_READ_CTRL	0xd7
> +#define ST7586_OTP_RW_CTRL	0xe0
> +#define ST7586_OTP_CTRL_OUT	0xe1
> +#define ST7586_OTP_READ		0xe3
> +
> +#define ST7586_DISP_CTRL_MX	BIT(6)
> +#define ST7586_DISP_CTRL_MY	BIT(7)
> +
> +static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,

Could you put a comment somewhere explaining what grey332 means, that 
it's not
a 332 pixel format that I first thought, but that you store 3x 2-bit 
pixles in one byte.

> +				       struct drm_framebuffer *fb,
> +				       struct drm_clip_rect *clip)
> +{
> +	size_t len = (clip->x2 - clip->x1) * (clip->y2 - clip->y1);
> +	unsigned int x, y;
> +	u8 *src, *buf, val;
> +
> +	/* 3 pixels per byte, so grow clip to nearest multiple of 3 */
> +	clip->x1 = clip->x1 / 3 * 3;

Something wrong here: 3 * 3.

> +	clip->x2 = (clip->x2 + 2) / 3 * 3;

You can use DIV_ROUND_UP().

> +
> +	buf = kmalloc(len, GFP_KERNEL);
> +	if (!buf)
> +		return;
> +
> +	tinydrm_xrgb8888_to_gray8(buf, vaddr, fb, clip);
> +	src = buf;
> +
> +	for (y = clip->y1; y < clip->y2; y++) {
> +		for (x = clip->x1; x < clip->x2; x += 3) {
> +			val = *src++ & 0xc0;
> +			if (val & 0xc0)
> +				val |= 0x20;
> +			val |= (*src++ & 0xc0) >> 3;
> +			if (val & 0x18)
> +				val |= 0x04;
> +			val |= *src++ >> 6;
> +			*dst++ = ~val;

I don't understand how this pixel packing matches the one described in
the datasheet. Why do you flip the bits at the end?

> +		}
> +	}
> +
> +	/* now adjust the clip so it applies to dst */
> +	clip->x1 /= 3;
> +	clip->x2 /= 3;
> +
> +	kfree(buf);
> +}
> +
> +static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
> +			   struct drm_clip_rect *clip)
> +{
> +	struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> +	struct dma_buf_attachment *import_attach = cma_obj->base.import_attach;
> +	struct drm_format_name_buf format_name;
> +	void *src = cma_obj->vaddr;
> +	int ret = 0;
> +
> +	if (import_attach) {
> +		ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
> +					       DMA_FROM_DEVICE);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	switch (fb->format->format) {
> +	case DRM_FORMAT_XRGB8888:
> +		st7586_xrgb8888_to_gray332(dst, src, fb, clip);
> +		break;
> +	default:
> +		dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
> +			     drm_get_format_name(fb->format->format,
> +						 &format_name));
> +		ret = -EINVAL;
> +		break;

You don't need this default, because you can only get the ones in 
st7586_formats.

> +	}
> +
> +	if (import_attach)
> +		dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);

ret = dma_buf_end_cpu_access(...)

> +
> +	return ret;
> +}
> +
> +static int st7586_fb_dirty(struct drm_framebuffer *fb,
> +			   struct drm_file *file_priv, unsigned int flags,
> +			   unsigned int color, struct drm_clip_rect *clips,
> +			   unsigned int num_clips)
> +{
> +	struct tinydrm_device *tdev = fb->dev->dev_private;
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +	struct drm_clip_rect clip;
> +	int ret = 0;
> +
> +	mutex_lock(&tdev->dirty_lock);
> +
> +	if (!mipi->enabled)
> +		goto out_unlock;
> +
> +	/* fbdev can flush even when we're not interested */
> +	if (tdev->pipe.plane.fb != fb)
> +		goto out_unlock;
> +
> +	tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
> +			    fb->height);
> +
> +	DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", fb->base.id,
> +		  clip.x1, clip.x2, clip.y1, clip.y2);
> +
> +	ret = st7586_buf_copy(mipi->tx_buf, fb, &clip);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* NB: st7586_buf_copy() modifies clip */
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
> +			 (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
> +			 (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
> +			 (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
> +			 (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
> +
> +	ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
> +				   (u8 *)mipi->tx_buf,
> +				   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
> +
> +out_unlock:
> +	mutex_unlock(&tdev->dirty_lock);
> +
> +	if (ret)
> +		dev_err_once(fb->dev->dev, "Failed to update display %d\n",
> +			     ret);
> +
> +	return ret;
> +}
> +
> +static const struct drm_framebuffer_funcs st7586_fb_funcs = {
> +	.destroy	= drm_fb_cma_destroy,
> +	.create_handle	= drm_fb_cma_create_handle,
> +	.dirty		= st7586_fb_dirty,
> +};
> +
> +void st7586_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 drm_framebuffer *fb = pipe->plane.fb;
> +	struct device *dev = tdev->drm->dev;
> +	int ret;
> +	u8 addr_mode;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	ret = regulator_enable(mipi->regulator);
> +	if (ret) {
> +		dev_err(dev, "Failed to enable regulator %d\n", ret);
> +		return;
> +	}
> +
> +	mipi_dbi_hw_reset(mipi);
> +	ret = mipi_dbi_command(mipi, ST7586_AUTO_READ_CTRL, 0x9f);
> +	if (ret) {
> +		dev_err(dev, "Error sending command %d\n", ret);
> +		regulator_disable(mipi->regulator);
> +		return;
> +	}
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_RW_CTRL, 0x00);
> +
> +	msleep(10);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_READ);
> +
> +	msleep(20);
> +
> +	mipi_dbi_command(mipi, ST7586_OTP_CTRL_OUT);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_SLEEP_MODE);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +
> +	msleep(50);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_VOP_OFFSET, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_VOP, 0xe3, 0x00);
> +	mipi_dbi_command(mipi, ST7586_SET_BIAS_SYSTEM, 0x02);
> +	mipi_dbi_command(mipi, ST7586_SET_BOOST_LEVEL, 0x04);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_ANALOG, 0x1d);
> +	mipi_dbi_command(mipi, ST7586_SET_NLINE_INV, 0x00);
> +	mipi_dbi_command(mipi, ST7586_DISP_MODE_GRAY);
> +	mipi_dbi_command(mipi, ST7586_ENABLE_DDRAM, 0x02);
> +
> +	switch (mipi->rotation) {
> +	default:
> +		addr_mode = 0x00;
> +		break;
> +	case 90:
> +		addr_mode = ST7586_DISP_CTRL_MY;
> +		break;
> +	case 180:
> +		addr_mode = ST7586_DISP_CTRL_MX | ST7586_DISP_CTRL_MY;
> +		break;
> +	case 270:
> +		addr_mode = ST7586_DISP_CTRL_MX;
> +		break;
> +	}
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_ADDRESS_MODE, addr_mode);
> +
> +	mipi_dbi_command(mipi, ST7586_SET_DISP_DUTY, 0x7f);
> +	mipi_dbi_command(mipi, ST7586_SET_PART_DISP, 0xa0);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_PARTIAL_AREA, 0x00, 0x00, 0x00, 0x77);
> +	mipi_dbi_command(mipi, MIPI_DCS_EXIT_INVERT_MODE);
> +
> +	msleep(100);
> +
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_ON);
> +
> +	mipi->enabled = true;
> +
> +	if (fb)
> +		fb->funcs->dirty(fb, NULL, 0, 0, NULL, 0);
> +
> +	tinydrm_enable_backlight(mipi->backlight);
> +}
> +
> +static void st7586_pipe_disable(struct drm_simple_display_pipe *pipe)
> +{
> +	struct tinydrm_device *tdev = pipe_to_tinydrm(pipe);
> +	struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	if (!mipi->enabled)
> +		return;
> +
> +	mipi->enabled = false;
> +	tinydrm_disable_backlight(mipi->backlight);
> +	mipi_dbi_command(mipi, MIPI_DCS_SET_DISPLAY_OFF);
> +	regulator_disable(mipi->regulator);
> +}
> +
> +static const u32 st7586_formats[] = {
> +	DRM_FORMAT_XRGB8888,
> +};
> +
> +static int st7586_init(struct device *dev, struct mipi_dbi *mipi,
> +		const struct drm_simple_display_pipe_funcs *pipe_funcs,
> +		struct drm_driver *driver, const struct drm_display_mode *mode,
> +		unsigned int rotation)
> +{
> +	size_t bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
> +	struct tinydrm_device *tdev = &mipi->tinydrm;
> +	int ret;
> +
> +	mutex_init(&mipi->cmdlock);
> +
> +	mipi->tx_buf = devm_kmalloc(dev, bufsize, GFP_KERNEL);
> +	if (!mipi->tx_buf)
> +		return -ENOMEM;
> +
> +	ret = devm_tinydrm_init(dev, tdev, &st7586_fb_funcs, driver);
> +	if (ret)
> +		return ret;
> +
> +	ret = tinydrm_display_pipe_init(tdev, pipe_funcs,
> +					DRM_MODE_CONNECTOR_VIRTUAL,
> +					st7586_formats,
> +					ARRAY_SIZE(st7586_formats),
> +					mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	tdev->drm->mode_config.preferred_depth = 32;
> +	mipi->rotation = rotation;
> +
> +	drm_mode_config_reset(tdev->drm);
> +
> +	DRM_DEBUG_KMS("preferred_depth=%u, rotation = %u\n",
> +		      tdev->drm->mode_config.preferred_depth, rotation);
> +
> +	return 0;
> +}
> +
> +static const struct drm_simple_display_pipe_funcs st7586_pipe_funcs = {
> +	.enable		= st7586_pipe_enable,
> +	.disable	= st7586_pipe_disable,
> +	.update		= tinydrm_display_pipe_update,
> +	.prepare_fb	= tinydrm_display_pipe_prepare_fb,
> +};
> +
> +static const struct drm_display_mode st7586_mode = {
> +	TINYDRM_MODE(178, 128, 37, 27),
> +};
> +
> +DEFINE_DRM_GEM_CMA_FOPS(st7586_fops);
> +
> +static struct drm_driver st7586_driver = {
> +	.driver_features	= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME |
> +				  DRIVER_ATOMIC,
> +	.fops			= &st7586_fops,
> +	TINYDRM_GEM_DRIVER_OPS,
> +	.lastclose		= tinydrm_lastclose,
> +	.debugfs_init		= mipi_dbi_debugfs_init,
> +	.name			= "st7586",
> +	.desc			= "Sitronix ST7586",
> +	.date			= "20170801",
> +	.major			= 1,
> +	.minor			= 0,
> +};
> +
> +static const struct of_device_id st7586_of_match[] = {
> +	{ .compatible = "lego,ev3-lcd" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, st7586_of_match);
> +
> +static const struct spi_device_id st7586_id[] = {
> +	{ "ev3-lcd", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(spi, st7586_id);
> +
> +static int st7586_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct tinydrm_device *tdev;
> +	struct mipi_dbi *mipi;
> +	struct gpio_desc *dc;
> +	u32 rotation = 0;
> +	int ret;
> +
> +	mipi = devm_kzalloc(dev, sizeof(*mipi), GFP_KERNEL);
> +	if (!mipi)
> +		return -ENOMEM;
> +

reset and dc is required following our discussion:

> +	mipi->reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> +	if (IS_ERR(mipi->reset)) {
> +		dev_err(dev, "Failed to get gpio 'reset'\n");
> +		return PTR_ERR(mipi->reset);
> +	}
> +
> +	dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
> +	if (IS_ERR(dc)) {
> +		dev_err(dev, "Failed to get gpio 'dc'\n");
> +		return PTR_ERR(dc);
> +	}
> +
> +	mipi->regulator = devm_regulator_get(dev, "power");
> +	if (IS_ERR(mipi->regulator))
> +		return PTR_ERR(mipi->regulator);
> +
> +	mipi->backlight = tinydrm_of_find_backlight(dev);
> +	if (IS_ERR(mipi->backlight))
> +		return PTR_ERR(mipi->backlight);
> +
> +	device_property_read_u32(dev, "rotation", &rotation);
> +
> +	ret = mipi_dbi_spi_init(spi, mipi, dc);
> +	if (ret)
> +		return ret;
> +
> +	/* Cannot read from this controller via SPI */
> +	mipi->read_commands = NULL;
> +
> +	/*
> +	 * we are using 8-bit data, so we are not actually swapping anything,
> +	 * but setting mipi->swap_bytes makes mipi_dbi_typec3_command() do the
> +	 * right thing and not use 16-bit transfers (which results in swapped
> +	 * bytes on little-endian systems and causes out of order data to be
> +	 * sent to the display).
> +	 */
> +	mipi->swap_bytes = true;
> +
> +	ret = st7586_init(&spi->dev, mipi, &st7586_pipe_funcs, &st7586_driver,
> +			  &st7586_mode, rotation);
> +	if (ret)
> +		return ret;
> +
> +	tdev = &mipi->tinydrm;
> +
> +	ret = devm_tinydrm_register(tdev);
> +	if (ret)
> +		return ret;
> +
> +	spi_set_drvdata(spi, mipi);
> +
> +	DRM_DEBUG_DRIVER("Initialized %s:%s @%uMHz on minor %d\n",
> +			 tdev->drm->driver->name, dev_name(dev),
> +			 spi->max_speed_hz / 1000000,
> +			 tdev->drm->primary->index);
> +
> +	return 0;
> +}
> +
> +static void st7586_shutdown(struct spi_device *spi)
> +{
> +	struct mipi_dbi *mipi = spi_get_drvdata(spi);
> +
> +	tinydrm_shutdown(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_suspend(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +
> +	return tinydrm_suspend(&mipi->tinydrm);
> +}
> +
> +static int __maybe_unused st7586_pm_resume(struct device *dev)
> +{
> +	struct mipi_dbi *mipi = dev_get_drvdata(dev);
> +
> +	return tinydrm_resume(&mipi->tinydrm);
> +}
> +
> +static const struct dev_pm_ops st7586_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(st7586_pm_suspend, st7586_pm_resume)
> +};

I checked davinci_all_defconfig and CONFIG_PM is not set.
If the board can't suspend, then you don't need this.

> +
> +static struct spi_driver st7586_spi_driver = {
> +	.driver = {
> +		.name = "st7586",
> +		.owner = THIS_MODULE,
> +		.of_match_table = st7586_of_match,
> +		.pm = &st7586_pm_ops,
> +	},
> +	.id_table = st7586_id,
> +	.probe = st7586_probe,
> +	.shutdown = st7586_shutdown,
> +};
> +module_spi_driver(st7586_spi_driver);
> +
> +MODULE_DESCRIPTION("Sitronix ST7586 DRM driver");
> +MODULE_AUTHOR("David Lechner <david@lechnology.com>");
> +MODULE_LICENSE("GPL");

Re: [PATCH v3 4/6] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[Noralf Trønnes]

Den 05.08.2017 20.19, skrev Noralf Trønnes:
>
> Den 04.08.2017 00.33, skrev David Lechner:
>> LEGO MINDSTORMS EV3 has an LCD with a ST7586 controller. This adds a new
>> module for the ST7586 controller with parameters for the LEGO MINDSTORMS
>> EV3 LCD display.
>>
>> Signed-off-by: David Lechner <david-nq/r/kbU++upp/zk7JDF2g@public.gmane.org>
>> ---

[...]

>> diff --git a/drivers/gpu/drm/tinydrm/st7586.c 
>> b/drivers/gpu/drm/tinydrm/st7586.c

[...]

>> +static void st7586_xrgb8888_to_gray332(u8 *dst, void *vaddr,
>
> Could you put a comment somewhere explaining what grey332 means, that 
> it's not
> a 332 pixel format that I first thought, but that you store 3x 2-bit 
> pixles in one byte.
>
>> +                       struct drm_framebuffer *fb,
>> +                       struct drm_clip_rect *clip)
>> +{
>> +    size_t len = (clip->x2 - clip->x1) * (clip->y2 - clip->y1);
>> +    unsigned int x, y;
>> +    u8 *src, *buf, val;
>> +
>> +    /* 3 pixels per byte, so grow clip to nearest multiple of 3 */
>> +    clip->x1 = clip->x1 / 3 * 3;
>
> Something wrong here: 3 * 3.
>
>> +    clip->x2 = (clip->x2 + 2) / 3 * 3;
>
> You can use DIV_ROUND_UP().
>

Now I see what you're doing, can you use roundup() and rounddown()?

>> +
>> +    buf = kmalloc(len, GFP_KERNEL);
>> +    if (!buf)
>> +        return;
>> +
>> +    tinydrm_xrgb8888_to_gray8(buf, vaddr, fb, clip);
>> +    src = buf;
>> +
>> +    for (y = clip->y1; y < clip->y2; y++) {
>> +        for (x = clip->x1; x < clip->x2; x += 3) {
>> +            val = *src++ & 0xc0;
>> +            if (val & 0xc0)
>> +                val |= 0x20;
>> +            val |= (*src++ & 0xc0) >> 3;
>> +            if (val & 0x18)
>> +                val |= 0x04;
>> +            val |= *src++ >> 6;
>> +            *dst++ = ~val;
>
> I don't understand how this pixel packing matches the one described in
> the datasheet. Why do you flip the bits at the end?
>
>> +        }
>> +    }
>> +
>> +    /* now adjust the clip so it applies to dst */
>> +    clip->x1 /= 3;
>> +    clip->x2 /= 3;

I don't like this, you are changing the clip into a buffer length.
Better do the calculation before you call mipi_dbi_command_buf().

>> +
>> +    kfree(buf);
>> +}
>> +
>> +static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
>> +               struct drm_clip_rect *clip)
>> +{
>> +    struct drm_gem_cma_object *cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> +    struct dma_buf_attachment *import_attach = 
>> cma_obj->base.import_attach;
>> +    struct drm_format_name_buf format_name;
>> +    void *src = cma_obj->vaddr;
>> +    int ret = 0;
>> +
>> +    if (import_attach) {
>> +        ret = dma_buf_begin_cpu_access(import_attach->dmabuf,
>> +                           DMA_FROM_DEVICE);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    switch (fb->format->format) {
>> +    case DRM_FORMAT_XRGB8888:
>> +        st7586_xrgb8888_to_gray332(dst, src, fb, clip);
>> +        break;

I assume you will be adding monochrome and greyscale formats here soon.
Maybe you should have a function st7586_grey8_pack() or something, and do:

switch (fb->format->format) {
         case DRM_FORMAT_XRGB8888:
                 buf = kmalloc();
                 tinydrm_xrgb8888_to_gray8(buf, src, fb, clip);
                 st7586_grey8_to_packed8(dst, buf, clip);
                 kfree();
                 break;

And then later add:

         case DRM_FORMAT_Y8:
                 st7586_grey8_to_packed8(dst, src,...);
                 break;
         case DRM_FORMAT_Y1:
                 st7586_mono_to_packed8(dst, src,...);
                 break;

Just a suggestion, feel free to do what you want.

A patch adding greyscale came today:
https://lists.freedesktop.org/archives/dri-devel/2017-August/149445.html

>> +    default:
>> +        dev_err_once(fb->dev->dev, "Format is not supported: %s\n",
>> +                 drm_get_format_name(fb->format->format,
>> +                         &format_name));
>> +        ret = -EINVAL;
>> +        break;
>
> You don't need this default, because you can only get the ones in 
> st7586_formats.
>
>> +    }
>> +
>> +    if (import_attach)
>> +        dma_buf_end_cpu_access(import_attach->dmabuf, DMA_FROM_DEVICE);
>
> ret = dma_buf_end_cpu_access(...)
>
>> +
>> +    return ret;
>> +}
>> +
>> +static int st7586_fb_dirty(struct drm_framebuffer *fb,
>> +               struct drm_file *file_priv, unsigned int flags,
>> +               unsigned int color, struct drm_clip_rect *clips,
>> +               unsigned int num_clips)
>> +{
>> +    struct tinydrm_device *tdev = fb->dev->dev_private;
>> +    struct mipi_dbi *mipi = mipi_dbi_from_tinydrm(tdev);
>> +    struct drm_clip_rect clip;
>> +    int ret = 0;
>> +
>> +    mutex_lock(&tdev->dirty_lock);
>> +
>> +    if (!mipi->enabled)
>> +        goto out_unlock;
>> +
>> +    /* fbdev can flush even when we're not interested */
>> +    if (tdev->pipe.plane.fb != fb)
>> +        goto out_unlock;
>> +
>> +    tinydrm_merge_clips(&clip, clips, num_clips, flags, fb->width,
>> +                fb->height);
>> +

I suggest you adjust the clip here since it applies to all formats:

     /* pixels are packed in threes */
     clip->x1 = rounddown(clip->x1, 3);
     clip->x2 = roundup(clip->x2, 3);

>> +    DRM_DEBUG("Flushing [FB:%d] x1=%u, x2=%u, y1=%u, y2=%u\n", 
>> fb->base.id,
>> +          clip.x1, clip.x2, clip.y1, clip.y2);
>> +
>> +    ret = st7586_buf_copy(mipi->tx_buf, fb, &clip);
>> +    if (ret)
>> +        goto out_unlock;
>> +
>> +    /* NB: st7586_buf_copy() modifies clip */
>> +
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_COLUMN_ADDRESS,
>> +             (clip.x1 >> 8) & 0xFF, clip.x1 & 0xFF,
>> +             (clip.x2 >> 8) & 0xFF, (clip.x2 - 1) & 0xFF);
>> +    mipi_dbi_command(mipi, MIPI_DCS_SET_PAGE_ADDRESS,
>> +             (clip.y1 >> 8) & 0xFF, clip.y1 & 0xFF,
>> +             (clip.y2 >> 8) & 0xFF, (clip.y2 - 1) & 0xFF);
>> +
>> +    ret = mipi_dbi_command_buf(mipi, MIPI_DCS_WRITE_MEMORY_START,
>> +                   (u8 *)mipi->tx_buf,
>> +                   (clip.x2 - clip.x1) * (clip.y2 - clip.y1));
>> +
>> +out_unlock:
>> +    mutex_unlock(&tdev->dirty_lock);
>> +
>> +    if (ret)
>> +        dev_err_once(fb->dev->dev, "Failed to update display %d\n",
>> +                 ret);
>> +
>> +    return ret;
>> +}
>> +


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 4/6] drm/tinydrm: add support for LEGO MINDSTORMS EV3 LCD

[David Lechner]

On 08/05/2017 01:19 PM, Noralf Trønnes wrote:
> 
> Den 04.08.2017 00.33, skrev David Lechner:
>> +
>> +    buf = kmalloc(len, GFP_KERNEL);
>> +    if (!buf)
>> +        return;
>> +
>> +    tinydrm_xrgb8888_to_gray8(buf, vaddr, fb, clip);
>> +    src = buf;
>> +
>> +    for (y = clip->y1; y < clip->y2; y++) {
>> +        for (x = clip->x1; x < clip->x2; x += 3) {
>> +            val = *src++ & 0xc0;
>> +            if (val & 0xc0)
>> +                val |= 0x20;
>> +            val |= (*src++ & 0xc0) >> 3;
>> +            if (val & 0x18)
>> +                val |= 0x04;
>> +            val |= *src++ >> 6;
>> +            *dst++ = ~val;
> 
> I don't understand how this pixel packing matches the one described in
> the datasheet. Why do you flip the bits at the end?
> 

I a trying to be too clever. :-)

Here is the comment I will add to the next revision:

/*
  * The ST7586 controller has an unusual pixel format where 2bpp 
grayscale is
  * packed 3 pixels per byte with the first two pixels using 3 bits and 
the 3rd
  * pixel using only 2 bits.
  *
  * |  D7  |  D6  |  D5  ||  D1  |  D0  || 2bpp |
  * | (D4) | (D3) | (D2) ||      |      || GRAY |
  * +------+------+------++------+------++------+
  * |  1   |  1   |  1   ||  1   |  1   || 0  0 | black
  * |  1   |  0   |  0   ||  1   |  0   || 0  1 | dark gray
  * |  0   |  1   |  0   ||  0   |  1   || 1  0 | light gray
  * |  0   |  0   |  0   ||  0   |  0   || 1  1 | white
  */


As you can see, in the controller DRAM 1's are black and 0's are white, 
but in the kernel, it is the opposite. Also, if you look at the truth 
table, you can see that the extra 3rd bit has the pattern if D7 == 0 or 
D6 == 0 then D5 is zero.

I suppose it could be better to do this with a lookup table:

static const u8 st7586_lookup[] = { 0x7, 0x4, 0x2, 0x0 };

...

	for (y = clip->y1; y < clip->y2; y++) {
		for (x = clip->x1; x < clip->x2; x += 3) {
			val = st7586_lookup[*src++ >> 6] << 5;
			val |= st7586_lookup[*src++ >> 6] << 2;
			val |= st7586_lookup[*src++ >> 6] >> 1;
			*dst++ = val;
		}
	}


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v3 5/6] ARM: dts: da850-lego-ev3: Add node for LCD display

[David Lechner]

This adds a new node for the LEGO MINDSTORMS EV3 LCD display.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850-lego-ev3.dts | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/arch/arm/boot/dts/da850-lego-ev3.dts b/arch/arm/boot/dts/da850-lego-ev3.dts
index 45983c0..249b317 100644
--- a/arch/arm/boot/dts/da850-lego-ev3.dts
+++ b/arch/arm/boot/dts/da850-lego-ev3.dts
@@ -249,6 +249,15 @@
 			0x4c 0x00000080 0x000000f0
 		>;
 	};
+
+	ev3_lcd_pins: pinmux_lcd {
+		pinctrl-single,bits = <
+			/* SIMO, GP2[11], GP2[12], CLK */
+			0x14 0x00188100 0x00ffff00
+			/* GP5[0] */
+			0x30 0x80000000 0xf0000000
+		>;
+	};
 };
 
 &pinconf {
@@ -357,6 +366,21 @@
 	};
 };
 
+&spi1 {
+	status = "okay";
+	pinctrl-0 = <&ev3_lcd_pins>;
+	pinctrl-names = "default";
+	cs-gpios = <&gpio 44 GPIO_ACTIVE_LOW>;
+
+	display@0{
+		compatible = "lego,ev3-lcd";
+		reg = <0>;
+		spi-max-frequency = <10000000>;
+		dc-gpios = <&gpio 43 GPIO_ACTIVE_HIGH>;
+		reset-gpios = <&gpio 80 GPIO_ACTIVE_HIGH>;
+	};
+};
+
 &ehrpwm0 {
 	status = "okay";
 };
-- 
2.7.4

[PATCH v3 6/6] ARM: davinci_all_defconfig: enable tinydrm and ST7586

[David Lechner]

This enables the tinydrm and ST7586 panel modules used by the display
on LEGO MINDSTORMS EV3.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/configs/davinci_all_defconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/davinci_all_defconfig b/arch/arm/configs/davinci_all_defconfig
index 06e2e2a..27d9720 100644
--- a/arch/arm/configs/davinci_all_defconfig
+++ b/arch/arm/configs/davinci_all_defconfig
@@ -143,6 +143,8 @@ CONFIG_VIDEO_ADV7343=m
 CONFIG_DRM=m
 CONFIG_DRM_TILCDC=m
 CONFIG_DRM_DUMB_VGA_DAC=m
+CONFIG_DRM_TINYDRM=m
+CONFIG_TINYDRM_ST7586=m
 CONFIG_FB=y
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB_DA8XX=y
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel