DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=from:organization:to:subject:date:user-agent:cc:references
         :in-reply-to:mime-version:content-type:content-transfer-encoding
         :message-id;
        b=mxnvEQ2rgx9k+FYn2Y5SpkSOzI4TXQJPoKMaBStYaWlEeIyfduqVBnwMvNL96a1voV
         nSB1OMMKCriHvY76JJIipcJnZKhGCNsxr5z2N2rGSeDgL9Wzd7G3LefGCG8c6sK/67Q7
         xWzELkhmfLIpgT6OCXIA5ktAH3QuVZwfvZqrs=
From: Marek Vasut <marek.vasut@gmail.com>
Organization: Hack&Dev
To: "Mark F. Brown" <mark.brown314@gmail.com>
Subject: Re: [PATCH v2 1/1] pxa168: added initial support for TPO TD043MTEA1 LCD display on Aspenite
Date: Thu, 26 Aug 2010 06:02:20 +0200
User-Agent: KMail/1.13.3 (Linux/2.6.34-1-amd64; KDE/4.4.4; x86_64; ; )
Cc: linux-arm-kernel@lists.infradead.org, eric.y.miao@gmail.com,
        linux-kernel@vger.kernel.org, Haojian Zhuang <hzhuang1@marvell.com>
References: <1282788292-5484-1-git-send-email-mark.brown314@gmail.com> <201008260426.39963.marek.vasut@gmail.com> <AANLkTin9Yq3v1HO0PH9t2XjqLtxn8_+0keG=1JLQ0=OA@mail.gmail.com>
In-Reply-To: <AANLkTin9Yq3v1HO0PH9t2XjqLtxn8_+0keG=1JLQ0=OA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-2"
Content-Transfer-Encoding: 8BIT
Message-Id: <201008260602.20685.marek.vasut@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 9346
Lines: 251

Dne ?t 26. srpna 2010 05:44:48 Mark F. Brown napsal(a):
> On Wed, Aug 25, 2010 at 10:26 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> > Dne ?t 26. srpna 2010 04:04:52 Mark F. Brown napsal(a):
> >> Signed-off-by: Mark F. Brown <mark.brown314@gmail.com>
> >> ---
> >>  arch/arm/mach-mmp/aspenite.c            |   60
> >> +++++++++++++++++++++++++++++++ arch/arm/mach-mmp/include/mach/pxa168.h
> >> | 8 ++++
> >>  arch/arm/mach-mmp/pxa168.c              |    4 ++
> >>  drivers/video/pxa168fb.c                |    4 +-
> >>  4 files changed, 74 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/arm/mach-mmp/aspenite.c b/arch/arm/mach-mmp/aspenite.c
> >> index 0629394..9e1bd6b 100644
> >> --- a/arch/arm/mach-mmp/aspenite.c
> >> +++ b/arch/arm/mach-mmp/aspenite.c
> >> @@ -23,6 +23,7 @@
> >>  #include <mach/mfp-pxa168.h>
> >>  #include <mach/pxa168.h>
> >>  #include <mach/gpio.h>
> >> +#include <video/pxa168fb.h>
> >> 
> >>  #include "common.h"
> >> 
> >> @@ -66,6 +67,36 @@ static unsigned long common_pin_config[] __initdata =
> >> { GPIO115_I2S_BCLK,
> >>       GPIO116_I2S_RXD,
> >>       GPIO117_I2S_TXD,
> >> +
> >> +     /* LCD */
> >> +     GPIO56_LCD_FCLK_RD,
> >> +     GPIO57_LCD_LCLK_A0,
> >> +     GPIO58_LCD_PCLK_WR,
> >> +     GPIO59_LCD_DENA_BIAS,
> >> +     GPIO60_LCD_DD0,
> >> +     GPIO61_LCD_DD1,
> >> +     GPIO62_LCD_DD2,
> >> +     GPIO63_LCD_DD3,
> >> +     GPIO64_LCD_DD4,
> >> +     GPIO65_LCD_DD5,
> >> +     GPIO66_LCD_DD6,
> >> +     GPIO67_LCD_DD7,
> >> +     GPIO68_LCD_DD8,
> >> +     GPIO69_LCD_DD9,
> >> +     GPIO70_LCD_DD10,
> >> +     GPIO71_LCD_DD11,
> >> +     GPIO72_LCD_DD12,
> >> +     GPIO73_LCD_DD13,
> >> +     GPIO74_LCD_DD14,
> >> +     GPIO75_LCD_DD15,
> >> +     GPIO76_LCD_DD16,
> >> +     GPIO77_LCD_DD17,
> >> +     GPIO78_LCD_DD18,
> >> +     GPIO79_LCD_DD19,
> >> +     GPIO80_LCD_DD20,
> >> +     GPIO81_LCD_DD21,
> >> +     GPIO82_LCD_DD22,
> >> +     GPIO83_LCD_DD23,
> >>  };
> >> 
> >>  static struct smc91x_platdata smc91x_info = {
> >> @@ -134,6 +165,34 @@ static struct i2c_board_info aspenite_i2c_info[]
> >> __initdata = { { I2C_BOARD_INFO("wm8753", 0x1b), },
> >>  };
> >> 
> >> +static struct fb_videomode video_modes[] = {
> >> +     [0] = {
> >> +             .pixclock       = 30120,
> >> +             .refresh        = 60,
> >> +             .xres           = 800,
> >> +             .yres           = 480,
> >> +             .hsync_len      = 1,
> >> +             .left_margin    = 215,
> >> +             .right_margin   = 40,
> >> +             .vsync_len      = 1,
> >> +             .upper_margin   = 34,
> >> +             .lower_margin   = 10,
> >> +             .sync           = FB_SYNC_VERT_HIGH_ACT |
> >> FB_SYNC_HOR_HIGH_ACT, +     },
> >> +};
> >> +
> >> +struct pxa168fb_mach_info aspenite_lcd_info = {
> >> +     .id                     = "Graphic Frame",
> >> +     .modes                  = video_modes,
> >> +     .num_modes              = ARRAY_SIZE(video_modes),
> >> +     .pix_fmt                = PIX_FMT_RGB565,
> >> +     .io_pin_allocation_mode = PIN_MODE_DUMB_24,
> >> +     .dumb_mode              = DUMB_MODE_RGB888,
> >> +     .active                 = 1,
> >> +     .panel_rbswap           = 0,
> >> +     .invert_pixclock        = 0,
> >> +};
> >> +
> >>  static void __init common_init(void)
> >>  {
> >>       mfp_config(ARRAY_AND_SIZE(common_pin_config));
> >> @@ -143,6 +202,7 @@ static void __init common_init(void)
> >>       pxa168_add_twsi(1, NULL, ARRAY_AND_SIZE(aspenite_i2c_info));
> >>       pxa168_add_ssp(1);
> >>       pxa168_add_nand(&aspenite_nand_info);
> >> +     pxa168_add_fb(&aspenite_lcd_info);
> >> 
> >>       /* off-chip devices */
> >>       platform_device_register(&smc91x_device);
> > 
> > ----------------------------------------------
> > ^--- one patch above
> > SNIP, please split into two patches
> > v--- another patch below
> > ----------------------------------------------
> > 
> >> diff --git a/arch/arm/mach-mmp/include/mach/pxa168.h
> >> b/arch/arm/mach-mmp/include/mach/pxa168.h index 27e1bc7..220738f 100644
> >> --- a/arch/arm/mach-mmp/include/mach/pxa168.h
> >> +++ b/arch/arm/mach-mmp/include/mach/pxa168.h
> >> @@ -10,6 +10,7 @@ extern void __init pxa168_init_irq(void);
> >>  #include <mach/devices.h>
> >>  #include <plat/i2c.h>
> >>  #include <plat/pxa3xx_nand.h>
> >> +#include <video/pxa168fb.h>
> >> 
> >>  extern struct pxa_device_desc pxa168_device_uart1;
> >>  extern struct pxa_device_desc pxa168_device_uart2;
> >> @@ -25,6 +26,7 @@ extern struct pxa_device_desc pxa168_device_ssp3;
> >>  extern struct pxa_device_desc pxa168_device_ssp4;
> >>  extern struct pxa_device_desc pxa168_device_ssp5;
> >>  extern struct pxa_device_desc pxa168_device_nand;
> >> +extern struct pxa_device_desc pxa168_device_fb;
> >> 
> >>  static inline int pxa168_add_uart(int id)
> >>  {
> >> @@ -97,4 +99,10 @@ static inline int pxa168_add_nand(struct
> >> pxa3xx_nand_platform_data *info) {
> >>       return pxa_register_device(&pxa168_device_nand, info,
> >> sizeof(*info)); }
> >> +
> >> +static inline int pxa168_add_fb(struct pxa168fb_mach_info *mi)
> >> +{
> >> +     return pxa_register_device(&pxa168_device_fb, mi, sizeof(*mi));
> >> +}
> >> +
> >>  #endif /* __ASM_MACH_PXA168_H */
> >> diff --git a/arch/arm/mach-mmp/pxa168.c b/arch/arm/mach-mmp/pxa168.c
> >> index 652ae66..886eadb 100644
> >> --- a/arch/arm/mach-mmp/pxa168.c
> >> +++ b/arch/arm/mach-mmp/pxa168.c
> >> @@ -25,6 +25,7 @@
> >>  #include <mach/dma.h>
> >>  #include <mach/devices.h>
> >>  #include <mach/mfp.h>
> >> +#include <video/pxa168fb.h>
> >> 
> >>  #include "common.h"
> >>  #include "clock.h"
> >> @@ -79,6 +80,7 @@ static APBC_CLK(ssp4, PXA168_SSP4, 4, 0);
> >>  static APBC_CLK(ssp5, PXA168_SSP5, 4, 0);
> >> 
> >>  static APMU_CLK(nand, NAND, 0x01db, 208000000);
> >> +static APMU_CLK(lcd, LCD, 0x7f, 312000000);
> >> 
> >>  /* device and clock bindings */
> >>  static struct clk_lookup pxa168_clkregs[] = {
> >> @@ -96,6 +98,7 @@ static struct clk_lookup pxa168_clkregs[] = {
> >>       INIT_CLKREG(&clk_ssp4, "pxa168-ssp.3", NULL),
> >>       INIT_CLKREG(&clk_ssp5, "pxa168-ssp.4", NULL),
> >>       INIT_CLKREG(&clk_nand, "pxa3xx-nand", NULL),
> >> +     INIT_CLKREG(&clk_lcd, "pxa168-fb", NULL),
> >>  };
> >> 
> >>  static int __init pxa168_init(void)
> >> @@ -147,3 +150,4 @@ PXA168_DEVICE(ssp2, "pxa168-ssp", 1, SSP2,
> >> 0xd401c000, 0x40, 54, 55); PXA168_DEVICE(ssp3, "pxa168-ssp", 2, SSP3,
> >> 0xd401f000, 0x40, 56, 57); PXA168_DEVICE(ssp4, "pxa168-ssp", 3, SSP4,
> >> 0xd4020000, 0x40, 58, 59); PXA168_DEVICE(ssp5, "pxa168-ssp", 4, SSP5,
> >> 0xd4021000, 0x40, 60, 61); +PXA168_DEVICE(fb, "pxa168-fb", -1, LCD,
> >> 0xd420b000, 0x1c8);
> >> diff --git a/drivers/video/pxa168fb.c b/drivers/video/pxa168fb.c
> >> index c91a7f7..d3e597e 100644
> >> --- a/drivers/video/pxa168fb.c
> >> +++ b/drivers/video/pxa168fb.c
> >> @@ -559,7 +559,7 @@ static struct fb_ops pxa168fb_ops = {
> >>       .fb_imageblit   = cfb_imageblit,
> >>  };
> >> 
> >> -static int __init pxa168fb_init_mode(struct fb_info *info,
> >> +static int pxa168fb_init_mode(struct fb_info *info,
> >>                             struct pxa168fb_mach_info *mi)
> > 
> > Why do you remove __init from it ?
> > 
> >>  {
> >>       struct pxa168fb_info *fbi = info->par;
> >> @@ -599,7 +599,7 @@ static int __init pxa168fb_init_mode(struct fb_info
> >> *info, return ret;
> >>  }
> >> 
> >> -static int __init pxa168fb_probe(struct platform_device *pdev)
> >> +static int pxa168fb_probe(struct platform_device *pdev)
> > 
> > Why do you remove __init from it ? (DTTO)
> > 
> >>  {
> >>       struct pxa168fb_mach_info *mi;
> >>       struct fb_info *info = 0;
> > 
> > Cheers
> 
> Hi Marek,
> 
Hi,

Yep, always keep CC :-)

> 1) I removed the __init because it was causing a section mismatch
> warning wth this kernel. Since pxa168fb_probe() is calling
> pxa168fb_init_mode(), I cannot declare pxa168fb_init_mode() as an
> __init function. If pxa168fb_probe gets called after the __init
> section is wiped then it will attempt to run the deleted
> pxa168fb_init_mode() code.
> 
> WARNING: vmlinux.o(.data+0x17984): Section mismatch in reference from
> the variable pxa168fb_driver to the function
> .init.text:pxa168fb_probe()
> The variable pxa168fb_driver references
> the function __init pxa168fb_probe()
> If the reference is valid then annotate the
> variable with __init* or __refdata (see linux/init.h) or name the variable:
> *_template, *_timer, *_sht, *_ops, *_probe, *_probe_one, *_console,
> 

Thanks for reporting this, please see my patch.
> 
> 2) What is the logic behind the delineation between patch files? I
> cannot build patch #1 without patch #2 based on how you defined the
> split. I need all of it in order to function.

Create small patches that do not break stuff and are easy to review. By adding 
small touches, you actually create a nice whole picture in the end :)

What I'd like to see is you to keep aspenite.c additions separate from cpu-wide 
additions etc. This eventually also simplifies bisection if something goes 
wrong.

Cheers
> 
> -- Mark
--
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/