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

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

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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.

This commit moves support for mentioned DT property to the common place
so it can be reused by other drivers.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
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 +++++++++++++++++++++++++
 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;
 	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
-- 
2.11.0

--
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

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

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

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.

This commit moves support for mentioned DT property to the common place
so it can be reused by other drivers.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
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 +++++++++++++++++++++++++
 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;
 	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
-- 
2.11.0

[PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

--
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

[PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

Re: [PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Marek Vasut]

On 03/30/2017 11:53 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Support for this property has been introduced in 2010 with commit
> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> parition probe") but it was never documented. Fix this by adding a
> proper description and example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
> index fc068b923d7a..1ada70e718b2 100644
> --- a/Documentation/devicetree/bindings/mtd/common.txt
> +++ b/Documentation/devicetree/bindings/mtd/common.txt
> @@ -6,10 +6,17 @@ Optional properties:
>    controller based name) in order to ease flash device identification
>    and/or describe what they are used for.
>  
> +- linux,part-probe: if present, this property should contain a list of strings
> +  with partition probes to be used for the flash device. A role of partition
> +  probe (parser) is to read/construct partition table and register found
> +  partitions. Getting partition table may be platform or device specific so
> +  various devices may use various Linux drivers for this purpose.

Why don't you just have partition not within partition node ? Then you
won't need this nonsense ...

>  Example:
>  
>  	flash@0 {
>  		label = "System-firmware";
> +		linux,part-probe = "cmdlinepart", "ofpart";
>  
>  		/* flash type specific properties */
>  	};
> 


-- 
Best regards,
Marek Vasut

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Marek Vasut]

On 03/30/2017 11:53 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Support for this property has been introduced in 2010 with commit
> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
> parition probe") but it was never documented. Fix this by adding a
> proper description and example.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
> index fc068b923d7a..1ada70e718b2 100644
> --- a/Documentation/devicetree/bindings/mtd/common.txt
> +++ b/Documentation/devicetree/bindings/mtd/common.txt
> @@ -6,10 +6,17 @@ Optional properties:
>    controller based name) in order to ease flash device identification
>    and/or describe what they are used for.
>  
> +- linux,part-probe: if present, this property should contain a list of strings
> +  with partition probes to be used for the flash device. A role of partition
> +  probe (parser) is to read/construct partition table and register found
> +  partitions. Getting partition table may be platform or device specific so
> +  various devices may use various Linux drivers for this purpose.

Why don't you just have partition not within partition node ? Then you
won't need this nonsense ...

>  Example:
>  
>  	flash@0 {
>  		label = "System-firmware";
> +		linux,part-probe = "cmdlinepart", "ofpart";
>  
>  		/* flash type specific properties */
>  	};
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

On 03/31/2017 01:26 AM, Marek Vasut wrote:
> On 03/30/2017 11:53 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> Support for this property has been introduced in 2010 with commit
>> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
>> parition probe") but it was never documented. Fix this by adding a
>> proper description and example.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
>> index fc068b923d7a..1ada70e718b2 100644
>> --- a/Documentation/devicetree/bindings/mtd/common.txt
>> +++ b/Documentation/devicetree/bindings/mtd/common.txt
>> @@ -6,10 +6,17 @@ Optional properties:
>>    controller based name) in order to ease flash device identification
>>    and/or describe what they are used for.
>>
>> +- linux,part-probe: if present, this property should contain a list of strings
>> +  with partition probes to be used for the flash device. A role of partition
>> +  probe (parser) is to read/construct partition table and register found
>> +  partitions. Getting partition table may be platform or device specific so
>> +  various devices may use various Linux drivers for this purpose.
>
> Why don't you just have partition not within partition node ? Then you
> won't need this nonsense ...

Can you stop this negative approach ("nonsense") for every single f* thing I
submit? It doesn't help and makes people not want to work with upstream mtd.

The only partitioner with support for partitions node is "fixed-partitions".
Take a look at original linux,part-probe usage:
arch/arm64/boot/dts/arm/juno-motherboard.dtsi
It's used to specify "afs" partitioner. It's dynamic. You can't replace it with
fixed partitions.

So this property is needed to specify platform/device specific partitioner that
should be used with some standard flash driver (like physmap_of or m25p80).

I was hoping following part of documentation makes is clear:
 > Getting partition table may be platform or device specific so
 > various devices may use various Linux drivers for this purpose.
--
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

Re: [PATCH 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

On 03/31/2017 01:26 AM, Marek Vasut wrote:
> On 03/30/2017 11:53 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Support for this property has been introduced in 2010 with commit
>> 9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
>> parition probe") but it was never documented. Fix this by adding a
>> proper description and example.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
>> index fc068b923d7a..1ada70e718b2 100644
>> --- a/Documentation/devicetree/bindings/mtd/common.txt
>> +++ b/Documentation/devicetree/bindings/mtd/common.txt
>> @@ -6,10 +6,17 @@ Optional properties:
>>    controller based name) in order to ease flash device identification
>>    and/or describe what they are used for.
>>
>> +- linux,part-probe: if present, this property should contain a list of strings
>> +  with partition probes to be used for the flash device. A role of partition
>> +  probe (parser) is to read/construct partition table and register found
>> +  partitions. Getting partition table may be platform or device specific so
>> +  various devices may use various Linux drivers for this purpose.
>
> Why don't you just have partition not within partition node ? Then you
> won't need this nonsense ...

Can you stop this negative approach ("nonsense") for every single f* thing I
submit? It doesn't help and makes people not want to work with upstream mtd.

The only partitioner with support for partitions node is "fixed-partitions".
Take a look at original linux,part-probe usage:
arch/arm64/boot/dts/arm/juno-motherboard.dtsi
It's used to specify "afs" partitioner. It's dynamic. You can't replace it with
fixed partitions.

So this property is needed to specify platform/device specific partitioner that
should be used with some standard flash driver (like physmap_of or m25p80).

I was hoping following part of documentation makes is clear:
 > Getting partition table may be platform or device specific so
 > various devices may use various Linux drivers for this purpose.

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

[Boris Brezillon]

Hi Rafal,

On Thu, 30 Mar 2017 23:53:31 +0200
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> 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 <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
> 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).

>  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.

>  	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

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

[Boris Brezillon]

Hi Rafal,

On Thu, 30 Mar 2017 23:53:31 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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 <rafal@milecki.pl>
> ---
> 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).

>  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.

>  	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

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

[Boris Brezillon]

On Fri, 31 Mar 2017 09:42:13 +0200
Boris Brezillon <boris.brezillon-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> Hi Rafal,
> 
> On Thu, 30 Mar 2017 23:53:31 +0200
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 
> > From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> > 
> > 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.

Please ignore this comment, it's already done in patch 2 :).

> Only then we'll see if this property is
> acceptable for them.
> 
--
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

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

[Boris Brezillon]

On Fri, 31 Mar 2017 09:42:13 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Hi Rafal,
> 
> On Thu, 30 Mar 2017 23:53:31 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
> 
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > 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.

Please ignore this comment, it's already done in patch 2 :).

> Only then we'll see if this property is
> acceptable for them.
> 

[PATCH V2 1/2] mtd: move code reading DT specified part probes to the core

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

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.

This commit moves support for mentioned DT property to the MTD core
file. Thanks to calling it on device parse registration (as suggested by
Boris) all drivers gain support for it for free.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
 drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 62fa6836f218..49dbb7235848 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -114,37 +114,9 @@ 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)
 {
-	const char * const *part_probe_types;
 	const struct of_device_id *match;
 	struct device_node *dp = dev->dev.of_node;
 	struct resource res;
@@ -310,14 +282,8 @@ 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;
-	}
-	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
+	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9dedd1062..a7ab0a24ba1e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	}
 }
 
+static const char * const *mtd_of_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;
+}
+
+static inline void mtd_of_free_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	const char * const *part_probe_types;
 	struct mtd_partitions parsed;
 	int ret;
 
 	mtd_set_dev_defaults(mtd);
 
+	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
+	if (!part_probe_types)
+		part_probe_types = types;
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
@@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		memset(&parsed, 0, sizeof(parsed));
 	}
 
+	if (part_probe_types != types)
+		mtd_of_free_probes(part_probe_types);
+
 	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
-- 
2.11.0

--
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

[PATCH V2 1/2] mtd: move code reading DT specified part probes to the core

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

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.

This commit moves support for mentioned DT property to the MTD core
file. Thanks to calling it on device parse registration (as suggested by
Boris) all drivers gain support for it for free.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
 drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 35 deletions(-)

diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
index 62fa6836f218..49dbb7235848 100644
--- a/drivers/mtd/maps/physmap_of.c
+++ b/drivers/mtd/maps/physmap_of.c
@@ -114,37 +114,9 @@ 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)
 {
-	const char * const *part_probe_types;
 	const struct of_device_id *match;
 	struct device_node *dp = dev->dev.of_node;
 	struct resource res;
@@ -310,14 +282,8 @@ 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;
-	}
-	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
+	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
 			NULL, 0);
-	of_free_probes(part_probe_types);
 
 	kfree(mtd_list);
 
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index 66a9dedd1062..a7ab0a24ba1e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
 	}
 }
 
+static const char * const *mtd_of_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;
+}
+
+static inline void mtd_of_free_probes(const char * const *probes)
+{
+	kfree(probes);
+}
+
 /**
  * mtd_device_parse_register - parse partitions and register an MTD device.
  *
@@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 			      const struct mtd_partition *parts,
 			      int nr_parts)
 {
+	const char * const *part_probe_types;
 	struct mtd_partitions parsed;
 	int ret;
 
 	mtd_set_dev_defaults(mtd);
 
+	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
+	if (!part_probe_types)
+		part_probe_types = types;
+
 	memset(&parsed, 0, sizeof(parsed));
 
 	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
@@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
 		memset(&parsed, 0, sizeof(parsed));
 	}
 
+	if (part_probe_types != types)
+		mtd_of_free_probes(part_probe_types);
+
 	ret = mtd_add_device_partitions(mtd, &parsed);
 	if (ret)
 		goto out;
-- 
2.11.0

[PATCH V2 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

--
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

[PATCH V2 2/2] dt-bindings: mtd: document linux,part-probe property

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Support for this property has been introduced in 2010 with commit
9d5da3a9b849 ("mtd: extend physmap_of to let the device tree specify the
parition probe") but it was never documented. Fix this by adding a
proper description and example.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 Documentation/devicetree/bindings/mtd/common.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/common.txt b/Documentation/devicetree/bindings/mtd/common.txt
index fc068b923d7a..1ada70e718b2 100644
--- a/Documentation/devicetree/bindings/mtd/common.txt
+++ b/Documentation/devicetree/bindings/mtd/common.txt
@@ -6,10 +6,17 @@ Optional properties:
   controller based name) in order to ease flash device identification
   and/or describe what they are used for.
 
+- linux,part-probe: if present, this property should contain a list of strings
+  with partition probes to be used for the flash device. A role of partition
+  probe (parser) is to read/construct partition table and register found
+  partitions. Getting partition table may be platform or device specific so
+  various devices may use various Linux drivers for this purpose.
+
 Example:
 
 	flash@0 {
 		label = "System-firmware";
+		linux,part-probe = "cmdlinepart", "ofpart";
 
 		/* flash type specific properties */
 	};
