PowerOP, PowerOP Core, 1/3
PATCH 1/1 PowerOP, PowerOP Core, 1/3
The PowerOP Core provides completely arch independent interface to create and control operating points which consist of arbitrary subset of power parameters available on a certain platform.
drivers/Kconfig +2 -0
@@ -74,4 +74,6 @@ source "drivers/rtc/Kconfig" source "drivers/dma/Kconfig" +source "drivers/powerop/Kconfig" + endmenu
drivers/Makefile +1 -0
@@ -65,6 +65,7 @@ obj-$(CONFIG_ISDN) += isdn/ obj-$(CONFIG_EDAC) += edac/ obj-$(CONFIG_MCA) += mca/ obj-$(CONFIG_EISA) += eisa/ +obj-$(CONFIG_POWEROP) += powerop/ obj-$(CONFIG_CPU_FREQ) += cpufreq/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_NEW_LEDS) += leds/
drivers/powerop/Kconfig new file +12 -0
@@ -0,0 +1,12 @@ +# +# powerop +# + +menu "PowerOP (Power Management)" + +config POWEROP + bool "PowerOP Core" + help + +endmenu +
drivers/powerop/Makefile new file +2 -0
@@ -0,0 +1,2 @@ +obj-$(CONFIG_POWEROP) += powerop.o +
drivers/powerop/powerop.c new file +454 -0
454 lines not on this page · raw message
include/linux/powerop.h new file +129 -0
129 lines not on this page · raw message
Quoted 1 line · Eugeny S. Mints
+#include <
linux/config.h>
Not needed anymore.
drivers/powerop/powerop.c line 26
Quoted 11 lines
drivers/powerop/powerop.c new file +11
-0+#include <linux/module.h> +#include <linux/init.h> +#include <linux/errno.h> +#include <linux/powerop.h> + +#define POWEROP_MAX_OPT_NAME_LENGTH 32 + +/* +* FIXME: temporary limit. next implementation will handle unlimited number +* of operating point +*/
Trailing spaces. Also in lots of other places in the patch, please remove them.
Quoted 1 line
+#define
POWEROP_MAX_OPT_NUMBER20
No tabs :(
drivers/powerop/powerop.c line 40
Quoted 13 lines
drivers/powerop/powerop.c new file +13
-0+/* current number of registered operating points */ +static int registered_opt_number; + +/* array of registered opereting point names */ +static char *registered_names[POWEROP_MAX_OPT_NUMBER ]; + +/* notifications about an operating point registration/deregistration */ +static BLOCKING_NOTIFIER_HEAD (powerop_notifier_list); + +struct powerop_point { +struct kobject kobj;/* hook to reference an operating point in +* some arch independent way +*/
What do you do with this kobject? It looks as if you only use the name portion of it, which seems like a big waste of memory for a whole kobject.
It's also the first time I've seen a struct kobject abused this way :)
drivers/powerop/powerop.c line 58
Quoted 2 lines
drivers/powerop/powerop.c new file +2
-0+static void * +create_point (const char *pwr_params,va_list args)
Return types on the same line please.
drivers/powerop/powerop.c line 65
Quoted 7 lines
drivers/powerop/powerop.c new file +7
-0+{ +void *res; + +down (&powerop_mutex); + res = powerop_driver && powerop_driver->create_point ? + powerop_driver->create_point (pwr_params, args) : +NULL ;
We do have the "if" statement in C. Please use it, you like this style a lot, but it's very hard to read for the majority of people.
Quoted 10 lines
+/*
+ *get_point- get value of specified power paramenter of operating
+ * point pointed by 'md_opt'
+ *
+ * INPUT:
+ *md_opt- pointer to operating point to be processed or NULL to get
+ * values of currently active operating point
+ *pwr_params- name of requested power parameters
+ *
+ * OUTPUT:
Quoted 8 lines
+ * none
+ *
+ * INPUT/OUTPUT:
+ * args - array of result placeholders
+ *
+ * RETURN:
+ * 0 on success, error code otherwise
+ */
What's with the wierd comment style? Either use kerneldoc, or nothing, don't invent your own INPUT, OUTPUT, RETURN, etc, style please.
drivers/powerop/powerop.c line 123
Quoted 9 lines
drivers/powerop/powerop.c new file +9
-0+/* PowerOP Core public interface */ + +int +powerop_driver_register (struct powerop_driver *p) +{ +int error =0 ; + +if (! powerop_driver) { +printk (KERN_INFO "PowerOP registering driver %s.\n" , p->name);
That's pretty noisy. Please make it a debugging thing only.
drivers/powerop/powerop.c line 127
Quoted 4 lines
drivers/powerop/powerop.c new file +4
-0+ powerop_driver = p; + + }else + error = -EBUSY ;
If you set error = -EBUSY on the first line of the function, these two lines can be dropped.
drivers/powerop/powerop.c line 136
Quoted 8 lines
drivers/powerop/powerop.c new file +8
-0+return error; +} + +void +powerop_driver_unregister (struct powerop_driver *p) +{ +if (powerop_driver == p) { +printk (KERN_INFO "PowerOP unregistering driver %s.\n" , p->name);
Same noise comment.
drivers/powerop/powerop.c line 142
Quoted 6 lines
drivers/powerop/powerop.c new file +6
-0+ powerop_driver =NULL ; + } +} + +EXPORT_SYMBOL_GPL (powerop_driver_register); +EXPORT_SYMBOL_GPL (powerop_driver_unregister);
But these after the individual functions please.
include/linux/powerop.h line 23
Quoted 6 lines
include/linux/powerop.h new file +6
-0+struct powerop_driver { +char *name; +void *(*create_point)(const char *pwr_params,va_list args); +int (*set_point)(void *md_opt); +int (*get_point)(void *md_opt,const char *pwr_params,va_list args); +};
Module owner perhaps too? You need to handle these drivers in modules properly with the correct usage counting.
Quoted 17 lines
include/linux/powerop.h new file +17
-0+ +int powerop_driver_register (struct powerop_driver *p); +void powerop_driver_unregister (struct powerop_driver *p); + +/* Main PowerOP Core interface */ + +/* +* powerop_register_point - add new operating point with a given name to +* operating points list. A caller passes power parameters for new operating +* points as pairs of name/value and passes only those parameter names the +* caller is interested in. PowerOP Core calls powerop driver to initialize +* arch dependent part of new operating point and links new named operating +* point to the list maintained by PowerOP Core +* +* +* INPUT +* id - operating point name
include/linux/powerop.h line 50
Quoted 10 lines
include/linux/powerop.h new file +10
-0+* pwr_params - set of (power parameter name, value) pairs +* +* OUTPUT +* none +* +* RETURN +* zero on success, error code otherwise +* +*/ +int powerop_register_point (const char *id,const char *pwr_params, ...);
Again with the wierd documentation style. Also, this does not belong in a .h file, kerneldoc can be generated from the .c files. Please do it that way instead of duplicating it twice.
thanks,
greg k-h
Greg,
Quoted 1 line · Greg KH
On Thu, Aug 24, 2006 at 05:10:47AM +0400, Eugeny S. Mints wrote:
[snip]
Quoted 11 lines
drivers/powerop/powerop.c line 40
Quoted 4 lines
drivers/powerop/powerop.c new file +4
-0+struct powerop_point { +struct kobject kobj;/* hook to reference an operating point in +* some arch independent way +*/ What do you do with this kobject? It looks as if you only use the name portion of it, which seems like a big waste of memory for a whole kobject.
It's also the first time I've seen a struct kobject abused this way :)
it is a shim for sysfs interface which is now implemented in the next take of the patchset.
Quoted 4 lines
drivers/powerop/powerop.c line 58
Quoted 2 lines
drivers/powerop/powerop.c new file +2
-0+static void * +create_point (const char *pwr_params,va_list args)Return types on the same line please.
I ran lindent script and now return types locate in the same line. Personally I'd prefer to have it as I put it since such type of coding allows to search a function _implementation_ much simpler by "grep ^function_name" and to distinguish implementation from function declaration/forward declaration/calls and similar function names. Isn't it a purpose of coding style among others? [snip]
Quoted 9 lines
include/linux/powerop.h line 23
Quoted 6 lines
include/linux/powerop.h new file +6
-0+struct powerop_driver { +char *name; +void *(*create_point)(const char *pwr_params,va_list args); +int (*set_point)(void *md_opt); +int (*get_point)(void *md_opt,const char *pwr_params,va_list args); +};Module owner perhaps too? You need to handle these drivers in modules properly with the correct usage counting.
Current design assumes only one powerop driver in the system and powerop core checks pointers to the routines against NULL value before every call. PowerOP core does not share any data with powerop driver so I don't see the reason to lock powerop driver in this case.
Eugeny
Quoted 36 lines
Quoted 17 lines
include/linux/powerop.h new file +17
-0+ +int powerop_driver_register (struct powerop_driver *p); +void powerop_driver_unregister (struct powerop_driver *p); + +/* Main PowerOP Core interface */ + +/* +* powerop_register_point - add new operating point with a given name to +* operating points list. A caller passes power parameters for new operating +* points as pairs of name/value and passes only those parameter names the +* caller is interested in. PowerOP Core calls powerop driver to initialize +* arch dependent part of new operating point and links new named operating +* point to the list maintained by PowerOP Core +* +* +* INPUT +* id - operating point name include/linux/powerop.h line 50
Quoted 10 lines
include/linux/powerop.h new file +10
-0+* pwr_params - set of (power parameter name, value) pairs +* +* OUTPUT +* none +* +* RETURN +* zero on success, error code otherwise +* +*/ +int powerop_register_point (const char *id,const char *pwr_params, ...);Again with the wierd documentation style. Also, this does not belong in a .h file, kerneldoc can be generated from the .c files. Please do it that way instead of duplicating it twice.
thanks,
greg k-h
Quoted 14 lines · Eugeny S. Mints
Greg,
Quoted 1 line · Greg KH
On Thu, Aug 24, 2006 at 05:10:47AM +0400, Eugeny S. Mints wrote:
[snip]
Quoted 9 lines
drivers/powerop/powerop.c line 40
Quoted 4 lines
drivers/powerop/powerop.c new file +4
-0+struct powerop_point { +struct kobject kobj;/* hook to reference an operating point in +* some arch independent way +*/ What do you do with this kobject? It looks as if you only use the name portion of it, which seems like a big waste of memory for a whole kobject.
Quoted 4 lines
Quoted 2 lines
It's also the first time I've seen a struct kobject abused this way :)
it is a shim for sysfs interface which is now implemented in the next take of the patchset.
Ok, will now look.
Quoted 10 lines
Quoted 4 lines
drivers/powerop/powerop.c line 58
Quoted 2 lines
drivers/powerop/powerop.c new file +2
-0+static void * +create_point (const char *pwr_params,va_list args)Return types on the same line please.
I ran lindent script and now return types locate in the same line. Personally I'd prefer to have it as I put it since such type of coding allows to search a function _implementation_ much simpler by "grep ^function_name" and to distinguish implementation from function declaration/forward declaration/calls and similar function names. Isn't it a purpose of coding style among others?
No, that's what ctags/cscope/whatever_you_like is for. Copy the code that Linus wrote in the core kernel to get the coding style correct.
Quoted 11 lines
[snip]
Quoted 10 lines
Quoted 7 lines
include/linux/powerop.h new file +5
-0+struct powerop_driver { +char *name; +void *(*create_point)(const char *pwr_params,va_list args); +int (*set_point)(void *md_opt); + int (*get_point)(void *md_opt, const char *pwr_params, va_listargs);
+};Module owner perhaps too? You need to handle these drivers in modules properly with the correct usage counting.
Quoted 3 lines
Current design assumes only one powerop driver in the system and powerop core
checks pointers to the routines against NULL value before every call.
What happens within the call to the module? module unload happens then, and _boom_. You get to keep the pieces of your kernel...
Please, if you are going to call into ANY code in a module, merely checking for NULL is good to make sure the call works, but you had better lock down that code and keep it from being unloaded when you make the call. That is what the module reference counting logic is for.
Quoted 2 lines
PowerOP core does not share any data with powerop driver so I don't see the reason to lock powerop driver in this case.
It's not a "data" issue, module reference counting has nothing to do with data (many people get that incorrect.) It's all about the codepaths.
So, you use kobjects and the like to reference count your data, and module reference counts on the code. Hopefully the two interconnect properly so you don't have data hanging around longer than the module...
Hope this helps,
greg k-h