[PATCH v3 1/5] drivers: bus: imx-weim: Remove private driver data

[PATCH v3 1/5] drivers: bus: imx-weim: Remove private driver data

[Alexander Shiyan]

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

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/bus/imx-weim.c | 41 ++++++++++++++---------------------------
 1 file changed, 14 insertions(+), 27 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 349f14e..0c0e6fe 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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;
 	}
 
-- 
1.8.1.5

[PATCH v3 2/5] drivers: bus: imx-weim: Simplify error path

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/bus/imx-weim.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0c0e6fe..0f4b081 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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;
 }
 
-- 
1.8.1.5

[PATCH v3 3/5] drivers: bus: imx-weim: use module_platform_driver_probe()

[Alexander Shiyan]

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

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/bus/imx-weim.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index 0f4b081..f872924 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -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");
-- 
1.8.1.5

[PATCH v3 4/5] drivers: bus: imx-weim: Add missing platform_friver.owne field

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 drivers/bus/imx-weim.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index f872924..dc860a4 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -108,8 +108,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);
-- 
1.8.1.5

[PATCH v3 5/5] drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 Documentation/devicetree/bindings/bus/imx-weim.txt | 17 +++--
 drivers/bus/Kconfig                                |  3 +-
 drivers/bus/imx-weim.c                             | 72 +++++++++++++++++-----
 3 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
index cedc2a9..0fd76c4 100644
--- a/Documentation/devicetree/bindings/bus/imx-weim.txt
+++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
@@ -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,18 @@ 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,
-			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
+			node's "reg" property. The number of registers depends
+			on the selected chip.
+			For i.MX1, i.MX21 ("fsl,imx1-weim") there are two
+			registers: CSxU, CSxL.
+			For i.MX25, i.MX27, i.MX31 and i.MX35 ("fsl,imx27-weim")
+			there are three registers: CSCRxU, CSCRxL, CSCRxA.
+			For i.MX50, i.MX53 ("fsl,imx50-weim"),
+			i.MX51 ("fsl,imx51-weim") and i.MX6Q ("fsl,imx6q-weim")
+			there are six registers: CSxGCR1, CSxGCR2, CSxRCR1,
+			CSxRCR2, CSxWCR1, CSxWCR2.
 
 Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
 
diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
index 1f70e84..552373c 100644
--- a/drivers/bus/Kconfig
+++ b/drivers/bus/Kconfig
@@ -8,10 +8,9 @@ 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.
 
 config MVEBU_MBUS
 	bool
diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
index dc860a4..541cf77 100644
--- a/drivers/bus/imx-weim.c
+++ b/drivers/bus/imx-weim.c
@@ -12,46 +12,83 @@
 #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 imx1_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 2,
+	.cs_stride	= 0x08,
+};
+
+static const struct imx_weim_devtype imx27_weim_devtype = {
+	.cs_count	= 6,
+	.cs_regs_count	= 3,
+	.cs_stride	= 0x10,
+};
+
+static const struct imx_weim_devtype imx50_weim_devtype = {
+	.cs_count	= 4,
+	.cs_regs_count	= 6,
+	.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[] = {
-	{ .compatible = "fsl,imx6q-weim", },
-	{}
+	/* i.MX1/21 */
+	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
+	/* i.MX25/27/31/35 */
+	{ .compatible = "fsl,imx27-weim", .data = &imx27_weim_devtype, },
+	/* i.MX50/53 */
+	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },
+	/* i.MX6Q */
+	{ .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);
 
-#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 +96,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);
@@ -81,6 +118,9 @@ static int __init weim_probe(struct platform_device *pdev)
 	void __iomem *base;
 	int ret;
 
+	if (!pdev->dev.of_node)
+		return -ENOTSUPP;
+
 	/* get the resource */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(&pdev->dev, res);
-- 
1.8.1.5

[PATCH v3 1/5] drivers: bus: imx-weim: Remove private driver data

[Sascha Hauer]

On Sun, Jun 23, 2013 at 11:48:14AM +0400, Alexander Shiyan wrote:
> Driver uses only probe function so no reason to keep variables
> in private driver data.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  drivers/bus/imx-weim.c | 41 ++++++++++++++---------------------------
>  1 file changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index 349f14e..0c0e6fe 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -12,11 +12,6 @@
>  #include <linux/io.h>
>  #include <linux/of_device.h>
>  
> -struct imx_weim {
> -	void __iomem *base;
> -	struct clk *clk;
> -};

I don't know how others see it, but I would keep the private data now
that it's there already. We don't know what it's good for in the future
and it doesn't hurt.

This is no strong opinion though since it always can be added back when
it's really needed.

Sascha

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

[PATCH v3 1/5] drivers: bus: imx-weim: Remove private driver data

[Fabio Estevam]

On Sun, Jun 23, 2013 at 4:23 PM, Sascha Hauer <s.hauer@pengutronix.de> wrote:

> I don't know how others see it, but I would keep the private data now
> that it's there already. We don't know what it's good for in the future
> and it doesn't hurt.

I agree with you, Sascha.

Re[2]: [PATCH v3 1/5] drivers: bus: imx-weim: Remove private driver data

[Alexander Shiyan]

> On Sun, Jun 23, 2013 at 11:48:14AM +0400, Alexander Shiyan wrote:
> > Driver uses only probe function so no reason to keep variables
> > in private driver data.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  drivers/bus/imx-weim.c | 41 ++++++++++++++---------------------------
> >  1 file changed, 14 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> > index 349f14e..0c0e6fe 100644
> > --- a/drivers/bus/imx-weim.c
> > +++ b/drivers/bus/imx-weim.c
> > @@ -12,11 +12,6 @@
> >  #include <linux/io.h>
> >  #include <linux/of_device.h>
> >  
> > -struct imx_weim {
> > -	void __iomem *base;
> > -	struct clk *clk;
> > -};
> 
> I don't know how others see it, but I would keep the private data now
> that it's there already. We don't know what it's good for in the future
> and it doesn't hurt.
> 
> This is no strong opinion though since it always can be added back when
> it's really needed.

A possible case where private data can be used if we alter this driver to a real bus-driver.
In the current state, the data is not used.
Indeed, the code can be returned later, if it will needed.
Thanks.

---

[PATCH v3 5/5] drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

[Shawn Guo]

On Sun, Jun 23, 2013 at 11:48:18AM +0400, Alexander Shiyan wrote:
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Can you write up something for the commit log?  Leaving it empty for
such a patch with tens of LOC is not a nice thing.

> ---
>  Documentation/devicetree/bindings/bus/imx-weim.txt | 17 +++--
>  drivers/bus/Kconfig                                |  3 +-
>  drivers/bus/imx-weim.c                             | 72 +++++++++++++++++-----
>  3 files changed, 69 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/bus/imx-weim.txt b/Documentation/devicetree/bindings/bus/imx-weim.txt
> index cedc2a9..0fd76c4 100644
> --- a/Documentation/devicetree/bindings/bus/imx-weim.txt
> +++ b/Documentation/devicetree/bindings/bus/imx-weim.txt
> @@ -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,18 @@ 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,
> -			EIM_CSnRCR2, EIM_CSnWCR1, EIM_CSnWCR2 in this order.
> +			node's "reg" property. The number of registers depends
> +			on the selected chip.
> +			For i.MX1, i.MX21 ("fsl,imx1-weim") there are two
> +			registers: CSxU, CSxL.
> +			For i.MX25, i.MX27, i.MX31 and i.MX35 ("fsl,imx27-weim")
> +			there are three registers: CSCRxU, CSCRxL, CSCRxA.
> +			For i.MX50, i.MX53 ("fsl,imx50-weim"),
> +			i.MX51 ("fsl,imx51-weim") and i.MX6Q ("fsl,imx6q-weim")
> +			there are six registers: CSxGCR1, CSxGCR2, CSxRCR1,
> +			CSxRCR2, CSxWCR1, CSxWCR2.
>  
>  Example for an imx6q-sabreauto board, the NOR flash connected to the WEIM:
>  
> diff --git a/drivers/bus/Kconfig b/drivers/bus/Kconfig
> index 1f70e84..552373c 100644
> --- a/drivers/bus/Kconfig
> +++ b/drivers/bus/Kconfig
> @@ -8,10 +8,9 @@ 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.
>  
>  config MVEBU_MBUS
>  	bool
> diff --git a/drivers/bus/imx-weim.c b/drivers/bus/imx-weim.c
> index dc860a4..541cf77 100644
> --- a/drivers/bus/imx-weim.c
> +++ b/drivers/bus/imx-weim.c
> @@ -12,46 +12,83 @@
>  #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;
> +};

