Hi, please see inline 2016-06-08 10:53 GMT+02:00 Thomas Monjalon <thomas.monjalon at 6wind.com>:
> Hi Zyta, > > 2016-06-01 09:56, zr at 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