From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932897AbcGLKeJ (ORCPT ); Tue, 12 Jul 2016 06:34:09 -0400 Received: from mail-wm0-f41.google.com ([74.125.82.41]:37186 "EHLO mail-wm0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932844AbcGLKeH (ORCPT ); Tue, 12 Jul 2016 06:34:07 -0400 Date: Tue, 12 Jul 2016 12:34:02 +0200 From: Daniel Vetter To: Thierry Reding Cc: Emil Velikov , Vinay Simha BN , open list , "open list:DRM PANEL DRIVERS" , Archit Taneja Subject: Re: [PATCH v5 2/2] drm/panel: Add JDI LT070ME05000 WUXGA DSI Panel Message-ID: <20160712103402.GA23520@phenom.ffwll.local> Mail-Followup-To: Thierry Reding , Emil Velikov , Vinay Simha BN , open list , "open list:DRM PANEL DRIVERS" , Archit Taneja References: <1466046050-20588-1-git-send-email-simhavcs@gmail.com> <1466046050-20588-2-git-send-email-simhavcs@gmail.com> <20160711122937.GB14709@ulmo.ba.sec> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160711122937.GB14709@ulmo.ba.sec> X-Operating-System: Linux phenom 4.6.0-rc5+ User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 11, 2016 at 02:29:37PM +0200, Thierry Reding wrote: > On Thu, Jun 16, 2016 at 06:02:53PM +0100, Emil Velikov wrote: > > On 16 June 2016 at 04:00, Vinay Simha BN wrote: > [...] > > > +static int jdi_panel_disable(struct drm_panel *panel) > > > +{ > > > + struct jdi_panel *jdi = to_jdi_panel(panel); > > > + > > > + if (!jdi->enabled) > > > + return 0; > > > + > > Thinking out loud: > > > > Thierry, > > Shouldn't we fold 'enabled' and 'prepared' in struct drm_panel and > > tweak the helpers respectively ? Is there any specific reason for > > keeping these in the drivers ? > > Yes, I think that would make sense eventually. It's clearly a recurring > pattern. Ideally nothing would be calling these functions more than once > and thereby making the checks unnecessary. In practice that may mean > that we need to put the variables and checks into the drm/panel core > because display drivers (as opposed to a sane core implementation) call > these. I suppose we could encourage proper usage by adding a couple of > WARNs here and there if expectations aren't met. > > I don't think doing this is terribly urgent because it's easy to rip out > of drivers once the drm/panel core supports it. And it's something that > we could even leave within drivers when the core supports it, so trivial > to remove one by one after the core patches have landed. As long as we have non-atomic drm drivers using this multiple enable/disable calls can happen. Atomic drivers should screw this up (ignoring a few misguided ones that mix atomic and legacy helpers in bad ways, but those are getting fixed). I think a good plan would be: 1. Move this tracking into drm panel helpers, ditch it from all drivers. 2. Add WARN_ON for multiple enables/disables, but only for DRIVER_ATOMIC. Makes sure we can remove this boilerplate, makes sure that atomic drivers are consistent, leaves existing drivers unharmed. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch