[RFC PATCH 0/5] Add ChromeOS EC CEC Support

[RFC PATCH 0/5] Add ChromeOS EC CEC Support

[Neil Armstrong]

Hi All,

The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
throught it's Embedded Controller, to enable the Linux CEC Core to communicate
with it and get the CEC Physical Address from the correct HDMI Connector, the
following must be added/changed:
- Add the CEC sub-device registration in the ChromeOS EC MFD Driver
- Add the CEC related commands and events definitions into the EC MFD driver
- Add a way to get a CEC notifier with it's (optional) connector name
- Add the CEC notifier to the i915 HDMI driver
- Add the proper ChromeOS EC CEC Driver

The CEC notifier with the connector name is the tricky point, since even on
Device-Tree platforms, there is no way to distinguish between multiple HDMI
connectors from the same DRM driver. The solution I implemented is pretty
simple and only adds an optional connector name to eventually distinguish
an HDMI connector notifier from another if they share the same device.

Feel free to comment this patchset !

Neil Armstrong (5):
  mfd: cros_ec_dev: Add CEC sub-device registration
  media: cec-notifier: Get notifier by device and connector name
  drm/i915: hdmi: add CEC notifier to intel_hdmi
  mfd: cros-ec: Introduce CEC commands and events definitions.
  media: platform: Add Chrome OS EC CEC driver

 drivers/gpu/drm/i915/intel_drv.h             |   2 +
 drivers/gpu/drm/i915/intel_hdmi.c            |  10 +
 drivers/media/cec/cec-notifier.c             |  30 ++-
 drivers/media/platform/Kconfig               |  11 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cros-ec/Makefile      |   1 +
 drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
 drivers/mfd/cros_ec_dev.c                    |  16 ++
 drivers/platform/chrome/cros_ec_proto.c      |  42 +++-
 include/linux/mfd/cros_ec.h                  |   2 +-
 include/linux/mfd/cros_ec_commands.h         |  80 +++++++
 include/media/cec-notifier.h                 |  44 +++-
 12 files changed, 556 insertions(+), 15 deletions(-)
 create mode 100644 drivers/media/platform/cros-ec/Makefile
 create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

-- 
2.7.4

[RFC PATCH 0/5] Add ChromeOS EC CEC Support

[Neil Armstrong]

Hi All,

The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
throught it's Embedded Controller, to enable the Linux CEC Core to communicate
with it and get the CEC Physical Address from the correct HDMI Connector, the
following must be added/changed:
- Add the CEC sub-device registration in the ChromeOS EC MFD Driver
- Add the CEC related commands and events definitions into the EC MFD driver
- Add a way to get a CEC notifier with it's (optional) connector name
- Add the CEC notifier to the i915 HDMI driver
- Add the proper ChromeOS EC CEC Driver

The CEC notifier with the connector name is the tricky point, since even on
Device-Tree platforms, there is no way to distinguish between multiple HDMI
connectors from the same DRM driver. The solution I implemented is pretty
simple and only adds an optional connector name to eventually distinguish
an HDMI connector notifier from another if they share the same device.

Feel free to comment this patchset !

Neil Armstrong (5):
  mfd: cros_ec_dev: Add CEC sub-device registration
  media: cec-notifier: Get notifier by device and connector name
  drm/i915: hdmi: add CEC notifier to intel_hdmi
  mfd: cros-ec: Introduce CEC commands and events definitions.
  media: platform: Add Chrome OS EC CEC driver

 drivers/gpu/drm/i915/intel_drv.h             |   2 +
 drivers/gpu/drm/i915/intel_hdmi.c            |  10 +
 drivers/media/cec/cec-notifier.c             |  30 ++-
 drivers/media/platform/Kconfig               |  11 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cros-ec/Makefile      |   1 +
 drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
 drivers/mfd/cros_ec_dev.c                    |  16 ++
 drivers/platform/chrome/cros_ec_proto.c      |  42 +++-
 include/linux/mfd/cros_ec.h                  |   2 +-
 include/linux/mfd/cros_ec_commands.h         |  80 +++++++
 include/media/cec-notifier.h                 |  44 +++-
 12 files changed, 556 insertions(+), 15 deletions(-)
 create mode 100644 drivers/media/platform/cros-ec/Makefile
 create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 1/5] mfd: cros_ec_dev: Add CEC sub-device registration

[Neil Armstrong]

The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
when the CEC feature bit is present.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mfd/cros_ec_dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index eafd06f..57064ec 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 	kfree(msg);
 }
 
+static void cros_ec_cec_register(struct cros_ec_dev *ec)
+{
+	int ret;
+	struct mfd_cell cec_cell = {
+		.name = "cros-ec-cec",
+	};
+
+	ret = mfd_add_devices(ec->dev, 0, &cec_cell, 1, NULL, 0, NULL);
+	if (ret)
+		dev_err(ec->dev, "failed to add EC CEC\n");
+}
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -422,6 +434,10 @@ static int ec_device_probe(struct platform_device *pdev)
 	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
 		cros_ec_sensors_register(ec);
 
+	/* check whether this EC handles CEC. */
+	if (cros_ec_check_features(ec, EC_FEATURE_CEC))
+		cros_ec_cec_register(ec);
+
 	/* Take control of the lightbar from the EC. */
 	lb_manual_suspend_ctrl(ec, 1);
 
-- 
2.7.4

[RFC PATCH 1/5] mfd: cros_ec_dev: Add CEC sub-device registration

[Neil Armstrong]

The EC can expose a CEC bus, thus add the cros-ec-cec MFD sub-device
when the CEC feature bit is present.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/mfd/cros_ec_dev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/mfd/cros_ec_dev.c b/drivers/mfd/cros_ec_dev.c
index eafd06f..57064ec 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -383,6 +383,18 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
 	kfree(msg);
 }
 
+static void cros_ec_cec_register(struct cros_ec_dev *ec)
+{
+	int ret;
+	struct mfd_cell cec_cell = {
+		.name = "cros-ec-cec",
+	};
+
+	ret = mfd_add_devices(ec->dev, 0, &cec_cell, 1, NULL, 0, NULL);
+	if (ret)
+		dev_err(ec->dev, "failed to add EC CEC\n");
+}
+
 static int ec_device_probe(struct platform_device *pdev)
 {
 	int retval = -ENOMEM;
@@ -422,6 +434,10 @@ static int ec_device_probe(struct platform_device *pdev)
 	if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE))
 		cros_ec_sensors_register(ec);
 
+	/* check whether this EC handles CEC. */
+	if (cros_ec_check_features(ec, EC_FEATURE_CEC))
+		cros_ec_cec_register(ec);
+
 	/* Take control of the lightbar from the EC. */
 	lb_manual_suspend_ctrl(ec, 1);
 
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/5] media: cec-notifier: Get notifier by device and connector name

[Neil Armstrong]

In non device-tree world, we can need to get the notifier by the driver
name directly and eventually defer probe if not yet created.

This patch adds a variant of the get function by using the device name
instead and will not create a notifier if not yet created.

But the i915 driver exposes at least 2 HDMI connectors, this patch also
adds the possibility to add a connector name tied to the notifier device
to form a tuple and associate different CEC controllers for each HDMI
connectors.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
 include/media/cec-notifier.h     | 44 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
