Fwd: Re: [PATCH v8 07/22] IB/hns: Add event queue support

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2832 bytes --]




-------- 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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Organization: Red Hat, Inc.
To: Lijun Ou <oulijun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

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-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD




-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]