Re: [PATCH] PowerOP, PowerOP Core, 1/3

[Greg KH <greg@kroah.com>]

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