Message-ID: <CACvgo52MeWN2q-0xPHq+YPSzJzoRXC5+rb_=xdic_015qy8Yig@mail.gmail.com>
Date:   Tue, 31 Jan 2017 21:32:42 +0000
From:   Emil Velikov <emil.l.velikov@...il.com>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Sean Paul <seanpaul@...omium.org>,
        devicetree <devicetree@...r.kernel.org>,
        "moderated list:ARM/S5P EXYNOS AR..." 
        <linux-samsung-soc@...r.kernel.org>,
        Donghwa Lee <dh09.lee@...sung.com>,
        "Linux-Kernel@...r. Kernel. Org" <linux-kernel@...r.kernel.org>,
        andi.shyti@...sung.com, jh80.chung@...sung.com,
        cw00.choi@...sung.com, Kukjin Kim <kgene@...nel.org>,
        ML dri-devel <dri-devel@...ts.freedesktop.org>,
        Hyungwon Hwang <human.hwang@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Hoegeun Kwon <hoegeun.kwon@...sung.com>
Subject: Re: [PATCH v8 2/3] drm/panel: Add support for S6E3HA2 panel driver on
 TM2 board

On 31 January 2017 at 21:17, Thierry Reding <thierry.reding@...il.com> wrote:
> On Tue, Jan 31, 2017 at 10:49:51AM -0500, Sean Paul wrote:
>> On Tue, Jan 31, 2017 at 04:02:26PM +0100, Thierry Reding wrote:
>> > On Tue, Jan 31, 2017 at 09:38:53AM -0500, Sean Paul wrote:
>> > > On Tue, Jan 31, 2017 at 09:54:49AM +0100, Thierry Reding wrote:
>> > > > On Tue, Jan 31, 2017 at 09:01:07AM +0900, Inki Dae wrote:
>> > > > >
>> > > > >
>> > > > > 2017년 01월 24일 10:50에 Hoegeun Kwon 이(가) 쓴 글:
>> > > > > > Dear Thierry,
>> > > > > >
>> > > > > > Could you please review this patch?
>> > > > >
>> > > > > Thierry, I think this patch has been reviewed enough but no comment
>> > > > > from you. Seems you are busy. I will pick up this.
>> > > >
>> > > > Sorry, but that's not how it works. This patch has gone through 8
>> > > > revisions within 4 weeks, and I tend to ignore patches like that until
>> > > > the dust settles.
>> > > >
>> > >
>> > > Seems like the dust was pretty settled. It was posted on 1/11, pinged on 1/24,
>> > > and picked up on 1/31. I don't think it's unreasonable to take it through
>> > > another tree after that.
>> > >
>> > > I wonder if drm_panel would benefit from the -misc group maintainership model
>> > > as drm_bridge does. By spreading out the workload, the high-maintenance
>> > > patches would hopefully find someone to shepherd them through.
>> >
>> > Except that nobody except me really cares.
>>
>> I certainly haven't been paying attention. My excuse, at least, is that you're a
>> great maintainer and I haven't thought the patches need a second look. Perhaps
>> if we moved towards a group, more people would be vested/care?
>
> I doubt that group maintainership would change much about the lack of
> peer review. Peer review makes maintenance a lot easier. Usually when I
> see that a patch has been reviewed by someone that I think I can trust,
> I only give it a very brief look before applying. However, if there's
> been no other review I need to take a lot more time to review.
>
>> > If we let people take patches
>> > through separate trees or group-maintained trees they'll likely go in
>> > without too much thought. DRM panel is somewhat different from core DRM
>> > in this regard because its infrastructure is minimal and there's little
>> > outside the panel-simple driver. So we're still at a stage where we need
>> > to fine-tune what drivers should look like and how we can improve.
>> >
>>
>> Fair point. With drm_bridge, I've been lending review help, but deferring to
>> Archit for merge on patches which I think he should look at. Gustavo is in a
>> similar place with fences. drm_panel seems like something that should follow
>> the same model. Maybe once more people (or one person) get up to speed on
>> things, we could share the load.
>
> I certainly wouldn't mind more people reviewing panel patches. Applying
> them is the easy part.
>
>> > > > Other than that, this continues the same madness that I've repeatedly
>> > > > complained about in the past. The whole mechanism of running through a
>> > > > series of writes and not caring about errors until the very end is
>> > > > something we've discussed at length in the past. It also in large parts
>> > > > duplicates a bunch of functions from other Samsung panel drivers that I
>> > > > already said should eventually be moved to something saner.
>> > > >
>> > >
>> > > FWIW, this type of error handling isn't my preference either. If we must defer,
>> > > I'd rather not keep it in ctx, but rather pass around an argument so it's more
>> > > obvious we need to deal with it in the return. That said, this seems like
>> > > a case of letting the perfect be the enemy of the good, surely something is
>> > > better than nothing?
>> >
>> > That's what I ended up saying the last two times. But this has got to
>> > stop at some point. If you look at most of the panel drivers, they look
>> > more like material for the staging tree rather than DRM proper.
>> >
>> > Yes, something is better than nothing, but we can't have this multiply
>> > further.
>> >
>>
>> So perhaps if we had more reviewers, we could tighten up the review feedback
>> loop and avoid this while still getting things merged?
>
> Maybe. Like I said, I would very much welcome more review on panel
> patches.
>
I think that there's a part that Inki and/or others might have
forgotten. Being the sole/core person reviewing this leads to a bit
too often repeat of the same principles ... which after a while gets a
bit [pick your favourite non-cool word].

In one of my experiences [whist attempting to help out], one had to
repeat/reword the exact same thing three times in order to be
addressed :-\

-Emil

Powered by blists - more mailing lists