On Sat, Jul 29, 2017 at 10:17 PM, David Lechner david@lechnology.com wrote:

This adds helper functions and support for ST7586 controllers. These controllers have an unusual memory layout where 3 pixels are packed into 1 byte.

+-------+-----------------+ | bit | 7 6 5 4 3 2 1 0 | +-------+-----------------+ | pixel | 0 0 0 1 1 1 2 2 | +-------+-----------------+

So, there are a nuber of places in the tinydrm pipline where this format

number

needs to be taken into consideration.

• • tinydrm_rgb565_to_st7586 - Convert RGB565 to ST7586 clip buffer

How this can be generic tinydrm helper? Why driver can't handle it by its own and avoid spreading stuff into generic header?

+void tinydrm_rgb565_to_st7586(u8 *dst, void *vaddr,

• • tinydrm_xrgb8888_to_st7586 - Convert XRGB8888 to ST7586 clip buffer

+void tinydrm_xrgb8888_to_st7586(u8 *dst, void *vaddr,

Ditto.

•   switch (fb->format->format) {
  

•   case DRM_FORMAT_RGB565:
  

•           if (swap)
  

•                   tinydrm_swab16(dst, src, fb, clip);
  

•           else
  

•                   tinydrm_memcpy(dst, src, fb, clip);
  

•   switch (pixel_fmt) {
  

•   case MIPI_DCS_PIXEL_FMT_16BIT:
  

•           switch (fb->format->format) {
  

•           case DRM_FORMAT_RGB565:
  

•                   if (swap)
  

•                           tinydrm_swab16(dst, src, fb, clip);
  

•                   else
  

•                           tinydrm_memcpy(dst, src, fb, clip);
  

•                   break;
  

Can't you use some other approach? Callbacks? Plugins?

•   switch (mipi->pixel_fmt) {
  

•   case MIPI_DCS_PIXEL_FMT_16BIT:
  

•           len = width * height * sizeof(u16);
  

•           break;
  

•   case MIPI_DCS_PIXEL_FMT_ST7586_332:
  

•           width = (width + 2) / 3;
  

•           len = width * height;
  

•           break;
  

Ditto.

•   case MIPI_DCS_PIXEL_FMT_ST7586_332:
  

•           /* 3 pixels per byte */
  

•           bufsize = (mode->vdisplay + 2) / 3 * mode->hdisplay;
  

•           break;
  

Ditto.

•   if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
  

•   if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes &&
  

•       mipi->pixel_fmt != MIPI_DCS_PIXEL_FMT_ST7586_332)
  

Ditto.

If we allow this we end up to have 100500 LOCs in tinydrm-helpers.c which will have nothing to do with the framework itself.