Fix minor bq27x00 displeasures

PATCH In review H. Nikolaus Schaller 4 participants 15 messages Oct 7, 2015 – Dec 5, 2015 W1 dallas's 1-wire bus

Share & tools

H. Nikolaus Schaller

author

2015-10-07 18:42 UTC #0 lore raw

cover Fix minor bq27x00 displeasures

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

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:42 UTC #1 lore raw

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

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/power/bq27x00_battery.c +1 -1

index 709d1e4..8287261f 100644
@@ -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;

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:44 UTC #2 lore raw

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

Sorry, please forget this.

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 21 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/power/bq27x00_battery.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

drivers/power/bq27x00_battery.c +1 -1

index 709d1e4..8287261f 100644
@@ -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;

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:48 UTC #3 lore raw

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

In reply to H. Nikolaus Schaller

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/w1/slaves/w1_bq27000.c | 2 --
 1 file changed, 2 deletions(-)

drivers/w1/slaves/w1_bq27000.c +0 -2

index 745bd6e..caafb17 100644
@@ -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;

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:44 UTC #4 lore raw

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

Sorry, please forget this.

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 21 lines

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
drivers/w1/slaves/w1_bq27000.c | 2 --

1 file changed, 2 deletions(-)

drivers/w1/slaves/w1_bq27000.c +0 -2

index 745bd6e..caafb17 100644
@@ -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;

Signature · 1 line

2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:48 UTC #5 lore raw

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

In reply to H. Nikolaus Schaller

From: NeilBrown <[email protected]>

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

Signed-off-by H. Nikolaus Schaller <[email protected]>

---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/power/bq27x00_battery.c +1 -1

index 8287261f..709d1e4 100644
@@ -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;

Signature · 1 line

2.5.1

Pali Rohár

2015-10-07 22:23 UTC #6 lore raw

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

Quoted 15 lines · On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:

From: NeilBrown <[email protected]>
Print message that battery is not calibrated only during debug build but not during normal operation.
Signed-off-by H. Nikolaus Schaller <[email protected]>
---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644

