| Message ID | 20211110023755.27176-1-rdunlap@infradead.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] iio/adc: ingenic: fix (MIPS) ingenic-adc build errors | expand |
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.
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. >
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.
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.
--- 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.
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(-)