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