| Message ID | 20161025002752.11939-1-brian.brooks@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 10/31 17:20:27, Bill Fischofer wrote: > I've added this to tomorrow's ODP call to discuss, however some general > comments: > > 1. These should be marked [RFC API-NEXT PATCH] so that Patchworks will pick > them up as patches. Without the PATCH keyword they just appear to be > comments on the list. > > 2. The short log should follow the CONTRIBUTING guidelines. In particular > these should be api: timer: <description> to flag that these are proposed > API changes. OK. I wasn't sure if 'api: ' prefix was needed since it includes implementation and test.. > 3. APIs should be in a separate patch for their implementation to > facilitate review. Generally it's best to have a patch for API definition, > another for implementation, another for validation, and another for > documentation so that these functional areas can be considered individually. Since these 2 patches are extremely succinct (< 100 lines) I thought it was easiest for reviewers to see it in 2 patches instead of 8. I didn't check the docs for guidelines on this. Are these 2 patches candidates for breaking up into multiple patches? > On Mon, Oct 24, 2016 at 7:27 PM, Brian Brooks <brian.brooks@linaro.org> > wrote: > > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > > --- > > include/odp/api/spec/timer.h | 9 +++++++++ > > platform/linux-generic/odp_timer.c | 5 +++++ > > test/common_plat/validation/api/timer/timer.c | 2 ++ > > 3 files changed, 16 insertions(+) > > > > diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h > > index 3f8fdd4..540da44 100644 > > --- a/include/odp/api/spec/timer.h > > +++ b/include/odp/api/spec/timer.h > > @@ -191,6 +191,15 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > > odp_timer_pool_info_t *info); > > > > /** > > + * Get resolution from timer pool > > + * > > + * @param tpid Timer pool identifier > > + * > > + * @return Timeout resolution in nanoseconds > > + */ > > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid); > > > > res is confusingly short here. Does it mean reserve? restore? I'd prefer > odp_timer_pool_resolution() Agree. Went with 'res' to be consistent with 'res_ns' in timer pool params. I think updating rest of code would be a net win but also as a follow-up patch to this (or someone could submit a patch which would force me to rebase this one with updated naming!). > > + > > +/** > > * Allocate a timer > > * > > * Create a timer (allocating all necessary resources e.g. timeout event) > > from > > diff --git a/platform/linux-generic/odp_timer.c > > b/platform/linux-generic/odp_timer.c > > index ee4c4c0..d4b30b2 100644 > > --- a/platform/linux-generic/odp_timer.c > > +++ b/platform/linux-generic/odp_timer.c > > @@ -837,6 +837,11 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > > return 0; > > } > > > > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid) > > +{ > > + return tpid->param.res_ns; > > +} > > + > > uint64_t odp_timer_pool_to_u64(odp_timer_pool_t tpid) > > { > > return _odp_pri(tpid); > > diff --git a/test/common_plat/validation/api/timer/timer.c > > b/test/common_plat/validation/api/timer/timer.c > > index 0007639..a8321f3 100644 > > --- a/test/common_plat/validation/api/timer/timer.c > > +++ b/test/common_plat/validation/api/timer/timer.c > > @@ -529,6 +529,8 @@ void timer_test_odp_timer_all(void) > > CU_ASSERT(tpinfo.param.max_tmo == MAX); > > CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); > > > > + CU_ASSERT(odp_timer_pool_res(tp) == RES); > > + > > LOG_DBG("Timer pool handle: %" PRIu64 "\n", > > odp_timer_pool_to_u64(tp)); > > LOG_DBG("#timers..: %u\n", NTIMERS); > > LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, > > -- > > 2.10.1 > > > >
I've added this to tomorrow's ODP call to discuss, however some general comments: 1. These should be marked [RFC API-NEXT PATCH] so that Patchworks will pick them up as patches. Without the PATCH keyword they just appear to be comments on the list. 2. The short log should follow the CONTRIBUTING guidelines. In particular these should be api: timer: <description> to flag that these are proposed API changes. 3. APIs should be in a separate patch for their implementation to facilitate review. Generally it's best to have a patch for API definition, another for implementation, another for validation, and another for documentation so that these functional areas can be considered individually. On Mon, Oct 24, 2016 at 7:27 PM, Brian Brooks <brian.brooks@linaro.org> wrote: > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > --- > include/odp/api/spec/timer.h | 9 +++++++++ > platform/linux-generic/odp_timer.c | 5 +++++ > test/common_plat/validation/api/timer/timer.c | 2 ++ > 3 files changed, 16 insertions(+) > > diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h > index 3f8fdd4..540da44 100644 > --- a/include/odp/api/spec/timer.h > +++ b/include/odp/api/spec/timer.h > @@ -191,6 +191,15 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > odp_timer_pool_info_t *info); > > /** > + * Get resolution from timer pool > + * > + * @param tpid Timer pool identifier > + * > + * @return Timeout resolution in nanoseconds > + */ > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid); > res is confusingly short here. Does it mean reserve? restore? I'd prefer odp_timer_pool_resolution() > + > +/** > * Allocate a timer > * > * Create a timer (allocating all necessary resources e.g. timeout event) > from > diff --git a/platform/linux-generic/odp_timer.c > b/platform/linux-generic/odp_timer.c > index ee4c4c0..d4b30b2 100644 > --- a/platform/linux-generic/odp_timer.c > +++ b/platform/linux-generic/odp_timer.c > @@ -837,6 +837,11 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > return 0; > } > > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid) > +{ > + return tpid->param.res_ns; > +} > + > uint64_t odp_timer_pool_to_u64(odp_timer_pool_t tpid) > { > return _odp_pri(tpid); > diff --git a/test/common_plat/validation/api/timer/timer.c > b/test/common_plat/validation/api/timer/timer.c > index 0007639..a8321f3 100644 > --- a/test/common_plat/validation/api/timer/timer.c > +++ b/test/common_plat/validation/api/timer/timer.c > @@ -529,6 +529,8 @@ void timer_test_odp_timer_all(void) > CU_ASSERT(tpinfo.param.max_tmo == MAX); > CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); > > + CU_ASSERT(odp_timer_pool_res(tp) == RES); > + > LOG_DBG("Timer pool handle: %" PRIu64 "\n", > odp_timer_pool_to_u64(tp)); > LOG_DBG("#timers..: %u\n", NTIMERS); > LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, > -- > 2.10.1 > >
_capabilities() to match other functions. On 25 October 2016 at 10:38, Brian Brooks <brian.brooks@linaro.org> wrote: > On 10/31 17:20:27, Bill Fischofer wrote: > > I've added this to tomorrow's ODP call to discuss, however some general > > comments: > > > > 1. These should be marked [RFC API-NEXT PATCH] so that Patchworks will > pick > > them up as patches. Without the PATCH keyword they just appear to be > > comments on the list. > > > > 2. The short log should follow the CONTRIBUTING guidelines. In particular > > these should be api: timer: <description> to flag that these are proposed > > API changes. > > OK. I wasn't sure if 'api: ' prefix was needed since it includes > implementation > and test.. > > > 3. APIs should be in a separate patch for their implementation to > > facilitate review. Generally it's best to have a patch for API > definition, > > another for implementation, another for validation, and another for > > documentation so that these functional areas can be considered > individually. > > Since these 2 patches are extremely succinct (< 100 lines) I thought it > was easiest for reviewers to see it in 2 patches instead of 8. I didn't > check > the docs for guidelines on this. Are these 2 patches candidates for > breaking up > into multiple patches? > > > On Mon, Oct 24, 2016 at 7:27 PM, Brian Brooks <brian.brooks@linaro.org> > > wrote: > > > > > Signed-off-by: Brian Brooks <brian.brooks@linaro.org> > > > --- > > > include/odp/api/spec/timer.h | 9 +++++++++ > > > platform/linux-generic/odp_timer.c | 5 +++++ > > > test/common_plat/validation/api/timer/timer.c | 2 ++ > > > 3 files changed, 16 insertions(+) > > > > > > diff --git a/include/odp/api/spec/timer.h > b/include/odp/api/spec/timer.h > > > index 3f8fdd4..540da44 100644 > > > --- a/include/odp/api/spec/timer.h > > > +++ b/include/odp/api/spec/timer.h > > > @@ -191,6 +191,15 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > > > odp_timer_pool_info_t *info); > > > > > > /** > > > + * Get resolution from timer pool > > > + * > > > + * @param tpid Timer pool identifier > > > + * > > > + * @return Timeout resolution in nanoseconds > > > + */ > > > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid); > > > > > > > res is confusingly short here. Does it mean reserve? restore? I'd prefer > > odp_timer_pool_resolution() > > Agree. Went with 'res' to be consistent with 'res_ns' in timer pool params. > I think updating rest of code would be a net win but also as a follow-up > patch to this (or someone could submit a patch which would force me to > rebase > this one with updated naming!). > > > > + > > > +/** > > > * Allocate a timer > > > * > > > * Create a timer (allocating all necessary resources e.g. timeout > event) > > > from > > > diff --git a/platform/linux-generic/odp_timer.c > > > b/platform/linux-generic/odp_timer.c > > > index ee4c4c0..d4b30b2 100644 > > > --- a/platform/linux-generic/odp_timer.c > > > +++ b/platform/linux-generic/odp_timer.c > > > @@ -837,6 +837,11 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, > > > return 0; > > > } > > > > > > +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid) > > > +{ > > > + return tpid->param.res_ns; > > > +} > > > + > > > uint64_t odp_timer_pool_to_u64(odp_timer_pool_t tpid) > > > { > > > return _odp_pri(tpid); > > > diff --git a/test/common_plat/validation/api/timer/timer.c > > > b/test/common_plat/validation/api/timer/timer.c > > > index 0007639..a8321f3 100644 > > > --- a/test/common_plat/validation/api/timer/timer.c > > > +++ b/test/common_plat/validation/api/timer/timer.c > > > @@ -529,6 +529,8 @@ void timer_test_odp_timer_all(void) > > > CU_ASSERT(tpinfo.param.max_tmo == MAX); > > > CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); > > > > > > + CU_ASSERT(odp_timer_pool_res(tp) == RES); > > > + > > > LOG_DBG("Timer pool handle: %" PRIu64 "\n", > > > odp_timer_pool_to_u64(tp)); > > > LOG_DBG("#timers..: %u\n", NTIMERS); > > > LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS, > > > -- > > > 2.10.1 > > > > > > >
diff --git a/include/odp/api/spec/timer.h b/include/odp/api/spec/timer.h index 3f8fdd4..540da44 100644 --- a/include/odp/api/spec/timer.h +++ b/include/odp/api/spec/timer.h @@ -191,6 +191,15 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, odp_timer_pool_info_t *info); /** + * Get resolution from timer pool + * + * @param tpid Timer pool identifier + * + * @return Timeout resolution in nanoseconds + */ +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid); + +/** * Allocate a timer * * Create a timer (allocating all necessary resources e.g. timeout event) from diff --git a/platform/linux-generic/odp_timer.c b/platform/linux-generic/odp_timer.c index ee4c4c0..d4b30b2 100644 --- a/platform/linux-generic/odp_timer.c +++ b/platform/linux-generic/odp_timer.c @@ -837,6 +837,11 @@ int odp_timer_pool_info(odp_timer_pool_t tpid, return 0; } +uint64_t odp_timer_pool_res(odp_timer_pool_t tpid) +{ + return tpid->param.res_ns; +} + uint64_t odp_timer_pool_to_u64(odp_timer_pool_t tpid) { return _odp_pri(tpid); diff --git a/test/common_plat/validation/api/timer/timer.c b/test/common_plat/validation/api/timer/timer.c index 0007639..a8321f3 100644 --- a/test/common_plat/validation/api/timer/timer.c +++ b/test/common_plat/validation/api/timer/timer.c @@ -529,6 +529,8 @@ void timer_test_odp_timer_all(void) CU_ASSERT(tpinfo.param.max_tmo == MAX); CU_ASSERT(strcmp(tpinfo.name, NAME) == 0); + CU_ASSERT(odp_timer_pool_res(tp) == RES); + LOG_DBG("Timer pool handle: %" PRIu64 "\n", odp_timer_pool_to_u64(tp)); LOG_DBG("#timers..: %u\n", NTIMERS); LOG_DBG("Tmo range: %u ms (%" PRIu64 " ticks)\n", RANGE_MS,
Signed-off-by: Brian Brooks <brian.brooks@linaro.org> --- include/odp/api/spec/timer.h | 9 +++++++++ platform/linux-generic/odp_timer.c | 5 +++++ test/common_plat/validation/api/timer/timer.c | 2 ++ 3 files changed, 16 insertions(+) -- 2.10.1