| Message ID | 1425179070-2736-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 2015-02-28 at 22:04 -0500, Yakir Yang wrote: > --- /dev/null > +++ b/sound/soc/rockchip/rockchip_hdmi_audio.c > @@ -0,0 +1,169 @@ > +/* > + * rockchip-hdmi-card.c Doesn't match the filename. Is this line needed anyway? > + * > + * ROCKCHIP ALSA SoC DAI driver for HDMI audio on rockchip processors. > + * Copyright (c) 2014, ROCKCHIP CORPORATION. All rights reserved. > + * Authors: Yakir Yang <ykk@rock-chips.com> > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.* > + * > + */ This states the license is plain GPL v2. > +MODULE_LICENSE("GPL"); So you probably want MODULE_LICENSE("GPL v2"); here. Paul Bolle
? 2015/3/2 17:07, Paul Bolle ??: > On Sat, 2015-02-28 at 22:04 -0500, Yakir Yang wrote: >> --- /dev/null >> +++ b/sound/soc/rockchip/rockchip_hdmi_audio.c >> @@ -0,0 +1,169 @@ >> +/* >> + * rockchip-hdmi-card.c > Doesn't match the filename. Is this line needed anyway? Thanks, this comment are good for read, and seems others codec driver also content this comment, so I think we can keep this comment. I will correct the file name it in next version. Thanks for you reply :) >> + * >> + * ROCKCHIP ALSA SoC DAI driver for HDMI audio on rockchip processors. >> + * Copyright (c) 2014, ROCKCHIP CORPORATION. All rights reserved. >> + * Authors: Yakir Yang <ykk@rock-chips.com> >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.* >> + * >> + */ > This states the license is plain GPL v2. Okay, thanks, correct it in next version. Thanks Yakir Yang Best regards. >> +MODULE_LICENSE("GPL"); > So you probably want > MODULE_LICENSE("GPL v2"); > > here. > > > Paul Bolle > > > >
On Sat, Feb 28, 2015 at 10:04:30PM -0500, Yakir Yang wrote: > + ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt); > + if (ret < 0) { > + dev_err(cpu_dai->dev, "failed to set cpu_dai fmt.\n"); > + return ret; > + } You've already set this in the dai_link, no need to do it again. > + dev_info(&pdev->dev, "hdmi audio init success.\n"); Please remove noisy prints like this. > +free_cpu_of_node: > + hdmi_audio_dai.cpu_of_node = NULL; > + hdmi_audio_dai.platform_of_node = NULL; > +free_priv_data: > + snd_soc_card_set_drvdata(card, NULL); > + platform_set_drvdata(pdev, NULL); > + card->dev = NULL; If any of these assignments is doing anything there's a problem with the code. > +{ > + struct snd_soc_card *card = platform_get_drvdata(pdev); > + > + snd_soc_unregister_card(card); devm_snd_soc_register_card() and you can remove this function entirely. > +static const struct of_device_id rockchip_hdmi_audio_of_match[] = { > + { .compatible = "rockchip,rk3288-hdmi-audio", }, > + {}, > +}; There is no documentation for this binding, binding documentation is mandatory. Based on the compatible string this looks like it's specific to the SoC rather than a design for a board - is the whole card part of the SoC?
Hi Mark, On 2015?03?27? 02:16, Mark Brown wrote: > On Sat, Feb 28, 2015 at 10:04:30PM -0500, Yakir Yang wrote: > >> + ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt); >> + if (ret < 0) { >> + dev_err(cpu_dai->dev, "failed to set cpu_dai fmt.\n"); >> + return ret; >> + } > You've already set this in the dai_link, no need to do it again. Okay, correct it in next v5. > + dev_info(&pdev->dev, "hdmi audio init success.\n"); > Please remove noisy prints like this. Okay, turn it to dev_debug(...) >> +free_cpu_of_node: >> + hdmi_audio_dai.cpu_of_node = NULL; >> + hdmi_audio_dai.platform_of_node = NULL; >> +free_priv_data: >> + snd_soc_card_set_drvdata(card, NULL); >> + platform_set_drvdata(pdev, NULL); >> + card->dev = NULL; > If any of these assignments is doing anything there's a problem with the > code. > Yes, when probe failed, program will goto this code. >> +{ >> + struct snd_soc_card *card = platform_get_drvdata(pdev); >> + >> + snd_soc_unregister_card(card); > devm_snd_soc_register_card() and you can remove this function entirely. do you mean that when I take devm_snd_soc_register_card() to register card, then I do not need unregister card any more(destroy with device) ? > >> +static const struct of_device_id rockchip_hdmi_audio_of_match[] = { >> + { .compatible = "rockchip,rk3288-hdmi-audio", }, >> + {}, >> +}; > There is no documentation for this binding, binding documentation is > mandatory. Based on the compatible string this looks like it's specific > to the SoC rather than a design for a board - is the whole card part of > the SoC? It's my fault, cause the dts patch have not CC you, I will correct it in next v5 Thanks :) Yakir
On Fri, Mar 27, 2015 at 09:16:17AM +0800, yakir wrote: > On 2015?03?27? 02:16, Mark Brown wrote: > >>+free_cpu_of_node: > >>+ hdmi_audio_dai.cpu_of_node = NULL; > >>+ hdmi_audio_dai.platform_of_node = NULL; > >>+free_priv_data: > >>+ snd_soc_card_set_drvdata(card, NULL); > >>+ platform_set_drvdata(pdev, NULL); > >>+ card->dev = NULL; > >If any of these assignments is doing anything there's a problem with the > >code. > Yes, when probe failed, program will goto this code. You're missing the point, these don't do anything useful. > >>+{ > >>+ struct snd_soc_card *card = platform_get_drvdata(pdev); > >>+ > >>+ snd_soc_unregister_card(card); > >devm_snd_soc_register_card() and you can remove this function entirely. > do you mean that when I take devm_snd_soc_register_card() to register card, > then I do not need unregister card any more(destroy with device) ? Yes, that is the whole point of the devm_ APIs.
diff --git a/sound/soc/rockchip/Kconfig b/sound/soc/rockchip/Kconfig index e181826..ed2b7f0 100644 --- a/sound/soc/rockchip/Kconfig +++ b/sound/soc/rockchip/Kconfig @@ -14,3 +14,12 @@ config SND_SOC_ROCKCHIP_I2S Say Y or M if you want to add support for I2S driver for Rockchip I2S device. The device supports upto maximum of 8 channels each for play and record. + +config SND_SOC_ROCKCHIP_HDMI_AUDIO + tristate "ASoC support for Rockchip HDMI audio" + depends on SND_SOC_ROCKCHIP + select SND_SOC_ROCKCHIP_I2S + select SND_SOC_DW_HDMI_AUDIO + help + Say Y or M here if you want to add support for SoC audio on Rockchip + HDMI, such as rk3288 hdmi. diff --git a/sound/soc/rockchip/Makefile b/sound/soc/rockchip/Makefile index b921909..b9185b3 100644 --- a/sound/soc/rockchip/Makefile +++ b/sound/soc/rockchip/Makefile @@ -1,4 +1,6 @@ # ROCKCHIP Platform Support snd-soc-i2s-objs := rockchip_i2s.o +snd-soc-rockchip-hdmi-audio-objs := rockchip_hdmi_audio.o obj-$(CONFIG_SND_SOC_ROCKCHIP_I2S) += snd-soc-i2s.o +obj-$(CONFIG_SND_SOC_ROCKCHIP_HDMI_AUDIO) += snd-soc-rockchip-hdmi-audio.o diff --git a/sound/soc/rockchip/rockchip_hdmi_audio.c b/sound/soc/rockchip/rockchip_hdmi_audio.c new file mode 100644 index 0000000..cf95037 --- /dev/null +++ b/sound/soc/rockchip/rockchip_hdmi_audio.c @@ -0,0 +1,169 @@ +/* + * rockchip-hdmi-card.c + * + * ROCKCHIP ALSA SoC DAI driver for HDMI audio on rockchip processors. + * Copyright (c) 2014, ROCKCHIP CORPORATION. All rights reserved. + * Authors: Yakir Yang <ykk@rock-chips.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope 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, see <http://www.gnu.org/licenses/>.* + * + */ +#include <linux/module.h> +#include <linux/platform_device.h> + +#include <sound/soc.h> +#include <sound/pcm.h> +#include <sound/core.h> +#include <sound/pcm_params.h> + +#include "rockchip_i2s.h" + +#define DRV_NAME "rockchip-hdmi-audio" + +static int rockchip_hdmi_audio_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 dai_fmt = rtd->dai_link->dai_fmt; + int mclk, ret; + + switch (params_rate(params)) { + case 8000: + case 16000: + case 24000: + case 32000: + case 48000: + case 64000: + case 96000: + mclk = 12288000; + break; + case 11025: + case 22050: + case 44100: + case 88200: + mclk = 11289600; + break; + default: + return -EINVAL; + } + + ret = snd_soc_dai_set_fmt(cpu_dai, dai_fmt); + if (ret < 0) { + dev_err(cpu_dai->dev, "failed to set cpu_dai fmt.\n"); + return ret; + } + + ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk, SND_SOC_CLOCK_OUT); + if (ret < 0) { + dev_err(cpu_dai->dev, "failed to set cpu_dai sysclk.\n"); + return ret; + } + + return 0; +} + +static struct snd_soc_ops hdmi_audio_dai_ops = { + .hw_params = rockchip_hdmi_audio_hw_params, +}; + +static struct snd_soc_dai_link hdmi_audio_dai = { + .name = "RockchipHDMI", + .stream_name = "RockchipHDMI", + .codec_name = "dw-hdmi-audio", + .codec_dai_name = "dw-hdmi-hifi", + .ops = &hdmi_audio_dai_ops, + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF | + SND_SOC_DAIFMT_CBS_CFS, +}; + +static struct snd_soc_card rockchip_hdmi_audio_card = { + .name = "RockchipHDMI", + .owner = THIS_MODULE, + .dai_link = &hdmi_audio_dai, + .num_links = 1, +}; + +static int rockchip_hdmi_audio_probe(struct platform_device *pdev) +{ + struct snd_soc_card *card = &rockchip_hdmi_audio_card; + struct device_node *np = pdev->dev.of_node; + int ret; + + card->dev = &pdev->dev; + platform_set_drvdata(pdev, card); + + hdmi_audio_dai.cpu_of_node = of_parse_phandle(np, "i2s-controller", 0); + if (!hdmi_audio_dai.cpu_of_node) { + dev_err(&pdev->dev, "Property 'i2s-controller' missing !\n"); + goto free_priv_data; + } + + hdmi_audio_dai.platform_of_node = hdmi_audio_dai.cpu_of_node; + + ret = snd_soc_register_card(card); + if (ret) { + dev_err(&pdev->dev, "register card failed (%d)\n", ret); + card->dev = NULL; + goto free_cpu_of_node; + } + + dev_info(&pdev->dev, "hdmi audio init success.\n"); + + return 0; + +free_cpu_of_node: + hdmi_audio_dai.cpu_of_node = NULL; + hdmi_audio_dai.platform_of_node = NULL; +free_priv_data: + snd_soc_card_set_drvdata(card, NULL); + platform_set_drvdata(pdev, NULL); + card->dev = NULL; + + return ret; +} + +static int rockchip_hdmi_audio_remove(struct platform_device *pdev) +{ + struct snd_soc_card *card = platform_get_drvdata(pdev); + + snd_soc_unregister_card(card); + snd_soc_card_set_drvdata(card, NULL); + platform_set_drvdata(pdev, NULL); + card->dev = NULL; + + return 0; +} + +static const struct of_device_id rockchip_hdmi_audio_of_match[] = { + { .compatible = "rockchip,rk3288-hdmi-audio", }, + {}, +}; + +static struct platform_driver rockchip_hdmi_audio_driver = { + .driver = { + .name = DRV_NAME, + .owner = THIS_MODULE, + .pm = &snd_soc_pm_ops, + .of_match_table = rockchip_hdmi_audio_of_match, + }, + .probe = rockchip_hdmi_audio_probe, + .remove = rockchip_hdmi_audio_remove, +}; +module_platform_driver(rockchip_hdmi_audio_driver); + +MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); +MODULE_DESCRIPTION("Rockchip HDMI Audio ASoC Interface"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" DRV_NAME); +MODULE_DEVICE_TABLE(of, rockchip_hdmi_audio_of_match);
Add a sound driver that combines rockchip-i2s cpu_dai and dw-hdmi-codec as codec_dai to provide hdmi audio output on rk3288 platforms. Signed-off-by: Yakir Yang <ykk@rock-chips.com> --- Changes in v4: - Add ".pm = &snd_soc_pm_ops," Changes in v3: - Delete the operation of jack in rockchip-hdmi-audio driver, get ready to switch to simple-audio-card driver. Changes in v2: - give "codec-name" & "codec-dai-name" an const name sound/soc/rockchip/Kconfig | 9 ++ sound/soc/rockchip/Makefile | 2 + sound/soc/rockchip/rockchip_hdmi_audio.c | 169 +++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 sound/soc/rockchip/rockchip_hdmi_audio.c