Using simple-card to replace kirkwood-t5325.c

Using simple-card to replace kirkwood-t5325.c

[Andrew Lunn]

Hi Folks

I'm an ALSA newbie, so if i say anything stupid, please let me know.

I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT,
using simple-card. I can get near, but i'm missing two things:

There does not appear to be a way to represent this in DT:

static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
{
	struct snd_soc_codec *codec = rtd->codec;
	struct snd_soc_dapm_context *dapm = &codec->dapm;

	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
	snd_soc_dapm_enable_pin(dapm, "Speaker");

	return 0;
}

It appears that quite a few drivers need to enable pins. It also seems
common to disable pins and to set them to NC. Is this a generic enough
feature it could be added to the DT binding?

The second thing i need is:

static int t5325_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 *codec_dai = rtd->codec_dai;
	unsigned int freq;

	freq = params_rate(params) * 256;

	return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);

}

static struct snd_soc_ops t5325_ops = {
       .hw_params = t5325_hw_params,
};


This seems a lot less common requirements. All the Marvell SoCs need
it, but not many others. So i don't think it makes sense to add it
directly to simple-card, otherwise simple-card quickly becomes
complex-card as everybody else wants there quirks adding.

Has there been any thoughts about turning simple-card.c into a
library, or adding hooks so that a driver can modify the created
dai_link structures to add in ops like this?

	 Thanks
		Andrew

Re: Using simple-card to replace kirkwood-t5325.c

[Lars-Peter Clausen]

On 04/15/2014 06:13 PM, Andrew Lunn wrote:
> Hi Folks

Adding a few people to Cc.

>
> I'm an ALSA newbie, so if i say anything stupid, please let me know.
>
> I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT,
> using simple-card. I can get near, but i'm missing two things:
>
> There does not appear to be a way to represent this in DT:
>
> static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
> {
> 	struct snd_soc_codec *codec = rtd->codec;
> 	struct snd_soc_dapm_context *dapm = &codec->dapm;
>
> 	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
> 	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
> 	snd_soc_dapm_enable_pin(dapm, "Speaker");
>
> 	return 0;
> }
>
> It appears that quite a few drivers need to enable pins.

I'm not sure where this got started, but this mostly seems to be 
cargo-culting. External pins are enabled by default, there is no need to 
call snd_soc_dapm_enable_pin() unless snd_soc_dapm_disable_pin() has been 
called before for the same pin.

> It also seems
> common to disable pins and to set them to NC. Is this a generic enough
> feature it could be added to the DT binding?

We should probably require that the DAPM routes and widgets are always 
completely specified. In this case the fully_routed flag can be set and 
unconnected pins will automatically be marked as not connected. Although it 
might be too late now to require this since there are already users of the 
simple card out in the wild which may have incomplete constraints.

>
> The second thing i need is:
>
> static int t5325_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 *codec_dai = rtd->codec_dai;
> 	unsigned int freq;
>
> 	freq = params_rate(params) * 256;
>
> 	return snd_soc_dai_set_sysclk(codec_dai, 0, freq, SND_SOC_CLOCK_IN);
>
> }
>
> static struct snd_soc_ops t5325_ops = {
>         .hw_params = t5325_hw_params,
> };
>
>
> This seems a lot less common requirements. All the Marvell SoCs need
> it, but not many others. So i don't think it makes sense to add it
> directly to simple-card, otherwise simple-card quickly becomes
> complex-card as everybody else wants there quirks adding.

Maybe the drivers can be reworked to not require this anymore. The CODEC 
driver may be able to figure this out on its own.

>
> Has there been any thoughts about turning simple-card.c into a
> library, or adding hooks so that a driver can modify the created
> dai_link structures to add in ops like this?

A few of the function that got added for the simple card are already 
available as library functions in soc-core.c. More can probably made 
available to other drivers if useful.

- Lars

Re: Using simple-card to replace kirkwood-t5325.c

[Mark Brown]

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

On Tue, Apr 15, 2014 at 08:40:49PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 06:13 PM, Andrew Lunn wrote:

> >	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
> >	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
> >	snd_soc_dapm_enable_pin(dapm, "Speaker");

> I'm not sure where this got started, but this mostly seems to be
> cargo-culting. External pins are enabled by default, there is no need to
> call snd_soc_dapm_enable_pin() unless snd_soc_dapm_disable_pin() has been
> called before for the same pin.

