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

["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
> [   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