Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)
Ping? On Mon, Sep 02, 2013 at 03:50:57PM +0100, Russell King - ARM Linux wrote: > On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote: > > From: Russell King > > > > This patch adds tda998x specific parameters to allow it to be configured > > for different boards using it. Also, this implements rudimentary audio > > support for S/PDIF attached controllers. > > > > Signed-off-by: Russell King > > Signed-off-by: Sebastian Hesselbarth > > Tested-by: Darren Etheridge > > --- > > It looks like there's been a bug introduced in this patch (which wasn't > in my original). > > > @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int > > mode) > > > > switch (mode) { > > case DRM_MODE_DPMS_ON: > > - /* enable audio and video ports */ > > - reg_write(encoder, REG_ENA_AP, 0xff); > > + /* 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); > > I also disabled the writing to REG_ENA_AP in the DPMS off path as well, > which clears this register. > > That seems to be missing from this patch, and it means that when the > display is placed into DPMS-off mode, the audio inputs are disabled, > never to be re-enabled. There is no need to disable the audio input > in DPMS-off mode. > > 8<= > From: Russell King > Subject: [PATCH] drm/i2c: Fix broken TDA998x audio > > In patch "drm/i2c: tda998x: add video and audio input configuration" in > its original version, disabling the audio input port was removed. The > version which was submitted for merging had this change deleted, which > results in audio being non-functional. Fix this by removing the write. > While here, update the comment too. > > Signed-off-by: Russell King > --- > drivers/gpu/drm/i2c/tda998x_drv.c |3 +-- > 1 files changed, 1 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c > index 5742cfc..59878af 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c > @@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int > mode) > reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2); > break; > case DRM_MODE_DPMS_OFF: > - /* disable audio and video ports */ > - reg_write(encoder, REG_ENA_AP, 0x00); > + /* 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); ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)
On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote: > From: Russell King > > This patch adds tda998x specific parameters to allow it to be configured > for different boards using it. Also, this implements rudimentary audio > support for S/PDIF attached controllers. > > Signed-off-by: Russell King > Signed-off-by: Sebastian Hesselbarth > Tested-by: Darren Etheridge > --- It looks like there's been a bug introduced in this patch (which wasn't in my original). > @@ -445,8 +681,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int > mode) > > switch (mode) { > case DRM_MODE_DPMS_ON: > - /* enable audio and video ports */ > - reg_write(encoder, REG_ENA_AP, 0xff); > + /* 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); I also disabled the writing to REG_ENA_AP in the DPMS off path as well, which clears this register. That seems to be missing from this patch, and it means that when the display is placed into DPMS-off mode, the audio inputs are disabled, never to be re-enabled. There is no need to disable the audio input in DPMS-off mode. 8<= From: Russell King Subject: [PATCH] drm/i2c: Fix broken TDA998x audio In patch "drm/i2c: tda998x: add video and audio input configuration" in its original version, disabling the audio input port was removed. The version which was submitted for merging had this change deleted, which results in audio being non-functional. Fix this by removing the write. While here, update the comment too. Signed-off-by: Russell King --- drivers/gpu/drm/i2c/tda998x_drv.c |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 5742cfc..59878af 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -705,8 +705,7 @@ tda998x_encoder_dpms(struct drm_encoder *encoder, int mode) reg_write(encoder, REG_VIP_CNTRL_2, priv->vip_cntrl_2); break; case DRM_MODE_DPMS_OFF: - /* disable audio and video ports */ - reg_write(encoder, REG_ENA_AP, 0x00); + /* 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); ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Wed, Aug 21, 2013 at 08:33:55PM +0200, Jean-Francois Moine wrote: > On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote: > > +static void > > +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int > > cnt) > > +{ > > + struct i2c_client *client = drm_i2c_encoder_get_client(encoder); > > + uint8_t buf[cnt+1]; > > + int ret; > > + > > + buf[0] = REG2ADDR(reg); > > + memcpy(&buf[1], p, cnt); > > + > > + set_page(encoder, reg); > > + > > + ret = i2c_master_send(client, buf, cnt + 1); > > + if (ret < 0) > > + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); > > +} > > + > > It seems simpler to reserve one byte in the caller buffer for the > register address and avoid a memcpy. And by doing so create an obtuse API. No thanks. > > +static void > > +tda998x_write_aif(struct drm_encoder *encoder, struct > > tda998x_encoder_params *p) > > +{ > > + uint8_t buf[PB(5) + 1]; > > + > > + buf[HB(0)] = 0x84; > > + buf[HB(1)] = 0x01; > > + buf[HB(2)] = 10; > > Why don't you use the constants which are defined in hdmi.h? Because I was not aware of them. > > + /* Write the CTS and N values */ > > + buf[0] = 0x44; > > + buf[1] = 0x42; > > + buf[2] = 0x01; > > The CTS value is strange, but that does not matter: its generation is > automatic (see above). Correct - however not strange, if you've read the HDMI specification. > > + buf[3] = n; > > + buf[4] = n >> 8; > > + buf[5] = n >> 16; > > + reg_write_range(encoder, REG_ACR_CTS_0, buf, 6); > > + > > + /* Set CTS clock reference */ > > + reg_write(encoder, 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); > > + > > + /* 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); > [snip] > > From what I understood in the NXP driver, buf[3] depends on the sample > rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz. Correct, but the driver has no way to know this. The above reflects how the NXP driver on the Cubox implements this (which we know works for multiple different sample rates). This is because we use SPDIF, which encodes the sample rate in the channel status information, which is then presumably passed through by the TDA998x. I wouldn't know, I don't have a HDMI debugger. What I do know is that it works for multiple sample rates.
Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Wed, Aug 21, 2013 at 08:33:55PM +0200, Jean-Francois Moine wrote: > On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote: > > +static void > > +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int > > cnt) > > +{ > > + struct i2c_client *client = drm_i2c_encoder_get_client(encoder); > > + uint8_t buf[cnt+1]; > > + int ret; > > + > > + buf[0] = REG2ADDR(reg); > > + memcpy(&buf[1], p, cnt); > > + > > + set_page(encoder, reg); > > + > > + ret = i2c_master_send(client, buf, cnt + 1); > > + if (ret < 0) > > + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); > > +} > > + > > It seems simpler to reserve one byte in the caller buffer for the > register address and avoid a memcpy. And by doing so create an obtuse API. No thanks. > > +static void > > +tda998x_write_aif(struct drm_encoder *encoder, struct > > tda998x_encoder_params *p) > > +{ > > + uint8_t buf[PB(5) + 1]; > > + > > + buf[HB(0)] = 0x84; > > + buf[HB(1)] = 0x01; > > + buf[HB(2)] = 10; > > Why don't you use the constants which are defined in hdmi.h? Because I was not aware of them. > > + /* Write the CTS and N values */ > > + buf[0] = 0x44; > > + buf[1] = 0x42; > > + buf[2] = 0x01; > > The CTS value is strange, but that does not matter: its generation is > automatic (see above). Correct - however not strange, if you've read the HDMI specification. > > + buf[3] = n; > > + buf[4] = n >> 8; > > + buf[5] = n >> 16; > > + reg_write_range(encoder, REG_ACR_CTS_0, buf, 6); > > + > > + /* Set CTS clock reference */ > > + reg_write(encoder, 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); > > + > > + /* 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); > [snip] > > From what I understood in the NXP driver, buf[3] depends on the sample > rate: 0xf1 for 44.1kHz and 0xd1 for 48kHz. Correct, but the driver has no way to know this. The above reflects how the NXP driver on the Cubox implements this (which we know works for multiple different sample rates). This is because we use SPDIF, which encodes the sample rate in the channel status information, which is then presumably passed through by the TDA998x. I wouldn't know, I don't have a HDMI debugger. What I do know is that it works for multiple sample rates. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote: > From: Russell King > > This patch adds tda998x specific parameters to allow it to be configured > for different boards using it. Also, this implements rudimentary audio > support for S/PDIF attached controllers. It seems that this patch mixes two different functions: - configuration - audio About configuration, why don't you use the standard way to set the device parameters, i.e. board info and DT? > Signed-off-by: Russell King > Signed-off-by: Sebastian Hesselbarth > Tested-by: Darren Etheridge > --- > Changelog: > for v1: > - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz > - also calculate CTS > v1->v2: > - Remove CTS calculation as it isn't used in current TDA998x setup > (Reported by Russell King) > - Remove debug left-over (Reported by Russell King) > - Use correct tda998x include (Reported by Russell King) > > Cc: David Airlie > Cc: Darren Etheridge > Cc: Rob Clark > Cc: Russell King > Cc: Daniel Vetter > Cc: dri-devel at lists.freedesktop.org > Cc: linux-kernel at vger.kernel.org > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 268 > +++-- > include/drm/i2c/tda998x.h | 30 + > 2 files changed, 290 insertions(+), 8 deletions(-) > create mode 100644 include/drm/i2c/tda998x.h > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c > index 527d11b..2b64dfa 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c [snip] > @@ -344,6 +390,23 @@ fail: > return ret; > } > > +static void > +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int > cnt) > +{ > + struct i2c_client *client = drm_i2c_encoder_get_client(encoder); > + uint8_t buf[cnt+1]; > + int ret; > + > + buf[0] = REG2ADDR(reg); > + memcpy(&buf[1], p, cnt); > + > + set_page(encoder, reg); > + > + ret = i2c_master_send(client, buf, cnt + 1); > + if (ret < 0) > + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); > +} > + It seems simpler to reserve one byte in the caller buffer for the register address and avoid a memcpy. > static uint8_t > reg_read(struct drm_encoder *encoder, uint16_t reg) > { > @@ -412,7 +475,7 @@ tda998x_reset(struct drm_encoder *encoder) > 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,0x03); > + 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); > @@ -424,11 +487,184 @@ tda998x_reset(struct drm_encoder *encoder) > reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24); > } > > +static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) > +{ > + uint8_t sum = 0; > + > + while (bytes--) > + sum += *buf++; > + return (255 - sum) + 1; > +} Simpler: static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) { int sum = 0;/* avoid byte computation */ while (bytes--) sum -= *buf++; /* avoid a substraction */ return sum; } > + > +#define HB(x) (x) > +#define PB(x) (HB(2) + 1 + (x)) > + > +static void > +tda998x_write_if(struct drm_encoder *encoder, 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); > +} > + > +static void > +tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params > *p) > +{ > + uint8_t buf[PB(5) + 1]; > + > + buf[HB(0)] = 0x84; > + buf[HB(1)] = 0x01; > + buf[HB(2)] = 10; Why don't you use the constants which are defined in hdmi.h? > + 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]; > + buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */ > + > + tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, > + sizeof(buf)); > +} > + > +static void > +tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode) > +{ > + uint8_t buf[PB(13) + 1]; > + > + memset(buf, 0, sizeof(buf)); > + buf[HB(0)] = 0x82; > + buf[HB(1)] = 0x02; > + buf[HB(2)] = 13; > + buf[PB(4)] = drm_match_cea_mode(mode); > + > + tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf, > + sizeof(buf)); > +} > + > +static void tda998x_audio_mute(struct drm_encoder *encoder, bool on) > +{ > + if (on) { > + reg_
Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote: > From: Russell King > > This patch adds tda998x specific parameters to allow it to be configured > for different boards using it. Also, this implements rudimentary audio > support for S/PDIF attached controllers. It seems that this patch mixes two different functions: - configuration - audio About configuration, why don't you use the standard way to set the device parameters, i.e. board info and DT? > Signed-off-by: Russell King > Signed-off-by: Sebastian Hesselbarth > Tested-by: Darren Etheridge > --- > Changelog: > for v1: > - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz > - also calculate CTS > v1->v2: > - Remove CTS calculation as it isn't used in current TDA998x setup > (Reported by Russell King) > - Remove debug left-over (Reported by Russell King) > - Use correct tda998x include (Reported by Russell King) > > Cc: David Airlie > Cc: Darren Etheridge > Cc: Rob Clark > Cc: Russell King > Cc: Daniel Vetter > Cc: dri-devel at lists.freedesktop.org > Cc: linux-kernel at vger.kernel.org > --- > drivers/gpu/drm/i2c/tda998x_drv.c | 268 > +++-- > include/drm/i2c/tda998x.h | 30 + > 2 files changed, 290 insertions(+), 8 deletions(-) > create mode 100644 include/drm/i2c/tda998x.h > > diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c > b/drivers/gpu/drm/i2c/tda998x_drv.c > index 527d11b..2b64dfa 100644 > --- a/drivers/gpu/drm/i2c/tda998x_drv.c > +++ b/drivers/gpu/drm/i2c/tda998x_drv.c [snip] > @@ -344,6 +390,23 @@ fail: > return ret; > } > > +static void > +reg_write_range(struct drm_encoder *encoder, uint16_t reg, uint8_t *p, int > cnt) > +{ > + struct i2c_client *client = drm_i2c_encoder_get_client(encoder); > + uint8_t buf[cnt+1]; > + int ret; > + > + buf[0] = REG2ADDR(reg); > + memcpy(&buf[1], p, cnt); > + > + set_page(encoder, reg); > + > + ret = i2c_master_send(client, buf, cnt + 1); > + if (ret < 0) > + dev_err(&client->dev, "Error %d writing to 0x%x\n", ret, reg); > +} > + It seems simpler to reserve one byte in the caller buffer for the register address and avoid a memcpy. > static uint8_t > reg_read(struct drm_encoder *encoder, uint16_t reg) > { > @@ -412,7 +475,7 @@ tda998x_reset(struct drm_encoder *encoder) > 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,0x03); > + 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); > @@ -424,11 +487,184 @@ tda998x_reset(struct drm_encoder *encoder) > reg_write(encoder, REG_MUX_VP_VIP_OUT, 0x24); > } > > +static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) > +{ > + uint8_t sum = 0; > + > + while (bytes--) > + sum += *buf++; > + return (255 - sum) + 1; > +} Simpler: static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes) { int sum = 0;/* avoid byte computation */ while (bytes--) sum -= *buf++; /* avoid a substraction */ return sum; } > + > +#define HB(x) (x) > +#define PB(x) (HB(2) + 1 + (x)) > + > +static void > +tda998x_write_if(struct drm_encoder *encoder, 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); > +} > + > +static void > +tda998x_write_aif(struct drm_encoder *encoder, struct tda998x_encoder_params > *p) > +{ > + uint8_t buf[PB(5) + 1]; > + > + buf[HB(0)] = 0x84; > + buf[HB(1)] = 0x01; > + buf[HB(2)] = 10; Why don't you use the constants which are defined in hdmi.h? > + 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]; > + buf[PB(5)] = p->audio_frame[5] & 0xf8; /* DM_INH + LSV */ > + > + tda998x_write_if(encoder, DIP_IF_FLAGS_IF4, REG_IF4_HB0, buf, > + sizeof(buf)); > +} > + > +static void > +tda998x_write_avi(struct drm_encoder *encoder, struct drm_display_mode *mode) > +{ > + uint8_t buf[PB(13) + 1]; > + > + memset(buf, 0, sizeof(buf)); > + buf[HB(0)] = 0x82; > + buf[HB(1)] = 0x02; > + buf[HB(2)] = 13; > + buf[PB(4)] = drm_match_cea_mode(mode); > + > + tda998x_write_if(encoder, DIP_IF_FLAGS_IF2, REG_IF2_HB0, buf, > + sizeof(buf)); > +} > + > +static void tda998x_audio_mute(struct drm_encoder *encoder, bool on) > +{ > + if (on) { > + reg_
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth wrote: > From: Russell King > > This patch adds tda998x specific parameters to allow it to be configured > for different boards using it. Also, this implements rudimentary audio > support for S/PDIF attached controllers. > > Signed-off-by: Russell King > Signed-off-by: Sebastian Hesselbarth > Tested-by: Darren Etheridge > --- I've merged the series, this one generates a warning though: CC [M] drivers/gpu/drm/i2c/tda998x_drv.o /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c: In function ?tda998x_encoder_mode_set?: /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: warning: ?clksel_fs? may be used uninitialized in this function [-Wmaybe-uninitialized] /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30: note: ?clksel_fs? was declared here /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: warning: ?clksel_aip? may be used uninitialized in this function [-Wmaybe-uninitialized] /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18: note: ?clksel_aip? was declared here It doesn't seem like a real problem, since the function is unlikely to be called any way to make that case happen. Dave.
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Mon, Aug 19, 2013 at 09:23:17AM +1000, Dave Airlie wrote: > On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth > wrote: > > From: Russell King > > > > This patch adds tda998x specific parameters to allow it to be configured > > for different boards using it. Also, this implements rudimentary audio > > support for S/PDIF attached controllers. > > > > Signed-off-by: Russell King > > Signed-off-by: Sebastian Hesselbarth > > Tested-by: Darren Etheridge > > --- > > I've merged the series, Thanks. > this one generates a warning though: > CC [M] drivers/gpu/drm/i2c/tda998x_drv.o > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c: > In function ?tda998x_encoder_mode_set?: > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: > warning: ?clksel_fs? may be used uninitialized in this function > [-Wmaybe-uninitialized] > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30: > note: ?clksel_fs? was declared here > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: > warning: ?clksel_aip? may be used uninitialized in this function > [-Wmaybe-uninitialized] > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18: > note: ?clksel_aip? was declared here > > It doesn't seem like a real problem, since the function is unlikely to > be called any way to make that case happen. Ok, I'll squash those warnings by a slight rearrangement of the code.
Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Mon, Aug 19, 2013 at 09:23:17AM +1000, Dave Airlie wrote: > On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth > wrote: > > From: Russell King > > > > This patch adds tda998x specific parameters to allow it to be configured > > for different boards using it. Also, this implements rudimentary audio > > support for S/PDIF attached controllers. > > > > Signed-off-by: Russell King > > Signed-off-by: Sebastian Hesselbarth > > Tested-by: Darren Etheridge > > --- > > I've merged the series, Thanks. > this one generates a warning though: > CC [M] drivers/gpu/drm/i2c/tda998x_drv.o > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c: > In function ‘tda998x_encoder_mode_set’: > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: > warning: ‘clksel_fs’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30: > note: ‘clksel_fs’ was declared here > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: > warning: ‘clksel_aip’ may be used uninitialized in this function > [-Wmaybe-uninitialized] > /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18: > note: ‘clksel_aip’ was declared here > > It doesn't seem like a real problem, since the function is unlikely to > be called any way to make that case happen. Ok, I'll squash those warnings by a slight rearrangement of the code. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth wrote: > From: Russell King > > This patch adds tda998x specific parameters to allow it to be configured > for different boards using it. Also, this implements rudimentary audio > support for S/PDIF attached controllers. > > Signed-off-by: Russell King > Signed-off-by: Sebastian Hesselbarth > Tested-by: Darren Etheridge > --- I've merged the series, this one generates a warning though: CC [M] drivers/gpu/drm/i2c/tda998x_drv.o /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c: In function ‘tda998x_encoder_mode_set’: /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: warning: ‘clksel_fs’ may be used uninitialized in this function [-Wmaybe-uninitialized] /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:30: note: ‘clksel_fs’ was declared here /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:637:11: warning: ‘clksel_aip’ may be used uninitialized in this function [-Wmaybe-uninitialized] /home/airlied/devel/kernel/drm-next/drivers/gpu/drm/i2c/tda998x_drv.c:573:18: note: ‘clksel_aip’ was declared here It doesn't seem like a real problem, since the function is unlikely to be called any way to make that case happen. Dave. ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
From: Russell King This patch adds tda998x specific parameters to allow it to be configured for different boards using it. Also, this implements rudimentary audio support for S/PDIF attached controllers. Signed-off-by: Russell King Signed-off-by: Sebastian Hesselbarth Tested-by: Darren Etheridge --- Changelog: for v1: - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz - also calculate CTS v1->v2: - Remove CTS calculation as it isn't used in current TDA998x setup (Reported by Russell King) - Remove debug left-over (Reported by Russell King) - Use correct tda998x include (Reported by Russell King) Cc: David Airlie Cc: Darren Etheridge Cc: Rob Clark Cc: Russell King Cc: Daniel Vetter Cc: dri-devel at lists.freedesktop.org Cc: linux-kernel at vger.kernel.org --- drivers/gpu/drm/i2c/tda998x_drv.c | 268 +++-- include/drm/i2c/tda998x.h | 30 + 2 files changed, 290 insertions(+), 8 deletions(-) create mode 100644 include/drm/i2c/tda998x.h diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 527d11b..2b64dfa 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -23,7 +23,7 @@ #include #include #include - +#include #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) @@ -32,9 +32,11 @@ struct tda998x_priv { uint16_t rev; uint8_t current_page; int dpms; + bool is_hdmi_sink; u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; + struct tda998x_encoder_params params; }; #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -71,10 +73,13 @@ struct tda998x_priv { # define I2C_MASTER_DIS_MM(1 << 0) # define I2C_MASTER_DIS_FILT (1 << 1) # define I2C_MASTER_APP_STRT_LAT (1 << 2) +#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */ +# define FEAT_POWERDOWN_SPDIF (1 << 3) #define REG_INT_FLAGS_0 REG(0x00, 0x0f) /* read/write */ #define REG_INT_FLAGS_1 REG(0x00, 0x10) /* read/write */ #define REG_INT_FLAGS_2 REG(0x00, 0x11) /* read/write */ # define INT_FLAGS_2_EDID_BLK_RD (1 << 1) +#define REG_ENA_ACLK REG(0x00, 0x16) /* read/write */ #define REG_ENA_VP_0 REG(0x00, 0x18) /* read/write */ #define REG_ENA_VP_1 REG(0x00, 0x19) /* read/write */ #define REG_ENA_VP_2 REG(0x00, 0x1a) /* read/write */ @@ -113,6 +118,7 @@ struct tda998x_priv { #define REG_VIP_CNTRL_5 REG(0x00, 0x25) /* write */ # define VIP_CNTRL_5_CKCASE (1 << 0) # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1) +#define REG_MUX_APREG(0x00, 0x26) /* read/write */ #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */ #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */ # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0) @@ -175,6 +181,12 @@ struct tda998x_priv { # define HVF_CNTRL_1_PAD(x) (((x) & 3) << 4) # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ +#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */ +# define I2S_FORMAT(x)(((x) & 3) << 0) +#define REG_AIP_CLKSELREG(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) /* Page 02h: PLL settings */ @@ -198,6 +210,12 @@ struct tda998x_priv { #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */ #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */ #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */ +# define AUDIO_DIV_SERCLK_1 0 +# define AUDIO_DIV_SERCLK_2 1 +# define AUDIO_DIV_SERCLK_4 2 +# define AUDIO_DIV_SERCLK_8 3 +# define AUDIO_DIV_SERCLK_16 4 +# define AUDIO_DIV_SERCLK_32 5 #define REG_SEL_CLK REG(0x02, 0x11) /* read/write */ # define SEL_CLK_SEL_CLK1 (1 << 0) # define SEL_CLK_SEL_VRF_CLK(x) (((x) & 3) << 1) @@ -216,6 +234,11 @@ struct tda998x_priv { /* Page 10h: information frames and packets */ +#define REG_IF1_HB0 REG(0x10, 0x20) /* read/write */ +#define REG_IF2_HB0 REG(0x10, 0x40) /* read/write */ +#define REG_IF3_HB0 REG(0x10, 0x60) /* read/write */ +#define REG_IF4_HB0 REG(0x10, 0x80) /* read/write */ +#define REG_IF5_HB0 REG(0x10, 0xa0) /* read/write */ /* Page 11h: audio settings and content info packets */ @@ -225,10 +248,33 @@ struct tda998x_priv { # define AIP_CNTRL_0_LAYOUT (1 << 2) # define AIP_CNTRL_0_ACR_MAN (1 << 5) # define AIP_CNTRL_0_RST_CTS (1 << 6) +#define REG_CA_I2SREG(0x11, 0x01) /* read/write */ +# define CA_I2S
[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration
From: Russell King This patch adds tda998x specific parameters to allow it to be configured for different boards using it. Also, this implements rudimentary audio support for S/PDIF attached controllers. Signed-off-by: Russell King Signed-off-by: Sebastian Hesselbarth Tested-by: Darren Etheridge --- Changelog: for v1: - set AUDIO_DIV to SERCLK/16 for modes with pixclk >100MHz - also calculate CTS v1->v2: - Remove CTS calculation as it isn't used in current TDA998x setup (Reported by Russell King) - Remove debug left-over (Reported by Russell King) - Use correct tda998x include (Reported by Russell King) Cc: David Airlie Cc: Darren Etheridge Cc: Rob Clark Cc: Russell King Cc: Daniel Vetter Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/i2c/tda998x_drv.c | 268 +++-- include/drm/i2c/tda998x.h | 30 + 2 files changed, 290 insertions(+), 8 deletions(-) create mode 100644 include/drm/i2c/tda998x.h diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 527d11b..2b64dfa 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -23,7 +23,7 @@ #include #include #include - +#include #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__) @@ -32,9 +32,11 @@ struct tda998x_priv { uint16_t rev; uint8_t current_page; int dpms; + bool is_hdmi_sink; u8 vip_cntrl_0; u8 vip_cntrl_1; u8 vip_cntrl_2; + struct tda998x_encoder_params params; }; #define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -71,10 +73,13 @@ struct tda998x_priv { # define I2C_MASTER_DIS_MM(1 << 0) # define I2C_MASTER_DIS_FILT (1 << 1) # define I2C_MASTER_APP_STRT_LAT (1 << 2) +#define REG_FEAT_POWERDOWNREG(0x00, 0x0e) /* read/write */ +# define FEAT_POWERDOWN_SPDIF (1 << 3) #define REG_INT_FLAGS_0 REG(0x00, 0x0f) /* read/write */ #define REG_INT_FLAGS_1 REG(0x00, 0x10) /* read/write */ #define REG_INT_FLAGS_2 REG(0x00, 0x11) /* read/write */ # define INT_FLAGS_2_EDID_BLK_RD (1 << 1) +#define REG_ENA_ACLK REG(0x00, 0x16) /* read/write */ #define REG_ENA_VP_0 REG(0x00, 0x18) /* read/write */ #define REG_ENA_VP_1 REG(0x00, 0x19) /* read/write */ #define REG_ENA_VP_2 REG(0x00, 0x1a) /* read/write */ @@ -113,6 +118,7 @@ struct tda998x_priv { #define REG_VIP_CNTRL_5 REG(0x00, 0x25) /* write */ # define VIP_CNTRL_5_CKCASE (1 << 0) # define VIP_CNTRL_5_SP_CNT(x)(((x) & 3) << 1) +#define REG_MUX_APREG(0x00, 0x26) /* read/write */ #define REG_MUX_VP_VIP_OUTREG(0x00, 0x27) /* read/write */ #define REG_MAT_CONTRLREG(0x00, 0x80) /* write */ # define MAT_CONTRL_MAT_SC(x) (((x) & 3) << 0) @@ -175,6 +181,12 @@ struct tda998x_priv { # define HVF_CNTRL_1_PAD(x) (((x) & 3) << 4) # define HVF_CNTRL_1_SEMI_PLANAR (1 << 6) #define REG_RPT_CNTRL REG(0x00, 0xf0) /* write */ +#define REG_I2S_FORMATREG(0x00, 0xfc) /* read/write */ +# define I2S_FORMAT(x)(((x) & 3) << 0) +#define REG_AIP_CLKSELREG(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) /* Page 02h: PLL settings */ @@ -198,6 +210,12 @@ struct tda998x_priv { #define REG_PLL_SCGR1 REG(0x02, 0x09) /* read/write */ #define REG_PLL_SCGR2 REG(0x02, 0x0a) /* read/write */ #define REG_AUDIO_DIV REG(0x02, 0x0e) /* read/write */ +# define AUDIO_DIV_SERCLK_1 0 +# define AUDIO_DIV_SERCLK_2 1 +# define AUDIO_DIV_SERCLK_4 2 +# define AUDIO_DIV_SERCLK_8 3 +# define AUDIO_DIV_SERCLK_16 4 +# define AUDIO_DIV_SERCLK_32 5 #define REG_SEL_CLK REG(0x02, 0x11) /* read/write */ # define SEL_CLK_SEL_CLK1 (1 << 0) # define SEL_CLK_SEL_VRF_CLK(x) (((x) & 3) << 1) @@ -216,6 +234,11 @@ struct tda998x_priv { /* Page 10h: information frames and packets */ +#define REG_IF1_HB0 REG(0x10, 0x20) /* read/write */ +#define REG_IF2_HB0 REG(0x10, 0x40) /* read/write */ +#define REG_IF3_HB0 REG(0x10, 0x60) /* read/write */ +#define REG_IF4_HB0 REG(0x10, 0x80) /* read/write */ +#define REG_IF5_HB0 REG(0x10, 0xa0) /* read/write */ /* Page 11h: audio settings and content info packets */ @@ -225,10 +248,33 @@ struct tda998x_priv { # define AIP_CNTRL_0_LAYOUT (1 << 2) # define AIP_CNTRL_0_ACR_MAN (1 << 5) # define AIP_CNTRL_0_RST_CTS (1 << 6) +#define REG_CA_I2SREG(0x11, 0x01) /* read/write */ +# define CA_