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 <abrod...@synopsys.com>
> > Cc: Daniel Vetter <dan...@ffwll.ch>
> > Cc: David Airlie <airl...@linux.ie>
> > ---
> > 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."
I did mean it. Or is this statement is incorrect and example of invocation of
drm_connector_register_all() should be different? Which one works better then?
> >
> > + * ->connect callback.
> Returns: section is missing, specifying how this can fail. Just copy the
> one from connector_register().
Yeah, correct. Blind copy-paste doesn't work equally good always :(
> >
> > + */
> > +int drm_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) {
> for_each_connector here please. And the s/plug/register/ naming discussion
> we've had.
Ok.
> >
> > + 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;
> > +}
> > +EXPORT_SYMBOL(drm_connector_plug_all);
> >
> > /**
> > * drm_connector_unplug_all - unregister connector userspace interfaces
> > @@ -1093,10 +1133,12 @@ void drm_connector_unplug_all(struct drm_device
> > *dev)
> > {
> > struct drm_connector *connector;
> >
> > - /* FIXME: taking the mode config mutex ends up in a clash with sysfs */
> > + mutex_lock(&dev->mode_config.mutex);
> You can't drop that FIXME, the bug is still there.
That's clear given your explanation in the previous email.
> >
> > +
> > list_for_each_entry(connector, &dev->mode_config.connector_list, head)
> > drm_connector_unregister(connector);
> >
> > + mutex_unlock(&dev->mode_config.mutex);
> > }
> > EXPORT_SYMBOL(drm_connector_unplug_all);
> >
> > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> > index 167c8d3..4a559c6 100644
> > --- a/drivers/gpu/drm/drm_drv.c
> > +++ b/drivers/gpu/drm/drm_drv.c
> > @@ -715,7 +715,8 @@ 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 and right after drm_dev_register() driver should call
> It'd do 2 sentences here for simplicity, not connect them with and. Also
> "... _the_ driver should ..."
Ok.
> >
> > + * drm_connector_plug_all() to register all connectors in sysfs.
> Maybe mention why this is separate: "This is a separate call for backwards
> compatibility with drivers still using the deprecated ->load() callback,
> where connectors are registered from within the ->load() callback."
Ok.
-Alexey
