Re: [PATCH v9 2/2] EDAC: add EDAC driver for DMC520

[Shiping Ji]

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