diff mbox

[v2] video: exynos: fix modular build

Message ID 1456490307-823812-1-git-send-email-arnd@arndb.de (mailing list archive)
State New, archived
Headers show

Commit Message

Arnd Bergmann Feb. 26, 2016, 12:38 p.m. UTC 
The s6e8ax0 driver has a dependency on BACKLIGHT_CLASS_DEVICE,
which can be configured as a loadable module, so we have to
make the driver a tristate symbol as well, to avoid this error:

drivers/built-in.o: In function `s6e8ax0_probe':
:(.text+0x23a48): undefined reference to `devm_backlight_device_register'

This also means we get another error from a missing export, which
this fixes as well:

ERROR: "exynos_mipi_dsi_register_lcd_driver" [drivers/video/fbdev/exynos/s6e8ax0.ko] undefined!

The drivers are all written to be loadable modules already,
except the Kconfig options for that are missing, which makes
the patch really easy.

Finally, the EXYNOS_VIDEO option is turned into tristate as well
for good measure, as all framebuffer drivers should be configurable
as modules, though this change is not strictly necessary.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
---
 drivers/video/fbdev/exynos/Kconfig           | 6 +++---
 drivers/video/fbdev/exynos/Makefile          | 6 ++++--
 drivers/video/fbdev/exynos/exynos_mipi_dsi.c | 1 +
 3 files changed, 8 insertions(+), 5 deletions(-)

v2: improved changelog after feedback from Paul Bolle

Comments

Tomi Valkeinen Feb. 29, 2016, 4:12 p.m. UTC | #1 
Hi,

On 26/02/16 14:38, Arnd Bergmann wrote:
> The s6e8ax0 driver has a dependency on BACKLIGHT_CLASS_DEVICE,
> which can be configured as a loadable module, so we have to
> make the driver a tristate symbol as well, to avoid this error:
> 
> drivers/built-in.o: In function `s6e8ax0_probe':
> :(.text+0x23a48): undefined reference to `devm_backlight_device_register'

If a 'bool' Kconfig option depends on BACKLIGHT_CLASS_DEVICE, shouldn't
the Kconfig dependency take care of having BACKLIGHT_CLASS_DEVICE as
built-in?

> This also means we get another error from a missing export, which
> this fixes as well:
> 
> ERROR: "exynos_mipi_dsi_register_lcd_driver" [drivers/video/fbdev/exynos/s6e8ax0.ko] undefined!
> 
> The drivers are all written to be loadable modules already,
> except the Kconfig options for that are missing, which makes
> the patch really easy.

Looks and sound fine, except doesn't this tell that the drivers have
never been tested as modules? Did you or someone else actually test these?

> diff --git a/drivers/video/fbdev/exynos/Makefile b/drivers/video/fbdev/exynos/Makefile
> index b5b1bd228abb..02d8dc522fea 100644
> --- a/drivers/video/fbdev/exynos/Makefile
> +++ b/drivers/video/fbdev/exynos/Makefile
> @@ -2,6 +2,8 @@
>  # Makefile for the exynos video drivers.
>  #
>  
> -obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
> -				     	exynos_mipi_dsi_lowlevel.o
> +obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos-mipi-dsi-mod.o
> +
> +exynos-mipi-dsi-mod-objs		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
> +					   exynos_mipi_dsi_lowlevel.o

Hmm, why is this makefile change needed?

 Tomi
Arnd Bergmann Feb. 29, 2016, 4:39 p.m. UTC | #2 
On Monday 29 February 2016 18:12:45 Tomi Valkeinen wrote:
> Hi,
> 
> On 26/02/16 14:38, Arnd Bergmann wrote:
> > The s6e8ax0 driver has a dependency on BACKLIGHT_CLASS_DEVICE,
> > which can be configured as a loadable module, so we have to
> > make the driver a tristate symbol as well, to avoid this error:
> > 
> > drivers/built-in.o: In function `s6e8ax0_probe':
> > :(.text+0x23a48): undefined reference to `devm_backlight_device_register'
> 
> If a 'bool' Kconfig option depends on BACKLIGHT_CLASS_DEVICE, shouldn't
> the Kconfig dependency take care of having BACKLIGHT_CLASS_DEVICE as
> built-in?

