Re: [PATCH] OMAPDSS: restore "name" sysfs entry.

From: NeilBrown
Date: Wed Feb 25 2015 - 04:21:09 EST

Next message: Heiko Carstens: "Re: [PATCH] dcssblk.c : Array index 'i' is used before limits check."
Previous message: Ingo Molnar: "Re: [PATCH 1/4] x86: entry.S: tidy up several suboptimal insns"
In reply to: Tomi Valkeinen: "Re: [PATCH] OMAPDSS: restore "name" sysfs entry."
Next in thread: Tomi Valkeinen: "Re: [PATCH] OMAPDSS: restore "name" sysfs entry."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

> Hi,
>
> On 24/02/15 22:31, NeilBrown wrote:
> > On Tue, 24 Feb 2015 12:40:32 +0200 Tomi Valkeinen <tomi.valkeinen@xxxxxx>
> > 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@xxxxxx>
> 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@xxxxxx>
> ---
> 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@xxxxxxx>

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

Attachment: pgp2mpf8cXd4U.pgp
Description: OpenPGP digital signature

Next message: Heiko Carstens: "Re: [PATCH] dcssblk.c : Array index 'i' is used before limits check."
Previous message: Ingo Molnar: "Re: [PATCH 1/4] x86: entry.S: tidy up several suboptimal insns"
In reply to: Tomi Valkeinen: "Re: [PATCH] OMAPDSS: restore "name" sysfs entry."
Next in thread: Tomi Valkeinen: "Re: [PATCH] OMAPDSS: restore "name" sysfs entry."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]