Re: [PATCH V2 7/8] drm/bridge: Add i2c based driver for ptn3460 bridge

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3193 bytes --]

On Wed, Jul 30, 2014 at 09:44:32PM +0530, Ajay kumar wrote:
> On Wed, Jul 30, 2014 at 9:10 PM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > On Wed, Jul 30, 2014 at 08:46:44PM +0530, Ajay kumar wrote:
> >> On Wed, Jul 30, 2014 at 5:35 PM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> > On Sat, Jul 26, 2014 at 12:52:09AM +0530, Ajay Kumar wrote:
> > [...]
> >> >> +int ptn3460_post_encoder_init(struct drm_bridge *bridge)
> >> >> +{
> >> >> +     struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
> >> >> +     int ret;
> >> >> +
> >> >> +     /* bridge is attached to encoder.
> >> >> +      * safe to remove it from the bridge_lookup table.
> >> >> +      */
> >> >> +     drm_bridge_remove_from_lookup(bridge);
> >> >
> >> > No, you should never do this. First, you're not adding it back to the
> >> > registry when the bridge is detached, so unloading and reloading the
> >> > display driver will fail. Second there should never be a need to remove
> >> > it from the registry as long as the driver itself is loaded. If you're
> >> > concerned about a single bridge being used multiple times, there's
> >> > already code to handle that in your previous patch:
> >> >
> >> >         int drm_bridge_attach_encoder(...)
> >> >         {
> >> >                 ...
> >> >
> >> >                 if (bridge->encoder)
> >> >                         return -EBUSY;
> >> >
> >> >                 ...
> >> >         }
> >> >
> >> > Generally the registry should contain a list of bridges that have been
> >> > registered, whether they're used or not is irrelevant.
> >> I was just wondering if it is ok to have a node in two independent lists?
> >> bridge_lookup_table and the other mode_config.bridge_list?
> >
> > Oh, it reuses the head field for the registry. I hadn't noticed before.
> > No, you certainly can't have the same node in two lists. Honestly I
> > don't quite understand why there was a need to expose drm_bridge as a
> > drm_mode_object in the first place since it's never exported to
> > userspace.
> >
> > So I think that perhaps we could simply get rid of the base field and
> > not tie in drm_bridge objects with the DRM object as we currently do.
> > But until Sean or Rob comment on this it might be better to simply add
> > another struct list_head field for the registry. That way both can
> > coexist and we can independently still decide to remove the base and
> > head fields if they're no longer needed.
> Ok. What shall I name the new list_head?

"list" would be a good choice in my opinion.

> > .get_modes() still needs to be done from the bridge because that is the
> > most closely connected to the display controller and therefore dictates
> > the timing that the display controller needs to generate.
> >
> > Querying the panel's .get_modes() might be useful to figure out which
> > emulation mode to use in the bridge.
> But, get_modes from panel returns me only the no_of_modes but
> not the actual mode structure. How do I compare the list of supported
> emulation modes?

You could iterate over the connector's probed_modes list which should
contain all the modes that the panel reported (after .get_modes() was
called).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]