[PATCH v3 0/10]

[PATCH v3 0/10]

[Yakir Yang]

RK3399 and RK3288 shared the same eDP IP controller, only some light
difference with VOP configure and GRF configure.

Also same misc fix to analogix_dp driver:
- Hotplug invalid which report by Dan Carpenter
- Make panel detect to an optional action
- correct the register bit define error in ANALOGIX_DP_PLL_REG_1


Changes in v3:
- Correct the misspell of "marcos" in commit message (Dominik, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
- Add reviewed flag from Stéphane.
    [https://chromium-review.googlesource.com/#/c/346312/16]
- Add tested flag from Javier.
- Write a kerneldoc-style comment explaining the chips data fields (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
- Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
- Add acked flag from Mark.
- Add reviewed flag from Tomasz.
    [https://chromium-review.googlesource.com/#/c/346315/15]
- Add tested flag from Javier
- Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
  reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
    [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
- Add tested flag from Javier
- Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
- Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
- Add the acked flag from Mark.
- Add the acked flag from Mark.
- Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
- Hook the connector's color_formats in .get_modes directly. (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346317/15]
- Add the acked flag from Mark.
- Add the reviewed flag from Tomasz.
    [https://chromium-review.googlesource.com/#/c/346853/12]
- Add the acked flag from Mark.
- Add reviewed flag from Stéphane.
    [https://chromium-review.googlesource.com/#/c/346319/15]
- Add tested flag from Javier

Changes in v2:
- new patch in v2
- rebase with drm-next, fix some conflicts
- new patch in v2

Yakir Yang (10):
  drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP
  drm/rockchip: analogix_dp: split the lcdc select setting into device
    data
  drm/bridge: analogix_dp: correct the register bit define error in
    ANALOGIX_DP_PLL_REG_1
  drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit
    setting
  drm/rockchip: analogix_dp: add rk3399 eDP support
  drm/rockchip: analogix_dp: make panel detect to an optional action
  drm/bridge: analogix_dp: passing the connector as an argument in
    .get_modes()
  drm/rockchip: analogix_dp: correct the connector display color format
    and bpc
  drm/rockchip: analogix_dp: update the comments about why need to
    hardcode VOP output mode
  drm/bridge: analogix_dp: fix no drm hpd event when panel plug in

 .../bindings/display/bridge/analogix_dp.txt        |   1 +
 .../display/rockchip/analogix_dp-rockchip.txt      |   3 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |   6 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   8 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  12 +-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  |   5 +-
 drivers/gpu/drm/exynos/exynos_dp.c                 |   4 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 158 ++++++++++++++-------
 include/drm/bridge/analogix_dp.h                   |   9 +-
 9 files changed, 141 insertions(+), 65 deletions(-)

-- 
1.9.1

[PATCH v3 01/10] drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP

[Yakir Yang]

Rename RK3288_DP macros to ROCKCHIP_DP, prepare to add eDP
support for more Rockchip chips.


Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
Changes in v3:
- Correct the misspell of "marcos" in commit message (Dominik, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
- Add reviewed flag from Stéphane.
    [https://chromium-review.googlesource.com/#/c/346312/16]
- Add tested flag from Javier.

Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 4 ++--
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 6 +++---
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 2 +-
 include/drm/bridge/analogix_dp.h                   | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7699597..4a1b3b8 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1207,9 +1207,9 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
 	struct video_info *video_info = &dp->video_info;
 
 	switch (dp->plat_data->dev_type) {
-	case RK3288_DP:
+	case ROCKCHIP_DP:
 		/*
-		 * Like Rk3288 DisplayPort TRM indicate that "Main link
+		 * Like Rockchip DisplayPort TRM indicate that "Main link
 		 * containing 4 physical lanes of 2.7/1.62 Gbps/lane".
 		 */
 		video_info->max_link_rate = 0x0A;
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 49205ef..931a76c 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -74,7 +74,7 @@ void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
 	reg = SEL_24M | TX_DVDD_BIT_1_0625V;
 	writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
 
-	if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP)) {
+	if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP)) {
 		writel(REF_CLK_24M, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
 		writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
 		writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
@@ -244,7 +244,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
 	u32 reg;
 	u32 phy_pd_addr = ANALOGIX_DP_PHY_PD;
 
-	if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP))
+	if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP))
 		phy_pd_addr = ANALOGIX_DP_PD;
 
 	switch (block) {
@@ -448,7 +448,7 @@ void analogix_dp_init_aux(struct analogix_dp_device *dp)
 	analogix_dp_reset_aux(dp);
 
 	/* Disable AUX transaction H/W retry */
-	if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP))
+	if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP))
 		reg = AUX_BIT_PERIOD_EXPECTED_DELAY(0) |
 		      AUX_HW_RETRY_COUNT_SEL(3) |
 		      AUX_HW_RETRY_INTERVAL_600_MICROSECONDS;
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 7f6a55c..2bc8a7e 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -270,7 +270,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 
 	dp->plat_data.encoder = &dp->encoder;
 
-	dp->plat_data.dev_type = RK3288_DP;
+	dp->plat_data.dev_type = ROCKCHIP_DP;
 	dp->plat_data.power_on = rockchip_dp_poweron;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 25afb31..9e5d013 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -15,7 +15,7 @@
 
 enum analogix_dp_devtype {
 	EXYNOS_DP,
-	RK3288_DP,
+	ROCKCHIP_DP,
 };
 
 struct analogix_dp_plat_data {
-- 
1.9.1

[PATCH v3 02/10] drm/rockchip: analogix_dp: split the lcdc select setting into device data

[Yakir Yang]

eDP controller need to declare which vop provide the video source,
and it's defined in GRF registers.

But different chips have different GRF register address, so we need to
create a device data to declare the GRF messages for each chips.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>
---
Changes in v3:
- Write a kerneldoc-style comment explaining the chips data fields (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
- Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
- Add acked flag from Mark.

Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 39 +++++++++++++++++++------
 1 file changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 2bc8a7e..3855f46 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -14,6 +14,7 @@
 
 #include <linux/component.h>
 #include <linux/mfd/syscon.h>
+#include <linux/of_device.h>
 #include <linux/of_graph.h>
 #include <linux/regmap.h>
 #include <linux/reset.h>
@@ -35,11 +36,17 @@
 
 #define to_dp(nm)	container_of(nm, struct rockchip_dp_device, nm)
 
-/* dp grf register offset */
-#define GRF_SOC_CON6                            0x025c
-#define GRF_EDP_LCD_SEL_MASK                    BIT(5)
-#define GRF_EDP_SEL_VOP_LIT                     BIT(5)
-#define GRF_EDP_SEL_VOP_BIG                     0
+/**
+ * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
+ * @lcdsel_grf_reg: grf register offset of lcdc select
+ * @lcdsel_big: reg value of selecting vop big for eDP
+ * @lcdsel_lit: reg value of selecting vop little for eDP
+ */
+struct rockchip_dp_chip_data {
+	u32	lcdsel_grf_reg;
+	u32	lcdsel_big;
+	u32	lcdsel_lit;
+};
 
 struct rockchip_dp_device {
 	struct drm_device        *drm_dev;
@@ -51,6 +58,8 @@ struct rockchip_dp_device {
 	struct regmap            *grf;
 	struct reset_control     *rst;
 
+	const struct rockchip_dp_chip_data *data;
+
 	struct analogix_dp_plat_data plat_data;
 };
 
@@ -119,13 +128,13 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 		return;
 
 	if (ret)
-		val = GRF_EDP_SEL_VOP_LIT | (GRF_EDP_LCD_SEL_MASK << 16);
+		val = dp->data->lcdsel_lit;
 	else
-		val = GRF_EDP_SEL_VOP_BIG | (GRF_EDP_LCD_SEL_MASK << 16);
+		val = dp->data->lcdsel_big;
 
 	dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
 
-	ret = regmap_write(dp->grf, GRF_SOC_CON6, val);
+	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
 	if (ret != 0) {
 		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
 		return;
@@ -246,6 +255,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 			    void *data)
 {
 	struct rockchip_dp_device *dp = dev_get_drvdata(dev);
+	const struct rockchip_dp_chip_data *dp_data;
 	struct drm_device *drm_dev = data;
 	int ret;
 
@@ -256,10 +266,15 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	 */
 	dev_set_drvdata(dev, NULL);
 
+	dp_data = of_device_get_match_data(dev);
+	if (!dp_data)
+		return -ENODEV;
+
 	ret = rockchip_dp_init(dp);
 	if (ret < 0)
 		return ret;
 
+	dp->data = dp_data;
 	dp->drm_dev = drm_dev;
 
 	ret = rockchip_dp_drm_create_encoder(dp);
@@ -365,8 +380,14 @@ static const struct dev_pm_ops rockchip_dp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
 };
 
+static const struct rockchip_dp_chip_data rk3288_dp = {
+	.lcdsel_grf_reg = 0x025c,
+	.lcdsel_big = 0 | BIT(21),
+	.lcdsel_lit = BIT(5) | BIT(21),
+};
+
 static const struct of_device_id rockchip_dp_dt_ids[] = {
-	{.compatible = "rockchip,rk3288-dp",},
+	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
 	{}
 };
 MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
-- 
1.9.1

[PATCH v3 03/10] drm/bridge: analogix_dp: correct the register bit define error in ANALOGIX_DP_PLL_REG_1

[Yakir Yang]

There're an register define error in ANALOGIX_DP_PLL_REG_1 which introduced
by commit bcec20fd5ad6 ("drm: bridge: analogix/dp: add some rk3288 special
registers setting").

The PHY PLL input clock source is selected by ANALOGIX_DP_PLL_REG_1
BIT 0, not BIT 1.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
Changes in v3:
- Add reviewed flag from Tomasz.
    [https://chromium-review.googlesource.com/#/c/346315/15]
- Add tested flag from Javier

Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
index 337912b..88d56ad 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
@@ -163,8 +163,8 @@
 #define HSYNC_POLARITY_CFG			(0x1 << 0)
 
 /* ANALOGIX_DP_PLL_REG_1 */
-#define REF_CLK_24M				(0x1 << 1)
-#define REF_CLK_27M				(0x0 << 1)
+#define REF_CLK_24M				(0x1 << 0)
+#define REF_CLK_27M				(0x0 << 0)
 
 /* ANALOGIX_DP_LANE_MAP */
 #define LANE3_MAP_LOGIC_LANE_0			(0x0 << 6)
-- 
1.9.1

[PATCH v3 04/10] drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit setting

[Yakir Yang]

As vendor document indicate, when REF_CLK bit set 0, then DP
phy's REF_CLK should switch to 24M source clock.

But due to IC PHY layout mistaken, some chips need to flip this
bit(like RK3288), and unfortunately they didn't indicate in the
DP version register. That's why we have to make this little hack.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
Changes in v3:
- Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
  reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
    [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
- Add tested flag from Javier

Changes in v2:
- new patch in v2

 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 6 +++++-
 drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 1 +
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c   | 3 +++
 include/drm/bridge/analogix_dp.h                  | 5 +++++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
index 931a76c..97ced6b 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
@@ -75,7 +75,11 @@ void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
 	writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
 
 	if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP)) {
-		writel(REF_CLK_24M, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
+		reg = REF_CLK_24M;
+		if (dp->plat_data->subdev_type == RK3288_DP)
+			reg ^= REF_CLK_MASK;
+
+		writel(reg, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
 		writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
 		writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
 		writel(0x58, dp->reg_base + ANALOGIX_DP_PLL_REG_4);
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
index 88d56ad..cdcc6c5 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
@@ -165,6 +165,7 @@
 /* ANALOGIX_DP_PLL_REG_1 */
 #define REF_CLK_24M				(0x1 << 0)
 #define REF_CLK_27M				(0x0 << 0)
+#define REF_CLK_MASK				(0x1 << 0)
 
 /* ANALOGIX_DP_LANE_MAP */
 #define LANE3_MAP_LOGIC_LANE_0			(0x0 << 6)
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 3855f46..315ebba 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -46,6 +46,7 @@ struct rockchip_dp_chip_data {
 	u32	lcdsel_grf_reg;
 	u32	lcdsel_big;
 	u32	lcdsel_lit;
+	u32	chip_type;
 };
 
 struct rockchip_dp_device {
@@ -286,6 +287,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.encoder = &dp->encoder;
 
 	dp->plat_data.dev_type = ROCKCHIP_DP;
+	dp->plat_data.subdev_type = dp_data->chip_type;
 	dp->plat_data.power_on = rockchip_dp_poweron;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
 
@@ -384,6 +386,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
 	.lcdsel_grf_reg = 0x025c,
 	.lcdsel_big = 0 | BIT(21),
 	.lcdsel_lit = BIT(5) | BIT(21),
+	.chip_type = RK3288_DP,
 };
 
 static const struct of_device_id rockchip_dp_dt_ids[] = {
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 9e5d013..06c0250 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -18,8 +18,13 @@ enum analogix_dp_devtype {
 	ROCKCHIP_DP,
 };
 
+enum analogix_dp_sub_devtype {
+	RK3288_DP,
+};
+
 struct analogix_dp_plat_data {
 	enum analogix_dp_devtype dev_type;
+	enum analogix_dp_sub_devtype subdev_type;
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
-- 
1.9.1

[PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Yakir Yang]

RK3399 and RK3288 shared the same eDP IP controller, only some light
difference with VOP configure and GRF configure.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>
---
Changes in v3:
- Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
- Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
- Add the acked flag from Mark.

Changes in v2:
- rebase with drm-next, fix some conflicts

 .../bindings/display/bridge/analogix_dp.txt        |  1 +
 .../display/rockchip/analogix_dp-rockchip.txt      |  3 +-
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 33 +++++++++++++++++++++-
 include/drm/bridge/analogix_dp.h                   |  1 +
 4 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
index 4f2ba8c..4a0f4f7 100644
--- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
+++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
@@ -5,6 +5,7 @@ Required properties for dp-controller:
 		platform specific such as:
 		 * "samsung,exynos5-dp"
 		 * "rockchip,rk3288-dp"
+		 * "rockchip,rk3399-edp"
 	-reg:
 		physical base address of the controller and length
 		of memory mapped region.
diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
index e832ff9..726c945 100644
--- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
@@ -2,7 +2,8 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
 ================================
 
 Required properties:
-- compatible: "rockchip,rk3288-edp";
+- compatible: "rockchip,rk3288-edp",
+	      "rockchip,rk3399-edp";
 
 - reg: physical base address of the controller and length
 
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 315ebba..bcd9ecc 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -153,6 +153,8 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
 				      struct drm_connector_state *conn_state)
 {
 	struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
+	struct rockchip_dp_device *dp = to_dp(encoder);
+	int ret;
 
 	/*
 	 * FIXME(Yakir): driver should configure the CRTC output video
@@ -167,7 +169,28 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
 	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
 	 * RGB666 input video mode, then screen would works prefect.
 	 */
-	s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
+
+	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
+	if (ret < 0)
+		return 0;
+
+	switch (dp->data->chip_type) {
+	case RK3399_EDP:
+		/*
+		 * For RK3399, VOP Lit must code the out mode to RGB888,
+		 * VOP Big must code the out mode to RGB10.
+		 */
+		if (ret)
+			s->output_mode = ROCKCHIP_OUT_MODE_P888;
+		else
+			s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
+		break;
+
+	default:
+		s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
+		break;
+	}
+
 	s->output_type = DRM_MODE_CONNECTOR_eDP;
 
 	return 0;
@@ -382,6 +405,13 @@ static const struct dev_pm_ops rockchip_dp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
 };
 
+static const struct rockchip_dp_chip_data rk3399_edp = {
+	.lcdsel_grf_reg = 0x6250,
+	.lcdsel_big = 0 | BIT(21),
+	.lcdsel_lit = BIT(5) | BIT(21),
+	.chip_type = RK3399_EDP,
+};
+
 static const struct rockchip_dp_chip_data rk3288_dp = {
 	.lcdsel_grf_reg = 0x025c,
 	.lcdsel_big = 0 | BIT(21),
@@ -391,6 +421,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
 
 static const struct of_device_id rockchip_dp_dt_ids[] = {
 	{.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
+	{.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
 	{}
 };
 MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 06c0250..82b8135 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -20,6 +20,7 @@ enum analogix_dp_devtype {
 
 enum analogix_dp_sub_devtype {
 	RK3288_DP,
+	RK3399_EDP,
 };
 
 struct analogix_dp_plat_data {
-- 
1.9.1

[PATCH v3 06/10] drm/rockchip: analogix_dp: make panel detect to an optional action

[Yakir Yang]

Some boards don't need to declare a panel device node, like the
display interface is DP monitors, so it's necessary to make the
panel detect to an optional action.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>
---
Changes in v3:
- Add the acked flag from Mark.

Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 48 ++++++++++++-------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index bcd9ecc..da2e844 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -332,38 +332,34 @@ static int rockchip_dp_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
 	struct rockchip_dp_device *dp;
-	struct drm_panel *panel;
 
 	port = of_graph_get_port_by_id(dev->of_node, 1);
-	if (!port) {
-		dev_err(dev, "can't find output port\n");
-		return -EINVAL;
-	}
-
-	endpoint = of_get_child_by_name(port, "endpoint");
-	of_node_put(port);
-	if (!endpoint) {
-		dev_err(dev, "no output endpoint found\n");
-		return -EINVAL;
-	}
-
-	panel_node = of_graph_get_remote_port_parent(endpoint);
-	of_node_put(endpoint);
-	if (!panel_node) {
-		dev_err(dev, "no output node found\n");
-		return -EINVAL;
-	}
-
-	panel = of_drm_find_panel(panel_node);
-	if (!panel) {
-		DRM_ERROR("failed to find panel\n");
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(dev, "no output endpoint found\n");
+			return -EINVAL;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(dev, "no output node found\n");
+			return -EINVAL;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		if (!panel) {
+			DRM_ERROR("failed to find panel\n");
+			of_node_put(panel_node);
+			return -EPROBE_DEFER;
+		}
 		of_node_put(panel_node);
-		return -EPROBE_DEFER;
 	}
 
-	of_node_put(panel_node);
-
 	dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp)
 		return -ENOMEM;
-- 
1.9.1

[PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Yakir Yang]

It's better to pass the connector to platform driver in .get_modes()
callback, just like what the .get_modes() helper function designed.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v3:
- Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]

Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
 drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
 include/drm/bridge/analogix_dp.h                   | 3 ++-
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 4a1b3b8..1a890fa 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
 		num_modes += drm_panel_get_modes(dp->plat_data->panel);
 
 	if (dp->plat_data->get_modes)
-		num_modes += dp->plat_data->get_modes(dp->plat_data);
+		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
 
 	return num_modes;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
index 468498e..8a555ed 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
 	return exynos_dp_crtc_clock_enable(plat_data, false);
 }
 
-static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
+static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
+			       struct drm_connector *connector)
 {
 	struct exynos_dp_device *dp = to_dp(plat_data);
-	struct drm_connector *connector = &dp->connector;
 	struct drm_display_mode *mode;
 	int num_modes = 0;
 
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index 82b8135..181db09 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
 	int (*power_off)(struct analogix_dp_plat_data *);
 	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
 		      struct drm_connector *);
-	int (*get_modes)(struct analogix_dp_plat_data *);
+	int (*get_modes)(struct analogix_dp_plat_data *,
+			 struct drm_connector *);
 };
 
 int analogix_dp_resume(struct device *dev);
-- 
1.9.1

[PATCH v3 08/10] drm/rockchip: analogix_dp: correct the connector display color format and bpc

[Yakir Yang]

Rockchip VOP couldn't output YUV video format for eDP controller, so
when driver detect connector support YUV video format, we need to hack
it down to RGB888.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>
---
Changes in v3:
- Hook the connector's color_formats in .get_modes directly. (Tomasz, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/346317/15]
- Add the acked flag from Mark.

Changes in v2: None

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index da2e844..95a6f60 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -102,6 +102,22 @@ static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
 	return 0;
 }
 
+static int rockchip_dp_get_modes(struct analogix_dp_plat_data *plat_data,
+				 struct drm_connector *connector)
+{
+	struct drm_display_info *di = &connector->display_info;
+
+	if (di->color_formats & DRM_COLOR_FORMAT_YCRCB444 ||
+	    di->color_formats & DRM_COLOR_FORMAT_YCRCB422) {
+		di->color_formats &= ~(DRM_COLOR_FORMAT_YCRCB422 |
+				       DRM_COLOR_FORMAT_YCRCB444);
+		di->color_formats |= DRM_COLOR_FORMAT_RGB444;
+		di->bpc = 8;
+	}
+
+	return 0;
+}
+
 static bool
 rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
 				   const struct drm_display_mode *mode,
@@ -313,6 +329,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
 	dp->plat_data.subdev_type = dp_data->chip_type;
 	dp->plat_data.power_on = rockchip_dp_poweron;
 	dp->plat_data.power_off = rockchip_dp_powerdown;
+	dp->plat_data.get_modes = rockchip_dp_get_modes;
 
 	return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
 }
-- 
1.9.1

[PATCH v3 09/10] drm/rockchip: analogix_dp: update the comments about why need to hardcode VOP output mode

[Yakir Yang]

The hardware IC designed that VOP must output the RGB10 video format to
eDP contoller, and if eDP panel only support RGB8, then eDP contoller
should cut down the video data, not via VOP contoller, that's why we need
to hardcode the VOP output mode to RGA10 here.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Acked-by: Mark Yao <mark.yao@rock-chips.com>
Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
---
Changes in v3:
- Add the reviewed flag from Tomasz.
    [https://chromium-review.googlesource.com/#/c/346853/12]
- Add the acked flag from Mark.

Changes in v2:
- new patch in v2

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 95a6f60..2ceb3f9 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -173,17 +173,11 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
 	int ret;
 
 	/*
-	 * FIXME(Yakir): driver should configure the CRTC output video
-	 * mode with the display information which indicated the monitor
-	 * support colorimetry.
-	 *
-	 * But don't know why the CRTC driver seems could only output the
-	 * RGBaaa rightly. For example, if connect the "innolux,n116bge"
-	 * eDP screen, EDID would indicated that screen only accepted the
-	 * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
-	 * screen would show a blue picture (RGB888 show a green picture).
-	 * But if I configure CTRC to RGBaaa, and eDP driver still keep
-	 * RGB666 input video mode, then screen would works prefect.
+	 * The hardware IC designed that VOP must output the RGB10 video
+	 * format to eDP contoller, and if eDP panel only support RGB8,
+	 * then eDP contoller should cut down the video data, not via VOP
+	 * contoller, that's why we need to hardcode the VOP output mode
+	 * to RGA10 here.
 	 */
 
 	ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
-- 
1.9.1

[PATCH v3 10/10] drm/bridge: analogix_dp: fix no drm hpd event when panel plug in

[Yakir Yang]

The enum value of DP_IRQ_TYPE_HP_CABLE_IN is zero, but driver only
send drm hp event when the irq_type and the enum value is true.

if (irq_type & DP_IRQ_TYPE_HP_CABLE_IN || ...)
	drm_helper_hpd_irq_event(dp->drm_dev);

So there would no drm hpd event when cable plug in, to fix that
just need to assign all hotplug enum with no-zero values.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
---
Changes in v3:
- Add reviewed flag from Stéphane.
    [https://chromium-review.googlesource.com/#/c/346319/15]
- Add tested flag from Javier

Changes in v2: None

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index f09275d..b456380 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -127,10 +127,10 @@ enum analog_power_block {
 };
 
 enum dp_irq_type {
-	DP_IRQ_TYPE_HP_CABLE_IN,
-	DP_IRQ_TYPE_HP_CABLE_OUT,
-	DP_IRQ_TYPE_HP_CHANGE,
-	DP_IRQ_TYPE_UNKNOWN,
+	DP_IRQ_TYPE_HP_CABLE_IN  = BIT(0),
+	DP_IRQ_TYPE_HP_CABLE_OUT = BIT(1),
+	DP_IRQ_TYPE_HP_CHANGE    = BIT(2),
+	DP_IRQ_TYPE_UNKNOWN      = BIT(3),
 };
 
 struct video_info {
-- 
1.9.1

Re: [PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Doug Anderson]

Yakir,

On Tue, Jun 14, 2016 at 4:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> RK3399 and RK3288 shared the same eDP IP controller, only some light
> difference with VOP configure and GRF configure.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v3:
> - Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
> - Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
> - Add the acked flag from Mark.
>
> Changes in v2:
> - rebase with drm-next, fix some conflicts
>
>  .../bindings/display/bridge/analogix_dp.txt        |  1 +
>  .../display/rockchip/analogix_dp-rockchip.txt      |  3 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 33 +++++++++++++++++++++-
>  include/drm/bridge/analogix_dp.h                   |  1 +
>  4 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> index 4f2ba8c..4a0f4f7 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> @@ -5,6 +5,7 @@ Required properties for dp-controller:
>                 platform specific such as:
>                  * "samsung,exynos5-dp"
>                  * "rockchip,rk3288-dp"
> +                * "rockchip,rk3399-edp"
>         -reg:
>                 physical base address of the controller and length
>                 of memory mapped region.
> diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> index e832ff9..726c945 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> @@ -2,7 +2,8 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
>  ================================
>
>  Required properties:
> -- compatible: "rockchip,rk3288-edp";
> +- compatible: "rockchip,rk3288-edp",
> +             "rockchip,rk3399-edp";

As commented by Tomasz on gerrit, there is a pre-existing typo here.
Specifically "rockchip,rk3288-edp" should be "rockchip,rk3288-dp".

The typo is pre-existing so I'm not sure you would need to spin this
series to fix it, but if you were spinning it anyway it wouldn't hurt
to fix.  Could also just send out a followon patch to fix this after
the series lands...

-Doug

Re: [PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Daniel Vetter]

On Tue, Jun 14, 2016 at 07:46:29PM +0800, Yakir Yang wrote:
> It's better to pass the connector to platform driver in .get_modes()
> callback, just like what the .get_modes() helper function designed.
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v3:
> - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
> 
> Changes in v2: None

Needs cc: Archit Tajena as drm_bridge maintainer (or at least inofficial
one), plus ack imo.
-Daniel

> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
>  include/drm/bridge/analogix_dp.h                   | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 4a1b3b8..1a890fa 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
>  		num_modes += drm_panel_get_modes(dp->plat_data->panel);
>  
>  	if (dp->plat_data->get_modes)
> -		num_modes += dp->plat_data->get_modes(dp->plat_data);
> +		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>  
>  	return num_modes;
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 468498e..8a555ed 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
>  	return exynos_dp_crtc_clock_enable(plat_data, false);
>  }
>  
> -static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
> +static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
> +			       struct drm_connector *connector)
>  {
>  	struct exynos_dp_device *dp = to_dp(plat_data);
> -	struct drm_connector *connector = &dp->connector;
>  	struct drm_display_mode *mode;
>  	int num_modes = 0;
>  
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 82b8135..181db09 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
>  	int (*power_off)(struct analogix_dp_plat_data *);
>  	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>  		      struct drm_connector *);
> -	int (*get_modes)(struct analogix_dp_plat_data *);
> +	int (*get_modes)(struct analogix_dp_plat_data *,
> +			 struct drm_connector *);
>  };
>  
>  int analogix_dp_resume(struct device *dev);
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Daniel Vetter]

On Tue, Jun 14, 2016 at 07:46:29PM +0800, Yakir Yang wrote:
> It's better to pass the connector to platform driver in .get_modes()
> callback, just like what the .get_modes() helper function designed.
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>

Also please drop the analogix prefix from the subject, this is for all
drm_bridge drivers.
-Daniel

> ---
> Changes in v3:
> - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
> 
> Changes in v2: None
> 
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
>  include/drm/bridge/analogix_dp.h                   | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 4a1b3b8..1a890fa 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
>  		num_modes += drm_panel_get_modes(dp->plat_data->panel);
>  
>  	if (dp->plat_data->get_modes)
> -		num_modes += dp->plat_data->get_modes(dp->plat_data);
> +		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>  
>  	return num_modes;
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 468498e..8a555ed 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
>  	return exynos_dp_crtc_clock_enable(plat_data, false);
>  }
>  
> -static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
> +static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
> +			       struct drm_connector *connector)
>  {
>  	struct exynos_dp_device *dp = to_dp(plat_data);
> -	struct drm_connector *connector = &dp->connector;
>  	struct drm_display_mode *mode;
>  	int num_modes = 0;
>  
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 82b8135..181db09 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
>  	int (*power_off)(struct analogix_dp_plat_data *);
>  	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>  		      struct drm_connector *);
> -	int (*get_modes)(struct analogix_dp_plat_data *);
> +	int (*get_modes)(struct analogix_dp_plat_data *,
> +			 struct drm_connector *);
>  };
>  
>  int analogix_dp_resume(struct device *dev);
> -- 
> 1.9.1
> 
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Daniel Vetter]

On Tue, Jun 14, 2016 at 06:26:56PM +0200, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 07:46:29PM +0800, Yakir Yang wrote:
> > It's better to pass the connector to platform driver in .get_modes()
> > callback, just like what the .get_modes() helper function designed.
> > 
> > Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> 
> Also please drop the analogix prefix from the subject, this is for all
> drm_bridge drivers.

Ok, strike all my comments, I was blind and didn't realize that this was a
private interface for only rockchip/exynos.
-Daniel

> -Daniel
> 
> > ---
> > Changes in v3:
> > - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
> >     [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
> > 
> > Changes in v2: None
> > 
> >  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
> >  drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
> >  include/drm/bridge/analogix_dp.h                   | 3 ++-
> >  3 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > index 4a1b3b8..1a890fa 100644
> > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> > @@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
> >  		num_modes += drm_panel_get_modes(dp->plat_data->panel);
> >  
> >  	if (dp->plat_data->get_modes)
> > -		num_modes += dp->plat_data->get_modes(dp->plat_data);
> > +		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
> >  
> >  	return num_modes;
> >  }
> > diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> > index 468498e..8a555ed 100644
> > --- a/drivers/gpu/drm/exynos/exynos_dp.c
> > +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> > @@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
> >  	return exynos_dp_crtc_clock_enable(plat_data, false);
> >  }
> >  
> > -static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
> > +static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
> > +			       struct drm_connector *connector)
> >  {
> >  	struct exynos_dp_device *dp = to_dp(plat_data);
> > -	struct drm_connector *connector = &dp->connector;
> >  	struct drm_display_mode *mode;
> >  	int num_modes = 0;
> >  
> > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> > index 82b8135..181db09 100644
> > --- a/include/drm/bridge/analogix_dp.h
> > +++ b/include/drm/bridge/analogix_dp.h
> > @@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
> >  	int (*power_off)(struct analogix_dp_plat_data *);
> >  	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
> >  		      struct drm_connector *);
> > -	int (*get_modes)(struct analogix_dp_plat_data *);
> > +	int (*get_modes)(struct analogix_dp_plat_data *,
> > +			 struct drm_connector *);
> >  };
> >  
> >  int analogix_dp_resume(struct device *dev);
> > -- 
> > 1.9.1
> > 
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Yakir Yang]

Doug,

On 06/14/2016 11:24 PM, Doug Anderson wrote:
> Yakir,
>
> On Tue, Jun 14, 2016 at 4:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> RK3399 and RK3288 shared the same eDP IP controller, only some light
>> difference with VOP configure and GRF configure.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v3:
>> - Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
>> - Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
>> - Add the acked flag from Mark.
>>
>> Changes in v2:
>> - rebase with drm-next, fix some conflicts
>>
>>   .../bindings/display/bridge/analogix_dp.txt        |  1 +
>>   .../display/rockchip/analogix_dp-rockchip.txt      |  3 +-
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 33 +++++++++++++++++++++-
>>   include/drm/bridge/analogix_dp.h                   |  1 +
>>   4 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> index 4f2ba8c..4a0f4f7 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> @@ -5,6 +5,7 @@ Required properties for dp-controller:
>>                  platform specific such as:
>>                   * "samsung,exynos5-dp"
>>                   * "rockchip,rk3288-dp"
>> +                * "rockchip,rk3399-edp"
>>          -reg:
>>                  physical base address of the controller and length
>>                  of memory mapped region.
>> diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> index e832ff9..726c945 100644
>> --- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> +++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> @@ -2,7 +2,8 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
>>   ================================
>>
>>   Required properties:
>> -- compatible: "rockchip,rk3288-edp";
>> +- compatible: "rockchip,rk3288-edp",
>> +             "rockchip,rk3399-edp";
> As commented by Tomasz on gerrit, there is a pre-existing typo here.
> Specifically "rockchip,rk3288-edp" should be "rockchip,rk3288-dp".
>
> The typo is pre-existing so I'm not sure you would need to spin this
> series to fix it, but if you were spinning it anyway it wouldn't hurt
> to fix.  Could also just send out a followon patch to fix this after
> the series lands...

