[PATCH v2 0/12] Those patches is used for dw_hdmi audio support

[PATCH v2 0/12] Those patches is used for dw_hdmi audio support

[Yakir Yang]

We found Designware hdmi driver only support audio clock config, we can not play sound through it.
To add Designware HDMI Audio support, we make those patch set:
 1): modify n/cts config order, according to dw_hdmi document.
 2): add Audio Sample Channel Status config interfaces to dw_hdmi driver.
 3): add audio support for more display resolutions(eg. 800x600).
 4): add audio support for No-CEA display resolutions.
 5): fixed dw_hdmi irq bug, add irq control to suspend/resume interfaces.
 6): add suspend/resume callback for dw_hdmi rockchip driver.
 7): filter interlace mode in rockchip vop driver.
 8): add hdmi audio config interfaces to dw_hdmi driver.
 9): creat "dw_hdmi-audio" platform device in dw_hdmi driver.
10): add codec driver for hdmi audio, callback dw_hdmi audio config functions.
11): add sound driver for hdmi audio, creat hdmi audio sound card.
12): add dt-bings file and add hdmi_audio node to corresponding dt file.


Changes in v2:
- adjust n/cts setting order
- Add audio sample channel status setting
- Add irq control to suspend/resume interfaces
- Add suspend/resume support for dw_hdmi_rockchip driver
- filter interlace display mode for rockchip vop
- add more n/cts combinations for more display resolutions
- enable audio support for No-CEA display mode
- Add audio config interfaces to dw_hdmi driver
- Update the audio control interfaces
- Update dw_hdmi audio control interfaces, and adjust jack report process
- give "codec-name" & "codec-dai-name" an const name
- remove codec-name and codec-dai-name
- rename rockchip,rockchip-hdmi-audio.txt to rockchip,rockchip-dw-hdmi-audio.txt

Yakir Yang (12):
  drm: bridge/dw_hdmi: adjust n/cts setting order
  drm: bridge/dw_hdmi: add audio sample channel status setting
  drm: bridge/dw_hdmi: add irq control to suspend/resume
  drm: rockchip/dw_hdmi_rockchip: add resume/suspend support
  drm: rockchip/vop: filter interlace display mode
  drm: bridge/dw_hdmi: add audio support for more display resolutions
  drm: bridge/dw_hdmi: enable audio support for No-CEA display
    resolutions
  drm: bridge/dw_hdmi: add audio config interfaces
  drm: bridge/dw_hdmi: creat dw-hdmi-audio platform device
  ASoC: dw-hdmi-audio: add codec driver for dw hdmi audio
  ASoC: rockchip-hdmi-audio: add sound driver for hdmi audio
  dt-bindings: Add documentation for Rockchip dw-hdmi-audio

 .../sound/rockchip,rockchip-dw-hdmi-audio.txt      |  12 +
 drivers/gpu/drm/bridge/dw_hdmi.c                   | 276 ++++++++++++++++--
 drivers/gpu/drm/bridge/dw_hdmi.h                   |  32 +++
 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c        |  13 +
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c        |   3 +-
 include/drm/bridge/dw_hdmi.h                       |  45 +++
 sound/soc/codecs/Kconfig                           |   4 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/dw-hdmi-audio.c                   | 314 +++++++++++++++++++++
 sound/soc/codecs/dw-hdmi-audio.h                   |  17 ++
 sound/soc/rockchip/Kconfig                         |   9 +
 sound/soc/rockchip/Makefile                        |   2 +
 sound/soc/rockchip/rockchip_hdmi_audio.c           | 196 +++++++++++++
 13 files changed, 895 insertions(+), 30 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
 create mode 100644 sound/soc/codecs/dw-hdmi-audio.c
 create mode 100644 sound/soc/codecs/dw-hdmi-audio.h
 create mode 100644 sound/soc/rockchip/rockchip_hdmi_audio.c

-- 
2.1.2

[PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order

[Yakir Yang]

For Designerware HDMI, the following write sequence is recommended:
1. aud_n3 (set bit ncts_atomic_write if desired)
2. aud_cts3 (set CTS_manual and CTS value if desired/enabled)
3. aud_cts2 (required in CTS_manual)
4. aud_cts1 (required in CTS_manual)
5. aud_n3 (bit ncts_atomic_write with same value as in step 1.)
6. aud_n2
7. aud_n1

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- adjust n/cts setting order

 drivers/gpu/drm/bridge/dw_hdmi.c | 37 +++++++++++++++++++++----------------
 drivers/gpu/drm/bridge/dw_hdmi.h |  6 ++++++
 2 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index cd6a706..423addc 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -177,26 +177,31 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
 	hdmi_modb(hdmi, data << shift, mask, reg);
 }
 
-static void hdmi_set_clock_regenerator_n(struct dw_hdmi *hdmi,
-					 unsigned int value)
+static void hdmi_regenerate_n_cts(struct dw_hdmi *hdmi, unsigned int n,
+				  unsigned int cts)
 {
-	hdmi_writeb(hdmi, value & 0xff, HDMI_AUD_N1);
-	hdmi_writeb(hdmi, (value >> 8) & 0xff, HDMI_AUD_N2);
-	hdmi_writeb(hdmi, (value >> 16) & 0x0f, HDMI_AUD_N3);
+	/* set ncts_atomic_write */
+	hdmi_modb(hdmi, HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET,
+		  HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK, HDMI_AUD_N3);
+
+	/* set cts manual */
+	hdmi_modb(hdmi, HDMI_AUD_CTS3_CTS_MANUAL,
+		  HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
 
 	/* nshift factor = 0 */
 	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_N_SHIFT_MASK, HDMI_AUD_CTS3);
-}
-
-static void hdmi_regenerate_cts(struct dw_hdmi *hdmi, unsigned int cts)
-{
-	/* Must be set/cleared first */
-	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
 
-	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
+	/* set cts values */
+	hdmi_modb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK),
+		  HDMI_AUD_CTS3_AUDCTS19_16_MASK, HDMI_AUD_CTS3);
 	hdmi_writeb(hdmi, (cts >> 8) & 0xff, HDMI_AUD_CTS2);
-	hdmi_writeb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK) |
-		    HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
+	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
+
+	/* set n values */
+	hdmi_modb(hdmi, (n >> 16) & HDMI_AUD_N3_AUDN19_16_MASK,
+		  HDMI_AUD_N3_AUDN19_16_MASK, HDMI_AUD_N3);
+	hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2);
+	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
 }
 
 static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
@@ -355,8 +360,8 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 		__func__, hdmi->sample_rate, hdmi->ratio,
 		pixel_clk, clk_n, clk_cts);
 
-	hdmi_set_clock_regenerator_n(hdmi, clk_n);
-	hdmi_regenerate_cts(hdmi, clk_cts);
+	hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts);
+	hdmi_set_schnl(hdmi);
 }
 
 static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi)
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 175dbc8..bc53452 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -884,6 +884,12 @@ enum {
 	HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL = 0x08,
 	HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_MASK = 0x04,
 
+/* AUD_N3 field values */
+	HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK = 0x80,
+	HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET = 0x80,
+	HDMI_AUD_N3_NCTS_ATOMIC_WRITE_CLEAR = 0x00,
+	HDMI_AUD_N3_AUDN19_16_MASK = 0x0f,
+
 /* AUD_CTS3 field values */
 	HDMI_AUD_CTS3_N_SHIFT_OFFSET = 5,
 	HDMI_AUD_CTS3_N_SHIFT_MASK = 0xe0,
-- 
2.1.2

Re: [PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:25:30AM -0500, Yakir Yang wrote:
> For Designerware HDMI, the following write sequence is recommended:
> 1. aud_n3 (set bit ncts_atomic_write if desired)
> 2. aud_cts3 (set CTS_manual and CTS value if desired/enabled)
> 3. aud_cts2 (required in CTS_manual)
> 4. aud_cts1 (required in CTS_manual)
> 5. aud_n3 (bit ncts_atomic_write with same value as in step 1.)
> 6. aud_n2
> 7. aud_n1
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v2:
> - adjust n/cts setting order
> 
>  drivers/gpu/drm/bridge/dw_hdmi.c | 37 +++++++++++++++++++++----------------
>  drivers/gpu/drm/bridge/dw_hdmi.h |  6 ++++++
>  2 files changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
> index cd6a706..423addc 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -177,26 +177,31 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>  	hdmi_modb(hdmi, data << shift, mask, reg);
>  }
>  
> -static void hdmi_set_clock_regenerator_n(struct dw_hdmi *hdmi,
> -					 unsigned int value)
> +static void hdmi_regenerate_n_cts(struct dw_hdmi *hdmi, unsigned int n,
> +				  unsigned int cts)
>  {
> -	hdmi_writeb(hdmi, value & 0xff, HDMI_AUD_N1);
> -	hdmi_writeb(hdmi, (value >> 8) & 0xff, HDMI_AUD_N2);
> -	hdmi_writeb(hdmi, (value >> 16) & 0x0f, HDMI_AUD_N3);
> +	/* set ncts_atomic_write */
> +	hdmi_modb(hdmi, HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET,
> +		  HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK, HDMI_AUD_N3);

Bits 7:4 of N3 are marked up in the iMX6 manuals as "reserved".  We need
some clarification from FSL whether this matters, but at the moment my
opinion on that is this should be conditionalised against the IP version.

Setting bit 7 as you do above may not cause any harm on iMX6, but on the
other hand, we shouldn't be setting bits which are marked as reserved.

> +
> +	/* set cts manual */
> +	hdmi_modb(hdmi, HDMI_AUD_CTS3_CTS_MANUAL,
> +		  HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
>  
>  	/* nshift factor = 0 */
>  	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_N_SHIFT_MASK, HDMI_AUD_CTS3);
> -}
> -
> -static void hdmi_regenerate_cts(struct dw_hdmi *hdmi, unsigned int cts)
> -{
> -	/* Must be set/cleared first */
> -	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
>  
> -	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
> +	/* set cts values */
> +	hdmi_modb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK),
> +		  HDMI_AUD_CTS3_AUDCTS19_16_MASK, HDMI_AUD_CTS3);
>  	hdmi_writeb(hdmi, (cts >> 8) & 0xff, HDMI_AUD_CTS2);
> -	hdmi_writeb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK) |
> -		    HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
> +	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
> +
> +	/* set n values */
> +	hdmi_modb(hdmi, (n >> 16) & HDMI_AUD_N3_AUDN19_16_MASK,
> +		  HDMI_AUD_N3_AUDN19_16_MASK, HDMI_AUD_N3);
> +	hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2);
> +	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);

This is definitely moving things in the right direction.  However, I would
ask that the read-modify-writes to HDMI_AUD_N3 are open-coded rather than
using hdmi_modb(), and consolidated.  We really don't need to keep
re-reading this register.

I'd also like to check that this does not cause a regression on iMX6 SoCs
with my audio driver; I'm aware that there are some issues to do with how
these registers are written, and the published errata workaround in the
iMX6 errata documentation doesn't seem to always work.

>  }
>  
>  static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
> @@ -355,8 +360,8 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
>  		__func__, hdmi->sample_rate, hdmi->ratio,
>  		pixel_clk, clk_n, clk_cts);
>  
> -	hdmi_set_clock_regenerator_n(hdmi, clk_n);
> -	hdmi_regenerate_cts(hdmi, clk_cts);
> +	hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts);
> +	hdmi_set_schnl(hdmi);

I'd prefer the addition of hdmi_set_schnl() to be in the patch which adds
that new function.  However, as I mentioned in my reply to that patch,
other versions of this IP do not have these registers, and it may not be
safe to write to them.  We need to find a way to know whether this IP
has the AHB DMA support or not and act accordingly.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 01/12] drm: bridge/dw_hdmi: adjust n/cts setting order

[Yang Kuankuan]

On 01/31/2015 06:07 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:25:30AM -0500, Yakir Yang wrote:
>> For Designerware HDMI, the following write sequence is recommended:
>> 1. aud_n3 (set bit ncts_atomic_write if desired)
>> 2. aud_cts3 (set CTS_manual and CTS value if desired/enabled)
>> 3. aud_cts2 (required in CTS_manual)
>> 4. aud_cts1 (required in CTS_manual)
>> 5. aud_n3 (bit ncts_atomic_write with same value as in step 1.)
>> 6. aud_n2
>> 7. aud_n1
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v2:
>> - adjust n/cts setting order
>>
>>   drivers/gpu/drm/bridge/dw_hdmi.c | 37 +++++++++++++++++++++----------------
>>   drivers/gpu/drm/bridge/dw_hdmi.h |  6 ++++++
>>   2 files changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
>> index cd6a706..423addc 100644
>> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
>> @@ -177,26 +177,31 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>>   	hdmi_modb(hdmi, data << shift, mask, reg);
>>   }
>>   
>> -static void hdmi_set_clock_regenerator_n(struct dw_hdmi *hdmi,
>> -					 unsigned int value)
>> +static void hdmi_regenerate_n_cts(struct dw_hdmi *hdmi, unsigned int n,
>> +				  unsigned int cts)
>>   {
>> -	hdmi_writeb(hdmi, value & 0xff, HDMI_AUD_N1);
>> -	hdmi_writeb(hdmi, (value >> 8) & 0xff, HDMI_AUD_N2);
>> -	hdmi_writeb(hdmi, (value >> 16) & 0x0f, HDMI_AUD_N3);
>> +	/* set ncts_atomic_write */
>> +	hdmi_modb(hdmi, HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET,
>> +		  HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK, HDMI_AUD_N3);
> Bits 7:4 of N3 are marked up in the iMX6 manuals as "reserved".  We need
> some clarification from FSL whether this matters, but at the moment my
> opinion on that is this should be conditionalised against the IP version.

Should we take the design_id(0x13 on iMX6, 0x20 on rk3288) to separate it.


> Setting bit 7 as you do above may not cause any harm on iMX6, but on the
> other hand, we shouldn't be setting bits which are marked as reserved.
>
>> +
>> +	/* set cts manual */
>> +	hdmi_modb(hdmi, HDMI_AUD_CTS3_CTS_MANUAL,
>> +		  HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
>>   
>>   	/* nshift factor = 0 */
>>   	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_N_SHIFT_MASK, HDMI_AUD_CTS3);
>> -}
>> -
>> -static void hdmi_regenerate_cts(struct dw_hdmi *hdmi, unsigned int cts)
>> -{
>> -	/* Must be set/cleared first */
>> -	hdmi_modb(hdmi, 0, HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
>>   
>> -	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
>> +	/* set cts values */
>> +	hdmi_modb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK),
>> +		  HDMI_AUD_CTS3_AUDCTS19_16_MASK, HDMI_AUD_CTS3);
>>   	hdmi_writeb(hdmi, (cts >> 8) & 0xff, HDMI_AUD_CTS2);
>> -	hdmi_writeb(hdmi, ((cts >> 16) & HDMI_AUD_CTS3_AUDCTS19_16_MASK) |
>> -		    HDMI_AUD_CTS3_CTS_MANUAL, HDMI_AUD_CTS3);
>> +	hdmi_writeb(hdmi, cts & 0xff, HDMI_AUD_CTS1);
>> +
>> +	/* set n values */
>> +	hdmi_modb(hdmi, (n >> 16) & HDMI_AUD_N3_AUDN19_16_MASK,
>> +		  HDMI_AUD_N3_AUDN19_16_MASK, HDMI_AUD_N3);
>> +	hdmi_writeb(hdmi, (n >> 8) & 0xff, HDMI_AUD_N2);
>> +	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
> This is definitely moving things in the right direction.  However, I would
> ask that the read-modify-writes to HDMI_AUD_N3 are open-coded rather than
> using hdmi_modb(), and consolidated.  We really don't need to keep
> re-reading this register.

