-------- Forwarded Message --------
Subject: Re: [PATCH v8 07/22] IB/hns: Add event queue support
Date: Wed, 25 May 2016 15:22:05 -0400
From: Doug Ledford <dledford@xxxxxxxxxx>
Organization: Red Hat, Inc.
To: Lijun Ou <oulijun@xxxxxxxxxx>
On 05/25/2016 11:05 AM, Lijun Ou wrote:
> This patch added event queue support for RoCE driver. It is used
> for RoCE interrupt. RoCE includes 32 synchronous event irqs, 1
> asynchronous event irq and 1 common overflow irq.
> +#define roce_writel(value, addr) writel((value), (addr))
> +#define roce_readl(addr) readl((addr))
These two defines are useless. They just make the code width longer for
no real purpose. Here's an example from my last email
>> + ((u64)le32_to_cpu(roce_readl(hr_dev->reg_base +
>> + ROCEE_SYS_IMAGE_GUID_H_REG)) <<
>> + ADDR_SHIFT_32);
If you are going to do a #define for this, at least let the #define do
some work for you. Something like:
#define hns_readl(dev, reg) readl((dev)->reg_base + (reg))
then the above can read more like this
>> + ((u64)le32_to_cpu(hns_readl(hr_dev,
>> + ROCEE_SYS_IMAGE_GUID_H_REG)) <<
>> + ADDR_SHIFT_32);
You could do additional tricks to make the overall length of lines
shorter while loosing none of the readability if you altered your
register names a little. And if all of your ROCEE registers have a
common prefix or postfix, you can embedd that in the #define to keep the
register names shorter in your file. For instance, they appear to all
start with ROCEE and end with REG, so maybe something like this:
#define hns_readl(dev, reg) readl((dev->)reg_base + ROCEE_##reg##_REG)
so that the above starts to look like this:
>> + ((u64)le32_to_cpu(hns_readl(hr_dev,
>> + SYS_IMAGE_GUID_H)) << 32);
which is much more readable in my opinion.
> +/* Address shift 12bit with the special hardware address operation of RoCEE */
> +#define ADDR_SHIFT_12 12
> +
And this is the one that should talk about the register that requires
the address be shifted as though using a 4K page size even when you aren't.
> /* Address shift 32bit with the special hardware address operation of RoCEE */
> #define ADDR_SHIFT_32 32
I really don't know when you actually need a define for a 32 bit shift....
> +/* Address shift 44bit with the special hardware address operation of RoCEE */
> +#define ADDR_SHIFT_44 44
This one, just like the 12 shift, needs an explanation of some sort.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: 0E572FDD
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: 0E572FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
