Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver

From: Ajay kumar <ajaynumb@gmail.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: "linux-samsung-soc@vger.kernel.org"
	<linux-samsung-soc@vger.kernel.org>,
	Sean Paul <seanpaul@google.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	sunil joshi <joshi@samsung.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	Javier Martinez Canillas <javier@dowhile0.org>,
	Prashanth G <prashanth.g@samsung.com>,
	Ajay Kumar <ajaykumar.rs@samsung.com>,
	Rahul Sharma <rahul.sharma@samsung.com>
Subject: Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
Date: Tue, 22 Jul 2014 11:35:52 +0530	[thread overview]
Message-ID: <CAEC9eQOTtFPx-tK8Loo=V1LxSm0i5zwmdCv8khRQr7pYZEiF3A@mail.gmail.com> (raw)
In-Reply-To: <20140721144427.GA28336@ulmo>

On Mon, Jul 21, 2014 at 8:14 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Jul 21, 2014 at 08:06:01PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>> On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
>> <thierry.reding@gmail.com> wrote:
>> > On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
>> >> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
>> >> <thierry.reding@gmail.com> wrote:
>> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> >> >> From: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >> >>
>> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>> >> >>
>> >> >> Signed-off-by: Rahul Sharma <Rahul.Sharma@samsung.com>
>> >> >> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
>> >> >> ---
>> >> >>  drivers/gpu/drm/exynos/exynos_dp_core.c |    5 +++++
>> >> >>  1 file changed, 5 insertions(+)
>> >> >>
>> >> >> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> index 0ca6256..82e2942 100644
>> >> >> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
>> >> >> @@ -31,6 +31,7 @@
>> >> >>  #include <drm/drm_panel.h>
>> >> >>  #include <drm/bridge/ptn3460.h>
>> >> >>  #include <drm/bridge/panel_binder.h>
>> >> >> +#include <drm/bridge/ps8622.h>
>> >> >>
>> >> >>  #include "exynos_drm_drv.h"
>> >> >>  #include "exynos_dp_core.h"
>> >> >> @@ -999,6 +1000,10 @@ static int exynos_drm_attach_lcd_bridge(struct exynos_dp_device *dp,
>> >> >>       if (find_bridge("nxp,ptn3460", &bridge)) {
>> >> >>               bridge_chain = ptn3460_init(dp->drm_dev, encoder, bridge.client,
>> >> >>                                                               bridge.node);
>> >> >> +     } else if (find_bridge("parade,ps8622", &bridge) ||
>> >> >> +                             find_bridge("parade,ps8625", &bridge)) {
>> >> >> +             bridge_chain = ps8622_init(dp->drm_dev, encoder, bridge.client,
>> >> >> +                                                             bridge.node);
>> >> >>       }
>> >> >
>> >> > We really ought to be adding some sort of registry at some point.
>> >> > Otherwise every driver that wants to use bridges needs to come up with a
>> >> > similar set of helpers to instantiate them.
>> >> >
>> >> > Also you're making this driver depend on (now) two bridges, whereas it
>> >> > really shouldn't matter which exact types it supports. Bridges should be
>> >> > exposed via a generic interface.
>> >>
>> >> How about moving out the find_bridge() function into a common header file,
>> >> and also supporting the list of bridge_init declarations in the same file?
>> >>
>> >> We get bridge chip node from phandle, and then pass the same node
>> >> to find_bridge() which searches the list using of_device_is_compatible()
>> >> to call the appropriate bridge_init function?
>> >
>> > That could work, but it's still somewhat unusual and shouldn't be
>> > required. I think we'd be better of with some sort of registry like we
>> > have for panels. That would mean that a driver that wants to use a
>> > bridge would do something like this:
>> >
>> >         struct drm_bridge *bridge;
>> >         struct device_node *np;
>> >
>> >         np = of_parse_phandle(dev->of_node, "bridge", 0);
>> >         if (np) {
>> >                 bridge = of_drm_find_bridge(np);
>> >                 of_node_put(np);
>> >
>> >                 if (!bridge)
>> >                         return -EPROBE_DEFER;
>> >         }
>> >
>> > An alternative way would be to add a non-OF wrapper around this, like
>> > this for example:
>> Let me try the DT version first :)
>>
>> >         bridge = drm_bridge_get(dev, NULL);
>> >
>> > Which would be conceptually the same as clk_get() or regulator_get() and
>> > could be easily extended to support non-DT setups as well.
>> >
>> > As for bridge drivers I think we may have to rework things a little, so
>> > that a driver can call some sequence like this:
>> >
>> >         struct foo_bridge {
>> >                 struct drm_bridge base;
>> >                 ...
>> >         };
>> >
>> >         static const struct drm_bridge_funcs foo_bridge_funcs = {
>> >                 ...
>> >         };
>> >
>> >         static int foo_probe(...)
>> >         {
>> >                 struct foo_bridge *bridge;
>> >                 int err;
>> >
>> >                 bridge = devm_kzalloc(dev, sizeof(*bridge), GFP_KERNEL);
>> >                 if (!bridge)
>> >                         return -ENOMEM;
>> >
>> >                 /* setup bridge (return -EPROBE_DEFER if necessary, ...) */
>> >
>> >                 /* register bridge with DRM */
>> >                 drm_bridge_init(&bridge->base);
>> >                 bridge->base.dev = dev;
>> >                 bridge->base.funcs = &foo_bridge_funcs;
>> >
>> >                 err = drm_bridge_add(&bridge->base);
>> >                 if (err < 0)
>> >                         return err;
>> >
>> >                 dev_set_drvdata(dev, bridge);
>> >                 ...
>> >         }
>> >
>> > drm_bridge_add() would add the bridge to a global list of bridge devices
>> > which drm_bridge_get()/of_drm_find_bridge() can use to find the one that
>> > it needs. The above has the big advantage that it'sdev->mode_config.bridge_list completely
>> > independent of the underlying bus that the bridge is on. It could be I2C
>> > or SPI, platform device or PCI device.
>> >
>> > Thierry
>> Ok. This is all about making the bridge driver confine to the driver model.
>> But, I would need a drm_device pointer to register the bridge to DRM core.
>> How do I get it?
>
> Once you've obtained a reference to the DRM bridge from your driver you
> should carry it around until you've set up the DRM device. Then you can
> hook it up, which possibly requires a new function (since it's what the
> drm_bridge_init() used to do).

Ok. We can have 2 parts:

Non-DRM part:
In order to get a reference for drm_bridge in my encoder driver, I
should be using
of_drm_find_bridge(). In order to do that, I should have already added
the bridge
into a static list of bridges(using something like drm_bridge_add).
Also, bridge_funcs pointer is assigned to the bridge object in the
bridge driver itself.

DRM part:
Assuming that the bridge driver probe will take care of calling
drm_bridge_add(),
I can then call drm_bridge_init from encoder driver, with a drm_bridge pointer
and a drm_device pointer, which will actually register the bridge into DRM core.

Ajay