| Message ID | 1416500488-7232-4-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
Hello Lukasz, On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > This patch extends the of-thermal.c to export copy of trip points for > a given thermal zone. > > Thermal drivers should use of_thermal_get_trip_points() method to get > pointer to table of thermal trip points. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > Changes for v2: > - New patch - as suggested by Eduardo Valentin > --- > drivers/thermal/of-thermal.c | 33 +++++++++++++++++++++++++++++++++ > drivers/thermal/thermal_core.h | 7 +++++++ > include/linux/thermal.h | 14 ++++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 336af7f..33921c5 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -89,6 +89,7 @@ struct __thermal_zone { > /* trip data */ > int ntrips; > struct __thermal_trip *trips; > + struct thermal_trip *gtrips; > > /* cooling binding data */ > int num_tbps; > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip) > return true; > } > > +/** > + * of_thermal_get_trip_points - function to get access to a globally exported > + * trip points > + * > + * @tz: pointer to a thermal zone > + * > + * This function provides a pointer to the copy of trip points table > + * > + * Return: pointer to trip points table, NULL otherwise > + */ > +const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *tz) > +{ > + struct __thermal_zone *data = tz->devdata; > + > + if (!data) > + return NULL; > + > + return data->gtrips; > +} > + EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, > enum thermal_trend *trend) > { > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np) > goto free_tbps; > } > > + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL); > + if (!tz->gtrips) { > + ret = -ENOMEM; > + goto free_tbps; > + } > + > + for (i = 0; i < tz->ntrips; i++) > + memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature), > + sizeof(*tz->gtrips)); > + > finish: > of_node_put(child); > tz->mode = THERMAL_DEVICE_DISABLED; > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz) > { > int i; > > + kfree(tz->gtrips); > for (i = 0; i < tz->num_tbps; i++) > of_node_put(tz->tbps[i].cooling_device); > kfree(tz->tbps); > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index 466208c..a9580ca 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > void of_thermal_destroy_zones(void); > int of_thermal_get_ntrips(struct thermal_zone_device *); > bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > +const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *); > #else > static inline int of_parse_thermal_zones(void) { return 0; } > static inline void of_thermal_destroy_zones(void) { } > @@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int) > { This produces compilation error when CONFIG_THERMAL_OF is not set. Name the parameters to fix. > return 0; > } > +static inline const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *) > +{ > + return NULL; > +} > #endif > > #endif /* __THERMAL_CORE_H__ */ > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 5bc28a7..88d7249 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > int (*get_trend)(void *, long *); > }; > > +/** > + * struct thermal_trip - Structure representing themal trip points exported from > + * of-thermal > + * The only problem I have with this name is that would look like it is in use in all thermal framework, which is not really the case. But I do think having a type here is a good thing. So, not sure :-) > + * @temperature: trip point temperature > + * @hysteresis: trip point hysteresis > + * @type: trip point type > + */ > +struct thermal_trip { > + unsigned long int temperature; > + unsigned long int hysteresis; > + enum thermal_trip_type type; > +}; > + > /* Function declarations */ > #ifdef CONFIG_THERMAL_OF > struct thermal_zone_device * > -- > 2.0.0.rc2 >
On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > This patch extends the of-thermal.c to export copy of trip points for > a given thermal zone. > > Thermal drivers should use of_thermal_get_trip_points() method to get > pointer to table of thermal trip points. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > Changes for v2: > - New patch - as suggested by Eduardo Valentin > --- > drivers/thermal/of-thermal.c | 33 +++++++++++++++++++++++++++++++++ > drivers/thermal/thermal_core.h | 7 +++++++ > include/linux/thermal.h | 14 ++++++++++++++ > 3 files changed, 54 insertions(+) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 336af7f..33921c5 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -89,6 +89,7 @@ struct __thermal_zone { > /* trip data */ > int ntrips; > struct __thermal_trip *trips; > + struct thermal_trip *gtrips; > > /* cooling binding data */ > int num_tbps; > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip) > return true; > } > > +/** > + * of_thermal_get_trip_points - function to get access to a globally exported > + * trip points > + * > + * @tz: pointer to a thermal zone > + * > + * This function provides a pointer to the copy of trip points table > + * > + * Return: pointer to trip points table, NULL otherwise > + */ > +const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *tz) Another thing, can you please check why scripts/kernel-doc does not like this prototype? It throws an warn: Warning(drivers/thermal/of-thermal.c:168): cannot understand function prototype: 'const struct thermal_trip * const of_thermal_get_trip_points(struct thermal_zone_device *tz) ' > +{ > + struct __thermal_zone *data = tz->devdata; > + > + if (!data) > + return NULL; > + > + return data->gtrips; > +} > + > static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, > enum thermal_trend *trend) > { > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np) > goto free_tbps; > } > > + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL); > + if (!tz->gtrips) { > + ret = -ENOMEM; > + goto free_tbps; > + } > + > + for (i = 0; i < tz->ntrips; i++) > + memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature), > + sizeof(*tz->gtrips)); > + > finish: > of_node_put(child); > tz->mode = THERMAL_DEVICE_DISABLED; > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz) > { > int i; > > + kfree(tz->gtrips); > for (i = 0; i < tz->num_tbps; i++) > of_node_put(tz->tbps[i].cooling_device); > kfree(tz->tbps); > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index 466208c..a9580ca 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > void of_thermal_destroy_zones(void); > int of_thermal_get_ntrips(struct thermal_zone_device *); > bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > +const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *); > #else > static inline int of_parse_thermal_zones(void) { return 0; } > static inline void of_thermal_destroy_zones(void) { } > @@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int) > { > return 0; > } > +static inline const struct thermal_trip * const > +of_thermal_get_trip_points(struct thermal_zone_device *) > +{ > + return NULL; > +} > #endif > > #endif /* __THERMAL_CORE_H__ */ > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > index 5bc28a7..88d7249 100644 > --- a/include/linux/thermal.h > +++ b/include/linux/thermal.h > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > int (*get_trend)(void *, long *); > }; > > +/** > + * struct thermal_trip - Structure representing themal trip points exported from > + * of-thermal > + * > + * @temperature: trip point temperature > + * @hysteresis: trip point hysteresis > + * @type: trip point type > + */ > +struct thermal_trip { > + unsigned long int temperature; > + unsigned long int hysteresis; > + enum thermal_trip_type type; > +}; > + > /* Function declarations */ > #ifdef CONFIG_THERMAL_OF > struct thermal_zone_device * > -- > 2.0.0.rc2 >
Hi Eduardo, > Hello Lukasz, > > On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > > This patch extends the of-thermal.c to export copy of trip points > > for a given thermal zone. > > > > Thermal drivers should use of_thermal_get_trip_points() method to > > get pointer to table of thermal trip points. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > Changes for v2: > > - New patch - as suggested by Eduardo Valentin > > --- > > drivers/thermal/of-thermal.c | 33 > > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | > > 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ > > 3 files changed, 54 insertions(+) > > > > diff --git a/drivers/thermal/of-thermal.c > > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 > > --- a/drivers/thermal/of-thermal.c > > +++ b/drivers/thermal/of-thermal.c > > @@ -89,6 +89,7 @@ struct __thermal_zone { > > /* trip data */ > > int ntrips; > > struct __thermal_trip *trips; > > + struct thermal_trip *gtrips; > > > > /* cooling binding data */ > > int num_tbps; > > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct > > thermal_zone_device *tz, int trip) return true; > > } > > > > +/** > > + * of_thermal_get_trip_points - function to get access to a > > globally exported > > + * trip points > > + * > > + * @tz: pointer to a thermal zone > > + * > > + * This function provides a pointer to the copy of trip points > > table > > + * > > + * Return: pointer to trip points table, NULL otherwise > > + */ > > +const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *tz) > > +{ > > + struct __thermal_zone *data = tz->devdata; > > + > > + if (!data) > > + return NULL; > > + > > + return data->gtrips; > > +} > > + > > EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); Ok. > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > int trip, enum thermal_trend *trend) > > { > > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct > > device_node *np) goto free_tbps; > > } > > > > + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), > > GFP_KERNEL); > > + if (!tz->gtrips) { > > + ret = -ENOMEM; > > + goto free_tbps; > > + } > > + > > + for (i = 0; i < tz->ntrips; i++) > > + memcpy(&(tz->gtrips[i]), > > &(tz->trips[i].temperature), > > + sizeof(*tz->gtrips)); > > + > > finish: > > of_node_put(child); > > tz->mode = THERMAL_DEVICE_DISABLED; > > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct > > __thermal_zone *tz) { > > int i; > > > > + kfree(tz->gtrips); > > for (i = 0; i < tz->num_tbps; i++) > > of_node_put(tz->tbps[i].cooling_device); > > kfree(tz->tbps); > > diff --git a/drivers/thermal/thermal_core.h > > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 > > --- a/drivers/thermal/thermal_core.h > > +++ b/drivers/thermal/thermal_core.h > > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > > void of_thermal_destroy_zones(void); > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > > +const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *); > > #else > > static inline int of_parse_thermal_zones(void) { return 0; } > > static inline void of_thermal_destroy_zones(void) { } > > @@ -102,6 +104,11 @@ static inline bool > > of_thermal_is_trip_en(struct thermal_zone_device *, int) { > > This produces compilation error when CONFIG_THERMAL_OF is not set. > Name the parameters to fix. As all the other cases, I will fix that. > > > return 0; > > } > > +static inline const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *) > > +{ > > + return NULL; > > +} > > #endif > > > > #endif /* __THERMAL_CORE_H__ */ > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > index 5bc28a7..88d7249 100644 > > --- a/include/linux/thermal.h > > +++ b/include/linux/thermal.h > > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > > int (*get_trend)(void *, long *); > > }; > > > > +/** > > + * struct thermal_trip - Structure representing themal trip points > > exported from > > + * of-thermal > > + * > > The only problem I have with this name is that would look like it is > in use in all thermal framework, which is not really the case. But I > do think having a type here is a good thing. So, not sure :-) It can stay to be struct thermal_trip or we can rename it to struct of_thermal_trip. I'm fine with both names. > > > + * @temperature: trip point temperature > > + * @hysteresis: trip point hysteresis > > + * @type: trip point type > > + */ > > +struct thermal_trip { > > + unsigned long int temperature; > > + unsigned long int hysteresis; > > + enum thermal_trip_type type; > > +}; > > + > > /* Function declarations */ > > #ifdef CONFIG_THERMAL_OF > > struct thermal_zone_device * > > -- > > 2.0.0.rc2 > >
Hi Eduardo, > On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > > This patch extends the of-thermal.c to export copy of trip points > > for a given thermal zone. > > > > Thermal drivers should use of_thermal_get_trip_points() method to > > get pointer to table of thermal trip points. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > Changes for v2: > > - New patch - as suggested by Eduardo Valentin > > --- > > drivers/thermal/of-thermal.c | 33 > > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | > > 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ > > 3 files changed, 54 insertions(+) > > > > diff --git a/drivers/thermal/of-thermal.c > > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 > > --- a/drivers/thermal/of-thermal.c > > +++ b/drivers/thermal/of-thermal.c > > @@ -89,6 +89,7 @@ struct __thermal_zone { > > /* trip data */ > > int ntrips; > > struct __thermal_trip *trips; > > + struct thermal_trip *gtrips; > > > > /* cooling binding data */ > > int num_tbps; > > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct > > thermal_zone_device *tz, int trip) return true; > > } > > > > +/** > > + * of_thermal_get_trip_points - function to get access to a > > globally exported > > + * trip points > > + * > > + * @tz: pointer to a thermal zone > > + * > > + * This function provides a pointer to the copy of trip points > > table > > + * > > + * Return: pointer to trip points table, NULL otherwise > > + */ > > +const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *tz) > > Another thing, can you please check why scripts/kernel-doc does not > like this prototype? It throws an warn: > Warning(drivers/thermal/of-thermal.c:168): cannot understand function > prototype: 'const struct thermal_trip * const > of_thermal_get_trip_points(struct thermal_zone_device *tz) ' > I will check that. However the warning looks a bit strange. > > > +{ > > + struct __thermal_zone *data = tz->devdata; > > + > > + if (!data) > > + return NULL; > > + > > + return data->gtrips; > > +} > > + > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > int trip, enum thermal_trend *trend) > > { > > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct > > device_node *np) goto free_tbps; > > } > > > > + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), > > GFP_KERNEL); > > + if (!tz->gtrips) { > > + ret = -ENOMEM; > > + goto free_tbps; > > + } > > + > > + for (i = 0; i < tz->ntrips; i++) > > + memcpy(&(tz->gtrips[i]), > > &(tz->trips[i].temperature), > > + sizeof(*tz->gtrips)); > > + > > finish: > > of_node_put(child); > > tz->mode = THERMAL_DEVICE_DISABLED; > > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct > > __thermal_zone *tz) { > > int i; > > > > + kfree(tz->gtrips); > > for (i = 0; i < tz->num_tbps; i++) > > of_node_put(tz->tbps[i].cooling_device); > > kfree(tz->tbps); > > diff --git a/drivers/thermal/thermal_core.h > > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 > > --- a/drivers/thermal/thermal_core.h > > +++ b/drivers/thermal/thermal_core.h > > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > > void of_thermal_destroy_zones(void); > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > > +const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *); > > #else > > static inline int of_parse_thermal_zones(void) { return 0; } > > static inline void of_thermal_destroy_zones(void) { } > > @@ -102,6 +104,11 @@ static inline bool > > of_thermal_is_trip_en(struct thermal_zone_device *, int) { > > return 0; > > } > > +static inline const struct thermal_trip * const > > +of_thermal_get_trip_points(struct thermal_zone_device *) > > +{ > > + return NULL; > > +} > > #endif > > > > #endif /* __THERMAL_CORE_H__ */ > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > index 5bc28a7..88d7249 100644 > > --- a/include/linux/thermal.h > > +++ b/include/linux/thermal.h > > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > > int (*get_trend)(void *, long *); > > }; > > > > +/** > > + * struct thermal_trip - Structure representing themal trip points > > exported from > > + * of-thermal > > + * > > + * @temperature: trip point temperature > > + * @hysteresis: trip point hysteresis > > + * @type: trip point type > > + */ > > +struct thermal_trip { > > + unsigned long int temperature; > > + unsigned long int hysteresis; > > + enum thermal_trip_type type; > > +}; > > + > > /* Function declarations */ > > #ifdef CONFIG_THERMAL_OF > > struct thermal_zone_device * > > -- > > 2.0.0.rc2 > >
Hello, On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: > Hi Eduardo, > > > Hello Lukasz, > > > > On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > > > This patch extends the of-thermal.c to export copy of trip points > > > for a given thermal zone. > > > > > > Thermal drivers should use of_thermal_get_trip_points() method to > > > get pointer to table of thermal trip points. > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > --- > > > Changes for v2: > > > - New patch - as suggested by Eduardo Valentin > > > --- > > > drivers/thermal/of-thermal.c | 33 > > > +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | > > > 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ > > > 3 files changed, 54 insertions(+) > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 > > > --- a/drivers/thermal/of-thermal.c > > > +++ b/drivers/thermal/of-thermal.c > > > @@ -89,6 +89,7 @@ struct __thermal_zone { > > > /* trip data */ > > > int ntrips; > > > struct __thermal_trip *trips; > > > + struct thermal_trip *gtrips; > > > > > > /* cooling binding data */ > > > int num_tbps; > > > @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct > > > thermal_zone_device *tz, int trip) return true; > > > } > > > > > > +/** > > > + * of_thermal_get_trip_points - function to get access to a > > > globally exported > > > + * trip points > > > + * > > > + * @tz: pointer to a thermal zone > > > + * > > > + * This function provides a pointer to the copy of trip points > > > table > > > + * > > > + * Return: pointer to trip points table, NULL otherwise > > > + */ > > > +const struct thermal_trip * const > > > +of_thermal_get_trip_points(struct thermal_zone_device *tz) > > > +{ > > > + struct __thermal_zone *data = tz->devdata; > > > + > > > + if (!data) > > > + return NULL; > > > + > > > + return data->gtrips; > > > +} > > > + > > > > EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > > Ok. > > > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > > int trip, enum thermal_trend *trend) > > > { > > > @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct > > > device_node *np) goto free_tbps; > > > } > > > > > > + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), > > > GFP_KERNEL); > > > + if (!tz->gtrips) { > > > + ret = -ENOMEM; > > > + goto free_tbps; > > > + } > > > + > > > + for (i = 0; i < tz->ntrips; i++) > > > + memcpy(&(tz->gtrips[i]), > > > &(tz->trips[i].temperature), > > > + sizeof(*tz->gtrips)); > > > + > > > finish: > > > of_node_put(child); > > > tz->mode = THERMAL_DEVICE_DISABLED; > > > @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct > > > __thermal_zone *tz) { > > > int i; > > > > > > + kfree(tz->gtrips); > > > for (i = 0; i < tz->num_tbps; i++) > > > of_node_put(tz->tbps[i].cooling_device); > > > kfree(tz->tbps); > > > diff --git a/drivers/thermal/thermal_core.h > > > b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 > > > --- a/drivers/thermal/thermal_core.h > > > +++ b/drivers/thermal/thermal_core.h > > > @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > > > void of_thermal_destroy_zones(void); > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > > > +const struct thermal_trip * const > > > +of_thermal_get_trip_points(struct thermal_zone_device *); > > > #else > > > static inline int of_parse_thermal_zones(void) { return 0; } > > > static inline void of_thermal_destroy_zones(void) { } > > > @@ -102,6 +104,11 @@ static inline bool > > > of_thermal_is_trip_en(struct thermal_zone_device *, int) { > > > > This produces compilation error when CONFIG_THERMAL_OF is not set. > > Name the parameters to fix. > > As all the other cases, I will fix that. > > > > > > return 0; > > > } > > > +static inline const struct thermal_trip * const > > > +of_thermal_get_trip_points(struct thermal_zone_device *) > > > +{ > > > + return NULL; > > > +} > > > #endif > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > > index 5bc28a7..88d7249 100644 > > > --- a/include/linux/thermal.h > > > +++ b/include/linux/thermal.h > > > @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > > > int (*get_trend)(void *, long *); > > > }; > > > > > > +/** > > > + * struct thermal_trip - Structure representing themal trip points > > > exported from > > > + * of-thermal > > > + * > > > > The only problem I have with this name is that would look like it is > > in use in all thermal framework, which is not really the case. But I > > do think having a type here is a good thing. So, not sure :-) > > It can stay to be struct thermal_trip or we can rename it to > struct of_thermal_trip. > > I'm fine with both names. Leave it as 'thermal_trip'. > > > > > > + * @temperature: trip point temperature > > > + * @hysteresis: trip point hysteresis > > > + * @type: trip point type > > > + */ > > > +struct thermal_trip { > > > + unsigned long int temperature; > > > + unsigned long int hysteresis; > > > + enum thermal_trip_type type; > > > +}; > > > + > > > /* Function declarations */ > > > #ifdef CONFIG_THERMAL_OF > > > struct thermal_zone_device * > > > -- > > > 2.0.0.rc2 > > > > > > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Hi Eduardo, Lukasz, [Combining my concerns as a single text blob here] I. Firstly, with the current patch 1. is it really needed to duplicate the struct thermal_trip? Why don’t we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it is not a big change. 2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am I missing something?). II. The other concern is more of a design question 1. Do we intend to keep the of-thermal as a middle layer between the thermal_core and the sensor device? OR, is the role of of-thermal just to parse the DT and opt out ? currently of-thermal is somewhat a hybrid of these as, in addition to parsing the dt, it holds on to the data related to trip points etc. etc. 2. assuming latter is true (OF is just a dt parser helper): we should not be adding more intelligence and dependencies linked to the OF. 3. assuming former is true (OF is a well-defined middle layer): All is good till the point OF maintains the trip points (which is a good thing since, OF caches on to the data); BUT, if we expose this information to the sensor device too (as this patch is doing), 3a. we violate the layering principle :-) 3b. more importantly, this is all just excessive logic that we put in which *could be useful* only if we intend to extend the role of OF as a trip point management layer that does more than just holding on to the data. This may include - -> The sensor devices to know nothing about the trip_points, instead the sensor devices would work on "temperature thresholds" and OF would map sensor thresholds to the actual trip points as needed (configured from DT); while the sensor devices stick to using "thresholds". -> Queuing from above, sensors, most of the time, only need to know a high and a low temp threshold; which essentially is a subset of the active/passive etc. trip points. Calculation of that based on the current temp, as of today is replicated across all the sensor drivers and can be hoisted up to the of-thermal. Seems like this is the opportune time to make a call about the role of of-thermal? On 11/26/2014 07:18 AM, Eduardo Valentin wrote: > * PGP Signed by an unknown key > > Hello, > > On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: >> Hi Eduardo, >> >>> Hello Lukasz, >>> >>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: >>>> This patch extends the of-thermal.c to export copy of trip points >>>> for a given thermal zone. >>>> >>>> Thermal drivers should use of_thermal_get_trip_points() method to >>>> get pointer to table of thermal trip points. >>>> >>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> >>>> --- >>>> Changes for v2: >>>> - New patch - as suggested by Eduardo Valentin >>>> --- >>>> drivers/thermal/of-thermal.c | 33 >>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | >>>> 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ >>>> 3 files changed, 54 insertions(+) >>>> >>>> diff --git a/drivers/thermal/of-thermal.c >>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 >>>> --- a/drivers/thermal/of-thermal.c >>>> +++ b/drivers/thermal/of-thermal.c >>>> @@ -89,6 +89,7 @@ struct __thermal_zone { >>>> /* trip data */ >>>> int ntrips; >>>> struct __thermal_trip *trips; >>>> + struct thermal_trip *gtrips; Do we really need this duplication ? >>>> >>>> /* cooling binding data */ >>>> int num_tbps; >>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct >>>> thermal_zone_device *tz, int trip) return true; >>>> } >>>> >>>> +/** >>>> + * of_thermal_get_trip_points - function to get access to a >>>> globally exported >>>> + * trip points >>>> + * >>>> + * @tz: pointer to a thermal zone >>>> + * >>>> + * This function provides a pointer to the copy of trip points >>>> table >>>> + * >>>> + * Return: pointer to trip points table, NULL otherwise >>>> + */ >>>> +const struct thermal_trip * const >>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz) >>>> +{ >>>> + struct __thermal_zone *data = tz->devdata; >>>> + >>>> + if (!data) >>>> + return NULL; >>>> + >>>> + return data->gtrips; >>>> +} >>>> + >>> >>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); >> >> Ok. >> >>> >>>> static int of_thermal_get_trend(struct thermal_zone_device *tz, >>>> int trip, enum thermal_trend *trend) >>>> { >>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct >>>> device_node *np) goto free_tbps; >>>> } >>>> >>>> + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), >>>> GFP_KERNEL); >>>> + if (!tz->gtrips) { >>>> + ret = -ENOMEM; >>>> + goto free_tbps; >>>> + } >>>> + >>>> + for (i = 0; i < tz->ntrips; i++) >>>> + memcpy(&(tz->gtrips[i]), >>>> &(tz->trips[i].temperature), >>>> + sizeof(*tz->gtrips)); >>>> + >>>> finish: >>>> of_node_put(child); >>>> tz->mode = THERMAL_DEVICE_DISABLED; >>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct >>>> __thermal_zone *tz) { >>>> int i; >>>> >>>> + kfree(tz->gtrips); >>>> for (i = 0; i < tz->num_tbps; i++) >>>> of_node_put(tz->tbps[i].cooling_device); >>>> kfree(tz->tbps); Couldn't find the code that updates *gtrips as a result of set_trip_temp via sysfs. >>>> diff --git a/drivers/thermal/thermal_core.h >>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 >>>> --- a/drivers/thermal/thermal_core.h >>>> +++ b/drivers/thermal/thermal_core.h >>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); >>>> void of_thermal_destroy_zones(void); >>>> int of_thermal_get_ntrips(struct thermal_zone_device *); >>>> bool of_thermal_is_trip_en(struct thermal_zone_device *, int); >>>> +const struct thermal_trip * const >>>> +of_thermal_get_trip_points(struct thermal_zone_device *); >>>> #else >>>> static inline int of_parse_thermal_zones(void) { return 0; } >>>> static inline void of_thermal_destroy_zones(void) { } >>>> @@ -102,6 +104,11 @@ static inline bool >>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) { >>> >>> This produces compilation error when CONFIG_THERMAL_OF is not set. >>> Name the parameters to fix. >> >> As all the other cases, I will fix that. >> >>> >>>> return 0; >>>> } >>>> +static inline const struct thermal_trip * const >>>> +of_thermal_get_trip_points(struct thermal_zone_device *) >>>> +{ >>>> + return NULL; >>>> +} >>>> #endif >>>> >>>> #endif /* __THERMAL_CORE_H__ */ >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >>>> index 5bc28a7..88d7249 100644 >>>> --- a/include/linux/thermal.h >>>> +++ b/include/linux/thermal.h >>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { >>>> int (*get_trend)(void *, long *); >>>> }; >>>> >>>> +/** >>>> + * struct thermal_trip - Structure representing themal trip points >>>> exported from >>>> + * of-thermal >>>> + * >>> >>> The only problem I have with this name is that would look like it is >>> in use in all thermal framework, which is not really the case. But I >>> do think having a type here is a good thing. So, not sure :-) >> >> It can stay to be struct thermal_trip or we can rename it to >> struct of_thermal_trip. >> >> I'm fine with both names. > > Leave it as 'thermal_trip'. > >> >>> >>>> + * @temperature: trip point temperature >>>> + * @hysteresis: trip point hysteresis >>>> + * @type: trip point type >>>> + */ >>>> +struct thermal_trip { >>>> + unsigned long int temperature; >>>> + unsigned long int hysteresis; >>>> + enum thermal_trip_type type; >>>> +}; >>>> + >>>> /* Function declarations */ >>>> #ifdef CONFIG_THERMAL_OF >>>> struct thermal_zone_device * >>>> -- >>>> 2.0.0.rc2 >>>> >> >> >> >> -- >> Best regards, >> >> Lukasz Majewski >> >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > * Unknown Key > * 0x7DA4E256 > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/26/2014 12:43 PM, navneet kumar wrote: > > Hi Eduardo, Lukasz, > > [Combining my concerns as a single text blob here] > > I. Firstly, with the current patch > 1. is it really needed to duplicate the struct thermal_trip? Why don’t > we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it > is not a big change. > > 2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am > I missing something?). > > II. The other concern is more of a design question > 1. Do we intend to keep the of-thermal as a middle layer between the > thermal_core and the sensor device? OR, is the role of of-thermal just > to parse the DT and opt out ? currently of-thermal is somewhat a hybrid > of these as, in addition to parsing the dt, it holds on to the data > related to trip points etc. etc. > > 2. assuming latter is true (OF is just a dt parser helper): we should > not be adding more intelligence and dependencies linked to the OF. > > 3. assuming former is true (OF is a well-defined middle layer): All is > good till the point OF maintains the trip points (which is a good thing > since, OF caches on to the data); BUT, if we expose this information to > the sensor device too (as this patch is doing), > > 3a. we violate the layering principle :-) > > 3b. more importantly, this is all just excessive logic that we > put in which *could be useful* only if we intend to extend the > role of OF as a trip point management layer that does more than > just holding on to the data. This may include - > > -> The sensor devices to know nothing about the > trip_points, instead the sensor devices would work on > "temperature thresholds" and OF would map sensor > thresholds to the actual trip points as needed > (configured from DT); while the sensor devices stick to > using "thresholds". > > -> Queuing from above, sensors, most of the time, only > need to know a high and a low temp threshold; which > essentially is a subset of the active/passive etc. trip > points. Calculation of that based on the current temp, > as of today is replicated across all the sensor drivers > and can be hoisted up to the of-thermal. > Sorry, lost you here. What replicated calculation do you refer to ? Thanks, Guenter > Seems like this is the opportune time to make a call about the role of of-thermal? > > On 11/26/2014 07:18 AM, Eduardo Valentin wrote: >> * PGP Signed by an unknown key >> >> Hello, >> >> On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: >>> Hi Eduardo, >>> >>>> Hello Lukasz, >>>> >>>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: >>>>> This patch extends the of-thermal.c to export copy of trip points >>>>> for a given thermal zone. >>>>> >>>>> Thermal drivers should use of_thermal_get_trip_points() method to >>>>> get pointer to table of thermal trip points. >>>>> >>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> >>>>> --- >>>>> Changes for v2: >>>>> - New patch - as suggested by Eduardo Valentin >>>>> --- >>>>> drivers/thermal/of-thermal.c | 33 >>>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | >>>>> 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ >>>>> 3 files changed, 54 insertions(+) >>>>> >>>>> diff --git a/drivers/thermal/of-thermal.c >>>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 >>>>> --- a/drivers/thermal/of-thermal.c >>>>> +++ b/drivers/thermal/of-thermal.c >>>>> @@ -89,6 +89,7 @@ struct __thermal_zone { >>>>> /* trip data */ >>>>> int ntrips; >>>>> struct __thermal_trip *trips; >>>>> + struct thermal_trip *gtrips; > Do we really need this duplication ? >>>>> >>>>> /* cooling binding data */ >>>>> int num_tbps; >>>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct >>>>> thermal_zone_device *tz, int trip) return true; >>>>> } >>>>> >>>>> +/** >>>>> + * of_thermal_get_trip_points - function to get access to a >>>>> globally exported >>>>> + * trip points >>>>> + * >>>>> + * @tz: pointer to a thermal zone >>>>> + * >>>>> + * This function provides a pointer to the copy of trip points >>>>> table >>>>> + * >>>>> + * Return: pointer to trip points table, NULL otherwise >>>>> + */ >>>>> +const struct thermal_trip * const >>>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz) >>>>> +{ >>>>> + struct __thermal_zone *data = tz->devdata; >>>>> + >>>>> + if (!data) >>>>> + return NULL; >>>>> + >>>>> + return data->gtrips; >>>>> +} >>>>> + >>>> >>>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); >>> >>> Ok. >>> >>>> >>>>> static int of_thermal_get_trend(struct thermal_zone_device *tz, >>>>> int trip, enum thermal_trend *trend) >>>>> { >>>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct >>>>> device_node *np) goto free_tbps; >>>>> } >>>>> >>>>> + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), >>>>> GFP_KERNEL); >>>>> + if (!tz->gtrips) { >>>>> + ret = -ENOMEM; >>>>> + goto free_tbps; >>>>> + } >>>>> + >>>>> + for (i = 0; i < tz->ntrips; i++) >>>>> + memcpy(&(tz->gtrips[i]), >>>>> &(tz->trips[i].temperature), >>>>> + sizeof(*tz->gtrips)); >>>>> + >>>>> finish: >>>>> of_node_put(child); >>>>> tz->mode = THERMAL_DEVICE_DISABLED; >>>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct >>>>> __thermal_zone *tz) { >>>>> int i; >>>>> >>>>> + kfree(tz->gtrips); >>>>> for (i = 0; i < tz->num_tbps; i++) >>>>> of_node_put(tz->tbps[i].cooling_device); >>>>> kfree(tz->tbps); > Couldn't find the code that updates *gtrips as a result of set_trip_temp via > sysfs. > >>>>> diff --git a/drivers/thermal/thermal_core.h >>>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 >>>>> --- a/drivers/thermal/thermal_core.h >>>>> +++ b/drivers/thermal/thermal_core.h >>>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); >>>>> void of_thermal_destroy_zones(void); >>>>> int of_thermal_get_ntrips(struct thermal_zone_device *); >>>>> bool of_thermal_is_trip_en(struct thermal_zone_device *, int); >>>>> +const struct thermal_trip * const >>>>> +of_thermal_get_trip_points(struct thermal_zone_device *); >>>>> #else >>>>> static inline int of_parse_thermal_zones(void) { return 0; } >>>>> static inline void of_thermal_destroy_zones(void) { } >>>>> @@ -102,6 +104,11 @@ static inline bool >>>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) { >>>> >>>> This produces compilation error when CONFIG_THERMAL_OF is not set. >>>> Name the parameters to fix. >>> >>> As all the other cases, I will fix that. >>> >>>> >>>>> return 0; >>>>> } >>>>> +static inline const struct thermal_trip * const >>>>> +of_thermal_get_trip_points(struct thermal_zone_device *) >>>>> +{ >>>>> + return NULL; >>>>> +} >>>>> #endif >>>>> >>>>> #endif /* __THERMAL_CORE_H__ */ >>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >>>>> index 5bc28a7..88d7249 100644 >>>>> --- a/include/linux/thermal.h >>>>> +++ b/include/linux/thermal.h >>>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { >>>>> int (*get_trend)(void *, long *); >>>>> }; >>>>> >>>>> +/** >>>>> + * struct thermal_trip - Structure representing themal trip points >>>>> exported from >>>>> + * of-thermal >>>>> + * >>>> >>>> The only problem I have with this name is that would look like it is >>>> in use in all thermal framework, which is not really the case. But I >>>> do think having a type here is a good thing. So, not sure :-) >>> >>> It can stay to be struct thermal_trip or we can rename it to >>> struct of_thermal_trip. >>> >>> I'm fine with both names. >> >> Leave it as 'thermal_trip'. >> >>> >>>> >>>>> + * @temperature: trip point temperature >>>>> + * @hysteresis: trip point hysteresis >>>>> + * @type: trip point type >>>>> + */ >>>>> +struct thermal_trip { >>>>> + unsigned long int temperature; >>>>> + unsigned long int hysteresis; >>>>> + enum thermal_trip_type type; >>>>> +}; >>>>> + >>>>> /* Function declarations */ >>>>> #ifdef CONFIG_THERMAL_OF >>>>> struct thermal_zone_device * >>>>> -- >>>>> 2.0.0.rc2 >>>>> >>> >>> >>> >>> -- >>> Best regards, >>> >>> Lukasz Majewski >>> >>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group >> >> * Unknown Key >> * 0x7DA4E256 >> > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Navneet, On Wed, Nov 26, 2014 at 12:43:18PM -0800, navneet kumar wrote: > > Hi Eduardo, Lukasz, > > [Combining my concerns as a single text blob here] > > I. Firstly, with the current patch > 1. is it really needed to duplicate the struct thermal_trip? Why don’t > we get rid of the __thermal_trip and stay with the 'thermal_trip' ? it > is not a big change. > > 2. gtrips is not updated on "set_trip_temp" etc. actions via sysfs. (am > I missing something?). > Good! Comments are always welcome, that's how we make patches :-). Both above make sense to me. > II. The other concern is more of a design question > 1. Do we intend to keep the of-thermal as a middle layer between the > thermal_core and the sensor device? OR, is the role of of-thermal just > to parse the DT and opt out ? currently of-thermal is somewhat a hybrid > of these as, in addition to parsing the dt, it holds on to the data > related to trip points etc. etc. > of-thermal has always been as your latter statement, and I intend to keep it as it should be, just a of parser for thermal data. > 2. assuming latter is true (OF is just a dt parser helper): we should > not be adding more intelligence and dependencies linked to the OF. > Yes this is right. We should never be adding intelligence to it, except for parsing the thermal data out of device tree and adding the proper structures inside the kernel. > 3. assuming former is true (OF is a well-defined middle layer): All is > good till the point OF maintains the trip points (which is a good thing > since, OF caches on to the data); BUT, if we expose this information to > the sensor device too (as this patch is doing), > the former is not true. > 3a. we violate the layering principle :-) > well, even if it was, I disagree here. :-) The split between data coming from DT and the driver is still in place. Besides, there is other layers that expose some of their data, and that doesn't violate their layering principles. CPUfreq, for one closer example, exposes its freq table. It would be different if by exposing this data, the users would be affecting the behavior of the layer. And that is not the intention of cpufreq table. In the same way, that is not the intention of this patch. > 3b. more importantly, this is all just excessive logic that we > put in which *could be useful* only if we intend to extend the > role of OF as a trip point management layer that does more than > just holding on to the data. This may include - > > -> The sensor devices to know nothing about the > trip_points, instead the sensor devices would work on > "temperature thresholds" and OF would map sensor > thresholds to the actual trip points as needed > (configured from DT); while the sensor devices stick to > using "thresholds". > > -> Queuing from above, sensors, most of the time, only > need to know a high and a low temp threshold; which > essentially is a subset of the active/passive etc. trip > points. Calculation of that based on the current temp, > as of today is replicated across all the sensor drivers > and can be hoisted up to the of-thermal. > There is no intention to add such logic to of thermal. The main reason is of-thermal should never be competing with thermal core. Any extension in of-thermal needs to be supported by thermal core. I believe all the above is left currently to thermal zone device drivers. The problem with of-thermal is that it had to be born as a thermal zone device driver. And that is because.. > Seems like this is the opportune time to make a call about the role of of-thermal? > .. the point one may be missing here is the fact that with current thermal subsystem implementation, handling thermal devices is somewhat different from other devices within the kernel. The real design issue (or not an issue) we have is the fact that thermal drivers adds both the driver and the device. Changing that is somewhat disruptive. Not impossible, but requires considering the existing driver's code. If we had a better split from who adds the device and who provides the driver, then of-thermal would never be a "glue layer" or born as thermal zone device driver. It would simply, parse DT, add the devices, done. Then, drivers would register themselves as handlers for specific thermal devices. That is the correct design, based on common design found in other parts of the kernel. Another little missing end would be the "compatible" string for thermal zone devices in DT. But as I said, it should not be a blocker. So, given the current situation, we have essentially two options: (a) stick to the same design we have for now, having of-thermal as dummy as possible, and grow in small steps towards the correct design; or (b) redesign thermal core to have a better split between devices and drivers, then adjust of-thermal. For now, the path we are taking is (a). In fact, to fit the needs of coming drivers, specially considering they are based on DT booting platforms, it is always tempting to add intelligence to of-thermal. But, as I mentioned, I want to avoid growing intelligence in it, for obvious reasons. > On 11/26/2014 07:18 AM, Eduardo Valentin wrote: > > * PGP Signed by an unknown key > > > > Hello, > > > > On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: > >> Hi Eduardo, > >> > >>> Hello Lukasz, > >>> > >>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > >>>> This patch extends the of-thermal.c to export copy of trip points > >>>> for a given thermal zone. > >>>> > >>>> Thermal drivers should use of_thermal_get_trip_points() method to > >>>> get pointer to table of thermal trip points. > >>>> > >>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > >>>> --- > >>>> Changes for v2: > >>>> - New patch - as suggested by Eduardo Valentin > >>>> --- > >>>> drivers/thermal/of-thermal.c | 33 > >>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | > >>>> 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ > >>>> 3 files changed, 54 insertions(+) > >>>> > >>>> diff --git a/drivers/thermal/of-thermal.c > >>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 > >>>> --- a/drivers/thermal/of-thermal.c > >>>> +++ b/drivers/thermal/of-thermal.c > >>>> @@ -89,6 +89,7 @@ struct __thermal_zone { > >>>> /* trip data */ > >>>> int ntrips; > >>>> struct __thermal_trip *trips; > >>>> + struct thermal_trip *gtrips; > Do we really need this duplication ? > >>>> > >>>> /* cooling binding data */ > >>>> int num_tbps; > >>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct > >>>> thermal_zone_device *tz, int trip) return true; > >>>> } > >>>> > >>>> +/** > >>>> + * of_thermal_get_trip_points - function to get access to a > >>>> globally exported > >>>> + * trip points > >>>> + * > >>>> + * @tz: pointer to a thermal zone > >>>> + * > >>>> + * This function provides a pointer to the copy of trip points > >>>> table > >>>> + * > >>>> + * Return: pointer to trip points table, NULL otherwise > >>>> + */ > >>>> +const struct thermal_trip * const > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz) > >>>> +{ > >>>> + struct __thermal_zone *data = tz->devdata; > >>>> + > >>>> + if (!data) > >>>> + return NULL; > >>>> + > >>>> + return data->gtrips; > >>>> +} > >>>> + > >>> > >>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > >> > >> Ok. > >> > >>> > >>>> static int of_thermal_get_trend(struct thermal_zone_device *tz, > >>>> int trip, enum thermal_trend *trend) > >>>> { > >>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct > >>>> device_node *np) goto free_tbps; > >>>> } > >>>> > >>>> + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), > >>>> GFP_KERNEL); > >>>> + if (!tz->gtrips) { > >>>> + ret = -ENOMEM; > >>>> + goto free_tbps; > >>>> + } > >>>> + > >>>> + for (i = 0; i < tz->ntrips; i++) > >>>> + memcpy(&(tz->gtrips[i]), > >>>> &(tz->trips[i].temperature), > >>>> + sizeof(*tz->gtrips)); > >>>> + > >>>> finish: > >>>> of_node_put(child); > >>>> tz->mode = THERMAL_DEVICE_DISABLED; > >>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct > >>>> __thermal_zone *tz) { > >>>> int i; > >>>> > >>>> + kfree(tz->gtrips); > >>>> for (i = 0; i < tz->num_tbps; i++) > >>>> of_node_put(tz->tbps[i].cooling_device); > >>>> kfree(tz->tbps); > Couldn't find the code that updates *gtrips as a result of set_trip_temp via > sysfs. > > >>>> diff --git a/drivers/thermal/thermal_core.h > >>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 > >>>> --- a/drivers/thermal/thermal_core.h > >>>> +++ b/drivers/thermal/thermal_core.h > >>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > >>>> void of_thermal_destroy_zones(void); > >>>> int of_thermal_get_ntrips(struct thermal_zone_device *); > >>>> bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > >>>> +const struct thermal_trip * const > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *); > >>>> #else > >>>> static inline int of_parse_thermal_zones(void) { return 0; } > >>>> static inline void of_thermal_destroy_zones(void) { } > >>>> @@ -102,6 +104,11 @@ static inline bool > >>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) { > >>> > >>> This produces compilation error when CONFIG_THERMAL_OF is not set. > >>> Name the parameters to fix. > >> > >> As all the other cases, I will fix that. > >> > >>> > >>>> return 0; > >>>> } > >>>> +static inline const struct thermal_trip * const > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *) > >>>> +{ > >>>> + return NULL; > >>>> +} > >>>> #endif > >>>> > >>>> #endif /* __THERMAL_CORE_H__ */ > >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h > >>>> index 5bc28a7..88d7249 100644 > >>>> --- a/include/linux/thermal.h > >>>> +++ b/include/linux/thermal.h > >>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > >>>> int (*get_trend)(void *, long *); > >>>> }; > >>>> > >>>> +/** > >>>> + * struct thermal_trip - Structure representing themal trip points > >>>> exported from > >>>> + * of-thermal > >>>> + * > >>> > >>> The only problem I have with this name is that would look like it is > >>> in use in all thermal framework, which is not really the case. But I > >>> do think having a type here is a good thing. So, not sure :-) > >> > >> It can stay to be struct thermal_trip or we can rename it to > >> struct of_thermal_trip. > >> > >> I'm fine with both names. > > > > Leave it as 'thermal_trip'. > > > >> > >>> > >>>> + * @temperature: trip point temperature > >>>> + * @hysteresis: trip point hysteresis > >>>> + * @type: trip point type > >>>> + */ > >>>> +struct thermal_trip { > >>>> + unsigned long int temperature; > >>>> + unsigned long int hysteresis; > >>>> + enum thermal_trip_type type; > >>>> +}; > >>>> + > >>>> /* Function declarations */ > >>>> #ifdef CONFIG_THERMAL_OF > >>>> struct thermal_zone_device * > >>>> -- > >>>> 2.0.0.rc2 > >>>> > >> > >> > >> > >> -- > >> Best regards, > >> > >> Lukasz Majewski > >> > >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > > > * Unknown Key > > * 0x7DA4E256 > >
On 11/26/2014 01:12 PM, Guenter Roeck wrote: > On 11/26/2014 12:43 PM, navneet kumar wrote: >> >> Hi Eduardo, Lukasz, >> >> [Combining my concerns as a single text blob here] >> >> I. Firstly, with the current patch >> 1. is it really needed to duplicate the struct thermal_trip? Why >> don’t >> we get rid of the __thermal_trip and stay with the 'thermal_trip' >> ? it >> is not a big change. >> >> 2. gtrips is not updated on "set_trip_temp" etc. actions via >> sysfs. (am >> I missing something?). >> >> II. The other concern is more of a design question >> 1. Do we intend to keep the of-thermal as a middle layer between the >> thermal_core and the sensor device? OR, is the role of of-thermal >> just >> to parse the DT and opt out ? currently of-thermal is somewhat a >> hybrid >> of these as, in addition to parsing the dt, it holds on to the data >> related to trip points etc. etc. >> >> 2. assuming latter is true (OF is just a dt parser helper): we should >> not be adding more intelligence and dependencies linked to the OF. >> >> 3. assuming former is true (OF is a well-defined middle layer): >> All is >> good till the point OF maintains the trip points (which is a good >> thing >> since, OF caches on to the data); BUT, if we expose this >> information to >> the sensor device too (as this patch is doing), >> >> 3a. we violate the layering principle :-) >> >> 3b. more importantly, this is all just excessive logic that we >> put in which *could be useful* only if we intend to extend the >> role of OF as a trip point management layer that does more than >> just holding on to the data. This may include - >> >> -> The sensor devices to know nothing about the >> trip_points, instead the sensor devices would work on >> "temperature thresholds" and OF would map sensor >> thresholds to the actual trip points as needed >> (configured from DT); while the sensor devices stick to >> using "thresholds". >> >> -> Queuing from above, sensors, most of the time, only >> need to know a high and a low temp threshold; which >> essentially is a subset of the active/passive etc. trip >> points. Calculation of that based on the current temp, >> as of today is replicated across all the sensor drivers >> and can be hoisted up to the of-thermal. >> > > Sorry, lost you here. What replicated calculation do you refer to ? > > Thanks, > Guenter > Some sensors have an ability to generate interrupts based upon a configured high and low temp thresholds/values. Once any of these limits is crossed, the sensor hardware has to be reconfigured with a new set of such values. I'll try to explain with an example - consider trip points TP1, TP2, TP3, TP4 sorted as low to high; and the current temp T1 such that - TP2<T1<TP3 With the conditions as is, and assuming the interrupts have been configured, an interrupt will trigger if T1 goes below TP2 or exceeds TP3. Now, say, T1 crosses TP3, the sensor hardware generates an interrupt (avoids polling on the zone temp). Furthermore, we require that the new limits for the interrupts be TP3 and TP4 for 'low' and 'high' thresholds interrupts respectively, such that an interrupt will be fired when temp goes below TP3 or exceeds TP4. Above case is when the temp crosses a set of High-low. The same situation may occur when a trip point temp is changed from sysfs, in which case, the thresholds for configuring the interrupts may have to be reconfigured. Today, the sensors find the high-low thresholds by - 1. querying the temp from the thermal_zone 2. traversing the trip points (which may have been cached locally by the sensor), and figuring out the high/low temperature thresholds. These above steps aka 'replicated calculation'. Whatever the case may be, the real question is the role and scope of of-thermal. >> Seems like this is the opportune time to make a call about the role of >> of-thermal? >> >> On 11/26/2014 07:18 AM, Eduardo Valentin wrote: >>> * PGP Signed by an unknown key >>> >>> Hello, >>> >>> On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: >>>> Hi Eduardo, >>>> >>>>> Hello Lukasz, >>>>> >>>>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: >>>>>> This patch extends the of-thermal.c to export copy of trip points >>>>>> for a given thermal zone. >>>>>> >>>>>> Thermal drivers should use of_thermal_get_trip_points() method to >>>>>> get pointer to table of thermal trip points. >>>>>> >>>>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> >>>>>> --- >>>>>> Changes for v2: >>>>>> - New patch - as suggested by Eduardo Valentin >>>>>> --- >>>>>> drivers/thermal/of-thermal.c | 33 >>>>>> +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | >>>>>> 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ >>>>>> 3 files changed, 54 insertions(+) >>>>>> >>>>>> diff --git a/drivers/thermal/of-thermal.c >>>>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 >>>>>> --- a/drivers/thermal/of-thermal.c >>>>>> +++ b/drivers/thermal/of-thermal.c >>>>>> @@ -89,6 +89,7 @@ struct __thermal_zone { >>>>>> /* trip data */ >>>>>> int ntrips; >>>>>> struct __thermal_trip *trips; >>>>>> + struct thermal_trip *gtrips; >> Do we really need this duplication ? >>>>>> >>>>>> /* cooling binding data */ >>>>>> int num_tbps; >>>>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct >>>>>> thermal_zone_device *tz, int trip) return true; >>>>>> } >>>>>> >>>>>> +/** >>>>>> + * of_thermal_get_trip_points - function to get access to a >>>>>> globally exported >>>>>> + * trip points >>>>>> + * >>>>>> + * @tz: pointer to a thermal zone >>>>>> + * >>>>>> + * This function provides a pointer to the copy of trip points >>>>>> table >>>>>> + * >>>>>> + * Return: pointer to trip points table, NULL otherwise >>>>>> + */ >>>>>> +const struct thermal_trip * const >>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz) >>>>>> +{ >>>>>> + struct __thermal_zone *data = tz->devdata; >>>>>> + >>>>>> + if (!data) >>>>>> + return NULL; >>>>>> + >>>>>> + return data->gtrips; >>>>>> +} >>>>>> + >>>>> >>>>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); >>>> >>>> Ok. >>>> >>>>> >>>>>> static int of_thermal_get_trend(struct thermal_zone_device *tz, >>>>>> int trip, enum thermal_trend *trend) >>>>>> { >>>>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct >>>>>> device_node *np) goto free_tbps; >>>>>> } >>>>>> >>>>>> + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), >>>>>> GFP_KERNEL); >>>>>> + if (!tz->gtrips) { >>>>>> + ret = -ENOMEM; >>>>>> + goto free_tbps; >>>>>> + } >>>>>> + >>>>>> + for (i = 0; i < tz->ntrips; i++) >>>>>> + memcpy(&(tz->gtrips[i]), >>>>>> &(tz->trips[i].temperature), >>>>>> + sizeof(*tz->gtrips)); >>>>>> + >>>>>> finish: >>>>>> of_node_put(child); >>>>>> tz->mode = THERMAL_DEVICE_DISABLED; >>>>>> @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct >>>>>> __thermal_zone *tz) { >>>>>> int i; >>>>>> >>>>>> + kfree(tz->gtrips); >>>>>> for (i = 0; i < tz->num_tbps; i++) >>>>>> of_node_put(tz->tbps[i].cooling_device); >>>>>> kfree(tz->tbps); >> Couldn't find the code that updates *gtrips as a result of >> set_trip_temp via >> sysfs. >> >>>>>> diff --git a/drivers/thermal/thermal_core.h >>>>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 >>>>>> --- a/drivers/thermal/thermal_core.h >>>>>> +++ b/drivers/thermal/thermal_core.h >>>>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); >>>>>> void of_thermal_destroy_zones(void); >>>>>> int of_thermal_get_ntrips(struct thermal_zone_device *); >>>>>> bool of_thermal_is_trip_en(struct thermal_zone_device *, int); >>>>>> +const struct thermal_trip * const >>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *); >>>>>> #else >>>>>> static inline int of_parse_thermal_zones(void) { return 0; } >>>>>> static inline void of_thermal_destroy_zones(void) { } >>>>>> @@ -102,6 +104,11 @@ static inline bool >>>>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) { >>>>> >>>>> This produces compilation error when CONFIG_THERMAL_OF is not set. >>>>> Name the parameters to fix. >>>> >>>> As all the other cases, I will fix that. >>>> >>>>> >>>>>> return 0; >>>>>> } >>>>>> +static inline const struct thermal_trip * const >>>>>> +of_thermal_get_trip_points(struct thermal_zone_device *) >>>>>> +{ >>>>>> + return NULL; >>>>>> +} >>>>>> #endif >>>>>> >>>>>> #endif /* __THERMAL_CORE_H__ */ >>>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h >>>>>> index 5bc28a7..88d7249 100644 >>>>>> --- a/include/linux/thermal.h >>>>>> +++ b/include/linux/thermal.h >>>>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { >>>>>> int (*get_trend)(void *, long *); >>>>>> }; >>>>>> >>>>>> +/** >>>>>> + * struct thermal_trip - Structure representing themal trip points >>>>>> exported from >>>>>> + * of-thermal >>>>>> + * >>>>> >>>>> The only problem I have with this name is that would look like it is >>>>> in use in all thermal framework, which is not really the case. But I >>>>> do think having a type here is a good thing. So, not sure :-) >>>> >>>> It can stay to be struct thermal_trip or we can rename it to >>>> struct of_thermal_trip. >>>> >>>> I'm fine with both names. >>> >>> Leave it as 'thermal_trip'. >>> >>>> >>>>> >>>>>> + * @temperature: trip point temperature >>>>>> + * @hysteresis: trip point hysteresis >>>>>> + * @type: trip point type >>>>>> + */ >>>>>> +struct thermal_trip { >>>>>> + unsigned long int temperature; >>>>>> + unsigned long int hysteresis; >>>>>> + enum thermal_trip_type type; >>>>>> +}; >>>>>> + >>>>>> /* Function declarations */ >>>>>> #ifdef CONFIG_THERMAL_OF >>>>>> struct thermal_zone_device * >>>>>> -- >>>>>> 2.0.0.rc2 >>>>>> >>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> >>>> Lukasz Majewski >>>> >>>> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group >>> >>> * Unknown Key >>> * 0x7DA4E256 >>> >> > > -- > 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/ -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Eduardo, > > Hello Navneet, > > On Wed, Nov 26, 2014 at 12:43:18PM -0800, navneet kumar wrote: > > > > Hi Eduardo, Lukasz, > > > > [Combining my concerns as a single text blob here] > > > > I. Firstly, with the current patch > > 1. is it really needed to duplicate the struct > > thermal_trip? Why don’t we get rid of the __thermal_trip and stay > > with the 'thermal_trip' ? it is not a big change. The __thermal_trip seems to be somehow "private" structure (to of-thermal) which holds not only data which could be exposed to sensors. > > > > 2. gtrips is not updated on "set_trip_temp" etc. actions > > via sysfs. (am I missing something?). True. This is a bug. Thanks for spotting. > > > > Good! Comments are always welcome, that's how we make patches :-). > > Both above make sense to me. > > > II. The other concern is more of a design question > > 1. Do we intend to keep the of-thermal as a middle layer > > between the thermal_core and the sensor device? OR, is the role of > > of-thermal just to parse the DT and opt out ? currently of-thermal > > is somewhat a hybrid of these as, in addition to parsing the dt, it > > holds on to the data related to trip points etc. etc. > > > > of-thermal has always been as your latter statement, and I intend to > keep it as it should be, just a of parser for thermal data. It seems to me that now of-thermal is doing more than parsing thermal data. For example it is an abstraction layer for calling .get_temp(), .get_trend(). It has its own set of "private" trip points which aren't exposed to sensors. Also, it registers thermal_zone_device. A lot of stuff is done by the of-thermal. Frankly, I don't mind, since this is a common code for many sensors. For example with of-thermal.c usage, we are able to cut down LOC number by half for Exynos TMU when moving to OF. > > > 2. assuming latter is true (OF is just a dt parser helper): > > we should not be adding more intelligence and dependencies linked > > to the OF. > > > > Yes this is right. We should never be adding intelligence to it, But a lot of stuff is already added and to be worse it is well adopted API by sensors. > except for parsing the thermal data out of device tree and adding the > proper structures inside the kernel. In my understanding the of-thermal code to expose the above features need to: - parse device tree - export trip points in a table to sensors - export cpu frequencies (OPPs?) with information about corresponding temperature And that is all. The sensor is then responsible for initializing HW and register the thermal zone with thermal_core. > > > 3. assuming former is true (OF is a well-defined middle > > layer): All is good till the point OF maintains the trip points > > (which is a good thing since, OF caches on to the data); BUT, if we > > expose this information to the sensor device too (as this patch is > > doing), > > > > the former is not true. > > > 3a. we violate the layering principle :-) > > Are we? of-thermal.c is parsing DT and it exposes read only information about trip points. Also it gives you information about e.g. number of available trip points. > > well, even if it was, I disagree here. :-) The split between data > coming from DT and the driver is still in place. Besides, there is > other layers that expose some of their data, and that doesn't violate > their layering principles. CPUfreq, for one closer example, exposes > its freq table. > > It would be different if by exposing this data, the users would be > affecting the behavior of the layer. And that is not the intention of > cpufreq table. In the same way, that is not the intention of this > patch. > > > 3b. more importantly, this is all just excessive > > logic that we put in which *could be useful* only if we intend to > > extend the role of OF as a trip point management layer that does > > more than just holding on to the data. This may include - > > > > -> The sensor devices to know nothing about > > the trip_points, instead the sensor devices would work on > > "temperature thresholds" and OF would map > > sensor thresholds to the actual trip points as needed > > (configured from DT); while the sensor > > devices stick to using "thresholds". > > > > -> Queuing from above, sensors, most of the > > time, only need to know a high and a low temp threshold; which > > essentially is a subset of the > > active/passive etc. trip points. Calculation of that based on the > > current temp, as of today is replicated across all the sensor > > drivers and can be hoisted up to the of-thermal. > > > > There is no intention to add such logic to of thermal. The main reason > is of-thermal should never be competing with thermal core. Any > extension in of-thermal needs to be supported by thermal core. > > I believe all the above is left currently to thermal zone device > drivers. The problem with of-thermal is that it had to be born as a > thermal zone device driver. And that is because.. > > > Seems like this is the opportune time to make a call about the role > > of of-thermal? > > > > .. the point one may be missing here is the fact that with current > thermal subsystem implementation, handling thermal devices is somewhat > different from other devices within the kernel. > > The real design issue (or not an issue) we have is the fact that > thermal drivers adds both the driver and the device. Changing that is > somewhat disruptive. Not impossible, but requires considering the > existing driver's code. > > If we had a better split from who adds the device and who provides the > driver, then of-thermal would never be a "glue layer" or born as > thermal zone device driver. It would simply, parse DT, add the > devices, done. > > Then, drivers would register themselves as handlers for specific > thermal devices. That is the correct design, based on common design > found in other parts of the kernel. Another little missing end would > be the "compatible" string for thermal zone devices in DT. But as I > said, it should not be a blocker. > > So, given the current situation, we have essentially two options: (a) > stick to the same design we have for now, having of-thermal as dummy > as possible, and grow in small steps towards the correct design; or > (b) redesign thermal core to have a better split between devices and > drivers, then adjust of-thermal. > > > For now, the path we are taking is (a). In fact, to fit the needs of > coming drivers, specially considering they are based on DT booting > platforms, it is always tempting to add intelligence to of-thermal. > But, as I mentioned, I want to avoid growing intelligence in it, for > obvious reasons. Eventually, it is your call if we make __thermal_trip [1] exported to sensors (with or without struct device_node *np) or if we have new structure (e.g. struct thermal_trip) which is a read only reference to relevant fields of [1]? > > > On 11/26/2014 07:18 AM, Eduardo Valentin wrote: > > > * PGP Signed by an unknown key > > > > > > Hello, > > > > > > On Wed, Nov 26, 2014 at 09:35:10AM +0100, Lukasz Majewski wrote: > > >> Hi Eduardo, > > >> > > >>> Hello Lukasz, > > >>> > > >>> On Thu, Nov 20, 2014 at 05:21:27PM +0100, Lukasz Majewski wrote: > > >>>> This patch extends the of-thermal.c to export copy of trip > > >>>> points for a given thermal zone. > > >>>> > > >>>> Thermal drivers should use of_thermal_get_trip_points() method > > >>>> to get pointer to table of thermal trip points. > > >>>> > > >>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > >>>> --- > > >>>> Changes for v2: > > >>>> - New patch - as suggested by Eduardo Valentin > > >>>> --- > > >>>> drivers/thermal/of-thermal.c | 33 > > >>>> +++++++++++++++++++++++++++++++++ > > >>>> drivers/thermal/thermal_core.h | 7 +++++++ > > >>>> include/linux/thermal.h | 14 ++++++++++++++ 3 files > > >>>> changed, 54 insertions(+) > > >>>> > > >>>> diff --git a/drivers/thermal/of-thermal.c > > >>>> b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 > > >>>> --- a/drivers/thermal/of-thermal.c > > >>>> +++ b/drivers/thermal/of-thermal.c > > >>>> @@ -89,6 +89,7 @@ struct __thermal_zone { > > >>>> /* trip data */ > > >>>> int ntrips; > > >>>> struct __thermal_trip *trips; > > >>>> + struct thermal_trip *gtrips; > > Do we really need this duplication ? > > >>>> > > >>>> /* cooling binding data */ > > >>>> int num_tbps; > > >>>> @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct > > >>>> thermal_zone_device *tz, int trip) return true; > > >>>> } > > >>>> > > >>>> +/** > > >>>> + * of_thermal_get_trip_points - function to get access to a > > >>>> globally exported > > >>>> + * trip points > > >>>> + * > > >>>> + * @tz: pointer to a thermal zone > > >>>> + * > > >>>> + * This function provides a pointer to the copy of trip points > > >>>> table > > >>>> + * > > >>>> + * Return: pointer to trip points table, NULL otherwise > > >>>> + */ > > >>>> +const struct thermal_trip * const > > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *tz) > > >>>> +{ > > >>>> + struct __thermal_zone *data = tz->devdata; > > >>>> + > > >>>> + if (!data) > > >>>> + return NULL; > > >>>> + > > >>>> + return data->gtrips; > > >>>> +} > > >>>> + > > >>> > > >>> EXPORT_SYMBOL_GPL(of_thermal_get_trip_points); > > >> > > >> Ok. > > >> > > >>> > > >>>> static int of_thermal_get_trend(struct thermal_zone_device > > >>>> *tz, int trip, enum thermal_trend *trend) > > >>>> { > > >>>> @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct > > >>>> device_node *np) goto free_tbps; > > >>>> } > > >>>> > > >>>> + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), > > >>>> GFP_KERNEL); > > >>>> + if (!tz->gtrips) { > > >>>> + ret = -ENOMEM; > > >>>> + goto free_tbps; > > >>>> + } > > >>>> + > > >>>> + for (i = 0; i < tz->ntrips; i++) > > >>>> + memcpy(&(tz->gtrips[i]), > > >>>> &(tz->trips[i].temperature), > > >>>> + sizeof(*tz->gtrips)); > > >>>> + > > >>>> finish: > > >>>> of_node_put(child); > > >>>> tz->mode = THERMAL_DEVICE_DISABLED; > > >>>> @@ -793,6 +825,7 @@ static inline void > > >>>> of_thermal_free_zone(struct __thermal_zone *tz) { > > >>>> int i; > > >>>> > > >>>> + kfree(tz->gtrips); > > >>>> for (i = 0; i < tz->num_tbps; i++) > > >>>> of_node_put(tz->tbps[i].cooling_device); > > >>>> kfree(tz->tbps); > > Couldn't find the code that updates *gtrips as a result of > > set_trip_temp via sysfs. > > > > >>>> diff --git a/drivers/thermal/thermal_core.h > > >>>> b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 > > >>>> --- a/drivers/thermal/thermal_core.h > > >>>> +++ b/drivers/thermal/thermal_core.h > > >>>> @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); > > >>>> void of_thermal_destroy_zones(void); > > >>>> int of_thermal_get_ntrips(struct thermal_zone_device *); > > >>>> bool of_thermal_is_trip_en(struct thermal_zone_device *, int); > > >>>> +const struct thermal_trip * const > > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *); > > >>>> #else > > >>>> static inline int of_parse_thermal_zones(void) { return 0; } > > >>>> static inline void of_thermal_destroy_zones(void) { } > > >>>> @@ -102,6 +104,11 @@ static inline bool > > >>>> of_thermal_is_trip_en(struct thermal_zone_device *, int) { > > >>> > > >>> This produces compilation error when CONFIG_THERMAL_OF is not > > >>> set. Name the parameters to fix. > > >> > > >> As all the other cases, I will fix that. > > >> > > >>> > > >>>> return 0; > > >>>> } > > >>>> +static inline const struct thermal_trip * const > > >>>> +of_thermal_get_trip_points(struct thermal_zone_device *) > > >>>> +{ > > >>>> + return NULL; > > >>>> +} > > >>>> #endif > > >>>> > > >>>> #endif /* __THERMAL_CORE_H__ */ > > >>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h > > >>>> index 5bc28a7..88d7249 100644 > > >>>> --- a/include/linux/thermal.h > > >>>> +++ b/include/linux/thermal.h > > >>>> @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { > > >>>> int (*get_trend)(void *, long *); > > >>>> }; > > >>>> > > >>>> +/** > > >>>> + * struct thermal_trip - Structure representing themal trip > > >>>> points exported from > > >>>> + * of-thermal > > >>>> + * > > >>> > > >>> The only problem I have with this name is that would look like > > >>> it is in use in all thermal framework, which is not really the > > >>> case. But I do think having a type here is a good thing. So, > > >>> not sure :-) > > >> > > >> It can stay to be struct thermal_trip or we can rename it to > > >> struct of_thermal_trip. > > >> > > >> I'm fine with both names. > > > > > > Leave it as 'thermal_trip'. > > > > > >> > > >>> > > >>>> + * @temperature: trip point temperature > > >>>> + * @hysteresis: trip point hysteresis > > >>>> + * @type: trip point type > > >>>> + */ > > >>>> +struct thermal_trip { > > >>>> + unsigned long int temperature; > > >>>> + unsigned long int hysteresis; > > >>>> + enum thermal_trip_type type; > > >>>> +}; > > >>>> + > > >>>> /* Function declarations */ > > >>>> #ifdef CONFIG_THERMAL_OF > > >>>> struct thermal_zone_device * > > >>>> -- > > >>>> 2.0.0.rc2 > > >>>> > > >> > > >> > > >> > > >> -- > > >> Best regards, > > >> > > >> Lukasz Majewski > > >> > > >> Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > > > > > * Unknown Key > > > * 0x7DA4E256 > > >
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index 336af7f..33921c5 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -89,6 +89,7 @@ struct __thermal_zone { /* trip data */ int ntrips; struct __thermal_trip *trips; + struct thermal_trip *gtrips; /* cooling binding data */ int num_tbps; @@ -152,6 +153,27 @@ bool of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip) return true; } +/** + * of_thermal_get_trip_points - function to get access to a globally exported + * trip points + * + * @tz: pointer to a thermal zone + * + * This function provides a pointer to the copy of trip points table + * + * Return: pointer to trip points table, NULL otherwise + */ +const struct thermal_trip * const +of_thermal_get_trip_points(struct thermal_zone_device *tz) +{ + struct __thermal_zone *data = tz->devdata; + + if (!data) + return NULL; + + return data->gtrips; +} + static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, enum thermal_trend *trend) { @@ -767,6 +789,16 @@ thermal_of_build_thermal_zone(struct device_node *np) goto free_tbps; } + tz->gtrips = kcalloc(tz->ntrips, sizeof(*tz->gtrips), GFP_KERNEL); + if (!tz->gtrips) { + ret = -ENOMEM; + goto free_tbps; + } + + for (i = 0; i < tz->ntrips; i++) + memcpy(&(tz->gtrips[i]), &(tz->trips[i].temperature), + sizeof(*tz->gtrips)); + finish: of_node_put(child); tz->mode = THERMAL_DEVICE_DISABLED; @@ -793,6 +825,7 @@ static inline void of_thermal_free_zone(struct __thermal_zone *tz) { int i; + kfree(tz->gtrips); for (i = 0; i < tz->num_tbps; i++) of_node_put(tz->tbps[i].cooling_device); kfree(tz->tbps); diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index 466208c..a9580ca 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -91,6 +91,8 @@ int of_parse_thermal_zones(void); void of_thermal_destroy_zones(void); int of_thermal_get_ntrips(struct thermal_zone_device *); bool of_thermal_is_trip_en(struct thermal_zone_device *, int); +const struct thermal_trip * const +of_thermal_get_trip_points(struct thermal_zone_device *); #else static inline int of_parse_thermal_zones(void) { return 0; } static inline void of_thermal_destroy_zones(void) { } @@ -102,6 +104,11 @@ static inline bool of_thermal_is_trip_en(struct thermal_zone_device *, int) { return 0; } +static inline const struct thermal_trip * const +of_thermal_get_trip_points(struct thermal_zone_device *) +{ + return NULL; +} #endif #endif /* __THERMAL_CORE_H__ */ diff --git a/include/linux/thermal.h b/include/linux/thermal.h index 5bc28a7..88d7249 100644 --- a/include/linux/thermal.h +++ b/include/linux/thermal.h @@ -303,6 +303,20 @@ struct thermal_zone_of_device_ops { int (*get_trend)(void *, long *); }; +/** + * struct thermal_trip - Structure representing themal trip points exported from + * of-thermal + * + * @temperature: trip point temperature + * @hysteresis: trip point hysteresis + * @type: trip point type + */ +struct thermal_trip { + unsigned long int temperature; + unsigned long int hysteresis; + enum thermal_trip_type type; +}; + /* Function declarations */ #ifdef CONFIG_THERMAL_OF struct thermal_zone_device *
This patch extends the of-thermal.c to export copy of trip points for a given thermal zone. Thermal drivers should use of_thermal_get_trip_points() method to get pointer to table of thermal trip points. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- Changes for v2: - New patch - as suggested by Eduardo Valentin --- drivers/thermal/of-thermal.c | 33 +++++++++++++++++++++++++++++++++ drivers/thermal/thermal_core.h | 7 +++++++ include/linux/thermal.h | 14 ++++++++++++++ 3 files changed, 54 insertions(+)