* [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-03-14 9:12 Lin Huang
2018-03-14 9:12 ` [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel Lin Huang
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Lin Huang @ 2018-03-14 9:12 UTC (permalink / raw)
To: thierry.reding
Cc: emil.l.velikov, briannorris, robh+dt, seanpaul, airlied,
linux-kernel, dri-devel, huang lin
From: huang lin <hl@rock-chips.com>
Refactor Innolux P079ZCA panel driver, let it support
multi panel.
Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
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
Changes in v4:
- Modify the patch which suggest by Thierry
drivers/gpu/drm/panel/panel-innolux-p079zca.c | 142 ++++++++++++++++++--------
1 file changed, 101 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 57df39b..2075a9d 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -20,12 +20,28 @@
#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;
+
+ 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 *desc;
struct backlight_device *backlight;
- struct regulator *supply;
+ struct regulator *vddi;
+ struct regulator *avdd;
+ struct regulator *avee;
struct gpio_desc *enable_gpio;
bool prepared;
@@ -77,9 +93,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;
@@ -89,17 +105,25 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
static int innolux_panel_prepare(struct drm_panel *panel)
{
struct innolux_panel *innolux = to_innolux_panel(panel);
- int err, regulator_err;
+ int err;
if (innolux->prepared)
return 0;
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);
@@ -133,12 +157,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;
}
@@ -162,7 +187,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,
@@ -175,15 +200,29 @@ static const struct drm_display_mode default_mode = {
.vrefresh = 60,
};
+static const struct panel_desc innolux_p079zca_panel_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->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;
}
@@ -191,9 +230,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->desc->size.width;
+ panel->connector->display_info.height_mm =
+ innolux->desc->size.height;
+ panel->connector->display_info.bpc = innolux->desc->bpc;
return 1;
}
@@ -207,19 +248,28 @@ 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 *desc)
{
- struct device *dev = &innolux->link->dev;
+ struct innolux_panel *innolux;
+ struct device *dev = &dsi->dev;
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->desc = desc;
+ innolux->vddi = devm_regulator_get(dev, "power");
+ innolux->avdd = devm_regulator_get(dev, "avdd");
+ innolux->avee = devm_regulator_get(dev, "avee");
innolux->enable_gpio = devm_gpiod_get_optional(dev, "enable",
GPIOD_OUT_HIGH);
@@ -236,9 +286,21 @@ 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;
+
+ dev_set_drvdata(dev, innolux);
+ innolux->link = dsi;
+
+ return 0;
+
+put_backlight:
+ put_device(&innolux->backlight->dev);
- return drm_panel_add(&innolux->base);
+ return err;
}
static void innolux_panel_del(struct innolux_panel *innolux)
@@ -249,28 +311,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 *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;
+ desc = id->data;
+ dsi->mode_flags = desc->flags;
+ dsi->format = desc->format;
+ dsi->lanes = desc->lanes;
- err = innolux_panel_add(innolux);
+ err = innolux_panel_add(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)
@@ -318,5 +377,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel
2018-03-14 9:12 [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Lin Huang
@ 2018-03-14 9:12 ` Lin Huang
2018-03-14 12:16 ` Emil Velikov
2018-03-14 9:12 ` [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings Lin Huang
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Lin Huang @ 2018-03-14 9:12 UTC (permalink / raw)
To: thierry.reding
Cc: emil.l.velikov, briannorris, robh+dt, seanpaul, airlied,
linux-kernel, dri-devel, Lin Huang
Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
the Innolux P079ZCA panel driver.
Change-Id: I97923aa3735f707332681691b0231c9421b427d0
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- None
Changes in v3:
- None
Changes in v4:
- download panel initial code
drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 +++++++++++++++++++++++++-
1 file changed, 187 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 2075a9d..883b279 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -20,6 +20,15 @@
#include <video/mipi_display.h>
+struct panel_init_cmd {
+ int len;
+ const char *data;
+};
+
+#define _INIT_CMD(...) { \
+ .len = sizeof((char[]){__VA_ARGS__}), \
+ .data = (char[]){__VA_ARGS__} }
+
struct panel_desc {
const struct drm_display_mode *modes;
unsigned int bpc;
@@ -30,6 +39,7 @@ struct panel_desc {
unsigned long flags;
enum mipi_dsi_pixel_format format;
+ const struct panel_init_cmd *init_cmds;
unsigned int lanes;
};
@@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
return err;
}
+ /* p097pfg: t15 */
+ msleep(100);
+
gpiod_set_value_cansleep(innolux->enable_gpio, 0);
- /* T8: 80ms - 1000ms */
+ /* p079zca: t8*/
msleep(80);
regulator_disable(innolux->avee);
@@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel *panel)
if (err < 0)
goto disable_avdd;
- /* T2: 15ms - 1000ms */
- usleep_range(15000, 16000);
+ /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
+ usleep_range(20000, 21000);
gpiod_set_value_cansleep(innolux->enable_gpio, 1);
- /* T4: 15ms - 1000ms */
- usleep_range(15000, 16000);
+ /* p079zca: t4, p097pfg: t5 */
+ usleep_range(20000, 21000);
+
+ if (innolux->desc->init_cmds) {
+ const struct panel_init_cmd *cmds =
+ innolux->desc->init_cmds;
+ int i;
+
+ for (i = 0; cmds[i].len != 0; i++) {
+ const struct panel_init_cmd *cmd = &cmds[i];
+
+ err = mipi_dsi_generic_write(innolux->link, cmd->data,
+ cmd->len);
+ if (err < 0) {
+ dev_err(panel->dev,
+ "failed to write command %d\n", i);
+ goto poweroff;
+ }
+
+ /*
+ * Included by random guessing, because without this
+ * (or at least, some delay), the panel sometimes
+ * didn't appear to pick up the command sequence.
+ */
+ err = mipi_dsi_dcs_nop(innolux->link);
+ if (err < 0) {
+ dev_err(panel->dev,
+ "failed to send DCS nop: %d\n", err);
+ goto poweroff;
+ }
+ }
+ }
err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
if (err < 0) {
@@ -213,6 +256,142 @@ static const struct panel_desc innolux_p079zca_panel_desc = {
.lanes = 4,
};
+static const struct drm_display_mode innolux_p097pfg_mode = {
+ .clock = 229000,
+ .hdisplay = 1536,
+ .hsync_start = 1536 + 100,
+ .hsync_end = 1536 + 100 + 24,
+ .htotal = 1536 + 100 + 24 + 100,
+ .vdisplay = 2048,
+ .vsync_start = 2048 + 100,
+ .vsync_end = 2048 + 100 + 2,
+ .vtotal = 2048 + 100 + 2 + 18,
+ .vrefresh = 60,
+};
+
+static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {
+ /* page 0 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x00),
+ _INIT_CMD(0xB1, 0xE8, 0x11),
+ _INIT_CMD(0xB2, 0x25, 0x02),
+ _INIT_CMD(0xB5, 0x08, 0x00),
+ _INIT_CMD(0xBC, 0x0F, 0x00),
+ _INIT_CMD(0xB8, 0x03, 0x06, 0x00, 0x00),
+ _INIT_CMD(0xBD, 0x01, 0x90, 0x14, 0x14),
+ _INIT_CMD(0x6F, 0x01),
+ _INIT_CMD(0xC0, 0x03),
+ _INIT_CMD(0x6F, 0x02),
+ _INIT_CMD(0xC1, 0x0D),
+ _INIT_CMD(0xD9, 0x01, 0x09, 0x70),
+ _INIT_CMD(0xC5, 0x12, 0x21, 0x00),
+ _INIT_CMD(0xBB, 0x93, 0x93),
+
+ /* page 1 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x01),
+ _INIT_CMD(0xB3, 0x3C, 0x3C),
+ _INIT_CMD(0xB4, 0x0F, 0x0F),
+ _INIT_CMD(0xB9, 0x45, 0x45),
+ _INIT_CMD(0xBA, 0x14, 0x14),
+ _INIT_CMD(0xCA, 0x02),
+ _INIT_CMD(0xCE, 0x04),
+ _INIT_CMD(0xC3, 0x9B, 0x9B),
+ _INIT_CMD(0xD8, 0xC0, 0x03),
+ _INIT_CMD(0xBC, 0x82, 0x01),
+ _INIT_CMD(0xBD, 0x9E, 0x01),
+
+ /* page 2 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x02),
+ _INIT_CMD(0xB0, 0x82),
+ _INIT_CMD(0xD1, 0x00, 0x00, 0x00, 0x3E, 0x00, 0x82, 0x00, 0xA5,
+ 0x00, 0xC1, 0x00, 0xEA, 0x01, 0x0D, 0x01, 0x40),
+ _INIT_CMD(0xD2, 0x01, 0x6A, 0x01, 0xA8, 0x01, 0xDC, 0x02, 0x29,
+ 0x02, 0x67, 0x02, 0x68, 0x02, 0xA8, 0x02, 0xF0),
+ _INIT_CMD(0xD3, 0x03, 0x19, 0x03, 0x49, 0x03, 0x67, 0x03, 0x8C,
+ 0x03, 0xA6, 0x03, 0xC7, 0x03, 0xDE, 0x03, 0xEC),
+ _INIT_CMD(0xD4, 0x03, 0xFF, 0x03, 0xFF),
+ _INIT_CMD(0xE0, 0x00, 0x00, 0x00, 0x86, 0x00, 0xC5, 0x00, 0xE5,
+ 0x00, 0xFF, 0x01, 0x26, 0x01, 0x45, 0x01, 0x75),
+ _INIT_CMD(0xE1, 0x01, 0x9C, 0x01, 0xD5, 0x02, 0x05, 0x02, 0x4D,
+ 0x02, 0x86, 0x02, 0x87, 0x02, 0xC3, 0x03, 0x03),
+ _INIT_CMD(0xE2, 0x03, 0x2A, 0x03, 0x56, 0x03, 0x72, 0x03, 0x94,
+ 0x03, 0xAC, 0x03, 0xCB, 0x03, 0xE0, 0x03, 0xED),
+ _INIT_CMD(0xE3, 0x03, 0xFF, 0x03, 0xFF),
+
+ /* page 3 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x03),
+ _INIT_CMD(0xB0, 0x00, 0x00, 0x00, 0x00),
+ _INIT_CMD(0xB1, 0x00, 0x00, 0x00, 0x00),
+ _INIT_CMD(0xB2, 0x00, 0x00, 0x06, 0x04, 0x01, 0x40, 0x85),
+ _INIT_CMD(0xB3, 0x10, 0x07, 0xFC, 0x04, 0x01, 0x40, 0x80),
+ _INIT_CMD(0xB6, 0xF0, 0x08, 0x00, 0x04, 0x00, 0x00, 0x00, 0x01,
+ 0x40, 0x80),
+ _INIT_CMD(0xBA, 0xC5, 0x07, 0x00, 0x04, 0x11, 0x25, 0x8C),
+ _INIT_CMD(0xBB, 0xC5, 0x07, 0x00, 0x03, 0x11, 0x25, 0x8C),
+ _INIT_CMD(0xC0, 0x00, 0x3C, 0x00, 0x00, 0x00, 0x80, 0x80),
+ _INIT_CMD(0xC1, 0x00, 0x3C, 0x00, 0x00, 0x00, 0x80, 0x80),
+ _INIT_CMD(0xC4, 0x00, 0x00),
+ _INIT_CMD(0xEF, 0x41),
+
+ /* page 4 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x04),
+ _INIT_CMD(0xEC, 0x4C),
+
+ /* page 5 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x05),
+ _INIT_CMD(0xB0, 0x13, 0x03, 0x03, 0x01),
+ _INIT_CMD(0xB1, 0x30, 0x00),
+ _INIT_CMD(0xB2, 0x02, 0x02, 0x00),
+ _INIT_CMD(0xB3, 0x82, 0x23, 0x82, 0x9D),
+ _INIT_CMD(0xB4, 0xC5, 0x75, 0x24, 0x57),
+ _INIT_CMD(0xB5, 0x00, 0xD4, 0x72, 0x11, 0x11, 0xAB, 0x0A),
+ _INIT_CMD(0xB6, 0x00, 0x00, 0xD5, 0x72, 0x24, 0x56),
+ _INIT_CMD(0xB7, 0x5C, 0xDC, 0x5C, 0x5C),
+ _INIT_CMD(0xB9, 0x0C, 0x00, 0x00, 0x01, 0x00),
+ _INIT_CMD(0xC0, 0x75, 0x11, 0x11, 0x54, 0x05),
+ _INIT_CMD(0xC6, 0x00, 0x00, 0x00, 0x00),
+ _INIT_CMD(0xD0, 0x00, 0x48, 0x08, 0x00, 0x00),
+ _INIT_CMD(0xD1, 0x00, 0x48, 0x09, 0x00, 0x00),
+
+ /* page 6 */
+ _INIT_CMD(0xF0, 0x55, 0xAA, 0x52, 0x08, 0x06),
+ _INIT_CMD(0xB0, 0x02, 0x32, 0x32, 0x08, 0x2F),
+ _INIT_CMD(0xB1, 0x2E, 0x15, 0x14, 0x13, 0x12),
+ _INIT_CMD(0xB2, 0x11, 0x10, 0x00, 0x3D, 0x3D),
+ _INIT_CMD(0xB3, 0x3D, 0x3D, 0x3D, 0x3D, 0x3D),
+ _INIT_CMD(0xB4, 0x3D, 0x32),
+ _INIT_CMD(0xB5, 0x03, 0x32, 0x32, 0x09, 0x2F),
+ _INIT_CMD(0xB6, 0x2E, 0x1B, 0x1A, 0x19, 0x18),
+ _INIT_CMD(0xB7, 0x17, 0x16, 0x01, 0x3D, 0x3D),
+ _INIT_CMD(0xB8, 0x3D, 0x3D, 0x3D, 0x3D, 0x3D),
+ _INIT_CMD(0xB9, 0x3D, 0x32),
+ _INIT_CMD(0xC0, 0x01, 0x32, 0x32, 0x09, 0x2F),
+ _INIT_CMD(0xC1, 0x2E, 0x1A, 0x1B, 0x16, 0x17),
+ _INIT_CMD(0xC2, 0x18, 0x19, 0x03, 0x3D, 0x3D),
+ _INIT_CMD(0xC3, 0x3D, 0x3D, 0x3D, 0x3D, 0x3D),
+ _INIT_CMD(0xC4, 0x3D, 0x32),
+ _INIT_CMD(0xC5, 0x00, 0x32, 0x32, 0x08, 0x2F),
+ _INIT_CMD(0xC6, 0x2E, 0x14, 0x15, 0x10, 0x11),
+ _INIT_CMD(0xC7, 0x12, 0x13, 0x02, 0x3D, 0x3D),
+ _INIT_CMD(0xC8, 0x3D, 0x3D, 0x3D, 0x3D, 0x3D),
+ _INIT_CMD(0xC9, 0x3D, 0x32),
+
+ {},
+};
+
+static const struct panel_desc innolux_p097pfg_panel_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,
+ .init_cmds = innolux_p097pfg_init_cmds,
+ .lanes = 8,
+};
+
static int innolux_panel_get_modes(struct drm_panel *panel)
{
struct drm_display_mode *mode;
@@ -251,6 +430,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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings
2018-03-14 9:12 [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Lin Huang
2018-03-14 9:12 ` [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel Lin Huang
@ 2018-03-14 9:12 ` Lin Huang
2018-03-14 12:18 ` Emil Velikov
2018-03-14 12:02 ` Emil Velikov
2018-04-26 14:10 ` Thierry Reding
3 siblings, 1 reply; 20+ messages in thread
From: Lin Huang @ 2018-03-14 9:12 UTC (permalink / raw)
To: thierry.reding
Cc: emil.l.velikov, briannorris, robh+dt, seanpaul, airlied,
linux-kernel, dri-devel, huang lin
From: huang lin <hl@rock-chips.com>
The Innolux P097PFG panel is 9.7" panel with 1536X2048
resolution, it reuse P079ZCA panel driver, so improve
p079ZCA dt-binding to support P097PFG.
Change-Id: I8704914898fe53b734d31fbe646df8aa5fd8b30d
Signed-off-by: Lin Huang <hl@rock-chips.com>
---
Changes in v2:
- None
Changes in v3:
- None
Changes in v4:
- None
.../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
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-14 9:12 [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Lin Huang
@ 2018-03-14 12:02 ` Emil Velikov
2018-03-14 9:12 ` [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings Lin Huang
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:02 UTC (permalink / raw)
To: Lin Huang
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi Lin,
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> From: huang lin <hl@rock-chips.com>
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> 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
> Changes in v4:
> - Modify the patch which suggest by Thierry
>
Thanks for splitting this up. I think there's another piece that fell
through the cracks.
I'm not deeply familiar with the driver, so just sharing some quick notes.
> struct innolux_panel {
> struct drm_panel base;
> struct mipi_dsi_device *link;
> + const struct panel_desc *desc;
>
> struct backlight_device *backlight;
> - struct regulator *supply;
> + struct regulator *vddi;
> + struct regulator *avdd;
> + struct regulator *avee;
These two seem are new addition, as opposed to a dummy refactor.
Are they optional, does one need them for the existing panel (separate
patch?) or only for the new one (squash with the new panel code)?
> struct gpio_desc *enable_gpio;
>
> bool prepared;
> @@ -77,9 +93,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;
Good call on dropping the early return here.
> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>
> - 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->desc = desc;
> + innolux->vddi = devm_regulator_get(dev, "power");
> + innolux->avdd = devm_regulator_get(dev, "avdd");
> + innolux->avee = devm_regulator_get(dev, "avee");
>
AFAICT devm_regulator_get returns a pointer which is unsuitable to be
passed into regulator_{enable,disable}.
Hence, the IS_ERR check should stay. If any of the regulators are
optional, you want to call regulator_{enable,disable} only as
applicable.
> @@ -318,5 +377,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>");
I don't think refactoring existing code classify as being the module author.
Then again, I could be wrong.
HTH
Emil
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-03-14 12:02 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:02 UTC (permalink / raw)
To: Lin Huang
Cc: David Airlie, Brian Norris, Linux-Kernel@Vger. Kernel. Org,
Rob Herring, Thierry Reding, ML dri-devel
Hi Lin,
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> From: huang lin <hl@rock-chips.com>
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> 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
> Changes in v4:
> - Modify the patch which suggest by Thierry
>
Thanks for splitting this up. I think there's another piece that fell
through the cracks.
I'm not deeply familiar with the driver, so just sharing some quick notes.
> struct innolux_panel {
> struct drm_panel base;
> struct mipi_dsi_device *link;
> + const struct panel_desc *desc;
>
> struct backlight_device *backlight;
> - struct regulator *supply;
> + struct regulator *vddi;
> + struct regulator *avdd;
> + struct regulator *avee;
These two seem are new addition, as opposed to a dummy refactor.
Are they optional, does one need them for the existing panel (separate
patch?) or only for the new one (squash with the new panel code)?
> struct gpio_desc *enable_gpio;
>
> bool prepared;
> @@ -77,9 +93,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;
Good call on dropping the early return here.
> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>
> - 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->desc = desc;
> + innolux->vddi = devm_regulator_get(dev, "power");
> + innolux->avdd = devm_regulator_get(dev, "avdd");
> + innolux->avee = devm_regulator_get(dev, "avee");
>
AFAICT devm_regulator_get returns a pointer which is unsuitable to be
passed into regulator_{enable,disable}.
Hence, the IS_ERR check should stay. If any of the regulators are
optional, you want to call regulator_{enable,disable} only as
applicable.
> @@ -318,5 +377,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>");
I don't think refactoring existing code classify as being the module author.
Then again, I could be wrong.
HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel
2018-03-14 9:12 ` [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel Lin Huang
@ 2018-03-14 12:16 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:16 UTC (permalink / raw)
To: Lin Huang
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
> the Innolux P079ZCA panel driver.
>
> Change-Id: I97923aa3735f707332681691b0231c9421b427d0
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - download panel initial code
>
> drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 +++++++++++++++++++++++++-
> 1 file changed, 187 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 2075a9d..883b279 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,6 +20,15 @@
>
> #include <video/mipi_display.h>
>
> +struct panel_init_cmd {
> + int len;
> + const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> struct panel_desc {
> const struct drm_display_mode *modes;
> unsigned int bpc;
> @@ -30,6 +39,7 @@ struct panel_desc {
>
> unsigned long flags;
> enum mipi_dsi_pixel_format format;
> + const struct panel_init_cmd *init_cmds;
> unsigned int lanes;
> };
>
> @@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> return err;
> }
>
> + /* p097pfg: t15 */
> + msleep(100);
> +
Based on the comment this is not needed for p079zca, yet still executed.
> gpiod_set_value_cansleep(innolux->enable_gpio, 0);
>
> - /* T8: 80ms - 1000ms */
> + /* p079zca: t8*/
> msleep(80);
>
And this is seem the opposite - zca only, yet executed on pfg.
One way to to avoid these and use the appropriate sleep throughout is
to store have the numbers in the respective panel_desc entries.
> regulator_disable(innolux->avee);
> @@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel *panel)
> if (err < 0)
> goto disable_avdd;
>
> - /* T2: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
> + usleep_range(20000, 21000);
>
> gpiod_set_value_cansleep(innolux->enable_gpio, 1);
>
> - /* T4: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t4, p097pfg: t5 */
> + usleep_range(20000, 21000);
> +
> + if (innolux->desc->init_cmds) {
> + const struct panel_init_cmd *cmds =
> + innolux->desc->init_cmds;
> + int i;
> +
> + for (i = 0; cmds[i].len != 0; i++) {
> + const struct panel_init_cmd *cmd = &cmds[i];
> +
> + err = mipi_dsi_generic_write(innolux->link, cmd->data,
> + cmd->len);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to write command %d\n", i);
> + goto poweroff;
> + }
> +
> + /*
> + * Included by random guessing, because without this
> + * (or at least, some delay), the panel sometimes
> + * didn't appear to pick up the command sequence.
> + */
This seems a bit hackish. Adding a reference to a bugreport/mailing
list discussion would be a nice move.
It'll save you/next person a lot of time searching for the specifics
of the problem.
> + err = mipi_dsi_dcs_nop(innolux->link);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS nop: %d\n", err);
> + goto poweroff;
> + }
> + }
> + }
>
> err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> if (err < 0) {
> @@ -213,6 +256,142 @@ static const struct panel_desc innolux_p079zca_panel_desc = {
> .lanes = 4,
> };
>
> +static const struct drm_display_mode innolux_p097pfg_mode = {
> + .clock = 229000,
> + .hdisplay = 1536,
> + .hsync_start = 1536 + 100,
> + .hsync_end = 1536 + 100 + 24,
> + .htotal = 1536 + 100 + 24 + 100,
> + .vdisplay = 2048,
> + .vsync_start = 2048 + 100,
> + .vsync_end = 2048 + 100 + 2,
> + .vtotal = 2048 + 100 + 2 + 18,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {
A lot of magic numbers :-(
Please reference the source of these - say XX spec. vY.
We could get a vY+1, which makes the nop workaround obsolete.
HTH
Emil
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel
@ 2018-03-14 12:16 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:16 UTC (permalink / raw)
To: Lin Huang
Cc: David Airlie, Brian Norris, Linux-Kernel@Vger. Kernel. Org,
Rob Herring, Thierry Reding, ML dri-devel
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> Support Innolux P097PFG 9.7" 1536x2048 TFT LCD panel, it reuse
> the Innolux P079ZCA panel driver.
>
> Change-Id: I97923aa3735f707332681691b0231c9421b427d0
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - download panel initial code
>
> drivers/gpu/drm/panel/panel-innolux-p079zca.c | 192 +++++++++++++++++++++++++-
> 1 file changed, 187 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> index 2075a9d..883b279 100644
> --- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> +++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
> @@ -20,6 +20,15 @@
>
> #include <video/mipi_display.h>
>
> +struct panel_init_cmd {
> + int len;
> + const char *data;
> +};
> +
> +#define _INIT_CMD(...) { \
> + .len = sizeof((char[]){__VA_ARGS__}), \
> + .data = (char[]){__VA_ARGS__} }
> +
> struct panel_desc {
> const struct drm_display_mode *modes;
> unsigned int bpc;
> @@ -30,6 +39,7 @@ struct panel_desc {
>
> unsigned long flags;
> enum mipi_dsi_pixel_format format;
> + const struct panel_init_cmd *init_cmds;
> unsigned int lanes;
> };
>
> @@ -88,9 +98,12 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
> return err;
> }
>
> + /* p097pfg: t15 */
> + msleep(100);
> +
Based on the comment this is not needed for p079zca, yet still executed.
> gpiod_set_value_cansleep(innolux->enable_gpio, 0);
>
> - /* T8: 80ms - 1000ms */
> + /* p079zca: t8*/
> msleep(80);
>
And this is seem the opposite - zca only, yet executed on pfg.
One way to to avoid these and use the appropriate sleep throughout is
to store have the numbers in the respective panel_desc entries.
> regulator_disable(innolux->avee);
> @@ -124,13 +137,43 @@ static int innolux_panel_prepare(struct drm_panel *panel)
> if (err < 0)
> goto disable_avdd;
>
> - /* T2: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t2 (20ms), p097pfg: t4 (15ms) */
> + usleep_range(20000, 21000);
>
> gpiod_set_value_cansleep(innolux->enable_gpio, 1);
>
> - /* T4: 15ms - 1000ms */
> - usleep_range(15000, 16000);
> + /* p079zca: t4, p097pfg: t5 */
> + usleep_range(20000, 21000);
> +
> + if (innolux->desc->init_cmds) {
> + const struct panel_init_cmd *cmds =
> + innolux->desc->init_cmds;
> + int i;
> +
> + for (i = 0; cmds[i].len != 0; i++) {
> + const struct panel_init_cmd *cmd = &cmds[i];
> +
> + err = mipi_dsi_generic_write(innolux->link, cmd->data,
> + cmd->len);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to write command %d\n", i);
> + goto poweroff;
> + }
> +
> + /*
> + * Included by random guessing, because without this
> + * (or at least, some delay), the panel sometimes
> + * didn't appear to pick up the command sequence.
> + */
This seems a bit hackish. Adding a reference to a bugreport/mailing
list discussion would be a nice move.
It'll save you/next person a lot of time searching for the specifics
of the problem.
> + err = mipi_dsi_dcs_nop(innolux->link);
> + if (err < 0) {
> + dev_err(panel->dev,
> + "failed to send DCS nop: %d\n", err);
> + goto poweroff;
> + }
> + }
> + }
>
> err = mipi_dsi_dcs_exit_sleep_mode(innolux->link);
> if (err < 0) {
> @@ -213,6 +256,142 @@ static const struct panel_desc innolux_p079zca_panel_desc = {
> .lanes = 4,
> };
>
> +static const struct drm_display_mode innolux_p097pfg_mode = {
> + .clock = 229000,
> + .hdisplay = 1536,
> + .hsync_start = 1536 + 100,
> + .hsync_end = 1536 + 100 + 24,
> + .htotal = 1536 + 100 + 24 + 100,
> + .vdisplay = 2048,
> + .vsync_start = 2048 + 100,
> + .vsync_end = 2048 + 100 + 2,
> + .vtotal = 2048 + 100 + 2 + 18,
> + .vrefresh = 60,
> +};
> +
> +static const struct panel_init_cmd innolux_p097pfg_init_cmds[] = {
A lot of magic numbers :-(
Please reference the source of these - say XX spec. vY.
We could get a vY+1, which makes the nop workaround obsolete.
HTH
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings
2018-03-14 9:12 ` [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings Lin Huang
@ 2018-03-14 12:18 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:18 UTC (permalink / raw)
To: Lin Huang
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> From: huang lin <hl@rock-chips.com>
>
> The Innolux P097PFG panel is 9.7" panel with 1536X2048
> resolution, it reuse P079ZCA panel driver, so improve
> p079ZCA dt-binding to support P097PFG.
>
> Change-Id: I8704914898fe53b734d31fbe646df8aa5fd8b30d
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
>
> .../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
>
Guess that answers my required/optional question earlier.
Currently the code assumes that all of these are required. And will
crash badly if any are missing.
-Emil
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings
@ 2018-03-14 12:18 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-14 12:18 UTC (permalink / raw)
To: Lin Huang
Cc: David Airlie, Brian Norris, Linux-Kernel@Vger. Kernel. Org,
Rob Herring, Thierry Reding, ML dri-devel
On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> From: huang lin <hl@rock-chips.com>
>
> The Innolux P097PFG panel is 9.7" panel with 1536X2048
> resolution, it reuse P079ZCA panel driver, so improve
> p079ZCA dt-binding to support P097PFG.
>
> Change-Id: I8704914898fe53b734d31fbe646df8aa5fd8b30d
> Signed-off-by: Lin Huang <hl@rock-chips.com>
> ---
> Changes in v2:
> - None
> Changes in v3:
> - None
> Changes in v4:
> - None
>
> .../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
>
Guess that answers my required/optional question earlier.
Currently the code assumes that all of these are required. And will
crash badly if any are missing.
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-14 12:02 ` Emil Velikov
(?)
@ 2018-03-15 2:35 ` hl
2018-03-19 13:09 ` Emil Velikov
-1 siblings, 1 reply; 20+ messages in thread
From: hl @ 2018-03-15 2:35 UTC (permalink / raw)
To: Emil Velikov
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi Emil,
On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
> Hi Lin,
>
> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>> From: huang lin <hl@rock-chips.com>
>>
>> Refactor Innolux P079ZCA panel driver, let it support
>> multi panel.
>>
>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>> 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
>> Changes in v4:
>> - Modify the patch which suggest by Thierry
>>
> Thanks for splitting this up. I think there's another piece that fell
> through the cracks.
> I'm not deeply familiar with the driver, so just sharing some quick notes.
>
>
>> struct innolux_panel {
>> struct drm_panel base;
>> struct mipi_dsi_device *link;
>> + const struct panel_desc *desc;
>>
>> struct backlight_device *backlight;
>> - struct regulator *supply;
>> + struct regulator *vddi;
>> + struct regulator *avdd;
>> + struct regulator *avee;
> These two seem are new addition, as opposed to a dummy refactor.
> Are they optional, does one need them for the existing panel (separate
> patch?) or only for the new one (squash with the new panel code)?
>
>
>> struct gpio_desc *enable_gpio;
>>
>> bool prepared;
>> @@ -77,9 +93,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;
> Good call on dropping the early return here.
>
>
>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>> - 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->desc = desc;
>> + innolux->vddi = devm_regulator_get(dev, "power");
>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>> + innolux->avee = devm_regulator_get(dev, "avee");
>>
> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
> passed into regulator_{enable,disable}.
> Hence, the IS_ERR check should stay. If any of the regulators are
> optional, you want to call regulator_{enable,disable} only as
> applicable.
devm_regulator_get() will use dummy_regulator if there not regulator pass to driver,
so it not affect regulator_{enable, disable}. These three regulator are optional,
the p079zca will use "power" and p097pgf will use "avdd" and "avee",
so i think it better not to check ERR here.
>
>> @@ -318,5 +377,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>");
> I don't think refactoring existing code classify as being the module author.
> Then again, I could be wrong.
>
> HTH
> Emil
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-15 2:35 ` hl
@ 2018-03-19 13:09 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-19 13:09 UTC (permalink / raw)
To: hl
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>
>> Hi Lin,
>>
>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>
>>> From: huang lin <hl@rock-chips.com>
>>>
>>> Refactor Innolux P079ZCA panel driver, let it support
>>> multi panel.
>>>
>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>> 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
>>> Changes in v4:
>>> - Modify the patch which suggest by Thierry
>>>
>> Thanks for splitting this up. I think there's another piece that fell
>> through the cracks.
>> I'm not deeply familiar with the driver, so just sharing some quick notes.
>>
>>
>>> struct innolux_panel {
>>> struct drm_panel base;
>>> struct mipi_dsi_device *link;
>>> + const struct panel_desc *desc;
>>>
>>> struct backlight_device *backlight;
>>> - struct regulator *supply;
>>> + struct regulator *vddi;
>>> + struct regulator *avdd;
>>> + struct regulator *avee;
>>
>> These two seem are new addition, as opposed to a dummy refactor.
>> Are they optional, does one need them for the existing panel (separate
>> patch?) or only for the new one (squash with the new panel code)?
>>
>>
>>> struct gpio_desc *enable_gpio;
>>>
>>> bool prepared;
>>> @@ -77,9 +93,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;
>>
>> Good call on dropping the early return here.
>>
>>
>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>> innolux_panel_funcs = {
>>> - 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->desc = desc;
>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>
>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>> passed into regulator_{enable,disable}.
>> Hence, the IS_ERR check should stay. If any of the regulators are
>> optional, you want to call regulator_{enable,disable} only as
>> applicable.
>
>
> devm_regulator_get() will use dummy_regulator if there not regulator pass to
> driver, so it not affect regulator_{enable, disable}.
One of us is getting confused here:
devm_regulator_get does not _use_ a regulator, it returns a pointer to
a regulator, right?
According to the 4.16-rc6 codebase - one error devm_regulator_get
returns a ERR_PTR [1].
With the pointer dereferenced in regulator_enable [2], without a
IS_ERR check, hence thing will go boom(?)
> These three regulator are
> optional,
> the p079zca will use "power" and ,
> so i think it better not to check ERR here.
>
What should happen if p079zca is missing "power" or p097pgf - "avdd" and "avee"?
Obviously the latter two should be introduced with the p097pgf support.
HTH
Emil
[1] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
[2] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-03-19 13:09 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-19 13:09 UTC (permalink / raw)
To: hl
Cc: David Airlie, Brian Norris, Linux-Kernel@Vger. Kernel. Org,
Rob Herring, Thierry Reding, ML dri-devel
On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>
>> Hi Lin,
>>
>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>
>>> From: huang lin <hl@rock-chips.com>
>>>
>>> Refactor Innolux P079ZCA panel driver, let it support
>>> multi panel.
>>>
>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>> 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
>>> Changes in v4:
>>> - Modify the patch which suggest by Thierry
>>>
>> Thanks for splitting this up. I think there's another piece that fell
>> through the cracks.
>> I'm not deeply familiar with the driver, so just sharing some quick notes.
>>
>>
>>> struct innolux_panel {
>>> struct drm_panel base;
>>> struct mipi_dsi_device *link;
>>> + const struct panel_desc *desc;
>>>
>>> struct backlight_device *backlight;
>>> - struct regulator *supply;
>>> + struct regulator *vddi;
>>> + struct regulator *avdd;
>>> + struct regulator *avee;
>>
>> These two seem are new addition, as opposed to a dummy refactor.
>> Are they optional, does one need them for the existing panel (separate
>> patch?) or only for the new one (squash with the new panel code)?
>>
>>
>>> struct gpio_desc *enable_gpio;
>>>
>>> bool prepared;
>>> @@ -77,9 +93,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;
>>
>> Good call on dropping the early return here.
>>
>>
>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>> innolux_panel_funcs = {
>>> - 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->desc = desc;
>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>
>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>> passed into regulator_{enable,disable}.
>> Hence, the IS_ERR check should stay. If any of the regulators are
>> optional, you want to call regulator_{enable,disable} only as
>> applicable.
>
>
> devm_regulator_get() will use dummy_regulator if there not regulator pass to
> driver, so it not affect regulator_{enable, disable}.
One of us is getting confused here:
devm_regulator_get does not _use_ a regulator, it returns a pointer to
a regulator, right?
According to the 4.16-rc6 codebase - one error devm_regulator_get
returns a ERR_PTR [1].
With the pointer dereferenced in regulator_enable [2], without a
IS_ERR check, hence thing will go boom(?)
> These three regulator are
> optional,
> the p079zca will use "power" and ,
> so i think it better not to check ERR here.
>
What should happen if p079zca is missing "power" or p097pgf - "avdd" and "avee"?
Obviously the latter two should be introduced with the p097pgf support.
HTH
Emil
[1] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
[2] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-19 13:09 ` Emil Velikov
(?)
@ 2018-03-20 6:24 ` hl
2018-03-20 10:20 ` Emil Velikov
-1 siblings, 1 reply; 20+ messages in thread
From: hl @ 2018-03-20 6:24 UTC (permalink / raw)
To: Emil Velikov
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi Emil,
On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
> On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
>> Hi Emil,
>>
>>
>>
>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>> Hi Lin,
>>>
>>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>> From: huang lin <hl@rock-chips.com>
>>>>
>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>> multi panel.
>>>>
>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>> 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
>>>> Changes in v4:
>>>> - Modify the patch which suggest by Thierry
>>>>
>>> Thanks for splitting this up. I think there's another piece that fell
>>> through the cracks.
>>> I'm not deeply familiar with the driver, so just sharing some quick notes.
>>>
>>>
>>>> struct innolux_panel {
>>>> struct drm_panel base;
>>>> struct mipi_dsi_device *link;
>>>> + const struct panel_desc *desc;
>>>>
>>>> struct backlight_device *backlight;
>>>> - struct regulator *supply;
>>>> + struct regulator *vddi;
>>>> + struct regulator *avdd;
>>>> + struct regulator *avee;
>>> These two seem are new addition, as opposed to a dummy refactor.
>>> Are they optional, does one need them for the existing panel (separate
>>> patch?) or only for the new one (squash with the new panel code)?
>>>
>>>
>>>> struct gpio_desc *enable_gpio;
>>>>
>>>> bool prepared;
>>>> @@ -77,9 +93,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;
>>> Good call on dropping the early return here.
>>>
>>>
>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>> innolux_panel_funcs = {
>>>> - 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->desc = desc;
>>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>>
>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>> passed into regulator_{enable,disable}.
>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>> optional, you want to call regulator_{enable,disable} only as
>>> applicable.
>>
>> devm_regulator_get() will use dummy_regulator if there not regulator pass to
>> driver, so it not affect regulator_{enable, disable}.
> One of us is getting confused here:
> devm_regulator_get does not _use_ a regulator, it returns a pointer to
> a regulator, right?
>
> According to the 4.16-rc6 codebase - one error
> returns a ERR_PTR [1].
devm_regulator_get() will not reurn a ERR_PTR, it will pass NORMAL_GET
mode to
_regulator_get() when you call devm_regulator_get(), and with following
code:
rdev = regulator_dev_lookup(dev, id);
if (IS_ERR(rdev)) {
.....
......
switch (get_type) {
case NORMAL_GET:
/*
* Assume that a regulator is physically present and
* enabled, even if it isn't hooked up, and just
* provide a dummy.
*/
dev_warn(dev,
"%s supply %s not found, using dummy regulator\n",
devname, id);
rdev = dummy_regulator_rdev;
get_device(&rdev->dev);
break;
...
...
}
....
regulator = create_regulator(rdev, dev, id);
...
it wil get a dummy_regulator for it.
> With the pointer dereferenced in regulator_enable [2], without a
> IS_ERR check, hence thing will go boom(?)
>
>> These three regulator are
>> optional,
>> the p079zca will use "power" and ,
>> so i think it better not to check ERR here.
>>
> What should happen if p079zca is missing "power" or p097pgf - "avdd" and "avee"?
> Obviously the latter two should be introduced with the p097pgf support.
i think it need dts to make sure configure the power node correct, if
missing
"power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
not work, but do not affcet other driver, the kernel do not crash(as i
explain before and i also test it).
>
> HTH
> Emil
>
> [1] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/devres.c#L27
> [2] https://elixir.bootlin.com/linux/v4.16-rc6/source/drivers/regulator/core.c#L2189
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-20 6:24 ` hl
@ 2018-03-20 10:20 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-20 10:20 UTC (permalink / raw)
To: hl
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
On 20 March 2018 at 06:24, hl <hl@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>
>> On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
>>>
>>> Hi Emil,
>>>
>>>
>>>
>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>>>
>>>> Hi Lin,
>>>>
>>>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>>>
>>>>> From: huang lin <hl@rock-chips.com>
>>>>>
>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>> multi panel.
>>>>>
>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>> 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
>>>>> Changes in v4:
>>>>> - Modify the patch which suggest by Thierry
>>>>>
>>>> Thanks for splitting this up. I think there's another piece that fell
>>>> through the cracks.
>>>> I'm not deeply familiar with the driver, so just sharing some quick
>>>> notes.
>>>>
>>>>
>>>>> struct innolux_panel {
>>>>> struct drm_panel base;
>>>>> struct mipi_dsi_device *link;
>>>>> + const struct panel_desc *desc;
>>>>>
>>>>> struct backlight_device *backlight;
>>>>> - struct regulator *supply;
>>>>> + struct regulator *vddi;
>>>>> + struct regulator *avdd;
>>>>> + struct regulator *avee;
>>>>
>>>> These two seem are new addition, as opposed to a dummy refactor.
>>>> Are they optional, does one need them for the existing panel (separate
>>>> patch?) or only for the new one (squash with the new panel code)?
>>>>
>>>>
>>>>> struct gpio_desc *enable_gpio;
>>>>>
>>>>> bool prepared;
>>>>> @@ -77,9 +93,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;
>>>>
>>>> Good call on dropping the early return here.
>>>>
>>>>
>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>>> innolux_panel_funcs = {
>>>>> - 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->desc = desc;
>>>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>>>
>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>>> passed into regulator_{enable,disable}.
>>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>>> optional, you want to call regulator_{enable,disable} only as
>>>> applicable.
>>>
>>>
>>> devm_regulator_get() will use dummy_regulator if there not regulator pass
>>> to
>>> driver, so it not affect regulator_{enable, disable}.
>>
>> One of us is getting confused here:
>> devm_regulator_get does not _use_ a regulator, it returns a pointer to
>> a regulator, right?
>>
>> According to the 4.16-rc6 codebase - one error
>> returns a ERR_PTR [1].
>
> devm_regulator_get() will not reurn a ERR_PTR, it will pass NORMAL_GET mode
> to
> _regulator_get() when you call devm_regulator_get(), and with following
> code:
>
Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);"
See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34
>> With the pointer dereferenced in regulator_enable [2], without a
>> IS_ERR check, hence thing will go boom(?)
>>
>>> These three regulator are
>>> optional,
>>> the p079zca will use "power" and ,
>>> so i think it better not to check ERR here.
>>>
>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and
>> "avee"?
>> Obviously the latter two should be introduced with the p097pgf support.
>
> i think it need dts to make sure configure the power node correct, if
> missing
> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
> not work, but do not affcet other driver, the kernel do not crash(as i
> explain before and i also test it).
>
If you know it won't work just don't continue? And yes, it will crash ;-)
Either way, if you don't like my feedback so be it.
HTH
Emil
P.S. As a non native English speaker to another - spell checker helps a lot ;-)
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-03-20 10:20 ` Emil Velikov
0 siblings, 0 replies; 20+ messages in thread
From: Emil Velikov @ 2018-03-20 10:20 UTC (permalink / raw)
To: hl
Cc: David Airlie, Brian Norris, Linux-Kernel@Vger. Kernel. Org,
Rob Herring, Thierry Reding, ML dri-devel
On 20 March 2018 at 06:24, hl <hl@rock-chips.com> wrote:
> Hi Emil,
>
>
>
> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>
>> On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
>>>
>>> Hi Emil,
>>>
>>>
>>>
>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>>>
>>>> Hi Lin,
>>>>
>>>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>>>
>>>>> From: huang lin <hl@rock-chips.com>
>>>>>
>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>> multi panel.
>>>>>
>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>> 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
>>>>> Changes in v4:
>>>>> - Modify the patch which suggest by Thierry
>>>>>
>>>> Thanks for splitting this up. I think there's another piece that fell
>>>> through the cracks.
>>>> I'm not deeply familiar with the driver, so just sharing some quick
>>>> notes.
>>>>
>>>>
>>>>> struct innolux_panel {
>>>>> struct drm_panel base;
>>>>> struct mipi_dsi_device *link;
>>>>> + const struct panel_desc *desc;
>>>>>
>>>>> struct backlight_device *backlight;
>>>>> - struct regulator *supply;
>>>>> + struct regulator *vddi;
>>>>> + struct regulator *avdd;
>>>>> + struct regulator *avee;
>>>>
>>>> These two seem are new addition, as opposed to a dummy refactor.
>>>> Are they optional, does one need them for the existing panel (separate
>>>> patch?) or only for the new one (squash with the new panel code)?
>>>>
>>>>
>>>>> struct gpio_desc *enable_gpio;
>>>>>
>>>>> bool prepared;
>>>>> @@ -77,9 +93,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;
>>>>
>>>> Good call on dropping the early return here.
>>>>
>>>>
>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>>> innolux_panel_funcs = {
>>>>> - 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->desc = desc;
>>>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>>>
>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>>> passed into regulator_{enable,disable}.
>>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>>> optional, you want to call regulator_{enable,disable} only as
>>>> applicable.
>>>
>>>
>>> devm_regulator_get() will use dummy_regulator if there not regulator pass
>>> to
>>> driver, so it not affect regulator_{enable, disable}.
>>
>> One of us is getting confused here:
>> devm_regulator_get does not _use_ a regulator, it returns a pointer to
>> a regulator, right?
>>
>> According to the 4.16-rc6 codebase - one error
>> returns a ERR_PTR [1].
>
> devm_regulator_get() will not reurn a ERR_PTR, it will pass NORMAL_GET mode
> to
> _regulator_get() when you call devm_regulator_get(), and with following
> code:
>
Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);"
See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34
>> With the pointer dereferenced in regulator_enable [2], without a
>> IS_ERR check, hence thing will go boom(?)
>>
>>> These three regulator are
>>> optional,
>>> the p079zca will use "power" and ,
>>> so i think it better not to check ERR here.
>>>
>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and
>> "avee"?
>> Obviously the latter two should be introduced with the p097pgf support.
>
> i think it need dts to make sure configure the power node correct, if
> missing
> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
> not work, but do not affcet other driver, the kernel do not crash(as i
> explain before and i also test it).
>
If you know it won't work just don't continue? And yes, it will crash ;-)
Either way, if you don't like my feedback so be it.
HTH
Emil
P.S. As a non native English speaker to another - spell checker helps a lot ;-)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-20 10:20 ` Emil Velikov
(?)
@ 2018-03-22 1:49 ` hl
-1 siblings, 0 replies; 20+ messages in thread
From: hl @ 2018-03-22 1:49 UTC (permalink / raw)
To: Emil Velikov
Cc: Thierry Reding, Brian Norris, Rob Herring, Sean Paul,
David Airlie, Linux-Kernel@Vger. Kernel. Org, ML dri-devel
Hi
On Tuesday, March 20, 2018 06:20 PM, Emil Velikov wrote:
> On 20 March 2018 at 06:24, hl <hl@rock-chips.com> wrote:
>> Hi Emil,
>>
>>
>>
>> On Monday, March 19, 2018 09:09 PM, Emil Velikov wrote:
>>> On 15 March 2018 at 02:35, hl <hl@rock-chips.com> wrote:
>>>> Hi Emil,
>>>>
>>>>
>>>>
>>>> On Wednesday, March 14, 2018 08:02 PM, Emil Velikov wrote:
>>>>> Hi Lin,
>>>>>
>>>>> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
>>>>>> From: huang lin <hl@rock-chips.com>
>>>>>>
>>>>>> Refactor Innolux P079ZCA panel driver, let it support
>>>>>> multi panel.
>>>>>>
>>>>>> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
>>>>>> 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
>>>>>> Changes in v4:
>>>>>> - Modify the patch which suggest by Thierry
>>>>>>
>>>>> Thanks for splitting this up. I think there's another piece that fell
>>>>> through the cracks.
>>>>> I'm not deeply familiar with the driver, so just sharing some quick
>>>>> notes.
>>>>>
>>>>>
>>>>>> struct innolux_panel {
>>>>>> struct drm_panel base;
>>>>>> struct mipi_dsi_device *link;
>>>>>> + const struct panel_desc *desc;
>>>>>>
>>>>>> struct backlight_device *backlight;
>>>>>> - struct regulator *supply;
>>>>>> + struct regulator *vddi;
>>>>>> + struct regulator *avdd;
>>>>>> + struct regulator *avee;
>>>>> These two seem are new addition, as opposed to a dummy refactor.
>>>>> Are they optional, does one need them for the existing panel (separate
>>>>> patch?) or only for the new one (squash with the new panel code)?
>>>>>
>>>>>
>>>>>> struct gpio_desc *enable_gpio;
>>>>>>
>>>>>> bool prepared;
>>>>>> @@ -77,9 +93,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;
>>>>> Good call on dropping the early return here.
>>>>>
>>>>>
>>>>>> @@ -207,19 +248,28 @@ static const struct drm_panel_funcs
>>>>>> innolux_panel_funcs = {
>>>>>> - 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->desc = desc;
>>>>>> + innolux->vddi = devm_regulator_get(dev, "power");
>>>>>> + innolux->avdd = devm_regulator_get(dev, "avdd");
>>>>>> + innolux->avee = devm_regulator_get(dev, "avee");
>>>>>>
>>>>> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
>>>>> passed into regulator_{enable,disable}.
>>>>> Hence, the IS_ERR check should stay. If any of the regulators are
>>>>> optional, you want to call regulator_{enable,disable} only as
>>>>> applicable.
>>>>
>>>> devm_regulator_get() will use dummy_regulator if there not regulator pass
>>>> to
>>>> driver, so it not affect regulator_{enable, disable}.
>>> One of us is getting confused here:
>>> devm_regulator_get does not _use_ a regulator, it returns a pointer to
>>> a regulator, right?
>>>
>>> According to the 4.16-rc6 codebase - one error
>>> returns a ERR_PTR [1].
>> devm_regulator_get() will not reurn a ERR_PTR, it will pass NORMAL_GET mode
>> to
>> _regulator_get() when you call devm_regulator_get(), and with following
>> code:
>>
> Just before the _regulator_get() call we have "return ERR_PTR(-ENOMEM);"
> See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/regulator/devres.c?h=v4.16-rc6#n34
Okay, i got what you concern now, yes, you are right, i will keep IS_ERR
checking here.
>
>>> With the pointer dereferenced in regulator_enable [2], without a
>>> IS_ERR check, hence thing will go boom(?)
>>>
>>>> These three regulator are
>>>> optional,
>>>> the p079zca will use "power" and ,
>>>> so i think it better not to check ERR here.
>>>>
>>> What should happen if p079zca is missing "power" or p097pgf - "avdd" and
>>> "avee"?
>>> Obviously the latter two should be introduced with the p097pgf support.
>> i think it need dts to make sure configure the power node correct, if
>> missing
>> "power" node fo p079zca or "avdd" "avee" node for p097pgf, the panel can
>> not work, but do not affcet other driver, the kernel do not crash(as i
>> explain before and i also test it).
>>
> If you know it won't work just don't continue? And yes, it will crash ;-)
> Either way, if you don't like my feedback so be it.
>
> HTH
> Emil
> P.S. As a non native English speaker to another - spell checker helps a lot ;-)
>
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-14 9:12 [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Lin Huang
@ 2018-04-26 14:10 ` Thierry Reding
2018-03-14 9:12 ` [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings Lin Huang
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: Thierry Reding @ 2018-04-26 14:10 UTC (permalink / raw)
To: Lin Huang
Cc: emil.l.velikov, briannorris, robh+dt, seanpaul, airlied,
linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 386 bytes --]
On Wed, Mar 14, 2018 at 05:12:14PM +0800, Lin Huang wrote:
> From: huang lin <hl@rock-chips.com>
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
When you send out a revised set of these patches, please drop the
Change-Id: line from the commit message. It is useless for upstream.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-04-26 14:10 ` Thierry Reding
0 siblings, 0 replies; 20+ messages in thread
From: Thierry Reding @ 2018-04-26 14:10 UTC (permalink / raw)
To: Lin Huang
Cc: airlied, briannorris, emil.l.velikov, linux-kernel, dri-devel, robh+dt
[-- Attachment #1.1: Type: text/plain, Size: 386 bytes --]
On Wed, Mar 14, 2018 at 05:12:14PM +0800, Lin Huang wrote:
> From: huang lin <hl@rock-chips.com>
>
> Refactor Innolux P079ZCA panel driver, let it support
> multi panel.
>
> Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
When you send out a revised set of these patches, please drop the
Change-Id: line from the commit message. It is useless for upstream.
Thierry
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 160 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
2018-03-14 12:02 ` Emil Velikov
@ 2018-06-14 12:49 ` Heiko Stuebner
-1 siblings, 0 replies; 20+ messages in thread
From: Heiko Stuebner @ 2018-06-14 12:49 UTC (permalink / raw)
To: dri-devel
Cc: Emil Velikov, Lin Huang, David Airlie, Brian Norris,
linux-kernel, Rob Herring, Thierry Reding
Am Mittwoch, 14. März 2018, 13:02:13 CEST schrieb Emil Velikov:
> Hi Lin,
>
> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> > From: huang lin <hl@rock-chips.com>
> >
> > Refactor Innolux P079ZCA panel driver, let it support
> > multi panel.
> >
> > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
[...]
> > @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>
> >
> > - 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->desc = desc;
> > + innolux->vddi = devm_regulator_get(dev, "power");
> > + innolux->avdd = devm_regulator_get(dev, "avdd");
> > + innolux->avee = devm_regulator_get(dev, "avee");
> >
> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
> passed into regulator_{enable,disable}.
> Hence, the IS_ERR check should stay. If any of the regulators are
> optional, you want to call regulator_{enable,disable} only as
> applicable.
using the regulator_bulk APIs should help to make this far easier,
as you can just define the per-panel supplies in in the panel_desc
and then get + enable the correct ones per bound panel.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver
@ 2018-06-14 12:49 ` Heiko Stuebner
0 siblings, 0 replies; 20+ messages in thread
From: Heiko Stuebner @ 2018-06-14 12:49 UTC (permalink / raw)
To: dri-devel
Cc: Lin Huang, David Airlie, Brian Norris, Emil Velikov,
linux-kernel, Rob Herring, Thierry Reding
Am Mittwoch, 14. März 2018, 13:02:13 CEST schrieb Emil Velikov:
> Hi Lin,
>
> On 14 March 2018 at 09:12, Lin Huang <hl@rock-chips.com> wrote:
> > From: huang lin <hl@rock-chips.com>
> >
> > Refactor Innolux P079ZCA panel driver, let it support
> > multi panel.
> >
> > Change-Id: If89be5e56dba8cb498e2d50c1bbeb0e8016123a2
> > Signed-off-by: Lin Huang <hl@rock-chips.com>
[...]
> > @@ -207,19 +248,28 @@ static const struct drm_panel_funcs innolux_panel_funcs = {
>
> >
> > - 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->desc = desc;
> > + innolux->vddi = devm_regulator_get(dev, "power");
> > + innolux->avdd = devm_regulator_get(dev, "avdd");
> > + innolux->avee = devm_regulator_get(dev, "avee");
> >
> AFAICT devm_regulator_get returns a pointer which is unsuitable to be
> passed into regulator_{enable,disable}.
> Hence, the IS_ERR check should stay. If any of the regulators are
> optional, you want to call regulator_{enable,disable} only as
> applicable.
using the regulator_bulk APIs should help to make this far easier,
as you can just define the per-panel supplies in in the panel_desc
and then get + enable the correct ones per bound panel.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-06-14 12:49 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-14 9:12 [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Lin Huang
2018-03-14 9:12 ` [PATCH v4 2/3] drm/panel: support Innolux P097PFG panel Lin Huang
2018-03-14 12:16 ` Emil Velikov
2018-03-14 12:16 ` Emil Velikov
2018-03-14 9:12 ` [PATCH v4 3/3] dt-bindings: Add INNOLUX P097PFG panel bindings Lin Huang
2018-03-14 12:18 ` Emil Velikov
2018-03-14 12:18 ` Emil Velikov
2018-03-14 12:02 ` [PATCH v4 1/3] drm/panel: refactor INNOLUX P079ZCA panel driver Emil Velikov
2018-03-14 12:02 ` Emil Velikov
2018-03-15 2:35 ` hl
2018-03-19 13:09 ` Emil Velikov
2018-03-19 13:09 ` Emil Velikov
2018-03-20 6:24 ` hl
2018-03-20 10:20 ` Emil Velikov
2018-03-20 10:20 ` Emil Velikov
2018-03-22 1:49 ` hl
2018-06-14 12:49 ` Heiko Stuebner
2018-06-14 12:49 ` Heiko Stuebner
2018-04-26 14:10 ` Thierry Reding
2018-04-26 14:10 ` Thierry Reding
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.