Re: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv

From: Rob Clark <robdclark@gmail.com>
To: Tomasz Figa <t.figa@samsung.com>
Cc: "Stéphane Marchesin" <marcheu@chromium.org>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>
Subject: Re: [PATCH v3 12/32] drm/exynos: Split manager/display/subdrv
Date: Fri, 29 Nov 2013 13:35:34 -0500	[thread overview]
Message-ID: <CAF6AEGv6iV27x_iC8xk_R4RcekYRgr5TBkDsRKEzdqj4J1exnQ@mail.gmail.com> (raw)
In-Reply-To: <1986269.EF7IBtZrRk@amdc1227>

On Fri, Nov 29, 2013 at 12:05 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> On Friday 29 of November 2013 09:13:19 Rob Clark wrote:
>> On Fri, Nov 29, 2013 at 4:10 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > I would mostly agree with you if we were discussing SoC-internal
>> > components here. Mostly, because the ARM world is more complex and you
>> > can see the same IP across completely different SoCs from different
>> > vendors.
>> >
>> > However, the topic here is about external devices, outside the SoC, such
>> > as different kind of bridges, like the PTN3460 eDP to LVDS bridge, which
>> > are likely to be reused across different platforms. Similar thing is with
>> > using different bridges on different boards using the same SoC platform.
>> > I don't think having an abstraction here would be any overabstraction at
>> > all. Anything less would be a huge "underabstraction" in fact.
>>
>>
>> I think no one is arguing that we don't eventually need some better
>> abstraction.  But as long as it is one-bridge and one-user, if the
>> patches otherwise have merit, add functionality that was missing
>> before and don't regress, then lack of infrastructure to match up
>> bridge and driver isn't something I will care about too much yet.
>> Things are allowed to be in-progress.  A missing abstraction for a 1:1
>> relationship is fine.
>
> This is not just one-bridge, one-user. This is about users of Exynos DRM
> we already have in-tree, such as Trats, Trats2 or Arndale, that the DRM
> bridge infrastructure could be used on and finally allowing to have
> display support on them. Of course you could merge this as is and
> then let someone else completely rewrite it (most likely in the same
> release cycle), but since it's not really much more work, I don't
> think there is any sense.

well, I'm not quite sure where I there is a pending complete
re-write..  it looks like the hard ptn3460 dependency is just a few
lines in one function.  Otherwise I'd agree with you.

(and even the patch that touches the code calling ptn3460_init() is
just moving it around from what I see)

> Moreover, let's stick to modern kernel driver coding standards. I don't
> think that "I want this patchset merged so badly" is really a good excuse
> to get around it. After all, there would be no GKH's staging tree, if
> nobody cared about quality in mainline.

And with my quality hat on, I could say that I'm not too fond of
unused (or 1:1 client to user) abstractions.  That is something that
should be introduced as we merge our 2nd or 3rd bridge.

BR,
-R

> Best regards,
> Tomasz
>