Please have a blank line here.

> +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 imx27_weim_devtype = {
> +	.cs_count	= 6,
> +	.cs_regs_count	= 3,
> +	.cs_stride	= 0x10,
> +};
> +
> +static const struct imx_weim_devtype imx50_weim_devtype = {
> +	.cs_count	= 4,
> +	.cs_regs_count	= 6,
> +	.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[] = {
> -	{ .compatible = "fsl,imx6q-weim", },
> -	{}
> +	/* i.MX1/21 */
> +	{ .compatible = "fsl,imx1-weim", .data = &imx1_weim_devtype, },
> +	/* i.MX25/27/31/35 */
> +	{ .compatible = "fsl,imx27-weim", .data = &imx27_weim_devtype, },
> +	/* i.MX50/53 */
> +	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },
> +	/* i.MX6Q */
> +	{ .compatible = "fsl,imx6q-weim", .data = &imx50_weim_devtype, },

I would suggest we have something like below to give it a hint that
i.MX6Q WEIM is the same type as i.MX50/53 one.

	/* i.MX50/53/6Q */
	{ .compatible = "fsl,imx50-weim", .data = &imx50_weim_devtype, },
	{ .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);
>  
> -#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 +96,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);
> @@ -81,6 +118,9 @@ static int __init weim_probe(struct platform_device *pdev)
>  	void __iomem *base;
>  	int ret;
>  
> +	if (!pdev->dev.of_node)
> +		return -ENOTSUPP;
> +
Is it really necessary?  Since we only support DT probe, we can not
reach here if the of_node is NULL.

Shawn

>  	/* get the resource */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(&pdev->dev, res);
> -- 
> 1.8.1.5
> 

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

[Alexander Shiyan]

> On Sun, Jun 23, 2013 at 11:48:18AM +0400, Alexander Shiyan wrote:
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> 
> Can you write up something for the commit log?  Leaving it empty for
> such a patch with tens of LOC is not a nice thing.

OK.

...
> > @@ -81,6 +118,9 @@ static int __init weim_probe(struct platform_device *pdev)
> >  	void __iomem *base;
> >  	int ret;
> >  
> > +	if (!pdev->dev.of_node)
> > +		return -ENOTSUPP;
> > +
> Is it really necessary?  Since we only support DT probe, we can not
> reach here if the of_node is NULL.

In fact, nothing prevents to add to the board file line like
platform_device_register_simple ("imx-weim", 0, res, res_cnt);
In this case, the core will crush. I make it impossible to use.
Consider it unnecessary checks?
Thanks!

---

[PATCH v3 5/5] drivers: bus: imx-weim: Add support for i.MX1/21/25/27/31/35/50/51/53

[Shawn Guo]

On Mon, Jun 24, 2013 at 03:49:53PM +0400, Alexander Shiyan wrote:
> > > @@ -81,6 +118,9 @@ static int __init weim_probe(struct platform_device *pdev)
> > >  	void __iomem *base;
> > >  	int ret;
> > >  
> > > +	if (!pdev->dev.of_node)
> > > +		return -ENOTSUPP;
> > > +
> > Is it really necessary?  Since we only support DT probe, we can not
> > reach here if the of_node is NULL.
> 
> In fact, nothing prevents to add to the board file line like
> platform_device_register_simple ("imx-weim", 0, res, res_cnt);

Reasonability prevents that:

1) We no longer accept any changes like that.
2) The device driver does not work with platform probe anyway.  What's
   the point of doing that?

> In this case, the core will crush. I make it impossible to use.
> Consider it unnecessary checks?

Yes, unnecessary check for me.

Shawn