Maybe we also can take design_id to separate it here.

>
> I'd also like to check that this does not cause a regression on iMX6 SoCs
> with my audio driver; I'm aware that there are some issues to do with how
> these registers are written, and the published errata workaround in the
> iMX6 errata documentation doesn't seem to always work.
>
>>   }
>>   
>>   static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
>> @@ -355,8 +360,8 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
>>   		__func__, hdmi->sample_rate, hdmi->ratio,
>>   		pixel_clk, clk_n, clk_cts);
>>   
>> -	hdmi_set_clock_regenerator_n(hdmi, clk_n);
>> -	hdmi_regenerate_cts(hdmi, clk_cts);
>> +	hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts);
>> +	hdmi_set_schnl(hdmi);
> I'd prefer the addition of hdmi_set_schnl() to be in the patch which adds
> that new function.  However, as I mentioned in my reply to that patch,
> other versions of this IP do not have these registers, and it may not be
> safe to write to them.  We need to find a way to know whether this IP
> has the AHB DMA support or not and act accordingly.
>

[PATCH v2 02/12] drm: bridge/dw_hdmi: add audio sample channel status setting

[Yakir Yang]

When transmitting IEC60985 linear PCM audio, we configure the
Aduio Sample Channel Status information of all the channel
status bits in the IEC60958 frame.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Add audio sample channel status setting

 drivers/gpu/drm/bridge/dw_hdmi.c | 41 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/bridge/dw_hdmi.h | 20 ++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 423addc..2ded957 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -204,6 +204,47 @@ static void hdmi_regenerate_n_cts(struct dw_hdmi *hdmi, unsigned int n,
 	hdmi_writeb(hdmi, n & 0xff, HDMI_AUD_N1);
 }
 
+static void hdmi_set_schnl(struct dw_hdmi *hdmi)
+{
+	u8 aud_schnl_samplerate;
+
+	switch (hdmi->sample_rate) {
+	case 32000:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K;
+		break;
+	case 44100:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_44K1;
+		break;
+	case 48000:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_48K;
+		break;
+	case 88200:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_88K2;
+		break;
+	case 96000:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_96K;
+		break;
+	case 176400:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_176K4;
+		break;
+	case 192000:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_192K;
+		break;
+	case 768000:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_768K;
+		break;
+	default:
+		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_44K1;
+		break;
+	}
+
+	/* set channel status register */
+	hdmi_modb(hdmi, aud_schnl_samplerate,
+		  HDMI_FC_AUDSCHNLS7_SMPRATE_MASK, HDMI_FC_AUDSCHNLS7);
+	hdmi_writeb(hdmi, ((~aud_schnl_samplerate) << 4) | 0x2,
+		    HDMI_FC_AUDSCHNLS8);
+}
+
 static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
 				   unsigned int ratio)
 {
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index bc53452..603e645 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -162,6 +162,17 @@
 #define HDMI_FC_SPDDEVICEINF                    0x1062
 #define HDMI_FC_AUDSCONF                        0x1063
 #define HDMI_FC_AUDSSTAT                        0x1064
+#define HDMI_FC_AUDSV                           0x1065
+#define HDMI_FC_AUDSU                           0x1066
+#define HDMI_FC_AUDSCHNLS0                      0x1067
+#define HDMI_FC_AUDSCHNLS1                      0x1068
+#define HDMI_FC_AUDSCHNLS2                      0x1069
+#define HDMI_FC_AUDSCHNLS3                      0x106a
+#define HDMI_FC_AUDSCHNLS4                      0x106b
+#define HDMI_FC_AUDSCHNLS5                      0x106c
+#define HDMI_FC_AUDSCHNLS6                      0x106d
+#define HDMI_FC_AUDSCHNLS7                      0x106e
+#define HDMI_FC_AUDSCHNLS8                      0x106f
 #define HDMI_FC_DATACH0FILL                     0x1070
 #define HDMI_FC_DATACH1FILL                     0x1071
 #define HDMI_FC_DATACH2FILL                     0x1072
@@ -731,6 +742,15 @@ enum {
 /* HDMI_FC_AUDSCHNLS7 field values */
 	HDMI_FC_AUDSCHNLS7_ACCURACY_OFFSET = 4,
 	HDMI_FC_AUDSCHNLS7_ACCURACY_MASK = 0x30,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_MASK = 0x0f,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_192K = 0xe,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_176K4 = 0xc,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_96K = 0xa,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_768K = 0x9,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_88K2 = 0x8,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_32K = 0x3,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_48K = 0x2,
+	HDMI_FC_AUDSCHNLS7_SMPRATE_44K1 = 0x0,
 
 /* HDMI_FC_AUDSCHNLS8 field values */
 	HDMI_FC_AUDSCHNLS8_ORIGSAMPFREQ_MASK = 0xf0,
-- 
2.1.2

Re: [PATCH v2 02/12] drm: bridge/dw_hdmi: add audio sample channel status setting

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:27:10AM -0500, Yakir Yang wrote:
> When transmitting IEC60985 linear PCM audio, we configure the
> Aduio Sample Channel Status information of all the channel
> status bits in the IEC60958 frame.
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v2:
> - Add audio sample channel status setting

See my comments on

"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"

sent immediately prior to this series.  This patch appears to be a copy of
that patch.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 02/12] drm: bridge/dw_hdmi: add audio sample channel status setting

[Yang Kuankuan]

On 01/31/2015 06:08 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:27:10AM -0500, Yakir Yang wrote:
>> When transmitting IEC60985 linear PCM audio, we configure the
>> Aduio Sample Channel Status information of all the channel
>> status bits in the IEC60958 frame.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v2:
>> - Add audio sample channel status setting
> See my comments on
>
> "[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"
>
> sent immediately prior to this series.  This patch appears to be a copy of
> that patch.
>
"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"

  should not be send, it's my mistaken, i will remove it.

Thanks.  : )

Re: [PATCH v2 02/12] drm: bridge/dw_hdmi: add audio sample channel status setting

[Russell King - ARM Linux]

On Sat, Jan 31, 2015 at 06:22:19AM -0500, Yang Kuankuan wrote:
> 
> On 01/31/2015 06:08 AM, Russell King - ARM Linux wrote:
> >On Fri, Jan 30, 2015 at 06:27:10AM -0500, Yakir Yang wrote:
> >>When transmitting IEC60985 linear PCM audio, we configure the
> >>Aduio Sample Channel Status information of all the channel
> >>status bits in the IEC60958 frame.
> >>
> >>Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> >>---
> >>Changes in v2:
> >>- Add audio sample channel status setting
> >See my comments on
> >
> >"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"
> >
> >sent immediately prior to this series.  This patch appears to be a copy of
> >that patch.
> >
> "[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"
> 
>  should not be send, it's my mistaken, i will remove it.

Please see my comments on the above patch for comments on _this_ patch.
All my comments on the mistakenly sent 01/11 also apply to 02/12.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 03/12] drm: bridge/dw_hdmi: add irq control to suspend/resume

[Yakir Yang]

when kernel enter into suspend, cpus will shutdown, hdmi registers
will reset invisibly. After kernel resume, drm core will call the
bridge enable function. All of hdmi registers will be setup again
except the interrupt registers. In that case we should mute all the
interrupt in suspend stage, and umnute the interrupt we need in the
resume stage.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Add irq control to suspend/resume interfaces

 drivers/gpu/drm/bridge/dw_hdmi.c | 43 ++++++++++++++++++++++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h     |  2 ++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 2ded957..13f26af 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1745,6 +1745,49 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
+int dw_hdmi_suspend(struct platform_device *pdev, pm_message_t state)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(&pdev->dev);
+	u8 ih_mute;
+
+	/* Disable all interrupts */
+	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
+
+	 /* Disable top level interrupt bits in HDMI block */
+	ih_mute = hdmi_readb(hdmi, HDMI_IH_MUTE) |
+		  HDMI_IH_MUTE_MUTE_WAKEUP_INTERRUPT |
+		  HDMI_IH_MUTE_MUTE_ALL_INTERRUPT;
+
+	hdmi_writeb(hdmi, ih_mute, HDMI_IH_MUTE);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_suspend);
+
+int dw_hdmi_resume(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(&pdev->dev);
+
+	initialize_hdmi_ih_mutes(hdmi);
+
+	dw_hdmi_fb_registered(hdmi);
+
+	/*
+	 * Configure registers related to HDMI interrupt
+	 * generation before registering IRQ.
+	 */
+	hdmi_writeb(hdmi, HDMI_PHY_HPD, HDMI_PHY_POL0);
+
+	/* Clear Hotplug interrupts */
+	hdmi_writeb(hdmi, HDMI_IH_PHY_STAT0_HPD, HDMI_IH_PHY_STAT0);
+
+	/* Unmute interrupts */
+	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_resume);
+
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
 MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
 MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 5a4f490..8476cfc 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -53,6 +53,8 @@ struct dw_hdmi_plat_data {
 					   struct drm_display_mode *mode);
 };
 
+int dw_hdmi_resume(struct platform_device *pdev);
+int dw_hdmi_suspend(struct platform_device *pdev, pm_message_t state);
 void dw_hdmi_unbind(struct device *dev, struct device *master, void *data);
 int dw_hdmi_bind(struct device *dev, struct device *master,
 		 void *data, struct drm_encoder *encoder,
-- 
2.1.2

Re: [PATCH v2 03/12] drm: bridge/dw_hdmi: add irq control to suspend/resume

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:28:00AM -0500, Yakir Yang wrote:
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
> index 2ded957..13f26af 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
> @@ -1745,6 +1745,49 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>  }
>  EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>  
> +int dw_hdmi_suspend(struct platform_device *pdev, pm_message_t state)
> +{
...
> +int dw_hdmi_resume(struct platform_device *pdev)
> +{

Please arrange for these functions to take a struct device rather than
a struct platform_device.  The generic part of this driver should remain
bus-agnostic.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 03/12] drm: bridge/dw_hdmi: add irq control to suspend/resume

[Yang Kuankuan]

On 01/31/2015 06:11 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:28:00AM -0500, Yakir Yang wrote:
>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
>> index 2ded957..13f26af 100644
>> --- a/drivers/gpu/drm/bridge/dw_hdmi.c
>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.c
>> @@ -1745,6 +1745,49 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
>>   }
>>   EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
>>   
>> +int dw_hdmi_suspend(struct platform_device *pdev, pm_message_t state)
>> +{
> ...
>> +int dw_hdmi_resume(struct platform_device *pdev)
>> +{
> Please arrange for these functions to take a struct device rather than
> a struct platform_device.  The generic part of this driver should remain
> bus-agnostic.
>
> Thanks.
okay, is it good to replace "struct platform_device" with "struct device" ?

Thanks : )

[PATCH v2 04/12] drm: rockchip/dw_hdmi_rockchip: add resume/suspend support

[Yakir Yang]

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Add suspend/resume support for dw_hdmi_rockchip driver

 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
index d236faa..2f8bacb 100644
--- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
+++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
@@ -323,9 +323,22 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static int dw_hdmi_rockchip_suspend(struct platform_device *pdev,
+				    pm_message_t state)
+{
+	return dw_hdmi_suspend(pdev, state);
+}
+
+static int dw_hdmi_rockchip_resume(struct platform_device *pdev)
+{
+	return dw_hdmi_resume(pdev);
+}
+
 static struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
 	.probe  = dw_hdmi_rockchip_probe,
 	.remove = dw_hdmi_rockchip_remove,
+	.resume = dw_hdmi_rockchip_resume,
+	.suspend = dw_hdmi_rockchip_suspend,
 	.driver = {
 		.name = "dwhdmi-rockchip",
 		.of_match_table = dw_hdmi_rockchip_dt_ids,
-- 
2.1.2

Re: [PATCH v2 04/12] drm: rockchip/dw_hdmi_rockchip: add resume/suspend support

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:28:59AM -0500, Yakir Yang wrote:
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v2:
> - Add suspend/resume support for dw_hdmi_rockchip driver
> 
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index d236faa..2f8bacb 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -323,9 +323,22 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int dw_hdmi_rockchip_suspend(struct platform_device *pdev,
> +				    pm_message_t state)
> +{
> +	return dw_hdmi_suspend(pdev, state);
> +}
> +
> +static int dw_hdmi_rockchip_resume(struct platform_device *pdev)
> +{
> +	return dw_hdmi_resume(pdev);
> +}
> +
>  static struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
>  	.probe  = dw_hdmi_rockchip_probe,
>  	.remove = dw_hdmi_rockchip_remove,
> +	.resume = dw_hdmi_rockchip_resume,
> +	.suspend = dw_hdmi_rockchip_suspend,
>  	.driver = {
>  		.name = "dwhdmi-rockchip",
>  		.of_match_table = dw_hdmi_rockchip_dt_ids,

Using the power management operations (setting the .pm member in
the embedded struct device_driver) is preferred over using the
.resume and .suspend methods.

Please update this patch to use the preferred method.  Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 04/12] drm: rockchip/dw_hdmi_rockchip: add resume/suspend support

[Yang Kuankuan]

On 01/31/2015 06:13 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:28:59AM -0500, Yakir Yang wrote:
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v2:
>> - Add suspend/resume support for dw_hdmi_rockchip driver
>>
>>   drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> index d236faa..2f8bacb 100644
>> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
>> @@ -323,9 +323,22 @@ static int dw_hdmi_rockchip_remove(struct platform_device *pdev)
>>   	return 0;
>>   }
>>   
>> +static int dw_hdmi_rockchip_suspend(struct platform_device *pdev,
>> +				    pm_message_t state)
>> +{
>> +	return dw_hdmi_suspend(pdev, state);
>> +}
>> +
>> +static int dw_hdmi_rockchip_resume(struct platform_device *pdev)
>> +{
>> +	return dw_hdmi_resume(pdev);
>> +}
>> +
>>   static struct platform_driver dw_hdmi_rockchip_pltfm_driver = {
>>   	.probe  = dw_hdmi_rockchip_probe,
>>   	.remove = dw_hdmi_rockchip_remove,
>> +	.resume = dw_hdmi_rockchip_resume,
>> +	.suspend = dw_hdmi_rockchip_suspend,
>>   	.driver = {
>>   		.name = "dwhdmi-rockchip",
>>   		.of_match_table = dw_hdmi_rockchip_dt_ids,
> Using the power management operations (setting the .pm member in
> the embedded struct device_driver) is preferred over using the
> .resume and .suspend methods.
>
> Please update this patch to use the preferred method.  Thanks.
>
Okay, I will fixed it in next version v3.

Thanks. : )

[PATCH v2 05/12] drm: rockchip/vop: filter interlace display mode

[Yakir Yang]

RK3288's VOP do not support INTERLACE display mode, so we should
remove those modes out of mode_ok list.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- filter interlace display mode for rockchip vop

 drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 9a5c571..bedab42 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -808,7 +808,8 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
 				const struct drm_display_mode *mode,
 				struct drm_display_mode *adjusted_mode)
 {
-	if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0)
+	if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0 ||
+	    (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE))
 		return false;
 
 	return true;
