diff mbox series

[1/2] gpio: omap: allow building the module with COMPILE_TEST=y

Message ID 20241203164143.29852-1-brgl@bgdev.pl
State New
Headers show
Series [1/2] gpio: omap: allow building the module with COMPILE_TEST=y | expand

Commit Message

Bartosz Golaszewski Dec. 3, 2024, 4:41 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

For better build coverage, allow building the gpio-omap driver with
COMPILE_TEST Kconfig option enabled.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/gpio/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

A. Sverdlin Dec. 3, 2024, 6:18 p.m. UTC | #1 
On Tue, 2024-12-03 at 17:41 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better build coverage, allow building the gpio-omap driver with
> COMPILE_TEST Kconfig option enabled.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7..fb923ccd79028 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>  config GPIO_OMAP
>  	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>  	default y if ARCH_OMAP
> -	depends on ARM
> +	depends on ARM || COMPILE_TEST
>  	select GENERIC_IRQ_CHIP
>  	select GPIOLIB_IRQCHIP
>  	help
A. Sverdlin Dec. 3, 2024, 6:19 p.m. UTC | #2 
On Tue, 2024-12-03 at 17:41 +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We can drop the else branch if we get the clock already prepared using
> the relevant helper.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  drivers/gpio/gpio-omap.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 54c4bfdccf568..57d299d5d0b16 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -1449,13 +1449,11 @@ static int omap_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	if (bank->dbck_flag) {
> -		bank->dbck = devm_clk_get(dev, "dbclk");
> +		bank->dbck = devm_clk_get_prepared(dev, "dbclk");
>  		if (IS_ERR(bank->dbck)) {
>  			dev_err(dev,
>  				"Could not get gpio dbck. Disable debounce\n");
>  			bank->dbck_flag = false;
> -		} else {
> -			clk_prepare(bank->dbck);
>  		}
>  	}
Andrew Davis Dec. 3, 2024, 6:41 p.m. UTC | #3 
On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> For better build coverage, allow building the gpio-omap driver with
> COMPILE_TEST Kconfig option enabled.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>   drivers/gpio/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 56fee58e281e7..fb923ccd79028 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>   config GPIO_OMAP
>   	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>   	default y if ARCH_OMAP
> -	depends on ARM
> +	depends on ARM || COMPILE_TEST

Why do we have this depends on ARM at all? It already has that condition
above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.

And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
could just do this:

--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -528,9 +528,9 @@ config GPIO_OCTEON
           family of SOCs.
  
  config GPIO_OMAP
-       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
-       default y if ARCH_OMAP
-       depends on ARM
+       tristate "TI OMAP GPIO support"
+       default y
+       depends on ARCH_OMAP2PLUS || COMPILE_TEST
         select GENERIC_IRQ_CHIP
         select GPIOLIB_IRQCHIP
         help

Andrew

>   	select GENERIC_IRQ_CHIP
>   	select GPIOLIB_IRQCHIP
>   	help
Bartosz Golaszewski Dec. 3, 2024, 8:36 p.m. UTC | #4 
On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
>
> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > For better build coverage, allow building the gpio-omap driver with
> > COMPILE_TEST Kconfig option enabled.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >   drivers/gpio/Kconfig | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 56fee58e281e7..fb923ccd79028 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -530,7 +530,7 @@ config GPIO_OCTEON
> >   config GPIO_OMAP
> >       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >       default y if ARCH_OMAP
> > -     depends on ARM
> > +     depends on ARM || COMPILE_TEST
>
> Why do we have this depends on ARM at all? It already has that condition
> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
>
> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
> could just do this:
>

I agree we can drop that bit.

> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -528,9 +528,9 @@ config GPIO_OCTEON
>            family of SOCs.
>
>   config GPIO_OMAP
> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> -       default y if ARCH_OMAP
> -       depends on ARM
> +       tristate "TI OMAP GPIO support"
> +       default y
> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST

This would default to y with COMPILE_TEST. We definitely don't want
that. IMO it should be:

tristate "TI OMAP GPIO support"
depends on ARCH_OMAP2PLUS || COMPILE_TEST
default y if ARCH_OMAP2PLUS

Bartosz
Andrew Davis Dec. 3, 2024, 9:54 p.m. UTC | #5 
On 12/3/24 2:36 PM, Bartosz Golaszewski wrote:
> On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
>>
>> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
>>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>>
>>> For better build coverage, allow building the gpio-omap driver with
>>> COMPILE_TEST Kconfig option enabled.
>>>
>>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>>> ---
>>>    drivers/gpio/Kconfig | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>>> index 56fee58e281e7..fb923ccd79028 100644
>>> --- a/drivers/gpio/Kconfig
>>> +++ b/drivers/gpio/Kconfig
>>> @@ -530,7 +530,7 @@ config GPIO_OCTEON
>>>    config GPIO_OMAP
>>>        tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>>>        default y if ARCH_OMAP
>>> -     depends on ARM
>>> +     depends on ARM || COMPILE_TEST
>>
>> Why do we have this depends on ARM at all? It already has that condition
>> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
>>
>> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
>> could just do this:
>>
> 
> I agree we can drop that bit.
> 
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -528,9 +528,9 @@ config GPIO_OCTEON
>>             family of SOCs.
>>
>>    config GPIO_OMAP
>> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
>> -       default y if ARCH_OMAP
>> -       depends on ARM
>> +       tristate "TI OMAP GPIO support"
>> +       default y
>> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST
> 
> This would default to y with COMPILE_TEST. We definitely don't want
> that. IMO it should be:
> 
> tristate "TI OMAP GPIO support"
> depends on ARCH_OMAP2PLUS || COMPILE_TEST
> default y if ARCH_OMAP2PLUS
> 

Looks good to me

Andrew
Bartosz Golaszewski Dec. 5, 2024, 10:27 a.m. UTC | #6 
On Tue, Dec 3, 2024 at 10:54 PM Andrew Davis <afd@ti.com> wrote:
>
> On 12/3/24 2:36 PM, Bartosz Golaszewski wrote:
> > On Tue, Dec 3, 2024 at 7:41 PM Andrew Davis <afd@ti.com> wrote:
> >>
> >> On 12/3/24 10:41 AM, Bartosz Golaszewski wrote:
> >>> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>>
> >>> For better build coverage, allow building the gpio-omap driver with
> >>> COMPILE_TEST Kconfig option enabled.
> >>>
> >>> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >>> ---
> >>>    drivers/gpio/Kconfig | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> >>> index 56fee58e281e7..fb923ccd79028 100644
> >>> --- a/drivers/gpio/Kconfig
> >>> +++ b/drivers/gpio/Kconfig
> >>> @@ -530,7 +530,7 @@ config GPIO_OCTEON
> >>>    config GPIO_OMAP
> >>>        tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >>>        default y if ARCH_OMAP
> >>> -     depends on ARM
> >>> +     depends on ARM || COMPILE_TEST
> >>
> >> Why do we have this depends on ARM at all? It already has that condition
> >> above on ARCH_OMAP2PLUS which limits to ARM outside of compile testing.
> >>
> >> And anything that selects ARCH_OMAP2PLUS also selects ARCH_OMAP, so we
> >> could just do this:
> >>
> >
> > I agree we can drop that bit.
> >
> >> --- a/drivers/gpio/Kconfig
> >> +++ b/drivers/gpio/Kconfig
> >> @@ -528,9 +528,9 @@ config GPIO_OCTEON
> >>             family of SOCs.
> >>
> >>    config GPIO_OMAP
> >> -       tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
> >> -       default y if ARCH_OMAP
> >> -       depends on ARM
> >> +       tristate "TI OMAP GPIO support"
> >> +       default y
> >> +       depends on ARCH_OMAP2PLUS || COMPILE_TEST
> >
> > This would default to y with COMPILE_TEST. We definitely don't want
> > that. IMO it should be:
> >
> > tristate "TI OMAP GPIO support"
> > depends on ARCH_OMAP2PLUS || COMPILE_TEST
> > default y if ARCH_OMAP2PLUS
> >
>
> Looks good to me
>
> Andrew

Nah, this is incorrect, it doesn't build gpio-omap for
omap1_defconfig. I has to depend on ARCH_OMAP under which all omap
platforms fall and they all use this driver.

Bart
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 56fee58e281e7..fb923ccd79028 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -530,7 +530,7 @@  config GPIO_OCTEON
 config GPIO_OMAP
 	tristate "TI OMAP GPIO support" if ARCH_OMAP2PLUS || COMPILE_TEST
 	default y if ARCH_OMAP
-	depends on ARM
+	depends on ARM || COMPILE_TEST
 	select GENERIC_IRQ_CHIP
 	select GPIOLIB_IRQCHIP
 	help