Re: [PATCH v2] video: exynos: fix modular build

[Tomi Valkeinen]

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

Attachment: signature.asc
Description: OpenPGP digital signature