-- 
2.1.2

Re: [PATCH v2 05/12] drm: rockchip/vop: filter interlace display mode

[Daniel Kurtz]

Hi ykk,

On Fri, Jan 30, 2015 at 7:29 PM, Yakir Yang <ykk@rock-chips.com> wrote:
> RK3288's VOP do not support INTERLACE display mode, so we should
> remove those modes out of mode_ok list.
>
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>


Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>

Can you move this patch out of your hdmi audio patch set, and send it
separately?
This fix is independent of the others.

Thanks!


> ---
> Changes in v2:
> - filter interlace display mode for rockchip vop
>
>  drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> index 9a5c571..bedab42 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
> @@ -808,7 +808,8 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
>                                 const struct drm_display_mode *mode,
>                                 struct drm_display_mode *adjusted_mode)
>  {
> -       if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0)
> +       if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0 ||
> +           (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE))
>                 return false;
>
>         return true;
> --
> 2.1.2
>
>
> --
> You received this message because you are subscribed to the Google Groups "rockchip-discuss" group.
> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/rockchip-discuss/1422617388-25476-1-git-send-email-ykk%40rock-chips.com.
>
> To unsubscribe from this group and stop receiving emails from it, send an email to rockchip-discuss+unsubscribe@chromium.org.

Re: [PATCH v2 05/12] drm: rockchip/vop: filter interlace display mode

[Yang Kuankuan]

On 02/02/2015 03:00 AM, Daniel Kurtz wrote:
> Hi ykk,
>
> On Fri, Jan 30, 2015 at 7:29 PM, Yakir Yang <ykk@rock-chips.com> wrote:
>> RK3288's VOP do not support INTERLACE display mode, so we should
>> remove those modes out of mode_ok list.
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>
> Reviewed-by: Daniel Kurtz <djkurtz@chromium.org>
>
> Can you move this patch out of your hdmi audio patch set, and send it
> separately?
> This fix is independent of the others.
>
> Thanks!
>

Okay. : )

>> ---
>> Changes in v2:
>> - filter interlace display mode for rockchip vop
>>
>>   drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> index 9a5c571..bedab42 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
>> @@ -808,7 +808,8 @@ static bool vop_crtc_mode_fixup(struct drm_crtc *crtc,
>>                                  const struct drm_display_mode *mode,
>>                                  struct drm_display_mode *adjusted_mode)
>>   {
>> -       if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0)
>> +       if (adjusted_mode->htotal == 0 || adjusted_mode->vtotal == 0 ||
>> +           (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE))
>>                  return false;
>>
>>          return true;
>> --
>> 2.1.2
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups "rockchip-discuss" group.
>> To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/rockchip-discuss/1422617388-25476-1-git-send-email-ykk%40rock-chips.com.
>>
>> To unsubscribe from this group and stop receiving emails from it, send an email to rockchip-discuss+unsubscribe@chromium.org.
>
>

[PATCH v2 06/12] drm: bridge/dw_hdmi: add audio support for more display resolutions

[Yakir Yang]

Add more n/cts values, in that case we can support audio for more
display resolutions (128 * SampleRate = PixelClock * N / CTS).

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- add more n/cts combinations for more display resolutions

 drivers/gpu/drm/bridge/dw_hdmi.c | 58 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 13f26af..e07723c 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -265,8 +265,24 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
 	case 44100:
 		if (pixel_clk == 25170000)
 			n = 7007;
+		else if (pixel_clk == 25175000)
+			n = 28224;
+		else if (pixel_clk == 40000000)
+			n = 7056;
+		else if (pixel_clk == 54000000)
+			n = 6272;
+		else if (pixel_clk == 65000000)
+			n = 7056;
 		else if (pixel_clk == 74170000)
 			n = 17836;
+		else if (pixel_clk == 74250000)
+			n = 6272;
+		else if (pixel_clk == 83500000)
+			n = 7056;
+		else if (pixel_clk == 106500000)
+			n = 4074;
+		else if (pixel_clk == 108000000)
+			n = 4018;
 		else if (pixel_clk == 148350000)
 			n = (ratio == 150) ? 17836 : 8918;
 		else
