Re: [PATCH v3 3/4] thermal/core: Build ascending ordered indexes for the trip points

[Daniel Lezcano]

On 19/07/2022 03:14, Zhang Rui wrote:
> On Mon, 2022-07-18 at 15:21 +0200, Daniel Lezcano wrote:
> > Hi Zhang,
> >
> > thanks for the review
> >
> > On 18/07/2022 07:28, Zhang Rui wrote:
> > > On Fri, 2022-07-15 at 23:09 +0200, Daniel Lezcano wrote:
> >
> > [ ... ]
> >
> > > > Instead of taking the risk of breaking the existing platforms,
> > > > use an
> > > > array of temperature ordered trip identifiers and make it
> > > > available
> > > > for the code needing to browse the trip points in an ordered way.
> > > >
> > > > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > > > ---
> >
> > [ ... ]
> >
> > > > +static void sort_trips_indexes(struct thermal_zone_device *tz)
> > > > +{
> > > > +       int i, j;
> > > > +
> > > > +       for (i = 0; i < tz->trips; i++)
> > > > +               tz->trips_indexes[i] = i;
> > > > +
> > > > +       for (i = 0; i < tz->trips; i++) {
> > > > +               for (j = i + 1; j < tz->trips; j++) {
> > > > +                       int t1, t2;
> > > > +
> > > > +                       tz->ops->get_trip_temp(tz, tz-
> > > > > trips_indexes[i], &t1);
> > >
> > > This line can be moved to the upper loop.
> >
> > Right, thanks!
> >
> > > > +                       tz->ops->get_trip_temp(tz, tz-
> > > > > trips_indexes[j], &t2);
> > > > +
> > >
> > > what about the disabled trip points?
> > >
> > > we should ignore those trip points and check the return value to
> > > make
> > > sure we're comparing the valid trip_temp values.
> >
> > We don't have to care about, whatever the position, the corresponding
> > trip id will be disabled by the trip init function before calling
> > this
> > one and ignored in the handle_thermal_trip() function
>
> hah, I missed this one and replied to your latest reply directly.
>
> The thing I'm concerning is that if we don't check the return value,
> for a disabled trip point, the trip_temp (t1/t2) returned is some
> random value, it all depends on the previous value set by last
> successful .get_trip_temp(), and this may screw up the sorting.

The indexes array is the same size as the trip array, that makes the 
code much less prone to errors.

To have the same number of trip points, the index of the disabled trip 
must be inserted also in the array. We don't care about its position in 
the indexes array because it is discarded in the handle_trip_point() 
function anyway. For this reason, the random temperature of the disabled 
trip point and the resulting position in the sorting is harmless.

It is made on purpose to ignore the return value, so we have a simpler code.

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog