[RFC,v2,1/4] drm: Introduce generic probe function for component based masters.
diff mbox

Message ID 1445257313-7147-2-git-send-email-Liviu.Dudau@arm.com
State New
Headers show

Commit Message

Liviu Dudau Oct. 19, 2015, 12:21 p.m. UTC
A lot of component based DRM drivers use a variant of the same code
as the probe function. They bind the crtc ports in the first iteration
and then scan through the child nodes and bind the encoders attached
to the remote endpoints. Factor the common code into a separate
function called drm_of_component_probe() in order to increase code
reuse.

Cc: David Airlie <airlied@linux.ie>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/drm_of.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_of.h     | 12 +++++++
 2 files changed, 104 insertions(+)

Comments

Russell King - ARM Linux Oct. 19, 2015, 12:25 p.m. UTC | #1
On Mon, Oct 19, 2015 at 01:21:50PM +0100, Liviu Dudau wrote:
> A lot of component based DRM drivers use a variant of the same code
> as the probe function. They bind the crtc ports in the first iteration
> and then scan through the child nodes and bind the encoders attached
> to the remote endpoints. Factor the common code into a separate
> function called drm_of_component_probe() in order to increase code
> reuse.
> 
> Cc: David Airlie <airlied@linux.ie>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> ---
>  drivers/gpu/drm/drm_of.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_of.h     | 12 +++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index be38840..cf16240 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,3 +1,4 @@
> +#include <linux/component.h>
>  #include <linux/export.h>
>  #include <linux/list.h>
>  #include <linux/of_graph.h>
> @@ -61,3 +62,94 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  	return possible_crtcs;
>  }
>  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> +
> +/**
> + * drm_of_component_probe - Generic probe function for a component based master
> + * @dev: master device containing the OF node
> + * @compare_of: compare function used for matching components
> + * @master_ops: component master ops to be used
> + *
> + * Parse the platform device OF node and bind all the components associated
> + * with the master. Interface ports are added before the encoders in order to
> + * satisfy their .bind requirements
> + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> + *
> + * Returns zero if successful, or one of the standard error codes if it fails.
> + */
> +int drm_of_component_probe(struct device *dev,
> +			   int (*compare_of)(struct device *, void *),
> +			   const struct component_master_ops *master_ops)
> +{
...
> +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> +	if (ret)
> +		return ret;

Please don't move this into here, it's completely inappropriate.  Just
because something makes use of this does not mean they only support
32-bit DMA.  Besides, this has nothing to do with whether or not it's
OF-based or not.
> +
> +	return component_master_add_with_match(dev, master_ops, match);
> +}
> +EXPORT_SYMBOL(drm_of_component_probe);
> diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> index 2441f71..7c0c05b 100644
> --- a/include/drm/drm_of.h
> +++ b/include/drm/drm_of.h
> @@ -1,18 +1,30 @@
>  #ifndef __DRM_OF_H__
>  #define __DRM_OF_H__
>  
> +struct component_master_ops;
> +struct device;
>  struct drm_device;
>  struct device_node;
>  
>  #ifdef CONFIG_OF
>  extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  					   struct device_node *port);
> +extern int drm_of_component_probe(struct device *dev,
> +				  int (*compare_of)(struct device *, void *),
> +				  const struct component_master_ops *master_ops);
>  #else
>  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
>  						  struct device_node *port)
>  {
>  	return 0;
>  }
> +
> +static inline int drm_of_component_probe(struct device *dev,
> +					int (*compare_of)(struct device *, void *),
> +					 const struct component_master_ops *master_ops)

