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
