linux-kernel - Re: [PATCH] power_supply: change the way how wm97xx-bat driver is registered

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 03 Nov 2008 16:54:57 +0000
From:	Jonathan Cameron <Jonathan.Cameron@...il.com>
To:	avorontsov@...mvista.com
CC:	Dmitry Baryshkov <dbaryshkov@...il.com>,
	linux-kernel@...r.kernel.org, cbou@...l.ru,
	Andrew Morton <akpm@...ux-foundation.org>,
	Marek Vasut <marek.vasut@...il.com>
Subject: Re: [PATCH] power_supply: change the way how wm97xx-bat driver is
 registered

Anton Vorontsov wrote:
> On Wed, Oct 29, 2008 at 12:04:10PM +0300, Dmitry Baryshkov wrote:
>> Do register the driver from wm97xx_bat_set_pdata instead of module init.
>> This has several positive outcome points:
>> 1) This driver for wm97xx-battery is now registered on demand, thus allowing
>>   other driver (e.g. tosa-battery) to claim the battery.
>> 2) After dropping __init from wm97xx_bat_set_pdata(), the wm97xx_bat driver
>>   can be correctly built as a module. Then one can request it and set the
>>   battery data.
>>
>> Signed-off-by: Dmitry Baryshkov <dbaryshkov@...il.com>
>> Cc: Marek Vasut <marek.vasut@...il.com>
>> ---
>>  drivers/power/Kconfig          |    4 ++--
>>  drivers/power/wm97xx_battery.c |   12 +++++-------
>>  include/linux/wm97xx_batt.h    |    7 +++++--
>>  3 files changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>> index 8e0c2b4..053f20b 100644
>> --- a/drivers/power/Kconfig
>> +++ b/drivers/power/Kconfig
>> @@ -57,8 +57,8 @@ config BATTERY_TOSA
>>  	  SL-6000 (tosa) models.
>>  
>>  config BATTERY_WM97XX
>> -	bool "WM97xx generic battery driver"
>> -	depends on TOUCHSCREEN_WM97XX=y
>> +	tristate "WM97xx generic battery driver"
>> +	depends on TOUCHSCREEN_WM97XX
>>  	help
>>  	  Say Y to enable support for battery measured by WM97xx aux port.
>>  
>> diff --git a/drivers/power/wm97xx_battery.c b/drivers/power/wm97xx_battery.c
>> index 8bde921..94727dc 100644
>> --- a/drivers/power/wm97xx_battery.c
>> +++ b/drivers/power/wm97xx_battery.c
>> @@ -248,23 +248,21 @@ static struct platform_driver wm97xx_bat_driver = {
>>  	.resume		= wm97xx_bat_resume,
>>  };
>>  
>> -static int __init wm97xx_bat_init(void)
>> -{
>> -	return platform_driver_register(&wm97xx_bat_driver);
>> -}
>> -
>>  static void __exit wm97xx_bat_exit(void)
>>  {
>>  	platform_driver_unregister(&wm97xx_bat_driver);
>>  }
>>  
>> -void __init wm97xx_bat_set_pdata(struct wm97xx_batt_info *data)
>> +int wm97xx_bat_set_pdata(struct wm97xx_batt_info *data)
>>  {
>> +	if (pdata)
>> +		return -EEXIST;
>> +
>>  	pdata = data;
>> +	return platform_driver_register(&wm97xx_bat_driver);
>>  }
>>  EXPORT_SYMBOL_GPL(wm97xx_bat_set_pdata);
>>  
>> -module_init(wm97xx_bat_init);
>>  module_exit(wm97xx_bat_exit);
>>  
>>  MODULE_LICENSE("GPL");
>> diff --git a/include/linux/wm97xx_batt.h b/include/linux/wm97xx_batt.h
>> index 9681d1a..3e42526 100644
>> --- a/include/linux/wm97xx_batt.h
>> +++ b/include/linux/wm97xx_batt.h
>> @@ -18,9 +18,12 @@ struct wm97xx_batt_info {
>>  };
>>  
>>  #ifdef CONFIG_BATTERY_WM97XX
>> -void __init wm97xx_bat_set_pdata(struct wm97xx_batt_info *data);
>> +int wm97xx_bat_set_pdata(struct wm97xx_batt_info *data);
>>  #else
>> -static inline void wm97xx_bat_set_pdata(struct wm97xx_batt_info *data) {}
>> +static inline int wm97xx_bat_set_pdata(struct wm97xx_batt_info *data)
>> +{
>> +	return -ENODEV;
>> +}
>>  #endif
> 
> How that supposed to work when BATTERY_WM97XX is a module?
> #ifdef CONFIG_BATTERY_WM97XX will evaluate to false, and you'll have
> dummy wm97xx_bat_set_pdata() that returns -ENODEV...
> 
> The whole wm97xx stuff is broken wrt device-driver model... Yeah,
> I know why -- we don't have the ADC API/Subsystem. It would be great
> if somebody could find time to forward-port the subsystem from the
> handhelds.org tree...
> 
> I don't know what happened to the industrialio subsystem though
> (http://lkml.org/lkml/2008/7/23/163), but it looked needlessly
> complicated, and probably Jonathan gave up on it... :-/
> 
It is needlessly complex for this sort of thing, but that's not it's
purpose.  (though that's not to say it won't be able to do this sort
of thing).  It's gotten a smigeon delayed due to a change of my own
requirements for what it does.  As a reminder, the purpose of that
subsystem was at least partly to provide reasonably high performance
data capture facilities (ring buffers, triggered sampling etc alongside
suitably powerful userspace interfaces.)  Possibly my apps are somewhat
unusual, but the complexity is absolutely necessary for what I'm doing
(annoyingly!)

The complexity is partly due to trying to elegantly allow much reduced
functionality such as this sort of app requires.   (leads to a lot
of fiddly testing!)

Anyhow, definitely not given up on it.  The intermediate versions are
getting a fair bit of testing, but isn't ready (in my view) to go
towards mainline yet.

Jonathan


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/