[PATCH] drm/exynos: fix multiple definition build error

[PATCH] drm/exynos: fix multiple definition build error

[Inki Dae]

This patch fixes multiple definition error like below when building it
as moudle with device tree support.

drivers/gpu/drm/exynos/exynos_drm_g2d.o: In function `.LANCHOR1':
exynos_drm_g2d.c:(.rodata+0x6c): multiple definition of `__mod_of_device_table'
drivers/gpu/drm/exynos/exynos_drm_fimd.o:exynos_drm_fimd.c:(.rodata+0x144): first defined here

Signed-off-by: Inki Dae <inki.dae@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_fimd.c |    2 +-
 drivers/gpu/drm/exynos/exynos_drm_g2d.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 746b282..1e02d13 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -117,7 +117,7 @@ static const struct of_device_id fimd_driver_dt_match[] = {
 	  .data = &exynos5_fimd_driver_data },
 	{},
 };
-MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);
+MODULE_DEVICE_TABLE(of_fimd, fimd_driver_dt_match);
 #endif
 
 static inline struct fimd_driver_data *drm_fimd_get_driver_data(
diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
index 47a493c..6a01ff1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c
@@ -1525,7 +1525,7 @@ static const struct of_device_id exynos_g2d_match[] = {
 	{ .compatible = "samsung,exynos5250-g2d" },
 	{},
 };
-MODULE_DEVICE_TABLE(of, exynos_g2d_match);
+MODULE_DEVICE_TABLE(of_g2d, exynos_g2d_match);
 #endif
 
 struct platform_driver g2d_driver = {
-- 
1.7.5.4

Re: [PATCH] drm/exynos: fix multiple definition build error

[Tomasz Figa]

Hi Inki,

On Friday 26 of April 2013 14:03:10 Inki Dae wrote:
> This patch fixes multiple definition error like below when building it
> as moudle with device tree support.
> 
> drivers/gpu/drm/exynos/exynos_drm_g2d.o: In function `.LANCHOR1':
> exynos_drm_g2d.c:(.rodata+0x6c): multiple definition of
> `__mod_of_device_table'
> drivers/gpu/drm/exynos/exynos_drm_fimd.o:exynos_drm_fimd.c:(.rodata+0x1
> 44): first defined here
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c |    2 +-
>  drivers/gpu/drm/exynos/exynos_drm_g2d.c  |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 746b282..1e02d13
> 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -117,7 +117,7 @@ static const struct of_device_id
> fimd_driver_dt_match[] = { .data = &exynos5_fimd_driver_data },
>  	{},
>  };
> -MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);
> +MODULE_DEVICE_TABLE(of_fimd, fimd_driver_dt_match);

I wonder if this change wouldn't break the purpose of having 
MODULE_DEVICE_TABLE at all.

As far as I remember, this is used to create a symbol with well known name 
that userspace tools can use to identify what devices are handled in this 
module. For example

MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);

results in creation of __mod_of_device_table symbol, of which tools, such 
as depmod are aware and can build a list of supported devices.

Your change will result in creation of __mod_of_fimd_device_table, which 
is unknown and won't be of any use.

By the way, looking at the definition of MODULE_DEVICE_TABLE, which is

139 #define MODULE_DEVICE_TABLE(type,name)          \
140   MODULE_GENERIC_TABLE(type##_device,name)

and then MODULE_GENERIC_TABLE

 85 #ifdef MODULE
 86 #define MODULE_GENERIC_TABLE(gtype,name)                        \
 87 extern const struct gtype##_id __mod_##gtype##_table            \
 88   __attribute__ ((unused, alias(__stringify(name))))
 89 
 90 #else  /* !MODULE */
 91 #define MODULE_GENERIC_TABLE(gtype,name)
 92 #endif

it seems like the exact line that will be generated after your change, 
will be

extern const struct of_fimd_device_id __mod_of_fimd_device_table
	__attribute__ ((unused, alias(__stringify(name))));

which seems wrong, because of_fimd_device_id is not a correct struct type.

Best regards,
Tomasz

Re: [PATCH] drm/exynos: fix multiple definition build error

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 3087 bytes --]

Hi Tomasz,


2013/4/26 Tomasz Figa <tomasz.figa@gmail.com>