No, that's not how Kconfig interprets it. There are many bool option
that depend on tristate options but can be enabled if the dependency
is built-in.

Take this one for example:

config FIRMWARE_EDID
       bool "Enable firmware EDID"
       depends on FB

We clearly want to be able to turn this on even for FB=m.

> > This also means we get another error from a missing export, which
> > this fixes as well:
> > 
> > ERROR: "exynos_mipi_dsi_register_lcd_driver" [drivers/video/fbdev/exynos/s6e8ax0.ko] undefined!
> > 
> > The drivers are all written to be loadable modules already,
> > except the Kconfig options for that are missing, which makes
> > the patch really easy.
> 
> Looks and sound fine, except doesn't this tell that the drivers have
> never been tested as modules? Did you or someone else actually test these?

No, this is not runtime tested. Generally there is very little that
can go wrong here though.

An alternative would be to change the dependency to

	depends on BACKLIGHT_CLASS_DEVICE=y

which doesn't allow the driver to be turned on for the =m case.
However, no other framebuffer driver does this.

> > diff --git a/drivers/video/fbdev/exynos/Makefile b/drivers/video/fbdev/exynos/Makefile
> > index b5b1bd228abb..02d8dc522fea 100644
> > --- a/drivers/video/fbdev/exynos/Makefile
> > +++ b/drivers/video/fbdev/exynos/Makefile
> > @@ -2,6 +2,8 @@
> >  # Makefile for the exynos video drivers.
> >  #
> >  
> > -obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
> > -				     	exynos_mipi_dsi_lowlevel.o
> > +obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos-mipi-dsi-mod.o
> > +
> > +exynos-mipi-dsi-mod-objs		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
> > +					   exynos_mipi_dsi_lowlevel.o
> 
> Hmm, why is this makefile change needed?

The original Makefile would link each file into a separate module, but that
cannot work, because they reference symbols from each other that are not
exported to other modules.

With my change, all the files get linked into a single module.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" 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. 29, 2016, 4:55 p.m. UTC | #3 
On 29/02/16 18:39, Arnd Bergmann wrote:
> On Monday 29 February 2016 18:12:45 Tomi Valkeinen wrote:
>> Hi,
>>
>> On 26/02/16 14:38, Arnd Bergmann wrote:
>>> The s6e8ax0 driver has a dependency on BACKLIGHT_CLASS_DEVICE,
>>> which can be configured as a loadable module, so we have to
>>> make the driver a tristate symbol as well, to avoid this error:
>>>
>>> drivers/built-in.o: In function `s6e8ax0_probe':
>>> :(.text+0x23a48): undefined reference to `devm_backlight_device_register'
>>
>> If a 'bool' Kconfig option depends on BACKLIGHT_CLASS_DEVICE, shouldn't
>> the Kconfig dependency take care of having BACKLIGHT_CLASS_DEVICE as
>> built-in?
> 
> No, that's not how Kconfig interprets it. There are many bool option
> that depend on tristate options but can be enabled if the dependency
> is built-in.
> 
> Take this one for example:
> 
> config FIRMWARE_EDID
>        bool "Enable firmware EDID"
>        depends on FB
> 
> We clearly want to be able to turn this on even for FB=m.

Right.

>>> This also means we get another error from a missing export, which
>>> this fixes as well:
>>>
>>> ERROR: "exynos_mipi_dsi_register_lcd_driver" [drivers/video/fbdev/exynos/s6e8ax0.ko] undefined!
>>>
>>> The drivers are all written to be loadable modules already,
>>> except the Kconfig options for that are missing, which makes
>>> the patch really easy.
>>
>> Looks and sound fine, except doesn't this tell that the drivers have
>> never been tested as modules? Did you or someone else actually test these?
> 
> No, this is not runtime tested. Generally there is very little that
> can go wrong here though.
> 
> An alternative would be to change the dependency to
> 
> 	depends on BACKLIGHT_CLASS_DEVICE=y
> 
> which doesn't allow the driver to be turned on for the =m case.
> However, no other framebuffer driver does this.