index 16dffa0..716070a 100644
--- a/drivers/media/cec/cec-notifier.c
+++ b/drivers/media/cec/cec-notifier.c
@@ -21,6 +21,7 @@ struct cec_notifier {
 	struct list_head head;
 	struct kref kref;
 	struct device *dev;
+	const char *conn;
 	struct cec_adapter *cec_adap;
 	void (*callback)(struct cec_adapter *adap, u16 pa);
 
@@ -30,13 +31,34 @@ struct cec_notifier {
 static LIST_HEAD(cec_notifiers);
 static DEFINE_MUTEX(cec_notifiers_lock);
 
-struct cec_notifier *cec_notifier_get(struct device *dev)
+struct cec_notifier *cec_notifier_get_byname(const char *name,
+					     const char *conn)
 {
 	struct cec_notifier *n;
 
 	mutex_lock(&cec_notifiers_lock);
 	list_for_each_entry(n, &cec_notifiers, head) {
-		if (n->dev == dev) {
+		if (!strcmp(dev_name(n->dev), name) &&
+		    (!conn || !strcmp(n->conn, conn))) {
+			kref_get(&n->kref);
+			mutex_unlock(&cec_notifiers_lock);
+			return n;
+		}
+	}
+	mutex_unlock(&cec_notifiers_lock);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
+
+struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
+{
+	struct cec_notifier *n;
+
+	mutex_lock(&cec_notifiers_lock);
+	list_for_each_entry(n, &cec_notifiers, head) {
+		if (n->dev == dev &&
+		    (!conn || !strcmp(n->conn, conn))) {
 			kref_get(&n->kref);
 			mutex_unlock(&cec_notifiers_lock);
 			return n;
@@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
 	if (!n)
 		goto unlock;
 	n->dev = dev;
+	if (conn)
+		n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);
 	n->phys_addr = CEC_PHYS_ADDR_INVALID;
 	mutex_init(&n->lock);
 	kref_init(&n->kref);
@@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
 	mutex_unlock(&cec_notifiers_lock);
 	return n;
 }
-EXPORT_SYMBOL_GPL(cec_notifier_get);
+EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
 
 static void cec_notifier_release(struct kref *kref)
 {
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index cf0add7..70f2974 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -20,6 +20,37 @@ struct cec_notifier;
 #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
 
 /**
+ * cec_notifier_get_byname - find a cec_notifier for the given device name
+ * and connector tuple.
+ * @name: device name that sends the events.
+ * @conn: the connector name from which the event occurs
+ *
+ * If a notifier for device @name exists, then increase the refcount and
+ * return that notifier.
+ *
+ * If it doesn't exist, return NULL
+ */
+struct cec_notifier *cec_notifier_get_byname(const char *name,
+					     const char *conn);
+
+/**
+ * cec_notifier_get_conn - find or create a new cec_notifier for the given
+ * device and connector tuple.
+ * @dev: device that sends the events.
+ * @conn: the connector name from which the event occurs
+ *
+ * If a notifier for device @dev already exists, then increase the refcount
+ * and return that notifier.
+ *
+ * If it doesn't exist, then allocate a new notifier struct and return a
+ * pointer to that new struct.
+ *
+ * Return NULL if the memory could not be allocated.
+ */
+struct cec_notifier *cec_notifier_get_conn(struct device *dev,
+					   const char *conn);
+
+/**
  * cec_notifier_get - find or create a new cec_notifier for the given device.
  * @dev: device that sends the events.
  *
@@ -31,7 +62,10 @@ struct cec_notifier;
  *
  * Return NULL if the memory could not be allocated.
  */
-struct cec_notifier *cec_notifier_get(struct device *dev);
+static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+{
+	return cec_notifier_get_conn(dev, NULL);
+}
 
 /**
  * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
@@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
 			       struct cec_notifier *notifier);
 
 #else
-static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
+							 const char *conn)
 {
 	/* A non-NULL pointer is expected on success */
 	return (struct cec_notifier *)0xdeadfeed;
 }
 
+static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+{
+	return cec_notifier_get_conn(dev, NULL);
+}
+
 static inline void cec_notifier_put(struct cec_notifier *n)
 {
 }
-- 
2.7.4

[RFC PATCH 2/5] media: cec-notifier: Get notifier by device and connector name

[Neil Armstrong]

In non device-tree world, we can need to get the notifier by the driver
name directly and eventually defer probe if not yet created.

This patch adds a variant of the get function by using the device name
instead and will not create a notifier if not yet created.

But the i915 driver exposes at least 2 HDMI connectors, this patch also
adds the possibility to add a connector name tied to the notifier device
to form a tuple and associate different CEC controllers for each HDMI
connectors.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
 include/media/cec-notifier.h     | 44 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
index 16dffa0..716070a 100644
--- a/drivers/media/cec/cec-notifier.c
+++ b/drivers/media/cec/cec-notifier.c
@@ -21,6 +21,7 @@ struct cec_notifier {
 	struct list_head head;
 	struct kref kref;
 	struct device *dev;
+	const char *conn;
 	struct cec_adapter *cec_adap;
 	void (*callback)(struct cec_adapter *adap, u16 pa);
 
@@ -30,13 +31,34 @@ struct cec_notifier {
 static LIST_HEAD(cec_notifiers);
 static DEFINE_MUTEX(cec_notifiers_lock);
 
-struct cec_notifier *cec_notifier_get(struct device *dev)
+struct cec_notifier *cec_notifier_get_byname(const char *name,
+					     const char *conn)
 {
 	struct cec_notifier *n;
 
 	mutex_lock(&cec_notifiers_lock);
 	list_for_each_entry(n, &cec_notifiers, head) {
-		if (n->dev == dev) {
+		if (!strcmp(dev_name(n->dev), name) &&
+		    (!conn || !strcmp(n->conn, conn))) {
+			kref_get(&n->kref);
+			mutex_unlock(&cec_notifiers_lock);
+			return n;
+		}
+	}
+	mutex_unlock(&cec_notifiers_lock);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
+
+struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
+{
+	struct cec_notifier *n;
+
+	mutex_lock(&cec_notifiers_lock);
+	list_for_each_entry(n, &cec_notifiers, head) {
+		if (n->dev == dev &&
+		    (!conn || !strcmp(n->conn, conn))) {
 			kref_get(&n->kref);
 			mutex_unlock(&cec_notifiers_lock);
 			return n;
@@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
 	if (!n)
 		goto unlock;
 	n->dev = dev;
+	if (conn)
+		n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);
 	n->phys_addr = CEC_PHYS_ADDR_INVALID;
 	mutex_init(&n->lock);
 	kref_init(&n->kref);
@@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
 	mutex_unlock(&cec_notifiers_lock);
 	return n;
 }
-EXPORT_SYMBOL_GPL(cec_notifier_get);
+EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
 
 static void cec_notifier_release(struct kref *kref)
 {
diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index cf0add7..70f2974 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -20,6 +20,37 @@ struct cec_notifier;
 #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
 
 /**
+ * cec_notifier_get_byname - find a cec_notifier for the given device name
+ * and connector tuple.
+ * @name: device name that sends the events.
+ * @conn: the connector name from which the event occurs
+ *
+ * If a notifier for device @name exists, then increase the refcount and
+ * return that notifier.
+ *
+ * If it doesn't exist, return NULL
+ */
+struct cec_notifier *cec_notifier_get_byname(const char *name,
+					     const char *conn);
+
+/**
+ * cec_notifier_get_conn - find or create a new cec_notifier for the given
+ * device and connector tuple.
+ * @dev: device that sends the events.
+ * @conn: the connector name from which the event occurs
+ *
+ * If a notifier for device @dev already exists, then increase the refcount
+ * and return that notifier.
+ *
+ * If it doesn't exist, then allocate a new notifier struct and return a
+ * pointer to that new struct.
+ *
+ * Return NULL if the memory could not be allocated.
+ */
+struct cec_notifier *cec_notifier_get_conn(struct device *dev,
+					   const char *conn);
+
+/**
  * cec_notifier_get - find or create a new cec_notifier for the given device.
  * @dev: device that sends the events.
  *
@@ -31,7 +62,10 @@ struct cec_notifier;
  *
  * Return NULL if the memory could not be allocated.
  */
-struct cec_notifier *cec_notifier_get(struct device *dev);
+static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+{
+	return cec_notifier_get_conn(dev, NULL);
+}
 
 /**
  * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
@@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
 			       struct cec_notifier *notifier);
 
 #else
-static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
+							 const char *conn)
 {
 	/* A non-NULL pointer is expected on success */
 	return (struct cec_notifier *)0xdeadfeed;
 }
 
+static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+{
+	return cec_notifier_get_conn(dev, NULL);
+}
+
 static inline void cec_notifier_put(struct cec_notifier *n)
 {
 }
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

This patchs adds the cec_notifier feature to the intel_hdmi part
of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
between each HDMI ports.
The changes will allow the i915 HDMI code to notify EDID and HPD changes
to an eventual CEC adapter.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d436858..b50e51b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -39,6 +39,7 @@
 #include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_atomic.h>
+#include <media/cec-notifier.h>
 
 /**
  * __wait_for - magic wait macro
@@ -1001,6 +1002,7 @@ struct intel_hdmi {
 	bool has_audio;
 	bool rgb_quant_range_selectable;
 	struct intel_connector *attached_connector;
+	struct cec_notifier *notifier;
 };
 
 struct intel_dp_mst_encoder;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1baef4a..9b94d72 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 		connected = true;
 	}
 
+	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
+
 	return connected;
 }
 
@@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
 	enum drm_connector_status status;
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 
+	if (status != connector_status_connected)
+		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
+
 	return status;
 }
 
@@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
+
+	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
+	if (!intel_hdmi->notifier)
+		DRM_DEBUG_KMS("CEC notifier get failed\n");
 }
 
 void intel_hdmi_init(struct drm_i915_private *dev_priv,
-- 
2.7.4

[RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

This patchs adds the cec_notifier feature to the intel_hdmi part
of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
between each HDMI ports.
The changes will allow the i915 HDMI code to notify EDID and HPD changes
to an eventual CEC adapter.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |  2 ++
 drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d436858..b50e51b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -39,6 +39,7 @@
 #include <drm/drm_dp_mst_helper.h>
 #include <drm/drm_rect.h>
 #include <drm/drm_atomic.h>
+#include <media/cec-notifier.h>
 
 /**
  * __wait_for - magic wait macro
@@ -1001,6 +1002,7 @@ struct intel_hdmi {
 	bool has_audio;
 	bool rgb_quant_range_selectable;
 	struct intel_connector *attached_connector;
+	struct cec_notifier *notifier;
 };
 
 struct intel_dp_mst_encoder;
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1baef4a..9b94d72 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 		connected = true;
 	}
 
+	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
+
 	return connected;
 }
 
@@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
 	enum drm_connector_status status;
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 
 	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 
+	if (status != connector_status_connected)
+		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
+
 	return status;
 }
 
@@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
 		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
 	}
+
+	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
+	if (!intel_hdmi->notifier)
+		DRM_DEBUG_KMS("CEC notifier get failed\n");
 }
 
 void intel_hdmi_init(struct drm_i915_private *dev_priv,
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 4/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Neil Armstrong]

From: Stefan Adolfsson <sadolfsson@chromium.org>

The EC can expose a CEC bus, this patch adds the CEC related definitions
needed by the cros-ec-cec driver.
Having a 16 byte mkbp event size makes it possible to send CEC
messages from the EC to the AP directly inside the mkbp event
instead of first doing a notification and then a read.

Signed-off-by: Stefan Adolfsson <sadolfsson@chromium.org>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/platform/chrome/cros_ec_proto.c | 42 +++++++++++++----
 include/linux/mfd/cros_ec.h             |  2 +-
 include/linux/mfd/cros_ec_commands.h    | 80 +++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e7bbdf9..ba47f79 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -504,29 +504,53 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
 
+static int get_next_event_xfer(struct cros_ec_device *ec_dev,
+			       struct cros_ec_command *msg,
+			       int version, uint32_t size)
+{
+	int ret;
+
+	msg->version = version;
+	msg->command = EC_CMD_GET_NEXT_EVENT;
+	msg->insize = size;
+	msg->outsize = 0;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret > 0) {
+		ec_dev->event_size = ret - 1;
+		memcpy(&ec_dev->event_data, msg->data, size);
+	}
+
+	return ret;
+}
+
 static int get_next_event(struct cros_ec_device *ec_dev)
 {
 	u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
 	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
+	static int cmd_version = 1;
 	int ret;
 
+	BUILD_BUG_ON(sizeof(union ec_response_get_next_data_v1) != 16);
+
 	if (ec_dev->suspended) {
 		dev_dbg(ec_dev->dev, "Device suspended.\n");
 		return -EHOSTDOWN;
 	}
 
-	msg->version = 0;
-	msg->command = EC_CMD_GET_NEXT_EVENT;
-	msg->insize = sizeof(ec_dev->event_data);
-	msg->outsize = 0;
+	if (cmd_version == 1) {
+		ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+					  sizeof(ec_dev->event_data));
+		if (ret != EC_RES_INVALID_VERSION)
+			return ret;
 
-	ret = cros_ec_cmd_xfer(ec_dev, msg);
-	if (ret > 0) {
-		ec_dev->event_size = ret - 1;
-		memcpy(&ec_dev->event_data, msg->data,
-		       sizeof(ec_dev->event_data));
+		/* Fallback to version 0 for future send attempts */
+		cmd_version = 0;
 	}
 
+	ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+				  sizeof(struct ec_response_get_next_event));
+
 	return ret;
 }
 
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 2d4e23c..f3415eb 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -147,7 +147,7 @@ struct cros_ec_device {
 	bool mkbp_event_supported;
 	struct blocking_notifier_head event_notifier;
 
-	struct ec_response_get_next_event event_data;
+	struct ec_response_get_next_event_v1 event_data;
 	int event_size;
 	u32 host_event_wake_mask;
 };
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index f2edd99..18df466 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -804,6 +804,8 @@ enum ec_feature_code {
 	EC_FEATURE_MOTION_SENSE_FIFO = 24,
 	/* EC has RTC feature that can be controlled by host commands */
 	EC_FEATURE_RTC = 27,
+	/* EC supports CEC commands */
+	EC_FEATURE_CEC = 35,
 };
 
 #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
@@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
 	/* EC sent a sysrq command */
 	EC_MKBP_EVENT_SYSRQ = 6,
 
+	/* Notify the AP that something happened on CEC */
+	EC_MKBP_CEC_EVENT = 8,
+
+	/* Send an incoming CEC message to the AP */
+	EC_MKBP_EVENT_CEC_MESSAGE = 9,
+
 	/* Number of MKBP events */
 	EC_MKBP_EVENT_COUNT,
 };
@@ -2093,12 +2101,31 @@ union ec_response_get_next_data {
 	uint32_t   sysrq;
 } __packed;
 
+union ec_response_get_next_data_v1 {
+	uint8_t   key_matrix[16];
+
+	/* Unaligned */
+	uint32_t  host_event;
+
+	uint32_t   buttons;
+	uint32_t   switches;
+	uint32_t   sysrq;
+	uint32_t   cec_events;
+	uint8_t    cec_message[16];
+} __packed;
+
 struct ec_response_get_next_event {
 	uint8_t event_type;
 	/* Followed by event data if any */
 	union ec_response_get_next_data data;
 } __packed;
 
+struct ec_response_get_next_event_v1 {
+	uint8_t event_type;
+	/* Followed by event data if any */
+	union ec_response_get_next_data_v1 data;
+} __packed;
+
 /* Bit indices for buttons and switches.*/
 /* Buttons */
 #define EC_MKBP_POWER_BUTTON	0
@@ -2828,6 +2855,59 @@ struct ec_params_reboot_ec {
 /* Current version of ACPI memory address space */
 #define EC_ACPI_MEM_VERSION_CURRENT 1
 
+/*****************************************************************************/
+/*
+ * HDMI CEC commands
+ *
+ * These commands are for sending and receiving message via HDMI CEC
+ */
+#define MAX_CEC_MSG_LEN 16
+
+/* CEC message from the AP to be written on the CEC bus */
+#define EC_CMD_CEC_WRITE_MSG 0x00B8
+
+/* Message to write to the CEC bus */
+struct ec_params_cec_write {
+	uint8_t msg[MAX_CEC_MSG_LEN];
+} __packed;
+
+/* Set various CEC parameters */
+#define EC_CMD_CEC_SET 0x00BA
+
+struct ec_params_cec_set {
+	uint8_t cmd; /* enum cec_command */
+	union {
+		uint8_t enable;
+		uint8_t address;
+	};
+} __packed;
+
+/* Read various CEC parameters */
+#define EC_CMD_CEC_GET 0x00BB
+
+struct ec_params_cec_get {
+	uint8_t cmd; /* enum cec_command */
+} __packed;
+
+struct ec_response_cec_get {
+	union {
+		uint8_t enable;
+		uint8_t address;
+	};
+}__packed;
+
+enum cec_command {
+	/* CEC reading, writing and events enable */
+	CEC_CMD_ENABLE,
+	/* CEC logical address  */
+	CEC_CMD_LOGICAL_ADDRESS,
+};
+
+/* Events from CEC to AP */
+enum mkbp_cec_event {
+	EC_MKBP_CEC_SEND_OK			= 1 << 0,
+	EC_MKBP_CEC_SEND_FAILED			= 1 << 1,
+};
 
 /*****************************************************************************/
 /*
-- 
2.7.4

[RFC PATCH 4/5] mfd: cros-ec: Introduce CEC commands and events definitions.

[Neil Armstrong]

From: Stefan Adolfsson <sadolfsson@chromium.org>

The EC can expose a CEC bus, this patch adds the CEC related definitions
needed by the cros-ec-cec driver.
Having a 16 byte mkbp event size makes it possible to send CEC
messages from the EC to the AP directly inside the mkbp event
instead of first doing a notification and then a read.

Signed-off-by: Stefan Adolfsson <sadolfsson@chromium.org>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/platform/chrome/cros_ec_proto.c | 42 +++++++++++++----
 include/linux/mfd/cros_ec.h             |  2 +-
 include/linux/mfd/cros_ec_commands.h    | 80 +++++++++++++++++++++++++++++++++
 3 files changed, 114 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e7bbdf9..ba47f79 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -504,29 +504,53 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
 }
 EXPORT_SYMBOL(cros_ec_cmd_xfer_status);
 
+static int get_next_event_xfer(struct cros_ec_device *ec_dev,
+			       struct cros_ec_command *msg,
+			       int version, uint32_t size)
+{
+	int ret;
+
+	msg->version = version;
+	msg->command = EC_CMD_GET_NEXT_EVENT;
+	msg->insize = size;
+	msg->outsize = 0;
+
+	ret = cros_ec_cmd_xfer(ec_dev, msg);
+	if (ret > 0) {
+		ec_dev->event_size = ret - 1;
+		memcpy(&ec_dev->event_data, msg->data, size);
+	}
+
+	return ret;
+}
+
 static int get_next_event(struct cros_ec_device *ec_dev)
 {
 	u8 buffer[sizeof(struct cros_ec_command) + sizeof(ec_dev->event_data)];
 	struct cros_ec_command *msg = (struct cros_ec_command *)&buffer;
+	static int cmd_version = 1;
 	int ret;
 
+	BUILD_BUG_ON(sizeof(union ec_response_get_next_data_v1) != 16);
+
 	if (ec_dev->suspended) {
 		dev_dbg(ec_dev->dev, "Device suspended.\n");
 		return -EHOSTDOWN;
 	}
 
-	msg->version = 0;
-	msg->command = EC_CMD_GET_NEXT_EVENT;
-	msg->insize = sizeof(ec_dev->event_data);
-	msg->outsize = 0;
+	if (cmd_version == 1) {
+		ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+					  sizeof(ec_dev->event_data));
+		if (ret != EC_RES_INVALID_VERSION)
+			return ret;
 
-	ret = cros_ec_cmd_xfer(ec_dev, msg);
-	if (ret > 0) {
-		ec_dev->event_size = ret - 1;
-		memcpy(&ec_dev->event_data, msg->data,
-		       sizeof(ec_dev->event_data));
+		/* Fallback to version 0 for future send attempts */
+		cmd_version = 0;
 	}
 
+	ret = get_next_event_xfer(ec_dev, msg, cmd_version,
+				  sizeof(struct ec_response_get_next_event));
+
 	return ret;
 }
 
diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
index 2d4e23c..f3415eb 100644
--- a/include/linux/mfd/cros_ec.h
+++ b/include/linux/mfd/cros_ec.h
@@ -147,7 +147,7 @@ struct cros_ec_device {
 	bool mkbp_event_supported;
 	struct blocking_notifier_head event_notifier;
 
-	struct ec_response_get_next_event event_data;
+	struct ec_response_get_next_event_v1 event_data;
 	int event_size;
 	u32 host_event_wake_mask;
 };
diff --git a/include/linux/mfd/cros_ec_commands.h b/include/linux/mfd/cros_ec_commands.h
index f2edd99..18df466 100644
--- a/include/linux/mfd/cros_ec_commands.h
+++ b/include/linux/mfd/cros_ec_commands.h
@@ -804,6 +804,8 @@ enum ec_feature_code {
 	EC_FEATURE_MOTION_SENSE_FIFO = 24,
 	/* EC has RTC feature that can be controlled by host commands */
 	EC_FEATURE_RTC = 27,
+	/* EC supports CEC commands */
+	EC_FEATURE_CEC = 35,
 };
 
 #define EC_FEATURE_MASK_0(event_code) (1UL << (event_code % 32))
@@ -2078,6 +2080,12 @@ enum ec_mkbp_event {
 	/* EC sent a sysrq command */
 	EC_MKBP_EVENT_SYSRQ = 6,
 
+	/* Notify the AP that something happened on CEC */
+	EC_MKBP_CEC_EVENT = 8,
+
+	/* Send an incoming CEC message to the AP */
+	EC_MKBP_EVENT_CEC_MESSAGE = 9,
+
 	/* Number of MKBP events */
 	EC_MKBP_EVENT_COUNT,
 };
@@ -2093,12 +2101,31 @@ union ec_response_get_next_data {
 	uint32_t   sysrq;
 } __packed;
 
+union ec_response_get_next_data_v1 {
+	uint8_t   key_matrix[16];
+
+	/* Unaligned */
+	uint32_t  host_event;
+
+	uint32_t   buttons;
+	uint32_t   switches;
+	uint32_t   sysrq;
+	uint32_t   cec_events;
+	uint8_t    cec_message[16];
+} __packed;
+
 struct ec_response_get_next_event {
 	uint8_t event_type;
 	/* Followed by event data if any */
 	union ec_response_get_next_data data;
 } __packed;
 
+struct ec_response_get_next_event_v1 {
+	uint8_t event_type;
+	/* Followed by event data if any */
+	union ec_response_get_next_data_v1 data;
+} __packed;
+
 /* Bit indices for buttons and switches.*/
 /* Buttons */
 #define EC_MKBP_POWER_BUTTON	0
@@ -2828,6 +2855,59 @@ struct ec_params_reboot_ec {
 /* Current version of ACPI memory address space */
 #define EC_ACPI_MEM_VERSION_CURRENT 1
 
+/*****************************************************************************/
+/*
+ * HDMI CEC commands
+ *
+ * These commands are for sending and receiving message via HDMI CEC
+ */
+#define MAX_CEC_MSG_LEN 16
+
+/* CEC message from the AP to be written on the CEC bus */
+#define EC_CMD_CEC_WRITE_MSG 0x00B8
+
+/* Message to write to the CEC bus */
+struct ec_params_cec_write {
+	uint8_t msg[MAX_CEC_MSG_LEN];
+} __packed;
+
+/* Set various CEC parameters */
+#define EC_CMD_CEC_SET 0x00BA
+
+struct ec_params_cec_set {
+	uint8_t cmd; /* enum cec_command */
+	union {
+		uint8_t enable;
+		uint8_t address;
+	};
+} __packed;
+
+/* Read various CEC parameters */
+#define EC_CMD_CEC_GET 0x00BB
+
+struct ec_params_cec_get {
+	uint8_t cmd; /* enum cec_command */
+} __packed;
+
+struct ec_response_cec_get {
+	union {
+		uint8_t enable;
+		uint8_t address;
+	};
+}__packed;
+
+enum cec_command {
+	/* CEC reading, writing and events enable */
+	CEC_CMD_ENABLE,
+	/* CEC logical address  */
+	CEC_CMD_LOGICAL_ADDRESS,
+};
+
+/* Events from CEC to AP */
+enum mkbp_cec_event {
+	EC_MKBP_CEC_SEND_OK			= 1 << 0,
+	EC_MKBP_CEC_SEND_FAILED			= 1 << 1,
+};
 
 /*****************************************************************************/
 /*
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
driver for such feature of the Embedded Controller.

This driver is part of the cros-ec MFD and will be add as a sub-device when
the feature bit is exposed by the EC.

The controller will only handle a single logical address and handles
all the messages retries and will only expose Success or Error.

When the logical address is invalid, the controller will act as a CEC sniffer
and transfer all messages on the bus.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/media/platform/Kconfig               |  11 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cros-ec/Makefile      |   1 +
 drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
 4 files changed, 345 insertions(+)
 create mode 100644 drivers/media/platform/cros-ec/Makefile
 create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index c7a1cf8..e55a8ed2 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
 
 if CEC_PLATFORM_DRIVERS
 
+config VIDEO_CROS_EC_CEC
+	tristate "Chrome OS EC CEC driver"
+	depends on MFD_CROS_EC || COMPILE_TEST
+	select CEC_CORE
+	select CEC_NOTIFIER
+	---help---
+	  If you say yes here you will get support for the
+	  Chrome OS Embedded Controller's CEC.
+	  The CEC bus is present in the HDMI connector and enables communication
+	  between compatible devices.
+
 config VIDEO_MESON_AO_CEC
 	tristate "Amlogic Meson AO CEC driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 932515d..0e0582e 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
 obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
 
 obj-y					+= meson/
+
+obj-y					+= cros-ec/
diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
new file mode 100644
index 0000000..9ce97f9
--- /dev/null
+++ b/drivers/media/platform/cros-ec/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
new file mode 100644
index 0000000..fea90da
--- /dev/null
+++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CEC driver for Chrome OS Embedded Controller
+ *
+ * Copyright (c) 2018 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/dmi.h>
+#include <linux/cec.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <media/cec.h>
+#include <media/cec-notifier.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+
+#define DRV_NAME	"cros-ec-cec"
+
+/**
+ * struct cros_ec_cec - Driver data for EC CEC
+ *
+ * @cros_ec: Pointer to EC device
+ * @notifier: Notifier info for responding to EC events
+ * @adap: CEC adapter
+ * @notify: CEC notifier pointer
+ * @rc_msg: storage for a received message
+ */
+struct cros_ec_cec {
+	struct cros_ec_device *cros_ec;
+	struct notifier_block notifier;
+	struct cec_adapter *adap;
+	struct cec_notifier *notify;
+	struct cec_msg rx_msg;
+};
+
+static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
+{
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
+	unsigned int len = cros_ec->event_size;
+
+	cros_ec_cec->rx_msg.len = len;
+	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
+
+	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
+}
+
+static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
+{
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	uint32_t events = cros_ec->event_data.data.cec_events;
+
+	if (events & EC_MKBP_CEC_SEND_OK)
+		cec_transmit_attempt_done(cros_ec_cec->adap,
+					  CEC_TX_STATUS_OK);
+
+	if (events & EC_MKBP_CEC_SEND_FAILED)
+		cec_transmit_attempt_done(cros_ec_cec->adap,
+					  CEC_TX_STATUS_ERROR);
+}
+
+static int cros_ec_cec_event(struct notifier_block *nb,
+	unsigned long queued_during_suspend, void *_notify)
+{
+	struct cros_ec_cec *cros_ec_cec;
+	struct cros_ec_device *cros_ec;
+
+	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
+	cros_ec = cros_ec_cec->cros_ec;
+
+	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
+		handle_cec_event(cros_ec_cec);
+		return NOTIFY_OK;
+	}
+
+	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
+		handle_cec_message(cros_ec_cec);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_set data;
+	} __packed msg;
+	int ret = 0;
+
+	if (logical_addr == CEC_LOG_ADDR_INVALID)
+		return 0;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_SET;
+	msg.msg.outsize = sizeof(msg.data);
+	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
+	msg.data.address = logical_addr;
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error setting CEC logical address on EC: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
+				u32 signal_free_time, struct cec_msg *cec_msg)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_write data;
+	} __packed msg;
+	int ret = 0;
+
+	if (cec_msg->len > MAX_CEC_MSG_LEN)
+		return -EINVAL;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
+	msg.msg.outsize = cec_msg->len;
+	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error writting CEC msg on EC: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_set data;
+	} __packed msg;
+	int ret = 0;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_SET;
+	msg.msg.outsize = sizeof(msg.data);
+	msg.data.cmd = CEC_CMD_ENABLE;
+	msg.data.enable = enable;
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error %sabling CEC on EC: %d\n",
+			(enable ? "en" : "dis"), ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct cec_adap_ops cros_ec_cec_ops = {
+	.adap_enable = cros_ec_cec_adap_enable,
+	.adap_log_addr = cros_ec_cec_set_log_addr,
+	.adap_transmit = cros_ec_cec_transmit,
+};
+
+#ifdef CONFIG_PM_SLEEP
+static int cros_ec_cec_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(cros_ec_cec->cros_ec->irq);
+
+	return 0;
+}
+
+static int cros_ec_cec_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(cros_ec_cec->cros_ec->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
+	cros_ec_cec_suspend, cros_ec_cec_resume);
+
+
+struct cec_dmi_match {
+	char *sys_vendor;
+	char *product_name;
+	char *devname;
+	char *conn;
+};
+
+static const struct cec_dmi_match cec_dmi_match_table[] = {
+	/* Google Fizz */
+	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
+};
+
+static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
+		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
+
+		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
+		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
+			*notify = cec_notifier_get_byname(m->devname, m->conn);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int cros_ec_cec_probe(struct platform_device *pdev)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
+	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct cros_ec_cec *cros_ec_cec;
+	unsigned int cec_caps = CEC_CAP_DEFAULTS;
+	int ret;
+
+	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
+				   GFP_KERNEL);
+	if (!cros_ec_cec)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, cros_ec_cec);
+	cros_ec_cec->cros_ec = cros_ec;
+
+	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
+	if (ret) {
+		dev_warn(&pdev->dev, "no CEC notifier available\n");
+		cec_caps |= CEC_CAP_PHYS_ADDR;
+	} else if (!cros_ec_cec->notify) {
+		return -EPROBE_DEFER;
+	}
+
+	ret = device_init_wakeup(&pdev->dev, 1);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize wakeup\n");
+		return ret;
+	}
+
+	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
+						 DRV_NAME, cec_caps, 1);
+	if (IS_ERR(cros_ec_cec->adap))
+		return PTR_ERR(cros_ec_cec->adap);
+
+	cros_ec_cec->adap->owner = THIS_MODULE;
+
+	/* Get CEC events from the EC. */
+	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
+	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
+					       &cros_ec_cec->notifier);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register notifier\n");
+		cec_delete_adapter(cros_ec_cec->adap);
+		return ret;
+	}
+
+	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
+	if (ret < 0) {
+		cec_delete_adapter(cros_ec_cec->adap);
+		return ret;
+	}
+
+	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
+
+	return 0;
+}
+
+static int cros_ec_cec_remove(struct platform_device *pdev)
+{
+	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = blocking_notifier_chain_unregister(
+			&cros_ec_cec->cros_ec->event_notifier,
+			&cros_ec_cec->notifier);
+
+	if (ret) {
+		dev_err(dev, "failed to unregister notifier\n");
+		return ret;
+	}
+
+	cec_unregister_adapter(cros_ec_cec->adap);
+
+	if (cros_ec_cec->notify)
+		cec_notifier_put(cros_ec_cec->notify);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_cec_driver = {
+	.probe = cros_ec_cec_probe,
+	.remove  = cros_ec_cec_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &cros_ec_cec_pm_ops,
+	},
+};
+
+module_platform_driver(cros_ec_cec_driver);
+
+MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.7.4

[RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
driver for such feature of the Embedded Controller.

This driver is part of the cros-ec MFD and will be add as a sub-device when
the feature bit is exposed by the EC.

The controller will only handle a single logical address and handles
all the messages retries and will only expose Success or Error.

When the logical address is invalid, the controller will act as a CEC sniffer
and transfer all messages on the bus.

Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/media/platform/Kconfig               |  11 +
 drivers/media/platform/Makefile              |   2 +
 drivers/media/platform/cros-ec/Makefile      |   1 +
 drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
 4 files changed, 345 insertions(+)
 create mode 100644 drivers/media/platform/cros-ec/Makefile
 create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index c7a1cf8..e55a8ed2 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
 
 if CEC_PLATFORM_DRIVERS
 
+config VIDEO_CROS_EC_CEC
+	tristate "Chrome OS EC CEC driver"
+	depends on MFD_CROS_EC || COMPILE_TEST
+	select CEC_CORE
+	select CEC_NOTIFIER
+	---help---
+	  If you say yes here you will get support for the
+	  Chrome OS Embedded Controller's CEC.
+	  The CEC bus is present in the HDMI connector and enables communication
+	  between compatible devices.
+
 config VIDEO_MESON_AO_CEC
 	tristate "Amlogic Meson AO CEC driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 932515d..0e0582e 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
 obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
 
 obj-y					+= meson/
+
+obj-y					+= cros-ec/
diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
new file mode 100644
index 0000000..9ce97f9
--- /dev/null
+++ b/drivers/media/platform/cros-ec/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
new file mode 100644
index 0000000..fea90da
--- /dev/null
+++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CEC driver for Chrome OS Embedded Controller
+ *
+ * Copyright (c) 2018 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/dmi.h>
+#include <linux/cec.h>
+#include <linux/slab.h>
+#include <linux/interrupt.h>
+#include <media/cec.h>
+#include <media/cec-notifier.h>
+#include <linux/mfd/cros_ec.h>
+#include <linux/mfd/cros_ec_commands.h>
+
+#define DRV_NAME	"cros-ec-cec"
+
+/**
+ * struct cros_ec_cec - Driver data for EC CEC
+ *
+ * @cros_ec: Pointer to EC device
+ * @notifier: Notifier info for responding to EC events
+ * @adap: CEC adapter
+ * @notify: CEC notifier pointer
+ * @rc_msg: storage for a received message
+ */
+struct cros_ec_cec {
+	struct cros_ec_device *cros_ec;
+	struct notifier_block notifier;
+	struct cec_adapter *adap;
+	struct cec_notifier *notify;
+	struct cec_msg rx_msg;
+};
+
+static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
+{
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
+	unsigned int len = cros_ec->event_size;
+
+	cros_ec_cec->rx_msg.len = len;
+	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
+
+	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
+}
+
+static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
+{
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	uint32_t events = cros_ec->event_data.data.cec_events;
+
+	if (events & EC_MKBP_CEC_SEND_OK)
+		cec_transmit_attempt_done(cros_ec_cec->adap,
+					  CEC_TX_STATUS_OK);
+
+	if (events & EC_MKBP_CEC_SEND_FAILED)
+		cec_transmit_attempt_done(cros_ec_cec->adap,
+					  CEC_TX_STATUS_ERROR);
+}
+
+static int cros_ec_cec_event(struct notifier_block *nb,
+	unsigned long queued_during_suspend, void *_notify)
+{
+	struct cros_ec_cec *cros_ec_cec;
+	struct cros_ec_device *cros_ec;
+
+	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
+	cros_ec = cros_ec_cec->cros_ec;
+
+	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
+		handle_cec_event(cros_ec_cec);
+		return NOTIFY_OK;
+	}
+
+	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
+		handle_cec_message(cros_ec_cec);
+		return NOTIFY_OK;
+	}
+
+	return NOTIFY_DONE;
+}
+
+static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_set data;
+	} __packed msg;
+	int ret = 0;
+
+	if (logical_addr == CEC_LOG_ADDR_INVALID)
+		return 0;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_SET;
+	msg.msg.outsize = sizeof(msg.data);
+	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
+	msg.data.address = logical_addr;
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error setting CEC logical address on EC: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
+				u32 signal_free_time, struct cec_msg *cec_msg)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_write data;
+	} __packed msg;
+	int ret = 0;
+
+	if (cec_msg->len > MAX_CEC_MSG_LEN)
+		return -EINVAL;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
+	msg.msg.outsize = cec_msg->len;
+	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error writting CEC msg on EC: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct cros_ec_cec *cros_ec_cec = adap->priv;
+	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
+	struct {
+		struct cros_ec_command msg;
+		struct ec_params_cec_set data;
+	} __packed msg;
+	int ret = 0;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.msg.command = EC_CMD_CEC_SET;
+	msg.msg.outsize = sizeof(msg.data);
+	msg.data.cmd = CEC_CMD_ENABLE;
+	msg.data.enable = enable;
+
+	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
+	if (ret < 0) {
+		dev_err(cros_ec->dev,
+			"error %sabling CEC on EC: %d\n",
+			(enable ? "en" : "dis"), ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct cec_adap_ops cros_ec_cec_ops = {
+	.adap_enable = cros_ec_cec_adap_enable,
+	.adap_log_addr = cros_ec_cec_set_log_addr,
+	.adap_transmit = cros_ec_cec_transmit,
+};
+
+#ifdef CONFIG_PM_SLEEP
+static int cros_ec_cec_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
+
+	if (device_may_wakeup(dev))
+		enable_irq_wake(cros_ec_cec->cros_ec->irq);
+
+	return 0;
+}
+
+static int cros_ec_cec_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
+
+	if (device_may_wakeup(dev))
+		disable_irq_wake(cros_ec_cec->cros_ec->irq);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
+	cros_ec_cec_suspend, cros_ec_cec_resume);
+
+
+struct cec_dmi_match {
+	char *sys_vendor;
+	char *product_name;
+	char *devname;
+	char *conn;
+};
+
+static const struct cec_dmi_match cec_dmi_match_table[] = {
+	/* Google Fizz */
+	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
+};
+
+static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
+		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
+
+		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
+		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
+			*notify = cec_notifier_get_byname(m->devname, m->conn);
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int cros_ec_cec_probe(struct platform_device *pdev)
+{
+	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
+	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
+	struct cros_ec_cec *cros_ec_cec;
+	unsigned int cec_caps = CEC_CAP_DEFAULTS;
+	int ret;
+
+	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
+				   GFP_KERNEL);
+	if (!cros_ec_cec)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, cros_ec_cec);
+	cros_ec_cec->cros_ec = cros_ec;
+
+	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
+	if (ret) {
+		dev_warn(&pdev->dev, "no CEC notifier available\n");
+		cec_caps |= CEC_CAP_PHYS_ADDR;
+	} else if (!cros_ec_cec->notify) {
+		return -EPROBE_DEFER;
+	}
+
+	ret = device_init_wakeup(&pdev->dev, 1);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize wakeup\n");
+		return ret;
+	}
+
+	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
+						 DRV_NAME, cec_caps, 1);
+	if (IS_ERR(cros_ec_cec->adap))
+		return PTR_ERR(cros_ec_cec->adap);
+
+	cros_ec_cec->adap->owner = THIS_MODULE;
+
+	/* Get CEC events from the EC. */
+	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
+	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
+					       &cros_ec_cec->notifier);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register notifier\n");
+		cec_delete_adapter(cros_ec_cec->adap);
+		return ret;
+	}
+
+	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
+	if (ret < 0) {
+		cec_delete_adapter(cros_ec_cec->adap);
+		return ret;
+	}
+
+	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
+
+	return 0;
+}
+
+static int cros_ec_cec_remove(struct platform_device *pdev)
+{
+	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	ret = blocking_notifier_chain_unregister(
+			&cros_ec_cec->cros_ec->event_notifier,
+			&cros_ec_cec->notifier);
+
+	if (ret) {
+		dev_err(dev, "failed to unregister notifier\n");
+		return ret;
+	}
+
+	cec_unregister_adapter(cros_ec_cec->adap);
+
+	if (cros_ec_cec->notify)
+		cec_notifier_put(cros_ec_cec->notify);
+
+	return 0;
+}
+
+static struct platform_driver cros_ec_cec_driver = {
+	.probe = cros_ec_cec_probe,
+	.remove  = cros_ec_cec_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.pm = &cros_ec_cec_pm_ops,
+	},
+};
+
+module_platform_driver(cros_ec_cec_driver);
+
+MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
+MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for Add ChromeOS EC CEC Support

