On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<javierm@xxxxxxxxxx> wrote:
> This deemed useful to avoid hardcoding a page height and allow to support
> other Solomon controller families, but dividing the screen in pages seems
> to be something that is specific to the SSD130x chip family.
>
> For example, SSD132x chip family divides the screen in segments (columns)
> and common outputs (rows), so the concept of screen pages does not exist
> for the SSD132x family.
>
> Let's drop this field from the device info struct and just use a constant
> SSD130X_PAGE_HEIGHT macro to define the page height. While being there,
> replace hardcoded 8 values in places where it is used as the page height.
>
> Signed-off-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>
> --- a/drivers/gpu/drm/solomon/ssd13xx.c
> +++ b/drivers/gpu/drm/solomon/ssd13xx.c
> @@ -465,13 +462,13 @@ static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
> 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 page_height = ssd13xx->device_info->page_height;
> + unsigned int page_height = SSD130X_PAGE_HEIGHT;
> unsigned int pages = DIV_ROUND_UP(height, page_height);
> struct drm_device *drm = &ssd13xx->drm;
> u32 array_idx = 0;
> int ret, i, j, k;
>
> - drm_WARN_ONCE(drm, y % 8 != 0, "y must be aligned to screen page\n");
> + drm_WARN_ONCE(drm, y % page_height != 0, "y must be aligned to screen page\n");
>
> /*
> * The screen is divided in pages, each having a height of 8
> @@ -503,27 +500,32 @@ static int ssd13xx_update_rect(struct ssd13xx_device *ssd13xx,
> */
>
> if (!ssd13xx->page_address_mode) {
> + u8 page_start;
> +
> /* Set address range for horizontal addressing mode */
> ret = ssd13xx_set_col_range(ssd13xx, ssd13xx->col_offset + x, width);
> if (ret < 0)
> return ret;
>
> - ret = ssd13xx_set_page_range(ssd13xx, ssd13xx->page_offset + y / 8, pages);
> + page_start = ssd13xx->page_offset + y / page_height;
> + ret = ssd13xx_set_page_range(ssd13xx, page_start, pages);
> if (ret < 0)
> return ret;
> }
>
> for (i = 0; i < pages; i++) {
> - int m = 8;
> + int m = page_height;
>
> /* Last page may be partial */
> - if (8 * (y / 8 + i + 1) > ssd13xx->height)
> - m = ssd13xx->height % 8;
> + if (page_height * (y / page_height + i + 1) > ssd13xx->height)
> + m = ssd13xx->height % page_height;
> +
> for (j = 0; j < width; j++) {
> u8 data = 0;
>
> for (k = 0; k < m; k++) {
> - u8 byte = buf[(8 * i + k) * line_length + j / 8];
> + u32 idx = (page_height * i + k) * line_length + j / 8;
Nit: I would use unsigned int for idx, as that matches all other
variables involved.
But given array_idx is u32, too, u32 may makes sense.
> + u8 byte = buf[idx];
> u8 bit = (byte >> (j % 8)) & 1;
>
> data |= bit << k;
Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
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
