Re: [PATCH v6 3/6] drm: Add driver for Solomon SSD130x OLED displays

Hi Javier,

On Mon, Feb 14, 2022 at 2:37 PM Javier Martinez Canillas
[off-list ref] wrote:

This adds a DRM driver for SSD1305, SSD1306, SSD1307 and SSD1309 Solomon
OLED display controllers.

It's only the core part of the driver and a bus specific driver is needed
for each transport interface supported by the display controllers.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

+static int ssd130x_update_rect(struct ssd130x_device *ssd130x, u8 *buf,
+                              struct drm_rect *rect)
+{
+       unsigned int x = rect->x1;
+       unsigned int y = rect->y1;
+       unsigned int width = drm_rect_width(rect);
+       unsigned int height = drm_rect_height(rect);
+       unsigned int line_length = DIV_ROUND_UP(width, 8);
+       unsigned int pages = DIV_ROUND_UP(y % 8 + height, 8);
+       u32 array_idx = 0;
+       int ret, i, j, k;
+       u8 *data_array = NULL;
+
+       data_array = kcalloc(width, pages, GFP_KERNEL);
+       if (!data_array)
+               return -ENOMEM;
+
+       /*
+        * The screen is divided in pages, each having a height of 8
+        * pixels, and the width of the screen. When sending a byte of
+        * data to the controller, it gives the 8 bits for the current
+        * column. I.e, the first byte are the 8 bits of the first
+        * column, then the 8 bits for the second column, etc.
+        *
+        *
+        * Representation of the screen, assuming it is 5 bits
+        * wide. Each letter-number combination is a bit that controls
+        * one pixel.
+        *
+        * A0 A1 A2 A3 A4
+        * B0 B1 B2 B3 B4
+        * C0 C1 C2 C3 C4
+        * D0 D1 D2 D3 D4
+        * E0 E1 E2 E3 E4
+        * F0 F1 F2 F3 F4
+        * G0 G1 G2 G3 G4
+        * H0 H1 H2 H3 H4
+        *
+        * If you want to update this screen, you need to send 5 bytes:
+        *  (1) A0 B0 C0 D0 E0 F0 G0 H0
+        *  (2) A1 B1 C1 D1 E1 F1 G1 H1
+        *  (3) A2 B2 C2 D2 E2 F2 G2 H2
+        *  (4) A3 B3 C3 D3 E3 F3 G3 H3
+        *  (5) A4 B4 C4 D4 E4 F4 G4 H4
+        */
+
+       ret = ssd130x_set_col_range(ssd130x, ssd130x->col_offset + x, width);
+       if (ret < 0)
+               goto out_free;
+
+       ret = ssd130x_set_page_range(ssd130x, ssd130x->page_offset + y / 8, pages);
+       if (ret < 0)
+               goto out_free;
+
+       for (i = y / 8; i < y / 8 + pages; i++) {
+               int m = 8;
+
+               /* Last page may be partial */
+               if (8 * (i + 1) > ssd130x->height)
+                       m = ssd130x->height % 8;
+               for (j = x; j < x + width; j++) {
+                       u8 data = 0;
+
+                       for (k = 0; k < m; k++) {
+                               u8 byte = buf[(8 * i + k) * line_length + j / 8];

As buf does not point to (0, 0), the above is not correct if rect.x1 !=
0 or rect.y1 != 0.  After fixing that, writing more than one text line
to the console works, but I still see an issue with updates where the
rectangle size and/or position are not aligned to 8 pixels horizontally.
Will do more investigation, and send fixes...

+                               u8 bit = (byte >> (j % 8)) & 1;
+
+                               data |= bit << k;
+                       }
+                       data_array[array_idx++] = data;
+               }
+       }
+
+       ret = ssd130x_write_data(ssd130x, data_array, width * pages);
+
+out_free:
+       kfree(data_array);
+       return ret;
+}
+
+static void ssd130x_clear_screen(struct ssd130x_device *ssd130x)
+{
+       u8 *buf = NULL;
+       struct drm_rect fullscreen = {
+               .x1 = 0,
+               .x2 = ssd130x->width,
+               .y1 = 0,
+               .y2 = ssd130x->height,
+       };
+
+       buf = kcalloc(ssd130x->width, ssd130x->height, GFP_KERNEL);

This buffer is larger than needed. Will send a fix.

+       if (!buf)
+               return;
+
+       ssd130x_update_rect(ssd130x, buf, &fullscreen);
+
+       kfree(buf);
+}
+
+static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct dma_buf_map *map,
+                               struct drm_rect *rect)
+{
+       struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
+       void *vmap = map->vaddr; /* TODO: Use mapping abstraction properly */
+       int ret = 0;
+       u8 *buf = NULL;
+
+       buf = kcalloc(fb->width, fb->height, GFP_KERNEL);

This buffer is much larger than needed. Will send a fix.

+       if (!buf)
+               return -ENOMEM;
+
+       drm_fb_xrgb8888_to_mono_reversed(buf, 0, vmap, fb, rect);
+
+       ssd130x_update_rect(ssd130x, buf, rect);
+
+       kfree(buf);
+
+       return ret;
+}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds