Re: [RFC v01 2/3] PowerCap: Add class driver

[Srinivas Pandruvada]

On 08/02/2013 03:43 PM, Joe Perches wrote:
> On Fri, 2013-08-02 at 11:08 -0700, Srinivas Pandruvada wrote:
> > Added power cap class driver, which provides an API for client drivers
> > to use and provide a consistant sys-fs interface to user mode.
> Just some stylistic notes.
>
> > diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
> []
> > +/* Power zone show function */
> > +#define define_device_show(_attr)		\
> > +static ssize_t show_##_attr(struct device *dev, struct device_attribute *attr,\
> > +			char *buf) \
> Using _attr and another variable named attr is at least
> visually confusing.
>
> Maybe use name or type instead of _attr?
>
> > +{ \
> > +	u64 value; \
> > +	ssize_t len = -EINVAL; \
> > +	struct powercap_zone_device *pcd_dev = dev_get_drvdata(dev); \
> > +	\
> > +	if (pcd_dev && pcd_dev->ops && pcd_dev->ops->get_##_attr) { \
> > +		mutex_lock(&pcd_dev->lock); \
> > +		if (!pcd_dev->ops->get_##_attr(pcd_dev, &value)) \
> > +			len = sprintf(buf, "%lld\n", value); \
> > +		mutex_unlock(&pcd_dev->lock); \
> > +	} \
> > +	\
> > +	return len; \
> > +}
> > +
> > +/* Power zone store function; only reset is possible */
> > +#define define_device_store(_attr)		\
> > +static ssize_t store_##_attr(struct device *dev,\
> > +				struct device_attribute *attr, \
> > +				const char *buf, size_t count) \
> here too
>
> []
>
> > +/* Power zone constraint show function */
> > +#define define_device_constraint_show(_attr) \
> > +static ssize_t show_constraint_##_attr(struct device *dev, \
> > +				struct device_attribute *attr,\
> > +				char *buf) \
> and here
>
> []
>
> > +/* Power zone constraint store function */
> > +#define define_device_constraint_store(_attr) \
> > +static ssize_t store_constraint_##_attr(struct device *dev,\
> > +				struct device_attribute *attr, \
> > +				const char *buf, size_t count) \
> etc...
>
> > +struct powercap_zone_constraint *powercap_zone_add_constraint(
> > +				struct powercap_zone_device *pcd_dev,
> > +				struct powercap_zone_constraint_ops *ops)
> > +{
> []
> > +	dev_dbg(&pcd_dev->device, "powercap_zone_add_constraint\n");
> These logging messages aren't useful.
> function tracing works very well.
Thanks. Will change in the next patchset.

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