Re: [PATCH v2 7/7] thermal: export thermal_zone_parameters to sysfs

[Javi Merino <javi.merino@arm.com>]

On Thu, Feb 26, 2015 at 10:10:07PM +0000, Eduardo Valentin wrote:
> On Fri, Feb 27, 2015 at 05:19:05PM +0000, Javi Merino wrote:
> > On Thu, Feb 26, 2015 at 10:04:24PM +0000, Eduardo Valentin wrote:
> > > On Thu, Feb 26, 2015 at 05:30:00PM -0400, Eduardo Valentin wrote:
> > > > On Thu, Feb 26, 2015 at 07:00:33PM +0000, Javi Merino wrote:

[snip]

> > > > > +
> > > > > +static ssize_t
> > > > > +sustainable_power_show(struct device *dev, struct device_attribute *devattr,
> > > > > +		       char *buf)
> > > > > +{
> > > > > +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> > > > > +
> > > > > +	if (tz->tzp)
> > > > > +		return sprintf(buf, "%u\n", tz->tzp->sustainable_power);
> > > > > +	else
> > > > > +		return -EIO;
> > > > > +}
> > > > > +
> > > > > +static ssize_t
> > > > > +sustainable_power_store(struct device *dev, struct device_attribute *devattr,
> > > > > +			const char *buf, size_t count)
> > > > > +{
> > > > > +	struct thermal_zone_device *tz = to_thermal_zone(dev);
> > > > > +	u32 sustainable_power;
> > > > > +
> > > > > +	if (!tz->tzp)
> > > > > +		return -EIO;
> > > > > +
> > > > > +	if (kstrtou32(buf, 10, &sustainable_power))
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	tz->tzp->sustainable_power = sustainable_power;
> > > > > +
> > > > > +	return count;
> > > > > +}
> > > > > +static DEVICE_ATTR(sustainable_power, S_IWUSR | S_IRUGO, sustainable_power_show,
> > > > > +		sustainable_power_store);
> > > > > +
> > > > > +#define create_s32_tzp_attr(name)					\
> > > > > +	static ssize_t							\
> > > > > +	name##_show(struct device *dev, struct device_attribute *devattr, \
> > > > > +		char *buf)						\
> > > > > +	{								\
> > > > > +	struct thermal_zone_device *tz = to_thermal_zone(dev);		\
> > > > > +									\
> > > > > +	if (tz->tzp)							\
> > > > > +		return sprintf(buf, "%u\n", tz->tzp->name);		\
> > > > > +	else								\
> > > > > +		return -EIO;						\
> > > > > +	}								\
> > > > > +									\
> > > > > +	static ssize_t							\
> > > > > +	name##_store(struct device *dev, struct device_attribute *devattr, \
> > > > > +		const char *buf, size_t count)				\
> > > > > +	{								\
> > > > > +		struct thermal_zone_device *tz = to_thermal_zone(dev);	\
> > > > > +		s32 value;						\
> > > > > +									\
> > > > > +		if (!tz->tzp)						\
> > > > > +			return -EIO;					\
> > > > > +									\
> > > > > +		if (kstrtos32(buf, 10, &value))				\
> > > > > +			return -EINVAL;					\
> > > > > +									\
> > > > > +		tz->tzp->name = value;					\
> > > > > +									\
> > > > > +		return count;						\
> > > > > +	}								\
> > > > > +	static DEVICE_ATTR(name, S_IWUSR | S_IRUGO, name##_show, name##_store)
> > > > > +
> > > > > +create_s32_tzp_attr(k_po);
> > > > > +create_s32_tzp_attr(k_pu);
> > > > > +create_s32_tzp_attr(k_i);
> > > > > +create_s32_tzp_attr(k_d);
> > > > > +create_s32_tzp_attr(integral_cutoff);
> > > > > +#undef create_s32_tzp_attr
> > > > > +
> > > > > +static struct device_attribute *dev_tzp_attrs[] = {
> > > > > +	&dev_attr_sustainable_power,
> > > > > +	&dev_attr_k_po,
> > > > > +	&dev_attr_k_pu,
> > > > > +	&dev_attr_k_i,
> > > > > +	&dev_attr_k_d,
> > > > > +	&dev_attr_integral_cutoff,
> > > > > +};
> > > > > +
> > > > > +static int create_power_allocator_tzp_attrs(struct device *dev)
> > > 
> > > I would rename this to thermal_create_zone_params_attrs and remove the
> > > ifdefiry.
> > 
> > Ok, I will remove them.
> > 
> > >           If you are not exposing the complete info under tzp, then make
> > > a comment about it.
> > 
> > There's nothing else worth populating.  The governor_name is not
> > interesting, you have the thermal zone policy for that.  Similar for
> > no_hwmon.  For tbps, weight and trip_mask are already exposed in the
> > instance.  The binding_limits is currently not available in sysfs, but
> > if we were to populate it, I would expose the ones that end up in the
> > instance, as we do with trips and weights.
> > 
> > Is it worth putting this in a comment in the code?
> 
> 
> Well, I was more interested in governor specific / internal data,
> accessible only in the governor file (e.g.: power_allocator.c). Do you
> think we need to expose something that is accessible only from the
> governor code or do you think the current info under tzp is enough?
> 
> 
> If the former is the case, then we need restructure with callbacks. If
> not, meaning, if all you need is in tzp, then we can keep the code in
> thermal core.

I don't want to expose anything from the governor file, I think the
current info under tzp is enough.

Cheers,
Javi