| Message ID | 1445590406-30448-1-git-send-email-pingbo.wen@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 23 October 2015 16:53:26 WEN Pingbo wrote: > 1. Converting timeval to ktime_t, and there is no need to handle sec and > usec separately > > 2. hp_sdc.rtv is only used for time diff, monotonic time is better here > > Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org> > --- This version is still correct and looks better than the first version, but I'd still like you to improve some details: - read some other changelogs and follow the common style. In particular, in the subject line say /what/ you do ("e.g. use ktime_get instead of do_gettimeofday", or "avoid using struct timespec") instead of /why/, but then explain in the changelog text what is wrong with the current version and why it gets changed like this. - Below the '---', add a short list of things you have changed since the previous versions. This part will not get copied into the git history. > - do_gettimeofday(&tv); > - if (tv.tv_sec > hp_sdc.rtv.tv_sec) > - tv.tv_usec += USEC_PER_SEC; > - > - if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) { > + if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) { > hp_sdc_transaction *curr; > uint8_t tmp; > > - printk(KERN_WARNING PREFIX "read timeout (%ius)!\n", > - (int)(tv.tv_usec - hp_sdc.rtv.tv_usec)); > + printk(KERN_WARNING PREFIX "read timeout (%llins)!\n", > + time_diff.tv64); > curr->idx += hp_sdc.rqty; > hp_sdc.rqty = 0; > tmp = curr->seq[curr->actidx]; A tiny style comment here: please use ktime_to_ns() to extract the nanoseconds out of the ktime_t type rather than accessing the tv64 member directly. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday, October 23, 2015 05:25 PM, Arnd Bergmann wrote: > On Friday 23 October 2015 16:53:26 WEN Pingbo wrote: >> 1. Converting timeval to ktime_t, and there is no need to handle sec and >> usec separately >> >> 2. hp_sdc.rtv is only used for time diff, monotonic time is better here >> >> Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org> >> --- > > This version is still correct and looks better than the first version, but > I'd still like you to improve some details: > > - read some other changelogs and follow the common style. In particular, > in the subject line say /what/ you do ("e.g. use ktime_get instead of > do_gettimeofday", or "avoid using struct timespec") instead of /why/, > but then explain in the changelog text what is wrong with the current > version and why it gets changed like this. > > - Below the '---', add a short list of things you have changed since > the previous versions. This part will not get copied into the git > history. > Ok, I will fix this in the next version. >> - do_gettimeofday(&tv); >> - if (tv.tv_sec > hp_sdc.rtv.tv_sec) >> - tv.tv_usec += USEC_PER_SEC; >> - >> - if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) { >> + if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) { >> hp_sdc_transaction *curr; >> uint8_t tmp; >> >> - printk(KERN_WARNING PREFIX "read timeout (%ius)!\n", >> - (int)(tv.tv_usec - hp_sdc.rtv.tv_usec)); >> + printk(KERN_WARNING PREFIX "read timeout (%llins)!\n", >> + time_diff.tv64); >> curr->idx += hp_sdc.rqty; >> hp_sdc.rqty = 0; >> tmp = curr->seq[curr->actidx]; > > A tiny style comment here: please use ktime_to_ns() to extract the > nanoseconds out of the ktime_t type rather than accessing the tv64 > member directly. Same here. Thank you Pingbo > > Arnd > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/serio/hp_sdc.c b/drivers/input/serio/hp_sdc.c index 852858e..8ef82ee 100644 --- a/drivers/input/serio/hp_sdc.c +++ b/drivers/input/serio/hp_sdc.c @@ -193,7 +193,7 @@ static void hp_sdc_take(int irq, void *dev_id, uint8_t status, uint8_t data) curr->seq[curr->idx++] = status; curr->seq[curr->idx++] = data; hp_sdc.rqty -= 2; - do_gettimeofday(&hp_sdc.rtv); + hp_sdc.rtv = ktime_get(); if (hp_sdc.rqty <= 0) { /* All data has been gathered. */ @@ -306,13 +306,9 @@ static void hp_sdc_tasklet(unsigned long foo) write_lock_irq(&hp_sdc.rtq_lock); if (hp_sdc.rcurr >= 0) { - struct timeval tv; + ktime_t time_diff = ktime_sub(ktime_get(), hp_sdc.rtv); - do_gettimeofday(&tv); - if (tv.tv_sec > hp_sdc.rtv.tv_sec) - tv.tv_usec += USEC_PER_SEC; - - if (tv.tv_usec - hp_sdc.rtv.tv_usec > HP_SDC_MAX_REG_DELAY) { + if (time_diff.tv64 > HP_SDC_MAX_REG_DELAY) { hp_sdc_transaction *curr; uint8_t tmp; @@ -321,8 +317,8 @@ static void hp_sdc_tasklet(unsigned long foo) * we'll need to figure out a way to communicate * it back to the application. and be less verbose. */ - printk(KERN_WARNING PREFIX "read timeout (%ius)!\n", - (int)(tv.tv_usec - hp_sdc.rtv.tv_usec)); + printk(KERN_WARNING PREFIX "read timeout (%llins)!\n", + time_diff.tv64); curr->idx += hp_sdc.rqty; hp_sdc.rqty = 0; tmp = curr->seq[curr->actidx]; @@ -551,7 +547,7 @@ unsigned long hp_sdc_put(void) /* Start a new read */ hp_sdc.rqty = curr->seq[curr->idx]; - do_gettimeofday(&hp_sdc.rtv); + hp_sdc.rtv = ktime_get(); curr->idx++; /* Still need to lock here in case of spurious irq. */ write_lock_irq(&hp_sdc.rtq_lock); diff --git a/include/linux/hp_sdc.h b/include/linux/hp_sdc.h index d392975..348a9b5 100644 --- a/include/linux/hp_sdc.h +++ b/include/linux/hp_sdc.h @@ -47,9 +47,9 @@ #endif -/* No 4X status reads take longer than this (in usec). +/* No 4X status reads take longer than this (in nsec). */ -#define HP_SDC_MAX_REG_DELAY 20000 +#define HP_SDC_MAX_REG_DELAY 20000000 typedef void (hp_sdc_irqhook) (int irq, void *dev_id, uint8_t status, uint8_t data); @@ -281,7 +281,7 @@ typedef struct { hp_sdc_transaction *tq[HP_SDC_QUEUE_LEN]; /* All pending read/writes */ int rcurr, rqty; /* Current read transact in process */ - struct timeval rtv; /* Time when current read started */ + ktime_t rtv; /* Time when current read started */ int wcurr; /* Current write transact in process */ int dev_err; /* carries status from registration */
1. Converting timeval to ktime_t, and there is no need to handle sec and usec separately 2. hp_sdc.rtv is only used for time diff, monotonic time is better here Signed-off-by: WEN Pingbo <pingbo.wen@linaro.org> --- drivers/input/serio/hp_sdc.c | 16 ++++++---------- include/linux/hp_sdc.h | 6 +++--- 2 files changed, 9 insertions(+), 13 deletions(-)