Re: [PATCH v2 2/6] drm/ssd130x: Add a per controller family functions table

[Thomas Zimmermann]

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