[PATCH] ASoC: add ak4554 driver

[PATCH] ASoC: add ak4554 driver

[Kuninori Morimoto]

ak4554 is very simple DA/AD converter which has no setting register.
But, playback format is SND_SOC_DAIFMT_RIGHT_J,
and, capture  format is SND_SOC_DAIFMT_LEFT_J on same bit clock, LR clock.
Because of that, snd_soc_dai_driver consists from
"playback only" and "capture only" here,
and it has software base symmetric_rates check.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/codecs/Kconfig  |    3 +
 sound/soc/codecs/Makefile |    2 +
 sound/soc/codecs/ak4554.c |  171 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 sound/soc/codecs/ak4554.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2f45f00..b4ec4ea 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -196,6 +196,9 @@ config SND_SOC_AK4104
 config SND_SOC_AK4535
 	tristate
 
+config SND_SOC_AK4554
+	tristate
+
 config SND_SOC_AK4641
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index b9e41c9..e601a47 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -11,6 +11,7 @@ snd-soc-adav80x-objs := adav80x.o
 snd-soc-ads117x-objs := ads117x.o
 snd-soc-ak4104-objs := ak4104.o
 snd-soc-ak4535-objs := ak4535.o
+snd-soc-ak4554-objs := ak4554.o
 snd-soc-ak4641-objs := ak4641.o
 snd-soc-ak4642-objs := ak4642.o
 snd-soc-ak4671-objs := ak4671.o
@@ -136,6 +137,7 @@ obj-$(CONFIG_SND_SOC_ADAV80X)  += snd-soc-adav80x.o
 obj-$(CONFIG_SND_SOC_ADS117X)	+= snd-soc-ads117x.o
 obj-$(CONFIG_SND_SOC_AK4104)	+= snd-soc-ak4104.o
 obj-$(CONFIG_SND_SOC_AK4535)	+= snd-soc-ak4535.o
+obj-$(CONFIG_SND_SOC_AK4554)	+= snd-soc-ak4554.o
 obj-$(CONFIG_SND_SOC_AK4641)	+= snd-soc-ak4641.o
 obj-$(CONFIG_SND_SOC_AK4642)	+= snd-soc-ak4642.o
 obj-$(CONFIG_SND_SOC_AK4671)	+= snd-soc-ak4671.o
