[PATCH] drm/pl111: add ARM_AMBA dependency

[PATCH] drm/pl111: add ARM_AMBA dependency

[Arnd Bergmann]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Eric Anholt]

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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170522/951802e5/attachment.sig>

[PATCH] drm/pl111: add ARM_AMBA dependency

[Arnd Bergmann]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Eric Anholt]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170522/130d7423/attachment-0001.sig>

[PATCH] drm/pl111: add ARM_AMBA dependency

[Arnd Bergmann]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Krzysztof Kozlowski]

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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drm/pl111: add ARM_AMBA dependency

[Arnd Bergmann]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Krzysztof Kozlowski]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Arnd Bergmann]

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

[PATCH] drm/pl111: add ARM_AMBA dependency

[Krzysztof Kozlowski]

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