[PATCH 0/4 v2] drm: introduce drm_connector_register_all() helper

[PATCH 0/4 v2] drm: introduce drm_connector_register_all() helper

[Alexey Brodkin]

As a pair to already existing drm_connector_unplug_all()
(which we'll rename in this series to drm_connector_unregister_all())
we're adding generic implementation of what is already done in some drivers
for registering all connectors.

After implementation of that new helper we're updating 2 drivers
that used to use it's own implementation:
 [1] atmel_hlcdc
 [2] rcar_du

And one driver that uses unregister():
 [1] udl

Other drivers still use load() callback and so should be first modified so
their load() gets called from their probe() explicitly.

Build- and run-tested on yet to be upstreamed ARC PGU (part of AXS10x board).

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

Alexey Brodkin (4):
  drm: rename drm_connector_unplug_all() to
    drm_connector_unregister_all()
  drm: introduce drm_connector_register_all() helper
  drm: atmel_hldc - use generic drm_connector_register_all() helper
  drm: rcar-du - use generic drm_connector_register_all() helper

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 +------------------
 drivers/gpu/drm/drm_crtc.c                   | 56 +++++++++++++++++++++++++---
 drivers/gpu/drm/drm_drv.c                    |  6 ++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.c        | 14 +------
 drivers/gpu/drm/udl/udl_drv.c                |  2 +-
 include/drm/drm_crtc.h                       |  5 ++-
 6 files changed, 63 insertions(+), 59 deletions(-)

Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
-- 
2.5.0

[PATCH 1/4 v2] drm: rename drm_connector_unplug_all() to drm_connector_unregister_all()

[Alexey Brodkin]

Current name is a bit misleading because what that helper function
really does it calls drm_connector_unregister() for all connectors.

This all has nothing to do with hotplugging so let's name things
properly.

And while at it remove potentially dangerous locking around
drm_connector_unregister() in rcar_du_remove() as mentioned
in kerneldoc for drm_connector_unregister_all().

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---

Changes v1 -> v2:
 * This patch was only introduced in v2.

 drivers/gpu/drm/drm_crtc.c            | 6 +++---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 5 +----
 drivers/gpu/drm/udl/udl_drv.c         | 2 +-
 include/drm/drm_crtc.h                | 4 ++--
 4 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 65258ac..76453cd 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1082,14 +1082,14 @@ EXPORT_SYMBOL(drm_connector_unregister);
 
 
 /**
- * drm_connector_unplug_all - unregister connector userspace interfaces
+ * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
  * This function unregisters all connector userspace interfaces in sysfs. Should
  * be call when the device is disconnected, e.g. from an usb driver's
  * ->disconnect callback.
  */
-void drm_connector_unplug_all(struct drm_device *dev)
+void drm_connector_unregister_all(struct drm_device *dev)
 {
 	struct drm_connector *connector;
 
@@ -1098,7 +1098,7 @@ void drm_connector_unplug_all(struct drm_device *dev)
 		drm_connector_unregister(connector);
 
 }
-EXPORT_SYMBOL(drm_connector_unplug_all);
+EXPORT_SYMBOL(drm_connector_unregister_all);
 
 /**
  * drm_encoder_init - Init a preallocated encoder
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index ed6006b..644db36 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -278,10 +278,7 @@ static int rcar_du_remove(struct platform_device *pdev)
 	struct rcar_du_device *rcdu = platform_get_drvdata(pdev);
 	struct drm_device *ddev = rcdu->ddev;
 
-	mutex_lock(&ddev->mode_config.mutex);
-	drm_connector_unplug_all(ddev);
-	mutex_unlock(&ddev->mode_config.mutex);
-
+	drm_connector_unregister_all(ddev);
 	drm_dev_unregister(ddev);
 
 	if (rcdu->fbdev)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 772ec9e..c204089 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -94,7 +94,7 @@ static void udl_usb_disconnect(struct usb_interface *interface)
 	struct drm_device *dev = usb_get_intfdata(interface);
 
 	drm_kms_helper_poll_disable(dev);
-	drm_connector_unplug_all(dev);
+	drm_connector_unregister_all(dev);
 	udl_fbdev_unplug(dev);
 	udl_drop_usb(dev);
 	drm_unplug_dev(dev);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 8c7fb3d..42d9f4d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2214,8 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unplug all connectors from sysfs for device */
-extern void drm_connector_unplug_all(struct drm_device *dev);
+/* helper to unregister all connectors from sysfs for device */
+extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
 extern void drm_bridge_remove(struct drm_bridge *bridge);
-- 
2.5.0

[PATCH 2/4 v2] drm: introduce drm_connector_register_all() helper

[Alexey Brodkin]

As a pair to already existing drm_connector_unregister_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 the generic one.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
---

Changes v1 -> v2:
 * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
 * Use drm_for_each_connector() instead of list_for_each_entry()
 * Updated kerneldoc for drm_dev_register()

 drivers/gpu/drm/drm_crtc.c | 50 +++++++++++++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/drm_drv.c  |  6 +++++-
 include/drm/drm_crtc.h     |  3 ++-
 3 files changed, 54 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 76453cd..2ad0ce9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1080,13 +1080,57 @@ void drm_connector_unregister(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_connector_unregister);
 
+/**
+ * drm_connector_register_all - register all connectors
+ * @dev: drm device
+ *
+ * This function just calls drm_connector_register() for all connectors.
+ * Should be called when the device is disconnected, e.g. from an usb driver's
+ * ->connect callback.
+ *
+ * Returns:
+ * Zero on success, error code on failure.
+ */
+int drm_connector_register_all(struct drm_device *dev)
+{
+	struct drm_connector *connector, *failed;
+	int ret;
+
+	mutex_lock(&dev->mode_config.mutex);
+
+	drm_for_each_connector(connector, dev) {
+		ret = drm_connector_register(connector);
+		if (ret) {
+			failed = connector;
+			goto err;
+		}
+	}
+
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return 0;
+
+err:
+	drm_for_each_connector(connector, dev) {
+		if (failed == connector)
+			break;
+
+		drm_connector_unregister(connector);
+	}
+
+	mutex_unlock(&dev->mode_config.mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(drm_connector_register_all);
 
 /**
  * drm_connector_unregister_all - unregister connector userspace interfaces
  * @dev: drm device
  *
  * This function unregisters all connector userspace interfaces in sysfs. Should
- * be call when the device is disconnected, e.g. from an usb driver's
+ * be called when the device is disconnected, e.g. from an usb driver's
  * ->disconnect callback.
  */
 void drm_connector_unregister_all(struct drm_device *dev)
@@ -1094,9 +1138,9 @@ void drm_connector_unregister_all(struct drm_device *dev)
 	struct drm_connector *connector;
 
 	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
+	drm_for_each_connector(connector, dev) {
 		drm_connector_unregister(connector);
-
+	}
 }
 EXPORT_SYMBOL(drm_connector_unregister_all);
 
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 167c8d3..5580a90 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -715,7 +715,11 @@ EXPORT_SYMBOL(drm_dev_unref);
  *
  * Register the DRM device @dev with the system, advertise device to user-space
  * and start normal device operation. @dev must be allocated via drm_dev_alloc()
- * previously.
+ * previously. Right after drm_dev_register() the driver should call
+ * drm_connector_plug_all() to register all connectors in sysfs. This is
+ * a separate call for backward compatibility with drivers still using
+ * the deprecated ->load() callback, where connectors are registered from within
+ * the ->load() callback.
  *
  * Never call this twice on any device!
  *
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 42d9f4d..6a34117 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -2214,7 +2214,8 @@ void drm_connector_unregister(struct drm_connector *connector);
 
 extern void drm_connector_cleanup(struct drm_connector *connector);
 extern unsigned int drm_connector_index(struct drm_connector *connector);
-/* helper to unregister all connectors from sysfs for device */
+/* helpers to {un}register all connectors from sysfs for device */
+extern int drm_connector_register_all(struct drm_device *dev);
 extern void drm_connector_unregister_all(struct drm_device *dev);
 
 extern int drm_bridge_add(struct drm_bridge *bridge);
-- 
2.5.0

[PATCH 3/4 v2] drm: atmel_hldc - use generic drm_connector_register_all() helper

[Alexey Brodkin]

This driver used to have its own implementation of connector_register_all()
which actually was taken as a prototype of drm_connector_register_all().

Now when drm_connector_register_all() exists reusing it here.

And while at it replace atmel_hlcdc_dc_connector_unplug_all()
with generic drm_connector_unregister_all().

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
---

No changes v1 -> v2.

 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 ++--------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 3d8d164..1c537e4 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -584,41 +584,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 	destroy_workqueue(dc->wq);
 }
 
-static int atmel_hlcdc_dc_connector_plug_all(struct drm_device *dev)
-{
-	struct drm_connector *connector, *failed;
-	int ret;
-
-	mutex_lock(&dev->mode_config.mutex);
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		ret = drm_connector_register(connector);
-		if (ret) {
-			failed = connector;
-			goto err;
-		}
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-	return 0;
-
-err:
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (failed == connector)
-			break;
-
-		drm_connector_unregister(connector);
-	}
-	mutex_unlock(&dev->mode_config.mutex);
-
-	return ret;
-}
-
-static void atmel_hlcdc_dc_connector_unplug_all(struct drm_device *dev)
-{
-	mutex_lock(&dev->mode_config.mutex);
-	drm_connector_unplug_all(dev);
-	mutex_unlock(&dev->mode_config.mutex);
-}
-
 static void atmel_hlcdc_dc_lastclose(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
@@ -736,7 +701,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_unload;
 
-	ret = atmel_hlcdc_dc_connector_plug_all(ddev);
+	ret = drm_connector_register_all(ddev);
 	if (ret)
 		goto err_unregister;
 
@@ -758,7 +723,7 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
 {
 	struct drm_device *ddev = platform_get_drvdata(pdev);
 
-	atmel_hlcdc_dc_connector_unplug_all(ddev);
+	drm_connector_unregister_all(ddev);
 	drm_dev_unregister(ddev);
 	atmel_hlcdc_dc_unload(ddev);
 	drm_dev_unref(ddev);
-- 
2.5.0

[PATCH 4/4 v2] drm: rcar-du - use generic drm_connector_register_all() helper

[Alexey Brodkin]

Now when generic drm_connector_register_all() helper exists we may safely
substiture with it driver-specific implementation of connectors plugging in
sysfs.

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: David Airlie <airlied@linux.ie>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---

No changes v1 -> v2.

 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 644db36..0f251dc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -361,14 +361,7 @@ static int rcar_du_probe(struct platform_device *pdev)
 	if (ret)
 		goto error;
 
-	mutex_lock(&ddev->mode_config.mutex);
-	drm_for_each_connector(connector, ddev) {
-		ret = drm_connector_register(connector);
-		if (ret < 0)
-			break;
-	}
-	mutex_unlock(&ddev->mode_config.mutex);
-
+	ret = drm_connector_register_all(ddev);
 	if (ret < 0)
 		goto error;
 
-- 
2.5.0

Re: [PATCH 3/4 v2] drm: atmel_hldc - use generic drm_connector_register_all() helper

[Boris Brezillon]

On Mon, 21 Mar 2016 15:28:39 +0300
Alexey Brodkin <Alexey.Brodkin@synopsys.com> wrote:

> This driver used to have its own implementation of connector_register_all()
> which actually was taken as a prototype of drm_connector_register_all().
> 
> Now when drm_connector_register_all() exists reusing it here.
> 
> And while at it replace atmel_hlcdc_dc_connector_unplug_all()
> with generic drm_connector_unregister_all().
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>

Acked-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> ---
> 
> No changes v1 -> v2.
> 
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 39 ++--------------------------
>  1 file changed, 2 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> index 3d8d164..1c537e4 100644
> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> @@ -584,41 +584,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
>  	destroy_workqueue(dc->wq);
>  }
>  
> -static int atmel_hlcdc_dc_connector_plug_all(struct drm_device *dev)
> -{
> -	struct drm_connector *connector, *failed;
> -	int ret;
> -
> -	mutex_lock(&dev->mode_config.mutex);
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		ret = drm_connector_register(connector);
> -		if (ret) {
> -			failed = connector;
> -			goto err;
> -		}
> -	}
> -	mutex_unlock(&dev->mode_config.mutex);
> -	return 0;
> -
> -err:
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		if (failed == connector)
> -			break;
> -
> -		drm_connector_unregister(connector);
> -	}
> -	mutex_unlock(&dev->mode_config.mutex);
> -
> -	return ret;
> -}
> -
> -static void atmel_hlcdc_dc_connector_unplug_all(struct drm_device *dev)
> -{
> -	mutex_lock(&dev->mode_config.mutex);
> -	drm_connector_unplug_all(dev);
> -	mutex_unlock(&dev->mode_config.mutex);
> -}
> -
>  static void atmel_hlcdc_dc_lastclose(struct drm_device *dev)
>  {
>  	struct atmel_hlcdc_dc *dc = dev->dev_private;
> @@ -736,7 +701,7 @@ static int atmel_hlcdc_dc_drm_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_unload;
>  
> -	ret = atmel_hlcdc_dc_connector_plug_all(ddev);
> +	ret = drm_connector_register_all(ddev);
>  	if (ret)
>  		goto err_unregister;
>  
> @@ -758,7 +723,7 @@ static int atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
>  {
>  	struct drm_device *ddev = platform_get_drvdata(pdev);
>  
> -	atmel_hlcdc_dc_connector_unplug_all(ddev);
> +	drm_connector_unregister_all(ddev);
>  	drm_dev_unregister(ddev);
>  	atmel_hlcdc_dc_unload(ddev);
>  	drm_dev_unref(ddev);



-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH 1/4 v2] drm: rename drm_connector_unplug_all() to drm_connector_unregister_all()

[Laurent Pinchart]

Hi Alexey,

Thank you for the patch.

On Monday 21 Mar 2016 15:28:37 Alexey Brodkin wrote:
> Current name is a bit misleading because what that helper function
> really does it calls drm_connector_unregister() for all connectors.
> 
> This all has nothing to do with hotplugging so let's name things
> properly.
> 
> And while at it remove potentially dangerous locking around
> drm_connector_unregister() in rcar_du_remove() as mentioned
> in kerneldoc for drm_connector_unregister_all().
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> Changes v1 -> v2:
>  * This patch was only introduced in v2.
> 
>  drivers/gpu/drm/drm_crtc.c            | 6 +++---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 5 +----
>  drivers/gpu/drm/udl/udl_drv.c         | 2 +-
>  include/drm/drm_crtc.h                | 4 ++--
>  4 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 65258ac..76453cd 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1082,14 +1082,14 @@ EXPORT_SYMBOL(drm_connector_unregister);
> 
> 
>  /**
> - * drm_connector_unplug_all - unregister connector userspace interfaces
> + * drm_connector_unregister_all - unregister connector userspace interfaces
> * @dev: drm device
>   *
>   * This function unregisters all connector userspace interfaces in sysfs.
> Should * be call when the device is disconnected, e.g. from an usb driver's
> * ->disconnect callback.
>   */
> -void drm_connector_unplug_all(struct drm_device *dev)
> +void drm_connector_unregister_all(struct drm_device *dev)
>  {
>  	struct drm_connector *connector;
> 
> @@ -1098,7 +1098,7 @@ void drm_connector_unplug_all(struct drm_device *dev)
>  		drm_connector_unregister(connector);
> 
>  }
> -EXPORT_SYMBOL(drm_connector_unplug_all);
> +EXPORT_SYMBOL(drm_connector_unregister_all);
> 
>  /**
>   * drm_encoder_init - Init a preallocated encoder
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index ed6006b..644db36 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -278,10 +278,7 @@ static int rcar_du_remove(struct platform_device *pdev)
> struct rcar_du_device *rcdu = platform_get_drvdata(pdev);
>  	struct drm_device *ddev = rcdu->ddev;
> 
> -	mutex_lock(&ddev->mode_config.mutex);
> -	drm_connector_unplug_all(ddev);
> -	mutex_unlock(&ddev->mode_config.mutex);
> -
> +	drm_connector_unregister_all(ddev);
>  	drm_dev_unregister(ddev);
> 
>  	if (rcdu->fbdev)
> diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
> index 772ec9e..c204089 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> @@ -94,7 +94,7 @@ static void udl_usb_disconnect(struct usb_interface
> *interface) struct drm_device *dev = usb_get_intfdata(interface);
> 
>  	drm_kms_helper_poll_disable(dev);
> -	drm_connector_unplug_all(dev);
> +	drm_connector_unregister_all(dev);
>  	udl_fbdev_unplug(dev);
>  	udl_drop_usb(dev);
>  	drm_unplug_dev(dev);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 8c7fb3d..42d9f4d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2214,8 +2214,8 @@ void drm_connector_unregister(struct drm_connector
> *connector);
> 
>  extern void drm_connector_cleanup(struct drm_connector *connector);
>  extern unsigned int drm_connector_index(struct drm_connector *connector);
> -/* helper to unplug all connectors from sysfs for device */
> -extern void drm_connector_unplug_all(struct drm_device *dev);
> +/* helper to unregister all connectors from sysfs for device */
> +extern void drm_connector_unregister_all(struct drm_device *dev);
> 
>  extern int drm_bridge_add(struct drm_bridge *bridge);
>  extern void drm_bridge_remove(struct drm_bridge *bridge);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/4 v2] drm: rcar-du - use generic drm_connector_register_all() helper

