[PATCH 0/3] power: bq27xxx: Fix multiple defined but not used warnings

[PATCH 0/3] power: bq27xxx: Fix multiple defined but not used warnings

[Sebastian Reichel]

Hi,

These patches fix multiple "defined but not used" warnings
in the bq27xxx driver.

-- Sebastian

Sebastian Reichel (3):
  power: bq27xxx_battery: fix platform probe
  power: bq27xxx_battery: move irq handler to i2c section
  power: bq27xxx_battery: fix defined but not used warnings

 drivers/power/bq27xxx_battery.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

-- 
2.6.1

[PATCH 1/3] power: bq27xxx_battery: fix platform probe

[Sebastian Reichel]

Add missing initialization of register mapping table to
platform probe function.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/bq27xxx_battery.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
index 994c78d9f426..69e6b371a51c 100644
--- a/drivers/power/bq27xxx_battery.c
+++ b/drivers/power/bq27xxx_battery.c
@@ -1292,6 +1292,7 @@ static int bq27xxx_battery_platform_probe(struct platform_device *pdev)
 
 	di->dev = &pdev->dev;
 	di->chip = pdata->chip;
+	di->regs = bq27xxx_regs[di->chip];
 
 	name = pdata->name ?: dev_name(&pdev->dev);
 	di->bus.read = &bq27xxx_battery_platform_read;
-- 
2.6.1

[PATCH 2/3] power: bq27xxx_battery: move irq handler to i2c section

[Sebastian Reichel]

The IRQ handler is not used by the platform based
code resulting in a 'defined but not used' warning,
if CONFIG_BQ27XXX_I2C is not enabled.

Signed-off-by: Sebastian Reichel <sre@kernel.org>
---
 drivers/power/bq27xxx_battery.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
index 69e6b371a51c..880233ce9343 100644
--- a/drivers/power/bq27xxx_battery.c
+++ b/drivers/power/bq27xxx_battery.c
@@ -762,15 +762,6 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
 	di->last_update = jiffies;
 }
 
-static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
-{
-	struct bq27xxx_device_info *di = data;
-
-	bq27xxx_battery_update(di);
-
-	return IRQ_HANDLED;
-}
-
 static void bq27xxx_battery_poll(struct work_struct *work)
 {
 	struct bq27xxx_device_info *di =
@@ -1061,6 +1052,15 @@ static void bq27xxx_powersupply_unregister(struct bq27xxx_device_info *di)
 static DEFINE_IDR(battery_id);
 static DEFINE_MUTEX(battery_mutex);
 
+static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
+{
+	struct bq27xxx_device_info *di = data;
+
+	bq27xxx_battery_update(di);
+
+	return IRQ_HANDLED;
+}
+
 static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 				    bool single)
 {
-- 
2.6.1

[PATCH 3/3] power: bq27xxx_battery: fix defined but not used warnings

[Sebastian Reichel]

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(-)

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
-- 
2.6.1

Re: [PATCH 3/3] power: bq27xxx_battery: fix defined but not used warnings

[Pali Rohár]

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?

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/3] power: bq27xxx_battery: fix platform probe

[Pali Rohár]

On Friday 16 October 2015 15:44:10 Sebastian Reichel wrote:
> Add missing initialization of register mapping table to
> platform probe function.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/power/bq27xxx_battery.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> index 994c78d9f426..69e6b371a51c 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -1292,6 +1292,7 @@ static int bq27xxx_battery_platform_probe(struct platform_device *pdev)
>  
>  	di->dev = &pdev->dev;
>  	di->chip = pdata->chip;
> +	di->regs = bq27xxx_regs[di->chip];
>  
>  	name = pdata->name ?: dev_name(&pdev->dev);
>  	di->bus.read = &bq27xxx_battery_platform_read;

Acked-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 2/3] power: bq27xxx_battery: move irq handler to i2c section

[Pali Rohár]

