Re: [PATCH 1/3] OMAP: DSS2: Add generic DPI panel display driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux