| Message ID | 1423047533-18467-1-git-send-email-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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; }
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> --- drivers/gpu/drm/drm_sysfs.c | 132 ++++++++++++++++++++++---------------------- 1 file changed, 67 insertions(+), 65 deletions(-)