Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1532973

Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case

Show all headers | View raw

On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote:
> On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote:
> > I was thinking while reviewing that the binary search serves more to
> > complicate things than to help -- it's much harder to read (and validate
> > that the loop termination logic is correct). And searching through a few
> > dozen table entries doesn't really get much benefit from a O(n) ->
> > O(log(n)) speed improvement.
> 
> true. but if in your code path you do several walks in the table just to
> check if parameters are valid, given that you could simply decide if
> they are valid or not with simpler if condition, then, still worth, no?
> :-)

Yes, your suggestions seems like they would have made the code both (a
little) more straightforward and efficient. But...

> > Anyway, I'm not sure if you were thinking along the same lines as me.
> > 
> 
> Something like that, except I though of something even simpler:
> +	if ((temp % table->step) != 0)
> +		return -ERANGE;
> 
> If temp passes that check, then you go to the temp -> code conversion.

...that check isn't valid as of patch 4, where Caesar adds handling for
intermediate steps. We really never should have been strictly snapping
to the 5C steps in the first place; intermediate values are OK.

So, we still need some kind of search to find the right step -- or
closest bracketing range, to compute the interpolated value. We should
only reject temperatures that are too high or too low for the ADC to
represent.


--- Side track ---

BTW, when we're considering rejecting temperatures here: shouldn't this
be fed back to the upper layers more nicely? We're improving the error
handling for this driver in this series, but it still leaves things
behaving a little odd. When I tested, I can do:

## set something obviously way too high
echo 700000 > trip_point_X_temp

and get a 0 (success) return code from the sysfs write() syscall, even
though the rockchip driver rejected it with -ERANGE. Is there really no
way to feed back thermal range limits of a sensor to the of-thermal
framework?

Brian

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 0/5] thermal: fixes the rockchip thermal Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
  [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
    Re: [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Eduardo Valentin <edubezval@gmail.com> - 2016-11-29 02:50 +0100
    Re: [PATCH v3 4/5] thermal: rockchip: optimize the conversion table Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 07:30 +0100
  [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Caesar Wang <wxt@rock-chips.com> - 2016-11-28 12:20 +0100
    Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-28 19:50 +0100
    Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-29 02:50 +0100
      Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-29 23:00 +0100
        Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 06:10 +0100
          Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Brian Norris <briannorris@chromium.org> - 2016-11-30 07:00 +0100
            Re: [PATCH v3 3/5] thermal: rockchip: fixes invalid temperature case Eduardo Valentin <edubezval@gmail.com> - 2016-11-30 07:30 +0100

csiph-web