Re: [PATCH 2/4] drm/i2c: nxp-tda998x (v3)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/29/2013 06:23 PM, Rob Clark wrote:
Driver for the NXP TDA998X i2c hdmi encoder slave.

Rob,

good to see a driver for TDA998x comming! I'd love to test
it on CuBox (mach-dove) but there is no gpu driver I can hook up,
yet. Anyway, I will make some comments how I think the driver
should be improved.

diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 1611836..4d341db 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -19,4 +19,10 @@ config DRM_I2C_SIL164
  	  when used in pairs) TMDS transmitters, used in some nVidia
  	  video cards.

+config DRM_I2C_NXP_TDA998X
+	tristate "NXP Semiconductors TDA998X HDMI encoder"
+	default m if DRM_TILCDC
+	help
+	  Support for NXP Semiconductors TDA998X HDMI encoders.

Maybe nit-picking but later down you actually support TDA9989,
TDA19988, and TDA19989. The latter ones don't fit in TDA998x,
so at least the Kconfig entry should reflect TDA9989/TDA1998x.

[...]
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
new file mode 100644
index 0000000..e68b58a
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -0,0 +1,906 @@
+/*
+ * Copyright (C) 2012 Texas Instruments
+ * Author: Rob Clark<robdclark@xxxxxxxxx>
+ *
+
[...]
+
+/* The TDA9988 series of devices use a paged register scheme.. to simplify
+ * things we encode the page # in upper bits of the register #.  To read/
+ * write a given register, we need to make sure CURPAGE register is set
+ * appropriately.  Which implies reads/writes are not atomic.  Fun!
+ */

Please have a look at regmap-i2c, it also supports paged i2c registers
and will save you _a lot_ of the i2c handling.

[...]
+
+/* Device versions: */
+#define TDA9989N2                 0x0101
+#define TDA19989                  0x0201
+#define TDA19989N2                0x0202
+#define TDA19988                  0x0301

Maybe split this into device_version/revision? What does N2 stand for
or is this the name NXP uses for that device?

> [...]
+static void
+cec_write(struct drm_encoder *encoder, uint16_t addr, uint8_t val)
+{
+	struct i2c_client *client = to_tda998x_priv(encoder)->cec;
+	uint8_t buf[] = {addr, val};
+	int ret;
+
+	ret = i2c_master_send(client, buf, ARRAY_SIZE(buf));
+	if (ret<  0)
+		dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret, addr);
+}

Has there been any decision on how to split/integrate cec from drm?
Or is there display stuff located in cec i2c slave (I see HPD in
..._detect below)?

[...]
+static void
+tda998x_encoder_save(struct drm_encoder *encoder)
+{
+	DBG("");
+}
+
+static void
+tda998x_encoder_restore(struct drm_encoder *encoder)
+{
+	DBG("");
+}

Hmmm, these DBG("") shouldn't be in upstream kernel code?

+static bool
+tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
+			  const struct drm_display_mode *mode,
+			  struct drm_display_mode *adjusted_mode)
+{
+	return true;
+}
+
+static int
+tda998x_encoder_mode_valid(struct drm_encoder *encoder,
+			  struct drm_display_mode *mode)
+{
+	return MODE_OK;
+}

At least a note would be helpful to see what callbacks are
not yet done. I guess there will be some kind of mode check
someday?

[...]
+static enum drm_connector_status
+tda998x_encoder_detect(struct drm_encoder *encoder,
+		      struct drm_connector *connector)
+{
+	uint8_t val = cec_read(encoder, REG_CEC_RXSHPDLEV);
+	return (val&  CEC_RXSHPDLEV_HPD) ? connector_status_connected :
+			connector_status_disconnected;
+}

This is where cec slave gets called from hdmi i2c driver. Any chance
there is HPD status in hdmi registers, too?

+/* I2C driver functions */
+
+static int
+tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	return 0;
+}
+
+static int
+tda998x_remove(struct i2c_client *client)
+{
+	return 0;
+}

