[PATCH v2 0/6] drm/bridge: ti-sn65dsi86: Misc improvements

[PATCH v2 0/6] drm/bridge: ti-sn65dsi86: Misc improvements

[Laurent Pinchart]

Hello,

This patch series contains miscellaneous improvements to the
ti-sn65dsi86 driver, and prepares it for optional connector creation and
DisplayPort (non-eDP) support.

The patches have been posted previously as part of the "[RFC PATCH
00/11] drm/bridge: ti-sn65dsi86: Support DisplayPort mode" series. The
last four patches have been left out as discussions are ongoing, this
series focusses on the base work that has mostly been approved during
the review of the RFC.

The code has been rebased on top of the latest drm-misc-next, and while
some changes to the ti-sn65dsi86 driver made conflict resolution
painful in patch 5/6, there was no big functional conflict.

Laurent Pinchart (6):
  dt-bindings: drm/bridge: ti-sn65dsi8: Make enable GPIO optional
  drm/bridge: ti-sn65dsi86: Make enable GPIO optional
  drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates
  drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge
  drm/bridge: ti-sn65dsi86: Group code in sections
  drm/bridge: ti-sn65dsi86: Split connector creation to a function

 .../bindings/display/bridge/ti,sn65dsi86.yaml |   1 -
 drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 703 ++++++++++--------
 2 files changed, 374 insertions(+), 330 deletions(-)


base-commit: 7601d53c2c49e3a7e8150e8cf332b3c17943f75a
-- 
Regards,

Laurent Pinchart

[PATCH v2 1/6] dt-bindings: drm/bridge: ti-sn65dsi8: Make enable GPIO optional

[Laurent Pinchart]

The SN65DSI86 EN pin can be hardwired to a high level, or connected to a
global reset signal, not controllable by the kernel. Make it optional in
those cases.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/display/bridge/ti,sn65dsi86.yaml         | 1 -
 1 file changed, 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
index 12b876a20574..1c2daf7c24cc 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
@@ -153,7 +153,6 @@ properties:
 required:
   - compatible
   - reg
-  - enable-gpios
   - vccio-supply
   - vpll-supply
   - vcca-supply
-- 
Regards,

Laurent Pinchart

[PATCH v2 2/6] drm/bridge: ti-sn65dsi86: Make enable GPIO optional

[Laurent Pinchart]

The enable signal may not be controllable by the kernel. Make it
optional.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 5d712c8c3c3b..f0c7c6d4b2c1 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -1459,7 +1459,8 @@ static int ti_sn65dsi86_probe(struct i2c_client *client,
 		return dev_err_probe(dev, PTR_ERR(pdata->regmap),
 				     "regmap i2c init failed\n");
 
-	pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	pdata->enable_gpio = devm_gpiod_get_optional(dev, "enable",
+						     GPIOD_OUT_LOW);
 	if (IS_ERR(pdata->enable_gpio))
 		return dev_err_probe(dev, PTR_ERR(pdata->enable_gpio),
 				     "failed to get enable gpio from DT\n");
-- 
Regards,

Laurent Pinchart

[PATCH v2 3/6] drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates

[Laurent Pinchart]

The valid rates are stored in an array of 8 booleans. Replace it with a
bitmask to save space.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index f0c7c6d4b2c1..28c1ea370ae4 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -616,9 +616,9 @@ static int ti_sn_bridge_calc_min_dp_rate_idx(struct ti_sn65dsi86 *pdata)
 	return i;
 }
 
-static void ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata,
-					  bool rate_valid[])
+static unsigned int ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata)
 {
+	unsigned int valid_rates = 0;
 	unsigned int rate_per_200khz;
 	unsigned int rate_mhz;
 	u8 dpcd_val;
@@ -658,13 +658,13 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata,
 			     j < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 			     j++) {
 				if (ti_sn_bridge_dp_rate_lut[j] == rate_mhz)
-					rate_valid[j] = true;
+					valid_rates |= BIT(j);
 			}
 		}
 
 		for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut); i++) {
-			if (rate_valid[i])
-				return;
+			if (valid_rates & BIT(i))
+				return valid_rates;
 		}
 		DRM_DEV_ERROR(pdata->dev,
 			      "No matching eDP rates in table; falling back\n");
@@ -686,15 +686,17 @@ static void ti_sn_bridge_read_valid_rates(struct ti_sn65dsi86 *pdata,
 			      (int)dpcd_val);
 		fallthrough;
 	case DP_LINK_BW_5_4:
-		rate_valid[7] = 1;
+		valid_rates |= BIT(7);
 		fallthrough;
 	case DP_LINK_BW_2_7:
-		rate_valid[4] = 1;
+		valid_rates |= BIT(4);
 		fallthrough;
 	case DP_LINK_BW_1_62:
-		rate_valid[1] = 1;
+		valid_rates |= BIT(1);
 		break;
 	}
+
+	return valid_rates;
 }
 
 static void ti_sn_bridge_set_video_timings(struct ti_sn65dsi86 *pdata)
@@ -812,8 +814,8 @@ static int ti_sn_link_training(struct ti_sn65dsi86 *pdata, int dp_rate_idx,
 static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
-	bool rate_valid[ARRAY_SIZE(ti_sn_bridge_dp_rate_lut)] = { };
 	const char *last_err_str = "No supported DP rate";
+	unsigned int valid_rates;
 	int dp_rate_idx;
 	unsigned int val;
 	int ret = -EINVAL;
@@ -852,13 +854,13 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG, DP_NUM_LANES_MASK,
 			   val);
 
-	ti_sn_bridge_read_valid_rates(pdata, rate_valid);
+	valid_rates = ti_sn_bridge_read_valid_rates(pdata);
 
 	/* Train until we run out of rates */
 	for (dp_rate_idx = ti_sn_bridge_calc_min_dp_rate_idx(pdata);
 	     dp_rate_idx < ARRAY_SIZE(ti_sn_bridge_dp_rate_lut);
 	     dp_rate_idx++) {
-		if (!rate_valid[dp_rate_idx])
+		if (!(valid_rates & BIT(dp_rate_idx)))
 			continue;
 
 		ret = ti_sn_link_training(pdata, dp_rate_idx, &last_err_str);
-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Laurent Pinchart]

To simplify interfacing with the panel, wrap it in a panel-bridge and
let the DRM bridge helpers handle chaining of operations.

This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
requires all components in the display pipeline to be represented by
bridges.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes since v1:

- Drop error message when drm_bridge_attach() fails (now printed by the
  function)
- Return ERR_PTR() directly
- Clarify which bridge is the next bridge
- Drop ti_sn65dsi86.panel field
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 32 +++++++++++++++++----------
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 28c1ea370ae4..18b4420cc6b3 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -127,7 +127,7 @@
  * @host_node:    Remote DSI node.
  * @dsi:          Our MIPI DSI source.
  * @refclk:       Our reference clock.
- * @panel:        Our panel.
+ * @next_bridge:  The bridge on the eDP side.
  * @enable_gpio:  The GPIO we toggle to enable the bridge.
  * @supplies:     Data for bulk enabling/disabling our regulators.
  * @dp_lanes:     Count of dp_lanes we're using.
@@ -159,7 +159,7 @@ struct ti_sn65dsi86 {
 	struct device_node		*host_node;
 	struct mipi_dsi_device		*dsi;
 	struct clk			*refclk;
-	struct drm_panel		*panel;
+	struct drm_bridge		*next_bridge;
 	struct gpio_desc		*enable_gpio;
 	struct regulator_bulk_data	supplies[SN_REGULATOR_SUPPLY_NUM];
 	int				dp_lanes;
@@ -404,7 +404,8 @@ connector_to_ti_sn65dsi86(struct drm_connector *connector)
 static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
 {
 	struct ti_sn65dsi86 *pdata = connector_to_ti_sn65dsi86(connector);
-	return drm_panel_get_modes(pdata->panel, connector);
+
+	return drm_bridge_get_modes(pdata->next_bridge, connector);
 }
 
 static enum drm_mode_status
@@ -530,8 +531,16 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 	}
 	pdata->dsi = dsi;
 
+	/* Attach the next bridge */
+	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
+				&pdata->bridge, flags);
+	if (ret < 0)
+		goto err_dsi_detach;
+
 	return 0;
 
+err_dsi_detach:
+	mipi_dsi_detach(dsi);
 err_dsi_attach:
 	mipi_dsi_device_unregister(dsi);
 err_dsi_host:
@@ -550,8 +559,6 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
-	drm_panel_disable(pdata->panel);
-
 	/* disable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE, 0);
 	/* semi auto link training mode OFF */
@@ -878,8 +885,6 @@ static void ti_sn_bridge_enable(struct drm_bridge *bridge)
 	/* enable video stream */
 	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG, VSTREAM_ENABLE,
 			   VSTREAM_ENABLE);
-
-	drm_panel_enable(pdata->panel);
 }
 
 static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
@@ -890,16 +895,12 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
 
 	if (!pdata->refclk)
 		ti_sn65dsi86_enable_comms(pdata);
-
-	drm_panel_prepare(pdata->panel);
 }
 
 static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
 {
 	struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
 
-	drm_panel_unprepare(pdata->panel);
-
 	if (!pdata->refclk)
 		ti_sn65dsi86_disable_comms(pdata);
 
@@ -1304,13 +1305,20 @@ static int ti_sn_bridge_probe(struct auxiliary_device *adev,
 {
 	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
 	struct device_node *np = pdata->dev->of_node;
+	struct drm_panel *panel;
 	int ret;
 
-	ret = drm_of_find_panel_or_bridge(np, 1, 0, &pdata->panel, NULL);
+	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
 	if (ret)
 		return dev_err_probe(&adev->dev, ret,
 				     "could not find any panel node\n");
 
+	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
+	if (IS_ERR(pdata->next_bridge)) {
+		DRM_ERROR("failed to create panel bridge\n");
+		return PTR_ERR(pdata->next_bridge);
+	}
+
 	ti_sn_bridge_parse_lanes(pdata, np);
 
 	ret = ti_sn_bridge_parse_dsi_host(pdata);
-- 
Regards,

Laurent Pinchart

[PATCH v2 5/6] drm/bridge: ti-sn65dsi86: Group code in sections

[Laurent Pinchart]

Reorganize the functions in sections, related to connector operations,
bridge operations, AUX adapter, GPIO controller and probe & remove.

This prepares for proper support of DRM_BRIDGE_ATTACH_NO_CONNECTOR that
will add more functions, to ensure that the code will stay readable.

No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 629 +++++++++++++-------------
 1 file changed, 326 insertions(+), 303 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index 18b4420cc6b3..feeda88c4ef0 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -394,7 +394,211 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata)
 	debugfs_create_file("status", 0600, debugfs, pdata, &status_fops);
 }
 
-/* Connector funcs */
+/* -----------------------------------------------------------------------------
+ * Auxiliary Devices (*not* AUX)
+ */
+
+static void ti_sn65dsi86_uninit_aux(void *data)
+{
+	auxiliary_device_uninit(data);
+}
+
+static void ti_sn65dsi86_delete_aux(void *data)
+{
+	auxiliary_device_delete(data);
+}
+
+/*
+ * AUX bus docs say that a non-NULL release is mandatory, but it makes no
+ * sense for the model used here where all of the aux devices are allocated
+ * in the single shared structure. We'll use this noop as a workaround.
+ */
+static void ti_sn65dsi86_noop(struct device *dev) {}
+
+static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
+				       struct auxiliary_device *aux,
+				       const char *name)
+{
+	struct device *dev = pdata->dev;
+	int ret;
+
+	aux->name = name;
+	aux->dev.parent = dev;
+	aux->dev.release = ti_sn65dsi86_noop;
+	device_set_of_node_from_dev(&aux->dev, dev);
+	ret = auxiliary_device_init(aux);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux);
+	if (ret)
+		return ret;
+
+	ret = auxiliary_device_add(aux);
+	if (ret)
+		return ret;
+	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux);
+
+	return ret;
+}
+
+/* -----------------------------------------------------------------------------
+ * AUX Adapter
+ */
+
+static struct ti_sn65dsi86 *aux_to_ti_sn65dsi86(struct drm_dp_aux *aux)
+{
+	return container_of(aux, struct ti_sn65dsi86, aux);
+}
+
+static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
+				  struct drm_dp_aux_msg *msg)
+{
+	struct ti_sn65dsi86 *pdata = aux_to_ti_sn65dsi86(aux);
+	u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
+	u32 request_val = AUX_CMD_REQ(msg->request);
+	u8 *buf = msg->buffer;
+	unsigned int len = msg->size;
+	unsigned int val;
+	int ret;
+	u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
+
+	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
+		return -EINVAL;
+
+	pm_runtime_get_sync(pdata->dev);
+	mutex_lock(&pdata->comms_mutex);
+
+	/*
+	 * If someone tries to do a DDC over AUX transaction before pre_enable()
+	 * on a device without a dedicated reference clock then we just can't
+	 * do it. Fail right away. This prevents non-refclk users from reading
+	 * the EDID before enabling the panel but such is life.
+	 */
+	if (!pdata->comms_enabled) {
+		ret = -EIO;
+		goto exit;
+	}
+
+	switch (request) {
+	case DP_AUX_NATIVE_WRITE:
+	case DP_AUX_I2C_WRITE:
+	case DP_AUX_NATIVE_READ:
+	case DP_AUX_I2C_READ:
+		regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
+		/* Assume it's good */
+		msg->reply = 0;
+		break;
+	default:
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
+	put_unaligned_be32((msg->address & SN_AUX_ADDR_MASK) << 8 | len,
+			   addr_len);
+	regmap_bulk_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, addr_len,
+			  ARRAY_SIZE(addr_len));
+
+	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
+		regmap_bulk_write(pdata->regmap, SN_AUX_WDATA_REG(0), buf, len);
+
+	/* Clear old status bits before start so we don't get confused */
+	regmap_write(pdata->regmap, SN_AUX_CMD_STATUS_REG,
+		     AUX_IRQ_STATUS_NAT_I2C_FAIL |
+		     AUX_IRQ_STATUS_AUX_RPLY_TOUT |
+		     AUX_IRQ_STATUS_AUX_SHORT);
+
+	regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val | AUX_CMD_SEND);
+
+	/* Zero delay loop because i2c transactions are slow already */
+	ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
+				       !(val & AUX_CMD_SEND), 0, 50 * 1000);
+	if (ret)
+		goto exit;
+
+	ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
+	if (ret)
+		goto exit;
+
+	if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
+		/*
+		 * The hardware tried the message seven times per the DP spec
+		 * but it hit a timeout. We ignore defers here because they're
+		 * handled in hardware.
+		 */
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
+	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
+		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
+		if (ret)
+			goto exit;
+	} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
+		switch (request) {
+		case DP_AUX_I2C_WRITE:
+		case DP_AUX_I2C_READ:
+			msg->reply |= DP_AUX_I2C_REPLY_NACK;
+			break;
+		case DP_AUX_NATIVE_READ:
+		case DP_AUX_NATIVE_WRITE:
+			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
+			break;
+		}
+		len = 0;
+		goto exit;
+	}
+
+	if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0)
+		ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
+
+exit:
+	mutex_unlock(&pdata->comms_mutex);
+	pm_runtime_mark_last_busy(pdata->dev);
+	pm_runtime_put_autosuspend(pdata->dev);
+
+	if (ret)
+		return ret;
+	return len;
+}
+
+static int ti_sn_aux_probe(struct auxiliary_device *adev,
+			   const struct auxiliary_device_id *id)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+	int ret;
+
+	pdata->aux.name = "ti-sn65dsi86-aux";
+	pdata->aux.dev = &adev->dev;
+	pdata->aux.transfer = ti_sn_aux_transfer;
+	drm_dp_aux_init(&pdata->aux);
+
+	ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
+	if (ret)
+		return ret;
+
+	/*
+	 * The eDP to MIPI bridge parts don't work until the AUX channel is
+	 * setup so we don't add it in the main driver probe, we add it now.
+	 */
+	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
+}
+
+static const struct auxiliary_device_id ti_sn_aux_id_table[] = {
+	{ .name = "ti_sn65dsi86.aux", },
+	{},
+};
+
+static struct auxiliary_driver ti_sn_aux_driver = {
+	.name = "aux",
+	.probe = ti_sn_aux_probe,
+	.id_table = ti_sn_aux_id_table,
+};
+
+/* -----------------------------------------------------------------------------
+ * DRM Connector Operations
+ */
+
 static struct ti_sn65dsi86 *
 connector_to_ti_sn65dsi86(struct drm_connector *connector)
 {
@@ -432,25 +636,15 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+/*------------------------------------------------------------------------------
+ * DRM Bridge
+ */
+
 static struct ti_sn65dsi86 *bridge_to_ti_sn65dsi86(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct ti_sn65dsi86, bridge);
 }
 
