Skip to contentSkip to conversation

drivers: bus: imx-weim: Remove private driver data

PATCH Superseded Alexander Shiyan 4 participants 14 messages Jun 19, 2013 – Jun 20, 2013

Subsystems (2)
Device tree bindings Arm i.MX and Layerscape

v2 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Alexander Shiyan

author
#0 lore raw

PATCH v2 1/7 drivers: bus: imx-weim: Remove private driver data

Driver uses only probe function so no reason to keep variables in private driver data.

Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/bus/imx-weim.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)
drivers/bus/imx-weim.c +14 -27
index 349f14e..0c0e6fe 100644
@@ -12,11 +12,6 @@
 #include <linux/io.h>
 #include <linux/of_device.h>
 
-struct imx_weim {
-	void __iomem *base;
-	struct clk *clk;
-};
-
 static const struct of_device_id weim_id_table[] = {
 	{ .compatible = "fsl,imx6q-weim", },
 	{}

@@ -27,10 +22,8 @@ MODULE_DEVICE_TABLE(of, weim_id_table);
 #define CS_REG_RANGE  0x18
 
 /* Parse and set the timing for this device. */
-static int
-weim_timing_setup(struct platform_device *pdev, struct device_node *np)
+static int weim_timing_setup(struct device_node *np, void __iomem *base)
 {
-	struct imx_weim *weim = platform_get_drvdata(pdev);
 	u32 value[CS_TIMING_LEN];
 	u32 cs_idx;
 	int ret;

@@ -52,11 +45,11 @@ weim_timing_setup(struct platform_device *pdev, struct device_node *np)
 
 	/* set the timing for WEIM */
 	for (i = 0; i < CS_TIMING_LEN; i++)
-		writel(value[i], weim->base + cs_idx * CS_REG_RANGE + i * 4);
+		writel(value[i], base + cs_idx * CS_REG_RANGE + i * 4);
 	return 0;
 }
 
-static int weim_parse_dt(struct platform_device *pdev)
+static int weim_parse_dt(struct platform_device *pdev, void __iomem *base)
 {
 	struct device_node *child;
 	int ret;

@@ -65,7 +58,7 @@ static int weim_parse_dt(struct platform_device *pdev)
 		if (!child->name)
 			continue;
 
-		ret = weim_timing_setup(pdev, child);
+		ret = weim_timing_setup(child, base);
 		if (ret) {
 			dev_err(&pdev->dev, "%s set timing failed.\n",
 				child->full_name);

@@ -82,38 +75,32 @@ static int weim_parse_dt(struct platform_device *pdev)
 
 static int weim_probe(struct platform_device *pdev)
 {
-	struct imx_weim *weim;
 	struct resource *res;
+	struct clk *clk;
+	void __iomem *base;
 	int ret = -EINVAL;
 
-	weim = devm_kzalloc(&pdev->dev, sizeof(*weim), GFP_KERNEL);
-	if (!weim) {
-		ret = -ENOMEM;
-		goto weim_err;
-	}
-	platform_set_drvdata(pdev, weim);
-
 	/* get the resource */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	weim->base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(weim->base)) {
-		ret = PTR_ERR(weim->base);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base)) {
+		ret = PTR_ERR(base);
 		goto weim_err;
 	}
 
 	/* get the clock */
-	weim->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(weim->clk))
+	clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(clk))
 		goto weim_err;
 
-	ret = clk_prepare_enable(weim->clk);
+	ret = clk_prepare_enable(clk);
 	if (ret)
 		goto weim_err;
 
 	/* parse the device node */
-	ret = weim_parse_dt(pdev);
+	ret = weim_parse_dt(pdev, base);
 	if (ret) {
-		clk_disable_unprepare(weim->clk);
+		clk_disable_unprepare(clk);
 		goto weim_err;
 	}
Signature · 1 line
1.8.1.5
author
#1 lore raw

PATCH v2 2/7 drivers: bus: imx-weim: Simplify error path

Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/bus/imx-weim.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)
drivers/bus/imx-weim.c +8 -14
index 0c0e6fe..0f4b081 100644
@@ -78,36 +78,30 @@ static int weim_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct clk *clk;
 	void __iomem *base;
-	int ret = -EINVAL;
+	int ret;
 
 	/* get the resource */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base)) {
-		ret = PTR_ERR(base);
-		goto weim_err;
-	}
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
 	/* get the clock */
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk))
-		goto weim_err;
+		return PTR_ERR(clk);
 
 	ret = clk_prepare_enable(clk);
 	if (ret)
-		goto weim_err;
+		return ret;
 
 	/* parse the device node */
 	ret = weim_parse_dt(pdev, base);
-	if (ret) {
+	if (ret)
 		clk_disable_unprepare(clk);
-		goto weim_err;
-	}
-
-	dev_info(&pdev->dev, "WEIM driver registered.\n");
-	return 0;
+	else
+		dev_info(&pdev->dev, "Driver registered.\n");
 
-weim_err:
 	return ret;
 }
Signature · 1 line
1.8.1.5
author
#2 lore raw

PATCH v2 3/7 drivers: bus: imx-weim: use module_platform_driver_probe()

In reply to Alexander Shiyan

Driver should be called only once at startup, so code converted to using module_platform_driver_probe().

Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/bus/imx-weim.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/bus/imx-weim.c +5 -5
index 0f4b081..f872924 100644
@@ -22,7 +22,7 @@ MODULE_DEVICE_TABLE(of, weim_id_table);
 #define CS_REG_RANGE  0x18
 
 /* Parse and set the timing for this device. */
-static int weim_timing_setup(struct device_node *np, void __iomem *base)
+static int __init weim_timing_setup(struct device_node *np, void __iomem *base)
 {
 	u32 value[CS_TIMING_LEN];
 	u32 cs_idx;

@@ -49,7 +49,8 @@ static int weim_timing_setup(struct device_node *np, void __iomem *base)
 	return 0;
 }
 
-static int weim_parse_dt(struct platform_device *pdev, void __iomem *base)
+static int __init weim_parse_dt(struct platform_device *pdev,
+				void __iomem *base)
 {
 	struct device_node *child;
 	int ret;

@@ -73,7 +74,7 @@ static int weim_parse_dt(struct platform_device *pdev, void __iomem *base)
 	return ret;
 }
 
-static int weim_probe(struct platform_device *pdev)
+static int __init weim_probe(struct platform_device *pdev)
 {
 	struct resource *res;
 	struct clk *clk;

@@ -110,10 +111,9 @@ static struct platform_driver weim_driver = {
 		.name = "imx-weim",
 		.of_match_table = weim_id_table,
 	},
-	.probe   = weim_probe,
 };
+module_platform_driver_probe(weim_driver, weim_probe);
 
-module_platform_driver(weim_driver);
 MODULE_AUTHOR("Freescale Semiconductor Inc.");
 MODULE_DESCRIPTION("i.MX EIM Controller Driver");
 MODULE_LICENSE("GPL");
Signature · 1 line
1.8.1.5
author
#3 lore raw

PATCH v2 4/7 drivers: bus: imx-weim: Preparation driver to support different CPUs

In reply to Alexander Shiyan
Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/bus/imx-weim.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)
drivers/bus/imx-weim.c +28 -16
index f872924..b558aa3 100644
@@ -12,46 +12,58 @@
 #include <linux/io.h>
 #include <linux/of_device.h>
 
+struct imx_weim_devtype {
+	unsigned int	cs_count;
+	unsigned int	cs_regs_count;
+	unsigned int	cs_stride;
+};
+
+static const struct imx_weim_devtype imx50_weim_devtype = {
+	.cs_count	= 4,
+	.cs_regs_count	= 6,
+	.cs_stride	= 0x18,
+};
+
 static const struct of_device_id weim_id_table[] = {
-	{ .compatible = "fsl,imx6q-weim", },
-	{}
+	/* i.MX50/53/6Q */
+	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+	{ }
 };
 MODULE_DEVICE_TABLE(of, weim_id_table);
 
-#define CS_TIMING_LEN 6
-#define CS_REG_RANGE  0x18
-
 /* Parse and set the timing for this device. */
-static int __init weim_timing_setup(struct device_node *np, void __iomem *base)
+static int __init weim_timing_setup(struct device_node *np, void __iomem *base,
+				    const struct imx_weim_devtype *devtype)
 {
-	u32 value[CS_TIMING_LEN];
-	u32 cs_idx;
-	int ret;
-	int i;
+	u32 cs_idx, value[devtype->cs_regs_count];
+	int i, ret;
 
 	/* get the CS index from this child node's "reg" property. */
 	ret = of_property_read_u32(np, "reg", &cs_idx);
 	if (ret)
 		return ret;
 
-	/* The weim has four chip selects. */
-	if (cs_idx > 3)
+	if (cs_idx >= devtype->cs_count)
 		return -EINVAL;
 
 	ret = of_property_read_u32_array(np, "fsl,weim-cs-timing",
-					value, CS_TIMING_LEN);
+					 value, devtype->cs_regs_count);
 	if (ret)
 		return ret;
 
 	/* set the timing for WEIM */
-	for (i = 0; i < CS_TIMING_LEN; i++)
-		writel(value[i], base + cs_idx * CS_REG_RANGE + i * 4);
+	for (i = 0; i < devtype->cs_regs_count; i++)
+		writel(value[i], base + cs_idx * devtype->cs_stride + i * 4);
+
 	return 0;
 }
 
 static int __init weim_parse_dt(struct platform_device *pdev,
 				void __iomem *base)
 {
+	const struct of_device_id *of_id = of_match_device(weim_id_table,
+							   &pdev->dev);
+	const struct imx_weim_devtype *devtype = of_id->data;
 	struct device_node *child;
 	int ret;
 

@@ -59,7 +71,7 @@ static int __init weim_parse_dt(struct platform_device *pdev,
 		if (!child->name)
 			continue;
 
-		ret = weim_timing_setup(child, base);
+		ret = weim_timing_setup(child, base, devtype);
 		if (ret) {
 			dev_err(&pdev->dev, "%s set timing failed.\n",
 				child->full_name);
Signature · 1 line
1.8.1.5
author
#4 lore raw

PATCH v2 5/7 drivers: bus: imx-weim: Add missing platform_friver.owner field

In reply to Alexander Shiyan
Signed-off-by Alexander Shiyan <[email protected]>
---
 drivers/bus/imx-weim.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/bus/imx-weim.c +3 -2
index b558aa3..b6479fb 100644
@@ -120,8 +120,9 @@ static int __init weim_probe(struct platform_device *pdev)
 
 static struct platform_driver weim_driver = {
 	.driver = {
-		.name = "imx-weim",
-		.of_match_table = weim_id_table,
+		.name		= "imx-weim",
+		.owner		= THIS_MODULE,
+		.of_match_table	= weim_id_table,
 	},
 };
 module_platform_driver_probe(weim_driver, weim_probe);
Signature · 1 line
1.8.1.5
author
#5 lore raw

PATCH v2 6/7 drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

In reply to Alexander Shiyan
Signed-off-by Alexander Shiyan <[email protected]>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt | 11 +++++++----
 drivers/bus/Kconfig                                |  4 ++--
 drivers/bus/imx-weim.c                             | 23 ++++++++++++++++++++++
 3 files changed, 32 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/bus/imx-weim.txt +7 -4
index cedc2a9..99406e4 100644
@@ -8,7 +8,7 @@ The actual devices are instantiated from the child nodes of a WEIM node.
 
 Required properties:
 
- - compatible:		Should be set to "fsl,imx6q-weim"
+ - compatible:		Should be set to "fsl,<soc>-weim"
  - reg:			A resource specifier for the register space
 			(see the example below)
  - clocks:		the clock, see the example below.

@@ -21,11 +21,14 @@ Required properties:
 
 Timing property for child nodes. It is mandatory, not optional.
 
- - fsl,weim-cs-timing:	The timing array, contains 6 timing values for the
+ - fsl,weim-cs-timing:	The timing array, contains timing values for the
 			child node. We can get the CS index from the child
-			node's "reg" property. This property contains the values
-			for the registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
+			node's "reg" property. For example, if i.MX6Q CPU
+			is used, this property contains the values for the
+			registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
 			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
+			For other i.MX CPUs count of register and register
+			names may be different.
 
 Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
drivers/bus/Kconfig +2 -2
index 1f70e84..4faedc21 100644
@@ -8,10 +8,10 @@ config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
 	help
-	  Driver for i.MX6 WEIM controller.
+	  Driver for i.MX WEIM controller.
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
-	  But now, we only support the Parallel NOR.
+	  But now, we only support the "of_physmap" driver.
 
 config MVEBU_MBUS
 	bool
drivers/bus/imx-weim.c +23 -0
index b6479fb..77fa1d4 100644
@@ -17,6 +17,17 @@ struct imx_weim_devtype {
 	unsigned int	cs_regs_count;
 	unsigned int	cs_stride;
 };
+static const struct imx_weim_devtype imx1_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 2,
+	.cs_stride	= 0x08,
+};
+
+static const struct imx_weim_devtype imx25_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 3,
+	.cs_stride	= 0x10,
+};
 
 static const struct imx_weim_devtype imx50_weim_devtype = {
 	.cs_count	= 4,

@@ -24,9 +35,21 @@ static const struct imx_weim_devtype imx50_weim_devtype = {
 	.cs_stride	= 0x18,
 };
 
+static const struct imx_weim_devtype imx51_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 6,
+	.cs_stride	= 0x18,
+};
+
 static const struct of_device_id weim_id_table[] = {
+	/* i.MX1/21 */
+	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
+	/* i.MX25/27/31/35 */
+	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },
 	/* i.MX50/53/6Q */
 	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+	/* i.MX51 */
+	{ .compatible = "fsl,imx51-weim", .data = &imx51_weim_devtype, },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, weim_id_table);
Signature · 1 line
1.8.1.5
maintainer
#6 lore raw

PATCH v2 6/7 drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

Hi Alexander,

Nice work!

Documentation/devicetree/bindings/bus/imx-weim.txt Required properties: · line 31

Quoted 11 lines · Alexander Shiyan

- - fsl,weim-cs-timing: The timing array, contains 6 timing values for the
+ - fsl,weim-cs-timing: The timing array, contains timing values for the

Documentation/devicetree/bindings/bus/imx-weim.txt +5 -2
 			child node. We can get the CS index from the child
-			node's "reg" property. This property contains the values
-			for the registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
+			node's "reg" property. For example, if i.MX6Q CPU
+			is used, this property contains the values for the
+			registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
 			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
+			For other i.MX CPUs count of register and register
+			names may be different.

I think here we should have a more precise description for each SoC.

Quoted 8 lines

Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:

drivers/bus/Kconfig +0 -0
index 1f70e84..4faedc21 100644

drivers/bus/Kconfig config IMX_WEIM · line 14

Quoted 9 lines
drivers/bus/Kconfig +2 -2
@@ -8,10 +8,10 @@ config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
 	help
-	  Driver for i.MX6 WEIM controller.
+	  Driver for i.MX WEIM controller.
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
-	  But now, we only support the Parallel NOR.
+	  But now, we only support the "of_physmap" driver.

This comment is wrong. In the early versions of this patch indeed only parallel NOR was supported, but now there is no limitation on the device type anymore.

Quoted 30 lines
drivers/bus/Kconfig +0 -0
 config MVEBU_MBUS
 	bool
drivers/bus/imx-weim.c +12 -0
index b6479fb..77fa1d4 100644
@@ -17,6 +17,17 @@ struct imx_weim_devtype {
 	unsigned int	cs_regs_count;
 	unsigned int	cs_stride;
 };
+static const struct imx_weim_devtype imx1_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 2,
+	.cs_stride	= 0x08,
+};
+
+static const struct imx_weim_devtype imx25_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 3,
+	.cs_stride	= 0x10,
+};
 
 static const struct imx_weim_devtype imx50_weim_devtype = {
 	.cs_count	= 4,

@@ -24,9 +35,21 @@ static const struct imx_weim_devtype imx50_weim_devtype = {
 	.cs_stride	= 0x18,
 };
 
+static const struct imx_weim_devtype imx51_weim_devtype = {

drivers/bus/imx-weim.c static const struct imx_weim_devtype imx50_weim_devtype = { · line 48

Quoted 10 lines
drivers/bus/imx-weim.c +9 -0
+	.cs_count	= 6,
+	.cs_regs_count	= 6,
+	.cs_stride	= 0x18,
+};
+
 static const struct of_device_id weim_id_table[] = {
+	/* i.MX1/21 */
+	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
+	/* i.MX25/27/31/35 */
+	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },

We usually name the compatible name after the oldest i.MX supporting this IP, not after the lowest number. So this should be imx27, not imx25.

Sascha

Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
author
#7 lore raw

Re[2]: [PATCH v2 6/7] drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

Quoted 5 lines

Hi Alexander,

Nice work!

On Wed, Jun 19, 2013 at 11:54:37PM +0400, Alexander Shiyan wrote:

Quoted 13 lines

Documentation/devicetree/bindings/bus/imx-weim.txt Required properties: · line 31

Quoted 11 lines

- - fsl,weim-cs-timing: The timing array, contains 6 timing values for the
+ - fsl,weim-cs-timing: The timing array, contains timing values for the

Documentation/devicetree/bindings/bus/imx-weim.txt +5 -2
 			child node. We can get the CS index from the child
-			node's "reg" property. This property contains the values
-			for the registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
+			node's "reg" property. For example, if i.MX6Q CPU
+			is used, this property contains the values for the
+			registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
 			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
+			For other i.MX CPUs count of register and register
+			names may be different.

I think here we should have a more precise description for each SoC.

OK.

Quoted 12 lines
Quoted 12 lines

Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:

drivers/bus/Kconfig +1 -1
index 1f70e84..4faedc21 100644
@@ -8,10 +8,10 @@ config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
 	help
-	  Driver for i.MX6 WEIM controller.
+	  Driver for i.MX WEIM controller.
Quoted 8 lines

drivers/bus/Kconfig config IMX_WEIM · line 14

Quoted 4 lines
drivers/bus/Kconfig +1 -1
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
-	  But now, we only support the Parallel NOR.
+	  But now, we only support the "of_physmap" driver.

This comment is wrong. In the early versions of this patch indeed only parallel NOR was supported, but now there is no limitation on the device type anymore.

I am do not think so. But I will not insist on his opinion. I checked the operation only for physmap-flash and mtd-ram.

...

Quoted 9 lines

drivers/bus/Kconfig config IMX_WEIM

Quoted 5 lines
drivers/bus/Kconfig +4 -0
 static const struct of_device_id weim_id_table[] = {
+	/* i.MX1/21 */
+	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
+	/* i.MX25/27/31/35 */
+	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },

We usually name the compatible name after the oldest i.MX supporting this IP, not after the lowest number. So this should be imx27, not imx25.

OK. Seems this fact also avoid [7/7] part. Is not it?

---

maintainer
#8 lore raw

PATCH v2 6/7 drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

Quoted 42 lines · Alexander Shiyan
Quoted 5 lines

Hi Alexander,

Nice work!

On Wed, Jun 19, 2013 at 11:54:37PM +0400, Alexander Shiyan wrote:

Quoted 13 lines

Documentation/devicetree/bindings/bus/imx-weim.txt Required properties: · line 31

Quoted 11 lines

- - fsl,weim-cs-timing: The timing array, contains 6 timing values for the
+ - fsl,weim-cs-timing: The timing array, contains timing values for the

Documentation/devicetree/bindings/bus/imx-weim.txt +5 -2
 			child node. We can get the CS index from the child
-			node's "reg" property. This property contains the values
-			for the registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
+			node's "reg" property. For example, if i.MX6Q CPU
+			is used, this property contains the values for the
+			registers EIM_CSnGCR1, EIM_CSnGCR2, EIM_CSnRCR1,
 			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
+			For other i.MX CPUs count of register and register
+			names may be different.

I think here we should have a more precise description for each SoC.

OK.

Quoted 12 lines
Quoted 12 lines

Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:

drivers/bus/Kconfig +1 -1
index 1f70e84..4faedc21 100644
@@ -8,10 +8,10 @@ config IMX_WEIM
 	bool "Freescale EIM DRIVER"
 	depends on ARCH_MXC
 	help
-	  Driver for i.MX6 WEIM controller.
+	  Driver for i.MX WEIM controller.
Quoted 8 lines

drivers/bus/Kconfig config IMX_WEIM · line 14

Quoted 4 lines
drivers/bus/Kconfig +1 -1
 	  The WEIM(Wireless External Interface Module) works like a bus.
 	  You can attach many different devices on it, such as NOR, onenand.
-	  But now, we only support the Parallel NOR.
+	  But now, we only support the "of_physmap" driver.

This comment is wrong. In the early versions of this patch indeed only parallel NOR was supported, but now there is no limitation on the device type anymore.

Quoted 2 lines

I am do not think so. But I will not insist on his opinion. I checked the operation only for physmap-flash and mtd-ram.

The first version of the driver looked for a child node named 'nor' and registered only that one. Now it calls of_platform_populate which registers everything found under the weim node.

Quoted 12 lines

...

Quoted 9 lines

drivers/bus/Kconfig

Quoted 5 lines

static const struct of_device_id weim_id_table[] = {

drivers/bus/Kconfig +4 -0
+	/* i.MX1/21 */
+	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
+	/* i.MX25/27/31/35 */
+	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },

We usually name the compatible name after the oldest i.MX supporting this IP, not after the lowest number. So this should be imx27, not imx25.

Quoted 1 line

OK. Seems this fact also avoid [7/7] part. Is not it?

I think i.MX50 is older than i.MX6, so 7/7 should be correct.

Sascha

Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
author
#9 lore raw

PATCH v2 7/7 drivers: bus: imx-weim: Change compatible string for i.MX6Q

In reply to Alexander Shiyan

This patch changes compatible string for i.MX6Q to "fsl,imx50-weim" since this is a lowest CPU type with same bus.

Signed-off-by Alexander Shiyan <[email protected]>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt | 2 +-
 drivers/bus/imx-weim.c                             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/bus/imx-weim.txt +1 -1
index 99406e4..b134811 100644
@@ -33,7 +33,7 @@ Timing property for child nodes. It is mandatory, not optional.
 Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
 
 	weim: weim at 021b8000 {
-		compatible = "fsl,imx6q-weim";
+		compatible = "fsl,imx6q-weim", "fsl,imx50-weim";
 		reg = <0x021b8000 0x4000>;
 		clocks = <&clks 196>;
 		#address-cells = <2>;
drivers/bus/imx-weim.c +1 -1
index 77fa1d4..3f665f8 100644
@@ -47,7 +47,7 @@ static const struct of_device_id weim_id_table[] = {
 	/* i.MX25/27/31/35 */
 	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },
 	/* i.MX50/53/6Q */
-	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },
 	/* i.MX51 */
 	{ .compatible = "fsl,imx51-weim", .data = &imx51_weim_devtype, },
 	{ }
Signature · 1 line
1.8.1.5
#10 lore raw

PATCH v2 7/7 drivers: bus: imx-weim: Change compatible string for i.MX6Q

Quoted 9 lines · Alexander Shiyan

This patch changes compatible string for i.MX6Q to "fsl,imx50-weim" since this is a lowest CPU type with same bus.

Signed-off-by Alexander Shiyan <[email protected]>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt | 2 +-
 drivers/bus/imx-weim.c                             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

I think patches #4, #6 and #7 can just be one patch.

Quoted 18 lines
Documentation/devicetree/bindings/bus/imx-weim.txt +1 -1
index 99406e4..b134811 100644
@@ -33,7 +33,7 @@ Timing property for child nodes. It is mandatory, not optional.
 Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
 
 	weim: weim at 021b8000 {
-		compatible = "fsl,imx6q-weim";
+		compatible = "fsl,imx6q-weim", "fsl,imx50-weim";
 		reg = <0x021b8000 0x4000>;
 		clocks = <&clks 196>;
 		#address-cells = <2>;
drivers/bus/imx-weim.c +0 -0
index 77fa1d4..3f665f8 100644

drivers/bus/imx-weim.c static const struct of_device_id weim_id_table[] = { · line 50

Quoted 5 lines
drivers/bus/imx-weim.c +1 -1
@@ -47,7 +47,7 @@ static const struct of_device_id weim_id_table[] = {
 	/* i.MX25/27/31/35 */
 	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },
 	/* i.MX50/53/6Q */
-	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },

The compatible "fsl,imx6q-weim" already has a user on the way to 3.11. Changing the match table without updating the user will break it right away. Also, since the series will be 3.12 material, doing this will result in an incompatible device tree between 3.11 and 3.12. For this reason, I suggest we keep using "fsl,imx6q-weim" for matching imx6q/50/53 type of weim device.

Shawn

Quoted 6 lines
drivers/bus/imx-weim.c +0 -0
 	/* i.MX51 */
 	{ .compatible = "fsl,imx51-weim", .data = &imx51_weim_devtype, },
 	{ }
Signature · 1 line
1.8.1.5
maintainer
#11 lore raw

PATCH v2 7/7 drivers: bus: imx-weim: Change compatible string for i.MX6Q

Quoted 34 lines · Shawn Guo
Quoted 9 lines · Alexander Shiyan

This patch changes compatible string for i.MX6Q to "fsl,imx50-weim" since this is a lowest CPU type with same bus.

Signed-off-by Alexander Shiyan <[email protected]>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt | 2 +-
 drivers/bus/imx-weim.c                             | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

I think patches #4, #6 and #7 can just be one patch.

Quoted 22 lines
Documentation/devicetree/bindings/bus/imx-weim.txt +1 -1
index 99406e4..b134811 100644
@@ -33,7 +33,7 @@ Timing property for child nodes. It is mandatory, not optional.
 Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
 
 	weim: weim at 021b8000 {
-		compatible = "fsl,imx6q-weim";
+		compatible = "fsl,imx6q-weim", "fsl,imx50-weim";
 		reg = <0x021b8000 0x4000>;
 		clocks = <&clks 196>;
 		#address-cells = <2>;
drivers/bus/imx-weim.c +0 -1
index 77fa1d4..3f665f8 100644
@@ -47,7 +47,7 @@ static const struct of_device_id weim_id_table[] = {
 	/* i.MX25/27/31/35 */
 	{ .compatible = "fsl,imx25-weim", .data = &imx25_weim_devtype, },
 	/* i.MX50/53/6Q */
-	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
Quoted 8 lines

drivers/bus/imx-weim.c static const struct of_device_id weim_id_table[] = { · line 50

Quoted 1 line
drivers/bus/imx-weim.c +1 -0
+	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },

The compatible "fsl,imx6q-weim" already has a user on the way to 3.11. Changing the match table without updating the user will break it right away. Also, since the series will be 3.12 material, doing this will result in an incompatible device tree between 3.11 and 3.12. For this reason, I suggest we keep using "fsl,imx6q-weim" for matching imx6q/50/53 type of weim device.

How about:

+ { .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+ { .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },

We could then change the in-kernel devicetrees how we need it without breaking existing devicetrees.

Sascha

Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
#12 lore raw

PATCH v2 7/7 drivers: bus: imx-weim: Change compatible string for i.MX6Q

Quoted 8 lines · Sascha Hauer

How about:

(quoted diff) +2 -0
+	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },

We could then change the in-kernel devicetrees how we need it without breaking existing devicetrees.

Yeah, it works for me.

Shawn

reviewer
#13 lore raw

PATCH v2 7/7 drivers: bus: imx-weim: Change compatible string for i.MX6Q

In reply to Sascha Hauer

Review

Quoted 6 lines · Sascha Hauer

On Thu, Jun 20, 2013 at 03:10:01PM +0800, Shawn Guo wrote:

How about:

(quoted diff) +2 -0
+       { .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },
+       { .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },

Why don't we just drop the mx50 case, since there is no kernel support for mx50?