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

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

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

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:

	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's completely
independent of the underlying bus that the bridge is on. It could be I2C
or SPI, platform device or PCI device.

Thierry

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