-static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
-{
-	unsigned int i;
-	const char * const ti_sn_bridge_supply_names[] = {
-		"vcca", "vcc", "vccio", "vpll",
-	};
-
-	for (i = 0; i < SN_REGULATOR_SUPPLY_NUM; i++)
-		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
-
-	return devm_regulator_bulk_get(pdata->dev, SN_REGULATOR_SUPPLY_NUM,
-				       pdata->supplies);
-}
-
 static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 			       enum drm_bridge_attach_flags flags)
 {
@@ -916,121 +1110,53 @@ static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
 	.post_disable = ti_sn_bridge_post_disable,
 };
 
-static struct ti_sn65dsi86 *aux_to_ti_sn65dsi86(struct drm_dp_aux *aux)
+static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
+				     struct device_node *np)
 {
-	return container_of(aux, struct ti_sn65dsi86, aux);
-}
-
-static ssize_t ti_sn_aux_transfer(struct drm_dp_aux *aux,
-				  struct drm_dp_aux_msg *msg)
-{
-	struct ti_sn65dsi86 *pdata = aux_to_ti_sn65dsi86(aux);
-	u32 request = msg->request & ~(DP_AUX_I2C_MOT | DP_AUX_I2C_WRITE_STATUS_UPDATE);
-	u32 request_val = AUX_CMD_REQ(msg->request);
-	u8 *buf = msg->buffer;
-	unsigned int len = msg->size;
-	unsigned int val;
-	int ret;
-	u8 addr_len[SN_AUX_LENGTH_REG + 1 - SN_AUX_ADDR_19_16_REG];
-
-	if (len > SN_AUX_MAX_PAYLOAD_BYTES)
-		return -EINVAL;
-
-	pm_runtime_get_sync(pdata->dev);
-	mutex_lock(&pdata->comms_mutex);
+	u32 lane_assignments[SN_MAX_DP_LANES] = { 0, 1, 2, 3 };
+	u32 lane_polarities[SN_MAX_DP_LANES] = { };
+	struct device_node *endpoint;
+	u8 ln_assign = 0;
+	u8 ln_polrs = 0;
+	int dp_lanes;
+	int i;
 
 	/*
-	 * If someone tries to do a DDC over AUX transaction before pre_enable()
-	 * on a device without a dedicated reference clock then we just can't
-	 * do it. Fail right away. This prevents non-refclk users from reading
-	 * the EDID before enabling the panel but such is life.
+	 * Read config from the device tree about lane remapping and lane
+	 * polarities.  These are optional and we assume identity map and
+	 * normal polarity if nothing is specified.  It's OK to specify just
+	 * data-lanes but not lane-polarities but not vice versa.
+	 *
+	 * Error checking is light (we just make sure we don't crash or
+	 * buffer overrun) and we assume dts is well formed and specifying
+	 * mappings that the hardware supports.
 	 */
-	if (!pdata->comms_enabled) {
-		ret = -EIO;
-		goto exit;
-	}
-
-	switch (request) {
-	case DP_AUX_NATIVE_WRITE:
-	case DP_AUX_I2C_WRITE:
-	case DP_AUX_NATIVE_READ:
-	case DP_AUX_I2C_READ:
-		regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val);
-		/* Assume it's good */
-		msg->reply = 0;
-		break;
-	default:
-		ret = -EINVAL;
-		goto exit;
-	}
-
-	BUILD_BUG_ON(sizeof(addr_len) != sizeof(__be32));
-	put_unaligned_be32((msg->address & SN_AUX_ADDR_MASK) << 8 | len,
-			   addr_len);
-	regmap_bulk_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, addr_len,
-			  ARRAY_SIZE(addr_len));
-
-	if (request == DP_AUX_NATIVE_WRITE || request == DP_AUX_I2C_WRITE)
-		regmap_bulk_write(pdata->regmap, SN_AUX_WDATA_REG(0), buf, len);
-
-	/* Clear old status bits before start so we don't get confused */
-	regmap_write(pdata->regmap, SN_AUX_CMD_STATUS_REG,
-		     AUX_IRQ_STATUS_NAT_I2C_FAIL |
-		     AUX_IRQ_STATUS_AUX_RPLY_TOUT |
-		     AUX_IRQ_STATUS_AUX_SHORT);
-
-	regmap_write(pdata->regmap, SN_AUX_CMD_REG, request_val | AUX_CMD_SEND);
-
-	/* Zero delay loop because i2c transactions are slow already */
-	ret = regmap_read_poll_timeout(pdata->regmap, SN_AUX_CMD_REG, val,
-				       !(val & AUX_CMD_SEND), 0, 50 * 1000);
-	if (ret)
-		goto exit;
-
-	ret = regmap_read(pdata->regmap, SN_AUX_CMD_STATUS_REG, &val);
-	if (ret)
-		goto exit;
-
-	if (val & AUX_IRQ_STATUS_AUX_RPLY_TOUT) {
-		/*
-		 * The hardware tried the message seven times per the DP spec
-		 * but it hit a timeout. We ignore defers here because they're
-		 * handled in hardware.
-		 */
-		ret = -ETIMEDOUT;
-		goto exit;
+	endpoint = of_graph_get_endpoint_by_regs(np, 1, -1);
+	dp_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
+	if (dp_lanes > 0 && dp_lanes <= SN_MAX_DP_LANES) {
+		of_property_read_u32_array(endpoint, "data-lanes",
+					   lane_assignments, dp_lanes);
+		of_property_read_u32_array(endpoint, "lane-polarities",
+					   lane_polarities, dp_lanes);
+	} else {
+		dp_lanes = SN_MAX_DP_LANES;
 	}
+	of_node_put(endpoint);
 
-	if (val & AUX_IRQ_STATUS_AUX_SHORT) {
-		ret = regmap_read(pdata->regmap, SN_AUX_LENGTH_REG, &len);
-		if (ret)
-			goto exit;
-	} else if (val & AUX_IRQ_STATUS_NAT_I2C_FAIL) {
-		switch (request) {
-		case DP_AUX_I2C_WRITE:
-		case DP_AUX_I2C_READ:
-			msg->reply |= DP_AUX_I2C_REPLY_NACK;
-			break;
-		case DP_AUX_NATIVE_READ:
-		case DP_AUX_NATIVE_WRITE:
-			msg->reply |= DP_AUX_NATIVE_REPLY_NACK;
-			break;
-		}
-		len = 0;
-		goto exit;
+	/*
+	 * Convert into register format.  Loop over all lanes even if
+	 * data-lanes had fewer elements so that we nicely initialize
+	 * the LN_ASSIGN register.
+	 */
+	for (i = SN_MAX_DP_LANES - 1; i >= 0; i--) {
+		ln_assign = ln_assign << LN_ASSIGN_WIDTH | lane_assignments[i];
+		ln_polrs = ln_polrs << 1 | lane_polarities[i];
 	}
 
-	if (request != DP_AUX_NATIVE_WRITE && request != DP_AUX_I2C_WRITE && len != 0)
-		ret = regmap_bulk_read(pdata->regmap, SN_AUX_RDATA_REG(0), buf, len);
-
-exit:
-	mutex_unlock(&pdata->comms_mutex);
-	pm_runtime_mark_last_busy(pdata->dev);
-	pm_runtime_put_autosuspend(pdata->dev);
-
-	if (ret)
-		return ret;
-	return len;
+	/* Stash in our struct for when we power on */
+	pdata->dp_lanes = dp_lanes;
+	pdata->ln_assign = ln_assign;
+	pdata->ln_polrs = ln_polrs;
 }
 
 static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
@@ -1047,6 +1173,72 @@ static int ti_sn_bridge_parse_dsi_host(struct ti_sn65dsi86 *pdata)
 	return 0;
 }
 
+static int ti_sn_bridge_probe(struct auxiliary_device *adev,
+			      const struct auxiliary_device_id *id)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+	struct device_node *np = pdata->dev->of_node;
+	struct drm_panel *panel;
+	int ret;
+
+	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
+	if (ret)
+		return dev_err_probe(&adev->dev, ret,
+				     "could not find any panel node\n");
+
+	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
+	if (IS_ERR(pdata->next_bridge)) {
+		DRM_ERROR("failed to create panel bridge\n");
+		return PTR_ERR(pdata->next_bridge);
+	}
+
+	ti_sn_bridge_parse_lanes(pdata, np);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret)
+		return ret;
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = np;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return 0;
+}
+
+static void ti_sn_bridge_remove(struct auxiliary_device *adev)
+{
+	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
+
+	if (!pdata)
+		return;
+
+	if (pdata->dsi) {
+		mipi_dsi_detach(pdata->dsi);
+		mipi_dsi_device_unregister(pdata->dsi);
+	}
+
+	drm_bridge_remove(&pdata->bridge);
+
+	of_node_put(pdata->host_node);
+}
+
+static const struct auxiliary_device_id ti_sn_bridge_id_table[] = {
+	{ .name = "ti_sn65dsi86.bridge", },
+	{},
+};
+
+static struct auxiliary_driver ti_sn_bridge_driver = {
+	.name = "bridge",
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id_table,
+};
+
+/* -----------------------------------------------------------------------------
+ * GPIO Controller
+ */
+
 #if defined(CONFIG_OF_GPIO)
 
 static int tn_sn_bridge_of_xlate(struct gpio_chip *chip,
@@ -1251,198 +1443,29 @@ static inline void ti_sn_gpio_unregister(void) {}
 
 #endif
 
-static void ti_sn_bridge_parse_lanes(struct ti_sn65dsi86 *pdata,
-				     struct device_node *np)
-{
-	u32 lane_assignments[SN_MAX_DP_LANES] = { 0, 1, 2, 3 };
-	u32 lane_polarities[SN_MAX_DP_LANES] = { };
-	struct device_node *endpoint;
-	u8 ln_assign = 0;
-	u8 ln_polrs = 0;
-	int dp_lanes;
-	int i;
-
-	/*
-	 * Read config from the device tree about lane remapping and lane
-	 * polarities.  These are optional and we assume identity map and
-	 * normal polarity if nothing is specified.  It's OK to specify just
-	 * data-lanes but not lane-polarities but not vice versa.
-	 *
-	 * Error checking is light (we just make sure we don't crash or
-	 * buffer overrun) and we assume dts is well formed and specifying
-	 * mappings that the hardware supports.
-	 */
-	endpoint = of_graph_get_endpoint_by_regs(np, 1, -1);
-	dp_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
-	if (dp_lanes > 0 && dp_lanes <= SN_MAX_DP_LANES) {
-		of_property_read_u32_array(endpoint, "data-lanes",
-					   lane_assignments, dp_lanes);
-		of_property_read_u32_array(endpoint, "lane-polarities",
-					   lane_polarities, dp_lanes);
-	} else {
-		dp_lanes = SN_MAX_DP_LANES;
-	}
-	of_node_put(endpoint);
-
-	/*
-	 * Convert into register format.  Loop over all lanes even if
-	 * data-lanes had fewer elements so that we nicely initialize
-	 * the LN_ASSIGN register.
-	 */
-	for (i = SN_MAX_DP_LANES - 1; i >= 0; i--) {
-		ln_assign = ln_assign << LN_ASSIGN_WIDTH | lane_assignments[i];
-		ln_polrs = ln_polrs << 1 | lane_polarities[i];
-	}
-
-	/* Stash in our struct for when we power on */
-	pdata->dp_lanes = dp_lanes;
-	pdata->ln_assign = ln_assign;
-	pdata->ln_polrs = ln_polrs;
-}
-
-static int ti_sn_bridge_probe(struct auxiliary_device *adev,
-			      const struct auxiliary_device_id *id)
-{
-	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
-	struct device_node *np = pdata->dev->of_node;
-	struct drm_panel *panel;
-	int ret;
-
-	ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, NULL);
-	if (ret)
-		return dev_err_probe(&adev->dev, ret,
-				     "could not find any panel node\n");
-
-	pdata->next_bridge = devm_drm_panel_bridge_add(pdata->dev, panel);
-	if (IS_ERR(pdata->next_bridge)) {
-		DRM_ERROR("failed to create panel bridge\n");
-		return PTR_ERR(pdata->next_bridge);
-	}
-
-	ti_sn_bridge_parse_lanes(pdata, np);
-
-	ret = ti_sn_bridge_parse_dsi_host(pdata);
-	if (ret)
-		return ret;
-
-	pdata->bridge.funcs = &ti_sn_bridge_funcs;
-	pdata->bridge.of_node = np;
-
-	drm_bridge_add(&pdata->bridge);
-
-	return 0;
-}
-
-static void ti_sn_bridge_remove(struct auxiliary_device *adev)
-{
-	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
-
-	if (!pdata)
-		return;
-
-	if (pdata->dsi) {
-		mipi_dsi_detach(pdata->dsi);
-		mipi_dsi_device_unregister(pdata->dsi);
-	}
-
-	drm_bridge_remove(&pdata->bridge);
-
-	of_node_put(pdata->host_node);
-}
-
-static const struct auxiliary_device_id ti_sn_bridge_id_table[] = {
-	{ .name = "ti_sn65dsi86.bridge", },
-	{},
-};
-
-static struct auxiliary_driver ti_sn_bridge_driver = {
-	.name = "bridge",
-	.probe = ti_sn_bridge_probe,
-	.remove = ti_sn_bridge_remove,
-	.id_table = ti_sn_bridge_id_table,
-};
+/* -----------------------------------------------------------------------------
+ * Probe & Remove
+ */
 
 static void ti_sn65dsi86_runtime_disable(void *data)
 {
 	pm_runtime_disable(data);
 }
 
-static void ti_sn65dsi86_uninit_aux(void *data)
+static int ti_sn65dsi86_parse_regulators(struct ti_sn65dsi86 *pdata)
 {
-	auxiliary_device_uninit(data);
-}
-
-static void ti_sn65dsi86_delete_aux(void *data)
-{
-	auxiliary_device_delete(data);
-}
-
-/*
- * AUX bus docs say that a non-NULL release is mandatory, but it makes no
- * sense for the model used here where all of the aux devices are allocated
- * in the single shared structure. We'll use this noop as a workaround.
- */
-static void ti_sn65dsi86_noop(struct device *dev) {}
-
-static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata,
-				       struct auxiliary_device *aux,
-				       const char *name)
-{
-	struct device *dev = pdata->dev;
-	int ret;
-
-	aux->name = name;
-	aux->dev.parent = dev;
-	aux->dev.release = ti_sn65dsi86_noop;
-	device_set_of_node_from_dev(&aux->dev, dev);
-	ret = auxiliary_device_init(aux);
-	if (ret)
-		return ret;
-	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux);
-	if (ret)
-		return ret;
+	unsigned int i;
+	const char * const ti_sn_bridge_supply_names[] = {
+		"vcca", "vcc", "vccio", "vpll",
+	};
 
-	ret = auxiliary_device_add(aux);
-	if (ret)
-		return ret;
-	ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux);
+	for (i = 0; i < SN_REGULATOR_SUPPLY_NUM; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
 
-	return ret;
+	return devm_regulator_bulk_get(pdata->dev, SN_REGULATOR_SUPPLY_NUM,
+				       pdata->supplies);
 }
 
-static int ti_sn_aux_probe(struct auxiliary_device *adev,
-			   const struct auxiliary_device_id *id)
-{
-	struct ti_sn65dsi86 *pdata = dev_get_drvdata(adev->dev.parent);
-	int ret;
-
-	pdata->aux.name = "ti-sn65dsi86-aux";
-	pdata->aux.dev = &adev->dev;
-	pdata->aux.transfer = ti_sn_aux_transfer;
-	drm_dp_aux_init(&pdata->aux);
-
-	ret = devm_of_dp_aux_populate_ep_devices(&pdata->aux);
-	if (ret)
-		return ret;
-
-	/*
-	 * The eDP to MIPI bridge parts don't work until the AUX channel is
-	 * setup so we don't add it in the main driver probe, we add it now.
-	 */
-	return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge");
-}
-
-static const struct auxiliary_device_id ti_sn_aux_id_table[] = {
-	{ .name = "ti_sn65dsi86.aux", },
-	{},
-};
-
-static struct auxiliary_driver ti_sn_aux_driver = {
-	.name = "aux",
-	.probe = ti_sn_aux_probe,
-	.id_table = ti_sn_aux_id_table,
-};
-
 static int ti_sn65dsi86_probe(struct i2c_client *client,
 			      const struct i2c_device_id *id)
 {
-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] drm/bridge: ti-sn65dsi86: Split connector creation to a function

[Laurent Pinchart]

To prepare for making connector creation option, move connector creation
out of ti_sn_bridge_attach to a separate function.

No functional change intended.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 31 ++++++++++++++++++---------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index feeda88c4ef0..9bf889302bcc 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -636,6 +636,25 @@ static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static int ti_sn_bridge_connector_init(struct ti_sn65dsi86 *pdata)
+{
+	int ret;
+
+	ret = drm_connector_init(pdata->bridge.dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_connector_attach_encoder(&pdata->connector, pdata->bridge.encoder);
+
+	return 0;
+}
+
 /*------------------------------------------------------------------------------
  * DRM Bridge
  */
@@ -669,17 +688,9 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
 		return ret;
 	}
 
