[PATCH 2/2] ARM i.MX23/28: Add framebuffer device support
Shawn Guo
shawn.guo at freescale.com
Fri Feb 18 04:17:18 EST 2011
Hi Sascha,
On Fri, Feb 18, 2011 at 09:46:25AM +0100, Sascha Hauer wrote:
> On Fri, Feb 18, 2011 at 02:14:41PM +0800, Shawn Guo wrote:
> > On Wed, Feb 16, 2011 at 10:56:39AM +0100, Sascha Hauer wrote:
> > > Signed-off-by: Sascha Hauer <s.hauer at pengutronix.de>
> > > Cc: Shawn Guo <shawn.guo at freescale.com>
> > > ---
> > > arch/arm/mach-mxs/clock-mx23.c | 2 +-
> > > arch/arm/mach-mxs/clock-mx28.c | 1 +
> > > arch/arm/mach-mxs/devices-mx23.h | 4 ++
> > > arch/arm/mach-mxs/devices-mx28.h | 4 ++
> > > arch/arm/mach-mxs/devices/Kconfig | 4 ++
> > > arch/arm/mach-mxs/devices/Makefile | 1 +
> > > arch/arm/mach-mxs/devices/platform-mxsfb.c | 46 ++++++++++++++++++++++++++++
> > > 7 files changed, 61 insertions(+), 1 deletions(-)
> > > create mode 100644 arch/arm/mach-mxs/devices/platform-mxsfb.c
> > >
> > > diff --git a/arch/arm/mach-mxs/clock-mx23.c b/arch/arm/mach-mxs/clock-mx23.c
> > > index ca72a05..bfc7f27 100644
> > > --- a/arch/arm/mach-mxs/clock-mx23.c
> > > +++ b/arch/arm/mach-mxs/clock-mx23.c
> > > @@ -446,7 +446,7 @@ static struct clk_lookup lookups[] = {
> > > _REGISTER_CLOCK(NULL, "hclk", hbus_clk)
> > > _REGISTER_CLOCK(NULL, "usb", usb_clk)
> > > _REGISTER_CLOCK(NULL, "audio", audio_clk)
> > > - _REGISTER_CLOCK(NULL, "pwm", pwm_clk)
> > Introducing the warning below ...
> >
> > arch/arm/mach-mxs/clock-mx23.c:430: warning: ‘pwm_clk’ defined but not used
>
> Right, it was not intended to remove the pwm here. Will fix.
>
> >
> > > + _REGISTER_CLOCK("imx23-fb", NULL, lcdif_clk)
> > > };
> > >
> > > static int clk_misc_init(void)
> > > diff --git a/arch/arm/mach-mxs/clock-mx28.c b/arch/arm/mach-mxs/clock-mx28.c
> > > index fd1c4c5..6a7ebcb 100644
> > > --- a/arch/arm/mach-mxs/clock-mx28.c
> > > +++ b/arch/arm/mach-mxs/clock-mx28.c
> > > @@ -620,6 +620,7 @@ static struct clk_lookup lookups[] = {
> > > _REGISTER_CLOCK(NULL, "pwm", pwm_clk)
> > > _REGISTER_CLOCK(NULL, "lradc", lradc_clk)
> > > _REGISTER_CLOCK(NULL, "spdif", spdif_clk)
> > > + _REGISTER_CLOCK("imx28-fb", NULL, lcdif_clk)
> > > };
> > >
> > > static int clk_misc_init(void)
> > > diff --git a/arch/arm/mach-mxs/devices-mx23.h b/arch/arm/mach-mxs/devices-mx23.h
> > > index 1256788..b9745a2 100644
> > > --- a/arch/arm/mach-mxs/devices-mx23.h
> > > +++ b/arch/arm/mach-mxs/devices-mx23.h
> > > @@ -10,7 +10,11 @@
> > > */
> > > #include <mach/mx23.h>
> > > #include <mach/devices-common.h>
> > > +#include <mach/fb.h>
> > >
> > Why do we have mxsfb platform device code breaking the consistency
> > that we are maintaining well so far?
>
> The rule is that we include header files where we need them.
> devices-common.h is not touched in this patch and it does not need
> mach/fb.h, hence it is not include there.
>
> >
> > Generally, we have this header included in devices-common.h
> >
> > > extern const struct amba_device mx23_duart_device __initconst;
> > > #define mx23_add_duart() \
> > > mxs_add_duart(&mx23_duart_device)
> > > +
> > > +struct platform_device *__init mx23_add_mxsfb(
> > > + const struct mxsfb_platform_data *pdata);
> >
> > Generally, this goes to devices-common.h
>
> No, devices-common.h only declares the mxs_* functions. There is no
> mxs_add_mxsfb in this patch which indeed would go to devices-common.h
>
Well, if you break the consistency in one place, you break the
consistency every. If you follow the convention to add mxs_add_fb
in platform_fb.c, declare it in devices-common.h, ..., everything
gets consistent. We will not have the particular and noticeable fb
device code everywhere.
> >
> > > diff --git a/arch/arm/mach-mxs/devices-mx28.h b/arch/arm/mach-mxs/devices-mx28.h
> > > index 33773a6..c902dc7 100644
> > > --- a/arch/arm/mach-mxs/devices-mx28.h
> > > +++ b/arch/arm/mach-mxs/devices-mx28.h
> > > @@ -10,6 +10,7 @@
> > > */
> > > #include <mach/mx28.h>
> > > #include <mach/devices-common.h>
> > > +#include <mach/fb.h>
> > >
> > > extern const struct amba_device mx28_duart_device __initconst;
> > > #define mx28_add_duart() \
> > > @@ -18,3 +19,6 @@ extern const struct amba_device mx28_duart_device __initconst;
> > > extern const struct mxs_fec_data mx28_fec_data[] __initconst;
> > > #define mx28_add_fec(id, pdata) \
> > > mxs_add_fec(&mx28_fec_data[id], pdata)
> > > +
> > > +struct platform_device *__init mx28_add_mxsfb(
> > > + const struct mxsfb_platform_data *pdata);
> >
> > ditto
> >
> > > diff --git a/arch/arm/mach-mxs/devices/Kconfig b/arch/arm/mach-mxs/devices/Kconfig
> > > index cf7dc1a..1538cb9 100644
> > > --- a/arch/arm/mach-mxs/devices/Kconfig
> > > +++ b/arch/arm/mach-mxs/devices/Kconfig
> > > @@ -4,3 +4,7 @@ config MXS_HAVE_AMBA_DUART
> > >
> > > config MXS_HAVE_PLATFORM_FEC
> > > bool
> > > +
> > > +config MXS_HAVE_PLATFORM_MXSFB
> > > + bool
> > > +
> >
> > I understand that "mxsfb" was picked up for the device name to
> > reflect the driver name. But since this is the device under mach- mxs
> > folder, can we simply call it "fb" just like the way you name fb.h?
> >
> > In this way, we have all mxs platform device naming schema aligned,
> > auart, duart, dma, fb, fec, flexcan, mmc ...
>
> I see it the other way round. mach/fb.h is too generic, I would prefer
> mach/mxsfb.h to be able to add support for a different framebuffer
> later. So I better change fb.h to mxsfb.h.
>
I'm fine with either way, but just wondering what kind of fb later
it is and its naming, relatively to mxsfs.
> >
> > > diff --git a/arch/arm/mach-mxs/devices/Makefile b/arch/arm/mach-mxs/devices/Makefile
> > > index d0a09f6..0cf8b48 100644
> > > --- a/arch/arm/mach-mxs/devices/Makefile
> > > +++ b/arch/arm/mach-mxs/devices/Makefile
> > > @@ -1,2 +1,3 @@
> > > obj-$(CONFIG_MXS_HAVE_AMBA_DUART) += amba-duart.o
> > > obj-$(CONFIG_MXS_HAVE_PLATFORM_FEC) += platform-fec.o
> > > +obj-$(CONFIG_MXS_HAVE_PLATFORM_MXSFB) += platform-mxsfb.o
> > > diff --git a/arch/arm/mach-mxs/devices/platform-mxsfb.c b/arch/arm/mach-mxs/devices/platform-mxsfb.c
> > > new file mode 100644
> > > index 0000000..632bbdc
> > > --- /dev/null
> > > +++ b/arch/arm/mach-mxs/devices/platform-mxsfb.c
> > > @@ -0,0 +1,46 @@
> > > +/*
> > > + * Copyright (C) 2011 Pengutronix, Sascha Hauer <s.hauer at 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 <asm/sizes.h>
> > > +#include <mach/mx23.h>
> > > +#include <mach/mx28.h>
> > > +#include <mach/devices-common.h>
> > > +#include <mach/fb.h>
> >
> > Generally, this goes to devices-common.h
> >
> > > +
> > > +#ifdef CONFIG_SOC_IMX23
> > > +struct platform_device *__init mx23_add_mxsfb(
> > > + const struct mxsfb_platform_data *pdata)
> > > +{
> > > + struct resource res[] = {
> > > + {
> > > + .start = MX23_LCDIF_BASE_ADDR,
> > > + .end = MX23_LCDIF_BASE_ADDR + SZ_8K - 1,
> > > + .flags = IORESOURCE_MEM,
> > > + },
> > > + };
> > > +
> > > + return mxs_add_platform_device_dmamask("imx23-fb", -1,
> > > + res, ARRAY_SIZE(res), pdata, sizeof(*pdata), DMA_BIT_MASK(32));
> > > +}
> > > +#endif /* ifdef CONFIG_SOC_IMX23 */
> > > +
> > > +#ifdef CONFIG_SOC_IMX28
> > > +struct platform_device *__init mx28_add_mxsfb(
> > > + const struct mxsfb_platform_data *pdata)
> > > +{
> > > + struct resource res[] = {
> > > + {
> > > + .start = MX28_LCDIF_BASE_ADDR,
> > > + .end = MX28_LCDIF_BASE_ADDR + SZ_8K - 1,
> > > + .flags = IORESOURCE_MEM,
> > > + },
> > > + };
> > > +
> > > + return mxs_add_platform_device_dmamask("imx28-fb", -1,
> > > + res, ARRAY_SIZE(res), pdata, sizeof(*pdata), DMA_BIT_MASK(32));
> > > +}
> > > +#endif /* ifdef CONFIG_SOC_IMX28 */
> >
> > Generally, we have macro mxs_fb_data_entry and function mxs_add_fb
> > in this file. It's nothing but all about consistency. We do not
> > want some later coming platform device looking at this as example,
> > and bring more inconsistency into mach-mxs platform device codes.
>
> My opinion on this is that we should not use complex ## macro constructs
> where not necessary. With a device which is only present once on the SoC
> it is not necessary, so I skippped it. And yes, if someone is in the
> same situation with a single device on a system, he actually should take
> this as an example. So no, I don't agree with you.
>
With a device presents on more than one SoC, it's also not necessary?
With this inconsistency introduced, you need always keep an eye on
the new device code to ensure that it's not looking at the improper
example.
Anyway, I'm not going to argue with you on this, it's your call,
since Russell is pulling i.mx bits from your tree ;)
--
Regards,
Shawn
More information about the linux-arm-kernel
mailing list