[Laurent Pinchart]

Hi Alexey,

Thank you for the patch.

On Monday 21 Mar 2016 15:28:40 Alexey Brodkin wrote:
> Now when generic drm_connector_register_all() helper exists we may safely

s/Now when/Now that a/

> substiture with it driver-specific implementation of connectors plugging in

s/substiture with it/substitute it for the/

> sysfs.

And while at it, in the subject line,

s/rcar-du - use/rcar-du: Use/

> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org

Apart for the commit message fixes,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> No changes v1 -> v2.
> 
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> b/drivers/gpu/drm/rcar-du/rcar_du_drv.c index 644db36..0f251dc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -361,14 +361,7 @@ static int rcar_du_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto error;
> 
> -	mutex_lock(&ddev->mode_config.mutex);
> -	drm_for_each_connector(connector, ddev) {
> -		ret = drm_connector_register(connector);
> -		if (ret < 0)
> -			break;
> -	}
> -	mutex_unlock(&ddev->mode_config.mutex);
> -
> +	ret = drm_connector_register_all(ddev);
>  	if (ret < 0)
>  		goto error;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/4 v2] drm: introduce drm_connector_register_all() helper

[Laurent Pinchart]

Hi Alexey,

Thank you for the patch.

On Monday 21 Mar 2016 15:28:38 Alexey Brodkin wrote:
> As a pair to already existing drm_connector_unregister_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 the generic one.
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: David Airlie <airlied@linux.ie>
> ---
> 
> Changes v1 -> v2:
>  * Rename drm_connector_unplug_all() to drm_connector_unregister_all()
>  * Use drm_for_each_connector() instead of list_for_each_entry()
>  * Updated kerneldoc for drm_dev_register()
> 
>  drivers/gpu/drm/drm_crtc.c | 50 ++++++++++++++++++++++++++++++++++++++++---
>  drivers/gpu/drm/drm_drv.c  |  6 +++++-
>  include/drm/drm_crtc.h     |  3 ++-
>  3 files changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 76453cd..2ad0ce9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1080,13 +1080,57 @@ void drm_connector_unregister(struct drm_connector
> *connector) }
>  EXPORT_SYMBOL(drm_connector_unregister);
> 
> +/**
> + * drm_connector_register_all - register all connectors
> + * @dev: drm device
> + *
> + * This function just calls drm_connector_register() for all connectors.
> + * Should be called when the device is disconnected, e.g. from an usb
> driver's
> + * ->connect callback.

There's no such thing as a USB driver connect callback :-)

> + *
> + * Returns:
> + * Zero on success, error code on failure.
> + */
> +int drm_connector_register_all(struct drm_device *dev)
> +{
> +	struct drm_connector *connector, *failed;
> +	int ret;
> +
> +	mutex_lock(&dev->mode_config.mutex);
> +
> +	drm_for_each_connector(connector, dev) {
> +		ret = drm_connector_register(connector);
> +		if (ret) {
> +			failed = connector;
> +			goto err;
> +		}
> +	}
> +
> +

One blank line is enough.

> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	return 0;
> +
> +err:
> +	drm_for_each_connector(connector, dev) {
> +		if (failed == connector)
> +			break;
> +
> +		drm_connector_unregister(connector);
> +	}

drm_connector_unregister() can safely be called on unregistered connectors, so 
you could simplify the implementation by calling 
drm_connector_unregister_all() above and remove the goto and this piece of 
error handling code.

> +	mutex_unlock(&dev->mode_config.mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(drm_connector_register_all);
> 
>  /**
>   * drm_connector_unregister_all - unregister connector userspace interfaces
> * @dev: drm device
>   *
>   * This function unregisters all connector userspace interfaces in sysfs.
> Should
> - * be call when the device is disconnected, e.g. from an usb driver's
> + * be called when the device is disconnected, e.g. from an usb driver's
>   * ->disconnect callback.
>   */
>  void drm_connector_unregister_all(struct drm_device *dev)
> @@ -1094,9 +1138,9 @@ void drm_connector_unregister_all(struct drm_device
> *dev) struct drm_connector *connector;
> 
>  	/* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> +	drm_for_each_connector(connector, dev) {
>  		drm_connector_unregister(connector);
> -
> +	}
>  }
>  EXPORT_SYMBOL(drm_connector_unregister_all);

I would move these two hunks to the previous patch as you already touch 
drm_connector_unregister_all() there, but that's up to you.

> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index 167c8d3..5580a90 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -715,7 +715,11 @@ EXPORT_SYMBOL(drm_dev_unref);
>   *
>   * Register the DRM device @dev with the system, advertise device to
> user-space * and start normal device operation. @dev must be allocated via
> drm_dev_alloc() - * previously.
> + * previously. Right after drm_dev_register() the driver should call
> + * drm_connector_plug_all() to register all connectors in sysfs. This is

s/drm_connector_plug_all/drm_connector_register_all/

> + * a separate call for backward compatibility with drivers still using
> + * the deprecated ->load() callback, where connectors are registered from
> within
> + * the ->load() callback.
>   *
>   * Never call this twice on any device!
>   *
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 42d9f4d..6a34117 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -2214,7 +2214,8 @@ void drm_connector_unregister(struct drm_connector
> *connector);
> 
>  extern void drm_connector_cleanup(struct drm_connector *connector);
>  extern unsigned int drm_connector_index(struct drm_connector *connector);
> -/* helper to unregister all connectors from sysfs for device */
> +/* helpers to {un}register all connectors from sysfs for device */
> +extern int drm_connector_register_all(struct drm_device *dev);
>  extern void drm_connector_unregister_all(struct drm_device *dev);
> 
>  extern int drm_bridge_add(struct drm_bridge *bridge);

-- 
Regards,

Laurent Pinchart