[lkml]   [2017]   [Oct]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 02/25] thermal/drivers/hisi: Remove the multiple sensors support

在 2017/10/18 5:07, Eduardo Valentin 写道:
> On Tue, Oct 17, 2017 at 09:03:40PM +0200, Daniel Lezcano wrote:
>> On 17/10/2017 20:25, Eduardo Valentin wrote:
>>> Hello,
>>> On Tue, Oct 17, 2017 at 02:28:27PM +0200, Daniel Lezcano wrote:
>>>> On 17/10/2017 05:54, Eduardo Valentin wrote:
>>>>> On Tue, Oct 10, 2017 at 08:02:27PM +0200, Daniel Lezcano wrote:
>>>>>> By essence, the tsensor does not really support multiple sensor at the same
>>>>>> time. It allows to set a sensor and use it to get the temperature, another
>>>>>> sensor could be switched but with a delay of 3-5ms. It is difficult to read
>>>>>> simultaneously several sensors without a big delay.
>>>>> Is 3-5 ms enough to loose an event? Is this really a problem?
>>>> There are several aspects:
>>>> - the multiple sensors is not needed here
>>> Well, that is debatable, I cannot really agree or disagree with the
>>> above statement without understanding the use cases and most important,
>>> the location of each sensor. What is the location of each sensor?
>>>> - the temperature controller is not designed to read several sensors at
>>>> the same time, we switch the sensor and that clears some internal
>>>> buffers and re-init the controller
>>> Which is still very helpful in case you have multiple hotspots that you
>>> want to track and they are exposed on different workloads. Sacrificing
>>> the availability of sensors is something needs a better justification
>>> other than "current code uses only one".
>>>> - some boards can take 40°C in 1 sec, the temperature increase is
>>>> insanely fast and reading several sensors add an extra 15ms.
>>> Ok... What is the difference in update rate with and without the switch
>>> of sensors? With the above worst case, you have about 4/6 mC/ms. Can
>>> your tsensor support that resolution for a single sensor? What is the
>>> maximum resolution a tsensor can support? What is the penalty added with
>>> switch?
>>> Based on this data, and the above 3-5ms, that means you would miss about
>>> ~ 3 - 4 mC while switching ( assuming tsensor can really achieve the
>>> above rate of change: 5ms * 4/6 mC /ms). Are you sure that is
>>> enough justification to drop three extra sensors?
>> Ok if I refer to the documentation the rate is 0.768 ms with the current
>> configuration.
>> The driver is currently bogus: register overwritten, bouncing interrupt,
>> unneeded lock, ... So the proposition was to remove the multiple sensors
>> support, clean the driver, and re-introduce it if there is a need.
>> If I remember correctly Leo, author of the driver, agreed on this. Leo ?
>> Note, I'm not strongly against multiple sensors support in the driver if
>> you think it is convenient but it is much simpler to remove the current
>> code as it is not used and put it back on top of a sane foundation
>> instead of circumventing that on the existing code.
> I am also fine with the above strategy, as long as you are sure you are
> not breaking anyone (specially userspace). Also, it would be good to get
> a reviewed-by from hisilicon just to confirm (Leo?).
I agree with Daniel, we only care about CPU temperature on hikey, and currently
mutiple sensors support are actually not used.
> Besides, once you get his reviewed-by, and add it to the patches,
> can you please resend the series with the minor issues I
> mentioned (a few minor checkpatch issues and one compilation warn that
> is added to the driver after the series is applied).
>> --
>> <> │ Open source software for ARM SoCs
>> Follow Linaro: <> Facebook |
>> <!/linaroorg> Twitter |
>> <> Blog

 \ /
  Last update: 2017-10-22 17:08    [W:0.099 / U:4.264 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site