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 <abrod...@synopsys.com>
> Cc: Daniel Vetter <dan...@ffwll.ch>
> Cc: David Airlie <airl...@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
