video: fbdev: omap2: omapfb: remove __exit annotation

if we leave __exit annotation, driver can't be unbound
through sysfs.

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index ec2d132..9cbf1ce 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c

@@ -2619,7 +2619,7 @@ err0:
 	return r;
 }
 
-static int __exit omapfb_remove(struct platform_device *pdev)
+static int omapfb_remove(struct platform_device *pdev)
 {
 	struct omapfb2_device *fbdev = platform_get_drvdata(pdev);
 

@@ -2636,7 +2636,7 @@ static int __exit omapfb_remove(struct platform_device *pdev)
 
 static struct platform_driver omapfb_driver = {
 	.probe		= omapfb_probe,
-	.remove         = __exit_p(omapfb_remove),
+	.remove         = omapfb_remove,
 	.driver         = {
 		.name   = "omapfb",
 		.owner  = THIS_MODULE,

-- 
2.1.0.GIT

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index 9cbf1ce..b4b9244 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c

@@ -2651,6 +2651,7 @@ module_param_named(mirror, def_mirror, bool, 0);
 
 module_platform_driver(omapfb_driver);
 
+MODULE_ALIAS("platform:omapfb");
 MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@nokia.com>");
 MODULE_DESCRIPTION("OMAP2/3 Framebuffer");
 MODULE_LICENSE("GPL v2");

-- 
2.1.0.GIT

Caused by a copy & paste error. Note that even with
this bug AM437x SK display still works because GPIO
mux mode is always enabled. It's still wrong to mux
somebody else's pin.

Luckily ball D25 (offset 0x238 - gpio5_8) on AM437x
isn't used for anything.

Signed-off-by: Felipe Balbi <redacted>
---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/am437x-sk-evm.dts b/arch/arm/boot/dts/am437x-sk-evm.dts
index 859ff3d..681be00 100644
--- a/arch/arm/boot/dts/am437x-sk-evm.dts
+++ b/arch/arm/boot/dts/am437x-sk-evm.dts

@@ -320,8 +320,7 @@
 
 	lcd_pins: lcd_pins {
 		pinctrl-single,pins = <
-			/* GPIO 5_8 to select LCD / HDMI */
-			0x238 (PIN_OUTPUT_PULLUP | MUX_MODE7)
+			0x1c (PIN_OUTPUT_PULLUP | MUX_MODE7) /* gpcm_ad7.gpio1_7 */
 		>;
 	};
 };

-- 
2.1.0.GIT

On Tue, Oct 14, 2014 at 01:28:54PM -0500, Felipe Balbi wrote:

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>

little note here. This makes omapfb load automatically, but display
still doesn't work with DSS as modules. Backlight is working and pixel
clock is running just fine. Still, nothing on display.

I thought I'd leave that for Tomi to deal with.

-- 
balbi

Hi,

On 14/10/14 21:28, Felipe Balbi wrote:

Interesting. I don't know if I'm doing something funny, but without this
patch, I can unbind omapfb, kind of.

"echo omapfb > unbind" goes ok, but remove is obviously not called.
Somehow omapfb device is still unbound from the driver, as I can then
bind it again, causing probe to be called. Which breaks everything.

I would've thought that unbinding is not possible if remove is missing,
but that doesn't seem to be the case. I guess it just means that remove
is not called when the driver & device are unbound.

We have 18 __exit_p()s in omapdss and related drivers. I guess they are
all broken the same way.

Note that omapfb unbind & bind does not work even with this patch, but
results in a crash as some old state is left into omapdss. The same
happens also with unloading and loading omapfb module (but keeping
omapdss module loaded).

So there seems to be more issues around this.

 Tomi

On 14/10/14 21:28, Felipe Balbi wrote:

Thanks, I've queued this.

Unfortunately it's somewhat based on luck if the automatic loading works
correctly. We can't add more displays after omapfb has been probed, so
all the panel and encoder drivers have to be loaded before omapfb.

We have two workarounds there, which help the situation a bit. First is
that if omapfb finds no displays, it returns EPROBE_DEFER. The second is
that if there are displays, but no main display (display0), then omapfb
returns EPROBE_DEFER.

So even with these workarounds it is possible that drivers for secondary
displays are loaded after omapfb, causing them to be ignored.

 Tomi

On 14/10/14 21:28, Felipe Balbi wrote:

I didn't verify the offset, but based on the comments this looks fine to me.

Acked-by: Tomi Valkeinen <redacted>

 Tomi

On 14/10/14 21:34, Felipe Balbi wrote:

On Tue, Oct 14, 2014 at 01:28:54PM -0500, Felipe Balbi wrote:

quoted

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>

little note here. This makes omapfb load automatically, but display
still doesn't work with DSS as modules. Backlight is working and pixel
clock is running just fine. Still, nothing on display.

I thought I'd leave that for Tomi to deal with.

I presume this is about am437x-sk. I don't have the board, so there's
not too much I can do except send emails.

So the panel works fine if display drivers are built-in?

The compatible string seems to be wrong in the .dts file. It says
"osddisplays,osd057T0559-34ts" which is not the panel used. But that
shouldn't affect this.

What do you mean with "pixel clock is running"?

 Tomi

HI,

On Wed, Oct 15, 2014 at 03:46:10PM +0300, Tomi Valkeinen wrote:

On 14/10/14 21:34, Felipe Balbi wrote:

quoted

On Tue, Oct 14, 2014 at 01:28:54PM -0500, Felipe Balbi wrote:

quoted

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>

little note here. This makes omapfb load automatically, but display
still doesn't work with DSS as modules. Backlight is working and pixel
clock is running just fine. Still, nothing on display.

I thought I'd leave that for Tomi to deal with.

I presume this is about am437x-sk. I don't have the board, so there's
not too much I can do except send emails.

So the panel works fine if display drivers are built-in?

yup.

The compatible string seems to be wrong in the .dts file. It says
"osddisplays,osd057T0559-34ts" which is not the panel used. But that
shouldn't affect this.

I got that from Darren, IIRC. Still, if it's wrong, it's wrong. What
should it be ?

What do you mean with "pixel clock is running"?

poking at the pixel clock with a scope, I can see it clocking.

-- 
balbi

On Wed, Oct 15, 2014 at 03:20:19PM +0300, Tomi Valkeinen wrote:

On 14/10/14 21:28, Felipe Balbi wrote:

quoted

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index 9cbf1ce..b4b9244 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c

@@ -2651,6 +2651,7 @@ module_param_named(mirror, def_mirror, bool, 0);
 
 module_platform_driver(omapfb_driver);
 
+MODULE_ALIAS("platform:omapfb");
 MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@nokia.com>");
 MODULE_DESCRIPTION("OMAP2/3 Framebuffer");
 MODULE_LICENSE("GPL v2");

Thanks, I've queued this.

Unfortunately it's somewhat based on luck if the automatic loading works
correctly. We can't add more displays after omapfb has been probed, so
all the panel and encoder drivers have to be loaded before omapfb.

We have two workarounds there, which help the situation a bit. First is
that if omapfb finds no displays, it returns EPROBE_DEFER. The second is
that if there are displays, but no main display (display0), then omapfb
returns EPROBE_DEFER.

So even with these workarounds it is possible that drivers for secondary
displays are loaded after omapfb, causing them to be ignored.

then there is another case to fix, right ? :-)

-- 
balbi

Hi,

On Wed, Oct 15, 2014 at 03:13:34PM +0300, Tomi Valkeinen wrote:

Hi,

On 14/10/14 21:28, Felipe Balbi wrote:

quoted

if we leave __exit annotation, driver can't be unbound
through sysfs.

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index ec2d132..9cbf1ce 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c

@@ -2619,7 +2619,7 @@ err0:
 	return r;
 }
 
