[PATCHv9 0/2] drm: tda998x: use cec_notifier_conn_(un)register

[PATCHv9 0/2] drm: tda998x: use cec_notifier_conn_(un)register

[Hans Verkuil]

This splits the previous v7.2 patch (1) into two parts: one that replaces
cec_notifier_get/put by cec_notifier_conn_(un)register, and one that
sets the connector info.

That second patch moves the CEC notifier code to tda998x_bridge_detach,
but Laurent is making changes in that area and prefers that this isn't
implemented yet.

Dariusz, can you comment on the use of the mutex in the second patch?
This second patch won't be merged for v5.5 so you can take your time :-)

But the replacement of the cec_notifier_get/put functions is something
that needs to be finished for v5.5.

By splitting this patch up I can merge the first half, but delay the
second half. This tda driver just won't be able to report the connector
information in the meantime.

Changes since v8:

- Moved the mutex addition to the second patch where I think it actually
  belongs.

Regards,

	Hans

(1) https://patchwork.linuxtv.org/patch/58464/

Dariusz Marcinkiewicz (2):
  drm: tda998x: use cec_notifier_conn_(un)register
  drm: tda998x: set the connector info

 drivers/gpu/drm/i2c/tda998x_drv.c | 38 ++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCHv9 1/2] drm: tda998x: use cec_notifier_conn_(un)register

[Hans Verkuil]

From: Dariusz Marcinkiewicz <darekm@google.com>

Use the new cec_notifier_conn_(un)register() functions to
(un)register the notifier for the HDMI connector.

Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 84c6d4c91c65..dde8decb52a6 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -805,8 +805,8 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
 				tda998x_edid_delay_start(priv);
 			} else {
 				schedule_work(&priv->detect_work);
-				cec_notifier_set_phys_addr(priv->cec_notify,
-						   CEC_PHYS_ADDR_INVALID);
+				cec_notifier_phys_addr_invalidate(
+						priv->cec_notify);
 			}
 
 			handled = true;
@@ -1790,8 +1790,7 @@ static void tda998x_destroy(struct device *dev)
 
 	i2c_unregister_device(priv->cec);
 
-	if (priv->cec_notify)
-		cec_notifier_put(priv->cec_notify);
+	cec_notifier_conn_unregister(priv->cec_notify);
 }
 
 static int tda998x_create(struct device *dev)
@@ -1916,7 +1915,7 @@ static int tda998x_create(struct device *dev)
 		cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
 	}
 
-	priv->cec_notify = cec_notifier_get(dev);
+	priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
 	if (!priv->cec_notify) {
 		ret = -ENOMEM;
 		goto fail;
-- 
2.23.0

[PATCHv9 2/2] drm: tda998x: set the connector info

[Hans Verkuil]

From: Dariusz Marcinkiewicz <darekm@google.com>

Fill in the cec_connector_info when calling cec_notifier_conn_register().

Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com>
Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index dde8decb52a6..b0620842fa3a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -82,6 +82,9 @@ struct tda998x_priv {
 	u8 audio_port_enable[AUDIO_ROUTE_NUM];
 	struct tda9950_glue cec_glue;
 	struct gpio_desc *calib;
+
+	/* protect cec_notify */
+	struct mutex cec_notify_mutex;
 	struct cec_notifier *cec_notify;
 };
 
@@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
 				tda998x_edid_delay_start(priv);
 			} else {
 				schedule_work(&priv->detect_work);
+
+				mutex_lock(&priv->cec_notify_mutex);
 				cec_notifier_phys_addr_invalidate(
 						priv->cec_notify);
+				mutex_unlock(&priv->cec_notify_mutex);
 			}
 
 			handled = true;
@@ -1331,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 				  struct drm_device *drm)
 {
 	struct drm_connector *connector = &priv->connector;
+	struct cec_connector_info conn_info;
+	struct cec_notifier *notifier;
 	int ret;
 
 	connector->interlace_allowed = 1;
@@ -1347,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
 	if (ret)
 		return ret;
 
+	cec_fill_conn_info_from_drm(&conn_info, connector);
+
+	notifier = cec_notifier_conn_register(priv->cec_glue.parent,
+					      NULL, &conn_info);
+	if (!notifier)
+		return -ENOMEM;
+
+	mutex_lock(&priv->cec_notify_mutex);
+	priv->cec_notify = notifier;
+	mutex_unlock(&priv->cec_notify_mutex);
+
 	drm_connector_attach_encoder(&priv->connector,
 				     priv->bridge.encoder);
 
@@ -1366,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge)
 {
 	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
 
+	mutex_lock(&priv->cec_notify_mutex);
+	cec_notifier_conn_unregister(priv->cec_notify);
+	priv->cec_notify = NULL;
+	mutex_unlock(&priv->cec_notify_mutex);
+
 	drm_connector_cleanup(&priv->connector);
 }
 
