On Wed, Nov 10, 2010 at 10:35 PM, Tomi Valkeinen <tomi.valkeinen@xxxxxxxxx> wrote: > Hi, > > On Tue, 2010-11-09 at 18:12 +0100, ext Bryan Wu wrote: >> Generic DPI panel driver includes the driver and 4 similar panel configurations. It >> will match the panel name which is passed from platform data and setup the >> right configurations. >> >> With generic DPI panel driver, we can remove those 4 duplicated panel display >> drivers. In the future, it is simple for us just add new panel configuration >> date in panel-generic-dpi.c to support new display panel. > > This is looking good, but still a couple of comments inline: > >> Signed-off-by: Bryan Wu <bryan.wu@xxxxxxxxxxxxx> >> --- >> .../arm/plat-omap/include/plat/panel-generic-dpi.h | 37 +++ >> drivers/video/omap2/displays/Kconfig | 8 + >> drivers/video/omap2/displays/Makefile | 1 + >> drivers/video/omap2/displays/panel-generic-dpi.c | 309 ++++++++++++++++++++ >> 4 files changed, 355 insertions(+), 0 deletions(-) >> create mode 100644 arch/arm/plat-omap/include/plat/panel-generic-dpi.h >> create mode 100644 drivers/video/omap2/displays/panel-generic-dpi.c >> >> diff --git a/arch/arm/plat-omap/include/plat/panel-generic-dpi.h b/arch/arm/plat-omap/include/plat/panel-generic-dpi.h >> new file mode 100644 >> index 0000000..7906197 >> --- /dev/null >> +++ b/arch/arm/plat-omap/include/plat/panel-generic-dpi.h >> @@ -0,0 +1,37 @@ >> +/* >> + * Header for generic DPI panel driver >> + * >> + * Copyright (C) 2010 Canonical Ltd. >> + * Author: Bryan Wu <bryan.wu@xxxxxxxxxxxxx> >> + * >> + * 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. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#ifndef __ARCH_ARM_PLAT_OMAP_PANEL_GENERIC_DPI_H >> +#define __ARCH_ARM_PLAT_OMAP_PANEL_GENERIC_DPI_H >> + >> +#include "display.h" >> + >> +/** >> + * struct panel_generic_dpi_data - panel driver configuration data >> + * @name: panel name >> + * @platform_enable: platform specific panel enable function >> + * @platform_disable: platform specific panel disable function >> + */ >> +struct panel_generic_dpi_data { >> + const char *name; >> + int (*platform_enable)(struct omap_dss_device *dssdev); >> + void (*platform_disable)(struct omap_dss_device *dssdev); >> +}; >> + >> +#endif /* __ARCH_ARM_PLAT_OMAP_PANEL_GENERIC_DPI_H */ >> diff --git a/drivers/video/omap2/displays/Kconfig b/drivers/video/omap2/displays/Kconfig >> index 12327bb..cb3e339 100644 >> --- a/drivers/video/omap2/displays/Kconfig >> +++ b/drivers/video/omap2/displays/Kconfig >> @@ -1,6 +1,14 @@ >> menu "OMAP2/3 Display Device Drivers" >> depends on OMAP2_DSS >> >> +config PANEL_GENERIC_DPI >> + tristate "Generic DPI Panel" >> + help >> + Generic DPI panel driver. >> + Supports DVI output for Beagle and OMAP3 SDP. >> + Supports LCD Panel used in TI SDP3430 and EVM boards, >> + OMAP3517 EVM boards and CM-T35. >> + >> config PANEL_GENERIC >> tristate "Generic Panel" >> help >> diff --git a/drivers/video/omap2/displays/Makefile b/drivers/video/omap2/displays/Makefile >> index aa38609..022058c 100644 >> --- a/drivers/video/omap2/displays/Makefile >> +++ b/drivers/video/omap2/displays/Makefile >> @@ -1,3 +1,4 @@ >> +obj-$(CONFIG_PANEL_GENERIC_DPI) += panel-generic-dpi.o >> obj-$(CONFIG_PANEL_GENERIC) += panel-generic.o >> obj-$(CONFIG_PANEL_SHARP_LS037V7DW01) += panel-sharp-ls037v7dw01.o >> obj-$(CONFIG_PANEL_SHARP_LQ043T1DG01) += panel-sharp-lq043t1dg01.o >> diff --git a/drivers/video/omap2/displays/panel-generic-dpi.c b/drivers/video/omap2/displays/panel-generic-dpi.c >> new file mode 100644 >> index 0000000..7ddd631 >> --- /dev/null >> +++ b/drivers/video/omap2/displays/panel-generic-dpi.c >> @@ -0,0 +1,309 @@ >> +/* >> + * Generic DPI Panels support >> + * >> + * Copyright (C) 2010 Canonical Ltd. >> + * Author: Bryan Wu <bryan.wu@xxxxxxxxxxxxx> >> + * >> + * Copyright (C) 2008 Nokia Corporation >> + * Author: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxx> >> + * >> + * 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. >> + * >> + * This program is distributed in the hope that it will be useful, but WITHOUT >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or >> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for >> + * more details. >> + * >> + * You should have received a copy of the GNU General Public License along with >> + * this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <linux/module.h> >> +#include <linux/delay.h> >> + >> +#include <plat/panel-generic-dpi.h> >> + >> +struct panel_config { >> + struct omap_video_timings timings; >> + >> + int acbi; /* ac-bias pin transitions per interrupt */ >> + /* Unit: line clocks */ >> + int acb; /* ac-bias pin frequency */ >> + >> + enum omap_panel_config config; >> + >> + /* >> + * Used to match device to panel configuration >> + * when use generic panel driver >> + */ >> + const char *name; >> +}; >> + >> +/* Panel configurations */ >> +static struct panel_config generic_dpi_panels[] = { >> + /* Generic Panel */ >> + { >> + { >> + .x_res = 640, >> + .y_res = 480, >> + >> + .pixel_clock = 23500, >> + >> + .hfp = 48, >> + .hsw = 32, >> + .hbp = 80, >> + >> + .vfp = 3, >> + .vsw = 4, >> + .vbp = 7, >> + }, >> + .acbi = 0x0, >> + .acb = 0x0, >> + .config = OMAP_DSS_LCD_TFT, >> + .name = "generic", >> + }, >> + >> + /* Sharp LQ043T1DG01 */ >> + { >> + { >> + .x_res = 480, >> + .y_res = 272, >> + >> + .pixel_clock = 9000, >> + >> + .hsw = 42, >> + .hfp = 3, >> + .hbp = 2, >> + >> + .vsw = 11, >> + .vfp = 3, >> + .vbp = 2, >> + }, >> + .acbi = 0x0, >> + .acb = 0x0, >> + .config = OMAP_DSS_LCD_TFT | OMAP_DSS_LCD_IVS | >> + OMAP_DSS_LCD_IHS | OMAP_DSS_LCD_IEO, >> + .name = "sharp_lq", >> + }, >> + >> + /* Sharp LS037V7DW01 */ >> + { >> + { >> + .x_res = 480, >> + .y_res = 640, >> + >> + .pixel_clock = 19200, >> + >> + .hsw = 2, >> + .hfp = 1, >> + .hbp = 28, >> + >> + .vsw = 1, >> + .vfp = 1, >> + .vbp = 1, >> + }, >> + .acbi = 0x0, >> + .acb = 0x28, >> + .config = OMAP_DSS_LCD_TFT | OMAP_DSS_LCD_IVS | >> + OMAP_DSS_LCD_IHS, >> + .name = "sharp_ls", >> + }, >> + >> + /* Toppoly TDO35S */ >> + { >> + { >> + .x_res = 480, >> + .y_res = 640, >> + >> + .pixel_clock = 26000, >> + >> + .hfp = 104, >> + .hsw = 8, >> + .hbp = 8, >> + >> + .vfp = 4, >> + .vsw = 2, >> + .vbp = 2, >> + }, >> + .acbi = 0x0, >> + .acb = 0x0, >> + .config = OMAP_DSS_LCD_TFT | OMAP_DSS_LCD_IVS | >> + OMAP_DSS_LCD_IHS | OMAP_DSS_LCD_IPC | >> + OMAP_DSS_LCD_ONOFF, >> + .name = "toppoly_tdo35s", >> + }, >> +}; >> + >> +static inline struct panel_generic_dpi_data >> +*get_panel_data(const struct omap_dss_device *dssdev) >> +{ >> + return (struct panel_generic_dpi_data *) dssdev->data; >> +} >> + >> + >> +static int generic_dpi_panel_power_on(struct omap_dss_device *dssdev) >> +{ >> + int r; >> + struct panel_generic_dpi_data *panel_data = get_panel_data(dssdev); >> + >> + if (dssdev->state == OMAP_DSS_DISPLAY_ACTIVE) >> + return 0; >> + >> + r = omapdss_dpi_display_enable(dssdev); >> + if (r) >> + goto err0; >> + >> + if (panel_data->platform_enable) { >> + r = panel_data->platform_enable(dssdev); >> + if (r) >> + goto err1; >> + } >> + >> + return 0; >> +err1: >> + omapdss_dpi_display_disable(dssdev); >> +err0: >> + return r; >> +} >> + >> +static void generic_dpi_panel_power_off(struct omap_dss_device *dssdev) >> +{ >> + struct panel_generic_dpi_data *panel_data = get_panel_data(dssdev); >> + >> + if (dssdev->state != OMAP_DSS_DISPLAY_ACTIVE) >> + return; >> + >> + if (panel_data->platform_disable) >> + panel_data->platform_disable(dssdev); >> + >> + omapdss_dpi_display_disable(dssdev); >> +} > > For both panel power on and off some panels require some sleep times: > when powering on, the video interface has to be enabled for some time > before the panel can be enabled, and similarly when powering off the > video interface needs to be on for some time after the panel has been > disabled. > Actually, in my previous version patches, I introduced .power_on_delay and .power_off_delay in panel_configs, which contains the sleep value for msleep(). But Archit told me this is not necessary, so I removed that. And I will call this in panel powering on function + /* wait couple of vsyncs until enabling the LCD */ + if (p->power_on_delay) + msleep(p->power_on_delay); + Is that OK for you? > See for example sharp_lq driver's power_on/off. > > This should be handled similarly that what we have in panel-taal: > configuration options in the panel_config struct for the sleep times > (and if it's 0, no sleep done). > >> + >> +static int generic_dpi_panel_probe(struct omap_dss_device *dssdev) >> +{ >> + struct panel_generic_dpi_data *panel_data = get_panel_data(dssdev); >> + struct panel_config *panel_config = NULL; >> + int i; >> + >> + dev_dbg(&dssdev->dev, "probe\n"); >> + >> + if (!panel_data || !panel_data->name) >> + return -EINVAL; >> + >> + for (i = 0; i < ARRAY_SIZE(generic_dpi_panels); i++) { >> + if (strcmp(panel_data->name, generic_dpi_panels[i].name) == 0) { >> + panel_config = &generic_dpi_panels[i]; >> + break; >> + } >> + } >> + >> + if (!panel_config) >> + return -EINVAL; >> + >> + dssdev->type = OMAP_DISPLAY_TYPE_DPI; > > Leave this display type also in to the board file. It defines the > interface (like the data_lines), not the panel. > No problem, I will change that back. > I think otherwise the patches look good. At some point we should add > backlight support also, so that we can remove a few drivers more (and > get rid of the extra backlight stuff in omap_dss_device struct). > Yeah, I plan to do that. But IMHO, how about just move those backlight support as backlight drivers and only remain display control in this directory. Thanks a lot, -- Bryan Wu <bryan.wu@xxxxxxxxxxxxx> Kernel Developer +86.138-1617-6545 Mobile Ubuntu Kernel Team Canonical Ltd. www.canonical.com Ubuntu - Linux for human beings | www.ubuntu.com -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