-- 
2.11.0

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

[Boris Brezillon]

On Fri, 31 Mar 2017 11:30:12 +0200
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> 
> 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.
> 
> This commit moves support for mentioned DT property to the MTD core
> file. Thanks to calling it on device parse registration (as suggested by
> Boris) all drivers gain support for it for free.
> 
> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> ---
>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++

Maybe you should split the patch:
1/ add core infrastructure
2/ remove open-coded version in drivers/mtd/maps/physmap_of.c

BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
for reading strings" patch is really useful, since you move to the
common infrastructure here.
By following my suggestion you get rid of the dependency you have
between this series and patch "mtd: physmap_of: use OF helpers for
reading strings".

>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 62fa6836f218..49dbb7235848 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -114,37 +114,9 @@ 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)
>  {
> -	const char * const *part_probe_types;
>  	const struct of_device_id *match;
>  	struct device_node *dp = dev->dev.of_node;
>  	struct resource res;
> @@ -310,14 +282,8 @@ 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;
> -	}
> -	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
> +	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
>  			NULL, 0);
> -	of_free_probes(part_probe_types);
>  
>  	kfree(mtd_list);
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9dedd1062..a7ab0a24ba1e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	}
>  }
>  
> +static const char * const *mtd_of_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;
> +}
> +
> +static inline void mtd_of_free_probes(const char * const *probes)
> +{
> +	kfree(probes);
> +}
> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	const char * const *part_probe_types;
>  	struct mtd_partitions parsed;
>  	int ret;
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
> +	if (!part_probe_types)
> +		part_probe_types = types;
> +
>  	memset(&parsed, 0, sizeof(parsed));
>  
>  	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> +	if (part_probe_types != types)
> +		mtd_of_free_probes(part_probe_types);
> +
>  	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;

--
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

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

[Boris Brezillon]

On Fri, 31 Mar 2017 11:30:12 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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.
> 
> This commit moves support for mentioned DT property to the MTD core
> file. Thanks to calling it on device parse registration (as suggested by
> Boris) all drivers gain support for it for free.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++

Maybe you should split the patch:
1/ add core infrastructure
2/ remove open-coded version in drivers/mtd/maps/physmap_of.c

BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
for reading strings" patch is really useful, since you move to the
common infrastructure here.
By following my suggestion you get rid of the dependency you have
between this series and patch "mtd: physmap_of: use OF helpers for
reading strings".