@@ -1789,8 +1813,6 @@ static void tda998x_destroy(struct device *dev)
 	cancel_work_sync(&priv->detect_work);
 
 	i2c_unregister_device(priv->cec);
-
-	cec_notifier_conn_unregister(priv->cec_notify);
 }
 
 static int tda998x_create(struct device *dev)
@@ -1811,6 +1833,7 @@ static int tda998x_create(struct device *dev)
 	mutex_init(&priv->mutex);	/* protect the page access */
 	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
 	mutex_init(&priv->edid_mutex);
+	mutex_init(&priv->cec_notify_mutex);
 	INIT_LIST_HEAD(&priv->bridge.list);
 	init_waitqueue_head(&priv->edid_delay_waitq);
 	timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
@@ -1915,12 +1938,6 @@ static int tda998x_create(struct device *dev)
 		cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
 	}
 
-	priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
-	if (!priv->cec_notify) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
 	priv->cec_glue.parent = dev;
 	priv->cec_glue.data = priv;
 	priv->cec_glue.init = tda998x_cec_hook_init;
-- 
2.23.0

Re: [PATCHv9 2/2] drm: tda998x: set the connector info

[Russell King - ARM Linux admin]

On Thu, Oct 17, 2019 at 09:28:42AM +0200, Hans Verkuil wrote:
> From: Dariusz Marcinkiewicz <darekm@google.com>
> 
> Fill in the cec_connector_info when calling cec_notifier_conn_register().
> 
> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com>
> Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index dde8decb52a6..b0620842fa3a 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -82,6 +82,9 @@ struct tda998x_priv {
>  	u8 audio_port_enable[AUDIO_ROUTE_NUM];
>  	struct tda9950_glue cec_glue;
>  	struct gpio_desc *calib;
> +
> +	/* protect cec_notify */
> +	struct mutex cec_notify_mutex;
>  	struct cec_notifier *cec_notify;
>  };
>  
> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>  				tda998x_edid_delay_start(priv);
>  			} else {
>  				schedule_work(&priv->detect_work);
> +
> +				mutex_lock(&priv->cec_notify_mutex);
>  				cec_notifier_phys_addr_invalidate(
>  						priv->cec_notify);
> +				mutex_unlock(&priv->cec_notify_mutex);
>  			}
>  
>  			handled = true;
> @@ -1331,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>  				  struct drm_device *drm)
>  {
>  	struct drm_connector *connector = &priv->connector;
> +	struct cec_connector_info conn_info;
> +	struct cec_notifier *notifier;
>  	int ret;
>  
>  	connector->interlace_allowed = 1;
> @@ -1347,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>  	if (ret)
>  		return ret;
>  
> +	cec_fill_conn_info_from_drm(&conn_info, connector);
> +
> +	notifier = cec_notifier_conn_register(priv->cec_glue.parent,
> +					      NULL, &conn_info);
> +	if (!notifier)
> +		return -ENOMEM;
> +
> +	mutex_lock(&priv->cec_notify_mutex);
> +	priv->cec_notify = notifier;
> +	mutex_unlock(&priv->cec_notify_mutex);

You haven't taken on board what I said about the mutex in this
instance.

> +
>  	drm_connector_attach_encoder(&priv->connector,
>  				     priv->bridge.encoder);
>  
> @@ -1366,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge)
>  {
>  	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
>  
> +	mutex_lock(&priv->cec_notify_mutex);
> +	cec_notifier_conn_unregister(priv->cec_notify);
> +	priv->cec_notify = NULL;
> +	mutex_unlock(&priv->cec_notify_mutex);
> +
>  	drm_connector_cleanup(&priv->connector);
>  }
>  
> @@ -1789,8 +1813,6 @@ static void tda998x_destroy(struct device *dev)
>  	cancel_work_sync(&priv->detect_work);
>  
>  	i2c_unregister_device(priv->cec);
> -
> -	cec_notifier_conn_unregister(priv->cec_notify);
>  }
>  
>  static int tda998x_create(struct device *dev)
> @@ -1811,6 +1833,7 @@ static int tda998x_create(struct device *dev)
>  	mutex_init(&priv->mutex);	/* protect the page access */
>  	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
>  	mutex_init(&priv->edid_mutex);
> +	mutex_init(&priv->cec_notify_mutex);
>  	INIT_LIST_HEAD(&priv->bridge.list);
>  	init_waitqueue_head(&priv->edid_delay_waitq);
>  	timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
> @@ -1915,12 +1938,6 @@ static int tda998x_create(struct device *dev)
>  		cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
>  	}
>  
> -	priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
> -	if (!priv->cec_notify) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
>  	priv->cec_glue.parent = dev;
>  	priv->cec_glue.data = priv;
>  	priv->cec_glue.init = tda998x_cec_hook_init;
> -- 
> 2.23.0
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCHv9 2/2] drm: tda998x: set the connector info

[Hans Verkuil]

On 10/17/19 10:11 AM, Russell King - ARM Linux admin wrote:
> On Thu, Oct 17, 2019 at 09:28:42AM +0200, Hans Verkuil wrote:
>> From: Dariusz Marcinkiewicz <darekm@google.com>
>>
>> Fill in the cec_connector_info when calling cec_notifier_conn_register().
>>
>> Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com>
>> Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/gpu/drm/i2c/tda998x_drv.c | 33 +++++++++++++++++++++++--------
>>  1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
>> index dde8decb52a6..b0620842fa3a 100644
>> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
>> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
>> @@ -82,6 +82,9 @@ struct tda998x_priv {
>>  	u8 audio_port_enable[AUDIO_ROUTE_NUM];
>>  	struct tda9950_glue cec_glue;
>>  	struct gpio_desc *calib;
>> +
>> +	/* protect cec_notify */
>> +	struct mutex cec_notify_mutex;
>>  	struct cec_notifier *cec_notify;
>>  };
>>  
>> @@ -805,8 +808,11 @@ static irqreturn_t tda998x_irq_thread(int irq, void *data)
>>  				tda998x_edid_delay_start(priv);
>>  			} else {
>>  				schedule_work(&priv->detect_work);
>> +
>> +				mutex_lock(&priv->cec_notify_mutex);
>>  				cec_notifier_phys_addr_invalidate(
>>  						priv->cec_notify);
>> +				mutex_unlock(&priv->cec_notify_mutex);
>>  			}
>>  
>>  			handled = true;
>> @@ -1331,6 +1337,8 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>>  				  struct drm_device *drm)
>>  {
>>  	struct drm_connector *connector = &priv->connector;
>> +	struct cec_connector_info conn_info;
>> +	struct cec_notifier *notifier;
>>  	int ret;
>>  
>>  	connector->interlace_allowed = 1;
>> @@ -1347,6 +1355,17 @@ static int tda998x_connector_init(struct tda998x_priv *priv,
>>  	if (ret)
>>  		return ret;
>>  
>> +	cec_fill_conn_info_from_drm(&conn_info, connector);
>> +
>> +	notifier = cec_notifier_conn_register(priv->cec_glue.parent,
>> +					      NULL, &conn_info);
>> +	if (!notifier)
>> +		return -ENOMEM;
>> +
>> +	mutex_lock(&priv->cec_notify_mutex);
>> +	priv->cec_notify = notifier;
>> +	mutex_unlock(&priv->cec_notify_mutex);
> 
> You haven't taken on board what I said about the mutex in this
> instance.

That's because I didn't. See the cover letter.

I need this info from the author of the patch, Dariusz. Once I have that,
and we agree with the reasoning, then I'll post a new patch for it.

For now all I am interested in is getting patch 1/2 merged. Patch 2/2 won't
be merged any time soon.

Regards,

	Hans

> 
>> +
>>  	drm_connector_attach_encoder(&priv->connector,
>>  				     priv->bridge.encoder);
>>  
>> @@ -1366,6 +1385,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge)
>>  {
>>  	struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge);
>>  
>> +	mutex_lock(&priv->cec_notify_mutex);
>> +	cec_notifier_conn_unregister(priv->cec_notify);
>> +	priv->cec_notify = NULL;
>> +	mutex_unlock(&priv->cec_notify_mutex);
>> +
>>  	drm_connector_cleanup(&priv->connector);
>>  }
>>  
>> @@ -1789,8 +1813,6 @@ static void tda998x_destroy(struct device *dev)
>>  	cancel_work_sync(&priv->detect_work);
>>  
>>  	i2c_unregister_device(priv->cec);
>> -
>> -	cec_notifier_conn_unregister(priv->cec_notify);
>>  }
>>  
>>  static int tda998x_create(struct device *dev)
>> @@ -1811,6 +1833,7 @@ static int tda998x_create(struct device *dev)
>>  	mutex_init(&priv->mutex);	/* protect the page access */
>>  	mutex_init(&priv->audio_mutex); /* protect access from audio thread */
>>  	mutex_init(&priv->edid_mutex);
>> +	mutex_init(&priv->cec_notify_mutex);
>>  	INIT_LIST_HEAD(&priv->bridge.list);
>>  	init_waitqueue_head(&priv->edid_delay_waitq);
>>  	timer_setup(&priv->edid_delay_timer, tda998x_edid_delay_done, 0);
>> @@ -1915,12 +1938,6 @@ static int tda998x_create(struct device *dev)
>>  		cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD);
>>  	}
>>  
>> -	priv->cec_notify = cec_notifier_conn_register(dev, NULL, NULL);
>> -	if (!priv->cec_notify) {
>> -		ret = -ENOMEM;
>> -		goto fail;
>> -	}
>> -
>>  	priv->cec_glue.parent = dev;
>>  	priv->cec_glue.data = priv;
>>  	priv->cec_glue.init = tda998x_cec_hook_init;
>> -- 
>> 2.23.0
>>
>>
>