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

Signed-off-by Alexander Shiyan <[email protected]>

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

Documentation/devicetree/bindings/bus/imx-weim.txt +12 -5

index cedc2a9..0fd76c4 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,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:
 

drivers/bus/Kconfig +1 -2

index 1f70e84..552373c 100644
@@ -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

drivers/bus/imx-weim.c +0 -0

index dc860a4..541cf77 100644

drivers/bus/imx-weim.c +24 -0

+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[] = {

drivers/bus/imx-weim.c +16 -14

+	/* 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);

drivers/bus/imx-weim.c +0 -0

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

Signature · 1 line

1.8.1.5

Quoted 2 lines

Signed-off-by Alexander Shiyan <[email protected]>

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.

drivers/bus/imx-weim.c static int __init weim_probe(struct platform_device *pdev) · line 123

Quoted 7 lines

drivers/bus/imx-weim.c +3 -0

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

Quoted 4 lines

Quoted 4 lines

drivers/bus/imx-weim.c +0 -0

@@ -81,6 +118,9 @@ static int __init weim_probe(struct platform_device *pdev)
 	void __iomem *base;
 	int ret;
 

Quoted 5 lines

drivers/bus/imx-weim.c static int __init weim_probe(struct platform_device *pdev) · line 123

Quoted 3 lines

drivers/bus/imx-weim.c +3 -0

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

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

index 349f14e..0c0e6fe 100644

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.

Quoted 13 lines

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

index 349f14e..0c0e6fe 100644

drivers/bus/imx-weim.c line 18

Quoted 7 lines

drivers/bus/imx-weim.c +0 -4

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