[PATCH 0/3] Virtual MTD concat device driver

[PATCH 0/3] Virtual MTD concat device driver

[Bernhard Frauendienst]

Hi everybody,

when porting my router board from a mach-file based OpenWRT target to a
device-tree based target, I found that there is no generic way to create
a mtd_concat device from within the dts. The following patches attempt
to provide that possibility.

According to [1], patches should be provided in a git tree if possible,
but to me it was a bit unclear whether repositories outside of
git.infradead.org are also acceptable. If this is the case, the changes
can be pulled from branch 'mtd_concat' at [2] (web view at [3]. The
commits are based on mtd/next even though the repository says forked
from torvalds/linux). In addition I'm sending the patches to the mailing
list.

Thanks for your feedback!

Best regards
Bernhard Frauendienst

PS: This is my first submission to this mailing list and the linux
kernel in general. I've tried to meet all requirements, but please
let me know if I have made any rookie mistakes.

[1] http://www.linux-mtd.infradead.org/source.html
[2] https://github.com/oxc/linux.git
[3] https://github.com/oxc/linux/commits/mtd_concat


Bernhard Frauendienst (3):
  mtd: core: add get_mtd_device_by_node
  dt-bindings: add bindings for mtd-concat devices
  mtd: mtdconcat: add dt driver for concat devices

 .../devicetree/bindings/mtd/mtd-concat.txt    |  21 +++
 drivers/mtd/Kconfig                           |   2 +
 drivers/mtd/Makefile                          |   3 +
 drivers/mtd/composite/Kconfig                 |  12 ++
 drivers/mtd/composite/Makefile                |   7 +
 drivers/mtd/composite/virt_concat.c           | 123 ++++++++++++++++++
 drivers/mtd/mtdcore.c                         |  39 ++++++
 include/linux/mtd/mtd.h                       |   2 +
 8 files changed, 209 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtd-concat.txt
 create mode 100644 drivers/mtd/composite/Kconfig
 create mode 100644 drivers/mtd/composite/Makefile
 create mode 100644 drivers/mtd/composite/virt_concat.c

-- 
2.17.1

[PATCH 1/3] mtd: core: add get_mtd_device_by_node

[Bernhard Frauendienst]

Add function to retrieve a mtd device by its OF node. Since drivers can
assign arbitrary names to mtd devices in the absence of a label
property, there is no other reliable way to retrieve a mtd device for a
given OF node.

Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
---
 drivers/mtd/mtdcore.c   | 39 +++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/mtd.h |  2 ++
 2 files changed, 41 insertions(+)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 97ac219c082e..e59b6a06814c 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -926,6 +926,45 @@ struct mtd_info *get_mtd_device_nm(const char *name)
 }
 EXPORT_SYMBOL_GPL(get_mtd_device_nm);
 
