[RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Lin Huang]

Refactor Innolux P079ZCA panel driver, let it support
multi panel.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- Change regulator property name to meet the panel datasheet 
Changes in v3:
- this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch 

 drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
 1 file changed, 105 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 6ba9344..1597744 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -20,12 +20,32 @@
 
 #include <video/mipi_display.h>
 
+struct panel_desc {
+	const struct drm_display_mode *modes;
+	unsigned int bpc;
+	struct {
+		unsigned int width;
+		unsigned int height;
+	} size;
+};
+
+struct panel_desc_dsi {
+	struct panel_desc desc;
+
+	unsigned long flags;
+	enum mipi_dsi_pixel_format format;
+	unsigned int lanes;
+};
+
 struct innolux_panel {
 	struct drm_panel base;
 	struct mipi_dsi_device *link;
+	const struct panel_desc_dsi *dsi_desc;
 
 	struct backlight_device *backlight;
-	struct regulator *supply;
+	struct regulator *vddi;
+	struct regulator *avdd;
+	struct regulator *avee;
 	struct gpio_desc *enable_gpio;
 
 	bool prepared;
@@ -78,9 +98,9 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
 	/* T8: 80ms - 1000ms */
 	msleep(80);
 
-	err = regulator_disable(innolux->supply);
-	if (err < 0)
-		return err;
+	regulator_disable(innolux->avee);
+	regulator_disable(innolux->avdd);
+	regulator_disable(innolux->vddi);
 
 	innolux->prepared = false;
 
@@ -97,10 +117,18 @@ static int innolux_panel_prepare(struct drm_panel *panel)
 
 	gpiod_set_value_cansleep(innolux->enable_gpio, 0);
 
-	err = regulator_enable(innolux->supply);
+	err = regulator_enable(innolux->vddi);
 	if (err < 0)
 		return err;
 
+	err = regulator_enable(innolux->avdd);
+	if (err < 0)
+		goto disable_vddi;
+
+	err = regulator_enable(innolux->avee);
+	if (err < 0)
+		goto disable_avdd;
+
 	/* T2: 15ms - 1000ms */
 	usleep_range(15000, 16000);
 
@@ -134,12 +162,13 @@ static int innolux_panel_prepare(struct drm_panel *panel)
 	return 0;
 
 poweroff:
-	regulator_err = regulator_disable(innolux->supply);
-	if (regulator_err)
-		DRM_DEV_ERROR(panel->dev, "failed to disable regulator: %d\n",
-			      regulator_err);
-
 	gpiod_set_value_cansleep(innolux->enable_gpio, 0);
+	regulator_disable(innolux->avee);
+disable_avdd:
+	regulator_disable(innolux->avdd);
+disable_vddi:
+	regulator_disable(innolux->vddi);
+
 	return err;
 }
 
@@ -164,7 +193,7 @@ static int innolux_panel_enable(struct drm_panel *panel)
 	return 0;
 }
 
-static const struct drm_display_mode default_mode = {
+static const struct drm_display_mode innolux_p079zca_mode = {
 	.clock = 56900,
 	.hdisplay = 768,
 	.hsync_start = 768 + 40,
@@ -177,15 +206,31 @@ static const struct drm_display_mode default_mode = {
 	.vrefresh = 60,
 };
 
+static const struct panel_desc_dsi innolux_p079zca_panel_desc = {
+	.desc = {
+		.modes = &innolux_p079zca_mode,
+		.bpc = 8,
+		.size = {
+			.width = 120,
+			.height = 160,
+		},
+	},
+	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+		 MIPI_DSI_MODE_LPM,
+	.format = MIPI_DSI_FMT_RGB888,
+	.lanes = 4,
+};
+
 static int innolux_panel_get_modes(struct drm_panel *panel)
 {
 	struct drm_display_mode *mode;
+	struct innolux_panel *innolux = to_innolux_panel(panel);
+	const struct drm_display_mode *m = innolux->dsi_desc->desc.modes;
 
-	mode = drm_mode_duplicate(panel->drm, &default_mode);
+	mode = drm_mode_duplicate(panel->drm, m);
 	if (!mode) {
 		DRM_DEV_ERROR(panel->drm->dev, "failed to add mode %ux%ux@%u\n",
-			      default_mode.hdisplay, default_mode.vdisplay,
-			      default_mode.vrefresh);
+			      m->hdisplay, m->vdisplay, m->vrefresh);
 		return -ENOMEM;
 	}
 
@@ -193,9 +238,11 @@ static int innolux_panel_get_modes(struct drm_panel *panel)
 
 	drm_mode_probed_add(panel->connector, mode);
 
-	panel->connector->display_info.width_mm = 120;
-	panel->connector->display_info.height_mm = 160;
-	panel->connector->display_info.bpc = 8;
+	panel->connector->display_info.width_mm =
+			innolux->dsi_desc->desc.size.width;
+	panel->connector->display_info.height_mm =
+			innolux->dsi_desc->desc.size.height;
+	panel->connector->display_info.bpc = innolux->dsi_desc->desc.bpc;
 
 	return 1;
 }
@@ -209,20 +256,36 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
 };
 
 static const struct of_device_id innolux_of_match[] = {
-	{ .compatible = "innolux,p079zca", },
-	{ }
+	{ .compatible = "innolux,p079zca",
+	  .data = &innolux_p079zca_panel_desc
+	},
 };
 MODULE_DEVICE_TABLE(of, innolux_of_match);
 