-	ret = drm_connector_init(bridge->dev, &pdata->connector,
-				 &ti_sn_bridge_connector_funcs,
-				 DRM_MODE_CONNECTOR_eDP);
-	if (ret) {
-		DRM_ERROR("Failed to initialize connector with drm\n");
+	ret = ti_sn_bridge_connector_init(pdata);
+	if (ret < 0)
 		goto err_conn_init;
-	}
-
-	drm_connector_helper_add(&pdata->connector,
-				 &ti_sn_bridge_connector_helper_funcs);
-	drm_connector_attach_encoder(&pdata->connector, bridge->encoder);
 
 	/*
 	 * TODO: ideally finding host resource and dsi dev registration needs
-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 0/6] drm/bridge: ti-sn65dsi86: Misc improvements

[Robert Foss]

Applied to drm-misc-next

https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0715786771f24190b3f2dcdcaf2af263c1ef46eb

On Thu, 24 Jun 2021 at 02:03, Laurent Pinchart
<laurent.pinchart+renesas@ideasonboard.com> wrote:
>
> Hello,
>
> This patch series contains miscellaneous improvements to the
> ti-sn65dsi86 driver, and prepares it for optional connector creation and
> DisplayPort (non-eDP) support.
>
> The patches have been posted previously as part of the "[RFC PATCH
> 00/11] drm/bridge: ti-sn65dsi86: Support DisplayPort mode" series. The
> last four patches have been left out as discussions are ongoing, this
> series focusses on the base work that has mostly been approved during
> the review of the RFC.
>
> The code has been rebased on top of the latest drm-misc-next, and while
> some changes to the ti-sn65dsi86 driver made conflict resolution
> painful in patch 5/6, there was no big functional conflict.
>
> Laurent Pinchart (6):
>   dt-bindings: drm/bridge: ti-sn65dsi8: Make enable GPIO optional
>   drm/bridge: ti-sn65dsi86: Make enable GPIO optional
>   drm/bridge: ti-sn65dsi86: Use bitmask to store valid rates
>   drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge
>   drm/bridge: ti-sn65dsi86: Group code in sections
>   drm/bridge: ti-sn65dsi86: Split connector creation to a function
>
>  .../bindings/display/bridge/ti,sn65dsi86.yaml |   1 -
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 703 ++++++++++--------
>  2 files changed, 374 insertions(+), 330 deletions(-)
>
>
> base-commit: 7601d53c2c49e3a7e8150e8cf332b3c17943f75a
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Stephen Boyd]

Quoting Laurent Pinchart (2021-06-23 17:03:02)
> To simplify interfacing with the panel, wrap it in a panel-bridge and
> let the DRM bridge helpers handle chaining of operations.
>
> This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> requires all components in the display pipeline to be represented by
> bridges.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> ---

With this patch applied I get two eDP devices on Lazor sc7180 (it is the
arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
looking for more info). As far as I can tell, we should only have one
eDP device on the board, for the bridge.

localhost ~ # ls -l /sys/class/drm/card1-eDP*
lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Laurent Pinchart]

Hi Stephen,

On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > let the DRM bridge helpers handle chaining of operations.
> >
> > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > requires all components in the display pipeline to be represented by
> > bridges.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> 
> With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> looking for more info). As far as I can tell, we should only have one
> eDP device on the board, for the bridge.
> 
> localhost ~ # ls -l /sys/class/drm/card1-eDP*
> lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2

Indeed.

Does the display driver use the DRM connector bridge helper and
DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Rob Clark]

On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Stephen,
>
> On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > let the DRM bridge helpers handle chaining of operations.
> > >
> > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > requires all components in the display pipeline to be represented by
> > > bridges.
> > >
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> >
> > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > looking for more info). As far as I can tell, we should only have one
> > eDP device on the board, for the bridge.
> >
> > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
>
> Indeed.
>
> Does the display driver use the DRM connector bridge helper and
> DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
>

There haven't been any recent changes about how we attach the bridge,
it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
having time to follow too closely the recent changes with bridge stuff
myself.

But now with this patch we have both the ti bridge and the panel
bridge creating a connector..  removing the connector created by the
ti bridge "fixes" things, but not sure if that would break something
on other platforms.  I guess there should now always be a panel
bridge, so removing ti_sn_bridge_connector_init() would be a sane
thing to do?

BR,
-R

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Stephen Boyd]

Quoting Rob Clark (2021-08-11 09:20:30)
> On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Stephen,
> >
> > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > let the DRM bridge helpers handle chaining of operations.
> > > >
> > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > requires all components in the display pipeline to be represented by
> > > > bridges.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > >
> > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > looking for more info). As far as I can tell, we should only have one
> > > eDP device on the board, for the bridge.
> > >
> > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> >
> > Indeed.
> >
> > Does the display driver use the DRM connector bridge helper and
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> >
>
> There haven't been any recent changes about how we attach the bridge,
> it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> having time to follow too closely the recent changes with bridge stuff
> myself.
>
> But now with this patch we have both the ti bridge and the panel
> bridge creating a connector..  removing the connector created by the
> ti bridge "fixes" things, but not sure if that would break something
> on other platforms.  I guess there should now always be a panel
> bridge, so removing ti_sn_bridge_connector_init() would be a sane
> thing to do?
>

So this patch works. We don't want to make the connector in this driver
for the next bridge because this driver is making the connector. I guess
eventually we'll drop this flag when this driver stops making the
connector here?

---8<---
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
index cd0fccdd8dfd..a8d4818484aa 100644
--- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,

 	/* Attach the next bridge */
 	ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
