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]

Date:   Wed, 11 Oct 2023 10:48:38 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
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

Geert Uytterhoeven <geert@...ux-m68k.org> writes:

> 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]?
>

I could do that, yeah. Originally thought that could be useful besides the
per chip functions, but I don't think that it's used for anything else...

[...]

>> +       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.
>

Yes, I didn't want to store the format info in struct ssd13xx_funcs
because the idea was for that struct to only have chip operations.

But could do that. I wonder if should rename that struct thought? to
something that better denotes is more than function handlers.

> 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 ;-)
>

Exactly, thinking in your patches is why I stored the full format info :)

> Gr{oetje,eeting}s,

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat