| Message ID | 1412872737-624-4-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > This patch extends the of-thermal.c to provide information about number of > available non critical (i.e. non HW) trip points in the system. > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > --- > drivers/thermal/of-thermal.c | 12 ++++++++++++ > drivers/thermal/thermal_core.h | 5 +++++ > 2 files changed, 17 insertions(+) > > diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c > index 23c8d6c..cd74e64 100644 > --- a/drivers/thermal/of-thermal.c > +++ b/drivers/thermal/of-thermal.c > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip) > return 1; > } > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *tz) > +{ > + struct __thermal_zone *data = tz->devdata; > + int i; > + > + for (i = 0; i < data->ntrips; i++) > + if (data->trips[i].type != THERMAL_TRIP_CRITICAL) > + continue; > + > + return --i; > +} > + I am not against this addition. But looks like we start to spread some specific APIs that may not be used to other drivers. Maybe having a single API to provide a read-only copy the list / array of trips might be a better approach. I will think of a better way. I also request you to document it accordingly. > static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, > enum thermal_trend *trend) > { > diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h > index ed8ff05..334a7be 100644 > --- a/drivers/thermal/thermal_core.h > +++ b/drivers/thermal/thermal_core.h > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > void of_thermal_destroy_zones(void); > int of_thermal_get_ntrips(struct thermal_zone_device *); > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *); > #else > static inline int of_parse_thermal_zones(void) { return 0; } > static inline void of_thermal_destroy_zones(void) { } > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct thermal_zone_device *, int) > { > return 0; > } > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *) here, it is supposed to be static inline. > +{ > + return 0; > +} > #endif > > #endif /* __THERMAL_CORE_H__ */ > -- > 2.0.0.rc2 >
Hi Eduardo, > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > > This patch extends the of-thermal.c to provide information about > > number of available non critical (i.e. non HW) trip points in the > > system. > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > --- > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > drivers/thermal/thermal_core.h | 5 +++++ > > 2 files changed, 17 insertions(+) > > > > diff --git a/drivers/thermal/of-thermal.c > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > --- a/drivers/thermal/of-thermal.c > > +++ b/drivers/thermal/of-thermal.c > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > thermal_zone_device *tz, int trip) return 1; > > } > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *tz) > > +{ > > + struct __thermal_zone *data = tz->devdata; > > + int i; > > + > > + for (i = 0; i < data->ntrips; i++) > > + if (data->trips[i].type != THERMAL_TRIP_CRITICAL) > > + continue; > > + > > + return --i; > > +} > > + > > > > I am not against this addition. But looks like we start to spread some > specific APIs that may not be used to other drivers. Why do you think that this is a specific API? In the thermal OF we can define trip point as "active", "passive", "hot" and "critical". With the first three we can handle them and properly react. For the last one SoC's PMU will power down the board. Do you know if any board (e.g. from TI) is NOT supposed to shut down when "critical" temperature is passed? The real problem here is the accessibility to __thermal_trip and __thermal_bind arrays. Use case: In the Exynos driver we do need to initialize TMU registers with threshold temperatures. The temperature is read via tz->ops->get_trip_temp() [1] (from of-thermal.c). Unfortunately, the current API is not exporting the number of non-critical trip points to know how many times call [1]. Of course we could by hand instantiate [1] n times, but this looks for me a bit clumsy. Additionally, we now have implicit assumption about the order of defined temperatures for trip points, but I think this is not a big issue. > Maybe having a > single API to provide a read-only copy the list / array of trips might > be a better approach. I will think of a better way. Definitely. Exporting available trip points is crucial. > > I also request you to document it accordingly. Ok, I will do that. > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > int trip, enum thermal_trend *trend) > > { > > diff --git a/drivers/thermal/thermal_core.h > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 > > --- a/drivers/thermal/thermal_core.h > > +++ b/drivers/thermal/thermal_core.h > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > void of_thermal_destroy_zones(void); > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *); > > #else > > static inline int of_parse_thermal_zones(void) { return 0; } > > static inline void of_thermal_destroy_zones(void) { } > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > thermal_zone_device *, int) { > > return 0; > > } > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *) > here, it is supposed to be static inline. > > > +{ > > + return 0; > > +} > > #endif > > > > #endif /* __THERMAL_CORE_H__ */ > > -- > > 2.0.0.rc2 > >
Hello Lukasz, On Fri, Nov 07, 2014 at 11:05:51AM +0100, Lukasz Majewski wrote: > Hi Eduardo, > > > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > > > This patch extends the of-thermal.c to provide information about > > > number of available non critical (i.e. non HW) trip points in the > > > system. > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > --- > > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > > drivers/thermal/thermal_core.h | 5 +++++ > > > 2 files changed, 17 insertions(+) > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > > --- a/drivers/thermal/of-thermal.c > > > +++ b/drivers/thermal/of-thermal.c > > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > > thermal_zone_device *tz, int trip) return 1; > > > } > > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *tz) > > > +{ > > > + struct __thermal_zone *data = tz->devdata; > > > + int i; > > > + > > > + for (i = 0; i < data->ntrips; i++) > > > + if (data->trips[i].type != THERMAL_TRIP_CRITICAL) > > > + continue; > > > + > > > + return --i; > > > +} > > > + > > > > > > > > I am not against this addition. But looks like we start to spread some > > specific APIs that may not be used to other drivers. > > Why do you think that this is a specific API? In the thermal OF we can > define trip point as "active", "passive", "hot" and "critical". > > With the first three we can handle them and properly react. For the last > one SoC's PMU will power down the board. > > Do you know if any board (e.g. from TI) is NOT supposed to shut down > when "critical" temperature is passed? > So, my point is not really about the usage of trip points. It is just that the of-thermal API will grow with in a wide way with specific functions to check some property on the trip point array. And not all drivers may be using these function, e.g. the above proposal. > The real problem here is the accessibility to __thermal_trip and > __thermal_bind arrays. > > Use case: > In the Exynos driver we do need to initialize TMU registers with > threshold temperatures. > The temperature is read via tz->ops->get_trip_temp() [1] (from > of-thermal.c). > Unfortunately, the current API is not exporting the number of > non-critical trip points to know how many times call [1]. > Of course we could by hand instantiate [1] n times, but this looks for > me a bit clumsy. I understand your use case. My point was simply that this use case may be specific to your driver (or few drivers). > > Additionally, we now have implicit assumption about the order of defined > temperatures for trip points, but I think this is not a big issue. > > > Maybe having a > > single API to provide a read-only copy the list / array of trips might > > be a better approach. I will think of a better way. > > Definitely. Exporting available trip points is crucial. > Yeah, I think it is the best thing to do. Share a read-only array / copy of the needed data, and then drivers would check what ever property they need from the array. Just make sure you document that this is a read-only array (i.e. any possible change they do, won't affect the original array). > > > > I also request you to document it accordingly. > > Ok, I will do that. Cool! > > > > > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > > int trip, enum thermal_trend *trend) > > > { > > > diff --git a/drivers/thermal/thermal_core.h > > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 > > > --- a/drivers/thermal/thermal_core.h > > > +++ b/drivers/thermal/thermal_core.h > > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > > void of_thermal_destroy_zones(void); > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *); > > > #else > > > static inline int of_parse_thermal_zones(void) { return 0; } > > > static inline void of_thermal_destroy_zones(void) { } > > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > > thermal_zone_device *, int) { > > > return 0; > > > } > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *) > > here, it is supposed to be static inline. > > > > > +{ > > > + return 0; > > > +} > > > #endif > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > -- > > > 2.0.0.rc2 > > > > > > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Hi Eduardo, > > Hello Lukasz, > > On Fri, Nov 07, 2014 at 11:05:51AM +0100, Lukasz Majewski wrote: > > Hi Eduardo, > > > > > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > > > > This patch extends the of-thermal.c to provide information about > > > > number of available non critical (i.e. non HW) trip points in > > > > the system. > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > --- > > > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > > > drivers/thermal/thermal_core.h | 5 +++++ > > > > 2 files changed, 17 insertions(+) > > > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > > > --- a/drivers/thermal/of-thermal.c > > > > +++ b/drivers/thermal/of-thermal.c > > > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > > > thermal_zone_device *tz, int trip) return 1; > > > > } > > > > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *tz) +{ > > > > + struct __thermal_zone *data = tz->devdata; > > > > + int i; > > > > + > > > > + for (i = 0; i < data->ntrips; i++) > > > > + if (data->trips[i].type != > > > > THERMAL_TRIP_CRITICAL) > > > > + continue; > > > > + > > > > + return --i; > > > > +} > > > > + > > > > > > > > > > > > I am not against this addition. But looks like we start to spread > > > some specific APIs that may not be used to other drivers. > > > > Why do you think that this is a specific API? In the thermal OF we > > can define trip point as "active", "passive", "hot" and "critical". > > > > With the first three we can handle them and properly react. For the > > last one SoC's PMU will power down the board. > > > > Do you know if any board (e.g. from TI) is NOT supposed to shut down > > when "critical" temperature is passed? > > > > So, my point is not really about the usage of trip points. It is just > that the of-thermal API will grow with in a wide way with specific > functions to check some property on the trip point array. And not all > drivers may be using these function, e.g. the above proposal. > > > The real problem here is the accessibility to __thermal_trip and > > __thermal_bind arrays. > > > > Use case: > > In the Exynos driver we do need to initialize TMU registers with > > threshold temperatures. > > The temperature is read via tz->ops->get_trip_temp() [1] (from > > of-thermal.c). > > Unfortunately, the current API is not exporting the number of > > non-critical trip points to know how many times call [1]. > > Of course we could by hand instantiate [1] n times, but this looks > > for me a bit clumsy. > > > I understand your use case. My point was simply that this use case may > be specific to your driver (or few drivers). > > > > Additionally, we now have implicit assumption about the order of > > defined temperatures for trip points, but I think this is not a big > > issue. > > > > > Maybe having a > > > single API to provide a read-only copy the list / array of trips > > > might be a better approach. I will think of a better way. > > > > Definitely. Exporting available trip points is crucial. > > > > Yeah, I think it is the best thing to do. Share a read-only array / > copy of the needed data, and then drivers would check what ever > property they need from the array. Just make sure you document that > this is a read-only array (i.e. any possible change they do, won't > affect the original array). I agree on that. > > > > > > > I also request you to document it accordingly. > > > > Ok, I will do that. > > Cool! > > > > > > > > > > > > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > > > int trip, enum thermal_trend *trend) > > > > { > > > > diff --git a/drivers/thermal/thermal_core.h > > > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 > > > > --- a/drivers/thermal/thermal_core.h > > > > +++ b/drivers/thermal/thermal_core.h > > > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > > > void of_thermal_destroy_zones(void); > > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *); #else > > > > static inline int of_parse_thermal_zones(void) { return 0; } > > > > static inline void of_thermal_destroy_zones(void) { } > > > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > > > thermal_zone_device *, int) { > > > > return 0; > > > > } > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *) > > > here, it is supposed to be static inline. > > > > > > > +{ > > > > + return 0; > > > > +} > > > > #endif > > > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > > -- > > > > 2.0.0.rc2 > > > > > > > > > > > > -- > > Best regards, > > > > Lukasz Majewski > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Hi Lukasz, On Fri, Nov 7, 2014 at 2:05 AM, Lukasz Majewski <l.majewski@samsung.com> wrote: > Hi Eduardo, > >> On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: >> > This patch extends the of-thermal.c to provide information about >> > number of available non critical (i.e. non HW) trip points in the >> > system. >> > >> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> >> > --- >> > drivers/thermal/of-thermal.c | 12 ++++++++++++ >> > drivers/thermal/thermal_core.h | 5 +++++ >> > 2 files changed, 17 insertions(+) >> > >> > diff --git a/drivers/thermal/of-thermal.c >> > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 >> > --- a/drivers/thermal/of-thermal.c >> > +++ b/drivers/thermal/of-thermal.c >> > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct >> > thermal_zone_device *tz, int trip) return 1; >> > } >> > >> > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *tz) >> > +{ >> > + struct __thermal_zone *data = tz->devdata; >> > + int i; >> > + >> > + for (i = 0; i < data->ntrips; i++) >> > + if (data->trips[i].type != THERMAL_TRIP_CRITICAL) >> > + continue; >> > + >> > + return --i; >> > +} >> > + >> >> >> >> I am not against this addition. But looks like we start to spread some >> specific APIs that may not be used to other drivers. > > Why do you think that this is a specific API? In the thermal OF we can > define trip point as "active", "passive", "hot" and "critical". > > With the first three we can handle them and properly react. For the last > one SoC's PMU will power down the board. > > Do you know if any board (e.g. from TI) is NOT supposed to shut down > when "critical" temperature is passed? > > The real problem here is the accessibility to __thermal_trip and > __thermal_bind arrays. > > Use case: > In the Exynos driver we do need to initialize TMU registers with > threshold temperatures. Is this indeed plural or you want to program the next trip point as your alarm temperature? If so, there was another patch floating around adding set_trips() callback that would get passed in the low and high trip points for the current temperature reading and you can use that data to program alarms. Rockchip thermal driver uses it. https://lkml.org/lkml/2014/6/27/76 Thanks.
Hi Eduardo, > > Hello Lukasz, > > On Fri, Nov 07, 2014 at 11:05:51AM +0100, Lukasz Majewski wrote: > > Hi Eduardo, > > > > > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > > > > This patch extends the of-thermal.c to provide information about > > > > number of available non critical (i.e. non HW) trip points in > > > > the system. > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > --- > > > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > > > drivers/thermal/thermal_core.h | 5 +++++ > > > > 2 files changed, 17 insertions(+) > > > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > > > --- a/drivers/thermal/of-thermal.c > > > > +++ b/drivers/thermal/of-thermal.c > > > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > > > thermal_zone_device *tz, int trip) return 1; > > > > } > > > > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *tz) +{ > > > > + struct __thermal_zone *data = tz->devdata; > > > > + int i; > > > > + > > > > + for (i = 0; i < data->ntrips; i++) > > > > + if (data->trips[i].type != > > > > THERMAL_TRIP_CRITICAL) > > > > + continue; > > > > + > > > > + return --i; > > > > +} > > > > + > > > > > > > > > > > > I am not against this addition. But looks like we start to spread > > > some specific APIs that may not be used to other drivers. > > > > Why do you think that this is a specific API? In the thermal OF we > > can define trip point as "active", "passive", "hot" and "critical". > > > > With the first three we can handle them and properly react. For the > > last one SoC's PMU will power down the board. > > > > Do you know if any board (e.g. from TI) is NOT supposed to shut down > > when "critical" temperature is passed? > > > > So, my point is not really about the usage of trip points. It is just > that the of-thermal API will grow with in a wide way with specific > functions to check some property on the trip point array. And not all > drivers may be using these function, e.g. the above proposal. > > > The real problem here is the accessibility to __thermal_trip and > > __thermal_bind arrays. > > > > Use case: > > In the Exynos driver we do need to initialize TMU registers with > > threshold temperatures. > > The temperature is read via tz->ops->get_trip_temp() [1] (from > > of-thermal.c). > > Unfortunately, the current API is not exporting the number of > > non-critical trip points to know how many times call [1]. > > Of course we could by hand instantiate [1] n times, but this looks > > for me a bit clumsy. > > > I understand your use case. My point was simply that this use case may > be specific to your driver (or few drivers). > > > > Additionally, we now have implicit assumption about the order of > > defined temperatures for trip points, but I think this is not a big > > issue. > > > > > Maybe having a > > > single API to provide a read-only copy the list / array of trips > > > might be a better approach. I will think of a better way. > > > > Definitely. Exporting available trip points is crucial. > > > > Yeah, I think it is the best thing to do. Share a read-only array / > copy of the needed data, and then drivers would check what ever > property they need from the array. Just make sure you document that > this is a read-only array (i.e. any possible change they do, won't > affect the original array). So I assume that you don't mind that I will prepare such of-thermal.c modification? > > > > > > > I also request you to document it accordingly. > > > > Ok, I will do that. > > Cool! > > > > > > > > > > > > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > > > int trip, enum thermal_trend *trend) > > > > { > > > > diff --git a/drivers/thermal/thermal_core.h > > > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 > > > > --- a/drivers/thermal/thermal_core.h > > > > +++ b/drivers/thermal/thermal_core.h > > > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > > > void of_thermal_destroy_zones(void); > > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *); #else > > > > static inline int of_parse_thermal_zones(void) { return 0; } > > > > static inline void of_thermal_destroy_zones(void) { } > > > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > > > thermal_zone_device *, int) { > > > > return 0; > > > > } > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > *) > > > here, it is supposed to be static inline. > > > > > > > +{ > > > > + return 0; > > > > +} > > > > #endif > > > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > > -- > > > > 2.0.0.rc2 > > > > > > > > > > > > -- > > Best regards, > > > > Lukasz Majewski > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
Hello Lukasz, On Wed, Nov 12, 2014 at 10:42:41AM +0100, Lukasz Majewski wrote: > Hi Eduardo, > > > > > Hello Lukasz, > > > > On Fri, Nov 07, 2014 at 11:05:51AM +0100, Lukasz Majewski wrote: > > > Hi Eduardo, > > > > > > > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski wrote: > > > > > This patch extends the of-thermal.c to provide information about > > > > > number of available non critical (i.e. non HW) trip points in > > > > > the system. > > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > > --- > > > > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > > > > drivers/thermal/thermal_core.h | 5 +++++ > > > > > 2 files changed, 17 insertions(+) > > > > > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > > > > --- a/drivers/thermal/of-thermal.c > > > > > +++ b/drivers/thermal/of-thermal.c > > > > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > > > > thermal_zone_device *tz, int trip) return 1; > > > > > } > > > > > > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > > *tz) +{ > > > > > + struct __thermal_zone *data = tz->devdata; > > > > > + int i; > > > > > + > > > > > + for (i = 0; i < data->ntrips; i++) > > > > > + if (data->trips[i].type != > > > > > THERMAL_TRIP_CRITICAL) > > > > > + continue; > > > > > + > > > > > + return --i; > > > > > +} > > > > > + > > > > > > > > > > > > > > > > I am not against this addition. But looks like we start to spread > > > > some specific APIs that may not be used to other drivers. > > > > > > Why do you think that this is a specific API? In the thermal OF we > > > can define trip point as "active", "passive", "hot" and "critical". > > > > > > With the first three we can handle them and properly react. For the > > > last one SoC's PMU will power down the board. > > > > > > Do you know if any board (e.g. from TI) is NOT supposed to shut down > > > when "critical" temperature is passed? > > > > > > > So, my point is not really about the usage of trip points. It is just > > that the of-thermal API will grow with in a wide way with specific > > functions to check some property on the trip point array. And not all > > drivers may be using these function, e.g. the above proposal. > > > > > The real problem here is the accessibility to __thermal_trip and > > > __thermal_bind arrays. > > > > > > Use case: > > > In the Exynos driver we do need to initialize TMU registers with > > > threshold temperatures. > > > The temperature is read via tz->ops->get_trip_temp() [1] (from > > > of-thermal.c). > > > Unfortunately, the current API is not exporting the number of > > > non-critical trip points to know how many times call [1]. > > > Of course we could by hand instantiate [1] n times, but this looks > > > for me a bit clumsy. > > > > > > I understand your use case. My point was simply that this use case may > > be specific to your driver (or few drivers). > > > > > > Additionally, we now have implicit assumption about the order of > > > defined temperatures for trip points, but I think this is not a big > > > issue. > > > > > > > Maybe having a > > > > single API to provide a read-only copy the list / array of trips > > > > might be a better approach. I will think of a better way. > > > > > > Definitely. Exporting available trip points is crucial. > > > > > > > Yeah, I think it is the best thing to do. Share a read-only array / > > copy of the needed data, and then drivers would check what ever > > property they need from the array. Just make sure you document that > > this is a read-only array (i.e. any possible change they do, won't > > affect the original array). > > So I assume that you don't mind that I will prepare such of-thermal.c > modification? No, please, feel free to send the proposal along with your patchset. To me, it makes sense you do it, because you will also present a real use case of this required change. > > > > > > > > > > > I also request you to document it accordingly. > > > > > > Ok, I will do that. > > > > Cool! > > > > > > > > > > > > > > > > > > > > > > static int of_thermal_get_trend(struct thermal_zone_device *tz, > > > > > int trip, enum thermal_trend *trend) > > > > > { > > > > > diff --git a/drivers/thermal/thermal_core.h > > > > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 > > > > > --- a/drivers/thermal/thermal_core.h > > > > > +++ b/drivers/thermal/thermal_core.h > > > > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > > > > void of_thermal_destroy_zones(void); > > > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > > > int of_thermal_is_trip_en(struct thermal_zone_device *, int); > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > > *); #else > > > > > static inline int of_parse_thermal_zones(void) { return 0; } > > > > > static inline void of_thermal_destroy_zones(void) { } > > > > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > > > > thermal_zone_device *, int) { > > > > > return 0; > > > > > } > > > > > +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device > > > > > *) > > > > here, it is supposed to be static inline. > > > > > > > > > +{ > > > > > + return 0; > > > > > +} > > > > > #endif > > > > > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > > > -- > > > > > 2.0.0.rc2 > > > > > > > > > > > > > > > > > -- > > > Best regards, > > > > > > Lukasz Majewski > > > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > > > -- > Best regards, > > Lukasz Majewski > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
On Tue, 18 Nov 2014 11:20:26 -0400 Eduardo Valentin <edubezval@gmail.com> wrote: > Hello Lukasz, > > On Wed, Nov 12, 2014 at 10:42:41AM +0100, Lukasz Majewski wrote: > > Hi Eduardo, > > > > > > > > Hello Lukasz, > > > > > > On Fri, Nov 07, 2014 at 11:05:51AM +0100, Lukasz Majewski wrote: > > > > Hi Eduardo, > > > > > > > > > On Thu, Oct 09, 2014 at 06:38:39PM +0200, Lukasz Majewski > > > > > wrote: > > > > > > This patch extends the of-thermal.c to provide information > > > > > > about number of available non critical (i.e. non HW) trip > > > > > > points in the system. > > > > > > > > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > > > > > --- > > > > > > drivers/thermal/of-thermal.c | 12 ++++++++++++ > > > > > > drivers/thermal/thermal_core.h | 5 +++++ > > > > > > 2 files changed, 17 insertions(+) > > > > > > > > > > > > diff --git a/drivers/thermal/of-thermal.c > > > > > > b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 > > > > > > --- a/drivers/thermal/of-thermal.c > > > > > > +++ b/drivers/thermal/of-thermal.c > > > > > > @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct > > > > > > thermal_zone_device *tz, int trip) return 1; > > > > > > } > > > > > > > > > > > > +int of_thermal_get_non_crit_ntrips(struct > > > > > > thermal_zone_device *tz) +{ > > > > > > + struct __thermal_zone *data = tz->devdata; > > > > > > + int i; > > > > > > + > > > > > > + for (i = 0; i < data->ntrips; i++) > > > > > > + if (data->trips[i].type != > > > > > > THERMAL_TRIP_CRITICAL) > > > > > > + continue; > > > > > > + > > > > > > + return --i; > > > > > > +} > > > > > > + > > > > > > > > > > > > > > > > > > > > I am not against this addition. But looks like we start to > > > > > spread some specific APIs that may not be used to other > > > > > drivers. > > > > > > > > Why do you think that this is a specific API? In the thermal OF > > > > we can define trip point as "active", "passive", "hot" and > > > > "critical". > > > > > > > > With the first three we can handle them and properly react. For > > > > the last one SoC's PMU will power down the board. > > > > > > > > Do you know if any board (e.g. from TI) is NOT supposed to shut > > > > down when "critical" temperature is passed? > > > > > > > > > > So, my point is not really about the usage of trip points. It is > > > just that the of-thermal API will grow with in a wide way with > > > specific functions to check some property on the trip point > > > array. And not all drivers may be using these function, e.g. the > > > above proposal. > > > > > > > The real problem here is the accessibility to __thermal_trip and > > > > __thermal_bind arrays. > > > > > > > > Use case: > > > > In the Exynos driver we do need to initialize TMU registers with > > > > threshold temperatures. > > > > The temperature is read via tz->ops->get_trip_temp() [1] (from > > > > of-thermal.c). > > > > Unfortunately, the current API is not exporting the number of > > > > non-critical trip points to know how many times call [1]. > > > > Of course we could by hand instantiate [1] n times, but this > > > > looks for me a bit clumsy. > > > > > > > > > I understand your use case. My point was simply that this use > > > case may be specific to your driver (or few drivers). > > > > > > > > Additionally, we now have implicit assumption about the order of > > > > defined temperatures for trip points, but I think this is not a > > > > big issue. > > > > > > > > > Maybe having a > > > > > single API to provide a read-only copy the list / array of > > > > > trips might be a better approach. I will think of a better > > > > > way. > > > > > > > > Definitely. Exporting available trip points is crucial. > > > > > > > > > > Yeah, I think it is the best thing to do. Share a read-only > > > array / copy of the needed data, and then drivers would check > > > what ever property they need from the array. Just make sure you > > > document that this is a read-only array (i.e. any possible change > > > they do, won't affect the original array). > > > > So I assume that you don't mind that I will prepare such > > of-thermal.c modification? > > No, please, feel free to send the proposal along with your patchset. > To me, it makes sense you do it, because you will also present a real > use case of this required change. Ok. I will rebase on top of your today's work. > > > > > > > > > > > > > > > > I also request you to document it accordingly. > > > > > > > > Ok, I will do that. > > > > > > Cool! > > > > > > > > > > > > > > > > > > > > > > > > > > > > > static int of_thermal_get_trend(struct thermal_zone_device > > > > > > *tz, int trip, enum thermal_trend *trend) > > > > > > { > > > > > > diff --git a/drivers/thermal/thermal_core.h > > > > > > b/drivers/thermal/thermal_core.h index ed8ff05..334a7be > > > > > > 100644 --- a/drivers/thermal/thermal_core.h > > > > > > +++ b/drivers/thermal/thermal_core.h > > > > > > @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); > > > > > > void of_thermal_destroy_zones(void); > > > > > > int of_thermal_get_ntrips(struct thermal_zone_device *); > > > > > > int of_thermal_is_trip_en(struct thermal_zone_device *, > > > > > > int); +int of_thermal_get_non_crit_ntrips(struct > > > > > > thermal_zone_device *); #else > > > > > > static inline int of_parse_thermal_zones(void) { return > > > > > > 0; } static inline void of_thermal_destroy_zones(void) { } > > > > > > @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct > > > > > > thermal_zone_device *, int) { > > > > > > return 0; > > > > > > } > > > > > > +int of_thermal_get_non_crit_ntrips(struct > > > > > > thermal_zone_device *) > > > > > here, it is supposed to be static inline. > > > > > > > > > > > +{ > > > > > > + return 0; > > > > > > +} > > > > > > #endif > > > > > > > > > > > > #endif /* __THERMAL_CORE_H__ */ > > > > > > -- > > > > > > 2.0.0.rc2 > > > > > > > > > > > > > > > > > > > > > > -- > > > > Best regards, > > > > > > > > Lukasz Majewski > > > > > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group > > > > > > > > -- > > Best regards, > > > > Lukasz Majewski > > > > Samsung R&D Institute Poland (SRPOL) | Linux Platform Group Best regards, Lukasz Majewski
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c index 23c8d6c..cd74e64 100644 --- a/drivers/thermal/of-thermal.c +++ b/drivers/thermal/of-thermal.c @@ -128,6 +128,18 @@ int of_thermal_is_trip_en(struct thermal_zone_device *tz, int trip) return 1; } +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *tz) +{ + struct __thermal_zone *data = tz->devdata; + int i; + + for (i = 0; i < data->ntrips; i++) + if (data->trips[i].type != THERMAL_TRIP_CRITICAL) + continue; + + return --i; +} + static int of_thermal_get_trend(struct thermal_zone_device *tz, int trip, enum thermal_trend *trend) { diff --git a/drivers/thermal/thermal_core.h b/drivers/thermal/thermal_core.h index ed8ff05..334a7be 100644 --- a/drivers/thermal/thermal_core.h +++ b/drivers/thermal/thermal_core.h @@ -83,6 +83,7 @@ int of_parse_thermal_zones(void); void of_thermal_destroy_zones(void); int of_thermal_get_ntrips(struct thermal_zone_device *); int of_thermal_is_trip_en(struct thermal_zone_device *, int); +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *); #else static inline int of_parse_thermal_zones(void) { return 0; } static inline void of_thermal_destroy_zones(void) { } @@ -94,6 +95,10 @@ int of_thermal_is_trip_en(struct thermal_zone_device *, int) { return 0; } +int of_thermal_get_non_crit_ntrips(struct thermal_zone_device *) +{ + return 0; +} #endif #endif /* __THERMAL_CORE_H__ */
This patch extends the of-thermal.c to provide information about number of available non critical (i.e. non HW) trip points in the system. Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> --- drivers/thermal/of-thermal.c | 12 ++++++++++++ drivers/thermal/thermal_core.h | 5 +++++ 2 files changed, 17 insertions(+)