This patch-set aims to make connector creation optional and prepare the bridge drivers for use in a chained setup.
The objective is that all bridge drivers shall support a chained setup connector creation is moved to the display drivers. This is just one step on this path.
Third iteration of this patchset covers several drivers, and a few panel adjustments.
The general approach for the bridge drivers: - Introduce drm_panel_brigde - Introduce bridge operations - Make connector creation optional
v3: - Rebase on top of drm-misc-next - Address kbuild test robot feedback
v2: - Updated bus_flags for boe,hv070wsa-100 - Collected r-b, but did not apply patches yet - On the panel side the panel-simple driver gained a default connector type for all the dumb panels that do not include so in their description. With this change panels always provide a connector type, and we have the potential to drop most uses of devm_drm_panel_bridge_add_typed(). - Added conversion of a few more bridge drivers
Patches can build but no run-time testing. So both test and review feedback appreciated!
Sam
Sam Ravnborg (21): drm/panel: add connector type to boe,hv070wsa-100 panel drm/panel: panel-simple: add default connector_type drm/bridge: tc358764: drop drm_connector_(un)register drm/bridge: tc358764: add drm_panel_bridge support drm/bridge: tc358764: make connector creation optional drm/bridge: tc358767: add drm_panel_bridge support drm/bridge: tc358767: add detect bridge operation drm/bridge: tc358767: add get_edid bride operation drm/bridge: tc358767: make connector creation optional drm/bridge: ti-tpd12s015: make connector creation optional drm/bridge: parade-ps8622: add drm_panel_bridge support drm/bridge: parade-ps8622: make connector creation optional drm/bridge: megachips: add helper to create connector drm/bridge: megachips: get drm_device from bridge drm/bridge: megachips: enable detect bridge operation drm/bridge: megachips: add get_edid bridge operation drm/bridge: megachips: make connector creation optional drm/bridge: nxp-ptn3460: add drm_panel_bridge support drm/bridge: nxp-ptn3460: add get_modes bridge operation drm/bridge: nxp-ptn3460: make connector creation optional drm/bridge: ti-sn65dsi86: add drm_panel_bridge support
.../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 92 +++++++++++------- drivers/gpu/drm/bridge/nxp-ptn3460.c | 107 +++++++++------------ drivers/gpu/drm/bridge/parade-ps8622.c | 54 +++-------- drivers/gpu/drm/bridge/tc358764.c | 66 +++---------- drivers/gpu/drm/bridge/tc358767.c | 98 +++++++++++-------- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 27 +++--- drivers/gpu/drm/bridge/ti-tpd12s015.c | 3 - drivers/gpu/drm/panel/panel-simple.c | 13 ++- 8 files changed, 216 insertions(+), 244 deletions(-)
The boe,hv070wsa-100 panel is a LVDS panel. Fix connector type to reflect this.
With this change users of this panel do not have to specify the connector type.
v3: - Drop PIXDATA bus_flag, not relevant
v2: - Add .bus_format (Laurent) - Add .bus_flags
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/panel/panel-simple.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 4f0ec5e5b0aa..7b5d212215e0 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -1188,6 +1188,9 @@ static const struct panel_desc boe_hv070wsa = { .width = 154, .height = 90, }, + .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_SPWG, + .bus_flags = DRM_BUS_FLAG_DE_HIGH, + .connector_type = DRM_MODE_CONNECTOR_LVDS, };
static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:23:57PM +0200, Sam Ravnborg wrote:
While at it, should you set .bpc to 8 ?
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
};
static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
All panels shall report a connector type. panel-simple has a lot of panels with no connector_type, and for these fall back to DPI as the default.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org --- drivers/gpu/drm/panel/panel-simple.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 7b5d212215e0..b3ec965721b0 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -500,6 +500,7 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) struct panel_simple *panel; struct display_timing dt; struct device_node *ddc; + int connector_type; int err;
panel = devm_kzalloc(dev, sizeof(*panel), GFP_KERNEL); @@ -566,8 +567,13 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) desc->bpc != 8); }
- drm_panel_init(&panel->base, dev, &panel_simple_funcs, - desc->connector_type); + /* Default DRM_MODE_CONNECTOR_DPI if no connector_type is set */ + if (desc->connector_type != 0) + connector_type = desc->connector_type; + else + connector_type = DRM_MODE_CONNECTOR_DPI; + + drm_panel_init(&panel->base, dev, &panel_simple_funcs, connector_type);
err = drm_panel_of_backlight(&panel->base); if (err)
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:23:58PM +0200, Sam Ravnborg wrote:
This avoids a WARN_ON(), which is good, but should we add a dev_warn() to get this fixed ?
Hi Laurent. On Sat, Jul 11, 2020 at 01:11:24AM +0300, Laurent Pinchart wrote:
We need a proper check for all relevant properties for DPI and the other connectors. Like you already did for LVDS.
The idea is we shall introduce the checks in one series, so users when they fix the warnings they are all good.
And then we will have to catch less during review which is good.
It is on my TODO list, but wanted to have some other stuff done first.
So in other words, good to go with this patch or do we need the proper checks in place first?
Sam
Warn is we detect a panel with missing descriptions. This is inpsired by a similar patch by Laurent that introduced checks for LVDS panels - this extends the checks to the reminaing type of connectors.
This is known to fail for some of the existing panels but added despite this as we need help from people using the panels to add the missing info. The checks are not complete but will catch the most common mistakes. The checks at the same time serves as documentation for the minimum required description for a panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Thierry Reding thierry.reding@gmail.com Cc: Sam Ravnborg sam@ravnborg.org ---
This is my attempt on the validation described in the previous mail. The assignment of default connector_type will then be a follow-up patch to this.
Sam
drivers/gpu/drm/panel/panel-simple.c | 32 ++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 2aff93accad5..025a7ccdfcb3 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -549,8 +549,12 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) panel_simple_parse_panel_timing_node(dev, panel, &dt); }
- if (desc->connector_type == DRM_MODE_CONNECTOR_LVDS) { - /* Catch common mistakes for LVDS panels. */ + /* Catch common mistakes for panels. */ + switch (desc->connector_type) { + case 0: + WARN(desc->connector_type == 0, "specify missing connector_type\n"); + break; + case DRM_MODE_CONNECTOR_LVDS: WARN_ON(desc->bus_flags & ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH | @@ -564,6 +568,30 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc) WARN_ON((desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_SPWG || desc->bus_format == MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA) && desc->bpc != 8); + break; + case DRM_MODE_CONNECTOR_eDP: + WARN_ON(desc->bus_format == 0); + WARN_ON(desc->bpc != 6 && desc->bpc != 8); + break; + case DRM_MODE_CONNECTOR_DSI: + WARN_ON(desc->bpc != 6 && desc->bpc != 8); + break; + case DRM_MODE_CONNECTOR_DPI: + WARN_ON(desc->bus_flags & + ~(DRM_BUS_FLAG_DE_LOW | + DRM_BUS_FLAG_DE_HIGH | + DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE | + DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE | + DRM_BUS_FLAG_DATA_MSB_TO_LSB | + DRM_BUS_FLAG_DATA_LSB_TO_MSB | + DRM_BUS_FLAG_SYNC_SAMPLE_POSEDGE | + DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE)); + WARN_ON(desc->bus_format == 0); + WARN_ON(desc->bpc != 6 && desc->bpc != 8); + break; + default: + WARN(true, "panel has unknown connector_type: %d\n", desc->connector_type); + break; }
drm_panel_init(&panel->base, dev, &panel_simple_funcs,
Hi Sam,
(CC'ing Daniel)
Thank you for the patch.
On Sat, Jul 11, 2020 at 11:47:26AM +0200, Sam Ravnborg wrote:
Warn is we detect a panel with missing descriptions.
s/is/if/
This is inpsired by a similar patch by Laurent that introduced checks for LVDS panels - this extends the checks to the reminaing type of
s/reminaing type/remaining types/
The checks look sane to me. For LVDS we've added the WARN_ON after checking all LVDS panels [1], so the warning will only get displayed for new panel drivers. For other types of panel, this will cause lots of WARN_ON to trigger. On one hand it gets the issues noticed, which should help fixing them, but on the other hand it will also scare lots of users and developers. I'm not sure if we should downgrade that to a dev_warn() for some time until we get at least the majority of the issues fixed. Daniel, any opinion ?
[1] Actually not quite, I've just sent "[PATCH] drm: panel: simple: Fix bpc for LG LB070WV8 panel" to fix one bpc issue.
drm_panel_init(&panel->base, dev, &panel_simple_funcs,
Hi Laurent.
On Sun, Jul 12, 2020 at 01:56:16AM +0300, Laurent Pinchart wrote:
We should be noisy, but not too noisy. dev_warn() is fine, maybe only dev_info()?
Unless I get other feedback I will convert to dev_info() in the next version of the patch. Fixing all panels before we add the checks, is not feasible. Access to datasheet is not easy/possible for many panels.
Sam
Hi Sam,
On Sun, Jul 12, 2020 at 12:58:19PM +0200, Sam Ravnborg wrote:
And that's the whole point of adding the warning, trying to get the people who have access to the panels to help fixing the problem.
I think a dev_warn() would be a good compromise, the missing info can have adverse consequences, so a warning seems appropriate.
Drop drm_connector handling that is not needed:
- drm_dev_register() in the display controller driver takes care of registering connectors. So the call to drm_connector_register() call is not needed in the bridge driver.
- Use of drm_connector_unregister() is only required for drivers that explicit have called drm_dev_register.
- The reference counting using drm_connector_put() is likewise not needed.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358764.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index 5ac1430fab04..a277739fab58 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -375,7 +375,6 @@ static int tc358764_attach(struct drm_bridge *bridge, drm_connector_attach_encoder(&ctx->connector, bridge->encoder); drm_panel_attach(ctx->panel, &ctx->connector); ctx->connector.funcs->reset(&ctx->connector); - drm_connector_register(&ctx->connector);
return 0; } @@ -384,10 +383,8 @@ static void tc358764_detach(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge);
- drm_connector_unregister(&ctx->connector); drm_panel_detach(ctx->panel); ctx->panel = NULL; - drm_connector_put(&ctx->connector); }
static const struct drm_bridge_funcs tc358764_bridge_funcs = {
Prepare the bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support.
The bridge panel will use the connector type reported by the panel, where the connector for this driver hardcode DRM_MODE_CONNECTOR_LVDS.
v2: - Use PTR_ERR_OR_ZERO() (kbuild test robot)
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: kbuild test robot lkp@intel.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358764.c | 57 ++++++++----------------------- 1 file changed, 14 insertions(+), 43 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index a277739fab58..e1f052a4f53b 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -156,7 +156,7 @@ struct tc358764 { struct drm_connector connector; struct regulator_bulk_data supplies[ARRAY_SIZE(tc358764_supplies)]; struct gpio_desc *gpio_reset; - struct drm_panel *panel; + struct drm_bridge *panel_bridge; int error; };
@@ -282,7 +282,7 @@ static int tc358764_get_modes(struct drm_connector *connector) { struct tc358764 *ctx = connector_to_tc358764(connector);
- return drm_panel_get_modes(ctx->panel, connector); + return drm_bridge_get_modes(ctx->panel_bridge, connector); }
static const @@ -298,23 +298,11 @@ static const struct drm_connector_funcs tc358764_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
-static void tc358764_disable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_disable(bridge_to_tc358764(bridge)->panel); - - if (ret < 0) - dev_err(ctx->dev, "error disabling panel (%d)\n", ret); -} - static void tc358764_post_disable(struct drm_bridge *bridge) { struct tc358764 *ctx = bridge_to_tc358764(bridge); int ret;
- ret = drm_panel_unprepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error unpreparing panel (%d)\n", ret); tc358764_reset(ctx); usleep_range(10000, 15000); ret = regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies); @@ -335,18 +323,6 @@ static void tc358764_pre_enable(struct drm_bridge *bridge) ret = tc358764_init(ctx); if (ret < 0) dev_err(ctx->dev, "error initializing bridge (%d)\n", ret); - ret = drm_panel_prepare(ctx->panel); - if (ret < 0) - dev_err(ctx->dev, "error preparing panel (%d)\n", ret); -} - -static void tc358764_enable(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - int ret = drm_panel_enable(ctx->panel); - - if (ret < 0) - dev_err(ctx->dev, "error enabling panel (%d)\n", ret); }
static int tc358764_attach(struct drm_bridge *bridge, @@ -356,6 +332,11 @@ static int tc358764_attach(struct drm_bridge *bridge, struct drm_device *drm = bridge->dev; int ret;
+ ret = drm_bridge_attach(bridge->encoder, ctx->panel_bridge, + bridge, flags); + if (ret < 0) + return ret; + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { DRM_ERROR("Fix bridge driver to make connector optional!"); return -EINVAL; @@ -373,32 +354,21 @@ static int tc358764_attach(struct drm_bridge *bridge, drm_connector_helper_add(&ctx->connector, &tc358764_connector_helper_funcs); drm_connector_attach_encoder(&ctx->connector, bridge->encoder); - drm_panel_attach(ctx->panel, &ctx->connector); ctx->connector.funcs->reset(&ctx->connector);
return 0; }
-static void tc358764_detach(struct drm_bridge *bridge) -{ - struct tc358764 *ctx = bridge_to_tc358764(bridge); - - drm_panel_detach(ctx->panel); - ctx->panel = NULL; -} - static const struct drm_bridge_funcs tc358764_bridge_funcs = { - .disable = tc358764_disable, .post_disable = tc358764_post_disable, - .enable = tc358764_enable, .pre_enable = tc358764_pre_enable, .attach = tc358764_attach, - .detach = tc358764_detach, };
static int tc358764_parse_dt(struct tc358764 *ctx) { struct device *dev = ctx->dev; + struct drm_panel *panel; int ret;
ctx->gpio_reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW); @@ -407,12 +377,13 @@ static int tc358764_parse_dt(struct tc358764 *ctx) return PTR_ERR(ctx->gpio_reset); }
- ret = drm_of_find_panel_or_bridge(ctx->dev->of_node, 1, 0, &ctx->panel, - NULL); - if (ret && ret != -EPROBE_DEFER) - dev_err(dev, "cannot find panel (%d)\n", ret); + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL); + if (ret) + return ret;
- return ret; + ctx->panel_bridge = devm_drm_panel_bridge_add(dev, panel); + + return PTR_ERR_OR_ZERO(ctx->panel_bridge); }
static int tc358764_configure_regulators(struct tc358764 *ctx)
Make the connector creation optional to enable usage of the tc358764 bridge with the DRM bridge connector helper.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358764.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358764.c b/drivers/gpu/drm/bridge/tc358764.c index e1f052a4f53b..4477224e5079 100644 --- a/drivers/gpu/drm/bridge/tc358764.c +++ b/drivers/gpu/drm/bridge/tc358764.c @@ -337,10 +337,8 @@ static int tc358764_attach(struct drm_bridge *bridge, if (ret < 0) return ret;
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0;
ctx->connector.polled = DRM_CONNECTOR_POLL_HPD; ret = drm_connector_init(drm, &ctx->connector,
Prepare the bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support.
The bridge driver assume the panel is optional. The relevant tests are migrated over to check for the pnale bridge to keep the same functionality.
Note: the bridge panel will use the connector type from the panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358767.c | 57 +++++++++++++++---------------- 1 file changed, 27 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index c2777b226c75..08d483664258 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -244,8 +244,8 @@ struct tc_data { struct drm_dp_aux aux;
struct drm_bridge bridge; + struct drm_bridge *panel_bridge; struct drm_connector connector; - struct drm_panel *panel;
/* link settings */ struct tc_edp_link link; @@ -1236,13 +1236,6 @@ static int tc_stream_disable(struct tc_data *tc) return 0; }
-static void tc_bridge_pre_enable(struct drm_bridge *bridge) -{ - struct tc_data *tc = bridge_to_tc(bridge); - - drm_panel_prepare(tc->panel); -} - static void tc_bridge_enable(struct drm_bridge *bridge) { struct tc_data *tc = bridge_to_tc(bridge); @@ -1266,8 +1259,6 @@ static void tc_bridge_enable(struct drm_bridge *bridge) tc_main_link_disable(tc); return; } - - drm_panel_enable(tc->panel); }
static void tc_bridge_disable(struct drm_bridge *bridge) @@ -1275,8 +1266,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge) struct tc_data *tc = bridge_to_tc(bridge); int ret;
- drm_panel_disable(tc->panel); - ret = tc_stream_disable(tc); if (ret < 0) dev_err(tc->dev, "main link stream stop error: %d\n", ret); @@ -1286,13 +1275,6 @@ static void tc_bridge_disable(struct drm_bridge *bridge) dev_err(tc->dev, "main link disable error: %d\n", ret); }
-static void tc_bridge_post_disable(struct drm_bridge *bridge) -{ - struct tc_data *tc = bridge_to_tc(bridge); - - drm_panel_unprepare(tc->panel); -} - static bool tc_bridge_mode_fixup(struct drm_bridge *bridge, const struct drm_display_mode *mode, struct drm_display_mode *adj) @@ -1348,9 +1330,11 @@ static int tc_connector_get_modes(struct drm_connector *connector) return 0; }
- count = drm_panel_get_modes(tc->panel, connector); - if (count > 0) - return count; + if (tc->panel_bridge) { + count = drm_bridge_get_modes(tc->panel_bridge, connector); + if (count > 0) + return count; + }
edid = drm_get_edid(connector, &tc->aux.ddc);
@@ -1378,7 +1362,7 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne int ret;
if (tc->hpd_pin < 0) { - if (tc->panel) + if (tc->panel_bridge) return connector_status_connected; else return connector_status_unknown; @@ -1413,6 +1397,13 @@ static int tc_bridge_attach(struct drm_bridge *bridge, struct drm_device *drm = bridge->dev; int ret;
+ if (tc->panel_bridge) { + ret = drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge, + &tc->bridge, flags); + if (ret < 0) + return ret; + } + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { DRM_ERROR("Fix bridge driver to make connector optional!"); return -EINVAL; @@ -1421,7 +1412,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge, /* Create DP/eDP connector */ drm_connector_helper_add(&tc->connector, &tc_connector_helper_funcs); ret = drm_connector_init(drm, &tc->connector, &tc_connector_funcs, - tc->panel ? DRM_MODE_CONNECTOR_eDP : + tc->panel_bridge ? DRM_MODE_CONNECTOR_eDP : DRM_MODE_CONNECTOR_DisplayPort); if (ret) return ret; @@ -1435,9 +1426,6 @@ static int tc_bridge_attach(struct drm_bridge *bridge, DRM_CONNECTOR_POLL_DISCONNECT; }
- if (tc->panel) - drm_panel_attach(tc->panel, &tc->connector); - drm_display_info_set_bus_formats(&tc->connector.display_info, &bus_format, 1); tc->connector.display_info.bus_flags = @@ -1453,10 +1441,8 @@ static const struct drm_bridge_funcs tc_bridge_funcs = { .attach = tc_bridge_attach, .mode_valid = tc_mode_valid, .mode_set = tc_bridge_mode_set, - .pre_enable = tc_bridge_pre_enable, .enable = tc_bridge_enable, .disable = tc_bridge_disable, - .post_disable = tc_bridge_post_disable, .mode_fixup = tc_bridge_mode_fixup, };
@@ -1547,6 +1533,7 @@ static irqreturn_t tc_irq_handler(int irq, void *arg) static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct device *dev = &client->dev; + struct drm_panel *panel; struct tc_data *tc; int ret;
@@ -1557,10 +1544,20 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) tc->dev = dev;
/* port@2 is the output port */ - ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &tc->panel, NULL); + ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL); if (ret && ret != -ENODEV) return ret;
+ if (panel) { + struct drm_bridge *pbridge; + + pbridge = devm_drm_panel_bridge_add(dev, panel); + if (IS_ERR(pbridge)) + return PTR_ERR(pbridge); + + tc->panel_bridge = pbridge; + } + /* Shut down GPIO is optional */ tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH); if (IS_ERR(tc->sd_gpio))
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:02PM +0200, Sam Ravnborg wrote:
s/pnale/panel/
With this both this driver and the panel bridge driver will create a connector. The simplest way to handle that is probably to pass flags & ~DRM_BRIDGE_ATTACH_NO_CONNECTOR to drm_bridge_attach(). It's a bit of a hack, but should go away once all users are converted to !DRM_BRIDGE_ATTACH_NO_CONNECTOR.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Hi Laurent.
On Sat, Jul 11, 2020 at 01:19:35AM +0300, Laurent Pinchart wrote:
I do not follow you here - sorry.
We have two situations:
display driver creates the connector - and passes DRM_BRIDGE_ATTACH_NO_CONNECTOR. - bridge driver shall not create connector - bridge panel shall not create connector
display driver expect bridge to create connector and passes 0 - bridge driver shall not create connector - bridge panel shall create connector
So the correct logic seems to be: - if there is a bridge panel do not create a connector in the bridge - otherwise just follow the flags
I will try to implement this.
Sam
Hi Sam,
On Sun, Jul 19, 2020 at 03:06:56PM +0200, Sam Ravnborg wrote:
I meant the other way around, creating a connector here and skipping it in the bridge panel driver. Which of the two (this driver and the bridge panel driver) has the most information to implement a connector ?
Prepare the bridge driver for chained operation by adding support for the detect operation.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358767.c | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 08d483664258..85973ae728db 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1353,10 +1353,9 @@ static const struct drm_connector_helper_funcs tc_connector_helper_funcs = { .get_modes = tc_connector_get_modes, };
-static enum drm_connector_status tc_connector_detect(struct drm_connector *connector, - bool force) +static enum drm_connector_status tc_bridge_detect(struct drm_bridge *bridge) { - struct tc_data *tc = connector_to_tc(connector); + struct tc_data *tc = bridge_to_tc(bridge); bool conn; u32 val; int ret; @@ -1380,6 +1379,14 @@ static enum drm_connector_status tc_connector_detect(struct drm_connector *conne return connector_status_disconnected; }
+static enum drm_connector_status +tc_connector_detect(struct drm_connector *connector, bool force) +{ + struct tc_data *tc = connector_to_tc(connector); + + return tc_bridge_detect(&tc->bridge); +} + static const struct drm_connector_funcs tc_connector_funcs = { .detect = tc_connector_detect, .fill_modes = drm_helper_probe_single_connector_modes, @@ -1444,6 +1451,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = { .enable = tc_bridge_enable, .disable = tc_bridge_disable, .mode_fixup = tc_bridge_mode_fixup, + .detect = tc_bridge_detect, };
static bool tc_readable_reg(struct device *dev, unsigned int reg) @@ -1677,6 +1685,8 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) return ret;
tc->bridge.funcs = &tc_bridge_funcs; + tc->bridge.ops = DRM_BRIDGE_OP_DETECT; + tc->bridge.of_node = dev->of_node; drm_bridge_add(&tc->bridge);
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:03PM +0200, Sam Ravnborg wrote:
I think you should set DRM_BRIDGE_OP_DETECT only when tc->hpd_pin is valid. I would also move the if (tc->hpd_pin < 0) case from tc_bridge_detect to +tc_connector_detect.
Prepare for chained bridge with the addition of get_edid support.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358767.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 85973ae728db..fb9d57967b2c 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1317,6 +1317,20 @@ static void tc_bridge_mode_set(struct drm_bridge *bridge, tc->mode = *mode; }
+static struct edid *tc_get_edid(struct drm_bridge *bridge, + struct drm_connector *connector) +{ + struct tc_data *tc = bridge_to_tc(bridge); + struct edid *edid; + + edid = drm_get_edid(connector, &tc->aux.ddc); + + kfree(tc->edid); + tc->edid = edid; + + return edid; +} + static int tc_connector_get_modes(struct drm_connector *connector) { struct tc_data *tc = connector_to_tc(connector); @@ -1336,12 +1350,7 @@ static int tc_connector_get_modes(struct drm_connector *connector) return count; }
- edid = drm_get_edid(connector, &tc->aux.ddc); - - kfree(tc->edid); - tc->edid = edid; - if (!edid) - return 0; + edid = tc_get_edid(&tc->bridge, connector);
drm_connector_update_edid_property(connector, edid); count = drm_add_edid_modes(connector, edid); @@ -1452,6 +1461,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = { .disable = tc_bridge_disable, .mode_fixup = tc_bridge_mode_fixup, .detect = tc_bridge_detect, + .get_edid = tc_get_edid, };
static bool tc_readable_reg(struct device *dev, unsigned int reg) @@ -1685,7 +1695,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id) return ret;
tc->bridge.funcs = &tc_bridge_funcs; - tc->bridge.ops = DRM_BRIDGE_OP_DETECT; + tc->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID;
tc->bridge.of_node = dev->of_node; drm_bridge_add(&tc->bridge);
Hi Sam,
Thank you for the patch.
s/bride/bridge/ in the subject line.
On Fri, Jul 03, 2020 at 09:24:04PM +0200, Sam Ravnborg wrote:
The caller (drm_bridge_connector_get_modes_edid()) calls kfree(edid), so if you want to store it internally, you'll have to make a copy. Can you skip internal storage altogether by freeing the memory in tc_connector_get_modes() ?
Display drivers are in the new model expected to create the connector using drm_bridge_connector_init(). Allow users of this bridge driver to support the new model by introducing support for optional connector creation.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/tc358767.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index fb9d57967b2c..2eb00d39f619 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -1421,8 +1421,7 @@ static int tc_bridge_attach(struct drm_bridge *bridge, }
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; + return 0; }
/* Create DP/eDP connector */
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:05PM +0200, Sam Ravnborg wrote:
I'd drop the braces too.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
/* Create DP/eDP connector */
The ti-tpd12s015 do not create any connector, so ignore the flags argument, just pass it on to the next bridge in the chain.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/ti-tpd12s015.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-tpd12s015.c b/drivers/gpu/drm/bridge/ti-tpd12s015.c index 514cbf0eac75..4f1666422ab2 100644 --- a/drivers/gpu/drm/bridge/ti-tpd12s015.c +++ b/drivers/gpu/drm/bridge/ti-tpd12s015.c @@ -43,9 +43,6 @@ static int tpd12s015_attach(struct drm_bridge *bridge, struct tpd12s015_device *tpd = to_tpd12s015(bridge); int ret;
- if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) - return -EINVAL; - ret = drm_bridge_attach(bridge->encoder, tpd->next_bridge, bridge, flags); if (ret < 0)
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:06PM +0200, Sam Ravnborg wrote:
The driver is only used by devices that use DRM_BRIDGE_ATTACH_NO_CONNECTOR. I'd rather keep this check and port other potential users to DRM_BRIDGE_ATTACH_NO_CONNECTOR instead of allowing operation in !DRM_BRIDGE_ATTACH_NO_CONNECTOR mode.
Prepare the bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support.
Note: the bridge panel will use the connector type from the panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/parade-ps8622.c | 46 ++++++++------------------ 1 file changed, 13 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index d789ea2a7fb9..6ab6f60b9091 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -45,7 +45,7 @@ struct ps8622_bridge { struct drm_connector connector; struct i2c_client *client; struct drm_bridge bridge; - struct drm_panel *panel; + struct drm_bridge *panel_bridge; struct regulator *v12; struct backlight_device *bl;
@@ -365,11 +365,6 @@ static void ps8622_pre_enable(struct drm_bridge *bridge) DRM_ERROR("fails to enable ps8622->v12"); }
- if (drm_panel_prepare(ps8622->panel)) { - DRM_ERROR("failed to prepare panel\n"); - return; - } - gpiod_set_value(ps8622->gpio_slp, 1);
/* @@ -399,24 +394,8 @@ static void ps8622_pre_enable(struct drm_bridge *bridge) ps8622->enabled = true; }
-static void ps8622_enable(struct drm_bridge *bridge) -{ - struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); - - if (drm_panel_enable(ps8622->panel)) { - DRM_ERROR("failed to enable panel\n"); - return; - } -} - static void ps8622_disable(struct drm_bridge *bridge) { - struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); - - if (drm_panel_disable(ps8622->panel)) { - DRM_ERROR("failed to disable panel\n"); - return; - } msleep(PS8622_PWMO_END_T12_MS); }
@@ -436,11 +415,6 @@ static void ps8622_post_disable(struct drm_bridge *bridge) */ gpiod_set_value(ps8622->gpio_slp, 0);
- if (drm_panel_unprepare(ps8622->panel)) { - DRM_ERROR("failed to unprepare panel\n"); - return; - } - if (ps8622->v12) regulator_disable(ps8622->v12);
@@ -461,7 +435,7 @@ static int ps8622_get_modes(struct drm_connector *connector)
ps8622 = connector_to_ps8622(connector);
- return drm_panel_get_modes(ps8622->panel, connector); + return drm_bridge_get_modes(ps8622->panel_bridge, connector); }
static const struct drm_connector_helper_funcs ps8622_connector_helper_funcs = { @@ -482,6 +456,9 @@ static int ps8622_attach(struct drm_bridge *bridge, struct ps8622_bridge *ps8622 = bridge_to_ps8622(bridge); int ret;
+ ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge, + &ps8622->bridge, flags); + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { DRM_ERROR("Fix bridge driver to make connector optional!"); return -EINVAL; @@ -505,9 +482,6 @@ static int ps8622_attach(struct drm_bridge *bridge, drm_connector_attach_encoder(&ps8622->connector, bridge->encoder);
- if (ps8622->panel) - drm_panel_attach(ps8622->panel, &ps8622->connector); - drm_helper_hpd_irq_event(ps8622->connector.dev);
return ret; @@ -515,7 +489,6 @@ static int ps8622_attach(struct drm_bridge *bridge,
static const struct drm_bridge_funcs ps8622_bridge_funcs = { .pre_enable = ps8622_pre_enable, - .enable = ps8622_enable, .disable = ps8622_disable, .post_disable = ps8622_post_disable, .attach = ps8622_attach, @@ -533,16 +506,23 @@ static int ps8622_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct ps8622_bridge *ps8622; + struct drm_bridge *pbridge; + struct drm_panel *panel; int ret;
ps8622 = devm_kzalloc(dev, sizeof(*ps8622), GFP_KERNEL); if (!ps8622) return -ENOMEM;
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ps8622->panel, NULL); + ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL); if (ret) return ret;
+ pbridge = devm_drm_panel_bridge_add(dev, panel); + if (IS_ERR(pbridge)) + return PTR_ERR(pbridge); + + ps8622->panel_bridge = pbridge; ps8622->client = client;
ps8622->v12 = devm_regulator_get(dev, "vdd12");
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:07PM +0200, Sam Ravnborg wrote:
What's the point in just waiting in the disable path ?
Same comment as earlier in this series about double connector creation.
Don't you need to check ret here ?
Make the connector creation optional to enable usage of the parade-ps8622 bridge with the DRM bridge connector helper.
This change moves drm_helper_hpd_irq_event() call in the attach function up before the connector creation.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/parade-ps8622.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/parade-ps8622.c b/drivers/gpu/drm/bridge/parade-ps8622.c index 6ab6f60b9091..54aa5270d2c9 100644 --- a/drivers/gpu/drm/bridge/parade-ps8622.c +++ b/drivers/gpu/drm/bridge/parade-ps8622.c @@ -459,10 +459,8 @@ static int ps8622_attach(struct drm_bridge *bridge, ret = drm_bridge_attach(ps8622->bridge.encoder, ps8622->panel_bridge, &ps8622->bridge, flags);
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0;
if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found"); @@ -482,7 +480,7 @@ static int ps8622_attach(struct drm_bridge *bridge, drm_connector_attach_encoder(&ps8622->connector, bridge->encoder);
- drm_helper_hpd_irq_event(ps8622->connector.dev); + drm_helper_hpd_irq_event(ps8622->bridge.dev);
return ret; }
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:08PM +0200, Sam Ravnborg wrote:
No it doesn't :-)
And I wonder why that call is actually needed.
Factor out connector creation to a small helper function.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Peter Senna Tschudin peter.senna@gmail.com Cc: Martin Donnelly martin.donnelly@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 47 +++++++++++-------- 1 file changed, 27 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 6200f12a37e6..258e0525cdcc 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -191,6 +191,32 @@ static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, };
+static int ge_b850v3_lvds_create_connector(struct drm_bridge *bridge) +{ + struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector; + int ret; + + if (!bridge->encoder) { + DRM_ERROR("Parent encoder object not found"); + return -ENODEV; + } + + connector->polled = DRM_CONNECTOR_POLL_HPD; + + drm_connector_helper_add(connector, + &ge_b850v3_lvds_connector_helper_funcs); + + ret = drm_connector_init(bridge->dev, connector, + &ge_b850v3_lvds_connector_funcs, + DRM_MODE_CONNECTOR_DisplayPort); + if (ret) { + DRM_ERROR("Failed to initialize connector with drm\n"); + return ret; + } + + return drm_connector_attach_encoder(connector, bridge->encoder); +} + static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id) { struct i2c_client *stdp4028_i2c @@ -209,7 +235,6 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id) static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, enum drm_bridge_attach_flags flags) { - struct drm_connector *connector = &ge_b850v3_lvds_ptr->connector; struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; int ret; @@ -219,25 +244,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, return -EINVAL; }
- if (!bridge->encoder) { - DRM_ERROR("Parent encoder object not found"); - return -ENODEV; - } - - connector->polled = DRM_CONNECTOR_POLL_HPD; - - drm_connector_helper_add(connector, - &ge_b850v3_lvds_connector_helper_funcs); - - ret = drm_connector_init(bridge->dev, connector, - &ge_b850v3_lvds_connector_funcs, - DRM_MODE_CONNECTOR_DisplayPort); - if (ret) { - DRM_ERROR("Failed to initialize connector with drm\n"); - return ret; - } - - ret = drm_connector_attach_encoder(connector, bridge->encoder); + ret = ge_b850v3_lvds_create_connector(bridge); if (ret) return ret;
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:09PM +0200, Sam Ravnborg wrote:
Wow, storing device state in a global variable... :-( How did this go past review ?
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Fix so drm_device is read from the bridge. This is a preparation for the connector being optional.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Peter Senna Tschudin peter.senna@gmail.com Cc: Martin Donnelly martin.donnelly@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 258e0525cdcc..cf1dfbc88acf 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -226,8 +226,8 @@ static irqreturn_t ge_b850v3_lvds_irq_handler(int irq, void *dev_id) STDP4028_DPTX_IRQ_STS_REG, STDP4028_DPTX_IRQ_CLEAR);
- if (ge_b850v3_lvds_ptr->connector.dev) - drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->connector.dev); + if (ge_b850v3_lvds_ptr->bridge.dev) + drm_kms_helper_hotplug_event(ge_b850v3_lvds_ptr->bridge.dev);
return IRQ_HANDLED; }
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:10PM +0200, Sam Ravnborg wrote:
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
To prepare for use in a chained bridge setup enable the detect operation.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Peter Senna Tschudin peter.senna@gmail.com Cc: Martin Donnelly martin.donnelly@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- .../gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index cf1dfbc88acf..78a9afe8f063 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -163,8 +163,8 @@ drm_connector_helper_funcs ge_b850v3_lvds_connector_helper_funcs = { .mode_valid = ge_b850v3_lvds_mode_valid, };
-static enum drm_connector_status ge_b850v3_lvds_detect( - struct drm_connector *connector, bool force) +static enum drm_connector_status ge_b850v3_lvds_bridge_detect( + struct drm_bridge *bridge) { struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; @@ -182,6 +182,12 @@ static enum drm_connector_status ge_b850v3_lvds_detect( return connector_status_unknown; }
+static enum drm_connector_status ge_b850v3_lvds_detect( + struct drm_connector *connector, bool force) +{ + return ge_b850v3_lvds_bridge_detect(&ge_b850v3_lvds_ptr->bridge); +} + static const struct drm_connector_funcs ge_b850v3_lvds_connector_funcs = { .fill_modes = drm_helper_probe_single_connector_modes, .detect = ge_b850v3_lvds_detect, @@ -263,6 +269,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge,
static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = { .attach = ge_b850v3_lvds_attach, + .detect = ge_b850v3_lvds_bridge_detect, };
static int ge_b850v3_lvds_init(struct device *dev) @@ -317,6 +324,7 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
/* drm bridge initialization */ ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; + ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT; ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:11PM +0200, Sam Ravnborg wrote:
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
To prepare for a chained bridge setup add support for the get_edid bridge operation.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Peter Senna Tschudin peter.senna@gmail.com Cc: Martin Donnelly martin.donnelly@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- .../bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 78a9afe8f063..5f06e18f0a61 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -131,21 +131,29 @@ static u8 *stdp2690_get_edid(struct i2c_client *client) return NULL; }
-static int ge_b850v3_lvds_get_modes(struct drm_connector *connector) +static struct edid *ge_b850v3_lvds_get_edid( + struct drm_bridge *bridge, struct drm_connector *connector) { struct i2c_client *client; - int num_modes = 0;
client = ge_b850v3_lvds_ptr->stdp2690_i2c;
kfree(ge_b850v3_lvds_ptr->edid); ge_b850v3_lvds_ptr->edid = (struct edid *)stdp2690_get_edid(client);
- if (ge_b850v3_lvds_ptr->edid) { - drm_connector_update_edid_property(connector, - ge_b850v3_lvds_ptr->edid); - num_modes = drm_add_edid_modes(connector, - ge_b850v3_lvds_ptr->edid); + return ge_b850v3_lvds_ptr->edid; +} + +static int ge_b850v3_lvds_get_modes(struct drm_connector *connector) +{ + struct edid *edid; + int num_modes = 0; + + edid = ge_b850v3_lvds_get_edid(&ge_b850v3_lvds_ptr->bridge, connector); + + if (edid) { + drm_connector_update_edid_property(connector, edid); + num_modes = drm_add_edid_modes(connector, edid); }
return num_modes; @@ -270,6 +278,7 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = { .attach = ge_b850v3_lvds_attach, .detect = ge_b850v3_lvds_bridge_detect, + .get_edid = ge_b850v3_lvds_get_edid, };
static int ge_b850v3_lvds_init(struct device *dev) @@ -324,7 +333,8 @@ static int stdp4028_ge_b850v3_fw_probe(struct i2c_client *stdp4028_i2c,
/* drm bridge initialization */ ge_b850v3_lvds_ptr->bridge.funcs = &ge_b850v3_lvds_funcs; - ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT; + ge_b850v3_lvds_ptr->bridge.ops = DRM_BRIDGE_OP_DETECT | + DRM_BRIDGE_OP_EDID; ge_b850v3_lvds_ptr->bridge.of_node = dev->of_node; drm_bridge_add(&ge_b850v3_lvds_ptr->bridge);
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:12PM +0200, Sam Ravnborg wrote:
As pointed out earlier in this series, you can't store a pointer to the edid, if will get freed by the caller. Fortunately it doesn't seem to be needed here either.
Make the connector creation optional to enable usage of the megachips-stdpxxxx-ge-b850v3-fw bridge with the DRM bridge connector helper.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Peter Senna Tschudin peter.senna@gmail.com Cc: Martin Donnelly martin.donnelly@ge.com Cc: Martyn Welch martyn.welch@collabora.co.uk Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- .../drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c index 5f06e18f0a61..991417ab35b6 100644 --- a/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c +++ b/drivers/gpu/drm/bridge/megachips-stdpxxxx-ge-b850v3-fw.c @@ -251,16 +251,6 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, { struct i2c_client *stdp4028_i2c = ge_b850v3_lvds_ptr->stdp4028_i2c; - int ret; - - if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } - - ret = ge_b850v3_lvds_create_connector(bridge); - if (ret) - return ret;
/* Configures the bridge to re-enable interrupts after each ack. */ i2c_smbus_write_word_data(stdp4028_i2c, @@ -272,7 +262,10 @@ static int ge_b850v3_lvds_attach(struct drm_bridge *bridge, STDP4028_DPTX_IRQ_EN_REG, STDP4028_DPTX_IRQ_CONFIG);
- return 0; + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0; + + return ge_b850v3_lvds_create_connector(bridge); }
static const struct drm_bridge_funcs ge_b850v3_lvds_funcs = {
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:13PM +0200, Sam Ravnborg wrote:
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Prepare the bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support.
Note: the bridge panel will use the connector type from the panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/nxp-ptn3460.c | 51 ++++++++-------------------- 1 file changed, 14 insertions(+), 37 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 438e566ce0a4..0bd9f0e451b3 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -30,7 +30,7 @@ struct ptn3460_bridge { struct i2c_client *client; struct drm_bridge bridge; struct edid *edid; - struct drm_panel *panel; + struct drm_bridge *panel_bridge; struct gpio_desc *gpio_pd_n; struct gpio_desc *gpio_rst_n; u32 edid_emulation; @@ -127,11 +127,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) usleep_range(10, 20); gpiod_set_value(ptn_bridge->gpio_rst_n, 1);
- if (drm_panel_prepare(ptn_bridge->panel)) { - DRM_ERROR("failed to prepare panel\n"); - return; - } - /* * There's a bug in the PTN chip where it falsely asserts hotplug before * it is fully functional. We're forced to wait for the maximum start up @@ -146,16 +141,6 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge) ptn_bridge->enabled = true; }
-static void ptn3460_enable(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); - - if (drm_panel_enable(ptn_bridge->panel)) { - DRM_ERROR("failed to enable panel\n"); - return; - } -} - static void ptn3460_disable(struct drm_bridge *bridge) { struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); @@ -165,25 +150,10 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
- if (drm_panel_disable(ptn_bridge->panel)) { - DRM_ERROR("failed to disable panel\n"); - return; - } - gpiod_set_value(ptn_bridge->gpio_rst_n, 1); gpiod_set_value(ptn_bridge->gpio_pd_n, 0); }
-static void ptn3460_post_disable(struct drm_bridge *bridge) -{ - struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); - - if (drm_panel_unprepare(ptn_bridge->panel)) { - DRM_ERROR("failed to unprepare panel\n"); - return; - } -} - static int ptn3460_get_modes(struct drm_connector *connector) { struct ptn3460_bridge *ptn_bridge; @@ -242,6 +212,11 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge, struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); int ret;
+ ret = drm_bridge_attach(bridge->encoder, ptn_bridge->panel_bridge, + bridge, flags); + if (ret < 0) + return ret; + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { DRM_ERROR("Fix bridge driver to make connector optional!"); return -EINVAL; @@ -265,9 +240,6 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge, drm_connector_attach_encoder(&ptn_bridge->connector, bridge->encoder);
- if (ptn_bridge->panel) - drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector); - drm_helper_hpd_irq_event(ptn_bridge->connector.dev);
return ret; @@ -275,9 +247,7 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge,
static const struct drm_bridge_funcs ptn3460_bridge_funcs = { .pre_enable = ptn3460_pre_enable, - .enable = ptn3460_enable, .disable = ptn3460_disable, - .post_disable = ptn3460_post_disable, .attach = ptn3460_bridge_attach, };
@@ -286,6 +256,8 @@ static int ptn3460_probe(struct i2c_client *client, { struct device *dev = &client->dev; struct ptn3460_bridge *ptn_bridge; + struct drm_bridge *pbridge; + struct drm_panel *panel; int ret;
ptn_bridge = devm_kzalloc(dev, sizeof(*ptn_bridge), GFP_KERNEL); @@ -293,10 +265,15 @@ static int ptn3460_probe(struct i2c_client *client, return -ENOMEM; }
- ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &ptn_bridge->panel, NULL); + ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, NULL); if (ret) return ret;
+ pbridge = devm_drm_panel_bridge_add(dev, panel); + if (IS_ERR(pbridge)) + return PTR_ERR(pbridge); + + ptn_bridge->panel_bridge = pbridge; ptn_bridge->client = client;
ptn_bridge->gpio_pd_n = devm_gpiod_get(&client->dev, "powerdown",
Add the get_modes() bridge operation to prepare for use as a chained bridge. Add helper function that is also used by the connector.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/nxp-ptn3460.c | 52 ++++++++++++++++++---------- 1 file changed, 33 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index 0bd9f0e451b3..e253c185f94c 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -154,17 +154,13 @@ static void ptn3460_disable(struct drm_bridge *bridge) gpiod_set_value(ptn_bridge->gpio_pd_n, 0); }
-static int ptn3460_get_modes(struct drm_connector *connector) +static struct edid *ptn3460_get_edid(struct drm_bridge *bridge, + struct drm_connector *connector) { - struct ptn3460_bridge *ptn_bridge; - u8 *edid; - int ret, num_modes = 0; + struct ptn3460_bridge *ptn_bridge = bridge_to_ptn3460(bridge); bool power_off; - - ptn_bridge = connector_to_ptn3460(connector); - - if (ptn_bridge->edid) - return drm_add_edid_modes(connector, ptn_bridge->edid); + u8 *edid; + int ret;
power_off = !ptn_bridge->enabled; ptn3460_pre_enable(&ptn_bridge->bridge); @@ -172,30 +168,46 @@ static int ptn3460_get_modes(struct drm_connector *connector) edid = kmalloc(EDID_LENGTH, GFP_KERNEL); if (!edid) { DRM_ERROR("Failed to allocate EDID\n"); - return 0; + return NULL; }
ret = ptn3460_read_bytes(ptn_bridge, PTN3460_EDID_ADDR, edid, - EDID_LENGTH); + EDID_LENGTH); if (ret) { kfree(edid); - goto out; + return NULL; }
+ if (power_off) + ptn3460_disable(&ptn_bridge->bridge); + + kfree(ptn_bridge->edid); ptn_bridge->edid = (struct edid *)edid; - drm_connector_update_edid_property(connector, ptn_bridge->edid);
- num_modes = drm_add_edid_modes(connector, ptn_bridge->edid); + return ptn_bridge->edid; +}
-out: - if (power_off) - ptn3460_disable(&ptn_bridge->bridge); +static int ptn3460_connector_get_modes(struct drm_connector *connector) +{ + struct ptn3460_bridge *ptn_bridge; + struct edid *edid; + + ptn_bridge = connector_to_ptn3460(connector); + + if (ptn_bridge->edid) + return drm_add_edid_modes(connector, ptn_bridge->edid); + + edid = ptn3460_get_edid(&ptn_bridge->bridge, connector); + if (!edid) + return 0; + + drm_connector_update_edid_property(connector, edid);
- return num_modes; + return drm_add_edid_modes(connector, edid); }
static const struct drm_connector_helper_funcs ptn3460_connector_helper_funcs = { - .get_modes = ptn3460_get_modes, + .get_modes = ptn3460_connector_get_modes, };
static const struct drm_connector_funcs ptn3460_connector_funcs = { @@ -249,6 +261,7 @@ static const struct drm_bridge_funcs ptn3460_bridge_funcs = { .pre_enable = ptn3460_pre_enable, .disable = ptn3460_disable, .attach = ptn3460_bridge_attach, + .get_edid = ptn3460_get_edid, };
static int ptn3460_probe(struct i2c_client *client, @@ -304,6 +317,7 @@ static int ptn3460_probe(struct i2c_client *client, }
ptn_bridge->bridge.funcs = &ptn3460_bridge_funcs; + ptn_bridge->bridge.ops = DRM_BRIDGE_OP_EDID; ptn_bridge->bridge.of_node = dev->of_node; drm_bridge_add(&ptn_bridge->bridge);
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:15PM +0200, Sam Ravnborg wrote:
Don't you need to power off in the error path ?
And here too ?
Same comment as earlier in this series about storing the edid.
Maybe
struct ptn3460_bridge *ptn_bridge = connector_to_ptn3460(connector);
?
Make the connector creation optional to enable usage of the nxp-ptn3460 bridge with the DRM bridge connector helper.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/nxp-ptn3460.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/bridge/nxp-ptn3460.c b/drivers/gpu/drm/bridge/nxp-ptn3460.c index e253c185f94c..6a65657087f9 100644 --- a/drivers/gpu/drm/bridge/nxp-ptn3460.c +++ b/drivers/gpu/drm/bridge/nxp-ptn3460.c @@ -229,10 +229,8 @@ static int ptn3460_bridge_attach(struct drm_bridge *bridge, if (ret < 0) return ret;
- if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) { - DRM_ERROR("Fix bridge driver to make connector optional!"); - return -EINVAL; - } + if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) + return 0;
if (!bridge->encoder) { DRM_ERROR("Parent encoder object not found");
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:16PM +0200, Sam Ravnborg wrote:
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Prepare the bridge driver for use in a chained setup by replacing direct use of drm_panel with drm_panel_bridge support.
Note: the bridge panel will use the connector type from the panel.
Signed-off-by: Sam Ravnborg sam@ravnborg.org Cc: Andrzej Hajda a.hajda@samsung.com Cc: Neil Armstrong narmstrong@baylibre.com Cc: Laurent Pinchart Laurent.pinchart@ideasonboard.com Cc: Jonas Karlman jonas@kwiboo.se Cc: Jernej Skrabec jernej.skrabec@siol.net --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index 0f75bb2d7f56..ecf0693e3018 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -144,7 +144,7 @@ struct ti_sn_bridge { struct device_node *host_node; struct mipi_dsi_device *dsi; struct clk *refclk; - struct drm_panel *panel; + struct drm_bridge *panel_bridge; struct gpio_desc *enable_gpio; struct regulator_bulk_data supplies[SN_REGULATOR_SUPPLY_NUM]; int dp_lanes; @@ -263,7 +263,7 @@ static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector) { struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
- return drm_panel_get_modes(pdata->panel, connector); + return drm_bridge_get_modes(pdata->panel_bridge, connector); }
static enum drm_mode_status @@ -395,9 +395,8 @@ static int ti_sn_bridge_attach(struct drm_bridge *bridge, pdata->dsi = dsi;
/* attach panel to bridge */ - drm_panel_attach(pdata->panel, &pdata->connector); - - return 0; + return drm_bridge_attach(bridge->encoder, pdata->panel_bridge, + bridge, flags);
err_dsi_attach: mipi_dsi_device_unregister(dsi); @@ -410,16 +409,12 @@ static void ti_sn_bridge_disable(struct drm_bridge *bridge) { struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(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 */ regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0); /* disable DP PLL */ regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0); - - drm_panel_unprepare(pdata->panel); }
static u32 ti_sn_bridge_get_dsi_freq(struct ti_sn_bridge *pdata) @@ -780,8 +775,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) @@ -811,8 +804,6 @@ static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge) */ regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG, HPD_DISABLE, HPD_DISABLE); - - drm_panel_prepare(pdata->panel); }
static void ti_sn_bridge_post_disable(struct drm_bridge *bridge) @@ -1163,6 +1154,8 @@ static int ti_sn_bridge_probe(struct i2c_client *client, const struct i2c_device_id *id) { struct ti_sn_bridge *pdata; + struct drm_bridge *bridge; + struct drm_panel *panel; int ret;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { @@ -1185,12 +1178,18 @@ static int ti_sn_bridge_probe(struct i2c_client *client, pdata->dev = &client->dev;
ret = drm_of_find_panel_or_bridge(pdata->dev->of_node, 1, 0, - &pdata->panel, NULL); + &panel, NULL); if (ret) { DRM_ERROR("could not find any panel node\n"); return ret; }
+ bridge = devm_drm_panel_bridge_add(pdata->dev, panel); + if (IS_ERR(bridge)) + return PTR_ERR(bridge); + + pdata->panel_bridge = bridge; + dev_set_drvdata(&client->dev, pdata);
pdata->enable_gpio = devm_gpiod_get(pdata->dev, "enable",
Hi Sam,
Thank you for the patch.
On Fri, Jul 03, 2020 at 09:24:17PM +0200, Sam Ravnborg wrote:
Same comment as earlier in this series regarding the flags.
I suppose attaching the panel bridge will be moved before creating the connector in a future patch series ?
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Sam Ravnborg