On Mon, 12 Aug 2019 10:37:44 +0800 Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > On Sun, 11 Aug 2019 at 16:03, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > On Tue, 6 Aug 2019 15:39:45 +0800 > > Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > > > > > Hi Jonathan, > > > > > > On Mon, 5 Aug 2019 at 21:50, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > > > > > On Mon, 29 Jul 2019 10:19:48 +0800 > > > > Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > > > > > > > > > Hi Jonathan, > > > > > > > > > > On Sun, 28 Jul 2019 at 01:27, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > > > > > > > > > On Thu, 25 Jul 2019 14:33:50 +0800 > > > > > > Baolin Wang <baolin.wang@xxxxxxxxxx> wrote: > > > > > > > > > > > > > From: Freeman Liu <freeman.liu@xxxxxxxxxx> > > > > > > > > > > > > > > On Spreadtrum platform, the headphone will read one ADC channel multiple > > > > > > > times to identify the headphone type, and the headphone identification is > > > > > > > sensitive of the ADC reading time. And we found it will take longer time > > > > > > > to reading ADC data by using interrupt mode comparing with the polling > > > > > > > mode, thus we should change to polling mode to improve the efficiency > > > > > > > of reading data, which can identify the headphone type successfully. > > > > > > > > > > > > > > Signed-off-by: Freeman Liu <freeman.liu@xxxxxxxxxx> > > > > > > > Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxx> > > > > > > > > > > > > Hi, > > > > > > > > > > > > My concerns with this sort of approach is that we may be sacrificing power > > > > > > efficiency for some usecases to support one demanding one. > > > > > > > > > > > > The maximum sleep time is 1 second (I think) which is probably too long > > > > > > to poll a register for in general. > > > > > > > > > > 1 second is the timeout time, that means something wrong when reading > > > > > the data taking 1 second, and we will poll the register status every > > > > > 500 us. > > > > > From the testing, polling mode takes less time than interrupt mode > > > > > when reading ADC data multiple times, so polling mode did not > > > > > sacrifice power > > > > > efficiency. > > > > > > > > Hmm. I'll go with a probably on that, depends on interrupt response > > > > latency etc so isn't entirely obvious. Faster response doesn't necessarily > > > > mean lower power. > > > > > > > > > > > > > > > Is there some way we can bound that time and perhaps switch between > > > > > > interrupt and polling modes depending on how long we expect to wait? > > > > > > > > > > I do not think the interrupt mode is needed any more, since the ADC > > > > > reading is so fast enough usually. Thanks. > > > > The reason for interrupts in such devices is usually precisely the opposite. > > > > > > > > You do it because things are slow enough that you can go to sleep > > > > for a long time before the interrupt occurs. > > > > > > > > So question becomes whether there are circumstances in which we are > > > > running with long timescales and would benefit from using interrupts. > > > > > > From our testing, the ADC version time is usually about 100us, it will > > > be faster to get data if we poll every 50us in this case. But if we > > > change to use interrupt mode, it will take millisecond level time to > > > get data. That will cause problems for those time sensitive scenarios, > > > like headphone detection, that's the main reason we can not use > > > interrupt mode. > > > > > > For those non-time-sensitive scenarios, yes, I agree with you, the > > > interrupt mode will get a better power efficiency. But ADC driver can > > > not know what scenarios asked by consumers, so changing to polling > > > mode seems the easiest way to solve the problem, and we've applied > > > this patch in our downstream kernel for a while, we did not see any > > > other problem. > > > > > > Thanks for your comments. > > > > OK. It's not ideal but sometimes such is life ;) > > Thanks for your understanding :) > > > > > So last question - fix or not? If a fix, can I have a fixes tag > > please. > > This is a bigger patch, I am afraid it can not be merged into stable > kernel, and original code can work at most scenarios. So I think no > need add stable tag for this patch. Thanks. > Fair enough. Needed a bit of merge effort as crossed with a generic cleanup patch it seems. Anyhow, hopefully I got it right. Pushed out as testing for the autobuilders to play with it. Thanks, Jonathan
