[PATCH 2/4] ARM: mxc: migrate mach-mx5 gpio driver to gpio-mxc

On Mon, May 30, 2011 at 05:16:34PM -0700, Olof Johansson wrote:

Hi,

On Mon, May 30, 2011 at 10:52:17PM +0800, Shawn Guo wrote:

quoted

It adds platform device for drivers/gpio/gpio-mxc, and migrates
mx50/mx51/mx53 gpio driver to gpio-mxc.

[...]

quoted

diff --git a/arch/arm/plat-mxc/devices/platform-gpio-mxc.c b/arch/arm/plat-mxc/devices/platform-gpio-mxc.c
new file mode 100644
index 0000000..3b10da0
--- /dev/null
+++ b/arch/arm/plat-mxc/devices/platform-gpio-mxc.c

@@ -0,0 +1,69 @@
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2011 Linaro Limited
+ *
+ * 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/compiler.h>
+#include <linux/err.h>
+#include <linux/init.h>
+
+#include <mach/hardware.h>
+#include <mach/devices-common.h>
+
+static struct platform_device *__init mxc_add_gpio(int id,
+	resource_size_t iobase, resource_size_t iosize, int irq, int irq_high)
+{
+	struct resource res[] = {
+		{
+			.start = iobase,
+			.end = iobase + iosize - 1,
+			.flags = IORESOURCE_MEM,
+		}, {
+			.start = irq,
+			.end = irq,
+			.flags = IORESOURCE_IRQ,
+		}, {
+			.start = irq_high,
+			.end = irq_high,
+			.flags = IORESOURCE_IRQ,
+		},
+	};
+
+	return platform_device_register_resndata(&mxc_aips_bus,
+			"gpio-mxc", id, res, ARRAY_SIZE(res), NULL, 0);

Why bother returning the value, it's never checked below?

Please help me understand.  You are saying the return value should be
checked below?

quoted

+static int __init mxc_add_mxc_gpio(void)

Minor nits: Redundant mxcs? Also, 'gpios' would be more accurate naming.

The first one is the namespace of plat-mxc function, and the second
one is to reflect gpio driver name 'mxc gpio'.

quoted

+{
+	if (cpu_is_mx50()) {
+		mxc_add_gpio(0, MX50_GPIO1_BASE_ADDR, SZ_16K, MX50_INT_GPIO1_LOW, MX50_INT_GPIO1_HIGH);
+		mxc_add_gpio(1, MX50_GPIO2_BASE_ADDR, SZ_16K, MX50_INT_GPIO2_LOW, MX50_INT_GPIO2_HIGH);
+		mxc_add_gpio(2, MX50_GPIO3_BASE_ADDR, SZ_16K, MX50_INT_GPIO3_LOW, MX50_INT_GPIO3_HIGH);
+		mxc_add_gpio(3, MX50_GPIO4_BASE_ADDR, SZ_16K, MX50_INT_GPIO4_LOW, MX50_INT_GPIO4_HIGH);
+		mxc_add_gpio(4, MX50_GPIO5_BASE_ADDR, SZ_16K, MX50_INT_GPIO5_LOW, MX50_INT_GPIO5_HIGH);
+		mxc_add_gpio(5, MX50_GPIO6_BASE_ADDR, SZ_16K, MX50_INT_GPIO6_LOW, MX50_INT_GPIO6_HIGH);
+	}
+
+	if (cpu_is_mx51()) {
+		mxc_add_gpio(0, MX51_GPIO1_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO1_LOW, MX51_MXC_INT_GPIO1_HIGH);
+		mxc_add_gpio(1, MX51_GPIO2_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO2_LOW, MX51_MXC_INT_GPIO2_HIGH);
+		mxc_add_gpio(2, MX51_GPIO3_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO3_LOW, MX51_MXC_INT_GPIO3_HIGH);
+		mxc_add_gpio(3, MX51_GPIO4_BASE_ADDR, SZ_16K, MX51_MXC_INT_GPIO4_LOW, MX51_MXC_INT_GPIO4_HIGH);
+	}
+
+	if (cpu_is_mx53()) {
+		mxc_add_gpio(0, MX53_GPIO1_BASE_ADDR, SZ_16K, MX53_INT_GPIO1_LOW, MX53_INT_GPIO1_HIGH);
+		mxc_add_gpio(1, MX53_GPIO2_BASE_ADDR, SZ_16K, MX53_INT_GPIO2_LOW, MX53_INT_GPIO2_HIGH);
+		mxc_add_gpio(2, MX53_GPIO3_BASE_ADDR, SZ_16K, MX53_INT_GPIO3_LOW, MX53_INT_GPIO3_HIGH);
+		mxc_add_gpio(3, MX53_GPIO4_BASE_ADDR, SZ_16K, MX53_INT_GPIO4_LOW, MX53_INT_GPIO4_HIGH);
+		mxc_add_gpio(4, MX53_GPIO5_BASE_ADDR, SZ_16K, MX53_INT_GPIO5_LOW, MX53_INT_GPIO5_HIGH);
+		mxc_add_gpio(5, MX53_GPIO6_BASE_ADDR, SZ_16K, MX53_INT_GPIO6_LOW, MX53_INT_GPIO6_HIGH);
+		mxc_add_gpio(6, MX53_GPIO7_BASE_ADDR, SZ_16K, MX53_INT_GPIO7_LOW, MX53_INT_GPIO7_HIGH);
+	}

The above tables are pretty crazy, and they only get worse later in the series
when the other SoCs are added. Is it really worth it to have a common initcall
entry for the various SoCs here?

This common initcall seems a good place to concentrate the gpio device
registration.  It's easy to look at the common/different things among
these SoCs.  The only problem you reminded me is the scanning of the
long cpu_is_mx list.  It can be optimized a little bit by sorting the
list from the latest (most used) SoC to the oldest (least used) one,
and breaking out the scanning immediately when hitting one.

It'd seem cleaner to me to just call the registration function for the
family you're running from in per-family-init code such as where it was
removed above (irq init, which makes some sense since the gpios provide
interrupt sources as well).

I just gave it a test, and it's not working at all.  (too early
to register device in irq init?)

-- 
Regards,
Shawn