-static int innolux_panel_add(struct innolux_panel *innolux)
+static int innolux_panel_add(struct mipi_dsi_device *dsi,
+			     const struct panel_desc_dsi *desc)
 {
-	struct device *dev = &innolux->link->dev;
+	struct innolux_panel *innolux;
+	struct device *dev = &dsi->dev;
 	struct device_node *np;
 	int err;
 
-	innolux->supply = devm_regulator_get(dev, "power");
-	if (IS_ERR(innolux->supply))
-		return PTR_ERR(innolux->supply);
+	innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
+	if (!innolux)
+		return -ENOMEM;
+
+	innolux->dsi_desc = desc;
+	innolux->vddi = devm_regulator_get(dev, "power");
+	if (IS_ERR(innolux->vddi))
+		return PTR_ERR(innolux->vddi);
+
+	innolux->avdd = devm_regulator_get(dev, "avdd");
+	if (IS_ERR(innolux->avdd))
+		return PTR_ERR(innolux->avdd);
+
+	innolux->avee = devm_regulator_get(dev, "avee");
+	if (IS_ERR(innolux->avee))
+		return PTR_ERR(innolux->avee);
 
 	innolux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
 						       GPIOD_OUT_HIGH);
@@ -243,14 +306,16 @@ static int innolux_panel_add(struct innolux_panel *innolux)
 
 	drm_panel_init(&innolux->base);
 	innolux->base.funcs = &innolux_panel_funcs;
-	innolux->base.dev = &innolux->link->dev;
+	innolux->base.dev = dev;
 
 	err = drm_panel_add(&innolux->base);
 	if (err < 0)
 		goto put_backlight;
 
-	return 0;
+	dev_set_drvdata(dev, innolux);
+	innolux->link = dsi;
 
+	return 0;
 put_backlight:
 	put_device(&innolux->backlight->dev);
 
@@ -267,28 +332,25 @@ static void innolux_panel_del(struct innolux_panel *innolux)
 
 static int innolux_panel_probe(struct mipi_dsi_device *dsi)
 {
-	struct innolux_panel *innolux;
-	int err;
 
-	dsi->lanes = 4;
-	dsi->format = MIPI_DSI_FMT_RGB888;
-	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
-			  MIPI_DSI_MODE_LPM;
-
-	innolux = devm_kzalloc(&dsi->dev, sizeof(*innolux), GFP_KERNEL);
-	if (!innolux)
-		return -ENOMEM;
+	const struct panel_desc_dsi *dsi_desc;
+	const struct of_device_id *id;
+	int err;
 
-	mipi_dsi_set_drvdata(dsi, innolux);
+	id = of_match_node(innolux_of_match, dsi->dev.of_node);
+	if (!id)
+		return -ENODEV;
 
-	innolux->link = dsi;
+	dsi_desc = id->data;
+	dsi->mode_flags = dsi_desc->flags;
+	dsi->format = dsi_desc->format;
+	dsi->lanes = dsi_desc->lanes;
 
-	err = innolux_panel_add(innolux);
+	err = innolux_panel_add(dsi, dsi_desc);
 	if (err < 0)
 		return err;
 
-	err = mipi_dsi_attach(dsi);
-	return err;
+	return mipi_dsi_attach(dsi);
 }
 
 static int innolux_panel_remove(struct mipi_dsi_device *dsi)
