[PATCH v4 10/21] mtd: support BB SRAM on ICP DAS LP-8x4x

[PATCH v4 10/21] mtd: support BB SRAM on ICP DAS LP-8x4x

[Sergei Ianovich]

This provides an MTD device driver for 512kB of battery backed up SRAM
on ICPDAS LP-8X4X programmable automation controllers.

SRAM chip is connected via FPGA and is not accessible without a driver,
unlike flash memory which is wired to CPU MMU.

This SRAM becomes an excellent persisent storage of volatile process
data like counter values and sensor statuses. Storing those data in
flash or mmc card is not a viable solution.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Brian Norris <computersforpeace@gmail.com>
---
   v3..v4 for Brian Norris 'Reviewed-by'
   * add doc file for DT binding
   * move DTS binding to a different patch (8/21)
   * drop unused include directive
   * drop safely unused callback
   * drop non-default partion probe types
   * drop duplicate error checks
   * drop duplicate error reporting
   * fixed error message on MTD registeration
   * fixed module removal routine

   v2..v3
   * no changes (except number 08/16 -> 10/21)

   v0..v2
   * use device tree
   * use devm helpers where possible

 .../devicetree/bindings/mtd/sram-lp8x4x.txt        |  22 +++
 arch/arm/configs/lp8x4x_defconfig                  |   1 +
 drivers/mtd/devices/Kconfig                        |  14 ++
 drivers/mtd/devices/Makefile                       |   1 +
 drivers/mtd/devices/sram_lp8x4x.c                  | 204 +++++++++++++++++++++
 5 files changed, 242 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
 create mode 100644 drivers/mtd/devices/sram_lp8x4x.c

diff --git a/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
new file mode 100644
index 0000000..8b1e864
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
@@ -0,0 +1,22 @@
+512kB battery backed up SRAM on LP-8x4x industrial computers
+
+Required properties:
+- compatible : should be "icpdas,sram-lp8x4x"
+
+- reg: physical base addresses and region lengths of
+       * IO memory range
+       * SRAM page selector
+
+- eeprom-gpios : should point to active-low write enable GPIO
+
+SRAM chip is connected via FPGA and is not accessible without a driver,
+unlike flash memory which is wired to CPU MMU. Driver is essentially
+an address translation routine.
+
+Example:
+
+	sram@a000 {
+		compatible = "icpdas,sram-lp8x4x";
+		reg = <0xa000 0x1000
+		       0x901e 0x1>;
+	};
diff --git a/arch/arm/configs/lp8x4x_defconfig b/arch/arm/configs/lp8x4x_defconfig
index d60e37a..17a4e6f 100644
--- a/arch/arm/configs/lp8x4x_defconfig
+++ b/arch/arm/configs/lp8x4x_defconfig
@@ -57,6 +57,7 @@ CONFIG_MTD_CFI_ADV_OPTIONS=y
 CONFIG_MTD_CFI_GEOMETRY=y
 CONFIG_MTD_CFI_INTELEXT=y
 CONFIG_MTD_PHYSMAP_OF=y
+CONFIG_MTD_SRAM_LP8X4X=y
 CONFIG_PROC_DEVICETREE=y
 CONFIG_BLK_DEV_LOOP=y
 CONFIG_BLK_DEV_LOOP_MIN_COUNT=2
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 1210bc2..fc8552b 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -225,4 +225,18 @@ config BCH_CONST_T
 	default 4
 endif
 
