On Thu, Aug 24, 2006 at 05:10:47AM +0400, Eugeny S. Mints wrote:
> +#include <linux/config.h>
Not needed anymore.
> +#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.
> +#define POWEROP_MAX_OPT_NUMBER 20
No tabs :(
> +/* 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 :)
> +static void *
> +create_point(const char *pwr_params, va_list args)
Return types on the same line please.
> +{
> + 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.
> +/*
> + * 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:
> + * 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.
> +/* 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.
> + powerop_driver = p;
> +
> + } else
> + error = -EBUSY;
If you set error = -EBUSY on the first line of the function, these two
lines can be dropped.
> + 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.
> + powerop_driver = NULL;
> + }
> +}
> +
> +EXPORT_SYMBOL_GPL(powerop_driver_register);
> +EXPORT_SYMBOL_GPL(powerop_driver_unregister);
But these after the individual functions please.
> +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.
> +
> +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
> + * 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