Some of these lines will fail checkpatch.  It would be nice to avoid
lines longer than 80 columns.
Liviu Dudau Oct. 19, 2015, 1:02 p.m. UTC | #2
On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 01:21:50PM +0100, Liviu Dudau wrote:
> > A lot of component based DRM drivers use a variant of the same code
> > as the probe function. They bind the crtc ports in the first iteration
> > and then scan through the child nodes and bind the encoders attached
> > to the remote endpoints. Factor the common code into a separate
> > function called drm_of_component_probe() in order to increase code
> > reuse.
> > 
> > Cc: David Airlie <airlied@linux.ie>
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > ---
> >  drivers/gpu/drm/drm_of.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_of.h     | 12 +++++++
> >  2 files changed, 104 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > index be38840..cf16240 100644
> > --- a/drivers/gpu/drm/drm_of.c
> > +++ b/drivers/gpu/drm/drm_of.c
> > @@ -1,3 +1,4 @@
> > +#include <linux/component.h>
> >  #include <linux/export.h>
> >  #include <linux/list.h>
> >  #include <linux/of_graph.h>
> > @@ -61,3 +62,94 @@ uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >  	return possible_crtcs;
> >  }
> >  EXPORT_SYMBOL(drm_of_find_possible_crtcs);
> > +
> > +/**
> > + * drm_of_component_probe - Generic probe function for a component based master
> > + * @dev: master device containing the OF node
> > + * @compare_of: compare function used for matching components
> > + * @master_ops: component master ops to be used
> > + *
> > + * Parse the platform device OF node and bind all the components associated
> > + * with the master. Interface ports are added before the encoders in order to
> > + * satisfy their .bind requirements
> > + * See Documentation/devicetree/bindings/graph.txt for the bindings.
> > + *
> > + * Returns zero if successful, or one of the standard error codes if it fails.
> > + */
> > +int drm_of_component_probe(struct device *dev,
> > +			   int (*compare_of)(struct device *, void *),
> > +			   const struct component_master_ops *master_ops)
> > +{
> ...
> > +	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
> > +	if (ret)
> > +		return ret;
> 
> Please don't move this into here, it's completely inappropriate.  Just
> because something makes use of this does not mean they only support
> 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> OF-based or not.

Understood. My thinking process was that component-based drivers are all
OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
present in 2 out of 3 drivers that are converted, so it looks to be common
enough that adding it to armada would not hurt. It was all done in the name of
collecting common code in a single function.

> > +
> > +	return component_master_add_with_match(dev, master_ops, match);
> > +}
> > +EXPORT_SYMBOL(drm_of_component_probe);
> > diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
> > index 2441f71..7c0c05b 100644
> > --- a/include/drm/drm_of.h
> > +++ b/include/drm/drm_of.h
> > @@ -1,18 +1,30 @@
> >  #ifndef __DRM_OF_H__
> >  #define __DRM_OF_H__
> >  
> > +struct component_master_ops;
> > +struct device;
> >  struct drm_device;
> >  struct device_node;
> >  
> >  #ifdef CONFIG_OF
> >  extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >  					   struct device_node *port);
> > +extern int drm_of_component_probe(struct device *dev,
> > +				  int (*compare_of)(struct device *, void *),
> > +				  const struct component_master_ops *master_ops);
> >  #else
> >  static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
> >  						  struct device_node *port)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline int drm_of_component_probe(struct device *dev,
> > +					int (*compare_of)(struct device *, void *),
> > +					 const struct component_master_ops *master_ops)
> 
> Some of these lines will fail checkpatch.  It would be nice to avoid
> lines longer than 80 columns.

I'm torn between following the 80 columns rule, aligning parameters to the
opening brace, keeping the return value on the same line as the function name
and having this high res monitor that can make code more readable by going to
86 columns. :)

I will update the patch to trim down the line width, thanks for reviewing it!

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
>
Russell King - ARM Linux Oct. 19, 2015, 1:26 p.m. UTC | #3
On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > Please don't move this into here, it's completely inappropriate.  Just
> > because something makes use of this does not mean they only support
> > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > OF-based or not.
> 
> Understood. My thinking process was that component-based drivers are all
> OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> present in 2 out of 3 drivers that are converted, so it looks to be common
> enough that adding it to armada would not hurt. It was all done in the name of
> collecting common code in a single function.

Which is an utterly crap reason.

It's also not appropriate.  I'm really not sure why you think that moving
this here would in any way be appropriate - from my point of view, the
mere proposal is utterly insane.

The "container" device does not do any DMA, so it's inappropriate for
it to have DMA masks set or negotiated on it.  So, actually, no one
should be setting the DMA mask for their container device.  It's wrong.

What if we have a 64-bit OF based platform wanting to use the component
helper, and they want to call this function?  You prevent them doing so
by moving this into here, because they're then forced down to 32-bit DMA.
Please, get rid of it, and leave this crappiness in the respective
drivers.
Liviu Dudau Oct. 19, 2015, 1:32 p.m. UTC | #4
On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > > Please don't move this into here, it's completely inappropriate.  Just
> > > because something makes use of this does not mean they only support
> > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > > OF-based or not.
> > 
> > Understood. My thinking process was that component-based drivers are all
> > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> > present in 2 out of 3 drivers that are converted, so it looks to be common
> > enough that adding it to armada would not hurt. It was all done in the name of
> > collecting common code in a single function.
> 
> Which is an utterly crap reason.
> 
> It's also not appropriate.  I'm really not sure why you think that moving
> this here would in any way be appropriate - from my point of view, the
> mere proposal is utterly insane.

The proposal is to collect similar code present in DRM drivers that act as
component masters in one place in order to reduce code duplication. I want to
add another DRM driver that will make use of the same code and did not see
any reason to copy paste one of the slightly similar versions that are now
peppered in the drivers/drm drivers.

Sorry for making you believe this is more than just code cleanup, but the cover
letter clearly stated in the title the intent.

> 
> The "container" device does not do any DMA, so it's inappropriate for
> it to have DMA masks set or negotiated on it.  So, actually, no one
> should be setting the DMA mask for their container device.  It's wrong.
> 
> What if we have a 64-bit OF based platform wanting to use the component
> helper, and they want to call this function?  You prevent them doing so
> by moving this into here, because they're then forced down to 32-bit DMA.
> Please, get rid of it, and leave this crappiness in the respective
> drivers.

OK, will do.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
>
Russell King - ARM Linux Oct. 19, 2015, 1:38 p.m. UTC | #5
On Mon, Oct 19, 2015 at 02:32:55PM +0100, Liviu Dudau wrote:
> On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> > > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > > > Please don't move this into here, it's completely inappropriate.  Just
> > > > because something makes use of this does not mean they only support
> > > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > > > OF-based or not.
> > > 
> > > Understood. My thinking process was that component-based drivers are all
> > > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> > > present in 2 out of 3 drivers that are converted, so it looks to be common
> > > enough that adding it to armada would not hurt. It was all done in the name of
> > > collecting common code in a single function.
> > 
> > Which is an utterly crap reason.
> > 
> > It's also not appropriate.  I'm really not sure why you think that moving
> > this here would in any way be appropriate - from my point of view, the
> > mere proposal is utterly insane.
> 
> The proposal is to collect similar code present in DRM drivers that act as
> component masters in one place in order to reduce code duplication. I want to
> add another DRM driver that will make use of the same code and did not see
> any reason to copy paste one of the slightly similar versions that are now
> peppered in the drivers/drm drivers.

I know why people want to set the DMA mask on the "software" component,
and that's drivers/gpu/drm/drm_gem_cma_helper.c, which wants to use
drm->dev as the struct device to allocate memory against.  That's not
really appropriate, and probably ought to get fixed.
Daniel Vetter Oct. 19, 2015, 2:42 p.m. UTC | #6
On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > > Please don't move this into here, it's completely inappropriate.  Just
> > > because something makes use of this does not mean they only support
> > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > > OF-based or not.
> > 
> > Understood. My thinking process was that component-based drivers are all
> > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> > present in 2 out of 3 drivers that are converted, so it looks to be common
> > enough that adding it to armada would not hurt. It was all done in the name of
> > collecting common code in a single function.
> 
> Which is an utterly crap reason.
> 
> It's also not appropriate.  I'm really not sure why you think that moving
> this here would in any way be appropriate - from my point of view, the
> mere proposal is utterly insane.
> 
> The "container" device does not do any DMA, so it's inappropriate for
> it to have DMA masks set or negotiated on it.  So, actually, no one
> should be setting the DMA mask for their container device.  It's wrong.

I think (and my opinion doesn't carry as much wheight here on dri-devel
than intel-gfx) the above is over the top bashing of a new contributor to
drm who really seems trying to do right. I think that's unecessary,
especially since you follow up with the reasonable reply below.

Thanks, Daniel

> What if we have a 64-bit OF based platform wanting to use the component
> helper, and they want to call this function?  You prevent them doing so
> by moving this into here, because they're then forced down to 32-bit DMA.
> Please, get rid of it, and leave this crappiness in the respective
> drivers.
> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
Russell King - ARM Linux Oct. 19, 2015, 2:50 p.m. UTC | #7
On Mon, Oct 19, 2015 at 04:42:25PM +0200, Daniel Vetter wrote:
> On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
> > On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> > > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > > > Please don't move this into here, it's completely inappropriate.  Just
> > > > because something makes use of this does not mean they only support
> > > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > > > OF-based or not.
> > > 
> > > Understood. My thinking process was that component-based drivers are all
> > > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> > > present in 2 out of 3 drivers that are converted, so it looks to be common
> > > enough that adding it to armada would not hurt. It was all done in the name of
> > > collecting common code in a single function.
> > 
> > Which is an utterly crap reason.
> > 
> > It's also not appropriate.  I'm really not sure why you think that moving
> > this here would in any way be appropriate - from my point of view, the
> > mere proposal is utterly insane.
> > 
> > The "container" device does not do any DMA, so it's inappropriate for
> > it to have DMA masks set or negotiated on it.  So, actually, no one
> > should be setting the DMA mask for their container device.  It's wrong.
> 
> I think (and my opinion doesn't carry as much wheight here on dri-devel
> than intel-gfx) the above is over the top bashing of a new contributor to
> drm who really seems trying to do right. I think that's unecessary,
> especially since you follow up with the reasonable reply below.

