* [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device
@ 2020-04-24 5:38 Jeff Chase
2020-04-24 5:38 ` [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver Jeff Chase
2020-05-11 21:20 ` [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Rob Herring
0 siblings, 2 replies; 11+ messages in thread
From: Jeff Chase @ 2020-04-24 5:38 UTC (permalink / raw
To: linux-media; +Cc: mchehab, hverkuil-cisco, robh+dt, devicetree, Jeff Chase
The ch7322 is a Chrontel CEC controller.
Signed-off-by: Jeff Chase <jnchase@google.com>
---
Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
2 files changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 4165352a590a..ec2ddc6cdf9a 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -48,6 +48,8 @@ properties:
- capella,cm32181
# CM3232: Ambient Light Sensor
- capella,cm3232
+ # CH7322: HDMI-CEC Controller
+ - chrontel,ch7322
# High-Precision Digital Thermometer
- dallas,ds1631
# Total-Elapsed-Time Recorder with Alarm
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index d3891386d671..7794ffccd325 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -187,6 +187,8 @@ patternProperties:
description: ChipOne
"^chipspark,.*":
description: ChipSPARK
+ "^chrontel,.*":
+ description: Chrontel, Inc.
"^chrp,.*":
description: Common Hardware Reference Platform
"^chunghwa,.*":
--
2.26.2.303.gf8c07b1a785-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-24 5:38 [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Jeff Chase
@ 2020-04-24 5:38 ` Jeff Chase
2020-04-24 12:24 ` Hans Verkuil
2020-04-25 9:17 ` Hans Verkuil
2020-05-11 21:20 ` [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Rob Herring
1 sibling, 2 replies; 11+ messages in thread
From: Jeff Chase @ 2020-04-24 5:38 UTC (permalink / raw
To: linux-media; +Cc: mchehab, hverkuil-cisco, robh+dt, devicetree, Jeff Chase
Add a CEC device driver for the Chrontel ch7322 CEC conroller.
This is an I2C device capable of sending and receiving CEC messages.
Signed-off-by: Jeff Chase <jnchase@google.com>
---
MAINTAINERS | 7 +
drivers/media/cec/Kconfig | 1 +
drivers/media/cec/Makefile | 2 +-
drivers/media/cec/i2c/Kconfig | 14 +
drivers/media/cec/i2c/Makefile | 5 +
drivers/media/cec/i2c/ch7322.c | 455 +++++++++++++++++++++++++++++++++
6 files changed, 483 insertions(+), 1 deletion(-)
create mode 100644 drivers/media/cec/i2c/Kconfig
create mode 100644 drivers/media/cec/i2c/Makefile
create mode 100644 drivers/media/cec/i2c/ch7322.c
diff --git a/MAINTAINERS b/MAINTAINERS
index d633a131dcd7..d43f5146cad6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4047,6 +4047,13 @@ F: drivers/power/supply/cros_usbpd-charger.c
N: cros_ec
N: cros-ec
+CHRONTEL CH7322 CEC DRIVER
+M: Jeff Chase <jnchase@google.com>
+L: linux-media@vger.kernel.org
+S: Maintained
+T: git git://linuxtv.org/media_tree.git
+F: drivers/media/cec/i2c/ch7322.c
+
CIRRUS LOGIC AUDIO CODEC DRIVERS
M: James Schulman <james.schulman@cirrus.com>
M: David Rhodes <david.rhodes@cirrus.com>
diff --git a/drivers/media/cec/Kconfig b/drivers/media/cec/Kconfig
index eea74b7cfa8c..3e934aa239ab 100644
--- a/drivers/media/cec/Kconfig
+++ b/drivers/media/cec/Kconfig
@@ -33,6 +33,7 @@ menuconfig MEDIA_CEC_SUPPORT
adapter that supports HDMI CEC.
if MEDIA_CEC_SUPPORT
+source "drivers/media/cec/i2c/Kconfig"
source "drivers/media/cec/platform/Kconfig"
source "drivers/media/cec/usb/Kconfig"
endif
diff --git a/drivers/media/cec/Makefile b/drivers/media/cec/Makefile
index 74e80e1b3571..23539339bc81 100644
--- a/drivers/media/cec/Makefile
+++ b/drivers/media/cec/Makefile
@@ -1,2 +1,2 @@
# SPDX-License-Identifier: GPL-2.0
-obj-y += core/ platform/ usb/
+obj-y += core/ i2c/ platform/ usb/
diff --git a/drivers/media/cec/i2c/Kconfig b/drivers/media/cec/i2c/Kconfig
new file mode 100644
index 000000000000..e445ca2110b3
--- /dev/null
+++ b/drivers/media/cec/i2c/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# I2C drivers
+
+config CEC_CH7322
+ tristate "Chrontel CH7322 CEC controller"
+ select I2C
+ select REGMAP_I2C
+ select CEC_CORE
+ help
+ This is a driver for the Chrontel CH7322 CEC controller. It uses the
+ generic CEC framework interface.
+ CEC bus is present in the HDMI connector and enables communication
+ between compatible devices.
diff --git a/drivers/media/cec/i2c/Makefile b/drivers/media/cec/i2c/Makefile
new file mode 100644
index 000000000000..d7496dfd0fa4
--- /dev/null
+++ b/drivers/media/cec/i2c/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the CEC I2C device drivers.
+#
+obj-$(CONFIG_CEC_CH7322) += ch7322.o
diff --git a/drivers/media/cec/i2c/ch7322.c b/drivers/media/cec/i2c/ch7322.c
new file mode 100644
index 000000000000..a9d66ec26440
--- /dev/null
+++ b/drivers/media/cec/i2c/ch7322.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Chrontel CH7322 CEC Controller
+ *
+ * Copyright 2020 Google LLC.
+ */
+#include <linux/cec.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <media/cec.h>
+
+#define CH7322_WRITE 0x00
+#define CH7322_WRITE_MSENT 0x80
+#define CH7322_WRITE_BOK 0x40
+#define CH7322_WRITE_NMASK 0x0F
+
+/* Write buffer is 0x01-0x10 */
+#define CH7322_WRBUF 0x01
+#define CH7322_WRBUF_LEN 0x10
+
+#define CH7322_READ 0x40
+#define CH7322_READ_NRDT 0x80
+#define CH7322_READ_MSENT 0x20
+#define CH7322_READ_NMASK 0x0F
+
+/* Read buffer is 0x41-0x50 */
+#define CH7322_RDBUF 0x41
+#define CH7322_RDBUF_LEN 0x10
+
+#define CH7322_MODE 0x11
+#define CH7322_MODE_AUTO 0x78
+#define CH7322_MODE_SW 0xB5
+
+#define CH7322_RESET 0x12
+#define CH7322_RESET_RST 0x00
+
+#define CH7322_POWER 0x13
+
+#define CH7322_CFG0 0x17
+#define CH7322_CFG0_EOBEN 0x40
+#define CH7322_CFG0_PEOB 0x20
+#define CH7322_CFG0_CLRSPP 0x10
+#define CH7322_CFG0_FLOW 0x08
+
+#define CH7322_CFG1 0x1A
+#define CH7322_CFG1_STDBYO 0x04
+#define CH7322_CFG1_HPBP 0x02
+#define CH7322_CFG1_PIO 0x01
+
+#define CH7322_INTCTL 0x1B
+#define CH7322_INTCTL_INTPB 0x80
+#define CH7322_INTCTL_STDBY 0x40
+#define CH7322_INTCTL_HPDFALL 0x20
+#define CH7322_INTCTL_HPDRISE 0x10
+#define CH7322_INTCTL_RXMSG 0x08
+#define CH7322_INTCTL_TXMSG 0x04
+#define CH7322_INTCTL_NEWPHA 0x02
+#define CH7322_INTCTL_ERROR 0x01
+
+#define CH7322_DVCLKFNH 0x1D
+#define CH7322_DVCLKFNL 0x1E
+
+#define CH7322_CTL 0x31
+#define CH7322_CTL_FSTDBY 0x80
+#define CH7322_CTL_PLSEN 0x40
+#define CH7322_CTL_PLSPB 0x20
+#define CH7322_CTL_SPADL 0x10
+#define CH7322_CTL_HINIT 0x08
+#define CH7322_CTL_WPHYA 0x04
+#define CH7322_CTL_H1T 0x02
+#define CH7322_CTL_S1T 0x01
+
+#define CH7322_PAWH 0x32
+#define CH7322_PAWL 0x32
+
+#define CH7322_ADDLR 0x3D
+#define CH7322_ADDLR_HPD 0x80
+#define CH7322_ADDLR_MASK 0x0F
+
+#define CH7322_INTDATA 0x3E
+#define CH7322_INTDATA_MODE 0x80
+#define CH7322_INTDATA_STDBY 0x40
+#define CH7322_INTDATA_HPDFALL 0x20
+#define CH7322_INTDATA_HPDRISE 0x10
+#define CH7322_INTDATA_RXMSG 0x08
+#define CH7322_INTDATA_TXMSG 0x04
+#define CH7322_INTDATA_NEWPHA 0x02
+#define CH7322_INTDATA_ERROR 0x01
+
+#define CH7322_EVENT 0x3F
+#define CH7322_EVENT_TXERR 0x80
+#define CH7322_EVENT_HRST 0x40
+#define CH7322_EVENT_HFST 0x20
+#define CH7322_EVENT_PHACHG 0x10
+#define CH7322_EVENT_ACTST 0x08
+#define CH7322_EVENT_PHARDY 0x04
+#define CH7322_EVENT_BSOK 0x02
+#define CH7322_EVENT_ERRADCF 0x01
+
+#define CH7322_DID 0x51
+#define CH7322_DID_CH7322 0x5B
+#define CH7322_DID_CH7323 0x5F
+
+#define CH7322_REVISIONID 0x52
+
+#define CH7322_PARH 0x53
+#define CH7322_PARL 0x54
+
+#define CH7322_IOCFG2 0x75
+#define CH7322_IOCFG_CIO 0x80
+#define CH7322_IOCFG_IOCFGMASK 0x78
+#define CH7322_IOCFG_AUDIO 0x04
+#define CH7322_IOCFG_SPAMST 0x02
+#define CH7322_IOCFG_SPAMSP 0x01
+
+#define CH7322_CTL3 0x7B
+#define CH7322_CTL3_SWENA 0x80
+#define CH7322_CTL3_FC_INIT 0x40
+#define CH7322_CTL3_SML_FL 0x20
+#define CH7322_CTL3_SM_RDST 0x10
+#define CH7322_CTL3_SPP_CIAH 0x08
+#define CH7322_CTL3_SPP_CIAL 0x04
+#define CH7322_CTL3_SPP_ACTH 0x02
+#define CH7322_CTL3_SPP_ACTL 0x01
+
+struct ch7322 {
+ struct i2c_client *i2c;
+ struct regmap *regmap;
+ struct cec_adapter *cec;
+ struct mutex mutex;
+ bool nack;
+};
+
+static const struct regmap_config ch7322_regmap = {
+ .reg_bits = 8,
+ .val_bits = 8,
+ .disable_locking = true,
+};
+
+static int ch7322_send_message(struct ch7322 *ch7322, const struct cec_msg *msg)
+{
+ unsigned int val;
+ unsigned int len = msg->len;
+ int ret = 0;
+ int i;
+
+ WARN_ON(!mutex_is_locked(&ch7322->mutex));
+
+ if (len > CH7322_WRBUF_LEN || len < 1)
+ return -EINVAL;
+
+ ret = regmap_read(ch7322->regmap, CH7322_WRITE, &val);
+ if (ret)
+ return ret;
+
+ /* Buffer not ready */
+ if (!(val & CH7322_WRITE_MSENT))
+ return -EBUSY;
+
+ /* error status is flipped for logical address broadcast */
+ ch7322->nack = cec_msg_initiator(msg) == cec_msg_destination(msg);
+
+ ret = regmap_write(ch7322->regmap, CH7322_WRITE, len - 1);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < len; i++) {
+ ret = regmap_write(ch7322->regmap,
+ CH7322_WRBUF + i, msg->msg[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int ch7322_receive_message(struct ch7322 *ch7322, struct cec_msg *msg)
+{
+ unsigned int val;
+ int ret = 0;
+ int i;
+
+ WARN_ON(!mutex_is_locked(&ch7322->mutex));
+
+ ret = regmap_read(ch7322->regmap, CH7322_READ, &val);
+ if (ret)
+ return ret;
+
+ /* Message not ready */
+ if (!(val & CH7322_READ_NRDT))
+ return -EIO;
+
+ msg->len = (val & CH7322_READ_NMASK) + 1;
+
+ /* Read entire RDBUF to clear state */
+ for (i = 0; i < CH7322_RDBUF_LEN; i++) {
+ ret = regmap_read(ch7322->regmap, CH7322_RDBUF + i, &val);
+ if (ret)
+ return ret;
+ msg->msg[i] = (u8) val;
+ }
+
+ return 0;
+}
+
+static void ch7322_tx_done(struct ch7322 *ch7322)
+{
+ int ret;
+ unsigned int val;
+ u8 status;
+
+ mutex_lock(&ch7322->mutex);
+ ret = regmap_read(ch7322->regmap, CH7322_WRITE, &val);
+ mutex_unlock(&ch7322->mutex);
+
+ if (ret) {
+ dev_err(&ch7322->i2c->dev, "transmit error\n");
+ status = CEC_TX_STATUS_ERROR;
+ } else if (val & CH7322_WRITE_BOK) {
+ status = ch7322->nack ? CEC_TX_STATUS_NACK : CEC_TX_STATUS_OK;
+ } else {
+ status = ch7322->nack ? CEC_TX_STATUS_OK : CEC_TX_STATUS_NACK;
+ }
+
+ dev_dbg(&ch7322->i2c->dev, "cec transmit done: %d\n", status);
+
+ cec_transmit_attempt_done(ch7322->cec, status);
+}
+
+static void ch7322_rx_done(struct ch7322 *ch7322)
+{
+ struct cec_msg msg;
+ int ret;
+
+ mutex_lock(&ch7322->mutex);
+ ret = ch7322_receive_message(ch7322, &msg);
+ mutex_unlock(&ch7322->mutex);
+
+ if (ret) {
+ dev_err(&ch7322->i2c->dev, "cec receive error: %d\n", ret);
+ return;
+ }
+
+ dev_dbg(&ch7322->i2c->dev, "cec receive: %x->%x: %x\n",
+ cec_msg_initiator(&msg), cec_msg_destination(&msg),
+ cec_msg_opcode(&msg));
+
+ cec_received_msg(ch7322->cec, &msg);
+}
+
+static void ch7322_phys_addr(struct ch7322 *ch7322)
+{
+ unsigned int pah, pal;
+ int ret = 0;
+
+ mutex_lock(&ch7322->mutex);
+ ret |= regmap_read(ch7322->regmap, CH7322_PARH, &pah);
+ ret |= regmap_read(ch7322->regmap, CH7322_PARL, &pal);
+ mutex_unlock(&ch7322->mutex);
+
+ if (ret)
+ dev_err(&ch7322->i2c->dev, "phys addr error\n");
+ else
+ cec_s_phys_addr(ch7322->cec, pal | (pah << 8), false);
+}
+
+static void ch7322_handle_events(struct ch7322 *ch7322)
+{
+ unsigned int data;
+
+ mutex_lock(&ch7322->mutex);
+ (void) regmap_read(ch7322->regmap, CH7322_INTDATA, &data);
+ (void) regmap_write(ch7322->regmap, CH7322_INTDATA, data);
+ mutex_unlock(&ch7322->mutex);
+
+ if (data & CH7322_INTDATA_HPDFALL) {
+ dev_dbg(&ch7322->i2c->dev, "hpdfall\n");
+ cec_phys_addr_invalidate(ch7322->cec);
+ }
+
+ if (data & CH7322_INTDATA_TXMSG)
+ ch7322_tx_done(ch7322);
+
+ if (data & CH7322_INTDATA_RXMSG)
+ ch7322_rx_done(ch7322);
+
+ if (data & CH7322_INTDATA_NEWPHA)
+ ch7322_phys_addr(ch7322);
+
+ if (data & CH7322_INTDATA_ERROR)
+ dev_err(&ch7322->i2c->dev, "unknown error\n");
+}
+
+
+static irqreturn_t ch7322_irq(int irq, void *dev)
+{
+ struct ch7322 *ch7322 = dev;
+
+ ch7322_handle_events(ch7322);
+
+ return IRQ_HANDLED;
+}
+
+static int ch7322_cec_adap_enable(struct cec_adapter *adap, bool enable)
+{
+ return 0;
+}
+
+static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
+{
+ struct ch7322 *ch7322 = cec_get_drvdata(adap);
+
+ dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
+
+ return 0;
+}
+
+static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
+ u32 signal_free_time, struct cec_msg *msg)
+{
+ struct ch7322 *ch7322 = cec_get_drvdata(adap);
+ int ret;
+
+ dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
+ cec_msg_initiator(msg), cec_msg_destination(msg),
+ cec_msg_opcode(msg));
+
+ mutex_lock(&ch7322->mutex);
+ ret = ch7322_send_message(ch7322, msg);
+ mutex_unlock(&ch7322->mutex);
+
+ return ret;
+}
+
+static const struct cec_adap_ops ch7322_cec_adap_ops = {
+ .adap_enable = ch7322_cec_adap_enable,
+ .adap_log_addr = ch7322_cec_adap_log_addr,
+ .adap_transmit = ch7322_cec_adap_transmit,
+};
+
+static int ch7322_probe(struct i2c_client *client)
+{
+ struct ch7322 *ch7322;
+ int ret;
+ unsigned int val;
+
+ ch7322 = devm_kzalloc(&client->dev, sizeof(*ch7322), GFP_KERNEL);
+ if (!ch7322)
+ return -ENOMEM;
+
+ ch7322->regmap = devm_regmap_init_i2c(client, &ch7322_regmap);
+ if (IS_ERR(ch7322->regmap))
+ return PTR_ERR(ch7322->regmap);
+
+ mutex_init(&ch7322->mutex);
+ ch7322->i2c = client;
+ ch7322->nack = false;
+
+ i2c_set_clientdata(client, ch7322);
+
+ mutex_lock(&ch7322->mutex);
+ ret = regmap_read(ch7322->regmap, CH7322_DID, &val);
+ mutex_unlock(&ch7322->mutex);
+
+ if (ret < 0)
+ goto err_mutex;
+
+ if (val != CH7322_DID_CH7322) {
+ ret = -ENOTSUPP;
+ goto err_mutex;
+ }
+
+ mutex_lock(&ch7322->mutex);
+ ret = regmap_write(ch7322->regmap, CH7322_MODE, CH7322_MODE_SW);
+ mutex_unlock(&ch7322->mutex);
+
+ if (ret < 0)
+ goto err_mutex;
+
+ ch7322->cec = cec_allocate_adapter(&ch7322_cec_adap_ops, ch7322,
+ dev_name(&client->dev), CEC_CAP_DEFAULTS, 1);
+
+ if (IS_ERR(ch7322->cec)) {
+ ret = PTR_ERR(ch7322->cec);
+ goto err_mutex;
+ }
+
+ ret = cec_register_adapter(ch7322->cec, &client->dev);
+ if (ret) {
+ cec_delete_adapter(ch7322->cec);
+ goto err_mutex;
+ }
+
+ ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+ ch7322_irq, IRQF_ONESHOT | IRQF_TRIGGER_RISING,
+ client->name, ch7322);
+ if (ret < 0)
+ goto err_cec;
+
+ /* Clear events & enable interrupt */
+ ch7322_handle_events(ch7322);
+ ret = regmap_write(ch7322->regmap, CH7322_CFG1, 0);
+ if (ret)
+ goto err_cec;
+
+ dev_info(&client->dev, "device registered\n");
+
+ return 0;
+
+err_cec:
+ cec_unregister_adapter(ch7322->cec);
+err_mutex:
+ mutex_destroy(&ch7322->mutex);
+ return ret;
+}
+
+static int ch7322_remove(struct i2c_client *client)
+{
+ struct ch7322 *ch7322 = i2c_get_clientdata(client);
+
+ /* Disable interrupt */
+ (void) regmap_write(ch7322->regmap, CH7322_CFG1,
+ CH7322_CFG1_STDBYO | CH7322_CFG1_HPBP);
+
+ cec_unregister_adapter(ch7322->cec);
+ mutex_destroy(&ch7322->mutex);
+
+ dev_info(&client->dev, "device unregistered\n");
+
+ return 0;
+}
+
+static const struct of_device_id ch7322_of_match[] = {
+ { .compatible = "chrontel,ch7322", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, ch7322_of_match);
+
+static struct i2c_driver ch7322_i2c_driver = {
+ .driver = {
+ .name = "ch7322",
+ .of_match_table = of_match_ptr(ch7322_of_match),
+ },
+ .probe_new = ch7322_probe,
+ .remove = ch7322_remove,
+};
+
+module_i2c_driver(ch7322_i2c_driver);
+
+MODULE_DESCRIPTION("Chrontel CH7322 CEC Controller Driver");
+MODULE_AUTHOR("Jeff Chase <jnchase@google.com>");
+MODULE_LICENSE("GPL");
--
2.26.2.303.gf8c07b1a785-goog
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-24 5:38 ` [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver Jeff Chase
@ 2020-04-24 12:24 ` Hans Verkuil
2020-04-24 19:33 ` Jeff Chase
2020-04-25 9:17 ` Hans Verkuil
1 sibling, 1 reply; 11+ messages in thread
From: Hans Verkuil @ 2020-04-24 12:24 UTC (permalink / raw
To: Jeff Chase, linux-media; +Cc: mchehab, robh+dt, devicetree
Hi Jeff,
Thank you for this driver.
I'll give a quick review below.
For v2 please run 'checkpatch.pl --strict' over the patches and fix any
issues.
Is the register documentation available somewhere? I only found the product brief.
On 24/04/2020 07:38, Jeff Chase wrote:
> Add a CEC device driver for the Chrontel ch7322 CEC conroller.
> This is an I2C device capable of sending and receiving CEC messages.
>
> Signed-off-by: Jeff Chase <jnchase@google.com>
> ---
> MAINTAINERS | 7 +
> drivers/media/cec/Kconfig | 1 +
> drivers/media/cec/Makefile | 2 +-
> drivers/media/cec/i2c/Kconfig | 14 +
> drivers/media/cec/i2c/Makefile | 5 +
> drivers/media/cec/i2c/ch7322.c | 455 +++++++++++++++++++++++++++++++++
> 6 files changed, 483 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/cec/i2c/Kconfig
> create mode 100644 drivers/media/cec/i2c/Makefile
> create mode 100644 drivers/media/cec/i2c/ch7322.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d633a131dcd7..d43f5146cad6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4047,6 +4047,13 @@ F: drivers/power/supply/cros_usbpd-charger.c
> N: cros_ec
> N: cros-ec
>
> +CHRONTEL CH7322 CEC DRIVER
> +M: Jeff Chase <jnchase@google.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +T: git git://linuxtv.org/media_tree.git
> +F: drivers/media/cec/i2c/ch7322.c
> +
> CIRRUS LOGIC AUDIO CODEC DRIVERS
> M: James Schulman <james.schulman@cirrus.com>
> M: David Rhodes <david.rhodes@cirrus.com>
> diff --git a/drivers/media/cec/Kconfig b/drivers/media/cec/Kconfig
> index eea74b7cfa8c..3e934aa239ab 100644
> --- a/drivers/media/cec/Kconfig
> +++ b/drivers/media/cec/Kconfig
> @@ -33,6 +33,7 @@ menuconfig MEDIA_CEC_SUPPORT
> adapter that supports HDMI CEC.
>
> if MEDIA_CEC_SUPPORT
> +source "drivers/media/cec/i2c/Kconfig"
> source "drivers/media/cec/platform/Kconfig"
> source "drivers/media/cec/usb/Kconfig"
> endif
> diff --git a/drivers/media/cec/Makefile b/drivers/media/cec/Makefile
> index 74e80e1b3571..23539339bc81 100644
> --- a/drivers/media/cec/Makefile
> +++ b/drivers/media/cec/Makefile
> @@ -1,2 +1,2 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-y += core/ platform/ usb/
> +obj-y += core/ i2c/ platform/ usb/
> diff --git a/drivers/media/cec/i2c/Kconfig b/drivers/media/cec/i2c/Kconfig
> new file mode 100644
> index 000000000000..e445ca2110b3
> --- /dev/null
> +++ b/drivers/media/cec/i2c/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# I2C drivers
> +
> +config CEC_CH7322
> + tristate "Chrontel CH7322 CEC controller"
> + select I2C
> + select REGMAP_I2C
> + select CEC_CORE
> + help
> + This is a driver for the Chrontel CH7322 CEC controller. It uses the
> + generic CEC framework interface.
> + CEC bus is present in the HDMI connector and enables communication
> + between compatible devices.
Reading the product brief of this device it seems that there are two modes:
it can handle HPD/EDID itself, or that can be left to the HDMI receiver/transmitter.
From what I can gather this driver only supports the first case. This should be
documented at least in the source code.
> diff --git a/drivers/media/cec/i2c/Makefile b/drivers/media/cec/i2c/Makefile
> new file mode 100644
> index 000000000000..d7496dfd0fa4
> --- /dev/null
> +++ b/drivers/media/cec/i2c/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for the CEC I2C device drivers.
> +#
> +obj-$(CONFIG_CEC_CH7322) += ch7322.o
> diff --git a/drivers/media/cec/i2c/ch7322.c b/drivers/media/cec/i2c/ch7322.c
> new file mode 100644
> index 000000000000..a9d66ec26440
> --- /dev/null
> +++ b/drivers/media/cec/i2c/ch7322.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Chrontel CH7322 CEC Controller
> + *
> + * Copyright 2020 Google LLC.
> + */
> +#include <linux/cec.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <media/cec.h>
> +
> +#define CH7322_WRITE 0x00
> +#define CH7322_WRITE_MSENT 0x80
> +#define CH7322_WRITE_BOK 0x40
> +#define CH7322_WRITE_NMASK 0x0F
> +
> +/* Write buffer is 0x01-0x10 */
> +#define CH7322_WRBUF 0x01
> +#define CH7322_WRBUF_LEN 0x10
> +
> +#define CH7322_READ 0x40
> +#define CH7322_READ_NRDT 0x80
> +#define CH7322_READ_MSENT 0x20
> +#define CH7322_READ_NMASK 0x0F
> +
> +/* Read buffer is 0x41-0x50 */
> +#define CH7322_RDBUF 0x41
> +#define CH7322_RDBUF_LEN 0x10
> +
> +#define CH7322_MODE 0x11
> +#define CH7322_MODE_AUTO 0x78
> +#define CH7322_MODE_SW 0xB5
> +
> +#define CH7322_RESET 0x12
> +#define CH7322_RESET_RST 0x00
> +
> +#define CH7322_POWER 0x13
> +
> +#define CH7322_CFG0 0x17
> +#define CH7322_CFG0_EOBEN 0x40
> +#define CH7322_CFG0_PEOB 0x20
> +#define CH7322_CFG0_CLRSPP 0x10
> +#define CH7322_CFG0_FLOW 0x08
> +
> +#define CH7322_CFG1 0x1A
> +#define CH7322_CFG1_STDBYO 0x04
> +#define CH7322_CFG1_HPBP 0x02
> +#define CH7322_CFG1_PIO 0x01
> +
> +#define CH7322_INTCTL 0x1B
> +#define CH7322_INTCTL_INTPB 0x80
> +#define CH7322_INTCTL_STDBY 0x40
> +#define CH7322_INTCTL_HPDFALL 0x20
> +#define CH7322_INTCTL_HPDRISE 0x10
> +#define CH7322_INTCTL_RXMSG 0x08
> +#define CH7322_INTCTL_TXMSG 0x04
> +#define CH7322_INTCTL_NEWPHA 0x02
> +#define CH7322_INTCTL_ERROR 0x01
> +
> +#define CH7322_DVCLKFNH 0x1D
> +#define CH7322_DVCLKFNL 0x1E
> +
> +#define CH7322_CTL 0x31
> +#define CH7322_CTL_FSTDBY 0x80
> +#define CH7322_CTL_PLSEN 0x40
> +#define CH7322_CTL_PLSPB 0x20
> +#define CH7322_CTL_SPADL 0x10
> +#define CH7322_CTL_HINIT 0x08
> +#define CH7322_CTL_WPHYA 0x04
> +#define CH7322_CTL_H1T 0x02
> +#define CH7322_CTL_S1T 0x01
> +
> +#define CH7322_PAWH 0x32
> +#define CH7322_PAWL 0x32
> +
> +#define CH7322_ADDLR 0x3D
> +#define CH7322_ADDLR_HPD 0x80
> +#define CH7322_ADDLR_MASK 0x0F
> +
> +#define CH7322_INTDATA 0x3E
> +#define CH7322_INTDATA_MODE 0x80
> +#define CH7322_INTDATA_STDBY 0x40
> +#define CH7322_INTDATA_HPDFALL 0x20
> +#define CH7322_INTDATA_HPDRISE 0x10
> +#define CH7322_INTDATA_RXMSG 0x08
> +#define CH7322_INTDATA_TXMSG 0x04
> +#define CH7322_INTDATA_NEWPHA 0x02
> +#define CH7322_INTDATA_ERROR 0x01
> +
> +#define CH7322_EVENT 0x3F
> +#define CH7322_EVENT_TXERR 0x80
> +#define CH7322_EVENT_HRST 0x40
> +#define CH7322_EVENT_HFST 0x20
> +#define CH7322_EVENT_PHACHG 0x10
> +#define CH7322_EVENT_ACTST 0x08
> +#define CH7322_EVENT_PHARDY 0x04
> +#define CH7322_EVENT_BSOK 0x02
> +#define CH7322_EVENT_ERRADCF 0x01
> +
> +#define CH7322_DID 0x51
> +#define CH7322_DID_CH7322 0x5B
> +#define CH7322_DID_CH7323 0x5F
> +
> +#define CH7322_REVISIONID 0x52
> +
> +#define CH7322_PARH 0x53
> +#define CH7322_PARL 0x54
> +
> +#define CH7322_IOCFG2 0x75
> +#define CH7322_IOCFG_CIO 0x80
> +#define CH7322_IOCFG_IOCFGMASK 0x78
> +#define CH7322_IOCFG_AUDIO 0x04
> +#define CH7322_IOCFG_SPAMST 0x02
> +#define CH7322_IOCFG_SPAMSP 0x01
> +
> +#define CH7322_CTL3 0x7B
> +#define CH7322_CTL3_SWENA 0x80
> +#define CH7322_CTL3_FC_INIT 0x40
> +#define CH7322_CTL3_SML_FL 0x20
> +#define CH7322_CTL3_SM_RDST 0x10
> +#define CH7322_CTL3_SPP_CIAH 0x08
> +#define CH7322_CTL3_SPP_CIAL 0x04
> +#define CH7322_CTL3_SPP_ACTH 0x02
> +#define CH7322_CTL3_SPP_ACTL 0x01
> +
> +struct ch7322 {
> + struct i2c_client *i2c;
> + struct regmap *regmap;
> + struct cec_adapter *cec;
> + struct mutex mutex;
> + bool nack;
> +};
> +
> +static const struct regmap_config ch7322_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .disable_locking = true,
> +};
> +
> +static int ch7322_send_message(struct ch7322 *ch7322, const struct cec_msg *msg)
> +{
> + unsigned int val;
> + unsigned int len = msg->len;
> + int ret = 0;
> + int i;
> +
> + WARN_ON(!mutex_is_locked(&ch7322->mutex));
> +
> + if (len > CH7322_WRBUF_LEN || len < 1)
> + return -EINVAL;
> +
> + ret = regmap_read(ch7322->regmap, CH7322_WRITE, &val);
> + if (ret)
> + return ret;
> +
> + /* Buffer not ready */
> + if (!(val & CH7322_WRITE_MSENT))
> + return -EBUSY;
> +
> + /* error status is flipped for logical address broadcast */
> + ch7322->nack = cec_msg_initiator(msg) == cec_msg_destination(msg);
> +
> + ret = regmap_write(ch7322->regmap, CH7322_WRITE, len - 1);
> + if (ret)
> + return ret;
> +
> + for (i = 0; i < len; i++) {
> + ret = regmap_write(ch7322->regmap,
> + CH7322_WRBUF + i, msg->msg[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int ch7322_receive_message(struct ch7322 *ch7322, struct cec_msg *msg)
> +{
> + unsigned int val;
> + int ret = 0;
> + int i;
> +
> + WARN_ON(!mutex_is_locked(&ch7322->mutex));
> +
> + ret = regmap_read(ch7322->regmap, CH7322_READ, &val);
> + if (ret)
> + return ret;
> +
> + /* Message not ready */
> + if (!(val & CH7322_READ_NRDT))
> + return -EIO;
> +
> + msg->len = (val & CH7322_READ_NMASK) + 1;
> +
> + /* Read entire RDBUF to clear state */
> + for (i = 0; i < CH7322_RDBUF_LEN; i++) {
> + ret = regmap_read(ch7322->regmap, CH7322_RDBUF + i, &val);
> + if (ret)
> + return ret;
> + msg->msg[i] = (u8) val;
> + }
> +
> + return 0;
> +}
> +
> +static void ch7322_tx_done(struct ch7322 *ch7322)
> +{
> + int ret;
> + unsigned int val;
> + u8 status;
> +
> + mutex_lock(&ch7322->mutex);
> + ret = regmap_read(ch7322->regmap, CH7322_WRITE, &val);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (ret) {
> + dev_err(&ch7322->i2c->dev, "transmit error\n");
> + status = CEC_TX_STATUS_ERROR;
> + } else if (val & CH7322_WRITE_BOK) {
> + status = ch7322->nack ? CEC_TX_STATUS_NACK : CEC_TX_STATUS_OK;
> + } else {
> + status = ch7322->nack ? CEC_TX_STATUS_OK : CEC_TX_STATUS_NACK;
> + }
The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
or Low Drive conditions? Just checking, not all hardware has support for that.
> +
> + dev_dbg(&ch7322->i2c->dev, "cec transmit done: %d\n", status);
> +
> + cec_transmit_attempt_done(ch7322->cec, status);
> +}
> +
> +static void ch7322_rx_done(struct ch7322 *ch7322)
> +{
> + struct cec_msg msg;
> + int ret;
> +
> + mutex_lock(&ch7322->mutex);
> + ret = ch7322_receive_message(ch7322, &msg);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (ret) {
> + dev_err(&ch7322->i2c->dev, "cec receive error: %d\n", ret);
> + return;
> + }
> +
> + dev_dbg(&ch7322->i2c->dev, "cec receive: %x->%x: %x\n",
> + cec_msg_initiator(&msg), cec_msg_destination(&msg),
> + cec_msg_opcode(&msg));
The cec framework has similar logging, so I'd remove this.
> +
> + cec_received_msg(ch7322->cec, &msg);
> +}
> +
> +static void ch7322_phys_addr(struct ch7322 *ch7322)
> +{
> + unsigned int pah, pal;
> + int ret = 0;
> +
> + mutex_lock(&ch7322->mutex);
> + ret |= regmap_read(ch7322->regmap, CH7322_PARH, &pah);
> + ret |= regmap_read(ch7322->regmap, CH7322_PARL, &pal);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (ret)
> + dev_err(&ch7322->i2c->dev, "phys addr error\n");
> + else
> + cec_s_phys_addr(ch7322->cec, pal | (pah << 8), false);
> +}
> +
> +static void ch7322_handle_events(struct ch7322 *ch7322)
> +{
> + unsigned int data;
> +
> + mutex_lock(&ch7322->mutex);
> + (void) regmap_read(ch7322->regmap, CH7322_INTDATA, &data);
> + (void) regmap_write(ch7322->regmap, CH7322_INTDATA, data);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (data & CH7322_INTDATA_HPDFALL) {
> + dev_dbg(&ch7322->i2c->dev, "hpdfall\n");
I think this dbg line can be dropped since the cec framework already
logs when cec_phys_addr_invalidate is called.
> + cec_phys_addr_invalidate(ch7322->cec);
> + }
> +
> + if (data & CH7322_INTDATA_TXMSG)
> + ch7322_tx_done(ch7322);
> +
> + if (data & CH7322_INTDATA_RXMSG)
> + ch7322_rx_done(ch7322);
> +
> + if (data & CH7322_INTDATA_NEWPHA)
> + ch7322_phys_addr(ch7322);
> +
> + if (data & CH7322_INTDATA_ERROR)
> + dev_err(&ch7322->i2c->dev, "unknown error\n");
> +}
> +
> +
> +static irqreturn_t ch7322_irq(int irq, void *dev)
> +{
> + struct ch7322 *ch7322 = dev;
> +
> + ch7322_handle_events(ch7322);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int ch7322_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> + return 0;
> +}
> +
> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
> +{
> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> +
> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
> +
> + return 0;
This can't be right. I expect that logical addresses are set/cleared here,
because the device needs to know that so that it can ignore messages not
intended for it.
> +}
> +
> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> + u32 signal_free_time, struct cec_msg *msg)
> +{
Does the hardware correctly handle Signal Free Time? If this isn't handled right
then one CEC device can flood the CEC bus, preventing anyone else from using it.
In some devices it has to be programmed, in others it is hardwired.
> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> + int ret;
> +
> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
> + cec_msg_initiator(msg), cec_msg_destination(msg),
> + cec_msg_opcode(msg));
> +
> + mutex_lock(&ch7322->mutex);
> + ret = ch7322_send_message(ch7322, msg);
> + mutex_unlock(&ch7322->mutex);
> +
> + return ret;
> +}
> +
> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
> + .adap_enable = ch7322_cec_adap_enable,
> + .adap_log_addr = ch7322_cec_adap_log_addr,
> + .adap_transmit = ch7322_cec_adap_transmit,
If the HW supports CEC monitoring (aka snooping), then I recommend that
adap_monitor_all_enable is also implemented. It's very useful for debugging
CEC in userspace. Not all HW supports it, though.
> +};
> +
> +static int ch7322_probe(struct i2c_client *client)
> +{
> + struct ch7322 *ch7322;
> + int ret;
> + unsigned int val;
> +
> + ch7322 = devm_kzalloc(&client->dev, sizeof(*ch7322), GFP_KERNEL);
> + if (!ch7322)
> + return -ENOMEM;
> +
> + ch7322->regmap = devm_regmap_init_i2c(client, &ch7322_regmap);
> + if (IS_ERR(ch7322->regmap))
> + return PTR_ERR(ch7322->regmap);
> +
> + mutex_init(&ch7322->mutex);
> + ch7322->i2c = client;
> + ch7322->nack = false;
> +
> + i2c_set_clientdata(client, ch7322);
> +
> + mutex_lock(&ch7322->mutex);
> + ret = regmap_read(ch7322->regmap, CH7322_DID, &val);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (ret < 0)
> + goto err_mutex;
> +
> + if (val != CH7322_DID_CH7322) {
> + ret = -ENOTSUPP;
> + goto err_mutex;
> + }
> +
> + mutex_lock(&ch7322->mutex);
> + ret = regmap_write(ch7322->regmap, CH7322_MODE, CH7322_MODE_SW);
> + mutex_unlock(&ch7322->mutex);
> +
> + if (ret < 0)
> + goto err_mutex;
> +
> + ch7322->cec = cec_allocate_adapter(&ch7322_cec_adap_ops, ch7322,
> + dev_name(&client->dev), CEC_CAP_DEFAULTS, 1);
> +
> + if (IS_ERR(ch7322->cec)) {
> + ret = PTR_ERR(ch7322->cec);
> + goto err_mutex;
> + }
> +
> + ret = cec_register_adapter(ch7322->cec, &client->dev);
> + if (ret) {
> + cec_delete_adapter(ch7322->cec);
> + goto err_mutex;
> + }
> +
> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + ch7322_irq, IRQF_ONESHOT | IRQF_TRIGGER_RISING,
> + client->name, ch7322);
> + if (ret < 0)
> + goto err_cec;
> +
> + /* Clear events & enable interrupt */
> + ch7322_handle_events(ch7322);
> + ret = regmap_write(ch7322->regmap, CH7322_CFG1, 0);
> + if (ret)
> + goto err_cec;
> +
> + dev_info(&client->dev, "device registered\n");
> +
> + return 0;
> +
> +err_cec:
> + cec_unregister_adapter(ch7322->cec);
> +err_mutex:
> + mutex_destroy(&ch7322->mutex);
> + return ret;
> +}
> +
> +static int ch7322_remove(struct i2c_client *client)
> +{
> + struct ch7322 *ch7322 = i2c_get_clientdata(client);
> +
> + /* Disable interrupt */
> + (void) regmap_write(ch7322->regmap, CH7322_CFG1,
> + CH7322_CFG1_STDBYO | CH7322_CFG1_HPBP);
> +
> + cec_unregister_adapter(ch7322->cec);
> + mutex_destroy(&ch7322->mutex);
> +
> + dev_info(&client->dev, "device unregistered\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id ch7322_of_match[] = {
> + { .compatible = "chrontel,ch7322", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, ch7322_of_match);
> +
> +static struct i2c_driver ch7322_i2c_driver = {
> + .driver = {
> + .name = "ch7322",
> + .of_match_table = of_match_ptr(ch7322_of_match),
> + },
> + .probe_new = ch7322_probe,
> + .remove = ch7322_remove,
> +};
> +
> +module_i2c_driver(ch7322_i2c_driver);
> +
> +MODULE_DESCRIPTION("Chrontel CH7322 CEC Controller Driver");
> +MODULE_AUTHOR("Jeff Chase <jnchase@google.com>");
> +MODULE_LICENSE("GPL");
>
Regards,
Hans
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-24 12:24 ` Hans Verkuil
@ 2020-04-24 19:33 ` Jeff Chase
2020-04-25 9:16 ` Hans Verkuil
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Chase @ 2020-04-24 19:33 UTC (permalink / raw
To: Hans Verkuil; +Cc: linux-media, mchehab, robh+dt, devicetree
Hi Hans,
Thank you for the quick review.
> Is the register documentation available somewhere? I only found the product brief.
No, it's not publicly available.
> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
> or Low Drive conditions? Just checking, not all hardware has support for that.
Correct, message transmit completion just has a one-bit status.
> > +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
> > +{
> > + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> > +
> > + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
> > +
> > + return 0;
>
> This can't be right. I expect that logical addresses are set/cleared here,
> because the device needs to know that so that it can ignore messages not
> intended for it.
As far as I can tell the device doesn't filter based on logical
address. I'll have to save
the logical address to the driver and filter manually.
> > +}
> > +
> > +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> > + u32 signal_free_time, struct cec_msg *msg)
> > +{
>
> Does the hardware correctly handle Signal Free Time? If this isn't handled right
> then one CEC device can flood the CEC bus, preventing anyone else from using it.
>
> In some devices it has to be programmed, in others it is hardwired.
It must be hardwired -- I don't see a way to program it.
> > + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> > + int ret;
> > +
> > + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
> > + cec_msg_initiator(msg), cec_msg_destination(msg),
> > + cec_msg_opcode(msg));
> > +
> > + mutex_lock(&ch7322->mutex);
> > + ret = ch7322_send_message(ch7322, msg);
> > + mutex_unlock(&ch7322->mutex);
> > +
> > + return ret;
> > +}
> > +
> > +static const struct cec_adap_ops ch7322_cec_adap_ops = {
> > + .adap_enable = ch7322_cec_adap_enable,
> > + .adap_log_addr = ch7322_cec_adap_log_addr,
> > + .adap_transmit = ch7322_cec_adap_transmit,
>
> If the HW supports CEC monitoring (aka snooping), then I recommend that
> adap_monitor_all_enable is also implemented. It's very useful for debugging
> CEC in userspace. Not all HW supports it, though.
Okay, I'll add this along with the logical address filtering I mentioned above.
Thanks,
Jeff
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-24 19:33 ` Jeff Chase
@ 2020-04-25 9:16 ` Hans Verkuil
2020-04-25 9:31 ` Hans Verkuil
2020-05-05 23:26 ` Jeff Chase
0 siblings, 2 replies; 11+ messages in thread
From: Hans Verkuil @ 2020-04-25 9:16 UTC (permalink / raw
To: Jeff Chase; +Cc: linux-media, mchehab, robh+dt, devicetree
On 24/04/2020 21:33, Jeff Chase wrote:
> Hi Hans,
>
> Thank you for the quick review.
>
>> Is the register documentation available somewhere? I only found the product brief.
>
> No, it's not publicly available.
>
>> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
>> or Low Drive conditions? Just checking, not all hardware has support for that.
>
> Correct, message transmit completion just has a one-bit status.
>
>>> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
>>> +{
>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>> +
>>> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
>>> +
>>> + return 0;
>>
>> This can't be right. I expect that logical addresses are set/cleared here,
>> because the device needs to know that so that it can ignore messages not
>> intended for it.
>
> As far as I can tell the device doesn't filter based on logical
> address. I'll have to save
> the logical address to the driver and filter manually.
That can't be right. If this CEC adapter is assigned logical address 4, and
it has to Ack any received messages from other CEC devices with destination 4,
and ignore (i.e. not explicitly Ack) messages with other destinations.
If the CEC adapter wouldn't know what LA to use, then it would have to Ack
all messages, regardless of the destination, which would make this a complete
mess.
There must be a register that tells the CEC adapter which logical address(es)
should be Acked. It's usually a bitmask (one bit for each possible LA) or the
LA itself is stored.
It might be that you still receive all messages (in which case monitor_all
is effectively always enabled), but it really needs to be told which LAs should
be Acked.
Regards,
Hans
>
>>> +}
>>> +
>>> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>> + u32 signal_free_time, struct cec_msg *msg)
>>> +{
>>
>> Does the hardware correctly handle Signal Free Time? If this isn't handled right
>> then one CEC device can flood the CEC bus, preventing anyone else from using it.
>>
>> In some devices it has to be programmed, in others it is hardwired.
>
> It must be hardwired -- I don't see a way to program it.
>
>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>> + int ret;
>>> +
>>> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
>>> + cec_msg_initiator(msg), cec_msg_destination(msg),
>>> + cec_msg_opcode(msg));
>>> +
>>> + mutex_lock(&ch7322->mutex);
>>> + ret = ch7322_send_message(ch7322, msg);
>>> + mutex_unlock(&ch7322->mutex);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
>>> + .adap_enable = ch7322_cec_adap_enable,
>>> + .adap_log_addr = ch7322_cec_adap_log_addr,
>>> + .adap_transmit = ch7322_cec_adap_transmit,
>>
>> If the HW supports CEC monitoring (aka snooping), then I recommend that
>> adap_monitor_all_enable is also implemented. It's very useful for debugging
>> CEC in userspace. Not all HW supports it, though.
>
> Okay, I'll add this along with the logical address filtering I mentioned above.
>
> Thanks,
> Jeff
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-24 5:38 ` [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver Jeff Chase
2020-04-24 12:24 ` Hans Verkuil
@ 2020-04-25 9:17 ` Hans Verkuil
1 sibling, 0 replies; 11+ messages in thread
From: Hans Verkuil @ 2020-04-25 9:17 UTC (permalink / raw
To: Jeff Chase, linux-media; +Cc: mchehab, robh+dt, devicetree
On 24/04/2020 07:38, Jeff Chase wrote:
> Add a CEC device driver for the Chrontel ch7322 CEC conroller.
> This is an I2C device capable of sending and receiving CEC messages.
>
> Signed-off-by: Jeff Chase <jnchase@google.com>
> ---
> MAINTAINERS | 7 +
> drivers/media/cec/Kconfig | 1 +
> drivers/media/cec/Makefile | 2 +-
> drivers/media/cec/i2c/Kconfig | 14 +
> drivers/media/cec/i2c/Makefile | 5 +
> drivers/media/cec/i2c/ch7322.c | 455 +++++++++++++++++++++++++++++++++
> 6 files changed, 483 insertions(+), 1 deletion(-)
> create mode 100644 drivers/media/cec/i2c/Kconfig
> create mode 100644 drivers/media/cec/i2c/Makefile
> create mode 100644 drivers/media/cec/i2c/ch7322.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d633a131dcd7..d43f5146cad6 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4047,6 +4047,13 @@ F: drivers/power/supply/cros_usbpd-charger.c
> N: cros_ec
> N: cros-ec
>
> +CHRONTEL CH7322 CEC DRIVER
> +M: Jeff Chase <jnchase@google.com>
> +L: linux-media@vger.kernel.org
> +S: Maintained
> +T: git git://linuxtv.org/media_tree.git
> +F: drivers/media/cec/i2c/ch7322.c
> +
> CIRRUS LOGIC AUDIO CODEC DRIVERS
> M: James Schulman <james.schulman@cirrus.com>
> M: David Rhodes <david.rhodes@cirrus.com>
> diff --git a/drivers/media/cec/Kconfig b/drivers/media/cec/Kconfig
> index eea74b7cfa8c..3e934aa239ab 100644
> --- a/drivers/media/cec/Kconfig
> +++ b/drivers/media/cec/Kconfig
> @@ -33,6 +33,7 @@ menuconfig MEDIA_CEC_SUPPORT
> adapter that supports HDMI CEC.
>
> if MEDIA_CEC_SUPPORT
> +source "drivers/media/cec/i2c/Kconfig"
> source "drivers/media/cec/platform/Kconfig"
> source "drivers/media/cec/usb/Kconfig"
> endif
> diff --git a/drivers/media/cec/Makefile b/drivers/media/cec/Makefile
> index 74e80e1b3571..23539339bc81 100644
> --- a/drivers/media/cec/Makefile
> +++ b/drivers/media/cec/Makefile
> @@ -1,2 +1,2 @@
> # SPDX-License-Identifier: GPL-2.0
> -obj-y += core/ platform/ usb/
> +obj-y += core/ i2c/ platform/ usb/
> diff --git a/drivers/media/cec/i2c/Kconfig b/drivers/media/cec/i2c/Kconfig
> new file mode 100644
> index 000000000000..e445ca2110b3
> --- /dev/null
> +++ b/drivers/media/cec/i2c/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# I2C drivers
> +
> +config CEC_CH7322
> + tristate "Chrontel CH7322 CEC controller"
> + select I2C
> + select REGMAP_I2C
> + select CEC_CORE
> + help
> + This is a driver for the Chrontel CH7322 CEC controller. It uses the
> + generic CEC framework interface.
> + CEC bus is present in the HDMI connector and enables communication
> + between compatible devices.
> diff --git a/drivers/media/cec/i2c/Makefile b/drivers/media/cec/i2c/Makefile
> new file mode 100644
> index 000000000000..d7496dfd0fa4
> --- /dev/null
> +++ b/drivers/media/cec/i2c/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for the CEC I2C device drivers.
> +#
> +obj-$(CONFIG_CEC_CH7322) += ch7322.o
> diff --git a/drivers/media/cec/i2c/ch7322.c b/drivers/media/cec/i2c/ch7322.c
> new file mode 100644
> index 000000000000..a9d66ec26440
> --- /dev/null
> +++ b/drivers/media/cec/i2c/ch7322.c
> @@ -0,0 +1,455 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Chrontel CH7322 CEC Controller
> + *
> + * Copyright 2020 Google LLC.
> + */
> +#include <linux/cec.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <media/cec.h>
> +
> +#define CH7322_WRITE 0x00
> +#define CH7322_WRITE_MSENT 0x80
> +#define CH7322_WRITE_BOK 0x40
> +#define CH7322_WRITE_NMASK 0x0F
> +
> +/* Write buffer is 0x01-0x10 */
> +#define CH7322_WRBUF 0x01
> +#define CH7322_WRBUF_LEN 0x10
> +
> +#define CH7322_READ 0x40
> +#define CH7322_READ_NRDT 0x80
> +#define CH7322_READ_MSENT 0x20
> +#define CH7322_READ_NMASK 0x0F
> +
> +/* Read buffer is 0x41-0x50 */
> +#define CH7322_RDBUF 0x41
> +#define CH7322_RDBUF_LEN 0x10
> +
> +#define CH7322_MODE 0x11
> +#define CH7322_MODE_AUTO 0x78
> +#define CH7322_MODE_SW 0xB5
> +
> +#define CH7322_RESET 0x12
> +#define CH7322_RESET_RST 0x00
> +
> +#define CH7322_POWER 0x13
> +
> +#define CH7322_CFG0 0x17
> +#define CH7322_CFG0_EOBEN 0x40
> +#define CH7322_CFG0_PEOB 0x20
> +#define CH7322_CFG0_CLRSPP 0x10
> +#define CH7322_CFG0_FLOW 0x08
> +
> +#define CH7322_CFG1 0x1A
> +#define CH7322_CFG1_STDBYO 0x04
> +#define CH7322_CFG1_HPBP 0x02
> +#define CH7322_CFG1_PIO 0x01
> +
> +#define CH7322_INTCTL 0x1B
> +#define CH7322_INTCTL_INTPB 0x80
> +#define CH7322_INTCTL_STDBY 0x40
> +#define CH7322_INTCTL_HPDFALL 0x20
> +#define CH7322_INTCTL_HPDRISE 0x10
> +#define CH7322_INTCTL_RXMSG 0x08
> +#define CH7322_INTCTL_TXMSG 0x04
> +#define CH7322_INTCTL_NEWPHA 0x02
> +#define CH7322_INTCTL_ERROR 0x01
> +
> +#define CH7322_DVCLKFNH 0x1D
> +#define CH7322_DVCLKFNL 0x1E
> +
> +#define CH7322_CTL 0x31
> +#define CH7322_CTL_FSTDBY 0x80
> +#define CH7322_CTL_PLSEN 0x40
> +#define CH7322_CTL_PLSPB 0x20
> +#define CH7322_CTL_SPADL 0x10
> +#define CH7322_CTL_HINIT 0x08
> +#define CH7322_CTL_WPHYA 0x04
> +#define CH7322_CTL_H1T 0x02
> +#define CH7322_CTL_S1T 0x01
> +
> +#define CH7322_PAWH 0x32
> +#define CH7322_PAWL 0x32
Shouldn't this be 0x33?
Regards,
Hans
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-25 9:16 ` Hans Verkuil
@ 2020-04-25 9:31 ` Hans Verkuil
2020-05-05 23:26 ` Jeff Chase
1 sibling, 0 replies; 11+ messages in thread
From: Hans Verkuil @ 2020-04-25 9:31 UTC (permalink / raw
To: Jeff Chase; +Cc: linux-media, mchehab, robh+dt, devicetree
On 25/04/2020 11:16, Hans Verkuil wrote:
> On 24/04/2020 21:33, Jeff Chase wrote:
>> Hi Hans,
>>
>> Thank you for the quick review.
>>
>>> Is the register documentation available somewhere? I only found the product brief.
>>
>> No, it's not publicly available.
>>
>>> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
>>> or Low Drive conditions? Just checking, not all hardware has support for that.
>>
>> Correct, message transmit completion just has a one-bit status.
>>
>>>> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
>>>> +{
>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>> +
>>>> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
>>>> +
>>>> + return 0;
>>>
>>> This can't be right. I expect that logical addresses are set/cleared here,
>>> because the device needs to know that so that it can ignore messages not
>>> intended for it.
>>
>> As far as I can tell the device doesn't filter based on logical
>> address. I'll have to save
>> the logical address to the driver and filter manually.
>
> That can't be right. If this CEC adapter is assigned logical address 4, and
> it has to Ack any received messages from other CEC devices with destination 4,
> and ignore (i.e. not explicitly Ack) messages with other destinations.
>
> If the CEC adapter wouldn't know what LA to use, then it would have to Ack
> all messages, regardless of the destination, which would make this a complete
> mess.
>
> There must be a register that tells the CEC adapter which logical address(es)
> should be Acked. It's usually a bitmask (one bit for each possible LA) or the
> LA itself is stored.
>
> It might be that you still receive all messages (in which case monitor_all
> is effectively always enabled), but it really needs to be told which LAs should
> be Acked.
Reading through the product brief they mention "Supported Logical Type". It could
be that this device is trying to be smart and that it has to be programmed with
the desired type (which in the linux CEC API is equivalent to CEC_LOG_ADDR_TYPE_*),
and that it claims the logical address based on that.
If so, then that is currently not supported in the CEC framework and some work
would have to be done there.
Regards,
Hans
>
> Regards,
>
> Hans
>
>>
>>>> +}
>>>> +
>>>> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>>> + u32 signal_free_time, struct cec_msg *msg)
>>>> +{
>>>
>>> Does the hardware correctly handle Signal Free Time? If this isn't handled right
>>> then one CEC device can flood the CEC bus, preventing anyone else from using it.
>>>
>>> In some devices it has to be programmed, in others it is hardwired.
>>
>> It must be hardwired -- I don't see a way to program it.
>>
>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>> + int ret;
>>>> +
>>>> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
>>>> + cec_msg_initiator(msg), cec_msg_destination(msg),
>>>> + cec_msg_opcode(msg));
>>>> +
>>>> + mutex_lock(&ch7322->mutex);
>>>> + ret = ch7322_send_message(ch7322, msg);
>>>> + mutex_unlock(&ch7322->mutex);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
>>>> + .adap_enable = ch7322_cec_adap_enable,
>>>> + .adap_log_addr = ch7322_cec_adap_log_addr,
>>>> + .adap_transmit = ch7322_cec_adap_transmit,
>>>
>>> If the HW supports CEC monitoring (aka snooping), then I recommend that
>>> adap_monitor_all_enable is also implemented. It's very useful for debugging
>>> CEC in userspace. Not all HW supports it, though.
>>
>> Okay, I'll add this along with the logical address filtering I mentioned above.
>>
>> Thanks,
>> Jeff
>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-04-25 9:16 ` Hans Verkuil
2020-04-25 9:31 ` Hans Verkuil
@ 2020-05-05 23:26 ` Jeff Chase
2020-05-06 6:42 ` Hans Verkuil
1 sibling, 1 reply; 11+ messages in thread
From: Jeff Chase @ 2020-05-05 23:26 UTC (permalink / raw
To: Hans Verkuil; +Cc: linux-media, mchehab, robh+dt, devicetree
On Sat, Apr 25, 2020 at 5:16 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 24/04/2020 21:33, Jeff Chase wrote:
> > Hi Hans,
> >
> > Thank you for the quick review.
> >
> >> Is the register documentation available somewhere? I only found the product brief.
> >
> > No, it's not publicly available.
> >
> >> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
> >> or Low Drive conditions? Just checking, not all hardware has support for that.
> >
> > Correct, message transmit completion just has a one-bit status.
> >
> >>> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
> >>> +{
> >>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> >>> +
> >>> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
> >>> +
> >>> + return 0;
> >>
> >> This can't be right. I expect that logical addresses are set/cleared here,
> >> because the device needs to know that so that it can ignore messages not
> >> intended for it.
> >
> > As far as I can tell the device doesn't filter based on logical
> > address. I'll have to save
> > the logical address to the driver and filter manually.
>
> That can't be right. If this CEC adapter is assigned logical address 4, and
> it has to Ack any received messages from other CEC devices with destination 4,
> and ignore (i.e. not explicitly Ack) messages with other destinations.
>
> If the CEC adapter wouldn't know what LA to use, then it would have to Ack
> all messages, regardless of the destination, which would make this a complete
> mess.
>
> There must be a register that tells the CEC adapter which logical address(es)
> should be Acked. It's usually a bitmask (one bit for each possible LA) or the
> LA itself is stored.
Sorry, you're right, of course. The register isn't in the
documentation I have but I found it referenced in some sample code. By
default it seems the device automatically stores the logical address
if it recognizes a polling message (with src == dst) that was not
ack'd. The behavior can be configured to allow explicit logical
address assignment instead. I assume that would be preferred?
Thanks,
Jeff
>
> It might be that you still receive all messages (in which case monitor_all
> is effectively always enabled), but it really needs to be told which LAs should
> be Acked.
>
> Regards,
>
> Hans
>
> >
> >>> +}
> >>> +
> >>> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> >>> + u32 signal_free_time, struct cec_msg *msg)
> >>> +{
> >>
> >> Does the hardware correctly handle Signal Free Time? If this isn't handled right
> >> then one CEC device can flood the CEC bus, preventing anyone else from using it.
> >>
> >> In some devices it has to be programmed, in others it is hardwired.
> >
> > It must be hardwired -- I don't see a way to program it.
> >
> >>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
> >>> + int ret;
> >>> +
> >>> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
> >>> + cec_msg_initiator(msg), cec_msg_destination(msg),
> >>> + cec_msg_opcode(msg));
> >>> +
> >>> + mutex_lock(&ch7322->mutex);
> >>> + ret = ch7322_send_message(ch7322, msg);
> >>> + mutex_unlock(&ch7322->mutex);
> >>> +
> >>> + return ret;
> >>> +}
> >>> +
> >>> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
> >>> + .adap_enable = ch7322_cec_adap_enable,
> >>> + .adap_log_addr = ch7322_cec_adap_log_addr,
> >>> + .adap_transmit = ch7322_cec_adap_transmit,
> >>
> >> If the HW supports CEC monitoring (aka snooping), then I recommend that
> >> adap_monitor_all_enable is also implemented. It's very useful for debugging
> >> CEC in userspace. Not all HW supports it, though.
> >
> > Okay, I'll add this along with the logical address filtering I mentioned above.
> >
> > Thanks,
> > Jeff
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver
2020-05-05 23:26 ` Jeff Chase
@ 2020-05-06 6:42 ` Hans Verkuil
0 siblings, 0 replies; 11+ messages in thread
From: Hans Verkuil @ 2020-05-06 6:42 UTC (permalink / raw
To: Jeff Chase; +Cc: linux-media, mchehab, robh+dt, devicetree
On 06/05/2020 01:26, Jeff Chase wrote:
> On Sat, Apr 25, 2020 at 5:16 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 24/04/2020 21:33, Jeff Chase wrote:
>>> Hi Hans,
>>>
>>> Thank you for the quick review.
>>>
>>>> Is the register documentation available somewhere? I only found the product brief.
>>>
>>> No, it's not publicly available.
>>>
>>>> The chip can only detect OK vs NACK? There are no error states for Arbitration Lost
>>>> or Low Drive conditions? Just checking, not all hardware has support for that.
>>>
>>> Correct, message transmit completion just has a one-bit status.
>>>
>>>>> +static int ch7322_cec_adap_log_addr(struct cec_adapter *adap, u8 log_addr)
>>>>> +{
>>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>>> +
>>>>> + dev_dbg(&ch7322->i2c->dev, "cec log addr: %x\n", log_addr);
>>>>> +
>>>>> + return 0;
>>>>
>>>> This can't be right. I expect that logical addresses are set/cleared here,
>>>> because the device needs to know that so that it can ignore messages not
>>>> intended for it.
>>>
>>> As far as I can tell the device doesn't filter based on logical
>>> address. I'll have to save
>>> the logical address to the driver and filter manually.
>>
>> That can't be right. If this CEC adapter is assigned logical address 4, and
>> it has to Ack any received messages from other CEC devices with destination 4,
>> and ignore (i.e. not explicitly Ack) messages with other destinations.
>>
>> If the CEC adapter wouldn't know what LA to use, then it would have to Ack
>> all messages, regardless of the destination, which would make this a complete
>> mess.
>>
>> There must be a register that tells the CEC adapter which logical address(es)
>> should be Acked. It's usually a bitmask (one bit for each possible LA) or the
>> LA itself is stored.
>
> Sorry, you're right, of course. The register isn't in the
> documentation I have but I found it referenced in some sample code. By
> default it seems the device automatically stores the logical address
> if it recognizes a polling message (with src == dst) that was not
> ack'd. The behavior can be configured to allow explicit logical
> address assignment instead. I assume that would be preferred?
Yes, that's preferred (and also the only thing the CEC framework supports).
Very odd that that register is undocumented.
Regards,
Hans
>
> Thanks,
> Jeff
>
>
>>
>> It might be that you still receive all messages (in which case monitor_all
>> is effectively always enabled), but it really needs to be told which LAs should
>> be Acked.
>>
>> Regards,
>>
>> Hans
>>
>>>
>>>>> +}
>>>>> +
>>>>> +static int ch7322_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
>>>>> + u32 signal_free_time, struct cec_msg *msg)
>>>>> +{
>>>>
>>>> Does the hardware correctly handle Signal Free Time? If this isn't handled right
>>>> then one CEC device can flood the CEC bus, preventing anyone else from using it.
>>>>
>>>> In some devices it has to be programmed, in others it is hardwired.
>>>
>>> It must be hardwired -- I don't see a way to program it.
>>>
>>>>> + struct ch7322 *ch7322 = cec_get_drvdata(adap);
>>>>> + int ret;
>>>>> +
>>>>> + dev_dbg(&ch7322->i2c->dev, "cec transmit: %x->%x: %x\n",
>>>>> + cec_msg_initiator(msg), cec_msg_destination(msg),
>>>>> + cec_msg_opcode(msg));
>>>>> +
>>>>> + mutex_lock(&ch7322->mutex);
>>>>> + ret = ch7322_send_message(ch7322, msg);
>>>>> + mutex_unlock(&ch7322->mutex);
>>>>> +
>>>>> + return ret;
>>>>> +}
>>>>> +
>>>>> +static const struct cec_adap_ops ch7322_cec_adap_ops = {
>>>>> + .adap_enable = ch7322_cec_adap_enable,
>>>>> + .adap_log_addr = ch7322_cec_adap_log_addr,
>>>>> + .adap_transmit = ch7322_cec_adap_transmit,
>>>>
>>>> If the HW supports CEC monitoring (aka snooping), then I recommend that
>>>> adap_monitor_all_enable is also implemented. It's very useful for debugging
>>>> CEC in userspace. Not all HW supports it, though.
>>>
>>> Okay, I'll add this along with the logical address filtering I mentioned above.
>>>
>>> Thanks,
>>> Jeff
>>>
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device
2020-04-24 5:38 [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Jeff Chase
2020-04-24 5:38 ` [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver Jeff Chase
@ 2020-05-11 21:20 ` Rob Herring
2020-05-12 23:23 ` Jeff Chase
1 sibling, 1 reply; 11+ messages in thread
From: Rob Herring @ 2020-05-11 21:20 UTC (permalink / raw
To: Jeff Chase; +Cc: linux-media, mchehab, hverkuil-cisco, devicetree
On Fri, Apr 24, 2020 at 01:38:18AM -0400, Jeff Chase wrote:
> The ch7322 is a Chrontel CEC controller.
>
> Signed-off-by: Jeff Chase <jnchase@google.com>
> ---
> Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 2 files changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 4165352a590a..ec2ddc6cdf9a 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -48,6 +48,8 @@ properties:
> - capella,cm32181
> # CM3232: Ambient Light Sensor
> - capella,cm3232
> + # CH7322: HDMI-CEC Controller
> + - chrontel,ch7322
I don't think this qualifies as a trivial device. It has HPDI, OE and
reset signals all likely hooked up to GPIOs. You might not have those
hooked up for s/w control, but someone will.
And I'd assume if you had multiple instances, they will need to be
associated with each connector.
Rob
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device
2020-05-11 21:20 ` [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Rob Herring
@ 2020-05-12 23:23 ` Jeff Chase
0 siblings, 0 replies; 11+ messages in thread
From: Jeff Chase @ 2020-05-12 23:23 UTC (permalink / raw
To: Rob Herring; +Cc: linux-media, mchehab, Hans Verkuil, devicetree
On Mon, May 11, 2020 at 5:20 PM Rob Herring <robh@kernel.org> wrote:
>
> On Fri, Apr 24, 2020 at 01:38:18AM -0400, Jeff Chase wrote:
> > The ch7322 is a Chrontel CEC controller.
> >
> > Signed-off-by: Jeff Chase <jnchase@google.com>
> > ---
> > Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
> > Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> > 2 files changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> > index 4165352a590a..ec2ddc6cdf9a 100644
> > --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> > +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> > @@ -48,6 +48,8 @@ properties:
> > - capella,cm32181
> > # CM3232: Ambient Light Sensor
> > - capella,cm3232
> > + # CH7322: HDMI-CEC Controller
> > + - chrontel,ch7322
>
> I don't think this qualifies as a trivial device. It has HPDI, OE and
> reset signals all likely hooked up to GPIOs. You might not have those
> hooked up for s/w control, but someone will.
>
> And I'd assume if you had multiple instances, they will need to be
> associated with each connector.
>
> Rob
Thank you for looking at this. Taking a step back for a moment, I am
developing this driver for an x86/ACPI platform on which I also
control the firmware. Is there a preference between using an ACPI ID
and a devicetree compatible id? I am trying to find out if the vendor
already has an ACPI ID for this device. If I find and use that should
I still add a compatible id anyways?
Jeff
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-05-12 23:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-24 5:38 [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Jeff Chase
2020-04-24 5:38 ` [PATCH 2/2] media: cec: i2c: ch7322: Add ch7322 CEC controller driver Jeff Chase
2020-04-24 12:24 ` Hans Verkuil
2020-04-24 19:33 ` Jeff Chase
2020-04-25 9:16 ` Hans Verkuil
2020-04-25 9:31 ` Hans Verkuil
2020-05-05 23:26 ` Jeff Chase
2020-05-06 6:42 ` Hans Verkuil
2020-04-25 9:17 ` Hans Verkuil
2020-05-11 21:20 ` [PATCH 1/2] dt-bindings: Add ch7322 as a trivial device Rob Herring
2020-05-12 23:23 ` Jeff Chase
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.