Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757336AbcCURLZ (ORCPT ); Mon, 21 Mar 2016 13:11:25 -0400 Received: from mail-wm0-f53.google.com ([74.125.82.53]:37675 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752150AbcCURLV (ORCPT ); Mon, 21 Mar 2016 13:11:21 -0400 Date: Mon, 21 Mar 2016 18:12:08 +0100 From: Daniel Vetter To: Alexey Brodkin Cc: "daniel@ffwll.ch" , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" Subject: Re: [PATCH 1/3] drm: introduce drm_connector_plug_all() helper Message-ID: <20160321171208.GO28483@phenom.ffwll.local> Mail-Followup-To: Alexey Brodkin , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "airlied@linux.ie" References: <1458295304-5528-1-git-send-email-abrodkin@synopsys.com> <1458295304-5528-2-git-send-email-abrodkin@synopsys.com> <20160318180609.GM14170@phenom.ffwll.local> <1458338329.2994.8.camel@synopsys.com> <20160319100258.GP14170@phenom.ffwll.local> <1458558140.12775.8.camel@synopsys.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1458558140.12775.8.camel@synopsys.com> X-Operating-System: Linux phenom 4.3.0-1-amd64 User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4038 Lines: 91 On Mon, Mar 21, 2016 at 11:02:21AM +0000, Alexey Brodkin wrote: > Hi Daniel, > > On Sat, 2016-03-19 at 11:02 +0100, Daniel Vetter wrote: > > On Fri, Mar 18, 2016 at 09:58:49PM +0000, Alexey Brodkin wrote: > > > > > > Hi Daniel, > > > > > > On Fri, 2016-03-18 at 19:06 +0100, Daniel Vetter wrote: > > > > > > > > On Fri, Mar 18, 2016 at 01:01:42PM +0300, Alexey Brodkin wrote: > > > > > > > > > > > > > > > As a pair to already existing drm_connector_unplug_all() we're adding > > > > > generic implementation of what is already done in some drivers. > > > > > > > > > > Once this helper is implemented we'll be ready to switch existing > > > > > driver-specific implementations with generic one. > > > > > > > > > > Signed-off-by: Alexey Brodkin > > > > > Cc: Daniel Vetter > > > > > Cc: David Airlie > > > > > --- > > > > > ?drivers/gpu/drm/drm_crtc.c | 44 +++++++++++++++++++++++++++++++++++++++++++- > > > > > ?drivers/gpu/drm/drm_drv.c??|??3 ++- > > > > > ?include/drm/drm_crtc.h?????|??3 ++- > > > > > ?3 files changed, 47 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c > > > > > index 65258ac..ce27420 100644 > > > > > --- a/drivers/gpu/drm/drm_crtc.c > > > > > +++ b/drivers/gpu/drm/drm_crtc.c > > > > > @@ -1080,6 +1080,46 @@ void drm_connector_unregister(struct drm_connector *connector) > > > > > ?} > > > > > ?EXPORT_SYMBOL(drm_connector_unregister); > > > > > ? > > > > > +/** > > > > > + * drm_connector_plug_all - register connector userspace interfaces > > > > > + * @dev: drm device > > > > > + * > > > > > + * This function registers all connector userspace interfaces in sysfs. Should > > > > > + * be call when the device is disconnected, e.g. from an usb driver's > > > > Still talks about disconnect ;-) Please also mention that this just calls > > > > drm_connector_register() exactly like this including () to generate a > > > > kerneldoc hyperlink. > > > Well I intentionally left in description of drm_connector_register_all(): > > > "Should be call when the device is disconnected, e.g. from an usb driver's > > > ?->connect callback." > > You use "disconnected" for connecting stuff. That doesn't make sense to me > > at all - register_all is for when you want to publish something, not for > > unpublishing when the device disappears. Or maybe this is a case of lost > > in translation, and you mean something else? > > Let me try to explain what I meant. > > We execute ->connect() callback of USB device when USB device is still in "disconnected" state > (well at least that was my thought). And only when that ->connect() callback succeeds we're > entering "connected" state. > > Probably above sentence is not correct. Then could you please help me with correct wording of > comment to?drm_connector_register_all()? Yeah, your sentence suggests more that register_all should be called when someone unplugs udl. What about: "This function registers all connectors in sysfs and other places so that userspace can start to access them. Drivers can call it after calling drm_dev_register() to complete the device registration, if they don't call drm_connector_register() on each connector individually. When a device is unplugged and should be removed from userspace access, call drm_connector_unregister_all(), which is the inverse of this function." And for drm_connector_unregister_all: "This functions unregisters all connectors from sysfs and other places so that userspace can no longer access them. Drivers should call this as the first step tearing down the device instace, or when the underlying physical device disappeared (e.g. USB unplug), right before calling drm_dev_unregister()." Thinking about this we might want to merge drm_dev_register and drm_connector_register_all, but that's a long-term goal which will need some trickery to no break drivers. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch