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

Javi Merino
2015-03-02T14:17:54Z

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