Re: [PATCH v3 2/3] drm/bridge: ti-sn65dsi86: Implement bridge connector operations

[Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>]

Hi Doug,

Quoting Doug Anderson (2022-03-10 23:10:20)
> Hi,
> 
> On Thu, Mar 10, 2022 at 7:22 AM Kieran Bingham
> <kieran.bingham+renesas@ideasonboard.com> wrote:
> >
> > From: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > Implement the bridge connector-related .get_edid() operation, and report
> > the related bridge capabilities and type.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - The connector .get_modes() operation doesn't rely on EDID anymore,
> >   __ti_sn_bridge_get_edid() and ti_sn_bridge_get_edid() got merged
> >   together
> >  - Fix on top of Sam Ravnborg's DRM_BRIDGE_STATE_OPS
> >
> > Changes since v2: [Kieran]
> >  - Only support EDID on DRM_MODE_CONNECTOR_DisplayPort modes.
> >
> >  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > index 93b54fcba8ba..d581c820e5d8 100644
> > --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> > @@ -1135,10 +1135,24 @@ static void ti_sn_bridge_atomic_post_disable(struct drm_bridge *bridge,
> >         pm_runtime_put_sync(pdata->dev);
> >  }
> >
> > +static struct edid *ti_sn_bridge_get_edid(struct drm_bridge *bridge,
> > +                                         struct drm_connector *connector)
> > +{
> > +       struct ti_sn65dsi86 *pdata = bridge_to_ti_sn65dsi86(bridge);
> > +       struct edid *edid;
> > +
> > +       pm_runtime_get_sync(pdata->dev);
> > +       edid = drm_get_edid(connector, &pdata->aux.ddc);
> > +       pm_runtime_put_autosuspend(pdata->dev);
> 
> I'm 99% sure that the pm_runtime calls here are not needed and can be
> removed.. The AUX transfer function handles the pm_runtime_get_sync()
> and it also does the "put" with autosuspend so that the whole EDID can
> be read without constantly powering the bridge up and down again.

Yes, digging through I agree - It does look like this may be the case.

ti_sn_aux_transfer() certainly looks like it handles the pm_runtime_
calls, and drm_get_edid() looks like it goes through there from the core
using the standard i2c interface, with nothing else expected to touch
the hw between.

So that more or less simplifies this function to just 
	return drm_get_edid(connector, &pdata->aux.ddc);

Thanks
--
Kieran