There are already several wm97xx-battery drivers. Do register the tosa one
only on tosa machines.
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/power/tosa_battery.c | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
index 2eab35a..4e52c22 100644
--- a/drivers/power/tosa_battery.c
+++ b/drivers/power/tosa_battery.c
@@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
static int __init tosa_bat_init(void)
{
+ if (!machine_is_tosa())
+ return -EINVAL;
+
return platform_driver_register(&tosa_bat_driver);
}
--
1.5.6.5
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 <[email protected]>
Cc: Marek Vasut <[email protected]>
---
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
#endif
--
1.5.6.5
2008/10/29 Dmitry Baryshkov <[email protected]>:
> There are already several wm97xx-battery drivers. Do register the tosa one
> only on tosa machines.
>
> Signed-off-by: Dmitry Baryshkov <[email protected]>
> ---
> drivers/power/tosa_battery.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
What about this patch? IMO it can and should be merged during 2.6.28-rc cycle.
--
With best wishes
Dmitry
2008/10/29 Dmitry Baryshkov <[email protected]>:
> 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 <[email protected]>
> Cc: Marek Vasut <[email protected]>
> ---
> drivers/power/Kconfig | 4 ++--
> drivers/power/wm97xx_battery.c | 12 +++++-------
> include/linux/wm97xx_batt.h | 7 +++++--
> 3 files changed, 12 insertions(+), 11 deletions(-)
What about this patch?
--
With best wishes
Dmitry
On Sunday 02 of November 2008 21:36:58 Dmitry wrote:
> 2008/10/29 Dmitry Baryshkov <[email protected]>:
> > 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 <[email protected]>
> > Cc: Marek Vasut <[email protected]>
> > ---
> > drivers/power/Kconfig | 4 ++--
> > drivers/power/wm97xx_battery.c | 12 +++++-------
> > include/linux/wm97xx_batt.h | 7 +++++--
> > 3 files changed, 12 insertions(+), 11 deletions(-)
>
> What about this patch?
Im sure you know what you are doing. I cant test it right now though, since I
have load of university work to do, sorry.
On Sun, Nov 02, 2008 at 11:36:08PM +0300, Dmitry wrote:
> 2008/10/29 Dmitry Baryshkov <[email protected]>:
> > There are already several wm97xx-battery drivers. Do register the tosa one
> > only on tosa machines.
> >
> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> > ---
> > drivers/power/tosa_battery.c | 3 +++
> > 1 files changed, 3 insertions(+), 0 deletions(-)
>
> What about this patch?
Sorry for the delay.
> IMO it can and should be merged during 2.6.28-rc cycle.
>
> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
> index 2eab35a..4e52c22 100644
> --- a/drivers/power/tosa_battery.c
> +++ b/drivers/power/tosa_battery.c
> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
>
> static int __init tosa_bat_init(void)
> {
> + if (!machine_is_tosa())
> + return -EINVAL;
> +
I tend to reject this approach. You should rename the driver instead.
I.e.
-.driver.name = "wm97xx-battery",
-.driver.name = "tosa-battery",
And make sure that this won't break users of that driver (though
I don't see any).
> return platform_driver_register(&tosa_bat_driver);
> }
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
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 <[email protected]>
> Cc: Marek Vasut <[email protected]>
> ---
> 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... :-/
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
2008/11/3 Anton Vorontsov <[email protected]>:
> On Sun, Nov 02, 2008 at 11:36:08PM +0300, Dmitry wrote:
>> 2008/10/29 Dmitry Baryshkov <[email protected]>:
>> > There are already several wm97xx-battery drivers. Do register the tosa one
>> > only on tosa machines.
>> >
>> > Signed-off-by: Dmitry Baryshkov <[email protected]>
>> > ---
>> > drivers/power/tosa_battery.c | 3 +++
>> > 1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> What about this patch?
>
> Sorry for the delay.
>
>> IMO it can and should be merged during 2.6.28-rc cycle.
>>
>> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
>> index 2eab35a..4e52c22 100644
>> --- a/drivers/power/tosa_battery.c
>> +++ b/drivers/power/tosa_battery.c
>> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
>>
>> static int __init tosa_bat_init(void)
>> {
>> + if (!machine_is_tosa())
>> + return -EINVAL;
>> +
>
> I tend to reject this approach. You should rename the driver instead.
>
> I.e.
> -.driver.name = "wm97xx-battery",
> -.driver.name = "tosa-battery",
>
> And make sure that this won't break users of that driver (though
> I don't see any).
The wm97xx-battery device is registered by
drivers/input/toucscreen/wm97xx-core.c
>
>> return platform_driver_register(&tosa_bat_driver);
>> }
>
> Thanks,
>
> --
> Anton Vorontsov
> email: [email protected]
> irc://irc.freenode.net/bd2
>
--
With best wishes
Dmitry
On Mon, Nov 03, 2008 at 07:20:06PM +0300, Dmitry wrote:
> 2008/11/3 Anton Vorontsov <[email protected]>:
> > On Sun, Nov 02, 2008 at 11:36:08PM +0300, Dmitry wrote:
> >> 2008/10/29 Dmitry Baryshkov <[email protected]>:
> >> > There are already several wm97xx-battery drivers. Do register the tosa one
> >> > only on tosa machines.
> >> >
> >> > Signed-off-by: Dmitry Baryshkov <[email protected]>
> >> > ---
> >> > drivers/power/tosa_battery.c | 3 +++
> >> > 1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> What about this patch?
> >
> > Sorry for the delay.
> >
> >> IMO it can and should be merged during 2.6.28-rc cycle.
> >>
> >> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
> >> index 2eab35a..4e52c22 100644
> >> --- a/drivers/power/tosa_battery.c
> >> +++ b/drivers/power/tosa_battery.c
> >> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
> >>
> >> static int __init tosa_bat_init(void)
> >> {
> >> + if (!machine_is_tosa())
> >> + return -EINVAL;
> >> +
> >
> > I tend to reject this approach. You should rename the driver instead.
> >
> > I.e.
> > -.driver.name = "wm97xx-battery",
> > -.driver.name = "tosa-battery",
> >
> > And make sure that this won't break users of that driver (though
> > I don't see any).
>
> The wm97xx-battery device is registered by
> drivers/input/toucscreen/wm97xx-core.c
Hmm.. So you can't rename the tosa driver, because then it won't
work, correct?
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
2008/11/3 Anton Vorontsov <[email protected]>:
> On Mon, Nov 03, 2008 at 07:20:06PM +0300, Dmitry wrote:
>> 2008/11/3 Anton Vorontsov <[email protected]>:
>> > On Sun, Nov 02, 2008 at 11:36:08PM +0300, Dmitry wrote:
>> >> 2008/10/29 Dmitry Baryshkov <[email protected]>:
>> >> > There are already several wm97xx-battery drivers. Do register the tosa one
>> >> > only on tosa machines.
>> >> >
>> >> > Signed-off-by: Dmitry Baryshkov <[email protected]>
>> >> > ---
>> >> > drivers/power/tosa_battery.c | 3 +++
>> >> > 1 files changed, 3 insertions(+), 0 deletions(-)
>> >>
>> >> What about this patch?
>> >
>> > Sorry for the delay.
>> >
>> >> IMO it can and should be merged during 2.6.28-rc cycle.
>> >>
>> >> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
>> >> index 2eab35a..4e52c22 100644
>> >> --- a/drivers/power/tosa_battery.c
>> >> +++ b/drivers/power/tosa_battery.c
>> >> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
>> >>
>> >> static int __init tosa_bat_init(void)
>> >> {
>> >> + if (!machine_is_tosa())
>> >> + return -EINVAL;
>> >> +
>> >
>> > I tend to reject this approach. You should rename the driver instead.
>> >
>> > I.e.
>> > -.driver.name = "wm97xx-battery",
>> > -.driver.name = "tosa-battery",
>> >
>> > And make sure that this won't break users of that driver (though
>> > I don't see any).
>>
>> The wm97xx-battery device is registered by
>> drivers/input/toucscreen/wm97xx-core.c
>
> Hmm.. So you can't rename the tosa driver, because then it won't
> work, correct?
Yup.
I see few ways to resolve this:
* write better wm97xx interface. Dunno if that is feasible or possible.
One of the possible solutions is to pass battery and ts device names
and data from within board data via ac97 layer to wm97xx-core. This
will provide several benefits (e.g. then we can drop lots of parameters
from wm97xx-core, which are really board parameters).
* Merge most of functionality of wm97xx-battery driver for Palms (by Marek),
my tosa battery driver, name that The Only wm97xx-battery Driver. However
on this path we will have to put lot's of code into hooks, which is
pretty much
ugly IMO.
--
With best wishes
Dmitry
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 <[email protected]>
>> Cc: Marek Vasut <[email protected]>
>> ---
>> 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
On Mon, Nov 03, 2008 at 07:41:47PM +0300, Dmitry wrote:
[...]
> >> >> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
> >> >> index 2eab35a..4e52c22 100644
> >> >> --- a/drivers/power/tosa_battery.c
> >> >> +++ b/drivers/power/tosa_battery.c
> >> >> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
> >> >>
> >> >> static int __init tosa_bat_init(void)
> >> >> {
> >> >> + if (!machine_is_tosa())
> >> >> + return -EINVAL;
> >> >> +
> >> >
> >> > I tend to reject this approach. You should rename the driver instead.
> >> >
> >> > I.e.
> >> > -.driver.name = "wm97xx-battery",
> >> > -.driver.name = "tosa-battery",
> >> >
> >> > And make sure that this won't break users of that driver (though
> >> > I don't see any).
> >>
> >> The wm97xx-battery device is registered by
> >> drivers/input/toucscreen/wm97xx-core.c
> >
> > Hmm.. So you can't rename the tosa driver, because then it won't
> > work, correct?
>
> Yup.
> I see few ways to resolve this:
> * write better wm97xx interface. Dunno if that is feasible or possible.
> One of the possible solutions is to pass battery and ts device names
> and data from within board data via ac97 layer to wm97xx-core. This
> will provide several benefits (e.g. then we can drop lots of parameters
> from wm97xx-core, which are really board parameters).
That would be great indeed. But for now, just don't compile the two
drivers on the tosa platform. No need for machine_is_*() hacks...
Thanks,
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
2008/11/3 Anton Vorontsov <[email protected]>:
> On Mon, Nov 03, 2008 at 07:41:47PM +0300, Dmitry wrote:
> [...]
>> >> >> diff --git a/drivers/power/tosa_battery.c b/drivers/power/tosa_battery.c
>> >> >> index 2eab35a..4e52c22 100644
>> >> >> --- a/drivers/power/tosa_battery.c
>> >> >> +++ b/drivers/power/tosa_battery.c
>> >> >> @@ -469,6 +469,9 @@ static struct platform_driver tosa_bat_driver = {
>> >> >>
>> >> >> static int __init tosa_bat_init(void)
>> >> >> {
>> >> >> + if (!machine_is_tosa())
>> >> >> + return -EINVAL;
>> >> >> +
>> >> >
>> >> > I tend to reject this approach. You should rename the driver instead.
>> >> >
>> >> > I.e.
>> >> > -.driver.name = "wm97xx-battery",
>> >> > -.driver.name = "tosa-battery",
>> >> >
>> >> > And make sure that this won't break users of that driver (though
>> >> > I don't see any).
>> >>
>> >> The wm97xx-battery device is registered by
>> >> drivers/input/toucscreen/wm97xx-core.c
>> >
>> > Hmm.. So you can't rename the tosa driver, because then it won't
>> > work, correct?
>>
>> Yup.
>> I see few ways to resolve this:
>> * write better wm97xx interface. Dunno if that is feasible or possible.
>> One of the possible solutions is to pass battery and ts device names
>> and data from within board data via ac97 layer to wm97xx-core. This
>> will provide several benefits (e.g. then we can drop lots of parameters
>> from wm97xx-core, which are really board parameters).
>
> That would be great indeed. But for now, just don't compile the two
> drivers on the tosa platform. No need for machine_is_*() hacks...
That is a bit strange requirement. During several past months there was
a lot of efforts put into supporting multi-machine PXA kernel images. And
this requirement goes against this.
--
With best wishes
Dmitry
On Mon, Nov 03, 2008 at 04:54:57PM +0000, Jonathan Cameron wrote:
[...]
> 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!)
Yeah, I understand that. But when you need simple driver for very
simple ADC device, the subsystem is a bit scary. Maybe we could just
implement "simple API" on top of it, that would hide the complexity.
Something like
value = adc_sample_pin(adc_device, "voltage");
And
struct adc_pin pins[2];
pins[0].name = "x-axis";
pins[0].num_samples = 5;
pins[1].name = "y-axis";
pins[2].num_samples = 5;
adc_sample_pins(adc_device, pins);
for (i = 0; i < ARRAY_SIZE(pins); i++) {
for (j = 0; j < pins[i].num_samples; i++)
process(pins[i].values[j].value);
}
That would work for most in-kernel ADC users (batteries, touchscreens).
> Anyhow, definitely not given up on it.
Great, looking forward to your patches.
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, Nov 03, 2008 at 08:09:14PM +0300, Dmitry wrote:
[...]
> >> Yup.
> >> I see few ways to resolve this:
> >> * write better wm97xx interface. Dunno if that is feasible or possible.
> >> One of the possible solutions is to pass battery and ts device names
> >> and data from within board data via ac97 layer to wm97xx-core. This
> >> will provide several benefits (e.g. then we can drop lots of parameters
> >> from wm97xx-core, which are really board parameters).
> >
> > That would be great indeed. But for now, just don't compile the two
> > drivers on the tosa platform. No need for machine_is_*() hacks...
>
> That is a bit strange requirement. During several past months there was
> a lot of efforts put into supporting multi-machine PXA kernel images.
Yes, I know this.
But you're implementing hacks instead of fixing the problem.
Btw, you inserted the machine_is() into the tosa driver, but
wm97xx driver registers unconditionally. That could be a problem
when wm97xx loads first, and then you try to load the tosa_battery
module on tosa machine.
Let's insert machine_is() hack into the wm97xx driver as well?
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
On Mon, Nov 03, 2008 at 07:41:47PM +0300, Dmitry wrote:
> 2008/11/3 Anton Vorontsov <[email protected]>:
> > Hmm.. So you can't rename the tosa driver, because then it won't
> > work, correct?
> * write better wm97xx interface. Dunno if that is feasible or possible.
> One of the possible solutions is to pass battery and ts device names
> and data from within board data via ac97 layer to wm97xx-core. This
> will provide several benefits (e.g. then we can drop lots of parameters
> from wm97xx-core, which are really board parameters).
There's only trivial writing required to do this in the WM97xx driver
itself - all that needs to be done is to allow the existing machine data
to be passed in as platform data and then add a couple of additional
hooks to that to allow instantation of things like the battery drivers
from machine code rather than doing it unconditionally as is done now.
The blocker currently is that it's not possible to pass any platform
data to an AC97 codec.
There was some work on doing that from Sebastian Siewior (CCed) but it
doesn't look like it ever made it to mainline. The last I saw of it[1]
it got stalled due to concerns about the difficulty in doing something
that is both type safe and general enough to cover complex cases like
the WM97xx parts that want to pass in lots of highly device and system
specific stuff. It did go into external ALSA for at least a while,
though.
It'd be really good to see some way of getting platform data to AC97
devices. It would make the WM97xx drivers a lot easier to use if we
could do it - I keep hoping to find the cycles to actively pursue this
myself but it's not happened yet.
[1] http://thread.gmane.org/gmane.linux.alsa.devel/52882
2008/11/3 Anton Vorontsov <[email protected]>:
> On Mon, Nov 03, 2008 at 08:09:14PM +0300, Dmitry wrote:
> [...]
>> >> Yup.
>> >> I see few ways to resolve this:
>> >> * write better wm97xx interface. Dunno if that is feasible or possible.
>> >> One of the possible solutions is to pass battery and ts device names
>> >> and data from within board data via ac97 layer to wm97xx-core. This
>> >> will provide several benefits (e.g. then we can drop lots of parameters
>> >> from wm97xx-core, which are really board parameters).
>> >
>> > That would be great indeed. But for now, just don't compile the two
>> > drivers on the tosa platform. No need for machine_is_*() hacks...
>>
>> That is a bit strange requirement. During several past months there was
>> a lot of efforts put into supporting multi-machine PXA kernel images.
>
> Yes, I know this.
>
> But you're implementing hacks instead of fixing the problem.
>
> Btw, you inserted the machine_is() into the tosa driver, but
> wm97xx driver registers unconditionally. That could be a problem
> when wm97xx loads first, and then you try to load the tosa_battery
> module on tosa machine.
>
> Let's insert machine_is() hack into the wm97xx driver as well?
That's why I suggested moving driver registration into _set_pdata()
function in that driver.
>
> --
> Anton Vorontsov
> email: [email protected]
> irc://irc.freenode.net/bd2
>
--
With best wishes
Dmitry
On Mon, Nov 03, 2008 at 06:11:51PM +0300, Anton Vorontsov wrote:
> On Wed, Oct 29, 2008 at 12:04:10PM +0300, Dmitry Baryshkov wrote:
> > #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...
This won't of course fix the wm97xx driver model, but the module issue
should be fixed. What about this patch?
>From f4b2e46cbac0d3ce77643c008d44fdaea8864366 Mon Sep 17 00:00:00 2001
From: Dmitry Baryshkov <[email protected]>
Date: Wed, 29 Oct 2008 12:00:57 +0300
Subject: [PATCH] power_supply: change the way how wm97xx-bat driver is registered
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 <[email protected]>
Cc: Marek Vasut <[email protected]>
Signed-off-by: Dmitry Baryshkov <[email protected]>
---
drivers/power/Kconfig | 4 ++--
drivers/power/wm97xx_battery.c | 12 +++++-------
include/linux/wm97xx_batt.h | 9 ++++++---
3 files changed, 13 insertions(+), 12 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..eb9cbf3 100644
--- a/include/linux/wm97xx_batt.h
+++ b/include/linux/wm97xx_batt.h
@@ -17,10 +17,13 @@ struct wm97xx_batt_info {
char *batt_name;
};
-#ifdef CONFIG_BATTERY_WM97XX
-void __init wm97xx_bat_set_pdata(struct wm97xx_batt_info *data);
+#if defined(CONFIG_BATTERY_WM97XX) || defined(CONFIG_BATTERY_WM97XX_MODULE)
+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
#endif
--
1.5.6.5
--
With best wishes
Dmitry
On Mon, Nov 03, 2008 at 09:29:55PM +0300, Dmitry Baryshkov wrote:
> On Mon, Nov 03, 2008 at 06:11:51PM +0300, Anton Vorontsov wrote:
> > On Wed, Oct 29, 2008 at 12:04:10PM +0300, Dmitry Baryshkov wrote:
> > > #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...
>
> This won't of course fix the wm97xx driver model, but the module issue
> should be fixed. What about this patch?
[...]
> -#ifdef CONFIG_BATTERY_WM97XX
> -void __init wm97xx_bat_set_pdata(struct wm97xx_batt_info *data);
> +#if defined(CONFIG_BATTERY_WM97XX) || defined(CONFIG_BATTERY_WM97XX_MODULE)
> +int wm97xx_bat_set_pdata(struct wm97xx_batt_info *data);
In case of CONFIG_BATTERY_WM97XX_MODULE, the build will break at link
time, since wm97xx_bat_set_pdata() might be called from the built-in
code (i.e. arch/arm/mach-pxa/palmtx.c).
> #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
>
> #endif
> --
> 1.5.6.5
--
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2