Re: [PATCH resend 2/5] video: fbdev: ssd1307fb: Simplify ssd1307fb_update_display()

Hi Sam,

On Mon, Jul 19, 2021 at 9:05 PM Sam Ravnborg [off-list ref] wrote:

On Wed, Jul 14, 2021 at 04:58:01PM +0200, Geert Uytterhoeven wrote:

quoted

Simplify the nested loops to handle conversion from linear frame buffer
to ssd1307 page layout:
  1. Move last page handling one level up, as the value of "m" is the
     same inside a page,
  2. array->data[] is filled linearly, so there is no need to
     recalculate array_idx over and over again; a simple increment is
     sufficient.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
 drivers/video/fbdev/ssd1307fb.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index e6b6263e3bef847f..6d7bd025bca1a175 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c

@@ -158,6 +158,7 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par)
      u8 *vmem = par->info->screen_buffer;
      unsigned int line_length = par->info->fix.line_length;
      unsigned int pages = DIV_ROUND_UP(par->height, 8);
+     u32 array_idx = 0;
      int ret, i, j, k;

      array = ssd1307fb_alloc_array(par->width * pages, SSD1307FB_DATA);

@@ -194,19 +195,21 @@ static int ssd1307fb_update_display(struct ssd1307fb_par *par)
       */

      for (i = 0; i < pages; i++) {
+             int m = 8;
+
+             /* Last page may be partial */
+             if (i + 1 == pages && par->height % 8)
+                     m = par->height % 8;
              for (j = 0; j < par->width; j++) {
-                     int m = 8;
-                     u32 array_idx = i * par->width + j;
-                     array->data[array_idx] = 0;
-                     /* Last page may be partial */
-                     if (i + 1 == pages && par->height % 8)
-                             m = par->height % 8;
+                     u8 data = 0;
+
                      for (k = 0; k < m; k++) {

If the last page is partial then m will be less than 8 for all bytes in
j = 0..par-width - but m should only be less than 8 for the last
iteration of the loop.

Do I miss something or is the code buggy?

"the loop" is the j-loop?
If m is less than 8 for the last page, it should be less than 8 for
all iterations of j, as all last bytes in each "line" (visible
row) are partial, cfr. the comments above the code, explaining the
representation of the screen.

quoted

                              u8 byte = vmem[(8 * i + k) * line_length +
                                             j / 8];
                              u8 bit = (byte >> (j % 8)) & 1;
-                             array->data[array_idx] |= bit << k;
+                             data |= bit << k;
                      }
+                     array->data[array_idx++] = data;
              }
      }

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