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.bro...@linaro.org>
> wrote:
>
> > Signed-off-by: Brian Brooks <brian.bro...@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
> >
> >
