Re: [PATCH v4 2/2] drm/panel: Add Feiyang FY07024DI26A30-D MIPI-DSI LCD panel

Hi Jagan.

Thanks for this nice patch, a few comments below.

On Fri, Jan 25, 2019 at 12:13:13AM +0530, Jagan Teki wrote:

Feiyang FY07024DI26A30-D is 1024x600, 4-lane MIPI-DSI LCD panel.

Add panel driver for it.

Signed-off-by: Jagan Teki jagan@amarulasolutions.com

Changes for v5:

• rebase on master
• adjust the hporch values to satisfy the refresh

Changes for v4:

• use simple structure for command init
• update proper comments on power, reset delay sequnce
• fix to use set_display_off in disable function
• move mode type to structure
• drop refres rate value, let drm compute

Changes for v2:

• new patch, derived from another dsi series

MAINTAINERS | 6 + drivers/gpu/drm/panel/Kconfig | 9 + drivers/gpu/drm/panel/Makefile | 1 + .../drm/panel/panel-feiyang-fy07024di26a30d.c | 286 ++++++++++++++++++ 4 files changed, 302 insertions(+) create mode 100644 drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c

diff --git a/MAINTAINERS b/MAINTAINERS index e3f785bad68b..5c8591eb5840 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4831,6 +4831,12 @@ T: git git://anongit.freedesktop.org/drm/drm-misc S: Maintained F: drivers/gpu/drm/tve200/

+DRM DRIVER FOR FEIYANG FY07024DI26A30-D MIPI-DSI LCD PANELS +M: Jagan Teki jagan@amarulasolutions.com +S: Maintained +F: drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c +F: Documentation/devicetree/bindings/display/panel/feiyang,fy07024di26a30d.txt

DRM DRIVER FOR ILITEK ILI9225 PANELS M: David Lechner david@lechnology.com S: Maintained diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig index 151ddf720b83..5304db7b7b55 100644 --- a/drivers/gpu/drm/panel/Kconfig +++ b/drivers/gpu/drm/panel/Kconfig @@ -47,6 +47,15 @@ config DRM_PANEL_SIMPLE that it can be automatically turned off when the panel goes into a low power state.

+config DRM_PANEL_FEIYANG_FY07024DI26A30D

• tristate "Feiyang FY07024DI26A30-D MIPI-DSI LCD panel"
• depends on OF
• depends on DRM_MIPI_DSI
• depends on BACKLIGHT_CLASS_DEVICE
• help

•  Say Y if you want to enable support for panels based on the
  

•  Feiyang FY07024DI26A30-D MIPI-DSI interface.
  

config DRM_PANEL_ILITEK_IL9322 tristate "Ilitek ILI9322 320x240 QVGA panels" depends on OF && SPI diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile index 54db0921f329..c88dacf9d439 100644 --- a/drivers/gpu/drm/panel/Makefile +++ b/drivers/gpu/drm/panel/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_DRM_PANEL_ARM_VERSATILE) += panel-arm-versatile.o obj-$(CONFIG_DRM_PANEL_BANANAPI_S070WV20_ICN6211) += panel-bananapi-s070wv20-icn6211.o obj-$(CONFIG_DRM_PANEL_LVDS) += panel-lvds.o obj-$(CONFIG_DRM_PANEL_SIMPLE) += panel-simple.o +obj-$(CONFIG_DRM_PANEL_FEIYANG_FY07024DI26A30D) += panel-feiyang-fy07024di26a30d.o obj-$(CONFIG_DRM_PANEL_ILITEK_IL9322) += panel-ilitek-ili9322.o obj-$(CONFIG_DRM_PANEL_ILITEK_ILI9881C) += panel-ilitek-ili9881c.o obj-$(CONFIG_DRM_PANEL_INNOLUX_P079ZCA) += panel-innolux-p079zca.o diff --git a/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c new file mode 100644 index 000000000000..f655647aeade --- /dev/null +++ b/drivers/gpu/drm/panel/panel-feiyang-fy07024di26a30d.c @@ -0,0 +1,286 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2018 Amarula Solutions
• • Author: Jagan Teki jagan@amarulasolutions.com
• */

+#include <linux/backlight.h> +#include <linux/delay.h> +#include <linux/module.h> +#include <linux/of_device.h>

+#include <linux/gpio/consumer.h> +#include <linux/regulator/consumer.h>

+#include <drm/drmP.h>

Please do not use drmP.h in new drivers. We are slowly gettting rid of it and in drm-misc.git the drmP.h file no longer contains anything (other than includes/forwards).

• gpiod_set_value(ctx->reset, 1);
• msleep(50);
• gpiod_set_value(ctx->reset, 0);
• /* T5 + T6 (avdd rise + video & logic signal rise)

• * T5 >= 10ms, 0 < T6 <= 10ms
  

• */
  

• msleep(20);
• gpiod_set_value(ctx->reset, 1);

Is it really necessary to do a 1 => 0 => 1 cycle here for reset? panel-simple do not need it.

+static int feiyang_dsi_probe(struct mipi_dsi_device *dsi) +{

• struct feiyang *ctx;
• int ret;
• ctx = devm_kzalloc(&dsi->dev, sizeof(*ctx), GFP_KERNEL);
• if (!ctx)

• return -ENOMEM;
  

• mipi_dsi_set_drvdata(dsi, ctx);

Add empty line atfer return -ENOMEM;?

• ctx->dsi = dsi;
• drm_panel_init(&ctx->panel);
• ctx->panel.dev = &dsi->dev;
• ctx->panel.funcs = &feiyang_funcs;
• ctx->dvdd = devm_regulator_get(&dsi->dev, "dvdd");
• if (IS_ERR(ctx->dvdd)) {

• DRM_DEV_ERROR(&dsi->dev, "Couldn't get dvdd regulator\n");
  

• return PTR_ERR(ctx->dvdd);
  

• }
• ctx->avdd = devm_regulator_get(&dsi->dev, "avdd");
• if (IS_ERR(ctx->avdd)) {

• DRM_DEV_ERROR(&dsi->dev, "Couldn't get avdd regulator\n");
  

• return PTR_ERR(ctx->avdd);
  

• }
• ctx->reset = devm_gpiod_get(&dsi->dev, "reset", GPIOD_OUT_LOW);
• if (IS_ERR(ctx->reset)) {

• DRM_DEV_ERROR(&dsi->dev, "Couldn't get our reset GPIO\n");
  

• return PTR_ERR(ctx->reset);
  

• }
• ctx->backlight = devm_of_find_backlight(&dsi->dev);
• if (IS_ERR(ctx->backlight))

• return PTR_ERR(ctx->backlight);
  

• ret = drm_panel_add(&ctx->panel);
• if (ret < 0)

• goto put_backlight;
  

• dsi->mode_flags = MIPI_DSI_MODE_VIDEO_BURST;
• dsi->format = MIPI_DSI_FMT_RGB888;
• dsi->lanes = 4;
• ret = mipi_dsi_attach(dsi);
• if (ret < 0)

• goto panel_remove;
  

• return ret;

+panel_remove:

• drm_panel_remove(&ctx->panel);

+put_backlight:

• if (ctx->backlight)

• put_device(&ctx->backlight->dev);
  

As devm_of_find_backlight() is used the put_device() will be done for you. No need to do it here. I think the same is true for the panel.

• return ret;

+}

+static int feiyang_dsi_remove(struct mipi_dsi_device *dsi) +{

• struct feiyang *ctx = mipi_dsi_get_drvdata(dsi);
• mipi_dsi_detach(dsi);
• drm_panel_remove(&ctx->panel);
• if (ctx->backlight)

• put_device(&ctx->backlight->dev);
  

Likewise, use of devm_of_find_backlight() will do this for you.

Sam