[PATCH v2 2/4] drm/bridge: ti-sn65dsi86: Make connector creation optional

[Kieran Bingham]

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 at ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas at 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 at ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas at 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/