On Fri, Nov 10, 2017 at 10:25:37AM +0200, Tero Kristo wrote:

TI Keystone and DRA7xx SoCs have support for EDAC on DDR3 memory that can
correct one bit errors and detect two bit errors. Add EDAC driver for this
feature which plugs into the generic kernel EDAC framework.

...

+static int ti_edac_probe(struct platform_device *pdev)
+{
+	int error_irq = 0, ret = -ENODEV;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+	void __iomem *reg;
+	struct mem_ctl_info *mci;
+	struct edac_mc_layer layers[1];
+	const struct of_device_id *id;
+	struct ti_edac *edac;
+	int my_id;
+
+	id = of_match_device(ti_edac_of_match, &pdev->dev);
+	if (!id)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	reg = devm_ioremap_resource(dev, res);
+	if (IS_ERR(reg)) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF controller regs not defined\n");
+		return PTR_ERR(reg);
+	}
+
+	layers[0].type = EDAC_MC_LAYER_ALL_MEM;
+	layers[0].size = 1;
+
+	/* Allocate ID number for our EMIF controller */
+	my_id = atomic_inc_return(&edac_id) - 1;

You can do ATOMIC_INIT(-1) and then you don't need that silly - 1.

But there's something else wrong with this ID generation:

+
+	mci = edac_mc_alloc(my_id, 1, layers, sizeof(*edac));
+	if (!mci)
+		return -ENOMEM;

<--- if you return here due to error, the next one which succeeds will
get the next number and you'd have a hole in the numbering and wonder
where did instance 0 go.

So I'm wondering if instead you could derive the ID from some controller
registers which are specific to the instance. Or some hardware detail
which is instance-specific.

In my driver, for example, the 0th instance has PCI device functions
00:18.x which is node 0, then node 1 has 00:19.x and so on, and this is
a stable numbering. If you could do that for your hw you'll be able to
pinpoint which instance and which DIMMs we're talking about reliably.

+
+	mci->pdev = &pdev->dev;
+	edac = mci->pvt_info;
+	edac->reg = reg;
+	platform_set_drvdata(pdev, mci);
+
+	mci->mtype_cap = MEM_FLAG_DDR3 | MEM_FLAG_DDR2;
+	mci->edac_ctl_cap = EDAC_FLAG_SECDED | EDAC_FLAG_NONE;
+	mci->mod_name = EDAC_MOD_NAME;
+	mci->ctl_name = id->compatible;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	/* Setup memory layout */
+	ti_edac_setup_dimm(mci, (u32)(id->data));
+
+	ret = edac_mc_add_mc(mci);

Do that...

+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "Failed to register mci: %d.\n", ret);
+		goto err;
+	}
+
+	/* add EMIF ECC error handler */
+	error_irq = platform_get_irq(pdev, 0);
+	if (!error_irq) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "EMIF irq number not defined.\n");
+		goto err2;
+	}
+
+	ret = devm_request_irq(dev, error_irq, ti_edac_isr, 0,
+			       "emif-edac-irq", mci);
+	if (ret) {
+		edac_printk(KERN_ERR, EDAC_MOD_NAME,
+			    "request_irq fail for EMIF EDAC irq\n");
+		goto err2;
+	}

... here, after you've requested IRQs successfully and you can save
yourself the err2 error path.

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.