> Hi Inki,
>
> On Friday 26 of April 2013 14:03:10 Inki Dae wrote:
> > This patch fixes multiple definition error like below when building it
> > as moudle with device tree support.
> >
> > drivers/gpu/drm/exynos/exynos_drm_g2d.o: In function `.LANCHOR1':
> > exynos_drm_g2d.c:(.rodata+0x6c): multiple definition of
> > `__mod_of_device_table'
> > drivers/gpu/drm/exynos/exynos_drm_fimd.o:exynos_drm_fimd.c:(.rodata+0x1
> > 44): first defined here
> >
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_fimd.c |    2 +-
> >  drivers/gpu/drm/exynos/exynos_drm_g2d.c  |    2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > b/drivers/gpu/drm/exynos/exynos_drm_fimd.c index 746b282..1e02d13
> > 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> > @@ -117,7 +117,7 @@ static const struct of_device_id
> > fimd_driver_dt_match[] = { .data = &exynos5_fimd_driver_data },
> >       {},
> >  };
> > -MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);
> > +MODULE_DEVICE_TABLE(of_fimd, fimd_driver_dt_match);
>
> I wonder if this change wouldn't break the purpose of having
> MODULE_DEVICE_TABLE at all.
>
> As far as I remember, this is used to create a symbol with well known name
> that userspace tools can use to identify what devices are handled in this
> module. For example
>
> MODULE_DEVICE_TABLE(of, fimd_driver_dt_match);
>
> results in creation of __mod_of_device_table symbol, of which tools, such
> as depmod are aware and can build a list of supported devices.
>
> Your change will result in creation of __mod_of_fimd_device_table, which
> is unknown and won't be of any use.
>
> By the way, looking at the definition of MODULE_DEVICE_TABLE, which is
>
> 139 #define MODULE_DEVICE_TABLE(type,name)          \
> 140   MODULE_GENERIC_TABLE(type##_device,name)
>
> and then MODULE_GENERIC_TABLE
>
>  85 #ifdef MODULE
>  86 #define MODULE_GENERIC_TABLE(gtype,name)                        \
>  87 extern const struct gtype##_id __mod_##gtype##_table            \
>  88   __attribute__ ((unused, alias(__stringify(name))))
>  89
>  90 #else  /* !MODULE */
>  91 #define MODULE_GENERIC_TABLE(gtype,name)
>  92 #endif
>
> it seems like the exact line that will be generated after your change,
> will be
>
> extern const struct of_fimd_device_id __mod_of_fimd_device_table
>         __attribute__ ((unused, alias(__stringify(name))));
>

Exactly right. it's my mistake. But now it seems that
__mode_of_device_table is multi defined at fimd and g2d side so there still
is module build error. :(

Thanks,
Inki Dae



>
> which seems wrong, because of_fimd_device_id is not a correct struct type.
>
> Best regards,
> Tomasz
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 4227 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/exynos: fix multiple definition build error

[Sylwester Nawrocki]

On 04/26/2013 10:20 AM, Inki Dae wrote:
> Exactly right. it's my mistake. But now it seems that __mode_of_device_table is
> multi defined at fimd and g2d side so there still is module build error. :(

Since all drivers seem to be linked into single a single module, you 
likely need to create a separate table of struct of_device_id just for 
the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain 
'compatible' strings for all devices. Or choose of_device_id for just 
one device and define MODULE_DEVICE_TABLE() for it in some common place,
e.g. exynos_drm_drv.c. I believe all devices should be listed though.


Thanks,
Sylwester

Re: [PATCH] drm/exynos: fix multiple definition build error

[Tomasz Figa]

On Friday 26 of April 2013 11:48:50 Sylwester Nawrocki wrote:
> On 04/26/2013 10:20 AM, Inki Dae wrote:
> > Exactly right. it's my mistake. But now it seems that
> > __mode_of_device_table is multi defined at fimd and g2d side so there
> > still is module build error. :(
> Since all drivers seem to be linked into single a single module, you
> likely need to create a separate table of struct of_device_id just for
> the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain
> 'compatible' strings for all devices. Or choose of_device_id for just
> one device and define MODULE_DEVICE_TABLE() for it in some common place,
> e.g. exynos_drm_drv.c. I believe all devices should be listed though.

IMHO, the most proper solution would be to split the module into parent 
exynos_drm module and per-device submodules, which would depend on the 
parent module.

This way you would be able to load dynamically any submodule you want, 
without recompiling the modules.

Best regards,
Tomasz

Re: [PATCH] drm/exynos: fix multiple definition build error

[Sylwester Nawrocki]

On 04/26/2013 09:42 PM, Tomasz Figa wrote:
> On Friday 26 of April 2013 11:48:50 Sylwester Nawrocki wrote:
>> On 04/26/2013 10:20 AM, Inki Dae wrote:
>>> Exactly right. it's my mistake. But now it seems that
>>> __mode_of_device_table is multi defined at fimd and g2d side so there
>>> still is module build error. :(
>> Since all drivers seem to be linked into single a single module, you
>> likely need to create a separate table of struct of_device_id just for
>> the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain
>> 'compatible' strings for all devices. Or choose of_device_id for just
>> one device and define MODULE_DEVICE_TABLE() for it in some common place,
>> e.g. exynos_drm_drv.c. I believe all devices should be listed though.
>
> IMHO, the most proper solution would be to split the module into parent
> exynos_drm module and per-device submodules, which would depend on the
> parent module.
>
> This way you would be able to load dynamically any submodule you want,
> without recompiling the modules.

Yes, this is how it would have been in a perfect world. Probably something
worth to consider for the future, it likely implies a lot of work.

OTOH if there is one device for which the driver will always be present
in the main module it should be enough to make a single entry
MODULE_DEVICE_TABLE including its compatible string to ensure the driver
is properly loaded, shouldn't it ?

Re: [PATCH] drm/exynos: fix multiple definition build error

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 1444 bytes --]

2013/4/27 Tomasz Figa <tomasz.figa-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> On Friday 26 of April 2013 11:48:50 Sylwester Nawrocki wrote:
> > On 04/26/2013 10:20 AM, Inki Dae wrote:
> > > Exactly right. it's my mistake. But now it seems that
> > > __mode_of_device_table is multi defined at fimd and g2d side so there
> > > still is module build error. :(
> > Since all drivers seem to be linked into single a single module, you
> > likely need to create a separate table of struct of_device_id just for
> > the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain
> > 'compatible' strings for all devices. Or choose of_device_id for just
> > one device and define MODULE_DEVICE_TABLE() for it in some common place,
> > e.g. exynos_drm_drv.c. I believe all devices should be listed though.
>
> IMHO, the most proper solution would be to split the module into parent
> exynos_drm module and per-device submodules, which would depend on the
> parent module.
>
>
This way had already been used a long time ago and there was some probing
issues so we made exynos drm framework to be one module.


>

This way you would be able to load dynamically any submodule you want,
> without recompiling the modules.
>
> Best regards,
> Tomasz
>
> _______________________________________________
> dri-devel mailing list
> dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 2764 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH] drm/exynos: fix multiple definition build error

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 2323 bytes --]

2013/4/27 Sylwester Nawrocki <sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

> On 04/26/2013 09:42 PM, Tomasz Figa wrote:
>
>> On Friday 26 of April 2013 11:48:50 Sylwester Nawrocki wrote:
>>
>>> On 04/26/2013 10:20 AM, Inki Dae wrote:
>>>
>>>> Exactly right. it's my mistake. But now it seems that
>>>> __mode_of_device_table is multi defined at fimd and g2d side so there
>>>> still is module build error. :(
>>>>
>>> Since all drivers seem to be linked into single a single module, you
>>> likely need to create a separate table of struct of_device_id just for
>>> the purpose of MODULE_DEVICE_TABLE(of, ...). This table would contain
>>> 'compatible' strings for all devices. Or choose of_device_id for just
>>> one device and define MODULE_DEVICE_TABLE() for it in some common place,
>>> e.g. exynos_drm_drv.c. I believe all devices should be listed though.
>>>
>>
>> IMHO, the most proper solution would be to split the module into parent
>> exynos_drm module and per-device submodules, which would depend on the
>> parent module.
>>
>> This way you would be able to load dynamically any submodule you want,
>> without recompiling the modules.
>>
>
> Yes, this is how it would have been in a perfect world. Probably something
> worth to consider for the future, it likely implies a lot of work.
>
>
For now, exynos drm framework has a problem to module support with device
tree. And for this, going back to previous style might be a right way but
this also still has the probing issue. Frankly spreaking, I don't really
want back to previous style because that way not only still has a probing
issue but also makes exynos drm framework so complicated(there are many
things to consider every time framework updating). So I'd like to resolve
this issue as is first.


>

OTOH if there is one device for which the driver will always be present
> in the main module it should be enough to make a single entry
> MODULE_DEVICE_TABLE including its compatible string to ensure the driver
> is properly loaded, shouldn't it ?
>
>

--
> To unsubscribe from this list: send the line "unsubscribe
> linux-samsung-soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/**majordomo-info.html<http://vger.kernel.org/majordomo-info.html>
>

[-- Attachment #1.2: Type: text/html, Size: 4064 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss