Hi Javier,
Thanks for the review.
On Mon, Jun 23, 2014 at 11:30 AM, Javier Martinez Canillas
<javier@xxxxxxxxxxxx> wrote:
> Hello Ajay,
>
> Not an extensive review since I'm not familiar with the graphics stack
> but a few things I noticed are commented below.
>
> On Wed, Jun 11, 2014 at 8:27 PM, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> wrote:
>> This patch adds a simple driver to handle all the LCD and LED
>> powerup/down routines needed to support eDP/LVDS panels.
>>
>> The LCD and LED units are usually powered up via regulators,
>> and almost on all boards, we will have a BACKLIGHT_EN pin to
>> enable/ disable the backlight.
>> Sometimes, we can have LCD_EN switches as well.
>>
>> The routines in this driver can be used to control
>> panel power sequence on such boards.
>>
>> Signed-off-by: Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>
>> Signed-off-by: Rahul Sharma <Rahul.Sharma@xxxxxxxxxxx>
>> ---
>> .../devicetree/bindings/panel/panel-lvds.txt | 50 ++++
>> drivers/gpu/drm/panel/Kconfig | 10 +
>> drivers/gpu/drm/panel/Makefile | 1 +
>> drivers/gpu/drm/panel/panel-lvds.c | 262 ++++++++++++++++++++
>> 4 files changed, 323 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/panel/panel-lvds.txt
>> create mode 100644 drivers/gpu/drm/panel/panel-lvds.c
>>
>> diff --git a/Documentation/devicetree/bindings/panel/panel-lvds.txt b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> new file mode 100644
>> index 0000000..7cb6084
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/panel/panel-lvds.txt
>> @@ -0,0 +1,50 @@
>> +panel interface for eDP/lvds panels
>> +
>> +Required properties:
>> + - compatible: "panel-lvds"
>> +
>> +Optional properties:
>> + -lcd-en-gpio:
>> + panel LCD poweron GPIO.
>> + Indicates which GPIO needs to be powered up as output
>> + to powerup/enable the switch to the LCD panel.
>> + -led-en-gpio:
>> + panel LED enable GPIO.
>> + Indicates which GPIO needs to be powered up as output
>> + to enable the backlight.
>> + -panel-prepare-delay:
>> + delay value in ms required for panel_prepare process
>> + Delay in ms needed for the panel LCD unit to
>> + powerup completely.
>> + ex: delay needed till eDP panel throws HPD.
>> + delay needed so that we cans tart reading edid.
>> + -panel-enable-delay:
>> + delay value in ms required for panel_enable process
>> + Delay in ms needed for the panel backlight/LED unit
>> + to powerup, and delay needed between video_enable and
>> + backlight_enable.
>> + -panel-disable-delay:
>> + delay value in ms required for panel_disable process
>> + Delay in ms needed for the panel backlight/LED unit
>> + powerdown, and delay needed between backlight_disable
>> + and video_disable.
>> + -panel-unprepare-delay:
>> + delay value in ms required for panel_post_disable process
>> + Delay in ms needed for the panel LCD unit to
>> + to powerdown completely, and the minimum delay needed
>> + before powering it on again.
>> + -panel-width-mm: physical panel width [mm]
>> + -panel-height-mm: physical panel height [mm]
>> +
>> +Example:
>> +
>> + panel-lvds {
>> + compatible = "panel-lvds";
>> + led-en-gpio = <&gpx3 0 1>;
>> + panel-prepare-delay = <40>;
>> + panel-enable-delay = <20>;
>> + panel-disable-delay = <20>;
>> + panel-unprepare-delay = <30>;
>> + panel-width-mm = <256>;
>> + panel-height-mm = <144>;
>> + };
>
> Recently it's considered a good practice to have the DT binding
> documentation in a separate patch.
Ok, will remember this.
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 4ec874d..8fe7ee5 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -30,4 +30,14 @@ config DRM_PANEL_S6E8AA0
>> select DRM_MIPI_DSI
>> select VIDEOMODE_HELPERS
>>
>> +config DRM_PANEL_EDP_LVDS
>> + tristate "support for eDP/LVDS panels"
>> + depends on OF && DRM_PANEL
>> + select VIDEOMODE_HELPERS
>> + help
>> + DRM panel driver for direct eDP panels or LVDS connected
>> + via DP bridges, that need at most a regulator for LCD unit,
>> + a regulator for LED unit and/or enable GPIOs for LCD or LED units.
>> + Delay values can also be specified to support powerup and
>> + powerdown process.
>> endmenu
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index 8b92921..eaafa01 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o
>> obj-$(CONFIG_DRM_PANEL_LD9040) += panel-ld9040.o
>> obj-$(CONFIG_DRM_PANEL_S6E8AA0) += panel-s6e8aa0.o
>> +obj-$(CONFIG_DRM_PANEL_EDP_LVDS) += panel-lvds.o
>> diff --git a/drivers/gpu/drm/panel/panel-lvds.c b/drivers/gpu/drm/panel/panel-lvds.c
>> new file mode 100644
>> index 0000000..2124fcb
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-lvds.c
>> @@ -0,0 +1,262 @@
>> +/*
>> + * panel driver for lvds and eDP panels
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd
>> + *
>> + * Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>
>> + *
>> + * 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.
>> + */
>> +
>> +#include <linux/gpio.h>
>> +#include <linux/module.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <video/of_videomode.h>
>> +#include <video/videomode.h>
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_crtc.h>
>> +#include <drm/drm_panel.h>
>> +
>> +struct panel_lvds {
>> + struct drm_panel base;
>> + struct regulator *backlight_fet;
>> + struct regulator *lcd_fet;
>> + struct videomode vm;
>> + int width_mm;
>> + int height_mm;
>> + bool backlight_fet_enabled;
>> + bool lcd_fet_enabled;
>> + int led_en_gpio;
>> + int lcd_en_gpio;
>> + int panel_prepare_delay;
>> + int panel_enable_delay;
>> + int panel_disable_delay;
>> + int panel_unprepare_delay;
>> +};
>> +
>> +static inline struct panel_lvds *to_panel(struct drm_panel *panel)
>> +{
>> + return container_of(panel, struct panel_lvds, base);
>> +}
>> +
>> +static int panel_lvds_prepare(struct drm_panel *panel)
>> +{
>> + struct panel_lvds *lvds_panel = to_panel(panel);
>> +
>> + if (!IS_ERR_OR_NULL(lvds_panel->lcd_fet))
>> + if (!lvds_panel->lcd_fet_enabled) {
>> + if (regulator_enable(lvds_panel->lcd_fet))
>> + DRM_ERROR("failed to enable LCD fet\n");
>> + lvds_panel->lcd_fet_enabled = true;
>
> If this is an expected condition then using DRM_INFO() instead? It
> looks like an error to me though so maybe should return an error in
> this case?
Right, we should ideally return an error here.
>> + }
>> +
>> + if (gpio_is_valid(lvds_panel->lcd_en_gpio))
>> + gpio_set_value(lvds_panel->lcd_en_gpio, 1);
>> +
>> + msleep(lvds_panel->panel_prepare_delay);
>> +
>> + return 0;
>> +}
>> +
>> +static int panel_lvds_enable(struct drm_panel *panel)
>> +{
>> + struct panel_lvds *lvds_panel = to_panel(panel);
>> +
>> + if (!IS_ERR_OR_NULL(lvds_panel->backlight_fet))
>> + if (!lvds_panel->backlight_fet_enabled) {
>> + if (regulator_enable(lvds_panel->backlight_fet))
>> + DRM_ERROR("failed to enable LED fet\n");
>> + lvds_panel->backlight_fet_enabled = true;
>
> Same here.
Ok.
>> + }
>> +
>> + msleep(lvds_panel->panel_enable_delay);
>> +
>> + if (gpio_is_valid(lvds_panel->led_en_gpio))
>> + gpio_set_value(lvds_panel->led_en_gpio, 1);
>> +
>> + return 0;
>> +}
>> +
>> +static int panel_lvds_disable(struct drm_panel *panel)
>> +{
>> + struct panel_lvds *lvds_panel = to_panel(panel);
>> +
>> + if (gpio_is_valid(lvds_panel->led_en_gpio))
>> + gpio_set_value(lvds_panel->led_en_gpio, 0);
>> +
>> + if (!IS_ERR_OR_NULL(lvds_panel->backlight_fet))
>> + if (lvds_panel->backlight_fet_enabled) {
>> + regulator_disable(lvds_panel->backlight_fet);
>> + lvds_panel->backlight_fet_enabled = false;
>> + }
>> +
>> + msleep(lvds_panel->panel_disable_delay);
>> +
>> + return 0;
>> +}
>> +
>> +static int panel_lvds_unprepare(struct drm_panel *panel)
>> +{
>> + struct panel_lvds *lvds_panel = to_panel(panel);
>> +
>> + if (gpio_is_valid(lvds_panel->lcd_en_gpio))
>> + gpio_set_value(lvds_panel->lcd_en_gpio, 0);
>> +
>> + if (!IS_ERR_OR_NULL(lvds_panel->lcd_fet))
>> + if (lvds_panel->lcd_fet_enabled) {
>> + regulator_disable(lvds_panel->lcd_fet);
>> + lvds_panel->lcd_fet_enabled = false;
>> + }
>> +
>> + msleep(lvds_panel->panel_unprepare_delay);
>> +
>> + return 0;
>> +}
>> +
>> +static int panel_lvds_get_modes(struct drm_panel *panel)
>> +{
>> + struct drm_connector *connector = panel->connector;
>> + struct panel_lvds *lvds_panel = to_panel(panel);
>> + struct drm_display_mode *mode;
>> +
>> + mode = drm_mode_create(connector->dev);
>> + if (!mode) {
>> + DRM_ERROR("failed to create a new display mode.\n");
>> + return 0;
>> + }
>> +
>> + drm_display_mode_from_videomode(&lvds_panel->vm, mode);
>> + mode->width_mm = lvds_panel->width_mm;
>> + mode->height_mm = lvds_panel->height_mm;
>> + connector->display_info.width_mm = mode->width_mm;
>> + connector->display_info.height_mm = mode->height_mm;
>> +
>> + mode->type = DRM_MODE_TYPE_DRIVER | DRM_MODE_TYPE_PREFERRED;
>> + drm_mode_set_name(mode);
>> + drm_mode_probed_add(connector, mode);
>> +
>> + return 1;
>> +}
>> +
>> +static const struct drm_panel_funcs panel_lvds_funcs = {
>> + .unprepare = panel_lvds_unprepare,
>> + .disable = panel_lvds_disable,
>> + .prepare = panel_lvds_prepare,
>> + .enable = panel_lvds_enable,
>> + .get_modes = panel_lvds_get_modes,
>> +};
>> +
>> +static int panel_lvds_probe(struct platform_device *pdev)
>> +{
>> + struct panel_lvds *panel;
>> + struct device *dev = &pdev->dev;
>> + struct device_node *node = dev->of_node;
>> + int ret;
>> +
>> + panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL);
>> + if (!panel)
>> + return -ENOMEM;
>> +
>> + ret = of_get_videomode(node, &panel->vm, 0);
>> + if (ret) {
>> + DRM_ERROR("failed: of_get_videomode() : %d\n", ret);
>> + return ret;
>> + }
>> +
>> + panel->lcd_fet = devm_regulator_get_optional(dev, "lcd_vdd");
>> + if (IS_ERR(panel->lcd_fet))
>> + return -EPROBE_DEFER;
>> +
>> + panel->backlight_fet = devm_regulator_get_optional(dev, "vcd_led");
>> + if (IS_ERR(panel->backlight_fet))
>> + return -EPROBE_DEFER;
>> +
>> + panel->lcd_en_gpio = of_get_named_gpio(node, "lcd-en-gpio", 0);
>> + panel->led_en_gpio = of_get_named_gpio(node, "led-en-gpio", 0);
>> +
>
> The old integer-based GPIO API has been deprecated and new drivers
> should use the new descriptor-based interface
> (Documentation/gpio/consumer.txt) instead if possible.
Ok, I just saw other panel drivers using the same.
Will change this.
>> + of_property_read_u32(node, "panel-width-mm", &panel->width_mm);
>> + of_property_read_u32(node, "panel-height-mm", &panel->height_mm);
>> +
>> + of_property_read_u32(node, "panel-prepare-delay",
>> + &panel->panel_prepare_delay);
>> + of_property_read_u32(node, "panel-enable-delay",
>> + &panel->panel_enable_delay);
>> + of_property_read_u32(node, "panel-disable-delay",
>> + &panel->panel_disable_delay);
>> + of_property_read_u32(node, "panel-unprepare-delay",
>> + &panel->panel_unprepare_delay);
>> +
>> + if (gpio_is_valid(panel->lcd_en_gpio)) {
>> + ret = devm_gpio_request_one(dev, panel->lcd_en_gpio,
>> + GPIOF_OUT_INIT_LOW, "lcd_en_gpio");
>> + if (ret) {
>> + DRM_ERROR("failed to get lcd-en gpio [%d]\n", ret);
>> + return 0;
>
> The probe() function returning 0 means success to the driver core but
> here is returning early due an error.
Right, will fix it.
>> + }
>> + } else {
>> + panel->lcd_en_gpio = -ENODEV;
>> + }
>> +
>> + if (gpio_is_valid(panel->led_en_gpio)) {
>> + ret = devm_gpio_request_one(dev, panel->led_en_gpio,
>> + GPIOF_OUT_INIT_LOW, "led_en_gpio");
>> + if (ret) {
>> + DRM_ERROR("failed to get led-en gpio [%d]\n", ret);
>> + return 0;
>
> Same here, it should return an error code instead 0.
Right, will fix it.
>> + }
>> + } else {
>> + panel->led_en_gpio = -ENODEV;
>> + }
>> +
>> + drm_panel_init(&panel->base);
>> + panel->base.dev = dev;
>> + panel->base.funcs = &panel_lvds_funcs;
>> +
>> + ret = drm_panel_add(&panel->base);
>> + if (ret < 0)
>> + return ret;
>> +
>> + dev_set_drvdata(dev, panel);
>> +
>> + return 0;
>> +}
>> +
>> +static int panel_lvds_remove(struct platform_device *pdev)
>> +{
>> + struct panel_lvds *panel = dev_get_drvdata(&pdev->dev);
>> +
>> + panel_lvds_disable(&panel->base);
>> + panel_lvds_unprepare(&panel->base);
>> +
>> + drm_panel_remove(&panel->base);
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id lvds_panel_dt_match[] = {
>> + { .compatible = "panel-lvds" },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, lvds_panel_dt_match);
>> +
>> +struct platform_driver lvds_panel_driver = {
>> + .driver = {
>> + .name = "panel_lvds",
>> + .owner = THIS_MODULE,
>> + .of_match_table = lvds_panel_dt_match,
>> + },
>> + .probe = panel_lvds_probe,
>> + .remove = panel_lvds_remove,
>> +};
>> +module_platform_driver(lvds_panel_driver);
>> +
>> +MODULE_AUTHOR("Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("lvds/eDP panel driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 1.7.9.5
>>
>
> Best regards,
> Javier
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
