[PATCH v2 5/7] drm/i2c: tda9950: add CEC driver

Hans Verkuil hverkuil at xs4all.nl
Wed Dec 6 14:11:43 UTC 2017


Hi Russell,

Some small comments below:

On 12/06/17 13:35, Russell King wrote:
> Add a CEC driver for the TDA9950, which is a stand-alone I2C CEC device,
> but is also integrated into HDMI transceivers such as the TDA9989 and
> TDA19989.
> 
> The TDA9950 contains a command processor which handles retransmissions
> and the low level bus protocol.  The driver just has to read and write
> the messages, and handle error conditions.
> 
> Signed-off-by: Russell King <rmk+kernel at armlinux.org.uk>
> ---
>  drivers/gpu/drm/i2c/Kconfig           |   5 +
>  drivers/gpu/drm/i2c/Makefile          |   1 +
>  drivers/gpu/drm/i2c/tda9950.c         | 507 ++++++++++++++++++++++++++++++++++
>  include/linux/platform_data/tda9950.h |  16 ++
>  4 files changed, 529 insertions(+)
>  create mode 100644 drivers/gpu/drm/i2c/tda9950.c
>  create mode 100644 include/linux/platform_data/tda9950.h
> 
> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> index a6c92beb410a..3a232f5ff0a1 100644
> --- a/drivers/gpu/drm/i2c/Kconfig
> +++ b/drivers/gpu/drm/i2c/Kconfig
> @@ -26,4 +26,9 @@ config DRM_I2C_NXP_TDA998X
>  	help
>  	  Support for NXP Semiconductors TDA998X HDMI encoders.
>  
> +config DRM_I2C_NXP_TDA9950
> +	tristate "NXP Semiconductors TDA9950/TDA998X HDMI CEC"
> +	select CEC_NOTIFIER
> +	select CEC_CORE
> +
>  endmenu
> diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile
> index b20100c18ffb..a962f6f08568 100644
> --- a/drivers/gpu/drm/i2c/Makefile
> +++ b/drivers/gpu/drm/i2c/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o
>  
>  tda998x-y := tda998x_drv.o
>  obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o
> +obj-$(CONFIG_DRM_I2C_NXP_TDA9950) += tda9950.o
> diff --git a/drivers/gpu/drm/i2c/tda9950.c b/drivers/gpu/drm/i2c/tda9950.c
> new file mode 100644
> index 000000000000..6f7a37ddda05
> --- /dev/null
> +++ b/drivers/gpu/drm/i2c/tda9950.c
> @@ -0,0 +1,507 @@
> +/*
> + *  TDA9950 Consumer Electronics Control driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * The NXP TDA9950 implements the HDMI Consumer Electronics Control
> + * interface.  The host interface is similar to a mailbox: the data
> + * registers starting at REG_CDR0 are written to send a command to the
> + * internal CPU, and replies are read from these registers.
> + *
> + * As the data registers represent a mailbox, they must be accessed
> + * as a single I2C transaction.  See the TDA9950 data sheet for details.
> + */
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/platform_data/tda9950.h>
> +#include <linux/slab.h>
> +#include <drm/drm_edid.h>
> +#include <media/cec.h>
> +#include <media/cec-notifier.h>
> +
> +enum {
> +	REG_CSR = 0x00,
> +	CSR_BUSY = BIT(7),
> +	CSR_INT  = BIT(6),
> +	CSR_ERR  = BIT(5),
> +
> +	REG_CER = 0x01,
> +
> +	REG_CVR = 0x02,
> +
> +	REG_CCR = 0x03,
> +	CCR_RESET = BIT(7),
> +	CCR_ON    = BIT(6),
> +
> +	REG_ACKH = 0x04,
> +	REG_ACKL = 0x05,
> +
> +	REG_CCONR = 0x06,
> +	CCONR_ENABLE_ERROR = BIT(4),
> +	CCONR_RETRY_MASK = 7,
> +
> +	REG_CDR0 = 0x07,
> +
> +	CDR1_REQ = 0x00,
> +	CDR1_CNF = 0x01,
> +	CDR1_IND = 0x81,
> +	CDR1_ERR = 0x82,
> +	CDR1_IER = 0x83,
> +
> +	CDR2_CNF_SUCCESS    = 0x00,
> +	CDR2_CNF_OFF_STATE  = 0x80,
> +	CDR2_CNF_BAD_REQ    = 0x81,
> +	CDR2_CNF_CEC_ACCESS = 0x82,
> +	CDR2_CNF_ARB_ERROR  = 0x83,
> +	CDR2_CNF_BAD_TIMING = 0x84,
> +	CDR2_CNF_NACK_ADDR  = 0x85,
> +	CDR2_CNF_NACK_DATA  = 0x86,
> +};
> +
> +struct tda9950_priv {
> +	struct i2c_client *client;
> +	struct device *hdmi;
> +	struct cec_adapter *adap;
> +	struct tda9950_glue *glue;
> +	u16 addresses;
> +	struct cec_msg rx_msg;
> +	struct cec_notifier *notify;
> +	bool open;
> +};
> +
> +static int tda9950_write_range(struct i2c_client *client, u8 addr, u8 *p, int cnt)
> +{
> +	struct i2c_msg msg;
> +	u8 buf[cnt + 1];
> +	int ret;
> +
> +	buf[0] = addr;
> +	memcpy(buf + 1, p, cnt);
> +
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = cnt + 1;
> +	msg.buf = buf;
> +
> +	dev_dbg(&client->dev, "wr 0x%02x: %*ph\n", addr, cnt, p);
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret, addr);
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static void tda9950_write(struct i2c_client *client, u8 addr, u8 val)
> +{
> +	tda9950_write_range(client, addr, &val, 1);
> +}
> +
> +static int tda9950_read_range(struct i2c_client *client, u8 addr, u8 *p, int cnt)
> +{
> +	struct i2c_msg msg[2];
> +	int ret;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].flags = 0;
> +	msg[0].len = 1;
> +	msg[0].buf = &addr;
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].len = cnt;
> +	msg[1].buf = p;
> +
> +	ret = i2c_transfer(client->adapter, msg, 2);
> +	if (ret < 0)
> +		dev_err(&client->dev, "Error %d reading from cec:0x%x\n", ret, addr);
> +
> +	dev_dbg(&client->dev, "rd 0x%02x: %*ph\n", addr, cnt, p);
> +
> +	return ret;
> +}
> +
> +static u8 tda9950_read(struct i2c_client *client, u8 addr)
> +{
> +	int ret;
> +	u8 val;
> +
> +	ret = tda9950_read_range(client, addr, &val, 1);
> +	if (ret < 0)
> +		val = 0;
> +
> +	return val;
> +}
> +
> +static irqreturn_t tda9950_irq(int irq, void *data)
> +{
> +	struct tda9950_priv *priv = data;
> +	unsigned int tx_status;
> +	u8 csr, cconr, buf[19];
> +	u8 arb_lost_cnt, nack_cnt, err_cnt;
> +
> +	if (!priv->open)
> +		return IRQ_NONE;
> +
> +	csr = tda9950_read(priv->client, REG_CSR);
> +	if (!(csr & CSR_INT))
> +		return IRQ_NONE;
> +
> +	cconr = tda9950_read(priv->client, REG_CCONR) & CCONR_RETRY_MASK;
> +
> +	tda9950_read_range(priv->client, REG_CDR0, buf, sizeof(buf));
> +
> +	/*
> +	 * This should never happen: the data sheet says that there will
> +	 * always be a valid message if the interrupt line is asserted.
> +	 */
> +	if (buf[0] == 0) {

Checking for <= 2 is safer. See also my comment below.

> +		dev_warn(&priv->client->dev, "interrupt pending, but no message?\n");
> +		return IRQ_NONE;
> +	}
> +
> +	switch (buf[1]) {
> +	case CDR1_CNF: /* transmit result */
> +		arb_lost_cnt = nack_cnt = err_cnt = 0;
> +		switch (buf[2]) {
> +		case CDR2_CNF_SUCCESS:
> +			tx_status = CEC_TX_STATUS_OK;
> +			break;
> +
> +		case CDR2_CNF_ARB_ERROR:
> +			tx_status = CEC_TX_STATUS_ARB_LOST;
> +			arb_lost_cnt = cconr;
> +			break;
> +
> +		case CDR2_CNF_NACK_ADDR:
> +			tx_status = CEC_TX_STATUS_NACK;
> +			nack_cnt = cconr;
> +			break;
> +
> +		default: /* some other error, refer to TDA9950 docs */
> +			dev_err(&priv->client->dev, "CNF reply error 0x%02x\n",
> +				buf[2]);
> +			tx_status = CEC_TX_STATUS_ERROR;
> +			err_cnt = cconr;
> +			break;
> +		}
> +		/* TDA9950 executes all retries for us */
> +		tx_status |= CEC_TX_STATUS_MAX_RETRIES;
> +		cec_transmit_done(priv->adap, tx_status, arb_lost_cnt,
> +				  nack_cnt, 0, err_cnt);
> +		break;
> +
> +	case CDR1_IND:
> +		priv->rx_msg.len = buf[0] - 2;

Does the datasheet give any guarantees that buf[0] is always between 3 and 18?

Note: it is possible for devices to send more than 16 bytes in a CEC message.
Not allowed, mind you, but it can be done and I suspect some do in certain
situations. So if the hardware just keeps counting you can get a rx_msg.len > 16
and then the memcpy below would overwrite memory. You want to clamp the length
to max 16.

> +		memcpy(priv->rx_msg.msg, buf + 2, priv->rx_msg.len);
> +		cec_received_msg(priv->adap, &priv->rx_msg);
> +		break;
> +
> +	default: /* unknown */
> +		dev_err(&priv->client->dev, "unknown service id 0x%02x\n",
> +			buf[1]);
> +		break;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int tda9950_cec_transmit(struct cec_adapter *adap, u8 attempts,
> +				u32 signal_free_time, struct cec_msg *msg)
> +{
> +	struct tda9950_priv *priv = adap->priv;
> +	u8 buf[16 + 2];

Use CEC_MAX_MSG_SIZE instead of 16.

> +
> +	buf[0] = 2 + msg->len;
> +	buf[1] = CDR1_REQ;
> +	memcpy(buf + 2, msg->msg, msg->len);
> +
> +	if (attempts > 5)
> +		attempts = 5;
> +
> +	tda9950_write(priv->client, REG_CCONR, attempts);
> +
> +	return tda9950_write_range(priv->client, REG_CDR0, buf, 2 + msg->len);
> +}
> +
> +static int tda9950_cec_adap_log_addr(struct cec_adapter *adap, u8 addr)
> +{
> +	struct tda9950_priv *priv = adap->priv;
> +	u16 addresses;
> +	u8 buf[2];
> +
> +	if (addr == CEC_LOG_ADDR_INVALID)
> +		addresses = priv->addresses = 0;
> +	else
> +		addresses = priv->addresses |= BIT(addr);
> +
> +	/* TDA9950 doesn't want address 15 set */
> +	addresses &= 0x7fff;
> +	buf[0] = addresses >> 8;
> +	buf[1] = addresses;
> +
> +	return tda9950_write_range(priv->client, REG_ACKH, buf, 2);
> +}
> +
> +/*
> + * When operating as part of the TDA998x, we need additional handling
> + * to initialise and shut down the TDA9950 part of the device.  These
> + * two hooks are provided to allow the TDA998x code to perform those
> + * activities.
> + */
> +static int tda9950_glue_open(struct tda9950_priv *priv)
> +{
> +	int ret = 0;
> +
> +	if (priv->glue && priv->glue->open)
> +		ret = priv->glue->open(priv->glue->data);
> +
> +	priv->open = true;
> +
> +	return ret;
> +}
> +
> +static void tda9950_glue_release(struct tda9950_priv *priv)
> +{
> +	priv->open = false;
> +
> +	if (priv->glue && priv->glue->release)
> +		priv->glue->release(priv->glue->data);
> +}
> +
> +static int tda9950_open(struct tda9950_priv *priv)
> +{
> +	struct i2c_client *client = priv->client;
> +	int ret;
> +
> +	ret = tda9950_glue_open(priv);
> +	if (ret)
> +		return ret;
> +
> +	/* Reset the TDA9950, and wait 250ms for it to recover */
> +	tda9950_write(client, REG_CCR, CCR_RESET);
> +	msleep(250);
> +
> +	tda9950_cec_adap_log_addr(priv->adap, CEC_LOG_ADDR_INVALID);
> +
> +	/* Start the command processor */
> +	tda9950_write(client, REG_CCR, CCR_ON);
> +
> +	return 0;
> +}
> +
> +static void tda9950_release(struct tda9950_priv *priv)
> +{
> +	struct i2c_client *client = priv->client;
> +	int timeout = 50;
> +	u8 csr;
> +
> +	/* Stop the command processor */
> +	tda9950_write(client, REG_CCR, 0);
> +
> +	/* Wait up to .5s for it to signal non-busy */
> +	do {
> +		csr = tda9950_read(client, REG_CSR);
> +		if (!(csr & CSR_BUSY) || --timeout)
> +			break;
> +		msleep(10);
> +	} while (1);
> +
> +	/* Warn the user that their IRQ may die if it's shared. */
> +	if (csr & CSR_BUSY)
> +		dev_warn(&client->dev, "command processor failed to stop, irq%d may die (csr=0x%02x)\n",
> +			 client->irq, csr);
> +
> +	tda9950_glue_release(priv);
> +}
> +
> +static int tda9950_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> +	struct tda9950_priv *priv = adap->priv;
> +
> +	if (!enable) {
> +		tda9950_release(priv);
> +		return 0;
> +	} else {
> +		return tda9950_open(priv);
> +	}
> +}
> +
> +static const struct cec_adap_ops tda9950_cec_ops = {
> +	.adap_enable = tda9950_cec_adap_enable,
> +	.adap_log_addr = tda9950_cec_adap_log_addr,
> +	.adap_transmit = tda9950_cec_transmit,
> +};
> +
> +/*
> + * When operating as part of the TDA998x, we need to claim additional
> + * resources.  These two hooks permit the management of those resources.
> + */
> +static void tda9950_devm_glue_exit(void *data)
> +{
> +	struct tda9950_glue *glue = data;
> +
> +	if (glue && glue->exit)
> +		glue->exit(glue->data);
> +}
> +
> +static int tda9950_devm_glue_init(struct device *dev, struct tda9950_glue *glue)
> +{
> +	int ret;
> +
> +	if (glue && glue->init) {
> +		ret = glue->init(glue->data);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = devm_add_action(dev, tda9950_devm_glue_exit, glue);
> +	if (ret)
> +		tda9950_devm_glue_exit(glue);
> +
> +	return ret;
> +}
> +
> +static void tda9950_cec_del(void *data)
> +{
> +	struct tda9950_priv *priv = data;
> +
> +	cec_delete_adapter(priv->adap);
> +}
> +
> +static int tda9950_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct tda9950_glue *glue = client->dev.platform_data;
> +	struct device *dev = &client->dev;
> +	struct tda9950_priv *priv;
> +	unsigned long irqflags;
> +	int ret;
> +	u8 cvr;
> +
> +	/*
> +	 * We must have I2C functionality: our multi-byte accesses
> +	 * must be performed as a single contiguous transaction.
> +	 */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		dev_err(&client->dev,
> +			"adapter does not support I2C functionality\n");
> +		return -ENXIO;
> +	}
> +
> +	/* We must have an interrupt to be functional. */
> +	if (client->irq <= 0) {
> +		dev_err(&client->dev, "driver requires an interrupt\n");
> +		return -ENXIO;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->client = client;
> +	priv->glue = glue;
> +
> +	i2c_set_clientdata(client, priv);
> +
> +	/*
> +	 * If we're part of a TDA998x, we want the class devices to be
> +	 * associated with the HDMI Tx so we have a tight relationship
> +	 * between the HDMI interface and the CEC interface.
> +	 */
> +	priv->hdmi = dev;
> +	if (glue && glue->parent)
> +		priv->hdmi = glue->parent;
> +
> +	priv->adap = cec_allocate_adapter(&tda9950_cec_ops, priv, "tda9950",
> +					  CEC_CAP_LOG_ADDRS |
> +					  CEC_CAP_TRANSMIT | CEC_CAP_RC,

Use CEC_CAP_DEFAULTS instead of these three explicit capabilities.

> +					  CEC_MAX_LOG_ADDRS);
> +	if (IS_ERR(priv->adap))
> +		return PTR_ERR(priv->adap);
> +
> +	ret = devm_add_action(dev, tda9950_cec_del, priv);
> +	if (ret) {
> +		cec_delete_adapter(priv->adap);
> +		return ret;
> +	}
> +
> +	ret = tda9950_devm_glue_init(dev, glue);
> +	if (ret)
> +		return ret;
> +
> +	ret = tda9950_glue_open(priv);
> +	if (ret)
> +		return ret;
> +
> +	cvr = tda9950_read(client, REG_CVR);
> +
> +	dev_info(&client->dev,
> +		 "TDA9950 CEC interface, hardware version %u.%u\n",
> +		 cvr >> 4, cvr & 15);
> +
> +	tda9950_glue_release(priv);
> +
> +	irqflags = IRQF_TRIGGER_FALLING;
> +	if (glue)
> +		irqflags = glue->irq_flags;
> +
> +	ret = devm_request_threaded_irq(dev, client->irq, NULL, tda9950_irq,
> +					irqflags | IRQF_SHARED | IRQF_ONESHOT,
> +					dev_name(&client->dev), priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	priv->notify = cec_notifier_get(priv->hdmi);
> +	if (!priv->notify)
> +		return -ENOMEM;
> +
> +	ret = cec_register_adapter(priv->adap, priv->hdmi);
> +	if (ret < 0) {
> +		cec_notifier_put(priv->notify);
> +		return ret;
> +	}
> +
> +	/*
> +	 * CEC documentation says we must not call cec_delete_adapter
> +	 * after a successful call to cec_register_adapter().
> +	 */
> +	devm_remove_action(dev, tda9950_cec_del, priv);
> +
> +	cec_register_cec_notifier(priv->adap, priv->notify);
> +
> +	return 0;
> +}
> +
> +static int tda9950_remove(struct i2c_client *client)
> +{
> +	struct tda9950_priv *priv = i2c_get_clientdata(client);
> +
> +	cec_unregister_adapter(priv->adap);
> +	cec_notifier_put(priv->notify);
> +
> +	return 0;
> +}
> +
> +static struct i2c_device_id tda9950_ids[] = {
> +	{ "tda9950", 0 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, tda9950_ids);
> +
> +static struct i2c_driver tda9950_driver = {
> +	.probe = tda9950_probe,
> +	.remove = tda9950_remove,
> +	.driver = {
> +		.name = "tda9950",
> +	},
> +	.id_table = tda9950_ids,
> +};
> +
> +module_i2c_driver(tda9950_driver);
> +
> +MODULE_AUTHOR("Russell King <rmk+kernel at armlinux.org.uk>");
> +MODULE_DESCRIPTION("TDA9950/TDA998x Consumer Electronics Control Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/platform_data/tda9950.h b/include/linux/platform_data/tda9950.h
> new file mode 100644
> index 000000000000..c65efd461102
> --- /dev/null
> +++ b/include/linux/platform_data/tda9950.h
> @@ -0,0 +1,16 @@
> +#ifndef LINUX_PLATFORM_DATA_TDA9950_H
> +#define LINUX_PLATFORM_DATA_TDA9950_H
> +
> +struct device;
> +
> +struct tda9950_glue {
> +	struct device *parent;
> +	unsigned long irq_flags;
> +	void *data;
> +	int (*init)(void *);
> +	void (*exit)(void *);
> +	int (*open)(void *);
> +	void (*release)(void *);
> +};
> +
> +#endif
> 

As mentioned earlier I will see if I can get it up and running on my
BeagleBone Black next week.

Regards,

	Hans


More information about the dri-devel mailing list