>  2 files changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 62fa6836f218..49dbb7235848 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -114,37 +114,9 @@ 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)
>  {
> -	const char * const *part_probe_types;
>  	const struct of_device_id *match;
>  	struct device_node *dp = dev->dev.of_node;
>  	struct resource res;
> @@ -310,14 +282,8 @@ 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;
> -	}
> -	mtd_device_parse_register(info->cmtd, part_probe_types, NULL,
> +	mtd_device_parse_register(info->cmtd, part_probe_types_def, NULL,
>  			NULL, 0);
> -	of_free_probes(part_probe_types);
>  
>  	kfree(mtd_list);
>  
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index 66a9dedd1062..a7ab0a24ba1e 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -664,6 +664,32 @@ static void mtd_set_dev_defaults(struct mtd_info *mtd)
>  	}
>  }
>  
> +static const char * const *mtd_of_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;
> +}
> +
> +static inline void mtd_of_free_probes(const char * const *probes)
> +{
> +	kfree(probes);
> +}
> +
>  /**
>   * mtd_device_parse_register - parse partitions and register an MTD device.
>   *
> @@ -698,11 +724,16 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  			      const struct mtd_partition *parts,
>  			      int nr_parts)
>  {
> +	const char * const *part_probe_types;
>  	struct mtd_partitions parsed;
>  	int ret;
>  
>  	mtd_set_dev_defaults(mtd);
>  
> +	part_probe_types = mtd_of_get_probes(mtd_get_of_node(mtd));
> +	if (!part_probe_types)
> +		part_probe_types = types;
> +
>  	memset(&parsed, 0, sizeof(parsed));
>  
>  	ret = parse_mtd_partitions(mtd, types, &parsed, parser_data);
> @@ -720,6 +751,9 @@ int mtd_device_parse_register(struct mtd_info *mtd, const char * const *types,
>  		memset(&parsed, 0, sizeof(parsed));
>  	}
>  
> +	if (part_probe_types != types)
> +		mtd_of_free_probes(part_probe_types);
> +
>  	ret = mtd_add_device_partitions(mtd, &parsed);
>  	if (ret)
>  		goto out;

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

[Rafał Miłecki]

On 03/31/2017 11:56 AM, Boris Brezillon wrote:
> On Fri, 31 Mar 2017 11:30:12 +0200
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>
>> 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.
>>
>> This commit moves support for mentioned DT property to the MTD core
>> file. Thanks to calling it on device parse registration (as suggested by
>> Boris) all drivers gain support for it for free.
>>
>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>> ---
>>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
>
> Maybe you should split the patch:
> 1/ add core infrastructure
> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c

What's the gain of that? Is this patch too complex to review properly? Will that
be useful for anyone to have it split? For me it only adds an intermediate code
duplication.


> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> for reading strings" patch is really useful, since you move to the
> common infrastructure here.
> By following my suggestion you get rid of the dependency you have
> between this series and patch "mtd: physmap_of: use OF helpers for
> reading strings".

I learned (the very hard way) MTD people can be really nitpicking so I'm
sending as simple patches as I can. I see it as the only way for someone from
OpenWrt/LEDE project to get patch through your review.

It's like with this patch: even a simple code move can be questioned. Please
drop this patchset, I'll resend it after/if I manage to get
[PATCH] mtd: physmap_of: use OF helpers for reading strings
accepted.
--
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

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

[Rafał Miłecki]

On 03/31/2017 11:56 AM, Boris Brezillon wrote:
> On Fri, 31 Mar 2017 11:30:12 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> 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.
>>
>> This commit moves support for mentioned DT property to the MTD core
>> file. Thanks to calling it on device parse registration (as suggested by
>> Boris) all drivers gain support for it for free.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
>
> Maybe you should split the patch:
> 1/ add core infrastructure
> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c

What's the gain of that? Is this patch too complex to review properly? Will that
be useful for anyone to have it split? For me it only adds an intermediate code
duplication.


> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> for reading strings" patch is really useful, since you move to the
> common infrastructure here.
> By following my suggestion you get rid of the dependency you have
> between this series and patch "mtd: physmap_of: use OF helpers for
> reading strings".

I learned (the very hard way) MTD people can be really nitpicking so I'm
sending as simple patches as I can. I see it as the only way for someone from
OpenWrt/LEDE project to get patch through your review.

It's like with this patch: even a simple code move can be questioned. Please
drop this patchset, I'll resend it after/if I manage to get
[PATCH] mtd: physmap_of: use OF helpers for reading strings
accepted.

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

[Boris Brezillon]

On Fri, 31 Mar 2017 12:12:56 +0200
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 03/31/2017 11:56 AM, Boris Brezillon wrote:
> > On Fri, 31 Mar 2017 11:30:12 +0200
> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >  
> >> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> >>
> >> 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.
> >>
> >> This commit moves support for mentioned DT property to the MTD core
> >> file. Thanks to calling it on device parse registration (as suggested by
> >> Boris) all drivers gain support for it for free.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> >> ---
> >>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
> >>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++  
> >
> > Maybe you should split the patch:
> > 1/ add core infrastructure
> > 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c  
> 
> What's the gain of that? Is this patch too complex to review properly? Will that
> be useful for anyone to have it split? For me it only adds an intermediate code
> duplication.

The gain is that we get rid of the dependency we have on patch "mtd:
physmap_of: use OF helpers for reading strings" which is not even
mentioned in your mails.

> 
> 
> > BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> > for reading strings" patch is really useful, since you move to the
> > common infrastructure here.
> > By following my suggestion you get rid of the dependency you have
> > between this series and patch "mtd: physmap_of: use OF helpers for
> > reading strings".  
> 
> I learned (the very hard way) MTD people can be really nitpicking so I'm
> sending as simple patches as I can. I see it as the only way for someone from
> OpenWrt/LEDE project to get patch through your review.

And I learned the hard way that OpenWRT/LEDE developers tend to not
listen to our advices and keep arguing on things that have been proven
to be existing because of bad decision they made at some point in the
project life. So I think we're even :-P.

> 
> It's like with this patch: even a simple code move can be questioned. Please
> drop this patchset, I'll resend it after/if I manage to get
> [PATCH] mtd: physmap_of: use OF helpers for reading strings
> accepted.

But really, what's the point of this patch? It's just a cleanup. You're
not fixing a bug or changing the behavior, and your real objective is
to get support for the linux,part-probe in the core, which will then
allow us to drop the open-coded version you have in physmap_of.c.

I don't think it deserves an intermediate patch, unless your real
objective is patchcount.

--
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

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

[Boris Brezillon]

On Fri, 31 Mar 2017 12:12:56 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> On 03/31/2017 11:56 AM, Boris Brezillon wrote:
> > On Fri, 31 Mar 2017 11:30:12 +0200
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >  
> >> From: Rafał Miłecki <rafal@milecki.pl>
> >>
> >> 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.
> >>
> >> This commit moves support for mentioned DT property to the MTD core
> >> file. Thanks to calling it on device parse registration (as suggested by
> >> Boris) all drivers gain support for it for free.
> >>
> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> >> ---
> >>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
> >>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++  
> >
> > Maybe you should split the patch:
> > 1/ add core infrastructure
> > 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c  
> 
> What's the gain of that? Is this patch too complex to review properly? Will that
> be useful for anyone to have it split? For me it only adds an intermediate code
> duplication.

The gain is that we get rid of the dependency we have on patch "mtd:
physmap_of: use OF helpers for reading strings" which is not even
mentioned in your mails.

> 
> 
> > BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> > for reading strings" patch is really useful, since you move to the
> > common infrastructure here.
> > By following my suggestion you get rid of the dependency you have
> > between this series and patch "mtd: physmap_of: use OF helpers for
> > reading strings".  
> 
> I learned (the very hard way) MTD people can be really nitpicking so I'm
> sending as simple patches as I can. I see it as the only way for someone from
> OpenWrt/LEDE project to get patch through your review.

And I learned the hard way that OpenWRT/LEDE developers tend to not
listen to our advices and keep arguing on things that have been proven
to be existing because of bad decision they made at some point in the
project life. So I think we're even :-P.

> 
> It's like with this patch: even a simple code move can be questioned. Please
> drop this patchset, I'll resend it after/if I manage to get
> [PATCH] mtd: physmap_of: use OF helpers for reading strings
> accepted.

But really, what's the point of this patch? It's just a cleanup. You're
not fixing a bug or changing the behavior, and your real objective is
to get support for the linux,part-probe in the core, which will then
allow us to drop the open-coded version you have in physmap_of.c.

I don't think it deserves an intermediate patch, unless your real
objective is patchcount.

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

[Rafał Miłecki]

On 03/31/2017 12:31 PM, Boris Brezillon wrote:
> On Fri, 31 Mar 2017 12:12:56 +0200
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On 03/31/2017 11:56 AM, Boris Brezillon wrote:
>>> On Fri, 31 Mar 2017 11:30:12 +0200
>>> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>>> From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>>
>>>> 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.
>>>>
>>>> This commit moves support for mentioned DT property to the MTD core
>>>> file. Thanks to calling it on device parse registration (as suggested by
>>>> Boris) all drivers gain support for it for free.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
>>>> ---
>>>>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>>>>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
>>>
>>> Maybe you should split the patch:
>>> 1/ add core infrastructure
>>> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c
>>
>> What's the gain of that? Is this patch too complex to review properly? Will that
>> be useful for anyone to have it split? For me it only adds an intermediate code
>> duplication.
>
> The gain is that we get rid of the dependency we have on patch "mtd:
> physmap_of: use OF helpers for reading strings" which is not even
> mentioned in your mails.

This is definitely my mistake, initially I pushed a proper comment below tear
line but have overwritten it later. Sorry!


>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
>>> for reading strings" patch is really useful, since you move to the
>>> common infrastructure here.
>>> By following my suggestion you get rid of the dependency you have
>>> between this series and patch "mtd: physmap_of: use OF helpers for
>>> reading strings".
>>
>> I learned (the very hard way) MTD people can be really nitpicking so I'm
>> sending as simple patches as I can. I see it as the only way for someone from
>> OpenWrt/LEDE project to get patch through your review.
>
> And I learned the hard way that OpenWRT/LEDE developers tend to not
> listen to our advices and keep arguing on things that have been proven
> to be existing because of bad decision they made at some point in the
> project life. So I think we're even :-P.

I wish you could sometimes forget what you've learned and review/discuss things
without all that negative approach I keep seeing.


>> It's like with this patch: even a simple code move can be questioned. Please
>> drop this patchset, I'll resend it after/if I manage to get
>> [PATCH] mtd: physmap_of: use OF helpers for reading strings
>> accepted.
>
> But really, what's the point of this patch? It's just a cleanup. You're
> not fixing a bug or changing the behavior, and your real objective is
> to get support for the linux,part-probe in the core, which will then
> allow us to drop the open-coded version you have in physmap_of.c.
>
> I don't think it deserves an intermediate patch, unless your real
> objective is patchcount.

OK, I'm going to trust that and see how easily I get can patch your way. I'll
resend combined version soon.
--
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

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

[Rafał Miłecki]

On 03/31/2017 12:31 PM, Boris Brezillon wrote:
> On Fri, 31 Mar 2017 12:12:56 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>> On 03/31/2017 11:56 AM, Boris Brezillon wrote:
>>> On Fri, 31 Mar 2017 11:30:12 +0200
>>> Rafał Miłecki <zajec5@gmail.com> wrote:
>>>
>>>> From: Rafał Miłecki <rafal@milecki.pl>
>>>>
>>>> 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.
>>>>
>>>> This commit moves support for mentioned DT property to the MTD core
>>>> file. Thanks to calling it on device parse registration (as suggested by
>>>> Boris) all drivers gain support for it for free.
>>>>
>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>>>> ---
>>>>  drivers/mtd/maps/physmap_of.c | 36 +-----------------------------------
>>>>  drivers/mtd/mtdcore.c         | 34 ++++++++++++++++++++++++++++++++++
>>>
>>> Maybe you should split the patch:
>>> 1/ add core infrastructure
>>> 2/ remove open-coded version in drivers/mtd/maps/physmap_of.c
>>
>> What's the gain of that? Is this patch too complex to review properly? Will that
>> be useful for anyone to have it split? For me it only adds an intermediate code
>> duplication.
>
> The gain is that we get rid of the dependency we have on patch "mtd:
> physmap_of: use OF helpers for reading strings" which is not even
> mentioned in your mails.

This is definitely my mistake, initially I pushed a proper comment below tear
line but have overwritten it later. Sorry!


>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
>>> for reading strings" patch is really useful, since you move to the
>>> common infrastructure here.
>>> By following my suggestion you get rid of the dependency you have
>>> between this series and patch "mtd: physmap_of: use OF helpers for
>>> reading strings".
>>
>> I learned (the very hard way) MTD people can be really nitpicking so I'm
>> sending as simple patches as I can. I see it as the only way for someone from
>> OpenWrt/LEDE project to get patch through your review.
>
> And I learned the hard way that OpenWRT/LEDE developers tend to not
> listen to our advices and keep arguing on things that have been proven
> to be existing because of bad decision they made at some point in the
> project life. So I think we're even :-P.

I wish you could sometimes forget what you've learned and review/discuss things
without all that negative approach I keep seeing.


>> It's like with this patch: even a simple code move can be questioned. Please
>> drop this patchset, I'll resend it after/if I manage to get
>> [PATCH] mtd: physmap_of: use OF helpers for reading strings
>> accepted.
>
> But really, what's the point of this patch? It's just a cleanup. You're
> not fixing a bug or changing the behavior, and your real objective is
> to get support for the linux,part-probe in the core, which will then
> allow us to drop the open-coded version you have in physmap_of.c.
>
> I don't think it deserves an intermediate patch, unless your real
> objective is patchcount.

OK, I'm going to trust that and see how easily I get can patch your way. I'll
resend combined version soon.

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

[Boris Brezillon]

Rafal,

On Fri, 31 Mar 2017 12:46:38 +0200
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> >>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> >>> for reading strings" patch is really useful, since you move to the
> >>> common infrastructure here.
> >>> By following my suggestion you get rid of the dependency you have
> >>> between this series and patch "mtd: physmap_of: use OF helpers for
> >>> reading strings".  
> >>
> >> I learned (the very hard way) MTD people can be really nitpicking so I'm
> >> sending as simple patches as I can. I see it as the only way for someone from
> >> OpenWrt/LEDE project to get patch through your review.  
> >
> > And I learned the hard way that OpenWRT/LEDE developers tend to not
> > listen to our advices and keep arguing on things that have been proven
> > to be existing because of bad decision they made at some point in the
> > project life. So I think we're even :-P.  
> 
> I wish you could sometimes forget what you've learned and review/discuss things
> without all that negative approach I keep seeing.

I try to stay objective, and if you look back at my review, you'll see
that I actually agree with most of your changes. So if one person is
taking it personally it's you, not me.

Now, regarding other contributions, like support for the TRX format, I
keep thinking that it's badly designed and should not be supported in
mainline. I clearly expressed my opinion, and I also said I wouldn't
block the patches if other MTD maintainers were okay to take them (which
is already a good thing, don't you think?). But don't expect me to say
"Youhou, let's merge this awesome feature!".

More generally, if you look back at all the contributions OpenWRT/LEDE
devs made, all uncontroversial features were merged rather quickly. For
the other ones, each time we tried to come up with alternative
solutions, but if you don't want to follow these suggestions (or at
least try them before saying it's impossible), then I think there's
nothing we can do on our side.

> 
> 
> >> It's like with this patch: even a simple code move can be questioned. Please
> >> drop this patchset, I'll resend it after/if I manage to get
> >> [PATCH] mtd: physmap_of: use OF helpers for reading strings
> >> accepted.  
> >
> > But really, what's the point of this patch? It's just a cleanup. You're
> > not fixing a bug or changing the behavior, and your real objective is
> > to get support for the linux,part-probe in the core, which will then
> > allow us to drop the open-coded version you have in physmap_of.c.
> >
> > I don't think it deserves an intermediate patch, unless your real
> > objective is patchcount.  
> 
> OK, I'm going to trust that and see how easily I get can patch your way. I'll
> resend combined version soon.

Let's wait for a DT review, since this is probably the main blocking
aspect.

Regards,

Boris
--
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

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

[Boris Brezillon]

Rafal,

On Fri, 31 Mar 2017 12:46:38 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> >>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> >>> for reading strings" patch is really useful, since you move to the
> >>> common infrastructure here.
> >>> By following my suggestion you get rid of the dependency you have
> >>> between this series and patch "mtd: physmap_of: use OF helpers for
> >>> reading strings".  
> >>
> >> I learned (the very hard way) MTD people can be really nitpicking so I'm
> >> sending as simple patches as I can. I see it as the only way for someone from
> >> OpenWrt/LEDE project to get patch through your review.  
> >
> > And I learned the hard way that OpenWRT/LEDE developers tend to not
> > listen to our advices and keep arguing on things that have been proven
> > to be existing because of bad decision they made at some point in the
> > project life. So I think we're even :-P.  
> 
> I wish you could sometimes forget what you've learned and review/discuss things
> without all that negative approach I keep seeing.

I try to stay objective, and if you look back at my review, you'll see
that I actually agree with most of your changes. So if one person is
taking it personally it's you, not me.

Now, regarding other contributions, like support for the TRX format, I
keep thinking that it's badly designed and should not be supported in
mainline. I clearly expressed my opinion, and I also said I wouldn't
block the patches if other MTD maintainers were okay to take them (which
is already a good thing, don't you think?). But don't expect me to say
"Youhou, let's merge this awesome feature!".

More generally, if you look back at all the contributions OpenWRT/LEDE
devs made, all uncontroversial features were merged rather quickly. For
the other ones, each time we tried to come up with alternative
solutions, but if you don't want to follow these suggestions (or at
least try them before saying it's impossible), then I think there's
nothing we can do on our side.

> 
> 
> >> It's like with this patch: even a simple code move can be questioned. Please
> >> drop this patchset, I'll resend it after/if I manage to get
> >> [PATCH] mtd: physmap_of: use OF helpers for reading strings
> >> accepted.  
> >
> > But really, what's the point of this patch? It's just a cleanup. You're
> > not fixing a bug or changing the behavior, and your real objective is
> > to get support for the linux,part-probe in the core, which will then
> > allow us to drop the open-coded version you have in physmap_of.c.
> >
> > I don't think it deserves an intermediate patch, unless your real
> > objective is patchcount.  
> 
> OK, I'm going to trust that and see how easily I get can patch your way. I'll
> resend combined version soon.

Let's wait for a DT review, since this is probably the main blocking
aspect.

Regards,

Boris

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

[Rafał Miłecki]

On 03/31/2017 01:41 PM, Boris Brezillon wrote:
> Rafal,
>
> On Fri, 31 Mar 2017 12:46:38 +0200
> Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
>>>>> for reading strings" patch is really useful, since you move to the
>>>>> common infrastructure here.
>>>>> By following my suggestion you get rid of the dependency you have
>>>>> between this series and patch "mtd: physmap_of: use OF helpers for
>>>>> reading strings".
>>>>
>>>> I learned (the very hard way) MTD people can be really nitpicking so I'm
>>>> sending as simple patches as I can. I see it as the only way for someone from
>>>> OpenWrt/LEDE project to get patch through your review.
>>>
>>> And I learned the hard way that OpenWRT/LEDE developers tend to not
>>> listen to our advices and keep arguing on things that have been proven
>>> to be existing because of bad decision they made at some point in the
>>> project life. So I think we're even :-P.
>>
>> I wish you could sometimes forget what you've learned and review/discuss things
>> without all that negative approach I keep seeing.
>
> I try to stay objective, and if you look back at my review, you'll see
> that I actually agree with most of your changes. So if one person is
> taking it personally it's you, not me.
>
> Now, regarding other contributions, like support for the TRX format, I
> keep thinking that it's badly designed and should not be supported in
> mainline. I clearly expressed my opinion, and I also said I wouldn't
> block the patches if other MTD maintainers were okay to take them (which
> is already a good thing, don't you think?). But don't expect me to say
> "Youhou, let's merge this awesome feature!".
>
> More generally, if you look back at all the contributions OpenWRT/LEDE
> devs made, all uncontroversial features were merged rather quickly. For
> the other ones, each time we tried to come up with alternative
> solutions, but if you don't want to follow these suggestions (or at
> least try them before saying it's impossible), then I think there's
> nothing we can do on our side.

Sounds fair from you, thanks. Please note I'm actually following your
suggestions, just recently I sent RFC init for initramfs which should handle
some of OpenWrt/LEDE hacks in user space as you told us to do this.

https://patchwork.ozlabs.org/patch/744093/


>>>> It's like with this patch: even a simple code move can be questioned. Please
>>>> drop this patchset, I'll resend it after/if I manage to get
>>>> [PATCH] mtd: physmap_of: use OF helpers for reading strings
>>>> accepted.
>>>
>>> But really, what's the point of this patch? It's just a cleanup. You're
>>> not fixing a bug or changing the behavior, and your real objective is
>>> to get support for the linux,part-probe in the core, which will then
>>> allow us to drop the open-coded version you have in physmap_of.c.
>>>
>>> I don't think it deserves an intermediate patch, unless your real
>>> objective is patchcount.
>>
>> OK, I'm going to trust that and see how easily I get can patch your way. I'll
>> resend combined version soon.
>
> Let's wait for a DT review, since this is probably the main blocking
> aspect.

OK.
--
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

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

[Rafał Miłecki]

On 03/31/2017 01:41 PM, Boris Brezillon wrote:
> Rafal,
>
> On Fri, 31 Mar 2017 12:46:38 +0200
> Rafał Miłecki <zajec5@gmail.com> wrote:
>
>>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
>>>>> for reading strings" patch is really useful, since you move to the
>>>>> common infrastructure here.
>>>>> By following my suggestion you get rid of the dependency you have
>>>>> between this series and patch "mtd: physmap_of: use OF helpers for
>>>>> reading strings".
>>>>
>>>> I learned (the very hard way) MTD people can be really nitpicking so I'm
>>>> sending as simple patches as I can. I see it as the only way for someone from
>>>> OpenWrt/LEDE project to get patch through your review.
>>>
>>> And I learned the hard way that OpenWRT/LEDE developers tend to not
>>> listen to our advices and keep arguing on things that have been proven
>>> to be existing because of bad decision they made at some point in the
>>> project life. So I think we're even :-P.
>>
>> I wish you could sometimes forget what you've learned and review/discuss things
>> without all that negative approach I keep seeing.
>
> I try to stay objective, and if you look back at my review, you'll see
> that I actually agree with most of your changes. So if one person is
> taking it personally it's you, not me.
>
> Now, regarding other contributions, like support for the TRX format, I
> keep thinking that it's badly designed and should not be supported in
> mainline. I clearly expressed my opinion, and I also said I wouldn't
> block the patches if other MTD maintainers were okay to take them (which
> is already a good thing, don't you think?). But don't expect me to say
> "Youhou, let's merge this awesome feature!".
>
> More generally, if you look back at all the contributions OpenWRT/LEDE
> devs made, all uncontroversial features were merged rather quickly. For
> the other ones, each time we tried to come up with alternative
> solutions, but if you don't want to follow these suggestions (or at
> least try them before saying it's impossible), then I think there's
> nothing we can do on our side.

Sounds fair from you, thanks. Please note I'm actually following your
suggestions, just recently I sent RFC init for initramfs which should handle
some of OpenWrt/LEDE hacks in user space as you told us to do this.

https://patchwork.ozlabs.org/patch/744093/


>>>> It's like with this patch: even a simple code move can be questioned. Please
>>>> drop this patchset, I'll resend it after/if I manage to get
>>>> [PATCH] mtd: physmap_of: use OF helpers for reading strings
>>>> accepted.
>>>
>>> But really, what's the point of this patch? It's just a cleanup. You're
>>> not fixing a bug or changing the behavior, and your real objective is
>>> to get support for the linux,part-probe in the core, which will then
>>> allow us to drop the open-coded version you have in physmap_of.c.
>>>
>>> I don't think it deserves an intermediate patch, unless your real
>>> objective is patchcount.
>>
>> OK, I'm going to trust that and see how easily I get can patch your way. I'll
>> resend combined version soon.
>
> Let's wait for a DT review, since this is probably the main blocking
> aspect.

OK.

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

[Boris Brezillon]

On Fri, 31 Mar 2017 14:23:11 +0200
Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 03/31/2017 01:41 PM, Boris Brezillon wrote:
> > Rafal,
> >
> > On Fri, 31 Mar 2017 12:46:38 +0200
> > Rafał Miłecki <zajec5-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >  
> >>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> >>>>> for reading strings" patch is really useful, since you move to the
> >>>>> common infrastructure here.
> >>>>> By following my suggestion you get rid of the dependency you have
> >>>>> between this series and patch "mtd: physmap_of: use OF helpers for
> >>>>> reading strings".  
> >>>>
> >>>> I learned (the very hard way) MTD people can be really nitpicking so I'm
> >>>> sending as simple patches as I can. I see it as the only way for someone from
> >>>> OpenWrt/LEDE project to get patch through your review.  
> >>>
> >>> And I learned the hard way that OpenWRT/LEDE developers tend to not
> >>> listen to our advices and keep arguing on things that have been proven
> >>> to be existing because of bad decision they made at some point in the
> >>> project life. So I think we're even :-P.  
> >>
> >> I wish you could sometimes forget what you've learned and review/discuss things
> >> without all that negative approach I keep seeing.  
> >
> > I try to stay objective, and if you look back at my review, you'll see
> > that I actually agree with most of your changes. So if one person is
> > taking it personally it's you, not me.
> >
> > Now, regarding other contributions, like support for the TRX format, I
> > keep thinking that it's badly designed and should not be supported in
> > mainline. I clearly expressed my opinion, and I also said I wouldn't
> > block the patches if other MTD maintainers were okay to take them (which
> > is already a good thing, don't you think?). But don't expect me to say
> > "Youhou, let's merge this awesome feature!".
> >
> > More generally, if you look back at all the contributions OpenWRT/LEDE
> > devs made, all uncontroversial features were merged rather quickly. For
> > the other ones, each time we tried to come up with alternative
> > solutions, but if you don't want to follow these suggestions (or at
> > least try them before saying it's impossible), then I think there's
> > nothing we can do on our side.  
> 
> Sounds fair from you, thanks. Please note I'm actually following your
> suggestions, just recently I sent RFC init for initramfs which should handle
> some of OpenWrt/LEDE hacks in user space as you told us to do this.
> 
> https://patchwork.ozlabs.org/patch/744093/

Yep, I followed the discussion you had with Richard on IRC. That's good
news.


--
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

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

[Boris Brezillon]

On Fri, 31 Mar 2017 14:23:11 +0200
Rafał Miłecki <zajec5@gmail.com> wrote:

> On 03/31/2017 01:41 PM, Boris Brezillon wrote:
> > Rafal,
> >
> > On Fri, 31 Mar 2017 12:46:38 +0200
> > Rafał Miłecki <zajec5@gmail.com> wrote:
> >  
> >>>>> BTW, not sure the intermediate "mtd: physmap_of: use OF helpers
> >>>>> for reading strings" patch is really useful, since you move to the
> >>>>> common infrastructure here.
> >>>>> By following my suggestion you get rid of the dependency you have
> >>>>> between this series and patch "mtd: physmap_of: use OF helpers for
> >>>>> reading strings".  
> >>>>
> >>>> I learned (the very hard way) MTD people can be really nitpicking so I'm
> >>>> sending as simple patches as I can. I see it as the only way for someone from
> >>>> OpenWrt/LEDE project to get patch through your review.  
> >>>
> >>> And I learned the hard way that OpenWRT/LEDE developers tend to not
> >>> listen to our advices and keep arguing on things that have been proven
> >>> to be existing because of bad decision they made at some point in the
> >>> project life. So I think we're even :-P.  
> >>
> >> I wish you could sometimes forget what you've learned and review/discuss things
> >> without all that negative approach I keep seeing.  
> >
> > I try to stay objective, and if you look back at my review, you'll see
> > that I actually agree with most of your changes. So if one person is
> > taking it personally it's you, not me.
> >
> > Now, regarding other contributions, like support for the TRX format, I
> > keep thinking that it's badly designed and should not be supported in
> > mainline. I clearly expressed my opinion, and I also said I wouldn't
> > block the patches if other MTD maintainers were okay to take them (which
> > is already a good thing, don't you think?). But don't expect me to say
> > "Youhou, let's merge this awesome feature!".
> >
> > More generally, if you look back at all the contributions OpenWRT/LEDE
> > devs made, all uncontroversial features were merged rather quickly. For
> > the other ones, each time we tried to come up with alternative
> > solutions, but if you don't want to follow these suggestions (or at
> > least try them before saying it's impossible), then I think there's
> > nothing we can do on our side.  
> 
> Sounds fair from you, thanks. Please note I'm actually following your
> suggestions, just recently I sent RFC init for initramfs which should handle
> some of OpenWrt/LEDE hacks in user space as you told us to do this.
> 
> https://patchwork.ozlabs.org/patch/744093/

Yep, I followed the discussion you had with Richard on IRC. That's good
news.