@@ -276,10 +292,26 @@ static unsigned int hdmi_compute_n(unsigned int freq, unsigned long pixel_clk,
 	case 48000:
 		if (pixel_clk == 25170000)
 			n = (ratio == 150) ? 9152 : 6864;
+		else if (pixel_clk == 25175000)
+			n = 12288;
 		else if (pixel_clk == 27020000)
 			n = (ratio == 150) ? 8192 : 6144;
+		else if (pixel_clk == 40000000)
+			n = 6144;
+		else if (pixel_clk == 54000000)
+			n = 6144;
+		else if (pixel_clk == 65000000)
+			n = 6144;
 		else if (pixel_clk == 74170000)
 			n = 11648;
+		else if (pixel_clk == 74250000)
+			n = 6144;
+		else if (pixel_clk == 83500000)
+			n = 6144;
+		else if (pixel_clk == 106500000)
+			n = 6144;
+		else if (pixel_clk == 108000000)
+			n = 6144;
 		else if (pixel_clk == 148350000)
 			n = (ratio == 150) ? 11648 : 5824;
 		else
@@ -327,10 +359,18 @@ static unsigned int hdmi_compute_cts(unsigned int freq, unsigned long pixel_clk,
 	case 96000:
 	case 192000:
 		switch (pixel_clk) {
+		case 25175000:
+			cts = 50350;
+			break;
 		case 25200000:
 		case 27000000:
+		case 40000000:
 		case 54000000:
+		case 65000000:
 		case 74250000:
+		case 83500000:
+		case 106500000:
+		case 108000000:
 		case 148500000:
 			cts = pixel_clk / 1000;
 			break;
@@ -351,18 +391,36 @@ static unsigned int hdmi_compute_cts(unsigned int freq, unsigned long pixel_clk,
 	case 88200:
 	case 176400:
 		switch (pixel_clk) {
+		case 25175000:
+			cts = 125875;
+			break;
 		case 25200000:
 			cts = 28000;
 			break;
 		case 27000000:
 			cts = 30000;
 			break;
+		case 40000000:
+			cts = 50000;
+			break;
 		case 54000000:
 			cts = 60000;
 			break;
+		case 65000000:
+			cts = 81250;
+			break;
 		case 74250000:
 			cts = 82500;
 			break;
+		case 83500000:
+			cts = 104375;
+			break;
+		case 106500000:
+			cts = 88750;
+			break;
+		case 108000000:
+			cts = 76875;
+			break;
 		case 148500000:
 			cts = 165000;
 			break;
-- 
2.1.2

Re: [PATCH v2 06/12] drm: bridge/dw_hdmi: add audio support for more display resolutions

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:30:39AM -0500, Yakir Yang wrote:
> Add more n/cts values, in that case we can support audio for more
> display resolutions (128 * SampleRate = PixelClock * N / CTS).

Where do these come from?  The iMX6 manuals give the set which are
already in the driver, and says that others are not supported - to
quote...

"Table below shows the CTS and N values for the supported standard.
All other TMDS clocks are not supported, the TMDS clocks divided or
multiplied by 1,001 coefficients are not supported."

and the table lists values for TMDS clocks of 25.2, 27, 54, 74.25,
148.5 and 297MHz.

This will need to be tested on iMX6 to validate whether it works.
If it doesn't work, we need to conditionalise the new TMDS clocks
with the IP version.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 06/12] drm: bridge/dw_hdmi: add audio support for more display resolutions

[Yang Kuankuan]

On 01/31/2015 06:20 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:30:39AM -0500, Yakir Yang wrote:
>> Add more n/cts values, in that case we can support audio for more
>> display resolutions (128 * SampleRate = PixelClock * N / CTS).
> Where do these come from?  The iMX6 manuals give the set which are
> already in the driver, and says that others are not supported - to
> quote...
>
> "Table below shows the CTS and N values for the supported standard.
> All other TMDS clocks are not supported, the TMDS clocks divided or
> multiplied by 1,001 coefficients are not supported."
>
> and the table lists values for TMDS clocks of 25.2, 27, 54, 74.25,
> 148.5 and 297MHz.
>
> This will need to be tested on iMX6 to validate whether it works.
> If it doesn't work, we need to conditionalise the new TMDS clocks
> with the IP version.
>
I just want to add audio support for some No-CEA display resolutions, 
and actually it can works on rk3288 platform.
If it can not work on iMX6 platform, i will add IP verison select in 
next version v3.

Thanks.  : )

[PATCH v2 07/12] drm: bridge/dw_hdmi: enable audio support for No-CEA display resolutions

[Yakir Yang]

If the monitor support audio, so we should support audio for it, even if
the display resolution is No-CEA mode.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- enable audio support for No-CEA display mode

 drivers/gpu/drm/bridge/dw_hdmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index e07723c..8cc7b3d 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1286,13 +1286,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 
 	hdmi->vic = drm_match_cea_mode(mode);
 
-	if (!hdmi->vic) {
+	if (hdmi->hdmi_data.video_mode.mdvi)
 		dev_dbg(hdmi->dev, "Non-CEA mode used in HDMI\n");
-		hdmi->hdmi_data.video_mode.mdvi = true;
-	} else {
+	else
 		dev_dbg(hdmi->dev, "CEA mode used vic=%d\n", hdmi->vic);
-		hdmi->hdmi_data.video_mode.mdvi = false;
-	}
 
 	if ((hdmi->vic == 6) || (hdmi->vic == 7) ||
 	    (hdmi->vic == 21) || (hdmi->vic == 22) ||
@@ -1496,6 +1493,7 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
+	struct hdmi_vmode *vmode = &hdmi->hdmi_data.video_mode;
 	struct edid *edid;
 	int ret;
 
@@ -1507,6 +1505,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
 			edid->width_cm, edid->height_cm);
 
+		vmode->mdvi = !drm_detect_hdmi_monitor(edid);
+
 		drm_mode_connector_update_edid_property(connector, edid);
 		ret = drm_add_edid_modes(connector, edid);
 		kfree(edid);
-- 
2.1.2

Re: [PATCH v2 07/12] drm: bridge/dw_hdmi: enable audio support for No-CEA display resolutions

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:31:33AM -0500, Yakir Yang wrote:
> If the monitor support audio, so we should support audio for it, even if
> the display resolution is No-CEA mode.

I can't find where it was documented at the moment, but I seem to remember
reading somewhere that the iMX6 SoC version of this IP doesn't support
audio with non-CEA modes - though, maybe that's a result of the limited
TMDS clocks which are supported.

This is a change which will need testing on iMX6.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yakir Yang]

Designware HDMI supports four interfaces to config hdmi audio
(I2S, S/PDIF, Generic Parallel Audio, AHB Audio DMA), but rk3288
only support two ways to config hdmi audio(I2S, S/PDIF), So we
take I2S as hdmi audio operation interfaces.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Add audio config interfaces to dw_hdmi driver

 drivers/gpu/drm/bridge/dw_hdmi.c | 70 +++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/bridge/dw_hdmi.h |  6 ++++
 include/drm/bridge/dw_hdmi.h     | 30 +++++++++++++++++
 3 files changed, 95 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 8cc7b3d..25c1678 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/hdmi.h>
+#include <linux/mutex.h>
 #include <linux/of_device.h>
 
 #include <drm/drm_of.h>
@@ -126,7 +127,10 @@ struct dw_hdmi {
 	struct i2c_adapter *ddc;
 	void __iomem *regs;
 
-	unsigned int sample_rate;
+	struct hdmi_audio_fmt aud_fmt;
+	struct mutex audio_mutex;
+	bool audio_enable;
+
 	int ratio;
 
 	void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
@@ -208,7 +212,7 @@ static void hdmi_set_schnl(struct dw_hdmi *hdmi)
 {
 	u8 aud_schnl_samplerate;
 
-	switch (hdmi->sample_rate) {
+	switch (hdmi->aud_fmt.sample_rate) {
 	case 32000:
 		aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K;
 		break;
@@ -444,9 +448,9 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 {
 	unsigned int clk_n, clk_cts;
 
-	clk_n = hdmi_compute_n(hdmi->sample_rate, pixel_clk,
+	clk_n = hdmi_compute_n(hdmi->aud_fmt.sample_rate, pixel_clk,
 			       hdmi->ratio);
-	clk_cts = hdmi_compute_cts(hdmi->sample_rate, pixel_clk,
+	clk_cts = hdmi_compute_cts(hdmi->aud_fmt.sample_rate, pixel_clk,
 				   hdmi->ratio);
 
 	if (!clk_cts) {
@@ -456,7 +460,7 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 	}
 
 	dev_dbg(hdmi->dev, "%s: samplerate=%d  ratio=%d  pixelclk=%lu  N=%d cts=%d\n",
-		__func__, hdmi->sample_rate, hdmi->ratio,
+		__func__, hdmi->aud_fmt.sample_rate, hdmi->ratio,
 		pixel_clk, clk_n, clk_cts);
 
 	hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts);
@@ -1023,6 +1027,25 @@ static void hdmi_tx_hdcp_config(struct dw_hdmi *hdmi)
 		  HDMI_A_HDCPCFG1_ENCRYPTIONDISABLE_MASK, HDMI_A_HDCPCFG1);
 }
 
+static void hdmi_config_audio(struct dw_hdmi *hdmi,
+			      struct hdmi_audio_fmt *aud_fmt)
+{
+	if (aud_fmt)
+		hdmi->aud_fmt = *aud_fmt;
+
+	hdmi_modb(hdmi, AUDIO_CONF0_INTERFACE_II2S,
+		  AUDIO_CONF0_INTERFACE_MSK, HDMI_AUD_CONF0);
+
+	hdmi_modb(hdmi, hdmi->aud_fmt.chan_num, AUDIO_CONF0_I2SINEN_MSK,
+		  HDMI_AUD_CONF0);
+
+	hdmi_modb(hdmi, hdmi->aud_fmt.word_length, AUDIO_CONF1_DATWIDTH_MSK,
+		  HDMI_AUD_CONF1);
+
+	hdmi_modb(hdmi, hdmi->aud_fmt.dai_fmt, AUDIO_CONF1_DATAMODE_MSK,
+		  HDMI_AUD_CONF1);
+}
+
 static void hdmi_config_AVI(struct dw_hdmi *hdmi)
 {
 	u8 val, pix_fmt, under_scan;
@@ -1212,6 +1235,34 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
 	hdmi->phy_enabled = false;
 }
 
+void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
+{
+	if (hdmi->audio_enable)
+		return;
+
+	mutex_lock(&hdmi->audio_mutex);
+	hdmi->audio_enable = true;
+	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+	mutex_unlock(&hdmi->audio_mutex);
+}
+
+void hdmi_audio_clk_disable(struct dw_hdmi *hdmi)
+{
+	if (!hdmi->audio_enable)
+		return;
+
+	mutex_lock(&hdmi->audio_mutex);
+	hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+	hdmi->audio_enable = false;
+	mutex_unlock(&hdmi->audio_mutex);
+}
+
+bool hdmi_get_connect_status(struct dw_hdmi *hdmi)
+{
+	return (hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD) ? true : false;
+}
+
 /* HDMI Initialization Step B.4 */
 static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 {
@@ -1240,11 +1291,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 		clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
 		hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
 	}
-}
 
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
-	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+	hdmi_audio_clk_disable(hdmi);
 }
 
 /* Workaround to clear the overflow condition */
@@ -1342,7 +1390,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 
 		/* HDMI Initialization Step E - Configure audio */
 		hdmi_clk_regenerator_update_pixel_clock(hdmi);
-		hdmi_enable_audio_clk(hdmi);
+		hdmi_audio_clk_enable(hdmi);
 
 		/* HDMI Initialization Step F - Configure AVI InfoFrame */
 		hdmi_config_AVI(hdmi);
@@ -1669,7 +1717,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 	hdmi->plat_data = plat_data;
 	hdmi->dev = dev;
 	hdmi->dev_type = plat_data->dev_type;
-	hdmi->sample_rate = 48000;
+	hdmi->aud_fmt.sample_rate = 48000;
 	hdmi->ratio = 100;
 	hdmi->encoder = encoder;
 
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 603e645..3ab14b6 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -904,6 +904,12 @@ enum {
 	HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL = 0x08,
 	HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_MASK = 0x04,
 
+/* AUD_CONF0 field values */
+	AUDIO_CONF0_INTERFACE_MSK = 0x20,
+	AUDIO_CONF0_INTERFACE_II2S = 0x20,
+	AUDIO_CONF0_INTERFACE_SPDIF = 0x00,
+	AUDIO_CONF0_INTERFACE_GPA = 0x00,
+
 /* AUD_N3 field values */
 	HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK = 0x80,
 	HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET = 0x80,
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 8476cfc..aafa447 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -44,6 +44,36 @@ struct dw_hdmi_sym_term {
 	u16 term;       /*transmission termination value*/
 };
 
+enum hdmi_audio_wordlength {
+	AUDIO_WORDLENGTH_16BIT = 16,
+	AUDIO_WORDLENGTH_24BIT = 24,
+	AUDIO_CONF1_DATWIDTH_MSK = 0x1F,
+};
+
+enum hdmi_audio_daifmt {
+	AUDIO_DAIFMT_IIS = 0x00,
+	AUDIO_DAIFMT_RIGHT_J = 0x20,
+	AUDIO_DAIFMT_LEFT_J = 0x40,
+	AUDIO_DAIFMT_BURST_1 = 0x60,
+	AUDIO_DAIFMT_BURST_2 = 0x80,
+	AUDIO_CONF1_DATAMODE_MSK = 0xE0,
+};
+
+enum hdmi_audio_channelnum {
+	AUDIO_CHANNELNUM_2 = 0x01,
+	AUDIO_CHANNELNUM_4 = 0x03,
+	AUDIO_CHANNELNUM_6 = 0x07,
+	AUDIO_CHANNELNUM_8 = 0x0F,
+	AUDIO_CONF0_I2SINEN_MSK = 0x0F,
+};
+
+struct hdmi_audio_fmt {
+	unsigned int sample_rate;
+	enum hdmi_audio_daifmt dai_fmt;
+	enum hdmi_audio_channelnum chan_num;
+	enum hdmi_audio_wordlength word_length;
+};
+
 struct dw_hdmi_plat_data {
 	enum dw_hdmi_devtype dev_type;
 	const struct dw_hdmi_mpll_config *mpll_cfg;
-- 
2.1.2

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:32:23AM -0500, Yakir Yang wrote:
> +static void hdmi_config_audio(struct dw_hdmi *hdmi,
> +			      struct hdmi_audio_fmt *aud_fmt)
> +{
> +	if (aud_fmt)
> +		hdmi->aud_fmt = *aud_fmt;
> +
> +	hdmi_modb(hdmi, AUDIO_CONF0_INTERFACE_II2S,
> +		  AUDIO_CONF0_INTERFACE_MSK, HDMI_AUD_CONF0);
> +
> +	hdmi_modb(hdmi, hdmi->aud_fmt.chan_num, AUDIO_CONF0_I2SINEN_MSK,
> +		  HDMI_AUD_CONF0);
> +
> +	hdmi_modb(hdmi, hdmi->aud_fmt.word_length, AUDIO_CONF1_DATWIDTH_MSK,
> +		  HDMI_AUD_CONF1);
> +
> +	hdmi_modb(hdmi, hdmi->aud_fmt.dai_fmt, AUDIO_CONF1_DATAMODE_MSK,
> +		  HDMI_AUD_CONF1);

These registers are not defined on iMX6 SoCs, so this probably needs to be
conditionalised with the dw-hdmi IP version.

> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
> +{
> +	if (hdmi->audio_enable)
> +		return;
> +
> +	mutex_lock(&hdmi->audio_mutex);
> +	hdmi->audio_enable = true;
> +	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> +	mutex_unlock(&hdmi->audio_mutex);

This is racy.  The test needs to be within the mutex-protected region.

> +}
> +
> +void hdmi_audio_clk_disable(struct dw_hdmi *hdmi)
> +{
> +	if (!hdmi->audio_enable)
> +		return;
> +
> +	mutex_lock(&hdmi->audio_mutex);
> +	hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> +		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> +	hdmi->audio_enable = false;
> +	mutex_unlock(&hdmi->audio_mutex);

Ditto.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yang Kuankuan]

On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:32:23AM -0500, Yakir Yang wrote:
>> +static void hdmi_config_audio(struct dw_hdmi *hdmi,
>> +			      struct hdmi_audio_fmt *aud_fmt)
>> +{
>> +	if (aud_fmt)
>> +		hdmi->aud_fmt = *aud_fmt;
>> +
>> +	hdmi_modb(hdmi, AUDIO_CONF0_INTERFACE_II2S,
>> +		  AUDIO_CONF0_INTERFACE_MSK, HDMI_AUD_CONF0);
>> +
>> +	hdmi_modb(hdmi, hdmi->aud_fmt.chan_num, AUDIO_CONF0_I2SINEN_MSK,
>> +		  HDMI_AUD_CONF0);
>> +
>> +	hdmi_modb(hdmi, hdmi->aud_fmt.word_length, AUDIO_CONF1_DATWIDTH_MSK,
>> +		  HDMI_AUD_CONF1);
>> +
>> +	hdmi_modb(hdmi, hdmi->aud_fmt.dai_fmt, AUDIO_CONF1_DATAMODE_MSK,
>> +		  HDMI_AUD_CONF1);
> These registers are not defined on iMX6 SoCs, so this probably needs to be
> conditionalised with the dw-hdmi IP version.
okay, i will fixed what you have suggest in cover-letter first.

Thanks. : )
>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
>> +{
>> +	if (hdmi->audio_enable)
>> +		return;
>> +
>> +	mutex_lock(&hdmi->audio_mutex);
>> +	hdmi->audio_enable = true;
>> +	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>> +	mutex_unlock(&hdmi->audio_mutex);
> This is racy.  The test needs to be within the mutex-protected region.
This function will be called by other driver (dw-hdmi-audio), both 
modify the variable "hdmi->audio_enable", so i add the mutex-protected.
>
>> +}
>> +
>> +void hdmi_audio_clk_disable(struct dw_hdmi *hdmi)
>> +{
>> +	if (!hdmi->audio_enable)
>> +		return;
>> +
>> +	mutex_lock(&hdmi->audio_mutex);
>> +	hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>> +		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>> +	hdmi->audio_enable = false;
>> +	mutex_unlock(&hdmi->audio_mutex);
> Ditto.
>

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Daniel Kurtz]

Hi ykk,

On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
>
> On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
>>
>>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
>>> +{
>>> +       if (hdmi->audio_enable)
>>> +               return;
>>> +
>>> +       mutex_lock(&hdmi->audio_mutex);
>>> +       hdmi->audio_enable = true;
>>> +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>> HDMI_MC_CLKDIS);
>>> +       mutex_unlock(&hdmi->audio_mutex);
>>
>> This is racy.  The test needs to be within the mutex-protected region.
>
> This function will be called by other driver (dw-hdmi-audio), both modify
> the variable "hdmi->audio_enable", so i add the mutex-protected.

>From your comment it isn't clear whether you understand what Russell meant.
He is say you should do the following:

{
       mutex_lock(&hdmi->audio_mutex);

       if (hdmi->audio_enable) {
              mutex_unlock(&hdmi->audio_mutex);
              return;
       }
       hdmi->audio_enable = true;
       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);

       mutex_unlock(&hdmi->audio_mutex);
}

By the way, it doesn't matter that the function is called from another driver.
What matters is that this function can be called concurrently on
multiple different threads of execution to change the hdmi audio
enable state.
>From DRM land, it is called with DRM lock held when enabling/disabling
hdmi audio (mode_set / DPMS).
It is also called from audio land, when enabling/disabling audio in
response to some audio events (userspace ioctls?).  I'm not sure
exactly how the audio side works, or what locks are involved, but this
mutex synchronizes calls from these two worlds to ensure that
"hdmi->audio_enable" field always matches the current (intended)
status of the hdmi audio clock.  This would be useful, for example, if
you needed to temporarily disable all HDMI clocks during a mode set,
and then restore the audio clock to its pre-mode_set state:

  // temporarily disable hdmi audio clk
  dw_hdmi_audio_clk_disable(hdmi);
  // do mode_set ...
  dw_hdmi_audio_clk_reenable(hdmi);

 static void dw_hdmi_audio_clk_reenable()
 {
    mutex_lock()
    if (hdmi->audio_enable)
      dw_hdmi_audio_clk_enable(hdmi)
    mutex_unlock()
  }

However, AFAICT, the "hdmi->audio_enable" field is never actually used
like this here or in later patches, so it and the mutex do not seem to
actually be doing anything useful.  In this patch it is probably
better to just drop the mutex and audio_enable, and add them as a
preparatory patch in the patch set where they will actually be used.

-Dan

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Russell King - ARM Linux]

On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
> Hi ykk,
> 
> On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
> >
> > On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
> >>
> >>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
> >>> +{
> >>> +       if (hdmi->audio_enable)
> >>> +               return;
> >>> +
> >>> +       mutex_lock(&hdmi->audio_mutex);
> >>> +       hdmi->audio_enable = true;
> >>> +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> >>> HDMI_MC_CLKDIS);
> >>> +       mutex_unlock(&hdmi->audio_mutex);
> >>
> >> This is racy.  The test needs to be within the mutex-protected region.
> >
> > This function will be called by other driver (dw-hdmi-audio), both modify
> > the variable "hdmi->audio_enable", so i add the mutex-protected.
> 
> >From your comment it isn't clear whether you understand what Russell meant.
> He is say you should do the following:
> 
> {
>        mutex_lock(&hdmi->audio_mutex);
> 
>        if (hdmi->audio_enable) {
>               mutex_unlock(&hdmi->audio_mutex);
>               return;
>        }
>        hdmi->audio_enable = true;
>        hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> 
>        mutex_unlock(&hdmi->audio_mutex);
> }

Yes, however, I prefer this kind of layout:

	mutex_lock(&hdmi->audio_mutex);
 
	if (!hdmi->audio_enable) {
		hdmi->audio_enable = true;
		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
			  HDMI_MC_CLKDIS);
 	}

	mutex_unlock(&hdmi->audio_mutex);

but that's a matter of personal opinion.  The important thing is that the
testing and setting of the flag are both within the protected region.

However, there are other bugs here: what if the audio driver is calling
the sample rate setting function at the same time that a mode switch is
occuring.  We actually need a mutex to protect more than just the
audio_enable flag.

> By the way, it doesn't matter that the function is called from another driver.
> What matters is that this function can be called concurrently on
> multiple different threads of execution to change the hdmi audio
> enable state.
> >From DRM land, it is called with DRM lock held when enabling/disabling
> hdmi audio (mode_set / DPMS).
> It is also called from audio land, when enabling/disabling audio in
> response to some audio events (userspace ioctls?).  I'm not sure
> exactly how the audio side works, or what locks are involved, but this
> mutex synchronizes calls from these two worlds to ensure that
> "hdmi->audio_enable" field always matches the current (intended)
> status of the hdmi audio clock.  This would be useful, for example, if
> you needed to temporarily disable all HDMI clocks during a mode set,
> and then restore the audio clock to its pre-mode_set state:

There's some rather quirky comments in the driver right now which make
me uneasy about changing things too much.

My initial idea would be that audio should remain disabled until the
audio driver wants it enabled, and the CTS/N values should either be
left alone, or set to a value which disables them (there is an iMX6
errata which says to set N=0 initially, but as seems common with iMX6
errata, I see no code implementing the method specified in the
documentation - I have found code implementing something similar
though.)

However, there is this in the binding function:

        /*
         * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
         * N and cts values before enabling phy
         */
        hdmi_init_clk_regenerator(hdmi);

which sets the N/CTS values assuming a 74.25MHz video clock and a 48kHz
sample rate.  I've always wondered why this is necessary (I haven't
experimented with that yet.)

Then there's this in the mode set function:

                /* HDMI Initialization Step E - Configure audio */
                hdmi_clk_regenerator_update_pixel_clock(hdmi);
                hdmi_enable_audio_clk(hdmi);

Where these "steps" come from, I've no idea (I can't find any documentation
which specifies them - maybe its from the Synopsis documentation?) but
this has always raised the question "what if audio is not enabled?"

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yang Kuankuan]

On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
>> Hi ykk,
>>
>> On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
>>> On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
>>>>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
>>>>> +{
>>>>> +       if (hdmi->audio_enable)
>>>>> +               return;
>>>>> +
>>>>> +       mutex_lock(&hdmi->audio_mutex);
>>>>> +       hdmi->audio_enable = true;
>>>>> +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>>>> HDMI_MC_CLKDIS);
>>>>> +       mutex_unlock(&hdmi->audio_mutex);
>>>> This is racy.  The test needs to be within the mutex-protected region.
>>> This function will be called by other driver (dw-hdmi-audio), both modify
>>> the variable "hdmi->audio_enable", so i add the mutex-protected.
>> >From your comment it isn't clear whether you understand what Russell meant.
>> He is say you should do the following:
>>
>> {
>>         mutex_lock(&hdmi->audio_mutex);
>>
>>         if (hdmi->audio_enable) {
>>                mutex_unlock(&hdmi->audio_mutex);
>>                return;
>>         }
>>         hdmi->audio_enable = true;
>>         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>>
>>         mutex_unlock(&hdmi->audio_mutex);
>> }
> Yes, however, I prefer this kind of layout:
>
> 	mutex_lock(&hdmi->audio_mutex);
>   
> 	if (!hdmi->audio_enable) {
> 		hdmi->audio_enable = true;
> 		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> 			  HDMI_MC_CLKDIS);
>   	}
>
> 	mutex_unlock(&hdmi->audio_mutex);
>
> but that's a matter of personal opinion.  The important thing is that the
> testing and setting of the flag are both within the protected region.
>
> However, there are other bugs here: what if the audio driver is calling
> the sample rate setting function at the same time that a mode switch is
> occuring.  We actually need a mutex to protect more than just the
> audio_enable flag.