[Patchwork]

== Series Details ==

Series: Add ChromeOS EC CEC Support
URL   : https://patchwork.freedesktop.org/series/43162/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
f0dc064a3cc9 mfd: cros_ec_dev: Add CEC sub-device registration
a596dfa2f304 media: cec-notifier: Get notifier by device and connector name
baa482463e7a drm/i915: hdmi: add CEC notifier to intel_hdmi
-:7: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#7: 
of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate

total: 0 errors, 1 warnings, 0 checks, 48 lines checked
364a2f79ce3d mfd: cros-ec: Introduce CEC commands and events definitions.
-:196: ERROR:SPACING: space required after that close brace '}'
#196: FILE: include/linux/mfd/cros_ec_commands.h:2897:
+}__packed;

total: 1 errors, 0 warnings, 0 checks, 180 lines checked
ff97617cef82 media: platform: Add Chrome OS EC CEC driver
-:15: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#15: 
When the logical address is invalid, the controller will act as a CEC sniffer

-:28: WARNING:CONFIG_DESCRIPTION: prefer 'help' over '---help---' for new help texts
#28: FILE: drivers/media/platform/Kconfig:549:
+config VIDEO_CROS_EC_CEC

-:53: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#53: 
new file mode 100644

-:131: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#131: FILE: drivers/media/platform/cros-ec/cros-ec-cec.c:67:
+static int cros_ec_cec_event(struct notifier_block *nb,
+	unsigned long queued_during_suspend, void *_notify)

-:203: WARNING:TYPO_SPELLING: 'writting' may be misspelled - perhaps 'writing'?
#203: FILE: drivers/media/platform/cros-ec/cros-ec-cec.c:139:
+			"error writting CEC msg on EC: %d\n", ret);

-:270: CHECK:LINE_SPACING: Please don't use multiple blank lines
#270: FILE: drivers/media/platform/cros-ec/cros-ec-cec.c:206:
+
+

