Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
On Tue, Jul 22, 2014 at 11:35:52AM +0530, Ajay kumar wrote:
> On Mon, Jul 21, 2014 at 8:14 PM, Thierry Reding
> 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
> >> 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
> >> >> wrote:
> >> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> >> >> From: Rahul Sharma
> >> >> >>
> >> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >> >> >>
> >> >> >> Signed-off-by: Rahul Sharma
> >> >> >> Signed-off-by: Ajay Kumar
> >> >> >> ---
> >> >> >> 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
> >> >> >> #include
> >> >> >> #include
> >> >> >> +#include
> >> >> >>
> >> >> >> #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_f
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
On Mon, Jul 21, 2014 at 8:14 PM, Thierry Reding
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
>> 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
>> >> wrote:
>> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> >> >> From: Rahul Sharma
>> >> >>
>> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>> >> >>
>> >> >> Signed-off-by: Rahul Sharma
>> >> >> Signed-off-by: Ajay Kumar
>> >> >> ---
>> >> >> 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
>> >> >> #include
>> >> >> #include
>> >> >> +#include
>> >> >>
>> >> >> #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_
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
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
> 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
> >> wrote:
> >> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> >> From: Rahul Sharma
> >> >>
> >> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >> >>
> >> >> Signed-off-by: Rahul Sharma
> >> >> Signed-off-by: Ajay Kumar
> >> >> ---
> >> >> 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
> >> >> #include
> >> >> #include
> >> >> +#include
> >> >>
> >> >> #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
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
Hi Thierry,
On Mon, Jul 21, 2014 at 6:24 PM, Thierry Reding
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
>> wrote:
>> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> >> From: Rahul Sharma
>> >>
>> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>> >>
>> >> Signed-off-by: Rahul Sharma
>> >> Signed-off-by: Ajay Kumar
>> >> ---
>> >> 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
>> >> #include
>> >> #include
>> >> +#include
>> >>
>> >> #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?
Ajay
--
To unsubscribe from this list: send the lin
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
On Mon, Jul 21, 2014 at 04:58:25PM +0530, Ajay kumar wrote:
> On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
> wrote:
> > On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> >> From: Rahul Sharma
> >>
> >> This patch adds ps8622 lvds bridge discovery code to the dp driver.
> >>
> >> Signed-off-by: Rahul Sharma
> >> Signed-off-by: Ajay Kumar
> >> ---
> >> 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
> >> #include
> >> #include
> >> +#include
> >>
> >> #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
pgpkq1_Z9U6wW.pgp
Description: PGP signature
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
On Mon, Jul 21, 2014 at 12:40 PM, Thierry Reding
wrote:
> On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
>> From: Rahul Sharma
>>
>> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>>
>> Signed-off-by: Rahul Sharma
>> Signed-off-by: Ajay Kumar
>> ---
>> 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
>> #include
>> #include
>> +#include
>>
>> #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?
Ajay
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH V5 12/12] drm/exynos: Add ps8622 lvds bridge discovery to DP driver
On Fri, Jul 18, 2014 at 02:13:58AM +0530, Ajay Kumar wrote:
> From: Rahul Sharma
>
> This patch adds ps8622 lvds bridge discovery code to the dp driver.
>
> Signed-off-by: Rahul Sharma
> Signed-off-by: Ajay Kumar
> ---
> 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
> #include
> #include
> +#include
>
> #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.
Thierry
pgpflAgLNpAhe.pgp
Description: PGP signature