Hmm, empty _probe and _remove? Maybe these should get some code
from _init below?

+static int
+tda998x_encoder_init(struct i2c_client *client,
+		    struct drm_device *dev,
+		    struct drm_encoder_slave *encoder_slave)
+{
+	struct drm_encoder *encoder =&encoder_slave->base;
+	struct tda998x_priv *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->current_page = 0;
+	priv->cec = i2c_new_dummy(client->adapter, 0x34);
+	priv->dpms = DRM_MODE_DPMS_OFF;
+
+	encoder_slave->slave_priv = priv;
+	encoder_slave->slave_funcs =&tda998x_encoder_funcs;
+
+	/* wake up the device: */
+	cec_write(encoder, REG_CEC_ENAMODS,
+			CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
+
+	tda998x_reset(encoder);
+
+	/* read version: */
+	priv->rev = reg_read(encoder, REG_VERSION_LSB) |
+			reg_read(encoder, REG_VERSION_MSB)<<  8;
+
+	/* mask off feature bits: */
+	priv->rev&= ~0x30; /* not-hdcp and not-scalar bit */

If revision register contains features, why not save them for later
driver improvements?

+	switch (priv->rev) {
+	case TDA9989N2:  dev_info(dev->dev, "found TDA9989 n2");  break;
+	case TDA19989:   dev_info(dev->dev, "found TDA19989");    break;
+	case TDA19989N2: dev_info(dev->dev, "found TDA19989 n2"); break;
+	case TDA19988:   dev_info(dev->dev, "found TDA19988");    break;
+	default:
+		DBG("found unsupported device: %04x", priv->rev);
+		goto fail;
+	}

I think printing revision is sufficient, no user will care about the
actual device or revision.

+	/* after reset, enable DDC: */
+	reg_write(encoder, REG_DDC_DISABLE, 0x00);
+
+	/* set clock on DDC channel: */
+	reg_write(encoder, REG_TX3, 39);

This should be kept disabled as long as there is no monitor attached
(HPD!)

+	/* if necessary, disable multi-master: */
+	if (priv->rev == TDA19989)
+		reg_set(encoder, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
+
+	cec_write(encoder, REG_CEC_FRO_IM_CLK_CTRL,
+			CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
+
+	return 0;
+
+fail:
+	/* if encoder_init fails, the encoder slave is never registered,
+	 * so cleanup here:
+	 */
+	if (priv->cec)
+		i2c_unregister_device(priv->cec);
+	kfree(priv);
+	encoder_slave->slave_priv = NULL;
+	encoder_slave->slave_funcs = NULL;
+	return -ENXIO;
+}
+
+static struct i2c_device_id tda998x_ids[] = {
+	{ "tda998x", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tda998x_ids);

Shouldn't the above carry the hdmi core i2c address at least?

+static struct drm_i2c_encoder_driver tda998x_driver = {
+	.i2c_driver = {
+		.probe = tda998x_probe,
+		.remove = tda998x_remove,
+		.driver = {
+			.name = "tda998x",
+		},
+		.id_table = tda998x_ids,
+	},
+	.encoder_init = tda998x_encoder_init,
+};
+
+/* Module initialization */
+
+static int __init
+tda998x_init(void)
+{
+	DBG("");
+	return drm_i2c_encoder_register(THIS_MODULE,&tda998x_driver);
+}
+
+static void __exit
+tda998x_exit(void)
+{
+	DBG("");
+	drm_i2c_encoder_unregister(&tda998x_driver);
+}
+
+MODULE_AUTHOR("Rob Clark<robdclark@xxxxxxxxx");
+MODULE_DESCRIPTION("NXP Semiconductors TDA998X HDMI Encoder");
+MODULE_LICENSE("GPL");

Maybe stick to one version of "TDA998X" or "TDA998x" ?

Regards,
  Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux