[PATCH v6 3/3] EDAC: nuvoton: Add NPCM memory controller driver

Paul Menzel pmenzel at molgen.mpg.de
Thu Apr 14 18:56:43 AEST 2022


Dear Borislav,


Am 14.04.22 um 10:42 schrieb Borislav Petkov:
> On Thu, Apr 14, 2022 at 09:55:05AM +0800, Medad Young wrote:
>>>> +                             if (mtype == MEM_TYPE_DDR4)
>>>> +                                     dimm->mtype = MEM_DDR4;
>>>> +                             else
>>>> +                                     dimm->mtype = MEM_EMPTY;
>>>
>>> Use ternary operator?
>>>
>>>       dimm-mtype = (mtype == MEM_TYPE_DDR4) ? MEM_DDR4 : MEM_EMPTY;
> 
> Ternary operator is less readable than a plain and simple if-else.
> 
>>>> +{
>>>> +     struct priv_data *priv = mci->pvt_info;
>>>> +     const struct npcm_edac_platform_data *npcm_chip = priv->npcm_chip;
>>>> +     u64 err_c_addr = 0x0;
>>>
>>> size_t
>>
>> OK
> 
> Why is size_t? error address doesn't have anything to do with a
> sizeof(), array indexing or loop counting.
> 
> It is an error address and having it in an u64 tells you exactly what
> its quantity is.

Good point. Sorry for missing that.

> So can we stop the silliness pls?

No idea, why you had to ask this question, while you statement before 
already made the point.

>>>> +static irqreturn_t edac_ecc_isr(int irq, void *dev_id)
>>>> +{
>>>> +     struct mem_ctl_info *mci = dev_id;
>>>> +     struct priv_data *priv = mci->pvt_info;
>>>> +     const struct npcm_edac_platform_data *npcm_chip = priv->npcm_chip;
>>>> +     u32 intr_status;
>>>> +     u32 val;
>>>> +
>>>> +     /* Check the intr status and confirm ECC error intr */
>>>> +     intr_status = readl(priv->reg + npcm_chip->ecc_ctl_int_status);
>>>> +
>>>> +     edac_dbg(3, "InterruptStatus : 0x%x\n", intr_status);
>>>
>>> Remove the space before the colon? Maybe use:
>>>
>>> "Interrupt status (intr_status): 0x%x\n"
> 
> And repeat "interrupt status"? Also silly. The question to ask
> yourselves should always be: is this error message helpful enough to its
> intended recipients.
> 
> When I see
> 
>    "Interrupt status (intr_status): 0x%x\n"
> 
> in my code, I go: "hm, where does this message come from?" because it
> ain't helpful enough. So I have to go stare at the code too.
> 
> I hope you're catching my drift.

Sorry I do not get your point. Would you elaborate on the debug message 
so it’s more useful? Or would you keep `InterruptStatus`, or – as it’s a 
debug message – use the variable name? My point was mainly about, why 
not use the variable name directly in the debug message, and the space 
before the colon.


Kind regards,

Paul


More information about the openbmc mailing list