Hi all, I was planning on adding some connector type specific stuff in the sysfs for DP, but ended up scrapping the plan midway through it. However I was left with some drm sysfs cleanup to make it easier; here are the *untested* patches. Basically the series addresses the code comment from Dave since the dawn of modesetting time:
/* * In the long run it maybe a good idea to make one set of * optionals per connector type. */
Any functional or user visible changes in the series are bugs.
BR, Jani.
Jani Nikula (4): drm/sysfs: add a helper for extracting connector type from kobject drm/sysfs: make optional attribute groups per connector type drm/sysfs: split DVI-I and TV-out attributes drm/sysfs: remove unnecessary connector type checks
drivers/gpu/drm/drm_sysfs.c | 160 +++++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 62 deletions(-)
This reduces duplication in the patches to follow. No functional changes.
Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_sysfs.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 5c99d3773212..9ce4d62feb46 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -362,17 +362,23 @@ static struct attribute *connector_opt_dev_attrs[] = { NULL };
-static umode_t connector_opt_dev_is_visible(struct kobject *kobj, - struct attribute *attr, int idx) +/* Connector type related helpers */ +static int kobj_connector_type(struct kobject *kobj) { struct device *dev = kobj_to_dev(kobj); struct drm_connector *connector = to_drm_connector(dev);
+ return connector->connector_type; +} + +static umode_t connector_opt_dev_is_visible(struct kobject *kobj, + struct attribute *attr, int idx) +{ /* * In the long run it maybe a good idea to make one set of * optionals per connector type. */ - switch (connector->connector_type) { + switch (kobj_connector_type(kobj)) { case DRM_MODE_CONNECTOR_DVII: case DRM_MODE_CONNECTOR_Composite: case DRM_MODE_CONNECTOR_SVIDEO:
Split DVI-I and TV-out (which remains a group of types). As an intermediate step, still share the attributes themselves between the two. No user visible changes.
Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_sysfs.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 9ce4d62feb46..edb23c36c74a 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -356,7 +356,7 @@ static struct attribute *connector_dev_attrs[] = { static DEVICE_ATTR_RO(subconnector); static DEVICE_ATTR_RO(select_subconnector);
-static struct attribute *connector_opt_dev_attrs[] = { +static struct attribute *connector_tv_dev_attrs[] = { &dev_attr_subconnector.attr, &dev_attr_select_subconnector.attr, NULL @@ -371,15 +371,17 @@ static int kobj_connector_type(struct kobject *kobj) return connector->connector_type; }
-static umode_t connector_opt_dev_is_visible(struct kobject *kobj, - struct attribute *attr, int idx) +static umode_t connector_is_dvii(struct kobject *kobj, + struct attribute *attr, int idx) +{ + return kobj_connector_type(kobj) == DRM_MODE_CONNECTOR_DVII ? + attr->mode : 0; +} + +static umode_t connector_is_tv(struct kobject *kobj, + struct attribute *attr, int idx) { - /* - * In the long run it maybe a good idea to make one set of - * optionals per connector type. - */ switch (kobj_connector_type(kobj)) { - case DRM_MODE_CONNECTOR_DVII: case DRM_MODE_CONNECTOR_Composite: case DRM_MODE_CONNECTOR_SVIDEO: case DRM_MODE_CONNECTOR_Component: @@ -407,14 +409,20 @@ static const struct attribute_group connector_dev_group = { .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_tv_dev_group = { + .attrs = connector_tv_dev_attrs, + .is_visible = connector_is_tv, +}; + +static const struct attribute_group connector_dvii_dev_group = { + .attrs = connector_tv_dev_attrs, /* same as tv */ + .is_visible = connector_is_dvii, };
static const struct attribute_group *connector_dev_groups[] = { &connector_dev_group, - &connector_opt_dev_group, + &connector_tv_dev_group, + &connector_dvii_dev_group, NULL };
The show methods for the attributes of DVI-I and TV-out types have a bunch of code to deal with the differences between the two. Just split the attributes into connector type specific ones. No functional changes.
Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_sysfs.c | 148 ++++++++++++++++++++++++++++++-------------- 1 file changed, 101 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index edb23c36c74a..d18c5aac6935 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -257,33 +257,28 @@ static ssize_t modes_show(struct device *device, return written; }
-static ssize_t subconnector_show(struct device *device, - struct device_attribute *attr, - char *buf) +static ssize_t tv_subconnector_show(struct device *device, + struct device_attribute *attr, + char *buf) { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev; - struct drm_property *prop = NULL; + struct drm_property *prop; uint64_t subconnector; - int is_tv = 0; int ret;
switch (connector->connector_type) { - case DRM_MODE_CONNECTOR_DVII: - prop = dev->mode_config.dvi_i_subconnector_property; - break; - case DRM_MODE_CONNECTOR_Composite: - case DRM_MODE_CONNECTOR_SVIDEO: - case DRM_MODE_CONNECTOR_Component: - case DRM_MODE_CONNECTOR_TV: - prop = dev->mode_config.tv_subconnector_property; - is_tv = 1; - break; - default: - DRM_ERROR("Wrong connector type for this property\n"); - return 0; + case DRM_MODE_CONNECTOR_Composite: + case DRM_MODE_CONNECTOR_SVIDEO: + case DRM_MODE_CONNECTOR_Component: + case DRM_MODE_CONNECTOR_TV: + break; + default: + DRM_ERROR("Wrong connector type for this property\n"); + return 0; }
+ prop = dev->mode_config.tv_subconnector_property; if (!prop) { DRM_ERROR("Unable to find subconnector property\n"); return 0; @@ -293,38 +288,90 @@ static ssize_t subconnector_show(struct device *device, if (ret) return 0;
- return snprintf(buf, PAGE_SIZE, "%s", is_tv ? - drm_get_tv_subconnector_name((int)subconnector) : - drm_get_dvi_i_subconnector_name((int)subconnector)); + return snprintf(buf, PAGE_SIZE, "%s", + drm_get_tv_subconnector_name((int)subconnector)); }
-static ssize_t select_subconnector_show(struct device *device, - struct device_attribute *attr, - char *buf) +static ssize_t tv_select_subconnector_show(struct device *device, + struct device_attribute *attr, + char *buf) { struct drm_connector *connector = to_drm_connector(device); struct drm_device *dev = connector->dev; - struct drm_property *prop = NULL; + struct drm_property *prop; uint64_t subconnector; - int is_tv = 0; int ret;
switch (connector->connector_type) { - case DRM_MODE_CONNECTOR_DVII: - prop = dev->mode_config.dvi_i_select_subconnector_property; - break; - case DRM_MODE_CONNECTOR_Composite: - case DRM_MODE_CONNECTOR_SVIDEO: - case DRM_MODE_CONNECTOR_Component: - case DRM_MODE_CONNECTOR_TV: - prop = dev->mode_config.tv_select_subconnector_property; - is_tv = 1; - break; - default: - DRM_ERROR("Wrong connector type for this property\n"); - return 0; + case DRM_MODE_CONNECTOR_Composite: + case DRM_MODE_CONNECTOR_SVIDEO: + case DRM_MODE_CONNECTOR_Component: + case DRM_MODE_CONNECTOR_TV: + break; + default: + DRM_ERROR("Wrong connector type for this property\n"); + return 0; + } + + prop = dev->mode_config.tv_select_subconnector_property; + if (!prop) { + DRM_ERROR("Unable to find select subconnector property\n"); + return 0; + } + + ret = drm_object_property_get_value(&connector->base, prop, &subconnector); + if (ret) + return 0; + + return snprintf(buf, PAGE_SIZE, "%s", + drm_get_tv_select_name((int)subconnector)); +} + +static ssize_t dvii_subconnector_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct drm_connector *connector = to_drm_connector(device); + struct drm_device *dev = connector->dev; + struct drm_property *prop; + uint64_t subconnector; + int ret; + + if (connector->connector_type != DRM_MODE_CONNECTOR_DVII) { + DRM_ERROR("Wrong connector type for this property\n"); + return 0; + } + + prop = dev->mode_config.dvi_i_subconnector_property; + if (!prop) { + DRM_ERROR("Unable to find subconnector property\n"); + return 0; + } + + ret = drm_object_property_get_value(&connector->base, prop, &subconnector); + if (ret) + return 0; + + return snprintf(buf, PAGE_SIZE, "%s", + drm_get_dvi_i_subconnector_name((int)subconnector)); +} + +static ssize_t dvii_select_subconnector_show(struct device *device, + struct device_attribute *attr, + char *buf) +{ + struct drm_connector *connector = to_drm_connector(device); + struct drm_device *dev = connector->dev; + struct drm_property *prop; + uint64_t subconnector; + int ret; + + if (connector->connector_type != DRM_MODE_CONNECTOR_DVII) { + DRM_ERROR("Wrong connector type for this property\n"); + return 0; }
+ prop = dev->mode_config.dvi_i_select_subconnector_property; if (!prop) { DRM_ERROR("Unable to find select subconnector property\n"); return 0; @@ -334,8 +381,7 @@ static ssize_t select_subconnector_show(struct device *device, if (ret) return 0;
- return snprintf(buf, PAGE_SIZE, "%s", is_tv ? - drm_get_tv_select_name((int)subconnector) : + return snprintf(buf, PAGE_SIZE, "%s", drm_get_dvi_i_select_name((int)subconnector)); }
@@ -352,13 +398,21 @@ static struct attribute *connector_dev_attrs[] = { NULL };
-/* These attributes are for both DVI-I connectors and all types of tv-out. */ -static DEVICE_ATTR_RO(subconnector); -static DEVICE_ATTR_RO(select_subconnector); +static DEVICE_ATTR_RO(tv_subconnector); +static DEVICE_ATTR_RO(tv_select_subconnector);
static struct attribute *connector_tv_dev_attrs[] = { - &dev_attr_subconnector.attr, - &dev_attr_select_subconnector.attr, + &dev_attr_tv_subconnector.attr, + &dev_attr_tv_select_subconnector.attr, + NULL +}; + +static DEVICE_ATTR_RO(dvii_subconnector); +static DEVICE_ATTR_RO(dvii_select_subconnector); + +static struct attribute *connector_dvii_dev_attrs[] = { + &dev_attr_dvii_subconnector.attr, + &dev_attr_dvii_select_subconnector.attr, NULL };
@@ -415,7 +469,7 @@ static const struct attribute_group connector_tv_dev_group = { };
static const struct attribute_group connector_dvii_dev_group = { - .attrs = connector_tv_dev_attrs, /* same as tv */ + .attrs = connector_dvii_dev_attrs, .is_visible = connector_is_dvii, };
These attributes should be exposed for the matching connector types only, so checking is redundant.
Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_sysfs.c | 32 -------------------------------- 1 file changed, 32 deletions(-)
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index d18c5aac6935..60b9f136ac44 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -267,17 +267,6 @@ static ssize_t tv_subconnector_show(struct device *device, uint64_t subconnector; int ret;
- switch (connector->connector_type) { - case DRM_MODE_CONNECTOR_Composite: - case DRM_MODE_CONNECTOR_SVIDEO: - case DRM_MODE_CONNECTOR_Component: - case DRM_MODE_CONNECTOR_TV: - break; - default: - DRM_ERROR("Wrong connector type for this property\n"); - return 0; - } - prop = dev->mode_config.tv_subconnector_property; if (!prop) { DRM_ERROR("Unable to find subconnector property\n"); @@ -302,17 +291,6 @@ static ssize_t tv_select_subconnector_show(struct device *device, uint64_t subconnector; int ret;
- switch (connector->connector_type) { - case DRM_MODE_CONNECTOR_Composite: - case DRM_MODE_CONNECTOR_SVIDEO: - case DRM_MODE_CONNECTOR_Component: - case DRM_MODE_CONNECTOR_TV: - break; - default: - DRM_ERROR("Wrong connector type for this property\n"); - return 0; - } - prop = dev->mode_config.tv_select_subconnector_property; if (!prop) { DRM_ERROR("Unable to find select subconnector property\n"); @@ -337,11 +315,6 @@ static ssize_t dvii_subconnector_show(struct device *device, uint64_t subconnector; int ret;
- if (connector->connector_type != DRM_MODE_CONNECTOR_DVII) { - DRM_ERROR("Wrong connector type for this property\n"); - return 0; - } - prop = dev->mode_config.dvi_i_subconnector_property; if (!prop) { DRM_ERROR("Unable to find subconnector property\n"); @@ -366,11 +339,6 @@ static ssize_t dvii_select_subconnector_show(struct device *device, uint64_t subconnector; int ret;
- if (connector->connector_type != DRM_MODE_CONNECTOR_DVII) { - DRM_ERROR("Wrong connector type for this property\n"); - return 0; - } - prop = dev->mode_config.dvi_i_select_subconnector_property; if (!prop) { DRM_ERROR("Unable to find select subconnector property\n");
Ping, anyone up for review?
BR, Jani.
On Tue, 24 Feb 2015, Jani Nikula jani.nikula@intel.com wrote:
Hi all, I was planning on adding some connector type specific stuff in the sysfs for DP, but ended up scrapping the plan midway through it. However I was left with some drm sysfs cleanup to make it easier; here are the *untested* patches. Basically the series addresses the code comment from Dave since the dawn of modesetting time:
/* * In the long run it maybe a good idea to make one set of * optionals per connector type. */
Any functional or user visible changes in the series are bugs.
BR, Jani.
Jani Nikula (4): drm/sysfs: add a helper for extracting connector type from kobject drm/sysfs: make optional attribute groups per connector type drm/sysfs: split DVI-I and TV-out attributes drm/sysfs: remove unnecessary connector type checks
drivers/gpu/drm/drm_sysfs.c | 160 +++++++++++++++++++++++++++----------------- 1 file changed, 98 insertions(+), 62 deletions(-)
-- 2.1.4
dri-devel@lists.freedesktop.org