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 Clarkrobdclark@gmail.com
[...]
+/* 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@gmail.com"); +MODULE_DESCRIPTION("NXP Semiconductors TDA998X HDMI Encoder"); +MODULE_LICENSE("GPL");
Maybe stick to one version of "TDA998X" or "TDA998x" ?
Regards, Sebastian