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

From: Paul Menzel
Date: Thu Apr 14 2022 - 04:56:53 EST


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