[PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[Randy Dunlap]

MIPS does not always provide clk*() interfaces and there are no
always-present stubs for them, so depending on "MIPS || COMPILE_TEST"
is not strong enough to prevent build errors.

Likewise MACH_INGENIC_SOC || COMPILE_TEST is not strong enough
since if only COMPILE_TEST=y (with some other MIPS MACH_ or CPU or
BOARD setting), there are still the same build errors.

It looks like depending on MACH_INGENIC is the only thing that is
sufficient here in order to prevent build errors.

mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4770_adc_init_clk_div':
ingenic-adc.c:(.text+0xe4): undefined reference to `clk_get_parent'
mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4725b_adc_init_clk_div':
ingenic-adc.c:(.text+0x1b8): undefined reference to `clk_get_parent'

Fixes: 1a78daea107d ("IIO: add Ingenic JZ47xx ADC driver.")
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reported-by: kernel test robot <lkp@intel.com>
Cc: Artur Rojek <contact@artur-rojek.eu>
Cc: Paul Cercueil <paul@crapouillou.net>
Cc: linux-mips@vger.kernel.org
Cc: Jonathan Cameron <jic23@kernel.org>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: linux-iio@vger.kernel.org
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
---
v2: use MACH_INGENIC instead of MACH_INGENIC_SOC (thanks, Paul)

 drivers/iio/adc/Kconfig |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-next-20211105.orig/drivers/iio/adc/Kconfig
+++ linux-next-20211105/drivers/iio/adc/Kconfig
@@ -501,7 +501,7 @@ config INA2XX_ADC
 
 config INGENIC_ADC
 	tristate "Ingenic JZ47xx SoCs ADC driver"
-	depends on MIPS || COMPILE_TEST
+	depends on MACH_INGENIC
 	select IIO_BUFFER
 	help
 	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.

Re: [PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[Jonathan Cameron]

On Tue,  9 Nov 2021 18:37:55 -0800
Randy Dunlap <rdunlap@infradead.org> wrote:

> MIPS does not always provide clk*() interfaces and there are no
> always-present stubs for them, so depending on "MIPS || COMPILE_TEST"
> is not strong enough to prevent build errors.
> 
> Likewise MACH_INGENIC_SOC || COMPILE_TEST is not strong enough
> since if only COMPILE_TEST=y (with some other MIPS MACH_ or CPU or
> BOARD setting), there are still the same build errors.
> 
> It looks like depending on MACH_INGENIC is the only thing that is
> sufficient here in order to prevent build errors.
> 
> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4770_adc_init_clk_div':
> ingenic-adc.c:(.text+0xe4): undefined reference to `clk_get_parent'
> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4725b_adc_init_clk_div':
> ingenic-adc.c:(.text+0x1b8): undefined reference to `clk_get_parent'
> 
> Fixes: 1a78daea107d ("IIO: add Ingenic JZ47xx ADC driver.")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> Reported-by: kernel test robot <lkp@intel.com>
> Cc: Artur Rojek <contact@artur-rojek.eu>
> Cc: Paul Cercueil <paul@crapouillou.net>
> Cc: linux-mips@vger.kernel.org
> Cc: Jonathan Cameron <jic23@kernel.org>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: linux-iio@vger.kernel.org
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>

I'm a bit confused.  There are stubs in include/linux/clk.h for these.
Why do those not apply here? Are these platforms built with CONFIG_CLK but
don't provide all the functions?

That sounds highly error prone and rather defeats the object of the
stubs.  Could we either provide the missing stubs, or solve this some other
way.  I'm not keen to massively cut the build coverage this driver is getting
by dropping COMPILE_TEST if there is any route to avoid doing so.

Based on the guess than any platform with clks must be able to turn them on
I grepped for int clk_enable() and there seem to be only two possiblities
bcm63xx and lantiq as sources of the build breakage.

Jonathan

> ---
> v2: use MACH_INGENIC instead of MACH_INGENIC_SOC (thanks, Paul)
> 
>  drivers/iio/adc/Kconfig |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- linux-next-20211105.orig/drivers/iio/adc/Kconfig
> +++ linux-next-20211105/drivers/iio/adc/Kconfig
> @@ -501,7 +501,7 @@ config INA2XX_ADC
>  
>  config INGENIC_ADC
>  	tristate "Ingenic JZ47xx SoCs ADC driver"
> -	depends on MIPS || COMPILE_TEST
> +	depends on MACH_INGENIC
>  	select IIO_BUFFER
>  	help
>  	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.

Re: [PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[Randy Dunlap]

On 11/12/21 9:29 AM, Jonathan Cameron wrote:
> On Tue,  9 Nov 2021 18:37:55 -0800
> Randy Dunlap <rdunlap@infradead.org> wrote:
> 
>> MIPS does not always provide clk*() interfaces and there are no
>> always-present stubs for them, so depending on "MIPS || COMPILE_TEST"
>> is not strong enough to prevent build errors.
>>
>> Likewise MACH_INGENIC_SOC || COMPILE_TEST is not strong enough
>> since if only COMPILE_TEST=y (with some other MIPS MACH_ or CPU or
>> BOARD setting), there are still the same build errors.
>>
>> It looks like depending on MACH_INGENIC is the only thing that is
>> sufficient here in order to prevent build errors.
>>
>> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4770_adc_init_clk_div':
>> ingenic-adc.c:(.text+0xe4): undefined reference to `clk_get_parent'
>> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4725b_adc_init_clk_div':
>> ingenic-adc.c:(.text+0x1b8): undefined reference to `clk_get_parent'
>>
>> Fixes: 1a78daea107d ("IIO: add Ingenic JZ47xx ADC driver.")
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Cc: Artur Rojek <contact@artur-rojek.eu>
>> Cc: Paul Cercueil <paul@crapouillou.net>
>> Cc: linux-mips@vger.kernel.org
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: linux-iio@vger.kernel.org
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
> I'm a bit confused.  There are stubs in include/linux/clk.h for these.
> Why do those not apply here? Are these platforms built with CONFIG_CLK but
> don't provide all the functions?
> 
> That sounds highly error prone and rather defeats the object of the
> stubs.  Could we either provide the missing stubs, or solve this some other
> way.  I'm not keen to massively cut the build coverage this driver is getting
> by dropping COMPILE_TEST if there is any route to avoid doing so.

I'm all for that (above), but it's a mess.

> Based on the guess than any platform with clks must be able to turn them on
> I grepped for int clk_enable() and there seem to be only two possiblities
> bcm63xx and lantiq as sources of the build breakage.

CONFIG_BCM63XX=y
# CONFIG_MACH_INGENIC_SOC is not set
CONFIG_INGENIC_ADC=y
CONFIG_HAVE_CLK=y


According to the build error messages (above), clk_get_parent()
is missing. Looking at <linux/clk.h>, for CONFIG_HAVE_CLK=y,
there is a prototype for clk_get_parent(), and if CONFIG_HAVE_CLK
is not set, there is a stub for it.

Now look at drivers/clk/clk.c and drivers/clk/Makefile:

clk_get_parent() is defined in clk.c, which is built when
CONFIG_COMMON_CLK=y, but that is not set in this .config file.

CONFIG_HAVE_CLK=y, but that doesn't get clk_get_parent()
compiled.

So to me it is a disparity or incongruity between HAVE_CLK and COMMON_CLK.

Russell- do you have any suggestions for how to straighten
this out?


> Jonathan
> 
>> ---
>> v2: use MACH_INGENIC instead of MACH_INGENIC_SOC (thanks, Paul)
>>
>>   drivers/iio/adc/Kconfig |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- linux-next-20211105.orig/drivers/iio/adc/Kconfig
>> +++ linux-next-20211105/drivers/iio/adc/Kconfig
>> @@ -501,7 +501,7 @@ config INA2XX_ADC
>>   
>>   config INGENIC_ADC
>>   	tristate "Ingenic JZ47xx SoCs ADC driver"
>> -	depends on MIPS || COMPILE_TEST
>> +	depends on MACH_INGENIC
>>   	select IIO_BUFFER
>>   	help
>>   	  Say yes here to build support for the Ingenic JZ47xx SoCs ADC unit.
> 


-- 
~Randy

Re: [PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[Russell King (Oracle)]

On Fri, Nov 12, 2021 at 04:39:04PM -0800, Randy Dunlap wrote:
> On 11/12/21 9:29 AM, Jonathan Cameron wrote:
> > On Tue,  9 Nov 2021 18:37:55 -0800
> > Randy Dunlap <rdunlap@infradead.org> wrote:
> > 
> > > MIPS does not always provide clk*() interfaces and there are no
> > > always-present stubs for them, so depending on "MIPS || COMPILE_TEST"
> > > is not strong enough to prevent build errors.
> > > 
> > > Likewise MACH_INGENIC_SOC || COMPILE_TEST is not strong enough
> > > since if only COMPILE_TEST=y (with some other MIPS MACH_ or CPU or
> > > BOARD setting), there are still the same build errors.
> > > 
> > > It looks like depending on MACH_INGENIC is the only thing that is
> > > sufficient here in order to prevent build errors.
> > > 
> > > mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4770_adc_init_clk_div':
> > > ingenic-adc.c:(.text+0xe4): undefined reference to `clk_get_parent'
> > > mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4725b_adc_init_clk_div':
> > > ingenic-adc.c:(.text+0x1b8): undefined reference to `clk_get_parent'
> > > 
> > > Fixes: 1a78daea107d ("IIO: add Ingenic JZ47xx ADC driver.")
> > > Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Cc: Artur Rojek <contact@artur-rojek.eu>
> > > Cc: Paul Cercueil <paul@crapouillou.net>
> > > Cc: linux-mips@vger.kernel.org
> > > Cc: Jonathan Cameron <jic23@kernel.org>
> > > Cc: Lars-Peter Clausen <lars@metafoo.de>
> > > Cc: linux-iio@vger.kernel.org
> > > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > 
> > I'm a bit confused.  There are stubs in include/linux/clk.h for these.
> > Why do those not apply here? Are these platforms built with CONFIG_CLK but
> > don't provide all the functions?
> > 
> > That sounds highly error prone and rather defeats the object of the
> > stubs.  Could we either provide the missing stubs, or solve this some other
> > way.  I'm not keen to massively cut the build coverage this driver is getting
> > by dropping COMPILE_TEST if there is any route to avoid doing so.
> 
> I'm all for that (above), but it's a mess.
> 
> > Based on the guess than any platform with clks must be able to turn them on
> > I grepped for int clk_enable() and there seem to be only two possiblities
> > bcm63xx and lantiq as sources of the build breakage.
> 
> CONFIG_BCM63XX=y
> # CONFIG_MACH_INGENIC_SOC is not set
> CONFIG_INGENIC_ADC=y
> CONFIG_HAVE_CLK=y
> 
> 
> According to the build error messages (above), clk_get_parent()
> is missing. Looking at <linux/clk.h>, for CONFIG_HAVE_CLK=y,
> there is a prototype for clk_get_parent(), and if CONFIG_HAVE_CLK
> is not set, there is a stub for it.
> 
> Now look at drivers/clk/clk.c and drivers/clk/Makefile:
> 
> clk_get_parent() is defined in clk.c, which is built when
> CONFIG_COMMON_CLK=y, but that is not set in this .config file.
> 
> CONFIG_HAVE_CLK=y, but that doesn't get clk_get_parent()
> compiled.
> 
> So to me it is a disparity or incongruity between HAVE_CLK and COMMON_CLK.

HAVE_CLK means we have the clk API implemented. COMMON_CLK is one such
implementation, and HAVE_LEGACY_CLK is another group of implementations.

BCM63XX has its own implementation and uses HAVE_LEGACY_CLK, which can
be found in arch/mips/bcm63xx/clk.c.

If it doesn't support parent clocks, then it should provide a stub
clk_get_parent() that returns NULL at the very least.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors

[Randy Dunlap]

On 11/13/21 12:34 AM, Russell King (Oracle) wrote:
> On Fri, Nov 12, 2021 at 04:39:04PM -0800, Randy Dunlap wrote:
>> On 11/12/21 9:29 AM, Jonathan Cameron wrote:
>>> On Tue,  9 Nov 2021 18:37:55 -0800
>>> Randy Dunlap <rdunlap@infradead.org> wrote:
>>>
>>>> MIPS does not always provide clk*() interfaces and there are no
>>>> always-present stubs for them, so depending on "MIPS || COMPILE_TEST"
>>>> is not strong enough to prevent build errors.
>>>>
>>>> Likewise MACH_INGENIC_SOC || COMPILE_TEST is not strong enough
>>>> since if only COMPILE_TEST=y (with some other MIPS MACH_ or CPU or
>>>> BOARD setting), there are still the same build errors.
>>>>
>>>> It looks like depending on MACH_INGENIC is the only thing that is
>>>> sufficient here in order to prevent build errors.
>>>>
>>>> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4770_adc_init_clk_div':
>>>> ingenic-adc.c:(.text+0xe4): undefined reference to `clk_get_parent'
>>>> mips-linux-ld: drivers/iio/adc/ingenic-adc.o: in function `jz4725b_adc_init_clk_div':
>>>> ingenic-adc.c:(.text+0x1b8): undefined reference to `clk_get_parent'
>>>>
>>>> Fixes: 1a78daea107d ("IIO: add Ingenic JZ47xx ADC driver.")
>>>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>>>> Reported-by: kernel test robot <lkp@intel.com>
>>>> Cc: Artur Rojek <contact@artur-rojek.eu>
>>>> Cc: Paul Cercueil <paul@crapouillou.net>
>>>> Cc: linux-mips@vger.kernel.org
>>>> Cc: Jonathan Cameron <jic23@kernel.org>
>>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>>>> Cc: linux-iio@vger.kernel.org
>>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>>> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
>>>
>>> I'm a bit confused.  There are stubs in include/linux/clk.h for these.
>>> Why do those not apply here? Are these platforms built with CONFIG_CLK but
>>> don't provide all the functions?
>>>
>>> That sounds highly error prone and rather defeats the object of the
>>> stubs.  Could we either provide the missing stubs, or solve this some other
>>> way.  I'm not keen to massively cut the build coverage this driver is getting
>>> by dropping COMPILE_TEST if there is any route to avoid doing so.
>>
>> I'm all for that (above), but it's a mess.
>>
>>> Based on the guess than any platform with clks must be able to turn them on
>>> I grepped for int clk_enable() and there seem to be only two possiblities
>>> bcm63xx and lantiq as sources of the build breakage.
>>
>> CONFIG_BCM63XX=y
>> # CONFIG_MACH_INGENIC_SOC is not set
>> CONFIG_INGENIC_ADC=y
>> CONFIG_HAVE_CLK=y
>>
>>
>> According to the build error messages (above), clk_get_parent()
>> is missing. Looking at <linux/clk.h>, for CONFIG_HAVE_CLK=y,
>> there is a prototype for clk_get_parent(), and if CONFIG_HAVE_CLK
>> is not set, there is a stub for it.
>>
>> Now look at drivers/clk/clk.c and drivers/clk/Makefile:
>>
>> clk_get_parent() is defined in clk.c, which is built when
>> CONFIG_COMMON_CLK=y, but that is not set in this .config file.
>>
>> CONFIG_HAVE_CLK=y, but that doesn't get clk_get_parent()
>> compiled.
>>
>> So to me it is a disparity or incongruity between HAVE_CLK and COMMON_CLK.
> 
> HAVE_CLK means we have the clk API implemented. COMMON_CLK is one such
> implementation, and HAVE_LEGACY_CLK is another group of implementations.
> 
> BCM63XX has its own implementation and uses HAVE_LEGACY_CLK, which can
> be found in arch/mips/bcm63xx/clk.c.
> 
> If it doesn't support parent clocks, then it should provide a stub
> clk_get_parent() that returns NULL at the very least.
> 

Russell- thanks for the explanation.
That works nicely.

Jonathan, I'll send a different patch.

-- 
~Randy