On 四, 2012-07-19 at 22:27 +0200, Rafael J. Wysocki wrote:
> On Thursday, July 19, 2012, Zhang Rui wrote:
> > From: Durgadoss R <dugardoss.r@xxxxxxxxx>
> >
> > Some of the thermal drivers using the Generic Thermal Framework
> > require (all/some) trip points to be writeable. This patch makes
> > the trip point temperatures writeable on a per-trip point basis,
> > and modifies the required function call in thermal.c. This patch
> > also updates the Documentation to reflect the new change.
> >
> > Signed-off-by: Durgadoss R <durgadoss.r@xxxxxxxxx>
> > Signed-off-by: Zhang Rui <rui.zhang@xxxxxxxxx>
> > ---
> > Documentation/thermal/sysfs-api.txt | 4 +-
> > drivers/acpi/thermal.c | 4 +-
> > drivers/platform/x86/acerhdf.c | 2 +-
> > drivers/platform/x86/intel_mid_thermal.c | 2 +-
> > drivers/thermal/spear_thermal.c | 2 +-
> > drivers/thermal/thermal_sys.c | 147 +++++++++++++++++++++---------
> > include/linux/thermal.h | 15 ++-
> > 7 files changed, 125 insertions(+), 51 deletions(-)
> >
> > diff --git a/Documentation/thermal/sysfs-api.txt b/Documentation/thermal/sysfs-api.txt
> > index 1733ab9..0c7c423 100644
> > --- a/Documentation/thermal/sysfs-api.txt
> > +++ b/Documentation/thermal/sysfs-api.txt
> > @@ -32,7 +32,8 @@ temperature) and throttle appropriate devices.
> >
> > 1.1 thermal zone device interface
> > 1.1.1 struct thermal_zone_device *thermal_zone_device_register(char *name,
> > - int trips, void *devdata, struct thermal_zone_device_ops *ops)
> > + int trips, int flag, void *devdata,
>
> The 'flags' argument should rather be called 'mask', or even 'writable_mask'.
>
Agreed.
> > #define to_cooling_device(_dev) \
> > container_of(_dev, struct thermal_cooling_device, device)
> > @@ -1089,9 +1084,83 @@ void thermal_zone_device_update(struct thermal_zone_device *tz)
> > EXPORT_SYMBOL(thermal_zone_device_update);
> > > /**
> > + * create_trip_attrs - create attributes for trip points
> > + * @tz: the thermal zone device
> > + * @flag: Writeable trip point bitmap.
> > + */
> > +static int create_trip_attrs(struct thermal_zone_device *tz, int flag)
> > +{
> > + int indx;
> > + int writeable;
> > +
> > + tz->trip_type_attrs =
> > + kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
> > + if (!tz->trip_type_attrs)
> > + return -ENOMEM;
> > +
> > + tz->trip_temp_attrs =
> > + kzalloc(sizeof(struct thermal_attr) * tz->trips, GFP_KERNEL);
> > + if (!tz->trip_temp_attrs) {
> > + kfree(tz->trip_type_attrs);
> > + return -ENOMEM;
> > + }
> > +
> > + for (indx = 0; indx < tz->trips; indx++) {
> > +
> > + /* create trip type attribute */
> > + snprintf(tz->trip_type_attrs[indx].name, THERMAL_NAME_LENGTH,
> > + "trip_point_%d_type", indx);
>
> If this really truncates the name, we'll run into problems in _show() and _store().
> Is there any solution to that?
>
as THERMAL_MAX_TRIP is smaller than 100, so the answer is no.
> > +
> > + sysfs_attr_init(&tz->trip_type_attrs[count].attr.attr);
> > + tz->trip_type_attrs[indx].attr.attr.name =
> > + tz->trip_type_attrs[indx].name;
> > + tz->trip_type_attrs[indx].attr.attr.mode = S_IRUGO;
> > + tz->trip_type_attrs[indx].attr.show = trip_point_type_show;
> > +
> > + device_create_file(&tz->device,
> > + &tz->trip_type_attrs[indx].attr);
> > +
> > + /* create trip temp attribute */
> > + writeable = flag & (1 << indx);
> > + snprintf(tz->trip_temp_attrs[indx].name, THERMAL_NAME_LENGTH,
> > + "trip_point_%d_temp", indx);
> > +
> > + sysfs_attr_init(&tz->trip_temp_attrs[indx].attr.attr);
> > + tz->trip_temp_attrs[indx].attr.attr.name =
> > + tz->trip_temp_attrs[indx].name;
> > + tz->trip_temp_attrs[indx].attr.attr.mode = S_IRUGO;
> > + tz->trip_temp_attrs[indx].attr.show = trip_point_temp_show;
> > + if (writeable) {
>
> Why don't you put the "flag & (1 << indx)" here directly instead of 'writable'?
>
agreed.
> > + tz->trip_temp_attrs[indx].attr.attr.mode |= S_IWUSR;
> > + tz->trip_temp_attrs[indx].attr.store =
> > + trip_point_temp_store;
> > + }
> > +
> > + device_create_file(&tz->device,
> > + &tz->trip_temp_attrs[indx].attr);
> > + }
> > + return 0;
> > +}
> > +
> > +static void remove_trip_attrs(struct thermal_zone_device *tz)
> > +{
> > + int indx;
> > +
> > + for (indx = 0; indx < tz->trips; indx++) {
> > + device_remove_file(&tz->device,
> > + &tz->trip_type_attrs[indx].attr);
> > + device_remove_file(&tz->device,
> > + &tz->trip_temp_attrs[indx].attr);
> > + }
> > + kfree(tz->trip_type_attrs);
> > + kfree(tz->trip_temp_attrs);
> > +}
> > +
> > +/**
> > * thermal_zone_device_register - register a new thermal zone device
> > * @type: the thermal zone device type
> > * @trips: the number of trip points the thermal zone support
> > + * @flag: a bit string indicating the writeablility of trip points
> > * @devdata: private device data
> > * @ops: standard thermal zone device callbacks
> > * @tc1: thermal coefficient 1 for passive calculations
> > @@ -1107,7 +1176,7 @@ EXPORT_SYMBOL(thermal_zone_device_update);
> > * section 11.1.5.1 of the ACPI specification 3.0.
> > */
> > struct thermal_zone_device *thermal_zone_device_register(char *type,
> > - int trips, void *devdata,
> > + int trips, int flag, void *devdata,
> > const struct thermal_zone_device_ops *ops,
> > int tc1, int tc2, int passive_delay, int polling_delay)
> > {
> > @@ -1124,6 +1193,9 @@ struct thermal_zone_device *thermal_zone_device_register(char *type,
> > if (trips > THERMAL_MAX_TRIPS || trips < 0)
> > return ERR_PTR(-EINVAL);
> >
> > + if (flag >> trips)
> > + return ERR_PTR(-EINVAL);
>
> I'm not sure if this check is necessary. Does it actually matter is someone
> passes ones in the unused bit positions?
>
I do not think we can trust this value if the caller even wants to set
writable mask for a non-exist trip point.
thanks,
rui
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
