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

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

> Best regards
> Thomas
>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat