Patchwork [2/3] ARM: mxs: add gpio-mxs platform devices

login
register
mail settings
Submitter Shawn Guo
Date May 20, 2011, 9:57 a.m.
Message ID <1305885446-27404-3-git-send-email-shawn.guo@linaro.org>
Download mbox | patch
Permalink /patch/96574/
State New
Headers show

Comments

Shawn Guo - May 20, 2011, 9:57 a.m.
Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/mach-mxs/devices/Makefile            |    1 +
 arch/arm/mach-mxs/devices/platform-gpio-mxs.c |   92 +++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/mach-mxs/devices/platform-gpio-mxs.c
Arnd Bergmann - May 20, 2011, 10:23 a.m.
On Friday 20 May 2011 11:57:25 Shawn Guo wrote:

> --- /dev/null
> +++ b/arch/arm/mach-mxs/devices/platform-gpio-mxs.c
> @@ -0,0 +1,92 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.
> + */

Hmm, I forgot to discuss this in Budapest, but I'm still not convinced about
the code ownership here, I think it should be Copyright Linaro Ltd when a
Linaro assignee writes it, not the member company that you work for.

If your manager thinks it should be copyright Freescale, I would suggest
we discuss it on the Linaro private mailing list so we can find a solution
that everyone is happy with. No need to bother the public with this.

> +#include <linux/compiler.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +
> +#include <mach/mx23.h>
> +#include <mach/mx28.h>
> +#include <mach/devices-common.h>
> +
> +struct mxs_gpio_data {
> +	int id;
> +	resource_size_t iobase;
> +	resource_size_t iosize;
> +	resource_size_t irq;
> +};

You don't use iosize anywhere.

