On Tue, Nov 10, 2015 at 12:59:29PM +0300, Dan Carpenter wrote:
> Gar... No. Please please get rid of the PC() macro. It makes the code
> impossible to understand because instead of hitting CTRL-[ you have
> decode it and then manually type out
>
> :cs find g CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT
>
> which is the length of a typical college essay. I meant just put a
> comment like this:
>
> /*
> * In the hardware spec these are prefixed with:
> * CCE_PCIE_CTRL_...
> * But it is too long to use in code.
> */
> #define XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK 0x1ull
>
> Or probably even better:
>
> #define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
> #define LANE_BUNDLE_MASK 0x3ull /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK */
> #define LANE_BUNDLE_SHIFT 0 /* CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT
> */
> #define LANE_DELAY_MASK 0xFull /*
> CCE_PCIE_CTRL_PCIE_LANE_DELAY_MASK */
> #define LANE_DELAY_SHIFT 2 /* CCE_PCIE_CTRL_PCIE_LANE_DELAY_SHIFT */
> #define MARGIN_OVERWRITE_SHIFT 8 /*
> CCE_PCIE_CTRL_XMT_MARGIN_OVERWRITE_ENABLE_SHIFT */
> #define MARGIN_SHIFT 9 /* CCE_PCIE_CTRL_XMT_MARGIN_SHIFT */
> #define MARGIN_G1G2_OVERWRITE_MASK 0x1ull /*
> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_MASK */
> #define MARGIN_G1G2_OVERWRITE_SHIFT 12 /*
> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_OVERWRITE_ENABLE_SHIFT */
> #define MARGIN_G1G2_MASK 0x7ull /*
> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK */
> #define MARGIN_G1G2_SHIFT 13 /*
> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT */
>
> Those lines go over the 80 character limit but it's fine.
My apologies for not understanding what you meant. I took your meaning to be
that we had to honor the checkpatch checks so while the PC macro was
undesirable it was ok if I just made some comments...
FWIW I don't like the PC macro either. But we have a tool which is generating
these names to be identical to the hardware spec. And we really want to
preserve those as a reference back to the spec. Creating additional names which
are in the code is a bit cumbersome but what if we do something like this:
<auto generated from spec>
...
#define CCE_PCIE_CTRL (CCE + 0x0000000000C0)
#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK 0x3ull
#define CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT 0
...
<Defined for use in the code>
#define LANE_BUNDLE_MASK CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_MASK
#define LANE_BUNDLE_SHIFT CCE_PCIE_CTRL_PCIE_LANE_BUNDLE_SHIFT
...
?????
An alternative would be to define some helper functions such as:
static inline u64 extract_xmt_margin_g1g2(u64 reg)
{
return (reg >> CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_SHIFT)
& CCE_PCIE_CTRL_XMT_MARGIN_GEN1_GEN2_MASK;
}
...
...
xmt_margin = extract_xmt_margin_g1g2(pcie_ctrl);
...
I prefer the second option as it preserves the register names right in the
code. So you can reference the hardware spec without looking anything up in a
header file.
I again apologize for misunderstanding your previous meaning.
Ira
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
