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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e6593ea5-daa1-466a-9d42-0c1872f41a13@suse.de>
Date:   Thu, 12 Oct 2023 09:39:53 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Javier Martinez Canillas <javierm@...hat.com>,
        linux-kernel@...r.kernel.org
Cc:     dri-devel@...ts.freedesktop.org,
        Maxime Ripard <mripard@...nel.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Conor Dooley <conor@...nel.org>,
        Peter Robinson <pbrobinson@...il.com>
Subject: Re: [PATCH v2 2/6] drm/ssd130x: Add a per controller family functions
 table

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@suse.de/
[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)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (841 bytes)