No sound captured with SGTL5000 on i.MX6 in I²S master mode

Commit Message

Michael Nazzareno Trimarchi Sept. 18, 2014, 5:26 p.m. UTC

Hi

On Thu, Sep 18, 2014 at 10:05:24AM -0700, Nicolin Chen wrote:
> On Thu, Sep 18, 2014 at 06:09:50PM +0200, Jean-Michel Hautbois wrote:
> > Well, audmux is not getting clock, this is normal I think, the clock I
> > was mentionning is the MCLK of SGTL5000 and this one is linked to
> > FPGA, but found after all.
> > I now had a further look into sound/soc/fsl/fsl_ssi.c and I see this :
> > 
> >     sprop = of_get_property(np, "fsl,mode", NULL);
> >     if (sprop) {
> >         if (!strcmp(sprop, "ac97-slave"))
> >             ssi_private->dai_fmt = SND_SOC_DAIFMT_AC97;
> >         else if (!strcmp(sprop, "i2s-slave"))
> >             ssi_private->dai_fmt = SND_SOC_DAIFMT_I2S |
> >                 SND_SOC_DAIFMT_CBM_CFM;
> >     }
> > 
> > I may have missed something, but it seems that i2s-master is not
> > parsed, and does not set dai_fmt ?
> 
> Actually this is kind of obsolete property because initially the SSI
> driver only supported i2s-slave. We've put an i2s-slave check here is
> to limit the driver by excluding other modes.
> 
> But now, the driver has the capability to derive clock from CCM and
> output it for external CODEC. So this one could be dropped in fact
> rather than adding a new i2s-master case IMO.
> 
> > arecord -v -V stereo -f cd -D hw:0,0 somefile.wav
> > Recording WAVE 'somefile.wav' : Signed 16 bit Little Endian, Rate
> > 44100 Hz, Stereo
> 
> > arecord: pcm_read:2031: read error: Input/output error
> 
> The problem here should be the AUDMUX configuration issue. The imx-
> sgtl5000.c driver only supports CODEC in master mode. So if you try
> to switch the CODEC slave mode, you shall also change not only the
> CBM_CFM to CBS_CFS but also swap the ext_port and int_port of AUDMUX
> (a little confusing approach here as the configuration of AUDMUX is
> routing the data and clocks from a source port to a destination port
> while each of side, external or internal, might be a source port --
> When using CBM_CFM, the source port should be external port; while
> using CBS_CFS, the source port should be the internal port.)
> 
> There may be another topic, however, actually the fsl-asoc-card
> driver does handle the master/slave mode supports. So if you are
> trying to add the CODEC slave mode support into imx-sgtl5000. I
> suggest you to try the fsl-asoc-card instead. Its DT binding's
> totally compatible with imx-sgtl5000's. What you need to do is
> just enable it (and disable imx-sgtl500) in the menuconfig or
> add these enable/disable into imx_v6_v7_defconfig.
> 

bug was reported by Jean-Michel. I don't know if it works because I can not
test but seems ok. Do we need this _set_dai_fmt in probe function?

code is a bit broken just because panic on probe path if pdev is null
[...]
	/*
	 * If codec-handle property is missing from SSI node, we assume
	 * that the machine driver uses new binding which does not require
	 * SSI driver to trigger machine driver's probe.
	 */
	if (!of_get_property(np, "codec-handle", NULL))
		goto done;
[...]
	ssi_private->pdev =
		platform_device_register_data(&pdev->dev, name, 0, NULL, 0);
[...]
done:
	if (ssi_private->dai_fmt)
		_fsl_ssi_set_dai_fmt(ssi_private, ssi_private->dai_fmt);



Michael

> Nicolin

Comments

Nicolin Chen Sept. 18, 2014, 5:45 p.m. UTC | #1

On Thu, Sep 18, 2014 at 07:26:11PM +0200, Michael Trimarchi wrote:
> bug was reported by Jean-Michel. I don't know if it works because I can not
> test but seems ok. Do we need this _set_dai_fmt in probe function?

I think yes. The AC97 part might need this _set_dai_fmt call in the
probe().

> code is a bit broken just because panic on probe path if pdev is null
> [...]
> 	/*
> 	 * If codec-handle property is missing from SSI node, we assume
> 	 * that the machine driver uses new binding which does not require
> 	 * SSI driver to trigger machine driver's probe.
> 	 */
> 	if (!of_get_property(np, "codec-handle", NULL))
> 		goto done;
> [...]
> 	ssi_private->pdev =
> 		platform_device_register_data(&pdev->dev, name, 0, NULL, 0);
> [...]
> done:
> 	if (ssi_private->dai_fmt)
> 		_fsl_ssi_set_dai_fmt(ssi_private, ssi_private->dai_fmt);
> 
> 
> diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
> index 87eb577..f63bc02 100644
> --- a/sound/soc/fsl/fsl_ssi.c
> +++ b/sound/soc/fsl/fsl_ssi.c
> @@ -758,7 +758,8 @@ static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
>  	ssi_private->dai_fmt = fmt;
>  
>  	if (fsl_ssi_is_i2s_master(ssi_private) && IS_ERR(ssi_private->baudclk)) {
> -		dev_err(&ssi_private->pdev->dev, "baudclk is missing which is necessary for master mode\n");
> +		pr_err("baudclk is missing which is necessary"
> +			" for master mode\n");

Ah, this is a problem. We should not use ssi_private->pdev->dev here.
I think it'll be better if adding a new parameter of *dev pointer to
this _set_dai_fmt() -- passing pdev->dev in probe() and cpu_dai->dev
in fsl_ssi_set_dai_fmt().

Nicolin

Patch

diff --git a/sound/soc/fsl/fsl_ssi.c b/sound/soc/fsl/fsl_ssi.c
index 87eb577..f63bc02 100644
--- a/sound/soc/fsl/fsl_ssi.c
+++ b/sound/soc/fsl/fsl_ssi.c
@@ -758,7 +758,8 @@  static int _fsl_ssi_set_dai_fmt(struct fsl_ssi_private *ssi_private,
 	ssi_private->dai_fmt = fmt;
 
 	if (fsl_ssi_is_i2s_master(ssi_private) && IS_ERR(ssi_private->baudclk)) {
-		dev_err(&ssi_private->pdev->dev, "baudclk is missing which is necessary for master mode\n");
+		pr_err("baudclk is missing which is necessary"
+			" for master mode\n");
 		return -EINVAL;
 	}