On Thu, Mar 30, 2023 at 12:15:49PM +0530, Jagan Teki wrote:
> On Wed, Mar 29, 2023 at 9:36 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 29, 2023 at 09:08:17PM +0530, Jagan Teki wrote:
> > > On Wed, Mar 29, 2023 at 8:29 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
> > > >
> > > > Hi,
> > > >
> > > > The patch prefix should be drm/sun4i:
> > >
> > > I did follow my previous prefix, I will update this.
> > >
> > > >
> > > > On Wed, Mar 29, 2023 at 06:49:29PM +0530, Jagan Teki wrote:
> > > > > Convert the encoder to bridge driver in order to standardize on a
> > > > > single API by supporting all varients of downstream bridge devices.
> > > >
> > > > Which variant, and why do we need to convert to a bridge to support all of them?
> > >
> > > Downstream bridge variants like DSI panel, DSI bridge and
> > > I2C-Configured DSI bridges. Bridge conversion would be required for
> > > the DSI host to access the more variety and complex downstream bridges
> > > in a standardized bridge chain way which is indeed complex for encoder
> > > driven DSI hosts.
> > >
> > > >
> > > > > The drm_encoder can't be removed as it's exposed to userspace, so it
> > > > > then becomes a dumb encoder, without any operation implemented.
> > > > >
> > > > > Tested on DSI Panel, DSI Bridge, I2C-Configured DSI Bridge.
> > > > >
> > > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx>
> > > >
> > > > [...]
> > > >
> > > > > +static const struct component_ops sun6i_dsi_ops;
> > > > > +
> > > > > static int sun6i_dsi_attach(struct mipi_dsi_host *host,
> > > > > struct mipi_dsi_device *device)
> > > > > {
> > > > > struct sun6i_dsi *dsi = host_to_sun6i_dsi(host);
> > > > > - struct drm_panel *panel = of_drm_find_panel(device->dev.of_node);
> > > >
> > > > That one looks unrelated. Why do you need that change?
> > >
> > > This was replaced with drmm_of_dsi_get_bridge for lookup of both panel
> > > and bridge. I think I will separate this into another patch.
> >
> > So, it looks to me that you're doing two (unrelated) things in that patch:
>
> Correct.
>
> >
> > - You modify the existing driver to be a bridge
>
> Yes, Convert to bridge driver - register drm_bridge_add and replace
> encoder ops with bridge ops.
>
> >
> > - And you support downstream device being bridges.
>
> Yes, Support the downstream bridge. (If I'm correct we can still use
> encoder ops with this).
>
> If we see the hierarchy of support it would
> 1. support the downstream bridge.
> 2. convert to the bridge driver.
>
> >
> > Both are orthogonal, can (and should!) be done separately, and I'm
> > pretty sure you don't actually need to do the former at all.
>
> Do you mean converting to bridge driver is not needed?
Yes, and given the current state of the DCS-in-HS discussion, I even
think it's does more harm than good.
Maxime
