On Wed, 30 Jul 2014, tthayer at opensource.altera.com wrote:

This should be in a separate patch.

Can you use the shorter version of the licence?

+ */

'\n' here.

+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mfd/altera-sdr.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+static const struct mfd_cell altera_sdr_devs[] = {
+#if defined(CONFIG_EDAC_ALTERA_MC)

No need to do this, as it will only be matched if the driver is
enabled.  Please remove the #iffery.

+	{
+		.name = "altr_sdram_edac",
+		.of_compatible = "altr,sdram-edac",

What other devices will there be?

+	},
+#endif
+};
+
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+	return readl(sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+	writel(value, sdr->reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);

Why are you abstracting these?

Might be better to use Regmap even.

+/* Get total memory size in bytes */
+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+	u32 size;
+	u32 read_reg, row, bank, col, cs, width;

Weird that size is on its own.  Either place on a single line or
separate them all.

+	read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+	if (read_reg < 0)
+		return 0;
+
+	width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+	if (width < 0)
+		return 0;
+
+	col = (read_reg & SDR_DRAMADDRW_COLBITS_MASK) >>
+		SDR_DRAMADDRW_COLBITS_LSB;
+	row = (read_reg & SDR_DRAMADDRW_ROWBITS_MASK) >>
+		SDR_DRAMADDRW_ROWBITS_LSB;
+	bank = (read_reg & SDR_DRAMADDRW_BANKBITS_MASK) >>
+		SDR_DRAMADDRW_BANKBITS_LSB;
+	cs = (read_reg & SDR_DRAMADDRW_CSBITS_MASK) >>
+		SDR_DRAMADDRW_CSBITS_LSB;

These would probably be better as macros.

+	/* Correct for ECC as its not addressible */
+	if (width == SDR_DRAMIFWIDTH_32B_ECC)
+		width = 32;
+	if (width == SDR_DRAMIFWIDTH_16B_ECC)
+		width = 16;
+
+	/* calculate the SDRAM size base on this info */
+	size = 1 << (row + bank + col);
+	size = size * cs * (width / 8);
+	return size;
+}
+EXPORT_SYMBOL_GPL(altera_sdr_mem_size);

Should this really be done in here?  Isn't this an SDRAM function?

+static int altera_sdr_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct altera_sdr *sdr;
+	struct resource *res;
+	void __iomem *base;
+	int ret;
+
+	sdr = devm_kzalloc(dev, sizeof(*sdr), GFP_KERNEL);
+	if (!sdr)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -ENOENT;
+
+	base = devm_ioremap(dev, res->start, resource_size(res));

Instead use devm_ioremap_resource(), then you can omit the error
checking of platform_get_resource().

+	if (!base)
+		return -ENOMEM;
+
+	sdr->dev = &pdev->dev;

Either use 'dev' here, or remove the top line in this function and use
&pdev->dev everywhere.  I personally prefer the latter.

+	sdr->reg_base = base;
+
+	ret = mfd_add_devices(sdr->dev, 0, altera_sdr_devs,
+			      ARRAY_SIZE(altera_sdr_devs), NULL, 0, NULL);
+	if (ret)
+		dev_err(sdr->dev, "error adding devices");
+
+	platform_set_drvdata(pdev, sdr);
+
+	dev_dbg(dev, "Altera SDR MFD registered\n");
+
+	return 0;
+}
+
+static int altera_sdr_remove(struct platform_device *pdev)
+{
+	struct altera_sdr *sdr = platform_get_drvdata(pdev);

No need for this, just use &pdev->dev.

+	mfd_remove_devices(sdr->dev);
+
+	return 0;
+}
+
+static const struct of_device_id of_altera_sdr_match[] = {
+	{ .compatible = "altr,sdr", },
+	{ },
+};
+
+static const struct platform_device_id altera_sdr_ids[] = {
+	{ "altera_sdr", },
+	{ }
+};

What's this for?

