Hello.
Some small comments/questions.
On Mon, Jun 24, 2019 at 6:03 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
...
> @@ -22,9 +22,11 @@ struct cec_notifier {
> struct list_head head;
> struct kref kref;
> struct device *hdmi_dev;
> + struct cec_connector_info conn_info;
> const char *conn_name;
> struct cec_adapter *cec_adap;
> void (*callback)(struct cec_adapter *adap, u16 pa);
> + bool called_cec_notifier_register;
If I read his correctly callback is set only by cec_notifier_register
(and not by the cec_notifier_cec_adap_register) so maybe that boolean
is not needed and just checking if the callback is set is enough to
tell those 2 cases apart?
...
> +struct cec_notifier *
> +cec_notifier_cec_adap_register(struct device *hdmi_dev, const char *conn_name,
> + struct cec_adapter *adap)
> +{
> + struct cec_notifier *n;
> +
> + if (WARN_ON(!adap))
> + return NULL;
> +
> + n = cec_notifier_get_conn(hdmi_dev, conn_name);
> + if (!n)
> + return n;
> +
> + mutex_lock(&n->lock);
> + n->cec_adap = adap;
> + adap->conn_info = n->conn_info;
Would it make sense to use cec_s_conn_info? There is a certain
asymmetry here - cec_s_phys_addr is used to configure physical
address, which also takes the adapter's lock while setting the
physical address. That lock is not taken while the connector info is
being set (not sure if that really matters for the code paths that
would call into this function).
> + adap->notifier = n;
> + cec_s_phys_addr(adap, n->phys_addr, false);
> + mutex_unlock(&n->lock);
> + return n;
> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_cec_adap_register);
> +
> +void cec_notifier_cec_adap_unregister(struct cec_notifier *n)
> +{
> + if (!n)
> + return;
> +
> + mutex_lock(&n->lock);
> + memset(&n->cec_adap->conn_info, 0, sizeof(n->cec_adap->conn_info));
Could cec_s_conn_info be used to reset the connector info?
Also, we explicitly clear connector info here. Since the notifier
provides both connector info and physical address, maybe it would make
sense to clear physical address as well?
...
> void cec_notifier_unregister(struct cec_notifier *n)
> {
> + /* Do nothing unless cec_notifier_register was called first */
> + if (!n->called_cec_notifier_register)
Could this check be made with n->lock held? cec_notifier_register sets
this value while holding that lock.
...
Thank you.
