2018-05-18 13:05:32

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 0/5] Add ChromeOS EC CEC Support

Hi All,

The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
through 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 !

Changes since v2:
- Add i915 port_identifier() and use this stable name as cec_notifier conn name
- Fixed and cleaned up the CEC commands and events handling
- Rebased the CEC sub-device registration on top of Enric's serie
- Fixed comments typo on cec driver
- Protected the DMI match only with PCI and DMI Kconfigs

Changes since v1:
- Added cec_notifier_put to intel_hdmi
- Fixed all small reported issues on the EC CEC driver
- Moved the cec_notifier_get out of the #if .. #else .. #endif

Changes since RFC:
- Moved CEC sub-device registration after CEC commands and events definitions patch
- Removed get_notifier_get_byname
- Added CEC_CORE select into i915 Kconfig
- Removed CEC driver fallback if notifier is not configured on HW, added explicit warn
- Fixed CEC core return type on error
- Moved to cros-ec-cec media platform directory
- Use bus_find_device() to find the pci i915 device instead of get_notifier_get_byname()
- Fix Logical Address setup
- Added comment about HW support
- Removed memset of msg structures

Neil Armstrong (5):
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.
mfd: cros_ec_dev: Add CEC sub-device registration
media: platform: Add Chrome OS EC CEC driver

drivers/gpu/drm/i915/Kconfig | 1 +
drivers/gpu/drm/i915/intel_display.h | 20 ++
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_hdmi.c | 13 +
drivers/media/cec/cec-notifier.c | 11 +-
drivers/media/platform/Kconfig | 11 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/cros-ec-cec/Makefile | 1 +
drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
drivers/mfd/cros_ec_dev.c | 16 ++
drivers/platform/chrome/cros_ec_proto.c | 40 ++-
include/linux/mfd/cros_ec.h | 2 +-
include/linux/mfd/cros_ec_commands.h | 80 ++++++
include/media/cec-notifier.h | 27 +-
14 files changed, 557 insertions(+), 16 deletions(-)
create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
create mode 100644 drivers/media/platform/cros-ec-cec/cros-ec-cec.c

--
2.7.4



2018-05-18 13:06:25

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 5/5] media: platform: Add Chrome OS EC CEC driver

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.

The controller will be tied to the HDMI CEC notifier by using the platform
DMI Data and the i915 device name and connector name.

Signed-off-by: Neil Armstrong <[email protected]>
---
drivers/media/platform/Kconfig | 11 +
drivers/media/platform/Makefile | 2 +
drivers/media/platform/cros-ec-cec/Makefile | 1 +
drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
4 files changed, 361 insertions(+)
create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
create mode 100644 drivers/media/platform/cros-ec-cec/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..830696f 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-cec/
diff --git a/drivers/media/platform/cros-ec-cec/Makefile b/drivers/media/platform/cros-ec-cec/Makefile
new file mode 100644
index 0000000..9ce97f9
--- /dev/null
+++ b/drivers/media/platform/cros-ec-cec/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
new file mode 100644
index 0000000..7e1e275
--- /dev/null
+++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * CEC driver for Chrome OS Embedded Controller
+ *
+ * Copyright (c) 2018 BayLibre, SAS
+ * Author: Neil Armstrong <[email protected]>
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/dmi.h>
+#include <linux/pci.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
+ * @rx_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);
+
+ /* FW takes care of all retries, tell core to avoid more retries */
+ if (events & EC_MKBP_CEC_SEND_FAILED)
+ cec_transmit_attempt_done(cros_ec_cec->adap,
+ CEC_TX_STATUS_MAX_RETRIES |
+ CEC_TX_STATUS_NACK);
+}
+
+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;
+
+ 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;
+
+ 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 writing 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;
+
+ 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);
+
+#if IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_DMI)
+
+/*
+ * The Firmware only handles a single CEC interface tied to a single HDMI
+ * connector we specify along with the DRM device name handling the HDMI output
+ */
+
+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", "Port B" },
+};
+
+static int cros_ec_cec_get_notifier(struct device *dev,
+ 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)) {
+ struct device *d;
+
+ /* Find the device, bail out if not yet registered */
+ d = bus_find_device_by_name(&pci_bus_type, NULL,
+ m->devname);
+ if (!d)
+ return -EPROBE_DEFER;
+
+ *notify = cec_notifier_get_conn(d, m->conn);
+ return 0;
+ }
+ }
+
+ /* Hardware support must be added in the cec_dmi_match_table */
+ dev_warn(dev, "CEC notifier not configured for this hardware\n");
+
+ return -ENODEV;
+}
+
+#else
+
+static int cros_ec_cec_get_notifier(struct device *dev,
+ struct cec_notifier **notify)
+{
+ return -ENODEV;
+}
+
+#endif
+
+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;
+ 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(&pdev->dev, &cros_ec_cec->notify);
+ if (ret)
+ return ret;
+
+ 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_CAP_DEFAULTS, 1);
+ if (IS_ERR(cros_ec_cec->adap))
+ return PTR_ERR(cros_ec_cec->adap);
+
+ /* 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 <[email protected]>");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
--
2.7.4


2018-05-18 13:07:19

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 2/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

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 <[email protected]>
---
drivers/gpu/drm/i915/Kconfig | 1 +
drivers/gpu/drm/i915/intel_display.h | 20 ++++++++++++++++++++
drivers/gpu/drm/i915/intel_drv.h | 2 ++
drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++
4 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index dfd9588..2d65d56 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -23,6 +23,7 @@ config DRM_I915
select SYNC_FILE
select IOSF_MBI
select CRC32
+ select CEC_CORE if CEC_NOTIFIER
help
Choose this option if you have a system that has "Intel Graphics
Media Accelerator" or "HD Graphics" integrated graphics,
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 4e7418b..c68d1c8 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -126,6 +126,26 @@ enum port {

#define port_name(p) ((p) + 'A')

+static inline const char *port_identifier(enum port port)
+{
+ switch (port) {
+ case PORT_A:
+ return "Port A";
+ case PORT_B:
+ return "Port B";
+ case PORT_C:
+ return "Port C";
+ case PORT_D:
+ return "Port D";
+ case PORT_E:
+ return "Port E";
+ case PORT_F:
+ return "Port F";
+ default:
+ return "<invalid>";
+ }
+}
+
enum dpio_channel {
DPIO_CH0,
DPIO_CH1
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..d522b5b 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;
}

@@ -2031,6 +2037,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder,

static void intel_hdmi_destroy(struct drm_connector *connector)
{
+ if (intel_attached_hdmi(connector)->notifier)
+ cec_notifier_put(intel_attached_hdmi(connector)->notifier);
kfree(to_intel_connector(connector)->detect_edid);
drm_connector_cleanup(connector);
kfree(connector);
@@ -2358,6 +2366,11 @@ 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,
+ port_identifier(port));
+ 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


2018-05-18 13:07:37

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

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 <[email protected]>
Signed-off-by: Neil Armstrong <[email protected]>
---
drivers/platform/chrome/cros_ec_proto.c | 40 +++++++++++++----
include/linux/mfd/cros_ec.h | 2 +-
include/linux/mfd/cros_ec_commands.h | 80 +++++++++++++++++++++++++++++++++
3 files changed, 112 insertions(+), 10 deletions(-)

diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index e7bbdf9..c4f6c44 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -504,10 +504,31 @@ 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, ec_dev->event_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;

if (ec_dev->suspended) {
@@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
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(struct ec_response_get_next_event_v1));
+ if (ret < 0 || msg->result != 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 f36125e..32caef3 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..16c3a2b 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


2018-05-18 13:08:41

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

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 <[email protected]>
---
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 1d6dc5c..272969e 100644
--- a/drivers/mfd/cros_ec_dev.c
+++ b/drivers/mfd/cros_ec_dev.c
@@ -383,6 +383,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
kfree(msg);
}

+static const struct mfd_cell cros_ec_cec_cells[] = {
+ { .name = "cros-ec-cec" }
+};
+
static const struct mfd_cell cros_ec_rtc_cells[] = {
{ .name = "cros-ec-rtc" }
};
@@ -426,6 +430,18 @@ 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 instance has CEC host command support */
+ if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
+ retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
+ cros_ec_cec_cells,
+ ARRAY_SIZE(cros_ec_cec_cells),
+ NULL, 0, NULL);
+ if (retval)
+ dev_err(ec->dev,
+ "failed to add cros-ec-cec device: %d\n",
+ retval);
+ }
+
/* Check whether this EC instance has RTC host command support */
if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
--
2.7.4