Okay, got it.

Thanks.  : )
>> By the way, it doesn't matter that the function is called from another driver.
>> What matters is that this function can be called concurrently on
>> multiple different threads of execution to change the hdmi audio
>> enable state.
>> >From DRM land, it is called with DRM lock held when enabling/disabling
>> hdmi audio (mode_set / DPMS).
>> It is also called from audio land, when enabling/disabling audio in
>> response to some audio events (userspace ioctls?).  I'm not sure
>> exactly how the audio side works, or what locks are involved, but this
>> mutex synchronizes calls from these two worlds to ensure that
>> "hdmi->audio_enable" field always matches the current (intended)
>> status of the hdmi audio clock.  This would be useful, for example, if
>> you needed to temporarily disable all HDMI clocks during a mode set,
>> and then restore the audio clock to its pre-mode_set state:
> There's some rather quirky comments in the driver right now which make
> me uneasy about changing things too much.
>
> My initial idea would be that audio should remain disabled until the
> audio driver wants it enabled, and the CTS/N values should either be
> left alone, or set to a value which disables them (there is an iMX6
> errata which says to set N=0 initially, but as seems common with iMX6
> errata, I see no code implementing the method specified in the
> documentation - I have found code implementing something similar
> though.)

I am confused with the way that audio driver realize the display 
resolution-changing.
If audio driver cannot knowing that, then audio clock may be closed 
permanently ?

>
> However, there is this in the binding function:
>
>          /*
>           * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
>           * N and cts values before enabling phy
>           */
>          hdmi_init_clk_regenerator(hdmi);
>
> which sets the N/CTS values assuming a 74.25MHz video clock and a 48kHz
> sample rate.  I've always wondered why this is necessary (I haven't
> experimented with that yet.)
>
> Then there's this in the mode set function:
>
>                  /* HDMI Initialization Step E - Configure audio */
>                  hdmi_clk_regenerator_update_pixel_clock(hdmi);
>                  hdmi_enable_audio_clk(hdmi);
>
> Where these "steps" come from, I've no idea (I can't find any documentation
> which specifies them - maybe its from the Synopsis documentation?) but
> this has always raised the question "what if audio is not enabled?"
>
static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi)
{
     mutex_lock(&hdmi->audio_mutex);

     if (hdmi->audio_enable)
         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
                             HDMI_MC_CLKDIS);
     else
         hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
                             HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);

     mutex_unlock(&hdmi->audio_mutex);
}

      /* HDMI Initialization Step E - Configure audio */
     hdmi_clk_regenerator_update_pixel_clock(hdmi);
     hdmi_keep_audio_clk_status(hdmi);

keep audio status maybe suitable here.

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Russell King - ARM Linux]

On Mon, Feb 02, 2015 at 07:32:05AM -0500, Yang Kuankuan wrote:
> On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote:
> >On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
> >>Hi ykk,
> >>
> >>On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
> >>>On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
> >>>>>+void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
> >>>>>+{
> >>>>>+       if (hdmi->audio_enable)
> >>>>>+               return;
> >>>>>+
> >>>>>+       mutex_lock(&hdmi->audio_mutex);
> >>>>>+       hdmi->audio_enable = true;
> >>>>>+       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> >>>>>HDMI_MC_CLKDIS);
> >>>>>+       mutex_unlock(&hdmi->audio_mutex);
> >>>>This is racy.  The test needs to be within the mutex-protected region.
> >>>This function will be called by other driver (dw-hdmi-audio), both modify
> >>>the variable "hdmi->audio_enable", so i add the mutex-protected.
> >>>From your comment it isn't clear whether you understand what Russell meant.
> >>He is say you should do the following:
> >>
> >>{
> >>        mutex_lock(&hdmi->audio_mutex);
> >>
> >>        if (hdmi->audio_enable) {
> >>               mutex_unlock(&hdmi->audio_mutex);
> >>               return;
> >>        }
> >>        hdmi->audio_enable = true;
> >>        hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> >>
> >>        mutex_unlock(&hdmi->audio_mutex);
> >>}
> >Yes, however, I prefer this kind of layout:
> >
> >	mutex_lock(&hdmi->audio_mutex);
> >	if (!hdmi->audio_enable) {
> >		hdmi->audio_enable = true;
> >		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
> >			  HDMI_MC_CLKDIS);
> >  	}
> >
> >	mutex_unlock(&hdmi->audio_mutex);
> >
> >but that's a matter of personal opinion.  The important thing is that the
> >testing and setting of the flag are both within the protected region.
> >
> >However, there are other bugs here: what if the audio driver is calling
> >the sample rate setting function at the same time that a mode switch is
> >occuring.  We actually need a mutex to protect more than just the
> >audio_enable flag.
> 
> Okay, got it.
> 
> Thanks.  : )

I've been moving my code closer to what you have posted.  I've split up
your patches into smaller steps so each change can be evaluated on iMX6.
So far:

drm: bridge/dw_hdmi: combine hdmi_set_clock_regenerator_n() and hdmi_regenerate_cts()

  This patch _just_ combines the two functions without any other changes.

drm: bridge/dw_hdmi: protect n/cts setting with a mutex

  This patch _just_ adds a mutex to protect the calls to
  hdmi_set_clk_regenerator(), since we will need to call that from both
  the DRM and audio drivers.

drm: bridge/dw_hdmi: adjust n/cts setting order

  This patch changes the order to:
  - write CTS3 CTS_manual = 0
  - write CTS3 N_shift = 0
  - write CTS3 CTS value
  - write CTS2 CTS value
  - write CTS1 CTS value
  - write N3 N value
  - write N2 N value
  - write N1 N value
  which is broadly what you're doing, but without the initial N3 write
  and without CTS_manual=1.  I've asked Freescale whether they can
  clarify what effect adding those would have on their SoCs.

Note: the mutex in my second patch needs to be a spinlock - as it seems
my new workaround for iMX6 ERR005174 needs to call the CTS/N setting
functions in an irqs-off region (from the ALSA ->trigger callback.)
That will need further rework of the CTS/N code to reduce the size of
the spinlock protected region.

Incidentally, your Synopsis IP indicates that it is the same revision
as the iMX6's IP revision which suffers from this ERR005174 errata.
I think you need to check whether this errata applies on your SoC too.
Seach for "iMX6 ERR005174" in google.

> >>By the way, it doesn't matter that the function is called from another driver.
> >>What matters is that this function can be called concurrently on
> >>multiple different threads of execution to change the hdmi audio
> >>enable state.
> >>>From DRM land, it is called with DRM lock held when enabling/disabling
> >>hdmi audio (mode_set / DPMS).
> >>It is also called from audio land, when enabling/disabling audio in
> >>response to some audio events (userspace ioctls?).  I'm not sure
> >>exactly how the audio side works, or what locks are involved, but this
> >>mutex synchronizes calls from these two worlds to ensure that
> >>"hdmi->audio_enable" field always matches the current (intended)
> >>status of the hdmi audio clock.  This would be useful, for example, if
> >>you needed to temporarily disable all HDMI clocks during a mode set,
> >>and then restore the audio clock to its pre-mode_set state:
> >There's some rather quirky comments in the driver right now which make
> >me uneasy about changing things too much.
> >
> >My initial idea would be that audio should remain disabled until the
> >audio driver wants it enabled, and the CTS/N values should either be
> >left alone, or set to a value which disables them (there is an iMX6
> >errata which says to set N=0 initially, but as seems common with iMX6
> >errata, I see no code implementing the method specified in the
> >documentation - I have found code implementing something similar
> >though.)
> 
> I am confused with the way that audio driver realize the display
> resolution-changing.
> If audio driver cannot knowing that, then audio clock may be closed
> permanently ?

The audio driver shouldn't care about the display resolution.  That
should be the responsibility of the dw_hdmi core - as it is at the
moment.

> static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi)
> {
>     mutex_lock(&hdmi->audio_mutex);
> 
>     if (hdmi->audio_enable)
>         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>                             HDMI_MC_CLKDIS);
>     else
>         hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>                             HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
> 
>     mutex_unlock(&hdmi->audio_mutex);
> }
> 
>      /* HDMI Initialization Step E - Configure audio */
>     hdmi_clk_regenerator_update_pixel_clock(hdmi);
>     hdmi_keep_audio_clk_status(hdmi);
> 
> keep audio status maybe suitable here.

What I don't know right now is what triggers this overflow indication in
HDMI_IH_FC_STAT2, and whether the code I quoted (which fakes the audio
setup) is actually necessary.

In other words:
- is it necessary to have the audio clock enabled when video is enabled
  to prevent the overflow indication?  We don't know.  Therefore, we
  can't say whether it is permitted to disable the audio clock when
  audio is inactive.
- is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz,
  or can we program a non-zero CTS value and a zero N at initialisation
  until the audio driver comes up?  Again, we don't know.

What we do know is that as the driver stands, it works for video output.
With my changes for AHB audio support on the iMX6 (which includes some
errata workarounds found in the iMX6 errata documentation to avoid FIFO
issues), we have working audio there.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yang Kuankuan]

On 02/02/2015 08:09 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 02, 2015 at 07:32:05AM -0500, Yang Kuankuan wrote:
>> On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote:
>>> On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
>>>> Hi ykk,
>>>>
>>>> On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
>>>>> On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
>>>>>>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
>>>>>>> +{
>>>>>>> +       if (hdmi->audio_enable)
>>>>>>> +               return;
>>>>>>> +
>>>>>>> +       mutex_lock(&hdmi->audio_mutex);
>>>>>>> +       hdmi->audio_enable = true;
>>>>>>> +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>>>>>> HDMI_MC_CLKDIS);
>>>>>>> +       mutex_unlock(&hdmi->audio_mutex);
>>>>>> This is racy.  The test needs to be within the mutex-protected region.
>>>>> This function will be called by other driver (dw-hdmi-audio), both modify
>>>>> the variable "hdmi->audio_enable", so i add the mutex-protected.
>>>> >From your comment it isn't clear whether you understand what Russell meant.
>>>> He is say you should do the following:
>>>>
>>>> {
>>>>         mutex_lock(&hdmi->audio_mutex);
>>>>
>>>>         if (hdmi->audio_enable) {
>>>>                mutex_unlock(&hdmi->audio_mutex);
>>>>                return;
>>>>         }
>>>>         hdmi->audio_enable = true;
>>>>         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>>>>
>>>>         mutex_unlock(&hdmi->audio_mutex);
>>>> }
>>> Yes, however, I prefer this kind of layout:
>>>
>>> 	mutex_lock(&hdmi->audio_mutex);
>>> 	if (!hdmi->audio_enable) {
>>> 		hdmi->audio_enable = true;
>>> 		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>> 			  HDMI_MC_CLKDIS);
>>>   	}
>>>
>>> 	mutex_unlock(&hdmi->audio_mutex);
>>>
>>> but that's a matter of personal opinion.  The important thing is that the
>>> testing and setting of the flag are both within the protected region.
>>>
>>> However, there are other bugs here: what if the audio driver is calling
>>> the sample rate setting function at the same time that a mode switch is
>>> occuring.  We actually need a mutex to protect more than just the
>>> audio_enable flag.
>> Okay, got it.
>>
>> Thanks.  : )
> I've been moving my code closer to what you have posted.  I've split up
> your patches into smaller steps so each change can be evaluated on iMX6.
> So far:

Thank you very much.  : )

>
> drm: bridge/dw_hdmi: combine hdmi_set_clock_regenerator_n() and hdmi_regenerate_cts()
>
>    This patch _just_ combines the two functions without any other changes.
>
> drm: bridge/dw_hdmi: protect n/cts setting with a mutex
>
>    This patch _just_ adds a mutex to protect the calls to
>    hdmi_set_clk_regenerator(), since we will need to call that from both
>    the DRM and audio drivers.
>
> drm: bridge/dw_hdmi: adjust n/cts setting order
>
>    This patch changes the order to:
>    - write CTS3 CTS_manual = 0
>    - write CTS3 N_shift = 0
>    - write CTS3 CTS value
>    - write CTS2 CTS value
>    - write CTS1 CTS value
>    - write N3 N value
>    - write N2 N value
>    - write N1 N value
>    which is broadly what you're doing, but without the initial N3 write
>    and without CTS_manual=1.  I've asked Freescale whether they can
>    clarify what effect adding those would have on their SoCs.
>
> Note: the mutex in my second patch needs to be a spinlock - as it seems
> my new workaround for iMX6 ERR005174 needs to call the CTS/N setting
> functions in an irqs-off region (from the ALSA ->trigger callback.)
> That will need further rework of the CTS/N code to reduce the size of
> the spinlock protected region.
>
> Incidentally, your Synopsis IP indicates that it is the same revision
> as the iMX6's IP revision which suffers from this ERR005174 errata.
> I think you need to check whether this errata applies on your SoC too.
> Seach for "iMX6 ERR005174" in google.

>>>> By the way, it doesn't matter that the function is called from another driver.
>>>> What matters is that this function can be called concurrently on
>>>> multiple different threads of execution to change the hdmi audio
>>>> enable state.
>>>> >From DRM land, it is called with DRM lock held when enabling/disabling
>>>> hdmi audio (mode_set / DPMS).
>>>> It is also called from audio land, when enabling/disabling audio in
>>>> response to some audio events (userspace ioctls?).  I'm not sure
>>>> exactly how the audio side works, or what locks are involved, but this
>>>> mutex synchronizes calls from these two worlds to ensure that
>>>> "hdmi->audio_enable" field always matches the current (intended)
>>>> status of the hdmi audio clock.  This would be useful, for example, if
>>>> you needed to temporarily disable all HDMI clocks during a mode set,
>>>> and then restore the audio clock to its pre-mode_set state:
>>> There's some rather quirky comments in the driver right now which make
>>> me uneasy about changing things too much.
>>>
>>> My initial idea would be that audio should remain disabled until the
>>> audio driver wants it enabled, and the CTS/N values should either be
>>> left alone, or set to a value which disables them (there is an iMX6
>>> errata which says to set N=0 initially, but as seems common with iMX6
>>> errata, I see no code implementing the method specified in the
>>> documentation - I have found code implementing something similar
>>> though.)
>> I am confused with the way that audio driver realize the display
>> resolution-changing.
>> If audio driver cannot knowing that, then audio clock may be closed
>> permanently ?
> The audio driver shouldn't care about the display resolution.  That
> should be the responsibility of the dw_hdmi core - as it is at the
> moment.
Do you mean that we should disable audio clock and deinit
the n/cts values, until we meet the audio enable single like this.

     if (hdmi->vmode.mdiv) {
         /* HDMI Initialization Step E - Configure audio */
         hdmi_clk_regenerator_update_pixel_clock(hdmi);
         hdmi_enable_audio_clk(hdmi);
     }

