diff mbox

[26/30] dmaengine: s3c24xx: enable COMPILE_TEST

Message ID 1473960849-23024-27-git-send-email-vinod.koul@intel.com (mailing list archive)
State Superseded
Headers show

Commit Message

Vinod Koul Sept. 15, 2016, 5:34 p.m. UTC
To get more coverage, enable COMPILE_TEST for this driver.

Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 drivers/dma/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Sept. 16, 2016, 12:56 p.m. UTC | #1
On 09/15/2016 07:34 PM, Vinod Koul wrote:
> To get more coverage, enable COMPILE_TEST for this driver.
> 
> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  drivers/dma/Kconfig | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> index 82d6e374d82f..60341185a6b9 100644
> --- a/drivers/dma/Kconfig
> +++ b/drivers/dma/Kconfig
> @@ -451,8 +451,8 @@ config STM32_DMA
>  	  here.
>  
>  config S3C24XX_DMAC
> -	bool "Samsung S3C24XX DMA support"
> -	depends on ARCH_S3C24XX
> +	bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
> +	default ARCH_S3C24XX

This is not equivalent to previous code.

I think it should be:
bool "Samsung S3C24XX DMA support" if COMPILE_TEST
depends on ARCH_S3C24XX || COMPILE_TEST

1. This way we do not enable it by default on the architecture (it
wasn't enabled so far),
2. The option is visible on compile test,
3. The runtime dependency is preserved.

Best regards,
Krzysztof

>  	select DMA_ENGINE
>  	select DMA_VIRTUAL_CHANNELS
>  	help
> 

--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 16, 2016, 2:03 p.m. UTC | #2
On Friday, September 16, 2016 2:56:32 PM CEST Krzysztof Kozlowski wrote:
> On 09/15/2016 07:34 PM, Vinod Koul wrote:
> > To get more coverage, enable COMPILE_TEST for this driver.
> > 
> > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  drivers/dma/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 82d6e374d82f..60341185a6b9 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -451,8 +451,8 @@ config STM32_DMA
> >         here.
> >  
> >  config S3C24XX_DMAC
> > -     bool "Samsung S3C24XX DMA support"
> > -     depends on ARCH_S3C24XX
> > +     bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
> > +     default ARCH_S3C24XX
> 
> This is not equivalent to previous code.
> 
> I think it should be:
> bool "Samsung S3C24XX DMA support" if COMPILE_TEST
> depends on ARCH_S3C24XX || COMPILE_TEST
> 
> 1. This way we do not enable it by default on the architecture (it
> wasn't enabled so far),
> 2. The option is visible on compile test,
> 3. The runtime dependency is preserved.

Unless you add the "default ARCH_S3C24XX", it will be disabled for
normal configurations and impossible to get turned on, which is
also wrong. I think in this case we want:

 config S3C24XX_DMAC
 	bool "Samsung S3C24XX DMA support"
-	depends on ARCH_S3C24XX
+	depends on ARCH_S3C24XX || COMPILE_TEST

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul Sept. 16, 2016, 2:51 p.m. UTC | #3
On Fri, Sep 16, 2016 at 02:56:32PM +0200, Krzysztof Kozlowski wrote:
> On 09/15/2016 07:34 PM, Vinod Koul wrote:
> > To get more coverage, enable COMPILE_TEST for this driver.
> > 
> > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > ---
> >  drivers/dma/Kconfig | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 82d6e374d82f..60341185a6b9 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -451,8 +451,8 @@ config STM32_DMA
> >  	  here.
> >  
> >  config S3C24XX_DMAC
> > -	bool "Samsung S3C24XX DMA support"
> > -	depends on ARCH_S3C24XX
> > +	bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
> > +	default ARCH_S3C24XX
> 
> This is not equivalent to previous code.

Yeah it is not and enables the driver by default. My bad, I should have
explicitly mentioned this.

My though was to indeed enable them by default, do you see any issues with
that?

> 
> I think it should be:
> bool "Samsung S3C24XX DMA support" if COMPILE_TEST
> depends on ARCH_S3C24XX || COMPILE_TEST
> 
> 1. This way we do not enable it by default on the architecture (it
> wasn't enabled so far),
> 2. The option is visible on compile test,
> 3. The runtime dependency is preserved.
> 
> Best regards,
> Krzysztof
> 
> >  	select DMA_ENGINE
> >  	select DMA_VIRTUAL_CHANNELS
> >  	help
> > 
>
Krzysztof Kozlowski Sept. 17, 2016, 6:38 p.m. UTC | #4
On Fri, Sep 16, 2016 at 04:03:57PM +0200, Arnd Bergmann wrote:
> On Friday, September 16, 2016 2:56:32 PM CEST Krzysztof Kozlowski wrote:
> > On 09/15/2016 07:34 PM, Vinod Koul wrote:
> > > To get more coverage, enable COMPILE_TEST for this driver.
> > > 
> > > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > >  drivers/dma/Kconfig | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > > index 82d6e374d82f..60341185a6b9 100644
> > > --- a/drivers/dma/Kconfig
> > > +++ b/drivers/dma/Kconfig
> > > @@ -451,8 +451,8 @@ config STM32_DMA
> > >         here.
> > >  
> > >  config S3C24XX_DMAC
> > > -     bool "Samsung S3C24XX DMA support"
> > > -     depends on ARCH_S3C24XX
> > > +     bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
> > > +     default ARCH_S3C24XX
> > 
> > This is not equivalent to previous code.
> > 
> > I think it should be:
> > bool "Samsung S3C24XX DMA support" if COMPILE_TEST
> > depends on ARCH_S3C24XX || COMPILE_TEST
> > 
> > 1. This way we do not enable it by default on the architecture (it
> > wasn't enabled so far),
> > 2. The option is visible on compile test,
> > 3. The runtime dependency is preserved.
> 
> Unless you add the "default ARCH_S3C24XX", it will be disabled for
> normal configurations and impossible to get turned on, which is
> also wrong. I think in this case we want:
> 
>  config S3C24XX_DMAC
>  	bool "Samsung S3C24XX DMA support"
> -	depends on ARCH_S3C24XX
> +	depends on ARCH_S3C24XX || COMPILE_TEST

Right, the bool should remain untouched.

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Krzysztof Kozlowski Sept. 17, 2016, 6:47 p.m. UTC | #5
On Fri, Sep 16, 2016 at 08:21:13PM +0530, Vinod Koul wrote:
> On Fri, Sep 16, 2016 at 02:56:32PM +0200, Krzysztof Kozlowski wrote:
> > On 09/15/2016 07:34 PM, Vinod Koul wrote:
> > > To get more coverage, enable COMPILE_TEST for this driver.
> > > 
> > > Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> > > ---
> > >  drivers/dma/Kconfig | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > > index 82d6e374d82f..60341185a6b9 100644
> > > --- a/drivers/dma/Kconfig
> > > +++ b/drivers/dma/Kconfig
> > > @@ -451,8 +451,8 @@ config STM32_DMA
> > >  	  here.
> > >  
> > >  config S3C24XX_DMAC
> > > -	bool "Samsung S3C24XX DMA support"
> > > -	depends on ARCH_S3C24XX
> > > +	bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
> > > +	default ARCH_S3C24XX
> > 
> > This is not equivalent to previous code.
> 
> Yeah it is not and enables the driver by default. My bad, I should have
> explicitly mentioned this.
> 
> My though was to indeed enable them by default, do you see any issues with
> that?

I don't see any particular problems with that. Default value is just a
hint and can be overridden. I think however that such change should be
in separate patch. One thing is adding compile-test and second is
turning this on for our defconfigs. They might have different outcomes
(failing of various archs/configs for the first, failing of our
defconfigs for the latter).

Best regards,
Krzysztof
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 82d6e374d82f..60341185a6b9 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -451,8 +451,8 @@  config STM32_DMA
 	  here.
 
 config S3C24XX_DMAC
-	bool "Samsung S3C24XX DMA support"
-	depends on ARCH_S3C24XX
+	bool "Samsung S3C24XX DMA support" if COMPILE_TEST && !ARCH_S3C24XX
+	default ARCH_S3C24XX
 	select DMA_ENGINE
 	select DMA_VIRTUAL_CHANNELS
 	help