@@ -326,7 +388,7 @@ static void innolux_panel_shutdown(struct mipi_dsi_device *dsi)
 
 static struct mipi_dsi_driver innolux_panel_driver = {
 	.driver = {
-		.name = "panel-innolux-p079zca",
+		.name = "panel-innolux-dsi",
 		.of_match_table = innolux_of_match,
 	},
 	.probe = innolux_panel_probe,
@@ -336,5 +398,6 @@ static struct mipi_dsi_driver innolux_panel_driver = {
 module_mipi_dsi_driver(innolux_panel_driver);
 
 MODULE_AUTHOR("Chris Zhong <zyw@rock-chips.com>");
+MODULE_AUTHOR("Lin Huang <hl@rock-chips.com>");
 MODULE_DESCRIPTION("Innolux P079ZCA panel driver");
 MODULE_LICENSE("GPL v2");
-- 
2.7.4

[RESEND PATCH v3 2/3] drm/panel: support Innolux P097PFG panel

[Lin Huang]

Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
the Innolux P079ZCA panel driver.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 drivers/gpu/drm/panel/Kconfig                 |  9 ++++----
 drivers/gpu/drm/panel/panel-innolux-p079zca.c | 31 +++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 726f3fb..429cf59 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -29,15 +29,14 @@ config DRM_PANEL_SIMPLE
 	  low power state.
 
 config DRM_PANEL_INNOLUX_P079ZCA
-	tristate "Innolux P079ZCA panel"
+	tristate "Innolux P079ZCA or P097PFG panel"
 	depends on OF
 	depends on DRM_MIPI_DSI
 	depends on BACKLIGHT_CLASS_DEVICE
 	help
-	  Say Y here if you want to enable support for Innolux P079ZCA
-	  TFT-LCD modules. The panel has a 1024x768 resolution and uses
-	  24 bit RGB per pixel. It provides a MIPI DSI interface to
-	  the host and has a built-in LED backlight.
+	  Say Y here if you want to enable support for Innolux P079ZCA or
+	  Innolux P097PFG panel. They provide a MIPI DSI interface to
+	  the host and have a built-in LED backlight.
 
 config DRM_PANEL_JDI_LT070ME05000
 	tristate "JDI LT070ME05000 WUXGA DSI panel"
diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 1597744..5d690b7 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -221,6 +221,34 @@ static const struct panel_desc_dsi innolux_p079zca_panel_desc = {
 	.lanes = 4,
 };
 
+static const struct drm_display_mode innolux_p097pfg_mode = {
+	.clock = 220000,
+	.hdisplay = 1536,
+	.hsync_start = 1536 + 100,
+	.hsync_end = 1536 + 100 + 24,
+	.htotal = 1536 + 100 + 24 + 100,
+	.vdisplay = 2048,
+	.vsync_start = 2048 + 18,
+	.vsync_end = 2048 + 18 + 2,
+	.vtotal = 2048 + 18 + 2 + 18,
+	.vrefresh = 60,
+};
+
+static const struct panel_desc_dsi innolux_p097pfg_panel_desc = {
+	.desc = {
+		.modes = &innolux_p097pfg_mode,
+		.bpc = 8,
+		.size = {
+			.width = 147,
+			.height = 196,
+		},
+	},
+	.flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+		 MIPI_DSI_MODE_LPM,
+	.format = MIPI_DSI_FMT_RGB888,
+	.lanes = 8,
+};
+
 static int innolux_panel_get_modes(struct drm_panel *panel)
 {
 	struct drm_display_mode *mode;
@@ -259,6 +287,9 @@ static const struct of_device_id innolux_of_match[] = {
 	{ .compatible = "innolux,p079zca",
 	  .data = &innolux_p079zca_panel_desc
 	},
+	{ .compatible = "innolux,p097pfg",
+	  .data = &innolux_p097pfg_panel_desc
+	}
 };
 MODULE_DEVICE_TABLE(of, innolux_of_match);
 
-- 
2.7.4

[RESEND PATCH v3 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings

[Lin Huang]

The Innolux P097PFG panel is 9.7" panel with 1536X2048
resolution, it reuse P079ZCA panel driver, so improve
p079ZCA dt-binding to support P097PFG.

Signed-off-by: Lin Huang <hl@rock-chips.com>
---
 .../devicetree/bindings/display/panel/innolux,p079zca.txt     | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
index d0f5516..8cadd8c 100644
--- a/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
+++ b/Documentation/devicetree/bindings/display/panel/innolux,p079zca.txt
@@ -1,13 +1,18 @@
 Innolux P079ZCA 7.85" 768x1024 TFT LCD panel
+Innolux P097PFG 9.7" 1536x2048 TFT LCD panel
 
 Required properties:
-- compatible: should be "innolux,p079zca"
+- compatible: should be should be one of the following.
+    -"innolux,p079zca" for Innolux 7.9" P079ZCA 768*1024 panel
+    -"innolux,p097pfg" for Innolux 9.7" P097PFG 1536*2048 panel
 - reg: DSI virtual channel of the peripheral
-- power-supply: phandle of the regulator that provides the supply voltage
 - enable-gpios: panel enable gpio
 
 Optional properties:
 - backlight: phandle of the backlight device attached to the panel
+- power-supply: phandle of the regulator that provides the supply voltage
+- avdd-supply: phandle of the regulator that provides positive voltage
+- avee-supply: phandle of the regulator that provides negative voltage
 
 Example:
 
@@ -16,6 +21,8 @@ Example:
 			compatible = "innolux,p079zca";
 			reg = <0>;
 			power-supply = <...>;
+			avdd-supply = <...>;
+			avee-supply = <...>;
 			backlight = <&backlight>;
 			enable-gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>;
 		};
-- 
2.7.4

Re: [RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Brian Norris]

Hi HL,

On Mon, Dec 04, 2017 at 03:17:48PM +0800, Lin Huang wrote:
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - Change regulator property name to meet the panel datasheet 
> Changes in v3:
> - this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch 
> 
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 6ba9344..1597744 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
...

> @@ -134,12 +162,13 @@ static int innolux_panel_prepare(struct drm_panel *panel)
>  	return 0;
>  
>  poweroff:
> -	regulator_err = regulator_disable(innolux->supply);

I think 'regulator_err' is unused after this patch. Please remove it.

> -	if (regulator_err)
> -		DRM_DEV_ERROR(panel->dev, "failed to disable regulator: %d\n",
> -			      regulator_err);
> -
>  	gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> +	regulator_disable(innolux->avee);
> +disable_avdd:
> +	regulator_disable(innolux->avdd);
> +disable_vddi:
> +	regulator_disable(innolux->vddi);
> +
>  	return err;
>  }
>  

...

> @@ -209,20 +256,36 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>  };
>  
>  static const struct of_device_id innolux_of_match[] = {
> -	{ .compatible = "innolux,p079zca", },
> -	{ }

You're deleting this terminator. Please don't.

> +	{ .compatible = "innolux,p079zca",
> +	  .data = &innolux_p079zca_panel_desc
> +	},
>  };
>  MODULE_DEVICE_TABLE(of, innolux_of_match);
>  

...

Brian

Re: [RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Brian Norris]

Hi HL,

On Mon, Dec 04, 2017 at 03:17:48PM +0800, Lin Huang wrote:
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - Change regulator property name to meet the panel datasheet 
> Changes in v3:
> - this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch 
> 
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 6ba9344..1597744 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
...

> @@ -134,12 +162,13 @@ static int innolux_panel_prepare(struct drm_panel *panel)
>  	return 0;
>  
>  poweroff:
> -	regulator_err = regulator_disable(innolux->supply);

I think 'regulator_err' is unused after this patch. Please remove it.

> -	if (regulator_err)
> -		DRM_DEV_ERROR(panel->dev, "failed to disable regulator: %d\n",
> -			      regulator_err);
> -
>  	gpiod_set_value_cansleep(innolux->enable_gpio, 0);
> +	regulator_disable(innolux->avee);
> +disable_avdd:
> +	regulator_disable(innolux->avdd);
> +disable_vddi:
> +	regulator_disable(innolux->vddi);
> +
>  	return err;
>  }
>  

