[PATCH v3 0/2] Add SECO Boards CEC device driver

[PATCH v3 0/2] Add SECO Boards CEC device driver

[ektor5]

This series of patches aims to add CEC functionalities to SECO
devices, in particular UDOO X86.

The communication is achieved via Braswell SMBus (i2c-i801) to the
onboard STM32 microcontroller that handles the CEC signals. The driver
use direct access to the PCI addresses, due to the limitations of the
specific driver in presence of ACPI calls.

The basic functionalities are tested with success with cec-ctl and
cec-compliance.

v3:
 - Refactored rx/tx for loops
 - Removed more debug prints
 - Sorted headers 

v2:
 - Removed useless debug prints
 - Added DMI && PCI to dependences
 - Removed useless ifdefs
 - Renamed all irda references to ir
 - Fixed SPDX clause
 - Several style fixes


Ettore Chimenti (2):
  media: add SECO cec driver
  seco-cec: add Consumer-IR support

 MAINTAINERS                                |   6 +
 drivers/media/platform/Kconfig             |  22 +
 drivers/media/platform/Makefile            |   2 +
 drivers/media/platform/seco-cec/Makefile   |   1 +
 drivers/media/platform/seco-cec/seco-cec.c | 822 +++++++++++++++++++++
 drivers/media/platform/seco-cec/seco-cec.h | 141 ++++
 6 files changed, 994 insertions(+)
 create mode 100644 drivers/media/platform/seco-cec/Makefile
 create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
 create mode 100644 drivers/media/platform/seco-cec/seco-cec.h

-- 
2.18.0

[PATCH v3 1/2] media: add SECO cec driver

[ektor5]

From: Ettore Chimenti <ek5.chimenti@gmail.com>

This patch adds support to the CEC device implemented with a STM32
microcontroller in X86 SECO Boards, including UDOO X86.

The communication is achieved via Braswell integrated SMBus
(i2c-i801). The driver use direct access to the PCI addresses, due to
the limitations of the specific driver in presence of ACPI calls.

The basic functionalities are tested with success with cec-ctl and
cec-compliance.

Inspired by cros-ec-cec implementation, attaches to i915 driver
cec-notifier.

Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
---
 MAINTAINERS                                |   6 +
 drivers/media/platform/Kconfig             |  12 +
 drivers/media/platform/Makefile            |   2 +
 drivers/media/platform/seco-cec/Makefile   |   1 +
 drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
 drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
 6 files changed, 850 insertions(+)
 create mode 100644 drivers/media/platform/seco-cec/Makefile
 create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
 create mode 100644 drivers/media/platform/seco-cec/seco-cec.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 4ece30f15777..1062912a5ff4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12972,6 +12972,12 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
 S:	Maintained
 F:	drivers/mmc/host/sdricoh_cs.c
 
+SECO BOARDS CEC DRIVER
+M:	Ettore Chimenti <ek5.chimenti@gmail.com>
+S:	Maintained
+F:	drivers/media/platform/seco-cec/seco-cec.c
+F:	drivers/media/platform/seco-cec/seco-cec.h
+
 SECURE COMPUTING
 M:	Kees Cook <keescook@chromium.org>
 R:	Andy Lutomirski <luto@amacapital.net>
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 94c1fe0e9787..51cd1fd005e3 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -613,6 +613,18 @@ config VIDEO_TEGRA_HDMI_CEC
 	 The CEC bus is present in the HDMI connector and enables communication
 	 between compatible devices.
 
+config VIDEO_SECO_CEC
+	tristate "SECO Boards HDMI CEC driver"
+	depends on (X86 || IA64) || COMPILE_TEST
+	depends on PCI && DMI
+	select CEC_CORE
+	select CEC_NOTIFIER
+	help
+	  This is a driver for SECO Boards integrated CEC interface.
+	  Selecting it will enable support for this device.
+	  CEC bus is present in the HDMI connector and enables communication
+	  between compatible devices.
+
 endif #CEC_PLATFORM_DRIVERS
 
 menuconfig SDR_PLATFORM_DRIVERS
diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
index 41322ab65802..5d2b06c4c68a 100644
--- a/drivers/media/platform/Makefile
+++ b/drivers/media/platform/Makefile
@@ -53,6 +53,8 @@ obj-$(CONFIG_VIDEO_TEGRA_HDMI_CEC)	+= tegra-cec/
 
 obj-y					+= stm32/
 
+obj-$(CONFIG_VIDEO_SECO_CEC)		+= seco-cec/
+
 obj-y					+= davinci/
 
 obj-$(CONFIG_VIDEO_SH_VOU)		+= sh_vou.o
