diff mbox

[2/2] ASoC: Add support for HifiBerry DAC

Message ID 1463130853-25096-3-git-send-email-kernel@martin.sperl.org (mailing list archive)
State New, archived
Headers show

Commit Message

Martin Sperl May 13, 2016, 9:14 a.m. UTC 
From: Florian Meier <florian.meier@koalo.de>

This adds a machine driver for the HifiBerry DAC.
It is a sound card that can be stacked onto the Raspberry Pi.

Signed-off-by: Florian Meier <florian.meier@koalo.de>

Changes to original patch by Florian Meier:
* added .owner to struct snd_rpi_hifiberry_dac (ref-count)

Signed-off-by: Matthias Reichl <hias@horus.com>

Changes to original patch by Florian Meier:
* change to use BCM2835 in config keys instead of bcm2708
* fixed checkpath errors (spaces, indentation)
* added dt-binding documentation

Signed-off-by: Martin Sperl <kernel@martin.sperl.org>
---
 .../bindings/sound/hifiberry,hifiberry-dac.txt     |  12 ++
 sound/soc/bcm/Kconfig                              |   7 ++
 sound/soc/bcm/Makefile                             |   3 +-
 sound/soc/bcm/hifiberry_dac.c                      | 126 +++++++++++++++++++++
 4 files changed, 147 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/sound/hifiberry,hifiberry-dac.txt
 create mode 100644 sound/soc/bcm/hifiberry_dac.c

--
2.1.4

Comments

Mark Brown May 13, 2016, 10:54 a.m. UTC | #1 
On Fri, May 13, 2016 at 09:14:13AM +0000, kernel@martin.sperl.org wrote:

> +static int snd_rpi_hifiberry_dac_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	return 0;
> +}

Remove empty functions.  Either they are redundant or you really need to
do something in them.

