[v3,21/22] drm/tilcdc: Initialize crtc->port

Submitted by Jyri Sarha on Feb. 23, 2016, 3:03 p.m.

Details

Message ID 16a4e753f0388fb15b71f30d0754073367798d6d.1456239300.git.jsarha@ti.com
State New
Headers show
Series "drm/ticdc: Accumulated fixes over the past couple of years" ( rev: 2 ) in DRI devel

Not browsing as part of any series.

Commit Message

Jyri Sarha Feb. 23, 2016, 3:03 p.m.
Initialize port device node pointer in the tilcdc crtc. Fixes "Falling
back to first CRTC" warning from tda998x driver.

The tda998x encoder driver calls drm_of_find_possible_crtcs() to
initialize possible_crtcs of struct drm_encoder. The crtc->port needs
to be initialized for drm_of_find_possible_crtcs() to work.

Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 248e3ea..1eb4e0e 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -124,6 +124,7 @@  static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
 
 	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
 
+	of_node_put(crtc->port);
 	drm_crtc_cleanup(crtc);
 	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
 
@@ -749,6 +750,7 @@  irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
 
 struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 {
+	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_crtc *tilcdc_crtc;
 	struct drm_crtc *crtc;
 	int ret;
@@ -775,6 +777,20 @@  struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
 
 	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
 
+	if (priv->is_componentized) {
+		struct device_node *ports =
+			of_get_child_by_name(dev->dev->of_node, "ports");
+
+		if (ports) {
+			crtc->port = of_get_child_by_name(ports, "port");
+			of_node_put(ports);
+		} else {
+			crtc->port =
+				of_get_child_by_name(dev->dev->of_node, "port");
+		}
+		WARN_ON(!crtc->port);
+	}
+
 	return crtc;
 
 fail:

Comments

On 23/02/16 17:03, Jyri Sarha wrote:
> Initialize port device node pointer in the tilcdc crtc. Fixes "Falling
> back to first CRTC" warning from tda998x driver.
> 
> The tda998x encoder driver calls drm_of_find_possible_crtcs() to
> initialize possible_crtcs of struct drm_encoder. The crtc->port needs
> to be initialized for drm_of_find_possible_crtcs() to work.
> 
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 248e3ea..1eb4e0e 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -124,6 +124,7 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>  
>  	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
>  
> +	of_node_put(crtc->port);
>  	drm_crtc_cleanup(crtc);
>  	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
>  
> @@ -749,6 +750,7 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>  
>  struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>  {
> +	struct tilcdc_drm_private *priv = dev->dev_private;
>  	struct tilcdc_crtc *tilcdc_crtc;
>  	struct drm_crtc *crtc;
>  	int ret;
> @@ -775,6 +777,20 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>  
>  	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>  
> +	if (priv->is_componentized) {
> +		struct device_node *ports =
> +			of_get_child_by_name(dev->dev->of_node, "ports");
> +
> +		if (ports) {
> +			crtc->port = of_get_child_by_name(ports, "port");
> +			of_node_put(ports);
> +		} else {
> +			crtc->port =
> +				of_get_child_by_name(dev->dev->of_node, "port");
> +		}
> +		WARN_ON(!crtc->port);
> +	}

You didn't comment on why this is not an error? Why should the driver
continue even if crtc->port is missing?

 Tomi
On 02/23/16 17:19, Tomi Valkeinen wrote:
>
>
> On 23/02/16 17:03, Jyri Sarha wrote:
>> Initialize port device node pointer in the tilcdc crtc. Fixes "Falling
>> back to first CRTC" warning from tda998x driver.
>>
>> The tda998x encoder driver calls drm_of_find_possible_crtcs() to
>> initialize possible_crtcs of struct drm_encoder. The crtc->port needs
>> to be initialized for drm_of_find_possible_crtcs() to work.
>>
>> Signed-off-by: Jyri Sarha <jsarha@ti.com>
>> ---
>>   drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> index 248e3ea..1eb4e0e 100644
>> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
>> @@ -124,6 +124,7 @@ static void tilcdc_crtc_destroy(struct drm_crtc *crtc)
>>
>>   	tilcdc_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
>>
>> +	of_node_put(crtc->port);
>>   	drm_crtc_cleanup(crtc);
>>   	drm_flip_work_cleanup(&tilcdc_crtc->unref_work);
>>
>> @@ -749,6 +750,7 @@ irqreturn_t tilcdc_crtc_irq(struct drm_crtc *crtc)
>>
>>   struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>>   {
>> +	struct tilcdc_drm_private *priv = dev->dev_private;
>>   	struct tilcdc_crtc *tilcdc_crtc;
>>   	struct drm_crtc *crtc;
>>   	int ret;
>> @@ -775,6 +777,20 @@ struct drm_crtc *tilcdc_crtc_create(struct drm_device *dev)
>>
>>   	drm_crtc_helper_add(crtc, &tilcdc_crtc_helper_funcs);
>>
>> +	if (priv->is_componentized) {
>> +		struct device_node *ports =
>> +			of_get_child_by_name(dev->dev->of_node, "ports");
>> +
>> +		if (ports) {
>> +			crtc->port = of_get_child_by_name(ports, "port");
>> +			of_node_put(ports);
>> +		} else {
>> +			crtc->port =
>> +				of_get_child_by_name(dev->dev->of_node, "port");
>> +		}
>> +		WARN_ON(!crtc->port);
>> +	}
>
> You didn't comment on why this is not an error? Why should the driver
> continue even if crtc->port is missing?
>

At least for the time being if the drm_of_find_possible_crtcs() fails 
the tda998x driver assumes the first crtc with a warning. So for that 
part everything will work just fine still.

Then it is another question how priv->is_componentized could be set and 
probing has gotten this far while there is no port node to be found. The 
WARN_ON() should really never happen as long as the code is the way it 
currently is.

BR,
Jyri
On 23/02/16 17:26, Jyri Sarha wrote:

>> You didn't comment on why this is not an error? Why should the driver
>> continue even if crtc->port is missing?
>>
> 
> At least for the time being if the drm_of_find_possible_crtcs() fails
> the tda998x driver assumes the first crtc with a warning. So for that
> part everything will work just fine still.
> 
> Then it is another question how priv->is_componentized could be set and
> probing has gotten this far while there is no port node to be found. The
> WARN_ON() should really never happen as long as the code is the way it
> currently is.

Ok. But I think it's either ok to not have crtc->port, and in that case
no print is needed, or it's not ok, and it's better to print an error
and fail.

Now it's kind of vague: the driver continues without crtc->port, but
gives a scary WARN.

 Tomi
On 02/23/16 17:32, Tomi Valkeinen wrote:
> On 23/02/16 17:26, Jyri Sarha wrote:
>
>>> You didn't comment on why this is not an error? Why should the driver
>>> continue even if crtc->port is missing?
>>>
>>
>> At least for the time being if the drm_of_find_possible_crtcs() fails
>> the tda998x driver assumes the first crtc with a warning. So for that
>> part everything will work just fine still.
>>
>> Then it is another question how priv->is_componentized could be set and
>> probing has gotten this far while there is no port node to be found. The
>> WARN_ON() should really never happen as long as the code is the way it
>> currently is.
>
> Ok. But I think it's either ok to not have crtc->port, and in that case
> no print is needed, or it's not ok, and it's better to print an error
> and fail.
>
> Now it's kind of vague: the driver continues without crtc->port, but
> gives a scary WARN.
>

The scary WARN is not for not having crtc->port initialized, but for 
breached internal sanity when a componentized probe has somehow reached 
this point without a port node to be found.