-static int __exit omapfb_remove(struct platform_device *pdev)
+static int omapfb_remove(struct platform_device *pdev)
 {
 	struct omapfb2_device *fbdev = platform_get_drvdata(pdev);
 

@@ -2636,7 +2636,7 @@ static int __exit omapfb_remove(struct platform_device *pdev)
 
 static struct platform_driver omapfb_driver = {
 	.probe		= omapfb_probe,
-	.remove         = __exit_p(omapfb_remove),
+	.remove         = omapfb_remove,
 	.driver         = {
 		.name   = "omapfb",
 		.owner  = THIS_MODULE,

Interesting. I don't know if I'm doing something funny, but without this
patch, I can unbind omapfb, kind of.

"echo omapfb > unbind" goes ok, but remove is obviously not called.

remove isn't called because it won't exist if it's built-in. Look at the
definition of __exit_p()

Somehow omapfb device is still unbound from the driver, as I can then
bind it again, causing probe to be called. Which breaks everything.

I would've thought that unbinding is not possible if remove is missing,
but that doesn't seem to be the case. I guess it just means that remove
is not called when the driver & device are unbound.

if no remove it provided on platform_driver structure, platform bus
assumes you have nothing to do on your ->remove(), so you end up leaking
all resources you allocated on ->probe() (unless you *really* don't need
to do anything on ->remove).

We have 18 __exit_p()s in omapdss and related drivers. I guess they are
all broken the same way.

yup, I should've grepped.

Note that omapfb unbind & bind does not work even with this patch, but
results in a crash as some old state is left into omapdss. The same
happens also with unloading and loading omapfb module (but keeping
omapdss module loaded).

It worked fine for me. I unbound and bound omapfb multiple times.

So there seems to be more issues around this.

quite a few more, I'd say

-- 
balbi

On 15/10/14 17:41, Felipe Balbi wrote:

quoted

Interesting. I don't know if I'm doing something funny, but without this
patch, I can unbind omapfb, kind of.

"echo omapfb > unbind" goes ok, but remove is obviously not called.

remove isn't called because it won't exist if it's built-in. Look at the
definition of __exit_p()

Yes, that's what I meant with "obviously".

quoted

Somehow omapfb device is still unbound from the driver, as I can then
bind it again, causing probe to be called. Which breaks everything.

I would've thought that unbinding is not possible if remove is missing,
but that doesn't seem to be the case. I guess it just means that remove
is not called when the driver & device are unbound.

if no remove it provided on platform_driver structure, platform bus
assumes you have nothing to do on your ->remove(), so you end up leaking
all resources you allocated on ->probe() (unless you *really* don't need
to do anything on ->remove).

Yep. That's quite odd, still. grep shows quite many uses of __exit_p(),
and all for remove callback. So, if you have something to release in
remove(), you should set it always, for both module and built-in. And if
you don't have anything to release, you would always just set .release
to NULL.

I mean, what's the use case for __exit_p()? With a quick glance, at
least some of the other users also use __exit_p() the same way omapdss
does (i.e. in the wrong way).

quoted

We have 18 __exit_p()s in omapdss and related drivers. I guess they are
all broken the same way.

yup, I should've grepped.

quoted

Note that omapfb unbind & bind does not work even with this patch, but
results in a crash as some old state is left into omapdss. The same
happens also with unloading and loading omapfb module (but keeping
omapdss module loaded).

It worked fine for me. I unbound and bound omapfb multiple times.

Hmm, ok. Odd, the bug was quite clear and I think it should happen every
time. Well, I was using omap4. If you used AM4xx, that's basically omap3
DSS. Maybe there's a diff there.

quoted

So there seems to be more issues around this.

quite a few more, I'd say

Yep, I'll have a look at this.

 Tomi

On 15/10/14 17:38, Felipe Balbi wrote:

On Wed, Oct 15, 2014 at 03:20:19PM +0300, Tomi Valkeinen wrote:

quoted

On 14/10/14 21:28, Felipe Balbi wrote:

quoted

without MODULE_ALIAS(), omapfb won't get loaded
automatically.

Signed-off-by: Felipe Balbi <redacted>
---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
index 9cbf1ce..b4b9244 100644
--- a/drivers/video/fbdev/omap2/omapfb/omapfb-main.c
+++ b/drivers/video/fbdev/omap2/omapfb/omapfb-main.c

@@ -2651,6 +2651,7 @@ module_param_named(mirror, def_mirror, bool, 0);
 
 module_platform_driver(omapfb_driver);
 
+MODULE_ALIAS("platform:omapfb");
 MODULE_AUTHOR("Tomi Valkeinen <tomi.valkeinen@nokia.com>");
 MODULE_DESCRIPTION("OMAP2/3 Framebuffer");
 MODULE_LICENSE("GPL v2");

Thanks, I've queued this.

Unfortunately it's somewhat based on luck if the automatic loading works
correctly. We can't add more displays after omapfb has been probed, so
all the panel and encoder drivers have to be loaded before omapfb.

We have two workarounds there, which help the situation a bit. First is
that if omapfb finds no displays, it returns EPROBE_DEFER. The second is
that if there are displays, but no main display (display0), then omapfb
returns EPROBE_DEFER.

So even with these workarounds it is possible that drivers for secondary
displays are loaded after omapfb, causing them to be ignored.

then there is another case to fix, right ? :-)

Yes, but don't hold your breath. The issue's been there for ages and no
good solutions have been found. The same problem is there for omapdrm.

 Tomi

Hi,

On Wed, Oct 15, 2014 at 06:43:40PM +0300, Tomi Valkeinen wrote:

quoted

quoted

Somehow omapfb device is still unbound from the driver, as I can then
bind it again, causing probe to be called. Which breaks everything.

I would've thought that unbinding is not possible if remove is missing,
but that doesn't seem to be the case. I guess it just means that remove
is not called when the driver & device are unbound.

if no remove it provided on platform_driver structure, platform bus
assumes you have nothing to do on your ->remove(), so you end up leaking
all resources you allocated on ->probe() (unless you *really* don't need
to do anything on ->remove).

Yep. That's quite odd, still. grep shows quite many uses of __exit_p(),
and all for remove callback. So, if you have something to release in
remove(), you should set it always, for both module and built-in. And if
you don't have anything to release, you would always just set .release
to NULL.

I mean, what's the use case for __exit_p()? With a quick glance, at
least some of the other users also use __exit_p() the same way omapdss
does (i.e. in the wrong way).

__exit_p() meant something else a few years back, perhaps those were
left over from some tree-wide cleanups.

quoted

quoted

We have 18 __exit_p()s in omapdss and related drivers. I guess they are
all broken the same way.

yup, I should've grepped.

quoted

Note that omapfb unbind & bind does not work even with this patch, but
results in a crash as some old state is left into omapdss. The same
happens also with unloading and loading omapfb module (but keeping
omapdss module loaded).

It worked fine for me. I unbound and bound omapfb multiple times.

Hmm, ok. Odd, the bug was quite clear and I think it should happen every
time. Well, I was using omap4. If you used AM4xx, that's basically omap3
DSS. Maybe there's a diff there.

could very well be :-)

quoted

quoted

So there seems to be more issues around this.

quite a few more, I'd say

Yep, I'll have a look at this.

alright

-- 
balbi

HI,

On Wed, Oct 15, 2014 at 03:24:20PM +0300, Tomi Valkeinen wrote:

On 14/10/14 21:28, Felipe Balbi wrote:

quoted

Caused by a copy & paste error. Note that even with
this bug AM437x SK display still works because GPIO
mux mode is always enabled. It's still wrong to mux
somebody else's pin.

Luckily ball D25 (offset 0x238 - gpio5_8) on AM437x
isn't used for anything.

Signed-off-by: Felipe Balbi <redacted>
---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/am437x-sk-evm.dts b/arch/arm/boot/dts/am437x-sk-evm.dts
index 859ff3d..681be00 100644
--- a/arch/arm/boot/dts/am437x-sk-evm.dts
+++ b/arch/arm/boot/dts/am437x-sk-evm.dts

@@ -320,8 +320,7 @@
 
 	lcd_pins: lcd_pins {
 		pinctrl-single,pins = <
-			/* GPIO 5_8 to select LCD / HDMI */
-			0x238 (PIN_OUTPUT_PULLUP | MUX_MODE7)
+			0x1c (PIN_OUTPUT_PULLUP | MUX_MODE7) /* gpcm_ad7.gpio1_7 */
 		>;
 	};
 };

