Re: [PATCH V2 03/10] ASoc: mxs: add mxs-sgtl5000 machine driver

Re: [PATCH V2 03/10] ASoc: mxs: add mxs-sgtl5000 machine driver

[Shawn Guo]

On Tue, Jul 12, 2011 at 11:04:38PM +0800, Dong Aisheng wrote:
> The driver only supports playback firstly.
> For recording, as we have to use two saif instances to implement full
> duplex (playback & recording) due to hardware limitation, we need to
> figure out a good design to fit in ASoC.
> 
> Signed-off-by: Dong Aisheng <b29396-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
> Cc: Liam Girdwood <lrg-l0cyMroinI0@public.gmane.org>
> Cc: Sascha Hauer <s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  sound/soc/mxs/mxs-sgtl5000.c |  170 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 170 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/mxs/mxs-sgtl5000.c b/sound/soc/mxs/mxs-sgtl5000.c
> new file mode 100644
> index 0000000..be9b5d5
> --- /dev/null
> +++ b/sound/soc/mxs/mxs-sgtl5000.c
> @@ -0,0 +1,170 @@
> +/*
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/fsl_devices.h>
> +#include <linux/gpio.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/soc.h>
> +#include <sound/jack.h>
> +#include <sound/soc-dapm.h>
> +#include <asm/mach-types.h>
> +
> +#include "../codecs/sgtl5000.h"
> +#include "mxs-saif.h"
> +
> +static int mxs_sgtl5000_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;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	unsigned int rate = params_rate(params);
> +	u32 dai_format, mclk;
> +	int ret;
> +
> +	/* sgtl5000 does not support 512*rate when in 96000 fs */
> +	rate = params_rate(params);
> +	switch (rate) {
> +	case 96000:
> +		mclk = 256 * rate;
> +		break;
> +	default:
> +		mclk = 512 * rate;
> +		break;
> +	}
> +
> +	/* Sgtl5000 sysclk should be >= 8MHz and <= 27M */
> +	if (mclk < 8000000 || mclk > 27000000)
> +		return -EINVAL;
> +
> +	/* Set SGTL5000's SYSCLK (provided by SAIF MCLK) */
> +	ret = snd_soc_dai_set_sysclk(codec_dai, SGTL5000_SYSCLK, mclk, 0);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* The SAIF MCLK should be the same as SGTL5000_SYSCLK */
> +	ret = snd_soc_dai_set_sysclk(cpu_dai, MXS_SAIF_MCLK, mclk, 0);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* set codec to slave mode */
> +	dai_format = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> +			SND_SOC_DAIFMT_CBS_CFS;
> +
> +	/* set codec DAI configuration */
> +	ret = snd_soc_dai_set_fmt(codec_dai, dai_format);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* set cpu DAI configuration */
> +	ret = snd_soc_dai_set_fmt(cpu_dai, dai_format);
> +	if (ret)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static struct snd_soc_ops mxs_sgtl5000_hifi_ops = {
> +	.hw_params = mxs_sgtl5000_hw_params,
> +};
> +
> +static struct snd_soc_dai_link mxs_sgtl5000_dai[] = {
> +	{
> +		.name		= "HiFi",
> +		.stream_name	= "HiFi Playback",
> +		.codec_dai_name	= "sgtl5000",
> +		.codec_name	= "sgtl5000.0-000a",
> +		.cpu_dai_name	= "mxs-saif.0",
> +		.platform_name	= "mxs-pcm-audio.0",
> +		.ops		= &mxs_sgtl5000_hifi_ops,
> +	},
> +};
> +

It's not related this patch itself, and I'm not against this patch
anyhow.  I just use it as an example to start a device tree related
discussion here.  [Cc-ed Grant and devicetree-discuss list]

Currently, soc-core requires machine driver register snd_soc_dai_link
with proper codec, cpu_dai and platform device name coded, so that it
can match/bind dai link.  It works for existing non-dt devices probe,
because they can have static device id assigned by platform code to
map the actual hardware instance.

For codec device example above, if the codec sgtl5000 connects to
i2c.0, the .codec_name has to be sgtl5000.0-000a, and if i2c.1 is
used, it has to be sgtl5000.1-000a.  And it can scale a little bit
by checking machine_is_xxx() and fix it up before calling
snd_soc_register_card(), if a board uses i2c instance different than
the default one.

But when I recently migrate imx audio driver to be device tree aware,
I feel that matching/binding codec using device name in soc-core does
not fit there.  The devices in device tree probe case either have an
unused device id (-1) or a dynamically assigned one which is not
necessarily mapped to actually hardware id.  This will make device
name of codec, cpu_dai, and platform too dynamic for soc-core to
match/bind them.

Actually, I found matching cpu_dai using name does not scale in imx-ssi
case even for non-dt probe.  imx provides some flexibility on the
connection between codec and ssi.  With proper audmux configuration,
codec can be connected to either ssi.0 or ssi.1 for given board design.
That said even for given board, .cpu_dai_name can be dynamic.  This is
something the current soc-core matching/binding approach hates to see.

I'm not sure if this is something that we can solve in soc-core (with
an improved matching/binding approach), or we have to sort it out in
every single machine driver when they move to device tree.  Mark,
Grant, you can tell that's why I start discussing this thing with you
guys :)

Regards,
Shawn

> +static struct snd_soc_card mxs_sgtl5000 = {
> +	.name		= "mxs_sgtl5000",
> +	.dai_link	= mxs_sgtl5000_dai,
> +	.num_links	= ARRAY_SIZE(mxs_sgtl5000_dai),
> +};
> +
> +static int __devinit mxs_sgtl5000_probe(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = &mxs_sgtl5000;
> +	int ret;
> +
> +	/*
> +	 * Set an init clock(11.28Mhz) for sgtl5000 initialization(i2c r/w).
> +	 * The Sgtl5000 sysclk is derived from saif0 mclk and it's range
> +	 * should be >= 8MHz and <= 27M.
> +	 */
> +	ret = mxs_saif_get_mclk(0, 44100 * 256, 44100);
> +	if (ret)
> +		return -EINVAL;
> +
> +	card->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, card);
> +
> +	ret = snd_soc_register_card(card);
> +	if (ret) {
> +		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int __devexit mxs_sgtl5000_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +
> +	mxs_saif_put_mclk(0);
> +
> +	snd_soc_unregister_card(card);
> +
> +	return 0;
> +}
> +
> +static struct platform_driver mxs_sgtl5000_audio_driver = {
> +	.driver = {
> +		.name = "mxs-sgtl5000",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = mxs_sgtl5000_probe,
> +	.remove = __devexit_p(mxs_sgtl5000_remove),
> +};
> +
> +static int __init mxs_sgtl5000_init(void)
> +{
> +	return platform_driver_register(&mxs_sgtl5000_audio_driver);
> +}
> +module_init(mxs_sgtl5000_init);
> +
> +static void __exit mxs_sgtl5000_exit(void)
> +{
> +	platform_driver_unregister(&mxs_sgtl5000_audio_driver);
> +}
> +module_exit(mxs_sgtl5000_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("MXS ALSA SoC Machine driver");
> +MODULE_LICENSE("GPL");
> -- 
> 1.7.0.4
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH V2 03/10] ASoc: mxs: add mxs-sgtl5000 machine driver

[Mark Brown]

On Fri, Jul 15, 2011 at 11:11:06PM +0800, Shawn Guo wrote:

> Currently, soc-core requires machine driver register snd_soc_dai_link
> with proper codec, cpu_dai and platform device name coded, so that it
> can match/bind dai link.  It works for existing non-dt devices probe,
> because they can have static device id assigned by platform code to
> map the actual hardware instance.

There's existing device tree code for PowerPC which seems to figure this
out by doing a DT lookup in the machine driver which appears to be sane
enough.  If we can't look up a struct device via the device tree we'd
seem to have a fairly serious problem as struct devices are how we talk
about devices at runtime in general.

To be honest I've not paid too much attention to the device tree code
people are currently using since discussion around it has always
involved repeatedly going over all the reasons why we actually need
machine drivers and that gets tedious after a while.

> Actually, I found matching cpu_dai using name does not scale in imx-ssi
> case even for non-dt probe.  imx provides some flexibility on the
> connection between codec and ssi.  With proper audmux configuration,
> codec can be connected to either ssi.0 or ssi.1 for given board design.
> That said even for given board, .cpu_dai_name can be dynamic.  This is
> something the current soc-core matching/binding approach hates to see.

This isn't a scalability thing and it's not related to binding, it's
because the AUXMUX isn't properly represented as a driver.  It probably
wants to be moved over to use the soc-dsp stuff Liam's doing at the
minute.  Currently we don't tell the core about the AUDMUX at all so
it's unreasonable to expect it to do anything except a static route.