[PATCH v5 00/23]
Subsystems (3)
v5 cover [PATCH v5 00/23]
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
Signature · 1 line
1.9.rc1
PATCH v5 1/23 drm/i2c: tda998x: simplify the i2c read/write functions
This patch simplifies the i2c read/write functions and permits them to be easily called in more contexts.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 322 +++++++++++++++++++------------------- 1 file changed, 162 insertions(+), 160 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +162 -160
@@ -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 0x0301static 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 ;
Signature · 1 line
1.9.rc1
PATCH v5 2/23 drm/i2c: tda998x: check more I/O errors
In reply to Jean-Francois MoineThis 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 57 +++++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 14 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +43 -14
@@ -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 */
Signature · 1 line
1.9.rc1
Re: [PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
Quoted 3 lines · Jean-Francois Moine
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:
drivers/gpu/drm/i2c/tda998x_drv.c reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt) · line 437
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +6
-2-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.
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_init(struct i2c_client *client, · line 1191
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +5
-2@@ -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.
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Re: [PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
Quoted 67 lines · On Sun, 2 Feb 2014 16:20:58 +0000 Russell King - ARM Linux…
Quoted 3 lines · Jean-Francois Moine
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:
drivers/gpu/drm/i2c/tda998x_drv.c reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt) · line 437
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +6
-2-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.
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_init(struct i2c_client *client, · line 1191
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +5
-2@@ -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 ifreg_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;
Quoted 3 lines
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.
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors
Quoted 70 lines · Jean-Francois Moine
Quoted 67 lines · On Sun, 2 Feb 2014 16:20:58 +0000 Russell King - ARM Linux…
Quoted 3 lines · Jean-Francois Moine
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:
drivers/gpu/drm/i2c/tda998x_drv.c reg_write_range(struct tda998x_priv *priv, uint16_t reg, uint8_t *p, int cnt) · line 437
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +6
-2-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.
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_init(struct i2c_client *client, · line 1191
Quoted 11 lines
drivers/gpu/drm/i2c/tda998x_drv.c +5
-2@@ -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 ifreg_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;
Quoted 8 lines
Quoted 3 lines
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.
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
PATCH v5 3/23 drm/i2c: tda998x: code cleanup
In reply to Jean-Francois MoineThis 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +7 -6
@@ -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 ;
Signature · 1 line
1.9.rc1
PATCH v5 4/23 drm/i2c: tda998x: change probe message origin
In reply to Jean-Francois MoineOn 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
--- drivers/gpu/drm/i2c/tda998x_drv.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +14 -5
@@ -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; }
Signature · 1 line
1.9.rc1
PATCH v5 5/23 drm/i2c: tda998x: don't freeze the system at audio startup time
In reply to Jean-Francois MoineThis patch prevents the system to be freezed at audio startup time, replacing mdelay by msleep.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -1
@@ -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 */
Signature · 1 line
1.9.rc1
PATCH v5 6/23 drm/i2c: tda998x: force the page register at startup time
In reply to Jean-Francois MoineThis patch forces the page register to be set on the first I/O operation.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -1
@@ -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 ;
Signature · 1 line
1.9.rc1
PATCH v5 7/23 drm/i2c: tda998x: fix bad value in the AIF
In reply to Jean-Francois MoineThe AIF has an uninitialized byte. This patch clears the whole buffer before filling it.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -1
@@ -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 ];
Signature · 1 line
1.9.rc1
PATCH v5 8/23 drm/i2c: tda998x: use HDMI constants
In reply to Jean-Francois MoineThis patch replaces hard coded values by hdmi constants.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +6 -6
@@ -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);
Signature · 1 line
1.9.rc1
PATCH v5 9/23 drm/i2c: tda998x: don't read write-only registers
In reply to Jean-Francois MoineThis 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 46 ++++++++++++++++++++------------------- 1 file changed, 24 insertions(+), 22 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +24 -22
@@ -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 );
Signature · 1 line
1.9.rc1
Re: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
Quoted 8 lines · Jean-Francois Moine
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET,
TBG_CNTRL_0andTBG_CNTRL_1have all bits cleared after reset, so, they may be fully re-written.The register
MAT_CONTRLis 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:
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_mode_set(struct drm_encoder *encoder, · line 944
Quoted 3 lines
/* must be last register set: */drivers/gpu/drm/i2c/tda998x_drv.c +1
-1-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.
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Re: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
Quoted 16 lines · On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux…
Quoted 8 lines · Jean-Francois Moine
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET,
TBG_CNTRL_0andTBG_CNTRL_1have all bits cleared after reset, so, they may be fully re-written.The register
MAT_CONTRLis 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:
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_mode_set(struct drm_encoder *encoder, · line 944
Quoted 3 lines
/* must be last register set: */drivers/gpu/drm/i2c/tda998x_drv.c +1
-1-reg_clear (priv,REG_TBG_CNTRL_0 ,TBG_CNTRL_0_SYNC_ONCE ); +reg_write (priv,REG_TBG_CNTRL_0 ,0 );
Quoted 8 lines
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.
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 09/23] drm/i2c: tda998x: don't read write-only registers
Quoted 28 lines · Jean-Francois Moine
Quoted 16 lines · On Sun, 2 Feb 2014 16:23:09 +0000 Russell King - ARM Linux…
Quoted 8 lines · Jean-Francois Moine
This patch takes care of the write-only registers of the tda998x.
The registers SOFTRESET,
TBG_CNTRL_0andTBG_CNTRL_1have all bits cleared after reset, so, they may be fully re-written.The register
MAT_CONTRLis 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:
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_mode_set(struct drm_encoder *encoder, · line 944
Quoted 3 lines
/* must be last register set: */drivers/gpu/drm/i2c/tda998x_drv.c +1
-1-reg_clear (priv,REG_TBG_CNTRL_0 ,TBG_CNTRL_0_SYNC_ONCE ); +reg_write (priv,REG_TBG_CNTRL_0 ,0 );Quoted 8 lines
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.
Quoted 6 lines
According to the tda9983b documentation, the register
TBG_CNTRL_0is 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 thereg_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.
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
PATCH v5 10/23 drm/i2c: tda998x: free the CEC device on encoder_destroy
In reply to Jean-Francois MoineThe cec i2c device is created in tda998x_encoder_init() when the DRM driver starts.
This patch frees it when the DRM driver is unloaded.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/i2c/tda998x_drv.c +2 -0
@@ -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); }
Signature · 1 line
1.9.rc1
PATCH v5 11/23 drm/i2c: tda998x: check the CEC device creation
In reply to Jean-Francois MoineThis patch checks if the CEC device is well created at intialization time.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/i2c/tda998x_drv.c +2 -0
@@ -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;
Signature · 1 line
1.9.rc1
PATCH v5 12/23 drm/i2c: tda998x: add DT support
In reply to Jean-Francois MoineThis patch adds DT support to the tda998x.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
drivers/gpu/drm/i2c/tda998x_drv.c +22 -0
@@ -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, },
Signature · 1 line
1.9.rc1
PATCH v5 13/23 drm/i2c: tda998x: always enable EDID read IRQ
In reply to Jean-Francois MoineThere is no need to enable/disable EDID read IRQ at each EDID block read. This patch enables the IRQ at init time.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +3 -5
@@ -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 */
Signature · 1 line
1.9.rc1
PATCH v5 14/23 drm/i2c: tda998x: use irq for connection status and EDID read
In reply to Jean-Francois MoineThis 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 113 +++++++++++++++++++++++++++++++++++--- 1 file changed, 104 insertions(+), 9 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +104 -9
@@ -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 );
Signature · 1 line
1.9.rc1
PATCH v5 15/23 drm/i2c: tda998x: add DT documentation
In reply to Jean-Francois Moine--- .../devicetree/bindings/drm/i2c/tda998x.txt | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 Documentation/devicetree/bindings/drm/i2c/tda998x.txt
Documentation/devicetree/bindings/drm/i2c/tda998x.txt new file +27 -0
@@ -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"; + };
Signature · 1 line
1.9.rc1
PATCH v5 16/23 drm/i2c: tda998x: fix the ENABLE_SPACE register
In reply to Jean-Francois MoineThis patch fixes the ENABLE_SPACE register, the value of which was inverted.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -1
@@ -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 ); }/*
Signature · 1 line
1.9.rc1
PATCH v5 17/23 drm/i2c: tda998x: set the PLL division factor in range 0..3
In reply to Jean-Francois MoineThe 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/i2c/tda998x_drv.c +6 -1
@@ -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 );
Signature · 1 line
1.9.rc1
Re: [PATCH v5 17/23] drm/i2c: tda998x: set the PLL division factor in range 0..3
Quoted 2 lines · Jean-Francois Moine
The predivider division factor of the register
PLL_SERIAL_2is in the range 0..3, the value 0 being used for a division by 1.
trivia:
drivers/gpu/drm/i2c/tda998x_drv.c
Quoted 1 line
drivers/gpu/drm/i2c/tda998x_drv.c +0
-0
[]
drivers/gpu/drm/i2c/tda998x_drv.c tda998x_encoder_mode_set(struct drm_encoder *encoder, · line 893
Quoted 9 lines
drivers/gpu/drm/i2c/tda998x_drv.c +5
-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 ; + }
perhaps
clamp(div, 1, 4) div--;
PATCH v5 18/23 drm/i2c: tda998x: make the audio code more readable
In reply to Jean-Francois MoineThis 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.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +13 -12
@@ -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 ;
Signature · 1 line
1.9.rc1
PATCH v5 19/23 drm/i2c: tda998x: remove the unused variable ca_i2s
In reply to Jean-Francois Moineca_i2s is only ever written to, but never read, so let's get rid of it.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -3
@@ -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 :
Signature · 1 line
1.9.rc1
PATCH v5 20/23 drm/i2c: tda998x: add the active aspect in HDMI AVI frame
In reply to Jean-Francois MoineThe picture aspect setting was zero, which is reserved. A setting of Same As Picture makes more sense.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 1 + 1 file changed, 1 insertion(+)
drivers/gpu/drm/i2c/tda998x_drv.c +1 -0
@@ -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);
Signature · 1 line
1.9.rc1
PATCH v5 21/23 drm/i2c: tda998x: change the frequence in the audio channel
In reply to Jean-Francois MoineThis patch sets the frequence as 'not indicated' instead of '48kHz' and uses the asound values in the channel status definition.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +5 -3
@@ -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);
Signature · 1 line
1.9.rc1
PATCH v5 22/23 drm/i2c: tda998x: code optimization
In reply to Jean-Francois MoineThis patch reduces the number of I2C exchanges by setting many bits in one write and removing a useless write.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +4 -8
@@ -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 */
Signature · 1 line
1.9.rc1
PATCH v5 23/23 drm/i2c: tda998x: adjust the audio clock divider for S/PDIF
In reply to Jean-Francois MoineAccording to some tests on the Cubox (Marvell Armada 510 + TDA19988), the S/PDIF input asks for a greater audio clock divider.
--- drivers/gpu/drm/i2c/tda998x_drv.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/gpu/drm/i2c/tda998x_drv.c +7 -3
@@ -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);/*
Signature · 1 line
1.9.rc1
Quoted 8 lines · Jean-Francois Moine
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.)
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Quoted 10 lines · Russell King - ARM Linux
Quoted 8 lines · Jean-Francois Moine
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
Quoted 8 lines
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?
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Quoted 4 lines · On Sun, 2 Feb 2014 18:04:34 +0000 Russell King - ARM Linux…
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:
Quoted 8 lines
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.gittda998x-develI'm proposing to send everything up to the tda998x-fixes tag next week for 3.13-rc kernels.
Everything is OK. Thank you.
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 10 lines · On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux…
Quoted 8 lines · Jean-Francois Moine
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
Quoted 8 lines
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?
Signature · 2 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/
Quoted 22 lines · Jean-Francois Moine
Quoted 10 lines · On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux…
Quoted 8 lines · Jean-Francois Moine
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 readQuoted 8 lines
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.)
Quoted 7 lines
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?
Quoted 3 lines
- on
encoder_destroy(), the functiondrm_i2c_encoder_destroy()unregisters the i2c client, so, with a DT, a secondencoder_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?
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Quoted 24 lines · Russell King - ARM Linux
Quoted 3 lines · Jean-Francois Moine
- on
encoder_destroy(), the functiondrm_i2c_encoder_destroy()unregisters the i2c client, so, with a DT, a secondencoder_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_clientitself 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 callsi2c_new_device(). This creates a _new_i2c_clientstructure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it'sencoder_initfunction.
Quoted 2 lines
None of this will happen if DT has already created an
i2c_clientat the appropriate address, because DRMsi2c_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
Quoted 25 lines · On Sun, 2 Feb 2014 18:23:49 +0000 Russell King - ARM Linux…
Quoted 24 lines · Jean-Francois Moine
Quoted 10 lines · On Sun, 2 Feb 2014 12:43:58 +0000 Russell King - ARM Linux…
Quoted 8 lines · Jean-Francois Moine
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 readQuoted 8 lines
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.
Quoted 8 lines
Quoted 5 lines
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...
Quoted 23 lines
Quoted 3 lines
- on
encoder_destroy(), the functiondrm_i2c_encoder_destroy()unregisters the i2c client, so, with a DT, a secondencoder_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_clientitself 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 callsi2c_new_device(). This creates a _new_i2c_clientstructure, again unconditionally, for the tda998x. This must be bound by the I2C subsystem to a driver - hopefully the tda998x driver, which then calls it'sencoder_initfunction.
Quoted 6 lines
None of this will happen if DT has already created an
i2c_clientat the appropriate address, because DRMsi2c_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
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 4 lines · Jean-Francois Moine
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.
Signature · 3 lines
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".
Quoted 20 lines · On Sun, 2 Feb 2014 19:15:05 +0000 Russell King - ARM Linux…
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_slavestuff - 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_dataarray. For DT setups, the devices are created by parsing the I2C bus node in DT.
Quoted 8 lines
Both cases result in a component being registered upon invocation of
tda998x_probe(), and removal of the component whentda998x_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...
Signature · 6 lines
Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ _______________________________________________ dri-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 2 lines · Jean-Francois Moine
- 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").