I didn't verify the offset, but based on the comments this looks fine to me.

Acked-by: Tomi Valkeinen <redacted>

Tony, are you taking this patch ? Looks like it has been forgotten and
now it needs to be backported to v3.17 and v3.18 (assuming it's too late
for v3.18).

-- 
balbi

On Thu, Dec 04, 2014 at 08:43:25AM -0600, Felipe Balbi wrote:

HI,

On Wed, Oct 15, 2014 at 03:24:20PM +0300, Tomi Valkeinen wrote:

quoted

On 14/10/14 21:28, Felipe Balbi wrote:

quoted

Caused by a copy & paste error. Note that even with
this bug AM437x SK display still works because GPIO
mux mode is always enabled. It's still wrong to mux
somebody else's pin.

Luckily ball D25 (offset 0x238 - gpio5_8) on AM437x
isn't used for anything.

Signed-off-by: Felipe Balbi <redacted>
---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/am437x-sk-evm.dts b/arch/arm/boot/dts/am437x-sk-evm.dts
index 859ff3d..681be00 100644
--- a/arch/arm/boot/dts/am437x-sk-evm.dts
+++ b/arch/arm/boot/dts/am437x-sk-evm.dts

@@ -320,8 +320,7 @@
 
 	lcd_pins: lcd_pins {
 		pinctrl-single,pins = <
-			/* GPIO 5_8 to select LCD / HDMI */
-			0x238 (PIN_OUTPUT_PULLUP | MUX_MODE7)
+			0x1c (PIN_OUTPUT_PULLUP | MUX_MODE7) /* gpcm_ad7.gpio1_7 */
 		>;
 	};
 };

I didn't verify the offset, but based on the comments this looks fine to me.

Acked-by: Tomi Valkeinen <redacted>

Tony, are you taking this patch ? Looks like it has been forgotten and
now it needs to be backported to v3.17 and v3.18 (assuming it's too late
for v3.18).

I'll send a new version and Cc stable.

-- 
balbi