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

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@suse.de

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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel