| Message ID | 1463130853-25096-3-git-send-email-kernel@martin.sperl.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
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.
> 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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> 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-- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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... -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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");