...

> @@ -209,20 +256,36 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>  };
>  
>  static const struct of_device_id innolux_of_match[] = {
> -	{ .compatible = "innolux,p079zca", },
> -	{ }

You're deleting this terminator. Please don't.

> +	{ .compatible = "innolux,p079zca",
> +	  .data = &innolux_p079zca_panel_desc
> +	},
>  };
>  MODULE_DEVICE_TABLE(of, innolux_of_match);
>  

...

Brian
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 5241 bytes --]

On Mon, Dec 04, 2017 at 03:17:48PM +0800, Lin Huang wrote:
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
> 
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - Change regulator property name to meet the panel datasheet 
> Changes in v3:
> - this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch 
> 
>  drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
>  1 file changed, 105 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 6ba9344..1597744 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,12 +20,32 @@
>  
>  #include <video/mipi_display.h>
>  
> +struct panel_desc {
> +	const struct drm_display_mode *modes;
> +	unsigned int bpc;
> +	struct {
> +		unsigned int width;
> +		unsigned int height;
> +	} size;
> +};
> +
> +struct panel_desc_dsi {
> +	struct panel_desc desc;
> +
> +	unsigned long flags;
> +	enum mipi_dsi_pixel_format format;
> +	unsigned int lanes;
> +};

There's no need for the two layers here. Just move everything from
struct panel_desc_dsi into struct panel_desc.

