subject:"\[PATCH v2 5\/8\] drm\/i2c\: tda998x\: add video and audio input configuration"

Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)

2013-09-23 Thread Russell King - ARM Linux

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 rmk+ker...@arm.linux.org.uk
  
  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 rmk+ker...@arm.linux.org.uk
  Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
  Tested-by: Darren Etheridge detheri...@ti.com
  ---
 
 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 rmk+ker...@arm.linux.org.uk
 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 rmk+ker...@arm.linux.org.uk
 --- 
  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)

2013-09-02 Thread Russell King - ARM Linux

On Wed, Aug 14, 2013 at 09:43:30PM +0200, Sebastian Hesselbarth wrote:
 From: Russell King rmk+ker...@arm.linux.org.uk
 
 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 rmk+ker...@arm.linux.org.uk
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
 Tested-by: Darren Etheridge detheri...@ti.com
 ---

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 rmk+ker...@arm.linux.org.uk
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 rmk+ker...@arm.linux.org.uk
--- 
 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

2013-08-22 Thread Russell King - ARM Linux

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([1], p, cnt);
> > +
> > +   set_page(encoder, reg);
> > +
> > +   ret = i2c_master_send(client, buf, cnt + 1);
> > +   if (ret < 0)
> > +   dev_err(>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.

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-21 Thread Jean-Francois Moine

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([1], p, cnt);
> +
> + set_page(encoder, reg);
> +
> + ret = i2c_master_send(client, buf, cnt + 1);
> + if (ret < 0)
> + dev_err(>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

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-21 Thread Jean-Francois Moine

On Wed Aug 14 12:43:30 PDT 2013, Sebastian Hesselbarth wrote:
 From: Russell King rmk+kernel at arm.linux.org.uk
 
 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 rmk+kernel at arm.linux.org.uk
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselbarth at gmail.com
 Tested-by: Darren Etheridge detheridge at ti.com
 ---
 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 airlied at linux.ie
 Cc: Darren Etheridge detheridge at ti.com
 Cc: Rob Clark robdclark at gmail.com
 Cc: Russell King rmk+kernel at arm.linux.org.uk
 Cc: Daniel Vetter daniel.vetter at ffwll.ch
 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

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-21 Thread Russell King - ARM Linux

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

2013-08-19 Thread Russell King - ARM Linux

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

2013-08-19 Thread Russell King - ARM Linux

On Mon, Aug 19, 2013 at 09:23:17AM +1000, Dave Airlie wrote:
 On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth
 sebastian.hesselba...@gmail.com wrote:
  From: Russell King rmk+ker...@arm.linux.org.uk
 
  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 rmk+ker...@arm.linux.org.uk
  Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
  Tested-by: Darren Etheridge detheri...@ti.com
  ---
 
 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

2013-08-18 Thread Dave Airlie

On Thu, Aug 15, 2013 at 5:43 AM, Sebastian Hesselbarth
sebastian.hesselba...@gmail.com wrote:
 From: Russell King rmk+ker...@arm.linux.org.uk

 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 rmk+ker...@arm.linux.org.uk
 Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
 Tested-by: Darren Etheridge detheri...@ti.com
 ---

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

2013-08-14 Thread Sebastian Hesselbarth

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

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

2013-08-14 Thread Sebastian Hesselbarth

From: Russell King rmk+ker...@arm.linux.org.uk

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 rmk+ker...@arm.linux.org.uk
Signed-off-by: Sebastian Hesselbarth sebastian.hesselba...@gmail.com
Tested-by: Darren Etheridge detheri...@ti.com
---
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 airl...@linux.ie
Cc: Darren Etheridge detheri...@ti.com
Cc: Rob Clark robdcl...@gmail.com
Cc: Russell King rmk+ker...@arm.linux.org.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
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 drm/drm_crtc_helper.h
 #include drm/drm_encoder_slave.h
 #include drm/drm_edid.h
-
+#include drm/i2c/tda998x.h
 
 #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 @@

Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)

Re: [PATCH] drm/i2c: Fix broken TDA998x audio (was: Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration)

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

Re: [PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

[PATCH v2 5/8] drm/i2c: tda998x: add video and audio input configuration

11 matches

Site Navigation

Mail list logo

Footer information