Hi Javier Am 12.10.23 um 10:02 schrieb Javier Martinez Canillas:
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.
Thanks a lot. I know you want to make a reference driver for others to study. So I think at least trying the multi-pipeline way is worth it.
From the mgag200 and ast drivers, I found that the refactoring patch sets can be large to the point where one wonders whether it's actually worth it. But the end results turned out ok. (ast still needs more work though.)
Best regards Thomas
Best regards Thomas
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
Attachment:
OpenPGP_signature.asc
Description: OpenPGP digital signature
