Thread (2 messages) 2 messages, 1 author, 2017-03-31

Re: [PATCH 1/2] mtd: move code reading DT specified part probes to the common place

From: Boris Brezillon <hidden>
Date: 2017-03-31 07:42:13 

Hi Rafal,

On Thu, 30 Mar 2017 23:53:31 +0200
Rafał Miłecki [off-list ref] wrote:

From: Rafał Miłecki <redacted>

Handling (creating) partitions for flash devices requires using a proper
driver that will read partition table (out of somewhere). We can't
simply try all existing drivers one by one, so MTD subsystem allows
drivers to specify a list of applicable part probes.

So far physmap_of was the only driver with support for linux,part-probe
DT property. Other ones had list or probes hardcoded which wasn't making
them really flexible. It prevented using common flash drivers on
platforms that required some specific partition table access.
I agree that having each flash device driver specify the set of
partition parsers it supports is a bad idea (most of the time,
partition table format are devicetype agnostic). On the other hand, I'm
not a big fan of this property, and I would prefer a solution where all
parsers are tested on each MTD device.
But testing all parsers sequentially is not a perfect solution either
because it increases boot-time and you can't really define the order in
which partition parsers are tested (which means that if you have 2
different partition table in 2 different format, you can't choose the
one that has precedence on the other).

I guess I can live with this "linux,part-probe" property, even if,
as the names implies, it's not really describing hardware, and as
such, does not have its place in DT :-P.

This commit moves support for mentioned DT property to the common place
so it can be reused by other drivers.
This property does not seem to be documented. Can you add a patch
documenting it in Documentation/devicetree/bindings/mtd/common.txt and
Cc the DT maintainers. Only then we'll see if this property is
acceptable for them.

Signed-off-by: Rafał Miłecki <redacted>
---
This patch is based on top of
[PATCH] mtd: physmap_of: use OF helpers for reading strings
---
 drivers/mtd/maps/physmap_of.c | 39 ++++++---------------------------------
 drivers/of/Kconfig            |  6 ++++++
 drivers/of/Makefile           |  1 +
 drivers/of/of_mtd.c           | 30 ++++++++++++++++++++++++++++++
 include/linux/of_mtd.h        | 25 +++++++++++++++++++++++++
No please, don't add these files back. If you need specific OF helpers
for the MTD subsystem, put them in drivers/mtd/mtdcore.c or
drivers/mtd/of.c if you think it really deserves a dedicated file (but
given the number of lines you add, I'm not sure it's the case).
quoted hunk
 5 files changed, 68 insertions(+), 33 deletions(-)
 create mode 100644 drivers/of/of_mtd.c
 create mode 100644 include/linux/of_mtd.h

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 62fa6836f218..fa54c07eb118 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -22,6 +22,7 @@
 #include <linux/mtd/concat.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_mtd.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 #include "physmap_of_gemini.h"
@@ -114,33 +115,6 @@ static struct mtd_info *obsolete_probe(struct platform_device *dev,
 static const char * const part_probe_types_def[] = {
 	"cmdlinepart", "RedBoot", "ofpart", "ofoldpart", NULL };
 
-static const char * const *of_get_probes(struct device_node *dp)
-{
-	const char **res;
-	int count;
-
-	count = of_property_count_strings(dp, "linux,part-probe");
-	if (count < 0)
-		return part_probe_types_def;
-
-	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
-	if (!res)
-		return NULL;
-
-	count = of_property_read_string_array(dp, "linux,part-probe", res,
-					      count);
-	if (count < 0)
-		return NULL;
-
-	return res;
-}
-
-static void of_free_probes(const char * const *probes)
-{
-	if (probes != part_probe_types_def)
-		kfree(probes);
-}
-
 static const struct of_device_id of_flash_match[];
 static int of_flash_probe(struct platform_device *dev)
 {
@@ -310,14 +284,13 @@ static int of_flash_probe(struct platform_device *dev)
 
 	info->cmtd->dev.parent = &dev->dev;
 	mtd_set_of_node(info->cmtd, dp);
-	part_probe_types = of_get_probes(dp);
-	if (!part_probe_types) {
-		err = -ENOMEM;
-		goto err_out;
-	}
+	part_probe_types = of_mtd_get_probes(dp);
+	if (!part_probe_types)
+		part_probe_types = part_probe_types_def;
Let's automate that a bit. The OF node is attached to the MTD device
when you call mtd_set_of_node(), so you have everything you need to
extract the partition parser list in mtd_device_parse_register().
If you do that, all MTD drivers would gain "linux,part-probe" support
for free.
quoted hunk
 	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
+	if (part_probe_types != part_probe_types_def)
+		of_mtd_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index ba7b034b2b91..18ac835a1ce4 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -78,6 +78,12 @@ config OF_MDIO
 	help
 	  OpenFirmware MDIO bus (Ethernet PHY) accessors
 
+config OF_MTD
+	def_tristate MTD
+	depends on MTD
+	help
+	  OpenFirmware MTD accessors
+
 config OF_PCI
 	def_tristate PCI
 	depends on PCI
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index d7efd9d458aa..965c2a691337 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_OF_IRQ)    += irq.o
 obj-$(CONFIG_OF_NET)	+= of_net.o
 obj-$(CONFIG_OF_UNITTEST) += unittest.o
 obj-$(CONFIG_OF_MDIO)	+= of_mdio.o
+obj-$(CONFIG_OF_MTD)	+= of_mtd.o
 obj-$(CONFIG_OF_PCI)	+= of_pci.o
 obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
 obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
diff --git a/drivers/of/of_mtd.c b/drivers/of/of_mtd.c
new file mode 100644
index 000000000000..ff851d7742d5
--- /dev/null
+++ b/drivers/of/of_mtd.c
@@ -0,0 +1,30 @@
+/*
+ * 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.
+ */
+
+#include <linux/of.h>
+#include <linux/of_mtd.h>
+
+const char * const *of_mtd_get_probes(struct device_node *np)
+{
+	const char **res;
+	int count;
+
+	count = of_property_count_strings(np, "linux,part-probe");
+	if (count < 0)
+		return NULL;
+
+	res = kzalloc((count + 1) * sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return NULL;
+
+	count = of_property_read_string_array(np, "linux,part-probe", res,
+					      count);
+	if (count < 0)
+		return NULL;
+
+	return res;
+}
+EXPORT_SYMBOL(of_mtd_get_probes);
diff --git a/include/linux/of_mtd.h b/include/linux/of_mtd.h
new file mode 100644
index 000000000000..d55f4aa684c5
--- /dev/null
+++ b/include/linux/of_mtd.h
@@ -0,0 +1,25 @@
+#ifndef __OF_MTD_H
+#define __OF_MTD_H
+
+#include <linux/slab.h>
+
+struct device_node;
+
+#ifdef CONFIG_OF_MTD
+const char * const *of_mtd_get_probes(struct device_node *np);
+static inline void of_mtd_free_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+#else
+const char * const *of_mtd_get_probes(struct device_node *np)
+{
+	return NULL;
+}
+
+static inline void of_mtd_free_probes(const char * const *probes)
+{
+}
+#endif
+
+#endif
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Related discussions
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help