Hi Gabe,
> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carri...@intel.com>
> Sent: Thursday, August 11, 2022 1:25 AM
> To: Naga Harish K, S V <s.v.naga.haris...@intel.com>; jer...@marvell.com
> Cc: pbhagavat...@marvell.com; sthot...@marvell.com; dev@dpdk.org
> Subject: RE: [PATCH v2 1/4] eventdev/timer: add periodic event timer
> support
>
> Hi Harish,
>
> > -----Original Message-----
> > From: Naga Harish K, S V <s.v.naga.haris...@intel.com>
> > Sent: Wednesday, August 10, 2022 2:07 AM
> > To: Carrillo, Erik G <erik.g.carri...@intel.com>; jer...@marvell.com
> > Cc: pbhagavat...@marvell.com; sthot...@marvell.com; dev@dpdk.org
> > Subject: [PATCH v2 1/4] eventdev/timer: add periodic event timer
> > support
> >
> > This patch adds support to configure and use periodic event timers in
> > software timer adapter.
> >
> > The structure ``rte_event_timer_adapter_stats`` is extended by adding
> > a new field, ``evtim_drop_count``. This stat represents the number of
> > times an event_timer expiry event is dropped by the event timer adapter.
> >
> > Signed-off-by: Naga Harish K S V <s.v.naga.haris...@intel.com>
> > ---
> > lib/eventdev/rte_event_timer_adapter.c | 86 ++++++++++++++++++-----
> --
> > - lib/eventdev/rte_event_timer_adapter.h | 2 +
> > lib/eventdev/rte_eventdev.c | 6 +-
> > 3 files changed, 67 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_event_timer_adapter.c
> > b/lib/eventdev/rte_event_timer_adapter.c
> > index e0d978d641..0de88dfc0f 100644
> > --- a/lib/eventdev/rte_event_timer_adapter.c
> > +++ b/lib/eventdev/rte_event_timer_adapter.c
> > @@ -53,6 +53,14 @@ static const struct event_timer_adapter_ops
> > swtim_ops; #define EVTIM_SVC_LOG_DBG(...) (void)0 #endif
> >
> > +static inline enum rte_timer_type
> > +get_event_timer_type(const struct rte_event_timer_adapter *adapter) {
>
> Let's call this function "get_timer_type" since it is selecting a type for an
> rte_timer.
>
Taken in v3 version of the patch
> > + return (adapter->data->conf.flags &
> > + RTE_EVENT_TIMER_ADAPTER_F_PERIODIC) ?
> > + PERIODICAL : SINGLE;
> > +}
> > +
> > static int
> > default_port_conf_cb(uint16_t id, uint8_t event_dev_id, uint8_t
> > *event_port_id,
> > void *conf_arg)
> > @@ -195,10 +203,11 @@ rte_event_timer_adapter_create_ext(
> > adapter->data->conf = *conf; /* copy conf structure */
> >
> > /* Query eventdev PMD for timer adapter capabilities and ops */
> > - ret = dev->dev_ops->timer_adapter_caps_get(dev,
> > + ret = dev->dev_ops->timer_adapter_caps_get ?
> > + dev->dev_ops-
> > >timer_adapter_caps_get(dev,
> > adapter->data->conf.flags,
> > &adapter->data->caps,
> > - &adapter->ops);
> > + &adapter->ops) : 0;
> > if (ret < 0) {
> > rte_errno = -ret;
> > goto free_memzone;
>
> IMO, this hunk would read better as:
>
> if (dev->dev_ops->timer_adapter_caps_get) {
> ret = dev->dev_ops->timer_adapter_caps_get(dev,
> adapter->data->conf.flags,
> &adapter->data->caps,
> &adapter->ops);
> if (ret < 0) {
> rte_errno = -ret;
> goto free_memzone;
> }
> }
>
Taken in v3 version of the patch
> > @@ -348,10 +357,11 @@ rte_event_timer_adapter_lookup(uint16_t
> > adapter_id)
> > dev = &rte_eventdevs[adapter->data->event_dev_id];
> >
> > /* Query eventdev PMD for timer adapter capabilities and ops */
> > - ret = dev->dev_ops->timer_adapter_caps_get(dev,
> > + ret = dev->dev_ops->timer_adapter_caps_get ?
> > + dev->dev_ops->timer_adapter_caps_get(dev,
> > adapter->data->conf.flags,
> > &adapter->data->caps,
> > - &adapter->ops);
> > + &adapter->ops) : 0;
> > if (ret < 0) {
> > rte_errno = EINVAL;
> > return NULL;
>
> Same comment as above for this hunk...
>
> Thanks,
> Erik