-				&pdata->bridge, flags);
+				&pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
 	if (ret < 0)
 		goto err_dsi_detach;

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Rob Clark]

On Wed, Aug 11, 2021 at 1:39 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Quoting Rob Clark (2021-08-11 09:20:30)
> > On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > >
> > > Hi Stephen,
> > >
> > > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > > let the DRM bridge helpers handle chaining of operations.
> > > > >
> > > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > > requires all components in the display pipeline to be represented by
> > > > > bridges.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > >
> > > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > > looking for more info). As far as I can tell, we should only have one
> > > > eDP device on the board, for the bridge.
> > > >
> > > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> > >
> > > Indeed.
> > >
> > > Does the display driver use the DRM connector bridge helper and
> > > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> > >
> >
> > There haven't been any recent changes about how we attach the bridge,
> > it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> > having time to follow too closely the recent changes with bridge stuff
> > myself.
> >
> > But now with this patch we have both the ti bridge and the panel
> > bridge creating a connector..  removing the connector created by the
> > ti bridge "fixes" things, but not sure if that would break something
> > on other platforms.  I guess there should now always be a panel
> > bridge, so removing ti_sn_bridge_connector_init() would be a sane
> > thing to do?
> >
>
> So this patch works. We don't want to make the connector in this driver
> for the next bridge because this driver is making the connector. I guess
> eventually we'll drop this flag when this driver stops making the
> connector here?
>
> ---8<---
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> index cd0fccdd8dfd..a8d4818484aa 100644
> --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
>
>         /* Attach the next bridge */
>         ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> -                               &pdata->bridge, flags);
> +                               &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>         if (ret < 0)
>                 goto err_dsi_detach;


I kinda think *all* bridges that create a connector (whether optional
or not) should OR in NO_CONNECTOR when attaching the next downstream
bridge.. since you never want multiple connectors

BR,
-R

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Laurent Pinchart]

On Wed, Aug 11, 2021 at 01:51:28PM -0700, Rob Clark wrote:
> On Wed, Aug 11, 2021 at 1:39 PM Stephen Boyd wrote:
> > Quoting Rob Clark (2021-08-11 09:20:30)
> > > On Wed, Aug 11, 2021 at 5:15 AM Laurent Pinchart wrote:
> > > > On Tue, Aug 10, 2021 at 10:26:33PM -0700, Stephen Boyd wrote:
> > > > > Quoting Laurent Pinchart (2021-06-23 17:03:02)
> > > > > > To simplify interfacing with the panel, wrap it in a panel-bridge and
> > > > > > let the DRM bridge helpers handle chaining of operations.
> > > > > >
> > > > > > This also prepares for support of DRM_BRIDGE_ATTACH_NO_CONNECTOR, which
> > > > > > requires all components in the display pipeline to be represented by
> > > > > > bridges.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > > > > > Reviewed-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > >
> > > > > With this patch applied I get two eDP devices on Lazor sc7180 (it is the
> > > > > arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor*.dts files if you're
> > > > > looking for more info). As far as I can tell, we should only have one
> > > > > eDP device on the board, for the bridge.
> > > > >
> > > > > localhost ~ # ls -l /sys/class/drm/card1-eDP*
> > > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-1 ->
> > > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-1
> > > > > lrwxrwxrwx. 1 root root 0 Aug 10 22:24 /sys/class/drm/card1-eDP-2 ->
> > > > > ../../devices/platform/soc@0/ae00000.mdss/drm/card1/card1-eDP-2
> > > >
> > > > Indeed.
> > > >
> > > > Does the display driver use the DRM connector bridge helper and
> > > > DRM_BRIDGE_ATTACH_NO_CONNECTOR on that platform ?
> > >
> > > There haven't been any recent changes about how we attach the bridge,
> > > it doesn't pass DRM_BRIDGE_ATTACH_NO_CONNECTOR.. tbh I've not been
> > > having time to follow too closely the recent changes with bridge stuff
> > > myself.
> > >
> > > But now with this patch we have both the ti bridge and the panel
> > > bridge creating a connector..  removing the connector created by the
> > > ti bridge "fixes" things, but not sure if that would break something
> > > on other platforms.  I guess there should now always be a panel
> > > bridge, so removing ti_sn_bridge_connector_init() would be a sane
> > > thing to do?
> >
> > So this patch works. We don't want to make the connector in this driver
> > for the next bridge because this driver is making the connector. I guess
> > eventually we'll drop this flag when this driver stops making the
> > connector here?
> >
> > ---8<---
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index cd0fccdd8dfd..a8d4818484aa 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -741,7 +741,7 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge,
> >
> >         /* Attach the next bridge */
> >         ret = drm_bridge_attach(bridge->encoder, pdata->next_bridge,
> > -                               &pdata->bridge, flags);
> > +                               &pdata->bridge, flags | DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >         if (ret < 0)
> >                 goto err_dsi_detach;
> 
> I kinda think *all* bridges that create a connector (whether optional
> or not) should OR in NO_CONNECTOR when attaching the next downstream
> bridge.. since you never want multiple connectors

Yes, that sounds reasonable to me. Stephen, would you like to set a
patch ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/6] drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge

[Stephen Boyd]

Quoting Laurent Pinchart (2021-08-11 15:40:24)
> On Wed, Aug 11, 2021 at 01:51:28PM -0700, Rob Clark wrote:
> >
> > I kinda think *all* bridges that create a connector (whether optional
> > or not) should OR in NO_CONNECTOR when attaching the next downstream
> > bridge.. since you never want multiple connectors
>
> Yes, that sounds reasonable to me. Stephen, would you like to set a
> patch ?
>

Sure I'll send a proper patch for this bridge driver.