[RFC PATCH v2 0/4] drm: Cleanup probe function for component based masters.

[RFC PATCH v2 0/4] drm: Cleanup probe function for component based masters.

[Liviu Dudau]

Changelog:
v2: Rebased the patchset on top of drm-next rather than Linus' latest -rc

A few drivers in drivers/gpu/drm are component-enabled and use quite similar
code sequences to probe for their encoder slaves at the remote end of the ports.
Move the code into a "generic" function and remove it from the drivers.

The end results is that drivers get a reference count fix (imx), more thorough
error checking (imx again), setting the DMA coherent mask (armada) plus a decrease
in the overall count of LoC.

I'm looking for comments and testing of the patchset (only compile tested from my
end as I don't have access to all the devices touched by the changes). My main
interest is in finding out if -EINVAL is the correct code to return if
dev->of_node == NULL (handy now, as it is different from the other possible error
codes and used in armada to trigger old platform_data support. Also looking for
thoughts on the correctness of the patch and if it possible to co-opt more drivers
into using the function.

Best regards,
Liviu

Liviu Dudau (4):
  drm: Introduce generic probe function for component based masters.
  drm/imx: Convert the probe function to the generic drm_of_component_probe()
  drm/rockchip: Convert the probe function to the generic drm_of_component_probe()
  drm/armada: Convert the probe function to the generic drm_of_component_probe()

 drivers/gpu/drm/armada/armada_drv.c         | 73 +++++++----------------
 drivers/gpu/drm/drm_of.c                    | 92 +++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/imx-drm-core.c          | 54 +----------------
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 86 ++-------------------------
 include/drm/drm_of.h                        | 12 ++++
 5 files changed, 133 insertions(+), 184 deletions(-)

-- 
2.6.0

[RFC PATCH v2 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 <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)
+{
+	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__ */
-- 
2.6.0

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

[Russell King - ARM Linux]

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.

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

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

[Liviu Dudau]

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.
> 

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

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

[Russell King - ARM Linux]

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.

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

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

[Liviu Dudau]

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.
> 

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

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

[Russell King - ARM Linux]

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.

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

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

[Daniel Vetter]

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.

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Russell King - ARM Linux]

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.

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

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

[Emil Velikov]

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

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

[Russell King - ARM Linux]

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.

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

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

[Liviu Dudau]

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.
> 

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

[RFC PATCH v2 2/4] drm/imx: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The generic function is functionally equivalent to the driver's
imx_drm_platform_probe(). Use the generic function and reduce the
overall code size.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/imx/imx-drm-core.c | 54 +-------------------------------------
 1 file changed, 1 insertion(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index de00a6c..384d819 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -531,59 +531,7 @@ static const struct component_master_ops imx_drm_ops = {
 
 static int imx_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device_node *ep, *port, *remote;
-	struct component_match *match = NULL;
-	int ret;
-	int i;
-
-	/*
-	 * Bind the IPU display interface ports first, so that
-	 * imx_drm_encoder_parse_of called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		component_match_add(&pdev->dev, &match, compare_of, port);
-	}
-
-	if (i == 0) {
-		dev_err(&pdev->dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
-
-	/* Then bind all encoders */
-	for (i = 0; ; i++) {
-		port = of_parse_phandle(pdev->dev.of_node, "ports", i);
-		if (!port)
-			break;
-
-		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(&pdev->dev, "parent device of %s is not available\n",
-					 remote->full_name);
-				of_node_put(remote);
-				continue;
-			}
-
-			component_match_add(&pdev->dev, &match, compare_of,
-					    remote);
-			of_node_put(remote);
-		}
-		of_node_put(port);
-	}
-
-	ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
-
-	return component_master_add_with_match(&pdev->dev, &imx_drm_ops, match);
+	return drm_of_component_probe(&pdev->dev, compare_of, &imx_drm_ops);
 }
 
 static int imx_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.0

[RFC PATCH v2 3/4] drm/rockchip: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

Use the generic drm_of_component_probe() function to probe for components.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 86 ++---------------------------
 1 file changed, 6 insertions(+), 80 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
index f22e1e1..a32b596 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.c
@@ -19,6 +19,7 @@
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_fb_helper.h>
+#include <drm/drm_of.h>
 #include <linux/dma-mapping.h>
 #include <linux/pm_runtime.h>
 #include <linux/module.h>
@@ -45,11 +46,6 @@ int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
 				   struct device *dev)
 {
 	struct dma_iommu_mapping *mapping = drm_dev->dev->archdata.mapping;
-	int ret;
-
-	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
-	if (ret)
-		return ret;
 
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
@@ -418,29 +414,6 @@ static int compare_of(struct device *dev, void *data)
 	return dev->of_node == np;
 }
 
-static void rockchip_add_endpoints(struct device *dev,
-				   struct component_match **match,
-				   struct device_node *port)
-{
-	struct device_node *ep, *remote;
-
-	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);
-	}
-}
-
 static int rockchip_drm_bind(struct device *dev)
 {
 	struct drm_device *drm;
@@ -483,61 +456,14 @@ static const struct component_master_ops rockchip_drm_ops = {
 
 static int rockchip_drm_platform_probe(struct platform_device *pdev)
 {
-	struct device *dev = &pdev->dev;
-	struct component_match *match = NULL;
-	struct device_node *np = dev->of_node;
-	struct device_node *port;
-	int i;
+	int ret = drm_of_component_probe(&pdev->dev, compare_of,
+					 &rockchip_drm_ops);
 
-	if (!np)
+	/* keep compatibility with old code that was returning -ENODEV */
+	if (ret == -EINVAL)
 		return -ENODEV;
-	/*
-	 * Bind the crtc ports first, so that
-	 * drm_of_find_possible_crtcs called from encoder .bind callbacks
-	 * works as expected.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "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->parent);
-		of_node_put(port);
-	}
-
-	if (i == 0) {
-		dev_err(dev, "missing 'ports' property\n");
-		return -ENODEV;
-	}
-
-	if (!match) {
-		dev_err(dev, "No available vop found for display-subsystem.\n");
-		return -ENODEV;
-	}
-	/*
-	 * For each bound crtc, bind the encoders attached to its
-	 * remote endpoint.
-	 */
-	for (i = 0;; i++) {
-		port = of_parse_phandle(np, "ports", i);
-		if (!port)
-			break;
-
-		if (!of_device_is_available(port->parent)) {
-			of_node_put(port);
-			continue;
-		}
-
-		rockchip_add_endpoints(dev, &match, port);
-		of_node_put(port);
-	}
-
-	return component_master_add_with_match(dev, &rockchip_drm_ops, match);
+	return ret;
 }
 
 static int rockchip_drm_platform_remove(struct platform_device *pdev)
-- 
2.6.0

[RFC PATCH v2 4/4] drm/armada: Convert the probe function to the generic drm_of_component_probe()

[Liviu Dudau]

The armada DRM driver keeps some old platform data compatibility in the
probe function that makes moving to the generic drm_of_component_probe()
a bit more complicated that it should. Refactor the probe function to do
the platform_data processing after the generic probe (and only if that
fails). This way future cleanup can further remove support for it.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/gpu/drm/armada/armada_drv.c | 73 +++++++++++--------------------------
 1 file changed, 22 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c
index 63d909e..25c42f3 100644
--- a/drivers/gpu/drm/armada/armada_drv.c
+++ b/drivers/gpu/drm/armada/armada_drv.c
@@ -11,6 +11,7 @@
 #include <linux/of_graph.h>
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_of.h>
 #include "armada_crtc.h"
 #include "armada_drm.h"
 #include "armada_gem.h"
@@ -265,78 +266,48 @@ static void armada_add_endpoints(struct device *dev,
 	}
 }
 
-static int armada_drm_find_components(struct device *dev,
-	struct component_match **match)
-{
-	struct device_node *port;
-	int i;
-
-	if (dev->of_node) {
-		struct device_node *np = dev->of_node;
-
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
-
-			component_match_add(dev, match, compare_of, port);
-			of_node_put(port);
-		}
+static const struct component_master_ops armada_master_ops = {
+	.bind = armada_drm_bind,
+	.unbind = armada_drm_unbind,
+};
 
-		if (i == 0) {
-			dev_err(dev, "missing 'ports' property\n");
-			return -ENODEV;
-		}
+static int armada_drm_probe(struct platform_device *pdev)
+{
+	struct component_match *match = NULL;
+	int ret;
 
-		for (i = 0; ; i++) {
-			port = of_parse_phandle(np, "ports", i);
-			if (!port)
-				break;
+	ret = drm_of_component_probe(&pdev->dev, compare_dev_name,
+				     &armada_master_ops);
+	if (ret != -EINVAL)
+		return ret;
 
-			armada_add_endpoints(dev, match, port);
-			of_node_put(port);
-		}
-	} else if (dev->platform_data) {
-		char **devices = dev->platform_data;
+	if (pdev->dev.platform_data) {
+		char **devices = pdev->dev.platform_data;
+		struct device_node *port;
 		struct device *d;
+		int i;
 
 		for (i = 0; devices[i]; i++)
-			component_match_add(dev, match, compare_dev_name,
+			component_match_add(&pdev->dev, match, compare_dev_name,
 					    devices[i]);
 
 		if (i == 0) {
-			dev_err(dev, "missing 'ports' property\n");
+			dev_err(&pdev->dev, "missing 'ports' property\n");
 			return -ENODEV;
 		}
 
 		for (i = 0; devices[i]; i++) {
 			d = bus_find_device_by_name(&platform_bus_type, NULL,
-					devices[i]);
+						    devices[i]);
 			if (d && d->of_node) {
 				for_each_child_of_node(d->of_node, port)
-					armada_add_endpoints(dev, match, port);
+					armada_add_endpoints(&pdev->dev, match,
+							     port);
 			}
 			put_device(d);
 		}
 	}
 
-	return 0;
-}
-
-static const struct component_master_ops armada_master_ops = {
-	.bind = armada_drm_bind,
-	.unbind = armada_drm_unbind,
-};
-
-static int armada_drm_probe(struct platform_device *pdev)
-{
-	struct component_match *match = NULL;
-	int ret;
-
-	ret = armada_drm_find_components(&pdev->dev, &match);
-	if (ret < 0)
-		return ret;
-
 	return component_master_add_with_match(&pdev->dev, &armada_master_ops,
 					       match);
 }
-- 
2.6.0