[6/9] ARM i.MX51: Add IPU device support
diff mbox series

Message ID 1291902441-24712-7-git-send-email-s.hauer@pengutronix.de
State New, archived
Headers show
Series
  • [RFC] i.MX51 Framebuffer support
Related show

Commit Message

Sascha Hauer Dec. 9, 2010, 1:47 p.m. UTC
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 arch/arm/mach-mx5/devices-imx51.h               |    4 ++
 arch/arm/plat-mxc/devices/Kconfig               |    4 ++
 arch/arm/plat-mxc/devices/Makefile              |    1 +
 arch/arm/plat-mxc/devices/platform-imx_ipuv3.c  |   47 +++++++++++++++++++++++
 arch/arm/plat-mxc/include/mach/devices-common.h |   10 +++++
 5 files changed, 66 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-mxc/devices/platform-imx_ipuv3.c

Comments

Arnd Bergmann Dec. 15, 2010, 3:49 p.m. UTC | #1
On Thursday 09 December 2010, Sascha Hauer wrote:
> +#define imx51_add_ipuv3(pdata) \
> +       imx_add_ipuv3(&imx51_ipuv3_data, pdata)

This looks  like a pointless abstraction, it does not make
the code smaller or easier to read. I know it's sometimes
tempting to use macros, but in most cases, you should try
not to.

> +#define imx51_ipuv3_data_entry_single(soc)				\
> +	{								\
> +		.iobase = soc ## _IPU_CTRL_BASE_ADDR,			\
> +		.irq_err = soc ## _INT_IPU_ERR,				\
> +		.irq = soc ## _INT_IPU_SYN,				\
> +	}
> +
> +#ifdef CONFIG_SOC_IMX51
> +const struct imx_ipuv3_data imx51_ipuv3_data __initconst =
> +	imx51_ipuv3_data_entry_single(MX51);
> +#endif /* ifdef CONFIG_SOC_IMX35 */

This looks really strange: You define a macro that is used
in only a single place, and the only user is a data structure
that references data from other files and is used elsewhere
as well.

Avoiding the macro would make it easier to grep for the use
of the identifiers.

If you put the data structure in the place where it is used,
you can avoid the #ifdef and make it static.

Also, the comment on the #endif does not match the #if.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Arnaud Patard (Rtp) Dec. 15, 2010, 4:26 p.m. UTC | #2
Arnd Bergmann <arnd@arndb.de> writes:

Hi,

> On Thursday 09 December 2010, Sascha Hauer wrote:
>> +#define imx51_add_ipuv3(pdata) \
>> +       imx_add_ipuv3(&imx51_ipuv3_data, pdata)
>
> This looks  like a pointless abstraction, it does not make
> the code smaller or easier to read. I know it's sometimes
> tempting to use macros, but in most cases, you should try
> not to.
>

it's how things have been handled atm in the imx code. I don't have
any preference on this at all but at least either we go on with it 
or we get rid of all theses #defines. It's a matter of consistency.
The thing is that I would consider removing the imx*add* stuff to be
a cleanup and should be done in a different patch, not in a patchset
adding IPU support for imx51.

Anyway, let's wait for Sascha's point of view, he knows the imx stuff
far better than me.

Arnaud
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Arnd Bergmann Dec. 15, 2010, 4:29 p.m. UTC | #3
On Wednesday 15 December 2010, Arnaud Patard wrote:
> The thing is that I would consider removing the imx*add* stuff to be
> a cleanup and should be done in a different patch, not in a patchset
> adding IPU support for imx51.

Yes, that sounds reasonable.

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

Patch
diff mbox series

diff --git a/arch/arm/mach-mx5/devices-imx51.h b/arch/arm/mach-mx5/devices-imx51.h
index 6302e46..851c114 100644
--- a/arch/arm/mach-mx5/devices-imx51.h
+++ b/arch/arm/mach-mx5/devices-imx51.h
@@ -47,3 +47,7 @@  extern const struct imx_spi_imx_data imx51_ecspi_data[] __initconst;
 extern const struct imx_imx2_wdt_data imx51_imx2_wdt_data[] __initconst;
 #define imx51_add_imx2_wdt(id, pdata)	\
 	imx_add_imx2_wdt(&imx51_imx2_wdt_data[id])
+
+extern const struct imx_ipuv3_data imx51_ipuv3_data __initconst;
+#define imx51_add_ipuv3(pdata)	\
+	imx_add_ipuv3(&imx51_ipuv3_data, pdata)
diff --git a/arch/arm/plat-mxc/devices/Kconfig b/arch/arm/plat-mxc/devices/Kconfig
index 2537166..262d9c5 100644
--- a/arch/arm/plat-mxc/devices/Kconfig
+++ b/arch/arm/plat-mxc/devices/Kconfig
@@ -71,3 +71,7 @@  config IMX_HAVE_PLATFORM_SDHCI_ESDHC_IMX
 
 config IMX_HAVE_PLATFORM_SPI_IMX
 	bool
+
+config IMX_HAVE_PLATFORM_IMX_IPUV3
+	bool
+
diff --git a/arch/arm/plat-mxc/devices/Makefile b/arch/arm/plat-mxc/devices/Makefile
index 75cd2ec..0a6be0a 100644
--- a/arch/arm/plat-mxc/devices/Makefile
+++ b/arch/arm/plat-mxc/devices/Makefile
@@ -22,3 +22,4 @@  obj-$(CONFIG_IMX_HAVE_PLATFORM_MXC_RNGA) += platform-mxc_rnga.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_MXC_W1) += platform-mxc_w1.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_SDHCI_ESDHC_IMX) += platform-sdhci-esdhc-imx.o
 obj-$(CONFIG_IMX_HAVE_PLATFORM_SPI_IMX) +=  platform-spi_imx.o
+obj-$(CONFIG_IMX_HAVE_PLATFORM_IMX_IPUV3) +=  platform-imx_ipuv3.o
diff --git a/arch/arm/plat-mxc/devices/platform-imx_ipuv3.c b/arch/arm/plat-mxc/devices/platform-imx_ipuv3.c
new file mode 100644
index 0000000..a470edb
--- /dev/null
+++ b/arch/arm/plat-mxc/devices/platform-imx_ipuv3.c
@@ -0,0 +1,47 @@ 
+/*
+ * Copyright (C) 2010 Pengutronix
+ * Uwe Kleine-Koenig <u.kleine-koenig@pengutronix.de>
+ *
+ * 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 <mach/hardware.h>
+#include <mach/devices-common.h>
+
+#define imx51_ipuv3_data_entry_single(soc)				\
+	{								\
+		.iobase = soc ## _IPU_CTRL_BASE_ADDR,			\
+		.irq_err = soc ## _INT_IPU_ERR,				\
+		.irq = soc ## _INT_IPU_SYN,				\
+	}
+
+#ifdef CONFIG_SOC_IMX51
+const struct imx_ipuv3_data imx51_ipuv3_data __initconst =
+	imx51_ipuv3_data_entry_single(MX51);
+#endif /* ifdef CONFIG_SOC_IMX35 */
+
+struct platform_device *__init imx_add_ipuv3(
+		const struct imx_ipuv3_data *data,
+		const struct imx_ipuv3_platform_data *pdata)
+{
+	struct resource res[] = {
+		{
+			.start = data->iobase,
+			.end = data->iobase + SZ_4K - 1,
+			.flags = IORESOURCE_MEM,
+		}, {
+			.start = data->irq_err,
+			.end = data->irq_err,
+			.flags = IORESOURCE_IRQ,
+		}, {
+			.start = data->irq,
+			.end = data->irq,
+			.flags = IORESOURCE_IRQ,
+		},
+	};
+
+	return imx_add_platform_device("imx-ipuv3", -1,
+			res, ARRAY_SIZE(res), pdata, sizeof(*pdata));
+}
+
diff --git a/arch/arm/plat-mxc/include/mach/devices-common.h b/arch/arm/plat-mxc/include/mach/devices-common.h
index 8658c9c..8f5197f 100644
--- a/arch/arm/plat-mxc/include/mach/devices-common.h
+++ b/arch/arm/plat-mxc/include/mach/devices-common.h
@@ -264,3 +264,13 @@  struct imx_spi_imx_data {
 struct platform_device *__init imx_add_spi_imx(
 		const struct imx_spi_imx_data *data,
 		const struct spi_imx_master *pdata);
+
+#include <mach/ipu-v3.h>
+struct imx_ipuv3_data {
+	resource_size_t iobase;
+	resource_size_t irq_err;
+	resource_size_t irq;
+};
+struct platform_device *__init imx_add_ipuv3(
+		const struct imx_ipuv3_data *data,
+		const struct imx_ipuv3_platform_data *pdata);