Thomas Zimmermann <tzimmermann@xxxxxxx> 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