> +#define mxs_gpio_data_entry_single(soc, _id)				\
> +	{								\
> +		.id = _id,						\
> +		.iobase = soc ## _PINCTRL ## _BASE_ADDR,		\
> +		.irq = soc ## _INT_GPIO ## _id,				\
> +	}
> +
> +#define mxs_gpio_data_entry(soc, _id)					\
> +	[_id] = mxs_gpio_data_entry_single(soc, _id)
> +
> +#ifdef CONFIG_SOC_IMX23
> +const struct mxs_gpio_data mx23_gpio_data[] __initconst = {
> +#define mx23_gpio_data_entry(_id)					\
> +	mxs_gpio_data_entry(MX23, _id)

I know it's tempting to use macros for these, but I think it obscures
the code a lot, especially when you use them to concatenate identifier
names. Why not just do:

	struct platform_device *gpios;
	gpios = platform_device_register_simple(mxs_host_bus, "mxs-gpio-master", 0, NULL, 0);

	mxs_register_gpio(gpios, 0, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_0);
	mxs_register_gpio(gpios, 1, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_1);
	mxs_register_gpio(gpios, 2, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_2);
	mxs_register_gpio(gpios, 3, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_3);

This is actually shorter and it makes it possible to grep for the
macros you use.

> +struct platform_device *__init mxs_add_gpio(
> +		const struct mxs_gpio_data *data)
> +{
> +	struct resource res[] = {
> +		{
> +			.start = data->iobase,
> +			.end = data->iobase + SZ_8K - 1,
> +			.flags = IORESOURCE_MEM,
> +		}, {
> +			.start = data->irq,
> +			.end = data->irq,
> +			.flags = IORESOURCE_IRQ,
> +		},
> +	};
> +
> +	return mxs_add_platform_device("mxs-gpio", data->id,
> +					res, ARRAY_SIZE(res), NULL, 0);
> +}

mxs_add_platform_device doesn't set the parent pointer correctly, I think you
should either fix that or open-code the platform device creation to do it
right.

	Arnd
Shawn Guo - May 20, 2011, 2:03 p.m.
On Fri, May 20, 2011 at 12:23:00PM +0200, Arnd Bergmann wrote:
> On Friday 20 May 2011 11:57:25 Shawn Guo wrote:
> 
> > --- /dev/null
> > +++ b/arch/arm/mach-mxs/devices/platform-gpio-mxs.c
> > @@ -0,0 +1,92 @@
> > +/*
> > + * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> > + *
> > + * 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.
> > + */
> 
> Hmm, I forgot to discuss this in Budapest, but I'm still not convinced about
> the code ownership here, I think it should be Copyright Linaro Ltd when a
> Linaro assignee writes it, not the member company that you work for.
> 
> If your manager thinks it should be copyright Freescale, I would suggest
> we discuss it on the Linaro private mailing list so we can find a solution
> that everyone is happy with. No need to bother the public with this.
> 
For this particular case, I started from copying platform-dma.c and
chose not to touch the copyright.

Speaking of the copyright between Linaro and Freescale, I would prefer
copyright both for most cases, as the patches from me will generally
be based on or referring to Freescale BSP.

Yes, we should discuss it in private.

> > +#include <linux/compiler.h>
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +
> > +#include <mach/mx23.h>
> > +#include <mach/mx28.h>
> > +#include <mach/devices-common.h>
> > +
> > +struct mxs_gpio_data {
> > +	int id;
> > +	resource_size_t iobase;
> > +	resource_size_t iosize;
> > +	resource_size_t irq;
> > +};
> 
> You don't use iosize anywhere.
> 
Sorry, it's a rushed copy/past.

> > +#define mxs_gpio_data_entry_single(soc, _id)				\
> > +	{								\
> > +		.id = _id,						\
> > +		.iobase = soc ## _PINCTRL ## _BASE_ADDR,		\
> > +		.irq = soc ## _INT_GPIO ## _id,				\
> > +	}
> > +
> > +#define mxs_gpio_data_entry(soc, _id)					\
> > +	[_id] = mxs_gpio_data_entry_single(soc, _id)
> > +
> > +#ifdef CONFIG_SOC_IMX23
> > +const struct mxs_gpio_data mx23_gpio_data[] __initconst = {
> > +#define mx23_gpio_data_entry(_id)					\
> > +	mxs_gpio_data_entry(MX23, _id)
> 
> I know it's tempting to use macros for these, but I think it obscures
> the code a lot, especially when you use them to concatenate identifier
> names. Why not just do:
> 
The pattern is being widely used in mxc/mxs platform device codes.
If you are not extremely unhappy about this, I would leave it as it
is to keep consistency.

> 	struct platform_device *gpios;
> 	gpios = platform_device_register_simple(mxs_host_bus, "mxs-gpio-master", 0, NULL, 0);
> 
> 	mxs_register_gpio(gpios, 0, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_0);
> 	mxs_register_gpio(gpios, 1, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_1);
> 	mxs_register_gpio(gpios, 2, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_2);
> 	mxs_register_gpio(gpios, 3, MX23_PINCTRL_BASE_ADDR, MX23_INT_GPIO_3);
> 
> This is actually shorter and it makes it possible to grep for the
> macros you use.
> 
> > +struct platform_device *__init mxs_add_gpio(
> > +		const struct mxs_gpio_data *data)
> > +{
> > +	struct resource res[] = {
> > +		{
> > +			.start = data->iobase,
> > +			.end = data->iobase + SZ_8K - 1,
> > +			.flags = IORESOURCE_MEM,
> > +		}, {
> > +			.start = data->irq,
> > +			.end = data->irq,
> > +			.flags = IORESOURCE_IRQ,
> > +		},
> > +	};
> > +
> > +	return mxs_add_platform_device("mxs-gpio", data->id,
> > +					res, ARRAY_SIZE(res), NULL, 0);
> > +}
> 
> mxs_add_platform_device doesn't set the parent pointer correctly, I think you
> should either fix that or open-code the platform device creation to do it
> right.
> 
I see the following in drivers/base/platform.c, function
platform_device_add():

        if (!pdev->dev.parent)
                pdev->dev.parent = &platform_bus;

So we are fine?
Arnd Bergmann - May 20, 2011, 2:23 p.m.
On Friday 20 May 2011 16:03:17 Shawn Guo wrote:
> On Fri, May 20, 2011 at 12:23:00PM +0200, Arnd Bergmann wrote:
> > 
> > I know it's tempting to use macros for these, but I think it obscures
> > the code a lot, especially when you use them to concatenate identifier
> > names. Why not just do:
> > 
> The pattern is being widely used in mxc/mxs platform device codes.
> If you are not extremely unhappy about this, I would leave it as it
> is to keep consistency.

I think it's a real pain to do it like this, and we need to start
at some point cleaning up the mess. Why not start now?

> > > +
> > > +	return mxs_add_platform_device("mxs-gpio", data->id,
> > > +					res, ARRAY_SIZE(res), NULL, 0);
> > > +}
> > 
> > mxs_add_platform_device doesn't set the parent pointer correctly, I think you
> > should either fix that or open-code the platform device creation to do it
> > right.
> > 
> I see the following in drivers/base/platform.c, function
> platform_device_add():
> 
>         if (!pdev->dev.parent)
>                 pdev->dev.parent = &platform_bus;
> 
> So we are fine?

No, this would put all gpio devices below the top-level /sys/devices/platform
directory, where they certainly don't belong. Please find a proper
place and put them there.

	Arnd
Shawn Guo - May 20, 2011, 2:40 p.m.
On Fri, May 20, 2011 at 04:23:40PM +0200, Arnd Bergmann wrote:
> On Friday 20 May 2011 16:03:17 Shawn Guo wrote:
> > On Fri, May 20, 2011 at 12:23:00PM +0200, Arnd Bergmann wrote:
> > > 
> > > I know it's tempting to use macros for these, but I think it obscures
> > > the code a lot, especially when you use them to concatenate identifier
> > > names. Why not just do:
> > > 
> > The pattern is being widely used in mxc/mxs platform device codes.
> > If you are not extremely unhappy about this, I would leave it as it
> > is to keep consistency.
> 
> I think it's a real pain to do it like this, and we need to start
> at some point cleaning up the mess. Why not start now?
> 
OK

> > > > +
> > > > +	return mxs_add_platform_device("mxs-gpio", data->id,
> > > > +					res, ARRAY_SIZE(res), NULL, 0);
> > > > +}
> > > 
> > > mxs_add_platform_device doesn't set the parent pointer correctly, I think you
> > > should either fix that or open-code the platform device creation to do it
> > > right.
> > > 
> > I see the following in drivers/base/platform.c, function
> > platform_device_add():
> > 
> >         if (!pdev->dev.parent)
> >                 pdev->dev.parent = &platform_bus;
> > 
> > So we are fine?
> 
> No, this would put all gpio devices below the top-level /sys/devices/platform
> directory, where they certainly don't belong. Please find a proper
> place and put them there.
> 
Understood.  Will do.
Shawn Guo - May 27, 2011, 8:29 a.m.
On Fri, May 20, 2011 at 12:23:00PM +0200, Arnd Bergmann wrote:
[...]
> > +#define mxs_gpio_data_entry_single(soc, _id)				\
> > +	{								\
> > +		.id = _id,						\
> > +		.iobase = soc ## _PINCTRL ## _BASE_ADDR,		\
> > +		.irq = soc ## _INT_GPIO ## _id,				\
> > +	}
> > +
> > +#define mxs_gpio_data_entry(soc, _id)					\
> > +	[_id] = mxs_gpio_data_entry_single(soc, _id)
> > +
> > +#ifdef CONFIG_SOC_IMX23
> > +const struct mxs_gpio_data mx23_gpio_data[] __initconst = {
> > +#define mx23_gpio_data_entry(_id)					\
> > +	mxs_gpio_data_entry(MX23, _id)
> 
> I know it's tempting to use macros for these, but I think it obscures
> the code a lot, especially when you use them to concatenate identifier
> names. Why not just do:
> 
> 	struct platform_device *gpios;
> 	gpios = platform_device_register_simple(mxs_host_bus, "mxs-gpio-master", 0, NULL, 0);
> 
platform_device_register_simple does not have parameter for 'parent',
and it sets 'parent' NULL anyway.
Arnd Bergmann - May 27, 2011, 8:48 a.m.
On Friday 27 May 2011, Shawn Guo wrote:

> > I know it's tempting to use macros for these, but I think it obscures
> > the code a lot, especially when you use them to concatenate identifier
> > names. Why not just do:
> > 
> >       struct platform_device *gpios;
> >       gpios = platform_device_register_simple(mxs_host_bus, "mxs-gpio-master", 0, NULL, 0);
> > 
> platform_device_register_simple does not have parameter for 'parent',
> and it sets 'parent' NULL anyway.
> 

Oops, my mistake. just use platform_device_register_resndata directly then.

	Arnd
Shawn Guo - May 27, 2011, 9:11 a.m.
On Fri, May 20, 2011 at 04:23:40PM +0200, Arnd Bergmann wrote:
[...]
> No, this would put all gpio devices below the top-level /sys/devices/platform
> directory, where they certainly don't belong. Please find a proper
> place and put them there.
> 
To make it clear, which one is the best to have all gpio devices below?

 * /sys/devices/platform/gpio-mxs
 * /sys/devices/platform/mxs
 * /sys/devices/platform/mxs/gpio
Arnd Bergmann - May 27, 2011, 11:10 a.m.
On Friday 27 May 2011, Shawn Guo wrote:
> On Fri, May 20, 2011 at 04:23:40PM +0200, Arnd Bergmann wrote:
> [...]
> > No, this would put all gpio devices below the top-level /sys/devices/platform
> > directory, where they certainly don't belong. Please find a proper
> > place and put them there.
> > 
> To make it clear, which one is the best to have all gpio devices below?
> 
>  * /sys/devices/platform/gpio-mxs
>  * /sys/devices/platform/mxs
>  * /sys/devices/platform/mxs/gpio

I would say the third one is closes to how it should be.

The (sysfs) device tree, like the of device tree, should ideally be
modeled after the high-level block diagram of the machine.

In case of i.MX23, that could look like

/sys/devices/system \
  /ahb
    /apbh
       /gpio0
       /gpio1
       /gpmi
    /usb0
    /apbx
       /i2c
       /uart0
       /pwm
       /spi
          /mmc
          /ethernet
       /usb
       /rtc
       /...


You can argue on whether it makes sense to include the top level, or if you
also want to model the various levels of ahb buses, so there is some freedom
here.

	Arnd

Patch

diff --git a/arch/arm/mach-mxs/devices/Makefile b/arch/arm/mach-mxs/devices/Makefile
index 324f282..351915c 100644
--- a/arch/arm/mach-mxs/devices/Makefile
+++ b/arch/arm/mach-mxs/devices/Makefile
@@ -6,4 +6,5 @@  obj-$(CONFIG_MXS_HAVE_PLATFORM_FLEXCAN) += platform-flexcan.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_I2C) += platform-mxs-i2c.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_MMC) += platform-mxs-mmc.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_MXS_PWM) += platform-mxs-pwm.o
+obj-y += platform-gpio-mxs.o
 obj-$(CONFIG_MXS_HAVE_PLATFORM_MXSFB) += platform-mxsfb.o
diff --git a/arch/arm/mach-mxs/devices/platform-gpio-mxs.c b/arch/arm/mach-mxs/devices/platform-gpio-mxs.c
new file mode 100644
index 0000000..3840d8c
--- /dev/null
+++ b/arch/arm/mach-mxs/devices/platform-gpio-mxs.c
@@ -0,0 +1,92 @@ 
+/*
+ * Copyright 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * 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/mx23.h>
+#include <mach/mx28.h>
+#include <mach/devices-common.h>
+
+struct mxs_gpio_data {
+	int id;
+	resource_size_t iobase;
+	resource_size_t iosize;
+	resource_size_t irq;
+};
+
+#define mxs_gpio_data_entry_single(soc, _id)				\
+	{								\
+		.id = _id,						\
+		.iobase = soc ## _PINCTRL ## _BASE_ADDR,		\
+		.irq = soc ## _INT_GPIO ## _id,				\
+	}
+
+#define mxs_gpio_data_entry(soc, _id)					\
+	[_id] = mxs_gpio_data_entry_single(soc, _id)
+
+#ifdef CONFIG_SOC_IMX23
+const struct mxs_gpio_data mx23_gpio_data[] __initconst = {
+#define mx23_gpio_data_entry(_id)					\
+	mxs_gpio_data_entry(MX23, _id)
+	mx23_gpio_data_entry(0),
+	mx23_gpio_data_entry(1),
+	mx23_gpio_data_entry(2),
+};
+#endif
+
+#ifdef CONFIG_SOC_IMX28
+const struct mxs_gpio_data mx28_gpio_data[] __initconst = {
+#define mx28_gpio_data_entry(_id)					\
+	mxs_gpio_data_entry(MX28, _id)
+	mx28_gpio_data_entry(0),
+	mx28_gpio_data_entry(1),
+	mx28_gpio_data_entry(2),
+	mx28_gpio_data_entry(3),
+	mx28_gpio_data_entry(4),
+};
+#endif
+
+struct platform_device *__init mxs_add_gpio(
+		const struct mxs_gpio_data *data)
+{
+	struct resource res[] = {
+		{
+			.start = data->iobase,
+			.end = data->iobase + SZ_8K - 1,
+			.flags = IORESOURCE_MEM,
+		}, {
+			.start = data->irq,
+			.end = data->irq,
+			.flags = IORESOURCE_IRQ,
+		},
+	};
+
+	return mxs_add_platform_device("mxs-gpio", data->id,
+					res, ARRAY_SIZE(res), NULL, 0);
+}
+
+static int __init mxs_add_mxs_gpio(void)
+{
+	int i;
+
+#ifdef CONFIG_SOC_IMX23
+	if (cpu_is_mx23())
+		for (i = 0; i < ARRAY_SIZE(mx23_gpio_data); i++)
+			mxs_add_gpio(&mx23_gpio_data[i]);
+#endif
+
+#ifdef CONFIG_SOC_IMX28
+	if (cpu_is_mx28())
+		for (i = 0; i < ARRAY_SIZE(mx28_gpio_data); i++)
+			mxs_add_gpio(&mx28_gpio_data[i]);
+#endif
+
+	return 0;
+}
+postcore_initcall(mxs_add_mxs_gpio);