[PATCH 0/4] Fix minor bq27x00 displeasures

[PATCH 0/4] Fix minor bq27x00 displeasures

[H. Nikolaus Schaller]

1. bq27x00 driver may fill the log with messages about missing calibration:
report only in debug mode
2. if bq27x00 is a w1/hdq slave the wrapper should modprobe to load
bq27x00_battery.ko if compiled as a module.

Both patches improve system battery (co)operation on the GTA04.

H. Nikolaus Schaller (3):
  restore bq27000 code of v4.3-rc4
  restore w1 slave bq27000 code of v4.3-rc4
  w1:slaves:bq27000: load battery driver kernel module if required

NeilBrown (1):
  power:bq27x00: don't fill system log by missing battery


-- 
2.5.1

[PATCH 1/4] restore bq27000 code of v4.3-rc4

[H. Nikolaus Schaller]

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
index 709d1e4..8287261f 100644
--- a/drivers/power/bq27x00_battery.c
+++ b/drivers/power/bq27x00_battery.c
@@ -491,7 +491,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
 	if (cache.flags >= 0) {
 		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
 				&& (cache.flags & BQ27000_FLAG_CI)) {
-			dev_dbg(di->dev, "battery is not calibrated! ignoring capacity values\n");
+			dev_info(di->dev, "battery is not calibrated! ignoring capacity values\n");
 			cache.capacity = -ENODATA;
 			cache.energy = -ENODATA;
 			cache.time_to_empty = -ENODATA;
-- 
2.5.1

[PATCH 2/4] restore w1 slave bq27000 code of v4.3-rc4

[H. Nikolaus Schaller]

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/w1/slaves/w1_bq27000.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index 745bd6e..caafb17 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -49,8 +49,6 @@ static int w1_bq27000_add_slave(struct w1_slave *sl)
 	int ret;
 	struct platform_device *pdev;
 
-	request_module("bq27x00_battery");  /* load as module if needed */
-
 	pdev = platform_device_alloc("bq27000-battery", -1);
 	if (!pdev) {
 		ret = -ENOMEM;
-- 
2.5.1

[PATCH 3/4] power:bq27x00: don't fill system log by missing battery

[H. Nikolaus Schaller]

From: NeilBrown <neilb@suse.de>

Print message that battery is not calibrated only during debug build but
not during normal operation.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
index 8287261f..709d1e4 100644
--- a/drivers/power/bq27x00_battery.c
+++ b/drivers/power/bq27x00_battery.c
@@ -491,7 +491,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
 	if (cache.flags >= 0) {
 		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
 				&& (cache.flags & BQ27000_FLAG_CI)) {
-			dev_info(di->dev, "battery is not calibrated! ignoring capacity values\n");
+			dev_dbg(di->dev, "battery is not calibrated! ignoring capacity values\n");
 			cache.capacity = -ENODATA;
 			cache.energy = -ENODATA;
 			cache.time_to_empty = -ENODATA;
-- 
2.5.1

[PATCH 4/4] w1:slaves:bq27000: load battery driver kernel module if required

[H. Nikolaus Schaller]

Explicitly call request_module() in the wrapper so that the bq27x00_battery
driver is loded on demand, when compiled as module.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/w1/slaves/w1_bq27000.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
index caafb17..745bd6e 100644
--- a/drivers/w1/slaves/w1_bq27000.c
+++ b/drivers/w1/slaves/w1_bq27000.c
@@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave *sl)
 	int ret;
 	struct platform_device *pdev;
 
+	request_module("bq27x00_battery");  /* load as module if needed */
+
 	pdev = platform_device_alloc("bq27000-battery", -1);
 	if (!pdev) {
 		ret = -ENOMEM;
-- 
2.5.1

Re: [PATCH 1/4] restore bq27000 code of v4.3-rc4

[H. Nikolaus Schaller]

Sorry, please forget this.

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/power/bq27x00_battery.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/bq27x00_battery.c b/drivers/power/bq27x00_battery.c
> index 709d1e4..8287261f 100644
> --- a/drivers/power/bq27x00_battery.c
> +++ b/drivers/power/bq27x00_battery.c
> @@ -491,7 +491,7 @@ static void bq27x00_update(struct bq27x00_device_info *di)
> 	if (cache.flags >= 0) {
> 		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
> 				&& (cache.flags & BQ27000_FLAG_CI)) {
> -			dev_dbg(di->dev, "battery is not calibrated! ignoring capacity values\n");
> +			dev_info(di->dev, "battery is not calibrated! ignoring capacity values\n");
> 			cache.capacity = -ENODATA;
> 			cache.energy = -ENODATA;
> 			cache.time_to_empty = -ENODATA;
> -- 
> 2.5.1
> 

Re: [PATCH 2/4] restore w1 slave bq27000 code of v4.3-rc4

[H. Nikolaus Schaller]

Sorry, please forget this.

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
> drivers/w1/slaves/w1_bq27000.c | 2 --
> 1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/w1/slaves/w1_bq27000.c b/drivers/w1/slaves/w1_bq27000.c
> index 745bd6e..caafb17 100644
> --- a/drivers/w1/slaves/w1_bq27000.c
> +++ b/drivers/w1/slaves/w1_bq27000.c
> @@ -49,8 +49,6 @@ static int w1_bq27000_add_slave(struct w1_slave *sl)
> 	int ret;
> 	struct platform_device *pdev;
> 
> -	request_module("bq27x00_battery");  /* load as module if needed */
> -
> 	pdev = platform_device_alloc("bq27000-battery", -1);
> 	if (!pdev) {
> 		ret = -ENOMEM;
> -- 
> 2.5.1
> 

Re: [PATCH 0/4] Fix minor bq27x00 displeasures

[H. Nikolaus Schaller]

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> 1. bq27x00 driver may fill the log with messages about missing calibration:
> report only in debug mode
> 2. if bq27x00 is a w1/hdq slave the wrapper should modprobe to load
> bq27x00_battery.ko if compiled as a module.
> 
> Both patches improve system battery (co)operation on the GTA04.
> 
> H. Nikolaus Schaller (3):

Sorry, please forget the first two patches (was a mistake when send-emailing a subset).

>  restore bq27000 code of v4.3-rc4
>  restore w1 slave bq27000 code of v4.3-rc4
>  w1:slaves:bq27000: load battery driver kernel module if required
> 
> NeilBrown (1):
>  power:bq27x00: don't fill system log by missing battery
> 
> 
> -- 
> 2.5.1
> 

Re: [PATCH 3/4] power:bq27x00: don't fill system log by missing battery

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1257 bytes --]

On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
> From: NeilBrown <neilb@suse.de>
> 
> Print message that battery is not calibrated only during debug build
> but not during normal operation.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/power/bq27x00_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/bq27x00_battery.c
> b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
> --- a/drivers/power/bq27x00_battery.c
> +++ b/drivers/power/bq27x00_battery.c
> @@ -491,7 +491,7 @@ static void bq27x00_update(struct
> bq27x00_device_info *di) if (cache.flags >= 0) {
>  		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
>  				&& (cache.flags & BQ27000_FLAG_CI)) {
> -			dev_info(di->dev, "battery is not calibrated! ignoring 
capacity
> values\n"); +			dev_dbg(di->dev, "battery is not calibrated!
> ignoring capacity values\n"); cache.capacity = -ENODATA;
>  			cache.energy = -ENODATA;
>  			cache.time_to_empty = -ENODATA;

Hi! I think that better approach would be to use WARN_ONCE or similar 
macro. Still use INFO level, just warn about this problem only once...

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 4/4] w1:slaves:bq27000: load battery driver kernel module if required

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1421 bytes --]

On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
> Explicitly call request_module() in the wrapper so that the
> bq27x00_battery driver is loded on demand, when compiled as module.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/w1/slaves/w1_bq27000.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/w1/slaves/w1_bq27000.c
> b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
> --- a/drivers/w1/slaves/w1_bq27000.c
> +++ b/drivers/w1/slaves/w1_bq27000.c
> @@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave
> *sl) int ret;
>  	struct platform_device *pdev;
> 
> +	request_module("bq27x00_battery");  /* load as module if needed */
> +
>  	pdev = platform_device_alloc("bq27000-battery", -1);
>  	if (!pdev) {
>  		ret = -ENOMEM;

Hi! Function platform_device_alloc would allocate kernel device and it 
should also load appropriate kernel driver (via platform:bq27000-battery 
alias). If that does not work, problem is somewhere else. Maybe depmod 
is not properly generated? Or bq27x00_batter.ko modules does not contain 
needed alias?

Adding request_module() here is not proper solution. Real problem is 
somewhere else... Anyway there is series of patches for bq27x00_battery 
which should cleanup some problems, so maybe they also fix your problem.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/4] power:bq27x00: don't fill system log by missing battery

[Neil Brown]

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

Pali Rohár <pali.rohar@gmail.com> writes:

> On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
>> From: NeilBrown <neilb@suse.de>
>> 
>> Print message that battery is not calibrated only during debug build
>> but not during normal operation.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  drivers/power/bq27x00_battery.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/power/bq27x00_battery.c
>> b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
>> --- a/drivers/power/bq27x00_battery.c
>> +++ b/drivers/power/bq27x00_battery.c
>> @@ -491,7 +491,7 @@ static void bq27x00_update(struct
>> bq27x00_device_info *di) if (cache.flags >= 0) {
>>  		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
>>  				&& (cache.flags & BQ27000_FLAG_CI)) {
>> -			dev_info(di->dev, "battery is not calibrated! ignoring 
> capacity
>> values\n"); +			dev_dbg(di->dev, "battery is not calibrated!
>> ignoring capacity values\n"); cache.capacity = -ENODATA;
>>  			cache.energy = -ENODATA;
>>  			cache.time_to_empty = -ENODATA;
>
> Hi! I think that better approach would be to use WARN_ONCE or similar 
> macro. Still use INFO level, just warn about this problem only once...

Why do you need any warning?
The status of whether the battery is calibrated is trivially determined
From the sysfs attributes (several of which will return ENODATA).
So if some app is being used to report battery status, then it can
easily report "not calibrated", and if no such app is being used, who
will care to know?

I'm not exactly against a once-only warning (though not with WARN_ONCE,
maybe dev_info_once()) but I don't think that it brings any value at
all.

Thanks,
NeilBrown

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

Re: [PATCH 4/4] w1:slaves:bq27000: load battery driver kernel module if required

[H. Nikolaus Schaller]

Am 08.10.2015 um 00:26 schrieb Pali Rohár <pali.rohar@gmail.com>:

> On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
>> Explicitly call request_module() in the wrapper so that the
>> bq27x00_battery driver is loded on demand, when compiled as module.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/w1/slaves/w1_bq27000.c | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/w1/slaves/w1_bq27000.c
>> b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
>> --- a/drivers/w1/slaves/w1_bq27000.c
>> +++ b/drivers/w1/slaves/w1_bq27000.c
>> @@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave
>> *sl) int ret;
>> 	struct platform_device *pdev;
>> 
>> +	request_module("bq27x00_battery");  /* load as module if needed */
>> +
>> 	pdev = platform_device_alloc("bq27000-battery", -1);
>> 	if (!pdev) {
>> 		ret = -ENOMEM;
> 
> Hi! Function platform_device_alloc would allocate kernel device and it 
> should also load appropriate kernel driver (via platform:bq27000-battery 
> alias). If that does not work, problem is somewhere else. Maybe depmod 
> is not properly generated? Or bq27x00_batter.ko modules does not contain 
> needed alias?

alias exists:

#ifdef CONFIG_BATTERY_BQ27X00_PLATFORM
MODULE_ALIAS("platform:bq27000-battery");
#endif

depmod entry exist:

root@letux:~# modprobe -c | fgrep bq27
alias i2c:bq27000_battery bq27x00_battery
alias i2c:bq27200 bq27x00_battery
alias i2c:bq27425 bq27x00_battery
alias i2c:bq27500 bq27x00_battery
alias i2c:bq27510 bq27x00_battery
alias i2c:bq27742 bq27x00_battery
alias platform:bq27000_battery bq27x00_battery
alias w1_family_0x01 w1_bq27000

And I have traced all calls to call_modprobe

[    9.969085] call_modprobe w1-family-0x01
[   10.189727] call_modprobe bq27x00_battery
[   29.879333] call_modprobe net-pf-10
[   30.530548] call_modprobe netdev-eth0
[   30.548156] call_modprobe eth0
[   30.565063] call_modprobe netdev-eth0
[   30.581420] call_modprobe eth0
[   31.301300] call_modprobe netdev-eth1
[   31.316101] call_modprobe eth1
[   32.008270] call_modprobe usbfunc:ecm
[   35.554748] call_modprobe net-pf-16-proto-9
[   36.243164] call_modprobe net-pf-31
[   36.614410] call_modprobe bt-proto-4
[   73.888122] call_modprobe net-pf-16-proto-9
[   73.921112] call_modprobe net-pf-16-proto-9
[   77.153625] call_modprobe net-pf-16-proto-9
[   77.188262] call_modprobe net-pf-16-proto-9
[   77.342895] call_modprobe net-pf-16-proto-9
[   77.389129] call_modprobe net-pf-16-proto-9
[   77.419006] call_modprobe net-pf-16-proto-9

And there is only the line I have added. I.e. I can't find an automatic attempt
to load the platform:bq27000-battery. It also doesn't help to change to 
platform_device_alloc("bq27000_battery", -1)

So I think platform_device_alloc() does only what it tells: it allocates a device
but does *not* try to load a kernel module. It only binds to a driver compiled
into the kernel. AFAIR the I2C bus driver also calls request_module() at some
stage after finding a client (i2c:bq27000_battery).

> Adding request_module() here is not proper solution. Real problem is 
> somewhere else... Anyway there is series of patches for bq27x00_battery 
> which should cleanup some problems, so maybe they also fix your problem.

Is there a tree where I can pull the patches to cross-check if they change
something?

BR,
Nikolaus

Re: [PATCH 4/4] w1:slaves:bq27000: load battery driver kernel module if required

[Pali Rohár]

On Thursday 08 October 2015 09:26:26 H. Nikolaus Schaller wrote:
> 
> Am 08.10.2015 um 00:26 schrieb Pali Rohár <pali.rohar@gmail.com>:
> 
> > On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
> >> Explicitly call request_module() in the wrapper so that the
> >> bq27x00_battery driver is loded on demand, when compiled as module.
> >> 
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >> drivers/w1/slaves/w1_bq27000.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/w1/slaves/w1_bq27000.c
> >> b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
> >> --- a/drivers/w1/slaves/w1_bq27000.c
> >> +++ b/drivers/w1/slaves/w1_bq27000.c
> >> @@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave
> >> *sl) int ret;
> >> 	struct platform_device *pdev;
> >> 
> >> +	request_module("bq27x00_battery");  /* load as module if needed */
> >> +
> >> 	pdev = platform_device_alloc("bq27000-battery", -1);
> >> 	if (!pdev) {
> >> 		ret = -ENOMEM;
> > 
> > Hi! Function platform_device_alloc would allocate kernel device and it 
> > should also load appropriate kernel driver (via platform:bq27000-battery 
> > alias). If that does not work, problem is somewhere else. Maybe depmod 
> > is not properly generated? Or bq27x00_batter.ko modules does not contain 
> > needed alias?
> 

At least driver is renaming in upstream kernel, so hardcoding driver
name is wrong approach. It will be broken... If we show that
request_module is really needed (but I believe not!) then please use
alias instead.

> alias exists:
> 
> #ifdef CONFIG_BATTERY_BQ27X00_PLATFORM
> MODULE_ALIAS("platform:bq27000-battery");
> #endif
> 
> depmod entry exist:
> 
> root@letux:~# modprobe -c | fgrep bq27
> alias i2c:bq27000_battery bq27x00_battery
> alias i2c:bq27200 bq27x00_battery
> alias i2c:bq27425 bq27x00_battery
> alias i2c:bq27500 bq27x00_battery
> alias i2c:bq27510 bq27x00_battery
> alias i2c:bq27742 bq27x00_battery
> alias platform:bq27000_battery bq27x00_battery
> alias w1_family_0x01 w1_bq27000
> 
> And I have traced all calls to call_modprobe
> 
> [    9.969085] call_modprobe w1-family-0x01
> [   10.189727] call_modprobe bq27x00_battery
> [   29.879333] call_modprobe net-pf-10
> [   30.530548] call_modprobe netdev-eth0
> [   30.548156] call_modprobe eth0
> [   30.565063] call_modprobe netdev-eth0
> [   30.581420] call_modprobe eth0
> [   31.301300] call_modprobe netdev-eth1
> [   31.316101] call_modprobe eth1
> [   32.008270] call_modprobe usbfunc:ecm
> [   35.554748] call_modprobe net-pf-16-proto-9
> [   36.243164] call_modprobe net-pf-31
> [   36.614410] call_modprobe bt-proto-4
> [   73.888122] call_modprobe net-pf-16-proto-9
> [   73.921112] call_modprobe net-pf-16-proto-9
> [   77.153625] call_modprobe net-pf-16-proto-9
> [   77.188262] call_modprobe net-pf-16-proto-9
> [   77.342895] call_modprobe net-pf-16-proto-9
> [   77.389129] call_modprobe net-pf-16-proto-9
> [   77.419006] call_modprobe net-pf-16-proto-9
> 
> And there is only the line I have added. I.e. I can't find an automatic attempt
> to load the platform:bq27000-battery. It also doesn't help to change to 
> platform_device_alloc("bq27000_battery", -1)
> 
> So I think platform_device_alloc() does only what it tells: it allocates a device
> but does *not* try to load a kernel module. It only binds to a driver compiled
> into the kernel. AFAIR the I2C bus driver also calls request_module() at some
> stage after finding a client (i2c:bq27000_battery).
> 

Try to call platform_device_register(). E.g for rx51-battery power
supply device it is working fine. Device itself is allocated and
registered in arch/arm/mach-omap2/board-rx51-peripherals.c and driver is
in drivers/power/rx51_battery.c. You can also try to reuse that code...
I see that w1 is quite different.

> > Adding request_module() here is not proper solution. Real problem is 
> > somewhere else... Anyway there is series of patches for bq27x00_battery 
> > which should cleanup some problems, so maybe they also fix your problem.
> 
> Is there a tree where I can pull the patches to cross-check if they change
> something?
> 

Sebastian? Where are those new bq27k patches now?

> BR,
> Nikolaus
> 

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

Re: [PATCH 4/4] w1:slaves:bq27000: load battery driver kernel module if required

[H. Nikolaus Schaller]

Hi,

Am 08.10.2015 um 09:54 schrieb Pali Rohár <pali.rohar@gmail.com>:

> On Thursday 08 October 2015 09:26:26 H. Nikolaus Schaller wrote:
>> 
>> Am 08.10.2015 um 00:26 schrieb Pali Rohár <pali.rohar@gmail.com>:
>> 
>>> On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
>>>> Explicitly call request_module() in the wrapper so that the
>>>> bq27x00_battery driver is loded on demand, when compiled as module.
>>>> 
>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>> ---
>>>> drivers/w1/slaves/w1_bq27000.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>> 
>>>> diff --git a/drivers/w1/slaves/w1_bq27000.c
>>>> b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
>>>> --- a/drivers/w1/slaves/w1_bq27000.c
>>>> +++ b/drivers/w1/slaves/w1_bq27000.c
>>>> @@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave
>>>> *sl) int ret;
>>>> 	struct platform_device *pdev;
>>>> 
>>>> +	request_module("bq27x00_battery");  /* load as module if needed */
>>>> +
>>>> 	pdev = platform_device_alloc("bq27000-battery", -1);
>>>> 	if (!pdev) {
>>>> 		ret = -ENOMEM;
>>> 
>>> Hi! Function platform_device_alloc would allocate kernel device and it 
>>> should also load appropriate kernel driver (via platform:bq27000-battery 
>>> alias). If that does not work, problem is somewhere else. Maybe depmod 
>>> is not properly generated? Or bq27x00_batter.ko modules does not contain 
>>> needed alias?
>> 
> 
> At least driver is renaming in upstream kernel, so hardcoding driver
> name is wrong approach. It will be broken... If we show that
> request_module is really needed (but I believe not!) then please use
> alias instead.

> 
>> alias exists:
>> 
>> #ifdef CONFIG_BATTERY_BQ27X00_PLATFORM
>> MODULE_ALIAS("platform:bq27000-battery");
>> #endif
>> 
>> depmod entry exist:
>> 
>> root@letux:~# modprobe -c | fgrep bq27
>> alias i2c:bq27000_battery bq27x00_battery
>> alias i2c:bq27200 bq27x00_battery
>> alias i2c:bq27425 bq27x00_battery
>> alias i2c:bq27500 bq27x00_battery
>> alias i2c:bq27510 bq27x00_battery
>> alias i2c:bq27742 bq27x00_battery
>> alias platform:bq27000_battery bq27x00_battery
>> alias w1_family_0x01 w1_bq27000
>> 
>> And I have traced all calls to call_modprobe
>> 
>> [    9.969085] call_modprobe w1-family-0x01
>> [   10.189727] call_modprobe bq27x00_battery

I have tried without request_module("bq27x00_battery") and this
call to call_modprobe() disappears.

But it still works, i.e. the module appears in lsmod!

I am a bit puzzled now...

What could have happened:
* there was no automatic module load when we developed
the patch (a while ago - it was sleeping in our development tree)
* when automatic load was improved somewhere else, we
did not notice (because a second request_module() does
not have a visible side-effects)
* so we just though that this is still required and did not check
that it isn't any more

So as it appears to work, we can simply drop it from the patch
set (and our development tree) v2.

Sorry for the noise and thanks for helping to find it.

>> [   29.879333] call_modprobe net-pf-10
>> [   30.530548] call_modprobe netdev-eth0
>> [   30.548156] call_modprobe eth0
>> [   30.565063] call_modprobe netdev-eth0
>> [   30.581420] call_modprobe eth0
>> [   31.301300] call_modprobe netdev-eth1
>> [   31.316101] call_modprobe eth1
>> [   32.008270] call_modprobe usbfunc:ecm
>> [   35.554748] call_modprobe net-pf-16-proto-9
>> [   36.243164] call_modprobe net-pf-31
>> [   36.614410] call_modprobe bt-proto-4
>> [   73.888122] call_modprobe net-pf-16-proto-9
>> [   73.921112] call_modprobe net-pf-16-proto-9
>> [   77.153625] call_modprobe net-pf-16-proto-9
>> [   77.188262] call_modprobe net-pf-16-proto-9
>> [   77.342895] call_modprobe net-pf-16-proto-9
>> [   77.389129] call_modprobe net-pf-16-proto-9
>> [   77.419006] call_modprobe net-pf-16-proto-9
>> 
>> And there is only the line I have added. I.e. I can't find an automatic attempt
>> to load the platform:bq27000-battery. It also doesn't help to change to 
>> platform_device_alloc("bq27000_battery", -1)
>> 
>> So I think platform_device_alloc() does only what it tells: it allocates a device
>> but does *not* try to load a kernel module. It only binds to a driver compiled
>> into the kernel. AFAIR the I2C bus driver also calls request_module() at some
>> stage after finding a client (i2c:bq27000_battery).
>> 
> 
> Try to call platform_device_register(). E.g for rx51-battery power
> supply device it is working fine. Device itself is allocated and
> registered in arch/arm/mach-omap2/board-rx51-peripherals.c and driver is
> in drivers/power/rx51_battery.c. You can also try to reuse that code...
> I see that w1 is quite different.
> 
>>> Adding request_module() here is not proper solution. Real problem is 
>>> somewhere else... Anyway there is series of patches for bq27x00_battery 
>>> which should cleanup some problems, so maybe they also fix your problem.
>> 
>> Is there a tree where I can pull the patches to cross-check if they change
>> something?
>> 
> 
> Sebastian? Where are those new bq27k patches now?
> 
>> BR,
>> Nikolaus
>> 
> 
> -- 
> Pali Rohár
> pali.rohar@gmail.com

BR,
Nikolaus

Re: [PATCH 3/4] power:bq27x00: don't fill system log by missing battery

[Sebastian Reichel]

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

On Thu, Oct 08, 2015 at 09:50:26AM +1100, Neil Brown wrote:
> Pali Rohár <pali.rohar@gmail.com> writes:
> 
> > On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
> >> From: NeilBrown <neilb@suse.de>
> >> 
> >> Print message that battery is not calibrated only during debug build
> >> but not during normal operation.
> >> 
> >> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> >> ---
> >>  drivers/power/bq27x00_battery.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/power/bq27x00_battery.c
> >> b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
> >> --- a/drivers/power/bq27x00_battery.c
> >> +++ b/drivers/power/bq27x00_battery.c
> >> @@ -491,7 +491,7 @@ static void bq27x00_update(struct
> >> bq27x00_device_info *di) if (cache.flags >= 0) {
> >>  		if (!is_bq27500 && !is_bq27425 && !is_bq27742 && !is_bq27510
> >>  				&& (cache.flags & BQ27000_FLAG_CI)) {
> >> -			dev_info(di->dev, "battery is not calibrated! ignoring 
> > capacity
> >> values\n"); +			dev_dbg(di->dev, "battery is not calibrated!
> >> ignoring capacity values\n"); cache.capacity = -ENODATA;
> >>  			cache.energy = -ENODATA;
> >>  			cache.time_to_empty = -ENODATA;
> >
> > Hi! I think that better approach would be to use WARN_ONCE or similar 
> > macro. Still use INFO level, just warn about this problem only once...
> 
> Why do you need any warning?
> The status of whether the battery is calibrated is trivially determined
> From the sysfs attributes (several of which will return ENODATA).
> So if some app is being used to report battery status, then it can
> easily report "not calibrated", and if no such app is being used, who
> will care to know?
> 
> I'm not exactly against a once-only warning (though not with WARN_ONCE,
> maybe dev_info_once()) but I don't think that it brings any value at
> all.

There is no use in having it printed every time, even for debugging.
It just adds lots of spam to the log/console. Anyone debugging the
driver will know, that the battery requires calibration and returns
-ENODATA in uncalibrated mode.

Users not knowing the bq27xxx chips might wonder why the driver
returns -ENODATA, though. For these users a single print
in the log is probably useful. Thus I will take this patch
with dev_info_once() and the following tags:

From: NeilBrown <neilb@suse.de>
Suggested-By: H. Nikolaus Schaller <hns@goldelico.com>
Suggested-By: Pali Rohár <pali.rohar@gmail.com>

-- Sebastian

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