| Message ID | 1305885446-27404-3-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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?
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
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.
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.
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
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
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
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);
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