+static struct platform_driver altera_sdr_driver = {
+	.driver = {
+		.name = "altera_sdr",
+		.owner = THIS_MODULE,

You can remove this line, it's taken care of for you.

+		.of_match_table = of_altera_sdr_match,
+	},
+	.probe		= altera_sdr_probe,
+	.remove		= altera_sdr_remove,
+	.id_table	= altera_sdr_ids,
+};
+
+static int __init altera_sdr_init(void)
+{
+	return platform_driver_register(&altera_sdr_driver);
+}
+postcore_initcall(altera_sdr_init);

Why was this chosen?

Use the short version.

+ */

'\n' here.

+#ifndef __LINUX_MFD_ALTERA_SDR_H
+#define __LINUX_MFD_ALTERA_SDR_H
+
+/* SDRAM Controller register offsets */
+#define SDR_CTLCFG_OFST                 0x00
+#define SDR_DRAMADDRW_OFST		0x2C
+#define SDR_DRAMIFWIDTH_OFST		0x30
+#define SDR_DRAMSTS_OFST                0x38
+#define SDR_DRAMINTR_OFST               0x3C
+#define SDR_SBECOUNT_OFST               0x40
+#define SDR_DBECOUNT_OFST               0x44
+#define SDR_ERRADDR_OFST                0x48
+#define SDR_DROPCOUNT_OFST              0x4C
+#define SDR_DROPADDR_OFST               0x50
+#define SDR_CTRLGRP_LOWPWREQ_OFST       0x54
+#define SDR_CTRLGRP_LOWPWRACK_OFST      0x58
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define SDR_CTLCFG_ECC_EN               0x400
+#define SDR_CTLCFG_ECC_CORR_EN          0x800
+#define SDR_CTLCFG_GEN_SB_ERR           0x2000
+#define SDR_CTLCFG_GEN_DB_ERR           0x4000
+
+#define SDR_CTLCFG_ECC_AUTO_EN          (SDR_CTLCFG_ECC_EN | \
+					 SDR_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller Address Widths Field Register */
+#define SDR_DRAMADDRW_COLBITS_MASK      0x001F
+#define SDR_DRAMADDRW_COLBITS_LSB       0
+#define SDR_DRAMADDRW_ROWBITS_MASK      0x03E0
+#define SDR_DRAMADDRW_ROWBITS_LSB       5
+#define SDR_DRAMADDRW_BANKBITS_MASK     0x1C00
+#define SDR_DRAMADDRW_BANKBITS_LSB      10
+#define SDR_DRAMADDRW_CSBITS_MASK       0xE000
+#define SDR_DRAMADDRW_CSBITS_LSB        13

We normally call _LSB, _SHIFT.

+/* SDRAM Controller Interface Data Width Defines */
+#define SDR_DRAMIFWIDTH_16B_ECC         24
+#define SDR_DRAMIFWIDTH_32B_ECC         40
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define SDR_DRAMSTS_SBEERR              0x04
+#define SDR_DRAMSTS_DBEERR              0x08
+#define SDR_DRAMSTS_CORR_DROP           0x10
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define SDR_DRAMINTR_INTREN             0x01
+#define SDR_DRAMINTR_SBEMASK            0x02
+#define SDR_DRAMINTR_DBEMASK            0x04
+#define SDR_DRAMINTR_CORRDROPMASK       0x08
+#define SDR_DRAMINTR_INTRCLR            0x10
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define SDR_SBECOUNT_COUNT_MASK         0x0F
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define SDR_DBECOUNT_COUNT_MASK         0x0F
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define SDR_ERRADDR_ADDR_MASK           0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define SDR_DROPCOUNT_CORRMASK          0x0F
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define SDR_DROPADDR_ADDR_MASK          0xFFFFFFFF
+
+#define SELFRSHREQ_POS                  3
+#define SELFRSHREQ_MASK                 0x8
+
+#define SELFRFSHMASK_POS                4
+#define SELFRFSHMASK_MASK               0x30
+
+#define SELFRFSHACK_POS                 1
+#define SELFRFSHACK_MASK                0x2
+
+struct altera_sdr {
+	struct device *dev;
+	void __iomem *reg_base;
+};
+
+/* Register access API */
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset);
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value);

Regmap?

+u32 altera_sdr_mem_size(struct altera_sdr *sdr);
+
+#endif /* __LINUX_MFD_ALTERA_SDR_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog