| Message ID | 1445003052-31373-4-git-send-email-sre@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Friday 16 October 2015 15:44:12 Sebastian Reichel wrote: > If BQ27XXX is enabled, but neither I2C based initialization, nor > platform based initialization are activated, bq27xxx_powersupply_init > and bq27xxx_powersupply_unregister are defined but not used. > > This configuration doesn't make sense, but there is no easy way > to make it unavailable in the build system, so just mark the > functions as __maybe_unused instead. > What about? #if defined(CONFIG_1) || defined(CONFIG_2) #define NEED_POWER_SUPPLY #endif And then wrap power supply code into #ifdef NEED_POWER_SUPPLY?
On 10/16/2015 09:47 AM, Pali Rohár wrote: > On Friday 16 October 2015 15:44:12 Sebastian Reichel wrote: >> If BQ27XXX is enabled, but neither I2C based initialization, nor >> platform based initialization are activated, bq27xxx_powersupply_init >> and bq27xxx_powersupply_unregister are defined but not used. >> >> This configuration doesn't make sense, but there is no easy way >> to make it unavailable in the build system, so just mark the >> functions as __maybe_unused instead. >> > > What about? > > #if defined(CONFIG_1) || defined(CONFIG_2) > #define NEED_POWER_SUPPLY > #endif > > And then wrap power supply code into #ifdef NEED_POWER_SUPPLY? > We would need to wrap everything in that then, so without I2C or platform enabled we end up compiling an empty file. I think the better fix would be to modify the Kconfig, something like: config BATTERY_BQ27XXX tristate depends on (BATTERY_BQ27XXX_I2C || BATTERY_BQ27XXX_PLATFORM) default y config BATTERY_BQ27XXX_I2C bool "BQ27200/BQ27500 support" depends on I2C help Say Y here to enable support for batteries with BQ27x00 (I2C) chips. config BATTERY_BQ27XXX_PLATFORM bool "BQ27000 support" help Say Y here to enable support for batteries with BQ27000 (HDQ) chips. This would be more in line with how other multi-bus devices handle this issue when nether bus is selected.
On Fri, Oct 16, 2015 at 10:01:33AM -0500, Andrew F. Davis wrote: > On 10/16/2015 09:47 AM, Pali Rohár wrote: > >On Friday 16 October 2015 15:44:12 Sebastian Reichel wrote: > >>If BQ27XXX is enabled, but neither I2C based initialization, nor > >>platform based initialization are activated, bq27xxx_powersupply_init > >>and bq27xxx_powersupply_unregister are defined but not used. > >> > >>This configuration doesn't make sense, but there is no easy way > >>to make it unavailable in the build system, so just mark the > >>functions as __maybe_unused instead. > >> > > > >What about? > > > >#if defined(CONFIG_1) || defined(CONFIG_2) > >#define NEED_POWER_SUPPLY > >#endif > > > >And then wrap power supply code into #ifdef NEED_POWER_SUPPLY? I thought about wrapping the whole code in an ifdef, but I don't think compiling an empty file is any better than adding the __maybe_unused flag. > We would need to wrap everything in that then, so without I2C or > platform enabled we end up compiling an empty file. I think the > better fix would be to modify the Kconfig, something like: Handling this in Kconfig is preferred of course. > config BATTERY_BQ27XXX > tristate > depends on (BATTERY_BQ27XXX_I2C || BATTERY_BQ27XXX_PLATFORM) > default y > > config BATTERY_BQ27XXX_I2C > bool "BQ27200/BQ27500 support" > depends on I2C > help > Say Y here to enable support for batteries with BQ27x00 (I2C) chips. > > config BATTERY_BQ27XXX_PLATFORM > bool "BQ27000 support" > help > Say Y here to enable support for batteries with BQ27000 (HDQ) chips. > > This would be more in line with how other multi-bus devices handle > this issue when nether bus is selected. If I'm not mistaken, this way it's impossible to build the code as module. You need to move the tristate to the individual bus options. This would require to split the code, so that two separate modules are built. I checked the tree and found a few examples: * building one modules for each bus, core support is autoselected, but always built-in - BMP085 * building a core modules + one module for each bus - AD525X_DPOT - ADE7854 - ADT7316 - INPUT_AD714X - INPUT_ADXL34X - TOUCHSCREEN_AD7879 - TOUCHSCREEN_CYTTSP_CORE - TOUCHSCREEN_CYTTSP4_CORE Instead of restructuring the whole driver, we could also just built the platform code unconditionally. The platform specific code is just a probe + remove function and bq27xxx_battery_platform_read(). The last one could be dropped, if the pdata->read function is modified (effectively moving the code into the w1 driver). -- Sebastian
On 10/19/2015 04:37 AM, Sebastian Reichel wrote: > On Fri, Oct 16, 2015 at 10:01:33AM -0500, Andrew F. Davis wrote: >> On 10/16/2015 09:47 AM, Pali Rohár wrote: >>> On Friday 16 October 2015 15:44:12 Sebastian Reichel wrote: >>>> If BQ27XXX is enabled, but neither I2C based initialization, nor >>>> platform based initialization are activated, bq27xxx_powersupply_init >>>> and bq27xxx_powersupply_unregister are defined but not used. >>>> >>>> This configuration doesn't make sense, but there is no easy way >>>> to make it unavailable in the build system, so just mark the >>>> functions as __maybe_unused instead. >>>> >>> >>> What about? >>> >>> #if defined(CONFIG_1) || defined(CONFIG_2) >>> #define NEED_POWER_SUPPLY >>> #endif >>> >>> And then wrap power supply code into #ifdef NEED_POWER_SUPPLY? > > I thought about wrapping the whole code in an ifdef, but I don't > think compiling an empty file is any better than adding the > __maybe_unused flag. > >> We would need to wrap everything in that then, so without I2C or >> platform enabled we end up compiling an empty file. I think the >> better fix would be to modify the Kconfig, something like: > > Handling this in Kconfig is preferred of course. > >> config BATTERY_BQ27XXX >> tristate >> depends on (BATTERY_BQ27XXX_I2C || BATTERY_BQ27XXX_PLATFORM) >> default y >> >> config BATTERY_BQ27XXX_I2C >> bool "BQ27200/BQ27500 support" >> depends on I2C >> help >> Say Y here to enable support for batteries with BQ27x00 (I2C) chips. >> >> config BATTERY_BQ27XXX_PLATFORM >> bool "BQ27000 support" >> help >> Say Y here to enable support for batteries with BQ27000 (HDQ) chips. >> >> This would be more in line with how other multi-bus devices handle >> this issue when nether bus is selected. > > If I'm not mistaken, this way it's impossible to build the code as > module. You need to move the tristate to the individual bus options. > This would require to split the code, so that two separate modules > are built. I checked the tree and found a few examples: > > * building one modules for each bus, core support is autoselected, but always built-in > - BMP085 > > * building a core modules + one module for each bus > - AD525X_DPOT > - ADE7854 > - ADT7316 > - INPUT_AD714X > - INPUT_ADXL34X > - TOUCHSCREEN_AD7879 > - TOUCHSCREEN_CYTTSP_CORE > - TOUCHSCREEN_CYTTSP4_CORE > > Instead of restructuring the whole driver, we could also just built > the platform code unconditionally. The platform specific code is > just a probe + remove function and bq27xxx_battery_platform_read(). > The last one could be dropped, if the pdata->read function is > modified (effectively moving the code into the w1 driver). > > -- Sebastian > A quick test shows the following Kconfig to work as desired, selecting one or both buses as modules builds the driver as a module, if ether are selected as built-in the whole driver is built-in. config BATTERY_BQ27XXX tristate depends on (BATTERY_BQ27XXX_I2C || BATTERY_BQ27XXX_PLATFORM) default y config BATTERY_BQ27XXX_I2C tristate "BQ27xxx I2C support" depends on I2C help Say Y here to enable support for batteries with BQ27xxx (I2C) chips. config BATTERY_BQ27XXX_PLATFORM tristate "BQ27xxx HDQ support" help Say Y here to enable support for batteries with BQ27xxx (HDQ) chips. We would then have to change: #ifdef CONFIG_BATTERY_BQ27XXX_I2C to #if defined(CONFIG_BATTERY_BQ27XXX_I2C) || defined(CONFIG_BATTERY_BQ27XXX_I2C_MODULE) (same for CONFIG_BATTERY_BQ27XXX_PLATFORM)
diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c index 880233ce9343..452c9f079e0d 100644 --- a/drivers/power/bq27xxx_battery.c +++ b/drivers/power/bq27xxx_battery.c @@ -991,7 +991,7 @@ static void bq27xxx_external_power_changed(struct power_supply *psy) schedule_delayed_work(&di->work, 0); } -static int bq27xxx_powersupply_init(struct bq27xxx_device_info *di, +static int __maybe_unused bq27xxx_powersupply_init(struct bq27xxx_device_info *di, const char *name) { int ret; @@ -1026,7 +1026,7 @@ static int bq27xxx_powersupply_init(struct bq27xxx_device_info *di, return 0; } -static void bq27xxx_powersupply_unregister(struct bq27xxx_device_info *di) +static void __maybe_unused bq27xxx_powersupply_unregister(struct bq27xxx_device_info *di) { /* * power_supply_unregister call bq27xxx_battery_get_property which
If BQ27XXX is enabled, but neither I2C based initialization, nor platform based initialization are activated, bq27xxx_powersupply_init and bq27xxx_powersupply_unregister are defined but not used. This configuration doesn't make sense, but there is no easy way to make it unavailable in the build system, so just mark the functions as __maybe_unused instead. Signed-off-by: Sebastian Reichel <sre@kernel.org> --- drivers/power/bq27xxx_battery.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)