Re: [PATCH v8 2/3] drm/panel: Add support for S6E3HA2 panel driver on TM2 board

[Thierry Reding]

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.

Thierry

Attachment: signature.asc
Description: PGP signature