+config MTD_SRAM_LP8X4X
+	tristate "SRAM on ICPDAS LP-8X4X"
+	depends on OF && ARCH_PXA
+       ---help---
+	 This provides an MTD device driver for 512kiB of battery backed up SRAM
+	 on ICPDAS LP-8X4X programmable automation controllers.
+
+	 SRAM chip is connected via FPGA and is not accessible without a driver,
+	 unlike flash memory which is wired to CPU MMU.
+
+	 Say N, unless you plan to run this kernel on LP-8X4X.
+
+	 If you say M, the module will be called sram_lp8x4x.
+
 endmenu
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index c68868f..a7d86e2 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
+obj-$(CONFIG_MTD_SRAM_LP8X4X)	+= sram_lp8x4x.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/sram_lp8x4x.c b/drivers/mtd/devices/sram_lp8x4x.c
new file mode 100644
index 0000000..4cfa70b
--- /dev/null
+++ b/drivers/mtd/devices/sram_lp8x4x.c
@@ -0,0 +1,204 @@
+/*
+ *  linux/drivers/mtd/devices/lp8x4x_sram.c
+ *
+ *  MTD Driver for SRAM on ICPDAS LP-8x4x
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation or any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/map.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>
+#include <linux/types.h>
+
+struct lp8x4x_sram_info {
+	void __iomem	*bank;
+	void __iomem	*virt;
+	struct mutex	lock;
+	unsigned	active_bank;
+	struct mtd_info	mtd;
+};
+
+static int
+lp8x4x_sram_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = instr->addr >> 11;
+	unsigned offset = (instr->addr & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < instr->len; i++) {
+		iowrite8(0xff, info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	instr->state = MTD_ERASE_DONE;
+	mtd_erase_callback(instr);
+
+	return 0;
+}
+
+static int
+lp8x4x_sram_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *b)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = to >> 11;
+	unsigned offset = (to & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		iowrite8(b[i], info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static int
+lp8x4x_sram_read(struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, u_char *b)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = from >> 11;
+	unsigned offset = (from & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		b[i] = ioread8(info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static struct of_device_id of_flash_match[] = {
+	{
+		.compatible	= "icpdas,sram-lp8x4x",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_flash_match);
+
+static int
+lp8x4x_sram_probe(struct platform_device *pdev)
+{
+	const struct of_device_id *match;
+	struct lp8x4x_sram_info *info;
+	struct resource *res_virt, *res_bank;
+	char sz_str[16];
+	struct mtd_part_parser_data ppdata;
+	int err = 0;
+
+	match = of_match_device(of_flash_match, &pdev->dev);
+	if (!match)
+		return -EINVAL;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
+	if (IS_ERR(info->virt))
+		return PTR_ERR(info->virt);
+
+	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
+	if (IS_ERR(info->bank))
+		return PTR_ERR(info->bank);
+
+	info->mtd.priv = info;
+	info->mtd.name = "SRAM";
+	info->mtd.type = MTD_RAM;
+	info->mtd.flags = MTD_CAP_RAM;
+	info->mtd.size = resource_size(res_virt) << 7;
+	info->mtd.erasesize = 512;
+	info->mtd.writesize = 4;
+	info->mtd._erase = lp8x4x_sram_erase;
+	info->mtd._write = lp8x4x_sram_write;
+	info->mtd._read = lp8x4x_sram_read;
+	info->mtd.owner = THIS_MODULE;
+
+	mutex_init(&info->lock);
+	iowrite8(info->active_bank, info->bank);
+	platform_set_drvdata(pdev, info);
+
+	ppdata.of_node = pdev->dev.of_node;
+	err = mtd_device_parse_register(&info->mtd, NULL, &ppdata,
+			NULL, 0);
+
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to register MTD\n");
+		return err;
+	}
+
+	string_get_size(info->mtd.size, STRING_UNITS_2, sz_str,
+			sizeof(sz_str));
+	dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str,
+			dev_name(&info->mtd.dev));
+	return 0;
+}
+
+static int
+lp8x4x_sram_remove(struct platform_device *dev)
+{
+	struct lp8x4x_sram_info *info = platform_get_drvdata(dev);
+	return mtd_device_unregister(&info->mtd);
+}
+
+static struct platform_driver lp8x4x_sram_driver = {
+	.driver = {
+		.name		= "sram-lp8x4x",
+		.owner		= THIS_MODULE,
+		.of_match_table = of_flash_match,
+	},
+	.probe		= lp8x4x_sram_probe,
+	.remove		= lp8x4x_sram_remove,
+};
+
+module_platform_driver(lp8x4x_sram_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sergei Ianovich <ynvich@gmail.com>");
+MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8x4x");
-- 
1.8.4.2

Re: [PATCH v4 10/21] mtd: support BB SRAM on ICP DAS LP-8x4x

[Brian Norris]

Hi Sergei,

A few more small comments.

On Wed, Apr 16, 2014 at 09:17:15PM +0400, Sergei Ianovich wrote:
> This provides an MTD device driver for 512kB of battery backed up SRAM
> on ICPDAS LP-8X4X programmable automation controllers.
> 
> SRAM chip is connected via FPGA and is not accessible without a driver,
> unlike flash memory which is wired to CPU MMU.
> 
> This SRAM becomes an excellent persisent storage of volatile process
> data like counter values and sensor statuses. Storing those data in
> flash or mmc card is not a viable solution.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> ---
>    v3..v4 for Brian Norris 'Reviewed-by'
>    * add doc file for DT binding
>    * move DTS binding to a different patch (8/21)
>    * drop unused include directive
>    * drop safely unused callback
>    * drop non-default partion probe types
>    * drop duplicate error checks
>    * drop duplicate error reporting
>    * fixed error message on MTD registeration
>    * fixed module removal routine

Thanks for the updates. This patch looks pretty good to me.

>    v2..v3
>    * no changes (except number 08/16 -> 10/21)
> 
>    v0..v2
>    * use device tree
>    * use devm helpers where possible
> 
>  .../devicetree/bindings/mtd/sram-lp8x4x.txt        |  22 +++
>  arch/arm/configs/lp8x4x_defconfig                  |   1 +
>  drivers/mtd/devices/Kconfig                        |  14 ++
>  drivers/mtd/devices/Makefile                       |   1 +
>  drivers/mtd/devices/sram_lp8x4x.c                  | 204 +++++++++++++++++++++
>  5 files changed, 242 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
>  create mode 100644 drivers/mtd/devices/sram_lp8x4x.c
> 
> diff --git a/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> new file mode 100644
> index 0000000..8b1e864
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> @@ -0,0 +1,22 @@
> +512kB battery backed up SRAM on LP-8x4x industrial computers
> +
> +Required properties:
> +- compatible : should be "icpdas,sram-lp8x4x"
> +
> +- reg: physical base addresses and region lengths of
> +       * IO memory range
> +       * SRAM page selector

Are these region types pretty static for this type of hardware? If not,
it helps to have a reg-names property in the DT, when there are 2 or
more register resources.

> +- eeprom-gpios : should point to active-low write enable GPIO

I'm curious: your driver doesn't actually utilize this binding. Is this
intentional? Is it actually optional? (I note that the example DT below
doesn't have this property...)

> +
> +SRAM chip is connected via FPGA and is not accessible without a driver,
> +unlike flash memory which is wired to CPU MMU. Driver is essentially
> +an address translation routine.
> +
> +Example:
> +
> +	sram@a000 {
> +		compatible = "icpdas,sram-lp8x4x";
> +		reg = <0xa000 0x1000
> +		       0x901e 0x1>;
> +	};
> diff --git a/arch/arm/configs/lp8x4x_defconfig b/arch/arm/configs/lp8x4x_defconfig
> index d60e37a..17a4e6f 100644
> --- a/arch/arm/configs/lp8x4x_defconfig
> +++ b/arch/arm/configs/lp8x4x_defconfig
> @@ -57,6 +57,7 @@ CONFIG_MTD_CFI_ADV_OPTIONS=y
>  CONFIG_MTD_CFI_GEOMETRY=y
>  CONFIG_MTD_CFI_INTELEXT=y
>  CONFIG_MTD_PHYSMAP_OF=y
> +CONFIG_MTD_SRAM_LP8X4X=y
>  CONFIG_PROC_DEVICETREE=y
>  CONFIG_BLK_DEV_LOOP=y
>  CONFIG_BLK_DEV_LOOP_MIN_COUNT=2

I can't take the defconfig update via MTD; it will need to go via the
appropriate ARM tree (arm-soc?). So this hunk needs to move to another
patch.

> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index 1210bc2..fc8552b 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -225,4 +225,18 @@ config BCH_CONST_T
>  	default 4
>  endif
>  
> +config MTD_SRAM_LP8X4X
> +	tristate "SRAM on ICPDAS LP-8X4X"
> +	depends on OF && ARCH_PXA
> +       ---help---
> +	 This provides an MTD device driver for 512kiB of battery backed up SRAM
> +	 on ICPDAS LP-8X4X programmable automation controllers.
> +
> +	 SRAM chip is connected via FPGA and is not accessible without a driver,
> +	 unlike flash memory which is wired to CPU MMU.
> +
> +	 Say N, unless you plan to run this kernel on LP-8X4X.
> +
> +	 If you say M, the module will be called sram_lp8x4x.
> +
>  endmenu
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index c68868f..a7d86e2 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
>  obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
> +obj-$(CONFIG_MTD_SRAM_LP8X4X)	+= sram_lp8x4x.o
>  
>  
>  CFLAGS_docg3.o			+= -I$(src)
> diff --git a/drivers/mtd/devices/sram_lp8x4x.c b/drivers/mtd/devices/sram_lp8x4x.c
> new file mode 100644
> index 0000000..4cfa70b
> --- /dev/null
> +++ b/drivers/mtd/devices/sram_lp8x4x.c
> @@ -0,0 +1,204 @@
> +/*
> + *  linux/drivers/mtd/devices/lp8x4x_sram.c
> + *
> + *  MTD Driver for SRAM on ICPDAS LP-8x4x
> + *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation or any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/map.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/string_helpers.h>
> +#include <linux/types.h>
> +
> +struct lp8x4x_sram_info {
> +	void __iomem	*bank;
> +	void __iomem	*virt;
> +	struct mutex	lock;
> +	unsigned	active_bank;
> +	struct mtd_info	mtd;
> +};
> +
> +static int
> +lp8x4x_sram_erase(struct mtd_info *mtd, struct erase_info *instr)
> +{
> +	struct lp8x4x_sram_info *info = mtd->priv;
> +	unsigned bank = instr->addr >> 11;
> +	unsigned offset = (instr->addr & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < instr->len; i++) {
> +		iowrite8(0xff, info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	instr->state = MTD_ERASE_DONE;
> +	mtd_erase_callback(instr);
> +
> +	return 0;
> +}
> +
> +static int
> +lp8x4x_sram_write(struct mtd_info *mtd, loff_t to, size_t len,
> +		size_t *retlen, const u_char *b)
> +{
> +	struct lp8x4x_sram_info *info = mtd->priv;
> +	unsigned bank = to >> 11;
> +	unsigned offset = (to & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < len; i++) {
> +		iowrite8(b[i], info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	*retlen = len;
> +	return 0;
> +}
> +
> +static int
> +lp8x4x_sram_read(struct mtd_info *mtd, loff_t from, size_t len,
> +		size_t *retlen, u_char *b)
> +{
> +	struct lp8x4x_sram_info *info = mtd->priv;
> +	unsigned bank = from >> 11;
> +	unsigned offset = (from & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < len; i++) {
> +		b[i] = ioread8(info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	*retlen = len;
> +	return 0;
> +}
> +
> +static struct of_device_id of_flash_match[] = {
> +	{
> +		.compatible	= "icpdas,sram-lp8x4x",
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_flash_match);
> +
> +static int
> +lp8x4x_sram_probe(struct platform_device *pdev)
> +{
> +	const struct of_device_id *match;
> +	struct lp8x4x_sram_info *info;
> +	struct resource *res_virt, *res_bank;
> +	char sz_str[16];
> +	struct mtd_part_parser_data ppdata;
> +	int err = 0;
> +
> +	match = of_match_device(of_flash_match, &pdev->dev);
> +	if (!match)
> +		return -EINVAL;

Does this of_match_device() serve any particular purpose? Your driver
already matches against these IDs, and you're not actually retrieving
any of-data from the match, so this looks redundant.

> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
> +	if (IS_ERR(info->virt))
> +		return PTR_ERR(info->virt);
> +
> +	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
> +	if (IS_ERR(info->bank))
> +		return PTR_ERR(info->bank);
> +
> +	info->mtd.priv = info;
> +	info->mtd.name = "SRAM";

Are you absolutely sure there is only ever a single SRAM device on a
given system? Because otherwise, you will get redundantly-named MTD's.
If the answer is no, you might consider a unique naming scheme.

> +	info->mtd.type = MTD_RAM;
> +	info->mtd.flags = MTD_CAP_RAM;
> +	info->mtd.size = resource_size(res_virt) << 7;
> +	info->mtd.erasesize = 512;
> +	info->mtd.writesize = 4;
> +	info->mtd._erase = lp8x4x_sram_erase;
> +	info->mtd._write = lp8x4x_sram_write;
> +	info->mtd._read = lp8x4x_sram_read;
> +	info->mtd.owner = THIS_MODULE;
> +
> +	mutex_init(&info->lock);
> +	iowrite8(info->active_bank, info->bank);
> +	platform_set_drvdata(pdev, info);
> +
> +	ppdata.of_node = pdev->dev.of_node;
> +	err = mtd_device_parse_register(&info->mtd, NULL, &ppdata,
> +			NULL, 0);
> +
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to register MTD\n");
> +		return err;
> +	}
> +
> +	string_get_size(info->mtd.size, STRING_UNITS_2, sz_str,
> +			sizeof(sz_str));
> +	dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str,
> +			dev_name(&info->mtd.dev));
> +	return 0;
> +}
> +
> +static int
> +lp8x4x_sram_remove(struct platform_device *dev)
> +{
> +	struct lp8x4x_sram_info *info = platform_get_drvdata(dev);
> +	return mtd_device_unregister(&info->mtd);
> +}
> +
> +static struct platform_driver lp8x4x_sram_driver = {
> +	.driver = {
> +		.name		= "sram-lp8x4x",
> +		.owner		= THIS_MODULE,
> +		.of_match_table = of_flash_match,
> +	},
> +	.probe		= lp8x4x_sram_probe,
> +	.remove		= lp8x4x_sram_remove,
> +};
> +
> +module_platform_driver(lp8x4x_sram_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sergei Ianovich <ynvich@gmail.com>");
> +MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8x4x");

Thanks,
Brian

Re: [PATCH v4 10/21] mtd: support BB SRAM on ICP DAS LP-8x4x

[ООО "ЭлектроПлюс"]

Hi Brian,

On Wed, 2014-04-30 at 10:21 -0700, Brian Norris wrote:
> A few more small comments.
> 
> On Wed, Apr 16, 2014 at 09:17:15PM +0400, Sergei Ianovich wrote:
> > +++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> > @@ -0,0 +1,22 @@
> > +512kB battery backed up SRAM on LP-8x4x industrial computers
> > +
> > +Required properties:
> > +- compatible : should be "icpdas,sram-lp8x4x"
> > +
> > +- reg: physical base addresses and region lengths of
> > +       * IO memory range
> > +       * SRAM page selector
> 
> Are these region types pretty static for this type of hardware? If not,
> it helps to have a reg-names property in the DT, when there are 2 or
> more register resources.

The regions are fixed. The addresses are hard-wired.

> > +- eeprom-gpios : should point to active-low write enable GPIO
> 
> I'm curious: your driver doesn't actually utilize this binding. Is this
> intentional? Is it actually optional? (I note that the example DT below
> doesn't have this property...)

Thanks for noticing. It's an artifact of copy-paste. I'll drop this.

> > +++ b/arch/arm/configs/lp8x4x_defconfig
> > @@ -57,6 +57,7 @@ CONFIG_MTD_CFI_ADV_OPTIONS=y
> >  CONFIG_MTD_CFI_GEOMETRY=y
> >  CONFIG_MTD_CFI_INTELEXT=y
> >  CONFIG_MTD_PHYSMAP_OF=y
> > +CONFIG_MTD_SRAM_LP8X4X=y
> >  CONFIG_PROC_DEVICETREE=y
> >  CONFIG_BLK_DEV_LOOP=y
> >  CONFIG_BLK_DEV_LOOP_MIN_COUNT=2
> 
> I can't take the defconfig update via MTD; it will need to go via the
> appropriate ARM tree (arm-soc?). So this hunk needs to move to another
> patch.

Sure. I'll remove this chunk and put it into main device patch.

> > +	match = of_match_device(of_flash_match, &pdev->dev);
> > +	if (!match)
> > +		return -EINVAL;
> 
> Does this of_match_device() serve any particular purpose? Your driver
> already matches against these IDs, and you're not actually retrieving
> any of-data from the match, so this looks redundant.

Point taken, I'll drop this.

> > +
> > +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> > +	if (!info)
> > +		return -ENOMEM;
> > +
> > +	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
> > +	if (IS_ERR(info->virt))
> > +		return PTR_ERR(info->virt);
> > +
> > +	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
> > +	if (IS_ERR(info->bank))
> > +		return PTR_ERR(info->bank);
> > +
> > +	info->mtd.priv = info;
> > +	info->mtd.name = "SRAM";
> 
> Are you absolutely sure there is only ever a single SRAM device on a
> given system? Because otherwise, you will get redundantly-named MTD's.
> If the answer is no, you might consider a unique naming scheme.

Like .999999 sure. This one is hard-wired. There is no extension slots
to plug in any memory device.

I'll post a new version with the rest of the series. Thanks for
reviewing.

[PATCH v5] mtd: support BB SRAM on ICP DAS LP-8x4x

[Sergei Ianovich]

This provides an MTD device driver for 512kB of battery backed up SRAM
on ICPDAS LP-8X4X programmable automation controllers.

SRAM chip is connected via FPGA and is not accessible without a driver,
unlike flash memory which is wired to CPU MMU.

This SRAM becomes an excellent persisent storage of volatile process
data like counter values and sensor statuses. Storing those data in
flash or mmc card is not a viable solution.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Brian Norris <computersforpeace@gmail.com>
---
   v4..v5
   * remove .owner from struct platform_driver
   * constify struct of_device_id
    for further Brian Norris comments:
   * drop unused property from doc file
   * move defconfig update to a different file
   * drop extra match w/ of_match_device()

   v3..v4 for Brian Norris 'Reviewed-by'
   * add doc file for DT binding
   * move DTS binding to a different patch (8/21)
   * drop unused include directive
   * drop safely unused callback
   * drop non-default partion probe types
   * drop duplicate error checks
   * drop duplicate error reporting
   * fixed error message on MTD registeration
   * fixed module removal routine

   v2..v3
   * no changes (except number 08/16 -> 10/21)

   v0..v2
   * use device tree
   * use devm helpers where possible

 .../devicetree/bindings/mtd/sram-lp8x4x.txt        |  20 +++
 drivers/mtd/devices/Kconfig                        |  14 ++
 drivers/mtd/devices/Makefile                       |   1 +
 drivers/mtd/devices/sram_lp8x4x.c                  | 199 +++++++++++++++++++++
 4 files changed, 234 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
 create mode 100644 drivers/mtd/devices/sram_lp8x4x.c

diff --git a/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
new file mode 100644
index 0000000..476934f
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
@@ -0,0 +1,20 @@
+512kB battery backed up SRAM on LP-8x4x industrial computers
+
+Required properties:
+- compatible : should be "icpdas,sram-lp8x4x"
+
+- reg: physical base addresses and region lengths of
+       * IO memory range
+       * SRAM page selector
+
+SRAM chip is connected via FPGA and is not accessible without a driver,
+unlike flash memory which is wired to CPU MMU. Driver is essentially
+an address translation routine.
+
+Example:
+
+	sram@a000 {
+		compatible = "icpdas,sram-lp8x4x";
+		reg = <0xa000 0x1000
+		       0x901e 0x1>;
+	};
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index f73c416..a4573f6 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -233,4 +233,18 @@ config BCH_CONST_T
 	default 4
 endif
 
+config MTD_SRAM_LP8X4X
+	tristate "SRAM on ICPDAS LP-8X4X"
+	depends on OF && ARCH_PXA
+       ---help---
+	 This provides an MTD device driver for 512kiB of battery backed up SRAM
+	 on ICPDAS LP-8X4X programmable automation controllers.
+
+	 SRAM chip is connected via FPGA and is not accessible without a driver,
+	 unlike flash memory which is wired to CPU MMU.
+
+	 Say N, unless you plan to run this kernel on LP-8X4X.
+
+	 If you say M, the module will be called sram_lp8x4x.
+
 endmenu
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index 7912d3a..2fd5b7a 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
 obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
+obj-$(CONFIG_MTD_SRAM_LP8X4X)	+= sram_lp8x4x.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/sram_lp8x4x.c b/drivers/mtd/devices/sram_lp8x4x.c
new file mode 100644
index 0000000..e43c7a7
--- /dev/null
+++ b/drivers/mtd/devices/sram_lp8x4x.c
@@ -0,0 +1,199 @@
+/*
+ *  linux/drivers/mtd/devices/lp8x4x_sram.c
+ *
+ *  MTD Driver for SRAM on ICPDAS LP-8x4x
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation or any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/map.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>
+#include <linux/types.h>
+
+struct lp8x4x_sram_info {
+	void __iomem	*bank;
+	void __iomem	*virt;
+	struct mutex	lock;
+	unsigned	active_bank;
+	struct mtd_info	mtd;
+};
+
+static int
+lp8x4x_sram_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = instr->addr >> 11;
+	unsigned offset = (instr->addr & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < instr->len; i++) {
+		iowrite8(0xff, info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	instr->state = MTD_ERASE_DONE;
+	mtd_erase_callback(instr);
+
+	return 0;
+}
+
+static int
+lp8x4x_sram_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *b)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = to >> 11;
+	unsigned offset = (to & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		iowrite8(b[i], info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static int
+lp8x4x_sram_read(struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, u_char *b)
+{
+	struct lp8x4x_sram_info *info = mtd->priv;
+	unsigned bank = from >> 11;
+	unsigned offset = (from & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		b[i] = ioread8(info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static const struct of_device_id of_flash_match[] = {
+	{
+		.compatible	= "icpdas,sram-lp8x4x",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_flash_match);
+
+static int
+lp8x4x_sram_probe(struct platform_device *pdev)
+{
+	struct lp8x4x_sram_info *info;
+	struct resource *res_virt, *res_bank;
+	char sz_str[16];
+	struct mtd_part_parser_data ppdata;
+	int err = 0;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
+	if (IS_ERR(info->virt))
+		return PTR_ERR(info->virt);
+
+	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
+	if (IS_ERR(info->bank))
+		return PTR_ERR(info->bank);
+
+	info->mtd.priv = info;
+	info->mtd.name = "SRAM";
+	info->mtd.type = MTD_RAM;
+	info->mtd.flags = MTD_CAP_RAM;
+	info->mtd.size = resource_size(res_virt) << 7;
+	info->mtd.erasesize = 512;
+	info->mtd.writesize = 4;
+	info->mtd._erase = lp8x4x_sram_erase;
+	info->mtd._write = lp8x4x_sram_write;
+	info->mtd._read = lp8x4x_sram_read;
+	info->mtd.owner = THIS_MODULE;
+
+	mutex_init(&info->lock);
+	iowrite8(info->active_bank, info->bank);
+	platform_set_drvdata(pdev, info);
+
+	ppdata.of_node = pdev->dev.of_node;
+	err = mtd_device_parse_register(&info->mtd, NULL, &ppdata,
+			NULL, 0);
+
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to register MTD\n");
+		return err;
+	}
+
+	string_get_size(info->mtd.size, 1, STRING_UNITS_2, sz_str,
+			sizeof(sz_str));
+	dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str,
+			dev_name(&info->mtd.dev));
+	return 0;
+}
+
+static int
+lp8x4x_sram_remove(struct platform_device *dev)
+{
+	struct lp8x4x_sram_info *info = platform_get_drvdata(dev);
+
+	return mtd_device_unregister(&info->mtd);
+}
+
+static struct platform_driver lp8x4x_sram_driver = {
+	.driver = {
+		.name		= "sram-lp8x4x",
+		.of_match_table = of_flash_match,
+	},
+	.probe		= lp8x4x_sram_probe,
+	.remove		= lp8x4x_sram_remove,
+};
+
+module_platform_driver(lp8x4x_sram_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sergei Ianovich <ynvich@gmail.com>");
+MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8x4x");
-- 
2.6.2

Re: [PATCH v5] mtd: support BB SRAM on ICP DAS LP-8x4x

[Rob Herring]

On Tue, Dec 15, 2015 at 09:58:53PM +0300, Sergei Ianovich wrote:
> This provides an MTD device driver for 512kB of battery backed up SRAM
> on ICPDAS LP-8X4X programmable automation controllers.
> 
> SRAM chip is connected via FPGA and is not accessible without a driver,
> unlike flash memory which is wired to CPU MMU.
> 
> This SRAM becomes an excellent persisent storage of volatile process
> data like counter values and sensor statuses. Storing those data in
> flash or mmc card is not a viable solution.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> ---
>    v4..v5
>    * remove .owner from struct platform_driver
>    * constify struct of_device_id
>     for further Brian Norris comments:
>    * drop unused property from doc file
>    * move defconfig update to a different file
>    * drop extra match w/ of_match_device()
> 
>    v3..v4 for Brian Norris 'Reviewed-by'
>    * add doc file for DT binding
>    * move DTS binding to a different patch (8/21)
>    * drop unused include directive
>    * drop safely unused callback
>    * drop non-default partion probe types
>    * drop duplicate error checks
>    * drop duplicate error reporting
>    * fixed error message on MTD registeration
>    * fixed module removal routine
> 
>    v2..v3
>    * no changes (except number 08/16 -> 10/21)
> 
>    v0..v2
>    * use device tree
>    * use devm helpers where possible
> 
>  .../devicetree/bindings/mtd/sram-lp8x4x.txt        |  20 +++
>  drivers/mtd/devices/Kconfig                        |  14 ++
>  drivers/mtd/devices/Makefile                       |   1 +
>  drivers/mtd/devices/sram_lp8x4x.c                  | 199 +++++++++++++++++++++
>  4 files changed, 234 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
>  create mode 100644 drivers/mtd/devices/sram_lp8x4x.c
> 
> diff --git a/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> new file mode 100644
> index 0000000..476934f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> @@ -0,0 +1,20 @@
> +512kB battery backed up SRAM on LP-8x4x industrial computers
> +
> +Required properties:
> +- compatible : should be "icpdas,sram-lp8x4x"

No wildcards please. Otherwise looks fine.

> +
> +- reg: physical base addresses and region lengths of
> +       * IO memory range
> +       * SRAM page selector
> +
> +SRAM chip is connected via FPGA and is not accessible without a driver,
> +unlike flash memory which is wired to CPU MMU. Driver is essentially
> +an address translation routine.
> +
> +Example:
> +
> +	sram@a000 {
> +		compatible = "icpdas,sram-lp8x4x";
> +		reg = <0xa000 0x1000
> +		       0x901e 0x1>;
> +	};

Re: [PATCH v5] mtd: support BB SRAM on ICP DAS LP-8x4x

[Sergei Ianovich]

On Sat, 2015-12-19 at 21:38 -0600, Rob Herring wrote:
> On Tue, Dec 15, 2015 at 09:58:53PM +0300, Sergei Ianovich wrote:
> > This provides an MTD device driver for 512kB of battery backed up
> > SRAM
> > on ICPDAS LP-8X4X programmable automation controllers.
> > 
> > SRAM chip is connected via FPGA and is not accessible without a
> > driver,
> > unlike flash memory which is wired to CPU MMU.
> > 
> > This SRAM becomes an excellent persisent storage of volatile process
> > data like counter values and sensor statuses. Storing those data in
> > flash or mmc card is not a viable solution.
> > 
> > Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> > Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> > ---
> >    v4..v5
> >    * remove .owner from struct platform_driver
> >    * constify struct of_device_id
> >     for further Brian Norris comments:
> >    * drop unused property from doc file
> >    * move defconfig update to a different file
> >    * drop extra match w/ of_match_device()
> > 
> >    v3..v4 for Brian Norris 'Reviewed-by'
> >    * add doc file for DT binding
> >    * move DTS binding to a different patch (8/21)
> >    * drop unused include directive
> >    * drop safely unused callback
> >    * drop non-default partion probe types
> >    * drop duplicate error checks
> >    * drop duplicate error reporting
> >    * fixed error message on MTD registeration
> >    * fixed module removal routine
> > 
> >    v2..v3
> >    * no changes (except number 08/16 -> 10/21)
> > 
> >    v0..v2
> >    * use device tree
> >    * use devm helpers where possible
> > 
> >  .../devicetree/bindings/mtd/sram-lp8x4x.txt        |  20 +++
> >  drivers/mtd/devices/Kconfig                        |  14 ++
> >  drivers/mtd/devices/Makefile                       |   1 +
> >  drivers/mtd/devices/sram_lp8x4x.c                  | 199
> > +++++++++++++++++++++
> >  4 files changed, 234 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mtd/sram-
> > lp8x4x.txt
> >  create mode 100644 drivers/mtd/devices/sram_lp8x4x.c
> > 
> > diff --git a/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> > b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> > new file mode 100644
> > index 0000000..476934f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mtd/sram-lp8x4x.txt
> > @@ -0,0 +1,20 @@
> > +512kB battery backed up SRAM on LP-8x4x industrial computers
> > +
> > +Required properties:
> > +- compatible : should be "icpdas,sram-lp8x4x"
> 
> No wildcards please. Otherwise looks fine.

There is a similar review comment from Arnd Bergmann in the discussion
of `[PATCH v5] serial: support for 16550A serial ports on LP-8x4x`.

I'll quote my latest clarification:
> ... This driver will support ports on LP-8081, 
> LP-8141, LP-8441, LP-8841. Last time I checked the vendor was announcing
> a series with 3 as the last digit. They use lp8x4x name, eg. in
> documentation like `LP-8x4x_ChangeLog.txt`. They ship their proprietary
> SDK in `lp8x4x_sdk_for_linux.tar`. All of this implies that it is a
> single board.

I think the solution should be the same for all LP-8x4x drivers (IRQ,
SRAM, SERIAL, IIO).

Re: [PATCH v5] mtd: support BB SRAM on ICP DAS LP-8x4x

[Brian Norris]

On Sun, Dec 20, 2015 at 01:43:58PM +0300, Sergei Ianovich wrote:
> On Sat, 2015-12-19 at 21:38 -0600, Rob Herring wrote:
> > On Tue, Dec 15, 2015 at 09:58:53PM +0300, Sergei Ianovich wrote:
> > > +Required properties:
> > > +- compatible : should be "icpdas,sram-lp8x4x"
> > 
> > No wildcards please. Otherwise looks fine.
> 
> There is a similar review comment from Arnd Bergmann in the discussion
> of `[PATCH v5] serial: support for 16550A serial ports on LP-8x4x`.
> 
> I'll quote my latest clarification:
> > ... This driver will support ports on LP-8081, 

^^ So 8081 doesn't even match the wildcard scheme you give in the
compatible string, proving the point of the Conventional Wisdom
suggestion Rob gave...

> > LP-8141, LP-8441, LP-8841. Last time I checked the vendor was announcing
> > a series with 3 as the last digit. They use lp8x4x name, eg. in
> > documentation like `LP-8x4x_ChangeLog.txt`. They ship their proprietary
> > SDK in `lp8x4x_sdk_for_linux.tar`. All of this implies that it is a
> > single board.
> 
> I think the solution should be the same for all LP-8x4x drivers (IRQ,
> SRAM, SERIAL, IIO).

The rationale is described here:

http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property

Quote:
> Warning: Don't use wildcard compatible values, like "fsl,mpc83xx-uart"
> or similar. Silicon vendors will invariably make a change that breaks
> your wildcard assumptions the moment it is too late to change it.
> Instead, choose a specific silicon implementations and make all
> subsequent silicon compatible with it.

I don't think your circumstance is anything unique.

Regards,
Brian

[PATCH v6] mtd: support BB SRAM on ICP DAS LP-8x4x

[Sergei Ianovich]

This provides an MTD device driver for 512kB of battery backed up SRAM
on ICPDAS LP-8X4X programmable automation controllers.

SRAM chip is connected via FPGA and is not accessible without a driver,
unlike flash memory which is wired to CPU MMU.

This SRAM becomes an excellent persisent storage of volatile process
data like counter values and sensor statuses. Storing those data in
flash or mmc card is not a viable solution.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
Reviewed-by: Brian Norris <computersforpeace@gmail.com>
CC: Rob Herring <robh@kernel.org>

   v5..v6
   * replace wildcards in compatible and module name
   * drop obsolete mtd_part_parser_data.of_node

   v4..v5
   * remove .owner from struct platform_driver
   * constify struct of_device_id
    for further Brian Norris comments:
   * drop unused property from doc file
   * move defconfig update to a different file
   * drop extra match w/ of_match_device()

   v3..v4 for Brian Norris 'Reviewed-by'
   * add doc file for DT binding
   * move DTS binding to a different patch (8/21)
   * drop unused include directive
   * drop safely unused callback
   * drop non-default partion probe types
   * drop duplicate error checks
   * drop duplicate error reporting
   * fixed error message on MTD registeration
   * fixed module removal routine

   v2..v3
   * no changes (except number 08/16 -> 10/21)

   v0..v2
   * use device tree
   * use devm helpers where possible
---
 .../devicetree/bindings/mtd/icpdas-lp8841-sram.txt |  23 +++
 drivers/mtd/devices/Kconfig                        |  14 ++
 drivers/mtd/devices/Makefile                       |   1 +
 drivers/mtd/devices/sram_lp8841.c                  | 198 +++++++++++++++++++++
 4 files changed, 236 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
 create mode 100644 drivers/mtd/devices/sram_lp8841.c

diff --git a/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt b/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
new file mode 100644
index 0000000..3c1007a
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
@@ -0,0 +1,23 @@
+512kB battery backed up SRAM on ICP DAS LP-8841 industrial computers
+
+LP-8441, LP-8141, and LP-8041 differ from LP-8841 only in expansion
+slot count.
+
+Required properties:
+- compatible : should be "icpdas,lp8841-sram"
+
+- reg: physical base addresses and region lengths of
+       * IO memory range
+       * SRAM page selector
+
+SRAM chip is connected via FPGA and is not accessible without a driver,
+unlike flash memory which is wired to CPU MMU. Driver is essentially
+an address translation routine.
+
+Example:
+
+	sram@a000 {
+		compatible = "icpdas,lp8841-sram";
+		reg = <0xa000 0x1000
+		       0x901e 0x1>;
+	};
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index f73c416..ecf5733 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -233,4 +233,18 @@ config BCH_CONST_T
 	default 4
 endif
 
+config MTD_SRAM_LP8841
+	tristate "SRAM on ICP DAS LP-8841"
+	depends on OF && ARCH_PXA
+       ---help---
+	 This provides an MTD device driver for 512kiB of battery backed up SRAM
+	 on ICPDAS LP-8X41 programmable automation controllers.
+
+	 SRAM chip is connected via FPGA and is not accessible without a driver,
+	 unlike flash memory which is wired to CPU MMU.
+
+	 Say N, unless you plan to run this kernel on LP-8X41.
+
+	 If you say M, the module will be called sram_lp8841.
+
 endmenu
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index 7912d3a..46df5d6 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
 obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
+obj-$(CONFIG_MTD_SRAM_LP8841)	+= sram_lp8841.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/sram_lp8841.c b/drivers/mtd/devices/sram_lp8841.c
new file mode 100644
index 0000000..5d5d942
--- /dev/null
+++ b/drivers/mtd/devices/sram_lp8841.c
@@ -0,0 +1,198 @@
+/*
+ *  linux/drivers/mtd/devices/sram_lp8841.c
+ *
+ *  MTD Driver for SRAM on ICP DAS LP-8841
+ *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation or any later version.
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/map.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/slab.h>
+#include <linux/string_helpers.h>
+#include <linux/types.h>
+
+struct lp8841_sram_info {
+	void __iomem	*bank;
+	void __iomem	*virt;
+	struct mutex	lock;
+	unsigned	active_bank;
+	struct mtd_info	mtd;
+};
+
+static int
+lp8841_sram_erase(struct mtd_info *mtd, struct erase_info *instr)
+{
+	struct lp8841_sram_info *info = mtd->priv;
+	unsigned bank = instr->addr >> 11;
+	unsigned offset = (instr->addr & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < instr->len; i++) {
+		iowrite8(0xff, info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	instr->state = MTD_ERASE_DONE;
+	mtd_erase_callback(instr);
+
+	return 0;
+}
+
+static int
+lp8841_sram_write(struct mtd_info *mtd, loff_t to, size_t len,
+		size_t *retlen, const u_char *b)
+{
+	struct lp8841_sram_info *info = mtd->priv;
+	unsigned bank = to >> 11;
+	unsigned offset = (to & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		iowrite8(b[i], info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static int
+lp8841_sram_read(struct mtd_info *mtd, loff_t from, size_t len,
+		size_t *retlen, u_char *b)
+{
+	struct lp8841_sram_info *info = mtd->priv;
+	unsigned bank = from >> 11;
+	unsigned offset = (from & 0x7ff) << 1;
+	loff_t i;
+
+	mutex_lock(&info->lock);
+	if (unlikely(bank != info->active_bank)) {
+		info->active_bank = bank;
+		iowrite8(bank, info->bank);
+	}
+	for (i = 0; i < len; i++) {
+		b[i] = ioread8(info->virt + offset);
+		offset += 2;
+		if (unlikely(offset == 0)) {
+			info->active_bank++;
+			iowrite8(info->active_bank, info->bank);
+		}
+	}
+	mutex_unlock(&info->lock);
+	*retlen = len;
+	return 0;
+}
+
+static const struct of_device_id of_flash_match[] = {
+	{
+		.compatible	= "icpdas,lp8841-sram",
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, of_flash_match);
+
+static int
+lp8841_sram_probe(struct platform_device *pdev)
+{
+	struct lp8841_sram_info *info;
+	struct resource *res_virt, *res_bank;
+	char sz_str[16];
+	struct mtd_part_parser_data ppdata;
+	int err = 0;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
+	if (IS_ERR(info->virt))
+		return PTR_ERR(info->virt);
+
+	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
+	if (IS_ERR(info->bank))
+		return PTR_ERR(info->bank);
+
+	info->mtd.priv = info;
+	info->mtd.name = "SRAM";
+	info->mtd.type = MTD_RAM;
+	info->mtd.flags = MTD_CAP_RAM;
+	info->mtd.size = resource_size(res_virt) << 7;
+	info->mtd.erasesize = 512;
+	info->mtd.writesize = 4;
+	info->mtd._erase = lp8841_sram_erase;
+	info->mtd._write = lp8841_sram_write;
+	info->mtd._read = lp8841_sram_read;
+	info->mtd.owner = THIS_MODULE;
+
+	mutex_init(&info->lock);
+	iowrite8(info->active_bank, info->bank);
+	platform_set_drvdata(pdev, info);
+
+	err = mtd_device_parse_register(&info->mtd, NULL, &ppdata,
+			NULL, 0);
+
+	if (err < 0) {
+		dev_err(&pdev->dev, "failed to register MTD\n");
+		return err;
+	}
+
+	string_get_size(info->mtd.size, 1, STRING_UNITS_2, sz_str,
+			sizeof(sz_str));
+	dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str,
+			dev_name(&info->mtd.dev));
+	return 0;
+}
+
+static int
+lp8841_sram_remove(struct platform_device *dev)
+{
+	struct lp8841_sram_info *info = platform_get_drvdata(dev);
+
+	return mtd_device_unregister(&info->mtd);
+}
+
+static struct platform_driver lp8841_sram_driver = {
+	.driver = {
+		.name		= "sram-lp8841",
+		.of_match_table = of_flash_match,
+	},
+	.probe		= lp8841_sram_probe,
+	.remove		= lp8841_sram_remove,
+};
+
+module_platform_driver(lp8841_sram_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Sergei Ianovich");
+MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8841");
-- 
2.7.0

Re: [PATCH v6] mtd: support BB SRAM on ICP DAS LP-8x4x

[Rob Herring]

On Tue, Feb 23, 2016 at 09:58:01PM +0300, Sergei Ianovich wrote:
> This provides an MTD device driver for 512kB of battery backed up SRAM
> on ICPDAS LP-8X4X programmable automation controllers.
> 
> SRAM chip is connected via FPGA and is not accessible without a driver,
> unlike flash memory which is wired to CPU MMU.
> 
> This SRAM becomes an excellent persisent storage of volatile process
> data like counter values and sensor statuses. Storing those data in
> flash or mmc card is not a viable solution.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> CC: Rob Herring <robh@kernel.org>
> 
>    v5..v6
>    * replace wildcards in compatible and module name
>    * drop obsolete mtd_part_parser_data.of_node
> 
>    v4..v5
>    * remove .owner from struct platform_driver
>    * constify struct of_device_id
>     for further Brian Norris comments:
>    * drop unused property from doc file
>    * move defconfig update to a different file
>    * drop extra match w/ of_match_device()
> 
>    v3..v4 for Brian Norris 'Reviewed-by'
>    * add doc file for DT binding
>    * move DTS binding to a different patch (8/21)
>    * drop unused include directive
>    * drop safely unused callback
>    * drop non-default partion probe types
>    * drop duplicate error checks
>    * drop duplicate error reporting
>    * fixed error message on MTD registeration
>    * fixed module removal routine
> 
>    v2..v3
>    * no changes (except number 08/16 -> 10/21)
> 
>    v0..v2
>    * use device tree
>    * use devm helpers where possible
> ---
>  .../devicetree/bindings/mtd/icpdas-lp8841-sram.txt |  23 +++

Acked-by: Rob Herring <robh@kernel.org>

>  drivers/mtd/devices/Kconfig                        |  14 ++
>  drivers/mtd/devices/Makefile                       |   1 +
>  drivers/mtd/devices/sram_lp8841.c                  | 198 +++++++++++++++++++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
>  create mode 100644 drivers/mtd/devices/sram_lp8841.c

Re: [PATCH v6] mtd: support BB SRAM on ICP DAS LP-8x4x

[Brian Norris]

Hi Sergei,

A few more issues...

On Tue, Feb 23, 2016 at 09:58:01PM +0300, Sergei Ianovich wrote:
> This provides an MTD device driver for 512kB of battery backed up SRAM
> on ICPDAS LP-8X4X programmable automation controllers.
> 
> SRAM chip is connected via FPGA and is not accessible without a driver,
> unlike flash memory which is wired to CPU MMU.
> 
> This SRAM becomes an excellent persisent storage of volatile process
> data like counter values and sensor statuses. Storing those data in
> flash or mmc card is not a viable solution.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> Reviewed-by: Brian Norris <computersforpeace@gmail.com>
> CC: Rob Herring <robh@kernel.org>
> 
>    v5..v6
>    * replace wildcards in compatible and module name
>    * drop obsolete mtd_part_parser_data.of_node
> 
>    v4..v5
>    * remove .owner from struct platform_driver
>    * constify struct of_device_id
>     for further Brian Norris comments:
>    * drop unused property from doc file
>    * move defconfig update to a different file
>    * drop extra match w/ of_match_device()
> 
>    v3..v4 for Brian Norris 'Reviewed-by'
>    * add doc file for DT binding
>    * move DTS binding to a different patch (8/21)
>    * drop unused include directive
>    * drop safely unused callback
>    * drop non-default partion probe types
>    * drop duplicate error checks
>    * drop duplicate error reporting
>    * fixed error message on MTD registeration
>    * fixed module removal routine
> 
>    v2..v3
>    * no changes (except number 08/16 -> 10/21)
> 
>    v0..v2
>    * use device tree
>    * use devm helpers where possible
> ---
>  .../devicetree/bindings/mtd/icpdas-lp8841-sram.txt |  23 +++
>  drivers/mtd/devices/Kconfig                        |  14 ++
>  drivers/mtd/devices/Makefile                       |   1 +
>  drivers/mtd/devices/sram_lp8841.c                  | 198 +++++++++++++++++++++
>  4 files changed, 236 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
>  create mode 100644 drivers/mtd/devices/sram_lp8841.c
> 
> diff --git a/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt b/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
> new file mode 100644
> index 0000000..3c1007a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/icpdas-lp8841-sram.txt
> @@ -0,0 +1,23 @@
> +512kB battery backed up SRAM on ICP DAS LP-8841 industrial computers
> +
> +LP-8441, LP-8141, and LP-8041 differ from LP-8841 only in expansion
> +slot count.
> +
> +Required properties:
> +- compatible : should be "icpdas,lp8841-sram"
> +
> +- reg: physical base addresses and region lengths of
> +       * IO memory range
> +       * SRAM page selector
> +
> +SRAM chip is connected via FPGA and is not accessible without a driver,
> +unlike flash memory which is wired to CPU MMU. Driver is essentially
> +an address translation routine.
> +
> +Example:
> +
> +	sram@a000 {
> +		compatible = "icpdas,lp8841-sram";
> +		reg = <0xa000 0x1000
> +		       0x901e 0x1>;
> +	};
> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index f73c416..ecf5733 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -233,4 +233,18 @@ config BCH_CONST_T
>  	default 4
>  endif
>  
> +config MTD_SRAM_LP8841
> +	tristate "SRAM on ICP DAS LP-8841"
> +	depends on OF && ARCH_PXA
> +       ---help---
> +	 This provides an MTD device driver for 512kiB of battery backed up SRAM
> +	 on ICPDAS LP-8X41 programmable automation controllers.
> +
> +	 SRAM chip is connected via FPGA and is not accessible without a driver,
> +	 unlike flash memory which is wired to CPU MMU.
> +
> +	 Say N, unless you plan to run this kernel on LP-8X41.
> +
> +	 If you say M, the module will be called sram_lp8841.
> +
>  endmenu
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index 7912d3a..46df5d6 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
>  obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
>  obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
> +obj-$(CONFIG_MTD_SRAM_LP8841)	+= sram_lp8841.o
>  
>  
>  CFLAGS_docg3.o			+= -I$(src)
> diff --git a/drivers/mtd/devices/sram_lp8841.c b/drivers/mtd/devices/sram_lp8841.c
> new file mode 100644
> index 0000000..5d5d942
> --- /dev/null
> +++ b/drivers/mtd/devices/sram_lp8841.c
> @@ -0,0 +1,198 @@
> +/*
> + *  linux/drivers/mtd/devices/sram_lp8841.c
> + *
> + *  MTD Driver for SRAM on ICP DAS LP-8841
> + *  Copyright (C) 2013 Sergei Ianovich <ynvich@gmail.com>
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation or any later version.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mtd/map.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/platform_device.h>
> +#include <linux/of_device.h>
> +#include <linux/slab.h>
> +#include <linux/string_helpers.h>
> +#include <linux/types.h>
> +
> +struct lp8841_sram_info {
> +	void __iomem	*bank;
> +	void __iomem	*virt;
> +	struct mutex	lock;
> +	unsigned	active_bank;
> +	struct mtd_info	mtd;
> +};
> +
> +static int
> +lp8841_sram_erase(struct mtd_info *mtd, struct erase_info *instr)
> +{
> +	struct lp8841_sram_info *info = mtd->priv;
> +	unsigned bank = instr->addr >> 11;
> +	unsigned offset = (instr->addr & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < instr->len; i++) {
> +		iowrite8(0xff, info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	instr->state = MTD_ERASE_DONE;
> +	mtd_erase_callback(instr);
> +
> +	return 0;
> +}
> +
> +static int
> +lp8841_sram_write(struct mtd_info *mtd, loff_t to, size_t len,
> +		size_t *retlen, const u_char *b)
> +{
> +	struct lp8841_sram_info *info = mtd->priv;
> +	unsigned bank = to >> 11;
> +	unsigned offset = (to & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < len; i++) {
> +		iowrite8(b[i], info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	*retlen = len;
> +	return 0;
> +}
> +
> +static int
> +lp8841_sram_read(struct mtd_info *mtd, loff_t from, size_t len,
> +		size_t *retlen, u_char *b)
> +{
> +	struct lp8841_sram_info *info = mtd->priv;
> +	unsigned bank = from >> 11;
> +	unsigned offset = (from & 0x7ff) << 1;
> +	loff_t i;
> +
> +	mutex_lock(&info->lock);
> +	if (unlikely(bank != info->active_bank)) {
> +		info->active_bank = bank;
> +		iowrite8(bank, info->bank);
> +	}
> +	for (i = 0; i < len; i++) {
> +		b[i] = ioread8(info->virt + offset);
> +		offset += 2;
> +		if (unlikely(offset == 0)) {
> +			info->active_bank++;
> +			iowrite8(info->active_bank, info->bank);
> +		}
> +	}
> +	mutex_unlock(&info->lock);
> +	*retlen = len;
> +	return 0;
> +}
> +
> +static const struct of_device_id of_flash_match[] = {
> +	{
> +		.compatible	= "icpdas,lp8841-sram",
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, of_flash_match);
> +
> +static int
> +lp8841_sram_probe(struct platform_device *pdev)
> +{
> +	struct lp8841_sram_info *info;
> +	struct resource *res_virt, *res_bank;
> +	char sz_str[16];
> +	struct mtd_part_parser_data ppdata;

You don't need this struct any more.

> +	int err = 0;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	res_virt = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->virt =  devm_ioremap_resource(&pdev->dev, res_virt);
> +	if (IS_ERR(info->virt))
> +		return PTR_ERR(info->virt);
> +
> +	res_bank = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	info->bank = devm_ioremap_resource(&pdev->dev, res_bank);
> +	if (IS_ERR(info->bank))
> +		return PTR_ERR(info->bank);
> +
> +	info->mtd.priv = info;
> +	info->mtd.name = "SRAM";
> +	info->mtd.type = MTD_RAM;
> +	info->mtd.flags = MTD_CAP_RAM;
> +	info->mtd.size = resource_size(res_virt) << 7;
> +	info->mtd.erasesize = 512;
> +	info->mtd.writesize = 4;

Can you please set mtd.writebufsize to an appropriate value too? Maybe
it's just the same as writesize.

> +	info->mtd._erase = lp8841_sram_erase;
> +	info->mtd._write = lp8841_sram_write;
> +	info->mtd._read = lp8841_sram_read;
> +	info->mtd.owner = THIS_MODULE;

If you set info->mtd.dev.parent (please do), you don't need the above
line.

> +
> +	mutex_init(&info->lock);
> +	iowrite8(info->active_bank, info->bank);
> +	platform_set_drvdata(pdev, info);
> +
> +	err = mtd_device_parse_register(&info->mtd, NULL, &ppdata,
> +			NULL, 0);

This can just be:

	err = mtd_device_register(&info->mtd, NULL, 0);

> +
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "failed to register MTD\n");
> +		return err;
> +	}
> +
> +	string_get_size(info->mtd.size, 1, STRING_UNITS_2, sz_str,
> +			sizeof(sz_str));
> +	dev_info(&pdev->dev, "using %s SRAM on LP-8X4X as %s\n", sz_str,
> +			dev_name(&info->mtd.dev));
> +	return 0;
> +}
> +
> +static int
> +lp8841_sram_remove(struct platform_device *dev)
> +{
> +	struct lp8841_sram_info *info = platform_get_drvdata(dev);
> +
> +	return mtd_device_unregister(&info->mtd);
> +}
> +
> +static struct platform_driver lp8841_sram_driver = {
> +	.driver = {
> +		.name		= "sram-lp8841",
> +		.of_match_table = of_flash_match,
> +	},
> +	.probe		= lp8841_sram_probe,
> +	.remove		= lp8841_sram_remove,
> +};
> +
> +module_platform_driver(lp8841_sram_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Sergei Ianovich");
> +MODULE_DESCRIPTION("MTD driver for SRAM on ICPDAS LP-8841");

Brian