Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v9 2/2] EDAC: add EDAC driver for DMC520
To:     Borislav Petkov <bp@alien8.de>
Cc:     James Morse <james.morse@arm.com>, robh+dt@kernel.org,
        mark.rutland@arm.com, devicetree@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>, linux-edac@vger.kernel.org,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        sashal@kernel.org, hangl@microsoft.com,
        Lei Wang <lewan@microsoft.com>, shji@microsoft.com,
        ruizhao@microsoft.com, Scott Branden <scott.branden@broadcom.com>,
        Yuqing Shen <yuqing.shen@broadcom.com>, ray.jui@broadcom.com,
        wangglei@gmail.com
References: <6a462190-0af2-094a-daa8-f480d54a1fbf@gmail.com>
 <20200117001843.GJ27148@zn.tnic>
From:   Shiping Ji <shiping.linux@gmail.com>
Message-ID: <d5989a4c-8173-2f03-7d20-6fdd32d19591@gmail.com>
Date:   Fri, 17 Jan 2020 10:31:18 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.3.1
MIME-Version: 1.0
In-Reply-To: <20200117001843.GJ27148@zn.tnic>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 1/16/2020 4:18 PM, Borislav Petkov wrote:

>> +/* The EDAC driver private data */
>> +struct dmc520_edac {
>> +	void __iomem *reg_base;
>> +	spinlock_t ecc_lock;
> 
> What does that spinlock protect? Also, its name is not very optimal.

This is to protect concurrent writes to the mci->error_desc as suggested by James when reviewing the patch v3.
 
>> +	reg_offset_low = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_31_00 :
>> +				 REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_31_00;
>> +	reg_offset_high = is_ce ? REG_OFFSET_DRAM_ECC_ERRC_INT_INFO_63_32 :
>> +				  REG_OFFSET_DRAM_ECC_ERRD_INT_INFO_63_32;
> 
> Those define names could be shorter.

I'm trying to find a good scheme to make them shorter, at the moment they are named according to the TRM.
 
>> +		if (irq >= 0) {
>> +			ret = devm_request_irq(&pdev->dev, irq,
>> +							dmc520_isr, IRQF_SHARED,
>> +							dev_name(&pdev->dev), mci);
> 
> Align arguments on the opening brace.

I'm not sure how this can be done perfectly with tabs only :)

All other comments have been addressed in the next patch, many thanks!

-- 
Best regards,
Shiping Ji