>> static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi)
>> {
>>      mutex_lock(&hdmi->audio_mutex);
>>
>>      if (hdmi->audio_enable)
>>          hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>                              HDMI_MC_CLKDIS);
>>      else
>>          hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>                              HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>>
>>      mutex_unlock(&hdmi->audio_mutex);
>> }
>>
>>       /* HDMI Initialization Step E - Configure audio */
>>      hdmi_clk_regenerator_update_pixel_clock(hdmi);
>>      hdmi_keep_audio_clk_status(hdmi);
>>
>> keep audio status maybe suitable here.
> What I don't know right now is what triggers this overflow indication in
> HDMI_IH_FC_STAT2, and whether the code I quoted (which fakes the audio
> setup) is actually necessary.
>
> In other words:
> - is it necessary to have the audio clock enabled when video is enabled
>    to prevent the overflow indication?  We don't know.  Therefore, we
>    can't say whether it is permitted to disable the audio clock when
>    audio is inactive.
> - is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz,
>    or can we program a non-zero CTS value and a zero N at initialisation
>    until the audio driver comes up?  Again, we don't know.
>
> What we do know is that as the driver stands, it works for video output.
> With my changes for AHB audio support on the iMX6 (which includes some
> errata workarounds found in the iMX6 errata documentation to avoid FIFO
> issues), we have working audio there.
>
I don't know the effect of overflow indication in HDMI_IH_FC_STAT2, seems
the irq function have not handle the FC_STAT2 interrupt in dw_hdmi driver,
also not found in your dw-hdmi-audio driver.

But I will talk with our IC colleges,
- is it necessary to have the audio clock enabled when video is enabled to
   prevent the overflow indication ?
       If it is not necessary, maybe we can keep the audio status in 
mode_set.

- Also is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz,
   or can we program a non-zero CTS value and a zero N at initialisation 
until
   the audio driver comes up

>>>>>>	However, there is this in the binding function:
>>>>>>
>>>>>>   * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator
>>>>>>   * N and cts values before enabling phy
>>>>>>   */
>>>>>>   hdmi_init_clk_regenerator(hdmi);

>>>>>>  which sets the N/CTS values assuming a 74.25MHz video clock and a 48kHz
>>>>>>  sample rate.  I've always wondered why this is necessary (I haven't
>>>>>>  experimented with that yet.)

>>>>>>  Then there's this in the mode set function:

>>>>>>  	/* HDMI Initialization Step E - Configure audio */
>>>>>>		hdmi_clk_regenerator_update_pixel_clock(hdmi);
>>>>>>		hdmi_enable_audio_clk(hdmi);

>>>>>>	Where these "steps" come from, I've no idea (I can't find any documentation
>>>>>>	which specifies them - maybe its from the Synopsis documentation?) but
>>>>>>	this has always raised the question "what if audio is not enabled?"

Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Yang Kuankuan]

On 02/02/2015 08:09 AM, Russell King - ARM Linux wrote:
> On Mon, Feb 02, 2015 at 07:32:05AM -0500, Yang Kuankuan wrote:
>> On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote:
>>> On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote:
>>>> Hi ykk,
>>>>
>>>> On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <ykk@rock-chips.com> wrote:
>>>>> On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote:
>>>>>>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi)
>>>>>>> +{
>>>>>>> +       if (hdmi->audio_enable)
>>>>>>> +               return;
>>>>>>> +
>>>>>>> +       mutex_lock(&hdmi->audio_mutex);
>>>>>>> +       hdmi->audio_enable = true;
>>>>>>> +       hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>>>>>> HDMI_MC_CLKDIS);
>>>>>>> +       mutex_unlock(&hdmi->audio_mutex);
>>>>>> This is racy.  The test needs to be within the mutex-protected region.
>>>>> This function will be called by other driver (dw-hdmi-audio), both modify
>>>>> the variable "hdmi->audio_enable", so i add the mutex-protected.
>>>> >From your comment it isn't clear whether you understand what Russell meant.
>>>> He is say you should do the following:
>>>>
>>>> {
>>>>         mutex_lock(&hdmi->audio_mutex);
>>>>
>>>>         if (hdmi->audio_enable) {
>>>>                mutex_unlock(&hdmi->audio_mutex);
>>>>                return;
>>>>         }
>>>>         hdmi->audio_enable = true;
>>>>         hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>>>>
>>>>         mutex_unlock(&hdmi->audio_mutex);
>>>> }
>>> Yes, however, I prefer this kind of layout:
>>>
>>> 	mutex_lock(&hdmi->audio_mutex);
>>> 	if (!hdmi->audio_enable) {
>>> 		hdmi->audio_enable = true;
>>> 		hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>> 			  HDMI_MC_CLKDIS);
>>>   	}
>>>
>>> 	mutex_unlock(&hdmi->audio_mutex);
>>>
>>> but that's a matter of personal opinion.  The important thing is that the
>>> testing and setting of the flag are both within the protected region.
>>>
>>> However, there are other bugs here: what if the audio driver is calling
>>> the sample rate setting function at the same time that a mode switch is
>>> occuring.  We actually need a mutex to protect more than just the
>>> audio_enable flag.
>> Okay, got it.
>>
>> Thanks.  : )
> I've been moving my code closer to what you have posted.  I've split up
> your patches into smaller steps so each change can be evaluated on iMX6.
> So far:
>
> drm: bridge/dw_hdmi: combine hdmi_set_clock_regenerator_n() and hdmi_regenerate_cts()
>
>    This patch _just_ combines the two functions without any other changes.
>
> drm: bridge/dw_hdmi: protect n/cts setting with a mutex
>
>    This patch _just_ adds a mutex to protect the calls to
>    hdmi_set_clk_regenerator(), since we will need to call that from both
>    the DRM and audio drivers.
>
> drm: bridge/dw_hdmi: adjust n/cts setting order
>
>    This patch changes the order to:
>    - write CTS3 CTS_manual = 0
>    - write CTS3 N_shift = 0
>    - write CTS3 CTS value
>    - write CTS2 CTS value
>    - write CTS1 CTS value
>    - write N3 N value
>    - write N2 N value
>    - write N1 N value
>    which is broadly what you're doing, but without the initial N3 write
>    and without CTS_manual=1.  I've asked Freescale whether they can
>    clarify what effect adding those would have on their SoCs.
>
> Note: the mutex in my second patch needs to be a spinlock - as it seems
> my new workaround for iMX6 ERR005174 needs to call the CTS/N setting
> functions in an irqs-off region (from the ALSA ->trigger callback.)
> That will need further rework of the CTS/N code to reduce the size of
> the spinlock protected region.
>
> Incidentally, your Synopsis IP indicates that it is the same revision
> as the iMX6's IP revision which suffers from this ERR005174 errata.
> I think you need to check whether this errata applies on your SoC too.
> Seach for "iMX6 ERR005174" in google.
Thank you very much.  : )
I will take this order in next v3.
>>>> By the way, it doesn't matter that the function is called from another driver.
>>>> What matters is that this function can be called concurrently on
>>>> multiple different threads of execution to change the hdmi audio
>>>> enable state.
>>>> >From DRM land, it is called with DRM lock held when enabling/disabling
>>>> hdmi audio (mode_set / DPMS).
>>>> It is also called from audio land, when enabling/disabling audio in
>>>> response to some audio events (userspace ioctls?).  I'm not sure
>>>> exactly how the audio side works, or what locks are involved, but this
>>>> mutex synchronizes calls from these two worlds to ensure that
>>>> "hdmi->audio_enable" field always matches the current (intended)
>>>> status of the hdmi audio clock.  This would be useful, for example, if
>>>> you needed to temporarily disable all HDMI clocks during a mode set,
>>>> and then restore the audio clock to its pre-mode_set state:
>>> There's some rather quirky comments in the driver right now which make
>>> me uneasy about changing things too much.
>>>
>>> My initial idea would be that audio should remain disabled until the
>>> audio driver wants it enabled, and the CTS/N values should either be
>>> left alone, or set to a value which disables them (there is an iMX6
>>> errata which says to set N=0 initially, but as seems common with iMX6
>>> errata, I see no code implementing the method specified in the
>>> documentation - I have found code implementing something similar
>>> though.)
>> I am confused with the way that audio driver realize the display
>> resolution-changing.
>> If audio driver cannot knowing that, then audio clock may be closed
>> permanently ?
> The audio driver shouldn't care about the display resolution.  That
> should be the responsibility of the dw_hdmi core - as it is at the
> moment.
Do you mean that we should disable audio clock and deinit
the n/cts values, until we meet the audio enable single like this.



>> static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi)
>> {
>>      mutex_lock(&hdmi->audio_mutex);
>>
>>      if (hdmi->audio_enable)
>>          hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>                              HDMI_MC_CLKDIS);
>>      else
>>          hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,
>>                              HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
>>
>>      mutex_unlock(&hdmi->audio_mutex);
>> }
>>
>>       /* HDMI Initialization Step E - Configure audio */
>>      hdmi_clk_regenerator_update_pixel_clock(hdmi);
>>      hdmi_keep_audio_clk_status(hdmi);
>>
>> keep audio status maybe suitable here.
> What I don't know right now is what triggers this overflow indication in
> HDMI_IH_FC_STAT2, and whether the code I quoted (which fakes the audio
> setup) is actually necessary.
>
> In other words:
> - is it necessary to have the audio clock enabled when video is enabled
>    to prevent the overflow indication?  We don't know.  Therefore, we
>    can't say whether it is permitted to disable the audio clock when
>    audio is inactive.
> - is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz,
>    or can we program a non-zero CTS value and a zero N at initialisation
>    until the audio driver comes up?  Again, we don't know.
>
> What we do know is that as the driver stands, it works for video output.
> With my changes for AHB audio support on the iMX6 (which includes some
> errata workarounds found in the iMX6 errata documentation to avoid FIFO
> issues), we have working audio there.
>

[PATCH v2 09/12] drm: bridge/dw_hdmi: creat dw-hdmi-audio platform device

[Yakir Yang]