-:364: CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#364: FILE: drivers/media/platform/cros-ec/cros-ec-cec.c:300:
+	ret = blocking_notifier_chain_unregister(

total: 0 errors, 4 warnings, 3 checks, 354 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Add ChromeOS EC CEC Support

[Patchwork]

== Series Details ==

Series: Add ChromeOS EC CEC Support
URL   : https://patchwork.freedesktop.org/series/43162/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4179 -> Patchwork_9002 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43162/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9002 that come from known issues:

  === IGT changes ===

    ==== Possible fixes ====

    igt@debugfs_test@read_all_entries:
      fi-snb-2520m:       INCOMPLETE (fdo#103713) -> PASS

    igt@gem_exec_suspend@basic-s4-devices:
      fi-kbl-7500u:       DMESG-WARN (fdo#105128) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
      fi-snb-2600:        DMESG-WARN (fdo#102365) -> PASS

    igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
      fi-bxt-dsi:         INCOMPLETE (fdo#103927) -> PASS

    
  fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365
  fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128


== Participating hosts (41 -> 37) ==

  Additional (1): fi-byt-j1900 
  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-skl-6700hq 


== Build changes ==

    * Linux: CI_DRM_4179 -> Patchwork_9002

  CI_DRM_4179: be6d36ea8d6130f54ab5ec816555f1a46bd95f7b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4479: 89ae332745e31a075747a63ac5acc5baccf75769 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9002: ff97617cef8256fb8f14dcdddf6c472070a4e92c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4479: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit


== Linux commits ==

ff97617cef82 media: platform: Add Chrome OS EC CEC driver
364a2f79ce3d mfd: cros-ec: Introduce CEC commands and events definitions.
baa482463e7a drm/i915: hdmi: add CEC notifier to intel_hdmi
a596dfa2f304 media: cec-notifier: Get notifier by device and connector name
f0dc064a3cc9 mfd: cros_ec_dev: Add CEC sub-device registration

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9002/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for Add ChromeOS EC CEC Support

[Patchwork]

== Series Details ==

Series: Add ChromeOS EC CEC Support
URL   : https://patchwork.freedesktop.org/series/43162/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4179_full -> Patchwork_9002_full =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9002_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9002_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/43162/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9002_full:

  === IGT changes ===

    ==== Possible regressions ====

    igt@gem_eio@execbuf:
      shard-hsw:          PASS -> DMESG-WARN

    igt@gem_eio@in-flight-contexts-1us:
      shard-apl:          PASS -> DMESG-WARN

    igt@gem_eio@in-flight-internal-10ms:
      shard-glk:          PASS -> DMESG-WARN +2

    igt@gem_eio@in-flight-internal-immediate:
      shard-snb:          PASS -> DMESG-WARN +1

    igt@kms_cursor_legacy@2x-nonblocking-modeset-vs-cursor-atomic:
      shard-glk:          PASS -> FAIL +1

    
    ==== Warnings ====

    igt@gem_mocs_settings@mocs-rc6-dirty-render:
      shard-kbl:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_9002_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    {igt@gem_exec_fence@basic-busy-all}:
      shard-apl:          PASS -> INCOMPLETE (fdo#103927)

    igt@gem_ppgtt@blt-vs-render-ctxn:
      shard-kbl:          PASS -> INCOMPLETE (fdo#106023, fdo#103665)

    igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
      shard-hsw:          PASS -> FAIL (fdo#105767)

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@2x-plain-flip-ts-check-interruptible:
      shard-hsw:          PASS -> FAIL (fdo#100368)

    igt@kms_flip@flip-vs-dpms-off-vs-modeset:
      shard-apl:          PASS -> DMESG-WARN (fdo#105602) +9

    igt@kms_flip@flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363, fdo#102887)

    igt@kms_flip_tiling@flip-to-y-tiled:
      shard-glk:          PASS -> FAIL (fdo#104724, fdo#103822)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-indfb-pgflip-blt:
      shard-kbl:          PASS -> DMESG-WARN (fdo#103558, fdo#105602) +9

    igt@kms_setmode@basic:
      shard-kbl:          PASS -> FAIL (fdo#99912)

    
    ==== Possible fixes ====

    igt@drv_suspend@fence-restore-untiled:
      shard-glk:          DMESG-WARN -> PASS +1

    igt@gem_eio@in-flight-1us:
      shard-hsw:          DMESG-WARN -> PASS +1

    igt@gem_eio@in-flight-immediate:
      shard-snb:          DMESG-WARN -> PASS

    igt@gem_eio@in-flight-internal-10ms:
      shard-apl:          DMESG-WARN -> PASS

    igt@kms_atomic_transition@1x-modeset-transitions-nonblocking-fencing:
      shard-glk:          FAIL -> PASS +3

    igt@kms_cursor_crc@cursor-64x21-sliding:
      shard-kbl:          DMESG-WARN (fdo#103558, fdo#105602) -> PASS +4

    igt@kms_cursor_legacy@flip-vs-cursor-legacy:
      shard-hsw:          FAIL (fdo#102670) -> PASS

    igt@kms_flip@2x-flip-vs-expired-vblank:
      shard-glk:          FAIL (fdo#105363) -> PASS

    igt@kms_flip_tiling@flip-x-tiled:
      shard-glk:          FAIL (fdo#104724, fdo#103822) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-a-tiling-x:
      shard-snb:          FAIL (fdo#104724, fdo#103166) -> PASS

    igt@kms_rotation_crc@primary-rotation-180:
      shard-snb:          FAIL (fdo#104724, fdo#103925) -> PASS

    igt@kms_vblank@pipe-b-ts-continuation-suspend:
      shard-kbl:          INCOMPLETE (fdo#103665) -> PASS

    
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#102670 https://bugs.freedesktop.org/show_bug.cgi?id=102670
  fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103558 https://bugs.freedesktop.org/show_bug.cgi?id=103558
  fdo#103665 https://bugs.freedesktop.org/show_bug.cgi?id=103665
  fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
  fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104724 https://bugs.freedesktop.org/show_bug.cgi?id=104724
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105602 https://bugs.freedesktop.org/show_bug.cgi?id=105602
  fdo#105767 https://bugs.freedesktop.org/show_bug.cgi?id=105767
  fdo#106023 https://bugs.freedesktop.org/show_bug.cgi?id=106023
  fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4179 -> Patchwork_9002

  CI_DRM_4179: be6d36ea8d6130f54ab5ec816555f1a46bd95f7b @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4479: 89ae332745e31a075747a63ac5acc5baccf75769 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9002: ff97617cef8256fb8f14dcdddf6c472070a4e92c @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4479: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9002/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 2/5] media: cec-notifier: Get notifier by device and connector name

[Hans Verkuil]

Hi Neil,

Thanks for this patch series!

Some comments below:

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> In non device-tree world, we can need to get the notifier by the driver
> name directly and eventually defer probe if not yet created.
> 
> This patch adds a variant of the get function by using the device name
> instead and will not create a notifier if not yet created.
> 
> But the i915 driver exposes at least 2 HDMI connectors, this patch also
> adds the possibility to add a connector name tied to the notifier device
> to form a tuple and associate different CEC controllers for each HDMI
> connectors.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
>  include/media/cec-notifier.h     | 44 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index 16dffa0..716070a 100644
> --- a/drivers/media/cec/cec-notifier.c
> +++ b/drivers/media/cec/cec-notifier.c
> @@ -21,6 +21,7 @@ struct cec_notifier {
>  	struct list_head head;
>  	struct kref kref;
>  	struct device *dev;
> +	const char *conn;
>  	struct cec_adapter *cec_adap;
>  	void (*callback)(struct cec_adapter *adap, u16 pa);
>  
> @@ -30,13 +31,34 @@ struct cec_notifier {
>  static LIST_HEAD(cec_notifiers);
>  static DEFINE_MUTEX(cec_notifiers_lock);
>  
> -struct cec_notifier *cec_notifier_get(struct device *dev)
> +struct cec_notifier *cec_notifier_get_byname(const char *name,
> +					     const char *conn)
>  {
>  	struct cec_notifier *n;
>  
>  	mutex_lock(&cec_notifiers_lock);
>  	list_for_each_entry(n, &cec_notifiers, head) {
> -		if (n->dev == dev) {
> +		if (!strcmp(dev_name(n->dev), name) &&
> +		    (!conn || !strcmp(n->conn, conn))) {
> +			kref_get(&n->kref);
> +			mutex_unlock(&cec_notifiers_lock);
> +			return n;
> +		}
> +	}
> +	mutex_unlock(&cec_notifiers_lock);
> +
> +	return NULL;

This doesn't seem right. For one it doesn't act like the other cec_notifier_get*
functions in that it doesn't make a new notifier if it wasn't found yet in the
list.

For another, I think this function shouldn't be here at all. How about calling
bus_find_device_by_name(), then use cec_notifier_get_conn()?

> +}
> +EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
> +
> +struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
> +{
> +	struct cec_notifier *n;
> +
> +	mutex_lock(&cec_notifiers_lock);
> +	list_for_each_entry(n, &cec_notifiers, head) {
> +		if (n->dev == dev &&
> +		    (!conn || !strcmp(n->conn, conn))) {
>  			kref_get(&n->kref);
>  			mutex_unlock(&cec_notifiers_lock);
>  			return n;
> @@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>  	if (!n)
>  		goto unlock;
>  	n->dev = dev;
> +	if (conn)
> +		n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);

The use of devm_kstrdup worries me. The problem is that when the 'dev' device
is removed, this memory is also automatically freed. But the notifier might
still have a reference through the CEC driver, so you end up with a n->conn
pointer that points to freed memory.

I think it is better to just use kstrdup and kfree it when the last notifier
reference is released.

>  	n->phys_addr = CEC_PHYS_ADDR_INVALID;
>  	mutex_init(&n->lock);
>  	kref_init(&n->kref);
> @@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>  	mutex_unlock(&cec_notifiers_lock);
>  	return n;
>  }
> -EXPORT_SYMBOL_GPL(cec_notifier_get);
> +EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
>  
>  static void cec_notifier_release(struct kref *kref)
>  {
> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
> index cf0add7..70f2974 100644
> --- a/include/media/cec-notifier.h
> +++ b/include/media/cec-notifier.h
> @@ -20,6 +20,37 @@ struct cec_notifier;
>  #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>  
>  /**
> + * cec_notifier_get_byname - find a cec_notifier for the given device name
> + * and connector tuple.
> + * @name: device name that sends the events.
> + * @conn: the connector name from which the event occurs
> + *
> + * If a notifier for device @name exists, then increase the refcount and
> + * return that notifier.
> + *
> + * If it doesn't exist, return NULL
> + */
> +struct cec_notifier *cec_notifier_get_byname(const char *name,
> +					     const char *conn);
> +
> +/**
> + * cec_notifier_get_conn - find or create a new cec_notifier for the given
> + * device and connector tuple.
> + * @dev: device that sends the events.
> + * @conn: the connector name from which the event occurs
> + *
> + * If a notifier for device @dev already exists, then increase the refcount
> + * and return that notifier.
> + *
> + * If it doesn't exist, then allocate a new notifier struct and return a
> + * pointer to that new struct.
> + *
> + * Return NULL if the memory could not be allocated.
> + */
> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
> +					   const char *conn);
> +
> +/**
>   * cec_notifier_get - find or create a new cec_notifier for the given device.
>   * @dev: device that sends the events.
>   *
> @@ -31,7 +62,10 @@ struct cec_notifier;
>   *
>   * Return NULL if the memory could not be allocated.
>   */
> -struct cec_notifier *cec_notifier_get(struct device *dev);
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> +	return cec_notifier_get_conn(dev, NULL);
> +}
>  
>  /**
>   * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
> @@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
>  			       struct cec_notifier *notifier);
>  
>  #else
> -static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
> +							 const char *conn)
>  {
>  	/* A non-NULL pointer is expected on success */
>  	return (struct cec_notifier *)0xdeadfeed;
>  }
>  
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> +	return cec_notifier_get_conn(dev, NULL);
> +}
> +
>  static inline void cec_notifier_put(struct cec_notifier *n)
>  {
>  }
> 

Regards,

	Hans

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> This patchs adds the cec_notifier feature to the intel_hdmi part
> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
> between each HDMI ports.
> The changes will allow the i915 HDMI code to notify EDID and HPD changes
> to an eventual CEC adapter.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d436858..b50e51b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -39,6 +39,7 @@
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
>  #include <drm/drm_atomic.h>
> +#include <media/cec-notifier.h>
>  
>  /**
>   * __wait_for - magic wait macro
> @@ -1001,6 +1002,7 @@ struct intel_hdmi {
>  	bool has_audio;
>  	bool rgb_quant_range_selectable;
>  	struct intel_connector *attached_connector;
> +	struct cec_notifier *notifier;
>  };
>  
>  struct intel_dp_mst_encoder;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1baef4a..9b94d72 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  		connected = true;
>  	}
>  
> +	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
> +
>  	return connected;
>  }
>  
> @@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
>  	enum drm_connector_status status;
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
> @@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  
> +	if (status != connector_status_connected)
> +		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
> +
>  	return status;
>  }
>  
> @@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>  	}
> +
> +	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
> +	if (!intel_hdmi->notifier)
> +		DRM_DEBUG_KMS("CEC notifier get failed\n");

You 'get' the notifier here, but where is the cec_notifier_put when the connector is deleted?

Regards,

	Hans

>  }
>  
>  void intel_hdmi_init(struct drm_i915_private *dev_priv,
> 

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> This patchs adds the cec_notifier feature to the intel_hdmi part
> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
> between each HDMI ports.
> The changes will allow the i915 HDMI code to notify EDID and HPD changes
> to an eventual CEC adapter.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d436858..b50e51b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -39,6 +39,7 @@
>  #include <drm/drm_dp_mst_helper.h>
>  #include <drm/drm_rect.h>
>  #include <drm/drm_atomic.h>
> +#include <media/cec-notifier.h>
>  
>  /**
>   * __wait_for - magic wait macro
> @@ -1001,6 +1002,7 @@ struct intel_hdmi {
>  	bool has_audio;
>  	bool rgb_quant_range_selectable;
>  	struct intel_connector *attached_connector;
> +	struct cec_notifier *notifier;
>  };
>  
>  struct intel_dp_mst_encoder;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1baef4a..9b94d72 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  		connected = true;
>  	}
>  
> +	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
> +
>  	return connected;
>  }
>  
> @@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  {
>  	enum drm_connector_status status;
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
> @@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>  
>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>  
> +	if (status != connector_status_connected)
> +		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
> +
>  	return status;
>  }
>  
> @@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>  	}
> +
> +	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
> +	if (!intel_hdmi->notifier)
> +		DRM_DEBUG_KMS("CEC notifier get failed\n");

You 'get' the notifier here, but where is the cec_notifier_put when the connector is deleted?

Regards,

	Hans

>  }
>  
>  void intel_hdmi_init(struct drm_i915_private *dev_priv,
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> This patchs adds the cec_notifier feature to the intel_hdmi part
> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
> between each HDMI ports.
> The changes will allow the i915 HDMI code to notify EDID and HPD changes
> to an eventual CEC adapter.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++

The Kconfig also needs to be changed. In the DRM_I915 you need to add:

	select CEC_CORE if CEC_NOTIFIER

Otherwise you'll get problems if the cec driver is a module and i915 is built-in.

Regards,

	Hans

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> This patchs adds the cec_notifier feature to the intel_hdmi part
> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
> between each HDMI ports.
> The changes will allow the i915 HDMI code to notify EDID and HPD changes
> to an eventual CEC adapter.
> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++

The Kconfig also needs to be changed. In the DRM_I915 you need to add:

	select CEC_CORE if CEC_NOTIFIER

Otherwise you'll get problems if the cec driver is a module and i915 is built-in.

Regards,

	Hans
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
> driver for such feature of the Embedded Controller.
> 
> This driver is part of the cros-ec MFD and will be add as a sub-device when
> the feature bit is exposed by the EC.
> 
> The controller will only handle a single logical address and handles
> all the messages retries and will only expose Success or Error.
> 
> When the logical address is invalid, the controller will act as a CEC sniffer
> and transfer all messages on the bus.

I did not see any support for this. If this works as you state here, then adding
support for CEC_CAP_MONITOR_ALL is highly recommended.

> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/media/platform/Kconfig               |  11 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cros-ec/Makefile      |   1 +
>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>  4 files changed, 345 insertions(+)
>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

Shouldn't the directory be called cros-ec-cec?

> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c7a1cf8..e55a8ed2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>  
>  if CEC_PLATFORM_DRIVERS
>  
> +config VIDEO_CROS_EC_CEC
> +	tristate "Chrome OS EC CEC driver"
> +	depends on MFD_CROS_EC || COMPILE_TEST
> +	select CEC_CORE
> +	select CEC_NOTIFIER
> +	---help---
> +	  If you say yes here you will get support for the
> +	  Chrome OS Embedded Controller's CEC.
> +	  The CEC bus is present in the HDMI connector and enables communication
> +	  between compatible devices.
> +
>  config VIDEO_MESON_AO_CEC
>  	tristate "Amlogic Meson AO CEC driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 932515d..0e0582e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>  
>  obj-y					+= meson/
> +
> +obj-y					+= cros-ec/
> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
> new file mode 100644
> index 0000000..9ce97f9
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
> new file mode 100644
> index 0000000..fea90da
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CEC driver for Chrome OS Embedded Controller
> + *
> + * Copyright (c) 2018 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/dmi.h>
> +#include <linux/cec.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <media/cec.h>
> +#include <media/cec-notifier.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +
> +#define DRV_NAME	"cros-ec-cec"
> +
> +/**
> + * struct cros_ec_cec - Driver data for EC CEC
> + *
> + * @cros_ec: Pointer to EC device
> + * @notifier: Notifier info for responding to EC events
> + * @adap: CEC adapter
> + * @notify: CEC notifier pointer
> + * @rc_msg: storage for a received message
> + */
> +struct cros_ec_cec {
> +	struct cros_ec_device *cros_ec;
> +	struct notifier_block notifier;
> +	struct cec_adapter *adap;
> +	struct cec_notifier *notify;
> +	struct cec_msg rx_msg;
> +};
> +
> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
> +{
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
> +	unsigned int len = cros_ec->event_size;
> +
> +	cros_ec_cec->rx_msg.len = len;

How robust is the underlying code and hardware? What happens if a
CEC message with more than 16 bytes is received?

Hard to test unless you have an RPi3 set up as a CEC debugger. See
last section in https://hverkuil.home.xs4all.nl/cec-status.txt.

Since you worked with CEC for a while now I recommend you set up such
a system. It's cheap and very useful.

> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
> +
> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
> +}
> +
> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
> +{
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	uint32_t events = cros_ec->event_data.data.cec_events;
> +
> +	if (events & EC_MKBP_CEC_SEND_OK)
> +		cec_transmit_attempt_done(cros_ec_cec->adap,
> +					  CEC_TX_STATUS_OK);
> +
> +	if (events & EC_MKBP_CEC_SEND_FAILED)
> +		cec_transmit_attempt_done(cros_ec_cec->adap,
> +					  CEC_TX_STATUS_ERROR);

You said above that the HW takes care of all the message retries.
If that's the case, then you need to use cec_transmit_done here
and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
The MAX_RETRIES flag tells the cec core that it shouldn't attempt
to retry the message because the HW already did that.

NACK is better than ERROR if the hardware supports only an OK/FAIL
result.

> +}
> +
> +static int cros_ec_cec_event(struct notifier_block *nb,
> +	unsigned long queued_during_suspend, void *_notify)
> +{
> +	struct cros_ec_cec *cros_ec_cec;
> +	struct cros_ec_device *cros_ec;
> +
> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
> +	cros_ec = cros_ec_cec->cros_ec;
> +
> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
> +		handle_cec_event(cros_ec_cec);
> +		return NOTIFY_OK;
> +	}
> +
> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
> +		handle_cec_message(cros_ec_cec);
> +		return NOTIFY_OK;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_set data;
> +	} __packed msg;

Just say: ... msg = {};

> +	int ret = 0;
> +
> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
> +		return 0;

This looks weird. If I had configured a LA before, then this should unconfigure
it. But it just keeps the existing LA, so any poll messages to us will still
be Acked. Or am I missing something?

> +
> +	memset(&msg, 0, sizeof(msg));

and you can drop this memset.

> +	msg.msg.command = EC_CMD_CEC_SET;
> +	msg.msg.outsize = sizeof(msg.data);
> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
> +	msg.data.address = logical_addr;
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error setting CEC logical address on EC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
> +				u32 signal_free_time, struct cec_msg *cec_msg)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_write data;
> +	} __packed msg;

... msg = {};

> +	int ret = 0;
> +
> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
> +		return -EINVAL;

No need, can never happen.

> +
> +	memset(&msg, 0, sizeof(msg));

and drop this.

> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
> +	msg.msg.outsize = cec_msg->len;
> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error writting CEC msg on EC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_set data;
> +	} __packed msg;
> +	int ret = 0;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.msg.command = EC_CMD_CEC_SET;
> +	msg.msg.outsize = sizeof(msg.data);
> +	msg.data.cmd = CEC_CMD_ENABLE;
> +	msg.data.enable = enable;
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error %sabling CEC on EC: %d\n",
> +			(enable ? "en" : "dis"), ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct cec_adap_ops cros_ec_cec_ops = {
> +	.adap_enable = cros_ec_cec_adap_enable,
> +	.adap_log_addr = cros_ec_cec_set_log_addr,
> +	.adap_transmit = cros_ec_cec_transmit,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cros_ec_cec_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
> +	cros_ec_cec_suspend, cros_ec_cec_resume);
> +
> +
> +struct cec_dmi_match {
> +	char *sys_vendor;
> +	char *product_name;
> +	char *devname;
> +	char *conn;
> +};
> +
> +static const struct cec_dmi_match cec_dmi_match_table[] = {
> +	/* Google Fizz */
> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
> +};
> +
> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
> +
> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
> +			*notify = cec_notifier_get_byname(m->devname, m->conn);

See my comments for patch 2/5.

> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int cros_ec_cec_probe(struct platform_device *pdev)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct cros_ec_cec *cros_ec_cec;
> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;

Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
signal?

> +	int ret;
> +
> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
> +				   GFP_KERNEL);
> +	if (!cros_ec_cec)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, cros_ec_cec);
> +	cros_ec_cec->cros_ec = cros_ec;
> +
> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
> +	if (ret) {
> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
> +		cec_caps |= CEC_CAP_PHYS_ADDR;

Can this happen? What hardware has this? I am strongly opposed to CEC drivers
using this capability unless there is no other option. It's a pain for userspace.

> +	} else if (!cros_ec_cec->notify) {
> +		return -EPROBE_DEFER;
> +	}
> +
> +	ret = device_init_wakeup(&pdev->dev, 1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
> +		return ret;
> +	}
> +
> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
> +						 DRV_NAME, cec_caps, 1);
> +	if (IS_ERR(cros_ec_cec->adap))
> +		return PTR_ERR(cros_ec_cec->adap);
> +
> +	cros_ec_cec->adap->owner = THIS_MODULE;

This can be dropped, cec_register_adapter() sets this already.

> +
> +	/* Get CEC events from the EC. */
> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
> +					       &cros_ec_cec->notifier);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register notifier\n");
> +		cec_delete_adapter(cros_ec_cec->adap);
> +		return ret;
> +	}
> +
> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
> +	if (ret < 0) {
> +		cec_delete_adapter(cros_ec_cec->adap);
> +		return ret;
> +	}
> +
> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_remove(struct platform_device *pdev)
> +{
> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = blocking_notifier_chain_unregister(
> +			&cros_ec_cec->cros_ec->event_notifier,
> +			&cros_ec_cec->notifier);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to unregister notifier\n");
> +		return ret;
> +	}
> +
> +	cec_unregister_adapter(cros_ec_cec->adap);
> +
> +	if (cros_ec_cec->notify)
> +		cec_notifier_put(cros_ec_cec->notify);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_cec_driver = {
> +	.probe = cros_ec_cec_probe,
> +	.remove  = cros_ec_cec_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = &cros_ec_cec_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(cros_ec_cec_driver);
> +
> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> 

Regards,

	Hans

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/2018 12:40 AM, Neil Armstrong wrote:
> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
> driver for such feature of the Embedded Controller.
> 
> This driver is part of the cros-ec MFD and will be add as a sub-device when
> the feature bit is exposed by the EC.
> 
> The controller will only handle a single logical address and handles
> all the messages retries and will only expose Success or Error.
> 
> When the logical address is invalid, the controller will act as a CEC sniffer
> and transfer all messages on the bus.

I did not see any support for this. If this works as you state here, then adding
support for CEC_CAP_MONITOR_ALL is highly recommended.

> 
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  drivers/media/platform/Kconfig               |  11 +
>  drivers/media/platform/Makefile              |   2 +
>  drivers/media/platform/cros-ec/Makefile      |   1 +
>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>  4 files changed, 345 insertions(+)
>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c

Shouldn't the directory be called cros-ec-cec?

> 
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index c7a1cf8..e55a8ed2 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>  
>  if CEC_PLATFORM_DRIVERS
>  
> +config VIDEO_CROS_EC_CEC
> +	tristate "Chrome OS EC CEC driver"
> +	depends on MFD_CROS_EC || COMPILE_TEST
> +	select CEC_CORE
> +	select CEC_NOTIFIER
> +	---help---
> +	  If you say yes here you will get support for the
> +	  Chrome OS Embedded Controller's CEC.
> +	  The CEC bus is present in the HDMI connector and enables communication
> +	  between compatible devices.
> +
>  config VIDEO_MESON_AO_CEC
>  	tristate "Amlogic Meson AO CEC driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 932515d..0e0582e 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>  
>  obj-y					+= meson/
> +
> +obj-y					+= cros-ec/
> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
> new file mode 100644
> index 0000000..9ce97f9
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
> new file mode 100644
> index 0000000..fea90da
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CEC driver for Chrome OS Embedded Controller
> + *
> + * Copyright (c) 2018 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/dmi.h>
> +#include <linux/cec.h>
> +#include <linux/slab.h>
> +#include <linux/interrupt.h>
> +#include <media/cec.h>
> +#include <media/cec-notifier.h>
> +#include <linux/mfd/cros_ec.h>
> +#include <linux/mfd/cros_ec_commands.h>
> +
> +#define DRV_NAME	"cros-ec-cec"
> +
> +/**
> + * struct cros_ec_cec - Driver data for EC CEC
> + *
> + * @cros_ec: Pointer to EC device
> + * @notifier: Notifier info for responding to EC events
> + * @adap: CEC adapter
> + * @notify: CEC notifier pointer
> + * @rc_msg: storage for a received message
> + */
> +struct cros_ec_cec {
> +	struct cros_ec_device *cros_ec;
> +	struct notifier_block notifier;
> +	struct cec_adapter *adap;
> +	struct cec_notifier *notify;
> +	struct cec_msg rx_msg;
> +};
> +
> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
> +{
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
> +	unsigned int len = cros_ec->event_size;
> +
> +	cros_ec_cec->rx_msg.len = len;

How robust is the underlying code and hardware? What happens if a
CEC message with more than 16 bytes is received?

Hard to test unless you have an RPi3 set up as a CEC debugger. See
last section in https://hverkuil.home.xs4all.nl/cec-status.txt.

Since you worked with CEC for a while now I recommend you set up such
a system. It's cheap and very useful.

> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
> +
> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
> +}
> +
> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
> +{
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	uint32_t events = cros_ec->event_data.data.cec_events;
> +
> +	if (events & EC_MKBP_CEC_SEND_OK)
> +		cec_transmit_attempt_done(cros_ec_cec->adap,
> +					  CEC_TX_STATUS_OK);
> +
> +	if (events & EC_MKBP_CEC_SEND_FAILED)
> +		cec_transmit_attempt_done(cros_ec_cec->adap,
> +					  CEC_TX_STATUS_ERROR);

You said above that the HW takes care of all the message retries.
If that's the case, then you need to use cec_transmit_done here
and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
The MAX_RETRIES flag tells the cec core that it shouldn't attempt
to retry the message because the HW already did that.

NACK is better than ERROR if the hardware supports only an OK/FAIL
result.

> +}
> +
> +static int cros_ec_cec_event(struct notifier_block *nb,
> +	unsigned long queued_during_suspend, void *_notify)
> +{
> +	struct cros_ec_cec *cros_ec_cec;
> +	struct cros_ec_device *cros_ec;
> +
> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
> +	cros_ec = cros_ec_cec->cros_ec;
> +
> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
> +		handle_cec_event(cros_ec_cec);
> +		return NOTIFY_OK;
> +	}
> +
> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
> +		handle_cec_message(cros_ec_cec);
> +		return NOTIFY_OK;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_set data;
> +	} __packed msg;

Just say: ... msg = {};

> +	int ret = 0;
> +
> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
> +		return 0;

This looks weird. If I had configured a LA before, then this should unconfigure
it. But it just keeps the existing LA, so any poll messages to us will still
be Acked. Or am I missing something?

> +
> +	memset(&msg, 0, sizeof(msg));

and you can drop this memset.

> +	msg.msg.command = EC_CMD_CEC_SET;
> +	msg.msg.outsize = sizeof(msg.data);
> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
> +	msg.data.address = logical_addr;
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error setting CEC logical address on EC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
> +				u32 signal_free_time, struct cec_msg *cec_msg)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_write data;
> +	} __packed msg;

... msg = {};

> +	int ret = 0;
> +
> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
> +		return -EINVAL;

No need, can never happen.

> +
> +	memset(&msg, 0, sizeof(msg));

and drop this.

> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
> +	msg.msg.outsize = cec_msg->len;
> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error writting CEC msg on EC: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
> +	struct {
> +		struct cros_ec_command msg;
> +		struct ec_params_cec_set data;
> +	} __packed msg;
> +	int ret = 0;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.msg.command = EC_CMD_CEC_SET;
> +	msg.msg.outsize = sizeof(msg.data);
> +	msg.data.cmd = CEC_CMD_ENABLE;
> +	msg.data.enable = enable;
> +
> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
> +	if (ret < 0) {
> +		dev_err(cros_ec->dev,
> +			"error %sabling CEC on EC: %d\n",
> +			(enable ? "en" : "dis"), ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct cec_adap_ops cros_ec_cec_ops = {
> +	.adap_enable = cros_ec_cec_adap_enable,
> +	.adap_log_addr = cros_ec_cec_set_log_addr,
> +	.adap_transmit = cros_ec_cec_transmit,
> +};
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int cros_ec_cec_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
> +
> +	if (device_may_wakeup(dev))
> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
> +
> +	if (device_may_wakeup(dev))
> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
> +
> +	return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
> +	cros_ec_cec_suspend, cros_ec_cec_resume);
> +
> +
> +struct cec_dmi_match {
> +	char *sys_vendor;
> +	char *product_name;
> +	char *devname;
> +	char *conn;
> +};
> +
> +static const struct cec_dmi_match cec_dmi_match_table[] = {
> +	/* Google Fizz */
> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
> +};
> +
> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
> +
> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
> +			*notify = cec_notifier_get_byname(m->devname, m->conn);

See my comments for patch 2/5.

> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int cros_ec_cec_probe(struct platform_device *pdev)
> +{
> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
> +	struct cros_ec_cec *cros_ec_cec;
> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;

Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
signal?

> +	int ret;
> +
> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
> +				   GFP_KERNEL);
> +	if (!cros_ec_cec)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, cros_ec_cec);
> +	cros_ec_cec->cros_ec = cros_ec;
> +
> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
> +	if (ret) {
> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
> +		cec_caps |= CEC_CAP_PHYS_ADDR;

Can this happen? What hardware has this? I am strongly opposed to CEC drivers
using this capability unless there is no other option. It's a pain for userspace.

> +	} else if (!cros_ec_cec->notify) {
> +		return -EPROBE_DEFER;
> +	}
> +
> +	ret = device_init_wakeup(&pdev->dev, 1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
> +		return ret;
> +	}
> +
> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
> +						 DRV_NAME, cec_caps, 1);
> +	if (IS_ERR(cros_ec_cec->adap))
> +		return PTR_ERR(cros_ec_cec->adap);
> +
> +	cros_ec_cec->adap->owner = THIS_MODULE;

This can be dropped, cec_register_adapter() sets this already.

> +
> +	/* Get CEC events from the EC. */
> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
> +					       &cros_ec_cec->notifier);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register notifier\n");
> +		cec_delete_adapter(cros_ec_cec->adap);
> +		return ret;
> +	}
> +
> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
> +	if (ret < 0) {
> +		cec_delete_adapter(cros_ec_cec->adap);
> +		return ret;
> +	}
> +
> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
> +
> +	return 0;
> +}
> +
> +static int cros_ec_cec_remove(struct platform_device *pdev)
> +{
> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	ret = blocking_notifier_chain_unregister(
> +			&cros_ec_cec->cros_ec->event_notifier,
> +			&cros_ec_cec->notifier);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to unregister notifier\n");
> +		return ret;
> +	}
> +
> +	cec_unregister_adapter(cros_ec_cec->adap);
> +
> +	if (cros_ec_cec->notify)
> +		cec_notifier_put(cros_ec_cec->notify);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver cros_ec_cec_driver = {
> +	.probe = cros_ec_cec_probe,
> +	.remove  = cros_ec_cec_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.pm = &cros_ec_cec_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(cros_ec_cec_driver);
> +
> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> 

Regards,

	Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

Hi Hans,

Thanks for the extensive review.

On 15/05/2018 08:58, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>> driver for such feature of the Embedded Controller.
>>
>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>> the feature bit is exposed by the EC.
>>
>> The controller will only handle a single logical address and handles
>> all the messages retries and will only expose Success or Error.
>>
>> When the logical address is invalid, the controller will act as a CEC sniffer
>> and transfer all messages on the bus.
> 
> I did not see any support for this. If this works as you state here, then adding
> support for CEC_CAP_MONITOR_ALL is highly recommended.

Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
dropped for the first revision.

> 
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/media/platform/Kconfig               |  11 +
>>  drivers/media/platform/Makefile              |   2 +
>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>  4 files changed, 345 insertions(+)
>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
> 
> Shouldn't the directory be called cros-ec-cec?
> 
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c7a1cf8..e55a8ed2 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>  
>>  if CEC_PLATFORM_DRIVERS
>>  
>> +config VIDEO_CROS_EC_CEC
>> +	tristate "Chrome OS EC CEC driver"
>> +	depends on MFD_CROS_EC || COMPILE_TEST
>> +	select CEC_CORE
>> +	select CEC_NOTIFIER
>> +	---help---
>> +	  If you say yes here you will get support for the
>> +	  Chrome OS Embedded Controller's CEC.
>> +	  The CEC bus is present in the HDMI connector and enables communication
>> +	  between compatible devices.
>> +
>>  config VIDEO_MESON_AO_CEC
>>  	tristate "Amlogic Meson AO CEC driver"
>>  	depends on ARCH_MESON || COMPILE_TEST
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 932515d..0e0582e 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>  
>>  obj-y					+= meson/
>> +
>> +obj-y					+= cros-ec/
>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>> new file mode 100644
>> index 0000000..9ce97f9
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>> new file mode 100644
>> index 0000000..fea90da
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>> @@ -0,0 +1,331 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * CEC driver for Chrome OS Embedded Controller
>> + *
>> + * Copyright (c) 2018 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dmi.h>
>> +#include <linux/cec.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <media/cec.h>
>> +#include <media/cec-notifier.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +
>> +#define DRV_NAME	"cros-ec-cec"
>> +
>> +/**
>> + * struct cros_ec_cec - Driver data for EC CEC
>> + *
>> + * @cros_ec: Pointer to EC device
>> + * @notifier: Notifier info for responding to EC events
>> + * @adap: CEC adapter
>> + * @notify: CEC notifier pointer
>> + * @rc_msg: storage for a received message
>> + */
>> +struct cros_ec_cec {
>> +	struct cros_ec_device *cros_ec;
>> +	struct notifier_block notifier;
>> +	struct cec_adapter *adap;
>> +	struct cec_notifier *notify;
>> +	struct cec_msg rx_msg;
>> +};
>> +
>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>> +{
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>> +	unsigned int len = cros_ec->event_size;
>> +
>> +	cros_ec_cec->rx_msg.len = len;
> 
> How robust is the underlying code and hardware? What happens if a
> CEC message with more than 16 bytes is received?
> 
> Hard to test unless you have an RPi3 set up as a CEC debugger. See
> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
> 
> Since you worked with CEC for a while now I recommend you set up such
> a system. It's cheap and very useful.

I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....

But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.

> 
>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>> +
>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>> +}
>> +
>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>> +{
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>> +
>> +	if (events & EC_MKBP_CEC_SEND_OK)
>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>> +					  CEC_TX_STATUS_OK);
>> +
>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>> +					  CEC_TX_STATUS_ERROR);
> 
> You said above that the HW takes care of all the message retries.
> If that's the case, then you need to use cec_transmit_done here
> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
> to retry the message because the HW already did that.
> 
> NACK is better than ERROR if the hardware supports only an OK/FAIL
> result.

Ok, we were unsure about this, thanks for the clarification.

> 
>> +}
>> +
>> +static int cros_ec_cec_event(struct notifier_block *nb,
>> +	unsigned long queued_during_suspend, void *_notify)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec;
>> +	struct cros_ec_device *cros_ec;
>> +
>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>> +	cros_ec = cros_ec_cec->cros_ec;
>> +
>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>> +		handle_cec_event(cros_ec_cec);
>> +		return NOTIFY_OK;
>> +	}
>> +
>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>> +		handle_cec_message(cros_ec_cec);
>> +		return NOTIFY_OK;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_set data;
>> +	} __packed msg;
> 
> Just say: ... msg = {};
> 
>> +	int ret = 0;
>> +
>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>> +		return 0;
> 
> This looks weird. If I had configured a LA before, then this should unconfigure
> it. But it just keeps the existing LA, so any poll messages to us will still
> be Acked. Or am I missing something?
> 
>> +
>> +	memset(&msg, 0, sizeof(msg));
> 
> and you can drop this memset.

I always forget this trick.

> 
>> +	msg.msg.command = EC_CMD_CEC_SET;
>> +	msg.msg.outsize = sizeof(msg.data);
>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>> +	msg.data.address = logical_addr;
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error setting CEC logical address on EC: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_write data;
>> +	} __packed msg;
> 
> ... msg = {};
> 
>> +	int ret = 0;
>> +
>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>> +		return -EINVAL;
> 
> No need, can never happen.

Forgor to remove it, the API changed in the meantime, now it can never happen !

> 
>> +
>> +	memset(&msg, 0, sizeof(msg));
> 
> and drop this.
> 
>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>> +	msg.msg.outsize = cec_msg->len;
>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error writting CEC msg on EC: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_set data;
>> +	} __packed msg;
>> +	int ret = 0;
>> +
>> +	memset(&msg, 0, sizeof(msg));
>> +	msg.msg.command = EC_CMD_CEC_SET;
>> +	msg.msg.outsize = sizeof(msg.data);
>> +	msg.data.cmd = CEC_CMD_ENABLE;
>> +	msg.data.enable = enable;
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error %sabling CEC on EC: %d\n",
>> +			(enable ? "en" : "dis"), ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>> +	.adap_enable = cros_ec_cec_adap_enable,
>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>> +	.adap_transmit = cros_ec_cec_transmit,
>> +};
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cros_ec_cec_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>> +
>> +	if (device_may_wakeup(dev))
>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>> +
>> +	if (device_may_wakeup(dev))
>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>> +
>> +
>> +struct cec_dmi_match {
>> +	char *sys_vendor;
>> +	char *product_name;
>> +	char *devname;
>> +	char *conn;
>> +};
>> +
>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>> +	/* Google Fizz */
>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>> +};
>> +
>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>> +{
>> +	int i;
>> +
>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>> +
>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
> 
> See my comments for patch 2/5.

I will change with you comment, looks way cleaner.

> 
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>> +{
>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>> +	struct cros_ec_cec *cros_ec_cec;
>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
> 
> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
> signal?

No, Yes, the EC FW is independent of the HPD status

> 
>> +	int ret;
>> +
>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>> +				   GFP_KERNEL);
>> +	if (!cros_ec_cec)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, cros_ec_cec);
>> +	cros_ec_cec->cros_ec = cros_ec;
>> +
>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>> +	if (ret) {
>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
> 
> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
> using this capability unless there is no other option. It's a pain for userspace.

It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
it won't fail but still enable the CEC interface without a notifier.

> 
>> +	} else if (!cros_ec_cec->notify) {
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	ret = device_init_wakeup(&pdev->dev, 1);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>> +		return ret;
>> +	}
>> +
>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>> +						 DRV_NAME, cec_caps, 1);
>> +	if (IS_ERR(cros_ec_cec->adap))
>> +		return PTR_ERR(cros_ec_cec->adap);
>> +
>> +	cros_ec_cec->adap->owner = THIS_MODULE;
> 
> This can be dropped, cec_register_adapter() sets this already.

