Re: [PATCH v2 1/1] pxa168: added initial support for TPO TD043MTEA1LCD display on Aspenite

From: Mark F. Brown
Date: Thu Aug 26 2010 - 00:04:42 EST

Next message: Minchan Kim: "Re: [PATCH/RFCv4 0/6] The Contiguous Memory Allocator framework"
Previous message: KOSAKI Motohiro: "Re: [PATCH] tcp: select(writefds) don't hang up when a peer close connection"
In reply to: Marek Vasut: "Re: [PATCH v2 1/1] pxa168: added initial support for TPO TD043MTEA1 LCD display on Aspenite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Makes sense!

Will do!

-- Mark

2010/8/26 Marek Vasut <marek.vasut@xxxxxxxxx>:
> 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@xxxxxxxxx> wrote:
>> > Dne Čt 26. srpna 2010 04:04:52 Mark F. Brown napsal(a):
>> >> Signed-off-by: Mark F. Brown <mark.brown314@xxxxxxxxx>
>> >> ---
>> >> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Minchan Kim: "Re: [PATCH/RFCv4 0/6] The Contiguous Memory Allocator framework"
Previous message: KOSAKI Motohiro: "Re: [PATCH] tcp: select(writefds) don't hang up when a peer close connection"
In reply to: Marek Vasut: "Re: [PATCH v2 1/1] pxa168: added initial support for TPO TD043MTEA1 LCD display on Aspenite"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]