Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Thomas Zimmermann <tzimmermann@suse.de>,
        linux-kernel@vger.kernel.org
Cc:     dri-devel@lists.freedesktop.org,
        Maxime Ripard <mripard@kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Conor Dooley <conor@kernel.org>,
        Peter Robinson <pbrobinson@gmail.com>
Subject: Re: [PATCH v2 2/6] drm/ssd130x: Add a per controller family
 functions table
In-Reply-To: <e6593ea5-daa1-466a-9d42-0c1872f41a13@suse.de>
References: <20231012065822.1007930-1-javierm@redhat.com>
 <20231012065822.1007930-3-javierm@redhat.com>
 <e6593ea5-daa1-466a-9d42-0c1872f41a13@suse.de>
Date:   Thu, 12 Oct 2023 10:02:35 +0200
Message-ID: <87a5so46as.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Thomas Zimmermann <tzimmermann@suse.de> 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