diff mbox

OMAPDSS: restore "name" sysfs entry.

Message ID 20150224203706.1eafa129@notabene.brown (mailing list archive)
State New, archived
Headers show

Commit Message

NeilBrown Feb. 24, 2015, 9:37 a.m. UTC 
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.

Fixes: 303e4697e762dc92a40405f4e4b8aac02cd0d70b
Signed-off-by: NeilBrown <neilb@suse.de>

--

Hi Tomi,
 I wonder if you would consider this for the next merge window (or maybe even sooner as a bug-fix).
I haven't tested it with an i2c display, but it certainly allows xorg to work
on my spi-attached display.

Thanks,
NeilBrown

Comments

Tomi Valkeinen Feb. 24, 2015, 10:40 a.m. UTC | #1 
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.

What X driver is that? What's it doing with the display name? Is it just
using the display name to show something for the user, and the returned
value can be essentially any string?

 Tomi
H. Nikolaus Schaller Feb. 24, 2015, 5:08 p.m. UTC | #2 
I have tested it with

X.Org X Server 1.12.4
Release Date: 2012-08-27
X Protocol Version 11, Revision 0
Build Operating System: Linux 3.2.0-4-mx5 armv7l Debian

(coming from Debian Wheezy) on:

* gta04 (SPI panel)
* openpandora (SPI panel)
* BeagleBoard XM (with RGB panel)
* PandaBoard ES (w/o panel)
* omap5432-evm + Pyra prototype (MIPI panel)

Without this patch, X Server simply fails with:

Fatal server error:
no screens found

So please add
Tested-by: Nikolaus Schaller <hns@goldelico.com>

Thanks,
Nikolaus


Am 24.02.2015 um 10:37 schrieb NeilBrown <neilb@suse.de>:

> 
> 
> 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.
> 
> Fixes: 303e4697e762dc92a40405f4e4b8aac02cd0d70b
> Signed-off-by: NeilBrown <neilb@suse.de>
> 
> --
> 
> Hi Tomi,
> I wonder if you would consider this for the next merge window (or maybe even sooner as a bug-fix).
> I haven't tested it with an i2c display, but it certainly allows xorg to work
> on my spi-attached display.
> 
> Thanks,
> NeilBrown
> 
> 
> diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> index 5a2095a98ed8..53897b743130 100644
> --- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
> +++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
> @@ -25,6 +25,15 @@
> #include <linux/platform_device.h>
> #include <linux/sysfs.h>
> 
> +#if IS_ENABLED(CONFIG_I2C)
> +#include <linux/i2c.h>
> +#else
> +static inline int i2c_verify_client(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
> #include <video/omapdss.h>
> #include "dss.h"
> 
> @@ -278,6 +287,7 @@ static ssize_t display_wss_store(struct device *dev,
> }
> 
> static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
> +static DEVICE_ATTR(name, S_IRUGO, display_name_show, NULL);
> static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
> 		display_enabled_show, display_enabled_store);
> static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
> @@ -315,6 +325,17 @@ int display_init_sysfs(struct platform_device *pdev)
> 			DSSERR("failed to create sysfs files\n");
> 			goto err;
> 		}
> +		if (!i2c_verify_client(dssdev->dev)) {
> +			/*
> +			 * Add 'name' file - not compatible with i2c, but
> +			 * need by Xorg for other devices.
> +			 */
> +			r = sysfs_create_file(kobj, &dev_attr_name.attr);
> +			if (r) {
> +				DSSERR("fail to create 'name' sysfs file\n");
> +				goto err;
> +			}
> +		}
> 
> 		r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
> 		if (r) {
> @@ -341,5 +362,7 @@ void display_uninit_sysfs(struct platform_device *pdev)
> 		sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
> 		sysfs_remove_files(&dssdev->dev->kobj,
> 				display_sysfs_attrs);
> +		sysfs_remove_file(&dssdev->dev->kobj,
> +				  &dev_attr_name.attr);
> 	}
> }

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
NeilBrown Feb. 24, 2015, 8:31 p.m. UTC | #3 
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.
> 
> What X driver is that? What's it doing with the display name? Is it just
> using the display name to show something for the user, and the returned
> value can be essentially any string?
> 
>  Tomi
> 
> 
/usr/lib/xorg/modules/drivers/omapfb_drv.so
from package xserver-xorg-video-omap3 in Debian.

I don't know where the main upstream source is, but here:

https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258

is the code which reads
   /sys/devices/platform/omapdss/display0/name
and fails if that file cannot be opened.

Thanks,
NeilBrown
Tony Lindgren Feb. 24, 2015, 8:48 p.m. UTC | #4 
* NeilBrown <neilb@suse.de> [150224 12:35]:
> 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.
> > 
> > What X driver is that? What's it doing with the display name? Is it just
> > using the display name to show something for the user, and the returned
> > value can be essentially any string?
> > 
> >  Tomi
> > 
> > 
> /usr/lib/xorg/modules/drivers/omapfb_drv.so
> from package xserver-xorg-video-omap3 in Debian.
> 
> I don't know where the main upstream source is, but here:
> 
> https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258
> 
> is the code which reads
>    /sys/devices/platform/omapdss/display0/name
> and fails if that file cannot be opened.

Sounds like a kernel interface we need to support forever
and fix the regression.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen Feb. 25, 2015, 7:04 a.m. UTC | #5 
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.
>>
>> What X driver is that? What's it doing with the display name? Is it just
>> using the display name to show something for the user, and the returned
>> value can be essentially any string?
>>
>>  Tomi
>>
>>
> /usr/lib/xorg/modules/drivers/omapfb_drv.so
> from package xserver-xorg-video-omap3 in Debian.
> 
> I don't know where the main upstream source is, but here:
> 
> https://gitorious.org/gnutoo-s-programs-for-shr/xf86-video-omapfb/source/28c006c94e57ea71df11ec4fff79d7ffcfc4860f:src/omapfb-output-dss.c#L258
> 
> is the code which reads
>    /sys/devices/platform/omapdss/display0/name
> and fails if that file cannot be opened.

Thanks. Unfortunately it looks to me that the omapfb_drv uses the
display name to configure things, and as in i2c's case the 'name' is not
the correct dss name, X will probably fail in interesting ways.

Of course, it's already broken in that way, so this fix improves the
situation for non-i2c displays. I'll have a look if I can figure out how
to fix this for all displays.

 Tomi
diff mbox

Patch

diff --git a/drivers/video/fbdev/omap2/dss/display-sysfs.c b/drivers/video/fbdev/omap2/dss/display-sysfs.c
index 5a2095a98ed8..53897b743130 100644
--- a/drivers/video/fbdev/omap2/dss/display-sysfs.c
+++ b/drivers/video/fbdev/omap2/dss/display-sysfs.c
@@ -25,6 +25,15 @@ 
 #include <linux/platform_device.h>
 #include <linux/sysfs.h>
 
+#if IS_ENABLED(CONFIG_I2C)
+#include <linux/i2c.h>
+#else
+static inline int i2c_verify_client(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 #include <video/omapdss.h>
 #include "dss.h"
 
@@ -278,6 +287,7 @@  static ssize_t display_wss_store(struct device *dev,
 }
 
 static DEVICE_ATTR(display_name, S_IRUGO, display_name_show, NULL);
+static DEVICE_ATTR(name, S_IRUGO, display_name_show, NULL);
 static DEVICE_ATTR(enabled, S_IRUGO|S_IWUSR,
 		display_enabled_show, display_enabled_store);
 static DEVICE_ATTR(tear_elim, S_IRUGO|S_IWUSR,
@@ -315,6 +325,17 @@  int display_init_sysfs(struct platform_device *pdev)
 			DSSERR("failed to create sysfs files\n");
 			goto err;
 		}
+		if (!i2c_verify_client(dssdev->dev)) {
+			/*
+			 * Add 'name' file - not compatible with i2c, but
+			 * need by Xorg for other devices.
+			 */
+			r = sysfs_create_file(kobj, &dev_attr_name.attr);
+			if (r) {
+				DSSERR("fail to create 'name' sysfs file\n");
+				goto err;
+			}
+		}
 
 		r = sysfs_create_link(&pdev->dev.kobj, kobj, dssdev->alias);
 		if (r) {
@@ -341,5 +362,7 @@  void display_uninit_sysfs(struct platform_device *pdev)
 		sysfs_remove_link(&pdev->dev.kobj, dssdev->alias);
 		sysfs_remove_files(&dssdev->dev->kobj,
 				display_sysfs_attrs);
+		sysfs_remove_file(&dssdev->dev->kobj,
+				  &dev_attr_name.attr);
 	}
 }