Re: PowerOP 1/3: PowerOP core

[Greg KH <greg@kroah.com>]

On Mon, Aug 08, 2005 at 07:51:57PM -0700, Todd Poynor wrote:
> +static void powerop_kobj_release(struct kobject *kobj)
> +{
> +	return;
> +}

Hint, if your release function is just a noop like this, your code is
wrong.  The kernel requires you to have a release function for a reason.
Please fix it.

> +struct powerop_param_attribute {
> +	int index;
> +        struct attribute        attr;
> +};

space vs. tab issue.

> +static ssize_t
> +powerop_param_attr_show(struct kobject * kobj, struct attribute * attr,
> +			char * buf)
> +{
> +	struct powerop_param_attribute * param_attr = to_param_attr(attr);
> +	struct powerop_point point;
> +	ssize_t ret = 0;
> +
> +	if ((ret = powerop_get_point(&point)) == 0)
> +		ret = sprintf(buf, "%d\n", point.param[param_attr->index]);

Please break this up into 3 lines instead of 2 to make it easier to read
and maintain over time.

You do this in other places too, please fix them too.

thanks,

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