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

[Thomas Zimmermann]

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.

Best regards
Thomas

[1] 
https://lore.kernel.org/dri-devel/07cc89a5-5200-72e6-f078-694c5820a99a@xxxxxxx/
[2] https://elixir.bootlin.com/linux/v5.5/source/drivers/gpu/drm/mgag200


>   extern const struct ssd130x_deviceinfo ssd130x_variants[];
>   
>   struct ssd130x_device *ssd130x_probe(struct device *dev, struct regmap *regmap);

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