video: fbdev: omap2: omapfb: remove __exit annotation

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

Signed-off-by Felipe Balbi <[email protected]>

Quoted 4 lines · Felipe Balbi

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

Signed-off-by Felipe Balbi <[email protected]>

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.

Quoted 10 lines · Felipe Balbi

Quoted 4 lines · Felipe Balbi

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

Signed-off-by Felipe Balbi <[email protected]>

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.

Quoted 1 line

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"?

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

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +0 -0

index 9cbf1ce..b4b9244 100644

Quoted 14 lines · Felipe Balbi

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

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +0 -0

index 9cbf1ce..b4b9244 100644

drivers/video/fbdev/omap2/omapfb/omapfb-main.c module_param_named(mirror, def_mirror, bool, 0); · line 2657

Quoted 7 lines

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -0

@@ -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 <[email protected]>");
 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.

Quoted 34 lines · Tomi Valkeinen

Quoted 14 lines · Felipe Balbi

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

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 1 +
 1 file changed, 1 insertion(+)

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +0 -0

index 9cbf1ce..b4b9244 100644

drivers/video/fbdev/omap2/omapfb/omapfb-main.c module_param_named(mirror, def_mirror, bool, 0); · line 2657

Quoted 7 lines

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -0

@@ -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 <[email protected]>");
 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.

Quoted 2 lines

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 ? :-)

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 <[email protected]>

---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/arm/boot/dts/am437x-sk-evm.dts +0 -0

index 859ff3d..681be00 100644

Quoted 18 lines · Felipe Balbi

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 <[email protected]>

---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/arm/boot/dts/am437x-sk-evm.dts +0 -0

index 859ff3d..681be00 100644

arch/arm/boot/dts/am437x-sk-evm.dts line 326

Quoted 9 lines

arch/arm/boot/dts/am437x-sk-evm.dts +1 -2

@@ -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 <[email protected]>

HI,

Quoted 31 lines · Tomi Valkeinen

Quoted 18 lines · Felipe Balbi

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 <[email protected]>

---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/arm/boot/dts/am437x-sk-evm.dts +0 -0

index 859ff3d..681be00 100644

arch/arm/boot/dts/am437x-sk-evm.dts line 326

Quoted 9 lines

arch/arm/boot/dts/am437x-sk-evm.dts +1 -2

@@ -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.

Quoted 1 line

Acked-by Tomi Valkeinen <[email protected]>

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).

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.

While at that, also replace a pullup with a pulldown as that gpio should be normally low, not high.

Cc <[email protected]> # v3.17+

Acked-by Tomi Valkeinen <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

---
 arch/arm/boot/dts/am437x-sk-evm.dts | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

arch/arm/boot/dts/am437x-sk-evm.dts +1 -2

index f7f77a7..0eceb8a 100644
@@ -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_PULLDOWN | MUX_MODE7) /* gpcm_ad7.gpio1_7 */
 		>;
 	};
 };

Signature · 1 line

2.1.0.GIT

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

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -1

index ec2d132..9cbf1ce 100644
@@ -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);
 

Hi,

Quoted 22 lines · Felipe Balbi

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

Signed-off-by Felipe Balbi <[email protected]>

---
 drivers/video/fbdev/omap2/omapfb/omapfb-main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -1

index ec2d132..9cbf1ce 100644
@@ -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);
 

drivers/video/fbdev/omap2/omapfb/omapfb-main.c static int __exit omapfb_remove(struct platform_device *pdev) · line 2642

Quoted 8 lines

drivers/video/fbdev/omap2/omapfb/omapfb-main.c +1 -1

@@ -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.

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.

Quoted 4 lines

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()

Quoted 6 lines

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).

Quoted 2 lines

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 4 lines

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.

Quoted 1 line

So there seems to be more issues around this.

quite a few more, I'd say

Quoted 11 lines

Quoted 6 lines

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 10 lines

Quoted 2 lines

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 4 lines

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).

Quoted 1 line

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 3 lines

Quoted 1 line

So there seems to be more issues around this.

quite a few more, I'd say

Yep, I'll have a look at this.