creat dw-hdmi-audio device dynamically in probe function,
and transfer some interfaces to dw-hdmi-audio driver for
setting hdmi audio format & control hdmi audio clock.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Update the audio control interfaces

 drivers/gpu/drm/bridge/dw_hdmi.c | 23 +++++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h     | 13 +++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 25c1678..75e3029 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -107,6 +107,7 @@ struct dw_hdmi {
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
 
+	struct platform_device *audio_pdev;
 	enum dw_hdmi_devtype dev_type;
 	struct device *dev;
 	struct clk *isfr_clk;
@@ -1703,6 +1704,8 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 		 struct resource *iores, int irq,
 		 const struct dw_hdmi_plat_data *plat_data)
 {
+	struct platform_device_info pdevinfo;
+	struct dw_hdmi_audio_data audio;
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
 	struct device_node *ddc_node;
@@ -1824,6 +1827,23 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 
 	dev_set_drvdata(dev, hdmi);
 
+	memset(&pdevinfo, 0, sizeof(pdevinfo));
+	pdevinfo.parent = dev;
+	pdevinfo.id = PLATFORM_DEVID_NONE;
+
+	audio.irq = irq;
+	audio.dw = hdmi;
+	audio.config_audio = hdmi_config_audio;
+	audio.enable_clk = hdmi_audio_clk_enable;
+	audio.disable_clk = hdmi_audio_clk_disable;
+	audio.get_connect_status = hdmi_get_connect_status;
+
+	pdevinfo.name = "dw-hdmi-audio";
+	pdevinfo.data = &audio;
+	pdevinfo.size_data = sizeof(audio);
+	pdevinfo.dma_mask = DMA_BIT_MASK(32);
+	hdmi->audio_pdev = platform_device_register_full(&pdevinfo);
+
 	return 0;
 
 err_iahb:
@@ -1839,6 +1859,9 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 {
 	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
 
+	if (!IS_ERR(hdmi->audio_pdev))
+		platform_device_unregister(hdmi->audio_pdev);
+
 	/* Disable all interrupts */
 	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index aafa447..2f29112 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -12,6 +12,8 @@
 
 #include <drm/drmP.h>
 
+struct dw_hdmi;
+
 enum {
 	DW_HDMI_RES_8,
 	DW_HDMI_RES_10,
@@ -74,6 +76,17 @@ struct hdmi_audio_fmt {
 	enum hdmi_audio_wordlength word_length;
 };
 
+struct dw_hdmi_audio_data {
+	struct dw_hdmi *dw;
+
+	int irq;
+	bool (*get_connect_status)(struct dw_hdmi *hdmi);
+
+	void (*enable_clk)(struct dw_hdmi *hdmi);
+	void (*disable_clk)(struct dw_hdmi *hdmi);
+	void (*config_audio)(struct dw_hdmi *hdmi, struct hdmi_audio_fmt *aud);
+};
+
 struct dw_hdmi_plat_data {
 	enum dw_hdmi_devtype dev_type;
 	const struct dw_hdmi_mpll_config *mpll_cfg;
-- 
2.1.2

[PATCH v2 10/12] ASoC: dw-hdmi-audio: add codec driver for dw hdmi audio

[Yakir Yang]

codec driver creat an standard alsa device, than config audio
and report jack status through some callback interfaces that
dw_hdmi driver support.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- Update dw_hdmi audio control interfaces, and adjust jack report process

 sound/soc/codecs/Kconfig         |   4 +
 sound/soc/codecs/Makefile        |   2 +
 sound/soc/codecs/dw-hdmi-audio.c | 314 +++++++++++++++++++++++++++++++++++++++
 sound/soc/codecs/dw-hdmi-audio.h |  17 +++
 4 files changed, 337 insertions(+)
 create mode 100644 sound/soc/codecs/dw-hdmi-audio.c
 create mode 100644 sound/soc/codecs/dw-hdmi-audio.h

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 8349f98..b34dd12 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -75,6 +75,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_MC13783 if MFD_MC13XXX
 	select SND_SOC_ML26124 if I2C
 	select SND_SOC_HDMI_CODEC
+	select SND_SOC_DW_HDMI_AUDIO
 	select SND_SOC_PCM1681 if I2C
 	select SND_SOC_PCM1792A if SPI_MASTER
 	select SND_SOC_PCM3008
@@ -459,6 +460,9 @@ config SND_SOC_MAX98095
 config SND_SOC_MAX9850
 	tristate
 
+config SND_SOC_DW_HDMI_AUDIO
+       tristate
+
 config SND_SOC_PCM1681
 	tristate "Texas Instruments PCM1681 CODEC"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bbdfd1e..0ebb664 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -68,6 +68,7 @@ snd-soc-max9850-objs := max9850.o
 snd-soc-mc13783-objs := mc13783.o
 snd-soc-ml26124-objs := ml26124.o
 snd-soc-hdmi-codec-objs := hdmi.o
+snd-soc-dw-hdmi-audio-objs := dw-hdmi-audio.o
 snd-soc-pcm1681-objs := pcm1681.o
 snd-soc-pcm1792a-codec-objs := pcm1792a.o
 snd-soc-pcm3008-objs := pcm3008.o
@@ -249,6 +250,7 @@ obj-$(CONFIG_SND_SOC_MAX9850)	+= snd-soc-max9850.o
 obj-$(CONFIG_SND_SOC_MC13783)	+= snd-soc-mc13783.o
 obj-$(CONFIG_SND_SOC_ML26124)	+= snd-soc-ml26124.o
 obj-$(CONFIG_SND_SOC_HDMI_CODEC) += snd-soc-hdmi-codec.o
+obj-$(CONFIG_SND_SOC_DW_HDMI_AUDIO) += snd-soc-dw-hdmi-audio.o
 obj-$(CONFIG_SND_SOC_PCM1681)	+= snd-soc-pcm1681.o
 obj-$(CONFIG_SND_SOC_PCM1792A)	+= snd-soc-pcm1792a-codec.o
 obj-$(CONFIG_SND_SOC_PCM3008)	+= snd-soc-pcm3008.o
diff --git a/sound/soc/codecs/dw-hdmi-audio.c b/sound/soc/codecs/dw-hdmi-audio.c
new file mode 100644
index 0000000..ed046ce
--- /dev/null
+++ b/sound/soc/codecs/dw-hdmi-audio.c
@@ -0,0 +1,314 @@
+/*
+ * dw-hdmi-codec.c
+ *
+ * DesignerWare ALSA SoC DAI driver for DW HDMI audio.
+ * Copyright (c) 2014,  CORPORATION. All rights reserved.
+ * Authors: Yakir Yang <ykk@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.*
+ *
+ */
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/moduleparam.h>
+
+#include <sound/pcm.h>
+#include <sound/soc.h>
+#include <sound/core.h>
+#include <sound/jack.h>
+#include <sound/initval.h>
+#include <sound/pcm_params.h>
+
+#include <drm/bridge/dw_hdmi.h>
+#include "dw-hdmi-audio.h"
+
+struct snd_dw_hdmi {
+	struct device *dev;
+	struct dw_hdmi_audio_data data;
+
+	u8 jack_status;
+	struct snd_soc_jack *jack;
+	struct delayed_work jack_work;
+};
+
+/**
+ * dw_hdmi_jack_detect - Enable hdmi detection via the HDMI IRQ
+ *
+ * @codec:  HDMI codec
+ * @jack:   jack to report detection events on
+ *
+ * Enable HDMI detection via IRQ on the HDMI.
+ *
+ * If no jack is supplied detection will be disabled.
+ */
+int dw_hdmi_jack_detect(struct snd_soc_codec *codec_dai,
+			struct snd_soc_jack *jack)
+{
+	struct snd_dw_hdmi *hdmi = snd_soc_codec_get_drvdata(codec_dai);
+
+	hdmi->jack = jack;
+	hdmi->jack_status = 0;
+
+	snd_soc_jack_report(hdmi->jack, 0, SND_JACK_LINEOUT);
+
+	queue_delayed_work(system_power_efficient_wq, &hdmi->jack_work,
+			   msecs_to_jiffies(50));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_jack_detect);
+
+static void dw_hdmi_jack_work(struct work_struct *work)
+{
+	struct snd_dw_hdmi *hdmi = container_of(work, struct snd_dw_hdmi,
+						jack_work.work);
+	u8 jack_status, status;
+
+	status = hdmi->data.get_connect_status(hdmi->data.dw);
+	jack_status = status ? SND_JACK_LINEOUT : 0;
+	if (jack_status != hdmi->jack_status) {
+		snd_soc_jack_report(hdmi->jack, jack_status,
+				    SND_JACK_LINEOUT);
+		hdmi->jack_status = jack_status;
+
+		dev_dbg(hdmi->dev, "jack report [%d]\n", hdmi->jack_status);
+	}
+}
+
+static irqreturn_t snd_dw_hdmi_irq(int irq, void *dev_id)
+{
+	struct snd_dw_hdmi *hdmi = dev_id;
+
+	queue_delayed_work(system_power_efficient_wq, &hdmi->jack_work,
+			   msecs_to_jiffies(50));
+
+	return IRQ_HANDLED;
+}
+
+static int dw_hdmi_dai_hw_params(struct snd_pcm_substream *substream,
+				 struct snd_pcm_hw_params *params,
+				 struct snd_soc_dai *codec_dai)
+{
+	struct snd_dw_hdmi *hdmi = snd_soc_dai_get_drvdata(codec_dai);
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct hdmi_audio_fmt hdmi_fmt;
+	unsigned int fmt, rate, chan, width;
+
+	fmt = rtd->dai_link->dai_fmt & SND_SOC_DAIFMT_FORMAT_MASK;
+	switch (fmt) {
+	case SND_SOC_DAIFMT_I2S:
+		hdmi_fmt.dai_fmt = AUDIO_DAIFMT_IIS;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		hdmi_fmt.dai_fmt = AUDIO_DAIFMT_LEFT_J;
+		break;
+	case SND_SOC_DAIFMT_RIGHT_J:
+		hdmi_fmt.dai_fmt = AUDIO_DAIFMT_RIGHT_J;
+		break;
+	default:
+		dev_err(codec_dai->dev, "DAI format unsupported");
+		return -EINVAL;
+	}
+	dev_dbg(codec_dai->dev, "[codec_dai]: dai_fmt = %d.\n", fmt);
+
+	width = params_width(params);
+	switch (width) {
+	case 16:
+	case 24:
+		hdmi_fmt.word_length = width;
+		break;
+	default:
+		dev_err(codec_dai->dev, "width[%d] not support!\n", width);
+		return -EINVAL;
+	}
+	dev_dbg(codec_dai->dev, "[codec_dai]: word_length = %d.\n", width);
+
+	chan = params_channels(params);
+	switch (chan) {
+	case 2:
+		hdmi_fmt.chan_num = AUDIO_CHANNELNUM_2;
+		break;
+	case 4:
+		hdmi_fmt.chan_num = AUDIO_CHANNELNUM_4;
+		break;
+	case 6:
+		hdmi_fmt.chan_num = AUDIO_CHANNELNUM_6;
+		break;
+	case 8:
+		hdmi_fmt.chan_num = AUDIO_CHANNELNUM_8;
+		break;
+	default:
+		dev_err(codec_dai->dev, "channel[%d] not support!\n", chan);
+		return -EINVAL;
+	}
+	dev_dbg(codec_dai->dev, "[codec_dai]: chan_num = %d.\n", chan);
+
+	rate = params_rate(params);
+	switch (rate) {
+	case 32000:
+	case 44100:
+	case 48000:
+	case 88200:
+	case 96000:
+	case 176400:
+	case 192000:
+		hdmi_fmt.sample_rate = rate;
+		break;
+	default:
+		dev_err(codec_dai->dev, "rate[%d] not support!\n", rate);
+		return -EINVAL;
+	}
+	dev_dbg(codec_dai->dev, "[codec_dai]: sample_rate = %d.\n", rate);
+
+	hdmi->data.config_audio(hdmi->data.dw, &hdmi_fmt);
+
+	return 0;
+}
+
+static int dw_hdmi_dai_trigger(struct snd_pcm_substream *substream,
+			       int cmd, struct snd_soc_dai *codec_dai)
+{
+	struct snd_dw_hdmi *hdmi = snd_soc_dai_get_drvdata(codec_dai);
+
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_START:
+	case SNDRV_PCM_TRIGGER_RESUME:
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		hdmi->data.enable_clk(hdmi->data.dw);
+		dev_dbg(codec_dai->dev, "[codec_dai]: trigger enable.\n");
+		break;
+
+	case SNDRV_PCM_TRIGGER_STOP:
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+		hdmi->data.disable_clk(hdmi->data.dw);
+		dev_dbg(codec_dai->dev, "[codec_dai]: trigger disable.\n");
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_dapm_widget dw_hdmi_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("TX"),
+};
+
+static const struct snd_soc_dapm_route dw_hdmi_routes[] = {
+	{ "TX", NULL, "Playback" },
+};
+
+static const struct snd_soc_dai_ops dw_hdmi_dai_ops = {
+	.hw_params = dw_hdmi_dai_hw_params,
+	.trigger = dw_hdmi_dai_trigger,
+};
+
+static struct snd_soc_dai_driver dw_hdmi_audio_dai = {
+	.name = "dw-hdmi-hifi",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 8,
+		.rates = SNDRV_PCM_RATE_32000 |
+			 SNDRV_PCM_RATE_44100 | SNDRV_PCM_RATE_48000 |
+			 SNDRV_PCM_RATE_88200 | SNDRV_PCM_RATE_96000 |
+			 SNDRV_PCM_RATE_176400 | SNDRV_PCM_RATE_192000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
+	},
+	.ops = &dw_hdmi_dai_ops,
+};
+
+static const struct snd_soc_codec_driver dw_hdmi_audio = {
+	.dapm_widgets = dw_hdmi_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(dw_hdmi_widgets),
+	.dapm_routes = dw_hdmi_routes,
+	.num_dapm_routes = ARRAY_SIZE(dw_hdmi_routes),
+};
+
+static int dw_hdmi_audio_probe(struct platform_device *pdev)
+{
+	struct dw_hdmi_audio_data *data = pdev->dev.platform_data;
+	struct snd_dw_hdmi *hdmi;
+	int ret;
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+
+	hdmi->data = *data;
+	hdmi->dev = &pdev->dev;
+	platform_set_drvdata(pdev, hdmi);
+
+	INIT_DELAYED_WORK(&hdmi->jack_work, dw_hdmi_jack_work);
+
+	ret = devm_request_irq(&pdev->dev, hdmi->data.irq, snd_dw_hdmi_irq,
+			       IRQF_SHARED, "dw-hdmi-audio", hdmi);
+	if (ret) {
+		dev_err(&pdev->dev, "request irq failed (%d)\n", ret);
+		goto free_hdmi_data;
+	}
+
+	ret = snd_soc_register_codec(&pdev->dev, &dw_hdmi_audio,
+				     &dw_hdmi_audio_dai, 1);
+	if (ret) {
+		dev_err(&pdev->dev, "register codec failed (%d)\n", ret);
+		goto free_irq;
+	}
+
+	dev_info(&pdev->dev, "hdmi audio init success.\n");
+
+	return 0;
+
+free_irq:
+	devm_free_irq(&pdev->dev, hdmi->data.irq, hdmi);
+free_hdmi_data:
+	devm_kfree(&pdev->dev, hdmi);
+
+	return ret;
+}
+
+static int dw_hdmi_audio_remove(struct platform_device *pdev)
+{
+	struct snd_dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	snd_soc_unregister_codec(&pdev->dev);
+	devm_free_irq(&pdev->dev, hdmi->data.irq, hdmi);
+	devm_kfree(&pdev->dev, hdmi);
+
+	return 0;
+}
+
+static const struct of_device_id dw_hdmi_audio_ids[] = {
+	{ .compatible = "dw-hdmi-audio", },
+	{ }
+};
+
+static struct platform_driver dw_hdmi_audio_driver = {
+	.driver = {
+		.name = "dw-hdmi-audio",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(dw_hdmi_audio_ids),
+	},
+	.probe = dw_hdmi_audio_probe,
+	.remove = dw_hdmi_audio_remove,
+};
+module_platform_driver(dw_hdmi_audio_driver);
+
+MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
+MODULE_DESCRIPTION("DW HDMI Audio ASoC Interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" "dw-hdmi-audio");
+MODULE_DEVICE_TABLE(of, dw_hdmi_audio_ids);
diff --git a/sound/soc/codecs/dw-hdmi-audio.h b/sound/soc/codecs/dw-hdmi-audio.h
new file mode 100644
index 0000000..2a7b5b4
--- /dev/null
+++ b/sound/soc/codecs/dw-hdmi-audio.h
@@ -0,0 +1,17 @@
+/*
+ * dw-hdmi-audio.h -- DW HDMI ALSA SoC Audio driver
+ *
+ * Copyright 2011-2012 DesignerWare Products
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DW_HDMI_AUDIO_H
+#define _DW_HDMI_AUDIO_H
+
+int dw_hdmi_jack_detect(struct snd_soc_codec *codec_dai,
+			struct snd_soc_jack *jack);
+
+#endif
-- 
2.1.2

Re: [PATCH v2 10/12] ASoC: dw-hdmi-audio: add codec driver for dw hdmi audio

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:41:11AM -0500, Yakir Yang wrote:
> codec driver creat an standard alsa device, than config audio
> and report jack status through some callback interfaces that
> dw_hdmi driver support.
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v2:
> - Update dw_hdmi audio control interfaces, and adjust jack report process

As you are aware, this can't work with iMX6 SoCs.  We need to come up with
some way to deal with the different IP cores in a sane way.

The "codec" part of this IP is different between the two implementations;
your ASoC codec driver is not useful on the iMX6 SoC implementation.

We need to find a way to allow these two implementations to co-exist
sanely.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH v2 11/12] ASoC: rockchip-hdmi-audio: add sound driver for hdmi audio

[Yakir Yang]

Add a sound driver that combines rockchip-i2s cpu_dai and dw-hdmi-codec
as codec_dai to provide hdmi audio output on rk3288 platforms.

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- give "codec-name" & "codec-dai-name" an const name

 sound/soc/rockchip/Kconfig               |   9 ++
 sound/soc/rockchip/Makefile              |   2 +
 sound/soc/rockchip/rockchip_hdmi_audio.c | 196 +++++++++++++++++++++++++++++++
 3 files changed, 207 insertions(+)
 create mode 100644 sound/soc/rockchip/rockchip_hdmi_audio.c

diff --git a/sound/soc/rockchip/Kconfig b/sound/soc/rockchip/Kconfig
index e181826..ed2b7f0 100644
--- a/sound/soc/rockchip/Kconfig
+++ b/sound/soc/rockchip/Kconfig
@@ -14,3 +14,12 @@ config SND_SOC_ROCKCHIP_I2S
 	  Say Y or M if you want to add support for I2S driver for
 	  Rockchip I2S device. The device supports upto maximum of
 	  8 channels each for play and record.
+
+config SND_SOC_ROCKCHIP_HDMI_AUDIO
+	tristate "ASoC support for Rockchip HDMI audio"
+	depends on SND_SOC_ROCKCHIP
+	select SND_SOC_ROCKCHIP_I2S
+	select SND_SOC_DW_HDMI_AUDIO
+	help
+	  Say Y or M here if you want to add support for SoC audio on Rockchip
+	  HDMI, such as rk3288 hdmi.
diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile
index b921909..b9185b3 100644
--- a/sound/soc/rockchip/Makefile
+++ b/sound/soc/rockchip/Makefile
@@ -1,4 +1,6 @@
 # ROCKCHIP Platform Support
 snd-soc-i2s-objs := rockchip_i2s.o
+snd-soc-rockchip-hdmi-audio-objs := rockchip_hdmi_audio.o
 
 obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-i2s.o
+obj-$(CONFIG_SND_SOC_ROCKCHIP_HDMI_AUDIO) += snd-soc-rockchip-hdmi-audio.o
diff --git a/sound/soc/rockchip/rockchip_hdmi_audio.c b/sound/soc/rockchip/rockchip_hdmi_audio.c
new file mode 100644
index 0000000..0c9f497
--- /dev/null
+++ b/sound/soc/rockchip/rockchip_hdmi_audio.c
@@ -0,0 +1,196 @@
+/*
+ * rockchip-hdmi-card.c
+ *
+ * ROCKCHIP ALSA SoC DAI driver for HDMI audio on rockchip processors.
+ * Copyright (c) 2014, ROCKCHIP CORPORATION. All rights reserved.
+ * Authors: Yakir Yang <ykk@rock-chips.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.*
+ *
+ */
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+
+#include <sound/soc.h>
+#include <sound/pcm.h>
+#include <sound/jack.h>
+#include <sound/core.h>
+#include <sound/pcm_params.h>
+
+#include "rockchip_i2s.h"
+#include "../codecs/dw-hdmi-audio.h"
+
+#define DRV_NAME "rockchip-hdmi-audio"
+
+struct hdmi_audio_private {
+	struct snd_soc_jack hdmi_jack;
+};
+
+static int rockchip_hdmi_audio_hw_params(struct snd_pcm_substream *substream,
+					struct snd_pcm_hw_params *params)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+	unsigned int dai_fmt = rtd->dai_link->dai_fmt;
+	int mclk, ret;
+
+	switch (params_rate(params)) {
+	case 8000:
+	case 16000:
+	case 24000:
+	case 32000:
+	case 48000:
+	case 64000:
+	case 96000:
+		mclk = 12288000;
+		break;
+	case 11025:
+	case 22050:
+	case 44100:
+	case 88200:
+		mclk = 11289600;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt);
+	if (ret < 0) {
+		dev_err(cpu_dai->dev, "failed to set cpu_dai fmt.\n");
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, SND_SOC_CLOCK_OUT);
+	if (ret < 0) {
+		dev_err(cpu_dai->dev, "failed to set cpu_dai sysclk.\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int hdmi_audio_dai_init(struct snd_soc_pcm_runtime *runtime)
+{
+	struct snd_soc_codec *codec = runtime->codec;
+	struct snd_soc_card *card = runtime->card;
+	struct hdmi_audio_private *drv = snd_soc_card_get_drvdata(card);
+
+	/* Enable headphone jack detection */
+	snd_soc_jack_new(codec, "HDMI Jack", SND_JACK_LINEOUT,
+			 &drv->hdmi_jack);
+
+	return dw_hdmi_jack_detect(codec, &drv->hdmi_jack);
+}
+
+static struct snd_soc_ops hdmi_audio_dai_ops = {
+	.hw_params = rockchip_hdmi_audio_hw_params,
+};
+
+static struct snd_soc_dai_link hdmi_audio_dai = {
+	.name = "RockchipHDMI",
+	.stream_name = "RockchipHDMI",
+	.codec_name = "dw-hdmi-audio",
+	.codec_dai_name = "dw-hdmi-hifi",
+	.init = hdmi_audio_dai_init,
+	.ops = &hdmi_audio_dai_ops,
+	.dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+		   SND_SOC_DAIFMT_CBS_CFS,
+};
+
+static struct snd_soc_card rockchip_hdmi_audio_card = {
+	.name = "RockchipHDMI",
+	.owner = THIS_MODULE,
+	.dai_link = &hdmi_audio_dai,
+	.num_links = 1,
+};
+
+static int rockchip_hdmi_audio_probe(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = &rockchip_hdmi_audio_card;
+	struct device_node *np = pdev->dev.of_node;
+	struct hdmi_audio_private *drv;
+	int ret;
+
+	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
+	if (!drv)
+		return -ENOMEM;
+
+	card->dev = &pdev->dev;
+	platform_set_drvdata(pdev, card);
+	snd_soc_card_set_drvdata(card, drv);
+
+	hdmi_audio_dai.cpu_of_node = of_parse_phandle(np, "i2s-controller", 0);
+	if (!hdmi_audio_dai.cpu_of_node) {
+		dev_err(&pdev->dev, "Property 'i2s-controller' missing !\n");
+		goto free_priv_data;
+	}
+
+	hdmi_audio_dai.platform_of_node = hdmi_audio_dai.cpu_of_node;
+
+	ret = snd_soc_register_card(card);
+	if (ret) {
+		dev_err(&pdev->dev, "register card failed (%d)\n", ret);
+		card->dev = NULL;
+		goto free_cpu_of_node;
+	}
+
+	dev_info(&pdev->dev, "hdmi audio init success.\n");
+
+	return 0;
+
+free_cpu_of_node:
+	hdmi_audio_dai.cpu_of_node = NULL;
+	hdmi_audio_dai.platform_of_node = NULL;
+free_priv_data:
+	snd_soc_card_set_drvdata(card, NULL);
+	platform_set_drvdata(pdev, NULL);
+	devm_kfree(&pdev->dev, drv);
+	card->dev = NULL;
+
+	return ret;
+}
+
+static int rockchip_hdmi_audio_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+
+	snd_soc_unregister_card(card);
+	snd_soc_card_set_drvdata(card, NULL);
+	platform_set_drvdata(pdev, NULL);
+	card->dev = NULL;
+
+	return 0;
+}
+
+static const struct of_device_id rockchip_hdmi_audio_of_match[] = {
+	{ .compatible = "rockchip,rk3288-hdmi-audio", },
+	{},
+};
+
+static struct platform_driver rockchip_hdmi_audio_driver = {
+	.driver = {
+		.name = DRV_NAME,
+		.owner = THIS_MODULE,
+		.of_match_table = rockchip_hdmi_audio_of_match,
+	},
+	.probe = rockchip_hdmi_audio_probe,
+	.remove = rockchip_hdmi_audio_remove,
+};
+module_platform_driver(rockchip_hdmi_audio_driver);
+
+MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip HDMI Audio ASoC Interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
+MODULE_DEVICE_TABLE(of, rockchip_hdmi_audio_of_match);
-- 
2.1.2

[PATCH v2 12/12] dt-bindings: Add documentation for Rockchip dw-hdmi-audio

[Yakir Yang]

Required properties:
- compatible: platform specific
- cpu-of-node: the device node of cpu_dai

Signed-off-by: Yakir Yang <ykk@rock-chips.com>
---
Changes in v2:
- remove codec-name and codec-dai-name
- rename rockchip,rockchip-hdmi-audio.txt to rockchip,rockchip-dw-hdmi-audio.txt

 .../bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt       | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt

diff --git a/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
new file mode 100644
index 0000000..5b86eed
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
@@ -0,0 +1,12 @@
+Rockchip hdmi audio bindings
+
+Required properties:
+- compatible: platform specific
+- cpu-of-node: the device node of cpu_dai
+
+Example:
+
+sound {
+	compatible = "rockchip,rk3288-hdmi-audio";
+	cpu-of-node = <&i2s>;
+};
-- 
2.1.2

Re: [PATCH v2 12/12] dt-bindings: Add documentation for Rockchip dw-hdmi-audio

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:44:13AM -0500, Yakir Yang wrote:
> Required properties:
> - compatible: platform specific
> - cpu-of-node: the device node of cpu_dai
> 
> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
> ---
> Changes in v2:
> - remove codec-name and codec-dai-name
> - rename rockchip,rockchip-hdmi-audio.txt to rockchip,rockchip-dw-hdmi-audio.txt
> 
>  .../bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt       | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
> new file mode 100644
> index 0000000..5b86eed
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
> @@ -0,0 +1,12 @@
> +Rockchip hdmi audio bindings
> +
> +Required properties:
> +- compatible: platform specific
> +- cpu-of-node: the device node of cpu_dai
> +
> +Example:
> +
> +sound {
> +	compatible = "rockchip,rk3288-hdmi-audio";
> +	cpu-of-node = <&i2s>;
> +};

In patch 11, it looks like you parse a property called i2s-controller.
This doesn't appear to be documented.  Maybe it's what you call
"cpu-of-node" above?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 12/12] dt-bindings: Add documentation for Rockchip dw-hdmi-audio

[Yang Kuankuan]

On 01/31/2015 06:36 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:44:13AM -0500, Yakir Yang wrote:
>> Required properties:
>> - compatible: platform specific
>> - cpu-of-node: the device node of cpu_dai
>>
>> Signed-off-by: Yakir Yang <ykk@rock-chips.com>
>> ---
>> Changes in v2:
>> - remove codec-name and codec-dai-name
>> - rename rockchip,rockchip-hdmi-audio.txt to rockchip,rockchip-dw-hdmi-audio.txt
>>
>>   .../bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt       | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
>> new file mode 100644
>> index 0000000..5b86eed
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/sound/rockchip,rockchip-dw-hdmi-audio.txt
>> @@ -0,0 +1,12 @@
>> +Rockchip hdmi audio bindings
>> +
>> +Required properties:
>> +- compatible: platform specific
>> +- cpu-of-node: the device node of cpu_dai
>> +
>> +Example:
>> +
>> +sound {
>> +	compatible = "rockchip,rk3288-hdmi-audio";
>> +	cpu-of-node = <&i2s>;
>> +};
> In patch 11, it looks like you parse a property called i2s-controller.
> This doesn't appear to be documented.  Maybe it's what you call
> "cpu-of-node" above?
>
Mistaken, i will modify dt-bings in next version v3.

Thanks for your kindness remind.  : )

Re: [PATCH v2 0/12] Those patches is used for dw_hdmi audio support

[Russell King - ARM Linux]

On Fri, Jan 30, 2015 at 06:23:51AM -0500, Yakir Yang wrote:
> We found Designware hdmi driver only support audio clock config, we can not play sound through it.
> To add Designware HDMI Audio support, we make those patch set:
>  1): modify n/cts config order, according to dw_hdmi document.
>  2): add Audio Sample Channel Status config interfaces to dw_hdmi driver.
>  3): add audio support for more display resolutions(eg. 800x600).
>  4): add audio support for No-CEA display resolutions.
>  5): fixed dw_hdmi irq bug, add irq control to suspend/resume interfaces.
>  6): add suspend/resume callback for dw_hdmi rockchip driver.
>  7): filter interlace mode in rockchip vop driver.
>  8): add hdmi audio config interfaces to dw_hdmi driver.
>  9): creat "dw_hdmi-audio" platform device in dw_hdmi driver.
> 10): add codec driver for hdmi audio, callback dw_hdmi audio config functions.
> 11): add sound driver for hdmi audio, creat hdmi audio sound card.
> 12): add dt-bings file and add hdmi_audio node to corresponding dt file.

