| Message ID | 20190814104520.6001-6-darekm@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: cec: convert DRM drivers to the new notifier API | expand |
On 8/14/19 12:45 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill > in the cec_connector_info. > > Changes since v6: > - move cec_notifier_conn_unregister to tda998x_bridge_detach, > - add a mutex protecting accesses to a CEC notifier. > Changes since v2: > - cec_notifier_phys_addr_invalidate where appropriate, > - don't check for NULL notifier before calling > cec_notifier_conn_unregister. > Changes since v1: > Add memory barrier to make sure that the notifier > becomes visible to the irq thread once it is > fully constructed. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 36 +++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 61e042918a7fc..643480415473f 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -82,6 +82,8 @@ struct tda998x_priv { > u8 audio_port_enable[AUDIO_ROUTE_NUM]; > struct tda9950_glue cec_glue; > struct gpio_desc *calib; > + > + struct mutex cec_notifiy_mutex; Typo: notifiy -> notify Regards, Hans > struct cec_notifier *cec_notify; > }; > > @@ -805,8 +807,11 @@ 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); > + > + mutex_lock(&priv->cec_notifiy_mutex); > + cec_notifier_phys_addr_invalidate( > + priv->cec_notify); > + mutex_unlock(&priv->cec_notifiy_mutex); > } > > handled = true; > @@ -1331,6 +1336,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 +1354,16 @@ 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); > + return -ENOMEM; > + > + mutex_lock(&priv->cec_notifiy_mutex); > + priv->cec_notify = notifier; > + mutex_unlock(&priv->cec_notifiy_mutex); > + > drm_connector_attach_encoder(&priv->connector, > priv->bridge.encoder); > > @@ -1366,6 +1383,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) > { > struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); > > + mutex_lock(&priv->cec_notifiy_mutex); > + cec_notifier_conn_unregister(priv->cec_notify); > + priv->cec_notify = NULL; > + mutex_unlock(&priv->cec_notifiy_mutex); > + > drm_connector_cleanup(&priv->connector); > } > > @@ -1789,9 +1811,6 @@ static void tda998x_destroy(struct device *dev) > cancel_work_sync(&priv->detect_work); > > i2c_unregister_device(priv->cec); > - > - if (priv->cec_notify) > - cec_notifier_put(priv->cec_notify); > } > > static int tda998x_create(struct device *dev) > @@ -1812,6 +1831,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_notifiy_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); > @@ -1916,12 +1936,6 @@ static int tda998x_create(struct device *dev) > cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); > } > > - priv->cec_notify = cec_notifier_get(dev); > - 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; >
On 8/14/19 12:45 PM, Dariusz Marcinkiewicz wrote: > Use the new cec_notifier_conn_(un)register() functions to > (un)register the notifier for the HDMI connector, and fill > in the cec_connector_info. > > Changes since v6: > - move cec_notifier_conn_unregister to tda998x_bridge_detach, > - add a mutex protecting accesses to a CEC notifier. > Changes since v2: > - cec_notifier_phys_addr_invalidate where appropriate, > - don't check for NULL notifier before calling > cec_notifier_conn_unregister. > Changes since v1: > Add memory barrier to make sure that the notifier > becomes visible to the irq thread once it is > fully constructed. > > Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 36 +++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c > index 61e042918a7fc..643480415473f 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -82,6 +82,8 @@ struct tda998x_priv { > u8 audio_port_enable[AUDIO_ROUTE_NUM]; > struct tda9950_glue cec_glue; > struct gpio_desc *calib; > + > + struct mutex cec_notifiy_mutex; > struct cec_notifier *cec_notify; > }; > > @@ -805,8 +807,11 @@ 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); > + > + mutex_lock(&priv->cec_notifiy_mutex); > + cec_notifier_phys_addr_invalidate( > + priv->cec_notify); > + mutex_unlock(&priv->cec_notifiy_mutex); > } > > handled = true; > @@ -1331,6 +1336,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 +1354,16 @@ 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); > + return -ENOMEM; You dropped a 'if (!notifier)' before the return! After adding back this 'if' it worked fine on my BeagleBone Black board, so after fixing this you can add my: Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> tag. Regards, Hans > + > + mutex_lock(&priv->cec_notifiy_mutex); > + priv->cec_notify = notifier; > + mutex_unlock(&priv->cec_notifiy_mutex); > + > drm_connector_attach_encoder(&priv->connector, > priv->bridge.encoder); > > @@ -1366,6 +1383,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) > { > struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); > > + mutex_lock(&priv->cec_notifiy_mutex); > + cec_notifier_conn_unregister(priv->cec_notify); > + priv->cec_notify = NULL; > + mutex_unlock(&priv->cec_notifiy_mutex); > + > drm_connector_cleanup(&priv->connector); > } > > @@ -1789,9 +1811,6 @@ static void tda998x_destroy(struct device *dev) > cancel_work_sync(&priv->detect_work); > > i2c_unregister_device(priv->cec); > - > - if (priv->cec_notify) > - cec_notifier_put(priv->cec_notify); > } > > static int tda998x_create(struct device *dev) > @@ -1812,6 +1831,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_notifiy_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); > @@ -1916,12 +1936,6 @@ static int tda998x_create(struct device *dev) > cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); > } > > - priv->cec_notify = cec_notifier_get(dev); > - 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; >
On Sun, Aug 25, 2019 at 3:12 PM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote: > > You dropped a 'if (!notifier)' before the return! > > After adding back this 'if' it worked fine on my BeagleBone Black board, > so after fixing this you can add my: > > Tested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Submitted v7.2. Thank you for testing!
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 61e042918a7fc..643480415473f 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -82,6 +82,8 @@ struct tda998x_priv { u8 audio_port_enable[AUDIO_ROUTE_NUM]; struct tda9950_glue cec_glue; struct gpio_desc *calib; + + struct mutex cec_notifiy_mutex; struct cec_notifier *cec_notify; }; @@ -805,8 +807,11 @@ 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); + + mutex_lock(&priv->cec_notifiy_mutex); + cec_notifier_phys_addr_invalidate( + priv->cec_notify); + mutex_unlock(&priv->cec_notifiy_mutex); } handled = true; @@ -1331,6 +1336,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 +1354,16 @@ 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); + return -ENOMEM; + + mutex_lock(&priv->cec_notifiy_mutex); + priv->cec_notify = notifier; + mutex_unlock(&priv->cec_notifiy_mutex); + drm_connector_attach_encoder(&priv->connector, priv->bridge.encoder); @@ -1366,6 +1383,11 @@ static void tda998x_bridge_detach(struct drm_bridge *bridge) { struct tda998x_priv *priv = bridge_to_tda998x_priv(bridge); + mutex_lock(&priv->cec_notifiy_mutex); + cec_notifier_conn_unregister(priv->cec_notify); + priv->cec_notify = NULL; + mutex_unlock(&priv->cec_notifiy_mutex); + drm_connector_cleanup(&priv->connector); } @@ -1789,9 +1811,6 @@ static void tda998x_destroy(struct device *dev) cancel_work_sync(&priv->detect_work); i2c_unregister_device(priv->cec); - - if (priv->cec_notify) - cec_notifier_put(priv->cec_notify); } static int tda998x_create(struct device *dev) @@ -1812,6 +1831,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_notifiy_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); @@ -1916,12 +1936,6 @@ static int tda998x_create(struct device *dev) cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); } - priv->cec_notify = cec_notifier_get(dev); - 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;
Use the new cec_notifier_conn_(un)register() functions to (un)register the notifier for the HDMI connector, and fill in the cec_connector_info. Changes since v6: - move cec_notifier_conn_unregister to tda998x_bridge_detach, - add a mutex protecting accesses to a CEC notifier. Changes since v2: - cec_notifier_phys_addr_invalidate where appropriate, - don't check for NULL notifier before calling cec_notifier_conn_unregister. Changes since v1: Add memory barrier to make sure that the notifier becomes visible to the irq thread once it is fully constructed. Signed-off-by: Dariusz Marcinkiewicz <darekm@google.com> --- drivers/gpu/drm/i2c/tda998x_drv.c | 36 +++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 11 deletions(-)