[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87a5so46as.fsf@minerva.mail-host-address-is-not-set>
Date: Thu, 12 Oct 2023 10:02:35 +0200
From: Javier Martinez Canillas <javierm@...hat.com>
To: Thomas Zimmermann <tzimmermann@...e.de>,
linux-kernel@...r.kernel.org
Cc: dri-devel@...ts.freedesktop.org,
Maxime Ripard <mripard@...nel.org>,
Geert Uytterhoeven <geert@...ux-m68k.org>,
Conor Dooley <conor@...nel.org>,
Peter Robinson <pbrobinson@...il.com>
Subject: Re: [PATCH v2 2/6] drm/ssd130x: Add a per controller family
functions table
Thomas Zimmermann <tzimmermann@...e.de> writes:
Hello Thomas,
Thanks a lot for your feedback.
> Hi Javier
>
> Am 12.10.23 um 08:58 schrieb Javier Martinez Canillas:
> [...]
>>
>> +struct ssd130x_funcs {
>> + int (*init)(struct ssd130x_device *ssd130x);
>> + int (*set_buffer_sizes)(struct ssd130x_device *ssd130x);
>> + void (*align_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect);
>> + int (*update_rect)(struct ssd130x_device *ssd130x, struct drm_rect *rect,
>> + u8 *buf, u8 *data_array);
>> + void (*clear_screen)(struct ssd130x_device *ssd130x,
>> + u8 *data_array);
>> + void (*fmt_convert)(struct iosys_map *dst, const unsigned int *dst_pitch,
>> + const struct iosys_map *src, const struct drm_framebuffer *fb,
>> + const struct drm_rect *clip);
>> +};
>> +
>
> You are reinventing DRM's atomic helpers. I strongly advised against
> doing that, as it often turns out bad. Maybe see my rant at [1] wrt to
> another driver.
>
> It's much better to create a separate mode-setting pipeline for the
> ssd132x series and share the common code among pipelines. Your driver
> will have a clean and readable implementation for each supported
> chipset. Compare an old version of mgag200 [2] with the current driver
> to see the difference.
>
I see what you mean. The reason why I didn't go that route was to minimize
code duplication, but you are correct that each level of indirection makes
the driver harder to read, to reason about and fragile (modifying a common
callback could have undesired effects on other chip families as you said).
I'll give it a try to what you propose in v3, have separate modesetting
pipeline for SSD130x and SSD132x, even if this could lead to a little more
duplicated code.
> Best regards
> Thomas
>
--
Best regards,
Javier Martinez Canillas
Core Platforms
Red Hat
Powered by blists - more mailing lists