diff --git a/sound/soc/codecs/ak4554.c b/sound/soc/codecs/ak4554.c
new file mode 100644
index 0000000..277f216
--- /dev/null
+++ b/sound/soc/codecs/ak4554.c
@@ -0,0 +1,171 @@
+/*
+ * ak4554.c
+ *
+ * Copyright (C) 2013 Renesas Solutions Corp.
+ * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+ *
+ *  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.
+ */
+
+#include <linux/module.h>
+#include <sound/soc.h>
+
+/*
+ * ak4554 is very simple DA/AD converter which has no setting register.
+ * But, playback format is SND_SOC_DAIFMT_RIGHT_J,
+ * and, capture  format is SND_SOC_DAIFMT_LEFT_J
+ * on same bit clock, LR clock.
+ * Because of that, snd_soc_dai_driver consists from
+ * "playback only" and "capture only" here.
+ */
+
+struct ak4554_priv {
+	int rate;
+	int usrcnt;
+};
+
+static int ak4554_dai_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	int dai_is_play = (dai->id == 0);
+	int fmt_is_play = 0;
+
+	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
+	case SND_SOC_DAIFMT_CBM_CFM:
+	case SND_SOC_DAIFMT_CBS_CFM:
+	case SND_SOC_DAIFMT_CBM_CFS:
+		dev_err(dai->dev, "can't be clock master\n");
+		return -EINVAL;
+	}
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+	case SND_SOC_DAIFMT_RIGHT_J:
+		fmt_is_play = 1;
+		break;
+	case SND_SOC_DAIFMT_LEFT_J:
+		fmt_is_play = 0;
+		break;
+	}
+
+	if (dai_is_play != fmt_is_play) {
+		dev_err(dai->dev, "format error\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ak4554_dai_hw_params(struct snd_pcm_substream *substream,
+				struct snd_pcm_hw_params *params,
+				struct snd_soc_dai *dai)
+{
+	struct ak4554_priv *priv = dev_get_drvdata(dai->dev);
+
+	if ((priv->usrcnt > 0) &&
+	    (priv->rate != params_rate(params))) {
+		dev_err(dai->dev, "asymmetric rate\n");
+		return -EIO;
+	}
+
+	priv->usrcnt++;
+	priv->rate = params_rate(params);
+
+	return 0;
+}
+
+static void ak4554_dai_shutdown(struct snd_pcm_substream *substream,
+				struct snd_soc_dai *dai)
+{
+	struct ak4554_priv *priv = dev_get_drvdata(dai->dev);
+
+	priv->usrcnt--;
+
+	if (priv->usrcnt <= 0)
+		priv->rate = 0;
+}
+
+static const struct snd_soc_dai_ops ak4554_dai_ops = {
+	.set_fmt	= ak4554_dai_set_fmt,
+	.hw_params	= ak4554_dai_hw_params,
+	.shutdown	= ak4554_dai_shutdown,
+};
+
+struct snd_soc_dai_driver ak4554_dai[] = {
+	{
+		.name = "ak4554-hifi-playback",
+		.playback = {
+			.stream_name = "Playback",
+			.channels_min = 2,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_8000_48000,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.ops = &ak4554_dai_ops,
+	}, {
+		.name = "ak4554-hifi-capture",
+		.capture = {
+			.stream_name = "Capture",
+			.channels_min = 2,
+			.channels_max = 2,
+			.rates = SNDRV_PCM_RATE_8000_48000,
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.ops = &ak4554_dai_ops,
+	}
+};
+EXPORT_SYMBOL_GPL(ak4554_dai);
+
+static int ak4554_probe(struct snd_soc_codec *codec)
+{
+	dev_info(codec->dev, "probed\n");
+	return 0;
+}
+
+static int ak4554_remove(struct snd_soc_codec *codec)
+{
+	dev_info(codec->dev, "removed\n");
+	return 0;
+}
+
+static struct snd_soc_codec_driver soc_codec_dev_ak4554 = {
+	.probe	= ak4554_probe,
+	.remove	= ak4554_remove,
+};
+
+static int ak4554_soc_probe(struct platform_device *pdev)
+{
+	struct ak4554_priv *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(struct ak4554_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&pdev->dev, priv);
+
+	return snd_soc_register_codec(&pdev->dev,
+				      &soc_codec_dev_ak4554,
+				      ak4554_dai, ARRAY_SIZE(ak4554_dai));
+}
+
+static int ak4554_soc_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_codec(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver ak4554_driver = {
+	.driver = {
+		.name = "ak4554-adc-dac",
+		.owner = THIS_MODULE,
+	},
+	.probe	= ak4554_soc_probe,
+	.remove	= ak4554_soc_remove,
+};
+module_platform_driver(ak4554_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SoC AK4554 driver");
+MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
-- 
1.7.9.5

Re: [PATCH] ASoC: add ak4554 driver

[Lars-Peter Clausen]

On 07/01/2013 08:43 AM, Kuninori Morimoto wrote:
> +
> +static int ak4554_dai_hw_params(struct snd_pcm_substream *substream,
> +				struct snd_pcm_hw_params *params,
> +				struct snd_soc_dai *dai)
> +{
> +	struct ak4554_priv *priv = dev_get_drvdata(dai->dev);
> +
> +	if ((priv->usrcnt > 0) &&
> +	    (priv->rate != params_rate(params))) {
> +		dev_err(dai->dev, "asymmetric rate\n");
> +		return -EIO;
> +	}
> +
> +	priv->usrcnt++;
> +	priv->rate = params_rate(params);

Set the symmetric_rates field of the dai driver to 1 to enforce symmetric
rates. No need to implement this by hand.

> +
> +	return 0;
> +}
> +
[...]
> +
> +struct snd_soc_dai_driver ak4554_dai[] = {
> +	{
> +		.name = "ak4554-hifi-playback",
> +		.playback = {
> +			.stream_name = "Playback",
> +			.channels_min = 2,
> +			.channels_max = 2,
> +			.rates = SNDRV_PCM_RATE_8000_48000,
> +			.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		},
> +		.ops = &ak4554_dai_ops,
> +	}, {
> +		.name = "ak4554-hifi-capture",
> +		.capture = {
> +			.stream_name = "Capture",
> +			.channels_min = 2,
> +			.channels_max = 2,
> +			.rates = SNDRV_PCM_RATE_8000_48000,
> +			.formats = SNDRV_PCM_FMTBIT_S16_LE,
> +		},
> +		.ops = &ak4554_dai_ops,
> +	}
> +};
> +EXPORT_SYMBOL_GPL(ak4554_dai);

I don't think this needs to be exported and the struct should be static

> +
> +static int ak4554_probe(struct snd_soc_codec *codec)
> +{
> +	dev_info(codec->dev, "probed\n");
> +	return 0;
> +}
> +
> +static int ak4554_remove(struct snd_soc_codec *codec)
> +{
> +	dev_info(codec->dev, "removed\n");
> +	return 0;
> +}

Remove the two functions above, there is not much point in printing these messages.

Re: [PATCH] ASoC: add ak4554 driver

[Mark Brown]

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

On Mon, Jul 01, 2013 at 05:32:29PM +0200, Lars-Peter Clausen wrote:
> On 07/01/2013 08:43 AM, Kuninori Morimoto wrote:

> > +	priv->usrcnt++;
> > +	priv->rate = params_rate(params);

> Set the symmetric_rates field of the dai driver to 1 to enforce symmetric
> rates. No need to implement this by hand.

That won't work because there are separate playback and capture DAIs at
the minute.  But I think that can be avoided, will comment when I review
tomorrow.

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

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

Re: [PATCH] ASoC: add ak4554 driver

[Kuninori Morimoto]

Hi Lars

Thank you for checking patch

> > +static int ak4554_dai_hw_params(struct snd_pcm_substream *substream,
> > +				struct snd_pcm_hw_params *params,
> > +				struct snd_soc_dai *dai)
> > +{
> > +	struct ak4554_priv *priv = dev_get_drvdata(dai->dev);
> > +
> > +	if ((priv->usrcnt > 0) &&
> > +	    (priv->rate != params_rate(params))) {
> > +		dev_err(dai->dev, "asymmetric rate\n");
> > +		return -EIO;
> > +	}
> > +
> > +	priv->usrcnt++;
> > +	priv->rate = params_rate(params);
> 
> Set the symmetric_rates field of the dai driver to 1 to enforce symmetric
> rates. No need to implement this by hand.

Hmm...
I guess this "symmetric_rates" works if dai has both .playback / .capture

 ak4554_dai
   - playback
   - capture

But, this ak4554 can't use above style, because its format is not symmetric.
 (playback is SND_SOC_DAIFMT_RIGHT_J, capture is SND_SOC_DAIFMT_LEFT_J)
So, it is using

 ak4554_dai-playback
   - playback
 ak4554_dai-capture
   - capture

I guess it can't use "symmetric_rates" on this.
We talked this on
http://comments.gmane.org/gmane.linux.alsa.devel/108346

And, I can update my log comment if it was un-understandable.

> > +struct snd_soc_dai_driver ak4554_dai[] = {
> > +	{
> > +		.name = "ak4554-hifi-playback",
> > +		.playback = {
> > +			.stream_name = "Playback",
> > +			.channels_min = 2,
> > +			.channels_max = 2,
> > +			.rates = SNDRV_PCM_RATE_8000_48000,
> > +			.formats = SNDRV_PCM_FMTBIT_S16_LE,
> > +		},
> > +		.ops = &ak4554_dai_ops,
> > +	}, {
> > +		.name = "ak4554-hifi-capture",
> > +		.capture = {
> > +			.stream_name = "Capture",
> > +			.channels_min = 2,
> > +			.channels_max = 2,
> > +			.rates = SNDRV_PCM_RATE_8000_48000,
> > +			.formats = SNDRV_PCM_FMTBIT_S16_LE,
> > +		},
> > +		.ops = &ak4554_dai_ops,
> > +	}
> > +};
> > +EXPORT_SYMBOL_GPL(ak4554_dai);
> 
> I don't think this needs to be exported and the struct should be static

Indeed.
Thank you, I will remove it in v2

> > +static int ak4554_probe(struct snd_soc_codec *codec)
> > +{
> > +	dev_info(codec->dev, "probed\n");
> > +	return 0;
> > +}
> > +
> > +static int ak4554_remove(struct snd_soc_codec *codec)
> > +{
> > +	dev_info(codec->dev, "removed\n");
> > +	return 0;
> > +}
> 
> Remove the two functions above, there is not much point in printing these messages.

OK, it will be removed in v2

Best regards
---
Kuninori Morimoto

Re: [PATCH] ASoC: add ak4554 driver

[Mark Brown]

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

On Sun, Jun 30, 2013 at 11:43:51PM -0700, Kuninori Morimoto wrote:

> ak4554 is very simple DA/AD converter which has no setting register.
> But, playback format is SND_SOC_DAIFMT_RIGHT_J,
> and, capture  format is SND_SOC_DAIFMT_LEFT_J on same bit clock, LR clock.
> Because of that, snd_soc_dai_driver consists from
> "playback only" and "capture only" here,
> and it has software base symmetric_rates check.

If the device is hard coded to these formats just use a single DAI
without a set_dai_fmt() operation, either the machine can create two
links to the DAI or things can be set up without any extra clocks so
that both formats are met.  Otherwise this looks good apart from the
things Lars-Peter already noticed.

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

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

Re: [PATCH] ASoC: add ak4554 driver

[Kuninori Morimoto]

Hi Mark

Thank you for your reply

> > ak4554 is very simple DA/AD converter which has no setting register.
> > But, playback format is SND_SOC_DAIFMT_RIGHT_J,
> > and, capture  format is SND_SOC_DAIFMT_LEFT_J on same bit clock, LR clock.
> > Because of that, snd_soc_dai_driver consists from
> > "playback only" and "capture only" here,
> > and it has software base symmetric_rates check.
> 
> If the device is hard coded to these formats just use a single DAI
> without a set_dai_fmt() operation, either the machine can create two
> links to the DAI or things can be set up without any extra clocks so
> that both formats are met.  Otherwise this looks good apart from the
> things Lars-Peter already noticed.

OK, then, it can remove self .symmetric_rates check.
It works with my current machine.

The image is like this, is it OK ?

CPU-DAI1 (plaback only fmt = RIGHT_J) --+-- ak4554 (it doesn't have set_fmt())
                                        |
CPU-DAI2 (capture only fmt = LEFT_J) ---+

Best regards
---
Kuninori Morimoto

Re: [PATCH] ASoC: add ak4554 driver

[Mark Brown]

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

On Tue, Jul 02, 2013 at 06:39:05PM -0700, Kuninori Morimoto wrote:

> The image is like this, is it OK ?

> CPU-DAI1 (plaback only fmt = RIGHT_J) --+-- ak4554 (it doesn't have set_fmt())
>                                         |
> CPU-DAI2 (capture only fmt = LEFT_J) ---+

Yes, that's what I meant.

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

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

[PATCH v2] ASoC: add ak4554 driver

[Kuninori Morimoto]

ak4554 is very simple DA/AD converter which has no setting register.
Note that it has hard coded asymmetric data format
playback : SND_SOC_DAIFMT_RIGHT_J
capture  : SND_SOC_DAIFMT_LEFT_J
This driver has single DAI and doesn't have set_fmt.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
v1 -> v2

 - single DAI
 - used symmetric_rates = 1
 - added CPU/Codec DAI image
 - git log was updtated
 - it has no .set_dai()

 sound/soc/codecs/Kconfig  |    3 ++
 sound/soc/codecs/Makefile |    2 ++
 sound/soc/codecs/ak4554.c |   79 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 84 insertions(+)
 create mode 100644 sound/soc/codecs/ak4554.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2f45f00..b4ec4ea 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -196,6 +196,9 @@ config SND_SOC_AK4104
 config SND_SOC_AK4535
 	tristate
 
+config SND_SOC_AK4554
+	tristate
+
 config SND_SOC_AK4641
 	tristate
 
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index b9e41c9..e601a47 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -11,6 +11,7 @@ snd-soc-adav80x-objs := adav80x.o
 snd-soc-ads117x-objs := ads117x.o
 snd-soc-ak4104-objs := ak4104.o
 snd-soc-ak4535-objs := ak4535.o
+snd-soc-ak4554-objs := ak4554.o
 snd-soc-ak4641-objs := ak4641.o
 snd-soc-ak4642-objs := ak4642.o
 snd-soc-ak4671-objs := ak4671.o
@@ -136,6 +137,7 @@ obj-$(CONFIG_SND_SOC_ADAV80X)  += snd-soc-adav80x.o
 obj-$(CONFIG_SND_SOC_ADS117X)	+= snd-soc-ads117x.o
 obj-$(CONFIG_SND_SOC_AK4104)	+= snd-soc-ak4104.o
 obj-$(CONFIG_SND_SOC_AK4535)	+= snd-soc-ak4535.o
+obj-$(CONFIG_SND_SOC_AK4554)	+= snd-soc-ak4554.o
 obj-$(CONFIG_SND_SOC_AK4641)	+= snd-soc-ak4641.o
 obj-$(CONFIG_SND_SOC_AK4642)	+= snd-soc-ak4642.o
 obj-$(CONFIG_SND_SOC_AK4671)	+= snd-soc-ak4671.o
diff --git a/sound/soc/codecs/ak4554.c b/sound/soc/codecs/ak4554.c
new file mode 100644
index 0000000..c1a1733
--- /dev/null
+++ b/sound/soc/codecs/ak4554.c
@@ -0,0 +1,79 @@
+/*
+ * ak4554.c
+ *
+ * Copyright (C) 2013 Renesas Solutions Corp.
+ * Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <sound/soc.h>
+
+/*
+ * ak4554 is very simple DA/AD converter which has no setting register.
+ *
+ * CAUTION
+ *
+ * ak4554 playback format is SND_SOC_DAIFMT_RIGHT_J,
+ * and,   capture  format is SND_SOC_DAIFMT_LEFT_J
+ * on same bit clock, LR clock.
+ * But, this driver doesn't have snd_soc_dai_ops :: set_fmt
+ *
+ * CPU/Codec DAI image
+ *
+ * CPU-DAI1 (plaback only fmt = RIGHT_J) --+-- ak4554
+ *					   |
+ * CPU-DAI2 (capture only fmt = LEFT_J) ---+
+ */
+
+static struct snd_soc_dai_driver ak4554_dai = {
+	.name = "ak4554-hifi",
+	.playback = {
+		.stream_name = "Playback",
+		.channels_min = 2,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+	.capture = {
+		.stream_name = "Capture",
+		.channels_min = 2,
+		.channels_max = 2,
+		.rates = SNDRV_PCM_RATE_8000_48000,
+		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+	},
+	.symmetric_rates = 1,
+};
+
+static struct snd_soc_codec_driver soc_codec_dev_ak4554 = {
+};
+
+static int ak4554_soc_probe(struct platform_device *pdev)
+{
+	return snd_soc_register_codec(&pdev->dev,
+				      &soc_codec_dev_ak4554,
+				      &ak4554_dai, 1);
+}
+
+static int ak4554_soc_remove(struct platform_device *pdev)
+{
+	snd_soc_unregister_codec(&pdev->dev);
+	return 0;
+}
+
+static struct platform_driver ak4554_driver = {
+	.driver = {
+		.name = "ak4554-adc-dac",
+		.owner = THIS_MODULE,
+	},
+	.probe	= ak4554_soc_probe,
+	.remove	= ak4554_soc_remove,
+};
+module_platform_driver(ak4554_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("SoC AK4554 driver");
+MODULE_AUTHOR("Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>");
-- 
1.7.9.5

Re: [PATCH v2] ASoC: add ak4554 driver

[Mark Brown]

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

On Wed, Jul 03, 2013 at 09:15:13PM -0700, Kuninori Morimoto wrote:
> ak4554 is very simple DA/AD converter which has no setting register.
> Note that it has hard coded asymmetric data format
> playback : SND_SOC_DAIFMT_RIGHT_J
> capture  : SND_SOC_DAIFMT_LEFT_J
> This driver has single DAI and doesn't have set_fmt.

Applied, thanks.

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

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