It's justified because it took _two_ messages to get the point across.
The first one asking nicely didn't make the necessary impact.
Emil Velikov Oct. 19, 2015, 3:04 p.m. UTC | #8
On 19 October 2015 at 15:50, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Oct 19, 2015 at 04:42:25PM +0200, Daniel Vetter wrote:
>> On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
>> > On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
>> > > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
>> > > > Please don't move this into here, it's completely inappropriate.  Just
>> > > > because something makes use of this does not mean they only support
>> > > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
>> > > > OF-based or not.
>> > >
>> > > Understood. My thinking process was that component-based drivers are all
>> > > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
>> > > present in 2 out of 3 drivers that are converted, so it looks to be common
>> > > enough that adding it to armada would not hurt. It was all done in the name of
>> > > collecting common code in a single function.
>> >
>> > Which is an utterly crap reason.
>> >
>> > It's also not appropriate.  I'm really not sure why you think that moving
>> > this here would in any way be appropriate - from my point of view, the
>> > mere proposal is utterly insane.
>> >
>> > The "container" device does not do any DMA, so it's inappropriate for
>> > it to have DMA masks set or negotiated on it.  So, actually, no one
>> > should be setting the DMA mask for their container device.  It's wrong.
>>
>> I think (and my opinion doesn't carry as much wheight here on dri-devel
>> than intel-gfx) the above is over the top bashing of a new contributor to
>> drm who really seems trying to do right. I think that's unecessary,
>> especially since you follow up with the reasonable reply below.
>
> It's justified because it took _two_ messages to get the point across.
> The first one asking nicely didn't make the necessary impact.
>
Unlike Daniel, I carry little to no weight here, yet bashing on people
if they don't get things correct the first time is inconsiderable.
We are people - we can misread/misinterpret things, have a headache
and/or just a bad day. If that makes you angry/annoyed, please don't
reply straight away, but give it a few hours/day for the emotions to
settle.

Thanks
Emil
Liviu Dudau Oct. 19, 2015, 3:07 p.m. UTC | #9
On Mon, Oct 19, 2015 at 03:50:27PM +0100, Russell King - ARM Linux wrote:
> On Mon, Oct 19, 2015 at 04:42:25PM +0200, Daniel Vetter wrote:
> > On Mon, Oct 19, 2015 at 02:26:38PM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Oct 19, 2015 at 02:02:58PM +0100, Liviu Dudau wrote:
> > > > On Mon, Oct 19, 2015 at 01:25:37PM +0100, Russell King - ARM Linux wrote:
> > > > > Please don't move this into here, it's completely inappropriate.  Just
> > > > > because something makes use of this does not mean they only support
> > > > > 32-bit DMA.  Besides, this has nothing to do with whether or not it's
> > > > > OF-based or not.
> > > > 
> > > > Understood. My thinking process was that component-based drivers are all
> > > > OF-enabled (how else do you make use of the framework?) and 32-bit DMA is
> > > > present in 2 out of 3 drivers that are converted, so it looks to be common
> > > > enough that adding it to armada would not hurt. It was all done in the name of
> > > > collecting common code in a single function.
> > > 
> > > Which is an utterly crap reason.
> > > 
> > > It's also not appropriate.  I'm really not sure why you think that moving
> > > this here would in any way be appropriate - from my point of view, the
> > > mere proposal is utterly insane.
> > > 
> > > The "container" device does not do any DMA, so it's inappropriate for
> > > it to have DMA masks set or negotiated on it.  So, actually, no one
> > > should be setting the DMA mask for their container device.  It's wrong.
> > 
> > I think (and my opinion doesn't carry as much wheight here on dri-devel
> > than intel-gfx) the above is over the top bashing of a new contributor to
> > drm who really seems trying to do right. I think that's unecessary,
> > especially since you follow up with the reasonable reply below.
> 
> It's justified because it took _two_ messages to get the point across.
> The first one asking nicely didn't make the necessary impact.

Russell, I've got your point and I have accepted it after first message. Go
back to my initial reply and re-read it. It starts with "Understood. ...."

I'm not looking for a flame war. I am more interested in knowing if you think
that the armada code re-org makes sense or the use of -EINVAL to mean that dev->of_node
is missing.

Coming to armada changes, I would also like to know if pdev->dev.platform_data is
in use at all. If so, are there any examples where that is being setup? Because I
think the "missing 'ports' property" message is missleading.

Best regards,
Liviu

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
>
Russell King - ARM Linux Oct. 19, 2015, 3:21 p.m. UTC | #10
On Mon, Oct 19, 2015 at 04:04:27PM +0100, Emil Velikov wrote:
> Unlike Daniel, I carry little to no weight here, yet bashing on people
> if they don't get things correct the first time is inconsiderable.
> We are people - we can misread/misinterpret things, have a headache
> and/or just a bad day. If that makes you angry/annoyed, please don't
> reply straight away, but give it a few hours/day for the emotions to
> settle.

Sorry, I can't do that.  I get far too much email.  If I leave something,
it gets buried, and I'll never reply.  Then people get annoyed that I
don't reply to emails.

There's no way to win here, sorry.

Patch
diff mbox

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index be38840..cf16240 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,3 +1,4 @@ 
+#include <linux/component.h>
 #include <linux/export.h>
 #include <linux/list.h>
 #include <linux/of_graph.h>
@@ -61,3 +62,94 @@  uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 	return possible_crtcs;
 }
 EXPORT_SYMBOL(drm_of_find_possible_crtcs);
+
+/**
+ * drm_of_component_probe - Generic probe function for a component based master
+ * @dev: master device containing the OF node
+ * @compare_of: compare function used for matching components
+ * @master_ops: component master ops to be used
+ *
+ * Parse the platform device OF node and bind all the components associated
+ * with the master. Interface ports are added before the encoders in order to
+ * satisfy their .bind requirements
+ * See Documentation/devicetree/bindings/graph.txt for the bindings.
+ *
+ * Returns zero if successful, or one of the standard error codes if it fails.
+ */
+int drm_of_component_probe(struct device *dev,
+			   int (*compare_of)(struct device *, void *),
+			   const struct component_master_ops *master_ops)
+{
+	struct device_node *ep, *port, *remote;
+	struct component_match *match = NULL;
+	int i, ret;
+
+	if (!dev->of_node)
+		return -EINVAL;
+
+	/*
+	 * Bind the crtc's ports first, so that drm_of_find_possible_crtcs()
+	 * called from encoder's .bind callbacks works as expected
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		component_match_add(dev, &match, compare_of, port);
+		of_node_put(port);
+	}
+
+	if (i == 0) {
+		dev_err(dev, "missing 'ports' property\n");
+		return -ENODEV;
+	}
+
+	if (!match) {
+		dev_err(dev, "no available port\n");
+		return -ENODEV;
+	}
+
+	/*
+	 * For bound crtcs, bind the encoders attached to their remote endpoint
+	 */
+	for (i = 0; ; i++) {
+		port = of_parse_phandle(dev->of_node, "ports", i);
+		if (!port)
+			break;
+
+		if (!of_device_is_available(port->parent)) {
+			of_node_put(port);
+			continue;
+		}
+
+		for_each_child_of_node(port, ep) {
+			remote = of_graph_get_remote_port_parent(ep);
+			if (!remote || !of_device_is_available(remote)) {
+				of_node_put(remote);
+				continue;
+			} else if (!of_device_is_available(remote->parent)) {
+				dev_warn(dev, "parent device of %s is not available\n",
+					 remote->full_name);
+				of_node_put(remote);
+				continue;
+			}
+
+			component_match_add(dev, &match, compare_of, remote);
+			of_node_put(remote);
+		}
+		of_node_put(port);
+	}
+
+	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	if (ret)
+		return ret;
+
+	return component_master_add_with_match(dev, master_ops, match);
+}
+EXPORT_SYMBOL(drm_of_component_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 2441f71..7c0c05b 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -1,18 +1,30 @@ 
 #ifndef __DRM_OF_H__
 #define __DRM_OF_H__
 
+struct component_master_ops;
+struct device;
 struct drm_device;
 struct device_node;
 
 #ifdef CONFIG_OF
 extern uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 					   struct device_node *port);
+extern int drm_of_component_probe(struct device *dev,
+				  int (*compare_of)(struct device *, void *),
+				  const struct component_master_ops *master_ops);
 #else
 static inline uint32_t drm_of_find_possible_crtcs(struct drm_device *dev,
 						  struct device_node *port)
 {
 	return 0;
 }
+
+static inline int drm_of_component_probe(struct device *dev,
+					int (*compare_of)(struct device *, void *),
+					 const struct component_master_ops *master_ops)
+{
+	return -EINVAL;
+}
 #endif
 
 #endif /* __DRM_OF_H__ */