> +static struct snd_soc_dai_link snd_rpi_hifiberry_dac_dai[] = {
> +{
> +	.name		= "HifiBerry DAC",
> +	.stream_name	= "HifiBerry DAC HiFi",
> +	.cpu_dai_name	= "bcm2708-i2s.0",
> +	.codec_dai_name	= "pcm5102a-hifi",
> +	.platform_name	= "bcm2708-i2s.0",
> +	.codec_name	= "pcm5102a-codec",

I would expect all this to be done with DT references in a DT driver
rather than hard coding static names - look at how simple-card does this.
You could almost use simple-card here but the BCLK ratio requirement is
probably a bit much, I'm not immediately seeing a nice way to specify
the ratio there since it depends on the sample width.
Martin Sperl May 13, 2016, 12:20 p.m. UTC | #2 
> On 13.05.2016, at 12:54, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, May 13, 2016 at 09:14:13AM +0000, kernel@martin.sperl.org wrote:
> 
>> +static int snd_rpi_hifiberry_dac_init(struct snd_soc_pcm_runtime *rtd)
>> +{
>> +	return 0;
>> +}
> 
> Remove empty functions.  Either they are redundant or you really need to
> do something in them.
> 
Sure this can get removed.

As said - it is just pushing the patches upstream,
so I did not even touch it. 

>> +static struct snd_soc_dai_link snd_rpi_hifiberry_dac_dai[] = {
>> +{
>> +	.name		= "HifiBerry DAC",
>> +	.stream_name	= "HifiBerry DAC HiFi",
>> +	.cpu_dai_name	= "bcm2708-i2s.0",
>> +	.codec_dai_name	= "pcm5102a-hifi",
>> +	.platform_name	= "bcm2708-i2s.0",
>> +	.codec_name	= "pcm5102a-codec",
> 
> I would expect all this to be done with DT references in a DT driver
> rather than hard coding static names - look at how simple-card does this.
> You could almost use simple-card here but the BCLK ratio requirement is
> probably a bit much, I'm not immediately seeing a nice way to specify
> the ratio there since it depends on the sample width.

Actually it is just: <sample-width> * <number of channels>
where number of channels is fixed to 2 for bcm2835-i2s.

So maybe that already happens automatically in the framework?

I will need to test it...

I also know that some of the audio guys working on the rpi
would like to be able to configure BCLK ratios based on both:
sample frequency and sample_width.

Basically they want to control the clock divider/clock parent
trying to avoid fractional dividers.

Something like this could get added to the core?
Is there interest in getting something like this?

Could look like this:
bclk-ratios = 
  /* for 96kHz at 16bit/channel use bclk-ratio 40 */
  <96000 16 40>, 
  /* for 48kHz at 32bit/channel use bclk-ratio 80 */
  <48000 16 80>,
  /* for any (other) sample frequency at 16 bit use bclk-ratio 32 */
  <0 16 32>,
  /* for any (other) sample frequency at 32 bit use bclk-ratio 64 */
  <0 32 64>;

but there could also be other approaches as well,
how this could get described in the device tree.
Mark Brown May 13, 2016, 1:21 p.m. UTC | #3 
On Fri, May 13, 2016 at 02:20:57PM +0200, Martin Sperl wrote:
> > On 13.05.2016, at 12:54, Mark Brown <broonie@kernel.org> wrote:

> > You could almost use simple-card here but the BCLK ratio requirement is
> > probably a bit much, I'm not immediately seeing a nice way to specify
> > the ratio there since it depends on the sample width.

> Actually it is just: <sample-width> * <number of channels>
> where number of channels is fixed to 2 for bcm2835-i2s.

> So maybe that already happens automatically in the framework?

No, it's not handled.  Lots of devices don't care or have different
requirements (eg, 256fs).  The tricky bit with specifying it is that the
ratio depends on the sample width here but it might depend on something
else with another device, or the number of channels might vary.  That
should be doable with DT but it feels like more trouble than it's
reasonable to ask you to take here.

> Something like this could get added to the core?
> Is there interest in getting something like this?

> Could look like this:
> bclk-ratios = 
>   /* for 96kHz at 16bit/channel use bclk-ratio 40 */
>   <96000 16 40>, 
>   /* for 48kHz at 32bit/channel use bclk-ratio 80 */
>   <48000 16 80>,
>   /* for any (other) sample frequency at 16 bit use bclk-ratio 32 */
>   <0 16 32>,
>   /* for any (other) sample frequency at 32 bit use bclk-ratio 64 */
>   <0 32 64>;

> but there could also be other approaches as well,
> how this could get described in the device tree.

You may well end up with something like that, yeah.
Martin Sperl May 13, 2016, 4:21 p.m. UTC | #4 
> On 13.05.2016, at 15:21, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, May 13, 2016 at 02:20:57PM +0200, Martin Sperl wrote:
>>> On 13.05.2016, at 12:54, Mark Brown <broonie@kernel.org> wrote:
> 
>>> You could almost use simple-card here but the BCLK ratio requirement is
>>> probably a bit much, I'm not immediately seeing a nice way to specify
>>> the ratio there since it depends on the sample width.
> 
>> Actually it is just: <sample-width> * <number of channels>
>> where number of channels is fixed to 2 for bcm2835-i2s.
> 
>> So maybe that already happens automatically in the framework?
> 
> No, it's not handled.  Lots of devices don't care or have different
> requirements (eg, 256fs).  The tricky bit with specifying it is that the
> ratio depends on the sample width here but it might depend on something
> else with another device, or the number of channels might vary.  That
> should be doable with DT but it feels like more trouble than it's
> reasonable to ask you to take here.
> 
>> Something like this could get added to the core?
>> Is there interest in getting something like this?
> 
>> Could look like this:
>> bclk-ratios = 
>>  /* for 96kHz at 16bit/channel use bclk-ratio 40 */
>>  <96000 16 40>, 
>>  /* for 48kHz at 32bit/channel use bclk-ratio 80 */
>>  <48000 16 80>,
>>  /* for any (other) sample frequency at 16 bit use bclk-ratio 32 */
>>  <0 16 32>,
>>  /* for any (other) sample frequency at 32 bit use bclk-ratio 64 */
>>  <0 32 64>;
> 
>> but there could also be other approaches as well,
>> how this could get described in the device tree.
> 
> You may well end up with something like that, yeah.
would something like this be a possibility:

sound {
  compatible = "simple-audio-card";
  simple-audio-card,name = "HifiBerry DAC";
...
  /* for 32bit@48k set bclk size to 40bit/channel = 80bits effectively */
  hw-params-filter@0 {
    match-rate = <48000>;
    match-sample-bits = <32>;
    match-channels = <2>;
    action = "set-fixed-bclk-ratio";
    action-value = <40>;
  };

  /* for 16bit@96k set bclk size to 20bit/channel = 40bits effectively */
  hw-params-filter@1 {
    match-rate = <96000>;
    match-sample-bits = <16>;
    match-channels = <2>;
    action = "set-fixed-bclk-ratio";
    action-value = <40>;
  };
  /* for all others take sample_bits * channel */
  hw-params-filter@999 {
    action = "set-bclk-ratio-multiple-sample-bits-channels";
  };
};

The title of nodes and properties can easily get changed.
Additional "match-*” properties can also get added easily.

The biggest question is about "action" or maybe “actions” 
(in the sense of an array, so that there can be multiple actions)?
Then if this should remain a string or should it be an id
(that we would need to define in an include)…

More actions could get added easily - there could even be a
framework that would allow drivers to specify more actions
inside different drivers that are assigned here.

I would think that the “first” one matches - order would be
enforced based on node name.

This would just get added to asoc_simple_card_hw_params.

This does not seem too complicated to create.

Thanks,
	Martin
Martin Sperl May 20, 2016, 12:28 p.m. UTC | #5 
> On 13.05.2016, at 15:21, Mark Brown <broonie@kernel.org> wrote:
> 
> On Fri, May 13, 2016 at 02:20:57PM +0200, Martin Sperl wrote:
>> Could look like this:
>> bclk-ratios = 
>>  /* for 96kHz at 16bit/channel use bclk-ratio 40 */
>>  <96000 16 40>, 
>>  /* for 48kHz at 32bit/channel use bclk-ratio 80 */
>>  <48000 16 80>,
>>  /* for any (other) sample frequency at 16 bit use bclk-ratio 32 */
>>  <0 16 32>,
>>  /* for any (other) sample frequency at 32 bit use bclk-ratio 64 */
>>  <0 32 64>;
> 
>> but there could also be other approaches as well,
>> how this could get described in the device tree.
> 
> You may well end up with something like that, yeah.

I am submitting a patch for this now...
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/sound/hifiberry,hifiberry-dac.txt b/Documentation/devicetree/bindings/sound/hifiberry,hifiberry-dac.txt
new file mode 100644
index 0000000..88ba248
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/hifiberry,hifiberry-dac.txt
@@ -0,0 +1,12 @@ 
+* Hifiberry-dac soundcard-hat for raspberry-pi
+
+Required properties:
+- compatible: "hifiberry,hifiberry-dac"
+- i2s-controller: phandle of the bcm2835 i2s controller
+
+Example:
+
+sound {
+      compatible = "hifiberry,hifiberry-dac";
+      i2s-controller = <&i2s>;
+};
diff --git a/sound/soc/bcm/Kconfig b/sound/soc/bcm/Kconfig
index 6a834e1..e912774 100644
--- a/sound/soc/bcm/Kconfig
+++ b/sound/soc/bcm/Kconfig
@@ -7,3 +7,10 @@  config SND_BCM2835_SOC_I2S
 	  Say Y or M if you want to add support for codecs attached to
 	  the BCM2835 I2S interface. You will also need
 	  to select the audio interfaces to support below.
+
+config SND_BCM2835_SOC_HIFIBERRY_DAC
+        tristate "Support for HifiBerry DAC"
+        depends on SND_BCM2835_SOC_I2S
+        select SND_SOC_PCM5102A
+        help
+         Say Y or M if you want to add support for HifiBerry DAC.
diff --git a/sound/soc/bcm/Makefile b/sound/soc/bcm/Makefile
index bc816b7..bc6e249 100644
--- a/sound/soc/bcm/Makefile
+++ b/sound/soc/bcm/Makefile
@@ -1,5 +1,6 @@ 
 # BCM2835 Platform Support
 snd-soc-bcm2835-i2s-objs := bcm2835-i2s.o
+snd-soc-hifiberry-dac-objs := hifiberry_dac.o

 obj-$(CONFIG_SND_BCM2835_SOC_I2S) += snd-soc-bcm2835-i2s.o
-
+obj-$(CONFIG_SND_BCM2835_SOC_HIFIBERRY_DAC) += snd-soc-hifiberry-dac.o
diff --git a/sound/soc/bcm/hifiberry_dac.c b/sound/soc/bcm/hifiberry_dac.c
new file mode 100644
index 0000000..9e79bbf
--- /dev/null
+++ b/sound/soc/bcm/hifiberry_dac.c
@@ -0,0 +1,126 @@ 
+/*
+ * ASoC Driver for HifiBerry DAC
+ *
+ * Author:	Florian Meier <florian.meier@koalo.de>
+ *		Copyright 2013
+ *
+ * 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.
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <sound/core.h>
+#include <sound/pcm.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+#include <sound/jack.h>
+
+static int snd_rpi_hifiberry_dac_init(struct snd_soc_pcm_runtime *rtd)
+{
+	return 0;
+}
+
+static int snd_rpi_hifiberry_dac_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 *cpu_dai = rtd->cpu_dai;
+
+	unsigned int sample_bits =
+		snd_pcm_format_physical_width(params_format(params));
+
+	return snd_soc_dai_set_bclk_ratio(cpu_dai, sample_bits * 2);
+}
+
+/* machine stream operations */
+static struct snd_soc_ops snd_rpi_hifiberry_dac_ops = {
+	.hw_params = snd_rpi_hifiberry_dac_hw_params,
+};
+
+static struct snd_soc_dai_link snd_rpi_hifiberry_dac_dai[] = {
+{
+	.name		= "HifiBerry DAC",
+	.stream_name	= "HifiBerry DAC HiFi",
+	.cpu_dai_name	= "bcm2708-i2s.0",
+	.codec_dai_name	= "pcm5102a-hifi",
+	.platform_name	= "bcm2708-i2s.0",
+	.codec_name	= "pcm5102a-codec",
+	.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				SND_SOC_DAIFMT_CBS_CFS,
+	.ops		= &snd_rpi_hifiberry_dac_ops,
+	.init		= snd_rpi_hifiberry_dac_init,
+},
+};
+
+/* audio machine driver */
+static struct snd_soc_card snd_rpi_hifiberry_dac = {
+	.name         = "snd_rpi_hifiberry_dac",
+	.owner        = THIS_MODULE,
+	.dai_link     = snd_rpi_hifiberry_dac_dai,
+	.num_links    = ARRAY_SIZE(snd_rpi_hifiberry_dac_dai),
+};
+
+static int snd_rpi_hifiberry_dac_probe(struct platform_device *pdev)
+{
+	struct device_node *i2s_node;
+	struct snd_soc_dai_link *dai;
+	int ret = 0;
+
+	snd_rpi_hifiberry_dac.dev = &pdev->dev;
+
+	if (pdev->dev.of_node) {
+		dai = &snd_rpi_hifiberry_dac_dai[0];
+		i2s_node = of_parse_phandle(pdev->dev.of_node,
+					    "i2s-controller", 0);
+
+		if (i2s_node) {
+			dai->cpu_dai_name = NULL;
+			dai->cpu_of_node = i2s_node;
+			dai->platform_name = NULL;
+			dai->platform_of_node = i2s_node;
+		}
+	}
+
+	ret = snd_soc_register_card(&snd_rpi_hifiberry_dac);
+	if (ret)
+		dev_err(&pdev->dev,
+			"snd_soc_register_card() failed: %d\n", ret);
+
+	return ret;
+}
+
+static int snd_rpi_hifiberry_dac_remove(struct platform_device *pdev)
+{
+	return snd_soc_unregister_card(&snd_rpi_hifiberry_dac);
+}
+
+static const struct of_device_id snd_rpi_hifiberry_dac_of_match[] = {
+	{ .compatible = "hifiberry,hifiberry-dac", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, snd_rpi_hifiberry_dac_of_match);
+
+static struct platform_driver snd_rpi_hifiberry_dac_driver = {
+	.driver = {
+		.name   = "snd-hifiberry-dac",
+		.owner  = THIS_MODULE,
+		.of_match_table = snd_rpi_hifiberry_dac_of_match,
+	},
+	.probe	= snd_rpi_hifiberry_dac_probe,
+	.remove	= snd_rpi_hifiberry_dac_remove,
+};
+
+module_platform_driver(snd_rpi_hifiberry_dac_driver);
+
+MODULE_AUTHOR("Florian Meier <florian.meier@koalo.de>");
+MODULE_DESCRIPTION("ASoC Driver for HifiBerry DAC");
+MODULE_LICENSE("GPL v2");