My bad, forget to fix that :(
it would be better to merge that fix into this series, would send a 
follow patch soon.

Thanks for reminding,
- Yakir

> -Doug
>
>
>

Re: [PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Yakir Yang]

On 06/15/2016 12:28 AM, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 06:26:56PM +0200, Daniel Vetter wrote:
>> On Tue, Jun 14, 2016 at 07:46:29PM +0800, Yakir Yang wrote:
>>> It's better to pass the connector to platform driver in .get_modes()
>>> callback, just like what the .get_modes() helper function designed.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Also please drop the analogix prefix from the subject, this is for all
>> drm_bridge drivers.
> Ok, strike all my comments, I was blind and didn't realize that this was a
> private interface for only rockchip/exynos.

:)

> -Daniel
>
>> -Daniel
>>
>>> ---
>>> Changes in v3:
>>> - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
>>>      [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
>>>
>>> Changes in v2: None
>>>
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
>>>   drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
>>>   include/drm/bridge/analogix_dp.h                   | 3 ++-
>>>   3 files changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 4a1b3b8..1a890fa 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
>>>   		num_modes += drm_panel_get_modes(dp->plat_data->panel);
>>>   
>>>   	if (dp->plat_data->get_modes)
>>> -		num_modes += dp->plat_data->get_modes(dp->plat_data);
>>> +		num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>>>   
>>>   	return num_modes;
>>>   }
>>> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
>>> index 468498e..8a555ed 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_dp.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
>>> @@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
>>>   	return exynos_dp_crtc_clock_enable(plat_data, false);
>>>   }
>>>   
>>> -static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
>>> +static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
>>> +			       struct drm_connector *connector)
>>>   {
>>>   	struct exynos_dp_device *dp = to_dp(plat_data);
>>> -	struct drm_connector *connector = &dp->connector;
>>>   	struct drm_display_mode *mode;
>>>   	int num_modes = 0;
>>>   
>>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>>> index 82b8135..181db09 100644
>>> --- a/include/drm/bridge/analogix_dp.h
>>> +++ b/include/drm/bridge/analogix_dp.h
>>> @@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
>>>   	int (*power_off)(struct analogix_dp_plat_data *);
>>>   	int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>>>   		      struct drm_connector *);
>>> -	int (*get_modes)(struct analogix_dp_plat_data *);
>>> +	int (*get_modes)(struct analogix_dp_plat_data *,
>>> +			 struct drm_connector *);
>>>   };
>>>   
>>>   int analogix_dp_resume(struct device *dev);
>>> -- 
>>> 1.9.1
>>>
>>>
>> -- 
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch

Re: [PATCH v3 0/10]

[Archit Taneja]

