On Wed, 25 Feb 2015 10:49:58 +0200 Tomi Valkeinen <tomi.valkeinen@ti.com>
wrote:

> Hi,
> 
> On 24/02/15 22:31, NeilBrown wrote:
> > On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <tomi.valkeinen@ti.com>
> > wrote:
> > 
> >> Hi,
> >>
> >> On 24/02/15 11:37, NeilBrown wrote:
> >>>
> >>>
> >>> commit 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> >>>     OMAPDSS: rename display-sysfs 'name' entry
> >>>
> >>> broke the xorg X server on my device as it couldn't find the display
> >>> any more.  It needs the 'name' file and now there isn't one.
> >>>
> >>> That commit claims that 'name' is not compatible with i2c or spi.
> >>> i2c does register it own 'name' file, but spi does not, hence my
> >>> problem - I have an spi display.
> >>>
> >>> So create a special case for i2c: add the name attribute for non-i2c
> >>> devices.
> 
> How about this patch instead. It separates the underlying device's sysfs directory 
> from the "displayX" directory, and allows us to have name & display_name. So it
> should work for any device type.
> 
> 
> From 8e411fa684d42fca35628b41837c6d72e87aaff0 Mon Sep 17 00:00:00 2001
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Wed, 25 Feb 2015 10:23:58 +0200
> Subject: [PATCH] OMAPDSS: fix regression with display sysfs files
> 
> omapdss's sysfs directories for displays used to have 'name' file,
> giving the name for the display. This file was later renamed to
> 'display_name' to avoid conflicts with i2c sysfs 'name' file. Looks like
> at least xserver-xorg-video-omap3 requires the 'name' file to be
> present.
> 
> To fix the regression, this patch creates new kobjects for each display,
> allowing us to create sysfs directories for the displays. This way we
> have the whole directory for omapdss, and there will be no sysfs file
> clashes with the underlying display device's sysfs files.
> 
> We can thus add the 'name' sysfs file back.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/fbdev/omap2/dss/display-sysfs.c | 179 ++++++++++++++------------
>  include/video/omapdss.h                       |   1 +
>  2 files changed, 96 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> index 5a2095a98ed8..12186557a9d4 100644
> --- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
> +++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> @@ -28,44 +28,22 @@
>  #include <video/omapdss.h>
>  #include "dss.h"
>  
> -static struct omap_dss_device *to_dss_device_sysfs(struct device *dev)
> +static ssize_t display_name_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = NULL;
> -
> -	for_each_dss_dev(dssdev) {
> -		if (dssdev->dev == dev) {
> -			omap_dss_put_device(dssdev);
> -			return dssdev;
> -		}
> -	}
> -
> -	return NULL;
> -}
> -
> -static ssize_t display_name_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> -{
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> -
>  	return snprintf(buf, PAGE_SIZE, "%s\n",
>  			dssdev->name ?
>  			dssdev->name : "");
>  }
>  
> -static ssize_t display_enabled_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_enabled_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
> -
>  	return snprintf(buf, PAGE_SIZE, "%d\n",
>  			omapdss_device_is_enabled(dssdev));
>  }
>  
> -static ssize_t display_enabled_store(struct device *dev,
> -		struct device_attribute *attr,
> +static ssize_t display_enabled_store(struct omap_dss_device *dssdev,
>  		const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int r;
>  	bool enable;
>  
> @@ -90,19 +68,16 @@ static ssize_t display_enabled_store(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t display_tear_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_tear_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	return snprintf(buf, PAGE_SIZE, "%d\n",
>  			dssdev->driver->get_te ?
>  			dssdev->driver->get_te(dssdev) : 0);
>  }
>  
> -static ssize_t display_tear_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_tear_store(struct omap_dss_device *dssdev,
> +	const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int r;
>  	bool te;
>  
> @@ -120,10 +95,8 @@ static ssize_t display_tear_store(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t display_timings_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_timings_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	struct omap_video_timings t;
>  
>  	if (!dssdev->driver->get_timings)
> @@ -137,10 +110,9 @@ static ssize_t display_timings_show(struct device *dev,
>  			t.y_res, t.vfp, t.vbp, t.vsw);
>  }
>  
> -static ssize_t display_timings_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_timings_store(struct omap_dss_device *dssdev,
> +	const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	struct omap_video_timings t = dssdev->panel.timings;
>  	int r, found;
>  
> @@ -176,10 +148,8 @@ static ssize_t display_timings_store(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t display_rotate_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_rotate_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int rotate;
>  	if (!dssdev->driver->get_rotate)
>  		return -ENOENT;
> @@ -187,10 +157,9 @@ static ssize_t display_rotate_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%u\n", rotate);
>  }
>  
> -static ssize_t display_rotate_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_rotate_store(struct omap_dss_device *dssdev,
> +	const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int rot, r;
>  
>  	if (!dssdev->driver->set_rotate || !dssdev->driver->get_rotate)
> @@ -207,10 +176,8 @@ static ssize_t display_rotate_store(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t display_mirror_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_mirror_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int mirror;
>  	if (!dssdev->driver->get_mirror)
>  		return -ENOENT;
> @@ -218,10 +185,9 @@ static ssize_t display_mirror_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%u\n", mirror);
>  }
>  
> -static ssize_t display_mirror_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_mirror_store(struct omap_dss_device *dssdev,
> +	const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	int r;
>  	bool mirror;
>  
> @@ -239,10 +205,8 @@ static ssize_t display_mirror_store(struct device *dev,
>  	return size;
>  }
>  
> -static ssize_t display_wss_show(struct device *dev,
> -		struct device_attribute *attr, char *buf)
> +static ssize_t display_wss_show(struct omap_dss_device *dssdev, char *buf)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	unsigned int wss;
>  
>  	if (!dssdev->driver->get_wss)
> @@ -253,10 +217,9 @@ static ssize_t display_wss_show(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "0x%05x\n", wss);
>  }
>  
> -static ssize_t display_wss_store(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t size)
> +static ssize_t display_wss_store(struct omap_dss_device *dssdev,
> +	const char *buf, size_t size)
>  {
> -	struct omap_dss_device *dssdev = to_dss_device_sysfs(dev);
>  	u32 wss;
>  	int r;
>  
> @@ -277,50 +240,94 @@ static ssize_t display_wss_store(struct device *dev,
>  	return size;
>  }
>  
> -static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> -static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
> +struct display_attribute {
> +	struct attribute attr;
> +	ssize_t (*show)(struct omap_dss_device *, char *);
> +	ssize_t	(*store)(struct omap_dss_device *, const char *, size_t);
> +};
> +
> +#define DISPLAY_ATTR(_name, _mode, _show, _store) \
> +	struct display_attribute display_attr_##_name = \
> +	__ATTR(_name, _mode, _show, _store)
> +
> +static DISPLAY_ATTR(name, S_IRUGO, display_name_show, NULL);
> +static DISPLAY_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> +static DISPLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
>  		display_enabled_show, display_enabled_store);
> -static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(tear_elim, S_IRUGO|S_IWUSR,
>  		display_tear_show, display_tear_store);
> -static DEVICE_ATTR(timings, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(timings, S_IRUGO|S_IWUSR,
>  		display_timings_show, display_timings_store);
> -static DEVICE_ATTR(rotate, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(rotate, S_IRUGO|S_IWUSR,
>  		display_rotate_show, display_rotate_store);
> -static DEVICE_ATTR(mirror, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(mirror, S_IRUGO|S_IWUSR,
>  		display_mirror_show, display_mirror_store);
> -static DEVICE_ATTR(wss, S_IRUGO|S_IWUSR,
> +static DISPLAY_ATTR(wss, S_IRUGO|S_IWUSR,
>  		display_wss_show, display_wss_store);
>  
> -static const struct attribute *display_sysfs_attrs[] = {
> -	&dev_attr_display_name.attr,
> -	&dev_attr_enabled.attr,
> -	&dev_attr_tear_elim.attr,
> -	&dev_attr_timings.attr,
> -	&dev_attr_rotate.attr,
> -	&dev_attr_mirror.attr,
> -	&dev_attr_wss.attr,
> +static struct attribute *display_sysfs_attrs[] = {
> +	&display_attr_name.attr,
> +	&display_attr_display_name.attr,
> +	&display_attr_enabled.attr,
> +	&display_attr_tear_elim.attr,
> +	&display_attr_timings.attr,
> +	&display_attr_rotate.attr,
> +	&display_attr_mirror.attr,
> +	&display_attr_wss.attr,
>  	NULL
>  };
>  
> +static ssize_t display_attr_show(struct kobject *kobj, struct attribute *attr,
> +		char *buf)
> +{
> +	struct omap_dss_device *dssdev;
> +	struct display_attribute *display_attr;
> +
> +	dssdev = container_of(kobj, struct omap_dss_device, kobj);
> +	display_attr = container_of(attr, struct display_attribute, attr);
> +
> +	if (!display_attr->show)
> +		return -ENOENT;
> +
> +	return display_attr->show(dssdev, buf);
> +}
> +
> +static ssize_t display_attr_store(struct kobject *kobj, struct attribute *attr,
> +		const char *buf, size_t size)
> +{
> +	struct omap_dss_device *dssdev;
> +	struct display_attribute *display_attr;
> +
> +	dssdev = container_of(kobj, struct omap_dss_device, kobj);
> +	display_attr = container_of(attr, struct display_attribute, attr);
> +
> +	if (!display_attr->store)
> +		return -ENOENT;
> +
> +	return display_attr->store(dssdev, buf, size);
> +}
> +
> +static const struct sysfs_ops display_sysfs_ops = {
> +	.show = display_attr_show,
> +	.store = display_attr_store,
> +};
> +
> +static struct kobj_type display_ktype = {
> +	.sysfs_ops = &display_sysfs_ops,
> +	.default_attrs = display_sysfs_attrs,
> +};
> +
>  int display_init_sysfs(struct platform_device *pdev)
>  {
>  	struct omap_dss_device *dssdev = NULL;
>  	int r;
>  
>  	for_each_dss_dev(dssdev) {
> -		struct kobject *kobj = &dssdev->dev->kobj;
> -
> -		r = sysfs_create_files(kobj, display_sysfs_attrs);
> +		r = kobject_init_and_add(&dssdev->kobj, &display_ktype,
> +			&pdev->dev.kobj, dssdev->alias);
>  		if (r) {
>  			DSSERR("failed to create sysfs files\n");
> -			goto err;
> -		}
> -
> -		r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
> -		if (r) {
> -			sysfs_remove_files(kobj, display_sysfs_attrs);
> -
> -			DSSERR("failed to create sysfs display link\n");
> +			omap_dss_put_device(dssdev);
>  			goto err;
>  		}
>  	}
> @@ -338,8 +345,12 @@ void display_uninit_sysfs(struct platform_device *pdev)
>  	struct omap_dss_device *dssdev = NULL;
>  
>  	for_each_dss_dev(dssdev) {
> -		sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
> -		sysfs_remove_files(&dssdev->dev->kobj,
> -				display_sysfs_attrs);
> +		if (kobject_name(&dssdev->kobj) == NULL)
> +			continue;
> +
> +		kobject_del(&dssdev->kobj);
> +		kobject_put(&dssdev->kobj);
> +
> +		memset(&dssdev->kobj, 0, sizeof(dssdev->kobj));
>  	}
>  }
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index 60de61fea8e3..c8ed15daad02 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -689,6 +689,7 @@ struct omapdss_dsi_ops {
>  };
>  
>  struct omap_dss_device {
> +	struct kobject kobj;
>  	struct device *dev;
>  
>  	struct module *owner;


Tested-by: NeilBrown <neilb@suse.de>

Before the patch:

# ls -l /sys/devices/platform/omapdss/display0 
lrwxrwxrwx 1 root root 0 Feb  8 12:57 /sys/devices/platform/omapdss/display0 -> ../spi_lcd/spi_master/spi32766/spi32766.0

After the patch:

# ls -l /sys/devices/platform/omapdss/display0 
total 0
-r--r--r-- 1 root root 4096 Feb  8 13:37 display_name
-rw-r--r-- 1 root root 4096 Feb  8 13:37 enabled
-rw-r--r-- 1 root root 4096 Feb  8 13:37 mirror
-r--r--r-- 1 root root 4096 Feb  8 13:37 name
-rw-r--r-- 1 root root 4096 Feb  8 13:37 rotate
-rw-r--r-- 1 root root 4096 Feb  8 13:37 tear_elim
-rw-r--r-- 1 root root 4096 Feb  8 13:37 timings
-rw-r--r-- 1 root root 4096 Feb  8 13:37 wss


So as you say it creates a directory just for the display0 device, and that
has the 'name' that we want.

This works for me, and it seems to me to be a better fit to the general
structure of /sys/devices - symlinks within /sys/devices are a substantial
minority, other than 'subsystem', 'device', 'driver' and 'bdi' which have
very generic meanings.

I guess I'm a little surprised that there doesn't seem to be any linkage from
the display0 to the spi device.  Maybe that isn't important.

Thanks a lot!

NeilBrown