Hi
Some minor nitpicks below:
On Tue, May 13, 2014 at 5:30 PM, Thierry Reding
<thierry.reding@xxxxxxxxx> wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
>
> Add a helper function that allows drivers to statically set the unique
> name of the device. This will allow platform and USB drivers to get rid
> of their DRM bus implementations and directly use drm_dev_alloc() and
> drm_dev_register().
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> Changes in v2:
> - move drm_set_unique() to drivers/gpu/drm/drm_stub.c
> - add kernel-doc
>
> drivers/gpu/drm/drm_ioctl.c | 23 +++++++++++++++++------
> drivers/gpu/drm/drm_stub.c | 24 ++++++++++++++++++++++++
> include/drm/drmP.h | 3 +++
> 3 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 38269d5aa333..a59f6c2d7586 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -131,13 +131,24 @@ static int drm_set_busid(struct drm_device *dev, struct drm_file *file_priv)
> if (master->unique != NULL)
> drm_unset_busid(dev, master);
>
> - ret = dev->driver->bus->set_busid(dev, master);
> - if (ret)
> - goto err;
> + if (dev->driver->bus && dev->driver->bus->set_busid) {
> + ret = dev->driver->bus->set_busid(dev, master);
> + if (ret) {
> + drm_unset_busid(dev, master);
> + return ret;
> + }
> + } else {
> + WARN(dev->unique == NULL,
> + "No drm_bus.set_busid() implementation provided by %ps. "
> + "Set the unique name explicitly using drm_set_unique().",
> + dev->driver);
return -EINVAL?
If you don't bail out here, the kstrdup() below will cause a
kernel-oops. There's no reason to WARN() if you purposely cause an
oops below.
> +
> + master->unique = kstrdup(dev->unique, GFP_KERNEL);
> + if (master->unique)
> + master->unique_len = strlen(dev->unique);
> + }
> +
> return 0;
> -err:
> - drm_unset_busid(dev, master);
> - return ret;
> }
>
> /**
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 1447b0ee3676..64cf97dc4ce4 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -535,6 +535,29 @@ static void drm_fs_inode_free(struct inode *inode)
> }
>
> /**
> + * drm_set_unique - Set the unique name of a DRM device
> + * @dev: device of which to set the unique name
> + * @fmt: format string for unique name
> + *
> + * Sets the unique name of a DRM device using the specified format string and
> + * a variable list of arguments. Drivers can use this at driver probe time if
> + * the unique name of the devices they drive is static.
> + */
> +int drm_set_unique(struct drm_device *dev, const char *fmt, ...)
I tried renaming all the device-management APIs to "drm_dev_*", so how
about moving this below drm_dev_unregister() and renaming it to
drm_dev_set_unique()? I prefer prefixes which describe the object the
function works on. set_unique() works on drm_device, so lets use the
known drm_dev_* prefix.
Just my opinion, others might disagree..
> +{
> + va_list ap;
> +
> + kfree(dev->unique);
> +
> + va_start(ap, fmt);
> + dev->unique = kvasprintf(GFP_KERNEL, fmt, ap);
> + va_end(ap);
> +
> + return dev->unique ? 0 : -ENOMEM;
> +}
> +EXPORT_SYMBOL(drm_set_unique);
> +
> +/**
> * drm_dev_alloc - Allocate new drm device
> * @driver: DRM driver to allocate device for
> * @parent: Parent device object
> @@ -649,6 +672,7 @@ static void drm_dev_release(struct kref *ref)
> drm_minor_free(dev, DRM_MINOR_CONTROL);
>
> mutex_destroy(&dev->master_mutex);
> + kfree(dev->unique);
> kfree(dev);
> }
>
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 6b4ed9e84e6f..dcec96b1154c 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1235,6 +1235,8 @@ struct drm_device {
> struct drm_vma_offset_manager *vma_offset_manager;
> /*@} */
> int switch_power_state;
> +
> + char *unique;
Can you add this to the group at the top of the object? This somehow belongs to
/** \name Lifetime Management */
as it is part of device initialization and API.
> };
>
> #define DRM_SWITCH_POWER_ON 0
> @@ -1680,6 +1682,7 @@ static __inline__ void drm_core_dropmap(struct drm_local_map *map)
>
> #include <drm/drm_mem_util.h>
>
> +int drm_set_unique(struct drm_device *dev, const char *fmt, ...);
Again just a minor style-nitpick: Move that below the drm_dev_*()
declarations, not above. Usual kernel-style is to have
allocators/lifetime-management first, followed by runtime management.
Otherwise looks good:
Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx>
Thanks
David
> struct drm_device *drm_dev_alloc(struct drm_driver *driver,
> struct device *parent);
> void drm_dev_ref(struct drm_device *dev);
> --
> 1.9.2
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
