Re: [PATCH] remoteproc: mediatek: Don't attempt to remap l1tcm memory if missing
On Thu, Jun 27, 2024 at 05:20:55PM -0400, Nícolas F. R. A. Prado wrote:
> The current code doesn't check whether platform_get_resource_byname()
> succeeded to get the l1tcm memory, which is optional, before attempting
> to map it. This results in the following error message when it is
> missing:
>
> mtk-scp 1050.scp: error -EINVAL: invalid resource (null)
>
> Add a check so that the remapping is only attempted if the memory region
> exists. This also allows to simplify the logic handling failure to
> remap, since a failure then is always a failure.
>
> Fixes: ca23ecfdbd44 ("remoteproc/mediatek: support L1TCM")
> Signed-off-by: Nícolas F. R. A. Prado
Reviewed-by: Tzung-Bi Shih
Re: [PATCH 1/2] remoteproc: mediatek: Make sure IPI buffer fits in L2TCM
On Thu, Mar 21, 2024 at 09:46:13AM +0100, AngeloGioacchino Del Regno wrote:
> The IPI buffer location is read from the firmware that we load to the
> System Companion Processor, and it's not granted that both the SRAM
> (L2TCM) size that is defined in the devicetree node is large enough
> for that, and while this is especially true for multi-core SCP, it's
> still useful to check on single-core variants as well.
>
> Failing to perform this check may make this driver perform R/W
> oeprations out of the L2TCM boundary, resulting (at best) in a
> kernel panic.
>
> To fix that, check that the IPI buffer fits, otherwise return a
> failure and refuse to boot the relevant SCP core (or the SCP at
> all, if this is single core).
>
> Fixes: 3efa0ea743b7 ("remoteproc/mediatek: read IPI buffer offset from FW")
> Signed-off-by: AngeloGioacchino Del Regno
>
Reviewed-by: Tzung-Bi Shih
Re: [PATCH RESEND v5 8/8] arm64: dts: mt6359: add PMIC MT6359 related nodes
On Sun, Jan 31, 2021 at 7:06 PM Matthias Brugger wrote:
> On 29/01/2021 10:49, Hsin-Hsiung Wang wrote:
> > + mt6359codec: mt6359codec {
> > + };
>
> I understand that the dmic-mode and mic-type-X depends on the actual board on
> which it is used. In that case I think we should add mt6359codec node in the
> dts
> instead of dtsi file. I'd advise to set these properties as well as otherwise
> we
> get a (slightly misleading) warning in the driver.
I feel it is better to include the node in dtsi to represent the whole
MT6359 PMIC.
We could either:
- Set default values of these properties in the dtsi to avoid the
warning message.
- Or
https://patchwork.kernel.org/project/alsa-devel/patch/20210202033557.1621029-1-tzun...@google.com/
Re: [PATCH] ASoC: mediatek: add MTK_PMIC_WRAP dependency
On Wed, Dec 30, 2020 at 11:43 PM Arnd Bergmann wrote: > > From: Arnd Bergmann > > Randconfig builds often show harmless warnings like > > WARNING: unmet direct dependencies detected for SND_SOC_MT6359 > Depends on [n]: SOUND [=y] && !UML && SND [=y] && SND_SOC [=y] && > MTK_PMIC_WRAP [=n] > Selected by [y]: > - SND_SOC_MT8192_MT6359_RT1015_RT5682 [=y] && SOUND [=y] && !UML && SND > [=y] && SND_SOC [=y] && I2C [=y] && SND_SOC_MT8192 [=y] > > Add a dependency to avoid that. > > Signed-off-by: Arnd Bergmann Acked-by: Tzung-Bi Shih
Re: [PATCH] ASoC: cros_ec_codec: fix uninitialized memory read
On Fri, Dec 4, 2020 at 4:28 PM Arnd Bergmann wrote: > > On Fri, Dec 4, 2020 at 3:56 AM Tzung-Bi Shih wrote: > > > > On Fri, Dec 4, 2020 at 6:55 AM Arnd Bergmann wrote: > > > > In the case in i2s_rx_event(), only the "cmd" member is used. But it > > is fine to please the compiler. > > I wouldn't do it just to please the compiler. I sent this patch since > the code clearly copies the uninitialized data here. If only > one byte is meant to be copied, then we should change the > function call to not pass the entire structure. I'll send a new > patch for that. My sentence may confuse you. But I mean: the uninitialized data doesn't introduce any bugs because it only uses the first byte in the case. Instead of your v2 (https://patchwork.kernel.org/project/alsa-devel/patch/20201204083624.2711356-1-a...@kernel.org/), I prefer this version v1.
Re: [PATCH] [v2] ASoC: cros_ec_codec: fix uninitialized memory read
On Fri, Dec 4, 2020 at 4:36 PM Arnd Bergmann wrote: > diff --git a/sound/soc/codecs/cros_ec_codec.c > b/sound/soc/codecs/cros_ec_codec.c > index 58894bf47514..6ec673573c70 100644 > --- a/sound/soc/codecs/cros_ec_codec.c > +++ b/sound/soc/codecs/cros_ec_codec.c > @@ -348,7 +348,7 @@ static int i2s_rx_event(struct snd_soc_dapm_widget *w, > } > > return send_ec_host_command(priv->ec_device, EC_CMD_EC_CODEC_I2S_RX, > - (uint8_t *)&p, sizeof(p), NULL, 0); > + &p.cmd, sizeof(p.cmd), NULL, 0); > } I would prefer your v1. Reasons: 1. The change is not just kernel related. There is a EC (embedded controller) firmware to collaborate with the code. The firmware doesn't know the kernel only copies the first byte of the packet (at least for now). See https://chromium.googlesource.com/chromiumos/platform/ec/+/refs/heads/master/common/audio_codec_i2s_rx.c#120. 2. We don't copy partial packets in a EC host command. IMHO, it is also not a big deal if copying a few unused bytes in the packet.
Re: [PATCH] ASoC: cros_ec_codec: fix uninitialized memory read
On Fri, Dec 4, 2020 at 6:55 AM Arnd Bergmann wrote:
>
> From: Arnd Bergmann
>
> gcc points out a memory area that is copied to a device
> but not initialized:
>
> sound/soc/codecs/cros_ec_codec.c: In function 'i2s_rx_event':
> arch/x86/include/asm/string_32.h:83:20: error: '*((void *)&p+4)' may be used
> uninitialized in this function [-Werror=maybe-uninitialized]
>83 | *((int *)to + 1) = *((int *)from + 1);
>
> Initialize all the unused fields to zero.
>
> Fixes: 727f1c71c780 ("ASoC: cros_ec_codec: refactor I2S RX")
> Signed-off-by: Arnd Bergmann
Acked-by: Tzung-Bi Shih
In the case in i2s_rx_event(), only the "cmd" member is used. But it
is fine to please the compiler.
struct __ec_align4 ec_param_ec_codec_i2s_rx {
uint8_t cmd; /* enum ec_codec_i2s_rx_subcmd */
uint8_t reserved[3];
union {
...
};
};
I am a bit curious about, in other use cases of
ec_param_ec_codec_i2s_rx, why the compiler doesn't complain about
uninitialization of the "reserved" member?
Re: [PATCH v4 2/2] ASoC: qcom: sc7180: Modify machine driver for 2mic
On Tue, Nov 3, 2020 at 10:54 AM Ajye Huang wrote:
>
> In addition, having mixer control to switch between DMICs by
> using "dmic-gpios" property.
>
> Refer to this one as an example,
> commit b7a742cff3f6 ("ASoC: AMD: Use mixer control to switch between DMICs")
>
> Signed-off-by: Ajye Huang
LGTM.
Reviewed-by: Tzung-Bi Shih
Re: [PATCH v3 2/2] ASoC: qcom: sc7180: Modify machine driver for 2mic
On Mon, Nov 2, 2020 at 2:24 PM Ajye Huang wrote:
>
> In addition, having mixer control to switch between DMICs by
> using "dmic-gpios" property.
>
> Refer to this one as an example,
> commit b7a742cff3f6 ("ASoC: AMD: Use mixer control to switch between DMICs")
>
> Signed-off-by: Ajye Huang
I am not sure if it would be better if you use another email (e.g.
@gmail) for signoff.
> +static int dmic_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm =
> snd_soc_dapm_kcontrol_dapm(kcontrol);
> + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(dapm->card);
> +
> + if (data)
You don't need to check for NULL. If snd_soc_card_get_drvdata()
returns NULL, it shouldn't run into here. See other
snd_soc_card_get_drvdata() calls in the file.
> +static int dmic_set(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_dapm_context *dapm =
> snd_soc_dapm_kcontrol_dapm(kcontrol);
> + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(dapm->card);
> +
> + if (data) {
Ditto.
> + if (IS_ERR(data->dmic_sel)) {
> + dev_err(&pdev->dev, "DMIC gpio failed err=%d\n",
> + PTR_ERR(data->dmic_sel));
> + return PTR_ERR(data->dmic_sel);
Remove 1 level indent.
Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Modify machine driver for 2mic
On Fri, Oct 30, 2020 at 6:55 PM Ajye Huang wrote: > But dmic_get() will need dmic_switch, should i keep dmic_switch? I see. I overlooked it. You can keep the dmic_switch for this purpose or just call gpiod_get_value_cansleep().
Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Modify machine driver for 2mic
On Fri, Oct 30, 2020 at 3:57 PM Ajye Huang wrote:
> +static struct gpio_desc *dmic_sel;
> +static int dmic_switch;
If you really need them, you should put them in struct sc7180_snd_data.
> +static int dmic_set(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + if (dmic_sel) {
if (IS_ERR(dmic_sel))
But I think you don't need to check dmic_sel. Suppose your _probe()
already returned error, the code here shouldn't be called.
> + dmic_switch = ucontrol->value.integer.value[0];
Looks like it can be a local variable. You don't need to save dmic_switch.
Re: [PATCH v11 2/3] ASoC: qcom: dt-bindings: Add sc7180 machine bindings
Hi, sorry for jumping into your discussion but I am trying to
summarize them to make sure we are on the same page. Pardon me to
manually copy-and-paste partial sentences to quote.
ACK:
- Don't expose DAI connections in compatible strings.
- Use "model" DT property to make the card more UCM2-friendly.
- Expose new DT properties to distinguish different DMIC models.
NACK:
- All the board variations using exactly the same compatible string.
=> This is less realistic. Although the CODECS information can be
retrieved from DT, it is inevitable to have some custom code for each
CODEC.
Per Mark's words:
> a different CODEC is something that often justifies a separate compatible
I think we should use different compatible strings for new CODECS
combinations. And we should try to reuse the machine driver if they
share the most code. In the worst case, introduce a new machine
driver for the new CODECS combinations.
- Srinivas's suggestion to set driver_name.
e.g. card->driver_name = "SM8250";
=> This sounds like a new DT property should be parsed in
sound/soc/qcom/common.c. For example: "qcom,family"? But as we do
less care about UCM2 for now, I would prefer to just leave it as is.
I would expect the following variants in DTS (just for example):
sound {
compatible = "qcom,sc7180-trogdor";
model = "sc7180-rt5682-max98357a-1mic";
}
sound {
compatible = "qcom,sc7180-trogdor";
model = "sc7180-rt5682-max98357a-2mic";
dmic-gpio = ...
}
sound {
compatible = "qcom,sc7180-pompom";
model = "sc7180-adau7002-max98357a";
}
Please correct me if there is any misunderstanding.
Re: [PATCH] remoteproc/mediatek: Add support for mt8192 SCP
On Thu, Sep 17, 2020 at 2:31 PM Pi-Hsun Shih wrote: > > Add support for mt8192 SCP. > > Signed-off-by: Pi-Hsun Shih Has done 1 round review on: https://crrev.com/c/2297082 > +#define MT8192_CORE0_R_GPR10x30044 > +#define MT8192_CORE0_R_GPR20x30048 Remove them because these 2 macros are unused now. With that, Reviewed-by: Tzung-Bi Shih
Re: [PATCH v8 3/3] ASoC: qcom: sc7180: Add machine driver for sound card registration
On Thu, Sep 10, 2020 at 1:24 PM Cheng-Yi Chiang wrote:
> +struct sc7180_snd_data {
> + u32 pri_mi2s_clk_count;
> + struct snd_soc_jack hs_jack;
> + struct device_node *hs_jack_of_node;
> + struct snd_soc_jack hdmi_jack;
> + struct device_node *hdmi_jack_of_node;
> +};
hs_jack_of_node and hdmi_jack_of_node are not using. Remove them.
> +static int sc7180_init(struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + return sc7180_headset_init(rtd);
> + case MI2S_SECONDARY:
> + return 0;
> + case HDMI:
I guess this enumeration has not merged yet? It doesn't sound like a
specific-enough naming.
> +static int sc7180_snd_platform_probe(struct platform_device *pdev)
> +{
> + struct snd_soc_card *card = &sc7180_card;
> + struct sc7180_snd_data *data;
> + struct device *dev = &pdev->dev;
> + int ret;
> +
> + /* Allocate the private data */
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + card->dev = dev;
> +
> + ret = qcom_snd_parse_of(card);
> + if (ret) {
> + dev_err(dev, "Error parsing OF data\n");
> + return ret;
> + }
> +
> + snd_soc_card_set_drvdata(card, data);
To be neat, move this line immediately before or after "card->dev = dev;".
Re: [PATCH v8 1/3] ASoC: hdmi-codec: Use set_jack ops to set jack
On Thu, Sep 10, 2020 at 1:24 PM Cheng-Yi Chiang wrote:
> diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
> index 8c6f540533ba..d1de5bcd5daa 100644
> --- a/sound/soc/codecs/hdmi-codec.c
> +++ b/sound/soc/codecs/hdmi-codec.c
> @@ -698,13 +698,9 @@ static void plugged_cb(struct device *dev, bool plugged)
> hdmi_codec_jack_report(hcp, 0);
> }
>
> -/**
> - * hdmi_codec_set_jack_detect - register HDMI plugged callback
> - * @component: the hdmi-codec instance
> - * @jack: ASoC jack to report (dis)connection events on
> - */
> -int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
> - struct snd_soc_jack *jack)
> +static int hdmi_codec_set_jack_detect(struct snd_soc_component *component,
> + struct snd_soc_jack *jack,
> + void *data)
To be neat, name it "hdmi_codec_set_jack".
> static int hdmi_dai_spdif_probe(struct snd_soc_dai *dai)
> {
> @@ -806,6 +801,7 @@ static const struct snd_soc_component_driver hdmi_driver
> = {
> .use_pmdown_time= 1,
> .endianness = 1,
> .non_legacy_dai_naming = 1,
> + .set_jack = hdmi_codec_set_jack_detect,
"hdmi_codec_set_jack" looks better to me.
If you would send a newer version, consider changing the name.
With that:
Reviewed-by: Tzung-Bi Shih
Re: [PATCH v5 2/2] dt-bindings: mediatek: mt6359: add codec document
On Thu, Aug 20, 2020 at 3:40 AM Mark Brown wrote:
>
> On Wed, Aug 19, 2020 at 11:42:27PM +0800, Tzung-Bi Shih wrote:
>
> > But I found struct mfd_cell also contains member .of_compatible. What
> > is the difference if we use compatible string (as is) for this device
> > instead of falling back to use device name to match?
>
> That's for binding the MFD subdevice to an OF node, you don't need to do
> that for a device like this - you can just use the of_node of the parent
> to get at the properties.
There is an issue we overlooked. In sound/soc/codecs/mt6359.c,
mt6359_parse_dt() cannot read the DT properties
(https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound/soc/codecs/mt6359.c?h=for-next#n2640).
The original DTS is as following:
pmic {
compatible = "mediatek,mt6359";
mt6359codec: mt6359codec {
compatible = "mediatek,mt6359-sound"; (1)
mediatek,dmic-mode = <1>;
mediatek,mic-type-0 = <2>;
}
}
After removing the line at (1), mt6359_parse_dt() cannot read the DT properties.
The PMIC drivers/mfd/mt6397-core.c:
- "mediatek,mt6359"
- has the struct mfd_cell of mt6359-sound
- adds all mfd_cells via devm_mfd_add_devices().
The audio codec sound/soc/codecs/mt6359.c:
- "mediatek,mt6359-sound"
Here are a few options we can come out with.
1. adds back the compatible string in the DTS
2. gets of_node of parent in mt6359.c, and iterates all children nodes
to get the desired properties
3. parses all children nodes in the PMIC driver, and put them into
either platform_data or device properties
(https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/include/linux/mfd/core.h?h=for-next#n77)
- The PMIC is common for several sub-devices. It makes less sense to
handle subdevice specific logic in the common code.
4. others
Could you share with us what would you suggest for fixing the issue?
Re: [PATCH v5 2/2] dt-bindings: mediatek: mt6359: add codec document
On Wed, Aug 19, 2020 at 6:38 PM Mark Brown wrote: > > On Mon, Aug 17, 2020 at 04:11:03PM +0800, Tzung-Bi Shih wrote: > > I misunderstood. It still needs the compatible string to match the > > corresponding driver. > > No, it doesn't. The MFD should be registering the platform device. I guess I see. It lists the mfd_cell when calling devm_mfd_add_devices() in drivers/mfd/mt6397-core.c. It falls back to use driver name and device name to match. As long as the name provided in mfd_cell matches the platform driver name, it works. But I found struct mfd_cell also contains member .of_compatible. What is the difference if we use compatible string (as is) for this device instead of falling back to use device name to match?
Re: [PATCH v5 2/2] dt-bindings: mediatek: mt6359: add codec document
On Mon, Aug 17, 2020 at 3:29 PM Tzung-Bi Shih wrote:
>
> On Mon, Aug 17, 2020 at 2:08 PM Jiaxin Yu wrote:
> > +properties:
> > + compatible:
> > +const: mediatek,mt6359-sound
>
> The compatible string has been removed since v3.
>
> > +required:
> > + - compatible
>
> The same here.
>
> > +examples:
> > + - |
> > +mt6359codec: mt6359codec {
> > + compatible = "mediatek,mt6359-sound";
>
> And the same here.
I misunderstood. It still needs the compatible string to match the
corresponding driver.
Re: [PATCH v5 1/2] ASoC: mediatek: mt6359: add codec driver
On Mon, Aug 17, 2020 at 2:08 PM Jiaxin Yu wrote: > > Add the mt6359 codec driver. > > Signed-off-by: Jiaxin Yu Reviewed-by: Tzung-Bi Shih LGTM, thanks.
Re: [PATCH v5 2/2] dt-bindings: mediatek: mt6359: add codec document
On Mon, Aug 17, 2020 at 2:08 PM Jiaxin Yu wrote:
> +properties:
> + compatible:
> +const: mediatek,mt6359-sound
The compatible string has been removed since v3.
> +required:
> + - compatible
The same here.
> +examples:
> + - |
> +mt6359codec: mt6359codec {
> + compatible = "mediatek,mt6359-sound";
And the same here.
Re: [PATCH v4 1/2] ASoC: mediatek: mt6359: add codec driver
On Sun, Aug 16, 2020 at 1:20 AM Jiaxin Yu wrote:
>
> +static int mt6359_platform_driver_probe(struct platform_device *pdev)
[snip]
> +
> + return devm_snd_soc_register_component(&pdev->dev,
> + &mt6359_soc_component_driver,
> + mt6359_dai_driver,
> + ARRAY_SIZE(mt6359_dai_driver));
> +}
> +
> +static int mt6359_platform_driver_remove(struct platform_device *pdev)
> +{
> + struct mt6359_priv *priv = dev_get_drvdata(&pdev->dev);
> + int ret;
> +
> + dev_dbg(&pdev->dev, "%s(), dev name %s\n",
> + __func__, dev_name(&pdev->dev));
> +
> + ret = regulator_disable(priv->avdd_reg);
> + if (ret) {
> + dev_err(&pdev->dev, "%s(), failed to disable regulator!\n",
> + __func__);
> + return ret;
> + }
> +
> + snd_soc_unregister_component(&pdev->dev);
You don't need to unregister the component which is already delegated to devm.
Re: [PATCH v3 0/2] Add mediatek codec mt6359 driver
On Fri, Aug 14, 2020 at 6:47 PM Jiaxin Yu wrote: > Jiaxin Yu (2): > WIP: ASoC: mediatek: mt6359: add codec driver > WIP: dt-bindings: mediatek: mt6359: add codec document Please remove the "WIP: " prefixes and resend again.
Re: [PATCH v2 1/2] ASoC: mediatek: mt6359: add codec driver
On Mon, Aug 10, 2020 at 11:11 AM Jiaxin Yu wrote: > > Add the mt6359 codec driver. > > Signed-off-by: Jiaxin Yu Reviewed-by: Tzung-Bi Shih This patch also reviewed few rounds on https://crrev.com/c/2299951
Re: [PATCH v3 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
On Fri, Jul 31, 2020 at 4:41 PM Cheng-Yi Chiang wrote: > > From: Ajit Pandey > > 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 > Signed-off-by: Cheng-Yi Chiang Reviewed-by: Tzung-Bi Shih LGTM.
Re: [PATCH v2 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
On Tue, Jul 21, 2020 at 6:44 PM Cheng-Yi Chiang wrote:
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index ..3beb2b129d01
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,380 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +//Copyright (c) 2020, The Linux Foundation. All rights reserved.
> +//
> +//sc7180.c -- ALSA SoC Machine driver for SC7180
Insert an extra space between // and text to make it look better.
> +static int sc7180_headset_init(struct snd_soc_component *component);
> +
> +static struct snd_soc_aux_dev sc7180_headset_dev = {
> + .dlc = COMP_EMPTY(),
> + .init = sc7180_headset_init,
> +};
Move definition of sc7180_headset_dev after sc7180_headset_init( ) so
that you don't need forward declaration of sc7180_headset_init( ).
> +static unsigned int primary_dai_fmt = SND_SOC_DAIFMT_CBS_CFS |
> + SND_SOC_DAIFMT_NB_NF |
> + SND_SOC_DAIFMT_I2S;
> +
> +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> +{
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_card *card = rtd->card;
> + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + if (++data->pri_mi2s_clk_count == 1) {
> + snd_soc_dai_set_sysclk(cpu_dai,
> + LPASS_MCLK0,
> + DEFAULT_MCLK_RATE,
> + SNDRV_PCM_STREAM_PLAYBACK);
> + }
> + snd_soc_dai_set_fmt(codec_dai, primary_dai_fmt);
My comment on the previous thread may mislead. My original intent:
move the DAIFMT setting to DAI link->dai_fmt in sc7180_parse_of( ).
If you need to keep it as is: inline the SND_SOC_DAIFMT_* into
snd_soc_dai_set_fmt( ) (i.e. eliminate primary_dai_fmt).
> +static int sc7180_parse_of(struct snd_soc_card *card)
> +{
> + struct device_node *np;
> + struct device_node *codec = NULL;
> + struct device_node *platform = NULL;
The function doesn't use platform.
> + struct device_node *cpu = NULL;
> + struct device *dev = card->dev;
> + struct snd_soc_dai_link *link;
> + struct of_phandle_args args;
> + struct snd_soc_dai_link_component *dlc;
> + int ret, num_links;
> +
> + ret = snd_soc_of_parse_card_name(card, "model");
> + if (ret) {
> + dev_err(dev, "Error parsing card name: %d\n", ret);
> + return ret;
> + }
> +
> + /* DAPM routes */
> + if (of_property_read_bool(dev->of_node, "audio-routing")) {
> + ret = snd_soc_of_parse_audio_routing(card,
> +"audio-routing");
> + if (ret)
> + return ret;
> + }
> +
> + /* headset aux dev. */
> + sc7180_headset_dev.dlc.of_node = of_parse_phandle(
> + dev->of_node, "aux-dev", 0);
> + if (!sc7180_headset_dev.dlc.of_node) {
> + dev_err(dev,
> + "Property 'aux-dev' missing/invalid\n");
> + return -EINVAL;
> + }
> +
> + /* Populate links */
> + num_links = of_get_child_count(dev->of_node);
Eliminate num_links but use card->num_links directly.
> +
> + /* Allocate the DAI link array */
> + card->dai_link = devm_kcalloc(dev, num_links, sizeof(*link),
> + GFP_KERNEL);
> + if (!card->dai_link)
> + return -ENOMEM;
> +
> + card->num_links = num_links;
Ditto, eliminate it.
> + link = card->dai_link;
> +
Eliminate the blank line to make "link = card->dai_link" and the
following for-loop "a whole thing".
> + for_each_child_of_node(dev->of_node, np) {
> + dlc = devm_kzalloc(dev, 2 * sizeof(*dlc), GFP_KERNEL);
> + if (!dlc)
> + return -ENOMEM;
> +
> + link->cpus = &dlc[0];
> + link->platforms = &dlc[1];
> +
> + link->num_cpus = 1;
> + link->num_platforms = 1;
> +
> + ret = of_property_read_string(np, "link-name", &link->name);
> + if (ret) {
> + dev_err(card->dev,
> + "error getting codec dai_link name\n");
> + goto err;
> + }
> +
> + link->playback_only = of_property_read_bool(np,
> + "playback_only");
> + link->capture_only = of_property_read_bool(np,
> +
Re: Speaker pops with max98357a on rk3399-gru-kevin since v5.7
On Tue, Jul 21, 2020 at 6:40 AM Alper Nebi Yasak wrote: > > On 17/07/2020 05:27, Tzung-Bi Shih wrote: > > I am not convinced the pop comes from 128f825aeab7. > > Maybe some pre-existing defect in rk3399_gru_sound got exposed by > 128f825aeab7 or the machine driver needs some changes to complement > that commit? Hi, I got a rk3399-gru-kevin and can reproduce the issue. Could you take a try on the proposed patch here https://patchwork.kernel.org/patch/11675533/ to see if it fixes?
Re: [PATCH] platform_data: cros_ec_commands.h: drop a duplicated word
On Sun, Jul 19, 2020 at 8:30 AM Randy Dunlap wrote: > > Drop the repeated word "using" in a comment. > > Signed-off-by: Randy Dunlap > Cc: Tzung-Bi Shih > Cc: Mark Brown > --- I guess you didn't include the maintainers: Benson Leung (maintainer:CHROMEOS EC SUBDRIVERS) Enric Balletbo i Serra (maintainer:CHROMEOS EC SUBDRIVERS) Guenter Roeck (reviewer:CHROMEOS EC SUBDRIVERS)
Re: [PATCH 2/2] ASoC: qcom: sc7180: Add machine driver for sound card registration
On Fri, Jul 17, 2020 at 8:02 PM Cheng-Yi Chiang wrote:
> diff --git a/sound/soc/qcom/sc7180.c b/sound/soc/qcom/sc7180.c
> new file mode 100644
> index ..cbe6b487d432
> --- /dev/null
> +++ b/sound/soc/qcom/sc7180.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + *
> + * sc7180.c -- ALSA SoC Machine driver for SC7180
> + */
Use "//" for all lines (see https://lkml.org/lkml/2020/5/14/332).
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include "../codecs/rt5682.h"
> +#include "common.h"
> +#include "lpass.h"
Insert a blank line in between <...> and "..." and sort the list
alphabetically to make it less likely to conflict.
> +static int sc7180_snd_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params)
> +{
Dummy function? Or is it still work in progress?
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> + int ret = 0;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + break;
> + case MI2S_SECONDARY:
> + break;
> + default:
> + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
> +static int sc7180_dai_init(struct snd_soc_pcm_runtime *rtd)
> +{
> + struct snd_soc_component *component;
> + struct snd_soc_card *card = rtd->card;
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + struct sc7180_snd_data *pdata = snd_soc_card_get_drvdata(card);
> + struct snd_jack *jack;
> + int rval;
> +
> + if (!pdata->jack_setup) {
> + 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 Headphone 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);
> + pdata->jack_setup = true;
This block is something I don't expect to be in "dai_init" (i.e. there
is only 1 headset jack, why do we need to run the code for n times).
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + jack = pdata->jack.jack;
> + component = codec_dai->component;
> +
> + 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;
> + }
> + break;
> + case MI2S_SECONDARY:
> + break;
> + default:
> + pr_err("%s: invalid dai id 0x%x\n", __func__, cpu_dai->id);
-EINVAL.
> +static int sc7180_snd_startup(struct snd_pcm_substream *substream)
> +{
> + unsigned int codec_dai_fmt = SND_SOC_DAIFMT_CBS_CFS;
> + struct snd_soc_pcm_runtime *rtd = substream->private_data;
> + struct snd_soc_card *card = rtd->card;
> + struct sc7180_snd_data *data = snd_soc_card_get_drvdata(card);
> + struct snd_soc_dai *cpu_dai = asoc_rtd_to_cpu(rtd, 0);
> + struct snd_soc_dai *codec_dai = asoc_rtd_to_codec(rtd, 0);
> + int ret;
> +
> + switch (cpu_dai->id) {
> + case MI2S_PRIMARY:
> + codec_dai_fmt |= SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_I2S;
If the format is fixed, could it put somewhere statically?
> + if (++data->pri_mi2s_clk_count == 1) {
Don't it need to be atomic?
> + snd_soc_dai_set_sysclk(cpu_dai,
> + LPASS_MCLK0,
> + DEFAULT_MCLK_RATE,
> + SNDRV_PCM_STREAM_PLAYBACK);
> + }
> + snd_soc_dai_set_fmt(codec_dai, codec_dai_fmt);
> +
> + /* Configure PLL1 for codec */
> + ret =
Re: Speaker pops with max98357a on rk3399-gru-kevin since v5.7
On Thu, Jul 16, 2020 at 7:49 PM Alper Nebi Yasak
wrote:
> I have been getting "pop" sounds from the speaker on my rk3399-gru-kevin
> for a while, and bisected it to 128f825aeab7 ("ASoC: max98357a: move
> control of SD_MODE to DAPM"), but looks like the pops were somewhat
> expected:
I am not convinced the pop comes from 128f825aeab7.
> As of v5.8-rc5 I'm still getting the speaker pops. More info below, but
> not all pops coincide with "set sdmode" messages, and vice versa.
> Reverting that commit stops the pops, but then the "Speakers Switch" can
> no longer mute the speakers.
(I don't have a rk3399-gru-kevin so I got another test machine with MAX98357A.)
(I was testing with and without an audio server.)
Observations:
- I can hear the pop either with or without 128f825aeab7 (with and
without sdmode-delay).
- The pop noise is not always. Higher probability after stopping
playback than before starting.
- As you also mentioned, the pop noise is not directly related to
SD_MODE transition.
Re: [PATCH v2] ASoC: cros_ec_codec: Log results when EC commands fail
On Fri, Jul 3, 2020 at 3:19 PM Yu-Hsuan Hsu wrote:
> Log results of failed EC commands to identify a problem more easily.
>
> Replace cros_ec_cmd_xfer_status with cros_ec_cmd_xfer because the result
> has already been checked in this function. The wrapper is not needed.
Alternatively, you can still use cros_ec_cmd_xfer_status( ). I guess
it is okay to have 2 logs for an error.
> diff --git a/sound/soc/codecs/cros_ec_codec.c
> b/sound/soc/codecs/cros_ec_codec.c
> index 8d45c628e988e..a4ab62f59efa6 100644
> --- a/sound/soc/codecs/cros_ec_codec.c
> +++ b/sound/soc/codecs/cros_ec_codec.c
> @@ -90,10 +90,17 @@ static int send_ec_host_command(struct cros_ec_device
> *ec_dev, uint32_t cmd,
> if (outsize)
> memcpy(msg->data, out, outsize);
>
> - ret = cros_ec_cmd_xfer_status(ec_dev, msg);
> + ret = cros_ec_cmd_xfer(ec_dev, msg);
> if (ret < 0)
I am thinking of if it is a better solution to print msg->result here.
> goto error;
>
> + if (msg->result != EC_RES_SUCCESS) {
> + dev_err(ec_dev->dev, "Command %d failed: %d\n", cmd,
> + msg->result);
> + ret = -EPROTO;
> + goto error;
> + }
So that you don't need this block.
Re: [PATCH v2 0/2] ASoC: mediatek: mt6358: support DMIC one-wire mode
On Fri, Jun 5, 2020 at 6:37 PM Jiaxin Yu wrote: > Jiaxin Yu (2): > ASoC: mediatek: mt6358: support DMIC one-wire mode Has done previous round review on https://crrev.com/c/2230089 > ASoC: dt-bindings: mediatek: mt6358: add dmic-mode property Has done previous round review on https://crrev.com/c/2230086 > Documentation/devicetree/bindings/sound/mt6358.txt | 6 ++ > sound/soc/codecs/mt6358.c | 23 > +- > 2 files changed, 28 insertions(+), 1 deletion(-) With that: Reviewed-by: Tzung-Bi Shih
Re: [PATCH] ASoC: mediatek: mt6358: support DMIC one-wire mode
On Fri, May 29, 2020 at 9:05 PM Mark Brown wrote: > > On Fri, May 29, 2020 at 07:22:43PM +0800, Tzung-Bi Shih wrote: > > On Fri, May 29, 2020 at 7:09 PM Mark Brown wrote: > > > > What is DMIC one wire mode? This doesn't sound like something I'd > > > expect to vary at runtime. > > > It means: 1 PDM data wire carries 2 channel data (rising edge for left > > and falling edge for right). > > I thought that was normal for DMICs - is this selecting between left and > right or something? Not sure what is the common name but use the same context here. MT6358 accepts up to 2 PDM wires for 2 DMICs. If one wire mode is on, MT6358 only accepts 1 PDM wire. If one wire mode is off, MT6358 merges L/R from 2 PDM wires into 1 I2S-like to SoC.
Re: [PATCH] ASoC: mediatek: mt6358: support DMIC one-wire mode
On Fri, May 29, 2020 at 7:09 PM Mark Brown wrote: > > On Fri, May 29, 2020 at 07:04:53PM +0800, Jiaxin Yu wrote: > > Supports DMIC one-wire mode. Adds a mixer control to enable and disable. > > What is DMIC one wire mode? This doesn't sound like something I'd > expect to vary at runtime. It means: 1 PDM data wire carries 2 channel data (rising edge for left and falling edge for right). The setting shouldn't and won't change at runtime. Would you suggest putting it into DTS binding?
Re: [V3 PATCH 2/2] ASoC: max98390: Added Amplifier Driver
On Wed, May 13, 2020 at 3:47 PM Steve Lee wrote:
> +++ b/sound/soc/codecs/max98390.c
> @@ -0,0 +1,1030 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * max98390.c -- MAX98390 ALSA Soc Audio driver
> + *
> + * Copyright (C) 2020 Maxim Integrated Products
> + *
> + */
My previous comments on Gerrit may confuse you. Please use C++ style
comments (i.e. // for all lines) here as the maintainer asked.
Mark, may I ask why we need to do so? Is it also applicable to header files?
> +static int max98390_dsm_calib_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + /* Do nothing. */
> +
> + return 0;
> +}
This is an intentional NOP.
> +static int max98390_dsm_calib_put(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + struct snd_soc_component *component =
> + snd_soc_kcontrol_component(kcontrol);
> +
> + max98390_dsm_calibrate(component);
> +
> + return 0;
> +}
Entry point from userspace to start to calibrate.
> +++ b/sound/soc/codecs/max98390.h
> @@ -0,0 +1,663 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020, Maxim Integrated.
> + */
I guess these block would also need to use C++ style comment.
Re: [V3 PATCH 2/2] ASoC: max98390: Added Amplifier Driver
On Wed, May 13, 2020 at 6:53 PM Mark Brown wrote: > > On Wed, May 13, 2020 at 04:45:23PM +0900, Steve Lee wrote: > > > Changes since V2: > > * Removed warn massage in max98390_dsm_calib_get func > > and add comment. > > The problem isn't the warning, the problem is that you have an empty > operation. You should either implement the function (eg, by caching the > value written) or remove it and fix whatever problems you were running > into further up the stack when it's missing. The purpose for the mixer control is: to signal max98390 to start to calibrate from userspace. Thus, max98390_dsm_calib_get() -> do nothing. max98390_dsm_calib_put() -> call max98390_dsm_calibrate().
Re: [PATCH 2/2] ASoC: max98390: Added Amplifier Driver
(The patch passed 2 round review in https://crrev.com/c/2083354)
On Sun, May 10, 2020 at 4:23 PM Steve Lee wrote:
> +static int max98390_dai_set_fmt(struct snd_soc_dai *codec_dai, unsigned int
> fmt)
> +{
> + struct snd_soc_component *component = codec_dai->component;
> + struct max98390_priv *max98390 =
> + snd_soc_component_get_drvdata(component);
> + unsigned int mode;
> + unsigned int format;
> + unsigned int invert;
> +
> + dev_dbg(component->dev, "%s: fmt 0x%08X\n", __func__, fmt);
> +
> + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
> + case SND_SOC_DAIFMT_CBS_CFS:
> + mode = MAX98390_PCM_MASTER_MODE_SLAVE;
> + break;
> + case SND_SOC_DAIFMT_CBM_CFM:
> + max98390->master = true;
> + mode = MAX98390_PCM_MASTER_MODE_MASTER;
> + break;
> + default:
> + dev_err(component->dev, "DAI clock mode unsupported\n");
> + return -EINVAL;
> + }
> +
> + regmap_update_bits(max98390->regmap,
> + MAX98390_PCM_MASTER_MODE,
> + MAX98390_PCM_MASTER_MODE_MASK,
> + mode);
> +
> + switch (fmt & SND_SOC_DAIFMT_INV_MASK) {
> + case SND_SOC_DAIFMT_NB_NF:
> + break;
> + case SND_SOC_DAIFMT_IB_NF:
> + invert = MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE;
> + break;
> + default:
> + dev_err(component->dev, "DAI invert mode unsupported\n");
> + return -EINVAL;
> + }
> +
> + regmap_update_bits(max98390->regmap,
> + MAX98390_PCM_MODE_CFG,
> + MAX98390_PCM_MODE_CFG_PCM_BCLKEDGE,
> + invert);
invert will be uninitialized in the SND_SOC_DAIFMT_NB_NF case.
> +static int max98390_dai_hw_params(struct snd_pcm_substream *substream,
> + struct snd_pcm_hw_params *params,
> + struct snd_soc_dai *dai)
Lack of 1 tab indent.
> +static int max98390_adaptive_rdc_get(struct snd_kcontrol *kcontrol,
> + struct snd_ctl_elem_value *ucontrol)
> +{
> + int rdc, rdc0;
> + struct snd_soc_component *component =
> + snd_soc_kcontrol_component(kcontrol);
> + struct max98390_priv *max98390 =
> + snd_soc_component_get_drvdata(component);
> +
> + regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE1, &rdc);
> + regmap_read(max98390->regmap, THERMAL_RDC_RD_BACK_BYTE0, &rdc0);
> + rdc0 |= rdc << 8;
> + ucontrol->value.integer.value[0] = rdc0;
ucontrol->value.integer.value[0] = rdc0 | (rdc << 8);
> +static int max98390_dsm_init(struct snd_soc_component *component)
> +{
> + int ret;
> + const char *filename;
> + struct max98390_priv *max98390 =
> + snd_soc_component_get_drvdata(component);
> + const struct firmware *fw = NULL;
> + char *dsm_param = NULL;
Don't need to initialize fw and dsm_param in the case.
> +
> + filename = "dsm_param.bin";
Either:
- initialize when declaring the variable
- remove the variable and inline into request_firmware() call
> + ret = request_firmware(&fw, filename, component->dev);
> + if (ret) {
> + dev_err(component->dev,
> + "Failed to acquire dsm params: %d\n", ret);
> + goto err;
> + }
> +
> + dev_info(component->dev,
> + "max98390: param fw size %d\n",
> + fw->size);
> + dsm_param = (char *)fw->data;
> + dsm_param += MAX98390_DSM_PAYLOAD_OFFSET;
> + regmap_bulk_write(max98390->regmap, DSM_EQ_BQ1_B0_BYTE0,
> + dsm_param,
> + fw->size - MAX98390_DSM_PAYLOAD_OFFSET);
> + release_firmware(fw);
> + regmap_write(max98390->regmap, MAX98390_R23E1_DSP_GLOBAL_EN, 0x01);
> +
> +err:
> + return ret;
> +}
> +static int max98390_probe(struct snd_soc_component *component)
> +{
> + struct max98390_priv *max98390 =
> + snd_soc_component_get_drvdata(component);
> +
> + /* Update dsm bin param */
This comment makes more sense if before max98390_dsm_init().
> + regmap_write(max98390->regmap, MAX98390_SOFTWARE_RESET, 0x01);
> + /* Sleep reset settle time */
> + msleep(20);
> + max98390_dsm_init(component);
> + /* Check Revision ID */
> + ret = regmap_read(max98390->regmap,
> + MAX98390_R24FF_REV_ID, ®);
> + if (ret < 0) {
if (ret)
Re: [PATCH] ASoC: cros_ec_codec: allocate shash_desc dynamically
On Fri, May 8, 2020 at 5:34 AM Arnd Bergmann wrote:
> Fixes: b6bc07d4360d ("ASoC: cros_ec_codec: support WoV")
> Signed-off-by: Arnd Bergmann
Reviewed-by: Tzung-Bi Shih
LGTM. Thanks for the fix.
Re: [PATCH] ASoC: mediatek: Check SND_SOC_CROS_EC_CODEC dependency
On Wed, Oct 23, 2019 at 4:38 PM maowenan wrote: > I receive below message after I post, do you know why? > ''' > Your mail to 'Alsa-devel' with the subject > > [PATCH] ASoC: mediatek: Check SND_SOC_CROS_EC_CODEC dependency > > Is being held until the list moderator can review it for approval. > > The reason it is being held: > > Post by non-member to a members-only list I don't exactly know. But I got similar messages when I first time sent mail to the alsa-devel. Have you subscribed to alsa-devel mailing list? I guess it is fine to wait maintainers to proceed your patch.
Re: [PATCH] ASoC: mediatek: Check SND_SOC_CROS_EC_CODEC dependency
On Wed, Oct 23, 2019 at 2:31 PM Mao Wenan wrote:
>
> If SND_SOC_MT8183_MT6358_TS3A227E_MAX98357A=y,
> below errors can be seen:
> sound/soc/codecs/cros_ec_codec.o: In function `send_ec_host_command':
> cros_ec_codec.c:(.text+0x534): undefined reference to
> `cros_ec_cmd_xfer_status'
> cros_ec_codec.c:(.text+0x101c): undefined reference to
> `cros_ec_get_host_event'
>
> This is because it will select SND_SOC_CROS_EC_CODEC
> after commit 2cc3cd5fdc8b ("ASoC: mediatek: mt8183: support WoV"),
> but SND_SOC_CROS_EC_CODEC depends on CROS_EC.
>
> Fixes: 2cc3cd5fdc8b ("ASoC: mediatek: mt8183: support WoV")
> Signed-off-by: Mao Wenan
> ---
> sound/soc/mediatek/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sound/soc/mediatek/Kconfig b/sound/soc/mediatek/Kconfig
> index 8b29f39..a656d20 100644
> --- a/sound/soc/mediatek/Kconfig
> +++ b/sound/soc/mediatek/Kconfig
> @@ -125,7 +125,7 @@ config SND_SOC_MT8183_MT6358_TS3A227E_MAX98357A
> select SND_SOC_MAX98357A
> select SND_SOC_BT_SCO
> select SND_SOC_TS3A227E
> - select SND_SOC_CROS_EC_CODEC
> + select SND_SOC_CROS_EC_CODEC if CROS_EC
> help
> This adds ASoC driver for Mediatek MT8183 boards
> with the MT6358 TS3A227E MAX98357A audio codec.
> --
> 2.7.4
>
Just realized your patch seems not showing in the list
(https://mailman.alsa-project.org/pipermail/alsa-devel/2019-October/thread.html).
I have no idea why.
Re: [PATCH] ASoC: mediatek: Check SND_SOC_CROS_EC_CODEC dependency
On Wed, Oct 23, 2019 at 2:31 PM Mao Wenan wrote:
>
> If SND_SOC_MT8183_MT6358_TS3A227E_MAX98357A=y,
> below errors can be seen:
> sound/soc/codecs/cros_ec_codec.o: In function `send_ec_host_command':
> cros_ec_codec.c:(.text+0x534): undefined reference to
> `cros_ec_cmd_xfer_status'
> cros_ec_codec.c:(.text+0x101c): undefined reference to
> `cros_ec_get_host_event'
>
> This is because it will select SND_SOC_CROS_EC_CODEC
> after commit 2cc3cd5fdc8b ("ASoC: mediatek: mt8183: support WoV"),
> but SND_SOC_CROS_EC_CODEC depends on CROS_EC.
>
> Fixes: 2cc3cd5fdc8b ("ASoC: mediatek: mt8183: support WoV")
> Signed-off-by: Mao Wenan
Acked-by: Tzung-Bi Shih
Thanks for the catching.
Re: [PATCH] firmware: vpd: Add an interface to read VPD value
On Mon, Oct 7, 2019 at 3:16 PM Cheng-Yi Chiang wrote:
>
> Add an interface for other driver to query VPD value.
> This will be used for ASoC machine driver to query calibration
> data stored in VPD for smart amplifier speaker resistor
> calibration.
>
> Signed-off-by: Cheng-Yi Chiang
> ---
> drivers/firmware/google/vpd.c | 16
> include/linux/firmware/google/google_vpd.h | 18 ++
> 2 files changed, 34 insertions(+)
> create mode 100644 include/linux/firmware/google/google_vpd.h
>
> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> index db0812263d46..71e9d2da63be 100644
> --- a/drivers/firmware/google/vpd.c
> +++ b/drivers/firmware/google/vpd.c
> @@ -65,6 +65,22 @@ static ssize_t vpd_attrib_read(struct file *filp, struct
> kobject *kobp,
>info->bin_attr.size);
> }
>
> +int vpd_attribute_read_value(bool ro, const char *key,
> +char **value, u32 value_len)
> +{
> + struct vpd_attrib_info *info;
> + struct vpd_section *sec = ro ? &ro_vpd : &rw_vpd;
> +
> + list_for_each_entry(info, &sec->attribs, list) {
> + if (strcmp(info->key, key) == 0) {
> + *value = kstrndup(info->value, value_len, GFP_KERNEL);
Value is not necessary a NULL-terminated string.
kmalloc(info->bin_attr.size) and memcpy(...) would make the most
sense.
The value_len parameter makes less sense. It seems the caller knows
the length of the value in advance.
Suggest to change the value_len to report the length of value. I.e.
*value_len = info->bin_attr.size;
Also please check the return value for memory allocation-like
functions (e.g. kstrndup, kmalloc) so that *value won't be NULL but
the function returned 0.
> + return 0;
> + }
> + }
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(vpd_attribute_read_value);
> +
> /*
> * vpd_section_check_key_name()
> *
> diff --git a/include/linux/firmware/google/google_vpd.h
> b/include/linux/firmware/google/google_vpd.h
> new file mode 100644
> index ..6f1160f28af8
> --- /dev/null
> +++ b/include/linux/firmware/google/google_vpd.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Google VPD interface.
> + *
> + * Copyright 2019 Google Inc.
> + */
> +
> +/* Interface for reading VPD value on Chrome platform. */
> +
> +#ifndef __GOOGLE_VPD_H
> +#define __GOOGLE_VPD_H
> +
> +#include
> +
> +int vpd_attribute_read_value(bool ro, const char *key,
> +char **value, u32 value_len);
> +
> +#endif /* __GOOGLE_VPD_H */
> --
> 2.23.0.581.g78d2f28ef7-goog
>
Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script
On Thu, Aug 1, 2019 at 6:59 PM Enric Balletbo i Serra wrote: > > Hi Tzung-Bi > > On 30/7/19 16:07, Tzung-Bi Shih wrote: > > Hi Enric, > > > > I found it is error-prone to replace the EC_CMDS after updated. > > Perhaps, we should introduce an intermediate file "cros_ec_trace.inc". > > I am not sure I get you here, a .inc? could you explain a bit more? > Manually generate .inc for all EC host commands: sed ... include/linux/mfd/cros_ec_commands.h | awk ... >drivers/platform/chrome/cros_ec_trace.inc In cros_ec_trace.c: #include "cros_ec_trace.inc" cros_ec_trace.inc: #ifndef EC_CMDS #define EC_CMDS \ ... #endif Override the whole file instead of replacing part of file to prevent cut-and-paste error.
Re: [PATCH v4] platform/chrome: cros_ec_trace: update generating script
Hi Enric, I found it is error-prone to replace the EC_CMDS after updated. Perhaps, we should introduce an intermediate file "cros_ec_trace.inc". The generating script replaces whole ".inc" file every time and the cros_ec_trace.c includes the "cros_ec_trace.inc". If this proposal makes sense to you, I can send the patch after this change landed for*-next.
[PATCH v4] platform/chrome: cros_ec_trace: update generating script
The original script generates unneeded ", \" on the last line which
results in compile error if one would forget to remove them. Update the
script to not generate ", \" on the last line. Also add a define guard
for EC_CMDS.
Signed-off-by: Tzung-Bi Shih
---
Changes from v1:
- simpler awk code
Changes from v2:
- use c style comments instead of c++ style
- use '@' delimiter in sed instead of '/' to avoid unintentional end of
comment "*/"
Changes from v3:
- more detail commit message
- add define guard for EC_CMDS
drivers/platform/chrome/cros_ec_trace.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_trace.c
b/drivers/platform/chrome/cros_ec_trace.c
index 0a76412095a9..1412ae024435 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -5,8 +5,27 @@
#define TRACE_SYMBOL(a) {a, #a}
-// Generate the list using the following script:
-// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p'
include/linux/mfd/cros_ec_commands.h
+/*
+ * Generate the list using the following script:
+ * sed -n 's@^#define \(EC_CMD_[[:alnum:]_]*\)\s.*@\tTRACE_SYMBOL(\1), \\@p' \
+ * include/linux/mfd/cros_ec_commands.h | awk '
+ * BEGIN {
+ * print "#ifndef EC_CMDS";
+ * print "#define EC_CMDS \\";
+ * }
+ * {
+ * if (NR != 1)
+ * print buf;
+ * buf = $0;
+ * }
+ * END {
+ * gsub(/, \\/, "", buf);
+ * print buf;
+ * print "#endif";
+ * }
+ * '
+ */
+#ifndef EC_CMDS
#define EC_CMDS \
TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
TRACE_SYMBOL(EC_CMD_HELLO), \
@@ -119,6 +138,7 @@
TRACE_SYMBOL(EC_CMD_PD_CHARGE_PORT_OVERRIDE), \
TRACE_SYMBOL(EC_CMD_PD_GET_LOG_ENTRY), \
TRACE_SYMBOL(EC_CMD_USB_PD_MUX_INFO)
+#endif
#define CREATE_TRACE_POINTS
#include "cros_ec_trace.h"
--
2.22.0.709.g102302147b-goog
[PATCH v3] platform/chrome: cros_ec_trace: update generating script
To remove ", \" from the last line.
Signed-off-by: Tzung-Bi Shih
---
Changes from v1:
- simpler awk code
Changes from v2:
- use c style comments instead of c++ style
- use '@' delimiter in sed instead of '/' to avoid unintentional end of
comment "*/"
drivers/platform/chrome/cros_ec_trace.c | 17 +++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_trace.c
b/drivers/platform/chrome/cros_ec_trace.c
index 0a76412095a9..f6034b774f9a 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -5,8 +5,21 @@
#define TRACE_SYMBOL(a) {a, #a}
-// Generate the list using the following script:
-// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p'
include/linux/mfd/cros_ec_commands.h
+/*
+ * Generate the list using the following script:
+ * sed -n 's@^#define \(EC_CMD_[[:alnum:]_]*\)\s.*@\tTRACE_SYMBOL(\1), \\@p' \
+ * include/linux/mfd/cros_ec_commands.h | awk '
+ * {
+ * if (NR != 1)
+ * print buf;
+ * buf = $0;
+ * }
+ * END {
+ * gsub(/, \\/, "", buf);
+ * print buf;
+ * }
+ * '
+ */
#define EC_CMDS \
TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
TRACE_SYMBOL(EC_CMD_HELLO), \
--
2.22.0.709.g102302147b-goog
[PATCH v2] platform/chrome: cros_ec_trace: update generating script
To remove ", \" from the last line.
Signed-off-by: Tzung-Bi Shih
---
Changes from v1:
- simpler awk code
drivers/platform/chrome/cros_ec_trace.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_trace.c
b/drivers/platform/chrome/cros_ec_trace.c
index 0a76412095a9..667460952dad 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -6,7 +6,18 @@
#define TRACE_SYMBOL(a) {a, #a}
// Generate the list using the following script:
-// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p'
include/linux/mfd/cros_ec_commands.h
+// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' \
+// include/linux/mfd/cros_ec_commands.h | awk '
+// {
+// if (NR != 1)
+// print buf;
+// buf = $0;
+// }
+// END {
+// gsub(/, \\/, "", buf);
+// print buf;
+// }
+// '
#define EC_CMDS \
TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
TRACE_SYMBOL(EC_CMD_HELLO), \
--
2.22.0.657.g960e92d24f-goog
[PATCH] platform/chrome: cros_ec_trace: update generating script
To remove ", \" from the last line.
Signed-off-by: Tzung-Bi Shih
---
drivers/platform/chrome/cros_ec_trace.c | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_trace.c
b/drivers/platform/chrome/cros_ec_trace.c
index 0a76412095a9..9b6aba22441f 100644
--- a/drivers/platform/chrome/cros_ec_trace.c
+++ b/drivers/platform/chrome/cros_ec_trace.c
@@ -6,7 +6,23 @@
#define TRACE_SYMBOL(a) {a, #a}
// Generate the list using the following script:
-// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p'
include/linux/mfd/cros_ec_commands.h
+// sed -n 's/^#define \(EC_CMD_[[:alnum:]_]*\)\s.*/\tTRACE_SYMBOL(\1), \\/p' \
+// include/linux/mfd/cros_ec_commands.h | awk '
+// BEGIN {
+// on = 0;
+// }
+// {
+// if (on)
+// print buf;
+// else
+// on = 1;
+// buf = $0;
+// }
+// END {
+// gsub(/, \\/, "", buf);
+// print buf;
+// }
+// '
#define EC_CMDS \
TRACE_SYMBOL(EC_CMD_PROTO_VERSION), \
TRACE_SYMBOL(EC_CMD_HELLO), \
--
2.22.0.657.g960e92d24f-goog
Re: [PATCH] ASoC: codecs: ad193x: Use regmap_multi_reg_write() when initializing
On Wed, Jul 10, 2019 at 6:51 PM Codrin Ciubotariu wrote: > > Using regmap_multi_reg_write() when we set the default values for our > registers makes the code smaller and easier to read. > > Suggested-by: Tzung-Bi Shih > Signed-off-by: Codrin Ciubotariu Reviewed-by: Tzung-Bi Shih LGTM. Thanks.
Re: linux-next: Fixes tag needs some work in the sound-asoc tree
On Thu, Jul 11, 2019 at 12:13 AM Stephen Rothwell wrote:
> In commit
>
> 6cd249cfad68 ("ASoC: max98357a: use mdelay for sdmode-delay")
>
> Fixes tag
>
> Fixes: cec5b01f8f1c ("ASoC: max98357a: avoid speaker pop when playback
>
> has these problem(s):
>
> - Subject has leading but no trailing parentheses
> - Subject has leading but no trailing quotes
>
> Please do not split Fixes tags over more than one line. Also, don't
> include blank lines among the tags.
I am sorry for this fault.
Mark, do you need me to resend the patch to fix the commit message?
Re: [PATCH] sound: soc: codecs: mt6358: change return type of mt6358_codec_init_reg
On Wed, Jul 10, 2019 at 2:25 AM Hariprasad Kelam wrote: > > As mt6358_codec_init_reg function always returns 0 , change return type > from int to void. > > fixes below issue reported by coccicheck > sound/soc/codecs/mt6358.c:2260:5-8: Unneeded variable: "ret". Return "0" > on line 2289 > > Signed-off-by: Hariprasad Kelam Acked-by: Tzung-Bi Shih Thanks for cleaning this up.
Re: [alsa-devel] [PATCH 1/2] ASoC: codecs: ad193x: Group register initialization at probe
On Thu, Jun 27, 2019 at 8:05 PM Codrin Ciubotariu
wrote:
> +struct ad193x_reg_default {
> + unsigned int reg;
> + unsigned int val;
> +};
You probably don't need to define this. There is a struct
reg_sequence in regmap.h.
> +
> +/* codec register values to set after reset */
> +static void ad193x_reg_default_init(struct ad193x_priv *ad193x)
> +{
> + const struct ad193x_reg_default reg_init[] = {
> + { 0, 0x99 }, /* PLL_CLK_CTRL0: pll input: mclki/xi
> 12.288Mhz */
> + { 1, 0x04 }, /* PLL_CLK_CTRL1: no on-chip Vref */
> + { 2, 0x40 }, /* DAC_CTRL0: TDM mode */
> + { 4, 0x1A }, /* DAC_CTRL2: 48kHz de-emphasis, unmute dac */
> + { 5, 0x00 }, /* DAC_CHNL_MUTE: unmute DAC channels */
> + };
> + const struct ad193x_reg_default reg_adc_init[] = {
> + { 14, 0x03 }, /* ADC_CTRL0: high-pass filter enable */
> + { 15, 0x43 }, /* ADC_CTRL1: sata delay=1, adc aux mode */
> + };
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(reg_init); i++)
> + regmap_write(ad193x->regmap, reg_init[i].reg,
> reg_init[i].val);
> +
> + if (ad193x_has_adc(ad193x)) {
> + for (i = 0; i < ARRAY_SIZE(reg_adc_init); i++) {
> + regmap_write(ad193x->regmap, reg_adc_init[i].reg,
> +reg_adc_init[i].val);
> + }
> + }
And you could use regmap_multi_reg_write( ) to substitute the two for-loops.
See https://mailman.alsa-project.org/pipermail/alsa-devel/2019-June/151090.html
as an example. It also has some reg initializations in component
probe( ).
Re: [BISECT] No audio after "ASoC: core: use component driver name as component name"
On Fri, May 31, 2019 at 5:27 PM Krzysztof Kozlowski wrote: > The problem might be in component name. The driver->name and > fmt_single_name(dev, &component->id) are: > snd_dmaengine_pcm != 383.i2s > snd_dmaengine_pcm != 383.i2s-sec > samsung-i2s != 383.i2s > > This commit should not go in without fixing the users of old > behavior... I could adjust the platform names for primary and > secondary links... but now it looks like two components will have the > same name. That is because the two component drivers used the same name in somehow. But yes, we should not have the commit without fixing potential errors for users depend on old behavior. Could you send a patch to revert the commit b19671d6caf1?
