| Message ID | 20090916011506.GC32194@mag.az.mvista.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hello. Mark A. Greer wrote: > From: Steve Chen <schen@mvista.com> Hm, is it Steve's patch? I'm seeing my stuff here. > Add graphics support for the Sharp LCD035Q3DG01 graphical > LCD that's on the User Interface (UI) daughter card of the > DA830/OMAP-L137 EVM. > > The LCD shares EMIFA lines with the NAND and NOR devices that > are also on the UI card so those lines are shared via a couple > of muxes. The muxes are controlled by the 'MUX_MODE' line on > the UI card. The 'MUX_MODE' line is controlled by pin P6 of > a pcf8574 i2c expander that's at i2c address 0x3f on UI card. > The i2c expander is controlled using the gpio infrastructure > from the board code using the 'setup()' and 'teardown()' > routines. > > Signed-off-by: Steve Chen <schen@mvista.com> > Signed-off-by: Mark A. Greer <mgreer@mvista.com> This patch really misses my signoff... > --- > arch/arm/mach-davinci/Kconfig | 21 ++++++++++++++ > arch/arm/mach-davinci/board-da830-evm.c | 44 ++++++++++++++++++++++++++++++- > arch/arm/mach-davinci/da830.c | 2 +- > 3 files changed, 65 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig > index 40866c6..7b6dddf 100644 > --- a/arch/arm/mach-davinci/Kconfig > +++ b/arch/arm/mach-davinci/Kconfig > @@ -101,6 +101,27 @@ config MACH_DAVINCI_DA830_EVM > help > Say Y here to select the TI DA830/OMAP-L137 Evaluation Module. > > +config DA830_UI > + bool "DA830/OMAP-L137 UI (User Interface) board support" > + depends on MACH_DAVINCI_DA830_EVM > + help > + Say Y here if you have the DA830/OMAP-L137 UI > + (User Interface) board installed and you want to > + enable the peripherals located on User Interface > + board. > + > +choice > + prompt "Select DA830/OMAP-L137 UI board peripheral" > + depends on DA830_UI > + > +config DA830_UI_LCD > + bool "LCD" > + help > + Say Y here to use the LCD as a framebuffer or simple character > + display. > + > +endchoice Certainly my addition thu the help text was somewhat reworded... > + > config MACH_DAVINCI_DA850_EVM > bool "TI DA850/OMAP-L138 Reference Platform" > default ARCH_DAVINCI_DA850 > diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c > index 69a791a..dfc4897 100644 > --- a/arch/arm/mach-davinci/board-da830-evm.c > +++ b/arch/arm/mach-davinci/board-da830-evm.c > @@ -13,7 +13,9 @@ > #include <linux/module.h> > #include <linux/init.h> > #include <linux/console.h> > +#include <linux/gpio.h> > #include <linux/i2c.h> > +#include <linux/i2c/pcf857x.h> > #include <linux/i2c/at24.h> > > #include <asm/mach-types.h> > @@ -38,6 +40,31 @@ static struct at24_platform_data da830_evm_i2c_eeprom_info = { > .context = (void *)0x7f00, > }; > > +static int da830_evm_ui_expander_setup(struct i2c_client *client, int gpio, > + unsigned ngpio, void *context) > +{ > + gpio_request(gpio + 6, "MUX_MODE"); > +#ifdef CONFIG_DA830_UI_LCD > + gpio_direction_output(gpio + 6, 0); > +#else /* Must be NAND or NOR */ > + gpio_direction_output(gpio + 6, 1); One is the default value after reset, no need to reprogram it. > +#endif > + return 0; > +} > + [...] > @@ -175,6 +206,17 @@ static __init void da830_evm_init(void) > if (ret) > pr_warning("da830_evm_init: mmc/sd registration failed: %d\n", > ret); > + > +#ifdef CONFIG_DA830_UI_LCD > + ret = da8xx_pinmux_setup(da830_lcdcntl_pins); > + if (ret) > + pr_warning("da830_evm_init: lcdcntl mux setup failed: %d\n", > + ret); > + > + ret = da8xx_register_lcdc(&sharp_lcd035q3dg01_pdata); It's again not clear why board specific LCD platfrom data ended up in devices-da8xx.c > + if (ret) > + pr_warning("da830_evm_init: lcd setup failed: %d\n", ret); > +#endif > } > > #ifdef CONFIG_SERIAL_8250_CONSOLE I see this has been committed too -- Kevin, really, could you allow more time on patch comments? WBR, Sergei
On Fri, Sep 18, 2009 at 09:05:56PM +0400, Sergei Shtylyov wrote: > Hello. > > Mark A. Greer wrote: > >> From: Steve Chen <schen@mvista.com> > > Hm, is it Steve's patch? I'm seeing my stuff here. When I first started putting this stuff together I asked you to review/sign-off on the patches that you had a hand in. As I recall, you gave a terse, gruff answer that included a statement that you didn't want your name on any of so you got your wish. Mark --
On Fri, Sep 18, 2009 at 11:31:53AM -0700, Mark A. Greer wrote: > On Fri, Sep 18, 2009 at 09:05:56PM +0400, Sergei Shtylyov wrote: > > Hello. > > > > Mark A. Greer wrote: > > > >> From: Steve Chen <schen@mvista.com> > > > > Hm, is it Steve's patch? I'm seeing my stuff here. > > When I first started putting this stuff together I asked you > to review/sign-off on the patches that you had a hand in. > As I recall, you gave a terse, gruff answer that included > a statement that you didn't want your name on any of so you > got your wish. I'm sorry for this outburst, Sergei, it was uncalled for. If I missed getting your sign off, it was not intentional. My apologies. Mark --
Sergei Shtylyov <sshtylyov@ru.mvista.com> writes: > I see this has been committed too -- Kevin, really, could you allow > more time on patch comments? OK, I looked at the timeline: 09/04: Mark's inital series 09/14: (10 days later) my initial response, respin request, no other problems from me no other comments seen from anyone else 09/15: v2 respin and commit to davinci git You had 11 days from initial posting to respond. How much time are you suggesting I wait? Also, if you have problems with the version committed to DaVinci git, post a patch for discussion with your arguments. These kinds of changes I tend to fold together before going to mainline so the mainline patch would be a single commit. Kevin
Hello. Kevin Hilman wrote: >> I see this has been committed too -- Kevin, really, could you allow >> more time on patch comments? >> > > OK, I looked at the timeline: > > 09/04: Mark's inital series > > 09/14: (10 days later) my initial response, > respin request, no other problems from me > no other comments seen from anyone else > > 09/15: v2 respin and commit to davinci git Hum, I didn't realize this patch was respinned -- its subject doesn't have a trace of that, so probably nobody had issues with it the first time... > You had 11 days from initial posting to respond. How much time are > you suggesting I wait? > Now I'm seeing that it's all entirely my fault of failing to notice the earlier series. Nevermind then -- I'm sorry for the noise. > Also, if you have problems with the version committed to DaVinci git, > post a patch for discussion with your arguments. These kinds of > changes I tend to fold together before going to mainline so the > mainline patch would be a single commit. > OK, I'll see what can be done... > Kevin WBR, Sergei
On Fri, Sep 18, 2009 at 09:05:56PM +0400, Sergei Shtylyov wrote: >> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig >> +config DA830_UI_LCD >> + bool "LCD" >> + help >> + Say Y here to use the LCD as a framebuffer or simple character >> + display. >> + >> +endchoice > > Certainly my addition thu the help text was somewhat reworded... Yes I reworded it. What is your issue? >> diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c >> +static int da830_evm_ui_expander_setup(struct i2c_client *client, int >> gpio, >> + unsigned ngpio, void *context) >> +{ >> + gpio_request(gpio + 6, "MUX_MODE"); >> +#ifdef CONFIG_DA830_UI_LCD >> + gpio_direction_output(gpio + 6, 0); >> +#else /* Must be NAND or NOR */ >> + gpio_direction_output(gpio + 6, 1); > > One is the default value after reset, no need to reprogram it. That's true but there's no harm in programming the proper value whether or not its a detault. In addition, you cannot always assume the hardware was reset (e.g., kexec). >> @@ -175,6 +206,17 @@ static __init void da830_evm_init(void) >> if (ret) >> pr_warning("da830_evm_init: mmc/sd registration failed: %d\n", >> ret); >> + >> +#ifdef CONFIG_DA830_UI_LCD >> + ret = da8xx_pinmux_setup(da830_lcdcntl_pins); >> + if (ret) >> + pr_warning("da830_evm_init: lcdcntl mux setup failed: %d\n", >> + ret); >> + >> + ret = da8xx_register_lcdc(&sharp_lcd035q3dg01_pdata); > > It's again not clear why board specific LCD platfrom data ended up in > devices-da8xx.c Firstly, that's where the definition for sharp_lk043t1dg01_pdata already was so I simply added to what already existed. Secondly, the lcd definitions are [more-or-less] board agnostic, not board specific. So, devices-da8xx.c seems like a good place; although, if more come along, they could be put in their own file. Putting them in the board files only eliminates the possibility of reusing the definitions. Mark --
Hello. Mark A. Greer wrote: > On Fri, Sep 18, 2009 at 09:05:56PM +0400, Sergei Shtylyov wrote: > > >>> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig >>> > > >>> +config DA830_UI_LCD >>> + bool "LCD" >>> + help >>> + Say Y here to use the LCD as a framebuffer or simple character >>> + display. >>> + >>> +endchoice >>> >> Certainly my addition thu the help text was somewhat reworded... >> > > Yes I reworded it. What is your issue? > No issues, just noticed the rewording... >>> diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c >>> > > >>> +static int da830_evm_ui_expander_setup(struct i2c_client *client, int >>> gpio, >>> + unsigned ngpio, void *context) >>> +{ >>> + gpio_request(gpio + 6, "MUX_MODE"); >>> +#ifdef CONFIG_DA830_UI_LCD >>> + gpio_direction_output(gpio + 6, 0); >>> +#else /* Must be NAND or NOR */ >>> + gpio_direction_output(gpio + 6, 1); >>> >> One is the default value after reset, no need to reprogram it. >> > > That's true but there's no harm in programming the proper value whether > or not its a detault. In addition, you cannot always assume the hardware > was reset (e.g., kexec). > OK. Our internal code additionally had the expander set to all ones which this code doesn't do, so setting the value explicitly could indeed be useful... >>> @@ -175,6 +206,17 @@ static __init void da830_evm_init(void) >>> if (ret) >>> pr_warning("da830_evm_init: mmc/sd registration failed: %d\n", >>> ret); >>> + >>> +#ifdef CONFIG_DA830_UI_LCD >>> + ret = da8xx_pinmux_setup(da830_lcdcntl_pins); >>> + if (ret) >>> + pr_warning("da830_evm_init: lcdcntl mux setup failed: %d\n", >>> + ret); >>> + >>> + ret = da8xx_register_lcdc(&sharp_lcd035q3dg01_pdata); >>> >> It's again not clear why board specific LCD platfrom data ended up in >> devices-da8xx.c >> > > Firstly, that's where the definition for sharp_lk043t1dg01_pdata > already was so I simply added to what already existed. Secondly, > the lcd definitions are [more-or-less] board agnostic, not board specific. > That is what I'm finding most strange. How the LCD controller choice (with it chosen specifically for implementing the board) can be board agnostic? AFAIU, this is as board specific as e.g. a NAND chip, yet you put thr NAND platfrom data into the board file despite the possibility of the same NAND chip being used on several different boards... > So, devices-da8xx.c seems like a good place; although, if more come along, > they could be put in their own file. Putting them in the board files > only eliminates the possibility of reusing the definitions. > The data definition seems so small that it doesn't seem worth reusing really... > Mark > -- > WBR, Sergei
On Wed, Sep 23, 2009 at 12:14:24AM +0400, Sergei Shtylyov wrote: > Hello. > > Mark A. Greer wrote: >> On Fri, Sep 18, 2009 at 09:05:56PM +0400, Sergei Shtylyov wrote: >> >> >>>> diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig >>>> >> >> >>>> +config DA830_UI_LCD >>>> + bool "LCD" >>>> + help >>>> + Say Y here to use the LCD as a framebuffer or simple character >>>> + display. >>>> + >>>> +endchoice >>>> >>> Certainly my addition thu the help text was somewhat reworded... >>> >> >> Yes I reworded it. What is your issue? >> > > No issues, just noticed the rewording... OK >>>> @@ -175,6 +206,17 @@ static __init void da830_evm_init(void) >>>> if (ret) >>>> pr_warning("da830_evm_init: mmc/sd registration failed: %d\n", >>>> ret); >>>> + >>>> +#ifdef CONFIG_DA830_UI_LCD >>>> + ret = da8xx_pinmux_setup(da830_lcdcntl_pins); >>>> + if (ret) >>>> + pr_warning("da830_evm_init: lcdcntl mux setup failed: %d\n", >>>> + ret); >>>> + >>>> + ret = da8xx_register_lcdc(&sharp_lcd035q3dg01_pdata); >>>> >>> It's again not clear why board specific LCD platfrom data ended up >>> in devices-da8xx.c >>> >> >> Firstly, that's where the definition for sharp_lk043t1dg01_pdata >> already was so I simply added to what already existed. Secondly, >> the lcd definitions are [more-or-less] board agnostic, not board specific. >> > > That is what I'm finding most strange. How the LCD controller choice > (with it chosen specifically for implementing the board) can be board > agnostic? > AFAIU, this is as board specific as e.g. a NAND chip, yet you put thr > NAND platfrom data into the board file despite the possibility of the > same NAND chip being used on several different boards... You make a good point but you missed my first point :) IOW, the initial patch put it in devices-da8xx.c. Feel free to change it, I have no strong preference. Mark --
diff --git a/arch/arm/mach-davinci/Kconfig b/arch/arm/mach-davinci/Kconfig index 40866c6..7b6dddf 100644 --- a/arch/arm/mach-davinci/Kconfig +++ b/arch/arm/mach-davinci/Kconfig @@ -101,6 +101,27 @@ config MACH_DAVINCI_DA830_EVM help Say Y here to select the TI DA830/OMAP-L137 Evaluation Module. +config DA830_UI + bool "DA830/OMAP-L137 UI (User Interface) board support" + depends on MACH_DAVINCI_DA830_EVM + help + Say Y here if you have the DA830/OMAP-L137 UI + (User Interface) board installed and you want to + enable the peripherals located on User Interface + board. + +choice + prompt "Select DA830/OMAP-L137 UI board peripheral" + depends on DA830_UI + +config DA830_UI_LCD + bool "LCD" + help + Say Y here to use the LCD as a framebuffer or simple character + display. + +endchoice + config MACH_DAVINCI_DA850_EVM bool "TI DA850/OMAP-L138 Reference Platform" default ARCH_DAVINCI_DA850 diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c index 69a791a..dfc4897 100644 --- a/arch/arm/mach-davinci/board-da830-evm.c +++ b/arch/arm/mach-davinci/board-da830-evm.c @@ -13,7 +13,9 @@ #include <linux/module.h> #include <linux/init.h> #include <linux/console.h> +#include <linux/gpio.h> #include <linux/i2c.h> +#include <linux/i2c/pcf857x.h> #include <linux/i2c/at24.h> #include <asm/mach-types.h> @@ -38,6 +40,31 @@ static struct at24_platform_data da830_evm_i2c_eeprom_info = { .context = (void *)0x7f00, }; +static int da830_evm_ui_expander_setup(struct i2c_client *client, int gpio, + unsigned ngpio, void *context) +{ + gpio_request(gpio + 6, "MUX_MODE"); +#ifdef CONFIG_DA830_UI_LCD + gpio_direction_output(gpio + 6, 0); +#else /* Must be NAND or NOR */ + gpio_direction_output(gpio + 6, 1); +#endif + return 0; +} + +static int da830_evm_ui_expander_teardown(struct i2c_client *client, int gpio, + unsigned ngpio, void *context) +{ + gpio_free(gpio + 6); + return 0; +} + +static struct pcf857x_platform_data da830_evm_ui_expander_info = { + .gpio_base = DAVINCI_N_GPIO, + .setup = da830_evm_ui_expander_setup, + .teardown = da830_evm_ui_expander_teardown, +}; + static struct i2c_board_info __initdata da830_evm_i2c_devices[] = { { I2C_BOARD_INFO("24c256", 0x50), @@ -45,7 +72,11 @@ static struct i2c_board_info __initdata da830_evm_i2c_devices[] = { }, { I2C_BOARD_INFO("tlv320aic3x", 0x18), - } + }, + { + I2C_BOARD_INFO("pcf8574", 0x3f), + .platform_data = &da830_evm_ui_expander_info, + }, }; static struct davinci_i2c_platform_data da830_evm_i2c_0_pdata = { @@ -175,6 +206,17 @@ static __init void da830_evm_init(void) if (ret) pr_warning("da830_evm_init: mmc/sd registration failed: %d\n", ret); + +#ifdef CONFIG_DA830_UI_LCD + ret = da8xx_pinmux_setup(da830_lcdcntl_pins); + if (ret) + pr_warning("da830_evm_init: lcdcntl mux setup failed: %d\n", + ret); + + ret = da8xx_register_lcdc(&sharp_lcd035q3dg01_pdata); + if (ret) + pr_warning("da830_evm_init: lcd setup failed: %d\n", ret); +#endif } #ifdef CONFIG_SERIAL_8250_CONSOLE diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c index f52174a..f0b2f96 100644 --- a/arch/arm/mach-davinci/da830.c +++ b/arch/arm/mach-davinci/da830.c @@ -411,7 +411,7 @@ static struct davinci_clk da830_clks[] = { CLK(NULL, "pwm2", &pwm2_clk), CLK("eqep.0", NULL, &eqep0_clk), CLK("eqep.1", NULL, &eqep1_clk), - CLK("da830_lcdc", NULL, &lcdc_clk), + CLK("da8xx_lcdc.0", NULL, &lcdc_clk), CLK("davinci-mcasp.0", NULL, &mcasp0_clk), CLK("davinci-mcasp.1", NULL, &mcasp1_clk), CLK("davinci-mcasp.2", NULL, &mcasp2_clk),