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

From: Shawn Guo
Date: Mon May 30 2011 - 22:48:33 EST


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:
> > It adds platform device for drivers/gpio/gpio-mxc, and migrates
> > mx50/mx51/mx53 gpio driver to gpio-mxc.
> >
>
> [...]
>
> > 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?

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/