+/**
+ *	get_mtd_device_by_node - obtain a validated handle for an MTD device
+ *	by of_node
+ *	@name: OF node of MTD device to open
+ *
+ *	This function returns MTD device description structure in case of
+ *	success and an error code in case of failure.
+ */
+struct mtd_info *get_mtd_device_by_node(const struct device_node *of_node)
+{
+	int err = -ENODEV;
+	struct mtd_info *mtd = NULL, *other;
+
+	mutex_lock(&mtd_table_mutex);
+
+	mtd_for_each_device(other) {
+		if (of_node == other->dev.of_node) {
+			mtd = other;
+			break;
+		}
+	}
+
+	if (!mtd)
+		goto out_unlock;
+
+	err = __get_mtd_device(mtd);
+	if (err)
+		goto out_unlock;
+
+	mutex_unlock(&mtd_table_mutex);
+	return mtd;
+
+out_unlock:
+	mutex_unlock(&mtd_table_mutex);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(get_mtd_device_by_node);
+
+
 void put_mtd_device(struct mtd_info *mtd)
 {
 	mutex_lock(&mtd_table_mutex);
diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
index cd0be91bdefa..fe71358f8eaa 100644
--- a/include/linux/mtd/mtd.h
+++ b/include/linux/mtd/mtd.h
@@ -570,6 +570,8 @@ extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
 extern int __get_mtd_device(struct mtd_info *mtd);
 extern void __put_mtd_device(struct mtd_info *mtd);
 extern struct mtd_info *get_mtd_device_nm(const char *name);
+extern struct mtd_info *get_mtd_device_by_node(
+		const struct device_node *of_node);
 extern void put_mtd_device(struct mtd_info *mtd);
 
 
-- 
2.17.1

Re: [PATCH 1/3] mtd: core: add get_mtd_device_by_node

[Miquel Raynal]

Hi Bernhard,

Minor nits below.

Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote on Thu,  6 Sep
2018 18:14:11 +0200:

> Add function to retrieve a mtd device by its OF node. Since drivers can
> assign arbitrary names to mtd devices in the absence of a label
> property, there is no other reliable way to retrieve a mtd device for a
> given OF node.
> 
> Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> ---
>  drivers/mtd/mtdcore.c   | 39 +++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/mtd.h |  2 ++
>  2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 97ac219c082e..e59b6a06814c 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -926,6 +926,45 @@ struct mtd_info *get_mtd_device_nm(const char *name)
>  }
>  EXPORT_SYMBOL_GPL(get_mtd_device_nm);
>  
> +/**
> + *	get_mtd_device_by_node - obtain a validated handle for an MTD device
> + *	by of_node
> + *	@name: OF node of MTD device to open

Should be @of_node

> + *
> + *	This function returns MTD device description structure in case of
> + *	success and an error code in case of failure.
> + */
> +struct mtd_info *get_mtd_device_by_node(const struct device_node *of_node)
> +{
> +	int err = -ENODEV;
> +	struct mtd_info *mtd = NULL, *other;
> +
> +	mutex_lock(&mtd_table_mutex);
> +
> +	mtd_for_each_device(other) {
> +		if (of_node == other->dev.of_node) {
> +			mtd = other;
> +			break;
> +		}
> +	}
> +
> +	if (!mtd)
> +		goto out_unlock;
> +
> +	err = __get_mtd_device(mtd);
> +	if (err)
> +		goto out_unlock;
> +
> +	mutex_unlock(&mtd_table_mutex);
> +	return mtd;
> +
> +out_unlock:
> +	mutex_unlock(&mtd_table_mutex);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(get_mtd_device_by_node);
> +
> +

Extra space here

>  void put_mtd_device(struct mtd_info *mtd)
>  {
>  	mutex_lock(&mtd_table_mutex);
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index cd0be91bdefa..fe71358f8eaa 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -570,6 +570,8 @@ extern struct mtd_info *get_mtd_device(struct mtd_info *mtd, int num);
>  extern int __get_mtd_device(struct mtd_info *mtd);
>  extern void __put_mtd_device(struct mtd_info *mtd);
>  extern struct mtd_info *get_mtd_device_nm(const char *name);
> +extern struct mtd_info *get_mtd_device_by_node(
> +		const struct device_node *of_node);
>  extern void put_mtd_device(struct mtd_info *mtd);
>  
>  

Otherwise,

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

[PATCH 2/3] dt-bindings: add bindings for mtd-concat devices

[Bernhard Frauendienst]

Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
---
 .../devicetree/bindings/mtd/mtd-concat.txt    | 21 +++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/mtd-concat.txt

diff --git a/Documentation/devicetree/bindings/mtd/mtd-concat.txt b/Documentation/devicetree/bindings/mtd/mtd-concat.txt
new file mode 100644
index 000000000000..9fade3af117b
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/mtd-concat.txt
@@ -0,0 +1,21 @@
+Virtual MTD concat device
+
+Requires properties:
+- devices: list of phandles to mtd nodes that should be concatenated
+
+Example:
+
+&spi {
+	flash0: flash@0 {
+		...
+	};
+	flash1: flash@1 {
+		...
+	};
+};
+
+flash {
+	compatible = "mtd-concat";
+
+	devices = <&flash0 &flash1>;
+}
-- 
2.17.1

[PATCH 3/3] mtd: mtdconcat: add dt driver for concat devices

[Bernhard Frauendienst]

Some mtd drivers like physmap variants have support for concatenating
multiple mtd devices, but there is no generic way to define such a
concat device from within the device tree.

This commit adds a driver for creating virtual mtd-concat devices. They
must have a compatible = "mtd-concat" line, and define a list of
devices to concat in the 'devices' property, for example:

flash {
  compatible = "mtd-concat";

  devices = <&flash0 &flash1>;
};

The driver is added to the very end of the mtd Makefile to increase the
likelyhood of all child devices already being loaded at the time of
probing, preventing unnecessary deferred probes.

Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
---
 drivers/mtd/Kconfig                 |   2 +
 drivers/mtd/Makefile                |   3 +
 drivers/mtd/composite/Kconfig       |  12 +++
 drivers/mtd/composite/Makefile      |   7 ++
 drivers/mtd/composite/virt_concat.c | 123 ++++++++++++++++++++++++++++
 5 files changed, 147 insertions(+)
 create mode 100644 drivers/mtd/composite/Kconfig
 create mode 100644 drivers/mtd/composite/Makefile
 create mode 100644 drivers/mtd/composite/virt_concat.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index c77f537323ec..6345d886d458 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -339,4 +339,6 @@ source "drivers/mtd/spi-nor/Kconfig"
 
 source "drivers/mtd/ubi/Kconfig"
 
+source "drivers/mtd/composite/Kconfig"
+
 endif # MTD
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 93473d215a38..57af7190b063 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -36,3 +36,6 @@ obj-y		+= chips/ lpddr/ maps/ devices/ nand/ tests/
 
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor/
 obj-$(CONFIG_MTD_UBI)		+= ubi/
+
+# Composite drivers must be loaded last
+obj-y		+= composite/
diff --git a/drivers/mtd/composite/Kconfig b/drivers/mtd/composite/Kconfig
new file mode 100644
index 000000000000..0490fc0284bb
--- /dev/null
+++ b/drivers/mtd/composite/Kconfig
@@ -0,0 +1,12 @@
+menu "Composite MTD device drivers"
+	depends on MTD!=n
+
+config MTD_VIRT_CONCAT
+	tristate "Virtual concat MTD device"
+	help
+	  This driver allows creation of a virtual MTD concat device, which
+	  concatenates multiple underlying MTD devices to a single device.
+	  This is required by some SoC boards where multiple memory banks are
+	  used as one device with partitions spanning across device boundaries.
+
+endmenu
diff --git a/drivers/mtd/composite/Makefile b/drivers/mtd/composite/Makefile
new file mode 100644
index 000000000000..7f4bdeee0e0a
--- /dev/null
+++ b/drivers/mtd/composite/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# linux/drivers/mtd/composite/Makefile
+#
+
+obj-$(CONFIG_MTD_VIRT_CONCAT)   += virt_concat.o
+
diff --git a/drivers/mtd/composite/virt_concat.c b/drivers/mtd/composite/virt_concat.c
new file mode 100644
index 000000000000..239c7cdd8bca
--- /dev/null
+++ b/drivers/mtd/composite/virt_concat.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Virtual concat MTD device driver
+ *
+ * Copyright (C) 2018 Bernhard Frauendienst
+ * Author: Bernhard Frauendienst, kernel@nospam.obeliks.de
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/mtd/concat.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+struct of_virt_concat {
+	struct mtd_info	 *cmtd;
+	int num_devices;
+	struct mtd_info	 **devices;
+};
+
+static int virt_concat_remove(struct platform_device *pdev)
+{
+	struct of_virt_concat *info;
+	int i;
+
+	info = platform_get_drvdata(pdev);
+	if (!info)
+		return 0;
+	platform_set_drvdata(pdev, NULL);
+
+	if (info->cmtd) {
+		mtd_device_unregister(info->cmtd);
+		mtd_concat_destroy(info->cmtd);
+	}
+
+	if (info->devices) {
+		for (i = 0; i < info->num_devices; i++)
+			put_mtd_device(info->devices[i]);
+
+		kfree(info->devices);
+	}
+
+	return 0;
+}
+
+static int virt_concat_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct of_phandle_iterator it;
+	int err, count;
+	struct of_virt_concat *info;
+	struct mtd_info *mtd;
+
+	count = of_count_phandle_with_args(node, "devices", NULL);
+	if (count <= 0)
+		return -EINVAL;
+
+	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+	err = -ENOMEM;
+	info->devices = kcalloc(count, sizeof(*(info->devices)), GFP_KERNEL);
+	if (!info->devices)
+		goto err_remove;
+
+	platform_set_drvdata(pdev, info);
+
+	of_for_each_phandle(&it, err, node, "devices", NULL, 0) {
+		mtd = get_mtd_device_by_node(it.node);
+		if (IS_ERR(mtd)) {
+			of_node_put(it.node);
+			err = -EPROBE_DEFER;
+			goto err_remove;
+		}
+
+		info->devices[info->num_devices++] = mtd;
+	}
+
+	err = -ENXIO;
+	info->cmtd = mtd_concat_create(info->devices, info->num_devices,
+		dev_name(&pdev->dev));
+	if (!info->cmtd)
+		goto err_remove;
+
+	info->cmtd->dev.parent = &pdev->dev;
+	mtd_set_of_node(info->cmtd, node);
+	mtd_device_register(info->cmtd, NULL, 0);
+
+	return 0;
+
+err_remove:
+	virt_concat_remove(pdev);
+
+	return err;
+}
+
+static const struct of_device_id virt_concat_of_match[] = {
+	{ .compatible = "mtd-concat", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, virt_concat_of_match);
+
+static struct platform_driver virt_concat_driver = {
+	.probe = virt_concat_probe,
+	.remove = virt_concat_remove,
+	.driver	 = {
+		.name   = "virt-mtdconcat",
+		.of_match_table = virt_concat_of_match,
+	},
+};
+
+module_platform_driver(virt_concat_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
+MODULE_DESCRIPTION("Virtual concat MTD device driver");
-- 
2.17.1

Re: [PATCH 3/3] mtd: mtdconcat: add dt driver for concat devices

[Miquel Raynal]

Hi Bernhard,

Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote on Thu,  6 Sep
2018 18:14:13 +0200:

> Some mtd drivers like physmap variants have support for concatenating
> multiple mtd devices, but there is no generic way to define such a
> concat device from within the device tree.
> 
> This commit adds a driver for creating virtual mtd-concat devices. They
> must have a compatible = "mtd-concat" line, and define a list of
> devices to concat in the 'devices' property, for example:
> 
> flash {
>   compatible = "mtd-concat";
> 
>   devices = <&flash0 &flash1>;
> };
> 
> The driver is added to the very end of the mtd Makefile to increase the
> likelyhood of all child devices already being loaded at the time of
> probing, preventing unnecessary deferred probes.

This is new for me so I'll let more experienced people talk about the
whole idea. Some questions/remarks below.
 
> 
> Signed-off-by: Bernhard Frauendienst <kernel@nospam.obeliks.de>
> ---
>  drivers/mtd/Kconfig                 |   2 +
>  drivers/mtd/Makefile                |   3 +
>  drivers/mtd/composite/Kconfig       |  12 +++
>  drivers/mtd/composite/Makefile      |   7 ++
>  drivers/mtd/composite/virt_concat.c | 123 ++++++++++++++++++++++++++++
>  5 files changed, 147 insertions(+)
>  create mode 100644 drivers/mtd/composite/Kconfig
>  create mode 100644 drivers/mtd/composite/Makefile
>  create mode 100644 drivers/mtd/composite/virt_concat.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index c77f537323ec..6345d886d458 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -339,4 +339,6 @@ source "drivers/mtd/spi-nor/Kconfig"
>  
>  source "drivers/mtd/ubi/Kconfig"
>  
> +source "drivers/mtd/composite/Kconfig"
> +
>  endif # MTD
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 93473d215a38..57af7190b063 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -36,3 +36,6 @@ obj-y		+= chips/ lpddr/ maps/ devices/ nand/ tests/
>  
>  obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor/
>  obj-$(CONFIG_MTD_UBI)		+= ubi/
> +
> +# Composite drivers must be loaded last
> +obj-y		+= composite/
> diff --git a/drivers/mtd/composite/Kconfig b/drivers/mtd/composite/Kconfig
> new file mode 100644
> index 000000000000..0490fc0284bb
> --- /dev/null
> +++ b/drivers/mtd/composite/Kconfig
> @@ -0,0 +1,12 @@
> +menu "Composite MTD device drivers"
> +	depends on MTD!=n
> +
> +config MTD_VIRT_CONCAT
> +	tristate "Virtual concat MTD device"
> +	help
> +	  This driver allows creation of a virtual MTD concat device, which
> +	  concatenates multiple underlying MTD devices to a single device.
> +	  This is required by some SoC boards where multiple memory banks are
> +	  used as one device with partitions spanning across device boundaries.
> +
> +endmenu
> diff --git a/drivers/mtd/composite/Makefile b/drivers/mtd/composite/Makefile
> new file mode 100644
> index 000000000000..7f4bdeee0e0a
> --- /dev/null
> +++ b/drivers/mtd/composite/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# linux/drivers/mtd/composite/Makefile
> +#
> +
> +obj-$(CONFIG_MTD_VIRT_CONCAT)   += virt_concat.o
> +
> diff --git a/drivers/mtd/composite/virt_concat.c b/drivers/mtd/composite/virt_concat.c
> new file mode 100644
> index 000000000000..239c7cdd8bca
> --- /dev/null
> +++ b/drivers/mtd/composite/virt_concat.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Virtual concat MTD device driver
> + *
> + * Copyright (C) 2018 Bernhard Frauendienst
> + * Author: Bernhard Frauendienst, kernel@nospam.obeliks.de
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

I don't think you need this paragraph now thanks to the SPDX tag.

> + */

Space here

> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/mtd/concat.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +

Please document this structure (with kernel doc headers would be nice)

> +struct of_virt_concat {
> +	struct mtd_info	 *cmtd;

Extra space             ^

> +	int num_devices;
> +	struct mtd_info	 **devices;

Ditto                   ^

> +};
> +
> +static int virt_concat_remove(struct platform_device *pdev)
> +{
> +	struct of_virt_concat *info;
> +	int i;
> +
> +	info = platform_get_drvdata(pdev);
> +	if (!info)
> +		return 0;
> +	platform_set_drvdata(pdev, NULL);

Is this really useful?

> +
> +	if (info->cmtd) {
> +		mtd_device_unregister(info->cmtd);
> +		mtd_concat_destroy(info->cmtd);
> +	}
> +
> +	if (info->devices) {
> +		for (i = 0; i < info->num_devices; i++)
> +			put_mtd_device(info->devices[i]);
> +
> +		kfree(info->devices);
> +	}
> +
> +	return 0;
> +}
> +
> +static int virt_concat_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct of_phandle_iterator it;
> +	int err, count;

I would put this...

> +	struct of_virt_concat *info;
> +	struct mtd_info *mtd;

...here

> +
> +	count = of_count_phandle_with_args(node, "devices", NULL);
> +	if (count <= 0)
> +		return -EINVAL;
> +
> +	info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +	err = -ENOMEM;
> +	info->devices = kcalloc(count, sizeof(*(info->devices)), GFP_KERNEL);

What's the reason for not using demv_kcalloc()?

> +	if (!info->devices)
> +		goto err_remove;
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	of_for_each_phandle(&it, err, node, "devices", NULL, 0) {
> +		mtd = get_mtd_device_by_node(it.node);
> +		if (IS_ERR(mtd)) {
> +			of_node_put(it.node);
> +			err = -EPROBE_DEFER;
> +			goto err_remove;
> +		}
> +
> +		info->devices[info->num_devices++] = mtd;
> +	}
> +
> +	err = -ENXIO;
> +	info->cmtd = mtd_concat_create(info->devices, info->num_devices,
> +		dev_name(&pdev->dev));

Indentation should go            there ^

> +	if (!info->cmtd)

I think we usually set err = 0 at the top and in the error path err =
-ENXIO. But that's just a matter of taste.

> +		goto err_remove;
> +
> +	info->cmtd->dev.parent = &pdev->dev;
> +	mtd_set_of_node(info->cmtd, node);
> +	mtd_device_register(info->cmtd, NULL, 0);
> +
> +	return 0;
> +
> +err_remove:
> +	virt_concat_remove(pdev);
> +
> +	return err;
> +}
> +
> +static const struct of_device_id virt_concat_of_match[] = {
> +	{ .compatible = "mtd-concat", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, virt_concat_of_match);
> +
> +static struct platform_driver virt_concat_driver = {
> +	.probe = virt_concat_probe,
> +	.remove = virt_concat_remove,
> +	.driver	 = {
> +		.name   = "virt-mtdconcat",
> +		.of_match_table = virt_concat_of_match,
> +	},
> +};
> +
> +module_platform_driver(virt_concat_driver);
> +
> +MODULE_LICENSE("GPL");

This does not match the license pointed in the header.

> +MODULE_AUTHOR("Bernhard Frauendienst <kernel@nospam.obeliks.de>");
> +MODULE_DESCRIPTION("Virtual concat MTD device driver");

Thanks,
Miquèl

Re: [PATCH 3/3] mtd: mtdconcat: add dt driver for concat devices

[Bernhard Frauendienst]

Hi Miquel,

thanks for your review, I've applied most of your suggested changes, 
except:

On 06/09/18 20:47, Miquel Raynal wrote:
>
>> +static int virt_concat_remove(struct platform_device *pdev)
>> +{
>> +	struct of_virt_concat *info;
>> +	int i;
>> +
>> +	info = platform_get_drvdata(pdev);
>> +	if (!info)
>> +		return 0;
>> +	platform_set_drvdata(pdev, NULL);
> Is this really useful?
I believe this is indeed useful, as `virt_concat_remove()` is also 
called as cleanup in error cases of `virt_concat_probe()`. Then it seems 
to be common practice to unset the driver data from the device.

Am I mistaken here?

Best regards
Bernhard

Re: [PATCH 3/3] mtd: mtdconcat: add dt driver for concat devices

[Miquel Raynal]

Hi Bernhard,

Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote on Fri, 7 Sep
2018 00:54:01 +0200:

> Hi Miquel,
> 
> thanks for your review, I've applied most of your suggested changes, except:
> 
> On 06/09/18 20:47, Miquel Raynal wrote:
> >  
> >> +static int virt_concat_remove(struct platform_device *pdev)
> >> +{
> >> +	struct of_virt_concat *info;
> >> +	int i;
> >> +
> >> +	info = platform_get_drvdata(pdev);
> >> +	if (!info)
> >> +		return 0;
> >> +	platform_set_drvdata(pdev, NULL);  
> > Is this really useful?  
> I believe this is indeed useful, as `virt_concat_remove()` is also called as cleanup in error cases of `virt_concat_probe()`. Then it seems to be common practice to unset the driver data from the device.
> 
> Am I mistaken here?

That's fine for me. Perhaps you can explain it with a comment.

I'm pretty sure there will be other "deeper" reviews coming on the
series.

Thanks,
Miquèl

Re: [PATCH 0/3] Virtual MTD concat device driver

[Boris Brezillon]

Hi Bernhard,

On Thu,  6 Sep 2018 18:14:10 +0200
Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:

> Hi everybody,
> 
> when porting my router board from a mach-file based OpenWRT target to a
> device-tree based target, I found that there is no generic way to create
> a mtd_concat device from within the dts. The following patches attempt
> to provide that possibility.

There's a fundamental issue with this patch series: DT is supposed to
represent the hardware, not how you want to use it, and clearly, your
concatenated MTD dev is a virtual device, not a real one, so I doubt it
will pass DT maintainers' review.

> 
> According to [1], patches should be provided in a git tree if possible,
> but to me it was a bit unclear whether repositories outside of
> git.infradead.org are also acceptable. If this is the case, the changes
> can be pulled from branch 'mtd_concat' at [2] (web view at [3]. The
> commits are based on mtd/next even though the repository says forked
> from torvalds/linux). In addition I'm sending the patches to the mailing
> list.
> 
> Thanks for your feedback!
> 
> Best regards
> Bernhard Frauendienst
> 
> PS: This is my first submission to this mailing list and the linux
> kernel in general. I've tried to meet all requirements, but please
> let me know if I have made any rookie mistakes.

Congrats (and welcome)! From a process PoV you did a good job. You
just miss a commit message in patch 2, and you forgot to Cc the DT
maintainers, the DT ML and the MTD maintainers. Other than that, the
coding style looks good, and the code itself looks clean.

Regards,

Boris

> 
> [1] http://www.linux-mtd.infradead.org/source.html
> [2] https://github.com/oxc/linux.git
> [3] https://github.com/oxc/linux/commits/mtd_concat
> 
> 
> Bernhard Frauendienst (3):
>   mtd: core: add get_mtd_device_by_node
>   dt-bindings: add bindings for mtd-concat devices
>   mtd: mtdconcat: add dt driver for concat devices
> 
>  .../devicetree/bindings/mtd/mtd-concat.txt    |  21 +++
>  drivers/mtd/Kconfig                           |   2 +
>  drivers/mtd/Makefile                          |   3 +
>  drivers/mtd/composite/Kconfig                 |  12 ++
>  drivers/mtd/composite/Makefile                |   7 +
>  drivers/mtd/composite/virt_concat.c           | 123 ++++++++++++++++++
>  drivers/mtd/mtdcore.c                         |  39 ++++++
>  include/linux/mtd/mtd.h                       |   2 +
>  8 files changed, 209 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/mtd-concat.txt
>  create mode 100644 drivers/mtd/composite/Kconfig
>  create mode 100644 drivers/mtd/composite/Makefile
>  create mode 100644 drivers/mtd/composite/virt_concat.c
> 

Re: [PATCH 0/3] Virtual MTD concat device driver

[Boris Brezillon]

On Thu, 6 Sep 2018 22:59:49 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Bernhard,
> 
> On Thu,  6 Sep 2018 18:14:10 +0200
> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
> 
> > Hi everybody,
> > 
> > when porting my router board from a mach-file based OpenWRT target to a
> > device-tree based target, I found that there is no generic way to create
> > a mtd_concat device from within the dts. The following patches attempt
> > to provide that possibility.  
> 
> There's a fundamental issue with this patch series: DT is supposed to
> represent the hardware, not how you want to use it, and clearly, your
> concatenated MTD dev is a virtual device, not a real one, so I doubt it
> will pass DT maintainers' review.

BTW, did you consider doing that through the command line?

Re: [PATCH 0/3] Virtual MTD concat device driver

[Bernhard Frauendienst]

Hi Boris

(sorry for the duplicate reply, the message to the list bounced because 
I had my mailer configured wrong)

On 06/09/18 23:03, Boris Brezillon wrote:
> On Thu, 6 Sep 2018 22:59:49 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>
>> Hi Bernhard,
>>
>> On Thu,  6 Sep 2018 18:14:10 +0200
>> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
>>
>>> Hi everybody,
>>>
>>> when porting my router board from a mach-file based OpenWRT target to a
>>> device-tree based target, I found that there is no generic way to create
>>> a mtd_concat device from within the dts. The following patches attempt
>>> to provide that possibility.
>> There's a fundamental issue with this patch series: DT is supposed to
>> represent the hardware, not how you want to use it, and clearly, your
>> concatenated MTD dev is a virtual device, not a real one, so I doubt it
>> will pass DT maintainers' review.
> BTW, did you consider doing that through the command line?

Would you mind elaborating on that?

The biggest problem that I see is that I have to specify the fixed 
partitions somewhere, and I would really like to keep that in the dts 
file, where they would also be if it was only a single chip.

Best Regards
Bernhard

Re: [PATCH 0/3] Virtual MTD concat device driver

[Boris Brezillon]

On Thu, 6 Sep 2018 23:16:01 +0200
Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:

> Hi Boris,
> 
> On 06/09/18 23:03, Boris Brezillon wrote:
> > On Thu, 6 Sep 2018 22:59:49 +0200
> > Boris Brezillon <boris.brezillon@bootlin.com> wrote:
> >  
> >> Hi Bernhard,
> >>
> >> On Thu,  6 Sep 2018 18:14:10 +0200
> >> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
> >>  
> >>> Hi everybody,
> >>>
> >>> when porting my router board from a mach-file based OpenWRT target to a
> >>> device-tree based target, I found that there is no generic way to create
> >>> a mtd_concat device from within the dts. The following patches attempt
> >>> to provide that possibility.  
> >> There's a fundamental issue with this patch series: DT is supposed to
> >> represent the hardware, not how you want to use it, and clearly, your
> >> concatenated MTD dev is a virtual device, not a real one, so I doubt it
> >> will pass DT maintainers' review.  
> > BTW, did you consider doing that through the command line?  
> 
> Would you mind elaborating on that?

Something like:

mtdconcat=<virtdev1>=<realdev1>,<realdev2>;<virtdev2>=<realdev3>,<realdev4>

> 
> The biggest problem that I see is that I have to specify the fixed 
> partitions somewhere, and I would really like to keep that in the dts 
> file, where they would also be if it was only a single chip.

Send a v2, add the DT maintainers+ML in Cc, and you'll see what
happens.

Re: [PATCH 0/3] Virtual MTD concat device driver

[Bernhard Frauendienst]

On 06/09/18 23:23, Boris Brezillon wrote:
> On Thu, 6 Sep 2018 23:16:01 +0200
> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
>
>> Hi Boris,
>>
>> On 06/09/18 23:03, Boris Brezillon wrote:
>>> On Thu, 6 Sep 2018 22:59:49 +0200
>>> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>>>   
>>>> Hi Bernhard,
>>>>
>>>> On Thu,  6 Sep 2018 18:14:10 +0200
>>>> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
>>>>   
>>>>> Hi everybody,
>>>>>
>>>>> when porting my router board from a mach-file based OpenWRT target to a
>>>>> device-tree based target, I found that there is no generic way to create
>>>>> a mtd_concat device from within the dts. The following patches attempt
>>>>> to provide that possibility.
>>>> There's a fundamental issue with this patch series: DT is supposed to
>>>> represent the hardware, not how you want to use it, and clearly, your
>>>> concatenated MTD dev is a virtual device, not a real one, so I doubt it
>>>> will pass DT maintainers' review.
>>> BTW, did you consider doing that through the command line?
>> Would you mind elaborating on that?
> Something like:
>
> mtdconcat=<virtdev1>=<realdev1>,<realdev2>;<virtdev2>=<realdev3>,<realdev4>
>
>> The biggest problem that I see is that I have to specify the fixed
>> partitions somewhere, and I would really like to keep that in the dts
>> file, where they would also be if it was only a single chip.
> Send a v2, add the DT maintainers+ML in Cc, and you'll see what
> happens.
I will do that. Thank you for your review and your input!

Re: [PATCH 0/3] Virtual MTD concat device driver

[Boris Brezillon]

On Thu,  6 Sep 2018 18:14:10 +0200
Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:

> Hi everybody,
> 
> when porting my router board from a mach-file based OpenWRT target to a
> device-tree based target, I found that there is no generic way to create
> a mtd_concat device from within the dts. The following patches attempt
> to provide that possibility.

May I ask what kind of devices you're trying to concatenate? Are they
of the same type? And finally, I'd like to understand the use case.

Re: [PATCH 0/3] Virtual MTD concat device driver

[Bernhard Frauendienst]

On 06/09/18 23:11, Boris Brezillon wrote:
> On Thu,  6 Sep 2018 18:14:10 +0200
> Bernhard Frauendienst <kernel@nospam.obeliks.de> wrote:
>
>> Hi everybody,
>>
>> when porting my router board from a mach-file based OpenWRT target to a
>> device-tree based target, I found that there is no generic way to create
>> a mtd_concat device from within the dts. The following patches attempt
>> to provide that possibility.
> May I ask what kind of devices you're trying to concatenate? Are they
> of the same type? And finally, I'd like to understand the use case.

Absolutely. Specifically, I'm working on specifying the device tree for 
the Buffalo WZR-HP-AG300H ([1], [2]). For persistent memory, it uses two 
Winbond 25Q128BVFG 16MiB chips and concatenates them to a 32MiB device.

I'm pretty sure the original firmware did this as well, but it's 
definitely done like this in the current OpenWRT implementation (using 
mach files).
The manufacturer made some other questionable choices (like attaching 
USB leds to the WiFi controller's gpio port), but apart from further 
Buffalo devices, there are also devices from other vendors that have a 
similar setup (e.g. ALFA Network AP120C, and Qihoo C301).

The current draft of the new dts file is here [3] (where the compatible 
line is still called virtual,mtd-concat, before I renamed it). As you 
can see, one of the partitions is crossing the device boundary, so I 
cannot simply distribute across the chips.

Regards
Bernhard

[1] https://wikidevi.com/wiki/Buffalo_WZR-HP-AG300H
[2] https://openwrt.org/toh/buffalo/wzr-hp-ag300h
[3] 
https://github.com/oxc/openwrt/blob/wzr-hp-ag300h/target/linux/ath79/dts/ar7161_buffalo_wzr-hp-ag300h.dts#L140