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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
