Re: [dpdk-dev] [PATCH v3 1/2] ethdev: add callback to get register size in bytes

[Zyta Szpak <zr@semihalf.com>]

Hi,
please see inline

2016-06-08 10:53 GMT+02:00 Thomas Monjalon <thomas.monjalon@6wind.com>:

> Hi Zyta,
>
> 2016-06-01 09:56, zr@semihalf.com:
> > rte_eth_dev_get_reg_length and rte_eth_dev_get_reg callbacks
> > do not provide register size to the app in any way. It is
> > needed to allocate proper number of bytes before retrieving
> > registers content with rte_eth_dev_get_reg.
>
> Yes, register size is needed.
> And I think it makes sense to register it in the struct rte_dev_reg_info.
> We already have a length field, so we could just add a width field.
>
That was my first thought to add reg_size to reg_info struct but
get_reg_length doesn' take reg_info as parameter so it would require
modification of this callback as well. This would interfere with the
author's vision. I think that adding a new one is clear and readable.

>
> > @@ -1455,6 +1458,8 @@ struct eth_dev_ops {
> >
> >       eth_get_reg_length_t get_reg_length;
> >       /**< Get # of registers */
> > +     eth_get_reg_width_t get_reg_width;
> > +     /**< Get # of bytes in register */
> >       eth_get_reg_t get_reg;
> >       /**< Get registers */
>
> I am not sure it is a good practice to add a new function for each
> parameter of a request.
> I would prefer having only one function rte_eth_dev_get_regs()
> which returns length and width if data is NULL.
> The first call is a parameter request before buffer allocation,
> and the second call fills the buffer.
>
> We can deprecate the old API and introduce this new one.
>
> Opinions?
>

In my opinion as it is now it works fine. Gathering all parameters in one
callback might be a good idea if the maintainer also agrees to that because
as I mentioned, it interferes.
Any other opinions?\

Best regards,
Zyta Szpak