No, I think it's clearly better to make them tristate. I think all
drivers should be buildable as modules. It just makes me a bit
uncomfortable to enable code that has never been ran.

>>> diff --git a/drivers/video/fbdev/exynos/Makefile b/drivers/video/fbdev/exynos/Makefile
>>> index b5b1bd228abb..02d8dc522fea 100644
>>> --- a/drivers/video/fbdev/exynos/Makefile
>>> +++ b/drivers/video/fbdev/exynos/Makefile
>>> @@ -2,6 +2,8 @@
>>>  # Makefile for the exynos video drivers.
>>>  #
>>>  
>>> -obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
>>> -				     	exynos_mipi_dsi_lowlevel.o
>>> +obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos-mipi-dsi-mod.o
>>> +
>>> +exynos-mipi-dsi-mod-objs		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
>>> +					   exynos_mipi_dsi_lowlevel.o
>>
>> Hmm, why is this makefile change needed?
> 
> The original Makefile would link each file into a separate module, but that
> cannot work, because they reference symbols from each other that are not
> exported to other modules.
> 
> With my change, all the files get linked into a single module.

Yes, of course.

Thanks, I'll queue this up for 4.6.

 Tomi
diff mbox

Patch

diff --git a/drivers/video/fbdev/exynos/Kconfig b/drivers/video/fbdev/exynos/Kconfig
index 1f16b4678c71..d916bef94f25 100644
--- a/drivers/video/fbdev/exynos/Kconfig
+++ b/drivers/video/fbdev/exynos/Kconfig
@@ -3,7 +3,7 @@ 
 #
 
 menuconfig EXYNOS_VIDEO
-	bool "Exynos Video driver support"
+	tristate "Exynos Video driver support"
 	depends on ARCH_S5PV210 || ARCH_EXYNOS
 	help
 	  This enables support for EXYNOS Video device.
@@ -15,13 +15,13 @@  if EXYNOS_VIDEO
 #
 
 config EXYNOS_MIPI_DSI
-	bool "EXYNOS MIPI DSI driver support."
+	tristate "EXYNOS MIPI DSI driver support."
 	select GENERIC_PHY
 	help
 	  This enables support for MIPI-DSI device.
 
 config EXYNOS_LCD_S6E8AX0
-	bool "S6E8AX0 MIPI AMOLED LCD Driver"
+	tristate "S6E8AX0 MIPI AMOLED LCD Driver"
 	depends on EXYNOS_MIPI_DSI && BACKLIGHT_CLASS_DEVICE
 	depends on (LCD_CLASS_DEVICE = y)
 	default n
diff --git a/drivers/video/fbdev/exynos/Makefile b/drivers/video/fbdev/exynos/Makefile
index b5b1bd228abb..02d8dc522fea 100644
--- a/drivers/video/fbdev/exynos/Makefile
+++ b/drivers/video/fbdev/exynos/Makefile
@@ -2,6 +2,8 @@ 
 # Makefile for the exynos video drivers.
 #
 
-obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
-				     	exynos_mipi_dsi_lowlevel.o
+obj-$(CONFIG_EXYNOS_MIPI_DSI)		+= exynos-mipi-dsi-mod.o
+
+exynos-mipi-dsi-mod-objs		+= exynos_mipi_dsi.o exynos_mipi_dsi_common.o \
+					   exynos_mipi_dsi_lowlevel.o
 obj-$(CONFIG_EXYNOS_LCD_S6E8AX0)	+= s6e8ax0.o
diff --git a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
index 951b592794e3..92e4af3caaf8 100644
--- a/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/fbdev/exynos/exynos_mipi_dsi.c
@@ -263,6 +263,7 @@  int exynos_mipi_dsi_register_lcd_driver(struct mipi_dsim_lcd_driver *lcd_drv)
 	return 0;
 
 }
+EXPORT_SYMBOL_GPL(exynos_mipi_dsi_register_lcd_driver);
 
 static struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(
 						struct mipi_dsim_device *dsim,