> +
>  struct innolux_panel {
>  	struct drm_panel base;
>  	struct mipi_dsi_device *link;
> +	const struct panel_desc_dsi *dsi_desc;

And then this can just become:

	const struct panel_desc *desc;

The _dsi suffix is redundant because this driver is exclusively for DSI
devices.

> -static int innolux_panel_add(struct innolux_panel *innolux)
> +static int innolux_panel_add(struct mipi_dsi_device *dsi,
> +			     const struct panel_desc_dsi *desc)
>  {
> -	struct device *dev = &innolux->link->dev;
> +	struct innolux_panel *innolux;
> +	struct device *dev = &dsi->dev;
>  	struct device_node *np;
>  	int err;
>  
> -	innolux->supply = devm_regulator_get(dev, "power");
> -	if (IS_ERR(innolux->supply))
> -		return PTR_ERR(innolux->supply);
> +	innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
> +	if (!innolux)
> +		return -ENOMEM;
> +
> +	innolux->dsi_desc = desc;
> +	innolux->vddi = devm_regulator_get(dev, "power");
> +	if (IS_ERR(innolux->vddi))
> +		return PTR_ERR(innolux->vddi);
> +
> +	innolux->avdd = devm_regulator_get(dev, "avdd");
> +	if (IS_ERR(innolux->avdd))
> +		return PTR_ERR(innolux->avdd);
> +
> +	innolux->avee = devm_regulator_get(dev, "avee");
> +	if (IS_ERR(innolux->avee))
> +		return PTR_ERR(innolux->avee);

According to the device tree bindings these regulators are all optional.
Now devm_regulator_get() will return a dummy regulator if one has not
been specified in DT, so this seems like it should work fine, even for
existing DTBs that don't have the avdd and avee regulators. However, the
DT bindings seem to be wrong, because these are in fact required
regulators.

>  
>  	innolux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
>  						       GPIOD_OUT_HIGH);
> @@ -243,14 +306,16 @@ static int innolux_panel_add(struct innolux_panel *innolux)
>  
>  	drm_panel_init(&innolux->base);
>  	innolux->base.funcs = &innolux_panel_funcs;
> -	innolux->base.dev = &innolux->link->dev;
> +	innolux->base.dev = dev;
>  
>  	err = drm_panel_add(&innolux->base);
>  	if (err < 0)
>  		goto put_backlight;
>  
> -	return 0;
> +	dev_set_drvdata(dev, innolux);
> +	innolux->link = dsi;
>  
> +	return 0;
>  put_backlight:
>  	put_device(&innolux->backlight->dev);
>  
> @@ -267,28 +332,25 @@ static void innolux_panel_del(struct innolux_panel *innolux)
>  
>  static int innolux_panel_probe(struct mipi_dsi_device *dsi)
>  {
> -	struct innolux_panel *innolux;
> -	int err;
>  
> -	dsi->lanes = 4;
> -	dsi->format = MIPI_DSI_FMT_RGB888;
> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> -			  MIPI_DSI_MODE_LPM;
> -
> -	innolux = devm_kzalloc(&dsi->dev, sizeof(*innolux), GFP_KERNEL);
> -	if (!innolux)
> -		return -ENOMEM;
> +	const struct panel_desc_dsi *dsi_desc;
> +	const struct of_device_id *id;
> +	int err;
>  
> -	mipi_dsi_set_drvdata(dsi, innolux);
> +	id = of_match_node(innolux_of_match, dsi->dev.of_node);
> +	if (!id)
> +		return -ENODEV;
>  
> -	innolux->link = dsi;
> +	dsi_desc = id->data;
> +	dsi->mode_flags = dsi_desc->flags;
> +	dsi->format = dsi_desc->format;
> +	dsi->lanes = dsi_desc->lanes;
>  
> -	err = innolux_panel_add(innolux);
> +	err = innolux_panel_add(dsi, dsi_desc);
>  	if (err < 0)
>  		return err;
>  
> -	err = mipi_dsi_attach(dsi);
> -	return err;
> +	return mipi_dsi_attach(dsi);
>  }
>  
>  static int innolux_panel_remove(struct mipi_dsi_device *dsi)
> @@ -326,7 +388,7 @@ static void innolux_panel_shutdown(struct mipi_dsi_device *dsi)
>  
>  static struct mipi_dsi_driver innolux_panel_driver = {
>  	.driver = {
> -		.name = "panel-innolux-p079zca",
> +		.name = "panel-innolux-dsi",

No need to change that. Also, probably a bad idea to change it because
there are (or will be) likely other DSI panels from Innolux that are not
compatible with this driver.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RESEND PATCH v3 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver

[hl]

Hi Thierry Reding,


On Monday, March 12, 2018 05:21 PM, Thierry Reding wrote:
> On Mon, Dec 04, 2017 at 03:17:48PM +0800, Lin Huang wrote:
>> Refactor Innolux P079ZCA panel driver, let it support
>> multi panel.
>>
>> Signed-off-by: Lin Huang <hl@rock-chips.com>
>> ---
>> Changes in v2:
>> - Change regulator property name to meet the panel datasheet
>> Changes in v3:
>> - this patch only refactor P079ZCA panel to support multi panel, support P097PFG panel in another patch
>>
>>   drivers/gpu/drm/panel/panel-innolux-p079zca.c | 147 ++++++++++++++++++--------
>>   1 file changed, 105 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> index 6ba9344..1597744 100644
>> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
>> @@ -20,12 +20,32 @@
>>   
>>   #include <video/mipi_display.h>
>>   
>> +struct panel_desc {
>> +	const struct drm_display_mode *modes;
>> +	unsigned int bpc;
>> +	struct {
>> +		unsigned int width;
>> +		unsigned int height;
>> +	} size;
>> +};
>> +
>> +struct panel_desc_dsi {
>> +	struct panel_desc desc;
>> +
>> +	unsigned long flags;
>> +	enum mipi_dsi_pixel_format format;
>> +	unsigned int lanes;
>> +};
> There's no need for the two layers here. Just move everything from
> struct panel_desc_dsi into struct panel_desc.
Okay, will fix it.
>
>> +
>>   struct innolux_panel {
>>   	struct drm_panel base;
>>   	struct mipi_dsi_device *link;
>> +	const struct panel_desc_dsi *dsi_desc;
> And then this can just become:
>
> 	const struct panel_desc *desc;
>
> The _dsi suffix is redundant because this driver is exclusively for DSI
> devices.
>
Got it.
>> -static int innolux_panel_add(struct innolux_panel *innolux)
>> +static int innolux_panel_add(struct mipi_dsi_device *dsi,
>> +			     const struct panel_desc_dsi *desc)
>>   {
>> -	struct device *dev = &innolux->link->dev;
>> +	struct innolux_panel *innolux;
>> +	struct device *dev = &dsi->dev;
>>   	struct device_node *np;
>>   	int err;
>>   
>> -	innolux->supply = devm_regulator_get(dev, "power");
>> -	if (IS_ERR(innolux->supply))
>> -		return PTR_ERR(innolux->supply);
>> +	innolux = devm_kzalloc(dev, sizeof(*innolux), GFP_KERNEL);
>> +	if (!innolux)
>> +		return -ENOMEM;
>> +
>> +	innolux->dsi_desc = desc;
>> +	innolux->vddi = devm_regulator_get(dev, "power");
>> +	if (IS_ERR(innolux->vddi))
>> +		return PTR_ERR(innolux->vddi);
>> +
>> +	innolux->avdd = devm_regulator_get(dev, "avdd");
>> +	if (IS_ERR(innolux->avdd))
>> +		return PTR_ERR(innolux->avdd);
>> +
>> +	innolux->avee = devm_regulator_get(dev, "avee");
>> +	if (IS_ERR(innolux->avee))
>> +		return PTR_ERR(innolux->avee);
> According to the device tree bindings these regulators are all optional.
> Now devm_regulator_get() will return a dummy regulator if one has not
> been specified in DT, so this seems like it should work fine, even for
> existing DTBs that don't have the avdd and avee regulators. However, the
> DT bindings seem to be wrong, because these are in fact required
> regulators.
Actually, the vddi is request, and avdd and avee is optional, i will 
only check
vddi error later.
>>   
>>   	innolux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
>>   						       GPIOD_OUT_HIGH);
>> @@ -243,14 +306,16 @@ static int innolux_panel_add(struct innolux_panel *innolux)
>>   
>>   	drm_panel_init(&innolux->base);
>>   	innolux->base.funcs = &innolux_panel_funcs;
>> -	innolux->base.dev = &innolux->link->dev;
>> +	innolux->base.dev = dev;
>>   
>>   	err = drm_panel_add(&innolux->base);
>>   	if (err < 0)
>>   		goto put_backlight;
>>   
>> -	return 0;
>> +	dev_set_drvdata(dev, innolux);
>> +	innolux->link = dsi;
>>   
>> +	return 0;
>>   put_backlight:
>>   	put_device(&innolux->backlight->dev);
>>   
>> @@ -267,28 +332,25 @@ static void innolux_panel_del(struct innolux_panel *innolux)
>>   
>>   static int innolux_panel_probe(struct mipi_dsi_device *dsi)
>>   {
>> -	struct innolux_panel *innolux;
>> -	int err;
>>   
>> -	dsi->lanes = 4;
>> -	dsi->format = MIPI_DSI_FMT_RGB888;
>> -	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>> -			  MIPI_DSI_MODE_LPM;
>> -
>> -	innolux = devm_kzalloc(&dsi->dev, sizeof(*innolux), GFP_KERNEL);
>> -	if (!innolux)
>> -		return -ENOMEM;
>> +	const struct panel_desc_dsi *dsi_desc;
>> +	const struct of_device_id *id;
>> +	int err;
>>   
>> -	mipi_dsi_set_drvdata(dsi, innolux);
>> +	id = of_match_node(innolux_of_match, dsi->dev.of_node);
>> +	if (!id)
>> +		return -ENODEV;
>>   
>> -	innolux->link = dsi;
>> +	dsi_desc = id->data;
>> +	dsi->mode_flags = dsi_desc->flags;
>> +	dsi->format = dsi_desc->format;
>> +	dsi->lanes = dsi_desc->lanes;
>>   
>> -	err = innolux_panel_add(innolux);
>> +	err = innolux_panel_add(dsi, dsi_desc);
>>   	if (err < 0)
>>   		return err;
>>   
>> -	err = mipi_dsi_attach(dsi);
>> -	return err;
>> +	return mipi_dsi_attach(dsi);
>>   }
>>   
>>   static int innolux_panel_remove(struct mipi_dsi_device *dsi)
>> @@ -326,7 +388,7 @@ static void innolux_panel_shutdown(struct mipi_dsi_device *dsi)
>>   
>>   static struct mipi_dsi_driver innolux_panel_driver = {
>>   	.driver = {
>> -		.name = "panel-innolux-p079zca",
>> +		.name = "panel-innolux-dsi",
> No need to change that. Also, probably a bad idea to change it because
> there are (or will be) likely other DSI panels from Innolux that are not
> compatible with this driver.
Okay, will fix it.
>
> Thierry