[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Mark yao]

On 2015年11月09日 19:57, Liviu Dudau wrote:
> Meanwhile, what is your suggestion regarding the patchset. I've seen David has
> sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
> revert for rockchip commit and then patch later the function?
Hi Liviu
 I had sent a patch to revert this rockchip commit, verfied rockchip 
drm works. So we can do the fix later.

Thanks all the same. :-)

-- ï¼­ark Yao
-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Mark yao]

On 2015年11月10日 03:49, Heiko Stuebner wrote:
> Hi Liviu,
>
> Am Montag, 9. November 2015, 12:07:20 schrieb Liviu Dudau:
>> On Mon, Nov 09, 2015 at 12:03:35PM +, Russell King - ARM Linux wrote:
>>> On Mon, Nov 09, 2015 at 11:57:27AM +, Liviu Dudau wrote:
 Meanwhile, what is your suggestion regarding the patchset. I've seen David 
 has
 sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
 revert for rockchip commit and then patch later the function?
>>> It definitely needs to be fixed, and I'd suggest its early enough in the
>>> -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe()
>>> to take two compare functions.
>> I still don't have a Rockchip board to test the patch, so I need to find out
>> someone willing to test them. Mark?
> I of course also have a plethora of rockchip boards, so can test stuff
> as well.
>
>
> Heiko
>
>
>
>
Hi Liviu
Sorry to reply late , of course I have rockchip boards and can do 
the test.

Hi Russell and Philipp,
 Thanks for your analysis, let me know what happen.

-- 
ï¼­ark Yao

-- next part --
An HTML attachment was scrubbed...
URL: 

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Tue, Nov 10, 2015 at 04:53:09PM +0800, Mark yao wrote:
>On 2015年11月09日 19:57, Liviu Dudau wrote:
> 
>  Meanwhile, what is your suggestion regarding the patchset. I've seen David 
> has
>  sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
>  revert for rockchip commit and then patch later the function?
> 
>Hi Liviu
>    I had sent a patch to revert this rockchip commit, verfied rockchip 
> drm works. So we can do the fix later.

Yeah, and I'm thinking about ACK-ing it. On one hand I would like to
have this fixed, on the other hand I don't want the rockchip platform
to be broken. But looking at the rest of the changes I need to do it
doesn't feel right to make you wait for them.

Best regards,
Liviu

> 
>Thanks all the same. :-)
> 
>-- ï¼­ark Yao

> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Heiko Stuebner]

Hi Liviu,

Am Montag, 9. November 2015, 12:07:20 schrieb Liviu Dudau:
> On Mon, Nov 09, 2015 at 12:03:35PM +, Russell King - ARM Linux wrote:
> > On Mon, Nov 09, 2015 at 11:57:27AM +, Liviu Dudau wrote:
> > > Meanwhile, what is your suggestion regarding the patchset. I've seen 
> > > David has
> > > sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
> > > revert for rockchip commit and then patch later the function?
> > 
> > It definitely needs to be fixed, and I'd suggest its early enough in the
> > -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe()
> > to take two compare functions.
> 
> I still don't have a Rockchip board to test the patch, so I need to find out
> someone willing to test them. Mark?

I of course also have a plethora of rockchip boards, so can test stuff
as well.


Heiko

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Mark yao]

On 2015年10月19日 23:07, 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 
> Signed-off-by: Liviu Dudau 
> ---
>   drivers/gpu/drm/drm_of.c | 88 
> 
>   include/drm/drm_of.h | 13 +++
>   2 files changed, 101 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> index be38840..493c05c 100644
> --- a/drivers/gpu/drm/drm_of.c
> +++ b/drivers/gpu/drm/drm_of.c
> @@ -1,3 +1,4 @@
> +#include 
>   #include 
>   #include 
>   #include 
> @@ -61,3 +62,90 @@ 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 *m_ops)
> +{
> + struct device_node *ep, *port, *remote;
> + struct component_match *match = NULL;
> + int i;
> +
> + 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, , compare_of, port);
Hi Liviu
   Rockchip drm can't work with drm_of_component_probe function now,

   At drm_of_component_probe:
 component_match_add(dev, , compare_of, port);
   And original rockchip drm use:
 component_match_add(dev, , compare_of, port->parent);

  That different "port" and "port->parent" cause crtc device node 
