[PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Kuninori Morimoto]

Dear Mark, Liam, Guennadi

These are ALSA V2 bug fix patches which are reported by Guennadi.

Kuninori Morimoto (5):
      fbdev: sh_mobile_hdmi: modify noisy comment out
      fbdev: sh_mobile_hdmi: modify flags name to more specific
      fbdev: sh_mobile_hdmi: modify snd_soc_dai_driver settings
      fbdev: sh_mobile_hdmi: add new label for sound error path
      ASoC: fsi-hdmi: remove unneeded header

To Mark.
Please care above patch independently from these patches.
[alsa-devel] [PATCH 5/5] ASoC: fsi-ak4642: modiry platform_name

These series are for ALSA side patches.
I will send SH side V2 patches soon.

Main diff v1 <==> v2 is
I added Guennadi's report mail to log area on each patches,
and care more.
But these series still didn't care above.
I added reasons.

-- Guennadi ------------------------------------------
> +config SND_FSI_HDMI
> +	bool "FSI-HDMI sound support"
> +	depends on SND_SOC_SH4_FSI && FB_SH_MOBILE_HDMI
> +	help
> +	  This option enables generic sound support for the
> +	  FSI - HDMI unit
> +

"bool" means, if someone is linking the whole ASoC into the kernel, they 
will not be able to build this as a module. Not a big deal, but you're 
stealing some freedom from the user.
-------------------------------------------------------

understand.
But the config which use "bool" for FSI-XXX is not only FSI-HDMI.
So, I will care your indication as "new function patch" in future.


-- Guennadi ------------------------------------------
With this config option you will have 3 SND_SOC_SH4_FSI implementations in 
the Kconfig, all selectable independently. Do you really think it makes 
sense and would work, if someone were to select more than one of those 
options at the same time?
-------------------------------------------------------

Yes. I created it for independently.
For example, you can select FSI-AK4642 and FSI-HDMI in same time,
and both works well for me on AP4 board now.

But I guess, if you select FSI-DA7210 and FSI-HDMI,
small patch which change FSIA <-> FSIB is needed.

  FSI-AK4642 : FSI-A
  FSI-DA7210 : FSI-B
  FSI-HDMI   : FSI-B

I think that it should be going to enable the selection of
"FSI-A" and "FSI-B" in the future.
But now, no way, no idea.
It is my task

By the way, about HDMI sound,
I know that there are some HDMI monitors which can not use sound.
Now I'm debuging about it.

-- Guennadi ------------------------------------------
> +		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,

Again, this I am not sure about. The datasheet says 16 to 32 bit are 
possible, but then I only see configuration for 16 to 24 bits, but in any 
case, I think, you'd want to implement .hw_params to support non-default 
formats.
-------------------------------------------------------

Yes. I should modify this .formats.
But I can not select, test, support 32bit in current environment.
So, please put it to my TODO list.

And yes. .hw_params implementation is important for advanced support.
But now, current HDMI sound support is still prototype (I didn't indicate though).
So. please put it to my TODO list too.

Best regards
--
Kuninori Morimoto

[PATCH 1/5 v2] fbdev: sh_mobile_hdmi: modify noisy comment out

[Kuninori Morimoto]

This patch solve below report from Guennadi

1)

> -	hdmi_write(hdmi, 0x00, HDMI_AUDIO_SETTING_1);
> +	switch (pdata->flags & HDMI_SRC_MASK) {
> +	default:
> +		/* FALL THROUGH */

I'm not sure I like the capitalisation here - no reason to shout;)

2)

> +/************************************************************************
> +
> +
> +			HDMI sound
> +
> +
> +************************************************************************/

I don't think this comment deserves 7 lines of text, besides breaking the
multiline comment style. If you think, one line like

/*			HDMI sound			*/

is not enough how about just

/*
 *			HDMI sound
 */

3)

> +/************************************************************************
> +
> +
> +			HDMI video
> +
> +
> +************************************************************************/

See above - 7 lines seem to be an overkill to me.

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

# I didn't care this on V1 patches.
o add Guennadi's mail on log area

 drivers/video/sh_mobile_hdmi.c |   21 +++++++--------------
 1 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 16187d6..0acd850 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -224,13 +224,9 @@ static u8 hdmi_read(struct sh_hdmi *hdmi, u8 reg)
 	return ioread8(hdmi->base + reg);
 }
 
-/************************************************************************
-
-
-			HDMI sound
-
-
-************************************************************************/
+/*
+ *	HDMI sound
+ */
 static unsigned int sh_hdmi_snd_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
@@ -273,13 +269,10 @@ static struct snd_soc_codec_driver soc_codec_dev_sh_hdmi = {
 	.write		= sh_hdmi_snd_write,
 };
 
-/************************************************************************
-
-
-			HDMI video
-
+/*
+ *	HDMI video
+ */
 
-************************************************************************/
 /* External video parameter settings */
 static void hdmi_external_video_param(struct sh_hdmi *hdmi)
 {
@@ -398,7 +391,7 @@ static void sh_hdmi_audio_config(struct sh_hdmi *hdmi)
 	 */
 	switch (pdata->flags & HDMI_SRC_MASK) {
 	default:
-		/* FALL THROUGH */
+		/* fall through */
 	case HDMI_SRC_I2S:
 		data = (0x0 << 3);
 		break;
-- 
1.7.0.4

[PATCH 2/5 v2] fbdev: sh_mobile_hdmi: modify flags name to more specific

[Kuninori Morimoto]

This patch solve below report from Guennadi

1)

> +/* Audio source select */
> +#define HDMI_SRC_MASK		(0xF << 0)
> +#define HDMI_SRC_I2S		(0 << 0) /* default */
> +#define HDMI_SRC_SPDIF		(1 << 0)
> +#define HDMI_SRC_DSD		(2 << 0)
> +#define HDMI_SRC_HBR		(3 << 0)

I would be more specific with these macro names, i.e., include "AUDIO" or
"SND" or something similar in them, e.g., HDMI_AUDIO_SRC_I2S.

2)

> +	case HDMI_SRC_I2S:
> +		data = (0x0 << 3);
> +		break;
> +	case HDMI_SRC_SPDIF:
> +		data = (0x1 << 3);
> +		break;
> +	case HDMI_SRC_DSD:
> +		data = (0x2 << 3);
> +		break;
> +	case HDMI_SRC_HBR:
> +		data = (0x3 << 3);

In all above cases parenthesis are superfluous.

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

o add Guennadi's mail on log area
o remove noisy parenthesis

 drivers/video/sh_mobile_hdmi.c |   18 +++++++++---------
 include/video/sh_mobile_hdmi.h |   10 +++++-----
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index 0acd850..beb04ef 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -389,20 +389,20 @@ static void sh_hdmi_audio_config(struct sh_hdmi *hdmi)
 	 * [6:5] set required down sampling rate if required
 	 * [4:3] set required audio source
 	 */
-	switch (pdata->flags & HDMI_SRC_MASK) {
+	switch (pdata->flags & HDMI_SND_SRC_MASK) {
 	default:
 		/* fall through */
-	case HDMI_SRC_I2S:
-		data = (0x0 << 3);
+	case HDMI_SND_SRC_I2S:
+		data = 0x0 << 3;
 		break;
-	case HDMI_SRC_SPDIF:
-		data = (0x1 << 3);
+	case HDMI_SND_SRC_SPDIF:
+		data = 0x1 << 3;
 		break;
-	case HDMI_SRC_DSD:
-		data = (0x2 << 3);
+	case HDMI_SND_SRC_DSD:
+		data = 0x2 << 3;
 		break;
-	case HDMI_SRC_HBR:
-		data = (0x3 << 3);
+	case HDMI_SND_SRC_HBR:
+		data = 0x3 << 3;
 		break;
 	}
 	hdmi_write(hdmi, data, HDMI_AUDIO_SETTING_1);
diff --git a/include/video/sh_mobile_hdmi.h b/include/video/sh_mobile_hdmi.h
index 929c2d3..1e1aa54 100644
--- a/include/video/sh_mobile_hdmi.h
+++ b/include/video/sh_mobile_hdmi.h
@@ -23,11 +23,11 @@ struct device;
  */
 
 /* Audio source select */
-#define HDMI_SRC_MASK		(0xF << 0)
-#define HDMI_SRC_I2S		(0 << 0) /* default */
-#define HDMI_SRC_SPDIF		(1 << 0)
-#define HDMI_SRC_DSD		(2 << 0)
-#define HDMI_SRC_HBR		(3 << 0)
+#define HDMI_SND_SRC_MASK	(0xF << 0)
+#define HDMI_SND_SRC_I2S	(0 << 0) /* default */
+#define HDMI_SND_SRC_SPDIF	(1 << 0)
+#define HDMI_SND_SRC_DSD	(2 << 0)
+#define HDMI_SND_SRC_HBR	(3 << 0)
 
 struct sh_mobile_hdmi_info {
 	struct sh_mobile_lcdc_chan_cfg	*lcd_chan;
-- 
1.7.0.4

[PATCH 3/5 v2] fbdev: sh_mobile_hdmi: modify snd_soc_dai_driver settings

[Kuninori Morimoto]

This patch solve below report from Guennadi


> +static struct snd_soc_dai_driver sh_hdmi_dai = {
> +	.name = "sh_mobile_hdmi-hifi",
> +	.playback = {
> +		.stream_name = "Playback",
> +		.channels_min = 1,

Can it actually do mono? Maybe at probe time you could look at audio flags
from your previous patch and, e.g., for SPDIF set channels_min to 2?

> +		.channels_max = 2,

That's the "smallest max," yes. With some other interfaces (I2S, DSD) it
can support up to 8 channels...

> +		.rates = SNDRV_PCM_RATE_8000_48000,

Hm, in the datasheet I see supported frequencies 32kHz to 192kHz. And if
you promise support for multiple frequencies, don't you want to implement
.hw_params? Besides, not all of these frequencies will be available,
depending on your supplied clock and your willingness to implement
downsampling.

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

o add Guennadi's mail on log area

 drivers/video/sh_mobile_hdmi.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index beb04ef..a2cb492 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -249,9 +249,12 @@ static struct snd_soc_dai_driver sh_hdmi_dai = {
 	.name = "sh_mobile_hdmi-hifi",
 	.playback = {
 		.stream_name = "Playback",
-		.channels_min = 1,
-		.channels_max = 2,
-		.rates = SNDRV_PCM_RATE_8000_48000,
+		.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,
 	},
 };
-- 
1.7.0.4

[PATCH 4/5 v2] fbdev: sh_mobile_hdmi: add new label for sound error path

[Kuninori Morimoto]

This patch solve below report from Guennadi


>  /* External video parameter settings */
>  static void hdmi_external_video_param(struct sh_hdmi *hdmi)
>  {
> @@ -804,6 +862,11 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>
> +	ret =  snd_soc_register_codec(&pdev->dev,
> +			&soc_codec_dev_sh_hdmi, &sh_hdmi_dai, 1);
> +	if (ret < 0)
> +		goto egetclk;
> +
>  	hdmi->dev = &pdev->dev;
>
>  	hdmi->hdmi_clk = clk_get(&pdev->dev, "ick");

NAK. This breaks the error path and has to be fixed. Firstly, please, use
a new label like "esndreg," secondly, you have to add

 	clk_disable(hdmi->hdmi_clk);
 erate:
 	clk_put(hdmi->hdmi_clk);
 egetclk:
+	snd_soc_unregister_codec(&pdev->dev);
+esndreg:
 	mutex_destroy(&hdmi->mutex);
 	kfree(hdmi);

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

o add Guennadi's mail on log area

 drivers/video/sh_mobile_hdmi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index a2cb492..ef989d9 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -967,7 +967,7 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
 	ret =  snd_soc_register_codec(&pdev->dev,
 			&soc_codec_dev_sh_hdmi, &sh_hdmi_dai, 1);
 	if (ret < 0)
-		goto egetclk;
+		goto esndreg;
 
 	hdmi->dev = &pdev->dev;
 
@@ -1054,6 +1054,8 @@ eclkenable:
 erate:
 	clk_put(hdmi->hdmi_clk);
 egetclk:
+	snd_soc_unregister_codec(&pdev->dev);
+esndreg:
 	kfree(hdmi);
 
 	return ret;
-- 
1.7.0.4

[PATCH 4/5 v2] fbdev: sh_mobile_hdmi: add new label for sound error path

[Kuninori Morimoto]

This patch solve below report from Guennadi


>  /* External video parameter settings */
>  static void hdmi_external_video_param(struct sh_hdmi *hdmi)
>  {
> @@ -804,6 +862,11 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>
> +	ret =  snd_soc_register_codec(&pdev->dev,
> +			&soc_codec_dev_sh_hdmi, &sh_hdmi_dai, 1);
> +	if (ret < 0)
> +		goto egetclk;
> +
>  	hdmi->dev = &pdev->dev;
>
>  	hdmi->hdmi_clk = clk_get(&pdev->dev, "ick");

NAK. This breaks the error path and has to be fixed. Firstly, please, use
a new label like "esndreg," secondly, you have to add

 	clk_disable(hdmi->hdmi_clk);
 erate:
 	clk_put(hdmi->hdmi_clk);
 egetclk:
+	snd_soc_unregister_codec(&pdev->dev);
+esndreg:
 	mutex_destroy(&hdmi->mutex);
 	kfree(hdmi);

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

o add Guennadi's mail on log area

 drivers/video/sh_mobile_hdmi.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/video/sh_mobile_hdmi.c b/drivers/video/sh_mobile_hdmi.c
index a2cb492..ef989d9 100644
--- a/drivers/video/sh_mobile_hdmi.c
+++ b/drivers/video/sh_mobile_hdmi.c
@@ -967,7 +967,7 @@ static int __init sh_hdmi_probe(struct platform_device *pdev)
 	ret =  snd_soc_register_codec(&pdev->dev,
 			&soc_codec_dev_sh_hdmi, &sh_hdmi_dai, 1);
 	if (ret < 0)
-		goto egetclk;
+		goto esndreg;
 
 	hdmi->dev = &pdev->dev;
 
@@ -1054,6 +1054,8 @@ eclkenable:
 erate:
 	clk_put(hdmi->hdmi_clk);
 egetclk:
+	snd_soc_unregister_codec(&pdev->dev);
+esndreg:
 	kfree(hdmi);
 
 	return ret;
-- 
1.7.0.4

[PATCH 5/5 v2] ASoC: fsi-hdmi: remove unneeded header

[Kuninori Morimoto]

This patch solve below report from Guennadi.
But I didn't remove #include <sound/sh_fsi.h>.
Because it have FSI_PORT_B define which is used on this file.


> +#include <linux/platform_device.h>
> +#include <sound/sh_fsi.h>
> +#include <video/sh_mobile_hdmi.h>

Now that everything is done with strings - do you still need these
headers?

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2
o add Guennadi's mail on log area

 sound/soc/sh/fsi-hdmi.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/sound/soc/sh/fsi-hdmi.c b/sound/soc/sh/fsi-hdmi.c
index 950e3e0..fcda149 100644
--- a/sound/soc/sh/fsi-hdmi.c
+++ b/sound/soc/sh/fsi-hdmi.c
@@ -11,7 +11,6 @@
 
 #include <linux/platform_device.h>
 #include <sound/sh_fsi.h>
-#include <video/sh_mobile_hdmi.h>
 
 static struct snd_soc_dai_link fsi_dai_link = {
 	.name		= "HDMI",
-- 
1.7.0.4

Re: [PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Guennadi Liakhovetski]

Hello Morimoto-san

Thanks
for your patches! But what about this my comment:

<quote>
Besides, I think, this will not link without CONFIG_SND_SOC.
</quote>

? Or is it wrong? If this is right, it would be better to either add a 
"select SND_SOC" to FB_SH_MOBILE_HDMI Kconfig entry, or put calls to 
snd_soc_(un)register_codec (and all the HDMI audio code) under "#ifdef 
CONFIG_SND_SOC." The letter is, probably, less elegant.

Thanks
Guennadi

On Thu, 9 Sep 2010, Kuninori Morimoto wrote:

> 
> Dear Mark, Liam, Guennadi
> 
> These are ALSA V2 bug fix patches which are reported by Guennadi.
> 
> Kuninori Morimoto (5):
>       fbdev: sh_mobile_hdmi: modify noisy comment out
>       fbdev: sh_mobile_hdmi: modify flags name to more specific
>       fbdev: sh_mobile_hdmi: modify snd_soc_dai_driver settings
>       fbdev: sh_mobile_hdmi: add new label for sound error path
>       ASoC: fsi-hdmi: remove unneeded header
> 
> To Mark.
> Please care above patch independently from these patches.
> [alsa-devel] [PATCH 5/5] ASoC: fsi-ak4642: modiry platform_name
> 
> These series are for ALSA side patches.
> I will send SH side V2 patches soon.
> 
> Main diff v1 <==> v2 is
> I added Guennadi's report mail to log area on each patches,
> and care more.
> But these series still didn't care above.
> I added reasons.
> 
> -- Guennadi ------------------------------------------
> > +config SND_FSI_HDMI
> > +	bool "FSI-HDMI sound support"
> > +	depends on SND_SOC_SH4_FSI && FB_SH_MOBILE_HDMI
> > +	help
> > +	  This option enables generic sound support for the
> > +	  FSI - HDMI unit
> > +
> 
> "bool" means, if someone is linking the whole ASoC into the kernel, they 
> will not be able to build this as a module. Not a big deal, but you're 
> stealing some freedom from the user.
> -------------------------------------------------------
> 
> understand.
> But the config which use "bool" for FSI-XXX is not only FSI-HDMI.
> So, I will care your indication as "new function patch" in future.
> 
> 
> -- Guennadi ------------------------------------------
> With this config option you will have 3 SND_SOC_SH4_FSI implementations in 
> the Kconfig, all selectable independently. Do you really think it makes 
> sense and would work, if someone were to select more than one of those 
> options at the same time?
> -------------------------------------------------------
> 
> Yes. I created it for independently.
> For example, you can select FSI-AK4642 and FSI-HDMI in same time,
> and both works well for me on AP4 board now.
> 
> But I guess, if you select FSI-DA7210 and FSI-HDMI,
> small patch which change FSIA <-> FSIB is needed.
> 
>   FSI-AK4642 : FSI-A
>   FSI-DA7210 : FSI-B
>   FSI-HDMI   : FSI-B
> 
> I think that it should be going to enable the selection of
> "FSI-A" and "FSI-B" in the future.
> But now, no way, no idea.
> It is my task
> 
> By the way, about HDMI sound,
> I know that there are some HDMI monitors which can not use sound.
> Now I'm debuging about it.
> 
> -- Guennadi ------------------------------------------
> > +		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
> 
> Again, this I am not sure about. The datasheet says 16 to 32 bit are 
> possible, but then I only see configuration for 16 to 24 bits, but in any 
> case, I think, you'd want to implement .hw_params to support non-default 
> formats.
> -------------------------------------------------------
> 
> Yes. I should modify this .formats.
> But I can not select, test, support 32bit in current environment.
> So, please put it to my TODO list.
> 
> And yes. .hw_params implementation is important for advanced support.
> But now, current HDMI sound support is still prototype (I didn't indicate though).
> So. please put it to my TODO list too.
> 
> Best regards
> --
> Kuninori Morimoto
>  
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Kuninori Morimoto]

Dear Guennadi

> <quote>
> Besides, I think, this will not link without CONFIG_SND_SOC.
> </quote>
> 
> ? Or is it wrong? If this is right, it would be better to either add a 
> "select SND_SOC" to FB_SH_MOBILE_HDMI Kconfig entry, or put calls to 
> snd_soc_(un)register_codec (and all the HDMI audio code) under "#ifdef 
> CONFIG_SND_SOC." The letter is, probably, less elegant.

Oh sorry. I didn't explain about it.
Yes.
My 1st idea was same as yours (add SND_SOC to Kconfig).
But I couldn't decide which is better.

I send "add select SND_SOC to Kconfig" patch.
OK ?

Best regards
--
Kuninori Morimoto

Re: [PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Guennadi Liakhovetski]

On Thu, 9 Sep 2010, Kuninori Morimoto wrote:

> 
> Dear Guennadi
> 
> > <quote>
> > Besides, I think, this will not link without CONFIG_SND_SOC.
> > </quote>
> > 
> > ? Or is it wrong? If this is right, it would be better to either add a 
> > "select SND_SOC" to FB_SH_MOBILE_HDMI Kconfig entry, or put calls to 
> > snd_soc_(un)register_codec (and all the HDMI audio code) under "#ifdef 
> > CONFIG_SND_SOC." The letter is, probably, less elegant.
> 
> Oh sorry. I didn't explain about it.
> Yes.
> My 1st idea was same as yours (add SND_SOC to Kconfig).
> But I couldn't decide which is better.
> 
> I send "add select SND_SOC to Kconfig" patch.
> OK ?

Suits me, thanks.

Regards
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 6/5] fbdev: sh_mobile_hdmi: Add select SND_SOC to Kconfig

[Kuninori Morimoto]

This patch solve compile error when .config doesn't have
CONFIG_SND_SOC which is needed for HDMI sound.
It was reported by Guennadi as follows


Besides, I think, this will not link without CONFIG_SND_SOC.

Reported-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/video/Kconfig |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 8b31fdf..43e90b8 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -1919,6 +1919,7 @@ config FB_SH_MOBILE_HDMI
 	tristate "SuperH Mobile HDMI controller support"
 	depends on FB_SH_MOBILE_LCDC
 	select FB_MODE_HELPERS
+	select SND_SOC
 	---help---
 	  Driver for the on-chip SH-Mobile HDMI controller.
 
-- 
1.7.0.4

Re: [PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Guennadi Liakhovetski]

On Thu, 9 Sep 2010, Kuninori Morimoto wrote:

> 
> Dear Mark, Liam, Guennadi

Hi Mark

Feel free to add my

Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

to these 6 patches.

Thanks
Guennadi

> 
> These are ALSA V2 bug fix patches which are reported by Guennadi.
> 
> Kuninori Morimoto (5):
>       fbdev: sh_mobile_hdmi: modify noisy comment out
>       fbdev: sh_mobile_hdmi: modify flags name to more specific
>       fbdev: sh_mobile_hdmi: modify snd_soc_dai_driver settings
>       fbdev: sh_mobile_hdmi: add new label for sound error path
>       ASoC: fsi-hdmi: remove unneeded header
> 
> To Mark.
> Please care above patch independently from these patches.
> [alsa-devel] [PATCH 5/5] ASoC: fsi-ak4642: modiry platform_name
> 
> These series are for ALSA side patches.
> I will send SH side V2 patches soon.
> 
> Main diff v1 <==> v2 is
> I added Guennadi's report mail to log area on each patches,
> and care more.
> But these series still didn't care above.
> I added reasons.
> 
> -- Guennadi ------------------------------------------
> > +config SND_FSI_HDMI
> > +	bool "FSI-HDMI sound support"
> > +	depends on SND_SOC_SH4_FSI && FB_SH_MOBILE_HDMI
> > +	help
> > +	  This option enables generic sound support for the
> > +	  FSI - HDMI unit
> > +
> 
> "bool" means, if someone is linking the whole ASoC into the kernel, they 
> will not be able to build this as a module. Not a big deal, but you're 
> stealing some freedom from the user.
> -------------------------------------------------------
> 
> understand.
> But the config which use "bool" for FSI-XXX is not only FSI-HDMI.
> So, I will care your indication as "new function patch" in future.
> 
> 
> -- Guennadi ------------------------------------------
> With this config option you will have 3 SND_SOC_SH4_FSI implementations in 
> the Kconfig, all selectable independently. Do you really think it makes 
> sense and would work, if someone were to select more than one of those 
> options at the same time?
> -------------------------------------------------------
> 
> Yes. I created it for independently.
> For example, you can select FSI-AK4642 and FSI-HDMI in same time,
> and both works well for me on AP4 board now.
> 
> But I guess, if you select FSI-DA7210 and FSI-HDMI,
> small patch which change FSIA <-> FSIB is needed.
> 
>   FSI-AK4642 : FSI-A
>   FSI-DA7210 : FSI-B
>   FSI-HDMI   : FSI-B
> 
> I think that it should be going to enable the selection of
> "FSI-A" and "FSI-B" in the future.
> But now, no way, no idea.
> It is my task
> 
> By the way, about HDMI sound,
> I know that there are some HDMI monitors which can not use sound.
> Now I'm debuging about it.
> 
> -- Guennadi ------------------------------------------
> > +		.formats = SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S24_LE,
> 
> Again, this I am not sure about. The datasheet says 16 to 32 bit are 
> possible, but then I only see configuration for 16 to 24 bits, but in any 
> case, I think, you'd want to implement .hw_params to support non-default 
> formats.
> -------------------------------------------------------
> 
> Yes. I should modify this .formats.
> But I can not select, test, support 32bit in current environment.
> So, please put it to my TODO list.
> 
> And yes. .hw_params implementation is important for advanced support.
> But now, current HDMI sound support is still prototype (I didn't indicate though).
> So. please put it to my TODO list too.
> 
> Best regards
> --
> Kuninori Morimoto
>  
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 0/5 v2] fbdev: sh_mobile_hdmi: bug fix patches V2

[Mark Brown]

On Fri, Sep 10, 2010 at 01:29:56PM +0200, Guennadi Liakhovetski wrote:
> Feel free to add my

> Reviewed-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

Applied, thanks.