Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points

From: Lukasz Majewski
Date: Thu Nov 27 2014 - 04:43:21 EST

Next message: Arend van Spriel: "Re: bluetooth related firmware loader spew on resume."
Previous message: Geert Uytterhoeven: "Re: [RFC PATCH 2/2] lib: devres: Add exec versions of devm_ioremap_resource and friends"
In reply to: Eduardo Valentin: "Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points"
Next in thread: Eduardo Valentin: "Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxxx>
> > >>>> ---
> > >>>> 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
> > >

--
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Arend van Spriel: "Re: bluetooth related firmware loader spew on resume."
Previous message: Geert Uytterhoeven: "Re: [RFC PATCH 2/2] lib: devres: Add exec versions of devm_ioremap_resource and friends"
In reply to: Eduardo Valentin: "Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points"
Next in thread: Eduardo Valentin: "Re: [PATCH v2 3/4] thermal: of: Extend of-thermal to export table of trip points"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]