drivers/power/bq27x00_battery.c static void bq27x00_update(struct · line 494

Quoted 4 lines

drivers/power/bq27x00_battery.c +0 -1

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

drivers/power/bq27x00_battery.c static void bq27x00_update(struct bq27x00_device_info *di) · line 497

Quoted 4 lines

values\n"); + dev_dbg(di->dev, "battery is not calibrated!
ignoring capacity values\n"); cache.capacity = -ENODATA;
drivers/power/bq27x00_battery.c +0 -0
 			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...

Signature · 2 lines

Pali Rohár
[email protected]

Neil Brown

2015-10-07 22:50 UTC #7 lore raw

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

Quoted 21 lines · Pali Rohár

Quoted 15 lines · On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
From: NeilBrown <[email protected]>
Print message that battery is not calibrated only during debug build but not during normal operation.
Signed-off-by H. Nikolaus Schaller <[email protected]>
---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
drivers/power/bq27x00_battery.c static void bq27x00_update(struct · line 494
Quoted 4 lines
drivers/power/bq27x00_battery.c +0 -1
@@ -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

Quoted 7 lines

drivers/power/bq27x00_battery.c static void bq27x00_update(struct bq27x00_device_info *di) · line 497
Quoted 4 lines
values\n"); + dev_dbg(di->dev, "battery is not calibrated!
ignoring capacity values\n"); cache.capacity = -ENODATA;
drivers/power/bq27x00_battery.c +0 -0
 			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

Sebastian Reichel

2015-12-05 00:38 UTC #8 lore raw

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

Quoted 38 lines · Neil Brown

Quoted 21 lines · Pali Rohár
Quoted 15 lines · On Wednesday 07 October 2015 20:42:38 H. Nikolaus Schaller wrote:
From: NeilBrown <[email protected]>
Print message that battery is not calibrated only during debug build but not during normal operation.
Signed-off-by H. Nikolaus Schaller <[email protected]>
---
 drivers/power/bq27x00_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
(quoted diff) +0 -0
b/drivers/power/bq27x00_battery.c index 8287261f..709d1e4 100644
drivers/power/bq27x00_battery.c static void bq27x00_update(struct · line 494
Quoted 4 lines
drivers/power/bq27x00_battery.c +0 -1
@@ -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
Quoted 7 lines
drivers/power/bq27x00_battery.c static void bq27x00_update(struct bq27x00_device_info *di) · line 497
Quoted 4 lines
values\n"); + dev_dbg(di->dev, "battery is not calibrated!
ignoring capacity values\n"); cache.capacity = -ENODATA;
drivers/power/bq27x00_battery.c +0 -0
 			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?

Quoted 3 lines

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 <[email protected]>

-- Sebastian

H. Nikolaus Schaller

author

2015-10-07 18:45 UTC #9 lore raw

In reply to H. Nikolaus Schaller

Am 07.10.2015 um 20:42 schrieb H. Nikolaus Schaller <[email protected]>:

Quoted 8 lines

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

Quoted 11 lines

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
Signature · 1 line
2.5.1

H. Nikolaus Schaller

author

2015-10-07 18:42 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 drivers/w1/slaves/w1_bq27000.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/w1/slaves/w1_bq27000.c +2 -0

index caafb17..745bd6e 100644
@@ -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;

Signature · 1 line

2.5.1

Pali Rohár

2015-10-07 22:26 UTC #11 lore raw

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

Quoted 13 lines · 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 <[email protected]>
---
 drivers/w1/slaves/w1_bq27000.c | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644

drivers/w1/slaves/w1_bq27000.c static int w1_bq27000_add_slave(struct w1_slave · line 56

Quoted 8 lines

drivers/w1/slaves/w1_bq27000.c +2 -0

@@ -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.

Signature · 2 lines

Pali Rohár
[email protected]

H. Nikolaus Schaller

author

2015-10-08 07:26 UTC #12 lore raw

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

Am 08.10.2015 um 00:26 schrieb Pali Rohár <[email protected]>:

Quoted 17 lines · On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:

Quoted 16 lines
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 <[email protected]>
---
drivers/w1/slaves/w1_bq27000.c | 2 ++
1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
drivers/w1/slaves/w1_bq27000.c +0 -0
@@ -49,6 +49,8 @@ static int w1_bq27000_add_slave(struct w1_slave
 *sl) int ret;
 	struct platform_device *pdev;
 

Quoted 11 lines

drivers/w1/slaves/w1_bq27000.c static int w1_bq27000_add_slave(struct w1_slave *sl) · line 56
Quoted 5 lines
drivers/w1/slaves/w1_bq27000.c +2 -0
+	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).

Quoted 3 lines

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

Pali Rohár

2015-10-08 07:54 UTC #13 lore raw

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

Quoted 32 lines · On Thursday 08 October 2015 09:26:26 H. Nikolaus Schaller wrote:

Am 08.10.2015 um 00:26 schrieb Pali Rohár <[email protected]>:
Quoted 28 lines · On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
Quoted 13 lines
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 <[email protected]>
---
drivers/w1/slaves/w1_bq27000.c | 2 ++
1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
drivers/w1/slaves/w1_bq27000.c static int w1_bq27000_add_slave(struct w1_slave · line 56
Quoted 8 lines
drivers/w1/slaves/w1_bq27000.c +2 -0
@@ -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.

Quoted 41 lines

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

Quoted 9 lines

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.

Quoted 7 lines

Quoted 3 lines
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?

Quoted 3 lines

BR,
Nikolaus

Signature · 2 lines

Pali Rohár
[email protected]

H. Nikolaus Schaller

author

2015-10-08 09:58 UTC #14 lore raw

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

Hi,

Am 08.10.2015 um 09:54 schrieb Pali Rohár <[email protected]>:

Quoted 34 lines · On Thursday 08 October 2015 09:26:26 H. Nikolaus Schaller wrote:

Quoted 32 lines
Am 08.10.2015 um 00:26 schrieb Pali Rohár <[email protected]>:
Quoted 28 lines · On Wednesday 07 October 2015 20:42:39 H. Nikolaus Schaller wrote:
Quoted 13 lines
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 <[email protected]>
---
drivers/w1/slaves/w1_bq27000.c | 2 ++
1 file changed, 2 insertions(+)
(quoted diff) +0 -0
b/drivers/w1/slaves/w1_bq27000.c index caafb17..745bd6e 100644
drivers/w1/slaves/w1_bq27000.c static int w1_bq27000_add_slave(struct w1_slave · line 56
Quoted 8 lines
drivers/w1/slaves/w1_bq27000.c +2 -0
@@ -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?

Quoted 28 lines

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.
Quoted 22 lines
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.

Quoted 43 lines

Quoted 19 lines
[   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
Quoted 9 lines
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.
Quoted 7 lines
Quoted 3 lines
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?

Quoted 9 lines

Sebastian? Where are those new bq27k patches now?
Quoted 3 lines
BR,
Nikolaus
Signature · 2 lines
Pali Rohár
[email protected]

BR,
Nikolaus