always mis-match.

  I'm confused that rockchip use same dts node map as imx drm 
driver, but it works
for imx drm, not work on rockchip drm.

> + of_node_put(port);
> + }
>
-- ï¼­ark Yao

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Philipp Zabel]

Am Montag, den 09.11.2015, 10:53 + schrieb Liviu Dudau:
> On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
> > On 2015年10月19日 23:07, 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 
> > >Signed-off-by: Liviu Dudau 
> > >---
> > >  drivers/gpu/drm/drm_of.c | 88 
> > > 
> > >  include/drm/drm_of.h | 13 +++
> > >  2 files changed, 101 insertions(+)
> > >
> > >diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> > >index be38840..493c05c 100644
> > >--- a/drivers/gpu/drm/drm_of.c
> > >+++ b/drivers/gpu/drm/drm_of.c
> > >@@ -1,3 +1,4 @@
> > >+#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > >@@ -61,3 +62,90 @@ 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 *m_ops)
> > >+{
> > >+  struct device_node *ep, *port, *remote;
> > >+  struct component_match *match = NULL;
> > >+  int i;
> > >+
> > >+  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, , compare_of, port);
> > Hi Liviu
> >   Rockchip drm can't work with drm_of_component_probe function now,
> > 
> >   At drm_of_component_probe:
> > component_match_add(dev, , compare_of, port);
> >   And original rockchip drm use:
> > component_match_add(dev, , compare_of, port->parent);
> > 
> >  That different "port" and "port->parent" cause crtc device node always
> > mis-match.
> > 
> >  I'm confused that rockchip use same dts node map as imx drm driver, but
> > it works
> > for imx drm, not work on rockchip drm.
> 
> Hi Mark,
> 
> I'm (slightly) confused as well. The drivers are different so there must be a 
> reason
> to account for the different behaviour. Unfortunately I don't have a Rockchip 
> based
> platform ready for testing, so I would appreciate if you could add some 
> debugging
> messages to drm_of_component_probe() when component_match_add is being called 
> and
> compare that with the version before my patch.

The reason is that rockchip has device tree probed devices that contain
the ports, but on imx crtc devices are platform devices created by the
driver, which don't have their own device tree node. They are associated
with the port nodes directly in ipu_drm_probe (ipuv3-crtc.c).

regards
Philipp

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Mon, Nov 09, 2015 at 12:03:35PM +, Russell King - ARM Linux wrote:
> On Mon, Nov 09, 2015 at 11:57:27AM +, Liviu Dudau wrote:
> > Meanwhile, what is your suggestion regarding the patchset. I've seen David 
> > has
> > sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
> > revert for rockchip commit and then patch later the function?
> 
> It definitely needs to be fixed, and I'd suggest its early enough in the
> -rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe()
> to take two compare functions.

I still don't have a Rockchip board to test the patch, so I need to find out
someone willing to test them. Mark?

> 
> I'd also suggest at this point another change: please rename it to
> drm_of_kms_component_probe() since this is only a generic case for KMS
> drivers.  GPU DRM drivers need something different.

OK, will do.

> 
> -- 
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> 

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Mon, Nov 09, 2015 at 11:57:27AM +, Liviu Dudau wrote:
> Meanwhile, what is your suggestion regarding the patchset. I've seen David has
> sent Linus a pull request for 4.4-rc1 that includes it. Should we send a
> revert for rockchip commit and then patch later the function?

It definitely needs to be fixed, and I'd suggest its early enough in the
-rc cycle (which hasn't begun yet) to simply fix drm_of_component_probe()
to take two compare functions.

I'd also suggest at this point another change: please rename it to
drm_of_kms_component_probe() since this is only a generic case for KMS
drivers.  GPU DRM drivers need something different.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Mon, Nov 09, 2015 at 11:43:00AM +, Russell King - ARM Linux wrote:
> On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
> > Hi Liviu
> >   Rockchip drm can't work with drm_of_component_probe function now,
> > 
> >   At drm_of_component_probe:
> > component_match_add(dev, , compare_of, port);
> >   And original rockchip drm use:
> > component_match_add(dev, , compare_of, port->parent);
> > 
> >  That different "port" and "port->parent" cause crtc device node always
> > mis-match.
> > 
> >  I'm confused that rockchip use same dts node map as imx drm driver, but
> > it works
> > for imx drm, not work on rockchip drm.
> 
> iMX is rather confusing because the whole device is rather complex.  It's
> a complete image processor unit, which has multiple functions within a
> single device.  It's a less than perfect example of how to deal with
> these issues (each time I look at it, I find more stuff it shouldn't be
> doing... like what I've found today.)
> 
> Basically, the device is declared in DT as:
> 
> ipu1: ipu at 0240 {
> compatible = "fsl,imx6q-ipu";
> ipu1_csi0: port at 0 {
> ...
> };
> ipu1_csi1: port at 1 {
> ...
> };
> ipu1_di0: port at 2 {
> ...
> };
> ipu1_di1: port at 3 {
> ...
> };
> };
> 
> ipu1 is the platform device, which is the _entire_ IPU device, containing
> multiple sub-devices - eg, two camera interfaces (csi) and two "CRTCs"
> (di).
> 
> The ipuv3 code creates platform devices for these, calling the CSI
> devices "imx-ipuv3-camera" and the DI devices "imx-ipuv3-crtc".
> Initially, these have no of_node attached (yuck!) but later have an
> of_node attached when the device is probed (yuck yuck yuck!) by
> ipu_drm_probe() - the of_node being the ipu1_di0 or ipu1_di1 node.
> 
> The display-subsystem property references these ipu1_di0/ipu1_di1 nodes:
> 
> display-subsystem {
> compatible = "fsl,imx-display-subsystem";
> ports = <_di0>, <_di1>, <_di0>, <_di1>;
> };
> 
> and so finds the "imx-ipuv3-crtc" platform devices rather than the
> parent ipu1 device.
> 
> It's not nice - I'd like to see this:
> 
> if (!dev->of_node) {
> /* Associate crtc device with the corresponding DI port node 
> */
> dev->of_node = ipu_drm_get_port_by_id(dev->parent->of_node,
>   pdata->di + 2);
> if (!dev->of_node) {
> dev_err(dev, "missing port@%d node in %s\n",
> pdata->di + 2, 
> dev->parent->of_node->full_name);return -ENODEV;
> }
> }
> 
> moved out of drivers/gpu/drm/imx/ipuv3-crtc.c and into
> drivers/gpu/ipu-v3/ipu-common.c where the platform devices are created
> so that we're only setting device of_node pointers at device creation
> time and not randomly during the probe sequence, even though the above
> is "safe" as far as the component helper's concerned.  There's no reason
> why it can't be done at the device creation time.

Hi Russell,

Thanks for your analysis, I keep delaying looking into the imx code more
seriously even if I have a SabreLite board that I could play with (not on my
day-to-day list of tasks).

> 
> Now, as to how to handle the differences here, I think a solution would
> be to pass in two compare_of function pointers: one for CRTCs and one
> for the encoders, since the two will need to be handled separately
> depending on the implementation.  Where you have one "parent" device of
> the CRTC node containing exactly one CRTC, then the "rockchip" method
> makes sense - though comparing the parent of the port node in
> CRTC compare_of would be where I'd put it.  If you have multiple
> CRTCs within one parent device, then something more complex like the
> iMX solution would be required.
> 
> In any case, passing port->parent is a data loss in the generic case:
> you lose the information in the compare_of function about exactly which
> port is required, so that must go into the CRTC compare_of.
> 
> So...
> 
> rockchip's CRTC compare_of() should be:
> 
> static int crtc_compare_of(struct device *dev, void *data)
> {
> struct device_node *np = data;
> 
> return dev->of_node == np->parent;
> }
> 
> and it should have an encoder_compare_of() which is its existing
> compare_of() renamed as such.
> 
> Then, we need drm_of_component_probe() to take _two_ comparison
> functions, one for the CRTCs and one for the encoders.

This hits very close to my experience. After sending this patchset I started
converting my HDLCD driver to use it and hit exactly the same issue, that the
same compare function is used for selecting CRTCs 

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
> Hi Liviu
>   Rockchip drm can't work with drm_of_component_probe function now,
> 
>   At drm_of_component_probe:
> component_match_add(dev, , compare_of, port);
>   And original rockchip drm use:
> component_match_add(dev, , compare_of, port->parent);
> 
>  That different "port" and "port->parent" cause crtc device node always
> mis-match.
> 
>  I'm confused that rockchip use same dts node map as imx drm driver, but
> it works
> for imx drm, not work on rockchip drm.

iMX is rather confusing because the whole device is rather complex.  It's
a complete image processor unit, which has multiple functions within a
single device.  It's a less than perfect example of how to deal with
these issues (each time I look at it, I find more stuff it shouldn't be
doing... like what I've found today.)

Basically, the device is declared in DT as:

ipu1: ipu at 0240 {
compatible = "fsl,imx6q-ipu";
ipu1_csi0: port at 0 {
...
};
ipu1_csi1: port at 1 {
...
};
ipu1_di0: port at 2 {
...
};
ipu1_di1: port at 3 {
...
};
};

ipu1 is the platform device, which is the _entire_ IPU device, containing
multiple sub-devices - eg, two camera interfaces (csi) and two "CRTCs"
(di).

The ipuv3 code creates platform devices for these, calling the CSI
devices "imx-ipuv3-camera" and the DI devices "imx-ipuv3-crtc".
Initially, these have no of_node attached (yuck!) but later have an
of_node attached when the device is probed (yuck yuck yuck!) by
ipu_drm_probe() - the of_node being the ipu1_di0 or ipu1_di1 node.

The display-subsystem property references these ipu1_di0/ipu1_di1 nodes:

display-subsystem {
compatible = "fsl,imx-display-subsystem";
ports = <_di0>, <_di1>, <_di0>, <_di1>;
};

and so finds the "imx-ipuv3-crtc" platform devices rather than the
parent ipu1 device.

It's not nice - I'd like to see this:

if (!dev->of_node) {
/* Associate crtc device with the corresponding DI port node */
dev->of_node = ipu_drm_get_port_by_id(dev->parent->of_node,
  pdata->di + 2);
if (!dev->of_node) {
dev_err(dev, "missing port@%d node in %s\n",
pdata->di + 2, 
dev->parent->of_node->full_name);return -ENODEV;
}
}

moved out of drivers/gpu/drm/imx/ipuv3-crtc.c and into
drivers/gpu/ipu-v3/ipu-common.c where the platform devices are created
so that we're only setting device of_node pointers at device creation
time and not randomly during the probe sequence, even though the above
is "safe" as far as the component helper's concerned.  There's no reason
why it can't be done at the device creation time.

Now, as to how to handle the differences here, I think a solution would
be to pass in two compare_of function pointers: one for CRTCs and one
for the encoders, since the two will need to be handled separately
depending on the implementation.  Where you have one "parent" device of
the CRTC node containing exactly one CRTC, then the "rockchip" method
makes sense - though comparing the parent of the port node in
CRTC compare_of would be where I'd put it.  If you have multiple
CRTCs within one parent device, then something more complex like the
iMX solution would be required.

In any case, passing port->parent is a data loss in the generic case:
you lose the information in the compare_of function about exactly which
port is required, so that must go into the CRTC compare_of.

So...

rockchip's CRTC compare_of() should be:

static int crtc_compare_of(struct device *dev, void *data)
{
struct device_node *np = data;

return dev->of_node == np->parent;
}

and it should have an encoder_compare_of() which is its existing
compare_of() renamed as such.

Then, we need drm_of_component_probe() to take _two_ comparison
functions, one for the CRTCs and one for the encoders.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:07:47PM +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.

Sorry, I take back my Acked-by.

> + /*
> +  * 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, , compare_of, port);
> + of_node_put(port);

We shouldn't be putting the ports here - we're still retaining a
reference against the port, even though it's by address.  Yes, we have
no way to release this reference, which probably ought to be fixed.
Please replace this of_node_put() here with a comment to that effect.

> + 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, , compare_of, remote);
> + of_node_put(remote);

Same here.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

On Mon, Nov 09, 2015 at 05:39:25PM +0800, Mark yao wrote:
> On 2015年10月19日 23:07, 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 
> >Signed-off-by: Liviu Dudau 
> >---
> >  drivers/gpu/drm/drm_of.c | 88 
> > 
> >  include/drm/drm_of.h | 13 +++
> >  2 files changed, 101 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
> >index be38840..493c05c 100644
> >--- a/drivers/gpu/drm/drm_of.c
> >+++ b/drivers/gpu/drm/drm_of.c
> >@@ -1,3 +1,4 @@
> >+#include 
> >  #include 
> >  #include 
> >  #include 
> >@@ -61,3 +62,90 @@ 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 *m_ops)
> >+{
> >+struct device_node *ep, *port, *remote;
> >+struct component_match *match = NULL;
> >+int i;
> >+
> >+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, , compare_of, port);
> Hi Liviu
>   Rockchip drm can't work with drm_of_component_probe function now,
> 
>   At drm_of_component_probe:
> component_match_add(dev, , compare_of, port);
>   And original rockchip drm use:
> component_match_add(dev, , compare_of, port->parent);
> 
>  That different "port" and "port->parent" cause crtc device node always
> mis-match.
> 
>  I'm confused that rockchip use same dts node map as imx drm driver, but
> it works
> for imx drm, not work on rockchip drm.

Hi Mark,

I'm (slightly) confused as well. The drivers are different so there must be a 
reason
to account for the different behaviour. Unfortunately I don't have a Rockchip 
based
platform ready for testing, so I would appreciate if you could add some 
debugging
messages to drm_of_component_probe() when component_match_add is being called 
and
compare that with the version before my patch.

Best regards,
Liviu


> 
> >+of_node_put(port);
> >+}
> >
> -- ï¼­ark Yao
> 
> 
> ___
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 

| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---
¯\_(ツ)_/¯

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Eric Anholt]

Liviu Dudau  writes:

> 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 
> Signed-off-by: Liviu Dudau 

Acked-by: Eric Anholt 

I'm unimpressed with of-graph, but this will really help me in trying it
out for vc4.
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: 

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Russell King - ARM Linux]

On Mon, Oct 19, 2015 at 04:07:47PM +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.

Thanks, this now looks perfect.

Acked-by: Russell King 

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[RFC PATCH v3 1/4] drm: Introduce generic probe function for component based masters.

[Liviu Dudau]

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 
Signed-off-by: Liviu Dudau 
---
 drivers/gpu/drm/drm_of.c | 88 
 include/drm/drm_of.h | 13 +++
 2 files changed, 101 insertions(+)

diff --git a/drivers/gpu/drm/drm_of.c b/drivers/gpu/drm/drm_of.c
index be38840..493c05c 100644
--- a/drivers/gpu/drm/drm_of.c
+++ b/drivers/gpu/drm/drm_of.c
@@ -1,3 +1,4 @@
+#include 
 #include 
 #include 
 #include 
@@ -61,3 +62,90 @@ 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 *m_ops)
+{
+   struct device_node *ep, *port, *remote;
+   struct component_match *match = NULL;
+   int i;
+
+   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, , 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, , compare_of, remote);
+   of_node_put(remote);
+   }
+   of_node_put(port);
+   }
+
+   return component_master_add_with_match(dev, m_ops, match);
+}
+EXPORT_SYMBOL(drm_of_component_probe);
diff --git a/include/drm/drm_of.h b/include/drm/drm_of.h
index 2441f71..8544665 100644
--- a/include/drm/drm_of.h
+++ b/include/drm/drm_of.h
@@ -1,18 +1,31 @@
 #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 *m_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,
+