Hi Doug,
Quoting Doug Anderson (2022-03-07 23:21:28)
Hi,
On Mon, Mar 7, 2022 at 10:00 AM Kieran Bingham kieran.bingham+renesas@ideasonboard.com wrote:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Now that the driver supports the connector-related bridge operations, make the connector creation optional. This enables usage of the sn65dsi86 with the DRM bridge connector helper.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
Changes since v1:
- None
drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-)
Can you please CC me on this series next time you send it out? I was pretty involved in previous reviews here. Luckily I got CCed on one of the patches so I knew to look, but since that one is (probably) getting dropped it'd be good to make sure I was on the others. It's also pretty important to include +Sam Ravnborg since there's a lot of overlap with his series [1]:
Absolutely - I was assuming you were the main target for reviews. I'm sorry - I also assumed get_maintainer.pl had / would add you - and I didn't check getting the patches out last night.
I'll be sure to manually add you.
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index ffb6c04f6c46..29f5f7123ed9 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -745,11 +745,6 @@ static int (struct drm_bridge *bridge, struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge); int ret;
if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {DRM_ERROR("Fix bridge driver to make connector optional!");return -EINVAL;}That won't work without some other fixes, sorry!
Ok ;-)
To be clear, my goal here has been to get the IRQ HPD working - so my main focus is there. I guess I now have to handle custodianship of these dependencies somehow too though.
The problem is that you'll break ti_sn_bridge_get_bpp(). That absolutely relies on having our own connector so you need to fix that _before_ making the connector optional.
Rob Clark made an attempt on this [2] but Laurent didn't like the solution he came up with. Sam's series that I mentioned [1] also made an attempt at this, specifically in patch 5 ("drm/bridge: ti-sn65dsi86: Fetch bpc via drm_bridge_state") of his series. Unfortunately, it didn't work because the "output_bus_cfg.format" wasn't set to anything. Personally I wouldn't mind Rob's solution as a stopgap if Laurent removes his NAK. Then we're not stuck while someone tries to find time to fix this correctly.
Ok - all of that is going to take some time for me to review and process.
One last problem here is that your patch doesn't actually apply to drm-misc/drm-misc-next, which is likely where it would land. I believe it conflicts with my recent commit e283820cbf80 ("drm/bridge: ti-sn65dsi86: Use drm_bridge_connector"). It looks like you based your series on mainline, but you should really be targeting drm-misc-next.
Ah sorry - I thought I had merged in drm-misc-next, but it was a week ago or so so I guess I'm already outdated.
I'll cleanup for a new base now.
-Doug
[1] https://lore.kernel.org/r/20220206154405.1243333-1-sam@ravnborg.org/ [2] https://lore.kernel.org/all/20210920225801.227211-4-robdclark@gmail.com/