2018-05-18 13:09:52

by Neil Armstrong

[permalink] [raw]
Subject: [PATCH v2 1/5] media: cec-notifier: Get notifier by device and connector name

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 <[email protected]>
---
drivers/media/cec/cec-notifier.c | 11 ++++++++---
include/media/cec-notifier.h | 27 ++++++++++++++++++++++++---
2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
index 16dffa0..dd2078b 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,14 @@ 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_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) {
+ if (n->dev == dev &&
+ (!conn || !strcmp(n->conn, conn))) {
kref_get(&n->kref);
mutex_unlock(&cec_notifiers_lock);
return n;
@@ -46,6 +48,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
if (!n)
goto unlock;
n->dev = dev;
+ if (conn)
+ n->conn = kstrdup(conn, GFP_KERNEL);
n->phys_addr = CEC_PHYS_ADDR_INVALID;
mutex_init(&n->lock);
kref_init(&n->kref);
@@ -54,7 +58,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)
{
@@ -62,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
container_of(kref, struct cec_notifier, kref);

list_del(&n->head);
+ kfree(n->conn);
kfree(n);
}

diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
index cf0add7..814eeef 100644
--- a/include/media/cec-notifier.h
+++ b/include/media/cec-notifier.h
@@ -20,8 +20,10 @@ struct cec_notifier;
#if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)

/**
- * cec_notifier_get - find or create a new cec_notifier for the given device.
+ * 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.
@@ -31,7 +33,8 @@ struct cec_notifier;
*
* Return NULL if the memory could not be allocated.
*/
-struct cec_notifier *cec_notifier_get(struct device *dev);
+struct cec_notifier *cec_notifier_get_conn(struct device *dev,
+ const char *conn);

/**
* cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
@@ -85,7 +88,8 @@ 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;
@@ -121,6 +125,23 @@ static inline void cec_register_cec_notifier(struct cec_adapter *adap,
#endif

/**
+ * cec_notifier_get - find or create a new cec_notifier for the given device.
+ * @dev: device that sends the events.
+ *
+ * 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.
+ */
+static inline struct cec_notifier *cec_notifier_get(struct device *dev)
+{
+ return cec_notifier_get_conn(dev, NULL);
+}
+
+/**
* cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
*
* @n: the CEC notifier
--
2.7.4


2018-05-18 13:24:55

by Ville Syrjälä

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

On Fri, May 18, 2018 at 03:05:01PM +0200, 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 <[email protected]>
> ---
> drivers/gpu/drm/i915/Kconfig | 1 +
> drivers/gpu/drm/i915/intel_display.h | 20 ++++++++++++++++++++
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++
> 4 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
> index dfd9588..2d65d56 100644
> --- a/drivers/gpu/drm/i915/Kconfig
> +++ b/drivers/gpu/drm/i915/Kconfig
> @@ -23,6 +23,7 @@ config DRM_I915
> select SYNC_FILE
> select IOSF_MBI
> select CRC32
> + select CEC_CORE if CEC_NOTIFIER
> help
> Choose this option if you have a system that has "Intel Graphics
> Media Accelerator" or "HD Graphics" integrated graphics,
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 4e7418b..c68d1c8 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -126,6 +126,26 @@ enum port {
>
> #define port_name(p) ((p) + 'A')
>
> +static inline const char *port_identifier(enum port port)
> +{
> + switch (port) {
> + case PORT_A:
> + return "Port A";
> + case PORT_B:
> + return "Port B";
> + case PORT_C:
> + return "Port C";
> + case PORT_D:
> + return "Port D";
> + case PORT_E:
> + return "Port E";
> + case PORT_F:
> + return "Port F";
> + default:
> + return "<invalid>";
> + }
> +}

I don't think we need this in the header. A static function will do.

And I was actually thinking something a bit fancier like:
snprintf("%s/port-%s", dev_name(dev), port_id(port));

Oh, I see you already pass the device in so I guess we don't need
that in the port id?

> +
> enum dpio_channel {
> DPIO_CH0,
> DPIO_CH1
> 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;

I was wondering if we need any ifdefs around this stuff, but I guess not
since it's just a pointer and all the functions seem to have empty
static inlines for the CEC=n case.

> };
>
> 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..d522b5b 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;
> }
>
> @@ -2031,6 +2037,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder,
>
> static void intel_hdmi_destroy(struct drm_connector *connector)
> {
> + if (intel_attached_hdmi(connector)->notifier)
> + cec_notifier_put(intel_attached_hdmi(connector)->notifier);
> kfree(to_intel_connector(connector)->detect_edid);
> drm_connector_cleanup(connector);
> kfree(connector);
> @@ -2358,6 +2366,11 @@ 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,
> + port_identifier(port));
> + 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrj?l?
Intel

2018-05-18 13:41:41

by Enric Balletbo Serra

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] mfd: cros_ec_dev: Add CEC sub-device registration

2018-05-18 15:05 GMT+02:00 Neil Armstrong <[email protected]>:
> 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 <[email protected]>
> ---
> 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 1d6dc5c..272969e 100644
> --- a/drivers/mfd/cros_ec_dev.c
> +++ b/drivers/mfd/cros_ec_dev.c
> @@ -383,6 +383,10 @@ static void cros_ec_sensors_register(struct cros_ec_dev *ec)
> kfree(msg);
> }
>
> +static const struct mfd_cell cros_ec_cec_cells[] = {
> + { .name = "cros-ec-cec" }
> +};
> +
> static const struct mfd_cell cros_ec_rtc_cells[] = {
> { .name = "cros-ec-rtc" }
> };
> @@ -426,6 +430,18 @@ 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 instance has CEC host command support */
> + if (cros_ec_check_features(ec, EC_FEATURE_CEC)) {
> + retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> + cros_ec_cec_cells,
> + ARRAY_SIZE(cros_ec_cec_cells),
> + NULL, 0, NULL);
> + if (retval)
> + dev_err(ec->dev,
> + "failed to add cros-ec-cec device: %d\n",
> + retval);
> + }
> +
> /* Check whether this EC instance has RTC host command support */
> if (cros_ec_check_features(ec, EC_FEATURE_RTC)) {
> retval = mfd_add_devices(ec->dev, PLATFORM_DEVID_AUTO,
> --
> 2.7.4
>

Reviewed-by: Enric Balletbo i Serra <[email protected]>

Thanks,
Enric

2018-05-18 14:04:58

by Enric Balletbo Serra

[permalink] [raw]
Subject: Re: [PATCH v2 0/5] Add ChromeOS EC CEC Support

Hi Neil,

2018-05-18 15:04 GMT+02:00 Neil Armstrong <[email protected]>:
> Hi All,
>
> The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
> through 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 !
>
> Changes since v2:
> - Add i915 port_identifier() and use this stable name as cec_notifier conn name
> - Fixed and cleaned up the CEC commands and events handling
> - Rebased the CEC sub-device registration on top of Enric's serie
> - Fixed comments typo on cec driver
> - Protected the DMI match only with PCI and DMI Kconfigs
>

Just because I got confused when I saw two v2 in my inbox. This is v3, right?

> Changes since v1:
> - Added cec_notifier_put to intel_hdmi
> - Fixed all small reported issues on the EC CEC driver
> - Moved the cec_notifier_get out of the #if .. #else .. #endif
>
> Changes since RFC:
> - Moved CEC sub-device registration after CEC commands and events definitions patch
> - Removed get_notifier_get_byname
> - Added CEC_CORE select into i915 Kconfig
> - Removed CEC driver fallback if notifier is not configured on HW, added explicit warn
> - Fixed CEC core return type on error
> - Moved to cros-ec-cec media platform directory
> - Use bus_find_device() to find the pci i915 device instead of get_notifier_get_byname()
> - Fix Logical Address setup
> - Added comment about HW support
> - Removed memset of msg structures
>
> Neil Armstrong (5):
> 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.
> mfd: cros_ec_dev: Add CEC sub-device registration
> media: platform: Add Chrome OS EC CEC driver
>
> drivers/gpu/drm/i915/Kconfig | 1 +
> drivers/gpu/drm/i915/intel_display.h | 20 ++
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> drivers/gpu/drm/i915/intel_hdmi.c | 13 +
> drivers/media/cec/cec-notifier.c | 11 +-
> drivers/media/platform/Kconfig | 11 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/cros-ec-cec/Makefile | 1 +
> drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
> drivers/mfd/cros_ec_dev.c | 16 ++
> drivers/platform/chrome/cros_ec_proto.c | 40 ++-
> include/linux/mfd/cros_ec.h | 2 +-
> include/linux/mfd/cros_ec_commands.h | 80 ++++++
> include/media/cec-notifier.h | 27 +-
> 14 files changed, 557 insertions(+), 16 deletions(-)
> create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
> create mode 100644 drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>
> --
> 2.7.4
>

2018-05-18 15:03:09

by Enric Balletbo Serra

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] media: platform: Add Chrome OS EC CEC driver

Hi Neil,

2018-05-18 15:05 GMT+02:00 Neil Armstrong <[email protected]>:
> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the

A minor nit, there is a "consensus" on tell cros-ec as "ChromeOS
Embedded Controller" or "ChromeOS EC". Yes, I know that you can see in
the kernel many other ways to refer to the ChromeOS EC, but to
standardize a little bit, could you replace all occurrences s/Chrome
OS/ChromeOS/. Thanks.

> 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.
>
> The controller will be tied to the HDMI CEC notifier by using the platform
> DMI Data and the i915 device name and connector name.
>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> drivers/media/platform/Kconfig | 11 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/cros-ec-cec/Makefile | 1 +
> drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
> 4 files changed, 361 insertions(+)
> create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
> create mode 100644 drivers/media/platform/cros-ec-cec/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"

here

> + 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.

here

> + 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..830696f 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-cec/
> diff --git a/drivers/media/platform/cros-ec-cec/Makefile b/drivers/media/platform/cros-ec-cec/Makefile
> new file mode 100644
> index 0000000..9ce97f9
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec-cec/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
> diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> new file mode 100644
> index 0000000..7e1e275
> --- /dev/null
> +++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * CEC driver for Chrome OS Embedded Controller

and here

> + *
> + * Copyright (c) 2018 BayLibre, SAS
> + * Author: Neil Armstrong <[email protected]>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/dmi.h>
> +#include <linux/pci.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
> + * @rx_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);
> +
> + /* FW takes care of all retries, tell core to avoid more retries */
> + if (events & EC_MKBP_CEC_SEND_FAILED)
> + cec_transmit_attempt_done(cros_ec_cec->adap,
> + CEC_TX_STATUS_MAX_RETRIES |
> + CEC_TX_STATUS_NACK);
> +}
> +
> +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;
> +
> + 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;
> +
> + 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 writing 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;
> +
> + 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);
> +
> +#if IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_DMI)
> +
> +/*
> + * The Firmware only handles a single CEC interface tied to a single HDMI
> + * connector we specify along with the DRM device name handling the HDMI output
> + */
> +
> +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", "Port B" },
> +};
> +
> +static int cros_ec_cec_get_notifier(struct device *dev,
> + 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)) {
> + struct device *d;
> +
> + /* Find the device, bail out if not yet registered */
> + d = bus_find_device_by_name(&pci_bus_type, NULL,
> + m->devname);
> + if (!d)
> + return -EPROBE_DEFER;
> +
> + *notify = cec_notifier_get_conn(d, m->conn);
> + return 0;
> + }
> + }
> +
> + /* Hardware support must be added in the cec_dmi_match_table */
> + dev_warn(dev, "CEC notifier not configured for this hardware\n");
> +
> + return -ENODEV;
> +}
> +
> +#else
> +
> +static int cros_ec_cec_get_notifier(struct device *dev,
> + struct cec_notifier **notify)
> +{
> + return -ENODEV;
> +}
> +
> +#endif
> +
> +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;
> + 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(&pdev->dev, &cros_ec_cec->notify);
> + if (ret)
> + return ret;
> +
> + 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_CAP_DEFAULTS, 1);
> + if (IS_ERR(cros_ec_cec->adap))
> + return PTR_ERR(cros_ec_cec->adap);
> +
> + /* 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 <[email protected]>");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" DRV_NAME);
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

The patch looks good to me, so

Reviewed-by: Enric Balletbo i Serra <[email protected]>

2018-05-18 15:48:57

by Sean Paul

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] media: cec-notifier: Get notifier by device and connector name

On Fri, May 18, 2018 at 03:05:00PM +0200, 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 <[email protected]>

Hi Neil,
Thanks for posting these!

> ---
> drivers/media/cec/cec-notifier.c | 11 ++++++++---
> include/media/cec-notifier.h | 27 ++++++++++++++++++++++++---
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
> index 16dffa0..dd2078b 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,14 @@ 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_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) {
> + if (n->dev == dev &&
> + (!conn || !strcmp(n->conn, conn))) {
> kref_get(&n->kref);
> mutex_unlock(&cec_notifiers_lock);
> return n;
> @@ -46,6 +48,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
> if (!n)
> goto unlock;
> n->dev = dev;
> + if (conn)
> + n->conn = kstrdup(conn, GFP_KERNEL);
> n->phys_addr = CEC_PHYS_ADDR_INVALID;
> mutex_init(&n->lock);
> kref_init(&n->kref);
> @@ -54,7 +58,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)
> {
> @@ -62,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
> container_of(kref, struct cec_notifier, kref);
>
> list_del(&n->head);
> + kfree(n->conn);
> kfree(n);
> }
>
> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
> index cf0add7..814eeef 100644
> --- a/include/media/cec-notifier.h
> +++ b/include/media/cec-notifier.h
> @@ -20,8 +20,10 @@ struct cec_notifier;
> #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>
> /**
> - * cec_notifier_get - find or create a new cec_notifier for the given device.
> + * 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

Probably best to use "name" instead of connector, since it doesn't necessarily
_have_ to be a connector name. So, cec_notifier_get_by_name(dev, name)

> *
> * If a notifier for device @dev already exists, then increase the refcount
> * and return that notifier.
> @@ -31,7 +33,8 @@ struct cec_notifier;
> *
> * Return NULL if the memory could not be allocated.
> */
> -struct cec_notifier *cec_notifier_get(struct device *dev);
> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
> + const char *conn);
>
> /**
> * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
> @@ -85,7 +88,8 @@ 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;
> @@ -121,6 +125,23 @@ static inline void cec_register_cec_notifier(struct cec_adapter *adap,
> #endif
>
> /**
> + * cec_notifier_get - find or create a new cec_notifier for the given device.
> + * @dev: device that sends the events.
> + *
> + * 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.

You might also want to cover the case where you have multiple named notifiers
for the same device. It looks like it just grabs the first one?

Sean

> + *
> + * Return NULL if the memory could not be allocated.
> + */
> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
> +{
> + return cec_notifier_get_conn(dev, NULL);
> +}
> +
> +/**
> * cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
> *
> * @n: the CEC notifier
> --
> 2.7.4
>

--
Sean Paul, Software Engineer, Google / Chromium OS

2018-05-18 16:20:18

by Enric Balletbo Serra

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

Hi Neil,

2018-05-18 15:05 GMT+02:00 Neil Armstrong <[email protected]>:
> 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 <[email protected]>
> Signed-off-by: Neil Armstrong <[email protected]>
> ---
> drivers/platform/chrome/cros_ec_proto.c | 40 +++++++++++++----
> include/linux/mfd/cros_ec.h | 2 +-
> include/linux/mfd/cros_ec_commands.h | 80 +++++++++++++++++++++++++++++++++
> 3 files changed, 112 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf9..c4f6c44 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -504,10 +504,31 @@ 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, ec_dev->event_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;

Personal opinion, but I don't like this static here, and also I don't
think this is scalable. Could we ask for the command version?

> int ret;
>
> if (ec_dev->suspended) {
> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> 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(struct ec_response_get_next_event_v1));
> + if (ret < 0 || msg->result != 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;
> }
>

So we always do a failed transfer on all these EC devices that does
not support CEC. I am wondering if wouldn't be better pass the command
version to the cros_ec_get_next_event function. The driver should know
which version to use, just a random idea.

> + 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 f36125e..32caef3 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..16c3a2b 100644
> --- a/include/linux/mfd/cros_ec_commands.h
> +++ b/include/linux/mfd/cros_ec_commands.h

This file is going to be very big and as requested by Lee I plan to
convert this file to the kernel-doc format, this patch introduces some
new structs so could you document the new structs in the suggested
format?

> @@ -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,

Use the BIT() macro.

> +};
>
> /*****************************************************************************/
> /*
> --
> 2.7.4
>

Thanks,
Enric

2018-05-21 08:53:48

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] media: cec-notifier: Get notifier by device and connector name

Hi Sean,

On 18/05/2018 17:48, Sean Paul wrote:
> On Fri, May 18, 2018 at 03:05:00PM +0200, 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 <[email protected]>
>
> Hi Neil,
> Thanks for posting these!
>
>> ---
>> drivers/media/cec/cec-notifier.c | 11 ++++++++---
>> include/media/cec-notifier.h | 27 ++++++++++++++++++++++++---
>> 2 files changed, 32 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/cec/cec-notifier.c b/drivers/media/cec/cec-notifier.c
>> index 16dffa0..dd2078b 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,14 @@ 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_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) {
>> + if (n->dev == dev &&
>> + (!conn || !strcmp(n->conn, conn))) {
>> kref_get(&n->kref);
>> mutex_unlock(&cec_notifiers_lock);
>> return n;
>> @@ -46,6 +48,8 @@ struct cec_notifier *cec_notifier_get(struct device *dev)
>> if (!n)
>> goto unlock;
>> n->dev = dev;
>> + if (conn)
>> + n->conn = kstrdup(conn, GFP_KERNEL);
>> n->phys_addr = CEC_PHYS_ADDR_INVALID;
>> mutex_init(&n->lock);
>> kref_init(&n->kref);
>> @@ -54,7 +58,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)
>> {
>> @@ -62,6 +66,7 @@ static void cec_notifier_release(struct kref *kref)
>> container_of(kref, struct cec_notifier, kref);
>>
>> list_del(&n->head);
>> + kfree(n->conn);
>> kfree(n);
>> }
>>
>> diff --git a/include/media/cec-notifier.h b/include/media/cec-notifier.h
>> index cf0add7..814eeef 100644
>> --- a/include/media/cec-notifier.h
>> +++ b/include/media/cec-notifier.h
>> @@ -20,8 +20,10 @@ struct cec_notifier;
>> #if IS_REACHABLE(CONFIG_CEC_CORE) && IS_ENABLED(CONFIG_CEC_NOTIFIER)
>>
>> /**
>> - * cec_notifier_get - find or create a new cec_notifier for the given device.
>> + * 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
>
> Probably best to use "name" instead of connector, since it doesn't necessarily
> _have_ to be a connector name. So, cec_notifier_get_by_name(dev, name)

I don't have a stong opinion, but since the CEC is tied to a connector, it should
mention connector, maybe conn_name ?

>
>> *
>> * If a notifier for device @dev already exists, then increase the refcount
>> * and return that notifier.
>> @@ -31,7 +33,8 @@ struct cec_notifier;
>> *
>> * Return NULL if the memory could not be allocated.
>> */
>> -struct cec_notifier *cec_notifier_get(struct device *dev);
>> +struct cec_notifier *cec_notifier_get_conn(struct device *dev,
>> + const char *conn);
>>
>> /**
>> * cec_notifier_put - decrease refcount and delete when the refcount reaches 0.
>> @@ -85,7 +88,8 @@ 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;
>> @@ -121,6 +125,23 @@ static inline void cec_register_cec_notifier(struct cec_adapter *adap,
>> #endif
>>
>> /**
>> + * cec_notifier_get - find or create a new cec_notifier for the given device.
>> + * @dev: device that sends the events.
>> + *
>> + * 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.
>
> You might also want to cover the case where you have multiple named notifiers
> for the same device. It looks like it just grabs the first one?

Yes, the code grabs the first one if not precised, do you mean I'll need to add
a line to document the behaviour ?

>
> Sean
>
>> + *
>> + * Return NULL if the memory could not be allocated.
>> + */
>> +static inline struct cec_notifier *cec_notifier_get(struct device *dev)
>> +{
>> + return cec_notifier_get_conn(dev, NULL);
>> +}
>> +
>> +/**
>> * cec_notifier_phys_addr_invalidate() - set the physical address to INVALID
>> *
>> * @n: the CEC notifier
>> --
>> 2.7.4
>>
>

Thanks,
Neil

2018-05-21 08:57:13

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] drm/i915: hdmi: add CEC notifier to intel_hdmi

Hi Ville,

On 18/05/2018 15:24, Ville Syrjälä wrote:
> On Fri, May 18, 2018 at 03:05:01PM +0200, 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 <[email protected]>
>> ---
>> drivers/gpu/drm/i915/Kconfig | 1 +
>> drivers/gpu/drm/i915/intel_display.h | 20 ++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_hdmi.c | 13 +++++++++++++
>> 4 files changed, 36 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
>> index dfd9588..2d65d56 100644
>> --- a/drivers/gpu/drm/i915/Kconfig
>> +++ b/drivers/gpu/drm/i915/Kconfig
>> @@ -23,6 +23,7 @@ config DRM_I915
>> select SYNC_FILE
>> select IOSF_MBI
>> select CRC32
>> + select CEC_CORE if CEC_NOTIFIER
>> help
>> Choose this option if you have a system that has "Intel Graphics
>> Media Accelerator" or "HD Graphics" integrated graphics,
>> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
>> index 4e7418b..c68d1c8 100644
>> --- a/drivers/gpu/drm/i915/intel_display.h
>> +++ b/drivers/gpu/drm/i915/intel_display.h
>> @@ -126,6 +126,26 @@ enum port {
>>
>> #define port_name(p) ((p) + 'A')
>>
>> +static inline const char *port_identifier(enum port port)
>> +{
>> + switch (port) {
>> + case PORT_A:
>> + return "Port A";
>> + case PORT_B:
>> + return "Port B";
>> + case PORT_C:
>> + return "Port C";
>> + case PORT_D:
>> + return "Port D";
>> + case PORT_E:
>> + return "Port E";
>> + case PORT_F:
>> + return "Port F";
>> + default:
>> + return "<invalid>";
>> + }
>> +}
>
> I don't think we need this in the header. A static function will do.

I just followed how other functions were declared, in the same way.

> And I was actually thinking something a bit fancier like:
> snprintf("%s/port-%s", dev_name(dev), port_id(port));
>
> Oh, I see you already pass the device in so I guess we don't need
> that in the port id?

Indeed, my bad I forgot to answer your last question, we already pass a struct
device which provides the device bus, name and id, so it's unneeded here.

>
>> +
>> enum dpio_channel {
>> DPIO_CH0,
>> DPIO_CH1
>> 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;
>
> I was wondering if we need any ifdefs around this stuff, but I guess not
> since it's just a pointer and all the functions seem to have empty
> static inlines for the CEC=n case.
>
>> };
>>
>> 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..d522b5b 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;
>> }
>>
>> @@ -2031,6 +2037,8 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder,
>>
>> static void intel_hdmi_destroy(struct drm_connector *connector)
>> {
>> + if (intel_attached_hdmi(connector)->notifier)
>> + cec_notifier_put(intel_attached_hdmi(connector)->notifier);
>> kfree(to_intel_connector(connector)->detect_edid);
>> drm_connector_cleanup(connector);
>> kfree(connector);
>> @@ -2358,6 +2366,11 @@ 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,
>> + port_identifier(port));
>> + 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>

Thans,
Neil

2018-05-21 09:00:56

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

Hi Enric,

On 18/05/2018 18:19, Enric Balletbo Serra wrote:
> Hi Neil,
>
> 2018-05-18 15:05 GMT+02:00 Neil Armstrong <[email protected]>:
>> 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 <[email protected]>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> drivers/platform/chrome/cros_ec_proto.c | 40 +++++++++++++----
>> include/linux/mfd/cros_ec.h | 2 +-
>> include/linux/mfd/cros_ec_commands.h | 80 +++++++++++++++++++++++++++++++++
>> 3 files changed, 112 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e7bbdf9..c4f6c44 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -504,10 +504,31 @@ 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, ec_dev->event_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;
>
> Personal opinion, but I don't like this static here, and also I don't
> think this is scalable. Could we ask for the command version?

I don't have an opinion, I only followed how it was implemented on the
chromeos kernel and adapted to mainline. If you have a better way, I'll use it !

>
>> int ret;
>>
>> if (ec_dev->suspended) {
>> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>> 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(struct ec_response_get_next_event_v1));
>> + if (ret < 0 || msg->result != 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;
>> }
>>
>
> So we always do a failed transfer on all these EC devices that does
> not support CEC. I am wondering if wouldn't be better pass the command
> version to the cros_ec_get_next_event function. The driver should know
> which version to use, just a random idea.

No, the driver cannot know the command version, this depends on the FW version
and the platform. AFAIK this must be discovered.

>
>> + 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 f36125e..32caef3 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..16c3a2b 100644
>> --- a/include/linux/mfd/cros_ec_commands.h
>> +++ b/include/linux/mfd/cros_ec_commands.h
>
> This file is going to be very big and as requested by Lee I plan to
> convert this file to the kernel-doc format, this patch introduces some
> new structs so could you document the new structs in the suggested
> format?

Ok

>
>> @@ -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,
>
> Use the BIT() macro.

Ok

>
>> +};
>>
>> /*****************************************************************************/
>> /*
>> --
>> 2.7.4
>>
>
> Thanks,
> Enric
>


2018-05-21 09:02:22

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 5/5] media: platform: Add Chrome OS EC CEC driver

Hi Enric,

On 18/05/2018 17:02, Enric Balletbo Serra wrote:
> Hi Neil,
>
> 2018-05-18 15:05 GMT+02:00 Neil Armstrong <[email protected]>:
>> The Chrome OS Embedded Controller can expose a CEC bus, this patch add the
>
> A minor nit, there is a "consensus" on tell cros-ec as "ChromeOS
> Embedded Controller" or "ChromeOS EC". Yes, I know that you can see in
> the kernel many other ways to refer to the ChromeOS EC, but to
> standardize a little bit, could you replace all occurrences s/Chrome
> OS/ChromeOS/. Thanks.

Ok, I'll do a cleanup.

>
>> 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.
>>
>> The controller will be tied to the HDMI CEC notifier by using the platform
>> DMI Data and the i915 device name and connector name.
>>
>> Signed-off-by: Neil Armstrong <[email protected]>
>> ---
>> drivers/media/platform/Kconfig | 11 +
>> drivers/media/platform/Makefile | 2 +
>> drivers/media/platform/cros-ec-cec/Makefile | 1 +
>> drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
>> 4 files changed, 361 insertions(+)
>> create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
>> create mode 100644 drivers/media/platform/cros-ec-cec/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"
>
> here
>
>> + 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.
>
> here
>
>> + 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..830696f 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-cec/
>> diff --git a/drivers/media/platform/cros-ec-cec/Makefile b/drivers/media/platform/cros-ec-cec/Makefile
>> new file mode 100644
>> index 0000000..9ce97f9
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec-cec/Makefile
>> @@ -0,0 +1 @@
>> +obj-$(CONFIG_VIDEO_CROS_EC_CEC) += cros-ec-cec.o
>> diff --git a/drivers/media/platform/cros-ec-cec/cros-ec-cec.c b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>> new file mode 100644
>> index 0000000..7e1e275
>> --- /dev/null
>> +++ b/drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>> @@ -0,0 +1,347 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * CEC driver for Chrome OS Embedded Controller
>
> and here
>
>> + *
>> + * Copyright (c) 2018 BayLibre, SAS
>> + * Author: Neil Armstrong <[email protected]>
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/dmi.h>
>> +#include <linux/pci.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
>> + * @rx_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);
>> +
>> + /* FW takes care of all retries, tell core to avoid more retries */
>> + if (events & EC_MKBP_CEC_SEND_FAILED)
>> + cec_transmit_attempt_done(cros_ec_cec->adap,
>> + CEC_TX_STATUS_MAX_RETRIES |
>> + CEC_TX_STATUS_NACK);
>> +}
>> +
>> +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;
>> +
>> + 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;
>> +
>> + 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 writing 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;
>> +
>> + 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);
>> +
>> +#if IS_ENABLED(CONFIG_PCI) && IS_ENABLED(CONFIG_DMI)
>> +
>> +/*
>> + * The Firmware only handles a single CEC interface tied to a single HDMI
>> + * connector we specify along with the DRM device name handling the HDMI output
>> + */
>> +
>> +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", "Port B" },
>> +};
>> +
>> +static int cros_ec_cec_get_notifier(struct device *dev,
>> + 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)) {
>> + struct device *d;
>> +
>> + /* Find the device, bail out if not yet registered */
>> + d = bus_find_device_by_name(&pci_bus_type, NULL,
>> + m->devname);
>> + if (!d)
>> + return -EPROBE_DEFER;
>> +
>> + *notify = cec_notifier_get_conn(d, m->conn);
>> + return 0;
>> + }
>> + }
>> +
>> + /* Hardware support must be added in the cec_dmi_match_table */
>> + dev_warn(dev, "CEC notifier not configured for this hardware\n");
>> +
>> + return -ENODEV;
>> +}
>> +
>> +#else
>> +
>> +static int cros_ec_cec_get_notifier(struct device *dev,
>> + struct cec_notifier **notify)
>> +{
>> + return -ENODEV;
>> +}
>> +
>> +#endif
>> +
>> +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;
>> + 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(&pdev->dev, &cros_ec_cec->notify);
>> + if (ret)
>> + return ret;
>> +
>> + 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_CAP_DEFAULTS, 1);
>> + if (IS_ERR(cros_ec_cec->adap))
>> + return PTR_ERR(cros_ec_cec->adap);
>> +
>> + /* 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 <[email protected]>");
>> +MODULE_LICENSE("GPL");
>> +MODULE_ALIAS("platform:" DRV_NAME);
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dri-devel mailing list
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> The patch looks good to me, so
>
> Reviewed-by: Enric Balletbo i Serra <[email protected]>
>

Thanks,
Neil

2018-05-21 09:04:29

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 0/5] Add ChromeOS EC CEC Support

On 18/05/2018 16:04, Enric Balletbo Serra wrote:
> Hi Neil,
>
> 2018-05-18 15:04 GMT+02:00 Neil Armstrong <[email protected]>:
>> Hi All,
>>
>> The new Google "Fizz" Intel-based ChromeOS device is gaining CEC support
>> through 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 !
>>
>> Changes since v2:
>> - Add i915 port_identifier() and use this stable name as cec_notifier conn name
>> - Fixed and cleaned up the CEC commands and events handling
>> - Rebased the CEC sub-device registration on top of Enric's serie
>> - Fixed comments typo on cec driver
>> - Protected the DMI match only with PCI and DMI Kconfigs
>>
>
> Just because I got confused when I saw two v2 in my inbox. This is v3, right?

Yes, sorry it's v3... next will be v4.

>
>> Changes since v1:
>> - Added cec_notifier_put to intel_hdmi
>> - Fixed all small reported issues on the EC CEC driver
>> - Moved the cec_notifier_get out of the #if .. #else .. #endif
>>
>> Changes since RFC:
>> - Moved CEC sub-device registration after CEC commands and events definitions patch
>> - Removed get_notifier_get_byname
>> - Added CEC_CORE select into i915 Kconfig
>> - Removed CEC driver fallback if notifier is not configured on HW, added explicit warn
>> - Fixed CEC core return type on error
>> - Moved to cros-ec-cec media platform directory
>> - Use bus_find_device() to find the pci i915 device instead of get_notifier_get_byname()
>> - Fix Logical Address setup
>> - Added comment about HW support
>> - Removed memset of msg structures
>>
>> Neil Armstrong (5):
>> 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.
>> mfd: cros_ec_dev: Add CEC sub-device registration
>> media: platform: Add Chrome OS EC CEC driver
>>
>> drivers/gpu/drm/i915/Kconfig | 1 +
>> drivers/gpu/drm/i915/intel_display.h | 20 ++
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> drivers/gpu/drm/i915/intel_hdmi.c | 13 +
>> drivers/media/cec/cec-notifier.c | 11 +-
>> drivers/media/platform/Kconfig | 11 +
>> drivers/media/platform/Makefile | 2 +
>> drivers/media/platform/cros-ec-cec/Makefile | 1 +
>> drivers/media/platform/cros-ec-cec/cros-ec-cec.c | 347 +++++++++++++++++++++++
>> drivers/mfd/cros_ec_dev.c | 16 ++
>> drivers/platform/chrome/cros_ec_proto.c | 40 ++-
>> include/linux/mfd/cros_ec.h | 2 +-
>> include/linux/mfd/cros_ec_commands.h | 80 ++++++
>> include/media/cec-notifier.h | 27 +-
>> 14 files changed, 557 insertions(+), 16 deletions(-)
>> create mode 100644 drivers/media/platform/cros-ec-cec/Makefile
>> create mode 100644 drivers/media/platform/cros-ec-cec/cros-ec-cec.c
>>
>> --
>> 2.7.4
>>


2018-05-24 05:50:07

by Benson Leung

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

Hi Neil, Hi Stefan,

On Fri, May 18, 2018 at 03:05:02PM +0200, Neil Armstrong wrote:
> 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 <[email protected]>
> Signed-off-by: Neil Armstrong <[email protected]>

It looks like this change squashes together this chromeos-4.4 CL
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1061879
and some other new changes to add cec to cros_ec_commands.

Could you separate the two? One patch that's as close to Stefan's
which introduces the 16 byte mkbp, and a second one that
adds the HDMI CEC commands?

Thanks,
Benson
> ---
> drivers/platform/chrome/cros_ec_proto.c | 40 +++++++++++++----
> include/linux/mfd/cros_ec.h | 2 +-
> include/linux/mfd/cros_ec_commands.h | 80 +++++++++++++++++++++++++++++++++
> 3 files changed, 112 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index e7bbdf9..c4f6c44 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -504,10 +504,31 @@ 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, ec_dev->event_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;
>
> if (ec_dev->suspended) {
> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
> 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(struct ec_response_get_next_event_v1));
> + if (ret < 0 || msg->result != 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 f36125e..32caef3 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..16c3a2b 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
>

--
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
[email protected]
Chromium OS Project
[email protected]


Attachments:
(No filename) (7.02 kB)
signature.asc (849.00 B)
Download all attachments

2018-05-24 08:39:23

by Neil Armstrong

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] mfd: cros-ec: Introduce CEC commands and events definitions.

Hi Benson,

On 24/05/2018 07:47, Benson Leung wrote:
> Hi Neil, Hi Stefan,
>
> On Fri, May 18, 2018 at 03:05:02PM +0200, Neil Armstrong wrote:
>> 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 <[email protected]>
>> Signed-off-by: Neil Armstrong <[email protected]>
>
> It looks like this change squashes together this chromeos-4.4 CL
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1061879
> and some other new changes to add cec to cros_ec_commands.
>
> Could you separate the two? One patch that's as close to Stefan's
> which introduces the 16 byte mkbp, and a second one that
> adds the HDMI CEC commands?

OK, will split.

Neil

>
> Thanks,
> Benson
>> ---
>> drivers/platform/chrome/cros_ec_proto.c | 40 +++++++++++++----
>> include/linux/mfd/cros_ec.h | 2 +-
>> include/linux/mfd/cros_ec_commands.h | 80 +++++++++++++++++++++++++++++++++
>> 3 files changed, 112 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
>> index e7bbdf9..c4f6c44 100644
>> --- a/drivers/platform/chrome/cros_ec_proto.c
>> +++ b/drivers/platform/chrome/cros_ec_proto.c
>> @@ -504,10 +504,31 @@ 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, ec_dev->event_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;
>>
>> if (ec_dev->suspended) {
>> @@ -515,18 +536,19 @@ static int get_next_event(struct cros_ec_device *ec_dev)
>> 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(struct ec_response_get_next_event_v1));
>> + if (ret < 0 || msg->result != 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 f36125e..32caef3 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..16c3a2b 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
>>
>



Attachments:
signature.asc (836.00 B)
OpenPGP digital signature