Re: [PATCH] drm: Use static attribute groups for managing connector sysfs entries

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Feb 04, 2015 at 11:58:53AM +0100, Takashi Iwai wrote:
> Instead of manual calls of device_create_file() and
> device_remove_file(), assign the static attribute groups to the device
> with device_create_with_groups().  The conditionally built sysfs
> entries are handled via is_visible callback.
> 
> This simplifies the code and also avoids the possible races.
> 
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>

lgtm, pulled into my drm-misc branch.
-Daniel

> ---
>  drivers/gpu/drm/drm_sysfs.c | 132 ++++++++++++++++++++++----------------------
>  1 file changed, 67 insertions(+), 65 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index cc3d6d6d67e0..5c99d3773212 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -339,19 +339,51 @@ static ssize_t select_subconnector_show(struct device *device,
>  			drm_get_dvi_i_select_name((int)subconnector));
>  }
>  
> -static struct device_attribute connector_attrs[] = {
> -	__ATTR_RO(status),
> -	__ATTR_RO(enabled),
> -	__ATTR_RO(dpms),
> -	__ATTR_RO(modes),
> +static DEVICE_ATTR_RO(status);
> +static DEVICE_ATTR_RO(enabled);
> +static DEVICE_ATTR_RO(dpms);
> +static DEVICE_ATTR_RO(modes);
> +
> +static struct attribute *connector_dev_attrs[] = {
> +	&dev_attr_status.attr,
> +	&dev_attr_enabled.attr,
> +	&dev_attr_dpms.attr,
> +	&dev_attr_modes.attr,
> +	NULL
>  };
>  
>  /* These attributes are for both DVI-I connectors and all types of tv-out. */
> -static struct device_attribute connector_attrs_opt1[] = {
> -	__ATTR_RO(subconnector),
> -	__ATTR_RO(select_subconnector),
> +static DEVICE_ATTR_RO(subconnector);
> +static DEVICE_ATTR_RO(select_subconnector);
> +
> +static struct attribute *connector_opt_dev_attrs[] = {
> +	&dev_attr_subconnector.attr,
> +	&dev_attr_select_subconnector.attr,
> +	NULL
>  };
>  
> +static umode_t connector_opt_dev_is_visible(struct kobject *kobj,
> +					    struct attribute *attr, int idx)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct drm_connector *connector = to_drm_connector(dev);
> +
> +	/*
> +	 * In the long run it maybe a good idea to make one set of
> +	 * optionals per connector type.
> +	 */
> +	switch (connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_DVII:
> +	case DRM_MODE_CONNECTOR_Composite:
> +	case DRM_MODE_CONNECTOR_SVIDEO:
> +	case DRM_MODE_CONNECTOR_Component:
> +	case DRM_MODE_CONNECTOR_TV:
> +		return attr->mode;
> +	}
> +
> +	return 0;
> +}
> +
>  static struct bin_attribute edid_attr = {
>  	.attr.name = "edid",
>  	.attr.mode = 0444,
> @@ -359,6 +391,27 @@ static struct bin_attribute edid_attr = {
>  	.read = edid_show,
>  };
>  
> +static struct bin_attribute *connector_bin_attrs[] = {
> +	&edid_attr,
> +	NULL
> +};
> +
> +static const struct attribute_group connector_dev_group = {
> +	.attrs = connector_dev_attrs,
> +	.bin_attrs = connector_bin_attrs,
> +};
> +
> +static const struct attribute_group connector_opt_dev_group = {
> +	.attrs = connector_opt_dev_attrs,
> +	.is_visible = connector_opt_dev_is_visible,
> +};
> +
> +static const struct attribute_group *connector_dev_groups[] = {
> +	&connector_dev_group,
> +	&connector_opt_dev_group,
> +	NULL
> +};
> +
>  /**
>   * drm_sysfs_connector_add - add a connector to sysfs
>   * @connector: connector to add
> @@ -371,73 +424,27 @@ static struct bin_attribute edid_attr = {
>  int drm_sysfs_connector_add(struct drm_connector *connector)
>  {
>  	struct drm_device *dev = connector->dev;
> -	int attr_cnt = 0;
> -	int opt_cnt = 0;
> -	int i;
> -	int ret;
>  
>  	if (connector->kdev)
>  		return 0;
>  
> -	connector->kdev = device_create(drm_class, dev->primary->kdev,
> -					0, connector, "card%d-%s",
> -					dev->primary->index, connector->name);
> +	connector->kdev =
> +		device_create_with_groups(drm_class, dev->primary->kdev, 0,
> +					  connector, connector_dev_groups,
> +					  "card%d-%s", dev->primary->index,
> +					  connector->name);
>  	DRM_DEBUG("adding \"%s\" to sysfs\n",
>  		  connector->name);
>  
>  	if (IS_ERR(connector->kdev)) {
>  		DRM_ERROR("failed to register connector device: %ld\n", PTR_ERR(connector->kdev));
> -		ret = PTR_ERR(connector->kdev);
> -		goto out;
> -	}
> -
> -	/* Standard attributes */
> -
> -	for (attr_cnt = 0; attr_cnt < ARRAY_SIZE(connector_attrs); attr_cnt++) {
> -		ret = device_create_file(connector->kdev, &connector_attrs[attr_cnt]);
> -		if (ret)
> -			goto err_out_files;
> +		return PTR_ERR(connector->kdev);
>  	}
>  
> -	/* Optional attributes */
> -	/*
> -	 * In the long run it maybe a good idea to make one set of
> -	 * optionals per connector type.
> -	 */
> -	switch (connector->connector_type) {
> -		case DRM_MODE_CONNECTOR_DVII:
> -		case DRM_MODE_CONNECTOR_Composite:
> -		case DRM_MODE_CONNECTOR_SVIDEO:
> -		case DRM_MODE_CONNECTOR_Component:
> -		case DRM_MODE_CONNECTOR_TV:
> -			for (opt_cnt = 0; opt_cnt < ARRAY_SIZE(connector_attrs_opt1); opt_cnt++) {
> -				ret = device_create_file(connector->kdev, &connector_attrs_opt1[opt_cnt]);
> -				if (ret)
> -					goto err_out_files;
> -			}
> -			break;
> -		default:
> -			break;
> -	}
> -
> -	ret = sysfs_create_bin_file(&connector->kdev->kobj, &edid_attr);
> -	if (ret)
> -		goto err_out_files;
> -
>  	/* Let userspace know we have a new connector */
>  	drm_sysfs_hotplug_event(dev);
>  
>  	return 0;
> -
> -err_out_files:
> -	for (i = 0; i < opt_cnt; i++)
> -		device_remove_file(connector->kdev, &connector_attrs_opt1[i]);
> -	for (i = 0; i < attr_cnt; i++)
> -		device_remove_file(connector->kdev, &connector_attrs[i]);
> -	device_unregister(connector->kdev);
> -
> -out:
> -	return ret;
>  }
>  
>  /**
> @@ -455,16 +462,11 @@ out:
>   */
>  void drm_sysfs_connector_remove(struct drm_connector *connector)
>  {
> -	int i;
> -
>  	if (!connector->kdev)
>  		return;
>  	DRM_DEBUG("removing \"%s\" from sysfs\n",
>  		  connector->name);
>  
> -	for (i = 0; i < ARRAY_SIZE(connector_attrs); i++)
> -		device_remove_file(connector->kdev, &connector_attrs[i]);
> -	sysfs_remove_bin_file(&connector->kdev->kobj, &edid_attr);
>  	device_unregister(connector->kdev);
>  	connector->kdev = NULL;
>  }
> -- 
> 2.2.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux