Re: [PATCH v2] staging/rdma/hfi1: set Gen3 half-swing for integrated devices

["ira.weiny" <ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html