Noted.

> 
>> +
>> +	/* Get CEC events from the EC. */
>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>> +					       &cros_ec_cec->notifier);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>> +		cec_delete_adapter(cros_ec_cec->adap);
>> +		return ret;
>> +	}
>> +
>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>> +	if (ret < 0) {
>> +		cec_delete_adapter(cros_ec_cec->adap);
>> +		return ret;
>> +	}
>> +
>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = blocking_notifier_chain_unregister(
>> +			&cros_ec_cec->cros_ec->event_notifier,
>> +			&cros_ec_cec->notifier);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "failed to unregister notifier\n");
>> +		return ret;
>> +	}
>> +
>> +	cec_unregister_adapter(cros_ec_cec->adap);
>> +
>> +	if (cros_ec_cec->notify)
>> +		cec_notifier_put(cros_ec_cec->notify);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver cros_ec_cec_driver = {
>> +	.probe = cros_ec_cec_probe,
>> +	.remove  = cros_ec_cec_remove,
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +		.pm = &cros_ec_cec_pm_ops,
>> +	},
>> +};
>> +
>> +module_platform_driver(cros_ec_cec_driver);
>> +
>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRV_NAME);
>>
> 
> Regards,
> 
> 	Hans
> 

Thanks,
Neil

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

Hi Hans,

Thanks for the extensive review.

On 15/05/2018 08:58, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>> driver for such feature of the Embedded Controller.
>>
>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>> the feature bit is exposed by the EC.
>>
>> The controller will only handle a single logical address and handles
>> all the messages retries and will only expose Success or Error.
>>
>> When the logical address is invalid, the controller will act as a CEC sniffer
>> and transfer all messages on the bus.
> 
> I did not see any support for this. If this works as you state here, then adding
> support for CEC_CAP_MONITOR_ALL is highly recommended.

Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
dropped for the first revision.

> 
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/media/platform/Kconfig               |  11 +
>>  drivers/media/platform/Makefile              |   2 +
>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>  4 files changed, 345 insertions(+)
>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
> 
> Shouldn't the directory be called cros-ec-cec?
> 
>>
>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>> index c7a1cf8..e55a8ed2 100644
>> --- a/drivers/media/platform/Kconfig
>> +++ b/drivers/media/platform/Kconfig
>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>  
>>  if CEC_PLATFORM_DRIVERS
>>  
>> +config VIDEO_CROS_EC_CEC
>> +	tristate "Chrome OS EC CEC driver"
>> +	depends on MFD_CROS_EC || COMPILE_TEST
>> +	select CEC_CORE
>> +	select CEC_NOTIFIER
>> +	---help---
>> +	  If you say yes here you will get support for the
>> +	  Chrome OS Embedded Controller's CEC.
>> +	  The CEC bus is present in the HDMI connector and enables communication
>> +	  between compatible devices.
>> +
>>  config VIDEO_MESON_AO_CEC
>>  	tristate "Amlogic Meson AO CEC driver"
>>  	depends on ARCH_MESON || COMPILE_TEST
>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>> index 932515d..0e0582e 100644
>> --- a/drivers/media/platform/Makefile
>> +++ b/drivers/media/platform/Makefile
>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>  
>>  obj-y					+= meson/
>> +
>> +obj-y					+= cros-ec/
>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>> new file mode 100644
>> index 0000000..9ce97f9
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>> new file mode 100644
>> index 0000000..fea90da
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>> @@ -0,0 +1,331 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * CEC driver for Chrome OS Embedded Controller
>> + *
>> + * Copyright (c) 2018 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dmi.h>
>> +#include <linux/cec.h>
>> +#include <linux/slab.h>
>> +#include <linux/interrupt.h>
>> +#include <media/cec.h>
>> +#include <media/cec-notifier.h>
>> +#include <linux/mfd/cros_ec.h>
>> +#include <linux/mfd/cros_ec_commands.h>
>> +
>> +#define DRV_NAME	"cros-ec-cec"
>> +
>> +/**
>> + * struct cros_ec_cec - Driver data for EC CEC
>> + *
>> + * @cros_ec: Pointer to EC device
>> + * @notifier: Notifier info for responding to EC events
>> + * @adap: CEC adapter
>> + * @notify: CEC notifier pointer
>> + * @rc_msg: storage for a received message
>> + */
>> +struct cros_ec_cec {
>> +	struct cros_ec_device *cros_ec;
>> +	struct notifier_block notifier;
>> +	struct cec_adapter *adap;
>> +	struct cec_notifier *notify;
>> +	struct cec_msg rx_msg;
>> +};
>> +
>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>> +{
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>> +	unsigned int len = cros_ec->event_size;
>> +
>> +	cros_ec_cec->rx_msg.len = len;
> 
> How robust is the underlying code and hardware? What happens if a
> CEC message with more than 16 bytes is received?
> 
> Hard to test unless you have an RPi3 set up as a CEC debugger. See
> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
> 
> Since you worked with CEC for a while now I recommend you set up such
> a system. It's cheap and very useful.

I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....

But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.

> 
>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>> +
>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>> +}
>> +
>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>> +{
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>> +
>> +	if (events & EC_MKBP_CEC_SEND_OK)
>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>> +					  CEC_TX_STATUS_OK);
>> +
>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>> +					  CEC_TX_STATUS_ERROR);
> 
> You said above that the HW takes care of all the message retries.
> If that's the case, then you need to use cec_transmit_done here
> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
> to retry the message because the HW already did that.
> 
> NACK is better than ERROR if the hardware supports only an OK/FAIL
> result.

Ok, we were unsure about this, thanks for the clarification.

> 
>> +}
>> +
>> +static int cros_ec_cec_event(struct notifier_block *nb,
>> +	unsigned long queued_during_suspend, void *_notify)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec;
>> +	struct cros_ec_device *cros_ec;
>> +
>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>> +	cros_ec = cros_ec_cec->cros_ec;
>> +
>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>> +		handle_cec_event(cros_ec_cec);
>> +		return NOTIFY_OK;
>> +	}
>> +
>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>> +		handle_cec_message(cros_ec_cec);
>> +		return NOTIFY_OK;
>> +	}
>> +
>> +	return NOTIFY_DONE;
>> +}
>> +
>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_set data;
>> +	} __packed msg;
> 
> Just say: ... msg = {};
> 
>> +	int ret = 0;
>> +
>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>> +		return 0;
> 
> This looks weird. If I had configured a LA before, then this should unconfigure
> it. But it just keeps the existing LA, so any poll messages to us will still
> be Acked. Or am I missing something?
> 
>> +
>> +	memset(&msg, 0, sizeof(msg));
> 
> and you can drop this memset.

I always forget this trick.

> 
>> +	msg.msg.command = EC_CMD_CEC_SET;
>> +	msg.msg.outsize = sizeof(msg.data);
>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>> +	msg.data.address = logical_addr;
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error setting CEC logical address on EC: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_write data;
>> +	} __packed msg;
> 
> ... msg = {};
> 
>> +	int ret = 0;
>> +
>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>> +		return -EINVAL;
> 
> No need, can never happen.

Forgor to remove it, the API changed in the meantime, now it can never happen !

> 
>> +
>> +	memset(&msg, 0, sizeof(msg));
> 
> and drop this.
> 
>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>> +	msg.msg.outsize = cec_msg->len;
>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error writting CEC msg on EC: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>> +	struct {
>> +		struct cros_ec_command msg;
>> +		struct ec_params_cec_set data;
>> +	} __packed msg;
>> +	int ret = 0;
>> +
>> +	memset(&msg, 0, sizeof(msg));
>> +	msg.msg.command = EC_CMD_CEC_SET;
>> +	msg.msg.outsize = sizeof(msg.data);
>> +	msg.data.cmd = CEC_CMD_ENABLE;
>> +	msg.data.enable = enable;
>> +
>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>> +	if (ret < 0) {
>> +		dev_err(cros_ec->dev,
>> +			"error %sabling CEC on EC: %d\n",
>> +			(enable ? "en" : "dis"), ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>> +	.adap_enable = cros_ec_cec_adap_enable,
>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>> +	.adap_transmit = cros_ec_cec_transmit,
>> +};
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int cros_ec_cec_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>> +
>> +	if (device_may_wakeup(dev))
>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>> +
>> +	if (device_may_wakeup(dev))
>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>> +
>> +
>> +struct cec_dmi_match {
>> +	char *sys_vendor;
>> +	char *product_name;
>> +	char *devname;
>> +	char *conn;
>> +};
>> +
>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>> +	/* Google Fizz */
>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>> +};
>> +
>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>> +{
>> +	int i;
>> +
>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>> +
>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
> 
> See my comments for patch 2/5.

I will change with you comment, looks way cleaner.

> 
>> +			return 0;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>> +{
>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>> +	struct cros_ec_cec *cros_ec_cec;
>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
> 
> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
> signal?

No, Yes, the EC FW is independent of the HPD status

> 
>> +	int ret;
>> +
>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>> +				   GFP_KERNEL);
>> +	if (!cros_ec_cec)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, cros_ec_cec);
>> +	cros_ec_cec->cros_ec = cros_ec;
>> +
>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>> +	if (ret) {
>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
> 
> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
> using this capability unless there is no other option. It's a pain for userspace.

It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
it won't fail but still enable the CEC interface without a notifier.

> 
>> +	} else if (!cros_ec_cec->notify) {
>> +		return -EPROBE_DEFER;
>> +	}
>> +
>> +	ret = device_init_wakeup(&pdev->dev, 1);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>> +		return ret;
>> +	}
>> +
>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>> +						 DRV_NAME, cec_caps, 1);
>> +	if (IS_ERR(cros_ec_cec->adap))
>> +		return PTR_ERR(cros_ec_cec->adap);
>> +
>> +	cros_ec_cec->adap->owner = THIS_MODULE;
> 
> This can be dropped, cec_register_adapter() sets this already.

Noted.

> 
>> +
>> +	/* Get CEC events from the EC. */
>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>> +					       &cros_ec_cec->notifier);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>> +		cec_delete_adapter(cros_ec_cec->adap);
>> +		return ret;
>> +	}
>> +
>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>> +	if (ret < 0) {
>> +		cec_delete_adapter(cros_ec_cec->adap);
>> +		return ret;
>> +	}
>> +
>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>> +
>> +	return 0;
>> +}
>> +
>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>> +{
>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>> +	struct device *dev = &pdev->dev;
>> +	int ret;
>> +
>> +	ret = blocking_notifier_chain_unregister(
>> +			&cros_ec_cec->cros_ec->event_notifier,
>> +			&cros_ec_cec->notifier);
>> +
>> +	if (ret) {
>> +		dev_err(dev, "failed to unregister notifier\n");
>> +		return ret;
>> +	}
>> +
>> +	cec_unregister_adapter(cros_ec_cec->adap);
>> +
>> +	if (cros_ec_cec->notify)
>> +		cec_notifier_put(cros_ec_cec->notify);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver cros_ec_cec_driver = {
>> +	.probe = cros_ec_cec_probe,
>> +	.remove  = cros_ec_cec_remove,
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +		.pm = &cros_ec_cec_pm_ops,
>> +	},
>> +};
>> +
>> +module_platform_driver(cros_ec_cec_driver);
>> +
>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRV_NAME);
>>
> 
> Regards,
> 
> 	Hans
> 

Thanks,
Neil

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 2/5] media: cec-notifier: Get notifier by device and connector name

[Neil Armstrong]

