[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <93ab2ae3-46f4-4245-8d2b-e87700372ba6@ideasonboard.com>
Date: Fri, 18 Jul 2025 14:13:03 +0300
From: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>
To: Luca Ceresoli <luca.ceresoli@...tlin.com>,
Michael Walle <mwalle@...nel.org>, Jyri Sarha <jyri.sarha@....fi>
Cc: Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/tidss: encoder: convert to devm_drm_bridge_alloc()
Hi,
On 17/07/2025 16:56, Luca Ceresoli wrote:
> Hello Jyri, Tomi, Michael,
>
> On Thu, 17 Jul 2025 09:49:44 +0200
> "Michael Walle" <mwalle@...nel.org> wrote:
>
>> Hi,
>>
>> thanks for taking a look!
>>
>>> However allocating an encoder using a bridge alloc function (while we
>>> used to call an encoder allocation function) looks counter-intuitive.
>>>
>>> We had discussed on IRC a different idea, adding a wrapper structure
>>> around the bridge. Quoting your proposal:
>>>
>>> struct tidss_encoder_bridge {
>>> struct drm_bridge bridge;
>>> struct tidss_encoder *encoder
>>> }
>>>
>>> and then in the bridge funcs go from drm_bridge to tidss_encoder_brigde
>>> and use the pointer to get the original private struct.
>>
>> I was doing that until I've realized that meson/meson_encoder_* is
>> doing it the way this patch does it.
>
> Which was done by, er, myself. O:-)
>
> To my excuse, meson was using *_encoder_alloc() but rather
> devm_kzalloc() + drm_simple_encoder_init(), and the change was
> semi-automated via a coccinelle script, so I didn't fully realize that.
>
>>> That would be cleaner and more intuitive, but use a bit more memory and
>>> have an additional pointer deref, thus I think we can live with the
>>> patch you just sent, at least for now.
>>
>> I'm fine with changing it to the wrapper struct. It's your/the
>> maintainers call :)
>
> I think the driver maintainers opinion is more relevant, but in lack of
> one I think we can take the patch as is, given it's already written.
>
> Jyri, Tomi?
I think this is fine, even though I do agree the tidss_encoder.c is very
confusing.
I'll pick this up. I think drm-misc-next-fixes is the correct branch here.
Tomi
Powered by blists - more mailing lists