I think the overall issue with this patch is working out how to support
both the iMX6 version of this IP, and the Rockchip version of the IP.

These two hardware IPs seem to be configured at synthesis time with
entirely different audio architectures, which change which registers
are available, and sometimes which bits in the registers are present,
which makes it more difficult to come up with a unified audio driver.

Also, I think that it would be a good idea to start documenting which
registers are available in which versions of the IP in dw_hdmi.h,
otherwise I can see that it's going to be very easy for someone to
assume that some register or bit which is available in one IP is
present on all.

The CONFIGx_ID register values for the iMX6 SoC are:

	CONFIG0_ID 0x8f
	CONFIG1_ID 0x01
	CONFIG2_ID 0xf2
	CONFIG3_ID 0x02

CONFIG0_ID appears to contain bits which indicate whether the IP
supports I2S and SPDIF mode.  Presumably your IP has bit 4 set for I2S,
and maybe bit 5 for SPDIF?

CONFIG1_ID bit 0 indicates whether the AHB interface is present, which
is presumably zero for your IP?

CONFIG3_ID bit 0 indicates whether "generic parallel audio, GPAUD" is
present.

Could you provide (in addition to the values printed in the message I
requested in another reply) the values of the CONFIGx_ID registers
please, and whether any of the bits in there are documented as being
applicable to audio.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 0/12] Those patches is used for dw_hdmi audio support

[Yang Kuankuan]

On 01/31/2015 07:00 AM, Russell King - ARM Linux wrote:
> On Fri, Jan 30, 2015 at 06:23:51AM -0500, Yakir Yang wrote:
>> We found Designware hdmi driver only support audio clock config, we can not play sound through it.
>> To add Designware HDMI Audio support, we make those patch set:
>>   1): modify n/cts config order, according to dw_hdmi document.
>>   2): add Audio Sample Channel Status config interfaces to dw_hdmi driver.
>>   3): add audio support for more display resolutions(eg. 800x600).
>>   4): add audio support for No-CEA display resolutions.
>>   5): fixed dw_hdmi irq bug, add irq control to suspend/resume interfaces.
>>   6): add suspend/resume callback for dw_hdmi rockchip driver.
>>   7): filter interlace mode in rockchip vop driver.
>>   8): add hdmi audio config interfaces to dw_hdmi driver.
>>   9): creat "dw_hdmi-audio" platform device in dw_hdmi driver.
>> 10): add codec driver for hdmi audio, callback dw_hdmi audio config functions.
>> 11): add sound driver for hdmi audio, creat hdmi audio sound card.
>> 12): add dt-bings file and add hdmi_audio node to corresponding dt file.
> I think the overall issue with this patch is working out how to support
> both the iMX6 version of this IP, and the Rockchip version of the IP.
>
> These two hardware IPs seem to be configured at synthesis time with
> entirely different audio architectures, which change which registers
> are available, and sometimes which bits in the registers are present,
> which makes it more difficult to come up with a unified audio driver.
>
> Also, I think that it would be a good idea to start documenting which
> registers are available in which versions of the IP in dw_hdmi.h,
> otherwise I can see that it's going to be very easy for someone to
> assume that some register or bit which is available in one IP is
> present on all.
>
> The CONFIGx_ID register values for the iMX6 SoC are:
>
> 	CONFIG0_ID 0x8f
> 	CONFIG1_ID 0x01
> 	CONFIG2_ID 0xf2
> 	CONFIG3_ID 0x02
>
> CONFIG0_ID appears to contain bits which indicate whether the IP
> supports I2S and SPDIF mode.  Presumably your IP has bit 4 set for I2S,
> and maybe bit 5 for SPDIF?
>
> CONFIG1_ID bit 0 indicates whether the AHB interface is present, which
> is presumably zero for your IP?
>
> CONFIG3_ID bit 0 indicates whether "generic parallel audio, GPAUD" is
> present.
>
> Could you provide (in addition to the values printed in the message I
> requested in another reply) the values of the CONFIGx_ID registers
> please, and whether any of the bits in there are documented as being
> applicable to audio.
>
> Thanks.
The IP version on rk3288 : dwhdmi-rockchip ff980000.hdmi: Detected HDMI 
controller 0x20:0xa:0xa0:0xc1

The CONFIGx_ID register values for the rk3288 soc are:
     CONFIG0_ID    0xbf
     CONFIG1_ID    0x22
     CONFIG2_ID    0xc2
     CONFIG3_ID    0x0

After looking at iMX6 DQRM, i found only CONFIG1_ID  & CONFIG3_ID are 
different.

CONFIG1_ID bit 5 indicates whether the HDMI2.0 is present. Bit 1 
indicates whether
configuration interface is APB interface.

CONFIG3_ID bit 3 indicates whether the AHBAUDDMA is present. Bit 1 
indicates whether
Generic Parallel Audio is present.