Re: [PATCH] hwmon/sensors: fix SENSORS_LM75 dependencies

Zhang Rui
2014-01-08T02:03:13Z

On Tue, 2014-01-07 at 15:21 +0100, Jean Delvare wrote:
> Hi Eduardo,
>
> On Tue, 7 Jan 2014 08:23:43 -0400, Eduardo Valentin wrote:
> > On 07-01-2014 08:04, Jean Delvare wrote:
> > > On Mon, 06 Jan 2014 18:26:34 -0800, Guenter Roeck wrote:
> > >> This needs to be addressed
> > >> in the thermal code, for example with dummy declarations if THERMAL=m but
> > >> SENSORS_LM75=y. The functions are already declared as dummies if THERMAL_OF=n.
> > >
> > > This won't fly I'm afraid, the number of hwmon drivers affected will
> > > grow in the future and you certainly don't want to have to change the
> > > generic thermal code every time a new driver is added/converted.
> >
> > Agreed
> >
> > >
> > > I've looked at the problem this morning and I will admit I do not even
> > > understand what the problem is. In Randy's config, CONFIG_THERMAL_OF=y
> > > so both thermal_zone_of_sensor_unregister and
> > > thermal_zone_of_sensor_register are built-in. SENSORS_LM75=y so the
> > > calls to these functions are built-in too. I just can't see how this
> > > can be a problem at link time. Can anyone enlighten me?
> >
> > I believe the problem is more in the fact that THERMAL_OF is a bool, but
> > the way it is in thermal Kconfig, it will link to the thermal module, in
> > case CONFIG_THERMAL=m.
>
> Doh. I've been looking at this for an hour and managed to miss
> that :( Thanks for explaining.
>
> > Thus I am proposing the following,
> > which limits the user to have THERMAL_OF only as builtin and whenever
> > is selected, it will select THERMAL too. That is:
> >
> >
> > From b643aa260ed4f3514d1ca51b1ecbe4be7652a8d0 Mon Sep 17 00:00:00 2001
> > From: Eduardo Valentin <eduardo.valentin@ti.com>
> > Date: Tue, 7 Jan 2014 08:04:02 -0400
> > Subject: [PATCH 1/1] thermal: fix compilation issue on CONFIG_THERMAL_OF
> > dependencies
> >
> > Users of API provided by THERMAL_OF config may suffer when
> > CONFIG_THERMAL=y, causing linking issues, such as:
> >
> > drivers/built-in.o: In function `lm75_remove':
> > lm75.c:(.text+0x12bd8c): undefined reference to
> > `thermal_zone_of_sensor_unregister'
> > drivers/built-in.o: In function `lm75_probe':
> > lm75.c:(.text+0x12c123): undefined reference to
> > `thermal_zone_of_sensor_register'
> >
> > Therefore, this patch limits the compilation build to always
> > have THERMAL=y, whenever THERMAL_OF=y. In this way, whenever
> > the API user is built, if THERMAL_OF=y, the build shall have
> > the full thermal support, otherwise, the thermal API will provide
> > stubs.
> >
> > Cc: Zhang Rui <rui.zhang@intel.com>
> > Cc: linux-pm@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Eduardo Valentin <eduardo.valentin@ti.com>
> > ---
> > drivers/thermal/Kconfig | 29 ++++++++++++++++-------------
> > 1 file changed, 16 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> > index 58f98bd..dc315e9 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -29,19 +29,6 @@ config THERMAL_HWMON
> > Say 'Y' here if you want all thermal sensors to
> > have hwmon sysfs interface too.
> >
> > -config THERMAL_OF
> > - bool
> > - prompt "APIs to parse thermal data out of device tree"
> > - depends on OF
> > - default y
> > - help
> > - This options provides helpers to add the support to
> > - read and parse thermal data definitions out of the
> > - device tree blob.
> > -
> > - Say 'Y' here if you need to build thermal infrastructure
> > - based on device tree.
> > -
> > choice
> > prompt "Default Thermal governor"
> > default THERMAL_DEFAULT_GOV_STEP_WISE
> > @@ -235,3 +222,19 @@ source "drivers/thermal/samsung/Kconfig"
> > endmenu
> >
> > endif
> > +
> > +menuconfig THERMAL_OF
> > + bool
> > + prompt "APIs to parse thermal data out of device tree"
> > + depends on OF
> > + select THERMAL
> > + default y
> > + help
> > + This options enables DT thermal API which adds support to
> > + read and parse thermal data definitions out of the
> > + device tree blob. This option is mostly used by embedded
> > + thermal drivers.
> > +
> > + Say 'Y' here if you need to build thermal infrastructure
> > + based on device tree.
> > +
>
> I suppose this works, however I believe there is value in allowing for
> modular building of as much code as possible.
>
> I have an alternative proposal, which lets thermal be built as module,
> and hopefully also addresses the issue (I can't test...) The only
> drawback is that the same dependency must be added for every other
> hwmon driver which optionally uses THERMAL_OF.
>
> From: Jean Delvare <khali@linux-fr.org>
>
> Based on an earlier attempt by Randy Dunlap.
>
> Fix SENSORS_LM75 dependencies to eliminate build errors:
>
> drivers/built-in.o: In function `lm75_remove':
> lm75.c:(.text+0x12bd8c): undefined reference to `thermal_zone_of_sensor_unregister'
> drivers/built-in.o: In function `lm75_probe':
> lm75.c:(.text+0x12c123): undefined reference to `thermal_zone_of_sensor_register'
>
> Add depends on THERMAL since that is what provides the
> register/unregister functions above, but only if THERMAL_OF was
> selected as this is an optional feature of the driver.
>
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> Cc: Randy Dunlap <rdunlap@infradead.org>
> Cc: Eduardo Valentin <eduardo.valentin@ti.com>

Acked-by: Zhang Rui <rui.zhang@intel.com>

thanks,
rui
> ---
> drivers/hwmon/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> --- linux-3.13-rc7.orig/drivers/hwmon/Kconfig 2014-01-07 09:01:24.812848091 +0100
> +++ linux-3.13-rc7/drivers/hwmon/Kconfig 2014-01-07 15:19:11.039472329 +0100
> @@ -650,6 +650,7 @@ config SENSORS_LM73
> config SENSORS_LM75
> tristate "National Semiconductor LM75 and compatibles"
> depends on I2C
> + depends on THERMAL || !THERMAL_OF
> help
> If you say yes here you get support for one common type of
> temperature sensor chip, with models including:
>
>