On 15/05/2018 08:27, Hans Verkuil wrote:
> Hi Neil,
> 
> Thanks for this patch series!
> 
> Some comments below:
> 
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> In non device-tree world, we can need to get the notifier by the driver
>> name directly and eventually defer probe if not yet created.
>>
>> This patch adds a variant of the get function by using the device name
>> instead and will not create a notifier if not yet created.
>>
>> But the i915 driver exposes at least 2 HDMI connectors, this patch also
>> adds the possibility to add a connector name tied to the notifier device
>> to form a tuple and associate different CEC controllers for each HDMI
>> connectors.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
>>  include/media/cec-notifier.h     | 44 ++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
>> index 16dffa0..716070a 100644
>> --- a/drivers/media/cec/cec-notifier.c
>> +++ b/drivers/media/cec/cec-notifier.c
>> @@ -21,6 +21,7 @@ struct cec_notifier {
>>  	struct list_head head;
>>  	struct kref kref;
>>  	struct device *dev;
>> +	const char *conn;
>>  	struct cec_adapter *cec_adap;
>>  	void (*callback)(struct cec_adapter *adap, u16 pa);
>>  
>> @@ -30,13 +31,34 @@ struct cec_notifier {
>>  static LIST_HEAD(cec_notifiers);
>>  static DEFINE_MUTEX(cec_notifiers_lock);
>>  
>> -struct cec_notifier *cec_notifier_get(struct device *dev)
>> +struct cec_notifier *cec_notifier_get_byname(const char *name,
>> +					     const char *conn)
>>  {
>>  	struct cec_notifier *n;
>>  
>>  	mutex_lock(&cec_notifiers_lock);
>>  	list_for_each_entry(n, &cec_notifiers, head) {
>> -		if (n->dev == dev) {
>> +		if (!strcmp(dev_name(n->dev), name) &&
>> +		    (!conn || !strcmp(n->conn, conn))) {
>> +			kref_get(&n->kref);
>> +			mutex_unlock(&cec_notifiers_lock);
>> +			return n;
>> +		}
>> +	}
>> +	mutex_unlock(&cec_notifiers_lock);
>> +
>> +	return NULL;
> 
> This doesn't seem right. For one it doesn't act like the other cec_notifier_get*
> functions in that it doesn't make a new notifier if it wasn't found yet in the
> list.
> 
> For another, I think this function shouldn't be here at all. How about calling
> bus_find_device_by_name(), then use cec_notifier_get_conn()?

Yes, it's safer and will keep the original cec_notifier_get() behavior.

> 
>> +}
>> +EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
>> +
>> +struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
>> +{
>> +	struct cec_notifier *n;
>> +
>> +	mutex_lock(&cec_notifiers_lock);
>> +	list_for_each_entry(n, &cec_notifiers, head) {
>> +		if (n->dev == dev &&
>> +		    (!conn || !strcmp(n->conn, conn))) {
>>  			kref_get(&n->kref);
>>  			mutex_unlock(&cec_notifiers_lock);
>>  			return n;
>> @@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>>  	if (!n)
>>  		goto unlock;
>>  	n->dev = dev;
>> +	if (conn)
>> +		n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);
> 
> The use of devm_kstrdup worries me. The problem is that when the 'dev' device
> is removed, this memory is also automatically freed. But the notifier might
> still have a reference through the CEC driver, so you end up with a n->conn
> pointer that points to freed memory.
> 
> I think it is better to just use kstrdup and kfree it when the last notifier
> reference is released.

Ok

> 
>>  	n->phys_addr = CEC_PHYS_ADDR_INVALID;
>>  	mutex_init(&n->lock);
>>  	kref_init(&n->kref);
>> @@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>>  	mutex_unlock(&cec_notifiers_lock);
>>  	return n;
>>  }
>> -EXPORT_SYMBOL_GPL(cec_notifier_get);
>> +EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
>>  
>>  static void cec_notifier_release(struct kref *kref)
>>  {
>> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
>> index cf0add7..70f2974 100644
>> --- a/include/media/cec-notifier.h
>> +++ b/include/media/cec-notifier.h
>> @@ -20,6 +20,37 @@ struct cec_notifier;
>>  #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>>  
>>  /**
>> + * cec_notifier_get_byname - find a cec_notifier for the given device name
>> + * and connector tuple.
>> + * @name: device name that sends the events.
>> + * @conn: the connector name from which the event occurs
>> + *
>> + * If a notifier for device @name exists, then increase the refcount and
>> + * return that notifier.
>> + *
>> + * If it doesn't exist, return NULL
>> + */
>> +struct cec_notifier *cec_notifier_get_byname(const char *name,
>> +					     const char *conn);
>> +
>> +/**
>> + * cec_notifier_get_conn - find or create a new cec_notifier for the given
>> + * device and connector tuple.
>> + * @dev: device that sends the events.
>> + * @conn: the connector name from which the event occurs
>> + *
>> + * If a notifier for device @dev already exists, then increase the refcount
>> + * and return that notifier.
>> + *
>> + * If it doesn't exist, then allocate a new notifier struct and return a
>> + * pointer to that new struct.
>> + *
>> + * Return NULL if the memory could not be allocated.
>> + */
>> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
>> +					   const char *conn);
>> +
>> +/**
>>   * cec_notifier_get - find or create a new cec_notifier for the given device.
>>   * @dev: device that sends the events.
>>   *
>> @@ -31,7 +62,10 @@ struct cec_notifier;
>>   *
>>   * Return NULL if the memory could not be allocated.
>>   */
>> -struct cec_notifier *cec_notifier_get(struct device *dev);
>> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> +	return cec_notifier_get_conn(dev, NULL);
>> +}
>>  
>>  /**
>>   * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
>> @@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
>>  			       struct cec_notifier *notifier);
>>  
>>  #else
>> -static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
>> +							 const char *conn)
>>  {
>>  	/* A non-NULL pointer is expected on success */
>>  	return (struct cec_notifier *)0xdeadfeed;
>>  }
>>  
>> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> +	return cec_notifier_get_conn(dev, NULL);
>> +}
>> +
>>  static inline void cec_notifier_put(struct cec_notifier *n)
>>  {
>>  }
>>
> 
> Regards,
> 
> 	Hans
> 

Re: [RFC PATCH 2/5] media: cec-notifier: Get notifier by device and connector name

[Neil Armstrong]

On 15/05/2018 08:27, Hans Verkuil wrote:
> Hi Neil,
> 
> Thanks for this patch series!
> 
> Some comments below:
> 
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> In non device-tree world, we can need to get the notifier by the driver
>> name directly and eventually defer probe if not yet created.
>>
>> This patch adds a variant of the get function by using the device name
>> instead and will not create a notifier if not yet created.
>>
>> But the i915 driver exposes at least 2 HDMI connectors, this patch also
>> adds the possibility to add a connector name tied to the notifier device
>> to form a tuple and associate different CEC controllers for each HDMI
>> connectors.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/media/cec/cec-notifier.c | 30 ++++++++++++++++++++++++---
>>  include/media/cec-notifier.h     | 44 ++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
>> index 16dffa0..716070a 100644
>> --- a/drivers/media/cec/cec-notifier.c
>> +++ b/drivers/media/cec/cec-notifier.c
>> @@ -21,6 +21,7 @@ struct cec_notifier {
>>  	struct list_head head;
>>  	struct kref kref;
>>  	struct device *dev;
>> +	const char *conn;
>>  	struct cec_adapter *cec_adap;
>>  	void (*callback)(struct cec_adapter *adap, u16 pa);
>>  
>> @@ -30,13 +31,34 @@ struct cec_notifier {
>>  static LIST_HEAD(cec_notifiers);
>>  static DEFINE_MUTEX(cec_notifiers_lock);
>>  
>> -struct cec_notifier *cec_notifier_get(struct device *dev)
>> +struct cec_notifier *cec_notifier_get_byname(const char *name,
>> +					     const char *conn)
>>  {
>>  	struct cec_notifier *n;
>>  
>>  	mutex_lock(&cec_notifiers_lock);
>>  	list_for_each_entry(n, &cec_notifiers, head) {
>> -		if (n->dev == dev) {
>> +		if (!strcmp(dev_name(n->dev), name) &&
>> +		    (!conn || !strcmp(n->conn, conn))) {
>> +			kref_get(&n->kref);
>> +			mutex_unlock(&cec_notifiers_lock);
>> +			return n;
>> +		}
>> +	}
>> +	mutex_unlock(&cec_notifiers_lock);
>> +
>> +	return NULL;
> 
> This doesn't seem right. For one it doesn't act like the other cec_notifier_get*
> functions in that it doesn't make a new notifier if it wasn't found yet in the
> list.
> 
> For another, I think this function shouldn't be here at all. How about calling
> bus_find_device_by_name(), then use cec_notifier_get_conn()?

Yes, it's safer and will keep the original cec_notifier_get() behavior.

> 
>> +}
>> +EXPORT_SYMBOL_GPL(cec_notifier_get_byname);
>> +
>> +struct cec_notifier *cec_notifier_get_conn(struct device *dev, const char *conn)
>> +{
>> +	struct cec_notifier *n;
>> +
>> +	mutex_lock(&cec_notifiers_lock);
>> +	list_for_each_entry(n, &cec_notifiers, head) {
>> +		if (n->dev == dev &&
>> +		    (!conn || !strcmp(n->conn, conn))) {
>>  			kref_get(&n->kref);
>>  			mutex_unlock(&cec_notifiers_lock);
>>  			return n;
>> @@ -46,6 +68,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>>  	if (!n)
>>  		goto unlock;
>>  	n->dev = dev;
>> +	if (conn)
>> +		n->conn = devm_kstrdup(dev, conn, GFP_KERNEL);
> 
> The use of devm_kstrdup worries me. The problem is that when the 'dev' device
> is removed, this memory is also automatically freed. But the notifier might
> still have a reference through the CEC driver, so you end up with a n->conn
> pointer that points to freed memory.
> 
> I think it is better to just use kstrdup and kfree it when the last notifier
> reference is released.

Ok

> 
>>  	n->phys_addr = CEC_PHYS_ADDR_INVALID;
>>  	mutex_init(&n->lock);
>>  	kref_init(&n->kref);
>> @@ -54,7 +78,7 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>>  	mutex_unlock(&cec_notifiers_lock);
>>  	return n;
>>  }
>> -EXPORT_SYMBOL_GPL(cec_notifier_get);
>> +EXPORT_SYMBOL_GPL(cec_notifier_get_conn);
>>  
>>  static void cec_notifier_release(struct kref *kref)
>>  {
>> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
>> index cf0add7..70f2974 100644
>> --- a/include/media/cec-notifier.h
>> +++ b/include/media/cec-notifier.h
>> @@ -20,6 +20,37 @@ struct cec_notifier;
>>  #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>>  
>>  /**
>> + * cec_notifier_get_byname - find a cec_notifier for the given device name
>> + * and connector tuple.
>> + * @name: device name that sends the events.
>> + * @conn: the connector name from which the event occurs
>> + *
>> + * If a notifier for device @name exists, then increase the refcount and
>> + * return that notifier.
>> + *
>> + * If it doesn't exist, return NULL
>> + */
>> +struct cec_notifier *cec_notifier_get_byname(const char *name,
>> +					     const char *conn);
>> +
>> +/**
>> + * cec_notifier_get_conn - find or create a new cec_notifier for the given
>> + * device and connector tuple.
>> + * @dev: device that sends the events.
>> + * @conn: the connector name from which the event occurs
>> + *
>> + * If a notifier for device @dev already exists, then increase the refcount
>> + * and return that notifier.
>> + *
>> + * If it doesn't exist, then allocate a new notifier struct and return a
>> + * pointer to that new struct.
>> + *
>> + * Return NULL if the memory could not be allocated.
>> + */
>> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
>> +					   const char *conn);
>> +
>> +/**
>>   * cec_notifier_get - find or create a new cec_notifier for the given device.
>>   * @dev: device that sends the events.
>>   *
>> @@ -31,7 +62,10 @@ struct cec_notifier;
>>   *
>>   * Return NULL if the memory could not be allocated.
>>   */
>> -struct cec_notifier *cec_notifier_get(struct device *dev);
>> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> +	return cec_notifier_get_conn(dev, NULL);
>> +}
>>  
>>  /**
>>   * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
>> @@ -85,12 +119,18 @@ void cec_register_cec_notifier(struct cec_adapter *adap,
>>  			       struct cec_notifier *notifier);
>>  
>>  #else
>> -static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +static inline struct cec_notifier *cec_notifier_get_conn(struct device *dev,
>> +							 const char *conn)
>>  {
>>  	/* A non-NULL pointer is expected on success */
>>  	return (struct cec_notifier *)0xdeadfeed;
>>  }
>>  
>> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> +	return cec_notifier_get_conn(dev, NULL);
>> +}
>> +
>>  static inline void cec_notifier_put(struct cec_notifier *n)
>>  {
>>  }
>>
> 
> Regards,
> 
> 	Hans
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

On 15/05/2018 08:34, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> This patchs adds the cec_notifier feature to the intel_hdmi part
>> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
>> between each HDMI ports.
>> The changes will allow the i915 HDMI code to notify EDID and HPD changes
>> to an eventual CEC adapter.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
> 
> The Kconfig also needs to be changed. In the DRM_I915 you need to add:
> 
> 	select CEC_CORE if CEC_NOTIFIER

OK, thanks

> 
> Otherwise you'll get problems if the cec driver is a module and i915 is built-in.
> 
> Regards,
> 
> 	Hans
> 

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

On 15/05/2018 08:34, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> This patchs adds the cec_notifier feature to the intel_hdmi part
>> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
>> between each HDMI ports.
>> The changes will allow the i915 HDMI code to notify EDID and HPD changes
>> to an eventual CEC adapter.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
> 
> The Kconfig also needs to be changed. In the DRM_I915 you need to add:
> 
> 	select CEC_CORE if CEC_NOTIFIER

OK, thanks

> 
> Otherwise you'll get problems if the cec driver is a module and i915 is built-in.
> 
> Regards,
> 
> 	Hans
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

On 15/05/2018 08:29, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> This patchs adds the cec_notifier feature to the intel_hdmi part
>> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
>> between each HDMI ports.
>> The changes will allow the i915 HDMI code to notify EDID and HPD changes
>> to an eventual CEC adapter.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d436858..b50e51b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -39,6 +39,7 @@
>>  #include <drm/drm_dp_mst_helper.h>
>>  #include <drm/drm_rect.h>
>>  #include <drm/drm_atomic.h>
>> +#include <media/cec-notifier.h>
>>  
>>  /**
>>   * __wait_for - magic wait macro
>> @@ -1001,6 +1002,7 @@ struct intel_hdmi {
>>  	bool has_audio;
>>  	bool rgb_quant_range_selectable;
>>  	struct intel_connector *attached_connector;
>> +	struct cec_notifier *notifier;
>>  };
>>  
>>  struct intel_dp_mst_encoder;
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 1baef4a..9b94d72 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>>  		connected = true;
>>  	}
>>  
>> +	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
>> +
>>  	return connected;
>>  }
>>  
>> @@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>  {
>>  	enum drm_connector_status status;
>>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>>  
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>  		      connector->base.id, connector->name);
>> @@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>  
>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>>  
>> +	if (status != connector_status_connected)
>> +		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
>> +
>>  	return status;
>>  }
>>  
>> @@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>>  	}
>> +
>> +	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
>> +	if (!intel_hdmi->notifier)
>> +		DRM_DEBUG_KMS("CEC notifier get failed\n");
> 
> You 'get' the notifier here, but where is the cec_notifier_put when the connector is deleted?

Because I failed to find a safe place for this !

I will have a second look...

> 
> Regards,
> 
> 	Hans
> 
>>  }
>>  
>>  void intel_hdmi_init(struct drm_i915_private *dev_priv,
>>
> 

Re: [RFC PATCH 3/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

[Neil Armstrong]

On 15/05/2018 08:29, Hans Verkuil wrote:
> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>> This patchs adds the cec_notifier feature to the intel_hdmi part
>> of the i915 DRM driver. It uses the HDMI DRM connector name to differentiate
>> between each HDMI ports.
>> The changes will allow the i915 HDMI code to notify EDID and HPD changes
>> to an eventual CEC adapter.
>>
>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h  |  2 ++
>>  drivers/gpu/drm/i915/intel_hdmi.c | 10 ++++++++++
>>  2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index d436858..b50e51b 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -39,6 +39,7 @@
>>  #include <drm/drm_dp_mst_helper.h>
>>  #include <drm/drm_rect.h>
>>  #include <drm/drm_atomic.h>
>> +#include <media/cec-notifier.h>
>>  
>>  /**
>>   * __wait_for - magic wait macro
>> @@ -1001,6 +1002,7 @@ struct intel_hdmi {
>>  	bool has_audio;
>>  	bool rgb_quant_range_selectable;
>>  	struct intel_connector *attached_connector;
>> +	struct cec_notifier *notifier;
>>  };
>>  
>>  struct intel_dp_mst_encoder;
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 1baef4a..9b94d72 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1868,6 +1868,8 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>>  		connected = true;
>>  	}
>>  
>> +	cec_notifier_set_phys_addr_from_edid(intel_hdmi->notifier, edid);
>> +
>>  	return connected;
>>  }
>>  
>> @@ -1876,6 +1878,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>  {
>>  	enum drm_connector_status status;
>>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>>  
>>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>  		      connector->base.id, connector->name);
>> @@ -1891,6 +1894,9 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
>>  
>>  	intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
>>  
>> +	if (status != connector_status_connected)
>> +		cec_notifier_phys_addr_invalidate(intel_hdmi->notifier);
>> +
>>  	return status;
>>  }
>>  
>> @@ -2358,6 +2364,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>  		u32 temp = I915_READ(PEG_BAND_GAP_DATA);
>>  		I915_WRITE(PEG_BAND_GAP_DATA, (temp & ~0xf) | 0xd);
>>  	}
>> +
>> +	intel_hdmi->notifier = cec_notifier_get_conn(dev->dev, connector->name);
>> +	if (!intel_hdmi->notifier)
>> +		DRM_DEBUG_KMS("CEC notifier get failed\n");
> 
> You 'get' the notifier here, but where is the cec_notifier_put when the connector is deleted?

Because I failed to find a safe place for this !

I will have a second look...

> 
> Regards,
> 
> 	Hans
> 
>>  }
>>  
>>  void intel_hdmi_init(struct drm_i915_private *dev_priv,
>>
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/18 09:25, Neil Armstrong wrote:
> Hi Hans,
> 
> Thanks for the extensive review.
> 
> On 15/05/2018 08:58, Hans Verkuil wrote:
>> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>>> driver for such feature of the Embedded Controller.
>>>
>>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>>> the feature bit is exposed by the EC.
>>>
>>> The controller will only handle a single logical address and handles
>>> all the messages retries and will only expose Success or Error.
>>>
>>> When the logical address is invalid, the controller will act as a CEC sniffer
>>> and transfer all messages on the bus.
>>
>> I did not see any support for this. If this works as you state here, then adding
>> support for CEC_CAP_MONITOR_ALL is highly recommended.
> 
> Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
> dropped for the first revision.
> 
>>
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/media/platform/Kconfig               |  11 +
>>>  drivers/media/platform/Makefile              |   2 +
>>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>>  4 files changed, 345 insertions(+)
>>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
>>
>> Shouldn't the directory be called cros-ec-cec?
>>
>>>
>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>> index c7a1cf8..e55a8ed2 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>>  
>>>  if CEC_PLATFORM_DRIVERS
>>>  
>>> +config VIDEO_CROS_EC_CEC
>>> +	tristate "Chrome OS EC CEC driver"
>>> +	depends on MFD_CROS_EC || COMPILE_TEST
>>> +	select CEC_CORE
>>> +	select CEC_NOTIFIER
>>> +	---help---
>>> +	  If you say yes here you will get support for the
>>> +	  Chrome OS Embedded Controller's CEC.
>>> +	  The CEC bus is present in the HDMI connector and enables communication
>>> +	  between compatible devices.
>>> +
>>>  config VIDEO_MESON_AO_CEC
>>>  	tristate "Amlogic Meson AO CEC driver"
>>>  	depends on ARCH_MESON || COMPILE_TEST
>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>> index 932515d..0e0582e 100644
>>> --- a/drivers/media/platform/Makefile
>>> +++ b/drivers/media/platform/Makefile
>>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>>  
>>>  obj-y					+= meson/
>>> +
>>> +obj-y					+= cros-ec/
>>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>>> new file mode 100644
>>> index 0000000..9ce97f9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/cros-ec/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>> new file mode 100644
>>> index 0000000..fea90da
>>> --- /dev/null
>>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>> @@ -0,0 +1,331 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * CEC driver for Chrome OS Embedded Controller
>>> + *
>>> + * Copyright (c) 2018 BayLibre, SAS
>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/cec.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/interrupt.h>
>>> +#include <media/cec.h>
>>> +#include <media/cec-notifier.h>
>>> +#include <linux/mfd/cros_ec.h>
>>> +#include <linux/mfd/cros_ec_commands.h>
>>> +
>>> +#define DRV_NAME	"cros-ec-cec"
>>> +
>>> +/**
>>> + * struct cros_ec_cec - Driver data for EC CEC
>>> + *
>>> + * @cros_ec: Pointer to EC device
>>> + * @notifier: Notifier info for responding to EC events
>>> + * @adap: CEC adapter
>>> + * @notify: CEC notifier pointer
>>> + * @rc_msg: storage for a received message
>>> + */
>>> +struct cros_ec_cec {
>>> +	struct cros_ec_device *cros_ec;
>>> +	struct notifier_block notifier;
>>> +	struct cec_adapter *adap;
>>> +	struct cec_notifier *notify;
>>> +	struct cec_msg rx_msg;
>>> +};
>>> +
>>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>>> +{
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>>> +	unsigned int len = cros_ec->event_size;
>>> +
>>> +	cros_ec_cec->rx_msg.len = len;
>>
>> How robust is the underlying code and hardware? What happens if a
>> CEC message with more than 16 bytes is received?
>>
>> Hard to test unless you have an RPi3 set up as a CEC debugger. See
>> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
>>
>> Since you worked with CEC for a while now I recommend you set up such
>> a system. It's cheap and very useful.
> 
> I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....
> 
> But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.

You really want a setup that uses the cec-gpio driver since that allows
you to do CEC error injection. If you can hook up the CEC pin to a GPIO
on the stm32 board, than that should work just as well as with the RPi3.

> 
>>
>>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>>> +
>>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>>> +}
>>> +
>>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>>> +{
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>>> +
>>> +	if (events & EC_MKBP_CEC_SEND_OK)
>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>> +					  CEC_TX_STATUS_OK);
>>> +
>>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>> +					  CEC_TX_STATUS_ERROR);
>>
>> You said above that the HW takes care of all the message retries.
>> If that's the case, then you need to use cec_transmit_done here
>> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
>> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
>> to retry the message because the HW already did that.
>>
>> NACK is better than ERROR if the hardware supports only an OK/FAIL
>> result.
> 
> Ok, we were unsure about this, thanks for the clarification.
> 
>>
>>> +}
>>> +
>>> +static int cros_ec_cec_event(struct notifier_block *nb,
>>> +	unsigned long queued_during_suspend, void *_notify)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec;
>>> +	struct cros_ec_device *cros_ec;
>>> +
>>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>>> +	cros_ec = cros_ec_cec->cros_ec;
>>> +
>>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>>> +		handle_cec_event(cros_ec_cec);
>>> +		return NOTIFY_OK;
>>> +	}
>>> +
>>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>>> +		handle_cec_message(cros_ec_cec);
>>> +		return NOTIFY_OK;
>>> +	}
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_set data;
>>> +	} __packed msg;
>>
>> Just say: ... msg = {};
>>
>>> +	int ret = 0;
>>> +
>>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>>> +		return 0;
>>
>> This looks weird. If I had configured a LA before, then this should unconfigure
>> it. But it just keeps the existing LA, so any poll messages to us will still
>> be Acked. Or am I missing something?

Did you miss this comment? It's weird what is happening here.

>>
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>
>> and you can drop this memset.
> 
> I always forget this trick.
> 
>>
>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>> +	msg.msg.outsize = sizeof(msg.data);
>>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>>> +	msg.data.address = logical_addr;
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error setting CEC logical address on EC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_write data;
>>> +	} __packed msg;
>>
>> ... msg = {};
>>
>>> +	int ret = 0;
>>> +
>>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>>> +		return -EINVAL;
>>
>> No need, can never happen.
> 
> Forgor to remove it, the API changed in the meantime, now it can never happen !
> 
>>
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>
>> and drop this.
>>
>>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>>> +	msg.msg.outsize = cec_msg->len;
>>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error writting CEC msg on EC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_set data;
>>> +	} __packed msg;
>>> +	int ret = 0;
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>> +	msg.msg.outsize = sizeof(msg.data);
>>> +	msg.data.cmd = CEC_CMD_ENABLE;
>>> +	msg.data.enable = enable;
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error %sabling CEC on EC: %d\n",
>>> +			(enable ? "en" : "dis"), ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>>> +	.adap_enable = cros_ec_cec_adap_enable,
>>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>>> +	.adap_transmit = cros_ec_cec_transmit,
>>> +};
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int cros_ec_cec_suspend(struct device *dev)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	if (device_may_wakeup(dev))
>>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_resume(struct device *dev)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	if (device_may_wakeup(dev))
>>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>>> +
>>> +
>>> +struct cec_dmi_match {
>>> +	char *sys_vendor;
>>> +	char *product_name;
>>> +	char *devname;
>>> +	char *conn;
>>> +};
>>> +
>>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>>> +	/* Google Fizz */
>>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>>> +};
>>> +
>>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>>> +
>>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
>>
>> See my comments for patch 2/5.
> 
> I will change with you comment, looks way cleaner.
> 
>>
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>>> +{
>>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>>> +	struct cros_ec_cec *cros_ec_cec;
>>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
>>
>> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
>> signal?
> 
> No, Yes, the EC FW is independent of the HPD status
> 
>>
>>> +	int ret;
>>> +
>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>> +				   GFP_KERNEL);
>>> +	if (!cros_ec_cec)
>>> +		return -ENOMEM;
>>> +
>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>> +	cros_ec_cec->cros_ec = cros_ec;
>>> +
>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>> +	if (ret) {
>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>
>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>> using this capability unless there is no other option. It's a pain for userspace.
> 
> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
> it won't fail but still enable the CEC interface without a notifier.

I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
where it is truly impossible to tell which output is connected to the CEC adapter.
That's the case with e.g. USB CEC dongles where you have no idea how the user
connected the HDMI cables.

But I assume that in this case it just means that the cec_dmi_match_table needs
to be updated, i.e. it is a driver bug.

Another thing: this driver assumes that there is only one CEC adapter for only
one HDMI output. But what if there are more HDMI outputs? Will there be one
CEC adapter for each output? Or does the hardware have no provisions for that?

Something should be mentioned about this in a comment.

Regards,

	Hans

> 
>>
>>> +	} else if (!cros_ec_cec->notify) {
>>> +		return -EPROBE_DEFER;
>>> +	}
>>> +
>>> +	ret = device_init_wakeup(&pdev->dev, 1);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>>> +						 DRV_NAME, cec_caps, 1);
>>> +	if (IS_ERR(cros_ec_cec->adap))
>>> +		return PTR_ERR(cros_ec_cec->adap);
>>> +
>>> +	cros_ec_cec->adap->owner = THIS_MODULE;
>>
>> This can be dropped, cec_register_adapter() sets this already.
> 
> Noted.
> 
>>
>>> +
>>> +	/* Get CEC events from the EC. */
>>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>>> +					       &cros_ec_cec->notifier);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>>> +	if (ret < 0) {
>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>> +		return ret;
>>> +	}
>>> +
>>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>>> +	struct device *dev = &pdev->dev;
>>> +	int ret;
>>> +
>>> +	ret = blocking_notifier_chain_unregister(
>>> +			&cros_ec_cec->cros_ec->event_notifier,
>>> +			&cros_ec_cec->notifier);
>>> +
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to unregister notifier\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	cec_unregister_adapter(cros_ec_cec->adap);
>>> +
>>> +	if (cros_ec_cec->notify)
>>> +		cec_notifier_put(cros_ec_cec->notify);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver cros_ec_cec_driver = {
>>> +	.probe = cros_ec_cec_probe,
>>> +	.remove  = cros_ec_cec_remove,
>>> +	.driver = {
>>> +		.name = DRV_NAME,
>>> +		.pm = &cros_ec_cec_pm_ops,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(cros_ec_cec_driver);
>>> +
>>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_ALIAS("platform:" DRV_NAME);
>>>
>>
>> Regards,
>>
>> 	Hans
>>
> 
> Thanks,
> Neil
> 

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/18 09:25, Neil Armstrong wrote:
> Hi Hans,
> 
> Thanks for the extensive review.
> 
> On 15/05/2018 08:58, Hans Verkuil wrote:
>> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>>> driver for such feature of the Embedded Controller.
>>>
>>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>>> the feature bit is exposed by the EC.
>>>
>>> The controller will only handle a single logical address and handles
>>> all the messages retries and will only expose Success or Error.
>>>
>>> When the logical address is invalid, the controller will act as a CEC sniffer
>>> and transfer all messages on the bus.
>>
>> I did not see any support for this. If this works as you state here, then adding
>> support for CEC_CAP_MONITOR_ALL is highly recommended.
> 
> Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
> dropped for the first revision.
> 
>>
>>>
>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>> ---
>>>  drivers/media/platform/Kconfig               |  11 +
>>>  drivers/media/platform/Makefile              |   2 +
>>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>>  4 files changed, 345 insertions(+)
>>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
>>
>> Shouldn't the directory be called cros-ec-cec?
>>
>>>
>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>> index c7a1cf8..e55a8ed2 100644
>>> --- a/drivers/media/platform/Kconfig
>>> +++ b/drivers/media/platform/Kconfig
>>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>>  
>>>  if CEC_PLATFORM_DRIVERS
>>>  
>>> +config VIDEO_CROS_EC_CEC
>>> +	tristate "Chrome OS EC CEC driver"
>>> +	depends on MFD_CROS_EC || COMPILE_TEST
>>> +	select CEC_CORE
>>> +	select CEC_NOTIFIER
>>> +	---help---
>>> +	  If you say yes here you will get support for the
>>> +	  Chrome OS Embedded Controller's CEC.
>>> +	  The CEC bus is present in the HDMI connector and enables communication
>>> +	  between compatible devices.
>>> +
>>>  config VIDEO_MESON_AO_CEC
>>>  	tristate "Amlogic Meson AO CEC driver"
>>>  	depends on ARCH_MESON || COMPILE_TEST
>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>> index 932515d..0e0582e 100644
>>> --- a/drivers/media/platform/Makefile
>>> +++ b/drivers/media/platform/Makefile
>>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>>  
>>>  obj-y					+= meson/
>>> +
>>> +obj-y					+= cros-ec/
>>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>>> new file mode 100644
>>> index 0000000..9ce97f9
>>> --- /dev/null
>>> +++ b/drivers/media/platform/cros-ec/Makefile
>>> @@ -0,0 +1 @@
>>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>> new file mode 100644
>>> index 0000000..fea90da
>>> --- /dev/null
>>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>> @@ -0,0 +1,331 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * CEC driver for Chrome OS Embedded Controller
>>> + *
>>> + * Copyright (c) 2018 BayLibre, SAS
>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/dmi.h>
>>> +#include <linux/cec.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/interrupt.h>
>>> +#include <media/cec.h>
>>> +#include <media/cec-notifier.h>
>>> +#include <linux/mfd/cros_ec.h>
>>> +#include <linux/mfd/cros_ec_commands.h>
>>> +
>>> +#define DRV_NAME	"cros-ec-cec"
>>> +
>>> +/**
>>> + * struct cros_ec_cec - Driver data for EC CEC
>>> + *
>>> + * @cros_ec: Pointer to EC device
>>> + * @notifier: Notifier info for responding to EC events
>>> + * @adap: CEC adapter
>>> + * @notify: CEC notifier pointer
>>> + * @rc_msg: storage for a received message
>>> + */
>>> +struct cros_ec_cec {
>>> +	struct cros_ec_device *cros_ec;
>>> +	struct notifier_block notifier;
>>> +	struct cec_adapter *adap;
>>> +	struct cec_notifier *notify;
>>> +	struct cec_msg rx_msg;
>>> +};
>>> +
>>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>>> +{
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>>> +	unsigned int len = cros_ec->event_size;
>>> +
>>> +	cros_ec_cec->rx_msg.len = len;
>>
>> How robust is the underlying code and hardware? What happens if a
>> CEC message with more than 16 bytes is received?
>>
>> Hard to test unless you have an RPi3 set up as a CEC debugger. See
>> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
>>
>> Since you worked with CEC for a while now I recommend you set up such
>> a system. It's cheap and very useful.
> 
> I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....
> 
> But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.

You really want a setup that uses the cec-gpio driver since that allows
you to do CEC error injection. If you can hook up the CEC pin to a GPIO
on the stm32 board, than that should work just as well as with the RPi3.

> 
>>
>>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>>> +
>>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>>> +}
>>> +
>>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>>> +{
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>>> +
>>> +	if (events & EC_MKBP_CEC_SEND_OK)
>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>> +					  CEC_TX_STATUS_OK);
>>> +
>>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>> +					  CEC_TX_STATUS_ERROR);
>>
>> You said above that the HW takes care of all the message retries.
>> If that's the case, then you need to use cec_transmit_done here
>> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
>> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
>> to retry the message because the HW already did that.
>>
>> NACK is better than ERROR if the hardware supports only an OK/FAIL
>> result.
> 
> Ok, we were unsure about this, thanks for the clarification.
> 
>>
>>> +}
>>> +
>>> +static int cros_ec_cec_event(struct notifier_block *nb,
>>> +	unsigned long queued_during_suspend, void *_notify)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec;
>>> +	struct cros_ec_device *cros_ec;
>>> +
>>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>>> +	cros_ec = cros_ec_cec->cros_ec;
>>> +
>>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>>> +		handle_cec_event(cros_ec_cec);
>>> +		return NOTIFY_OK;
>>> +	}
>>> +
>>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>>> +		handle_cec_message(cros_ec_cec);
>>> +		return NOTIFY_OK;
>>> +	}
>>> +
>>> +	return NOTIFY_DONE;
>>> +}
>>> +
>>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_set data;
>>> +	} __packed msg;
>>
>> Just say: ... msg = {};
>>
>>> +	int ret = 0;
>>> +
>>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>>> +		return 0;
>>
>> This looks weird. If I had configured a LA before, then this should unconfigure
>> it. But it just keeps the existing LA, so any poll messages to us will still
>> be Acked. Or am I missing something?

