| Message ID | 20170522152058.3346428-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Arnd Bergmann <arnd@arndb.de> writes: > The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, > but it just causes needless warnings: > > drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] > drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] > > This removes the #ifdef instead, and adds a dependency on ARM_AMBA to > only let us build the driver when the base support is enabled. > > Unfortunately, this requires removing one redundant 'select ARM_AMBA' > line from mach-s3c64xx to avoid a circular dependency. > > It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST > is turned on, but that should be a separate patch and may cause other > build regressions. If I understand the Kconfig, you're effectively disabling the build on COMPILE_TEST && !ARM. I'd rather that we just fix up the warnings, so that people can keep build-testing DRM drivers: https://patchwork.kernel.org/patch/9737857/
On Mon, May 22, 2017 at 05:20:08PM +0200, Arnd Bergmann wrote: > The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, > but it just causes needless warnings: > > drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] > drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] > > This removes the #ifdef instead, and adds a dependency on ARM_AMBA to > only let us build the driver when the base support is enabled. > > Unfortunately, this requires removing one redundant 'select ARM_AMBA' > line from mach-s3c64xx to avoid a circular dependency. > > It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST > is turned on, but that should be a separate patch and may cause other > build regressions. > > Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > arch/arm/mach-s3c64xx/Kconfig | 1 - > drivers/gpu/drm/pl111/Kconfig | 1 + > drivers/gpu/drm/pl111/pl111_drv.c | 2 -- > 3 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig > index 459214fa20b4..5ee5ad74a3d6 100644 > --- a/arch/arm/mach-s3c64xx/Kconfig > +++ b/arch/arm/mach-s3c64xx/Kconfig > @@ -40,7 +40,6 @@ config CPU_S3C6410 > > config S3C64XX_PL080 > def_bool DMADEVICES > - select ARM_AMBA I must admit that I miss how pl111 DRM driver create circular dependency with S3C64XX_PL080 (or AMBA_PL08X?). Maybe it is unrelated change and should be a separate patch? Best regards, Krzysztof > select AMBA_PL08X > > config S3C64XX_SETUP_SDHCI > diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig > index 309f4fd52de7..e2f393fcc4bf 100644 > --- a/drivers/gpu/drm/pl111/Kconfig > +++ b/drivers/gpu/drm/pl111/Kconfig > @@ -2,6 +2,7 @@ config DRM_PL111 > tristate "DRM Support for PL111 CLCD Controller" > depends on DRM > depends on ARM || ARM64 || COMPILE_TEST > + depends on ARM_AMBA > depends on COMMON_CLK > select DRM_KMS_HELPER > select DRM_KMS_CMA_HELPER > diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c > index 97095b1aa961..dce382f97f8c 100644 > --- a/drivers/gpu/drm/pl111/pl111_drv.c > +++ b/drivers/gpu/drm/pl111/pl111_drv.c > @@ -179,7 +179,6 @@ static struct drm_driver pl111_drm_driver = { > #endif > }; > > -#ifdef CONFIG_ARM_AMBA > static int pl111_amba_probe(struct amba_device *amba_dev, > const struct amba_id *id) > { > @@ -262,7 +261,6 @@ static struct amba_driver pl111_amba_driver = { > }; > > module_amba_driver(pl111_amba_driver); > -#endif /* CONFIG_ARM_AMBA */ > > MODULE_DESCRIPTION(DRIVER_DESC); > MODULE_AUTHOR("ARM Ltd."); > -- > 2.9.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 22, 2017 at 6:32 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Mon, May 22, 2017 at 05:20:08PM +0200, Arnd Bergmann wrote: >> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >> but it just causes needless warnings: >> >> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >> >> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >> only let us build the driver when the base support is enabled. >> >> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >> line from mach-s3c64xx to avoid a circular dependency. >> >> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >> is turned on, but that should be a separate patch and may cause other >> build regressions. >> >> Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") >> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >> --- >> arch/arm/mach-s3c64xx/Kconfig | 1 - >> drivers/gpu/drm/pl111/Kconfig | 1 + >> drivers/gpu/drm/pl111/pl111_drv.c | 2 -- >> 3 files changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig >> index 459214fa20b4..5ee5ad74a3d6 100644 >> --- a/arch/arm/mach-s3c64xx/Kconfig >> +++ b/arch/arm/mach-s3c64xx/Kconfig >> @@ -40,7 +40,6 @@ config CPU_S3C6410 >> >> config S3C64XX_PL080 >> def_bool DMADEVICES >> - select ARM_AMBA > > I must admit that I miss how pl111 DRM driver create circular dependency > with S3C64XX_PL080 (or AMBA_PL08X?). Maybe it is unrelated change and > should be a separate patch? When I add 'depends on ARM_AMBA' for pl111, I get this dependency loop: drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:381: symbol FB_CYBER2000_DDC depends on FB_CYBER2000 For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:369: symbol FB_CYBER2000 depends on FB For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:72: symbol DRM_KMS_FB_HELPER is selected by DRM_KMS_CMA_HELPER For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/Kconfig:137: symbol DRM_KMS_CMA_HELPER is selected by DRM_PL111 For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on ARM_AMBA For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/amba/Kconfig:1: symbol ARM_AMBA is selected by S3C64XX_PL080 For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" arch/arm/mach-s3c64xx/Kconfig:42: symbol S3C64XX_PL080 default value contains DMADEVICES For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SND_SOC_SH4_SIU For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" sound/soc/sh/Kconfig:29: symbol SND_SOC_SH4_SIU is selected by SND_SIU_MIGOR For a resolution refer to Documentation/kbuild/kconfig-language.txt subsection "Kconfig recursive dependency limitations" sound/soc/sh/Kconfig:59: symbol SND_SIU_MIGOR depends on I2C We could break the loop at any of those places, but the S3C64XX_PL080 symbol seemed the easiest way since the 'select' there is completely unnecessary and it directly relates to the change I made in PL111. Arnd
On Mon, May 22, 2017 at 6:23 PM, Eric Anholt <eric@anholt.net> wrote: > Arnd Bergmann <arnd@arndb.de> writes: > >> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >> but it just causes needless warnings: >> >> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >> >> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >> only let us build the driver when the base support is enabled. >> >> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >> line from mach-s3c64xx to avoid a circular dependency. >> >> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >> is turned on, but that should be a separate patch and may cause other >> build regressions. > > If I understand the Kconfig, you're effectively disabling the build on > COMPILE_TEST && !ARM. I'd rather that we just fix up the warnings, so > that people can keep build-testing DRM drivers: > > https://patchwork.kernel.org/patch/9737857/ That works too. When I tried the same, I thought it was a little too silly to have a file that effectively compiles into nothing. My initial approach did two things differently though: - use __maybe_unused instead of __used, since - annotate pl111_amba_driver instead of pl111_drm_driver/pl111_modeset_init, and keep only module_amba_driver() in the #ifdef. Arnd
Arnd Bergmann <arnd@arndb.de> writes: > On Mon, May 22, 2017 at 6:23 PM, Eric Anholt <eric@anholt.net> wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> >>> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >>> but it just causes needless warnings: >>> >>> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >>> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >>> >>> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >>> only let us build the driver when the base support is enabled. >>> >>> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >>> line from mach-s3c64xx to avoid a circular dependency. >>> >>> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >>> is turned on, but that should be a separate patch and may cause other >>> build regressions. >> >> If I understand the Kconfig, you're effectively disabling the build on >> COMPILE_TEST && !ARM. I'd rather that we just fix up the warnings, so >> that people can keep build-testing DRM drivers: >> >> https://patchwork.kernel.org/patch/9737857/ > > That works too. When I tried the same, I thought it was a little too silly to > have a file that effectively compiles into nothing. > > My initial approach did two things differently though: > > - use __maybe_unused instead of __used, since > > - annotate pl111_amba_driver instead of > pl111_drm_driver/pl111_modeset_init, and keep only > module_amba_driver() in the #ifdef. Oh, yeah, we've eliminated the amba_request_regions() that used to be why the probe had to be under the #ifdef, so your solution would get us better coverage and simpler code. If you could send that patch to dri-devel today, I'll get it applied.
On Mon, May 22, 2017 at 11:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, May 22, 2017 at 6:32 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On Mon, May 22, 2017 at 05:20:08PM +0200, Arnd Bergmann wrote: >>> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >>> but it just causes needless warnings: >>> >>> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >>> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >>> >>> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >>> only let us build the driver when the base support is enabled. >>> >>> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >>> line from mach-s3c64xx to avoid a circular dependency. >>> >>> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >>> is turned on, but that should be a separate patch and may cause other >>> build regressions. >>> >>> Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") >>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>> --- >>> arch/arm/mach-s3c64xx/Kconfig | 1 - >>> drivers/gpu/drm/pl111/Kconfig | 1 + >>> drivers/gpu/drm/pl111/pl111_drv.c | 2 -- >>> 3 files changed, 1 insertion(+), 3 deletions(-) >>> >>> diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig >>> index 459214fa20b4..5ee5ad74a3d6 100644 >>> --- a/arch/arm/mach-s3c64xx/Kconfig >>> +++ b/arch/arm/mach-s3c64xx/Kconfig >>> @@ -40,7 +40,6 @@ config CPU_S3C6410 >>> >>> config S3C64XX_PL080 >>> def_bool DMADEVICES >>> - select ARM_AMBA >> >> I must admit that I miss how pl111 DRM driver create circular dependency >> with S3C64XX_PL080 (or AMBA_PL08X?). Maybe it is unrelated change and >> should be a separate patch? > > When I add 'depends on ARM_AMBA' for pl111, I get this dependency loop: > > drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/video/fbdev/Kconfig:381: symbol FB_CYBER2000_DDC depends on FB_CYBER2000 > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/video/fbdev/Kconfig:369: symbol FB_CYBER2000 depends on FB > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/gpu/drm/Kconfig:72: symbol DRM_KMS_FB_HELPER is selected by > DRM_KMS_CMA_HELPER > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/gpu/drm/Kconfig:137: symbol DRM_KMS_CMA_HELPER is selected by DRM_PL111 > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on ARM_AMBA > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/amba/Kconfig:1: symbol ARM_AMBA is selected by S3C64XX_PL080 > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > arch/arm/mach-s3c64xx/Kconfig:42: symbol S3C64XX_PL080 default value > contains DMADEVICES > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SND_SOC_SH4_SIU > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > sound/soc/sh/Kconfig:29: symbol SND_SOC_SH4_SIU is selected by SND_SIU_MIGOR > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > sound/soc/sh/Kconfig:59: symbol SND_SIU_MIGOR depends on I2C > > We could break the loop at any of those places, but the > S3C64XX_PL080 symbol seemed the easiest way since the > 'select' there is completely unnecessary and it directly relates > to the change I made in PL111. Assuming ARM_AMBA is unnecessary here, then it should be unnecessary in other platforms as well but we have it (git grep 'select ARM_AMBA'). The ARM_AMBA is a dependency of AMBA_PL08X which is selected by S3C64xx... so IMHO this is needed. Best regards, Krzysztof
On Tue, May 23, 2017 at 1:17 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: > On Mon, May 22, 2017 at 11:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Mon, May 22, 2017 at 6:32 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>> On Mon, May 22, 2017 at 05:20:08PM +0200, Arnd Bergmann wrote: >>>> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >>>> but it just causes needless warnings: >>>> >>>> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >>>> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >>>> >>>> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >>>> only let us build the driver when the base support is enabled. >>>> >>>> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >>>> line from mach-s3c64xx to avoid a circular dependency. >>>> >>>> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >>>> is turned on, but that should be a separate patch and may cause other >>>> build regressions. >>>> >>>> Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") >>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>>> --- >>>> arch/arm/mach-s3c64xx/Kconfig | 1 - >>>> drivers/gpu/drm/pl111/Kconfig | 1 + >>>> drivers/gpu/drm/pl111/pl111_drv.c | 2 -- >>>> 3 files changed, 1 insertion(+), 3 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig >>>> index 459214fa20b4..5ee5ad74a3d6 100644 >>>> --- a/arch/arm/mach-s3c64xx/Kconfig >>>> +++ b/arch/arm/mach-s3c64xx/Kconfig >>>> @@ -40,7 +40,6 @@ config CPU_S3C6410 >>>> >>>> config S3C64XX_PL080 >>>> def_bool DMADEVICES >>>> - select ARM_AMBA >>> >>> I must admit that I miss how pl111 DRM driver create circular dependency >>> with S3C64XX_PL080 (or AMBA_PL08X?). Maybe it is unrelated change and >>> should be a separate patch? >> >> When I add 'depends on ARM_AMBA' for pl111, I get this dependency loop: >> >> drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/video/fbdev/Kconfig:381: symbol FB_CYBER2000_DDC depends on FB_CYBER2000 >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/video/fbdev/Kconfig:369: symbol FB_CYBER2000 depends on FB >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/gpu/drm/Kconfig:72: symbol DRM_KMS_FB_HELPER is selected by >> DRM_KMS_CMA_HELPER >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/gpu/drm/Kconfig:137: symbol DRM_KMS_CMA_HELPER is selected by DRM_PL111 >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on ARM_AMBA >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/amba/Kconfig:1: symbol ARM_AMBA is selected by S3C64XX_PL080 >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> arch/arm/mach-s3c64xx/Kconfig:42: symbol S3C64XX_PL080 default value >> contains DMADEVICES >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SND_SOC_SH4_SIU >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> sound/soc/sh/Kconfig:29: symbol SND_SOC_SH4_SIU is selected by SND_SIU_MIGOR >> For a resolution refer to Documentation/kbuild/kconfig-language.txt >> subsection "Kconfig recursive dependency limitations" >> sound/soc/sh/Kconfig:59: symbol SND_SIU_MIGOR depends on I2C >> >> We could break the loop at any of those places, but the >> S3C64XX_PL080 symbol seemed the easiest way since the >> 'select' there is completely unnecessary and it directly relates >> to the change I made in PL111. > > Assuming ARM_AMBA is unnecessary here, then it should be unnecessary > in other platforms as well but we have it (git grep 'select > ARM_AMBA'). The ARM_AMBA is a dependency of AMBA_PL08X which is > selected by S3C64xx... so IMHO this is needed. I guess I should have explained it in more detail: S3C64XX_PL080 is set to 'ARCH_S3C64XX && DMADEVICES', and ARCH_S3C64XX selects ARM_AMBA, so the 'select ARM_AMBA' for S3C64XX_PL080 is redundant. We still need it, but it's already guaranteed to be enabled. Arnd
On Tue, May 23, 2017 at 1:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Tue, May 23, 2017 at 1:17 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: >> On Mon, May 22, 2017 at 11:37 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Mon, May 22, 2017 at 6:32 PM, Krzysztof Kozlowski <krzk@kernel.org> wrote: >>>> On Mon, May 22, 2017 at 05:20:08PM +0200, Arnd Bergmann wrote: >>>>> The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, >>>>> but it just causes needless warnings: >>>>> >>>>> drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] >>>>> drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] >>>>> >>>>> This removes the #ifdef instead, and adds a dependency on ARM_AMBA to >>>>> only let us build the driver when the base support is enabled. >>>>> >>>>> Unfortunately, this requires removing one redundant 'select ARM_AMBA' >>>>> line from mach-s3c64xx to avoid a circular dependency. >>>>> >>>>> It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST >>>>> is turned on, but that should be a separate patch and may cause other >>>>> build regressions. >>>>> >>>>> Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") >>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de> >>>>> --- >>>>> arch/arm/mach-s3c64xx/Kconfig | 1 - >>>>> drivers/gpu/drm/pl111/Kconfig | 1 + >>>>> drivers/gpu/drm/pl111/pl111_drv.c | 2 -- >>>>> 3 files changed, 1 insertion(+), 3 deletions(-) >>>>> >>>>> diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig >>>>> index 459214fa20b4..5ee5ad74a3d6 100644 >>>>> --- a/arch/arm/mach-s3c64xx/Kconfig >>>>> +++ b/arch/arm/mach-s3c64xx/Kconfig >>>>> @@ -40,7 +40,6 @@ config CPU_S3C6410 >>>>> >>>>> config S3C64XX_PL080 >>>>> def_bool DMADEVICES >>>>> - select ARM_AMBA >>>> >>>> I must admit that I miss how pl111 DRM driver create circular dependency >>>> with S3C64XX_PL080 (or AMBA_PL08X?). Maybe it is unrelated change and >>>> should be a separate patch? >>> >>> When I add 'depends on ARM_AMBA' for pl111, I get this dependency loop: >>> >>> drivers/i2c/Kconfig:7: symbol I2C is selected by FB_DDC >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/video/fbdev/Kconfig:63: symbol FB_DDC is selected by FB_CYBER2000_DDC >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/video/fbdev/Kconfig:381: symbol FB_CYBER2000_DDC depends on FB_CYBER2000 >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/video/fbdev/Kconfig:369: symbol FB_CYBER2000 depends on FB >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/video/fbdev/Kconfig:5: symbol FB is selected by DRM_KMS_FB_HELPER >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/gpu/drm/Kconfig:72: symbol DRM_KMS_FB_HELPER is selected by >>> DRM_KMS_CMA_HELPER >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/gpu/drm/Kconfig:137: symbol DRM_KMS_CMA_HELPER is selected by DRM_PL111 >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on ARM_AMBA >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/amba/Kconfig:1: symbol ARM_AMBA is selected by S3C64XX_PL080 >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> arch/arm/mach-s3c64xx/Kconfig:42: symbol S3C64XX_PL080 default value >>> contains DMADEVICES >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> drivers/dma/Kconfig:5: symbol DMADEVICES is selected by SND_SOC_SH4_SIU >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> sound/soc/sh/Kconfig:29: symbol SND_SOC_SH4_SIU is selected by SND_SIU_MIGOR >>> For a resolution refer to Documentation/kbuild/kconfig-language.txt >>> subsection "Kconfig recursive dependency limitations" >>> sound/soc/sh/Kconfig:59: symbol SND_SIU_MIGOR depends on I2C >>> >>> We could break the loop at any of those places, but the >>> S3C64XX_PL080 symbol seemed the easiest way since the >>> 'select' there is completely unnecessary and it directly relates >>> to the change I made in PL111. >> >> Assuming ARM_AMBA is unnecessary here, then it should be unnecessary >> in other platforms as well but we have it (git grep 'select >> ARM_AMBA'). The ARM_AMBA is a dependency of AMBA_PL08X which is >> selected by S3C64xx... so IMHO this is needed. > > I guess I should have explained it in more detail: S3C64XX_PL080 > is set to 'ARCH_S3C64XX && DMADEVICES', and ARCH_S3C64XX > selects ARM_AMBA, so the 'select ARM_AMBA' for S3C64XX_PL080 > is redundant. We still need it, but it's already guaranteed to be enabled. Ah, yes, I missed the S3C64XX_PL080. Looks fine. For the s3c64xx: Acked-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof
On Tue, May 23, 2017 at 12:08 AM, Eric Anholt <eric@anholt.net> wrote: > > Oh, yeah, we've eliminated the amba_request_regions() that used to be > why the probe had to be under the #ifdef, so your solution would get us > better coverage and simpler code. If you could send that patch to > dri-devel today, I'll get it applied. Sent now. I'll also send the oneline patch for s3c64xx separately, in case we hit the same circular dependency another time. Arnd
diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig index 459214fa20b4..5ee5ad74a3d6 100644 --- a/arch/arm/mach-s3c64xx/Kconfig +++ b/arch/arm/mach-s3c64xx/Kconfig @@ -40,7 +40,6 @@ config CPU_S3C6410 config S3C64XX_PL080 def_bool DMADEVICES - select ARM_AMBA select AMBA_PL08X config S3C64XX_SETUP_SDHCI diff --git a/drivers/gpu/drm/pl111/Kconfig b/drivers/gpu/drm/pl111/Kconfig index 309f4fd52de7..e2f393fcc4bf 100644 --- a/drivers/gpu/drm/pl111/Kconfig +++ b/drivers/gpu/drm/pl111/Kconfig @@ -2,6 +2,7 @@ config DRM_PL111 tristate "DRM Support for PL111 CLCD Controller" depends on DRM depends on ARM || ARM64 || COMPILE_TEST + depends on ARM_AMBA depends on COMMON_CLK select DRM_KMS_HELPER select DRM_KMS_CMA_HELPER diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c index 97095b1aa961..dce382f97f8c 100644 --- a/drivers/gpu/drm/pl111/pl111_drv.c +++ b/drivers/gpu/drm/pl111/pl111_drv.c @@ -179,7 +179,6 @@ static struct drm_driver pl111_drm_driver = { #endif }; -#ifdef CONFIG_ARM_AMBA static int pl111_amba_probe(struct amba_device *amba_dev, const struct amba_id *id) { @@ -262,7 +261,6 @@ static struct amba_driver pl111_amba_driver = { }; module_amba_driver(pl111_amba_driver); -#endif /* CONFIG_ARM_AMBA */ MODULE_DESCRIPTION(DRIVER_DESC); MODULE_AUTHOR("ARM Ltd.");
The driver is written in a way to enable compile-testing without CONFIG_ARM_AMBA, but it just causes needless warnings: drivers/gpu/drm/pl111/pl111_drv.c:149:26: error: 'pl111_drm_driver' defined but not used [-Werror=unused-variable] drivers/gpu/drm/pl111/pl111_drv.c:81:12: error: 'pl111_modeset_init' defined but not used [-Werror=unused-function] This removes the #ifdef instead, and adds a dependency on ARM_AMBA to only let us build the driver when the base support is enabled. Unfortunately, this requires removing one redundant 'select ARM_AMBA' line from mach-s3c64xx to avoid a circular dependency. It might be good to allow manually enabling ARM_AMBA when COMPILE_TEST is turned on, but that should be a separate patch and may cause other build regressions. Fixes: bed41005e617 ("drm/pl111: Initial drm/kms driver for pl111") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- arch/arm/mach-s3c64xx/Kconfig | 1 - drivers/gpu/drm/pl111/Kconfig | 1 + drivers/gpu/drm/pl111/pl111_drv.c | 2 -- 3 files changed, 1 insertion(+), 3 deletions(-)