It started before I ever started working on ASoC.  Sometimes it's done
for documentation.

> >It also seems
> >common to disable pins and to set them to NC. Is this a generic enough
> >feature it could be added to the DT binding?

> We should probably require that the DAPM routes and widgets are always
> completely specified. In this case the fully_routed flag can be set and
> unconnected pins will automatically be marked as not connected. Although it
> might be too late now to require this since there are already users of the
> simple card out in the wild which may have incomplete constraints.

I think we can do something like say that anything that specifies off
chip widgets needs to fully specify.

> >static int t5325_hw_params(struct snd_pcm_substream *substream,
> >        	   struct snd_pcm_hw_params *params)
> >{

> >This seems a lot less common requirements. All the Marvell SoCs need
> >it, but not many others. So i don't think it makes sense to add it
> >directly to simple-card, otherwise simple-card quickly becomes
> >complex-card as everybody else wants there quirks adding.

> Maybe the drivers can be reworked to not require this anymore. The CODEC
> driver may be able to figure this out on its own.

I don't think it can, that looks like the CODEC MCLK being supplied by
the SoC (it's nothing to do with a requirement from the SoC really).
Ideally this would be handled through the clock API but that's a bit
fail at the minute for architecture neutral code.  It's a bit of a hack
but specifying the ratio in the DT (which I thought we supported in
simple-card already but don't seem to) would sidestep the issue.

> >Has there been any thoughts about turning simple-card.c into a
> >library, or adding hooks so that a driver can modify the created
> >dai_link structures to add in ops like this?

> A few of the function that got added for the simple card are already
> available as library functions in soc-core.c. More can probably made
> available to other drivers if useful.

Yes, that's the preferred approach - where it's useful.

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

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

Re: Using simple-card to replace kirkwood-t5325.c

[Andrew Lunn]

On Tue, Apr 15, 2014 at 08:40:49PM +0200, Lars-Peter Clausen wrote:
> On 04/15/2014 06:13 PM, Andrew Lunn wrote:
> >Hi Folks
> 
> Adding a few people to Cc.
> 
> >
> >I'm an ALSA newbie, so if i say anything stupid, please let me know.
> >
> >I'm trying to replace sound/soc/kirkwood/kirkwood-t5325.c with DT,
> >using simple-card. I can get near, but i'm missing two things:
> >
> >There does not appear to be a way to represent this in DT:
> >
> >static int t5325_dai_init(struct snd_soc_pcm_runtime *rtd)
> >{
> >	struct snd_soc_codec *codec = rtd->codec;
> >	struct snd_soc_dapm_context *dapm = &codec->dapm;
> >
> >	snd_soc_dapm_enable_pin(dapm, "Mic Jack");
> >	snd_soc_dapm_enable_pin(dapm, "Headphone Jack");
> >	snd_soc_dapm_enable_pin(dapm, "Speaker");
> >
> >	return 0;
> >}
> >
> >It appears that quite a few drivers need to enable pins.
> 
> I'm not sure where this got started, but this mostly seems to be
> cargo-culting. External pins are enabled by default, there is no
> need to call snd_soc_dapm_enable_pin() unless
> snd_soc_dapm_disable_pin() has been called before for the same pin.

Ah, thanks for saying this. I just tested without, and all is good.

Thanks
	Andrew

Re: Using simple-card to replace kirkwood-t5325.c

[Andrew Lunn]

> > >static int t5325_hw_params(struct snd_pcm_substream *substream,
> > >        	   struct snd_pcm_hw_params *params)
> > >{
> 
> > >This seems a lot less common requirements. All the Marvell SoCs need
> > >it, but not many others. So i don't think it makes sense to add it
> > >directly to simple-card, otherwise simple-card quickly becomes
> > >complex-card as everybody else wants there quirks adding.
> 
> > Maybe the drivers can be reworked to not require this anymore. The CODEC
> > driver may be able to figure this out on its own.
> 
> I don't think it can, that looks like the CODEC MCLK being supplied by
> the SoC (it's nothing to do with a requirement from the SoC really).
> Ideally this would be handled through the clock API but that's a bit
> fail at the minute for architecture neutral code.  It's a bit of a hack
> but specifying the ratio in the DT (which I thought we supported in
> simple-card already but don't seem to) would sidestep the issue.

I can go the hack route and add the ratio as a DT property. I just
thought i would ask now, rather than get a NACK later when i submit
the patch.

    Thanks
	Andrew