| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFv8NwKG9nKXLJY7L79qh=HxxPgY=h_xd8EoGN90uuXFQAz2QQ@mail.gmail.com>
Date: Thu, 27 Aug 2020 16:28:07 +0800
From: Cheng-yi Chiang <cychiang@...omium.org>
To: Stephan Gerhold <stephan@...hold.net>
Cc: Taniya Das <tdas@...eaurora.org>,
"moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..."
<alsa-devel@...a-project.org>,
Banajit Goswami <bgoswami@...eaurora.org>,
Takashi Iwai <tiwai@...e.com>,
Rohit kumar <rohitkr@...eaurora.org>,
Patrick Lai <plai@...eaurora.org>,
Ajit Pandey <ajitp@...eaurora.org>,
Tzung-Bi Shih <tzungbi@...gle.com>,
Andy Gross <agross@...nel.org>,
Dylan Reid <dgreid@...omium.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>, Tzung-Bi Shih <tzungbi@...omium.org>,
linux-arm-msm <linux-arm-msm@...r.kernel.org>,
Rob Herring <robh+dt@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Linux ARM <linux-arm-kernel@...ts.infradead.org>,
Doug Anderson <dianders@...omium.org>,
Liam Girdwood <lgirdwood@...il.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Mark Brown <broonie@...nel.org>,
Srini Kandagatla <srinivas.kandagatla@...aro.org>
Subject: Re: [PATCH v6 2/2] ASoC: qcom: sc7180: Add machine driver for sound
card registration
On Wed, Aug 26, 2020 at 10:48 PM Stephan Gerhold <stephan@...hold.net> wrote:
>
> On Wed, Aug 26, 2020 at 07:04:54PM +0800, Cheng-Yi Chiang wrote:
> > From: Ajit Pandey <ajitp@...eaurora.org>
> >
> > Add new driver to register sound card on sc7180 trogdor board and
> > do the required configuration for lpass cpu dai and external codecs
> > connected over MI2S interfaces.
> >
> > Signed-off-by: Ajit Pandey <ajitp@...eaurora.org>
> > Signed-off-by: Cheng-Yi Chiang <cychiang@...omium.org>
> > Reviewed-by: Tzung-Bi Shih <tzungbi@...gle.com>
> > ---
> > sound/soc/qcom/Kconfig | 12 ++
> > sound/soc/qcom/Makefile | 2 +
> > sound/soc/qcom/sc7180.c | 244 ++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 258 insertions(+)
> > create mode 100644 sound/soc/qcom/sc7180.c
> >
> > diff --git a/sound/soc/qcom/Kconfig b/sound/soc/qcom/Kconfig
> > index a607ace8b089..63678b746299 100644
> > --- a/sound/soc/qcom/Kconfig
> > +++ b/sound/soc/qcom/Kconfig
> > @@ -116,4 +116,16 @@ config SND_SOC_SDM845
> > SDM845 SoC-based systems.
> > Say Y if you want to use audio device on this SoCs.
> >
> > +config SND_SOC_SC7180
> > + tristate "SoC Machine driver for SC7180 boards"
> > + depends on SND_SOC_QCOM
> > + select SND_SOC_QCOM_COMMON
> > + select SND_SOC_LPASS_SC7180
> > + select SND_SOC_MAX98357A
> > + select SND_SOC_RT5682
> > + help
> > + To add support for audio on Qualcomm Technologies Inc.
> > + SC7180 SoC-based systems.
> > + Say Y if you want to use audio device on this SoCs.
> > +
> > endif #SND_SOC_QCOM
> > diff --git a/sound/soc/qcom/Makefile b/sound/soc/qcom/Makefile
> > index 7972c9479ab0..0cdcbf367ef1 100644
> > --- a/sound/soc/qcom/Makefile
> > +++ b/sound/soc/qcom/Makefile
> > @@ -17,12 +17,14 @@ snd-soc-storm-objs := storm.o
> > snd-soc-apq8016-sbc-objs := apq8016_sbc.o
> > snd-soc-apq8096-objs := apq8096.o
> > snd-soc-sdm845-objs := sdm845.o
> > +snd-soc-sc7180-objs := sc7180.o
> > snd-soc-qcom-common-objs := common.o
> >
> > obj-$(CONFIG_SND_SOC_STORM) += snd-soc-storm.o
> > obj-$(CONFIG_SND_SOC_APQ8016_SBC) += snd-soc-apq8016-sbc.o
> > obj-$(CONFIG_SND_SOC_MSM8996) += snd-soc-apq8096.o
> > obj-$(CONFIG_SND_SOC_SDM845) += snd-soc-sdm845.o
> > +obj-$(CONFIG_SND_SOC_SC7180) += snd-soc-sc7180.o
> > obj-$(CONFIG_SND_SOC_QCOM_COMMON) += snd-soc-qcom-common.o
> >
> > #DSP lib
> > diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> > new file mode 100644
> > index 000000000000..7849376f63ba
> > --- /dev/null
> > +++ b/sound/soc/qcom/sc7180.c
> > @@ -0,0 +1,244 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (c) 2020, The Linux Foundation. All rights reserved.
> > +//
> > +// sc7180.c -- ALSA SoC Machine driver for SC7180
> > +
> > +#include <dt-bindings/sound/sc7180-lpass.h>
> > +#include <linux/module.h>
> > +#include <linux/of_device.h>
> > +#include <linux/platform_device.h>
> > +#include <sound/core.h>
> > +#include <sound/jack.h>
> > +#include <sound/pcm.h>
> > +#include <sound/pcm_params.h>
> > +#include <sound/soc.h>
> > +#include <uapi/linux/input-event-codes.h>
> > +
> > +#include "../codecs/rt5682.h"
> > +#include "common.h"
> > +#include "lpass.h"
> > +
> > +#define DEFAULT_SAMPLE_RATE_48K 48000
> > +#define DEFAULT_MCLK_RATE 19200000
> > +#define RT5682_PLL1_FREQ (48000 * 512)
> > +
> > +struct sc7180_snd_data {
> > + struct snd_soc_jack jack;
> > + u32 pri_mi2s_clk_count;
> > +};
> > +
> > +static void sc7180_jack_free(struct snd_jack *jack)
> > +{
> > + struct snd_soc_component *component = jack->private_data;
> > +
> > + snd_soc_component_set_jack(component, NULL, NULL);
> > +}
> > +
> > +static int sc7180_headset_init(struct snd_soc_component *component)
> > +{
> > + struct snd_soc_card *card = component->card;
> > + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> > + struct snd_jack *jack;
> > + int rval;
> > +
> > + rval = snd_soc_card_jack_new(
> > + card, "Headset Jack",
> > + SND_JACK_HEADSET |
> > + SND_JACK_HEADPHONE |
> > + SND_JACK_BTN_0 | SND_JACK_BTN_1 |
> > + SND_JACK_BTN_2 | SND_JACK_BTN_3,
> > + &pdata->jack, NULL, 0);
> > +
> > + if (rval < 0) {
> > + dev_err(card->dev, "Unable to add Headset Jack\n");
> > + return rval;
> > + }
> > +
> > + jack = pdata->jack.jack;
> > +
> > + snd_jack_set_key(jack, SND_JACK_BTN_0, KEY_PLAYPAUSE);
> > + snd_jack_set_key(jack, SND_JACK_BTN_1, KEY_VOICECOMMAND);
> > + snd_jack_set_key(jack, SND_JACK_BTN_2, KEY_VOLUMEUP);
> > + snd_jack_set_key(jack, SND_JACK_BTN_3, KEY_VOLUMEDOWN);
> > +
> > + jack->private_data = component;
> > + jack->private_free = sc7180_jack_free;
> > +
> > + rval = snd_soc_component_set_jack(component,
> > + &pdata->jack, NULL);
> > + if (rval != 0 && rval != -EOPNOTSUPP) {
> > + dev_warn(card->dev, "Failed to set jack: %d\n", rval);
> > + return rval;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static struct snd_soc_aux_dev sc7180_headset_dev = {
> > + .dlc = COMP_EMPTY(),
> > + .init = sc7180_headset_init,
> > +};
> > +
>
> [...]
>
> > +static struct snd_soc_card sc7180_card = {
> > + .owner = THIS_MODULE,
> > + .aux_dev = &sc7180_headset_dev,
> > + .num_aux_devs = 1,
> > + .dapm_widgets = sc7180_snd_widgets,
> > + .num_dapm_widgets = ARRAY_SIZE(sc7180_snd_widgets),
> > +};
> > +
> > +static int sc7180_parse_aux_of(struct device *dev)
> > +{
> > + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> > + dev->of_node, "aux-dev", 0);
> > +
> > + if (!sc7180_headset_dev.dlc.of_node)
> > + return -EINVAL;
> > + return 0;
> > +}
> > +
>
> Thanks for noting that this conflicts with my patch set that adds the
> "aux-devs" property for the device tree [1], I didn't see this before.
>
> The use of aux-dev in this patch looks a bit weird to me...
>
> As I understand, the "auxiliary devices" of a sound card are intended to
> be used for components that should be probed even though they don't
> appear within one of the DAI links. Examples for that are especially
> analog amplifiers and other components that do not have digital audio
> input/output.
>
> On the other hand, in this patch it seems to be just a way to mark the
> DAI component that will provide the headphone jack detection. In your
> example, the component that provides the headphone jack then appears
> both as DAI component and as auxiliary device:
>
> aux-dev = <&alc5682>;
>
> dai-link@0 {
> link-name = "MultiMedia0";
> reg = <0>;
> cpu {
> sound-dai = <&lpass_cpu 0>;
> };
> codec {
> sound-dai = <&alc5682 0>;
> };
> };
>
> Adding &alc5682 to snd_soc_card->aux_dev is kind of pointless in this
> case because it will already be probed as part of the DAI link.
>
> The only thing you gain is that you have the init() callback which gives
> you the component that provides the headphone jack. But if someone wants
> to add an actual auxiliary device later (e.g. an analog amplifier),
> they would run into trouble...
>
> I wonder if it would be better to just have some sort of phandle, e.g.
>
> audio-jack = <&alc5682>;
>
> but instead of creating an auxiliary device for this you would e.g.
> iterate over the list of components to find the one the phandle refers to.
Hi Stephan,
I can try this approach. Thanks for the suggestion. I think this
would also be better than the previous approach that the machine
driver set init for jack on certain DAI.
>
> Or maybe someone else can comment if using an auxiliary device for this
> does really make sense?
>
> Thanks,
> Stephan
>
> [1]: https://lore.kernel.org/alsa-devel/20200826095141.94017-1-stephan@gerhold.net/
Powered by blists - more mailing lists