Message-ID: <CAFv8Nw+DxXJQMs+iv7OWoM_ZLz3hZyrwrZLW0AbLVnfHdUvP=Q@mail.gmail.com>
Date:   Fri, 20 Sep 2019 21:16:54 +0800
From:   Cheng-yi Chiang <cychiang@...omium.org>
To:     Jernej Škrabec <jernej.skrabec@...l.net>
Cc:     linux-kernel <linux-kernel@...r.kernel.org>,
        Neil Armstrong <narmstrong@...libre.com>,
        Jerome Brunet <jbrunet@...libre.com>,
        Jonas Karlman <jonas@...boo.se>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Takashi Iwai <tiwai@...e.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        Russell King <rmk+kernel@...linux.org.uk>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Heiko Stuebner <heiko@...ech.de>,
        Doug Anderson <dianders@...omium.org>,
        Dylan Reid <dgreid@...omium.org>, tzungbi@...omium.org,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>, dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v7 3/4] ASoC: rockchip_max98090: Add dai_link for HDMI

On Thu, Sep 19, 2019 at 11:08 PM Jernej Škrabec <jernej.skrabec@...l.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@...omium.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);
>
>
>
>

Powered by blists - more mailing lists