On Friday 16 October 2015 15:44:11 Sebastian Reichel wrote:
> The IRQ handler is not used by the platform based
> code resulting in a 'defined but not used' warning,
> if CONFIG_BQ27XXX_I2C is not enabled.
> 
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>  drivers/power/bq27xxx_battery.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> index 69e6b371a51c..880233ce9343 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -762,15 +762,6 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
>  	di->last_update = jiffies;
>  }
>  
> -static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
> -{
> -	struct bq27xxx_device_info *di = data;
> -
> -	bq27xxx_battery_update(di);
> -
> -	return IRQ_HANDLED;
> -}
> -
>  static void bq27xxx_battery_poll(struct work_struct *work)
>  {
>  	struct bq27xxx_device_info *di =
> @@ -1061,6 +1052,15 @@ static void bq27xxx_powersupply_unregister(struct bq27xxx_device_info *di)
>  static DEFINE_IDR(battery_id);
>  static DEFINE_MUTEX(battery_mutex);
>  
> +static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
> +{
> +	struct bq27xxx_device_info *di = data;
> +
> +	bq27xxx_battery_update(di);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>  				    bool single)
>  {

Acked-by: Pali Rohár <pali.rohar@gmail.com>

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH 1/3] power: bq27xxx_battery: fix platform probe

[Andrew F. Davis]

On 10/16/2015 08:44 AM, Sebastian Reichel wrote:
> Add missing initialization of register mapping table to
> platform probe function.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   drivers/power/bq27xxx_battery.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> index 994c78d9f426..69e6b371a51c 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -1292,6 +1292,7 @@ static int bq27xxx_battery_platform_probe(struct platform_device *pdev)
>
>   	di->dev = &pdev->dev;
>   	di->chip = pdata->chip;
> +	di->regs = bq27xxx_regs[di->chip];
>
>   	name = pdata->name ?: dev_name(&pdev->dev);
>   	di->bus.read = &bq27xxx_battery_platform_read;
>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

Re: [PATCH 3/3] power: bq27xxx_battery: fix defined but not used warnings

[Andrew F. Davis]

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.

-- 
Andrew F. Davis

Re: [PATCH 2/3] power: bq27xxx_battery: move irq handler to i2c section

[Andrew F. Davis]

On 10/16/2015 08:44 AM, Sebastian Reichel wrote:
> The IRQ handler is not used by the platform based
> code resulting in a 'defined but not used' warning,
> if CONFIG_BQ27XXX_I2C is not enabled.
>
> Signed-off-by: Sebastian Reichel <sre@kernel.org>
> ---
>   drivers/power/bq27xxx_battery.c | 18 +++++++++---------
>   1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/power/bq27xxx_battery.c b/drivers/power/bq27xxx_battery.c
> index 69e6b371a51c..880233ce9343 100644
> --- a/drivers/power/bq27xxx_battery.c
> +++ b/drivers/power/bq27xxx_battery.c
> @@ -762,15 +762,6 @@ static void bq27xxx_battery_update(struct bq27xxx_device_info *di)
>   	di->last_update = jiffies;
>   }
>
> -static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
> -{
> -	struct bq27xxx_device_info *di = data;
> -
> -	bq27xxx_battery_update(di);
> -
> -	return IRQ_HANDLED;
> -}
> -
>   static void bq27xxx_battery_poll(struct work_struct *work)
>   {
>   	struct bq27xxx_device_info *di =
> @@ -1061,6 +1052,15 @@ static void bq27xxx_powersupply_unregister(struct bq27xxx_device_info *di)
>   static DEFINE_IDR(battery_id);
>   static DEFINE_MUTEX(battery_mutex);
>
> +static irqreturn_t bq27xxx_battery_irq_handler_thread(int irq, void *data)
> +{
> +	struct bq27xxx_device_info *di = data;
> +
> +	bq27xxx_battery_update(di);
> +
> +	return IRQ_HANDLED;
> +}
> +
>   static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>   				    bool single)
>   {
>

Acked-by: Andrew F. Davis <afd@ti.com>

-- 
Andrew F. Davis

Re: [PATCH 3/3] power: bq27xxx_battery: fix defined but not used warnings

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2654 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] power: bq27xxx_battery: fix defined but not used warnings

[Andrew F. Davis]

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)

-- 
Andrew F. Davis