diff --git a/drivers/media/platform/seco-cec/Makefile b/drivers/media/platform/seco-cec/Makefile
new file mode 100644
index 000000000000..09900b087d02
--- /dev/null
+++ b/drivers/media/platform/seco-cec/Makefile
@@ -0,0 +1 @@
+obj-y += seco-cec.o
diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
new file mode 100644
index 000000000000..9ac9a1f36bc0
--- /dev/null
+++ b/drivers/media/platform/seco-cec/seco-cec.c
@@ -0,0 +1,699 @@
+// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
+/*
+ * CEC driver for SECO X86 Boards
+ *
+ * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
+ * Copyright (C) 2018, SECO Srl.
+ * Copyright (C) 2018, Aidilab Srl.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/dmi.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+
+/* CEC Framework */
+#include <media/cec.h>
+
+#include "seco-cec.h"
+
+struct secocec_data {
+	struct device *dev;
+	struct platform_device *pdev;
+	struct cec_adapter *cec_adap;
+	struct cec_notifier *notifier;
+	int irq;
+};
+
+#define smb_wr16(cmd, data) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
+					     cmd, data, SMBUS_WRITE, NULL)
+#define smb_rd16(cmd, res) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
+				       cmd, 0, SMBUS_READ, res)
+
+static int smb_word_op(short data_format, u16 slave_addr, u8 cmd, u16 data,
+		       u8 operation, u16 *result)
+{
+	unsigned int count;
+	short _data_format;
+	int status = 0;
+
+	switch (data_format) {
+	case CMD_BYTE_DATA:
+		_data_format = BRA_SMB_CMD_BYTE_DATA;
+		break;
+	case CMD_WORD_DATA:
+		_data_format = BRA_SMB_CMD_WORD_DATA;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	/* Active wait until ready */
+	for (count = 0; count <= SMBTIMEOUT; ++count) {
+		if (!(inb(HSTS) & BRA_INUSE_STS))
+			break;
+		udelay(SMB_POLL_UDELAY);
+	}
+
+	if (count > SMBTIMEOUT)
+		/* Reset the lock instead of failing */
+		outb(0xff, HSTS);
+
+	outb(0x00, HCNT);
+	outb((u8)(slave_addr & 0xfe) | operation, XMIT_SLVA);
+	outb(cmd, HCMD);
+	inb(HCNT);
+
+	if (operation == SMBUS_WRITE) {
+		outb((u8)data, HDAT0);
+		outb((u8)(data >> 8), HDAT1);
+	}
+
+	outb(BRA_START + _data_format, HCNT);
+
+	for (count = 0; count <= SMBTIMEOUT; count++) {
+		if (!(inb(HSTS) & BRA_HOST_BUSY))
+			break;
+		udelay(SMB_POLL_UDELAY);
+	}
+
+	if (count > SMBTIMEOUT) {
+		status = -EBUSY;
+		goto err;
+	}
+
+	if (inb(HSTS) & BRA_HSTS_ERR_MASK) {
+		status = -EIO;
+		goto err;
+	}
+
+	if (operation == SMBUS_READ)
+		*result = ((inb(HDAT0) & 0xff) + ((inb(HDAT1) & 0xff) << 8));
+
+err:
+	outb(0xff, HSTS);
+	return status;
+}
+
+static int secocec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+	struct secocec_data *cec = cec_get_drvdata(adap);
+	struct device *dev = cec->dev;
+	u16 val = 0;
+	int status;
+
+	if (enable) {
+		/* Clear the status register */
+		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
+		if (status)
+			goto err;
+
+		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+		if (status)
+			goto err;
+
+		/* Enable the interrupts */
+		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+		if (status)
+			goto err;
+
+		status = smb_wr16(SECOCEC_ENABLE_REG_1,
+				  val | SECOCEC_ENABLE_REG_1_CEC);
+		if (status)
+			goto err;
+
+		dev_dbg(dev, "Device enabled");
+	} else {
+		/* Clear the status register */
+		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
+		if (status)
+			goto err;
+
+		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+		if (status)
+			goto err;
+
+		/* Disable the interrupts */
+		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+		if (status)
+			goto err;
+
+		status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
+				  ~SECOCEC_ENABLE_REG_1_CEC &
+				  ~SECOCEC_ENABLE_REG_1_IR);
+		if (status)
+			goto err;
+
+		dev_dbg(dev, "Device disabled");
+	}
+
+	return 0;
+err:
+	dev_err(dev, "Adapter setup failed (%d)", status);
+	return status;
+}
+
+static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
+{
+	u16 enable_val = 0;
+	int status;
+
+	/* Disable device */
+	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
+	if (status)
+		return status;
+
+	status = smb_wr16(SECOCEC_ENABLE_REG_1,
+			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
+	if (status)
+		return status;
+
+	/* Write logical address */
+	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);
+	if (status)
+		return status;
+
+	/* Re-enable device */
+	status = smb_wr16(SECOCEC_ENABLE_REG_1,
+			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
+	if (status)
+		return status;
+
+	return 0;
+}
+
+static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+				 u32 signal_free_time, struct cec_msg *msg)
+{
+	struct secocec_data *cec = cec_get_drvdata(adap);
+	struct device *dev = cec->dev;
+	u16 payload_len, payload_id_len, destination, val = 0;
+	u8 *payload_msg;
+	int status;
+	u8 i;
+
+	/* Device msg len already accounts for header */
+	payload_id_len = msg->len - 1;
+
+	/* Send data length */
+	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
+	if (status)
+		goto err;
+
+	/* Send Operation ID if present */
+	if (payload_id_len > 0) {
+		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
+		if (status)
+			goto err;
+	}
+	/* Send data if present */
+	if (payload_id_len > 1) {
+		/* Only data; */
+		payload_len = msg->len - 2;
+		payload_msg = &msg->msg[2];
+
+		/* Copy message into registers */
+		for (i = 0; i < payload_len; i += 2) {
+			/* hi byte */
+			val = payload_msg[i + 1] << 8;
+
+			/* lo byte */
+			val |= payload_msg[i];
+
+			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
+			if (status)
+				goto err;
+		}
+	}
+	/* Send msg source/destination and fire msg */
+	destination = msg->msg[0];
+	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
+	if (status)
+		goto err;
+
+	return 0;
+
+err:
+	dev_err(dev, "Transmit failed (%d)", status);
+	return status;
+}
+
+static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)
+{
+	int status = 0;
+
+	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
+		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
+			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
+			status = -EAGAIN;
+		} else {
+			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
+			status = -EIO;
+		}
+	} else {
+		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
+	}
+
+	/* Reset status reg */
+	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
+		SECOCEC_STATUS_MSG_SENT_MASK |
+		SECOCEC_STATUS_TX_NACK_ERROR;
+	smb_wr16(SECOCEC_STATUS, status_val);
+
+	return status;
+}
+
+static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
+{
+	struct secocec_data *cec = cec_get_drvdata(adap);
+	struct device *dev = cec->dev;
+	struct cec_msg msg = { };
+	bool flag_overflow = false;
+	u8 payload_len, i = 0;
+	u8 *payload_msg;
+	u16 val = 0;
+	int status;
+
+	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
+		dev_warn(dev, "Received more than 16 bytes. Discarding");
+		flag_overflow = true;
+	}
+
+	if (status_val & SECOCEC_STATUS_RX_ERROR_MASK) {
+		dev_warn(dev, "Message received with errors. Discarding");
+		status = -EIO;
+		goto rxerr;
+	}
+
+	/* Read message length */
+	status = smb_rd16(SECOCEC_READ_DATA_LENGTH, &val);
+	if (status)
+		goto err;
+
+	/* Device msg len already accounts for the header */
+	msg.len = min(val + 1, CEC_MAX_MSG_SIZE);
+
+	/* Read logical address */
+	status = smb_rd16(SECOCEC_READ_BYTE0, &val);
+	if (status)
+		goto err;
+
+	/* device stores source LA and destination */
+	msg.msg[0] = val;
+
+	/* Read operation ID */
+	status = smb_rd16(SECOCEC_READ_OPERATION_ID, &val);
+	if (status)
+		goto err;
+
+	msg.msg[1] = val;
+
+	/* Read data if present */
+	if (msg.len > 1) {
+		payload_len = msg.len - 2;
+		payload_msg = &msg.msg[2];
+
+		/* device stores 2 bytes in every 16-bit val */
+		for (i = 0; i < payload_len; i += 2) {
+			status = smb_rd16(SECOCEC_READ_DATA_00 + i / 2, &val);
+			if (status)
+				goto err;
+
+			/* low byte, skipping header */
+			payload_msg[i] = val & 0x00ff;
+
+			/* hi byte */
+			payload_msg[i + 1] = (val & 0xff00) >> 8;
+		}
+	}
+
+	cec_received_msg(cec->cec_adap, &msg);
+
+	/* Reset status reg */
+	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK;
+	if (flag_overflow)
+		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
+
+	status = smb_wr16(SECOCEC_STATUS, status_val);
+	if (status)
+		goto err;
+
+	return 0;
+
+rxerr:
+	/* Reset error reg */
+	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK |
+		SECOCEC_STATUS_RX_ERROR_MASK;
+	if (flag_overflow)
+		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
+	smb_wr16(SECOCEC_STATUS, status_val);
+
+err:
+	dev_err(dev, "Receive message failed (%d)", status);
+	return status;
+}
+
+struct cec_adap_ops secocec_cec_adap_ops = {
+	/* Low-level callbacks */
+	.adap_enable = secocec_adap_enable,
+	.adap_log_addr = secocec_adap_log_addr,
+	.adap_transmit = secocec_adap_transmit,
+};
+
+static irqreturn_t secocec_irq_handler(int irq, void *priv)
+{
+	struct secocec_data *cec = priv;
+	struct device *dev = cec->dev;
+	u16 status_val, cec_val, val = 0;
+	int status;
+
+	/*  Read status register */
+	status = smb_rd16(SECOCEC_STATUS_REG_1, &status_val);
+	if (status)
+		goto err;
+
+	if (status_val & SECOCEC_STATUS_REG_1_CEC) {
+		/* Read CEC status register */
+		status = smb_rd16(SECOCEC_STATUS, &cec_val);
+		if (status)
+			goto err;
+
+		if (cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK)
+			secocec_rx_done(cec->cec_adap, cec_val);
+
+		if (cec_val & SECOCEC_STATUS_MSG_SENT_MASK)
+			secocec_tx_done(cec->cec_adap, cec_val);
+
+		if ((~cec_val & SECOCEC_STATUS_MSG_SENT_MASK) &&
+		    (~cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK))
+			dev_warn(dev,
+				 "Message not received or sent, but interrupt fired");
+
+		val = SECOCEC_STATUS_REG_1_CEC;
+	}
+
+	if (status_val & SECOCEC_STATUS_REG_1_IR) {
+		dev_dbg(dev, "IR RC5 Interrupt Caught");
+		val |= SECOCEC_STATUS_REG_1_IR;
+		/* TODO IRDA RX */
+	}
+
+	/*  Reset status register */
+	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+	if (status)
+		goto err;
+
+	return IRQ_HANDLED;
+
+err:
+	dev_err(dev, "IRQ: Read/Write SMBus operation failed (%d)", status);
+
+	/*  Reset status register */
+	val = SECOCEC_STATUS_REG_1_CEC | SECOCEC_STATUS_REG_1_IR;
+	smb_wr16(SECOCEC_STATUS_REG_1, val);
+
+	return IRQ_HANDLED;
+}
+
+struct cec_dmi_match {
+	char *sys_vendor;
+	char *product_name;
+	char *devname;
+	char *conn;
+};
+
+static const struct cec_dmi_match secocec_dmi_match_table[] = {
+	/* UDOO X86 */
+	{ "SECO", "UDOO x86", "0000:00:02.0", "Port B" },
+};
+
+static int secocec_cec_get_notifier(struct cec_notifier **notify)
+{
+	int i;
+
+	for (i = 0 ; i < ARRAY_SIZE(secocec_dmi_match_table) ; ++i) {
+		const struct cec_dmi_match *m = &secocec_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;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int secocec_acpi_probe(struct secocec_data *sdev)
+{
+	struct device *dev = sdev->dev;
+	struct gpio_desc *gpio;
+	int irq = 0;
+
+	gpio = devm_gpiod_get(dev, NULL, GPIOF_IN);
+	if (IS_ERR(gpio)) {
+		dev_err(dev, "Cannot request interrupt gpio");
+		return PTR_ERR(gpio);
+	}
+
+	irq = gpiod_to_irq(gpio);
+	if (irq < 0) {
+		dev_err(dev, "Cannot find valid irq");
+		return -ENODEV;
+	}
+	dev_dbg(dev, "irq-gpio is bound to IRQ %d", irq);
+
+	sdev->irq = irq;
+
+	return 0;
+}
+
+static int secocec_probe(struct platform_device *pdev)
+{
+	struct secocec_data *secocec;
+	struct device *dev = &pdev->dev;
+	int ret;
+	u16 val;
+
+	secocec = devm_kzalloc(dev, sizeof(*secocec), GFP_KERNEL);
+	if (!secocec)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, secocec);
+
+	/* Request SMBus regions */
+	if (!request_muxed_region(BRA_SMB_BASE_ADDR, 7, "CEC00001")) {
+		dev_err(dev, "Request memory region failed");
+		return -ENXIO;
+	}
+
+	secocec->pdev = pdev;
+	secocec->dev = dev;
+
+	if (!has_acpi_companion(dev)) {
+		dev_dbg(dev, "Cannot find any ACPI companion");
+		ret = -ENODEV;
+		goto err;
+	}
+
+	ret = secocec_acpi_probe(secocec);
+	if (ret) {
+		dev_err(dev, "Cannot assign gpio to IRQ");
+		ret = -ENODEV;
+		goto err;
+	}
+
+	dev_dbg(dev, "IRQ detected at %d", secocec->irq);
+
+	/* Firmware version check */
+	ret = smb_rd16(SECOCEC_VERSION, &val);
+	if (ret) {
+		dev_err(dev, "Cannot check fw version");
+		goto err;
+	}
+	if (val < SECOCEC_LATEST_FW) {
+		dev_err(dev, "CEC Firmware not supported (v.%04x). Use ver > v.%04x",
+			val, SECOCEC_LATEST_FW);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	ret = secocec_cec_get_notifier(&secocec->notifier);
+	if (ret) {
+		dev_err(dev, "no CEC notifier available\n");
+		goto err;
+	}
+
+	ret = devm_request_threaded_irq(dev,
+					secocec->irq,
+					NULL,
+					secocec_irq_handler,
+					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					dev_name(&pdev->dev), secocec);
+
+	if (ret) {
+		dev_err(dev, "Cannot request IRQ %d", secocec->irq);
+		ret = -EIO;
+		goto err;
+	}
+
+	/* Allocate CEC adapter */
+	secocec->cec_adap = cec_allocate_adapter(&secocec_cec_adap_ops,
+						 secocec,
+						 dev_name(dev),
+						 CEC_CAP_DEFAULTS,
+						 SECOCEC_MAX_ADDRS);
+
+	if (IS_ERR(secocec->cec_adap)) {
+		ret = PTR_ERR(secocec->cec_adap);
+		goto err;
+	}
+
+	ret = cec_register_adapter(secocec->cec_adap, dev);
+	if (ret)
+		goto err_delete_adapter;
+
+	if (secocec->notifier)
+		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
+
+	platform_set_drvdata(pdev, secocec);
+
+	dev_dbg(dev, "Device registered");
+
+	return ret;
+
+err_delete_adapter:
+	cec_delete_adapter(secocec->cec_adap);
+err:
+	dev_err(dev, "%s device probe failed\n", dev_name(dev));
+
+	return ret;
+}
+
+static int secocec_remove(struct platform_device *pdev)
+{
+	struct secocec_data *secocec = platform_get_drvdata(pdev);
+
+	cec_unregister_adapter(secocec->cec_adap);
+
+	if (secocec->notifier)
+		cec_notifier_put(secocec->notifier);
+
+	release_region(BRA_SMB_BASE_ADDR, 7);
+
+	dev_dbg(&pdev->dev, "CEC device removed");
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int secocec_suspend(struct device *dev)
+{
+	int status;
+	u16 val;
+
+	dev_dbg(dev, "Device going to suspend, disabling");
+
+	/* Clear the status register */
+	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
+	if (status)
+		goto err;
+
+	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+	if (status)
+		goto err;
+
+	/* Disable the interrupts */
+	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+	if (status)
+		goto err;
+
+	status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
+			  ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
+	if (status)
+		goto err;
+
+	return 0;
+
+err:
+	dev_err(dev, "Suspend failed (err: %d)", status);
+	return status;
+}
+
+static int secocec_resume(struct device *dev)
+{
+	int status;
+	u16 val;
+
+	dev_dbg(dev, "Resuming device from suspend");
+
+	/* Clear the status register */
+	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
+	if (status)
+		goto err;
+
+	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+	if (status)
+		goto err;
+
+	/* Enable the interrupts */
+	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+	if (status)
+		goto err;
+
+	status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
+	if (status)
+		goto err;
+
+	dev_dbg(dev, "Device resumed from suspend");
+
+	return 0;
+
+err:
+	dev_err(dev, "Resume failed (err: %d)", status);
+	return status;
+}
+
+static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
+#define SECOCEC_PM_OPS (&secocec_pm_ops)
+#else
+#define SECOCEC_PM_OPS NULL
+#endif
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id secocec_acpi_match[] = {
+	{"CEC00001", 0},
+	{},
+};
+
+MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
+#endif
+
+static struct platform_driver secocec_driver = {
+	.driver = {
+		   .name = SECOCEC_DEV_NAME,
+		   .acpi_match_table = ACPI_PTR(secocec_acpi_match),
+		   .pm = SECOCEC_PM_OPS,
+	},
+	.probe = secocec_probe,
+	.remove = secocec_remove,
+};
+
+module_platform_driver(secocec_driver);
+
+MODULE_DESCRIPTION("SECO CEC X86 Driver");
+MODULE_AUTHOR("Ettore Chimenti <ek5.chimenti@gmail.com>");
+MODULE_LICENSE("Dual BSD/GPL");
diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
new file mode 100644
index 000000000000..93020900935e
--- /dev/null
+++ b/drivers/media/platform/seco-cec/seco-cec.h
@@ -0,0 +1,130 @@
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
+/*
+ * SECO X86 Boards CEC register defines
+ *
+ * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
+ * Copyright (C) 2018, SECO Srl.
+ * Copyright (C) 2018, Aidilab Srl.
+ */
+
+#ifndef __SECO_CEC_H__
+#define __SECO_CEC_H__
+
+#define SECOCEC_MAX_ADDRS		1
+#define SECOCEC_DEV_NAME		"secocec"
+#define SECOCEC_LATEST_FW		0x0f0b
+
+#define SMBTIMEOUT			0xffff
+#define SMB_POLL_UDELAY			10
+
+#define SMBUS_WRITE			0
+#define SMBUS_READ			1
+
+#define CMD_BYTE_DATA			0
+#define CMD_WORD_DATA			1
+
+/*
+ * SMBus definitons for Braswell
+ */
+
+#define BRA_DONE_STATUS			BIT(7)
+#define BRA_INUSE_STS			BIT(6)
+#define BRA_FAILED_OP			BIT(4)
+#define BRA_BUS_ERR			BIT(3)
+#define BRA_DEV_ERR			BIT(2)
+#define BRA_INTR			BIT(1)
+#define BRA_HOST_BUSY			BIT(0)
+#define BRA_HSTS_ERR_MASK   (BRA_FAILED_OP | BRA_BUS_ERR | BRA_DEV_ERR)
+
+#define BRA_PEC_EN			BIT(7)
+#define BRA_START			BIT(6)
+#define BRA_LAST__BYTE			BIT(5)
+#define BRA_INTREN			BIT(0)
+#define BRA_SMB_CMD			(7 << 2)
+#define BRA_SMB_CMD_QUICK		(0 << 2)
+#define BRA_SMB_CMD_BYTE		(1 << 2)
+#define BRA_SMB_CMD_BYTE_DATA		(2 << 2)
+#define BRA_SMB_CMD_WORD_DATA		(3 << 2)
+#define BRA_SMB_CMD_PROCESS_CALL	(4 << 2)
+#define BRA_SMB_CMD_BLOCK		(5 << 2)
+#define BRA_SMB_CMD_I2CREAD		(6 << 2)
+#define BRA_SMB_CMD_BLOCK_PROCESS	(7 << 2)
+
+#define BRA_SMB_BASE_ADDR  0x2040
+#define HSTS               (BRA_SMB_BASE_ADDR + 0)
+#define HCNT               (BRA_SMB_BASE_ADDR + 2)
+#define HCMD               (BRA_SMB_BASE_ADDR + 3)
+#define XMIT_SLVA          (BRA_SMB_BASE_ADDR + 4)
+#define HDAT0              (BRA_SMB_BASE_ADDR + 5)
+#define HDAT1              (BRA_SMB_BASE_ADDR + 6)
+
+/*
+ * Microcontroller Address
+ */
+
+#define SECOCEC_MICRO_ADDRESS		0x40
+
+/*
+ * STM32 SMBus Registers
+ */
+
+#define SECOCEC_VERSION			0x00
+#define SECOCEC_ENABLE_REG_1		0x01
+#define SECOCEC_ENABLE_REG_2		0x02
+#define SECOCEC_STATUS_REG_1		0x03
+#define SECOCEC_STATUS_REG_2		0x04
+
+#define SECOCEC_STATUS			0x28
+#define SECOCEC_DEVICE_LA		0x29
+#define SECOCEC_READ_OPERATION_ID	0x2a
+#define SECOCEC_READ_DATA_LENGTH	0x2b
+#define SECOCEC_READ_DATA_00		0x2c
+#define SECOCEC_READ_DATA_02		0x2d
+#define SECOCEC_READ_DATA_04		0x2e
+#define SECOCEC_READ_DATA_06		0x2f
+#define SECOCEC_READ_DATA_08		0x30
+#define SECOCEC_READ_DATA_10		0x31
+#define SECOCEC_READ_DATA_12		0x32
+#define SECOCEC_READ_BYTE0		0x33
+#define SECOCEC_WRITE_OPERATION_ID	0x34
+#define SECOCEC_WRITE_DATA_LENGTH	0x35
+#define SECOCEC_WRITE_DATA_00		0x36
+#define SECOCEC_WRITE_DATA_02		0x37
+#define SECOCEC_WRITE_DATA_04		0x38
+#define SECOCEC_WRITE_DATA_06		0x39
+#define SECOCEC_WRITE_DATA_08		0x3a
+#define SECOCEC_WRITE_DATA_10		0x3b
+#define SECOCEC_WRITE_DATA_12		0x3c
+#define SECOCEC_WRITE_BYTE0		0x3d
+
+#define SECOCEC_IR_READ_DATA		0x3e
+
+/*
+ * Enabling register
+ */
+
+#define SECOCEC_ENABLE_REG_1_CEC		0x1000
+#define SECOCEC_ENABLE_REG_1_IR			0x2000
+#define SECOCEC_ENABLE_REG_1_IR_PASSTHROUGH	0x4000
+
+/*
+ * Status register
+ */
+
+#define SECOCEC_STATUS_REG_1_CEC	SECOCEC_ENABLE_REG_1_CEC
+#define SECOCEC_STATUS_REG_1_IR		SECOCEC_ENABLE_REG_1_IR
+#define SECOCEC_STATUS_REG_1_IR_PASSTHR	SECOCEC_ENABLE_REG_1_IR_PASSTHR
+
+/*
+ * Status data
+ */
+
+#define SECOCEC_STATUS_MSG_RECEIVED_MASK	BIT(0)
+#define SECOCEC_STATUS_RX_ERROR_MASK		BIT(1)
+#define SECOCEC_STATUS_MSG_SENT_MASK		BIT(2)
+#define SECOCEC_STATUS_TX_ERROR_MASK		BIT(3)
+
+#define SECOCEC_STATUS_TX_NACK_ERROR		BIT(4)
+#define SECOCEC_STATUS_RX_OVERFLOW_MASK		BIT(5)
+
+#endif /* __SECO_CEC_H__ */
-- 
2.18.0

Re: [PATCH v3 1/2] media: add SECO cec driver

[Hans Verkuil]

Hi Ettore,

Just a few small things and it is ready to go:

On 10/17/2018 11:31 PM, ektor5 wrote:
> From: Ettore Chimenti <ek5.chimenti@gmail.com>
> 
> This patch adds support to the CEC device implemented with a STM32
> microcontroller in X86 SECO Boards, including UDOO X86.
> 
> The communication is achieved via Braswell integrated SMBus
> (i2c-i801). The driver use direct access to the PCI addresses, due to
> the limitations of the specific driver in presence of ACPI calls.
> 
> The basic functionalities are tested with success with cec-ctl and
> cec-compliance.
> 
> Inspired by cros-ec-cec implementation, attaches to i915 driver
> cec-notifier.
> 
> Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
> ---
>  MAINTAINERS                                |   6 +
>  drivers/media/platform/Kconfig             |  12 +
>  drivers/media/platform/Makefile            |   2 +
>  drivers/media/platform/seco-cec/Makefile   |   1 +
>  drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
>  drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
>  6 files changed, 850 insertions(+)
>  create mode 100644 drivers/media/platform/seco-cec/Makefile
>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4ece30f15777..1062912a5ff4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12972,6 +12972,12 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
>  S:	Maintained
>  F:	drivers/mmc/host/sdricoh_cs.c
>  
> +SECO BOARDS CEC DRIVER
> +M:	Ettore Chimenti <ek5.chimenti@gmail.com>
> +S:	Maintained
> +F:	drivers/media/platform/seco-cec/seco-cec.c
> +F:	drivers/media/platform/seco-cec/seco-cec.h
> +
>  SECURE COMPUTING
>  M:	Kees Cook <keescook@chromium.org>
>  R:	Andy Lutomirski <luto@amacapital.net>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 94c1fe0e9787..51cd1fd005e3 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -613,6 +613,18 @@ config VIDEO_TEGRA_HDMI_CEC
>  	 The CEC bus is present in the HDMI connector and enables communication
>  	 between compatible devices.
>  
> +config VIDEO_SECO_CEC
> +	tristate "SECO Boards HDMI CEC driver"
> +	depends on (X86 || IA64) || COMPILE_TEST
> +	depends on PCI && DMI
> +	select CEC_CORE
> +	select CEC_NOTIFIER
> +	help
> +	  This is a driver for SECO Boards integrated CEC interface.
> +	  Selecting it will enable support for this device.
> +	  CEC bus is present in the HDMI connector and enables communication
> +	  between compatible devices.
> +
>  endif #CEC_PLATFORM_DRIVERS
>  
>  menuconfig SDR_PLATFORM_DRIVERS
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 41322ab65802..5d2b06c4c68a 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -53,6 +53,8 @@ obj-$(CONFIG_VIDEO_TEGRA_HDMI_CEC)	+= tegra-cec/
>  
>  obj-y					+= stm32/
>  
> +obj-$(CONFIG_VIDEO_SECO_CEC)		+= seco-cec/
> +
>  obj-y					+= davinci/
>  
>  obj-$(CONFIG_VIDEO_SH_VOU)		+= sh_vou.o
> diff --git a/drivers/media/platform/seco-cec/Makefile b/drivers/media/platform/seco-cec/Makefile
> new file mode 100644
> index 000000000000..09900b087d02
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/Makefile
> @@ -0,0 +1 @@
> +obj-y += seco-cec.o
> diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
> new file mode 100644
> index 000000000000..9ac9a1f36bc0
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/seco-cec.c
> @@ -0,0 +1,699 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + * CEC driver for SECO X86 Boards
> + *
> + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> + * Copyright (C) 2018, SECO Srl.
> + * Copyright (C) 2018, Aidilab Srl.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +
> +/* CEC Framework */
> +#include <media/cec.h>
> +
> +#include "seco-cec.h"
> +
> +struct secocec_data {
> +	struct device *dev;
> +	struct platform_device *pdev;
> +	struct cec_adapter *cec_adap;
> +	struct cec_notifier *notifier;
> +	int irq;
> +};
> +
> +#define smb_wr16(cmd, data) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> +					     cmd, data, SMBUS_WRITE, NULL)
> +#define smb_rd16(cmd, res) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> +				       cmd, 0, SMBUS_READ, res)
> +
> +static int smb_word_op(short data_format, u16 slave_addr, u8 cmd, u16 data,
> +		       u8 operation, u16 *result)
> +{
> +	unsigned int count;
> +	short _data_format;
> +	int status = 0;
> +
> +	switch (data_format) {
> +	case CMD_BYTE_DATA:
> +		_data_format = BRA_SMB_CMD_BYTE_DATA;
> +		break;
> +	case CMD_WORD_DATA:
> +		_data_format = BRA_SMB_CMD_WORD_DATA;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Active wait until ready */
> +	for (count = 0; count <= SMBTIMEOUT; ++count) {
> +		if (!(inb(HSTS) & BRA_INUSE_STS))
> +			break;
> +		udelay(SMB_POLL_UDELAY);
> +	}
> +
> +	if (count > SMBTIMEOUT)
> +		/* Reset the lock instead of failing */
> +		outb(0xff, HSTS);
> +
> +	outb(0x00, HCNT);
> +	outb((u8)(slave_addr & 0xfe) | operation, XMIT_SLVA);
> +	outb(cmd, HCMD);
> +	inb(HCNT);
> +
> +	if (operation == SMBUS_WRITE) {
> +		outb((u8)data, HDAT0);
> +		outb((u8)(data >> 8), HDAT1);
> +	}
> +
> +	outb(BRA_START + _data_format, HCNT);
> +
> +	for (count = 0; count <= SMBTIMEOUT; count++) {
> +		if (!(inb(HSTS) & BRA_HOST_BUSY))
> +			break;
> +		udelay(SMB_POLL_UDELAY);
> +	}
> +
> +	if (count > SMBTIMEOUT) {
> +		status = -EBUSY;
> +		goto err;
> +	}
> +
> +	if (inb(HSTS) & BRA_HSTS_ERR_MASK) {
> +		status = -EIO;
> +		goto err;
> +	}
> +
> +	if (operation == SMBUS_READ)
> +		*result = ((inb(HDAT0) & 0xff) + ((inb(HDAT1) & 0xff) << 8));
> +
> +err:
> +	outb(0xff, HSTS);
> +	return status;
> +}
> +
> +static int secocec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	u16 val = 0;
> +	int status;
> +
> +	if (enable) {
> +		/* Clear the status register */
> +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +		if (status)
> +			goto err;
> +
> +		/* Enable the interrupts */
> +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +				  val | SECOCEC_ENABLE_REG_1_CEC);
> +		if (status)
> +			goto err;
> +
> +		dev_dbg(dev, "Device enabled");
> +	} else {
> +		/* Clear the status register */
> +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +		if (status)
> +			goto err;
> +
> +		/* Disable the interrupts */
> +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> +				  ~SECOCEC_ENABLE_REG_1_CEC &
> +				  ~SECOCEC_ENABLE_REG_1_IR);
> +		if (status)
> +			goto err;
> +
> +		dev_dbg(dev, "Device disabled");
> +	}
> +
> +	return 0;
> +err:
> +	dev_err(dev, "Adapter setup failed (%d)", status);

Drop the dev_dbg/dev_err lines: the cec module has enough debugging
already.

> +	return status;

This should be:

	return enable ? status : 0;

Disabling shouldn't return an error, it would result in a WARN from
the cec framework.

In fact, I would drop all the error checks in the disable path since they
are ignored anyway.

> +}
> +
> +static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	u16 enable_val = 0;
> +	int status;

If logical_addr == CEC_LOG_ADDR_INVALID, then this function must not return
an error or the CEC framework will WARN about it.

> +
> +	/* Disable device */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
> +	if (status)
> +		return status;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		return status;
> +
> +	/* Write logical address */
> +	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);

So does writing CEC_LOG_ADDR_INVALID (0xff) invalidate all logical
addresses? I see no special code for that. If that is indeed the
case, then you should document this.

> +	if (status)
> +		return status;
> +
> +	/* Re-enable device */
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				 u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	u16 payload_len, payload_id_len, destination, val = 0;
> +	u8 *payload_msg;
> +	int status;
> +	u8 i;
> +
> +	/* Device msg len already accounts for header */
> +	payload_id_len = msg->len - 1;
> +
> +	/* Send data length */
> +	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
> +	if (status)
> +		goto err;
> +
> +	/* Send Operation ID if present */
> +	if (payload_id_len > 0) {
> +		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
> +		if (status)
> +			goto err;
> +	}
> +	/* Send data if present */
> +	if (payload_id_len > 1) {
> +		/* Only data; */
> +		payload_len = msg->len - 2;
> +		payload_msg = &msg->msg[2];
> +
> +		/* Copy message into registers */
> +		for (i = 0; i < payload_len; i += 2) {
> +			/* hi byte */
> +			val = payload_msg[i + 1] << 8;
> +
> +			/* lo byte */
> +			val |= payload_msg[i];
> +
> +			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
> +			if (status)
> +				goto err;
> +		}
> +	}
> +	/* Send msg source/destination and fire msg */
> +	destination = msg->msg[0];
> +	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
> +	if (status)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Transmit failed (%d)", status);

You can drop this: the cec module has already debug code for that.

> +	return status;
> +}
> +
> +static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)

Just return void since the return code is ignored anyway.

> +{
> +	int status = 0;
> +
> +	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
> +		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
> +			status = -EAGAIN;
> +		} else {
> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
> +			status = -EIO;
> +		}
> +	} else {
> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> +	}
> +
> +	/* Reset status reg */
> +	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
> +		SECOCEC_STATUS_MSG_SENT_MASK |
> +		SECOCEC_STATUS_TX_NACK_ERROR;
> +	smb_wr16(SECOCEC_STATUS, status_val);
> +
> +	return status;
> +}
> +
> +static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
> +{

Ditto.

> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	struct cec_msg msg = { };
> +	bool flag_overflow = false;
> +	u8 payload_len, i = 0;
> +	u8 *payload_msg;
> +	u16 val = 0;
> +	int status;
> +
> +	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
> +		dev_warn(dev, "Received more than 16 bytes. Discarding");

Is it better to just receive the first 16 bytes?

> +		flag_overflow = true;
> +	}
> +
> +	if (status_val & SECOCEC_STATUS_RX_ERROR_MASK) {
> +		dev_warn(dev, "Message received with errors. Discarding");
> +		status = -EIO;
> +		goto rxerr;
> +	}
> +
> +	/* Read message length */
> +	status = smb_rd16(SECOCEC_READ_DATA_LENGTH, &val);
> +	if (status)
> +		goto err;
> +
> +	/* Device msg len already accounts for the header */
> +	msg.len = min(val + 1, CEC_MAX_MSG_SIZE);
> +
> +	/* Read logical address */
> +	status = smb_rd16(SECOCEC_READ_BYTE0, &val);
> +	if (status)
> +		goto err;
> +
> +	/* device stores source LA and destination */
> +	msg.msg[0] = val;
> +
> +	/* Read operation ID */
> +	status = smb_rd16(SECOCEC_READ_OPERATION_ID, &val);
> +	if (status)
> +		goto err;
> +
> +	msg.msg[1] = val;
> +
> +	/* Read data if present */
> +	if (msg.len > 1) {
> +		payload_len = msg.len - 2;
> +		payload_msg = &msg.msg[2];
> +
> +		/* device stores 2 bytes in every 16-bit val */
> +		for (i = 0; i < payload_len; i += 2) {
> +			status = smb_rd16(SECOCEC_READ_DATA_00 + i / 2, &val);
> +			if (status)
> +				goto err;
> +
> +			/* low byte, skipping header */
> +			payload_msg[i] = val & 0x00ff;
> +
> +			/* hi byte */
> +			payload_msg[i + 1] = (val & 0xff00) >> 8;
> +		}
> +	}
> +
> +	cec_received_msg(cec->cec_adap, &msg);
> +
> +	/* Reset status reg */
> +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK;
> +	if (flag_overflow)
> +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> +
> +	status = smb_wr16(SECOCEC_STATUS, status_val);
> +	if (status)
> +		goto err;
> +
> +	return 0;
> +
> +rxerr:
> +	/* Reset error reg */
> +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK |
> +		SECOCEC_STATUS_RX_ERROR_MASK;
> +	if (flag_overflow)
> +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> +	smb_wr16(SECOCEC_STATUS, status_val);
> +
> +err:
> +	dev_err(dev, "Receive message failed (%d)", status);
> +	return status;
> +}
> +
> +struct cec_adap_ops secocec_cec_adap_ops = {
> +	/* Low-level callbacks */
> +	.adap_enable = secocec_adap_enable,
> +	.adap_log_addr = secocec_adap_log_addr,
> +	.adap_transmit = secocec_adap_transmit,
> +};
> +
> +static irqreturn_t secocec_irq_handler(int irq, void *priv)
> +{
> +	struct secocec_data *cec = priv;
> +	struct device *dev = cec->dev;
> +	u16 status_val, cec_val, val = 0;
> +	int status;
> +
> +	/*  Read status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &status_val);
> +	if (status)
> +		goto err;
> +
> +	if (status_val & SECOCEC_STATUS_REG_1_CEC) {
> +		/* Read CEC status register */
> +		status = smb_rd16(SECOCEC_STATUS, &cec_val);
> +		if (status)
> +			goto err;
> +
> +		if (cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK)
> +			secocec_rx_done(cec->cec_adap, cec_val);
> +
> +		if (cec_val & SECOCEC_STATUS_MSG_SENT_MASK)
> +			secocec_tx_done(cec->cec_adap, cec_val);
> +
> +		if ((~cec_val & SECOCEC_STATUS_MSG_SENT_MASK) &&
> +		    (~cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK))
> +			dev_warn(dev,
> +				 "Message not received or sent, but interrupt fired");

Use dev_warn_once

> +
> +		val = SECOCEC_STATUS_REG_1_CEC;
> +	}
> +
> +	if (status_val & SECOCEC_STATUS_REG_1_IR) {
> +		dev_dbg(dev, "IR RC5 Interrupt Caught");
> +		val |= SECOCEC_STATUS_REG_1_IR;
> +		/* TODO IRDA RX */
> +	}
> +
> +	/*  Reset status register */
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	return IRQ_HANDLED;
> +
> +err:
> +	dev_err(dev, "IRQ: Read/Write SMBus operation failed (%d)", status);

dev_err_once

This really shouldn't happen, but if it happens, then you don't want
this flooding the log.

