On 18/10/2017 17:51, Eduardo Valentin wrote: > On Wed, Oct 18, 2017 at 09:48:21AM +0800, Leo Yan wrote: >> On Tue, Oct 17, 2017 at 02:07:08PM -0700, Eduardo Valentin wrote: >>> 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?). >> >> Sorry I missed to reply this patch. And yes, I have tested and >> reviewed it at my side: >> >> Reviewed-by: Leo Yan <leo....@linaro.org> >> >> P.s. I am working for Linaro; I am continously co-working with >> Hisilicon to maintain this driver due it's important for Hikey/Hikey960 >> two boards stability; this driver also is important for our daily >> profiling for power and performance. Eduardo, so please let us know if >> you still need ack from Hisilicon engineer. > > > Yeah, I think adding your Reviewed-by and Kevin's is enough for this > series to go through. As I asked Daniel already, only few minor stuff > needs to be fixed along with the addition of the reviewed-by's.
The different warnings you reported are fixed and the reviewed-by's / acked-by's added. I think the patches 19-25 may need an extra look, so I will resend all the other patches meanwhile. Does it sound good? -- <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