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

[Jean Delvare]

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@xxxxxx>
> 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@xxxxxxxxx>
> Cc: linux-pm@xxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Signed-off-by: Eduardo Valentin <eduardo.valentin@xxxxxx>
> ---
>  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@xxxxxxxxxxxx>

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@xxxxxxxxxxxx>
Cc: Randy Dunlap <rdunlap@xxxxxxxxxxxxx>
Cc: Eduardo Valentin <eduardo.valentin@xxxxxx>
---
 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:


-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/