> +
> +	/*  Reset status register */
> +	val = SECOCEC_STATUS_REG_1_CEC | SECOCEC_STATUS_REG_1_IR;
> +	smb_wr16(SECOCEC_STATUS_REG_1, val);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +struct cec_dmi_match {
> +	char *sys_vendor;
> +	char *product_name;
> +	char *devname;
> +	char *conn;
> +};
> +
> +static const struct cec_dmi_match secocec_dmi_match_table[] = {
> +	/* UDOO X86 */
> +	{ "SECO", "UDOO x86", "0000:00:02.0", "Port B" },
> +};
> +
> +static int secocec_cec_get_notifier(struct cec_notifier **notify)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < ARRAY_SIZE(secocec_dmi_match_table) ; ++i) {
> +		const struct cec_dmi_match *m = &secocec_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;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int secocec_acpi_probe(struct secocec_data *sdev)
> +{
> +	struct device *dev = sdev->dev;
> +	struct gpio_desc *gpio;
> +	int irq = 0;
> +
> +	gpio = devm_gpiod_get(dev, NULL, GPIOF_IN);
> +	if (IS_ERR(gpio)) {
> +		dev_err(dev, "Cannot request interrupt gpio");
> +		return PTR_ERR(gpio);
> +	}
> +
> +	irq = gpiod_to_irq(gpio);
> +	if (irq < 0) {
> +		dev_err(dev, "Cannot find valid irq");
> +		return -ENODEV;
> +	}
> +	dev_dbg(dev, "irq-gpio is bound to IRQ %d", irq);
> +
> +	sdev->irq = irq;
> +
> +	return 0;
> +}
> +
> +static int secocec_probe(struct platform_device *pdev)
> +{
> +	struct secocec_data *secocec;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +	u16 val;
> +
> +	secocec = devm_kzalloc(dev, sizeof(*secocec), GFP_KERNEL);
> +	if (!secocec)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, secocec);
> +
> +	/* Request SMBus regions */
> +	if (!request_muxed_region(BRA_SMB_BASE_ADDR, 7, "CEC00001")) {
> +		dev_err(dev, "Request memory region failed");
> +		return -ENXIO;
> +	}
> +
> +	secocec->pdev = pdev;
> +	secocec->dev = dev;
> +
> +	if (!has_acpi_companion(dev)) {
> +		dev_dbg(dev, "Cannot find any ACPI companion");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	ret = secocec_acpi_probe(secocec);
> +	if (ret) {
> +		dev_err(dev, "Cannot assign gpio to IRQ");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	dev_dbg(dev, "IRQ detected at %d", secocec->irq);
> +
> +	/* Firmware version check */
> +	ret = smb_rd16(SECOCEC_VERSION, &val);
> +	if (ret) {
> +		dev_err(dev, "Cannot check fw version");
> +		goto err;
> +	}
> +	if (val < SECOCEC_LATEST_FW) {
> +		dev_err(dev, "CEC Firmware not supported (v.%04x). Use ver > v.%04x",
> +			val, SECOCEC_LATEST_FW);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = secocec_cec_get_notifier(&secocec->notifier);
> +	if (ret) {
> +		dev_err(dev, "no CEC notifier available\n");
> +		goto err;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev,
> +					secocec->irq,
> +					NULL,
> +					secocec_irq_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					dev_name(&pdev->dev), secocec);
> +
> +	if (ret) {
> +		dev_err(dev, "Cannot request IRQ %d", secocec->irq);
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	/* Allocate CEC adapter */
> +	secocec->cec_adap = cec_allocate_adapter(&secocec_cec_adap_ops,
> +						 secocec,
> +						 dev_name(dev),
> +						 CEC_CAP_DEFAULTS,
> +						 SECOCEC_MAX_ADDRS);
> +
> +	if (IS_ERR(secocec->cec_adap)) {
> +		ret = PTR_ERR(secocec->cec_adap);
> +		goto err;
> +	}
> +
> +	ret = cec_register_adapter(secocec->cec_adap, dev);
> +	if (ret)
> +		goto err_delete_adapter;
> +
> +	if (secocec->notifier)
> +		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
> +
> +	platform_set_drvdata(pdev, secocec);
> +
> +	dev_dbg(dev, "Device registered");
> +
> +	return ret;
> +
> +err_delete_adapter:
> +	cec_delete_adapter(secocec->cec_adap);
> +err:
> +	dev_err(dev, "%s device probe failed\n", dev_name(dev));
> +
> +	return ret;
> +}
> +
> +static int secocec_remove(struct platform_device *pdev)
> +{
> +	struct secocec_data *secocec = platform_get_drvdata(pdev);
> +
> +	cec_unregister_adapter(secocec->cec_adap);
> +
> +	if (secocec->notifier)
> +		cec_notifier_put(secocec->notifier);
> +
> +	release_region(BRA_SMB_BASE_ADDR, 7);
> +
> +	dev_dbg(&pdev->dev, "CEC device removed");
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int secocec_suspend(struct device *dev)
> +{
> +	int status;
> +	u16 val;
> +
> +	dev_dbg(dev, "Device going to suspend, disabling");
> +
> +	/* Clear the status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	/* Disable the interrupts */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> +			  ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
> +	if (status)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Suspend failed (err: %d)", status);
> +	return status;
> +}
> +
> +static int secocec_resume(struct device *dev)
> +{
> +	int status;
> +	u16 val;
> +
> +	dev_dbg(dev, "Resuming device from suspend");
> +
> +	/* Clear the status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	/* Enable the interrupts */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		goto err;
> +
> +	dev_dbg(dev, "Device resumed from suspend");
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Resume failed (err: %d)", status);
> +	return status;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
> +#define SECOCEC_PM_OPS (&secocec_pm_ops)
> +#else
> +#define SECOCEC_PM_OPS NULL
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id secocec_acpi_match[] = {
> +	{"CEC00001", 0},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
> +#endif
> +
> +static struct platform_driver secocec_driver = {
> +	.driver = {
> +		   .name = SECOCEC_DEV_NAME,
> +		   .acpi_match_table = ACPI_PTR(secocec_acpi_match),
> +		   .pm = SECOCEC_PM_OPS,
> +	},
> +	.probe = secocec_probe,
> +	.remove = secocec_remove,
> +};
> +
> +module_platform_driver(secocec_driver);
> +
> +MODULE_DESCRIPTION("SECO CEC X86 Driver");
> +MODULE_AUTHOR("Ettore Chimenti <ek5.chimenti@gmail.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
> new file mode 100644
> index 000000000000..93020900935e
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/seco-cec.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/*
> + * SECO X86 Boards CEC register defines
> + *
> + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> + * Copyright (C) 2018, SECO Srl.
> + * Copyright (C) 2018, Aidilab Srl.
> + */
> +
> +#ifndef __SECO_CEC_H__
> +#define __SECO_CEC_H__
> +
> +#define SECOCEC_MAX_ADDRS		1
> +#define SECOCEC_DEV_NAME		"secocec"
> +#define SECOCEC_LATEST_FW		0x0f0b
> +
> +#define SMBTIMEOUT			0xffff
> +#define SMB_POLL_UDELAY			10
> +
> +#define SMBUS_WRITE			0
> +#define SMBUS_READ			1
> +
> +#define CMD_BYTE_DATA			0
> +#define CMD_WORD_DATA			1
> +
> +/*
> + * SMBus definitons for Braswell
> + */
> +
> +#define BRA_DONE_STATUS			BIT(7)
> +#define BRA_INUSE_STS			BIT(6)
> +#define BRA_FAILED_OP			BIT(4)
> +#define BRA_BUS_ERR			BIT(3)
> +#define BRA_DEV_ERR			BIT(2)
> +#define BRA_INTR			BIT(1)
> +#define BRA_HOST_BUSY			BIT(0)
> +#define BRA_HSTS_ERR_MASK   (BRA_FAILED_OP | BRA_BUS_ERR | BRA_DEV_ERR)
> +
> +#define BRA_PEC_EN			BIT(7)
> +#define BRA_START			BIT(6)
> +#define BRA_LAST__BYTE			BIT(5)
> +#define BRA_INTREN			BIT(0)
> +#define BRA_SMB_CMD			(7 << 2)
> +#define BRA_SMB_CMD_QUICK		(0 << 2)
> +#define BRA_SMB_CMD_BYTE		(1 << 2)
> +#define BRA_SMB_CMD_BYTE_DATA		(2 << 2)
> +#define BRA_SMB_CMD_WORD_DATA		(3 << 2)
> +#define BRA_SMB_CMD_PROCESS_CALL	(4 << 2)
> +#define BRA_SMB_CMD_BLOCK		(5 << 2)
> +#define BRA_SMB_CMD_I2CREAD		(6 << 2)
> +#define BRA_SMB_CMD_BLOCK_PROCESS	(7 << 2)
> +
> +#define BRA_SMB_BASE_ADDR  0x2040
> +#define HSTS               (BRA_SMB_BASE_ADDR + 0)
> +#define HCNT               (BRA_SMB_BASE_ADDR + 2)
> +#define HCMD               (BRA_SMB_BASE_ADDR + 3)
> +#define XMIT_SLVA          (BRA_SMB_BASE_ADDR + 4)
> +#define HDAT0              (BRA_SMB_BASE_ADDR + 5)
> +#define HDAT1              (BRA_SMB_BASE_ADDR + 6)
> +
> +/*
> + * Microcontroller Address
> + */
> +
> +#define SECOCEC_MICRO_ADDRESS		0x40
> +
> +/*
> + * STM32 SMBus Registers
> + */
> +
> +#define SECOCEC_VERSION			0x00
> +#define SECOCEC_ENABLE_REG_1		0x01
> +#define SECOCEC_ENABLE_REG_2		0x02
> +#define SECOCEC_STATUS_REG_1		0x03
> +#define SECOCEC_STATUS_REG_2		0x04
> +
> +#define SECOCEC_STATUS			0x28
> +#define SECOCEC_DEVICE_LA		0x29
> +#define SECOCEC_READ_OPERATION_ID	0x2a
> +#define SECOCEC_READ_DATA_LENGTH	0x2b
> +#define SECOCEC_READ_DATA_00		0x2c
> +#define SECOCEC_READ_DATA_02		0x2d
> +#define SECOCEC_READ_DATA_04		0x2e
> +#define SECOCEC_READ_DATA_06		0x2f
> +#define SECOCEC_READ_DATA_08		0x30
> +#define SECOCEC_READ_DATA_10		0x31
> +#define SECOCEC_READ_DATA_12		0x32
> +#define SECOCEC_READ_BYTE0		0x33
> +#define SECOCEC_WRITE_OPERATION_ID	0x34
> +#define SECOCEC_WRITE_DATA_LENGTH	0x35
> +#define SECOCEC_WRITE_DATA_00		0x36
> +#define SECOCEC_WRITE_DATA_02		0x37
> +#define SECOCEC_WRITE_DATA_04		0x38
> +#define SECOCEC_WRITE_DATA_06		0x39
> +#define SECOCEC_WRITE_DATA_08		0x3a
> +#define SECOCEC_WRITE_DATA_10		0x3b
> +#define SECOCEC_WRITE_DATA_12		0x3c
> +#define SECOCEC_WRITE_BYTE0		0x3d
> +
> +#define SECOCEC_IR_READ_DATA		0x3e
> +
> +/*
> + * Enabling register
> + */
> +
> +#define SECOCEC_ENABLE_REG_1_CEC		0x1000
> +#define SECOCEC_ENABLE_REG_1_IR			0x2000
> +#define SECOCEC_ENABLE_REG_1_IR_PASSTHROUGH	0x4000
> +
> +/*
> + * Status register
> + */
> +
> +#define SECOCEC_STATUS_REG_1_CEC	SECOCEC_ENABLE_REG_1_CEC
> +#define SECOCEC_STATUS_REG_1_IR		SECOCEC_ENABLE_REG_1_IR
> +#define SECOCEC_STATUS_REG_1_IR_PASSTHR	SECOCEC_ENABLE_REG_1_IR_PASSTHR
> +
> +/*
> + * Status data
> + */
> +
> +#define SECOCEC_STATUS_MSG_RECEIVED_MASK	BIT(0)
> +#define SECOCEC_STATUS_RX_ERROR_MASK		BIT(1)
> +#define SECOCEC_STATUS_MSG_SENT_MASK		BIT(2)
> +#define SECOCEC_STATUS_TX_ERROR_MASK		BIT(3)
> +
> +#define SECOCEC_STATUS_TX_NACK_ERROR		BIT(4)
> +#define SECOCEC_STATUS_RX_OVERFLOW_MASK		BIT(5)
> +
> +#endif /* __SECO_CEC_H__ */
> 

Regards,

	Hans

Re: [PATCH v3 1/2] media: add SECO cec driver

[ektor5]

Hi Hans,

On Thu, Oct 18, 2018 at 09:14:55AM +0200, Hans Verkuil wrote:
> Hi Ettore,
> 
> Just a few small things and it is ready to go:
> 
> On 10/17/2018 11:31 PM, ektor5 wrote:
> > From: Ettore Chimenti <ek5.chimenti@gmail.com>
> > 
> > This patch adds support to the CEC device implemented with a STM32
> > microcontroller in X86 SECO Boards, including UDOO X86.
> > 
> > The communication is achieved via Braswell integrated SMBus
> > (i2c-i801). The driver use direct access to the PCI addresses, due to
> > the limitations of the specific driver in presence of ACPI calls.
> > 
> > The basic functionalities are tested with success with cec-ctl and
> > cec-compliance.
> > 
> > Inspired by cros-ec-cec implementation, attaches to i915 driver
> > cec-notifier.
> > 
> > Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
> > ---
> >  MAINTAINERS                                |   6 +
> >  drivers/media/platform/Kconfig             |  12 +
> >  drivers/media/platform/Makefile            |   2 +
> >  drivers/media/platform/seco-cec/Makefile   |   1 +
> >  drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
> >  drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
> >  6 files changed, 850 insertions(+)
> >  create mode 100644 drivers/media/platform/seco-cec/Makefile
> >  create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
> >  create mode 100644 drivers/media/platform/seco-cec/seco-cec.h
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4ece30f15777..1062912a5ff4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12972,6 +12972,12 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
> >  S:	Maintained
> >  F:	drivers/mmc/host/sdricoh_cs.c
> >  
> > +SECO BOARDS CEC DRIVER
> > +M:	Ettore Chimenti <ek5.chimenti@gmail.com>
> > +S:	Maintained
> > +F:	drivers/media/platform/seco-cec/seco-cec.c
> > +F:	drivers/media/platform/seco-cec/seco-cec.h
> > +
> >  SECURE COMPUTING
> >  M:	Kees Cook <keescook@chromium.org>
> >  R:	Andy Lutomirski <luto@amacapital.net>
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 94c1fe0e9787..51cd1fd005e3 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -613,6 +613,18 @@ config VIDEO_TEGRA_HDMI_CEC
> >  	 The CEC bus is present in the HDMI connector and enables communication
> >  	 between compatible devices.
> >  
> > +config VIDEO_SECO_CEC
> > +	tristate "SECO Boards HDMI CEC driver"
> > +	depends on (X86 || IA64) || COMPILE_TEST
> > +	depends on PCI && DMI
> > +	select CEC_CORE
> > +	select CEC_NOTIFIER
> > +	help
> > +	  This is a driver for SECO Boards integrated CEC interface.
> > +	  Selecting it will enable support for this device.
> > +	  CEC bus is present in the HDMI connector and enables communication
> > +	  between compatible devices.
> > +
> >  endif #CEC_PLATFORM_DRIVERS
> >  
> >  menuconfig SDR_PLATFORM_DRIVERS
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 41322ab65802..5d2b06c4c68a 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -53,6 +53,8 @@ obj-$(CONFIG_VIDEO_TEGRA_HDMI_CEC)	+= tegra-cec/
> >  
> >  obj-y					+= stm32/
> >  
> > +obj-$(CONFIG_VIDEO_SECO_CEC)		+= seco-cec/
> > +
> >  obj-y					+= davinci/
> >  
> >  obj-$(CONFIG_VIDEO_SH_VOU)		+= sh_vou.o
> > diff --git a/drivers/media/platform/seco-cec/Makefile b/drivers/media/platform/seco-cec/Makefile
> > new file mode 100644
> > index 000000000000..09900b087d02
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += seco-cec.o
> > diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
> > new file mode 100644
> > index 000000000000..9ac9a1f36bc0
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/seco-cec.c
> > @@ -0,0 +1,699 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +/*
> > + * CEC driver for SECO X86 Boards
> > + *
> > + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> > + * Copyright (C) 2018, SECO Srl.
> > + * Copyright (C) 2018, Aidilab Srl.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmi.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* CEC Framework */
> > +#include <media/cec.h>
> > +
> > +#include "seco-cec.h"
> > +
> > +struct secocec_data {
> > +	struct device *dev;
> > +	struct platform_device *pdev;
> > +	struct cec_adapter *cec_adap;
> > +	struct cec_notifier *notifier;
> > +	int irq;
> > +};
> > +
> > +#define smb_wr16(cmd, data) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> > +					     cmd, data, SMBUS_WRITE, NULL)
> > +#define smb_rd16(cmd, res) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> > +				       cmd, 0, SMBUS_READ, res)
> > +
> > +static int smb_word_op(short data_format, u16 slave_addr, u8 cmd, u16 data,
> > +		       u8 operation, u16 *result)
> > +{
> > +	unsigned int count;
> > +	short _data_format;
> > +	int status = 0;
> > +
> > +	switch (data_format) {
> > +	case CMD_BYTE_DATA:
> > +		_data_format = BRA_SMB_CMD_BYTE_DATA;
> > +		break;
> > +	case CMD_WORD_DATA:
> > +		_data_format = BRA_SMB_CMD_WORD_DATA;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Active wait until ready */
> > +	for (count = 0; count <= SMBTIMEOUT; ++count) {
> > +		if (!(inb(HSTS) & BRA_INUSE_STS))
> > +			break;
> > +		udelay(SMB_POLL_UDELAY);
> > +	}
> > +
> > +	if (count > SMBTIMEOUT)
> > +		/* Reset the lock instead of failing */
> > +		outb(0xff, HSTS);
> > +
> > +	outb(0x00, HCNT);
> > +	outb((u8)(slave_addr & 0xfe) | operation, XMIT_SLVA);
> > +	outb(cmd, HCMD);
> > +	inb(HCNT);
> > +
> > +	if (operation == SMBUS_WRITE) {
> > +		outb((u8)data, HDAT0);
> > +		outb((u8)(data >> 8), HDAT1);
> > +	}
> > +
> > +	outb(BRA_START + _data_format, HCNT);
> > +
> > +	for (count = 0; count <= SMBTIMEOUT; count++) {
> > +		if (!(inb(HSTS) & BRA_HOST_BUSY))
> > +			break;
> > +		udelay(SMB_POLL_UDELAY);
> > +	}
> > +
> > +	if (count > SMBTIMEOUT) {
> > +		status = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	if (inb(HSTS) & BRA_HSTS_ERR_MASK) {
> > +		status = -EIO;
> > +		goto err;
> > +	}
> > +
> > +	if (operation == SMBUS_READ)
> > +		*result = ((inb(HDAT0) & 0xff) + ((inb(HDAT1) & 0xff) << 8));
> > +
> > +err:
> > +	outb(0xff, HSTS);
> > +	return status;
> > +}
> > +
> > +static int secocec_adap_enable(struct cec_adapter *adap, bool enable)
> > +{
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	u16 val = 0;
> > +	int status;
> > +
> > +	if (enable) {
> > +		/* Clear the status register */
> > +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +		if (status)
> > +			goto err;
> > +
> > +		/* Enable the interrupts */
> > +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +				  val | SECOCEC_ENABLE_REG_1_CEC);
> > +		if (status)
> > +			goto err;
> > +
> > +		dev_dbg(dev, "Device enabled");
> > +	} else {
> > +		/* Clear the status register */
> > +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +		if (status)
> > +			goto err;
> > +
> > +		/* Disable the interrupts */
> > +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> > +				  ~SECOCEC_ENABLE_REG_1_CEC &
> > +				  ~SECOCEC_ENABLE_REG_1_IR);
> > +		if (status)
> > +			goto err;
> > +
> > +		dev_dbg(dev, "Device disabled");
> > +	}
> > +
> > +	return 0;
> > +err:
> > +	dev_err(dev, "Adapter setup failed (%d)", status);
> 
> Drop the dev_dbg/dev_err lines: the cec module has enough debugging
> already.
> 
> > +	return status;
> 
> This should be:
> 
> 	return enable ? status : 0;
> 
> Disabling shouldn't return an error, it would result in a WARN from
> the cec framework.
> 
> In fact, I would drop all the error checks in the disable path since they
> are ignored anyway.

Ah, ok. I'll remove all goto in the disable part.

> 
> > +}
> > +
> > +static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > +{
> > +	u16 enable_val = 0;
> > +	int status;
> 
> If logical_addr == CEC_LOG_ADDR_INVALID, then this function must not return
> an error or the CEC framework will WARN about it.
> 
> > +
> > +	/* Disable device */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
> > +	if (status)
> > +		return status;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		return status;
> > +
> > +	/* Write logical address */
> > +	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);
> 
> So does writing CEC_LOG_ADDR_INVALID (0xff) invalidate all logical
> addresses? I see no special code for that. If that is indeed the
> case, then you should document this.

The micro can have only one LA at a time. 

In the micro datasheet there isn't any reference to an invalid addr, so
it shouldn't complain. It will just set LA = 0xf. Is this correct or it
should disable the device instead?

> 
> > +	if (status)
> > +		return status;
> > +
> > +	/* Re-enable device */
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		return status;
> > +
> > +	return 0;
> > +}
> > +
> > +static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > +				 u32 signal_free_time, struct cec_msg *msg)
> > +{
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	u16 payload_len, payload_id_len, destination, val = 0;
> > +	u8 *payload_msg;
> > +	int status;
> > +	u8 i;
> > +
> > +	/* Device msg len already accounts for header */
> > +	payload_id_len = msg->len - 1;
> > +
> > +	/* Send data length */
> > +	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Send Operation ID if present */
> > +	if (payload_id_len > 0) {
> > +		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
> > +		if (status)
> > +			goto err;
> > +	}
> > +	/* Send data if present */
> > +	if (payload_id_len > 1) {
> > +		/* Only data; */
> > +		payload_len = msg->len - 2;
> > +		payload_msg = &msg->msg[2];
> > +
> > +		/* Copy message into registers */
> > +		for (i = 0; i < payload_len; i += 2) {
> > +			/* hi byte */
> > +			val = payload_msg[i + 1] << 8;
> > +
> > +			/* lo byte */
> > +			val |= payload_msg[i];
> > +
> > +			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
> > +			if (status)
> > +				goto err;
> > +		}
> > +	}
> > +	/* Send msg source/destination and fire msg */
> > +	destination = msg->msg[0];
> > +	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
> > +	if (status)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Transmit failed (%d)", status);
> 
> You can drop this: the cec module has already debug code for that.
> 
> > +	return status;
> > +}
> > +
> > +static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)
> 
> Just return void since the return code is ignored anyway.

Ok, will drop those.

> 
> > +{
> > +	int status = 0;
> > +
> > +	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
> > +		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
> > +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
> > +			status = -EAGAIN;
> > +		} else {
> > +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
> > +			status = -EIO;
> > +		}
> > +	} else {
> > +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> > +	}
> > +
> > +	/* Reset status reg */
> > +	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
> > +		SECOCEC_STATUS_MSG_SENT_MASK |
> > +		SECOCEC_STATUS_TX_NACK_ERROR;
> > +	smb_wr16(SECOCEC_STATUS, status_val);
> > +
> > +	return status;
> > +}
> > +
> > +static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
> > +{
> 
> Ditto.
> 
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	struct cec_msg msg = { };
> > +	bool flag_overflow = false;
> > +	u8 payload_len, i = 0;
> > +	u8 *payload_msg;
> > +	u16 val = 0;
> > +	int status;
> > +
> > +	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
> > +		dev_warn(dev, "Received more than 16 bytes. Discarding");
> 
> Is it better to just receive the first 16 bytes?

In case of an overflow, it should discard the overflowing bytes, but I
don't have any equipment that can test this. (this device can send up to
16 bytes).

> 
> > +		flag_overflow = true;
> > +	}
> > +
> > +	if (status_val & SECOCEC_STATUS_RX_ERROR_MASK) {
> > +		dev_warn(dev, "Message received with errors. Discarding");
> > +		status = -EIO;
> > +		goto rxerr;
> > +	}
> > +
> > +	/* Read message length */
> > +	status = smb_rd16(SECOCEC_READ_DATA_LENGTH, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Device msg len already accounts for the header */
> > +	msg.len = min(val + 1, CEC_MAX_MSG_SIZE);
> > +
> > +	/* Read logical address */
> > +	status = smb_rd16(SECOCEC_READ_BYTE0, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* device stores source LA and destination */
> > +	msg.msg[0] = val;
> > +
> > +	/* Read operation ID */
> > +	status = smb_rd16(SECOCEC_READ_OPERATION_ID, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	msg.msg[1] = val;
> > +
> > +	/* Read data if present */
> > +	if (msg.len > 1) {
> > +		payload_len = msg.len - 2;
> > +		payload_msg = &msg.msg[2];
> > +
> > +		/* device stores 2 bytes in every 16-bit val */
> > +		for (i = 0; i < payload_len; i += 2) {
> > +			status = smb_rd16(SECOCEC_READ_DATA_00 + i / 2, &val);
> > +			if (status)
> > +				goto err;
> > +
> > +			/* low byte, skipping header */
> > +			payload_msg[i] = val & 0x00ff;
> > +
> > +			/* hi byte */
> > +			payload_msg[i + 1] = (val & 0xff00) >> 8;
> > +		}
> > +	}
> > +
> > +	cec_received_msg(cec->cec_adap, &msg);
> > +
> > +	/* Reset status reg */
> > +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK;
> > +	if (flag_overflow)
> > +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS, status_val);
> > +	if (status)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +rxerr:
> > +	/* Reset error reg */
> > +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK |
> > +		SECOCEC_STATUS_RX_ERROR_MASK;
> > +	if (flag_overflow)
> > +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> > +	smb_wr16(SECOCEC_STATUS, status_val);
> > +
> > +err:
> > +	dev_err(dev, "Receive message failed (%d)", status);
> > +	return status;
> > +}
> > +
> > +struct cec_adap_ops secocec_cec_adap_ops = {
> > +	/* Low-level callbacks */
> > +	.adap_enable = secocec_adap_enable,
> > +	.adap_log_addr = secocec_adap_log_addr,
> > +	.adap_transmit = secocec_adap_transmit,
> > +};
> > +
> > +static irqreturn_t secocec_irq_handler(int irq, void *priv)
> > +{
> > +	struct secocec_data *cec = priv;
> > +	struct device *dev = cec->dev;
> > +	u16 status_val, cec_val, val = 0;
> > +	int status;
> > +
> > +	/*  Read status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &status_val);
> > +	if (status)
> > +		goto err;
> > +
> > +	if (status_val & SECOCEC_STATUS_REG_1_CEC) {
> > +		/* Read CEC status register */
> > +		status = smb_rd16(SECOCEC_STATUS, &cec_val);
> > +		if (status)
> > +			goto err;
> > +
> > +		if (cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK)
> > +			secocec_rx_done(cec->cec_adap, cec_val);
> > +
> > +		if (cec_val & SECOCEC_STATUS_MSG_SENT_MASK)
> > +			secocec_tx_done(cec->cec_adap, cec_val);
> > +
> > +		if ((~cec_val & SECOCEC_STATUS_MSG_SENT_MASK) &&
> > +		    (~cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK))
> > +			dev_warn(dev,
> > +				 "Message not received or sent, but interrupt fired");
> 
> Use dev_warn_once
> 
> > +
> > +		val = SECOCEC_STATUS_REG_1_CEC;
> > +	}
> > +
> > +	if (status_val & SECOCEC_STATUS_REG_1_IR) {
> > +		dev_dbg(dev, "IR RC5 Interrupt Caught");
> > +		val |= SECOCEC_STATUS_REG_1_IR;
> > +		/* TODO IRDA RX */
> > +	}
> > +
> > +	/*  Reset status register */
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	return IRQ_HANDLED;
> > +
> > +err:
> > +	dev_err(dev, "IRQ: Read/Write SMBus operation failed (%d)", status);
> 
> dev_err_once
> 
> This really shouldn't happen, but if it happens, then you don't want
> this flooding the log.

I didn't know this function, will use more often for sure.

> 
> > +
> > +	/*  Reset status register */
> > +	val = SECOCEC_STATUS_REG_1_CEC | SECOCEC_STATUS_REG_1_IR;
> > +	smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +struct cec_dmi_match {
> > +	char *sys_vendor;
> > +	char *product_name;
> > +	char *devname;
> > +	char *conn;
> > +};
> > +
> > +static const struct cec_dmi_match secocec_dmi_match_table[] = {
> > +	/* UDOO X86 */
> > +	{ "SECO", "UDOO x86", "0000:00:02.0", "Port B" },
> > +};
> > +
> > +static int secocec_cec_get_notifier(struct cec_notifier **notify)
> > +{
> > +	int i;
> > +
> > +	for (i = 0 ; i < ARRAY_SIZE(secocec_dmi_match_table) ; ++i) {
> > +		const struct cec_dmi_match *m = &secocec_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;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int secocec_acpi_probe(struct secocec_data *sdev)
> > +{
> > +	struct device *dev = sdev->dev;
> > +	struct gpio_desc *gpio;
> > +	int irq = 0;
> > +
> > +	gpio = devm_gpiod_get(dev, NULL, GPIOF_IN);
> > +	if (IS_ERR(gpio)) {
> > +		dev_err(dev, "Cannot request interrupt gpio");
> > +		return PTR_ERR(gpio);
> > +	}
> > +
> > +	irq = gpiod_to_irq(gpio);
> > +	if (irq < 0) {
> > +		dev_err(dev, "Cannot find valid irq");
> > +		return -ENODEV;
> > +	}
> > +	dev_dbg(dev, "irq-gpio is bound to IRQ %d", irq);
> > +
> > +	sdev->irq = irq;
> > +
> > +	return 0;
> > +}
> > +
> > +static int secocec_probe(struct platform_device *pdev)
> > +{
> > +	struct secocec_data *secocec;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +	u16 val;
> > +
> > +	secocec = devm_kzalloc(dev, sizeof(*secocec), GFP_KERNEL);
> > +	if (!secocec)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, secocec);
> > +
> > +	/* Request SMBus regions */
> > +	if (!request_muxed_region(BRA_SMB_BASE_ADDR, 7, "CEC00001")) {
> > +		dev_err(dev, "Request memory region failed");
> > +		return -ENXIO;
> > +	}
> > +
> > +	secocec->pdev = pdev;
> > +	secocec->dev = dev;
> > +
> > +	if (!has_acpi_companion(dev)) {
> > +		dev_dbg(dev, "Cannot find any ACPI companion");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	ret = secocec_acpi_probe(secocec);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot assign gpio to IRQ");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	dev_dbg(dev, "IRQ detected at %d", secocec->irq);
> > +
> > +	/* Firmware version check */
> > +	ret = smb_rd16(SECOCEC_VERSION, &val);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot check fw version");
> > +		goto err;
> > +	}
> > +	if (val < SECOCEC_LATEST_FW) {
> > +		dev_err(dev, "CEC Firmware not supported (v.%04x). Use ver > v.%04x",
> > +			val, SECOCEC_LATEST_FW);
> > +		ret = -EINVAL;
> > +		goto err;
> > +	}
> > +
> > +	ret = secocec_cec_get_notifier(&secocec->notifier);
> > +	if (ret) {
> > +		dev_err(dev, "no CEC notifier available\n");
> > +		goto err;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(dev,
> > +					secocec->irq,
> > +					NULL,
> > +					secocec_irq_handler,
> > +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +					dev_name(&pdev->dev), secocec);
> > +
> > +	if (ret) {
> > +		dev_err(dev, "Cannot request IRQ %d", secocec->irq);
> > +		ret = -EIO;
> > +		goto err;
> > +	}
> > +
> > +	/* Allocate CEC adapter */
> > +	secocec->cec_adap = cec_allocate_adapter(&secocec_cec_adap_ops,
> > +						 secocec,
> > +						 dev_name(dev),
> > +						 CEC_CAP_DEFAULTS,
> > +						 SECOCEC_MAX_ADDRS);
> > +
> > +	if (IS_ERR(secocec->cec_adap)) {
> > +		ret = PTR_ERR(secocec->cec_adap);
> > +		goto err;
> > +	}
> > +
> > +	ret = cec_register_adapter(secocec->cec_adap, dev);
> > +	if (ret)
> > +		goto err_delete_adapter;
> > +
> > +	if (secocec->notifier)
> > +		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
> > +
> > +	platform_set_drvdata(pdev, secocec);
> > +
> > +	dev_dbg(dev, "Device registered");
> > +
> > +	return ret;
> > +
> > +err_delete_adapter:
> > +	cec_delete_adapter(secocec->cec_adap);
> > +err:
> > +	dev_err(dev, "%s device probe failed\n", dev_name(dev));
> > +
> > +	return ret;
> > +}
> > +
> > +static int secocec_remove(struct platform_device *pdev)
> > +{
> > +	struct secocec_data *secocec = platform_get_drvdata(pdev);
> > +
> > +	cec_unregister_adapter(secocec->cec_adap);
> > +
> > +	if (secocec->notifier)
> > +		cec_notifier_put(secocec->notifier);
> > +
> > +	release_region(BRA_SMB_BASE_ADDR, 7);
> > +
> > +	dev_dbg(&pdev->dev, "CEC device removed");
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int secocec_suspend(struct device *dev)
> > +{
> > +	int status;
> > +	u16 val;
> > +
> > +	dev_dbg(dev, "Device going to suspend, disabling");
> > +
> > +	/* Clear the status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Disable the interrupts */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> > +			  ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
> > +	if (status)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Suspend failed (err: %d)", status);
> > +	return status;
> > +}
> > +
> > +static int secocec_resume(struct device *dev)
> > +{
> > +	int status;
> > +	u16 val;
> > +
> > +	dev_dbg(dev, "Resuming device from suspend");
> > +
> > +	/* Clear the status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Enable the interrupts */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		goto err;
> > +
> > +	dev_dbg(dev, "Device resumed from suspend");
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Resume failed (err: %d)", status);
> > +	return status;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
> > +#define SECOCEC_PM_OPS (&secocec_pm_ops)
> > +#else
> > +#define SECOCEC_PM_OPS NULL
> > +#endif
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id secocec_acpi_match[] = {
> > +	{"CEC00001", 0},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
> > +#endif
> > +
> > +static struct platform_driver secocec_driver = {
> > +	.driver = {
> > +		   .name = SECOCEC_DEV_NAME,
> > +		   .acpi_match_table = ACPI_PTR(secocec_acpi_match),
> > +		   .pm = SECOCEC_PM_OPS,
> > +	},
> > +	.probe = secocec_probe,
> > +	.remove = secocec_remove,
> > +};
> > +
> > +module_platform_driver(secocec_driver);
> > +
> > +MODULE_DESCRIPTION("SECO CEC X86 Driver");
> > +MODULE_AUTHOR("Ettore Chimenti <ek5.chimenti@gmail.com>");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
> > new file mode 100644
> > index 000000000000..93020900935e
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/seco-cec.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> > +/*
> > + * SECO X86 Boards CEC register defines
> > + *
> > + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> > + * Copyright (C) 2018, SECO Srl.
> > + * Copyright (C) 2018, Aidilab Srl.
> > + */
> > +
> > +#ifndef __SECO_CEC_H__
> > +#define __SECO_CEC_H__
> > +
> > +#define SECOCEC_MAX_ADDRS		1
> > +#define SECOCEC_DEV_NAME		"secocec"
> > +#define SECOCEC_LATEST_FW		0x0f0b
> > +
> > +#define SMBTIMEOUT			0xffff
> > +#define SMB_POLL_UDELAY			10
> > +
> > +#define SMBUS_WRITE			0
> > +#define SMBUS_READ			1
> > +
> > +#define CMD_BYTE_DATA			0
> > +#define CMD_WORD_DATA			1
> > +
> > +/*
> > + * SMBus definitons for Braswell
> > + */
> > +
> > +#define BRA_DONE_STATUS			BIT(7)
> > +#define BRA_INUSE_STS			BIT(6)
> > +#define BRA_FAILED_OP			BIT(4)
> > +#define BRA_BUS_ERR			BIT(3)
> > +#define BRA_DEV_ERR			BIT(2)
> > +#define BRA_INTR			BIT(1)
> > +#define BRA_HOST_BUSY			BIT(0)
> > +#define BRA_HSTS_ERR_MASK   (BRA_FAILED_OP | BRA_BUS_ERR | BRA_DEV_ERR)
> > +
> > +#define BRA_PEC_EN			BIT(7)
> > +#define BRA_START			BIT(6)
> > +#define BRA_LAST__BYTE			BIT(5)
> > +#define BRA_INTREN			BIT(0)
> > +#define BRA_SMB_CMD			(7 << 2)
> > +#define BRA_SMB_CMD_QUICK		(0 << 2)
> > +#define BRA_SMB_CMD_BYTE		(1 << 2)
> > +#define BRA_SMB_CMD_BYTE_DATA		(2 << 2)
> > +#define BRA_SMB_CMD_WORD_DATA		(3 << 2)
> > +#define BRA_SMB_CMD_PROCESS_CALL	(4 << 2)
> > +#define BRA_SMB_CMD_BLOCK		(5 << 2)
> > +#define BRA_SMB_CMD_I2CREAD		(6 << 2)
> > +#define BRA_SMB_CMD_BLOCK_PROCESS	(7 << 2)
> > +
> > +#define BRA_SMB_BASE_ADDR  0x2040
> > +#define HSTS               (BRA_SMB_BASE_ADDR + 0)
> > +#define HCNT               (BRA_SMB_BASE_ADDR + 2)
> > +#define HCMD               (BRA_SMB_BASE_ADDR + 3)
> > +#define XMIT_SLVA          (BRA_SMB_BASE_ADDR + 4)
> > +#define HDAT0              (BRA_SMB_BASE_ADDR + 5)
> > +#define HDAT1              (BRA_SMB_BASE_ADDR + 6)
> > +
> > +/*
> > + * Microcontroller Address
> > + */
> > +
> > +#define SECOCEC_MICRO_ADDRESS		0x40
> > +
> > +/*
> > + * STM32 SMBus Registers
> > + */
> > +
> > +#define SECOCEC_VERSION			0x00
> > +#define SECOCEC_ENABLE_REG_1		0x01
> > +#define SECOCEC_ENABLE_REG_2		0x02
> > +#define SECOCEC_STATUS_REG_1		0x03
> > +#define SECOCEC_STATUS_REG_2		0x04
> > +
> > +#define SECOCEC_STATUS			0x28
> > +#define SECOCEC_DEVICE_LA		0x29
> > +#define SECOCEC_READ_OPERATION_ID	0x2a
> > +#define SECOCEC_READ_DATA_LENGTH	0x2b
> > +#define SECOCEC_READ_DATA_00		0x2c
> > +#define SECOCEC_READ_DATA_02		0x2d
> > +#define SECOCEC_READ_DATA_04		0x2e
> > +#define SECOCEC_READ_DATA_06		0x2f
> > +#define SECOCEC_READ_DATA_08		0x30
> > +#define SECOCEC_READ_DATA_10		0x31
> > +#define SECOCEC_READ_DATA_12		0x32
> > +#define SECOCEC_READ_BYTE0		0x33
> > +#define SECOCEC_WRITE_OPERATION_ID	0x34
> > +#define SECOCEC_WRITE_DATA_LENGTH	0x35
> > +#define SECOCEC_WRITE_DATA_00		0x36
> > +#define SECOCEC_WRITE_DATA_02		0x37
> > +#define SECOCEC_WRITE_DATA_04		0x38
> > +#define SECOCEC_WRITE_DATA_06		0x39
> > +#define SECOCEC_WRITE_DATA_08		0x3a
> > +#define SECOCEC_WRITE_DATA_10		0x3b
> > +#define SECOCEC_WRITE_DATA_12		0x3c
> > +#define SECOCEC_WRITE_BYTE0		0x3d
> > +
> > +#define SECOCEC_IR_READ_DATA		0x3e
> > +
> > +/*
> > + * Enabling register
> > + */
> > +
> > +#define SECOCEC_ENABLE_REG_1_CEC		0x1000
> > +#define SECOCEC_ENABLE_REG_1_IR			0x2000
> > +#define SECOCEC_ENABLE_REG_1_IR_PASSTHROUGH	0x4000
> > +
> > +/*
> > + * Status register
> > + */
> > +
> > +#define SECOCEC_STATUS_REG_1_CEC	SECOCEC_ENABLE_REG_1_CEC
> > +#define SECOCEC_STATUS_REG_1_IR		SECOCEC_ENABLE_REG_1_IR
> > +#define SECOCEC_STATUS_REG_1_IR_PASSTHR	SECOCEC_ENABLE_REG_1_IR_PASSTHR
> > +
> > +/*
> > + * Status data
> > + */
> > +
> > +#define SECOCEC_STATUS_MSG_RECEIVED_MASK	BIT(0)
> > +#define SECOCEC_STATUS_RX_ERROR_MASK		BIT(1)
> > +#define SECOCEC_STATUS_MSG_SENT_MASK		BIT(2)
> > +#define SECOCEC_STATUS_TX_ERROR_MASK		BIT(3)
> > +
> > +#define SECOCEC_STATUS_TX_NACK_ERROR		BIT(4)
> > +#define SECOCEC_STATUS_RX_OVERFLOW_MASK		BIT(5)
> > +
> > +#endif /* __SECO_CEC_H__ */
> > 

Thanks,
	Ettore

> 
> Regards,
> 
> 	Hans

Re: [PATCH v3 1/2] media: add SECO cec driver

[Hans Verkuil]

On 10/19/18 15:02, ektor5 wrote:
> Hi Hans,
> 
> On Thu, Oct 18, 2018 at 09:14:55AM +0200, Hans Verkuil wrote:
>> Hi Ettore,
>>
>> Just a few small things and it is ready to go:
>>
>> On 10/17/2018 11:31 PM, ektor5 wrote:
>>> From: Ettore Chimenti <ek5.chimenti@gmail.com>
>>>
>>> This patch adds support to the CEC device implemented with a STM32
>>> microcontroller in X86 SECO Boards, including UDOO X86.
>>>
>>> The communication is achieved via Braswell integrated SMBus
>>> (i2c-i801). The driver use direct access to the PCI addresses, due to
>>> the limitations of the specific driver in presence of ACPI calls.
>>>
>>> The basic functionalities are tested with success with cec-ctl and
>>> cec-compliance.
>>>
>>> Inspired by cros-ec-cec implementation, attaches to i915 driver
>>> cec-notifier.
>>>
>>> Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
>>> ---
>>>  MAINTAINERS                                |   6 +
>>>  drivers/media/platform/Kconfig             |  12 +
>>>  drivers/media/platform/Makefile            |   2 +
>>>  drivers/media/platform/seco-cec/Makefile   |   1 +
>>>  drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
>>>  drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
>>>  6 files changed, 850 insertions(+)
>>>  create mode 100644 drivers/media/platform/seco-cec/Makefile
>>>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
>>>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.h

<snip>

>>> +static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
>>> +{
>>> +	u16 enable_val = 0;
>>> +	int status;
>>
>> If logical_addr == CEC_LOG_ADDR_INVALID, then this function must not return
>> an error or the CEC framework will WARN about it.
>>
>>> +
>>> +	/* Disable device */
>>> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
>>> +			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	/* Write logical address */
>>> +	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);
>>
>> So does writing CEC_LOG_ADDR_INVALID (0xff) invalidate all logical
>> addresses? I see no special code for that. If that is indeed the
>> case, then you should document this.
> 
> The micro can have only one LA at a time. 
> 
> In the micro datasheet there isn't any reference to an invalid addr, so
> it shouldn't complain. It will just set LA = 0xf. Is this correct or it
> should disable the device instead?

Setting it to 0xf is OK, but you might want to write 'logical_addr & 0xf'
to the register with a note that CEC_LOG_ADDR_INVALID is mapped to the
'Unregistered' logical address.

> 
>>
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	/* Re-enable device */
>>> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
>>> +			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
>>> +	if (status)
>>> +		return status;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> +				 u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>> +	struct secocec_data *cec = cec_get_drvdata(adap);
>>> +	struct device *dev = cec->dev;
>>> +	u16 payload_len, payload_id_len, destination, val = 0;
>>> +	u8 *payload_msg;
>>> +	int status;
>>> +	u8 i;
>>> +
>>> +	/* Device msg len already accounts for header */
>>> +	payload_id_len = msg->len - 1;
>>> +
>>> +	/* Send data length */
>>> +	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
>>> +	if (status)
>>> +		goto err;
>>> +
>>> +	/* Send Operation ID if present */
>>> +	if (payload_id_len > 0) {
>>> +		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
>>> +		if (status)
>>> +			goto err;
>>> +	}
>>> +	/* Send data if present */
>>> +	if (payload_id_len > 1) {
>>> +		/* Only data; */
>>> +		payload_len = msg->len - 2;
>>> +		payload_msg = &msg->msg[2];
>>> +
>>> +		/* Copy message into registers */
>>> +		for (i = 0; i < payload_len; i += 2) {
>>> +			/* hi byte */
>>> +			val = payload_msg[i + 1] << 8;
>>> +
>>> +			/* lo byte */
>>> +			val |= payload_msg[i];
>>> +
>>> +			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
>>> +			if (status)
>>> +				goto err;
>>> +		}
>>> +	}
>>> +	/* Send msg source/destination and fire msg */
>>> +	destination = msg->msg[0];
>>> +	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
>>> +	if (status)
>>> +		goto err;
>>> +
>>> +	return 0;
>>> +
>>> +err:
>>> +	dev_err(dev, "Transmit failed (%d)", status);
>>
>> You can drop this: the cec module has already debug code for that.
>>
>>> +	return status;
>>> +}
>>> +
>>> +static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)
>>
>> Just return void since the return code is ignored anyway.
> 
> Ok, will drop those.
> 
>>
>>> +{
>>> +	int status = 0;
>>> +
>>> +	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
>>> +		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
>>> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
>>> +			status = -EAGAIN;
>>> +		} else {
>>> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
>>> +			status = -EIO;
>>> +		}
>>> +	} else {
>>> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
>>> +	}
>>> +
>>> +	/* Reset status reg */
>>> +	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
>>> +		SECOCEC_STATUS_MSG_SENT_MASK |
>>> +		SECOCEC_STATUS_TX_NACK_ERROR;
>>> +	smb_wr16(SECOCEC_STATUS, status_val);
>>> +
>>> +	return status;
>>> +}
>>> +
>>> +static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
>>> +{
>>
>> Ditto.
>>
>>> +	struct secocec_data *cec = cec_get_drvdata(adap);
>>> +	struct device *dev = cec->dev;
>>> +	struct cec_msg msg = { };
>>> +	bool flag_overflow = false;
>>> +	u8 payload_len, i = 0;
>>> +	u8 *payload_msg;
>>> +	u16 val = 0;
>>> +	int status;
>>> +
>>> +	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
>>> +		dev_warn(dev, "Received more than 16 bytes. Discarding");
>>
>> Is it better to just receive the first 16 bytes?
> 
> In case of an overflow, it should discard the overflowing bytes, but I
> don't have any equipment that can test this. (this device can send up to
> 16 bytes).

Add a note that this might not be necessary, but that you couldn't test
this. I can test it when I find some time.

> 
>>
>>> +		flag_overflow = true;
>>> +	}

Regards,

	Hans

Re: [PATCH v3 1/2] media: add SECO cec driver

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 25935 bytes --]

Hi Ettore,
   thanks for the patches.

A few other things, please see below.

On Wed, Oct 17, 2018 at 11:31:41PM +0200, ektor5 wrote:
> From: Ettore Chimenti <ek5.chimenti@gmail.com>
>
> This patch adds support to the CEC device implemented with a STM32
> microcontroller in X86 SECO Boards, including UDOO X86.
>
> The communication is achieved via Braswell integrated SMBus
> (i2c-i801). The driver use direct access to the PCI addresses, due to
> the limitations of the specific driver in presence of ACPI calls.
>
> The basic functionalities are tested with success with cec-ctl and
> cec-compliance.
>
> Inspired by cros-ec-cec implementation, attaches to i915 driver
> cec-notifier.
>
> Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
> ---
>  MAINTAINERS                                |   6 +
>  drivers/media/platform/Kconfig             |  12 +
>  drivers/media/platform/Makefile            |   2 +
>  drivers/media/platform/seco-cec/Makefile   |   1 +
>  drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
>  drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
>  6 files changed, 850 insertions(+)
>  create mode 100644 drivers/media/platform/seco-cec/Makefile
>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
>  create mode 100644 drivers/media/platform/seco-cec/seco-cec.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4ece30f15777..1062912a5ff4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12972,6 +12972,12 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
>  S:	Maintained
>  F:	drivers/mmc/host/sdricoh_cs.c
>
> +SECO BOARDS CEC DRIVER
> +M:	Ettore Chimenti <ek5.chimenti@gmail.com>
> +S:	Maintained
> +F:	drivers/media/platform/seco-cec/seco-cec.c
> +F:	drivers/media/platform/seco-cec/seco-cec.h
> +
>  SECURE COMPUTING
>  M:	Kees Cook <keescook@chromium.org>
>  R:	Andy Lutomirski <luto@amacapital.net>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 94c1fe0e9787..51cd1fd005e3 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -613,6 +613,18 @@ config VIDEO_TEGRA_HDMI_CEC
>  	 The CEC bus is present in the HDMI connector and enables communication
>  	 between compatible devices.
>
> +config VIDEO_SECO_CEC
> +	tristate "SECO Boards HDMI CEC driver"
> +	depends on (X86 || IA64) || COMPILE_TEST
> +	depends on PCI && DMI
> +	select CEC_CORE
> +	select CEC_NOTIFIER
> +	help
> +	  This is a driver for SECO Boards integrated CEC interface.
> +	  Selecting it will enable support for this device.
> +	  CEC bus is present in the HDMI connector and enables communication
> +	  between compatible devices.
> +
>  endif #CEC_PLATFORM_DRIVERS
>
>  menuconfig SDR_PLATFORM_DRIVERS
> diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> index 41322ab65802..5d2b06c4c68a 100644
> --- a/drivers/media/platform/Makefile
> +++ b/drivers/media/platform/Makefile
> @@ -53,6 +53,8 @@ obj-$(CONFIG_VIDEO_TEGRA_HDMI_CEC)	+= tegra-cec/
>
>  obj-y					+= stm32/
>
> +obj-$(CONFIG_VIDEO_SECO_CEC)		+= seco-cec/
> +
>  obj-y					+= davinci/
>
>  obj-$(CONFIG_VIDEO_SH_VOU)		+= sh_vou.o
> diff --git a/drivers/media/platform/seco-cec/Makefile b/drivers/media/platform/seco-cec/Makefile
> new file mode 100644
> index 000000000000..09900b087d02
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/Makefile
> @@ -0,0 +1 @@
> +obj-y += seco-cec.o
> diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
> new file mode 100644
> index 000000000000..9ac9a1f36bc0
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/seco-cec.c
> @@ -0,0 +1,699 @@
> +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> +/*
> + * CEC driver for SECO X86 Boards
> + *
> + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> + * Copyright (C) 2018, SECO Srl.
> + * Copyright (C) 2018, Aidilab Srl.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/dmi.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +
> +/* CEC Framework */
> +#include <media/cec.h>
> +
> +#include "seco-cec.h"
> +
> +struct secocec_data {
> +	struct device *dev;
> +	struct platform_device *pdev;
> +	struct cec_adapter *cec_adap;
> +	struct cec_notifier *notifier;
> +	int irq;
> +};
> +
> +#define smb_wr16(cmd, data) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> +					     cmd, data, SMBUS_WRITE, NULL)
> +#define smb_rd16(cmd, res) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> +				       cmd, 0, SMBUS_READ, res)
> +
> +static int smb_word_op(short data_format, u16 slave_addr, u8 cmd, u16 data,
> +		       u8 operation, u16 *result)
> +{
> +	unsigned int count;
> +	short _data_format;
> +	int status = 0;
> +
> +	switch (data_format) {
> +	case CMD_BYTE_DATA:
> +		_data_format = BRA_SMB_CMD_BYTE_DATA;
> +		break;
> +	case CMD_WORD_DATA:
> +		_data_format = BRA_SMB_CMD_WORD_DATA;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	/* Active wait until ready */
> +	for (count = 0; count <= SMBTIMEOUT; ++count) {
> +		if (!(inb(HSTS) & BRA_INUSE_STS))
> +			break;
> +		udelay(SMB_POLL_UDELAY);
> +	}

Do you have any characterization for this?
You are waiting up to 650msec, isn't it very long?

> +
> +	if (count > SMBTIMEOUT)
> +		/* Reset the lock instead of failing */
> +		outb(0xff, HSTS);
> +
> +	outb(0x00, HCNT);
> +	outb((u8)(slave_addr & 0xfe) | operation, XMIT_SLVA);
> +	outb(cmd, HCMD);
> +	inb(HCNT);
> +
> +	if (operation == SMBUS_WRITE) {
> +		outb((u8)data, HDAT0);
> +		outb((u8)(data >> 8), HDAT1);
> +	}
> +
> +	outb(BRA_START + _data_format, HCNT);
> +
> +	for (count = 0; count <= SMBTIMEOUT; count++) {
> +		if (!(inb(HSTS) & BRA_HOST_BUSY))
> +			break;
> +		udelay(SMB_POLL_UDELAY);
> +	}
> +
> +	if (count > SMBTIMEOUT) {
> +		status = -EBUSY;
> +		goto err;
> +	}
> +
> +	if (inb(HSTS) & BRA_HSTS_ERR_MASK) {
> +		status = -EIO;
> +		goto err;
> +	}
> +
> +	if (operation == SMBUS_READ)
> +		*result = ((inb(HDAT0) & 0xff) + ((inb(HDAT1) & 0xff) << 8));
> +
> +err:
> +	outb(0xff, HSTS);
> +	return status;
> +}
> +
> +static int secocec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	u16 val = 0;
> +	int status;
> +
> +	if (enable) {
> +		/* Clear the status register */
> +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +		if (status)
> +			goto err;
> +
> +		/* Enable the interrupts */
> +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +				  val | SECOCEC_ENABLE_REG_1_CEC);
> +		if (status)
> +			goto err;
> +
> +		dev_dbg(dev, "Device enabled");
> +	} else {
> +		/* Clear the status register */
> +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +		if (status)
> +			goto err;
> +
> +		/* Disable the interrupts */
> +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +		if (status)
> +			goto err;
> +
> +		status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> +				  ~SECOCEC_ENABLE_REG_1_CEC &
> +				  ~SECOCEC_ENABLE_REG_1_IR);
> +		if (status)
> +			goto err;
> +
> +		dev_dbg(dev, "Device disabled");

I would drop these two, as they don't add any debug value, just tell
you everything went fine. But it's a nit, feel free to chose yourself.

> +	}
> +
> +	return 0;
> +err:
> +	dev_err(dev, "Adapter setup failed (%d)", status);
> +	return status;
> +}
> +
> +static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> +{
> +	u16 enable_val = 0;
> +	int status;
> +
> +	/* Disable device */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
> +	if (status)
> +		return status;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		return status;
> +
> +	/* Write logical address */
> +	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);
> +	if (status)
> +		return status;
> +
> +	/* Re-enable device */
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> +			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		return status;
> +
> +	return 0;
> +}
> +
> +static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> +				 u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	u16 payload_len, payload_id_len, destination, val = 0;
> +	u8 *payload_msg;
> +	int status;
> +	u8 i;
> +
> +	/* Device msg len already accounts for header */
> +	payload_id_len = msg->len - 1;
> +
> +	/* Send data length */
> +	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
> +	if (status)
> +		goto err;
> +
> +	/* Send Operation ID if present */
> +	if (payload_id_len > 0) {
> +		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
> +		if (status)
> +			goto err;
> +	}
> +	/* Send data if present */
> +	if (payload_id_len > 1) {
> +		/* Only data; */
> +		payload_len = msg->len - 2;
> +		payload_msg = &msg->msg[2];
> +
> +		/* Copy message into registers */
> +		for (i = 0; i < payload_len; i += 2) {
> +			/* hi byte */
> +			val = payload_msg[i + 1] << 8;
> +
> +			/* lo byte */
> +			val |= payload_msg[i];
> +
> +			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
> +			if (status)
> +				goto err;
> +		}
> +	}
> +	/* Send msg source/destination and fire msg */
> +	destination = msg->msg[0];
> +	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
> +	if (status)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Transmit failed (%d)", status);
> +	return status;
> +}
> +
> +static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)
> +{
> +	int status = 0;
> +
> +	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
> +		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
> +			status = -EAGAIN;
> +		} else {
> +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
> +			status = -EIO;
> +		}
> +	} else {
> +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> +	}
> +
> +	/* Reset status reg */
> +	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
> +		SECOCEC_STATUS_MSG_SENT_MASK |
> +		SECOCEC_STATUS_TX_NACK_ERROR;
> +	smb_wr16(SECOCEC_STATUS, status_val);
> +
> +	return status;
> +}
> +
> +static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
> +{
> +	struct secocec_data *cec = cec_get_drvdata(adap);
> +	struct device *dev = cec->dev;
> +	struct cec_msg msg = { };
> +	bool flag_overflow = false;
> +	u8 payload_len, i = 0;
> +	u8 *payload_msg;
> +	u16 val = 0;
> +	int status;
> +
> +	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
> +		dev_warn(dev, "Received more than 16 bytes. Discarding");
> +		flag_overflow = true;
> +	}
> +
> +	if (status_val & SECOCEC_STATUS_RX_ERROR_MASK) {
> +		dev_warn(dev, "Message received with errors. Discarding");
> +		status = -EIO;
> +		goto rxerr;
> +	}
> +
> +	/* Read message length */
> +	status = smb_rd16(SECOCEC_READ_DATA_LENGTH, &val);
> +	if (status)
> +		goto err;
> +
> +	/* Device msg len already accounts for the header */
> +	msg.len = min(val + 1, CEC_MAX_MSG_SIZE);
> +
> +	/* Read logical address */
> +	status = smb_rd16(SECOCEC_READ_BYTE0, &val);
> +	if (status)
> +		goto err;
> +
> +	/* device stores source LA and destination */
> +	msg.msg[0] = val;
> +
> +	/* Read operation ID */
> +	status = smb_rd16(SECOCEC_READ_OPERATION_ID, &val);
> +	if (status)
> +		goto err;
> +
> +	msg.msg[1] = val;
> +
> +	/* Read data if present */
> +	if (msg.len > 1) {
> +		payload_len = msg.len - 2;
> +		payload_msg = &msg.msg[2];
> +
> +		/* device stores 2 bytes in every 16-bit val */
> +		for (i = 0; i < payload_len; i += 2) {
> +			status = smb_rd16(SECOCEC_READ_DATA_00 + i / 2, &val);
> +			if (status)
> +				goto err;
> +
> +			/* low byte, skipping header */
> +			payload_msg[i] = val & 0x00ff;
> +
> +			/* hi byte */
> +			payload_msg[i + 1] = (val & 0xff00) >> 8;
> +		}
> +	}
> +
> +	cec_received_msg(cec->cec_adap, &msg);
> +
> +	/* Reset status reg */
> +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK;
> +	if (flag_overflow)
> +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> +
> +	status = smb_wr16(SECOCEC_STATUS, status_val);
> +	if (status)
> +		goto err;

