From: Cheng-yi Chiang <cychiang@chromium.org>
To: "Jernej Škrabec" <jernej.skrabec@siol.net>
Cc: "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER
MANAGEM..." <alsa-devel@alsa-project.org>,
Heiko Stuebner <heiko@sntech.de>,
Neil Armstrong <narmstrong@baylibre.com>,
David Airlie <airlied@linux.ie>,
dri-devel@lists.freedesktop.org,
linux-kernel <linux-kernel@vger.kernel.org>,
Hans Verkuil <hverkuil@xs4all.nl>,
Andrzej Hajda <a.hajda@samsung.com>,
Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
Jerome Brunet <jbrunet@baylibre.com>,
Takashi Iwai <tiwai@suse.com>,
linux-rockchip@lists.infradead.org,
Dylan Reid <dgreid@chromium.org>,
tzungbi@chromium.org, Jonas Karlman <jonas@kwiboo.se>,
Liam Girdwood <lgirdwood@gmail.com>,
Russell King <rmk+kernel@armlinux.org.uk>,
Mark Brown <broonie@kernel.org>, Jaroslav Kysela <perex@perex.cz>,
linux-arm-kernel@lists.infradead.org,
Doug Anderson <dianders@chromium.org>,
Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH v7 3/4] ASoC: rockchip_max98090: Add dai_link for HDMI
Date: Fri, 20 Sep 2019 21:16:54 +0800 [thread overview]
Message-ID: <CAFv8Nw+DxXJQMs+iv7OWoM_ZLz3hZyrwrZLW0AbLVnfHdUvP=Q@mail.gmail.com> (raw)
In-Reply-To: <1660944.WuPFEyXK2U@jernej-laptop>
On Thu, Sep 19, 2019 at 11:08 PM Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Hi!
>
> Dne četrtek, 19. september 2019 ob 15:54:49 CEST je Cheng-Yi Chiang
> napisal(a):
> > Use two dai_links. One for HDMI and one for max98090.
> > With this setup, audio can play to speaker and HDMI selectively.
> >
> > Signed-off-by: Cheng-Yi Chiang <cychiang@chromium.org>
> > ---
> > .../boot/dts/rk3288-veyron-analog-audio.dtsi | 1 +
> > sound/soc/rockchip/rockchip_max98090.c | 129 ++++++++++++++----
> > 2 files changed, 103 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> > b/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi index
> > 445270aa136e..51208d161d65 100644
> > --- a/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> > +++ b/arch/arm/boot/dts/rk3288-veyron-analog-audio.dtsi
> > @@ -17,6 +17,7 @@
> > rockchip,hp-det-gpios = <&gpio6 RK_PA5
> GPIO_ACTIVE_HIGH>;
> > rockchip,mic-det-gpios = <&gpio6 RK_PB3
> GPIO_ACTIVE_LOW>;
> > rockchip,headset-codec = <&headsetcodec>;
> > + rockchip,hdmi-codec = <&hdmi>;
> > };
> > };
> >
> > diff --git a/sound/soc/rockchip/rockchip_max98090.c
> > b/sound/soc/rockchip/rockchip_max98090.c index c5fc24675a33..6c217492bb30
> > 100644
> > --- a/sound/soc/rockchip/rockchip_max98090.c
> > +++ b/sound/soc/rockchip/rockchip_max98090.c
> > @@ -11,6 +11,7 @@
> > #include <linux/gpio.h>
> > #include <linux/of_gpio.h>
> > #include <sound/core.h>
> > +#include <sound/hdmi-codec.h>
> > #include <sound/jack.h>
> > #include <sound/pcm.h>
> > #include <sound/pcm_params.h>
> > @@ -41,6 +42,7 @@ static const struct snd_soc_dapm_widget rk_dapm_widgets[]
> > = { SND_SOC_DAPM_MIC("Headset Mic", NULL),
> > SND_SOC_DAPM_MIC("Int Mic", NULL),
> > SND_SOC_DAPM_SPK("Speaker", NULL),
> > + SND_SOC_DAPM_LINE("HDMI", NULL),
> > };
> >
> > static const struct snd_soc_dapm_route rk_audio_map[] = {
> > @@ -52,6 +54,7 @@ static const struct snd_soc_dapm_route rk_audio_map[] = {
> > {"Headphone", NULL, "HPR"},
> > {"Speaker", NULL, "SPKL"},
> > {"Speaker", NULL, "SPKR"},
> > + {"HDMI", NULL, "TX"},
> > };
> >
> > static const struct snd_kcontrol_new rk_mc_controls[] = {
> > @@ -59,6 +62,7 @@ static const struct snd_kcontrol_new rk_mc_controls[] = {
> > SOC_DAPM_PIN_SWITCH("Headset Mic"),
> > SOC_DAPM_PIN_SWITCH("Int Mic"),
> > SOC_DAPM_PIN_SWITCH("Speaker"),
> > + SOC_DAPM_PIN_SWITCH("HDMI"),
> > };
> >
> > static int rk_aif1_hw_params(struct snd_pcm_substream *substream,
> > @@ -92,38 +96,63 @@ static int rk_aif1_hw_params(struct snd_pcm_substream
> > *substream,
> >
> > ret = snd_soc_dai_set_sysclk(cpu_dai, 0, mclk,
> > SND_SOC_CLOCK_OUT);
> > - if (ret < 0) {
> > - dev_err(codec_dai->dev, "Can't set codec clock %d\n",
> ret);
> > + if (ret) {
> > + dev_err(cpu_dai->dev, "Can't set cpu dai clock %d\n",
> ret);
> > return ret;
> > }
> >
> > + /* HDMI codec dai does not need to set sysclk. */
> > + if (!strcmp(rtd->dai_link->name, "HDMI"))
> > + return 0;
> > +
> > ret = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
> > SND_SOC_CLOCK_IN);
> > - if (ret < 0) {
> > - dev_err(codec_dai->dev, "Can't set codec clock %d\n",
> ret);
> > + if (ret) {
> > + dev_err(codec_dai->dev, "Can't set codec dai clock
> %d\n", ret);
> > return ret;
> > }
> >
> > - return ret;
> > + return 0;
> > }
> >
> > static const struct snd_soc_ops rk_aif1_ops = {
> > .hw_params = rk_aif1_hw_params,
> > };
> >
> > -SND_SOC_DAILINK_DEFS(hifi,
> > - DAILINK_COMP_ARRAY(COMP_EMPTY()),
> > - DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")),
> > - DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > -
> > -static struct snd_soc_dai_link rk_dailink = {
> > - .name = "max98090",
> > - .stream_name = "Audio",
> > - .ops = &rk_aif1_ops,
> > - /* set max98090 as slave */
> > - .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> > - SND_SOC_DAIFMT_CBS_CFS,
> > - SND_SOC_DAILINK_REG(hifi),
> > +SND_SOC_DAILINK_DEFS(analog,
> > + DAILINK_COMP_ARRAY(COMP_EMPTY()),
> > + DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "HiFi")),
> > + DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > +
> > +SND_SOC_DAILINK_DEFS(hdmi,
> > + DAILINK_COMP_ARRAY(COMP_EMPTY()),
> > + DAILINK_COMP_ARRAY(COMP_CODEC(NULL, "i2s-hifi")),
> > + DAILINK_COMP_ARRAY(COMP_EMPTY()));
> > +
> > +enum {
> > + DAILINK_MAX98090,
> > + DAILINK_HDMI,
> > +};
> > +
> > +/* max98090 and HDMI codec dai_link */
> > +static struct snd_soc_dai_link rk_dailinks[] = {
> > + [DAILINK_MAX98090] = {
> > + .name = "max98090",
> > + .stream_name = "Analog",
> > + .ops = &rk_aif1_ops,
> > + /* set max98090 as slave */
> > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_CBS_CFS,
> > + SND_SOC_DAILINK_REG(analog),
> > + },
> > + [DAILINK_HDMI] = {
> > + .name = "HDMI",
> > + .stream_name = "HDMI",
> > + .ops = &rk_aif1_ops,
> > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
> > + SND_SOC_DAIFMT_CBS_CFS,
> > + SND_SOC_DAILINK_REG(hdmi),
> > + }
> > };
> >
> > static int rk_98090_headset_init(struct snd_soc_component *component);
> > @@ -136,8 +165,8 @@ static struct snd_soc_aux_dev rk_98090_headset_dev = {
> > static struct snd_soc_card snd_soc_card_rk = {
> > .name = "ROCKCHIP-I2S",
> > .owner = THIS_MODULE,
> > - .dai_link = &rk_dailink,
> > - .num_links = 1,
> > + .dai_link = rk_dailinks,
> > + .num_links = ARRAY_SIZE(rk_dailinks),
> > .aux_dev = &rk_98090_headset_dev,
> > .num_aux_devs = 1,
> > .dapm_widgets = rk_dapm_widgets,
> > @@ -173,27 +202,73 @@ static int snd_rk_mc_probe(struct platform_device
> > *pdev) int ret = 0;
> > struct snd_soc_card *card = &snd_soc_card_rk;
> > struct device_node *np = pdev->dev.of_node;
> > + struct device_node *np_analog;
> > + struct device_node *np_cpu;
> > + struct device_node *np_hdmi_codec;
> > + struct of_phandle_args args;
> >
> > /* register the soc card */
> > card->dev = &pdev->dev;
> >
> > - rk_dailink.codecs->of_node = of_parse_phandle(np,
> > - "rockchip,audio-codec", 0);
> > - if (!rk_dailink.codecs->of_node) {
> > + np_analog = of_parse_phandle(np, "rockchip,audio-codec", 0);
> > + if (!np_analog) {
> > dev_err(&pdev->dev,
> > "Property 'rockchip,audio-codec' missing or
> invalid\n");
> > return -EINVAL;
> > }
> > + rk_dailinks[DAILINK_MAX98090].codecs->of_node = np_analog;
> > +
> > + ret = of_parse_phandle_with_fixed_args(np, "rockchip,audio-codec",
> > + 0, 0, &args);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Unable to parse property 'rockchip,audio-
> codec'\n");
> > + return ret;
> > + }
> > +
> > + ret = snd_soc_get_dai_name(
> > + &args, &rk_dailinks[DAILINK_MAX98090].codecs-
> >dai_name);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to get codec dai_name\n");
> > + return ret;
> > + }
> > +
> > + np_cpu = of_parse_phandle(np, "rockchip,i2s-controller", 0);
> >
> > - rk_dailink.cpus->of_node = of_parse_phandle(np,
> > - "rockchip,i2s-controller", 0);
> > - if (!rk_dailink.cpus->of_node) {
> > + if (!np_cpu) {
> > dev_err(&pdev->dev,
> > "Property 'rockchip,i2s-controller' missing
> or invalid\n");
> > return -EINVAL;
> > }
> >
> > - rk_dailink.platforms->of_node = rk_dailink.cpus->of_node;
> > + np_hdmi_codec = of_parse_phandle(np, "rockchip,hdmi-codec", 0);
> > + if (!np_hdmi_codec) {
> > + dev_err(&pdev->dev,
> > + "Property 'rockchip,hdmi-codec' missing or
> invalid\n");
> > + return -EINVAL;
> > + }
>
> Property "rockchip,hdmi-codec" is added in this series, right? You can't make
> it mandatory, because kernel must be backward compatible with old device tree
> files and they don't have this property.
>
> Think about use case when user happily used this driver and after kernel
> update, it suddenly stops working. You can't assume that board DTB file will be
> updated along with kernel update.
>
> Just make it optional and don't expose jack functionality if it's not present.
Hi Jernej,
Thanks for the reply.
I see. Yes I can make it optional.
But it will become a little bit messy for two types of usage to share
one machine driver.
I think I will create two instances of structs for
dapm widgets,
dapm routes,
kcontrols,
dai_links
for "max98090 only" and "max98090+hdmi"
and set those fields in snd_soc_card depending on depending on the property.
These two usages can still share most of the function calls.
Hope this looks clean.
If you have a cleaner way of sharing machine driver please let me know.
I'll post an update probably next week.
Thanks a lot!
>
> Best regards,
> Jernej
>
> > +
> > + rk_dailinks[DAILINK_HDMI].codecs->of_node = np_hdmi_codec;
> > +
> > + ret = of_parse_phandle_with_fixed_args(np, "rockchip,hdmi-codec",
> > + 0, 0, &args);
> > + if (ret) {
> > + dev_err(&pdev->dev,
> > + "Unable to parse property 'rockchip,hdmi-
> codec'\n");
> > + return ret;
> > + }
> > +
> > + ret = snd_soc_get_dai_name(
> > + &args, &rk_dailinks[DAILINK_HDMI].codecs-
> >dai_name);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Unable to get hdmi codec
> dai_name\n");
> > + return ret;
> > + }
> > +
> > + rk_dailinks[DAILINK_MAX98090].cpus->of_node = np_cpu;
> > + rk_dailinks[DAILINK_MAX98090].platforms->of_node = np_cpu;
> > + rk_dailinks[DAILINK_HDMI].cpus->of_node = np_cpu;
> > + rk_dailinks[DAILINK_HDMI].platforms->of_node = np_cpu;
> >
> > rk_98090_headset_dev.codec_of_node = of_parse_phandle(np,
> > "rockchip,headset-codec", 0);
>
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-09-20 13:17 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-19 13:54 [PATCH v7 0/4] Add HDMI jack support on RK3288 Cheng-Yi Chiang
2019-09-19 13:54 ` [PATCH v7 1/4] drm: bridge: dw-hdmi: Report connector status using callback Cheng-Yi Chiang
2019-09-19 13:54 ` [PATCH v7 2/4] ASoC: rockchip-max98090: Add description for rockchip, hdmi-codec Cheng-Yi Chiang
2019-09-19 13:54 ` [PATCH v7 3/4] ASoC: rockchip_max98090: Add dai_link for HDMI Cheng-Yi Chiang
2019-09-19 15:08 ` Jernej Škrabec
2019-09-20 13:16 ` Cheng-yi Chiang [this message]
2019-09-19 13:54 ` [PATCH v7 4/4] ASoC: rockchip_max98090: Add HDMI jack support Cheng-Yi Chiang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFv8Nw+DxXJQMs+iv7OWoM_ZLz3hZyrwrZLW0AbLVnfHdUvP=Q@mail.gmail.com' \
--to=cychiang@chromium.org \
--cc=Laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=alsa-devel@alsa-project.org \
--cc=broonie@kernel.org \
--cc=daniel@ffwll.ch \
--cc=dgreid@chromium.org \
--cc=dianders@chromium.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=heiko@sntech.de \
--cc=hverkuil@xs4all.nl \
--cc=jbrunet@baylibre.com \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=lgirdwood@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=narmstrong@baylibre.com \
--cc=perex@perex.cz \
--cc=rmk+kernel@armlinux.org.uk \
--cc=tiwai@suse.com \
--cc=tzungbi@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).