On Sun, Apr 24, 2011 at 03:40:53PM -0400, Jean Delvare wrote: > Hi Guenter, > > On Sat, 23 Apr 2011 18:58:53 -0700, Guenter Roeck wrote: > > On Sat, Apr 23, 2011 at 09:07:03AM -0400, Jean Delvare wrote: > > > It's about time to revert 16d752397301b95abaa95cbaf9e785d221872311. > > > Anybody running a kernel >= 2.6.40 would also be running a recent > > > enough version of lm-sensors. > > > > > > Signed-off-by: Jean Delvare <khali@xxxxxxxxxxxx> > > > Cc: Rene Herman <rene.herman@xxxxxxxxx> > > > Cc: Len Brown <len.brown@xxxxxxxxx> > > > > Two other and less complex options: > > > > - Add > > select THERMAL_HWMON if HWMON=y || HWMON=THERMAL > > to "menuconfig THERMAL", and keep THERMAL_HWMON as non-selectable bool > > [ I don't know what HWMON=THERMAL means, though ] > > HWMON=THERMAL means: both HWMON and THERMAL built as modules, or both > built into the kernel, or neither built at all. Given that the "y" case > is already mentioned explicitly, and the whole thing isn't even > evaluated if THERMAL=n, HWMON=THERMAL here is essentially a shortcut > for "THERMAL=m && HWMON=m". > > > - Add > > select HWMON > > to "menuconfig THERMAL", and remove THERMAL_HWMON. > > Thanks for the suggestions. I admit I blindly reverted the original > commit without putting much thought into possible alternatives. > > Your second suggestion is by far the most simple. However it has a > drawback, it makes it impossible to select THERMAL without HWMON. This > is no concern to me, but maybe some embedded systems will be unhappy > about this. HWMON has no interest in the absence of local users. > Yes, that is why I suggested it as second option. I don't think the assertion is correct, though. There could well be a SNMP hardware monitoring MIB implementation (or something similar) using the hwmon sysfs ABI to interface with the kernel; such an interface to the outside world would make a lot of sense in an embedded system. > Your first suggestion makes a lot of sense, as it avoids repeating the > same non-trivial conditional construct in 3 different places. But OTOH > it leaves in Kconfig an option that shouldn't be there, as nobody > should depend on its value. I consider it an abuse of the Kconfig > system. > Not really; that kind of construct is used all over the place. Sure, it is commonly used if the same option is auto-enabled from more than one other options, but in my opinion it makes sense here as well: If one wants to change the condition for enabling THERMAL_HWMON, it is easier to change in one place instead of having to track down all the places in the code. Yet another possibility, which is used all over the place, would be config THERMAL_HWMON bool depends on THERMAL depends on HWMON=y || HWMON=THERMAL default y ie remove the ability to select it manually. Maybe that would be more appropriate. > In fact, my impression is that the repeated tests, and the fact that two > of them are in <linux/thermal.h>, are the consequence of bad code > design. The fact that THERMAL_HWMON affects a public structure when the > implementation is completely internal to the thermal driver makes no > sense. It should be possible to implement things differently. > Yes, it definitely violates the notion of "no ifdefs in the code". It might be more difficult to get that kind of code into the kernel today. Thanks, Guenter _______________________________________________ lm-sensors mailing list lm-sensors@xxxxxxxxxxxxxx http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