On 6/14/2016 5:15 PM, Yakir Yang wrote:
> RK3399 and RK3288 shared the same eDP IP controller, only some light
> difference with VOP configure and GRF configure.
>
> Also same misc fix to analogix_dp driver:
> - Hotplug invalid which report by Dan Carpenter
> - Make panel detect to an optional action
> - correct the register bit define error in ANALOGIX_DP_PLL_REG_1
>
>
> Changes in v3:
> - Correct the misspell of "marcos" in commit message (Dominik, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
> - Add reviewed flag from Stéphane.
>      [https://chromium-review.googlesource.com/#/c/346312/16]
> - Add tested flag from Javier.
> - Write a kerneldoc-style comment explaining the chips data fields (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
> - Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
> - Add acked flag from Mark.
> - Add reviewed flag from Tomasz.
>      [https://chromium-review.googlesource.com/#/c/346315/15]
> - Add tested flag from Javier
> - Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
>    reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
>      [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
> - Add tested flag from Javier
> - Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
> - Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
> - Add the acked flag from Mark.
> - Add the acked flag from Mark.
> - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
> - Hook the connector's color_formats in .get_modes directly. (Tomasz, reviewed at Google Gerrit)
>      [https://chromium-review.googlesource.com/#/c/346317/15]
> - Add the acked flag from Mark.
> - Add the reviewed flag from Tomasz.
>      [https://chromium-review.googlesource.com/#/c/346853/12]
> - Add the acked flag from Mark.
> - Add reviewed flag from Stéphane.
>      [https://chromium-review.googlesource.com/#/c/346319/15]
> - Add tested flag from Javier
>
> Changes in v2:
> - new patch in v2
> - rebase with drm-next, fix some conflicts
> - new patch in v2
>
> Yakir Yang (10):
>    drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP
>    drm/rockchip: analogix_dp: split the lcdc select setting into device
>      data
>    drm/bridge: analogix_dp: correct the register bit define error in
>      ANALOGIX_DP_PLL_REG_1
>    drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit
>      setting
>    drm/rockchip: analogix_dp: add rk3399 eDP support
>    drm/rockchip: analogix_dp: make panel detect to an optional action
>    drm/bridge: analogix_dp: passing the connector as an argument in
>      .get_modes()
>    drm/rockchip: analogix_dp: correct the connector display color format
>      and bpc
>    drm/rockchip: analogix_dp: update the comments about why need to
>      hardcode VOP output mode
>    drm/bridge: analogix_dp: fix no drm hpd event when panel plug in

Is the plan to take all the bridge+rockchip stuff via the rockchip pull
request?

Thanks,
Archit

>
>   .../bindings/display/bridge/analogix_dp.txt        |   1 +
>   .../display/rockchip/analogix_dp-rockchip.txt      |   3 +-
>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |   6 +-
>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   8 +-
>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  12 +-
>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  |   5 +-
>   drivers/gpu/drm/exynos/exynos_dp.c                 |   4 +-
>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 158 ++++++++++++++-------
>   include/drm/bridge/analogix_dp.h                   |   9 +-
>   9 files changed, 141 insertions(+), 65 deletions(-)
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 0/10]

[Yakir Yang]

Archit,

On 06/21/2016 09:46 PM, Archit Taneja wrote:
>
>
> On 6/14/2016 5:15 PM, Yakir Yang wrote:
>> RK3399 and RK3288 shared the same eDP IP controller, only some light
>> difference with VOP configure and GRF configure.
>>
>> Also same misc fix to analogix_dp driver:
>> - Hotplug invalid which report by Dan Carpenter
>> - Make panel detect to an optional action
>> - correct the register bit define error in ANALOGIX_DP_PLL_REG_1
>>
>>
>> Changes in v3:
>> - Correct the misspell of "marcos" in commit message (Dominik, 
>> reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
>> - Add reviewed flag from Stéphane.
>>      [https://chromium-review.googlesource.com/#/c/346312/16]
>> - Add tested flag from Javier.
>> - Write a kerneldoc-style comment explaining the chips data fields 
>> (Tomasz, reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
>> - Drop the '.lcdcsel_mask' number in chips data field (Tomasz, 
>> reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
>> - Add acked flag from Mark.
>> - Add reviewed flag from Tomasz.
>>      [https://chromium-review.googlesource.com/#/c/346315/15]
>> - Add tested flag from Javier
>> - Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
>>    reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
>> [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
>> - Add tested flag from Javier
>> - Give the "rk3399-edp" a separate line for clarity in document 
>> (Tomasz, reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
>> - Move 'output_type' setting before the return statement (Tomasz, 
>> reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
>> - Add the acked flag from Mark.
>> - Add the acked flag from Mark.
>> - Avoid to change any internal driver state in .mode_valid interface. 
>> (Tomasz, reviewed at Google Gerrit)
>> [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
>> - Hook the connector's color_formats in .get_modes directly. (Tomasz, 
>> reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346317/15]
>> - Add the acked flag from Mark.
>> - Add the reviewed flag from Tomasz.
>>      [https://chromium-review.googlesource.com/#/c/346853/12]
>> - Add the acked flag from Mark.
>> - Add reviewed flag from Stéphane.
>>      [https://chromium-review.googlesource.com/#/c/346319/15]
>> - Add tested flag from Javier
>>
>> Changes in v2:
>> - new patch in v2
>> - rebase with drm-next, fix some conflicts
>> - new patch in v2
>>
>> Yakir Yang (10):
>>    drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP
>>    drm/rockchip: analogix_dp: split the lcdc select setting into device
>>      data
>>    drm/bridge: analogix_dp: correct the register bit define error in
>>      ANALOGIX_DP_PLL_REG_1
>>    drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit
>>      setting
>>    drm/rockchip: analogix_dp: add rk3399 eDP support
>>    drm/rockchip: analogix_dp: make panel detect to an optional action
>>    drm/bridge: analogix_dp: passing the connector as an argument in
>>      .get_modes()
>>    drm/rockchip: analogix_dp: correct the connector display color format
>>      and bpc
>>    drm/rockchip: analogix_dp: update the comments about why need to
>>      hardcode VOP output mode
>>    drm/bridge: analogix_dp: fix no drm hpd event when panel plug in
>
> Is the plan to take all the bridge+rockchip stuff via the rockchip pull
> request?
>

Yep, most of those patch need to rely on others, so it's better to 
collect all of them into one pull request ;)

Thanks,
- Yakir

> Thanks,
> Archit
>
>>
>>   .../bindings/display/bridge/analogix_dp.txt        |   1 +
>>   .../display/rockchip/analogix_dp-rockchip.txt      |   3 +-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |   6 +-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   8 +-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  12 +-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  |   5 +-
>>   drivers/gpu/drm/exynos/exynos_dp.c                 |   4 +-
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 158 
>> ++++++++++++++-------
>>   include/drm/bridge/analogix_dp.h                   |   9 +-
>>   9 files changed, 141 insertions(+), 65 deletions(-)
>>
>

[PATCH v3.1 1/2] drm/rockchip: analogix_dp: introduce the pclk for grf

[Yakir Yang]

For RK3399's GRF module, if we want to operate the graphic related grf
registers, we need to enable the pclk_vio_grf which supply power for VIO
GRF IOs, so it's better to introduce an optional grf clock in driver.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Hi all,

This is an external patch for analogix_dp misc cleanup thread [0]
[0]: https://patchwork.kernel.org/patch/9175613/

BR,
- Yakir

 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 787dc51..6a83f1a 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -56,6 +56,7 @@ struct rockchip_dp_device {
 	struct drm_display_mode  mode;
 
 	struct clk               *pclk;
+	struct clk               *grfclk;
 	struct regmap            *grf;
 	struct reset_control     *rst;
 
@@ -151,11 +152,20 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 
 	dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
 
+	if (dp->grfclk) {
+		ret = clk_prepare_enable(dp->grfclk);
+		if (ret < 0) {
+			dev_err(dp->dev, "failed to enable grfclk %d\n", ret);
+			return;
+		}
+	}
+
 	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
-	if (ret != 0) {
+	if (ret != 0)
 		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
-		return;
-	}
+
+	if (dp->grfclk)
+		clk_disable_unprepare(dp->grfclk);
 }
 
 static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
@@ -235,6 +245,10 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
 		return PTR_ERR(dp->grf);
 	}
 
+	dp->grfclk = devm_clk_get(dev, "grf");
+	if (IS_ERR(dp->grfclk))
+		dp->grfclk = NULL;
+
 	dp->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(dp->pclk)) {
 		dev_err(dev, "failed to get pclk property\n");
-- 
1.9.1

[PATCH v3.1 2/2] dt-bindings: analogix_dp: rockchip: correct the wrong compatible name

[Yakir Yang]

The document about rockchip platform make a mistaken in available
compatible name of "rk3288-edp", we should correct it to "rk3288-dp"
which correspond to the compatible name in driver.

This mistaken was introduced in commit be91c36247089 ("dt-bindings:
add document for rockchip variant of analogix_dp").

Reported-by: Tomasz Figa <tfiga@chromium.com>
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Hi all,

This is an external patch for analogix_dp misc cleanup thread [0]
[0]: https://patchwork.kernel.org/patch/9175613/

BR,
- Yakir

 .../devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt       | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
index 726c945..827cc36 100644
--- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
@@ -2,7 +2,7 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
 ================================
 
 Required properties:
-- compatible: "rockchip,rk3288-edp",
+- compatible: "rockchip,rk3288-dp",
 	      "rockchip,rk3399-edp";
 
 - reg: physical base address of the controller and length
-- 
1.9.1

Re: [PATCH v3 0/10]

[Archit Taneja]

On 6/22/2016 7:54 AM, Yakir Yang wrote:
> Archit,
>
> On 06/21/2016 09:46 PM, Archit Taneja wrote:
>>
>>
>> On 6/14/2016 5:15 PM, Yakir Yang wrote:
>>> RK3399 and RK3288 shared the same eDP IP controller, only some light
>>> difference with VOP configure and GRF configure.
>>>
>>> Also same misc fix to analogix_dp driver:
>>> - Hotplug invalid which report by Dan Carpenter
>>> - Make panel detect to an optional action
>>> - correct the register bit define error in ANALOGIX_DP_PLL_REG_1
>>>
>>>
>>> Changes in v3:
>>> - Correct the misspell of "marcos" in commit message (Dominik,
>>> reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
>>> - Add reviewed flag from Stéphane.
>>>      [https://chromium-review.googlesource.com/#/c/346312/16]
>>> - Add tested flag from Javier.
>>> - Write a kerneldoc-style comment explaining the chips data fields
>>> (Tomasz, reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
>>>
>>> - Drop the '.lcdcsel_mask' number in chips data field (Tomasz,
>>> reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
>>>
>>> - Add acked flag from Mark.
>>> - Add reviewed flag from Tomasz.
>>>      [https://chromium-review.googlesource.com/#/c/346315/15]
>>> - Add tested flag from Javier
>>> - Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
>>>    reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
>>> [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
>>>
>>> - Add tested flag from Javier
>>> - Give the "rk3399-edp" a separate line for clarity in document
>>> (Tomasz, reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
>>>
>>> - Move 'output_type' setting before the return statement (Tomasz,
>>> reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
>>>
>>> - Add the acked flag from Mark.
>>> - Add the acked flag from Mark.
>>> - Avoid to change any internal driver state in .mode_valid interface.
>>> (Tomasz, reviewed at Google Gerrit)
>>> [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
>>>
>>> - Hook the connector's color_formats in .get_modes directly. (Tomasz,
>>> reviewed at Google Gerrit)
>>>      [https://chromium-review.googlesource.com/#/c/346317/15]
>>> - Add the acked flag from Mark.
>>> - Add the reviewed flag from Tomasz.
>>>      [https://chromium-review.googlesource.com/#/c/346853/12]
>>> - Add the acked flag from Mark.
>>> - Add reviewed flag from Stéphane.
>>>      [https://chromium-review.googlesource.com/#/c/346319/15]
>>> - Add tested flag from Javier
>>>
>>> Changes in v2:
>>> - new patch in v2
>>> - rebase with drm-next, fix some conflicts
>>> - new patch in v2
>>>
>>> Yakir Yang (10):
>>>    drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP
>>>    drm/rockchip: analogix_dp: split the lcdc select setting into device
>>>      data
>>>    drm/bridge: analogix_dp: correct the register bit define error in
>>>      ANALOGIX_DP_PLL_REG_1
>>>    drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit
>>>      setting
>>>    drm/rockchip: analogix_dp: add rk3399 eDP support
>>>    drm/rockchip: analogix_dp: make panel detect to an optional action
>>>    drm/bridge: analogix_dp: passing the connector as an argument in
>>>      .get_modes()
>>>    drm/rockchip: analogix_dp: correct the connector display color format
>>>      and bpc
>>>    drm/rockchip: analogix_dp: update the comments about why need to
>>>      hardcode VOP output mode
>>>    drm/bridge: analogix_dp: fix no drm hpd event when panel plug in
>>
>> Is the plan to take all the bridge+rockchip stuff via the rockchip pull
>> request?
>>
>
> Yep, most of those patch need to rely on others, so it's better to
> collect all of them into one pull request ;)

Cool. Sounds good.

Archit

>
> Thanks,
> - Yakir
>
>> Thanks,
>> Archit
>>
>>>
>>>   .../bindings/display/bridge/analogix_dp.txt        |   1 +
>>>   .../display/rockchip/analogix_dp-rockchip.txt      |   3 +-
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.c |   6 +-
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h |   8 +-
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  |  12 +-
>>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h  |   5 +-
>>>   drivers/gpu/drm/exynos/exynos_dp.c                 |   4 +-
>>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 158
>>> ++++++++++++++-------
>>>   include/drm/bridge/analogix_dp.h                   |   9 +-
>>>   9 files changed, 141 insertions(+), 65 deletions(-)
>>>
>>
>
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/2] drm/rockchip: analogix_dp: introduce the pclk for grf

[Yakir Yang]

For RK3399's GRF module, if we want to operate the graphic related grf
registers, we need to enable the pclk_vio_grf which supply power for VIO
GRF IOs, so it's better to introduce an optional grf clock in driver.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Hi all,

This is an external patch for analogix_dp misc cleanup thread [0]
[0]: https://patchwork.kernel.org/patch/9175613/

BR,
- Yakir

Changes in v4:
- Check the the error code properly, 'EPROBE_DEFER' should be returned,
  'ENOENT' should assign a NULL point to grfclk, other errors should be
  regarded as failed. (Tomasz, Doug, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/351821/20/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@249]
- Add the document about optional 'grf' clock (Tomasz, Doug, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/351821/]

Changes in v3:
- Add this patch in v3

 .../display/rockchip/analogix_dp-rockchip.txt      |  6 ++++++
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 23 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
index 726c945..0b39256 100644
--- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
@@ -28,6 +28,12 @@ Required properties:
     Port 0: contained 2 endpoints, connecting to the output of vop.
     Port 1: contained 1 endpoint, connecting to the input of panel.
 
+Optional property for different chips:
+- clocks: from common clock binding: handle to grf_vio clock.
+
+- clock-names: from common clock binding:
+	       Required elements: "grf"
+
 For the below properties, please refer to Analogix DP binding document:
  * Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
 - phys (required)
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 787dc51..864ef92 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -56,6 +56,7 @@ struct rockchip_dp_device {
 	struct drm_display_mode  mode;
 
 	struct clk               *pclk;
+	struct clk               *grfclk;
 	struct regmap            *grf;
 	struct reset_control     *rst;
 
@@ -151,11 +152,17 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 
 	dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
 
-	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
-	if (ret != 0) {
-		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
+	ret = clk_prepare_enable(dp->grfclk);
+	if (ret < 0) {
+		dev_err(dp->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
+
+	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
+	if (ret != 0)
+		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
+
+	clk_disable_unprepare(dp->grfclk);
 }
 
 static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
@@ -235,6 +242,16 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
 		return PTR_ERR(dp->grf);
 	}
 
+	dp->grfclk = devm_clk_get(dev, "grf");
+	if (PTR_ERR(dp->grfclk) == -ENOENT) {
+		dp->grfclk = NULL;
+	} else if (PTR_ERR(dp->grfclk) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(dp->grfclk)) {
+		dev_err(dev, "failed to get grf clock\n");
+		return PTR_ERR(dp->cfgclk);
+	}
+
 	dp->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(dp->pclk)) {
 		dev_err(dev, "failed to get pclk property\n");
-- 
1.9.1

[PATCH v4 2/2] dt-bindings: analogix_dp: rockchip: correct the wrong compatible name

[Yakir Yang]

The document about rockchip platform make a mistaken in available
compatible name of "rk3288-edp", we should correct it to "rk3288-dp"
which correspond to the compatible name in driver.

This mistaken was introduced in commit be91c36247089 ("dt-bindings:
add document for rockchip variant of analogix_dp").

Reported-by: Tomasz Figa <tfiga@chromium.com>
Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Hi all,

This is an external patch for analogix_dp misc cleanup thread [0]
[0]: https://patchwork.kernel.org/patch/9175613/

BR,
- Yakir

Changes in v4: None
Changes in v3:
- Add this patch in v3

 .../devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt       | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
index 0b39256..01cced1 100644
--- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
@@ -2,7 +2,7 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
 ================================
 
 Required properties:
-- compatible: "rockchip,rk3288-edp",
+- compatible: "rockchip,rk3288-dp",
 	      "rockchip,rk3399-edp";
 
 - reg: physical base address of the controller and length
-- 
1.9.1

[PATCH v4.1 1/2] drm/rockchip: analogix_dp: introduce the pclk for grf

[Yakir Yang]

For RK3399's GRF module, if we want to operate the graphic related grf
registers, we need to enable the pclk_vio_grf which supply power for VIO
GRF IOs, so it's better to introduce an optional grf clock in driver.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Hi all,

This is an external patch for analogix_dp misc cleanup thread [0]
[0]: https://patchwork.kernel.org/patch/9175613/

BR,
- Yakir

Changes in v4.1:
- Fix compiled error, sorry.
  "dp->cgfclk"  -->  'dp->grfclk'

Changes in v4:
- Check the the error code properly, 'EPROBE_DEFER' should be returned,
  'ENOENT' should assign a NULL point to grfclk, other errors should be
  regarded as failed. (Tomasz, Doug, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/351821/20/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@249]
- Add the document about optional 'grf' clock (Tomasz, Doug, reviewed at Google Gerrit)
    [https://chromium-review.googlesource.com/#/c/351821/]

Changes in v3:
- Add this patch in v3

 .../display/rockchip/analogix_dp-rockchip.txt      |  6 ++++++
 drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 23 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
index 726c945..0b39256 100644
--- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
+++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
@@ -28,6 +28,12 @@ Required properties:
     Port 0: contained 2 endpoints, connecting to the output of vop.
     Port 1: contained 1 endpoint, connecting to the input of panel.
 
+Optional property for different chips:
+- clocks: from common clock binding: handle to grf_vio clock.
+
+- clock-names: from common clock binding:
+	       Required elements: "grf"
+
 For the below properties, please refer to Analogix DP binding document:
  * Documentation/devicetree/bindings/drm/bridge/analogix_dp.txt
 - phys (required)
diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index 787dc51..864ef92 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -56,6 +56,7 @@ struct rockchip_dp_device {
 	struct drm_display_mode  mode;
 
 	struct clk               *pclk;
+	struct clk               *grfclk;
 	struct regmap            *grf;
 	struct reset_control     *rst;
 
@@ -151,11 +152,17 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
 
 	dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
 
-	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
-	if (ret != 0) {
-		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
+	ret = clk_prepare_enable(dp->grfclk);
+	if (ret < 0) {
+		dev_err(dp->dev, "failed to enable grfclk %d\n", ret);
 		return;
 	}
+
+	ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
+	if (ret != 0)
+		dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
+
+	clk_disable_unprepare(dp->grfclk);
 }
 
 static void rockchip_dp_drm_encoder_nop(struct drm_encoder *encoder)
@@ -235,6 +242,16 @@ static int rockchip_dp_init(struct rockchip_dp_device *dp)
 		return PTR_ERR(dp->grf);
 	}
 
+	dp->grfclk = devm_clk_get(dev, "grf");
+	if (PTR_ERR(dp->grfclk) == -ENOENT) {
+		dp->grfclk = NULL;
+	} else if (PTR_ERR(dp->grfclk) == -EPROBE_DEFER) {
+		return -EPROBE_DEFER;
+	} else if (IS_ERR(dp->grfclk)) {
+		dev_err(dev, "failed to get grf clock\n");
+		return PTR_ERR(dp->grfclk);
+	}
+
 	dp->pclk = devm_clk_get(dev, "pclk");
 	if (IS_ERR(dp->pclk)) {
 		dev_err(dev, "failed to get pclk property\n");
-- 
1.9.1

Re: [PATCH v4.1 1/2] drm/rockchip: analogix_dp: introduce the pclk for grf

[Doug Anderson]

Yakir,

On Wed, Jun 22, 2016 at 6:58 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> For RK3399's GRF module, if we want to operate the graphic related grf
> registers, we need to enable the pclk_vio_grf which supply power for VIO
> GRF IOs, so it's better to introduce an optional grf clock in driver.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Hi all,
>
> This is an external patch for analogix_dp misc cleanup thread [0]
> [0]: https://patchwork.kernel.org/patch/9175613/
>
> BR,
> - Yakir
>
> Changes in v4.1:
> - Fix compiled error, sorry.
>   "dp->cgfclk"  -->  'dp->grfclk'
>
> Changes in v4:
> - Check the the error code properly, 'EPROBE_DEFER' should be returned,
>   'ENOENT' should assign a NULL point to grfclk, other errors should be
>   regarded as failed. (Tomasz, Doug, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/351821/20/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@249]
> - Add the document about optional 'grf' clock (Tomasz, Doug, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/351821/]
>
> Changes in v3:
> - Add this patch in v3
>
>  .../display/rockchip/analogix_dp-rockchip.txt      |  6 ++++++
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 23 +++++++++++++++++++---
>  2 files changed, 26 insertions(+), 3 deletions(-)

I probably would have split into two patches so the bindings was its
own patch, but I don't think it's strictly required.

In any case, this seems good to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v4 2/2] dt-bindings: analogix_dp: rockchip: correct the wrong compatible name

[Doug Anderson]

Hi,

On Wed, Jun 22, 2016 at 6:47 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> The document about rockchip platform make a mistaken in available
> compatible name of "rk3288-edp", we should correct it to "rk3288-dp"
> which correspond to the compatible name in driver.
>
> This mistaken was introduced in commit be91c36247089 ("dt-bindings:
> add document for rockchip variant of analogix_dp").
>
> Reported-by: Tomasz Figa <tfiga@chromium.com>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Hi all,
>
> This is an external patch for analogix_dp misc cleanup thread [0]
> [0]: https://patchwork.kernel.org/patch/9175613/
>
> BR,
> - Yakir
>
> Changes in v4: None
> Changes in v3:
> - Add this patch in v3
>
>  .../devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt       | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 01/10] drm/bridge: analogix_dp: rename RK3288_DP to ROCKCHIP_DP

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Rename RK3288_DP macros to ROCKCHIP_DP, prepare to add eDP
> support for more Rockchip chips.
>
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>


I'm ok with this as long as there aren't going to be future rockchip
tweaks to the driver that will need to be separate from those already
there (ie: versioned). In the tegra driver, the product numbers serve
to represent the first product with said feature or quirk, the same
could have been done here.

However, if we know that all rockchip devices in the future will not
need to differentiate from each other, this seems fine.

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> Changes in v3:
> - Correct the misspell of "marcos" in commit message (Dominik, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346312/9//COMMIT_MSG@9]
> - Add reviewed flag from Stéphane.
>     [https://chromium-review.googlesource.com/#/c/346312/16]
> - Add tested flag from Javier.
>
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 4 ++--
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c  | 6 +++---
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 2 +-
>  include/drm/bridge/analogix_dp.h                   | 2 +-
>  4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 7699597..4a1b3b8 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -1207,9 +1207,9 @@ static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
>         struct video_info *video_info = &dp->video_info;
>
>         switch (dp->plat_data->dev_type) {
> -       case RK3288_DP:
> +       case ROCKCHIP_DP:
>                 /*
> -                * Like Rk3288 DisplayPort TRM indicate that "Main link
> +                * Like Rockchip DisplayPort TRM indicate that "Main link
>                  * containing 4 physical lanes of 2.7/1.62 Gbps/lane".
>                  */
>                 video_info->max_link_rate = 0x0A;
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 49205ef..931a76c 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -74,7 +74,7 @@ void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
>         reg = SEL_24M | TX_DVDD_BIT_1_0625V;
>         writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
>
> -       if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP)) {
> +       if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP)) {
>                 writel(REF_CLK_24M, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
>                 writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
>                 writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
> @@ -244,7 +244,7 @@ void analogix_dp_set_analog_power_down(struct analogix_dp_device *dp,
>         u32 reg;
>         u32 phy_pd_addr = ANALOGIX_DP_PHY_PD;
>
> -       if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP))
> +       if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP))
>                 phy_pd_addr = ANALOGIX_DP_PD;
>
>         switch (block) {
> @@ -448,7 +448,7 @@ void analogix_dp_init_aux(struct analogix_dp_device *dp)
>         analogix_dp_reset_aux(dp);
>
>         /* Disable AUX transaction H/W retry */
> -       if (dp->plat_data && (dp->plat_data->dev_type == RK3288_DP))
> +       if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP))
>                 reg = AUX_BIT_PERIOD_EXPECTED_DELAY(0) |
>                       AUX_HW_RETRY_COUNT_SEL(3) |
>                       AUX_HW_RETRY_INTERVAL_600_MICROSECONDS;
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 7f6a55c..2bc8a7e 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -270,7 +270,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>
>         dp->plat_data.encoder = &dp->encoder;
>
> -       dp->plat_data.dev_type = RK3288_DP;
> +       dp->plat_data.dev_type = ROCKCHIP_DP;
>         dp->plat_data.power_on = rockchip_dp_poweron;
>         dp->plat_data.power_off = rockchip_dp_powerdown;
>
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 25afb31..9e5d013 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -15,7 +15,7 @@
>
>  enum analogix_dp_devtype {
>         EXYNOS_DP,
> -       RK3288_DP,
> +       ROCKCHIP_DP,
>  };
>
>  struct analogix_dp_plat_data {
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 04/10] drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit setting

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> As vendor document indicate, when REF_CLK bit set 0, then DP
> phy's REF_CLK should switch to 24M source clock.
>
> But due to IC PHY layout mistaken, some chips need to flip this
> bit(like RK3288), and unfortunately they didn't indicate in the
> DP version register. That's why we have to make this little hack.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
> ---
> Changes in v3:
> - Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
>   reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
>     [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
> - Add tested flag from Javier
>
> Changes in v2:
> - new patch in v2
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 6 +++++-
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 1 +
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c   | 3 +++
>  include/drm/bridge/analogix_dp.h                  | 5 +++++
>  4 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 931a76c..97ced6b 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -75,7 +75,11 @@ void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
>         writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
>
>         if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP)) {
> -               writel(REF_CLK_24M, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
> +               reg = REF_CLK_24M;
> +               if (dp->plat_data->subdev_type == RK3288_DP)
> +                       reg ^= REF_CLK_MASK;
> +
> +               writel(reg, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
>                 writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
>                 writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
>                 writel(0x58, dp->reg_base + ANALOGIX_DP_PLL_REG_4);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> index 88d56ad..cdcc6c5 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> @@ -165,6 +165,7 @@
>  /* ANALOGIX_DP_PLL_REG_1 */
>  #define REF_CLK_24M                            (0x1 << 0)
>  #define REF_CLK_27M                            (0x0 << 0)
> +#define REF_CLK_MASK                           (0x1 << 0)
>
>  /* ANALOGIX_DP_LANE_MAP */
>  #define LANE3_MAP_LOGIC_LANE_0                 (0x0 << 6)
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 3855f46..315ebba 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -46,6 +46,7 @@ struct rockchip_dp_chip_data {
>         u32     lcdsel_grf_reg;
>         u32     lcdsel_big;
>         u32     lcdsel_lit;
> +       u32     chip_type;
>  };
>
>  struct rockchip_dp_device {
> @@ -286,6 +287,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>         dp->plat_data.encoder = &dp->encoder;
>
>         dp->plat_data.dev_type = ROCKCHIP_DP;
> +       dp->plat_data.subdev_type = dp_data->chip_type;
>         dp->plat_data.power_on = rockchip_dp_poweron;
>         dp->plat_data.power_off = rockchip_dp_powerdown;
>
> @@ -384,6 +386,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
>         .lcdsel_grf_reg = 0x025c,
>         .lcdsel_big = 0 | BIT(21),
>         .lcdsel_lit = BIT(5) | BIT(21),
> +       .chip_type = RK3288_DP,
>  };
>
>  static const struct of_device_id rockchip_dp_dt_ids[] = {
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 9e5d013..06c0250 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -18,8 +18,13 @@ enum analogix_dp_devtype {
>         ROCKCHIP_DP,
>  };
>
> +enum analogix_dp_sub_devtype {
> +       RK3288_DP,
> +};
> +
>  struct analogix_dp_plat_data {
>         enum analogix_dp_devtype dev_type;
> +       enum analogix_dp_sub_devtype subdev_type;


So this is what I was talking about in my review of the first patch of
the series.

I don't personally think the dev and subdev types add any clarity
here, just more state. I'd prefer that you put the product number in
the top level devtype, and add a helper function like:

static bool is_rockchip(enum analogix_dp_devtype type) {
  return type == ... || type == ...
}

Sean

>         struct drm_panel *panel;
>         struct drm_encoder *encoder;
>         struct drm_connector *connector;
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> RK3399 and RK3288 shared the same eDP IP controller, only some light
> difference with VOP configure and GRF configure.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v3:
> - Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
> - Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
> - Add the acked flag from Mark.
>
> Changes in v2:
> - rebase with drm-next, fix some conflicts
>
>  .../bindings/display/bridge/analogix_dp.txt        |  1 +
>  .../display/rockchip/analogix_dp-rockchip.txt      |  3 +-
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 33 +++++++++++++++++++++-
>  include/drm/bridge/analogix_dp.h                   |  1 +
>  4 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> index 4f2ba8c..4a0f4f7 100644
> --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
> @@ -5,6 +5,7 @@ Required properties for dp-controller:
>                 platform specific such as:
>                  * "samsung,exynos5-dp"
>                  * "rockchip,rk3288-dp"
> +                * "rockchip,rk3399-edp"
>         -reg:
>                 physical base address of the controller and length
>                 of memory mapped region.
> diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> index e832ff9..726c945 100644
> --- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> +++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
> @@ -2,7 +2,8 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
>  ================================
>
>  Required properties:
> -- compatible: "rockchip,rk3288-edp";
> +- compatible: "rockchip,rk3288-edp",
> +             "rockchip,rk3399-edp";
>
>  - reg: physical base address of the controller and length
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 315ebba..bcd9ecc 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -153,6 +153,8 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>                                       struct drm_connector_state *conn_state)
>  {
>         struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
> +       struct rockchip_dp_device *dp = to_dp(encoder);
> +       int ret;
>
>         /*
>          * FIXME(Yakir): driver should configure the CRTC output video
> @@ -167,7 +169,28 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>          * But if I configure CTRC to RGBaaa, and eDP driver still keep
>          * RGB666 input video mode, then screen would works prefect.
>          */
> -       s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
> +
> +       ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> +       if (ret < 0)
> +               return 0;

What is s->output_mode set to in the failure case? How about s->output_type?

> +
> +       switch (dp->data->chip_type) {
> +       case RK3399_EDP:
> +               /*
> +                * For RK3399, VOP Lit must code the out mode to RGB888,
> +                * VOP Big must code the out mode to RGB10.
> +                */
> +               if (ret)
> +                       s->output_mode = ROCKCHIP_OUT_MODE_P888;
> +               else
> +                       s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
> +               break;
> +
> +       default:
> +               s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
> +               break;
> +       }
> +

This chunk seems overly complicated. I'd suggest:

s->output_type = DRM_MODE_CONNECTOR_eDP;
s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
if (dp->data->chip_type == RK3399_EDP) {
        ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
        if (ret > 0)
               s->output_mode = ROCKCHIP_OUT_MODE_P888;
}


>         s->output_type = DRM_MODE_CONNECTOR_eDP;
>
>         return 0;
> @@ -382,6 +405,13 @@ static const struct dev_pm_ops rockchip_dp_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
>  };
>
> +static const struct rockchip_dp_chip_data rk3399_edp = {
> +       .lcdsel_grf_reg = 0x6250,
> +       .lcdsel_big = 0 | BIT(21),
> +       .lcdsel_lit = BIT(5) | BIT(21),
> +       .chip_type = RK3399_EDP,
> +};
> +
>  static const struct rockchip_dp_chip_data rk3288_dp = {
>         .lcdsel_grf_reg = 0x025c,
>         .lcdsel_big = 0 | BIT(21),
> @@ -391,6 +421,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
>
>  static const struct of_device_id rockchip_dp_dt_ids[] = {
>         {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
> +       {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 06c0250..82b8135 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -20,6 +20,7 @@ enum analogix_dp_devtype {
>
>  enum analogix_dp_sub_devtype {
>         RK3288_DP,
> +       RK3399_EDP,
>  };
>
>  struct analogix_dp_plat_data {
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 06/10] drm/rockchip: analogix_dp: make panel detect to an optional action

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Some boards don't need to declare a panel device node, like the
> display interface is DP monitors, so it's necessary to make the
> panel detect to an optional action.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v3:
> - Add the acked flag from Mark.
>
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 48 ++++++++++++-------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index bcd9ecc..da2e844 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -332,38 +332,34 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>  {
>         struct device *dev = &pdev->dev;
>         struct device_node *panel_node, *port, *endpoint;
> +       struct drm_panel *panel = NULL;
>         struct rockchip_dp_device *dp;
> -       struct drm_panel *panel;
>
>         port = of_graph_get_port_by_id(dev->of_node, 1);
> -       if (!port) {
> -               dev_err(dev, "can't find output port\n");
> -               return -EINVAL;
> -       }
> -
> -       endpoint = of_get_child_by_name(port, "endpoint");
> -       of_node_put(port);
> -       if (!endpoint) {
> -               dev_err(dev, "no output endpoint found\n");
> -               return -EINVAL;
> -       }
> -
> -       panel_node = of_graph_get_remote_port_parent(endpoint);
> -       of_node_put(endpoint);
> -       if (!panel_node) {
> -               dev_err(dev, "no output node found\n");
> -               return -EINVAL;
> -       }
> -
> -       panel = of_drm_find_panel(panel_node);
> -       if (!panel) {
> -               DRM_ERROR("failed to find panel\n");
> +       if (port) {
> +               endpoint = of_get_child_by_name(port, "endpoint");
> +               of_node_put(port);
> +               if (!endpoint) {
> +                       dev_err(dev, "no output endpoint found\n");
> +                       return -EINVAL;
> +               }
> +
> +               panel_node = of_graph_get_remote_port_parent(endpoint);
> +               of_node_put(endpoint);
> +               if (!panel_node) {
> +                       dev_err(dev, "no output node found\n");
> +                       return -EINVAL;
> +               }
> +
> +               panel = of_drm_find_panel(panel_node);
> +               if (!panel) {
> +                       DRM_ERROR("failed to find panel\n");
> +                       of_node_put(panel_node);
> +                       return -EPROBE_DEFER;
> +               }
>                 of_node_put(panel_node);

Minor nit: Move of_node_put(panel_node) directly below
of_drm_find_panel to avoid duplicating it in both error and normal
paths (like you've done above).

Sean

> -               return -EPROBE_DEFER;
>         }
>
> -       of_node_put(panel_node);
> -
>         dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
>         if (!dp)
>                 return -ENOMEM;
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 07/10] drm/bridge: analogix_dp: passing the connector as an argument in .get_modes()

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> It's better to pass the connector to platform driver in .get_modes()
> callback, just like what the .get_modes() helper function designed.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> Changes in v3:
> - Avoid to change any internal driver state in .mode_valid interface. (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346318/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@113]
>
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 2 +-
>  drivers/gpu/drm/exynos/exynos_dp.c                 | 4 ++--
>  include/drm/bridge/analogix_dp.h                   | 3 ++-
>  3 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 4a1b3b8..1a890fa 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -938,7 +938,7 @@ int analogix_dp_get_modes(struct drm_connector *connector)
>                 num_modes += drm_panel_get_modes(dp->plat_data->panel);
>
>         if (dp->plat_data->get_modes)
> -               num_modes += dp->plat_data->get_modes(dp->plat_data);
> +               num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
>
>         return num_modes;
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_dp.c b/drivers/gpu/drm/exynos/exynos_dp.c
> index 468498e..8a555ed 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp.c
> @@ -67,10 +67,10 @@ static int exynos_dp_poweroff(struct analogix_dp_plat_data *plat_data)
>         return exynos_dp_crtc_clock_enable(plat_data, false);
>  }
>
> -static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data)
> +static int exynos_dp_get_modes(struct analogix_dp_plat_data *plat_data,
> +                              struct drm_connector *connector)
>  {
>         struct exynos_dp_device *dp = to_dp(plat_data);
> -       struct drm_connector *connector = &dp->connector;
>         struct drm_display_mode *mode;
>         int num_modes = 0;
>
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index 82b8135..181db09 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -34,7 +34,8 @@ struct analogix_dp_plat_data {
>         int (*power_off)(struct analogix_dp_plat_data *);
>         int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
>                       struct drm_connector *);
> -       int (*get_modes)(struct analogix_dp_plat_data *);
> +       int (*get_modes)(struct analogix_dp_plat_data *,
> +                        struct drm_connector *);
>  };
>
>  int analogix_dp_resume(struct device *dev);
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 08/10] drm/rockchip: analogix_dp: correct the connector display color format and bpc

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> Rockchip VOP couldn't output YUV video format for eDP controller, so
> when driver detect connector support YUV video format, we need to hack
> it down to RGB888.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v3:
> - Hook the connector's color_formats in .get_modes directly. (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346317/15]
> - Add the acked flag from Mark.
>
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index da2e844..95a6f60 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -102,6 +102,22 @@ static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
>         return 0;
>  }
>
> +static int rockchip_dp_get_modes(struct analogix_dp_plat_data *plat_data,
> +                                struct drm_connector *connector)
> +{
> +       struct drm_display_info *di = &connector->display_info;
> +
> +       if (di->color_formats & DRM_COLOR_FORMAT_YCRCB444 ||
> +           di->color_formats & DRM_COLOR_FORMAT_YCRCB422) {
> +               di->color_formats &= ~(DRM_COLOR_FORMAT_YCRCB422 |
> +                                      DRM_COLOR_FORMAT_YCRCB444);
> +               di->color_formats |= DRM_COLOR_FORMAT_RGB444;
> +               di->bpc = 8;

Probably a stupid question, but are you guaranteed that the panel will
support this color format?

> +       }
> +

I think this can be simplified as follows:

/* A comment here about why we're doing this */
u32 mask = DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422;
if ((di->color_formats & mask)) {
    DRM_DEBUG_KMS("Swapping display color format from YUV to RGB\n");
    di->color_formats &= ~mask;
    di->color_formats |= DRM_COLOR_FORMAT_RGB444;
    di->bpc = 8;
}

> +       return 0;
> +}
> +
>  static bool
>  rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>                                    const struct drm_display_mode *mode,
> @@ -313,6 +329,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>         dp->plat_data.subdev_type = dp_data->chip_type;
>         dp->plat_data.power_on = rockchip_dp_poweron;
>         dp->plat_data.power_off = rockchip_dp_powerdown;
> +       dp->plat_data.get_modes = rockchip_dp_get_modes;
>
>         return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
>  }
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 09/10] drm/rockchip: analogix_dp: update the comments about why need to hardcode VOP output mode

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> The hardware IC designed that VOP must output the RGB10 video format to
> eDP contoller, and if eDP panel only support RGB8, then eDP contoller
> should cut down the video data, not via VOP contoller, that's why we need
> to hardcode the VOP output mode to RGA10 here.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
> ---
> Changes in v3:
> - Add the reviewed flag from Tomasz.
>     [https://chromium-review.googlesource.com/#/c/346853/12]
> - Add the acked flag from Mark.
>
> Changes in v2:
> - new patch in v2
>
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 16 +++++-----------
>  1 file changed, 5 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 95a6f60..2ceb3f9 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -173,17 +173,11 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>         int ret;
>
>         /*
> -        * FIXME(Yakir): driver should configure the CRTC output video
> -        * mode with the display information which indicated the monitor
> -        * support colorimetry.
> -        *
> -        * But don't know why the CRTC driver seems could only output the
> -        * RGBaaa rightly. For example, if connect the "innolux,n116bge"
> -        * eDP screen, EDID would indicated that screen only accepted the
> -        * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
> -        * screen would show a blue picture (RGB888 show a green picture).
> -        * But if I configure CTRC to RGBaaa, and eDP driver still keep
> -        * RGB666 input video mode, then screen would works prefect.
> +        * The hardware IC designed that VOP must output the RGB10 video
> +        * format to eDP contoller, and if eDP panel only support RGB8,
> +        * then eDP contoller should cut down the video data, not via VOP
> +        * contoller, that's why we need to hardcode the VOP output mode
> +        * to RGA10 here.

s/contoller/controller/

>          */
>
>         ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 10/10] drm/bridge: analogix_dp: fix no drm hpd event when panel plug in

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> The enum value of DP_IRQ_TYPE_HP_CABLE_IN is zero, but driver only
> send drm hp event when the irq_type and the enum value is true.
>
> if (irq_type & DP_IRQ_TYPE_HP_CABLE_IN || ...)
>         drm_helper_hpd_irq_event(dp->drm_dev);
>
> So there would no drm hpd event when cable plug in, to fix that
> just need to assign all hotplug enum with no-zero values.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> Changes in v3:
> - Add reviewed flag from Stéphane.
>     [https://chromium-review.googlesource.com/#/c/346319/15]
> - Add tested flag from Javier
>
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> index f09275d..b456380 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -127,10 +127,10 @@ enum analog_power_block {
>  };
>
>  enum dp_irq_type {
> -       DP_IRQ_TYPE_HP_CABLE_IN,
> -       DP_IRQ_TYPE_HP_CABLE_OUT,
> -       DP_IRQ_TYPE_HP_CHANGE,
> -       DP_IRQ_TYPE_UNKNOWN,
> +       DP_IRQ_TYPE_HP_CABLE_IN  = BIT(0),
> +       DP_IRQ_TYPE_HP_CABLE_OUT = BIT(1),
> +       DP_IRQ_TYPE_HP_CHANGE    = BIT(2),
> +       DP_IRQ_TYPE_UNKNOWN      = BIT(3),
>  };
>
>  struct video_info {
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/10] drm/rockchip: analogix_dp: split the lcdc select setting into device data

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> eDP controller need to declare which vop provide the video source,
> and it's defined in GRF registers.
>
> But different chips have different GRF register address, so we need to
> create a device data to declare the GRF messages for each chips.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Acked-by: Mark Yao <mark.yao@rock-chips.com>
> ---
> Changes in v3:
> - Write a kerneldoc-style comment explaining the chips data fields (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@39]
> - Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed at Google Gerrit)
>     [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@382]
> - Add acked flag from Mark.
>
> Changes in v2: None
>
>  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 39 +++++++++++++++++++------
>  1 file changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> index 2bc8a7e..3855f46 100644
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -14,6 +14,7 @@
>
>  #include <linux/component.h>
>  #include <linux/mfd/syscon.h>
> +#include <linux/of_device.h>
>  #include <linux/of_graph.h>
>  #include <linux/regmap.h>
>  #include <linux/reset.h>
> @@ -35,11 +36,17 @@
>
>  #define to_dp(nm)      container_of(nm, struct rockchip_dp_device, nm)
>
> -/* dp grf register offset */
> -#define GRF_SOC_CON6                            0x025c
> -#define GRF_EDP_LCD_SEL_MASK                    BIT(5)
> -#define GRF_EDP_SEL_VOP_LIT                     BIT(5)
> -#define GRF_EDP_SEL_VOP_BIG                     0
> +/**
> + * struct rockchip_dp_chip_data - splite the grf setting of kind of chips
> + * @lcdsel_grf_reg: grf register offset of lcdc select
> + * @lcdsel_big: reg value of selecting vop big for eDP
> + * @lcdsel_lit: reg value of selecting vop little for eDP
> + */
> +struct rockchip_dp_chip_data {
> +       u32     lcdsel_grf_reg;
> +       u32     lcdsel_big;
> +       u32     lcdsel_lit;
> +};
>
>  struct rockchip_dp_device {
>         struct drm_device        *drm_dev;
> @@ -51,6 +58,8 @@ struct rockchip_dp_device {
>         struct regmap            *grf;
>         struct reset_control     *rst;
>
> +       const struct rockchip_dp_chip_data *data;
> +
>         struct analogix_dp_plat_data plat_data;
>  };
>
> @@ -119,13 +128,13 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder)
>                 return;
>
>         if (ret)
> -               val = GRF_EDP_SEL_VOP_LIT | (GRF_EDP_LCD_SEL_MASK << 16);
> +               val = dp->data->lcdsel_lit;
>         else
> -               val = GRF_EDP_SEL_VOP_BIG | (GRF_EDP_LCD_SEL_MASK << 16);
> +               val = dp->data->lcdsel_big;
>
>         dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
>
> -       ret = regmap_write(dp->grf, GRF_SOC_CON6, val);
> +       ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
>         if (ret != 0) {
>                 dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
>                 return;
> @@ -246,6 +255,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>                             void *data)
>  {
>         struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> +       const struct rockchip_dp_chip_data *dp_data;
>         struct drm_device *drm_dev = data;
>         int ret;
>
> @@ -256,10 +266,15 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>          */
>         dev_set_drvdata(dev, NULL);
>
> +       dp_data = of_device_get_match_data(dev);
> +       if (!dp_data)
> +               return -ENODEV;
> +
>         ret = rockchip_dp_init(dp);
>         if (ret < 0)
>                 return ret;
>
> +       dp->data = dp_data;
>         dp->drm_dev = drm_dev;
>
>         ret = rockchip_dp_drm_create_encoder(dp);
> @@ -365,8 +380,14 @@ static const struct dev_pm_ops rockchip_dp_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
>  };
>
> +static const struct rockchip_dp_chip_data rk3288_dp = {
> +       .lcdsel_grf_reg = 0x025c,
> +       .lcdsel_big = 0 | BIT(21),
> +       .lcdsel_lit = BIT(5) | BIT(21),

I'm not sure what convention is in other drivers, but this seems less
readable to me.

I'd prefer that the magic numbers were assigned to a #define that
corresponds to a TRM name in some header. Then you have names in the
assignment instead of meaningless numbers.

Sean

> +};
> +
>  static const struct of_device_id rockchip_dp_dt_ids[] = {
> -       {.compatible = "rockchip,rk3288-dp",},
> +       {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 03/10] drm/bridge: analogix_dp: correct the register bit define error in ANALOGIX_DP_PLL_REG_1

[Sean Paul]

On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> There're an register define error in ANALOGIX_DP_PLL_REG_1 which introduced
> by commit bcec20fd5ad6 ("drm: bridge: analogix/dp: add some rk3288 special
> registers setting").
>
> The PHY PLL input clock source is selected by ANALOGIX_DP_PLL_REG_1
> BIT 0, not BIT 1.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
> Changes in v3:
> - Add reviewed flag from Tomasz.
>     [https://chromium-review.googlesource.com/#/c/346315/15]
> - Add tested flag from Javier
>
> Changes in v2: None
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> index 337912b..88d56ad 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
> @@ -163,8 +163,8 @@
>  #define HSYNC_POLARITY_CFG                     (0x1 << 0)
>
>  /* ANALOGIX_DP_PLL_REG_1 */
> -#define REF_CLK_24M                            (0x1 << 1)
> -#define REF_CLK_27M                            (0x0 << 1)
> +#define REF_CLK_24M                            (0x1 << 0)
> +#define REF_CLK_27M                            (0x0 << 0)
>
>  /* ANALOGIX_DP_LANE_MAP */
>  #define LANE3_MAP_LOGIC_LANE_0                 (0x0 << 6)
> --
> 1.9.1
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 02/10] drm/rockchip: analogix_dp: split the lcdc select setting into device data

[Heiko Stuebner]

Am Donnerstag, 23. Juni 2016, 10:32:53 schrieb Sean Paul:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
> > eDP controller need to declare which vop provide the video source,
> > and it's defined in GRF registers.
> > 
> > But different chips have different GRF register address, so we need to
> > create a device data to declare the GRF messages for each chips.
> > 
> > Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> > Acked-by: Mark Yao <mark.yao@rock-chips.com>
> > ---
> > Changes in v3:
> > - Write a kerneldoc-style comment explaining the chips data fields
> > (Tomasz, reviewed at Google Gerrit)> 
> >     [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/
> >     drm/rockchip/analogix_dp-rockchip.c@39]> 
> > - Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed
> > at Google Gerrit)> 
> >     [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/
> >     drm/rockchip/analogix_dp-rockchip.c@382]> 
> > - Add acked flag from Mark.
> > 
> > Changes in v2: None
> > 
> >  drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 39
> >  +++++++++++++++++++------ 1 file changed, 30 insertions(+), 9
> >  deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index
> > 2bc8a7e..3855f46 100644
> > --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> > @@ -14,6 +14,7 @@
> > 
> >  #include <linux/component.h>
> >  #include <linux/mfd/syscon.h>
> > 
> > +#include <linux/of_device.h>
> > 
> >  #include <linux/of_graph.h>
> >  #include <linux/regmap.h>
> >  #include <linux/reset.h>
> > 
> > @@ -35,11 +36,17 @@
> > 
> >  #define to_dp(nm)      container_of(nm, struct rockchip_dp_device, nm)
> > 
> > -/* dp grf register offset */
> > -#define GRF_SOC_CON6                            0x025c
> > -#define GRF_EDP_LCD_SEL_MASK                    BIT(5)
> > -#define GRF_EDP_SEL_VOP_LIT                     BIT(5)
> > -#define GRF_EDP_SEL_VOP_BIG                     0
> > +/**
> > + * struct rockchip_dp_chip_data - splite the grf setting of kind of
> > chips + * @lcdsel_grf_reg: grf register offset of lcdc select
> > + * @lcdsel_big: reg value of selecting vop big for eDP
> > + * @lcdsel_lit: reg value of selecting vop little for eDP
> > + */
> > +struct rockchip_dp_chip_data {
> > +       u32     lcdsel_grf_reg;
> > +       u32     lcdsel_big;
> > +       u32     lcdsel_lit;
> > +};
> > 
> >  struct rockchip_dp_device {
> >  
> >         struct drm_device        *drm_dev;
> > 
> > @@ -51,6 +58,8 @@ struct rockchip_dp_device {
> > 
> >         struct regmap            *grf;
> >         struct reset_control     *rst;
> > 
> > +       const struct rockchip_dp_chip_data *data;
> > +
> > 
> >         struct analogix_dp_plat_data plat_data;
> >  
> >  };
> > 
> > @@ -119,13 +128,13 @@ static void rockchip_dp_drm_encoder_enable(struct
> > drm_encoder *encoder)> 
> >                 return;
> >         
> >         if (ret)
> > 
> > -               val = GRF_EDP_SEL_VOP_LIT | (GRF_EDP_LCD_SEL_MASK <<
> > 16);
> > +               val = dp->data->lcdsel_lit;
> > 
> >         else
> > 
> > -               val = GRF_EDP_SEL_VOP_BIG | (GRF_EDP_LCD_SEL_MASK <<
> > 16);
> > +               val = dp->data->lcdsel_big;
> > 
> >         dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" :
> >         "BIG");
> > 
> > -       ret = regmap_write(dp->grf, GRF_SOC_CON6, val);
> > +       ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
> > 
> >         if (ret != 0) {
> >         
> >                 dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
> >                 return;
> > 
> > @@ -246,6 +255,7 @@ static int rockchip_dp_bind(struct device *dev,
> > struct device *master,> 
> >                             void *data)
> >  
> >  {
> >  
> >         struct rockchip_dp_device *dp = dev_get_drvdata(dev);
> > 
> > +       const struct rockchip_dp_chip_data *dp_data;
> > 
> >         struct drm_device *drm_dev = data;
> >         int ret;
> > 
> > @@ -256,10 +266,15 @@ static int rockchip_dp_bind(struct device *dev,
> > struct device *master,> 
> >          */
> >         
> >         dev_set_drvdata(dev, NULL);
> > 
> > +       dp_data = of_device_get_match_data(dev);
> > +       if (!dp_data)
> > +               return -ENODEV;
> > +
> > 
> >         ret = rockchip_dp_init(dp);
> >         if (ret < 0)
> >         
> >                 return ret;
> > 
> > +       dp->data = dp_data;
> > 
> >         dp->drm_dev = drm_dev;
> >         
> >         ret = rockchip_dp_drm_create_encoder(dp);
> > 
> > @@ -365,8 +380,14 @@ static const struct dev_pm_ops rockchip_dp_pm_ops =
> > {> 
> >         SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
> >  
> >  };
> > 
> > +static const struct rockchip_dp_chip_data rk3288_dp = {
> > +       .lcdsel_grf_reg = 0x025c,
> > +       .lcdsel_big = 0 | BIT(21),
> > +       .lcdsel_lit = BIT(5) | BIT(21),
> 
> I'm not sure what convention is in other drivers, but this seems less
> readable to me.
> 
> I'd prefer that the magic numbers were assigned to a #define that
> corresponds to a TRM name in some header. Then you have names in the
> assignment instead of meaningless numbers.

I guess what irks me personally more is that it hides the write-mask as well

So I guess something like the following would also define it somewhat future-
proof

#define RK3288_GRF_SOC_CON6	0x25c
#define RK3288_EDP_LCDC_SEL		BIT(5)

#define HIWORD_UPDATE(val, mask) \
		(val | (mask) << 16)

static const struct rockchip_dp_chip_data rk3288_dp = {
	.lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
	.lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
	.lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),

Re: [PATCH v4 2/2] dt-bindings: analogix_dp: rockchip: correct the wrong compatible name

[Yakir Yang]

Doug,

On 06/23/2016 01:17 PM, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 22, 2016 at 6:47 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> The document about rockchip platform make a mistaken in available
>> compatible name of "rk3288-edp", we should correct it to "rk3288-dp"
>> which correspond to the compatible name in driver.
>>
>> This mistaken was introduced in commit be91c36247089 ("dt-bindings:
>> add document for rockchip variant of analogix_dp").
>>
>> Reported-by: Tomasz Figa <tfiga@chromium.com>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Hi all,
>>
>> This is an external patch for analogix_dp misc cleanup thread [0]
>> [0]: https://patchwork.kernel.org/patch/9175613/
>>
>> BR,
>> - Yakir
>>
>> Changes in v4: None
>> Changes in v3:
>> - Add this patch in v3
>>
>>   .../devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt       | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
Thanks ;)
>
>

Re: [PATCH v4.1 1/2] drm/rockchip: analogix_dp: introduce the pclk for grf

[Yakir Yang]

Doug,

On 06/23/2016 01:16 PM, Doug Anderson wrote:
> Yakir,
>
> On Wed, Jun 22, 2016 at 6:58 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> For RK3399's GRF module, if we want to operate the graphic related grf
>> registers, we need to enable the pclk_vio_grf which supply power for VIO
>> GRF IOs, so it's better to introduce an optional grf clock in driver.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Hi all,
>>
>> This is an external patch for analogix_dp misc cleanup thread [0]
>> [0]: https://patchwork.kernel.org/patch/9175613/
>>
>> BR,
>> - Yakir
>>
>> Changes in v4.1:
>> - Fix compiled error, sorry.
>>    "dp->cgfclk"  -->  'dp->grfclk'
>>
>> Changes in v4:
>> - Check the the error code properly, 'EPROBE_DEFER' should be returned,
>>    'ENOENT' should assign a NULL point to grfclk, other errors should be
>>    regarded as failed. (Tomasz, Doug, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/351821/20/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@249]
>> - Add the document about optional 'grf' clock (Tomasz, Doug, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/351821/]
>>
>> Changes in v3:
>> - Add this patch in v3
>>
>>   .../display/rockchip/analogix_dp-rockchip.txt      |  6 ++++++
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 23 +++++++++++++++++++---
>>   2 files changed, 26 insertions(+), 3 deletions(-)
> I probably would have split into two patches so the bindings was its
> own patch, but I don't think it's strictly required.
>
> In any case, this seems good to me.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
>
This is not big change, so collect them into one patch should be more 
cleaner, thanks for your reviewed ;)
>

Re: [PATCH v3 10/10] drm/bridge: analogix_dp: fix no drm hpd event when panel plug in

[Yakir Yang]

Sean,

On 06/23/2016 10:24 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> The enum value of DP_IRQ_TYPE_HP_CABLE_IN is zero, but driver only
>> send drm hp event when the irq_type and the enum value is true.
>>
>> if (irq_type & DP_IRQ_TYPE_HP_CABLE_IN || ...)
>>          drm_helper_hpd_irq_event(dp->drm_dev);
>>
>> So there would no drm hpd event when cable plug in, to fix that
>> just need to assign all hotplug enum with no-zero values.
>>
>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Reviewed-by: Stéphane Marchesin <marcheu@chromium.org>
>> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>
>

Thanks  ;)

>> ---
>> Changes in v3:
>> - Add reviewed flag from Stéphane.
>>      [https://chromium-review.googlesource.com/#/c/346319/15]
>> - Add tested flag from Javier
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> index f09275d..b456380 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> @@ -127,10 +127,10 @@ enum analog_power_block {
>>   };
>>
>>   enum dp_irq_type {
>> -       DP_IRQ_TYPE_HP_CABLE_IN,
>> -       DP_IRQ_TYPE_HP_CABLE_OUT,
>> -       DP_IRQ_TYPE_HP_CHANGE,
>> -       DP_IRQ_TYPE_UNKNOWN,
>> +       DP_IRQ_TYPE_HP_CABLE_IN  = BIT(0),
>> +       DP_IRQ_TYPE_HP_CABLE_OUT = BIT(1),
>> +       DP_IRQ_TYPE_HP_CHANGE    = BIT(2),
>> +       DP_IRQ_TYPE_UNKNOWN      = BIT(3),
>>   };
>>
>>   struct video_info {
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 08/10] drm/rockchip: analogix_dp: correct the connector display color format and bpc

[Yakir Yang]

Sean,

On 06/23/2016 10:19 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Rockchip VOP couldn't output YUV video format for eDP controller, so
>> when driver detect connector support YUV video format, we need to hack
>> it down to RGB888.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v3:
>> - Hook the connector's color_formats in .get_modes directly. (Tomasz, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346317/15]
>> - Add the acked flag from Mark.
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> index da2e844..95a6f60 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -102,6 +102,22 @@ static int rockchip_dp_powerdown(struct analogix_dp_plat_data *plat_data)
>>          return 0;
>>   }
>>
>> +static int rockchip_dp_get_modes(struct analogix_dp_plat_data *plat_data,
>> +                                struct drm_connector *connector)
>> +{
>> +       struct drm_display_info *di = &connector->display_info;
>> +
>> +       if (di->color_formats & DRM_COLOR_FORMAT_YCRCB444 ||
>> +           di->color_formats & DRM_COLOR_FORMAT_YCRCB422) {
>> +               di->color_formats &= ~(DRM_COLOR_FORMAT_YCRCB422 |
>> +                                      DRM_COLOR_FORMAT_YCRCB444);
>> +               di->color_formats |= DRM_COLOR_FORMAT_RGB444;
>> +               di->bpc = 8;
> Probably a stupid question, but are you guaranteed that the panel will
> support this color format?

Hmm... I'm not sure, but it works on my several panels. I guess RGB is 
more common then YUV, so if panel support YUV format, it should support 
RGB888 too.

>> +       }
>> +
> I think this can be simplified as follows:
>
> /* A comment here about why we're doing this */
> u32 mask = DRM_COLOR_FORMAT_YCRCB444 | DRM_COLOR_FORMAT_YCRCB422;
> if ((di->color_formats & mask)) {
>      DRM_DEBUG_KMS("Swapping display color format from YUV to RGB\n");
>      di->color_formats &= ~mask;
>      di->color_formats |= DRM_COLOR_FORMAT_RGB444;
>      di->bpc = 8;
> }

Done, good idea.

Thanks,
- Yakir

>> +       return 0;
>> +}
>> +
>>   static bool
>>   rockchip_dp_drm_encoder_mode_fixup(struct drm_encoder *encoder,
>>                                     const struct drm_display_mode *mode,
>> @@ -313,6 +329,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>>          dp->plat_data.subdev_type = dp_data->chip_type;
>>          dp->plat_data.power_on = rockchip_dp_poweron;
>>          dp->plat_data.power_off = rockchip_dp_powerdown;
>> +       dp->plat_data.get_modes = rockchip_dp_get_modes;
>>
>>          return analogix_dp_bind(dev, dp->drm_dev, &dp->plat_data);
>>   }
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 09/10] drm/rockchip: analogix_dp: update the comments about why need to hardcode VOP output mode

[Yakir Yang]

Sean,

On 06/23/2016 10:22 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> The hardware IC designed that VOP must output the RGB10 video format to
>> eDP contoller, and if eDP panel only support RGB8, then eDP contoller
>> should cut down the video data, not via VOP contoller, that's why we need
>> to hardcode the VOP output mode to RGA10 here.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>> Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
>> ---
>> Changes in v3:
>> - Add the reviewed flag from Tomasz.
>>      [https://chromium-review.googlesource.com/#/c/346853/12]
>> - Add the acked flag from Mark.
>>
>> Changes in v2:
>> - new patch in v2
>>
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 16 +++++-----------
>>   1 file changed, 5 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> index 95a6f60..2ceb3f9 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -173,17 +173,11 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>>          int ret;
>>
>>          /*
>> -        * FIXME(Yakir): driver should configure the CRTC output video
>> -        * mode with the display information which indicated the monitor
>> -        * support colorimetry.
>> -        *
>> -        * But don't know why the CRTC driver seems could only output the
>> -        * RGBaaa rightly. For example, if connect the "innolux,n116bge"
>> -        * eDP screen, EDID would indicated that screen only accepted the
>> -        * 6bpc mode. But if I configure CRTC to RGB666 output, then eDP
>> -        * screen would show a blue picture (RGB888 show a green picture).
>> -        * But if I configure CTRC to RGBaaa, and eDP driver still keep
>> -        * RGB666 input video mode, then screen would works prefect.
>> +        * The hardware IC designed that VOP must output the RGB10 video
>> +        * format to eDP contoller, and if eDP panel only support RGB8,
>> +        * then eDP contoller should cut down the video data, not via VOP
>> +        * contoller, that's why we need to hardcode the VOP output mode
>> +        * to RGA10 here.
> s/contoller/controller/
>

Done,

>>           */
>>
>>          ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 06/10] drm/rockchip: analogix_dp: make panel detect to an optional action

[Yakir Yang]

Sean,

On 06/23/2016 10:10 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> Some boards don't need to declare a panel device node, like the
>> display interface is DP monitors, so it's necessary to make the
>> panel detect to an optional action.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v3:
>> - Add the acked flag from Mark.
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 48 ++++++++++++-------------
>>   1 file changed, 22 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> index bcd9ecc..da2e844 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -332,38 +332,34 @@ static int rockchip_dp_probe(struct platform_device *pdev)
>>   {
>>          struct device *dev = &pdev->dev;
>>          struct device_node *panel_node, *port, *endpoint;
>> +       struct drm_panel *panel = NULL;
>>          struct rockchip_dp_device *dp;
>> -       struct drm_panel *panel;
>>
>>          port = of_graph_get_port_by_id(dev->of_node, 1);
>> -       if (!port) {
>> -               dev_err(dev, "can't find output port\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       endpoint = of_get_child_by_name(port, "endpoint");
>> -       of_node_put(port);
>> -       if (!endpoint) {
>> -               dev_err(dev, "no output endpoint found\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       panel_node = of_graph_get_remote_port_parent(endpoint);
>> -       of_node_put(endpoint);
>> -       if (!panel_node) {
>> -               dev_err(dev, "no output node found\n");
>> -               return -EINVAL;
>> -       }
>> -
>> -       panel = of_drm_find_panel(panel_node);
>> -       if (!panel) {
>> -               DRM_ERROR("failed to find panel\n");
>> +       if (port) {
>> +               endpoint = of_get_child_by_name(port, "endpoint");
>> +               of_node_put(port);
>> +               if (!endpoint) {
>> +                       dev_err(dev, "no output endpoint found\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               panel_node = of_graph_get_remote_port_parent(endpoint);
>> +               of_node_put(endpoint);
>> +               if (!panel_node) {
>> +                       dev_err(dev, "no output node found\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               panel = of_drm_find_panel(panel_node);
>> +               if (!panel) {
>> +                       DRM_ERROR("failed to find panel\n");
>> +                       of_node_put(panel_node);
>> +                       return -EPROBE_DEFER;
>> +               }
>>                  of_node_put(panel_node);
> Minor nit: Move of_node_put(panel_node) directly below
> of_drm_find_panel to avoid duplicating it in both error and normal
> paths (like you've done above).

Yep, done.

Thanks,
- Yakir

> Sean
>
>> -               return -EPROBE_DEFER;
>>          }
>>
>> -       of_node_put(panel_node);
>> -
>>          dp = devm_kzalloc(dev, sizeof(*dp), GFP_KERNEL);
>>          if (!dp)
>>                  return -ENOMEM;
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 05/10] drm/rockchip: analogix_dp: add rk3399 eDP support

[Yakir Yang]

Sean,

On 06/23/2016 09:48 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> RK3399 and RK3288 shared the same eDP IP controller, only some light
>> difference with VOP configure and GRF configure.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>> ---
>> Changes in v3:
>> - Give the "rk3399-edp" a separate line for clarity in document (Tomasz, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346314/10/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt@5]
>> - Move 'output_type' setting before the return statement (Tomasz, reviewed at Google Gerrit)
>>      [https://chromium-review.googlesource.com/#/c/346314/10/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c@154]
>> - Add the acked flag from Mark.
>>
>> Changes in v2:
>> - rebase with drm-next, fix some conflicts
>>
>>   .../bindings/display/bridge/analogix_dp.txt        |  1 +
>>   .../display/rockchip/analogix_dp-rockchip.txt      |  3 +-
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c    | 33 +++++++++++++++++++++-
>>   include/drm/bridge/analogix_dp.h                   |  1 +
>>   4 files changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> index 4f2ba8c..4a0f4f7 100644
>> --- a/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> +++ b/Documentation/devicetree/bindings/display/bridge/analogix_dp.txt
>> @@ -5,6 +5,7 @@ Required properties for dp-controller:
>>                  platform specific such as:
>>                   * "samsung,exynos5-dp"
>>                   * "rockchip,rk3288-dp"
>> +                * "rockchip,rk3399-edp"
>>          -reg:
>>                  physical base address of the controller and length
>>                  of memory mapped region.
>> diff --git a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> index e832ff9..726c945 100644
>> --- a/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> +++ b/Documentation/devicetree/bindings/display/rockchip/analogix_dp-rockchip.txt
>> @@ -2,7 +2,8 @@ Rockchip RK3288 specific extensions to the Analogix Display Port
>>   ================================
>>
>>   Required properties:
>> -- compatible: "rockchip,rk3288-edp";
>> +- compatible: "rockchip,rk3288-edp",
>> +             "rockchip,rk3399-edp";
>>
>>   - reg: physical base address of the controller and length
>>
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> index 315ebba..bcd9ecc 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -153,6 +153,8 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>>                                        struct drm_connector_state *conn_state)
>>   {
>>          struct rockchip_crtc_state *s = to_rockchip_crtc_state(crtc_state);
>> +       struct rockchip_dp_device *dp = to_dp(encoder);
>> +       int ret;
>>
>>          /*
>>           * FIXME(Yakir): driver should configure the CRTC output video
>> @@ -167,7 +169,28 @@ rockchip_dp_drm_encoder_atomic_check(struct drm_encoder *encoder,
>>           * But if I configure CTRC to RGBaaa, and eDP driver still keep
>>           * RGB666 input video mode, then screen would works prefect.
>>           */
>> -       s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
>> +
>> +       ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>> +       if (ret < 0)
>> +               return 0;
> What is s->output_mode set to in the failure case? How about s->output_type?

I plan to return '-ENODEV' when 'drm_of_encoder_active_endpoint_id' 
failed to found, but now i found there is a bug.

When the first time '.atomic_check' was called, encoder have binded to 
any crtc, so the 'drm_of_encoder_active_endpoint_id' would be failed.
-  If we return failure in this case, then '.atomic_check' would be 
called again, then failed again, dead loop.
-  If we return zero in this case and set a default 
output_mode/output_type, then '.atomic_check' would be successes, then 
'crtc->enable' function would use that default setting.
    When framework called the 'atomic_check' again, then 
'drm_of_encoder_active_endpoint_id' would return the right value, and 
'crtc_state would be set rightly. *But* 'vop->enable' wouldn't take care 
of those setting, caused it would return directly after detect vop have 
been enabled.

    This bug need patches both in connector and crtc driver, need more 
discuss about this. ;)

Thanks,
- Yakir

>> +
>> +       switch (dp->data->chip_type) {
>> +       case RK3399_EDP:
>> +               /*
>> +                * For RK3399, VOP Lit must code the out mode to RGB888,
>> +                * VOP Big must code the out mode to RGB10.
>> +                */
>> +               if (ret)
>> +                       s->output_mode = ROCKCHIP_OUT_MODE_P888;
>> +               else
>> +                       s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
>> +               break;
>> +
>> +       default:
>> +               s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
>> +               break;
>> +       }
>> +
> This chunk seems overly complicated. I'd suggest:
>
> s->output_type = DRM_MODE_CONNECTOR_eDP;
> s->output_mode = ROCKCHIP_OUT_MODE_AAAA;
> if (dp->data->chip_type == RK3399_EDP) {
>          ret = drm_of_encoder_active_endpoint_id(dp->dev->of_node, encoder);
>          if (ret > 0)
>                 s->output_mode = ROCKCHIP_OUT_MODE_P888;
> }

Ah, much clean, done.

Thanks,
- Yakir

>
>>          s->output_type = DRM_MODE_CONNECTOR_eDP;
>>
>>          return 0;
>> @@ -382,6 +405,13 @@ static const struct dev_pm_ops rockchip_dp_pm_ops = {
>>          SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
>>   };
>>
>> +static const struct rockchip_dp_chip_data rk3399_edp = {
>> +       .lcdsel_grf_reg = 0x6250,
>> +       .lcdsel_big = 0 | BIT(21),
>> +       .lcdsel_lit = BIT(5) | BIT(21),
>> +       .chip_type = RK3399_EDP,
>> +};
>> +
>>   static const struct rockchip_dp_chip_data rk3288_dp = {
>>          .lcdsel_grf_reg = 0x025c,
>>          .lcdsel_big = 0 | BIT(21),
>> @@ -391,6 +421,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
>>
>>   static const struct of_device_id rockchip_dp_dt_ids[] = {
>>          {.compatible = "rockchip,rk3288-dp", .data = &rk3288_dp },
>> +       {.compatible = "rockchip,rk3399-edp", .data = &rk3399_edp },
>>          {}
>>   };
>>   MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids);
>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>> index 06c0250..82b8135 100644
>> --- a/include/drm/bridge/analogix_dp.h
>> +++ b/include/drm/bridge/analogix_dp.h
>> @@ -20,6 +20,7 @@ enum analogix_dp_devtype {
>>
>>   enum analogix_dp_sub_devtype {
>>          RK3288_DP,
>> +       RK3399_EDP,
>>   };
>>
>>   struct analogix_dp_plat_data {
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 02/10] drm/rockchip: analogix_dp: split the lcdc select setting into device data[Involving remittance information, please pay attention to the safety of property]

[Yakir Yang]

Heiko & Sean

On 06/24/2016 12:16 AM, Heiko Stuebner wrote:
> Am Donnerstag, 23. Juni 2016, 10:32:53 schrieb Sean Paul:
>> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>>> eDP controller need to declare which vop provide the video source,
>>> and it's defined in GRF registers.
>>>
>>> But different chips have different GRF register address, so we need to
>>> create a device data to declare the GRF messages for each chips.
>>>
>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>>> Acked-by: Mark Yao <mark.yao@rock-chips.com>
>>> ---
>>> Changes in v3:
>>> - Write a kerneldoc-style comment explaining the chips data fields
>>> (Tomasz, reviewed at Google Gerrit)>
>>>      [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/
>>>      drm/rockchip/analogix_dp-rockchip.c@39]>
>>> - Drop the '.lcdcsel_mask' number in chips data field (Tomasz, reviewed
>>> at Google Gerrit)>
>>>      [https://chromium-review.googlesource.com/#/c/346313/10/drivers/gpu/
>>>      drm/rockchip/analogix_dp-rockchip.c@382]>
>>> - Add acked flag from Mark.
>>>
>>> Changes in v2: None
>>>
>>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 39
>>>   +++++++++++++++++++------ 1 file changed, 30 insertions(+), 9
>>>   deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index
>>> 2bc8a7e..3855f46 100644
>>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>>> @@ -14,6 +14,7 @@
>>>
>>>   #include <linux/component.h>
>>>   #include <linux/mfd/syscon.h>
>>>
>>> +#include <linux/of_device.h>
>>>
>>>   #include <linux/of_graph.h>
>>>   #include <linux/regmap.h>
>>>   #include <linux/reset.h>
>>>
>>> @@ -35,11 +36,17 @@
>>>
>>>   #define to_dp(nm)      container_of(nm, struct rockchip_dp_device, nm)
>>>
>>> -/* dp grf register offset */
>>> -#define GRF_SOC_CON6                            0x025c
>>> -#define GRF_EDP_LCD_SEL_MASK                    BIT(5)
>>> -#define GRF_EDP_SEL_VOP_LIT                     BIT(5)
>>> -#define GRF_EDP_SEL_VOP_BIG                     0
>>> +/**
>>> + * struct rockchip_dp_chip_data - splite the grf setting of kind of
>>> chips + * @lcdsel_grf_reg: grf register offset of lcdc select
>>> + * @lcdsel_big: reg value of selecting vop big for eDP
>>> + * @lcdsel_lit: reg value of selecting vop little for eDP
>>> + */
>>> +struct rockchip_dp_chip_data {
>>> +       u32     lcdsel_grf_reg;
>>> +       u32     lcdsel_big;
>>> +       u32     lcdsel_lit;
>>> +};
>>>
>>>   struct rockchip_dp_device {
>>>   
>>>          struct drm_device        *drm_dev;
>>>
>>> @@ -51,6 +58,8 @@ struct rockchip_dp_device {
>>>
>>>          struct regmap            *grf;
>>>          struct reset_control     *rst;
>>>
>>> +       const struct rockchip_dp_chip_data *data;
>>> +
>>>
>>>          struct analogix_dp_plat_data plat_data;
>>>   
>>>   };
>>>
>>> @@ -119,13 +128,13 @@ static void rockchip_dp_drm_encoder_enable(struct
>>> drm_encoder *encoder)>
>>>                  return;
>>>          
>>>          if (ret)
>>>
>>> -               val = GRF_EDP_SEL_VOP_LIT | (GRF_EDP_LCD_SEL_MASK <<
>>> 16);
>>> +               val = dp->data->lcdsel_lit;
>>>
>>>          else
>>>
>>> -               val = GRF_EDP_SEL_VOP_BIG | (GRF_EDP_LCD_SEL_MASK <<
>>> 16);
>>> +               val = dp->data->lcdsel_big;
>>>
>>>          dev_dbg(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" :
>>>          "BIG");
>>>
>>> -       ret = regmap_write(dp->grf, GRF_SOC_CON6, val);
>>> +       ret = regmap_write(dp->grf, dp->data->lcdsel_grf_reg, val);
>>>
>>>          if (ret != 0) {
>>>          
>>>                  dev_err(dp->dev, "Could not write to GRF: %d\n", ret);
>>>                  return;
>>>
>>> @@ -246,6 +255,7 @@ static int rockchip_dp_bind(struct device *dev,
>>> struct device *master,>
>>>                              void *data)
>>>   
>>>   {
>>>   
>>>          struct rockchip_dp_device *dp = dev_get_drvdata(dev);
>>>
>>> +       const struct rockchip_dp_chip_data *dp_data;
>>>
>>>          struct drm_device *drm_dev = data;
>>>          int ret;
>>>
>>> @@ -256,10 +266,15 @@ static int rockchip_dp_bind(struct device *dev,
>>> struct device *master,>
>>>           */
>>>          
>>>          dev_set_drvdata(dev, NULL);
>>>
>>> +       dp_data = of_device_get_match_data(dev);
>>> +       if (!dp_data)
>>> +               return -ENODEV;
>>> +
>>>
>>>          ret = rockchip_dp_init(dp);
>>>          if (ret < 0)
>>>          
>>>                  return ret;
>>>
>>> +       dp->data = dp_data;
>>>
>>>          dp->drm_dev = drm_dev;
>>>          
>>>          ret = rockchip_dp_drm_create_encoder(dp);
>>>
>>> @@ -365,8 +380,14 @@ static const struct dev_pm_ops rockchip_dp_pm_ops =
>>> {>
>>>          SET_SYSTEM_SLEEP_PM_OPS(rockchip_dp_suspend, rockchip_dp_resume)
>>>   
>>>   };
>>>
>>> +static const struct rockchip_dp_chip_data rk3288_dp = {
>>> +       .lcdsel_grf_reg = 0x025c,
>>> +       .lcdsel_big = 0 | BIT(21),
>>> +       .lcdsel_lit = BIT(5) | BIT(21),
>> I'm not sure what convention is in other drivers, but this seems less
>> readable to me.
>>
>> I'd prefer that the magic numbers were assigned to a #define that
>> corresponds to a TRM name in some header. Then you have names in the
>> assignment instead of meaningless numbers.
> I guess what irks me personally more is that it hides the write-mask as well
>
> So I guess something like the following would also define it somewhat future-
> proof
>
> #define RK3288_GRF_SOC_CON6	0x25c
> #define RK3288_EDP_LCDC_SEL		BIT(5)
>
> #define HIWORD_UPDATE(val, mask) \
> 		(val | (mask) << 16)
>
> static const struct rockchip_dp_chip_data rk3288_dp = {
> 	.lcdsel_grf_reg = RK3288_GRF_SOC_CON6,
> 	.lcdsel_big = HIWORD_UPDATE(0, RK3288_EDP_LCDC_SEL),
> 	.lcdsel_lit = HIWORD_UPDATE(RK3288_EDP_LCDC_SEL, RK3288_EDP_LCDC_SEL),
>

Ah, great, thanks for your code, this should be more readable now.

Thanks,
- Yakir

>
>

Re: [PATCH v3 03/10] drm/bridge: analogix_dp: correct the register bit define error in ANALOGIX_DP_PLL_REG_1

[Yakir Yang]

Sean,

On 06/23/2016 10:33 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> There're an register define error in ANALOGIX_DP_PLL_REG_1 which introduced
>> by commit bcec20fd5ad6 ("drm: bridge: analogix/dp: add some rk3288 special
>> registers setting").
>>
>> The PHY PLL input clock source is selected by ANALOGIX_DP_PLL_REG_1
>> BIT 0, not BIT 1.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Reviewed-by: Tomasz Figa <tomasz.figa@chromium.com>
>> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
> Reviewed-by: Sean Paul <seanpaul@chromium.org>

Thanks

>> ---
>> Changes in v3:
>> - Add reviewed flag from Tomasz.
>>      [https://chromium-review.googlesource.com/#/c/346315/15]
>> - Add tested flag from Javier
>>
>> Changes in v2: None
>>
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> index 337912b..88d56ad 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> @@ -163,8 +163,8 @@
>>   #define HSYNC_POLARITY_CFG                     (0x1 << 0)
>>
>>   /* ANALOGIX_DP_PLL_REG_1 */
>> -#define REF_CLK_24M                            (0x1 << 1)
>> -#define REF_CLK_27M                            (0x0 << 1)
>> +#define REF_CLK_24M                            (0x1 << 0)
>> +#define REF_CLK_27M                            (0x0 << 0)
>>
>>   /* ANALOGIX_DP_LANE_MAP */
>>   #define LANE3_MAP_LOGIC_LANE_0                 (0x0 << 6)
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH v3 04/10] drm/bridge: analogix_dp: some rockchip chips need to flip REF_CLK bit setting

[Yakir Yang]

Sean,

On 06/23/2016 09:27 PM, Sean Paul wrote:
> On Tue, Jun 14, 2016 at 7:46 AM, Yakir Yang <ykk@rock-chips.com> wrote:
>> As vendor document indicate, when REF_CLK bit set 0, then DP
>> phy's REF_CLK should switch to 24M source clock.
>>
>> But due to IC PHY layout mistaken, some chips need to flip this
>> bit(like RK3288), and unfortunately they didn't indicate in the
>> DP version register. That's why we have to make this little hack.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> Tested-by: Javier Martinez Canillas <javier@osg.samsung.com>
>> ---
>> Changes in v3:
>> - Make this hack code more clear (Tomasz, reviewed at Google Gerrit)
>>    reg = ~reg & REF_CLK_MASK;  --->  reg ^= REF_CLK_MASK;
>>      [https://chromium-review.googlesource.com/#/c/346852/7/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c@80]
>> - Add tested flag from Javier
>>
>> Changes in v2:
>> - new patch in v2
>>
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 6 +++++-
>>   drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 1 +
>>   drivers/gpu/drm/rockchip/analogix_dp-rockchip.c   | 3 +++
>>   include/drm/bridge/analogix_dp.h                  | 5 +++++
>>   4 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> index 931a76c..97ced6b 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>> @@ -75,7 +75,11 @@ void analogix_dp_init_analog_param(struct analogix_dp_device *dp)
>>          writel(reg, dp->reg_base + ANALOGIX_DP_ANALOG_CTL_2);
>>
>>          if (dp->plat_data && (dp->plat_data->dev_type == ROCKCHIP_DP)) {
>> -               writel(REF_CLK_24M, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
>> +               reg = REF_CLK_24M;
>> +               if (dp->plat_data->subdev_type == RK3288_DP)
>> +                       reg ^= REF_CLK_MASK;
>> +
>> +               writel(reg, dp->reg_base + ANALOGIX_DP_PLL_REG_1);
>>                  writel(0x95, dp->reg_base + ANALOGIX_DP_PLL_REG_2);
>>                  writel(0x40, dp->reg_base + ANALOGIX_DP_PLL_REG_3);
>>                  writel(0x58, dp->reg_base + ANALOGIX_DP_PLL_REG_4);
>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> index 88d56ad..cdcc6c5 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h
>> @@ -165,6 +165,7 @@
>>   /* ANALOGIX_DP_PLL_REG_1 */
>>   #define REF_CLK_24M                            (0x1 << 0)
>>   #define REF_CLK_27M                            (0x0 << 0)
>> +#define REF_CLK_MASK                           (0x1 << 0)
>>
>>   /* ANALOGIX_DP_LANE_MAP */
>>   #define LANE3_MAP_LOGIC_LANE_0                 (0x0 << 6)
>> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> index 3855f46..315ebba 100644
>> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
>> @@ -46,6 +46,7 @@ struct rockchip_dp_chip_data {
>>          u32     lcdsel_grf_reg;
>>          u32     lcdsel_big;
>>          u32     lcdsel_lit;
>> +       u32     chip_type;
>>   };
>>
>>   struct rockchip_dp_device {
>> @@ -286,6 +287,7 @@ static int rockchip_dp_bind(struct device *dev, struct device *master,
>>          dp->plat_data.encoder = &dp->encoder;
>>
>>          dp->plat_data.dev_type = ROCKCHIP_DP;
>> +       dp->plat_data.subdev_type = dp_data->chip_type;
>>          dp->plat_data.power_on = rockchip_dp_poweron;
>>          dp->plat_data.power_off = rockchip_dp_powerdown;
>>
>> @@ -384,6 +386,7 @@ static const struct rockchip_dp_chip_data rk3288_dp = {
>>          .lcdsel_grf_reg = 0x025c,
>>          .lcdsel_big = 0 | BIT(21),
>>          .lcdsel_lit = BIT(5) | BIT(21),
>> +       .chip_type = RK3288_DP,
>>   };
>>
>>   static const struct of_device_id rockchip_dp_dt_ids[] = {
>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>> index 9e5d013..06c0250 100644
>> --- a/include/drm/bridge/analogix_dp.h
>> +++ b/include/drm/bridge/analogix_dp.h
>> @@ -18,8 +18,13 @@ enum analogix_dp_devtype {
>>          ROCKCHIP_DP,
>>   };
>>
>> +enum analogix_dp_sub_devtype {
>> +       RK3288_DP,
>> +};
>> +
>>   struct analogix_dp_plat_data {
>>          enum analogix_dp_devtype dev_type;
>> +       enum analogix_dp_sub_devtype subdev_type;
>
> So this is what I was talking about in my review of the first patch of
> the series.
>
> I don't personally think the dev and subdev types add any clarity
> here, just more state. I'd prefer that you put the product number in
> the top level devtype, and add a helper function like:
>
> static bool is_rockchip(enum analogix_dp_devtype type) {
>    return type == ... || type == ...
> }

Good idea, done.

Thanks,
- Yakir

> Sean
>
>>          struct drm_panel *panel;
>>          struct drm_encoder *encoder;
>>          struct drm_connector *connector;
>> --
>> 1.9.1
>>
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>