Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <20231009183522.543918-1-javierm@redhat.com> <20231009183522.543918-4-javierm@redhat.com>
In-Reply-To: <20231009183522.543918-4-javierm@redhat.com>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Wed, 11 Oct 2023 09:39:44 +0200
Message-ID: <CAMuHMdWSOcgV-qseaidUKcJswiJzr2+JQqB=k6tasaUXiEyiHw@mail.gmail.com>
Subject: Re: [PATCH 3/8] drm/ssd13xx: Replace .page_height field in device
 info with a constant
To:     Javier Martinez Canillas <javierm@redhat.com>
Cc:     linux-kernel@vger.kernel.org, Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Daniel Vetter <daniel@ffwll.ch>,
        David Airlie <airlied@gmail.com>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<javierm@redhat.com> 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@redhat.com>

> --- 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@glider.be>

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