linux-kernel - Re: [PATCH 5/8] drm/ssd13xx: Add a per controller family functions table

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuHMdX0gvW4Y5ppWi=qF6mZF00m4G-fP1-Wg+sKW43Ku5fz9A@mail.gmail.com>
Date:   Wed, 11 Oct 2023 10:09:50 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Javier Martinez Canillas <javierm@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH 5/8] drm/ssd13xx: Add a per controller family functions table

Hi Javier,

On Mon, Oct 9, 2023 at 8:36 PM Javier Martinez Canillas
<javierm@...hat.com> wrote:
> To allow the driver to decouple the common logic in the function callbacks
> from the behaviour that is specific of a given Solomon controller family.
>
> For this, include a chip family to the device info and add fields to the
> driver-private device struct, to store the hardware buffer maximum size,
> the intermediate buffer maximum size and pixel format, and a set of per
> chip family function handlers.
>
> Signed-off-by: Javier Martinez Canillas <javierm@...hat.com>

Thanks for your patch!

> --- a/drivers/gpu/drm/solomon/ssd13xx.c
> +++ b/drivers/gpu/drm/solomon/ssd13xx.c
> @@ -104,6 +104,7 @@ const struct ssd13xx_deviceinfo ssd13xx_variants[] = {
>                 .default_width = 132,
>                 .default_height = 64,
>                 .page_mode_only = 1,
> +               .family_id = SSD130X_FAMILY,

Why not store &ssd13xx_family_funcs[SSD130X_FAMILY]?

>         },
>         [SSD1305_ID] = {
>                 .default_vcomh = 0x34,

> @@ -602,6 +605,15 @@ static void ssd13xx_clear_screen(struct ssd13xx_device *ssd13xx, u8 *data_array)
>         }
>  }
>
> +static const struct ssd13xx_funcs ssd13xx_family_funcs[] = {
> +       [SSD130X_FAMILY] = {
> +               .init = ssd130x_init,
> +               .update_rect = ssd130x_update_rect,
> +               .clear_screen = ssd130x_clear_screen,
> +               .fmt_convert = drm_fb_xrgb8888_to_mono,
> +       },
> +};
> +
>  static int ssd13xx_fb_blit_rect(struct drm_framebuffer *fb,
>                                 const struct iosys_map *vmap,
>                                 struct drm_rect *rect, u8 *buf,

> @@ -1195,11 +1198,38 @@ static int ssd13xx_get_resources(struct ssd13xx_device *ssd13xx)
>         return 0;
>  }
>
> +static int ssd13xx_set_buffer_sizes(struct ssd13xx_device *ssd13xx,
> +                                   enum ssd13xx_family_ids family_id)
> +{
> +       const struct drm_format_info *fi;
> +       unsigned int buffer_pitch;
> +
> +       switch (family_id) {
> +       case SSD130X_FAMILY:
> +               unsigned int pages = DIV_ROUND_UP(ssd13xx->height, SSD130X_PAGE_HEIGHT);
> +
> +               ssd13xx->data_array_size = ssd13xx->width * pages;
> +
> +               fi = drm_format_info(DRM_FORMAT_R1);
> +               break;
> +       }

Please don't mix ssd13xx_funcs[family_id] and switch (family_id).
The switch() could be replaced by an extra function pointer in
ssd13xx_funcs, and by storing fi in ssd13xx_funcs, too.

Note that you don't really need the full drm_format_info, the number
of bits per pixel is sufficient.  But having the drm_format_info is
nice, as fmt_convert() will need it later when adding support for
fbdev emulation using R1 or R4 ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-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