[PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

When configuring sgtl5000 to work in slave mode the following error is seen:

root@freescale /$ aplay dtmf_mono_16b_48000.wav 
[   11.406460] random: nonblocking pool is initialized
[   11.600461] sgtl5000 1-000a: PLL not supported in slave mode
[   11.609052] sgtl5000 1-000a: ASoC: can't set sgtl5000 hw params: -22
ALSA lib pcm_direct.c:980:(snd1_pcm_direct_initialize_slave) unable to install hw params
ALSA lib pcm_dmix.c:1030:(snd_pcm_dmix_open) unable to initialize slave
aplay: main:660: audio open error: Invalid argument

Do not error out when sgtl5000_set_clock() reaches the default path for slave 
mode.

While at it, fix a typo in the comment.

Tested on a imx53-qsb configured as ssi in master and sgtl5000 as slave.

Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 sound/soc/codecs/sgtl5000.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index a604a22..b641ed1 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -619,14 +619,10 @@ static int sgtl5000_set_clock(struct snd_soc_codec *codec, int frame_rate)
 			SGTL5000_MCLK_FREQ_SHIFT;
 		break;
 	default:
-		/* if mclk not satisify the divider, use pll */
+		/* if mclk not satisfy the divider, use pll */
 		if (sgtl5000->master) {
 			clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
 				SGTL5000_MCLK_FREQ_SHIFT;
-		} else {
-			dev_err(codec->dev,
-				"PLL not supported in slave mode\n");
-			return -EINVAL;
 		}
 	}
 
-- 
1.9.1

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 871 bytes --]

On Thu, Sep 18, 2014 at 08:17:25PM -0300, Fabio Estevam wrote:

>  	default:
> -		/* if mclk not satisify the divider, use pll */
> +		/* if mclk not satisfy the divider, use pll */
>  		if (sgtl5000->master) {
>  			clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
>  				SGTL5000_MCLK_FREQ_SHIFT;
> -		} else {
> -			dev_err(codec->dev,
> -				"PLL not supported in slave mode\n");
> -			return -EINVAL;
>  		}
>  	}

Are you sure that the configuration that results is valid?  Typically
the requirements for MCLK to other clock ratios are very similar for
master and slave modes, it's just that it tends to be a lot more obvious
when things go wrong in master mode since directly visible clocks tend
to go wrong as opposed to performance problems.  Are the dividers that
we can't get configuration for purely for generating BCLK/LRCLK in
master mode or are they for other things?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Fabio Estevam]

Hi Mark,

On Mon, Sep 22, 2014 at 10:54 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Sep 18, 2014 at 08:17:25PM -0300, Fabio Estevam wrote:
>
>>       default:
>> -             /* if mclk not satisify the divider, use pll */
>> +             /* if mclk not satisfy the divider, use pll */
>>               if (sgtl5000->master) {
>>                       clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
>>                               SGTL5000_MCLK_FREQ_SHIFT;
>> -             } else {
>> -                     dev_err(codec->dev,
>> -                             "PLL not supported in slave mode\n");
>> -                     return -EINVAL;
>>               }
>>       }
>
> Are you sure that the configuration that results is valid?  Typically
> the requirements for MCLK to other clock ratios are very similar for
> master and slave modes, it's just that it tends to be a lot more obvious
> when things go wrong in master mode since directly visible clocks tend
> to go wrong as opposed to performance problems.  Are the dividers that
> we can't get configuration for purely for generating BCLK/LRCLK in
> master mode or are they for other things?

Tested sgtl5000 slave mode with different sampling rates and it plays well.

Any particular register I should monitor?

Sorry, but I guess I did not understand your last question.

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1028 bytes --]

On Mon, Sep 29, 2014 at 10:17:57AM -0300, Fabio Estevam wrote:
> On Mon, Sep 22, 2014 at 10:54 PM, Mark Brown <broonie@kernel.org> wrote:

> > Are you sure that the configuration that results is valid?  Typically
> > the requirements for MCLK to other clock ratios are very similar for
> > master and slave modes, it's just that it tends to be a lot more obvious
> > when things go wrong in master mode since directly visible clocks tend
> > to go wrong as opposed to performance problems.  Are the dividers that
> > we can't get configuration for purely for generating BCLK/LRCLK in
> > master mode or are they for other things?

> Tested sgtl5000 slave mode with different sampling rates and it plays well.

> Any particular register I should monitor?

Did you just listen or did you measure the performance?

> Sorry, but I guess I did not understand your last question.

I'm really not sure how to simplify it...  clearly we're skipping some
configuration here, what does it do - is it purely for generating BCLK
and LRCLK?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Michael Trimarchi]

Hi Mark

On Tue, Sep 30, 2014 at 9:29 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Sep 29, 2014 at 10:17:57AM -0300, Fabio Estevam wrote:
>> On Mon, Sep 22, 2014 at 10:54 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > Are you sure that the configuration that results is valid?  Typically
>> > the requirements for MCLK to other clock ratios are very similar for
>> > master and slave modes, it's just that it tends to be a lot more obvious
>> > when things go wrong in master mode since directly visible clocks tend
>> > to go wrong as opposed to performance problems.  Are the dividers that
>> > we can't get configuration for purely for generating BCLK/LRCLK in
>> > master mode or are they for other things?
>
>> Tested sgtl5000 slave mode with different sampling rates and it plays well.
>
>> Any particular register I should monitor?
>
> Did you just listen or did you measure the performance?
>
>> Sorry, but I guess I did not understand your last question.
>
> I'm really not sure how to simplify it...  clearly we're skipping some
> configuration here, what does it do - is it purely for generating BCLK
> and LRCLK?

Are you talking about clock_in and sigma-delta? So no  the bclk and lrclk but
the clkin of the codec.

Michael

>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 941 bytes --]

On Tue, Sep 30, 2014 at 09:38:21PM +0200, Michael Trimarchi wrote:
> On Tue, Sep 30, 2014 at 9:29 PM, Mark Brown <broonie@kernel.org> wrote:

> >> Any particular register I should monitor?

> > Did you just listen or did you measure the performance?

> >> Sorry, but I guess I did not understand your last question.

> > I'm really not sure how to simplify it...  clearly we're skipping some
> > configuration here, what does it do - is it purely for generating BCLK
> > and LRCLK?

> Are you talking about clock_in and sigma-delta? So no  the bclk and lrclk but
> the clkin of the codec.

I'm sorry but I can't parse what you're saying terribly clearly - please
bear in mind that I don't know anything about this device, that's one of
the reasons I'm having to ask questions here.  If the configuration that
is being skipped is not about generating BCLK and LRCLK then it seems
like it's going to be wanted in slave mode as well as master.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Fabio Estevam]

Hi Mark,

On Wed, Oct 1, 2014 at 3:27 PM, Mark Brown <broonie@kernel.org> wrote:
> On Tue, Sep 30, 2014 at 09:38:21PM +0200, Michael Trimarchi wrote:
>> On Tue, Sep 30, 2014 at 9:29 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> >> Any particular register I should monitor?
>
>> > Did you just listen or did you measure the performance?
>
>> >> Sorry, but I guess I did not understand your last question.
>
>> > I'm really not sure how to simplify it...  clearly we're skipping some
>> > configuration here, what does it do - is it purely for generating BCLK
>> > and LRCLK?
>
>> Are you talking about clock_in and sigma-delta? So no  the bclk and lrclk but
>> the clkin of the codec.
>
> I'm sorry but I can't parse what you're saying terribly clearly - please
> bear in mind that I don't know anything about this device, that's one of
> the reasons I'm having to ask questions here.  If the configuration that
> is being skipped is not about generating BCLK and LRCLK then it seems
> like it's going to be wanted in slave mode as well as master.

This patch does not skip any configuration. It just remove the error
case for sgtl5000 in slave mode.

Not sure why the error case was there in the first place, as I think
sgtl5000 slave mode has never been tested before.

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1032 bytes --]

On Wed, Oct 01, 2014 at 03:44:59PM -0300, Fabio Estevam wrote:
> On Wed, Oct 1, 2014 at 3:27 PM, Mark Brown <broonie@kernel.org> wrote:

> > I'm sorry but I can't parse what you're saying terribly clearly - please
> > bear in mind that I don't know anything about this device, that's one of
> > the reasons I'm having to ask questions here.  If the configuration that
> > is being skipped is not about generating BCLK and LRCLK then it seems
> > like it's going to be wanted in slave mode as well as master.

> This patch does not skip any configuration. It just remove the error
> case for sgtl5000 in slave mode.

> Not sure why the error case was there in the first place, as I think
> sgtl5000 slave mode has never been tested before.

The comment says "if mclk not satisfy the divider, use pll" and then
does in fact skip configuring the PLL (or at least a register change
which looks like it does that).  Looking at the code it seems like
there's a requirement for MCLK to be one of the standard multiples of
the sample rate.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: sgtl5000: Allow the codec to work in slave mode

[Fabio Estevam]

On Thu, Oct 2, 2014 at 2:58 PM, Mark Brown <broonie@kernel.org> wrote:

> The comment says "if mclk not satisfy the divider, use pll" and then
> does in fact skip configuring the PLL (or at least a register change
> which looks like it does that).  Looking at the code it seems like
> there's a requirement for MCLK to be one of the standard multiples of
> the sample rate.

Ok, got it now.

For sgtl5000 to operate in slave mode it can only work in "Synchronous
SYS_MCLK input" mode.

In this mode only the following rates can be supported: 256*Fs, 384*Fs, 512*Fs.

On the board I was testing this I was getting a ratio of 233, so the
original code returned -EINVAL.

So the current behaviour looks correct.

We could improve the 'PLL not supported in slave mode' error message
though putting the actual ratio we got.

Will send a patch for this shortly.