This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write - code cleanup and fix some small errors - use global constants - don't read write-only registers - add DT support - use IRQ for connection status and EDID read
- v5 - remarks from Russell King - free the IRQ on encoder destroy (patch 14) - fix bad comment (patch 23) - removal of 'set the video mode from the adjusted value' which was breaking tilcdc (Darren Etheridge) - use the IRQ trigger type from platform/DT (patch 14)
- v4 - remarks from Russell King (thank you, Russell) - more comments about the patches - remove 'fix a NULL pointer dereference' - get the IRQ number from the i2c client - remove 'get a better status of the connection' - split the previous 'fix the ENABLE_SPACE register' - add asounddef.h variables in 'change the frequence in the audio channel' - add 'always enable EDID read IRQ' - remove audio changes which will be in a next patch set about information exchanges with the audio subsystem - v3 - remarks from Russell King - more comments about the patches - change variable name instead of copy (patch 7) - add 'fix bad value in the AIF' (patch 8) - add 'fix a NULL pointer dereference' (patch 13) - add 'add DT documentation' (patch 16) - remove 'use the tda998x video format when cea mode' - remove 'change the video quantization - remove 'fix the value of the TBG_CNTRL_1 register' - remove 'tda998x: move the TBG_CNTRL_0 register setting' - change fields of the register AIP_CLKSEL (patch 19) - remove 'adjust the audio CTS to the mode clock' - don't put a comment between field definition (patch 23) - some more remarks may be found in the patches 10, 14, 15 - removal of the tda codec interface which will be the subject of an other patch series - v2 - decompose patches with different topics - fix some bad i2c register values - add audio codec interface
Jean-Francois Moine (23): drm/i2c: tda998x: simplify the i2c read/write functions drm/i2c: tda998x: check more I/O errors drm/i2c: tda998x: code cleanup drm/i2c: tda998x: change probe message origin drm/i2c: tda998x: don't freeze the system at audio startup time drm/i2c: tda998x: force the page register at startup time drm/i2c: tda998x: fix bad value in the AIF drm/i2c: tda998x: use HDMI constants drm/i2c: tda998x: don't read write-only registers drm/i2c: tda998x: free the CEC device on encoder_destroy drm/i2c: tda998x: check the CEC device creation drm/i2c: tda998x: add DT support drm/i2c: tda998x: always enable EDID read IRQ drm/i2c: tda998x: use irq for connection status and EDID read drm/i2c: tda998x: add DT documentation drm/i2c: tda998x: fix the ENABLE_SPACE register drm/i2c: tda998x: set the PLL division factor in range 0..3 drm/i2c: tda998x: make the audio code more readable drm/i2c: tda998x: remove the unused variable ca_i2s drm/i2c: tda998x: add the active aspect in HDMI AVI frame drm/i2c: tda998x: change the frequence in the audio channel drm/i2c: tda998x: code optimization drm/i2c: tda998x: adjust the audio clock divider for S/PDIF
.../devicetree/bindings/drm/i2c/tda998x.txt | 27 + drivers/gpu/drm/i2c/tda998x_drv.c | 609 +++++++++++++-------- 2 files changed, 416 insertions(+), 220 deletions(-) create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
The predivider division factor of the register PLL_SERIAL_2 is in the range 0..3, the value 0 being used for a division by 1.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 36e95b2..873e1e9 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -214,7 +214,7 @@ struct tda998x_priv { # define PLL_SERIAL_1_SRL_IZ(x) (((x) & 3) << 1) # define PLL_SERIAL_1_SRL_MAN_IZ (1 << 6) #define REG_PLL_SERIAL_2 REG(0x02, 0x01) /* read/write */ -# define PLL_SERIAL_2_SRL_NOSC(x) (((x) & 3) << 0) +# define PLL_SERIAL_2_SRL_NOSC(x) ((x) << 0) # define PLL_SERIAL_2_SRL_PR(x) (((x) & 0xf) << 4) #define REG_PLL_SERIAL_3 REG(0x02, 0x02) /* read/write */ # define PLL_SERIAL_3_SRL_CCIR (1 << 0) @@ -886,6 +886,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, }
div = 148500 / mode->clock; + if (div != 0) { + div--; + if (div > 3) + div = 3; + }
/* mute the audio FIFO: */ reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
On Sat, 2014-01-25 at 18:14 +0100, Jean-Francois Moine wrote:
The predivider division factor of the register PLL_SERIAL_2 is in the range 0..3, the value 0 being used for a division by 1.
trivia:
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
[]
@@ -886,6 +886,11 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, }
div = 148500 / mode->clock;
- if (div != 0) {
div--;if (div > 3)div = 3;- }
perhaps clamp(div, 1, 4) div--;
According to some tests on the Cubox (Marvell Armada 510 + TDA19988), the S/PDIF input asks for a greater audio clock divider.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 4f1a047..2f97290 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -680,10 +680,14 @@ tda998x_configure_audio(struct tda998x_priv *priv, * There is no detailed info in the datasheet, so we just * assume 100MHz requires larger divider. */ + adiv = AUDIO_DIV_SERCLK_8; if (mode->clock > 100000) - adiv = AUDIO_DIV_SERCLK_16; - else - adiv = AUDIO_DIV_SERCLK_8; + adiv++; /* AUDIO_DIV_SERCLK_16 */ + + /* S/PDIF asks for a larger divider */ + if (p->audio_format == AFMT_SPDIF) + adiv++; /* AUDIO_DIV_SERCLK_16 or _32 */ + reg_write(priv, REG_AUDIO_DIV, adiv);
/*
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- .../devicetree/bindings/drm/i2c/tda998x.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt new file mode 100644 index 0000000..d7df01c --- /dev/null +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -0,0 +1,27 @@ +Device-Tree bindings for the NXP TDA998x HDMI transmitter + +Required properties; + - compatible: must be "nxp,tda998x" + +Optional properties: + - interrupts: interrupt number and trigger type + default: polling + + - pinctrl-0: pin control group to be used for + screen plug/unplug interrupt. + + - pinctrl-names: must contain a "default" entry. + + - video-ports: 24 bits value which defines how the video controller + output is wired to the TDA998x input - default: <0x230145> + +Example: + + tda998x: hdmi-encoder { + compatible = "nxp,tda998x"; + reg = <0x70>; + interrupt-parent = <&gpio0>; + interrupts = <27 2>; /* falling edge */ + pinctrl-0 = <&pmx_camera>; + pinctrl-names = "default"; + };
This patch fixes the ENABLE_SPACE register, the value of which was inverted.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 7fab61a..36e95b2 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -965,7 +965,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
if (priv->rev == TDA19988) { /* let incoming pixels fill the active space (if any) */ - reg_write(priv, REG_ENABLE_SPACE, 0x01); + reg_write(priv, REG_ENABLE_SPACE, 0x00); }
/*
There is no need to enable/disable EDID read IRQ at each EDID block read. This patch enables the IRQ at init time.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index d4074e0..5b0b6ca 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -977,9 +977,6 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk) uint8_t offset, segptr; int ret, i;
- /* enable EDID read irq: */ - reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); - offset = (blk & 1) ? 128 : 0; segptr = blk / 2;
@@ -1014,8 +1011,6 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk) return ret; }
- reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); - return 0; }
@@ -1234,6 +1229,9 @@ tda998x_encoder_init(struct i2c_client *client, cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL, CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
+ /* enable EDID read irq: */ + reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); + if (!np) return 0; /* non-DT */
This patch adds the optional treatment of the tda998x IRQ.
The interrupt function is used to know the display connection status without polling and to speedup reading the EDID.
The IRQ number and trigger type are defined in the i2c client either by platform data or in the DT.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 113 +++++++++++++++++++++++++++++++++++--- 1 file changed, 104 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 5b0b6ca..7fab61a 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -19,6 +19,7 @@
#include <linux/hdmi.h> #include <linux/module.h> +#include <linux/irq.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -39,6 +40,10 @@ struct tda998x_priv { u8 vip_cntrl_1; u8 vip_cntrl_2; struct tda998x_encoder_params params; + + wait_queue_head_t wq_edid; + volatile int wq_edid_wait; + struct drm_encoder *encoder; };
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -305,11 +310,16 @@ struct tda998x_priv {
/* CEC registers: (not paged) */ +#define REG_CEC_INTSTATUS 0xee /* read */ +# define CEC_INTSTATUS_CEC (1 << 0) +# define CEC_INTSTATUS_HDMI (1 << 1) #define REG_CEC_FRO_IM_CLK_CTRL 0xfb /* read/write */ # define CEC_FRO_IM_CLK_CTRL_GHOST_DIS (1 << 7) # define CEC_FRO_IM_CLK_CTRL_ENA_OTP (1 << 6) # define CEC_FRO_IM_CLK_CTRL_IMCLK_SEL (1 << 1) # define CEC_FRO_IM_CLK_CTRL_FRO_DIV (1 << 0) +#define REG_CEC_RXSHPDINTENA 0xfc /* read/write */ +#define REG_CEC_RXSHPDINT 0xfd /* read */ #define REG_CEC_RXSHPDLEV 0xfe /* read */ # define CEC_RXSHPDLEV_RXSENS (1 << 0) # define CEC_RXSHPDLEV_HPD (1 << 1) @@ -523,6 +533,35 @@ tda998x_reset(struct tda998x_priv *priv) reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24); }
+/* + * only 2 interrupts may occur: screen plug/unplug and EDID read + */ +static irqreturn_t tda998x_irq_thread(int irq, void *data) +{ + struct tda998x_priv *priv = data; + u8 sta, cec, lvl, flag0, flag1, flag2; + + if (!priv) + return IRQ_HANDLED; + sta = cec_read(priv, REG_CEC_INTSTATUS); + cec = cec_read(priv, REG_CEC_RXSHPDINT); + lvl = cec_read(priv, REG_CEC_RXSHPDLEV); + flag0 = reg_read(priv, REG_INT_FLAGS_0); + flag1 = reg_read(priv, REG_INT_FLAGS_1); + flag2 = reg_read(priv, REG_INT_FLAGS_2); + DRM_DEBUG_DRIVER( + "tda irq sta %02x cec %02x lvl %02x f0 %02x f1 %02x f2 %02x\n", + sta, cec, lvl, flag0, flag1, flag2); + if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) { + priv->wq_edid_wait = 0; + wake_up(&priv->wq_edid); + } else if (cec != 0) { /* HPD change */ + if (priv->encoder && priv->encoder->dev) + drm_helper_hpd_irq_event(priv->encoder->dev); + } + return IRQ_HANDLED; +} + static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) { uint8_t sum = 0; @@ -986,23 +1025,36 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk) reg_write(priv, REG_DDC_SEGM, segptr);
/* enable reading EDID: */ + priv->wq_edid_wait = 1; reg_write(priv, REG_EDID_CTRL, 0x1);
/* flag must be cleared by sw: */ reg_write(priv, REG_EDID_CTRL, 0x0);
/* wait for block read to complete: */ - for (i = 100; i > 0; i--) { - ret = reg_read(priv, REG_INT_FLAGS_2); - if (ret < 0) - return ret; - if (ret & INT_FLAGS_2_EDID_BLK_RD) - break; - msleep(1); + if (priv->hdmi->irq) { + i = wait_event_timeout(priv->wq_edid, + !priv->wq_edid_wait, + msecs_to_jiffies(100)); + if (i < 0) { + dev_err(encoder->dev->dev, "read edid wait err %d\n", i); + return i; + } + } else { + for (i = 10; i > 0; i--) { + msleep(10); + ret = reg_read(priv, REG_INT_FLAGS_2); + if (ret < 0) + return ret; + if (ret & INT_FLAGS_2_EDID_BLK_RD) + break; + } }
- if (i == 0) + if (i == 0) { + dev_err(encoder->dev->dev, "read edid timeout\n"); return -ETIMEDOUT; + }
ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH); if (ret != EDID_LENGTH) { @@ -1100,7 +1152,13 @@ static int tda998x_encoder_create_resources(struct drm_encoder *encoder, struct drm_connector *connector) { - DBG(""); + struct tda998x_priv *priv = to_tda998x_priv(encoder); + + if (priv->hdmi->irq) + connector->polled = DRM_CONNECTOR_POLL_HPD; + else + connector->polled = DRM_CONNECTOR_POLL_CONNECT | + DRM_CONNECTOR_POLL_DISCONNECT; return 0; }
@@ -1119,6 +1177,13 @@ tda998x_encoder_destroy(struct drm_encoder *encoder) { struct tda998x_priv *priv = to_tda998x_priv(encoder); drm_i2c_encoder_destroy(encoder); + + /* disable all IRQs and free the IRQ handler */ + cec_write(priv, REG_CEC_RXSHPDINTENA, 0); + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); + if (priv->hdmi->irq) + free_irq(priv->hdmi->irq, priv); + if (priv->cec) i2c_unregister_device(priv->cec); kfree(priv); @@ -1176,6 +1241,7 @@ tda998x_encoder_init(struct i2c_client *client, priv->cec = i2c_new_dummy(client->adapter, 0x34); if (!priv->cec) return -ENODEV; + priv->encoder = &encoder_slave->base; priv->dpms = DRM_MODE_DPMS_OFF;
encoder_slave->slave_priv = priv; @@ -1229,6 +1295,35 @@ tda998x_encoder_init(struct i2c_client *client, cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL, CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
+ /* initialize the optional IRQ */ + if (client->irq) { + int irqf_trigger; + + /* init read EDID waitqueue */ + init_waitqueue_head(&priv->wq_edid); + + /* clear pending interrupts */ + reg_read(priv, REG_INT_FLAGS_0); + reg_read(priv, REG_INT_FLAGS_1); + reg_read(priv, REG_INT_FLAGS_2); + + irqf_trigger = + irqd_get_trigger_type(irq_get_irq_data(client->irq)); + ret = request_threaded_irq(client->irq, NULL, + tda998x_irq_thread, + irqf_trigger | IRQF_ONESHOT, + "tda998x", priv); + if (ret) { + dev_err(&client->dev, + "failed to request IRQ#%u: %d\n", + client->irq, ret); + goto fail; + } + + /* enable HPD irq */ + cec_write(priv, REG_CEC_RXSHPDINTENA, CEC_RXSHPDLEV_HPD); + } + /* enable EDID read irq: */ reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
This patch sets the frequence as 'not indicated' instead of '48kHz' and uses the asound values in the channel status definition.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index dba5b96..dedeb22 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -20,6 +20,7 @@ #include <linux/hdmi.h> #include <linux/module.h> #include <linux/irq.h> +#include <sound/asoundef.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -710,10 +711,11 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
/* Write the channel status */ - buf[0] = 0x04; + buf[0] = IEC958_AES0_CON_NOT_COPYRIGHT; buf[1] = 0x00; - buf[2] = 0x00; - buf[3] = 0xf1; + buf[2] = IEC958_AES3_CON_FS_NOTID; + buf[3] = IEC958_AES4_CON_ORIGFS_NOTID | + IEC958_AES4_CON_MAX_WORDLEN_24; reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
tda998x_audio_mute(priv, true);
This patch reduces the number of I2C exchanges by setting many bits in one write and removing a useless write.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index dedeb22..4f1a047 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -669,10 +669,8 @@ tda998x_configure_audio(struct tda998x_priv *priv, }
reg_write(priv, REG_AIP_CLKSEL, clksel_aip); - reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT); - - /* Enable automatic CTS generation */ - reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN); + reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT | + AIP_CNTRL_0_ACR_MAN); /* auto CTS */ reg_write(priv, REG_CTS_N, cts_n);
/* @@ -908,10 +906,10 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0)); reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) | VIP_CNTRL_4_BLC(0)); - reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ); - reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE); + reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR | + PLL_SERIAL_3_SRL_DE); reg_write(priv, REG_SERIALIZER, 0); reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
@@ -931,8 +929,6 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, /* set BIAS tmds value: */ reg_write(priv, REG_ANA_GENERAL, 0x09);
- reg_write(priv, REG_TBG_CNTRL_0, 0); - /* * Sync on rising HSYNC/VSYNC */
This patch adds a definition of the values of the MUX_AP register and simplifies the macro's defining the fields of the AIP_CLKSEL register. This makes the format specific audio init sequence more readable.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 873e1e9..3e56302 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -126,6 +126,8 @@ struct tda998x_priv { # define VIP_CNTRL_5_CKCASE (1 << 0) # define VIP_CNTRL_5_SP_CNT(x) (((x) & 3) << 1) #define REG_MUX_AP REG(0x00, 0x26) /* read/write */ +# define MUX_AP_SELECT_I2S 0x64 +# define MUX_AP_SELECT_SPDIF 0x40 #define REG_MUX_VP_VIP_OUT REG(0x00, 0x27) /* read/write */ #define REG_MAT_CONTRL REG(0x00, 0x80) /* write */ # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0) @@ -203,10 +205,11 @@ struct tda998x_priv { #define REG_I2S_FORMAT REG(0x00, 0xfc) /* read/write */ # define I2S_FORMAT(x) (((x) & 3) << 0) #define REG_AIP_CLKSEL REG(0x00, 0xfd) /* write */ -# define AIP_CLKSEL_FS(x) (((x) & 3) << 0) -# define AIP_CLKSEL_CLK_POL(x) (((x) & 1) << 2) -# define AIP_CLKSEL_AIP(x) (((x) & 7) << 3) - +# define AIP_CLKSEL_AIP_SPDIF (0 << 3) +# define AIP_CLKSEL_AIP_I2S (1 << 3) +# define AIP_CLKSEL_FS_ACLK (0 << 0) +# define AIP_CLKSEL_FS_MCLK (1 << 0) +# define AIP_CLKSEL_FS_FS64SPDIF (2 << 0)
/* Page 02h: PLL settings */ #define REG_PLL_SERIAL_1 REG(0x02, 0x00) /* read/write */ @@ -645,19 +648,17 @@ tda998x_configure_audio(struct tda998x_priv *priv, /* Set audio input source */ switch (p->audio_format) { case AFMT_SPDIF: - reg_write(priv, REG_MUX_AP, 0x40); - clksel_aip = AIP_CLKSEL_AIP(0); - /* FS64SPDIF */ - clksel_fs = AIP_CLKSEL_FS(2); + reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_SPDIF); + clksel_aip = AIP_CLKSEL_AIP_SPDIF; + clksel_fs = AIP_CLKSEL_FS_FS64SPDIF; cts_n = CTS_N_M(3) | CTS_N_K(3); ca_i2s = 0; break;
case AFMT_I2S: - reg_write(priv, REG_MUX_AP, 0x64); - clksel_aip = AIP_CLKSEL_AIP(1); - /* ACLK */ - clksel_fs = AIP_CLKSEL_FS(0); + reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); + clksel_aip = AIP_CLKSEL_AIP_I2S; + clksel_fs = AIP_CLKSEL_FS_ACLK; cts_n = CTS_N_M(3) | CTS_N_K(3); ca_i2s = CA_I2S_CA_I2S(0); break;
ca_i2s is only ever written to, but never read, so let's get rid of it.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 3e56302..1242da1 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -638,7 +638,7 @@ static void tda998x_configure_audio(struct tda998x_priv *priv, struct drm_display_mode *mode, struct tda998x_encoder_params *p) { - uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv; + uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv; uint32_t n;
/* Enable audio ports */ @@ -652,7 +652,6 @@ tda998x_configure_audio(struct tda998x_priv *priv, clksel_aip = AIP_CLKSEL_AIP_SPDIF; clksel_fs = AIP_CLKSEL_FS_FS64SPDIF; cts_n = CTS_N_M(3) | CTS_N_K(3); - ca_i2s = 0; break;
case AFMT_I2S: @@ -660,7 +659,6 @@ tda998x_configure_audio(struct tda998x_priv *priv, clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; cts_n = CTS_N_M(3) | CTS_N_K(3); - ca_i2s = CA_I2S_CA_I2S(0); break;
default:
The picture aspect setting was zero, which is reserved. A setting of Same As Picture makes more sense.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 1242da1..dba5b96 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -616,6 +616,7 @@ tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode) buf[HB(1)] = 0x02; buf[HB(2)] = HDMI_AVI_INFOFRAME_SIZE; buf[PB(1)] = HDMI_SCAN_MODE_UNDERSCAN; + buf[PB(2)] = HDMI_ACTIVE_ASPECT_PICTURE; buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2; buf[PB(4)] = drm_match_cea_mode(mode);
This patch replaces hard coded values by hdmi constants.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Acked-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 16f2473..bb0c6ac 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -549,12 +549,12 @@ tda998x_write_if(struct tda998x_priv *priv, uint8_t bit, uint16_t addr, static void tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) { - uint8_t buf[PB(5) + 1]; + u8 buf[PB(HDMI_AUDIO_INFOFRAME_SIZE) + 1];
memset(buf, 0, sizeof(buf)); - buf[HB(0)] = 0x84; + buf[HB(0)] = HDMI_INFOFRAME_TYPE_AUDIO; buf[HB(1)] = 0x01; - buf[HB(2)] = 10; + buf[HB(2)] = HDMI_AUDIO_INFOFRAME_SIZE; buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */ buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */ buf[PB(4)] = p->audio_frame[4]; @@ -567,12 +567,12 @@ tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) static void tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode) { - uint8_t buf[PB(13) + 1]; + u8 buf[PB(HDMI_AVI_INFOFRAME_SIZE) + 1];
memset(buf, 0, sizeof(buf)); - buf[HB(0)] = 0x82; + buf[HB(0)] = HDMI_INFOFRAME_TYPE_AVI; buf[HB(1)] = 0x02; - buf[HB(2)] = 13; + buf[HB(2)] = HDMI_AVI_INFOFRAME_SIZE; buf[PB(1)] = HDMI_SCAN_MODE_UNDERSCAN; buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2; buf[PB(4)] = drm_match_cea_mode(mode);
This patch adds DT support to the tda998x.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 351da92..d4074e0 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1164,6 +1164,8 @@ tda998x_encoder_init(struct i2c_client *client, struct drm_encoder_slave *encoder_slave) { struct tda998x_priv *priv; + struct device_node *np = client->dev.of_node; + u32 video; int ret;
priv = kzalloc(sizeof(*priv), GFP_KERNEL); @@ -1232,6 +1234,17 @@ tda998x_encoder_init(struct i2c_client *client, cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL, CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
+ if (!np) + return 0; /* non-DT */ + + /* get the optional video properties */ + ret = of_property_read_u32(np, "video-ports", &video); + if (ret == 0) { + priv->vip_cntrl_0 = video >> 16; + priv->vip_cntrl_1 = video >> 8; + priv->vip_cntrl_2 = video; + } + return 0;
fail: @@ -1246,6 +1259,14 @@ fail: return -ENXIO; }
+#ifdef CONFIG_OF +static const struct of_device_id tda998x_dt_ids[] = { + { .compatible = "nxp,tda998x", }, + { } +}; +MODULE_DEVICE_TABLE(of, tda998x_dt_ids); +#endif + static struct i2c_device_id tda998x_ids[] = { { "tda998x", 0 }, { } @@ -1258,6 +1279,7 @@ static struct drm_i2c_encoder_driver tda998x_driver = { .remove = tda998x_remove, .driver = { .name = "tda998x", + .of_match_table = of_match_ptr(tda998x_dt_ids), }, .id_table = tda998x_ids, },
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits cleared after reset, so, they may be fully re-written.
The register MAT_CONTRL is set to MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) after reset, so, it may be fully set again to this value.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 46 ++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index bb0c6ac..d6d0e80 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -495,9 +495,9 @@ static void tda998x_reset(struct tda998x_priv *priv) { /* reset audio and i2c master: */ - reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); + reg_write(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); msleep(50); - reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); + reg_write(priv, REG_SOFTRESET, 0); msleep(50);
/* reset transmitter: */ @@ -852,7 +852,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
/* set HDMI HDCP mode off: */ - reg_set(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); + reg_write(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); reg_clear(priv, REG_TX33, TX33_HDMI); reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
@@ -879,38 +879,28 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, PLL_SERIAL_2_SRL_PR(rep));
/* set color matrix bypass flag: */ - reg_set(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP); + reg_write(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP | + MAT_CONTRL_MAT_SC(1));
/* set BIAS tmds value: */ reg_write(priv, REG_ANA_GENERAL, 0x09);
- reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD); + reg_write(priv, REG_TBG_CNTRL_0, 0);
/* * Sync on rising HSYNC/VSYNC */ - reg_write(priv, REG_VIP_CNTRL_3, 0); - reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS); + reg = VIP_CNTRL_3_SYNC_HS;
/* * TDA19988 requires high-active sync at input stage, * so invert low-active sync provided by master encoder here */ if (mode->flags & DRM_MODE_FLAG_NHSYNC) - reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL); + reg |= VIP_CNTRL_3_H_TGL; if (mode->flags & DRM_MODE_FLAG_NVSYNC) - reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL); - - /* - * Always generate sync polarity relative to input sync and - * revert input stage toggled sync at output stage - */ - reg = TBG_CNTRL_1_TGL_EN; - if (mode->flags & DRM_MODE_FLAG_NHSYNC) - reg |= TBG_CNTRL_1_H_TGL; - if (mode->flags & DRM_MODE_FLAG_NVSYNC) - reg |= TBG_CNTRL_1_V_TGL; - reg_write(priv, REG_TBG_CNTRL_1, reg); + reg |= VIP_CNTRL_3_V_TGL; + reg_write(priv, REG_VIP_CNTRL_3, reg);
reg_write(priv, REG_VIDFORMAT, 0x00); reg_write16(priv, REG_REFPIX_MSB, ref_pix); @@ -939,13 +929,25 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, reg_write(priv, REG_ENABLE_SPACE, 0x01); }
+ /* + * Always generate sync polarity relative to input sync and + * revert input stage toggled sync at output stage + */ + reg = TBG_CNTRL_1_DWIN_DIS | TBG_CNTRL_1_TGL_EN; + if (mode->flags & DRM_MODE_FLAG_NHSYNC) + reg |= TBG_CNTRL_1_H_TGL; + if (mode->flags & DRM_MODE_FLAG_NVSYNC) + reg |= TBG_CNTRL_1_V_TGL; + reg_write(priv, REG_TBG_CNTRL_1, reg); + /* must be last register set: */ - reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); + reg_write(priv, REG_TBG_CNTRL_0, 0);
/* Only setup the info frames if the sink is HDMI */ if (priv->is_hdmi_sink) { /* We need to turn HDMI HDCP stuff on to get audio through */ - reg_clear(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); + reg &= ~TBG_CNTRL_1_DWIN_DIS; + reg_write(priv, REG_TBG_CNTRL_1, reg); reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1)); reg_set(priv, REG_TX33, TX33_HDMI);
On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote:
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits cleared after reset, so, they may be fully re-written.
The register MAT_CONTRL is set to MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) after reset, so, it may be fully set again to this value.
I said in v3 of this patch, which seems to remain unaddressed:
/* must be last register set: */
reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
reg_write(priv, REG_TBG_CNTRL_0, 0);
Register changes which have a potential effect shouldn't be part of a patch which is really only trying to avoid reading from write only registers.
This could be a potential functional change - and it's probably one which Rob Clark should at least be made aware of. As I commented last time, when you're changing register values in an otherwise innocuous patch, you should comment about them in the patch description.
On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote:
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits cleared after reset, so, they may be fully re-written.
The register MAT_CONTRL is set to MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) after reset, so, it may be fully set again to this value.
I said in v3 of this patch, which seems to remain unaddressed:
/* must be last register set: */
reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
reg_write(priv, REG_TBG_CNTRL_0, 0);Register changes which have a potential effect shouldn't be part of a patch which is really only trying to avoid reading from write only registers.
This could be a potential functional change - and it's probably one which Rob Clark should at least be made aware of. As I commented last time, when you're changing register values in an otherwise innocuous patch, you should comment about them in the patch description.
According to the tda9983b documentation, the register TBG_CNTRL_0 is set to 0 at reset time. I think that it is the same for all the tda998x family. In the other hand, this register is supposed to be write only, so reading it may return any value, and the reg_clear() function may set any other bits. Then, clearing one bit is less secure than clearing the full register.
On Sun, Feb 02, 2014 at 06:45:12PM +0100, Jean-Francois Moine wrote:
On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Sat, Jan 25, 2014 at 06:14:42PM +0100, Jean-Francois Moine wrote:
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET, TBG_CNTRL_0 and TBG_CNTRL_1 have all bits cleared after reset, so, they may be fully re-written.
The register MAT_CONTRL is set to MAT_CONTRL_MAT_BP | MAT_CONTRL_MAT_SC(1) after reset, so, it may be fully set again to this value.
I said in v3 of this patch, which seems to remain unaddressed:
/* must be last register set: */
reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
reg_write(priv, REG_TBG_CNTRL_0, 0);Register changes which have a potential effect shouldn't be part of a patch which is really only trying to avoid reading from write only registers.
This could be a potential functional change - and it's probably one which Rob Clark should at least be made aware of. As I commented last time, when you're changing register values in an otherwise innocuous patch, you should comment about them in the patch description.
According to the tda9983b documentation, the register TBG_CNTRL_0 is set to 0 at reset time. I think that it is the same for all the tda998x family. In the other hand, this register is supposed to be write only, so reading it may return any value, and the reg_clear() function may set any other bits. Then, clearing one bit is less secure than clearing the full register.
Okay, in that case I'm happy with this patch.
The cec i2c device is created in tda998x_encoder_init() when the DRM driver starts. This patch frees it when the DRM driver is unloaded.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Acked-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index d6d0e80..7d8bc22 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1124,6 +1124,8 @@ tda998x_encoder_destroy(struct drm_encoder *encoder) { struct tda998x_priv *priv = to_tda998x_priv(encoder); drm_i2c_encoder_destroy(encoder); + if (priv->cec) + i2c_unregister_device(priv->cec); kfree(priv); }
This patch checks if the CEC device is well created at intialization time.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 7d8bc22..351da92 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1177,6 +1177,8 @@ tda998x_encoder_init(struct i2c_client *client, priv->current_page = 0xff; priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34); + if (!priv->cec) + return -ENODEV; priv->dpms = DRM_MODE_DPMS_OFF;
encoder_slave->slave_priv = priv;
This patch prevents the system to be freezed at audio startup time, replacing mdelay by msleep.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Acked-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index fbd7937..96b5966 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -678,7 +678,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
tda998x_audio_mute(priv, true); - mdelay(20); + msleep(20); tda998x_audio_mute(priv, false);
/* Write the audio information packet */
The AIF has an uninitialized byte. This patch clears the whole buffer before filling it.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Acked-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b688801..16f2473 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -551,10 +551,10 @@ tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) { uint8_t buf[PB(5) + 1];
+ memset(buf, 0, sizeof(buf)); buf[HB(0)] = 0x84; buf[HB(1)] = 0x01; buf[HB(2)] = 10; - buf[PB(0)] = 0; buf[PB(1)] = p->audio_frame[1] & 0x07; /* CC */ buf[PB(2)] = p->audio_frame[2] & 0x1c; /* SF */ buf[PB(4)] = p->audio_frame[4];
This patch adds more error checking inn I2C I/O functions. In case of I/O error, this permits to avoid writing in bad controller pages, a bad chipset detection or looping when getting the EDID.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 57 +++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 26dd299..11f0607 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -362,7 +362,7 @@ fail: return 0; }
-static void +static int set_page(struct tda998x_priv *priv, uint16_t reg) { if (REG2PAGE(reg) != priv->current_page) { @@ -371,11 +371,14 @@ set_page(struct tda998x_priv *priv, uint16_t reg) REG_CURPAGE, REG2PAGE(reg) }; int ret = i2c_master_send(client, buf, sizeof(buf)); - if (ret < 0) + if (ret < 0) { dev_err(&client->dev, "Error %d writing to REG_CURPAGE\n", ret); + return ret; + }
priv->current_page = REG2PAGE(reg); } + return 0; }
static int @@ -385,7 +388,9 @@ reg_read_range(struct tda998x_priv *priv, uint16_t reg, char *buf, int cnt) uint8_t addr = REG2ADDR(reg); int ret;
- set_page(priv, reg); + ret = set_page(priv, reg); + if (ret < 0) + return ret;
ret = i2c_master_send(client, &addr, sizeof(addr)); if (ret < 0) @@ -412,18 +417,24 @@ reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt) buf[0] = REG2ADDR(reg); memcpy(&buf[1], p, cnt);
- set_page(priv, reg); + ret = set_page(priv, reg); + if (ret < 0) + return;
ret = i2c_master_send(client, buf, cnt + 1); if (ret < 0) dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); }
-static uint8_t +static int reg_read(struct tda998x_priv *priv, uint16_t reg) { uint8_t val = 0; - reg_read_range(priv, reg, &val, sizeof(val)); + int ret; + + ret = reg_read_range(priv, reg, &val, sizeof(val)); + if (ret < 0) + return ret; return val; }
@@ -434,7 +445,9 @@ reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val) uint8_t buf[] = {REG2ADDR(reg), val}; int ret;
- set_page(priv, reg); + ret = set_page(priv, reg); + if (ret < 0) + return;
ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); if (ret < 0) @@ -448,7 +461,9 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val) uint8_t buf[] = {REG2ADDR(reg), val >> 8, val}; int ret;
- set_page(priv, reg); + ret = set_page(priv, reg); + if (ret < 0) + return;
ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); if (ret < 0) @@ -458,13 +473,21 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val) static void reg_set(struct tda998x_priv *priv, uint16_t reg, uint8_t val) { - reg_write(priv, reg, reg_read(priv, reg) | val); + int old_val; + + old_val = reg_read(priv, reg); + if (old_val >= 0) + reg_write(priv, reg, old_val | val); }
static void reg_clear(struct tda998x_priv *priv, uint16_t reg, uint8_t val) { - reg_write(priv, reg, reg_read(priv, reg) & ~val); + int old_val; + + old_val = reg_read(priv, reg); + if (old_val >= 0) + reg_write(priv, reg, old_val & ~val); }
static void @@ -970,8 +993,10 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
/* wait for block read to complete: */ for (i = 100; i > 0; i--) { - uint8_t val = reg_read(priv, REG_INT_FLAGS_2); - if (val & INT_FLAGS_2_EDID_BLK_RD) + ret = reg_read(priv, REG_INT_FLAGS_2); + if (ret < 0) + return ret; + if (ret & INT_FLAGS_2_EDID_BLK_RD) break; msleep(1); } @@ -1134,6 +1159,7 @@ tda998x_encoder_init(struct i2c_client *client, struct drm_encoder_slave *encoder_slave) { struct tda998x_priv *priv; + int ret;
priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (!priv) @@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client, tda998x_reset(priv);
/* read version: */ - priv->rev = reg_read(priv, REG_VERSION_LSB) | - reg_read(priv, REG_VERSION_MSB) << 8; + ret = reg_read(priv, REG_VERSION_LSB) | + (reg_read(priv, REG_VERSION_MSB) << 8); + if (ret < 0) + goto fail; + priv->rev = ret;
/* mask off feature bits: */ priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
This patch adds more error checking inn I2C I/O functions. In case of I/O error, this permits to avoid writing in bad controller pages, a bad chipset detection or looping when getting the EDID.
I've just looked at this again, and spotted something:
-static uint8_t +static int reg_read(struct tda998x_priv *priv, uint16_t reg) { uint8_t val = 0;
- reg_read_range(priv, reg, &val, sizeof(val));
- int ret;
- ret = reg_read_range(priv, reg, &val, sizeof(val));
- if (ret < 0)
return ret;
So yes, this can return negative numbers.
@@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client, tda998x_reset(priv);
/* read version: */
- priv->rev = reg_read(priv, REG_VERSION_LSB) |
reg_read(priv, REG_VERSION_MSB) << 8;
- ret = reg_read(priv, REG_VERSION_LSB) |
(reg_read(priv, REG_VERSION_MSB) << 8);- if (ret < 0)
goto fail;- priv->rev = ret;
Two issues here:
1. The additional parens are /really/ not required. 2. What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
If we're going to the extent of attempting to make the read/write functions return errors, we should at least handle errors generated by them properly, otherwise it's pointless making them return errors.
ret = reg_read(priv, REG_VERSION_LSB); if (ret < 0) goto fail;
priv->rev = ret;
ret = reg_read(priv, REG_VERSION_MSB); if (ret < 0) goto fail;
priv->rev |= ret << 8;
If you want it to look slightly nicer:
int rev_lo, rev_hi;
rev_lo = reg_read(priv, REG_VERSION_LSB); rev_hi = reg_read(priv, REG_VERSION_MSB); if (rev_lo < 0 || rev_hi < 0) { ret = rev_lo < 0 ? rev_lo : rev_hi; goto fail; }
priv->rev = rev_lo | rev_hi << 8;
I'm happy to commit such a change after this patch to clean it up, or if you want to regenerate your patch 2 and post /just/ that incorporating this change.
On Sun, 2 Feb 2014 16:20:58 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
This patch adds more error checking inn I2C I/O functions. In case of I/O error, this permits to avoid writing in bad controller pages, a bad chipset detection or looping when getting the EDID.
I've just looked at this again, and spotted something:
-static uint8_t +static int reg_read(struct tda998x_priv *priv, uint16_t reg) { uint8_t val = 0;
- reg_read_range(priv, reg, &val, sizeof(val));
- int ret;
- ret = reg_read_range(priv, reg, &val, sizeof(val));
- if (ret < 0)
return ret;So yes, this can return negative numbers.
@@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client, tda998x_reset(priv);
/* read version: */
- priv->rev = reg_read(priv, REG_VERSION_LSB) |
reg_read(priv, REG_VERSION_MSB) << 8;
- ret = reg_read(priv, REG_VERSION_LSB) |
(reg_read(priv, REG_VERSION_MSB) << 8);- if (ret < 0)
goto fail;- priv->rev = ret;
Two issues here:
- The additional parens are /really/ not required.
- What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
If we're going to the extent of attempting to make the read/write functions return errors, we should at least handle errors generated by them properly, otherwise it's pointless making them return errors.
ret = reg_read(priv, REG_VERSION_LSB); if (ret < 0) goto fail;
priv->rev = ret;
ret = reg_read(priv, REG_VERSION_MSB); if (ret < 0) goto fail;
priv->rev |= ret << 8;
If you want it to look slightly nicer:
int rev_lo, rev_hi;
rev_lo = reg_read(priv, REG_VERSION_LSB); rev_hi = reg_read(priv, REG_VERSION_MSB); if (rev_lo < 0 || rev_hi < 0) { ret = rev_lo < 0 ? rev_lo : rev_hi; goto fail; }
priv->rev = rev_lo | rev_hi << 8;
I'm happy to commit such a change after this patch to clean it up, or if you want to regenerate your patch 2 and post /just/ that incorporating this change.
I think that my code works correctly: when there is an error, the result of reg_read() is minus the error code, and this error code is always lower than 8388607 (0x7fffff). Then, reg_read() << 8 will always be negative.
Otherwise, I may redo a patch about the useless parenthesis.
On Sun, Feb 02, 2014 at 06:30:00PM +0100, Jean-Francois Moine wrote:
On Sun, 2 Feb 2014 16:20:58 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:
This patch adds more error checking inn I2C I/O functions. In case of I/O error, this permits to avoid writing in bad controller pages, a bad chipset detection or looping when getting the EDID.
I've just looked at this again, and spotted something:
-static uint8_t +static int reg_read(struct tda998x_priv *priv, uint16_t reg) { uint8_t val = 0;
- reg_read_range(priv, reg, &val, sizeof(val));
- int ret;
- ret = reg_read_range(priv, reg, &val, sizeof(val));
- if (ret < 0)
return ret;So yes, this can return negative numbers.
@@ -1158,8 +1184,11 @@ tda998x_encoder_init(struct i2c_client *client, tda998x_reset(priv);
/* read version: */
- priv->rev = reg_read(priv, REG_VERSION_LSB) |
reg_read(priv, REG_VERSION_MSB) << 8;
- ret = reg_read(priv, REG_VERSION_LSB) |
(reg_read(priv, REG_VERSION_MSB) << 8);- if (ret < 0)
goto fail;- priv->rev = ret;
Two issues here:
- The additional parens are /really/ not required.
- What if reg_read(priv, REG_VERSION_MSB) returns a negative number?
If we're going to the extent of attempting to make the read/write functions return errors, we should at least handle errors generated by them properly, otherwise it's pointless making them return errors.
ret = reg_read(priv, REG_VERSION_LSB); if (ret < 0) goto fail;
priv->rev = ret;
ret = reg_read(priv, REG_VERSION_MSB); if (ret < 0) goto fail;
priv->rev |= ret << 8;
If you want it to look slightly nicer:
int rev_lo, rev_hi;
rev_lo = reg_read(priv, REG_VERSION_LSB); rev_hi = reg_read(priv, REG_VERSION_MSB); if (rev_lo < 0 || rev_hi < 0) { ret = rev_lo < 0 ? rev_lo : rev_hi; goto fail; }
priv->rev = rev_lo | rev_hi << 8;
I'm happy to commit such a change after this patch to clean it up, or if you want to regenerate your patch 2 and post /just/ that incorporating this change.
I think that my code works correctly: when there is an error, the result of reg_read() is minus the error code, and this error code is always lower than 8388607 (0x7fffff). Then, reg_read() << 8 will always be negative.
The issue I'm pointing out is not whether it will be interpreted as an error or not, it's whether the value is a correct error code. If we don't care about the reason why an error occurs, we might as well just return -1.
I've added my own patch which adjusts it as per above to my tree anyway, so I'm not that worried about this.
This patch forces the page register to be set on the first I/O operation.
Tested-by: Russell King rmk+kernel@arm.linux.org.uk Acked-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 96b5966..b688801 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1170,7 +1170,7 @@ tda998x_encoder_init(struct i2c_client *client, priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
- priv->current_page = 0; + priv->current_page = 0xff; priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34); priv->dpms = DRM_MODE_DPMS_OFF;
This patch: - replaces ARRAY_SIZE() by sizeof() when a number of bytes is needed, - adds a linefeed in an error message and - removes an useless variable setting.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 11f0607..320b37f 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -335,7 +335,7 @@ cec_write(struct tda998x_priv *priv, uint16_t addr, uint8_t val) uint8_t buf[] = {addr, val}; int ret;
- ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); + ret = i2c_master_send(client, buf, sizeof(buf)); if (ret < 0) dev_err(&client->dev, "Error %d writing to cec:0x%x\n", ret, addr); } @@ -372,7 +372,8 @@ set_page(struct tda998x_priv *priv, uint16_t reg) }; int ret = i2c_master_send(client, buf, sizeof(buf)); if (ret < 0) { - dev_err(&client->dev, "Error %d writing to REG_CURPAGE\n", ret); + dev_err(&client->dev, "setpage %04x err %d\n", + reg, ret); return ret; }
@@ -449,7 +450,7 @@ reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val) if (ret < 0) return;
- ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); + ret = i2c_master_send(client, buf, sizeof(buf)); if (ret < 0) dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); } @@ -465,7 +466,7 @@ reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val) if (ret < 0) return;
- ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); + ret = i2c_master_send(client, buf, sizeof(buf)); if (ret < 0) dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); } @@ -1006,7 +1007,7 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH); if (ret != EDID_LENGTH) { - dev_err(encoder->dev->dev, "failed to read edid block %d: %d", + dev_err(encoder->dev->dev, "failed to read edid block %d: %d\n", blk, ret); return ret; } @@ -1020,7 +1021,7 @@ static uint8_t * do_get_edid(struct drm_encoder *encoder) { struct tda998x_priv *priv = to_tda998x_priv(encoder); - int j = 0, valid_extensions = 0; + int j, valid_extensions = 0; uint8_t *block, *new; bool print_bad_edid = drm_debug & DRM_UT_KMS;
On probe, a message giving the TDA chip version seems to come from the DRM driver:
armada-drm armada-510-drm: found TDA19988
This patch changes the originator of the message to the TDA driver:
tda998x 0-0070: found TDA19988
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 320b37f..fbd7937 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -1195,12 +1195,21 @@ tda998x_encoder_init(struct i2c_client *client, priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */
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; + case TDA9989N2: + dev_info(&client->dev, "found TDA9989 n2"); + break; + case TDA19989: + dev_info(&client->dev, "found TDA19989"); + break; + case TDA19989N2: + dev_info(&client->dev, "found TDA19989 n2"); + break; + case TDA19988: + dev_info(&client->dev, "found TDA19988"); + break; default: - DBG("found unsupported device: %04x", priv->rev); + dev_err(&client->dev, "found unsupported device: %04x\n", + priv->rev); goto fail; }
This patch simplifies the i2c read/write functions and permits them to be easily called in more contexts.
Acked-by: Russell King rmk+kernel@arm.linux.org.uk Tested-by: Russell King rmk+kernel@arm.linux.org.uk Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 322 +++++++++++++++++++------------------- 1 file changed, 162 insertions(+), 160 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 400b0c4..26dd299 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -30,6 +30,7 @@
struct tda998x_priv { struct i2c_client *cec; + struct i2c_client *hdmi; uint16_t rev; uint8_t current_page; int dpms; @@ -328,9 +329,9 @@ struct tda998x_priv { #define TDA19988 0x0301
static void -cec_write(struct drm_encoder *encoder, uint16_t addr, uint8_t val) +cec_write(struct tda998x_priv *priv, uint16_t addr, uint8_t val) { - struct i2c_client *client = to_tda998x_priv(encoder)->cec; + struct i2c_client *client = priv->cec; uint8_t buf[] = {addr, val}; int ret;
@@ -340,9 +341,9 @@ cec_write(struct drm_encoder *encoder, uint16_t addr, uint8_t val) }
static uint8_t -cec_read(struct drm_encoder *encoder, uint8_t addr) +cec_read(struct tda998x_priv *priv, uint8_t addr) { - struct i2c_client *client = to_tda998x_priv(encoder)->cec; + struct i2c_client *client = priv->cec; uint8_t val; int ret;
@@ -362,12 +363,10 @@ fail: }
static void -set_page(struct drm_encoder *encoder, uint16_t reg) +set_page(struct tda998x_priv *priv, uint16_t reg) { - struct tda998x_priv *priv = to_tda998x_priv(encoder); - if (REG2PAGE(reg) != priv->current_page) { - struct i2c_client *client = drm_i2c_encoder_get_client(encoder); + struct i2c_client *client = priv->hdmi; uint8_t buf[] = { REG_CURPAGE, REG2PAGE(reg) }; @@ -380,13 +379,13 @@ set_page(struct drm_encoder *encoder, uint16_t reg) }
static int -reg_read_range(struct drm_encoder *encoder, uint16_t reg, char *buf, int cnt) +reg_read_range(struct tda998x_priv *priv, uint16_t reg, char *buf, int cnt) { - struct i2c_client *client = drm_i2c_encoder_get_client(encoder); + struct i2c_client *client = priv->hdmi; uint8_t addr = REG2ADDR(reg); int ret;
- set_page(encoder, reg); + set_page(priv, reg);
ret = i2c_master_send(client, &addr, sizeof(addr)); if (ret < 0) @@ -404,16 +403,16 @@ fail: }
static void -reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt) +reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt) { - struct i2c_client *client = drm_i2c_encoder_get_client(encoder); + struct i2c_client *client = priv->hdmi; uint8_t buf[cnt+1]; int ret;
buf[0] = REG2ADDR(reg); memcpy(&buf[1], p, cnt);
- set_page(encoder, reg); + set_page(priv, reg);
ret = i2c_master_send(client, buf, cnt + 1); if (ret < 0) @@ -421,21 +420,21 @@ reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int cnt) }
static uint8_t -reg_read(struct drm_encoder *encoder, uint16_t reg) +reg_read(struct tda998x_priv *priv, uint16_t reg) { uint8_t val = 0; - reg_read_range(encoder, reg, &val, sizeof(val)); + reg_read_range(priv, reg, &val, sizeof(val)); return val; }
static void -reg_write(struct drm_encoder *encoder, uint16_t reg, uint8_t val) +reg_write(struct tda998x_priv *priv, uint16_t reg, uint8_t val) { - struct i2c_client *client = drm_i2c_encoder_get_client(encoder); + struct i2c_client *client = priv->hdmi; uint8_t buf[] = {REG2ADDR(reg), val}; int ret;
- set_page(encoder, reg); + set_page(priv, reg);
ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); if (ret < 0) @@ -443,13 +442,13 @@ reg_write(struct drm_encoder *encoder, uint16_t reg, uint8_t val) }
static void -reg_write16(struct drm_encoder *encoder, uint16_t reg, uint16_t val) +reg_write16(struct tda998x_priv *priv, uint16_t reg, uint16_t val) { - struct i2c_client *client = drm_i2c_encoder_get_client(encoder); + struct i2c_client *client = priv->hdmi; uint8_t buf[] = {REG2ADDR(reg), val >> 8, val}; int ret;
- set_page(encoder, reg); + set_page(priv, reg);
ret = i2c_master_send(client, buf, ARRAY_SIZE(buf)); if (ret < 0) @@ -457,47 +456,47 @@ reg_write16(struct drm_encoder *encoder, uint16_t reg, uint16_t val) }
static void -reg_set(struct drm_encoder *encoder, uint16_t reg, uint8_t val) +reg_set(struct tda998x_priv *priv, uint16_t reg, uint8_t val) { - reg_write(encoder, reg, reg_read(encoder, reg) | val); + reg_write(priv, reg, reg_read(priv, reg) | val); }
static void -reg_clear(struct drm_encoder *encoder, uint16_t reg, uint8_t val) +reg_clear(struct tda998x_priv *priv, uint16_t reg, uint8_t val) { - reg_write(encoder, reg, reg_read(encoder, reg) & ~val); + reg_write(priv, reg, reg_read(priv, reg) & ~val); }
static void -tda998x_reset(struct drm_encoder *encoder) +tda998x_reset(struct tda998x_priv *priv) { /* reset audio and i2c master: */ - reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); + reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); msleep(50); - reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); + reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO | SOFTRESET_I2C_MASTER); msleep(50);
/* reset transmitter: */ - reg_set(encoder, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR); - reg_clear(encoder, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR); + reg_set(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR); + reg_clear(priv, REG_MAIN_CNTRL0, MAIN_CNTRL0_SR);
/* PLL registers common configuration */ - reg_write(encoder, REG_PLL_SERIAL_1, 0x00); - reg_write(encoder, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1)); - reg_write(encoder, REG_PLL_SERIAL_3, 0x00); - reg_write(encoder, REG_SERIALIZER, 0x00); - reg_write(encoder, REG_BUFFER_OUT, 0x00); - reg_write(encoder, REG_PLL_SCG1, 0x00); - reg_write(encoder, REG_AUDIO_DIV, AUDIO_DIV_SERCLK_8); - reg_write(encoder, REG_SEL_CLK, SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK); - reg_write(encoder, REG_PLL_SCGN1, 0xfa); - reg_write(encoder, REG_PLL_SCGN2, 0x00); - reg_write(encoder, REG_PLL_SCGR1, 0x5b); - reg_write(encoder, REG_PLL_SCGR2, 0x00); - reg_write(encoder, REG_PLL_SCG2, 0x10); + reg_write(priv, REG_PLL_SERIAL_1, 0x00); + reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(1)); + reg_write(priv, REG_PLL_SERIAL_3, 0x00); + reg_write(priv, REG_SERIALIZER, 0x00); + reg_write(priv, REG_BUFFER_OUT, 0x00); + reg_write(priv, REG_PLL_SCG1, 0x00); + reg_write(priv, REG_AUDIO_DIV, AUDIO_DIV_SERCLK_8); + reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK); + reg_write(priv, REG_PLL_SCGN1, 0xfa); + reg_write(priv, REG_PLL_SCGN2, 0x00); + reg_write(priv, REG_PLL_SCGR1, 0x5b); + reg_write(priv, REG_PLL_SCGR2, 0x00); + reg_write(priv, REG_PLL_SCG2, 0x10);
/* Write the default value MUX register */ - reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24); + reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24); }
static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) @@ -513,18 +512,18 @@ static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) #define PB(x) (HB(2) + 1 + (x))
static void -tda998x_write_if(struct drm_encoder *encoder, uint8_t bit, uint16_t addr, +tda998x_write_if(struct tda998x_priv *priv, uint8_t bit, uint16_t addr, uint8_t *buf, size_t size) { buf[PB(0)] = tda998x_cksum(buf, size);
- reg_clear(encoder, REG_DIP_IF_FLAGS, bit); - reg_write_range(encoder, addr, buf, size); - reg_set(encoder, REG_DIP_IF_FLAGS, bit); + reg_clear(priv, REG_DIP_IF_FLAGS, bit); + reg_write_range(priv, addr, buf, size); + reg_set(priv, REG_DIP_IF_FLAGS, bit); }
static void -tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p) +tda998x_write_aif(struct tda998x_priv *priv, struct tda998x_encoder_params *p) { uint8_t buf[PB(5) + 1];
@@ -537,12 +536,12 @@ tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params *p) buf[PB(4)] = p->audio_frame[4]; buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */
- tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, + tda998x_write_if(priv, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, sizeof(buf)); }
static void -tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode) +tda998x_write_avi(struct tda998x_priv *priv, struct drm_display_mode *mode) { uint8_t buf[PB(13) + 1];
@@ -554,36 +553,36 @@ tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode) buf[PB(3)] = HDMI_QUANTIZATION_RANGE_FULL << 2; buf[PB(4)] = drm_match_cea_mode(mode);
- tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf, + tda998x_write_if(priv, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf, sizeof(buf)); }
-static void tda998x_audio_mute(struct drm_encoder *encoder, bool on) +static void tda998x_audio_mute(struct tda998x_priv *priv, bool on) { if (on) { - reg_set(encoder, REG_SOFTRESET, SOFTRESET_AUDIO); - reg_clear(encoder, REG_SOFTRESET, SOFTRESET_AUDIO); - reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO); + reg_set(priv, REG_SOFTRESET, SOFTRESET_AUDIO); + reg_clear(priv, REG_SOFTRESET, SOFTRESET_AUDIO); + reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO); } else { - reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO); + reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO); } }
static void -tda998x_configure_audio(struct drm_encoder *encoder, +tda998x_configure_audio(struct tda998x_priv *priv, struct drm_display_mode *mode, struct tda998x_encoder_params *p) { uint8_t buf[6], clksel_aip, clksel_fs, ca_i2s, cts_n, adiv; uint32_t n;
/* Enable audio ports */ - reg_write(encoder, REG_ENA_AP, p->audio_cfg); - reg_write(encoder, REG_ENA_ACLK, p->audio_clk_cfg); + reg_write(priv, REG_ENA_AP, p->audio_cfg); + reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
/* Set audio input source */ switch (p->audio_format) { case AFMT_SPDIF: - reg_write(encoder, REG_MUX_AP, 0x40); + reg_write(priv, REG_MUX_AP, 0x40); clksel_aip = AIP_CLKSEL_AIP(0); /* FS64SPDIF */ clksel_fs = AIP_CLKSEL_FS(2); @@ -592,7 +591,7 @@ tda998x_configure_audio(struct drm_encoder *encoder, break;
case AFMT_I2S: - reg_write(encoder, REG_MUX_AP, 0x64); + reg_write(priv, REG_MUX_AP, 0x64); clksel_aip = AIP_CLKSEL_AIP(1); /* ACLK */ clksel_fs = AIP_CLKSEL_FS(0); @@ -605,12 +604,12 @@ tda998x_configure_audio(struct drm_encoder *encoder, return; }
- reg_write(encoder, REG_AIP_CLKSEL, clksel_aip); - reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT); + reg_write(priv, REG_AIP_CLKSEL, clksel_aip); + reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT);
/* Enable automatic CTS generation */ - reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN); - reg_write(encoder, REG_CTS_N, cts_n); + reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_ACR_MAN); + reg_write(priv, REG_CTS_N, cts_n);
/* * Audio input somehow depends on HDMI line rate which is @@ -623,7 +622,7 @@ tda998x_configure_audio(struct drm_encoder *encoder, adiv = AUDIO_DIV_SERCLK_16; else adiv = AUDIO_DIV_SERCLK_8; - reg_write(encoder, REG_AUDIO_DIV, adiv); + reg_write(priv, REG_AUDIO_DIV, adiv);
/* * This is the approximate value of N, which happens to be @@ -638,28 +637,28 @@ tda998x_configure_audio(struct drm_encoder *encoder, buf[3] = n; buf[4] = n >> 8; buf[5] = n >> 16; - reg_write_range(encoder, REG_ACR_CTS_0, buf, 6); + reg_write_range(priv, REG_ACR_CTS_0, buf, 6);
/* Set CTS clock reference */ - reg_write(encoder, REG_AIP_CLKSEL, clksel_aip | clksel_fs); + reg_write(priv, REG_AIP_CLKSEL, clksel_aip | clksel_fs);
/* Reset CTS generator */ - reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); - reg_clear(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); + reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS); + reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_CTS);
/* Write the channel status */ buf[0] = 0x04; buf[1] = 0x00; buf[2] = 0x00; buf[3] = 0xf1; - reg_write_range(encoder, REG_CH_STAT_B(0), buf, 4); + reg_write_range(priv, REG_CH_STAT_B(0), buf, 4);
- tda998x_audio_mute(encoder, true); + tda998x_audio_mute(priv, true); mdelay(20); - tda998x_audio_mute(encoder, false); + tda998x_audio_mute(priv, false);
/* Write the audio information packet */ - tda998x_write_aif(encoder, p); + tda998x_write_aif(priv, p); }
/* DRM encoder functions */ @@ -701,19 +700,19 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) switch (mode) { case DRM_MODE_DPMS_ON: /* enable video ports, audio will be enabled later */ - reg_write(encoder, REG_ENA_VP_0, 0xff); - reg_write(encoder, REG_ENA_VP_1, 0xff); - reg_write(encoder, REG_ENA_VP_2, 0xff); + reg_write(priv, REG_ENA_VP_0, 0xff); + reg_write(priv, REG_ENA_VP_1, 0xff); + reg_write(priv, REG_ENA_VP_2, 0xff); /* set muxing after enabling ports: */ - reg_write(encoder, REG_VIP_CNTRL_0, priv->vip_cntrl_0); - reg_write(encoder, REG_VIP_CNTRL_1, priv->vip_cntrl_1); - reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2); + reg_write(priv, REG_VIP_CNTRL_0, priv->vip_cntrl_0); + reg_write(priv, REG_VIP_CNTRL_1, priv->vip_cntrl_1); + reg_write(priv, REG_VIP_CNTRL_2, priv->vip_cntrl_2); break; case DRM_MODE_DPMS_OFF: /* disable video ports */ - reg_write(encoder, REG_ENA_VP_0, 0x00); - reg_write(encoder, REG_ENA_VP_1, 0x00); - reg_write(encoder, REG_ENA_VP_2, 0x00); + reg_write(priv, REG_ENA_VP_0, 0x00); + reg_write(priv, REG_ENA_VP_1, 0x00); + reg_write(priv, REG_ENA_VP_2, 0x00); break; }
@@ -826,57 +825,57 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, div = 148500 / mode->clock;
/* mute the audio FIFO: */ - reg_set(encoder, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO); + reg_set(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_RST_FIFO);
/* set HDMI HDCP mode off: */ - reg_set(encoder, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); - reg_clear(encoder, REG_TX33, TX33_HDMI); + reg_set(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); + reg_clear(priv, REG_TX33, TX33_HDMI); + reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0));
- reg_write(encoder, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(0)); /* no pre-filter or interpolator: */ - reg_write(encoder, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) | + reg_write(priv, REG_HVF_CNTRL_0, HVF_CNTRL_0_PREFIL(0) | HVF_CNTRL_0_INTPOL(0)); - reg_write(encoder, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0)); - reg_write(encoder, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) | + reg_write(priv, REG_VIP_CNTRL_5, VIP_CNTRL_5_SP_CNT(0)); + reg_write(priv, REG_VIP_CNTRL_4, VIP_CNTRL_4_BLANKIT(0) | VIP_CNTRL_4_BLC(0)); - reg_clear(encoder, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR); + reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_CCIR);
- reg_clear(encoder, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ); - reg_clear(encoder, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE); - reg_write(encoder, REG_SERIALIZER, 0); - reg_write(encoder, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0)); + reg_clear(priv, REG_PLL_SERIAL_1, PLL_SERIAL_1_SRL_MAN_IZ); + reg_clear(priv, REG_PLL_SERIAL_3, PLL_SERIAL_3_SRL_DE); + reg_write(priv, REG_SERIALIZER, 0); + reg_write(priv, REG_HVF_CNTRL_1, HVF_CNTRL_1_VQR(0));
/* TODO enable pixel repeat for pixel rates less than 25Msamp/s */ rep = 0; - reg_write(encoder, REG_RPT_CNTRL, 0); - reg_write(encoder, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) | + reg_write(priv, REG_RPT_CNTRL, 0); + reg_write(priv, REG_SEL_CLK, SEL_CLK_SEL_VRF_CLK(0) | SEL_CLK_SEL_CLK1 | SEL_CLK_ENA_SC_CLK);
- reg_write(encoder, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) | + reg_write(priv, REG_PLL_SERIAL_2, PLL_SERIAL_2_SRL_NOSC(div) | PLL_SERIAL_2_SRL_PR(rep));
/* set color matrix bypass flag: */ - reg_set(encoder, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP); + reg_set(priv, REG_MAT_CONTRL, MAT_CONTRL_MAT_BP);
/* set BIAS tmds value: */ - reg_write(encoder, REG_ANA_GENERAL, 0x09); + reg_write(priv, REG_ANA_GENERAL, 0x09);
- reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD); + reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_MTHD);
/* * Sync on rising HSYNC/VSYNC */ - reg_write(encoder, REG_VIP_CNTRL_3, 0); - reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS); + reg_write(priv, REG_VIP_CNTRL_3, 0); + reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_SYNC_HS);
/* * TDA19988 requires high-active sync at input stage, * so invert low-active sync provided by master encoder here */ if (mode->flags & DRM_MODE_FLAG_NHSYNC) - reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL); + reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_H_TGL); if (mode->flags & DRM_MODE_FLAG_NVSYNC) - reg_set(encoder, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL); + reg_set(priv, REG_VIP_CNTRL_3, VIP_CNTRL_3_V_TGL);
/* * Always generate sync polarity relative to input sync and @@ -887,49 +886,49 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder, reg |= TBG_CNTRL_1_H_TGL; if (mode->flags & DRM_MODE_FLAG_NVSYNC) reg |= TBG_CNTRL_1_V_TGL; - reg_write(encoder, REG_TBG_CNTRL_1, reg); - - reg_write(encoder, REG_VIDFORMAT, 0x00); - reg_write16(encoder, REG_REFPIX_MSB, ref_pix); - reg_write16(encoder, REG_REFLINE_MSB, ref_line); - reg_write16(encoder, REG_NPIX_MSB, n_pix); - reg_write16(encoder, REG_NLINE_MSB, n_line); - reg_write16(encoder, REG_VS_LINE_STRT_1_MSB, vs1_line_s); - reg_write16(encoder, REG_VS_PIX_STRT_1_MSB, vs1_pix_s); - reg_write16(encoder, REG_VS_LINE_END_1_MSB, vs1_line_e); - reg_write16(encoder, REG_VS_PIX_END_1_MSB, vs1_pix_e); - reg_write16(encoder, REG_VS_LINE_STRT_2_MSB, vs2_line_s); - reg_write16(encoder, REG_VS_PIX_STRT_2_MSB, vs2_pix_s); - reg_write16(encoder, REG_VS_LINE_END_2_MSB, vs2_line_e); - reg_write16(encoder, REG_VS_PIX_END_2_MSB, vs2_pix_e); - reg_write16(encoder, REG_HS_PIX_START_MSB, hs_pix_s); - reg_write16(encoder, REG_HS_PIX_STOP_MSB, hs_pix_e); - reg_write16(encoder, REG_VWIN_START_1_MSB, vwin1_line_s); - reg_write16(encoder, REG_VWIN_END_1_MSB, vwin1_line_e); - reg_write16(encoder, REG_VWIN_START_2_MSB, vwin2_line_s); - reg_write16(encoder, REG_VWIN_END_2_MSB, vwin2_line_e); - reg_write16(encoder, REG_DE_START_MSB, de_pix_s); - reg_write16(encoder, REG_DE_STOP_MSB, de_pix_e); + reg_write(priv, REG_TBG_CNTRL_1, reg); + + reg_write(priv, REG_VIDFORMAT, 0x00); + reg_write16(priv, REG_REFPIX_MSB, ref_pix); + reg_write16(priv, REG_REFLINE_MSB, ref_line); + reg_write16(priv, REG_NPIX_MSB, n_pix); + reg_write16(priv, REG_NLINE_MSB, n_line); + reg_write16(priv, REG_VS_LINE_STRT_1_MSB, vs1_line_s); + reg_write16(priv, REG_VS_PIX_STRT_1_MSB, vs1_pix_s); + reg_write16(priv, REG_VS_LINE_END_1_MSB, vs1_line_e); + reg_write16(priv, REG_VS_PIX_END_1_MSB, vs1_pix_e); + reg_write16(priv, REG_VS_LINE_STRT_2_MSB, vs2_line_s); + reg_write16(priv, REG_VS_PIX_STRT_2_MSB, vs2_pix_s); + reg_write16(priv, REG_VS_LINE_END_2_MSB, vs2_line_e); + reg_write16(priv, REG_VS_PIX_END_2_MSB, vs2_pix_e); + reg_write16(priv, REG_HS_PIX_START_MSB, hs_pix_s); + reg_write16(priv, REG_HS_PIX_STOP_MSB, hs_pix_e); + reg_write16(priv, REG_VWIN_START_1_MSB, vwin1_line_s); + reg_write16(priv, REG_VWIN_END_1_MSB, vwin1_line_e); + reg_write16(priv, REG_VWIN_START_2_MSB, vwin2_line_s); + reg_write16(priv, REG_VWIN_END_2_MSB, vwin2_line_e); + reg_write16(priv, REG_DE_START_MSB, de_pix_s); + reg_write16(priv, REG_DE_STOP_MSB, de_pix_e);
if (priv->rev == TDA19988) { /* let incoming pixels fill the active space (if any) */ - reg_write(encoder, REG_ENABLE_SPACE, 0x01); + reg_write(priv, REG_ENABLE_SPACE, 0x01); }
/* must be last register set: */ - reg_clear(encoder, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE); + reg_clear(priv, REG_TBG_CNTRL_0, TBG_CNTRL_0_SYNC_ONCE);
/* Only setup the info frames if the sink is HDMI */ if (priv->is_hdmi_sink) { /* We need to turn HDMI HDCP stuff on to get audio through */ - reg_clear(encoder, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); - reg_write(encoder, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1)); - reg_set(encoder, REG_TX33, TX33_HDMI); + reg_clear(priv, REG_TBG_CNTRL_1, TBG_CNTRL_1_DWIN_DIS); + reg_write(priv, REG_ENC_CNTRL, ENC_CNTRL_CTL_CODE(1)); + reg_set(priv, REG_TX33, TX33_HDMI);
- tda998x_write_avi(encoder, adjusted_mode); + tda998x_write_avi(priv, adjusted_mode);
if (priv->params.audio_cfg) - tda998x_configure_audio(encoder, adjusted_mode, + tda998x_configure_audio(priv, adjusted_mode, &priv->params); } } @@ -938,7 +937,9 @@ 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); + struct tda998x_priv *priv = to_tda998x_priv(encoder); + uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV); + return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected : connector_status_disconnected; } @@ -946,29 +947,30 @@ tda998x_encoder_detect(struct drm_encoder *encoder, static int read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk) { + struct tda998x_priv *priv = to_tda998x_priv(encoder); uint8_t offset, segptr; int ret, i;
/* enable EDID read irq: */ - reg_set(encoder, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); + reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
offset = (blk & 1) ? 128 : 0; segptr = blk / 2;
- reg_write(encoder, REG_DDC_ADDR, 0xa0); - reg_write(encoder, REG_DDC_OFFS, offset); - reg_write(encoder, REG_DDC_SEGM_ADDR, 0x60); - reg_write(encoder, REG_DDC_SEGM, segptr); + reg_write(priv, REG_DDC_ADDR, 0xa0); + reg_write(priv, REG_DDC_OFFS, offset); + reg_write(priv, REG_DDC_SEGM_ADDR, 0x60); + reg_write(priv, REG_DDC_SEGM, segptr);
/* enable reading EDID: */ - reg_write(encoder, REG_EDID_CTRL, 0x1); + reg_write(priv, REG_EDID_CTRL, 0x1);
/* flag must be cleared by sw: */ - reg_write(encoder, REG_EDID_CTRL, 0x0); + reg_write(priv, REG_EDID_CTRL, 0x0);
/* wait for block read to complete: */ for (i = 100; i > 0; i--) { - uint8_t val = reg_read(encoder, REG_INT_FLAGS_2); + uint8_t val = reg_read(priv, REG_INT_FLAGS_2); if (val & INT_FLAGS_2_EDID_BLK_RD) break; msleep(1); @@ -977,14 +979,14 @@ read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk) if (i == 0) return -ETIMEDOUT;
- ret = reg_read_range(encoder, REG_EDID_DATA_0, buf, EDID_LENGTH); + ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH); if (ret != EDID_LENGTH) { dev_err(encoder->dev->dev, "failed to read edid block %d: %d", blk, ret); return ret; }
- reg_clear(encoder, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD); + reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
return 0; } @@ -1001,7 +1003,7 @@ do_get_edid(struct drm_encoder *encoder) return NULL;
if (priv->rev == TDA19988) - reg_clear(encoder, REG_TX4, TX4_PD_RAM); + reg_clear(priv, REG_TX4, TX4_PD_RAM);
/* base block fetch */ if (read_edid_block(encoder, block, 0)) @@ -1041,13 +1043,13 @@ do_get_edid(struct drm_encoder *encoder)
done: if (priv->rev == TDA19988) - reg_set(encoder, REG_TX4, TX4_PD_RAM); + reg_set(priv, REG_TX4, TX4_PD_RAM);
return block;
fail: if (priv->rev == TDA19988) - reg_set(encoder, REG_TX4, TX4_PD_RAM); + reg_set(priv, REG_TX4, TX4_PD_RAM); dev_warn(encoder->dev->dev, "failed to read EDID\n"); kfree(block); return NULL; @@ -1131,7 +1133,6 @@ 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); @@ -1143,6 +1144,7 @@ tda998x_encoder_init(struct i2c_client *client, priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
priv->current_page = 0; + priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34); priv->dpms = DRM_MODE_DPMS_OFF;
@@ -1150,14 +1152,14 @@ tda998x_encoder_init(struct i2c_client *client, encoder_slave->slave_funcs = &tda998x_encoder_funcs;
/* wake up the device: */ - cec_write(encoder, REG_CEC_ENAMODS, + cec_write(priv, REG_CEC_ENAMODS, CEC_ENAMODS_EN_RXSENS | CEC_ENAMODS_EN_HDMI);
- tda998x_reset(encoder); + tda998x_reset(priv);
/* read version: */ - priv->rev = reg_read(encoder, REG_VERSION_LSB) | - reg_read(encoder, REG_VERSION_MSB) << 8; + priv->rev = reg_read(priv, REG_VERSION_LSB) | + reg_read(priv, REG_VERSION_MSB) << 8;
/* mask off feature bits: */ priv->rev &= ~0x30; /* not-hdcp and not-scalar bit */ @@ -1173,16 +1175,16 @@ tda998x_encoder_init(struct i2c_client *client, }
/* after reset, enable DDC: */ - reg_write(encoder, REG_DDC_DISABLE, 0x00); + reg_write(priv, REG_DDC_DISABLE, 0x00);
/* set clock on DDC channel: */ - reg_write(encoder, REG_TX3, 39); + reg_write(priv, REG_TX3, 39);
/* if necessary, disable multi-master: */ if (priv->rev == TDA19989) - reg_set(encoder, REG_I2C_MASTER, I2C_MASTER_DIS_MM); + reg_set(priv, REG_I2C_MASTER, I2C_MASTER_DIS_MM);
- cec_write(encoder, REG_CEC_FRO_IM_CLK_CTRL, + cec_write(priv, REG_CEC_FRO_IM_CLK_CTRL, CEC_FRO_IM_CLK_CTRL_GHOST_DIS | CEC_FRO_IM_CLK_CTRL_IMCLK_SEL);
return 0;
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote:
This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write
- code cleanup and fix some small errors
- use global constants
- don't read write-only registers
- add DT support
- use IRQ for connection status and EDID read
I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate.
I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.)
On Sun, Feb 02, 2014 at 12:43:58PM +0000, Russell King - ARM Linux wrote:
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote:
This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write
- code cleanup and fix some small errors
- use global constants
- don't read write-only registers
- add DT support
- use IRQ for connection status and EDID read
I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate.
I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.)
Okay, out of your patches, I would like to queue up the following for submission into -rc kernels:
drm/i2c: tda998x: fix bad value in the AIF drm/i2c: tda998x: check the CEC device creation drm/i2c: tda998x: free the CEC device on encoder_destroy drm/i2c: tda998x: force the page register at startup time drm/i2c: tda998x: set the PLL division factor in range 0..3 drm/i2c: tda998x: fix the ENABLE_SPACE register
since these fix real bugs. Moving them to the front of the queue isn't that big a deal (I've done it here in my git tree - yes, there were a few conflicts, but nothing serious.)
I'll also consider these for -rc too:
drm/i2c: tda998x: use HDMI constants drm/i2c: tda998x: add the active aspect in HDMI AVI frame drm/i2c: tda998x: change the frequence in the audio channel
if we have reports of display devices affected by the info frame errors.
As far as the last summary line goes, I'll change it to:
"Use ALSA IEC958 definitions and update audio frequency"
Note that in general, bug fixes should always be towards the beginning of a patch series, so that they can be applied independently of the remainder of the development, which also makes them easier to backport to stable kernels if that's deemed necessary.
As for the remainder, I think moving the DT documentation patch immediately after the addition of DT code (or even just before it) is good form.
So, in summary, I'm pretty happy with this again - and it's all been tested here with no apparant detrimental effects. All committed and queued up here:
http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=tda998x-devel
If you want to pull it back to check:
git://ftp.arm.linux.org.uk/~rmk/linux-cubox.git tda998x-devel
I'm proposing to send everything up to the tda998x-fixes tag next week for 3.13-rc kernels.
Rob, do you want to provide any acks for this or are you happy?
On Sun, 2 Feb 2014 18:04:34 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
So, in summary, I'm pretty happy with this again - and it's all been tested here with no apparant detrimental effects. All committed and queued up here:
http://ftp.arm.linux.org.uk/cgit/linux-cubox.git/log/?h=tda998x-devel
If you want to pull it back to check:
git://ftp.arm.linux.org.uk/~rmk/linux-cubox.git tda998x-devel
I'm proposing to send everything up to the tda998x-fixes tag next week for 3.13-rc kernels.
Everything is OK. Thank you.
On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote:
This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write
- code cleanup and fix some small errors
- use global constants
- don't read write-only registers
- add DT support
- use IRQ for connection status and EDID read
I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate.
I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.)
Thanks.
BTW, I found some problems with the patch 12 'add DT support' you already acked:
- the .of_match_table is not needed because the i2c client is created by the i2c subsystem from the 'reg' in the DT,
- on encoder_destroy(), the function drm_i2c_encoder_destroy() unregisters the i2c client, so, with a DT, a second encoder_init() would crash.
Do you better like a new patch or a fix?
On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote:
On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote:
This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write
- code cleanup and fix some small errors
- use global constants
- don't read write-only registers
- add DT support
- use IRQ for connection status and EDID read
I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate.
I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.)
Thanks.
BTW, I found some problems with the patch 12 'add DT support' you already acked:
- the .of_match_table is not needed because the i2c client is created by the i2c subsystem from the 'reg' in the DT,
Okay - may it be a good idea to have someone knowledgable of I2C give it a review?
- on encoder_destroy(), the function drm_i2c_encoder_destroy() unregisters the i2c client, so, with a DT, a second encoder_init() would crash.
I think this is one of the down-sides of trying to bolt DT into this: the drm encoder slave support is not designed to cope with an i2c client device pre-created.
In fact, I can't see how this stuff comes anywhere close to working in a DT setup: in such a scenario, you declare that there's a tda998x device in DT. I2C parses this, and creates an i2c_client itself for the tda998x.
When the TDA998x driver initialises, it finds this i2c client and binds to it, calling tda998x_probe(), which does nothing.
However, the only way to attach a slave encoder to a DRM device is via a call to drm_i2c_encoder_init(), which unconditionally calls i2c_new_device(). This creates a _new_ i2c_client structure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it's encoder_init function.
None of this will happen if DT has already created an i2c_client at the appropriate address, because DRMs i2c_new_device() will fail.
My hypothesis is that you have other patches to I2C and/or DRM to sort this out which you haven't been posting with this series. So, could you please provide some hints as to how this works?
On 02/02/2014 07:23 PM, Russell King - ARM Linux wrote:
On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote:
- on encoder_destroy(), the function drm_i2c_encoder_destroy() unregisters the i2c client, so, with a DT, a second encoder_init() would crash.
I think this is one of the down-sides of trying to bolt DT into this: the drm encoder slave support is not designed to cope with an i2c client device pre-created.
In fact, I can't see how this stuff comes anywhere close to working in a DT setup: in such a scenario, you declare that there's a tda998x device in DT. I2C parses this, and creates an i2c_client itself for the tda998x.
When the TDA998x driver initialises, it finds this i2c client and binds to it, calling tda998x_probe(), which does nothing.
However, the only way to attach a slave encoder to a DRM device is via a call to drm_i2c_encoder_init(), which unconditionally calls i2c_new_device(). This creates a _new_ i2c_client structure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it's encoder_init function.
None of this will happen if DT has already created an i2c_client at the appropriate address, because DRMs i2c_new_device() will fail.
drm_i2c_encoder_init() could look at .of_node of the i2c_board_info. If it is there, do not try to i2c_new_device as it has already been registered by DT i2c auto-probing.
Sebastian
On Sun, 2 Feb 2014 18:23:49 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote:
On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
On Wed, Jan 29, 2014 at 10:01:22AM +0100, Jean-Francois Moine wrote:
This patch set contains various extensions to the tda998x driver:
- simplify the i2c read/write
- code cleanup and fix some small errors
- use global constants
- don't read write-only registers
- add DT support
- use IRQ for connection status and EDID read
I discussed these patches with Rob Clark recently, and the conclusion we came to is that I'll merge them into a git tree, test them, and once I'm happy I'll send a pull request as appropriate.
I'll go through them later today. Those patches which have been re- posted without any change for the last few times (the first few) I'll take into my git tree today so you don't have to keep re-posting them (more importantly, I won't have to keep on looking at them either.)
Thanks.
BTW, I found some problems with the patch 12 'add DT support' you already acked:
- the .of_match_table is not needed because the i2c client is created by the i2c subsystem from the 'reg' in the DT,
Okay - may it be a good idea to have someone knowledgable of I2C give it a review?
Sure! There is still a lot of magic in the DT.
I used the tda998x in the DT for a long time without .of_match_table and it loaded correctly. I added it in the patch just because my other modules had it.
A few days ago, when I moved the tda998x audio codec description in the DT as a subnode of the tda998x i2c, the codec module was not loaded. An of_platform_populate() of the subnodes was needed in the tda998x i2c driver. I could not find why...
- on encoder_destroy(), the function drm_i2c_encoder_destroy() unregisters the i2c client, so, with a DT, a second encoder_init() would crash.
I think this is one of the down-sides of trying to bolt DT into this: the drm encoder slave support is not designed to cope with an i2c client device pre-created.
In fact, I can't see how this stuff comes anywhere close to working in a DT setup: in such a scenario, you declare that there's a tda998x device in DT. I2C parses this, and creates an i2c_client itself for the tda998x.
When the TDA998x driver initialises, it finds this i2c client and binds to it, calling tda998x_probe(), which does nothing.
However, the only way to attach a slave encoder to a DRM device is via a call to drm_i2c_encoder_init(), which unconditionally calls i2c_new_device(). This creates a _new_ i2c_client structure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it's encoder_init function.
None of this will happen if DT has already created an i2c_client at the appropriate address, because DRMs i2c_new_device() will fail.
My hypothesis is that you have other patches to I2C and/or DRM to sort this out which you haven't been posting with this series. So, could you please provide some hints as to how this works?
I explained how to use the tda998x in a DT context in a message to Jyri Sarha:
http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html
On Sun, Feb 02, 2014 at 07:54:00PM +0100, Jean-Francois Moine wrote:
I explained how to use the tda998x in a DT context in a message to Jyri Sarha:
http://lists.freedesktop.org/archives/dri-devel/2014-January/052936.html
Okay, so there's a bunch of changes required to the DRM slave support which aren't in place yet.
In which case, it may be better to reorder the remaining patches such that the DT changes are at the very end - which means we can still benefit from the rest of the patches if the DT solution remains an open question.
We do have another option now that my generic component support is in mainline (merged during this window), which should result in a much cleaner solution. If we convert TDA998x to a component, armada DRM to a component master (and same for other tda998x users) then we don't need the drm_encoder_slave stuff - all that goes away since it's no longer necessary.
We also solve this problem as well - because we're then not messing around with working out if there's a DT node present: the TDA998x device must pre-exist. For non-DT setups, this can be done when the I2C bus is created - devices on it would be created using the standard mechanisms already present via the i2c_board_data array. For DT setups, the devices are created by parsing the I2C bus node in DT.
Both cases result in a component being registered upon invocation of tda998x_probe(), and removal of the component when tda998x_remove() is called. The tda998x driver becomes a standard I2C driver.
This is something I've been intending to look at now that the component stuff is in place - as I said previously when the questions around DT and Armada DRM were first posed, we need to solve these issues in a generic way first, rather than hacking around them.
On Sun, 2 Feb 2014 19:15:05 +0000 Russell King - ARM Linux linux@arm.linux.org.uk wrote:
In which case, it may be better to reorder the remaining patches such that the DT changes are at the very end - which means we can still benefit from the rest of the patches if the DT solution remains an open question.
We do have another option now that my generic component support is in mainline (merged during this window), which should result in a much cleaner solution. If we convert TDA998x to a component, armada DRM to a component master (and same for other tda998x users) then we don't need the drm_encoder_slave stuff - all that goes away since it's no longer necessary.
We also solve this problem as well - because we're then not messing around with working out if there's a DT node present: the TDA998x device must pre-exist. For non-DT setups, this can be done when the I2C bus is created - devices on it would be created using the standard mechanisms already present via the i2c_board_data array. For DT setups, the devices are created by parsing the I2C bus node in DT.
Both cases result in a component being registered upon invocation of tda998x_probe(), and removal of the component when tda998x_remove() is called. The tda998x driver becomes a standard I2C driver.
This is something I've been intending to look at now that the component stuff is in place - as I said previously when the questions around DT and Armada DRM were first posed, we need to solve these issues in a generic way first, rather than hacking around them.
Agree. I was looking in the same direction...
On Sun, Feb 02, 2014 at 07:06:06PM +0100, Jean-Francois Moine wrote:
- the .of_match_table is not needed because the i2c client is created by the i2c subsystem from the 'reg' in the DT,
It's generally better to have an explict set of OF IDs even if the default does work - matching purely on the device name does work almost all the time but there are collisions out there with different manufacturers using the same prefix for their chips (the example I always trot out is that both Wolfson and Wondermedia use "wm").
dri-devel@lists.freedesktop.org
-
Jean-Francois Moine -
Joe Perches -
Mark Brown -
Russell King - ARM Linux -
Sebastian Hesselbarth