If you fail here, you would not catch the error in the caller, as you
don't check for the return value, nor reset the
SECOCEC_STATUS_MSG_RECEIVED_MASK flag. Same for the tx function.

> +
> +	return 0;
> +
> +rxerr:
> +	/* Reset error reg */
> +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK |
> +		SECOCEC_STATUS_RX_ERROR_MASK;
> +	if (flag_overflow)
> +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> +	smb_wr16(SECOCEC_STATUS, status_val);
> +
> +err:
> +	dev_err(dev, "Receive message failed (%d)", status);
> +	return status;
> +}
> +
> +struct cec_adap_ops secocec_cec_adap_ops = {
> +	/* Low-level callbacks */
> +	.adap_enable = secocec_adap_enable,
> +	.adap_log_addr = secocec_adap_log_addr,
> +	.adap_transmit = secocec_adap_transmit,
> +};
> +
> +static irqreturn_t secocec_irq_handler(int irq, void *priv)
> +{
> +	struct secocec_data *cec = priv;
> +	struct device *dev = cec->dev;
> +	u16 status_val, cec_val, val = 0;
> +	int status;
> +
> +	/*  Read status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &status_val);
> +	if (status)
> +		goto err;
> +
> +	if (status_val & SECOCEC_STATUS_REG_1_CEC) {
> +		/* Read CEC status register */
> +		status = smb_rd16(SECOCEC_STATUS, &cec_val);
> +		if (status)
> +			goto err;
> +
> +		if (cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK)
> +			secocec_rx_done(cec->cec_adap, cec_val);
> +
> +		if (cec_val & SECOCEC_STATUS_MSG_SENT_MASK)
> +			secocec_tx_done(cec->cec_adap, cec_val);

Not checking return values of those two functions. Please see comment
in the rx_done function.

> +
> +		if ((~cec_val & SECOCEC_STATUS_MSG_SENT_MASK) &&
> +		    (~cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK))
> +			dev_warn(dev,
> +				 "Message not received or sent, but interrupt fired");
> +
> +		val = SECOCEC_STATUS_REG_1_CEC;
> +	}
> +
> +	if (status_val & SECOCEC_STATUS_REG_1_IR) {
> +		dev_dbg(dev, "IR RC5 Interrupt Caught");

I would drop any informative printouts from an irq routine.

> +		val |= SECOCEC_STATUS_REG_1_IR;
> +		/* TODO IRDA RX */
> +	}
> +
> +	/*  Reset status register */
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	return IRQ_HANDLED;
> +
> +err:
> +	dev_err(dev, "IRQ: Read/Write SMBus operation failed (%d)", status);
> +
> +	/*  Reset status register */
> +	val = SECOCEC_STATUS_REG_1_CEC | SECOCEC_STATUS_REG_1_IR;
> +	smb_wr16(SECOCEC_STATUS_REG_1, val);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +struct cec_dmi_match {
> +	char *sys_vendor;
> +	char *product_name;
> +	char *devname;
> +	char *conn;
> +};
> +
> +static const struct cec_dmi_match secocec_dmi_match_table[] = {
> +	/* UDOO X86 */
> +	{ "SECO", "UDOO x86", "0000:00:02.0", "Port B" },
> +};
> +
> +static int secocec_cec_get_notifier(struct cec_notifier **notify)
> +{
> +	int i;
> +
> +	for (i = 0 ; i < ARRAY_SIZE(secocec_dmi_match_table) ; ++i) {
> +		const struct cec_dmi_match *m = &secocec_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;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int secocec_acpi_probe(struct secocec_data *sdev)
> +{
> +	struct device *dev = sdev->dev;
> +	struct gpio_desc *gpio;
> +	int irq = 0;
> +
> +	gpio = devm_gpiod_get(dev, NULL, GPIOF_IN);
> +	if (IS_ERR(gpio)) {
> +		dev_err(dev, "Cannot request interrupt gpio");
> +		return PTR_ERR(gpio);
> +	}
> +
> +	irq = gpiod_to_irq(gpio);
> +	if (irq < 0) {
> +		dev_err(dev, "Cannot find valid irq");
> +		return -ENODEV;
> +	}
> +	dev_dbg(dev, "irq-gpio is bound to IRQ %d", irq);
> +
> +	sdev->irq = irq;
> +
> +	return 0;
> +}
> +
> +static int secocec_probe(struct platform_device *pdev)
> +{
> +	struct secocec_data *secocec;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +	u16 val;
> +
> +	secocec = devm_kzalloc(dev, sizeof(*secocec), GFP_KERNEL);
> +	if (!secocec)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(dev, secocec);
> +
> +	/* Request SMBus regions */
> +	if (!request_muxed_region(BRA_SMB_BASE_ADDR, 7, "CEC00001")) {
> +		dev_err(dev, "Request memory region failed");
> +		return -ENXIO;
> +	}
> +
> +	secocec->pdev = pdev;
> +	secocec->dev = dev;
> +
> +	if (!has_acpi_companion(dev)) {
> +		dev_dbg(dev, "Cannot find any ACPI companion");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	ret = secocec_acpi_probe(secocec);
> +	if (ret) {
> +		dev_err(dev, "Cannot assign gpio to IRQ");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	dev_dbg(dev, "IRQ detected at %d", secocec->irq);

You have just printed this out in secocec_acpi_probe()

Apart a few minors you can fix in next iteration, this looks good now.
Once fixed, please add my:

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

> +
> +	/* Firmware version check */
> +	ret = smb_rd16(SECOCEC_VERSION, &val);
> +	if (ret) {
> +		dev_err(dev, "Cannot check fw version");
> +		goto err;
> +	}
> +	if (val < SECOCEC_LATEST_FW) {
> +		dev_err(dev, "CEC Firmware not supported (v.%04x). Use ver > v.%04x",
> +			val, SECOCEC_LATEST_FW);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	ret = secocec_cec_get_notifier(&secocec->notifier);
> +	if (ret) {
> +		dev_err(dev, "no CEC notifier available\n");
> +		goto err;
> +	}
> +
> +	ret = devm_request_threaded_irq(dev,
> +					secocec->irq,
> +					NULL,
> +					secocec_irq_handler,
> +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					dev_name(&pdev->dev), secocec);
> +
> +	if (ret) {
> +		dev_err(dev, "Cannot request IRQ %d", secocec->irq);
> +		ret = -EIO;
> +		goto err;
> +	}
> +
> +	/* Allocate CEC adapter */
> +	secocec->cec_adap = cec_allocate_adapter(&secocec_cec_adap_ops,
> +						 secocec,
> +						 dev_name(dev),
> +						 CEC_CAP_DEFAULTS,
> +						 SECOCEC_MAX_ADDRS);
> +
> +	if (IS_ERR(secocec->cec_adap)) {
> +		ret = PTR_ERR(secocec->cec_adap);
> +		goto err;
> +	}
> +
> +	ret = cec_register_adapter(secocec->cec_adap, dev);
> +	if (ret)
> +		goto err_delete_adapter;
> +
> +	if (secocec->notifier)
> +		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
> +
> +	platform_set_drvdata(pdev, secocec);
> +
> +	dev_dbg(dev, "Device registered");
> +
> +	return ret;
> +
> +err_delete_adapter:
> +	cec_delete_adapter(secocec->cec_adap);
> +err:
> +	dev_err(dev, "%s device probe failed\n", dev_name(dev));
> +
> +	return ret;
> +}
> +
> +static int secocec_remove(struct platform_device *pdev)
> +{
> +	struct secocec_data *secocec = platform_get_drvdata(pdev);
> +
> +	cec_unregister_adapter(secocec->cec_adap);
> +
> +	if (secocec->notifier)
> +		cec_notifier_put(secocec->notifier);
> +
> +	release_region(BRA_SMB_BASE_ADDR, 7);
> +
> +	dev_dbg(&pdev->dev, "CEC device removed");
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int secocec_suspend(struct device *dev)
> +{
> +	int status;
> +	u16 val;
> +
> +	dev_dbg(dev, "Device going to suspend, disabling");
> +
> +	/* Clear the status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	/* Disable the interrupts */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> +			  ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
> +	if (status)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Suspend failed (err: %d)", status);
> +	return status;
> +}
> +
> +static int secocec_resume(struct device *dev)
> +{
> +	int status;
> +	u16 val;
> +
> +	dev_dbg(dev, "Resuming device from suspend");
> +
> +	/* Clear the status register */
> +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> +	if (status)
> +		goto err;
> +
> +	/* Enable the interrupts */
> +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> +	if (status)
> +		goto err;
> +
> +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
> +	if (status)
> +		goto err;
> +
> +	dev_dbg(dev, "Device resumed from suspend");
> +
> +	return 0;
> +
> +err:
> +	dev_err(dev, "Resume failed (err: %d)", status);
> +	return status;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
> +#define SECOCEC_PM_OPS (&secocec_pm_ops)
> +#else
> +#define SECOCEC_PM_OPS NULL
> +#endif
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id secocec_acpi_match[] = {
> +	{"CEC00001", 0},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
> +#endif
> +
> +static struct platform_driver secocec_driver = {
> +	.driver = {
> +		   .name = SECOCEC_DEV_NAME,
> +		   .acpi_match_table = ACPI_PTR(secocec_acpi_match),
> +		   .pm = SECOCEC_PM_OPS,
> +	},
> +	.probe = secocec_probe,
> +	.remove = secocec_remove,
> +};
> +
> +module_platform_driver(secocec_driver);
> +
> +MODULE_DESCRIPTION("SECO CEC X86 Driver");
> +MODULE_AUTHOR("Ettore Chimenti <ek5.chimenti@gmail.com>");
> +MODULE_LICENSE("Dual BSD/GPL");
> diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
> new file mode 100644
> index 000000000000..93020900935e
> --- /dev/null
> +++ b/drivers/media/platform/seco-cec/seco-cec.h
> @@ -0,0 +1,130 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> +/*
> + * SECO X86 Boards CEC register defines
> + *
> + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> + * Copyright (C) 2018, SECO Srl.
> + * Copyright (C) 2018, Aidilab Srl.
> + */
> +
> +#ifndef __SECO_CEC_H__
> +#define __SECO_CEC_H__
> +
> +#define SECOCEC_MAX_ADDRS		1
> +#define SECOCEC_DEV_NAME		"secocec"
> +#define SECOCEC_LATEST_FW		0x0f0b
> +
> +#define SMBTIMEOUT			0xffff
> +#define SMB_POLL_UDELAY			10
> +
> +#define SMBUS_WRITE			0
> +#define SMBUS_READ			1
> +
> +#define CMD_BYTE_DATA			0
> +#define CMD_WORD_DATA			1
> +
> +/*
> + * SMBus definitons for Braswell
> + */
> +
> +#define BRA_DONE_STATUS			BIT(7)
> +#define BRA_INUSE_STS			BIT(6)
> +#define BRA_FAILED_OP			BIT(4)
> +#define BRA_BUS_ERR			BIT(3)
> +#define BRA_DEV_ERR			BIT(2)
> +#define BRA_INTR			BIT(1)
> +#define BRA_HOST_BUSY			BIT(0)
> +#define BRA_HSTS_ERR_MASK   (BRA_FAILED_OP | BRA_BUS_ERR | BRA_DEV_ERR)
> +
> +#define BRA_PEC_EN			BIT(7)
> +#define BRA_START			BIT(6)
> +#define BRA_LAST__BYTE			BIT(5)
> +#define BRA_INTREN			BIT(0)
> +#define BRA_SMB_CMD			(7 << 2)
> +#define BRA_SMB_CMD_QUICK		(0 << 2)
> +#define BRA_SMB_CMD_BYTE		(1 << 2)
> +#define BRA_SMB_CMD_BYTE_DATA		(2 << 2)
> +#define BRA_SMB_CMD_WORD_DATA		(3 << 2)
> +#define BRA_SMB_CMD_PROCESS_CALL	(4 << 2)
> +#define BRA_SMB_CMD_BLOCK		(5 << 2)
> +#define BRA_SMB_CMD_I2CREAD		(6 << 2)
> +#define BRA_SMB_CMD_BLOCK_PROCESS	(7 << 2)
> +
> +#define BRA_SMB_BASE_ADDR  0x2040
> +#define HSTS               (BRA_SMB_BASE_ADDR + 0)
> +#define HCNT               (BRA_SMB_BASE_ADDR + 2)
> +#define HCMD               (BRA_SMB_BASE_ADDR + 3)
> +#define XMIT_SLVA          (BRA_SMB_BASE_ADDR + 4)
> +#define HDAT0              (BRA_SMB_BASE_ADDR + 5)
> +#define HDAT1              (BRA_SMB_BASE_ADDR + 6)
> +
> +/*
> + * Microcontroller Address
> + */
> +
> +#define SECOCEC_MICRO_ADDRESS		0x40
> +
> +/*
> + * STM32 SMBus Registers
> + */
> +
> +#define SECOCEC_VERSION			0x00
> +#define SECOCEC_ENABLE_REG_1		0x01
> +#define SECOCEC_ENABLE_REG_2		0x02
> +#define SECOCEC_STATUS_REG_1		0x03
> +#define SECOCEC_STATUS_REG_2		0x04
> +
> +#define SECOCEC_STATUS			0x28
> +#define SECOCEC_DEVICE_LA		0x29
> +#define SECOCEC_READ_OPERATION_ID	0x2a
> +#define SECOCEC_READ_DATA_LENGTH	0x2b
> +#define SECOCEC_READ_DATA_00		0x2c
> +#define SECOCEC_READ_DATA_02		0x2d
> +#define SECOCEC_READ_DATA_04		0x2e
> +#define SECOCEC_READ_DATA_06		0x2f
> +#define SECOCEC_READ_DATA_08		0x30
> +#define SECOCEC_READ_DATA_10		0x31
> +#define SECOCEC_READ_DATA_12		0x32
> +#define SECOCEC_READ_BYTE0		0x33
> +#define SECOCEC_WRITE_OPERATION_ID	0x34
> +#define SECOCEC_WRITE_DATA_LENGTH	0x35
> +#define SECOCEC_WRITE_DATA_00		0x36
> +#define SECOCEC_WRITE_DATA_02		0x37
> +#define SECOCEC_WRITE_DATA_04		0x38
> +#define SECOCEC_WRITE_DATA_06		0x39
> +#define SECOCEC_WRITE_DATA_08		0x3a
> +#define SECOCEC_WRITE_DATA_10		0x3b
> +#define SECOCEC_WRITE_DATA_12		0x3c
> +#define SECOCEC_WRITE_BYTE0		0x3d
> +
> +#define SECOCEC_IR_READ_DATA		0x3e
> +
> +/*
> + * Enabling register
> + */
> +
> +#define SECOCEC_ENABLE_REG_1_CEC		0x1000
> +#define SECOCEC_ENABLE_REG_1_IR			0x2000
> +#define SECOCEC_ENABLE_REG_1_IR_PASSTHROUGH	0x4000
> +
> +/*
> + * Status register
> + */
> +
> +#define SECOCEC_STATUS_REG_1_CEC	SECOCEC_ENABLE_REG_1_CEC
> +#define SECOCEC_STATUS_REG_1_IR		SECOCEC_ENABLE_REG_1_IR
> +#define SECOCEC_STATUS_REG_1_IR_PASSTHR	SECOCEC_ENABLE_REG_1_IR_PASSTHR
> +
> +/*
> + * Status data
> + */
> +
> +#define SECOCEC_STATUS_MSG_RECEIVED_MASK	BIT(0)
> +#define SECOCEC_STATUS_RX_ERROR_MASK		BIT(1)
> +#define SECOCEC_STATUS_MSG_SENT_MASK		BIT(2)
> +#define SECOCEC_STATUS_TX_ERROR_MASK		BIT(3)
> +
> +#define SECOCEC_STATUS_TX_NACK_ERROR		BIT(4)
> +#define SECOCEC_STATUS_RX_OVERFLOW_MASK		BIT(5)
> +
> +#endif /* __SECO_CEC_H__ */
> --
> 2.18.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/2] media: add SECO cec driver

[ektor5]

On Thu, Oct 18, 2018 at 10:47:28AM +0200, jacopo mondi wrote:
> Hi Ettore,
>    thanks for the patches.
> 
> A few other things, please see below.
> 
> On Wed, Oct 17, 2018 at 11:31:41PM +0200, ektor5 wrote:
> > From: Ettore Chimenti <ek5.chimenti@gmail.com>
> >
> > This patch adds support to the CEC device implemented with a STM32
> > microcontroller in X86 SECO Boards, including UDOO X86.
> >
> > The communication is achieved via Braswell integrated SMBus
> > (i2c-i801). The driver use direct access to the PCI addresses, due to
> > the limitations of the specific driver in presence of ACPI calls.
> >
> > The basic functionalities are tested with success with cec-ctl and
> > cec-compliance.
> >
> > Inspired by cros-ec-cec implementation, attaches to i915 driver
> > cec-notifier.
> >
> > Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
> > ---
> >  MAINTAINERS                                |   6 +
> >  drivers/media/platform/Kconfig             |  12 +
> >  drivers/media/platform/Makefile            |   2 +
> >  drivers/media/platform/seco-cec/Makefile   |   1 +
> >  drivers/media/platform/seco-cec/seco-cec.c | 699 +++++++++++++++++++++
> >  drivers/media/platform/seco-cec/seco-cec.h | 130 ++++
> >  6 files changed, 850 insertions(+)
> >  create mode 100644 drivers/media/platform/seco-cec/Makefile
> >  create mode 100644 drivers/media/platform/seco-cec/seco-cec.c
> >  create mode 100644 drivers/media/platform/seco-cec/seco-cec.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4ece30f15777..1062912a5ff4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12972,6 +12972,12 @@ L:	sdricohcs-devel@lists.sourceforge.net (subscribers-only)
> >  S:	Maintained
> >  F:	drivers/mmc/host/sdricoh_cs.c
> >
> > +SECO BOARDS CEC DRIVER
> > +M:	Ettore Chimenti <ek5.chimenti@gmail.com>
> > +S:	Maintained
> > +F:	drivers/media/platform/seco-cec/seco-cec.c
> > +F:	drivers/media/platform/seco-cec/seco-cec.h
> > +
> >  SECURE COMPUTING
> >  M:	Kees Cook <keescook@chromium.org>
> >  R:	Andy Lutomirski <luto@amacapital.net>
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 94c1fe0e9787..51cd1fd005e3 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -613,6 +613,18 @@ config VIDEO_TEGRA_HDMI_CEC
> >  	 The CEC bus is present in the HDMI connector and enables communication
> >  	 between compatible devices.
> >
> > +config VIDEO_SECO_CEC
> > +	tristate "SECO Boards HDMI CEC driver"
> > +	depends on (X86 || IA64) || COMPILE_TEST
> > +	depends on PCI && DMI
> > +	select CEC_CORE
> > +	select CEC_NOTIFIER
> > +	help
> > +	  This is a driver for SECO Boards integrated CEC interface.
> > +	  Selecting it will enable support for this device.
> > +	  CEC bus is present in the HDMI connector and enables communication
> > +	  between compatible devices.
> > +
> >  endif #CEC_PLATFORM_DRIVERS
> >
> >  menuconfig SDR_PLATFORM_DRIVERS
> > diff --git a/drivers/media/platform/Makefile b/drivers/media/platform/Makefile
> > index 41322ab65802..5d2b06c4c68a 100644
> > --- a/drivers/media/platform/Makefile
> > +++ b/drivers/media/platform/Makefile
> > @@ -53,6 +53,8 @@ obj-$(CONFIG_VIDEO_TEGRA_HDMI_CEC)	+= tegra-cec/
> >
> >  obj-y					+= stm32/
> >
> > +obj-$(CONFIG_VIDEO_SECO_CEC)		+= seco-cec/
> > +
> >  obj-y					+= davinci/
> >
> >  obj-$(CONFIG_VIDEO_SH_VOU)		+= sh_vou.o
> > diff --git a/drivers/media/platform/seco-cec/Makefile b/drivers/media/platform/seco-cec/Makefile
> > new file mode 100644
> > index 000000000000..09900b087d02
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/Makefile
> > @@ -0,0 +1 @@
> > +obj-y += seco-cec.o
> > diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
> > new file mode 100644
> > index 000000000000..9ac9a1f36bc0
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/seco-cec.c
> > @@ -0,0 +1,699 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause
> > +/*
> > + * CEC driver for SECO X86 Boards
> > + *
> > + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> > + * Copyright (C) 2018, SECO Srl.
> > + * Copyright (C) 2018, Aidilab Srl.
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/dmi.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/gpio.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* CEC Framework */
> > +#include <media/cec.h>
> > +
> > +#include "seco-cec.h"
> > +
> > +struct secocec_data {
> > +	struct device *dev;
> > +	struct platform_device *pdev;
> > +	struct cec_adapter *cec_adap;
> > +	struct cec_notifier *notifier;
> > +	int irq;
> > +};
> > +
> > +#define smb_wr16(cmd, data) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> > +					     cmd, data, SMBUS_WRITE, NULL)
> > +#define smb_rd16(cmd, res) smb_word_op(CMD_WORD_DATA, SECOCEC_MICRO_ADDRESS, \
> > +				       cmd, 0, SMBUS_READ, res)
> > +
> > +static int smb_word_op(short data_format, u16 slave_addr, u8 cmd, u16 data,
> > +		       u8 operation, u16 *result)
> > +{
> > +	unsigned int count;
> > +	short _data_format;
> > +	int status = 0;
> > +
> > +	switch (data_format) {
> > +	case CMD_BYTE_DATA:
> > +		_data_format = BRA_SMB_CMD_BYTE_DATA;
> > +		break;
> > +	case CMD_WORD_DATA:
> > +		_data_format = BRA_SMB_CMD_WORD_DATA;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Active wait until ready */
> > +	for (count = 0; count <= SMBTIMEOUT; ++count) {
> > +		if (!(inb(HSTS) & BRA_INUSE_STS))
> > +			break;
> > +		udelay(SMB_POLL_UDELAY);
> > +	}
> 
> Do you have any characterization for this?
> You are waiting up to 650msec, isn't it very long?

You're right. Reducing it to 41ms, it was arbitrary.

> 
> > +
> > +	if (count > SMBTIMEOUT)
> > +		/* Reset the lock instead of failing */
> > +		outb(0xff, HSTS);
> > +
> > +	outb(0x00, HCNT);
> > +	outb((u8)(slave_addr & 0xfe) | operation, XMIT_SLVA);
> > +	outb(cmd, HCMD);
> > +	inb(HCNT);
> > +
> > +	if (operation == SMBUS_WRITE) {
> > +		outb((u8)data, HDAT0);
> > +		outb((u8)(data >> 8), HDAT1);
> > +	}
> > +
> > +	outb(BRA_START + _data_format, HCNT);
> > +
> > +	for (count = 0; count <= SMBTIMEOUT; count++) {
> > +		if (!(inb(HSTS) & BRA_HOST_BUSY))
> > +			break;
> > +		udelay(SMB_POLL_UDELAY);
> > +	}
> > +
> > +	if (count > SMBTIMEOUT) {
> > +		status = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	if (inb(HSTS) & BRA_HSTS_ERR_MASK) {
> > +		status = -EIO;
> > +		goto err;
> > +	}
> > +
> > +	if (operation == SMBUS_READ)
> > +		*result = ((inb(HDAT0) & 0xff) + ((inb(HDAT1) & 0xff) << 8));
> > +
> > +err:
> > +	outb(0xff, HSTS);
> > +	return status;
> > +}
> > +
> > +static int secocec_adap_enable(struct cec_adapter *adap, bool enable)
> > +{
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	u16 val = 0;
> > +	int status;
> > +
> > +	if (enable) {
> > +		/* Clear the status register */
> > +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +		if (status)
> > +			goto err;
> > +
> > +		/* Enable the interrupts */
> > +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +				  val | SECOCEC_ENABLE_REG_1_CEC);
> > +		if (status)
> > +			goto err;
> > +
> > +		dev_dbg(dev, "Device enabled");
> > +	} else {
> > +		/* Clear the status register */
> > +		status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +		if (status)
> > +			goto err;
> > +
> > +		/* Disable the interrupts */
> > +		status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +		if (status)
> > +			goto err;
> > +
> > +		status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> > +				  ~SECOCEC_ENABLE_REG_1_CEC &
> > +				  ~SECOCEC_ENABLE_REG_1_IR);
> > +		if (status)
> > +			goto err;
> > +
> > +		dev_dbg(dev, "Device disabled");
> 
> I would drop these two, as they don't add any debug value, just tell
> you everything went fine. But it's a nit, feel free to chose yourself.
> 
> > +	}
> > +
> > +	return 0;
> > +err:
> > +	dev_err(dev, "Adapter setup failed (%d)", status);
> > +	return status;
> > +}
> > +
> > +static int secocec_adap_log_addr(struct cec_adapter *adap, u8 logical_addr)
> > +{
> > +	u16 enable_val = 0;
> > +	int status;
> > +
> > +	/* Disable device */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &enable_val);
> > +	if (status)
> > +		return status;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +			  enable_val & ~SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		return status;
> > +
> > +	/* Write logical address */
> > +	status = smb_wr16(SECOCEC_DEVICE_LA, logical_addr);
> > +	if (status)
> > +		return status;
> > +
> > +	/* Re-enable device */
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1,
> > +			  enable_val | SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		return status;
> > +
> > +	return 0;
> > +}
> > +
> > +static int secocec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > +				 u32 signal_free_time, struct cec_msg *msg)
> > +{
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	u16 payload_len, payload_id_len, destination, val = 0;
> > +	u8 *payload_msg;
> > +	int status;
> > +	u8 i;
> > +
> > +	/* Device msg len already accounts for header */
> > +	payload_id_len = msg->len - 1;
> > +
> > +	/* Send data length */
> > +	status = smb_wr16(SECOCEC_WRITE_DATA_LENGTH, payload_id_len);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Send Operation ID if present */
> > +	if (payload_id_len > 0) {
> > +		status = smb_wr16(SECOCEC_WRITE_OPERATION_ID, msg->msg[1]);
> > +		if (status)
> > +			goto err;
> > +	}
> > +	/* Send data if present */
> > +	if (payload_id_len > 1) {
> > +		/* Only data; */
> > +		payload_len = msg->len - 2;
> > +		payload_msg = &msg->msg[2];
> > +
> > +		/* Copy message into registers */
> > +		for (i = 0; i < payload_len; i += 2) {
> > +			/* hi byte */
> > +			val = payload_msg[i + 1] << 8;
> > +
> > +			/* lo byte */
> > +			val |= payload_msg[i];
> > +
> > +			status = smb_wr16(SECOCEC_WRITE_DATA_00 + i / 2, val);
> > +			if (status)
> > +				goto err;
> > +		}
> > +	}
> > +	/* Send msg source/destination and fire msg */
> > +	destination = msg->msg[0];
> > +	status = smb_wr16(SECOCEC_WRITE_BYTE0, destination);
> > +	if (status)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Transmit failed (%d)", status);
> > +	return status;
> > +}
> > +
> > +static int secocec_tx_done(struct cec_adapter *adap, u16 status_val)
> > +{
> > +	int status = 0;
> > +
> > +	if (status_val & SECOCEC_STATUS_TX_ERROR_MASK) {
> > +		if (status_val & SECOCEC_STATUS_TX_NACK_ERROR) {
> > +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_NACK);
> > +			status = -EAGAIN;
> > +		} else {
> > +			cec_transmit_attempt_done(adap, CEC_TX_STATUS_ERROR);
> > +			status = -EIO;
> > +		}
> > +	} else {
> > +		cec_transmit_attempt_done(adap, CEC_TX_STATUS_OK);
> > +	}
> > +
> > +	/* Reset status reg */
> > +	status_val = SECOCEC_STATUS_TX_ERROR_MASK |
> > +		SECOCEC_STATUS_MSG_SENT_MASK |
> > +		SECOCEC_STATUS_TX_NACK_ERROR;
> > +	smb_wr16(SECOCEC_STATUS, status_val);
> > +
> > +	return status;
> > +}
> > +
> > +static int secocec_rx_done(struct cec_adapter *adap, u16 status_val)
> > +{
> > +	struct secocec_data *cec = cec_get_drvdata(adap);
> > +	struct device *dev = cec->dev;
> > +	struct cec_msg msg = { };
> > +	bool flag_overflow = false;
> > +	u8 payload_len, i = 0;
> > +	u8 *payload_msg;
> > +	u16 val = 0;
> > +	int status;
> > +
> > +	if (status_val & SECOCEC_STATUS_RX_OVERFLOW_MASK) {
> > +		dev_warn(dev, "Received more than 16 bytes. Discarding");
> > +		flag_overflow = true;
> > +	}
> > +
> > +	if (status_val & SECOCEC_STATUS_RX_ERROR_MASK) {
> > +		dev_warn(dev, "Message received with errors. Discarding");
> > +		status = -EIO;
> > +		goto rxerr;
> > +	}
> > +
> > +	/* Read message length */
> > +	status = smb_rd16(SECOCEC_READ_DATA_LENGTH, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Device msg len already accounts for the header */
> > +	msg.len = min(val + 1, CEC_MAX_MSG_SIZE);
> > +
> > +	/* Read logical address */
> > +	status = smb_rd16(SECOCEC_READ_BYTE0, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* device stores source LA and destination */
> > +	msg.msg[0] = val;
> > +
> > +	/* Read operation ID */
> > +	status = smb_rd16(SECOCEC_READ_OPERATION_ID, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	msg.msg[1] = val;
> > +
> > +	/* Read data if present */
> > +	if (msg.len > 1) {
> > +		payload_len = msg.len - 2;
> > +		payload_msg = &msg.msg[2];
> > +
> > +		/* device stores 2 bytes in every 16-bit val */
> > +		for (i = 0; i < payload_len; i += 2) {
> > +			status = smb_rd16(SECOCEC_READ_DATA_00 + i / 2, &val);
> > +			if (status)
> > +				goto err;
> > +
> > +			/* low byte, skipping header */
> > +			payload_msg[i] = val & 0x00ff;
> > +
> > +			/* hi byte */
> > +			payload_msg[i + 1] = (val & 0xff00) >> 8;
> > +		}
> > +	}
> > +
> > +	cec_received_msg(cec->cec_adap, &msg);
> > +
> > +	/* Reset status reg */
> > +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK;
> > +	if (flag_overflow)
> > +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS, status_val);
> > +	if (status)
> > +		goto err;
> 
> If you fail here, you would not catch the error in the caller, as you
> don't check for the return value, nor reset the
> SECOCEC_STATUS_MSG_RECEIVED_MASK flag. Same for the tx function.
> 
> > +
> > +	return 0;
> > +
> > +rxerr:
> > +	/* Reset error reg */
> > +	status_val = SECOCEC_STATUS_MSG_RECEIVED_MASK |
> > +		SECOCEC_STATUS_RX_ERROR_MASK;
> > +	if (flag_overflow)
> > +		status_val |= SECOCEC_STATUS_RX_OVERFLOW_MASK;
> > +	smb_wr16(SECOCEC_STATUS, status_val);
> > +
> > +err:
> > +	dev_err(dev, "Receive message failed (%d)", status);
> > +	return status;
> > +}
> > +
> > +struct cec_adap_ops secocec_cec_adap_ops = {
> > +	/* Low-level callbacks */
> > +	.adap_enable = secocec_adap_enable,
> > +	.adap_log_addr = secocec_adap_log_addr,
> > +	.adap_transmit = secocec_adap_transmit,
> > +};
> > +
> > +static irqreturn_t secocec_irq_handler(int irq, void *priv)
> > +{
> > +	struct secocec_data *cec = priv;
> > +	struct device *dev = cec->dev;
> > +	u16 status_val, cec_val, val = 0;
> > +	int status;
> > +
> > +	/*  Read status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &status_val);
> > +	if (status)
> > +		goto err;
> > +
> > +	if (status_val & SECOCEC_STATUS_REG_1_CEC) {
> > +		/* Read CEC status register */
> > +		status = smb_rd16(SECOCEC_STATUS, &cec_val);
> > +		if (status)
> > +			goto err;
> > +
> > +		if (cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK)
> > +			secocec_rx_done(cec->cec_adap, cec_val);
> > +
> > +		if (cec_val & SECOCEC_STATUS_MSG_SENT_MASK)
> > +			secocec_tx_done(cec->cec_adap, cec_val);
> 
> Not checking return values of those two functions. Please see comment
> in the rx_done function.
> 
> > +
> > +		if ((~cec_val & SECOCEC_STATUS_MSG_SENT_MASK) &&
> > +		    (~cec_val & SECOCEC_STATUS_MSG_RECEIVED_MASK))
> > +			dev_warn(dev,
> > +				 "Message not received or sent, but interrupt fired");
> > +
> > +		val = SECOCEC_STATUS_REG_1_CEC;
> > +	}
> > +
> > +	if (status_val & SECOCEC_STATUS_REG_1_IR) {
> > +		dev_dbg(dev, "IR RC5 Interrupt Caught");
> 
> I would drop any informative printouts from an irq routine.
> 
> > +		val |= SECOCEC_STATUS_REG_1_IR;
> > +		/* TODO IRDA RX */
> > +	}
> > +
> > +	/*  Reset status register */
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	return IRQ_HANDLED;
> > +
> > +err:
> > +	dev_err(dev, "IRQ: Read/Write SMBus operation failed (%d)", status);
> > +
> > +	/*  Reset status register */
> > +	val = SECOCEC_STATUS_REG_1_CEC | SECOCEC_STATUS_REG_1_IR;
> > +	smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +struct cec_dmi_match {
> > +	char *sys_vendor;
> > +	char *product_name;
> > +	char *devname;
> > +	char *conn;
> > +};
> > +
> > +static const struct cec_dmi_match secocec_dmi_match_table[] = {
> > +	/* UDOO X86 */
> > +	{ "SECO", "UDOO x86", "0000:00:02.0", "Port B" },
> > +};
> > +
> > +static int secocec_cec_get_notifier(struct cec_notifier **notify)
> > +{
> > +	int i;
> > +
> > +	for (i = 0 ; i < ARRAY_SIZE(secocec_dmi_match_table) ; ++i) {
> > +		const struct cec_dmi_match *m = &secocec_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;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int secocec_acpi_probe(struct secocec_data *sdev)
> > +{
> > +	struct device *dev = sdev->dev;
> > +	struct gpio_desc *gpio;
> > +	int irq = 0;
> > +
> > +	gpio = devm_gpiod_get(dev, NULL, GPIOF_IN);
> > +	if (IS_ERR(gpio)) {
> > +		dev_err(dev, "Cannot request interrupt gpio");
> > +		return PTR_ERR(gpio);
> > +	}
> > +
> > +	irq = gpiod_to_irq(gpio);
> > +	if (irq < 0) {
> > +		dev_err(dev, "Cannot find valid irq");
> > +		return -ENODEV;
> > +	}
> > +	dev_dbg(dev, "irq-gpio is bound to IRQ %d", irq);
> > +
> > +	sdev->irq = irq;
> > +
> > +	return 0;
> > +}
> > +
> > +static int secocec_probe(struct platform_device *pdev)
> > +{
> > +	struct secocec_data *secocec;
> > +	struct device *dev = &pdev->dev;
> > +	int ret;
> > +	u16 val;
> > +
> > +	secocec = devm_kzalloc(dev, sizeof(*secocec), GFP_KERNEL);
> > +	if (!secocec)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, secocec);
> > +
> > +	/* Request SMBus regions */
> > +	if (!request_muxed_region(BRA_SMB_BASE_ADDR, 7, "CEC00001")) {
> > +		dev_err(dev, "Request memory region failed");
> > +		return -ENXIO;
> > +	}
> > +
> > +	secocec->pdev = pdev;
> > +	secocec->dev = dev;
> > +
> > +	if (!has_acpi_companion(dev)) {
> > +		dev_dbg(dev, "Cannot find any ACPI companion");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	ret = secocec_acpi_probe(secocec);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot assign gpio to IRQ");
> > +		ret = -ENODEV;
> > +		goto err;
> > +	}
> > +
> > +	dev_dbg(dev, "IRQ detected at %d", secocec->irq);
> 
> You have just printed this out in secocec_acpi_probe()
> 
> Apart a few minors you can fix in next iteration, this looks good now.
> Once fixed, please add my:
> 
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks a lot Jacopo,
	Ettore

> 
> Thanks
>    j
> 
> > +
> > +	/* Firmware version check */
> > +	ret = smb_rd16(SECOCEC_VERSION, &val);
> > +	if (ret) {
> > +		dev_err(dev, "Cannot check fw version");
> > +		goto err;
> > +	}
> > +	if (val < SECOCEC_LATEST_FW) {
> > +		dev_err(dev, "CEC Firmware not supported (v.%04x). Use ver > v.%04x",
> > +			val, SECOCEC_LATEST_FW);
> > +		ret = -EINVAL;
> > +		goto err;
> > +	}
> > +
> > +	ret = secocec_cec_get_notifier(&secocec->notifier);
> > +	if (ret) {
> > +		dev_err(dev, "no CEC notifier available\n");
> > +		goto err;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(dev,
> > +					secocec->irq,
> > +					NULL,
> > +					secocec_irq_handler,
> > +					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > +					dev_name(&pdev->dev), secocec);
> > +
> > +	if (ret) {
> > +		dev_err(dev, "Cannot request IRQ %d", secocec->irq);
> > +		ret = -EIO;
> > +		goto err;
> > +	}
> > +
> > +	/* Allocate CEC adapter */
> > +	secocec->cec_adap = cec_allocate_adapter(&secocec_cec_adap_ops,
> > +						 secocec,
> > +						 dev_name(dev),
> > +						 CEC_CAP_DEFAULTS,
> > +						 SECOCEC_MAX_ADDRS);
> > +
> > +	if (IS_ERR(secocec->cec_adap)) {
> > +		ret = PTR_ERR(secocec->cec_adap);
> > +		goto err;
> > +	}
> > +
> > +	ret = cec_register_adapter(secocec->cec_adap, dev);
> > +	if (ret)
> > +		goto err_delete_adapter;
> > +
> > +	if (secocec->notifier)
> > +		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
> > +
> > +	platform_set_drvdata(pdev, secocec);
> > +
> > +	dev_dbg(dev, "Device registered");
> > +
> > +	return ret;
> > +
> > +err_delete_adapter:
> > +	cec_delete_adapter(secocec->cec_adap);
> > +err:
> > +	dev_err(dev, "%s device probe failed\n", dev_name(dev));
> > +
> > +	return ret;
> > +}
> > +
> > +static int secocec_remove(struct platform_device *pdev)
> > +{
> > +	struct secocec_data *secocec = platform_get_drvdata(pdev);
> > +
> > +	cec_unregister_adapter(secocec->cec_adap);
> > +
> > +	if (secocec->notifier)
> > +		cec_notifier_put(secocec->notifier);
> > +
> > +	release_region(BRA_SMB_BASE_ADDR, 7);
> > +
> > +	dev_dbg(&pdev->dev, "CEC device removed");
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM_SLEEP
> > +static int secocec_suspend(struct device *dev)
> > +{
> > +	int status;
> > +	u16 val;
> > +
> > +	dev_dbg(dev, "Device going to suspend, disabling");
> > +
> > +	/* Clear the status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Disable the interrupts */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val &
> > +			  ~SECOCEC_ENABLE_REG_1_CEC & ~SECOCEC_ENABLE_REG_1_IR);
> > +	if (status)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Suspend failed (err: %d)", status);
> > +	return status;
> > +}
> > +
> > +static int secocec_resume(struct device *dev)
> > +{
> > +	int status;
> > +	u16 val;
> > +
> > +	dev_dbg(dev, "Resuming device from suspend");
> > +
> > +	/* Clear the status register */
> > +	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
> > +	if (status)
> > +		goto err;
> > +
> > +	/* Enable the interrupts */
> > +	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
> > +	if (status)
> > +		goto err;
> > +
> > +	status = smb_wr16(SECOCEC_ENABLE_REG_1, val | SECOCEC_ENABLE_REG_1_CEC);
> > +	if (status)
> > +		goto err;
> > +
> > +	dev_dbg(dev, "Device resumed from suspend");
> > +
> > +	return 0;
> > +
> > +err:
> > +	dev_err(dev, "Resume failed (err: %d)", status);
> > +	return status;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(secocec_pm_ops, secocec_suspend, secocec_resume);
> > +#define SECOCEC_PM_OPS (&secocec_pm_ops)
> > +#else
> > +#define SECOCEC_PM_OPS NULL
> > +#endif
> > +
> > +#ifdef CONFIG_ACPI
> > +static const struct acpi_device_id secocec_acpi_match[] = {
> > +	{"CEC00001", 0},
> > +	{},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(acpi, secocec_acpi_match);
> > +#endif
> > +
> > +static struct platform_driver secocec_driver = {
> > +	.driver = {
> > +		   .name = SECOCEC_DEV_NAME,
> > +		   .acpi_match_table = ACPI_PTR(secocec_acpi_match),
> > +		   .pm = SECOCEC_PM_OPS,
> > +	},
> > +	.probe = secocec_probe,
> > +	.remove = secocec_remove,
> > +};
> > +
> > +module_platform_driver(secocec_driver);
> > +
> > +MODULE_DESCRIPTION("SECO CEC X86 Driver");
> > +MODULE_AUTHOR("Ettore Chimenti <ek5.chimenti@gmail.com>");
> > +MODULE_LICENSE("Dual BSD/GPL");
> > diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
> > new file mode 100644
> > index 000000000000..93020900935e
> > --- /dev/null
> > +++ b/drivers/media/platform/seco-cec/seco-cec.h
> > @@ -0,0 +1,130 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> > +/*
> > + * SECO X86 Boards CEC register defines
> > + *
> > + * Author:  Ettore Chimenti <ek5.chimenti@gmail.com>
> > + * Copyright (C) 2018, SECO Srl.
> > + * Copyright (C) 2018, Aidilab Srl.
> > + */
> > +
> > +#ifndef __SECO_CEC_H__
> > +#define __SECO_CEC_H__
> > +
> > +#define SECOCEC_MAX_ADDRS		1
> > +#define SECOCEC_DEV_NAME		"secocec"
> > +#define SECOCEC_LATEST_FW		0x0f0b
> > +
> > +#define SMBTIMEOUT			0xffff
> > +#define SMB_POLL_UDELAY			10
> > +
> > +#define SMBUS_WRITE			0
> > +#define SMBUS_READ			1
> > +
> > +#define CMD_BYTE_DATA			0
> > +#define CMD_WORD_DATA			1
> > +
> > +/*
> > + * SMBus definitons for Braswell
> > + */
> > +
> > +#define BRA_DONE_STATUS			BIT(7)
> > +#define BRA_INUSE_STS			BIT(6)
> > +#define BRA_FAILED_OP			BIT(4)
> > +#define BRA_BUS_ERR			BIT(3)
> > +#define BRA_DEV_ERR			BIT(2)
> > +#define BRA_INTR			BIT(1)
> > +#define BRA_HOST_BUSY			BIT(0)
> > +#define BRA_HSTS_ERR_MASK   (BRA_FAILED_OP | BRA_BUS_ERR | BRA_DEV_ERR)
> > +
> > +#define BRA_PEC_EN			BIT(7)
> > +#define BRA_START			BIT(6)
> > +#define BRA_LAST__BYTE			BIT(5)
> > +#define BRA_INTREN			BIT(0)
> > +#define BRA_SMB_CMD			(7 << 2)
> > +#define BRA_SMB_CMD_QUICK		(0 << 2)
> > +#define BRA_SMB_CMD_BYTE		(1 << 2)
> > +#define BRA_SMB_CMD_BYTE_DATA		(2 << 2)
> > +#define BRA_SMB_CMD_WORD_DATA		(3 << 2)
> > +#define BRA_SMB_CMD_PROCESS_CALL	(4 << 2)
> > +#define BRA_SMB_CMD_BLOCK		(5 << 2)
> > +#define BRA_SMB_CMD_I2CREAD		(6 << 2)
> > +#define BRA_SMB_CMD_BLOCK_PROCESS	(7 << 2)
> > +
> > +#define BRA_SMB_BASE_ADDR  0x2040
> > +#define HSTS               (BRA_SMB_BASE_ADDR + 0)
> > +#define HCNT               (BRA_SMB_BASE_ADDR + 2)
> > +#define HCMD               (BRA_SMB_BASE_ADDR + 3)
> > +#define XMIT_SLVA          (BRA_SMB_BASE_ADDR + 4)
> > +#define HDAT0              (BRA_SMB_BASE_ADDR + 5)
> > +#define HDAT1              (BRA_SMB_BASE_ADDR + 6)
> > +
> > +/*
> > + * Microcontroller Address
> > + */
> > +
> > +#define SECOCEC_MICRO_ADDRESS		0x40
> > +
> > +/*
> > + * STM32 SMBus Registers
> > + */
> > +
> > +#define SECOCEC_VERSION			0x00
> > +#define SECOCEC_ENABLE_REG_1		0x01
> > +#define SECOCEC_ENABLE_REG_2		0x02
> > +#define SECOCEC_STATUS_REG_1		0x03
> > +#define SECOCEC_STATUS_REG_2		0x04
> > +
> > +#define SECOCEC_STATUS			0x28
> > +#define SECOCEC_DEVICE_LA		0x29
> > +#define SECOCEC_READ_OPERATION_ID	0x2a
> > +#define SECOCEC_READ_DATA_LENGTH	0x2b
> > +#define SECOCEC_READ_DATA_00		0x2c
> > +#define SECOCEC_READ_DATA_02		0x2d
> > +#define SECOCEC_READ_DATA_04		0x2e
> > +#define SECOCEC_READ_DATA_06		0x2f
> > +#define SECOCEC_READ_DATA_08		0x30
> > +#define SECOCEC_READ_DATA_10		0x31
> > +#define SECOCEC_READ_DATA_12		0x32
> > +#define SECOCEC_READ_BYTE0		0x33
> > +#define SECOCEC_WRITE_OPERATION_ID	0x34
> > +#define SECOCEC_WRITE_DATA_LENGTH	0x35
> > +#define SECOCEC_WRITE_DATA_00		0x36
> > +#define SECOCEC_WRITE_DATA_02		0x37
> > +#define SECOCEC_WRITE_DATA_04		0x38
> > +#define SECOCEC_WRITE_DATA_06		0x39
> > +#define SECOCEC_WRITE_DATA_08		0x3a
> > +#define SECOCEC_WRITE_DATA_10		0x3b
> > +#define SECOCEC_WRITE_DATA_12		0x3c
> > +#define SECOCEC_WRITE_BYTE0		0x3d
> > +
> > +#define SECOCEC_IR_READ_DATA		0x3e
> > +
> > +/*
> > + * Enabling register
> > + */
> > +
> > +#define SECOCEC_ENABLE_REG_1_CEC		0x1000
> > +#define SECOCEC_ENABLE_REG_1_IR			0x2000
> > +#define SECOCEC_ENABLE_REG_1_IR_PASSTHROUGH	0x4000
> > +
> > +/*
> > + * Status register
> > + */
> > +
> > +#define SECOCEC_STATUS_REG_1_CEC	SECOCEC_ENABLE_REG_1_CEC
> > +#define SECOCEC_STATUS_REG_1_IR		SECOCEC_ENABLE_REG_1_IR
> > +#define SECOCEC_STATUS_REG_1_IR_PASSTHR	SECOCEC_ENABLE_REG_1_IR_PASSTHR
> > +
> > +/*
> > + * Status data
> > + */
> > +
> > +#define SECOCEC_STATUS_MSG_RECEIVED_MASK	BIT(0)
> > +#define SECOCEC_STATUS_RX_ERROR_MASK		BIT(1)
> > +#define SECOCEC_STATUS_MSG_SENT_MASK		BIT(2)
> > +#define SECOCEC_STATUS_TX_ERROR_MASK		BIT(3)
> > +
> > +#define SECOCEC_STATUS_TX_NACK_ERROR		BIT(4)
> > +#define SECOCEC_STATUS_RX_OVERFLOW_MASK		BIT(5)
> > +
> > +#endif /* __SECO_CEC_H__ */
> > --
> > 2.18.0
> >

[PATCH v3 2/2] seco-cec: add Consumer-IR support

[ektor5]

From: Ettore Chimenti <ek5.chimenti@gmail.com>

Introduce support for Consumer-IR into seco-cec driver, as it shares the
same interrupt for receiving messages.
The device decodes RC5 signals only, defaults to hauppauge mapping.
It will spawn an input interface using the RC framework (like CEC
device).

Signed-off-by: Ettore Chimenti <ek5.chimenti@gmail.com>
Reviewed-by: Sean Young <sean@mess.org>
---
 drivers/media/platform/Kconfig             |  10 ++
 drivers/media/platform/seco-cec/seco-cec.c | 125 ++++++++++++++++++++-
 drivers/media/platform/seco-cec/seco-cec.h |  11 ++
 3 files changed, 145 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 51cd1fd005e3..e6b45da2af6d 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -625,6 +625,16 @@ config VIDEO_SECO_CEC
 	  CEC bus is present in the HDMI connector and enables communication
 	  between compatible devices.
 
+config VIDEO_SECO_RC
+	bool "SECO Boards IR RC5 support"
+	depends on VIDEO_SECO_CEC
+	select RC_CORE
+	help
+	  If you say yes here you will get support for the
+	  SECO Boards Consumer-IR in seco-cec driver.
+	  The embedded controller supports RC5 protocol only, default mapping
+	  is set to rc-hauppauge.
+
 endif #CEC_PLATFORM_DRIVERS
 
 menuconfig SDR_PLATFORM_DRIVERS
diff --git a/drivers/media/platform/seco-cec/seco-cec.c b/drivers/media/platform/seco-cec/seco-cec.c
index 9ac9a1f36bc0..c5af913d4715 100644
--- a/drivers/media/platform/seco-cec/seco-cec.c
+++ b/drivers/media/platform/seco-cec/seco-cec.c
@@ -26,6 +26,8 @@ struct secocec_data {
 	struct platform_device *pdev;
 	struct cec_adapter *cec_adap;
 	struct cec_notifier *notifier;
+	struct rc_dev *ir;
+	char ir_input_phys[32];
 	int irq;
 };
 
@@ -364,6 +366,114 @@ struct cec_adap_ops secocec_cec_adap_ops = {
 	.adap_transmit = secocec_adap_transmit,
 };
 
+#ifdef CONFIG_VIDEO_SECO_RC
+static int secocec_ir_probe(void *priv)
+{
+	struct secocec_data *cec = priv;
+	struct device *dev = cec->dev;
+	int status;
+	u16 val;
+
+	/* Prepare the RC input device */
+	cec->ir = devm_rc_allocate_device(dev, RC_DRIVER_SCANCODE);
+	if (!cec->ir)
+		return -ENOMEM;
+
+	snprintf(cec->ir_input_phys, sizeof(cec->ir_input_phys),
+		 "%s/input0", dev_name(dev));
+
+	cec->ir->device_name = dev_name(dev);
+	cec->ir->input_phys = cec->ir_input_phys;
+	cec->ir->input_id.bustype = BUS_HOST;
+	cec->ir->input_id.vendor = 0;
+	cec->ir->input_id.product = 0;
+	cec->ir->input_id.version = 1;
+	cec->ir->driver_name = SECOCEC_DEV_NAME;
+	cec->ir->allowed_protocols = RC_PROTO_BIT_RC5;
+	cec->ir->priv = cec;
+	cec->ir->map_name = RC_MAP_HAUPPAUGE;
+	cec->ir->timeout = MS_TO_NS(100);
+
+	/* Clear the status register */
+	status = smb_rd16(SECOCEC_STATUS_REG_1, &val);
+	if (status != 0)
+		goto err;
+
+	status = smb_wr16(SECOCEC_STATUS_REG_1, val);
+	if (status != 0)
+		goto err;
+
+	/* Enable the interrupts */
+	status = smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+	if (status != 0)
+		goto err;
+
+	status = smb_wr16(SECOCEC_ENABLE_REG_1,
+			  val | SECOCEC_ENABLE_REG_1_IR);
+	if (status != 0)
+		goto err;
+
+	dev_dbg(dev, "IR enabled");
+
+	status = devm_rc_register_device(dev, cec->ir);
+
+	if (status) {
+		dev_err(dev, "Failed to prepare input device");
+		cec->ir = NULL;
+		goto err;
+	}
+
+	return 0;
+
+err:
+	smb_rd16(SECOCEC_ENABLE_REG_1, &val);
+
+	smb_wr16(SECOCEC_ENABLE_REG_1,
+		 val & ~SECOCEC_ENABLE_REG_1_IR);
+
+	dev_dbg(dev, "IR disabled");
+	return status;
+}
+
+static int secocec_ir_rx(struct secocec_data *priv)
+{
+	struct secocec_data *cec = priv;
+	struct device *dev = cec->dev;
+	u16 val, status, key, addr, toggle;
+
+	if (!cec->ir)
+		return -ENODEV;
+
+	status = smb_rd16(SECOCEC_IR_READ_DATA, &val);
+	if (status != 0)
+		goto err;
+
+	key = val & SECOCEC_IR_COMMAND_MASK;
+	addr = (val & SECOCEC_IR_ADDRESS_MASK) >> SECOCEC_IR_ADDRESS_SHL;
+	toggle = (val & SECOCEC_IR_TOGGLE_MASK) >> SECOCEC_IR_TOGGLE_SHL;
+
+	rc_keydown(cec->ir, RC_PROTO_RC5, RC_SCANCODE_RC5(addr, key), toggle);
+
+	dev_dbg(dev, "IR key pressed: 0x%02x addr 0x%02x toggle 0x%02x", key,
+		addr, toggle);
+
+	return 0;
+
+err:
+	dev_err(dev, "IR Receive message failed (%d)", status);
+	return -EIO;
+}
+#else
+static void secocec_ir_rx(struct secocec_data *priv)
+{
+}
+
+static int secocec_ir_probe(void *priv)
+{
+	return 0;
+}
+#endif
+
 static irqreturn_t secocec_irq_handler(int irq, void *priv)
 {
 	struct secocec_data *cec = priv;
@@ -399,7 +509,8 @@ static irqreturn_t secocec_irq_handler(int irq, void *priv)
 	if (status_val & SECOCEC_STATUS_REG_1_IR) {
 		dev_dbg(dev, "IR RC5 Interrupt Caught");
 		val |= SECOCEC_STATUS_REG_1_IR;
-		/* TODO IRDA RX */
+
+		secocec_ir_rx(cec);
 	}
 
 	/*  Reset status register */
@@ -569,6 +680,10 @@ static int secocec_probe(struct platform_device *pdev)
 	if (secocec->notifier)
 		cec_register_cec_notifier(secocec->cec_adap, secocec->notifier);
 
+	ret = secocec_ir_probe(secocec);
+	if (ret)
+		goto err_delete_adapter;
+
 	platform_set_drvdata(pdev, secocec);
 
 	dev_dbg(dev, "Device registered");
@@ -586,7 +701,15 @@ static int secocec_probe(struct platform_device *pdev)
 static int secocec_remove(struct platform_device *pdev)
 {
 	struct secocec_data *secocec = platform_get_drvdata(pdev);
+	u16 val;
+
+	if (secocec->ir) {
+		smb_rd16(SECOCEC_ENABLE_REG_1, &val);
 
+		smb_wr16(SECOCEC_ENABLE_REG_1, val & ~SECOCEC_ENABLE_REG_1_IR);
+
+		dev_dbg(&pdev->dev, "IR disabled");
+	}
 	cec_unregister_adapter(secocec->cec_adap);
 
 	if (secocec->notifier)
diff --git a/drivers/media/platform/seco-cec/seco-cec.h b/drivers/media/platform/seco-cec/seco-cec.h
index 93020900935e..3f1aad89b073 100644
--- a/drivers/media/platform/seco-cec/seco-cec.h
+++ b/drivers/media/platform/seco-cec/seco-cec.h
@@ -99,6 +99,17 @@
 
 #define SECOCEC_IR_READ_DATA		0x3e
 
+/*
+ * IR
+ */
+
+#define SECOCEC_IR_COMMAND_MASK		0x007F
+#define SECOCEC_IR_COMMAND_SHL		0
+#define SECOCEC_IR_ADDRESS_MASK		0x1F00
+#define SECOCEC_IR_ADDRESS_SHL		7
+#define SECOCEC_IR_TOGGLE_MASK		0x8000
+#define SECOCEC_IR_TOGGLE_SHL		15
+
 /*
  * Enabling register
  */
-- 
2.18.0