Message-ID: <c6b60b30-5a9f-134b-b1fb-b84704a3ccb2@ieee.org>
Date:   Wed, 5 Jan 2022 18:23:52 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.1
From:   Alex Elder <elder@ieee.org>
Subject: Re: [PATCH 04/20] bus: mhi: Cleanup the register definitions used in
 headers
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        mhi@lists.linux.dev
Cc:     hemantk@codeaurora.org, bbhatt@codeaurora.org,
        quic_jhugo@quicinc.com, vinod.koul@linaro.org,
        bjorn.andersson@linaro.org, dmitry.baryshkov@linaro.org,
        skananth@codeaurora.org, vpernami@codeaurora.org,
        vbadigan@codeaurora.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211202113553.238011-1-manivannan.sadhasivam@linaro.org>
 <20211202113553.238011-5-manivannan.sadhasivam@linaro.org>
Content-Language: en-US
In-Reply-To: <20211202113553.238011-5-manivannan.sadhasivam@linaro.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> Cleanup includes:
> 
> 1. Moving the MHI register bit definitions to common.h header (only the
>     register offsets differ between host and ep not the bit definitions)

The register offsets do differ, but the group of registers for the host
differs from the group of registers for the endpoint by a fixed amount.
(MHIREGLEN = 0x0000 for host, or 0x100 for endpoint; CRCBAP_LOWER is
0x0068 for host, 0x0168 for endpoint.)

In other words, can you instead use the same symbolic offsets, but
have the endpoint add 0x0100 to them all?  It would make the fact
that they're both referencing the same basic in-memory structure
more obvious.

> 2. Using the GENMASK macro for masks
> 3. Removing brackets for single values
> 4. Using lowercase for hex values

Yay!!! For all three of the above.

More below.

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   drivers/bus/mhi/common.h        | 129 ++++++++++++---
>   drivers/bus/mhi/host/internal.h | 282 +++++++++++---------------------
>   2 files changed, 207 insertions(+), 204 deletions(-)
> 
> diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> index 2ea438205617..c1272d61e54e 100644
> --- a/drivers/bus/mhi/common.h
> +++ b/drivers/bus/mhi/common.h
> @@ -9,32 +9,123 @@
>   
>   #include <linux/mhi.h>
>   
> +/* MHI register bits */
> +#define MHIREGLEN_MHIREGLEN_MASK		GENMASK(31, 0)
> +#define MHIREGLEN_MHIREGLEN_SHIFT		0

Again, please eliminate all _SHIFT definitions where they define
the low bit position of a mask.

Maybe you can add some underscores for readability?

Even if you don't do that, you could add a comment here or there to
explain what certain abbreviations stand for, to make it easier to
understand.  E.g., CHDB = channel doorbell, CCA = channel context
array, BAP = base address pointer.

					-Alex


> +#define MHIVER_MHIVER_MASK			GENMASK(31, 0)
> +#define MHIVER_MHIVER_SHIFT			0
> +
> +#define MHICFG_NHWER_MASK			GENMASK(31, 24)
> +#define MHICFG_NHWER_SHIFT			24
> +#define MHICFG_NER_MASK				GENMASK(23, 16)
> +#define MHICFG_NER_SHIFT			16
> +#define MHICFG_NHWCH_MASK			GENMASK(15, 8)
> +#define MHICFG_NHWCH_SHIFT			8
> +#define MHICFG_NCH_MASK				GENMASK(7, 0)
> +#define MHICFG_NCH_SHIFT			0

. . .