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
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
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");
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);
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);
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
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 ?
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
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;
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
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 ?
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.
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) {
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
Applied to drm-misc-next
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=0715786771f24190b3f2dcd...
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
dri-devel@lists.freedesktop.org