Mykyta Iziumtsev(MykytaI) replied on github web page: platform/linux-generic/pktio/mdev/e1000e.c line 9 @@ -0,0 +1,605 @@ +/* Copyright (c) 2018, Linaro Limited + * All rights reserved. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +#include "config.h" + +#ifdef ODP_MDEV
Comment: Looking at include/config.h.in (where ODP_MDEV comes from) it's not the only naming violation. There are at least 7 other pktio (internal) macros which don't start with underscore. Please confirm that we really need to add underscore for ODP_MDEV and I'll update next patch version with that ... > Bill Fischofer(Bill-Fischofer-Linaro) wrote: > @muvarov In this case the variable is being declared at the beginning of > scope. The scope in this instance is the block defined by the `for` loop. > That citation is saying you shouldn't declare variables in the middle of a > scope. While that's still correct C, it can be confusing. >> muvarov wrote >> ./doc/application-api-guide/api_guide_lines.dox line 162 >>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>> @muvarov This idiom is part of C99, so I see no problem with it here. It's >>> also why checkpatch doesn't complain. >>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>> We have the various `odpdrv_mmio_uxx_read/write()` APIs. Should we also >>>> have `_addr` variants that would similarly enable address loading to >>>> handle this sort of awkward casting? E.g.: >>>> ``` >>>> rxq->doorbell = odpdrv_u32le_addr(pkt_e100e->mmio + E1000_RDT_OFFSET); >>>> ``` >>>> Similar comments for other address setups contained here. >>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>> Might be good to document where the various "magic numbers" come from. >>>>> Presumably from some hardware manual? Does Intel provide a `.h` file that >>>>> we can reference/`#include` for these rather than duplicate them here? >>>>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote: >>>>>> Agree with @muvarov. `_ODP_MDEV` is fine here. The `ODP` prefix should >>>>>> be reserved for things that are part of the external ODP API. >>>>>>> muvarov wrote >>>>>>> place type out of for(). checkpatch should warn about that. >>>>>>>> muvarov wrote >>>>>>>> please keep ODP_ and odp_ prefix only for odp api. Internal things >>>>>>>> should be named as _ODP_ or without that prefix. This comment not just >>>>>>>> for this commit but it has to be corrected. https://github.com/Linaro/odp/pull/408#discussion_r163485705 updated_at 2018-01-24 08:59:06