Did you miss this comment? It's weird what is happening here.

>>
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>
>> and you can drop this memset.
> 
> I always forget this trick.
> 
>>
>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>> +	msg.msg.outsize = sizeof(msg.data);
>>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>>> +	msg.data.address = logical_addr;
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error setting CEC logical address on EC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_write data;
>>> +	} __packed msg;
>>
>> ... msg = {};
>>
>>> +	int ret = 0;
>>> +
>>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>>> +		return -EINVAL;
>>
>> No need, can never happen.
> 
> Forgor to remove it, the API changed in the meantime, now it can never happen !
> 
>>
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>
>> and drop this.
>>
>>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>>> +	msg.msg.outsize = cec_msg->len;
>>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error writting CEC msg on EC: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>> +	struct {
>>> +		struct cros_ec_command msg;
>>> +		struct ec_params_cec_set data;
>>> +	} __packed msg;
>>> +	int ret = 0;
>>> +
>>> +	memset(&msg, 0, sizeof(msg));
>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>> +	msg.msg.outsize = sizeof(msg.data);
>>> +	msg.data.cmd = CEC_CMD_ENABLE;
>>> +	msg.data.enable = enable;
>>> +
>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>> +	if (ret < 0) {
>>> +		dev_err(cros_ec->dev,
>>> +			"error %sabling CEC on EC: %d\n",
>>> +			(enable ? "en" : "dis"), ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>>> +	.adap_enable = cros_ec_cec_adap_enable,
>>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>>> +	.adap_transmit = cros_ec_cec_transmit,
>>> +};
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int cros_ec_cec_suspend(struct device *dev)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	if (device_may_wakeup(dev))
>>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_resume(struct device *dev)
>>> +{
>>> +	struct platform_device *pdev = to_platform_device(dev);
>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>> +
>>> +	if (device_may_wakeup(dev))
>>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>>> +
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
>>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>>> +
>>> +
>>> +struct cec_dmi_match {
>>> +	char *sys_vendor;
>>> +	char *product_name;
>>> +	char *devname;
>>> +	char *conn;
>>> +};
>>> +
>>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>>> +	/* Google Fizz */
>>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>>> +};
>>> +
>>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>>> +{
>>> +	int i;
>>> +
>>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>>> +
>>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
>>
>> See my comments for patch 2/5.
> 
> I will change with you comment, looks way cleaner.
> 
>>
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>>> +{
>>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>>> +	struct cros_ec_cec *cros_ec_cec;
>>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
>>
>> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
>> signal?
> 
> No, Yes, the EC FW is independent of the HPD status
> 
>>
>>> +	int ret;
>>> +
>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>> +				   GFP_KERNEL);
>>> +	if (!cros_ec_cec)
>>> +		return -ENOMEM;
>>> +
>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>> +	cros_ec_cec->cros_ec = cros_ec;
>>> +
>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>> +	if (ret) {
>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>
>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>> using this capability unless there is no other option. It's a pain for userspace.
> 
> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
> it won't fail but still enable the CEC interface without a notifier.

I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
where it is truly impossible to tell which output is connected to the CEC adapter.
That's the case with e.g. USB CEC dongles where you have no idea how the user
connected the HDMI cables.

But I assume that in this case it just means that the cec_dmi_match_table needs
to be updated, i.e. it is a driver bug.

Another thing: this driver assumes that there is only one CEC adapter for only
one HDMI output. But what if there are more HDMI outputs? Will there be one
CEC adapter for each output? Or does the hardware have no provisions for that?

Something should be mentioned about this in a comment.

Regards,

	Hans

> 
>>
>>> +	} else if (!cros_ec_cec->notify) {
>>> +		return -EPROBE_DEFER;
>>> +	}
>>> +
>>> +	ret = device_init_wakeup(&pdev->dev, 1);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>>> +						 DRV_NAME, cec_caps, 1);
>>> +	if (IS_ERR(cros_ec_cec->adap))
>>> +		return PTR_ERR(cros_ec_cec->adap);
>>> +
>>> +	cros_ec_cec->adap->owner = THIS_MODULE;
>>
>> This can be dropped, cec_register_adapter() sets this already.
> 
> Noted.
> 
>>
>>> +
>>> +	/* Get CEC events from the EC. */
>>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>>> +					       &cros_ec_cec->notifier);
>>> +	if (ret) {
>>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>> +		return ret;
>>> +	}
>>> +
>>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>>> +	if (ret < 0) {
>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>> +		return ret;
>>> +	}
>>> +
>>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>>> +{
>>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>>> +	struct device *dev = &pdev->dev;
>>> +	int ret;
>>> +
>>> +	ret = blocking_notifier_chain_unregister(
>>> +			&cros_ec_cec->cros_ec->event_notifier,
>>> +			&cros_ec_cec->notifier);
>>> +
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to unregister notifier\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	cec_unregister_adapter(cros_ec_cec->adap);
>>> +
>>> +	if (cros_ec_cec->notify)
>>> +		cec_notifier_put(cros_ec_cec->notify);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver cros_ec_cec_driver = {
>>> +	.probe = cros_ec_cec_probe,
>>> +	.remove  = cros_ec_cec_remove,
>>> +	.driver = {
>>> +		.name = DRV_NAME,
>>> +		.pm = &cros_ec_cec_pm_ops,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(cros_ec_cec_driver);
>>> +
>>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_ALIAS("platform:" DRV_NAME);
>>>
>>
>> Regards,
>>
>> 	Hans
>>
> 
> Thanks,
> Neil
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

On 15/05/2018 10:10, Hans Verkuil wrote:
> On 05/15/18 09:25, Neil Armstrong wrote:
>> Hi Hans,
>>
>> Thanks for the extensive review.
>>
>> On 15/05/2018 08:58, Hans Verkuil wrote:
>>> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>>>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>>>> driver for such feature of the Embedded Controller.
>>>>
>>>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>>>> the feature bit is exposed by the EC.
>>>>
>>>> The controller will only handle a single logical address and handles
>>>> all the messages retries and will only expose Success or Error.
>>>>
>>>> When the logical address is invalid, the controller will act as a CEC sniffer
>>>> and transfer all messages on the bus.
>>>
>>> I did not see any support for this. If this works as you state here, then adding
>>> support for CEC_CAP_MONITOR_ALL is highly recommended.
>>
>> Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
>> dropped for the first revision.
>>
>>>
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/media/platform/Kconfig               |  11 +
>>>>  drivers/media/platform/Makefile              |   2 +
>>>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>>>  4 files changed, 345 insertions(+)
>>>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
>>>
>>> Shouldn't the directory be called cros-ec-cec?

Forgot this one, moved to cros-ec-cec directory.

>>>
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index c7a1cf8..e55a8ed2 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>>>  
>>>>  if CEC_PLATFORM_DRIVERS
>>>>  
>>>> +config VIDEO_CROS_EC_CEC
>>>> +	tristate "Chrome OS EC CEC driver"
>>>> +	depends on MFD_CROS_EC || COMPILE_TEST
>>>> +	select CEC_CORE
>>>> +	select CEC_NOTIFIER
>>>> +	---help---
>>>> +	  If you say yes here you will get support for the
>>>> +	  Chrome OS Embedded Controller's CEC.
>>>> +	  The CEC bus is present in the HDMI connector and enables communication
>>>> +	  between compatible devices.
>>>> +
>>>>  config VIDEO_MESON_AO_CEC
>>>>  	tristate "Amlogic Meson AO CEC driver"
>>>>  	depends on ARCH_MESON || COMPILE_TEST
>>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>>> index 932515d..0e0582e 100644
>>>> --- a/drivers/media/platform/Makefile
>>>> +++ b/drivers/media/platform/Makefile
>>>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>>>  
>>>>  obj-y					+= meson/
>>>> +
>>>> +obj-y					+= cros-ec/
>>>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>>>> new file mode 100644
>>>> index 0000000..9ce97f9
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/cros-ec/Makefile
>>>> @@ -0,0 +1 @@
>>>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>>>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>>> new file mode 100644
>>>> index 0000000..fea90da
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>>> @@ -0,0 +1,331 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * CEC driver for Chrome OS Embedded Controller
>>>> + *
>>>> + * Copyright (c) 2018 BayLibre, SAS
>>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/cec.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <media/cec.h>
>>>> +#include <media/cec-notifier.h>
>>>> +#include <linux/mfd/cros_ec.h>
>>>> +#include <linux/mfd/cros_ec_commands.h>
>>>> +
>>>> +#define DRV_NAME	"cros-ec-cec"
>>>> +
>>>> +/**
>>>> + * struct cros_ec_cec - Driver data for EC CEC
>>>> + *
>>>> + * @cros_ec: Pointer to EC device
>>>> + * @notifier: Notifier info for responding to EC events
>>>> + * @adap: CEC adapter
>>>> + * @notify: CEC notifier pointer
>>>> + * @rc_msg: storage for a received message
>>>> + */
>>>> +struct cros_ec_cec {
>>>> +	struct cros_ec_device *cros_ec;
>>>> +	struct notifier_block notifier;
>>>> +	struct cec_adapter *adap;
>>>> +	struct cec_notifier *notify;
>>>> +	struct cec_msg rx_msg;
>>>> +};
>>>> +
>>>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>>>> +{
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>>>> +	unsigned int len = cros_ec->event_size;
>>>> +
>>>> +	cros_ec_cec->rx_msg.len = len;
>>>
>>> How robust is the underlying code and hardware? What happens if a
>>> CEC message with more than 16 bytes is received?
>>>
>>> Hard to test unless you have an RPi3 set up as a CEC debugger. See
>>> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
>>>
>>> Since you worked with CEC for a while now I recommend you set up such
>>> a system. It's cheap and very useful.
>>
>> I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....
>>
>> But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.
> 
> You really want a setup that uses the cec-gpio driver since that allows
> you to do CEC error injection. If you can hook up the CEC pin to a GPIO
> on the stm32 board, than that should work just as well as with the RPi3.
> 
>>
>>>
>>>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>>>> +
>>>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>>>> +}
>>>> +
>>>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>>>> +{
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>>>> +
>>>> +	if (events & EC_MKBP_CEC_SEND_OK)
>>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>>> +					  CEC_TX_STATUS_OK);
>>>> +
>>>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>>> +					  CEC_TX_STATUS_ERROR);
>>>
>>> You said above that the HW takes care of all the message retries.
>>> If that's the case, then you need to use cec_transmit_done here
>>> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
>>> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
>>> to retry the message because the HW already did that.
>>>
>>> NACK is better than ERROR if the hardware supports only an OK/FAIL
>>> result.
>>
>> Ok, we were unsure about this, thanks for the clarification.
>>
>>>
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_event(struct notifier_block *nb,
>>>> +	unsigned long queued_during_suspend, void *_notify)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec;
>>>> +	struct cros_ec_device *cros_ec;
>>>> +
>>>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>>>> +	cros_ec = cros_ec_cec->cros_ec;
>>>> +
>>>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>>>> +		handle_cec_event(cros_ec_cec);
>>>> +		return NOTIFY_OK;
>>>> +	}
>>>> +
>>>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>>>> +		handle_cec_message(cros_ec_cec);
>>>> +		return NOTIFY_OK;
>>>> +	}
>>>> +
>>>> +	return NOTIFY_DONE;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_set data;
>>>> +	} __packed msg;
>>>
>>> Just say: ... msg = {};
>>>
>>>> +	int ret = 0;
>>>> +
>>>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>>>> +		return 0;
>>>
>>> This looks weird. If I had configured a LA before, then this should unconfigure
>>> it. But it just keeps the existing LA, so any poll messages to us will still
>>> be Acked. Or am I missing something?
> 
> Did you miss this comment? It's weird what is happening here.

I forgot to answer, there was a bug in the firmware, now it's fixed and we can unconfigure the LA with CEC_LOG_ADDR_INVALID.

> 
>>>
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>
>>> and you can drop this memset.
>>
>> I always forget this trick.
>>
>>>
>>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>>> +	msg.msg.outsize = sizeof(msg.data);
>>>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>>>> +	msg.data.address = logical_addr;
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error setting CEC logical address on EC: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>>>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_write data;
>>>> +	} __packed msg;
>>>
>>> ... msg = {};
>>>
>>>> +	int ret = 0;
>>>> +
>>>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>>>> +		return -EINVAL;
>>>
>>> No need, can never happen.
>>
>> Forgor to remove it, the API changed in the meantime, now it can never happen !
>>
>>>
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>
>>> and drop this.
>>>
>>>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>>>> +	msg.msg.outsize = cec_msg->len;
>>>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error writting CEC msg on EC: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_set data;
>>>> +	} __packed msg;
>>>> +	int ret = 0;
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>>> +	msg.msg.outsize = sizeof(msg.data);
>>>> +	msg.data.cmd = CEC_CMD_ENABLE;
>>>> +	msg.data.enable = enable;
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error %sabling CEC on EC: %d\n",
>>>> +			(enable ? "en" : "dis"), ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>>>> +	.adap_enable = cros_ec_cec_adap_enable,
>>>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>>>> +	.adap_transmit = cros_ec_cec_transmit,
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int cros_ec_cec_suspend(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +	if (device_may_wakeup(dev))
>>>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_resume(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +	if (device_may_wakeup(dev))
>>>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>>>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>>>> +
>>>> +
>>>> +struct cec_dmi_match {
>>>> +	char *sys_vendor;
>>>> +	char *product_name;
>>>> +	char *devname;
>>>> +	char *conn;
>>>> +};
>>>> +
>>>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>>>> +	/* Google Fizz */
>>>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>>>> +};
>>>> +
>>>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>>>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>>>> +
>>>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>>>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>>>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
>>>
>>> See my comments for patch 2/5.
>>
>> I will change with you comment, looks way cleaner.
>>
>>>
>>>> +			return 0;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>>>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>>>> +	struct cros_ec_cec *cros_ec_cec;
>>>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
>>>
>>> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
>>> signal?
>>
>> No, Yes, the EC FW is independent of the HPD status
>>
>>>
>>>> +	int ret;
>>>> +
>>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>>> +				   GFP_KERNEL);
>>>> +	if (!cros_ec_cec)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>>> +	cros_ec_cec->cros_ec = cros_ec;
>>>> +
>>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>>> +	if (ret) {
>>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>>
>>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>>> using this capability unless there is no other option. It's a pain for userspace.
>>
>> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
>> it won't fail but still enable the CEC interface without a notifier.
> 
> I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
> where it is truly impossible to tell which output is connected to the CEC adapter.
> That's the case with e.g. USB CEC dongles where you have no idea how the user
> connected the HDMI cables.
> 
> But I assume that in this case it just means that the cec_dmi_match_table needs
> to be updated, i.e. it is a driver bug.

Yep, maybe a dev_warn should be added to notify this bug ?

> 
> Another thing: this driver assumes that there is only one CEC adapter for only
> one HDMI output. But what if there are more HDMI outputs? Will there be one
> CEC adapter for each output? Or does the hardware have no provisions for that?

The current EC interface only exposes a single CEC interface for now, there is no
plan yet for multiple HDMI outputs handling.

> 
> Something should be mentioned about this in a comment.

Ok

Thanks,
Neil

> 
> Regards,
> 
> 	Hans
> 
>>
>>>
>>>> +	} else if (!cros_ec_cec->notify) {
>>>> +		return -EPROBE_DEFER;
>>>> +	}
>>>> +
>>>> +	ret = device_init_wakeup(&pdev->dev, 1);
>>>> +	if (ret) {
>>>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>>>> +						 DRV_NAME, cec_caps, 1);
>>>> +	if (IS_ERR(cros_ec_cec->adap))
>>>> +		return PTR_ERR(cros_ec_cec->adap);
>>>> +
>>>> +	cros_ec_cec->adap->owner = THIS_MODULE;
>>>
>>> This can be dropped, cec_register_adapter() sets this already.
>>
>> Noted.
>>
>>>
>>>> +
>>>> +	/* Get CEC events from the EC. */
>>>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>>>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>>>> +					       &cros_ec_cec->notifier);
>>>> +	if (ret) {
>>>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>>>> +	if (ret < 0) {
>>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>>>> +	struct device *dev = &pdev->dev;
>>>> +	int ret;
>>>> +
>>>> +	ret = blocking_notifier_chain_unregister(
>>>> +			&cros_ec_cec->cros_ec->event_notifier,
>>>> +			&cros_ec_cec->notifier);
>>>> +
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to unregister notifier\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cec_unregister_adapter(cros_ec_cec->adap);
>>>> +
>>>> +	if (cros_ec_cec->notify)
>>>> +		cec_notifier_put(cros_ec_cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver cros_ec_cec_driver = {
>>>> +	.probe = cros_ec_cec_probe,
>>>> +	.remove  = cros_ec_cec_remove,
>>>> +	.driver = {
>>>> +		.name = DRV_NAME,
>>>> +		.pm = &cros_ec_cec_pm_ops,
>>>> +	},
>>>> +};
>>>> +
>>>> +module_platform_driver(cros_ec_cec_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>>>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_ALIAS("platform:" DRV_NAME);
>>>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>
>> Thanks,
>> Neil
>>
> 

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Neil Armstrong]

On 15/05/2018 10:10, Hans Verkuil wrote:
> On 05/15/18 09:25, Neil Armstrong wrote:
>> Hi Hans,
>>
>> Thanks for the extensive review.
>>
>> On 15/05/2018 08:58, Hans Verkuil wrote:
>>> On 05/15/2018 12:40 AM, Neil Armstrong wrote:
>>>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>>>> driver for such feature of the Embedded Controller.
>>>>
>>>> This driver is part of the cros-ec MFD and will be add as a sub-device when
>>>> the feature bit is exposed by the EC.
>>>>
>>>> The controller will only handle a single logical address and handles
>>>> all the messages retries and will only expose Success or Error.
>>>>
>>>> When the logical address is invalid, the controller will act as a CEC sniffer
>>>> and transfer all messages on the bus.
>>>
>>> I did not see any support for this. If this works as you state here, then adding
>>> support for CEC_CAP_MONITOR_ALL is highly recommended.
>>
>> Oops, I forgot to remove this phrase, the FW will maybe support it, but it has been
>> dropped for the first revision.
>>
>>>
>>>>
>>>> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
>>>> ---
>>>>  drivers/media/platform/Kconfig               |  11 +
>>>>  drivers/media/platform/Makefile              |   2 +
>>>>  drivers/media/platform/cros-ec/Makefile      |   1 +
>>>>  drivers/media/platform/cros-ec/cros-ec-cec.c | 331 +++++++++++++++++++++++++++
>>>>  4 files changed, 345 insertions(+)
>>>>  create mode 100644 drivers/media/platform/cros-ec/Makefile
>>>>  create mode 100644 drivers/media/platform/cros-ec/cros-ec-cec.c
>>>
>>> Shouldn't the directory be called cros-ec-cec?

Forgot this one, moved to cros-ec-cec directory.

>>>
>>>>
>>>> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
>>>> index c7a1cf8..e55a8ed2 100644
>>>> --- a/drivers/media/platform/Kconfig
>>>> +++ b/drivers/media/platform/Kconfig
>>>> @@ -546,6 +546,17 @@ menuconfig CEC_PLATFORM_DRIVERS
>>>>  
>>>>  if CEC_PLATFORM_DRIVERS
>>>>  
>>>> +config VIDEO_CROS_EC_CEC
>>>> +	tristate "Chrome OS EC CEC driver"
>>>> +	depends on MFD_CROS_EC || COMPILE_TEST
>>>> +	select CEC_CORE
>>>> +	select CEC_NOTIFIER
>>>> +	---help---
>>>> +	  If you say yes here you will get support for the
>>>> +	  Chrome OS Embedded Controller's CEC.
>>>> +	  The CEC bus is present in the HDMI connector and enables communication
>>>> +	  between compatible devices.
>>>> +
>>>>  config VIDEO_MESON_AO_CEC
>>>>  	tristate "Amlogic Meson AO CEC driver"
>>>>  	depends on ARCH_MESON || COMPILE_TEST
>>>> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
>>>> index 932515d..0e0582e 100644
>>>> --- a/drivers/media/platform/Makefile
>>>> +++ b/drivers/media/platform/Makefile
>>>> @@ -92,3 +92,5 @@ obj-$(CONFIG_VIDEO_QCOM_CAMSS)		+= qcom/camss-8x16/
>>>>  obj-$(CONFIG_VIDEO_QCOM_VENUS)		+= qcom/venus/
>>>>  
>>>>  obj-y					+= meson/
>>>> +
>>>> +obj-y					+= cros-ec/
>>>> diff --git a/drivers/media/platform/cros-ec/Makefile b/drivers/media/platform/cros-ec/Makefile
>>>> new file mode 100644
>>>> index 0000000..9ce97f9
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/cros-ec/Makefile
>>>> @@ -0,0 +1 @@
>>>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>>>> diff --git a/drivers/media/platform/cros-ec/cros-ec-cec.c b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>>> new file mode 100644
>>>> index 0000000..fea90da
>>>> --- /dev/null
>>>> +++ b/drivers/media/platform/cros-ec/cros-ec-cec.c
>>>> @@ -0,0 +1,331 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * CEC driver for Chrome OS Embedded Controller
>>>> + *
>>>> + * Copyright (c) 2018 BayLibre, SAS
>>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>>> + */
>>>> +
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/dmi.h>
>>>> +#include <linux/cec.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <media/cec.h>
>>>> +#include <media/cec-notifier.h>
>>>> +#include <linux/mfd/cros_ec.h>
>>>> +#include <linux/mfd/cros_ec_commands.h>
>>>> +
>>>> +#define DRV_NAME	"cros-ec-cec"
>>>> +
>>>> +/**
>>>> + * struct cros_ec_cec - Driver data for EC CEC
>>>> + *
>>>> + * @cros_ec: Pointer to EC device
>>>> + * @notifier: Notifier info for responding to EC events
>>>> + * @adap: CEC adapter
>>>> + * @notify: CEC notifier pointer
>>>> + * @rc_msg: storage for a received message
>>>> + */
>>>> +struct cros_ec_cec {
>>>> +	struct cros_ec_device *cros_ec;
>>>> +	struct notifier_block notifier;
>>>> +	struct cec_adapter *adap;
>>>> +	struct cec_notifier *notify;
>>>> +	struct cec_msg rx_msg;
>>>> +};
>>>> +
>>>> +static void handle_cec_message(struct cros_ec_cec *cros_ec_cec)
>>>> +{
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	uint8_t *cec_message = cros_ec->event_data.data.cec_message;
>>>> +	unsigned int len = cros_ec->event_size;
>>>> +
>>>> +	cros_ec_cec->rx_msg.len = len;
>>>
>>> How robust is the underlying code and hardware? What happens if a
>>> CEC message with more than 16 bytes is received?
>>>
>>> Hard to test unless you have an RPi3 set up as a CEC debugger. See
>>> last section in https://hverkuil.home.xs4all.nl/cec-status.txt.
>>>
>>> Since you worked with CEC for a while now I recommend you set up such
>>> a system. It's cheap and very useful.
>>
>> I will definitely setup this kind of system, I tried using am Amlogic SoC, but it really can't monitor the bus....
>>
>> But I also have an ST STM32F0 Development board with double HDMI connector to develop CEC, I'll maybe try to use it for that purpose.
> 
> You really want a setup that uses the cec-gpio driver since that allows
> you to do CEC error injection. If you can hook up the CEC pin to a GPIO
> on the stm32 board, than that should work just as well as with the RPi3.
> 
>>
>>>
>>>> +	memcpy(cros_ec_cec->rx_msg.msg, cec_message, len);
>>>> +
>>>> +	cec_received_msg(cros_ec_cec->adap, &cros_ec_cec->rx_msg);
>>>> +}
>>>> +
>>>> +static void handle_cec_event(struct cros_ec_cec *cros_ec_cec)
>>>> +{
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	uint32_t events = cros_ec->event_data.data.cec_events;
>>>> +
>>>> +	if (events & EC_MKBP_CEC_SEND_OK)
>>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>>> +					  CEC_TX_STATUS_OK);
>>>> +
>>>> +	if (events & EC_MKBP_CEC_SEND_FAILED)
>>>> +		cec_transmit_attempt_done(cros_ec_cec->adap,
>>>> +					  CEC_TX_STATUS_ERROR);
>>>
>>> You said above that the HW takes care of all the message retries.
>>> If that's the case, then you need to use cec_transmit_done here
>>> and pass in status CEC_TX_STATUS_MAX_RETRIES | CEC_TX_STATUS_NACK.
>>> The MAX_RETRIES flag tells the cec core that it shouldn't attempt
>>> to retry the message because the HW already did that.
>>>
>>> NACK is better than ERROR if the hardware supports only an OK/FAIL
>>> result.
>>
>> Ok, we were unsure about this, thanks for the clarification.
>>
>>>
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_event(struct notifier_block *nb,
>>>> +	unsigned long queued_during_suspend, void *_notify)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec;
>>>> +	struct cros_ec_device *cros_ec;
>>>> +
>>>> +	cros_ec_cec = container_of(nb, struct cros_ec_cec, notifier);
>>>> +	cros_ec = cros_ec_cec->cros_ec;
>>>> +
>>>> +	if (cros_ec->event_data.event_type == EC_MKBP_CEC_EVENT) {
>>>> +		handle_cec_event(cros_ec_cec);
>>>> +		return NOTIFY_OK;
>>>> +	}
>>>> +
>>>> +	if (cros_ec->event_data.event_type == EC_MKBP_EVENT_CEC_MESSAGE) {
>>>> +		handle_cec_message(cros_ec_cec);
>>>> +		return NOTIFY_OK;
>>>> +	}
>>>> +
>>>> +	return NOTIFY_DONE;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_set_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_set data;
>>>> +	} __packed msg;
>>>
>>> Just say: ... msg = {};
>>>
>>>> +	int ret = 0;
>>>> +
>>>> +	if (logical_addr == CEC_LOG_ADDR_INVALID)
>>>> +		return 0;
>>>
>>> This looks weird. If I had configured a LA before, then this should unconfigure
>>> it. But it just keeps the existing LA, so any poll messages to us will still
>>> be Acked. Or am I missing something?
> 
> Did you miss this comment? It's weird what is happening here.

I forgot to answer, there was a bug in the firmware, now it's fixed and we can unconfigure the LA with CEC_LOG_ADDR_INVALID.

> 
>>>
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>
>>> and you can drop this memset.
>>
>> I always forget this trick.
>>
>>>
>>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>>> +	msg.msg.outsize = sizeof(msg.data);
>>>> +	msg.data.cmd = CEC_CMD_LOGICAL_ADDRESS;
>>>> +	msg.data.address = logical_addr;
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error setting CEC logical address on EC: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_transmit(struct cec_adapter *adap, u8 attempts,
>>>> +				u32 signal_free_time, struct cec_msg *cec_msg)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_write data;
>>>> +	} __packed msg;
>>>
>>> ... msg = {};
>>>
>>>> +	int ret = 0;
>>>> +
>>>> +	if (cec_msg->len > MAX_CEC_MSG_LEN)
>>>> +		return -EINVAL;
>>>
>>> No need, can never happen.
>>
>> Forgor to remove it, the API changed in the meantime, now it can never happen !
>>
>>>
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>
>>> and drop this.
>>>
>>>> +	msg.msg.command = EC_CMD_CEC_WRITE_MSG;
>>>> +	msg.msg.outsize = cec_msg->len;
>>>> +	memcpy(msg.data.msg, cec_msg->msg, cec_msg->len);
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error writting CEC msg on EC: %d\n", ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_adap_enable(struct cec_adapter *adap, bool enable)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = adap->priv;
>>>> +	struct cros_ec_device *cros_ec = cros_ec_cec->cros_ec;
>>>> +	struct {
>>>> +		struct cros_ec_command msg;
>>>> +		struct ec_params_cec_set data;
>>>> +	} __packed msg;
>>>> +	int ret = 0;
>>>> +
>>>> +	memset(&msg, 0, sizeof(msg));
>>>> +	msg.msg.command = EC_CMD_CEC_SET;
>>>> +	msg.msg.outsize = sizeof(msg.data);
>>>> +	msg.data.cmd = CEC_CMD_ENABLE;
>>>> +	msg.data.enable = enable;
>>>> +
>>>> +	ret = cros_ec_cmd_xfer_status(cros_ec, &msg.msg);
>>>> +	if (ret < 0) {
>>>> +		dev_err(cros_ec->dev,
>>>> +			"error %sabling CEC on EC: %d\n",
>>>> +			(enable ? "en" : "dis"), ret);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct cec_adap_ops cros_ec_cec_ops = {
>>>> +	.adap_enable = cros_ec_cec_adap_enable,
>>>> +	.adap_log_addr = cros_ec_cec_set_log_addr,
>>>> +	.adap_transmit = cros_ec_cec_transmit,
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int cros_ec_cec_suspend(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +	if (device_may_wakeup(dev))
>>>> +		enable_irq_wake(cros_ec_cec->cros_ec->irq);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_resume(struct device *dev)
>>>> +{
>>>> +	struct platform_device *pdev = to_platform_device(dev);
>>>> +	struct cros_ec_cec *cros_ec_cec = dev_get_drvdata(&pdev->dev);
>>>> +
>>>> +	if (device_may_wakeup(dev))
>>>> +		disable_irq_wake(cros_ec_cec->cros_ec->irq);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(cros_ec_cec_pm_ops,
>>>> +	cros_ec_cec_suspend, cros_ec_cec_resume);
>>>> +
>>>> +
>>>> +struct cec_dmi_match {
>>>> +	char *sys_vendor;
>>>> +	char *product_name;
>>>> +	char *devname;
>>>> +	char *conn;
>>>> +};
>>>> +
>>>> +static const struct cec_dmi_match cec_dmi_match_table[] = {
>>>> +	/* Google Fizz */
>>>> +	{ "Google", "Fizz", "0000:00:02.0", "HDMI-A-1" },
>>>> +};
>>>> +
>>>> +static int cros_ec_cec_get_notifier(struct cec_notifier **notify)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	for (i = 0 ; i < ARRAY_SIZE(cec_dmi_match_table) ; ++i) {
>>>> +		const struct cec_dmi_match *m = &cec_dmi_match_table[i];
>>>> +
>>>> +		if (dmi_match(DMI_SYS_VENDOR, m->sys_vendor) &&
>>>> +		    dmi_match(DMI_PRODUCT_NAME, m->product_name)) {
>>>> +			*notify = cec_notifier_get_byname(m->devname, m->conn);
>>>
>>> See my comments for patch 2/5.
>>
>> I will change with you comment, looks way cleaner.
>>
>>>
>>>> +			return 0;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return -EINVAL;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct cros_ec_dev *ec_dev = dev_get_drvdata(pdev->dev.parent);
>>>> +	struct cros_ec_device *cros_ec = ec_dev->ec_dev;
>>>> +	struct cros_ec_cec *cros_ec_cec;
>>>> +	unsigned int cec_caps = CEC_CAP_DEFAULTS;
>>>
>>> Does this need CEC_CAP_NEEDS_HPD? Can it still use CEC if there is no HPD
>>> signal?
>>
>> No, Yes, the EC FW is independent of the HPD status
>>
>>>
>>>> +	int ret;
>>>> +
>>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>>> +				   GFP_KERNEL);
>>>> +	if (!cros_ec_cec)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>>> +	cros_ec_cec->cros_ec = cros_ec;
>>>> +
>>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>>> +	if (ret) {
>>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>>
>>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>>> using this capability unless there is no other option. It's a pain for userspace.
>>
>> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
>> it won't fail but still enable the CEC interface without a notifier.
> 
> I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
> where it is truly impossible to tell which output is connected to the CEC adapter.
> That's the case with e.g. USB CEC dongles where you have no idea how the user
> connected the HDMI cables.
> 
> But I assume that in this case it just means that the cec_dmi_match_table needs
> to be updated, i.e. it is a driver bug.

Yep, maybe a dev_warn should be added to notify this bug ?

> 
> Another thing: this driver assumes that there is only one CEC adapter for only
> one HDMI output. But what if there are more HDMI outputs? Will there be one
> CEC adapter for each output? Or does the hardware have no provisions for that?

The current EC interface only exposes a single CEC interface for now, there is no
plan yet for multiple HDMI outputs handling.

> 
> Something should be mentioned about this in a comment.

Ok

Thanks,
Neil

> 
> Regards,
> 
> 	Hans
> 
>>
>>>
>>>> +	} else if (!cros_ec_cec->notify) {
>>>> +		return -EPROBE_DEFER;
>>>> +	}
>>>> +
>>>> +	ret = device_init_wakeup(&pdev->dev, 1);
>>>> +	if (ret) {
>>>> +		dev_err(&pdev->dev, "failed to initialize wakeup\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cros_ec_cec->adap = cec_allocate_adapter(&cros_ec_cec_ops, cros_ec_cec,
>>>> +						 DRV_NAME, cec_caps, 1);
>>>> +	if (IS_ERR(cros_ec_cec->adap))
>>>> +		return PTR_ERR(cros_ec_cec->adap);
>>>> +
>>>> +	cros_ec_cec->adap->owner = THIS_MODULE;
>>>
>>> This can be dropped, cec_register_adapter() sets this already.
>>
>> Noted.
>>
>>>
>>>> +
>>>> +	/* Get CEC events from the EC. */
>>>> +	cros_ec_cec->notifier.notifier_call = cros_ec_cec_event;
>>>> +	ret = blocking_notifier_chain_register(&cros_ec->event_notifier,
>>>> +					       &cros_ec_cec->notifier);
>>>> +	if (ret) {
>>>> +		dev_err(&pdev->dev, "failed to register notifier\n");
>>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	ret = cec_register_adapter(cros_ec_cec->adap, &pdev->dev);
>>>> +	if (ret < 0) {
>>>> +		cec_delete_adapter(cros_ec_cec->adap);
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cec_register_cec_notifier(cros_ec_cec->adap, cros_ec_cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int cros_ec_cec_remove(struct platform_device *pdev)
>>>> +{
>>>> +	struct cros_ec_cec *cros_ec_cec = platform_get_drvdata(pdev);
>>>> +	struct device *dev = &pdev->dev;
>>>> +	int ret;
>>>> +
>>>> +	ret = blocking_notifier_chain_unregister(
>>>> +			&cros_ec_cec->cros_ec->event_notifier,
>>>> +			&cros_ec_cec->notifier);
>>>> +
>>>> +	if (ret) {
>>>> +		dev_err(dev, "failed to unregister notifier\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	cec_unregister_adapter(cros_ec_cec->adap);
>>>> +
>>>> +	if (cros_ec_cec->notify)
>>>> +		cec_notifier_put(cros_ec_cec->notify);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static struct platform_driver cros_ec_cec_driver = {
>>>> +	.probe = cros_ec_cec_probe,
>>>> +	.remove  = cros_ec_cec_remove,
>>>> +	.driver = {
>>>> +		.name = DRV_NAME,
>>>> +		.pm = &cros_ec_cec_pm_ops,
>>>> +	},
>>>> +};
>>>> +
>>>> +module_platform_driver(cros_ec_cec_driver);
>>>> +
>>>> +MODULE_DESCRIPTION("CEC driver for Chrome OS ECs");
>>>> +MODULE_AUTHOR("Neil Armstrong <narmstrong@baylibre.com>");
>>>> +MODULE_LICENSE("GPL");
>>>> +MODULE_ALIAS("platform:" DRV_NAME);
>>>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>
>> Thanks,
>> Neil
>>
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/18 10:28, Neil Armstrong wrote:
>>>>> +	int ret;
>>>>> +
>>>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>>>> +				   GFP_KERNEL);
>>>>> +	if (!cros_ec_cec)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>>>> +	cros_ec_cec->cros_ec = cros_ec;
>>>>> +
>>>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>>>> +	if (ret) {
>>>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>>>
>>>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>>>> using this capability unless there is no other option. It's a pain for userspace.
>>>
>>> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
>>> it won't fail but still enable the CEC interface without a notifier.
>>
>> I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
>> where it is truly impossible to tell which output is connected to the CEC adapter.
>> That's the case with e.g. USB CEC dongles where you have no idea how the user
>> connected the HDMI cables.
>>
>> But I assume that in this case it just means that the cec_dmi_match_table needs
>> to be updated, i.e. it is a driver bug.
> 
> Yep, maybe a dev_warn should be added to notify this bug ?

Yes, a dev_warn and then return -ENODEV.

> 
>>
>> Another thing: this driver assumes that there is only one CEC adapter for only
>> one HDMI output. But what if there are more HDMI outputs? Will there be one
>> CEC adapter for each output? Or does the hardware have no provisions for that?
> 
> The current EC interface only exposes a single CEC interface for now, there is no
> plan yet for multiple HDMI outputs handling.
> 
>>
>> Something should be mentioned about this in a comment.
> 
> Ok

Thanks!

	Hans

Re: [RFC PATCH 5/5] media: platform: Add Chrome OS EC CEC driver

[Hans Verkuil]

On 05/15/18 10:28, Neil Armstrong wrote:
>>>>> +	int ret;
>>>>> +
>>>>> +	cros_ec_cec = devm_kzalloc(&pdev->dev, sizeof(*cros_ec_cec),
>>>>> +				   GFP_KERNEL);
>>>>> +	if (!cros_ec_cec)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	platform_set_drvdata(pdev, cros_ec_cec);
>>>>> +	cros_ec_cec->cros_ec = cros_ec;
>>>>> +
>>>>> +	ret = cros_ec_cec_get_notifier(&cros_ec_cec->notify);
>>>>> +	if (ret) {
>>>>> +		dev_warn(&pdev->dev, "no CEC notifier available\n");
>>>>> +		cec_caps |= CEC_CAP_PHYS_ADDR;
>>>>
>>>> Can this happen? What hardware has this? I am strongly opposed to CEC drivers
>>>> using this capability unless there is no other option. It's a pain for userspace.
>>>
>>> It's in case an HW having a CEC capable FW but not in the cec_dmi_match_table, in this case
>>> it won't fail but still enable the CEC interface without a notifier.
>>
>> I don't think that's a good idea. CAP_PHYS_ADDR should *only* be used in situations
>> where it is truly impossible to tell which output is connected to the CEC adapter.
>> That's the case with e.g. USB CEC dongles where you have no idea how the user
>> connected the HDMI cables.
>>
>> But I assume that in this case it just means that the cec_dmi_match_table needs
>> to be updated, i.e. it is a driver bug.
> 
> Yep, maybe a dev_warn should be added to notify this bug ?

Yes, a dev_warn and then return -ENODEV.

> 
>>
>> Another thing: this driver assumes that there is only one CEC adapter for only
>> one HDMI output. But what if there are more HDMI outputs? Will there be one
>> CEC adapter for each output? Or does the hardware have no provisions for that?
> 
> The current EC interface only exposes a single CEC interface for now, there is no
> plan yet for multiple HDMI outputs handling.
> 
>>
>> Something should be mentioned about this in a comment.
> 
> Ok

Thanks!

	Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx